On Tue, Jun 30, 2009 at 2:37 PM, Chris Wilson<ch...@chris-wilson.co.uk> wrote: > I've just got around to playing with the modesetting page-flip ioctl and > found a nasty rendering glitch where the flip occurred before the > rendering was flushed. This appears to be because the finish of the > pending_flip is queued at the same time as the async set_base(). > > Applying the following patch prevents the glitch for me:
Yikes, that's embarassing. I saw the same kind of glitch that when running this with wayland too but never debugged it. Your patch looks good, but I haven't tested it yet. thanks, Kristian > >From aa017e6056cf2faf6be7eeaa71d2fded4a9f6295 Mon Sep 17 00:00:00 2001 > From: Chris Wilson <ch...@chris-wilson.co.uk> > Date: Tue, 30 Jun 2009 18:21:54 +0100 > Subject: [PATCH 1/3] drm: delay unpinning the current fb til after the flip > is complete > > --- > drivers/gpu/drm/drm_crtc.c | 45 +++++++++++++++++++++++++++++++++++-------- > drivers/gpu/drm/drm_irq.c | 7 +++-- > include/drm/drm_crtc.h | 4 ++- > 3 files changed, 43 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 6a5a779..32212e6 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -344,20 +344,30 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb) > EXPORT_SYMBOL(drm_framebuffer_cleanup); > > /** > - * drm_crtc_async_work - do a set_base call from a work queue > + * drm_crtc_async_flip - do a set_base call from a work queue > * @work: work struct > * > * Called when a set_base call is queued by the page flip code. This > * allows the flip ioctl itself to return immediately and allow userspace > * to continue working. > */ > -static void drm_crtc_async_work(struct work_struct *work) > +static void drm_crtc_async_flip(struct work_struct *work) > { > - struct drm_crtc *crtc = container_of(work, struct drm_crtc, > async_work); > + struct drm_crtc *crtc = container_of(work, struct drm_crtc, > async_flip); > struct drm_device *dev = crtc->dev; > + struct drm_pending_flip *pending; > + > + BUG_ON(crtc->pending_flip == NULL); > > mutex_lock(&dev->struct_mutex); > crtc->funcs->set_base(crtc, 0, 0, NULL); > + > + pending = crtc->pending_flip; > + crtc->pending_flip = NULL; > + > + pending->frame = drm_vblank_count(dev, crtc->pipe); > + list_add_tail(&pending->link, &dev->flip_list); > + > mutex_unlock(&dev->struct_mutex); > } > > @@ -384,7 +394,7 @@ void drm_crtc_init(struct drm_device *dev, struct > drm_crtc *crtc, int pipe, > > list_add_tail(&crtc->head, &dev->mode_config.crtc_list); > dev->mode_config.num_crtc++; > - INIT_WORK(&crtc->async_work, drm_crtc_async_work); > + INIT_WORK(&crtc->async_flip, drm_crtc_async_flip); > mutex_unlock(&dev->mode_config.mutex); > } > EXPORT_SYMBOL(drm_crtc_init); > @@ -404,7 +414,7 @@ void drm_crtc_cleanup(struct drm_crtc *crtc) > struct drm_device *dev = crtc->dev; > > mutex_lock(&dev->mode_config.mutex); > - flush_work(&crtc->async_work); > + flush_work(&crtc->async_flip); > > if (crtc->gamma_store) { > kfree(crtc->gamma_store); > @@ -2569,9 +2579,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, > void *data, > goto out_unlock; > } > > - pending->frame = drm_vblank_count(dev, crtc->pipe); > - list_add_tail(&pending->link, &dev->flip_list); > - > /* > * The set_base call will change the domain on the new fb, > * which will force the rendering to finish and block the > @@ -2580,7 +2587,27 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, > void *data, > */ > crtc->fb = obj_to_fb(fb_obj); > > - schedule_work(&crtc->async_work); > + if (crtc->pending_flip != NULL) { > + struct drm_pending_flip *old_flip; > + > + /* We have an outstanding flip request for this crtc/pipe. > + * In order to satisfy the user we can either queue the requests > + * and apply them on sequential vblanks, or we can drop old > + * requests. > + * > + * Here we choose to discard the previous request for > + * simplicity. Note that since we have not yet applied the > + * previous flip, we need to preserve the original (i.e. still > + * current) fb. > + */ > + > + old_flip = crtc->pending_flip; > + pending->old_fb = old_flip->old_fb; > + old_flip->old_fb = NULL; > + drm_finish_pending_flip (dev, old_flip, 0); > + } else > + schedule_work(&crtc->async_flip); > + crtc->pending_flip = pending; > > mutex_unlock(&dev->struct_mutex); > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > index 8b4d0c8..c7a17f6 100644 > --- a/drivers/gpu/drm/drm_irq.c > +++ b/drivers/gpu/drm/drm_irq.c > @@ -72,7 +72,7 @@ int drm_irq_by_busid(struct drm_device *dev, void *data, > return 0; > } > > -#define vblank_after(a,b) ((long)(b) - (long)(a) < 0) > +#define vblank_passed(a,b) ((long)(a - b) > 0) > > void drm_finish_pending_flip(struct drm_device *dev, > struct drm_pending_flip *f, u32 frame) > @@ -87,7 +87,8 @@ void drm_finish_pending_flip(struct drm_device *dev, > list_del_init(&f->link); > list_add_tail(&f->pending_event.link, > &f->pending_event.file_priv->event_list); > - f->old_fb->funcs->unpin(f->old_fb); > + if (f->old_fb) > + f->old_fb->funcs->unpin(f->old_fb); > wake_up_interruptible(&f->pending_event.file_priv->event_wait); > } > > @@ -102,7 +103,7 @@ static void drm_flip_work_func(struct work_struct *work) > > list_for_each_entry_safe(f, t, &dev->flip_list, link) { > frame = drm_vblank_count(dev, f->pipe); > - if (vblank_after(frame, f->frame)) > + if (vblank_passed(frame, f->frame)) > drm_finish_pending_flip(dev, f, frame); > } > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 99fae10..0b5dc47 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -294,6 +294,7 @@ struct drm_property { > struct drm_crtc; > struct drm_connector; > struct drm_encoder; > +struct drm_pending_flip; > > /** > * drm_crtc_funcs - control CRTCs for a given device > @@ -388,7 +389,8 @@ struct drm_crtc { > uint16_t *gamma_store; > > /* Allow async set_pipe_base calls for flipping */ > - struct work_struct async_work; > + struct work_struct async_flip; > + struct drm_pending_flip *pending_flip; > > /* if you are using the helper */ > void *helper_private; > -- > 1.6.3.3 > > > > > > ------------------------------------------------------------------------------ > -- > _______________________________________________ > Dri-devel mailing list > Dri-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/dri-devel > ------------------------------------------------------------------------------ -- _______________________________________________ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel