* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Enable Plane Input CSC for YUV to RGB Conversion
2018-10-23 20:11 [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB Conversion Uma Shankar
@ 2018-10-23 20:11 ` Patchwork
2018-10-23 20:12 ` ✗ Fi.CI.SPARSE: " Patchwork
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2018-10-23 20:11 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Enable Plane Input CSC for YUV to RGB Conversion
URL : https://patchwork.freedesktop.org/series/51404/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
5dc1b15925ab drm/i915: Enable Plane Input CSC for YUV to RGB Conversion
-:54: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'pipe' - possible side-effects?
#54: FILE: drivers/gpu/drm/i915/i915_reg.h:6590:
+#define PLANE_INPUT_CSC_RY_GY(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RY_GY_1(pipe), \
+ _PLANE_INPUT_CSC_RY_GY_2(pipe))
-:72: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'pipe' - possible side-effects?
#72: FILE: drivers/gpu/drm/i915/i915_reg.h:6608:
+#define PLANE_INPUT_CSC_BY(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BY_1(pipe), \
+ _PLANE_INPUT_CSC_BY_2(pipe))
-:90: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'pipe' - possible side-effects?
#90: FILE: drivers/gpu/drm/i915/i915_reg.h:6626:
+#define PLANE_INPUT_CSC_RU_GU(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RU_GU_1(pipe), \
+ _PLANE_INPUT_CSC_RU_GU_2(pipe))
-:108: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'pipe' - possible side-effects?
#108: FILE: drivers/gpu/drm/i915/i915_reg.h:6644:
+#define PLANE_INPUT_CSC_BU(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BU_1(pipe), \
+ _PLANE_INPUT_CSC_BU_2(pipe))
-:126: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'pipe' - possible side-effects?
#126: FILE: drivers/gpu/drm/i915/i915_reg.h:6662:
+#define PLANE_INPUT_CSC_RV_GV(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RV_GV_1(pipe), \
+ _PLANE_INPUT_CSC_RV_GV_2(pipe))
-:144: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'pipe' - possible side-effects?
#144: FILE: drivers/gpu/drm/i915/i915_reg.h:6680:
+#define PLANE_INPUT_CSC_BV(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BV_1(pipe), \
+ _PLANE_INPUT_CSC_BV_2(pipe))
-:162: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'pipe' - possible side-effects?
#162: FILE: drivers/gpu/drm/i915/i915_reg.h:6698:
+#define PLANE_INPUT_CSC_PREOFF_HI(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_HI_1(pipe), \
+ _PLANE_INPUT_CSC_PREOFF_HI_2(pipe))
-:180: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'pipe' - possible side-effects?
#180: FILE: drivers/gpu/drm/i915/i915_reg.h:6716:
+#define PLANE_INPUT_CSC_PREOFF_ME(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_ME_1(pipe), \
+ _PLANE_INPUT_CSC_PREOFF_ME_2(pipe))
-:198: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'pipe' - possible side-effects?
#198: FILE: drivers/gpu/drm/i915/i915_reg.h:6734:
+#define PLANE_INPUT_CSC_PREOFF_LO(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_LO_1(pipe), \
+ _PLANE_INPUT_CSC_PREOFF_LO_2(pipe))
-:216: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'pipe' - possible side-effects?
#216: FILE: drivers/gpu/drm/i915/i915_reg.h:6752:
+#define PLANE_INPUT_CSC_POSTOFF_HI(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_HI_1(pipe), \
+ _PLANE_INPUT_CSC_POSTOFF_HI_2(pipe))
-:234: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'pipe' - possible side-effects?
#234: FILE: drivers/gpu/drm/i915/i915_reg.h:6770:
+#define PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_ME_1(pipe), \
+ _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe))
-:252: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'pipe' - possible side-effects?
#252: FILE: drivers/gpu/drm/i915/i915_reg.h:6788:
+#define PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_LO_1(pipe), \
+ _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe))
-:293: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#293: FILE: drivers/gpu/drm/i915/intel_display.c:3664:
+void icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
+ const struct intel_plane_state *plane_state)
total: 0 errors, 0 warnings, 13 checks, 350 lines checked
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB Conversion
@ 2018-10-23 20:11 Uma Shankar
2018-10-23 20:11 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
` (4 more replies)
0 siblings, 5 replies; 12+ messages in thread
From: Uma Shankar @ 2018-10-23 20:11 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 plane input csc registers and 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.
Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 243 +++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_display.c | 76 ++++++++++-
2 files changed, 313 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8bd61f9..f0f6f7c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6553,6 +6553,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) /* Pre-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)
@@ -6569,6 +6570,248 @@ 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_3_A 0x703E0
+
+#define _PLANE_INPUT_CSC_RY_GY_1_B 0x711E0
+#define _PLANE_INPUT_CSC_RY_GY_2_B 0x712E0
+#define _PLANE_INPUT_CSC_RY_GY_3_B 0x713E0
+
+#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_RY_GY(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RY_GY_1(pipe), \
+ _PLANE_INPUT_CSC_RY_GY_2(pipe))
+
+#define _PLANE_INPUT_CSC_BY_1_A 0x701E4
+#define _PLANE_INPUT_CSC_BY_2_A 0x702E4
+#define _PLANE_INPUT_CSC_BY_3_A 0x703E4
+
+#define _PLANE_INPUT_CSC_BY_1_B 0x711E4
+#define _PLANE_INPUT_CSC_BY_2_B 0x712E4
+#define _PLANE_INPUT_CSC_BY_3_B 0x713E4
+
+#define _PLANE_INPUT_CSC_BY_1(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_BY_1_A, \
+ _PLANE_INPUT_CSC_BY_1_B)
+#define _PLANE_INPUT_CSC_BY_2(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_BY_2_A, \
+ _PLANE_INPUT_CSC_BY_2_B)
+#define PLANE_INPUT_CSC_BY(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BY_1(pipe), \
+ _PLANE_INPUT_CSC_BY_2(pipe))
+
+#define _PLANE_INPUT_CSC_RU_GU_1_A 0x701E8
+#define _PLANE_INPUT_CSC_RU_GU_2_A 0x702E8
+#define _PLANE_INPUT_CSC_RU_GU_3_A 0x703E8
+
+#define _PLANE_INPUT_CSC_RU_GU_1_B 0x711E8
+#define _PLANE_INPUT_CSC_RU_GU_2_B 0x712E8
+#define _PLANE_INPUT_CSC_RU_GU_3_B 0x713E8
+
+#define _PLANE_INPUT_CSC_RU_GU_1(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_RU_GU_1_A, \
+ _PLANE_INPUT_CSC_RU_GU_1_B)
+#define _PLANE_INPUT_CSC_RU_GU_2(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_RU_GU_2_A, \
+ _PLANE_INPUT_CSC_RU_GU_2_B)
+#define PLANE_INPUT_CSC_RU_GU(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RU_GU_1(pipe), \
+ _PLANE_INPUT_CSC_RU_GU_2(pipe))
+
+#define _PLANE_INPUT_CSC_BU_1_A 0x701EC
+#define _PLANE_INPUT_CSC_BU_2_A 0x702EC
+#define _PLANE_INPUT_CSC_BU_3_A 0x703EC
+
+#define _PLANE_INPUT_CSC_BU_1_B 0x711EC
+#define _PLANE_INPUT_CSC_BU_2_B 0x712EC
+#define _PLANE_INPUT_CSC_BU_3_B 0x713EC
+
+#define _PLANE_INPUT_CSC_BU_1(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_BU_1_A, \
+ _PLANE_INPUT_CSC_BU_1_B)
+#define _PLANE_INPUT_CSC_BU_2(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_BU_2_A, \
+ _PLANE_INPUT_CSC_BU_2_B)
+#define PLANE_INPUT_CSC_BU(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BU_1(pipe), \
+ _PLANE_INPUT_CSC_BU_2(pipe))
+
+#define _PLANE_INPUT_CSC_RV_GV_1_A 0x701F0
+#define _PLANE_INPUT_CSC_RV_GV_2_A 0x702F0
+#define _PLANE_INPUT_CSC_RV_GV_3_A 0x703F0
+
+#define _PLANE_INPUT_CSC_RV_GV_1_B 0x711F0
+#define _PLANE_INPUT_CSC_RV_GV_2_B 0x712F0
+#define _PLANE_INPUT_CSC_RV_GV_3_B 0x713F0
+
+#define _PLANE_INPUT_CSC_RV_GV_1(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_RV_GV_1_A, \
+ _PLANE_INPUT_CSC_RV_GV_1_B)
+#define _PLANE_INPUT_CSC_RV_GV_2(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_RV_GV_2_A, \
+ _PLANE_INPUT_CSC_RV_GV_2_B)
+#define PLANE_INPUT_CSC_RV_GV(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RV_GV_1(pipe), \
+ _PLANE_INPUT_CSC_RV_GV_2(pipe))
+
+#define _PLANE_INPUT_CSC_BV_1_A 0x701F4
+#define _PLANE_INPUT_CSC_BV_2_A 0x702F4
+#define _PLANE_INPUT_CSC_BV_3_A 0x703F4
+
+#define _PLANE_INPUT_CSC_BV_1_B 0x711F4
+#define _PLANE_INPUT_CSC_BV_2_B 0x712F4
+#define _PLANE_INPUT_CSC_BV_3_B 0x713F4
+
+#define _PLANE_INPUT_CSC_BV_1(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_BV_1_A, \
+ _PLANE_INPUT_CSC_BV_1_B)
+#define _PLANE_INPUT_CSC_BV_2(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_BV_2_A, \
+ _PLANE_INPUT_CSC_BV_2_B)
+#define PLANE_INPUT_CSC_BV(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BV_1(pipe), \
+ _PLANE_INPUT_CSC_BV_2(pipe))
+
+#define _PLANE_INPUT_CSC_PREOFF_HI_1_A 0x701F8
+#define _PLANE_INPUT_CSC_PREOFF_HI_2_A 0x702F8
+#define _PLANE_INPUT_CSC_PREOFF_HI_3_A 0x703F8
+
+#define _PLANE_INPUT_CSC_PREOFF_HI_1_B 0x711F8
+#define _PLANE_INPUT_CSC_PREOFF_HI_2_B 0x712F8
+#define _PLANE_INPUT_CSC_PREOFF_HI_3_B 0x713F8
+
+#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_HI(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_HI_1(pipe), \
+ _PLANE_INPUT_CSC_PREOFF_HI_2(pipe))
+
+#define _PLANE_INPUT_CSC_PREOFF_ME_1_A 0x701FC
+#define _PLANE_INPUT_CSC_PREOFF_ME_2_A 0x702FC
+#define _PLANE_INPUT_CSC_PREOFF_ME_3_A 0x703FC
+
+#define _PLANE_INPUT_CSC_PREOFF_ME_1_B 0x711FC
+#define _PLANE_INPUT_CSC_PREOFF_ME_2_B 0x712FC
+#define _PLANE_INPUT_CSC_PREOFF_ME_3_B 0x713FC
+
+#define _PLANE_INPUT_CSC_PREOFF_ME_1(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_ME_1_A, \
+ _PLANE_INPUT_CSC_PREOFF_ME_1_B)
+#define _PLANE_INPUT_CSC_PREOFF_ME_2(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_ME_2_A, \
+ _PLANE_INPUT_CSC_PREOFF_ME_2_B)
+#define PLANE_INPUT_CSC_PREOFF_ME(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_ME_1(pipe), \
+ _PLANE_INPUT_CSC_PREOFF_ME_2(pipe))
+
+#define _PLANE_INPUT_CSC_PREOFF_LO_1_A 0x70200
+#define _PLANE_INPUT_CSC_PREOFF_LO_2_A 0x70300
+#define _PLANE_INPUT_CSC_PREOFF_LO_3_A 0x70400
+
+#define _PLANE_INPUT_CSC_PREOFF_LO_1_B 0x71200
+#define _PLANE_INPUT_CSC_PREOFF_LO_2_B 0x71300
+#define _PLANE_INPUT_CSC_PREOFF_LO_3_B 0x71400
+
+#define _PLANE_INPUT_CSC_PREOFF_LO_1(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_LO_1_A, \
+ _PLANE_INPUT_CSC_PREOFF_LO_1_B)
+#define _PLANE_INPUT_CSC_PREOFF_LO_2(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_LO_2_A, \
+ _PLANE_INPUT_CSC_PREOFF_LO_2_B)
+#define PLANE_INPUT_CSC_PREOFF_LO(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_LO_1(pipe), \
+ _PLANE_INPUT_CSC_PREOFF_LO_2(pipe))
+
+#define _PLANE_INPUT_CSC_POSTOFF_HI_1_A 0x70204
+#define _PLANE_INPUT_CSC_POSTOFF_HI_2_A 0x70304
+#define _PLANE_INPUT_CSC_POSTOFF_HI_3_A 0x70404
+
+#define _PLANE_INPUT_CSC_POSTOFF_HI_1_B 0x71204
+#define _PLANE_INPUT_CSC_POSTOFF_HI_2_B 0x71304
+#define _PLANE_INPUT_CSC_POSTOFF_HI_3_B 0x71404
+
+#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_HI(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_HI_1(pipe), \
+ _PLANE_INPUT_CSC_POSTOFF_HI_2(pipe))
+
+#define _PLANE_INPUT_CSC_POSTOFF_ME_1_A 0x70208
+#define _PLANE_INPUT_CSC_POSTOFF_ME_2_A 0x70308
+#define _PLANE_INPUT_CSC_POSTOFF_ME_3_A 0x70408
+
+#define _PLANE_INPUT_CSC_POSTOFF_ME_1_B 0x71208
+#define _PLANE_INPUT_CSC_POSTOFF_ME_2_B 0x71308
+#define _PLANE_INPUT_CSC_POSTOFF_ME_3_B 0x71408
+
+#define _PLANE_INPUT_CSC_POSTOFF_ME_1(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_ME_1_A, \
+ _PLANE_INPUT_CSC_POSTOFF_ME_1_B)
+#define _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_ME_2_A, \
+ _PLANE_INPUT_CSC_POSTOFF_ME_2_B)
+#define PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_ME_1(pipe), \
+ _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe))
+
+#define _PLANE_INPUT_CSC_POSTOFF_LO_1_A 0x7020C
+#define _PLANE_INPUT_CSC_POSTOFF_LO_2_A 0x7030C
+#define _PLANE_INPUT_CSC_POSTOFF_LO_3_A 0x7040C
+
+#define _PLANE_INPUT_CSC_POSTOFF_LO_1_B 0x7120C
+#define _PLANE_INPUT_CSC_POSTOFF_LO_2_B 0x7130C
+#define _PLANE_INPUT_CSC_POSTOFF_LO_3_B 0x7140C
+
+#define _PLANE_INPUT_CSC_POSTOFF_LO_1(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_LO_1_A, \
+ _PLANE_INPUT_CSC_POSTOFF_LO_1_B)
+#define _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe) \
+ _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_LO_2_A, \
+ _PLANE_INPUT_CSC_POSTOFF_LO_2_B)
+#define PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane) \
+ _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_LO_1(pipe), \
+ _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe))
+/*
+ * These values are direct register values specified in the Bspec,
+ * for YUV->RGB Full Range conversion matrix (colorspace BT709)
+ */
+#define CSC_BT709_YUV_TO_RGB_RY_GY 0x7C987800
+#define CSC_BT709_YUV_TO_RGB_BY 0x0
+#define CSC_BT709_YUV_TO_RGB_RU_GU 0x9EF87800
+#define CSC_BT709_YUV_TO_RGB_BU 0xABF8
+#define CSC_BT709_YUV_TO_RGB_RV_GV 0x7800
+#define CSC_BT709_YUV_TO_RGB_BV 0x7ED8
+
+/*
+ * These values are direct register values specified in the Bspec,
+ * for YUV->RGB Full Range conversion matrix (colorspace BT601)
+ */
+#define CSC_BT601_YUV_TO_RGB_RY_GY 0x7AF87800
+#define CSC_BT601_YUV_TO_RGB_BY 0x0
+#define CSC_BT601_YUV_TO_RGB_RU_GU 0x8B287800
+#define CSC_BT601_YUV_TO_RGB_BU 0x9AC0
+#define CSC_BT601_YUV_TO_RGB_RV_GV 0x7800
+#define CSC_BT601_YUV_TO_RGB_BV 0x7DD8
+
+/* Preoffset values for YUV to RGB Conversion */
+#define PREOFF_YUV_TO_RGB_HI 0x800
+#define PREOFF_YUV_TO_RGB_ME 0xF00
+#define PREOFF_YUV_TO_RGB_LO 0x800
#define _PLANE_CTL_1_B 0x71180
#define _PLANE_CTL_2_B 0x71280
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fc7e3b0..38b41ed 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3689,12 +3689,66 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
return plane_ctl;
}
+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;
+
+ if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709) {
+ I915_WRITE(PLANE_INPUT_CSC_RY_GY(pipe, plane),
+ CSC_BT709_YUV_TO_RGB_RY_GY);
+ I915_WRITE(PLANE_INPUT_CSC_BY(pipe, plane),
+ CSC_BT709_YUV_TO_RGB_BY);
+ I915_WRITE(PLANE_INPUT_CSC_RU_GU(pipe, plane),
+ CSC_BT709_YUV_TO_RGB_RU_GU);
+ I915_WRITE(PLANE_INPUT_CSC_BU(pipe, plane),
+ CSC_BT709_YUV_TO_RGB_BU);
+ I915_WRITE(PLANE_INPUT_CSC_RV_GV(pipe, plane),
+ CSC_BT709_YUV_TO_RGB_RV_GV);
+ I915_WRITE(PLANE_INPUT_CSC_BV(pipe, plane),
+ CSC_BT709_YUV_TO_RGB_BV);
+ } else {
+ I915_WRITE(PLANE_INPUT_CSC_RY_GY(pipe, plane),
+ CSC_BT601_YUV_TO_RGB_RY_GY);
+ I915_WRITE(PLANE_INPUT_CSC_BY(pipe, plane),
+ CSC_BT601_YUV_TO_RGB_BY);
+ I915_WRITE(PLANE_INPUT_CSC_RU_GU(pipe, plane),
+ CSC_BT601_YUV_TO_RGB_RU_GU);
+ I915_WRITE(PLANE_INPUT_CSC_BU(pipe, plane),
+ CSC_BT601_YUV_TO_RGB_BU);
+ I915_WRITE(PLANE_INPUT_CSC_RV_GV(pipe, plane),
+ CSC_BT601_YUV_TO_RGB_RV_GV);
+ I915_WRITE(PLANE_INPUT_CSC_BV(pipe, plane),
+ CSC_BT601_YUV_TO_RGB_BV);
+ }
+
+ I915_WRITE(PLANE_INPUT_CSC_PREOFF_HI(pipe, plane),
+ PREOFF_YUV_TO_RGB_HI);
+ I915_WRITE(PLANE_INPUT_CSC_PREOFF_ME(pipe, plane),
+ PREOFF_YUV_TO_RGB_ME);
+ I915_WRITE(PLANE_INPUT_CSC_PREOFF_LO(pipe, plane),
+ PREOFF_YUV_TO_RGB_LO);
+
+ I915_WRITE(PLANE_INPUT_CSC_POSTOFF_HI(pipe, plane), 0x0);
+ I915_WRITE(PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane), 0x0);
+ I915_WRITE(PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane), 0x0);
+}
+
u32 glk_plane_color_ctl(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);
const struct drm_framebuffer *fb = plane_state->base.fb;
+ struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
+ enum plane_id plane_id = plane->id;
+
u32 plane_color_ctl = 0;
if (INTEL_GEN(dev_priv) < 11) {
@@ -3705,13 +3759,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 (INTEL_GEN(dev_priv) < 11 || plane_id > 2) {
+ 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;
--
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] 12+ messages in thread
* ✗ Fi.CI.SPARSE: warning for drm/i915: Enable Plane Input CSC for YUV to RGB Conversion
2018-10-23 20:11 [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB Conversion Uma Shankar
2018-10-23 20:11 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
@ 2018-10-23 20:12 ` Patchwork
2018-10-23 20:36 ` ✓ Fi.CI.BAT: success " Patchwork
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2018-10-23 20:12 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Enable Plane Input CSC for YUV to RGB Conversion
URL : https://patchwork.freedesktop.org/series/51404/
State : warning
== Summary ==
$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Enable Plane Input CSC for YUV to RGB Conversion
+drivers/gpu/drm/i915/intel_display.c:3663:6: warning: symbol 'icl_program_input_csc_coeff' was not declared. Should it be static?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Enable Plane Input CSC for YUV to RGB Conversion
2018-10-23 20:11 [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB Conversion Uma Shankar
2018-10-23 20:11 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2018-10-23 20:12 ` ✗ Fi.CI.SPARSE: " Patchwork
@ 2018-10-23 20:36 ` Patchwork
2018-10-23 23:58 ` ✓ Fi.CI.IGT: " Patchwork
2018-10-24 10:47 ` [PATCH] " Maarten Lankhorst
4 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2018-10-23 20:36 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Enable Plane Input CSC for YUV to RGB Conversion
URL : https://patchwork.freedesktop.org/series/51404/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_5023 -> Patchwork_10552 =
== Summary - SUCCESS ==
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/51404/revisions/1/mbox/
== Known issues ==
Here are the changes found in Patchwork_10552 that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
fi-icl-u: NOTRUN -> INCOMPLETE (fdo#107713)
igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
fi-byt-clapper: PASS -> FAIL (fdo#107362, fdo#103191) +1
==== Possible fixes ====
igt@drv_selftest@live_execlists:
fi-apl-guc: INCOMPLETE (fdo#106693) -> PASS
igt@gem_ctx_switch@basic-default:
fi-icl-u: DMESG-FAIL -> PASS
igt@gem_exec_suspend@basic-s3:
fi-blb-e6850: INCOMPLETE (fdo#107718) -> PASS
igt@kms_pipe_crc_basic@read-crc-pipe-a:
fi-byt-clapper: FAIL (fdo#107362) -> PASS
fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
fdo#106693 https://bugs.freedesktop.org/show_bug.cgi?id=106693
fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
fdo#107713 https://bugs.freedesktop.org/show_bug.cgi?id=107713
fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
== Participating hosts (49 -> 44) ==
Additional (1): fi-kbl-7560u
Missing (6): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600
== Build changes ==
* Linux: CI_DRM_5023 -> Patchwork_10552
CI_DRM_5023: 166bc98d7b77005943ab670506f164783cdc3f56 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4688: fa6dbf8c048961356fd642df047cb58ab49309b2 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10552: 5dc1b15925ab1d33088de2914386fbb82e69d379 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
5dc1b15925ab drm/i915: Enable Plane Input CSC for YUV to RGB Conversion
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10552/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915: Enable Plane Input CSC for YUV to RGB Conversion
2018-10-23 20:11 [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB Conversion Uma Shankar
` (2 preceding siblings ...)
2018-10-23 20:36 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-10-23 23:58 ` Patchwork
2018-10-24 10:47 ` [PATCH] " Maarten Lankhorst
4 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2018-10-23 23:58 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Enable Plane Input CSC for YUV to RGB Conversion
URL : https://patchwork.freedesktop.org/series/51404/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_5023_full -> Patchwork_10552_full =
== Summary - WARNING ==
Minor unknown changes coming with Patchwork_10552_full need to be verified
manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_10552_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_10552_full:
=== IGT changes ===
==== Warnings ====
igt@kms_atomic_transition@2x-modeset-transitions:
shard-hsw: PASS -> SKIP
igt@pm_rc6_residency@rc6-accuracy:
shard-kbl: PASS -> SKIP
shard-snb: PASS -> SKIP
== Known issues ==
Here are the changes found in Patchwork_10552_full that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@gem_ctx_bad_destroy@invalid-default-ctx:
shard-apl: PASS -> DMESG-WARN (fdo#103558, fdo#105602) +13
igt@kms_busy@extended-pageflip-hang-newfb-render-b:
shard-apl: NOTRUN -> DMESG-WARN (fdo#107956)
igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b:
shard-snb: NOTRUN -> DMESG-WARN (fdo#107956)
igt@kms_color@pipe-b-degamma:
shard-apl: PASS -> FAIL (fdo#104782)
igt@kms_cursor_crc@cursor-256x85-onscreen:
shard-apl: NOTRUN -> FAIL (fdo#103232)
igt@kms_cursor_crc@cursor-256x85-random:
shard-apl: PASS -> FAIL (fdo#103232) +2
igt@kms_cursor_crc@cursor-64x64-sliding:
shard-glk: PASS -> FAIL (fdo#103232) +1
igt@kms_cursor_crc@cursor-64x64-suspend:
shard-apl: NOTRUN -> FAIL (fdo#103232, fdo#103191)
igt@kms_flip@flip-vs-fences-interruptible:
shard-kbl: PASS -> DMESG-WARN (fdo#105345)
igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-blt:
shard-apl: PASS -> FAIL (fdo#103167) +1
igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-onoff:
shard-glk: PASS -> FAIL (fdo#103167) +4
igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min:
shard-kbl: PASS -> DMESG-WARN (fdo#105345, fdo#103313) +1
igt@kms_plane_alpha_blend@pipe-c-alpha-7efc:
shard-skl: NOTRUN -> FAIL (fdo#108146)
igt@kms_plane_alpha_blend@pipe-c-constant-alpha-max:
shard-apl: PASS -> DMESG-FAIL (fdo#108145, fdo#103558, fdo#105602)
igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
shard-apl: PASS -> FAIL (fdo#103166) +1
igt@kms_plane_multiple@atomic-pipe-c-tiling-y:
shard-glk: PASS -> FAIL (fdo#103166) +1
igt@kms_setmode@basic:
shard-snb: NOTRUN -> FAIL (fdo#99912)
igt@pm_rpm@sysfs-read:
shard-skl: PASS -> INCOMPLETE (fdo#107807)
igt@pm_rpm@system-suspend-execbuf:
shard-skl: PASS -> INCOMPLETE (fdo#104108, fdo#107807)
==== Possible fixes ====
igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-c:
shard-apl: DMESG-WARN (fdo#107956) -> PASS
igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
shard-glk: FAIL (fdo#108145) -> PASS
igt@kms_cursor_crc@cursor-256x256-sliding:
shard-glk: FAIL (fdo#103232) -> PASS +1
igt@kms_cursor_crc@cursor-64x21-offscreen:
shard-skl: FAIL (fdo#103232) -> PASS
igt@kms_cursor_crc@cursor-size-change:
shard-apl: FAIL (fdo#103232) -> PASS
igt@kms_flip@busy-flip-interruptible:
shard-skl: FAIL (fdo#103257) -> PASS
igt@kms_flip@flip-vs-expired-vblank-interruptible:
shard-skl: FAIL (fdo#105363) -> PASS
igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-wc:
shard-apl: FAIL (fdo#103167) -> PASS
igt@kms_frontbuffer_tracking@psr-rgb101010-draw-mmap-cpu:
shard-skl: FAIL (fdo#103167) -> PASS +1
igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
shard-skl: INCOMPLETE (fdo#104108, fdo#107773) -> PASS
igt@kms_rotation_crc@sprite-rotation-180:
shard-apl: INCOMPLETE (fdo#103927) -> PASS
igt@pm_rpm@legacy-planes-dpms:
shard-skl: INCOMPLETE (fdo#107807, fdo#105959) -> PASS
==== Warnings ====
igt@kms_sysfs_edid_timing:
shard-apl: FAIL (fdo#100047) -> DMESG-FAIL (fdo#103558, fdo#105602, fdo#100047)
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
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#103257 https://bugs.freedesktop.org/show_bug.cgi?id=103257
fdo#103313 https://bugs.freedesktop.org/show_bug.cgi?id=103313
fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
fdo#104782 https://bugs.freedesktop.org/show_bug.cgi?id=104782
fdo#105345 https://bugs.freedesktop.org/show_bug.cgi?id=105345
fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
fdo#105959 https://bugs.freedesktop.org/show_bug.cgi?id=105959
fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807
fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
fdo#108146 https://bugs.freedesktop.org/show_bug.cgi?id=108146
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
== Participating hosts (6 -> 6) ==
No changes in participating hosts
== Build changes ==
* Linux: CI_DRM_5023 -> Patchwork_10552
CI_DRM_5023: 166bc98d7b77005943ab670506f164783cdc3f56 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4688: fa6dbf8c048961356fd642df047cb58ab49309b2 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10552: 5dc1b15925ab1d33088de2914386fbb82e69d379 @ 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_10552/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB Conversion
2018-10-23 20:11 [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB Conversion Uma Shankar
` (3 preceding siblings ...)
2018-10-23 23:58 ` ✓ Fi.CI.IGT: " Patchwork
@ 2018-10-24 10:47 ` Maarten Lankhorst
2018-10-24 11:19 ` Shankar, Uma
4 siblings, 1 reply; 12+ messages in thread
From: Maarten Lankhorst @ 2018-10-24 10:47 UTC (permalink / raw)
To: Uma Shankar, intel-gfx; +Cc: ville.syrjala, maarten.lankhorst
Op 23-10-18 om 22:11 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 plane input csc registers and 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.
>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 243 +++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_display.c | 76 ++++++++++-
> 2 files changed, 313 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8bd61f9..f0f6f7c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6553,6 +6553,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) /* Pre-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)
> @@ -6569,6 +6570,248 @@ 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_3_A 0x703E0
> +
> +#define _PLANE_INPUT_CSC_RY_GY_1_B 0x711E0
> +#define _PLANE_INPUT_CSC_RY_GY_2_B 0x712E0
> +#define _PLANE_INPUT_CSC_RY_GY_3_B 0x713E0
> +
> +#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_RY_GY(pipe, plane) \
> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RY_GY_1(pipe), \
> + _PLANE_INPUT_CSC_RY_GY_2(pipe))
> +
> +#define _PLANE_INPUT_CSC_BY_1_A 0x701E4
> +#define _PLANE_INPUT_CSC_BY_2_A 0x702E4
> +#define _PLANE_INPUT_CSC_BY_3_A 0x703E4
> +
> +#define _PLANE_INPUT_CSC_BY_1_B 0x711E4
> +#define _PLANE_INPUT_CSC_BY_2_B 0x712E4
> +#define _PLANE_INPUT_CSC_BY_3_B 0x713E4
> +
> +#define _PLANE_INPUT_CSC_BY_1(pipe) \
> + _PIPE(pipe, _PLANE_INPUT_CSC_BY_1_A, \
> + _PLANE_INPUT_CSC_BY_1_B)
> +#define _PLANE_INPUT_CSC_BY_2(pipe) \
> + _PIPE(pipe, _PLANE_INPUT_CSC_BY_2_A, \
> + _PLANE_INPUT_CSC_BY_2_B)
> +#define PLANE_INPUT_CSC_BY(pipe, plane) \
> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BY_1(pipe), \
> + _PLANE_INPUT_CSC_BY_2(pipe))
> +
> +#define _PLANE_INPUT_CSC_RU_GU_1_A 0x701E8
> +#define _PLANE_INPUT_CSC_RU_GU_2_A 0x702E8
> +#define _PLANE_INPUT_CSC_RU_GU_3_A 0x703E8
> +
> +#define _PLANE_INPUT_CSC_RU_GU_1_B 0x711E8
> +#define _PLANE_INPUT_CSC_RU_GU_2_B 0x712E8
> +#define _PLANE_INPUT_CSC_RU_GU_3_B 0x713E8
> +
> +#define _PLANE_INPUT_CSC_RU_GU_1(pipe) \
> + _PIPE(pipe, _PLANE_INPUT_CSC_RU_GU_1_A, \
> + _PLANE_INPUT_CSC_RU_GU_1_B)
> +#define _PLANE_INPUT_CSC_RU_GU_2(pipe) \
> + _PIPE(pipe, _PLANE_INPUT_CSC_RU_GU_2_A, \
> + _PLANE_INPUT_CSC_RU_GU_2_B)
> +#define PLANE_INPUT_CSC_RU_GU(pipe, plane) \
> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RU_GU_1(pipe), \
> + _PLANE_INPUT_CSC_RU_GU_2(pipe))
> +
> +#define _PLANE_INPUT_CSC_BU_1_A 0x701EC
> +#define _PLANE_INPUT_CSC_BU_2_A 0x702EC
> +#define _PLANE_INPUT_CSC_BU_3_A 0x703EC
> +
> +#define _PLANE_INPUT_CSC_BU_1_B 0x711EC
> +#define _PLANE_INPUT_CSC_BU_2_B 0x712EC
> +#define _PLANE_INPUT_CSC_BU_3_B 0x713EC
> +
> +#define _PLANE_INPUT_CSC_BU_1(pipe) \
> + _PIPE(pipe, _PLANE_INPUT_CSC_BU_1_A, \
> + _PLANE_INPUT_CSC_BU_1_B)
> +#define _PLANE_INPUT_CSC_BU_2(pipe) \
> + _PIPE(pipe, _PLANE_INPUT_CSC_BU_2_A, \
> + _PLANE_INPUT_CSC_BU_2_B)
> +#define PLANE_INPUT_CSC_BU(pipe, plane) \
> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BU_1(pipe), \
> + _PLANE_INPUT_CSC_BU_2(pipe))
> +
> +#define _PLANE_INPUT_CSC_RV_GV_1_A 0x701F0
> +#define _PLANE_INPUT_CSC_RV_GV_2_A 0x702F0
> +#define _PLANE_INPUT_CSC_RV_GV_3_A 0x703F0
> +
> +#define _PLANE_INPUT_CSC_RV_GV_1_B 0x711F0
> +#define _PLANE_INPUT_CSC_RV_GV_2_B 0x712F0
> +#define _PLANE_INPUT_CSC_RV_GV_3_B 0x713F0
> +
> +#define _PLANE_INPUT_CSC_RV_GV_1(pipe) \
> + _PIPE(pipe, _PLANE_INPUT_CSC_RV_GV_1_A, \
> + _PLANE_INPUT_CSC_RV_GV_1_B)
> +#define _PLANE_INPUT_CSC_RV_GV_2(pipe) \
> + _PIPE(pipe, _PLANE_INPUT_CSC_RV_GV_2_A, \
> + _PLANE_INPUT_CSC_RV_GV_2_B)
> +#define PLANE_INPUT_CSC_RV_GV(pipe, plane) \
> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RV_GV_1(pipe), \
> + _PLANE_INPUT_CSC_RV_GV_2(pipe))
> +
> +#define _PLANE_INPUT_CSC_BV_1_A 0x701F4
> +#define _PLANE_INPUT_CSC_BV_2_A 0x702F4
> +#define _PLANE_INPUT_CSC_BV_3_A 0x703F4
> +
> +#define _PLANE_INPUT_CSC_BV_1_B 0x711F4
> +#define _PLANE_INPUT_CSC_BV_2_B 0x712F4
> +#define _PLANE_INPUT_CSC_BV_3_B 0x713F4
> +
> +#define _PLANE_INPUT_CSC_BV_1(pipe) \
> + _PIPE(pipe, _PLANE_INPUT_CSC_BV_1_A, \
> + _PLANE_INPUT_CSC_BV_1_B)
> +#define _PLANE_INPUT_CSC_BV_2(pipe) \
> + _PIPE(pipe, _PLANE_INPUT_CSC_BV_2_A, \
> + _PLANE_INPUT_CSC_BV_2_B)
> +#define PLANE_INPUT_CSC_BV(pipe, plane) \
> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BV_1(pipe), \
> + _PLANE_INPUT_CSC_BV_2(pipe))
> +
> +#define _PLANE_INPUT_CSC_PREOFF_HI_1_A 0x701F8
> +#define _PLANE_INPUT_CSC_PREOFF_HI_2_A 0x702F8
> +#define _PLANE_INPUT_CSC_PREOFF_HI_3_A 0x703F8
> +
> +#define _PLANE_INPUT_CSC_PREOFF_HI_1_B 0x711F8
> +#define _PLANE_INPUT_CSC_PREOFF_HI_2_B 0x712F8
> +#define _PLANE_INPUT_CSC_PREOFF_HI_3_B 0x713F8
> +
> +#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_HI(pipe, plane) \
> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_HI_1(pipe), \
> + _PLANE_INPUT_CSC_PREOFF_HI_2(pipe))
> +
> +#define _PLANE_INPUT_CSC_PREOFF_ME_1_A 0x701FC
> +#define _PLANE_INPUT_CSC_PREOFF_ME_2_A 0x702FC
> +#define _PLANE_INPUT_CSC_PREOFF_ME_3_A 0x703FC
> +
> +#define _PLANE_INPUT_CSC_PREOFF_ME_1_B 0x711FC
> +#define _PLANE_INPUT_CSC_PREOFF_ME_2_B 0x712FC
> +#define _PLANE_INPUT_CSC_PREOFF_ME_3_B 0x713FC
> +
> +#define _PLANE_INPUT_CSC_PREOFF_ME_1(pipe) \
> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_ME_1_A, \
> + _PLANE_INPUT_CSC_PREOFF_ME_1_B)
> +#define _PLANE_INPUT_CSC_PREOFF_ME_2(pipe) \
> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_ME_2_A, \
> + _PLANE_INPUT_CSC_PREOFF_ME_2_B)
> +#define PLANE_INPUT_CSC_PREOFF_ME(pipe, plane) \
> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_ME_1(pipe), \
> + _PLANE_INPUT_CSC_PREOFF_ME_2(pipe))
> +
> +#define _PLANE_INPUT_CSC_PREOFF_LO_1_A 0x70200
> +#define _PLANE_INPUT_CSC_PREOFF_LO_2_A 0x70300
> +#define _PLANE_INPUT_CSC_PREOFF_LO_3_A 0x70400
> +
> +#define _PLANE_INPUT_CSC_PREOFF_LO_1_B 0x71200
> +#define _PLANE_INPUT_CSC_PREOFF_LO_2_B 0x71300
> +#define _PLANE_INPUT_CSC_PREOFF_LO_3_B 0x71400
> +
> +#define _PLANE_INPUT_CSC_PREOFF_LO_1(pipe) \
> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_LO_1_A, \
> + _PLANE_INPUT_CSC_PREOFF_LO_1_B)
> +#define _PLANE_INPUT_CSC_PREOFF_LO_2(pipe) \
> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_LO_2_A, \
> + _PLANE_INPUT_CSC_PREOFF_LO_2_B)
> +#define PLANE_INPUT_CSC_PREOFF_LO(pipe, plane) \
> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_LO_1(pipe), \
> + _PLANE_INPUT_CSC_PREOFF_LO_2(pipe))
> +
> +#define _PLANE_INPUT_CSC_POSTOFF_HI_1_A 0x70204
> +#define _PLANE_INPUT_CSC_POSTOFF_HI_2_A 0x70304
> +#define _PLANE_INPUT_CSC_POSTOFF_HI_3_A 0x70404
> +
> +#define _PLANE_INPUT_CSC_POSTOFF_HI_1_B 0x71204
> +#define _PLANE_INPUT_CSC_POSTOFF_HI_2_B 0x71304
> +#define _PLANE_INPUT_CSC_POSTOFF_HI_3_B 0x71404
> +
> +#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_HI(pipe, plane) \
> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_HI_1(pipe), \
> + _PLANE_INPUT_CSC_POSTOFF_HI_2(pipe))
> +
> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1_A 0x70208
> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2_A 0x70308
> +#define _PLANE_INPUT_CSC_POSTOFF_ME_3_A 0x70408
> +
> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1_B 0x71208
> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2_B 0x71308
> +#define _PLANE_INPUT_CSC_POSTOFF_ME_3_B 0x71408
> +
> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1(pipe) \
> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_ME_1_A, \
> + _PLANE_INPUT_CSC_POSTOFF_ME_1_B)
> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe) \
> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_ME_2_A, \
> + _PLANE_INPUT_CSC_POSTOFF_ME_2_B)
> +#define PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane) \
> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_ME_1(pipe), \
> + _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe))
> +
> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1_A 0x7020C
> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2_A 0x7030C
> +#define _PLANE_INPUT_CSC_POSTOFF_LO_3_A 0x7040C
> +
> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1_B 0x7120C
> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2_B 0x7130C
> +#define _PLANE_INPUT_CSC_POSTOFF_LO_3_B 0x7140C
> +
> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1(pipe) \
> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_LO_1_A, \
> + _PLANE_INPUT_CSC_POSTOFF_LO_1_B)
> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe) \
> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_LO_2_A, \
> + _PLANE_INPUT_CSC_POSTOFF_LO_2_B)
> +#define PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane) \
> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_LO_1(pipe), \
> + _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe))
Would probably be best to keep those separate.
> +/*
> + * These values are direct register values specified in the Bspec,
> + * for YUV->RGB Full Range conversion matrix (colorspace BT709)
> + */
> +#define CSC_BT709_YUV_TO_RGB_RY_GY 0x7C987800
> +#define CSC_BT709_YUV_TO_RGB_BY 0x0
> +#define CSC_BT709_YUV_TO_RGB_RU_GU 0x9EF87800
> +#define CSC_BT709_YUV_TO_RGB_BU 0xABF8
> +#define CSC_BT709_YUV_TO_RGB_RV_GV 0x7800
> +#define CSC_BT709_YUV_TO_RGB_BV 0x7ED8
> +
> +/*
> + * These values are direct register values specified in the Bspec,
> + * for YUV->RGB Full Range conversion matrix (colorspace BT601)
> + */
> +#define CSC_BT601_YUV_TO_RGB_RY_GY 0x7AF87800
> +#define CSC_BT601_YUV_TO_RGB_BY 0x0
> +#define CSC_BT601_YUV_TO_RGB_RU_GU 0x8B287800
> +#define CSC_BT601_YUV_TO_RGB_BU 0x9AC0
> +#define CSC_BT601_YUV_TO_RGB_RV_GV 0x7800
> +#define CSC_BT601_YUV_TO_RGB_BV 0x7DD8
> +
> +/* Preoffset values for YUV to RGB Conversion */
> +#define PREOFF_YUV_TO_RGB_HI 0x800
> +#define PREOFF_YUV_TO_RGB_ME 0xF00
> +#define PREOFF_YUV_TO_RGB_LO 0x800
>
> #define _PLANE_CTL_1_B 0x71180
> #define _PLANE_CTL_2_B 0x71280
Probably move those coefficients to intel_color.c ?
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fc7e3b0..38b41ed 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3689,12 +3689,66 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> return plane_ctl;
> }
>
> +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;
> +
> + if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709) {
> + I915_WRITE(PLANE_INPUT_CSC_RY_GY(pipe, plane),
> + CSC_BT709_YUV_TO_RGB_RY_GY);
> + I915_WRITE(PLANE_INPUT_CSC_BY(pipe, plane),
> + CSC_BT709_YUV_TO_RGB_BY);
> + I915_WRITE(PLANE_INPUT_CSC_RU_GU(pipe, plane),
> + CSC_BT709_YUV_TO_RGB_RU_GU);
> + I915_WRITE(PLANE_INPUT_CSC_BU(pipe, plane),
> + CSC_BT709_YUV_TO_RGB_BU);
> + I915_WRITE(PLANE_INPUT_CSC_RV_GV(pipe, plane),
> + CSC_BT709_YUV_TO_RGB_RV_GV);
> + I915_WRITE(PLANE_INPUT_CSC_BV(pipe, plane),
> + CSC_BT709_YUV_TO_RGB_BV);
> + } else {
> + I915_WRITE(PLANE_INPUT_CSC_RY_GY(pipe, plane),
> + CSC_BT601_YUV_TO_RGB_RY_GY);
> + I915_WRITE(PLANE_INPUT_CSC_BY(pipe, plane),
> + CSC_BT601_YUV_TO_RGB_BY);
> + I915_WRITE(PLANE_INPUT_CSC_RU_GU(pipe, plane),
> + CSC_BT601_YUV_TO_RGB_RU_GU);
> + I915_WRITE(PLANE_INPUT_CSC_BU(pipe, plane),
> + CSC_BT601_YUV_TO_RGB_BU);
> + I915_WRITE(PLANE_INPUT_CSC_RV_GV(pipe, plane),
> + CSC_BT601_YUV_TO_RGB_RV_GV);
> + I915_WRITE(PLANE_INPUT_CSC_BV(pipe, plane),
> + CSC_BT601_YUV_TO_RGB_BV);
> + }
Considering there's going to be BT.601, BT.709 and perhaps newer colorspaces with limited vs full range, would it make more sense to generate the values?
> +
> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_HI(pipe, plane),
> + PREOFF_YUV_TO_RGB_HI);
> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_ME(pipe, plane),
> + PREOFF_YUV_TO_RGB_ME);
> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_LO(pipe, plane),
> + PREOFF_YUV_TO_RGB_LO);
> +
> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_HI(pipe, plane), 0x0);
> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane), 0x0);
> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane), 0x0);
> +}
We already have some support code for CSC matrices in intel_color.c, so I think it makes sense to program this from there..
> u32 glk_plane_color_ctl(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);
> const struct drm_framebuffer *fb = plane_state->base.fb;
> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> + enum plane_id plane_id = plane->id;
> +
> u32 plane_color_ctl = 0;
>
> if (INTEL_GEN(dev_priv) < 11) {
> @@ -3705,13 +3759,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 (INTEL_GEN(dev_priv) < 11 || plane_id > 2) {
We now have icl_is_hdr_plane(), it just landed :)
> + 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;
The coefficients are likely different for full vs limited range, and I don't see that handled at the moment. :(
> + }
> }
>
> return plane_color_ctl;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB Conversion
2018-10-24 10:47 ` [PATCH] " Maarten Lankhorst
@ 2018-10-24 11:19 ` Shankar, Uma
2018-10-24 12:07 ` Maarten Lankhorst
0 siblings, 1 reply; 12+ messages in thread
From: Shankar, Uma @ 2018-10-24 11:19 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: Wednesday, October 24, 2018 4:18 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] [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB
>Conversion
>
>Op 23-10-18 om 22:11 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 plane input csc registers and 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.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_reg.h | 243
>+++++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/intel_display.c | 76 ++++++++++-
>> 2 files changed, 313 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h index 8bd61f9..f0f6f7c 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -6553,6 +6553,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) /* Pre-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)
>> @@ -6569,6 +6570,248 @@ 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_3_A 0x703E0
>> +
>> +#define _PLANE_INPUT_CSC_RY_GY_1_B 0x711E0
>> +#define _PLANE_INPUT_CSC_RY_GY_2_B 0x712E0
>> +#define _PLANE_INPUT_CSC_RY_GY_3_B 0x713E0
>> +
>> +#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_RY_GY(pipe, plane) \
>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RY_GY_1(pipe), \
>> + _PLANE_INPUT_CSC_RY_GY_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_BY_1_A 0x701E4
>> +#define _PLANE_INPUT_CSC_BY_2_A 0x702E4
>> +#define _PLANE_INPUT_CSC_BY_3_A 0x703E4
>> +
>> +#define _PLANE_INPUT_CSC_BY_1_B 0x711E4
>> +#define _PLANE_INPUT_CSC_BY_2_B 0x712E4
>> +#define _PLANE_INPUT_CSC_BY_3_B 0x713E4
>> +
>> +#define _PLANE_INPUT_CSC_BY_1(pipe) \
>> + _PIPE(pipe, _PLANE_INPUT_CSC_BY_1_A, \
>> + _PLANE_INPUT_CSC_BY_1_B)
>> +#define _PLANE_INPUT_CSC_BY_2(pipe) \
>> + _PIPE(pipe, _PLANE_INPUT_CSC_BY_2_A, \
>> + _PLANE_INPUT_CSC_BY_2_B)
>> +#define PLANE_INPUT_CSC_BY(pipe, plane) \
>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BY_1(pipe), \
>> + _PLANE_INPUT_CSC_BY_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_RU_GU_1_A 0x701E8
>> +#define _PLANE_INPUT_CSC_RU_GU_2_A 0x702E8
>> +#define _PLANE_INPUT_CSC_RU_GU_3_A 0x703E8
>> +
>> +#define _PLANE_INPUT_CSC_RU_GU_1_B 0x711E8
>> +#define _PLANE_INPUT_CSC_RU_GU_2_B 0x712E8
>> +#define _PLANE_INPUT_CSC_RU_GU_3_B 0x713E8
>> +
>> +#define _PLANE_INPUT_CSC_RU_GU_1(pipe) \
>> + _PIPE(pipe, _PLANE_INPUT_CSC_RU_GU_1_A, \
>> + _PLANE_INPUT_CSC_RU_GU_1_B)
>> +#define _PLANE_INPUT_CSC_RU_GU_2(pipe) \
>> + _PIPE(pipe, _PLANE_INPUT_CSC_RU_GU_2_A, \
>> + _PLANE_INPUT_CSC_RU_GU_2_B)
>> +#define PLANE_INPUT_CSC_RU_GU(pipe, plane) \
>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RU_GU_1(pipe), \
>> + _PLANE_INPUT_CSC_RU_GU_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_BU_1_A 0x701EC
>> +#define _PLANE_INPUT_CSC_BU_2_A 0x702EC
>> +#define _PLANE_INPUT_CSC_BU_3_A 0x703EC
>> +
>> +#define _PLANE_INPUT_CSC_BU_1_B 0x711EC
>> +#define _PLANE_INPUT_CSC_BU_2_B 0x712EC
>> +#define _PLANE_INPUT_CSC_BU_3_B 0x713EC
>> +
>> +#define _PLANE_INPUT_CSC_BU_1(pipe) \
>> + _PIPE(pipe, _PLANE_INPUT_CSC_BU_1_A, \
>> + _PLANE_INPUT_CSC_BU_1_B)
>> +#define _PLANE_INPUT_CSC_BU_2(pipe) \
>> + _PIPE(pipe, _PLANE_INPUT_CSC_BU_2_A, \
>> + _PLANE_INPUT_CSC_BU_2_B)
>> +#define PLANE_INPUT_CSC_BU(pipe, plane) \
>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BU_1(pipe), \
>> + _PLANE_INPUT_CSC_BU_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_RV_GV_1_A 0x701F0
>> +#define _PLANE_INPUT_CSC_RV_GV_2_A 0x702F0
>> +#define _PLANE_INPUT_CSC_RV_GV_3_A 0x703F0
>> +
>> +#define _PLANE_INPUT_CSC_RV_GV_1_B 0x711F0
>> +#define _PLANE_INPUT_CSC_RV_GV_2_B 0x712F0
>> +#define _PLANE_INPUT_CSC_RV_GV_3_B 0x713F0
>> +
>> +#define _PLANE_INPUT_CSC_RV_GV_1(pipe) \
>> + _PIPE(pipe, _PLANE_INPUT_CSC_RV_GV_1_A, \
>> + _PLANE_INPUT_CSC_RV_GV_1_B)
>> +#define _PLANE_INPUT_CSC_RV_GV_2(pipe) \
>> + _PIPE(pipe, _PLANE_INPUT_CSC_RV_GV_2_A, \
>> + _PLANE_INPUT_CSC_RV_GV_2_B)
>> +#define PLANE_INPUT_CSC_RV_GV(pipe, plane) \
>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RV_GV_1(pipe), \
>> + _PLANE_INPUT_CSC_RV_GV_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_BV_1_A 0x701F4
>> +#define _PLANE_INPUT_CSC_BV_2_A 0x702F4
>> +#define _PLANE_INPUT_CSC_BV_3_A 0x703F4
>> +
>> +#define _PLANE_INPUT_CSC_BV_1_B 0x711F4
>> +#define _PLANE_INPUT_CSC_BV_2_B 0x712F4
>> +#define _PLANE_INPUT_CSC_BV_3_B 0x713F4
>> +
>> +#define _PLANE_INPUT_CSC_BV_1(pipe) \
>> + _PIPE(pipe, _PLANE_INPUT_CSC_BV_1_A, \
>> + _PLANE_INPUT_CSC_BV_1_B)
>> +#define _PLANE_INPUT_CSC_BV_2(pipe) \
>> + _PIPE(pipe, _PLANE_INPUT_CSC_BV_2_A, \
>> + _PLANE_INPUT_CSC_BV_2_B)
>> +#define PLANE_INPUT_CSC_BV(pipe, plane) \
>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BV_1(pipe), \
>> + _PLANE_INPUT_CSC_BV_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_PREOFF_HI_1_A 0x701F8
>> +#define _PLANE_INPUT_CSC_PREOFF_HI_2_A 0x702F8
>> +#define _PLANE_INPUT_CSC_PREOFF_HI_3_A 0x703F8
>> +
>> +#define _PLANE_INPUT_CSC_PREOFF_HI_1_B 0x711F8
>> +#define _PLANE_INPUT_CSC_PREOFF_HI_2_B 0x712F8
>> +#define _PLANE_INPUT_CSC_PREOFF_HI_3_B 0x713F8
>> +
>> +#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_HI(pipe, plane) \
>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_HI_1(pipe), \
>> + _PLANE_INPUT_CSC_PREOFF_HI_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1_A 0x701FC
>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2_A 0x702FC
>> +#define _PLANE_INPUT_CSC_PREOFF_ME_3_A 0x703FC
>> +
>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1_B 0x711FC
>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2_B 0x712FC
>> +#define _PLANE_INPUT_CSC_PREOFF_ME_3_B 0x713FC
>> +
>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1(pipe) \
>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_ME_1_A, \
>> + _PLANE_INPUT_CSC_PREOFF_ME_1_B)
>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2(pipe) \
>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_ME_2_A, \
>> + _PLANE_INPUT_CSC_PREOFF_ME_2_B)
>> +#define PLANE_INPUT_CSC_PREOFF_ME(pipe, plane) \
>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_ME_1(pipe), \
>> + _PLANE_INPUT_CSC_PREOFF_ME_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1_A 0x70200
>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2_A 0x70300
>> +#define _PLANE_INPUT_CSC_PREOFF_LO_3_A 0x70400
>> +
>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1_B 0x71200
>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2_B 0x71300
>> +#define _PLANE_INPUT_CSC_PREOFF_LO_3_B 0x71400
>> +
>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1(pipe) \
>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_LO_1_A, \
>> + _PLANE_INPUT_CSC_PREOFF_LO_1_B)
>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2(pipe) \
>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_LO_2_A, \
>> + _PLANE_INPUT_CSC_PREOFF_LO_2_B)
>> +#define PLANE_INPUT_CSC_PREOFF_LO(pipe, plane) \
>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_LO_1(pipe), \
>> + _PLANE_INPUT_CSC_PREOFF_LO_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_1_A 0x70204
>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_2_A 0x70304
>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_3_A 0x70404
>> +
>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_1_B 0x71204
>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_2_B 0x71304
>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_3_B 0x71404
>> +
>> +#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_HI(pipe, plane) \
>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_HI_1(pipe), \
>> + _PLANE_INPUT_CSC_POSTOFF_HI_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1_A 0x70208
>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2_A 0x70308
>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_3_A 0x70408
>> +
>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1_B 0x71208
>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2_B 0x71308
>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_3_B 0x71408
>> +
>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1(pipe) \
>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_ME_1_A, \
>> + _PLANE_INPUT_CSC_POSTOFF_ME_1_B)
>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe) \
>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_ME_2_A, \
>> + _PLANE_INPUT_CSC_POSTOFF_ME_2_B)
>> +#define PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane) \
>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_ME_1(pipe), \
>> + _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1_A 0x7020C
>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2_A 0x7030C
>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_3_A 0x7040C
>> +
>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1_B 0x7120C
>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2_B 0x7130C
>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_3_B 0x7140C
>> +
>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1(pipe) \
>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_LO_1_A, \
>> + _PLANE_INPUT_CSC_POSTOFF_LO_1_B)
>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe) \
>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_LO_2_A, \
>> + _PLANE_INPUT_CSC_POSTOFF_LO_2_B)
>> +#define PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane) \
>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_LO_1(pipe), \
>> + _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe))
>Would probably be best to keep those separate.
You mean in a separate patch ? If yes, I can do that and segregate the register macro
definitions in a separate patch.
>> +/*
>> + * These values are direct register values specified in the Bspec,
>> + * for YUV->RGB Full Range conversion matrix (colorspace BT709) */
>> +#define CSC_BT709_YUV_TO_RGB_RY_GY 0x7C987800
>> +#define CSC_BT709_YUV_TO_RGB_BY 0x0
>> +#define CSC_BT709_YUV_TO_RGB_RU_GU 0x9EF87800
>> +#define CSC_BT709_YUV_TO_RGB_BU 0xABF8
>> +#define CSC_BT709_YUV_TO_RGB_RV_GV 0x7800
>> +#define CSC_BT709_YUV_TO_RGB_BV 0x7ED8
>> +
>> +/*
>> + * These values are direct register values specified in the Bspec,
>> + * for YUV->RGB Full Range conversion matrix (colorspace BT601) */
>> +#define CSC_BT601_YUV_TO_RGB_RY_GY 0x7AF87800
>> +#define CSC_BT601_YUV_TO_RGB_BY 0x0
>> +#define CSC_BT601_YUV_TO_RGB_RU_GU 0x8B287800
>> +#define CSC_BT601_YUV_TO_RGB_BU 0x9AC0
>> +#define CSC_BT601_YUV_TO_RGB_RV_GV 0x7800
>> +#define CSC_BT601_YUV_TO_RGB_BV 0x7DD8
>> +
>> +/* Preoffset values for YUV to RGB Conversion */
>> +#define PREOFF_YUV_TO_RGB_HI 0x800
>> +#define PREOFF_YUV_TO_RGB_ME 0xF00
>> +#define PREOFF_YUV_TO_RGB_LO 0x800
>>
>> #define _PLANE_CTL_1_B 0x71180
>> #define _PLANE_CTL_2_B 0x71280
>Probably move those coefficients to intel_color.c ?
Sure, I will do that.
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index fc7e3b0..38b41ed 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3689,12 +3689,66 @@ u32 skl_plane_ctl(const struct intel_crtc_state
>*crtc_state,
>> return plane_ctl;
>> }
>>
>> +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;
>> +
>> + if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709) {
>> + I915_WRITE(PLANE_INPUT_CSC_RY_GY(pipe, plane),
>> + CSC_BT709_YUV_TO_RGB_RY_GY);
>> + I915_WRITE(PLANE_INPUT_CSC_BY(pipe, plane),
>> + CSC_BT709_YUV_TO_RGB_BY);
>> + I915_WRITE(PLANE_INPUT_CSC_RU_GU(pipe, plane),
>> + CSC_BT709_YUV_TO_RGB_RU_GU);
>> + I915_WRITE(PLANE_INPUT_CSC_BU(pipe, plane),
>> + CSC_BT709_YUV_TO_RGB_BU);
>> + I915_WRITE(PLANE_INPUT_CSC_RV_GV(pipe, plane),
>> + CSC_BT709_YUV_TO_RGB_RV_GV);
>> + I915_WRITE(PLANE_INPUT_CSC_BV(pipe, plane),
>> + CSC_BT709_YUV_TO_RGB_BV);
>> + } else {
>> + I915_WRITE(PLANE_INPUT_CSC_RY_GY(pipe, plane),
>> + CSC_BT601_YUV_TO_RGB_RY_GY);
>> + I915_WRITE(PLANE_INPUT_CSC_BY(pipe, plane),
>> + CSC_BT601_YUV_TO_RGB_BY);
>> + I915_WRITE(PLANE_INPUT_CSC_RU_GU(pipe, plane),
>> + CSC_BT601_YUV_TO_RGB_RU_GU);
>> + I915_WRITE(PLANE_INPUT_CSC_BU(pipe, plane),
>> + CSC_BT601_YUV_TO_RGB_BU);
>> + I915_WRITE(PLANE_INPUT_CSC_RV_GV(pipe, plane),
>> + CSC_BT601_YUV_TO_RGB_RV_GV);
>> + I915_WRITE(PLANE_INPUT_CSC_BV(pipe, plane),
>> + CSC_BT601_YUV_TO_RGB_BV);
>> + }
>Considering there's going to be BT.601, BT.709 and perhaps newer colorspaces
>with limited vs full range, would it make more sense to generate the values?
These are all floating point coefficient values and driver has restrictions on the
floating math, so would be tough to generate them. Currently only BT709
and BT601 was supported. Later BT2020 co-eficients can be added. Other way can be
to expose this as a property and get these values from userspace (I would not want to
go that path as it will be an extra ABI burden and will require a userspace).
Personally I feel, defining 6 macro value for a new colorspace would be an easier option.
>> +
>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_HI(pipe, plane),
>> + PREOFF_YUV_TO_RGB_HI);
>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_ME(pipe, plane),
>> + PREOFF_YUV_TO_RGB_ME);
>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_LO(pipe, plane),
>> + PREOFF_YUV_TO_RGB_LO);
>> +
>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_HI(pipe, plane), 0x0);
>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane), 0x0);
>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane), 0x0); }
>We already have some support code for CSC matrices in intel_color.c, so I think it
>makes sense to program this from there..
Since for the non HDR planes CSC bits are programmed here, it would be good if
all planes are handled at one place. I will move the function to intel_color.c to keep
all the color related functions in one file.
>> u32 glk_plane_color_ctl(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);
>> const struct drm_framebuffer *fb = plane_state->base.fb;
>> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
>> + enum plane_id plane_id = plane->id;
>> +
>> u32 plane_color_ctl = 0;
>>
>> if (INTEL_GEN(dev_priv) < 11) {
>> @@ -3705,13 +3759,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 (INTEL_GEN(dev_priv) < 11 || plane_id > 2) {
>We now have icl_is_hdr_plane(), it just landed :)
>> + 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;
>The coefficients are likely different for full vs limited range, and I don't see that
>handled at the moment. :(
I have supported the FULL Range YUV to RGB conversion in this patch. Will add
limited range along with BT2020 with a later patch. This should unblock all the
current YUV Full Range planar format implementation. Hope this is ok ?
Thanks for the review Maarten. Will send out the updated patch based on your
recommendations.
Regards,
Uma Shankar
>> + }
>> }
>>
>> return plane_color_ctl;
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB Conversion
2018-10-24 11:19 ` Shankar, Uma
@ 2018-10-24 12:07 ` Maarten Lankhorst
2018-10-24 13:12 ` Shankar, Uma
0 siblings, 1 reply; 12+ messages in thread
From: Maarten Lankhorst @ 2018-10-24 12:07 UTC (permalink / raw)
To: Shankar, Uma, intel-gfx@lists.freedesktop.org
Cc: Syrjala, Ville, Lankhorst, Maarten
Op 24-10-18 om 13:19 schreef Shankar, Uma:
>
>> -----Original Message-----
>> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
>> Sent: Wednesday, October 24, 2018 4:18 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] [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB
>> Conversion
>>
>> Op 23-10-18 om 22:11 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 plane input csc registers and 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.
>>>
>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_reg.h | 243
>> +++++++++++++++++++++++++++++++++++
>>> drivers/gpu/drm/i915/intel_display.c | 76 ++++++++++-
>>> 2 files changed, 313 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>>> b/drivers/gpu/drm/i915/i915_reg.h index 8bd61f9..f0f6f7c 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -6553,6 +6553,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) /* Pre-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)
>>> @@ -6569,6 +6570,248 @@ 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_3_A 0x703E0
>>> +
>>> +#define _PLANE_INPUT_CSC_RY_GY_1_B 0x711E0
>>> +#define _PLANE_INPUT_CSC_RY_GY_2_B 0x712E0
>>> +#define _PLANE_INPUT_CSC_RY_GY_3_B 0x713E0
>>> +
>>> +#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_RY_GY(pipe, plane) \
>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RY_GY_1(pipe), \
>>> + _PLANE_INPUT_CSC_RY_GY_2(pipe))
>>> +
>>> +#define _PLANE_INPUT_CSC_BY_1_A 0x701E4
>>> +#define _PLANE_INPUT_CSC_BY_2_A 0x702E4
>>> +#define _PLANE_INPUT_CSC_BY_3_A 0x703E4
>>> +
>>> +#define _PLANE_INPUT_CSC_BY_1_B 0x711E4
>>> +#define _PLANE_INPUT_CSC_BY_2_B 0x712E4
>>> +#define _PLANE_INPUT_CSC_BY_3_B 0x713E4
>>> +
>>> +#define _PLANE_INPUT_CSC_BY_1(pipe) \
>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BY_1_A, \
>>> + _PLANE_INPUT_CSC_BY_1_B)
>>> +#define _PLANE_INPUT_CSC_BY_2(pipe) \
>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BY_2_A, \
>>> + _PLANE_INPUT_CSC_BY_2_B)
>>> +#define PLANE_INPUT_CSC_BY(pipe, plane) \
>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BY_1(pipe), \
>>> + _PLANE_INPUT_CSC_BY_2(pipe))
>>> +
>>> +#define _PLANE_INPUT_CSC_RU_GU_1_A 0x701E8
>>> +#define _PLANE_INPUT_CSC_RU_GU_2_A 0x702E8
>>> +#define _PLANE_INPUT_CSC_RU_GU_3_A 0x703E8
>>> +
>>> +#define _PLANE_INPUT_CSC_RU_GU_1_B 0x711E8
>>> +#define _PLANE_INPUT_CSC_RU_GU_2_B 0x712E8
>>> +#define _PLANE_INPUT_CSC_RU_GU_3_B 0x713E8
>>> +
>>> +#define _PLANE_INPUT_CSC_RU_GU_1(pipe) \
>>> + _PIPE(pipe, _PLANE_INPUT_CSC_RU_GU_1_A, \
>>> + _PLANE_INPUT_CSC_RU_GU_1_B)
>>> +#define _PLANE_INPUT_CSC_RU_GU_2(pipe) \
>>> + _PIPE(pipe, _PLANE_INPUT_CSC_RU_GU_2_A, \
>>> + _PLANE_INPUT_CSC_RU_GU_2_B)
>>> +#define PLANE_INPUT_CSC_RU_GU(pipe, plane) \
>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RU_GU_1(pipe), \
>>> + _PLANE_INPUT_CSC_RU_GU_2(pipe))
>>> +
>>> +#define _PLANE_INPUT_CSC_BU_1_A 0x701EC
>>> +#define _PLANE_INPUT_CSC_BU_2_A 0x702EC
>>> +#define _PLANE_INPUT_CSC_BU_3_A 0x703EC
>>> +
>>> +#define _PLANE_INPUT_CSC_BU_1_B 0x711EC
>>> +#define _PLANE_INPUT_CSC_BU_2_B 0x712EC
>>> +#define _PLANE_INPUT_CSC_BU_3_B 0x713EC
>>> +
>>> +#define _PLANE_INPUT_CSC_BU_1(pipe) \
>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BU_1_A, \
>>> + _PLANE_INPUT_CSC_BU_1_B)
>>> +#define _PLANE_INPUT_CSC_BU_2(pipe) \
>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BU_2_A, \
>>> + _PLANE_INPUT_CSC_BU_2_B)
>>> +#define PLANE_INPUT_CSC_BU(pipe, plane) \
>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BU_1(pipe), \
>>> + _PLANE_INPUT_CSC_BU_2(pipe))
>>> +
>>> +#define _PLANE_INPUT_CSC_RV_GV_1_A 0x701F0
>>> +#define _PLANE_INPUT_CSC_RV_GV_2_A 0x702F0
>>> +#define _PLANE_INPUT_CSC_RV_GV_3_A 0x703F0
>>> +
>>> +#define _PLANE_INPUT_CSC_RV_GV_1_B 0x711F0
>>> +#define _PLANE_INPUT_CSC_RV_GV_2_B 0x712F0
>>> +#define _PLANE_INPUT_CSC_RV_GV_3_B 0x713F0
>>> +
>>> +#define _PLANE_INPUT_CSC_RV_GV_1(pipe) \
>>> + _PIPE(pipe, _PLANE_INPUT_CSC_RV_GV_1_A, \
>>> + _PLANE_INPUT_CSC_RV_GV_1_B)
>>> +#define _PLANE_INPUT_CSC_RV_GV_2(pipe) \
>>> + _PIPE(pipe, _PLANE_INPUT_CSC_RV_GV_2_A, \
>>> + _PLANE_INPUT_CSC_RV_GV_2_B)
>>> +#define PLANE_INPUT_CSC_RV_GV(pipe, plane) \
>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RV_GV_1(pipe), \
>>> + _PLANE_INPUT_CSC_RV_GV_2(pipe))
>>> +
>>> +#define _PLANE_INPUT_CSC_BV_1_A 0x701F4
>>> +#define _PLANE_INPUT_CSC_BV_2_A 0x702F4
>>> +#define _PLANE_INPUT_CSC_BV_3_A 0x703F4
>>> +
>>> +#define _PLANE_INPUT_CSC_BV_1_B 0x711F4
>>> +#define _PLANE_INPUT_CSC_BV_2_B 0x712F4
>>> +#define _PLANE_INPUT_CSC_BV_3_B 0x713F4
>>> +
>>> +#define _PLANE_INPUT_CSC_BV_1(pipe) \
>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BV_1_A, \
>>> + _PLANE_INPUT_CSC_BV_1_B)
>>> +#define _PLANE_INPUT_CSC_BV_2(pipe) \
>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BV_2_A, \
>>> + _PLANE_INPUT_CSC_BV_2_B)
>>> +#define PLANE_INPUT_CSC_BV(pipe, plane) \
>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BV_1(pipe), \
>>> + _PLANE_INPUT_CSC_BV_2(pipe))
>>> +
>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_1_A 0x701F8
>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_2_A 0x702F8
>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_3_A 0x703F8
>>> +
>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_1_B 0x711F8
>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_2_B 0x712F8
>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_3_B 0x713F8
>>> +
>>> +#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_HI(pipe, plane) \
>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_HI_1(pipe), \
>>> + _PLANE_INPUT_CSC_PREOFF_HI_2(pipe))
>>> +
>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1_A 0x701FC
>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2_A 0x702FC
>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_3_A 0x703FC
>>> +
>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1_B 0x711FC
>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2_B 0x712FC
>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_3_B 0x713FC
>>> +
>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1(pipe) \
>>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_ME_1_A, \
>>> + _PLANE_INPUT_CSC_PREOFF_ME_1_B)
>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2(pipe) \
>>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_ME_2_A, \
>>> + _PLANE_INPUT_CSC_PREOFF_ME_2_B)
>>> +#define PLANE_INPUT_CSC_PREOFF_ME(pipe, plane) \
>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_ME_1(pipe), \
>>> + _PLANE_INPUT_CSC_PREOFF_ME_2(pipe))
>>> +
>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1_A 0x70200
>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2_A 0x70300
>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_3_A 0x70400
>>> +
>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1_B 0x71200
>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2_B 0x71300
>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_3_B 0x71400
>>> +
>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1(pipe) \
>>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_LO_1_A, \
>>> + _PLANE_INPUT_CSC_PREOFF_LO_1_B)
>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2(pipe) \
>>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_LO_2_A, \
>>> + _PLANE_INPUT_CSC_PREOFF_LO_2_B)
>>> +#define PLANE_INPUT_CSC_PREOFF_LO(pipe, plane) \
>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_LO_1(pipe), \
>>> + _PLANE_INPUT_CSC_PREOFF_LO_2(pipe))
>>> +
>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_1_A 0x70204
>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_2_A 0x70304
>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_3_A 0x70404
>>> +
>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_1_B 0x71204
>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_2_B 0x71304
>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_3_B 0x71404
>>> +
>>> +#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_HI(pipe, plane) \
>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_HI_1(pipe), \
>>> + _PLANE_INPUT_CSC_POSTOFF_HI_2(pipe))
>>> +
>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1_A 0x70208
>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2_A 0x70308
>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_3_A 0x70408
>>> +
>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1_B 0x71208
>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2_B 0x71308
>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_3_B 0x71408
>>> +
>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1(pipe) \
>>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_ME_1_A, \
>>> + _PLANE_INPUT_CSC_POSTOFF_ME_1_B)
>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe) \
>>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_ME_2_A, \
>>> + _PLANE_INPUT_CSC_POSTOFF_ME_2_B)
>>> +#define PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane) \
>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_ME_1(pipe), \
>>> + _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe))
>>> +
>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1_A 0x7020C
>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2_A 0x7030C
>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_3_A 0x7040C
>>> +
>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1_B 0x7120C
>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2_B 0x7130C
>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_3_B 0x7140C
>>> +
>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1(pipe) \
>>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_LO_1_A, \
>>> + _PLANE_INPUT_CSC_POSTOFF_LO_1_B)
>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe) \
>>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_LO_2_A, \
>>> + _PLANE_INPUT_CSC_POSTOFF_LO_2_B)
>>> +#define PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane) \
>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_LO_1(pipe), \
>>> + _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe))
>> Would probably be best to keep those separate.
> You mean in a separate patch ? If yes, I can do that and segregate the register macro
> definitions in a separate patch.
>
>>> +/*
>>> + * These values are direct register values specified in the Bspec,
>>> + * for YUV->RGB Full Range conversion matrix (colorspace BT709) */
>>> +#define CSC_BT709_YUV_TO_RGB_RY_GY 0x7C987800
>>> +#define CSC_BT709_YUV_TO_RGB_BY 0x0
>>> +#define CSC_BT709_YUV_TO_RGB_RU_GU 0x9EF87800
>>> +#define CSC_BT709_YUV_TO_RGB_BU 0xABF8
>>> +#define CSC_BT709_YUV_TO_RGB_RV_GV 0x7800
>>> +#define CSC_BT709_YUV_TO_RGB_BV 0x7ED8
>>> +
>>> +/*
>>> + * These values are direct register values specified in the Bspec,
>>> + * for YUV->RGB Full Range conversion matrix (colorspace BT601) */
>>> +#define CSC_BT601_YUV_TO_RGB_RY_GY 0x7AF87800
>>> +#define CSC_BT601_YUV_TO_RGB_BY 0x0
>>> +#define CSC_BT601_YUV_TO_RGB_RU_GU 0x8B287800
>>> +#define CSC_BT601_YUV_TO_RGB_BU 0x9AC0
>>> +#define CSC_BT601_YUV_TO_RGB_RV_GV 0x7800
>>> +#define CSC_BT601_YUV_TO_RGB_BV 0x7DD8
>>> +
>>> +/* Preoffset values for YUV to RGB Conversion */
>>> +#define PREOFF_YUV_TO_RGB_HI 0x800
>>> +#define PREOFF_YUV_TO_RGB_ME 0xF00
>>> +#define PREOFF_YUV_TO_RGB_LO 0x800
>>>
>>> #define _PLANE_CTL_1_B 0x71180
>>> #define _PLANE_CTL_2_B 0x71280
>> Probably move those coefficients to intel_color.c ?
> Sure, I will do that.
>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>> b/drivers/gpu/drm/i915/intel_display.c
>>> index fc7e3b0..38b41ed 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -3689,12 +3689,66 @@ u32 skl_plane_ctl(const struct intel_crtc_state
>> *crtc_state,
>>> return plane_ctl;
>>> }
>>>
>>> +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;
>>> +
>>> + if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709) {
>>> + I915_WRITE(PLANE_INPUT_CSC_RY_GY(pipe, plane),
>>> + CSC_BT709_YUV_TO_RGB_RY_GY);
>>> + I915_WRITE(PLANE_INPUT_CSC_BY(pipe, plane),
>>> + CSC_BT709_YUV_TO_RGB_BY);
>>> + I915_WRITE(PLANE_INPUT_CSC_RU_GU(pipe, plane),
>>> + CSC_BT709_YUV_TO_RGB_RU_GU);
>>> + I915_WRITE(PLANE_INPUT_CSC_BU(pipe, plane),
>>> + CSC_BT709_YUV_TO_RGB_BU);
>>> + I915_WRITE(PLANE_INPUT_CSC_RV_GV(pipe, plane),
>>> + CSC_BT709_YUV_TO_RGB_RV_GV);
>>> + I915_WRITE(PLANE_INPUT_CSC_BV(pipe, plane),
>>> + CSC_BT709_YUV_TO_RGB_BV);
>>> + } else {
>>> + I915_WRITE(PLANE_INPUT_CSC_RY_GY(pipe, plane),
>>> + CSC_BT601_YUV_TO_RGB_RY_GY);
>>> + I915_WRITE(PLANE_INPUT_CSC_BY(pipe, plane),
>>> + CSC_BT601_YUV_TO_RGB_BY);
>>> + I915_WRITE(PLANE_INPUT_CSC_RU_GU(pipe, plane),
>>> + CSC_BT601_YUV_TO_RGB_RU_GU);
>>> + I915_WRITE(PLANE_INPUT_CSC_BU(pipe, plane),
>>> + CSC_BT601_YUV_TO_RGB_BU);
>>> + I915_WRITE(PLANE_INPUT_CSC_RV_GV(pipe, plane),
>>> + CSC_BT601_YUV_TO_RGB_RV_GV);
>>> + I915_WRITE(PLANE_INPUT_CSC_BV(pipe, plane),
>>> + CSC_BT601_YUV_TO_RGB_BV);
>>> + }
>> Considering there's going to be BT.601, BT.709 and perhaps newer colorspaces
>> with limited vs full range, would it make more sense to generate the values?
> These are all floating point coefficient values and driver has restrictions on the
> floating math, so would be tough to generate them. Currently only BT709
> and BT601 was supported. Later BT2020 co-eficients can be added. Other way can be
> to expose this as a property and get these values from userspace (I would not want to
> go that path as it will be an extra ABI burden and will require a userspace).
>
> Personally I feel, defining 6 macro value for a new colorspace would be an easier option.
>
>>> +
>>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_HI(pipe, plane),
>>> + PREOFF_YUV_TO_RGB_HI);
>>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_ME(pipe, plane),
>>> + PREOFF_YUV_TO_RGB_ME);
>>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_LO(pipe, plane),
>>> + PREOFF_YUV_TO_RGB_LO);
>>> +
>>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_HI(pipe, plane), 0x0);
>>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane), 0x0);
>>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane), 0x0); }
>> We already have some support code for CSC matrices in intel_color.c, so I think it
>> makes sense to program this from there..
> Since for the non HDR planes CSC bits are programmed here, it would be good if
> all planes are handled at one place. I will move the function to intel_color.c to keep
> all the color related functions in one file.
>
>>> u32 glk_plane_color_ctl(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);
>>> const struct drm_framebuffer *fb = plane_state->base.fb;
>>> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
>>> + enum plane_id plane_id = plane->id;
>>> +
>>> u32 plane_color_ctl = 0;
>>>
>>> if (INTEL_GEN(dev_priv) < 11) {
>>> @@ -3705,13 +3759,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 (INTEL_GEN(dev_priv) < 11 || plane_id > 2) {
>> We now have icl_is_hdr_plane(), it just landed :)
>>> + 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;
>> The coefficients are likely different for full vs limited range, and I don't see that
>> handled at the moment. :(
> I have supported the FULL Range YUV to RGB conversion in this patch. Will add
> limited range along with BT2020 with a later patch. This should unblock all the
> current YUV Full Range planar format implementation. Hope this is ok ?
>
> Thanks for the review Maarten. Will send out the updated patch based on your
> recommendations.
This would work for NV12, but what about P01X or the Y21X formats? Do they need slightly different coefficients for each?
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB Conversion
2018-10-24 12:07 ` Maarten Lankhorst
@ 2018-10-24 13:12 ` Shankar, Uma
2018-10-24 13:42 ` Maarten Lankhorst
0 siblings, 1 reply; 12+ messages in thread
From: Shankar, Uma @ 2018-10-24 13:12 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: Wednesday, October 24, 2018 5:37 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] [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB
>Conversion
>
>Op 24-10-18 om 13:19 schreef Shankar, Uma:
>>
>>> -----Original Message-----
>>> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
>>> Sent: Wednesday, October 24, 2018 4:18 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] [PATCH] drm/i915: Enable Plane Input CSC for
>>> YUV to RGB Conversion
>>>
>>> Op 23-10-18 om 22:11 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 plane input csc registers and 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.
>>>>
>>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>>>> ---
>>>> drivers/gpu/drm/i915/i915_reg.h | 243
>>> +++++++++++++++++++++++++++++++++++
>>>> drivers/gpu/drm/i915/intel_display.c | 76 ++++++++++-
>>>> 2 files changed, 313 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>>>> b/drivers/gpu/drm/i915/i915_reg.h index 8bd61f9..f0f6f7c 100644
>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>> @@ -6553,6 +6553,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) /*
>Pre-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)
>>>> @@ -6569,6 +6570,248 @@ 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_3_A 0x703E0
>>>> +
>>>> +#define _PLANE_INPUT_CSC_RY_GY_1_B 0x711E0
>>>> +#define _PLANE_INPUT_CSC_RY_GY_2_B 0x712E0
>>>> +#define _PLANE_INPUT_CSC_RY_GY_3_B 0x713E0
>>>> +
>>>> +#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_RY_GY(pipe, plane) \
>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RY_GY_1(pipe), \
>>>> + _PLANE_INPUT_CSC_RY_GY_2(pipe))
>>>> +
>>>> +#define _PLANE_INPUT_CSC_BY_1_A 0x701E4
>>>> +#define _PLANE_INPUT_CSC_BY_2_A 0x702E4
>>>> +#define _PLANE_INPUT_CSC_BY_3_A 0x703E4
>>>> +
>>>> +#define _PLANE_INPUT_CSC_BY_1_B 0x711E4
>>>> +#define _PLANE_INPUT_CSC_BY_2_B 0x712E4
>>>> +#define _PLANE_INPUT_CSC_BY_3_B 0x713E4
>>>> +
>>>> +#define _PLANE_INPUT_CSC_BY_1(pipe) \
>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BY_1_A, \
>>>> + _PLANE_INPUT_CSC_BY_1_B)
>>>> +#define _PLANE_INPUT_CSC_BY_2(pipe) \
>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BY_2_A, \
>>>> + _PLANE_INPUT_CSC_BY_2_B)
>>>> +#define PLANE_INPUT_CSC_BY(pipe, plane) \
>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BY_1(pipe), \
>>>> + _PLANE_INPUT_CSC_BY_2(pipe))
>>>> +
>>>> +#define _PLANE_INPUT_CSC_RU_GU_1_A 0x701E8
>>>> +#define _PLANE_INPUT_CSC_RU_GU_2_A 0x702E8
>>>> +#define _PLANE_INPUT_CSC_RU_GU_3_A 0x703E8
>>>> +
>>>> +#define _PLANE_INPUT_CSC_RU_GU_1_B 0x711E8
>>>> +#define _PLANE_INPUT_CSC_RU_GU_2_B 0x712E8
>>>> +#define _PLANE_INPUT_CSC_RU_GU_3_B 0x713E8
>>>> +
>>>> +#define _PLANE_INPUT_CSC_RU_GU_1(pipe) \
>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_RU_GU_1_A, \
>>>> + _PLANE_INPUT_CSC_RU_GU_1_B)
>>>> +#define _PLANE_INPUT_CSC_RU_GU_2(pipe) \
>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_RU_GU_2_A, \
>>>> + _PLANE_INPUT_CSC_RU_GU_2_B)
>>>> +#define PLANE_INPUT_CSC_RU_GU(pipe, plane) \
>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RU_GU_1(pipe), \
>>>> + _PLANE_INPUT_CSC_RU_GU_2(pipe))
>>>> +
>>>> +#define _PLANE_INPUT_CSC_BU_1_A 0x701EC
>>>> +#define _PLANE_INPUT_CSC_BU_2_A 0x702EC
>>>> +#define _PLANE_INPUT_CSC_BU_3_A 0x703EC
>>>> +
>>>> +#define _PLANE_INPUT_CSC_BU_1_B 0x711EC
>>>> +#define _PLANE_INPUT_CSC_BU_2_B 0x712EC
>>>> +#define _PLANE_INPUT_CSC_BU_3_B 0x713EC
>>>> +
>>>> +#define _PLANE_INPUT_CSC_BU_1(pipe) \
>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BU_1_A, \
>>>> + _PLANE_INPUT_CSC_BU_1_B)
>>>> +#define _PLANE_INPUT_CSC_BU_2(pipe) \
>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BU_2_A, \
>>>> + _PLANE_INPUT_CSC_BU_2_B)
>>>> +#define PLANE_INPUT_CSC_BU(pipe, plane) \
>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BU_1(pipe), \
>>>> + _PLANE_INPUT_CSC_BU_2(pipe))
>>>> +
>>>> +#define _PLANE_INPUT_CSC_RV_GV_1_A 0x701F0
>>>> +#define _PLANE_INPUT_CSC_RV_GV_2_A 0x702F0
>>>> +#define _PLANE_INPUT_CSC_RV_GV_3_A 0x703F0
>>>> +
>>>> +#define _PLANE_INPUT_CSC_RV_GV_1_B 0x711F0
>>>> +#define _PLANE_INPUT_CSC_RV_GV_2_B 0x712F0
>>>> +#define _PLANE_INPUT_CSC_RV_GV_3_B 0x713F0
>>>> +
>>>> +#define _PLANE_INPUT_CSC_RV_GV_1(pipe) \
>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_RV_GV_1_A, \
>>>> + _PLANE_INPUT_CSC_RV_GV_1_B)
>>>> +#define _PLANE_INPUT_CSC_RV_GV_2(pipe) \
>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_RV_GV_2_A, \
>>>> + _PLANE_INPUT_CSC_RV_GV_2_B)
>>>> +#define PLANE_INPUT_CSC_RV_GV(pipe, plane) \
>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RV_GV_1(pipe), \
>>>> + _PLANE_INPUT_CSC_RV_GV_2(pipe))
>>>> +
>>>> +#define _PLANE_INPUT_CSC_BV_1_A 0x701F4
>>>> +#define _PLANE_INPUT_CSC_BV_2_A 0x702F4
>>>> +#define _PLANE_INPUT_CSC_BV_3_A 0x703F4
>>>> +
>>>> +#define _PLANE_INPUT_CSC_BV_1_B 0x711F4
>>>> +#define _PLANE_INPUT_CSC_BV_2_B 0x712F4
>>>> +#define _PLANE_INPUT_CSC_BV_3_B 0x713F4
>>>> +
>>>> +#define _PLANE_INPUT_CSC_BV_1(pipe) \
>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BV_1_A, \
>>>> + _PLANE_INPUT_CSC_BV_1_B)
>>>> +#define _PLANE_INPUT_CSC_BV_2(pipe) \
>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BV_2_A, \
>>>> + _PLANE_INPUT_CSC_BV_2_B)
>>>> +#define PLANE_INPUT_CSC_BV(pipe, plane) \
>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BV_1(pipe), \
>>>> + _PLANE_INPUT_CSC_BV_2(pipe))
>>>> +
>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_1_A 0x701F8
>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_2_A 0x702F8
>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_3_A 0x703F8
>>>> +
>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_1_B 0x711F8
>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_2_B 0x712F8
>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_3_B 0x713F8
>>>> +
>>>> +#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_HI(pipe, plane) \
>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_HI_1(pipe), \
>>>> + _PLANE_INPUT_CSC_PREOFF_HI_2(pipe))
>>>> +
>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1_A 0x701FC
>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2_A 0x702FC
>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_3_A 0x703FC
>>>> +
>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1_B 0x711FC
>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2_B 0x712FC
>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_3_B 0x713FC
>>>> +
>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1(pipe) \
>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_ME_1_A, \
>>>> + _PLANE_INPUT_CSC_PREOFF_ME_1_B)
>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2(pipe) \
>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_ME_2_A, \
>>>> + _PLANE_INPUT_CSC_PREOFF_ME_2_B)
>>>> +#define PLANE_INPUT_CSC_PREOFF_ME(pipe, plane) \
>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_ME_1(pipe), \
>>>> + _PLANE_INPUT_CSC_PREOFF_ME_2(pipe))
>>>> +
>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1_A 0x70200
>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2_A 0x70300
>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_3_A 0x70400
>>>> +
>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1_B 0x71200
>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2_B 0x71300
>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_3_B 0x71400
>>>> +
>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1(pipe) \
>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_LO_1_A, \
>>>> + _PLANE_INPUT_CSC_PREOFF_LO_1_B)
>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2(pipe) \
>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_LO_2_A, \
>>>> + _PLANE_INPUT_CSC_PREOFF_LO_2_B)
>>>> +#define PLANE_INPUT_CSC_PREOFF_LO(pipe, plane) \
>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_LO_1(pipe), \
>>>> + _PLANE_INPUT_CSC_PREOFF_LO_2(pipe))
>>>> +
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_1_A 0x70204
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_2_A 0x70304
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_3_A 0x70404
>>>> +
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_1_B 0x71204
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_2_B 0x71304
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_3_B 0x71404
>>>> +
>>>> +#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_HI(pipe, plane) \
>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_HI_1(pipe), \
>>>> + _PLANE_INPUT_CSC_POSTOFF_HI_2(pipe))
>>>> +
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1_A 0x70208
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2_A 0x70308
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_3_A 0x70408
>>>> +
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1_B 0x71208
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2_B 0x71308
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_3_B 0x71408
>>>> +
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1(pipe) \
>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_ME_1_A, \
>>>> + _PLANE_INPUT_CSC_POSTOFF_ME_1_B)
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe) \
>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_ME_2_A, \
>>>> + _PLANE_INPUT_CSC_POSTOFF_ME_2_B)
>>>> +#define PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane) \
>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_ME_1(pipe), \
>>>> + _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe))
>>>> +
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1_A 0x7020C
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2_A 0x7030C
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_3_A 0x7040C
>>>> +
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1_B 0x7120C
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2_B 0x7130C
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_3_B 0x7140C
>>>> +
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1(pipe) \
>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_LO_1_A, \
>>>> + _PLANE_INPUT_CSC_POSTOFF_LO_1_B)
>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe) \
>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_LO_2_A, \
>>>> + _PLANE_INPUT_CSC_POSTOFF_LO_2_B)
>>>> +#define PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane) \
>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_LO_1(pipe), \
>>>> + _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe))
>>> Would probably be best to keep those separate.
>> You mean in a separate patch ? If yes, I can do that and segregate the
>> register macro definitions in a separate patch.
>>
>>>> +/*
>>>> + * These values are direct register values specified in the Bspec,
>>>> + * for YUV->RGB Full Range conversion matrix (colorspace BT709) */
>>>> +#define CSC_BT709_YUV_TO_RGB_RY_GY 0x7C987800
>>>> +#define CSC_BT709_YUV_TO_RGB_BY 0x0
>>>> +#define CSC_BT709_YUV_TO_RGB_RU_GU 0x9EF87800
>>>> +#define CSC_BT709_YUV_TO_RGB_BU 0xABF8
>>>> +#define CSC_BT709_YUV_TO_RGB_RV_GV 0x7800
>>>> +#define CSC_BT709_YUV_TO_RGB_BV 0x7ED8
>>>> +
>>>> +/*
>>>> + * These values are direct register values specified in the Bspec,
>>>> + * for YUV->RGB Full Range conversion matrix (colorspace BT601) */
>>>> +#define CSC_BT601_YUV_TO_RGB_RY_GY 0x7AF87800
>>>> +#define CSC_BT601_YUV_TO_RGB_BY 0x0
>>>> +#define CSC_BT601_YUV_TO_RGB_RU_GU 0x8B287800
>>>> +#define CSC_BT601_YUV_TO_RGB_BU 0x9AC0
>>>> +#define CSC_BT601_YUV_TO_RGB_RV_GV 0x7800
>>>> +#define CSC_BT601_YUV_TO_RGB_BV 0x7DD8
>>>> +
>>>> +/* Preoffset values for YUV to RGB Conversion */
>>>> +#define PREOFF_YUV_TO_RGB_HI 0x800
>>>> +#define PREOFF_YUV_TO_RGB_ME 0xF00
>>>> +#define PREOFF_YUV_TO_RGB_LO 0x800
>>>>
>>>> #define _PLANE_CTL_1_B 0x71180
>>>> #define _PLANE_CTL_2_B 0x71280
>>> Probably move those coefficients to intel_color.c ?
>> Sure, I will do that.
>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>>> b/drivers/gpu/drm/i915/intel_display.c
>>>> index fc7e3b0..38b41ed 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -3689,12 +3689,66 @@ u32 skl_plane_ctl(const struct
>>>> intel_crtc_state
>>> *crtc_state,
>>>> return plane_ctl;
>>>> }
>>>>
>>>> +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;
>>>> +
>>>> + if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709) {
>>>> + I915_WRITE(PLANE_INPUT_CSC_RY_GY(pipe, plane),
>>>> + CSC_BT709_YUV_TO_RGB_RY_GY);
>>>> + I915_WRITE(PLANE_INPUT_CSC_BY(pipe, plane),
>>>> + CSC_BT709_YUV_TO_RGB_BY);
>>>> + I915_WRITE(PLANE_INPUT_CSC_RU_GU(pipe, plane),
>>>> + CSC_BT709_YUV_TO_RGB_RU_GU);
>>>> + I915_WRITE(PLANE_INPUT_CSC_BU(pipe, plane),
>>>> + CSC_BT709_YUV_TO_RGB_BU);
>>>> + I915_WRITE(PLANE_INPUT_CSC_RV_GV(pipe, plane),
>>>> + CSC_BT709_YUV_TO_RGB_RV_GV);
>>>> + I915_WRITE(PLANE_INPUT_CSC_BV(pipe, plane),
>>>> + CSC_BT709_YUV_TO_RGB_BV);
>>>> + } else {
>>>> + I915_WRITE(PLANE_INPUT_CSC_RY_GY(pipe, plane),
>>>> + CSC_BT601_YUV_TO_RGB_RY_GY);
>>>> + I915_WRITE(PLANE_INPUT_CSC_BY(pipe, plane),
>>>> + CSC_BT601_YUV_TO_RGB_BY);
>>>> + I915_WRITE(PLANE_INPUT_CSC_RU_GU(pipe, plane),
>>>> + CSC_BT601_YUV_TO_RGB_RU_GU);
>>>> + I915_WRITE(PLANE_INPUT_CSC_BU(pipe, plane),
>>>> + CSC_BT601_YUV_TO_RGB_BU);
>>>> + I915_WRITE(PLANE_INPUT_CSC_RV_GV(pipe, plane),
>>>> + CSC_BT601_YUV_TO_RGB_RV_GV);
>>>> + I915_WRITE(PLANE_INPUT_CSC_BV(pipe, plane),
>>>> + CSC_BT601_YUV_TO_RGB_BV);
>>>> + }
>>> Considering there's going to be BT.601, BT.709 and perhaps newer
>>> colorspaces with limited vs full range, would it make more sense to generate
>the values?
>> These are all floating point coefficient values and driver has
>> restrictions on the floating math, so would be tough to generate them.
>> Currently only BT709 and BT601 was supported. Later BT2020
>> co-eficients can be added. Other way can be to expose this as a
>> property and get these values from userspace (I would not want to go that path
>as it will be an extra ABI burden and will require a userspace).
>>
>> Personally I feel, defining 6 macro value for a new colorspace would be an
>easier option.
>>
>>>> +
>>>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_HI(pipe, plane),
>>>> + PREOFF_YUV_TO_RGB_HI);
>>>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_ME(pipe, plane),
>>>> + PREOFF_YUV_TO_RGB_ME);
>>>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_LO(pipe, plane),
>>>> + PREOFF_YUV_TO_RGB_LO);
>>>> +
>>>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_HI(pipe, plane), 0x0);
>>>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane), 0x0);
>>>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane), 0x0); }
>>> We already have some support code for CSC matrices in intel_color.c,
>>> so I think it makes sense to program this from there..
>> Since for the non HDR planes CSC bits are programmed here, it would be
>> good if all planes are handled at one place. I will move the function
>> to intel_color.c to keep all the color related functions in one file.
>>
>>>> u32 glk_plane_color_ctl(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);
>>>> const struct drm_framebuffer *fb = plane_state->base.fb;
>>>> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
>>>> + enum plane_id plane_id = plane->id;
>>>> +
>>>> u32 plane_color_ctl = 0;
>>>>
>>>> if (INTEL_GEN(dev_priv) < 11) {
>>>> @@ -3705,13 +3759,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 (INTEL_GEN(dev_priv) < 11 || plane_id > 2) {
>>> We now have icl_is_hdr_plane(), it just landed :)
>>>> + 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;
>>> The coefficients are likely different for full vs limited range, and
>>> I don't see that handled at the moment. :(
>> I have supported the FULL Range YUV to RGB conversion in this patch.
>> Will add limited range along with BT2020 with a later patch. This
>> should unblock all the current YUV Full Range planar format implementation.
>Hope this is ok ?
>>
>> Thanks for the review Maarten. Will send out the updated patch based
>> on your recommendations.
>This would work for NV12, but what about P01X or the Y21X formats? Do they
>need slightly different coefficients for each?
All the planar formats will be up sampled before conversion. So standard YUV to
RGB conversion matrix should work even for P01X and Y21X formats.
>~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB Conversion
2018-10-24 13:12 ` Shankar, Uma
@ 2018-10-24 13:42 ` Maarten Lankhorst
2018-10-24 14:40 ` Ville Syrjälä
0 siblings, 1 reply; 12+ messages in thread
From: Maarten Lankhorst @ 2018-10-24 13:42 UTC (permalink / raw)
To: Shankar, Uma, intel-gfx@lists.freedesktop.org
Cc: Syrjala, Ville, Lankhorst, Maarten
Op 24-10-18 om 15:12 schreef Shankar, Uma:
>
>> -----Original Message-----
>> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
>> Sent: Wednesday, October 24, 2018 5:37 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] [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB
>> Conversion
>>
>> Op 24-10-18 om 13:19 schreef Shankar, Uma:
>>>> -----Original Message-----
>>>> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
>>>> Sent: Wednesday, October 24, 2018 4:18 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] [PATCH] drm/i915: Enable Plane Input CSC for
>>>> YUV to RGB Conversion
>>>>
>>>> Op 23-10-18 om 22:11 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 plane input csc registers and 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.
>>>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>>>>> ---
>>>>> drivers/gpu/drm/i915/i915_reg.h | 243
>>>> +++++++++++++++++++++++++++++++++++
>>>>> drivers/gpu/drm/i915/intel_display.c | 76 ++++++++++-
>>>>> 2 files changed, 313 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>>>>> b/drivers/gpu/drm/i915/i915_reg.h index 8bd61f9..f0f6f7c 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>>> @@ -6553,6 +6553,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) /*
>> Pre-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)
>>>>> @@ -6569,6 +6570,248 @@ 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_3_A 0x703E0
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_RY_GY_1_B 0x711E0
>>>>> +#define _PLANE_INPUT_CSC_RY_GY_2_B 0x712E0
>>>>> +#define _PLANE_INPUT_CSC_RY_GY_3_B 0x713E0
>>>>> +
>>>>> +#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_RY_GY(pipe, plane) \
>>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RY_GY_1(pipe), \
>>>>> + _PLANE_INPUT_CSC_RY_GY_2(pipe))
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_BY_1_A 0x701E4
>>>>> +#define _PLANE_INPUT_CSC_BY_2_A 0x702E4
>>>>> +#define _PLANE_INPUT_CSC_BY_3_A 0x703E4
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_BY_1_B 0x711E4
>>>>> +#define _PLANE_INPUT_CSC_BY_2_B 0x712E4
>>>>> +#define _PLANE_INPUT_CSC_BY_3_B 0x713E4
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_BY_1(pipe) \
>>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BY_1_A, \
>>>>> + _PLANE_INPUT_CSC_BY_1_B)
>>>>> +#define _PLANE_INPUT_CSC_BY_2(pipe) \
>>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BY_2_A, \
>>>>> + _PLANE_INPUT_CSC_BY_2_B)
>>>>> +#define PLANE_INPUT_CSC_BY(pipe, plane) \
>>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BY_1(pipe), \
>>>>> + _PLANE_INPUT_CSC_BY_2(pipe))
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_RU_GU_1_A 0x701E8
>>>>> +#define _PLANE_INPUT_CSC_RU_GU_2_A 0x702E8
>>>>> +#define _PLANE_INPUT_CSC_RU_GU_3_A 0x703E8
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_RU_GU_1_B 0x711E8
>>>>> +#define _PLANE_INPUT_CSC_RU_GU_2_B 0x712E8
>>>>> +#define _PLANE_INPUT_CSC_RU_GU_3_B 0x713E8
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_RU_GU_1(pipe) \
>>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_RU_GU_1_A, \
>>>>> + _PLANE_INPUT_CSC_RU_GU_1_B)
>>>>> +#define _PLANE_INPUT_CSC_RU_GU_2(pipe) \
>>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_RU_GU_2_A, \
>>>>> + _PLANE_INPUT_CSC_RU_GU_2_B)
>>>>> +#define PLANE_INPUT_CSC_RU_GU(pipe, plane) \
>>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RU_GU_1(pipe), \
>>>>> + _PLANE_INPUT_CSC_RU_GU_2(pipe))
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_BU_1_A 0x701EC
>>>>> +#define _PLANE_INPUT_CSC_BU_2_A 0x702EC
>>>>> +#define _PLANE_INPUT_CSC_BU_3_A 0x703EC
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_BU_1_B 0x711EC
>>>>> +#define _PLANE_INPUT_CSC_BU_2_B 0x712EC
>>>>> +#define _PLANE_INPUT_CSC_BU_3_B 0x713EC
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_BU_1(pipe) \
>>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BU_1_A, \
>>>>> + _PLANE_INPUT_CSC_BU_1_B)
>>>>> +#define _PLANE_INPUT_CSC_BU_2(pipe) \
>>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BU_2_A, \
>>>>> + _PLANE_INPUT_CSC_BU_2_B)
>>>>> +#define PLANE_INPUT_CSC_BU(pipe, plane) \
>>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BU_1(pipe), \
>>>>> + _PLANE_INPUT_CSC_BU_2(pipe))
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_RV_GV_1_A 0x701F0
>>>>> +#define _PLANE_INPUT_CSC_RV_GV_2_A 0x702F0
>>>>> +#define _PLANE_INPUT_CSC_RV_GV_3_A 0x703F0
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_RV_GV_1_B 0x711F0
>>>>> +#define _PLANE_INPUT_CSC_RV_GV_2_B 0x712F0
>>>>> +#define _PLANE_INPUT_CSC_RV_GV_3_B 0x713F0
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_RV_GV_1(pipe) \
>>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_RV_GV_1_A, \
>>>>> + _PLANE_INPUT_CSC_RV_GV_1_B)
>>>>> +#define _PLANE_INPUT_CSC_RV_GV_2(pipe) \
>>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_RV_GV_2_A, \
>>>>> + _PLANE_INPUT_CSC_RV_GV_2_B)
>>>>> +#define PLANE_INPUT_CSC_RV_GV(pipe, plane) \
>>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RV_GV_1(pipe), \
>>>>> + _PLANE_INPUT_CSC_RV_GV_2(pipe))
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_BV_1_A 0x701F4
>>>>> +#define _PLANE_INPUT_CSC_BV_2_A 0x702F4
>>>>> +#define _PLANE_INPUT_CSC_BV_3_A 0x703F4
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_BV_1_B 0x711F4
>>>>> +#define _PLANE_INPUT_CSC_BV_2_B 0x712F4
>>>>> +#define _PLANE_INPUT_CSC_BV_3_B 0x713F4
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_BV_1(pipe) \
>>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BV_1_A, \
>>>>> + _PLANE_INPUT_CSC_BV_1_B)
>>>>> +#define _PLANE_INPUT_CSC_BV_2(pipe) \
>>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BV_2_A, \
>>>>> + _PLANE_INPUT_CSC_BV_2_B)
>>>>> +#define PLANE_INPUT_CSC_BV(pipe, plane) \
>>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BV_1(pipe), \
>>>>> + _PLANE_INPUT_CSC_BV_2(pipe))
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_1_A 0x701F8
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_2_A 0x702F8
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_3_A 0x703F8
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_1_B 0x711F8
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_2_B 0x712F8
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_3_B 0x713F8
>>>>> +
>>>>> +#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_HI(pipe, plane) \
>>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_HI_1(pipe), \
>>>>> + _PLANE_INPUT_CSC_PREOFF_HI_2(pipe))
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1_A 0x701FC
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2_A 0x702FC
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_3_A 0x703FC
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1_B 0x711FC
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2_B 0x712FC
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_3_B 0x713FC
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1(pipe) \
>>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_ME_1_A, \
>>>>> + _PLANE_INPUT_CSC_PREOFF_ME_1_B)
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2(pipe) \
>>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_ME_2_A, \
>>>>> + _PLANE_INPUT_CSC_PREOFF_ME_2_B)
>>>>> +#define PLANE_INPUT_CSC_PREOFF_ME(pipe, plane) \
>>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_ME_1(pipe), \
>>>>> + _PLANE_INPUT_CSC_PREOFF_ME_2(pipe))
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1_A 0x70200
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2_A 0x70300
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_3_A 0x70400
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1_B 0x71200
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2_B 0x71300
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_3_B 0x71400
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1(pipe) \
>>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_LO_1_A, \
>>>>> + _PLANE_INPUT_CSC_PREOFF_LO_1_B)
>>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2(pipe) \
>>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_LO_2_A, \
>>>>> + _PLANE_INPUT_CSC_PREOFF_LO_2_B)
>>>>> +#define PLANE_INPUT_CSC_PREOFF_LO(pipe, plane) \
>>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_LO_1(pipe), \
>>>>> + _PLANE_INPUT_CSC_PREOFF_LO_2(pipe))
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_1_A 0x70204
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_2_A 0x70304
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_3_A 0x70404
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_1_B 0x71204
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_2_B 0x71304
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_3_B 0x71404
>>>>> +
>>>>> +#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_HI(pipe, plane) \
>>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_HI_1(pipe), \
>>>>> + _PLANE_INPUT_CSC_POSTOFF_HI_2(pipe))
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1_A 0x70208
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2_A 0x70308
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_3_A 0x70408
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1_B 0x71208
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2_B 0x71308
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_3_B 0x71408
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1(pipe) \
>>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_ME_1_A, \
>>>>> + _PLANE_INPUT_CSC_POSTOFF_ME_1_B)
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe) \
>>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_ME_2_A, \
>>>>> + _PLANE_INPUT_CSC_POSTOFF_ME_2_B)
>>>>> +#define PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane) \
>>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_ME_1(pipe), \
>>>>> + _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe))
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1_A 0x7020C
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2_A 0x7030C
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_3_A 0x7040C
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1_B 0x7120C
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2_B 0x7130C
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_3_B 0x7140C
>>>>> +
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1(pipe) \
>>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_LO_1_A, \
>>>>> + _PLANE_INPUT_CSC_POSTOFF_LO_1_B)
>>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe) \
>>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_LO_2_A, \
>>>>> + _PLANE_INPUT_CSC_POSTOFF_LO_2_B)
>>>>> +#define PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane) \
>>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_LO_1(pipe), \
>>>>> + _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe))
>>>> Would probably be best to keep those separate.
>>> You mean in a separate patch ? If yes, I can do that and segregate the
>>> register macro definitions in a separate patch.
>>>
>>>>> +/*
>>>>> + * These values are direct register values specified in the Bspec,
>>>>> + * for YUV->RGB Full Range conversion matrix (colorspace BT709) */
>>>>> +#define CSC_BT709_YUV_TO_RGB_RY_GY 0x7C987800
>>>>> +#define CSC_BT709_YUV_TO_RGB_BY 0x0
>>>>> +#define CSC_BT709_YUV_TO_RGB_RU_GU 0x9EF87800
>>>>> +#define CSC_BT709_YUV_TO_RGB_BU 0xABF8
>>>>> +#define CSC_BT709_YUV_TO_RGB_RV_GV 0x7800
>>>>> +#define CSC_BT709_YUV_TO_RGB_BV 0x7ED8
>>>>> +
>>>>> +/*
>>>>> + * These values are direct register values specified in the Bspec,
>>>>> + * for YUV->RGB Full Range conversion matrix (colorspace BT601) */
>>>>> +#define CSC_BT601_YUV_TO_RGB_RY_GY 0x7AF87800
>>>>> +#define CSC_BT601_YUV_TO_RGB_BY 0x0
>>>>> +#define CSC_BT601_YUV_TO_RGB_RU_GU 0x8B287800
>>>>> +#define CSC_BT601_YUV_TO_RGB_BU 0x9AC0
>>>>> +#define CSC_BT601_YUV_TO_RGB_RV_GV 0x7800
>>>>> +#define CSC_BT601_YUV_TO_RGB_BV 0x7DD8
>>>>> +
>>>>> +/* Preoffset values for YUV to RGB Conversion */
>>>>> +#define PREOFF_YUV_TO_RGB_HI 0x800
>>>>> +#define PREOFF_YUV_TO_RGB_ME 0xF00
>>>>> +#define PREOFF_YUV_TO_RGB_LO 0x800
>>>>>
>>>>> #define _PLANE_CTL_1_B 0x71180
>>>>> #define _PLANE_CTL_2_B 0x71280
>>>> Probably move those coefficients to intel_color.c ?
>>> Sure, I will do that.
>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>>>> b/drivers/gpu/drm/i915/intel_display.c
>>>>> index fc7e3b0..38b41ed 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>> @@ -3689,12 +3689,66 @@ u32 skl_plane_ctl(const struct
>>>>> intel_crtc_state
>>>> *crtc_state,
>>>>> return plane_ctl;
>>>>> }
>>>>>
>>>>> +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;
>>>>> +
>>>>> + if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709) {
>>>>> + I915_WRITE(PLANE_INPUT_CSC_RY_GY(pipe, plane),
>>>>> + CSC_BT709_YUV_TO_RGB_RY_GY);
>>>>> + I915_WRITE(PLANE_INPUT_CSC_BY(pipe, plane),
>>>>> + CSC_BT709_YUV_TO_RGB_BY);
>>>>> + I915_WRITE(PLANE_INPUT_CSC_RU_GU(pipe, plane),
>>>>> + CSC_BT709_YUV_TO_RGB_RU_GU);
>>>>> + I915_WRITE(PLANE_INPUT_CSC_BU(pipe, plane),
>>>>> + CSC_BT709_YUV_TO_RGB_BU);
>>>>> + I915_WRITE(PLANE_INPUT_CSC_RV_GV(pipe, plane),
>>>>> + CSC_BT709_YUV_TO_RGB_RV_GV);
>>>>> + I915_WRITE(PLANE_INPUT_CSC_BV(pipe, plane),
>>>>> + CSC_BT709_YUV_TO_RGB_BV);
>>>>> + } else {
>>>>> + I915_WRITE(PLANE_INPUT_CSC_RY_GY(pipe, plane),
>>>>> + CSC_BT601_YUV_TO_RGB_RY_GY);
>>>>> + I915_WRITE(PLANE_INPUT_CSC_BY(pipe, plane),
>>>>> + CSC_BT601_YUV_TO_RGB_BY);
>>>>> + I915_WRITE(PLANE_INPUT_CSC_RU_GU(pipe, plane),
>>>>> + CSC_BT601_YUV_TO_RGB_RU_GU);
>>>>> + I915_WRITE(PLANE_INPUT_CSC_BU(pipe, plane),
>>>>> + CSC_BT601_YUV_TO_RGB_BU);
>>>>> + I915_WRITE(PLANE_INPUT_CSC_RV_GV(pipe, plane),
>>>>> + CSC_BT601_YUV_TO_RGB_RV_GV);
>>>>> + I915_WRITE(PLANE_INPUT_CSC_BV(pipe, plane),
>>>>> + CSC_BT601_YUV_TO_RGB_BV);
>>>>> + }
>>>> Considering there's going to be BT.601, BT.709 and perhaps newer
>>>> colorspaces with limited vs full range, would it make more sense to generate
>> the values?
>>> These are all floating point coefficient values and driver has
>>> restrictions on the floating math, so would be tough to generate them.
>>> Currently only BT709 and BT601 was supported. Later BT2020
>>> co-eficients can be added. Other way can be to expose this as a
>>> property and get these values from userspace (I would not want to go that path
>> as it will be an extra ABI burden and will require a userspace).
>>> Personally I feel, defining 6 macro value for a new colorspace would be an
>> easier option.
>>>>> +
>>>>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_HI(pipe, plane),
>>>>> + PREOFF_YUV_TO_RGB_HI);
>>>>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_ME(pipe, plane),
>>>>> + PREOFF_YUV_TO_RGB_ME);
>>>>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_LO(pipe, plane),
>>>>> + PREOFF_YUV_TO_RGB_LO);
>>>>> +
>>>>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_HI(pipe, plane), 0x0);
>>>>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane), 0x0);
>>>>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane), 0x0); }
>>>> We already have some support code for CSC matrices in intel_color.c,
>>>> so I think it makes sense to program this from there..
>>> Since for the non HDR planes CSC bits are programmed here, it would be
>>> good if all planes are handled at one place. I will move the function
>>> to intel_color.c to keep all the color related functions in one file.
>>>
>>>>> u32 glk_plane_color_ctl(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);
>>>>> const struct drm_framebuffer *fb = plane_state->base.fb;
>>>>> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
>>>>> + enum plane_id plane_id = plane->id;
>>>>> +
>>>>> u32 plane_color_ctl = 0;
>>>>>
>>>>> if (INTEL_GEN(dev_priv) < 11) {
>>>>> @@ -3705,13 +3759,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 (INTEL_GEN(dev_priv) < 11 || plane_id > 2) {
>>>> We now have icl_is_hdr_plane(), it just landed :)
>>>>> + 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;
>>>> The coefficients are likely different for full vs limited range, and
>>>> I don't see that handled at the moment. :(
>>> I have supported the FULL Range YUV to RGB conversion in this patch.
>>> Will add limited range along with BT2020 with a later patch. This
>>> should unblock all the current YUV Full Range planar format implementation.
>> Hope this is ok ?
>>> Thanks for the review Maarten. Will send out the updated patch based
>>> on your recommendations.
>> This would work for NV12, but what about P01X or the Y21X formats? Do they
>> need slightly different coefficients for each?
> All the planar formats will be up sampled before conversion. So standard YUV to
> RGB conversion matrix should work even for P01X and Y21X formats.
Not opposed to hardcoding then, but would be nice to have those values in nice static const u16[3][3] array instead of a bunch of defines..
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB Conversion
2018-10-24 13:42 ` Maarten Lankhorst
@ 2018-10-24 14:40 ` Ville Syrjälä
2018-10-24 15:55 ` Shankar, Uma
0 siblings, 1 reply; 12+ messages in thread
From: Ville Syrjälä @ 2018-10-24 14:40 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: intel-gfx@lists.freedesktop.org, Syrjala, Ville,
Lankhorst, Maarten
On Wed, Oct 24, 2018 at 03:42:34PM +0200, Maarten Lankhorst wrote:
> Op 24-10-18 om 15:12 schreef Shankar, Uma:
> >
> >> -----Original Message-----
> >> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> >> Sent: Wednesday, October 24, 2018 5:37 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] [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB
> >> Conversion
> >>
> >> Op 24-10-18 om 13:19 schreef Shankar, Uma:
> >>>> -----Original Message-----
> >>>> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> >>>> Sent: Wednesday, October 24, 2018 4:18 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] [PATCH] drm/i915: Enable Plane Input CSC for
> >>>> YUV to RGB Conversion
> >>>>
> >>>> Op 23-10-18 om 22:11 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 plane input csc registers and 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.
> >>>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >>>>> ---
> >>>>> drivers/gpu/drm/i915/i915_reg.h | 243
> >>>> +++++++++++++++++++++++++++++++++++
> >>>>> drivers/gpu/drm/i915/intel_display.c | 76 ++++++++++-
> >>>>> 2 files changed, 313 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> >>>>> b/drivers/gpu/drm/i915/i915_reg.h index 8bd61f9..f0f6f7c 100644
> >>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>>>> @@ -6553,6 +6553,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) /*
> >> Pre-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)
> >>>>> @@ -6569,6 +6570,248 @@ 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_3_A 0x703E0
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_RY_GY_1_B 0x711E0
> >>>>> +#define _PLANE_INPUT_CSC_RY_GY_2_B 0x712E0
> >>>>> +#define _PLANE_INPUT_CSC_RY_GY_3_B 0x713E0
> >>>>> +
> >>>>> +#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_RY_GY(pipe, plane) \
> >>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RY_GY_1(pipe), \
> >>>>> + _PLANE_INPUT_CSC_RY_GY_2(pipe))
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_BY_1_A 0x701E4
> >>>>> +#define _PLANE_INPUT_CSC_BY_2_A 0x702E4
> >>>>> +#define _PLANE_INPUT_CSC_BY_3_A 0x703E4
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_BY_1_B 0x711E4
> >>>>> +#define _PLANE_INPUT_CSC_BY_2_B 0x712E4
> >>>>> +#define _PLANE_INPUT_CSC_BY_3_B 0x713E4
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_BY_1(pipe) \
> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BY_1_A, \
> >>>>> + _PLANE_INPUT_CSC_BY_1_B)
> >>>>> +#define _PLANE_INPUT_CSC_BY_2(pipe) \
> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BY_2_A, \
> >>>>> + _PLANE_INPUT_CSC_BY_2_B)
> >>>>> +#define PLANE_INPUT_CSC_BY(pipe, plane) \
> >>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BY_1(pipe), \
> >>>>> + _PLANE_INPUT_CSC_BY_2(pipe))
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_RU_GU_1_A 0x701E8
> >>>>> +#define _PLANE_INPUT_CSC_RU_GU_2_A 0x702E8
> >>>>> +#define _PLANE_INPUT_CSC_RU_GU_3_A 0x703E8
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_RU_GU_1_B 0x711E8
> >>>>> +#define _PLANE_INPUT_CSC_RU_GU_2_B 0x712E8
> >>>>> +#define _PLANE_INPUT_CSC_RU_GU_3_B 0x713E8
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_RU_GU_1(pipe) \
> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_RU_GU_1_A, \
> >>>>> + _PLANE_INPUT_CSC_RU_GU_1_B)
> >>>>> +#define _PLANE_INPUT_CSC_RU_GU_2(pipe) \
> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_RU_GU_2_A, \
> >>>>> + _PLANE_INPUT_CSC_RU_GU_2_B)
> >>>>> +#define PLANE_INPUT_CSC_RU_GU(pipe, plane) \
> >>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RU_GU_1(pipe), \
> >>>>> + _PLANE_INPUT_CSC_RU_GU_2(pipe))
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_BU_1_A 0x701EC
> >>>>> +#define _PLANE_INPUT_CSC_BU_2_A 0x702EC
> >>>>> +#define _PLANE_INPUT_CSC_BU_3_A 0x703EC
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_BU_1_B 0x711EC
> >>>>> +#define _PLANE_INPUT_CSC_BU_2_B 0x712EC
> >>>>> +#define _PLANE_INPUT_CSC_BU_3_B 0x713EC
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_BU_1(pipe) \
> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BU_1_A, \
> >>>>> + _PLANE_INPUT_CSC_BU_1_B)
> >>>>> +#define _PLANE_INPUT_CSC_BU_2(pipe) \
> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BU_2_A, \
> >>>>> + _PLANE_INPUT_CSC_BU_2_B)
> >>>>> +#define PLANE_INPUT_CSC_BU(pipe, plane) \
> >>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BU_1(pipe), \
> >>>>> + _PLANE_INPUT_CSC_BU_2(pipe))
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_RV_GV_1_A 0x701F0
> >>>>> +#define _PLANE_INPUT_CSC_RV_GV_2_A 0x702F0
> >>>>> +#define _PLANE_INPUT_CSC_RV_GV_3_A 0x703F0
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_RV_GV_1_B 0x711F0
> >>>>> +#define _PLANE_INPUT_CSC_RV_GV_2_B 0x712F0
> >>>>> +#define _PLANE_INPUT_CSC_RV_GV_3_B 0x713F0
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_RV_GV_1(pipe) \
> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_RV_GV_1_A, \
> >>>>> + _PLANE_INPUT_CSC_RV_GV_1_B)
> >>>>> +#define _PLANE_INPUT_CSC_RV_GV_2(pipe) \
> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_RV_GV_2_A, \
> >>>>> + _PLANE_INPUT_CSC_RV_GV_2_B)
> >>>>> +#define PLANE_INPUT_CSC_RV_GV(pipe, plane) \
> >>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RV_GV_1(pipe), \
> >>>>> + _PLANE_INPUT_CSC_RV_GV_2(pipe))
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_BV_1_A 0x701F4
> >>>>> +#define _PLANE_INPUT_CSC_BV_2_A 0x702F4
> >>>>> +#define _PLANE_INPUT_CSC_BV_3_A 0x703F4
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_BV_1_B 0x711F4
> >>>>> +#define _PLANE_INPUT_CSC_BV_2_B 0x712F4
> >>>>> +#define _PLANE_INPUT_CSC_BV_3_B 0x713F4
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_BV_1(pipe) \
> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BV_1_A, \
> >>>>> + _PLANE_INPUT_CSC_BV_1_B)
> >>>>> +#define _PLANE_INPUT_CSC_BV_2(pipe) \
> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BV_2_A, \
> >>>>> + _PLANE_INPUT_CSC_BV_2_B)
> >>>>> +#define PLANE_INPUT_CSC_BV(pipe, plane) \
> >>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BV_1(pipe), \
> >>>>> + _PLANE_INPUT_CSC_BV_2(pipe))
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_1_A 0x701F8
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_2_A 0x702F8
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_3_A 0x703F8
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_1_B 0x711F8
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_2_B 0x712F8
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_3_B 0x713F8
> >>>>> +
> >>>>> +#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_HI(pipe, plane) \
> >>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_HI_1(pipe), \
> >>>>> + _PLANE_INPUT_CSC_PREOFF_HI_2(pipe))
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1_A 0x701FC
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2_A 0x702FC
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_3_A 0x703FC
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1_B 0x711FC
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2_B 0x712FC
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_3_B 0x713FC
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1(pipe) \
> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_ME_1_A, \
> >>>>> + _PLANE_INPUT_CSC_PREOFF_ME_1_B)
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2(pipe) \
> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_ME_2_A, \
> >>>>> + _PLANE_INPUT_CSC_PREOFF_ME_2_B)
> >>>>> +#define PLANE_INPUT_CSC_PREOFF_ME(pipe, plane) \
> >>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_ME_1(pipe), \
> >>>>> + _PLANE_INPUT_CSC_PREOFF_ME_2(pipe))
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1_A 0x70200
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2_A 0x70300
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_3_A 0x70400
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1_B 0x71200
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2_B 0x71300
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_3_B 0x71400
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1(pipe) \
> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_LO_1_A, \
> >>>>> + _PLANE_INPUT_CSC_PREOFF_LO_1_B)
> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2(pipe) \
> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_LO_2_A, \
> >>>>> + _PLANE_INPUT_CSC_PREOFF_LO_2_B)
> >>>>> +#define PLANE_INPUT_CSC_PREOFF_LO(pipe, plane) \
> >>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_LO_1(pipe), \
> >>>>> + _PLANE_INPUT_CSC_PREOFF_LO_2(pipe))
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_1_A 0x70204
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_2_A 0x70304
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_3_A 0x70404
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_1_B 0x71204
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_2_B 0x71304
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_3_B 0x71404
> >>>>> +
> >>>>> +#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_HI(pipe, plane) \
> >>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_HI_1(pipe), \
> >>>>> + _PLANE_INPUT_CSC_POSTOFF_HI_2(pipe))
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1_A 0x70208
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2_A 0x70308
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_3_A 0x70408
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1_B 0x71208
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2_B 0x71308
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_3_B 0x71408
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1(pipe) \
> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_ME_1_A, \
> >>>>> + _PLANE_INPUT_CSC_POSTOFF_ME_1_B)
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe) \
> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_ME_2_A, \
> >>>>> + _PLANE_INPUT_CSC_POSTOFF_ME_2_B)
> >>>>> +#define PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane) \
> >>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_ME_1(pipe), \
> >>>>> + _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe))
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1_A 0x7020C
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2_A 0x7030C
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_3_A 0x7040C
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1_B 0x7120C
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2_B 0x7130C
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_3_B 0x7140C
> >>>>> +
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1(pipe) \
> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_LO_1_A, \
> >>>>> + _PLANE_INPUT_CSC_POSTOFF_LO_1_B)
> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe) \
> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_LO_2_A, \
> >>>>> + _PLANE_INPUT_CSC_POSTOFF_LO_2_B)
> >>>>> +#define PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane) \
> >>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_LO_1(pipe), \
> >>>>> + _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe))
> >>>> Would probably be best to keep those separate.
> >>> You mean in a separate patch ? If yes, I can do that and segregate the
> >>> register macro definitions in a separate patch.
> >>>
> >>>>> +/*
> >>>>> + * These values are direct register values specified in the Bspec,
> >>>>> + * for YUV->RGB Full Range conversion matrix (colorspace BT709) */
> >>>>> +#define CSC_BT709_YUV_TO_RGB_RY_GY 0x7C987800
> >>>>> +#define CSC_BT709_YUV_TO_RGB_BY 0x0
> >>>>> +#define CSC_BT709_YUV_TO_RGB_RU_GU 0x9EF87800
> >>>>> +#define CSC_BT709_YUV_TO_RGB_BU 0xABF8
> >>>>> +#define CSC_BT709_YUV_TO_RGB_RV_GV 0x7800
> >>>>> +#define CSC_BT709_YUV_TO_RGB_BV 0x7ED8
> >>>>> +
> >>>>> +/*
> >>>>> + * These values are direct register values specified in the Bspec,
> >>>>> + * for YUV->RGB Full Range conversion matrix (colorspace BT601) */
> >>>>> +#define CSC_BT601_YUV_TO_RGB_RY_GY 0x7AF87800
> >>>>> +#define CSC_BT601_YUV_TO_RGB_BY 0x0
> >>>>> +#define CSC_BT601_YUV_TO_RGB_RU_GU 0x8B287800
> >>>>> +#define CSC_BT601_YUV_TO_RGB_BU 0x9AC0
> >>>>> +#define CSC_BT601_YUV_TO_RGB_RV_GV 0x7800
> >>>>> +#define CSC_BT601_YUV_TO_RGB_BV 0x7DD8
> >>>>> +
> >>>>> +/* Preoffset values for YUV to RGB Conversion */
> >>>>> +#define PREOFF_YUV_TO_RGB_HI 0x800
> >>>>> +#define PREOFF_YUV_TO_RGB_ME 0xF00
> >>>>> +#define PREOFF_YUV_TO_RGB_LO 0x800
> >>>>>
> >>>>> #define _PLANE_CTL_1_B 0x71180
> >>>>> #define _PLANE_CTL_2_B 0x71280
> >>>> Probably move those coefficients to intel_color.c ?
> >>> Sure, I will do that.
> >>>
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
> >>>>> b/drivers/gpu/drm/i915/intel_display.c
> >>>>> index fc7e3b0..38b41ed 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>>> @@ -3689,12 +3689,66 @@ u32 skl_plane_ctl(const struct
> >>>>> intel_crtc_state
> >>>> *crtc_state,
> >>>>> return plane_ctl;
> >>>>> }
> >>>>>
> >>>>> +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;
> >>>>> +
> >>>>> + if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709) {
> >>>>> + I915_WRITE(PLANE_INPUT_CSC_RY_GY(pipe, plane),
> >>>>> + CSC_BT709_YUV_TO_RGB_RY_GY);
> >>>>> + I915_WRITE(PLANE_INPUT_CSC_BY(pipe, plane),
> >>>>> + CSC_BT709_YUV_TO_RGB_BY);
> >>>>> + I915_WRITE(PLANE_INPUT_CSC_RU_GU(pipe, plane),
> >>>>> + CSC_BT709_YUV_TO_RGB_RU_GU);
> >>>>> + I915_WRITE(PLANE_INPUT_CSC_BU(pipe, plane),
> >>>>> + CSC_BT709_YUV_TO_RGB_BU);
> >>>>> + I915_WRITE(PLANE_INPUT_CSC_RV_GV(pipe, plane),
> >>>>> + CSC_BT709_YUV_TO_RGB_RV_GV);
> >>>>> + I915_WRITE(PLANE_INPUT_CSC_BV(pipe, plane),
> >>>>> + CSC_BT709_YUV_TO_RGB_BV);
> >>>>> + } else {
> >>>>> + I915_WRITE(PLANE_INPUT_CSC_RY_GY(pipe, plane),
> >>>>> + CSC_BT601_YUV_TO_RGB_RY_GY);
> >>>>> + I915_WRITE(PLANE_INPUT_CSC_BY(pipe, plane),
> >>>>> + CSC_BT601_YUV_TO_RGB_BY);
> >>>>> + I915_WRITE(PLANE_INPUT_CSC_RU_GU(pipe, plane),
> >>>>> + CSC_BT601_YUV_TO_RGB_RU_GU);
> >>>>> + I915_WRITE(PLANE_INPUT_CSC_BU(pipe, plane),
> >>>>> + CSC_BT601_YUV_TO_RGB_BU);
> >>>>> + I915_WRITE(PLANE_INPUT_CSC_RV_GV(pipe, plane),
> >>>>> + CSC_BT601_YUV_TO_RGB_RV_GV);
> >>>>> + I915_WRITE(PLANE_INPUT_CSC_BV(pipe, plane),
> >>>>> + CSC_BT601_YUV_TO_RGB_BV);
> >>>>> + }
> >>>> Considering there's going to be BT.601, BT.709 and perhaps newer
> >>>> colorspaces with limited vs full range, would it make more sense to generate
> >> the values?
> >>> These are all floating point coefficient values and driver has
> >>> restrictions on the floating math, so would be tough to generate them.
> >>> Currently only BT709 and BT601 was supported. Later BT2020
> >>> co-eficients can be added. Other way can be to expose this as a
> >>> property and get these values from userspace (I would not want to go that path
> >> as it will be an extra ABI burden and will require a userspace).
> >>> Personally I feel, defining 6 macro value for a new colorspace would be an
> >> easier option.
> >>>>> +
> >>>>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_HI(pipe, plane),
> >>>>> + PREOFF_YUV_TO_RGB_HI);
> >>>>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_ME(pipe, plane),
> >>>>> + PREOFF_YUV_TO_RGB_ME);
> >>>>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_LO(pipe, plane),
> >>>>> + PREOFF_YUV_TO_RGB_LO);
> >>>>> +
> >>>>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_HI(pipe, plane), 0x0);
> >>>>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane), 0x0);
> >>>>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane), 0x0); }
> >>>> We already have some support code for CSC matrices in intel_color.c,
> >>>> so I think it makes sense to program this from there..
> >>> Since for the non HDR planes CSC bits are programmed here, it would be
> >>> good if all planes are handled at one place. I will move the function
> >>> to intel_color.c to keep all the color related functions in one file.
> >>>
> >>>>> u32 glk_plane_color_ctl(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);
> >>>>> const struct drm_framebuffer *fb = plane_state->base.fb;
> >>>>> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> >>>>> + enum plane_id plane_id = plane->id;
> >>>>> +
> >>>>> u32 plane_color_ctl = 0;
> >>>>>
> >>>>> if (INTEL_GEN(dev_priv) < 11) {
> >>>>> @@ -3705,13 +3759,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 (INTEL_GEN(dev_priv) < 11 || plane_id > 2) {
> >>>> We now have icl_is_hdr_plane(), it just landed :)
> >>>>> + 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;
> >>>> The coefficients are likely different for full vs limited range, and
> >>>> I don't see that handled at the moment. :(
> >>> I have supported the FULL Range YUV to RGB conversion in this patch.
> >>> Will add limited range along with BT2020 with a later patch. This
> >>> should unblock all the current YUV Full Range planar format implementation.
> >> Hope this is ok ?
> >>> Thanks for the review Maarten. Will send out the updated patch based
> >>> on your recommendations.
> >> This would work for NV12, but what about P01X or the Y21X formats? Do they
> >> need slightly different coefficients for each?
> > All the planar formats will be up sampled before conversion. So standard YUV to
> > RGB conversion matrix should work even for P01X and Y21X formats.
> Not opposed to hardcoding then, but would be nice to have those values in nice static const u16[3][3] array instead of a bunch of defines..
Just like chv_update_csc() already does it.
I'd actually like to see a tool in igt that would be capable
of calculating the coefficients for each hardware generation
with a different matrix. We have igt_color_encoding so it
should be mostly a matter of converting the resulting matrix
to the correct fixed point (or whatever) precision.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB Conversion
2018-10-24 14:40 ` Ville Syrjälä
@ 2018-10-24 15:55 ` Shankar, Uma
0 siblings, 0 replies; 12+ messages in thread
From: Shankar, Uma @ 2018-10-24 15:55 UTC (permalink / raw)
To: Ville Syrjälä, Maarten Lankhorst
Cc: intel-gfx@lists.freedesktop.org, Syrjala, Ville,
Lankhorst, Maarten
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Wednesday, October 24, 2018 8:10 PM
>To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>Cc: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org;
>Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
><maarten.lankhorst@intel.com>
>Subject: Re: [Intel-gfx] [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB
>Conversion
>
>On Wed, Oct 24, 2018 at 03:42:34PM +0200, Maarten Lankhorst wrote:
>> Op 24-10-18 om 15:12 schreef Shankar, Uma:
>> >
>> >> -----Original Message-----
>> >> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
>> >> Sent: Wednesday, October 24, 2018 5:37 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] [PATCH] drm/i915: Enable Plane Input CSC
>> >> for YUV to RGB Conversion
>> >>
>> >> Op 24-10-18 om 13:19 schreef Shankar, Uma:
>> >>>> -----Original Message-----
>> >>>> From: Maarten Lankhorst
>> >>>> [mailto:maarten.lankhorst@linux.intel.com]
>> >>>> Sent: Wednesday, October 24, 2018 4:18 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] [PATCH] drm/i915: Enable Plane Input CSC
>> >>>> for YUV to RGB Conversion
>> >>>>
>> >>>> Op 23-10-18 om 22:11 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 plane input csc registers and
>> >>>>> 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.
>> >>>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> >>>>> ---
>> >>>>> drivers/gpu/drm/i915/i915_reg.h | 243
>> >>>> +++++++++++++++++++++++++++++++++++
>> >>>>> drivers/gpu/drm/i915/intel_display.c | 76 ++++++++++-
>> >>>>> 2 files changed, 313 insertions(+), 6 deletions(-)
>> >>>>>
>> >>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> >>>>> b/drivers/gpu/drm/i915/i915_reg.h index 8bd61f9..f0f6f7c 100644
>> >>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> >>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> >>>>> @@ -6553,6 +6553,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) /*
>> >> Pre-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)
>> >>>>> @@ -6569,6 +6570,248 @@ 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_3_A 0x703E0
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_RY_GY_1_B 0x711E0
>> >>>>> +#define _PLANE_INPUT_CSC_RY_GY_2_B 0x712E0
>> >>>>> +#define _PLANE_INPUT_CSC_RY_GY_3_B 0x713E0
>> >>>>> +
>> >>>>> +#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_RY_GY(pipe, plane) \
>> >>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RY_GY_1(pipe), \
>> >>>>> + _PLANE_INPUT_CSC_RY_GY_2(pipe))
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_BY_1_A 0x701E4
>> >>>>> +#define _PLANE_INPUT_CSC_BY_2_A 0x702E4
>> >>>>> +#define _PLANE_INPUT_CSC_BY_3_A 0x703E4
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_BY_1_B 0x711E4
>> >>>>> +#define _PLANE_INPUT_CSC_BY_2_B 0x712E4
>> >>>>> +#define _PLANE_INPUT_CSC_BY_3_B 0x713E4
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_BY_1(pipe) \
>> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BY_1_A, \
>> >>>>> + _PLANE_INPUT_CSC_BY_1_B)
>> >>>>> +#define _PLANE_INPUT_CSC_BY_2(pipe) \
>> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BY_2_A, \
>> >>>>> + _PLANE_INPUT_CSC_BY_2_B)
>> >>>>> +#define PLANE_INPUT_CSC_BY(pipe, plane) \
>> >>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BY_1(pipe), \
>> >>>>> + _PLANE_INPUT_CSC_BY_2(pipe))
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_RU_GU_1_A 0x701E8
>> >>>>> +#define _PLANE_INPUT_CSC_RU_GU_2_A 0x702E8
>> >>>>> +#define _PLANE_INPUT_CSC_RU_GU_3_A 0x703E8
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_RU_GU_1_B 0x711E8
>> >>>>> +#define _PLANE_INPUT_CSC_RU_GU_2_B 0x712E8
>> >>>>> +#define _PLANE_INPUT_CSC_RU_GU_3_B 0x713E8
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_RU_GU_1(pipe) \
>> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_RU_GU_1_A, \
>> >>>>> + _PLANE_INPUT_CSC_RU_GU_1_B)
>> >>>>> +#define _PLANE_INPUT_CSC_RU_GU_2(pipe) \
>> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_RU_GU_2_A, \
>> >>>>> + _PLANE_INPUT_CSC_RU_GU_2_B)
>> >>>>> +#define PLANE_INPUT_CSC_RU_GU(pipe, plane) \
>> >>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RU_GU_1(pipe), \
>> >>>>> + _PLANE_INPUT_CSC_RU_GU_2(pipe))
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_BU_1_A 0x701EC
>> >>>>> +#define _PLANE_INPUT_CSC_BU_2_A 0x702EC
>> >>>>> +#define _PLANE_INPUT_CSC_BU_3_A 0x703EC
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_BU_1_B 0x711EC
>> >>>>> +#define _PLANE_INPUT_CSC_BU_2_B 0x712EC
>> >>>>> +#define _PLANE_INPUT_CSC_BU_3_B 0x713EC
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_BU_1(pipe) \
>> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BU_1_A, \
>> >>>>> + _PLANE_INPUT_CSC_BU_1_B)
>> >>>>> +#define _PLANE_INPUT_CSC_BU_2(pipe) \
>> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BU_2_A, \
>> >>>>> + _PLANE_INPUT_CSC_BU_2_B)
>> >>>>> +#define PLANE_INPUT_CSC_BU(pipe, plane) \
>> >>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BU_1(pipe), \
>> >>>>> + _PLANE_INPUT_CSC_BU_2(pipe))
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_RV_GV_1_A 0x701F0
>> >>>>> +#define _PLANE_INPUT_CSC_RV_GV_2_A 0x702F0
>> >>>>> +#define _PLANE_INPUT_CSC_RV_GV_3_A 0x703F0
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_RV_GV_1_B 0x711F0
>> >>>>> +#define _PLANE_INPUT_CSC_RV_GV_2_B 0x712F0
>> >>>>> +#define _PLANE_INPUT_CSC_RV_GV_3_B 0x713F0
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_RV_GV_1(pipe) \
>> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_RV_GV_1_A, \
>> >>>>> + _PLANE_INPUT_CSC_RV_GV_1_B)
>> >>>>> +#define _PLANE_INPUT_CSC_RV_GV_2(pipe) \
>> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_RV_GV_2_A, \
>> >>>>> + _PLANE_INPUT_CSC_RV_GV_2_B)
>> >>>>> +#define PLANE_INPUT_CSC_RV_GV(pipe, plane) \
>> >>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RV_GV_1(pipe), \
>> >>>>> + _PLANE_INPUT_CSC_RV_GV_2(pipe))
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_BV_1_A 0x701F4
>> >>>>> +#define _PLANE_INPUT_CSC_BV_2_A 0x702F4
>> >>>>> +#define _PLANE_INPUT_CSC_BV_3_A 0x703F4
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_BV_1_B 0x711F4
>> >>>>> +#define _PLANE_INPUT_CSC_BV_2_B 0x712F4
>> >>>>> +#define _PLANE_INPUT_CSC_BV_3_B 0x713F4
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_BV_1(pipe) \
>> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BV_1_A, \
>> >>>>> + _PLANE_INPUT_CSC_BV_1_B)
>> >>>>> +#define _PLANE_INPUT_CSC_BV_2(pipe) \
>> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_BV_2_A, \
>> >>>>> + _PLANE_INPUT_CSC_BV_2_B)
>> >>>>> +#define PLANE_INPUT_CSC_BV(pipe, plane) \
>> >>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BV_1(pipe), \
>> >>>>> + _PLANE_INPUT_CSC_BV_2(pipe))
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_1_A 0x701F8
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_2_A 0x702F8
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_3_A 0x703F8
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_1_B 0x711F8
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_2_B 0x712F8
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_HI_3_B 0x713F8
>> >>>>> +
>> >>>>> +#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_HI(pipe, plane) \
>> >>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_HI_1(pipe),
>\
>> >>>>> + _PLANE_INPUT_CSC_PREOFF_HI_2(pipe))
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1_A 0x701FC
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2_A 0x702FC
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_3_A 0x703FC
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1_B 0x711FC
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2_B 0x712FC
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_3_B 0x713FC
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1(pipe) \
>> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_ME_1_A, \
>> >>>>> + _PLANE_INPUT_CSC_PREOFF_ME_1_B)
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2(pipe) \
>> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_ME_2_A, \
>> >>>>> + _PLANE_INPUT_CSC_PREOFF_ME_2_B)
>> >>>>> +#define PLANE_INPUT_CSC_PREOFF_ME(pipe, plane) \
>> >>>>> + _MMIO_PLANE(plane,
>_PLANE_INPUT_CSC_PREOFF_ME_1(pipe), \
>> >>>>> + _PLANE_INPUT_CSC_PREOFF_ME_2(pipe))
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1_A 0x70200
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2_A 0x70300
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_3_A 0x70400
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1_B 0x71200
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2_B 0x71300
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_3_B 0x71400
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1(pipe) \
>> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_LO_1_A, \
>> >>>>> + _PLANE_INPUT_CSC_PREOFF_LO_1_B)
>> >>>>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2(pipe) \
>> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_LO_2_A, \
>> >>>>> + _PLANE_INPUT_CSC_PREOFF_LO_2_B)
>> >>>>> +#define PLANE_INPUT_CSC_PREOFF_LO(pipe, plane) \
>> >>>>> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_LO_1(pipe),
>\
>> >>>>> + _PLANE_INPUT_CSC_PREOFF_LO_2(pipe))
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_1_A 0x70204
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_2_A 0x70304
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_3_A 0x70404
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_1_B 0x71204
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_2_B 0x71304
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_3_B 0x71404
>> >>>>> +
>> >>>>> +#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_HI(pipe, plane) \
>> >>>>> + _MMIO_PLANE(plane,
>_PLANE_INPUT_CSC_POSTOFF_HI_1(pipe), \
>> >>>>> + _PLANE_INPUT_CSC_POSTOFF_HI_2(pipe))
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1_A
> 0x70208
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2_A
> 0x70308
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_3_A
> 0x70408
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1_B
> 0x71208
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2_B
> 0x71308
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_3_B
> 0x71408
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1(pipe) \
>> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_ME_1_A, \
>> >>>>> + _PLANE_INPUT_CSC_POSTOFF_ME_1_B)
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe) \
>> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_ME_2_A, \
>> >>>>> + _PLANE_INPUT_CSC_POSTOFF_ME_2_B)
>> >>>>> +#define PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane) \
>> >>>>> + _MMIO_PLANE(plane,
>_PLANE_INPUT_CSC_POSTOFF_ME_1(pipe), \
>> >>>>> + _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe))
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1_A
> 0x7020C
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2_A
> 0x7030C
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_3_A
> 0x7040C
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1_B
> 0x7120C
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2_B
> 0x7130C
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_3_B
> 0x7140C
>> >>>>> +
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1(pipe) \
>> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_LO_1_A, \
>> >>>>> + _PLANE_INPUT_CSC_POSTOFF_LO_1_B)
>> >>>>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe) \
>> >>>>> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_LO_2_A, \
>> >>>>> + _PLANE_INPUT_CSC_POSTOFF_LO_2_B)
>> >>>>> +#define PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane) \
>> >>>>> + _MMIO_PLANE(plane,
>_PLANE_INPUT_CSC_POSTOFF_LO_1(pipe), \
>> >>>>> + _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe))
>> >>>> Would probably be best to keep those separate.
>> >>> You mean in a separate patch ? If yes, I can do that and segregate
>> >>> the register macro definitions in a separate patch.
>> >>>
>> >>>>> +/*
>> >>>>> + * These values are direct register values specified in the
>> >>>>> +Bspec,
>> >>>>> + * for YUV->RGB Full Range conversion matrix (colorspace BT709) */
>> >>>>> +#define CSC_BT709_YUV_TO_RGB_RY_GY 0x7C987800
>> >>>>> +#define CSC_BT709_YUV_TO_RGB_BY 0x0
>> >>>>> +#define CSC_BT709_YUV_TO_RGB_RU_GU 0x9EF87800
>> >>>>> +#define CSC_BT709_YUV_TO_RGB_BU 0xABF8
>> >>>>> +#define CSC_BT709_YUV_TO_RGB_RV_GV 0x7800
>> >>>>> +#define CSC_BT709_YUV_TO_RGB_BV 0x7ED8
>> >>>>> +
>> >>>>> +/*
>> >>>>> + * These values are direct register values specified in the
>> >>>>> +Bspec,
>> >>>>> + * for YUV->RGB Full Range conversion matrix (colorspace BT601) */
>> >>>>> +#define CSC_BT601_YUV_TO_RGB_RY_GY 0x7AF87800
>> >>>>> +#define CSC_BT601_YUV_TO_RGB_BY 0x0
>> >>>>> +#define CSC_BT601_YUV_TO_RGB_RU_GU 0x8B287800
>> >>>>> +#define CSC_BT601_YUV_TO_RGB_BU 0x9AC0
>> >>>>> +#define CSC_BT601_YUV_TO_RGB_RV_GV 0x7800
>> >>>>> +#define CSC_BT601_YUV_TO_RGB_BV 0x7DD8
>> >>>>> +
>> >>>>> +/* Preoffset values for YUV to RGB Conversion */
>> >>>>> +#define PREOFF_YUV_TO_RGB_HI 0x800
>> >>>>> +#define PREOFF_YUV_TO_RGB_ME 0xF00
>> >>>>> +#define PREOFF_YUV_TO_RGB_LO 0x800
>> >>>>>
>> >>>>> #define _PLANE_CTL_1_B 0x71180
>> >>>>> #define _PLANE_CTL_2_B 0x71280
>> >>>> Probably move those coefficients to intel_color.c ?
>> >>> Sure, I will do that.
>> >>>
>> >>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> >>>>> b/drivers/gpu/drm/i915/intel_display.c
>> >>>>> index fc7e3b0..38b41ed 100644
>> >>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>> >>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> >>>>> @@ -3689,12 +3689,66 @@ u32 skl_plane_ctl(const struct
>> >>>>> intel_crtc_state
>> >>>> *crtc_state,
>> >>>>> return plane_ctl;
>> >>>>> }
>> >>>>>
>> >>>>> +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;
>> >>>>> +
>> >>>>> + if (plane_state->base.color_encoding ==
>DRM_COLOR_YCBCR_BT709) {
>> >>>>> + I915_WRITE(PLANE_INPUT_CSC_RY_GY(pipe, plane),
>> >>>>> + CSC_BT709_YUV_TO_RGB_RY_GY);
>> >>>>> + I915_WRITE(PLANE_INPUT_CSC_BY(pipe, plane),
>> >>>>> + CSC_BT709_YUV_TO_RGB_BY);
>> >>>>> + I915_WRITE(PLANE_INPUT_CSC_RU_GU(pipe, plane),
>> >>>>> + CSC_BT709_YUV_TO_RGB_RU_GU);
>> >>>>> + I915_WRITE(PLANE_INPUT_CSC_BU(pipe, plane),
>> >>>>> + CSC_BT709_YUV_TO_RGB_BU);
>> >>>>> + I915_WRITE(PLANE_INPUT_CSC_RV_GV(pipe, plane),
>> >>>>> + CSC_BT709_YUV_TO_RGB_RV_GV);
>> >>>>> + I915_WRITE(PLANE_INPUT_CSC_BV(pipe, plane),
>> >>>>> + CSC_BT709_YUV_TO_RGB_BV);
>> >>>>> + } else {
>> >>>>> + I915_WRITE(PLANE_INPUT_CSC_RY_GY(pipe, plane),
>> >>>>> + CSC_BT601_YUV_TO_RGB_RY_GY);
>> >>>>> + I915_WRITE(PLANE_INPUT_CSC_BY(pipe, plane),
>> >>>>> + CSC_BT601_YUV_TO_RGB_BY);
>> >>>>> + I915_WRITE(PLANE_INPUT_CSC_RU_GU(pipe, plane),
>> >>>>> + CSC_BT601_YUV_TO_RGB_RU_GU);
>> >>>>> + I915_WRITE(PLANE_INPUT_CSC_BU(pipe, plane),
>> >>>>> + CSC_BT601_YUV_TO_RGB_BU);
>> >>>>> + I915_WRITE(PLANE_INPUT_CSC_RV_GV(pipe, plane),
>> >>>>> + CSC_BT601_YUV_TO_RGB_RV_GV);
>> >>>>> + I915_WRITE(PLANE_INPUT_CSC_BV(pipe, plane),
>> >>>>> + CSC_BT601_YUV_TO_RGB_BV);
>> >>>>> + }
>> >>>> Considering there's going to be BT.601, BT.709 and perhaps newer
>> >>>> colorspaces with limited vs full range, would it make more sense
>> >>>> to generate
>> >> the values?
>> >>> These are all floating point coefficient values and driver has
>> >>> restrictions on the floating math, so would be tough to generate them.
>> >>> Currently only BT709 and BT601 was supported. Later BT2020
>> >>> co-eficients can be added. Other way can be to expose this as a
>> >>> property and get these values from userspace (I would not want to
>> >>> go that path
>> >> as it will be an extra ABI burden and will require a userspace).
>> >>> Personally I feel, defining 6 macro value for a new colorspace
>> >>> would be an
>> >> easier option.
>> >>>>> +
>> >>>>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_HI(pipe, plane),
>> >>>>> + PREOFF_YUV_TO_RGB_HI);
>> >>>>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_ME(pipe, plane),
>> >>>>> + PREOFF_YUV_TO_RGB_ME);
>> >>>>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_LO(pipe, plane),
>> >>>>> + PREOFF_YUV_TO_RGB_LO);
>> >>>>> +
>> >>>>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_HI(pipe, plane), 0x0);
>> >>>>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane),
>0x0);
>> >>>>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane), 0x0);
>}
>> >>>> We already have some support code for CSC matrices in
>> >>>> intel_color.c, so I think it makes sense to program this from there..
>> >>> Since for the non HDR planes CSC bits are programmed here, it
>> >>> would be good if all planes are handled at one place. I will move
>> >>> the function to intel_color.c to keep all the color related functions in one
>file.
>> >>>
>> >>>>> u32 glk_plane_color_ctl(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);
>> >>>>> const struct drm_framebuffer *fb = plane_state->base.fb;
>> >>>>> + struct intel_plane *plane = to_intel_plane(plane_state-
>>base.plane);
>> >>>>> + enum plane_id plane_id = plane->id;
>> >>>>> +
>> >>>>> u32 plane_color_ctl = 0;
>> >>>>>
>> >>>>> if (INTEL_GEN(dev_priv) < 11) { @@ -3705,13 +3759,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 (INTEL_GEN(dev_priv) < 11 || plane_id > 2) {
>> >>>> We now have icl_is_hdr_plane(), it just landed :)
>> >>>>> + 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;
>> >>>> The coefficients are likely different for full vs limited range,
>> >>>> and I don't see that handled at the moment. :(
>> >>> I have supported the FULL Range YUV to RGB conversion in this patch.
>> >>> Will add limited range along with BT2020 with a later patch. This
>> >>> should unblock all the current YUV Full Range planar format
>implementation.
>> >> Hope this is ok ?
>> >>> Thanks for the review Maarten. Will send out the updated patch
>> >>> based on your recommendations.
>> >> This would work for NV12, but what about P01X or the Y21X formats?
>> >> Do they need slightly different coefficients for each?
>> > All the planar formats will be up sampled before conversion. So
>> > standard YUV to RGB conversion matrix should work even for P01X and Y21X
>formats.
>> Not opposed to hardcoding then, but would be nice to have those values in nice
>static const u16[3][3] array instead of a bunch of defines..
>
>Just like chv_update_csc() already does it.
Sure, got it. Will update the patch accordingly.
>I'd actually like to see a tool in igt that would be capable of calculating the
>coefficients for each hardware generation with a different matrix. We have
>igt_color_encoding so it should be mostly a matter of converting the resulting
>matrix to the correct fixed point (or whatever) precision.
I will try to create something on these lines. Atleast a mechanism to get
the values which matches to bspec register expectations.
Thanks & Regards,
Uma Shankar
>--
>Ville Syrjälä
>Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-10-24 15:55 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-23 20:11 [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB Conversion Uma Shankar
2018-10-23 20:11 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2018-10-23 20:12 ` ✗ Fi.CI.SPARSE: " Patchwork
2018-10-23 20:36 ` ✓ Fi.CI.BAT: success " Patchwork
2018-10-23 23:58 ` ✓ Fi.CI.IGT: " Patchwork
2018-10-24 10:47 ` [PATCH] " Maarten Lankhorst
2018-10-24 11:19 ` Shankar, Uma
2018-10-24 12:07 ` Maarten Lankhorst
2018-10-24 13:12 ` Shankar, Uma
2018-10-24 13:42 ` Maarten Lankhorst
2018-10-24 14:40 ` Ville Syrjälä
2018-10-24 15:55 ` Shankar, Uma
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox