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

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Steve,

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

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

Should this be changed to infmt for consistency?

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

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

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

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

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

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

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

regards
Philipp