| Message ID | 1437051471-15397-6-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 16-07-15 om 14:57 schreef Maarten Lankhorst: > 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 | 72 +++++++++++++++++------------------- > drivers/gpu/drm/i915/intel_drv.h | 2 - > 3 files changed, 33 insertions(+), 43 deletions(-) > This breaks DPMS off, because it might wait for flips before continuing. It looks like all drivers use drmModeConnectorSetProperty correctly, so it's safe to fix dpms to return an error. Can I get the changes to dpms handling in intel applied first, and the first 4 patches from this series? ~Maarten
On Mon, Jul 20, 2015 at 04:52:11PM +0200, Maarten Lankhorst wrote: > Op 16-07-15 om 14:57 schreef Maarten Lankhorst: > > 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 | 72 +++++++++++++++++------------------- > > drivers/gpu/drm/i915/intel_drv.h | 2 - > > 3 files changed, 33 insertions(+), 43 deletions(-) > > > This breaks DPMS off, because it might wait for flips before continuing. > It looks like all drivers use drmModeConnectorSetProperty correctly, > so it's safe to fix dpms to return an error. > > Can I get the changes to dpms handling in intel applied first, and the first 4 patches from this series? Yeah makes sense. -Daniel
Op 21-07-15 om 08:51 schreef Daniel Vetter: > On Mon, Jul 20, 2015 at 04:52:11PM +0200, Maarten Lankhorst wrote: >> Op 16-07-15 om 14:57 schreef Maarten Lankhorst: >>> 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 | 72 +++++++++++++++++------------------- >>> drivers/gpu/drm/i915/intel_drv.h | 2 - >>> 3 files changed, 33 insertions(+), 43 deletions(-) >>> >> This breaks DPMS off, because it might wait for flips before continuing. >> It looks like all drivers use drmModeConnectorSetProperty correctly, >> so it's safe to fix dpms to return an error. >> >> Can I get the changes to dpms handling in intel applied first, and the first 4 patches from this series? > Ok patch sent to dri-devel, after dpms returns a value this patch should be safe to apply. ~Maarten
On Thu, Jul 16, 2015 at 02:57:51PM +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 | 72 +++++++++++++++++------------------- > drivers/gpu/drm/i915/intel_drv.h | 2 - > 3 files changed, 33 insertions(+), 43 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > index e2531cf59266..09a0ad611002 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -214,8 +214,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 5a462e9a4d14..8ef0dbb33afd 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3250,32 +3250,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; > @@ -3890,15 +3864,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); > @@ -3909,11 +3891,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); > - } > + return 0; > } > > /* Program iCLKIP clock to the desired frequency */ > @@ -4773,9 +4751,6 @@ static void intel_pre_plane_update(struct intel_crtc *crtc) > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc_atomic_commit *atomic = &crtc->atomic; > > - if (atomic->wait_for_flips) > - intel_crtc_wait_for_pending_flips(&crtc->base); > - > if (atomic->disable_fbc) > intel_fbc_disable_crtc(crtc); > > @@ -11701,7 +11676,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, > > switch (plane->type) { > case DRM_PLANE_TYPE_PRIMARY: > - intel_crtc->atomic.wait_for_flips = true; > intel_crtc->atomic.pre_disable_primary = turn_off; > intel_crtc->atomic.post_enable_primary = turn_on; > > @@ -13473,6 +13447,12 @@ intel_prepare_plane_fb(struct drm_plane *plane, > struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb); > int ret = 0; > > + if (plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj) { Would feel safer is we just asked if the CRTC had pending flips irrespective of old_obj. Do you plan on moving the pending flips from CRTC to plane? That would seem to be implied here. > + ret = intel_crtc_wait_for_pending_flips(plane->state->crtc); > + if (ret) > + return ret; > + } > + > if (obj == old_obj) > return 0; > > @@ -13492,6 +13472,20 @@ intel_prepare_plane_fb(struct drm_plane *plane, > ret = intel_pin_and_fence_fb_obj(plane, fb, new_state, NULL, NULL); > } > > + /* 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. > + */ > + if (!ret && plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj) > + ret = i915_gem_object_wait_rendering(old_obj, true); Technically I can create a batch with a WAIT_ON_EVENT for a secondary plane as well. This path need only be done in a modeset, not for a simple plane flip. (But we actually need to serialise with rendering to old_obj before regarding the plane flip as complete.) Is there a plane-prepare-modeset hook? -Chris
Op 21-07-15 om 13:26 schreef Chris Wilson: > On Thu, Jul 16, 2015 at 02:57:51PM +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 | 72 +++++++++++++++++------------------- >> drivers/gpu/drm/i915/intel_drv.h | 2 - >> 3 files changed, 33 insertions(+), 43 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c >> index e2531cf59266..09a0ad611002 100644 >> --- a/drivers/gpu/drm/i915/intel_atomic.c >> +++ b/drivers/gpu/drm/i915/intel_atomic.c >> @@ -214,8 +214,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 5a462e9a4d14..8ef0dbb33afd 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -3250,32 +3250,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; >> @@ -3890,15 +3864,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); >> @@ -3909,11 +3891,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); >> - } >> + return 0; >> } >> >> /* Program iCLKIP clock to the desired frequency */ >> @@ -4773,9 +4751,6 @@ static void intel_pre_plane_update(struct intel_crtc *crtc) >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct intel_crtc_atomic_commit *atomic = &crtc->atomic; >> >> - if (atomic->wait_for_flips) >> - intel_crtc_wait_for_pending_flips(&crtc->base); >> - >> if (atomic->disable_fbc) >> intel_fbc_disable_crtc(crtc); >> >> @@ -11701,7 +11676,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, >> >> switch (plane->type) { >> case DRM_PLANE_TYPE_PRIMARY: >> - intel_crtc->atomic.wait_for_flips = true; >> intel_crtc->atomic.pre_disable_primary = turn_off; >> intel_crtc->atomic.post_enable_primary = turn_on; >> >> @@ -13473,6 +13447,12 @@ intel_prepare_plane_fb(struct drm_plane *plane, >> struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb); >> int ret = 0; >> >> + if (plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj) { > Would feel safer is we just asked if the CRTC had pending flips > irrespective of old_obj. Do you plan on moving the pending flips from > CRTC to plane? That would seem to be implied here. This preserves old behavior since page flips can only happen on primary planes. Do you want cursor updates to wait on pending primary flips? >> + ret = intel_crtc_wait_for_pending_flips(plane->state->crtc); >> + if (ret) >> + return ret; >> + } >> + >> if (obj == old_obj) >> return 0; >> >> @@ -13492,6 +13472,20 @@ intel_prepare_plane_fb(struct drm_plane *plane, >> ret = intel_pin_and_fence_fb_obj(plane, fb, new_state, NULL, NULL); >> } >> >> + /* 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. >> + */ >> + if (!ret && plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj) >> + ret = i915_gem_object_wait_rendering(old_obj, true); > Technically I can create a batch with a WAIT_ON_EVENT for a secondary > plane as well. This path need only be done in a modeset, not for a > simple plane flip. (But we actually need to serialise with rendering to > old_obj before regarding the plane flip as complete.) Is there a > plane-prepare-modeset hook? The comment didn't explain that, is hiding this behind a needs_modeset(crtc_state) sufficient here? ~Maarten
On Tue, Jul 21, 2015 at 02:33:33PM +0200, Maarten Lankhorst wrote: > >> + if (plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj) { > > Would feel safer is we just asked if the CRTC had pending flips > > irrespective of old_obj. Do you plan on moving the pending flips from > > CRTC to plane? That would seem to be implied here. > This preserves old behavior since page flips can only happen on primary planes. > Do you want cursor updates to wait on pending primary flips? Cursor updates need to be serialized with primary updates, yes (or else there are some fun scenarios in which you can hang the display engine). What I was actually thinking was using plane->state for tracking the flips. Having just reread what I wrote that didn't come across. I would rather see fewer primary_plane special cases and more no-ops on secondary planes. I think you will end up using such infrastructure on the secondary planes eventually. > >> + /* 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. > >> + */ > >> + if (!ret && plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj) > >> + ret = i915_gem_object_wait_rendering(old_obj, true); > > Technically I can create a batch with a WAIT_ON_EVENT for a secondary > > plane as well. This path need only be done in a modeset, not for a > > simple plane flip. (But we actually need to serialise with rendering to > > old_obj before regarding the plane flip as complete.) Is there a > > plane-prepare-modeset hook? > The comment didn't explain that, is hiding this behind a needs_modeset(crtc_state) sufficient here? For the purpose of limiting the wait to when the pipe dimensions may change, yes. -Chris
Op 21-07-15 om 15:31 schreef Chris Wilson: > On Tue, Jul 21, 2015 at 02:33:33PM +0200, Maarten Lankhorst wrote: >>>> + if (plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj) { >>> Would feel safer is we just asked if the CRTC had pending flips >>> irrespective of old_obj. Do you plan on moving the pending flips from >>> CRTC to plane? That would seem to be implied here. >> This preserves old behavior since page flips can only happen on primary planes. >> Do you want cursor updates to wait on pending primary flips? > Cursor updates need to be serialized with primary updates, yes (or else > there are some fun scenarios in which you can hang the display engine). Ok but right now we don't sync this.. won't blocking on flips for moving the mouse in Xorg be bad for performance? > What I was actually thinking was using plane->state for tracking the > flips. Having just reread what I wrote that didn't come across. I would > rather see fewer primary_plane special cases and more no-ops on > secondary planes. I think you will end up using such infrastructure > on the secondary planes eventually. I think the current infrastructure will die eventually and perhaps be done asynchronously. I just don't know yet what will replace it. >>>> + /* 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. >>>> + */ >>>> + if (!ret && plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj) >>>> + ret = i915_gem_object_wait_rendering(old_obj, true); >>> Technically I can create a batch with a WAIT_ON_EVENT for a secondary >>> plane as well. This path need only be done in a modeset, not for a >>> simple plane flip. (But we actually need to serialise with rendering to >>> old_obj before regarding the plane flip as complete.) Is there a >>> plane-prepare-modeset hook? >> The comment didn't explain that, is hiding this behind a needs_modeset(crtc_state) sufficient here? > For the purpose of limiting the wait to when the pipe dimensions may > change, yes. Is that only needed during full modesets, or when changing PIPESRC too? ~Maarten
On Tue, Jul 21, 2015 at 03:59:25PM +0200, Maarten Lankhorst wrote: > Op 21-07-15 om 15:31 schreef Chris Wilson: > > On Tue, Jul 21, 2015 at 02:33:33PM +0200, Maarten Lankhorst wrote: > >>>> + if (plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj) { > >>> Would feel safer is we just asked if the CRTC had pending flips > >>> irrespective of old_obj. Do you plan on moving the pending flips from > >>> CRTC to plane? That would seem to be implied here. > >> This preserves old behavior since page flips can only happen on primary planes. > >> Do you want cursor updates to wait on pending primary flips? > > Cursor updates need to be serialized with primary updates, yes (or else > > there are some fun scenarios in which you can hang the display engine). > Ok but right now we don't sync this.. won't blocking on flips for moving the mouse in Xorg be bad for performance? Off the top of my head, we only need to worry about modesets, which presumedly are serialized between the planes. > > What I was actually thinking was using plane->state for tracking the > > flips. Having just reread what I wrote that didn't come across. I would > > rather see fewer primary_plane special cases and more no-ops on > > secondary planes. I think you will end up using such infrastructure > > on the secondary planes eventually. > I think the current infrastructure will die eventually and perhaps be done asynchronously. I just don't know yet what will replace it. > > >>>> + /* 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. > >>>> + */ > >>>> + if (!ret && plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj) > >>>> + ret = i915_gem_object_wait_rendering(old_obj, true); > >>> Technically I can create a batch with a WAIT_ON_EVENT for a secondary > >>> plane as well. This path need only be done in a modeset, not for a > >>> simple plane flip. (But we actually need to serialise with rendering to > >>> old_obj before regarding the plane flip as complete.) Is there a > >>> plane-prepare-modeset hook? > >> The comment didn't explain that, is hiding this behind a needs_modeset(crtc_state) sufficient here? > > For the purpose of limiting the wait to when the pipe dimensions may > > change, yes. > Is that only needed during full modesets, or when changing PIPESRC too? The triggers are the display modeline values (whatever HTOTAL/VTOTAL registers collectively are called), so just full modesets. -Chris
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index e2531cf59266..09a0ad611002 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -214,8 +214,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 5a462e9a4d14..8ef0dbb33afd 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3250,32 +3250,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; @@ -3890,15 +3864,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); @@ -3909,11 +3891,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); - } + return 0; } /* Program iCLKIP clock to the desired frequency */ @@ -4773,9 +4751,6 @@ static void intel_pre_plane_update(struct intel_crtc *crtc) struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc_atomic_commit *atomic = &crtc->atomic; - if (atomic->wait_for_flips) - intel_crtc_wait_for_pending_flips(&crtc->base); - if (atomic->disable_fbc) intel_fbc_disable_crtc(crtc); @@ -11701,7 +11676,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, switch (plane->type) { case DRM_PLANE_TYPE_PRIMARY: - intel_crtc->atomic.wait_for_flips = true; intel_crtc->atomic.pre_disable_primary = turn_off; intel_crtc->atomic.post_enable_primary = turn_on; @@ -13473,6 +13447,12 @@ intel_prepare_plane_fb(struct drm_plane *plane, struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb); int ret = 0; + if (plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj) { + ret = intel_crtc_wait_for_pending_flips(plane->state->crtc); + if (ret) + return ret; + } + if (obj == old_obj) return 0; @@ -13492,6 +13472,20 @@ intel_prepare_plane_fb(struct drm_plane *plane, ret = intel_pin_and_fence_fb_obj(plane, fb, new_state, NULL, NULL); } + /* 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. + */ + if (!ret && plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj) + ret = i915_gem_object_wait_rendering(old_obj, true); + if (ret == 0) i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index e6b04941f9ca..0920a6fc459a 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -489,7 +489,6 @@ struct skl_pipe_wm { */ struct intel_crtc_atomic_commit { /* Sleepable operations to perform before commit */ - bool wait_for_flips; bool disable_fbc; bool disable_ips; bool disable_cxsr; @@ -1126,7 +1125,6 @@ enum intel_display_power_domain intel_display_port_power_domain(struct intel_encoder *intel_encoder); void intel_mode_from_pipe_config(struct drm_display_mode *mode, struct intel_crtc_state *pipe_config); -void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc); void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file); int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state);
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 | 72 +++++++++++++++++------------------- drivers/gpu/drm/i915/intel_drv.h | 2 - 3 files changed, 33 insertions(+), 43 deletions(-)