| Message ID | 20190927183650.31345-17-bparrot@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: vpe: maintenance | expand |
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@ti.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. > + > +/* > + * 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. > +{ > + 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. 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. 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 */ >
Hans Verkuil <hverkuil@xs4all.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@ti.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 */ > > >
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; +} + +/* + * 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) +{ + if (is_fmt_rgb(fourcc)) + return V4L2_COLORSPACE_SRGB; + + return V4L2_COLORSPACE_SMPTE170M; +} + /* 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 */
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@ti.com> --- drivers/media/platform/ti-vpe/vpe.c | 41 ++++++++++++++++++++++------- 1 file changed, 32 insertions(+), 9 deletions(-)