Re: [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements

[Hugues FRUCHET]

Hi Maxime,

On 10/04/2018 05:04 PM, Maxime Ripard wrote:
> Hi!
> 
> On Mon, Oct 01, 2018 at 02:12:31PM +, Hugues FRUCHET wrote:
 This is working perfectly fine on my parallel setup and allows me to
 well support VGA@30fps (instead 27) and also support XGA(1024x768)@15fps
 that I never seen working before.
 So at least for the parallel setup, this serie is working fine for all
 the discrete resolutions and framerate exposed by the driver for the 
 moment:
 * QCIF 176x144 15/30fps
 * QVGA 320x240 15/30fps
 * VGA 640x480 15/30fps
 * 480p 720x480 15/30fps
 * XGA 1024x768 15/30fps
 * 720p 1280x720 15/30fps
 * 1080p 1920x1080 15/30fps
 * 5Mp 2592x1944 15fps
>>>
>>> I'm glad this is working for you as well. I guess I'll resubmit these
>>> patches, but this time making sure someone with a CSI setup tests
>>> before merging. I crtainly don't want to repeat the previous disaster.
>>>
>>> Do you have those patches rebased somewhere? I'm not quite sure how to
>>> fix the conflict with the v4l2_find_nearest_size addition.
>>>
 Moreover I'm not clear on relationship between rate and pixel clock
 frequency.
 I've understood that to DVP_PCLK_DIVIDER (0x3824) register
 and VFIFO_CTRL0C (0x460c) affects the effective pixel clock frequency.
 All the resolutions up to 720x576 are forcing a manual value of 2 for
 divider (0x460c=0x22), but including 720p and more, the divider value is
 controlled by "auto-mode" (0x460c=0x20)... from what I measured and
 understood, for those resolutions, the divider must be set to 1 in order
 that your rate computation match the effective pixel clock on output,
 see [2].

 So I wonder if this PCLK divider register should be included
 or not into your rate computation, what do you think ?
>>>
>>> Have you tried change the PCLK divider while in auto-mode? IIRC, I did
>>> that and it was affecting the PCLK rate on my scope, but I wouldn't be
>>> definitive about it.
>>
>> I have tested to change PCLK divider while in auto mode but no effect.
>>
>>> Can we always set the mode to auto and divider to 1, even for the
>>> lower resolutions?
>>
>> This is breaking 176x144@30fps on my side, because of pixel clock too
>> high (112MHz vs 70 MHz max).
> 
> Ok.
> 
>> Instead of using auto mode, my proposal was the inverse: use manual mode
>> with the proper divider to fit the max pixel clock constraint.
> 
> Oh. That would work for me too yeah. How do you want to deal with it?
> Should I send your rebased patches, and you add that change as a
> subsequent patch?

Yes, this is the best option, and we can then ask people having CSI 
setup to check for non-regression after having applied this important 
clock serie patch.
Hoping that this will also work on their setup so that we can move 
forward on next OV5640 improvements.

> 
> Thanks!
> Maxime
> 

BR,
Hugues.

Re: [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements

[Maxime Ripard]

Hi!

On Mon, Oct 01, 2018 at 02:12:31PM +, Hugues FRUCHET wrote:
> >> This is working perfectly fine on my parallel setup and allows me to
> >> well support VGA@30fps (instead 27) and also support XGA(1024x768)@15fps
> >> that I never seen working before.
> >> So at least for the parallel setup, this serie is working fine for all
> >> the discrete resolutions and framerate exposed by the driver for the 
> >> moment:
> >> * QCIF 176x144 15/30fps
> >> * QVGA 320x240 15/30fps
> >> * VGA 640x480 15/30fps
> >> * 480p 720x480 15/30fps
> >> * XGA 1024x768 15/30fps
> >> * 720p 1280x720 15/30fps
> >> * 1080p 1920x1080 15/30fps
> >> * 5Mp 2592x1944 15fps
> > 
> > I'm glad this is working for you as well. I guess I'll resubmit these
> > patches, but this time making sure someone with a CSI setup tests
> > before merging. I crtainly don't want to repeat the previous disaster.
> > 
> > Do you have those patches rebased somewhere? I'm not quite sure how to
> > fix the conflict with the v4l2_find_nearest_size addition.
> > 
> >> Moreover I'm not clear on relationship between rate and pixel clock
> >> frequency.
> >> I've understood that to DVP_PCLK_DIVIDER (0x3824) register
> >> and VFIFO_CTRL0C (0x460c) affects the effective pixel clock frequency.
> >> All the resolutions up to 720x576 are forcing a manual value of 2 for
> >> divider (0x460c=0x22), but including 720p and more, the divider value is
> >> controlled by "auto-mode" (0x460c=0x20)... from what I measured and
> >> understood, for those resolutions, the divider must be set to 1 in order
> >> that your rate computation match the effective pixel clock on output,
> >> see [2].
> >>
> >> So I wonder if this PCLK divider register should be included
> >> or not into your rate computation, what do you think ?
> > 
> > Have you tried change the PCLK divider while in auto-mode? IIRC, I did
> > that and it was affecting the PCLK rate on my scope, but I wouldn't be
> > definitive about it.
> 
> I have tested to change PCLK divider while in auto mode but no effect.
> 
> > Can we always set the mode to auto and divider to 1, even for the
> > lower resolutions?
>
> This is breaking 176x144@30fps on my side, because of pixel clock too 
> high (112MHz vs 70 MHz max).

Ok.

> Instead of using auto mode, my proposal was the inverse: use manual mode 
> with the proper divider to fit the max pixel clock constraint.

Oh. That would work for me too yeah. How do you want to deal with it?
Should I send your rebased patches, and you add that change as a
subsequent patch?

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements

[Hugues FRUCHET]

Hi Maxime,

On 09/28/2018 06:05 PM, Maxime Ripard wrote:
> Hi Hugues,
> 
> On Thu, Sep 27, 2018 at 03:59:04PM +, Hugues FRUCHET wrote:
>> Hi Maxime & all OV5640 stakeholders,
>>
>> I've just pushed a new patchset also related to rate/pixel clock
>> handling [1], based on your V3 great work:
>>   >media: ov5640: Adjust the clock based on the expected rate
>>   >media: ov5640: Remove the clocks registers initialization
>>   >media: ov5640: Remove redundant defines
>>   >media: ov5640: Remove redundant register setup
>>   >media: ov5640: Compute the clock rate at runtime
>>   >media: ov5640: Remove pixel clock rates
>>   >media: ov5640: Enhance FPS handling
>>
>> This is working perfectly fine on my parallel setup and allows me to
>> well support VGA@30fps (instead 27) and also support XGA(1024x768)@15fps
>> that I never seen working before.
>> So at least for the parallel setup, this serie is working fine for all
>> the discrete resolutions and framerate exposed by the driver for the moment:
>> * QCIF 176x144 15/30fps
>> * QVGA 320x240 15/30fps
>> * VGA 640x480 15/30fps
>> * 480p 720x480 15/30fps
>> * XGA 1024x768 15/30fps
>> * 720p 1280x720 15/30fps
>> * 1080p 1920x1080 15/30fps
>> * 5Mp 2592x1944 15fps
> 
> I'm glad this is working for you as well. I guess I'll resubmit these
> patches, but this time making sure someone with a CSI setup tests
> before merging. I crtainly don't want to repeat the previous disaster.
> 
> Do you have those patches rebased somewhere? I'm not quite sure how to
> fix the conflict with the v4l2_find_nearest_size addition.
> 
>> Moreover I'm not clear on relationship between rate and pixel clock
>> frequency.
>> I've understood that to DVP_PCLK_DIVIDER (0x3824) register
>> and VFIFO_CTRL0C (0x460c) affects the effective pixel clock frequency.
>> All the resolutions up to 720x576 are forcing a manual value of 2 for
>> divider (0x460c=0x22), but including 720p and more, the divider value is
>> controlled by "auto-mode" (0x460c=0x20)... from what I measured and
>> understood, for those resolutions, the divider must be set to 1 in order
>> that your rate computation match the effective pixel clock on output,
>> see [2].
>>
>> So I wonder if this PCLK divider register should be included
>> or not into your rate computation, what do you think ?
> 
> Have you tried change the PCLK divider while in auto-mode? IIRC, I did
> that and it was affecting the PCLK rate on my scope, but I wouldn't be
> definitive about it.

I have tested to change PCLK divider while in auto mode but no effect.

> 
> Can we always set the mode to auto and divider to 1, even for the
> lower resolutions?
This is breaking 176x144@30fps on my side, because of pixel clock too 
high (112MHz vs 70 MHz max).

Instead of using auto mode, my proposal was the inverse: use manual mode 
with the proper divider to fit the max pixel clock constraint.


BR,
Hugues.

Re: [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements

[Maxime Ripard]

Hi Hugues,

On Thu, Sep 27, 2018 at 03:59:04PM +, Hugues FRUCHET wrote:
> Hi Maxime & all OV5640 stakeholders,
> 
> I've just pushed a new patchset also related to rate/pixel clock 
> handling [1], based on your V3 great work:
>  >media: ov5640: Adjust the clock based on the expected rate
>  >media: ov5640: Remove the clocks registers initialization
>  >media: ov5640: Remove redundant defines
>  >media: ov5640: Remove redundant register setup
>  >media: ov5640: Compute the clock rate at runtime
>  >media: ov5640: Remove pixel clock rates
>  >media: ov5640: Enhance FPS handling
> 
> This is working perfectly fine on my parallel setup and allows me to 
> well support VGA@30fps (instead 27) and also support XGA(1024x768)@15fps 
> that I never seen working before.
> So at least for the parallel setup, this serie is working fine for all 
> the discrete resolutions and framerate exposed by the driver for the moment:
> * QCIF 176x144 15/30fps
> * QVGA 320x240 15/30fps
> * VGA 640x480 15/30fps
> * 480p 720x480 15/30fps
> * XGA 1024x768 15/30fps
> * 720p 1280x720 15/30fps
> * 1080p 1920x1080 15/30fps
> * 5Mp 2592x1944 15fps

I'm glad this is working for you as well. I guess I'll resubmit these
patches, but this time making sure someone with a CSI setup tests
before merging. I crtainly don't want to repeat the previous disaster.

Do you have those patches rebased somewhere? I'm not quite sure how to
fix the conflict with the v4l2_find_nearest_size addition.

> Moreover I'm not clear on relationship between rate and pixel clock 
> frequency.
> I've understood that to DVP_PCLK_DIVIDER (0x3824) register
> and VFIFO_CTRL0C (0x460c) affects the effective pixel clock frequency.
> All the resolutions up to 720x576 are forcing a manual value of 2 for 
> divider (0x460c=0x22), but including 720p and more, the divider value is 
> controlled by "auto-mode" (0x460c=0x20)... from what I measured and
> understood, for those resolutions, the divider must be set to 1 in order 
> that your rate computation match the effective pixel clock on output, 
> see [2].
> 
> So I wonder if this PCLK divider register should be included
> or not into your rate computation, what do you think ?

Have you tried change the PCLK divider while in auto-mode? IIRC, I did
that and it was affecting the PCLK rate on my scope, but I wouldn't be
definitive about it.

Can we always set the mode to auto and divider to 1, even for the
lower resolutions?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements

[Hugues FRUCHET]

Hi Maxime & all OV5640 stakeholders,

I've just pushed a new patchset also related to rate/pixel clock 
handling [1], based on your V3 great work:
 >media: ov5640: Adjust the clock based on the expected rate
 >media: ov5640: Remove the clocks registers initialization
 >media: ov5640: Remove redundant defines
 >media: ov5640: Remove redundant register setup
 >media: ov5640: Compute the clock rate at runtime
 >media: ov5640: Remove pixel clock rates
 >media: ov5640: Enhance FPS handling

This is working perfectly fine on my parallel setup and allows me to 
well support VGA@30fps (instead 27) and also support XGA(1024x768)@15fps 
that I never seen working before.
So at least for the parallel setup, this serie is working fine for all 
the discrete resolutions and framerate exposed by the driver for the moment:
* QCIF 176x144 15/30fps
* QVGA 320x240 15/30fps
* VGA 640x480 15/30fps
* 480p 720x480 15/30fps
* XGA 1024x768 15/30fps
* 720p 1280x720 15/30fps
* 1080p 1920x1080 15/30fps
* 5Mp 2592x1944 15fps

Moreover I'm not clear on relationship between rate and pixel clock 
frequency.
I've understood that to DVP_PCLK_DIVIDER (0x3824) register
and VFIFO_CTRL0C (0x460c) affects the effective pixel clock frequency.
All the resolutions up to 720x576 are forcing a manual value of 2 for 
divider (0x460c=0x22), but including 720p and more, the divider value is 
controlled by "auto-mode" (0x460c=0x20)... from what I measured and
understood, for those resolutions, the divider must be set to 1 in order 
that your rate computation match the effective pixel clock on output, 
see [2].

So I wonder if this PCLK divider register should be included
or not into your rate computation, what do you think ?


[1] OV5640: reduce rate according to maximum pixel clock 
https://www.spinics.net/lists/linux-media/msg140958.html:
[2] media: ov5640: move parallel port pixel clock divider out of 
registers set https://www.spinics.net/lists/linux-media/msg140960.html

BR,
Hugues.

On 05/17/2018 10:53 AM, Maxime Ripard wrote:
> Hi,
> 
> Here is a "small" series that mostly cleans up the ov5640 driver code,
> slowly getting rid of the big data array for more understandable code
> (hopefully).
> 
> The biggest addition would be the clock rate computation at runtime,
> instead of relying on those arrays to setup the clock tree
> properly. As a side effect, it fixes the framerate that was off by
> around 10% on the smaller resolutions, and we now support 60fps.
> 
> This also introduces a bunch of new features.
> 
> Let me know what you think,
> Maxime
> 
> Changes from v2:
>- Rebased on latest Sakari PR
>- Fixed the issues reported by Hugues: improper FPS returned for
>  formats, improper rounding of the FPS, some with his suggestions,
>  some by simplifying the logic.
>- Expanded the clock tree comments based on the feedback from Samuel
>  Bobrowicz and Loic Poulain
>- Merged some of the changes made by Samuel Bobrowicz to fix the
>  MIPI rate computation, fix the call sites of the
>  ov5640_set_timings function, the auto-exposure calculation call,
>  etc.
>- Split the patches into smaller ones in order to make it more
>  readable (hopefully)
> 
> Changes from v1:
>- Integrated Hugues' suggestions to fix v4l2-compliance
>- Fixed the bus width with JPEG
>- Dropped the clock rate calculation loops for something simpler as
>  suggested by Sakari
>- Cache the exposure value instead of using the control value
>- Rebased on top of 4.17
> 
> Maxime Ripard (11):
>media: ov5640: Adjust the clock based on the expected rate
>media: ov5640: Remove the clocks registers initialization
>media: ov5640: Remove redundant defines
>media: ov5640: Remove redundant register setup
>media: ov5640: Compute the clock rate at runtime
>media: ov5640: Remove pixel clock rates
>media: ov5640: Enhance FPS handling
>media: ov5640: Make the return rate type more explicit
>media: ov5640: Make the FPS clamping / rounding more extendable
>media: ov5640: Add 60 fps support
>media: ov5640: Remove duplicate auto-exposure setup
> 
> Samuel Bobrowicz (1):
>media: ov5640: Fix timings setup code
> 
>   drivers/media/i2c/ov5640.c | 701 +
>   1 file changed, 392 insertions(+), 309 deletions(-)
> 

Re: [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements

[Sakari Ailus]

On Thu, May 17, 2018 at 01:22:07PM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Thu, May 17, 2018 at 11:24:04AM +0200, Daniel Mack wrote:
> > Hi,
> > 
> > On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:
> > > Here is a "small" series that mostly cleans up the ov5640 driver code,
> > > slowly getting rid of the big data array for more understandable code
> > > (hopefully).
> > > 
> > > The biggest addition would be the clock rate computation at runtime,
> > > instead of relying on those arrays to setup the clock tree
> > > properly. As a side effect, it fixes the framerate that was off by
> > > around 10% on the smaller resolutions, and we now support 60fps.
> > > 
> > > This also introduces a bunch of new features.
> > 
> > I'd like to give this a try. What tree should this patch set be applied on?
> > I had no luck with media_tree/for-4.18-6.
> 
> Maybe it wasn't the latest after all, sorry. It's based on Sakari's
> for-4.18-3 PR (67f76c65e94f).

I've pushed these here:



-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements

[Maxime Ripard]

Hi,

On Thu, May 17, 2018 at 11:24:04AM +0200, Daniel Mack wrote:
> Hi,
> 
> On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:
> > Here is a "small" series that mostly cleans up the ov5640 driver code,
> > slowly getting rid of the big data array for more understandable code
> > (hopefully).
> > 
> > The biggest addition would be the clock rate computation at runtime,
> > instead of relying on those arrays to setup the clock tree
> > properly. As a side effect, it fixes the framerate that was off by
> > around 10% on the smaller resolutions, and we now support 60fps.
> > 
> > This also introduces a bunch of new features.
> 
> I'd like to give this a try. What tree should this patch set be applied on?
> I had no luck with media_tree/for-4.18-6.

Maybe it wasn't the latest after all, sorry. It's based on Sakari's
for-4.18-3 PR (67f76c65e94f).

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements

[Daniel Mack]

Hi,

On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:

Here is a "small" series that mostly cleans up the ov5640 driver code,
slowly getting rid of the big data array for more understandable code
(hopefully).

The biggest addition would be the clock rate computation at runtime,
instead of relying on those arrays to setup the clock tree
properly. As a side effect, it fixes the framerate that was off by
around 10% on the smaller resolutions, and we now support 60fps.

This also introduces a bunch of new features.


I'd like to give this a try. What tree should this patch set be applied 
on? I had no luck with media_tree/for-4.18-6.


Thanks,
Daniel




Let me know what you think,
Maxime

Changes from v2:
   - Rebased on latest Sakari PR
   - Fixed the issues reported by Hugues: improper FPS returned for
 formats, improper rounding of the FPS, some with his suggestions,
 some by simplifying the logic.
   - Expanded the clock tree comments based on the feedback from Samuel
 Bobrowicz and Loic Poulain
   - Merged some of the changes made by Samuel Bobrowicz to fix the
 MIPI rate computation, fix the call sites of the
 ov5640_set_timings function, the auto-exposure calculation call,
 etc.
   - Split the patches into smaller ones in order to make it more
 readable (hopefully)

Changes from v1:
   - Integrated Hugues' suggestions to fix v4l2-compliance
   - Fixed the bus width with JPEG
   - Dropped the clock rate calculation loops for something simpler as
 suggested by Sakari
   - Cache the exposure value instead of using the control value
   - Rebased on top of 4.17

Maxime Ripard (11):
   media: ov5640: Adjust the clock based on the expected rate
   media: ov5640: Remove the clocks registers initialization
   media: ov5640: Remove redundant defines
   media: ov5640: Remove redundant register setup
   media: ov5640: Compute the clock rate at runtime
   media: ov5640: Remove pixel clock rates
   media: ov5640: Enhance FPS handling
   media: ov5640: Make the return rate type more explicit
   media: ov5640: Make the FPS clamping / rounding more extendable
   media: ov5640: Add 60 fps support
   media: ov5640: Remove duplicate auto-exposure setup

Samuel Bobrowicz (1):
   media: ov5640: Fix timings setup code

  drivers/media/i2c/ov5640.c | 701 +
  1 file changed, 392 insertions(+), 309 deletions(-)

[PATCH v3 00/12] media: ov5640: Misc cleanup and improvements

[Maxime Ripard]

Hi,

Here is a "small" series that mostly cleans up the ov5640 driver code,
slowly getting rid of the big data array for more understandable code
(hopefully).

The biggest addition would be the clock rate computation at runtime,
instead of relying on those arrays to setup the clock tree
properly. As a side effect, it fixes the framerate that was off by
around 10% on the smaller resolutions, and we now support 60fps.

This also introduces a bunch of new features.

Let me know what you think,
Maxime

Changes from v2:
  - Rebased on latest Sakari PR
  - Fixed the issues reported by Hugues: improper FPS returned for
formats, improper rounding of the FPS, some with his suggestions,
some by simplifying the logic.
  - Expanded the clock tree comments based on the feedback from Samuel
Bobrowicz and Loic Poulain
  - Merged some of the changes made by Samuel Bobrowicz to fix the
MIPI rate computation, fix the call sites of the
ov5640_set_timings function, the auto-exposure calculation call,
etc.
  - Split the patches into smaller ones in order to make it more
readable (hopefully)

Changes from v1:
  - Integrated Hugues' suggestions to fix v4l2-compliance
  - Fixed the bus width with JPEG
  - Dropped the clock rate calculation loops for something simpler as
suggested by Sakari
  - Cache the exposure value instead of using the control value
  - Rebased on top of 4.17

Maxime Ripard (11):
  media: ov5640: Adjust the clock based on the expected rate
  media: ov5640: Remove the clocks registers initialization
  media: ov5640: Remove redundant defines
  media: ov5640: Remove redundant register setup
  media: ov5640: Compute the clock rate at runtime
  media: ov5640: Remove pixel clock rates
  media: ov5640: Enhance FPS handling
  media: ov5640: Make the return rate type more explicit
  media: ov5640: Make the FPS clamping / rounding more extendable
  media: ov5640: Add 60 fps support
  media: ov5640: Remove duplicate auto-exposure setup

Samuel Bobrowicz (1):
  media: ov5640: Fix timings setup code

 drivers/media/i2c/ov5640.c | 701 +
 1 file changed, 392 insertions(+), 309 deletions(-)

-- 
2.17.0