diff mbox

[1/3] drm/i915: Fix MI_DISPLAY_FLIP plane select offset on Ivybridge.

Message ID 1307487281-3015-1-git-send-email-kenneth@whitecape.org
State New, archived
Headers show

Commit Message

Kenneth Graunke June 7, 2011, 10:54 p.m. UTC
According to BSpec volume 1c.4 section 3.2.9, Display (Plane) Select is
now at bits 21:19 instead of 21:20.

Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
---
 drivers/gpu/drm/i915/intel_display.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

Comments

Keith Packard June 7, 2011, 11:14 p.m. UTC | #1
On Tue,  7 Jun 2011 15:54:39 -0700, Kenneth Graunke <kenneth@whitecape.org> wrote:
> According to BSpec volume 1c.4 section 3.2.9, Display (Plane) Select is
> now at bits 21:19 instead of 21:20.
> 
> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>

I will note that the docs have an obvious bug -- 21:8 are 'reserved' on
IVB while 21:19 are 'Display (Plane) Select'. I trust you've actually
tried this on hardware and noticed that it works better now?


> +
> +	case 7:
> +		OUT_RING(MI_DISPLAY_FLIP | (intel_crtc->plane << 19));
> +		OUT_RING(fb->pitch | obj->tiling_mode);
> +		OUT_RING(obj->gtt_offset);
> +
> +		pf = I915_READ(PF_CTL(pipe)) & PF_ENABLE;
> +		pipesrc = I915_READ(PIPESRC(pipe)) & 0x0fff0fff;
> +		OUT_RING(pf | pipesrc);

What's this last DWORD supposed to be for? The IVB spec says length
should be '1' and there should be only 3 DWORDS in this command.
Kenneth Graunke June 8, 2011, 12:55 a.m. UTC | #2
On 06/07/2011 04:14 PM, Keith Packard wrote:
> On Tue,  7 Jun 2011 15:54:39 -0700, Kenneth Graunke<kenneth@whitecape.org>  wrote:
>> According to BSpec volume 1c.4 section 3.2.9, Display (Plane) Select is
>> now at bits 21:19 instead of 21:20.
>>
>> Signed-off-by: Kenneth Graunke<kenneth@whitecape.org>
>
> I will note that the docs have an obvious bug -- 21:8 are 'reserved' on
> IVB while 21:19 are 'Display (Plane) Select'.

In the latest version of the Render Engine Command Streamer chapter, 
18:8 are 'reserved' on IVB.  Perhaps you have an old copy?

Apparently MI_DISPLAY_FLIP also exists in the Blitter Engine on IVB.  I 
suspect we actually need to use that, but I haven't tried to yet.

> I trust you've actually
> tried this on hardware and noticed that it works better now?

No, actually...page flips are still broken.  I suspect this patch is 
necessary but insufficient.  Feel free to hold off on merging it.

>> +
>> +	case 7:
>> +		OUT_RING(MI_DISPLAY_FLIP | (intel_crtc->plane<<  19));
>> +		OUT_RING(fb->pitch | obj->tiling_mode);
>> +		OUT_RING(obj->gtt_offset);
>> +
>> +		pf = I915_READ(PF_CTL(pipe))&  PF_ENABLE;
>> +		pipesrc = I915_READ(PIPESRC(pipe))&  0x0fff0fff;
>> +		OUT_RING(pf | pipesrc);
>
> What's this last DWORD supposed to be for? The IVB spec says length
> should be '1' and there should be only 3 DWORDS in this command.

Good question.  My reading of the docs say that it should be 3 DWORDs on 
SNB as well; this code was directly lifted from "case 6" above (save the 
first line).

--Kenneth
Keith Packard June 8, 2011, 1:09 a.m. UTC | #3
On Tue, 07 Jun 2011 17:55:05 -0700, Kenneth Graunke <kenneth@whitecape.org> wrote:

> In the latest version of the Render Engine Command Streamer chapter, 
> 18:8 are 'reserved' on IVB.  Perhaps you have an old copy?

Yeah, I haven't sync'd for a week or so.

> Apparently MI_DISPLAY_FLIP also exists in the Blitter Engine on IVB.  I 
> suspect we actually need to use that, but I haven't tried to yet.

> No, actually...page flips are still broken.  I suspect this patch is 
> necessary but insufficient.  Feel free to hold off on merging it.

Sounds like a bit more testing and fixing is required; I'll leave things
alone until we know how to make it work.

> Good question.  My reading of the docs say that it should be 3 DWORDs on 
> SNB as well; this code was directly lifted from "case 6" above (save the 
> first line).

Might be interesting to see if page flipping works when we send the
command correctly :-)
Kenneth Graunke June 8, 2011, 9:54 a.m. UTC | #4
On 06/08/2011 02:36 AM, Chris Wilson wrote:
> On Tue, 07 Jun 2011 17:55:05 -0700, Kenneth Graunke<kenneth@whitecape.org>  wrote:
>> On 06/07/2011 04:14 PM, Keith Packard wrote:
>>> What's this last DWORD supposed to be for? The IVB spec says length
>>> should be '1' and there should be only 3 DWORDS in this command.
>>
>> Good question.  My reading of the docs say that it should be 3 DWORDs on
>> SNB as well; this code was directly lifted from "case 6" above (save the
>> first line).
>
> In my ancient copy of the specs, MI_DISPLAY_FLIP is 4 dwords, with this
> last dword for programming the panel-fitter and pipesrc. And it also
> says that command is applicable to IVB, but as I said it is an ancient
> copy.
> -Chris

That's correct.  SNB /does/ include a 4th DWord as you describe---newer 
specs got mangled and lost that info, somehow.  Argh.

IVB does only use three DWords, so the last OUT_RING should be nixed. 
Removing that does change the behavior, but doesn't yet make it work.

Thanks!
--Kenneth
Chris Wilson June 8, 2011, 10:57 a.m. UTC | #5
On Wed, 08 Jun 2011 02:54:36 -0700, Kenneth Graunke <kenneth@whitecape.org> wrote:
> IVB does only use three DWords, so the last OUT_RING should be nixed. 
> Removing that does change the behavior, but doesn't yet make it work.

Note you can't just drop it since we need to keep the quadword
alignment, so shrink the command and add a trailing nop.
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 81a9059..60b2941 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6390,7 +6390,6 @@  static int intel_crtc_page_flip(struct drm_crtc *crtc,
 		break;
 
 	case 6:
-	case 7:
 		OUT_RING(MI_DISPLAY_FLIP |
 			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
 		OUT_RING(fb->pitch | obj->tiling_mode);
@@ -6400,6 +6399,16 @@  static int intel_crtc_page_flip(struct drm_crtc *crtc,
 		pipesrc = I915_READ(PIPESRC(pipe)) & 0x0fff0fff;
 		OUT_RING(pf | pipesrc);
 		break;
+
+	case 7:
+		OUT_RING(MI_DISPLAY_FLIP | (intel_crtc->plane << 19));
+		OUT_RING(fb->pitch | obj->tiling_mode);
+		OUT_RING(obj->gtt_offset);
+
+		pf = I915_READ(PF_CTL(pipe)) & PF_ENABLE;
+		pipesrc = I915_READ(PIPESRC(pipe)) & 0x0fff0fff;
+		OUT_RING(pf | pipesrc);
+		break;
 	}
 	ADVANCE_LP_RING();