Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1599348 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and  getting of frame rates Sakari Ailus <sakari.ailus@iki.fi> - 2017-03-13 14:20 +0100
    Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and  getting of frame rates Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-13 14:30 +0100
      Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and  getting of frame rates Philipp Zabel <p.zabel@pengutronix.de> - 2017-03-13 15:00 +0100
        Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and  getting of frame rates Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-13 19:10 +0100
          Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and  getting of frame rates Sakari Ailus <sakari.ailus@iki.fi> - 2017-03-13 22:10 +0100
            Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and  getting of frame rates Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-13 22:40 +0100
            Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and  getting of frame rates Hans Verkuil <hverkuil@xs4all.nl> - 2017-03-14 08:40 +0100
              Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and  getting of frame rates Philipp Zabel <p.zabel@pengutronix.de> - 2017-03-14 11:50 +0100
      Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and  getting of frame rates Sakari Ailus <sakari.ailus@iki.fi> - 2017-03-13 22:00 +0100
        Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and  getting of frame rates Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-13 22:10 +0100

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

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.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [next] | [standalone]

#1599352

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.

[toc] | [prev] | [next] | [standalone]

#1599395

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.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1599668

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.

Steve

[toc] | [prev] | [next] | [standalone]

#1599827

Hi Steve,

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

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

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

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

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

#1599847

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.

[toc] | [prev] | [next] | [standalone]

#1600080

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

[toc] | [prev] | [next] | [standalone]

#1600207

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

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

[toc] | [prev] | [next] | [standalone]

#1599812

Hi Russell,

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

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.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

#1599815

On Mon, Mar 13, 2017 at 10:56:46PM +0200, Sakari Ailus wrote:
> Hi Russell,
> 
> On Mon, Mar 13, 2017 at 01:27:02PM +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.
> 
> 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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web