| Message ID | 562507A3.3080901@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2015-10-19 at 17:09 +0200, Maarten Lankhorst wrote: > Op 19-10-15 om 15:16 schreef Ander Conselvan De Oliveira: > > On Wed, 2015-09-23 at 13:27 +0200, Maarten Lankhorst wrote: > > > Move it from intel_crtc_atomic_commit to prepare_plane_fb. > > > Waiting is done before committing, otherwise it's too late > > > to undo the changes. > > > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_atomic.c | 2 - > > > drivers/gpu/drm/i915/intel_display.c | 107 ++++++++++++++++++++---------- > > > ---- > > > - > > > drivers/gpu/drm/i915/intel_drv.h | 2 - > > > 3 files changed, 62 insertions(+), 49 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c > > > b/drivers/gpu/drm/i915/intel_atomic.c > > > index f1975f267710..25a891aa3824 100644 > > > --- a/drivers/gpu/drm/i915/intel_atomic.c > > > +++ b/drivers/gpu/drm/i915/intel_atomic.c > > > @@ -205,8 +205,6 @@ int intel_atomic_setup_scalers(struct drm_device *dev, > > > * but since this plane is unchanged just > > > do > > > the > > > * minimum required validation. > > > */ > > > - if (plane->type == > > > DRM_PLANE_TYPE_PRIMARY) > > > - intel_crtc->atomic.wait_for_flips > > > = > > > true; > > > crtc_state->base.planes_changed = true; > > > } > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index 25e1eac260fd..cd651ff6c15b 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -3221,32 +3221,6 @@ void intel_finish_reset(struct drm_device *dev) > > > drm_modeset_unlock_all(dev); > > > } > > > > > > -static void > > > -intel_finish_fb(struct drm_framebuffer *old_fb) > > > -{ > > > - struct drm_i915_gem_object *obj = intel_fb_obj(old_fb); > > > - struct drm_i915_private *dev_priv = to_i915(obj->base.dev); > > > - bool was_interruptible = dev_priv->mm.interruptible; > > > - int ret; > > > - > > > - /* 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 > > > - * framebuffer. Note that we rely on userspace rendering > > > - * into the buffer attached to the pipe they are waiting > > > - * on. If not, userspace generates a GPU hang with IPEHR > > > - * point to the MI_WAIT_FOR_EVENT. > > > - * > > > - * This should only fail upon a hung GPU, in which case we > > > - * can safely continue. > > > - */ > > > - dev_priv->mm.interruptible = false; > > > - ret = i915_gem_object_wait_rendering(obj, true); > > > - dev_priv->mm.interruptible = was_interruptible; > > > - > > > - WARN_ON(ret); > > > -} > > > - > > > static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc) > > > { > > > struct drm_device *dev = crtc->dev; > > > @@ -3867,15 +3841,23 @@ static void page_flip_completed(struct intel_crtc > > > *intel_crtc) > > > work->pending_flip_obj); > > > } > > > > > > -void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc) > > > +static int 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; > > > + long ret; > > > > > > WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue)); > > > - if (WARN_ON(wait_event_timeout(dev_priv->pending_flip_queue, > > > - > > > !intel_crtc_has_pending_flip(crtc), > > > - 60*HZ) == 0)) { > > > + > > > + ret = wait_event_interruptible_timeout( > > > + dev_priv->pending_flip_queue, > > > + !intel_crtc_has_pending_flip(crtc > > > ), > > > + 60*HZ); > > > + > > > + if (ret < 0) > > > + return ret; > > > + > > > + if (ret == 0) { > > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > > > > spin_lock_irq(&dev->event_lock); > > > @@ -3886,11 +3868,7 @@ void intel_crtc_wait_for_pending_flips(struct > > > drm_crtc > > > *crtc) > > > spin_unlock_irq(&dev->event_lock); > > > } > > > > > > - if (crtc->primary->fb) { > > > - mutex_lock(&dev->struct_mutex); > > > - intel_finish_fb(crtc->primary->fb); > > > - mutex_unlock(&dev->struct_mutex); > > > - } > > There is another caller of intel_crtc_wait_for_pending_flips() besides the > > one > > touched in this patch: intel_crtc_disable_noatomic(). In your previous > > series > > you dropped that call based on the fact that there shouldn't be any pending > > flips at that point, but that patch has been dropped. > > > > Wouldn't it be better to add a WARN_ON as Chris suggested then instead of > > keeping the wait for flips but without the work around? > > > Apply with --scissors > > ---8<------ > intel_crtc_disable_noatomic is called from hw readout during init, resume and > possibly reset. > During init it's too early to have a page flip queued, before suspending all > page flips > should be finished and during hw reset all page flips should be removed. > > It's a bug when there are pending flips here, complain with WARN_ON instead of > handling it. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > -- > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index f59029c3e577..95c59b5202c5 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6225,7 +6225,8 @@ static void intel_crtc_disable_noatomic(struct drm_crtc > *crtc) > return; > > if (to_intel_plane_state(crtc->primary->state)->visible) { > - intel_crtc_wait_for_pending_flips(crtc); > + WARN_ON(intel_crtc->unpin_work); > + > intel_pre_disable_primary(crtc); > } > > Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
On Mon, Oct 19, 2015 at 05:09:23PM +0200, Maarten Lankhorst wrote: > Op 19-10-15 om 15:16 schreef Ander Conselvan De Oliveira: > > On Wed, 2015-09-23 at 13:27 +0200, Maarten Lankhorst wrote: > >> Move it from intel_crtc_atomic_commit to prepare_plane_fb. > >> Waiting is done before committing, otherwise it's too late > >> to undo the changes. > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_atomic.c | 2 - > >> drivers/gpu/drm/i915/intel_display.c | 107 ++++++++++++++++++++-------------- > >> - > >> drivers/gpu/drm/i915/intel_drv.h | 2 - > >> 3 files changed, 62 insertions(+), 49 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c > >> b/drivers/gpu/drm/i915/intel_atomic.c > >> index f1975f267710..25a891aa3824 100644 > >> --- a/drivers/gpu/drm/i915/intel_atomic.c > >> +++ b/drivers/gpu/drm/i915/intel_atomic.c > >> @@ -205,8 +205,6 @@ int intel_atomic_setup_scalers(struct drm_device *dev, > >> * but since this plane is unchanged just do > >> the > >> * minimum required validation. > >> */ > >> - if (plane->type == DRM_PLANE_TYPE_PRIMARY) > >> - intel_crtc->atomic.wait_for_flips = > >> true; > >> crtc_state->base.planes_changed = true; > >> } > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c > >> b/drivers/gpu/drm/i915/intel_display.c > >> index 25e1eac260fd..cd651ff6c15b 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -3221,32 +3221,6 @@ void intel_finish_reset(struct drm_device *dev) > >> drm_modeset_unlock_all(dev); > >> } > >> > >> -static void > >> -intel_finish_fb(struct drm_framebuffer *old_fb) > >> -{ > >> - struct drm_i915_gem_object *obj = intel_fb_obj(old_fb); > >> - struct drm_i915_private *dev_priv = to_i915(obj->base.dev); > >> - bool was_interruptible = dev_priv->mm.interruptible; > >> - int ret; > >> - > >> - /* 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 > >> - * framebuffer. Note that we rely on userspace rendering > >> - * into the buffer attached to the pipe they are waiting > >> - * on. If not, userspace generates a GPU hang with IPEHR > >> - * point to the MI_WAIT_FOR_EVENT. > >> - * > >> - * This should only fail upon a hung GPU, in which case we > >> - * can safely continue. > >> - */ > >> - dev_priv->mm.interruptible = false; > >> - ret = i915_gem_object_wait_rendering(obj, true); > >> - dev_priv->mm.interruptible = was_interruptible; > >> - > >> - WARN_ON(ret); > >> -} > >> - > >> static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc) > >> { > >> struct drm_device *dev = crtc->dev; > >> @@ -3867,15 +3841,23 @@ static void page_flip_completed(struct intel_crtc > >> *intel_crtc) > >> work->pending_flip_obj); > >> } > >> > >> -void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc) > >> +static int 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; > >> + long ret; > >> > >> WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue)); > >> - if (WARN_ON(wait_event_timeout(dev_priv->pending_flip_queue, > >> - !intel_crtc_has_pending_flip(crtc), > >> - 60*HZ) == 0)) { > >> + > >> + ret = wait_event_interruptible_timeout( > >> + dev_priv->pending_flip_queue, > >> + !intel_crtc_has_pending_flip(crtc), > >> + 60*HZ); > >> + > >> + if (ret < 0) > >> + return ret; > >> + > >> + if (ret == 0) { > >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > >> > >> spin_lock_irq(&dev->event_lock); > >> @@ -3886,11 +3868,7 @@ void intel_crtc_wait_for_pending_flips(struct drm_crtc > >> *crtc) > >> spin_unlock_irq(&dev->event_lock); > >> } > >> > >> - if (crtc->primary->fb) { > >> - mutex_lock(&dev->struct_mutex); > >> - intel_finish_fb(crtc->primary->fb); > >> - mutex_unlock(&dev->struct_mutex); > >> - } > > There is another caller of intel_crtc_wait_for_pending_flips() besides the one > > touched in this patch: intel_crtc_disable_noatomic(). In your previous series > > you dropped that call based on the fact that there shouldn't be any pending > > flips at that point, but that patch has been dropped. > > > > Wouldn't it be better to add a WARN_ON as Chris suggested then instead of > > keeping the wait for flips but without the work around? > > > Apply with --scissors > > ---8<------ > intel_crtc_disable_noatomic is called from hw readout during init, resume and possibly reset. > During init it's too early to have a page flip queued, before suspending all page flips > should be finished and during hw reset all page flips should be removed. > > It's a bug when there are pending flips here, complain with WARN_ON instead of handling it. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > -- > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index f59029c3e577..95c59b5202c5 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6225,7 +6225,8 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) > return; > > if (to_intel_plane_state(crtc->primary->state)->visible) { > - intel_crtc_wait_for_pending_flips(crtc); > + WARN_ON(intel_crtc->unpin_work); > + > intel_pre_disable_primary(crtc); > } > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f59029c3e577..95c59b5202c5 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6225,7 +6225,8 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) return; if (to_intel_plane_state(crtc->primary->state)->visible) { - intel_crtc_wait_for_pending_flips(crtc); + WARN_ON(intel_crtc->unpin_work); + intel_pre_disable_primary(crtc); }