>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] >Sent: Thursday, March 14, 2019 1:21 AM >To: Shankar, Uma <uma.shankar@xxxxxxxxx> >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Roper, Matthew D ><matthew.d.roper@xxxxxxxxx> >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@xxxxxxxxxxxxxxx] >> >Sent: Tuesday, February 19, 2019 1:02 AM >> >To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> >Cc: Shankar, Uma <uma.shankar@xxxxxxxxx>; Roper, Matthew D >> ><matthew.d.roper@xxxxxxxxx> >> >Subject: [PATCH 4/7] drm/i915: Clean up ilk/icl pipe/output CSC >> >programming >> > >> >From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> >> > >> >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@xxxxxxxxxxxxxxx> >> >--- >> > 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@xxxxxxxxx> >> >> >> >+ >> >+/* 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
