Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates

[Sakari Ailus]

Hi Steve,

On Mon, Mar 13, 2017 at 11:06:22AM -0700, Steve Longerbeam wrote:
> 
> 
> On 03/13/2017 06:55 AM, Philipp Zabel wrote:
> >On Mon, 2017-03-13 at 13:27 +0000, Russell King - ARM Linux wrote:
> >>On Mon, Mar 13, 2017 at 03:16:48PM +0200, Sakari Ailus wrote:
> >>>The vast majority of existing drivers do not implement them nor the user
> >>>space expects having to set them. Making that mandatory would break existing
> >>>user space.
> >>>
> >>>In addition, that does not belong to link validation either: link validation
> >>>should only include static properties of the link that are required for
> >>>correct hardware operation. Frame rate is not such property: hardware that
> >>>supports the MC interface generally does not recognise such concept (with
> >>>the exception of some sensors). Additionally, it is dynamic: the frame rate
> >>>can change during streaming, making its validation at streamon time useless.
> >>
> >>So how do we configure the CSI, which can do frame skipping?
> >>
> >>With what you're proposing, it means it's possible to configure the
> >>camera sensor source pad to do 50fps.  Configure the CSI sink pad to
> >>an arbitary value, such as 30fps, and configure the CSI source pad to
> >>15fps.
> >>
> >>What you actually get out of the CSI is 25fps, which bears very little
> >>with the actual values used on the CSI source pad.
> >>
> >>You could say "CSI should ask the camera sensor" - well, that's fine
> >>if it's immediately downstream, but otherwise we'd need to go walking
> >>down the graph to find something that resembles its source - there may
> >>be mux and CSI2 interface subdev blocks in that path.  Or we just accept
> >>that frame rates are completely arbitary and bear no useful meaning what
> >>so ever.
> >
> >Which would include the frame interval returned by VIDIOC_G_PARM on the
> >connected video device, as that gets its information from the CSI output
> >pad's frame interval.
> >
> 
> I'm kinda in the middle on this topic. I agree with Sakari that
> frame rate can fluctuate, but that should only be temporary. If
> the frame rate permanently shifts from what a subdev reports via
> g_frame_interval, then that is a system problem. So I agree with
> Phillip and Russell that a link validation of frame interval still
> makes sense.
> 
> But I also have to agree with Sakari that a subdev that has no
> control over frame rate has no business implementing those ops.
> 
> And then I agree with Russell that for subdevs that do have control
> over frame rate, they would have to walk the graph to find the frame
> rate source.
> 
> So we're stuck in a broken situation: either the subdevs have to walk
> the graph to find the source of frame rate, or s_frame_interval
> would have to be mandatory and validated between pads, same as set_fmt.

It's not broken; what we are missing though is documentation on how to
control devices that can change the frame rate i.e. presumably drop frames
occasionally.

If you're doing something that hasn't been done before, it may be that new
documentation needs to be written to accomodate that use case. As we have an
existing interface (VIDIOC_SUBDEV_[GS]_FRAME_INTERVAL) it does make sense
to use that. What is not possible, though, is to mandate its use in link
validation everywhere.

If you had a hardware limitation that would require that the frame rate is
constant, then we'd need to handle that in link validation for that
particular piece of hardware. But there really is no case for doing that for
everything else.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@xxxxxx	XMPP: sailus@xxxxxxxxxxxxxx