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: 10383 bytes --]

On Fri, Feb 19, 2021 at 4:22 AM Mario Kleiner <mario.kleiner.de@gmail.com>
wrote:

>
>
> 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
>
>
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 <mario.kleiner.de@gmail.com>

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 <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 &&
>>
>
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
>> > Intel-gfx@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> --
>> Ville Syrjälä
>> Intel
>>
>

[-- Attachment #1.2: Type: text/html, Size: 15063 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