* [PATCH v6 1/7] gpu: ipu-v3: ipu-ic: Fix saturation bit offset in TPMEM
[not found] <20190307233356.23748-1-slongerbeam@gmail.com>
@ 2019-03-07 23:33 ` Steve Longerbeam
2019-03-08 10:24 ` Philipp Zabel
2019-03-07 23:33 ` [PATCH v6 2/7] gpu: ipu-v3: ipu-ic: Fix BT.601 coefficients Steve Longerbeam
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Steve Longerbeam @ 2019-03-07 23:33 UTC (permalink / raw)
To: linux-media
Cc: Tim Harvey, Steve Longerbeam, stable, Philipp Zabel,
open list:DRM DRIVERS FOR FREESCALE IMX, open list
The saturation bit was being set at bit 9 in the second 32-bit word
of the TPMEM CSC. This isn't correct, the saturation bit is bit 42,
which is bit 10 of the second word.
Fixes: 1aa8ea0d2bd5d ("gpu: ipu-v3: Add Image Converter unit")
Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
Cc: stable@vger.kernel.org
---
drivers/gpu/ipu-v3/ipu-ic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
index 594c3cbc8291..18816ccf600e 100644
--- a/drivers/gpu/ipu-v3/ipu-ic.c
+++ b/drivers/gpu/ipu-v3/ipu-ic.c
@@ -257,7 +257,7 @@ static int init_csc(struct ipu_ic *ic,
writel(param, base++);
param = ((a[0] & 0x1fe0) >> 5) | (params->scale << 8) |
- (params->sat << 9);
+ (params->sat << 10);
writel(param, base++);
param = ((a[1] & 0x1f) << 27) | ((c[0][1] & 0x1ff) << 18) |
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v6 2/7] gpu: ipu-v3: ipu-ic: Fix BT.601 coefficients
[not found] <20190307233356.23748-1-slongerbeam@gmail.com>
2019-03-07 23:33 ` [PATCH v6 1/7] gpu: ipu-v3: ipu-ic: Fix saturation bit offset in TPMEM Steve Longerbeam
@ 2019-03-07 23:33 ` Steve Longerbeam
2019-03-08 10:23 ` Philipp Zabel
2019-03-07 23:33 ` [PATCH v6 3/7] gpu: ipu-v3: ipu-ic: Fully describe colorspace conversions Steve Longerbeam
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Steve Longerbeam @ 2019-03-07 23:33 UTC (permalink / raw)
To: linux-media
Cc: Tim Harvey, Steve Longerbeam, stable, Philipp Zabel,
open list:DRM DRIVERS FOR FREESCALE IMX, open list
The ycbcr2rgb and inverse rgb2ycbcr tables define the BT.601 Y'CbCr
encoding coefficients.
The rgb2ycbcr table specifically describes the BT.601 encoding from
full range RGB to full range YUV. Add table comments to make this more
clear.
The ycbcr2rgb inverse table describes encoding YUV limited range to RGB
full range. To be consistent with the rgb2ycbcr table, convert this to
YUV full range to RGB full range, and adjust/expand on the comments.
The ic_csc_rgb2rgb table is just an identity matrix, so rename to
ic_encode_identity.
Fixes: 1aa8ea0d2bd5d ("gpu: ipu-v3: Add Image Converter unit")
Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
Cc: stable@vger.kernel.org
---
drivers/gpu/ipu-v3/ipu-ic.c | 61 ++++++++++++++++++++++---------------
1 file changed, 37 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
index 18816ccf600e..b63a2826b629 100644
--- a/drivers/gpu/ipu-v3/ipu-ic.c
+++ b/drivers/gpu/ipu-v3/ipu-ic.c
@@ -175,7 +175,7 @@ static inline void ipu_ic_write(struct ipu_ic *ic, u32 value, unsigned offset)
writel(value, ic->priv->base + offset);
}
-struct ic_csc_params {
+struct ic_encode_coeff {
s16 coeff[3][3]; /* signed 9-bit integer coefficients */
s16 offset[3]; /* signed 11+2-bit fixed point offset */
u8 scale:2; /* scale coefficients * 2^(scale-1) */
@@ -183,13 +183,15 @@ struct ic_csc_params {
};
/*
- * Y = R * .299 + G * .587 + B * .114;
- * U = R * -.169 + G * -.332 + B * .500 + 128.;
- * V = R * .500 + G * -.419 + B * -.0813 + 128.;
+ * BT.601 encoding from RGB full range to YUV full range:
+ *
+ * Y = .2990 * R + .5870 * G + .1140 * B
+ * U = -.1687 * R - .3313 * G + .5000 * B + 128
+ * V = .5000 * R - .4187 * G - .0813 * B + 128
*/
-static const struct ic_csc_params ic_csc_rgb2ycbcr = {
+static const struct ic_encode_coeff ic_encode_rgb2ycbcr_601 = {
.coeff = {
- { 77, 150, 29 },
+ { 77, 150, 29 },
{ 469, 427, 128 },
{ 128, 405, 491 },
},
@@ -197,8 +199,11 @@ static const struct ic_csc_params ic_csc_rgb2ycbcr = {
.scale = 1,
};
-/* transparent RGB->RGB matrix for graphics combining */
-static const struct ic_csc_params ic_csc_rgb2rgb = {
+/*
+ * identity matrix, used for transparent RGB->RGB graphics
+ * combining.
+ */
+static const struct ic_encode_coeff ic_encode_identity = {
.coeff = {
{ 128, 0, 0 },
{ 0, 128, 0 },
@@ -208,17 +213,25 @@ static const struct ic_csc_params ic_csc_rgb2rgb = {
};
/*
- * R = (1.164 * (Y - 16)) + (1.596 * (Cr - 128));
- * G = (1.164 * (Y - 16)) - (0.392 * (Cb - 128)) - (0.813 * (Cr - 128));
- * B = (1.164 * (Y - 16)) + (2.017 * (Cb - 128);
+ * Inverse BT.601 encoding from YUV full range to RGB full range:
+ *
+ * R = 1. * Y + 0 * (Cb - 128) + 1.4020 * (Cr - 128)
+ * G = 1. * Y - .3442 * (Cb - 128) - 0.7142 * (Cr - 128)
+ * B = 1. * Y + 1.7720 * (Cb - 128) + 0 * (Cr - 128)
+ *
+ * equivalently (factoring out the offsets):
+ *
+ * R = 1. * Y + 0 * Cb + 1.4020 * Cr - 179.456
+ * G = 1. * Y - .3442 * Cb - 0.7142 * Cr + 135.475
+ * B = 1. * Y + 1.7720 * Cb + 0 * Cr - 226.816
*/
-static const struct ic_csc_params ic_csc_ycbcr2rgb = {
+static const struct ic_encode_coeff ic_encode_ycbcr2rgb_601 = {
.coeff = {
- { 149, 0, 204 },
- { 149, 462, 408 },
- { 149, 255, 0 },
+ { 128, 0, 179 },
+ { 128, 468, 421 },
+ { 128, 227, 0 },
},
- .offset = { -446, 266, -554 },
+ .offset = { -359, 271, -454 },
.scale = 2,
};
@@ -228,7 +241,7 @@ static int init_csc(struct ipu_ic *ic,
int csc_index)
{
struct ipu_ic_priv *priv = ic->priv;
- const struct ic_csc_params *params;
+ const struct ic_encode_coeff *coeff;
u32 __iomem *base;
const u16 (*c)[3];
const u16 *a;
@@ -238,26 +251,26 @@ static int init_csc(struct ipu_ic *ic,
(priv->tpmem_base + ic->reg->tpmem_csc[csc_index]);
if (inf == IPUV3_COLORSPACE_YUV && outf == IPUV3_COLORSPACE_RGB)
- params = &ic_csc_ycbcr2rgb;
+ coeff = &ic_encode_ycbcr2rgb_601;
else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_YUV)
- params = &ic_csc_rgb2ycbcr;
+ coeff = &ic_encode_rgb2ycbcr_601;
else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_RGB)
- params = &ic_csc_rgb2rgb;
+ coeff = &ic_encode_identity;
else {
dev_err(priv->ipu->dev, "Unsupported color space conversion\n");
return -EINVAL;
}
/* Cast to unsigned */
- c = (const u16 (*)[3])params->coeff;
- a = (const u16 *)params->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) | (params->scale << 8) |
- (params->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) |
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [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 ` [PATCH v6 1/7] gpu: ipu-v3: ipu-ic: Fix saturation bit offset in TPMEM Steve Longerbeam
2019-03-07 23:33 ` [PATCH v6 2/7] gpu: ipu-v3: ipu-ic: Fix BT.601 coefficients Steve Longerbeam
@ 2019-03-07 23:33 ` Steve Longerbeam
2019-03-08 11:46 ` Philipp Zabel
2019-03-07 23:33 ` [PATCH v6 4/7] gpu: ipu-v3: ipu-ic: Add support for Rec.709 encoding Steve Longerbeam
2019-03-07 23:33 ` [PATCH v6 5/7] gpu: ipu-v3: ipu-ic: Add support for limited range encoding Steve Longerbeam
4 siblings, 1 reply; 12+ 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] 12+ messages in thread
* [PATCH v6 4/7] gpu: ipu-v3: ipu-ic: Add support for Rec.709 encoding
[not found] <20190307233356.23748-1-slongerbeam@gmail.com>
` (2 preceding siblings ...)
2019-03-07 23:33 ` [PATCH v6 3/7] gpu: ipu-v3: ipu-ic: Fully describe colorspace conversions Steve Longerbeam
@ 2019-03-07 23:33 ` Steve Longerbeam
2019-03-07 23:33 ` [PATCH v6 5/7] gpu: ipu-v3: ipu-ic: Add support for limited range encoding Steve Longerbeam
4 siblings, 0 replies; 12+ messages in thread
From: Steve Longerbeam @ 2019-03-07 23:33 UTC (permalink / raw)
To: linux-media
Cc: open list, open list:DRM DRIVERS FOR FREESCALE IMX, Steve Longerbeam
Add support for Rec.709 encoding and inverse encoding.
Reported-by: Tim Harvey <tharvey@gateworks.com>
Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
Changes in v5:
- moved API changes to a previous patch.
- moved CSC coeff calc to new function calc_csc_coeffs().
Changes in v4:
- fix compile error.
Chnges in v3:
- none.
Changes in v2:
- only return "Unsupported YCbCr encoding" error if inf != outf,
since if inf == outf, the identity matrix can be used. Reported
by Tim Harvey.
---
drivers/gpu/ipu-v3/ipu-ic.c | 63 ++++++++++++++++++++++++++++++++-----
1 file changed, 56 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
index c4048c921801..1460901af9b5 100644
--- a/drivers/gpu/ipu-v3/ipu-ic.c
+++ b/drivers/gpu/ipu-v3/ipu-ic.c
@@ -214,6 +214,23 @@ static const struct ic_encode_coeff ic_encode_identity = {
.scale = 2,
};
+/*
+ * REC.709 encoding from RGB full range to YUV full range:
+ *
+ * Y = .2126 * R + .7152 * G + .0722 * B
+ * U = -.1146 * R - .3854 * G + .5000 * B + 128
+ * V = .5000 * R - .4542 * G - .0458 * B + 128
+ */
+static const struct ic_encode_coeff ic_encode_rgb2ycbcr_709 = {
+ .coeff = {
+ { 54, 183, 19 },
+ { 483, 413, 128 },
+ { 128, 396, 500 },
+ },
+ .offset = { 0, 512, 512 },
+ .scale = 1,
+};
+
/*
* Inverse BT.601 encoding from YUV full range to RGB full range:
*
@@ -237,11 +254,35 @@ static const struct ic_encode_coeff ic_encode_ycbcr2rgb_601 = {
.scale = 2,
};
+/*
+ * Inverse REC.709 encoding from YUV full range to RGB full range:
+ *
+ * R = 1. * Y + 0 * (Cb - 128) + 1.5748 * (Cr - 128)
+ * G = 1. * Y - .1873 * (Cb - 128) - .4681 * (Cr - 128)
+ * B = 1. * Y + 1.8556 * (Cb - 128) + 0 * (Cr - 128)
+ *
+ * equivalently (factoring out the offsets):
+ *
+ * R = 1. * Y + 0 * Cb + 1.5748 * Cr - 201.574
+ * G = 1. * Y - .1873 * Cb - .4681 * Cr + 83.891
+ * B = 1. * Y + 1.8556 * Cb + 0 * Cr - 237.517
+ */
+static const struct ic_encode_coeff ic_encode_ycbcr2rgb_709 = {
+ .coeff = {
+ { 128, 0, 202 },
+ { 128, 488, 452 },
+ { 128, 238, 0 },
+ },
+ .offset = { -403, 168, -475 },
+ .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)
{
+ const struct ic_encode_coeff *encode_coeff;
bool inverse_encode;
if (in->colorspace != out->colorspace) {
@@ -249,11 +290,6 @@ static int calc_csc_coeffs(struct ipu_ic_priv *priv,
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 &&
@@ -278,8 +314,21 @@ static int calc_csc_coeffs(struct ipu_ic_priv *priv,
inverse_encode = (in->cs == IPUV3_COLORSPACE_YUV);
- *coeff_out = inverse_encode ?
- ic_encode_ycbcr2rgb_601 : ic_encode_rgb2ycbcr_601;
+ switch (out->enc) {
+ case V4L2_YCBCR_ENC_601:
+ encode_coeff = inverse_encode ?
+ &ic_encode_ycbcr2rgb_601 : &ic_encode_rgb2ycbcr_601;
+ break;
+ case V4L2_YCBCR_ENC_709:
+ encode_coeff = inverse_encode ?
+ &ic_encode_ycbcr2rgb_709 : &ic_encode_rgb2ycbcr_709;
+ break;
+ default:
+ dev_err(priv->ipu->dev, "Unsupported YCbCr encoding\n");
+ return -ENOTSUPP;
+ }
+
+ *coeff_out = *encode_coeff;
return 0;
}
--
2.17.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v6 5/7] gpu: ipu-v3: ipu-ic: Add support for limited range encoding
[not found] <20190307233356.23748-1-slongerbeam@gmail.com>
` (3 preceding siblings ...)
2019-03-07 23:33 ` [PATCH v6 4/7] gpu: ipu-v3: ipu-ic: Add support for Rec.709 encoding Steve Longerbeam
@ 2019-03-07 23:33 ` Steve Longerbeam
2019-03-08 11:57 ` Philipp Zabel
4 siblings, 1 reply; 12+ messages in thread
From: Steve Longerbeam @ 2019-03-07 23:33 UTC (permalink / raw)
To: linux-media
Cc: Tim Harvey, Steve Longerbeam, Philipp Zabel,
open list:DRM DRIVERS FOR FREESCALE IMX, open list
Add support for the following conversions:
- YUV full-range to YUV limited-range
- YUV limited-range to YUV full-range
- YUV limited-range to RGB full-range
- RGB full-range to YUV limited-range
The last two conversions require operating on the YUV full-range
encoding and inverse encoding coefficients, with the YUV-to-YUV
limited<->full coefficients. The formula to convert is
M_c = M_a * M_b
O_c = M_a * O_b + O_a
For calculating the RGB full-range to YUV limited-range coefficients:
[M_a, O_a] = YUV full-range to YUV limited-range coefficients.
[M_b, O_b] = RGB full-range to YUV full-range coefficients.
For calculating the YUV limited-range to RGB full-range coefficients:
[M_a, O_a] = YUV full-range to RGB full-range coefficients.
[M_b, O_b] = YUV limited-range to YUV full-range coefficients.
The calculation of [M_c, O_c] is carried out by the function
transform_coeffs().
In the future if RGB limited range encoding is required, the same
function can be used. And cascaded to create all combinations of
encoding for YUV limited/full range <-> RGB limited/full range,
passing the output coefficients from one call as the input for the
next.
For example, to create YUV full-range to RGB limited-range coefficients:
[M_a, O_a] = RGB full-range to RGB limited-range coefficients.
[M_b, O_b] = YUV full-range to RGB full-range coefficients.
and that output sent as input to create YUV limited-range to RGB
limited-range coefficients:
[M_a, O_a] = YUV full-range to RGB limited-range coefficients.
[M_b, O_b] = YUV limited-range to YUV full-range coefficients.
Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
drivers/gpu/ipu-v3/ipu-ic.c | 281 +++++++++++++++++++++++++++++++++---
1 file changed, 263 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
index 1460901af9b5..a7dd85f8d832 100644
--- a/drivers/gpu/ipu-v3/ipu-ic.c
+++ b/drivers/gpu/ipu-v3/ipu-ic.c
@@ -178,10 +178,10 @@ static inline void ipu_ic_write(struct ipu_ic *ic, u32 value, unsigned offset)
}
struct ic_encode_coeff {
- s16 coeff[3][3]; /* signed 9-bit integer coefficients */
- s16 offset[3]; /* signed 11+2-bit fixed point offset */
- u8 scale:2; /* scale coefficients * 2^(scale-1) */
- bool sat:1; /* saturate to (16, 235(Y) / 240(U, V)) */
+ int coeff[3][3]; /* signed 9-bit integer coefficients */
+ int offset[3]; /* signed 13-bit integer offset */
+ int scale; /* scale coefficients * 2^(scale-1) */
+ bool sat; /* saturate to (16, 235(Y) / 240(U, V)) */
};
/*
@@ -277,6 +277,231 @@ static const struct ic_encode_coeff ic_encode_ycbcr2rgb_709 = {
.scale = 2,
};
+/*
+ * YUV full range to YUV limited range:
+ *
+ * Y_lim = 0.8588 * Y_full + 16
+ * Cb_lim = 0.8784 * (Cb_full - 128) + 128
+ * Cr_lim = 0.8784 * (Cr_full - 128) + 128
+ */
+static const struct ic_encode_coeff ic_encode_ycbcr_full2lim = {
+ .coeff = {
+ { 219, 0, 0 },
+ { 0, 224, 0 },
+ { 0, 0, 224 },
+ },
+ .offset = { 64, 62, 62 },
+ .scale = 1,
+};
+
+/*
+ * YUV limited range to YUV full range:
+ *
+ * Y_full = 1.1644 * (Y_lim - 16)
+ * Cb_full = 1.1384 * (Cb_lim - 128) + 128
+ * Cr_full = 1.1384 * (Cr_lim - 128) + 128
+ */
+static const struct ic_encode_coeff ic_encode_ycbcr_lim2full = {
+ .coeff = {
+ { 149, 0, 0 },
+ { 0, 145, 0 },
+ { 0, 0, 145 },
+ },
+ .offset = { -37, -35, -35 },
+ .scale = 2,
+};
+
+/*
+ * RGB full range to RGB limited range:
+ *
+ * R_lim = 0.8588 * R_full + 16
+ * G_lim = 0.8588 * G_full + 16
+ * B_lim = 0.8588 * B_full + 16
+ */
+static const struct ic_encode_coeff
+ic_encode_rgb_full2lim __maybe_unused = {
+ .coeff = {
+ { 220, 0, 0 },
+ { 0, 220, 0 },
+ { 0, 0, 220 },
+ },
+ .offset = { 64, 64, 64 },
+ .scale = 1,
+};
+
+/*
+ * RGB limited range to RGB full range:
+ *
+ * R_full = 1.1644 * (R_lim - 16)
+ * G_full = 1.1644 * (G_lim - 16)
+ * B_full = 1.1644 * (B_lim - 16)
+ */
+static const struct ic_encode_coeff
+ic_encode_rgb_lim2full __maybe_unused = {
+ .coeff = {
+ { 149, 0, 0 },
+ { 0, 149, 0 },
+ { 0, 0, 149 },
+ },
+ .offset = { -37, -37, -37 },
+ .scale = 2,
+};
+
+/*
+ * Convert a coefficient and scale value in TPMEM register format
+ * to a signed int times 256 (fix the radix point). The TPMEM register
+ * coefficient format is a signed 9-bit value (sign bit at bit 8,
+ * mantissa = coeff * 2 ^ (8 - scale - 1)).
+ */
+static int coeff_fix(int coeff, int scale)
+{
+ if (coeff >= 256)
+ coeff -= 512;
+ if (scale == 0)
+ return DIV_ROUND_CLOSEST(coeff, 2);
+ return coeff << (scale - 1);
+}
+
+/*
+ * Convert a signed int coefficient times 256 to TPMEM register
+ * format, given a scale value = TPMEM scale - 1.
+ */
+static int coeff_normalize(int coeff, int scale)
+{
+ coeff = DIV_ROUND_CLOSEST(coeff, 1 << scale);
+ if (coeff < 0)
+ coeff += 512;
+ return coeff;
+}
+
+/*
+ * Convert an offset and scale value in TPMEM register format to a
+ * signed int times 256 (fix the radix point). The TPMEM register
+ * offset format is a signed 13-bit value (sign bit at bit 12,
+ * mantissa = offset * 2 ^ (2 - (scale - 1)).
+ */
+static int offset_fix(int offset, int scale)
+{
+ return offset << (8 - (2 - (scale - 1)));
+}
+
+/*
+ * Convert a signed int offset times 256 to TPMEM register
+ * format, given a scale value = TPMEM scale - 1.
+ */
+static int offset_normalize(int off, int scale)
+{
+ return DIV_ROUND_CLOSEST(off, 1 << (8 - (2 - scale)));
+}
+
+/*
+ * Find the scale value that fits the given coefficient within
+ * the 8-bit TPMEM mantissa.
+ */
+static int get_coeff_scale(int coeff)
+{
+ int scale = 0;
+
+ while (abs(coeff) >= 256 && scale <= 2) {
+ coeff = DIV_ROUND_CLOSEST(coeff, 2);
+ scale++;
+ }
+
+ return scale;
+}
+
+/*
+ * The above defined encoding coefficients all encode between
+ * full-range RGB and full-range YCbCr.
+ *
+ * This function calculates a matrix M_c and offset vector O_c, given
+ * input matrices M_a, M_b and offset vectors O_a, O_b, such that:
+ *
+ * M_c = M_a * M_b
+ * O_c = M_a * O_b + O_a
+ *
+ * This operation will transform the full-range coefficients to
+ * coefficients that encode to or from limited range YCbCr or RGB.
+ *
+ * For example, to transform ic_encode_rgb2ycbcr_601 to encode to
+ * limited-range YCbCr:
+ *
+ * [M_a, O_a] = ic_encode_ycbcr_full2lim
+ * [M_b, O_b] = ic_encode_rgb2ycbcr_601
+ *
+ * To transform the inverse coefficients ic_encode_ycbcr2rgb_601 to
+ * encode from limited-range YCbCr:
+ *
+ * [M_a, O_a] = ic_encode_ycbcr2rgb_601
+ * [M_b, O_b] = ic_encode_ycbcr_lim2full
+ *
+ * The function can also be used to create RGB limited range
+ * coefficients, and cascaded to create all combinations of
+ * encodings between YCbCr limited/full range <-> RGB limited/full
+ * range.
+ */
+static void transform_coeffs(struct ic_encode_coeff *out,
+ const struct ic_encode_coeff *a,
+ const struct ic_encode_coeff *b)
+{
+ int c_a, c_b, c_out;
+ int o_a, o_b, o_out;
+ int outscale = 0;
+ int i, j, k;
+
+ for (i = 0; i < 3; i++) {
+ o_out = 0;
+ for (j = 0; j < 3; j++) {
+ int scale;
+
+ /* M_c[i,j] = M_a[i,k] * M_b[k,j] */
+ c_out = 0;
+ for (k = 0; k < 3; k++) {
+ c_a = coeff_fix(a->coeff[i][k], a->scale);
+ c_b = coeff_fix(b->coeff[k][j], b->scale);
+ c_out += c_a * c_b;
+ }
+
+ c_out = DIV_ROUND_CLOSEST(c_out, 1 << 8);
+ out->coeff[i][j] = c_out;
+
+ /*
+ * get scale for this coefficient and update
+ * final output scale.
+ */
+ scale = get_coeff_scale(c_out);
+ outscale = max(outscale, scale);
+
+ /* M_a[i,j] * O_b[j] */
+ c_a = coeff_fix(a->coeff[i][j], a->scale);
+ o_b = offset_fix(b->offset[j], b->scale);
+ o_out += DIV_ROUND_CLOSEST(c_a * o_b, 1 << 8);
+ }
+
+ /* O_c[i] = (M_a * O_b)[i] + O_a[i] */
+ o_a = offset_fix(a->offset[i], a->scale);
+ o_out += o_a;
+
+ out->offset[i] = o_out;
+ }
+
+ /*
+ * normalize output coefficients and offsets to TPMEM
+ * register format.
+ */
+ for (i = 0; i < 3; i++) {
+ for (j = 0; j < 3; j++) {
+ c_out = out->coeff[i][j];
+ out->coeff[i][j] = coeff_normalize(c_out, outscale);
+ }
+
+ o_out = out->offset[i];
+ out->offset[i] = offset_normalize(o_out, outscale);
+ }
+
+ out->scale = outscale + 1;
+}
+
static int calc_csc_coeffs(struct ipu_ic_priv *priv,
struct ic_encode_coeff *coeff_out,
const struct ipu_ic_colorspace *in,
@@ -290,14 +515,6 @@ static int calc_csc_coeffs(struct ipu_ic_priv *priv,
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 &&
@@ -307,7 +524,18 @@ static int calc_csc_coeffs(struct ipu_ic_priv *priv,
}
if (in->cs == out->cs) {
- *coeff_out = ic_encode_identity;
+ if (in->quant == out->quant) {
+ *coeff_out = ic_encode_identity;
+ } else if (in->quant == V4L2_QUANTIZATION_FULL_RANGE) {
+ /* YUV full-range to YUV limited-range */
+ *coeff_out = ic_encode_ycbcr_full2lim;
+
+ /* set saturation bit for YUV limited-range output */
+ coeff_out->sat = true;
+ } else {
+ /* YUV limited-range to YUV full-range */
+ *coeff_out = ic_encode_ycbcr_lim2full;
+ }
return 0;
}
@@ -328,7 +556,24 @@ static int calc_csc_coeffs(struct ipu_ic_priv *priv,
return -ENOTSUPP;
}
- *coeff_out = *encode_coeff;
+ if (in->quant == out->quant) {
+ /*
+ * YUV full-range to RGB full-range, or
+ * RGB full-range to YUV full-range.
+ */
+ *coeff_out = *encode_coeff;
+ } else if (inverse_encode) {
+ /* YUV limited-range to RGB full-range */
+ transform_coeffs(coeff_out, encode_coeff,
+ &ic_encode_ycbcr_lim2full);
+ } else {
+ /* RGB full-range to YUV limited-range */
+ transform_coeffs(coeff_out, &ic_encode_ycbcr_full2lim,
+ encode_coeff);
+
+ /* set saturation bit for YUV limited-range output */
+ coeff_out->sat = true;
+ }
return 0;
}
@@ -340,9 +585,9 @@ static int init_csc(struct ipu_ic *ic,
{
struct ipu_ic_priv *priv = ic->priv;
struct ic_encode_coeff coeff;
+ const unsigned int (*c)[3];
+ const unsigned int *a;
u32 __iomem *base;
- const u16 (*c)[3];
- const u16 *a;
u32 param;
int ret;
@@ -354,8 +599,8 @@ static int init_csc(struct ipu_ic *ic,
(priv->tpmem_base + ic->reg->tpmem_csc[csc_index]);
/* Cast to unsigned */
- c = (const u16 (*)[3])coeff.coeff;
- a = (const u16 *)coeff.offset;
+ c = (const unsigned int (*)[3])coeff.coeff;
+ a = (const unsigned int *)coeff.offset;
param = ((a[0] & 0x1f) << 27) | ((c[0][0] & 0x1ff) << 18) |
((c[1][1] & 0x1ff) << 9) | (c[2][2] & 0x1ff);
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v6 2/7] gpu: ipu-v3: ipu-ic: Fix BT.601 coefficients
2019-03-07 23:33 ` [PATCH v6 2/7] gpu: ipu-v3: ipu-ic: Fix BT.601 coefficients Steve Longerbeam
@ 2019-03-08 10:23 ` Philipp Zabel
2019-03-09 1:00 ` Steve Longerbeam
0 siblings, 1 reply; 12+ messages in thread
From: Philipp Zabel @ 2019-03-08 10:23 UTC (permalink / raw)
To: Steve Longerbeam, linux-media
Cc: Tim Harvey, stable, open list:DRM DRIVERS FOR FREESCALE IMX, open list
Hi Steve,
On Thu, 2019-03-07 at 15:33 -0800, Steve Longerbeam wrote:
> The ycbcr2rgb and inverse rgb2ycbcr tables define the BT.601 Y'CbCr
> encoding coefficients.
>
> The rgb2ycbcr table specifically describes the BT.601 encoding from
> full range RGB to full range YUV. Add table comments to make this more
> clear.
>
> The ycbcr2rgb inverse table describes encoding YUV limited range to RGB
> full range. To be consistent with the rgb2ycbcr table, convert this to
> YUV full range to RGB full range, and adjust/expand on the comments.
>
> The ic_csc_rgb2rgb table is just an identity matrix, so rename to
> ic_encode_identity.
>
> Fixes: 1aa8ea0d2bd5d ("gpu: ipu-v3: Add Image Converter unit")
>
> Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> Cc: stable@vger.kernel.org
> ---
> drivers/gpu/ipu-v3/ipu-ic.c | 61 ++++++++++++++++++++++---------------
> 1 file changed, 37 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
> index 18816ccf600e..b63a2826b629 100644
> --- a/drivers/gpu/ipu-v3/ipu-ic.c
> +++ b/drivers/gpu/ipu-v3/ipu-ic.c
> @@ -175,7 +175,7 @@ static inline void ipu_ic_write(struct ipu_ic *ic, u32 value, unsigned offset)
> writel(value, ic->priv->base + offset);
> }
>
> -struct ic_csc_params {
> +struct ic_encode_coeff {
This less accurate. These are called IC (Task) Parameters in the
reference manual, the 64-bit aligned words are called CSC words. Beside
the coefficients, this structure also contains the coefficient scale,
the offsets, and the saturation mode flag.
> s16 coeff[3][3]; /* signed 9-bit integer coefficients */
> s16 offset[3]; /* signed 11+2-bit fixed point offset */
> u8 scale:2; /* scale coefficients * 2^(scale-1) */
> @@ -183,13 +183,15 @@ struct ic_csc_params {
> };
>
> /*
> - * Y = R * .299 + G * .587 + B * .114;
> - * U = R * -.169 + G * -.332 + B * .500 + 128.;
> - * V = R * .500 + G * -.419 + B * -.0813 + 128.;
> + * BT.601 encoding from RGB full range to YUV full range:
> + *
> + * Y = .2990 * R + .5870 * G + .1140 * B
> + * U = -.1687 * R - .3313 * G + .5000 * B + 128
> + * V = .5000 * R - .4187 * G - .0813 * B + 128
> */
> -static const struct ic_csc_params ic_csc_rgb2ycbcr = {
> +static const struct ic_encode_coeff ic_encode_rgb2ycbcr_601 = {
> .coeff = {
> - { 77, 150, 29 },
> + { 77, 150, 29 },
> { 469, 427, 128 },
> { 128, 405, 491 },
We could subtract 512 from the negative values, to improve readability.
> },
> @@ -197,8 +199,11 @@ static const struct ic_csc_params ic_csc_rgb2ycbcr = {
> .scale = 1,
> };
>
> -/* transparent RGB->RGB matrix for graphics combining */
> -static const struct ic_csc_params ic_csc_rgb2rgb = {
> +/*
> + * identity matrix, used for transparent RGB->RGB graphics
> + * combining.
> + */
> +static const struct ic_encode_coeff ic_encode_identity = {
> .coeff = {
> { 128, 0, 0 },
> { 0, 128, 0 },
> @@ -208,17 +213,25 @@ static const struct ic_csc_params ic_csc_rgb2rgb = {
> };
>
> /*
> - * R = (1.164 * (Y - 16)) + (1.596 * (Cr - 128));
> - * G = (1.164 * (Y - 16)) - (0.392 * (Cb - 128)) - (0.813 * (Cr - 128));
> - * B = (1.164 * (Y - 16)) + (2.017 * (Cb - 128);
> + * Inverse BT.601 encoding from YUV full range to RGB full range:
> + *
> + * R = 1. * Y + 0 * (Cb - 128) + 1.4020 * (Cr - 128)
> + * G = 1. * Y - .3442 * (Cb - 128) - 0.7142 * (Cr - 128)
Should that be ^^^^^ .3441 and ^^^^^ .7141 ?
The coefficients and offsets after rounding should end up the same.
Also, let's consistently either add the leading zero, or leave it out.
> + * B = 1. * Y + 1.7720 * (Cb - 128) + 0 * (Cr - 128)
> + *
> + * equivalently (factoring out the offsets):
> + *
> + * R = 1. * Y + 0 * Cb + 1.4020 * Cr - 179.456
> + * G = 1. * Y - .3442 * Cb - 0.7142 * Cr + 135.475
> + * B = 1. * Y + 1.7720 * Cb + 0 * Cr - 226.816
> */
> -static const struct ic_csc_params ic_csc_ycbcr2rgb = {
> +static const struct ic_encode_coeff ic_encode_ycbcr2rgb_601 = {
> .coeff = {
> - { 149, 0, 204 },
> - { 149, 462, 408 },
> - { 149, 255, 0 },
> + { 128, 0, 179 },
> + { 128, 468, 421 },
> + { 128, 227, 0 },
> },
> - .offset = { -446, 266, -554 },
> + .offset = { -359, 271, -454 },
These seem to be correct. Again, I think this would be easier to read if
the negative coefficients were written with a sign as well.
> .scale = 2,
> };
>
> @@ -228,7 +241,7 @@ static int init_csc(struct ipu_ic *ic,
> int csc_index)
> {
> struct ipu_ic_priv *priv = ic->priv;
> - const struct ic_csc_params *params;
> + const struct ic_encode_coeff *coeff;
> u32 __iomem *base;
> const u16 (*c)[3];
> const u16 *a;
> @@ -238,26 +251,26 @@ static int init_csc(struct ipu_ic *ic,
> (priv->tpmem_base + ic->reg->tpmem_csc[csc_index]);
>
> if (inf == IPUV3_COLORSPACE_YUV && outf == IPUV3_COLORSPACE_RGB)
> - params = &ic_csc_ycbcr2rgb;
> + coeff = &ic_encode_ycbcr2rgb_601;
> else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_YUV)
> - params = &ic_csc_rgb2ycbcr;
> + coeff = &ic_encode_rgb2ycbcr_601;
> else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_RGB)
> - params = &ic_csc_rgb2rgb;
> + coeff = &ic_encode_identity;
> else {
> dev_err(priv->ipu->dev, "Unsupported color space conversion\n");
> return -EINVAL;
> }
>
> /* Cast to unsigned */
> - c = (const u16 (*)[3])params->coeff;
> - a = (const u16 *)params->offset;
> + c = (const u16 (*)[3])coeff->coeff;
> + a = (const u16 *)coeff->offset;
This looks weird to me. I'd be in favor of not renaming the type.
regards
Philipp
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6 1/7] gpu: ipu-v3: ipu-ic: Fix saturation bit offset in TPMEM
2019-03-07 23:33 ` [PATCH v6 1/7] gpu: ipu-v3: ipu-ic: Fix saturation bit offset in TPMEM Steve Longerbeam
@ 2019-03-08 10:24 ` Philipp Zabel
0 siblings, 0 replies; 12+ messages in thread
From: Philipp Zabel @ 2019-03-08 10:24 UTC (permalink / raw)
To: Steve Longerbeam, linux-media
Cc: Tim Harvey, stable, open list:DRM DRIVERS FOR FREESCALE IMX, open list
On Thu, 2019-03-07 at 15:33 -0800, Steve Longerbeam wrote:
> The saturation bit was being set at bit 9 in the second 32-bit word
> of the TPMEM CSC. This isn't correct, the saturation bit is bit 42,
> which is bit 10 of the second word.
>
> Fixes: 1aa8ea0d2bd5d ("gpu: ipu-v3: Add Image Converter unit")
>
> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> Cc: stable@vger.kernel.org
> ---
> drivers/gpu/ipu-v3/ipu-ic.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
> index 594c3cbc8291..18816ccf600e 100644
> --- a/drivers/gpu/ipu-v3/ipu-ic.c
> +++ b/drivers/gpu/ipu-v3/ipu-ic.c
> @@ -257,7 +257,7 @@ static int init_csc(struct ipu_ic *ic,
> writel(param, base++);
>
> param = ((a[0] & 0x1fe0) >> 5) | (params->scale << 8) |
> - (params->sat << 9);
> + (params->sat << 10);
> writel(param, base++);
>
> param = ((a[1] & 0x1f) << 27) | ((c[0][1] & 0x1ff) << 18) |
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
regards
Philipp
^ permalink raw reply [flat|nested] 12+ 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; 12+ 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] 12+ messages in thread
* Re: [PATCH v6 5/7] gpu: ipu-v3: ipu-ic: Add support for limited range encoding
2019-03-07 23:33 ` [PATCH v6 5/7] gpu: ipu-v3: ipu-ic: Add support for limited range encoding Steve Longerbeam
@ 2019-03-08 11:57 ` Philipp Zabel
2019-03-09 1:26 ` Steve Longerbeam
0 siblings, 1 reply; 12+ messages in thread
From: Philipp Zabel @ 2019-03-08 11:57 UTC (permalink / raw)
To: Steve Longerbeam, linux-media
Cc: Tim Harvey, open list:DRM DRIVERS FOR FREESCALE IMX, open list
On Thu, 2019-03-07 at 15:33 -0800, Steve Longerbeam wrote:
> Add support for the following conversions:
>
> - YUV full-range to YUV limited-range
> - YUV limited-range to YUV full-range
> - YUV limited-range to RGB full-range
> - RGB full-range to YUV limited-range
>
> The last two conversions require operating on the YUV full-range
> encoding and inverse encoding coefficients, with the YUV-to-YUV
> limited<->full coefficients. The formula to convert is
>
> M_c = M_a * M_b
> O_c = M_a * O_b + O_a
>
> For calculating the RGB full-range to YUV limited-range coefficients:
>
> [M_a, O_a] = YUV full-range to YUV limited-range coefficients.
> [M_b, O_b] = RGB full-range to YUV full-range coefficients.
>
> For calculating the YUV limited-range to RGB full-range coefficients:
>
> [M_a, O_a] = YUV full-range to RGB full-range coefficients.
> [M_b, O_b] = YUV limited-range to YUV full-range coefficients.
>
> The calculation of [M_c, O_c] is carried out by the function
> transform_coeffs().
>
> In the future if RGB limited range encoding is required, the same
> function can be used. And cascaded to create all combinations of
> encoding for YUV limited/full range <-> RGB limited/full range,
> passing the output coefficients from one call as the input for the
> next.
>
> For example, to create YUV full-range to RGB limited-range coefficients:
>
> [M_a, O_a] = RGB full-range to RGB limited-range coefficients.
> [M_b, O_b] = YUV full-range to RGB full-range coefficients.
>
> and that output sent as input to create YUV limited-range to RGB
> limited-range coefficients:
>
> [M_a, O_a] = YUV full-range to RGB limited-range coefficients.
> [M_b, O_b] = YUV limited-range to YUV full-range coefficients.
>
> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
I'm not a big fan of this. Wouldn't it be much easier to compute all
necessary task parameter sets offline with high precision, and store the
precomputed sets in the compact representation?
regards
Philipp
> ---
> drivers/gpu/ipu-v3/ipu-ic.c | 281 +++++++++++++++++++++++++++++++++---
> 1 file changed, 263 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
> index 1460901af9b5..a7dd85f8d832 100644
> --- a/drivers/gpu/ipu-v3/ipu-ic.c
> +++ b/drivers/gpu/ipu-v3/ipu-ic.c
> @@ -178,10 +178,10 @@ static inline void ipu_ic_write(struct ipu_ic *ic, u32 value, unsigned offset)
> }
>
> struct ic_encode_coeff {
> - s16 coeff[3][3]; /* signed 9-bit integer coefficients */
> - s16 offset[3]; /* signed 11+2-bit fixed point offset */
> - u8 scale:2; /* scale coefficients * 2^(scale-1) */
> - bool sat:1; /* saturate to (16, 235(Y) / 240(U, V)) */
> + int coeff[3][3]; /* signed 9-bit integer coefficients */
> + int offset[3]; /* signed 13-bit integer offset */
> + int scale; /* scale coefficients * 2^(scale-1) */
> + bool sat; /* saturate to (16, 235(Y) / 240(U, V)) */
> };
>
> /*
> @@ -277,6 +277,231 @@ static const struct ic_encode_coeff ic_encode_ycbcr2rgb_709 = {
> .scale = 2,
> };
>
> +/*
> + * YUV full range to YUV limited range:
> + *
> + * Y_lim = 0.8588 * Y_full + 16
> + * Cb_lim = 0.8784 * (Cb_full - 128) + 128
> + * Cr_lim = 0.8784 * (Cr_full - 128) + 128
> + */
> +static const struct ic_encode_coeff ic_encode_ycbcr_full2lim = {
> + .coeff = {
> + { 219, 0, 0 },
> + { 0, 224, 0 },
> + { 0, 0, 224 },
> + },
> + .offset = { 64, 62, 62 },
> + .scale = 1,
> +};
> +
> +/*
> + * YUV limited range to YUV full range:
> + *
> + * Y_full = 1.1644 * (Y_lim - 16)
> + * Cb_full = 1.1384 * (Cb_lim - 128) + 128
> + * Cr_full = 1.1384 * (Cr_lim - 128) + 128
> + */
> +static const struct ic_encode_coeff ic_encode_ycbcr_lim2full = {
> + .coeff = {
> + { 149, 0, 0 },
> + { 0, 145, 0 },
> + { 0, 0, 145 },
> + },
> + .offset = { -37, -35, -35 },
> + .scale = 2,
> +};
> +
> +/*
> + * RGB full range to RGB limited range:
> + *
> + * R_lim = 0.8588 * R_full + 16
> + * G_lim = 0.8588 * G_full + 16
> + * B_lim = 0.8588 * B_full + 16
> + */
> +static const struct ic_encode_coeff
> +ic_encode_rgb_full2lim __maybe_unused = {
> + .coeff = {
> + { 220, 0, 0 },
> + { 0, 220, 0 },
> + { 0, 0, 220 },
> + },
> + .offset = { 64, 64, 64 },
> + .scale = 1,
> +};
> +
> +/*
> + * RGB limited range to RGB full range:
> + *
> + * R_full = 1.1644 * (R_lim - 16)
> + * G_full = 1.1644 * (G_lim - 16)
> + * B_full = 1.1644 * (B_lim - 16)
> + */
> +static const struct ic_encode_coeff
> +ic_encode_rgb_lim2full __maybe_unused = {
> + .coeff = {
> + { 149, 0, 0 },
> + { 0, 149, 0 },
> + { 0, 0, 149 },
> + },
> + .offset = { -37, -37, -37 },
> + .scale = 2,
> +};
> +
> +/*
> + * Convert a coefficient and scale value in TPMEM register format
> + * to a signed int times 256 (fix the radix point). The TPMEM register
> + * coefficient format is a signed 9-bit value (sign bit at bit 8,
> + * mantissa = coeff * 2 ^ (8 - scale - 1)).
> + */
> +static int coeff_fix(int coeff, int scale)
> +{
> + if (coeff >= 256)
> + coeff -= 512;
> + if (scale == 0)
> + return DIV_ROUND_CLOSEST(coeff, 2);
> + return coeff << (scale - 1);
> +}
> +
> +/*
> + * Convert a signed int coefficient times 256 to TPMEM register
> + * format, given a scale value = TPMEM scale - 1.
> + */
> +static int coeff_normalize(int coeff, int scale)
> +{
> + coeff = DIV_ROUND_CLOSEST(coeff, 1 << scale);
> + if (coeff < 0)
> + coeff += 512;
> + return coeff;
> +}
> +
> +/*
> + * Convert an offset and scale value in TPMEM register format to a
> + * signed int times 256 (fix the radix point). The TPMEM register
> + * offset format is a signed 13-bit value (sign bit at bit 12,
> + * mantissa = offset * 2 ^ (2 - (scale - 1)).
> + */
> +static int offset_fix(int offset, int scale)
> +{
> + return offset << (8 - (2 - (scale - 1)));
> +}
> +
> +/*
> + * Convert a signed int offset times 256 to TPMEM register
> + * format, given a scale value = TPMEM scale - 1.
> + */
> +static int offset_normalize(int off, int scale)
> +{
> + return DIV_ROUND_CLOSEST(off, 1 << (8 - (2 - scale)));
> +}
> +
> +/*
> + * Find the scale value that fits the given coefficient within
> + * the 8-bit TPMEM mantissa.
> + */
> +static int get_coeff_scale(int coeff)
> +{
> + int scale = 0;
> +
> + while (abs(coeff) >= 256 && scale <= 2) {
> + coeff = DIV_ROUND_CLOSEST(coeff, 2);
> + scale++;
> + }
> +
> + return scale;
> +}
> +
> +/*
> + * The above defined encoding coefficients all encode between
> + * full-range RGB and full-range YCbCr.
> + *
> + * This function calculates a matrix M_c and offset vector O_c, given
> + * input matrices M_a, M_b and offset vectors O_a, O_b, such that:
> + *
> + * M_c = M_a * M_b
> + * O_c = M_a * O_b + O_a
> + *
> + * This operation will transform the full-range coefficients to
> + * coefficients that encode to or from limited range YCbCr or RGB.
> + *
> + * For example, to transform ic_encode_rgb2ycbcr_601 to encode to
> + * limited-range YCbCr:
> + *
> + * [M_a, O_a] = ic_encode_ycbcr_full2lim
> + * [M_b, O_b] = ic_encode_rgb2ycbcr_601
> + *
> + * To transform the inverse coefficients ic_encode_ycbcr2rgb_601 to
> + * encode from limited-range YCbCr:
> + *
> + * [M_a, O_a] = ic_encode_ycbcr2rgb_601
> + * [M_b, O_b] = ic_encode_ycbcr_lim2full
> + *
> + * The function can also be used to create RGB limited range
> + * coefficients, and cascaded to create all combinations of
> + * encodings between YCbCr limited/full range <-> RGB limited/full
> + * range.
> + */
> +static void transform_coeffs(struct ic_encode_coeff *out,
> + const struct ic_encode_coeff *a,
> + const struct ic_encode_coeff *b)
> +{
> + int c_a, c_b, c_out;
> + int o_a, o_b, o_out;
> + int outscale = 0;
> + int i, j, k;
> +
> + for (i = 0; i < 3; i++) {
> + o_out = 0;
> + for (j = 0; j < 3; j++) {
> + int scale;
> +
> + /* M_c[i,j] = M_a[i,k] * M_b[k,j] */
> + c_out = 0;
> + for (k = 0; k < 3; k++) {
> + c_a = coeff_fix(a->coeff[i][k], a->scale);
> + c_b = coeff_fix(b->coeff[k][j], b->scale);
> + c_out += c_a * c_b;
> + }
> +
> + c_out = DIV_ROUND_CLOSEST(c_out, 1 << 8);
> + out->coeff[i][j] = c_out;
> +
> + /*
> + * get scale for this coefficient and update
> + * final output scale.
> + */
> + scale = get_coeff_scale(c_out);
> + outscale = max(outscale, scale);
> +
> + /* M_a[i,j] * O_b[j] */
> + c_a = coeff_fix(a->coeff[i][j], a->scale);
> + o_b = offset_fix(b->offset[j], b->scale);
> + o_out += DIV_ROUND_CLOSEST(c_a * o_b, 1 << 8);
> + }
> +
> + /* O_c[i] = (M_a * O_b)[i] + O_a[i] */
> + o_a = offset_fix(a->offset[i], a->scale);
> + o_out += o_a;
> +
> + out->offset[i] = o_out;
> + }
> +
> + /*
> + * normalize output coefficients and offsets to TPMEM
> + * register format.
> + */
> + for (i = 0; i < 3; i++) {
> + for (j = 0; j < 3; j++) {
> + c_out = out->coeff[i][j];
> + out->coeff[i][j] = coeff_normalize(c_out, outscale);
> + }
> +
> + o_out = out->offset[i];
> + out->offset[i] = offset_normalize(o_out, outscale);
> + }
> +
> + out->scale = outscale + 1;
> +}
> +
> static int calc_csc_coeffs(struct ipu_ic_priv *priv,
> struct ic_encode_coeff *coeff_out,
> const struct ipu_ic_colorspace *in,
> @@ -290,14 +515,6 @@ static int calc_csc_coeffs(struct ipu_ic_priv *priv,
> 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 &&
> @@ -307,7 +524,18 @@ static int calc_csc_coeffs(struct ipu_ic_priv *priv,
> }
>
> if (in->cs == out->cs) {
> - *coeff_out = ic_encode_identity;
> + if (in->quant == out->quant) {
> + *coeff_out = ic_encode_identity;
> + } else if (in->quant == V4L2_QUANTIZATION_FULL_RANGE) {
> + /* YUV full-range to YUV limited-range */
> + *coeff_out = ic_encode_ycbcr_full2lim;
> +
> + /* set saturation bit for YUV limited-range output */
> + coeff_out->sat = true;
> + } else {
> + /* YUV limited-range to YUV full-range */
> + *coeff_out = ic_encode_ycbcr_lim2full;
> + }
>
> return 0;
> }
> @@ -328,7 +556,24 @@ static int calc_csc_coeffs(struct ipu_ic_priv *priv,
> return -ENOTSUPP;
> }
>
> - *coeff_out = *encode_coeff;
> + if (in->quant == out->quant) {
> + /*
> + * YUV full-range to RGB full-range, or
> + * RGB full-range to YUV full-range.
> + */
> + *coeff_out = *encode_coeff;
> + } else if (inverse_encode) {
> + /* YUV limited-range to RGB full-range */
> + transform_coeffs(coeff_out, encode_coeff,
> + &ic_encode_ycbcr_lim2full);
> + } else {
> + /* RGB full-range to YUV limited-range */
> + transform_coeffs(coeff_out, &ic_encode_ycbcr_full2lim,
> + encode_coeff);
> +
> + /* set saturation bit for YUV limited-range output */
> + coeff_out->sat = true;
> + }
>
> return 0;
> }
> @@ -340,9 +585,9 @@ static int init_csc(struct ipu_ic *ic,
> {
> struct ipu_ic_priv *priv = ic->priv;
> struct ic_encode_coeff coeff;
> + const unsigned int (*c)[3];
> + const unsigned int *a;
> u32 __iomem *base;
> - const u16 (*c)[3];
> - const u16 *a;
> u32 param;
> int ret;
>
> @@ -354,8 +599,8 @@ static int init_csc(struct ipu_ic *ic,
> (priv->tpmem_base + ic->reg->tpmem_csc[csc_index]);
>
> /* Cast to unsigned */
> - c = (const u16 (*)[3])coeff.coeff;
> - a = (const u16 *)coeff.offset;
> + c = (const unsigned int (*)[3])coeff.coeff;
> + a = (const unsigned int *)coeff.offset;
>
> param = ((a[0] & 0x1f) << 27) | ((c[0][0] & 0x1ff) << 18) |
> ((c[1][1] & 0x1ff) << 9) | (c[2][2] & 0x1ff);
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6 2/7] gpu: ipu-v3: ipu-ic: Fix BT.601 coefficients
2019-03-08 10:23 ` Philipp Zabel
@ 2019-03-09 1:00 ` Steve Longerbeam
0 siblings, 0 replies; 12+ messages in thread
From: Steve Longerbeam @ 2019-03-09 1:00 UTC (permalink / raw)
To: Philipp Zabel, linux-media
Cc: Tim Harvey, stable, open list:DRM DRIVERS FOR FREESCALE IMX, open list
On 3/8/19 2:23 AM, Philipp Zabel wrote:
> Hi Steve,
>
> On Thu, 2019-03-07 at 15:33 -0800, Steve Longerbeam wrote:
>> The ycbcr2rgb and inverse rgb2ycbcr tables define the BT.601 Y'CbCr
>> encoding coefficients.
>>
>> The rgb2ycbcr table specifically describes the BT.601 encoding from
>> full range RGB to full range YUV. Add table comments to make this more
>> clear.
>>
>> The ycbcr2rgb inverse table describes encoding YUV limited range to RGB
>> full range. To be consistent with the rgb2ycbcr table, convert this to
>> YUV full range to RGB full range, and adjust/expand on the comments.
>>
>> The ic_csc_rgb2rgb table is just an identity matrix, so rename to
>> ic_encode_identity.
>>
>> Fixes: 1aa8ea0d2bd5d ("gpu: ipu-v3: Add Image Converter unit")
>>
>> Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
>> Cc: stable@vger.kernel.org
>> ---
>> drivers/gpu/ipu-v3/ipu-ic.c | 61 ++++++++++++++++++++++---------------
>> 1 file changed, 37 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
>> index 18816ccf600e..b63a2826b629 100644
>> --- a/drivers/gpu/ipu-v3/ipu-ic.c
>> +++ b/drivers/gpu/ipu-v3/ipu-ic.c
>> @@ -175,7 +175,7 @@ static inline void ipu_ic_write(struct ipu_ic *ic, u32 value, unsigned offset)
>> writel(value, ic->priv->base + offset);
>> }
>>
>> -struct ic_csc_params {
>> +struct ic_encode_coeff {
> This less accurate. These are called IC (Task) Parameters in the
> reference manual, the 64-bit aligned words are called CSC words. Beside
> the coefficients, this structure also contains the coefficient scale,
> the offsets, and the saturation mode flag.
It seemed to me the renaming was more clear, but I agree the former name
conforms better to the manual nomenclature. I will revert this renaming.
>
>> s16 coeff[3][3]; /* signed 9-bit integer coefficients */
>> s16 offset[3]; /* signed 11+2-bit fixed point offset */
>> u8 scale:2; /* scale coefficients * 2^(scale-1) */
>> @@ -183,13 +183,15 @@ struct ic_csc_params {
>> };
>>
>> /*
>> - * Y = R * .299 + G * .587 + B * .114;
>> - * U = R * -.169 + G * -.332 + B * .500 + 128.;
>> - * V = R * .500 + G * -.419 + B * -.0813 + 128.;
>> + * BT.601 encoding from RGB full range to YUV full range:
>> + *
>> + * Y = .2990 * R + .5870 * G + .1140 * B
>> + * U = -.1687 * R - .3313 * G + .5000 * B + 128
>> + * V = .5000 * R - .4187 * G - .0813 * B + 128
>> */
>> -static const struct ic_csc_params ic_csc_rgb2ycbcr = {
>> +static const struct ic_encode_coeff ic_encode_rgb2ycbcr_601 = {
>> .coeff = {
>> - { 77, 150, 29 },
>> + { 77, 150, 29 },
>> { 469, 427, 128 },
>> { 128, 405, 491 },
> We could subtract 512 from the negative values, to improve readability.
Agreed.
>
>> },
>> @@ -197,8 +199,11 @@ static const struct ic_csc_params ic_csc_rgb2ycbcr = {
>> .scale = 1,
>> };
>>
>> -/* transparent RGB->RGB matrix for graphics combining */
>> -static const struct ic_csc_params ic_csc_rgb2rgb = {
>> +/*
>> + * identity matrix, used for transparent RGB->RGB graphics
>> + * combining.
>> + */
>> +static const struct ic_encode_coeff ic_encode_identity = {
>> .coeff = {
>> { 128, 0, 0 },
>> { 0, 128, 0 },
>> @@ -208,17 +213,25 @@ static const struct ic_csc_params ic_csc_rgb2rgb = {
>> };
>>
>> /*
>> - * R = (1.164 * (Y - 16)) + (1.596 * (Cr - 128));
>> - * G = (1.164 * (Y - 16)) - (0.392 * (Cb - 128)) - (0.813 * (Cr - 128));
>> - * B = (1.164 * (Y - 16)) + (2.017 * (Cb - 128);
>> + * Inverse BT.601 encoding from YUV full range to RGB full range:
>> + *
>> + * R = 1. * Y + 0 * (Cb - 128) + 1.4020 * (Cr - 128)
>> + * G = 1. * Y - .3442 * (Cb - 128) - 0.7142 * (Cr - 128)
> Should that be ^^^^^ .3441 and ^^^^^ .7141 ?
> The coefficients and offsets after rounding should end up the same.
Ok.
>
> Also, let's consistently either add the leading zero, or leave it out.
Yes.
>
>> + * B = 1. * Y + 1.7720 * (Cb - 128) + 0 * (Cr - 128)
>> + *
>> + * equivalently (factoring out the offsets):
>> + *
>> + * R = 1. * Y + 0 * Cb + 1.4020 * Cr - 179.456
>> + * G = 1. * Y - .3442 * Cb - 0.7142 * Cr + 135.475
>> + * B = 1. * Y + 1.7720 * Cb + 0 * Cr - 226.816
>> */
>> -static const struct ic_csc_params ic_csc_ycbcr2rgb = {
>> +static const struct ic_encode_coeff ic_encode_ycbcr2rgb_601 = {
>> .coeff = {
>> - { 149, 0, 204 },
>> - { 149, 462, 408 },
>> - { 149, 255, 0 },
>> + { 128, 0, 179 },
>> + { 128, 468, 421 },
>> + { 128, 227, 0 },
>> },
>> - .offset = { -446, 266, -554 },
>> + .offset = { -359, 271, -454 },
> These seem to be correct. Again, I think this would be easier to read if
> the negative coefficients were written with a sign as well.
>
>> .scale = 2,
>> };
>>
>> @@ -228,7 +241,7 @@ static int init_csc(struct ipu_ic *ic,
>> int csc_index)
>> {
>> struct ipu_ic_priv *priv = ic->priv;
>> - const struct ic_csc_params *params;
>> + const struct ic_encode_coeff *coeff;
>> u32 __iomem *base;
>> const u16 (*c)[3];
>> const u16 *a;
>> @@ -238,26 +251,26 @@ static int init_csc(struct ipu_ic *ic,
>> (priv->tpmem_base + ic->reg->tpmem_csc[csc_index]);
>>
>> if (inf == IPUV3_COLORSPACE_YUV && outf == IPUV3_COLORSPACE_RGB)
>> - params = &ic_csc_ycbcr2rgb;
>> + coeff = &ic_encode_ycbcr2rgb_601;
>> else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_YUV)
>> - params = &ic_csc_rgb2ycbcr;
>> + coeff = &ic_encode_rgb2ycbcr_601;
>> else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_RGB)
>> - params = &ic_csc_rgb2rgb;
>> + coeff = &ic_encode_identity;
>> else {
>> dev_err(priv->ipu->dev, "Unsupported color space conversion\n");
>> return -EINVAL;
>> }
>>
>> /* Cast to unsigned */
>> - c = (const u16 (*)[3])params->coeff;
>> - a = (const u16 *)params->offset;
>> + c = (const u16 (*)[3])coeff->coeff;
>> + a = (const u16 *)coeff->offset;
> This looks weird to me. I'd be in favor of not renaming the type.
Ok.
Steve
^ permalink raw reply [flat|nested] 12+ 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; 12+ 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6 5/7] gpu: ipu-v3: ipu-ic: Add support for limited range encoding
2019-03-08 11:57 ` Philipp Zabel
@ 2019-03-09 1:26 ` Steve Longerbeam
0 siblings, 0 replies; 12+ messages in thread
From: Steve Longerbeam @ 2019-03-09 1:26 UTC (permalink / raw)
To: Philipp Zabel, linux-media
Cc: open list, open list:DRM DRIVERS FOR FREESCALE IMX
On 3/8/19 3:57 AM, Philipp Zabel wrote:
> On Thu, 2019-03-07 at 15:33 -0800, Steve Longerbeam wrote:
>> Add support for the following conversions:
>>
>> - YUV full-range to YUV limited-range
>> - YUV limited-range to YUV full-range
>> - YUV limited-range to RGB full-range
>> - RGB full-range to YUV limited-range
>>
>> The last two conversions require operating on the YUV full-range
>> encoding and inverse encoding coefficients, with the YUV-to-YUV
>> limited<->full coefficients. The formula to convert is
>>
>> M_c = M_a * M_b
>> O_c = M_a * O_b + O_a
>>
>> For calculating the RGB full-range to YUV limited-range coefficients:
>>
>> [M_a, O_a] = YUV full-range to YUV limited-range coefficients.
>> [M_b, O_b] = RGB full-range to YUV full-range coefficients.
>>
>> For calculating the YUV limited-range to RGB full-range coefficients:
>>
>> [M_a, O_a] = YUV full-range to RGB full-range coefficients.
>> [M_b, O_b] = YUV limited-range to YUV full-range coefficients.
>>
>> The calculation of [M_c, O_c] is carried out by the function
>> transform_coeffs().
>>
>> In the future if RGB limited range encoding is required, the same
>> function can be used. And cascaded to create all combinations of
>> encoding for YUV limited/full range <-> RGB limited/full range,
>> passing the output coefficients from one call as the input for the
>> next.
>>
>> For example, to create YUV full-range to RGB limited-range coefficients:
>>
>> [M_a, O_a] = RGB full-range to RGB limited-range coefficients.
>> [M_b, O_b] = YUV full-range to RGB full-range coefficients.
>>
>> and that output sent as input to create YUV limited-range to RGB
>> limited-range coefficients:
>>
>> [M_a, O_a] = YUV full-range to RGB limited-range coefficients.
>> [M_b, O_b] = YUV limited-range to YUV full-range coefficients.
>>
>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> I'm not a big fan of this. Wouldn't it be much easier to compute all
> necessary task parameter sets offline with high precision, and store the
> precomputed sets in the compact representation?
I am thinking of when support might be added for the other encoding
standards. With this transform function, only two new task parameter
structs need to be added, one for yuv-full-to-rgb-full, and one for
rgb-full-to-yuv-full. Without transform_coeffs(), four structs would
have to be added (adding encoding to and from yuv-limited). And if
rgb-limited support is added, it would mean a total of eight new structs
for a new encoding standard. But with transform_coeffs(), still only the
two structs above are needed, and the function would compute the others
automatically in runtime.
Steve
>
>
>> ---
>> drivers/gpu/ipu-v3/ipu-ic.c | 281 +++++++++++++++++++++++++++++++++---
>> 1 file changed, 263 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
>> index 1460901af9b5..a7dd85f8d832 100644
>> --- a/drivers/gpu/ipu-v3/ipu-ic.c
>> +++ b/drivers/gpu/ipu-v3/ipu-ic.c
>> @@ -178,10 +178,10 @@ static inline void ipu_ic_write(struct ipu_ic *ic, u32 value, unsigned offset)
>> }
>>
>> struct ic_encode_coeff {
>> - s16 coeff[3][3]; /* signed 9-bit integer coefficients */
>> - s16 offset[3]; /* signed 11+2-bit fixed point offset */
>> - u8 scale:2; /* scale coefficients * 2^(scale-1) */
>> - bool sat:1; /* saturate to (16, 235(Y) / 240(U, V)) */
>> + int coeff[3][3]; /* signed 9-bit integer coefficients */
>> + int offset[3]; /* signed 13-bit integer offset */
>> + int scale; /* scale coefficients * 2^(scale-1) */
>> + bool sat; /* saturate to (16, 235(Y) / 240(U, V)) */
>> };
>>
>> /*
>> @@ -277,6 +277,231 @@ static const struct ic_encode_coeff ic_encode_ycbcr2rgb_709 = {
>> .scale = 2,
>> };
>>
>> +/*
>> + * YUV full range to YUV limited range:
>> + *
>> + * Y_lim = 0.8588 * Y_full + 16
>> + * Cb_lim = 0.8784 * (Cb_full - 128) + 128
>> + * Cr_lim = 0.8784 * (Cr_full - 128) + 128
>> + */
>> +static const struct ic_encode_coeff ic_encode_ycbcr_full2lim = {
>> + .coeff = {
>> + { 219, 0, 0 },
>> + { 0, 224, 0 },
>> + { 0, 0, 224 },
>> + },
>> + .offset = { 64, 62, 62 },
>> + .scale = 1,
>> +};
>> +
>> +/*
>> + * YUV limited range to YUV full range:
>> + *
>> + * Y_full = 1.1644 * (Y_lim - 16)
>> + * Cb_full = 1.1384 * (Cb_lim - 128) + 128
>> + * Cr_full = 1.1384 * (Cr_lim - 128) + 128
>> + */
>> +static const struct ic_encode_coeff ic_encode_ycbcr_lim2full = {
>> + .coeff = {
>> + { 149, 0, 0 },
>> + { 0, 145, 0 },
>> + { 0, 0, 145 },
>> + },
>> + .offset = { -37, -35, -35 },
>> + .scale = 2,
>> +};
>> +
>> +/*
>> + * RGB full range to RGB limited range:
>> + *
>> + * R_lim = 0.8588 * R_full + 16
>> + * G_lim = 0.8588 * G_full + 16
>> + * B_lim = 0.8588 * B_full + 16
>> + */
>> +static const struct ic_encode_coeff
>> +ic_encode_rgb_full2lim __maybe_unused = {
>> + .coeff = {
>> + { 220, 0, 0 },
>> + { 0, 220, 0 },
>> + { 0, 0, 220 },
>> + },
>> + .offset = { 64, 64, 64 },
>> + .scale = 1,
>> +};
>> +
>> +/*
>> + * RGB limited range to RGB full range:
>> + *
>> + * R_full = 1.1644 * (R_lim - 16)
>> + * G_full = 1.1644 * (G_lim - 16)
>> + * B_full = 1.1644 * (B_lim - 16)
>> + */
>> +static const struct ic_encode_coeff
>> +ic_encode_rgb_lim2full __maybe_unused = {
>> + .coeff = {
>> + { 149, 0, 0 },
>> + { 0, 149, 0 },
>> + { 0, 0, 149 },
>> + },
>> + .offset = { -37, -37, -37 },
>> + .scale = 2,
>> +};
>> +
>> +/*
>> + * Convert a coefficient and scale value in TPMEM register format
>> + * to a signed int times 256 (fix the radix point). The TPMEM register
>> + * coefficient format is a signed 9-bit value (sign bit at bit 8,
>> + * mantissa = coeff * 2 ^ (8 - scale - 1)).
>> + */
>> +static int coeff_fix(int coeff, int scale)
>> +{
>> + if (coeff >= 256)
>> + coeff -= 512;
>> + if (scale == 0)
>> + return DIV_ROUND_CLOSEST(coeff, 2);
>> + return coeff << (scale - 1);
>> +}
>> +
>> +/*
>> + * Convert a signed int coefficient times 256 to TPMEM register
>> + * format, given a scale value = TPMEM scale - 1.
>> + */
>> +static int coeff_normalize(int coeff, int scale)
>> +{
>> + coeff = DIV_ROUND_CLOSEST(coeff, 1 << scale);
>> + if (coeff < 0)
>> + coeff += 512;
>> + return coeff;
>> +}
>> +
>> +/*
>> + * Convert an offset and scale value in TPMEM register format to a
>> + * signed int times 256 (fix the radix point). The TPMEM register
>> + * offset format is a signed 13-bit value (sign bit at bit 12,
>> + * mantissa = offset * 2 ^ (2 - (scale - 1)).
>> + */
>> +static int offset_fix(int offset, int scale)
>> +{
>> + return offset << (8 - (2 - (scale - 1)));
>> +}
>> +
>> +/*
>> + * Convert a signed int offset times 256 to TPMEM register
>> + * format, given a scale value = TPMEM scale - 1.
>> + */
>> +static int offset_normalize(int off, int scale)
>> +{
>> + return DIV_ROUND_CLOSEST(off, 1 << (8 - (2 - scale)));
>> +}
>> +
>> +/*
>> + * Find the scale value that fits the given coefficient within
>> + * the 8-bit TPMEM mantissa.
>> + */
>> +static int get_coeff_scale(int coeff)
>> +{
>> + int scale = 0;
>> +
>> + while (abs(coeff) >= 256 && scale <= 2) {
>> + coeff = DIV_ROUND_CLOSEST(coeff, 2);
>> + scale++;
>> + }
>> +
>> + return scale;
>> +}
>> +
>> +/*
>> + * The above defined encoding coefficients all encode between
>> + * full-range RGB and full-range YCbCr.
>> + *
>> + * This function calculates a matrix M_c and offset vector O_c, given
>> + * input matrices M_a, M_b and offset vectors O_a, O_b, such that:
>> + *
>> + * M_c = M_a * M_b
>> + * O_c = M_a * O_b + O_a
>> + *
>> + * This operation will transform the full-range coefficients to
>> + * coefficients that encode to or from limited range YCbCr or RGB.
>> + *
>> + * For example, to transform ic_encode_rgb2ycbcr_601 to encode to
>> + * limited-range YCbCr:
>> + *
>> + * [M_a, O_a] = ic_encode_ycbcr_full2lim
>> + * [M_b, O_b] = ic_encode_rgb2ycbcr_601
>> + *
>> + * To transform the inverse coefficients ic_encode_ycbcr2rgb_601 to
>> + * encode from limited-range YCbCr:
>> + *
>> + * [M_a, O_a] = ic_encode_ycbcr2rgb_601
>> + * [M_b, O_b] = ic_encode_ycbcr_lim2full
>> + *
>> + * The function can also be used to create RGB limited range
>> + * coefficients, and cascaded to create all combinations of
>> + * encodings between YCbCr limited/full range <-> RGB limited/full
>> + * range.
>> + */
>> +static void transform_coeffs(struct ic_encode_coeff *out,
>> + const struct ic_encode_coeff *a,
>> + const struct ic_encode_coeff *b)
>> +{
>> + int c_a, c_b, c_out;
>> + int o_a, o_b, o_out;
>> + int outscale = 0;
>> + int i, j, k;
>> +
>> + for (i = 0; i < 3; i++) {
>> + o_out = 0;
>> + for (j = 0; j < 3; j++) {
>> + int scale;
>> +
>> + /* M_c[i,j] = M_a[i,k] * M_b[k,j] */
>> + c_out = 0;
>> + for (k = 0; k < 3; k++) {
>> + c_a = coeff_fix(a->coeff[i][k], a->scale);
>> + c_b = coeff_fix(b->coeff[k][j], b->scale);
>> + c_out += c_a * c_b;
>> + }
>> +
>> + c_out = DIV_ROUND_CLOSEST(c_out, 1 << 8);
>> + out->coeff[i][j] = c_out;
>> +
>> + /*
>> + * get scale for this coefficient and update
>> + * final output scale.
>> + */
>> + scale = get_coeff_scale(c_out);
>> + outscale = max(outscale, scale);
>> +
>> + /* M_a[i,j] * O_b[j] */
>> + c_a = coeff_fix(a->coeff[i][j], a->scale);
>> + o_b = offset_fix(b->offset[j], b->scale);
>> + o_out += DIV_ROUND_CLOSEST(c_a * o_b, 1 << 8);
>> + }
>> +
>> + /* O_c[i] = (M_a * O_b)[i] + O_a[i] */
>> + o_a = offset_fix(a->offset[i], a->scale);
>> + o_out += o_a;
>> +
>> + out->offset[i] = o_out;
>> + }
>> +
>> + /*
>> + * normalize output coefficients and offsets to TPMEM
>> + * register format.
>> + */
>> + for (i = 0; i < 3; i++) {
>> + for (j = 0; j < 3; j++) {
>> + c_out = out->coeff[i][j];
>> + out->coeff[i][j] = coeff_normalize(c_out, outscale);
>> + }
>> +
>> + o_out = out->offset[i];
>> + out->offset[i] = offset_normalize(o_out, outscale);
>> + }
>> +
>> + out->scale = outscale + 1;
>> +}
>> +
>> static int calc_csc_coeffs(struct ipu_ic_priv *priv,
>> struct ic_encode_coeff *coeff_out,
>> const struct ipu_ic_colorspace *in,
>> @@ -290,14 +515,6 @@ static int calc_csc_coeffs(struct ipu_ic_priv *priv,
>> 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 &&
>> @@ -307,7 +524,18 @@ static int calc_csc_coeffs(struct ipu_ic_priv *priv,
>> }
>>
>> if (in->cs == out->cs) {
>> - *coeff_out = ic_encode_identity;
>> + if (in->quant == out->quant) {
>> + *coeff_out = ic_encode_identity;
>> + } else if (in->quant == V4L2_QUANTIZATION_FULL_RANGE) {
>> + /* YUV full-range to YUV limited-range */
>> + *coeff_out = ic_encode_ycbcr_full2lim;
>> +
>> + /* set saturation bit for YUV limited-range output */
>> + coeff_out->sat = true;
>> + } else {
>> + /* YUV limited-range to YUV full-range */
>> + *coeff_out = ic_encode_ycbcr_lim2full;
>> + }
>>
>> return 0;
>> }
>> @@ -328,7 +556,24 @@ static int calc_csc_coeffs(struct ipu_ic_priv *priv,
>> return -ENOTSUPP;
>> }
>>
>> - *coeff_out = *encode_coeff;
>> + if (in->quant == out->quant) {
>> + /*
>> + * YUV full-range to RGB full-range, or
>> + * RGB full-range to YUV full-range.
>> + */
>> + *coeff_out = *encode_coeff;
>> + } else if (inverse_encode) {
>> + /* YUV limited-range to RGB full-range */
>> + transform_coeffs(coeff_out, encode_coeff,
>> + &ic_encode_ycbcr_lim2full);
>> + } else {
>> + /* RGB full-range to YUV limited-range */
>> + transform_coeffs(coeff_out, &ic_encode_ycbcr_full2lim,
>> + encode_coeff);
>> +
>> + /* set saturation bit for YUV limited-range output */
>> + coeff_out->sat = true;
>> + }
>>
>> return 0;
>> }
>> @@ -340,9 +585,9 @@ static int init_csc(struct ipu_ic *ic,
>> {
>> struct ipu_ic_priv *priv = ic->priv;
>> struct ic_encode_coeff coeff;
>> + const unsigned int (*c)[3];
>> + const unsigned int *a;
>> u32 __iomem *base;
>> - const u16 (*c)[3];
>> - const u16 *a;
>> u32 param;
>> int ret;
>>
>> @@ -354,8 +599,8 @@ static int init_csc(struct ipu_ic *ic,
>> (priv->tpmem_base + ic->reg->tpmem_csc[csc_index]);
>>
>> /* Cast to unsigned */
>> - c = (const u16 (*)[3])coeff.coeff;
>> - a = (const u16 *)coeff.offset;
>> + c = (const unsigned int (*)[3])coeff.coeff;
>> + a = (const unsigned int *)coeff.offset;
>>
>> param = ((a[0] & 0x1f) << 27) | ((c[0][0] & 0x1ff) << 18) |
>> ((c[1][1] & 0x1ff) << 9) | (c[2][2] & 0x1ff);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-03-09 1:26 UTC | newest]
Thread overview: 12+ 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 1/7] gpu: ipu-v3: ipu-ic: Fix saturation bit offset in TPMEM Steve Longerbeam
2019-03-08 10:24 ` Philipp Zabel
2019-03-07 23:33 ` [PATCH v6 2/7] gpu: ipu-v3: ipu-ic: Fix BT.601 coefficients Steve Longerbeam
2019-03-08 10:23 ` Philipp Zabel
2019-03-09 1:00 ` Steve Longerbeam
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
2019-03-07 23:33 ` [PATCH v6 4/7] gpu: ipu-v3: ipu-ic: Add support for Rec.709 encoding Steve Longerbeam
2019-03-07 23:33 ` [PATCH v6 5/7] gpu: ipu-v3: ipu-ic: Add support for limited range encoding Steve Longerbeam
2019-03-08 11:57 ` Philipp Zabel
2019-03-09 1:26 ` Steve Longerbeam
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).