dma-fence: Deadline awareness

From: Rob Clark <[email protected]>

For an atomic commit updating a single CRTC (ie. a pageflip) calculate the next vblank time, and inform the fence(s) of that deadline.

v2: Comment typo fix (danvet)

Signed-off-by Rob Clark <[email protected]>

Reviewed-by Daniel Vetter <[email protected]>

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/drm_atomic_helper.c | 36 +++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

drivers/gpu/drm/drm_atomic_helper.c +13 -0

index d579fd8f7cb8..d8ee98ce2fc5 100644
@@ -1511,6 +1511,40 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
 
+/*
+ * For atomic updates which touch just a single CRTC, calculate the time of the
+ * next vblank, and inform all the fences of the deadline.
+ */
+static void set_fence_deadline(struct drm_device *dev,
+			       struct drm_atomic_state *state)
+{
+	struct drm_crtc *crtc, *wait_crtc = NULL;
+	struct drm_crtc_state *new_crtc_state;
+	struct drm_plane *plane;
+	struct drm_plane_state *new_plane_state;
+	ktime_t vbltime;
+	int i;

drivers/gpu/drm/drm_atomic_helper.c +13 -0

+
+	if (drm_crtc_next_vblank_start(wait_crtc, &vbltime))
+		return;
+
+	for_each_new_plane_in_state (state, plane, new_plane_state, i) {
+		if (!new_plane_state->fence)
+			continue;
+		dma_fence_set_deadline(new_plane_state->fence, vbltime);
+	}
+}
+
 /**
  * drm_atomic_helper_wait_for_fences - wait for fences stashed in plane state
  * @dev: DRM device

@@ -1540,6 +1574,8 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
 	struct drm_plane_state *new_plane_state;
 	int i, ret;
 
+	set_fence_deadline(dev, state);
+
 	for_each_new_plane_in_state(state, plane, new_plane_state, i) {
 		if (!new_plane_state->fence)
 			continue;

Signature · 1 line

2.39.1

Quoted 35 lines · Rob Clark

From: Rob Clark <[email protected]>

For an atomic commit updating a single CRTC (ie. a pageflip) calculate the next vblank time, and inform the fence(s) of that deadline.

v2: Comment typo fix (danvet)

Signed-off-by Rob Clark <[email protected]>

Reviewed-by Daniel Vetter <[email protected]>

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/drm_atomic_helper.c | 36 +++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

drivers/gpu/drm/drm_atomic_helper.c +13 -0

index d579fd8f7cb8..d8ee98ce2fc5 100644
@@ -1511,6 +1511,40 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
 
+/*
+ * For atomic updates which touch just a single CRTC, calculate the time of the
+ * next vblank, and inform all the fences of the deadline.
+ */
+static void set_fence_deadline(struct drm_device *dev,
+                            struct drm_atomic_state *state)
+{
+     struct drm_crtc *crtc, *wait_crtc = NULL;
+     struct drm_crtc_state *new_crtc_state;
+     struct drm_plane *plane;
+     struct drm_plane_state *new_plane_state;
+     ktime_t vbltime;
+     int i;

drivers/gpu/drm/drm_atomic_helper.c void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, · line 1536

Quoted 10 lines

drivers/gpu/drm/drm_atomic_helper.c +10 -0

+
+     for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) {
+             if (wait_crtc)
+                     return;
+             wait_crtc = crtc;
+     }
+
+     /* If no CRTCs updated, then nothing to do: */
+     if (!wait_crtc)
+             return;

Is there an actual point in limiting this to single crtc updates? That immediately excludes tiled displays/etc.

Handling an arbitrary number of crtcs shouldn't really be a lot more complicated should it?

Quoted 24 lines

drivers/gpu/drm/drm_atomic_helper.c +13 -1

+
+     if (drm_crtc_next_vblank_start(wait_crtc, &vbltime))
+             return;
+
+     for_each_new_plane_in_state (state, plane, new_plane_state, i) {
+             if (!new_plane_state->fence)
+                     continue;
+             dma_fence_set_deadline(new_plane_state->fence, vbltime);
+     }
+}
+
 /**
  * drm_atomic_helper_wait_for_fences - wait for fences stashed in plane state
  * @dev: DRM device

@@ -1540,6 +1574,8 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
      struct drm_plane_state *new_plane_state;
      int i, ret;
 
+     set_fence_deadline(dev, state);
+
      for_each_new_plane_in_state(state, plane, new_plane_state, i) {
              if (!new_plane_state->fence)
                      continue;
--

Quoted 1 line

2.39.1

--
Ville Syrjälä
Intel

Quoted 51 lines · Ville Syrjälä

Quoted 35 lines · Rob Clark

From: Rob Clark <[email protected]>

For an atomic commit updating a single CRTC (ie. a pageflip) calculate the next vblank time, and inform the fence(s) of that deadline.

v2: Comment typo fix (danvet)

Signed-off-by Rob Clark <[email protected]>

Reviewed-by Daniel Vetter <[email protected]>

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/drm_atomic_helper.c | 36 +++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

drivers/gpu/drm/drm_atomic_helper.c +13 -0

index d579fd8f7cb8..d8ee98ce2fc5 100644
@@ -1511,6 +1511,40 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
 
+/*
+ * For atomic updates which touch just a single CRTC, calculate the time of the
+ * next vblank, and inform all the fences of the deadline.
+ */
+static void set_fence_deadline(struct drm_device *dev,
+                            struct drm_atomic_state *state)
+{
+     struct drm_crtc *crtc, *wait_crtc = NULL;
+     struct drm_crtc_state *new_crtc_state;
+     struct drm_plane *plane;
+     struct drm_plane_state *new_plane_state;
+     ktime_t vbltime;
+     int i;

drivers/gpu/drm/drm_atomic_helper.c void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, · line 1536

Quoted 10 lines

drivers/gpu/drm/drm_atomic_helper.c +10 -0

+
+     for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) {
+             if (wait_crtc)
+                     return;
+             wait_crtc = crtc;
+     }
+
+     /* If no CRTCs updated, then nothing to do: */
+     if (!wait_crtc)
+             return;

Is there an actual point in limiting this to single crtc updates? That immediately excludes tiled displays/etc.

Quoted 2 lines

Handling an arbitrary number of crtcs shouldn't really be a lot more complicated should it?

I guess I could find the soonest upcoming vblank of all the CRTCs and use that as the deadline?

From: Rob Clark <[email protected]>

Track the nearest deadline on a fence timeline and set a timer to expire shortly before to trigger boost if the fence has not yet been signaled.

v2: rebase

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/msm_fence.c | 74 +++++++++++++++++++++++++++++++++
  drivers/gpu/drm/msm/msm_fence.h | 20 +++++++++
  2 files changed, 94 insertions(+)

drivers/gpu/drm/msm/msm_fence.c +74 -0

index 56641408ea74..51b461f32103 100644
@@ -8,6 +8,35 @@
 
  #include "msm_drv.h"
  #include "msm_fence.h"
+#include "msm_gpu.h"
+
+static struct msm_gpu *fctx2gpu(struct msm_fence_context *fctx)
+{
+	struct msm_drm_private *priv = fctx->dev->dev_private;
+	return priv->gpu;
+}
+
+static enum hrtimer_restart deadline_timer(struct hrtimer *t)
+{
+	struct msm_fence_context *fctx = container_of(t,
+			struct msm_fence_context, deadline_timer);
+
+	kthread_queue_work(fctx2gpu(fctx)->worker, &fctx->deadline_work);
+
+	return HRTIMER_NORESTART;
+}
+
+static void deadline_work(struct kthread_work *work)
+{
+	struct msm_fence_context *fctx = container_of(work,
+			struct msm_fence_context, deadline_work);
+
+	/* If deadline fence has already passed, nothing to do: */
+	if (msm_fence_completed(fctx, fctx->next_deadline_fence))
+		return;
+
+	msm_devfreq_boost(fctx2gpu(fctx), 2);
+}
 
 
  struct msm_fence_context *

@@ -36,6 +65,13 @@ msm_fence_context_alloc(struct drm_device *dev, volatile uint32_t *fenceptr,
  	fctx->completed_fence = fctx->last_fence;
  	*fctx->fenceptr = fctx->last_fence;
 
+	hrtimer_init(&fctx->deadline_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
+	fctx->deadline_timer.function = deadline_timer;
+
+	kthread_init_work(&fctx->deadline_work, deadline_work);
+
+	fctx->next_deadline = ktime_get();
+
  	return fctx;
  }
 

@@ -62,6 +98,8 @@ void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence)
  	spin_lock_irqsave(&fctx->spinlock, flags);
  	if (fence_after(fence, fctx->completed_fence))
  		fctx->completed_fence = fence;
+	if (msm_fence_completed(fctx, fctx->next_deadline_fence))
+		hrtimer_cancel(&fctx->deadline_timer);
  	spin_unlock_irqrestore(&fctx->spinlock, flags);
  }
 

@@ -92,10 +130,46 @@ static bool msm_fence_signaled(struct dma_fence *fence)
  	return msm_fence_completed(f->fctx, f->base.seqno);
  }
 
+static void msm_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+	struct msm_fence *f = to_msm_fence(fence);
+	struct msm_fence_context *fctx = f->fctx;
+	unsigned long flags;
+	ktime_t now;
+
+	spin_lock_irqsave(&fctx->spinlock, flags);
+	now = ktime_get();
+
+	if (ktime_after(now, fctx->next_deadline) ||
+			ktime_before(deadline, fctx->next_deadline)) {
+		fctx->next_deadline = deadline;
+		fctx->next_deadline_fence =
+			max(fctx->next_deadline_fence, (uint32_t)fence->seqno);
+
+		/*
+		 * Set timer to trigger boost 3ms before deadline, or
+		 * if we are already less than 3ms before the deadline
+		 * schedule boost work immediately.
+		 */
+		deadline = ktime_sub(deadline, ms_to_ktime(3));
+
+		if (ktime_after(now, deadline)) {
+			kthread_queue_work(fctx2gpu(fctx)->worker,
+					&fctx->deadline_work);
+		} else {
+			hrtimer_start(&fctx->deadline_timer, deadline,
+					HRTIMER_MODE_ABS);
+		}
+	}
+
+	spin_unlock_irqrestore(&fctx->spinlock, flags);
+}
+
  static const struct dma_fence_ops msm_fence_ops = {
  	.get_driver_name = msm_fence_get_driver_name,
  	.get_timeline_name = msm_fence_get_timeline_name,
  	.signaled = msm_fence_signaled,
+	.set_deadline = msm_fence_set_deadline,
  };
 
  struct dma_fence *

drivers/gpu/drm/msm/msm_fence.h +20 -0

index 7f1798c54cd1..cdaebfb94f5c 100644
@@ -52,6 +52,26 @@ struct msm_fence_context {
  	volatile uint32_t *fenceptr;
 
  	spinlock_t spinlock;
+
+	/*
+	 * TODO this doesn't really deal with multiple deadlines, like
+	 * if userspace got multiple frames ahead.. OTOH atomic updates
+	 * don't queue, so maybe that is ok
+	 */
+
+	/** next_deadline: Time of next deadline */
+	ktime_t next_deadline;
+
+	/**
+	 * next_deadline_fence:
+	 *
+	 * Fence value for next pending deadline.  The deadline timer is
+	 * canceled when this fence is signaled.
+	 */
+	uint32_t next_deadline_fence;
+
+	struct hrtimer deadline_timer;
+	struct kthread_work deadline_work;
  };
 
  struct msm_fence_context * msm_fence_context_alloc(struct drm_device *dev,

Quoted 5 lines · Rob Clark

From: Rob Clark <[email protected]>

Track the nearest deadline on a fence timeline and set a timer to expire shortly before to trigger boost if the fence has not yet been signaled.

Quoted 7 lines

v2: rebase

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/msm_fence.c | 74 +++++++++++++++++++++++++++++++++
  drivers/gpu/drm/msm/msm_fence.h | 20 +++++++++
  2 files changed, 94 insertions(+)

Reviewed-by Dmitry Baryshkov <[email protected]>

A small question: do we boost to fit into the deadline or to miss the deadline for as little as possible? If the former is the case, we might need to adjust 3ms depending on the workload.

Quoted 132 lines

drivers/gpu/drm/msm/msm_fence.c +74 -0

index 56641408ea74..51b461f32103 100644
@@ -8,6 +8,35 @@
 
  #include "msm_drv.h"
  #include "msm_fence.h"
+#include "msm_gpu.h"
+
+static struct msm_gpu *fctx2gpu(struct msm_fence_context *fctx)
+{
+     struct msm_drm_private *priv = fctx->dev->dev_private;
+     return priv->gpu;
+}
+
+static enum hrtimer_restart deadline_timer(struct hrtimer *t)
+{
+     struct msm_fence_context *fctx = container_of(t,
+                     struct msm_fence_context, deadline_timer);
+
+     kthread_queue_work(fctx2gpu(fctx)->worker, &fctx->deadline_work);
+
+     return HRTIMER_NORESTART;
+}
+
+static void deadline_work(struct kthread_work *work)
+{
+     struct msm_fence_context *fctx = container_of(work,
+                     struct msm_fence_context, deadline_work);
+
+     /* If deadline fence has already passed, nothing to do: */
+     if (msm_fence_completed(fctx, fctx->next_deadline_fence))
+             return;
+
+     msm_devfreq_boost(fctx2gpu(fctx), 2);
+}
 
 
  struct msm_fence_context *

@@ -36,6 +65,13 @@ msm_fence_context_alloc(struct drm_device *dev, volatile uint32_t *fenceptr,
      fctx->completed_fence = fctx->last_fence;
      *fctx->fenceptr = fctx->last_fence;
 
+     hrtimer_init(&fctx->deadline_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
+     fctx->deadline_timer.function = deadline_timer;
+
+     kthread_init_work(&fctx->deadline_work, deadline_work);
+
+     fctx->next_deadline = ktime_get();
+
      return fctx;
  }
 

@@ -62,6 +98,8 @@ void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence)
      spin_lock_irqsave(&fctx->spinlock, flags);
      if (fence_after(fence, fctx->completed_fence))
              fctx->completed_fence = fence;
+     if (msm_fence_completed(fctx, fctx->next_deadline_fence))
+             hrtimer_cancel(&fctx->deadline_timer);
      spin_unlock_irqrestore(&fctx->spinlock, flags);
  }
 

@@ -92,10 +130,46 @@ static bool msm_fence_signaled(struct dma_fence *fence)
      return msm_fence_completed(f->fctx, f->base.seqno);
  }
 
+static void msm_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+     struct msm_fence *f = to_msm_fence(fence);
+     struct msm_fence_context *fctx = f->fctx;
+     unsigned long flags;
+     ktime_t now;
+
+     spin_lock_irqsave(&fctx->spinlock, flags);
+     now = ktime_get();
+
+     if (ktime_after(now, fctx->next_deadline) ||
+                     ktime_before(deadline, fctx->next_deadline)) {
+             fctx->next_deadline = deadline;
+             fctx->next_deadline_fence =
+                     max(fctx->next_deadline_fence, (uint32_t)fence->seqno);
+
+             /*
+              * Set timer to trigger boost 3ms before deadline, or
+              * if we are already less than 3ms before the deadline
+              * schedule boost work immediately.
+              */
+             deadline = ktime_sub(deadline, ms_to_ktime(3));
+
+             if (ktime_after(now, deadline)) {
+                     kthread_queue_work(fctx2gpu(fctx)->worker,
+                                     &fctx->deadline_work);
+             } else {
+                     hrtimer_start(&fctx->deadline_timer, deadline,
+                                     HRTIMER_MODE_ABS);
+             }
+     }
+
+     spin_unlock_irqrestore(&fctx->spinlock, flags);
+}
+
  static const struct dma_fence_ops msm_fence_ops = {
      .get_driver_name = msm_fence_get_driver_name,
      .get_timeline_name = msm_fence_get_timeline_name,
      .signaled = msm_fence_signaled,
+     .set_deadline = msm_fence_set_deadline,
  };
 
  struct dma_fence *

drivers/gpu/drm/msm/msm_fence.h +13 -0

index 7f1798c54cd1..cdaebfb94f5c 100644
@@ -52,6 +52,26 @@ struct msm_fence_context {
      volatile uint32_t *fenceptr;
 
      spinlock_t spinlock;
+
+     /*
+      * TODO this doesn't really deal with multiple deadlines, like
+      * if userspace got multiple frames ahead.. OTOH atomic updates
+      * don't queue, so maybe that is ok
+      */
+
+     /** next_deadline: Time of next deadline */
+     ktime_t next_deadline;
+
+     /**
+      * next_deadline_fence:
+      *

drivers/gpu/drm/msm/msm_fence.h struct msm_fence_context { · line 77

Quoted 10 lines

drivers/gpu/drm/msm/msm_fence.h +7 -0

+      * Fence value for next pending deadline.  The deadline timer is
+      * canceled when this fence is signaled.
+      */
+     uint32_t next_deadline_fence;
+
+     struct hrtimer deadline_timer;
+     struct kthread_work deadline_work;
  };
 
  struct msm_fence_context * msm_fence_context_alloc(struct drm_device *dev,

--
With best wishes
Dmitry

Quoted 17 lines · Dmitry Baryshkov

Quoted 5 lines · Rob Clark

From: Rob Clark <[email protected]>

Track the nearest deadline on a fence timeline and set a timer to expire shortly before to trigger boost if the fence has not yet been signaled.

Quoted 7 lines

v2: rebase

Signed-off-by Rob Clark <[email protected]>

---
   drivers/gpu/drm/msm/msm_fence.c | 74 +++++++++++++++++++++++++++++++++
   drivers/gpu/drm/msm/msm_fence.h | 20 +++++++++
   2 files changed, 94 insertions(+)

Reviewed-by Dmitry Baryshkov <[email protected]>

Quoted 3 lines

A small question: do we boost to fit into the deadline or to miss the deadline for as little as possible? If the former is the case, we might need to adjust 3ms depending on the workload.

The goal is as much to run with higher clock on the next frame as it is to not miss a deadline. Ie. we don't want devfreq to come to the conclusion that running at <50% clks is best due to the amount of utilization caused by missing ever other vblank.

But 3ms is mostly just "seems like a good compromise" value. It might change.

BR,
-R

Quoted 147 lines

Quoted 132 lines

drivers/gpu/drm/msm/msm_fence.c +74 -0

index 56641408ea74..51b461f32103 100644
@@ -8,6 +8,35 @@
 
   #include "msm_drv.h"
   #include "msm_fence.h"
+#include "msm_gpu.h"
+
+static struct msm_gpu *fctx2gpu(struct msm_fence_context *fctx)
+{
+     struct msm_drm_private *priv = fctx->dev->dev_private;
+     return priv->gpu;
+}
+
+static enum hrtimer_restart deadline_timer(struct hrtimer *t)
+{
+     struct msm_fence_context *fctx = container_of(t,
+                     struct msm_fence_context, deadline_timer);
+
+     kthread_queue_work(fctx2gpu(fctx)->worker, &fctx->deadline_work);
+
+     return HRTIMER_NORESTART;
+}
+
+static void deadline_work(struct kthread_work *work)
+{
+     struct msm_fence_context *fctx = container_of(work,
+                     struct msm_fence_context, deadline_work);
+
+     /* If deadline fence has already passed, nothing to do: */
+     if (msm_fence_completed(fctx, fctx->next_deadline_fence))
+             return;
+
+     msm_devfreq_boost(fctx2gpu(fctx), 2);
+}
 
 
   struct msm_fence_context *

@@ -36,6 +65,13 @@ msm_fence_context_alloc(struct drm_device *dev, volatile uint32_t *fenceptr,
       fctx->completed_fence = fctx->last_fence;
       *fctx->fenceptr = fctx->last_fence;
 
+     hrtimer_init(&fctx->deadline_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
+     fctx->deadline_timer.function = deadline_timer;
+
+     kthread_init_work(&fctx->deadline_work, deadline_work);
+
+     fctx->next_deadline = ktime_get();
+
       return fctx;
   }
 

@@ -62,6 +98,8 @@ void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence)
       spin_lock_irqsave(&fctx->spinlock, flags);
       if (fence_after(fence, fctx->completed_fence))
               fctx->completed_fence = fence;
+     if (msm_fence_completed(fctx, fctx->next_deadline_fence))
+             hrtimer_cancel(&fctx->deadline_timer);
       spin_unlock_irqrestore(&fctx->spinlock, flags);
   }
 

@@ -92,10 +130,46 @@ static bool msm_fence_signaled(struct dma_fence *fence)
       return msm_fence_completed(f->fctx, f->base.seqno);
   }
 
+static void msm_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+     struct msm_fence *f = to_msm_fence(fence);
+     struct msm_fence_context *fctx = f->fctx;
+     unsigned long flags;
+     ktime_t now;
+
+     spin_lock_irqsave(&fctx->spinlock, flags);
+     now = ktime_get();
+
+     if (ktime_after(now, fctx->next_deadline) ||
+                     ktime_before(deadline, fctx->next_deadline)) {
+             fctx->next_deadline = deadline;
+             fctx->next_deadline_fence =
+                     max(fctx->next_deadline_fence, (uint32_t)fence->seqno);
+
+             /*
+              * Set timer to trigger boost 3ms before deadline, or
+              * if we are already less than 3ms before the deadline
+              * schedule boost work immediately.
+              */
+             deadline = ktime_sub(deadline, ms_to_ktime(3));
+
+             if (ktime_after(now, deadline)) {
+                     kthread_queue_work(fctx2gpu(fctx)->worker,
+                                     &fctx->deadline_work);
+             } else {
+                     hrtimer_start(&fctx->deadline_timer, deadline,
+                                     HRTIMER_MODE_ABS);
+             }
+     }
+
+     spin_unlock_irqrestore(&fctx->spinlock, flags);
+}
+
   static const struct dma_fence_ops msm_fence_ops = {
       .get_driver_name = msm_fence_get_driver_name,
       .get_timeline_name = msm_fence_get_timeline_name,
       .signaled = msm_fence_signaled,
+     .set_deadline = msm_fence_set_deadline,
   };
 
   struct dma_fence *

drivers/gpu/drm/msm/msm_fence.h +13 -0

index 7f1798c54cd1..cdaebfb94f5c 100644
@@ -52,6 +52,26 @@ struct msm_fence_context {
       volatile uint32_t *fenceptr;
 
       spinlock_t spinlock;
+
+     /*
+      * TODO this doesn't really deal with multiple deadlines, like
+      * if userspace got multiple frames ahead.. OTOH atomic updates
+      * don't queue, so maybe that is ok
+      */
+
+     /** next_deadline: Time of next deadline */
+     ktime_t next_deadline;
+
+     /**
+      * next_deadline_fence:
+      *

drivers/gpu/drm/msm/msm_fence.h struct msm_fence_context { · line 77

Quoted 10 lines

drivers/gpu/drm/msm/msm_fence.h +7 -0

+      * Fence value for next pending deadline.  The deadline timer is
+      * canceled when this fence is signaled.
+      */
+     uint32_t next_deadline_fence;
+
+     struct hrtimer deadline_timer;
+     struct kthread_work deadline_work;
   };
 
   struct msm_fence_context * msm_fence_context_alloc(struct drm_device *dev,

--
With best wishes
Dmitry

From: Rob Clark <[email protected]>

Add a way for various userspace waits to signal urgency.

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/msm_drv.c | 12 ++++++++----
  drivers/gpu/drm/msm/msm_gem.c |  5 +++++
  include/uapi/drm/msm_drm.h    | 14 ++++++++++++--
  3 files changed, 25 insertions(+), 6 deletions(-)

From: Rob Clark <[email protected]>

Drop our custom thing and switch to drm_crtc_next_vblank_start() for calculating the time of the start of the next vblank period.

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 15 ---------------
  drivers/gpu/drm/msm/msm_atomic.c        |  8 +++++---
  drivers/gpu/drm/msm/msm_kms.h           |  8 --------
  3 files changed, 5 insertions(+), 26 deletions(-)

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +0 -15

index a683bd9b5a04..43996aecaf8c 100644
@@ -411,20 +411,6 @@ static void dpu_kms_disable_commit(struct msm_kms *kms)
  	pm_runtime_put_sync(&dpu_kms->pdev->dev);
  }
 
-static ktime_t dpu_kms_vsync_time(struct msm_kms *kms, struct drm_crtc *crtc)
-{
-	struct drm_encoder *encoder;
-
-	drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) {
-		ktime_t vsync_time;
-
-		if (dpu_encoder_vsync_time(encoder, &vsync_time) == 0)
-			return vsync_time;
-	}
-
-	return ktime_get();
-}
-
  static void dpu_kms_prepare_commit(struct msm_kms *kms,
  		struct drm_atomic_state *state)
  {

@@ -953,7 +939,6 @@ static const struct msm_kms_funcs kms_funcs = {
  	.irq             = dpu_core_irq,
  	.enable_commit   = dpu_kms_enable_commit,
  	.disable_commit  = dpu_kms_disable_commit,
-	.vsync_time      = dpu_kms_vsync_time,
  	.prepare_commit  = dpu_kms_prepare_commit,
  	.flush_commit    = dpu_kms_flush_commit,
  	.wait_flush      = dpu_kms_wait_flush,

drivers/gpu/drm/msm/msm_atomic.c +5 -3

index 1686fbb611fd..c5e71c05f038 100644
@@ -186,8 +186,7 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
  	struct msm_kms *kms = priv->kms;
  	struct drm_crtc *async_crtc = NULL;
  	unsigned crtc_mask = get_crtc_mask(state);
-	bool async = kms->funcs->vsync_time &&
-			can_do_async(state, &async_crtc);
+	bool async = can_do_async(state, &async_crtc);
 
  	trace_msm_atomic_commit_tail_start(async, crtc_mask);
 

@@ -231,7 +230,9 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
 
  			kms->pending_crtc_mask |= crtc_mask;
 
-			vsync_time = kms->funcs->vsync_time(kms, async_crtc);
+			if (drm_crtc_next_vblank_start(async_crtc, &vsync_time))
+				goto fallback;
+
  			wakeup_time = ktime_sub(vsync_time, ms_to_ktime(1));
 
  			msm_hrtimer_queue_work(&timer->work, wakeup_time,

@@ -253,6 +254,7 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
  		return;
  	}
 
+fallback:
  	/*
  	 * If there is any async flush pending on updated crtcs, fold
  	 * them into the current flush.

drivers/gpu/drm/msm/msm_kms.h +0 -8

index f8ed7588928c..086a3f1ff956 100644
@@ -59,14 +59,6 @@ struct msm_kms_funcs {
  	void (*enable_commit)(struct msm_kms *kms);
  	void (*disable_commit)(struct msm_kms *kms);
 
-	/**
-	 * If the kms backend supports async commit, it should implement
-	 * this method to return the time of the next vsync.  This is
-	 * used to determine a time slightly before vsync, for the async
-	 * commit timer to run and complete an async commit.
-	 */
-	ktime_t (*vsync_time)(struct msm_kms *kms, struct drm_crtc *crtc);
-
  	/**
  	 * Prepare for atomic commit.  This is called after any previous
  	 * (async or otherwise) commit has completed.

From: Rob Clark <[email protected]>

v2: rebase

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/i915/i915_request.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

drivers/gpu/drm/i915/i915_request.c +0 -0

index 7503dcb9043b..44491e7e214c 100644
@@ -97,6 +97,25 @@ static bool i915_fence_enable_signaling(struct dma_fence *fence)
 	return i915_request_enable_breadcrumb(to_request(fence));
 }

drivers/gpu/drm/i915/i915_request.c +5 -0

+
+	intel_rps_boost(rq);
+}
+
 static signed long i915_fence_wait(struct dma_fence *fence,
 				   bool interruptible,
 				   signed long timeout)

@@ -182,6 +201,7 @@ const struct dma_fence_ops i915_fence_ops = {
 	.signaled = i915_fence_signaled,
 	.wait = i915_fence_wait,
 	.release = i915_fence_release,
+	.set_deadline = i915_fence_set_deadline,
 };
 
 static void irq_execute_cb(struct irq_work *wrk)

Signature · 1 line

2.39.1

Quoted 2 lines · Rob Clark

From: Rob Clark <[email protected]>

missing some wording here...

Quoted 15 lines

v2: rebase

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/i915/i915_request.c | 20 ++++++++++++++++++++
  1 file changed, 20 insertions(+)

drivers/gpu/drm/i915/i915_request.c +0 -0

index 7503dcb9043b..44491e7e214c 100644
@@ -97,6 +97,25 @@ static bool i915_fence_enable_signaling(struct dma_fence *fence)
  	return i915_request_enable_breadcrumb(to_request(fence));
  }
 

drivers/gpu/drm/i915/i915_request.c static bool i915_fence_enable_signaling(struct dma_fence *fence) · line 108

Quoted 9 lines

drivers/gpu/drm/i915/i915_request.c +9 -0

+static void i915_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+	struct i915_request *rq = to_request(fence);
+
+	if (i915_request_completed(rq))
+		return;
+
+	if (i915_request_started(rq))
+		return;

why do we skip the boost if already started?
don't we want to boost the freq anyway?

drivers/gpu/drm/i915/i915_request.c static bool i915_fence_enable_signaling(struct dma_fence *fence) · line 114

Quoted 6 lines

drivers/gpu/drm/i915/i915_request.c +6 -0

+
+	/*
+	 * TODO something more clever for deadlines that are in the
+	 * future.  I think probably track the nearest deadline in
+	 * rq->timeline and set timer to trigger boost accordingly?
+	 */

I'm afraid it will be very hard to find some heuristics of what's late enough for the boost no?
I mean, how early to boost the freq on an upcoming deadline for the timer?

Quoted 19 lines

drivers/gpu/drm/i915/i915_request.c +5 -0

+
+	intel_rps_boost(rq);
+}
+
  static signed long i915_fence_wait(struct dma_fence *fence,
  				   bool interruptible,
  				   signed long timeout)

@@ -182,6 +201,7 @@ const struct dma_fence_ops i915_fence_ops = {
  	.signaled = i915_fence_signaled,
  	.wait = i915_fence_wait,
  	.release = i915_fence_release,
+	.set_deadline = i915_fence_set_deadline,
  };
 
  static void irq_execute_cb(struct irq_work *wrk)

Signature · 1 line

2.39.1

Quoted 24 lines · Rodrigo Vivi

Quoted 2 lines · Rob Clark

From: Rob Clark <[email protected]>

missing some wording here...

Quoted 18 lines

v2: rebase

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/i915/i915_request.c | 20 ++++++++++++++++++++
  1 file changed, 20 insertions(+)

drivers/gpu/drm/i915/i915_request.c +4 -0

index 7503dcb9043b..44491e7e214c 100644
@@ -97,6 +97,25 @@ static bool i915_fence_enable_signaling(struct dma_fence *fence)
  	return i915_request_enable_breadcrumb(to_request(fence));
  }
+static void i915_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+	struct i915_request *rq = to_request(fence);
+

Quoted 8 lines

drivers/gpu/drm/i915/i915_request.c static bool i915_fence_enable_signaling(struct dma_fence *fence) · line 108

Quoted 5 lines

drivers/gpu/drm/i915/i915_request.c +5 -0

+	if (i915_request_completed(rq))
+		return;
+
+	if (i915_request_started(rq))
+		return;

why do we skip the boost if already started?
don't we want to boost the freq anyway?

I'd wager Rob is just copying the current i915 wait boost logic.

Quoted 11 lines

drivers/gpu/drm/i915/i915_request.c static bool i915_fence_enable_signaling(struct dma_fence *fence) · line 114

Quoted 6 lines

drivers/gpu/drm/i915/i915_request.c +6 -0

+
+	/*
+	 * TODO something more clever for deadlines that are in the
+	 * future.  I think probably track the nearest deadline in
+	 * rq->timeline and set timer to trigger boost accordingly?
+	 */

I'm afraid it will be very hard to find some heuristics of what's late enough for the boost no?
I mean, how early to boost the freq on an upcoming deadline for the timer?

We can off load this patch from Rob and deal with it separately, or after the fact?

It's a half solution without a smarter scheduler too. Like https://lore.kernel.org/all/[email protected]/,
or if GuC plans to do something like that at any point.

Or bump the priority too if deadline is looming?

IMO it is not very effective to fiddle with the heuristic on an ad-hoc basis. For instance I have a new heuristics which improves the problematic OpenCL cases for further 5% (relative to the current waitboost improvement from adding missing syncobj waitboost). But I can't really test properly for regressions over platforms, stacks, workloads.. :(

Regards,

Tvrtko

Quoted 18 lines

Quoted 18 lines

drivers/gpu/drm/i915/i915_request.c +5 -0

+
+	intel_rps_boost(rq);
+}
+
  static signed long i915_fence_wait(struct dma_fence *fence,
  				   bool interruptible,
  				   signed long timeout)

@@ -182,6 +201,7 @@ const struct dma_fence_ops i915_fence_ops = {
  	.signaled = i915_fence_signaled,
  	.wait = i915_fence_wait,
  	.release = i915_fence_release,
+	.set_deadline = i915_fence_set_deadline,
  };
  static void irq_execute_cb(struct irq_work *wrk)

Signature · 1 line

2.39.1

Quoted 21 lines · Rodrigo Vivi

Quoted 2 lines · Rob Clark

From: Rob Clark <[email protected]>

missing some wording here...

Quoted 15 lines

v2: rebase

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/i915/i915_request.c | 20 ++++++++++++++++++++
  1 file changed, 20 insertions(+)

drivers/gpu/drm/i915/i915_request.c +0 -0

index 7503dcb9043b..44491e7e214c 100644
@@ -97,6 +97,25 @@ static bool i915_fence_enable_signaling(struct dma_fence *fence)
     return i915_request_enable_breadcrumb(to_request(fence));
  }
 

Quoted 12 lines

drivers/gpu/drm/i915/i915_request.c static bool i915_fence_enable_signaling(struct dma_fence *fence) · line 108

Quoted 9 lines

drivers/gpu/drm/i915/i915_request.c +9 -0

+static void i915_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+    struct i915_request *rq = to_request(fence);
+
+    if (i915_request_completed(rq))
+            return;
+
+    if (i915_request_started(rq))
+            return;

why do we skip the boost if already started?
don't we want to boost the freq anyway?

I'd wager Rob is just copying the current i915 wait boost logic.

Quoted 7 lines

Quoted 6 lines

drivers/gpu/drm/i915/i915_request.c +6 -0

+
+    /*
+     * TODO something more clever for deadlines that are in the
+     * future.  I think probably track the nearest deadline in
+     * rq->timeline and set timer to trigger boost accordingly?
+     */

Quoted 4 lines

I'm afraid it will be very hard to find some heuristics of what's late enough for the boost no?
I mean, how early to boost the freq on an upcoming deadline for the timer?

We can off load this patch from Rob and deal with it separately, or after the fact?

It's a half solution without a smarter scheduler too. Like
https://lore.kernel.org/all/[email protected]/,
or if GuC plans to do something like that at any point.

Or bump the priority too if deadline is looming?

IMO it is not very effective to fiddle with the heuristic on an ad-hoc basis. For instance I have a new heuristics which improves the problematic OpenCL cases for further 5% (relative to the current waitboost improvement from adding missing syncobj waitboost). But I can't really test properly for regressions over platforms, stacks, workloads.. :(

Regards,

Tvrtko

Quoted 19 lines

Quoted 19 lines

drivers/gpu/drm/i915/i915_request.c +5 -1

+
+    intel_rps_boost(rq);
+}
+
  static signed long i915_fence_wait(struct dma_fence *fence,
                                bool interruptible,
                                signed long timeout)

@@ -182,6 +201,7 @@ const struct dma_fence_ops i915_fence_ops = {
     .signaled = i915_fence_signaled,
     .wait = i915_fence_wait,
     .release = i915_fence_release,
+    .set_deadline = i915_fence_set_deadline,
  };
 
  static void irq_execute_cb(struct irq_work *wrk)
--

2.39.1

Quoted 37 lines · Tvrtko Ursulin

Quoted 33 lines · Rodrigo Vivi

Quoted 2 lines · Rob Clark

From: Rob Clark <[email protected]>

missing some wording here...

Quoted 14 lines

v2: rebase

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/i915/i915_request.c | 20 ++++++++++++++++++++
  1 file changed, 20 insertions(+)

drivers/gpu/drm/i915/i915_request.c +0 -0

index 7503dcb9043b..44491e7e214c 100644
@@ -97,6 +97,25 @@ static bool i915_fence_enable_signaling(struct dma_fence *fence)
     return i915_request_enable_breadcrumb(to_request(fence));
  }

drivers/gpu/drm/i915/i915_request.c static bool i915_fence_enable_signaling(struct dma_fence *fence) · line 108

Quoted 10 lines

drivers/gpu/drm/i915/i915_request.c +9 -0

 
+static void i915_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+    struct i915_request *rq = to_request(fence);
+
+    if (i915_request_completed(rq))
+            return;
+
+    if (i915_request_started(rq))
+            return;

why do we skip the boost if already started?
don't we want to boost the freq anyway?

Quoted 1 line

I'd wager Rob is just copying the current i915 wait boost logic.

Yup, and probably incorrectly.. Matt reported fewer boosts/sec compared to your RFC, this could be the bug

Quoted 39 lines · Rob Clark

Quoted 37 lines · Tvrtko Ursulin

Quoted 33 lines · Rodrigo Vivi

Quoted 2 lines · Rob Clark

From: Rob Clark <[email protected]>

missing some wording here...

Quoted 14 lines

v2: rebase

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/i915/i915_request.c | 20 ++++++++++++++++++++
  1 file changed, 20 insertions(+)

drivers/gpu/drm/i915/i915_request.c +0 -0

index 7503dcb9043b..44491e7e214c 100644
@@ -97,6 +97,25 @@ static bool i915_fence_enable_signaling(struct dma_fence *fence)
     return i915_request_enable_breadcrumb(to_request(fence));
  }

drivers/gpu/drm/i915/i915_request.c static bool i915_fence_enable_signaling(struct dma_fence *fence) · line 108

Quoted 10 lines

drivers/gpu/drm/i915/i915_request.c +9 -0

 
+static void i915_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+    struct i915_request *rq = to_request(fence);
+
+    if (i915_request_completed(rq))
+            return;
+
+    if (i915_request_started(rq))
+            return;

why do we skip the boost if already started?
don't we want to boost the freq anyway?

Quoted 4 lines

Quoted 1 line

I'd wager Rob is just copying the current i915 wait boost logic.

Yup, and probably incorrectly.. Matt reported fewer boosts/sec compared to your RFC, this could be the bug

I don't think i915 calls drm_atomic_helper_wait_for_fences() so that could explain something.

Quoted 42 lines · Ville Syrjälä

Quoted 41 lines · Rob Clark

Quoted 38 lines · Tvrtko Ursulin

Quoted 21 lines · Rodrigo Vivi

Quoted 2 lines · Rob Clark

From: Rob Clark <[email protected]>

missing some wording here...

Quoted 15 lines

v2: rebase

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/i915/i915_request.c | 20 ++++++++++++++++++++
  1 file changed, 20 insertions(+)

drivers/gpu/drm/i915/i915_request.c +0 -0

index 7503dcb9043b..44491e7e214c 100644
@@ -97,6 +97,25 @@ static bool i915_fence_enable_signaling(struct dma_fence *fence)
     return i915_request_enable_breadcrumb(to_request(fence));
  }
 

Quoted 12 lines

drivers/gpu/drm/i915/i915_request.c static bool i915_fence_enable_signaling(struct dma_fence *fence) · line 108

Quoted 9 lines

drivers/gpu/drm/i915/i915_request.c +9 -0

+static void i915_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+    struct i915_request *rq = to_request(fence);
+
+    if (i915_request_completed(rq))
+            return;
+
+    if (i915_request_started(rq))
+            return;

why do we skip the boost if already started?
don't we want to boost the freq anyway?

I'd wager Rob is just copying the current i915 wait boost logic.

Quoted 5 lines

Quoted 2 lines

Yup, and probably incorrectly.. Matt reported fewer boosts/sec compared to your RFC, this could be the bug

I don't think i915 calls drm_atomic_helper_wait_for_fences() so that could explain something.

Oh, I guess this wasn't even supposed to take over the current display boost stuff since you didn't remove the old one.

The current one just boosts after a missed vblank. The deadline could use your timer approach I suppose and boost already a bit earlier in the hopes of not missing the vblank.

--
Ville Syrjälä
Intel

Quoted 37 lines · Tvrtko Ursulin

Quoted 33 lines · Rodrigo Vivi

Quoted 2 lines · Rob Clark

From: Rob Clark <[email protected]>

missing some wording here...

Quoted 14 lines

v2: rebase

Signed-off-by Rob Clark <[email protected]>

---
   drivers/gpu/drm/i915/i915_request.c | 20 ++++++++++++++++++++
   1 file changed, 20 insertions(+)

drivers/gpu/drm/i915/i915_request.c +0 -0

index 7503dcb9043b..44491e7e214c 100644
@@ -97,6 +97,25 @@ static bool i915_fence_enable_signaling(struct dma_fence *fence)
      return i915_request_enable_breadcrumb(to_request(fence));
   }

drivers/gpu/drm/i915/i915_request.c static bool i915_fence_enable_signaling(struct dma_fence *fence) · line 108

Quoted 10 lines

drivers/gpu/drm/i915/i915_request.c +9 -0

 
+static void i915_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+    struct i915_request *rq = to_request(fence);
+
+    if (i915_request_completed(rq))
+            return;
+
+    if (i915_request_started(rq))
+            return;

why do we skip the boost if already started?
don't we want to boost the freq anyway?

Quoted 1 line

I'd wager Rob is just copying the current i915 wait boost logic.

Yup, and probably incorrectly.. Matt reported fewer boosts/sec compared to your RFC, this could be the bug

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

drivers/gpu/drm/i915/i915_request.c +6 -0

+
+    /*
+     * TODO something more clever for deadlines that are in the
+     * future.  I think probably track the nearest deadline in
+     * rq->timeline and set timer to trigger boost accordingly?
+     */

Quoted 7 lines

Quoted 4 lines

I'm afraid it will be very hard to find some heuristics of what's late enough for the boost no?
I mean, how early to boost the freq on an upcoming deadline for the timer?

We can off load this patch from Rob and deal with it separately, or after the fact?

That is completely my intention, I expect you to replace my i915 patch ;-)

Rough idea when everyone is happy with the core bits is to setup an immutable branch without the driver specific patches, which could be merged into drm-next and $driver-next and then each driver team can add there own driver patches on top

BR,
-R

Quoted 36 lines

It's a half solution without a smarter scheduler too. Like
https://lore.kernel.org/all/[email protected]/,
or if GuC plans to do something like that at any point.

Or bump the priority too if deadline is looming?

IMO it is not very effective to fiddle with the heuristic on an ad-hoc basis. For instance I have a new heuristics which improves the problematic OpenCL cases for further 5% (relative to the current waitboost improvement from adding missing syncobj waitboost). But I can't really test properly for regressions over platforms, stacks, workloads.. :(

Regards,

Tvrtko

Quoted 19 lines

Quoted 19 lines

drivers/gpu/drm/i915/i915_request.c +5 -1

+
+    intel_rps_boost(rq);
+}
+
   static signed long i915_fence_wait(struct dma_fence *fence,
                                 bool interruptible,
                                 signed long timeout)

@@ -182,6 +201,7 @@ const struct dma_fence_ops i915_fence_ops = {
      .signaled = i915_fence_signaled,
      .wait = i915_fence_wait,
      .release = i915_fence_release,
+    .set_deadline = i915_fence_set_deadline,
   };
 
   static void irq_execute_cb(struct irq_work *wrk)
--

2.39.1

Quoted 41 lines · Rob Clark

Quoted 38 lines · Tvrtko Ursulin

Quoted 21 lines · Rodrigo Vivi

Quoted 2 lines · Rob Clark

From: Rob Clark <[email protected]>

missing some wording here...

Quoted 15 lines

v2: rebase

Signed-off-by Rob Clark <[email protected]>

---
   drivers/gpu/drm/i915/i915_request.c | 20 ++++++++++++++++++++
   1 file changed, 20 insertions(+)

drivers/gpu/drm/i915/i915_request.c +0 -0

index 7503dcb9043b..44491e7e214c 100644
@@ -97,6 +97,25 @@ static bool i915_fence_enable_signaling(struct dma_fence *fence)
      return i915_request_enable_breadcrumb(to_request(fence));
   }
 

Quoted 12 lines

drivers/gpu/drm/i915/i915_request.c static bool i915_fence_enable_signaling(struct dma_fence *fence) · line 108

Quoted 9 lines

drivers/gpu/drm/i915/i915_request.c +9 -0

+static void i915_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+    struct i915_request *rq = to_request(fence);
+
+    if (i915_request_completed(rq))
+            return;
+
+    if (i915_request_started(rq))
+            return;

why do we skip the boost if already started?
don't we want to boost the freq anyway?

I'd wager Rob is just copying the current i915 wait boost logic.

Quoted 2 lines

Yup, and probably incorrectly.. Matt reported fewer boosts/sec compared to your RFC, this could be the bug

Hm, there I have preserved this same !i915_request_started logic.

Presumably it's not just fewer boosts but lower performance. How is he setting the deadline? Somehow from clFlush or so?

Regards,

Tvrtko

P.S. Take note that I did not post the latest version of my RFC. The one where I fix the fence chain and array misses you pointed out. I did not think it would be worthwhile given no universal love for it, but if people are testing with it more widely that I was aware perhaps I should.

Quoted 61 lines

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

drivers/gpu/drm/i915/i915_request.c +6 -0

+
+    /*
+     * TODO something more clever for deadlines that are in the
+     * future.  I think probably track the nearest deadline in
+     * rq->timeline and set timer to trigger boost accordingly?
+     */

Quoted 7 lines

Quoted 4 lines

I'm afraid it will be very hard to find some heuristics of what's late enough for the boost no?
I mean, how early to boost the freq on an upcoming deadline for the timer?

We can off load this patch from Rob and deal with it separately, or after the fact?

That is completely my intention, I expect you to replace my i915 patch ;-)

Rough idea when everyone is happy with the core bits is to setup an immutable branch without the driver specific patches, which could be merged into drm-next and $driver-next and then each driver team can add there own driver patches on top

BR,
-R

Quoted 36 lines

It's a half solution without a smarter scheduler too. Like
https://lore.kernel.org/all/[email protected]/,
or if GuC plans to do something like that at any point.

Or bump the priority too if deadline is looming?

IMO it is not very effective to fiddle with the heuristic on an ad-hoc basis. For instance I have a new heuristics which improves the problematic OpenCL cases for further 5% (relative to the current waitboost improvement from adding missing syncobj waitboost). But I can't really test properly for regressions over platforms, stacks, workloads.. :(

Regards,

Tvrtko

Quoted 19 lines

Quoted 19 lines

drivers/gpu/drm/i915/i915_request.c +5 -1

+
+    intel_rps_boost(rq);
+}
+
   static signed long i915_fence_wait(struct dma_fence *fence,
                                 bool interruptible,
                                 signed long timeout)

@@ -182,6 +201,7 @@ const struct dma_fence_ops i915_fence_ops = {
      .signaled = i915_fence_signaled,
      .wait = i915_fence_wait,
      .release = i915_fence_release,
+    .set_deadline = i915_fence_set_deadline,
   };
 
   static void irq_execute_cb(struct irq_work *wrk)
--

2.39.1

Quoted 39 lines · Rob Clark

Quoted 37 lines · Tvrtko Ursulin

Quoted 33 lines · Rodrigo Vivi

Quoted 2 lines · Rob Clark

From: Rob Clark <[email protected]>

missing some wording here...

Quoted 14 lines

v2: rebase

Signed-off-by Rob Clark <[email protected]>

---
   drivers/gpu/drm/i915/i915_request.c | 20 ++++++++++++++++++++
   1 file changed, 20 insertions(+)

drivers/gpu/drm/i915/i915_request.c +0 -0

index 7503dcb9043b..44491e7e214c 100644
@@ -97,6 +97,25 @@ static bool i915_fence_enable_signaling(struct dma_fence *fence)
      return i915_request_enable_breadcrumb(to_request(fence));
   }

drivers/gpu/drm/i915/i915_request.c static bool i915_fence_enable_signaling(struct dma_fence *fence) · line 108

Quoted 10 lines

drivers/gpu/drm/i915/i915_request.c +9 -0

 
+static void i915_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+    struct i915_request *rq = to_request(fence);
+
+    if (i915_request_completed(rq))
+            return;
+
+    if (i915_request_started(rq))
+            return;

why do we skip the boost if already started?
don't we want to boost the freq anyway?

Quoted 4 lines

Quoted 1 line

I'd wager Rob is just copying the current i915 wait boost logic.

Yup, and probably incorrectly.. Matt reported fewer boosts/sec compared to your RFC, this could be the bug

Hm, there I have preserved this same !i915_request_started logic.

Presumably it's not just fewer boosts but lower performance. How is he setting the deadline? Somehow from clFlush or so?

Regards,

Tvrtko

P.S. Take note that I did not post the latest version of my RFC. The one where I fix the fence chain and array misses you pointed out. I did not think it would be worthwhile given no universal love for it, but if people are testing with it more widely that I was aware perhaps I should.

Quoted 2 lines · Rob Clark

From: Rob Clark <[email protected]>

missing some wording here...

Quoted 15 lines

v2: rebase

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/i915/i915_request.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

drivers/gpu/drm/i915/i915_request.c +0 -0

index 7503dcb9043b..44491e7e214c 100644
@@ -97,6 +97,25 @@ static bool i915_fence_enable_signaling(struct dma_fence *fence)
      return i915_request_enable_breadcrumb(to_request(fence));
 }

drivers/gpu/drm/i915/i915_request.c static bool i915_fence_enable_signaling(struct dma_fence *fence) · line 108

Quoted 10 lines

drivers/gpu/drm/i915/i915_request.c +9 -0

 
+static void i915_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+     struct i915_request *rq = to_request(fence);
+
+     if (i915_request_completed(rq))
+             return;
+
+     if (i915_request_started(rq))
+             return;

why do we skip the boost if already started?
don't we want to boost the freq anyway?

drivers/gpu/drm/i915/i915_request.c static bool i915_fence_enable_signaling(struct dma_fence *fence) · line 114

Quoted 6 lines

drivers/gpu/drm/i915/i915_request.c +6 -0

+
+     /*
+      * TODO something more clever for deadlines that are in the
+      * future.  I think probably track the nearest deadline in
+      * rq->timeline and set timer to trigger boost accordingly?
+      */

I'm afraid it will be very hard to find some heuristics of what's late enough for the boost no?
I mean, how early to boost the freq on an upcoming deadline for the timer?

Quoted 18 lines

drivers/gpu/drm/i915/i915_request.c +5 -1

+
+     intel_rps_boost(rq);
+}
+
 static signed long i915_fence_wait(struct dma_fence *fence,
                                 bool interruptible,
                                 signed long timeout)

@@ -182,6 +201,7 @@ const struct dma_fence_ops i915_fence_ops = {
      .signaled = i915_fence_signaled,
      .wait = i915_fence_wait,
      .release = i915_fence_release,
+     .set_deadline = i915_fence_set_deadline,
 };
 
 static void irq_execute_cb(struct irq_work *wrk)
--

2.39.1