Re: [PATCH] drm/i915/display: Use dma_fence interfaces instead of i915_sw_fence

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxxxxxxxx>
Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
Cc: José Roberto de Souza <jose.souza@xxxxxxxxx>

Signed-off-by: Jouni Högander <jouni.hogander@xxxxxxxxx>
---
  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



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux