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

[Hans Verkuil]

On 03/13/2017 10:03 PM, Sakari Ailus wrote:
> 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.
> 

General note: I would strongly recommend that g/s_parm support is removed in
v4l2_subdev in favor of g/s_frame_interval.

g/s_parm is an abomination...

There seem to be only a few i2c drivers that use g/s_parm, so this shouldn't
be a lot of work.

Having two APIs for the same thing is always very bad.

Regards,

	Hans