Re: [PATCH 0/2] media: ov5640: drive-by frame_interval cleanups

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

[-- Attachment #1: Type: text/plain, Size: 3097 bytes --]

Hi Jai,
   thanks for testing

On Mon, May 15, 2023 at 05:25:55PM +0530, Jai Luthra wrote:
> Hi Jacopo, Guoniu,
>
> On 05/05/23 12:46, Jacopo Mondi wrote:
> > While looking at Guoniu Zhou patches I noticed that there were a few cleanups
> > related to the usage of frame_interval fileds for MIPI CSI-2 framerate
> > calculations.
> >
> > No functional changes intended, just cleanups.
> >
> > Guoniu: could you please test these on your setup as well ? A tested-by tag
> > would be useful!
> >
>
> Thanks for the latest fixes!
>
> Testing on my setup (CSI module w/ 2 lanes), I notice two weird issues and
> wonder if you see the same behavior on your setups?
>
> Issue 1
> -------
>
> On a fresh boot the sensor streams at 60fps, and checking link_freq from
> v4l2-ctl I get 384Mhz. But G_FRAME_INTERVAL returns 30FPS when using
> `media-ctl -p`:
> [stream:0 fmt:UYVY8_1X16/640x480@1/30]

the g/s_frame_interval calls are not relevant for MIPI CSI-2

I wonder if we should/could return -EINVAL in this case


>
> Issue 2
> -------
>
> If I manually set the frame interval to @1/60 using media-ctl, and then
> stream it - actual framerate gets reduced to 30FPS:

Ah this shouldn't happen. s_frame_interval -should not- modify the
timings on a CSI-2 setup

If not returning -EINVAL, we should at least return immediately

>
> root@am62xx-evm:~# yavta -s 640x480 -f UYVY /dev/video0 -c5
> ....
> 0 (0) [-] any 0 614400 B 401.488754 401.488855 12.719 fps ts mono/EoF
> 1 (1) [-] any 1 614400 B 401.522057 401.522147 30.027 fps ts mono/EoF
> 2 (2) [-] any 2 614400 B 401.555434 401.555584 29.961 fps ts mono/EoF
> 3 (3) [-] any 3 614400 B 401.588723 401.588814 30.040 fps ts mono/EoF
> 4 (4) [-] any 4 614400 B 401.622051 401.622135 30.005 fps ts mono/EoF
> Captured 5 frames in 0.212005 seconds (23.584252 fps, 14490164.140730 B/s).
> 8 buffers released.
>
> After setting frame interval to @1/60, the link-frequency got reduced to
> 192Mhz, which probably explains the low framerate.
>
> root@am62xx-evm:~# v4l2-ctl -d /dev/v4l-subdev2 -C link_frequency
> link_frequency: 19 (192000000 0xb71b000)
>
> I will take a deeper look at update_pixel_rate() function to try and fix
> this - but wanted to confirm if this also happens on your CSI sensors?
>
> I also repeated same tests without this series and still saw both issues. In
> fact Issue 2 was worse because the sensor did not stream *at all* if I
> changed frame interval to @1/60. My guess is PATCH 2/2 fixes that by not
> updating the VBLANK using the DVP values.

Probably yes, and this confirms to me that we should return early in
s_frame_interval if we're CSI-2 (or if this doesn't contradict the
specification even return an error).

Thanks
   j

>
> For the series:
>
> Tested-by: Jai Luthra <j-luthra@ti.com>
>
> Thanks,
> Jai
>
> > Thanks
> >    j
> >
> > Jacopo Mondi (2):
> >    media: ov5640: Remove unused 'framerate' parameter
> >    media: ov5640: Drop dead code using frame_interval
> >
> >   drivers/media/i2c/ov5640.c | 17 +----------------
> >   1 file changed, 1 insertion(+), 16 deletions(-)
> >
> > --
> > 2.40.1
> >






[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]