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

[Mario Kleiner <mario.kleiner.de@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7847 bytes --]

On Thu, Feb 11, 2021 at 1:29 PM Ville Syrjälä <ville.syrjala@linux.intel.com>
wrote:

> On Thu, Jan 28, 2021 at 11:24:13AM -0800, Matt Roper wrote:
> > From: Nischal Varide <nischal.varide@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.
>
> 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 <uma.shankar@intel.com>
> > Signed-off-by: Nischal Varide <nischal.varide@intel.com>
> > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  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
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel
>

[-- Attachment #1.2: Type: text/html, Size: 9964 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx