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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux