[PATCH] drm/i915: Fix offset page-flips on i965+

[PATCH] drm/i915: Fix offset page-flips on i965+

[Chris Wilson]

i965 uses the Display Registers to compute the offset from the display
base so the new base does not need adjusting when flipping. The older
chipsets use a fence to access the display and so do perceive the
surface as linear and have a single base register which is reprogrammed
using the flip.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Reported-by: Marty Jack <martyj19@comcast.net>
---
 drivers/gpu/drm/i915/intel_display.c |   40 ++++++++++++++++++++-------------
 1 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5596a27..2bb3196 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5028,9 +5028,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_unpin_work *work;
 	unsigned long flags, offset;
-	int pipesrc_reg = (intel_crtc->pipe == 0) ? PIPEASRC : PIPEBSRC;
-	int ret, pipesrc;
-	u32 flip_mask;
+	int ret;
 
 	work = kzalloc(sizeof *work, GFP_KERNEL);
 	if (work == NULL)
@@ -5079,12 +5077,14 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	atomic_inc(&obj_priv->pending_flip);
 	work->pending_flip_obj = obj;
 
-	if (intel_crtc->plane)
-		flip_mask = MI_WAIT_FOR_PLANE_B_FLIP;
-	else
-		flip_mask = MI_WAIT_FOR_PLANE_A_FLIP;
-
 	if (IS_GEN3(dev) || IS_GEN2(dev)) {
+		u32 flip_mask;
+
+		if (intel_crtc->plane)
+			flip_mask = MI_WAIT_FOR_PLANE_B_FLIP;
+		else
+			flip_mask = MI_WAIT_FOR_PLANE_A_FLIP;
+
 		BEGIN_LP_RING(2);
 		OUT_RING(MI_WAIT_FOR_EVENT | flip_mask);
 		OUT_RING(0);
@@ -5092,28 +5092,36 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	}
 
 	/* Offset into the new buffer for cases of shared fbs between CRTCs */
-	offset = obj_priv->gtt_offset;
-	offset += (crtc->y * fb->pitch) + (crtc->x * (fb->bits_per_pixel) / 8);
+	offset = crtc->y * fb->pitch + crtc->x * fb->bits_per_pixel/8;
 
 	BEGIN_LP_RING(4);
 	if (IS_I965G(dev)) {
+		int pipe = intel_crtc->pipe;
+		u32 pf, pipesrc;
+
 		OUT_RING(MI_DISPLAY_FLIP |
 			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
-		OUT_RING(fb->pitch);
-		OUT_RING(offset | obj_priv->tiling_mode);
-		pipesrc = I915_READ(pipesrc_reg); 
-		OUT_RING(pipesrc & 0x0fff0fff);
+		OUT_RING(fb->pitch | obj_priv->tiling_mode);
+		/* i965+ uses the linear or tiled offsets from the
+		 * Display Registers (which do not change across a page-flip)
+		 * so we need only reprogram the base address.
+		 */
+		OUT_RING(obj_priv->gtt_offset);
+
+		pf = I915_READ(pipe == 0 ? PFA_CTL_1 : PFB_CTL_1) & PF_ENABLE;
+		pipesrc = I915_READ(pipe == 0 ? PIPEASRC : PIPEBSRC) & 0x0fff0fff;
+		OUT_RING(pf | pipesrc);
 	} else if (IS_GEN3(dev)) {
 		OUT_RING(MI_DISPLAY_FLIP_I915 |
 			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
 		OUT_RING(fb->pitch);
-		OUT_RING(offset);
+		OUT_RING(obj_priv->gtt_offset + offset);
 		OUT_RING(MI_NOOP);
 	} else {
 		OUT_RING(MI_DISPLAY_FLIP |
 			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
 		OUT_RING(fb->pitch);
-		OUT_RING(offset);
+		OUT_RING(obj_priv->gtt_offset + offset);
 		OUT_RING(MI_NOOP);
 	}
 	ADVANCE_LP_RING();
-- 
1.7.1

Re: [PATCH] drm/i915: Fix offset page-flips on i965+

[Chris Wilson]

On Sun,  8 Aug 2010 10:20:25 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>  		OUT_RING(MI_DISPLAY_FLIP |
>  			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
> -		OUT_RING(fb->pitch);
> -		OUT_RING(offset | obj_priv->tiling_mode);
> -		pipesrc = I915_READ(pipesrc_reg); 
> -		OUT_RING(pipesrc & 0x0fff0fff);
> +		OUT_RING(fb->pitch | obj_priv->tiling_mode);
> +		/* i965+ uses the linear or tiled offsets from the
> +		 * Display Registers (which do not change across a page-flip)
> +		 * so we need only reprogram the base address.
> +		 */
> +		OUT_RING(obj_priv->gtt_offset);

There's a discrepancy here between our internal docs and the publish PDFs
for MI_DISPLAY_FLIP. In our docs, we have pitch | tiling. In the
PDF, it's address | tiling. There are other inconsistencies in the
bitfields surround this in the PDF, so it is worth a second set of eyes
checking this.

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Fix offset page-flips on i965+

[Daniel Vetter]

On Sun, Aug 08, 2010 at 12:39:00PM +0100, Chris Wilson wrote:
> On Sun,  8 Aug 2010 10:20:25 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >  		OUT_RING(MI_DISPLAY_FLIP |
> >  			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
> > -		OUT_RING(fb->pitch);
> > -		OUT_RING(offset | obj_priv->tiling_mode);
> > -		pipesrc = I915_READ(pipesrc_reg); 
> > -		OUT_RING(pipesrc & 0x0fff0fff);
> > +		OUT_RING(fb->pitch | obj_priv->tiling_mode);
> > +		/* i965+ uses the linear or tiled offsets from the
> > +		 * Display Registers (which do not change across a page-flip)
> > +		 * so we need only reprogram the base address.
> > +		 */
> > +		OUT_RING(obj_priv->gtt_offset);
> 
> There's a discrepancy here between our internal docs and the publish PDFs
> for MI_DISPLAY_FLIP. In our docs, we have pitch | tiling. In the
> PDF, it's address | tiling. There are other inconsistencies in the
> bitfields surround this in the PDF, so it is worth a second set of eyes
> checking this.

Just fyi, here's what I've distilled from the published docs. In short,
it's mess.

i965/G45 documentation:
dword 0, bit 22:
"This field specifies whether the flip operation should be performed
asynchronously to vertical retrace."

But then goes on to define:
0 = Asynchronous flip, synchronize with vblank.
1 = Syncrhronous flip, do "as soon as possible"
This seems to be the definition actually used (synchronous wrt command
execution), not the one in the intro (synchronous wrt vblank).

Whatever, current code sets this bit to 0. Docs then mention that for
asynchrouns flips, pitch and tiling mode get ignored (but should be set to
the same values for all flips). Pitch is in dword 1, tiling mode dword 2,
bit 0.

HD documentation:
dword0, bit 22: Should always be set to 1 due to hw limitations. Current
code is not doing this.

Sync/async seem to mean the same as in the i965/G45 docs, save for
dword 2, bit 0: (This is the old tiling mode bit that get's ignored)
DevSNB+, Flip type
0 = Sync Flip (synced to vblank)
1 = Async Flip (immediatly)
It also mentions that this only works for X-Tiled buffers (probably only
the immediate flip, for the rest tiling is most likely ignored, anyway).
So the tiling_mode be seems unnecessary and could hence be recycled.

Current code doesn't special-case ilk, so I think this only applies to
Sandybridge and later (the set dword 0, bit 22 always to 1, too). After
all, my i5 can do pageflips ;)

In conclusion I think we need an if (IS_SNB(dev)) that sets dword 0, bit
22 to 1 and ensures that dword 2, bit 0 is zero. For the rest of of the
IS_I965G branch we might as well write 0 instead of pitch and tiling_mode.

Whatever, current code seems to work.

-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: Fix offset page-flips on i965+

[Daniel Vetter]

On Sun, Aug 08, 2010 at 02:24:11PM +0200, Daniel Vetter wrote:
> In conclusion I think we need an if (IS_SNB(dev)) that sets dword 0, bit
> 22 to 1 and ensures that dword 2, bit 0 is zero. For the rest of of the
> IS_I965G branch we might as well write 0 instead of pitch and tiling_mode.

I've played around a bit and it looks like everyone is wrong (minus the
current code;). Tested on my ilk (thinkpad t410) ymmv.

Not setting pitch or tiling_mode is a bad idea and results in garbage on
the screen. So hw does indeed read this values (despite what the docs seem
to claim). But at least for my ilk here, published docs are correct: Or'ing
it in with the pitch results in garbage, too.

The original patch is therefore broken, 'cause it moves around the
tiling_mode bit (with mentioning this in the changelog).

I suspect something changed with sandybridge and whoever wrote the docs
made a mess out of it.

-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH] drm/i915: gen4 vs gen5 DISPLAY_FLIP discrepancy?

[Chris Wilson]

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bdd04ae..cd606e5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5120,7 +5120,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	offset = crtc->y * fb->pitch + crtc->x * fb->bits_per_pixel/8;
 
 	BEGIN_LP_RING(4);
-	if (IS_I965G(dev)) {
+	if (IS_GEN5(dev)) {
 		int pipe = intel_crtc->pipe;
 		u32 pf, pipesrc;
 
@@ -5136,6 +5136,18 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 		pf = I915_READ(pipe == 0 ? PFA_CTL_1 : PFB_CTL_1) & PF_ENABLE;
 		pipesrc = I915_READ(pipe == 0 ? PIPEASRC : PIPEBSRC) & 0x0fff0fff;
 		OUT_RING(pf | pipesrc);
+	} else if (IS_I965G(dev)) {
+		int pipe = intel_crtc->pipe;
+		u32 pf, pipesrc;
+
+		OUT_RING(MI_DISPLAY_FLIP |
+			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
+		OUT_RING(fb->pitch);
+		OUT_RING(obj_priv->gtt_offset| obj_priv->tiling_mode);
+
+		pf = I915_READ(pipe == 0 ? PFA_CTL_1 : PFB_CTL_1) & PF_ENABLE;
+		pipesrc = I915_READ(pipe == 0 ? PIPEASRC : PIPEBSRC) & 0x0fff0fff;
+		OUT_RING(pf | pipesrc);
 	} else if (IS_GEN3(dev)) {
 		OUT_RING(MI_DISPLAY_FLIP_I915 |
 			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
-- 
1.7.1

Re: [PATCH] drm/i915: Fix offset page-flips on i965+

[Jesse Barnes]

On Sun,  8 Aug 2010 10:20:25 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> i965 uses the Display Registers to compute the offset from the display
> base so the new base does not need adjusting when flipping. The older
> chipsets use a fence to access the display and so do perceive the
> surface as linear and have a single base register which is reprogrammed
> using the flip.

Mostly correct, we should get this in quickly to fix
https://bugs.freedesktop.org/show_bug.cgi?id=28518 (which is my fault;
I tested on 945 and assumed the fix applied to all future generations.
Of course I should have realized the hw guys like to keep us on our
toes so they keep changing register and command layouts on us).

Comments below.

> -	if (intel_crtc->plane)
> -		flip_mask = MI_WAIT_FOR_PLANE_B_FLIP;
> -	else
> -		flip_mask = MI_WAIT_FOR_PLANE_A_FLIP;
> -
>  	if (IS_GEN3(dev) || IS_GEN2(dev)) {
> +		u32 flip_mask;
> +
> +		if (intel_crtc->plane)
> +			flip_mask = MI_WAIT_FOR_PLANE_B_FLIP;
> +		else
> +			flip_mask = MI_WAIT_FOR_PLANE_A_FLIP;
> +
>  		BEGIN_LP_RING(2);
>  		OUT_RING(MI_WAIT_FOR_EVENT | flip_mask);
>  		OUT_RING(0);
> @@ -5092,28 +5092,36 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	}

Nice cleanup.

>  
>  	/* Offset into the new buffer for cases of shared fbs between CRTCs */
> -	offset = obj_priv->gtt_offset;
> -	offset += (crtc->y * fb->pitch) + (crtc->x * (fb->bits_per_pixel) / 8);
> +	offset = crtc->y * fb->pitch + crtc->x * fb->bits_per_pixel/8;

Yep.

>  
>  	BEGIN_LP_RING(4);
>  	if (IS_I965G(dev)) {
> +		int pipe = intel_crtc->pipe;
> +		u32 pf, pipesrc;
> +
>  		OUT_RING(MI_DISPLAY_FLIP |
>  			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
> -		OUT_RING(fb->pitch);
> -		OUT_RING(offset | obj_priv->tiling_mode);
> -		pipesrc = I915_READ(pipesrc_reg); 
> -		OUT_RING(pipesrc & 0x0fff0fff);
> +		OUT_RING(fb->pitch | obj_priv->tiling_mode);
> +		/* i965+ uses the linear or tiled offsets from the
> +		 * Display Registers (which do not change across a page-flip)
> +		 * so we need only reprogram the base address.
> +		 */
> +		OUT_RING(obj_priv->gtt_offset);
> +
> +		pf = I915_READ(pipe == 0 ? PFA_CTL_1 : PFB_CTL_1) & PF_ENABLE;
> +		pipesrc = I915_READ(pipe == 0 ? PIPEASRC : PIPEBSRC) & 0x0fff0fff;
> +		OUT_RING(pf | pipesrc);

Should be
	OUT_RING(fb->pitch);
	OUT_RING(obj_priv->gtt_offset | obj_priv->tiling_mode);
(as discussed on IRC the fields are documented incorrectly in some
places and the tiling param is required). Also I'm worried about the
panel fitting bits; I know ILK has more flexible panel fitting, but I'd
like to see this tested on 965 and GM45 with a non-native mode to be
sure these bits also work there.

>  	} else if (IS_GEN3(dev)) {
>  		OUT_RING(MI_DISPLAY_FLIP_I915 |
>  			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>  		OUT_RING(fb->pitch);
> -		OUT_RING(offset);
> +		OUT_RING(obj_priv->gtt_offset + offset);
>  		OUT_RING(MI_NOOP);
>  	} else {
>  		OUT_RING(MI_DISPLAY_FLIP |
>  			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>  		OUT_RING(fb->pitch);
> -		OUT_RING(offset);
> +		OUT_RING(obj_priv->gtt_offset + offset);
>  		OUT_RING(MI_NOOP);
>  	}
>  	ADVANCE_LP_RING();

These bits look good.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 1/2] drm/i915: Include a generation number in the device info

[Chris Wilson]

To simplify the IS_GEN[234] macros and to enable switching.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.c |   61 ++++++++++++++++++---------------------
 drivers/gpu/drm/i915/i915_drv.h |   27 ++++-------------
 2 files changed, 34 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5044f65..8cc0b50 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -61,91 +61,86 @@ extern int intel_agp_enabled;
 	.driver_data = (unsigned long) info }
 
 static const struct intel_device_info intel_i830_info = {
-	.is_i8xx = 1, .is_mobile = 1, .cursor_needs_physical = 1,
+	.gen = 2, .is_i8xx = 1, .is_mobile = 1, .cursor_needs_physical = 1,
 };
 
 static const struct intel_device_info intel_845g_info = {
-	.is_i8xx = 1,
+	.gen = 2, .is_i8xx = 1,
 };
 
 static const struct intel_device_info intel_i85x_info = {
-	.is_i8xx = 1, .is_i85x = 1, .is_mobile = 1,
+	.gen = 2, .is_i8xx = 1, .is_i85x = 1, .is_mobile = 1,
 	.cursor_needs_physical = 1,
 };
 
 static const struct intel_device_info intel_i865g_info = {
-	.is_i8xx = 1,
+	.gen = 2, .is_i8xx = 1,
 };
 
 static const struct intel_device_info intel_i915g_info = {
-	.is_i915g = 1, .is_i9xx = 1, .cursor_needs_physical = 1,
+	.gen = 3, .is_i915g = 1, .is_i9xx = 1, .cursor_needs_physical = 1,
 };
 static const struct intel_device_info intel_i915gm_info = {
-	.is_i9xx = 1,  .is_mobile = 1,
+	.gen = 3, .is_i9xx = 1,  .is_mobile = 1,
 	.cursor_needs_physical = 1,
 };
 static const struct intel_device_info intel_i945g_info = {
-	.is_i9xx = 1, .has_hotplug = 1, .cursor_needs_physical = 1,
+	.gen = 3, .is_i9xx = 1, .has_hotplug = 1, .cursor_needs_physical = 1,
 };
 static const struct intel_device_info intel_i945gm_info = {
-	.is_i945gm = 1, .is_i9xx = 1, .is_mobile = 1,
+	.gen = 3, .is_i945gm = 1, .is_i9xx = 1, .is_mobile = 1,
 	.has_hotplug = 1, .cursor_needs_physical = 1,
 };
 
 static const struct intel_device_info intel_i965g_info = {
-	.is_broadwater = 1, .is_i965g = 1, .is_i9xx = 1, .has_hotplug = 1,
+	.gen = 4, .is_broadwater = 1, .is_i965g = 1, .is_i9xx = 1,
+	.has_hotplug = 1,
 };
 
 static const struct intel_device_info intel_i965gm_info = {
-	.is_crestline = 1, .is_i965g = 1, .is_i965gm = 1, .is_i9xx = 1,
-	.is_mobile = 1, .has_fbc = 1, .has_rc6 = 1,
-	.has_hotplug = 1,
+	.gen = 4, .is_crestline = 1, .is_i965g = 1, .is_i965gm = 1, .is_i9xx = 1,
+	.is_mobile = 1, .has_fbc = 1, .has_rc6 = 1, .has_hotplug = 1,
 };
 
 static const struct intel_device_info intel_g33_info = {
-	.is_g33 = 1, .is_i9xx = 1, .need_gfx_hws = 1,
-	.has_hotplug = 1,
+	.gen = 3, .is_g33 = 1, .is_i9xx = 1,
+	.need_gfx_hws = 1, .has_hotplug = 1,
 };
 
 static const struct intel_device_info intel_g45_info = {
-	.is_i965g = 1, .is_g4x = 1, .is_i9xx = 1, .need_gfx_hws = 1,
-	.has_pipe_cxsr = 1,
-	.has_hotplug = 1,
+	.gen = 4, .is_i965g = 1, .is_g4x = 1, .is_i9xx = 1, .need_gfx_hws = 1,
+	.has_pipe_cxsr = 1, .has_hotplug = 1,
 };
 
 static const struct intel_device_info intel_gm45_info = {
-	.is_i965g = 1, .is_g4x = 1, .is_i9xx = 1,
+	.gen = 4, .is_i965g = 1, .is_g4x = 1, .is_i9xx = 1,
 	.is_mobile = 1, .need_gfx_hws = 1, .has_fbc = 1, .has_rc6 = 1,
-	.has_pipe_cxsr = 1,
-	.has_hotplug = 1,
+	.has_pipe_cxsr = 1, .has_hotplug = 1,
 };
 
 static const struct intel_device_info intel_pineview_info = {
-	.is_g33 = 1, .is_pineview = 1, .is_mobile = 1, .is_i9xx = 1,
-	.need_gfx_hws = 1,
-	.has_hotplug = 1,
+	.gen = 3, .is_g33 = 1, .is_pineview = 1, .is_mobile = 1, .is_i9xx = 1,
+	.need_gfx_hws = 1, .has_hotplug = 1,
 };
 
 static const struct intel_device_info intel_ironlake_d_info = {
-	.is_ironlake = 1, .is_i965g = 1, .is_i9xx = 1, .need_gfx_hws = 1,
-	.has_pipe_cxsr = 1,
-	.has_hotplug = 1,
+	.gen = 5, .is_ironlake = 1, .is_i965g = 1, .is_i9xx = 1,
+	.need_gfx_hws = 1, .has_pipe_cxsr = 1, .has_hotplug = 1,
 };
 
 static const struct intel_device_info intel_ironlake_m_info = {
-	.is_ironlake = 1, .is_mobile = 1, .is_i965g = 1, .is_i9xx = 1,
-	.need_gfx_hws = 1, .has_fbc = 1, .has_rc6 = 1,
-	.has_hotplug = 1,
+	.gen = 5, .is_ironlake = 1, .is_mobile = 1, .is_i965g = 1, .is_i9xx = 1,
+	.need_gfx_hws = 1, .has_fbc = 1, .has_rc6 = 1, .has_hotplug = 1,
 };
 
 static const struct intel_device_info intel_sandybridge_d_info = {
-	.is_i965g = 1, .is_i9xx = 1, .need_gfx_hws = 1,
-	.has_hotplug = 1, .is_gen6 = 1,
+	.gen = 6, .is_i965g = 1, .is_i9xx = 1,
+	.need_gfx_hws = 1, .has_hotplug = 1,
 };
 
 static const struct intel_device_info intel_sandybridge_m_info = {
-	.is_i965g = 1, .is_mobile = 1, .is_i9xx = 1, .need_gfx_hws = 1,
-	.has_hotplug = 1, .is_gen6 = 1,
+	.gen = 6, .is_i965g = 1, .is_mobile = 1, .is_i9xx = 1,
+	.need_gfx_hws = 1, .has_hotplug = 1,
 };
 
 static const struct pci_device_id pciidlist[] = {		/* aka */
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 90b65cd..442e72e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -191,6 +191,7 @@ struct drm_i915_display_funcs {
 };
 
 struct intel_device_info {
+	u8 gen;
 	u8 is_mobile : 1;
 	u8 is_i8xx : 1;
 	u8 is_i85x : 1;
@@ -206,7 +207,6 @@ struct intel_device_info {
 	u8 is_broadwater : 1;
 	u8 is_crestline : 1;
 	u8 is_ironlake : 1;
-	u8 is_gen6 : 1;
 	u8 has_fbc : 1;
 	u8 has_rc6 : 1;
 	u8 has_pipe_cxsr : 1;
@@ -1162,7 +1162,6 @@ extern void intel_overlay_print_error_state(struct seq_file *m, struct intel_ove
 #define IS_845G(dev)		((dev)->pci_device == 0x2562)
 #define IS_I85X(dev)		(INTEL_INFO(dev)->is_i85x)
 #define IS_I865G(dev)		((dev)->pci_device == 0x2572)
-#define IS_GEN2(dev)		(INTEL_INFO(dev)->is_i8xx)
 #define IS_I915G(dev)		(INTEL_INFO(dev)->is_i915g)
 #define IS_I915GM(dev)		((dev)->pci_device == 0x2592)
 #define IS_I945G(dev)		((dev)->pci_device == 0x2772)
@@ -1181,27 +1180,13 @@ extern void intel_overlay_print_error_state(struct seq_file *m, struct intel_ove
 #define IS_IRONLAKE_M(dev)	((dev)->pci_device == 0x0046)
 #define IS_IRONLAKE(dev)	(INTEL_INFO(dev)->is_ironlake)
 #define IS_I9XX(dev)		(INTEL_INFO(dev)->is_i9xx)
-#define IS_GEN6(dev)		(INTEL_INFO(dev)->is_gen6)
 #define IS_MOBILE(dev)		(INTEL_INFO(dev)->is_mobile)
 
-#define IS_GEN3(dev)	(IS_I915G(dev) ||			\
-			 IS_I915GM(dev) ||			\
-			 IS_I945G(dev) ||			\
-			 IS_I945GM(dev) ||			\
-			 IS_G33(dev) || \
-			 IS_PINEVIEW(dev))
-#define IS_GEN4(dev)	((dev)->pci_device == 0x2972 ||		\
-			 (dev)->pci_device == 0x2982 ||		\
-			 (dev)->pci_device == 0x2992 ||		\
-			 (dev)->pci_device == 0x29A2 ||		\
-			 (dev)->pci_device == 0x2A02 ||		\
-			 (dev)->pci_device == 0x2A12 ||		\
-			 (dev)->pci_device == 0x2E02 ||		\
-			 (dev)->pci_device == 0x2E12 ||		\
-			 (dev)->pci_device == 0x2E22 ||		\
-			 (dev)->pci_device == 0x2E32 ||		\
-			 (dev)->pci_device == 0x2A42 ||		\
-			 (dev)->pci_device == 0x2E42)
+#define IS_GEN2(dev)	(INTEL_INFO(dev)->gen == 2)
+#define IS_GEN3(dev)	(INTEL_INFO(dev)->gen == 3)
+#define IS_GEN4(dev)	(INTEL_INFO(dev)->gen == 4)
+#define IS_GEN5(dev)	(INTEL_INFO(dev)->gen == 5)
+#define IS_GEN6(dev)	(INTEL_INFO(dev)->gen == 6)
 
 #define HAS_BSD(dev)            (IS_IRONLAKE(dev) || IS_G4X(dev))
 #define I915_NEED_GFX_HWS(dev)	(INTEL_INFO(dev)->need_gfx_hws)
-- 
1.7.1

Re: [PATCH 1/2] drm/i915: Include a generation number in the device info

[Adam Jackson]

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

On Wed, 2010-08-11 at 10:25 +0100, Chris Wilson wrote:
> To simplify the IS_GEN[234] macros and to enable switching.

I think your three cent titanium tax doesn't go too far enough.

830, 845g, 85x, and 865 all now have

    .gen = 2,
    .is_i8xx = 1,

and nothing later has .is_i8xx, so that field can just go.  Indeed you
do:

>  #define IS_I865G(dev)		((dev)->pci_device == 0x2572)
> -#define IS_GEN2(dev)		(INTEL_INFO(dev)->is_i8xx)
>  #define IS_I915G(dev)		(INTEL_INFO(dev)->is_i915g)

which appears to be the only reference.

Likewise, all the gen[3456] parts have .is_i9xx = 1, so you could do:

-#define IS_I9XX(dev) (INTEL_INFO(dev)->is_i9xx)
+#define IS_I9XX(dev) (INTEL_INFO(dev)->gen > 2)

and drop .is_i9xx.

- 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: [PATCH 1/2] drm/i915: Include a generation number in the device info

[Chris Wilson]

On Wed, 11 Aug 2010 10:14:44 -0400, Adam Jackson <ajax@redhat.com> wrote:
> On Wed, 2010-08-11 at 10:25 +0100, Chris Wilson wrote:
> > To simplify the IS_GEN[234] macros and to enable switching.
> 
> I think your three cent titanium tax doesn't go too far enough.
> 
> 830, 845g, 85x, and 865 all now have
> 
>     .gen = 2,
>     .is_i8xx = 1,
> 
> and nothing later has .is_i8xx, so that field can just go.

Yes. I was just worried about some subtle semantic difference between
is_i8xx and is_i9xx and the generation number. I still look at IS_I965G()
and shudder. So for the first patch, I wanted something obvious and simple
that gave me the ability to switch(INTEL_INFO(dev)->gen). I'd like to
replace some of the coarse is_i9xx with more fine-grained capability bits
that are more self-descriptive and clear when they can be relied upon.

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 2/2] drm/i915: Fix offset page-flips on i965+

[Chris Wilson]

i965 uses the Display Registers to compute the offset from the display
base so the new base does not need adjusting when flipping. The older
chipsets use a fence to access the display and so do perceive the
surface as linear and have a single base register which is reprogrammed
using the flip.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Reported-by: Marty Jack <martyj19@comcast.net>
---
 drivers/gpu/drm/i915/intel_display.c |   67 ++++++++++++++++++++++++----------
 1 files changed, 48 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6ae4f69..d5dd86f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5063,9 +5063,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_unpin_work *work;
 	unsigned long flags, offset;
-	int pipesrc_reg = (intel_crtc->pipe == 0) ? PIPEASRC : PIPEBSRC;
-	int ret, pipesrc;
-	u32 flip_mask;
+	int pipe = intel_crtc->pipe;
+	u32 pf, pipesrc;
+	int ret;
 
 	work = kzalloc(sizeof *work, GFP_KERNEL);
 	if (work == NULL)
@@ -5114,12 +5114,14 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	atomic_inc(&obj_priv->pending_flip);
 	work->pending_flip_obj = obj;
 
-	if (intel_crtc->plane)
-		flip_mask = MI_WAIT_FOR_PLANE_B_FLIP;
-	else
-		flip_mask = MI_WAIT_FOR_PLANE_A_FLIP;
-
 	if (IS_GEN3(dev) || IS_GEN2(dev)) {
+		u32 flip_mask;
+
+		if (intel_crtc->plane)
+			flip_mask = MI_WAIT_FOR_PLANE_B_FLIP;
+		else
+			flip_mask = MI_WAIT_FOR_PLANE_A_FLIP;
+
 		BEGIN_LP_RING(2);
 		OUT_RING(MI_WAIT_FOR_EVENT | flip_mask);
 		OUT_RING(0);
@@ -5127,29 +5129,56 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	}
 
 	/* Offset into the new buffer for cases of shared fbs between CRTCs */
-	offset = obj_priv->gtt_offset;
-	offset += (crtc->y * fb->pitch) + (crtc->x * (fb->bits_per_pixel) / 8);
+	offset = crtc->y * fb->pitch + crtc->x * fb->bits_per_pixel/8;
 
 	BEGIN_LP_RING(4);
-	if (IS_I965G(dev)) {
+	switch(INTEL_INFO(dev)->gen) {
+	case 2:
 		OUT_RING(MI_DISPLAY_FLIP |
 			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
 		OUT_RING(fb->pitch);
-		OUT_RING(offset | obj_priv->tiling_mode);
-		pipesrc = I915_READ(pipesrc_reg); 
-		OUT_RING(pipesrc & 0x0fff0fff);
-	} else if (IS_GEN3(dev)) {
+		OUT_RING(obj_priv->gtt_offset + offset);
+		OUT_RING(MI_NOOP);
+		break;
+
+	case 3:
 		OUT_RING(MI_DISPLAY_FLIP_I915 |
 			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
 		OUT_RING(fb->pitch);
-		OUT_RING(offset);
+		OUT_RING(obj_priv->gtt_offset + offset);
 		OUT_RING(MI_NOOP);
-	} else {
+		break;
+
+	case 4:
+	case 5:
+		/* i965+ uses the linear or tiled offsets from the
+		 * Display Registers (which do not change across a page-flip)
+		 * so we need only reprogram the base address.
+		 */
 		OUT_RING(MI_DISPLAY_FLIP |
 			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
 		OUT_RING(fb->pitch);
-		OUT_RING(offset);
-		OUT_RING(MI_NOOP);
+		OUT_RING(obj_priv->gtt_offset | obj_priv->tiling_mode);
+
+		/* XXX Enabling the panel-fitter across page-flip is so far
+		 * untested on non-native modes, so ignore it for now.
+		 * pf = I915_READ(pipe == 0 ? PFA_CTL_1 : PFB_CTL_1) & PF_ENABLE;
+		 */
+		pf = 0;
+		pipesrc = I915_READ(pipe == 0 ? PIPEASRC : PIPEBSRC) & 0x0fff0fff;
+		OUT_RING(pf | pipesrc);
+		break;
+
+	case 6:
+		OUT_RING(MI_DISPLAY_FLIP |
+			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
+		OUT_RING(fb->pitch | obj_priv->tiling_mode);
+		OUT_RING(obj_priv->gtt_offset);
+
+		pf = I915_READ(pipe == 0 ? PFA_CTL_1 : PFB_CTL_1) & PF_ENABLE;
+		pipesrc = I915_READ(pipe == 0 ? PIPEASRC : PIPEBSRC) & 0x0fff0fff;
+		OUT_RING(pf | pipesrc);
+		break;
 	}
 	ADVANCE_LP_RING();
 
-- 
1.7.1

Re: [PATCH 2/2] drm/i915: Fix offset page-flips on i965+

[Jesse Barnes]

On Wed, 11 Aug 2010 10:25:47 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> i965 uses the Display Registers to compute the offset from the display
> base so the new base does not need adjusting when flipping. The older
> chipsets use a fence to access the display and so do perceive the
> surface as linear and have a single base register which is reprogrammed
> using the flip.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Reported-by: Marty Jack <martyj19@comcast.net>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   67 ++++++++++++++++++++++++----------
>  1 files changed, 48 insertions(+), 19 deletions(-)

Looks good.  Marty, any chance you could reply with a tested-by?

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center