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

Tue Jun 1 12:13:11 UTC 2021

-----Original Message-----
From: Modem, Bhanuprakash <bhanuprakash.modem at intel.com> 
Sent: Wednesday, May 26, 2021 9:29 PM
To: Jani Nikula <jani.nikula at linux.intel.com>; intel-gfx at lists.freedesktop.org; Varide, Nischal <nischal.varide at intel.com>; Shankar, Uma <uma.shankar at intel.com>; Gupta, Anshuman <anshuman.gupta at intel.com>
Subject: RE: [Intel-gfx] [PATCH 1/2] drm/i915/xelpd: Enabling dithering after the CC1

> From: Jani Nikula <jani.nikula at linux.intel.com>
> Sent: Wednesday, May 26, 2021 7:34 PM
> To: Modem, Bhanuprakash <bhanuprakash.modem at intel.com>; intel- 
> gfx at lists.freedesktop.org; Varide, Nischal <nischal.varide at intel.com>; 
> Shankar, Uma <uma.shankar at intel.com>; Gupta, Anshuman 
> <anshuman.gupta at intel.com>
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/xelpd: Enabling 
> dithering after the CC1
> 
> 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.

Patch 2/2 in this series has a dependency on this patch. And I haven't made any changes in this patch, so not added my Signed-off-by :-)

- Bhanu

> 
> > ---
> >  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().

Yes ,agree and will do the needful here.

> 
> >  	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.
There are two bits for controlling the dithering one at pipe level and other at gamma level, 
So the checks at two places .

> >  		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.
Yes..Noted

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

Yes..can change the name to GAMMA_DITHER_AFTER_CC1
> >
> >  /* DMC */
> >  #define DMC_PROGRAM(i)		_MMIO(0x80000 + (i) * 4)
> 
> --
> Jani Nikula, Intel Open Source Graphics Center