Message-ID: <20190930202443.tvu4htu7w5sv33ee@ti.com>
Date:   Mon, 30 Sep 2019 15:24:43 -0500
From:   Benoit Parrot <bparrot@...com>
To:     Hans Verkuil <hverkuil@...all.nl>
CC:     <linux-media@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [Patch 16/16] media: ti-vpe: vpe: don't rely on colorspace
 member for conversion

Hans Verkuil <hverkuil@...all.nl> wrote on Mon [2019-Sep-30 11:05:13 +0200]:
> On 9/27/19 8:36 PM, Benoit Parrot wrote:
> > Up to now VPE was relying on the colorspace value of struct v4l2_format
> > as an indication to perform color space conversion from YUV to RGB or
> > not.
> > 
> > Instead we should used the source/destination fourcc codes as a more
> > reliable indication to perform color space conversion or not.
> > 
> > Signed-off-by: Benoit Parrot <bparrot@...com>
> > ---
> >  drivers/media/platform/ti-vpe/vpe.c | 41 ++++++++++++++++++++++-------
> >  1 file changed, 32 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
> > index e30981cd3e8f..d002adc6263f 100644
> > --- a/drivers/media/platform/ti-vpe/vpe.c
> > +++ b/drivers/media/platform/ti-vpe/vpe.c
> > @@ -353,6 +353,32 @@ enum {
> >  	Q_DATA_DST = 1,
> >  };
> >  
> > +static bool is_fmt_rgb(u32 fourcc)
> > +{
> > +	if (fourcc == V4L2_PIX_FMT_RGB24 ||
> > +	    fourcc == V4L2_PIX_FMT_BGR24 ||
> > +	    fourcc == V4L2_PIX_FMT_RGB32 ||
> > +	    fourcc == V4L2_PIX_FMT_BGR32 ||
> > +	    fourcc == V4L2_PIX_FMT_RGB565 ||
> > +	    fourcc == V4L2_PIX_FMT_RGB555)
> > +		return true;
> > +
> > +	return false;
> > +}
> 
> Why not add a 'bool is_rgb' to struct vpe_fmt?
> 
> It is all too easy to forget to update this function if a new RGB format is
> added to the vpe_formats array in the future.

Yeah I debate going that route also.
I can change it easily enough.

Although, depending on the further changes required below this may be moot.
We'll see.

> 
> > +
> > +/*
> > + * This helper is only used to setup the color space converter
> > + * the actual value returned is only to broadly differentiate between
> > + * RGB and YUV
> > + */
> > +static enum  v4l2_colorspace fourcc_to_colorspace(u32 fourcc)
> 
> double space after enum.

Arrg, I'll fix that.

> 
> > +{
> > +	if (is_fmt_rgb(fourcc))
> > +		return V4L2_COLORSPACE_SRGB;
> > +
> > +	return V4L2_COLORSPACE_SMPTE170M;
> > +}
> 
> This is highly confusing. RGB or YUV conversion does not change the colorspace
> at all.
> 

Well I see here that I am missing some understanding.
But as you saw all I am trying to get to is this: do we need to setup YUV
to RGB conversion or not.

Regarding csc_set_coeff() I was not trying to re-write it but use the
pixel_formats as hint. I understand that this might not be as flexible as
it needs to be (meaning handling colorspace, xfer_func, ycbr_encoding or
quantization in a sane way) but to at least get the correct direction of the
conversion. At the moment VPE driver only handles YUV to RGB conversion.


> There are four colorimetry related fields: colorspace, xfer_func, ycbcr_enc and
> quantization. Most hardware (including this one AFAICT) have a 3x3 matrix + a
> vector to do the conversion. This only allows you to convert between different
> ycbcr encodings and quantization ranges. The colorspace and xfer_func parameters
> stay unchanged (you need gamma tables and two other 3x3 matrices for that).
> 
> So if you want to set up the 3x3 matrix + vector correctly, then you need to
> provide the ycbcr_enc value + quantization value of the source to your function.
> ycbcr_enc is only valid of YUV pixelformats, of course, and you can use
> V4L2_MAP_COLORSPACE_DEFAULT, V4L2_MAP_YCBCR_ENC_DEFAULT and V4L2_MAP_QUANTIZATION_DEFAULT
> if one or more of these values as set by userspace are 0.
> 
> Since the V4L2 API does not (yet) support setting ycbcr_enc and quantization for the
> capture queue you have to provide that information yourself:
> 
> For RGB ycbcr_enc is of course ignored, and quantization should be full range.
> For YUV it depends: the quantization is always limited range, but for the ycbcr_enc
> you have a choice: if the source was YUV, then you can decide to just copy the
> ycbcr_enc value. Alternatively, you can convert to 601 for SDTV and 709 for anything
> else. The latter is also the recommended choice if the source was RGB.
> 
> In any case, please do not use enum v4l2_colorspace in the csc_set_coeff function:
> it's misleading.

But I guess I can rewrite the csc_set_coeff() module API to use at least
pixel_format instead of enum_colorspace at least as a first step.

Full color space parameters support (meaning all 4 of them) would probably
be better done as its own future patch series.

Benoit

> 
> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/colorspaces.html and the following two
> sections contain a lot of information about how colorspaces work.
> 
> Regards,
> 
> 	Hans
> 
> > +
> >  /* find our format description corresponding to the passed v4l2_format */
> >  static struct vpe_fmt *__find_format(u32 fourcc)
> >  {
> > @@ -764,11 +790,10 @@ static void set_src_registers(struct vpe_ctx *ctx)
> >  static void set_dst_registers(struct vpe_ctx *ctx)
> >  {
> >  	struct vpe_mmr_adb *mmr_adb = ctx->mmr_adb.addr;
> > -	enum v4l2_colorspace clrspc = ctx->q_data[Q_DATA_DST].colorspace;
> >  	struct vpe_fmt *fmt = ctx->q_data[Q_DATA_DST].fmt;
> >  	u32 val = 0;
> >  
> > -	if (clrspc == V4L2_COLORSPACE_SRGB) {
> > +	if (is_fmt_rgb(fmt->fourcc)) {
> >  		val |= VPE_RGB_OUT_SELECT;
> >  		vpdma_set_bg_color(ctx->dev->vpdma,
> >  			(struct vpdma_data_format *)fmt->vpdma_fmt[0], 0xff);
> > @@ -912,7 +937,8 @@ static int set_srcdst_params(struct vpe_ctx *ctx)
> >  	set_dei_regs(ctx);
> >  
> >  	csc_set_coeff(ctx->dev->csc, &mmr_adb->csc_regs[0],
> > -		s_q_data->colorspace, d_q_data->colorspace);
> > +		      fourcc_to_colorspace(s_q_data->fmt->fourcc),
> > +		      fourcc_to_colorspace(d_q_data->fmt->fourcc));
> >  
> >  	sc_set_hs_coeffs(ctx->dev->sc, ctx->sc_coeff_h.addr, src_w, dst_w);
> >  	sc_set_vs_coeffs(ctx->dev->sc, ctx->sc_coeff_v.addr, src_h, dst_h);
> > @@ -1285,7 +1311,7 @@ static void device_run(void *priv)
> >  	if (ctx->deinterlacing)
> >  		add_out_dtd(ctx, VPE_PORT_MV_OUT);
> >  
> > -	if (d_q_data->colorspace == V4L2_COLORSPACE_SRGB) {
> > +	if (is_fmt_rgb(d_q_data->fmt->fourcc)) {
> >  		add_out_dtd(ctx, VPE_PORT_RGB_OUT);
> >  	} else {
> >  		add_out_dtd(ctx, VPE_PORT_LUMA_OUT);
> > @@ -1327,7 +1353,7 @@ static void device_run(void *priv)
> >  	}
> >  
> >  	/* sync on channel control descriptors for output ports */
> > -	if (d_q_data->colorspace == V4L2_COLORSPACE_SRGB) {
> > +	if (is_fmt_rgb(d_q_data->fmt->fourcc)) {
> >  		vpdma_add_sync_on_channel_ctd(&ctx->desc_list,
> >  			VPE_CHAN_RGB_OUT);
> >  	} else {
> > @@ -1682,10 +1708,7 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
> >  		height = pix->height;
> >  
> >  	if (!pix->colorspace) {
> > -		if (fmt->fourcc == V4L2_PIX_FMT_RGB24 ||
> > -				fmt->fourcc == V4L2_PIX_FMT_BGR24 ||
> > -				fmt->fourcc == V4L2_PIX_FMT_RGB32 ||
> > -				fmt->fourcc == V4L2_PIX_FMT_BGR32) {
> > +		if (is_fmt_rgb(fmt->fourcc)) {
> >  			pix->colorspace = V4L2_COLORSPACE_SRGB;
> >  		} else {
> >  			if (height > 1280)	/* HD */
> > 
> 

Powered by blists - more mailing lists