| Message ID | 1351793163-8542-2-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 1 Nov 2012 20:06:00 +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. > > Now even the pending_flip counter is gone, so we should just > use intel_crtc_wait_for_pending_flips(), but since we're already holding > struct_mutex when we would call that function, we need another version > of it, that itself doesn't lock struct_mutex. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Your earlier point was that intel_finish_fb() is being called in the wrong place, if you fix that first you should not need the major surgery. -Chris
On Fri, Nov 02, 2012 at 01:26:56PM +0000, Chris Wilson wrote: > On Thu, 1 Nov 2012 20:06:00 +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. > > > > Now even the pending_flip counter is gone, so we should just > > use intel_crtc_wait_for_pending_flips(), but since we're already holding > > struct_mutex when we would call that function, we need another version > > of it, that itself doesn't lock struct_mutex. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Your earlier point was that intel_finish_fb() is being called in the wrong > place, if you fix that first you should not need the major surgery. I don't think it's the wrong place as such. We do need it for the panning case. The only issue with the current place is that we end up calling it twice in the full modeset path; once in crtc_disable(), and then later in intel_pipe_set_base(). I could move the call up from intel_pipe_set_base() to intel_crtc_set_config() so that it only gets called for panning. This would also solve the locking issue, but it doesn't seem as efficient as the current sequence, because we'd end up pinning the new buffer after waiting for page flips. With the current sequence the flip can complete in parallel while we're doing the pin operation. Another alternative would be to leave the call where it is in intel_pipe_set_base(), but simply drop and reacquire struct_mutex around the call. That would avoid the need for the _locked() variant. That would still leave us with the double call for full modeset, but I'm not sure that part is worth fixing. If we really want to fix it, then we could add a paramter to intel_pipe_set_base(), or maybe look at some other bit of state to detect the full modeset case, and skip the call.
On Thu, 1 Nov 2012 20:06:00 +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. > > Now even the pending_flip counter is gone, so we should just > use intel_crtc_wait_for_pending_flips(), but since we're already holding > struct_mutex when we would call that function, we need another version > of it, that itself doesn't lock struct_mutex. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 51 +++++++++++++++++++++------------ > 1 files changed, 32 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 1a38267..7bf4749 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2228,6 +2228,37 @@ 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) > +{ Can we rearrange this such that the waiting logic is inside _locked() and then intel_crtc_wiat_for_pending_flips() becomes a wrapper that acquires the struct_mutex and then calls _locked()? Just to keep the code simpler at the expense of the pathological case. -Chris
On Fri, Nov 02, 2012 at 03:25:59PM +0000, Chris Wilson wrote: > On Thu, 1 Nov 2012 20:06:00 +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. > > > > Now even the pending_flip counter is gone, so we should just > > use intel_crtc_wait_for_pending_flips(), but since we're already holding > > struct_mutex when we would call that function, we need another version > > of it, that itself doesn't lock struct_mutex. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 51 +++++++++++++++++++++------------ > > 1 files changed, 32 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 1a38267..7bf4749 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -2228,6 +2228,37 @@ 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) > > +{ > > Can we rearrange this such that the waiting logic is inside _locked() > and then intel_crtc_wiat_for_pending_flips() becomes a wrapper that > acquires the struct_mutex and then calls _locked()? Just to keep the > code simpler at the expense of the pathological case. Yeah that looks doable. It does mean we'll be holding struct_mutex around the wait_event() always. As I was already doing that for the panning case, doing the same in the crtc_disable() case shouldn't be any worse. But now I started to wonder a bit about the performance implications of keeping struct_mutex locked for that long...
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1a38267..7bf4749 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2228,6 +2228,37 @@ 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)); + + intel_finish_fb(crtc->fb); +} + static int intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, struct drm_framebuffer *fb) @@ -2261,8 +2292,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) { @@ -2901,23 +2931,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 (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(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev;