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

[Jani Nikula]

On Wed, 26 May 2021, Bhanuprakash Modem <bhanuprakash.modem at intel.com> wrote:
> From: Nischal Varide <nischal.varide at intel.com>
>
> 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.
>
> Signed-off-by: Nischal Varide <nischal.varide at intel.com>

When you're sending someone else's patches, you need to add your own
Signed-off-by here.

> ---
>  drivers/gpu/drm/i915/display/intel_color.c   | 15 +++++++++++++++
>  drivers/gpu/drm/i915/display/intel_display.c |  7 ++++++-
>  drivers/gpu/drm/i915/i915_reg.h              |  3 ++-
>  3 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index dab892d2251b..4ad5bd849695 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1605,6 +1605,20 @@ static u32 icl_csc_mode(const struct intel_crtc_state *crtc_state)
>  	return csc_mode;
>  }
>  
> +static u32 dither_after_cc1_12bpc(const struct intel_crtc_state *crtc_state)
> +{
> +	u32 gamma_mode = crtc_state->gamma_mode;
> +	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> +
> +	if (DISPLAY_VER(i915) >= 13) {
> +		if (!crtc_state->dither_force_disable &&
> +		    (crtc_state->pipe_bpp == 36))
> +			gamma_mode |= GAMMA_MODE_DITHER_AFTER_CC1;
> +	}
> +
> +	return gamma_mode;
> +}
> +
>  static int icl_color_check(struct intel_crtc_state *crtc_state)
>  {
>  	int ret;
> @@ -1615,6 +1629,7 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
>  
>  	crtc_state->gamma_mode = icl_gamma_mode(crtc_state);
>  
> +	crtc_state->gamma_mode = dither_after_cc1_12bpc(crtc_state);

We don't really do the kind of thing where you need a sequence of calls
where one depends on the other, adding to the same state member. At a
glance, this just looks wrong, superficially overwriting the previously
set value. I'd just add the check at the end of icl_gamma_mode().

>  	crtc_state->csc_mode = icl_csc_mode(crtc_state);
>  
>  	crtc_state->preload_luts = intel_can_preload_luts(crtc_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 0bb2e582c87f..1a658bdaeab6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -5741,7 +5741,12 @@ 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
> +	 */
> +
> +	if (crtc_state->dither && (crtc_state->pipe_bpp != 36))

This now duplicates the pipe_bpp condition in two places, which seems a
bit fragile. Maybe the check should be on gamma_mode? It would remove
the need for the whole comment above.

>  		val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
>  
>  	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 4dbe79009c0e..5700097475c0 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6155,7 +6155,7 @@ enum {
>  #define   PIPEMISC_DITHER_8_BPC		(0 << 5)
>  #define   PIPEMISC_DITHER_10_BPC	(1 << 5)
>  #define   PIPEMISC_DITHER_6_BPC		(2 << 5)
> -#define   PIPEMISC_DITHER_12_BPC	(3 << 5)
> +#define   PIPEMISC_DITHER_12_BPC	(4 << 5)

We already use the macro. You can't just replace this like this without
an explanation. Why would this not break existing stuff?

>  #define   PIPEMISC_DITHER_ENABLE	(1 << 4)
>  #define   PIPEMISC_DITHER_TYPE_MASK	(3 << 2)
>  #define   PIPEMISC_DITHER_TYPE_SP	(0 << 2)
> @@ -7726,6 +7726,7 @@ enum {
>  #define  GAMMA_MODE_MODE_12BIT	(2 << 0)
>  #define  GAMMA_MODE_MODE_SPLIT	(3 << 0) /* ivb-bdw */
>  #define  GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED	(3 << 0) /* icl + */
> +#define  GAMMA_MODE_DITHER_AFTER_CC1 (1 << 26)

The bits are supposed to be defined in the order from highest to lowest
bit. See the big comment at the beginning of the file.

It's confusing that this is named GAMMA_MODE_ while it's not included in
GAMMA_MODE_MASK (and likely shouldn't be).

>  
>  /* DMC */
>  #define DMC_PROGRAM(i)		_MMIO(0x80000 + (i) * 4)

-- 
Jani Nikula, Intel Open Source Graphics Center