| Message ID | 1491486929.2392.29.camel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 06, 2017 at 03:55:29PM +0200, Philipp Zabel wrote: > + > + /* Retain current field setting as default */ > + if (sdformat->format.field == V4L2_FIELD_ANY) > + sdformat->format.field = fmt->field; > + > + /* Retain current colorspace setting as default */ > + if (sdformat->format.colorspace == V4L2_COLORSPACE_DEFAULT) { > + sdformat->format.colorspace = fmt->colorspace; > + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) > + sdformat->format.xfer_func = fmt->xfer_func; > + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) > + sdformat->format.ycbcr_enc = fmt->ycbcr_enc; > + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) > + sdformat->format.quantization = fmt->quantization; > + } else { > + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) { > + sdformat->format.xfer_func = > + V4L2_MAP_XFER_FUNC_DEFAULT( > + sdformat->format.colorspace); > + } > + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) { > + sdformat->format.ycbcr_enc = > + V4L2_MAP_YCBCR_ENC_DEFAULT( > + sdformat->format.colorspace); > + } > + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) { > + sdformat->format.quantization = > + V4L2_MAP_QUANTIZATION_DEFAULT( > + cc->cs != IPUV3_COLORSPACE_YUV, > + sdformat->format.colorspace, > + sdformat->format.ycbcr_enc); > + } > + } Would it make sense for this to be a helper function?
On 04/06/2017 03:55 PM, Philipp Zabel wrote: > If the the field order is set to ANY in set_fmt, choose the currently > set field order. If the colorspace is set to DEFAULT, choose the current > colorspace. If any of xfer_func, ycbcr_enc or quantization are set to > DEFAULT, either choose the current setting, or the default setting for the > new colorspace, if non-DEFAULT colorspace was given. > > This allows to let field order and colorimetry settings be propagated > from upstream by calling media-ctl on the upstream entity source pad, > and then call media-ctl on the sink pad to manually set the input frame > interval, without changing the already set field order and colorimetry > information. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > This is based on imx-media-staging-md-v14, and it is supposed to allow > configuring the pipeline with media-ctl like this: > > 1) media-ctl --set-v4l2 "'tc358743 1-000f':0[fmt:UYVY8_1X16/1920x1080]" > 2) media-ctl --set-v4l2 "'imx6-mipi-csi2':1[fmt:UYVY8_1X16/1920x108]" > 3) media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY8_1X16/1920x1080]" > 4) media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY8_1X16/1920x1080@1/60]" > 5) media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/1920x1080@1/30]" > > Without having step 4) overwrite the colorspace and field order set on > 'ipu1_csi0':0 by the propagation in step 3). > --- > drivers/staging/media/imx/imx-media-csi.c | 34 +++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c > index 64dc454f6b371..d94ce1de2bf05 100644 > --- a/drivers/staging/media/imx/imx-media-csi.c > +++ b/drivers/staging/media/imx/imx-media-csi.c > @@ -1325,6 +1325,40 @@ static int csi_set_fmt(struct v4l2_subdev *sd, > csi_try_fmt(priv, sensor, cfg, sdformat, crop, compose, &cc); > > fmt = __csi_get_fmt(priv, cfg, sdformat->pad, sdformat->which); > + > + /* Retain current field setting as default */ > + if (sdformat->format.field == V4L2_FIELD_ANY) > + sdformat->format.field = fmt->field; sdformat->format.field should never be FIELD_ANY. If it is, then that's a subdev bug and I'm pretty sure FIELD_NONE was intended. > + > + /* Retain current colorspace setting as default */ > + if (sdformat->format.colorspace == V4L2_COLORSPACE_DEFAULT) { > + sdformat->format.colorspace = fmt->colorspace; No! Subdevs should never return COLORSPACE_DEFAULT. If they do, then fix them. If this happens a lot (I'm not sure how reliably subdevs fill this in) you could set it to COLORSPACE_RAW. Perhaps with a WARN_ON_ONCE. > + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) > + sdformat->format.xfer_func = fmt->xfer_func; > + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) > + sdformat->format.ycbcr_enc = fmt->ycbcr_enc; > + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) > + sdformat->format.quantization = fmt->quantization; Nack. This is meaningless. > + } else { > + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) { > + sdformat->format.xfer_func = > + V4L2_MAP_XFER_FUNC_DEFAULT( > + sdformat->format.colorspace); > + } > + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) { > + sdformat->format.ycbcr_enc = > + V4L2_MAP_YCBCR_ENC_DEFAULT( > + sdformat->format.colorspace); > + } > + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) { > + sdformat->format.quantization = > + V4L2_MAP_QUANTIZATION_DEFAULT( > + cc->cs != IPUV3_COLORSPACE_YUV, > + sdformat->format.colorspace, > + sdformat->format.ycbcr_enc); > + } This isn't wrong, but it is perfectly fine to keep the DEFAULT here and let the application call V4L2_MAP_. I get the feeling this patch is a workaround for subdev errors. Either that, or the commit log doesn't give me enough information to really understand the problem that's being addressed here. Regards, Hans > + } > + > *fmt = sdformat->format; > > if (sdformat->pad == CSI_SINK_PAD) { >
On Thu, Apr 06, 2017 at 04:20:21PM +0200, Hans Verkuil wrote: > On 04/06/2017 03:55 PM, Philipp Zabel wrote: > > If the the field order is set to ANY in set_fmt, choose the currently > > set field order. If the colorspace is set to DEFAULT, choose the current > > colorspace. If any of xfer_func, ycbcr_enc or quantization are set to > > DEFAULT, either choose the current setting, or the default setting for the > > new colorspace, if non-DEFAULT colorspace was given. > > > > This allows to let field order and colorimetry settings be propagated > > from upstream by calling media-ctl on the upstream entity source pad, > > and then call media-ctl on the sink pad to manually set the input frame > > interval, without changing the already set field order and colorimetry > > information. > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > This is based on imx-media-staging-md-v14, and it is supposed to allow > > configuring the pipeline with media-ctl like this: > > > > 1) media-ctl --set-v4l2 "'tc358743 1-000f':0[fmt:UYVY8_1X16/1920x1080]" > > 2) media-ctl --set-v4l2 "'imx6-mipi-csi2':1[fmt:UYVY8_1X16/1920x108]" > > 3) media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY8_1X16/1920x1080]" > > 4) media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY8_1X16/1920x1080@1/60]" > > 5) media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/1920x1080@1/30]" > > > > Without having step 4) overwrite the colorspace and field order set on > > 'ipu1_csi0':0 by the propagation in step 3). > > --- > > drivers/staging/media/imx/imx-media-csi.c | 34 +++++++++++++++++++++++++++++++ > > 1 file changed, 34 insertions(+) > > > > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c > > index 64dc454f6b371..d94ce1de2bf05 100644 > > --- a/drivers/staging/media/imx/imx-media-csi.c > > +++ b/drivers/staging/media/imx/imx-media-csi.c > > @@ -1325,6 +1325,40 @@ static int csi_set_fmt(struct v4l2_subdev *sd, > > csi_try_fmt(priv, sensor, cfg, sdformat, crop, compose, &cc); > > > > fmt = __csi_get_fmt(priv, cfg, sdformat->pad, sdformat->which); > > + > > + /* Retain current field setting as default */ > > + if (sdformat->format.field == V4L2_FIELD_ANY) > > + sdformat->format.field = fmt->field; > > sdformat->format.field should never be FIELD_ANY. If it is, then that's a > subdev bug and I'm pretty sure FIELD_NONE was intended. Please explain further - sdformat->format.field is the field passed _in_ from userspace, and from what I can see, userspace is free to pass in any value through the ioctl as check_format() does nothing to validate that the various format.* fields are sane. This patch is detecting that the user is requesting FIELD_ANY, and fixing it up. Surely that's the right thing to do, and is way more preferable than just accepting the FIELD_ANY from userspace and storing that value?
On Thu, 2017-04-06 at 16:20 +0200, Hans Verkuil wrote: > On 04/06/2017 03:55 PM, Philipp Zabel wrote: > > If the the field order is set to ANY in set_fmt, choose the currently > > set field order. If the colorspace is set to DEFAULT, choose the current > > colorspace. If any of xfer_func, ycbcr_enc or quantization are set to > > DEFAULT, either choose the current setting, or the default setting for the > > new colorspace, if non-DEFAULT colorspace was given. > > > > This allows to let field order and colorimetry settings be propagated > > from upstream by calling media-ctl on the upstream entity source pad, > > and then call media-ctl on the sink pad to manually set the input frame > > interval, without changing the already set field order and colorimetry > > information. > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > This is based on imx-media-staging-md-v14, and it is supposed to allow > > configuring the pipeline with media-ctl like this: > > > > 1) media-ctl --set-v4l2 "'tc358743 1-000f':0[fmt:UYVY8_1X16/1920x1080]" > > 2) media-ctl --set-v4l2 "'imx6-mipi-csi2':1[fmt:UYVY8_1X16/1920x108]" > > 3) media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY8_1X16/1920x1080]" > > 4) media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY8_1X16/1920x1080@1/60]" > > 5) media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/1920x1080@1/30]" > > > > Without having step 4) overwrite the colorspace and field order set on > > 'ipu1_csi0':0 by the propagation in step 3). > > --- > > drivers/staging/media/imx/imx-media-csi.c | 34 +++++++++++++++++++++++++++++++ > > 1 file changed, 34 insertions(+) > > > > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c > > index 64dc454f6b371..d94ce1de2bf05 100644 > > --- a/drivers/staging/media/imx/imx-media-csi.c > > +++ b/drivers/staging/media/imx/imx-media-csi.c > > @@ -1325,6 +1325,40 @@ static int csi_set_fmt(struct v4l2_subdev *sd, > > csi_try_fmt(priv, sensor, cfg, sdformat, crop, compose, &cc); > > > > fmt = __csi_get_fmt(priv, cfg, sdformat->pad, sdformat->which); > > + > > + /* Retain current field setting as default */ > > + if (sdformat->format.field == V4L2_FIELD_ANY) > > + sdformat->format.field = fmt->field; > > sdformat->format.field should never be FIELD_ANY. If it is, then that's a > subdev bug and I'm pretty sure FIELD_NONE was intended. This is the subdev. sdformat is passed in from userspace, so we have to deal with it being set to ANY. I'm trying hard right now not to return ANY though. The values in sdformat->format are applied to fmt down below. > > + > > + /* Retain current colorspace setting as default */ > > + if (sdformat->format.colorspace == V4L2_COLORSPACE_DEFAULT) { > > + sdformat->format.colorspace = fmt->colorspace; > > No! Subdevs should never return COLORSPACE_DEFAULT. If they do, then fix > them. If this happens a lot (I'm not sure how reliably subdevs fill this > in) you could set it to COLORSPACE_RAW. Perhaps with a WARN_ON_ONCE. Same here, if userspace calls VIDIOC_SUBDEV_S_FMT with DEFAULT colorspace, I don't want to return that unchanged, but overwrite it with the currently set value. > > + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) > > + sdformat->format.xfer_func = fmt->xfer_func; > > + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) > > + sdformat->format.ycbcr_enc = fmt->ycbcr_enc; > > + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) > > + sdformat->format.quantization = fmt->quantization; > > Nack. This is meaningless. It isn't. It may be wrong, but it is not without effect. If the colorimetry info (in fmt) is currently set to something that is non-standard, calling VIDIOC_SUBDEV_S_FMT with DEFAULT in xfer_func, ycbcr_enc or quantization will cause those old values to be retained, instead of overwriting them to DEFAULT. > > + } else { > > + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) { > > + sdformat->format.xfer_func = > > + V4L2_MAP_XFER_FUNC_DEFAULT( > > + sdformat->format.colorspace); > > + } > > + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) { > > + sdformat->format.ycbcr_enc = > > + V4L2_MAP_YCBCR_ENC_DEFAULT( > > + sdformat->format.colorspace); > > + } > > + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) { > > + sdformat->format.quantization = > > + V4L2_MAP_QUANTIZATION_DEFAULT( > > + cc->cs != IPUV3_COLORSPACE_YUV, > > + sdformat->format.colorspace, > > + sdformat->format.ycbcr_enc); > > + } > > This isn't wrong, but it is perfectly fine to keep the DEFAULT here and let > the application call V4L2_MAP_. > > I get the feeling this patch is a workaround for subdev errors. Either that, > or the commit log doesn't give me enough information to really understand the > problem that's being addressed here. It's the latter. I should have written VIDIOC_SUBDEV_S_FMT instead of just set_fmt in the comment. > Regards, > > Hans > > > + } > > + > > *fmt = sdformat->format; Here sdformat is applied to the subdev pad. regards Philipp
On Thu, 2017-04-06 at 15:05 +0100, Russell King - ARM Linux wrote: > On Thu, Apr 06, 2017 at 03:55:29PM +0200, Philipp Zabel wrote: > > + > > + /* Retain current field setting as default */ > > + if (sdformat->format.field == V4L2_FIELD_ANY) > > + sdformat->format.field = fmt->field; > > + > > + /* Retain current colorspace setting as default */ > > + if (sdformat->format.colorspace == V4L2_COLORSPACE_DEFAULT) { > > + sdformat->format.colorspace = fmt->colorspace; > > + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) > > + sdformat->format.xfer_func = fmt->xfer_func; > > + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) > > + sdformat->format.ycbcr_enc = fmt->ycbcr_enc; > > + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) > > + sdformat->format.quantization = fmt->quantization; > > + } else { > > + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) { > > + sdformat->format.xfer_func = > > + V4L2_MAP_XFER_FUNC_DEFAULT( > > + sdformat->format.colorspace); > > + } > > + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) { > > + sdformat->format.ycbcr_enc = > > + V4L2_MAP_YCBCR_ENC_DEFAULT( > > + sdformat->format.colorspace); > > + } > > + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) { > > + sdformat->format.quantization = > > + V4L2_MAP_QUANTIZATION_DEFAULT( > > + cc->cs != IPUV3_COLORSPACE_YUV, > > + sdformat->format.colorspace, > > + sdformat->format.ycbcr_enc); > > + } > > + } > > Would it make sense for this to be a helper function? Quite possible, the next subdev that has to set frame_interval on both pads manually because its upstream source pad doesn't suport frame_interval might want to do the same. regards Philipp
On Thu, Apr 06, 2017 at 05:01:52PM +0200, Philipp Zabel wrote: > On Thu, 2017-04-06 at 15:05 +0100, Russell King - ARM Linux wrote: > > On Thu, Apr 06, 2017 at 03:55:29PM +0200, Philipp Zabel wrote: > > > + > > > + /* Retain current field setting as default */ > > > + if (sdformat->format.field == V4L2_FIELD_ANY) > > > + sdformat->format.field = fmt->field; > > > + > > > + /* Retain current colorspace setting as default */ > > > + if (sdformat->format.colorspace == V4L2_COLORSPACE_DEFAULT) { > > > + sdformat->format.colorspace = fmt->colorspace; > > > + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) > > > + sdformat->format.xfer_func = fmt->xfer_func; > > > + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) > > > + sdformat->format.ycbcr_enc = fmt->ycbcr_enc; > > > + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) > > > + sdformat->format.quantization = fmt->quantization; > > > + } else { > > > + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) { > > > + sdformat->format.xfer_func = > > > + V4L2_MAP_XFER_FUNC_DEFAULT( > > > + sdformat->format.colorspace); > > > + } > > > + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) { > > > + sdformat->format.ycbcr_enc = > > > + V4L2_MAP_YCBCR_ENC_DEFAULT( > > > + sdformat->format.colorspace); > > > + } > > > + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) { > > > + sdformat->format.quantization = > > > + V4L2_MAP_QUANTIZATION_DEFAULT( > > > + cc->cs != IPUV3_COLORSPACE_YUV, > > > + sdformat->format.colorspace, > > > + sdformat->format.ycbcr_enc); > > > + } > > > + } > > > > Would it make sense for this to be a helper function? > > Quite possible, the next subdev that has to set frame_interval on both > pads manually because its upstream source pad doesn't suport > frame_interval might want to do the same. Hmm. I'm not sure I agree with this approach. If a subdev hardware does not support any modification of the colourspace or field, then it should not be modifyable at the source pad - it should retain the propagated settings from the sink pad. I thought I had already sent a patch doing exactly that.
On Thu, 2017-04-06 at 16:20 +0200, Hans Verkuil wrote: > On 04/06/2017 03:55 PM, Philipp Zabel wrote: > > If the the field order is set to ANY in set_fmt, choose the currently > > set field order. If the colorspace is set to DEFAULT, choose the current > > colorspace. If any of xfer_func, ycbcr_enc or quantization are set to > > DEFAULT, either choose the current setting, or the default setting for the > > new colorspace, if non-DEFAULT colorspace was given. > > > > This allows to let field order and colorimetry settings be propagated > > from upstream by calling media-ctl on the upstream entity source pad, > > and then call media-ctl on the sink pad to manually set the input frame > > interval, without changing the already set field order and colorimetry > > information. > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > This is based on imx-media-staging-md-v14, and it is supposed to allow > > configuring the pipeline with media-ctl like this: > > > > 1) media-ctl --set-v4l2 "'tc358743 1-000f':0[fmt:UYVY8_1X16/1920x1080]" > > 2) media-ctl --set-v4l2 "'imx6-mipi-csi2':1[fmt:UYVY8_1X16/1920x108]" > > 3) media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY8_1X16/1920x1080]" To be more clear, this media-ctl command will call VIDIOC_SUBDEV_S_FMT on the 'ipu1_csi0':0 sink pad during the propagation phase, with field and colorspace set to whatever was propagated from the 'tc358743 1-000f':0 pad in the previous steps (as returned by VIDIOC_SUBDEV_G_FMT on the 'ipu1_csi0_mux':2 source pad). > > 4) media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY8_1X16/1920x1080@1/60]" And this media-ctl command will call VIDIOC_SUBDEV_S_FMT on the 'ipu1_csi0':0 sink pad with V4L2_FIELD_ANY (since there is no "field:" set), and with V4L2_COLORSPACE_DEFAULT (since there is no colorspace:" set) and so on. I don't want the correct values from step 3) to be replaced with DEFAULT here. > > 5) media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/1920x1080@1/30]" > > > > Without having step 4) overwrite the colorspace and field order set on > > 'ipu1_csi0':0 by the propagation in step 3). > > --- > > drivers/staging/media/imx/imx-media-csi.c | 34 +++++++++++++++++++++++++++++++ > > 1 file changed, 34 insertions(+) > > > > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c > > index 64dc454f6b371..d94ce1de2bf05 100644 > > --- a/drivers/staging/media/imx/imx-media-csi.c > > +++ b/drivers/staging/media/imx/imx-media-csi.c > > @@ -1325,6 +1325,40 @@ static int csi_set_fmt(struct v4l2_subdev *sd, So the VIDIOC_SUBDEV_S_FMT callback has to check for DEFAULT values and do something about them. > > csi_try_fmt(priv, sensor, cfg, sdformat, crop, compose, &cc); > > > > fmt = __csi_get_fmt(priv, cfg, sdformat->pad, sdformat->which); > > + > > + /* Retain current field setting as default */ > > + if (sdformat->format.field == V4L2_FIELD_ANY) > > + sdformat->format.field = fmt->field; > > sdformat->format.field should never be FIELD_ANY. If it is, then that's a > subdev bug and I'm pretty sure FIELD_NONE was intended. > > > + > > + /* Retain current colorspace setting as default */ > > + if (sdformat->format.colorspace == V4L2_COLORSPACE_DEFAULT) { > > + sdformat->format.colorspace = fmt->colorspace; > > No! Subdevs should never return COLORSPACE_DEFAULT. If they do, then fix > them. If this happens a lot (I'm not sure how reliably subdevs fill this > in) you could set it to COLORSPACE_RAW. Perhaps with a WARN_ON_ONCE. > > > + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) > > + sdformat->format.xfer_func = fmt->xfer_func; > > + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) > > + sdformat->format.ycbcr_enc = fmt->ycbcr_enc; > > + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) > > + sdformat->format.quantization = fmt->quantization; > > Nack. This is meaningless. > > > + } else { > > + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) { > > + sdformat->format.xfer_func = > > + V4L2_MAP_XFER_FUNC_DEFAULT( > > + sdformat->format.colorspace); > > + } > > + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) { > > + sdformat->format.ycbcr_enc = > > + V4L2_MAP_YCBCR_ENC_DEFAULT( > > + sdformat->format.colorspace); > > + } > > + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) { > > + sdformat->format.quantization = > > + V4L2_MAP_QUANTIZATION_DEFAULT( > > + cc->cs != IPUV3_COLORSPACE_YUV, > > + sdformat->format.colorspace, > > + sdformat->format.ycbcr_enc); > > + } > > This isn't wrong, but it is perfectly fine to keep the DEFAULT here and let > the application call V4L2_MAP_. > > I get the feeling this patch is a workaround for subdev errors. Either that, > or the commit log doesn't give me enough information to really understand the > problem that's being addressed here. > > Regards, > > Hans > > > + } > > + > > *fmt = sdformat->format; > > > > if (sdformat->pad == CSI_SINK_PAD) { > > regards Philipp
On Thu, 2017-04-06 at 16:10 +0100, Russell King - ARM Linux wrote: > On Thu, Apr 06, 2017 at 05:01:52PM +0200, Philipp Zabel wrote: > > On Thu, 2017-04-06 at 15:05 +0100, Russell King - ARM Linux wrote: > > > On Thu, Apr 06, 2017 at 03:55:29PM +0200, Philipp Zabel wrote: > > > > + > > > > + /* Retain current field setting as default */ > > > > + if (sdformat->format.field == V4L2_FIELD_ANY) > > > > + sdformat->format.field = fmt->field; > > > > + > > > > + /* Retain current colorspace setting as default */ > > > > + if (sdformat->format.colorspace == V4L2_COLORSPACE_DEFAULT) { > > > > + sdformat->format.colorspace = fmt->colorspace; > > > > + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) > > > > + sdformat->format.xfer_func = fmt->xfer_func; > > > > + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) > > > > + sdformat->format.ycbcr_enc = fmt->ycbcr_enc; > > > > + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) > > > > + sdformat->format.quantization = fmt->quantization; > > > > + } else { > > > > + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) { > > > > + sdformat->format.xfer_func = > > > > + V4L2_MAP_XFER_FUNC_DEFAULT( > > > > + sdformat->format.colorspace); > > > > + } > > > > + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) { > > > > + sdformat->format.ycbcr_enc = > > > > + V4L2_MAP_YCBCR_ENC_DEFAULT( > > > > + sdformat->format.colorspace); > > > > + } > > > > + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) { > > > > + sdformat->format.quantization = > > > > + V4L2_MAP_QUANTIZATION_DEFAULT( > > > > + cc->cs != IPUV3_COLORSPACE_YUV, > > > > + sdformat->format.colorspace, > > > > + sdformat->format.ycbcr_enc); > > > > + } > > > > + } > > > > > > Would it make sense for this to be a helper function? > > > > Quite possible, the next subdev that has to set frame_interval on both > > pads manually because its upstream source pad doesn't suport > > frame_interval might want to do the same. > > Hmm. I'm not sure I agree with this approach. If a subdev hardware > does not support any modification of the colourspace or field, then > it should not be modifyable at the source pad - it should retain the > propagated settings from the sink pad. This new code is only relevant for the CSI_SINK_PAD. > I thought I had already sent a patch doing exactly that. Yes. Right above the modification there is a call to csi_try_fmt which will already fix up sdformat->format for the source pads. So for the CSI_SRC_PAD_DIRECT and CSI_SRC_PAD_IDMAC this should amount to a no-op. If might be better to move this into a separate function and only call it if sdformat->pad == CSI_SINK_PAD. regards Philipp
On 04/06/2017 04:54 PM, Philipp Zabel wrote: > On Thu, 2017-04-06 at 16:20 +0200, Hans Verkuil wrote: >> On 04/06/2017 03:55 PM, Philipp Zabel wrote: >>> If the the field order is set to ANY in set_fmt, choose the currently >>> set field order. If the colorspace is set to DEFAULT, choose the current >>> colorspace. If any of xfer_func, ycbcr_enc or quantization are set to >>> DEFAULT, either choose the current setting, or the default setting for the >>> new colorspace, if non-DEFAULT colorspace was given. >>> >>> This allows to let field order and colorimetry settings be propagated >>> from upstream by calling media-ctl on the upstream entity source pad, >>> and then call media-ctl on the sink pad to manually set the input frame >>> interval, without changing the already set field order and colorimetry >>> information. >>> >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> >>> --- >>> This is based on imx-media-staging-md-v14, and it is supposed to allow >>> configuring the pipeline with media-ctl like this: >>> >>> 1) media-ctl --set-v4l2 "'tc358743 1-000f':0[fmt:UYVY8_1X16/1920x1080]" >>> 2) media-ctl --set-v4l2 "'imx6-mipi-csi2':1[fmt:UYVY8_1X16/1920x108]" >>> 3) media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY8_1X16/1920x1080]" >>> 4) media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY8_1X16/1920x1080@1/60]" >>> 5) media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/1920x1080@1/30]" >>> >>> Without having step 4) overwrite the colorspace and field order set on >>> 'ipu1_csi0':0 by the propagation in step 3). >>> --- >>> drivers/staging/media/imx/imx-media-csi.c | 34 +++++++++++++++++++++++++++++++ >>> 1 file changed, 34 insertions(+) >>> >>> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c >>> index 64dc454f6b371..d94ce1de2bf05 100644 >>> --- a/drivers/staging/media/imx/imx-media-csi.c >>> +++ b/drivers/staging/media/imx/imx-media-csi.c >>> @@ -1325,6 +1325,40 @@ static int csi_set_fmt(struct v4l2_subdev *sd, >>> csi_try_fmt(priv, sensor, cfg, sdformat, crop, compose, &cc); >>> >>> fmt = __csi_get_fmt(priv, cfg, sdformat->pad, sdformat->which); >>> + >>> + /* Retain current field setting as default */ >>> + if (sdformat->format.field == V4L2_FIELD_ANY) >>> + sdformat->format.field = fmt->field; >> >> sdformat->format.field should never be FIELD_ANY. If it is, then that's a >> subdev bug and I'm pretty sure FIELD_NONE was intended. > > This is the subdev. sdformat is passed in from userspace, so we have to > deal with it being set to ANY. I'm trying hard right now not to return > ANY though. The values in sdformat->format are applied to fmt down > below. Do you have a git tree with this patch? It is really hard to review without having the full imx-media-csi.c source. I think one problem is that it is not clearly defined how subdevs and colorspace information should work. Regards, Hans > >>> + >>> + /* Retain current colorspace setting as default */ >>> + if (sdformat->format.colorspace == V4L2_COLORSPACE_DEFAULT) { >>> + sdformat->format.colorspace = fmt->colorspace; >> >> No! Subdevs should never return COLORSPACE_DEFAULT. If they do, then fix >> them. If this happens a lot (I'm not sure how reliably subdevs fill this >> in) you could set it to COLORSPACE_RAW. Perhaps with a WARN_ON_ONCE. > > Same here, if userspace calls VIDIOC_SUBDEV_S_FMT with DEFAULT > colorspace, I don't want to return that unchanged, but overwrite it with > the currently set value. > >>> + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) >>> + sdformat->format.xfer_func = fmt->xfer_func; >>> + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) >>> + sdformat->format.ycbcr_enc = fmt->ycbcr_enc; >>> + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) >>> + sdformat->format.quantization = fmt->quantization; >> >> Nack. This is meaningless. > > It isn't. It may be wrong, but it is not without effect. If the > colorimetry info (in fmt) is currently set to something that is > non-standard, calling VIDIOC_SUBDEV_S_FMT with DEFAULT in xfer_func, > ycbcr_enc or quantization will cause those old values to be retained, > instead of overwriting them to DEFAULT. > >>> + } else { >>> + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) { >>> + sdformat->format.xfer_func = >>> + V4L2_MAP_XFER_FUNC_DEFAULT( >>> + sdformat->format.colorspace); >>> + } >>> + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) { >>> + sdformat->format.ycbcr_enc = >>> + V4L2_MAP_YCBCR_ENC_DEFAULT( >>> + sdformat->format.colorspace); >>> + } >>> + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) { >>> + sdformat->format.quantization = >>> + V4L2_MAP_QUANTIZATION_DEFAULT( >>> + cc->cs != IPUV3_COLORSPACE_YUV, >>> + sdformat->format.colorspace, >>> + sdformat->format.ycbcr_enc); >>> + } >> >> This isn't wrong, but it is perfectly fine to keep the DEFAULT here and let >> the application call V4L2_MAP_. >> >> I get the feeling this patch is a workaround for subdev errors. Either that, >> or the commit log doesn't give me enough information to really understand the >> problem that's being addressed here. > > It's the latter. I should have written VIDIOC_SUBDEV_S_FMT instead of > just set_fmt in the comment. > >> Regards, >> >> Hans >> >>> + } >>> + >>> *fmt = sdformat->format; > > Here sdformat is applied to the subdev pad. > > regards > Philipp >
On Thu, 2017-04-06 at 17:43 +0200, Hans Verkuil wrote: > On 04/06/2017 04:54 PM, Philipp Zabel wrote: > > On Thu, 2017-04-06 at 16:20 +0200, Hans Verkuil wrote: > >> On 04/06/2017 03:55 PM, Philipp Zabel wrote: > >>> If the the field order is set to ANY in set_fmt, choose the currently > >>> set field order. If the colorspace is set to DEFAULT, choose the current > >>> colorspace. If any of xfer_func, ycbcr_enc or quantization are set to > >>> DEFAULT, either choose the current setting, or the default setting for the > >>> new colorspace, if non-DEFAULT colorspace was given. > >>> > >>> This allows to let field order and colorimetry settings be propagated > >>> from upstream by calling media-ctl on the upstream entity source pad, > >>> and then call media-ctl on the sink pad to manually set the input frame > >>> interval, without changing the already set field order and colorimetry > >>> information. > >>> > >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > >>> --- > >>> This is based on imx-media-staging-md-v14, and it is supposed to allow > >>> configuring the pipeline with media-ctl like this: > >>> > >>> 1) media-ctl --set-v4l2 "'tc358743 1-000f':0[fmt:UYVY8_1X16/1920x1080]" > >>> 2) media-ctl --set-v4l2 "'imx6-mipi-csi2':1[fmt:UYVY8_1X16/1920x108]" > >>> 3) media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY8_1X16/1920x1080]" > >>> 4) media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY8_1X16/1920x1080@1/60]" > >>> 5) media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/1920x1080@1/30]" > >>> > >>> Without having step 4) overwrite the colorspace and field order set on > >>> 'ipu1_csi0':0 by the propagation in step 3). > >>> --- > >>> drivers/staging/media/imx/imx-media-csi.c | 34 +++++++++++++++++++++++++++++++ > >>> 1 file changed, 34 insertions(+) > >>> > >>> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c > >>> index 64dc454f6b371..d94ce1de2bf05 100644 > >>> --- a/drivers/staging/media/imx/imx-media-csi.c > >>> +++ b/drivers/staging/media/imx/imx-media-csi.c > >>> @@ -1325,6 +1325,40 @@ static int csi_set_fmt(struct v4l2_subdev *sd, > >>> csi_try_fmt(priv, sensor, cfg, sdformat, crop, compose, &cc); > >>> > >>> fmt = __csi_get_fmt(priv, cfg, sdformat->pad, sdformat->which); > >>> + > >>> + /* Retain current field setting as default */ > >>> + if (sdformat->format.field == V4L2_FIELD_ANY) > >>> + sdformat->format.field = fmt->field; > >> > >> sdformat->format.field should never be FIELD_ANY. If it is, then that's a > >> subdev bug and I'm pretty sure FIELD_NONE was intended. > > > > This is the subdev. sdformat is passed in from userspace, so we have to > > deal with it being set to ANY. I'm trying hard right now not to return > > ANY though. The values in sdformat->format are applied to fmt down > > below. > > Do you have a git tree with this patch? It is really hard to review without > having the full imx-media-csi.c source. The patch applies on top of https://github.com/slongerbeam/mediatree.git imx-media-staging-md-v14 I have uploaded a branch git://git.pengutronix.de/git/pza/linux imx-media-staging-md-v14+color with the patch applied on top. > I think one problem is that it is not clearly defined how subdevs and colorspace > information should work. regards Philipp
On 04/06/2017 06:01 PM, Philipp Zabel wrote: > On Thu, 2017-04-06 at 17:43 +0200, Hans Verkuil wrote: >> On 04/06/2017 04:54 PM, Philipp Zabel wrote: >>> On Thu, 2017-04-06 at 16:20 +0200, Hans Verkuil wrote: >>>> On 04/06/2017 03:55 PM, Philipp Zabel wrote: >>>>> If the the field order is set to ANY in set_fmt, choose the currently >>>>> set field order. If the colorspace is set to DEFAULT, choose the current >>>>> colorspace. If any of xfer_func, ycbcr_enc or quantization are set to >>>>> DEFAULT, either choose the current setting, or the default setting for the >>>>> new colorspace, if non-DEFAULT colorspace was given. >>>>> >>>>> This allows to let field order and colorimetry settings be propagated >>>>> from upstream by calling media-ctl on the upstream entity source pad, >>>>> and then call media-ctl on the sink pad to manually set the input frame >>>>> interval, without changing the already set field order and colorimetry >>>>> information. >>>>> >>>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> >>>>> --- >>>>> This is based on imx-media-staging-md-v14, and it is supposed to allow >>>>> configuring the pipeline with media-ctl like this: >>>>> >>>>> 1) media-ctl --set-v4l2 "'tc358743 1-000f':0[fmt:UYVY8_1X16/1920x1080]" >>>>> 2) media-ctl --set-v4l2 "'imx6-mipi-csi2':1[fmt:UYVY8_1X16/1920x108]" >>>>> 3) media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY8_1X16/1920x1080]" >>>>> 4) media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY8_1X16/1920x1080@1/60]" >>>>> 5) media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/1920x1080@1/30]" >>>>> >>>>> Without having step 4) overwrite the colorspace and field order set on >>>>> 'ipu1_csi0':0 by the propagation in step 3). >>>>> --- >>>>> drivers/staging/media/imx/imx-media-csi.c | 34 +++++++++++++++++++++++++++++++ >>>>> 1 file changed, 34 insertions(+) >>>>> >>>>> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c >>>>> index 64dc454f6b371..d94ce1de2bf05 100644 >>>>> --- a/drivers/staging/media/imx/imx-media-csi.c >>>>> +++ b/drivers/staging/media/imx/imx-media-csi.c >>>>> @@ -1325,6 +1325,40 @@ static int csi_set_fmt(struct v4l2_subdev *sd, >>>>> csi_try_fmt(priv, sensor, cfg, sdformat, crop, compose, &cc); >>>>> >>>>> fmt = __csi_get_fmt(priv, cfg, sdformat->pad, sdformat->which); >>>>> + >>>>> + /* Retain current field setting as default */ >>>>> + if (sdformat->format.field == V4L2_FIELD_ANY) >>>>> + sdformat->format.field = fmt->field; >>>> >>>> sdformat->format.field should never be FIELD_ANY. If it is, then that's a >>>> subdev bug and I'm pretty sure FIELD_NONE was intended. >>> >>> This is the subdev. sdformat is passed in from userspace, so we have to >>> deal with it being set to ANY. I'm trying hard right now not to return >>> ANY though. The values in sdformat->format are applied to fmt down >>> below. >> >> Do you have a git tree with this patch? It is really hard to review without >> having the full imx-media-csi.c source. > > The patch applies on top of > > https://github.com/slongerbeam/mediatree.git imx-media-staging-md-v14 > > I have uploaded a branch > > git://git.pengutronix.de/git/pza/linux imx-media-staging-md-v14+color > > with the patch applied on top. > >> I think one problem is that it is not clearly defined how subdevs and colorspace >> information should work. Ah, having the full source helped. Ignore my previous review, it was incorrect. I'll have to think about this some more. I'll get back to this, but it may take some time since my vacation starts tomorrow. The spec is simply unclear about how to handle this so we have to come up with some guidelines. Regards, Hans
On 04/06/2017 08:25 AM, Philipp Zabel wrote: > On Thu, 2017-04-06 at 16:10 +0100, Russell King - ARM Linux wrote: >> On Thu, Apr 06, 2017 at 05:01:52PM +0200, Philipp Zabel wrote: >>> On Thu, 2017-04-06 at 15:05 +0100, Russell King - ARM Linux wrote: >>>> On Thu, Apr 06, 2017 at 03:55:29PM +0200, Philipp Zabel wrote: >>>>> + >>>>> + /* Retain current field setting as default */ >>>>> + if (sdformat->format.field == V4L2_FIELD_ANY) >>>>> + sdformat->format.field = fmt->field; >>>>> + >>>>> + /* Retain current colorspace setting as default */ >>>>> + if (sdformat->format.colorspace == V4L2_COLORSPACE_DEFAULT) { >>>>> + sdformat->format.colorspace = fmt->colorspace; >>>>> + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) >>>>> + sdformat->format.xfer_func = fmt->xfer_func; >>>>> + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) >>>>> + sdformat->format.ycbcr_enc = fmt->ycbcr_enc; >>>>> + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) >>>>> + sdformat->format.quantization = fmt->quantization; >>>>> + } else { >>>>> + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) { >>>>> + sdformat->format.xfer_func = >>>>> + V4L2_MAP_XFER_FUNC_DEFAULT( >>>>> + sdformat->format.colorspace); >>>>> + } >>>>> + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) { >>>>> + sdformat->format.ycbcr_enc = >>>>> + V4L2_MAP_YCBCR_ENC_DEFAULT( >>>>> + sdformat->format.colorspace); >>>>> + } >>>>> + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) { >>>>> + sdformat->format.quantization = >>>>> + V4L2_MAP_QUANTIZATION_DEFAULT( >>>>> + cc->cs != IPUV3_COLORSPACE_YUV, >>>>> + sdformat->format.colorspace, >>>>> + sdformat->format.ycbcr_enc); >>>>> + } >>>>> + } >>>> >>>> Would it make sense for this to be a helper function? >>> >>> Quite possible, the next subdev that has to set frame_interval on both >>> pads manually because its upstream source pad doesn't suport >>> frame_interval might want to do the same. >> >> Hmm. I'm not sure I agree with this approach. If a subdev hardware >> does not support any modification of the colourspace or field, then >> it should not be modifyable at the source pad - it should retain the >> propagated settings from the sink pad. > > This new code is only relevant for the CSI_SINK_PAD. > >> I thought I had already sent a patch doing exactly that. > > Yes. Right above the modification there is a call to csi_try_fmt which > will already fix up sdformat->format for the source pads. So for the > CSI_SRC_PAD_DIRECT and CSI_SRC_PAD_IDMAC this should amount to a no-op. > > If might be better to move this into a separate function and only call > it if sdformat->pad == CSI_SINK_PAD. I've done this, I will follow with the new patch. Philipp, let me know if this looks ok to you and I will add your sign-off. Steve
On Wed, 2017-04-12 at 09:03 +0200, Hans Verkuil wrote: [...] > >> Do you have a git tree with this patch? It is really hard to review without > >> having the full imx-media-csi.c source. > > > > The patch applies on top of > > > > https://github.com/slongerbeam/mediatree.git imx-media-staging-md-v14 > > > > I have uploaded a branch > > > > git://git.pengutronix.de/git/pza/linux imx-media-staging-md-v14+color > > > > with the patch applied on top. > > > >> I think one problem is that it is not clearly defined how subdevs and colorspace > >> information should work. > > Ah, having the full source helped. > > Ignore my previous review, it was incorrect. Ok. > I'll have to think about this some more. I'll get back to this, but it may take some > time since my vacation starts tomorrow. The spec is simply unclear about how to handle > this so we have to come up with some guidelines. Yes, please. Until then, have a nice vacation. regards Philipp
Hi Philipp, Sorry for the very long delay, but I finally had some time to think about this. On 04/06/2017 03:55 PM, Philipp Zabel wrote: > If the the field order is set to ANY in set_fmt, choose the currently > set field order. If the colorspace is set to DEFAULT, choose the current > colorspace. If any of xfer_func, ycbcr_enc or quantization are set to > DEFAULT, either choose the current setting, or the default setting for the > new colorspace, if non-DEFAULT colorspace was given. > > This allows to let field order and colorimetry settings be propagated > from upstream by calling media-ctl on the upstream entity source pad, > and then call media-ctl on the sink pad to manually set the input frame > interval, without changing the already set field order and colorimetry > information. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > This is based on imx-media-staging-md-v14, and it is supposed to allow > configuring the pipeline with media-ctl like this: > > 1) media-ctl --set-v4l2 "'tc358743 1-000f':0[fmt:UYVY8_1X16/1920x1080]" > 2) media-ctl --set-v4l2 "'imx6-mipi-csi2':1[fmt:UYVY8_1X16/1920x108]" > 3) media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY8_1X16/1920x1080]" > 4) media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY8_1X16/1920x1080@1/60]" > 5) media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/1920x1080@1/30]" > > Without having step 4) overwrite the colorspace and field order set on > 'ipu1_csi0':0 by the propagation in step 3). > --- > drivers/staging/media/imx/imx-media-csi.c | 34 +++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c > index 64dc454f6b371..d94ce1de2bf05 100644 > --- a/drivers/staging/media/imx/imx-media-csi.c > +++ b/drivers/staging/media/imx/imx-media-csi.c > @@ -1325,6 +1325,40 @@ static int csi_set_fmt(struct v4l2_subdev *sd, > csi_try_fmt(priv, sensor, cfg, sdformat, crop, compose, &cc); > > fmt = __csi_get_fmt(priv, cfg, sdformat->pad, sdformat->which); > + > + /* Retain current field setting as default */ > + if (sdformat->format.field == V4L2_FIELD_ANY) > + sdformat->format.field = fmt->field; This is OK. > + > + /* Retain current colorspace setting as default */ > + if (sdformat->format.colorspace == V4L2_COLORSPACE_DEFAULT) { > + sdformat->format.colorspace = fmt->colorspace; > + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) > + sdformat->format.xfer_func = fmt->xfer_func; > + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) > + sdformat->format.ycbcr_enc = fmt->ycbcr_enc; > + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) > + sdformat->format.quantization = fmt->quantization; If sdformat->format.colorspace == V4L2_COLORSPACE_DEFAULT, then you can just copy all four fields from fmt to sdformat->format. The other three fields are meaningless when colorspace == V4L2_COLORSPACE_DEFAULT. > + } else { > + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) { > + sdformat->format.xfer_func = > + V4L2_MAP_XFER_FUNC_DEFAULT( > + sdformat->format.colorspace); > + } > + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) { > + sdformat->format.ycbcr_enc = > + V4L2_MAP_YCBCR_ENC_DEFAULT( > + sdformat->format.colorspace); > + } > + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) { > + sdformat->format.quantization = > + V4L2_MAP_QUANTIZATION_DEFAULT( > + cc->cs != IPUV3_COLORSPACE_YUV, > + sdformat->format.colorspace, > + sdformat->format.ycbcr_enc); > + } Is this needed for validation? Currently these fields play no role in the default link validation. Which I think is actually the right thing to do, unless the subdev can do actual colorspace conversion. I would just drop the whole 'else' here. Actually, wouldn't it be better to always just copy this information from fmt? This subdev doesn't do any colorspace conversion, it just passes on this information. I.e., you can't set it in any meaningful way. Regards, Hans > + } > + > *fmt = sdformat->format; > > if (sdformat->pad == CSI_SINK_PAD) { >
Hi Hans, On Mon, 2017-05-08 at 10:27 +0200, Hans Verkuil wrote: > Hi Philipp, > > Sorry for the very long delay, but I finally had some time to think about this. Thank you for your thoughts. > On 04/06/2017 03:55 PM, Philipp Zabel wrote: > > If the the field order is set to ANY in set_fmt, choose the currently > > set field order. If the colorspace is set to DEFAULT, choose the current > > colorspace. If any of xfer_func, ycbcr_enc or quantization are set to > > DEFAULT, either choose the current setting, or the default setting for the > > new colorspace, if non-DEFAULT colorspace was given. > > > > This allows to let field order and colorimetry settings be propagated > > from upstream by calling media-ctl on the upstream entity source pad, > > and then call media-ctl on the sink pad to manually set the input frame > > interval, without changing the already set field order and colorimetry > > information. > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > This is based on imx-media-staging-md-v14, and it is supposed to allow > > configuring the pipeline with media-ctl like this: > > > > 1) media-ctl --set-v4l2 "'tc358743 1-000f':0[fmt:UYVY8_1X16/1920x1080]" > > 2) media-ctl --set-v4l2 "'imx6-mipi-csi2':1[fmt:UYVY8_1X16/1920x108]" > > 3) media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY8_1X16/1920x1080]" > > 4) media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY8_1X16/1920x1080@1/60]" > > 5) media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/1920x1080@1/30]" > > > > Without having step 4) overwrite the colorspace and field order set on > > 'ipu1_csi0':0 by the propagation in step 3). > > --- > > drivers/staging/media/imx/imx-media-csi.c | 34 +++++++++++++++++++++++++++++++ > > 1 file changed, 34 insertions(+) > > > > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c > > index 64dc454f6b371..d94ce1de2bf05 100644 > > --- a/drivers/staging/media/imx/imx-media-csi.c > > +++ b/drivers/staging/media/imx/imx-media-csi.c > > @@ -1325,6 +1325,40 @@ static int csi_set_fmt(struct v4l2_subdev *sd, > > csi_try_fmt(priv, sensor, cfg, sdformat, crop, compose, &cc); > > > > fmt = __csi_get_fmt(priv, cfg, sdformat->pad, sdformat->which); > > + > > + /* Retain current field setting as default */ > > + if (sdformat->format.field == V4L2_FIELD_ANY) > > + sdformat->format.field = fmt->field; > > This is OK. Would this be a "may" or a "should"? As in, "If SUBDEV_S_FMT is called with field == ANY, the driver may/should set field to the previously configured interlacing field order". What would be the correct place to document this behaviour? Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst? > > + /* Retain current colorspace setting as default */ > > + if (sdformat->format.colorspace == V4L2_COLORSPACE_DEFAULT) { > > + sdformat->format.colorspace = fmt->colorspace; > > + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) > > + sdformat->format.xfer_func = fmt->xfer_func; > > + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) > > + sdformat->format.ycbcr_enc = fmt->ycbcr_enc; > > + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) > > + sdformat->format.quantization = fmt->quantization; > > If sdformat->format.colorspace == V4L2_COLORSPACE_DEFAULT, then you can just copy > all four fields from fmt to sdformat->format. The other three fields are meaningless > when colorspace == V4L2_COLORSPACE_DEFAULT. Ok, good. Ignoring the transfer function / YCbCr encoding / quantization range fields when colorspace is DEFAULT would simplify this part to: if (sdformat->format.colorspace == V4L2_COLORSPACE_DEFAULT) { sdformat->format.colorspace = fmt->colorspace; sdformat->format.xfer_func = fmt->xfer_func; sdformat->format.ycbcr_enc = fmt->ycbcr_enc; sdformat->format.quantization = fmt->quantization; } Is that expectation already written down somewhere? If not, should we add it to Documentation/media/uapi/v4l/pixfmt-006.rst? > > + } else { > > + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) { > > + sdformat->format.xfer_func = > > + V4L2_MAP_XFER_FUNC_DEFAULT( > > + sdformat->format.colorspace); > > + } > > + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) { > > + sdformat->format.ycbcr_enc = > > + V4L2_MAP_YCBCR_ENC_DEFAULT( > > + sdformat->format.colorspace); > > + } > > + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) { > > + sdformat->format.quantization = > > + V4L2_MAP_QUANTIZATION_DEFAULT( > > + cc->cs != IPUV3_COLORSPACE_YUV, > > + sdformat->format.colorspace, > > + sdformat->format.ycbcr_enc); > > + } > > Is this needed for validation? Currently these fields play no role in the > default link validation. Which I think is actually the right thing to do, > unless the subdev can do actual colorspace conversion. The CSI subdevice can't do colorspace conversion, but exactly the same applies to the IC subdevices, which can. Also I'd like this information to be correct, as the /dev/videoX capture device takes its colorspace information from the CSI source pad. The problem I wanted to solve here initially was that if the colorspace information was previously set correctly: media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY8_1X16/1920x1080@1/60 colorspace:rec709 xfer:709 ycbcr:709 quantization:lim-range]" -> V4L2_COLORSPACE_REC709 V4L2_XFER_FUNC_709 V4L2_YCBCR_ENC_709 V4L2_QUANTIZATION_LIMITED_RANGE A later media-ctl call to just change the frame interval would overwrite the colorspace information: media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY8_1X16/1920x1080@1/30]" -> V4L2_COLORSPACE_DEFAULT V4L2_XFER_FUNC_DEFAULT V4L2_YCBCR_ENC_DEFAULT V4L2_QUANTIZATION_DEFAULT > I would just drop the whole 'else' here. That would allow to set the actual colorspace information reported by SUBDEV_G_FMT on both sink and source pads (and by extension, the colorspace information reported by G_FMT on /dev/videoX) to V4L2_{XFER_FUNC,YCBCR_ENC,QUANTIZATION}_DEFAULT. media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY8_1X16/1920x1080 colorspace:rec709]" -> V4L2_COLORSPACE_REC709 V4L2_XFER_FUNC_DEFAULT V4L2_YCBCR_ENC_DEFAULT V4L2_QUANTIZATION_DEFAULT I suppose that is acceptable as given the colorspace, the other DEFAULT values have a unique meaning, but wouldn't it be nicer to show userspace what these default inputs actually map to? > Actually, wouldn't it be better to always just copy this information from > fmt? This subdev doesn't do any colorspace conversion, it just passes on > this information. I.e., you can't set it in any meaningful way. csi_set_fmt on the sink pad is the function that actually sets fmt in the first place. If we always copy colorspace information from fmt, it can't be set to the correct value at all. regards Philipp
On 05/08/2017 11:36 AM, Philipp Zabel wrote: > Hi Hans, > > On Mon, 2017-05-08 at 10:27 +0200, Hans Verkuil wrote: >> Hi Philipp, >> >> Sorry for the very long delay, but I finally had some time to think about this. > > Thank you for your thoughts. > >> On 04/06/2017 03:55 PM, Philipp Zabel wrote: >>> If the the field order is set to ANY in set_fmt, choose the currently >>> set field order. If the colorspace is set to DEFAULT, choose the current >>> colorspace. If any of xfer_func, ycbcr_enc or quantization are set to >>> DEFAULT, either choose the current setting, or the default setting for the >>> new colorspace, if non-DEFAULT colorspace was given. >>> >>> This allows to let field order and colorimetry settings be propagated >>> from upstream by calling media-ctl on the upstream entity source pad, >>> and then call media-ctl on the sink pad to manually set the input frame >>> interval, without changing the already set field order and colorimetry >>> information. >>> >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> >>> --- >>> This is based on imx-media-staging-md-v14, and it is supposed to allow >>> configuring the pipeline with media-ctl like this: >>> >>> 1) media-ctl --set-v4l2 "'tc358743 1-000f':0[fmt:UYVY8_1X16/1920x1080]" >>> 2) media-ctl --set-v4l2 "'imx6-mipi-csi2':1[fmt:UYVY8_1X16/1920x108]" >>> 3) media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY8_1X16/1920x1080]" >>> 4) media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY8_1X16/1920x1080@1/60]" >>> 5) media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/1920x1080@1/30]" >>> >>> Without having step 4) overwrite the colorspace and field order set on >>> 'ipu1_csi0':0 by the propagation in step 3). >>> --- >>> drivers/staging/media/imx/imx-media-csi.c | 34 +++++++++++++++++++++++++++++++ >>> 1 file changed, 34 insertions(+) >>> >>> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c >>> index 64dc454f6b371..d94ce1de2bf05 100644 >>> --- a/drivers/staging/media/imx/imx-media-csi.c >>> +++ b/drivers/staging/media/imx/imx-media-csi.c >>> @@ -1325,6 +1325,40 @@ static int csi_set_fmt(struct v4l2_subdev *sd, >>> csi_try_fmt(priv, sensor, cfg, sdformat, crop, compose, &cc); >>> >>> fmt = __csi_get_fmt(priv, cfg, sdformat->pad, sdformat->which); >>> + >>> + /* Retain current field setting as default */ >>> + if (sdformat->format.field == V4L2_FIELD_ANY) >>> + sdformat->format.field = fmt->field; >> >> This is OK. > > Would this be a "may" or a "should"? As in, > "If SUBDEV_S_FMT is called with field == ANY, the driver may/should set > field to the previously configured interlacing field order". To quote the FIELD_ANY documentation: file:///home/hans/work/src/v4l/media-git/Documentation/output/html/uapi/v4l/field-order.html#field-order "Drivers must never return V4L2_FIELD_ANY." How a driver replaces FIELD_ANY is something that I am not sure should be explicitly stated in the documentation. In many cases there is only one choice (usually FIELD_NONE). In cases like this I think your proposed rule is a good recommendation. I would probably phrase it like that. > What would be the correct place to document this behaviour? > Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst? Yes. > >>> + /* Retain current colorspace setting as default */ >>> + if (sdformat->format.colorspace == V4L2_COLORSPACE_DEFAULT) { >>> + sdformat->format.colorspace = fmt->colorspace; >>> + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) >>> + sdformat->format.xfer_func = fmt->xfer_func; >>> + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) >>> + sdformat->format.ycbcr_enc = fmt->ycbcr_enc; >>> + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) >>> + sdformat->format.quantization = fmt->quantization; >> >> If sdformat->format.colorspace == V4L2_COLORSPACE_DEFAULT, then you can just copy >> all four fields from fmt to sdformat->format. The other three fields are meaningless >> when colorspace == V4L2_COLORSPACE_DEFAULT. > > Ok, good. Ignoring the transfer function / YCbCr encoding / quantization > range fields when colorspace is DEFAULT would simplify this part to: > > if (sdformat->format.colorspace == V4L2_COLORSPACE_DEFAULT) { > sdformat->format.colorspace = fmt->colorspace; > sdformat->format.xfer_func = fmt->xfer_func; > sdformat->format.ycbcr_enc = fmt->ycbcr_enc; > sdformat->format.quantization = fmt->quantization; > } > > Is that expectation already written down somewhere? If not, should we > add it to Documentation/media/uapi/v4l/pixfmt-006.rst? I don't think it is written down. It would be a good idea to make this explicit. > >>> + } else { >>> + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) { >>> + sdformat->format.xfer_func = >>> + V4L2_MAP_XFER_FUNC_DEFAULT( >>> + sdformat->format.colorspace); >>> + } >>> + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) { >>> + sdformat->format.ycbcr_enc = >>> + V4L2_MAP_YCBCR_ENC_DEFAULT( >>> + sdformat->format.colorspace); >>> + } >>> + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) { >>> + sdformat->format.quantization = >>> + V4L2_MAP_QUANTIZATION_DEFAULT( >>> + cc->cs != IPUV3_COLORSPACE_YUV, >>> + sdformat->format.colorspace, >>> + sdformat->format.ycbcr_enc); >>> + } >> >> Is this needed for validation? Currently these fields play no role in the >> default link validation. Which I think is actually the right thing to do, >> unless the subdev can do actual colorspace conversion. > > The CSI subdevice can't do colorspace conversion, but exactly the same > applies to the IC subdevices, which can. Also I'd like this information > to be correct, as the /dev/videoX capture device takes its colorspace > information from the CSI source pad. > > The problem I wanted to solve here initially was that if the colorspace > information was previously set correctly: > > media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY8_1X16/1920x1080@1/60 colorspace:rec709 xfer:709 ycbcr:709 quantization:lim-range]" > -> > V4L2_COLORSPACE_REC709 > V4L2_XFER_FUNC_709 > V4L2_YCBCR_ENC_709 > V4L2_QUANTIZATION_LIMITED_RANGE > > A later media-ctl call to just change the frame interval would overwrite the colorspace information: > > media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY8_1X16/1920x1080@1/30]" > -> > V4L2_COLORSPACE_DEFAULT > V4L2_XFER_FUNC_DEFAULT > V4L2_YCBCR_ENC_DEFAULT > V4L2_QUANTIZATION_DEFAULT Not anymore since if media-ctl sets colorspace to DEFAULT, then it will just copy the last configuration. If media-ctl sets colorspace to a non-DEFAULT value, then I assume that it is a new explicitly defined colorspace. > >> I would just drop the whole 'else' here. > > That would allow to set the actual colorspace information reported by > SUBDEV_G_FMT on both sink and source pads (and by extension, the > colorspace information reported by G_FMT on /dev/videoX) to > V4L2_{XFER_FUNC,YCBCR_ENC,QUANTIZATION}_DEFAULT. > > media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY8_1X16/1920x1080 colorspace:rec709]" > -> > V4L2_COLORSPACE_REC709 > V4L2_XFER_FUNC_DEFAULT > V4L2_YCBCR_ENC_DEFAULT > V4L2_QUANTIZATION_DEFAULT > > I suppose that is acceptable as given the colorspace, the other DEFAULT > values have a unique meaning, but wouldn't it be nicer to show userspace > what these default inputs actually map to? Good question. Right now this isn't done in other drivers. I have been struggling with this question myself. My problem is that I don't like to have to copy this code in every driver. Ideally this should be handled in the v4l2 core, but I am not sure if all the required information is available there. I would postpone this, and look at this in a separate patch. >> Actually, wouldn't it be better to always just copy this information from >> fmt? This subdev doesn't do any colorspace conversion, it just passes on >> this information. I.e., you can't set it in any meaningful way. > > csi_set_fmt on the sink pad is the function that actually sets fmt in > the first place. If we always copy colorspace information from fmt, it > can't be set to the correct value at all. Yes, sorry about that. Ignore that comment. Regards, Hans
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c index 64dc454f6b371..d94ce1de2bf05 100644 --- a/drivers/staging/media/imx/imx-media-csi.c +++ b/drivers/staging/media/imx/imx-media-csi.c @@ -1325,6 +1325,40 @@ static int csi_set_fmt(struct v4l2_subdev *sd, csi_try_fmt(priv, sensor, cfg, sdformat, crop, compose, &cc); fmt = __csi_get_fmt(priv, cfg, sdformat->pad, sdformat->which); + + /* Retain current field setting as default */ + if (sdformat->format.field == V4L2_FIELD_ANY) + sdformat->format.field = fmt->field; + + /* Retain current colorspace setting as default */ + if (sdformat->format.colorspace == V4L2_COLORSPACE_DEFAULT) { + sdformat->format.colorspace = fmt->colorspace; + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) + sdformat->format.xfer_func = fmt->xfer_func; + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) + sdformat->format.ycbcr_enc = fmt->ycbcr_enc; + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) + sdformat->format.quantization = fmt->quantization; + } else { + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) { + sdformat->format.xfer_func = + V4L2_MAP_XFER_FUNC_DEFAULT( + sdformat->format.colorspace); + } + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) { + sdformat->format.ycbcr_enc = + V4L2_MAP_YCBCR_ENC_DEFAULT( + sdformat->format.colorspace); + } + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) { + sdformat->format.quantization = + V4L2_MAP_QUANTIZATION_DEFAULT( + cc->cs != IPUV3_COLORSPACE_YUV, + sdformat->format.colorspace, + sdformat->format.ycbcr_enc); + } + } + *fmt = sdformat->format; if (sdformat->pad == CSI_SINK_PAD) {
If the the field order is set to ANY in set_fmt, choose the currently set field order. If the colorspace is set to DEFAULT, choose the current colorspace. If any of xfer_func, ycbcr_enc or quantization are set to DEFAULT, either choose the current setting, or the default setting for the new colorspace, if non-DEFAULT colorspace was given. This allows to let field order and colorimetry settings be propagated from upstream by calling media-ctl on the upstream entity source pad, and then call media-ctl on the sink pad to manually set the input frame interval, without changing the already set field order and colorimetry information. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- This is based on imx-media-staging-md-v14, and it is supposed to allow configuring the pipeline with media-ctl like this: 1) media-ctl --set-v4l2 "'tc358743 1-000f':0[fmt:UYVY8_1X16/1920x1080]" 2) media-ctl --set-v4l2 "'imx6-mipi-csi2':1[fmt:UYVY8_1X16/1920x108]" 3) media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY8_1X16/1920x1080]" 4) media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY8_1X16/1920x1080@1/60]" 5) media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/1920x1080@1/30]" Without having step 4) overwrite the colorspace and field order set on 'ipu1_csi0':0 by the propagation in step 3). --- drivers/staging/media/imx/imx-media-csi.c | 34 +++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)