Re: [PATCH 2/2] drm/i915: Add module parameter to en-/disable hw color correction.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux