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

[Mario Kleiner <mario.kleiner.de@xxxxxxxxx>]

On 09/26/2017 07:05 AM, Daniel Vetter wrote:
> 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

Hi Daniel,

this isn't meant as a permanent solution, but as a debugging aid, and as 
the equivalent of an air-bag in a car. You hope you won't need it, but 
it is good to have. In the past it would have been very handy for me to 
have a master-switch for this, debugging problems on users machines 
related to "pixels don't appear on the outputs as specified in the 
OpenGL rendering code". When looking over the docs for color correction 
i just realized the hardware has an easy way to disable this part of the 
pipeline, so i thought this could make debugging so much easier - at 
least for me. I had the impression that many current i915 module 
parameters are of this nature.

The debug switch also provides a temporary workaround on production 
systems if a problem is related to color correction, not meant as a 
permanent solution. Many of my users are challenged already by the fact 
that Linux is not macOS, and editing a config file or installing a 
prebuilt kernel from a .dpkg is already borderline rocket science for 
them, that's why those module parameters would be nice to have.

My actual plan is to implement true 10 bit -> 12 bit gamma table 
support, hopefully still for the 4.15 kernel.

I have experimental patches for using the precision luts with 1024 slots 
and 10 bit output width, ie. 10 bit in -> 10 bit out on Ironlake and 
later. I'll send those out in their hacky state just for reference.

However the better plan i have in mind is to extend the code so that if 
(we are in single-lut mode (DEGAMMA_LUT == 0)) AND (the userspace 
provided input lut is monotonically increasing) we switch from the dual 
512 slot 10 bit luts to the 512 slot 12 bit lut. This would also be 
applicable to the 256 slot legacy gamma tables, which are always 
single-lut and can be upsampled from 256 slots to 512 slots.

The reason is that the dual-512 slot luts are not good enough to handle 
a 10 bit framebuffer. As far as i read the PRMs, a 10 bit fb value would 
simply get truncated to 9 bits to select one of the 512 slots, so we 
would lose 1 bit of precision, which makes 10 bit framebuffers mostly 
pointless, at least for scientific/medical/HDR applications.

The 512 slot 12 bit lut is perfect for such applications, as the PRMs 
say the hw will linearly interpolate between the nearest neighbor slots 
of the 512 slot lut for the given fb input value -> works with 10 bit 
fb's. Would also work with those 16 bit half-float fb format that is 
supported by current hw but currently unused - but could be handy for 
future HDR applications. Also 12 bit output precision is nice for better 
gamma correction on true 10-12 bit displays over DP/HDMI deep color.

I will try to work on this within the next 1-2 weeks.

Now here's a catch i found while testing with the 1024 slot 10 bit luts, 
which i found very surprising:

- If i have a standard XR24 framebuffer on the primary plane, the 1024 
slot/10 bit lut's work exactly as expected, as verified on a XR24 fb via 
photometer measurements and tweaking the values in the gamma tables -- 
and the "force dithering on" module parameter patch, as i don't have a 
true 10 bit panel around atm.

- As soon as a XR30 fb is active (X11 DefaultDepth 30, or Weston in 
gbm-format=xrgb2101010 mode), the fb pixels do reach the outputs with 10 
bit precision without any truncation, as expected, but the lut is 
*bypassed*! Loading arbitrary values into the gamma luts has no effect 
at all, unless switching to a XR24 plane. I don't know if some 
non-obvious (to me) setup is required for anything > XR24, or if 
something is going wrong with the hw. This on all tested gen's since 
Ironlake.

Haven't tested the 512 slot 12 bit wide lut yet, but that's the next step.

-mario

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