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@xxxxxxxxxx>
> ---
> 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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
