| Message ID | 1307487281-3015-1-git-send-email-kenneth@whitecape.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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 :-)
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
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 --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();
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(-)