| Message ID | 1354041298-18898-3-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 27, 2012 at 08:34:56PM +0200, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > intel_pipe_set_base() never actually waited for any pending page flips > on the CRTC. It looks like it tried to, by calling intel_finish_fb() on > the current front buffer. But the pending flips were actually tracked > in the BO of the previous front buffer, so the call to intel_finish_fb() > never did anything useful. > > intel_crtc_wait_for_pending_flips() is the current _working_ way to wait > for pending page flips. So use it in intel_pipe_set_base() too. Some > refactoring was necessary to avoid locking struct_mutex twice. > > v2: Shuffle the code around so that intel_crtc_wait_for_pending_flips() > just wraps intel_crtc_wait_for_pending_flips_locked(). > > v3: Kill leftover wait_event() in intel_finish_fb() > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/intel_display.c | 80 +++++++++++++++++---------------- > 1 files changed, 41 insertions(+), 39 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 8c2d810..ea710af 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2238,10 +2238,6 @@ intel_finish_fb(struct drm_framebuffer *old_fb) > bool was_interruptible = dev_priv->mm.interruptible; > int ret; > > - wait_event(dev_priv->pending_flip_queue, > - i915_reset_in_progress(&dev_priv->gpu_error) || > - atomic_read(&obj->pending_flip) == 0); > - > /* Big Hammer, we also need to ensure that any pending > * MI_WAIT_FOR_EVENT inside a user batch buffer on the > * current scanout is retired before unpinning the old > @@ -2284,6 +2280,46 @@ static void intel_crtc_update_sarea_pos(struct drm_crtc *crtc, int x, int y) > } > } > > +static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc) > +{ > + struct drm_device *dev = crtc->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + unsigned long flags; > + bool pending; > + > + if (i915_reset_in_progress(&dev_priv->gpu_error)) > + return false; This check is not enough, since a full gpu reset might have happened while we didn't look. Which means that we'll still be waiting forever. To really close all gaps and races I think we need to full multi-state transistions like it's already implemented in __wait_seqno (minus the first kick, since no one is holding the dev->struct_mutex while waiting for a pageflip to complete). So - we need a reset_counter here like in wait_seqno - need to reset the unpin_work state (and fire off any pending drm events while at it) to unblock kernel waiters and userspace Also: igt test highly preferred ;-) Best would be to convert the hangman test over to the subtest infrastructure (maybe easier when first ported python and using argpars, but I don't mind bash's getopt support) and then adding a new subtestcase which exercises flips vs. hangs. Yours, Daniel > + > + spin_lock_irqsave(&dev->event_lock, flags); > + pending = to_intel_crtc(crtc)->unpin_work != NULL; > + spin_unlock_irqrestore(&dev->event_lock, flags); > + > + return pending; > +} > + > +static void intel_crtc_wait_for_pending_flips_locked(struct drm_crtc *crtc) > +{ > + struct drm_device *dev = crtc->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + if (crtc->fb == NULL) > + return; > + > + wait_event(dev_priv->pending_flip_queue, > + !intel_crtc_has_pending_flip(crtc)); > + > + intel_finish_fb(crtc->fb); > +} > + > +static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc) > +{ > + struct drm_device *dev = crtc->dev; > + > + mutex_lock(&dev->struct_mutex); > + intel_crtc_wait_for_pending_flips_locked(crtc); > + mutex_unlock(&dev->struct_mutex); > +} > + > static int > intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, > struct drm_framebuffer *fb) > @@ -2317,8 +2353,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, > return ret; > } > > - if (crtc->fb) > - intel_finish_fb(crtc->fb); > + intel_crtc_wait_for_pending_flips_locked(crtc); > > ret = dev_priv->display.update_plane(crtc, fb, x, y); > if (ret) { > @@ -2950,39 +2985,6 @@ static void ironlake_fdi_disable(struct drm_crtc *crtc) > udelay(100); > } > > -static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc) > -{ > - struct drm_device *dev = crtc->dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > - unsigned long flags; > - bool pending; > - > - if (i915_reset_in_progress(&dev_priv->gpu_error)) > - return false; > - > - spin_lock_irqsave(&dev->event_lock, flags); > - pending = to_intel_crtc(crtc)->unpin_work != NULL; > - spin_unlock_irqrestore(&dev->event_lock, flags); > - > - return pending; > -} > - > -static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc) > -{ > - struct drm_device *dev = crtc->dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > - > - if (crtc->fb == NULL) > - return; > - > - wait_event(dev_priv->pending_flip_queue, > - !intel_crtc_has_pending_flip(crtc)); > - > - mutex_lock(&dev->struct_mutex); > - intel_finish_fb(crtc->fb); > - mutex_unlock(&dev->struct_mutex); > -} > - > static bool ironlake_crtc_driving_pch(struct drm_crtc *crtc) > { > struct drm_device *dev = crtc->dev; > -- > 1.7.8.6 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Nov 28, 2012 at 09:51:18PM +0100, Daniel Vetter wrote: > On Tue, Nov 27, 2012 at 08:34:56PM +0200, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > intel_pipe_set_base() never actually waited for any pending page flips > > on the CRTC. It looks like it tried to, by calling intel_finish_fb() on > > the current front buffer. But the pending flips were actually tracked > > in the BO of the previous front buffer, so the call to intel_finish_fb() > > never did anything useful. > > > > intel_crtc_wait_for_pending_flips() is the current _working_ way to wait > > for pending page flips. So use it in intel_pipe_set_base() too. Some > > refactoring was necessary to avoid locking struct_mutex twice. > > > > v2: Shuffle the code around so that intel_crtc_wait_for_pending_flips() > > just wraps intel_crtc_wait_for_pending_flips_locked(). > > > > v3: Kill leftover wait_event() in intel_finish_fb() > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/intel_display.c | 80 +++++++++++++++++---------------- > > 1 files changed, 41 insertions(+), 39 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 8c2d810..ea710af 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -2238,10 +2238,6 @@ intel_finish_fb(struct drm_framebuffer *old_fb) > > bool was_interruptible = dev_priv->mm.interruptible; > > int ret; > > > > - wait_event(dev_priv->pending_flip_queue, > > - i915_reset_in_progress(&dev_priv->gpu_error) || > > - atomic_read(&obj->pending_flip) == 0); > > - > > /* Big Hammer, we also need to ensure that any pending > > * MI_WAIT_FOR_EVENT inside a user batch buffer on the > > * current scanout is retired before unpinning the old > > @@ -2284,6 +2280,46 @@ static void intel_crtc_update_sarea_pos(struct drm_crtc *crtc, int x, int y) > > } > > } > > > > +static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc) > > +{ > > + struct drm_device *dev = crtc->dev; > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + unsigned long flags; > > + bool pending; > > + > > + if (i915_reset_in_progress(&dev_priv->gpu_error)) > > + return false; > > This check is not enough, since a full gpu reset might have happened while > we didn't look. Which means that we'll still be waiting forever. To really > close all gaps and races I think we need to full multi-state transistions > like it's already implemented in __wait_seqno (minus the first kick, since > no one is holding the dev->struct_mutex while waiting for a pageflip to > complete). So > - we need a reset_counter here like in wait_seqno > - need to reset the unpin_work state (and fire off any pending drm events > while at it) to unblock kernel waiters and userspace > > Also: igt test highly preferred ;-) Best would be to convert the hangman > test over to the subtest infrastructure (maybe easier when first ported > python and using argpars, but I don't mind bash's getopt support) and then > adding a new subtestcase which exercises flips vs. hangs. I'm going to plead innocent on this one. I just copied code around, but you wrote it ;)
On Wed, Nov 28, 2012 at 09:51:18PM +0100, Daniel Vetter wrote: > On Tue, Nov 27, 2012 at 08:34:56PM +0200, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > intel_pipe_set_base() never actually waited for any pending page flips > > on the CRTC. It looks like it tried to, by calling intel_finish_fb() on > > the current front buffer. But the pending flips were actually tracked > > in the BO of the previous front buffer, so the call to intel_finish_fb() > > never did anything useful. > > > > intel_crtc_wait_for_pending_flips() is the current _working_ way to wait > > for pending page flips. So use it in intel_pipe_set_base() too. Some > > refactoring was necessary to avoid locking struct_mutex twice. > > > > v2: Shuffle the code around so that intel_crtc_wait_for_pending_flips() > > just wraps intel_crtc_wait_for_pending_flips_locked(). > > > > v3: Kill leftover wait_event() in intel_finish_fb() > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/intel_display.c | 80 +++++++++++++++++---------------- > > 1 files changed, 41 insertions(+), 39 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 8c2d810..ea710af 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -2238,10 +2238,6 @@ intel_finish_fb(struct drm_framebuffer *old_fb) > > bool was_interruptible = dev_priv->mm.interruptible; > > int ret; > > > > - wait_event(dev_priv->pending_flip_queue, > > - i915_reset_in_progress(&dev_priv->gpu_error) || > > - atomic_read(&obj->pending_flip) == 0); > > - > > /* Big Hammer, we also need to ensure that any pending > > * MI_WAIT_FOR_EVENT inside a user batch buffer on the > > * current scanout is retired before unpinning the old > > @@ -2284,6 +2280,46 @@ static void intel_crtc_update_sarea_pos(struct drm_crtc *crtc, int x, int y) > > } > > } > > > > +static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc) > > +{ > > + struct drm_device *dev = crtc->dev; > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + unsigned long flags; > > + bool pending; > > + > > + if (i915_reset_in_progress(&dev_priv->gpu_error)) > > + return false; > > This check is not enough, since a full gpu reset might have happened while > we didn't look. Which means that we'll still be waiting forever. To really > close all gaps and races I think we need to full multi-state transistions > like it's already implemented in __wait_seqno (minus the first kick, since > no one is holding the dev->struct_mutex while waiting for a pageflip to > complete). So > - we need a reset_counter here like in wait_seqno > - need to reset the unpin_work state (and fire off any pending drm events > while at it) to unblock kernel waiters and userspace I had another look at this, and I think you're aiming for something other than what this patch is trying to do. The thing is that we are holding struct_mutex while waiting for pending flips here, so we just need to get out asap to allow the reset work do its thing. Completing pending page flips when a reset occurs is separate issue. This code never did any of that, and I don't see why it should. We would need to complete them anyway regardless of whether anyone is currently waiting for them. Perhaps we can just call intel_finish_page_flip() from the reset code and call it a day. I suppose doing that could end up unpinning the current scanout buffer in case the display registers were never written with the new values. One solution for that would be to always do a set_base() after a reset. That would make sure we end up showing the latest buffer at least, although the contents could be total garbage.
On Fri, Nov 30, 2012 at 3:01 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > I had another look at this, and I think you're aiming for something other > than what this patch is trying to do. Oh, I know that I'm trying to volunteer you to do a bit more ... it's why I'm the bastard maintainer ;-) > The thing is that we are holding struct_mutex while waiting for pending > flips here, so we just need to get out asap to allow the reset work do > its thing. Hm, I didn't notice that we're holding the struct_mutex there, so I guess we need to indeed bail out. Or rework the finsh_fb vs. gpu hang logic to not require the struct_mutex in finish_fb ... > Completing pending page flips when a reset occurs is separate issue. > This code never did any of that, and I don't see why it should. We > would need to complete them anyway regardless of whether anyone is > currently waiting for them. Yeah, that's my idea, but I haven't though through the details (see my ignorance with locking details above ...). > Perhaps we can just call intel_finish_page_flip() from the reset > code and call it a day. I suppose doing that could end up unpinning > the current scanout buffer in case the display registers were never > written with the new values. One solution for that would be to always > do a set_base() after a reset. That would make sure we end up showing > the latest buffer at least, although the contents could be total > garbage. Hm, I think first we should aim to no longer hang either the kernel or leave stuck userspace (since the drm_event never shows up) behind. Atm a gpu hang is allowed to corrupt pretty much everything. Later on, when we successfully avoid the hung kernel/userspace problems we can worry about making it look decent. Judging by how long it took to get basic reset working somewhat okish, it'll take a while. And we need some form of testcase to exercise the code. Cheers, Daniel
On Fri, Nov 30, 2012 at 04:44:04PM +0100, Daniel Vetter wrote: > On Fri, Nov 30, 2012 at 3:01 PM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > I had another look at this, and I think you're aiming for something other > > than what this patch is trying to do. > > Oh, I know that I'm trying to volunteer you to do a bit more ... it's > why I'm the bastard maintainer ;-) > > > The thing is that we are holding struct_mutex while waiting for pending > > flips here, so we just need to get out asap to allow the reset work do > > its thing. > > Hm, I didn't notice that we're holding the struct_mutex there, so I > guess we need to indeed bail out. Or rework the finsh_fb vs. gpu hang > logic to not require the struct_mutex in finish_fb ... > > > Completing pending page flips when a reset occurs is separate issue. > > This code never did any of that, and I don't see why it should. We > > would need to complete them anyway regardless of whether anyone is > > currently waiting for them. > > Yeah, that's my idea, but I haven't though through the details (see my > ignorance with locking details above ...). > > > Perhaps we can just call intel_finish_page_flip() from the reset > > code and call it a day. I suppose doing that could end up unpinning > > the current scanout buffer in case the display registers were never > > written with the new values. One solution for that would be to always > > do a set_base() after a reset. That would make sure we end up showing > > the latest buffer at least, although the contents could be total > > garbage. > > Hm, I think first we should aim to no longer hang either the kernel or > leave stuck userspace (since the drm_event never shows up) behind. Atm > a gpu hang is allowed to corrupt pretty much everything. Later on, > when we successfully avoid the hung kernel/userspace problems we can > worry about making it look decent. Judging by how long it took to get > basic reset working somewhat okish, it'll take a while. And we need > some form of testcase to exercise the code. At least the current patch would avoid having the process stuck in D state. So I think it's better than nothing. I do have other things on my plate, so I don't want to spend any more time on this currently.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 8c2d810..ea710af 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2238,10 +2238,6 @@ intel_finish_fb(struct drm_framebuffer *old_fb) bool was_interruptible = dev_priv->mm.interruptible; int ret; - wait_event(dev_priv->pending_flip_queue, - i915_reset_in_progress(&dev_priv->gpu_error) || - atomic_read(&obj->pending_flip) == 0); - /* Big Hammer, we also need to ensure that any pending * MI_WAIT_FOR_EVENT inside a user batch buffer on the * current scanout is retired before unpinning the old @@ -2284,6 +2280,46 @@ static void intel_crtc_update_sarea_pos(struct drm_crtc *crtc, int x, int y) } } +static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc) +{ + struct drm_device *dev = crtc->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + unsigned long flags; + bool pending; + + if (i915_reset_in_progress(&dev_priv->gpu_error)) + return false; + + spin_lock_irqsave(&dev->event_lock, flags); + pending = to_intel_crtc(crtc)->unpin_work != NULL; + spin_unlock_irqrestore(&dev->event_lock, flags); + + return pending; +} + +static void intel_crtc_wait_for_pending_flips_locked(struct drm_crtc *crtc) +{ + struct drm_device *dev = crtc->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + + if (crtc->fb == NULL) + return; + + wait_event(dev_priv->pending_flip_queue, + !intel_crtc_has_pending_flip(crtc)); + + intel_finish_fb(crtc->fb); +} + +static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc) +{ + struct drm_device *dev = crtc->dev; + + mutex_lock(&dev->struct_mutex); + intel_crtc_wait_for_pending_flips_locked(crtc); + mutex_unlock(&dev->struct_mutex); +} + static int intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, struct drm_framebuffer *fb) @@ -2317,8 +2353,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, return ret; } - if (crtc->fb) - intel_finish_fb(crtc->fb); + intel_crtc_wait_for_pending_flips_locked(crtc); ret = dev_priv->display.update_plane(crtc, fb, x, y); if (ret) { @@ -2950,39 +2985,6 @@ static void ironlake_fdi_disable(struct drm_crtc *crtc) udelay(100); } -static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc) -{ - struct drm_device *dev = crtc->dev; - struct drm_i915_private *dev_priv = dev->dev_private; - unsigned long flags; - bool pending; - - if (i915_reset_in_progress(&dev_priv->gpu_error)) - return false; - - spin_lock_irqsave(&dev->event_lock, flags); - pending = to_intel_crtc(crtc)->unpin_work != NULL; - spin_unlock_irqrestore(&dev->event_lock, flags); - - return pending; -} - -static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc) -{ - struct drm_device *dev = crtc->dev; - struct drm_i915_private *dev_priv = dev->dev_private; - - if (crtc->fb == NULL) - return; - - wait_event(dev_priv->pending_flip_queue, - !intel_crtc_has_pending_flip(crtc)); - - mutex_lock(&dev->struct_mutex); - intel_finish_fb(crtc->fb); - mutex_unlock(&dev->struct_mutex); -} - static bool ironlake_crtc_driving_pch(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev;