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

"Varide, Nischal" <nischal.varide@xxxxxxxxx> · Mon, 1 Mar 2021 04:57:37 +0000

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@xxxxxxxxx>

Sent: Friday, February 19, 2021 11:15 AM

To: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>

Cc: Roper, Matthew D <matthew.d.roper@xxxxxxxxx>; intel-gfx <intel-gfx@xxxxxxxxxxxxxxxxxxxxx>; Varide, Nischal <nischal.varide@xxxxxxxxx>; dri-devel <dri-devel@xxxxxxxxxxxxxxxxxxxxx>

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@xxxxxxxxx> wrote:

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

On Thu, Jan 28, 2021 at 11:24:13AM -0800, Matt Roper wrote:

> From: Nischal Varide <nischal.varide@xxxxxxxxx>

> 

> 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@xxxxxxxxx>

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@xxxxxxxxx>

> Signed-off-by: Nischal Varide <nischal.varide@xxxxxxxxx>

> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@xxxxxxxxx>

> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>

> ---

>  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@xxxxxxxxxxxxxxxxxxxxx

> 
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 

Ville Syrjälä

Intel

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