On Fri, Sep 15, 2017 at 05:48:25PM +0200, Mario Kleiner wrote:
> The new module parameter enable_hw_color_correction defaults to
> true, to retain the current behaviour. If set to false, it will
> disable all hardware color correction, like gamma/degamma and
> csc.
>
> This is useful for debugging gamma table / csc precision problems,
> and to ensure unmodified pixel passthrough from framebuffer to
> outputs, e.g., for scientific applications which critically depend
> on perfect pixel passthrough. While i hope this switch generally
> won't be needed, it provides extra peace-of-mind - an "airbag" for
> color correction trouble.
>
> Tested on Ironlake, IvyBridge, Haswell, Skylake.
>
> One unexpected result during testing was that while this works on
> all tested gpu's with a 8 bpc XR24 framebuffer as primary plane,
> if a 10 bpc XR30 fb is active, then hw gamma tables seem to get
> automatically bypassed on at least the tested IvyBridge and later
> (but not on the tested Ironlake), regardless of hw programming,
> at least for the legacy 256->8 bit luts and the 1024->10 bit
> precision luts. However, the type of selected - but bypassed -
> hw gamma table still determines output precision, ie. even an
> auto-bypassed legacy 256 slot 8 bit lut in XR30 fb mode still
> restricts the effective output precision to 8 bit, while an
> auto-bypassed precision lut doesn't restrict precision.
Instead of a modparam I think the right thing to fix here is the driver
setup. Enabling the legacy gamma table is indeed documented to restrict
the pipe to 8bpc (the 2 additional bits for 10bpc are just padded).
Having driver options for "pls give me non-broken behaviour" doesn't make
any sense to me.
-Daniel
>
> Iow. this patch is needed even with XR30 fb's for actual 10
> bit precision output, even though the hw seems to sort of ignore
> the tested gamma tables for XR30 fb's.
>
> Signed-off-by: Mario Kleiner <mario.kleiner.de@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_params.c | 5 +++++
> drivers/gpu/drm/i915/i915_params.h | 3 ++-
> drivers/gpu/drm/i915/intel_display.c | 26 +++++++++++++++++---------
> drivers/gpu/drm/i915/intel_sprite.c | 21 ++++++++++++++++-----
> 4 files changed, 40 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 07ec3a96457c..8f6a176a97e1 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -66,6 +66,7 @@ struct i915_params i915 __read_mostly = {
> .enable_dpcd_backlight = false,
> .enable_gvt = false,
> .enable_dithering = -1,
> + .enable_hw_color_correction = true,
> };
>
> module_param_named(modeset, i915.modeset, int, 0400);
> @@ -262,3 +263,7 @@ MODULE_PARM_DESC(enable_gvt,
> module_param_named(enable_dithering, i915.enable_dithering, int, 0644);
> MODULE_PARM_DESC(enable_dithering,
> "Enable dithering (-1=auto [default], 0=force off on all outputs, 1=force on on all outputs)");
> +
> +module_param_named(enable_hw_color_correction, i915.enable_hw_color_correction, bool, 0644);
> +MODULE_PARM_DESC(enable_hw_color_correction,
> + "Enable hardware color correction like gamma luts and csc (default: true)");
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 7e365cd4fc91..f5c9163d2675 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -69,7 +69,8 @@
> func(bool, nuclear_pageflip); \
> func(bool, enable_dp_mst); \
> func(bool, enable_dpcd_backlight); \
> - func(bool, enable_gvt)
> + func(bool, enable_gvt); \
> + func(bool, enable_hw_color_correction)
>
> #define MEMBER(T, member) T member
> struct i915_params {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bea471a96820..1e1b157353a9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3184,13 +3184,17 @@ static u32 i9xx_plane_ctl(const struct intel_crtc_state *crtc_state,
> unsigned int rotation = plane_state->base.rotation;
> u32 dspcntr;
>
> - dspcntr = DISPLAY_PLANE_ENABLE | DISPPLANE_GAMMA_ENABLE;
> + dspcntr = DISPLAY_PLANE_ENABLE;
> +
> + if (i915.enable_hw_color_correction)
> + dspcntr |= DISPPLANE_GAMMA_ENABLE;
>
> if (IS_G4X(dev_priv) || IS_GEN5(dev_priv) ||
> IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv))
> dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
>
> - if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> + if ((IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) &&
> + i915.enable_hw_color_correction)
> dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
>
> if (INTEL_GEN(dev_priv) < 4)
> @@ -3514,7 +3518,8 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
>
> plane_ctl = PLANE_CTL_ENABLE;
>
> - if (!IS_GEMINILAKE(dev_priv) && !IS_CANNONLAKE(dev_priv)) {
> + if (!IS_GEMINILAKE(dev_priv) && !IS_CANNONLAKE(dev_priv) &&
> + i915.enable_hw_color_correction) {
> plane_ctl |=
> PLANE_CTL_PIPE_GAMMA_ENABLE |
> PLANE_CTL_PIPE_CSC_ENABLE |
> @@ -3571,7 +3576,8 @@ static void skylake_update_primary_plane(struct intel_plane *plane,
>
> spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>
> - if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) {
> + if ((IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) &&
> + i915.enable_hw_color_correction) {
> I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> PLANE_COLOR_PIPE_GAMMA_ENABLE |
> PLANE_COLOR_PIPE_CSC_ENABLE |
> @@ -9431,7 +9437,7 @@ static u32 i845_cursor_ctl(const struct intel_crtc_state *crtc_state,
> const struct drm_framebuffer *fb = plane_state->base.fb;
>
> return CURSOR_ENABLE |
> - CURSOR_GAMMA_ENABLE |
> + (i915.enable_hw_color_correction ? CURSOR_GAMMA_ENABLE : 0) |
> CURSOR_FORMAT_ARGB |
> CURSOR_STRIDE(fb->pitches[0]);
> }
> @@ -9544,12 +9550,14 @@ static u32 i9xx_cursor_ctl(const struct intel_crtc_state *crtc_state,
> struct drm_i915_private *dev_priv =
> to_i915(plane_state->base.plane->dev);
> struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> - u32 cntl;
> + u32 cntl = 0;
>
> - cntl = MCURSOR_GAMMA_ENABLE;
> + if (i915.enable_hw_color_correction) {
> + cntl = MCURSOR_GAMMA_ENABLE;
>
> - if (HAS_DDI(dev_priv))
> - cntl |= CURSOR_PIPE_CSC_ENABLE;
> + if (HAS_DDI(dev_priv))
> + cntl |= CURSOR_PIPE_CSC_ENABLE;
> + }
>
> cntl |= MCURSOR_PIPE_SELECT(crtc->pipe);
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 524933b01483..6e6a7377a7bc 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -258,7 +258,8 @@ skl_update_plane(struct intel_plane *plane,
>
> spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>
> - if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) {
> + if ((IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) &&
> + i915.enable_hw_color_correction) {
> I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> PLANE_COLOR_PIPE_GAMMA_ENABLE |
> PLANE_COLOR_PIPE_CSC_ENABLE |
> @@ -371,7 +372,10 @@ static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> u32 sprctl;
>
> - sprctl = SP_ENABLE | SP_GAMMA_ENABLE;
> + sprctl = SP_ENABLE;
> +
> + if (i915.enable_hw_color_correction)
> + sprctl |= SP_GAMMA_ENABLE;
>
> switch (fb->format->format) {
> case DRM_FORMAT_YUYV:
> @@ -511,12 +515,16 @@ static u32 ivb_sprite_ctl(const struct intel_crtc_state *crtc_state,
> const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> u32 sprctl;
>
> - sprctl = SPRITE_ENABLE | SPRITE_GAMMA_ENABLE;
> + sprctl = SPRITE_ENABLE;
> +
> + if (i915.enable_hw_color_correction)
> + sprctl |= SPRITE_GAMMA_ENABLE;
>
> if (IS_IVYBRIDGE(dev_priv))
> sprctl |= SPRITE_TRICKLE_FEED_DISABLE;
>
> - if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> + if ((IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) &&
> + i915.enable_hw_color_correction)
> sprctl |= SPRITE_PIPE_CSC_ENABLE;
>
> switch (fb->format->format) {
> @@ -651,7 +659,10 @@ static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
> const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> u32 dvscntr;
>
> - dvscntr = DVS_ENABLE | DVS_GAMMA_ENABLE;
> + dvscntr = DVS_ENABLE;
> +
> + if (i915.enable_hw_color_correction)
> + dvscntr |= DVS_GAMMA_ENABLE;
>
> if (IS_GEN6(dev_priv))
> dvscntr |= DVS_TRICKLE_FEED_DISABLE;
> --
> 2.13.0.rc1.294.g07d810a77f
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
