[18/18] drm/i915/display13: Enabling dithering after the CC1 pipe

Commit Message

Matt Roper Jan. 28, 2021, 7:24 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.

Cc: Uma Shankar <[e-mail]>
Signed-off-by: Nischal Varide <[e-mail]>
Signed-off-by: Bhanuprakash Modem <[e-mail]>
Signed-off-by: Matt Roper <[e-mail]>
---
 drivers/gpu/drm/i915/display/intel_color.c   | 16 ++++++++++++++++
 drivers/gpu/drm/i915/display/intel_display.c |  9 ++++++++-
 drivers/gpu/drm/i915/i915_reg.h              |  3 ++-
 3 files changed, 26 insertions(+), 2 deletions(-)

Comments

Ville Syrjälä Feb. 11, 2021, 12:29 p.m. UTC | #1

On Thu, Jan 28, 2021 at 11:24:13AM -0800, Matt Roper 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.

Dithering is probably going to require a whole uapi bikeshed.
Not sure we can just enable it unilaterally.

Ccing dri-devel, and Mario who had issues with dithering in the
past...

> 
> Cc: Uma Shankar <[e-mail]>
> Signed-off-by: Nischal Varide <[e-mail]>
> Signed-off-by: Bhanuprakash Modem <[e-mail]>
> Signed-off-by: Matt Roper <[e-mail]>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c   | 16 ++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_display.c |  9 ++++++++-
>  drivers/gpu/drm/i915/i915_reg.h              |  3 ++-
>  3 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index ff7dcb7088bf..9a0572bbc5db 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1604,6 +1604,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 (HAS_DISPLAY13(i915)) {
> +		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;
> @@ -1614,6 +1628,8 @@ 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 4dc4b1be0809..e3dbcd956fc6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -8098,9 +8098,15 @@ 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 ||
>  	    crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444)
>  		val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
> @@ -10760,6 +10766,7 @@ intel_modeset_pipe_config(struct intel_atomic_state *state,
>  	 */
>  	pipe_config->dither = (pipe_config->pipe_bpp == 6*3) &&
>  		!pipe_config->dither_force_disable;
> +
>  	drm_dbg_kms(&i915->drm,
>  		    "hw max bpp: %i, pipe bpp: %i, dithering: %i\n",
>  		    base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 128b835c0adb..27f25214a839 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6132,7 +6132,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)
> @@ -7668,6 +7668,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/CSR */
>  #define CSR_PROGRAM(i)		_MMIO(0x80000 + (i) * 4)
> -- 
> 2.25.4
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Mario Kleiner Feb. 19, 2021, 3:22 a.m. UTC | #2

On Thu, Feb 11, 2021 at 1:29 PM Ville Syrjälä <[e-mail]>
wrote:

> On Thu, Jan 28, 2021 at 11:24:13AM -0800, Matt Roper 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.
>
> Dithering is probably going to require a whole uapi bikeshed.
> Not sure we can just enable it unilaterally.
>
> Ccing dri-devel, and Mario who had issues with dithering in the
> past...
>
> Thanks for the cc Ville!

The problem with dithering on Intel is that various tested Intel gpu's
(Ironlake, IvyBridge, Haswell, Skylake iirc.) are dithering when they
shouldn't. If one has a standard 8 bpc framebuffer feeding into a standard
(legacy) 256 slots, 8 bit wide lut which was loaded with an identity
mapping, feeding into a standard 8 bpc video output (DVI/HDMI/DP), the
expected result is that pixels rendered into the framebuffer show up
unmodified at the video output. What happens instead is that some dithering
is needlessly applied. This is bad for various neuroscience/medical
research equipment that requires pixels to pass unmodified in a pure 8 bpc
configuration, e.g., because some digital info is color-encoded in-band in
the rendered image to control research hardware, a la "if rgb pixel (123,
12, 23) is detected in the digital video stream, emit some trigger signal,
or timestamp that moment with a hw clock, or start or stop some scientific
recording equipment". Also there exist specialized visual stimulators to
drive special displays with more than 12 bpc, e.g., 16 bpc, and so they
encode the 8MSB of 16 bpc color values in pixels in even columns, and the
8LSB in the odd columns of the framebuffer. Unexpected dithering makes such
equipment completely unusable. By now I must have spent months of my life,
just trying to deal with dithering induced problems on different gpu's due
to hw quirks or bugs somewhere in the graphics stack.

Atm. the intel kms driver disables dithering for anything with >= 8 bpc as
a fix for this harmful hardware quirk.

Ideally we'd have uapi that makes dithering controllable per connector
(on/off/auto, selectable depth), also in a way that those controls are
exposed as RandR output properties, easily controllable by X clients. And
some safe default in case the client can't access the properties (like I'd
expect to happen with the dozens of Wayland compositors under the sun).
Various drivers had this over time, e.g., AMD classic kms path (if i don't
misremember) and nouveau, but some of it also got lost in the new atomic
kms variants, and Intel never exposed this.

Or maybe some method that checks the values actually stored in the hw
lut's, CTM etc. and if the values suggest no dithering should be needed,
disable the dithering. E.g., if output depth is 8 bpc, one only needs
dithering if the slots in the final active hw lut do have any meaningful
values in the lower bits below the top 8 MSB, ie. if the content is
actually > 8 bpc net bit depth.

-mario

>
> > Cc: Uma Shankar <[e-mail]>
> > Signed-off-by: Nischal Varide <[e-mail]>
> > Signed-off-by: Bhanuprakash Modem <[e-mail]>
> > Signed-off-by: Matt Roper <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/display/intel_color.c   | 16 ++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_display.c |  9 ++++++++-
> >  drivers/gpu/drm/i915/i915_reg.h              |  3 ++-
> >  3 files changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> > index ff7dcb7088bf..9a0572bbc5db 100644
> > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > @@ -1604,6 +1604,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 (HAS_DISPLAY13(i915)) {
> > +             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;
> > @@ -1614,6 +1628,8 @@ 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 4dc4b1be0809..e3dbcd956fc6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -8098,9 +8098,15 @@ 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 ||
> >           crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444)
> >               val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
> > @@ -10760,6 +10766,7 @@ intel_modeset_pipe_config(struct
> intel_atomic_state *state,
> >        */
> >       pipe_config->dither = (pipe_config->pipe_bpp == 6*3) &&
> >               !pipe_config->dither_force_disable;
> > +
> >       drm_dbg_kms(&i915->drm,
> >                   "hw max bpp: %i, pipe bpp: %i, dithering: %i\n",
> >                   base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> > index 128b835c0adb..27f25214a839 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6132,7 +6132,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)
> > @@ -7668,6 +7668,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/CSR */
> >  #define CSR_PROGRAM(i)               _MMIO(0x80000 + (i) * 4)
> > --
> > 2.25.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel
>

Mario Kleiner Feb. 19, 2021, 5:44 a.m. UTC | #3

On Fri, Feb 19, 2021 at 4:22 AM Mario Kleiner <[e-mail]>
wrote:

>
>
> On Thu, Feb 11, 2021 at 1:29 PM Ville Syrjälä <
> [e-mail]> wrote:
>
>> On Thu, Jan 28, 2021 at 11:24:13AM -0800, Matt Roper 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.
>>
>> Dithering is probably going to require a whole uapi bikeshed.
>> Not sure we can just enable it unilaterally.
>>
>> Ccing dri-devel, and Mario who had issues with dithering in the
>> past...
>>
>> Thanks for the cc Ville!
>
> The problem with dithering on Intel is that various tested Intel gpu's
> (Ironlake, IvyBridge, Haswell, Skylake iirc.) are dithering when they
> shouldn't. If one has a standard 8 bpc framebuffer feeding into a standard
> (legacy) 256 slots, 8 bit wide lut which was loaded with an identity
> mapping, feeding into a standard 8 bpc video output (DVI/HDMI/DP), the
> expected result is that pixels rendered into the framebuffer show up
> unmodified at the video output. What happens instead is that some dithering
> is needlessly applied. This is bad for various neuroscience/medical
> research equipment that requires pixels to pass unmodified in a pure 8 bpc
> configuration, e.g., because some digital info is color-encoded in-band in
> the rendered image to control research hardware, a la "if rgb pixel (123,
> 12, 23) is detected in the digital video stream, emit some trigger signal,
> or timestamp that moment with a hw clock, or start or stop some scientific
> recording equipment". Also there exist specialized visual stimulators to
> drive special displays with more than 12 bpc, e.g., 16 bpc, and so they
> encode the 8MSB of 16 bpc color values in pixels in even columns, and the
> 8LSB in the odd columns of the framebuffer. Unexpected dithering makes such
> equipment completely unusable. By now I must have spent months of my life,
> just trying to deal with dithering induced problems on different gpu's due
> to hw quirks or bugs somewhere in the graphics stack.
>
> Atm. the intel kms driver disables dithering for anything with >= 8 bpc as
> a fix for this harmful hardware quirk.
>
> Ideally we'd have uapi that makes dithering controllable per connector
> (on/off/auto, selectable depth), also in a way that those controls are
> exposed as RandR output properties, easily controllable by X clients. And
> some safe default in case the client can't access the properties (like I'd
> expect to happen with the dozens of Wayland compositors under the sun).
> Various drivers had this over time, e.g., AMD classic kms path (if i don't
> misremember) and nouveau, but some of it also got lost in the new atomic
> kms variants, and Intel never exposed this.
>
> Or maybe some method that checks the values actually stored in the hw
> lut's, CTM etc. and if the values suggest no dithering should be needed,
> disable the dithering. E.g., if output depth is 8 bpc, one only needs
> dithering if the slots in the final active hw lut do have any meaningful
> values in the lower bits below the top 8 MSB, ie. if the content is
> actually > 8 bpc net bit depth.
>
> -mario
>
>
One cup of coffee later... I think this specific patch should be ok wrt. my
use cases. The majority of the above mentioned research devices are
single/dual-link DVI receivers, ie. 8 bpc video sinks. I'm only aware of
one recent device that has a DisplayPort receiver who could act as a > 8
bpc video sink. See the following link for advanced examples of such
devices: https://vpixx.com/our-products/video-i-o-hub/

I cannot think of a use case that would require more than 8 bits for inband
signalling given that that was good enough for the last 20 years, or for
encoding very high color precision content -- the 16 bpc precision that one
can get out of the current even/odd pixel = 8 MSB + 8 LSB encoding scheme
should be enough for the foreseeable future. Therefore dithering shouldn't
pose a problem if it leaves the 8 MSB of each pixel color component intact,
and spatial dithering as employed here usually only touches the least
significant bit (or maybe the 2 LSB's?).

As this patch only enables dithering on 12 bpc video sinks, if i understand
pipe_bpp correctly, it could only "corrupt" one bit and leave at least the
10-11 MSB's intact, right?

pipe_bpp == 24 is the case that would really hurt a lot of researchers if
dithering would be enabled without providing good uapi or other mechanisms
to prevent it.

So:

Acked-by: Mario Kleiner <[e-mail]>

One suggestion: It would be good to also add a bit of drm_dbg_kms() logging
to the new code-patch, so that this 12 bpc dithering enable on
HAS_DISPLAY13 hw also shows up in the logs, not just the standard 6 bpc
enable. Helped a lot in debugging dithering issues if there was a reliable
trace in the logs of what was active when. One suggestion for that inside
your patch below...

>
>> > Cc: Uma Shankar <[e-mail]>
>> > Signed-off-by: Nischal Varide <[e-mail]>
>> > Signed-off-by: Bhanuprakash Modem <[e-mail]>
>> > Signed-off-by: Matt Roper <[e-mail]>
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_color.c   | 16 ++++++++++++++++
>> >  drivers/gpu/drm/i915/display/intel_display.c |  9 ++++++++-
>> >  drivers/gpu/drm/i915/i915_reg.h              |  3 ++-
>> >  3 files changed, 26 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_color.c
>> b/drivers/gpu/drm/i915/display/intel_color.c
>> > index ff7dcb7088bf..9a0572bbc5db 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_color.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_color.c
>> > @@ -1604,6 +1604,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 (HAS_DISPLAY13(i915)) {
>> > +             if (!crtc_state->dither_force_disable &&
>>
>
Replace  !crtc_state->dither_force_disable by crtc_state->dither

> > +                 (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;
>> > @@ -1614,6 +1628,8 @@ 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 4dc4b1be0809..e3dbcd956fc6 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> > @@ -8098,9 +8098,15 @@ 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 ||
>> >           crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444)
>> >               val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
>> > @@ -10760,6 +10766,7 @@ intel_modeset_pipe_config(struct
>> intel_atomic_state *state,
>> >        */
>>
>
Instead of...


> >       pipe_config->dither = (pipe_config->pipe_bpp == 6*3) &&
>> >               !pipe_config->dither_force_disable;
>> > +
>>
>
... use ...

>       pipe_config->dither = ((pipe_config->pipe_bpp == 6*3) ||
>> (HAS_DISPLAY13(i915) && pipe_config->pipe_bpp == 12*3)) &&
>> !pipe_config->dither_force_disable;
>>
>
... so that the dither enable/disable decision and logging happens in one
location in the code?

>       drm_dbg_kms(&i915->drm,
>> >                   "hw max bpp: %i, pipe bpp: %i, dithering: %i\n",
>> >                   base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
>>
>
Thanks,
-mario




> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h
>> > index 128b835c0adb..27f25214a839 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -6132,7 +6132,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)
>> > @@ -7668,6 +7668,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/CSR */
>> >  #define CSR_PROGRAM(i)               _MMIO(0x80000 + (i) * 4)
>> > --
>> > 2.25.4
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > [e-mail]
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> --
>> Ville Syrjälä
>> Intel
>>
>

Nischal Varide March 1, 2021, 4:57 a.m. UTC | #4

Looks like there are two options.

  1.  Enable or Disable Dithering via kernel command line or sysfs.
  2.  To implement new Uapi.
May be the first one is more feasible and faster

Regards
Nischal

From: Mario Kleiner <[e-mail]>
Sent: Friday, February 19, 2021 11:15 AM
To: Ville Syrjälä <[e-mail]>
Cc: Roper, Matthew D <[e-mail]>; intel-gfx <[e-mail]>; Varide, Nischal <[e-mail]>; dri-devel <[e-mail]>
Subject: Re: [Intel-gfx] [PATCH 18/18] drm/i915/display13: Enabling dithering after the CC1 pipe



On Fri, Feb 19, 2021 at 4:22 AM Mario Kleiner <[e-mail]<mailto:[e-mail]>> wrote:


On Thu, Feb 11, 2021 at 1:29 PM Ville Syrjälä <[e-mail]<mailto:[e-mail]>> wrote:
On Thu, Jan 28, 2021 at 11:24:13AM -0800, Matt Roper wrote:
> From: Nischal Varide <[e-mail]<mailto:[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.

Dithering is probably going to require a whole uapi bikeshed.
Not sure we can just enable it unilaterally.

Ccing dri-devel, and Mario who had issues with dithering in the
past...
Thanks for the cc Ville!

The problem with dithering on Intel is that various tested Intel gpu's (Ironlake, IvyBridge, Haswell, Skylake iirc.) are dithering when they shouldn't. If one has a standard 8 bpc framebuffer feeding into a standard (legacy) 256 slots, 8 bit wide lut which was loaded with an identity mapping, feeding into a standard 8 bpc video output (DVI/HDMI/DP), the expected result is that pixels rendered into the framebuffer show up unmodified at the video output. What happens instead is that some dithering is needlessly applied. This is bad for various neuroscience/medical research equipment that requires pixels to pass unmodified in a pure 8 bpc configuration, e.g., because some digital info is color-encoded in-band in the rendered image to control research hardware, a la "if rgb pixel (123, 12, 23) is detected in the digital video stream, emit some trigger signal, or timestamp that moment with a hw clock, or start or stop some scientific recording equipment". Also there exist specialized visual stimulators to drive special displays with more than 12 bpc, e.g., 16 bpc, and so they encode the 8MSB of 16 bpc color values in pixels in even columns, and the 8LSB in the odd columns of the framebuffer. Unexpected dithering makes such equipment completely unusable. By now I must have spent months of my life, just trying to deal with dithering induced problems on different gpu's due to hw quirks or bugs somewhere in the graphics stack.

Atm. the intel kms driver disables dithering for anything with >= 8 bpc as a fix for this harmful hardware quirk.

Ideally we'd have uapi that makes dithering controllable per connector (on/off/auto, selectable depth), also in a way that those controls are exposed as RandR output properties, easily controllable by X clients. And some safe default in case the client can't access the properties (like I'd expect to happen with the dozens of Wayland compositors under the sun). Various drivers had this over time, e.g., AMD classic kms path (if i don't misremember) and nouveau, but some of it also got lost in the new atomic kms variants, and Intel never exposed this.

Or maybe some method that checks the values actually stored in the hw lut's, CTM etc. and if the values suggest no dithering should be needed, disable the dithering. E.g., if output depth is 8 bpc, one only needs dithering if the slots in the final active hw lut do have any meaningful values in the lower bits below the top 8 MSB, ie. if the content is actually > 8 bpc net bit depth.

-mario


One cup of coffee later... I think this specific patch should be ok wrt. my use cases. The majority of the above mentioned research devices are single/dual-link DVI receivers, ie. 8 bpc video sinks. I'm only aware of one recent device that has a DisplayPort receiver who could act as a > 8 bpc video sink. See the following link for advanced examples of such devices: https://vpixx.com/our-products/video-i-o-hub/

I cannot think of a use case that would require more than 8 bits for inband signalling given that that was good enough for the last 20 years, or for encoding very high color precision content -- the 16 bpc precision that one can get out of the current even/odd pixel = 8 MSB + 8 LSB encoding scheme should be enough for the foreseeable future. Therefore dithering shouldn't pose a problem if it leaves the 8 MSB of each pixel color component intact, and spatial dithering as employed here usually only touches the least significant bit (or maybe the 2 LSB's?).

As this patch only enables dithering on 12 bpc video sinks, if i understand pipe_bpp correctly, it could only "corrupt" one bit and leave at least the 10-11 MSB's intact, right?

pipe_bpp == 24 is the case that would really hurt a lot of researchers if dithering would be enabled without providing good uapi or other mechanisms to prevent it.

So:

Acked-by: Mario Kleiner <[e-mail]<mailto:[e-mail]>>

One suggestion: It would be good to also add a bit of drm_dbg_kms() logging to the new code-patch, so that this 12 bpc dithering enable on HAS_DISPLAY13 hw also shows up in the logs, not just the standard 6 bpc enable. Helped a lot in debugging dithering issues if there was a reliable trace in the logs of what was active when. One suggestion for that inside your patch below...

>
> Cc: Uma Shankar <[e-mail]<mailto:[e-mail]>>
> Signed-off-by: Nischal Varide <[e-mail]<mailto:[e-mail]>>
> Signed-off-by: Bhanuprakash Modem <[e-mail]<mailto:[e-mail]>>
> Signed-off-by: Matt Roper <[e-mail]<mailto:[e-mail]>>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c   | 16 ++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_display.c |  9 ++++++++-
>  drivers/gpu/drm/i915/i915_reg.h              |  3 ++-
>  3 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index ff7dcb7088bf..9a0572bbc5db 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1604,6 +1604,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 (HAS_DISPLAY13(i915)) {
> +             if (!crtc_state->dither_force_disable &&

Replace  !crtc_state->dither_force_disable by crtc_state->dither
> +                 (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;
> @@ -1614,6 +1628,8 @@ 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 4dc4b1be0809..e3dbcd956fc6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -8098,9 +8098,15 @@ 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 ||
>           crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444)
>               val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
> @@ -10760,6 +10766,7 @@ intel_modeset_pipe_config(struct intel_atomic_state *state,
>        */

Instead of...

>       pipe_config->dither = (pipe_config->pipe_bpp == 6*3) &&
>               !pipe_config->dither_force_disable;
> +

... use ...

>       pipe_config->dither = ((pipe_config->pipe_bpp == 6*3) || (HAS_DISPLAY13(i915) && pipe_config->pipe_bpp == 12*3)) && !pipe_config->dither_force_disable;

... so that the dither enable/disable decision and logging happens in one location in the code?

>       drm_dbg_kms(&i915->drm,
>                   "hw max bpp: %i, pipe bpp: %i, dithering: %i\n",
>                   base_bpp, pipe_config->pipe_bpp, pipe_config->dither);

Thanks,
-mario



> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 128b835c0adb..27f25214a839 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6132,7 +6132,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)
> @@ -7668,6 +7668,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/CSR */
>  #define CSR_PROGRAM(i)               _MMIO(0x80000 + (i) * 4)
> --
> 2.25.4
>
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]<mailto:[e-mail]>
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Ville Syrjälä
Intel

Ilia Mirkin March 1, 2021, 5:43 a.m. UTC | #5

Just wanted to mention ... nouveau supports two separate properties,
one controlling the type of dithering, and the other the dithering
depth:

        dithering depth: auto
                supported: auto, 6 bpc, 8 bpc
        dithering mode: auto
                supported: auto, off, static 2x2, dynamic 2x2, temporal

I think these are the properties Mario was alluding to. If I have an
8bpc buffer and set the dithering depth to 6bpc, it will dither down
to this. This is useful for LVDS panels primarily. Sometimes we know
it's 6bpc (and "auto" works), sometimes we don't.

These properties are largely a reflection of how the hardware works.
For example,

https://nvidia.github.io/open-gpu-doc/classes/display/cl917d.h
search for SET_DITHER_CONTROL.

Perhaps this "API" would not be appropriate for Intel HW, not sure.
But there's definitely precedent.

Cheers,

  -ilia

On Sun, Feb 28, 2021 at 11:57 PM Varide, Nischal
<[e-mail]> wrote:
>
> Looks like there are two options.
>
> Enable or Disable Dithering via kernel command line or sysfs.
> To implement new Uapi.
>
> May be the first one is more feasible and faster
>
>
>
> Regards
>
> Nischal
>
>
>
> From: Mario Kleiner <[e-mail]>
> Sent: Friday, February 19, 2021 11:15 AM
> To: Ville Syrjälä <[e-mail]>
> Cc: Roper, Matthew D <[e-mail]>; intel-gfx <[e-mail]>; Varide, Nischal <[e-mail]>; dri-devel <[e-mail]>
> Subject: Re: [Intel-gfx] [PATCH 18/18] drm/i915/display13: Enabling dithering after the CC1 pipe
>
>
>
>
>
>
>
> On Fri, Feb 19, 2021 at 4:22 AM Mario Kleiner <[e-mail]> wrote:
>
>
>
>
>
> On Thu, Feb 11, 2021 at 1:29 PM Ville Syrjälä <[e-mail]> wrote:
>
> On Thu, Jan 28, 2021 at 11:24:13AM -0800, Matt Roper 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.
>
> Dithering is probably going to require a whole uapi bikeshed.
> Not sure we can just enable it unilaterally.
>
> Ccing dri-devel, and Mario who had issues with dithering in the
> past...
>
> Thanks for the cc Ville!
>
>
>
> The problem with dithering on Intel is that various tested Intel gpu's (Ironlake, IvyBridge, Haswell, Skylake iirc.) are dithering when they shouldn't. If one has a standard 8 bpc framebuffer feeding into a standard (legacy) 256 slots, 8 bit wide lut which was loaded with an identity mapping, feeding into a standard 8 bpc video output (DVI/HDMI/DP), the expected result is that pixels rendered into the framebuffer show up unmodified at the video output. What happens instead is that some dithering is needlessly applied. This is bad for various neuroscience/medical research equipment that requires pixels to pass unmodified in a pure 8 bpc configuration, e.g., because some digital info is color-encoded in-band in the rendered image to control research hardware, a la "if rgb pixel (123, 12, 23) is detected in the digital video stream, emit some trigger signal, or timestamp that moment with a hw clock, or start or stop some scientific recording equipment". Also there exist specialized visual stimulators to drive special displays with more than 12 bpc, e.g., 16 bpc, and so they encode the 8MSB of 16 bpc color values in pixels in even columns, and the 8LSB in the odd columns of the framebuffer. Unexpected dithering makes such equipment completely unusable. By now I must have spent months of my life, just trying to deal with dithering induced problems on different gpu's due to hw quirks or bugs somewhere in the graphics stack.
>
>
>
> Atm. the intel kms driver disables dithering for anything with >= 8 bpc as a fix for this harmful hardware quirk.
>
>
>
> Ideally we'd have uapi that makes dithering controllable per connector (on/off/auto, selectable depth), also in a way that those controls are exposed as RandR output properties, easily controllable by X clients. And some safe default in case the client can't access the properties (like I'd expect to happen with the dozens of Wayland compositors under the sun). Various drivers had this over time, e.g., AMD classic kms path (if i don't misremember) and nouveau, but some of it also got lost in the new atomic kms variants, and Intel never exposed this.
>
>
>
> Or maybe some method that checks the values actually stored in the hw lut's, CTM etc. and if the values suggest no dithering should be needed, disable the dithering. E.g., if output depth is 8 bpc, one only needs dithering if the slots in the final active hw lut do have any meaningful values in the lower bits below the top 8 MSB, ie. if the content is actually > 8 bpc net bit depth.
>
>
>
> -mario
>
>
>
>
>
> One cup of coffee later... I think this specific patch should be ok wrt. my use cases. The majority of the above mentioned research devices are single/dual-link DVI receivers, ie. 8 bpc video sinks. I'm only aware of one recent device that has a DisplayPort receiver who could act as a > 8 bpc video sink. See the following link for advanced examples of such devices: https://vpixx.com/our-products/video-i-o-hub/
>
>
>
> I cannot think of a use case that would require more than 8 bits for inband signalling given that that was good enough for the last 20 years, or for encoding very high color precision content -- the 16 bpc precision that one can get out of the current even/odd pixel = 8 MSB + 8 LSB encoding scheme should be enough for the foreseeable future. Therefore dithering shouldn't pose a problem if it leaves the 8 MSB of each pixel color component intact, and spatial dithering as employed here usually only touches the least significant bit (or maybe the 2 LSB's?).
>
>
>
> As this patch only enables dithering on 12 bpc video sinks, if i understand pipe_bpp correctly, it could only "corrupt" one bit and leave at least the 10-11 MSB's intact, right?
>
>
>
> pipe_bpp == 24 is the case that would really hurt a lot of researchers if dithering would be enabled without providing good uapi or other mechanisms to prevent it.
>
>
>
> So:
>
>
>
> Acked-by: Mario Kleiner <[e-mail]>
>
>
>
> One suggestion: It would be good to also add a bit of drm_dbg_kms() logging to the new code-patch, so that this 12 bpc dithering enable on HAS_DISPLAY13 hw also shows up in the logs, not just the standard 6 bpc enable. Helped a lot in debugging dithering issues if there was a reliable trace in the logs of what was active when. One suggestion for that inside your patch below...
>
>
>
> >
> > Cc: Uma Shankar <[e-mail]>
> > Signed-off-by: Nischal Varide <[e-mail]>
> > Signed-off-by: Bhanuprakash Modem <[e-mail]>
> > Signed-off-by: Matt Roper <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/display/intel_color.c   | 16 ++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_display.c |  9 ++++++++-
> >  drivers/gpu/drm/i915/i915_reg.h              |  3 ++-
> >  3 files changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> > index ff7dcb7088bf..9a0572bbc5db 100644
> > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > @@ -1604,6 +1604,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 (HAS_DISPLAY13(i915)) {
> > +             if (!crtc_state->dither_force_disable &&
>
>
>
> Replace  !crtc_state->dither_force_disable by crtc_state->dither
>
> > +                 (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;
> > @@ -1614,6 +1628,8 @@ 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 4dc4b1be0809..e3dbcd956fc6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -8098,9 +8098,15 @@ 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 ||
> >           crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444)
> >               val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
> > @@ -10760,6 +10766,7 @@ intel_modeset_pipe_config(struct intel_atomic_state *state,
> >        */
>
>
>
> Instead of...
>
>
>
> >       pipe_config->dither = (pipe_config->pipe_bpp == 6*3) &&
> >               !pipe_config->dither_force_disable;
> > +
>
>
>
> ... use ...
>
>
>
> >       pipe_config->dither = ((pipe_config->pipe_bpp == 6*3) || (HAS_DISPLAY13(i915) && pipe_config->pipe_bpp == 12*3)) && !pipe_config->dither_force_disable;
>
>
>
> ... so that the dither enable/disable decision and logging happens in one location in the code?
>
>
>
> >       drm_dbg_kms(&i915->drm,
> >                   "hw max bpp: %i, pipe bpp: %i, dithering: %i\n",
> >                   base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
>
>
>
> Thanks,
>
> -mario
>
>
>
>
>
>
>
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 128b835c0adb..27f25214a839 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6132,7 +6132,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)
> > @@ -7668,6 +7668,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/CSR */
> >  #define CSR_PROGRAM(i)               _MMIO(0x80000 + (i) * 4)
> > --
> > 2.25.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel
>
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index ff7dcb7088bf..9a0572bbc5db 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -1604,6 +1604,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 (HAS_DISPLAY13(i915)) {
+		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;
@@ -1614,6 +1628,8 @@  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 4dc4b1be0809..e3dbcd956fc6 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -8098,9 +8098,15 @@  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 ||
 	    crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444)
 		val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
@@ -10760,6 +10766,7 @@  intel_modeset_pipe_config(struct intel_atomic_state *state,
 	 */
 	pipe_config->dither = (pipe_config->pipe_bpp == 6*3) &&
 		!pipe_config->dither_force_disable;
+
 	drm_dbg_kms(&i915->drm,
 		    "hw max bpp: %i, pipe bpp: %i, dithering: %i\n",
 		    base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 128b835c0adb..27f25214a839 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6132,7 +6132,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)
@@ -7668,6 +7668,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/CSR */
 #define CSR_PROGRAM(i)		_MMIO(0x80000 + (i) * 4)