Re: [Intel-gfx] [PATCH] drm/i915/xelpd: Enabling dithering after the CC1

[Jani Nikula <jani.nikula@intel.com>]

Cc: Ville and Daniel for archeology...

On Fri, 11 Jun 2021, Nischal Varide <nischal.varide@intel.com> wrote:
> If the panel is 12bpc then Dithering is not enabled in the Legacy
> dithering block , instead its Enabled after the C1 CC1 pipe post
> color space conversion.For a 6bpc pannel Dithering is enabled in
> Legacy block.

Currently, we only ever enable dithering for 6 bpc displays. See commit
e8fa4270536d ("drm/i915: Only dither on 6bpc panels"). This is decided
at the end of intel_modeset_pipe_config().

The big question here is if we want to expand the use of dithering. I
guess we could be able to reduce banding if we did?

> Signed-off-by: Nischal Varide <nischal.varide@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c   |  7 +++++++
>  drivers/gpu/drm/i915/display/intel_display.c | 11 ++++++++++-
>  drivers/gpu/drm/i915/i915_reg.h              |  1 +
>  3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index dab892d2251b..c7af583200c4 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1574,6 +1574,7 @@ static int glk_color_check(struct intel_crtc_state *crtc_state)
>  static u32 icl_gamma_mode(const struct intel_crtc_state *crtc_state)
>  {
>  	u32 gamma_mode = 0;
> +	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
>  
>  	if (crtc_state->hw.degamma_lut)
>  		gamma_mode |= PRE_CSC_GAMMA_ENABLE;
> @@ -1588,6 +1589,12 @@ static u32 icl_gamma_mode(const struct intel_crtc_state *crtc_state)
>  	else
>  		gamma_mode |= GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED;
>  
> +	if (DISPLAY_VER(i915) >= 13) {
> +		if (!crtc_state->dither_force_disable &&
> +				(crtc_state->pipe_bpp == 36))
> +			gamma_mode |= POST_CC1_DITHER_ENABLE;
> +	}

This enables dithering independent of crtc_state->dither. That doesn't
seem like a good idea. I think the decision should be made at the end of
intel_modeset_pipe_config().

	if (DISPLAY_VER(i915) >= 13 && crtc_state->dither &&
	    crtc_state->pipe_bpp == 36)
		gamma_mode |= POST_CC1_DITHER_ENABLE;

Obviously, as we currently only enable dithering for 6 bpc, this would
become a nop if it looked at crtc_state->dither and pipe_bpp only.

> +
>  	return gamma_mode;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 362bff9beb5c..3a7feb246745 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -5762,7 +5762,16 @@ static void bdw_set_pipemisc(const struct intel_crtc_state *crtc_state)
>  		break;
>  	}
>  
> -	if (crtc_state->dither)
> +	/*
> +	 * If 12bpc panel then, Enables dithering after the CC1 pipe
> +	 * post color space conversion and not here for display_ver
> +	 * greater than or equal to thirteen.
> +	 */
> +
> +	if (crtc_state->dither && (crtc_state->pipe_bpp != 36))
> +		val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
> +
> +	if (crtc_state->dither && (crtc_state->pipe_bpp == 36) && (DISPLAY_VER(dev_priv) < 13))
>  		val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;

This is what you're trying to say:

	/* 12 bpc dithering is done at post CSC gamma for display 13+ */
	if (crtc_state->dither &&
	    (crtc_state->pipe_bpp != 36 || DISPLAY_VER(dev_priv) < 13))
		val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;

Again, this is a nop until we decide to use dithering more.

>  
>  	if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index e915ec034c98..33dba13fa94d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7743,6 +7743,7 @@ enum {
>  #define GAMMA_MODE(pipe) _MMIO_PIPE(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B)
>  #define  PRE_CSC_GAMMA_ENABLE	(1 << 31)
>  #define  POST_CSC_GAMMA_ENABLE	(1 << 30)
> +#define  POST_CC1_DITHER_ENABLE (1 << 26)
>  #define  GAMMA_MODE_MODE_MASK	(3 << 0)
>  #define  GAMMA_MODE_MODE_8BIT	(0 << 0)
>  #define  GAMMA_MODE_MODE_10BIT	(1 << 0)

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx