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

[Philipp Zabel]

On Tue, 2017-03-14 at 08:34 +0100, Hans Verkuil wrote:
> 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 +, 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...

Agreed. Just in this specific case I was talking about G_PARM on
the /dev/video node, not the v4l2_subdev nodes. This is currently used
by non-subdev-aware userspace to obtain the framerate from the video
capture device.

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

regards
Philipp

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

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

[Russell King - ARM Linux]

On Mon, Mar 13, 2017 at 11:03:50PM +0200, Sakari Ailus wrote:
> Hi Steve,
> 
> On Mon, Mar 13, 2017 at 11:06:22AM -0700, Steve Longerbeam wrote:
> > 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.

It's not about "presumably drop frames occasionally."  The definition
of "occasional" is "occurring or appearing at irregular or infrequent
intervals".  Another word which describes what you're saying would be
"randomly drop frames" which would be a quite absurd "feature" to
include in hardware.

It's about _deterministically_ omitting frames from the capture.

The hardware provides two controls:
1. the size of a group of frames - between 1 and 5 frames
2. select which frames within the group are dropped using a bitmask

This gives a _regular_ pattern of dropped frames.

The rate scaling is given by: hweight(bitmask) / group size.

So, for example, if you're receiving a 50fps TV broadcast, and want to
capture at 25fps, you can set the group size to 2, and set the frame
drop to binary "01" or "10" - if it's interlaced, this would have the
effect of selecting one field, or skipping every other frame if
progressive.

That's not "we'll occasionally drop some frames", that's frame rate
scaling.  Just like you can scale the size of an image by omitting
every other pixel and every other line, this hardware allows omitting
every other frame or more.

So, to configure this feature, CSI needs to know two bits of information:

1. The _source_ frame rate.
2. The desired _sink_ frame rate.

>From that, it can compute how many frames to drop, and size of the group.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

[Russell King - ARM Linux]

On Mon, Mar 13, 2017 at 10:56:46PM +0200, Sakari Ailus wrote:
> Hi Russell,
> 
> On Mon, Mar 13, 2017 at 01:27:02PM +, 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.
> 
> The user is responsible for configuring the pipeline. It is thus not
> unreasonable to as the user to configure the frame rate as well if there are
> device in the pipeline that can affect the frame rate. The way I proposed to
> implement it is compliant with the existing API and entirely deterministic,
> contrary to what you're saying.

You haven't really addressed my point at all.

What you seem to be saying is that you're quite happy for the situation
(which is a total misconfiguration) to exist.  Given the vapourware of
userspace (which I don't see changing in any kind of reasonable timeline)
I think this is completely absurd.

I'll state clearly now: everything that we've discussed so far, I'm
finding very hard to take anything you've said seriously.  I think we
have very different and incompatible point of views about what is
acceptable from a user point of view, so much so that we're never going
to agree on any point.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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 +, 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.ai...@iki.fi XMPP: sai...@retiisi.org.uk

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

[Sakari Ailus]

Hi Russell,

On Mon, Mar 13, 2017 at 01:27:02PM +, 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.

The user is responsible for configuring the pipeline. It is thus not
unreasonable to as the user to configure the frame rate as well if there are
device in the pipeline that can affect the frame rate. The way I proposed to
implement it is compliant with the existing API and entirely deterministic,
contrary to what you're saying.

It's not the job of the CSI sub-device to figure it out.

S_PARM and G_PARM function as interface on the V4L2 API to access the frame
rate on plain V4L2 devices. The i.MX6 IPU is not a plain V4L2 device.
Essentially the V4L2 device node you have there is an interface to a rather
plain DMA engine, no more.

There have been plans to add device profiles to the documentation but that
work has not yet been done.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

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

[Steve Longerbeam]

On 03/13/2017 06:55 AM, Philipp Zabel wrote:

On Mon, 2017-03-13 at 13:27 +, 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.

Steve

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

[Philipp Zabel]

On Mon, 2017-03-13 at 13:27 +, 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.

regards
Philipp

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

[Russell King - ARM Linux]

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

[Sakari Ailus]

Hi Philipp,

On Tue, Feb 21, 2017 at 09:50:23AM +0100, Philipp Zabel wrote:
...
> > > Then there's the issue where, if you have this setup:
> > >
> > >  camera --> csi2 receiver --> csi --> capture
> > >
> > > and the "csi" subdev can skip frames, you need to know (a) at the CSI
> > > sink pad what the frame rate is of the source (b) what the desired
> > > source pad frame rate is, so you can configure the frame skipping.
> > > So, does the csi subdev have to walk back through the media graph
> > > looking for the frame rate?  Does the capture device have to walk back
> > > through the media graph looking for some subdev to tell it what the
> > > frame rate is - the capture device certainly can't go straight to the
> > > sensor to get an answer to that question, because that bypasses the
> > > effect of the CSI frame skipping (which will lower the frame rate.)
> > >
> > > IMHO, frame rate is just another format property, just like the
> > > resolution and data format itself, and v4l2 should be treating it no
> > > differently.
> > >
> > 
> > I agree, frame rate, if indicated/specified by both sides of a link,
> > should match. So maybe this should be part of v4l2 link validation.
> > 
> > This might be a good time to propose the following patch.
> 
> I agree with Steve and Russell. I don't see why the (nominal) frame
> interval should be handled differently than resolution, data format, and
> colorspace information. I think it should just be propagated in the same
> way, and there is no reason to have two connected pads set to a
> different interval. That would make implementing the g/s_frame_interval
> subdev calls mandatory.

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.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

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

[Steve Longerbeam]

On 02/21/2017 02:21 PM, Steve Longerbeam wrote:



On 02/21/2017 04:15 AM, Sakari Ailus wrote:

Hi Steve,

On Mon, Feb 20, 2017 at 02:56:15PM -0800, Steve Longerbeam wrote:



On 02/20/2017 02:04 PM, Sakari Ailus wrote:

Hi Steve,

On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote:

From: Russell King 

Setting and getting frame rates is part of the negotiation mechanism
between subdevs.  The lack of support means that a frame rate at the
sensor can't be negotiated through the subdev path.


Just wondering --- what do you need this for?



Hi Sakari,

i.MX does need the ability to negotiate the frame rates in the
pipelines. The CSI has the ability to skip frames at the output,
which is something Philipp added to the CSI subdev. That affects
frame interval at the CSI output.

But as Russell pointed out, the lack of [gs]_frame_interval op
causes media-ctl to fail:

media-ctl -v -d /dev/media1 --set-v4l2
'"imx6-mipi-csi2":1[fmt:SGBRG8/512x512@1/30]'

Opening media device /dev/media1
Enumerating entities
Found 29 entities
Enumerating pads and links
Setting up format SGBRG8 512x512 on pad imx6-mipi-csi2/1
Format set: SGBRG8 512x512
Setting up frame interval 1/30 on entity imx6-mipi-csi2
Unable to set frame interval: Inappropriate ioctl for device
(-25)Unable to
setup formats: Inappropriate ioctl for device (25)


So i.MX needs to implement this op in every subdev in the
pipeline, otherwise it's not possible to configure the
pipeline with media-ctl.


The frame rate is only set on the sub-device which you explicitly set it.
I.e. setting the frame rate fails if it's not supported on a pad.

Philipp recently posted patches that add frame rate propagation to
media-ctl.

Frame rate is typically settable (and gettable) only on sensor
sub-device's
source pad,  which means it normally would not be propagated by the
kernel
but with Philipp's patches, on the sink pad of the bus receiver.
Receivers
don't have a way to control it nor they implement the IOCTLs, so that
would
indeed result in an error.



Frame rate is really an essential piece of information. The spatial
dimensions and data type provided by set_fmt are really only half the
equation, the other is temporal, i.e. the data rate.

It's true that subdevices have no control over the frame rate at their
sink pads, but the same argument applies to set_fmt. Even if it has
no control over the data format it receives, it still needs that
information in order to determine the correct format at the source.
The same argument applies to frame rate.

So in my opinion, the behavior of [gs]_frame_interval should be, if a
subdevice is capable of modifying the frame rate, then it should
implement [gs]_frame_interval at _all_ of its pads, similar to set_fmt.
And frame rate should really be part of link validation the same as
set_fmt is.



Actually, if frame rate were added to link validation then
[gs]_frame_interval would have to be mandatory, even if the
subdev has no control over frame rate, again this is like
set_fmt. Otherwise, if a subdev has not implemented
[gs]_frame_interval, then frame rate validation across
the whole pipeline is broken. Because, if we have

A -> B -> C

and B has not implemented [gs]_frame_interval, and C is expecting
30 fps, then pipeline validation would succeed even though A is 
outputting 60 fps.


Steve

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

[Steve Longerbeam]

On 02/21/2017 04:15 AM, Sakari Ailus wrote:

Hi Steve,

On Mon, Feb 20, 2017 at 02:56:15PM -0800, Steve Longerbeam wrote:



On 02/20/2017 02:04 PM, Sakari Ailus wrote:

Hi Steve,

On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote:

From: Russell King 

Setting and getting frame rates is part of the negotiation mechanism
between subdevs.  The lack of support means that a frame rate at the
sensor can't be negotiated through the subdev path.


Just wondering --- what do you need this for?



Hi Sakari,

i.MX does need the ability to negotiate the frame rates in the
pipelines. The CSI has the ability to skip frames at the output,
which is something Philipp added to the CSI subdev. That affects
frame interval at the CSI output.

But as Russell pointed out, the lack of [gs]_frame_interval op
causes media-ctl to fail:

media-ctl -v -d /dev/media1 --set-v4l2
'"imx6-mipi-csi2":1[fmt:SGBRG8/512x512@1/30]'

Opening media device /dev/media1
Enumerating entities
Found 29 entities
Enumerating pads and links
Setting up format SGBRG8 512x512 on pad imx6-mipi-csi2/1
Format set: SGBRG8 512x512
Setting up frame interval 1/30 on entity imx6-mipi-csi2
Unable to set frame interval: Inappropriate ioctl for device (-25)Unable to
setup formats: Inappropriate ioctl for device (25)


So i.MX needs to implement this op in every subdev in the
pipeline, otherwise it's not possible to configure the
pipeline with media-ctl.


The frame rate is only set on the sub-device which you explicitly set it.
I.e. setting the frame rate fails if it's not supported on a pad.

Philipp recently posted patches that add frame rate propagation to
media-ctl.

Frame rate is typically settable (and gettable) only on sensor sub-device's
source pad,  which means it normally would not be propagated by the kernel
but with Philipp's patches, on the sink pad of the bus receiver. Receivers
don't have a way to control it nor they implement the IOCTLs, so that would
indeed result in an error.



Frame rate is really an essential piece of information. The spatial
dimensions and data type provided by set_fmt are really only half the
equation, the other is temporal, i.e. the data rate.

It's true that subdevices have no control over the frame rate at their
sink pads, but the same argument applies to set_fmt. Even if it has
no control over the data format it receives, it still needs that
information in order to determine the correct format at the source.
The same argument applies to frame rate.

So in my opinion, the behavior of [gs]_frame_interval should be, if a
subdevice is capable of modifying the frame rate, then it should
implement [gs]_frame_interval at _all_ of its pads, similar to set_fmt.
And frame rate should really be part of link validation the same as
set_fmt is.

Steve

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

[Sakari Ailus]

On Tue, Feb 21, 2017 at 04:03:32PM +, Russell King - ARM Linux wrote:
> On Tue, Feb 21, 2017 at 05:38:34PM +0200, Sakari Ailus wrote:
> > Hi Russell,
> > 
> > On Tue, Feb 21, 2017 at 01:21:32PM +, Russell King - ARM Linux wrote:
> > > On Tue, Feb 21, 2017 at 02:37:57PM +0200, Sakari Ailus wrote:
> > > > Hi Russell,
> > > > 
> > > > On Tue, Feb 21, 2017 at 12:13:32AM +, Russell King - ARM Linux 
> > > > wrote:
> > > > > On Tue, Feb 21, 2017 at 12:04:10AM +0200, Sakari Ailus wrote:
> > > > > > On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote:
> > > > > > > From: Russell King 
> > > > > > > 
> > > > > > > Setting and getting frame rates is part of the negotiation 
> > > > > > > mechanism
> > > > > > > between subdevs.  The lack of support means that a frame rate at 
> > > > > > > the
> > > > > > > sensor can't be negotiated through the subdev path.
> > > > > > 
> > > > > > Just wondering --- what do you need this for?
> > > > > 
> > > > > The v4l2 documentation contradicts the media-ctl implementation.
> > > > > 
> > > > > While v4l2 documentation says:
> > > > > 
> > > > >   These ioctls are used to get and set the frame interval at specific
> > > > >   subdev pads in the image pipeline. The frame interval only makes 
> > > > > sense
> > > > >   for sub-devices that can control the frame period on their own. This
> > > > >   includes, for instance, image sensors and TV tuners. Sub-devices 
> > > > > that
> > > > >   don't support frame intervals must not implement these ioctls.
> > > > > 
> > > > > However, when trying to configure the pipeline using media-ctl, eg:
> > > > > 
> > > > > media-ctl -d /dev/media1 --set-v4l2 '"imx219 pixel 
> > > > > 0-0010":0[crop:(0,0)/3264x2464]'
> > > > > media-ctl -d /dev/media1 --set-v4l2 '"imx219 
> > > > > 0-0010":1[fmt:SRGGB10/3264x2464@1/30]'
> > > > > media-ctl -d /dev/media1 --set-v4l2 '"imx219 
> > > > > 0-0010":0[fmt:SRGGB8/816x616@1/30]'
> > > > > media-ctl -d /dev/media1 --set-v4l2 
> > > > > '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
> > > > > Unable to setup formats: Inappropriate ioctl for device (25)
> > > > > media-ctl -d /dev/media1 --set-v4l2 
> > > > > '"ipu1_csi0_mux":2[fmt:SRGGB8/816x616@1/30]'
> > > > > media-ctl -d /dev/media1 --set-v4l2 
> > > > > '"ipu1_csi0":2[fmt:SRGGB8/816x616@1/30]'
> > > > > 
> > > > > The problem there is that the format setting for the csi2 does not get
> > > > > propagated forward:
> > > > 
> > > > The CSI-2 receivers typically do not implement frame interval IOCTLs as 
> > > > they
> > > > do not control the frame interval. Some sensors or TV tuners typically 
> > > > do,
> > > > so they implement these IOCTLs.
> > > 
> > > No, TV tuners do not.  The frame rate for a TV tuner is set by the
> > > broadcaster, not by the tuner.  The tuner can't change that frame rate.
> > > The tuner may opt to "skip" fields or frames.  That's no different from
> > > what the CSI block in my example below is capable of doing.
> > > 
> > > Treating a tuner differently from the CSI block is inconsistent and
> > > completely wrong.
> > 
> > I agree tuners in that sense are somewhat similar, and they are not treated
> > differently because they are tuners (and not CSI-2 receivers). Neither can
> > control the frame rate of the incoming video stream.
> > 
> > Conceivably a tuner could implement G_FRAME_INTERVAL IOCTL, but based on a
> > quick glance none appears to. Neither do CSI-2 receivers. Only sensor
> > drivers do currently.
> 
> Please look again.  I am being very careful with "CSI" vs "CSI-2" in my
> emails, you are conflating the two.
> 
> In all my emails so far, "CSI" refers to a block of hardware that is
> responsible for receiving an image stream from some kind of source.  It
> contains hardware that supports frame skipping.

Ah, I missed the difference. Thanks for pointing it out.

Still, that does not change how the skipping would work nor how I proposed
it would be configured from the user space.

> 
> "CSI-2" refers to a different block of hardware that is responsible for
> receiving a serially encoded stream from a MIPI-CSI-2 compliant source
> and providing it to the "CSI" block.
> 
> I would have thought my diagram that I drew would have made it clear that
> they were different blocks of hardware, but I guess in this case, the old
> saying "a picture is worth 1000 words" is simply not true.
> 
> > Images are transmitted as series of lines, with each line ending in a
> > horizontal blanking period, and each frame ending with a similar period of
> 
> I'm sorry, are you seriously teaching me to suck rocks?  I am insulted.
> 
> I've been involved in TV and video for many years, I don't need you to
> tell me how video is transmitted.
> 
> Sorry, you've just lost my interest in further discussion.

There's no need to feel insulted; that certainly was not the intention.

I've proposed you a solution, please comment on that.

-- 
Regards,

Sakari Ailus
e-mail: 

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

[Russell King - ARM Linux]

On Tue, Feb 21, 2017 at 05:38:34PM +0200, Sakari Ailus wrote:
> Hi Russell,
> 
> On Tue, Feb 21, 2017 at 01:21:32PM +, Russell King - ARM Linux wrote:
> > On Tue, Feb 21, 2017 at 02:37:57PM +0200, Sakari Ailus wrote:
> > > Hi Russell,
> > > 
> > > On Tue, Feb 21, 2017 at 12:13:32AM +, Russell King - ARM Linux wrote:
> > > > On Tue, Feb 21, 2017 at 12:04:10AM +0200, Sakari Ailus wrote:
> > > > > On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote:
> > > > > > From: Russell King 
> > > > > > 
> > > > > > Setting and getting frame rates is part of the negotiation mechanism
> > > > > > between subdevs.  The lack of support means that a frame rate at the
> > > > > > sensor can't be negotiated through the subdev path.
> > > > > 
> > > > > Just wondering --- what do you need this for?
> > > > 
> > > > The v4l2 documentation contradicts the media-ctl implementation.
> > > > 
> > > > While v4l2 documentation says:
> > > > 
> > > >   These ioctls are used to get and set the frame interval at specific
> > > >   subdev pads in the image pipeline. The frame interval only makes sense
> > > >   for sub-devices that can control the frame period on their own. This
> > > >   includes, for instance, image sensors and TV tuners. Sub-devices that
> > > >   don't support frame intervals must not implement these ioctls.
> > > > 
> > > > However, when trying to configure the pipeline using media-ctl, eg:
> > > > 
> > > > media-ctl -d /dev/media1 --set-v4l2 '"imx219 pixel 
> > > > 0-0010":0[crop:(0,0)/3264x2464]'
> > > > media-ctl -d /dev/media1 --set-v4l2 '"imx219 
> > > > 0-0010":1[fmt:SRGGB10/3264x2464@1/30]'
> > > > media-ctl -d /dev/media1 --set-v4l2 '"imx219 
> > > > 0-0010":0[fmt:SRGGB8/816x616@1/30]'
> > > > media-ctl -d /dev/media1 --set-v4l2 
> > > > '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
> > > > Unable to setup formats: Inappropriate ioctl for device (25)
> > > > media-ctl -d /dev/media1 --set-v4l2 
> > > > '"ipu1_csi0_mux":2[fmt:SRGGB8/816x616@1/30]'
> > > > media-ctl -d /dev/media1 --set-v4l2 
> > > > '"ipu1_csi0":2[fmt:SRGGB8/816x616@1/30]'
> > > > 
> > > > The problem there is that the format setting for the csi2 does not get
> > > > propagated forward:
> > > 
> > > The CSI-2 receivers typically do not implement frame interval IOCTLs as 
> > > they
> > > do not control the frame interval. Some sensors or TV tuners typically do,
> > > so they implement these IOCTLs.
> > 
> > No, TV tuners do not.  The frame rate for a TV tuner is set by the
> > broadcaster, not by the tuner.  The tuner can't change that frame rate.
> > The tuner may opt to "skip" fields or frames.  That's no different from
> > what the CSI block in my example below is capable of doing.
> > 
> > Treating a tuner differently from the CSI block is inconsistent and
> > completely wrong.
> 
> I agree tuners in that sense are somewhat similar, and they are not treated
> differently because they are tuners (and not CSI-2 receivers). Neither can
> control the frame rate of the incoming video stream.
> 
> Conceivably a tuner could implement G_FRAME_INTERVAL IOCTL, but based on a
> quick glance none appears to. Neither do CSI-2 receivers. Only sensor
> drivers do currently.

Please look again.  I am being very careful with "CSI" vs "CSI-2" in my
emails, you are conflating the two.

In all my emails so far, "CSI" refers to a block of hardware that is
responsible for receiving an image stream from some kind of source.  It
contains hardware that supports frame skipping.

"CSI-2" refers to a different block of hardware that is responsible for
receiving a serially encoded stream from a MIPI-CSI-2 compliant source
and providing it to the "CSI" block.

I would have thought my diagram that I drew would have made it clear that
they were different blocks of hardware, but I guess in this case, the old
saying "a picture is worth 1000 words" is simply not true.

> Images are transmitted as series of lines, with each line ending in a
> horizontal blanking period, and each frame ending with a similar period of

I'm sorry, are you seriously teaching me to suck rocks?  I am insulted.

I've been involved in TV and video for many years, I don't need you to
tell me how video is transmitted.

Sorry, you've just lost my interest in further discussion.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

[Sakari Ailus]

Hi Russell,

On Tue, Feb 21, 2017 at 01:21:32PM +, Russell King - ARM Linux wrote:
> On Tue, Feb 21, 2017 at 02:37:57PM +0200, Sakari Ailus wrote:
> > Hi Russell,
> > 
> > On Tue, Feb 21, 2017 at 12:13:32AM +, Russell King - ARM Linux wrote:
> > > On Tue, Feb 21, 2017 at 12:04:10AM +0200, Sakari Ailus wrote:
> > > > On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote:
> > > > > From: Russell King 
> > > > > 
> > > > > Setting and getting frame rates is part of the negotiation mechanism
> > > > > between subdevs.  The lack of support means that a frame rate at the
> > > > > sensor can't be negotiated through the subdev path.
> > > > 
> > > > Just wondering --- what do you need this for?
> > > 
> > > The v4l2 documentation contradicts the media-ctl implementation.
> > > 
> > > While v4l2 documentation says:
> > > 
> > >   These ioctls are used to get and set the frame interval at specific
> > >   subdev pads in the image pipeline. The frame interval only makes sense
> > >   for sub-devices that can control the frame period on their own. This
> > >   includes, for instance, image sensors and TV tuners. Sub-devices that
> > >   don't support frame intervals must not implement these ioctls.
> > > 
> > > However, when trying to configure the pipeline using media-ctl, eg:
> > > 
> > > media-ctl -d /dev/media1 --set-v4l2 '"imx219 pixel 
> > > 0-0010":0[crop:(0,0)/3264x2464]'
> > > media-ctl -d /dev/media1 --set-v4l2 '"imx219 
> > > 0-0010":1[fmt:SRGGB10/3264x2464@1/30]'
> > > media-ctl -d /dev/media1 --set-v4l2 '"imx219 
> > > 0-0010":0[fmt:SRGGB8/816x616@1/30]'
> > > media-ctl -d /dev/media1 --set-v4l2 
> > > '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
> > > Unable to setup formats: Inappropriate ioctl for device (25)
> > > media-ctl -d /dev/media1 --set-v4l2 
> > > '"ipu1_csi0_mux":2[fmt:SRGGB8/816x616@1/30]'
> > > media-ctl -d /dev/media1 --set-v4l2 
> > > '"ipu1_csi0":2[fmt:SRGGB8/816x616@1/30]'
> > > 
> > > The problem there is that the format setting for the csi2 does not get
> > > propagated forward:
> > 
> > The CSI-2 receivers typically do not implement frame interval IOCTLs as they
> > do not control the frame interval. Some sensors or TV tuners typically do,
> > so they implement these IOCTLs.
> 
> No, TV tuners do not.  The frame rate for a TV tuner is set by the
> broadcaster, not by the tuner.  The tuner can't change that frame rate.
> The tuner may opt to "skip" fields or frames.  That's no different from
> what the CSI block in my example below is capable of doing.
> 
> Treating a tuner differently from the CSI block is inconsistent and
> completely wrong.

I agree tuners in that sense are somewhat similar, and they are not treated
differently because they are tuners (and not CSI-2 receivers). Neither can
control the frame rate of the incoming video stream.

Conceivably a tuner could implement G_FRAME_INTERVAL IOCTL, but based on a
quick glance none appears to. Neither do CSI-2 receivers. Only sensor
drivers do currently.

> 
> > There are alternative ways to specify the frame rate.
> 
> Empty statements (or hand-waving type statements) I'm afraid don't
> contribute to the discussion, because they mean nothing to me.  Please
> give an example, or flesh out what you mean.

Images are transmitted as series of lines, with each line ending in a
horizontal blanking period, and each frame ending with a similar period of
vertical blanking. The blanking configuration in the units of pixels and
lines at their pixel clock is a native unit which sensors typically use, and
some drivers expose the blanking controls directly to the user.



> 
> > > $ strace media-ctl -d /dev/media1 --set-v4l2 
> > > '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
> > > ...
> > > open("/dev/v4l-subdev16", O_RDWR)   = 3
> > > ioctl(3, VIDIOC_SUBDEV_S_FMT, 0xbec16244) = 0
> > > ioctl(3, VIDIOC_SUBDEV_S_FRAME_INTERVAL, 0xbec162a4) = -1 ENOTTY 
> > > (Inappropriate
> > > ioctl for device)
> > > fstat64(1, {st_mode=S_IFCHR|0600, st_rdev=makedev(136, 2), ...}) = 0
> > > write(1, "Unable to setup formats: Inappro"..., 61) = 61
> > > Unable to setup formats: Inappropriate ioctl for device (25)
> > > close(3)= 0
> > > exit_group(1)   = ?
> > > +++ exited with 1 +++
> > > 
> > > because media-ctl exits as soon as it encouters the error while trying
> > > to set the frame rate.
> > > 
> > > This makes implementing setup of the media pipeline in shell scripts
> > > unnecessarily difficult - as you need to then know whether an entity
> > > is likely not to support the VIDIOC_SUBDEV_S_FRAME_INTERVAL call,
> > > and either avoid specifying a frame rate:
> > 
> > You should remove the frame interval setting from sub-devices that do not
> > support it.
> 
> That means we end up with horribly complex scripts.  This "solution" does
> not 

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

[Russell King - ARM Linux]

On Tue, Feb 21, 2017 at 02:37:57PM +0200, Sakari Ailus wrote:
> Hi Russell,
> 
> On Tue, Feb 21, 2017 at 12:13:32AM +, Russell King - ARM Linux wrote:
> > On Tue, Feb 21, 2017 at 12:04:10AM +0200, Sakari Ailus wrote:
> > > On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote:
> > > > From: Russell King 
> > > > 
> > > > Setting and getting frame rates is part of the negotiation mechanism
> > > > between subdevs.  The lack of support means that a frame rate at the
> > > > sensor can't be negotiated through the subdev path.
> > > 
> > > Just wondering --- what do you need this for?
> > 
> > The v4l2 documentation contradicts the media-ctl implementation.
> > 
> > While v4l2 documentation says:
> > 
> >   These ioctls are used to get and set the frame interval at specific
> >   subdev pads in the image pipeline. The frame interval only makes sense
> >   for sub-devices that can control the frame period on their own. This
> >   includes, for instance, image sensors and TV tuners. Sub-devices that
> >   don't support frame intervals must not implement these ioctls.
> > 
> > However, when trying to configure the pipeline using media-ctl, eg:
> > 
> > media-ctl -d /dev/media1 --set-v4l2 '"imx219 pixel 
> > 0-0010":0[crop:(0,0)/3264x2464]'
> > media-ctl -d /dev/media1 --set-v4l2 '"imx219 
> > 0-0010":1[fmt:SRGGB10/3264x2464@1/30]'
> > media-ctl -d /dev/media1 --set-v4l2 '"imx219 
> > 0-0010":0[fmt:SRGGB8/816x616@1/30]'
> > media-ctl -d /dev/media1 --set-v4l2 
> > '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
> > Unable to setup formats: Inappropriate ioctl for device (25)
> > media-ctl -d /dev/media1 --set-v4l2 
> > '"ipu1_csi0_mux":2[fmt:SRGGB8/816x616@1/30]'
> > media-ctl -d /dev/media1 --set-v4l2 '"ipu1_csi0":2[fmt:SRGGB8/816x616@1/30]'
> > 
> > The problem there is that the format setting for the csi2 does not get
> > propagated forward:
> 
> The CSI-2 receivers typically do not implement frame interval IOCTLs as they
> do not control the frame interval. Some sensors or TV tuners typically do,
> so they implement these IOCTLs.

No, TV tuners do not.  The frame rate for a TV tuner is set by the
broadcaster, not by the tuner.  The tuner can't change that frame rate.
The tuner may opt to "skip" fields or frames.  That's no different from
what the CSI block in my example below is capable of doing.

Treating a tuner differently from the CSI block is inconsistent and
completely wrong.

> There are alternative ways to specify the frame rate.

Empty statements (or hand-waving type statements) I'm afraid don't
contribute to the discussion, because they mean nothing to me.  Please
give an example, or flesh out what you mean.

> > $ strace media-ctl -d /dev/media1 --set-v4l2 
> > '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
> > ...
> > open("/dev/v4l-subdev16", O_RDWR)   = 3
> > ioctl(3, VIDIOC_SUBDEV_S_FMT, 0xbec16244) = 0
> > ioctl(3, VIDIOC_SUBDEV_S_FRAME_INTERVAL, 0xbec162a4) = -1 ENOTTY 
> > (Inappropriate
> > ioctl for device)
> > fstat64(1, {st_mode=S_IFCHR|0600, st_rdev=makedev(136, 2), ...}) = 0
> > write(1, "Unable to setup formats: Inappro"..., 61) = 61
> > Unable to setup formats: Inappropriate ioctl for device (25)
> > close(3)= 0
> > exit_group(1)   = ?
> > +++ exited with 1 +++
> > 
> > because media-ctl exits as soon as it encouters the error while trying
> > to set the frame rate.
> > 
> > This makes implementing setup of the media pipeline in shell scripts
> > unnecessarily difficult - as you need to then know whether an entity
> > is likely not to support the VIDIOC_SUBDEV_S_FRAME_INTERVAL call,
> > and either avoid specifying a frame rate:
> 
> You should remove the frame interval setting from sub-devices that do not
> support it.

That means we end up with horribly complex scripts.  This "solution" does
not scale.  Therefore, it is not a "solution".

It's fine if you want to write a script to setup the media pipeline using
media-ctl, listing _each_ media-ctl command individually, with arguments
specific to each step, but as I've already said, that does not scale.

I don't want to end up writing separate scripts to configure the pipeline
for different parameters or setups.  I don't want to teach users how to
do that either.

How are users supposed to cope with this craziness?  Are they expected to
write their own scripts and understand this stuff?

As far as I can see, there are no applications out there at the moment that
come close to understanding how to configure a media pipeline, so users
have to understand how to use media-ctl to configure the pipeline manually.
Are we really expecting users to write scripts to do this, and understand
all these nuances?

IMHO, this is completely crazy, and hasn't been fully thought out.

> > $ strace media-ctl -d /dev/media1 --set-v4l2 
> > '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616]'
> > ...
> > open("/dev/v4l-subdev16", O_RDWR)   = 3
> > ioctl(3, 

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

[Sakari Ailus]

Hi Russell,

On Tue, Feb 21, 2017 at 12:13:32AM +, Russell King - ARM Linux wrote:
> On Tue, Feb 21, 2017 at 12:04:10AM +0200, Sakari Ailus wrote:
> > On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote:
> > > From: Russell King 
> > > 
> > > Setting and getting frame rates is part of the negotiation mechanism
> > > between subdevs.  The lack of support means that a frame rate at the
> > > sensor can't be negotiated through the subdev path.
> > 
> > Just wondering --- what do you need this for?
> 
> The v4l2 documentation contradicts the media-ctl implementation.
> 
> While v4l2 documentation says:
> 
>   These ioctls are used to get and set the frame interval at specific
>   subdev pads in the image pipeline. The frame interval only makes sense
>   for sub-devices that can control the frame period on their own. This
>   includes, for instance, image sensors and TV tuners. Sub-devices that
>   don't support frame intervals must not implement these ioctls.
> 
> However, when trying to configure the pipeline using media-ctl, eg:
> 
> media-ctl -d /dev/media1 --set-v4l2 '"imx219 pixel 
> 0-0010":0[crop:(0,0)/3264x2464]'
> media-ctl -d /dev/media1 --set-v4l2 '"imx219 
> 0-0010":1[fmt:SRGGB10/3264x2464@1/30]'
> media-ctl -d /dev/media1 --set-v4l2 '"imx219 
> 0-0010":0[fmt:SRGGB8/816x616@1/30]'
> media-ctl -d /dev/media1 --set-v4l2 
> '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
> Unable to setup formats: Inappropriate ioctl for device (25)
> media-ctl -d /dev/media1 --set-v4l2 
> '"ipu1_csi0_mux":2[fmt:SRGGB8/816x616@1/30]'
> media-ctl -d /dev/media1 --set-v4l2 '"ipu1_csi0":2[fmt:SRGGB8/816x616@1/30]'
> 
> The problem there is that the format setting for the csi2 does not get
> propagated forward:

The CSI-2 receivers typically do not implement frame interval IOCTLs as they
do not control the frame interval. Some sensors or TV tuners typically do,
so they implement these IOCTLs.

There are alternative ways to specify the frame rate.

> 
> $ strace media-ctl -d /dev/media1 --set-v4l2 
> '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
> ...
> open("/dev/v4l-subdev16", O_RDWR)   = 3
> ioctl(3, VIDIOC_SUBDEV_S_FMT, 0xbec16244) = 0
> ioctl(3, VIDIOC_SUBDEV_S_FRAME_INTERVAL, 0xbec162a4) = -1 ENOTTY 
> (Inappropriate
> ioctl for device)
> fstat64(1, {st_mode=S_IFCHR|0600, st_rdev=makedev(136, 2), ...}) = 0
> write(1, "Unable to setup formats: Inappro"..., 61) = 61
> Unable to setup formats: Inappropriate ioctl for device (25)
> close(3)= 0
> exit_group(1)   = ?
> +++ exited with 1 +++
> 
> because media-ctl exits as soon as it encouters the error while trying
> to set the frame rate.
> 
> This makes implementing setup of the media pipeline in shell scripts
> unnecessarily difficult - as you need to then know whether an entity
> is likely not to support the VIDIOC_SUBDEV_S_FRAME_INTERVAL call,
> and either avoid specifying a frame rate:

You should remove the frame interval setting from sub-devices that do not
support it.

> 
> $ strace media-ctl -d /dev/media1 --set-v4l2 
> '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616]'
> ...
> open("/dev/v4l-subdev16", O_RDWR)   = 3
> ioctl(3, VIDIOC_SUBDEV_S_FMT, 0xbeb1a254) = 0
> open("/dev/v4l-subdev0", O_RDWR)= 4
> ioctl(4, VIDIOC_SUBDEV_S_FMT, 0xbeb1a254) = 0
> close(4)= 0
> close(3)= 0
> exit_group(0)   = ?
> +++ exited with 0 +++
> 
> or manually setting the format on the sink.
> 
> Allowing the S_FRAME_INTERVAL call seems to me to be more in keeping
> with the negotiation mechanism that is implemented in subdevs, and
> IMHO should be implemented inside the kernel as a pad operation along
> with the format negotiation, especially so as frame skipping is
> defined as scaling, in just the same way as the frame size is also
> scaling:

The origins of the S_FRAME_INTERVAL IOCTL for sub-devices are the S_PARM
IOCTL for video nodes. It is used to control the frame rate for more simple
devices that do not expose the Media controller interface. The similar
S_FRAME_INTERVAL was added for sub-devices as well, and it has been so far
used to control the frame interval for sensors (and G_FRAME_INTERVAL to
obtain the frame interval for TV tuners, for instance).

The pad argument was added there but media-ctl only supported setting the
frame interval on pad 0, which, coincidentally, worked well for sensor
devices.

The link validation is primarily done in order to ensure the validity of the
hardware configuration: streaming may not be started if the hardware
configuration is not valid.

Also, frame interval is not a static property during streaming: it may be
changed without the knowledge of the other sub-device drivers downstream. It
neither is a property of hardware receiving or processing images: if there
are limitations in processing pixels, then they in practice are related to
pixel rates or 

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

[Sakari Ailus]

Hi Steve,

On Mon, Feb 20, 2017 at 02:56:15PM -0800, Steve Longerbeam wrote:
> 
> 
> On 02/20/2017 02:04 PM, Sakari Ailus wrote:
> >Hi Steve,
> >
> >On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote:
> >>From: Russell King 
> >>
> >>Setting and getting frame rates is part of the negotiation mechanism
> >>between subdevs.  The lack of support means that a frame rate at the
> >>sensor can't be negotiated through the subdev path.
> >
> >Just wondering --- what do you need this for?
> 
> 
> Hi Sakari,
> 
> i.MX does need the ability to negotiate the frame rates in the
> pipelines. The CSI has the ability to skip frames at the output,
> which is something Philipp added to the CSI subdev. That affects
> frame interval at the CSI output.
> 
> But as Russell pointed out, the lack of [gs]_frame_interval op
> causes media-ctl to fail:
> 
> media-ctl -v -d /dev/media1 --set-v4l2
> '"imx6-mipi-csi2":1[fmt:SGBRG8/512x512@1/30]'
> 
> Opening media device /dev/media1
> Enumerating entities
> Found 29 entities
> Enumerating pads and links
> Setting up format SGBRG8 512x512 on pad imx6-mipi-csi2/1
> Format set: SGBRG8 512x512
> Setting up frame interval 1/30 on entity imx6-mipi-csi2
> Unable to set frame interval: Inappropriate ioctl for device (-25)Unable to
> setup formats: Inappropriate ioctl for device (25)
> 
> 
> So i.MX needs to implement this op in every subdev in the
> pipeline, otherwise it's not possible to configure the
> pipeline with media-ctl.

The frame rate is only set on the sub-device which you explicitly set it.
I.e. setting the frame rate fails if it's not supported on a pad.

Philipp recently posted patches that add frame rate propagation to
media-ctl.

Frame rate is typically settable (and gettable) only on sensor sub-device's
source pad, which means it normally would not be propagated by the kernel
but with Philipp's patches, on the sink pad of the bus receiver. Receivers
don't have a way to control it nor they implement the IOCTLs, so that would
indeed result in an error.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

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

[Philipp Zabel]

On Mon, 2017-02-20 at 16:18 -0800, Steve Longerbeam wrote:
> 
> On 02/20/2017 04:13 PM, Russell King - ARM Linux wrote:
> > On Tue, Feb 21, 2017 at 12:04:10AM +0200, Sakari Ailus wrote:
> >> On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote:
> >>> From: Russell King 
> >>>
> >>> Setting and getting frame rates is part of the negotiation mechanism
> >>> between subdevs.  The lack of support means that a frame rate at the
> >>> sensor can't be negotiated through the subdev path.
> >>
> >> Just wondering --- what do you need this for?
> >
> > The v4l2 documentation contradicts the media-ctl implementation.
> >
> > While v4l2 documentation says:
> >
> >   These ioctls are used to get and set the frame interval at specific
> >   subdev pads in the image pipeline. The frame interval only makes sense
> >   for sub-devices that can control the frame period on their own. This
> >   includes, for instance, image sensors and TV tuners. Sub-devices that
> >   don't support frame intervals must not implement these ioctls.
> >
> > However, when trying to configure the pipeline using media-ctl, eg:
> >
> > media-ctl -d /dev/media1 --set-v4l2 '"imx219 pixel 
> > 0-0010":0[crop:(0,0)/3264x2464]'
> > media-ctl -d /dev/media1 --set-v4l2 '"imx219 
> > 0-0010":1[fmt:SRGGB10/3264x2464@1/30]'
> > media-ctl -d /dev/media1 --set-v4l2 '"imx219 
> > 0-0010":0[fmt:SRGGB8/816x616@1/30]'
> > media-ctl -d /dev/media1 --set-v4l2 
> > '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
> > Unable to setup formats: Inappropriate ioctl for device (25)
> > media-ctl -d /dev/media1 --set-v4l2 
> > '"ipu1_csi0_mux":2[fmt:SRGGB8/816x616@1/30]'
> > media-ctl -d /dev/media1 --set-v4l2 '"ipu1_csi0":2[fmt:SRGGB8/816x616@1/30]'
> >
> > The problem there is that the format setting for the csi2 does not get
> > propagated forward:
> >
> > $ strace media-ctl -d /dev/media1 --set-v4l2 
> > '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
> > ...
> > open("/dev/v4l-subdev16", O_RDWR)   = 3
> > ioctl(3, VIDIOC_SUBDEV_S_FMT, 0xbec16244) = 0
> > ioctl(3, VIDIOC_SUBDEV_S_FRAME_INTERVAL, 0xbec162a4) = -1 ENOTTY 
> > (Inappropriate
> > ioctl for device)
> > fstat64(1, {st_mode=S_IFCHR|0600, st_rdev=makedev(136, 2), ...}) = 0
> > write(1, "Unable to setup formats: Inappro"..., 61) = 61
> > Unable to setup formats: Inappropriate ioctl for device (25)
> > close(3)= 0
> > exit_group(1)   = ?
> > +++ exited with 1 +++
> >
> > because media-ctl exits as soon as it encouters the error while trying
> > to set the frame rate.
> >
> > This makes implementing setup of the media pipeline in shell scripts
> > unnecessarily difficult - as you need to then know whether an entity
> > is likely not to support the VIDIOC_SUBDEV_S_FRAME_INTERVAL call,
> > and either avoid specifying a frame rate:
> >
> > $ strace media-ctl -d /dev/media1 --set-v4l2 
> > '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616]'
> > ...
> > open("/dev/v4l-subdev16", O_RDWR)   = 3
> > ioctl(3, VIDIOC_SUBDEV_S_FMT, 0xbeb1a254) = 0
> > open("/dev/v4l-subdev0", O_RDWR)= 4
> > ioctl(4, VIDIOC_SUBDEV_S_FMT, 0xbeb1a254) = 0
> > close(4)= 0
> > close(3)= 0
> > exit_group(0)   = ?
> > +++ exited with 0 +++
> >
> > or manually setting the format on the sink.
> >
> > Allowing the S_FRAME_INTERVAL call seems to me to be more in keeping
> > with the negotiation mechanism that is implemented in subdevs, and
> > IMHO should be implemented inside the kernel as a pad operation along
> > with the format negotiation, especially so as frame skipping is
> > defined as scaling, in just the same way as the frame size is also
> > scaling:
> >
> >-  ``MEDIA_ENT_F_PROC_VIDEO_SCALER``
> >
> >-  Video scaler. An entity capable of video scaling must have
> >   at least one sink pad and one source pad, and scale the
> >   video frame(s) received on its sink pad(s) to a different
> >   resolution output on its source pad(s). The range of
> >   supported scaling ratios is entity-specific and can differ
> >   between the horizontal and vertical directions (in particular
> >   scaling can be supported in one direction only). Binning and
> >   skipping are considered as scaling.
> >
> > Although, this is vague, as it doesn't define what it means by "skipping",
> > whether that's skipping pixels (iow, sub-sampling) or whether that's
> > frame skipping.

I'd interpret this as meaning pixel skipping, not frame skipping.

> > Then there's the issue where, if you have this setup:
> >
> >  camera --> csi2 receiver --> csi --> capture
> >
> > and the "csi" subdev can skip frames, you need to know (a) at the CSI
> > sink pad what the frame rate is of the source (b) what the desired
> > source pad frame rate is, so you can configure the frame skipping.
> > So, does the csi subdev have to walk 

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

[Steve Longerbeam]

On 02/20/2017 04:13 PM, Russell King - ARM Linux wrote:

On Tue, Feb 21, 2017 at 12:04:10AM +0200, Sakari Ailus wrote:

On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote:

From: Russell King 

Setting and getting frame rates is part of the negotiation mechanism
between subdevs.  The lack of support means that a frame rate at the
sensor can't be negotiated through the subdev path.


Just wondering --- what do you need this for?


The v4l2 documentation contradicts the media-ctl implementation.

While v4l2 documentation says:

  These ioctls are used to get and set the frame interval at specific
  subdev pads in the image pipeline. The frame interval only makes sense
  for sub-devices that can control the frame period on their own. This
  includes, for instance, image sensors and TV tuners. Sub-devices that
  don't support frame intervals must not implement these ioctls.

However, when trying to configure the pipeline using media-ctl, eg:

media-ctl -d /dev/media1 --set-v4l2 '"imx219 pixel 
0-0010":0[crop:(0,0)/3264x2464]'
media-ctl -d /dev/media1 --set-v4l2 '"imx219 
0-0010":1[fmt:SRGGB10/3264x2464@1/30]'
media-ctl -d /dev/media1 --set-v4l2 '"imx219 0-0010":0[fmt:SRGGB8/816x616@1/30]'
media-ctl -d /dev/media1 --set-v4l2 
'"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
Unable to setup formats: Inappropriate ioctl for device (25)
media-ctl -d /dev/media1 --set-v4l2 '"ipu1_csi0_mux":2[fmt:SRGGB8/816x616@1/30]'
media-ctl -d /dev/media1 --set-v4l2 '"ipu1_csi0":2[fmt:SRGGB8/816x616@1/30]'

The problem there is that the format setting for the csi2 does not get
propagated forward:

$ strace media-ctl -d /dev/media1 --set-v4l2 
'"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
...
open("/dev/v4l-subdev16", O_RDWR)   = 3
ioctl(3, VIDIOC_SUBDEV_S_FMT, 0xbec16244) = 0
ioctl(3, VIDIOC_SUBDEV_S_FRAME_INTERVAL, 0xbec162a4) = -1 ENOTTY (Inappropriate
ioctl for device)
fstat64(1, {st_mode=S_IFCHR|0600, st_rdev=makedev(136, 2), ...}) = 0
write(1, "Unable to setup formats: Inappro"..., 61) = 61
Unable to setup formats: Inappropriate ioctl for device (25)
close(3)= 0
exit_group(1)   = ?
+++ exited with 1 +++

because media-ctl exits as soon as it encouters the error while trying
to set the frame rate.

This makes implementing setup of the media pipeline in shell scripts
unnecessarily difficult - as you need to then know whether an entity
is likely not to support the VIDIOC_SUBDEV_S_FRAME_INTERVAL call,
and either avoid specifying a frame rate:

$ strace media-ctl -d /dev/media1 --set-v4l2 
'"imx6-mipi-csi2":1[fmt:SRGGB8/816x616]'
...
open("/dev/v4l-subdev16", O_RDWR)   = 3
ioctl(3, VIDIOC_SUBDEV_S_FMT, 0xbeb1a254) = 0
open("/dev/v4l-subdev0", O_RDWR)= 4
ioctl(4, VIDIOC_SUBDEV_S_FMT, 0xbeb1a254) = 0
close(4)= 0
close(3)= 0
exit_group(0)   = ?
+++ exited with 0 +++

or manually setting the format on the sink.

Allowing the S_FRAME_INTERVAL call seems to me to be more in keeping
with the negotiation mechanism that is implemented in subdevs, and
IMHO should be implemented inside the kernel as a pad operation along
with the format negotiation, especially so as frame skipping is
defined as scaling, in just the same way as the frame size is also
scaling:

   -  ``MEDIA_ENT_F_PROC_VIDEO_SCALER``

   -  Video scaler. An entity capable of video scaling must have
  at least one sink pad and one source pad, and scale the
  video frame(s) received on its sink pad(s) to a different
  resolution output on its source pad(s). The range of
  supported scaling ratios is entity-specific and can differ
  between the horizontal and vertical directions (in particular
  scaling can be supported in one direction only). Binning and
  skipping are considered as scaling.

Although, this is vague, as it doesn't define what it means by "skipping",
whether that's skipping pixels (iow, sub-sampling) or whether that's
frame skipping.

Then there's the issue where, if you have this setup:

 camera --> csi2 receiver --> csi --> capture

and the "csi" subdev can skip frames, you need to know (a) at the CSI
sink pad what the frame rate is of the source (b) what the desired
source pad frame rate is, so you can configure the frame skipping.
So, does the csi subdev have to walk back through the media graph
looking for the frame rate?  Does the capture device have to walk back
through the media graph looking for some subdev to tell it what the
frame rate is - the capture device certainly can't go straight to the
sensor to get an answer to that question, because that bypasses the
effect of the CSI frame skipping (which will lower the frame rate.)

IMHO, frame rate is just another format property, just like the
resolution and data format itself, and v4l2 should be treating it no
differently.



I agree, frame 

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

[Russell King - ARM Linux]

On Tue, Feb 21, 2017 at 12:04:10AM +0200, Sakari Ailus wrote:
> On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote:
> > From: Russell King 
> > 
> > Setting and getting frame rates is part of the negotiation mechanism
> > between subdevs.  The lack of support means that a frame rate at the
> > sensor can't be negotiated through the subdev path.
> 
> Just wondering --- what do you need this for?

The v4l2 documentation contradicts the media-ctl implementation.

While v4l2 documentation says:

  These ioctls are used to get and set the frame interval at specific
  subdev pads in the image pipeline. The frame interval only makes sense
  for sub-devices that can control the frame period on their own. This
  includes, for instance, image sensors and TV tuners. Sub-devices that
  don't support frame intervals must not implement these ioctls.

However, when trying to configure the pipeline using media-ctl, eg:

media-ctl -d /dev/media1 --set-v4l2 '"imx219 pixel 
0-0010":0[crop:(0,0)/3264x2464]'
media-ctl -d /dev/media1 --set-v4l2 '"imx219 
0-0010":1[fmt:SRGGB10/3264x2464@1/30]'
media-ctl -d /dev/media1 --set-v4l2 '"imx219 0-0010":0[fmt:SRGGB8/816x616@1/30]'
media-ctl -d /dev/media1 --set-v4l2 
'"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
Unable to setup formats: Inappropriate ioctl for device (25)
media-ctl -d /dev/media1 --set-v4l2 '"ipu1_csi0_mux":2[fmt:SRGGB8/816x616@1/30]'
media-ctl -d /dev/media1 --set-v4l2 '"ipu1_csi0":2[fmt:SRGGB8/816x616@1/30]'

The problem there is that the format setting for the csi2 does not get
propagated forward:

$ strace media-ctl -d /dev/media1 --set-v4l2 
'"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
...
open("/dev/v4l-subdev16", O_RDWR)   = 3
ioctl(3, VIDIOC_SUBDEV_S_FMT, 0xbec16244) = 0
ioctl(3, VIDIOC_SUBDEV_S_FRAME_INTERVAL, 0xbec162a4) = -1 ENOTTY (Inappropriate
ioctl for device)
fstat64(1, {st_mode=S_IFCHR|0600, st_rdev=makedev(136, 2), ...}) = 0
write(1, "Unable to setup formats: Inappro"..., 61) = 61
Unable to setup formats: Inappropriate ioctl for device (25)
close(3)= 0
exit_group(1)   = ?
+++ exited with 1 +++

because media-ctl exits as soon as it encouters the error while trying
to set the frame rate.

This makes implementing setup of the media pipeline in shell scripts
unnecessarily difficult - as you need to then know whether an entity
is likely not to support the VIDIOC_SUBDEV_S_FRAME_INTERVAL call,
and either avoid specifying a frame rate:

$ strace media-ctl -d /dev/media1 --set-v4l2 
'"imx6-mipi-csi2":1[fmt:SRGGB8/816x616]'
...
open("/dev/v4l-subdev16", O_RDWR)   = 3
ioctl(3, VIDIOC_SUBDEV_S_FMT, 0xbeb1a254) = 0
open("/dev/v4l-subdev0", O_RDWR)= 4
ioctl(4, VIDIOC_SUBDEV_S_FMT, 0xbeb1a254) = 0
close(4)= 0
close(3)= 0
exit_group(0)   = ?
+++ exited with 0 +++

or manually setting the format on the sink.

Allowing the S_FRAME_INTERVAL call seems to me to be more in keeping
with the negotiation mechanism that is implemented in subdevs, and
IMHO should be implemented inside the kernel as a pad operation along
with the format negotiation, especially so as frame skipping is
defined as scaling, in just the same way as the frame size is also
scaling:

   -  ``MEDIA_ENT_F_PROC_VIDEO_SCALER``

   -  Video scaler. An entity capable of video scaling must have
  at least one sink pad and one source pad, and scale the
  video frame(s) received on its sink pad(s) to a different
  resolution output on its source pad(s). The range of
  supported scaling ratios is entity-specific and can differ
  between the horizontal and vertical directions (in particular
  scaling can be supported in one direction only). Binning and
  skipping are considered as scaling.

Although, this is vague, as it doesn't define what it means by "skipping",
whether that's skipping pixels (iow, sub-sampling) or whether that's
frame skipping.

Then there's the issue where, if you have this setup:

 camera --> csi2 receiver --> csi --> capture

and the "csi" subdev can skip frames, you need to know (a) at the CSI
sink pad what the frame rate is of the source (b) what the desired
source pad frame rate is, so you can configure the frame skipping.
So, does the csi subdev have to walk back through the media graph
looking for the frame rate?  Does the capture device have to walk back
through the media graph looking for some subdev to tell it what the
frame rate is - the capture device certainly can't go straight to the
sensor to get an answer to that question, because that bypasses the
effect of the CSI frame skipping (which will lower the frame rate.)

IMHO, frame rate is just another format property, just like the
resolution and data format itself, and v4l2 should be treating it no
differently.

In any case, the documentation vs media-ctl create 

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

[Steve Longerbeam]

On 02/20/2017 02:56 PM, Steve Longerbeam wrote:



On 02/20/2017 02:04 PM, Sakari Ailus wrote:

Hi Steve,

On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote:

From: Russell King 

Setting and getting frame rates is part of the negotiation mechanism
between subdevs.  The lack of support means that a frame rate at the
sensor can't be negotiated through the subdev path.


Just wondering --- what do you need this for?



Hi Sakari,

i.MX does need the ability to negotiate the frame rates in the
pipelines. The CSI has the ability to skip frames at the output,
which is something Philipp added to the CSI subdev. That affects
frame interval at the CSI output.

But as Russell pointed out, the lack of [gs]_frame_interval op
causes media-ctl to fail:

media-ctl -v -d /dev/media1 --set-v4l2
'"imx6-mipi-csi2":1[fmt:SGBRG8/512x512@1/30]'

Opening media device /dev/media1
Enumerating entities
Found 29 entities
Enumerating pads and links
Setting up format SGBRG8 512x512 on pad imx6-mipi-csi2/1
Format set: SGBRG8 512x512
Setting up frame interval 1/30 on entity imx6-mipi-csi2
Unable to set frame interval: Inappropriate ioctl for device (-25)Unable
to setup formats: Inappropriate ioctl for device (25)


So i.MX needs to implement this op in every subdev in the
pipeline, otherwise it's not possible to configure the
pipeline with media-ctl.




Hi Russell,

But Sakari brings up a good point. The mipi csi-2 receiver doesn't
have any control over frame rate, so why do you even need to
give it this information via media-ctl?

Steve

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

[Steve Longerbeam]

On 02/20/2017 02:04 PM, Sakari Ailus wrote:

Hi Steve,

On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote:

From: Russell King 

Setting and getting frame rates is part of the negotiation mechanism
between subdevs.  The lack of support means that a frame rate at the
sensor can't be negotiated through the subdev path.


Just wondering --- what do you need this for?



Hi Sakari,

i.MX does need the ability to negotiate the frame rates in the
pipelines. The CSI has the ability to skip frames at the output,
which is something Philipp added to the CSI subdev. That affects
frame interval at the CSI output.

But as Russell pointed out, the lack of [gs]_frame_interval op
causes media-ctl to fail:

media-ctl -v -d /dev/media1 --set-v4l2 
'"imx6-mipi-csi2":1[fmt:SGBRG8/512x512@1/30]'


Opening media device /dev/media1
Enumerating entities
Found 29 entities
Enumerating pads and links
Setting up format SGBRG8 512x512 on pad imx6-mipi-csi2/1
Format set: SGBRG8 512x512
Setting up frame interval 1/30 on entity imx6-mipi-csi2
Unable to set frame interval: Inappropriate ioctl for device (-25)Unable 
to setup formats: Inappropriate ioctl for device (25)



So i.MX needs to implement this op in every subdev in the
pipeline, otherwise it's not possible to configure the
pipeline with media-ctl.


Steve

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

[Sakari Ailus]

Hi Steve,

On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote:
> From: Russell King 
> 
> Setting and getting frame rates is part of the negotiation mechanism
> between subdevs.  The lack of support means that a frame rate at the
> sensor can't be negotiated through the subdev path.

Just wondering --- what do you need this for?

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

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

[Russell King - ARM Linux]

On Sat, Feb 18, 2017 at 09:29:17AM -0800, Steve Longerbeam wrote:
> On 02/18/2017 01:23 AM, Russell King - ARM Linux wrote:
> >On Fri, Feb 17, 2017 at 05:12:44PM -0800, Steve Longerbeam wrote:
> >>Hi Russell,
> >>
> >>I signed-off on this but after more review I'm not sure this is right.
> >>
> >>The CSI-2 receiver really has no control over frame rate. It's output
> >>frame rate is the same as the rate that is delivered to it.
> >>
> >>So this subdev should either not implement these ops, or it should
> >>refer them to the attached source subdev.
> >
> >Where in the V4L2 documentation does it say that is permissible?
> >
> 
> https://www.linuxtv.org/downloads/v4l-dvb-apis-old/vidioc-subdev-g-frame-interval.html
> 
> "The frame interval only makes sense for sub-devices that can control the
> frame period on their own. This includes, for instance, image sensors and TV
> tuners. Sub-devices that don't support frame intervals must not implement
> these ioctls."

That sounds clear - but the TV tuner example seems odd - the frame rate
is determined at transmission time, not reception time.  Yes, it's
possible to skip frames (which would be scaling) but you can't
_control_ the frame rate per se.

> >If you don't implement these, media-ctl fails to propagate _anything_
> >to the next sink pad if you specify a frame rate, because media-ctl
> >throws an error and exits immediately.
> >
> 
> But I agree with you here. I think our only option is to ignore that
> quoted requirement above and propagate [gs]_frame_interval all the way
> to the CSI (which can control the frame rate via frame skipping).

Sounds like something to tackle the media maintainers over - the
documentation vs media-ctl seem to have different ideas on this
point.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

[Russell King - ARM Linux]

On Fri, Feb 17, 2017 at 05:12:44PM -0800, Steve Longerbeam wrote:
> Hi Russell,
> 
> I signed-off on this but after more review I'm not sure this is right.
> 
> The CSI-2 receiver really has no control over frame rate. It's output
> frame rate is the same as the rate that is delivered to it.
> 
> So this subdev should either not implement these ops, or it should
> refer them to the attached source subdev.

Where in the V4L2 documentation does it say that is permissible?

If you don't implement these, media-ctl fails to propagate _anything_
to the next sink pad if you specify a frame rate, because media-ctl
throws an error and exits immediately.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

[Steve Longerbeam]

On 02/15/2017 06:19 PM, Steve Longerbeam wrote:

From: Russell King 

Setting and getting frame rates is part of the negotiation mechanism
between subdevs.  The lack of support means that a frame rate at the
sensor can't be negotiated through the subdev path.

Add support at MIPI CSI2 level for handling this part of the
negotiation.

Signed-off-by: Russell King 
Signed-off-by: Steve Longerbeam 



Hi Russell,

I signed-off on this but after more review I'm not sure this is right.

The CSI-2 receiver really has no control over frame rate. It's output
frame rate is the same as the rate that is delivered to it.

So this subdev should either not implement these ops, or it should
refer them to the attached source subdev.

Steve


---
 drivers/staging/media/imx/imx6-mipi-csi2.c | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c 
b/drivers/staging/media/imx/imx6-mipi-csi2.c
index 23dca80..c62f14e 100644
--- a/drivers/staging/media/imx/imx6-mipi-csi2.c
+++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
@@ -34,6 +34,7 @@ struct csi2_dev {
struct v4l2_subdev  sd;
struct media_pad   pad[CSI2_NUM_PADS];
struct v4l2_mbus_framefmt format_mbus;
+   struct v4l2_fract  frame_interval;
struct clk *dphy_clk;
struct clk *cfg_clk;
struct clk *pix_clk; /* what is this? */
@@ -397,6 +398,30 @@ static int csi2_set_fmt(struct v4l2_subdev *sd,
return 0;
 }

+static int csi2_g_frame_interval(struct v4l2_subdev *sd,
+struct v4l2_subdev_frame_interval *fi)
+{
+   struct csi2_dev *csi2 = sd_to_dev(sd);
+
+   fi->interval = csi2->frame_interval;
+
+   return 0;
+}
+
+static int csi2_s_frame_interval(struct v4l2_subdev *sd,
+struct v4l2_subdev_frame_interval *fi)
+{
+   struct csi2_dev *csi2 = sd_to_dev(sd);
+
+   /* Output pads mirror active input pad, no limits on input pads */
+   if (fi->pad != CSI2_SINK_PAD)
+   fi->interval = csi2->frame_interval;
+
+   csi2->frame_interval = fi->interval;
+
+   return 0;
+}
+
 /*
  * retrieve our pads parsed from the OF graph by the media device
  */
@@ -430,6 +455,8 @@ static struct v4l2_subdev_core_ops csi2_core_ops = {

 static struct v4l2_subdev_video_ops csi2_video_ops = {
.s_stream = csi2_s_stream,
+   .g_frame_interval = csi2_g_frame_interval,
+   .s_frame_interval = csi2_s_frame_interval,
 };

 static struct v4l2_subdev_pad_ops csi2_pad_ops = {



--
Steve Longerbeam | Senior Embedded Engineer, ESD Services
Mentor Embedded(tm) | 46871 Bayside Parkway, Fremont, CA 94538
P 510.354.5838 | M 408.410.2735

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

[Steve Longerbeam]

On 02/15/2017 06:19 PM, Steve Longerbeam wrote:

From: Russell King 

Setting and getting frame rates is part of the negotiation mechanism
between subdevs.  The lack of support means that a frame rate at the
sensor can't be negotiated through the subdev path.

Add support at MIPI CSI2 level for handling this part of the
negotiation.

Signed-off-by: Russell King 
Signed-off-by: Steve Longerbeam 



Hi Russell,

I signed-off on this but after more review I'm not sure this is right.

The CSI-2 receiver really has no control over frame rate. It's output
frame rate is the same as the rate that is delivered to it.

So this subdev should either not implement these ops, or it should
refer them to the attached source subdev.

Steve


---
 drivers/staging/media/imx/imx6-mipi-csi2.c | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c 
b/drivers/staging/media/imx/imx6-mipi-csi2.c
index 23dca80..c62f14e 100644
--- a/drivers/staging/media/imx/imx6-mipi-csi2.c
+++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
@@ -34,6 +34,7 @@ struct csi2_dev {
struct v4l2_subdev  sd;
struct media_pad   pad[CSI2_NUM_PADS];
struct v4l2_mbus_framefmt format_mbus;
+   struct v4l2_fract  frame_interval;
struct clk *dphy_clk;
struct clk *cfg_clk;
struct clk *pix_clk; /* what is this? */
@@ -397,6 +398,30 @@ static int csi2_set_fmt(struct v4l2_subdev *sd,
return 0;
 }

+static int csi2_g_frame_interval(struct v4l2_subdev *sd,
+struct v4l2_subdev_frame_interval *fi)
+{
+   struct csi2_dev *csi2 = sd_to_dev(sd);
+
+   fi->interval = csi2->frame_interval;
+
+   return 0;
+}
+
+static int csi2_s_frame_interval(struct v4l2_subdev *sd,
+struct v4l2_subdev_frame_interval *fi)
+{
+   struct csi2_dev *csi2 = sd_to_dev(sd);
+
+   /* Output pads mirror active input pad, no limits on input pads */
+   if (fi->pad != CSI2_SINK_PAD)
+   fi->interval = csi2->frame_interval;
+
+   csi2->frame_interval = fi->interval;
+
+   return 0;
+}
+
 /*
  * retrieve our pads parsed from the OF graph by the media device
  */
@@ -430,6 +455,8 @@ static struct v4l2_subdev_core_ops csi2_core_ops = {

 static struct v4l2_subdev_video_ops csi2_video_ops = {
.s_stream = csi2_s_stream,
+   .g_frame_interval = csi2_g_frame_interval,
+   .s_frame_interval = csi2_s_frame_interval,
 };

 static struct v4l2_subdev_pad_ops csi2_pad_ops = {



--
Steve Longerbeam | Senior Embedded Engineer, ESD Services
Mentor Embedded(tm) | 46871 Bayside Parkway, Fremont, CA 94538
P 510.354.5838 | M 408.410.2735

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

[Steve Longerbeam]

From: Russell King 

Setting and getting frame rates is part of the negotiation mechanism
between subdevs.  The lack of support means that a frame rate at the
sensor can't be negotiated through the subdev path.

Add support at MIPI CSI2 level for handling this part of the
negotiation.

Signed-off-by: Russell King 
Signed-off-by: Steve Longerbeam 
---
 drivers/staging/media/imx/imx6-mipi-csi2.c | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c 
b/drivers/staging/media/imx/imx6-mipi-csi2.c
index 23dca80..c62f14e 100644
--- a/drivers/staging/media/imx/imx6-mipi-csi2.c
+++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
@@ -34,6 +34,7 @@ struct csi2_dev {
struct v4l2_subdev  sd;
struct media_pad   pad[CSI2_NUM_PADS];
struct v4l2_mbus_framefmt format_mbus;
+   struct v4l2_fract  frame_interval;
struct clk *dphy_clk;
struct clk *cfg_clk;
struct clk *pix_clk; /* what is this? */
@@ -397,6 +398,30 @@ static int csi2_set_fmt(struct v4l2_subdev *sd,
return 0;
 }
 
+static int csi2_g_frame_interval(struct v4l2_subdev *sd,
+struct v4l2_subdev_frame_interval *fi)
+{
+   struct csi2_dev *csi2 = sd_to_dev(sd);
+
+   fi->interval = csi2->frame_interval;
+
+   return 0;
+}
+
+static int csi2_s_frame_interval(struct v4l2_subdev *sd,
+struct v4l2_subdev_frame_interval *fi)
+{
+   struct csi2_dev *csi2 = sd_to_dev(sd);
+
+   /* Output pads mirror active input pad, no limits on input pads */
+   if (fi->pad != CSI2_SINK_PAD)
+   fi->interval = csi2->frame_interval;
+
+   csi2->frame_interval = fi->interval;
+
+   return 0;
+}
+
 /*
  * retrieve our pads parsed from the OF graph by the media device
  */
@@ -430,6 +455,8 @@ static struct v4l2_subdev_core_ops csi2_core_ops = {
 
 static struct v4l2_subdev_video_ops csi2_video_ops = {
.s_stream = csi2_s_stream,
+   .g_frame_interval = csi2_g_frame_interval,
+   .s_frame_interval = csi2_s_frame_interval,
 };
 
 static struct v4l2_subdev_pad_ops csi2_pad_ops = {
-- 
2.7.4