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

From: Rui Miguel Silva
Date: Wed Oct 23 2019 - 05:07:13 EST

Next message: Peter Zijlstra: "Re: [PATCH v4 15/16] module: Move where we mark modules RO,X"
Previous message: Rafael J. Wysocki: "Re: [PATCH v9 1/3] PM: wakeup: Add routine to help fetch wakeup source object."
In reply to: Steve Longerbeam: "Re: [PATCH v8 5/5] media: imx: Try colorimetry at both sink and source pads"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 <slongerbeam@xxxxxxxxx>
>>>> ---
>>>> 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
>>>>

Next message: Peter Zijlstra: "Re: [PATCH v4 15/16] module: Move where we mark modules RO,X"
Previous message: Rafael J. Wysocki: "Re: [PATCH v9 1/3] PM: wakeup: Add routine to help fetch wakeup source object."
In reply to: Steve Longerbeam: "Re: [PATCH v8 5/5] media: imx: Try colorimetry at both sink and source pads"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]