Re: [PATCH] don't wait on page flips if none are pending
On Monday, June 23, 2008 11:50 pm Michel Dänzer wrote: > On Mon, 2008-06-23 at 11:02 -0700, Jesse Barnes wrote: > > On Monday, June 23, 2008 12:51 am Michel Dänzer wrote: > > > On Fri, 2008-06-20 at 14:27 -0700, Jesse Barnes wrote: > > > > On Friday, June 20, 2008 2:09 pm Jesse Barnes wrote: > > > > > Michel, can you take a look at this? vblank wait is working really > > > > > well for me with the latest bits, but I found what I think is a > > > > > page flip related bug on 965. > > > > > > No this isn't correct, it also needs to wait for scheduled non-flip > > > buffer swaps. > > > > > > Isn't this just a workaround for the lack of the 2D driver patch and > > > not necessary with it? > > > > I'm not sure what 2D patch you mean (page flipping for 965 I suppose), > > No, the modeset ioctl call patch. I thought the vbl_waited/pending > fields were getting initialized and the only problem was the lack of > that. > > > but clearly there's something wrong here since w/o this patch we end up > > doing an absolute wait on a 0 vblank count. :) Maybe I need to > > initialize the vbl_pending/vbl_waited values somewhere else instead? > > Either that, or just disable this code in LOCK_HARDWARE() for the i965 > driver I suppose, if it doesn't support scheduled buffer swaps. > > Note that I'm not sure about the status of intel page flipping and > scheduled swaps on the current master/gem branches, it may be better to > hold off on adding those features until things have settled down there. Yeah, I'm not sure either. A lot of nice consolidation has happened, so enabling scheduled swaps and page flipping on 965 doesn't seem too hard, but for 7.1 that may be a little aggressive, since that may uncover more issues than just the ones I found the other day. I'll do some more testing and see if I can convince myself things are in good enough shape, otherwise I'll just disable that code for 965 for now. Thanks, Jesse - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://sourceforge.net/services/buy/index.php -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] don't wait on page flips if none are pending
On Tue, 2008-06-24 at 08:50 +0200, Michel Dänzer wrote: > On Mon, 2008-06-23 at 11:02 -0700, Jesse Barnes wrote: > > On Monday, June 23, 2008 12:51 am Michel Dänzer wrote: > > > On Fri, 2008-06-20 at 14:27 -0700, Jesse Barnes wrote: > > > > On Friday, June 20, 2008 2:09 pm Jesse Barnes wrote: > > > > > > I've been testing with the attached pre- & post-modeset ioctl patch to > > > > > the 2D driver. > > > > > > That one looks good; if anything, maybe slightly too careful about > > > omitting redundant ioctl calls, but better safe than sorry I guess. :) > > > > > > > Oops, I've also been testing with this. > > > > > > Yeah, that driver workaround should definitely no longer be necessary. > > > > Ok, thanks for checking. Though in looking at the pre- & post-modeset code > > again, I'm not sure what we have is optimal. The main reason for it is to > > prevent the user visible vblank count from going backwards, so it seems > > like > > we could just add the pre-modeset value to the post-modeset value and be > > done > > with it, rather than treating mode set like a wraparound (which will cause > > our counter to wraparound much more quickly). > > It isn't always treated as a workaround. Only if the driver counter has > moved backwards does it compensate for the resulting spurious wraparound > in drm_update_vblank_count(). Forgot to add: This works as expected for me, whereas the previous pre/post-modeset handling didn't (the counter sometimes ended up so far off what it was supposed to be that vblank wait ioctls would have hit the 3 second timeout, except that never actually happens in the X server due to getting constantly interrupted by the smart scheduler's SIGALRM, resulting in an infinite loop). -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://sourceforge.net/services/buy/index.php -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] don't wait on page flips if none are pending
On Mon, 2008-06-23 at 11:02 -0700, Jesse Barnes wrote: > On Monday, June 23, 2008 12:51 am Michel Dänzer wrote: > > On Fri, 2008-06-20 at 14:27 -0700, Jesse Barnes wrote: > > > On Friday, June 20, 2008 2:09 pm Jesse Barnes wrote: > > > > Michel, can you take a look at this? vblank wait is working really > > > > well for me with the latest bits, but I found what I think is a page > > > > flip related bug on 965. > > > > No this isn't correct, it also needs to wait for scheduled non-flip > > buffer swaps. > > > > Isn't this just a workaround for the lack of the 2D driver patch and not > > necessary with it? > > I'm not sure what 2D patch you mean (page flipping for 965 I suppose), No, the modeset ioctl call patch. I thought the vbl_waited/pending fields were getting initialized and the only problem was the lack of that. > but clearly there's something wrong here since w/o this patch we end up doing > an > absolute wait on a 0 vblank count. :) Maybe I need to initialize the > vbl_pending/vbl_waited values somewhere else instead? Either that, or just disable this code in LOCK_HARDWARE() for the i965 driver I suppose, if it doesn't support scheduled buffer swaps. Note that I'm not sure about the status of intel page flipping and scheduled swaps on the current master/gem branches, it may be better to hold off on adding those features until things have settled down there. > > > > I've been testing with the attached pre- & post-modeset ioctl patch to > > > > the 2D driver. > > > > That one looks good; if anything, maybe slightly too careful about > > omitting redundant ioctl calls, but better safe than sorry I guess. :) > > > > > Oops, I've also been testing with this. > > > > Yeah, that driver workaround should definitely no longer be necessary. > > Ok, thanks for checking. Though in looking at the pre- & post-modeset code > again, I'm not sure what we have is optimal. The main reason for it is to > prevent the user visible vblank count from going backwards, so it seems like > we could just add the pre-modeset value to the post-modeset value and be done > with it, rather than treating mode set like a wraparound (which will cause > our counter to wraparound much more quickly). It isn't always treated as a workaround. Only if the driver counter has moved backwards does it compensate for the resulting spurious wraparound in drm_update_vblank_count(). -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://sourceforge.net/services/buy/index.php -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] don't wait on page flips if none are pending
On Monday, June 23, 2008 11:02 am Jesse Barnes wrote:
> On Monday, June 23, 2008 12:51 am Michel Dänzer wrote:
> > On Fri, 2008-06-20 at 14:27 -0700, Jesse Barnes wrote:
> > > On Friday, June 20, 2008 2:09 pm Jesse Barnes wrote:
> > > > Michel, can you take a look at this? vblank wait is working really
> > > > well for me with the latest bits, but I found what I think is a page
> > > > flip related bug on 965.
> >
> > No this isn't correct, it also needs to wait for scheduled non-flip
> > buffer swaps.
> >
> > Isn't this just a workaround for the lack of the 2D driver patch and not
> > necessary with it?
>
> I'm not sure what 2D patch you mean (page flipping for 965 I suppose), but
> clearly there's something wrong here since w/o this patch we end up doing
> an absolute wait on a 0 vblank count. :) Maybe I need to initialize the
> vbl_pending/vbl_waited values somewhere else instead? I'll poke around the
> page flipping & buffer swap code to see if I can make sense of it. Would
> be nice to have that feature available on 965, and preferably enable it by
> default if it works.
I guess this is what you meant? Looks like we need changes throughout the
whole stack to support page flipping on 965... Just tested this set of
patches, things seem to work ok, though I get pretty massive tearing on
glxgears by default, which I suppose is expected.
Thanks,
Jesse
diff --git a/shared-core/i915_dma.c b/shared-core/i915_dma.c
index 3f4fd1b..645213b 100644
--- a/shared-core/i915_dma.c
+++ b/shared-core/i915_dma.c
@@ -642,7 +642,7 @@ int i915_dispatch_batchbuffer(struct drm_device * dev,
static void i915_do_dispatch_flip(struct drm_device * dev, int plane, int sync)
{
drm_i915_private_t *dev_priv = dev->dev_private;
- u32 num_pages, current_page, next_page, dspbase;
+ u32 num_pages, current_page, next_page, base, offset;
int shift = 2 * plane, x, y;
RING_LOCALS;
@@ -654,13 +654,13 @@ static void i915_do_dispatch_flip(struct drm_device * dev, int plane, int sync)
switch (next_page) {
default:
case 0:
- dspbase = dev_priv->sarea_priv->front_offset;
+ base = dev_priv->sarea_priv->front_offset;
break;
case 1:
- dspbase = dev_priv->sarea_priv->back_offset;
+ base = dev_priv->sarea_priv->back_offset;
break;
case 2:
- dspbase = dev_priv->sarea_priv->third_offset;
+ base = dev_priv->sarea_priv->third_offset;
break;
}
@@ -672,20 +672,29 @@ static void i915_do_dispatch_flip(struct drm_device * dev, int plane, int sync)
y = dev_priv->sarea_priv->planeB_y;
}
- dspbase += (y * dev_priv->sarea_priv->pitch + x) * dev_priv->cpp;
+ offset = (y * dev_priv->sarea_priv->pitch + x) * dev_priv->cpp;
DRM_DEBUG("plane=%d current_page=%d dspbase=0x%x\n", plane, current_page,
- dspbase);
+ base + offset);
- BEGIN_LP_RING(4);
- OUT_RING(sync ? 0 :
- (MI_WAIT_FOR_EVENT | (plane ? MI_WAIT_FOR_PLANE_B_FLIP :
- MI_WAIT_FOR_PLANE_A_FLIP)));
- OUT_RING(CMD_OP_DISPLAYBUFFER_INFO | (sync ? 0 : ASYNC_FLIP) |
- (plane ? DISPLAY_PLANE_B : DISPLAY_PLANE_A));
- OUT_RING(dev_priv->sarea_priv->pitch * dev_priv->cpp);
- OUT_RING(dspbase);
- ADVANCE_LP_RING();
+ if (IS_I965G(dev)) {
+ BEGIN_LP_RING(4);
+ OUT_RING(MI_DISPLAY_FLIP | (sync ? 0 : DISPLAY_FLIP_ASYNC) |
+ (plane ? DISPLAY_FLIP_PLANE_B : 0));
+ OUT_RING(dev_priv->sarea_priv->pitch * dev_priv->cpp);
+ OUT_RING((base + offset) | DISPLAY_FLIP_TILED);
+ OUT_RING(x << DISPLAY_FLIP_HSIZE_SHIFT | y);
+ } else {
+ BEGIN_LP_RING(4);
+ OUT_RING(sync ? 0 :
+ (MI_WAIT_FOR_EVENT | (plane ? MI_WAIT_FOR_PLANE_B_FLIP :
+ MI_WAIT_FOR_PLANE_A_FLIP)));
+ OUT_RING(CMD_OP_DISPLAYBUFFER_INFO | (sync ? 0 : ASYNC_FLIP) |
+ (plane ? DISPLAY_PLANE_B : DISPLAY_PLANE_A));
+ OUT_RING(dev_priv->sarea_priv->pitch * dev_priv->cpp);
+ OUT_RING(base + offset);
+ ADVANCE_LP_RING();
+ }
dev_priv->sarea_priv->pf_current_page &= ~(0x3 << shift);
dev_priv->sarea_priv->pf_current_page |= next_page << shift;
diff --git a/src/i830_display.c b/src/i830_display.c
index 56a718d..04e5927 100644
--- a/src/i830_display.c
+++ b/src/i830_display.c
@@ -35,6 +35,7 @@
#include
#include
#include
+#include
#include "xf86.h"
#include "i830.h"
@@ -730,6 +731,40 @@ i830_use_fb_compression(xf86CrtcPtr crtc)
return TRUE;
}
+#if defined(DRM_IOCTL_MODESET_CTL) && defined(XF86DRI)
+static void i830_modeset_ctl(xf86CrtcPtr crtc, int pre)
+{
+ScrnInfoPtr pScrn = crtc->scrn;
+I830Ptr pI830 = I830PTR(pScrn);
+I830CrtcPrivatePtr intel_crtc = crtc->driver_private;
+struct drm_modeset_ctl modeset;
+
+modeset.crtc = intel_crtc->plane;
+return;
+
+/*
+ * DPMS will be called many times (especially off), but we only
+ * want to catch the transition from on->off and off->on.
+ */
+if (pre && intel_crtc->dpms_mode != DPMSModeOff) {
+ /* On -> off is a pre modeset */
+ modeset.cmd = _DRM_PRE_MODESET;
+ ioctl(pI830->drmSubFD, DRM_IOCTL_MODESET_CTL, &modeset);
+ ErrorF("modeset: on -> off on plane %d\n",
Re: [PATCH] don't wait on page flips if none are pending
On Monday, June 23, 2008 12:51 am Michel Dänzer wrote: > On Fri, 2008-06-20 at 14:27 -0700, Jesse Barnes wrote: > > On Friday, June 20, 2008 2:09 pm Jesse Barnes wrote: > > > Michel, can you take a look at this? vblank wait is working really > > > well for me with the latest bits, but I found what I think is a page > > > flip related bug on 965. > > No this isn't correct, it also needs to wait for scheduled non-flip > buffer swaps. > > Isn't this just a workaround for the lack of the 2D driver patch and not > necessary with it? I'm not sure what 2D patch you mean (page flipping for 965 I suppose), but clearly there's something wrong here since w/o this patch we end up doing an absolute wait on a 0 vblank count. :) Maybe I need to initialize the vbl_pending/vbl_waited values somewhere else instead? I'll poke around the page flipping & buffer swap code to see if I can make sense of it. Would be nice to have that feature available on 965, and preferably enable it by default if it works. > > > I've been testing with the attached pre- & post-modeset ioctl patch to > > > the 2D driver. > > That one looks good; if anything, maybe slightly too careful about > omitting redundant ioctl calls, but better safe than sorry I guess. :) > > > Oops, I've also been testing with this. > > Yeah, that driver workaround should definitely no longer be necessary. Ok, thanks for checking. Though in looking at the pre- & post-modeset code again, I'm not sure what we have is optimal. The main reason for it is to prevent the user visible vblank count from going backwards, so it seems like we could just add the pre-modeset value to the post-modeset value and be done with it, rather than treating mode set like a wraparound (which will cause our counter to wraparound much more quickly). > > My current thinking is that the only non-racy way to handle the "in > > vblank" case is to only use the frame counter to keep the vblank count > > accurate across interrupt disabled periods. While the interrupt is > > enabled, however, the driver should internally keep a counter going > > based on the vblank interrupt and use that to return a value to the > > core. > > > > If the radeon driver has to do something similar, we could probably > > just update the core drm_handle_vblank() function to use the atomic > > counter instead, which would save a bunch of duplicated driver code... > > Any thoughts there? > > I've been thinking about this as well, should be worth a try. > > Something else I think we need to do before merging vblank-rework into > the kernel is to keep the vblank interrupts enabled and use an atomic > counter until the 2D driver calls the modeset ioctl (or a kernel > modesetting driver does the corresponding thing to indicate it handles > this correctly). Yeah, that would be a good compatibility measure. I'll try adding that when I hack up what's described above. Thanks, Jesse - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://sourceforge.net/services/buy/index.php -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] don't wait on page flips if none are pending
On Fri, 2008-06-20 at 14:27 -0700, Jesse Barnes wrote: > On Friday, June 20, 2008 2:09 pm Jesse Barnes wrote: > > Michel, can you take a look at this? vblank wait is working really well > > for me with the latest bits, but I found what I think is a page flip > > related bug on 965. No this isn't correct, it also needs to wait for scheduled non-flip buffer swaps. Isn't this just a workaround for the lack of the 2D driver patch and not necessary with it? > > I've been testing with the attached pre- & post-modeset ioctl patch to the > > 2D driver. That one looks good; if anything, maybe slightly too careful about omitting redundant ioctl calls, but better safe than sorry I guess. :) > Oops, I've also been testing with this. Yeah, that driver workaround should definitely no longer be necessary. > And looking again at c7ee6cc269c26d8e7ed98a16a272eca63daab201 (the "in > vblank" handling). I think my 965 test machine is fast enough that I > haven't seen much tearing on it yet, but I think for slower machines > we'll definitely need a real fix. > > My current thinking is that the only non-racy way to handle the "in > vblank" case is to only use the frame counter to keep the vblank count > accurate across interrupt disabled periods. While the interrupt is > enabled, however, the driver should internally keep a counter going > based on the vblank interrupt and use that to return a value to the > core. > > If the radeon driver has to do something similar, we could probably > just update the core drm_handle_vblank() function to use the atomic > counter instead, which would save a bunch of duplicated driver code... > Any thoughts there? I've been thinking about this as well, should be worth a try. Something else I think we need to do before merging vblank-rework into the kernel is to keep the vblank interrupts enabled and use an atomic counter until the 2D driver calls the modeset ioctl (or a kernel modesetting driver does the corresponding thing to indicate it handles this correctly). -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://sourceforge.net/services/buy/index.php -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] don't wait on page flips if none are pending
On Friday, June 20, 2008 2:09 pm Jesse Barnes wrote: > Michel, can you take a look at this? vblank wait is working really well > for me with the latest bits, but I found what I think is a page flip > related bug on 965. I've been testing with the attached pre- & > post-modeset ioctl patch to the 2D driver. Changing modes, adding & > removing outputs and moving a window waiting on vblank events all seem > solid & tear free on my 965 (though on 915 we may still have some tearing > due to the way interrupts work on that chip, still have to test & fix if > necessary). > > What do you think? Oops, I've also been testing with this. And looking again at c7ee6cc269c26d8e7ed98a16a272eca63daab201 (the "in vblank" handling). I think my 965 test machine is fast enough that I haven't seen much tearing on it yet, but I think for slower machines we'll definitely need a real fix. My current thinking is that the only non-racy way to handle the "in vblank" case is to only use the frame counter to keep the vblank count accurate across interrupt disabled periods. While the interrupt is enabled, however, the driver should internally keep a counter going based on the vblank interrupt and use that to return a value to the core. If the radeon driver has to do something similar, we could probably just update the core drm_handle_vblank() function to use the atomic counter instead, which would save a bunch of duplicated driver code... Any thoughts there? Thanks, Jesse - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://sourceforge.net/services/buy/index.php -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] don't wait on page flips if none are pending
On Friday, June 20, 2008 2:27 pm Jesse Barnes wrote: > On Friday, June 20, 2008 2:09 pm Jesse Barnes wrote: > > Michel, can you take a look at this? vblank wait is working really well > > for me with the latest bits, but I found what I think is a page flip > > related bug on 965. I've been testing with the attached pre- & > > post-modeset ioctl patch to the 2D driver. Changing modes, adding & > > removing outputs and moving a window waiting on vblank events all seem > > solid & tear free on my 965 (though on 915 we may still have some tearing > > due to the way interrupts work on that chip, still have to test & fix if > > necessary). > > > > What do you think? > > Oops, I've also been testing with this. And looking again at > c7ee6cc269c26d8e7ed98a16a272eca63daab201 (the "in vblank" handling). I > think my 965 test machine is fast enough that I haven't seen much tearing > on it yet, but I think for slower machines we'll definitely need a real > fix. > > My current thinking is that the only non-racy way to handle the "in vblank" > case is to only use the frame counter to keep the vblank count accurate > across interrupt disabled periods. While the interrupt is enabled, > however, the driver should internally keep a counter going based on the > vblank interrupt and use that to return a value to the core. > > If the radeon driver has to do something similar, we could probably just > update the core drm_handle_vblank() function to use the atomic counter > instead, which would save a bunch of duplicated driver code... Any > thoughts there? Oh, and the patch. Jesse diff --git a/shared-core/i915_irq.c b/shared-core/i915_irq.c index 2287cd0..688a58e 100644 --- a/shared-core/i915_irq.c +++ b/shared-core/i915_irq.c @@ -425,13 +425,6 @@ u32 i915_get_vblank_counter(struct drm_device *dev, int plane) if (i915_in_vblank(dev, pipe)) count++; #endif - /* count may be reset by other driver(e.g. 2D driver), - we have no way to know if it is wrapped or resetted - when count is zero. do a rough guess. - */ - if (count == 0 && dev->last_vblank[pipe] < dev->max_vblank_count/2) - dev->last_vblank[pipe] = 0; - return count; } - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://sourceforge.net/services/buy/index.php-- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
