mbox series

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

Message ID 20230505071619.63229-1-jacopo.mondi@ideasonboard.com
Headers show
Series media: ov5640: drive-by frame_interval cleanups | expand

Message

Jacopo Mondi May 5, 2023, 7:16 a.m. UTC
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
  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

Comments

G.N. Zhou May 11, 2023, 9:14 a.m. UTC | #1
I tested the patch on NXP iMX8MP platform and no issues found.

Test-by: Guoniu.zhou <guoniu.zhou@nxp.com>

> -----Original Message-----
> From: G.N. Zhou
> Sent: 2023年5月5日 16:03
> To: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Cc: slongerbeam@gmail.com; linux-media@vger.kernel.org;
> mchehab@kernel.org
> Subject: RE: [EXT] [PATCH 0/2] media: ov5640: drive-by frame_interval cleanups
> 
> Sure, will update later.
> 
> > -----Original Message-----
> > From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Sent: 2023年5月5日 15:16
> > To: G.N. Zhou <guoniu.zhou@nxp.com>
> > Cc: Jacopo Mondi <jacopo.mondi@ideasonboard.com>;
> > slongerbeam@gmail.com; linux-media@vger.kernel.org;
> mchehab@kernel.org
> > Subject: [EXT] [PATCH 0/2] media: ov5640: drive-by frame_interval
> > cleanups
> >
> > Caution: This is an external email. Please take care when clicking
> > links or opening attachments. When in doubt, report the message using the
> 'Report this email'
> > button
> >
> >
> > 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
> >   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
Jacopo Mondi May 11, 2023, 10:51 a.m. UTC | #2
Hello

On Thu, May 11, 2023 at 09:14:11AM +0000, G.N. Zhou wrote:
> I tested the patch on NXP iMX8MP platform and no issues found.
>

Thanks for testing

> Test-by: Guoniu.zhou <guoniu.zhou@nxp.com>

FYI the tag is meant to be

Tested-by:

Thanks for testing

>
> > -----Original Message-----
> > From: G.N. Zhou
> > Sent: 2023年5月5日 16:03
> > To: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Cc: slongerbeam@gmail.com; linux-media@vger.kernel.org;
> > mchehab@kernel.org
> > Subject: RE: [EXT] [PATCH 0/2] media: ov5640: drive-by frame_interval cleanups
> >
> > Sure, will update later.
> >
> > > -----Original Message-----
> > > From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > Sent: 2023年5月5日 15:16
> > > To: G.N. Zhou <guoniu.zhou@nxp.com>
> > > Cc: Jacopo Mondi <jacopo.mondi@ideasonboard.com>;
> > > slongerbeam@gmail.com; linux-media@vger.kernel.org;
> > mchehab@kernel.org
> > > Subject: [EXT] [PATCH 0/2] media: ov5640: drive-by frame_interval
> > > cleanups
> > >
> > > Caution: This is an external email. Please take care when clicking
> > > links or opening attachments. When in doubt, report the message using the
> > 'Report this email'
> > > button
> > >
> > >
> > > 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
> > >   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
>
Jai Luthra May 15, 2023, 11:55 a.m. UTC | #3
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]

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:

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.

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
>