On 04/13/2017 03:09 AM, Philipp Zabel wrote:
Hi Steve,
thanks for cleaning this up. Apart from the comments below, this looks
good to me.
On Wed, 2017-04-12 at 17:45 -0700, Steve Longerbeam wrote:
This patch adds a call to imx_media_fill_empty_mbus_fields() in the
*_try_fmt() functions at the sink pads, to set empty field order and
colorimetry parameters.
If the field order is set to ANY, choose the currently set field order
at the sink pad. If the colorspace is set to DEFAULT, choose the
current colorspace at the sink pad. If any of xfer_func, ycbcr_enc
or quantization are set to DEFAULT, either choose the current sink pad
setting, or the default setting for the new colorspace, if non-DEFAULT
colorspace was given.
Colorimetry is also propagated from sink to source pads anywhere
this has not already been done. The exception is ic-prpencvf at the
source pad, since the Image Converter outputs fixed quantization and
Y`CbCr encoding.
Signed-off-by: Steve Longerbeam <steve_longerbeam@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?
sure, will do.
switch (sdformat->pad) {
case PRP_SINK_PAD:
v4l_bound_align_image(&sdformat->format.width, MIN_W, MAX_W,
@@ -193,11 +195,12 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
cc = imx_media_find_ipu_format(code, CS_SEL_ANY);
sdformat->format.code = cc->codes[0];
}
+
+ imx_media_fill_empty_mbus_fields(&sdformat->format, fmt);
break;
case PRP_SRC_PAD_PRPENC:
case PRP_SRC_PAD_PRPVF:
/* Output pads mirror input pad */
- fmt = __prp_get_fmt(priv, cfg, PRP_SINK_PAD, sdformat->which);
sdformat->format = *fmt;
break;
}
diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
index 860b406..cdfb51a 100644
--- a/drivers/staging/media/imx/imx-ic-prpencvf.c
+++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
@@ -772,6 +772,8 @@ static void prp_try_fmt(struct prp_priv *priv,
struct v4l2_subdev_format *sdformat,
const struct imx_media_pixfmt **cc)
{
+ struct v4l2_mbus_framefmt *infmt;
+
*cc = imx_media_find_ipu_format(sdformat->format.code, CS_SEL_ANY);
if (!*cc) {
u32 code;
@@ -781,11 +783,9 @@ static void prp_try_fmt(struct prp_priv *priv,
sdformat->format.code = (*cc)->codes[0];
}
- if (sdformat->pad == PRPENCVF_SRC_PAD) {
- struct v4l2_mbus_framefmt *infmt =
- __prp_get_fmt(priv, cfg, PRPENCVF_SINK_PAD,
- sdformat->which);
+ infmt = __prp_get_fmt(priv, cfg, PRPENCVF_SINK_PAD, sdformat->which);
+ if (sdformat->pad == PRPENCVF_SRC_PAD) {
if (sdformat->format.field != V4L2_FIELD_NONE)
sdformat->format.field = infmt->field;
@@ -804,12 +804,29 @@ static void prp_try_fmt(struct prp_priv *priv,
&sdformat->format.height,
infmt->height / 4, MAX_H_SRC,
H_ALIGN_SRC, S_ALIGN);
+
+ /*
+ * The Image Converter produces fixed quantization
+ * (full range for RGB, limited range for YUV), and
+ * uses a fixed Y`CbCr encoding (V4L2_YCBCR_ENC_601).
+ * For colorspace and transfer func, just propagate
+ * from the sink.
+ */
+ sdformat->format.quantization =
+ ((*cc)->cs != IPUV3_COLORSPACE_YUV) ?
+ V4L2_QUANTIZATION_FULL_RANGE :
+ V4L2_QUANTIZATION_LIM_RANGE;
+ sdformat->format.ycbcr_enc = V4L2_YCBCR_ENC_601;
Support for V4L2_YCBCR_ENC_709 and quantization options could be added
to the IPUv3 core code, so this limitation could be relaxed later.
Yes, I was going to mention that too. We can add coefficient tables
to ipu-ic for all the encodings enumerated in enum v4l2_ycbcr_encoding.
I know that quantization is programmable in the DP, but is it in the
IC? AFAICT there is none.
+ sdformat->format.colorspace = infmt->colorspace;
+ sdformat->format.xfer_func = infmt->xfer_func;
} else {
v4l_bound_align_image(&sdformat->format.width,
MIN_W_SINK, MAX_W_SINK, W_ALIGN_SINK,
&sdformat->format.height,
MIN_H_SINK, MAX_H_SINK, H_ALIGN_SINK,
S_ALIGN);
+
+ imx_media_fill_empty_mbus_fields(&sdformat->format, infmt);
So we should limit quantization and ycbcr_enc here as well. Right now
there is no way to convert from limited range RGB to full range RGB, for
example.
True, yes, I was scratching my head about that. I'll move the above to
apply to all pads.
}
}
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index b11e80f..730d1aa 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1220,11 +1220,11 @@ static void csi_try_fmt(struct csi_priv *priv,
struct v4l2_mbus_framefmt *infmt;
u32 code;
+ infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD, sdformat->which);
+
switch (sdformat->pad) {
case CSI_SRC_PAD_DIRECT:
case CSI_SRC_PAD_IDMAC:
- infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD,
- sdformat->which);
incc = imx_media_find_mbus_format(infmt->code,
CS_SEL_ANY, true);
@@ -1260,6 +1260,12 @@ static void csi_try_fmt(struct csi_priv *priv,
sdformat->format.field = (infmt->height == 480) ?
V4L2_FIELD_SEQ_TB : V4L2_FIELD_SEQ_BT;
}
+
+ /* propagate colorimetry from sink */
+ sdformat->format.colorspace = infmt->colorspace;
+ sdformat->format.xfer_func = infmt->xfer_func;
+ sdformat->format.quantization = infmt->quantization;
+ sdformat->format.ycbcr_enc = infmt->ycbcr_enc;
break;
case CSI_SINK_PAD:
v4l_bound_align_image(&sdformat->format.width, MIN_W, MAX_W,
@@ -1286,6 +1292,8 @@ static void csi_try_fmt(struct csi_priv *priv,
CS_SEL_ANY, false);
sdformat->format.code = (*cc)->codes[0];
}
+
+ imx_media_fill_empty_mbus_fields(&sdformat->format, infmt);
break;
}
}
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 7b2f92d..b07d0ae 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -464,6 +464,59 @@ int imx_media_init_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
}
EXPORT_SYMBOL_GPL(imx_media_init_mbus_fmt);
+/*
+ * Check whether the field or colorimetry params in tryfmt are
+ * uninitialized, and if so fill them with the values from fmt.
+ * The exception is when tryfmt->colorspace has been initialized,
+ * if so all the further default colorimetry params can be derived
+ * from tryfmt->colorspace.
+ */
+void imx_media_fill_empty_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
+ struct v4l2_mbus_framefmt *fmt)
+{
+ /* fill field if necessary */
+ if (tryfmt->field == V4L2_FIELD_ANY)
+ tryfmt->field = fmt->field;
+
+ /* fill colorimetry if necessary */
+ if (tryfmt->colorspace == V4L2_COLORSPACE_DEFAULT) {
+ tryfmt->colorspace = fmt->colorspace;
+ if (tryfmt->xfer_func == V4L2_XFER_FUNC_DEFAULT)
+ tryfmt->xfer_func = fmt->xfer_func;
+ if (tryfmt->ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT)
+ tryfmt->ycbcr_enc = fmt->ycbcr_enc;
+ if (tryfmt->quantization == V4L2_QUANTIZATION_DEFAULT)
+ tryfmt->quantization = fmt->quantization;
+ } else {
+ const struct imx_media_pixfmt *cc;
+ bool is_rgb = false;
+
+ cc = imx_media_find_mbus_format(tryfmt->code,
+ CS_SEL_ANY, false);
+ if (!cc)
+ cc = imx_media_find_ipu_format(tryfmt->code,
+ CS_SEL_ANY);
+ if (cc && cc->cs != IPUV3_COLORSPACE_YUV)
+ is_rgb = true;
+
+ if (tryfmt->xfer_func == V4L2_XFER_FUNC_DEFAULT) {
+ tryfmt->xfer_func =
+ V4L2_MAP_XFER_FUNC_DEFAULT(tryfmt->colorspace);
+ }
+ if (tryfmt->ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) {
+ tryfmt->ycbcr_enc =
+ V4L2_MAP_YCBCR_ENC_DEFAULT(tryfmt->colorspace);
+ }
+ if (tryfmt->quantization == V4L2_QUANTIZATION_DEFAULT) {
+ tryfmt->quantization =
+ V4L2_MAP_QUANTIZATION_DEFAULT(
+ is_rgb, tryfmt->colorspace,
+ tryfmt->ycbcr_enc);
+ }
+ }
+}
+EXPORT_SYMBOL_GPL(imx_media_fill_empty_mbus_fields);
+
I'd s/empty/default/ in the function name.
sounds good.
After we agree on and write down some guidelines on expected behaviour
in the spec, this could be lifted into v4l2-core.
Agreed.
Steve
