On Mon, Nov 26, 2012 at 10:21:28PM +0100, Daniel Vetter wrote:
> On Mon, Nov 05, 2012 at 03:20:53PM +0200, ville.syrjala at linux.intel.com wrote:
> > From: Ville Syrj?l? <ville.syrjala at 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.
> >
> > Signed-off-by: Ville Syrj?l? <ville.syrjala at linux.intel.com>
> > ---
> > v2: Shuffle the code around so that intel_crtc_wait_for_pending_flips()
> > just wraps intel_crtc_wait_for_pending_flips_locked().
>
> Sorry for the long delay in looking at this. One bikeshed here: I prefer
> the patch changelog before the sob lines so that it gets included in the
> commit message - most often it's rather interesting read, especially for
> patches that take a few revisions to get right. More substantial comment
> below.
>
> > drivers/gpu/drm/i915/intel_display.c | 76 ++++++++++++++++++---------------
> > 1 files changed, 41 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 1a38267..a18e6e6 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2228,6 +2228,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 (atomic_read(&dev_priv->mm.wedged))
> > + 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));
>
> I think we also need to add a dev_priv->mm.wedged check here, since the
> gpu might die and never execute the pageflip. Otoh we don't complete any
> pageflips that never executed due to a gpu hang, so maybe also add a big
> FIXME. But with the wedged check we should at least not hang in an
> non-interruptible wait.
>
> The other thing is that the wait_even in finish_fb is not superflous,
> since we should never see a framebuffer with pending flips for _this_ crtc
> (it could have a pending flip on another crtc). So I think that code in
> finish_fb should die, leaving just the comment and the finish_gpu call.
>
> Cheers, Daniel
>
> PS: Testcase would be awesome, but I have no ideas beyond what we already
> have in flip_test unfortunately ...
Ok, the patch is actually correct, I just couldn't read C code for a
moment. Still I think a v3 with the superflous wait_event in finish_fb
ditched would look nice.
Now Chris reminded me that you have a few other patches around this in the
"drm/i915: i915_gem_execbuffer_wait_for_flips and other flip stuff"
thread. And I have a few reset state transition improvements in the
pipeline for 3.9 at
http://cgit.freedesktop.org/~danvet/drm/log/?h=robustify-reset-transitions
Can you please rebase that entire patch series on top of those patches?
For patch 3 I think we should go with option b) and outright kill the
wait-for-flips in the execbuf ioctl. Maybe harrass Jesse and Kristian for
a formal ack on that one.
Also, if you can throw an additional patch on top to properly clear any
outstanding flips when the gpu hangs, that would be awesome - since
currently pageflips will get busted if the gpu hangs. And if you could
throw a testcase in for fun ...
Cheers, Daniel
PS: The testcase should be easy to pull of by adapting ZZ_hangman with a
different workload. Like flip_test ...
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
