| Message ID | 1487211578-11360-30-git-send-email-steve_longerbeam@mentor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/15/2017 06:19 PM, Steve Longerbeam wrote: > From: Russell King <rmk+kernel@armlinux.org.uk> > > 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 <rmk+kernel@armlinux.org.uk> > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> 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 = { >
On 02/15/2017 06:19 PM, Steve Longerbeam wrote: > From: Russell King <rmk+kernel@armlinux.org.uk> > > 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 <rmk+kernel@armlinux.org.uk> > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> 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 = { >
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.
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
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.
Hi Steve, On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote: > From: Russell King <rmk+kernel@armlinux.org.uk> > > 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?
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 <rmk+kernel@armlinux.org.uk> >> >> 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
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 <rmk+kernel@armlinux.org.uk> >>> >>> 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
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 <rmk+kernel@armlinux.org.uk> > > > > 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 situation, one that probably needs solving one way or another.
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 <rmk+kernel@armlinux.org.uk> > >> > >>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.
Hi Russell, On Tue, Feb 21, 2017 at 12:13:32AM +0000, 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 <rmk+kernel@armlinux.org.uk> > > > > > > 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 rates). > > - ``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. Skipping in the context is used to refer to sub-sampling. The term is often used in conjunction of sensors. The documentation could certainly be clarified here. > > 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 situation, one that probably needs solving one way or another. Before going to solutions I need to ask: what do you want to achieve?
On Tue, Feb 21, 2017 at 02:37:57PM +0200, Sakari Ailus wrote: > Hi Russell, > > On Tue, Feb 21, 2017 at 12:13:32AM +0000, 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 <rmk+kernel@armlinux.org.uk> > > > > > > > > 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 > > 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 rates). So what about the case where we have a subdev (CSI) that is capable of frame rate reduction, that needs to know the input frame rate and the desired output frame rate? It seems to me that this has not been thought through... > > - ``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. > > Skipping in the context is used to refer to sub-sampling. The term is often > used in conjunction of sensors. The documentation could certainly be > clarified here. It definitely needs to be, it's currently mis-leading. > > 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 situation, one that probably needs solving one way or another. > > Before going to solutions I need to ask: what do you want to achieve? Full and consistent support for the hardware, and a sane and consistent way to setup a media pipeline that is easy for everyone to understand.
Hi Russell, On Tue, Feb 21, 2017 at 01:21:32PM +0000, 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 +0000, 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 <rmk+kernel@armlinux.org.uk> > > > > > > > > > > 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. <URL: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 "solution". I have to disagree with that: if a piece of hardware does not offer to control or, if a concept is not even relevant for a piece of hardware, then a driver for that piece of hardware should not expose an interface to control such a feature. Doing so would provide no value and at the same time would be simply confusing for the user space. > > 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. Pipeline configuration as such is highly hardware specific. There are rules, but there are details in hardware that have to be taken into account, such as mandated cropping in certain situations. You have to simply accept that: when it comes to camera Image Signal Processors, there are no standard pipelines. Each ISP is different from the rest, more or less, and often more so. As the interface is generic, you can write generic programs that use that interface, but you need to be able to adapt to the differences in the functionality of the hardware. Frankly, I think this is just needless noise stemming from a problem that's not really difficult to solve --- if that technical problem really even exists. But let's not debate that; I accept that dropping frames is what you're willing to do. > > 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 > > > 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 rates). > > So what about the case where we have a subdev (CSI) that is capable of > frame rate reduction, that needs to know the input frame rate and the > desired output frame rate? It seems to me that this has not been > thought through... That's because I believe you're the first one wanting to willfully throw away perfectly good frames. :-) If you want to do that, a simple option could be to just support [GS]_FRAME_INTERVAL on all pads of a sub-device that can drop frames. But it should not be include in pipeline validation, it simply does not belong there for the reasons stated previously. The user would be responsible for configuring the frame rates right. That information would simply be used to configure frame dropping frequency. I'd like to have a comment from Laurent and Hans on this. > > > > - ``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. > > > > Skipping in the context is used to refer to sub-sampling. The term is often > > used in conjunction of sensors. The documentation could certainly be > > clarified here. > > It definitely needs to be, it's currently mis-leading. If you're not familiar with terminology typically used with many camera sensor, perhaps so. The documentation should indeed not assume that; I'll submit a patch to fix that. > > > > 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 situation, one that probably needs solving one way or another. > > > > Before going to solutions I need to ask: what do you want to achieve? > > Full and consistent support for the hardware, and a sane and consistent > way to setup a media pipeline that is easy for everyone to understand. That's essentially what we do have: the same interface is supported on a large variety of different hardware devices. However, not all IOCTLs are supported by all device drivers.
On Tue, Feb 21, 2017 at 05:38:34PM +0200, Sakari Ailus wrote: > Hi Russell, > > On Tue, Feb 21, 2017 at 01:21:32PM +0000, 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 +0000, 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 <rmk+kernel@armlinux.org.uk> > > > > > > > > > > > > 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.
On Tue, Feb 21, 2017 at 04:03:32PM +0000, 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 +0000, 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 +0000, 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 <rmk+kernel@armlinux.org.uk> > > > > > > > > > > > > > > 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.
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 <rmk+kernel@armlinux.org.uk> >>>> >>>> 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
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 <rmk+kernel@armlinux.org.uk> >>>>> >>>>> 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
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 = {