Module parameters to override color management/dithering.

Module parameters to override color management/dithering.

[Mario Kleiner]

Hi,

so these two patches add i915 module parameters to globally override
how the driver handles dithering and gamma/csc conversion.

They serve two purposes: First as debug aid and "airbag" for working
around potential precision problems in getting pixels from rendering
to the display outputs. This mostly for applications that critically
depend on getting pixels untampered from the fb to the outputs, e.g.,
scientific neuro-science/vision research/medical applications. Having
the ability to bypass parts of the pipeline can help a lot in debugging
such problems on remote user machines, and to allow such users to
work around the problems until proper fixes are made. I expect this
to become especially useful when dealing with all the Wayland compositor
implementations, which so far don't have a standardized application/user
controllable equivalent to RandR protocol / xrandr tools.

The second, short-term purpose is to enable true 10 bit output from
rendering, so people with urgent 10 bit precision needs can benefit
from the Mesa patches i started working on for i965 (rev 1 on the
mailing-list, rev 2 to come soon).

I realize the merge window for Linux 4.14 is almost over, but wanted
to ask if it would be possible to slip these patches into 4.14 if
they aren't considered too intrusive?

These are tested on Ironlake, Ivybridge, Haswell and Skylake, also
with a photometer to see what actually comes out of the display for
different settings.

The bigger plan is to enhance the gamma table support, so we could
also use > 8 bit precision gamma tables on Ironlake and later, both
for the legacy gamma ioctl and the new color mgmt. method. I do have
proof of concept patches for using the 1024->10 precision luts on
Ironlake and later. Tweaking the gamma tables i upload via RandR and
measuring with photometer showed my poc patches work. However, as
described in the 2nd patch, at least the tested legacy luts and
1024->10 precision luts seem to get mostly ignored/bypassed in the hw
when a XR30 fb is attached to the primary plane. Not sure if some setup
is missing, or if this is some hardware quirk? Couldn't find anything
in the PRM's so far.

What i'd actually like to implement for Ironlake+ instead of the
1024->10 bit luts is this:

If in dual-gamma lut mode, or if the input gamma table is not
monotonically increasing, do what is done now (legacy luts for legacy
gamma ioctl, split 512->10 big luts for new path).

If only a single gamma lut is requested (DEGAMMA_LUT == 0), and the
provided input lut is monotonically increasing, switch to the linearly
interpolated 512->12 bit lut instead, which exists on Ironlake+. Also
for the legacy gamma ioctl, so existing apps can benefit from the
higher precision. This would enable > 8 bit framebuffers to be output
properly and with high quality gamma correction.

Thanks,
-mario

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/i915: Add module parameter to force en-/disable dithering.

[Mario Kleiner]

i915.enable_dithering allows to force dithering on all outputs
on (=1) or off (=0). The default is -1 for current automatic
per-pipe selection.

This is useful for debugging and for special case scenarios,
e.g., providing simulated 10 bpc output on 8 bpc digital sinks
if a 10 bpc framebuffer + rendering is in use.

A more flexible solution would be connector properties, like
other drivers (radeon, amdgpu, nouveau) already provide. A
global override via module parameter is useful even with such
connector properties, e.g., for scientific applications which
require strict control over dithering, to have an override
for DE's which may not expose such properties via some standard
protocol in a user-controllable way, e.g., afaik all currently
existing Wayland compositors.

Tested on Ironlake, IvyBridge, Haswell, Skylake.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
---
 drivers/gpu/drm/i915/i915_params.c   | 5 +++++
 drivers/gpu/drm/i915/i915_params.h   | 1 +
 drivers/gpu/drm/i915/intel_display.c | 5 +++++
 3 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 8ab003dca113..07ec3a96457c 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -65,6 +65,7 @@ struct i915_params i915 __read_mostly = {
 	.inject_load_failure = 0,
 	.enable_dpcd_backlight = false,
 	.enable_gvt = false,
+	.enable_dithering = -1,
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);
@@ -257,3 +258,7 @@ MODULE_PARM_DESC(enable_dpcd_backlight,
 module_param_named(enable_gvt, i915.enable_gvt, bool, 0400);
 MODULE_PARM_DESC(enable_gvt,
 	"Enable support for Intel GVT-g graphics virtualization host support(default:false)");
+
+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)");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index ac844709c97e..7e365cd4fc91 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -54,6 +54,7 @@
 	func(int, edp_vswing); \
 	func(int, reset); \
 	func(unsigned int, inject_load_failure); \
+	func(int, enable_dithering); \
 	/* leave bools at the end to not create holes */ \
 	func(bool, alpha_support); \
 	func(bool, enable_cmd_parser); \
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0e93ec201fe3..bea471a96820 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10978,6 +10978,11 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 	 */
 	pipe_config->dither = (pipe_config->pipe_bpp == 6*3) &&
 		!pipe_config->dither_force_disable;
+
+	/* Override of auto-selected dither mode via module parameter? */
+	if (i915.enable_dithering != -1)
+		pipe_config->dither = i915.enable_dithering > 0 ? true : false;
+
 	DRM_DEBUG_KMS("hw max bpp: %i, pipe bpp: %i, dithering: %i\n",
 		      base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
 
-- 
2.13.0.rc1.294.g07d810a77f

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Mario Kleiner]

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.

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@gmail.com>
---
 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Add module parameter to force en-/disable dithering.

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Add module parameter to force en-/disable dithering.
URL   : https://patchwork.freedesktop.org/series/30428/
State : success

== Summary ==

Series 30428v1 series starting with [1/2] drm/i915: Add module parameter to force en-/disable dithering.
https://patchwork.freedesktop.org/api/1.0/series/30428/revisions/1/mbox/

Test pm_rpm:
        Subgroup basic-rte:
                pass       -> DMESG-WARN (fi-cfl-s) fdo#102294

fdo#102294 https://bugs.freedesktop.org/show_bug.cgi?id=102294

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:451s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:460s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:379s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:537s
fi-bwr-2160      total:289  pass:184  dwarn:0   dfail:0   fail:0   skip:105 time:267s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:510s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:506s
fi-byt-n2820     total:289  pass:250  dwarn:1   dfail:0   fail:0   skip:38  time:495s
fi-cfl-s         total:289  pass:222  dwarn:35  dfail:0   fail:0   skip:32  time:550s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:427s
fi-glk-2a        total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:597s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:430s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:414s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:445s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:500s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:465s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:499s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:585s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:587s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:553s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:464s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:525s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:507s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:458s
fi-skl-x1585l    total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:489s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:577s
fi-snb-2600      total:289  pass:248  dwarn:0   dfail:0   fail:2   skip:39  time:426s

9adc9e93d6243c82bcefd175c2d11770802de194 drm-tip: 2017y-09m-15d-11h-44m-46s UTC integration manifest
aff8e23f84b0 drm/i915: Add module parameter to en-/disable hw color correction.
a6b79d2f7735 drm/i915: Add module parameter to force en-/disable dithering.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5713/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915: Add module parameter to force en-/disable dithering.

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Add module parameter to force en-/disable dithering.
URL   : https://patchwork.freedesktop.org/series/30428/
State : success

== Summary ==

Test perf:
        Subgroup polling:
                pass       -> FAIL       (shard-hsw) fdo#102252 +1

fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252

shard-hsw        total:2313 pass:1245 dwarn:0   dfail:0   fail:13  skip:1055 time:9402s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5713/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Module parameters to override color management/dithering.

[Jani Nikula]

On Fri, 15 Sep 2017, Mario Kleiner <mario.kleiner.de@gmail.com> wrote:
> Hi,
>
> so these two patches add i915 module parameters to globally override
> how the driver handles dithering and gamma/csc conversion.
>
> They serve two purposes: First as debug aid and "airbag" for working
> around potential precision problems in getting pixels from rendering
> to the display outputs. This mostly for applications that critically
> depend on getting pixels untampered from the fb to the outputs, e.g.,
> scientific neuro-science/vision research/medical applications. Having
> the ability to bypass parts of the pipeline can help a lot in debugging
> such problems on remote user machines, and to allow such users to
> work around the problems until proper fixes are made. I expect this
> to become especially useful when dealing with all the Wayland compositor
> implementations, which so far don't have a standardized application/user
> controllable equivalent to RandR protocol / xrandr tools.
>
> The second, short-term purpose is to enable true 10 bit output from
> rendering, so people with urgent 10 bit precision needs can benefit
> from the Mesa patches i started working on for i965 (rev 1 on the
> mailing-list, rev 2 to come soon).
>
> I realize the merge window for Linux 4.14 is almost over, but wanted
> to ask if it would be possible to slip these patches into 4.14 if
> they aren't considered too intrusive?

For future reference, drm features for an upcoming merge window need to
be merged upstream by about -rc5, i.e. several weeks before the merge
window even opens.

As to the patches, we're wondering about ways to axe existing module
parameters, so we'd prefer not adding new ones. You mention connector
properties; common properties across drivers sounds like the way to go.

BR,
Jani.



>
> These are tested on Ironlake, Ivybridge, Haswell and Skylake, also
> with a photometer to see what actually comes out of the display for
> different settings.
>
> The bigger plan is to enhance the gamma table support, so we could
> also use > 8 bit precision gamma tables on Ironlake and later, both
> for the legacy gamma ioctl and the new color mgmt. method. I do have
> proof of concept patches for using the 1024->10 precision luts on
> Ironlake and later. Tweaking the gamma tables i upload via RandR and
> measuring with photometer showed my poc patches work. However, as
> described in the 2nd patch, at least the tested legacy luts and
> 1024->10 precision luts seem to get mostly ignored/bypassed in the hw
> when a XR30 fb is attached to the primary plane. Not sure if some setup
> is missing, or if this is some hardware quirk? Couldn't find anything
> in the PRM's so far.
>
> What i'd actually like to implement for Ironlake+ instead of the
> 1024->10 bit luts is this:
>
> If in dual-gamma lut mode, or if the input gamma table is not
> monotonically increasing, do what is done now (legacy luts for legacy
> gamma ioctl, split 512->10 big luts for new path).
>
> If only a single gamma lut is requested (DEGAMMA_LUT == 0), and the
> provided input lut is monotonically increasing, switch to the linearly
> interpolated 512->12 bit lut instead, which exists on Ironlake+. Also
> for the legacy gamma ioctl, so existing apps can benefit from the
> higher precision. This would enable > 8 bit framebuffers to be output
> properly and with high quality gamma correction.
>
> Thanks,
> -mario
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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@gmail.com>
> ---
>  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Mario Kleiner]

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@gmail.com>
>> ---
>>   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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Fri, Sep 29, 2017 at 08:57:47AM +0200, Mario Kleiner wrote:
> 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.

We really don't like the proliferation of module debug options. The only
time we are unhappily ok with them is if the hw just doesn't work as
documented, or we have some other issue that just doesn't make much sense
and results in random issues. PSR not working, rc6 not working, that kind
of stuff.

But this here is just a case of "not yet fully implemented", there's
really no need for users out there in the field to be able to change stuff
without changing code to help us diagnos an issue we don't yet understand.
The only one who needs this right now is you, for helping develop proper
10+bit gamma support, but I don't see how that would be useful to anyone
else.

On the issue itself: What I mean is that when there's no gamma table we
should properly bypass all the lut stuff automatically, to preserve the
full bits. Kinda as an interim improvement for your use-case, since it
sounds very much like you want both higher bit depth and gamma tables.

> 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.

I haven't checked, but iirc the Bspec wording for this stuff is very
funny, and explains which modes work with which bit depths. I thought it
only depends upon the pipe bit depth, not the primary plane colors, but
maybe I'm mistaken on that one.

You probably want to have all the public Bespec for ilk-skl at hand,
sometimes something gets dropped by accident in the scrubbing that should
be retained.

Another request for this: Please add new testcases to the kms_color_mgmt
igt testcase for all the new cases you're adding, especially once we start
upscaling LUTs.
-Daniel

> 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@gmail.com>
> > > ---
> > >   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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel