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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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