8-bit vs. 10-bit palette mode, and LVDS dithering

8-bit vs. 10-bit palette mode, and LVDS dithering

[Peter Clifton]

Hi guys,

I noticed an anomaly in the register settings on my GM45, according to
PIPEBCONF, it is set in 10-bit palette mode, yet the KMS code programs
the palette registers as if it is in 8-bit mode, and doesn't touch the
PIPEBGCMAX{RED,GREEN,BLUE} entries for the final interpolation step.

I've played with it, and I "think" my screen looks nicer with the
palette reset to 8-bit mode. The KMS driver never touches this register
normally, and I think it should reset it in intel_display.c's
intel_crtc_load_lut.

Ideally, it would be nice to have a higher resolution gamma correction.
Will it work if I drm_mode_crtc_set_gamma_size(&intel_crtc->base, 129);
and feed that into the 10-bit linear-interpolated lookup table?


At what stage is the LVDS dithering involved here.. gamma correction
probably makes a difference to whether a dither works nicely or not, and
I'm wondering whether I can tune the dithering process at all.

(I've got an HP laptop with a really bad colour profile, so it needs a
fair bit of correction to make colours appear properly. The hardware
hacker in me wonders if there is an EEPROM of gamma correction constants
on the panel which could be "fixed", but sadly litte data is available
from the panel manufacturer).

Regards,

-- 
Peter Clifton

Electrical Engineering Division,
Engineering Department,
University of Cambridge,
9, JJ Thomson Avenue,
Cambridge
CB3 0FA

Tel: +44 (0)7729 980173 - (No signal in the lab!)
Tel: +44 (0)1223 748328 - (Shared lab phone, ask for me)

Re: 8-bit vs. 10-bit palette mode, and LVDS dithering

[Adam Jackson]

[-- Attachment #1.1: Type: text/plain, Size: 2212 bytes --]

On Sat, 2010-04-24 at 17:25 +0100, Peter Clifton wrote:

> I noticed an anomaly in the register settings on my GM45, according to
> PIPEBCONF, it is set in 10-bit palette mode, yet the KMS code programs
> the palette registers as if it is in 8-bit mode, and doesn't touch the
> PIPEBGCMAX{RED,GREEN,BLUE} entries for the final interpolation step.
> 
> I've played with it, and I "think" my screen looks nicer with the
> palette reset to 8-bit mode. The KMS driver never touches this register
> normally, and I think it should reset it in intel_display.c's
> intel_crtc_load_lut.

You are almost certainly correct, we should be forcing 8-bit gamma mode.
We're loading the palette as though we are, it's very unlikely to look
right if programmed for 10-bit.

> Ideally, it would be nice to have a higher resolution gamma correction.
> Will it work if I drm_mode_crtc_set_gamma_size(&intel_crtc->base, 129);
> and feed that into the 10-bit linear-interpolated lookup table?

There's a 256-stop assumption hardcoded in a bunch of places, and you
want to be compatible with whatever ramp size userspace passes down.
But, once you fix that, yes, 10-bit linear gamma should work regardless
of the actual pixel depth.  It's definitely something we should fix
though, 30bpp displays won't look any better than 24bpp if we don't do
wider gamma.

Cantiga's 10-bit palette mode is... weird.  It really only works for
TrueColor visuals, and it's a bit counterintuitive that the 129-stop
ramp would be more precise than the 256-stop ramp.  We should probably
only expose it for 30bpp framebuffers (which should then be TrueColor
only in X).

> At what stage is the LVDS dithering involved here.. gamma correction
> probably makes a difference to whether a dither works nicely or not, and
> I'm wondering whether I can tune the dithering process at all.

I suspect gamma applies _after_ truncation and dither, and that dither
doesn't have any knowledge of the gamma ramp and is only trying to
approximate the bits being sourced from the framebuffer.  There's
probably some clever non-monotonic-increasing ramp you could program
that would let one deduce where gamma happens.

- ajax

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH] drm/intel: Set 8-bit gamma mode for the palette

[Peter Clifton]

We're programming an 8-bit palette, so ensure we set the hardware
up appropriately. Also, rename the palreg variable pal_reg for more
consistency with the rest of this file.
---
 drivers/gpu/drm/i915/intel_display.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4e5e688..006e5a5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3430,7 +3430,10 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int palreg = (intel_crtc->pipe == 0) ? PALETTE_A : PALETTE_B;
+	int pipe = intel_crtc->pipe;
+	int pal_reg = (pipe == 0) ? PALETTE_A : PALETTE_B;
+	int pipeconf_reg = (pipe == 0) ? PIPEACONF : PIPEBCONF;
+	int pipeconf = I915_READ(pipeconf_reg);
 	int i;
 
 	/* The clocks have to be on to load the palette. */
@@ -3439,11 +3442,16 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
 
 	/* use legacy palette for Ironlake */
 	if (IS_IRONLAKE(dev))
-		palreg = (intel_crtc->pipe == 0) ? LGC_PALETTE_A :
-						   LGC_PALETTE_B;
+		pal_reg = (intel_crtc->pipe == 0) ? LGC_PALETTE_A :
+						    LGC_PALETTE_B;
+
+	/* Switch to 8-bit gamma mode */
+	pipeconf &= ~PIPEACONF_GAMMA;
+	I915_WRITE(pipeconf_reg, pipeconf);
+	I915_READ(pipeconf_reg);
 
 	for (i = 0; i < 256; i++) {
-		I915_WRITE(palreg + 4 * i,
+		I915_WRITE(pal_reg + 4 * i,
 			   (intel_crtc->lut_r[i] << 16) |
 			   (intel_crtc->lut_g[i] << 8) |
 			   intel_crtc->lut_b[i]);
-- 
1.7.0.4

Re: [PATCH] drm/intel: Set 8-bit gamma mode for the palette

[Adam Jackson]

[-- Attachment #1.1: Type: text/plain, Size: 1365 bytes --]

On Mon, 2010-04-26 at 22:19 +0100, Peter Clifton wrote:

> @@ -3439,11 +3442,16 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
>  
>  	/* use legacy palette for Ironlake */
>  	if (IS_IRONLAKE(dev))
> -		palreg = (intel_crtc->pipe == 0) ? LGC_PALETTE_A :
> -						   LGC_PALETTE_B;
> +		pal_reg = (intel_crtc->pipe == 0) ? LGC_PALETTE_A :
> +						    LGC_PALETTE_B;
> +
> +	/* Switch to 8-bit gamma mode */
> +	pipeconf &= ~PIPEACONF_GAMMA;
> +	I915_WRITE(pipeconf_reg, pipeconf);
> +	I915_READ(pipeconf_reg);

This is wrong for gen5, where bits 25 and 24 control gamma:

Value Name     Description                    Project
00b   8 bit    8-bit Legacy Palette Mode      All
01b   10 bt    10-bit Precision Palette Mode  All
10b   12 bit   12-bit Interpolated Gamma Mode All
11b   Reserved Reserved                       All

Where 10-bit is a real 1024-stop gamma, and 12-bit is 512 lerp'd stops
(like gen4's 129 lerp'd stops).  I don't think it'd matter in the real
world since I doubt any OEM is insane enough to set up 12-bit in the
BIOS, but.

On gen4 and earlier though, bit 25 is border color enable.  Pretty sure
we only use that in VGA load detection.  Although, we use it in VGA load
detection even on gen5, which means load detection on gen5 almost
certainly does not work.

Patch looks good otherwise.

- ajax

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: 8-bit vs. 10-bit palette mode, and LVDS dithering

[Peter Clifton]

On Mon, 2010-04-26 at 10:29 -0400, Adam Jackson wrote:
> On Sat, 2010-04-24 at 17:25 +0100, Peter Clifton wrote:
> 
> > I noticed an anomaly in the register settings on my GM45, according to
> > PIPEBCONF, it is set in 10-bit palette mode, yet the KMS code programs
> > the palette registers as if it is in 8-bit mode, and doesn't touch the
> > PIPEBGCMAX{RED,GREEN,BLUE} entries for the final interpolation step.
> > 
> > I've played with it, and I "think" my screen looks nicer with the
> > palette reset to 8-bit mode. The KMS driver never touches this register
> > normally, and I think it should reset it in intel_display.c's
> > intel_crtc_load_lut.
> 
> You are almost certainly correct, we should be forcing 8-bit gamma mode.
> We're loading the palette as though we are, it's very unlikely to look
> right if programmed for 10-bit.
> 
> > Ideally, it would be nice to have a higher resolution gamma correction.
> > Will it work if I drm_mode_crtc_set_gamma_size(&intel_crtc->base, 129);
> > and feed that into the 10-bit linear-interpolated lookup table?

Comments on the following patch series (please don't apply as is!)

> There's a 256-stop assumption hardcoded in a bunch of places, and you
> want to be compatible with whatever ramp size userspace passes down.
> But, once you fix that, yes, 10-bit linear gamma should work regardless
> of the actual pixel depth.

If we start asking for a 129 entry LUT, the Xorg stack needs to be fixed
not to try and shove it a 256 entry table. There will be of course,
versions which still do that - and ideally the palette would still work
for those - either by fall-back to 8-bit mode, or by fudging a 10-bit
table from the passed data.

By patch 2 in the following series, this works by picking every other
entry out of the 256 entry table, and then accept the discontinuity by
taking the 256th entry in the passed LUT for the 129th register set.


-- 
Peter Clifton

Electrical Engineering Division,
Engineering Department,
University of Cambridge,
9, JJ Thomson Avenue,
Cambridge
CB3 0FA

Tel: +44 (0)7729 980173 - (No signal in the lab!)
Tel: +44 (0)1223 748328 - (Shared lab phone, ask for me)

[PATCH 1/3] drm/intel: Store full 16 bits of colors passed to gamma LUT

[Peter Clifton]

Store the full precision, even if we only use the high byte
when programming the palette registers.
---
 drivers/gpu/drm/i915/intel_display.c |   30 +++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h     |    2 +-
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 006e5a5..456f738 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3452,9 +3452,9 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
 
 	for (i = 0; i < 256; i++) {
 		I915_WRITE(pal_reg + 4 * i,
-			   (intel_crtc->lut_r[i] << 16) |
-			   (intel_crtc->lut_g[i] << 8) |
-			   intel_crtc->lut_b[i]);
+			   ((intel_crtc->lut_r[i] >> 8) << 16) |
+			   ((intel_crtc->lut_g[i] >> 8) << 8) |
+			   (intel_crtc->lut_b[i] >> 8));
 	}
 }
 
@@ -3609,9 +3609,9 @@ void intel_crtc_fb_gamma_set(struct drm_crtc *crtc, u16 red, u16 green,
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-	intel_crtc->lut_r[regno] = red >> 8;
-	intel_crtc->lut_g[regno] = green >> 8;
-	intel_crtc->lut_b[regno] = blue >> 8;
+	intel_crtc->lut_r[regno] = red;
+	intel_crtc->lut_g[regno] = green;
+	intel_crtc->lut_b[regno] = blue;
 }
 
 void intel_crtc_fb_gamma_get(struct drm_crtc *crtc, u16 *red, u16 *green,
@@ -3619,9 +3619,9 @@ void intel_crtc_fb_gamma_get(struct drm_crtc *crtc, u16 *red, u16 *green,
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-	*red = intel_crtc->lut_r[regno] << 8;
-	*green = intel_crtc->lut_g[regno] << 8;
-	*blue = intel_crtc->lut_b[regno] << 8;
+	*red = intel_crtc->lut_r[regno];
+	*green = intel_crtc->lut_g[regno];
+	*blue = intel_crtc->lut_b[regno];
 }
 
 static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
@@ -3634,9 +3634,9 @@ static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
 		return;
 
 	for (i = 0; i < 256; i++) {
-		intel_crtc->lut_r[i] = red[i] >> 8;
-		intel_crtc->lut_g[i] = green[i] >> 8;
-		intel_crtc->lut_b[i] = blue[i] >> 8;
+		intel_crtc->lut_r[i] = red[i];
+		intel_crtc->lut_g[i] = green[i];
+		intel_crtc->lut_b[i] = blue[i];
 	}
 
 	intel_crtc_load_lut(crtc);
@@ -4294,9 +4294,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	intel_crtc->pipe = pipe;
 	intel_crtc->plane = pipe;
 	for (i = 0; i < 256; i++) {
-		intel_crtc->lut_r[i] = i;
-		intel_crtc->lut_g[i] = i;
-		intel_crtc->lut_b[i] = i;
+		intel_crtc->lut_r[i] = i << 8 | i;
+		intel_crtc->lut_g[i] = i << 8 | i;
+		intel_crtc->lut_b[i] = i << 8 | i;
 	}
 
 	/* Swap pipes & planes for FBC on pre-965 */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3dcf5cc..6f09806 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -143,7 +143,7 @@ struct intel_crtc {
 	enum plane plane;
 	struct drm_gem_object *cursor_bo;
 	uint32_t cursor_addr;
-	u8 lut_r[256], lut_g[256], lut_b[256];
+	u16 lut_r[256], lut_g[256], lut_b[256];
 	int dpms_mode;
 	bool busy; /* is scanout buffer being updated frequently? */
 	struct timer_list idle_timer;
-- 
1.7.0.4

[PATCH 2/3] drm/intel: Attempt to use 10-bit gamma palette mode

[Peter Clifton]

---
 drivers/gpu/drm/i915/i915_reg.h      |    7 ++++++-
 drivers/gpu/drm/i915/intel_display.c |   33 ++++++++++++++++++++++++++-------
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index ab1bd2d..7a0c6ac 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1766,6 +1766,9 @@
 #define   PIPECONF_INTERLACE_W_FIELD_INDICATION	(6 << 21)
 #define   PIPECONF_INTERLACE_FIELD_0_ONLY		(7 << 21)
 #define   PIPECONF_CXSR_DOWNCLOCK	(1<<16)
+#define PIPEAGCMAXRED		0x70010
+#define PIPEAGCMAXGREEN		0x70014
+#define PIPEAGCMAXBLUE		0x70018
 #define PIPEASTAT		0x70024
 #define   PIPE_FIFO_UNDERRUN_STATUS		(1UL<<31)
 #define   PIPE_CRC_ERROR_ENABLE			(1UL<<29)
@@ -1958,13 +1961,15 @@
 /* Pipe B */
 #define PIPEBDSL		0x71000
 #define PIPEBCONF		0x71008
+#define PIPEBGCMAXRED		0x71010
+#define PIPEBGCMAXGREEN		0x71014
+#define PIPEBGCMAXBLUE		0x71018
 #define PIPEBSTAT		0x71024
 #define PIPEBFRAMEHIGH		0x71040
 #define PIPEBFRAMEPIXEL		0x71044
 #define PIPEB_FRMCOUNT_GM45	0x71040
 #define PIPEB_FLIPCOUNT_GM45	0x71044
 
-
 /* Display B control */
 #define DSPBCNTR		0x71180
 #define   DISPPLANE_ALPHA_TRANS_ENABLE		(1<<15)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 456f738..5e8191a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3433,6 +3433,9 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	int pal_reg = (pipe == 0) ? PALETTE_A : PALETTE_B;
 	int pipeconf_reg = (pipe == 0) ? PIPEACONF : PIPEBCONF;
+	int maxr_reg = (pipe == 0) ? PIPEAGCMAXRED : PIPEBGCMAXRED;
+	int maxg_reg = (pipe == 0) ? PIPEAGCMAXGREEN : PIPEBGCMAXGREEN;
+	int maxb_reg = (pipe == 0) ? PIPEAGCMAXBLUE : PIPEBGCMAXBLUE;
 	int pipeconf = I915_READ(pipeconf_reg);
 	int i;
 
@@ -3445,17 +3448,33 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
 		pal_reg = (intel_crtc->pipe == 0) ? LGC_PALETTE_A :
 						    LGC_PALETTE_B;
 
-	/* Switch to 8-bit gamma mode */
-	pipeconf &= ~PIPEACONF_GAMMA;
+	/* Switch to 10-bit gamma mode */
+	pipeconf |= PIPEACONF_GAMMA;
 	I915_WRITE(pipeconf_reg, pipeconf);
 	I915_READ(pipeconf_reg);
 
-	for (i = 0; i < 256; i++) {
-		I915_WRITE(pal_reg + 4 * i,
-			   ((intel_crtc->lut_r[i] >> 8) << 16) |
-			   ((intel_crtc->lut_g[i] >> 8) << 8) |
-			   (intel_crtc->lut_b[i] >> 8));
+	/* Use every other value from the LUT passed,
+	 * 10-bit mode uses 128 entries. */
+	for (i = 0; i < 128; i++) {
+		I915_WRITE(pal_reg + 8 * i,
+			   ((intel_crtc->lut_r[2 * i] & 0xFF) << 16) |
+			   ((intel_crtc->lut_g[2 * i] & 0xFF) << 8) |
+			   (intel_crtc->lut_b[2 * i] & 0xFF));
+		I915_WRITE(pal_reg + 8 * i + 4,
+			   ((intel_crtc->lut_r[2 * i] >> 8) << 16) |
+			   ((intel_crtc->lut_g[2 * i] >> 8) << 8) |
+			   (intel_crtc->lut_b[2 * i] >> 8));
 	}
+
+	/* FIXME: Distortion here, we're trying to get 129 evenly spaced
+	 * samples from a LUT with 256 entries. We use 0, 2, 4 ... 254,
+	 * for the main palette, then entry 255 for this last register.
+	 */
+	/* Note that these registers _could_ take the LUT value of
+	 * 1024, but we're maxing out at 1023.984375 as it is easier. */
+	I915_WRITE(maxr_reg, intel_crtc->lut_r[255]);
+	I915_WRITE(maxg_reg, intel_crtc->lut_g[255]);
+	I915_WRITE(maxb_reg, intel_crtc->lut_b[255]);
 }
 
 static int intel_crtc_cursor_set(struct drm_crtc *crtc,
-- 
1.7.0.4

Re: [PATCH 2/3] drm/intel: Attempt to use 10-bit gamma palette mode

[Andrew Lutomirski]

Should this patch be enough to output 10 bits/channel on a digital
output?  I just ordered some 10-bit monitors and it would be fun to
use them in all their high-precision glory?

It should be relatively easy to use spotread (from Argyll) along with
some hacked-up gamma loader to verify that it's working.

--Andy

On Mon, Apr 26, 2010 at 6:24 PM, Peter Clifton <pcjc2@cam.ac.uk> wrote:
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |    7 ++++++-
>  drivers/gpu/drm/i915/intel_display.c |   33 ++++++++++++++++++++++++++-------
>  2 files changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index ab1bd2d..7a0c6ac 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1766,6 +1766,9 @@
>  #define   PIPECONF_INTERLACE_W_FIELD_INDICATION        (6 << 21)
>  #define   PIPECONF_INTERLACE_FIELD_0_ONLY              (7 << 21)
>  #define   PIPECONF_CXSR_DOWNCLOCK      (1<<16)
> +#define PIPEAGCMAXRED          0x70010
> +#define PIPEAGCMAXGREEN                0x70014
> +#define PIPEAGCMAXBLUE         0x70018
>  #define PIPEASTAT              0x70024
>  #define   PIPE_FIFO_UNDERRUN_STATUS            (1UL<<31)
>  #define   PIPE_CRC_ERROR_ENABLE                        (1UL<<29)
> @@ -1958,13 +1961,15 @@
>  /* Pipe B */
>  #define PIPEBDSL               0x71000
>  #define PIPEBCONF              0x71008
> +#define PIPEBGCMAXRED          0x71010
> +#define PIPEBGCMAXGREEN                0x71014
> +#define PIPEBGCMAXBLUE         0x71018
>  #define PIPEBSTAT              0x71024
>  #define PIPEBFRAMEHIGH         0x71040
>  #define PIPEBFRAMEPIXEL                0x71044
>  #define PIPEB_FRMCOUNT_GM45    0x71040
>  #define PIPEB_FLIPCOUNT_GM45   0x71044
>
> -
>  /* Display B control */
>  #define DSPBCNTR               0x71180
>  #define   DISPPLANE_ALPHA_TRANS_ENABLE         (1<<15)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 456f738..5e8191a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3433,6 +3433,9 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
>        int pipe = intel_crtc->pipe;
>        int pal_reg = (pipe == 0) ? PALETTE_A : PALETTE_B;
>        int pipeconf_reg = (pipe == 0) ? PIPEACONF : PIPEBCONF;
> +       int maxr_reg = (pipe == 0) ? PIPEAGCMAXRED : PIPEBGCMAXRED;
> +       int maxg_reg = (pipe == 0) ? PIPEAGCMAXGREEN : PIPEBGCMAXGREEN;
> +       int maxb_reg = (pipe == 0) ? PIPEAGCMAXBLUE : PIPEBGCMAXBLUE;
>        int pipeconf = I915_READ(pipeconf_reg);
>        int i;
>
> @@ -3445,17 +3448,33 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
>                pal_reg = (intel_crtc->pipe == 0) ? LGC_PALETTE_A :
>                                                    LGC_PALETTE_B;
>
> -       /* Switch to 8-bit gamma mode */
> -       pipeconf &= ~PIPEACONF_GAMMA;
> +       /* Switch to 10-bit gamma mode */
> +       pipeconf |= PIPEACONF_GAMMA;
>        I915_WRITE(pipeconf_reg, pipeconf);
>        I915_READ(pipeconf_reg);
>
> -       for (i = 0; i < 256; i++) {
> -               I915_WRITE(pal_reg + 4 * i,
> -                          ((intel_crtc->lut_r[i] >> 8) << 16) |
> -                          ((intel_crtc->lut_g[i] >> 8) << 8) |
> -                          (intel_crtc->lut_b[i] >> 8));
> +       /* Use every other value from the LUT passed,
> +        * 10-bit mode uses 128 entries. */
> +       for (i = 0; i < 128; i++) {
> +               I915_WRITE(pal_reg + 8 * i,
> +                          ((intel_crtc->lut_r[2 * i] & 0xFF) << 16) |
> +                          ((intel_crtc->lut_g[2 * i] & 0xFF) << 8) |
> +                          (intel_crtc->lut_b[2 * i] & 0xFF));
> +               I915_WRITE(pal_reg + 8 * i + 4,
> +                          ((intel_crtc->lut_r[2 * i] >> 8) << 16) |
> +                          ((intel_crtc->lut_g[2 * i] >> 8) << 8) |
> +                          (intel_crtc->lut_b[2 * i] >> 8));
>        }
> +
> +       /* FIXME: Distortion here, we're trying to get 129 evenly spaced
> +        * samples from a LUT with 256 entries. We use 0, 2, 4 ... 254,
> +        * for the main palette, then entry 255 for this last register.
> +        */
> +       /* Note that these registers _could_ take the LUT value of
> +        * 1024, but we're maxing out at 1023.984375 as it is easier. */
> +       I915_WRITE(maxr_reg, intel_crtc->lut_r[255]);
> +       I915_WRITE(maxg_reg, intel_crtc->lut_g[255]);
> +       I915_WRITE(maxb_reg, intel_crtc->lut_b[255]);
>  }
>
>  static int intel_crtc_cursor_set(struct drm_crtc *crtc,
> --
> 1.7.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Re: [PATCH 2/3] drm/intel: Attempt to use 10-bit gamma palette mode

[Peter Clifton]

On Mon, 2010-04-26 at 21:09 -0400, Andrew Lutomirski wrote:
> Should this patch be enough to output 10 bits/channel on a digital
> output?  I just ordered some 10-bit monitors and it would be fun to
> use them in all their high-precision glory?

Should be enough to get 10-bit gamma correction working. There was
another patch to enable 30-bit displays (already applied):

commit a4f45cf178f0d0ad4e516e020818b5f1c00e3d63
Author: Kristian Høgsberg <krh@bitplanet.net>
Date:   Mon Oct 19 14:35:30 2009 -0400

    drm/i915: Support 30 bit depth modes
    
    Signed-off-by: Kristian Høgsberg <krh@redhat.com>
    Signed-off-by: Eric Anholt <eric@anholt.net>

-- 
Peter Clifton

Electrical Engineering Division,
Engineering Department,
University of Cambridge,
9, JJ Thomson Avenue,
Cambridge
CB3 0FA

Tel: +44 (0)7729 980173 - (No signal in the lab!)
Tel: +44 (0)1223 748328 - (Shared lab phone, ask for me)

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

[PATCH 3/3] drm/intel: Use 10-bit palette properly, only store 129 entries

[Peter Clifton]

---
 drivers/gpu/drm/i915/intel_display.c |   38 ++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_drv.h     |    2 +-
 2 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5e8191a..16cc13c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3453,28 +3453,22 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
 	I915_WRITE(pipeconf_reg, pipeconf);
 	I915_READ(pipeconf_reg);
 
-	/* Use every other value from the LUT passed,
-	 * 10-bit mode uses 128 entries. */
 	for (i = 0; i < 128; i++) {
 		I915_WRITE(pal_reg + 8 * i,
-			   ((intel_crtc->lut_r[2 * i] & 0xFF) << 16) |
-			   ((intel_crtc->lut_g[2 * i] & 0xFF) << 8) |
-			   (intel_crtc->lut_b[2 * i] & 0xFF));
+			   ((intel_crtc->lut_r[i] & 0xFF) << 16) |
+			   ((intel_crtc->lut_g[i] & 0xFF) << 8) |
+			   (intel_crtc->lut_b[i] & 0xFF));
 		I915_WRITE(pal_reg + 8 * i + 4,
-			   ((intel_crtc->lut_r[2 * i] >> 8) << 16) |
-			   ((intel_crtc->lut_g[2 * i] >> 8) << 8) |
-			   (intel_crtc->lut_b[2 * i] >> 8));
+			   ((intel_crtc->lut_r[i] >> 8) << 16) |
+			   ((intel_crtc->lut_g[i] >> 8) << 8) |
+			   (intel_crtc->lut_b[i] >> 8));
 	}
 
-	/* FIXME: Distortion here, we're trying to get 129 evenly spaced
-	 * samples from a LUT with 256 entries. We use 0, 2, 4 ... 254,
-	 * for the main palette, then entry 255 for this last register.
-	 */
 	/* Note that these registers _could_ take the LUT value of
 	 * 1024, but we're maxing out at 1023.984375 as it is easier. */
-	I915_WRITE(maxr_reg, intel_crtc->lut_r[255]);
-	I915_WRITE(maxg_reg, intel_crtc->lut_g[255]);
-	I915_WRITE(maxb_reg, intel_crtc->lut_b[255]);
+	I915_WRITE(maxr_reg, intel_crtc->lut_r[128]);
+	I915_WRITE(maxg_reg, intel_crtc->lut_g[128]);
+	I915_WRITE(maxb_reg, intel_crtc->lut_b[128]);
 }
 
 static int intel_crtc_cursor_set(struct drm_crtc *crtc,
@@ -3649,10 +3643,10 @@ static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int i;
 
-	if (size != 256)
+	if (size != 129)
 		return;
 
-	for (i = 0; i < 256; i++) {
+	for (i = 0; i < 129; i++) {
 		intel_crtc->lut_r[i] = red[i];
 		intel_crtc->lut_g[i] = green[i];
 		intel_crtc->lut_b[i] = blue[i];
@@ -4309,13 +4303,13 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 
 	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
 
-	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
+	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 129);
 	intel_crtc->pipe = pipe;
 	intel_crtc->plane = pipe;
-	for (i = 0; i < 256; i++) {
-		intel_crtc->lut_r[i] = i << 8 | i;
-		intel_crtc->lut_g[i] = i << 8 | i;
-		intel_crtc->lut_b[i] = i << 8 | i;
+	for (i = 0; i < 129; i++) {
+		intel_crtc->lut_r[i] = (u16)((int)0xFFFF * i / 128);
+		intel_crtc->lut_g[i] = (u16)((int)0xFFFF * i / 128);
+		intel_crtc->lut_b[i] = (u16)((int)0xFFFF * i / 128);
 	}
 
 	/* Swap pipes & planes for FBC on pre-965 */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 6f09806..cbd3c50 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -143,7 +143,7 @@ struct intel_crtc {
 	enum plane plane;
 	struct drm_gem_object *cursor_bo;
 	uint32_t cursor_addr;
-	u16 lut_r[256], lut_g[256], lut_b[256];
+	u16 lut_r[129], lut_g[129], lut_b[129];
 	int dpms_mode;
 	bool busy; /* is scanout buffer being updated frequently? */
 	struct timer_list idle_timer;
-- 
1.7.0.4

Re: [PATCH 3/3] drm/intel: Use 10-bit palette properly, only store 129 entries

[Adam Jackson]

[-- Attachment #1.1: Type: text/plain, Size: 1235 bytes --]

On Mon, 2010-04-26 at 23:24 +0100, Peter Clifton wrote:
> ---
>  drivers/gpu/drm/i915/intel_display.c |   38 ++++++++++++++-------------------
>  drivers/gpu/drm/i915/intel_drv.h     |    2 +-
>  2 files changed, 17 insertions(+), 23 deletions(-)

Nak.

This will break DirectColor visuals in X.  DirectColor assumes that
there's a straight lookup table for each color channel.  If the red
channel of a pixel is 0 in DirectColor, then the actual red value output
will be whatever value is in the 0th slot of the gamma table.  You don't
have 256 stops in 10-bit-gamma mode, so you can't do 8 bits per channel
DirectColor.

Even for TrueColor, which is what normal people expect, this will break
gamma setup entirely.  Userspace is currently hardcoded to use a gamma
size of 256, and:

> @@ -3649,10 +3643,10 @@ static void intel_crtc_gamma_set(struct
> drm_crtc *crtc, u16 *red, u16 *green,
>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>         int i;
>  
> -       if (size != 256)
> +       if (size != 129)
>                 return;

This is why I said gen4 should probably only expose the 129-stop gamma
for 30bpp framebuffers in X, and then only expose TrueColor visuals.

- ajax

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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