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

Groups > linux.kernel > #1622642 > unrolled thread

Re: [PATCH] [media] imx: csi: retain current field order and colorimetry setting as default

Started bySteve Longerbeam <slongerbeam@gmail.com>
First post2017-04-13 02:40 +0200
Last post2017-04-18 11:40 +0200
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

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

Contents

  Re: [PATCH] [media] imx: csi: retain current field order and  colorimetry setting as default Steve Longerbeam <slongerbeam@gmail.com> - 2017-04-13 02:40 +0200
    [PATCH 40/40] media: imx: set and propagate empty field, colorimetry params Steve Longerbeam <slongerbeam@gmail.com> - 2017-04-13 02:50 +0200
      Re: [PATCH 40/40] media: imx: set and propagate empty field,  colorimetry params Philipp Zabel <p.zabel@pengutronix.de> - 2017-04-13 12:20 +0200
        Re: [PATCH 40/40] media: imx: set and propagate empty field,  colorimetry params Steve Longerbeam <slongerbeam@gmail.com> - 2017-04-13 18:50 +0200
          Re: [PATCH 40/40] media: imx: set and propagate empty field,  colorimetry params Philipp Zabel <p.zabel@pengutronix.de> - 2017-04-18 11:40 +0200

#1622642 — Re: [PATCH] [media] imx: csi: retain current field order and colorimetry setting as default

FromSteve Longerbeam <slongerbeam@gmail.com>
Date2017-04-13 02:40 +0200
SubjectRe: [PATCH] [media] imx: csi: retain current field order and colorimetry setting as default
Message-ID<tvAXL-3zm-1@gated-at.bofh.it>

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

[toc] | [next] | [standalone]

#1622651 — [PATCH 40/40] media: imx: set and propagate empty field, colorimetry params

FromSteve Longerbeam <slongerbeam@gmail.com>
Date2017-04-13 02:50 +0200
Subject[PATCH 40/40] media: imx: set and propagate empty field, colorimetry params
Message-ID<tvB7s-3CX-23@gated-at.bofh.it>
In reply to#1622642
This patch adds a call to imx_media_fill_empty_mbus_fields() in the
*_try_fmt() functions at the sink pads, to set empty field order and
colorimetry parameters.

If the field order is set to ANY, choose the currently set field order
at the sink pad. If the colorspace is set to DEFAULT, choose the
current colorspace at the sink pad.  If any of xfer_func, ycbcr_enc
or quantization are set to DEFAULT, either choose the current sink pad
setting, or the default setting for the new colorspace, if non-DEFAULT
colorspace was given.

Colorimetry is also propagated from sink to source pads anywhere
this has not already been done. The exception is ic-prpencvf at the
source pad, since the Image Converter outputs fixed quantization and
Y`CbCr encoding.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/staging/media/imx/imx-ic-prp.c      |  5 ++-
 drivers/staging/media/imx/imx-ic-prpencvf.c | 25 +++++++++++---
 drivers/staging/media/imx/imx-media-csi.c   | 12 +++++--
 drivers/staging/media/imx/imx-media-utils.c | 53 +++++++++++++++++++++++++++++
 drivers/staging/media/imx/imx-media-vdic.c  |  7 ++--
 drivers/staging/media/imx/imx-media.h       |  3 +-
 6 files changed, 95 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/media/imx/imx-ic-prp.c b/drivers/staging/media/imx/imx-ic-prp.c
index b4d4e48..8baa0d5 100644
--- a/drivers/staging/media/imx/imx-ic-prp.c
+++ b/drivers/staging/media/imx/imx-ic-prp.c
@@ -180,6 +180,8 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
 		goto out;
 	}
 
+	fmt = __prp_get_fmt(priv, cfg, PRP_SINK_PAD, sdformat->which);
+
 	switch (sdformat->pad) {
 	case PRP_SINK_PAD:
 		v4l_bound_align_image(&sdformat->format.width, MIN_W, MAX_W,
@@ -193,11 +195,12 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
 			cc = imx_media_find_ipu_format(code, CS_SEL_ANY);
 			sdformat->format.code = cc->codes[0];
 		}
+
+		imx_media_fill_empty_mbus_fields(&sdformat->format, fmt);
 		break;
 	case PRP_SRC_PAD_PRPENC:
 	case PRP_SRC_PAD_PRPVF:
 		/* Output pads mirror input pad */
-		fmt = __prp_get_fmt(priv, cfg, PRP_SINK_PAD, sdformat->which);
 		sdformat->format = *fmt;
 		break;
 	}
diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
index 860b406..cdfb51a 100644
--- a/drivers/staging/media/imx/imx-ic-prpencvf.c
+++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
@@ -772,6 +772,8 @@ static void prp_try_fmt(struct prp_priv *priv,
 			struct v4l2_subdev_format *sdformat,
 			const struct imx_media_pixfmt **cc)
 {
+	struct v4l2_mbus_framefmt *infmt;
+
 	*cc = imx_media_find_ipu_format(sdformat->format.code, CS_SEL_ANY);
 	if (!*cc) {
 		u32 code;
@@ -781,11 +783,9 @@ static void prp_try_fmt(struct prp_priv *priv,
 		sdformat->format.code = (*cc)->codes[0];
 	}
 
-	if (sdformat->pad == PRPENCVF_SRC_PAD) {
-		struct v4l2_mbus_framefmt *infmt =
-			__prp_get_fmt(priv, cfg, PRPENCVF_SINK_PAD,
-				      sdformat->which);
+	infmt = __prp_get_fmt(priv, cfg, PRPENCVF_SINK_PAD, sdformat->which);
 
+	if (sdformat->pad == PRPENCVF_SRC_PAD) {
 		if (sdformat->format.field != V4L2_FIELD_NONE)
 			sdformat->format.field = infmt->field;
 
@@ -804,12 +804,29 @@ static void prp_try_fmt(struct prp_priv *priv,
 					      &sdformat->format.height,
 					      infmt->height / 4, MAX_H_SRC,
 					      H_ALIGN_SRC, S_ALIGN);
+
+		/*
+		 * The Image Converter produces fixed quantization
+		 * (full range for RGB, limited range for YUV), and
+		 * uses a fixed Y`CbCr encoding (V4L2_YCBCR_ENC_601).
+		 * For colorspace and transfer func, just propagate
+		 * from the sink.
+		 */
+		sdformat->format.quantization =
+			((*cc)->cs != IPUV3_COLORSPACE_YUV) ?
+			V4L2_QUANTIZATION_FULL_RANGE :
+			V4L2_QUANTIZATION_LIM_RANGE;
+		sdformat->format.ycbcr_enc = V4L2_YCBCR_ENC_601;
+		sdformat->format.colorspace = infmt->colorspace;
+		sdformat->format.xfer_func = infmt->xfer_func;
 	} else {
 		v4l_bound_align_image(&sdformat->format.width,
 				      MIN_W_SINK, MAX_W_SINK, W_ALIGN_SINK,
 				      &sdformat->format.height,
 				      MIN_H_SINK, MAX_H_SINK, H_ALIGN_SINK,
 				      S_ALIGN);
+
+		imx_media_fill_empty_mbus_fields(&sdformat->format, infmt);
 	}
 }
 
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index b11e80f..730d1aa 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1220,11 +1220,11 @@ static void csi_try_fmt(struct csi_priv *priv,
 	struct v4l2_mbus_framefmt *infmt;
 	u32 code;
 
+	infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD, sdformat->which);
+
 	switch (sdformat->pad) {
 	case CSI_SRC_PAD_DIRECT:
 	case CSI_SRC_PAD_IDMAC:
-		infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD,
-				      sdformat->which);
 		incc = imx_media_find_mbus_format(infmt->code,
 						  CS_SEL_ANY, true);
 
@@ -1260,6 +1260,12 @@ static void csi_try_fmt(struct csi_priv *priv,
 			sdformat->format.field =  (infmt->height == 480) ?
 				V4L2_FIELD_SEQ_TB : V4L2_FIELD_SEQ_BT;
 		}
+
+		/* propagate colorimetry from sink */
+		sdformat->format.colorspace = infmt->colorspace;
+		sdformat->format.xfer_func = infmt->xfer_func;
+		sdformat->format.quantization = infmt->quantization;
+		sdformat->format.ycbcr_enc = infmt->ycbcr_enc;
 		break;
 	case CSI_SINK_PAD:
 		v4l_bound_align_image(&sdformat->format.width, MIN_W, MAX_W,
@@ -1286,6 +1292,8 @@ static void csi_try_fmt(struct csi_priv *priv,
 							CS_SEL_ANY, false);
 			sdformat->format.code = (*cc)->codes[0];
 		}
+
+		imx_media_fill_empty_mbus_fields(&sdformat->format, infmt);
 		break;
 	}
 }
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 7b2f92d..b07d0ae 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -464,6 +464,59 @@ int imx_media_init_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
 }
 EXPORT_SYMBOL_GPL(imx_media_init_mbus_fmt);
 
+/*
+ * Check whether the field or colorimetry params in tryfmt are
+ * uninitialized, and if so fill them with the values from fmt.
+ * The exception is when tryfmt->colorspace has been initialized,
+ * if so all the further default colorimetry params can be derived
+ * from tryfmt->colorspace.
+ */
+void imx_media_fill_empty_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
+				      struct v4l2_mbus_framefmt *fmt)
+{
+	/* fill field if necessary */
+	if (tryfmt->field == V4L2_FIELD_ANY)
+		tryfmt->field = fmt->field;
+
+	/* fill colorimetry if necessary */
+	if (tryfmt->colorspace == V4L2_COLORSPACE_DEFAULT) {
+		tryfmt->colorspace = fmt->colorspace;
+		if (tryfmt->xfer_func == V4L2_XFER_FUNC_DEFAULT)
+			tryfmt->xfer_func = fmt->xfer_func;
+		if (tryfmt->ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT)
+			tryfmt->ycbcr_enc = fmt->ycbcr_enc;
+		if (tryfmt->quantization == V4L2_QUANTIZATION_DEFAULT)
+			tryfmt->quantization = fmt->quantization;
+	} else {
+		const struct imx_media_pixfmt *cc;
+		bool is_rgb = false;
+
+		cc = imx_media_find_mbus_format(tryfmt->code,
+						CS_SEL_ANY, false);
+		if (!cc)
+			cc = imx_media_find_ipu_format(tryfmt->code,
+						       CS_SEL_ANY);
+		if (cc && cc->cs != IPUV3_COLORSPACE_YUV)
+			is_rgb = true;
+
+		if (tryfmt->xfer_func == V4L2_XFER_FUNC_DEFAULT) {
+			tryfmt->xfer_func =
+				V4L2_MAP_XFER_FUNC_DEFAULT(tryfmt->colorspace);
+		}
+		if (tryfmt->ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) {
+			tryfmt->ycbcr_enc =
+				V4L2_MAP_YCBCR_ENC_DEFAULT(tryfmt->colorspace);
+		}
+		if (tryfmt->quantization == V4L2_QUANTIZATION_DEFAULT) {
+			tryfmt->quantization =
+				V4L2_MAP_QUANTIZATION_DEFAULT(
+					is_rgb, tryfmt->colorspace,
+					tryfmt->ycbcr_enc);
+		}
+	}
+}
+EXPORT_SYMBOL_GPL(imx_media_fill_empty_mbus_fields);
+
 int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
 				  struct v4l2_mbus_framefmt *mbus,
 				  const struct imx_media_pixfmt *cc)
diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c
index 0da45cf..b71d522 100644
--- a/drivers/staging/media/imx/imx-media-vdic.c
+++ b/drivers/staging/media/imx/imx-media-vdic.c
@@ -577,10 +577,11 @@ static void vdic_try_fmt(struct vdic_priv *priv,
 		sdformat->format.code = (*cc)->codes[0];
 	}
 
+	infmt = __vdic_get_fmt(priv, cfg, priv->active_input_pad,
+			       sdformat->which);
+
 	switch (sdformat->pad) {
 	case VDIC_SRC_PAD_DIRECT:
-		infmt = __vdic_get_fmt(priv, cfg, priv->active_input_pad,
-				       sdformat->which);
 		sdformat->format = *infmt;
 		/* output is always progressive! */
 		sdformat->format.field = V4L2_FIELD_NONE;
@@ -592,6 +593,8 @@ static void vdic_try_fmt(struct vdic_priv *priv,
 				      &sdformat->format.height,
 				      MIN_H, MAX_H_VDIC, H_ALIGN, S_ALIGN);
 
+		imx_media_fill_empty_mbus_fields(&sdformat->format, infmt);
+
 		/* input must be interlaced! Choose SEQ_TB if not */
 		if (!V4L2_FIELD_HAS_BOTH(sdformat->format.field))
 			sdformat->format.field = V4L2_FIELD_SEQ_TB;
diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
index 9985aa0..3bc685c 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -189,7 +189,8 @@ int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel);
 int imx_media_init_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
 			    u32 width, u32 height, u32 code, u32 field,
 			    const struct imx_media_pixfmt **cc);
-
+void imx_media_fill_empty_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
+				      struct v4l2_mbus_framefmt *fmt);
 int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
 				  struct v4l2_mbus_framefmt *mbus,
 				  const struct imx_media_pixfmt *cc);
-- 
2.7.4

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

#1622907 — Re: [PATCH 40/40] media: imx: set and propagate empty field, colorimetry params

FromPhilipp Zabel <p.zabel@pengutronix.de>
Date2017-04-13 12:20 +0200
SubjectRe: [PATCH 40/40] media: imx: set and propagate empty field, colorimetry params
Message-ID<tvK15-1zL-45@gated-at.bofh.it>
In reply to#1622651
Hi Steve,

thanks for cleaning this up. Apart from the comments below, this looks
good to me.

On Wed, 2017-04-12 at 17:45 -0700, Steve Longerbeam wrote:
> This patch adds a call to imx_media_fill_empty_mbus_fields() in the
> *_try_fmt() functions at the sink pads, to set empty field order and
> colorimetry parameters.
> 
> If the field order is set to ANY, choose the currently set field order
> at the sink pad. If the colorspace is set to DEFAULT, choose the
> current colorspace at the sink pad.  If any of xfer_func, ycbcr_enc
> or quantization are set to DEFAULT, either choose the current sink pad
> setting, or the default setting for the new colorspace, if non-DEFAULT
> colorspace was given.
> 
> Colorimetry is also propagated from sink to source pads anywhere
> this has not already been done. The exception is ic-prpencvf at the
> source pad, since the Image Converter outputs fixed quantization and
> Y`CbCr encoding.
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
>  drivers/staging/media/imx/imx-ic-prp.c      |  5 ++-
>  drivers/staging/media/imx/imx-ic-prpencvf.c | 25 +++++++++++---
>  drivers/staging/media/imx/imx-media-csi.c   | 12 +++++--
>  drivers/staging/media/imx/imx-media-utils.c | 53 +++++++++++++++++++++++++++++
>  drivers/staging/media/imx/imx-media-vdic.c  |  7 ++--
>  drivers/staging/media/imx/imx-media.h       |  3 +-
>  6 files changed, 95 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx-ic-prp.c b/drivers/staging/media/imx/imx-ic-prp.c
> index b4d4e48..8baa0d5 100644
> --- a/drivers/staging/media/imx/imx-ic-prp.c
> +++ b/drivers/staging/media/imx/imx-ic-prp.c
> @@ -180,6 +180,8 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
>  		goto out;
>  	}
>  
> +	fmt = __prp_get_fmt(priv, cfg, PRP_SINK_PAD, sdformat->which);
> +

Should this be changed to infmt for consistency?

>  	switch (sdformat->pad) {
>  	case PRP_SINK_PAD:
>  		v4l_bound_align_image(&sdformat->format.width, MIN_W, MAX_W,
> @@ -193,11 +195,12 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
>  			cc = imx_media_find_ipu_format(code, CS_SEL_ANY);
>  			sdformat->format.code = cc->codes[0];
>  		}
> +
> +		imx_media_fill_empty_mbus_fields(&sdformat->format, fmt);
>  		break;
>  	case PRP_SRC_PAD_PRPENC:
>  	case PRP_SRC_PAD_PRPVF:
>  		/* Output pads mirror input pad */
> -		fmt = __prp_get_fmt(priv, cfg, PRP_SINK_PAD, sdformat->which);
>  		sdformat->format = *fmt;
>  		break;
>  	}
> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
> index 860b406..cdfb51a 100644
> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> @@ -772,6 +772,8 @@ static void prp_try_fmt(struct prp_priv *priv,
>  			struct v4l2_subdev_format *sdformat,
>  			const struct imx_media_pixfmt **cc)
>  {
> +	struct v4l2_mbus_framefmt *infmt;
> +
>  	*cc = imx_media_find_ipu_format(sdformat->format.code, CS_SEL_ANY);
>  	if (!*cc) {
>  		u32 code;
> @@ -781,11 +783,9 @@ static void prp_try_fmt(struct prp_priv *priv,
>  		sdformat->format.code = (*cc)->codes[0];
>  	}
>  
> -	if (sdformat->pad == PRPENCVF_SRC_PAD) {
> -		struct v4l2_mbus_framefmt *infmt =
> -			__prp_get_fmt(priv, cfg, PRPENCVF_SINK_PAD,
> -				      sdformat->which);
> +	infmt = __prp_get_fmt(priv, cfg, PRPENCVF_SINK_PAD, sdformat->which);
>  
> +	if (sdformat->pad == PRPENCVF_SRC_PAD) {
>  		if (sdformat->format.field != V4L2_FIELD_NONE)
>  			sdformat->format.field = infmt->field;
>  
> @@ -804,12 +804,29 @@ static void prp_try_fmt(struct prp_priv *priv,
>  					      &sdformat->format.height,
>  					      infmt->height / 4, MAX_H_SRC,
>  					      H_ALIGN_SRC, S_ALIGN);
> +
> +		/*
> +		 * The Image Converter produces fixed quantization
> +		 * (full range for RGB, limited range for YUV), and
> +		 * uses a fixed Y`CbCr encoding (V4L2_YCBCR_ENC_601).
> +		 * For colorspace and transfer func, just propagate
> +		 * from the sink.
> +		 */
> +		sdformat->format.quantization =
> +			((*cc)->cs != IPUV3_COLORSPACE_YUV) ?
> +			V4L2_QUANTIZATION_FULL_RANGE :
> +			V4L2_QUANTIZATION_LIM_RANGE;
> +		sdformat->format.ycbcr_enc = V4L2_YCBCR_ENC_601;

Support for V4L2_YCBCR_ENC_709 and quantization options could be added
to the IPUv3 core code, so this limitation could be relaxed later.

> +		sdformat->format.colorspace = infmt->colorspace;
> +		sdformat->format.xfer_func = infmt->xfer_func;
>  	} else {
>  		v4l_bound_align_image(&sdformat->format.width,
>  				      MIN_W_SINK, MAX_W_SINK, W_ALIGN_SINK,
>  				      &sdformat->format.height,
>  				      MIN_H_SINK, MAX_H_SINK, H_ALIGN_SINK,
>  				      S_ALIGN);
> +
> +		imx_media_fill_empty_mbus_fields(&sdformat->format, infmt);

So we should limit quantization and ycbcr_enc here as well. Right now
there is no way to convert from limited range RGB to full range RGB, for
example.

>  	}
>  }
>  
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index b11e80f..730d1aa 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -1220,11 +1220,11 @@ static void csi_try_fmt(struct csi_priv *priv,
>  	struct v4l2_mbus_framefmt *infmt;
>  	u32 code;
>  
> +	infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD, sdformat->which);
> +
>  	switch (sdformat->pad) {
>  	case CSI_SRC_PAD_DIRECT:
>  	case CSI_SRC_PAD_IDMAC:
> -		infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD,
> -				      sdformat->which);
>  		incc = imx_media_find_mbus_format(infmt->code,
>  						  CS_SEL_ANY, true);
>  
> @@ -1260,6 +1260,12 @@ static void csi_try_fmt(struct csi_priv *priv,
>  			sdformat->format.field =  (infmt->height == 480) ?
>  				V4L2_FIELD_SEQ_TB : V4L2_FIELD_SEQ_BT;
>  		}
> +
> +		/* propagate colorimetry from sink */
> +		sdformat->format.colorspace = infmt->colorspace;
> +		sdformat->format.xfer_func = infmt->xfer_func;
> +		sdformat->format.quantization = infmt->quantization;
> +		sdformat->format.ycbcr_enc = infmt->ycbcr_enc;
>  		break;
>  	case CSI_SINK_PAD:
>  		v4l_bound_align_image(&sdformat->format.width, MIN_W, MAX_W,
> @@ -1286,6 +1292,8 @@ static void csi_try_fmt(struct csi_priv *priv,
>  							CS_SEL_ANY, false);
>  			sdformat->format.code = (*cc)->codes[0];
>  		}
> +
> +		imx_media_fill_empty_mbus_fields(&sdformat->format, infmt);
>  		break;
>  	}
>  }
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index 7b2f92d..b07d0ae 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -464,6 +464,59 @@ int imx_media_init_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
>  }
>  EXPORT_SYMBOL_GPL(imx_media_init_mbus_fmt);
>  
> +/*
> + * Check whether the field or colorimetry params in tryfmt are
> + * uninitialized, and if so fill them with the values from fmt.
> + * The exception is when tryfmt->colorspace has been initialized,
> + * if so all the further default colorimetry params can be derived
> + * from tryfmt->colorspace.
> + */
> +void imx_media_fill_empty_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
> +				      struct v4l2_mbus_framefmt *fmt)
> +{
> +	/* fill field if necessary */
> +	if (tryfmt->field == V4L2_FIELD_ANY)
> +		tryfmt->field = fmt->field;
> +
> +	/* fill colorimetry if necessary */
> +	if (tryfmt->colorspace == V4L2_COLORSPACE_DEFAULT) {
> +		tryfmt->colorspace = fmt->colorspace;
> +		if (tryfmt->xfer_func == V4L2_XFER_FUNC_DEFAULT)
> +			tryfmt->xfer_func = fmt->xfer_func;
> +		if (tryfmt->ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT)
> +			tryfmt->ycbcr_enc = fmt->ycbcr_enc;
> +		if (tryfmt->quantization == V4L2_QUANTIZATION_DEFAULT)
> +			tryfmt->quantization = fmt->quantization;
> +	} else {
> +		const struct imx_media_pixfmt *cc;
> +		bool is_rgb = false;
> +
> +		cc = imx_media_find_mbus_format(tryfmt->code,
> +						CS_SEL_ANY, false);
> +		if (!cc)
> +			cc = imx_media_find_ipu_format(tryfmt->code,
> +						       CS_SEL_ANY);
> +		if (cc && cc->cs != IPUV3_COLORSPACE_YUV)
> +			is_rgb = true;
> +
> +		if (tryfmt->xfer_func == V4L2_XFER_FUNC_DEFAULT) {
> +			tryfmt->xfer_func =
> +				V4L2_MAP_XFER_FUNC_DEFAULT(tryfmt->colorspace);
> +		}
> +		if (tryfmt->ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) {
> +			tryfmt->ycbcr_enc =
> +				V4L2_MAP_YCBCR_ENC_DEFAULT(tryfmt->colorspace);
> +		}
> +		if (tryfmt->quantization == V4L2_QUANTIZATION_DEFAULT) {
> +			tryfmt->quantization =
> +				V4L2_MAP_QUANTIZATION_DEFAULT(
> +					is_rgb, tryfmt->colorspace,
> +					tryfmt->ycbcr_enc);
> +		}
> +	}
> +}
> +EXPORT_SYMBOL_GPL(imx_media_fill_empty_mbus_fields);
> +

I'd s/empty/default/ in the function name.

After we agree on and write down some guidelines on expected behaviour
in the spec, this could be lifted into v4l2-core.

regards
Philipp

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

#1623178 — Re: [PATCH 40/40] media: imx: set and propagate empty field, colorimetry params

FromSteve Longerbeam <slongerbeam@gmail.com>
Date2017-04-13 18:50 +0200
SubjectRe: [PATCH 40/40] media: imx: set and propagate empty field, colorimetry params
Message-ID<tvQ6u-5Bw-17@gated-at.bofh.it>
In reply to#1622907

On 04/13/2017 03:09 AM, Philipp Zabel wrote:
> Hi Steve,
>
> thanks for cleaning this up. Apart from the comments below, this looks
> good to me.
>
> On Wed, 2017-04-12 at 17:45 -0700, Steve Longerbeam wrote:
>> This patch adds a call to imx_media_fill_empty_mbus_fields() in the
>> *_try_fmt() functions at the sink pads, to set empty field order and
>> colorimetry parameters.
>>
>> If the field order is set to ANY, choose the currently set field order
>> at the sink pad. If the colorspace is set to DEFAULT, choose the
>> current colorspace at the sink pad.  If any of xfer_func, ycbcr_enc
>> or quantization are set to DEFAULT, either choose the current sink pad
>> setting, or the default setting for the new colorspace, if non-DEFAULT
>> colorspace was given.
>>
>> Colorimetry is also propagated from sink to source pads anywhere
>> this has not already been done. The exception is ic-prpencvf at the
>> source pad, since the Image Converter outputs fixed quantization and
>> Y`CbCr encoding.
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> ---
>>  drivers/staging/media/imx/imx-ic-prp.c      |  5 ++-
>>  drivers/staging/media/imx/imx-ic-prpencvf.c | 25 +++++++++++---
>>  drivers/staging/media/imx/imx-media-csi.c   | 12 +++++--
>>  drivers/staging/media/imx/imx-media-utils.c | 53 +++++++++++++++++++++++++++++
>>  drivers/staging/media/imx/imx-media-vdic.c  |  7 ++--
>>  drivers/staging/media/imx/imx-media.h       |  3 +-
>>  6 files changed, 95 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/staging/media/imx/imx-ic-prp.c b/drivers/staging/media/imx/imx-ic-prp.c
>> index b4d4e48..8baa0d5 100644
>> --- a/drivers/staging/media/imx/imx-ic-prp.c
>> +++ b/drivers/staging/media/imx/imx-ic-prp.c
>> @@ -180,6 +180,8 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
>>  		goto out;
>>  	}
>>
>> +	fmt = __prp_get_fmt(priv, cfg, PRP_SINK_PAD, sdformat->which);
>> +
>
> Should this be changed to infmt for consistency?

sure, will do.

>
>>  	switch (sdformat->pad) {
>>  	case PRP_SINK_PAD:
>>  		v4l_bound_align_image(&sdformat->format.width, MIN_W, MAX_W,
>> @@ -193,11 +195,12 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
>>  			cc = imx_media_find_ipu_format(code, CS_SEL_ANY);
>>  			sdformat->format.code = cc->codes[0];
>>  		}
>> +
>> +		imx_media_fill_empty_mbus_fields(&sdformat->format, fmt);
>>  		break;
>>  	case PRP_SRC_PAD_PRPENC:
>>  	case PRP_SRC_PAD_PRPVF:
>>  		/* Output pads mirror input pad */
>> -		fmt = __prp_get_fmt(priv, cfg, PRP_SINK_PAD, sdformat->which);
>>  		sdformat->format = *fmt;
>>  		break;
>>  	}
>> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
>> index 860b406..cdfb51a 100644
>> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
>> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
>> @@ -772,6 +772,8 @@ static void prp_try_fmt(struct prp_priv *priv,
>>  			struct v4l2_subdev_format *sdformat,
>>  			const struct imx_media_pixfmt **cc)
>>  {
>> +	struct v4l2_mbus_framefmt *infmt;
>> +
>>  	*cc = imx_media_find_ipu_format(sdformat->format.code, CS_SEL_ANY);
>>  	if (!*cc) {
>>  		u32 code;
>> @@ -781,11 +783,9 @@ static void prp_try_fmt(struct prp_priv *priv,
>>  		sdformat->format.code = (*cc)->codes[0];
>>  	}
>>
>> -	if (sdformat->pad == PRPENCVF_SRC_PAD) {
>> -		struct v4l2_mbus_framefmt *infmt =
>> -			__prp_get_fmt(priv, cfg, PRPENCVF_SINK_PAD,
>> -				      sdformat->which);
>> +	infmt = __prp_get_fmt(priv, cfg, PRPENCVF_SINK_PAD, sdformat->which);
>>
>> +	if (sdformat->pad == PRPENCVF_SRC_PAD) {
>>  		if (sdformat->format.field != V4L2_FIELD_NONE)
>>  			sdformat->format.field = infmt->field;
>>
>> @@ -804,12 +804,29 @@ static void prp_try_fmt(struct prp_priv *priv,
>>  					      &sdformat->format.height,
>>  					      infmt->height / 4, MAX_H_SRC,
>>  					      H_ALIGN_SRC, S_ALIGN);
>> +
>> +		/*
>> +		 * The Image Converter produces fixed quantization
>> +		 * (full range for RGB, limited range for YUV), and
>> +		 * uses a fixed Y`CbCr encoding (V4L2_YCBCR_ENC_601).
>> +		 * For colorspace and transfer func, just propagate
>> +		 * from the sink.
>> +		 */
>> +		sdformat->format.quantization =
>> +			((*cc)->cs != IPUV3_COLORSPACE_YUV) ?
>> +			V4L2_QUANTIZATION_FULL_RANGE :
>> +			V4L2_QUANTIZATION_LIM_RANGE;
>> +		sdformat->format.ycbcr_enc = V4L2_YCBCR_ENC_601;
>
> Support for V4L2_YCBCR_ENC_709 and quantization options could be added
> to the IPUv3 core code, so this limitation could be relaxed later.

Yes, I was going to mention that too. We can add coefficient tables
to ipu-ic for all the encodings enumerated in enum v4l2_ycbcr_encoding.

I know that quantization is programmable in the DP, but is it in the
IC? AFAICT there is none.

>
>> +		sdformat->format.colorspace = infmt->colorspace;
>> +		sdformat->format.xfer_func = infmt->xfer_func;
>>  	} else {
>>  		v4l_bound_align_image(&sdformat->format.width,
>>  				      MIN_W_SINK, MAX_W_SINK, W_ALIGN_SINK,
>>  				      &sdformat->format.height,
>>  				      MIN_H_SINK, MAX_H_SINK, H_ALIGN_SINK,
>>  				      S_ALIGN);
>> +
>> +		imx_media_fill_empty_mbus_fields(&sdformat->format, infmt);
>
> So we should limit quantization and ycbcr_enc here as well. Right now
> there is no way to convert from limited range RGB to full range RGB, for
> example.

True, yes, I was scratching my head about that. I'll move the above to
apply to all pads.

>
>>  	}
>>  }
>>
>> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
>> index b11e80f..730d1aa 100644
>> --- a/drivers/staging/media/imx/imx-media-csi.c
>> +++ b/drivers/staging/media/imx/imx-media-csi.c
>> @@ -1220,11 +1220,11 @@ static void csi_try_fmt(struct csi_priv *priv,
>>  	struct v4l2_mbus_framefmt *infmt;
>>  	u32 code;
>>
>> +	infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD, sdformat->which);
>> +
>>  	switch (sdformat->pad) {
>>  	case CSI_SRC_PAD_DIRECT:
>>  	case CSI_SRC_PAD_IDMAC:
>> -		infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD,
>> -				      sdformat->which);
>>  		incc = imx_media_find_mbus_format(infmt->code,
>>  						  CS_SEL_ANY, true);
>>
>> @@ -1260,6 +1260,12 @@ static void csi_try_fmt(struct csi_priv *priv,
>>  			sdformat->format.field =  (infmt->height == 480) ?
>>  				V4L2_FIELD_SEQ_TB : V4L2_FIELD_SEQ_BT;
>>  		}
>> +
>> +		/* propagate colorimetry from sink */
>> +		sdformat->format.colorspace = infmt->colorspace;
>> +		sdformat->format.xfer_func = infmt->xfer_func;
>> +		sdformat->format.quantization = infmt->quantization;
>> +		sdformat->format.ycbcr_enc = infmt->ycbcr_enc;
>>  		break;
>>  	case CSI_SINK_PAD:
>>  		v4l_bound_align_image(&sdformat->format.width, MIN_W, MAX_W,
>> @@ -1286,6 +1292,8 @@ static void csi_try_fmt(struct csi_priv *priv,
>>  							CS_SEL_ANY, false);
>>  			sdformat->format.code = (*cc)->codes[0];
>>  		}
>> +
>> +		imx_media_fill_empty_mbus_fields(&sdformat->format, infmt);
>>  		break;
>>  	}
>>  }
>> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
>> index 7b2f92d..b07d0ae 100644
>> --- a/drivers/staging/media/imx/imx-media-utils.c
>> +++ b/drivers/staging/media/imx/imx-media-utils.c
>> @@ -464,6 +464,59 @@ int imx_media_init_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
>>  }
>>  EXPORT_SYMBOL_GPL(imx_media_init_mbus_fmt);
>>
>> +/*
>> + * Check whether the field or colorimetry params in tryfmt are
>> + * uninitialized, and if so fill them with the values from fmt.
>> + * The exception is when tryfmt->colorspace has been initialized,
>> + * if so all the further default colorimetry params can be derived
>> + * from tryfmt->colorspace.
>> + */
>> +void imx_media_fill_empty_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
>> +				      struct v4l2_mbus_framefmt *fmt)
>> +{
>> +	/* fill field if necessary */
>> +	if (tryfmt->field == V4L2_FIELD_ANY)
>> +		tryfmt->field = fmt->field;
>> +
>> +	/* fill colorimetry if necessary */
>> +	if (tryfmt->colorspace == V4L2_COLORSPACE_DEFAULT) {
>> +		tryfmt->colorspace = fmt->colorspace;
>> +		if (tryfmt->xfer_func == V4L2_XFER_FUNC_DEFAULT)
>> +			tryfmt->xfer_func = fmt->xfer_func;
>> +		if (tryfmt->ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT)
>> +			tryfmt->ycbcr_enc = fmt->ycbcr_enc;
>> +		if (tryfmt->quantization == V4L2_QUANTIZATION_DEFAULT)
>> +			tryfmt->quantization = fmt->quantization;
>> +	} else {
>> +		const struct imx_media_pixfmt *cc;
>> +		bool is_rgb = false;
>> +
>> +		cc = imx_media_find_mbus_format(tryfmt->code,
>> +						CS_SEL_ANY, false);
>> +		if (!cc)
>> +			cc = imx_media_find_ipu_format(tryfmt->code,
>> +						       CS_SEL_ANY);
>> +		if (cc && cc->cs != IPUV3_COLORSPACE_YUV)
>> +			is_rgb = true;
>> +
>> +		if (tryfmt->xfer_func == V4L2_XFER_FUNC_DEFAULT) {
>> +			tryfmt->xfer_func =
>> +				V4L2_MAP_XFER_FUNC_DEFAULT(tryfmt->colorspace);
>> +		}
>> +		if (tryfmt->ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) {
>> +			tryfmt->ycbcr_enc =
>> +				V4L2_MAP_YCBCR_ENC_DEFAULT(tryfmt->colorspace);
>> +		}
>> +		if (tryfmt->quantization == V4L2_QUANTIZATION_DEFAULT) {
>> +			tryfmt->quantization =
>> +				V4L2_MAP_QUANTIZATION_DEFAULT(
>> +					is_rgb, tryfmt->colorspace,
>> +					tryfmt->ycbcr_enc);
>> +		}
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(imx_media_fill_empty_mbus_fields);
>> +
>
> I'd s/empty/default/ in the function name.

sounds good.

>
> After we agree on and write down some guidelines on expected behaviour
> in the spec, this could be lifted into v4l2-core.

Agreed.

Steve

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

#1625183 — Re: [PATCH 40/40] media: imx: set and propagate empty field, colorimetry params

FromPhilipp Zabel <p.zabel@pengutronix.de>
Date2017-04-18 11:40 +0200
SubjectRe: [PATCH 40/40] media: imx: set and propagate empty field, colorimetry params
Message-ID<txxM6-4vX-25@gated-at.bofh.it>
In reply to#1623178
On Thu, 2017-04-13 at 09:40 -0700, Steve Longerbeam wrote:
[...]
> >> @@ -804,12 +804,29 @@ static void prp_try_fmt(struct prp_priv *priv,
> >>  					      &sdformat->format.height,
> >>  					      infmt->height / 4, MAX_H_SRC,
> >>  					      H_ALIGN_SRC, S_ALIGN);
> >> +
> >> +		/*
> >> +		 * The Image Converter produces fixed quantization
> >> +		 * (full range for RGB, limited range for YUV), and
> >> +		 * uses a fixed Y`CbCr encoding (V4L2_YCBCR_ENC_601).
> >> +		 * For colorspace and transfer func, just propagate
> >> +		 * from the sink.
> >> +		 */
> >> +		sdformat->format.quantization =
> >> +			((*cc)->cs != IPUV3_COLORSPACE_YUV) ?
> >> +			V4L2_QUANTIZATION_FULL_RANGE :
> >> +			V4L2_QUANTIZATION_LIM_RANGE;
> >> +		sdformat->format.ycbcr_enc = V4L2_YCBCR_ENC_601;
> >
> > Support for V4L2_YCBCR_ENC_709 and quantization options could be added
> > to the IPUv3 core code, so this limitation could be relaxed later.
> 
> Yes, I was going to mention that too. We can add coefficient tables
> to ipu-ic for all the encodings enumerated in enum v4l2_ycbcr_encoding.

Exactly.

> I know that quantization is programmable in the DP, but is it in the
> IC? AFAICT there is none.

We have a freely programmable 4x3 matrix multiplication both before and
after processing in each task, and there is a saturation mode switch
that can limit the first component to (16...235) and the other two to
(16...240). That should be enough for at least full/limited range YCbCr
quantizations. So we apparently can't saturate to limited range RGB, but
for example full-range -> limited-range RGB conversions should be
perfectly possible.

regards
Philipp

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web