Re: [PATCH 22/28] media: ti-vpe: cal: set field always to V4L2_FIELD_NONE

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Tomi,

Thank you for the patch.

On Mon, Apr 12, 2021 at 02:34:51PM +0300, Tomi Valkeinen wrote:
> cal_camerarx_sd_set_fmt() accepts any value for the format field, but
> there should be no reason to have any other value accepted than
> V4L2_FIELD_NONE. So set the field always to V4L2_FIELD_NONE.

What if the source produces V4L2_FIELD_INTERLACED_(TB|BT) ? Shouldn't we
accept that and propagate it ? Same for V4L2_FIELD_TOP and
V4L2_FIELD_BOTTOM I suppose. V4L2_FIELD_SEQ_(TB|BT) is likely not needed
as I doubt sources will send that. V4L2_FIELD_ALTERNATE is a bit more
tricky, as the driver has to report which field a particular buffer
contains, and I'm not sure we could do so (maybe based on the CSI-2
frame number ?).

I'm fine dropping support for interlaced formats until someone wants to
support them though, it's up to you.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/platform/ti-vpe/cal-camerarx.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c
> index 245c601b992c..880261d53a1d 100644
> --- a/drivers/media/platform/ti-vpe/cal-camerarx.c
> +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c
> @@ -702,10 +702,7 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
>  	if (!fmtinfo)
>  		fmtinfo = &cal_formats[0];
>  
> -	/*
> -	 * Clamp the size, update the code. The field and colorspace are
> -	 * accepted as-is.
> -	 */
> +	/* Clamp the size, update the code. The colorspace is accepted as-is. */
>  	bpp = ALIGN(fmtinfo->bpp, 8);
>  
>  	format->format.width = clamp_t(unsigned int, format->format.width,
> @@ -715,6 +712,7 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
>  					CAL_MIN_HEIGHT_LINES,
>  					CAL_MAX_HEIGHT_LINES);
>  	format->format.code = fmtinfo->code;
> +	format->format.field = V4L2_FIELD_NONE;
>  
>  	/* Store the format and propagate it to the source pad. */
>  	fmt = cal_camerarx_get_pad_format(phy, cfg, CAL_CAMERARX_PAD_SINK,

-- 
Regards,

Laurent Pinchart