[Intel-gfx] [PATCH 4/7] drm/i915: Clean up ilk/icl pipe/output CSC programming
Shankar, Uma
uma.shankar at intel.com
Thu Mar 14 13:23:00 UTC 2019
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
>Sent: Thursday, March 14, 2019 1:21 AM
>To: Shankar, Uma <uma.shankar at intel.com>
>Cc: intel-gfx at lists.freedesktop.org; Roper, Matthew D
><matthew.d.roper at intel.com>
>Subject: Re: [PATCH 4/7] drm/i915: Clean up ilk/icl pipe/output CSC programming
>
>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.
Hmm yes indeed the spec is putting full range there. So this is fine then.
Reviewed-by: Uma Shankar <uma.shankar at intel.com>
>>
>>
>> >+
>> >+/* 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.
Hmm ok, got it. In that case, its ok to drop it.
>>
>> >- }
>> >+ 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