* [PATCH v4 0/4] media: imx: Add support for BT.709 encoding @ 2019-02-09 1:47 Steve Longerbeam 2019-02-09 1:47 ` [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices Steve Longerbeam ` (3 more replies) 0 siblings, 4 replies; 17+ messages in thread From: Steve Longerbeam @ 2019-02-09 1:47 UTC (permalink / raw) To: linux-media; +Cc: Tim Harvey, Steve Longerbeam This patchset adds support for the BT.709 encoding and inverse encoding matrices to the ipu_ic task init functions. The imx-media driver can now support both BT.601 and BT.709 encoding. History: v4: - fix a compile error in init_csc(), reported by Tim Harvey. v3: - fix some inconsistent From: and Signed-off-by:'s. No functional changes. v2: - rename ic_csc_rgb2rgb matrix to ic_csc_identity. - only return "Unsupported YCbCr encoding" error if inf != outf, since if inf == outf, the identity matrix can be used. Reported by Tim Harvey. - move ic_route check above default colorimetry checks, and fill default colorspace for ic_route, otherwise it's not possible to set BT.709 encoding for ic routes. Steve Longerbeam (4): gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices gpu: ipu-v3: ipu-ic: Simplify selection of encoding matrix gpu: ipu-v3: ipu-ic: Add support for BT.709 encoding media: imx: Allow BT.709 encoding for IC routes drivers/gpu/ipu-v3/ipu-ic.c | 96 +++++++++++++++++---- drivers/gpu/ipu-v3/ipu-image-convert.c | 1 + drivers/staging/media/imx/imx-ic-prpencvf.c | 4 +- drivers/staging/media/imx/imx-media-utils.c | 20 +++-- include/video/imx-ipu-v3.h | 5 +- 5 files changed, 98 insertions(+), 28 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices 2019-02-09 1:47 [PATCH v4 0/4] media: imx: Add support for BT.709 encoding Steve Longerbeam @ 2019-02-09 1:47 ` Steve Longerbeam 2019-02-11 9:58 ` Philipp Zabel 2019-02-09 1:47 ` [PATCH v4 2/4] gpu: ipu-v3: ipu-ic: Simplify selection of encoding matrix Steve Longerbeam ` (2 subsequent siblings) 3 siblings, 1 reply; 17+ messages in thread From: Steve Longerbeam @ 2019-02-09 1:47 UTC (permalink / raw) To: linux-media Cc: Tim Harvey, Steve Longerbeam, Philipp Zabel, open list:DRM DRIVERS FOR FREESCALE IMX, open list The ycbcr2rgb and inverse rgb2ycbcr matrices define the BT.601 encoding coefficients, so rename them to indicate that. And add some comments to make clear these are BT.601 coefficients encoding between YUV limited range and RGB full range. The ic_csc_rgb2rgb matrix is just an identity matrix, so rename to ic_csc_identity. No functional changes. Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> --- Changes in v2: - rename ic_csc_rgb2rgb matrix to ic_csc_identity. --- drivers/gpu/ipu-v3/ipu-ic.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c index 594c3cbc8291..3ef61f0b509b 100644 --- a/drivers/gpu/ipu-v3/ipu-ic.c +++ b/drivers/gpu/ipu-v3/ipu-ic.c @@ -183,11 +183,13 @@ struct ic_csc_params { }; /* + * BT.601 encoding from RGB full range to YUV limited range: + * * Y = R * .299 + G * .587 + B * .114; * U = R * -.169 + G * -.332 + B * .500 + 128.; * V = R * .500 + G * -.419 + B * -.0813 + 128.; */ -static const struct ic_csc_params ic_csc_rgb2ycbcr = { +static const struct ic_csc_params ic_csc_rgb2ycbcr_bt601 = { .coeff = { { 77, 150, 29 }, { 469, 427, 128 }, @@ -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_csc_params ic_csc_identity = { .coeff = { { 128, 0, 0 }, { 0, 128, 0 }, @@ -208,11 +213,13 @@ static const struct ic_csc_params ic_csc_rgb2rgb = { }; /* + * Inverse BT.601 encoding from YUV limited range to RGB full range: + * * 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); */ -static const struct ic_csc_params ic_csc_ycbcr2rgb = { +static const struct ic_csc_params ic_csc_ycbcr2rgb_bt601 = { .coeff = { { 149, 0, 204 }, { 149, 462, 408 }, @@ -238,11 +245,11 @@ 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; + params = &ic_csc_ycbcr2rgb_bt601; else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_YUV) - params = &ic_csc_rgb2ycbcr; + params = &ic_csc_rgb2ycbcr_bt601; else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_RGB) - params = &ic_csc_rgb2rgb; + params = &ic_csc_identity; else { dev_err(priv->ipu->dev, "Unsupported color space conversion\n"); return -EINVAL; -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices 2019-02-09 1:47 ` [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices Steve Longerbeam @ 2019-02-11 9:58 ` Philipp Zabel 2019-02-11 18:24 ` Steve Longerbeam 2019-02-11 23:33 ` Steve Longerbeam 0 siblings, 2 replies; 17+ messages in thread From: Philipp Zabel @ 2019-02-11 9:58 UTC (permalink / raw) To: Steve Longerbeam, linux-media Cc: Tim Harvey, open list:DRM DRIVERS FOR FREESCALE IMX, open list On Fri, 2019-02-08 at 17:47 -0800, Steve Longerbeam wrote: > The ycbcr2rgb and inverse rgb2ycbcr matrices define the BT.601 encoding > coefficients, so rename them to indicate that. And add some comments > to make clear these are BT.601 coefficients encoding between YUV limited > range and RGB full range. The ic_csc_rgb2rgb matrix is just an identity > matrix, so rename to ic_csc_identity. No functional changes. > > Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> > --- > Changes in v2: > - rename ic_csc_rgb2rgb matrix to ic_csc_identity. > --- > drivers/gpu/ipu-v3/ipu-ic.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c > index 594c3cbc8291..3ef61f0b509b 100644 > --- a/drivers/gpu/ipu-v3/ipu-ic.c > +++ b/drivers/gpu/ipu-v3/ipu-ic.c > @@ -183,11 +183,13 @@ struct ic_csc_params { > }; > > /* > + * BT.601 encoding from RGB full range to YUV limited range: > + * > * Y = R * .299 + G * .587 + B * .114; > * U = R * -.169 + G * -.332 + B * .500 + 128.; > * V = R * .500 + G * -.419 + B * -.0813 + 128.; Hm, this is a conversion to full range BT.601. For limited range, the matrix coefficients 0.2990 0.5870 0.1140 -0.1687 -0.3313 0.5000 0.5000 -0.4187 -0.0813 should be multiplied with 219/255 (Y) and 224/255 (U,V), respectively: Y = R * .2568 + G * .5041 + B * .0979 + 16; U = R * -.1482 + G * -.2910 + B * .4392 + 128; V = R * .4392 + G * -.3678 + B * -.0714 + 128; > */ > -static const struct ic_csc_params ic_csc_rgb2ycbcr = { > +static const struct ic_csc_params ic_csc_rgb2ycbcr_bt601 = { > .coeff = { > { 77, 150, 29 }, > { 469, 427, 128 }, > @@ -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_csc_params ic_csc_identity = { > .coeff = { > { 128, 0, 0 }, > { 0, 128, 0 }, > @@ -208,11 +213,13 @@ static const struct ic_csc_params ic_csc_rgb2rgb = { > }; > > /* > + * Inverse BT.601 encoding from YUV limited range to RGB full range: > + * > * 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); > */ This looks correct. > -static const struct ic_csc_params ic_csc_ycbcr2rgb = { > +static const struct ic_csc_params ic_csc_ycbcr2rgb_bt601 = { > .coeff = { > { 149, 0, 204 }, > { 149, 462, 408 }, > @@ -238,11 +245,11 @@ 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; > + params = &ic_csc_ycbcr2rgb_bt601; > else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_YUV) > - params = &ic_csc_rgb2ycbcr; > + params = &ic_csc_rgb2ycbcr_bt601; > else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_RGB) > - params = &ic_csc_rgb2rgb; > + params = &ic_csc_identity; > else { > dev_err(priv->ipu->dev, "Unsupported color space conversion\n"); > return -EINVAL; regards Philipp ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices 2019-02-11 9:58 ` Philipp Zabel @ 2019-02-11 18:24 ` Steve Longerbeam 2019-02-12 10:17 ` Philipp Zabel 2019-02-11 23:33 ` Steve Longerbeam 1 sibling, 1 reply; 17+ messages in thread From: Steve Longerbeam @ 2019-02-11 18:24 UTC (permalink / raw) To: Philipp Zabel, linux-media Cc: Tim Harvey, open list:DRM DRIVERS FOR FREESCALE IMX, open list Hi Philipp, On 2/11/19 1:58 AM, Philipp Zabel wrote: > On Fri, 2019-02-08 at 17:47 -0800, Steve Longerbeam wrote: >> The ycbcr2rgb and inverse rgb2ycbcr matrices define the BT.601 encoding >> coefficients, so rename them to indicate that. And add some comments >> to make clear these are BT.601 coefficients encoding between YUV limited >> range and RGB full range. The ic_csc_rgb2rgb matrix is just an identity >> matrix, so rename to ic_csc_identity. No functional changes. >> >> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> >> --- >> Changes in v2: >> - rename ic_csc_rgb2rgb matrix to ic_csc_identity. >> --- >> drivers/gpu/ipu-v3/ipu-ic.c | 21 ++++++++++++++------- >> 1 file changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c >> index 594c3cbc8291..3ef61f0b509b 100644 >> --- a/drivers/gpu/ipu-v3/ipu-ic.c >> +++ b/drivers/gpu/ipu-v3/ipu-ic.c >> @@ -183,11 +183,13 @@ struct ic_csc_params { >> }; >> >> /* >> + * BT.601 encoding from RGB full range to YUV limited range: >> + * >> * Y = R * .299 + G * .587 + B * .114; >> * U = R * -.169 + G * -.332 + B * .500 + 128.; >> * V = R * .500 + G * -.419 + B * -.0813 + 128.; > Hm, this is a conversion to full range BT.601. For limited range, the > matrix coefficients > > 0.2990 0.5870 0.1140 > -0.1687 -0.3313 0.5000 > 0.5000 -0.4187 -0.0813 > > should be multiplied with 219/255 (Y) and 224/255 (U,V), respectively: > > Y = R * .2568 + G * .5041 + B * .0979 + 16; > U = R * -.1482 + G * -.2910 + B * .4392 + 128; > V = R * .4392 + G * -.3678 + B * -.0714 + 128; Looking more closely at these coefficients now, I see you are right, they are the BT.601 YUV full-range coefficients (Y range 0 to 1, U and V range -0.5 to 0.5). Well, not even that -- the coefficients are not being scaled to the limited ranges, but the 0.5 offset (128) _is_ being added to U/V, but no offset for Y. So it is even more messed up. Your corrected coefficients and offsets look correct to me: Y coefficients scaled to (235 - 16) / 255 and U/V coefficients scaled to (240 - 16) / 255, and add the offsets for both Y and U/V. But what about this "SAT_MODE" field in the IC task parameter memory? According to the manual the hardware will automatically convert the written coefficients to the correct limited ranges. I see there is a "sat" field defined in the struct but is not being set in the tables. So what should we do, define the full range coefficients, and make use of SAT_MODE h/w feature, or scale/offset the coefficients ourselves and not use SAT_MODE? I'm inclined to do the former. Steve > >> */ >> -static const struct ic_csc_params ic_csc_rgb2ycbcr = { >> +static const struct ic_csc_params ic_csc_rgb2ycbcr_bt601 = { >> .coeff = { >> { 77, 150, 29 }, >> { 469, 427, 128 }, >> @@ -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_csc_params ic_csc_identity = { >> .coeff = { >> { 128, 0, 0 }, >> { 0, 128, 0 }, >> @@ -208,11 +213,13 @@ static const struct ic_csc_params ic_csc_rgb2rgb = { >> }; >> >> /* >> + * Inverse BT.601 encoding from YUV limited range to RGB full range: >> + * >> * 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); >> */ > This looks correct. > >> -static const struct ic_csc_params ic_csc_ycbcr2rgb = { >> +static const struct ic_csc_params ic_csc_ycbcr2rgb_bt601 = { >> .coeff = { >> { 149, 0, 204 }, >> { 149, 462, 408 }, >> @@ -238,11 +245,11 @@ 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; >> + params = &ic_csc_ycbcr2rgb_bt601; >> else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_YUV) >> - params = &ic_csc_rgb2ycbcr; >> + params = &ic_csc_rgb2ycbcr_bt601; >> else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_RGB) >> - params = &ic_csc_rgb2rgb; >> + params = &ic_csc_identity; >> else { >> dev_err(priv->ipu->dev, "Unsupported color space conversion\n"); >> return -EINVAL; > regards > Philipp ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices 2019-02-11 18:24 ` Steve Longerbeam @ 2019-02-12 10:17 ` Philipp Zabel 2019-02-12 17:42 ` Steve Longerbeam 0 siblings, 1 reply; 17+ messages in thread From: Philipp Zabel @ 2019-02-12 10:17 UTC (permalink / raw) To: Steve Longerbeam, linux-media Cc: Tim Harvey, open list:DRM DRIVERS FOR FREESCALE IMX, open list Hi Steve, On Mon, 2019-02-11 at 10:24 -0800, Steve Longerbeam wrote: [...] > Looking more closely at these coefficients now, I see you are right, > they are the BT.601 YUV full-range coefficients (Y range 0 to 1, U and V > range -0.5 to 0.5). Well, not even that -- the coefficients are not > being scaled to the limited ranges, but the 0.5 offset (128) _is_ being > added to U/V, but no offset for Y. So it is even more messed up. > > Your corrected coefficients and offsets look correct to me: Y > coefficients scaled to (235 - 16) / 255 and U/V coefficients scaled to > (240 - 16) / 255, and add the offsets for both Y and U/V. > > But what about this "SAT_MODE" field in the IC task parameter memory? That just controls the saturation. The result after the matrix multiplication is either saturated to [0..255] or to [16..235]/[16..240] when converting from the internal representation to the 8 bit output. > According to the manual the hardware will automatically convert the > written coefficients to the correct limited ranges. Where did you get that from? "The final calculation result is limited according to the SAT_MODE parameter and rounded to 8 bits." I see no mention of coefficients being modified. > I see there is a "sat" field defined in the struct but is not being > set in the tables. > > So what should we do, define the full range coefficients, and make use > of SAT_MODE h/w feature, or scale/offset the coefficients ourselves and > not use SAT_MODE? I'm inclined to do the former. SAT_MODE should be set for conversions to YUV limited range so that the coefficients can be rounded to the closest value. Otherwise we'd have to round towards zero, possibly with a larger error, to make sure the results are inside the valid ranges. regards Philipp ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices 2019-02-12 10:17 ` Philipp Zabel @ 2019-02-12 17:42 ` Steve Longerbeam 2019-02-13 10:35 ` Philipp Zabel 0 siblings, 1 reply; 17+ messages in thread From: Steve Longerbeam @ 2019-02-12 17:42 UTC (permalink / raw) To: Philipp Zabel, linux-media Cc: Tim Harvey, open list:DRM DRIVERS FOR FREESCALE IMX, open list On 2/12/19 2:17 AM, Philipp Zabel wrote: > Hi Steve, > > On Mon, 2019-02-11 at 10:24 -0800, Steve Longerbeam wrote: > [...] >> Looking more closely at these coefficients now, I see you are right, >> they are the BT.601 YUV full-range coefficients (Y range 0 to 1, U and V >> range -0.5 to 0.5). Well, not even that -- the coefficients are not >> being scaled to the limited ranges, but the 0.5 offset (128) _is_ being >> added to U/V, but no offset for Y. So it is even more messed up. >> >> Your corrected coefficients and offsets look correct to me: Y >> coefficients scaled to (235 - 16) / 255 and U/V coefficients scaled to >> (240 - 16) / 255, and add the offsets for both Y and U/V. >> >> But what about this "SAT_MODE" field in the IC task parameter memory? > That just controls the saturation. The result after the matrix > multiplication is either saturated to [0..255] or to [16..235]/[16..240] > when converting from the internal representation to the 8 bit output. By saturation I think you mean clipped to those ranges? > >> According to the manual the hardware will automatically convert the >> written coefficients to the correct limited ranges. > Where did you get that from? "The final calculation result is limited > according to the SAT_MODE parameter and rounded to 8 bits." I see no > mention of coefficients being modified. Well, as is often the case with this manual, I was interpreting based on poorly written information. By "final calculation result is limited according to the SAT_MODE parameter" I interpreted that to mean the hardware enables scaling from full range to limited range. But I concede that it more likely means it clips the output to those ranges. > >> I see there is a "sat" field defined in the struct but is not being >> set in the tables. >> >> So what should we do, define the full range coefficients, and make use >> of SAT_MODE h/w feature, or scale/offset the coefficients ourselves and >> not use SAT_MODE? I'm inclined to do the former. > SAT_MODE should be set for conversions to YUV limited range so that the > coefficients can be rounded to the closest value. Well, we have already rounded the coefficients to the nearest int in the tables. Do you mean the final result (coeff * color component + offset) is rounded? > Otherwise we'd have to > round towards zero, possibly with a larger error, to make sure the > results are inside the valid ranges. Makes sense, I will turn on that bit for limited range YUV output for v5, so that the final color component result is clipped to limited range and rounded. Steve ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices 2019-02-12 17:42 ` Steve Longerbeam @ 2019-02-13 10:35 ` Philipp Zabel 2019-02-14 18:54 ` Steve Longerbeam 0 siblings, 1 reply; 17+ messages in thread From: Philipp Zabel @ 2019-02-13 10:35 UTC (permalink / raw) To: Steve Longerbeam, linux-media Cc: Tim Harvey, open list:DRM DRIVERS FOR FREESCALE IMX, open list On Tue, 2019-02-12 at 09:42 -0800, Steve Longerbeam wrote: [...] > > > But what about this "SAT_MODE" field in the IC task parameter memory? > > > > That just controls the saturation. The result after the matrix > > multiplication is either saturated to [0..255] or to [16..235]/[16..240] > > when converting from the internal representation to the 8 bit output. > > By saturation I think you mean clipped to those ranges? Yes, thanks. I didn't realize it sounds weird to use saturated this way. See: https://en.wikipedia.org/wiki/Saturation_arithmetic > > > According to the manual the hardware will automatically convert the > > > written coefficients to the correct limited ranges. > > > > Where did you get that from? "The final calculation result is limited > > according to the SAT_MODE parameter and rounded to 8 bits." I see no > > mention of coefficients being modified. > > Well, as is often the case with this manual, I was interpreting based on > poorly written information. By "final calculation result is limited > according to the SAT_MODE parameter" I interpreted that to mean the > hardware enables scaling from full range to limited range. But I concede > that it more likely means it clips the output to those ranges. Ok, with this manual I'm never sure that there aren't some conflicting statements somewhere else that I might have overlooked. Good to see that we are at least basing our interpretations on the same text. > > > I see there is a "sat" field defined in the struct but is not being > > > set in the tables. > > > > > > So what should we do, define the full range coefficients, and make use > > > of SAT_MODE h/w feature, or scale/offset the coefficients ourselves and > > > not use SAT_MODE? I'm inclined to do the former. > > > > SAT_MODE should be set for conversions to YUV limited range so that the > > coefficients can be rounded to the closest value. > > Well, we have already rounded the coefficients to the nearest int in the > tables. Do you mean the final result (coeff * color component + offset) > is rounded? The manual says so: "The final calculation result is limited according to the SAT_MODE parameter and rounded to 8 bits", but that's not what I meant. Still, I might have been mistaken. I think due to the fact that the coefficients are multiplied by up to 255 (max pixel value) and then effectively divided by 256 when converting to 8 bit, the only way to overflow limited range is if two coefficients are rounded away from zero in the calculation of a single component. This doesn't seem to happen in practice. A constructed example, conversion to YUV limited range with carefully chosen coefficients. Y = R * .1817 + G * .6153 + B * .0618 + 16; Note that .1817 + .6153 + .0618 < 219/255. With rounded coefficients though: Y = (R * 47 + G * 158 + B * 16 + (64 << 6)) / 256 = 236.136 Now 47 + 158 + 16 > 219, and the result is out of range. regards Philipp ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices 2019-02-13 10:35 ` Philipp Zabel @ 2019-02-14 18:54 ` Steve Longerbeam 0 siblings, 0 replies; 17+ messages in thread From: Steve Longerbeam @ 2019-02-14 18:54 UTC (permalink / raw) To: Philipp Zabel, linux-media Cc: Tim Harvey, open list:DRM DRIVERS FOR FREESCALE IMX, open list On 2/13/19 2:35 AM, Philipp Zabel wrote: > On Tue, 2019-02-12 at 09:42 -0800, Steve Longerbeam wrote: > [...] >>>> But what about this "SAT_MODE" field in the IC task parameter memory? >>> That just controls the saturation. The result after the matrix >>> multiplication is either saturated to [0..255] or to [16..235]/[16..240] >>> when converting from the internal representation to the 8 bit output. >> By saturation I think you mean clipped to those ranges? > Yes, thanks. I didn't realize it sounds weird to use saturated this way. > See:https://en.wikipedia.org/wiki/Saturation_arithmetic Ok, saturation can mean the same thing as clipping/clamping. Thanks for the article. I tested a RGB->YUV pipeline with the .sat bit set in the BT.601 rgb2yuv table, with the following pipeline on the SabreSD: 'ov5640 1-003c':0 [fmt:RGB565_2X8_LE/1024x768@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] 'imx6-mipi-csi2':0 [fmt:RGB565_2X8_LE/1024x768 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] 'imx6-mipi-csi2':2 [fmt:RGB565_2X8_LE/1024x768 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] 'ipu1_csi1':0 [fmt:RGB565_2X8_LE/1024x768@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range crop.bounds:(0,0)/1024x768 crop:(0,0)/1024x768 compose.bounds:(0,0)/1024x768 compose:(0,0)/1024x768] 'ipu1_csi1':1 [fmt:ARGB8888_1X32/1024x768@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] 'ipu1_ic_prp':0 [fmt:ARGB8888_1X32/1024x768@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] 'ipu1_ic_prp':1 [fmt:ARGB8888_1X32/1024x768@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] 'ipu1_ic_prpenc':0 [fmt:ARGB8888_1X32/1024x768@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] 'ipu1_ic_prpenc':1 [fmt:AYUV8_1X32/800x600@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] /dev/video0:0 Format Video Capture: Width/Height : 800/600 Pixel Format : 'YV12' Field : None Bytes per Line : 800 Size Image : 720000 Colorspace : sRGB Transfer Function : sRGB YCbCr/HSV Encoding: ITU-R 601 Quantization : Limited Range Flags : The result being that the captured image colors are all off (there's a bright pink shade to the images). But I discovered the init_csc() function was not setting the saturation bit at the correct bit offset within the TPMEM. The saturation bit is bit 42, or bit 10 of the second 32-bit word. But the code was writing to bit 9 of the second word. After correcting this, saturation is working fine. I have added another patch that fixes this for v5 series. > >>> SAT_MODE should be set for conversions to YUV limited range so that the >>> coefficients can be rounded to the closest value. >> Well, we have already rounded the coefficients to the nearest int in the >> tables. Do you mean the final result (coeff * color component + offset) >> is rounded? > The manual says so: "The final calculation result is limited according > to the SAT_MODE parameter and rounded to 8 bits", but that's not what I > meant. Still, I might have been mistaken. > > I think due to the fact that the coefficients are multiplied by up to > 255 (max pixel value) and then effectively divided by 256 when > converting to 8 bit, the only way to overflow limited range is if two > coefficients are rounded away from zero in the calculation of a single > component. This doesn't seem to happen in practice. > > A constructed example, conversion to YUV limited range with carefully > chosen coefficients. > > Y = R * .1817 + G * .6153 + B * .0618 + 16; > > Note that .1817 + .6153 + .0618 < 219/255. > With rounded coefficients though: > > Y = (R * 47 + G * 158 + B * 16 + (64 << 6)) / 256 = 236.136 Yes, for a rec.709 conversion and max/worst-case RGB signal = (255,255,255). But the rec.709 coefficients for Y are actually Y = (R * 47 + G * 157 + B * 16 + (16 << 8)) / 256 which for RGB = (255,255,255), Y = 235.14, which doesn't overflow limited range. Steve ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices 2019-02-11 9:58 ` Philipp Zabel 2019-02-11 18:24 ` Steve Longerbeam @ 2019-02-11 23:33 ` Steve Longerbeam 1 sibling, 0 replies; 17+ messages in thread From: Steve Longerbeam @ 2019-02-11 23:33 UTC (permalink / raw) To: Philipp Zabel, linux-media Cc: Tim Harvey, open list:DRM DRIVERS FOR FREESCALE IMX, open list On 2/11/19 1:58 AM, Philipp Zabel wrote: > On Fri, 2019-02-08 at 17:47 -0800, Steve Longerbeam wrote: >> The ycbcr2rgb and inverse rgb2ycbcr matrices define the BT.601 encoding >> coefficients, so rename them to indicate that. And add some comments >> to make clear these are BT.601 coefficients encoding between YUV limited >> range and RGB full range. The ic_csc_rgb2rgb matrix is just an identity >> matrix, so rename to ic_csc_identity. No functional changes. >> >> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> >> --- >> Changes in v2: >> - rename ic_csc_rgb2rgb matrix to ic_csc_identity. >> --- >> drivers/gpu/ipu-v3/ipu-ic.c | 21 ++++++++++++++------- >> 1 file changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c >> index 594c3cbc8291..3ef61f0b509b 100644 >> --- a/drivers/gpu/ipu-v3/ipu-ic.c >> +++ b/drivers/gpu/ipu-v3/ipu-ic.c >> @@ -183,11 +183,13 @@ struct ic_csc_params { >> }; >> >> /* >> + * BT.601 encoding from RGB full range to YUV limited range: >> + * >> * Y = R * .299 + G * .587 + B * .114; >> * U = R * -.169 + G * -.332 + B * .500 + 128.; >> * V = R * .500 + G * -.419 + B * -.0813 + 128.; > Hm, this is a conversion to full range BT.601. For limited range, the > matrix coefficients > > 0.2990 0.5870 0.1140 > -0.1687 -0.3313 0.5000 > 0.5000 -0.4187 -0.0813 > > should be multiplied with 219/255 (Y) and 224/255 (U,V), respectively: > > Y = R * .2568 + G * .5041 + B * .0979 + 16; > U = R * -.1482 + G * -.2910 + B * .4392 + 128; > V = R * .4392 + G * -.3678 + B * -.0714 + 128; > >> */ >> -static const struct ic_csc_params ic_csc_rgb2ycbcr = { >> +static const struct ic_csc_params ic_csc_rgb2ycbcr_bt601 = { >> .coeff = { >> { 77, 150, 29 }, >> { 469, 427, 128 }, >> @@ -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_csc_params ic_csc_identity = { >> .coeff = { >> { 128, 0, 0 }, >> { 0, 128, 0 }, >> @@ -208,11 +213,13 @@ static const struct ic_csc_params ic_csc_rgb2rgb = { >> }; >> >> /* >> + * Inverse BT.601 encoding from YUV limited range to RGB full range: >> + * >> * 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); >> */ > This looks correct. Agreed, the coefficients in the comments are correct, but the actual table values were a bit off. I will correct them for v5 (Green offset should be 272 in the table, not 266). Steve > >> -static const struct ic_csc_params ic_csc_ycbcr2rgb = { >> +static const struct ic_csc_params ic_csc_ycbcr2rgb_bt601 = { >> .coeff = { >> { 149, 0, 204 }, >> { 149, 462, 408 }, >> @@ -238,11 +245,11 @@ 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; >> + params = &ic_csc_ycbcr2rgb_bt601; >> else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_YUV) >> - params = &ic_csc_rgb2ycbcr; >> + params = &ic_csc_rgb2ycbcr_bt601; >> else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_RGB) >> - params = &ic_csc_rgb2rgb; >> + params = &ic_csc_identity; >> else { >> dev_err(priv->ipu->dev, "Unsupported color space conversion\n"); >> return -EINVAL; > regards > Philipp ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4 2/4] gpu: ipu-v3: ipu-ic: Simplify selection of encoding matrix 2019-02-09 1:47 [PATCH v4 0/4] media: imx: Add support for BT.709 encoding Steve Longerbeam 2019-02-09 1:47 ` [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices Steve Longerbeam @ 2019-02-09 1:47 ` Steve Longerbeam 2019-02-11 10:00 ` Philipp Zabel 2019-02-09 1:47 ` [PATCH v4 3/4] gpu: ipu-v3: ipu-ic: Add support for BT.709 encoding Steve Longerbeam 2019-02-09 1:47 ` [PATCH v4 4/4] media: imx: Allow BT.709 encoding for IC routes Steve Longerbeam 3 siblings, 1 reply; 17+ messages in thread From: Steve Longerbeam @ 2019-02-09 1:47 UTC (permalink / raw) To: linux-media Cc: Tim Harvey, Steve Longerbeam, Philipp Zabel, open list:DRM DRIVERS FOR FREESCALE IMX, open list Simplify the selection of the Y'CbCr encoding matrices in init_csc(). A side-effect of this change is that init_csc() now allows YUV->YUV using the identity matrix, intead of returning error. Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> --- drivers/gpu/ipu-v3/ipu-ic.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c index 3ef61f0b509b..e459615a49a1 100644 --- a/drivers/gpu/ipu-v3/ipu-ic.c +++ b/drivers/gpu/ipu-v3/ipu-ic.c @@ -244,16 +244,12 @@ static int init_csc(struct ipu_ic *ic, base = (u32 __iomem *) (priv->tpmem_base + ic->reg->tpmem_csc[csc_index]); - if (inf == IPUV3_COLORSPACE_YUV && outf == IPUV3_COLORSPACE_RGB) + if (inf == outf) + params = &ic_csc_identity; + else if (inf == IPUV3_COLORSPACE_YUV) params = &ic_csc_ycbcr2rgb_bt601; - else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_YUV) + else params = &ic_csc_rgb2ycbcr_bt601; - else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_RGB) - params = &ic_csc_identity; - else { - dev_err(priv->ipu->dev, "Unsupported color space conversion\n"); - return -EINVAL; - } /* Cast to unsigned */ c = (const u16 (*)[3])params->coeff; -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/4] gpu: ipu-v3: ipu-ic: Simplify selection of encoding matrix 2019-02-09 1:47 ` [PATCH v4 2/4] gpu: ipu-v3: ipu-ic: Simplify selection of encoding matrix Steve Longerbeam @ 2019-02-11 10:00 ` Philipp Zabel 0 siblings, 0 replies; 17+ messages in thread From: Philipp Zabel @ 2019-02-11 10:00 UTC (permalink / raw) To: Steve Longerbeam, linux-media Cc: Tim Harvey, open list:DRM DRIVERS FOR FREESCALE IMX, open list On Fri, 2019-02-08 at 17:47 -0800, Steve Longerbeam wrote: > Simplify the selection of the Y'CbCr encoding matrices in init_csc(). > A side-effect of this change is that init_csc() now allows YUV->YUV > using the identity matrix, intead of returning error. > > Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> Note that this only works if both YUV encodings have the same range. regards Philipp ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4 3/4] gpu: ipu-v3: ipu-ic: Add support for BT.709 encoding 2019-02-09 1:47 [PATCH v4 0/4] media: imx: Add support for BT.709 encoding Steve Longerbeam 2019-02-09 1:47 ` [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices Steve Longerbeam 2019-02-09 1:47 ` [PATCH v4 2/4] gpu: ipu-v3: ipu-ic: Simplify selection of encoding matrix Steve Longerbeam @ 2019-02-09 1:47 ` Steve Longerbeam 2019-02-11 10:12 ` Philipp Zabel 2019-02-09 1:47 ` [PATCH v4 4/4] media: imx: Allow BT.709 encoding for IC routes Steve Longerbeam 3 siblings, 1 reply; 17+ messages in thread From: Steve Longerbeam @ 2019-02-09 1:47 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 Pass v4l2 encoding enum to the ipu_ic task init functions, and add support for the BT.709 encoding and inverse encoding matrices. Reported-by: Tim Harvey <tharvey@gateworks.com> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> --- 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 | 71 +++++++++++++++++++-- drivers/gpu/ipu-v3/ipu-image-convert.c | 1 + drivers/staging/media/imx/imx-ic-prpencvf.c | 4 +- include/video/imx-ipu-v3.h | 5 +- 4 files changed, 71 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c index e459615a49a1..c5f83d7e357f 100644 --- a/drivers/gpu/ipu-v3/ipu-ic.c +++ b/drivers/gpu/ipu-v3/ipu-ic.c @@ -212,6 +212,23 @@ static const struct ic_csc_params ic_csc_identity = { .scale = 2, }; +/* + * BT.709 encoding from RGB full range to YUV limited range: + * + * Y = R * .2126 + G * .7152 + B * .0722; + * U = R * -.1146 + G * -.3854 + B * .5000 + 128.; + * V = R * .5000 + G * -.4542 + B * -.0458 + 128.; + */ +static const struct ic_csc_params ic_csc_rgb2ycbcr_bt709 = { + .coeff = { + { 54, 183, 18 }, + { 483, 413, 128 }, + { 128, 396, 500 }, + }, + .offset = { 0, 512, 512 }, + .scale = 1, +}; + /* * Inverse BT.601 encoding from YUV limited range to RGB full range: * @@ -229,12 +246,31 @@ static const struct ic_csc_params ic_csc_ycbcr2rgb_bt601 = { .scale = 2, }; +/* + * Inverse BT.709 encoding from YUV limited range to RGB full range: + * + * R = (1. * (Y - 16)) + (1.5748 * (Cr - 128)); + * G = (1. * (Y - 16)) - (0.1873 * (Cb - 128)) - (0.4681 * (Cr - 128)); + * B = (1. * (Y - 16)) + (1.8556 * (Cb - 128); + */ +static const struct ic_csc_params ic_csc_ycbcr2rgb_bt709 = { + .coeff = { + { 128, 0, 202 }, + { 128, 488, 452 }, + { 128, 238, 0 }, + }, + .offset = { -435, 136, -507 }, + .scale = 2, +}; + static int init_csc(struct ipu_ic *ic, enum ipu_color_space inf, enum ipu_color_space outf, + enum v4l2_ycbcr_encoding encoding, int csc_index) { struct ipu_ic_priv *priv = ic->priv; + const struct ic_csc_params *params_rgb2yuv, *params_yuv2rgb; const struct ic_csc_params *params; u32 __iomem *base; const u16 (*c)[3]; @@ -244,12 +280,30 @@ static int init_csc(struct ipu_ic *ic, base = (u32 __iomem *) (priv->tpmem_base + ic->reg->tpmem_csc[csc_index]); + switch (encoding) { + case V4L2_YCBCR_ENC_601: + params_rgb2yuv = &ic_csc_rgb2ycbcr_bt601; + params_yuv2rgb = &ic_csc_ycbcr2rgb_bt601; + break; + case V4L2_YCBCR_ENC_709: + params_rgb2yuv = &ic_csc_rgb2ycbcr_bt709; + params_yuv2rgb = &ic_csc_ycbcr2rgb_bt709; + break; + default: + if (inf != outf) { + dev_err(priv->ipu->dev, + "Unsupported YCbCr encoding\n"); + return -EINVAL; + } + break; + } + if (inf == outf) params = &ic_csc_identity; else if (inf == IPUV3_COLORSPACE_YUV) - params = &ic_csc_ycbcr2rgb_bt601; + params = params_yuv2rgb; else - params = &ic_csc_rgb2ycbcr_bt601; + params = params_rgb2yuv; /* Cast to unsigned */ c = (const u16 (*)[3])params->coeff; @@ -390,6 +444,7 @@ EXPORT_SYMBOL_GPL(ipu_ic_task_disable); int ipu_ic_task_graphics_init(struct ipu_ic *ic, enum ipu_color_space in_g_cs, + enum v4l2_ycbcr_encoding encoding, bool galpha_en, u32 galpha, bool colorkey_en, u32 colorkey) { @@ -408,7 +463,7 @@ int ipu_ic_task_graphics_init(struct ipu_ic *ic, if (!(ic_conf & ic->bit->ic_conf_csc1_en)) { /* need transparent CSC1 conversion */ ret = init_csc(ic, IPUV3_COLORSPACE_RGB, - IPUV3_COLORSPACE_RGB, 0); + IPUV3_COLORSPACE_RGB, encoding, 0); if (ret) goto unlock; } @@ -416,7 +471,7 @@ int ipu_ic_task_graphics_init(struct ipu_ic *ic, ic->g_in_cs = in_g_cs; if (ic->g_in_cs != ic->out_cs) { - ret = init_csc(ic, ic->g_in_cs, ic->out_cs, 1); + ret = init_csc(ic, ic->g_in_cs, ic->out_cs, encoding, 1); if (ret) goto unlock; } @@ -450,6 +505,7 @@ int ipu_ic_task_init_rsc(struct ipu_ic *ic, int out_width, int out_height, enum ipu_color_space in_cs, enum ipu_color_space out_cs, + enum v4l2_ycbcr_encoding encoding, u32 rsc) { struct ipu_ic_priv *priv = ic->priv; @@ -485,7 +541,7 @@ int ipu_ic_task_init_rsc(struct ipu_ic *ic, ic->out_cs = out_cs; if (ic->in_cs != ic->out_cs) { - ret = init_csc(ic, ic->in_cs, ic->out_cs, 0); + ret = init_csc(ic, ic->in_cs, ic->out_cs, encoding, 0); if (ret) goto unlock; } @@ -499,10 +555,11 @@ int ipu_ic_task_init(struct ipu_ic *ic, int in_width, int in_height, int out_width, int out_height, enum ipu_color_space in_cs, - enum ipu_color_space out_cs) + enum ipu_color_space out_cs, + enum v4l2_ycbcr_encoding encoding) { return ipu_ic_task_init_rsc(ic, in_width, in_height, out_width, - out_height, in_cs, out_cs, 0); + out_height, in_cs, out_cs, encoding, 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..8b37daa99f58 100644 --- a/drivers/gpu/ipu-v3/ipu-image-convert.c +++ b/drivers/gpu/ipu-v3/ipu-image-convert.c @@ -1358,6 +1358,7 @@ static int convert_start(struct ipu_image_convert_run *run, unsigned int tile) dest_width, dest_height, src_cs, dest_cs, + d_image->base.pix.ycbcr_enc, rsc); if (ret) { dev_err(priv->ipu->dev, "ipu_ic_task_init failed, %d\n", ret); diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c index 376b504e8a42..60ecf5809cc1 100644 --- a/drivers/staging/media/imx/imx-ic-prpencvf.c +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c @@ -484,7 +484,7 @@ static int prp_setup_rotation(struct prp_priv *priv) ret = ipu_ic_task_init(priv->ic, infmt->width, infmt->height, outfmt->height, outfmt->width, - incc->cs, outcc->cs); + incc->cs, outcc->cs, outfmt->ycbcr_enc); if (ret) { v4l2_err(&ic_priv->sd, "ipu_ic_task_init failed, %d\n", ret); goto free_rot1; @@ -587,7 +587,7 @@ static int prp_setup_norotation(struct prp_priv *priv) ret = ipu_ic_task_init(priv->ic, infmt->width, infmt->height, outfmt->width, outfmt->height, - incc->cs, outcc->cs); + incc->cs, outcc->cs, outfmt->ycbcr_enc); 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..b19d1e23eece 100644 --- a/include/video/imx-ipu-v3.h +++ b/include/video/imx-ipu-v3.h @@ -391,15 +391,18 @@ int ipu_ic_task_init(struct ipu_ic *ic, int in_width, int in_height, int out_width, int out_height, enum ipu_color_space in_cs, - enum ipu_color_space out_cs); + enum ipu_color_space out_cs, + enum v4l2_ycbcr_encoding encoding); int ipu_ic_task_init_rsc(struct ipu_ic *ic, int in_width, int in_height, int out_width, int out_height, enum ipu_color_space in_cs, enum ipu_color_space out_cs, + enum v4l2_ycbcr_encoding encoding, u32 rsc); int ipu_ic_task_graphics_init(struct ipu_ic *ic, enum ipu_color_space in_g_cs, + enum v4l2_ycbcr_encoding encoding, 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] 17+ messages in thread
* Re: [PATCH v4 3/4] gpu: ipu-v3: ipu-ic: Add support for BT.709 encoding 2019-02-09 1:47 ` [PATCH v4 3/4] gpu: ipu-v3: ipu-ic: Add support for BT.709 encoding Steve Longerbeam @ 2019-02-11 10:12 ` Philipp Zabel 2019-02-12 1:20 ` Steve Longerbeam 0 siblings, 1 reply; 17+ messages in thread From: Philipp Zabel @ 2019-02-11 10:12 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 Fri, 2019-02-08 at 17:47 -0800, Steve Longerbeam wrote: > Pass v4l2 encoding enum to the ipu_ic task init functions, and add > support for the BT.709 encoding and inverse encoding matrices. > > Reported-by: Tim Harvey <tharvey@gateworks.com> > Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> > --- > 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 | 71 +++++++++++++++++++-- > drivers/gpu/ipu-v3/ipu-image-convert.c | 1 + > drivers/staging/media/imx/imx-ic-prpencvf.c | 4 +- > include/video/imx-ipu-v3.h | 5 +- > 4 files changed, 71 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c > index e459615a49a1..c5f83d7e357f 100644 > --- a/drivers/gpu/ipu-v3/ipu-ic.c > +++ b/drivers/gpu/ipu-v3/ipu-ic.c > @@ -212,6 +212,23 @@ static const struct ic_csc_params ic_csc_identity = { > .scale = 2, > }; > > +/* > + * BT.709 encoding from RGB full range to YUV limited range: > + * > + * Y = R * .2126 + G * .7152 + B * .0722; > + * U = R * -.1146 + G * -.3854 + B * .5000 + 128.; > + * V = R * .5000 + G * -.4542 + B * -.0458 + 128.; This is a conversion to YUV full range. Limited range should be: Y R * .1826 + G * .6142 + B * .0620 + 16; U = R * -.1007 + G * -.3385 + B * .4392 + 128; V R * .4392 + G * -.3990 + B * -.0402 + 128; > + */ > +static const struct ic_csc_params ic_csc_rgb2ycbcr_bt709 = { > + .coeff = { > + { 54, 183, 18 }, > + { 483, 413, 128 }, > + { 128, 396, 500 }, > + }, > + .offset = { 0, 512, 512 }, > + .scale = 1, > +}; > + > /* > * Inverse BT.601 encoding from YUV limited range to RGB full range: > * > @@ -229,12 +246,31 @@ static const struct ic_csc_params ic_csc_ycbcr2rgb_bt601 = { > .scale = 2, > }; > > +/* > + * Inverse BT.709 encoding from YUV limited range to RGB full range: > + * > + * R = (1. * (Y - 16)) + (1.5748 * (Cr - 128)); > + * G = (1. * (Y - 16)) - (0.1873 * (Cb - 128)) - (0.4681 * (Cr - 128)); > + * B = (1. * (Y - 16)) + (1.8556 * (Cb - 128); The coefficients look like full range again, conversion from limited range YUV should look like: R = (1.1644 * (Y - 16)) + (1.7927 * (Cr - 128)); G = (1.1644 * (Y - 16)) - (0.2132 * (Cb - 128)) - (0.5329 * (Cr - 128)); B = (1.1644 * (Y - 16)) + (2.1124 * (Cb - 128); > + */ > +static const struct ic_csc_params ic_csc_ycbcr2rgb_bt709 = { > + .coeff = { > + { 128, 0, 202 }, > + { 128, 488, 452 }, > + { 128, 238, 0 }, > + }, > + .offset = { -435, 136, -507 }, > + .scale = 2, > +}; > + > static int init_csc(struct ipu_ic *ic, > enum ipu_color_space inf, > enum ipu_color_space outf, > + enum v4l2_ycbcr_encoding encoding, Should we support YUV BT.601 <-> YUV REC.709 conversions? That would require separate encodings for input and output. Also, this might be a good time to think about adding quantization range parameters as well. regards Philipp ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 3/4] gpu: ipu-v3: ipu-ic: Add support for BT.709 encoding 2019-02-11 10:12 ` Philipp Zabel @ 2019-02-12 1:20 ` Steve Longerbeam 2019-02-12 11:34 ` Philipp Zabel 0 siblings, 1 reply; 17+ messages in thread From: Steve Longerbeam @ 2019-02-12 1:20 UTC (permalink / raw) To: Philipp Zabel, 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 2/11/19 2:12 AM, Philipp Zabel wrote: > On Fri, 2019-02-08 at 17:47 -0800, Steve Longerbeam wrote: >> Pass v4l2 encoding enum to the ipu_ic task init functions, and add >> support for the BT.709 encoding and inverse encoding matrices. >> >> Reported-by: Tim Harvey <tharvey@gateworks.com> >> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> >> --- >> 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 | 71 +++++++++++++++++++-- >> drivers/gpu/ipu-v3/ipu-image-convert.c | 1 + >> drivers/staging/media/imx/imx-ic-prpencvf.c | 4 +- >> include/video/imx-ipu-v3.h | 5 +- >> 4 files changed, 71 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c >> index e459615a49a1..c5f83d7e357f 100644 >> --- a/drivers/gpu/ipu-v3/ipu-ic.c >> +++ b/drivers/gpu/ipu-v3/ipu-ic.c >> @@ -212,6 +212,23 @@ static const struct ic_csc_params ic_csc_identity = { >> .scale = 2, >> }; >> >> +/* >> + * BT.709 encoding from RGB full range to YUV limited range: >> + * >> + * Y = R * .2126 + G * .7152 + B * .0722; >> + * U = R * -.1146 + G * -.3854 + B * .5000 + 128.; >> + * V = R * .5000 + G * -.4542 + B * -.0458 + 128.; > This is a conversion to YUV full range. Limited range should be: > > Y R * .1826 + G * .6142 + B * .0620 + 16; > U = R * -.1007 + G * -.3385 + B * .4392 + 128; > V R * .4392 + G * -.3990 + B * -.0402 + 128; Yep, I fixed these to encode to limited range YUV, and ... >> + */ >> +static const struct ic_csc_params ic_csc_rgb2ycbcr_bt709 = { >> + .coeff = { >> + { 54, 183, 18 }, >> + { 483, 413, 128 }, >> + { 128, 396, 500 }, >> + }, >> + .offset = { 0, 512, 512 }, >> + .scale = 1, >> +}; >> + >> /* >> * Inverse BT.601 encoding from YUV limited range to RGB full range: >> * >> @@ -229,12 +246,31 @@ static const struct ic_csc_params ic_csc_ycbcr2rgb_bt601 = { >> .scale = 2, >> }; >> >> +/* >> + * Inverse BT.709 encoding from YUV limited range to RGB full range: >> + * >> + * R = (1. * (Y - 16)) + (1.5748 * (Cr - 128)); >> + * G = (1. * (Y - 16)) - (0.1873 * (Cb - 128)) - (0.4681 * (Cr - 128)); >> + * B = (1. * (Y - 16)) + (1.8556 * (Cb - 128); > The coefficients look like full range again, conversion from limited > range YUV should look like: > > R = (1.1644 * (Y - 16)) + (1.7927 * (Cr - 128)); > G = (1.1644 * (Y - 16)) - (0.2132 * (Cb - 128)) - (0.5329 * (Cr - 128)); > B = (1.1644 * (Y - 16)) + (2.1124 * (Cb - 128); fixed these to inverse encode from limited range YUV. >> + */ >> +static const struct ic_csc_params ic_csc_ycbcr2rgb_bt709 = { >> + .coeff = { >> + { 128, 0, 202 }, >> + { 128, 488, 452 }, >> + { 128, 238, 0 }, >> + }, >> + .offset = { -435, 136, -507 }, >> + .scale = 2, >> +}; >> + >> static int init_csc(struct ipu_ic *ic, >> enum ipu_color_space inf, >> enum ipu_color_space outf, >> + enum v4l2_ycbcr_encoding encoding, > Should we support YUV BT.601 <-> YUV REC.709 conversions? That would > require separate encodings for input and output. How about if we pass the input and output encodings to the init ic task functions, but for now require they be the same? We can support transcoding in a later series. > Also, this might be a > good time to think about adding quantization range parameters as well. Again, I think for now, just include input/output quantization but require full range for RGB and limited range for YUV. But that really balloons the arguments to ipu_ic_task_init_*(). Should we create an ipu_ic_task_init structure? Steve ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 3/4] gpu: ipu-v3: ipu-ic: Add support for BT.709 encoding 2019-02-12 1:20 ` Steve Longerbeam @ 2019-02-12 11:34 ` Philipp Zabel 2019-02-12 17:50 ` Steve Longerbeam 0 siblings, 1 reply; 17+ messages in thread From: Philipp Zabel @ 2019-02-12 11:34 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 Hi Steve, On Mon, 2019-02-11 at 17:20 -0800, Steve Longerbeam wrote: [...] > > Should we support YUV BT.601 <-> YUV REC.709 conversions? That would > > require separate encodings for input and output. > > How about if we pass the input and output encodings to the init ic task > functions, but for now require they be the same? We can support > transcoding in a later series. [...] > Again, I think for now, just include input/output quantization but > require full range for RGB and limited range for YUV. Yes, that is fine. I'd just like to avoid unnecessary interface changes between ipu-v3 and imx-media. So if we have to change it right now, why not plan ahead. > But that really balloons the arguments to ipu_ic_task_init_*(). Should > we create an ipu_ic_task_init structure? I wonder if we should just expose struct ic_csc_params and provide a helper to fill it given colorspace and V4L2 encoding/quantization parameters. Something like: struct ipu_ic_csc_params csc; imx_media_init_ic_csc_params(&csc, in_cs, in_encoding, in_quantization, out_cs, out_encoding, out_quantization); ipu_ic_task_init(ic, in_width, in_height, out_width, out_height, &csc); // or ipu_ic_task_init_rsc(ic, rsc, &csc); regards Philipp ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 3/4] gpu: ipu-v3: ipu-ic: Add support for BT.709 encoding 2019-02-12 11:34 ` Philipp Zabel @ 2019-02-12 17:50 ` Steve Longerbeam 0 siblings, 0 replies; 17+ messages in thread From: Steve Longerbeam @ 2019-02-12 17:50 UTC (permalink / raw) To: Philipp Zabel, 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 2/12/19 3:34 AM, Philipp Zabel wrote: > Hi Steve, > > On Mon, 2019-02-11 at 17:20 -0800, Steve Longerbeam wrote: > [...] >>> Should we support YUV BT.601 <-> YUV REC.709 conversions? That would >>> require separate encodings for input and output. >> How about if we pass the input and output encodings to the init ic task >> functions, but for now require they be the same? We can support >> transcoding in a later series. > [...] >> Again, I think for now, just include input/output quantization but >> require full range for RGB and limited range for YUV. > Yes, that is fine. I'd just like to avoid unnecessary interface changes > between ipu-v3 and imx-media. So if we have to change it right now, why > not plan ahead. Agreed! > >> But that really balloons the arguments to ipu_ic_task_init_*(). Should >> we create an ipu_ic_task_init structure? > I wonder if we should just expose struct ic_csc_params I had basically the same idea. I wasn't thinking of creating a helper to fill in the params but sure, I'll add that. Steve > and provide a > helper to fill it given colorspace and V4L2 encoding/quantization > parameters. Something like: > > struct ipu_ic_csc_params csc; > > imx_media_init_ic_csc_params(&csc, > in_cs, in_encoding, in_quantization, > out_cs, out_encoding, out_quantization); > > ipu_ic_task_init(ic, > in_width, in_height, > out_width, out_height, &csc); > // or > ipu_ic_task_init_rsc(ic, rsc, &csc); > > regards > Philipp ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4 4/4] media: imx: Allow BT.709 encoding for IC routes 2019-02-09 1:47 [PATCH v4 0/4] media: imx: Add support for BT.709 encoding Steve Longerbeam ` (2 preceding siblings ...) 2019-02-09 1:47 ` [PATCH v4 3/4] gpu: ipu-v3: ipu-ic: Add support for BT.709 encoding Steve Longerbeam @ 2019-02-09 1:47 ` Steve Longerbeam 3 siblings, 0 replies; 17+ messages in thread From: Steve Longerbeam @ 2019-02-09 1:47 UTC (permalink / raw) To: linux-media Cc: Tim Harvey, Steve Longerbeam, Philipp Zabel, Mauro Carvalho Chehab, Greg Kroah-Hartman, open list:STAGING SUBSYSTEM, open list The IC now supports BT.709 Y'CbCr encoding, in addition to existing BT.601 encoding, so allow both, for pipelines that route through the IC. Reported-by: Tim Harvey <tharvey@gateworks.com> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> --- Changes in v2: - move ic_route check above default colorimetry checks, and fill default colorimetry for ic_route, otherwise it's not possible to set BT.709 encoding for ic routes. --- drivers/staging/media/imx/imx-media-utils.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c index 5f110d90a4ef..dde0e47550d7 100644 --- a/drivers/staging/media/imx/imx-media-utils.c +++ b/drivers/staging/media/imx/imx-media-utils.c @@ -544,6 +544,19 @@ void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt, if (tryfmt->field == V4L2_FIELD_ANY) tryfmt->field = fmt->field; + if (ic_route) { + if (tryfmt->colorspace == V4L2_COLORSPACE_DEFAULT) + tryfmt->colorspace = fmt->colorspace; + + tryfmt->quantization = is_rgb ? + V4L2_QUANTIZATION_FULL_RANGE : + V4L2_QUANTIZATION_LIM_RANGE; + + if (tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_601 && + tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_709) + tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601; + } + /* fill colorimetry if necessary */ if (tryfmt->colorspace == V4L2_COLORSPACE_DEFAULT) { tryfmt->colorspace = fmt->colorspace; @@ -566,13 +579,6 @@ void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt, tryfmt->ycbcr_enc); } } - - if (ic_route) { - tryfmt->quantization = is_rgb ? - V4L2_QUANTIZATION_FULL_RANGE : - V4L2_QUANTIZATION_LIM_RANGE; - tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601; - } } EXPORT_SYMBOL_GPL(imx_media_fill_default_mbus_fields); -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2019-02-14 18:54 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-09 1:47 [PATCH v4 0/4] media: imx: Add support for BT.709 encoding Steve Longerbeam 2019-02-09 1:47 ` [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices Steve Longerbeam 2019-02-11 9:58 ` Philipp Zabel 2019-02-11 18:24 ` Steve Longerbeam 2019-02-12 10:17 ` Philipp Zabel 2019-02-12 17:42 ` Steve Longerbeam 2019-02-13 10:35 ` Philipp Zabel 2019-02-14 18:54 ` Steve Longerbeam 2019-02-11 23:33 ` Steve Longerbeam 2019-02-09 1:47 ` [PATCH v4 2/4] gpu: ipu-v3: ipu-ic: Simplify selection of encoding matrix Steve Longerbeam 2019-02-11 10:00 ` Philipp Zabel 2019-02-09 1:47 ` [PATCH v4 3/4] gpu: ipu-v3: ipu-ic: Add support for BT.709 encoding Steve Longerbeam 2019-02-11 10:12 ` Philipp Zabel 2019-02-12 1:20 ` Steve Longerbeam 2019-02-12 11:34 ` Philipp Zabel 2019-02-12 17:50 ` Steve Longerbeam 2019-02-09 1:47 ` [PATCH v4 4/4] media: imx: Allow BT.709 encoding for IC routes 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).