[1/2] drm/i915/xelpd: Enabling dithering after the CC1

Commit Message

Modem, Bhanuprakash May 26, 2021, 6:17 p.m. UTC

From: Nischal Varide <[e-mail]>

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 <[e-mail]>
---
 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(-)

Comments

Jani Nikula May 26, 2021, 2:04 p.m. UTC | #1

On Wed, 26 May 2021, Bhanuprakash Modem <[e-mail]> wrote:
> From: Nischal Varide <[e-mail]>
>
> 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 <[e-mail]>

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)

Modem, Bhanuprakash May 26, 2021, 3:58 p.m. UTC | #2

> From: Jani Nikula <[e-mail]>
> Sent: Wednesday, May 26, 2021 7:34 PM
> To: Modem, Bhanuprakash <[e-mail]>; intel-
> [e-mail]; Varide, Nischal <[e-mail]>;
> Shankar, Uma <[e-mail]>; Gupta, Anshuman
> <[e-mail]>
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/xelpd: Enabling dithering after
> the CC1
> 
> On Wed, 26 May 2021, Bhanuprakash Modem <[e-mail]> wrote:
> > From: Nischal Varide <[e-mail]>
> >
> > 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 <[e-mail]>
> 
> 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().
> 
> >  	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

Jani Nikula May 26, 2021, 4:11 p.m. UTC | #3

On Wed, 26 May 2021, "Modem, Bhanuprakash" <[e-mail]> wrote:
>> From: Jani Nikula <[e-mail]>
>> 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 :-)

Signed-off-by isn't about changes.

https://developercertificate.org/

BR,
Jani.

Nischal Varide June 1, 2021, 12:13 p.m. UTC | #4

-----Original Message-----
From: Modem, Bhanuprakash <[e-mail]> 
Sent: Wednesday, May 26, 2021 9:29 PM
To: Jani Nikula <[e-mail]>; [e-mail]; Varide, Nischal <[e-mail]>; Shankar, Uma <[e-mail]>; Gupta, Anshuman <[e-mail]>
Subject: RE: [Intel-gfx] [PATCH 1/2] drm/i915/xelpd: Enabling dithering after the CC1

> From: Jani Nikula <[e-mail]>
> Sent: Wednesday, May 26, 2021 7:34 PM
> To: Modem, Bhanuprakash <[e-mail]>; intel- 
> [e-mail]; Varide, Nischal <[e-mail]>; 
> Shankar, Uma <[e-mail]>; Gupta, Anshuman 
> <[e-mail]>
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/xelpd: Enabling 
> dithering after the CC1
> 
> On Wed, 26 May 2021, Bhanuprakash Modem <[e-mail]> wrote:
> > From: Nischal Varide <[e-mail]>
> >
> > 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 <[e-mail]>
> 
> 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

Nischal Varide June 2, 2021, 12:47 p.m. UTC | #5

-----Original Message-----
From: Modem, Bhanuprakash <[e-mail]>
Sent: Wednesday, May 26, 2021 9:29 PM
To: Jani Nikula <[e-mail]>; [e-mail]; Varide, Nischal <[e-mail]>; Shankar, Uma <[e-mail]>; Gupta, Anshuman <[e-mail]>
Subject: RE: [Intel-gfx] [PATCH 1/2] drm/i915/xelpd: Enabling dithering after the CC1

> From: Jani Nikula <[e-mail]>
> Sent: Wednesday, May 26, 2021 7:34 PM
> To: Modem, Bhanuprakash <[e-mail]>; intel- 
> [e-mail]; Varide, Nischal <[e-mail]>; 
> Shankar, Uma <[e-mail]>; Gupta, Anshuman 
> <[e-mail]>
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/xelpd: Enabling 
> dithering after the CC1
> 
> On Wed, 26 May 2021, Bhanuprakash Modem <[e-mail]> wrote:
> > From: Nischal Varide <[e-mail]>
> >
> > 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 <[e-mail]>
> 
> 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?
 This is by referring a wrong register in bspec, not needed and removing it
> >  #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

Patch

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);
 	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))
 		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)
 #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)
 
 /* DMC */
 #define DMC_PROGRAM(i)		_MMIO(0x80000 + (i) * 4)