[Intel-gfx] [PATCH 4/7] drm/i915: Clean up ilk/icl pipe/output CSC programming
Ville Syrjälä
ville.syrjala at linux.intel.com
Wed Mar 13 19:51:03 UTC 2019
On Wed, Mar 13, 2019 at 04:38:01PM +0000, Shankar, Uma wrote:
>
>
> >-----Original Message-----
> >From: Ville Syrjala [mailto:ville.syrjala at linux.intel.com]
> >Sent: Tuesday, February 19, 2019 1:02 AM
> >To: intel-gfx at lists.freedesktop.org
> >Cc: Shankar, Uma <uma.shankar at intel.com>; Roper, Matthew D
> ><matthew.d.roper at intel.com>
> >Subject: [PATCH 4/7] drm/i915: Clean up ilk/icl pipe/output CSC programming
> >
> >From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> >We have far too much messy duplicated code in the pipe/output CSC programming.
> >Simply provide two functions
> >(ilk_update_pipe_csc() and icl_update_output_csc()) to program the relevant CSC
> >registers. The desired offsets and coefficients are passed in as parameters.
> >
> >Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >---
> > drivers/gpu/drm/i915/intel_color.c | 168 ++++++++++++++---------------
> > 1 file changed, 82 insertions(+), 86 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> >index ddc48c0d45ac..61cb69058b35 100644
> >--- a/drivers/gpu/drm/i915/intel_color.c
> >+++ b/drivers/gpu/drm/i915/intel_color.c
> >@@ -40,23 +40,6 @@
> > #define CTM_COEFF_ABS(coeff) ((coeff) & (CTM_COEFF_SIGN - 1))
> >
> > #define LEGACY_LUT_LENGTH 256
> >-
> >-/* Post offset values for RGB->YCBCR conversion */ -#define
> >POSTOFF_RGB_TO_YUV_HI 0x800 -#define POSTOFF_RGB_TO_YUV_ME 0x100 -
> >#define POSTOFF_RGB_TO_YUV_LO 0x800
> >-
> >-/*
> >- * These values are direct register values specified in the Bspec,
> >- * for RGB->YUV conversion matrix (colorspace BT709)
> >- */
> >-#define CSC_RGB_TO_YUV_RU_GU 0x2ba809d8 -#define CSC_RGB_TO_YUV_BU
> >0x37e80000 -#define CSC_RGB_TO_YUV_RY_GY 0x1e089cc0 -#define
> >CSC_RGB_TO_YUV_BY 0xb5280000 -#define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8 -
> >#define CSC_RGB_TO_YUV_BV 0x1e080000
> >-
> > /*
> > * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
> > * format). This macro takes the coefficient we want transformed and the @@ -74,6
> >+57,31 @@
> > #define ILK_CSC_COEFF_1_0 \
> > ((7 << 12) | ILK_CSC_COEFF_FP(CTM_COEFF_1_0, 8))
> >
> >+#define ILK_CSC_POSTOFF_LIMITED_RANGE (16 * (1 << 12) / 255)
> >+
> >+static const u16 ilk_csc_off_zero[3] = {};
> >+
> >+static const u16 ilk_csc_postoff_limited_range[3] = {
> >+ ILK_CSC_POSTOFF_LIMITED_RANGE,
> >+ ILK_CSC_POSTOFF_LIMITED_RANGE,
> >+ ILK_CSC_POSTOFF_LIMITED_RANGE,
> >+};
> >+
> >+/*
> >+ * These values are direct register values specified in the Bspec,
> >+ * for RGB->YUV conversion matrix (colorspace BT709) */ static const
> >+u16 ilk_csc_coeff_rgb_to_ycbcr[9] = {
> >+ 0x1e08, 0x9cc0, 0xb528,
> >+ 0x2ba8, 0x09d8, 0x37e8,
> >+ 0xbce8, 0x9ad8, 0x1e08,
> >+};
>
> I am not sure if the matrix coefficients are correct. Can you please cross check, if I am
> missing something. Spec has these as values (hoping table doesn’t get distorted while sending :))
> Bt.601 Bt.709
> Value Program Value Program
> RU 0.2990 0x1990 0.21260 0x2D98
> GU 0.5870 0x0968 0.71520 0x0B70
> BU 0.1140 0x3E98 0.07220 0x3940
> RV -0.1687 0xAAC8 -0.11460 0xBEA8
> GV -0.3313 0x9A98 -0.38540 0x9C58
> BV 0.5000 0x0800 0.50000 0x0800
> RY 0.5000 0x0800 0.50000 0x0800
> GY -0.4187 0x9D68 -0.45420 0x9E88
> BY -0.0813 0xBA68 -0.04580 0xB5E0
My calculations are giving me this:
0x1e10, 0x9cc8, 0xb528,
0x2bb0, 0x09d0, 0x37f0,
0xbce0, 0x9ad8, 0x1e10,
The difference between my numbers and the ones in the code seem
to be more or less just due to rounding.
The numbers in the spec would appear to be for full range output,
but we want limited range.
>
>
> >+
> >+/* Post offset values for RGB->YCBCR conversion */ static const u16
> >+ilk_csc_postoff_rgb_to_ycbcr[3] = {
> >+ 0x0800, 0x0100, 0x0800,
> >+};
> >+
> > static bool lut_is_legacy(const struct drm_property_blob *lut) {
> > return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH; @@ -113,54
> >+121,60 @@ static u64 *ctm_mult_by_limited(u64 *result, const u64 *input)
> > return result;
> > }
> >
> >-static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc *crtc)
> >+static void ilk_update_pipe_csc(struct intel_crtc *crtc,
> >+ const u16 preoff[3],
> >+ const u16 coeff[9],
> >+ const u16 postoff[3])
> > {
> > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > enum pipe pipe = crtc->pipe;
> >
> >- if (INTEL_GEN(dev_priv) < 11) {
> >- I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
> >- I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
> >- I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0);
> >+ I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), preoff[0]);
> >+ I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), preoff[1]);
> >+ I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), preoff[2]);
> >
> >- I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe),
> >CSC_RGB_TO_YUV_RU_GU);
> >- I915_WRITE(PIPE_CSC_COEFF_BU(pipe), CSC_RGB_TO_YUV_BU);
> >+ I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe), coeff[0] << 16 | coeff[1]);
> >+ I915_WRITE(PIPE_CSC_COEFF_BY(pipe), coeff[2] << 16);
> >
> >- I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe),
> >CSC_RGB_TO_YUV_RY_GY);
> >- I915_WRITE(PIPE_CSC_COEFF_BY(pipe), CSC_RGB_TO_YUV_BY);
> >+ I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe), coeff[3] << 16 | coeff[4]);
> >+ I915_WRITE(PIPE_CSC_COEFF_BU(pipe), coeff[5] << 16);
> >
> >- I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe),
> >CSC_RGB_TO_YUV_RV_GV);
> >- I915_WRITE(PIPE_CSC_COEFF_BV(pipe), CSC_RGB_TO_YUV_BV);
> >+ I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe), coeff[6] << 16 | coeff[7]);
> >+ I915_WRITE(PIPE_CSC_COEFF_BV(pipe), coeff[8] << 16);
> >
> >- I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe),
> >POSTOFF_RGB_TO_YUV_HI);
> >- I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe),
> >POSTOFF_RGB_TO_YUV_ME);
> >- I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe),
> >POSTOFF_RGB_TO_YUV_LO);
> >- } else {
> >- I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_HI(pipe), 0);
> >- I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_ME(pipe), 0);
> >- I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_LO(pipe), 0);
> >-
> >- I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RU_GU(pipe),
> >- CSC_RGB_TO_YUV_RU_GU);
> >- I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BU(pipe),
> >CSC_RGB_TO_YUV_BU);
> >-
> >- I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RY_GY(pipe),
> >- CSC_RGB_TO_YUV_RY_GY);
> >- I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BY(pipe),
> >CSC_RGB_TO_YUV_BY);
> >-
> >- I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RV_GV(pipe),
> >- CSC_RGB_TO_YUV_RV_GV);
> >- I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BV(pipe),
> >CSC_RGB_TO_YUV_BV);
> >-
> >- I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_HI(pipe),
> >- POSTOFF_RGB_TO_YUV_HI);
> >- I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_ME(pipe),
> >- POSTOFF_RGB_TO_YUV_ME);
> >- I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_LO(pipe),
> >- POSTOFF_RGB_TO_YUV_LO);
> >+ if (INTEL_GEN(dev_priv) >= 7) {
> >+ I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff[0]);
> >+ I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff[1]);
> >+ I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff[2]);
> > }
> > }
> >
> >+static void icl_update_output_csc(struct intel_crtc *crtc,
> >+ const u16 preoff[3],
> >+ const u16 coeff[9],
> >+ const u16 postoff[3])
> >+{
> >+ struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >+ enum pipe pipe = crtc->pipe;
> >+
> >+ I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_HI(pipe), preoff[0]);
> >+ I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_ME(pipe), preoff[1]);
> >+ I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_LO(pipe), preoff[2]);
> >+
> >+ I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RY_GY(pipe), coeff[0] << 16 |
> >coeff[1]);
> >+ I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BY(pipe), coeff[2]);
> >+
> >+ I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RU_GU(pipe), coeff[3] << 16 |
> >coeff[4]);
> >+ I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BU(pipe), coeff[5]);
> >+
> >+ I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RV_GV(pipe), coeff[6] << 16 |
> >coeff[7]);
> >+ I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BV(pipe), coeff[8]);
> >+
> >+ I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_HI(pipe), postoff[0]);
> >+ I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_ME(pipe), postoff[1]);
> >+ I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_LO(pipe), postoff[2]); }
> >+
> > static bool ilk_csc_limited_range(const struct intel_crtc_state *crtc_state) {
> > struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> >@@ -185,7 +199,15 @@ static void ilk_load_csc_matrix(const struct intel_crtc_state
> >*crtc_state)
> >
> > if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
> > crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) {
> >- ilk_load_ycbcr_conversion_matrix(crtc);
> >+ if (INTEL_GEN(dev_priv) >= 11)
> >+ icl_update_output_csc(crtc, ilk_csc_off_zero,
> >+ ilk_csc_coeff_rgb_to_ycbcr,
> >+ ilk_csc_postoff_rgb_to_ycbcr);
> >+ else
> >+ ilk_update_pipe_csc(crtc, ilk_csc_off_zero,
> >+ ilk_csc_coeff_rgb_to_ycbcr,
> >+ ilk_csc_postoff_rgb_to_ycbcr);
> >+
> > I915_WRITE(PIPE_CSC_MODE(pipe), crtc_state->csc_mode);
> > /*
> > * On pre GEN11 output CSC is not there, so with 1 pipe CSC @@ -
> >258,38 +280,12 @@ static void ilk_load_csc_matrix(const struct intel_crtc_state
> >*crtc_state)
> > }
> > }
> >
> >- I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe), coeffs[0] << 16 | coeffs[1]);
> >- I915_WRITE(PIPE_CSC_COEFF_BY(pipe), coeffs[2] << 16);
> >-
> >- I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe), coeffs[3] << 16 | coeffs[4]);
> >- I915_WRITE(PIPE_CSC_COEFF_BU(pipe), coeffs[5] << 16);
> >-
> >- I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe), coeffs[6] << 16 | coeffs[7]);
> >- I915_WRITE(PIPE_CSC_COEFF_BV(pipe), coeffs[8] << 16);
> >+ ilk_update_pipe_csc(crtc, ilk_csc_off_zero, coeffs,
> >+ limited_color_range ?
> >+ ilk_csc_postoff_limited_range :
> >+ ilk_csc_off_zero);
> >
> >- I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
> >- I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
> >- I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0);
> >-
> >- if (INTEL_GEN(dev_priv) > 6) {
> >- u16 postoff = 0;
> >-
> >- if (limited_color_range)
> >- postoff = (16 * (1 << 12) / 255) & 0x1fff;
> >-
> >- I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff);
> >- I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff);
> >- I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff);
> >-
> >- I915_WRITE(PIPE_CSC_MODE(pipe), crtc_state->csc_mode);
> >- } else {
> >- u32 mode = CSC_MODE_YUV_TO_RGB;
> >-
> >- if (limited_color_range)
> >- mode |= CSC_BLACK_SCREEN_OFFSET;
> >-
> >- I915_WRITE(PIPE_CSC_MODE(pipe), mode);
>
> Looks like this is not handled and got dropped. Pre Gen7 stuff.
Yeah, we don't use the CSC (yet) on pre-HSW. Even if we start to
use it we'll not be using it for RGB full->limited conversion.
So we won't actually program it like this in the end.
>
> >- }
> >+ I915_WRITE(PIPE_CSC_MODE(pipe), crtc_state->csc_mode);
> > }
> >
> > /*
> >--
> >2.19.2
>
--
Ville Syrjälä
Intel
More information about the Intel-gfx
mailing list