On Fri, Feb 19, 2021 at 4:22 AM Mario Kleiner <mario.kleiner...@gmail.com> wrote:
> > > On Thu, Feb 11, 2021 at 1:29 PM Ville Syrjälä < > ville.syrj...@linux.intel.com> wrote: > >> On Thu, Jan 28, 2021 at 11:24:13AM -0800, Matt Roper wrote: >> > From: Nischal Varide <nischal.var...@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...@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.shan...@intel.com> >> > Signed-off-by: Nischal Varide <nischal.var...@intel.com> >> > Signed-off-by: Bhanuprakash Modem <bhanuprakash.mo...@intel.com> >> > Signed-off-by: Matt Roper <matthew.d.ro...@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 >> >
_______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
