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.
-mario
>
> 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 &&
> + (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@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
