> -----Original Message-----
> From: Intel-gfx <intel-gfx-boun...@lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: Monday, November 14, 2022 9:08 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH v3 18/20] drm/i915: Use gamma LUT for RGB limited
> range compression
>
> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>
> On ilk+ and glk class hardware we current make a mess of things when we have
> to
> both generate limited range output and use the hw gamma LUT. Since we do the
> range compression using the pipe CSC unit (which is situated before the gamma
> LUT
> in the pipe) we are in fact applying the gamma to the limited range data
> instead of
> the full range data as the user intended.
>
> We can work around this by applying the range compression via the gamma LUT
> instead of using the pipe CSC for it.
> Fairly easy to do now that we have the internal post_csc_lut attachment point
> where
> we can stick our new cooked LUT.
>
> On ilk+ this only needs to be dome when using the split gamma mode or when the
Nit: Typo in "done"
Looks Good to me.
Reviewed-by: Uma Shankar <uma.shan...@intel.com>
> ctm is enabled since otherwise we can simply reorder the LUT vs. CSC. On glk
> we
> need to do this any time a gamma LUT is used since no reordering is possible.
> We do lose a bit of coverage in intel_color_assert_luts(), but so be it.
>
> Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_color.c | 133 +++++++++++++++++----
> 1 file changed, 111 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index c336524d9225..dee0382015a5 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -249,17 +249,44 @@ static void icl_update_output_csc(struct intel_crtc
> *crtc,
> intel_de_write_fw(i915, PIPE_CSC_OUTPUT_POSTOFF_LO(pipe), postoff[2]);
> }
>
> +static bool ilk_limited_range(const struct intel_crtc_state
> +*crtc_state) {
> + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> +
> + /* icl+ have dedicated output CSC */
> + if (DISPLAY_VER(i915) >= 11)
> + return false;
> +
> + /* pre-hsw have PIPECONF_COLOR_RANGE_SELECT */
> + if (DISPLAY_VER(i915) < 7 || IS_IVYBRIDGE(i915))
> + return false;
> +
> + return crtc_state->limited_color_range; }
> +
> +static bool ilk_lut_limited_range(const struct intel_crtc_state
> +*crtc_state) {
> + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> +
> + if (!ilk_limited_range(crtc_state))
> + return false;
> +
> + if (crtc_state->c8_planes)
> + return false;
> +
> + if (DISPLAY_VER(i915) == 10)
> + return crtc_state->hw.gamma_lut;
> + else
> + return crtc_state->hw.gamma_lut &&
> + (crtc_state->hw.degamma_lut || crtc_state->hw.ctm); }
> +
> static bool ilk_csc_limited_range(const struct intel_crtc_state *crtc_state)
> {
> - struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> + if (!ilk_limited_range(crtc_state))
> + return false;
>
> - /*
> - * FIXME if there's a gamma LUT after the CSC, we should
> - * do the range compression using the gamma LUT instead.
> - */
> - return crtc_state->limited_color_range &&
> - (IS_HASWELL(i915) || IS_BROADWELL(i915) ||
> - IS_DISPLAY_VER(i915, 9, 10));
> + return !ilk_lut_limited_range(crtc_state);
> }
>
> static void ilk_csc_convert_ctm(const struct intel_crtc_state *crtc_state,
> @@ -
> 603,9 +630,18 @@ create_linear_lut(struct drm_i915_private *i915, int
> lut_size)
> return blob;
> }
>
> +static u16 lut_limited_range(unsigned int value) {
> + unsigned int min = 16 << 8;
> + unsigned int max = 235 << 8;
> +
> + return value * (max - min) / 0xffff + min; }
> +
> static struct drm_property_blob *
> create_resized_lut(struct drm_i915_private *i915,
> - const struct drm_property_blob *blob_in, int lut_out_size)
> + const struct drm_property_blob *blob_in, int lut_out_size,
> + bool limited_color_range)
> {
> int i, lut_in_size = drm_color_lut_size(blob_in);
> struct drm_property_blob *blob_out;
> @@ -621,8 +657,18 @@ create_resized_lut(struct drm_i915_private *i915,
> lut_in = blob_in->data;
> lut_out = blob_out->data;
>
> - for (i = 0; i < lut_out_size; i++)
> - lut_out[i] = lut_in[i * (lut_in_size - 1) / (lut_out_size - 1)];
> + for (i = 0; i < lut_out_size; i++) {
> + const struct drm_color_lut *entry =
> + &lut_in[i * (lut_in_size - 1) / (lut_out_size - 1)];
> +
> + if (limited_color_range) {
> + lut_out[i].red = lut_limited_range(entry->red);
> + lut_out[i].green = lut_limited_range(entry->green);
> + lut_out[i].blue = lut_limited_range(entry->blue);
> + } else {
> + lut_out[i] = *entry;
> + }
> + }
>
> return blob_out;
> }
> @@ -1423,6 +1469,7 @@ void intel_color_assert_luts(const struct
> intel_crtc_state
> *crtc_state)
> crtc_state->pre_csc_lut !=
> crtc_state->hw.degamma_lut
> &&
> crtc_state->pre_csc_lut != i915-
> >display.color.glk_linear_degamma_lut);
> drm_WARN_ON(&i915->drm,
> + !ilk_lut_limited_range(crtc_state) &&
> crtc_state->post_csc_lut != NULL &&
> crtc_state->post_csc_lut !=
> crtc_state->hw.gamma_lut);
> } else if (crtc_state->gamma_mode != GAMMA_MODE_MODE_SPLIT) { @@
> -1430,6 +1477,7 @@ void intel_color_assert_luts(const struct intel_crtc_state
> *crtc_state)
> crtc_state->pre_csc_lut !=
> crtc_state->hw.degamma_lut
> &&
> crtc_state->pre_csc_lut !=
> crtc_state->hw.gamma_lut);
> drm_WARN_ON(&i915->drm,
> + !ilk_lut_limited_range(crtc_state) &&
> crtc_state->post_csc_lut !=
> crtc_state->hw.degamma_lut
> &&
> crtc_state->post_csc_lut !=
> crtc_state->hw.gamma_lut);
> }
> @@ -1563,8 +1611,28 @@ static u32 ilk_csc_mode(const struct intel_crtc_state
> *crtc_state)
> CSC_POSITION_BEFORE_GAMMA;
> }
>
> -static void ilk_assign_luts(struct intel_crtc_state *crtc_state)
> +static int ilk_assign_luts(struct intel_crtc_state *crtc_state)
> {
> + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> +
> + if (ilk_lut_limited_range(crtc_state)) {
> + struct drm_property_blob *gamma_lut;
> +
> + gamma_lut = create_resized_lut(i915, crtc_state->hw.gamma_lut,
> + drm_color_lut_size(crtc_state-
> >hw.gamma_lut),
> + true);
> + if (IS_ERR(gamma_lut))
> + return PTR_ERR(gamma_lut);
> +
> + drm_property_replace_blob(&crtc_state->post_csc_lut,
> gamma_lut);
> +
> + drm_property_blob_put(gamma_lut);
> +
> + drm_property_replace_blob(&crtc_state->pre_csc_lut,
> +crtc_state->hw.degamma_lut);
> +
> + return 0;
> + }
> +
> if (crtc_state->hw.degamma_lut ||
> crtc_state->csc_mode & CSC_POSITION_BEFORE_GAMMA) {
> drm_property_replace_blob(&crtc_state->pre_csc_lut,
> @@ -1577,6 +1645,8 @@ static void ilk_assign_luts(struct intel_crtc_state
> *crtc_state)
> drm_property_replace_blob(&crtc_state->post_csc_lut,
> NULL);
> }
> +
> + return 0;
> }
>
> static int ilk_color_check(struct intel_crtc_state *crtc_state) @@ -1613,7
> +1683,9
> @@ static int ilk_color_check(struct intel_crtc_state *crtc_state)
> if (ret)
> return ret;
>
> - ilk_assign_luts(crtc_state);
> + ret = ilk_assign_luts(crtc_state);
> + if (ret)
> + return ret;
>
> crtc_state->preload_luts = intel_can_preload_luts(crtc_state);
>
> @@ -1649,19 +1721,19 @@ static int ivb_assign_luts(struct intel_crtc_state
> *crtc_state)
> struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> struct drm_property_blob *degamma_lut, *gamma_lut;
>
> - if (crtc_state->gamma_mode != GAMMA_MODE_MODE_SPLIT) {
> - ilk_assign_luts(crtc_state);
> - return 0;
> - }
> + if (crtc_state->gamma_mode != GAMMA_MODE_MODE_SPLIT)
> + return ilk_assign_luts(crtc_state);
>
> drm_WARN_ON(&i915->drm, drm_color_lut_size(crtc_state-
> >hw.degamma_lut) != 1024);
> drm_WARN_ON(&i915->drm, drm_color_lut_size(crtc_state-
> >hw.gamma_lut) != 1024);
>
> - degamma_lut = create_resized_lut(i915, crtc_state->hw.degamma_lut,
> 512);
> + degamma_lut = create_resized_lut(i915, crtc_state->hw.degamma_lut, 512,
> + false);
> if (IS_ERR(degamma_lut))
> return PTR_ERR(degamma_lut);
>
> - gamma_lut = create_resized_lut(i915, crtc_state->hw.gamma_lut, 512);
> + gamma_lut = create_resized_lut(i915, crtc_state->hw.gamma_lut, 512,
> + ilk_lut_limited_range(crtc_state));
> if (IS_ERR(gamma_lut)) {
> drm_property_blob_put(degamma_lut);
> return PTR_ERR(gamma_lut);
> @@ -1750,7 +1822,8 @@ static int glk_assign_luts(struct intel_crtc_state
> *crtc_state)
> struct drm_property_blob *gamma_lut;
>
> gamma_lut = create_resized_lut(i915, crtc_state->hw.gamma_lut,
> - INTEL_INFO(i915)-
> >display.color.degamma_lut_size);
> + INTEL_INFO(i915)-
> >display.color.degamma_lut_size,
> + false);
> if (IS_ERR(gamma_lut))
> return PTR_ERR(gamma_lut);
>
> @@ -1762,7 +1835,23 @@ static int glk_assign_luts(struct intel_crtc_state
> *crtc_state)
> return 0;
> }
>
> - intel_assign_luts(crtc_state);
> + if (ilk_lut_limited_range(crtc_state)) {
> + struct drm_property_blob *gamma_lut;
> +
> + gamma_lut = create_resized_lut(i915, crtc_state->hw.gamma_lut,
> + drm_color_lut_size(crtc_state-
> >hw.gamma_lut),
> + true);
> + if (IS_ERR(gamma_lut))
> + return PTR_ERR(gamma_lut);
> +
> + drm_property_replace_blob(&crtc_state->post_csc_lut,
> gamma_lut);
> +
> + drm_property_blob_put(gamma_lut);
> + } else {
> + drm_property_replace_blob(&crtc_state->post_csc_lut, crtc_state-
> >hw.gamma_lut);
> + }
> +
> + drm_property_replace_blob(&crtc_state->pre_csc_lut,
> +crtc_state->hw.degamma_lut);
>
> /*
> * On GLK+ both pipe CSC and degamma LUT are controlled @@ -1821,7
> +1910,7 @@ static int glk_color_check(struct intel_crtc_state *crtc_state)
> glk_use_pre_csc_lut_for_gamma(crtc_state) ||
> crtc_state->hw.degamma_lut ||
> crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
> - crtc_state->hw.ctm || crtc_state->limited_color_range;
> + crtc_state->hw.ctm || ilk_csc_limited_range(crtc_state);
>
> crtc_state->gamma_mode = glk_gamma_mode(crtc_state);
>
> --
> 2.37.4
