| Message ID | 20220927233821.8007-4-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: lcdif: Improve YUV support | expand |
On 9/28/22 01:38, Laurent Pinchart wrote: > Up to and including v1.3, HDMI supported limited quantization range only > for YCbCr. HDMI v1.4 introduced selectable quantization ranges, but this > features isn't supported in the dw-hdmi driver that is used in > conjunction with the LCDIF in the i.MX8MP. The HDMI YCbCr output is thus > always advertised in the AVI infoframe as limited range. > > The LCDIF driver, on the other hand, configures the CSC to produce full > range YCbCr. This mismatch results in loss of details and incorrect > colours. Fix it by switching to limited range YCbCr. > > Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant") > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/mxsfb/lcdif_kms.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c > index 1f22ea5896d5..ba84b51598b3 100644 > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c > @@ -53,16 +53,16 @@ static void lcdif_set_formats(struct lcdif_drm_private *lcdif, > writel(DISP_PARA_LINE_PATTERN_UYVY_H, > lcdif->base + LCDC_V8_DISP_PARA); > > - /* CSC: BT.601 Full Range RGB to YCbCr coefficients. */ > - writel(CSC0_COEF0_A2(0x096) | CSC0_COEF0_A1(0x04c), > + /* CSC: BT.601 Limited Range RGB to YCbCr coefficients. */ > + writel(CSC0_COEF0_A2(0x081) | CSC0_COEF0_A1(0x042), > lcdif->base + LCDC_V8_CSC0_COEF0); > - writel(CSC0_COEF1_B1(0x7d5) | CSC0_COEF1_A3(0x01d), > + writel(CSC0_COEF1_B1(0x7da) | CSC0_COEF1_A3(0x019), > lcdif->base + LCDC_V8_CSC0_COEF1); > - writel(CSC0_COEF2_B3(0x080) | CSC0_COEF2_B2(0x7ac), > + writel(CSC0_COEF2_B3(0x070) | CSC0_COEF2_B2(0x7b6), > lcdif->base + LCDC_V8_CSC0_COEF2); > - writel(CSC0_COEF3_C2(0x795) | CSC0_COEF3_C1(0x080), > + writel(CSC0_COEF3_C2(0x7a2) | CSC0_COEF3_C1(0x070), > lcdif->base + LCDC_V8_CSC0_COEF3); > - writel(CSC0_COEF4_D1(0x000) | CSC0_COEF4_C3(0x7ec), > + writel(CSC0_COEF4_D1(0x010) | CSC0_COEF4_C3(0x7ee), > lcdif->base + LCDC_V8_CSC0_COEF4); > writel(CSC0_COEF5_D3(0x080) | CSC0_COEF5_D2(0x080), > lcdif->base + LCDC_V8_CSC0_COEF5); Would it make sense to use the same coeffs as csc2_coef_bt601_lim in drivers/media/platform/nxp/imx-pxp.c , since the block is most likely identical ?
Hi Marek, On Wed, Sep 28, 2022 at 02:12:19AM +0200, Marek Vasut wrote: > On 9/28/22 01:38, Laurent Pinchart wrote: > > Up to and including v1.3, HDMI supported limited quantization range only > > for YCbCr. HDMI v1.4 introduced selectable quantization ranges, but this > > features isn't supported in the dw-hdmi driver that is used in > > conjunction with the LCDIF in the i.MX8MP. The HDMI YCbCr output is thus > > always advertised in the AVI infoframe as limited range. > > > > The LCDIF driver, on the other hand, configures the CSC to produce full > > range YCbCr. This mismatch results in loss of details and incorrect > > colours. Fix it by switching to limited range YCbCr. > > > > Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant") > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > drivers/gpu/drm/mxsfb/lcdif_kms.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c > > index 1f22ea5896d5..ba84b51598b3 100644 > > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c > > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c > > @@ -53,16 +53,16 @@ static void lcdif_set_formats(struct lcdif_drm_private *lcdif, > > writel(DISP_PARA_LINE_PATTERN_UYVY_H, > > lcdif->base + LCDC_V8_DISP_PARA); > > > > - /* CSC: BT.601 Full Range RGB to YCbCr coefficients. */ > > - writel(CSC0_COEF0_A2(0x096) | CSC0_COEF0_A1(0x04c), > > + /* CSC: BT.601 Limited Range RGB to YCbCr coefficients. */ > > + writel(CSC0_COEF0_A2(0x081) | CSC0_COEF0_A1(0x042), > > lcdif->base + LCDC_V8_CSC0_COEF0); > > - writel(CSC0_COEF1_B1(0x7d5) | CSC0_COEF1_A3(0x01d), > > + writel(CSC0_COEF1_B1(0x7da) | CSC0_COEF1_A3(0x019), > > lcdif->base + LCDC_V8_CSC0_COEF1); > > - writel(CSC0_COEF2_B3(0x080) | CSC0_COEF2_B2(0x7ac), > > + writel(CSC0_COEF2_B3(0x070) | CSC0_COEF2_B2(0x7b6), > > lcdif->base + LCDC_V8_CSC0_COEF2); > > - writel(CSC0_COEF3_C2(0x795) | CSC0_COEF3_C1(0x080), > > + writel(CSC0_COEF3_C2(0x7a2) | CSC0_COEF3_C1(0x070), > > lcdif->base + LCDC_V8_CSC0_COEF3); > > - writel(CSC0_COEF4_D1(0x000) | CSC0_COEF4_C3(0x7ec), > > + writel(CSC0_COEF4_D1(0x010) | CSC0_COEF4_C3(0x7ee), > > lcdif->base + LCDC_V8_CSC0_COEF4); > > writel(CSC0_COEF5_D3(0x080) | CSC0_COEF5_D2(0x080), > > lcdif->base + LCDC_V8_CSC0_COEF5); > > Would it make sense to use the same coeffs as csc2_coef_bt601_lim in > drivers/media/platform/nxp/imx-pxp.c , since the block is most likely > identical ? The coefficients in this patch have been computed to distribute the error in such a way that the sum of all lines stays the same. This avoids biases and overflow, but it likely makes very little difference in practice. I'm thus fine with the coefficients from imx-pxp.c.
On 9/28/22 02:21, Laurent Pinchart wrote: Hi, [...] >>> - /* CSC: BT.601 Full Range RGB to YCbCr coefficients. */ >>> - writel(CSC0_COEF0_A2(0x096) | CSC0_COEF0_A1(0x04c), >>> + /* CSC: BT.601 Limited Range RGB to YCbCr coefficients. */ >>> + writel(CSC0_COEF0_A2(0x081) | CSC0_COEF0_A1(0x042), >>> lcdif->base + LCDC_V8_CSC0_COEF0); >>> - writel(CSC0_COEF1_B1(0x7d5) | CSC0_COEF1_A3(0x01d), >>> + writel(CSC0_COEF1_B1(0x7da) | CSC0_COEF1_A3(0x019), >>> lcdif->base + LCDC_V8_CSC0_COEF1); >>> - writel(CSC0_COEF2_B3(0x080) | CSC0_COEF2_B2(0x7ac), >>> + writel(CSC0_COEF2_B3(0x070) | CSC0_COEF2_B2(0x7b6), >>> lcdif->base + LCDC_V8_CSC0_COEF2); >>> - writel(CSC0_COEF3_C2(0x795) | CSC0_COEF3_C1(0x080), >>> + writel(CSC0_COEF3_C2(0x7a2) | CSC0_COEF3_C1(0x070), >>> lcdif->base + LCDC_V8_CSC0_COEF3); >>> - writel(CSC0_COEF4_D1(0x000) | CSC0_COEF4_C3(0x7ec), >>> + writel(CSC0_COEF4_D1(0x010) | CSC0_COEF4_C3(0x7ee), >>> lcdif->base + LCDC_V8_CSC0_COEF4); >>> writel(CSC0_COEF5_D3(0x080) | CSC0_COEF5_D2(0x080), >>> lcdif->base + LCDC_V8_CSC0_COEF5); >> >> Would it make sense to use the same coeffs as csc2_coef_bt601_lim in >> drivers/media/platform/nxp/imx-pxp.c , since the block is most likely >> identical ? > > The coefficients in this patch have been computed to distribute the > error in such a way that the sum of all lines stays the same. This > avoids biases and overflow, but it likely makes very little difference > in practice. I'm thus fine with the coefficients from imx-pxp.c. Would it then make sense to update the coeffs in the pxp driver instead? Either option works for me.
On Wed, Sep 28, 2022 at 02:37:04AM +0200, Marek Vasut wrote: > On 9/28/22 02:21, Laurent Pinchart wrote: > > Hi, > > [...] > > >>> - /* CSC: BT.601 Full Range RGB to YCbCr coefficients. */ > >>> - writel(CSC0_COEF0_A2(0x096) | CSC0_COEF0_A1(0x04c), > >>> + /* CSC: BT.601 Limited Range RGB to YCbCr coefficients. */ > >>> + writel(CSC0_COEF0_A2(0x081) | CSC0_COEF0_A1(0x042), > >>> lcdif->base + LCDC_V8_CSC0_COEF0); > >>> - writel(CSC0_COEF1_B1(0x7d5) | CSC0_COEF1_A3(0x01d), > >>> + writel(CSC0_COEF1_B1(0x7da) | CSC0_COEF1_A3(0x019), > >>> lcdif->base + LCDC_V8_CSC0_COEF1); > >>> - writel(CSC0_COEF2_B3(0x080) | CSC0_COEF2_B2(0x7ac), > >>> + writel(CSC0_COEF2_B3(0x070) | CSC0_COEF2_B2(0x7b6), > >>> lcdif->base + LCDC_V8_CSC0_COEF2); > >>> - writel(CSC0_COEF3_C2(0x795) | CSC0_COEF3_C1(0x080), > >>> + writel(CSC0_COEF3_C2(0x7a2) | CSC0_COEF3_C1(0x070), > >>> lcdif->base + LCDC_V8_CSC0_COEF3); > >>> - writel(CSC0_COEF4_D1(0x000) | CSC0_COEF4_C3(0x7ec), > >>> + writel(CSC0_COEF4_D1(0x010) | CSC0_COEF4_C3(0x7ee), > >>> lcdif->base + LCDC_V8_CSC0_COEF4); > >>> writel(CSC0_COEF5_D3(0x080) | CSC0_COEF5_D2(0x080), > >>> lcdif->base + LCDC_V8_CSC0_COEF5); > >> > >> Would it make sense to use the same coeffs as csc2_coef_bt601_lim in > >> drivers/media/platform/nxp/imx-pxp.c , since the block is most likely > >> identical ? > > > > The coefficients in this patch have been computed to distribute the > > error in such a way that the sum of all lines stays the same. This > > avoids biases and overflow, but it likely makes very little difference > > in practice. I'm thus fine with the coefficients from imx-pxp.c. > > Would it then make sense to update the coeffs in the pxp driver instead? > > Either option works for me. It could, but I won't be able to easily test it. As the hardware clamps the calculated value, there's no risk of wraparound, and the difference in the +/-1 error distribution will not be noticeable, so I'll just copy the coefficients from imx-pxp.c to ensure coherency.
On 9/28/22 02:40, Laurent Pinchart wrote: > On Wed, Sep 28, 2022 at 02:37:04AM +0200, Marek Vasut wrote: >> On 9/28/22 02:21, Laurent Pinchart wrote: >> >> Hi, >> >> [...] >> >>>>> - /* CSC: BT.601 Full Range RGB to YCbCr coefficients. */ >>>>> - writel(CSC0_COEF0_A2(0x096) | CSC0_COEF0_A1(0x04c), >>>>> + /* CSC: BT.601 Limited Range RGB to YCbCr coefficients. */ >>>>> + writel(CSC0_COEF0_A2(0x081) | CSC0_COEF0_A1(0x042), >>>>> lcdif->base + LCDC_V8_CSC0_COEF0); >>>>> - writel(CSC0_COEF1_B1(0x7d5) | CSC0_COEF1_A3(0x01d), >>>>> + writel(CSC0_COEF1_B1(0x7da) | CSC0_COEF1_A3(0x019), >>>>> lcdif->base + LCDC_V8_CSC0_COEF1); >>>>> - writel(CSC0_COEF2_B3(0x080) | CSC0_COEF2_B2(0x7ac), >>>>> + writel(CSC0_COEF2_B3(0x070) | CSC0_COEF2_B2(0x7b6), >>>>> lcdif->base + LCDC_V8_CSC0_COEF2); >>>>> - writel(CSC0_COEF3_C2(0x795) | CSC0_COEF3_C1(0x080), >>>>> + writel(CSC0_COEF3_C2(0x7a2) | CSC0_COEF3_C1(0x070), >>>>> lcdif->base + LCDC_V8_CSC0_COEF3); >>>>> - writel(CSC0_COEF4_D1(0x000) | CSC0_COEF4_C3(0x7ec), >>>>> + writel(CSC0_COEF4_D1(0x010) | CSC0_COEF4_C3(0x7ee), >>>>> lcdif->base + LCDC_V8_CSC0_COEF4); >>>>> writel(CSC0_COEF5_D3(0x080) | CSC0_COEF5_D2(0x080), >>>>> lcdif->base + LCDC_V8_CSC0_COEF5); >>>> >>>> Would it make sense to use the same coeffs as csc2_coef_bt601_lim in >>>> drivers/media/platform/nxp/imx-pxp.c , since the block is most likely >>>> identical ? >>> >>> The coefficients in this patch have been computed to distribute the >>> error in such a way that the sum of all lines stays the same. This >>> avoids biases and overflow, but it likely makes very little difference >>> in practice. I'm thus fine with the coefficients from imx-pxp.c. >> >> Would it then make sense to update the coeffs in the pxp driver instead? >> >> Either option works for me. > > It could, but I won't be able to easily test it. As the hardware clamps > the calculated value, there's no risk of wraparound, and the difference > in the +/-1 error distribution will not be noticeable, so I'll just copy > the coefficients from imx-pxp.c to ensure coherency. That works too, thanks !
diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c index 1f22ea5896d5..ba84b51598b3 100644 --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c @@ -53,16 +53,16 @@ static void lcdif_set_formats(struct lcdif_drm_private *lcdif, writel(DISP_PARA_LINE_PATTERN_UYVY_H, lcdif->base + LCDC_V8_DISP_PARA); - /* CSC: BT.601 Full Range RGB to YCbCr coefficients. */ - writel(CSC0_COEF0_A2(0x096) | CSC0_COEF0_A1(0x04c), + /* CSC: BT.601 Limited Range RGB to YCbCr coefficients. */ + writel(CSC0_COEF0_A2(0x081) | CSC0_COEF0_A1(0x042), lcdif->base + LCDC_V8_CSC0_COEF0); - writel(CSC0_COEF1_B1(0x7d5) | CSC0_COEF1_A3(0x01d), + writel(CSC0_COEF1_B1(0x7da) | CSC0_COEF1_A3(0x019), lcdif->base + LCDC_V8_CSC0_COEF1); - writel(CSC0_COEF2_B3(0x080) | CSC0_COEF2_B2(0x7ac), + writel(CSC0_COEF2_B3(0x070) | CSC0_COEF2_B2(0x7b6), lcdif->base + LCDC_V8_CSC0_COEF2); - writel(CSC0_COEF3_C2(0x795) | CSC0_COEF3_C1(0x080), + writel(CSC0_COEF3_C2(0x7a2) | CSC0_COEF3_C1(0x070), lcdif->base + LCDC_V8_CSC0_COEF3); - writel(CSC0_COEF4_D1(0x000) | CSC0_COEF4_C3(0x7ec), + writel(CSC0_COEF4_D1(0x010) | CSC0_COEF4_C3(0x7ee), lcdif->base + LCDC_V8_CSC0_COEF4); writel(CSC0_COEF5_D3(0x080) | CSC0_COEF5_D2(0x080), lcdif->base + LCDC_V8_CSC0_COEF5);
Up to and including v1.3, HDMI supported limited quantization range only for YCbCr. HDMI v1.4 introduced selectable quantization ranges, but this features isn't supported in the dw-hdmi driver that is used in conjunction with the LCDIF in the i.MX8MP. The HDMI YCbCr output is thus always advertised in the AVI infoframe as limited range. The LCDIF driver, on the other hand, configures the CSC to produce full range YCbCr. This mismatch results in loss of details and incorrect colours. Fix it by switching to limited range YCbCr. Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant") Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/gpu/drm/mxsfb/lcdif_kms.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)