| Message ID | 20231016080803.3313020-1-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display: Use dma_fence interfaces instead of i915_sw_fence | expand |
Hey, Thanks, this version looks a lot better than duplicating drm_gem_plane_helper_prepare_fb functionality. :) Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> On 2023-10-16 10:08, Jouni Högander wrote: > We are preparing for Xe driver. Xe driver doesn't have i915_sw_fence > implementation. Lets drop i915_sw_fence usage from display code and > use dma_fence interfaces directly. > > For this purpose stack dma fences from related objects into old and new > plane states using drm_gem_plane_helper_prepare_fb. Then wait for these > stacked fences during atomic commit. > > There is no be need for separate GPU reset handling in > intel_atomic_commit_fence_wait as the fences are signaled when GPU hang is > detected and GPU is being reset. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > --- > drivers/gpu/drm/i915/display/intel_atomic.c | 3 - > .../gpu/drm/i915/display/intel_atomic_plane.c | 49 +++--------- > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++------------- > .../drm/i915/display/intel_display_types.h | 2 - > 4 files changed, 37 insertions(+), 95 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c > index 5d18145da279..ec0d5168b503 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c > @@ -331,9 +331,6 @@ void intel_atomic_state_free(struct drm_atomic_state *_state) > > drm_atomic_state_default_release(&state->base); > kfree(state->global_objs); > - > - i915_sw_fence_fini(&state->commit_ready); > - > kfree(state); > } > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > index b1074350616c..d4f9168ec42c 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > @@ -32,6 +32,7 @@ > */ > > #include <drm/drm_atomic_helper.h> > +#include <drm/drm_gem_atomic_helper.h> > #include <drm/drm_blend.h> > #include <drm/drm_fourcc.h> > > @@ -1035,7 +1036,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > struct intel_atomic_state *state = > to_intel_atomic_state(new_plane_state->uapi.state); > struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > - const struct intel_plane_state *old_plane_state = > + struct intel_plane_state *old_plane_state = > intel_atomic_get_old_plane_state(state, plane); > struct drm_i915_gem_object *obj = intel_fb_obj(new_plane_state->hw.fb); > struct drm_i915_gem_object *old_obj = intel_fb_obj(old_plane_state->hw.fb); > @@ -1057,56 +1058,30 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > * This should only fail upon a hung GPU, in which case we > * can safely continue. > */ > - if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > - ret = i915_sw_fence_await_reservation(&state->commit_ready, > - old_obj->base.resv, > - false, 0, > - GFP_KERNEL); > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state) && > + !dma_resv_test_signaled(old_obj->base.resv, > + dma_resv_usage_rw(false))) { > + ret = drm_gem_plane_helper_prepare_fb(_plane, &old_plane_state->uapi); > if (ret < 0) > return ret; > } > } > > - if (new_plane_state->uapi.fence) { /* explicit fencing */ > - i915_gem_fence_wait_priority(new_plane_state->uapi.fence, > - &attr); > - ret = i915_sw_fence_await_dma_fence(&state->commit_ready, > - new_plane_state->uapi.fence, > - i915_fence_timeout(dev_priv), > - GFP_KERNEL); > - if (ret < 0) > - return ret; > - } > - > if (!obj) > return 0; > > - > ret = intel_plane_pin_fb(new_plane_state); > if (ret) > return ret; > > - i915_gem_object_wait_priority(obj, 0, &attr); > - > - if (!new_plane_state->uapi.fence) { /* implicit fencing */ > - struct dma_resv_iter cursor; > - struct dma_fence *fence; > + ret = drm_gem_plane_helper_prepare_fb(_plane, &new_plane_state->uapi); > + if (ret < 0) > + goto unpin_fb; > > - ret = i915_sw_fence_await_reservation(&state->commit_ready, > - obj->base.resv, false, > - i915_fence_timeout(dev_priv), > - GFP_KERNEL); > - if (ret < 0) > - goto unpin_fb; > + if (new_plane_state->uapi.fence) { > + i915_gem_fence_wait_priority(new_plane_state->uapi.fence, > + &attr); > > - dma_resv_iter_begin(&cursor, obj->base.resv, > - DMA_RESV_USAGE_WRITE); > - dma_resv_for_each_fence_unlocked(&cursor, fence) { > - intel_display_rps_boost_after_vblank(new_plane_state->hw.crtc, > - fence); > - } > - dma_resv_iter_end(&cursor); > - } else { > intel_display_rps_boost_after_vblank(new_plane_state->hw.crtc, > new_plane_state->uapi.fence); > } > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 28d85e1e858e..48df0559df00 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -48,6 +48,7 @@ > #include "g4x_dp.h" > #include "g4x_hdmi.h" > #include "hsw_ips.h" > +#include "i915_config.h" > #include "i915_drv.h" > #include "i915_reg.h" > #include "i915_utils.h" > @@ -7056,29 +7057,34 @@ void intel_atomic_helper_free_state_worker(struct work_struct *work) > > static void intel_atomic_commit_fence_wait(struct intel_atomic_state *intel_state) > { > - struct wait_queue_entry wait_fence, wait_reset; > - struct drm_i915_private *dev_priv = to_i915(intel_state->base.dev); > - > - init_wait_entry(&wait_fence, 0); > - init_wait_entry(&wait_reset, 0); > - for (;;) { > - prepare_to_wait(&intel_state->commit_ready.wait, > - &wait_fence, TASK_UNINTERRUPTIBLE); > - prepare_to_wait(bit_waitqueue(&to_gt(dev_priv)->reset.flags, > - I915_RESET_MODESET), > - &wait_reset, TASK_UNINTERRUPTIBLE); > + struct drm_i915_private *i915 = to_i915(intel_state->base.dev); > + struct drm_plane *plane; > + struct drm_plane_state *new_plane_state, *old_plane_state; > + int ret, i; > > + for_each_oldnew_plane_in_state(&intel_state->base, plane, > + old_plane_state, new_plane_state, i) { > + if (new_plane_state->fence) { > + ret = dma_fence_wait_timeout(new_plane_state->fence, > + false, > + i915_fence_timeout(i915)); > + if (ret <= 0) > + break; > > - if (i915_sw_fence_done(&intel_state->commit_ready) || > - test_bit(I915_RESET_MODESET, &to_gt(dev_priv)->reset.flags)) > - break; > + dma_fence_put(new_plane_state->fence); > + new_plane_state->fence = NULL; > + } > + if (old_plane_state->fence) { > + ret = dma_fence_wait_timeout(old_plane_state->fence, > + false, > + i915_fence_timeout(i915)); > + if (ret <= 0) > + break; > > - schedule(); > + dma_fence_put(old_plane_state->fence); > + old_plane_state->fence = NULL; > + } > } > - finish_wait(&intel_state->commit_ready.wait, &wait_fence); > - finish_wait(bit_waitqueue(&to_gt(dev_priv)->reset.flags, > - I915_RESET_MODESET), > - &wait_reset); > } > > static void intel_atomic_cleanup_work(struct work_struct *work) > @@ -7370,32 +7376,6 @@ static void intel_atomic_commit_work(struct work_struct *work) > intel_atomic_commit_tail(state); > } > > -static int > -intel_atomic_commit_ready(struct i915_sw_fence *fence, > - enum i915_sw_fence_notify notify) > -{ > - struct intel_atomic_state *state = > - container_of(fence, struct intel_atomic_state, commit_ready); > - > - switch (notify) { > - case FENCE_COMPLETE: > - /* we do blocking waits in the worker, nothing to do here */ > - break; > - case FENCE_FREE: > - { > - struct drm_i915_private *i915 = to_i915(state->base.dev); > - struct intel_atomic_helper *helper = > - &i915->display.atomic_helper; > - > - if (llist_add(&state->freed, &helper->free_list)) > - queue_work(i915->unordered_wq, &helper->free_work); > - break; > - } > - } > - > - return NOTIFY_DONE; > -} > - > static void intel_atomic_track_fbs(struct intel_atomic_state *state) > { > struct intel_plane_state *old_plane_state, *new_plane_state; > @@ -7418,10 +7398,6 @@ int intel_atomic_commit(struct drm_device *dev, struct drm_atomic_state *_state, > > state->wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm); > > - drm_atomic_state_get(&state->base); > - i915_sw_fence_init(&state->commit_ready, > - intel_atomic_commit_ready); > - > /* > * The intel_legacy_cursor_update() fast path takes care > * of avoiding the vblank waits for simple cursor > @@ -7454,7 +7430,6 @@ int intel_atomic_commit(struct drm_device *dev, struct drm_atomic_state *_state, > if (ret) { > drm_dbg_atomic(&dev_priv->drm, > "Preparing state failed with %i\n", ret); > - i915_sw_fence_commit(&state->commit_ready); > intel_runtime_pm_put(&dev_priv->runtime_pm, state->wakeref); > return ret; > } > @@ -7470,8 +7445,6 @@ int intel_atomic_commit(struct drm_device *dev, struct drm_atomic_state *_state, > struct intel_crtc *crtc; > int i; > > - i915_sw_fence_commit(&state->commit_ready); > - > for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) > intel_color_cleanup_commit(new_crtc_state); > > @@ -7485,7 +7458,6 @@ int intel_atomic_commit(struct drm_device *dev, struct drm_atomic_state *_state, > drm_atomic_state_get(&state->base); > INIT_WORK(&state->base.commit_work, intel_atomic_commit_work); > > - i915_sw_fence_commit(&state->commit_ready); > if (nonblock && state->modeset) { > queue_work(dev_priv->display.wq.modeset, &state->base.commit_work); > } else if (nonblock) { > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > index 8d8b2f8d37a9..a685c527c7d1 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -670,8 +670,6 @@ struct intel_atomic_state { > > bool rps_interactive; > > - struct i915_sw_fence commit_ready; > - > struct llist_node freed; > }; >
On Mon, Oct 16, 2023 at 11:08:03AM +0300, Jouni Högander wrote: > We are preparing for Xe driver. Xe driver doesn't have i915_sw_fence > implementation. Lets drop i915_sw_fence usage from display code and > use dma_fence interfaces directly. > > For this purpose stack dma fences from related objects into old and new > plane states using drm_gem_plane_helper_prepare_fb. Then wait for these > stacked fences during atomic commit. > > There is no be need for separate GPU reset handling in > intel_atomic_commit_fence_wait as the fences are signaled when GPU hang is > detected and GPU is being reset. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > --- > drivers/gpu/drm/i915/display/intel_atomic.c | 3 - > .../gpu/drm/i915/display/intel_atomic_plane.c | 49 +++--------- > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++------------- > .../drm/i915/display/intel_display_types.h | 2 - > 4 files changed, 37 insertions(+), 95 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c > index 5d18145da279..ec0d5168b503 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c > @@ -331,9 +331,6 @@ void intel_atomic_state_free(struct drm_atomic_state *_state) > > drm_atomic_state_default_release(&state->base); > kfree(state->global_objs); > - > - i915_sw_fence_fini(&state->commit_ready); > - > kfree(state); > } > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > index b1074350616c..d4f9168ec42c 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > @@ -32,6 +32,7 @@ > */ > > #include <drm/drm_atomic_helper.h> > +#include <drm/drm_gem_atomic_helper.h> > #include <drm/drm_blend.h> > #include <drm/drm_fourcc.h> > > @@ -1035,7 +1036,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > struct intel_atomic_state *state = > to_intel_atomic_state(new_plane_state->uapi.state); > struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > - const struct intel_plane_state *old_plane_state = > + struct intel_plane_state *old_plane_state = > intel_atomic_get_old_plane_state(state, plane); > struct drm_i915_gem_object *obj = intel_fb_obj(new_plane_state->hw.fb); > struct drm_i915_gem_object *old_obj = intel_fb_obj(old_plane_state->hw.fb); > @@ -1057,56 +1058,30 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > * This should only fail upon a hung GPU, in which case we > * can safely continue. > */ > - if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > - ret = i915_sw_fence_await_reservation(&state->commit_ready, > - old_obj->base.resv, > - false, 0, > - GFP_KERNEL); > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state) && > + !dma_resv_test_signaled(old_obj->base.resv, > + dma_resv_usage_rw(false))) { > + ret = drm_gem_plane_helper_prepare_fb(_plane, &old_plane_state->uapi); > if (ret < 0) > return ret; > } > } > > - if (new_plane_state->uapi.fence) { /* explicit fencing */ > - i915_gem_fence_wait_priority(new_plane_state->uapi.fence, > - &attr); > - ret = i915_sw_fence_await_dma_fence(&state->commit_ready, > - new_plane_state->uapi.fence, > - i915_fence_timeout(dev_priv), > - GFP_KERNEL); > - if (ret < 0) > - return ret; > - } > - > if (!obj) > return 0; > > - > ret = intel_plane_pin_fb(new_plane_state); > if (ret) > return ret; > > - i915_gem_object_wait_priority(obj, 0, &attr); > - > - if (!new_plane_state->uapi.fence) { /* implicit fencing */ > - struct dma_resv_iter cursor; > - struct dma_fence *fence; > + ret = drm_gem_plane_helper_prepare_fb(_plane, &new_plane_state->uapi); I don't think we can use that as is due to bigjoiner stuff. I think we'd need a slightly lower level variant that takes the fb+fence in explicitly instead of the full plane state. And I suppose we already have a slight bug here where only the master pipe's plane will consult the explicit fence and the rest will take the implicit sync path. > + if (ret < 0) > + goto unpin_fb; > > - ret = i915_sw_fence_await_reservation(&state->commit_ready, > - obj->base.resv, false, > - i915_fence_timeout(dev_priv), > - GFP_KERNEL); > - if (ret < 0) > - goto unpin_fb; > + if (new_plane_state->uapi.fence) { > + i915_gem_fence_wait_priority(new_plane_state->uapi.fence, > + &attr); > > - dma_resv_iter_begin(&cursor, obj->base.resv, > - DMA_RESV_USAGE_WRITE); > - dma_resv_for_each_fence_unlocked(&cursor, fence) { > - intel_display_rps_boost_after_vblank(new_plane_state->hw.crtc, > - fence); > - } > - dma_resv_iter_end(&cursor); > - } else { > intel_display_rps_boost_after_vblank(new_plane_state->hw.crtc, > new_plane_state->uapi.fence); > } > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 28d85e1e858e..48df0559df00 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -48,6 +48,7 @@ > #include "g4x_dp.h" > #include "g4x_hdmi.h" > #include "hsw_ips.h" > +#include "i915_config.h" > #include "i915_drv.h" > #include "i915_reg.h" > #include "i915_utils.h" > @@ -7056,29 +7057,34 @@ void intel_atomic_helper_free_state_worker(struct work_struct *work) > > static void intel_atomic_commit_fence_wait(struct intel_atomic_state *intel_state) > { > - struct wait_queue_entry wait_fence, wait_reset; > - struct drm_i915_private *dev_priv = to_i915(intel_state->base.dev); > - > - init_wait_entry(&wait_fence, 0); > - init_wait_entry(&wait_reset, 0); > - for (;;) { > - prepare_to_wait(&intel_state->commit_ready.wait, > - &wait_fence, TASK_UNINTERRUPTIBLE); > - prepare_to_wait(bit_waitqueue(&to_gt(dev_priv)->reset.flags, > - I915_RESET_MODESET), > - &wait_reset, TASK_UNINTERRUPTIBLE); > + struct drm_i915_private *i915 = to_i915(intel_state->base.dev); > + struct drm_plane *plane; > + struct drm_plane_state *new_plane_state, *old_plane_state; > + int ret, i; > > + for_each_oldnew_plane_in_state(&intel_state->base, plane, > + old_plane_state, new_plane_state, i) { > + if (new_plane_state->fence) { > + ret = dma_fence_wait_timeout(new_plane_state->fence, > + false, > + i915_fence_timeout(i915)); > + if (ret <= 0) > + break; > > - if (i915_sw_fence_done(&intel_state->commit_ready) || > - test_bit(I915_RESET_MODESET, &to_gt(dev_priv)->reset.flags)) > - break; > + dma_fence_put(new_plane_state->fence); > + new_plane_state->fence = NULL; > + } > + if (old_plane_state->fence) { > + ret = dma_fence_wait_timeout(old_plane_state->fence, > + false, > + i915_fence_timeout(i915)); > + if (ret <= 0) > + break; > > - schedule(); > + dma_fence_put(old_plane_state->fence); > + old_plane_state->fence = NULL; > + } > } > - finish_wait(&intel_state->commit_ready.wait, &wait_fence); > - finish_wait(bit_waitqueue(&to_gt(dev_priv)->reset.flags, > - I915_RESET_MODESET), > - &wait_reset); > } > > static void intel_atomic_cleanup_work(struct work_struct *work) > @@ -7370,32 +7376,6 @@ static void intel_atomic_commit_work(struct work_struct *work) > intel_atomic_commit_tail(state); > } > > -static int > -intel_atomic_commit_ready(struct i915_sw_fence *fence, > - enum i915_sw_fence_notify notify) > -{ > - struct intel_atomic_state *state = > - container_of(fence, struct intel_atomic_state, commit_ready); > - > - switch (notify) { > - case FENCE_COMPLETE: > - /* we do blocking waits in the worker, nothing to do here */ > - break; > - case FENCE_FREE: > - { > - struct drm_i915_private *i915 = to_i915(state->base.dev); > - struct intel_atomic_helper *helper = > - &i915->display.atomic_helper; > - > - if (llist_add(&state->freed, &helper->free_list)) > - queue_work(i915->unordered_wq, &helper->free_work); > - break; > - } > - } > - > - return NOTIFY_DONE; > -} > - > static void intel_atomic_track_fbs(struct intel_atomic_state *state) > { > struct intel_plane_state *old_plane_state, *new_plane_state; > @@ -7418,10 +7398,6 @@ int intel_atomic_commit(struct drm_device *dev, struct drm_atomic_state *_state, > > state->wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm); > > - drm_atomic_state_get(&state->base); > - i915_sw_fence_init(&state->commit_ready, > - intel_atomic_commit_ready); > - > /* > * The intel_legacy_cursor_update() fast path takes care > * of avoiding the vblank waits for simple cursor > @@ -7454,7 +7430,6 @@ int intel_atomic_commit(struct drm_device *dev, struct drm_atomic_state *_state, > if (ret) { > drm_dbg_atomic(&dev_priv->drm, > "Preparing state failed with %i\n", ret); > - i915_sw_fence_commit(&state->commit_ready); > intel_runtime_pm_put(&dev_priv->runtime_pm, state->wakeref); > return ret; > } > @@ -7470,8 +7445,6 @@ int intel_atomic_commit(struct drm_device *dev, struct drm_atomic_state *_state, > struct intel_crtc *crtc; > int i; > > - i915_sw_fence_commit(&state->commit_ready); > - > for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) > intel_color_cleanup_commit(new_crtc_state); > > @@ -7485,7 +7458,6 @@ int intel_atomic_commit(struct drm_device *dev, struct drm_atomic_state *_state, > drm_atomic_state_get(&state->base); > INIT_WORK(&state->base.commit_work, intel_atomic_commit_work); > > - i915_sw_fence_commit(&state->commit_ready); > if (nonblock && state->modeset) { > queue_work(dev_priv->display.wq.modeset, &state->base.commit_work); > } else if (nonblock) { > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > index 8d8b2f8d37a9..a685c527c7d1 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -670,8 +670,6 @@ struct intel_atomic_state { > > bool rps_interactive; > > - struct i915_sw_fence commit_ready; > - > struct llist_node freed; > }; > > -- > 2.34.1
On 2023-10-18 17:19, Ville Syrjälä wrote: > On Mon, Oct 16, 2023 at 11:08:03AM +0300, Jouni Högander wrote: >> We are preparing for Xe driver. Xe driver doesn't have i915_sw_fence >> implementation. Lets drop i915_sw_fence usage from display code and >> use dma_fence interfaces directly. >> >> For this purpose stack dma fences from related objects into old and new >> plane states using drm_gem_plane_helper_prepare_fb. Then wait for these >> stacked fences during atomic commit. >> >> There is no be need for separate GPU reset handling in >> intel_atomic_commit_fence_wait as the fences are signaled when GPU hang is >> detected and GPU is being reset. >> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: José Roberto de Souza <jose.souza@intel.com> >> >> Signed-off-by: Jouni Högander <jouni.hogander@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_atomic.c | 3 - >> .../gpu/drm/i915/display/intel_atomic_plane.c | 49 +++--------- >> drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++------------- >> .../drm/i915/display/intel_display_types.h | 2 - >> 4 files changed, 37 insertions(+), 95 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c >> index 5d18145da279..ec0d5168b503 100644 >> --- a/drivers/gpu/drm/i915/display/intel_atomic.c >> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c >> @@ -331,9 +331,6 @@ void intel_atomic_state_free(struct drm_atomic_state *_state) >> >> drm_atomic_state_default_release(&state->base); >> kfree(state->global_objs); >> - >> - i915_sw_fence_fini(&state->commit_ready); >> - >> kfree(state); >> } >> >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c >> index b1074350616c..d4f9168ec42c 100644 >> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c >> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c >> @@ -32,6 +32,7 @@ >> */ >> >> #include <drm/drm_atomic_helper.h> >> +#include <drm/drm_gem_atomic_helper.h> >> #include <drm/drm_blend.h> >> #include <drm/drm_fourcc.h> >> >> @@ -1035,7 +1036,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, >> struct intel_atomic_state *state = >> to_intel_atomic_state(new_plane_state->uapi.state); >> struct drm_i915_private *dev_priv = to_i915(plane->base.dev); >> - const struct intel_plane_state *old_plane_state = >> + struct intel_plane_state *old_plane_state = >> intel_atomic_get_old_plane_state(state, plane); >> struct drm_i915_gem_object *obj = intel_fb_obj(new_plane_state->hw.fb); >> struct drm_i915_gem_object *old_obj = intel_fb_obj(old_plane_state->hw.fb); >> @@ -1057,56 +1058,30 @@ intel_prepare_plane_fb(struct drm_plane *_plane, >> * This should only fail upon a hung GPU, in which case we >> * can safely continue. >> */ >> - if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { >> - ret = i915_sw_fence_await_reservation(&state->commit_ready, >> - old_obj->base.resv, >> - false, 0, >> - GFP_KERNEL); >> + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state) && >> + !dma_resv_test_signaled(old_obj->base.resv, >> + dma_resv_usage_rw(false))) { >> + ret = drm_gem_plane_helper_prepare_fb(_plane, &old_plane_state->uapi); >> if (ret < 0) >> return ret; >> } >> } >> >> - if (new_plane_state->uapi.fence) { /* explicit fencing */ >> - i915_gem_fence_wait_priority(new_plane_state->uapi.fence, >> - &attr); >> - ret = i915_sw_fence_await_dma_fence(&state->commit_ready, >> - new_plane_state->uapi.fence, >> - i915_fence_timeout(dev_priv), >> - GFP_KERNEL); >> - if (ret < 0) >> - return ret; >> - } >> - >> if (!obj) >> return 0; >> >> - >> ret = intel_plane_pin_fb(new_plane_state); >> if (ret) >> return ret; >> >> - i915_gem_object_wait_priority(obj, 0, &attr); >> - >> - if (!new_plane_state->uapi.fence) { /* implicit fencing */ >> - struct dma_resv_iter cursor; >> - struct dma_fence *fence; >> + ret = drm_gem_plane_helper_prepare_fb(_plane, &new_plane_state->uapi); > > I don't think we can use that as is due to bigjoiner stuff. > I think we'd need a slightly lower level variant that takes > the fb+fence in explicitly instead of the full plane state. > > And I suppose we already have a slight bug here where only the > master pipe's plane will consult the explicit fence and the rest > will take the implicit sync path. Why would bigjoiner fail? If bigjoiner happens, the uapi fb will be fenced at least once. Cheers, ~Maarten
On Wed, Oct 18, 2023 at 05:23:00PM +0200, Maarten Lankhorst wrote: > > > On 2023-10-18 17:19, Ville Syrjälä wrote: > > On Mon, Oct 16, 2023 at 11:08:03AM +0300, Jouni Högander wrote: > >> We are preparing for Xe driver. Xe driver doesn't have i915_sw_fence > >> implementation. Lets drop i915_sw_fence usage from display code and > >> use dma_fence interfaces directly. > >> > >> For this purpose stack dma fences from related objects into old and new > >> plane states using drm_gem_plane_helper_prepare_fb. Then wait for these > >> stacked fences during atomic commit. > >> > >> There is no be need for separate GPU reset handling in > >> intel_atomic_commit_fence_wait as the fences are signaled when GPU hang is > >> detected and GPU is being reset. > >> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> Cc: José Roberto de Souza <jose.souza@intel.com> > >> > >> Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > >> --- > >> drivers/gpu/drm/i915/display/intel_atomic.c | 3 - > >> .../gpu/drm/i915/display/intel_atomic_plane.c | 49 +++--------- > >> drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++------------- > >> .../drm/i915/display/intel_display_types.h | 2 - > >> 4 files changed, 37 insertions(+), 95 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c > >> index 5d18145da279..ec0d5168b503 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_atomic.c > >> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c > >> @@ -331,9 +331,6 @@ void intel_atomic_state_free(struct drm_atomic_state *_state) > >> > >> drm_atomic_state_default_release(&state->base); > >> kfree(state->global_objs); > >> - > >> - i915_sw_fence_fini(&state->commit_ready); > >> - > >> kfree(state); > >> } > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >> index b1074350616c..d4f9168ec42c 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >> @@ -32,6 +32,7 @@ > >> */ > >> > >> #include <drm/drm_atomic_helper.h> > >> +#include <drm/drm_gem_atomic_helper.h> > >> #include <drm/drm_blend.h> > >> #include <drm/drm_fourcc.h> > >> > >> @@ -1035,7 +1036,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > >> struct intel_atomic_state *state = > >> to_intel_atomic_state(new_plane_state->uapi.state); > >> struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > >> - const struct intel_plane_state *old_plane_state = > >> + struct intel_plane_state *old_plane_state = > >> intel_atomic_get_old_plane_state(state, plane); > >> struct drm_i915_gem_object *obj = intel_fb_obj(new_plane_state->hw.fb); > >> struct drm_i915_gem_object *old_obj = intel_fb_obj(old_plane_state->hw.fb); > >> @@ -1057,56 +1058,30 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > >> * This should only fail upon a hung GPU, in which case we > >> * can safely continue. > >> */ > >> - if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > >> - ret = i915_sw_fence_await_reservation(&state->commit_ready, > >> - old_obj->base.resv, > >> - false, 0, > >> - GFP_KERNEL); > >> + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state) && > >> + !dma_resv_test_signaled(old_obj->base.resv, > >> + dma_resv_usage_rw(false))) { > >> + ret = drm_gem_plane_helper_prepare_fb(_plane, &old_plane_state->uapi); > >> if (ret < 0) > >> return ret; > >> } > >> } > >> > >> - if (new_plane_state->uapi.fence) { /* explicit fencing */ > >> - i915_gem_fence_wait_priority(new_plane_state->uapi.fence, > >> - &attr); > >> - ret = i915_sw_fence_await_dma_fence(&state->commit_ready, > >> - new_plane_state->uapi.fence, > >> - i915_fence_timeout(dev_priv), > >> - GFP_KERNEL); > >> - if (ret < 0) > >> - return ret; > >> - } > >> - > >> if (!obj) > >> return 0; > >> > >> - > >> ret = intel_plane_pin_fb(new_plane_state); > >> if (ret) > >> return ret; > >> > >> - i915_gem_object_wait_priority(obj, 0, &attr); > >> - > >> - if (!new_plane_state->uapi.fence) { /* implicit fencing */ > >> - struct dma_resv_iter cursor; > >> - struct dma_fence *fence; > >> + ret = drm_gem_plane_helper_prepare_fb(_plane, &new_plane_state->uapi); > > > > I don't think we can use that as is due to bigjoiner stuff. > > I think we'd need a slightly lower level variant that takes > > the fb+fence in explicitly instead of the full plane state. > > > > And I suppose we already have a slight bug here where only the > > master pipe's plane will consult the explicit fence and the rest > > will take the implicit sync path. > Why would bigjoiner fail? If bigjoiner happens, the uapi fb will be > fenced at least once. Hmm. Yeah, I suppose that should cover it since we don't consider plane visibility anywhere. And, I guess I was wrong about the existing bug as well since there should be no uapi.fb set on the slave planes, and so no extra implicit sync fence will be added.
On Mon, Oct 16, 2023 at 11:08:03AM +0300, Jouni Högander wrote: > We are preparing for Xe driver. Xe driver doesn't have i915_sw_fence > implementation. Lets drop i915_sw_fence usage from display code and > use dma_fence interfaces directly. > > For this purpose stack dma fences from related objects into old and new > plane states using drm_gem_plane_helper_prepare_fb. Then wait for these > stacked fences during atomic commit. > > There is no be need for separate GPU reset handling in > intel_atomic_commit_fence_wait as the fences are signaled when GPU hang is > detected and GPU is being reset. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > --- > drivers/gpu/drm/i915/display/intel_atomic.c | 3 - > .../gpu/drm/i915/display/intel_atomic_plane.c | 49 +++--------- > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++------------- > .../drm/i915/display/intel_display_types.h | 2 - > 4 files changed, 37 insertions(+), 95 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c > index 5d18145da279..ec0d5168b503 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c > @@ -331,9 +331,6 @@ void intel_atomic_state_free(struct drm_atomic_state *_state) > > drm_atomic_state_default_release(&state->base); > kfree(state->global_objs); > - > - i915_sw_fence_fini(&state->commit_ready); > - > kfree(state); > } > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > index b1074350616c..d4f9168ec42c 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > @@ -32,6 +32,7 @@ > */ > > #include <drm/drm_atomic_helper.h> > +#include <drm/drm_gem_atomic_helper.h> > #include <drm/drm_blend.h> > #include <drm/drm_fourcc.h> > > @@ -1035,7 +1036,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > struct intel_atomic_state *state = > to_intel_atomic_state(new_plane_state->uapi.state); > struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > - const struct intel_plane_state *old_plane_state = > + struct intel_plane_state *old_plane_state = > intel_atomic_get_old_plane_state(state, plane); > struct drm_i915_gem_object *obj = intel_fb_obj(new_plane_state->hw.fb); > struct drm_i915_gem_object *old_obj = intel_fb_obj(old_plane_state->hw.fb); > @@ -1057,56 +1058,30 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > * This should only fail upon a hung GPU, in which case we > * can safely continue. > */ > - if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > - ret = i915_sw_fence_await_reservation(&state->commit_ready, > - old_obj->base.resv, > - false, 0, > - GFP_KERNEL); > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state) && > + !dma_resv_test_signaled(old_obj->base.resv, > + dma_resv_usage_rw(false))) { > + ret = drm_gem_plane_helper_prepare_fb(_plane, &old_plane_state->uapi); This I think is broken. The old plane state and its fence can still be in use by the previous commit, so we cannot mutate it here. Thus we really need to get the implicit fence from the old fb chained into the new plane state's fence. > if (ret < 0) > return ret; > } > } > > - if (new_plane_state->uapi.fence) { /* explicit fencing */ > - i915_gem_fence_wait_priority(new_plane_state->uapi.fence, > - &attr); > - ret = i915_sw_fence_await_dma_fence(&state->commit_ready, > - new_plane_state->uapi.fence, > - i915_fence_timeout(dev_priv), > - GFP_KERNEL); > - if (ret < 0) > - return ret; > - } > - > if (!obj) > return 0; > > - > ret = intel_plane_pin_fb(new_plane_state); > if (ret) > return ret; > > - i915_gem_object_wait_priority(obj, 0, &attr); > - > - if (!new_plane_state->uapi.fence) { /* implicit fencing */ > - struct dma_resv_iter cursor; > - struct dma_fence *fence; > + ret = drm_gem_plane_helper_prepare_fb(_plane, &new_plane_state->uapi); > + if (ret < 0) > + goto unpin_fb; > > - ret = i915_sw_fence_await_reservation(&state->commit_ready, > - obj->base.resv, false, > - i915_fence_timeout(dev_priv), > - GFP_KERNEL); > - if (ret < 0) > - goto unpin_fb; > + if (new_plane_state->uapi.fence) { > + i915_gem_fence_wait_priority(new_plane_state->uapi.fence, > + &attr); > > - dma_resv_iter_begin(&cursor, obj->base.resv, > - DMA_RESV_USAGE_WRITE); > - dma_resv_for_each_fence_unlocked(&cursor, fence) { > - intel_display_rps_boost_after_vblank(new_plane_state->hw.crtc, > - fence); > - } > - dma_resv_iter_end(&cursor); > - } else { > intel_display_rps_boost_after_vblank(new_plane_state->hw.crtc, > new_plane_state->uapi.fence); > } > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 28d85e1e858e..48df0559df00 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -48,6 +48,7 @@ > #include "g4x_dp.h" > #include "g4x_hdmi.h" > #include "hsw_ips.h" > +#include "i915_config.h" > #include "i915_drv.h" > #include "i915_reg.h" > #include "i915_utils.h" > @@ -7056,29 +7057,34 @@ void intel_atomic_helper_free_state_worker(struct work_struct *work) > > static void intel_atomic_commit_fence_wait(struct intel_atomic_state *intel_state) > { > - struct wait_queue_entry wait_fence, wait_reset; > - struct drm_i915_private *dev_priv = to_i915(intel_state->base.dev); > - > - init_wait_entry(&wait_fence, 0); > - init_wait_entry(&wait_reset, 0); > - for (;;) { > - prepare_to_wait(&intel_state->commit_ready.wait, > - &wait_fence, TASK_UNINTERRUPTIBLE); > - prepare_to_wait(bit_waitqueue(&to_gt(dev_priv)->reset.flags, > - I915_RESET_MODESET), > - &wait_reset, TASK_UNINTERRUPTIBLE); > + struct drm_i915_private *i915 = to_i915(intel_state->base.dev); > + struct drm_plane *plane; > + struct drm_plane_state *new_plane_state, *old_plane_state; > + int ret, i; > > + for_each_oldnew_plane_in_state(&intel_state->base, plane, > + old_plane_state, new_plane_state, i) { > + if (new_plane_state->fence) { > + ret = dma_fence_wait_timeout(new_plane_state->fence, > + false, > + i915_fence_timeout(i915)); > + if (ret <= 0) > + break; > > - if (i915_sw_fence_done(&intel_state->commit_ready) || > - test_bit(I915_RESET_MODESET, &to_gt(dev_priv)->reset.flags)) > - break; > + dma_fence_put(new_plane_state->fence); > + new_plane_state->fence = NULL; > + } > + if (old_plane_state->fence) { > + ret = dma_fence_wait_timeout(old_plane_state->fence, > + false, > + i915_fence_timeout(i915)); > + if (ret <= 0) > + break; > > - schedule(); > + dma_fence_put(old_plane_state->fence); > + old_plane_state->fence = NULL; > + } > } > - finish_wait(&intel_state->commit_ready.wait, &wait_fence); > - finish_wait(bit_waitqueue(&to_gt(dev_priv)->reset.flags, > - I915_RESET_MODESET), > - &wait_reset); > } > > static void intel_atomic_cleanup_work(struct work_struct *work) > @@ -7370,32 +7376,6 @@ static void intel_atomic_commit_work(struct work_struct *work) > intel_atomic_commit_tail(state); > } > > -static int > -intel_atomic_commit_ready(struct i915_sw_fence *fence, > - enum i915_sw_fence_notify notify) > -{ > - struct intel_atomic_state *state = > - container_of(fence, struct intel_atomic_state, commit_ready); > - > - switch (notify) { > - case FENCE_COMPLETE: > - /* we do blocking waits in the worker, nothing to do here */ > - break; > - case FENCE_FREE: > - { > - struct drm_i915_private *i915 = to_i915(state->base.dev); > - struct intel_atomic_helper *helper = > - &i915->display.atomic_helper; > - > - if (llist_add(&state->freed, &helper->free_list)) > - queue_work(i915->unordered_wq, &helper->free_work); > - break; > - } > - } > - > - return NOTIFY_DONE; > -} > - > static void intel_atomic_track_fbs(struct intel_atomic_state *state) > { > struct intel_plane_state *old_plane_state, *new_plane_state; > @@ -7418,10 +7398,6 @@ int intel_atomic_commit(struct drm_device *dev, struct drm_atomic_state *_state, > > state->wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm); > > - drm_atomic_state_get(&state->base); > - i915_sw_fence_init(&state->commit_ready, > - intel_atomic_commit_ready); > - > /* > * The intel_legacy_cursor_update() fast path takes care > * of avoiding the vblank waits for simple cursor > @@ -7454,7 +7430,6 @@ int intel_atomic_commit(struct drm_device *dev, struct drm_atomic_state *_state, > if (ret) { > drm_dbg_atomic(&dev_priv->drm, > "Preparing state failed with %i\n", ret); > - i915_sw_fence_commit(&state->commit_ready); > intel_runtime_pm_put(&dev_priv->runtime_pm, state->wakeref); > return ret; > } > @@ -7470,8 +7445,6 @@ int intel_atomic_commit(struct drm_device *dev, struct drm_atomic_state *_state, > struct intel_crtc *crtc; > int i; > > - i915_sw_fence_commit(&state->commit_ready); > - > for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) > intel_color_cleanup_commit(new_crtc_state); > > @@ -7485,7 +7458,6 @@ int intel_atomic_commit(struct drm_device *dev, struct drm_atomic_state *_state, > drm_atomic_state_get(&state->base); > INIT_WORK(&state->base.commit_work, intel_atomic_commit_work); > > - i915_sw_fence_commit(&state->commit_ready); > if (nonblock && state->modeset) { > queue_work(dev_priv->display.wq.modeset, &state->base.commit_work); > } else if (nonblock) { > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > index 8d8b2f8d37a9..a685c527c7d1 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -670,8 +670,6 @@ struct intel_atomic_state { > > bool rps_interactive; > > - struct i915_sw_fence commit_ready; > - > struct llist_node freed; > }; > > -- > 2.34.1
On 2023-10-18 17:38, Ville Syrjälä wrote: > On Mon, Oct 16, 2023 at 11:08:03AM +0300, Jouni Högander wrote: >> We are preparing for Xe driver. Xe driver doesn't have i915_sw_fence >> implementation. Lets drop i915_sw_fence usage from display code and >> use dma_fence interfaces directly. >> >> For this purpose stack dma fences from related objects into old and new >> plane states using drm_gem_plane_helper_prepare_fb. Then wait for these >> stacked fences during atomic commit. >> >> There is no be need for separate GPU reset handling in >> intel_atomic_commit_fence_wait as the fences are signaled when GPU hang is >> detected and GPU is being reset. >> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: José Roberto de Souza <jose.souza@intel.com> >> >> Signed-off-by: Jouni Högander <jouni.hogander@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_atomic.c | 3 - >> .../gpu/drm/i915/display/intel_atomic_plane.c | 49 +++--------- >> drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++------------- >> .../drm/i915/display/intel_display_types.h | 2 - >> 4 files changed, 37 insertions(+), 95 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c >> index 5d18145da279..ec0d5168b503 100644 >> --- a/drivers/gpu/drm/i915/display/intel_atomic.c >> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c >> @@ -331,9 +331,6 @@ void intel_atomic_state_free(struct drm_atomic_state *_state) >> >> drm_atomic_state_default_release(&state->base); >> kfree(state->global_objs); >> - >> - i915_sw_fence_fini(&state->commit_ready); >> - >> kfree(state); >> } >> >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c >> index b1074350616c..d4f9168ec42c 100644 >> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c >> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c >> @@ -32,6 +32,7 @@ >> */ >> >> #include <drm/drm_atomic_helper.h> >> +#include <drm/drm_gem_atomic_helper.h> >> #include <drm/drm_blend.h> >> #include <drm/drm_fourcc.h> >> >> @@ -1035,7 +1036,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, >> struct intel_atomic_state *state = >> to_intel_atomic_state(new_plane_state->uapi.state); >> struct drm_i915_private *dev_priv = to_i915(plane->base.dev); >> - const struct intel_plane_state *old_plane_state = >> + struct intel_plane_state *old_plane_state = >> intel_atomic_get_old_plane_state(state, plane); >> struct drm_i915_gem_object *obj = intel_fb_obj(new_plane_state->hw.fb); >> struct drm_i915_gem_object *old_obj = intel_fb_obj(old_plane_state->hw.fb); >> @@ -1057,56 +1058,30 @@ intel_prepare_plane_fb(struct drm_plane *_plane, >> * This should only fail upon a hung GPU, in which case we >> * can safely continue. >> */ >> - if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { >> - ret = i915_sw_fence_await_reservation(&state->commit_ready, >> - old_obj->base.resv, >> - false, 0, >> - GFP_KERNEL); >> + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state) && >> + !dma_resv_test_signaled(old_obj->base.resv, >> + dma_resv_usage_rw(false))) { >> + ret = drm_gem_plane_helper_prepare_fb(_plane, &old_plane_state->uapi); > > This I think is broken. The old plane state and its fence can still be > in use by the previous commit, so we cannot mutate it here. Thus we > really need to get the implicit fence from the old fb chained into the > new plane state's fence. Is it even needed though? If new_plane_state always calls prepare_fb. Cheers, ~Maarten
On Wed, Oct 18, 2023 at 07:13:12PM +0200, Maarten Lankhorst wrote: > > > On 2023-10-18 17:38, Ville Syrjälä wrote: > > On Mon, Oct 16, 2023 at 11:08:03AM +0300, Jouni Högander wrote: > >> We are preparing for Xe driver. Xe driver doesn't have i915_sw_fence > >> implementation. Lets drop i915_sw_fence usage from display code and > >> use dma_fence interfaces directly. > >> > >> For this purpose stack dma fences from related objects into old and new > >> plane states using drm_gem_plane_helper_prepare_fb. Then wait for these > >> stacked fences during atomic commit. > >> > >> There is no be need for separate GPU reset handling in > >> intel_atomic_commit_fence_wait as the fences are signaled when GPU hang is > >> detected and GPU is being reset. > >> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> Cc: José Roberto de Souza <jose.souza@intel.com> > >> > >> Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > >> --- > >> drivers/gpu/drm/i915/display/intel_atomic.c | 3 - > >> .../gpu/drm/i915/display/intel_atomic_plane.c | 49 +++--------- > >> drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++------------- > >> .../drm/i915/display/intel_display_types.h | 2 - > >> 4 files changed, 37 insertions(+), 95 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c > >> index 5d18145da279..ec0d5168b503 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_atomic.c > >> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c > >> @@ -331,9 +331,6 @@ void intel_atomic_state_free(struct drm_atomic_state *_state) > >> > >> drm_atomic_state_default_release(&state->base); > >> kfree(state->global_objs); > >> - > >> - i915_sw_fence_fini(&state->commit_ready); > >> - > >> kfree(state); > >> } > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >> index b1074350616c..d4f9168ec42c 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >> @@ -32,6 +32,7 @@ > >> */ > >> > >> #include <drm/drm_atomic_helper.h> > >> +#include <drm/drm_gem_atomic_helper.h> > >> #include <drm/drm_blend.h> > >> #include <drm/drm_fourcc.h> > >> > >> @@ -1035,7 +1036,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > >> struct intel_atomic_state *state = > >> to_intel_atomic_state(new_plane_state->uapi.state); > >> struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > >> - const struct intel_plane_state *old_plane_state = > >> + struct intel_plane_state *old_plane_state = > >> intel_atomic_get_old_plane_state(state, plane); > >> struct drm_i915_gem_object *obj = intel_fb_obj(new_plane_state->hw.fb); > >> struct drm_i915_gem_object *old_obj = intel_fb_obj(old_plane_state->hw.fb); > >> @@ -1057,56 +1058,30 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > >> * This should only fail upon a hung GPU, in which case we > >> * can safely continue. > >> */ > >> - if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > >> - ret = i915_sw_fence_await_reservation(&state->commit_ready, > >> - old_obj->base.resv, > >> - false, 0, > >> - GFP_KERNEL); > >> + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state) && > >> + !dma_resv_test_signaled(old_obj->base.resv, > >> + dma_resv_usage_rw(false))) { > >> + ret = drm_gem_plane_helper_prepare_fb(_plane, &old_plane_state->uapi); > > > > This I think is broken. The old plane state and its fence can still be > > in use by the previous commit, so we cannot mutate it here. Thus we > > really need to get the implicit fence from the old fb chained into the > > new plane state's fence. > Is it even needed though? If new_plane_state always calls prepare_fb. It's explained in the comment.
On Wed, 2023-10-18 at 20:59 +0300, Ville Syrjälä wrote: > On Wed, Oct 18, 2023 at 07:13:12PM +0200, Maarten Lankhorst wrote: > > > > > > On 2023-10-18 17:38, Ville Syrjälä wrote: > > > On Mon, Oct 16, 2023 at 11:08:03AM +0300, Jouni Högander wrote: > > > > We are preparing for Xe driver. Xe driver doesn't have > > > > i915_sw_fence > > > > implementation. Lets drop i915_sw_fence usage from display code > > > > and > > > > use dma_fence interfaces directly. > > > > > > > > For this purpose stack dma fences from related objects into old > > > > and new > > > > plane states using drm_gem_plane_helper_prepare_fb. Then wait > > > > for these > > > > stacked fences during atomic commit. > > > > > > > > There is no be need for separate GPU reset handling in > > > > intel_atomic_commit_fence_wait as the fences are signaled when > > > > GPU hang is > > > > detected and GPU is being reset. > > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_atomic.c | 3 - > > > > .../gpu/drm/i915/display/intel_atomic_plane.c | 49 +++------- > > > > -- > > > > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++---- > > > > --------- > > > > .../drm/i915/display/intel_display_types.h | 2 - > > > > 4 files changed, 37 insertions(+), 95 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c > > > > b/drivers/gpu/drm/i915/display/intel_atomic.c > > > > index 5d18145da279..ec0d5168b503 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c > > > > @@ -331,9 +331,6 @@ void intel_atomic_state_free(struct > > > > drm_atomic_state *_state) > > > > > > > > drm_atomic_state_default_release(&state->base); > > > > kfree(state->global_objs); > > > > - > > > > - i915_sw_fence_fini(&state->commit_ready); > > > > - > > > > kfree(state); > > > > } > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > index b1074350616c..d4f9168ec42c 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > @@ -32,6 +32,7 @@ > > > > */ > > > > > > > > #include <drm/drm_atomic_helper.h> > > > > +#include <drm/drm_gem_atomic_helper.h> > > > > #include <drm/drm_blend.h> > > > > #include <drm/drm_fourcc.h> > > > > > > > > @@ -1035,7 +1036,7 @@ intel_prepare_plane_fb(struct drm_plane > > > > *_plane, > > > > struct intel_atomic_state *state = > > > > to_intel_atomic_state(new_plane_state- > > > > >uapi.state); > > > > struct drm_i915_private *dev_priv = to_i915(plane- > > > > >base.dev); > > > > - const struct intel_plane_state *old_plane_state = > > > > + struct intel_plane_state *old_plane_state = > > > > intel_atomic_get_old_plane_state(state, plane); > > > > struct drm_i915_gem_object *obj = > > > > intel_fb_obj(new_plane_state->hw.fb); > > > > struct drm_i915_gem_object *old_obj = > > > > intel_fb_obj(old_plane_state->hw.fb); > > > > @@ -1057,56 +1058,30 @@ intel_prepare_plane_fb(struct drm_plane > > > > *_plane, > > > > * This should only fail upon a hung GPU, in > > > > which case we > > > > * can safely continue. > > > > */ > > > > - if (new_crtc_state && > > > > intel_crtc_needs_modeset(new_crtc_state)) { > > > > - ret = > > > > i915_sw_fence_await_reservation(&state->commit_ready, > > > > - > > > > old_obj->base.resv, > > > > - > > > > false, 0, > > > > - > > > > GFP_KERNEL); > > > > + if (new_crtc_state && > > > > intel_crtc_needs_modeset(new_crtc_state) && > > > > + !dma_resv_test_signaled(old_obj->base.resv, > > > > + > > > > dma_resv_usage_rw(false))) { > > > > + ret = > > > > drm_gem_plane_helper_prepare_fb(_plane, &old_plane_state- > > > > >uapi); > > > > > > This I think is broken. The old plane state and its fence can > > > still be > > > in use by the previous commit, so we cannot mutate it here. Thus > > > we > > > really need to get the implicit fence from the old fb chained > > > into the > > > new plane state's fence. > > Is it even needed though? If new_plane_state always calls > > prepare_fb. > > It's explained in the comment. Thank you Ville and Maarten for checking my patch. I have addressed Villes comment about old plane state fence. Please check new version. BR, Jouni Högander >
diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c index 5d18145da279..ec0d5168b503 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic.c +++ b/drivers/gpu/drm/i915/display/intel_atomic.c @@ -331,9 +331,6 @@ void intel_atomic_state_free(struct drm_atomic_state *_state) drm_atomic_state_default_release(&state->base); kfree(state->global_objs); - - i915_sw_fence_fini(&state->commit_ready); - kfree(state); } diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c index b1074350616c..d4f9168ec42c 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c @@ -32,6 +32,7 @@ */ #include <drm/drm_atomic_helper.h> +#include <drm/drm_gem_atomic_helper.h> #include <drm/drm_blend.h> #include <drm/drm_fourcc.h> @@ -1035,7 +1036,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, struct intel_atomic_state *state = to_intel_atomic_state(new_plane_state->uapi.state); struct drm_i915_private *dev_priv = to_i915(plane->base.dev); - const struct intel_plane_state *old_plane_state = + struct intel_plane_state *old_plane_state = intel_atomic_get_old_plane_state(state, plane); struct drm_i915_gem_object *obj = intel_fb_obj(new_plane_state->hw.fb); struct drm_i915_gem_object *old_obj = intel_fb_obj(old_plane_state->hw.fb); @@ -1057,56 +1058,30 @@ intel_prepare_plane_fb(struct drm_plane *_plane, * This should only fail upon a hung GPU, in which case we * can safely continue. */ - if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { - ret = i915_sw_fence_await_reservation(&state->commit_ready, - old_obj->base.resv, - false, 0, - GFP_KERNEL); + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state) && + !dma_resv_test_signaled(old_obj->base.resv, + dma_resv_usage_rw(false))) { + ret = drm_gem_plane_helper_prepare_fb(_plane, &old_plane_state->uapi); if (ret < 0) return ret; } } - if (new_plane_state->uapi.fence) { /* explicit fencing */ - i915_gem_fence_wait_priority(new_plane_state->uapi.fence, - &attr); - ret = i915_sw_fence_await_dma_fence(&state->commit_ready, - new_plane_state->uapi.fence, - i915_fence_timeout(dev_priv), - GFP_KERNEL); - if (ret < 0) - return ret; - } - if (!obj) return 0; - ret = intel_plane_pin_fb(new_plane_state); if (ret) return ret; - i915_gem_object_wait_priority(obj, 0, &attr); - - if (!new_plane_state->uapi.fence) { /* implicit fencing */ - struct dma_resv_iter cursor; - struct dma_fence *fence; + ret = drm_gem_plane_helper_prepare_fb(_plane, &new_plane_state->uapi); + if (ret < 0) + goto unpin_fb; - ret = i915_sw_fence_await_reservation(&state->commit_ready, - obj->base.resv, false, - i915_fence_timeout(dev_priv), - GFP_KERNEL); - if (ret < 0) - goto unpin_fb; + if (new_plane_state->uapi.fence) { + i915_gem_fence_wait_priority(new_plane_state->uapi.fence, + &attr); - dma_resv_iter_begin(&cursor, obj->base.resv, - DMA_RESV_USAGE_WRITE); - dma_resv_for_each_fence_unlocked(&cursor, fence) { - intel_display_rps_boost_after_vblank(new_plane_state->hw.crtc, - fence); - } - dma_resv_iter_end(&cursor); - } else { intel_display_rps_boost_after_vblank(new_plane_state->hw.crtc, new_plane_state->uapi.fence); } diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 28d85e1e858e..48df0559df00 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -48,6 +48,7 @@ #include "g4x_dp.h" #include "g4x_hdmi.h" #include "hsw_ips.h" +#include "i915_config.h" #include "i915_drv.h" #include "i915_reg.h" #include "i915_utils.h" @@ -7056,29 +7057,34 @@ void intel_atomic_helper_free_state_worker(struct work_struct *work) static void intel_atomic_commit_fence_wait(struct intel_atomic_state *intel_state) { - struct wait_queue_entry wait_fence, wait_reset; - struct drm_i915_private *dev_priv = to_i915(intel_state->base.dev); - - init_wait_entry(&wait_fence, 0); - init_wait_entry(&wait_reset, 0); - for (;;) { - prepare_to_wait(&intel_state->commit_ready.wait, - &wait_fence, TASK_UNINTERRUPTIBLE); - prepare_to_wait(bit_waitqueue(&to_gt(dev_priv)->reset.flags, - I915_RESET_MODESET), - &wait_reset, TASK_UNINTERRUPTIBLE); + struct drm_i915_private *i915 = to_i915(intel_state->base.dev); + struct drm_plane *plane; + struct drm_plane_state *new_plane_state, *old_plane_state; + int ret, i; + for_each_oldnew_plane_in_state(&intel_state->base, plane, + old_plane_state, new_plane_state, i) { + if (new_plane_state->fence) { + ret = dma_fence_wait_timeout(new_plane_state->fence, + false, + i915_fence_timeout(i915)); + if (ret <= 0) + break; - if (i915_sw_fence_done(&intel_state->commit_ready) || - test_bit(I915_RESET_MODESET, &to_gt(dev_priv)->reset.flags)) - break; + dma_fence_put(new_plane_state->fence); + new_plane_state->fence = NULL; + } + if (old_plane_state->fence) { + ret = dma_fence_wait_timeout(old_plane_state->fence, + false, + i915_fence_timeout(i915)); + if (ret <= 0) + break; - schedule(); + dma_fence_put(old_plane_state->fence); + old_plane_state->fence = NULL; + } } - finish_wait(&intel_state->commit_ready.wait, &wait_fence); - finish_wait(bit_waitqueue(&to_gt(dev_priv)->reset.flags, - I915_RESET_MODESET), - &wait_reset); } static void intel_atomic_cleanup_work(struct work_struct *work) @@ -7370,32 +7376,6 @@ static void intel_atomic_commit_work(struct work_struct *work) intel_atomic_commit_tail(state); } -static int -intel_atomic_commit_ready(struct i915_sw_fence *fence, - enum i915_sw_fence_notify notify) -{ - struct intel_atomic_state *state = - container_of(fence, struct intel_atomic_state, commit_ready); - - switch (notify) { - case FENCE_COMPLETE: - /* we do blocking waits in the worker, nothing to do here */ - break; - case FENCE_FREE: - { - struct drm_i915_private *i915 = to_i915(state->base.dev); - struct intel_atomic_helper *helper = - &i915->display.atomic_helper; - - if (llist_add(&state->freed, &helper->free_list)) - queue_work(i915->unordered_wq, &helper->free_work); - break; - } - } - - return NOTIFY_DONE; -} - static void intel_atomic_track_fbs(struct intel_atomic_state *state) { struct intel_plane_state *old_plane_state, *new_plane_state; @@ -7418,10 +7398,6 @@ int intel_atomic_commit(struct drm_device *dev, struct drm_atomic_state *_state, state->wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm); - drm_atomic_state_get(&state->base); - i915_sw_fence_init(&state->commit_ready, - intel_atomic_commit_ready); - /* * The intel_legacy_cursor_update() fast path takes care * of avoiding the vblank waits for simple cursor @@ -7454,7 +7430,6 @@ int intel_atomic_commit(struct drm_device *dev, struct drm_atomic_state *_state, if (ret) { drm_dbg_atomic(&dev_priv->drm, "Preparing state failed with %i\n", ret); - i915_sw_fence_commit(&state->commit_ready); intel_runtime_pm_put(&dev_priv->runtime_pm, state->wakeref); return ret; } @@ -7470,8 +7445,6 @@ int intel_atomic_commit(struct drm_device *dev, struct drm_atomic_state *_state, struct intel_crtc *crtc; int i; - i915_sw_fence_commit(&state->commit_ready); - for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) intel_color_cleanup_commit(new_crtc_state); @@ -7485,7 +7458,6 @@ int intel_atomic_commit(struct drm_device *dev, struct drm_atomic_state *_state, drm_atomic_state_get(&state->base); INIT_WORK(&state->base.commit_work, intel_atomic_commit_work); - i915_sw_fence_commit(&state->commit_ready); if (nonblock && state->modeset) { queue_work(dev_priv->display.wq.modeset, &state->base.commit_work); } else if (nonblock) { diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index 8d8b2f8d37a9..a685c527c7d1 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -670,8 +670,6 @@ struct intel_atomic_state { bool rps_interactive; - struct i915_sw_fence commit_ready; - struct llist_node freed; };
We are preparing for Xe driver. Xe driver doesn't have i915_sw_fence implementation. Lets drop i915_sw_fence usage from display code and use dma_fence interfaces directly. For this purpose stack dma fences from related objects into old and new plane states using drm_gem_plane_helper_prepare_fb. Then wait for these stacked fences during atomic commit. There is no be need for separate GPU reset handling in intel_atomic_commit_fence_wait as the fences are signaled when GPU hang is detected and GPU is being reset. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: José Roberto de Souza <jose.souza@intel.com> Signed-off-by: Jouni Högander <jouni.hogander@intel.com> --- drivers/gpu/drm/i915/display/intel_atomic.c | 3 - .../gpu/drm/i915/display/intel_atomic_plane.c | 49 +++--------- drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++------------- .../drm/i915/display/intel_display_types.h | 2 - 4 files changed, 37 insertions(+), 95 deletions(-)