Re: [PATCH v10 18/30] rcar-vin: add check for colorspace

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

Hi Niklas,

Thank you for the patch.

On Monday, 29 January 2018 18:34:23 EET Niklas Söderlund wrote:
> Add a check to ensure the colorspace from user-space is good. On Gen2 it
> works without this change as the sensor sets the colorspace but on Gen3
> this can fail if the colorspace provided by the user is not good. The
> values to check for comes from v4l2-compliance sources which is the tool
> that found this error. If this check is not preformed v4l2-compliance

s/preformed/performed/

> fails when it tests colorspace.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> 841d62ca27e026d7..6403650aff22a2ed 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -23,6 +23,7 @@
>  #include "rcar-vin.h"
> 
>  #define RVIN_DEFAULT_FORMAT	V4L2_PIX_FMT_YUYV
> +#define RVIN_DEFAULT_COLORSPACE	V4L2_COLORSPACE_SRGB
> 
>  /* ------------------------------------------------------------------------
>   * Format Conversions
> @@ -115,6 +116,10 @@ static int rvin_format_align(struct rvin_dev *vin,
> struct v4l2_pix_format *pix) break;
>  	}
> 
> +	/* Check that colorspace is reasonable */
> +	if (!pix->colorspace || pix->colorspace >= 0xff)

I'd write the first check as

	pix->colorspace == V4L2_COLORSPACE_DEFAULT

For the second check I don't think 0xff is a meaningful value. We currently 
have 12 colorspaces defined. If we want to be future-proof I'd add a 
V4L2_COLORSPACE_MAX entry to enum v4l2_colorspace and use that for the check. 
Alternatively you could use V4L2_COLORSPACE_DCI_P3 but the driver would need 
to be updated when new colorspaces get added.

> +		pix->colorspace = RVIN_DEFAULT_COLORSPACE;
> +
>  	/* HW limit width to a multiple of 32 (2^5) for NV16 else 2 (2^1) */
>  	walign = vin->format.pixelformat == V4L2_PIX_FMT_NV16 ? 5 : 1;

-- 
Regards,

Laurent Pinchart