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

[Hugues FRUCHET <hugues.fruchet@st.com>]

Hi Maxime,

On 10/04/2018 05:04 PM, Maxime Ripard wrote:
> Hi!
> 
> On Mon, Oct 01, 2018 at 02:12:31PM +0000, 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.