* ✗ Fi.CI.CHECKPATCH: warning for Enable Plane Input CSC for ICL (rev4)
2018-10-26 10:01 [v5 0/2] Enable Plane Input CSC for ICL Uma Shankar
@ 2018-10-26 9:50 ` Patchwork
2018-10-26 9:52 ` ✗ Fi.CI.SPARSE: " Patchwork
` (4 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2018-10-26 9:50 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx
== Series Details ==
Series: Enable Plane Input CSC for ICL (rev4)
URL : https://patchwork.freedesktop.org/series/51463/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
d1602d4945dc drm/i915/icl: Define Plane Input CSC Coefficient Registers
-:56: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'pipe' - possible side-effects?
#56: FILE: drivers/gpu/drm/i915/i915_reg.h:6603:
+#define PLANE_INPUT_CSC_COEFF(pipe, plane, index) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RY_GY_1(pipe) + (index) * 4, \
+ _PLANE_INPUT_CSC_RY_GY_2(pipe) + (index) * 4)
-:56: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'index' - possible side-effects?
#56: FILE: drivers/gpu/drm/i915/i915_reg.h:6603:
+#define PLANE_INPUT_CSC_COEFF(pipe, plane, index) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RY_GY_1(pipe) + (index) * 4, \
+ _PLANE_INPUT_CSC_RY_GY_2(pipe) + (index) * 4)
-:72: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'pipe' - possible side-effects?
#72: FILE: drivers/gpu/drm/i915/i915_reg.h:6619:
+#define PLANE_INPUT_CSC_PREOFF(pipe, plane, index) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_HI_1(pipe) + (index) * 4, \
+ _PLANE_INPUT_CSC_PREOFF_HI_2(pipe) + (index) * 4)
-:72: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'index' - possible side-effects?
#72: FILE: drivers/gpu/drm/i915/i915_reg.h:6619:
+#define PLANE_INPUT_CSC_PREOFF(pipe, plane, index) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_HI_1(pipe) + (index) * 4, \
+ _PLANE_INPUT_CSC_PREOFF_HI_2(pipe) + (index) * 4)
-:88: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'pipe' - possible side-effects?
#88: FILE: drivers/gpu/drm/i915/i915_reg.h:6635:
+#define PLANE_INPUT_CSC_POSTOFF(pipe, plane, index) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_HI_1(pipe) + (index) * 4, \
+ _PLANE_INPUT_CSC_POSTOFF_HI_2(pipe) + (index) * 4)
-:88: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'index' - possible side-effects?
#88: FILE: drivers/gpu/drm/i915/i915_reg.h:6635:
+#define PLANE_INPUT_CSC_POSTOFF(pipe, plane, index) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_HI_1(pipe) + (index) * 4, \
+ _PLANE_INPUT_CSC_POSTOFF_HI_2(pipe) + (index) * 4)
total: 0 errors, 0 warnings, 6 checks, 62 lines checked
b6ccca5a6aa4 drm/i915/icl: Enable Plane Input CSC for YUV to RGB Conversion
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* ✗ Fi.CI.SPARSE: warning for Enable Plane Input CSC for ICL (rev4)
2018-10-26 10:01 [v5 0/2] Enable Plane Input CSC for ICL Uma Shankar
2018-10-26 9:50 ` ✗ Fi.CI.CHECKPATCH: warning for Enable Plane Input CSC for ICL (rev4) Patchwork
@ 2018-10-26 9:52 ` Patchwork
2018-10-26 10:01 ` [v5 1/2] drm/i915/icl: Define Plane Input CSC Coefficient Registers Uma Shankar
` (3 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2018-10-26 9:52 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx
== Series Details ==
Series: Enable Plane Input CSC for ICL (rev4)
URL : https://patchwork.freedesktop.org/series/51463/
State : warning
== Summary ==
$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915/icl: Define Plane Input CSC Coefficient Registers
Okay!
Commit: drm/i915/icl: Enable Plane Input CSC for YUV to RGB Conversion
+drivers/gpu/drm/i915/intel_color.c:105:33: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_color.c:105:33: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_color.c:105:33: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_color.c:105:33: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_color.c:105:33: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_color.c:105:33: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_color.c:105:33: warning: expression using sizeof(void)
-drivers/gpu/drm/i915/intel_color.c:105:33: warning: expression using sizeof(void)
-drivers/gpu/drm/i915/intel_color.c:105:33: warning: expression using sizeof(void)
-drivers/gpu/drm/i915/intel_color.c:105:33: warning: expression using sizeof(void)
-drivers/gpu/drm/i915/intel_color.c:105:33: warning: expression using sizeof(void)
-drivers/gpu/drm/i915/intel_color.c:105:33: warning: expression using sizeof(void)
-drivers/gpu/drm/i915/intel_color.c:105:33: warning: expression using sizeof(void)
-drivers/gpu/drm/i915/intel_color.c:105:33: warning: expression using sizeof(void)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* [v5 0/2] Enable Plane Input CSC for ICL
@ 2018-10-26 10:01 Uma Shankar
2018-10-26 9:50 ` ✗ Fi.CI.CHECKPATCH: warning for Enable Plane Input CSC for ICL (rev4) Patchwork
` (5 more replies)
0 siblings, 6 replies; 13+ messages in thread
From: Uma Shankar @ 2018-10-26 10:01 UTC (permalink / raw)
To: intel-gfx; +Cc: ville.syrjala, maarten.lankhorst
This patch series enables plane input csc feature for
ICL. This is needed for YUV to RGB conversion on bottom
3 planes on ICL, other planes are handled in the legacy
way using fixed function hardware.
This series enables color conversion for Full Range YUV data,
limited range handling will be done as a separate patch.
v2: Separated the patch into 2 parts as per Maarten's comments.
Addressed Ville and Maarten's review comment.
v3: Redesigned the register macro definition as per Matt's comment.
Addressed Maarten's review comment.
v4: Added support for Limited Range Color Handling.
v5: Fixed Matt and Maarten's review comments.
Note: This is currently untested and floated to get feedback
on the design and implementation for this feature. In parallel,
I will test this on actual ICL hardware and confirm with planar
formats.
Uma Shankar (2):
drm/i915/icl: Define Plane Input CSC Coefficient Registers
drm/i915/icl: Enable Plane Input CSC for YUV to RGB Conversion
drivers/gpu/drm/i915/i915_reg.h | 50 +++++++++++++++++++++++
drivers/gpu/drm/i915/intel_color.c | 79 ++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_display.c | 23 ++++++++---
drivers/gpu/drm/i915/intel_drv.h | 2 +
4 files changed, 148 insertions(+), 6 deletions(-)
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* [v5 1/2] drm/i915/icl: Define Plane Input CSC Coefficient Registers
2018-10-26 10:01 [v5 0/2] Enable Plane Input CSC for ICL Uma Shankar
2018-10-26 9:50 ` ✗ Fi.CI.CHECKPATCH: warning for Enable Plane Input CSC for ICL (rev4) Patchwork
2018-10-26 9:52 ` ✗ Fi.CI.SPARSE: " Patchwork
@ 2018-10-26 10:01 ` Uma Shankar
2018-10-26 10:01 ` [v5 2/2] drm/i915/icl: Enable Plane Input CSC for YUV to RGB Conversion Uma Shankar
` (2 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Uma Shankar @ 2018-10-26 10:01 UTC (permalink / raw)
To: intel-gfx; +Cc: ville.syrjala, maarten.lankhorst
Defined the plane input csc coefficient registers and macros.
6 registers are used to program a total of 9 coefficients,
added macros to define each of them for all the planes
supporting the feature on pipes. On ICL, bottom 3 planes
have this capability.
v2: Segregated the register macro definition as separate patch
as per Maarten's suggestion.
v3: Removed a redundant 3rd Pipe register definition and
simplified the equally spaced register definition by adding an
offset as per Matt's comment.
v4: No Change
v5: Renamed the register Macro as per Matt's suggestion.
Signed-off-by: Uma Shankar <uma.shankar@intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 50 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 69eb573..87c275c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6569,6 +6569,7 @@ enum {
#define _PLANE_COLOR_CTL_3_A 0x703CC /* GLK+ */
#define PLANE_COLOR_PIPE_GAMMA_ENABLE (1 << 30) /* Pre-ICL */
#define PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE (1 << 28)
+#define PLANE_COLOR_INPUT_CSC_ENABLE (1 << 20) /* ICL+ */
#define PLANE_COLOR_PIPE_CSC_ENABLE (1 << 23) /* Pre-ICL */
#define PLANE_COLOR_CSC_MODE_BYPASS (0 << 17)
#define PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709 (1 << 17)
@@ -6585,6 +6586,55 @@ enum {
#define _PLANE_NV12_BUF_CFG_1_A 0x70278
#define _PLANE_NV12_BUF_CFG_2_A 0x70378
+/* Input CSC Register Definitions */
+#define _PLANE_INPUT_CSC_RY_GY_1_A 0x701E0
+#define _PLANE_INPUT_CSC_RY_GY_2_A 0x702E0
+
+#define _PLANE_INPUT_CSC_RY_GY_1_B 0x711E0
+#define _PLANE_INPUT_CSC_RY_GY_2_B 0x712E0
+
+#define _PLANE_INPUT_CSC_RY_GY_1(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_RY_GY_1_A, \
+ _PLANE_INPUT_CSC_RY_GY_1_B)
+#define _PLANE_INPUT_CSC_RY_GY_2(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_RY_GY_2_A, \
+ _PLANE_INPUT_CSC_RY_GY_2_B)
+
+#define PLANE_INPUT_CSC_COEFF(pipe, plane, index) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RY_GY_1(pipe) + (index) * 4, \
+ _PLANE_INPUT_CSC_RY_GY_2(pipe) + (index) * 4)
+
+#define _PLANE_INPUT_CSC_PREOFF_HI_1_A 0x701F8
+#define _PLANE_INPUT_CSC_PREOFF_HI_2_A 0x702F8
+
+#define _PLANE_INPUT_CSC_PREOFF_HI_1_B 0x711F8
+#define _PLANE_INPUT_CSC_PREOFF_HI_2_B 0x712F8
+
+#define _PLANE_INPUT_CSC_PREOFF_HI_1(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_HI_1_A, \
+ _PLANE_INPUT_CSC_PREOFF_HI_1_B)
+#define _PLANE_INPUT_CSC_PREOFF_HI_2(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_HI_2_A, \
+ _PLANE_INPUT_CSC_PREOFF_HI_2_B)
+#define PLANE_INPUT_CSC_PREOFF(pipe, plane, index) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_HI_1(pipe) + (index) * 4, \
+ _PLANE_INPUT_CSC_PREOFF_HI_2(pipe) + (index) * 4)
+
+#define _PLANE_INPUT_CSC_POSTOFF_HI_1_A 0x70204
+#define _PLANE_INPUT_CSC_POSTOFF_HI_2_A 0x70304
+
+#define _PLANE_INPUT_CSC_POSTOFF_HI_1_B 0x71204
+#define _PLANE_INPUT_CSC_POSTOFF_HI_2_B 0x71304
+
+#define _PLANE_INPUT_CSC_POSTOFF_HI_1(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_HI_1_A, \
+ _PLANE_INPUT_CSC_POSTOFF_HI_1_B)
+#define _PLANE_INPUT_CSC_POSTOFF_HI_2(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_HI_2_A, \
+ _PLANE_INPUT_CSC_POSTOFF_HI_2_B)
+#define PLANE_INPUT_CSC_POSTOFF(pipe, plane, index) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_HI_1(pipe) + (index) * 4, \
+ _PLANE_INPUT_CSC_POSTOFF_HI_2(pipe) + (index) * 4)
#define _PLANE_CTL_1_B 0x71180
#define _PLANE_CTL_2_B 0x71280
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [v5 2/2] drm/i915/icl: Enable Plane Input CSC for YUV to RGB Conversion
2018-10-26 10:01 [v5 0/2] Enable Plane Input CSC for ICL Uma Shankar
` (2 preceding siblings ...)
2018-10-26 10:01 ` [v5 1/2] drm/i915/icl: Define Plane Input CSC Coefficient Registers Uma Shankar
@ 2018-10-26 10:01 ` Uma Shankar
2018-10-29 23:29 ` Matt Roper
2018-10-30 10:55 ` Maarten Lankhorst
2018-10-26 10:08 ` ✓ Fi.CI.BAT: success for Enable Plane Input CSC for ICL (rev4) Patchwork
2018-10-26 16:28 ` ✓ Fi.CI.IGT: " Patchwork
5 siblings, 2 replies; 13+ messages in thread
From: Uma Shankar @ 2018-10-26 10:01 UTC (permalink / raw)
To: intel-gfx; +Cc: ville.syrjala, maarten.lankhorst
Plane input CSC needs to be enabled to convert frambuffers from
YUV to RGB. This is needed for bottom 3 planes on ICL, rest of
the planes have hardcoded conversion and taken care by the legacy
code.
This patch defines the co-efficient values for YUV to RGB conversion
in BT709 and BT601 formats. It programs the coefficients and enables
the plane input csc unit in hardware.
Note: This is currently untested and floated to get an early feedback
on the design and implementation for this feature. In parallel,
I will test this on actual ICL hardware and confirm with planar
formats.
v2: Addressed Maarten's and Ville's review comments and added the
coefficients in a 2D array instead of independent Macros.
v3: Added individual coefficient matrix (9 values) instead of 6
register values as per Maarten's comment. Also addresed a shift
issue with B channel coefficient.
v4: Added support for Limited Range Color Handling
v5: Fixed Matt and Maarten's review comments.
Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
drivers/gpu/drm/i915/intel_color.c | 79 ++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_display.c | 23 ++++++++---
drivers/gpu/drm/i915/intel_drv.h | 2 +
3 files changed, 98 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 5127da2..681cd13 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -57,6 +57,15 @@
#define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8
#define CSC_RGB_TO_YUV_BV 0x1e080000
+#define ROFF(x) (((x) & 0xffff) << 16)
+#define GOFF(x) (((x) & 0xffff) << 0)
+#define BOFF(x) (((x) & 0xffff) << 16)
+
+/* Preoffset values for YUV to RGB Conversion */
+#define PREOFF_YUV_TO_RGB_HI 0x1800
+#define PREOFF_YUV_TO_RGB_ME 0x1F00
+#define PREOFF_YUV_TO_RGB_LO 0x1800
+
/*
* Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
* format). This macro takes the coefficient we want transformed and the
@@ -643,6 +652,76 @@ int intel_color_check(struct drm_crtc *crtc,
return -EINVAL;
}
+void icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
+ const struct intel_plane_state *plane_state)
+{
+ struct drm_i915_private *dev_priv =
+ to_i915(plane_state->base.plane->dev);
+ struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+ enum pipe pipe = crtc->pipe;
+ struct intel_plane *intel_plane =
+ to_intel_plane(plane_state->base.plane);
+ enum plane_id plane = intel_plane->id;
+
+ static const u16 input_csc_matrix[][9] = {
+ /* BT.601 full range YCbCr -> full range RGB */
+ [DRM_COLOR_YCBCR_BT601] = {
+ 0x7AF8, 0x7800, 0x0,
+ 0x8B28, 0x7800, 0x9AC0,
+ 0x0, 0x7800, 0x7DD8,
+ },
+ /* BT.709 full range YCbCr -> full range RGB */
+ [DRM_COLOR_YCBCR_BT709] = {
+ 0x7C98, 0x7800, 0x0,
+ 0x9EF8, 0x7800, 0xABF8,
+ 0x0, 0x7800, 0x7ED8,
+ },
+ };
+
+ /* Matrix for Limited Range to Full Range Conversion */
+ static const u16 input_csc_matrix_lr[][9] = {
+ /* BT.601 Limted range YCbCr -> full range RGB */
+ [DRM_COLOR_YCBCR_BT601] = {
+ 0x7CC8, 0x7950, 0x0,
+ 0x8CB8, 0x7918, 0x9C40,
+ 0x0, 0x7918, 0x7FC8,
+ },
+ /* BT.709 Limited range YCbCr -> full range RGB */
+ [DRM_COLOR_YCBCR_BT709] = {
+ 0x7EA8, 0x7950, 0x0,
+ 0x8888, 0x7918, 0xADA8,
+ 0x0, 0x7918, 0x6870,
+ },
+ };
+ const u16 *csc;
+
+ if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
+ csc = input_csc_matrix[plane_state->base.color_encoding];
+ else
+ csc = input_csc_matrix_lr[plane_state->base.color_encoding];
+
+ I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 0), ROFF(csc[0]) |
+ GOFF(csc[1]));
+ I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 1), BOFF(csc[2]));
+ I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 2), ROFF(csc[3]) |
+ GOFF(csc[4]));
+ I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 3), BOFF(csc[5]));
+ I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 4), ROFF(csc[6]) |
+ GOFF(csc[7]));
+ I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 5), BOFF(csc[8]));
+
+ I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 0),
+ PREOFF_YUV_TO_RGB_HI);
+ I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 1),
+ PREOFF_YUV_TO_RGB_ME);
+ I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 2),
+ PREOFF_YUV_TO_RGB_LO);
+
+ I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 0), 0x0);
+ I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 1), 0x0);
+ I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 2), 0x0);
+}
+
void intel_color_init(struct drm_crtc *crtc)
{
struct drm_i915_private *dev_priv = to_i915(crtc->dev);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fe045ab..d16a064 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3666,6 +3666,7 @@ u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
struct drm_i915_private *dev_priv =
to_i915(plane_state->base.plane->dev);
const struct drm_framebuffer *fb = plane_state->base.fb;
+ struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
u32 plane_color_ctl = 0;
if (INTEL_GEN(dev_priv) < 11) {
@@ -3676,13 +3677,23 @@ u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state);
if (fb->format->is_yuv) {
- if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
- plane_color_ctl |= PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
- else
- plane_color_ctl |= PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
+ if (!icl_is_hdr_plane(plane)) {
+ if (plane_state->base.color_encoding ==
+ DRM_COLOR_YCBCR_BT709)
+ plane_color_ctl |=
+ PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
+ else
+ plane_color_ctl |=
+ PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
- if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
- plane_color_ctl |= PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
+ if (plane_state->base.color_range ==
+ DRM_COLOR_YCBCR_FULL_RANGE)
+ plane_color_ctl |=
+ PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
+ } else {
+ icl_program_input_csc_coeff(crtc_state, plane_state);
+ plane_color_ctl |= PLANE_COLOR_INPUT_CSC_ENABLE;
+ }
}
return plane_color_ctl;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index db24308..bd9e946 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2285,6 +2285,8 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
int intel_color_check(struct drm_crtc *crtc, struct drm_crtc_state *state);
void intel_color_set_csc(struct drm_crtc_state *crtc_state);
void intel_color_load_luts(struct drm_crtc_state *crtc_state);
+void icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
+ const struct intel_plane_state *plane_state);
/* intel_lspcon.c */
bool lspcon_init(struct intel_digital_port *intel_dig_port);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 13+ messages in thread
* ✓ Fi.CI.BAT: success for Enable Plane Input CSC for ICL (rev4)
2018-10-26 10:01 [v5 0/2] Enable Plane Input CSC for ICL Uma Shankar
` (3 preceding siblings ...)
2018-10-26 10:01 ` [v5 2/2] drm/i915/icl: Enable Plane Input CSC for YUV to RGB Conversion Uma Shankar
@ 2018-10-26 10:08 ` Patchwork
2018-10-26 16:28 ` ✓ Fi.CI.IGT: " Patchwork
5 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2018-10-26 10:08 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx
== Series Details ==
Series: Enable Plane Input CSC for ICL (rev4)
URL : https://patchwork.freedesktop.org/series/51463/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_5041 -> Patchwork_10599 =
== Summary - SUCCESS ==
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/51463/revisions/4/mbox/
== Known issues ==
Here are the changes found in Patchwork_10599 that come from known issues:
=== IGT changes ===
==== Possible fixes ====
igt@gem_exec_suspend@basic-s4-devices:
fi-blb-e6850: INCOMPLETE (fdo#107718) -> PASS
fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
== Participating hosts (44 -> 42) ==
Additional (1): fi-byt-j1900
Missing (3): fi-ilk-m540 fi-bsw-cyan fi-glk-j4005
== Build changes ==
* Linux: CI_DRM_5041 -> Patchwork_10599
CI_DRM_5041: a45f611eae66546bb297a81c88e89bfd409bf199 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4695: 81b66cf2806d6a8e9516580fb31879677487d32b @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10599: b6ccca5a6aa430884a1a147e598222f134403cec @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
b6ccca5a6aa4 drm/i915/icl: Enable Plane Input CSC for YUV to RGB Conversion
d1602d4945dc drm/i915/icl: Define Plane Input CSC Coefficient Registers
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10599/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* ✓ Fi.CI.IGT: success for Enable Plane Input CSC for ICL (rev4)
2018-10-26 10:01 [v5 0/2] Enable Plane Input CSC for ICL Uma Shankar
` (4 preceding siblings ...)
2018-10-26 10:08 ` ✓ Fi.CI.BAT: success for Enable Plane Input CSC for ICL (rev4) Patchwork
@ 2018-10-26 16:28 ` Patchwork
5 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2018-10-26 16:28 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx
== Series Details ==
Series: Enable Plane Input CSC for ICL (rev4)
URL : https://patchwork.freedesktop.org/series/51463/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_5041_full -> Patchwork_10599_full =
== Summary - WARNING ==
Minor unknown changes coming with Patchwork_10599_full need to be verified
manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_10599_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
== Possible new issues ==
Here are the unknown changes that may have been introduced in Patchwork_10599_full:
=== IGT changes ===
==== Warnings ====
igt@pm_rc6_residency@rc6-accuracy:
shard-snb: PASS -> SKIP
== Known issues ==
Here are the changes found in Patchwork_10599_full that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@gem_busy@close-race:
shard-apl: PASS -> DMESG-FAIL (fdo#108561)
igt@gem_exec_schedule@pi-ringfull-bsd:
shard-skl: NOTRUN -> FAIL (fdo#103158)
igt@kms_atomic_transition@1x-modeset-transitions:
shard-skl: NOTRUN -> FAIL (fdo#107815, fdo#108470)
igt@kms_busy@basic-modeset-a:
shard-apl: PASS -> DMESG-WARN (fdo#108549) +9
igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-c:
shard-skl: NOTRUN -> DMESG-WARN (fdo#107956) +1
igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
shard-glk: PASS -> FAIL (fdo#108145)
igt@kms_color@pipe-a-ctm-max:
shard-apl: PASS -> FAIL (fdo#108147)
igt@kms_color@pipe-c-legacy-gamma:
shard-apl: PASS -> FAIL (fdo#104782)
igt@kms_cursor_crc@cursor-128x128-suspend:
shard-apl: PASS -> FAIL (fdo#103232, fdo#103191)
igt@kms_cursor_crc@cursor-64x64-random:
shard-apl: PASS -> FAIL (fdo#103232)
igt@kms_fbcon_fbt@psr:
shard-skl: NOTRUN -> FAIL (fdo#107882)
igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-move:
shard-glk: PASS -> FAIL (fdo#103167)
igt@kms_frontbuffer_tracking@psr-suspend:
shard-skl: NOTRUN -> INCOMPLETE (fdo#107773, fdo#106978, fdo#104108)
igt@kms_plane@pixel-format-pipe-b-planes:
shard-skl: NOTRUN -> DMESG-FAIL (fdo#103166, fdo#106885) +1
igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb:
shard-skl: NOTRUN -> FAIL (fdo#108145) +3
igt@kms_plane_multiple@atomic-pipe-a-tiling-yf:
shard-apl: PASS -> FAIL (fdo#103166)
igt@kms_setmode@basic:
shard-skl: NOTRUN -> FAIL (fdo#99912)
igt@kms_vblank@pipe-b-ts-continuation-modeset-hang:
shard-apl: PASS -> DMESG-FAIL (fdo#108549)
igt@perf@blocking:
shard-hsw: PASS -> FAIL (fdo#102252)
igt@pm_rpm@gem-idle:
shard-skl: NOTRUN -> INCOMPLETE (fdo#107807)
igt@pm_rpm@modeset-non-lpsp-stress:
shard-skl: SKIP -> INCOMPLETE (fdo#107807)
==== Possible fixes ====
igt@drv_suspend@shrink:
shard-glk: INCOMPLETE (k.org#198133, fdo#103359, fdo#106886) -> PASS
igt@gem_busy@close-race:
shard-glk: DMESG-FAIL (fdo#108561) -> PASS
igt@kms_ccs@pipe-a-crc-primary-basic:
shard-skl: FAIL (fdo#107725) -> PASS
igt@kms_chv_cursor_fail@pipe-a-64x64-left-edge:
shard-skl: FAIL (fdo#104671) -> PASS
igt@kms_cursor_crc@cursor-128x42-sliding:
shard-apl: FAIL (fdo#103232) -> PASS +2
igt@kms_cursor_crc@cursor-64x21-random:
shard-apl: DMESG-WARN (fdo#108549) -> PASS +10
igt@kms_flip@plain-flip-fb-recreate-interruptible:
shard-skl: FAIL (fdo#100368) -> PASS
igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-plflip-blt:
shard-skl: FAIL (fdo#105682) -> PASS
igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-cpu:
shard-apl: FAIL (fdo#103167) -> PASS
igt@kms_frontbuffer_tracking@fbc-farfromfence:
shard-skl: FAIL (fdo#103167) -> PASS +1
igt@kms_frontbuffer_tracking@fbc-rgb565-draw-render:
shard-skl: FAIL (fdo#105682) -> SKIP
igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
shard-apl: FAIL (fdo#103166) -> PASS +1
igt@kms_vblank@pipe-b-ts-continuation-modeset-rpm:
shard-apl: DMESG-FAIL (fdo#108549) -> PASS
==== Warnings ====
igt@kms_cursor_crc@cursor-64x64-sliding:
shard-apl: DMESG-WARN (fdo#108549) -> FAIL (fdo#103232)
igt@kms_frontbuffer_tracking@fbc-1p-rte:
shard-apl: DMESG-WARN (fdo#108549) -> DMESG-FAIL (fdo#103167)
igt@kms_plane_alpha_blend@pipe-c-alpha-opaque-fb:
shard-apl: DMESG-FAIL (fdo#108549, fdo#108145) -> FAIL (fdo#108145)
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
fdo#104671 https://bugs.freedesktop.org/show_bug.cgi?id=104671
fdo#104782 https://bugs.freedesktop.org/show_bug.cgi?id=104782
fdo#105682 https://bugs.freedesktop.org/show_bug.cgi?id=105682
fdo#106885 https://bugs.freedesktop.org/show_bug.cgi?id=106885
fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
fdo#106978 https://bugs.freedesktop.org/show_bug.cgi?id=106978
fdo#107725 https://bugs.freedesktop.org/show_bug.cgi?id=107725
fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807
fdo#107815 https://bugs.freedesktop.org/show_bug.cgi?id=107815
fdo#107882 https://bugs.freedesktop.org/show_bug.cgi?id=107882
fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
fdo#108147 https://bugs.freedesktop.org/show_bug.cgi?id=108147
fdo#108470 https://bugs.freedesktop.org/show_bug.cgi?id=108470
fdo#108549 https://bugs.freedesktop.org/show_bug.cgi?id=108549
fdo#108561 https://bugs.freedesktop.org/show_bug.cgi?id=108561
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133
== Participating hosts (6 -> 6) ==
No changes in participating hosts
== Build changes ==
* Linux: CI_DRM_5041 -> Patchwork_10599
CI_DRM_5041: a45f611eae66546bb297a81c88e89bfd409bf199 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4695: 81b66cf2806d6a8e9516580fb31879677487d32b @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10599: b6ccca5a6aa430884a1a147e598222f134403cec @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10599/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [v5 2/2] drm/i915/icl: Enable Plane Input CSC for YUV to RGB Conversion
2018-10-26 10:01 ` [v5 2/2] drm/i915/icl: Enable Plane Input CSC for YUV to RGB Conversion Uma Shankar
@ 2018-10-29 23:29 ` Matt Roper
2018-10-31 12:34 ` Shankar, Uma
2018-10-30 10:55 ` Maarten Lankhorst
1 sibling, 1 reply; 13+ messages in thread
From: Matt Roper @ 2018-10-29 23:29 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx, ville.syrjala, maarten.lankhorst
On Fri, Oct 26, 2018 at 03:31:57PM +0530, Uma Shankar wrote:
> Plane input CSC needs to be enabled to convert frambuffers from
> YUV to RGB. This is needed for bottom 3 planes on ICL, rest of
> the planes have hardcoded conversion and taken care by the legacy
> code.
>
> This patch defines the co-efficient values for YUV to RGB conversion
> in BT709 and BT601 formats. It programs the coefficients and enables
> the plane input csc unit in hardware.
>
> Note: This is currently untested and floated to get an early feedback
> on the design and implementation for this feature. In parallel,
> I will test this on actual ICL hardware and confirm with planar
> formats.
>
> v2: Addressed Maarten's and Ville's review comments and added the
> coefficients in a 2D array instead of independent Macros.
>
> v3: Added individual coefficient matrix (9 values) instead of 6
> register values as per Maarten's comment. Also addresed a shift
> issue with B channel coefficient.
>
> v4: Added support for Limited Range Color Handling
>
> v5: Fixed Matt and Maarten's review comments.
>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
> drivers/gpu/drm/i915/intel_color.c | 79 ++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_display.c | 23 ++++++++---
> drivers/gpu/drm/i915/intel_drv.h | 2 +
> 3 files changed, 98 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 5127da2..681cd13 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -57,6 +57,15 @@
> #define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8
> #define CSC_RGB_TO_YUV_BV 0x1e080000
>
> +#define ROFF(x) (((x) & 0xffff) << 16)
> +#define GOFF(x) (((x) & 0xffff) << 0)
> +#define BOFF(x) (((x) & 0xffff) << 16)
> +
> +/* Preoffset values for YUV to RGB Conversion */
> +#define PREOFF_YUV_TO_RGB_HI 0x1800
> +#define PREOFF_YUV_TO_RGB_ME 0x1F00
> +#define PREOFF_YUV_TO_RGB_LO 0x1800
> +
> /*
> * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
> * format). This macro takes the coefficient we want transformed and the
> @@ -643,6 +652,76 @@ int intel_color_check(struct drm_crtc *crtc,
> return -EINVAL;
> }
>
> +void icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
> + const struct intel_plane_state *plane_state)
> +{
> + struct drm_i915_private *dev_priv =
> + to_i915(plane_state->base.plane->dev);
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> + enum pipe pipe = crtc->pipe;
> + struct intel_plane *intel_plane =
> + to_intel_plane(plane_state->base.plane);
> + enum plane_id plane = intel_plane->id;
> +
> + static const u16 input_csc_matrix[][9] = {
Can you add comments to these indicating the human-readable values they
translate to?
> + /* BT.601 full range YCbCr -> full range RGB */
> + [DRM_COLOR_YCBCR_BT601] = {
> + 0x7AF8, 0x7800, 0x0,
> + 0x8B28, 0x7800, 0x9AC0,
> + 0x0, 0x7800, 0x7DD8,
> + },
> + /* BT.709 full range YCbCr -> full range RGB */
> + [DRM_COLOR_YCBCR_BT709] = {
> + 0x7C98, 0x7800, 0x0,
> + 0x9EF8, 0x7800, 0xABF8,
> + 0x0, 0x7800, 0x7ED8,
> + },
> + };
> +
> + /* Matrix for Limited Range to Full Range Conversion */
> + static const u16 input_csc_matrix_lr[][9] = {
> + /* BT.601 Limted range YCbCr -> full range RGB */
> + [DRM_COLOR_YCBCR_BT601] = {
> + 0x7CC8, 0x7950, 0x0,
> + 0x8CB8, 0x7918, 0x9C40,
> + 0x0, 0x7918, 0x7FC8,
Are these obtained by scaling the first row (Y-based) by 256/219 and the
other two rows (Cb and Cr) by 256/224? If so, it looks like you've
always rounded down, whereas in some cases rounding up gives you a
closer value (and matches how the bspec seems to have chosen the full
range encodings for their example).
[ 0x7CD0, 0x7958, 0x0 ]
[ 0x8CC0, 0x7928, 0x9C48 ]
[ 0x0, 0x7928, 0x7FD8 ]
Our encodings of the 1.0 value on the second two rows seems to deviate
slightly more for some reason; not sure why that is.
For completeness, here's how I came up with 0x7928:
1 * 256/224 = 1.142857143
Sign bit = 0
Exponent bits = 0b111
Mantissa bits = round(1.142857143 << 8)
= round(292.571428571)
= 293
= 0b100100101
Reserved bits = 0b000
Result = 0111 1001 0010 1000
= 0x7928
If you did floor() instead of round() for the mantissa, you'd get 292,
which would translate to 0x7920 instead.
> + },
> + /* BT.709 Limited range YCbCr -> full range RGB */
> + [DRM_COLOR_YCBCR_BT709] = {
> + 0x7EA8, 0x7950, 0x0,
> + 0x8888, 0x7918, 0xADA8,
> + 0x0, 0x7918, 0x6870,
For these I get
[ 0x7EB8, 0x7958, 0 ]
[ 0x8890, 0x7928, 0xADB0 ]
[ 0x0, 0x7928, 0x6878 ]
So all the numbers are still pretty close to what you have.
Matt
> + },
> + };
> + const u16 *csc;
> +
> + if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> + csc = input_csc_matrix[plane_state->base.color_encoding];
> + else
> + csc = input_csc_matrix_lr[plane_state->base.color_encoding];
> +
> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 0), ROFF(csc[0]) |
> + GOFF(csc[1]));
> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 1), BOFF(csc[2]));
> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 2), ROFF(csc[3]) |
> + GOFF(csc[4]));
> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 3), BOFF(csc[5]));
> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 4), ROFF(csc[6]) |
> + GOFF(csc[7]));
> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 5), BOFF(csc[8]));
> +
> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 0),
> + PREOFF_YUV_TO_RGB_HI);
> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 1),
> + PREOFF_YUV_TO_RGB_ME);
> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 2),
> + PREOFF_YUV_TO_RGB_LO);
> +
> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 0), 0x0);
> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 1), 0x0);
> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 2), 0x0);
> +}
> +
> void intel_color_init(struct drm_crtc *crtc)
> {
> struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fe045ab..d16a064 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3666,6 +3666,7 @@ u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
> struct drm_i915_private *dev_priv =
> to_i915(plane_state->base.plane->dev);
> const struct drm_framebuffer *fb = plane_state->base.fb;
> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> u32 plane_color_ctl = 0;
>
> if (INTEL_GEN(dev_priv) < 11) {
> @@ -3676,13 +3677,23 @@ u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
> plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state);
>
> if (fb->format->is_yuv) {
> - if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
> - plane_color_ctl |= PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
> - else
> - plane_color_ctl |= PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
> + if (!icl_is_hdr_plane(plane)) {
> + if (plane_state->base.color_encoding ==
> + DRM_COLOR_YCBCR_BT709)
> + plane_color_ctl |=
> + PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
> + else
> + plane_color_ctl |=
> + PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
>
> - if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> - plane_color_ctl |= PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
> + if (plane_state->base.color_range ==
> + DRM_COLOR_YCBCR_FULL_RANGE)
> + plane_color_ctl |=
> + PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
> + } else {
> + icl_program_input_csc_coeff(crtc_state, plane_state);
> + plane_color_ctl |= PLANE_COLOR_INPUT_CSC_ENABLE;
> + }
> }
>
> return plane_color_ctl;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index db24308..bd9e946 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2285,6 +2285,8 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> int intel_color_check(struct drm_crtc *crtc, struct drm_crtc_state *state);
> void intel_color_set_csc(struct drm_crtc_state *crtc_state);
> void intel_color_load_luts(struct drm_crtc_state *crtc_state);
> +void icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
> + const struct intel_plane_state *plane_state);
>
> /* intel_lspcon.c */
> bool lspcon_init(struct intel_digital_port *intel_dig_port);
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [v5 2/2] drm/i915/icl: Enable Plane Input CSC for YUV to RGB Conversion
2018-10-26 10:01 ` [v5 2/2] drm/i915/icl: Enable Plane Input CSC for YUV to RGB Conversion Uma Shankar
2018-10-29 23:29 ` Matt Roper
@ 2018-10-30 10:55 ` Maarten Lankhorst
2018-10-31 12:56 ` Shankar, Uma
1 sibling, 1 reply; 13+ messages in thread
From: Maarten Lankhorst @ 2018-10-30 10:55 UTC (permalink / raw)
To: Uma Shankar, intel-gfx; +Cc: ville.syrjala, maarten.lankhorst
Op 26-10-18 om 12:01 schreef Uma Shankar:
> Plane input CSC needs to be enabled to convert frambuffers from
> YUV to RGB. This is needed for bottom 3 planes on ICL, rest of
> the planes have hardcoded conversion and taken care by the legacy
> code.
>
> This patch defines the co-efficient values for YUV to RGB conversion
> in BT709 and BT601 formats. It programs the coefficients and enables
> the plane input csc unit in hardware.
>
> Note: This is currently untested and floated to get an early feedback
> on the design and implementation for this feature. In parallel,
> I will test this on actual ICL hardware and confirm with planar
> formats.
>
> v2: Addressed Maarten's and Ville's review comments and added the
> coefficients in a 2D array instead of independent Macros.
>
> v3: Added individual coefficient matrix (9 values) instead of 6
> register values as per Maarten's comment. Also addresed a shift
> issue with B channel coefficient.
>
> v4: Added support for Limited Range Color Handling
>
> v5: Fixed Matt and Maarten's review comments.
>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
> drivers/gpu/drm/i915/intel_color.c | 79 ++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_display.c | 23 ++++++++---
> drivers/gpu/drm/i915/intel_drv.h | 2 +
> 3 files changed, 98 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 5127da2..681cd13 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -57,6 +57,15 @@
> #define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8
> #define CSC_RGB_TO_YUV_BV 0x1e080000
>
> +#define ROFF(x) (((x) & 0xffff) << 16)
> +#define GOFF(x) (((x) & 0xffff) << 0)
> +#define BOFF(x) (((x) & 0xffff) << 16)
> +
> +/* Preoffset values for YUV to RGB Conversion */
> +#define PREOFF_YUV_TO_RGB_HI 0x1800
> +#define PREOFF_YUV_TO_RGB_ME 0x1F00
> +#define PREOFF_YUV_TO_RGB_LO 0x1800
> +
> /*
> * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
> * format). This macro takes the coefficient we want transformed and the
> @@ -643,6 +652,76 @@ int intel_color_check(struct drm_crtc *crtc,
> return -EINVAL;
> }
>
> +void icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
> + const struct intel_plane_state *plane_state)
> +{
> + struct drm_i915_private *dev_priv =
> + to_i915(plane_state->base.plane->dev);
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> + enum pipe pipe = crtc->pipe;
> + struct intel_plane *intel_plane =
> + to_intel_plane(plane_state->base.plane);
> + enum plane_id plane = intel_plane->id;
> +
> + static const u16 input_csc_matrix[][9] = {
> + /* BT.601 full range YCbCr -> full range RGB */
> + [DRM_COLOR_YCBCR_BT601] = {
> + 0x7AF8, 0x7800, 0x0,
> + 0x8B28, 0x7800, 0x9AC0,
> + 0x0, 0x7800, 0x7DD8,
> + },
> + /* BT.709 full range YCbCr -> full range RGB */
> + [DRM_COLOR_YCBCR_BT709] = {
> + 0x7C98, 0x7800, 0x0,
> + 0x9EF8, 0x7800, 0xABF8,
> + 0x0, 0x7800, 0x7ED8,
> + },
> + };
> +
> + /* Matrix for Limited Range to Full Range Conversion */
> + static const u16 input_csc_matrix_lr[][9] = {
> + /* BT.601 Limted range YCbCr -> full range RGB */
> + [DRM_COLOR_YCBCR_BT601] = {
> + 0x7CC8, 0x7950, 0x0,
> + 0x8CB8, 0x7918, 0x9C40,
> + 0x0, 0x7918, 0x7FC8,
> + },
> + /* BT.709 Limited range YCbCr -> full range RGB */
> + [DRM_COLOR_YCBCR_BT709] = {
> + 0x7EA8, 0x7950, 0x0,
> + 0x8888, 0x7918, 0xADA8,
> + 0x0, 0x7918, 0x6870,
> + },
> + };
> + const u16 *csc;
> +
> + if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> + csc = input_csc_matrix[plane_state->base.color_encoding];
> + else
> + csc = input_csc_matrix_lr[plane_state->base.color_encoding];
> +
> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 0), ROFF(csc[0]) |
> + GOFF(csc[1]));
> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 1), BOFF(csc[2]));
> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 2), ROFF(csc[3]) |
> + GOFF(csc[4]));
> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 3), BOFF(csc[5]));
> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 4), ROFF(csc[6]) |
> + GOFF(csc[7]));
> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 5), BOFF(csc[8]));
> +
> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 0),
> + PREOFF_YUV_TO_RGB_HI);
> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 1),
> + PREOFF_YUV_TO_RGB_ME);
> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 2),
> + PREOFF_YUV_TO_RGB_LO);
> +
> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 0), 0x0);
> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 1), 0x0);
> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 2), 0x0);
> +}
> +
> void intel_color_init(struct drm_crtc *crtc)
> {
> struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fe045ab..d16a064 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3666,6 +3666,7 @@ u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
> struct drm_i915_private *dev_priv =
> to_i915(plane_state->base.plane->dev);
> const struct drm_framebuffer *fb = plane_state->base.fb;
> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> u32 plane_color_ctl = 0;
>
> if (INTEL_GEN(dev_priv) < 11) {
> @@ -3676,13 +3677,23 @@ u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
> plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state);
>
> if (fb->format->is_yuv) {
> - if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
> - plane_color_ctl |= PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
> - else
> - plane_color_ctl |= PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
> + if (!icl_is_hdr_plane(plane)) {
> + if (plane_state->base.color_encoding ==
> + DRM_COLOR_YCBCR_BT709)
> + plane_color_ctl |=
> + PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
> + else
> + plane_color_ctl |=
> + PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
>
> - if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> - plane_color_ctl |= PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
> + if (plane_state->base.color_range ==
> + DRM_COLOR_YCBCR_FULL_RANGE)
> + plane_color_ctl |=
> + PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
> + } else {
> + icl_program_input_csc_coeff(crtc_state, plane_state);
> + plane_color_ctl |= PLANE_COLOR_INPUT_CSC_ENABLE;
> + }
> }
>
> return plane_color_ctl;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index db24308..bd9e946 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2285,6 +2285,8 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> int intel_color_check(struct drm_crtc *crtc, struct drm_crtc_state *state);
> void intel_color_set_csc(struct drm_crtc_state *crtc_state);
> void intel_color_load_luts(struct drm_crtc_state *crtc_state);
> +void icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
> + const struct intel_plane_state *plane_state);
>
> /* intel_lspcon.c */
> bool lspcon_init(struct intel_digital_port *intel_dig_port);
Did some testing with this patch and got these values for HDR vs SDR planes..
Reference image, our NV12 implementation puts the chroma pixel center on
the left, so the gray values are ignored:
https://mblankhorst.nl/etc/nv12-reference.png
HDR plane with input CSC:
https://mblankhorst.nl/etc/nv12-hdr.png
SDR plane with fixed CSC:
https://mblankhorst.nl/etc/nv12-sdr.png
Looks like the SDR plane colors are slightly off between white and black, but
HDR is what I expect it to be.
So I think patches look good, and if you implement Matt's suggestions, then
this series is:
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [v5 2/2] drm/i915/icl: Enable Plane Input CSC for YUV to RGB Conversion
2018-10-29 23:29 ` Matt Roper
@ 2018-10-31 12:34 ` Shankar, Uma
2018-10-31 16:40 ` Matt Roper
0 siblings, 1 reply; 13+ messages in thread
From: Shankar, Uma @ 2018-10-31 12:34 UTC (permalink / raw)
To: Roper, Matthew D
Cc: intel-gfx@lists.freedesktop.org, Syrjala, Ville,
Lankhorst, Maarten
>-----Original Message-----
>From: Roper, Matthew D
>Sent: Tuesday, October 30, 2018 4:59 AM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>;
>Lankhorst, Maarten <maarten.lankhorst@intel.com>
>Subject: Re: [Intel-gfx] [v5 2/2] drm/i915/icl: Enable Plane Input CSC for YUV to
>RGB Conversion
>
>On Fri, Oct 26, 2018 at 03:31:57PM +0530, Uma Shankar wrote:
>> Plane input CSC needs to be enabled to convert frambuffers from YUV to
>> RGB. This is needed for bottom 3 planes on ICL, rest of the planes
>> have hardcoded conversion and taken care by the legacy code.
>>
>> This patch defines the co-efficient values for YUV to RGB conversion
>> in BT709 and BT601 formats. It programs the coefficients and enables
>> the plane input csc unit in hardware.
>>
>> Note: This is currently untested and floated to get an early feedback
>> on the design and implementation for this feature. In parallel, I will
>> test this on actual ICL hardware and confirm with planar formats.
>>
>> v2: Addressed Maarten's and Ville's review comments and added the
>> coefficients in a 2D array instead of independent Macros.
>>
>> v3: Added individual coefficient matrix (9 values) instead of 6
>> register values as per Maarten's comment. Also addresed a shift issue
>> with B channel coefficient.
>>
>> v4: Added support for Limited Range Color Handling
>>
>> v5: Fixed Matt and Maarten's review comments.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_color.c | 79
>++++++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/intel_display.c | 23 ++++++++---
>> drivers/gpu/drm/i915/intel_drv.h | 2 +
>> 3 files changed, 98 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color.c
>> b/drivers/gpu/drm/i915/intel_color.c
>> index 5127da2..681cd13 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -57,6 +57,15 @@
>> #define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8 #define
>CSC_RGB_TO_YUV_BV
>> 0x1e080000
>>
>> +#define ROFF(x) (((x) & 0xffff) << 16)
>> +#define GOFF(x) (((x) & 0xffff) << 0)
>> +#define BOFF(x) (((x) & 0xffff) << 16)
>> +
>> +/* Preoffset values for YUV to RGB Conversion */
>> +#define PREOFF_YUV_TO_RGB_HI 0x1800
>> +#define PREOFF_YUV_TO_RGB_ME 0x1F00
>> +#define PREOFF_YUV_TO_RGB_LO 0x1800
>> +
>> /*
>> * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
>> * format). This macro takes the coefficient we want transformed and
>> the @@ -643,6 +652,76 @@ int intel_color_check(struct drm_crtc *crtc,
>> return -EINVAL;
>> }
>>
>> +void icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
>> + const struct intel_plane_state *plane_state) {
>> + struct drm_i915_private *dev_priv =
>> + to_i915(plane_state->base.plane->dev);
>> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> + enum pipe pipe = crtc->pipe;
>> + struct intel_plane *intel_plane =
>> + to_intel_plane(plane_state->base.plane);
>> + enum plane_id plane = intel_plane->id;
>> +
>> + static const u16 input_csc_matrix[][9] = {
>
>Can you add comments to these indicating the human-readable values they
>translate to?
Sure Matt, will add that.
>> + /* BT.601 full range YCbCr -> full range RGB */
>> + [DRM_COLOR_YCBCR_BT601] = {
>> + 0x7AF8, 0x7800, 0x0,
>> + 0x8B28, 0x7800, 0x9AC0,
>> + 0x0, 0x7800, 0x7DD8,
>> + },
>> + /* BT.709 full range YCbCr -> full range RGB */
>> + [DRM_COLOR_YCBCR_BT709] = {
>> + 0x7C98, 0x7800, 0x0,
>> + 0x9EF8, 0x7800, 0xABF8,
>> + 0x0, 0x7800, 0x7ED8,
>> + },
>> + };
>> +
>> + /* Matrix for Limited Range to Full Range Conversion */
>> + static const u16 input_csc_matrix_lr[][9] = {
>> + /* BT.601 Limted range YCbCr -> full range RGB */
>> + [DRM_COLOR_YCBCR_BT601] = {
>> + 0x7CC8, 0x7950, 0x0,
>> + 0x8CB8, 0x7918, 0x9C40,
>> + 0x0, 0x7918, 0x7FC8,
>
>Are these obtained by scaling the first row (Y-based) by 256/219 and the other
>two rows (Cb and Cr) by 256/224? If so, it looks like you've always rounded
>down, whereas in some cases rounding up gives you a closer value (and matches
>how the bspec seems to have chosen the full range encodings for their example).
Yes, this is how it's done. But the only reason of delta is that you have taken 8 bit max
value as 256, but I am taking it as 255. I feel it should be (1 << bpc ie 8) - 1. If you put that,
the values which you got will match to what I have here. Please let me know if that's ok.
And thanks for detailed review and useful pointers.
Regards,
Uma Shankar
> [ 0x7CD0, 0x7958, 0x0 ]
> [ 0x8CC0, 0x7928, 0x9C48 ]
> [ 0x0, 0x7928, 0x7FD8 ]
>
>Our encodings of the 1.0 value on the second two rows seems to deviate slightly
>more for some reason; not sure why that is.
>
>For completeness, here's how I came up with 0x7928:
>
> 1 * 256/224 = 1.142857143
> Sign bit = 0
> Exponent bits = 0b111
> Mantissa bits = round(1.142857143 << 8)
> = round(292.571428571)
> = 293
> = 0b100100101
> Reserved bits = 0b000
>
> Result = 0111 1001 0010 1000
> = 0x7928
>
>If you did floor() instead of round() for the mantissa, you'd get 292, which would
>translate to 0x7920 instead.
>
>> + },
>> + /* BT.709 Limited range YCbCr -> full range RGB */
>> + [DRM_COLOR_YCBCR_BT709] = {
>> + 0x7EA8, 0x7950, 0x0,
>> + 0x8888, 0x7918, 0xADA8,
>> + 0x0, 0x7918, 0x6870,
>
>For these I get
>
> [ 0x7EB8, 0x7958, 0 ]
> [ 0x8890, 0x7928, 0xADB0 ]
> [ 0x0, 0x7928, 0x6878 ]
>
>So all the numbers are still pretty close to what you have.
>
>
>Matt
>
>> + },
>> + };
>> + const u16 *csc;
>> +
>> + if (plane_state->base.color_range ==
>DRM_COLOR_YCBCR_FULL_RANGE)
>> + csc = input_csc_matrix[plane_state->base.color_encoding];
>> + else
>> + csc = input_csc_matrix_lr[plane_state->base.color_encoding];
>> +
>> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 0), ROFF(csc[0]) |
>> + GOFF(csc[1]));
>> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 1), BOFF(csc[2]));
>> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 2), ROFF(csc[3]) |
>> + GOFF(csc[4]));
>> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 3), BOFF(csc[5]));
>> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 4), ROFF(csc[6]) |
>> + GOFF(csc[7]));
>> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 5), BOFF(csc[8]));
>> +
>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 0),
>> + PREOFF_YUV_TO_RGB_HI);
>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 1),
>> + PREOFF_YUV_TO_RGB_ME);
>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 2),
>> + PREOFF_YUV_TO_RGB_LO);
>> +
>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 0), 0x0);
>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 1), 0x0);
>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 2), 0x0); }
>> +
>> void intel_color_init(struct drm_crtc *crtc) {
>> struct drm_i915_private *dev_priv = to_i915(crtc->dev); diff --git
>> a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index fe045ab..d16a064 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3666,6 +3666,7 @@ u32 glk_plane_color_ctl(const struct intel_crtc_state
>*crtc_state,
>> struct drm_i915_private *dev_priv =
>> to_i915(plane_state->base.plane->dev);
>> const struct drm_framebuffer *fb = plane_state->base.fb;
>> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
>> u32 plane_color_ctl = 0;
>>
>> if (INTEL_GEN(dev_priv) < 11) {
>> @@ -3676,13 +3677,23 @@ u32 glk_plane_color_ctl(const struct
>intel_crtc_state *crtc_state,
>> plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state);
>>
>> if (fb->format->is_yuv) {
>> - if (plane_state->base.color_encoding ==
>DRM_COLOR_YCBCR_BT709)
>> - plane_color_ctl |=
>PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
>> - else
>> - plane_color_ctl |=
>PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
>> + if (!icl_is_hdr_plane(plane)) {
>> + if (plane_state->base.color_encoding ==
>> + DRM_COLOR_YCBCR_BT709)
>> + plane_color_ctl |=
>> +
> PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
>> + else
>> + plane_color_ctl |=
>> +
> PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
>>
>> - if (plane_state->base.color_range ==
>DRM_COLOR_YCBCR_FULL_RANGE)
>> - plane_color_ctl |=
>PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
>> + if (plane_state->base.color_range ==
>> + DRM_COLOR_YCBCR_FULL_RANGE)
>> + plane_color_ctl |=
>> +
> PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
>> + } else {
>> + icl_program_input_csc_coeff(crtc_state, plane_state);
>> + plane_color_ctl |= PLANE_COLOR_INPUT_CSC_ENABLE;
>> + }
>> }
>>
>> return plane_color_ctl;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index db24308..bd9e946 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -2285,6 +2285,8 @@ int intel_plane_atomic_check_with_state(const
>> struct intel_crtc_state *old_crtc_ int intel_color_check(struct
>> drm_crtc *crtc, struct drm_crtc_state *state); void
>> intel_color_set_csc(struct drm_crtc_state *crtc_state); void
>> intel_color_load_luts(struct drm_crtc_state *crtc_state);
>> +void icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
>> + const struct intel_plane_state *plane_state);
>>
>> /* intel_lspcon.c */
>> bool lspcon_init(struct intel_digital_port *intel_dig_port);
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>--
>Matt Roper
>Graphics Software Engineer
>IoTG Platform Enabling & Development
>Intel Corporation
>(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [v5 2/2] drm/i915/icl: Enable Plane Input CSC for YUV to RGB Conversion
2018-10-30 10:55 ` Maarten Lankhorst
@ 2018-10-31 12:56 ` Shankar, Uma
0 siblings, 0 replies; 13+ messages in thread
From: Shankar, Uma @ 2018-10-31 12:56 UTC (permalink / raw)
To: Maarten Lankhorst, intel-gfx@lists.freedesktop.org
Cc: Syrjala, Ville, Lankhorst, Maarten
>-----Original Message-----
>From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
>Sent: Tuesday, October 30, 2018 4:25 PM
>To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org
>Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
><maarten.lankhorst@intel.com>
>Subject: Re: [Intel-gfx] [v5 2/2] drm/i915/icl: Enable Plane Input CSC for YUV to
>RGB Conversion
>
>Op 26-10-18 om 12:01 schreef Uma Shankar:
>> Plane input CSC needs to be enabled to convert frambuffers from YUV to
>> RGB. This is needed for bottom 3 planes on ICL, rest of the planes
>> have hardcoded conversion and taken care by the legacy code.
>>
>> This patch defines the co-efficient values for YUV to RGB conversion
>> in BT709 and BT601 formats. It programs the coefficients and enables
>> the plane input csc unit in hardware.
>>
>> Note: This is currently untested and floated to get an early feedback
>> on the design and implementation for this feature. In parallel, I will
>> test this on actual ICL hardware and confirm with planar formats.
>>
>> v2: Addressed Maarten's and Ville's review comments and added the
>> coefficients in a 2D array instead of independent Macros.
>>
>> v3: Added individual coefficient matrix (9 values) instead of 6
>> register values as per Maarten's comment. Also addresed a shift issue
>> with B channel coefficient.
>>
>> v4: Added support for Limited Range Color Handling
>>
>> v5: Fixed Matt and Maarten's review comments.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_color.c | 79
>++++++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/intel_display.c | 23 ++++++++---
>> drivers/gpu/drm/i915/intel_drv.h | 2 +
>> 3 files changed, 98 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color.c
>> b/drivers/gpu/drm/i915/intel_color.c
>> index 5127da2..681cd13 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -57,6 +57,15 @@
>> #define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8 #define
>CSC_RGB_TO_YUV_BV
>> 0x1e080000
>>
>> +#define ROFF(x) (((x) & 0xffff) << 16)
>> +#define GOFF(x) (((x) & 0xffff) << 0)
>> +#define BOFF(x) (((x) & 0xffff) << 16)
>> +
>> +/* Preoffset values for YUV to RGB Conversion */
>> +#define PREOFF_YUV_TO_RGB_HI 0x1800
>> +#define PREOFF_YUV_TO_RGB_ME 0x1F00
>> +#define PREOFF_YUV_TO_RGB_LO 0x1800
>> +
>> /*
>> * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
>> * format). This macro takes the coefficient we want transformed and
>> the @@ -643,6 +652,76 @@ int intel_color_check(struct drm_crtc *crtc,
>> return -EINVAL;
>> }
>>
>> +void icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
>> + const struct intel_plane_state *plane_state) {
>> + struct drm_i915_private *dev_priv =
>> + to_i915(plane_state->base.plane->dev);
>> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> + enum pipe pipe = crtc->pipe;
>> + struct intel_plane *intel_plane =
>> + to_intel_plane(plane_state->base.plane);
>> + enum plane_id plane = intel_plane->id;
>> +
>> + static const u16 input_csc_matrix[][9] = {
>> + /* BT.601 full range YCbCr -> full range RGB */
>> + [DRM_COLOR_YCBCR_BT601] = {
>> + 0x7AF8, 0x7800, 0x0,
>> + 0x8B28, 0x7800, 0x9AC0,
>> + 0x0, 0x7800, 0x7DD8,
>> + },
>> + /* BT.709 full range YCbCr -> full range RGB */
>> + [DRM_COLOR_YCBCR_BT709] = {
>> + 0x7C98, 0x7800, 0x0,
>> + 0x9EF8, 0x7800, 0xABF8,
>> + 0x0, 0x7800, 0x7ED8,
>> + },
>> + };
>> +
>> + /* Matrix for Limited Range to Full Range Conversion */
>> + static const u16 input_csc_matrix_lr[][9] = {
>> + /* BT.601 Limted range YCbCr -> full range RGB */
>> + [DRM_COLOR_YCBCR_BT601] = {
>> + 0x7CC8, 0x7950, 0x0,
>> + 0x8CB8, 0x7918, 0x9C40,
>> + 0x0, 0x7918, 0x7FC8,
>> + },
>> + /* BT.709 Limited range YCbCr -> full range RGB */
>> + [DRM_COLOR_YCBCR_BT709] = {
>> + 0x7EA8, 0x7950, 0x0,
>> + 0x8888, 0x7918, 0xADA8,
>> + 0x0, 0x7918, 0x6870,
>> + },
>> + };
>> + const u16 *csc;
>> +
>> + if (plane_state->base.color_range ==
>DRM_COLOR_YCBCR_FULL_RANGE)
>> + csc = input_csc_matrix[plane_state->base.color_encoding];
>> + else
>> + csc = input_csc_matrix_lr[plane_state->base.color_encoding];
>> +
>> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 0), ROFF(csc[0]) |
>> + GOFF(csc[1]));
>> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 1), BOFF(csc[2]));
>> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 2), ROFF(csc[3]) |
>> + GOFF(csc[4]));
>> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 3), BOFF(csc[5]));
>> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 4), ROFF(csc[6]) |
>> + GOFF(csc[7]));
>> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 5), BOFF(csc[8]));
>> +
>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 0),
>> + PREOFF_YUV_TO_RGB_HI);
>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 1),
>> + PREOFF_YUV_TO_RGB_ME);
>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 2),
>> + PREOFF_YUV_TO_RGB_LO);
>> +
>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 0), 0x0);
>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 1), 0x0);
>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 2), 0x0); }
>> +
>> void intel_color_init(struct drm_crtc *crtc) {
>> struct drm_i915_private *dev_priv = to_i915(crtc->dev); diff --git
>> a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index fe045ab..d16a064 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3666,6 +3666,7 @@ u32 glk_plane_color_ctl(const struct intel_crtc_state
>*crtc_state,
>> struct drm_i915_private *dev_priv =
>> to_i915(plane_state->base.plane->dev);
>> const struct drm_framebuffer *fb = plane_state->base.fb;
>> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
>> u32 plane_color_ctl = 0;
>>
>> if (INTEL_GEN(dev_priv) < 11) {
>> @@ -3676,13 +3677,23 @@ u32 glk_plane_color_ctl(const struct
>intel_crtc_state *crtc_state,
>> plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state);
>>
>> if (fb->format->is_yuv) {
>> - if (plane_state->base.color_encoding ==
>DRM_COLOR_YCBCR_BT709)
>> - plane_color_ctl |=
>PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
>> - else
>> - plane_color_ctl |=
>PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
>> + if (!icl_is_hdr_plane(plane)) {
>> + if (plane_state->base.color_encoding ==
>> + DRM_COLOR_YCBCR_BT709)
>> + plane_color_ctl |=
>> +
> PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
>> + else
>> + plane_color_ctl |=
>> +
> PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
>>
>> - if (plane_state->base.color_range ==
>DRM_COLOR_YCBCR_FULL_RANGE)
>> - plane_color_ctl |=
>PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
>> + if (plane_state->base.color_range ==
>> + DRM_COLOR_YCBCR_FULL_RANGE)
>> + plane_color_ctl |=
>> +
> PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
>> + } else {
>> + icl_program_input_csc_coeff(crtc_state, plane_state);
>> + plane_color_ctl |= PLANE_COLOR_INPUT_CSC_ENABLE;
>> + }
>> }
>>
>> return plane_color_ctl;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index db24308..bd9e946 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -2285,6 +2285,8 @@ int intel_plane_atomic_check_with_state(const
>> struct intel_crtc_state *old_crtc_ int intel_color_check(struct
>> drm_crtc *crtc, struct drm_crtc_state *state); void
>> intel_color_set_csc(struct drm_crtc_state *crtc_state); void
>> intel_color_load_luts(struct drm_crtc_state *crtc_state);
>> +void icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
>> + const struct intel_plane_state *plane_state);
>>
>> /* intel_lspcon.c */
>> bool lspcon_init(struct intel_digital_port *intel_dig_port);
>
>Did some testing with this patch and got these values for HDR vs SDR planes..
>
>Reference image, our NV12 implementation puts the chroma pixel center on the
>left, so the gray values are ignored:
>https://mblankhorst.nl/etc/nv12-reference.png
>
>HDR plane with input CSC:
>https://mblankhorst.nl/etc/nv12-hdr.png
>
>SDR plane with fixed CSC:
>https://mblankhorst.nl/etc/nv12-sdr.png
>
>Looks like the SDR plane colors are slightly off between white and black, but HDR
>is what I expect it to be.
>
>So I think patches look good, and if you implement Matt's suggestions, then this
>series is:
>Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Surely will address Matt's comment and update the series. Thanks a lot for trying this out
and all your valuable feedback and review comments on this.
Regards,
Uma Shankar
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [v5 2/2] drm/i915/icl: Enable Plane Input CSC for YUV to RGB Conversion
2018-10-31 12:34 ` Shankar, Uma
@ 2018-10-31 16:40 ` Matt Roper
2018-11-01 6:33 ` Shankar, Uma
0 siblings, 1 reply; 13+ messages in thread
From: Matt Roper @ 2018-10-31 16:40 UTC (permalink / raw)
To: Shankar, Uma
Cc: intel-gfx@lists.freedesktop.org, Syrjala, Ville,
Lankhorst, Maarten
On Wed, Oct 31, 2018 at 05:34:19AM -0700, Shankar, Uma wrote:
>
>
> >-----Original Message-----
> >From: Roper, Matthew D
> >Sent: Tuesday, October 30, 2018 4:59 AM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>;
> >Lankhorst, Maarten <maarten.lankhorst@intel.com>
> >Subject: Re: [Intel-gfx] [v5 2/2] drm/i915/icl: Enable Plane Input CSC for YUV to
> >RGB Conversion
> >
> >On Fri, Oct 26, 2018 at 03:31:57PM +0530, Uma Shankar wrote:
> >> Plane input CSC needs to be enabled to convert frambuffers from YUV to
> >> RGB. This is needed for bottom 3 planes on ICL, rest of the planes
> >> have hardcoded conversion and taken care by the legacy code.
> >>
> >> This patch defines the co-efficient values for YUV to RGB conversion
> >> in BT709 and BT601 formats. It programs the coefficients and enables
> >> the plane input csc unit in hardware.
> >>
> >> Note: This is currently untested and floated to get an early feedback
> >> on the design and implementation for this feature. In parallel, I will
> >> test this on actual ICL hardware and confirm with planar formats.
> >>
> >> v2: Addressed Maarten's and Ville's review comments and added the
> >> coefficients in a 2D array instead of independent Macros.
> >>
> >> v3: Added individual coefficient matrix (9 values) instead of 6
> >> register values as per Maarten's comment. Also addresed a shift issue
> >> with B channel coefficient.
> >>
> >> v4: Added support for Limited Range Color Handling
> >>
> >> v5: Fixed Matt and Maarten's review comments.
> >>
> >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >> ---
> >> drivers/gpu/drm/i915/intel_color.c | 79
> >++++++++++++++++++++++++++++++++++++
> >> drivers/gpu/drm/i915/intel_display.c | 23 ++++++++---
> >> drivers/gpu/drm/i915/intel_drv.h | 2 +
> >> 3 files changed, 98 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_color.c
> >> b/drivers/gpu/drm/i915/intel_color.c
> >> index 5127da2..681cd13 100644
> >> --- a/drivers/gpu/drm/i915/intel_color.c
> >> +++ b/drivers/gpu/drm/i915/intel_color.c
> >> @@ -57,6 +57,15 @@
> >> #define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8 #define
> >CSC_RGB_TO_YUV_BV
> >> 0x1e080000
> >>
> >> +#define ROFF(x) (((x) & 0xffff) << 16)
> >> +#define GOFF(x) (((x) & 0xffff) << 0)
> >> +#define BOFF(x) (((x) & 0xffff) << 16)
> >> +
> >> +/* Preoffset values for YUV to RGB Conversion */
> >> +#define PREOFF_YUV_TO_RGB_HI 0x1800
> >> +#define PREOFF_YUV_TO_RGB_ME 0x1F00
> >> +#define PREOFF_YUV_TO_RGB_LO 0x1800
> >> +
> >> /*
> >> * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
> >> * format). This macro takes the coefficient we want transformed and
> >> the @@ -643,6 +652,76 @@ int intel_color_check(struct drm_crtc *crtc,
> >> return -EINVAL;
> >> }
> >>
> >> +void icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
> >> + const struct intel_plane_state *plane_state) {
> >> + struct drm_i915_private *dev_priv =
> >> + to_i915(plane_state->base.plane->dev);
> >> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >> + enum pipe pipe = crtc->pipe;
> >> + struct intel_plane *intel_plane =
> >> + to_intel_plane(plane_state->base.plane);
> >> + enum plane_id plane = intel_plane->id;
> >> +
> >> + static const u16 input_csc_matrix[][9] = {
> >
> >Can you add comments to these indicating the human-readable values they
> >translate to?
>
> Sure Matt, will add that.
>
> >> + /* BT.601 full range YCbCr -> full range RGB */
> >> + [DRM_COLOR_YCBCR_BT601] = {
> >> + 0x7AF8, 0x7800, 0x0,
> >> + 0x8B28, 0x7800, 0x9AC0,
> >> + 0x0, 0x7800, 0x7DD8,
> >> + },
> >> + /* BT.709 full range YCbCr -> full range RGB */
> >> + [DRM_COLOR_YCBCR_BT709] = {
> >> + 0x7C98, 0x7800, 0x0,
> >> + 0x9EF8, 0x7800, 0xABF8,
> >> + 0x0, 0x7800, 0x7ED8,
> >> + },
> >> + };
> >> +
> >> + /* Matrix for Limited Range to Full Range Conversion */
> >> + static const u16 input_csc_matrix_lr[][9] = {
> >> + /* BT.601 Limted range YCbCr -> full range RGB */
> >> + [DRM_COLOR_YCBCR_BT601] = {
> >> + 0x7CC8, 0x7950, 0x0,
> >> + 0x8CB8, 0x7918, 0x9C40,
> >> + 0x0, 0x7918, 0x7FC8,
> >
> >Are these obtained by scaling the first row (Y-based) by 256/219 and the other
> >two rows (Cb and Cr) by 256/224? If so, it looks like you've always rounded
> >down, whereas in some cases rounding up gives you a closer value (and matches
> >how the bspec seems to have chosen the full range encodings for their example).
>
> Yes, this is how it's done. But the only reason of delta is that you have taken 8 bit max
> value as 256, but I am taking it as 255. I feel it should be (1 << bpc ie 8) - 1. If you put that,
> the values which you got will match to what I have here. Please let me know if that's ok.
Ah, okay, that makes sense. I do get the same values as you for the
BT.601 table that way, but I still get a slight rounding difference in
one cell of the BT.709 matrix down below.
>
> And thanks for detailed review and useful pointers.
>
> Regards,
> Uma Shankar
>
> > [ 0x7CD0, 0x7958, 0x0 ]
> > [ 0x8CC0, 0x7928, 0x9C48 ]
> > [ 0x0, 0x7928, 0x7FD8 ]
> >
> >Our encodings of the 1.0 value on the second two rows seems to deviate slightly
> >more for some reason; not sure why that is.
> >
> >For completeness, here's how I came up with 0x7928:
> >
> > 1 * 256/224 = 1.142857143
> > Sign bit = 0
> > Exponent bits = 0b111
> > Mantissa bits = round(1.142857143 << 8)
> > = round(292.571428571)
> > = 293
> > = 0b100100101
> > Reserved bits = 0b000
> >
> > Result = 0111 1001 0010 1000
> > = 0x7928
> >
> >If you did floor() instead of round() for the mantissa, you'd get 292, which would
> >translate to 0x7920 instead.
> >
> >> + },
> >> + /* BT.709 Limited range YCbCr -> full range RGB */
> >> + [DRM_COLOR_YCBCR_BT709] = {
> >> + 0x7EA8, 0x7950, 0x0,
> >> + 0x8888, 0x7918, 0xADA8,
> >> + 0x0, 0x7918, 0x6870,
For BU I get 0xADA0 instead of 0xADA8.
-.187 * 255/224 = -0.212879464
Sign bit = 1
Exponent bits = 0b010
Mantissa bits = round(0.212879464 << (9+2))
= round(435.977142272)
= 436
= 0b110110100
Reserved bits = 0b000
Result = 1010 1101 1010 0000
= 0xADA0
All of the other values match now though.
Matt
> >
> >For these I get
> >
> > [ 0x7EB8, 0x7958, 0 ]
> > [ 0x8890, 0x7928, 0xADB0 ]
> > [ 0x0, 0x7928, 0x6878 ]
> >
> >So all the numbers are still pretty close to what you have.
> >
> >
> >Matt
> >
> >> + },
> >> + };
> >> + const u16 *csc;
> >> +
> >> + if (plane_state->base.color_range ==
> >DRM_COLOR_YCBCR_FULL_RANGE)
> >> + csc = input_csc_matrix[plane_state->base.color_encoding];
> >> + else
> >> + csc = input_csc_matrix_lr[plane_state->base.color_encoding];
> >> +
> >> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 0), ROFF(csc[0]) |
> >> + GOFF(csc[1]));
> >> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 1), BOFF(csc[2]));
> >> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 2), ROFF(csc[3]) |
> >> + GOFF(csc[4]));
> >> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 3), BOFF(csc[5]));
> >> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 4), ROFF(csc[6]) |
> >> + GOFF(csc[7]));
> >> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 5), BOFF(csc[8]));
> >> +
> >> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 0),
> >> + PREOFF_YUV_TO_RGB_HI);
> >> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 1),
> >> + PREOFF_YUV_TO_RGB_ME);
> >> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 2),
> >> + PREOFF_YUV_TO_RGB_LO);
> >> +
> >> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 0), 0x0);
> >> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 1), 0x0);
> >> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 2), 0x0); }
> >> +
> >> void intel_color_init(struct drm_crtc *crtc) {
> >> struct drm_i915_private *dev_priv = to_i915(crtc->dev); diff --git
> >> a/drivers/gpu/drm/i915/intel_display.c
> >> b/drivers/gpu/drm/i915/intel_display.c
> >> index fe045ab..d16a064 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -3666,6 +3666,7 @@ u32 glk_plane_color_ctl(const struct intel_crtc_state
> >*crtc_state,
> >> struct drm_i915_private *dev_priv =
> >> to_i915(plane_state->base.plane->dev);
> >> const struct drm_framebuffer *fb = plane_state->base.fb;
> >> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> >> u32 plane_color_ctl = 0;
> >>
> >> if (INTEL_GEN(dev_priv) < 11) {
> >> @@ -3676,13 +3677,23 @@ u32 glk_plane_color_ctl(const struct
> >intel_crtc_state *crtc_state,
> >> plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state);
> >>
> >> if (fb->format->is_yuv) {
> >> - if (plane_state->base.color_encoding ==
> >DRM_COLOR_YCBCR_BT709)
> >> - plane_color_ctl |=
> >PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
> >> - else
> >> - plane_color_ctl |=
> >PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
> >> + if (!icl_is_hdr_plane(plane)) {
> >> + if (plane_state->base.color_encoding ==
> >> + DRM_COLOR_YCBCR_BT709)
> >> + plane_color_ctl |=
> >> +
> > PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
> >> + else
> >> + plane_color_ctl |=
> >> +
> > PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
> >>
> >> - if (plane_state->base.color_range ==
> >DRM_COLOR_YCBCR_FULL_RANGE)
> >> - plane_color_ctl |=
> >PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
> >> + if (plane_state->base.color_range ==
> >> + DRM_COLOR_YCBCR_FULL_RANGE)
> >> + plane_color_ctl |=
> >> +
> > PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
> >> + } else {
> >> + icl_program_input_csc_coeff(crtc_state, plane_state);
> >> + plane_color_ctl |= PLANE_COLOR_INPUT_CSC_ENABLE;
> >> + }
> >> }
> >>
> >> return plane_color_ctl;
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> >> b/drivers/gpu/drm/i915/intel_drv.h
> >> index db24308..bd9e946 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -2285,6 +2285,8 @@ int intel_plane_atomic_check_with_state(const
> >> struct intel_crtc_state *old_crtc_ int intel_color_check(struct
> >> drm_crtc *crtc, struct drm_crtc_state *state); void
> >> intel_color_set_csc(struct drm_crtc_state *crtc_state); void
> >> intel_color_load_luts(struct drm_crtc_state *crtc_state);
> >> +void icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
> >> + const struct intel_plane_state *plane_state);
> >>
> >> /* intel_lspcon.c */
> >> bool lspcon_init(struct intel_digital_port *intel_dig_port);
> >> --
> >> 1.9.1
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >--
> >Matt Roper
> >Graphics Software Engineer
> >IoTG Platform Enabling & Development
> >Intel Corporation
> >(916) 356-2795
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [v5 2/2] drm/i915/icl: Enable Plane Input CSC for YUV to RGB Conversion
2018-10-31 16:40 ` Matt Roper
@ 2018-11-01 6:33 ` Shankar, Uma
0 siblings, 0 replies; 13+ messages in thread
From: Shankar, Uma @ 2018-11-01 6:33 UTC (permalink / raw)
To: Roper, Matthew D
Cc: intel-gfx@lists.freedesktop.org, Syrjala, Ville,
Lankhorst, Maarten
>-----Original Message-----
>From: Roper, Matthew D
>Sent: Wednesday, October 31, 2018 10:11 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>;
>Lankhorst, Maarten <maarten.lankhorst@intel.com>
>Subject: Re: [Intel-gfx] [v5 2/2] drm/i915/icl: Enable Plane Input CSC for YUV to
>RGB Conversion
>
>On Wed, Oct 31, 2018 at 05:34:19AM -0700, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Roper, Matthew D
>> >Sent: Tuesday, October 30, 2018 4:59 AM
>> >To: Shankar, Uma <uma.shankar@intel.com>
>> >Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville
>> ><ville.syrjala@intel.com>; Lankhorst, Maarten
>> ><maarten.lankhorst@intel.com>
>> >Subject: Re: [Intel-gfx] [v5 2/2] drm/i915/icl: Enable Plane Input
>> >CSC for YUV to RGB Conversion
>> >
>> >On Fri, Oct 26, 2018 at 03:31:57PM +0530, Uma Shankar wrote:
>> >> Plane input CSC needs to be enabled to convert frambuffers from YUV
>> >> to RGB. This is needed for bottom 3 planes on ICL, rest of the
>> >> planes have hardcoded conversion and taken care by the legacy code.
>> >>
>> >> This patch defines the co-efficient values for YUV to RGB
>> >> conversion in BT709 and BT601 formats. It programs the coefficients
>> >> and enables the plane input csc unit in hardware.
>> >>
>> >> Note: This is currently untested and floated to get an early
>> >> feedback on the design and implementation for this feature. In
>> >> parallel, I will test this on actual ICL hardware and confirm with planar
>formats.
>> >>
>> >> v2: Addressed Maarten's and Ville's review comments and added the
>> >> coefficients in a 2D array instead of independent Macros.
>> >>
>> >> v3: Added individual coefficient matrix (9 values) instead of 6
>> >> register values as per Maarten's comment. Also addresed a shift
>> >> issue with B channel coefficient.
>> >>
>> >> v4: Added support for Limited Range Color Handling
>> >>
>> >> v5: Fixed Matt and Maarten's review comments.
>> >>
>> >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> >> ---
>> >> drivers/gpu/drm/i915/intel_color.c | 79
>> >++++++++++++++++++++++++++++++++++++
>> >> drivers/gpu/drm/i915/intel_display.c | 23 ++++++++---
>> >> drivers/gpu/drm/i915/intel_drv.h | 2 +
>> >> 3 files changed, 98 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/intel_color.c
>> >> b/drivers/gpu/drm/i915/intel_color.c
>> >> index 5127da2..681cd13 100644
>> >> --- a/drivers/gpu/drm/i915/intel_color.c
>> >> +++ b/drivers/gpu/drm/i915/intel_color.c
>> >> @@ -57,6 +57,15 @@
>> >> #define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8 #define
>> >CSC_RGB_TO_YUV_BV
>> >> 0x1e080000
>> >>
>> >> +#define ROFF(x) (((x) & 0xffff) << 16)
>> >> +#define GOFF(x) (((x) & 0xffff) << 0)
>> >> +#define BOFF(x) (((x) & 0xffff) << 16)
>> >> +
>> >> +/* Preoffset values for YUV to RGB Conversion */
>> >> +#define PREOFF_YUV_TO_RGB_HI 0x1800
>> >> +#define PREOFF_YUV_TO_RGB_ME 0x1F00
>> >> +#define PREOFF_YUV_TO_RGB_LO 0x1800
>> >> +
>> >> /*
>> >> * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
>> >> * format). This macro takes the coefficient we want transformed
>> >> and the @@ -643,6 +652,76 @@ int intel_color_check(struct drm_crtc *crtc,
>> >> return -EINVAL;
>> >> }
>> >>
>> >> +void icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
>> >> + const struct intel_plane_state *plane_state) {
>> >> + struct drm_i915_private *dev_priv =
>> >> + to_i915(plane_state->base.plane->dev);
>> >> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> >> + enum pipe pipe = crtc->pipe;
>> >> + struct intel_plane *intel_plane =
>> >> + to_intel_plane(plane_state->base.plane);
>> >> + enum plane_id plane = intel_plane->id;
>> >> +
>> >> + static const u16 input_csc_matrix[][9] = {
>> >
>> >Can you add comments to these indicating the human-readable values
>> >they translate to?
>>
>> Sure Matt, will add that.
>>
>> >> + /* BT.601 full range YCbCr -> full range RGB */
>> >> + [DRM_COLOR_YCBCR_BT601] = {
>> >> + 0x7AF8, 0x7800, 0x0,
>> >> + 0x8B28, 0x7800, 0x9AC0,
>> >> + 0x0, 0x7800, 0x7DD8,
>> >> + },
>> >> + /* BT.709 full range YCbCr -> full range RGB */
>> >> + [DRM_COLOR_YCBCR_BT709] = {
>> >> + 0x7C98, 0x7800, 0x0,
>> >> + 0x9EF8, 0x7800, 0xABF8,
>> >> + 0x0, 0x7800, 0x7ED8,
>> >> + },
>> >> + };
>> >> +
>> >> + /* Matrix for Limited Range to Full Range Conversion */
>> >> + static const u16 input_csc_matrix_lr[][9] = {
>> >> + /* BT.601 Limted range YCbCr -> full range RGB */
>> >> + [DRM_COLOR_YCBCR_BT601] = {
>> >> + 0x7CC8, 0x7950, 0x0,
>> >> + 0x8CB8, 0x7918, 0x9C40,
>> >> + 0x0, 0x7918, 0x7FC8,
>> >
>> >Are these obtained by scaling the first row (Y-based) by 256/219 and
>> >the other two rows (Cb and Cr) by 256/224? If so, it looks like
>> >you've always rounded down, whereas in some cases rounding up gives
>> >you a closer value (and matches how the bspec seems to have chosen the full
>range encodings for their example).
>>
>> Yes, this is how it's done. But the only reason of delta is that you
>> have taken 8 bit max value as 256, but I am taking it as 255. I feel
>> it should be (1 << bpc ie 8) - 1. If you put that, the values which you got will
>match to what I have here. Please let me know if that's ok.
>
>Ah, okay, that makes sense. I do get the same values as you for the
>BT.601 table that way, but I still get a slight rounding difference in one cell of the
>BT.709 matrix down below.
>
>>
>> And thanks for detailed review and useful pointers.
>>
>> Regards,
>> Uma Shankar
>>
>> > [ 0x7CD0, 0x7958, 0x0 ]
>> > [ 0x8CC0, 0x7928, 0x9C48 ]
>> > [ 0x0, 0x7928, 0x7FD8 ]
>> >
>> >Our encodings of the 1.0 value on the second two rows seems to
>> >deviate slightly more for some reason; not sure why that is.
>> >
>> >For completeness, here's how I came up with 0x7928:
>> >
>> > 1 * 256/224 = 1.142857143
>> > Sign bit = 0
>> > Exponent bits = 0b111
>> > Mantissa bits = round(1.142857143 << 8)
>> > = round(292.571428571)
>> > = 293
>> > = 0b100100101
>> > Reserved bits = 0b000
>> >
>> > Result = 0111 1001 0010 1000
>> > = 0x7928
>> >
>> >If you did floor() instead of round() for the mantissa, you'd get
>> >292, which would translate to 0x7920 instead.
>> >
>> >> + },
>> >> + /* BT.709 Limited range YCbCr -> full range RGB */
>> >> + [DRM_COLOR_YCBCR_BT709] = {
>> >> + 0x7EA8, 0x7950, 0x0,
>> >> + 0x8888, 0x7918, 0xADA8,
>> >> + 0x0, 0x7918, 0x6870,
>
>For BU I get 0xADA0 instead of 0xADA8.
>
> -.187 * 255/224 = -0.212879464
> Sign bit = 1
> Exponent bits = 0b010
> Mantissa bits = round(0.212879464 << (9+2))
> = round(435.977142272)
> = 436
> = 0b110110100
> Reserved bits = 0b000
> Result = 1010 1101 1010 0000
> = 0xADA0
>
>All of the other values match now though.
Actually this is due to precision I took as reference value. The
Actual value is -0.18732427293, if we take this much precision,
the value will match to what I got. So I feel this should be ok.
>
>Matt
>
>
>> >
>> >For these I get
>> >
>> > [ 0x7EB8, 0x7958, 0 ]
>> > [ 0x8890, 0x7928, 0xADB0 ]
>> > [ 0x0, 0x7928, 0x6878 ]
>> >
>> >So all the numbers are still pretty close to what you have.
>> >
>> >
>> >Matt
>> >
>> >> + },
>> >> + };
>> >> + const u16 *csc;
>> >> +
>> >> + if (plane_state->base.color_range ==
>> >DRM_COLOR_YCBCR_FULL_RANGE)
>> >> + csc = input_csc_matrix[plane_state->base.color_encoding];
>> >> + else
>> >> + csc = input_csc_matrix_lr[plane_state->base.color_encoding];
>> >> +
>> >> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 0), ROFF(csc[0]) |
>> >> + GOFF(csc[1]));
>> >> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 1), BOFF(csc[2]));
>> >> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 2), ROFF(csc[3]) |
>> >> + GOFF(csc[4]));
>> >> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 3), BOFF(csc[5]));
>> >> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 4), ROFF(csc[6]) |
>> >> + GOFF(csc[7]));
>> >> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 5), BOFF(csc[8]));
>> >> +
>> >> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 0),
>> >> + PREOFF_YUV_TO_RGB_HI);
>> >> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 1),
>> >> + PREOFF_YUV_TO_RGB_ME);
>> >> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 2),
>> >> + PREOFF_YUV_TO_RGB_LO);
>> >> +
>> >> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 0), 0x0);
>> >> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 1), 0x0);
>> >> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 2), 0x0); }
>> >> +
>> >> void intel_color_init(struct drm_crtc *crtc) {
>> >> struct drm_i915_private *dev_priv = to_i915(crtc->dev); diff
>> >> --git a/drivers/gpu/drm/i915/intel_display.c
>> >> b/drivers/gpu/drm/i915/intel_display.c
>> >> index fe045ab..d16a064 100644
>> >> --- a/drivers/gpu/drm/i915/intel_display.c
>> >> +++ b/drivers/gpu/drm/i915/intel_display.c
>> >> @@ -3666,6 +3666,7 @@ u32 glk_plane_color_ctl(const struct
>> >> intel_crtc_state
>> >*crtc_state,
>> >> struct drm_i915_private *dev_priv =
>> >> to_i915(plane_state->base.plane->dev);
>> >> const struct drm_framebuffer *fb = plane_state->base.fb;
>> >> + struct intel_plane *plane =
>> >> +to_intel_plane(plane_state->base.plane);
>> >> u32 plane_color_ctl = 0;
>> >>
>> >> if (INTEL_GEN(dev_priv) < 11) {
>> >> @@ -3676,13 +3677,23 @@ u32 glk_plane_color_ctl(const struct
>> >intel_crtc_state *crtc_state,
>> >> plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state);
>> >>
>> >> if (fb->format->is_yuv) {
>> >> - if (plane_state->base.color_encoding ==
>> >DRM_COLOR_YCBCR_BT709)
>> >> - plane_color_ctl |=
>> >PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
>> >> - else
>> >> - plane_color_ctl |=
>> >PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
>> >> + if (!icl_is_hdr_plane(plane)) {
>> >> + if (plane_state->base.color_encoding ==
>> >> + DRM_COLOR_YCBCR_BT709)
>> >> + plane_color_ctl |=
>> >> +
>> > PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
>> >> + else
>> >> + plane_color_ctl |=
>> >> +
>> > PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
>> >>
>> >> - if (plane_state->base.color_range ==
>> >DRM_COLOR_YCBCR_FULL_RANGE)
>> >> - plane_color_ctl |=
>> >PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
>> >> + if (plane_state->base.color_range ==
>> >> + DRM_COLOR_YCBCR_FULL_RANGE)
>> >> + plane_color_ctl |=
>> >> +
>> > PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
>> >> + } else {
>> >> + icl_program_input_csc_coeff(crtc_state, plane_state);
>> >> + plane_color_ctl |= PLANE_COLOR_INPUT_CSC_ENABLE;
>> >> + }
>> >> }
>> >>
>> >> return plane_color_ctl;
>> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> >> b/drivers/gpu/drm/i915/intel_drv.h
>> >> index db24308..bd9e946 100644
>> >> --- a/drivers/gpu/drm/i915/intel_drv.h
>> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> >> @@ -2285,6 +2285,8 @@ int intel_plane_atomic_check_with_state(const
>> >> struct intel_crtc_state *old_crtc_ int intel_color_check(struct
>> >> drm_crtc *crtc, struct drm_crtc_state *state); void
>> >> intel_color_set_csc(struct drm_crtc_state *crtc_state); void
>> >> intel_color_load_luts(struct drm_crtc_state *crtc_state);
>> >> +void icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
>> >> + const struct intel_plane_state *plane_state);
>> >>
>> >> /* intel_lspcon.c */
>> >> bool lspcon_init(struct intel_digital_port *intel_dig_port);
>> >> --
>> >> 1.9.1
>> >>
>> >> _______________________________________________
>> >> Intel-gfx mailing list
>> >> Intel-gfx@lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>> >--
>> >Matt Roper
>> >Graphics Software Engineer
>> >IoTG Platform Enabling & Development
>> >Intel Corporation
>> >(916) 356-2795
>
>--
>Matt Roper
>Graphics Software Engineer
>IoTG Platform Enabling & Development
>Intel Corporation
>(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-11-01 6:33 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-26 10:01 [v5 0/2] Enable Plane Input CSC for ICL Uma Shankar
2018-10-26 9:50 ` ✗ Fi.CI.CHECKPATCH: warning for Enable Plane Input CSC for ICL (rev4) Patchwork
2018-10-26 9:52 ` ✗ Fi.CI.SPARSE: " Patchwork
2018-10-26 10:01 ` [v5 1/2] drm/i915/icl: Define Plane Input CSC Coefficient Registers Uma Shankar
2018-10-26 10:01 ` [v5 2/2] drm/i915/icl: Enable Plane Input CSC for YUV to RGB Conversion Uma Shankar
2018-10-29 23:29 ` Matt Roper
2018-10-31 12:34 ` Shankar, Uma
2018-10-31 16:40 ` Matt Roper
2018-11-01 6:33 ` Shankar, Uma
2018-10-30 10:55 ` Maarten Lankhorst
2018-10-31 12:56 ` Shankar, Uma
2018-10-26 10:08 ` ✓ Fi.CI.BAT: success for Enable Plane Input CSC for ICL (rev4) Patchwork
2018-10-26 16:28 ` ✓ Fi.CI.IGT: " Patchwork
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox