[v8,5/5] media: imx: Try colorimetry at both sink and source pads

Commit Message

Steve Longerbeam May 22, 2019, 1:03 a.m. UTC

Retask imx_media_fill_default_mbus_fields() to try colorimetry parameters,
renaming it to to imx_media_try_colorimetry(), and call it at both sink and
source pad try_fmt's. The unrelated check for uninitialized field value is
moved out to appropriate places in each subdev try_fmt.

The IC now supports Rec.709 and BT.601 Y'CbCr encoding, and both limited
and full range quantization for both YUV and RGB space, so allow those
for pipelines that route through the IC.

Signed-off-by: Steve Longerbeam <[e-mail]>
---
Changes in v7:
- squashed with "media: imx: Allow Rec.709 encoding for IC routes".
- remove the RGB full-range quantization restriction for IC routes.
---
 drivers/staging/media/imx/imx-ic-prp.c      |  6 +-
 drivers/staging/media/imx/imx-ic-prpencvf.c |  8 +--
 drivers/staging/media/imx/imx-media-csi.c   | 19 +++---
 drivers/staging/media/imx/imx-media-utils.c | 73 ++++++++++-----------
 drivers/staging/media/imx/imx-media-vdic.c  |  5 +-
 drivers/staging/media/imx/imx-media.h       |  5 +-
 drivers/staging/media/imx/imx7-media-csi.c  |  8 +--
 7 files changed, 62 insertions(+), 62 deletions(-)

Comments

Hans Verkuil May 27, 2019, 9:47 a.m. UTC | #1

On 5/22/19 3:03 AM, Steve Longerbeam wrote:
> Retask imx_media_fill_default_mbus_fields() to try colorimetry parameters,
> renaming it to to imx_media_try_colorimetry(), and call it at both sink and
> source pad try_fmt's. The unrelated check for uninitialized field value is
> moved out to appropriate places in each subdev try_fmt.
> 
> The IC now supports Rec.709 and BT.601 Y'CbCr encoding, and both limited
> and full range quantization for both YUV and RGB space, so allow those
> for pipelines that route through the IC.
> 
> Signed-off-by: Steve Longerbeam <[e-mail]>

Acked-by: Hans Verkuil <[e-mail]>

It's probably easiest to apply this whole series to the gpu subsystem.
You have my Acked-by for this media patch.

Regards,

	Hans

> ---
> Changes in v7:
> - squashed with "media: imx: Allow Rec.709 encoding for IC routes".
> - remove the RGB full-range quantization restriction for IC routes.
> ---
>  drivers/staging/media/imx/imx-ic-prp.c      |  6 +-
>  drivers/staging/media/imx/imx-ic-prpencvf.c |  8 +--
>  drivers/staging/media/imx/imx-media-csi.c   | 19 +++---
>  drivers/staging/media/imx/imx-media-utils.c | 73 ++++++++++-----------
>  drivers/staging/media/imx/imx-media-vdic.c  |  5 +-
>  drivers/staging/media/imx/imx-media.h       |  5 +-
>  drivers/staging/media/imx/imx7-media-csi.c  |  8 +--
>  7 files changed, 62 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx-ic-prp.c b/drivers/staging/media/imx/imx-ic-prp.c
> index 10ffe00f1a54..f87fe0203720 100644
> --- a/drivers/staging/media/imx/imx-ic-prp.c
> +++ b/drivers/staging/media/imx/imx-ic-prp.c
> @@ -193,8 +193,8 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
>  			sdformat->format.code = cc->codes[0];
>  		}
>  
> -		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
> -						   true);
> +		if (sdformat->format.field == V4L2_FIELD_ANY)
> +			sdformat->format.field = V4L2_FIELD_NONE;
>  		break;
>  	case PRP_SRC_PAD_PRPENC:
>  	case PRP_SRC_PAD_PRPVF:
> @@ -203,6 +203,8 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
>  		break;
>  	}
>  
> +	imx_media_try_colorimetry(&sdformat->format, true);
> +
>  	fmt = __prp_get_fmt(priv, cfg, sdformat->pad, sdformat->which);
>  	*fmt = sdformat->format;
>  out:
> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
> index e8b36a181ccc..f2fe3c11c70e 100644
> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> @@ -907,8 +907,6 @@ static void prp_try_fmt(struct prp_priv *priv,
>  		/* 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;
>  	} else {
>  		v4l_bound_align_image(&sdformat->format.width,
>  				      MIN_W_SINK, MAX_W_SINK, W_ALIGN_SINK,
> @@ -916,9 +914,11 @@ static void prp_try_fmt(struct prp_priv *priv,
>  				      MIN_H_SINK, MAX_H_SINK, H_ALIGN_SINK,
>  				      S_ALIGN);
>  
> -		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
> -						   true);
> +		if (sdformat->format.field == V4L2_FIELD_ANY)
> +			sdformat->format.field = V4L2_FIELD_NONE;
>  	}
> +
> +	imx_media_try_colorimetry(&sdformat->format, true);
>  }
>  
>  static int prp_set_fmt(struct v4l2_subdev *sd,
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index 1d248aca40a9..dce4addadff4 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -1375,9 +1375,15 @@ static void csi_try_field(struct csi_priv *priv,
>  	struct v4l2_mbus_framefmt *infmt =
>  		__csi_get_fmt(priv, cfg, CSI_SINK_PAD, sdformat->which);
>  
> -	/* no restrictions on sink pad field type */
> -	if (sdformat->pad == CSI_SINK_PAD)
> +	/*
> +	 * no restrictions on sink pad field type except must
> +	 * be initialized.
> +	 */
> +	if (sdformat->pad == CSI_SINK_PAD) {
> +		if (sdformat->format.field == V4L2_FIELD_ANY)
> +			sdformat->format.field = V4L2_FIELD_NONE;
>  		return;
> +	}
>  
>  	switch (infmt->field) {
>  	case V4L2_FIELD_SEQ_TB:
> @@ -1455,8 +1461,6 @@ static void csi_try_fmt(struct csi_priv *priv,
>  		/* 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:
> @@ -1476,10 +1480,6 @@ static void csi_try_fmt(struct csi_priv *priv,
>  
>  		csi_try_field(priv, cfg, sdformat);
>  
> -		imx_media_fill_default_mbus_fields(
> -			&sdformat->format, infmt,
> -			priv->active_output_pad == CSI_SRC_PAD_DIRECT);
> -
>  		/* Reset crop and compose rectangles */
>  		crop->left = 0;
>  		crop->top = 0;
> @@ -1495,6 +1495,9 @@ static void csi_try_fmt(struct csi_priv *priv,
>  
>  		break;
>  	}
> +
> +	imx_media_try_colorimetry(&sdformat->format,
> +			priv->active_output_pad == CSI_SRC_PAD_DIRECT);
>  }
>  
>  static int csi_set_fmt(struct v4l2_subdev *sd,
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index b41842dba5ec..05b63395084e 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -511,21 +511,18 @@ int imx_media_init_cfg(struct v4l2_subdev *sd,
>  EXPORT_SYMBOL_GPL(imx_media_init_cfg);
>  
>  /*
> - * Check whether the field and colorimetry parameters in tryfmt are
> - * uninitialized, and if so fill them with the values from fmt,
> - * or if tryfmt->colorspace has been initialized, all the default
> - * colorimetry params can be derived from tryfmt->colorspace.
> + * Default the colorspace in tryfmt to SRGB if set to an unsupported
> + * colorspace or not initialized. Then set the remaining colorimetry
> + * parameters based on the colorspace if they are uninitialized.
>   *
>   * tryfmt->code must be set on entry.
>   *
>   * If this format is destined to be routed through the Image Converter,
> - * quantization and Y`CbCr encoding must be fixed. The IC expects and
> - * produces fixed quantization and Y`CbCr encoding at its input and output
> - * (full range for RGB, limited range for YUV, and V4L2_YCBCR_ENC_601).
> + * Y`CbCr encoding must be fixed. The IC supports only BT.601 Y`CbCr
> + * or Rec.709 Y`CbCr encoding.
>   */
> -void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
> -					struct v4l2_mbus_framefmt *fmt,
> -					bool ic_route)
> +void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
> +			       bool ic_route)
>  {
>  	const struct imx_media_pixfmt *cc;
>  	bool is_rgb = false;
> @@ -533,44 +530,46 @@ void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
>  	cc = imx_media_find_mbus_format(tryfmt->code, CS_SEL_ANY, true);
>  	if (!cc)
>  		cc = imx_media_find_ipu_format(tryfmt->code, CS_SEL_ANY);
> -	if (cc && cc->cs != IPUV3_COLORSPACE_YUV)
> +	if (cc && cc->cs == IPUV3_COLORSPACE_RGB)
>  		is_rgb = true;
>  
> -	/* fill field if necessary */
> -	if (tryfmt->field == V4L2_FIELD_ANY)
> -		tryfmt->field = fmt->field;
> +	switch (tryfmt->colorspace) {
> +	case V4L2_COLORSPACE_SMPTE170M:
> +	case V4L2_COLORSPACE_REC709:
> +	case V4L2_COLORSPACE_JPEG:
> +	case V4L2_COLORSPACE_SRGB:
> +	case V4L2_COLORSPACE_BT2020:
> +	case V4L2_COLORSPACE_OPRGB:
> +	case V4L2_COLORSPACE_DCI_P3:
> +	case V4L2_COLORSPACE_RAW:
> +		break;
> +	default:
> +		tryfmt->colorspace = V4L2_COLORSPACE_SRGB;
> +		break;
> +	}
> +
> +	if (tryfmt->xfer_func == V4L2_XFER_FUNC_DEFAULT)
> +		tryfmt->xfer_func =
> +			V4L2_MAP_XFER_FUNC_DEFAULT(tryfmt->colorspace);
>  
> -	/* fill colorimetry if necessary */
> -	if (tryfmt->colorspace == V4L2_COLORSPACE_DEFAULT) {
> -		tryfmt->colorspace = fmt->colorspace;
> -		tryfmt->xfer_func = fmt->xfer_func;
> -		tryfmt->ycbcr_enc = fmt->ycbcr_enc;
> -		tryfmt->quantization = fmt->quantization;
> +	if (ic_route) {
> +		if (tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_601 &&
> +		    tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_709)
> +			tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>  	} else {
> -		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);
> -		}
>  	}
>  
> -	if (ic_route) {
> -		tryfmt->quantization = is_rgb ?
> -			V4L2_QUANTIZATION_FULL_RANGE :
> -			V4L2_QUANTIZATION_LIM_RANGE;
> -		tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
> -	}
> +	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_default_mbus_fields);
> +EXPORT_SYMBOL_GPL(imx_media_try_colorimetry);
>  
>  int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>  				  struct v4l2_rect *compose,
> diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c
> index 4487374c9435..fbafd7fb7aeb 100644
> --- a/drivers/staging/media/imx/imx-media-vdic.c
> +++ b/drivers/staging/media/imx/imx-media-vdic.c
> @@ -617,14 +617,13 @@ static void vdic_try_fmt(struct vdic_priv *priv,
>  				      &sdformat->format.height,
>  				      MIN_H, MAX_H_VDIC, H_ALIGN, S_ALIGN);
>  
> -		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
> -						   true);
> -
>  		/* input must be interlaced! Choose SEQ_TB if not */
>  		if (!V4L2_FIELD_HAS_BOTH(sdformat->format.field))
>  			sdformat->format.field = V4L2_FIELD_SEQ_TB;
>  		break;
>  	}
> +
> +	imx_media_try_colorimetry(&sdformat->format, true);
>  }
>  
>  static int vdic_set_fmt(struct v4l2_subdev *sd,
> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> index 6587aa49e005..23024c9bc887 100644
> --- a/drivers/staging/media/imx/imx-media.h
> +++ b/drivers/staging/media/imx/imx-media.h
> @@ -172,9 +172,8 @@ int imx_media_init_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
>  			    const struct imx_media_pixfmt **cc);
>  int imx_media_init_cfg(struct v4l2_subdev *sd,
>  		       struct v4l2_subdev_pad_config *cfg);
> -void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
> -					struct v4l2_mbus_framefmt *fmt,
> -					bool ic_route);
> +void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
> +			       bool ic_route);
>  int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>  				  struct v4l2_rect *compose,
>  				  const struct v4l2_mbus_framefmt *mbus,
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index a708a0340eb1..6e2f4c3eb24f 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -1003,8 +1003,6 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>  
>  		sdformat->format.colorspace = in_fmt->colorspace;
>  		sdformat->format.xfer_func = in_fmt->xfer_func;
> -		sdformat->format.quantization = in_fmt->quantization;
> -		sdformat->format.ycbcr_enc = in_fmt->ycbcr_enc;
>  		break;
>  	case IMX7_CSI_PAD_SINK:
>  		*cc = imx_media_find_mbus_format(sdformat->format.code,
> @@ -1015,14 +1013,14 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>  							 false);
>  			sdformat->format.code = (*cc)->codes[0];
>  		}
> -
> -		imx_media_fill_default_mbus_fields(&sdformat->format, in_fmt,
> -						   false);
>  		break;
>  	default:
>  		return -EINVAL;
>  		break;
>  	}
> +
> +	imx_media_try_colorimetry(&sdformat->format, false);
> +
>  	return 0;
>  }
>  
>

Philipp Zabel June 14, 2019, 2:10 p.m. UTC | #2

Hi Steve,

On Tue, 2019-05-21 at 18:03 -0700, Steve Longerbeam wrote:
> Retask imx_media_fill_default_mbus_fields() to try colorimetry parameters,
> renaming it to to imx_media_try_colorimetry(), and call it at both sink and
> source pad try_fmt's. The unrelated check for uninitialized field value is
> moved out to appropriate places in each subdev try_fmt.
> 
> The IC now supports Rec.709 and BT.601 Y'CbCr encoding, and both limited
> and full range quantization for both YUV and RGB space, so allow those
> for pipelines that route through the IC.
> 
> Signed-off-by: Steve Longerbeam <[e-mail]>

I've applied them on the imx-drm/next branch with Hans' Acked-by on 5/5.

regards
Philipp

Laurent Pinchart Oct. 22, 2019, 1:44 a.m. UTC | #3

Hi Steve,

On Tue, May 21, 2019 at 06:03:17PM -0700, Steve Longerbeam wrote:
> Retask imx_media_fill_default_mbus_fields() to try colorimetry parameters,
> renaming it to to imx_media_try_colorimetry(), and call it at both sink and
> source pad try_fmt's. The unrelated check for uninitialized field value is
> moved out to appropriate places in each subdev try_fmt.
> 
> The IC now supports Rec.709 and BT.601 Y'CbCr encoding, and both limited
> and full range quantization for both YUV and RGB space, so allow those
> for pipelines that route through the IC.
> 
> Signed-off-by: Steve Longerbeam <[e-mail]>
> ---
> Changes in v7:
> - squashed with "media: imx: Allow Rec.709 encoding for IC routes".
> - remove the RGB full-range quantization restriction for IC routes.
> ---
>  drivers/staging/media/imx/imx-ic-prp.c      |  6 +-
>  drivers/staging/media/imx/imx-ic-prpencvf.c |  8 +--
>  drivers/staging/media/imx/imx-media-csi.c   | 19 +++---
>  drivers/staging/media/imx/imx-media-utils.c | 73 ++++++++++-----------
>  drivers/staging/media/imx/imx-media-vdic.c  |  5 +-
>  drivers/staging/media/imx/imx-media.h       |  5 +-
>  drivers/staging/media/imx/imx7-media-csi.c  |  8 +--
>  7 files changed, 62 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx-ic-prp.c b/drivers/staging/media/imx/imx-ic-prp.c
> index 10ffe00f1a54..f87fe0203720 100644
> --- a/drivers/staging/media/imx/imx-ic-prp.c
> +++ b/drivers/staging/media/imx/imx-ic-prp.c
> @@ -193,8 +193,8 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
>  			sdformat->format.code = cc->codes[0];
>  		}
>  
> -		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
> -						   true);
> +		if (sdformat->format.field == V4L2_FIELD_ANY)
> +			sdformat->format.field = V4L2_FIELD_NONE;
>  		break;
>  	case PRP_SRC_PAD_PRPENC:
>  	case PRP_SRC_PAD_PRPVF:
> @@ -203,6 +203,8 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
>  		break;
>  	}
>  
> +	imx_media_try_colorimetry(&sdformat->format, true);
> +
>  	fmt = __prp_get_fmt(priv, cfg, sdformat->pad, sdformat->which);
>  	*fmt = sdformat->format;
>  out:
> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
> index e8b36a181ccc..f2fe3c11c70e 100644
> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> @@ -907,8 +907,6 @@ static void prp_try_fmt(struct prp_priv *priv,
>  		/* 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;
>  	} else {
>  		v4l_bound_align_image(&sdformat->format.width,
>  				      MIN_W_SINK, MAX_W_SINK, W_ALIGN_SINK,
> @@ -916,9 +914,11 @@ static void prp_try_fmt(struct prp_priv *priv,
>  				      MIN_H_SINK, MAX_H_SINK, H_ALIGN_SINK,
>  				      S_ALIGN);
>  
> -		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
> -						   true);
> +		if (sdformat->format.field == V4L2_FIELD_ANY)
> +			sdformat->format.field = V4L2_FIELD_NONE;
>  	}
> +
> +	imx_media_try_colorimetry(&sdformat->format, true);
>  }
>  
>  static int prp_set_fmt(struct v4l2_subdev *sd,
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index 1d248aca40a9..dce4addadff4 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -1375,9 +1375,15 @@ static void csi_try_field(struct csi_priv *priv,
>  	struct v4l2_mbus_framefmt *infmt =
>  		__csi_get_fmt(priv, cfg, CSI_SINK_PAD, sdformat->which);
>  
> -	/* no restrictions on sink pad field type */
> -	if (sdformat->pad == CSI_SINK_PAD)
> +	/*
> +	 * no restrictions on sink pad field type except must
> +	 * be initialized.
> +	 */
> +	if (sdformat->pad == CSI_SINK_PAD) {
> +		if (sdformat->format.field == V4L2_FIELD_ANY)
> +			sdformat->format.field = V4L2_FIELD_NONE;
>  		return;
> +	}
>  
>  	switch (infmt->field) {
>  	case V4L2_FIELD_SEQ_TB:
> @@ -1455,8 +1461,6 @@ static void csi_try_fmt(struct csi_priv *priv,
>  		/* 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:
> @@ -1476,10 +1480,6 @@ static void csi_try_fmt(struct csi_priv *priv,
>  
>  		csi_try_field(priv, cfg, sdformat);
>  
> -		imx_media_fill_default_mbus_fields(
> -			&sdformat->format, infmt,
> -			priv->active_output_pad == CSI_SRC_PAD_DIRECT);
> -
>  		/* Reset crop and compose rectangles */
>  		crop->left = 0;
>  		crop->top = 0;
> @@ -1495,6 +1495,9 @@ static void csi_try_fmt(struct csi_priv *priv,
>  
>  		break;
>  	}
> +
> +	imx_media_try_colorimetry(&sdformat->format,
> +			priv->active_output_pad == CSI_SRC_PAD_DIRECT);
>  }
>  
>  static int csi_set_fmt(struct v4l2_subdev *sd,
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index b41842dba5ec..05b63395084e 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -511,21 +511,18 @@ int imx_media_init_cfg(struct v4l2_subdev *sd,
>  EXPORT_SYMBOL_GPL(imx_media_init_cfg);
>  
>  /*
> - * Check whether the field and colorimetry parameters in tryfmt are
> - * uninitialized, and if so fill them with the values from fmt,
> - * or if tryfmt->colorspace has been initialized, all the default
> - * colorimetry params can be derived from tryfmt->colorspace.
> + * Default the colorspace in tryfmt to SRGB if set to an unsupported
> + * colorspace or not initialized. Then set the remaining colorimetry
> + * parameters based on the colorspace if they are uninitialized.
>   *
>   * tryfmt->code must be set on entry.
>   *
>   * If this format is destined to be routed through the Image Converter,
> - * quantization and Y`CbCr encoding must be fixed. The IC expects and
> - * produces fixed quantization and Y`CbCr encoding at its input and output
> - * (full range for RGB, limited range for YUV, and V4L2_YCBCR_ENC_601).
> + * Y`CbCr encoding must be fixed. The IC supports only BT.601 Y`CbCr
> + * or Rec.709 Y`CbCr encoding.
>   */
> -void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
> -					struct v4l2_mbus_framefmt *fmt,
> -					bool ic_route)
> +void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
> +			       bool ic_route)
>  {
>  	const struct imx_media_pixfmt *cc;
>  	bool is_rgb = false;
> @@ -533,44 +530,46 @@ void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
>  	cc = imx_media_find_mbus_format(tryfmt->code, CS_SEL_ANY, true);
>  	if (!cc)
>  		cc = imx_media_find_ipu_format(tryfmt->code, CS_SEL_ANY);
> -	if (cc && cc->cs != IPUV3_COLORSPACE_YUV)
> +	if (cc && cc->cs == IPUV3_COLORSPACE_RGB)
>  		is_rgb = true;
>  
> -	/* fill field if necessary */
> -	if (tryfmt->field == V4L2_FIELD_ANY)
> -		tryfmt->field = fmt->field;

Have you tested this patch on i.MX7 ? It breaks pipeline validation as
the field field isn't handled anymore in imx7_csi_try_fmt(), due to the
removal if this code.

I have a written a fix, I'll prepare a patch.

> +	switch (tryfmt->colorspace) {
> +	case V4L2_COLORSPACE_SMPTE170M:
> +	case V4L2_COLORSPACE_REC709:
> +	case V4L2_COLORSPACE_JPEG:
> +	case V4L2_COLORSPACE_SRGB:
> +	case V4L2_COLORSPACE_BT2020:
> +	case V4L2_COLORSPACE_OPRGB:
> +	case V4L2_COLORSPACE_DCI_P3:
> +	case V4L2_COLORSPACE_RAW:
> +		break;
> +	default:
> +		tryfmt->colorspace = V4L2_COLORSPACE_SRGB;
> +		break;
> +	}
> +
> +	if (tryfmt->xfer_func == V4L2_XFER_FUNC_DEFAULT)
> +		tryfmt->xfer_func =
> +			V4L2_MAP_XFER_FUNC_DEFAULT(tryfmt->colorspace);
>  
> -	/* fill colorimetry if necessary */
> -	if (tryfmt->colorspace == V4L2_COLORSPACE_DEFAULT) {
> -		tryfmt->colorspace = fmt->colorspace;
> -		tryfmt->xfer_func = fmt->xfer_func;
> -		tryfmt->ycbcr_enc = fmt->ycbcr_enc;
> -		tryfmt->quantization = fmt->quantization;
> +	if (ic_route) {
> +		if (tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_601 &&
> +		    tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_709)
> +			tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>  	} else {
> -		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);
> -		}
>  	}
>  
> -	if (ic_route) {
> -		tryfmt->quantization = is_rgb ?
> -			V4L2_QUANTIZATION_FULL_RANGE :
> -			V4L2_QUANTIZATION_LIM_RANGE;
> -		tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
> -	}
> +	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_default_mbus_fields);
> +EXPORT_SYMBOL_GPL(imx_media_try_colorimetry);
>  
>  int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>  				  struct v4l2_rect *compose,
> diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c
> index 4487374c9435..fbafd7fb7aeb 100644
> --- a/drivers/staging/media/imx/imx-media-vdic.c
> +++ b/drivers/staging/media/imx/imx-media-vdic.c
> @@ -617,14 +617,13 @@ static void vdic_try_fmt(struct vdic_priv *priv,
>  				      &sdformat->format.height,
>  				      MIN_H, MAX_H_VDIC, H_ALIGN, S_ALIGN);
>  
> -		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
> -						   true);
> -
>  		/* input must be interlaced! Choose SEQ_TB if not */
>  		if (!V4L2_FIELD_HAS_BOTH(sdformat->format.field))
>  			sdformat->format.field = V4L2_FIELD_SEQ_TB;
>  		break;
>  	}
> +
> +	imx_media_try_colorimetry(&sdformat->format, true);
>  }
>  
>  static int vdic_set_fmt(struct v4l2_subdev *sd,
> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> index 6587aa49e005..23024c9bc887 100644
> --- a/drivers/staging/media/imx/imx-media.h
> +++ b/drivers/staging/media/imx/imx-media.h
> @@ -172,9 +172,8 @@ int imx_media_init_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
>  			    const struct imx_media_pixfmt **cc);
>  int imx_media_init_cfg(struct v4l2_subdev *sd,
>  		       struct v4l2_subdev_pad_config *cfg);
> -void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
> -					struct v4l2_mbus_framefmt *fmt,
> -					bool ic_route);
> +void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
> +			       bool ic_route);
>  int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>  				  struct v4l2_rect *compose,
>  				  const struct v4l2_mbus_framefmt *mbus,
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index a708a0340eb1..6e2f4c3eb24f 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -1003,8 +1003,6 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>  
>  		sdformat->format.colorspace = in_fmt->colorspace;
>  		sdformat->format.xfer_func = in_fmt->xfer_func;
> -		sdformat->format.quantization = in_fmt->quantization;
> -		sdformat->format.ycbcr_enc = in_fmt->ycbcr_enc;
>  		break;
>  	case IMX7_CSI_PAD_SINK:
>  		*cc = imx_media_find_mbus_format(sdformat->format.code,
> @@ -1015,14 +1013,14 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>  							 false);
>  			sdformat->format.code = (*cc)->codes[0];
>  		}
> -
> -		imx_media_fill_default_mbus_fields(&sdformat->format, in_fmt,
> -						   false);
>  		break;
>  	default:
>  		return -EINVAL;
>  		break;
>  	}
> +
> +	imx_media_try_colorimetry(&sdformat->format, false);
> +
>  	return 0;
>  }
>  
> -- 
> 2.17.1
>

Steve Longerbeam Oct. 22, 2019, 3:26 a.m. UTC | #4

Hi Laurent,

On 10/21/19 6:44 PM, Laurent Pinchart wrote:
> Hi Steve,
>
> On Tue, May 21, 2019 at 06:03:17PM -0700, Steve Longerbeam wrote:
>> Retask imx_media_fill_default_mbus_fields() to try colorimetry parameters,
>> renaming it to to imx_media_try_colorimetry(), and call it at both sink and
>> source pad try_fmt's. The unrelated check for uninitialized field value is
>> moved out to appropriate places in each subdev try_fmt.
>>
>> The IC now supports Rec.709 and BT.601 Y'CbCr encoding, and both limited
>> and full range quantization for both YUV and RGB space, so allow those
>> for pipelines that route through the IC.
>>
>> Signed-off-by: Steve Longerbeam <[e-mail]>
>> ---
>> Changes in v7:
>> - squashed with "media: imx: Allow Rec.709 encoding for IC routes".
>> - remove the RGB full-range quantization restriction for IC routes.
>> ---
>>   drivers/staging/media/imx/imx-ic-prp.c      |  6 +-
>>   drivers/staging/media/imx/imx-ic-prpencvf.c |  8 +--
>>   drivers/staging/media/imx/imx-media-csi.c   | 19 +++---
>>   drivers/staging/media/imx/imx-media-utils.c | 73 ++++++++++-----------
>>   drivers/staging/media/imx/imx-media-vdic.c  |  5 +-
>>   drivers/staging/media/imx/imx-media.h       |  5 +-
>>   drivers/staging/media/imx/imx7-media-csi.c  |  8 +--
>>   7 files changed, 62 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/staging/media/imx/imx-ic-prp.c b/drivers/staging/media/imx/imx-ic-prp.c
>> index 10ffe00f1a54..f87fe0203720 100644
>> --- a/drivers/staging/media/imx/imx-ic-prp.c
>> +++ b/drivers/staging/media/imx/imx-ic-prp.c
>> @@ -193,8 +193,8 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
>>   			sdformat->format.code = cc->codes[0];
>>   		}
>>   
>> -		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
>> -						   true);
>> +		if (sdformat->format.field == V4L2_FIELD_ANY)
>> +			sdformat->format.field = V4L2_FIELD_NONE;
>>   		break;
>>   	case PRP_SRC_PAD_PRPENC:
>>   	case PRP_SRC_PAD_PRPVF:
>> @@ -203,6 +203,8 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
>>   		break;
>>   	}
>>   
>> +	imx_media_try_colorimetry(&sdformat->format, true);
>> +
>>   	fmt = __prp_get_fmt(priv, cfg, sdformat->pad, sdformat->which);
>>   	*fmt = sdformat->format;
>>   out:
>> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
>> index e8b36a181ccc..f2fe3c11c70e 100644
>> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
>> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
>> @@ -907,8 +907,6 @@ static void prp_try_fmt(struct prp_priv *priv,
>>   		/* 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;
>>   	} else {
>>   		v4l_bound_align_image(&sdformat->format.width,
>>   				      MIN_W_SINK, MAX_W_SINK, W_ALIGN_SINK,
>> @@ -916,9 +914,11 @@ static void prp_try_fmt(struct prp_priv *priv,
>>   				      MIN_H_SINK, MAX_H_SINK, H_ALIGN_SINK,
>>   				      S_ALIGN);
>>   
>> -		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
>> -						   true);
>> +		if (sdformat->format.field == V4L2_FIELD_ANY)
>> +			sdformat->format.field = V4L2_FIELD_NONE;
>>   	}
>> +
>> +	imx_media_try_colorimetry(&sdformat->format, true);
>>   }
>>   
>>   static int prp_set_fmt(struct v4l2_subdev *sd,
>> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
>> index 1d248aca40a9..dce4addadff4 100644
>> --- a/drivers/staging/media/imx/imx-media-csi.c
>> +++ b/drivers/staging/media/imx/imx-media-csi.c
>> @@ -1375,9 +1375,15 @@ static void csi_try_field(struct csi_priv *priv,
>>   	struct v4l2_mbus_framefmt *infmt =
>>   		__csi_get_fmt(priv, cfg, CSI_SINK_PAD, sdformat->which);
>>   
>> -	/* no restrictions on sink pad field type */
>> -	if (sdformat->pad == CSI_SINK_PAD)
>> +	/*
>> +	 * no restrictions on sink pad field type except must
>> +	 * be initialized.
>> +	 */
>> +	if (sdformat->pad == CSI_SINK_PAD) {
>> +		if (sdformat->format.field == V4L2_FIELD_ANY)
>> +			sdformat->format.field = V4L2_FIELD_NONE;
>>   		return;
>> +	}
>>   
>>   	switch (infmt->field) {
>>   	case V4L2_FIELD_SEQ_TB:
>> @@ -1455,8 +1461,6 @@ static void csi_try_fmt(struct csi_priv *priv,
>>   		/* 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:
>> @@ -1476,10 +1480,6 @@ static void csi_try_fmt(struct csi_priv *priv,
>>   
>>   		csi_try_field(priv, cfg, sdformat);
>>   
>> -		imx_media_fill_default_mbus_fields(
>> -			&sdformat->format, infmt,
>> -			priv->active_output_pad == CSI_SRC_PAD_DIRECT);
>> -
>>   		/* Reset crop and compose rectangles */
>>   		crop->left = 0;
>>   		crop->top = 0;
>> @@ -1495,6 +1495,9 @@ static void csi_try_fmt(struct csi_priv *priv,
>>   
>>   		break;
>>   	}
>> +
>> +	imx_media_try_colorimetry(&sdformat->format,
>> +			priv->active_output_pad == CSI_SRC_PAD_DIRECT);
>>   }
>>   
>>   static int csi_set_fmt(struct v4l2_subdev *sd,
>> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
>> index b41842dba5ec..05b63395084e 100644
>> --- a/drivers/staging/media/imx/imx-media-utils.c
>> +++ b/drivers/staging/media/imx/imx-media-utils.c
>> @@ -511,21 +511,18 @@ int imx_media_init_cfg(struct v4l2_subdev *sd,
>>   EXPORT_SYMBOL_GPL(imx_media_init_cfg);
>>   
>>   /*
>> - * Check whether the field and colorimetry parameters in tryfmt are
>> - * uninitialized, and if so fill them with the values from fmt,
>> - * or if tryfmt->colorspace has been initialized, all the default
>> - * colorimetry params can be derived from tryfmt->colorspace.
>> + * Default the colorspace in tryfmt to SRGB if set to an unsupported
>> + * colorspace or not initialized. Then set the remaining colorimetry
>> + * parameters based on the colorspace if they are uninitialized.
>>    *
>>    * tryfmt->code must be set on entry.
>>    *
>>    * If this format is destined to be routed through the Image Converter,
>> - * quantization and Y`CbCr encoding must be fixed. The IC expects and
>> - * produces fixed quantization and Y`CbCr encoding at its input and output
>> - * (full range for RGB, limited range for YUV, and V4L2_YCBCR_ENC_601).
>> + * Y`CbCr encoding must be fixed. The IC supports only BT.601 Y`CbCr
>> + * or Rec.709 Y`CbCr encoding.
>>    */
>> -void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
>> -					struct v4l2_mbus_framefmt *fmt,
>> -					bool ic_route)
>> +void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
>> +			       bool ic_route)
>>   {
>>   	const struct imx_media_pixfmt *cc;
>>   	bool is_rgb = false;
>> @@ -533,44 +530,46 @@ void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
>>   	cc = imx_media_find_mbus_format(tryfmt->code, CS_SEL_ANY, true);
>>   	if (!cc)
>>   		cc = imx_media_find_ipu_format(tryfmt->code, CS_SEL_ANY);
>> -	if (cc && cc->cs != IPUV3_COLORSPACE_YUV)
>> +	if (cc && cc->cs == IPUV3_COLORSPACE_RGB)
>>   		is_rgb = true;
>>   
>> -	/* fill field if necessary */
>> -	if (tryfmt->field == V4L2_FIELD_ANY)
>> -		tryfmt->field = fmt->field;
> Have you tested this patch on i.MX7 ?

Nope.

>   It breaks pipeline validation as
> the field field isn't handled anymore in imx7_csi_try_fmt(), due to the
> removal if this code.

Yep, patches crossed paths.
>
> I have a written a fix, I'll prepare a patch.

Please do, thanks.

Steve


>> +	switch (tryfmt->colorspace) {
>> +	case V4L2_COLORSPACE_SMPTE170M:
>> +	case V4L2_COLORSPACE_REC709:
>> +	case V4L2_COLORSPACE_JPEG:
>> +	case V4L2_COLORSPACE_SRGB:
>> +	case V4L2_COLORSPACE_BT2020:
>> +	case V4L2_COLORSPACE_OPRGB:
>> +	case V4L2_COLORSPACE_DCI_P3:
>> +	case V4L2_COLORSPACE_RAW:
>> +		break;
>> +	default:
>> +		tryfmt->colorspace = V4L2_COLORSPACE_SRGB;
>> +		break;
>> +	}
>> +
>> +	if (tryfmt->xfer_func == V4L2_XFER_FUNC_DEFAULT)
>> +		tryfmt->xfer_func =
>> +			V4L2_MAP_XFER_FUNC_DEFAULT(tryfmt->colorspace);
>>   
>> -	/* fill colorimetry if necessary */
>> -	if (tryfmt->colorspace == V4L2_COLORSPACE_DEFAULT) {
>> -		tryfmt->colorspace = fmt->colorspace;
>> -		tryfmt->xfer_func = fmt->xfer_func;
>> -		tryfmt->ycbcr_enc = fmt->ycbcr_enc;
>> -		tryfmt->quantization = fmt->quantization;
>> +	if (ic_route) {
>> +		if (tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_601 &&
>> +		    tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_709)
>> +			tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>>   	} else {
>> -		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);
>> -		}
>>   	}
>>   
>> -	if (ic_route) {
>> -		tryfmt->quantization = is_rgb ?
>> -			V4L2_QUANTIZATION_FULL_RANGE :
>> -			V4L2_QUANTIZATION_LIM_RANGE;
>> -		tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>> -	}
>> +	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_default_mbus_fields);
>> +EXPORT_SYMBOL_GPL(imx_media_try_colorimetry);
>>   
>>   int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>>   				  struct v4l2_rect *compose,
>> diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c
>> index 4487374c9435..fbafd7fb7aeb 100644
>> --- a/drivers/staging/media/imx/imx-media-vdic.c
>> +++ b/drivers/staging/media/imx/imx-media-vdic.c
>> @@ -617,14 +617,13 @@ static void vdic_try_fmt(struct vdic_priv *priv,
>>   				      &sdformat->format.height,
>>   				      MIN_H, MAX_H_VDIC, H_ALIGN, S_ALIGN);
>>   
>> -		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
>> -						   true);
>> -
>>   		/* input must be interlaced! Choose SEQ_TB if not */
>>   		if (!V4L2_FIELD_HAS_BOTH(sdformat->format.field))
>>   			sdformat->format.field = V4L2_FIELD_SEQ_TB;
>>   		break;
>>   	}
>> +
>> +	imx_media_try_colorimetry(&sdformat->format, true);
>>   }
>>   
>>   static int vdic_set_fmt(struct v4l2_subdev *sd,
>> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
>> index 6587aa49e005..23024c9bc887 100644
>> --- a/drivers/staging/media/imx/imx-media.h
>> +++ b/drivers/staging/media/imx/imx-media.h
>> @@ -172,9 +172,8 @@ int imx_media_init_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
>>   			    const struct imx_media_pixfmt **cc);
>>   int imx_media_init_cfg(struct v4l2_subdev *sd,
>>   		       struct v4l2_subdev_pad_config *cfg);
>> -void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
>> -					struct v4l2_mbus_framefmt *fmt,
>> -					bool ic_route);
>> +void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
>> +			       bool ic_route);
>>   int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>>   				  struct v4l2_rect *compose,
>>   				  const struct v4l2_mbus_framefmt *mbus,
>> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
>> index a708a0340eb1..6e2f4c3eb24f 100644
>> --- a/drivers/staging/media/imx/imx7-media-csi.c
>> +++ b/drivers/staging/media/imx/imx7-media-csi.c
>> @@ -1003,8 +1003,6 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>>   
>>   		sdformat->format.colorspace = in_fmt->colorspace;
>>   		sdformat->format.xfer_func = in_fmt->xfer_func;
>> -		sdformat->format.quantization = in_fmt->quantization;
>> -		sdformat->format.ycbcr_enc = in_fmt->ycbcr_enc;
>>   		break;
>>   	case IMX7_CSI_PAD_SINK:
>>   		*cc = imx_media_find_mbus_format(sdformat->format.code,
>> @@ -1015,14 +1013,14 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>>   							 false);
>>   			sdformat->format.code = (*cc)->codes[0];
>>   		}
>> -
>> -		imx_media_fill_default_mbus_fields(&sdformat->format, in_fmt,
>> -						   false);
>>   		break;
>>   	default:
>>   		return -EINVAL;
>>   		break;
>>   	}
>> +
>> +	imx_media_try_colorimetry(&sdformat->format, false);
>> +
>>   	return 0;
>>   }
>>   
>> -- 
>> 2.17.1
>>

Steve Longerbeam Oct. 22, 2019, 3:55 a.m. UTC | #5

On 10/21/19 8:26 PM, Steve Longerbeam wrote:
> Hi Laurent,
>
> On 10/21/19 6:44 PM, Laurent Pinchart wrote:
>> Hi Steve,
>>
>> On Tue, May 21, 2019 at 06:03:17PM -0700, Steve Longerbeam wrote:
>>> Retask imx_media_fill_default_mbus_fields() to try colorimetry 
>>> parameters,
>>> renaming it to to imx_media_try_colorimetry(), and call it at both 
>>> sink and
>>> source pad try_fmt's. The unrelated check for uninitialized field 
>>> value is
>>> moved out to appropriate places in each subdev try_fmt.
>>>
>>> The IC now supports Rec.709 and BT.601 Y'CbCr encoding, and both 
>>> limited
>>> and full range quantization for both YUV and RGB space, so allow those
>>> for pipelines that route through the IC.
>>>
>>> Signed-off-by: Steve Longerbeam <[e-mail]>
>>> ---
>>> Changes in v7:
>>> - squashed with "media: imx: Allow Rec.709 encoding for IC routes".
>>> - remove the RGB full-range quantization restriction for IC routes.
>>> ---
>>>   drivers/staging/media/imx/imx-ic-prp.c      |  6 +-
>>>   drivers/staging/media/imx/imx-ic-prpencvf.c |  8 +--
>>>   drivers/staging/media/imx/imx-media-csi.c   | 19 +++---
>>>   drivers/staging/media/imx/imx-media-utils.c | 73 
>>> ++++++++++-----------
>>>   drivers/staging/media/imx/imx-media-vdic.c  |  5 +-
>>>   drivers/staging/media/imx/imx-media.h       |  5 +-
>>>   drivers/staging/media/imx/imx7-media-csi.c  |  8 +--
>>>   7 files changed, 62 insertions(+), 62 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/imx/imx-ic-prp.c 
>>> b/drivers/staging/media/imx/imx-ic-prp.c
>>> index 10ffe00f1a54..f87fe0203720 100644
>>> --- a/drivers/staging/media/imx/imx-ic-prp.c
>>> +++ b/drivers/staging/media/imx/imx-ic-prp.c
>>> @@ -193,8 +193,8 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
>>>               sdformat->format.code = cc->codes[0];
>>>           }
>>>   - imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
>>> -                           true);
>>> +        if (sdformat->format.field == V4L2_FIELD_ANY)
>>> +            sdformat->format.field = V4L2_FIELD_NONE;
>>>           break;
>>>       case PRP_SRC_PAD_PRPENC:
>>>       case PRP_SRC_PAD_PRPVF:
>>> @@ -203,6 +203,8 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
>>>           break;
>>>       }
>>>   +    imx_media_try_colorimetry(&sdformat->format, true);
>>> +
>>>       fmt = __prp_get_fmt(priv, cfg, sdformat->pad, sdformat->which);
>>>       *fmt = sdformat->format;
>>>   out:
>>> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c 
>>> b/drivers/staging/media/imx/imx-ic-prpencvf.c
>>> index e8b36a181ccc..f2fe3c11c70e 100644
>>> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
>>> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
>>> @@ -907,8 +907,6 @@ static void prp_try_fmt(struct prp_priv *priv,
>>>           /* 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;
>>>       } else {
>>> v4l_bound_align_image(&sdformat->format.width,
>>>                         MIN_W_SINK, MAX_W_SINK, W_ALIGN_SINK,
>>> @@ -916,9 +914,11 @@ static void prp_try_fmt(struct prp_priv *priv,
>>>                         MIN_H_SINK, MAX_H_SINK, H_ALIGN_SINK,
>>>                         S_ALIGN);
>>>   - imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
>>> -                           true);
>>> +        if (sdformat->format.field == V4L2_FIELD_ANY)
>>> +            sdformat->format.field = V4L2_FIELD_NONE;
>>>       }
>>> +
>>> +    imx_media_try_colorimetry(&sdformat->format, true);
>>>   }
>>>     static int prp_set_fmt(struct v4l2_subdev *sd,
>>> diff --git a/drivers/staging/media/imx/imx-media-csi.c 
>>> b/drivers/staging/media/imx/imx-media-csi.c
>>> index 1d248aca40a9..dce4addadff4 100644
>>> --- a/drivers/staging/media/imx/imx-media-csi.c
>>> +++ b/drivers/staging/media/imx/imx-media-csi.c
>>> @@ -1375,9 +1375,15 @@ static void csi_try_field(struct csi_priv *priv,
>>>       struct v4l2_mbus_framefmt *infmt =
>>>           __csi_get_fmt(priv, cfg, CSI_SINK_PAD, sdformat->which);
>>>   -    /* no restrictions on sink pad field type */
>>> -    if (sdformat->pad == CSI_SINK_PAD)
>>> +    /*
>>> +     * no restrictions on sink pad field type except must
>>> +     * be initialized.
>>> +     */
>>> +    if (sdformat->pad == CSI_SINK_PAD) {
>>> +        if (sdformat->format.field == V4L2_FIELD_ANY)
>>> +            sdformat->format.field = V4L2_FIELD_NONE;
>>>           return;
>>> +    }
>>>         switch (infmt->field) {
>>>       case V4L2_FIELD_SEQ_TB:
>>> @@ -1455,8 +1461,6 @@ static void csi_try_fmt(struct csi_priv *priv,
>>>           /* 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:
>>> @@ -1476,10 +1480,6 @@ static void csi_try_fmt(struct csi_priv *priv,
>>>             csi_try_field(priv, cfg, sdformat);
>>>   -        imx_media_fill_default_mbus_fields(
>>> -            &sdformat->format, infmt,
>>> -            priv->active_output_pad == CSI_SRC_PAD_DIRECT);
>>> -
>>>           /* Reset crop and compose rectangles */
>>>           crop->left = 0;
>>>           crop->top = 0;
>>> @@ -1495,6 +1495,9 @@ static void csi_try_fmt(struct csi_priv *priv,
>>>             break;
>>>       }
>>> +
>>> +    imx_media_try_colorimetry(&sdformat->format,
>>> +            priv->active_output_pad == CSI_SRC_PAD_DIRECT);
>>>   }
>>>     static int csi_set_fmt(struct v4l2_subdev *sd,
>>> diff --git a/drivers/staging/media/imx/imx-media-utils.c 
>>> b/drivers/staging/media/imx/imx-media-utils.c
>>> index b41842dba5ec..05b63395084e 100644
>>> --- a/drivers/staging/media/imx/imx-media-utils.c
>>> +++ b/drivers/staging/media/imx/imx-media-utils.c
>>> @@ -511,21 +511,18 @@ int imx_media_init_cfg(struct v4l2_subdev *sd,
>>>   EXPORT_SYMBOL_GPL(imx_media_init_cfg);
>>>     /*
>>> - * Check whether the field and colorimetry parameters in tryfmt are
>>> - * uninitialized, and if so fill them with the values from fmt,
>>> - * or if tryfmt->colorspace has been initialized, all the default
>>> - * colorimetry params can be derived from tryfmt->colorspace.
>>> + * Default the colorspace in tryfmt to SRGB if set to an unsupported
>>> + * colorspace or not initialized. Then set the remaining colorimetry
>>> + * parameters based on the colorspace if they are uninitialized.
>>>    *
>>>    * tryfmt->code must be set on entry.
>>>    *
>>>    * If this format is destined to be routed through the Image 
>>> Converter,
>>> - * quantization and Y`CbCr encoding must be fixed. The IC expects and
>>> - * produces fixed quantization and Y`CbCr encoding at its input and 
>>> output
>>> - * (full range for RGB, limited range for YUV, and 
>>> V4L2_YCBCR_ENC_601).
>>> + * Y`CbCr encoding must be fixed. The IC supports only BT.601 Y`CbCr
>>> + * or Rec.709 Y`CbCr encoding.
>>>    */
>>> -void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt 
>>> *tryfmt,
>>> -                    struct v4l2_mbus_framefmt *fmt,
>>> -                    bool ic_route)
>>> +void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
>>> +                   bool ic_route)
>>>   {
>>>       const struct imx_media_pixfmt *cc;
>>>       bool is_rgb = false;
>>> @@ -533,44 +530,46 @@ void imx_media_fill_default_mbus_fields(struct 
>>> v4l2_mbus_framefmt *tryfmt,
>>>       cc = imx_media_find_mbus_format(tryfmt->code, CS_SEL_ANY, true);
>>>       if (!cc)
>>>           cc = imx_media_find_ipu_format(tryfmt->code, CS_SEL_ANY);
>>> -    if (cc && cc->cs != IPUV3_COLORSPACE_YUV)
>>> +    if (cc && cc->cs == IPUV3_COLORSPACE_RGB)
>>>           is_rgb = true;
>>>   -    /* fill field if necessary */
>>> -    if (tryfmt->field == V4L2_FIELD_ANY)
>>> -        tryfmt->field = fmt->field;
>> Have you tested this patch on i.MX7 ?
>
> Nope.
>
>>   It breaks pipeline validation as
>> the field field isn't handled anymore in imx7_csi_try_fmt(), due to the
>> removal if this code.
>
> Yep, patches crossed paths.

Hmm, actually after closer review it looks like this was missed in this 
patch. So in the IMX7_CSI_PAD_SINK case in imx7_csi_try_fmt(), need to add:

if (sdformat->format.field == V4L2_FIELD_ANY)
         sdformat->format.field = V4L2_FIELD_NONE;

Steve

>
>
>
>>> +    switch (tryfmt->colorspace) {
>>> +    case V4L2_COLORSPACE_SMPTE170M:
>>> +    case V4L2_COLORSPACE_REC709:
>>> +    case V4L2_COLORSPACE_JPEG:
>>> +    case V4L2_COLORSPACE_SRGB:
>>> +    case V4L2_COLORSPACE_BT2020:
>>> +    case V4L2_COLORSPACE_OPRGB:
>>> +    case V4L2_COLORSPACE_DCI_P3:
>>> +    case V4L2_COLORSPACE_RAW:
>>> +        break;
>>> +    default:
>>> +        tryfmt->colorspace = V4L2_COLORSPACE_SRGB;
>>> +        break;
>>> +    }
>>> +
>>> +    if (tryfmt->xfer_func == V4L2_XFER_FUNC_DEFAULT)
>>> +        tryfmt->xfer_func =
>>> + V4L2_MAP_XFER_FUNC_DEFAULT(tryfmt->colorspace);
>>>   -    /* fill colorimetry if necessary */
>>> -    if (tryfmt->colorspace == V4L2_COLORSPACE_DEFAULT) {
>>> -        tryfmt->colorspace = fmt->colorspace;
>>> -        tryfmt->xfer_func = fmt->xfer_func;
>>> -        tryfmt->ycbcr_enc = fmt->ycbcr_enc;
>>> -        tryfmt->quantization = fmt->quantization;
>>> +    if (ic_route) {
>>> +        if (tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_601 &&
>>> +            tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_709)
>>> +            tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>>>       } else {
>>> -        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);
>>> -        }
>>>       }
>>>   -    if (ic_route) {
>>> -        tryfmt->quantization = is_rgb ?
>>> -            V4L2_QUANTIZATION_FULL_RANGE :
>>> -            V4L2_QUANTIZATION_LIM_RANGE;
>>> -        tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>>> -    }
>>> +    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_default_mbus_fields);
>>> +EXPORT_SYMBOL_GPL(imx_media_try_colorimetry);
>>>     int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>>>                     struct v4l2_rect *compose,
>>> diff --git a/drivers/staging/media/imx/imx-media-vdic.c 
>>> b/drivers/staging/media/imx/imx-media-vdic.c
>>> index 4487374c9435..fbafd7fb7aeb 100644
>>> --- a/drivers/staging/media/imx/imx-media-vdic.c
>>> +++ b/drivers/staging/media/imx/imx-media-vdic.c
>>> @@ -617,14 +617,13 @@ static void vdic_try_fmt(struct vdic_priv *priv,
>>>                         &sdformat->format.height,
>>>                         MIN_H, MAX_H_VDIC, H_ALIGN, S_ALIGN);
>>>   - imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
>>> -                           true);
>>> -
>>>           /* input must be interlaced! Choose SEQ_TB if not */
>>>           if (!V4L2_FIELD_HAS_BOTH(sdformat->format.field))
>>>               sdformat->format.field = V4L2_FIELD_SEQ_TB;
>>>           break;
>>>       }
>>> +
>>> +    imx_media_try_colorimetry(&sdformat->format, true);
>>>   }
>>>     static int vdic_set_fmt(struct v4l2_subdev *sd,
>>> diff --git a/drivers/staging/media/imx/imx-media.h 
>>> b/drivers/staging/media/imx/imx-media.h
>>> index 6587aa49e005..23024c9bc887 100644
>>> --- a/drivers/staging/media/imx/imx-media.h
>>> +++ b/drivers/staging/media/imx/imx-media.h
>>> @@ -172,9 +172,8 @@ int imx_media_init_mbus_fmt(struct 
>>> v4l2_mbus_framefmt *mbus,
>>>                   const struct imx_media_pixfmt **cc);
>>>   int imx_media_init_cfg(struct v4l2_subdev *sd,
>>>                  struct v4l2_subdev_pad_config *cfg);
>>> -void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt 
>>> *tryfmt,
>>> -                    struct v4l2_mbus_framefmt *fmt,
>>> -                    bool ic_route);
>>> +void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
>>> +                   bool ic_route);
>>>   int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>>>                     struct v4l2_rect *compose,
>>>                     const struct v4l2_mbus_framefmt *mbus,
>>> diff --git a/drivers/staging/media/imx/imx7-media-csi.c 
>>> b/drivers/staging/media/imx/imx7-media-csi.c
>>> index a708a0340eb1..6e2f4c3eb24f 100644
>>> --- a/drivers/staging/media/imx/imx7-media-csi.c
>>> +++ b/drivers/staging/media/imx/imx7-media-csi.c
>>> @@ -1003,8 +1003,6 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>>>             sdformat->format.colorspace = in_fmt->colorspace;
>>>           sdformat->format.xfer_func = in_fmt->xfer_func;
>>> -        sdformat->format.quantization = in_fmt->quantization;
>>> -        sdformat->format.ycbcr_enc = in_fmt->ycbcr_enc;
>>>           break;
>>>       case IMX7_CSI_PAD_SINK:
>>>           *cc = imx_media_find_mbus_format(sdformat->format.code,
>>> @@ -1015,14 +1013,14 @@ static int imx7_csi_try_fmt(struct imx7_csi 
>>> *csi,
>>>                                false);
>>>               sdformat->format.code = (*cc)->codes[0];
>>>           }
>>> -
>>> - imx_media_fill_default_mbus_fields(&sdformat->format, in_fmt,
>>> -                           false);
>>>           break;
>>>       default:
>>>           return -EINVAL;
>>>           break;
>>>       }
>>> +
>>> +    imx_media_try_colorimetry(&sdformat->format, false);
>>> +
>>>       return 0;
>>>   }
>>>   --
>>> 2.17.1
>>>
>

Rui Miguel Silva Oct. 22, 2019, 1:34 p.m. UTC | #6

Hi Laurent,
On Tue 22 Oct 2019 at 02:44, Laurent Pinchart wrote:
> Hi Steve,
>
> On Tue, May 21, 2019 at 06:03:17PM -0700, Steve Longerbeam wrote:
>> Retask imx_media_fill_default_mbus_fields() to try colorimetry parameters,
>> renaming it to to imx_media_try_colorimetry(), and call it at both sink and
>> source pad try_fmt's. The unrelated check for uninitialized field value is
>> moved out to appropriate places in each subdev try_fmt.
>>
>> The IC now supports Rec.709 and BT.601 Y'CbCr encoding, and both limited
>> and full range quantization for both YUV and RGB space, so allow those
>> for pipelines that route through the IC.
>>
>> Signed-off-by: Steve Longerbeam <[e-mail]>
>> ---
>> Changes in v7:
>> - squashed with "media: imx: Allow Rec.709 encoding for IC routes".
>> - remove the RGB full-range quantization restriction for IC routes.
>> ---
>>  drivers/staging/media/imx/imx-ic-prp.c      |  6 +-
>>  drivers/staging/media/imx/imx-ic-prpencvf.c |  8 +--
>>  drivers/staging/media/imx/imx-media-csi.c   | 19 +++---
>>  drivers/staging/media/imx/imx-media-utils.c | 73 ++++++++++-----------
>>  drivers/staging/media/imx/imx-media-vdic.c  |  5 +-
>>  drivers/staging/media/imx/imx-media.h       |  5 +-
>>  drivers/staging/media/imx/imx7-media-csi.c  |  8 +--
>>  7 files changed, 62 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/staging/media/imx/imx-ic-prp.c b/drivers/staging/media/imx/imx-ic-prp.c
>> index 10ffe00f1a54..f87fe0203720 100644
>> --- a/drivers/staging/media/imx/imx-ic-prp.c
>> +++ b/drivers/staging/media/imx/imx-ic-prp.c
>> @@ -193,8 +193,8 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
>>  			sdformat->format.code = cc->codes[0];
>>  		}
>>
>> -		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
>> -						   true);
>> +		if (sdformat->format.field == V4L2_FIELD_ANY)
>> +			sdformat->format.field = V4L2_FIELD_NONE;
>>  		break;
>>  	case PRP_SRC_PAD_PRPENC:
>>  	case PRP_SRC_PAD_PRPVF:
>> @@ -203,6 +203,8 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
>>  		break;
>>  	}
>>
>> +	imx_media_try_colorimetry(&sdformat->format, true);
>> +
>>  	fmt = __prp_get_fmt(priv, cfg, sdformat->pad, sdformat->which);
>>  	*fmt = sdformat->format;
>>  out:
>> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
>> index e8b36a181ccc..f2fe3c11c70e 100644
>> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
>> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
>> @@ -907,8 +907,6 @@ static void prp_try_fmt(struct prp_priv *priv,
>>  		/* 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;
>>  	} else {
>>  		v4l_bound_align_image(&sdformat->format.width,
>>  				      MIN_W_SINK, MAX_W_SINK, W_ALIGN_SINK,
>> @@ -916,9 +914,11 @@ static void prp_try_fmt(struct prp_priv *priv,
>>  				      MIN_H_SINK, MAX_H_SINK, H_ALIGN_SINK,
>>  				      S_ALIGN);
>>
>> -		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
>> -						   true);
>> +		if (sdformat->format.field == V4L2_FIELD_ANY)
>> +			sdformat->format.field = V4L2_FIELD_NONE;
>>  	}
>> +
>> +	imx_media_try_colorimetry(&sdformat->format, true);
>>  }
>>
>>  static int prp_set_fmt(struct v4l2_subdev *sd,
>> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
>> index 1d248aca40a9..dce4addadff4 100644
>> --- a/drivers/staging/media/imx/imx-media-csi.c
>> +++ b/drivers/staging/media/imx/imx-media-csi.c
>> @@ -1375,9 +1375,15 @@ static void csi_try_field(struct csi_priv *priv,
>>  	struct v4l2_mbus_framefmt *infmt =
>>  		__csi_get_fmt(priv, cfg, CSI_SINK_PAD, sdformat->which);
>>
>> -	/* no restrictions on sink pad field type */
>> -	if (sdformat->pad == CSI_SINK_PAD)
>> +	/*
>> +	 * no restrictions on sink pad field type except must
>> +	 * be initialized.
>> +	 */
>> +	if (sdformat->pad == CSI_SINK_PAD) {
>> +		if (sdformat->format.field == V4L2_FIELD_ANY)
>> +			sdformat->format.field = V4L2_FIELD_NONE;
>>  		return;
>> +	}
>>
>>  	switch (infmt->field) {
>>  	case V4L2_FIELD_SEQ_TB:
>> @@ -1455,8 +1461,6 @@ static void csi_try_fmt(struct csi_priv *priv,
>>  		/* 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:
>> @@ -1476,10 +1480,6 @@ static void csi_try_fmt(struct csi_priv *priv,
>>
>>  		csi_try_field(priv, cfg, sdformat);
>>
>> -		imx_media_fill_default_mbus_fields(
>> -			&sdformat->format, infmt,
>> -			priv->active_output_pad == CSI_SRC_PAD_DIRECT);
>> -
>>  		/* Reset crop and compose rectangles */
>>  		crop->left = 0;
>>  		crop->top = 0;
>> @@ -1495,6 +1495,9 @@ static void csi_try_fmt(struct csi_priv *priv,
>>
>>  		break;
>>  	}
>> +
>> +	imx_media_try_colorimetry(&sdformat->format,
>> +			priv->active_output_pad == CSI_SRC_PAD_DIRECT);
>>  }
>>
>>  static int csi_set_fmt(struct v4l2_subdev *sd,
>> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
>> index b41842dba5ec..05b63395084e 100644
>> --- a/drivers/staging/media/imx/imx-media-utils.c
>> +++ b/drivers/staging/media/imx/imx-media-utils.c
>> @@ -511,21 +511,18 @@ int imx_media_init_cfg(struct v4l2_subdev *sd,
>>  EXPORT_SYMBOL_GPL(imx_media_init_cfg);
>>
>>  /*
>> - * Check whether the field and colorimetry parameters in tryfmt are
>> - * uninitialized, and if so fill them with the values from fmt,
>> - * or if tryfmt->colorspace has been initialized, all the default
>> - * colorimetry params can be derived from tryfmt->colorspace.
>> + * Default the colorspace in tryfmt to SRGB if set to an unsupported
>> + * colorspace or not initialized. Then set the remaining colorimetry
>> + * parameters based on the colorspace if they are uninitialized.
>>   *
>>   * tryfmt->code must be set on entry.
>>   *
>>   * If this format is destined to be routed through the Image Converter,
>> - * quantization and Y`CbCr encoding must be fixed. The IC expects and
>> - * produces fixed quantization and Y`CbCr encoding at its input and output
>> - * (full range for RGB, limited range for YUV, and V4L2_YCBCR_ENC_601).
>> + * Y`CbCr encoding must be fixed. The IC supports only BT.601 Y`CbCr
>> + * or Rec.709 Y`CbCr encoding.
>>   */
>> -void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
>> -					struct v4l2_mbus_framefmt *fmt,
>> -					bool ic_route)
>> +void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
>> +			       bool ic_route)
>>  {
>>  	const struct imx_media_pixfmt *cc;
>>  	bool is_rgb = false;
>> @@ -533,44 +530,46 @@ void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
>>  	cc = imx_media_find_mbus_format(tryfmt->code, CS_SEL_ANY, true);
>>  	if (!cc)
>>  		cc = imx_media_find_ipu_format(tryfmt->code, CS_SEL_ANY);
>> -	if (cc && cc->cs != IPUV3_COLORSPACE_YUV)
>> +	if (cc && cc->cs == IPUV3_COLORSPACE_RGB)
>>  		is_rgb = true;
>>
>> -	/* fill field if necessary */
>> -	if (tryfmt->field == V4L2_FIELD_ANY)
>> -		tryfmt->field = fmt->field;
>
> Have you tested this patch on i.MX7 ? It breaks pipeline validation as
> the field field isn't handled anymore in imx7_csi_try_fmt(), due to the
> removal if this code.
>
> I have a written a fix, I'll prepare a patch.
>

Somehow I missed completely this series. I do not recall it at all.
Thanks in advance for your patch Laurent.

---
Cheers,
	Rui

>
>> +	switch (tryfmt->colorspace) {
>> +	case V4L2_COLORSPACE_SMPTE170M:
>> +	case V4L2_COLORSPACE_REC709:
>> +	case V4L2_COLORSPACE_JPEG:
>> +	case V4L2_COLORSPACE_SRGB:
>> +	case V4L2_COLORSPACE_BT2020:
>> +	case V4L2_COLORSPACE_OPRGB:
>> +	case V4L2_COLORSPACE_DCI_P3:
>> +	case V4L2_COLORSPACE_RAW:
>> +		break;
>> +	default:
>> +		tryfmt->colorspace = V4L2_COLORSPACE_SRGB;
>> +		break;
>> +	}
>> +
>> +	if (tryfmt->xfer_func == V4L2_XFER_FUNC_DEFAULT)
>> +		tryfmt->xfer_func =
>> +			V4L2_MAP_XFER_FUNC_DEFAULT(tryfmt->colorspace);
>>
>> -	/* fill colorimetry if necessary */
>> -	if (tryfmt->colorspace == V4L2_COLORSPACE_DEFAULT) {
>> -		tryfmt->colorspace = fmt->colorspace;
>> -		tryfmt->xfer_func = fmt->xfer_func;
>> -		tryfmt->ycbcr_enc = fmt->ycbcr_enc;
>> -		tryfmt->quantization = fmt->quantization;
>> +	if (ic_route) {
>> +		if (tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_601 &&
>> +		    tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_709)
>> +			tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>>  	} else {
>> -		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);
>> -		}
>>  	}
>>
>> -	if (ic_route) {
>> -		tryfmt->quantization = is_rgb ?
>> -			V4L2_QUANTIZATION_FULL_RANGE :
>> -			V4L2_QUANTIZATION_LIM_RANGE;
>> -		tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>> -	}
>> +	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_default_mbus_fields);
>> +EXPORT_SYMBOL_GPL(imx_media_try_colorimetry);
>>
>>  int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>>  				  struct v4l2_rect *compose,
>> diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c
>> index 4487374c9435..fbafd7fb7aeb 100644
>> --- a/drivers/staging/media/imx/imx-media-vdic.c
>> +++ b/drivers/staging/media/imx/imx-media-vdic.c
>> @@ -617,14 +617,13 @@ static void vdic_try_fmt(struct vdic_priv *priv,
>>  				      &sdformat->format.height,
>>  				      MIN_H, MAX_H_VDIC, H_ALIGN, S_ALIGN);
>>
>> -		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
>> -						   true);
>> -
>>  		/* input must be interlaced! Choose SEQ_TB if not */
>>  		if (!V4L2_FIELD_HAS_BOTH(sdformat->format.field))
>>  			sdformat->format.field = V4L2_FIELD_SEQ_TB;
>>  		break;
>>  	}
>> +
>> +	imx_media_try_colorimetry(&sdformat->format, true);
>>  }
>>
>>  static int vdic_set_fmt(struct v4l2_subdev *sd,
>> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
>> index 6587aa49e005..23024c9bc887 100644
>> --- a/drivers/staging/media/imx/imx-media.h
>> +++ b/drivers/staging/media/imx/imx-media.h
>> @@ -172,9 +172,8 @@ int imx_media_init_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
>>  			    const struct imx_media_pixfmt **cc);
>>  int imx_media_init_cfg(struct v4l2_subdev *sd,
>>  		       struct v4l2_subdev_pad_config *cfg);
>> -void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
>> -					struct v4l2_mbus_framefmt *fmt,
>> -					bool ic_route);
>> +void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
>> +			       bool ic_route);
>>  int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>>  				  struct v4l2_rect *compose,
>>  				  const struct v4l2_mbus_framefmt *mbus,
>> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
>> index a708a0340eb1..6e2f4c3eb24f 100644
>> --- a/drivers/staging/media/imx/imx7-media-csi.c
>> +++ b/drivers/staging/media/imx/imx7-media-csi.c
>> @@ -1003,8 +1003,6 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>>
>>  		sdformat->format.colorspace = in_fmt->colorspace;
>>  		sdformat->format.xfer_func = in_fmt->xfer_func;
>> -		sdformat->format.quantization = in_fmt->quantization;
>> -		sdformat->format.ycbcr_enc = in_fmt->ycbcr_enc;
>>  		break;
>>  	case IMX7_CSI_PAD_SINK:
>>  		*cc = imx_media_find_mbus_format(sdformat->format.code,
>> @@ -1015,14 +1013,14 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>>  							 false);
>>  			sdformat->format.code = (*cc)->codes[0];
>>  		}
>> -
>> -		imx_media_fill_default_mbus_fields(&sdformat->format, in_fmt,
>> -						   false);
>>  		break;
>>  	default:
>>  		return -EINVAL;
>>  		break;
>>  	}
>> +
>> +	imx_media_try_colorimetry(&sdformat->format, false);
>> +
>>  	return 0;
>>  }
>>
>> --
>> 2.17.1
>>

Steve Longerbeam Oct. 22, 2019, 4:26 p.m. UTC | #7

Hi Laurent, Rui,

Besides field type ANY, the imx7 CSI should probably be dealing with 
other field type conversions as well. I may be mistaken, but like the 
imx6, the imx7 does not have the ability to detect whether a captured 
field is a top field or a bottom field, so it can't support ALTERNATE 
mode. It should probably capture two consecutive fields and in that case 
and report seq-tb or seq-bt at its output. Also the imx6 supports 
interlacing field lines, if that is the case for imx7 it should also 
support converting ALTERNATE to INTERLACED at its output.

Steve


On 10/22/19 6:34 AM, Rui Miguel Silva wrote:
> Hi Laurent,
> On Tue 22 Oct 2019 at 02:44, Laurent Pinchart wrote:
>> Hi Steve,
>>
>> On Tue, May 21, 2019 at 06:03:17PM -0700, Steve Longerbeam wrote:
>>> Retask imx_media_fill_default_mbus_fields() to try colorimetry parameters,
>>> renaming it to to imx_media_try_colorimetry(), and call it at both sink and
>>> source pad try_fmt's. The unrelated check for uninitialized field value is
>>> moved out to appropriate places in each subdev try_fmt.
>>>
>>> The IC now supports Rec.709 and BT.601 Y'CbCr encoding, and both limited
>>> and full range quantization for both YUV and RGB space, so allow those
>>> for pipelines that route through the IC.
>>>
>>> Signed-off-by: Steve Longerbeam <[e-mail]>
>>> ---
>>> Changes in v7:
>>> - squashed with "media: imx: Allow Rec.709 encoding for IC routes".
>>> - remove the RGB full-range quantization restriction for IC routes.
>>> ---
>>>   drivers/staging/media/imx/imx-ic-prp.c      |  6 +-
>>>   drivers/staging/media/imx/imx-ic-prpencvf.c |  8 +--
>>>   drivers/staging/media/imx/imx-media-csi.c   | 19 +++---
>>>   drivers/staging/media/imx/imx-media-utils.c | 73 ++++++++++-----------
>>>   drivers/staging/media/imx/imx-media-vdic.c  |  5 +-
>>>   drivers/staging/media/imx/imx-media.h       |  5 +-
>>>   drivers/staging/media/imx/imx7-media-csi.c  |  8 +--
>>>   7 files changed, 62 insertions(+), 62 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/imx/imx-ic-prp.c b/drivers/staging/media/imx/imx-ic-prp.c
>>> index 10ffe00f1a54..f87fe0203720 100644
>>> --- a/drivers/staging/media/imx/imx-ic-prp.c
>>> +++ b/drivers/staging/media/imx/imx-ic-prp.c
>>> @@ -193,8 +193,8 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
>>>   			sdformat->format.code = cc->codes[0];
>>>   		}
>>>
>>> -		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
>>> -						   true);
>>> +		if (sdformat->format.field == V4L2_FIELD_ANY)
>>> +			sdformat->format.field = V4L2_FIELD_NONE;
>>>   		break;
>>>   	case PRP_SRC_PAD_PRPENC:
>>>   	case PRP_SRC_PAD_PRPVF:
>>> @@ -203,6 +203,8 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
>>>   		break;
>>>   	}
>>>
>>> +	imx_media_try_colorimetry(&sdformat->format, true);
>>> +
>>>   	fmt = __prp_get_fmt(priv, cfg, sdformat->pad, sdformat->which);
>>>   	*fmt = sdformat->format;
>>>   out:
>>> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
>>> index e8b36a181ccc..f2fe3c11c70e 100644
>>> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
>>> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
>>> @@ -907,8 +907,6 @@ static void prp_try_fmt(struct prp_priv *priv,
>>>   		/* 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;
>>>   	} else {
>>>   		v4l_bound_align_image(&sdformat->format.width,
>>>   				      MIN_W_SINK, MAX_W_SINK, W_ALIGN_SINK,
>>> @@ -916,9 +914,11 @@ static void prp_try_fmt(struct prp_priv *priv,
>>>   				      MIN_H_SINK, MAX_H_SINK, H_ALIGN_SINK,
>>>   				      S_ALIGN);
>>>
>>> -		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
>>> -						   true);
>>> +		if (sdformat->format.field == V4L2_FIELD_ANY)
>>> +			sdformat->format.field = V4L2_FIELD_NONE;
>>>   	}
>>> +
>>> +	imx_media_try_colorimetry(&sdformat->format, true);
>>>   }
>>>
>>>   static int prp_set_fmt(struct v4l2_subdev *sd,
>>> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
>>> index 1d248aca40a9..dce4addadff4 100644
>>> --- a/drivers/staging/media/imx/imx-media-csi.c
>>> +++ b/drivers/staging/media/imx/imx-media-csi.c
>>> @@ -1375,9 +1375,15 @@ static void csi_try_field(struct csi_priv *priv,
>>>   	struct v4l2_mbus_framefmt *infmt =
>>>   		__csi_get_fmt(priv, cfg, CSI_SINK_PAD, sdformat->which);
>>>
>>> -	/* no restrictions on sink pad field type */
>>> -	if (sdformat->pad == CSI_SINK_PAD)
>>> +	/*
>>> +	 * no restrictions on sink pad field type except must
>>> +	 * be initialized.
>>> +	 */
>>> +	if (sdformat->pad == CSI_SINK_PAD) {
>>> +		if (sdformat->format.field == V4L2_FIELD_ANY)
>>> +			sdformat->format.field = V4L2_FIELD_NONE;
>>>   		return;
>>> +	}
>>>
>>>   	switch (infmt->field) {
>>>   	case V4L2_FIELD_SEQ_TB:
>>> @@ -1455,8 +1461,6 @@ static void csi_try_fmt(struct csi_priv *priv,
>>>   		/* 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:
>>> @@ -1476,10 +1480,6 @@ static void csi_try_fmt(struct csi_priv *priv,
>>>
>>>   		csi_try_field(priv, cfg, sdformat);
>>>
>>> -		imx_media_fill_default_mbus_fields(
>>> -			&sdformat->format, infmt,
>>> -			priv->active_output_pad == CSI_SRC_PAD_DIRECT);
>>> -
>>>   		/* Reset crop and compose rectangles */
>>>   		crop->left = 0;
>>>   		crop->top = 0;
>>> @@ -1495,6 +1495,9 @@ static void csi_try_fmt(struct csi_priv *priv,
>>>
>>>   		break;
>>>   	}
>>> +
>>> +	imx_media_try_colorimetry(&sdformat->format,
>>> +			priv->active_output_pad == CSI_SRC_PAD_DIRECT);
>>>   }
>>>
>>>   static int csi_set_fmt(struct v4l2_subdev *sd,
>>> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
>>> index b41842dba5ec..05b63395084e 100644
>>> --- a/drivers/staging/media/imx/imx-media-utils.c
>>> +++ b/drivers/staging/media/imx/imx-media-utils.c
>>> @@ -511,21 +511,18 @@ int imx_media_init_cfg(struct v4l2_subdev *sd,
>>>   EXPORT_SYMBOL_GPL(imx_media_init_cfg);
>>>
>>>   /*
>>> - * Check whether the field and colorimetry parameters in tryfmt are
>>> - * uninitialized, and if so fill them with the values from fmt,
>>> - * or if tryfmt->colorspace has been initialized, all the default
>>> - * colorimetry params can be derived from tryfmt->colorspace.
>>> + * Default the colorspace in tryfmt to SRGB if set to an unsupported
>>> + * colorspace or not initialized. Then set the remaining colorimetry
>>> + * parameters based on the colorspace if they are uninitialized.
>>>    *
>>>    * tryfmt->code must be set on entry.
>>>    *
>>>    * If this format is destined to be routed through the Image Converter,
>>> - * quantization and Y`CbCr encoding must be fixed. The IC expects and
>>> - * produces fixed quantization and Y`CbCr encoding at its input and output
>>> - * (full range for RGB, limited range for YUV, and V4L2_YCBCR_ENC_601).
>>> + * Y`CbCr encoding must be fixed. The IC supports only BT.601 Y`CbCr
>>> + * or Rec.709 Y`CbCr encoding.
>>>    */
>>> -void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
>>> -					struct v4l2_mbus_framefmt *fmt,
>>> -					bool ic_route)
>>> +void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
>>> +			       bool ic_route)
>>>   {
>>>   	const struct imx_media_pixfmt *cc;
>>>   	bool is_rgb = false;
>>> @@ -533,44 +530,46 @@ void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
>>>   	cc = imx_media_find_mbus_format(tryfmt->code, CS_SEL_ANY, true);
>>>   	if (!cc)
>>>   		cc = imx_media_find_ipu_format(tryfmt->code, CS_SEL_ANY);
>>> -	if (cc && cc->cs != IPUV3_COLORSPACE_YUV)
>>> +	if (cc && cc->cs == IPUV3_COLORSPACE_RGB)
>>>   		is_rgb = true;
>>>
>>> -	/* fill field if necessary */
>>> -	if (tryfmt->field == V4L2_FIELD_ANY)
>>> -		tryfmt->field = fmt->field;
>> Have you tested this patch on i.MX7 ? It breaks pipeline validation as
>> the field field isn't handled anymore in imx7_csi_try_fmt(), due to the
>> removal if this code.
>>
>> I have a written a fix, I'll prepare a patch.
>>
> Somehow I missed completely this series. I do not recall it at all.
> Thanks in advance for your patch Laurent.
>
> ---
> Cheers,
> 	Rui
>
>>> +	switch (tryfmt->colorspace) {
>>> +	case V4L2_COLORSPACE_SMPTE170M:
>>> +	case V4L2_COLORSPACE_REC709:
>>> +	case V4L2_COLORSPACE_JPEG:
>>> +	case V4L2_COLORSPACE_SRGB:
>>> +	case V4L2_COLORSPACE_BT2020:
>>> +	case V4L2_COLORSPACE_OPRGB:
>>> +	case V4L2_COLORSPACE_DCI_P3:
>>> +	case V4L2_COLORSPACE_RAW:
>>> +		break;
>>> +	default:
>>> +		tryfmt->colorspace = V4L2_COLORSPACE_SRGB;
>>> +		break;
>>> +	}
>>> +
>>> +	if (tryfmt->xfer_func == V4L2_XFER_FUNC_DEFAULT)
>>> +		tryfmt->xfer_func =
>>> +			V4L2_MAP_XFER_FUNC_DEFAULT(tryfmt->colorspace);
>>>
>>> -	/* fill colorimetry if necessary */
>>> -	if (tryfmt->colorspace == V4L2_COLORSPACE_DEFAULT) {
>>> -		tryfmt->colorspace = fmt->colorspace;
>>> -		tryfmt->xfer_func = fmt->xfer_func;
>>> -		tryfmt->ycbcr_enc = fmt->ycbcr_enc;
>>> -		tryfmt->quantization = fmt->quantization;
>>> +	if (ic_route) {
>>> +		if (tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_601 &&
>>> +		    tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_709)
>>> +			tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>>>   	} else {
>>> -		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);
>>> -		}
>>>   	}
>>>
>>> -	if (ic_route) {
>>> -		tryfmt->quantization = is_rgb ?
>>> -			V4L2_QUANTIZATION_FULL_RANGE :
>>> -			V4L2_QUANTIZATION_LIM_RANGE;
>>> -		tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>>> -	}
>>> +	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_default_mbus_fields);
>>> +EXPORT_SYMBOL_GPL(imx_media_try_colorimetry);
>>>
>>>   int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>>>   				  struct v4l2_rect *compose,
>>> diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c
>>> index 4487374c9435..fbafd7fb7aeb 100644
>>> --- a/drivers/staging/media/imx/imx-media-vdic.c
>>> +++ b/drivers/staging/media/imx/imx-media-vdic.c
>>> @@ -617,14 +617,13 @@ static void vdic_try_fmt(struct vdic_priv *priv,
>>>   				      &sdformat->format.height,
>>>   				      MIN_H, MAX_H_VDIC, H_ALIGN, S_ALIGN);
>>>
>>> -		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
>>> -						   true);
>>> -
>>>   		/* input must be interlaced! Choose SEQ_TB if not */
>>>   		if (!V4L2_FIELD_HAS_BOTH(sdformat->format.field))
>>>   			sdformat->format.field = V4L2_FIELD_SEQ_TB;
>>>   		break;
>>>   	}
>>> +
>>> +	imx_media_try_colorimetry(&sdformat->format, true);
>>>   }
>>>
>>>   static int vdic_set_fmt(struct v4l2_subdev *sd,
>>> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
>>> index 6587aa49e005..23024c9bc887 100644
>>> --- a/drivers/staging/media/imx/imx-media.h
>>> +++ b/drivers/staging/media/imx/imx-media.h
>>> @@ -172,9 +172,8 @@ int imx_media_init_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
>>>   			    const struct imx_media_pixfmt **cc);
>>>   int imx_media_init_cfg(struct v4l2_subdev *sd,
>>>   		       struct v4l2_subdev_pad_config *cfg);
>>> -void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
>>> -					struct v4l2_mbus_framefmt *fmt,
>>> -					bool ic_route);
>>> +void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
>>> +			       bool ic_route);
>>>   int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>>>   				  struct v4l2_rect *compose,
>>>   				  const struct v4l2_mbus_framefmt *mbus,
>>> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
>>> index a708a0340eb1..6e2f4c3eb24f 100644
>>> --- a/drivers/staging/media/imx/imx7-media-csi.c
>>> +++ b/drivers/staging/media/imx/imx7-media-csi.c
>>> @@ -1003,8 +1003,6 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>>>
>>>   		sdformat->format.colorspace = in_fmt->colorspace;
>>>   		sdformat->format.xfer_func = in_fmt->xfer_func;
>>> -		sdformat->format.quantization = in_fmt->quantization;
>>> -		sdformat->format.ycbcr_enc = in_fmt->ycbcr_enc;
>>>   		break;
>>>   	case IMX7_CSI_PAD_SINK:
>>>   		*cc = imx_media_find_mbus_format(sdformat->format.code,
>>> @@ -1015,14 +1013,14 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>>>   							 false);
>>>   			sdformat->format.code = (*cc)->codes[0];
>>>   		}
>>> -
>>> -		imx_media_fill_default_mbus_fields(&sdformat->format, in_fmt,
>>> -						   false);
>>>   		break;
>>>   	default:
>>>   		return -EINVAL;
>>>   		break;
>>>   	}
>>> +
>>> +	imx_media_try_colorimetry(&sdformat->format, false);
>>> +
>>>   	return 0;
>>>   }
>>>
>>> --
>>> 2.17.1
>>>

Rui Miguel Silva Oct. 23, 2019, 9:07 a.m. UTC | #8

Hi Steve,
On Tue 22 Oct 2019 at 17:26, Steve Longerbeam wrote:
> Hi Laurent, Rui,
>
> Besides field type ANY, the imx7 CSI should probably be dealing with other field
> type conversions as well. I may be mistaken, but like the imx6, the imx7 does
> not have the ability to detect whether a captured field is a top field or a
> bottom field, so it can't support ALTERNATE mode. It should probably capture two
> consecutive fields and in that case and report seq-tb or seq-bt at its output.
> Also the imx6 supports interlacing field lines, if that is the case for imx7 it
> should also support converting ALTERNATE to INTERLACED at its output.
>

Yeah, that makes sense to me, I already saw yours csi_try_field
that does something in this lines.

I will try to handle that in imx7 also.

Thanks for your inputs here.

Cheers,
    Rui
>
> Steve
>
>
> On 10/22/19 6:34 AM, Rui Miguel Silva wrote:
>> Hi Laurent,
>> On Tue 22 Oct 2019 at 02:44, Laurent Pinchart wrote:
>>> Hi Steve,
>>>
>>> On Tue, May 21, 2019 at 06:03:17PM -0700, Steve Longerbeam wrote:
>>>> Retask imx_media_fill_default_mbus_fields() to try colorimetry parameters,
>>>> renaming it to to imx_media_try_colorimetry(), and call it at both sink and
>>>> source pad try_fmt's. The unrelated check for uninitialized field value is
>>>> moved out to appropriate places in each subdev try_fmt.
>>>>
>>>> The IC now supports Rec.709 and BT.601 Y'CbCr encoding, and both limited
>>>> and full range quantization for both YUV and RGB space, so allow those
>>>> for pipelines that route through the IC.
>>>>
>>>> Signed-off-by: Steve Longerbeam <[e-mail]>
>>>> ---
>>>> Changes in v7:
>>>> - squashed with "media: imx: Allow Rec.709 encoding for IC routes".
>>>> - remove the RGB full-range quantization restriction for IC routes.
>>>> ---
>>>>   drivers/staging/media/imx/imx-ic-prp.c      |  6 +-
>>>>   drivers/staging/media/imx/imx-ic-prpencvf.c |  8 +--
>>>>   drivers/staging/media/imx/imx-media-csi.c   | 19 +++---
>>>>   drivers/staging/media/imx/imx-media-utils.c | 73 ++++++++++-----------
>>>>   drivers/staging/media/imx/imx-media-vdic.c  |  5 +-
>>>>   drivers/staging/media/imx/imx-media.h       |  5 +-
>>>>   drivers/staging/media/imx/imx7-media-csi.c  |  8 +--
>>>>   7 files changed, 62 insertions(+), 62 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/media/imx/imx-ic-prp.c b/drivers/staging/media/imx/imx-ic-prp.c
>>>> index 10ffe00f1a54..f87fe0203720 100644
>>>> --- a/drivers/staging/media/imx/imx-ic-prp.c
>>>> +++ b/drivers/staging/media/imx/imx-ic-prp.c
>>>> @@ -193,8 +193,8 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
>>>>   			sdformat->format.code = cc->codes[0];
>>>>   		}
>>>>
>>>> -		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
>>>> -						   true);
>>>> +		if (sdformat->format.field == V4L2_FIELD_ANY)
>>>> +			sdformat->format.field = V4L2_FIELD_NONE;
>>>>   		break;
>>>>   	case PRP_SRC_PAD_PRPENC:
>>>>   	case PRP_SRC_PAD_PRPVF:
>>>> @@ -203,6 +203,8 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
>>>>   		break;
>>>>   	}
>>>>
>>>> +	imx_media_try_colorimetry(&sdformat->format, true);
>>>> +
>>>>   	fmt = __prp_get_fmt(priv, cfg, sdformat->pad, sdformat->which);
>>>>   	*fmt = sdformat->format;
>>>>   out:
>>>> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
>>>> index e8b36a181ccc..f2fe3c11c70e 100644
>>>> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
>>>> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
>>>> @@ -907,8 +907,6 @@ static void prp_try_fmt(struct prp_priv *priv,
>>>>   		/* 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;
>>>>   	} else {
>>>>   		v4l_bound_align_image(&sdformat->format.width,
>>>>   				      MIN_W_SINK, MAX_W_SINK, W_ALIGN_SINK,
>>>> @@ -916,9 +914,11 @@ static void prp_try_fmt(struct prp_priv *priv,
>>>>   				      MIN_H_SINK, MAX_H_SINK, H_ALIGN_SINK,
>>>>   				      S_ALIGN);
>>>>
>>>> -		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
>>>> -						   true);
>>>> +		if (sdformat->format.field == V4L2_FIELD_ANY)
>>>> +			sdformat->format.field = V4L2_FIELD_NONE;
>>>>   	}
>>>> +
>>>> +	imx_media_try_colorimetry(&sdformat->format, true);
>>>>   }
>>>>
>>>>   static int prp_set_fmt(struct v4l2_subdev *sd,
>>>> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
>>>> index 1d248aca40a9..dce4addadff4 100644
>>>> --- a/drivers/staging/media/imx/imx-media-csi.c
>>>> +++ b/drivers/staging/media/imx/imx-media-csi.c
>>>> @@ -1375,9 +1375,15 @@ static void csi_try_field(struct csi_priv *priv,
>>>>   	struct v4l2_mbus_framefmt *infmt =
>>>>   		__csi_get_fmt(priv, cfg, CSI_SINK_PAD, sdformat->which);
>>>>
>>>> -	/* no restrictions on sink pad field type */
>>>> -	if (sdformat->pad == CSI_SINK_PAD)
>>>> +	/*
>>>> +	 * no restrictions on sink pad field type except must
>>>> +	 * be initialized.
>>>> +	 */
>>>> +	if (sdformat->pad == CSI_SINK_PAD) {
>>>> +		if (sdformat->format.field == V4L2_FIELD_ANY)
>>>> +			sdformat->format.field = V4L2_FIELD_NONE;
>>>>   		return;
>>>> +	}
>>>>
>>>>   	switch (infmt->field) {
>>>>   	case V4L2_FIELD_SEQ_TB:
>>>> @@ -1455,8 +1461,6 @@ static void csi_try_fmt(struct csi_priv *priv,
>>>>   		/* 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:
>>>> @@ -1476,10 +1480,6 @@ static void csi_try_fmt(struct csi_priv *priv,
>>>>
>>>>   		csi_try_field(priv, cfg, sdformat);
>>>>
>>>> -		imx_media_fill_default_mbus_fields(
>>>> -			&sdformat->format, infmt,
>>>> -			priv->active_output_pad == CSI_SRC_PAD_DIRECT);
>>>> -
>>>>   		/* Reset crop and compose rectangles */
>>>>   		crop->left = 0;
>>>>   		crop->top = 0;
>>>> @@ -1495,6 +1495,9 @@ static void csi_try_fmt(struct csi_priv *priv,
>>>>
>>>>   		break;
>>>>   	}
>>>> +
>>>> +	imx_media_try_colorimetry(&sdformat->format,
>>>> +			priv->active_output_pad == CSI_SRC_PAD_DIRECT);
>>>>   }
>>>>
>>>>   static int csi_set_fmt(struct v4l2_subdev *sd,
>>>> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
>>>> index b41842dba5ec..05b63395084e 100644
>>>> --- a/drivers/staging/media/imx/imx-media-utils.c
>>>> +++ b/drivers/staging/media/imx/imx-media-utils.c
>>>> @@ -511,21 +511,18 @@ int imx_media_init_cfg(struct v4l2_subdev *sd,
>>>>   EXPORT_SYMBOL_GPL(imx_media_init_cfg);
>>>>
>>>>   /*
>>>> - * Check whether the field and colorimetry parameters in tryfmt are
>>>> - * uninitialized, and if so fill them with the values from fmt,
>>>> - * or if tryfmt->colorspace has been initialized, all the default
>>>> - * colorimetry params can be derived from tryfmt->colorspace.
>>>> + * Default the colorspace in tryfmt to SRGB if set to an unsupported
>>>> + * colorspace or not initialized. Then set the remaining colorimetry
>>>> + * parameters based on the colorspace if they are uninitialized.
>>>>    *
>>>>    * tryfmt->code must be set on entry.
>>>>    *
>>>>    * If this format is destined to be routed through the Image Converter,
>>>> - * quantization and Y`CbCr encoding must be fixed. The IC expects and
>>>> - * produces fixed quantization and Y`CbCr encoding at its input and output
>>>> - * (full range for RGB, limited range for YUV, and V4L2_YCBCR_ENC_601).
>>>> + * Y`CbCr encoding must be fixed. The IC supports only BT.601 Y`CbCr
>>>> + * or Rec.709 Y`CbCr encoding.
>>>>    */
>>>> -void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
>>>> -					struct v4l2_mbus_framefmt *fmt,
>>>> -					bool ic_route)
>>>> +void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
>>>> +			       bool ic_route)
>>>>   {
>>>>   	const struct imx_media_pixfmt *cc;
>>>>   	bool is_rgb = false;
>>>> @@ -533,44 +530,46 @@ void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
>>>>   	cc = imx_media_find_mbus_format(tryfmt->code, CS_SEL_ANY, true);
>>>>   	if (!cc)
>>>>   		cc = imx_media_find_ipu_format(tryfmt->code, CS_SEL_ANY);
>>>> -	if (cc && cc->cs != IPUV3_COLORSPACE_YUV)
>>>> +	if (cc && cc->cs == IPUV3_COLORSPACE_RGB)
>>>>   		is_rgb = true;
>>>>
>>>> -	/* fill field if necessary */
>>>> -	if (tryfmt->field == V4L2_FIELD_ANY)
>>>> -		tryfmt->field = fmt->field;
>>> Have you tested this patch on i.MX7 ? It breaks pipeline validation as
>>> the field field isn't handled anymore in imx7_csi_try_fmt(), due to the
>>> removal if this code.
>>>
>>> I have a written a fix, I'll prepare a patch.
>>>
>> Somehow I missed completely this series. I do not recall it at all.
>> Thanks in advance for your patch Laurent.
>>
>> ---
>> Cheers,
>> 	Rui
>>
>>>> +	switch (tryfmt->colorspace) {
>>>> +	case V4L2_COLORSPACE_SMPTE170M:
>>>> +	case V4L2_COLORSPACE_REC709:
>>>> +	case V4L2_COLORSPACE_JPEG:
>>>> +	case V4L2_COLORSPACE_SRGB:
>>>> +	case V4L2_COLORSPACE_BT2020:
>>>> +	case V4L2_COLORSPACE_OPRGB:
>>>> +	case V4L2_COLORSPACE_DCI_P3:
>>>> +	case V4L2_COLORSPACE_RAW:
>>>> +		break;
>>>> +	default:
>>>> +		tryfmt->colorspace = V4L2_COLORSPACE_SRGB;
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	if (tryfmt->xfer_func == V4L2_XFER_FUNC_DEFAULT)
>>>> +		tryfmt->xfer_func =
>>>> +			V4L2_MAP_XFER_FUNC_DEFAULT(tryfmt->colorspace);
>>>>
>>>> -	/* fill colorimetry if necessary */
>>>> -	if (tryfmt->colorspace == V4L2_COLORSPACE_DEFAULT) {
>>>> -		tryfmt->colorspace = fmt->colorspace;
>>>> -		tryfmt->xfer_func = fmt->xfer_func;
>>>> -		tryfmt->ycbcr_enc = fmt->ycbcr_enc;
>>>> -		tryfmt->quantization = fmt->quantization;
>>>> +	if (ic_route) {
>>>> +		if (tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_601 &&
>>>> +		    tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_709)
>>>> +			tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>>>>   	} else {
>>>> -		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);
>>>> -		}
>>>>   	}
>>>>
>>>> -	if (ic_route) {
>>>> -		tryfmt->quantization = is_rgb ?
>>>> -			V4L2_QUANTIZATION_FULL_RANGE :
>>>> -			V4L2_QUANTIZATION_LIM_RANGE;
>>>> -		tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>>>> -	}
>>>> +	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_default_mbus_fields);
>>>> +EXPORT_SYMBOL_GPL(imx_media_try_colorimetry);
>>>>
>>>>   int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>>>>   				  struct v4l2_rect *compose,
>>>> diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c
>>>> index 4487374c9435..fbafd7fb7aeb 100644
>>>> --- a/drivers/staging/media/imx/imx-media-vdic.c
>>>> +++ b/drivers/staging/media/imx/imx-media-vdic.c
>>>> @@ -617,14 +617,13 @@ static void vdic_try_fmt(struct vdic_priv *priv,
>>>>   				      &sdformat->format.height,
>>>>   				      MIN_H, MAX_H_VDIC, H_ALIGN, S_ALIGN);
>>>>
>>>> -		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
>>>> -						   true);
>>>> -
>>>>   		/* input must be interlaced! Choose SEQ_TB if not */
>>>>   		if (!V4L2_FIELD_HAS_BOTH(sdformat->format.field))
>>>>   			sdformat->format.field = V4L2_FIELD_SEQ_TB;
>>>>   		break;
>>>>   	}
>>>> +
>>>> +	imx_media_try_colorimetry(&sdformat->format, true);
>>>>   }
>>>>
>>>>   static int vdic_set_fmt(struct v4l2_subdev *sd,
>>>> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
>>>> index 6587aa49e005..23024c9bc887 100644
>>>> --- a/drivers/staging/media/imx/imx-media.h
>>>> +++ b/drivers/staging/media/imx/imx-media.h
>>>> @@ -172,9 +172,8 @@ int imx_media_init_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
>>>>   			    const struct imx_media_pixfmt **cc);
>>>>   int imx_media_init_cfg(struct v4l2_subdev *sd,
>>>>   		       struct v4l2_subdev_pad_config *cfg);
>>>> -void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
>>>> -					struct v4l2_mbus_framefmt *fmt,
>>>> -					bool ic_route);
>>>> +void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
>>>> +			       bool ic_route);
>>>>   int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>>>>   				  struct v4l2_rect *compose,
>>>>   				  const struct v4l2_mbus_framefmt *mbus,
>>>> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
>>>> index a708a0340eb1..6e2f4c3eb24f 100644
>>>> --- a/drivers/staging/media/imx/imx7-media-csi.c
>>>> +++ b/drivers/staging/media/imx/imx7-media-csi.c
>>>> @@ -1003,8 +1003,6 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>>>>
>>>>   		sdformat->format.colorspace = in_fmt->colorspace;
>>>>   		sdformat->format.xfer_func = in_fmt->xfer_func;
>>>> -		sdformat->format.quantization = in_fmt->quantization;
>>>> -		sdformat->format.ycbcr_enc = in_fmt->ycbcr_enc;
>>>>   		break;
>>>>   	case IMX7_CSI_PAD_SINK:
>>>>   		*cc = imx_media_find_mbus_format(sdformat->format.code,
>>>> @@ -1015,14 +1013,14 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>>>>   							 false);
>>>>   			sdformat->format.code = (*cc)->codes[0];
>>>>   		}
>>>> -
>>>> -		imx_media_fill_default_mbus_fields(&sdformat->format, in_fmt,
>>>> -						   false);
>>>>   		break;
>>>>   	default:
>>>>   		return -EINVAL;
>>>>   		break;
>>>>   	}
>>>> +
>>>> +	imx_media_try_colorimetry(&sdformat->format, false);
>>>> +
>>>>   	return 0;
>>>>   }
>>>>
>>>> --
>>>> 2.17.1
>>>>

Patch

diff --git a/drivers/staging/media/imx/imx-ic-prp.c b/drivers/staging/media/imx/imx-ic-prp.c
index 10ffe00f1a54..f87fe0203720 100644
--- a/drivers/staging/media/imx/imx-ic-prp.c
+++ b/drivers/staging/media/imx/imx-ic-prp.c
@@ -193,8 +193,8 @@  static int prp_set_fmt(struct v4l2_subdev *sd,
 			sdformat->format.code = cc->codes[0];
 		}
 
-		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
-						   true);
+		if (sdformat->format.field == V4L2_FIELD_ANY)
+			sdformat->format.field = V4L2_FIELD_NONE;
 		break;
 	case PRP_SRC_PAD_PRPENC:
 	case PRP_SRC_PAD_PRPVF:
@@ -203,6 +203,8 @@  static int prp_set_fmt(struct v4l2_subdev *sd,
 		break;
 	}
 
+	imx_media_try_colorimetry(&sdformat->format, true);
+
 	fmt = __prp_get_fmt(priv, cfg, sdformat->pad, sdformat->which);
 	*fmt = sdformat->format;
 out:
diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
index e8b36a181ccc..f2fe3c11c70e 100644
--- a/drivers/staging/media/imx/imx-ic-prpencvf.c
+++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
@@ -907,8 +907,6 @@  static void prp_try_fmt(struct prp_priv *priv,
 		/* 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;
 	} else {
 		v4l_bound_align_image(&sdformat->format.width,
 				      MIN_W_SINK, MAX_W_SINK, W_ALIGN_SINK,
@@ -916,9 +914,11 @@  static void prp_try_fmt(struct prp_priv *priv,
 				      MIN_H_SINK, MAX_H_SINK, H_ALIGN_SINK,
 				      S_ALIGN);
 
-		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
-						   true);
+		if (sdformat->format.field == V4L2_FIELD_ANY)
+			sdformat->format.field = V4L2_FIELD_NONE;
 	}
+
+	imx_media_try_colorimetry(&sdformat->format, true);
 }
 
 static int prp_set_fmt(struct v4l2_subdev *sd,
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 1d248aca40a9..dce4addadff4 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1375,9 +1375,15 @@  static void csi_try_field(struct csi_priv *priv,
 	struct v4l2_mbus_framefmt *infmt =
 		__csi_get_fmt(priv, cfg, CSI_SINK_PAD, sdformat->which);
 
-	/* no restrictions on sink pad field type */
-	if (sdformat->pad == CSI_SINK_PAD)
+	/*
+	 * no restrictions on sink pad field type except must
+	 * be initialized.
+	 */
+	if (sdformat->pad == CSI_SINK_PAD) {
+		if (sdformat->format.field == V4L2_FIELD_ANY)
+			sdformat->format.field = V4L2_FIELD_NONE;
 		return;
+	}
 
 	switch (infmt->field) {
 	case V4L2_FIELD_SEQ_TB:
@@ -1455,8 +1461,6 @@  static void csi_try_fmt(struct csi_priv *priv,
 		/* 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:
@@ -1476,10 +1480,6 @@  static void csi_try_fmt(struct csi_priv *priv,
 
 		csi_try_field(priv, cfg, sdformat);
 
-		imx_media_fill_default_mbus_fields(
-			&sdformat->format, infmt,
-			priv->active_output_pad == CSI_SRC_PAD_DIRECT);
-
 		/* Reset crop and compose rectangles */
 		crop->left = 0;
 		crop->top = 0;
@@ -1495,6 +1495,9 @@  static void csi_try_fmt(struct csi_priv *priv,
 
 		break;
 	}
+
+	imx_media_try_colorimetry(&sdformat->format,
+			priv->active_output_pad == CSI_SRC_PAD_DIRECT);
 }
 
 static int csi_set_fmt(struct v4l2_subdev *sd,
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index b41842dba5ec..05b63395084e 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -511,21 +511,18 @@  int imx_media_init_cfg(struct v4l2_subdev *sd,
 EXPORT_SYMBOL_GPL(imx_media_init_cfg);
 
 /*
- * Check whether the field and colorimetry parameters in tryfmt are
- * uninitialized, and if so fill them with the values from fmt,
- * or if tryfmt->colorspace has been initialized, all the default
- * colorimetry params can be derived from tryfmt->colorspace.
+ * Default the colorspace in tryfmt to SRGB if set to an unsupported
+ * colorspace or not initialized. Then set the remaining colorimetry
+ * parameters based on the colorspace if they are uninitialized.
  *
  * tryfmt->code must be set on entry.
  *
  * If this format is destined to be routed through the Image Converter,
- * quantization and Y`CbCr encoding must be fixed. The IC expects and
- * produces fixed quantization and Y`CbCr encoding at its input and output
- * (full range for RGB, limited range for YUV, and V4L2_YCBCR_ENC_601).
+ * Y`CbCr encoding must be fixed. The IC supports only BT.601 Y`CbCr
+ * or Rec.709 Y`CbCr encoding.
  */
-void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
-					struct v4l2_mbus_framefmt *fmt,
-					bool ic_route)
+void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
+			       bool ic_route)
 {
 	const struct imx_media_pixfmt *cc;
 	bool is_rgb = false;
@@ -533,44 +530,46 @@  void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
 	cc = imx_media_find_mbus_format(tryfmt->code, CS_SEL_ANY, true);
 	if (!cc)
 		cc = imx_media_find_ipu_format(tryfmt->code, CS_SEL_ANY);
-	if (cc && cc->cs != IPUV3_COLORSPACE_YUV)
+	if (cc && cc->cs == IPUV3_COLORSPACE_RGB)
 		is_rgb = true;
 
-	/* fill field if necessary */
-	if (tryfmt->field == V4L2_FIELD_ANY)
-		tryfmt->field = fmt->field;
+	switch (tryfmt->colorspace) {
+	case V4L2_COLORSPACE_SMPTE170M:
+	case V4L2_COLORSPACE_REC709:
+	case V4L2_COLORSPACE_JPEG:
+	case V4L2_COLORSPACE_SRGB:
+	case V4L2_COLORSPACE_BT2020:
+	case V4L2_COLORSPACE_OPRGB:
+	case V4L2_COLORSPACE_DCI_P3:
+	case V4L2_COLORSPACE_RAW:
+		break;
+	default:
+		tryfmt->colorspace = V4L2_COLORSPACE_SRGB;
+		break;
+	}
+
+	if (tryfmt->xfer_func == V4L2_XFER_FUNC_DEFAULT)
+		tryfmt->xfer_func =
+			V4L2_MAP_XFER_FUNC_DEFAULT(tryfmt->colorspace);
 
-	/* fill colorimetry if necessary */
-	if (tryfmt->colorspace == V4L2_COLORSPACE_DEFAULT) {
-		tryfmt->colorspace = fmt->colorspace;
-		tryfmt->xfer_func = fmt->xfer_func;
-		tryfmt->ycbcr_enc = fmt->ycbcr_enc;
-		tryfmt->quantization = fmt->quantization;
+	if (ic_route) {
+		if (tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_601 &&
+		    tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_709)
+			tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
 	} else {
-		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);
-		}
 	}
 
-	if (ic_route) {
-		tryfmt->quantization = is_rgb ?
-			V4L2_QUANTIZATION_FULL_RANGE :
-			V4L2_QUANTIZATION_LIM_RANGE;
-		tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
-	}
+	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_default_mbus_fields);
+EXPORT_SYMBOL_GPL(imx_media_try_colorimetry);
 
 int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
 				  struct v4l2_rect *compose,
diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c
index 4487374c9435..fbafd7fb7aeb 100644
--- a/drivers/staging/media/imx/imx-media-vdic.c
+++ b/drivers/staging/media/imx/imx-media-vdic.c
@@ -617,14 +617,13 @@  static void vdic_try_fmt(struct vdic_priv *priv,
 				      &sdformat->format.height,
 				      MIN_H, MAX_H_VDIC, H_ALIGN, S_ALIGN);
 
-		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
-						   true);
-
 		/* input must be interlaced! Choose SEQ_TB if not */
 		if (!V4L2_FIELD_HAS_BOTH(sdformat->format.field))
 			sdformat->format.field = V4L2_FIELD_SEQ_TB;
 		break;
 	}
+
+	imx_media_try_colorimetry(&sdformat->format, true);
 }
 
 static int vdic_set_fmt(struct v4l2_subdev *sd,
diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
index 6587aa49e005..23024c9bc887 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -172,9 +172,8 @@  int imx_media_init_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
 			    const struct imx_media_pixfmt **cc);
 int imx_media_init_cfg(struct v4l2_subdev *sd,
 		       struct v4l2_subdev_pad_config *cfg);
-void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
-					struct v4l2_mbus_framefmt *fmt,
-					bool ic_route);
+void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
+			       bool ic_route);
 int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
 				  struct v4l2_rect *compose,
 				  const struct v4l2_mbus_framefmt *mbus,
diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index a708a0340eb1..6e2f4c3eb24f 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -1003,8 +1003,6 @@  static int imx7_csi_try_fmt(struct imx7_csi *csi,
 
 		sdformat->format.colorspace = in_fmt->colorspace;
 		sdformat->format.xfer_func = in_fmt->xfer_func;
-		sdformat->format.quantization = in_fmt->quantization;
-		sdformat->format.ycbcr_enc = in_fmt->ycbcr_enc;
 		break;
 	case IMX7_CSI_PAD_SINK:
 		*cc = imx_media_find_mbus_format(sdformat->format.code,
@@ -1015,14 +1013,14 @@  static int imx7_csi_try_fmt(struct imx7_csi *csi,
 							 false);
 			sdformat->format.code = (*cc)->codes[0];
 		}
-
-		imx_media_fill_default_mbus_fields(&sdformat->format, in_fmt,
-						   false);
 		break;
 	default:
 		return -EINVAL;
 		break;
 	}
+
+	imx_media_try_colorimetry(&sdformat->format, false);
+
 	return 0;
 }