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...
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
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
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
Looks like there are two options.
1. Enable or Disable Dithering via kernel command line or sysfs. 2. To implement new Uapi. May be the first one is more feasible and faster
Regards Nischal
From: Mario Kleiner mario.kleiner.de@gmail.com Sent: Friday, February 19, 2021 11:15 AM To: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Roper, Matthew D matthew.d.roper@intel.com; intel-gfx intel-gfx@lists.freedesktop.org; Varide, Nischal nischal.varide@intel.com; dri-devel dri-devel@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@gmail.commailto:mario.kleiner.de@gmail.com> wrote:
On Thu, Feb 11, 2021 at 1:29 PM Ville Syrjälä <ville.syrjala@linux.intel.commailto: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.commailto: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.commailto: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.commailto:uma.shankar@intel.com> Signed-off-by: Nischal Varide <nischal.varide@intel.commailto:nischal.varide@intel.com> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.commailto:bhanuprakash.modem@intel.com> Signed-off-by: Matt Roper <matthew.d.roper@intel.commailto: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.orgmailto:Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Ville Syrjälä Intel
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@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@gmail.com Sent: Friday, February 19, 2021 11:15 AM To: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Roper, Matthew D matthew.d.roper@intel.com; intel-gfx intel-gfx@lists.freedesktop.org; Varide, Nischal nischal.varide@intel.com; dri-devel dri-devel@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@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
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Ilia Mirkin -
Mario Kleiner -
Varide, Nischal -
Ville Syrjälä