Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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: sakari.ai...@iki.fi XMPP: sai...@retiisi.
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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. http://hverkuil.home.xs4all.nl/spec/uapi/v4l/extended-controls.html#image-source-control-ids> > > > > $ 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 "solutio
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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, VIDIOC_SUBDEV_S_FMT, 0xbeb1a254) = 0 >
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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 image sizes (i.e. not frame
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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 back through the media graph >
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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 rate, if indicated/specified
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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 something of a very obscure situ
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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." 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). Steve ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel