[Intel-gfx] [PATCH 18/18] drm/i915/display13: Enabling dithering after the CC1 pipe
Ilia Mirkin
imirkin at alum.mit.edu
Mon Mar 1 05:43:33 UTC 2021
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
<nischal.varide at intel.com> 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 <mario.kleiner.de at gmail.com>
> Sent: Friday, February 19, 2021 11:15 AM
> To: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Roper, Matthew D <matthew.d.roper at intel.com>; intel-gfx <intel-gfx at lists.freedesktop.org>; Varide, Nischal <nischal.varide at intel.com>; dri-devel <dri-devel at lists.freedesktop.org>
> 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 <mario.kleiner.de at gmail.com> wrote:
>
>
>
>
>
> On Thu, Feb 11, 2021 at 1:29 PM Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
>
> On Thu, Jan 28, 2021 at 11:24:13AM -0800, Matt Roper 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.
>
> 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 at 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 at intel.com>
> > Signed-off-by: Nischal Varide <nischal.varide at intel.com>
> > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper at 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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the Intel-gfx
mailing list