* [PATCH v6 3/7] gpu: ipu-v3: ipu-ic: Fully describe colorspace conversions [not found] <20190307233356.23748-1-slongerbeam@gmail.com> @ 2019-03-07 23:33 ` Steve Longerbeam 2019-03-08 11:46 ` Philipp Zabel 0 siblings, 1 reply; 3+ messages in thread From: Steve Longerbeam @ 2019-03-07 23:33 UTC (permalink / raw) To: linux-media Cc: Tim Harvey, Steve Longerbeam, Philipp Zabel, Mauro Carvalho Chehab, Greg Kroah-Hartman, Bartlomiej Zolnierkiewicz, open list:DRM DRIVERS FOR FREESCALE IMX, open list, open list:STAGING SUBSYSTEM, open list:FRAMEBUFFER LAYER Only providing the input and output RGB/YUV space to the IC task init functions is not sufficient. To fully characterize a colorspace conversion, the colorspace (chromaticities), Y'CbCr encoding standard, and quantization also need to be specified. Define a 'struct ipu_ic_colorspace' that includes all the above, and pass the input and output ipu_ic_colorspace to the IC task init functions. This allows to actually enforce the fact that the IC: - can only encode to/from YUV full range (follow-up patch will remove this restriction). - can only encode to/from RGB full range. - can only encode using BT.601 standard (follow-up patch will add Rec.709 encoding support). - cannot convert colorspaces from input to output, the input and output colorspace chromaticities must be the same. The determination of the CSC coefficients based on the input/output colorspace parameters are moved to a new function calc_csc_coeffs(), called by init_csc(). Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> --- drivers/gpu/ipu-v3/ipu-ic.c | 136 +++++++++++++------- drivers/gpu/ipu-v3/ipu-image-convert.c | 27 ++-- drivers/staging/media/imx/imx-ic-prpencvf.c | 22 +++- include/video/imx-ipu-v3.h | 37 +++++- 4 files changed, 154 insertions(+), 68 deletions(-) diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c index b63a2826b629..c4048c921801 100644 --- a/drivers/gpu/ipu-v3/ipu-ic.c +++ b/drivers/gpu/ipu-v3/ipu-ic.c @@ -146,8 +146,10 @@ struct ipu_ic { const struct ic_task_regoffs *reg; const struct ic_task_bitfields *bit; - enum ipu_color_space in_cs, g_in_cs; - enum ipu_color_space out_cs; + struct ipu_ic_colorspace in_cs; + struct ipu_ic_colorspace g_in_cs; + struct ipu_ic_colorspace out_cs; + bool graphics; bool rotation; bool in_use; @@ -235,42 +237,83 @@ static const struct ic_encode_coeff ic_encode_ycbcr2rgb_601 = { .scale = 2, }; +static int calc_csc_coeffs(struct ipu_ic_priv *priv, + struct ic_encode_coeff *coeff_out, + const struct ipu_ic_colorspace *in, + const struct ipu_ic_colorspace *out) +{ + bool inverse_encode; + + if (in->colorspace != out->colorspace) { + dev_err(priv->ipu->dev, "Cannot convert colorspaces\n"); + return -ENOTSUPP; + } + + if (out->enc != V4L2_YCBCR_ENC_601) { + dev_err(priv->ipu->dev, "Only BT.601 encoding supported\n"); + return -ENOTSUPP; + } + + if ((in->cs = IPUV3_COLORSPACE_YUV && + in->quant != V4L2_QUANTIZATION_FULL_RANGE) || + (out->cs = IPUV3_COLORSPACE_YUV && + out->quant != V4L2_QUANTIZATION_FULL_RANGE)) { + dev_err(priv->ipu->dev, "Limited range YUV not supported\n"); + return -ENOTSUPP; + } + + if ((in->cs = IPUV3_COLORSPACE_RGB && + in->quant != V4L2_QUANTIZATION_FULL_RANGE) || + (out->cs = IPUV3_COLORSPACE_RGB && + out->quant != V4L2_QUANTIZATION_FULL_RANGE)) { + dev_err(priv->ipu->dev, "Limited range RGB not supported\n"); + return -ENOTSUPP; + } + + if (in->cs = out->cs) { + *coeff_out = ic_encode_identity; + + return 0; + } + + inverse_encode = (in->cs = IPUV3_COLORSPACE_YUV); + + *coeff_out = inverse_encode ? + ic_encode_ycbcr2rgb_601 : ic_encode_rgb2ycbcr_601; + + return 0; +} + static int init_csc(struct ipu_ic *ic, - enum ipu_color_space inf, - enum ipu_color_space outf, + const struct ipu_ic_colorspace *in, + const struct ipu_ic_colorspace *out, int csc_index) { struct ipu_ic_priv *priv = ic->priv; - const struct ic_encode_coeff *coeff; + struct ic_encode_coeff coeff; u32 __iomem *base; const u16 (*c)[3]; const u16 *a; u32 param; + int ret; + + ret = calc_csc_coeffs(priv, &coeff, in, out); + if (ret) + return ret; base = (u32 __iomem *) (priv->tpmem_base + ic->reg->tpmem_csc[csc_index]); - if (inf = IPUV3_COLORSPACE_YUV && outf = IPUV3_COLORSPACE_RGB) - coeff = &ic_encode_ycbcr2rgb_601; - else if (inf = IPUV3_COLORSPACE_RGB && outf = IPUV3_COLORSPACE_YUV) - coeff = &ic_encode_rgb2ycbcr_601; - else if (inf = IPUV3_COLORSPACE_RGB && outf = IPUV3_COLORSPACE_RGB) - coeff = &ic_encode_identity; - else { - dev_err(priv->ipu->dev, "Unsupported color space conversion\n"); - return -EINVAL; - } - /* Cast to unsigned */ - c = (const u16 (*)[3])coeff->coeff; - a = (const u16 *)coeff->offset; + c = (const u16 (*)[3])coeff.coeff; + a = (const u16 *)coeff.offset; param = ((a[0] & 0x1f) << 27) | ((c[0][0] & 0x1ff) << 18) | ((c[1][1] & 0x1ff) << 9) | (c[2][2] & 0x1ff); writel(param, base++); - param = ((a[0] & 0x1fe0) >> 5) | (coeff->scale << 8) | - (coeff->sat << 10); + param = ((a[0] & 0x1fe0) >> 5) | (coeff.scale << 8) | + (coeff.sat << 10); writel(param, base++); param = ((a[1] & 0x1f) << 27) | ((c[0][1] & 0x1ff) << 18) | @@ -357,14 +400,14 @@ void ipu_ic_task_enable(struct ipu_ic *ic) if (ic->rotation) ic_conf |= ic->bit->ic_conf_rot_en; - if (ic->in_cs != ic->out_cs) + if (ic->in_cs.cs != ic->out_cs.cs) ic_conf |= ic->bit->ic_conf_csc1_en; if (ic->graphics) { ic_conf |= ic->bit->ic_conf_cmb_en; ic_conf |= ic->bit->ic_conf_csc1_en; - if (ic->g_in_cs != ic->out_cs) + if (ic->g_in_cs.cs != ic->out_cs.cs) ic_conf |= ic->bit->ic_conf_csc2_en; } @@ -399,7 +442,7 @@ void ipu_ic_task_disable(struct ipu_ic *ic) EXPORT_SYMBOL_GPL(ipu_ic_task_disable); int ipu_ic_task_graphics_init(struct ipu_ic *ic, - enum ipu_color_space in_g_cs, + const struct ipu_ic_colorspace *g_in_cs, bool galpha_en, u32 galpha, bool colorkey_en, u32 colorkey) { @@ -416,20 +459,25 @@ int ipu_ic_task_graphics_init(struct ipu_ic *ic, ic_conf = ipu_ic_read(ic, IC_CONF); if (!(ic_conf & ic->bit->ic_conf_csc1_en)) { + struct ipu_ic_colorspace rgb_cs; + + ipu_ic_fill_colorspace(&rgb_cs, + V4L2_COLORSPACE_SRGB, + V4L2_YCBCR_ENC_601, + V4L2_QUANTIZATION_FULL_RANGE, + IPUV3_COLORSPACE_RGB); + /* need transparent CSC1 conversion */ - ret = init_csc(ic, IPUV3_COLORSPACE_RGB, - IPUV3_COLORSPACE_RGB, 0); + ret = init_csc(ic, &rgb_cs, &rgb_cs, 0); if (ret) goto unlock; } - ic->g_in_cs = in_g_cs; + ic->g_in_cs = *g_in_cs; - if (ic->g_in_cs != ic->out_cs) { - ret = init_csc(ic, ic->g_in_cs, ic->out_cs, 1); - if (ret) - goto unlock; - } + ret = init_csc(ic, &ic->g_in_cs, &ic->out_cs, 1); + if (ret) + goto unlock; if (galpha_en) { ic_conf |= IC_CONF_IC_GLB_LOC_A; @@ -456,10 +504,10 @@ int ipu_ic_task_graphics_init(struct ipu_ic *ic, EXPORT_SYMBOL_GPL(ipu_ic_task_graphics_init); int ipu_ic_task_init_rsc(struct ipu_ic *ic, + const struct ipu_ic_colorspace *in_cs, + const struct ipu_ic_colorspace *out_cs, int in_width, int in_height, int out_width, int out_height, - enum ipu_color_space in_cs, - enum ipu_color_space out_cs, u32 rsc) { struct ipu_ic_priv *priv = ic->priv; @@ -491,28 +539,24 @@ int ipu_ic_task_init_rsc(struct ipu_ic *ic, ipu_ic_write(ic, rsc, ic->reg->rsc); /* Setup color space conversion */ - ic->in_cs = in_cs; - ic->out_cs = out_cs; + ic->in_cs = *in_cs; + ic->out_cs = *out_cs; - if (ic->in_cs != ic->out_cs) { - ret = init_csc(ic, ic->in_cs, ic->out_cs, 0); - if (ret) - goto unlock; - } + ret = init_csc(ic, &ic->in_cs, &ic->out_cs, 0); -unlock: spin_unlock_irqrestore(&priv->lock, flags); return ret; } int ipu_ic_task_init(struct ipu_ic *ic, + const struct ipu_ic_colorspace *in_cs, + const struct ipu_ic_colorspace *out_cs, int in_width, int in_height, - int out_width, int out_height, - enum ipu_color_space in_cs, - enum ipu_color_space out_cs) + int out_width, int out_height) { - return ipu_ic_task_init_rsc(ic, in_width, in_height, out_width, - out_height, in_cs, out_cs, 0); + return ipu_ic_task_init_rsc(ic, in_cs, out_cs, + in_width, in_height, + out_width, out_height, 0); } EXPORT_SYMBOL_GPL(ipu_ic_task_init); diff --git a/drivers/gpu/ipu-v3/ipu-image-convert.c b/drivers/gpu/ipu-v3/ipu-image-convert.c index 13103ab86050..ccbc8f4d95d7 100644 --- a/drivers/gpu/ipu-v3/ipu-image-convert.c +++ b/drivers/gpu/ipu-v3/ipu-image-convert.c @@ -1317,7 +1317,7 @@ static int convert_start(struct ipu_image_convert_run *run, unsigned int tile) struct ipu_image_convert_priv *priv = chan->priv; struct ipu_image_convert_image *s_image = &ctx->in; struct ipu_image_convert_image *d_image = &ctx->out; - enum ipu_color_space src_cs, dest_cs; + struct ipu_ic_colorspace src_cs, dest_cs; unsigned int dst_tile = ctx->out_tile_map[tile]; unsigned int dest_width, dest_height; unsigned int col, row; @@ -1327,8 +1327,16 @@ static int convert_start(struct ipu_image_convert_run *run, unsigned int tile) dev_dbg(priv->ipu->dev, "%s: task %u: starting ctx %p run %p tile %u -> %u\n", __func__, chan->ic_task, ctx, run, tile, dst_tile); - src_cs = ipu_pixelformat_to_colorspace(s_image->fmt->fourcc); - dest_cs = ipu_pixelformat_to_colorspace(d_image->fmt->fourcc); + ipu_ic_fill_colorspace(&src_cs, + s_image->base.pix.colorspace, + s_image->base.pix.ycbcr_enc, + s_image->base.pix.quantization, + ipu_pixelformat_to_colorspace(s_image->fmt->fourcc)); + ipu_ic_fill_colorspace(&dest_cs, + d_image->base.pix.colorspace, + d_image->base.pix.ycbcr_enc, + d_image->base.pix.quantization, + ipu_pixelformat_to_colorspace(d_image->fmt->fourcc)); if (ipu_rot_mode_is_irt(ctx->rot_mode)) { /* swap width/height for resizer */ @@ -1352,13 +1360,12 @@ static int convert_start(struct ipu_image_convert_run *run, unsigned int tile) s_image->tile[tile].height, dest_width, dest_height, rsc); /* setup the IC resizer and CSC */ - ret = ipu_ic_task_init_rsc(chan->ic, - s_image->tile[tile].width, - s_image->tile[tile].height, - dest_width, - dest_height, - src_cs, dest_cs, - rsc); + ret = ipu_ic_task_init_rsc(chan->ic, &src_cs, &dest_cs, + s_image->tile[tile].width, + s_image->tile[tile].height, + dest_width, + dest_height, + rsc); if (ret) { dev_err(priv->ipu->dev, "ipu_ic_task_init failed, %d\n", ret); return ret; diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c index 5c8e6ad8c025..10f2c7684727 100644 --- a/drivers/staging/media/imx/imx-ic-prpencvf.c +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c @@ -458,6 +458,7 @@ static int prp_setup_rotation(struct prp_priv *priv) struct imx_media_video_dev *vdev = priv->vdev; struct imx_ic_priv *ic_priv = priv->ic_priv; const struct imx_media_pixfmt *outcc, *incc; + struct ipu_ic_colorspace in_cs, out_cs; struct v4l2_mbus_framefmt *infmt; struct v4l2_pix_format *outfmt; dma_addr_t phys[2]; @@ -468,6 +469,11 @@ static int prp_setup_rotation(struct prp_priv *priv) incc = priv->cc[PRPENCVF_SINK_PAD]; outcc = vdev->cc; + ipu_ic_fill_colorspace(&in_cs, infmt->colorspace, infmt->ycbcr_enc, + infmt->quantization, incc->cs); + ipu_ic_fill_colorspace(&out_cs, outfmt->colorspace, outfmt->ycbcr_enc, + outfmt->quantization, outcc->cs); + ret = imx_media_alloc_dma_buf(priv->md, &priv->rot_buf[0], outfmt->sizeimage); if (ret) { @@ -481,10 +487,9 @@ static int prp_setup_rotation(struct prp_priv *priv) goto free_rot0; } - ret = ipu_ic_task_init(priv->ic, + ret = ipu_ic_task_init(priv->ic, &in_cs, &out_cs, infmt->width, infmt->height, - outfmt->height, outfmt->width, - incc->cs, outcc->cs); + outfmt->height, outfmt->width); if (ret) { v4l2_err(&ic_priv->sd, "ipu_ic_task_init failed, %d\n", ret); goto free_rot1; @@ -574,6 +579,7 @@ static int prp_setup_norotation(struct prp_priv *priv) struct imx_media_video_dev *vdev = priv->vdev; struct imx_ic_priv *ic_priv = priv->ic_priv; const struct imx_media_pixfmt *outcc, *incc; + struct ipu_ic_colorspace in_cs, out_cs; struct v4l2_mbus_framefmt *infmt; struct v4l2_pix_format *outfmt; dma_addr_t phys[2]; @@ -584,10 +590,14 @@ static int prp_setup_norotation(struct prp_priv *priv) incc = priv->cc[PRPENCVF_SINK_PAD]; outcc = vdev->cc; - ret = ipu_ic_task_init(priv->ic, + ipu_ic_fill_colorspace(&in_cs, infmt->colorspace, infmt->ycbcr_enc, + infmt->quantization, incc->cs); + ipu_ic_fill_colorspace(&out_cs, outfmt->colorspace, outfmt->ycbcr_enc, + outfmt->quantization, outcc->cs); + + ret = ipu_ic_task_init(priv->ic, &in_cs, &out_cs, infmt->width, infmt->height, - outfmt->width, outfmt->height, - incc->cs, outcc->cs); + outfmt->width, outfmt->height); if (ret) { v4l2_err(&ic_priv->sd, "ipu_ic_task_init failed, %d\n", ret); return ret; diff --git a/include/video/imx-ipu-v3.h b/include/video/imx-ipu-v3.h index c887f4bee5f8..498f4ffc1819 100644 --- a/include/video/imx-ipu-v3.h +++ b/include/video/imx-ipu-v3.h @@ -386,20 +386,45 @@ enum ipu_ic_task { IC_NUM_TASKS, }; +/* + * The parameters that describe a colorspace according to the + * Image Converter: colorspace (chromaticities), Y'CbCr encoding, + * quantization, and "colorspace" (RGB or YUV). + */ +struct ipu_ic_colorspace { + enum v4l2_colorspace colorspace; + enum v4l2_ycbcr_encoding enc; + enum v4l2_quantization quant; + enum ipu_color_space cs; +}; + +static inline void +ipu_ic_fill_colorspace(struct ipu_ic_colorspace *ic_cs, + enum v4l2_colorspace colorspace, + enum v4l2_ycbcr_encoding enc, + enum v4l2_quantization quant, + enum ipu_color_space cs) +{ + ic_cs->colorspace = colorspace; + ic_cs->enc = enc; + ic_cs->quant = quant; + ic_cs->cs = cs; +} + struct ipu_ic; int ipu_ic_task_init(struct ipu_ic *ic, + const struct ipu_ic_colorspace *in_cs, + const struct ipu_ic_colorspace *out_cs, int in_width, int in_height, - int out_width, int out_height, - enum ipu_color_space in_cs, - enum ipu_color_space out_cs); + int out_width, int out_height); int ipu_ic_task_init_rsc(struct ipu_ic *ic, + const struct ipu_ic_colorspace *in_cs, + const struct ipu_ic_colorspace *out_cs, int in_width, int in_height, int out_width, int out_height, - enum ipu_color_space in_cs, - enum ipu_color_space out_cs, u32 rsc); int ipu_ic_task_graphics_init(struct ipu_ic *ic, - enum ipu_color_space in_g_cs, + const struct ipu_ic_colorspace *g_in_cs, bool galpha_en, u32 galpha, bool colorkey_en, u32 colorkey); void ipu_ic_task_enable(struct ipu_ic *ic); -- 2.17.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v6 3/7] gpu: ipu-v3: ipu-ic: Fully describe colorspace conversions 2019-03-07 23:33 ` [PATCH v6 3/7] gpu: ipu-v3: ipu-ic: Fully describe colorspace conversions Steve Longerbeam @ 2019-03-08 11:46 ` Philipp Zabel 2019-03-09 1:16 ` Steve Longerbeam 0 siblings, 1 reply; 3+ messages in thread From: Philipp Zabel @ 2019-03-08 11:46 UTC (permalink / raw) To: Steve Longerbeam, linux-media Cc: Tim Harvey, Mauro Carvalho Chehab, Greg Kroah-Hartman, Bartlomiej Zolnierkiewicz, open list:DRM DRIVERS FOR FREESCALE IMX, open list, open list:STAGING SUBSYSTEM, open list:FRAMEBUFFER LAYER On Thu, 2019-03-07 at 15:33 -0800, Steve Longerbeam wrote: > Only providing the input and output RGB/YUV space to the IC task init > functions is not sufficient. To fully characterize a colorspace > conversion, the colorspace (chromaticities), Y'CbCr encoding standard, > and quantization also need to be specified. > > Define a 'struct ipu_ic_colorspace' that includes all the above, and pass > the input and output ipu_ic_colorspace to the IC task init functions. > > This allows to actually enforce the fact that the IC: > > - can only encode to/from YUV full range (follow-up patch will remove > this restriction). > - can only encode to/from RGB full range. > - can only encode using BT.601 standard (follow-up patch will add > Rec.709 encoding support). > - cannot convert colorspaces from input to output, the > input and output colorspace chromaticities must be the same. > > The determination of the CSC coefficients based on the input/output > colorspace parameters are moved to a new function calc_csc_coeffs(), > called by init_csc(). > > Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> > --- > drivers/gpu/ipu-v3/ipu-ic.c | 136 +++++++++++++------- > drivers/gpu/ipu-v3/ipu-image-convert.c | 27 ++-- > drivers/staging/media/imx/imx-ic-prpencvf.c | 22 +++- > include/video/imx-ipu-v3.h | 37 +++++- > 4 files changed, 154 insertions(+), 68 deletions(-) > > diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c > index b63a2826b629..c4048c921801 100644 > --- a/drivers/gpu/ipu-v3/ipu-ic.c > +++ b/drivers/gpu/ipu-v3/ipu-ic.c > @@ -146,8 +146,10 @@ struct ipu_ic { > const struct ic_task_regoffs *reg; > const struct ic_task_bitfields *bit; > > - enum ipu_color_space in_cs, g_in_cs; > - enum ipu_color_space out_cs; > + struct ipu_ic_colorspace in_cs; > + struct ipu_ic_colorspace g_in_cs; > + struct ipu_ic_colorspace out_cs; > + > bool graphics; > bool rotation; > bool in_use; > @@ -235,42 +237,83 @@ static const struct ic_encode_coeff ic_encode_ycbcr2rgb_601 = { > .scale = 2, > }; > > +static int calc_csc_coeffs(struct ipu_ic_priv *priv, > + struct ic_encode_coeff *coeff_out, > + const struct ipu_ic_colorspace *in, > + const struct ipu_ic_colorspace *out) > +{ > + bool inverse_encode; > + > + if (in->colorspace != out->colorspace) { > + dev_err(priv->ipu->dev, "Cannot convert colorspaces\n"); > + return -ENOTSUPP; > + } I don't think this is useful enough to warrant having the colorspace field in ipu_ic_colorspace. Let the caller make sure of this, same as for xfer_func. > + if (out->enc != V4L2_YCBCR_ENC_601) { > + dev_err(priv->ipu->dev, "Only BT.601 encoding supported\n"); > + return -ENOTSUPP; > + } This is only important if out->cs is IPUV3_COLORSPACE_YUV, right? If the output is RGB this field shouldn't matter. > + > + if ((in->cs = IPUV3_COLORSPACE_YUV && > + in->quant != V4L2_QUANTIZATION_FULL_RANGE) || > + (out->cs = IPUV3_COLORSPACE_YUV && > + out->quant != V4L2_QUANTIZATION_FULL_RANGE)) { > + dev_err(priv->ipu->dev, "Limited range YUV not supported\n"); > + return -ENOTSUPP; > + } > + > + if ((in->cs = IPUV3_COLORSPACE_RGB && > + in->quant != V4L2_QUANTIZATION_FULL_RANGE) || > + (out->cs = IPUV3_COLORSPACE_RGB && > + out->quant != V4L2_QUANTIZATION_FULL_RANGE)) { > + dev_err(priv->ipu->dev, "Limited range RGB not supported\n"); > + return -ENOTSUPP; > + } > + > + if (in->cs = out->cs) { > + *coeff_out = ic_encode_identity; > + > + return 0; > + } > + > + inverse_encode = (in->cs = IPUV3_COLORSPACE_YUV); What does inverse_encode mean in this context? > + > + *coeff_out = inverse_encode ? > + ic_encode_ycbcr2rgb_601 : ic_encode_rgb2ycbcr_601; > + > + return 0; > +} > + > static int init_csc(struct ipu_ic *ic, > - enum ipu_color_space inf, > - enum ipu_color_space outf, > + const struct ipu_ic_colorspace *in, > + const struct ipu_ic_colorspace *out, > int csc_index) > { > struct ipu_ic_priv *priv = ic->priv; > - const struct ic_encode_coeff *coeff; > + struct ic_encode_coeff coeff; I understand this is a preparation for patch 5, but on its own this introduces an unnecessary copy. > u32 __iomem *base; > const u16 (*c)[3]; > const u16 *a; > u32 param; > + int ret; > + > + ret = calc_csc_coeffs(priv, &coeff, in, out); > + if (ret) > + return ret; > > base = (u32 __iomem *) > (priv->tpmem_base + ic->reg->tpmem_csc[csc_index]); > > - if (inf = IPUV3_COLORSPACE_YUV && outf = IPUV3_COLORSPACE_RGB) > - coeff = &ic_encode_ycbcr2rgb_601; > - else if (inf = IPUV3_COLORSPACE_RGB && outf = IPUV3_COLORSPACE_YUV) > - coeff = &ic_encode_rgb2ycbcr_601; > - else if (inf = IPUV3_COLORSPACE_RGB && outf = IPUV3_COLORSPACE_RGB) > - coeff = &ic_encode_identity; > - else { > - dev_err(priv->ipu->dev, "Unsupported color space conversion\n"); > - return -EINVAL; > - } > - > /* Cast to unsigned */ > - c = (const u16 (*)[3])coeff->coeff; > - a = (const u16 *)coeff->offset; > + c = (const u16 (*)[3])coeff.coeff; > + a = (const u16 *)coeff.offset; > > param = ((a[0] & 0x1f) << 27) | ((c[0][0] & 0x1ff) << 18) | > ((c[1][1] & 0x1ff) << 9) | (c[2][2] & 0x1ff); > writel(param, base++); > > - param = ((a[0] & 0x1fe0) >> 5) | (coeff->scale << 8) | > - (coeff->sat << 10); > + param = ((a[0] & 0x1fe0) >> 5) | (coeff.scale << 8) | > + (coeff.sat << 10); > writel(param, base++); > > param = ((a[1] & 0x1f) << 27) | ((c[0][1] & 0x1ff) << 18) | > @@ -357,14 +400,14 @@ void ipu_ic_task_enable(struct ipu_ic *ic) > if (ic->rotation) > ic_conf |= ic->bit->ic_conf_rot_en; > > - if (ic->in_cs != ic->out_cs) > + if (ic->in_cs.cs != ic->out_cs.cs) > ic_conf |= ic->bit->ic_conf_csc1_en; > > if (ic->graphics) { > ic_conf |= ic->bit->ic_conf_cmb_en; > ic_conf |= ic->bit->ic_conf_csc1_en; > > - if (ic->g_in_cs != ic->out_cs) > + if (ic->g_in_cs.cs != ic->out_cs.cs) > ic_conf |= ic->bit->ic_conf_csc2_en; > } > > @@ -399,7 +442,7 @@ void ipu_ic_task_disable(struct ipu_ic *ic) > EXPORT_SYMBOL_GPL(ipu_ic_task_disable); > > int ipu_ic_task_graphics_init(struct ipu_ic *ic, > - enum ipu_color_space in_g_cs, > + const struct ipu_ic_colorspace *g_in_cs, What made you decide not to expose the task parameter structure? I was hoping we could eventually move the V4L2 colorimetry settings to conversion matrix translation into imx-media. Btw, do you have any plans for using IC composition? ipu_ic_task_graphics_init() is currently unused... > bool galpha_en, u32 galpha, > bool colorkey_en, u32 colorkey) > { > @@ -416,20 +459,25 @@ int ipu_ic_task_graphics_init(struct ipu_ic *ic, > ic_conf = ipu_ic_read(ic, IC_CONF); > > if (!(ic_conf & ic->bit->ic_conf_csc1_en)) { > + struct ipu_ic_colorspace rgb_cs; > + > + ipu_ic_fill_colorspace(&rgb_cs, > + V4L2_COLORSPACE_SRGB, > + V4L2_YCBCR_ENC_601, > + V4L2_QUANTIZATION_FULL_RANGE, > + IPUV3_COLORSPACE_RGB); > + > /* need transparent CSC1 conversion */ > - ret = init_csc(ic, IPUV3_COLORSPACE_RGB, > - IPUV3_COLORSPACE_RGB, 0); > + ret = init_csc(ic, &rgb_cs, &rgb_cs, 0); > if (ret) > goto unlock; > } > > - ic->g_in_cs = in_g_cs; > + ic->g_in_cs = *g_in_cs; > > - if (ic->g_in_cs != ic->out_cs) { > - ret = init_csc(ic, ic->g_in_cs, ic->out_cs, 1); > - if (ret) > - goto unlock; > - } > + ret = init_csc(ic, &ic->g_in_cs, &ic->out_cs, 1); > + if (ret) > + goto unlock; I had to look twice, but this is fine. If ic->g_in_cs = ic->out_cs, ipu_ic_task_enable() won't enable CSC2 in IC_CONF, and these TPMEM values will be ignored. > > if (galpha_en) { > ic_conf |= IC_CONF_IC_GLB_LOC_A; > @@ -456,10 +504,10 @@ int ipu_ic_task_graphics_init(struct ipu_ic *ic, > EXPORT_SYMBOL_GPL(ipu_ic_task_graphics_init); > > int ipu_ic_task_init_rsc(struct ipu_ic *ic, > + const struct ipu_ic_colorspace *in_cs, > + const struct ipu_ic_colorspace *out_cs, > int in_width, int in_height, > int out_width, int out_height, > - enum ipu_color_space in_cs, > - enum ipu_color_space out_cs, > u32 rsc) > { > struct ipu_ic_priv *priv = ic->priv; > @@ -491,28 +539,24 @@ int ipu_ic_task_init_rsc(struct ipu_ic *ic, > ipu_ic_write(ic, rsc, ic->reg->rsc); > > /* Setup color space conversion */ > - ic->in_cs = in_cs; > - ic->out_cs = out_cs; > + ic->in_cs = *in_cs; > + ic->out_cs = *out_cs; > > - if (ic->in_cs != ic->out_cs) { > - ret = init_csc(ic, ic->in_cs, ic->out_cs, 0); > - if (ret) > - goto unlock; > - } > + ret = init_csc(ic, &ic->in_cs, &ic->out_cs, 0); Same as above for CSC1. > -unlock: > spin_unlock_irqrestore(&priv->lock, flags); > return ret; > } > > int ipu_ic_task_init(struct ipu_ic *ic, > + const struct ipu_ic_colorspace *in_cs, > + const struct ipu_ic_colorspace *out_cs, > int in_width, int in_height, > - int out_width, int out_height, > - enum ipu_color_space in_cs, > - enum ipu_color_space out_cs) > + int out_width, int out_height) > { > - return ipu_ic_task_init_rsc(ic, in_width, in_height, out_width, > - out_height, in_cs, out_cs, 0); > + return ipu_ic_task_init_rsc(ic, in_cs, out_cs, > + in_width, in_height, > + out_width, out_height, 0); > } > EXPORT_SYMBOL_GPL(ipu_ic_task_init); > > diff --git a/drivers/gpu/ipu-v3/ipu-image-convert.c b/drivers/gpu/ipu-v3/ipu-image-convert.c > index 13103ab86050..ccbc8f4d95d7 100644 > --- a/drivers/gpu/ipu-v3/ipu-image-convert.c > +++ b/drivers/gpu/ipu-v3/ipu-image-convert.c > @@ -1317,7 +1317,7 @@ static int convert_start(struct ipu_image_convert_run *run, unsigned int tile) > struct ipu_image_convert_priv *priv = chan->priv; > struct ipu_image_convert_image *s_image = &ctx->in; > struct ipu_image_convert_image *d_image = &ctx->out; > - enum ipu_color_space src_cs, dest_cs; > + struct ipu_ic_colorspace src_cs, dest_cs; > unsigned int dst_tile = ctx->out_tile_map[tile]; > unsigned int dest_width, dest_height; > unsigned int col, row; > @@ -1327,8 +1327,16 @@ static int convert_start(struct ipu_image_convert_run *run, unsigned int tile) > dev_dbg(priv->ipu->dev, "%s: task %u: starting ctx %p run %p tile %u -> %u\n", > __func__, chan->ic_task, ctx, run, tile, dst_tile); > > - src_cs = ipu_pixelformat_to_colorspace(s_image->fmt->fourcc); > - dest_cs = ipu_pixelformat_to_colorspace(d_image->fmt->fourcc); > + ipu_ic_fill_colorspace(&src_cs, > + s_image->base.pix.colorspace, > + s_image->base.pix.ycbcr_enc, > + s_image->base.pix.quantization, > + ipu_pixelformat_to_colorspace(s_image->fmt->fourcc)); > + ipu_ic_fill_colorspace(&dest_cs, > + d_image->base.pix.colorspace, > + d_image->base.pix.ycbcr_enc, > + d_image->base.pix.quantization, > + ipu_pixelformat_to_colorspace(d_image->fmt->fourcc)); If ipu_ic_task_init(_rsc) could be passed the task parameter structure, it could be calculated once in ipu_image_convert_prepare and stored in ipu_image_convert_ctx for repeated use. regards Philipp ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v6 3/7] gpu: ipu-v3: ipu-ic: Fully describe colorspace conversions 2019-03-08 11:46 ` Philipp Zabel @ 2019-03-09 1:16 ` Steve Longerbeam 0 siblings, 0 replies; 3+ messages in thread From: Steve Longerbeam @ 2019-03-09 1:16 UTC (permalink / raw) To: Philipp Zabel, linux-media Cc: open list:STAGING SUBSYSTEM, open list:FRAMEBUFFER LAYER, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, open list, open list:DRM DRIVERS FOR FREESCALE IMX, Mauro Carvalho Chehab On 3/8/19 3:46 AM, Philipp Zabel wrote: > On Thu, 2019-03-07 at 15:33 -0800, Steve Longerbeam wrote: >> Only providing the input and output RGB/YUV space to the IC task init >> functions is not sufficient. To fully characterize a colorspace >> conversion, the colorspace (chromaticities), Y'CbCr encoding standard, >> and quantization also need to be specified. >> >> Define a 'struct ipu_ic_colorspace' that includes all the above, and pass >> the input and output ipu_ic_colorspace to the IC task init functions. >> >> This allows to actually enforce the fact that the IC: >> >> - can only encode to/from YUV full range (follow-up patch will remove >> this restriction). >> - can only encode to/from RGB full range. >> - can only encode using BT.601 standard (follow-up patch will add >> Rec.709 encoding support). >> - cannot convert colorspaces from input to output, the >> input and output colorspace chromaticities must be the same. >> >> The determination of the CSC coefficients based on the input/output >> colorspace parameters are moved to a new function calc_csc_coeffs(), >> called by init_csc(). >> >> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> >> --- >> drivers/gpu/ipu-v3/ipu-ic.c | 136 +++++++++++++------- >> drivers/gpu/ipu-v3/ipu-image-convert.c | 27 ++-- >> drivers/staging/media/imx/imx-ic-prpencvf.c | 22 +++- >> include/video/imx-ipu-v3.h | 37 +++++- >> 4 files changed, 154 insertions(+), 68 deletions(-) >> >> diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c >> index b63a2826b629..c4048c921801 100644 >> --- a/drivers/gpu/ipu-v3/ipu-ic.c >> +++ b/drivers/gpu/ipu-v3/ipu-ic.c >> @@ -146,8 +146,10 @@ struct ipu_ic { >> const struct ic_task_regoffs *reg; >> const struct ic_task_bitfields *bit; >> >> - enum ipu_color_space in_cs, g_in_cs; >> - enum ipu_color_space out_cs; >> + struct ipu_ic_colorspace in_cs; >> + struct ipu_ic_colorspace g_in_cs; >> + struct ipu_ic_colorspace out_cs; >> + >> bool graphics; >> bool rotation; >> bool in_use; >> @@ -235,42 +237,83 @@ static const struct ic_encode_coeff ic_encode_ycbcr2rgb_601 = { >> .scale = 2, >> }; >> >> +static int calc_csc_coeffs(struct ipu_ic_priv *priv, >> + struct ic_encode_coeff *coeff_out, >> + const struct ipu_ic_colorspace *in, >> + const struct ipu_ic_colorspace *out) >> +{ >> + bool inverse_encode; >> + >> + if (in->colorspace != out->colorspace) { >> + dev_err(priv->ipu->dev, "Cannot convert colorspaces\n"); >> + return -ENOTSUPP; >> + } > I don't think this is useful enough to warrant having the colorspace > field in ipu_ic_colorspace. Let the caller make sure of this, same as > for xfer_func. Ok, for xfer_func it is implicit that the gamma function must be the same for input and output, so I agree it might as well be implicit for chromaticities too. > >> + if (out->enc != V4L2_YCBCR_ENC_601) { >> + dev_err(priv->ipu->dev, "Only BT.601 encoding supported\n"); >> + return -ENOTSUPP; >> + } > This is only important if out->cs is IPUV3_COLORSPACE_YUV, right? If the > output is RGB this field shouldn't matter. It matters for encoding YUV to RGB, or the inverse RGB to YUV. The encoding standard doesn't matter only if no encoding/inverse encoding is requested (YUV to YUV or RGB to RGB). > >> + >> + if ((in->cs = IPUV3_COLORSPACE_YUV && >> + in->quant != V4L2_QUANTIZATION_FULL_RANGE) || >> + (out->cs = IPUV3_COLORSPACE_YUV && >> + out->quant != V4L2_QUANTIZATION_FULL_RANGE)) { >> + dev_err(priv->ipu->dev, "Limited range YUV not supported\n"); >> + return -ENOTSUPP; >> + } >> + >> + if ((in->cs = IPUV3_COLORSPACE_RGB && >> + in->quant != V4L2_QUANTIZATION_FULL_RANGE) || >> + (out->cs = IPUV3_COLORSPACE_RGB && >> + out->quant != V4L2_QUANTIZATION_FULL_RANGE)) { >> + dev_err(priv->ipu->dev, "Limited range RGB not supported\n"); >> + return -ENOTSUPP; >> + } >> + >> + if (in->cs = out->cs) { >> + *coeff_out = ic_encode_identity; >> + >> + return 0; >> + } >> + >> + inverse_encode = (in->cs = IPUV3_COLORSPACE_YUV); > What does inverse_encode mean in this context? It means YUV to RGB. At this point in the function it is determined that encoding or inverse encoding is requested. > >> + >> + *coeff_out = inverse_encode ? >> + ic_encode_ycbcr2rgb_601 : ic_encode_rgb2ycbcr_601; >> + >> + return 0; >> +} >> + >> static int init_csc(struct ipu_ic *ic, >> - enum ipu_color_space inf, >> - enum ipu_color_space outf, >> + const struct ipu_ic_colorspace *in, >> + const struct ipu_ic_colorspace *out, >> int csc_index) >> { >> struct ipu_ic_priv *priv = ic->priv; >> - const struct ic_encode_coeff *coeff; >> + struct ic_encode_coeff coeff; > I understand this is a preparation for patch 5, but on its own this > introduces an unnecessary copy. True, I'll try to remove the copy in this patch. > >> u32 __iomem *base; >> const u16 (*c)[3]; >> const u16 *a; >> u32 param; >> + int ret; >> + >> + ret = calc_csc_coeffs(priv, &coeff, in, out); >> + if (ret) >> + return ret; >> >> base = (u32 __iomem *) >> (priv->tpmem_base + ic->reg->tpmem_csc[csc_index]); >> >> - if (inf = IPUV3_COLORSPACE_YUV && outf = IPUV3_COLORSPACE_RGB) >> - coeff = &ic_encode_ycbcr2rgb_601; >> - else if (inf = IPUV3_COLORSPACE_RGB && outf = IPUV3_COLORSPACE_YUV) >> - coeff = &ic_encode_rgb2ycbcr_601; >> - else if (inf = IPUV3_COLORSPACE_RGB && outf = IPUV3_COLORSPACE_RGB) >> - coeff = &ic_encode_identity; >> - else { >> - dev_err(priv->ipu->dev, "Unsupported color space conversion\n"); >> - return -EINVAL; >> - } >> - >> /* Cast to unsigned */ >> - c = (const u16 (*)[3])coeff->coeff; >> - a = (const u16 *)coeff->offset; >> + c = (const u16 (*)[3])coeff.coeff; >> + a = (const u16 *)coeff.offset; >> >> param = ((a[0] & 0x1f) << 27) | ((c[0][0] & 0x1ff) << 18) | >> ((c[1][1] & 0x1ff) << 9) | (c[2][2] & 0x1ff); >> writel(param, base++); >> >> - param = ((a[0] & 0x1fe0) >> 5) | (coeff->scale << 8) | >> - (coeff->sat << 10); >> + param = ((a[0] & 0x1fe0) >> 5) | (coeff.scale << 8) | >> + (coeff.sat << 10); >> writel(param, base++); >> >> param = ((a[1] & 0x1f) << 27) | ((c[0][1] & 0x1ff) << 18) | >> @@ -357,14 +400,14 @@ void ipu_ic_task_enable(struct ipu_ic *ic) >> if (ic->rotation) >> ic_conf |= ic->bit->ic_conf_rot_en; >> >> - if (ic->in_cs != ic->out_cs) >> + if (ic->in_cs.cs != ic->out_cs.cs) >> ic_conf |= ic->bit->ic_conf_csc1_en; >> >> if (ic->graphics) { >> ic_conf |= ic->bit->ic_conf_cmb_en; >> ic_conf |= ic->bit->ic_conf_csc1_en; >> >> - if (ic->g_in_cs != ic->out_cs) >> + if (ic->g_in_cs.cs != ic->out_cs.cs) >> ic_conf |= ic->bit->ic_conf_csc2_en; >> } >> >> @@ -399,7 +442,7 @@ void ipu_ic_task_disable(struct ipu_ic *ic) >> EXPORT_SYMBOL_GPL(ipu_ic_task_disable); >> >> int ipu_ic_task_graphics_init(struct ipu_ic *ic, >> - enum ipu_color_space in_g_cs, >> + const struct ipu_ic_colorspace *g_in_cs, > What made you decide not to expose the task parameter structure? > > I was hoping we could eventually move the V4L2 colorimetry settings to > conversion matrix translation into imx-media. Sure, I'm fine with that. I'll move the task parameter struct to imx-ipu-v3.h. > > Btw, do you have any plans for using IC composition? > ipu_ic_task_graphics_init() is currently unused... No plans for IC composition, I've only been keeping the function up-to-date. I've never even tested this, it might not even work. Should it be removed? > >> bool galpha_en, u32 galpha, >> bool colorkey_en, u32 colorkey) >> { >> @@ -416,20 +459,25 @@ int ipu_ic_task_graphics_init(struct ipu_ic *ic, >> ic_conf = ipu_ic_read(ic, IC_CONF); >> >> if (!(ic_conf & ic->bit->ic_conf_csc1_en)) { >> + struct ipu_ic_colorspace rgb_cs; >> + >> + ipu_ic_fill_colorspace(&rgb_cs, >> + V4L2_COLORSPACE_SRGB, >> + V4L2_YCBCR_ENC_601, >> + V4L2_QUANTIZATION_FULL_RANGE, >> + IPUV3_COLORSPACE_RGB); >> + >> /* need transparent CSC1 conversion */ >> - ret = init_csc(ic, IPUV3_COLORSPACE_RGB, >> - IPUV3_COLORSPACE_RGB, 0); >> + ret = init_csc(ic, &rgb_cs, &rgb_cs, 0); >> if (ret) >> goto unlock; >> } >> >> - ic->g_in_cs = in_g_cs; >> + ic->g_in_cs = *g_in_cs; >> >> - if (ic->g_in_cs != ic->out_cs) { >> - ret = init_csc(ic, ic->g_in_cs, ic->out_cs, 1); >> - if (ret) >> - goto unlock; >> - } >> + ret = init_csc(ic, &ic->g_in_cs, &ic->out_cs, 1); >> + if (ret) >> + goto unlock; > I had to look twice, but this is fine. If ic->g_in_cs = ic->out_cs, > ipu_ic_task_enable() won't enable CSC2 in IC_CONF, and these TPMEM > values will be ignored. > >> >> if (galpha_en) { >> ic_conf |= IC_CONF_IC_GLB_LOC_A; >> @@ -456,10 +504,10 @@ int ipu_ic_task_graphics_init(struct ipu_ic *ic, >> EXPORT_SYMBOL_GPL(ipu_ic_task_graphics_init); >> >> int ipu_ic_task_init_rsc(struct ipu_ic *ic, >> + const struct ipu_ic_colorspace *in_cs, >> + const struct ipu_ic_colorspace *out_cs, >> int in_width, int in_height, >> int out_width, int out_height, >> - enum ipu_color_space in_cs, >> - enum ipu_color_space out_cs, >> u32 rsc) >> { >> struct ipu_ic_priv *priv = ic->priv; >> @@ -491,28 +539,24 @@ int ipu_ic_task_init_rsc(struct ipu_ic *ic, >> ipu_ic_write(ic, rsc, ic->reg->rsc); >> >> /* Setup color space conversion */ >> - ic->in_cs = in_cs; >> - ic->out_cs = out_cs; >> + ic->in_cs = *in_cs; >> + ic->out_cs = *out_cs; >> >> - if (ic->in_cs != ic->out_cs) { >> - ret = init_csc(ic, ic->in_cs, ic->out_cs, 0); >> - if (ret) >> - goto unlock; >> - } >> + ret = init_csc(ic, &ic->in_cs, &ic->out_cs, 0); > Same as above for CSC1. > >> -unlock: >> spin_unlock_irqrestore(&priv->lock, flags); >> return ret; >> } >> >> int ipu_ic_task_init(struct ipu_ic *ic, >> + const struct ipu_ic_colorspace *in_cs, >> + const struct ipu_ic_colorspace *out_cs, >> int in_width, int in_height, >> - int out_width, int out_height, >> - enum ipu_color_space in_cs, >> - enum ipu_color_space out_cs) >> + int out_width, int out_height) >> { >> - return ipu_ic_task_init_rsc(ic, in_width, in_height, out_width, >> - out_height, in_cs, out_cs, 0); >> + return ipu_ic_task_init_rsc(ic, in_cs, out_cs, >> + in_width, in_height, >> + out_width, out_height, 0); >> } >> EXPORT_SYMBOL_GPL(ipu_ic_task_init); >> >> diff --git a/drivers/gpu/ipu-v3/ipu-image-convert.c b/drivers/gpu/ipu-v3/ipu-image-convert.c >> index 13103ab86050..ccbc8f4d95d7 100644 >> --- a/drivers/gpu/ipu-v3/ipu-image-convert.c >> +++ b/drivers/gpu/ipu-v3/ipu-image-convert.c >> @@ -1317,7 +1317,7 @@ static int convert_start(struct ipu_image_convert_run *run, unsigned int tile) >> struct ipu_image_convert_priv *priv = chan->priv; >> struct ipu_image_convert_image *s_image = &ctx->in; >> struct ipu_image_convert_image *d_image = &ctx->out; >> - enum ipu_color_space src_cs, dest_cs; >> + struct ipu_ic_colorspace src_cs, dest_cs; >> unsigned int dst_tile = ctx->out_tile_map[tile]; >> unsigned int dest_width, dest_height; >> unsigned int col, row; >> @@ -1327,8 +1327,16 @@ static int convert_start(struct ipu_image_convert_run *run, unsigned int tile) >> dev_dbg(priv->ipu->dev, "%s: task %u: starting ctx %p run %p tile %u -> %u\n", >> __func__, chan->ic_task, ctx, run, tile, dst_tile); >> >> - src_cs = ipu_pixelformat_to_colorspace(s_image->fmt->fourcc); >> - dest_cs = ipu_pixelformat_to_colorspace(d_image->fmt->fourcc); >> + ipu_ic_fill_colorspace(&src_cs, >> + s_image->base.pix.colorspace, >> + s_image->base.pix.ycbcr_enc, >> + s_image->base.pix.quantization, >> + ipu_pixelformat_to_colorspace(s_image->fmt->fourcc)); >> + ipu_ic_fill_colorspace(&dest_cs, >> + d_image->base.pix.colorspace, >> + d_image->base.pix.ycbcr_enc, >> + d_image->base.pix.quantization, >> + ipu_pixelformat_to_colorspace(d_image->fmt->fourcc)); > If ipu_ic_task_init(_rsc) could be passed the task parameter structure, > it could be calculated once in ipu_image_convert_prepare and stored in > ipu_image_convert_ctx for repeated use. Yes, I'll add this for v7. Steve ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-03-09 1:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20190307233356.23748-1-slongerbeam@gmail.com>
2019-03-07 23:33 ` [PATCH v6 3/7] gpu: ipu-v3: ipu-ic: Fully describe colorspace conversions Steve Longerbeam
2019-03-08 11:46 ` Philipp Zabel
2019-03-09 1:16 ` Steve Longerbeam
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox