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

  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
  

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
  

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
  

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
>
  

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
>
  

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
> >
  

Hi Jacopo,

On May 16, 2023 at 09:46:57 +0200, Jacopo Mondi wrote:
> 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).

Oh makes sense, thanks.

I can update s_frame_interval to return -EINVAL for CSI-2 as a follow-up 
series.

Will also try if g_frame_interval can report correct framerate (60fps vs 
30fps) depending upon the bus type, as I don't think returning -EINVAL 
would be correct behavior. If that does not work maybe we can unset the 
ops hooks before registering the subdev with the framework.

> 
> 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
> > >
> 
> 
> 
> 
>
  

Hi Jai

On Wed, May 17, 2023 at 01:29:13PM +0530, Jai Luthra wrote:
> Hi Jacopo,
>
> On May 16, 2023 at 09:46:57 +0200, Jacopo Mondi wrote:
> > 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).
>
> Oh makes sense, thanks.
>
> I can update s_frame_interval to return -EINVAL for CSI-2 as a follow-up
> series.
>
> Will also try if g_frame_interval can report correct framerate (60fps vs
> 30fps) depending upon the bus type, as I don't think returning -EINVAL
> would be correct behavior. If that does not work maybe we can unset the
> ops hooks before registering the subdev with the framework.
>

I would rather considering disabling the whole s/g/enum_frame_interval
operations in CSI-2 mode.

The specification report as an accepted error code

EINVAL
The struct v4l2_subdev_frame_interval pad references a non-existing
pad, or the pad doesn't support frame intervals.

https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.html

Hans Sakari and Laurent in cc: would it be acceptable to disable the
frame_interval operations completely when the sensor is used in MIPI
CSI-2 mode and only allow them in parallel mode ? Is returning -EINVAL
acceptable in that case ?

> >
> > 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
> > > >
> >
> >
> >
> >
> >
>
>
  

Hi Jacopo,

On Wed, May 17, 2023 at 11:05:08AM +0200, Jacopo Mondi wrote:
> Hi Jai
> 
> On Wed, May 17, 2023 at 01:29:13PM +0530, Jai Luthra wrote:
> > Hi Jacopo,
> >
> > On May 16, 2023 at 09:46:57 +0200, Jacopo Mondi wrote:
> > > 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).
> >
> > Oh makes sense, thanks.
> >
> > I can update s_frame_interval to return -EINVAL for CSI-2 as a follow-up
> > series.
> >
> > Will also try if g_frame_interval can report correct framerate (60fps vs
> > 30fps) depending upon the bus type, as I don't think returning -EINVAL
> > would be correct behavior. If that does not work maybe we can unset the
> > ops hooks before registering the subdev with the framework.
> >
> 
> I would rather considering disabling the whole s/g/enum_frame_interval
> operations in CSI-2 mode.
> 
> The specification report as an accepted error code
> 
> EINVAL
> The struct v4l2_subdev_frame_interval pad references a non-existing
> pad, or the pad doesn't support frame intervals.
> 
> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.html
> 
> Hans Sakari and Laurent in cc: would it be acceptable to disable the
> frame_interval operations completely when the sensor is used in MIPI
> CSI-2 mode and only allow them in parallel mode ? Is returning -EINVAL
> acceptable in that case ?

I don't think the bus *should* really make a difference. The driver has
supported s_frame_interval in the past and drop that support completely
likely would cause issues.

Although --- if in this case supporting s_frame_interval just for parallel
really helps keeping the driver somehow sane and doesn't break anything, I
have no objections.

-EINVAL works for me.
  

On Wed, May 24, 2023 at 12:21:52PM +0000, Sakari Ailus wrote:
> On Wed, May 17, 2023 at 11:05:08AM +0200, Jacopo Mondi wrote:
> > On Wed, May 17, 2023 at 01:29:13PM +0530, Jai Luthra wrote:
> > > On May 16, 2023 at 09:46:57 +0200, Jacopo Mondi wrote:
> > > > On Mon, May 15, 2023 at 05:25:55PM +0530, Jai Luthra wrote:
> > > > > 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).
> > >
> > > Oh makes sense, thanks.
> > >
> > > I can update s_frame_interval to return -EINVAL for CSI-2 as a follow-up
> > > series.
> > >
> > > Will also try if g_frame_interval can report correct framerate (60fps vs
> > > 30fps) depending upon the bus type, as I don't think returning -EINVAL
> > > would be correct behavior. If that does not work maybe we can unset the
> > > ops hooks before registering the subdev with the framework.
> > 
> > I would rather considering disabling the whole s/g/enum_frame_interval
> > operations in CSI-2 mode.
> > 
> > The specification report as an accepted error code
> > 
> > EINVAL
> > The struct v4l2_subdev_frame_interval pad references a non-existing
> > pad, or the pad doesn't support frame intervals.
> > 
> > https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.html
> > 
> > Hans Sakari and Laurent in cc: would it be acceptable to disable the
> > frame_interval operations completely when the sensor is used in MIPI
> > CSI-2 mode and only allow them in parallel mode ? Is returning -EINVAL
> > acceptable in that case ?
> 
> I don't think the bus *should* really make a difference. The driver has
> supported s_frame_interval in the past and drop that support completely
> likely would cause issues.
> 
> Although --- if in this case supporting s_frame_interval just for parallel
> really helps keeping the driver somehow sane and doesn't break anything, I
> have no objections.
> 
> -EINVAL works for me.

I'd rather drop it in both cases, but for parallel mode I believe it
could cause regressions.
  

Hi Laurent, Jacopo,

On May 24, 2023 at 16:06:24 +0300, Laurent Pinchart wrote:
> On Wed, May 24, 2023 at 12:21:52PM +0000, Sakari Ailus wrote:
> > On Wed, May 17, 2023 at 11:05:08AM +0200, Jacopo Mondi wrote:
> > > On Wed, May 17, 2023 at 01:29:13PM +0530, Jai Luthra wrote:
> > > > On May 16, 2023 at 09:46:57 +0200, Jacopo Mondi wrote:
> > > > > On Mon, May 15, 2023 at 05:25:55PM +0530, Jai Luthra wrote:
> > > > > > 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).
> > > >
> > > > Oh makes sense, thanks.
> > > >
> > > > I can update s_frame_interval to return -EINVAL for CSI-2 as a follow-up
> > > > series.
> > > >
> > > > Will also try if g_frame_interval can report correct framerate (60fps vs
> > > > 30fps) depending upon the bus type, as I don't think returning -EINVAL
> > > > would be correct behavior. If that does not work maybe we can unset the
> > > > ops hooks before registering the subdev with the framework.
> > > 
> > > I would rather considering disabling the whole s/g/enum_frame_interval
> > > operations in CSI-2 mode.
> > > 
> > > The specification report as an accepted error code
> > > 
> > > EINVAL
> > > The struct v4l2_subdev_frame_interval pad references a non-existing
> > > pad, or the pad doesn't support frame intervals.
> > > 
> > > https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.html
> > > 
> > > Hans Sakari and Laurent in cc: would it be acceptable to disable the
> > > frame_interval operations completely when the sensor is used in MIPI
> > > CSI-2 mode and only allow them in parallel mode ? Is returning -EINVAL
> > > acceptable in that case ?
> > 
> > I don't think the bus *should* really make a difference. The driver has
> > supported s_frame_interval in the past and drop that support completely
> > likely would cause issues.
> > 
> > Although --- if in this case supporting s_frame_interval just for parallel
> > really helps keeping the driver somehow sane and doesn't break anything, I
> > have no objections.
> > 
> > -EINVAL works for me.
> 
> I'd rather drop it in both cases, but for parallel mode I believe it
> could cause regressions.

Thanks, I can send a follow-up series to return -EINVAL for 
[gs]_frame_interval ops when in MIPI mode.

---

Although I noticed one issue with fixing the framerate to 60fps for 
640x480 (VGA) mode -- it does not work with all the MIPI CSI-2 modules I 
have available for testing:
1. Digilent PCam5C
2. ALINX AN5641
3. Technexion TEVI-OV5640-RPI

Module 3 works fine at both 60fps and 30fps for VGA mode.

Module 1 and 2 work with 30fps, but give blank (black) frames at the 
default link frequency (which is at 60fps).

The XVCLK for module 1 & 2 is 12Mhz while for module 3 it is 24Mhz, so I 
suspect some issue in the sensor clock tree.

Surprisingly 1024x768@1/30 works fine on all modules, which is at the 
same link frequency (384Mhz) as 640x480@1/60 so I don't think the issue 
is around achieving a high MIPI clock with the lower XVCLK.

---

Jacopo,

Would it be acceptable to bump down the default link frequency to 192Mhz 
instead of 384Mhz (and thus framerate fixed to 30fps for VGA) in my 
follow-up series?

Once I can debug the issue with clock tree, we can bump it back up to 
384Mhz/60fps.

> 
> -- 
> Regards,
> 
> Laurent Pinchart

Thanks,
Jai