dma-fence: Deadline awareness

From: Rob Clark <[email protected]>

Add a way to hint to the fence signaler of an upcoming deadline, such as vblank, which the fence waiter would prefer not to miss. This is to aid the fence signaler in making power management decisions, like boosting frequency as the deadline approaches and awareness of missing deadlines so that can be factored in to the frequency scaling.

v2: Drop dma_fence::deadline and related logic to filter duplicate
deadlines, to avoid increasing dma_fence size. The fence-context implementation will need similar logic to track deadlines of all the fences on the same timeline. [ckoenig]
v3: Clarify locking wrt. set_deadline callback

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

Reviewed-by Christian König <[email protected]>

---
  drivers/dma-buf/dma-fence.c | 20 ++++++++++++++++++++
  include/linux/dma-fence.h   | 20 ++++++++++++++++++++
  2 files changed, 40 insertions(+)

drivers/dma-buf/dma-fence.c +20 -0

index 0de0482cd36e..763b32627684 100644
@@ -912,6 +912,26 @@ dma_fence_wait_any_timeout(struct dma_fence **fences, uint32_t count,
  }
  EXPORT_SYMBOL(dma_fence_wait_any_timeout);
 
+
+/**
+ * dma_fence_set_deadline - set desired fence-wait deadline
+ * @fence:    the fence that is to be waited on
+ * @deadline: the time by which the waiter hopes for the fence to be
+ *            signaled
+ *
+ * Inform the fence signaler of an upcoming deadline, such as vblank, by
+ * which point the waiter would prefer the fence to be signaled by.  This
+ * is intended to give feedback to the fence signaler to aid in power
+ * management decisions, such as boosting GPU frequency if a periodic
+ * vblank deadline is approaching.
+ */
+void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+	if (fence->ops->set_deadline && !dma_fence_is_signaled(fence))
+		fence->ops->set_deadline(fence, deadline);
+}
+EXPORT_SYMBOL(dma_fence_set_deadline);
+
  /**
   * dma_fence_describe - Dump fence describtion into seq_file
   * @fence: the 6fence to describe

include/linux/dma-fence.h +0 -0

index 775cdc0b4f24..d77f6591c453 100644

include/linux/dma-fence.h +19 -0

  	DMA_FENCE_FLAG_USER_BITS, /* must always be last member */
  };
 

@@ -257,6 +258,23 @@ struct dma_fence_ops {
  	 */
  	void (*timeline_value_str)(struct dma_fence *fence,
  				   char *str, int size);
+
+	/**
+	 * @set_deadline:
+	 *
+	 * Callback to allow a fence waiter to inform the fence signaler of
+	 * an upcoming deadline, such as vblank, by which point the waiter
+	 * would prefer the fence to be signaled by.  This is intended to
+	 * give feedback to the fence signaler to aid in power management
+	 * decisions, such as boosting GPU frequency.
+	 *
+	 * This is called without &dma_fence.lock held, it can be called
+	 * multiple times and from any context.  Locking is up to the callee
+	 * if it has some state to manage.
+	 *
+	 * This callback is optional.
+	 */
+	void (*set_deadline)(struct dma_fence *fence, ktime_t deadline);
  };
 
  void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,

@@ -583,6 +601,8 @@ static inline signed long dma_fence_wait(struct dma_fence *fence, bool intr)
  	return ret < 0 ? ret : 0;
  }
 
+void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline);
+
  struct dma_fence *dma_fence_get_stub(void);
  struct dma_fence *dma_fence_allocate_private_stub(void);
  u64 dma_fence_context_alloc(unsigned num);

Quoted 59 lines

From: Rob Clark <[email protected]>

Add a way to hint to the fence signaler of an upcoming deadline, such as vblank, which the fence waiter would prefer not to miss.  This is to aid the fence signaler in making power management decisions, like boosting frequency as the deadline approaches and awareness of missing deadlines so that can be factored in to the frequency scaling.

v2: Drop dma_fence::deadline and related logic to filter duplicate deadlines, to avoid increasing dma_fence size.  The fence-context implementation will need similar logic to track deadlines of all the fences on the same timeline.  [ckoenig]
v3: Clarify locking wrt. set_deadline callback

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

Reviewed-by Christian König <[email protected]>

---
  drivers/dma-buf/dma-fence.c | 20 ++++++++++++++++++++ include/linux/dma-fence.h   | 20 ++++++++++++++++++++

  2 files changed, 40 insertions(+)

drivers/dma-buf/dma-fence.c +0 -0

index 0de0482cd36e..763b32627684 100644
@@ -912,6 +912,26 @@ dma_fence_wait_any_timeout(struct dma_fence 
 **fences, uint32_t count,
   }
   EXPORT_SYMBOL(dma_fence_wait_any_timeout);

  +
+/**
+ * dma_fence_set_deadline - set desired fence-wait deadline
+ * @fence:    the fence that is to be waited on
+ * @deadline: the time by which the waiter hopes for the fence to be
+ *            signaled
+ *
+ * Inform the fence signaler of an upcoming deadline, such as vblank, by
+ * which point the waiter would prefer the fence to be signaled by. This
+ * is intended to give feedback to the fence signaler to aid in power
+ * management decisions, such as boosting GPU frequency if a periodic
+ * vblank deadline is approaching.
+ */

drivers/dma-buf/dma-fence.c +7 -0

+void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+    if (fence->ops->set_deadline && !dma_fence_is_signaled(fence))
+        fence->ops->set_deadline(fence, deadline);
+}
+EXPORT_SYMBOL(dma_fence_set_deadline);
+

  /**
   * dma_fence_describe - Dump fence describtion into seq_file
   * @fence: the 6fence to describe

include/linux/dma-fence.h +0 -0

index 775cdc0b4f24..d77f6591c453 100644

include/linux/dma-fence.h enum dma_fence_flag_bits { · line 102

Quoted 4 lines

include/linux/dma-fence.h +1 -0

@@ -99,6 +99,7 @@ enum dma_fence_flag_bits {
       DMA_FENCE_FLAG_SIGNALED_BIT,
       DMA_FENCE_FLAG_TIMESTAMP_BIT,
       DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
+    DMA_FENCE_FLAG_HAS_DEADLINE_BIT,

Would this bit be better left out from core implementation, given how the approach is the component which implements dma-fence has to track the actual deadline and all?

Also taking a step back - are we all okay with starting to expand the relatively simple core synchronisation primitive with side channel data like this? What would be the criteria for what side channel data would be acceptable? Taking note the thing lives outside drivers/gpu/.

Regards,

Tvrtko

Quoted 35 lines

      DMA_FENCE_FLAG_USER_BITS, /* must always be last member */
  };
  @@ -257,6 +258,23 @@ struct dma_fence_ops {
       */
      void (*timeline_value_str)(struct dma_fence *fence,
                     char *str, int size);
+
+    /**
+     * @set_deadline:
+     *
+     * Callback to allow a fence waiter to inform the fence signaler of +     * an upcoming deadline, such as vblank, by which point the waiter +     * would prefer the fence to be signaled by.  This is intended to +     * give feedback to the fence signaler to aid in power management +     * decisions, such as boosting GPU frequency.
+     *
+     * This is called without &dma_fence.lock held, it can be called +     * multiple times and from any context.  Locking is up to the callee
+     * if it has some state to manage.
+     *
+     * This callback is optional.
+     */
+    void (*set_deadline)(struct dma_fence *fence, ktime_t deadline); };
    void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,

include/linux/dma-fence.h +1 -0

@@ -583,6 +601,8 @@ static inline signed long dma_fence_wait(struct 
 dma_fence *fence, bool intr)
       return ret < 0 ? ret : 0;
   }
   +void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline);
+
   struct dma_fence *dma_fence_get_stub(void);
   struct dma_fence *dma_fence_allocate_private_stub(void);

  u64 dma_fence_context_alloc(unsigned num);

Am 22.02.23 um 11:23 schrieb Tvrtko Ursulin:

Quoted 68 lines · Rob Clark

Quoted 59 lines

From: Rob Clark <[email protected]>

Add a way to hint to the fence signaler of an upcoming deadline, such as vblank, which the fence waiter would prefer not to miss.  This is to aid the fence signaler in making power management decisions, like boosting frequency as the deadline approaches and awareness of missing deadlines so that can be factored in to the frequency scaling.

v2: Drop dma_fence::deadline and related logic to filter duplicate deadlines, to avoid increasing dma_fence size.  The fence-context implementation will need similar logic to track deadlines of all the fences on the same timeline.  [ckoenig]
v3: Clarify locking wrt. set_deadline callback

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

Reviewed-by Christian König <[email protected]>

---
  drivers/dma-buf/dma-fence.c | 20 ++++++++++++++++++++ include/linux/dma-fence.h   | 20 ++++++++++++++++++++

  2 files changed, 40 insertions(+)

drivers/dma-buf/dma-fence.c +0 -0

index 0de0482cd36e..763b32627684 100644
@@ -912,6 +912,26 @@ dma_fence_wait_any_timeout(struct dma_fence 
 **fences, uint32_t count,
   }
   EXPORT_SYMBOL(dma_fence_wait_any_timeout);

  +
+/**
+ * dma_fence_set_deadline - set desired fence-wait deadline
+ * @fence:    the fence that is to be waited on
+ * @deadline: the time by which the waiter hopes for the fence to be
+ *            signaled
+ *
+ * Inform the fence signaler of an upcoming deadline, such as vblank, by
+ * which point the waiter would prefer the fence to be signaled by. This
+ * is intended to give feedback to the fence signaler to aid in power
+ * management decisions, such as boosting GPU frequency if a periodic
+ * vblank deadline is approaching.
+ */

drivers/dma-buf/dma-fence.c +7 -0

+void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+    if (fence->ops->set_deadline && !dma_fence_is_signaled(fence))
+        fence->ops->set_deadline(fence, deadline);
+}
+EXPORT_SYMBOL(dma_fence_set_deadline);
+

  /**
   * dma_fence_describe - Dump fence describtion into seq_file
   * @fence: the 6fence to describe

include/linux/dma-fence.h +0 -0

index 775cdc0b4f24..d77f6591c453 100644

include/linux/dma-fence.h enum dma_fence_flag_bits { · line 102

Quoted 4 lines

include/linux/dma-fence.h +1 -0

@@ -99,6 +99,7 @@ enum dma_fence_flag_bits {
       DMA_FENCE_FLAG_SIGNALED_BIT,
       DMA_FENCE_FLAG_TIMESTAMP_BIT,
       DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
+    DMA_FENCE_FLAG_HAS_DEADLINE_BIT,

Would this bit be better left out from core implementation, given how the approach is the component which implements dma-fence has to track the actual deadline and all?

Quoted 1 line

Also taking a step back - are we all okay with starting to expand the relatively simple core synchronisation primitive with side channel data like this? What would be the criteria for what side channel data would be acceptable? Taking note the thing lives outside drivers/gpu/.

I had similar concerns and it took me a moment as well to understand the background why this is necessary. I essentially don't see much other approach we could do.

Yes, this is GPU/CRTC specific, but we somehow need a common interface for communicating it between drivers and that's the dma_fence object as far as I can see.

Quoted 76 lines · Christian König

Am 22.02.23 um 11:23 schrieb Tvrtko Ursulin:

Quoted 70 lines · Rob Clark

Quoted 59 lines

From: Rob Clark <[email protected]>

Add a way to hint to the fence signaler of an upcoming deadline, such as vblank, which the fence waiter would prefer not to miss. This is to aid the fence signaler in making power management decisions, like boosting frequency as the deadline approaches and awareness of missing deadlines so that can be factored in to the frequency scaling.

v2: Drop dma_fence::deadline and related logic to filter duplicate
deadlines, to avoid increasing dma_fence size. The fence-context implementation will need similar logic to track deadlines of all the fences on the same timeline. [ckoenig]
v3: Clarify locking wrt. set_deadline callback

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

Reviewed-by Christian König <[email protected]>

---
  drivers/dma-buf/dma-fence.c | 20 ++++++++++++++++++++
  include/linux/dma-fence.h   | 20 ++++++++++++++++++++
  2 files changed, 40 insertions(+)

drivers/dma-buf/dma-fence.c +7 -0

index 0de0482cd36e..763b32627684 100644
@@ -912,6 +912,26 @@ dma_fence_wait_any_timeout(struct dma_fence
 **fences, uint32_t count,
  }
  EXPORT_SYMBOL(dma_fence_wait_any_timeout);
  +
+/**
+ * dma_fence_set_deadline - set desired fence-wait deadline
+ * @fence:    the fence that is to be waited on
+ * @deadline: the time by which the waiter hopes for the fence to be
+ *            signaled
+ *
+ * Inform the fence signaler of an upcoming deadline, such as

vblank, by
+ * which point the waiter would prefer the fence to be signaled by. This
+ * is intended to give feedback to the fence signaler to aid in power
+ * management decisions, such as boosting GPU frequency if a periodic
+ * vblank deadline is approaching.
+ */

drivers/dma-buf/dma-fence.c +7 -0

+void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+    if (fence->ops->set_deadline && !dma_fence_is_signaled(fence))
+        fence->ops->set_deadline(fence, deadline);
+}
+EXPORT_SYMBOL(dma_fence_set_deadline);
+
  /**
   * dma_fence_describe - Dump fence describtion into seq_file
   * @fence: the 6fence to describe

include/linux/dma-fence.h +0 -0

index 775cdc0b4f24..d77f6591c453 100644

include/linux/dma-fence.h enum dma_fence_flag_bits { · line 102

Quoted 4 lines

include/linux/dma-fence.h +1 -0

@@ -99,6 +99,7 @@ enum dma_fence_flag_bits {
      DMA_FENCE_FLAG_SIGNALED_BIT,
      DMA_FENCE_FLAG_TIMESTAMP_BIT,
      DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
+    DMA_FENCE_FLAG_HAS_DEADLINE_BIT,

Would this bit be better left out from core implementation, given how the approach is the component which implements dma-fence has to track the actual deadline and all?

Quoted 4 lines

Also taking a step back - are we all okay with starting to expand the relatively simple core synchronisation primitive with side channel data like this? What would be the criteria for what side channel data would be acceptable? Taking note the thing lives outside drivers/gpu/.

Quoted 7 lines

I had similar concerns and it took me a moment as well to understand the background why this is necessary. I essentially don't see much other approach we could do.

Yes, this is GPU/CRTC specific, but we somehow need a common interface for communicating it between drivers and that's the dma_fence object as far as I can see.

Yeah I also don't see any other easy options. Just wanted to raise this as something which probably needs some wider acks.

Also what about the "low level" part of my question about the reason, or benefits, of defining the deadline bit in the common layer?

Regards,

Tvrtko

Quoted 90 lines · Tvrtko Ursulin

Quoted 76 lines · Christian König

Am 22.02.23 um 11:23 schrieb Tvrtko Ursulin:

Quoted 70 lines · Rob Clark

Quoted 59 lines

From: Rob Clark <[email protected]>

Add a way to hint to the fence signaler of an upcoming deadline, such as vblank, which the fence waiter would prefer not to miss. This is to aid the fence signaler in making power management decisions, like boosting frequency as the deadline approaches and awareness of missing deadlines so that can be factored in to the frequency scaling.

v2: Drop dma_fence::deadline and related logic to filter duplicate
deadlines, to avoid increasing dma_fence size. The fence-context implementation will need similar logic to track deadlines of all the fences on the same timeline. [ckoenig]
v3: Clarify locking wrt. set_deadline callback

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

Reviewed-by Christian König <[email protected]>

---
   drivers/dma-buf/dma-fence.c | 20 ++++++++++++++++++++
   include/linux/dma-fence.h   | 20 ++++++++++++++++++++
   2 files changed, 40 insertions(+)

drivers/dma-buf/dma-fence.c +7 -0

index 0de0482cd36e..763b32627684 100644
@@ -912,6 +912,26 @@ dma_fence_wait_any_timeout(struct dma_fence
 **fences, uint32_t count,
   }
   EXPORT_SYMBOL(dma_fence_wait_any_timeout);
   +
+/**
+ * dma_fence_set_deadline - set desired fence-wait deadline
+ * @fence:    the fence that is to be waited on
+ * @deadline: the time by which the waiter hopes for the fence to be
+ *            signaled
+ *
+ * Inform the fence signaler of an upcoming deadline, such as

vblank, by
+ * which point the waiter would prefer the fence to be signaled by. This
+ * is intended to give feedback to the fence signaler to aid in power
+ * management decisions, such as boosting GPU frequency if a periodic
+ * vblank deadline is approaching.
+ */

drivers/dma-buf/dma-fence.c +7 -0

+void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+    if (fence->ops->set_deadline && !dma_fence_is_signaled(fence))
+        fence->ops->set_deadline(fence, deadline);
+}
+EXPORT_SYMBOL(dma_fence_set_deadline);
+
   /**
    * dma_fence_describe - Dump fence describtion into seq_file
    * @fence: the 6fence to describe

include/linux/dma-fence.h +0 -0

index 775cdc0b4f24..d77f6591c453 100644

include/linux/dma-fence.h enum dma_fence_flag_bits { · line 102

Quoted 4 lines

include/linux/dma-fence.h +1 -0

@@ -99,6 +99,7 @@ enum dma_fence_flag_bits {
       DMA_FENCE_FLAG_SIGNALED_BIT,
       DMA_FENCE_FLAG_TIMESTAMP_BIT,
       DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
+    DMA_FENCE_FLAG_HAS_DEADLINE_BIT,

Would this bit be better left out from core implementation, given how the approach is the component which implements dma-fence has to track the actual deadline and all?

Quoted 4 lines

Also taking a step back - are we all okay with starting to expand the relatively simple core synchronisation primitive with side channel data like this? What would be the criteria for what side channel data would be acceptable? Taking note the thing lives outside drivers/gpu/.

Quoted 7 lines

I had similar concerns and it took me a moment as well to understand the background why this is necessary. I essentially don't see much other approach we could do.

Yes, this is GPU/CRTC specific, but we somehow need a common interface for communicating it between drivers and that's the dma_fence object as far as I can see.

Yeah I also don't see any other easy options. Just wanted to raise this as something which probably needs some wider acks.

Quoted 2 lines

Also what about the "low level" part of my question about the reason, or benefits, of defining the deadline bit in the common layer?

We could leave DMA_FENCE_FLAG_HAS_DEADLINE_BIT out, but OTOH managing a bitmask that is partially defined in core enum and partially in backend-driver has it's own drawbacks, and it isn't like we are running out of bits.. :shrug:

Quoted 92 lines · Rob Clark

Quoted 90 lines · Tvrtko Ursulin

Quoted 76 lines · Christian König

Am 22.02.23 um 11:23 schrieb Tvrtko Ursulin:

Quoted 70 lines · Rob Clark

Quoted 59 lines

From: Rob Clark <[email protected]>

Add a way to hint to the fence signaler of an upcoming deadline, such as vblank, which the fence waiter would prefer not to miss. This is to aid the fence signaler in making power management decisions, like boosting frequency as the deadline approaches and awareness of missing deadlines so that can be factored in to the frequency scaling.

v2: Drop dma_fence::deadline and related logic to filter duplicate
deadlines, to avoid increasing dma_fence size. The fence-context implementation will need similar logic to track deadlines of all the fences on the same timeline. [ckoenig]
v3: Clarify locking wrt. set_deadline callback

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

Reviewed-by Christian König <[email protected]>

---
   drivers/dma-buf/dma-fence.c | 20 ++++++++++++++++++++
   include/linux/dma-fence.h   | 20 ++++++++++++++++++++
   2 files changed, 40 insertions(+)

drivers/dma-buf/dma-fence.c +7 -0

index 0de0482cd36e..763b32627684 100644
@@ -912,6 +912,26 @@ dma_fence_wait_any_timeout(struct dma_fence
 **fences, uint32_t count,
   }
   EXPORT_SYMBOL(dma_fence_wait_any_timeout);
   +
+/**
+ * dma_fence_set_deadline - set desired fence-wait deadline
+ * @fence:    the fence that is to be waited on
+ * @deadline: the time by which the waiter hopes for the fence to be
+ *            signaled
+ *
+ * Inform the fence signaler of an upcoming deadline, such as

vblank, by
+ * which point the waiter would prefer the fence to be signaled by. This
+ * is intended to give feedback to the fence signaler to aid in power
+ * management decisions, such as boosting GPU frequency if a periodic
+ * vblank deadline is approaching.
+ */

drivers/dma-buf/dma-fence.c +7 -0

+void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+    if (fence->ops->set_deadline && !dma_fence_is_signaled(fence))
+        fence->ops->set_deadline(fence, deadline);
+}
+EXPORT_SYMBOL(dma_fence_set_deadline);
+
   /**
    * dma_fence_describe - Dump fence describtion into seq_file
    * @fence: the 6fence to describe

include/linux/dma-fence.h +0 -0

index 775cdc0b4f24..d77f6591c453 100644

include/linux/dma-fence.h enum dma_fence_flag_bits { · line 102

Quoted 4 lines

include/linux/dma-fence.h +1 -0

@@ -99,6 +99,7 @@ enum dma_fence_flag_bits {
       DMA_FENCE_FLAG_SIGNALED_BIT,
       DMA_FENCE_FLAG_TIMESTAMP_BIT,
       DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
+    DMA_FENCE_FLAG_HAS_DEADLINE_BIT,

Would this bit be better left out from core implementation, given how the approach is the component which implements dma-fence has to track the actual deadline and all?

Quoted 4 lines

Also taking a step back - are we all okay with starting to expand the relatively simple core synchronisation primitive with side channel data like this? What would be the criteria for what side channel data would be acceptable? Taking note the thing lives outside drivers/gpu/.

Quoted 7 lines

I had similar concerns and it took me a moment as well to understand the background why this is necessary. I essentially don't see much other approach we could do.

Yes, this is GPU/CRTC specific, but we somehow need a common interface for communicating it between drivers and that's the dma_fence object as far as I can see.

Yeah I also don't see any other easy options. Just wanted to raise this as something which probably needs some wider acks.

Quoted 7 lines

Quoted 2 lines

Also what about the "low level" part of my question about the reason, or benefits, of defining the deadline bit in the common layer?

We could leave DMA_FENCE_FLAG_HAS_DEADLINE_BIT out, but OTOH managing a bitmask that is partially defined in core enum and partially in backend-driver has it's own drawbacks, and it isn't like we are running out of bits.. :shrug:

There is DMA_FENCE_FLAG_USER_BITS onwards which implementations could use to store their stuff?

And if we skip forward to "drm/scheduler: Add fence deadline support" that's the only place bit is used, right? Would it simply work to look at drm_sched_fence->deadline == 0 as bit not set? Or you see a need to interoperate with other fence implementations via that bit somehow?

Regards,

Tvrtko

From: Rob Clark <[email protected]>

Add a way to hint to the fence signaler of an upcoming deadline, such as vblank, which the fence waiter would prefer not to miss. This is to aid the fence signaler in making power management decisions, like boosting frequency as the deadline approaches and awareness of missing deadlines so that can be factored in to the frequency scaling.

v2: Drop dma_fence::deadline and related logic to filter duplicate
deadlines, to avoid increasing dma_fence size. The fence-context implementation will need similar logic to track deadlines of all the fences on the same timeline. [ckoenig]
v3: Clarify locking wrt. set_deadline callback

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

Reviewed-by Christian König <[email protected]>

---
 drivers/dma-buf/dma-fence.c | 20 ++++++++++++++++++++
 include/linux/dma-fence.h   | 20 ++++++++++++++++++++
 2 files changed, 40 insertions(+)

drivers/dma-buf/dma-fence.c +0 -0

index 0de0482cd36e..763b32627684 100644

drivers/dma-buf/dma-fence.c +18 -0

+ * dma_fence_set_deadline - set desired fence-wait deadline
+ * @fence:    the fence that is to be waited on
+ * @deadline: the time by which the waiter hopes for the fence to be
+ *            signaled
+ *
+ * Inform the fence signaler of an upcoming deadline, such as vblank, by
+ * which point the waiter would prefer the fence to be signaled by.  This
+ * is intended to give feedback to the fence signaler to aid in power
+ * management decisions, such as boosting GPU frequency if a periodic
+ * vblank deadline is approaching.
+ */
+void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+	if (fence->ops->set_deadline && !dma_fence_is_signaled(fence))
+		fence->ops->set_deadline(fence, deadline);
+}
+EXPORT_SYMBOL(dma_fence_set_deadline);
+
 /**
  * dma_fence_describe - Dump fence describtion into seq_file
  * @fence: the 6fence to describe

include/linux/dma-fence.h +20 -0

index 775cdc0b4f24..d77f6591c453 100644
@@ -99,6 +99,7 @@ enum dma_fence_flag_bits {
 	DMA_FENCE_FLAG_SIGNALED_BIT,
 	DMA_FENCE_FLAG_TIMESTAMP_BIT,
 	DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
+	DMA_FENCE_FLAG_HAS_DEADLINE_BIT,
 	DMA_FENCE_FLAG_USER_BITS, /* must always be last member */
 };
 

@@ -257,6 +258,23 @@ struct dma_fence_ops {
 	 */
 	void (*timeline_value_str)(struct dma_fence *fence,
 				   char *str, int size);
+
+	/**
+	 * @set_deadline:
+	 *
+	 * Callback to allow a fence waiter to inform the fence signaler of
+	 * an upcoming deadline, such as vblank, by which point the waiter
+	 * would prefer the fence to be signaled by.  This is intended to
+	 * give feedback to the fence signaler to aid in power management
+	 * decisions, such as boosting GPU frequency.
+	 *
+	 * This is called without &dma_fence.lock held, it can be called
+	 * multiple times and from any context.  Locking is up to the callee
+	 * if it has some state to manage.
+	 *
+	 * This callback is optional.
+	 */
+	void (*set_deadline)(struct dma_fence *fence, ktime_t deadline);
 };
 
 void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,

@@ -583,6 +601,8 @@ static inline signed long dma_fence_wait(struct dma_fence *fence, bool intr)
 	return ret < 0 ? ret : 0;
 }
 
+void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline);
+
 struct dma_fence *dma_fence_get_stub(void);
 struct dma_fence *dma_fence_allocate_private_stub(void);
 u64 dma_fence_context_alloc(unsigned num);

From: Rob Clark <[email protected]>

Propagate the deadline to all the fences in the chain.

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

Reviewed-by Christian König <[email protected]> for this one.

---
  drivers/dma-buf/dma-fence-chain.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)

drivers/dma-buf/dma-fence-chain.c +0 -0

index a0d920576ba6..4684874af612 100644

drivers/dma-buf/dma-fence-chain.c +6 -0

+
+		dma_fence_set_deadline(f, deadline);
+	}
+}
+
  const struct dma_fence_ops dma_fence_chain_ops = {
  	.use_64bit_seqno = true,
  	.get_driver_name = dma_fence_chain_get_driver_name,

@@ -213,6 +225,7 @@ const struct dma_fence_ops dma_fence_chain_ops = {
  	.enable_signaling = dma_fence_chain_enable_signaling,
  	.signaled = dma_fence_chain_signaled,
  	.release = dma_fence_chain_release,
+	.set_deadline = dma_fence_chain_set_deadline,
  };
  EXPORT_SYMBOL(dma_fence_chain_ops);
 

Quoted 15 lines · Rob Clark

From: Rob Clark <[email protected]>

Propagate the deadline to all the fences in the chain.

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

Reviewed-by Christian König <[email protected]> for this one.

---
  drivers/dma-buf/dma-fence-chain.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)

drivers/dma-buf/dma-fence-chain.c +0 -0

index a0d920576ba6..4684874af612 100644

drivers/dma-buf/dma-fence-chain.c static void dma_fence_chain_release(struct dma_fence *fence) · line 215

Quoted 10 lines

drivers/dma-buf/dma-fence-chain.c +7 -0

@@ -206,6 +206,18 @@ static void dma_fence_chain_release(struct dma_fence *fence)
      dma_fence_free(fence);
  }
 
+
+static void dma_fence_chain_set_deadline(struct dma_fence *fence,
+                                      ktime_t deadline)
+{
+     dma_fence_chain_for_each(fence, fence) {
+             struct dma_fence_chain *chain = to_dma_fence_chain(fence);
+             struct dma_fence *f = chain ? chain->fence : fence;

Low level comment - above two lines could be replaced with:

        struct dma_fence *f = dma_fence_chain_contained(fence);

Although to be fair I am not sure that wouldn't be making it less readable. From the point of view that fence might not be a chain, so dma_fence_chain_contained() reads a bit dodgy as an API.

Regards,

Tvrtko

Quoted 16 lines

drivers/dma-buf/dma-fence-chain.c +6 -0

+
+             dma_fence_set_deadline(f, deadline);
+     }
+}
+
  const struct dma_fence_ops dma_fence_chain_ops = {
      .use_64bit_seqno = true,
      .get_driver_name = dma_fence_chain_get_driver_name,

@@ -213,6 +225,7 @@ const struct dma_fence_ops dma_fence_chain_ops = {
      .enable_signaling = dma_fence_chain_enable_signaling,
      .signaled = dma_fence_chain_signaled,
      .release = dma_fence_chain_release,
+     .set_deadline = dma_fence_chain_set_deadline,
  };
  EXPORT_SYMBOL(dma_fence_chain_ops);
 

From: Rob Clark <[email protected]>

Add a way to set a deadline on remaining resv fences according to the requested usage.

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

---
  drivers/dma-buf/dma-resv.c | 19 +++++++++++++++++++
  include/linux/dma-resv.h   |  2 ++
  2 files changed, 21 insertions(+)

drivers/dma-buf/dma-resv.c +0 -0

index 1c76aed8e262..0c86f6d577ab 100644

drivers/dma-buf/dma-resv.c +14 -0

+ */
+void dma_resv_set_deadline(struct dma_resv *obj, enum dma_resv_usage usage,
+			   ktime_t deadline)
+{
+	struct dma_resv_iter cursor;
+	struct dma_fence *fence;
+
+	dma_resv_iter_begin(&cursor, obj, usage);
+	dma_resv_for_each_fence_unlocked(&cursor, fence) {
+		dma_fence_set_deadline(fence, deadline);
+	}
+	dma_resv_iter_end(&cursor);
+}
+EXPORT_SYMBOL_GPL(dma_resv_set_deadline);
 
  /**
   * dma_resv_test_signaled - Test if a reservation object's fences have been

include/linux/dma-resv.h +2 -0

index 0637659a702c..8d0e34dad446 100644
@@ -479,6 +479,8 @@ int dma_resv_get_singleton(struct dma_resv *obj, enum dma_resv_usage usage,
  int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src);
  long dma_resv_wait_timeout(struct dma_resv *obj, enum dma_resv_usage usage,
  			   bool intr, unsigned long timeout);
+void dma_resv_set_deadline(struct dma_resv *obj, enum dma_resv_usage usage,
+			   ktime_t deadline);
  bool dma_resv_test_signaled(struct dma_resv *obj, enum dma_resv_usage usage);
  void dma_resv_describe(struct dma_resv *obj, struct seq_file *seq);
 

From: Rob Clark <[email protected]>

The initial purpose is for igt tests, but this would also be useful for compositors that wait until close to vblank deadline to make decisions about which frame to show.

The igt tests can be found at:

https://gitlab.freedesktop.org/robclark/igt-gpu-tools/-/commits/fence-deadline

v2: Clarify the timebase, add link to igt tests

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

---
  drivers/dma-buf/sync_file.c    | 19 +++++++++++++++++++
  include/uapi/linux/sync_file.h | 22 ++++++++++++++++++++++
  2 files changed, 41 insertions(+)

drivers/dma-buf/sync_file.c +19 -0

index af57799c86ce..fb6ca1032885 100644
@@ -350,6 +350,22 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
  	return ret;
  }
 
+static int sync_file_ioctl_set_deadline(struct sync_file *sync_file,
+					unsigned long arg)
+{
+	struct sync_set_deadline ts;
+
+	if (copy_from_user(&ts, (void __user *)arg, sizeof(ts)))
+		return -EFAULT;
+
+	if (ts.pad)
+		return -EINVAL;
+
+	dma_fence_set_deadline(sync_file->fence, ktime_set(ts.tv_sec, ts.tv_nsec));
+
+	return 0;
+}
+
  static long sync_file_ioctl(struct file *file, unsigned int cmd,
  			    unsigned long arg)
  {

@@ -362,6 +378,9 @@ static long sync_file_ioctl(struct file *file, unsigned int cmd,
  	case SYNC_IOC_FILE_INFO:
  		return sync_file_ioctl_fence_info(sync_file, arg);
 
+	case SYNC_IOC_SET_DEADLINE:
+		return sync_file_ioctl_set_deadline(sync_file, arg);
+
  	default:
  		return -ENOTTY;
  	}

include/uapi/linux/sync_file.h +2 -0

index ee2dcfb3d660..c8666580816f 100644
@@ -67,6 +67,20 @@ struct sync_file_info {
  	__u64	sync_fence_info;
  };
 
+/**
+ * struct sync_set_deadline - set a deadline on a fence

include/uapi/linux/sync_file.h +10 -0

+};
+
  #define SYNC_IOC_MAGIC		'>'
 
  /**

@@ -95,4 +109,12 @@ struct sync_file_info {
   */
  #define SYNC_IOC_FILE_INFO	_IOWR(SYNC_IOC_MAGIC, 4, struct sync_file_info)
 
+
+/**
+ * DOC: SYNC_IOC_SET_DEADLINE - set a deadline on a fence
+ *
+ * Allows userspace to set a deadline on a fence, see dma_fence_set_deadline()
+ */
+#define SYNC_IOC_SET_DEADLINE	_IOW(SYNC_IOC_MAGIC, 5, struct sync_set_deadline)
+
  #endif /* _UAPI_LINUX_SYNC_H */

Am 18.02.23 um 22:15 schrieb Rob Clark:

Quoted 65 lines

From: Rob Clark <[email protected]>

The initial purpose is for igt tests, but this would also be useful for compositors that wait until close to vblank deadline to make decisions about which frame to show.

The igt tests can be found at:

https://gitlab.freedesktop.org/robclark/igt-gpu-tools/-/commits/fence-deadline

v2: Clarify the timebase, add link to igt tests

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

---
  drivers/dma-buf/sync_file.c    | 19 +++++++++++++++++++
  include/uapi/linux/sync_file.h | 22 ++++++++++++++++++++++
  2 files changed, 41 insertions(+)

drivers/dma-buf/sync_file.c +19 -0

index af57799c86ce..fb6ca1032885 100644
@@ -350,6 +350,22 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
      return ret;
  }
 
+static int sync_file_ioctl_set_deadline(struct sync_file *sync_file,
+                                     unsigned long arg)
+{
+     struct sync_set_deadline ts;
+
+     if (copy_from_user(&ts, (void __user *)arg, sizeof(ts)))
+             return -EFAULT;
+
+     if (ts.pad)
+             return -EINVAL;
+
+     dma_fence_set_deadline(sync_file->fence, ktime_set(ts.tv_sec, ts.tv_nsec));
+
+     return 0;
+}
+
  static long sync_file_ioctl(struct file *file, unsigned int cmd,
                          unsigned long arg)
  {

@@ -362,6 +378,9 @@ static long sync_file_ioctl(struct file *file, unsigned int cmd,
      case SYNC_IOC_FILE_INFO:
              return sync_file_ioctl_fence_info(sync_file, arg);
 
+     case SYNC_IOC_SET_DEADLINE:
+             return sync_file_ioctl_set_deadline(sync_file, arg);
+
      default:
              return -ENOTTY;
      }

include/uapi/linux/sync_file.h +2 -0

index ee2dcfb3d660..c8666580816f 100644
@@ -67,6 +67,20 @@ struct sync_file_info {
      __u64   sync_fence_info;
  };
 
+/**
+ * struct sync_set_deadline - set a deadline on a fence

include/uapi/linux/sync_file.h struct sync_file_info { · line 81

Quoted 10 lines

include/uapi/linux/sync_file.h +10 -0

+ * @tv_sec:  seconds elapsed since epoch
+ * @tv_nsec: nanoseconds elapsed since the time given by the tv_sec
+ * @pad:     must be zero
+ *
+ * The timebase for the deadline is CLOCK_MONOTONIC (same as vblank)
+ */
+struct sync_set_deadline {
+     __s64   tv_sec;
+     __s32   tv_nsec;
+     __u32   pad;

IIRC struct timespec defined this as time_t/long (which is horrible for an UAPI because of the sizeof(long) dependency), one possible alternative is to use 64bit nanoseconds from CLOCK_MONOTONIC (which is essentially ktime).

Not 100% sure if there is any preferences documented, but I think the later might be better.

Either way the patch is Acked-by: Christian König
<[email protected]> for this patch.

Regards,
Christian.

Quoted 18 lines

include/uapi/linux/sync_file.h +10 -0

+};
+
  #define SYNC_IOC_MAGIC              '>'
 
  /**

@@ -95,4 +109,12 @@ struct sync_file_info {
   */
  #define SYNC_IOC_FILE_INFO  _IOWR(SYNC_IOC_MAGIC, 4, struct sync_file_info)
 
+
+/**
+ * DOC: SYNC_IOC_SET_DEADLINE - set a deadline on a fence
+ *
+ * Allows userspace to set a deadline on a fence, see dma_fence_set_deadline()
+ */
+#define SYNC_IOC_SET_DEADLINE        _IOW(SYNC_IOC_MAGIC, 5, struct sync_set_deadline)
+
  #endif /* _UAPI_LINUX_SYNC_H */

Quoted 83 lines · Christian König

Am 18.02.23 um 22:15 schrieb Rob Clark:

Quoted 65 lines

From: Rob Clark <[email protected]>

The initial purpose is for igt tests, but this would also be useful for compositors that wait until close to vblank deadline to make decisions about which frame to show.

The igt tests can be found at:

https://gitlab.freedesktop.org/robclark/igt-gpu-tools/-/commits/fence-deadline

v2: Clarify the timebase, add link to igt tests

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

---
  drivers/dma-buf/sync_file.c    | 19 +++++++++++++++++++
  include/uapi/linux/sync_file.h | 22 ++++++++++++++++++++++
  2 files changed, 41 insertions(+)

drivers/dma-buf/sync_file.c +19 -0

index af57799c86ce..fb6ca1032885 100644
@@ -350,6 +350,22 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
      return ret;
  }
 
+static int sync_file_ioctl_set_deadline(struct sync_file *sync_file,
+                                     unsigned long arg)
+{
+     struct sync_set_deadline ts;
+
+     if (copy_from_user(&ts, (void __user *)arg, sizeof(ts)))
+             return -EFAULT;
+
+     if (ts.pad)
+             return -EINVAL;
+
+     dma_fence_set_deadline(sync_file->fence, ktime_set(ts.tv_sec, ts.tv_nsec));
+
+     return 0;
+}
+
  static long sync_file_ioctl(struct file *file, unsigned int cmd,
                          unsigned long arg)
  {

@@ -362,6 +378,9 @@ static long sync_file_ioctl(struct file *file, unsigned int cmd,
      case SYNC_IOC_FILE_INFO:
              return sync_file_ioctl_fence_info(sync_file, arg);
 
+     case SYNC_IOC_SET_DEADLINE:
+             return sync_file_ioctl_set_deadline(sync_file, arg);
+
      default:
              return -ENOTTY;
      }

include/uapi/linux/sync_file.h +2 -0

index ee2dcfb3d660..c8666580816f 100644
@@ -67,6 +67,20 @@ struct sync_file_info {
      __u64   sync_fence_info;
  };
 
+/**
+ * struct sync_set_deadline - set a deadline on a fence

include/uapi/linux/sync_file.h struct sync_file_info { · line 81

Quoted 10 lines

include/uapi/linux/sync_file.h +10 -0

+ * @tv_sec:  seconds elapsed since epoch
+ * @tv_nsec: nanoseconds elapsed since the time given by the tv_sec
+ * @pad:     must be zero
+ *
+ * The timebase for the deadline is CLOCK_MONOTONIC (same as vblank)
+ */
+struct sync_set_deadline {
+     __s64   tv_sec;
+     __s32   tv_nsec;
+     __u32   pad;  

IIRC struct timespec defined this as time_t/long (which is horrible for an UAPI because of the sizeof(long) dependency), one possible alternative is to use 64bit nanoseconds from CLOCK_MONOTONIC (which is essentially ktime).

Quoted 2 lines

Not 100% sure if there is any preferences documented, but I think the later might be better.

The original thought is that this maps directly to clock_gettime() without extra conversion needed, and is similar to other pre-ktime_t UAPI. But OTOH if userspace wants to add an offset, it is maybe better to convert completely to ns in userspace and use a u64 (as that is what ns_to_ktime() uses).. (and OFC whatever decision here also applies to the syncobj wait ioctls)

I'm leaning towards u64 CLOCK_MONOTONIC ns if no one has a good argument against that.

BR,
-R

Quoted 25 lines

Either way the patch is Acked-by: Christian König
<[email protected]> for this patch.

Regards,
Christian.

Quoted 18 lines

include/uapi/linux/sync_file.h +10 -0

+};
+
  #define SYNC_IOC_MAGIC              '>'
 
  /**

@@ -95,4 +109,12 @@ struct sync_file_info {
   */
  #define SYNC_IOC_FILE_INFO  _IOWR(SYNC_IOC_MAGIC, 4, struct sync_file_info)
 
+
+/**
+ * DOC: SYNC_IOC_SET_DEADLINE - set a deadline on a fence
+ *
+ * Allows userspace to set a deadline on a fence, see dma_fence_set_deadline()
+ */
+#define SYNC_IOC_SET_DEADLINE        _IOW(SYNC_IOC_MAGIC, 5, struct sync_set_deadline)
+
  #endif /* _UAPI_LINUX_SYNC_H */  

Quoted 81 lines · Christian König

Am 18.02.23 um 22:15 schrieb Rob Clark:

Quoted 65 lines

From: Rob Clark <[email protected]>

The initial purpose is for igt tests, but this would also be useful for compositors that wait until close to vblank deadline to make decisions about which frame to show.

The igt tests can be found at:

https://gitlab.freedesktop.org/robclark/igt-gpu-tools/-/commits/fence-deadline

v2: Clarify the timebase, add link to igt tests

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

---
   drivers/dma-buf/sync_file.c    | 19 +++++++++++++++++++
   include/uapi/linux/sync_file.h | 22 ++++++++++++++++++++++
   2 files changed, 41 insertions(+)

drivers/dma-buf/sync_file.c +19 -0

index af57799c86ce..fb6ca1032885 100644
@@ -350,6 +350,22 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
       return ret;
   }
 
+static int sync_file_ioctl_set_deadline(struct sync_file *sync_file,
+                                     unsigned long arg)
+{
+     struct sync_set_deadline ts;
+
+     if (copy_from_user(&ts, (void __user *)arg, sizeof(ts)))
+             return -EFAULT;
+
+     if (ts.pad)
+             return -EINVAL;
+
+     dma_fence_set_deadline(sync_file->fence, ktime_set(ts.tv_sec, ts.tv_nsec));
+
+     return 0;
+}
+
   static long sync_file_ioctl(struct file *file, unsigned int cmd,
                           unsigned long arg)
   {

@@ -362,6 +378,9 @@ static long sync_file_ioctl(struct file *file, unsigned int cmd,
       case SYNC_IOC_FILE_INFO:
               return sync_file_ioctl_fence_info(sync_file, arg);
 
+     case SYNC_IOC_SET_DEADLINE:
+             return sync_file_ioctl_set_deadline(sync_file, arg);
+
       default:
               return -ENOTTY;
       }

include/uapi/linux/sync_file.h +2 -0

index ee2dcfb3d660..c8666580816f 100644
@@ -67,6 +67,20 @@ struct sync_file_info {
       __u64   sync_fence_info;
   };
 
+/**
+ * struct sync_set_deadline - set a deadline on a fence

include/uapi/linux/sync_file.h struct sync_file_info { · line 81

Quoted 10 lines

include/uapi/linux/sync_file.h +10 -0

+ * @tv_sec:  seconds elapsed since epoch
+ * @tv_nsec: nanoseconds elapsed since the time given by the tv_sec
+ * @pad:     must be zero
+ *
+ * The timebase for the deadline is CLOCK_MONOTONIC (same as vblank)
+ */
+struct sync_set_deadline {
+     __s64   tv_sec;
+     __s32   tv_nsec;
+     __u32   pad;

IIRC struct timespec defined this as time_t/long (which is horrible for an UAPI because of the sizeof(long) dependency), one possible alternative is to use 64bit nanoseconds from CLOCK_MONOTONIC (which is essentially ktime).

Quoted 2 lines

Not 100% sure if there is any preferences documented, but I think the later might be better.

The original thought is that this maps directly to clock_gettime() without extra conversion needed, and is similar to other pre-ktime_t UAPI. But OTOH if userspace wants to add an offset, it is maybe better to convert completely to ns in userspace and use a u64 (as that is what ns_to_ktime() uses).. (and OFC whatever decision here also applies to the syncobj wait ioctls)

I'm leaning towards u64 CLOCK_MONOTONIC ns if no one has a good argument against that.

BR,
-R

Quoted 24 lines

Either way the patch is Acked-by: Christian König
<[email protected]> for this patch.

Regards,
Christian.

Quoted 18 lines

include/uapi/linux/sync_file.h +10 -0

+};
+
   #define SYNC_IOC_MAGIC              '>'
 
   /**

@@ -95,4 +109,12 @@ struct sync_file_info {
    */
   #define SYNC_IOC_FILE_INFO  _IOWR(SYNC_IOC_MAGIC, 4, struct sync_file_info)
 
+
+/**
+ * DOC: SYNC_IOC_SET_DEADLINE - set a deadline on a fence
+ *
+ * Allows userspace to set a deadline on a fence, see dma_fence_set_deadline()
+ */
+#define SYNC_IOC_SET_DEADLINE        _IOW(SYNC_IOC_MAGIC, 5, struct sync_set_deadline)
+
   #endif /* _UAPI_LINUX_SYNC_H */

From: Rob Clark <[email protected]>

The initial purpose is for igt tests, but this would also be useful for compositors that wait until close to vblank deadline to make decisions about which frame to show.

The igt tests can be found at:

https://gitlab.freedesktop.org/robclark/igt-gpu-tools/-/commits/fence-deadline

v2: Clarify the timebase, add link to igt tests

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

---
 drivers/dma-buf/sync_file.c    | 19 +++++++++++++++++++
 include/uapi/linux/sync_file.h | 22 ++++++++++++++++++++++
 2 files changed, 41 insertions(+)

drivers/dma-buf/sync_file.c +19 -0

index af57799c86ce..fb6ca1032885 100644
@@ -350,6 +350,22 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 	return ret;
 }
 
+static int sync_file_ioctl_set_deadline(struct sync_file *sync_file,
+					unsigned long arg)
+{
+	struct sync_set_deadline ts;
+
+	if (copy_from_user(&ts, (void __user *)arg, sizeof(ts)))
+		return -EFAULT;
+
+	if (ts.pad)
+		return -EINVAL;
+
+	dma_fence_set_deadline(sync_file->fence, ktime_set(ts.tv_sec, ts.tv_nsec));
+
+	return 0;
+}
+
 static long sync_file_ioctl(struct file *file, unsigned int cmd,
 			    unsigned long arg)
 {

@@ -362,6 +378,9 @@ static long sync_file_ioctl(struct file *file, unsigned int cmd,
 	case SYNC_IOC_FILE_INFO:
 		return sync_file_ioctl_fence_info(sync_file, arg);
 
+	case SYNC_IOC_SET_DEADLINE:
+		return sync_file_ioctl_set_deadline(sync_file, arg);
+
 	default:
 		return -ENOTTY;
 	}

include/uapi/linux/sync_file.h +0 -0

index ee2dcfb3d660..c8666580816f 100644

include/uapi/linux/sync_file.h +18 -0

+ * @pad:	must be zero
+ *
+ * The timebase for the deadline is CLOCK_MONOTONIC (same as vblank)
+ */
+struct sync_set_deadline {
+	__s64	tv_sec;
+	__s32	tv_nsec;
+	__u32	pad;
+};
+
 #define SYNC_IOC_MAGIC		'>'
 
 /**

@@ -95,4 +109,12 @@ struct sync_file_info {
  */
 #define SYNC_IOC_FILE_INFO	_IOWR(SYNC_IOC_MAGIC, 4, struct sync_file_info)
 
+
+/**
+ * DOC: SYNC_IOC_SET_DEADLINE - set a deadline on a fence
+ *
+ * Allows userspace to set a deadline on a fence, see dma_fence_set_deadline()
+ */
+#define SYNC_IOC_SET_DEADLINE	_IOW(SYNC_IOC_MAGIC, 5, struct sync_set_deadline)
+
 #endif /* _UAPI_LINUX_SYNC_H */

From: Rob Clark <[email protected]>

Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent wait (as opposed to a "housekeeping" wait to know when to cleanup after some work has completed). Usermode components of GPU driver stacks often poll() on fence fd's to know when it is safe to do things like free or reuse a buffer, but they can also poll() on a fence fd when waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag lets the kernel differentiate these two cases.

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

---
  drivers/dma-buf/sync_file.c | 8 ++++++++
  1 file changed, 8 insertions(+)

drivers/dma-buf/sync_file.c +8 -0

index fb6ca1032885..c30b2085ee0a 100644
@@ -192,6 +192,14 @@ static __poll_t sync_file_poll(struct file *file, poll_table *wait)
  {
  	struct sync_file *sync_file = file->private_data;
 
+	/*
+	 * The POLLPRI/EPOLLPRI flag can be used to signal that
+	 * userspace wants the fence to signal ASAP, express this
+	 * as an immediate deadline.
+	 */
+	if (poll_requested_events(wait) & EPOLLPRI)
+		dma_fence_set_deadline(sync_file->fence, ktime_get());
+
  	poll_wait(file, &sync_file->wq, wait);
 
  	if (list_empty(&sync_file->cb.node) &&

Am 18.02.23 um 22:15 schrieb Rob Clark:

Quoted 10 lines

From: Rob Clark <[email protected]>

Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent wait (as opposed to a "housekeeping" wait to know when to cleanup after some work has completed). Usermode components of GPU driver stacks often poll() on fence fd's to know when it is safe to do things like free or reuse a buffer, but they can also poll() on a fence fd when waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag lets the kernel differentiate these two cases.

Quoted 1 line

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

The code looks clean, but the different poll flags and their meaning are certainly not my field of expertise.

Feel free to add Acked-by: Christian König <[email protected]>, somebody with more background in this should probably take a look as well.

Regards,
Christian.

Quoted 23 lines

---
  drivers/dma-buf/sync_file.c | 8 ++++++++
  1 file changed, 8 insertions(+)

drivers/dma-buf/sync_file.c +8 -0

index fb6ca1032885..c30b2085ee0a 100644
@@ -192,6 +192,14 @@ static __poll_t sync_file_poll(struct file *file, poll_table *wait)
  {
  	struct sync_file *sync_file = file->private_data;
 
+	/*
+	 * The POLLPRI/EPOLLPRI flag can be used to signal that
+	 * userspace wants the fence to signal ASAP, express this
+	 * as an immediate deadline.
+	 */
+	if (poll_requested_events(wait) & EPOLLPRI)
+		dma_fence_set_deadline(sync_file->fence, ktime_get());
+
  	poll_wait(file, &sync_file->wq, wait);
 
  	if (list_empty(&sync_file->cb.node) &&  

From: Rob Clark <[email protected]>

Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent wait (as opposed to a "housekeeping" wait to know when to cleanup after some work has completed). Usermode components of GPU driver stacks often poll() on fence fd's to know when it is safe to do things like free or reuse a buffer, but they can also poll() on a fence fd when waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag lets the kernel differentiate these two cases.

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

---
 drivers/dma-buf/sync_file.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/dma-buf/sync_file.c +8 -0

index fb6ca1032885..c30b2085ee0a 100644
@@ -192,6 +192,14 @@ static __poll_t sync_file_poll(struct file *file, poll_table *wait)
 {
 	struct sync_file *sync_file = file->private_data;
 
+	/*
+	 * The POLLPRI/EPOLLPRI flag can be used to signal that
+	 * userspace wants the fence to signal ASAP, express this
+	 * as an immediate deadline.
+	 */
+	if (poll_requested_events(wait) & EPOLLPRI)
+		dma_fence_set_deadline(sync_file->fence, ktime_get());
+
 	poll_wait(file, &sync_file->wq, wait);
 
 	if (list_empty(&sync_file->cb.node) &&

Quoted 10 lines · On Sat, 18 Feb 2023 13:15:49 -0800 Rob Clark <[email protected]> wrote:

From: Rob Clark <[email protected]>

Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent wait (as opposed to a "housekeeping" wait to know when to cleanup after some work has completed). Usermode components of GPU driver stacks often poll() on fence fd's to know when it is safe to do things like free or reuse a buffer, but they can also poll() on a fence fd when waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag lets the kernel differentiate these two cases.

Quoted 1 line

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

Hi,

where would the UAPI documentation of this go?
It seems to be missing.

If a Wayland compositor is polling application fences to know which client buffer to use in its rendering, should the compositor poll with PRI or not? If a compositor polls with PRI, then all fences from all applications would always be PRI. Would that be harmful somehow or would it be beneficial?

Thanks,
pq

Quoted 23 lines

---
 drivers/dma-buf/sync_file.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/dma-buf/sync_file.c +8 -0

index fb6ca1032885..c30b2085ee0a 100644
@@ -192,6 +192,14 @@ static __poll_t sync_file_poll(struct file *file, poll_table *wait)
 {
      struct sync_file *sync_file = file->private_data;
 
+     /*
+      * The POLLPRI/EPOLLPRI flag can be used to signal that
+      * userspace wants the fence to signal ASAP, express this
+      * as an immediate deadline.
+      */
+     if (poll_requested_events(wait) & EPOLLPRI)
+             dma_fence_set_deadline(sync_file->fence, ktime_get());
+
      poll_wait(file, &sync_file->wq, wait);
 
      if (list_empty(&sync_file->cb.node) &&

Quoted 14 lines · Pekka Paalanen

Quoted 10 lines · On Sat, 18 Feb 2023 13:15:49 -0800 Rob Clark <[email protected]> wrote:

From: Rob Clark <[email protected]>

Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent wait (as opposed to a "housekeeping" wait to know when to cleanup after some work has completed). Usermode components of GPU driver stacks often poll() on fence fd's to know when it is safe to do things like free or reuse a buffer, but they can also poll() on a fence fd when waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag lets the kernel differentiate these two cases.

Quoted 6 lines

Quoted 1 line

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

Hi,

where would the UAPI documentation of this go?
It seems to be missing.

Good question, I am not sure. The poll() man page has a description, but my usage doesn't fit that _exactly_ (but OTOH the description is a bit vague).

Quoted 5 lines

If a Wayland compositor is polling application fences to know which client buffer to use in its rendering, should the compositor poll with PRI or not? If a compositor polls with PRI, then all fences from all applications would always be PRI. Would that be harmful somehow or would it be beneficial?

I think a compositor would rather use the deadline ioctl and then poll without PRI. Otherwise you are giving an urgency signal to the fence signaller which might not necessarily be needed.

The places where I expect PRI to be useful is more in mesa (things like glFinish(), readpix, and other similar sorts of blocking APIs)

BR,
-R

Quoted 29 lines

Thanks,
pq

Quoted 23 lines

---
 drivers/dma-buf/sync_file.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/dma-buf/sync_file.c +8 -0

index fb6ca1032885..c30b2085ee0a 100644
@@ -192,6 +192,14 @@ static __poll_t sync_file_poll(struct file *file, poll_table *wait)
 {
      struct sync_file *sync_file = file->private_data;
 
+     /*
+      * The POLLPRI/EPOLLPRI flag can be used to signal that
+      * userspace wants the fence to signal ASAP, express this
+      * as an immediate deadline.
+      */
+     if (poll_requested_events(wait) & EPOLLPRI)
+             dma_fence_set_deadline(sync_file->fence, ktime_get());
+
      poll_wait(file, &sync_file->wq, wait);
 
      if (list_empty(&sync_file->cb.node) &&  

Quoted 32 lines · On Mon, 20 Feb 2023 08:14:47 -0800 Rob Clark <[email protected]> wrote:

Quoted 14 lines · Pekka Paalanen

Quoted 10 lines · On Sat, 18 Feb 2023 13:15:49 -0800 Rob Clark <[email protected]> wrote:

From: Rob Clark <[email protected]>

Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent wait (as opposed to a "housekeeping" wait to know when to cleanup after some work has completed). Usermode components of GPU driver stacks often poll() on fence fd's to know when it is safe to do things like free or reuse a buffer, but they can also poll() on a fence fd when waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag lets the kernel differentiate these two cases.

Quoted 6 lines

Quoted 1 line

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

Hi,

where would the UAPI documentation of this go?
It seems to be missing.

Good question, I am not sure. The poll() man page has a description, but my usage doesn't fit that _exactly_ (but OTOH the description is a bit vague).

Quoted 5 lines

If a Wayland compositor is polling application fences to know which client buffer to use in its rendering, should the compositor poll with PRI or not? If a compositor polls with PRI, then all fences from all applications would always be PRI. Would that be harmful somehow or would it be beneficial?

Quoted 6 lines

I think a compositor would rather use the deadline ioctl and then poll without PRI. Otherwise you are giving an urgency signal to the fence signaller which might not necessarily be needed.

The places where I expect PRI to be useful is more in mesa (things like glFinish(), readpix, and other similar sorts of blocking APIs)

Sounds good. Docs... ;-)

Hmm, so a compositor should set the deadline when it processes the wl_surface.commit, and not when it actually starts repainting, to give time for the driver to react and the GPU to do some more work. The deadline would be the time when the compositor starts its repaint, so it knows if the buffer is ready or not.

Thanks,
pq

Quoted 33 lines

BR,
-R

Quoted 29 lines

Thanks,
pq

Quoted 23 lines

---
 drivers/dma-buf/sync_file.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/dma-buf/sync_file.c +8 -0

index fb6ca1032885..c30b2085ee0a 100644
@@ -192,6 +192,14 @@ static __poll_t sync_file_poll(struct file *file, poll_table *wait)
 {
      struct sync_file *sync_file = file->private_data;
 
+     /*
+      * The POLLPRI/EPOLLPRI flag can be used to signal that
+      * userspace wants the fence to signal ASAP, express this
+      * as an immediate deadline.
+      */
+     if (poll_requested_events(wait) & EPOLLPRI)
+             dma_fence_set_deadline(sync_file->fence, ktime_get());
+
      poll_wait(file, &sync_file->wq, wait);
 
      if (list_empty(&sync_file->cb.node) &&

Quoted 43 lines · Pekka Paalanen

Quoted 32 lines · On Mon, 20 Feb 2023 08:14:47 -0800 Rob Clark <[email protected]> wrote:

Quoted 14 lines · Pekka Paalanen

Quoted 10 lines · On Sat, 18 Feb 2023 13:15:49 -0800 Rob Clark <[email protected]> wrote:

From: Rob Clark <[email protected]>

Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent wait (as opposed to a "housekeeping" wait to know when to cleanup after some work has completed). Usermode components of GPU driver stacks often poll() on fence fd's to know when it is safe to do things like free or reuse a buffer, but they can also poll() on a fence fd when waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag lets the kernel differentiate these two cases.

Quoted 6 lines

Quoted 1 line

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

Hi,

where would the UAPI documentation of this go?
It seems to be missing.

Good question, I am not sure. The poll() man page has a description, but my usage doesn't fit that _exactly_ (but OTOH the description is a bit vague).

Quoted 5 lines

If a Wayland compositor is polling application fences to know which client buffer to use in its rendering, should the compositor poll with PRI or not? If a compositor polls with PRI, then all fences from all applications would always be PRI. Would that be harmful somehow or would it be beneficial?

Quoted 6 lines

I think a compositor would rather use the deadline ioctl and then poll without PRI. Otherwise you are giving an urgency signal to the fence signaller which might not necessarily be needed.

The places where I expect PRI to be useful is more in mesa (things like glFinish(), readpix, and other similar sorts of blocking APIs)

Quoted 7 lines

Sounds good. Docs... ;-)

Hmm, so a compositor should set the deadline when it processes the wl_surface.commit, and not when it actually starts repainting, to give time for the driver to react and the GPU to do some more work. The deadline would be the time when the compositor starts its repaint, so it knows if the buffer is ready or not.

Technically we don't know when the commit is supposed to be shown. Just passing a deadline of the next possible deadline however is probably a good enough guess for this feature to be useful.

One thing that neither API allows us to do is tell the kernel in advance when we're going to submit work and what the deadline for it is and unfortunately that work is the most timing sensitive.

Quoted 40 lines

Thanks,
pq

Quoted 33 lines

BR,
-R

Quoted 29 lines

Thanks,
pq

Quoted 23 lines

---
 drivers/dma-buf/sync_file.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/dma-buf/sync_file.c +8 -0

index fb6ca1032885..c30b2085ee0a 100644
@@ -192,6 +192,14 @@ static __poll_t sync_file_poll(struct file *file, poll_table *wait)
 {
      struct sync_file *sync_file = file->private_data;
 
+     /*
+      * The POLLPRI/EPOLLPRI flag can be used to signal that
+      * userspace wants the fence to signal ASAP, express this
+      * as an immediate deadline.
+      */
+     if (poll_requested_events(wait) & EPOLLPRI)
+             dma_fence_set_deadline(sync_file->fence, ktime_get());
+
      poll_wait(file, &sync_file->wq, wait);
 
      if (list_empty(&sync_file->cb.node) &&

Quoted 14 lines · Pekka Paalanen

Quoted 10 lines · On Sat, 18 Feb 2023 13:15:49 -0800 Rob Clark <[email protected]> wrote:

From: Rob Clark <[email protected]>

Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent wait (as opposed to a "housekeeping" wait to know when to cleanup after some work has completed). Usermode components of GPU driver stacks often poll() on fence fd's to know when it is safe to do things like free or reuse a buffer, but they can also poll() on a fence fd when waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag lets the kernel differentiate these two cases.

Quoted 6 lines

Quoted 1 line

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

Hi,

where would the UAPI documentation of this go?
It seems to be missing.

Good question, I am not sure. The poll() man page has a description, but my usage doesn't fit that _exactly_ (but OTOH the description is a bit vague).

Quoted 5 lines

If a Wayland compositor is polling application fences to know which client buffer to use in its rendering, should the compositor poll with PRI or not? If a compositor polls with PRI, then all fences from all applications would always be PRI. Would that be harmful somehow or would it be beneficial?

I think a compositor would rather use the deadline ioctl and then poll without PRI. Otherwise you are giving an urgency signal to the fence signaller which might not necessarily be needed.

The places where I expect PRI to be useful is more in mesa (things like glFinish(), readpix, and other similar sorts of blocking APIs)

Quoted 32 lines · Rob Clark

Quoted 14 lines · Pekka Paalanen

Quoted 10 lines · On Sat, 18 Feb 2023 13:15:49 -0800 Rob Clark <[email protected]> wrote:

From: Rob Clark <[email protected]>

Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent wait (as opposed to a "housekeeping" wait to know when to cleanup after some work has completed). Usermode components of GPU driver stacks often poll() on fence fd's to know when it is safe to do things like free or reuse a buffer, but they can also poll() on a fence fd when waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag lets the kernel differentiate these two cases.

Quoted 6 lines

Quoted 1 line

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

Hi,

where would the UAPI documentation of this go?
It seems to be missing.

Good question, I am not sure. The poll() man page has a description, but my usage doesn't fit that _exactly_ (but OTOH the description is a bit vague).

Quoted 5 lines

If a Wayland compositor is polling application fences to know which client buffer to use in its rendering, should the compositor poll with PRI or not? If a compositor polls with PRI, then all fences from all applications would always be PRI. Would that be harmful somehow or would it be beneficial?

Quoted 6 lines

I think a compositor would rather use the deadline ioctl and then poll without PRI. Otherwise you are giving an urgency signal to the fence signaller which might not necessarily be needed.

The places where I expect PRI to be useful is more in mesa (things like glFinish(), readpix, and other similar sorts of blocking APIs)

Hi,

Hmm, but then user-space could do the opposite, namely, submit work as usual--never using the SET_DEADLINE ioctl, and then at the end, poll using (E)POLLPRI. That seems like a possible usage pattern, unintended--maybe, but possible. Do we want to discourage this? Wouldn't SET_DEADLINE be enough? I mean, one can call SET_DEADLINE with the current time, and then wouldn't that be equivalent to (E)POLLPRI?

Quoted 40 lines · Luben Tuikov

Quoted 32 lines · Rob Clark

Quoted 14 lines · Pekka Paalanen

Quoted 10 lines · On Sat, 18 Feb 2023 13:15:49 -0800 Rob Clark <[email protected]> wrote:

From: Rob Clark <[email protected]>

Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent wait (as opposed to a "housekeeping" wait to know when to cleanup after some work has completed). Usermode components of GPU driver stacks often poll() on fence fd's to know when it is safe to do things like free or reuse a buffer, but they can also poll() on a fence fd when waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag lets the kernel differentiate these two cases.

Quoted 6 lines

Quoted 1 line

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

Hi,

where would the UAPI documentation of this go?
It seems to be missing.

Good question, I am not sure. The poll() man page has a description, but my usage doesn't fit that _exactly_ (but OTOH the description is a bit vague).

Quoted 5 lines

If a Wayland compositor is polling application fences to know which client buffer to use in its rendering, should the compositor poll with PRI or not? If a compositor polls with PRI, then all fences from all applications would always be PRI. Would that be harmful somehow or would it be beneficial?

Quoted 6 lines

I think a compositor would rather use the deadline ioctl and then poll without PRI. Otherwise you are giving an urgency signal to the fence signaller which might not necessarily be needed.

The places where I expect PRI to be useful is more in mesa (things like glFinish(), readpix, and other similar sorts of blocking APIs)

Quoted 7 lines

Hi,

Hmm, but then user-space could do the opposite, namely, submit work as usual--never using the SET_DEADLINE ioctl, and then at the end, poll using (E)POLLPRI. That seems like a possible usage pattern, unintended--maybe, but possible. Do we want to discourage this? Wouldn't SET_DEADLINE be enough? I mean, one can call SET_DEADLINE with the current time, and then wouldn't that be equivalent to (E)POLLPRI?

Yeah, (E)POLLPRI isn't strictly needed if we have SET_DEADLINE. It is slightly more convenient if you want an immediate deadline (single syscall instead of two), but not strictly needed. OTOH it piggy-backs on existing UABI.

Quoted 49 lines · On Tue, 21 Feb 2023 09:53:56 -0800 Rob Clark <[email protected]> wrote:

Quoted 40 lines · Luben Tuikov

Quoted 32 lines · Rob Clark

Quoted 14 lines · Pekka Paalanen

Quoted 10 lines · On Sat, 18 Feb 2023 13:15:49 -0800 Rob Clark <[email protected]> wrote:

From: Rob Clark <[email protected]>

Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent wait (as opposed to a "housekeeping" wait to know when to cleanup after some work has completed). Usermode components of GPU driver stacks often poll() on fence fd's to know when it is safe to do things like free or reuse a buffer, but they can also poll() on a fence fd when waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag lets the kernel differentiate these two cases.

Quoted 6 lines

Quoted 1 line

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

Hi,

where would the UAPI documentation of this go?
It seems to be missing.

Good question, I am not sure. The poll() man page has a description, but my usage doesn't fit that _exactly_ (but OTOH the description is a bit vague).

Quoted 5 lines

If a Wayland compositor is polling application fences to know which client buffer to use in its rendering, should the compositor poll with PRI or not? If a compositor polls with PRI, then all fences from all applications would always be PRI. Would that be harmful somehow or would it be beneficial?

Quoted 6 lines

I think a compositor would rather use the deadline ioctl and then poll without PRI. Otherwise you are giving an urgency signal to the fence signaller which might not necessarily be needed.

The places where I expect PRI to be useful is more in mesa (things like glFinish(), readpix, and other similar sorts of blocking APIs)

Quoted 7 lines

Hi,

Hmm, but then user-space could do the opposite, namely, submit work as usual--never using the SET_DEADLINE ioctl, and then at the end, poll using (E)POLLPRI. That seems like a possible usage pattern, unintended--maybe, but possible. Do we want to discourage this? Wouldn't SET_DEADLINE be enough? I mean, one can call SET_DEADLINE with the current time, and then wouldn't that be equivalent to (E)POLLPRI?

Quoted 4 lines

Yeah, (E)POLLPRI isn't strictly needed if we have SET_DEADLINE. It is slightly more convenient if you want an immediate deadline (single syscall instead of two), but not strictly needed. OTOH it piggy-backs on existing UABI.

In that case, I would be conservative, and not add the POLLPRI semantics. An UAPI addition that is not strictly needed and somewhat unclear if it violates any design principles is best not done, until it is proven to be beneficial.

Quoted 49 lines · On Tue, 21 Feb 2023 09:53:56 -0800 Rob Clark <[email protected]> wrote:

Quoted 40 lines · Luben Tuikov

Quoted 32 lines · Rob Clark

Quoted 14 lines · Pekka Paalanen

Quoted 10 lines · On Sat, 18 Feb 2023 13:15:49 -0800 Rob Clark <[email protected]> wrote:

From: Rob Clark <[email protected]>

Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent wait (as opposed to a "housekeeping" wait to know when to cleanup after some work has completed). Usermode components of GPU driver stacks often poll() on fence fd's to know when it is safe to do things like free or reuse a buffer, but they can also poll() on a fence fd when waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag lets the kernel differentiate these two cases.

Quoted 6 lines

Quoted 1 line

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

Hi,

where would the UAPI documentation of this go?
It seems to be missing.

Good question, I am not sure. The poll() man page has a description, but my usage doesn't fit that _exactly_ (but OTOH the description is a bit vague).

Quoted 5 lines

If a Wayland compositor is polling application fences to know which client buffer to use in its rendering, should the compositor poll with PRI or not? If a compositor polls with PRI, then all fences from all applications would always be PRI. Would that be harmful somehow or would it be beneficial?

Quoted 6 lines

I think a compositor would rather use the deadline ioctl and then poll without PRI. Otherwise you are giving an urgency signal to the fence signaller which might not necessarily be needed.

The places where I expect PRI to be useful is more in mesa (things like glFinish(), readpix, and other similar sorts of blocking APIs)

Quoted 7 lines

Hi,

Hmm, but then user-space could do the opposite, namely, submit work as usual--never using the SET_DEADLINE ioctl, and then at the end, poll using (E)POLLPRI. That seems like a possible usage pattern, unintended--maybe, but possible. Do we want to discourage this? Wouldn't SET_DEADLINE be enough? I mean, one can call SET_DEADLINE with the current time, and then wouldn't that be equivalent to (E)POLLPRI?

Quoted 4 lines

Yeah, (E)POLLPRI isn't strictly needed if we have SET_DEADLINE. It is slightly more convenient if you want an immediate deadline (single syscall instead of two), but not strictly needed. OTOH it piggy-backs on existing UABI.

In that case, I would be conservative, and not add the POLLPRI semantics. An UAPI addition that is not strictly needed and somewhat unclear if it violates any design principles is best not done, until it is proven to be beneficial.

Besides, a Wayland compositor does not necessary need to add the fd to its main event loop for poll. It could just SET_DEADLINE, and then when it renders simply check if the fence passed or not already. Not polling means the compositor does not need to wake up at the moment the fence signals to just record a flag.

On another matter, if the application uses SET_DEADLINE with one timestamp, and the compositor uses SET_DEADLINE on the same thing with another timestamp, what should happen?

Maybe it's a soft-realtime app whose primary goal is not display, and it needs the result faster than the window server?

Maybe SET_DEADLINE should set the deadline only to an earlier timestamp and never later?

Thanks,
pq

Quoted 63 lines · Pekka Paalanen

Quoted 49 lines · On Tue, 21 Feb 2023 09:53:56 -0800 Rob Clark <[email protected]> wrote:

Quoted 40 lines · Luben Tuikov

Quoted 32 lines · Rob Clark

Quoted 14 lines · Pekka Paalanen

Quoted 10 lines · On Sat, 18 Feb 2023 13:15:49 -0800 Rob Clark <[email protected]> wrote:

From: Rob Clark <[email protected]>

Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent wait (as opposed to a "housekeeping" wait to know when to cleanup after some work has completed). Usermode components of GPU driver stacks often poll() on fence fd's to know when it is safe to do things like free or reuse a buffer, but they can also poll() on a fence fd when waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag lets the kernel differentiate these two cases.

Quoted 6 lines

Quoted 1 line

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

Hi,

where would the UAPI documentation of this go?
It seems to be missing.

Good question, I am not sure. The poll() man page has a description, but my usage doesn't fit that _exactly_ (but OTOH the description is a bit vague).

Quoted 5 lines

If a Wayland compositor is polling application fences to know which client buffer to use in its rendering, should the compositor poll with PRI or not? If a compositor polls with PRI, then all fences from all applications would always be PRI. Would that be harmful somehow or would it be beneficial?

Quoted 6 lines

I think a compositor would rather use the deadline ioctl and then poll without PRI. Otherwise you are giving an urgency signal to the fence signaller which might not necessarily be needed.

The places where I expect PRI to be useful is more in mesa (things like glFinish(), readpix, and other similar sorts of blocking APIs)

Quoted 7 lines

Hi,

Hmm, but then user-space could do the opposite, namely, submit work as usual--never using the SET_DEADLINE ioctl, and then at the end, poll using (E)POLLPRI. That seems like a possible usage pattern, unintended--maybe, but possible. Do we want to discourage this? Wouldn't SET_DEADLINE be enough? I mean, one can call SET_DEADLINE with the current time, and then wouldn't that be equivalent to (E)POLLPRI?

Quoted 4 lines

Yeah, (E)POLLPRI isn't strictly needed if we have SET_DEADLINE. It is slightly more convenient if you want an immediate deadline (single syscall instead of two), but not strictly needed. OTOH it piggy-backs on existing UABI.

In that case, I would be conservative, and not add the POLLPRI semantics. An UAPI addition that is not strictly needed and somewhat unclear if it violates any design principles is best not done, until it is proven to be beneficial.

Quoted 5 lines

Besides, a Wayland compositor does not necessary need to add the fd to its main event loop for poll. It could just SET_DEADLINE, and then when it renders simply check if the fence passed or not already. Not polling means the compositor does not need to wake up at the moment the fence signals to just record a flag.

poll(POLLPRI) isn't intended for wayland.. but is a thing I want in mesa for fence waits. I _could_ use SET_DEADLINE but it is two syscalls and correspondingly more code ;-)

Quoted 3 lines

On another matter, if the application uses SET_DEADLINE with one timestamp, and the compositor uses SET_DEADLINE on the same thing with another timestamp, what should happen?

The expectation is that many deadline hints can be set on a fence. The fence signaller should track the soonest deadline.

Quoted 70 lines · On Wed, 22 Feb 2023 07:37:26 -0800 Rob Clark <[email protected]> wrote:

Quoted 63 lines · Pekka Paalanen

Quoted 49 lines · On Tue, 21 Feb 2023 09:53:56 -0800 Rob Clark <[email protected]> wrote:

Quoted 40 lines · Luben Tuikov

Quoted 32 lines · Rob Clark

Quoted 14 lines · Pekka Paalanen

Quoted 10 lines · On Sat, 18 Feb 2023 13:15:49 -0800 Rob Clark <[email protected]> wrote:

From: Rob Clark <[email protected]>

Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent wait (as opposed to a "housekeeping" wait to know when to cleanup after some work has completed). Usermode components of GPU driver stacks often poll() on fence fd's to know when it is safe to do things like free or reuse a buffer, but they can also poll() on a fence fd when waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag lets the kernel differentiate these two cases.

Quoted 6 lines

Quoted 1 line

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

Hi,

where would the UAPI documentation of this go?
It seems to be missing.

Good question, I am not sure. The poll() man page has a description, but my usage doesn't fit that _exactly_ (but OTOH the description is a bit vague).

Quoted 5 lines

If a Wayland compositor is polling application fences to know which client buffer to use in its rendering, should the compositor poll with PRI or not? If a compositor polls with PRI, then all fences from all applications would always be PRI. Would that be harmful somehow or would it be beneficial?

Quoted 6 lines

I think a compositor would rather use the deadline ioctl and then poll without PRI. Otherwise you are giving an urgency signal to the fence signaller which might not necessarily be needed.

The places where I expect PRI to be useful is more in mesa (things like glFinish(), readpix, and other similar sorts of blocking APIs)

Quoted 7 lines

Hi,

Hmm, but then user-space could do the opposite, namely, submit work as usual--never using the SET_DEADLINE ioctl, and then at the end, poll using (E)POLLPRI. That seems like a possible usage pattern, unintended--maybe, but possible. Do we want to discourage this? Wouldn't SET_DEADLINE be enough? I mean, one can call SET_DEADLINE with the current time, and then wouldn't that be equivalent to (E)POLLPRI?

Quoted 4 lines

Yeah, (E)POLLPRI isn't strictly needed if we have SET_DEADLINE. It is slightly more convenient if you want an immediate deadline (single syscall instead of two), but not strictly needed. OTOH it piggy-backs on existing UABI.

In that case, I would be conservative, and not add the POLLPRI semantics. An UAPI addition that is not strictly needed and somewhat unclear if it violates any design principles is best not done, until it is proven to be beneficial.

Quoted 5 lines

Besides, a Wayland compositor does not necessary need to add the fd to its main event loop for poll. It could just SET_DEADLINE, and then when it renders simply check if the fence passed or not already. Not polling means the compositor does not need to wake up at the moment the fence signals to just record a flag.

Quoted 3 lines

poll(POLLPRI) isn't intended for wayland.. but is a thing I want in mesa for fence waits. I _could_ use SET_DEADLINE but it is two syscalls and correspondingly more code ;-)

But is it actually beneficial? "More code" seems quite irrelevant.

Would there be a hundred or more of those per frame? Or would it be always limited to one or two? Or totally depend on what the application is doing? Is it a significant impact?

Quoted 6 lines

Quoted 3 lines

On another matter, if the application uses SET_DEADLINE with one timestamp, and the compositor uses SET_DEADLINE on the same thing with another timestamp, what should happen?

The expectation is that many deadline hints can be set on a fence. The fence signaller should track the soonest deadline.

You need to document that as UAPI, since it is observable to userspace. It would be bad if drivers or subsystems would differ in behaviour.

Thanks,
pq

Quoted 5 lines · Pekka Paalanen

Quoted 1 line · On Wed, 22 Feb 2023 07:37:26 -0800 Rob Clark <[email protected]> wrote:

On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen <[email protected]> wrote:

Quoted 10 lines

Quoted 6 lines

Quoted 3 lines

On another matter, if the application uses SET_DEADLINE with one timestamp, and the compositor uses SET_DEADLINE on the same thing with another timestamp, what should happen?

The expectation is that many deadline hints can be set on a fence. The fence signaller should track the soonest deadline.

You need to document that as UAPI, since it is observable to userspace. It would be bad if drivers or subsystems would differ in behaviour.

It is in the end a hint. It is about giving the driver more information so that it can make better choices. But the driver is even free to ignore it. So maybe "expectation" is too strong of a word. Rather, any other behavior doesn't really make sense. But it could end up being dictated by how the hw and/or fw works.

Quoted 6 lines · On Thu, 23 Feb 2023 10:51:48 -0800 Rob Clark <[email protected]> wrote:

Quoted 5 lines · Pekka Paalanen

Quoted 1 line · On Wed, 22 Feb 2023 07:37:26 -0800 Rob Clark <[email protected]> wrote:

On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen <[email protected]> wrote:

...

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

Quoted 3 lines

On another matter, if the application uses SET_DEADLINE with one timestamp, and the compositor uses SET_DEADLINE on the same thing with another timestamp, what should happen?

The expectation is that many deadline hints can be set on a fence. The fence signaller should track the soonest deadline.

You need to document that as UAPI, since it is observable to userspace. It would be bad if drivers or subsystems would differ in behaviour.

Quoted 5 lines

It is in the end a hint. It is about giving the driver more information so that it can make better choices. But the driver is even free to ignore it. So maybe "expectation" is too strong of a word. Rather, any other behavior doesn't really make sense. But it could end up being dictated by how the hw and/or fw works.

It will stop being a hint once it has been implemented and used in the wild long enough. The kernel userspace regression rules make sure of that.

See the topic of implementing triple-buffering in Mutter in order to put more work to the GPU in order to have the GPU ramp up clocks in order to not miss rendering deadlines. I don't think that patch set has landed in Mutter upstream, but I hear distributions in downstream are already carrying it.

https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1383
https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1441

Granted, GPU clocks are just one side of that story it seems, and triple-buffering may have other benefits.

If SET_DEADLINE would fix that problem without triple-buffering, it is definitely userspace observable, expected and eventually required behaviour.

Thanks,
pq

Quoted 29 lines · Pekka Paalanen

Quoted 6 lines · On Thu, 23 Feb 2023 10:51:48 -0800 Rob Clark <[email protected]> wrote:

Quoted 5 lines · Pekka Paalanen

Quoted 1 line · On Wed, 22 Feb 2023 07:37:26 -0800 Rob Clark <[email protected]> wrote:

On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen <[email protected]> wrote:

...

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

Quoted 3 lines

On another matter, if the application uses SET_DEADLINE with one timestamp, and the compositor uses SET_DEADLINE on the same thing with another timestamp, what should happen?

The expectation is that many deadline hints can be set on a fence. The fence signaller should track the soonest deadline.

You need to document that as UAPI, since it is observable to userspace. It would be bad if drivers or subsystems would differ in behaviour.

Quoted 5 lines

It is in the end a hint. It is about giving the driver more information so that it can make better choices. But the driver is even free to ignore it. So maybe "expectation" is too strong of a word. Rather, any other behavior doesn't really make sense. But it could end up being dictated by how the hw and/or fw works.

Quoted 3 lines

It will stop being a hint once it has been implemented and used in the wild long enough. The kernel userspace regression rules make sure of that.

Yeah, tricky and maybe a gray area in this case. I think we eluded elsewhere in the thread that renaming the thing might be an option.

So maybe instead of deadline, which is a very strong word, use something along the lines of "present time hint", or "signalled time hint"? Maybe reads clumsy. Just throwing some ideas for a start.

Quoted 34 lines · On Fri, 24 Feb 2023 09:41:46 +0000 Tvrtko Ursulin…

Quoted 29 lines · Pekka Paalanen

Quoted 6 lines · On Thu, 23 Feb 2023 10:51:48 -0800 Rob Clark <[email protected]> wrote:

Quoted 5 lines · Pekka Paalanen

Quoted 1 line · On Wed, 22 Feb 2023 07:37:26 -0800 Rob Clark <[email protected]> wrote:

On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen <[email protected]> wrote:

...

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

Quoted 3 lines

On another matter, if the application uses SET_DEADLINE with one timestamp, and the compositor uses SET_DEADLINE on the same thing with another timestamp, what should happen?

The expectation is that many deadline hints can be set on a fence. The fence signaller should track the soonest deadline.

You need to document that as UAPI, since it is observable to userspace. It would be bad if drivers or subsystems would differ in behaviour.

Quoted 5 lines

It is in the end a hint. It is about giving the driver more information so that it can make better choices. But the driver is even free to ignore it. So maybe "expectation" is too strong of a word. Rather, any other behavior doesn't really make sense. But it could end up being dictated by how the hw and/or fw works.

Quoted 3 lines

It will stop being a hint once it has been implemented and used in the wild long enough. The kernel userspace regression rules make sure of that.

Quoted 6 lines

Yeah, tricky and maybe a gray area in this case. I think we eluded elsewhere in the thread that renaming the thing might be an option.

So maybe instead of deadline, which is a very strong word, use something along the lines of "present time hint", or "signalled time hint"? Maybe reads clumsy. Just throwing some ideas for a start.

You can try, but I fear that if it ever changes behaviour and someone notices that, it's labelled as a kernel regression. I don't think documentation has ever been the authoritative definition of UABI in Linux, it just guides drivers and userspace towards a common understanding and common usage patterns.

So even if the UABI contract is not documented (ugh), you need to be prepared to set the UABI contract through kernel implementation.

If you do not document the UABI contract, then different drivers are likely to implement it differently, leading to differing behaviour. Also userspace will invent wild ways to abuse the UABI if there is no documentation guiding it on proper use. If userspace or end users observe different behaviour, that's bad even if it's not a regression.

I don't like the situation either, but it is what it is. UABI stability trumps everything regardless of whether it was documented or not.

I bet userspace is going to use this as a "make it faster, make it hotter" button. I would not be surprised if someone wrote a LD_PRELOAD library that stamps any and all fences with an expired deadline to just squeeze out a little more through some weird side-effect.

Well, that's hopefully overboard in scaring, but in the end, I would like to see UABI documented so I can have a feeling of what it is for and how it was intended to be used. That's all.

Quoted 44 lines · Pekka Paalanen

Quoted 34 lines · On Fri, 24 Feb 2023 09:41:46 +0000 Tvrtko Ursulin…

Quoted 29 lines · Pekka Paalanen

Quoted 6 lines · On Thu, 23 Feb 2023 10:51:48 -0800 Rob Clark <[email protected]> wrote:

Quoted 5 lines · Pekka Paalanen

Quoted 1 line · On Wed, 22 Feb 2023 07:37:26 -0800 Rob Clark <[email protected]> wrote:

On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen <[email protected]> wrote:

...

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

Quoted 3 lines

On another matter, if the application uses SET_DEADLINE with one timestamp, and the compositor uses SET_DEADLINE on the same thing with another timestamp, what should happen?

The expectation is that many deadline hints can be set on a fence. The fence signaller should track the soonest deadline.

You need to document that as UAPI, since it is observable to userspace. It would be bad if drivers or subsystems would differ in behaviour.

Quoted 5 lines

It is in the end a hint. It is about giving the driver more information so that it can make better choices. But the driver is even free to ignore it. So maybe "expectation" is too strong of a word. Rather, any other behavior doesn't really make sense. But it could end up being dictated by how the hw and/or fw works.

Quoted 3 lines

It will stop being a hint once it has been implemented and used in the wild long enough. The kernel userspace regression rules make sure of that.

Quoted 6 lines

Yeah, tricky and maybe a gray area in this case. I think we eluded elsewhere in the thread that renaming the thing might be an option.

So maybe instead of deadline, which is a very strong word, use something along the lines of "present time hint", or "signalled time hint"? Maybe reads clumsy. Just throwing some ideas for a start.

Quoted 8 lines

You can try, but I fear that if it ever changes behaviour and someone notices that, it's labelled as a kernel regression. I don't think documentation has ever been the authoritative definition of UABI in Linux, it just guides drivers and userspace towards a common understanding and common usage patterns.

So even if the UABI contract is not documented (ugh), you need to be prepared to set the UABI contract through kernel implementation.

To be the devil's advocate it probably wouldn't be an ABI regression but just an regression. Same way as what nice(2) priorities mean hasn't always been the same over the years, I don't think there is a strict contract.

Having said that, it may be different with latency sensitive stuff such as UIs though since it is very observable and can be very painful to users.

Quoted 9 lines

If you do not document the UABI contract, then different drivers are likely to implement it differently, leading to differing behaviour. Also userspace will invent wild ways to abuse the UABI if there is no documentation guiding it on proper use. If userspace or end users observe different behaviour, that's bad even if it's not a regression.

I don't like the situation either, but it is what it is. UABI stability trumps everything regardless of whether it was documented or not.

Quoted 8 lines

I bet userspace is going to use this as a "make it faster, make it hotter" button. I would not be surprised if someone wrote a LD_PRELOAD library that stamps any and all fences with an expired deadline to just squeeze out a little more through some weird side-effect.

Well, that's hopefully overboard in scaring, but in the end, I would like to see UABI documented so I can have a feeling of what it is for and how it was intended to be used. That's all.

We share the same concern. If you read elsewhere in these threads you will notice I have been calling this an "arms race". If the ability to make yourself go faster does not required additional privilege I also worry everyone will do it at which point it becomes pointless. So yes, I do share this concern about exposing any of this as an unprivileged uapi.

Is it possible to limit access to only compositors in some sane way? Sounds tricky when dma-fence should be disconnected from DRM..

Quoted 86 lines · On Fri, 24 Feb 2023 10:50:51 +0000 Tvrtko Ursulin…

Quoted 44 lines · Pekka Paalanen

Quoted 34 lines · On Fri, 24 Feb 2023 09:41:46 +0000 Tvrtko Ursulin…

Quoted 29 lines · Pekka Paalanen

Quoted 6 lines · On Thu, 23 Feb 2023 10:51:48 -0800 Rob Clark <[email protected]> wrote:

Quoted 5 lines · Pekka Paalanen

Quoted 1 line · On Wed, 22 Feb 2023 07:37:26 -0800 Rob Clark <[email protected]> wrote:

On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen <[email protected]> wrote:

...

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

Quoted 3 lines

On another matter, if the application uses SET_DEADLINE with one timestamp, and the compositor uses SET_DEADLINE on the same thing with another timestamp, what should happen?

The expectation is that many deadline hints can be set on a fence. The fence signaller should track the soonest deadline.

You need to document that as UAPI, since it is observable to userspace. It would be bad if drivers or subsystems would differ in behaviour.

Quoted 5 lines

It is in the end a hint. It is about giving the driver more information so that it can make better choices. But the driver is even free to ignore it. So maybe "expectation" is too strong of a word. Rather, any other behavior doesn't really make sense. But it could end up being dictated by how the hw and/or fw works.

Quoted 3 lines

It will stop being a hint once it has been implemented and used in the wild long enough. The kernel userspace regression rules make sure of that.

Quoted 6 lines

Yeah, tricky and maybe a gray area in this case. I think we eluded elsewhere in the thread that renaming the thing might be an option.

So maybe instead of deadline, which is a very strong word, use something along the lines of "present time hint", or "signalled time hint"? Maybe reads clumsy. Just throwing some ideas for a start.

Quoted 8 lines

You can try, but I fear that if it ever changes behaviour and someone notices that, it's labelled as a kernel regression. I don't think documentation has ever been the authoritative definition of UABI in Linux, it just guides drivers and userspace towards a common understanding and common usage patterns.

So even if the UABI contract is not documented (ugh), you need to be prepared to set the UABI contract through kernel implementation.

To be the devil's advocate it probably wouldn't be an ABI regression but just an regression. Same way as what nice(2) priorities mean hasn't always been the same over the years, I don't think there is a strict contract.

Having said that, it may be different with latency sensitive stuff such as UIs though since it is very observable and can be very painful to users.

Quoted 9 lines

If you do not document the UABI contract, then different drivers are likely to implement it differently, leading to differing behaviour. Also userspace will invent wild ways to abuse the UABI if there is no documentation guiding it on proper use. If userspace or end users observe different behaviour, that's bad even if it's not a regression.

I don't like the situation either, but it is what it is. UABI stability trumps everything regardless of whether it was documented or not.

Quoted 8 lines

I bet userspace is going to use this as a "make it faster, make it hotter" button. I would not be surprised if someone wrote a LD_PRELOAD library that stamps any and all fences with an expired deadline to just squeeze out a little more through some weird side-effect.

Well, that's hopefully overboard in scaring, but in the end, I would like to see UABI documented so I can have a feeling of what it is for and how it was intended to be used. That's all.

We share the same concern. If you read elsewhere in these threads you will notice I have been calling this an "arms race". If the ability to make yourself go faster does not required additional privilege I also worry everyone will do it at which point it becomes pointless. So yes, I do share this concern about exposing any of this as an unprivileged uapi.

Quoted 2 lines

Is it possible to limit access to only compositors in some sane way? Sounds tricky when dma-fence should be disconnected from DRM..

Maybe it's not that bad in this particular case, because we are talking only about boosting GPU clocks which benefits everyone (except battery life) and it does not penalize other programs like e.g. job priorities do.

Drivers are not going to use the deadline for scheduling priorities, right? I don't recall seeing any mention of that.

...right?

Quoted 89 lines · Pekka Paalanen

Quoted 86 lines · On Fri, 24 Feb 2023 10:50:51 +0000 Tvrtko Ursulin…

Quoted 44 lines · Pekka Paalanen

Quoted 34 lines · On Fri, 24 Feb 2023 09:41:46 +0000 Tvrtko Ursulin…

Quoted 29 lines · Pekka Paalanen

Quoted 6 lines · On Thu, 23 Feb 2023 10:51:48 -0800 Rob Clark <[email protected]> wrote:

Quoted 5 lines · Pekka Paalanen

Quoted 1 line · On Wed, 22 Feb 2023 07:37:26 -0800 Rob Clark <[email protected]> wrote:

On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen <[email protected]> wrote:

...

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

Quoted 3 lines

On another matter, if the application uses SET_DEADLINE with one timestamp, and the compositor uses SET_DEADLINE on the same thing with another timestamp, what should happen?

The expectation is that many deadline hints can be set on a fence. The fence signaller should track the soonest deadline.

You need to document that as UAPI, since it is observable to userspace. It would be bad if drivers or subsystems would differ in behaviour.

Quoted 5 lines

It is in the end a hint. It is about giving the driver more information so that it can make better choices. But the driver is even free to ignore it. So maybe "expectation" is too strong of a word. Rather, any other behavior doesn't really make sense. But it could end up being dictated by how the hw and/or fw works.

Quoted 3 lines

It will stop being a hint once it has been implemented and used in the wild long enough. The kernel userspace regression rules make sure of that.

Quoted 6 lines

Yeah, tricky and maybe a gray area in this case. I think we eluded elsewhere in the thread that renaming the thing might be an option.

So maybe instead of deadline, which is a very strong word, use something along the lines of "present time hint", or "signalled time hint"? Maybe reads clumsy. Just throwing some ideas for a start.

Quoted 8 lines

You can try, but I fear that if it ever changes behaviour and someone notices that, it's labelled as a kernel regression. I don't think documentation has ever been the authoritative definition of UABI in Linux, it just guides drivers and userspace towards a common understanding and common usage patterns.

So even if the UABI contract is not documented (ugh), you need to be prepared to set the UABI contract through kernel implementation.

To be the devil's advocate it probably wouldn't be an ABI regression but just an regression. Same way as what nice(2) priorities mean hasn't always been the same over the years, I don't think there is a strict contract.

Having said that, it may be different with latency sensitive stuff such as UIs though since it is very observable and can be very painful to users.

Quoted 9 lines

If you do not document the UABI contract, then different drivers are likely to implement it differently, leading to differing behaviour. Also userspace will invent wild ways to abuse the UABI if there is no documentation guiding it on proper use. If userspace or end users observe different behaviour, that's bad even if it's not a regression.

I don't like the situation either, but it is what it is. UABI stability trumps everything regardless of whether it was documented or not.

Quoted 8 lines

I bet userspace is going to use this as a "make it faster, make it hotter" button. I would not be surprised if someone wrote a LD_PRELOAD library that stamps any and all fences with an expired deadline to just squeeze out a little more through some weird side-effect.

Well, that's hopefully overboard in scaring, but in the end, I would like to see UABI documented so I can have a feeling of what it is for and how it was intended to be used. That's all.

We share the same concern. If you read elsewhere in these threads you will notice I have been calling this an "arms race". If the ability to make yourself go faster does not required additional privilege I also worry everyone will do it at which point it becomes pointless. So yes, I do share this concern about exposing any of this as an unprivileged uapi.

Quoted 7 lines

Quoted 2 lines

Is it possible to limit access to only compositors in some sane way? Sounds tricky when dma-fence should be disconnected from DRM..

Maybe it's not that bad in this particular case, because we are talking only about boosting GPU clocks which benefits everyone (except battery life) and it does not penalize other programs like e.g. job priorities do.

Apart from efficiency that you mentioned, which does not always favor higher clocks, sometimes thermal budget is also shared between CPU and GPU. So more GPU clocks can mean fewer CPU clocks. It's really hard to make optimal choices without the full coordination between both schedulers.

But that is even not the main point, which is that if everyone sets the immediate deadline then having the deadline API is a bit pointless. For instance there is a reason negative nice needs CAP_SYS_ADMIN.

However Rob has also pointed out the existence of uclamp.min via sched_setattr which is unprivileged and can influence frequency selection in the CPU world, so I conceded on that point. If CPU world has accepted it so can we I guess.

So IMO we are back to whether we can agree defining it is a hint is good enough, be in via the name of the ioctl/flag itself or via documentation.

Quoted 4 lines

Drivers are not going to use the deadline for scheduling priorities, right? I don't recall seeing any mention of that.

...right?

I wouldn't have thought it would be beneficial to preclude that, or assume what drivers would do with the info to begin with.

For instance in i915 we almost had a deadline based scheduler which was much fairer than the current priority sorted fifo and in an ideal world we would either revive or re-implement that idea. In which case considering the fence deadline would naturally slot in and give true integration with compositor deadlines (not just boost clocks and pray it helps).

Quoted 124 lines · Tvrtko Ursulin

Quoted 89 lines · Pekka Paalanen

Quoted 86 lines · On Fri, 24 Feb 2023 10:50:51 +0000 Tvrtko Ursulin…

Quoted 44 lines · Pekka Paalanen

Quoted 34 lines · On Fri, 24 Feb 2023 09:41:46 +0000 Tvrtko Ursulin…

Quoted 29 lines · Pekka Paalanen

Quoted 6 lines · On Thu, 23 Feb 2023 10:51:48 -0800 Rob Clark <[email protected]> wrote:

Quoted 5 lines · Pekka Paalanen

Quoted 1 line · On Wed, 22 Feb 2023 07:37:26 -0800 Rob Clark <[email protected]> wrote:

On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen <[email protected]> wrote:

...

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

Quoted 3 lines

On another matter, if the application uses SET_DEADLINE with one timestamp, and the compositor uses SET_DEADLINE on the same thing with another timestamp, what should happen?

The expectation is that many deadline hints can be set on a fence. The fence signaller should track the soonest deadline.

You need to document that as UAPI, since it is observable to userspace. It would be bad if drivers or subsystems would differ in behaviour.

Quoted 5 lines

It is in the end a hint. It is about giving the driver more information so that it can make better choices. But the driver is even free to ignore it. So maybe "expectation" is too strong of a word. Rather, any other behavior doesn't really make sense. But it could end up being dictated by how the hw and/or fw works.

Quoted 3 lines

It will stop being a hint once it has been implemented and used in the wild long enough. The kernel userspace regression rules make sure of that.

Quoted 6 lines

Yeah, tricky and maybe a gray area in this case. I think we eluded elsewhere in the thread that renaming the thing might be an option.

So maybe instead of deadline, which is a very strong word, use something along the lines of "present time hint", or "signalled time hint"? Maybe reads clumsy. Just throwing some ideas for a start.

Quoted 8 lines

You can try, but I fear that if it ever changes behaviour and someone notices that, it's labelled as a kernel regression. I don't think documentation has ever been the authoritative definition of UABI in Linux, it just guides drivers and userspace towards a common understanding and common usage patterns.

So even if the UABI contract is not documented (ugh), you need to be prepared to set the UABI contract through kernel implementation.

To be the devil's advocate it probably wouldn't be an ABI regression but just an regression. Same way as what nice(2) priorities mean hasn't always been the same over the years, I don't think there is a strict contract.

Having said that, it may be different with latency sensitive stuff such as UIs though since it is very observable and can be very painful to users.

Quoted 9 lines

If you do not document the UABI contract, then different drivers are likely to implement it differently, leading to differing behaviour. Also userspace will invent wild ways to abuse the UABI if there is no documentation guiding it on proper use. If userspace or end users observe different behaviour, that's bad even if it's not a regression.

I don't like the situation either, but it is what it is. UABI stability trumps everything regardless of whether it was documented or not.

Quoted 8 lines

I bet userspace is going to use this as a "make it faster, make it hotter" button. I would not be surprised if someone wrote a LD_PRELOAD library that stamps any and all fences with an expired deadline to just squeeze out a little more through some weird side-effect.

Well, that's hopefully overboard in scaring, but in the end, I would like to see UABI documented so I can have a feeling of what it is for and how it was intended to be used. That's all.

We share the same concern. If you read elsewhere in these threads you will notice I have been calling this an "arms race". If the ability to make yourself go faster does not required additional privilege I also worry everyone will do it at which point it becomes pointless. So yes, I do share this concern about exposing any of this as an unprivileged uapi.

Quoted 7 lines

Quoted 2 lines

Is it possible to limit access to only compositors in some sane way? Sounds tricky when dma-fence should be disconnected from DRM..

Maybe it's not that bad in this particular case, because we are talking only about boosting GPU clocks which benefits everyone (except battery life) and it does not penalize other programs like e.g. job priorities do.

Apart from efficiency that you mentioned, which does not always favor higher clocks, sometimes thermal budget is also shared between CPU and GPU. So more GPU clocks can mean fewer CPU clocks. It's really hard to make optimal choices without the full coordination between both schedulers.

But that is even not the main point, which is that if everyone sets the immediate deadline then having the deadline API is a bit pointless. For instance there is a reason negative nice needs CAP_SYS_ADMIN.

However Rob has also pointed out the existence of uclamp.min via sched_setattr which is unprivileged and can influence frequency selection in the CPU world, so I conceded on that point. If CPU world has accepted it so can we I guess.

So IMO we are back to whether we can agree defining it is a hint is good enough, be in via the name of the ioctl/flag itself or via documentation.

Quoted 4 lines

Drivers are not going to use the deadline for scheduling priorities, right? I don't recall seeing any mention of that.

...right?

I wouldn't have thought it would be beneficial to preclude that, or assume what drivers would do with the info to begin with.

Quoted 6 lines

For instance in i915 we almost had a deadline based scheduler which was much fairer than the current priority sorted fifo and in an ideal world we would either revive or re-implement that idea. In which case considering the fence deadline would naturally slot in and give true integration with compositor deadlines (not just boost clocks and pray it helps).

How is user-space to decide whether to use ioctl(SET_DEADLINE) or poll(POLLPRI)?

--
Regards,
Luben

Quoted 126 lines · Luben Tuikov

Quoted 124 lines · Tvrtko Ursulin

Quoted 89 lines · Pekka Paalanen

Quoted 86 lines · On Fri, 24 Feb 2023 10:50:51 +0000 Tvrtko Ursulin…

Quoted 44 lines · Pekka Paalanen

Quoted 34 lines · On Fri, 24 Feb 2023 09:41:46 +0000 Tvrtko Ursulin…

Quoted 29 lines · Pekka Paalanen

Quoted 6 lines · On Thu, 23 Feb 2023 10:51:48 -0800 Rob Clark <[email protected]> wrote:

Quoted 5 lines · Pekka Paalanen

Quoted 1 line · On Wed, 22 Feb 2023 07:37:26 -0800 Rob Clark <[email protected]> wrote:

On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen <[email protected]> wrote:

...

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

Quoted 3 lines

On another matter, if the application uses SET_DEADLINE with one timestamp, and the compositor uses SET_DEADLINE on the same thing with another timestamp, what should happen?

The expectation is that many deadline hints can be set on a fence. The fence signaller should track the soonest deadline.

You need to document that as UAPI, since it is observable to userspace. It would be bad if drivers or subsystems would differ in behaviour.

Quoted 5 lines

It is in the end a hint. It is about giving the driver more information so that it can make better choices. But the driver is even free to ignore it. So maybe "expectation" is too strong of a word. Rather, any other behavior doesn't really make sense. But it could end up being dictated by how the hw and/or fw works.

Quoted 3 lines

It will stop being a hint once it has been implemented and used in the wild long enough. The kernel userspace regression rules make sure of that.

Quoted 6 lines

Yeah, tricky and maybe a gray area in this case. I think we eluded elsewhere in the thread that renaming the thing might be an option.

So maybe instead of deadline, which is a very strong word, use something along the lines of "present time hint", or "signalled time hint"? Maybe reads clumsy. Just throwing some ideas for a start.

Quoted 8 lines

You can try, but I fear that if it ever changes behaviour and someone notices that, it's labelled as a kernel regression. I don't think documentation has ever been the authoritative definition of UABI in Linux, it just guides drivers and userspace towards a common understanding and common usage patterns.

So even if the UABI contract is not documented (ugh), you need to be prepared to set the UABI contract through kernel implementation.

To be the devil's advocate it probably wouldn't be an ABI regression but just an regression. Same way as what nice(2) priorities mean hasn't always been the same over the years, I don't think there is a strict contract.

Having said that, it may be different with latency sensitive stuff such as UIs though since it is very observable and can be very painful to users.

Quoted 9 lines

If you do not document the UABI contract, then different drivers are likely to implement it differently, leading to differing behaviour. Also userspace will invent wild ways to abuse the UABI if there is no documentation guiding it on proper use. If userspace or end users observe different behaviour, that's bad even if it's not a regression.

I don't like the situation either, but it is what it is. UABI stability trumps everything regardless of whether it was documented or not.

Quoted 8 lines

I bet userspace is going to use this as a "make it faster, make it hotter" button. I would not be surprised if someone wrote a LD_PRELOAD library that stamps any and all fences with an expired deadline to just squeeze out a little more through some weird side-effect.

Well, that's hopefully overboard in scaring, but in the end, I would like to see UABI documented so I can have a feeling of what it is for and how it was intended to be used. That's all.

We share the same concern. If you read elsewhere in these threads you will notice I have been calling this an "arms race". If the ability to make yourself go faster does not required additional privilege I also worry everyone will do it at which point it becomes pointless. So yes, I do share this concern about exposing any of this as an unprivileged uapi.

Quoted 7 lines

Quoted 2 lines

Is it possible to limit access to only compositors in some sane way? Sounds tricky when dma-fence should be disconnected from DRM..

Maybe it's not that bad in this particular case, because we are talking only about boosting GPU clocks which benefits everyone (except battery life) and it does not penalize other programs like e.g. job priorities do.

Apart from efficiency that you mentioned, which does not always favor higher clocks, sometimes thermal budget is also shared between CPU and GPU. So more GPU clocks can mean fewer CPU clocks. It's really hard to make optimal choices without the full coordination between both schedulers.

But that is even not the main point, which is that if everyone sets the immediate deadline then having the deadline API is a bit pointless. For instance there is a reason negative nice needs CAP_SYS_ADMIN.

However Rob has also pointed out the existence of uclamp.min via sched_setattr which is unprivileged and can influence frequency selection in the CPU world, so I conceded on that point. If CPU world has accepted it so can we I guess.

So IMO we are back to whether we can agree defining it is a hint is good enough, be in via the name of the ioctl/flag itself or via documentation.

Quoted 4 lines

Drivers are not going to use the deadline for scheduling priorities, right? I don't recall seeing any mention of that.

...right?

I wouldn't have thought it would be beneficial to preclude that, or assume what drivers would do with the info to begin with.

Quoted 8 lines

Quoted 6 lines

For instance in i915 we almost had a deadline based scheduler which was much fairer than the current priority sorted fifo and in an ideal world we would either revive or re-implement that idea. In which case considering the fence deadline would naturally slot in and give true integration with compositor deadlines (not just boost clocks and pray it helps).

How is user-space to decide whether to use ioctl(SET_DEADLINE) or poll(POLLPRI)?

Implementation of blocking gl/vk/cl APIs, like glFinish() would use poll(POLLPRI). It could also set an immediate deadline and then call poll() without POLLPRI.

Other than compositors which do frame-pacing I expect the main usage of either of these is mesa.

BR,
-R

Quoted 4 lines

--
Regards,
Luben

Quoted 136 lines · Rob Clark

Quoted 126 lines · Luben Tuikov

Quoted 124 lines · Tvrtko Ursulin

Quoted 89 lines · Pekka Paalanen

Quoted 86 lines · On Fri, 24 Feb 2023 10:50:51 +0000 Tvrtko Ursulin…

Quoted 44 lines · Pekka Paalanen

Quoted 34 lines · On Fri, 24 Feb 2023 09:41:46 +0000 Tvrtko Ursulin…

Quoted 29 lines · Pekka Paalanen

Quoted 6 lines · On Thu, 23 Feb 2023 10:51:48 -0800 Rob Clark <[email protected]> wrote:

Quoted 5 lines · Pekka Paalanen

Quoted 1 line · On Wed, 22 Feb 2023 07:37:26 -0800 Rob Clark <[email protected]> wrote:

On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen <[email protected]> wrote:

...

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

Quoted 3 lines

On another matter, if the application uses SET_DEADLINE with one
timestamp, and the compositor uses SET_DEADLINE on the same thing with
another timestamp, what should happen?

The expectation is that many deadline hints can be set on a fence. The fence signaller should track the soonest deadline.

You need to document that as UAPI, since it is observable to userspace. It would be bad if drivers or subsystems would differ in behaviour.

Quoted 5 lines

It is in the end a hint. It is about giving the driver more information so that it can make better choices. But the driver is even free to ignore it. So maybe "expectation" is too strong of a word. Rather, any other behavior doesn't really make sense. But it could end up being dictated by how the hw and/or fw works.

Quoted 3 lines

It will stop being a hint once it has been implemented and used in the wild long enough. The kernel userspace regression rules make sure of that.

Quoted 6 lines

Yeah, tricky and maybe a gray area in this case. I think we eluded elsewhere in the thread that renaming the thing might be an option.

So maybe instead of deadline, which is a very strong word, use something along the lines of "present time hint", or "signalled time hint"? Maybe reads clumsy. Just throwing some ideas for a start.

Quoted 8 lines

You can try, but I fear that if it ever changes behaviour and someone notices that, it's labelled as a kernel regression. I don't think documentation has ever been the authoritative definition of UABI in Linux, it just guides drivers and userspace towards a common understanding and common usage patterns.

So even if the UABI contract is not documented (ugh), you need to be prepared to set the UABI contract through kernel implementation.

To be the devil's advocate it probably wouldn't be an ABI regression but just an regression. Same way as what nice(2) priorities mean hasn't always been the same over the years, I don't think there is a strict contract.

Having said that, it may be different with latency sensitive stuff such as UIs though since it is very observable and can be very painful to users.

Quoted 9 lines

If you do not document the UABI contract, then different drivers are likely to implement it differently, leading to differing behaviour. Also userspace will invent wild ways to abuse the UABI if there is no documentation guiding it on proper use. If userspace or end users observe different behaviour, that's bad even if it's not a regression.

I don't like the situation either, but it is what it is. UABI stability trumps everything regardless of whether it was documented or not.

Quoted 8 lines

I bet userspace is going to use this as a "make it faster, make it hotter" button. I would not be surprised if someone wrote a LD_PRELOAD library that stamps any and all fences with an expired deadline to just squeeze out a little more through some weird side-effect.

Well, that's hopefully overboard in scaring, but in the end, I would like to see UABI documented so I can have a feeling of what it is for and how it was intended to be used. That's all.

We share the same concern. If you read elsewhere in these threads you will notice I have been calling this an "arms race". If the ability to make yourself go faster does not required additional privilege I also worry everyone will do it at which point it becomes pointless. So yes, I do share this concern about exposing any of this as an unprivileged uapi.

Quoted 7 lines

Quoted 2 lines

Is it possible to limit access to only compositors in some sane way? Sounds tricky when dma-fence should be disconnected from DRM..

Maybe it's not that bad in this particular case, because we are talking only about boosting GPU clocks which benefits everyone (except battery life) and it does not penalize other programs like e.g. job priorities do.

Apart from efficiency that you mentioned, which does not always favor higher clocks, sometimes thermal budget is also shared between CPU and GPU. So more GPU clocks can mean fewer CPU clocks. It's really hard to make optimal choices without the full coordination between both schedulers.

But that is even not the main point, which is that if everyone sets the immediate deadline then having the deadline API is a bit pointless. For instance there is a reason negative nice needs CAP_SYS_ADMIN.

However Rob has also pointed out the existence of uclamp.min via sched_setattr which is unprivileged and can influence frequency selection in the CPU world, so I conceded on that point. If CPU world has accepted it so can we I guess.

So IMO we are back to whether we can agree defining it is a hint is good enough, be in via the name of the ioctl/flag itself or via documentation.

Quoted 4 lines

Drivers are not going to use the deadline for scheduling priorities, right? I don't recall seeing any mention of that.

...right?

I wouldn't have thought it would be beneficial to preclude that, or assume what drivers would do with the info to begin with.

Quoted 8 lines

Quoted 6 lines

For instance in i915 we almost had a deadline based scheduler which was much fairer than the current priority sorted fifo and in an ideal world we would either revive or re-implement that idea. In which case considering the fence deadline would naturally slot in and give true integration with compositor deadlines (not just boost clocks and pray it helps).

How is user-space to decide whether to use ioctl(SET_DEADLINE) or poll(POLLPRI)?

Quoted 6 lines

Implementation of blocking gl/vk/cl APIs, like glFinish() would use poll(POLLPRI). It could also set an immediate deadline and then call poll() without POLLPRI.

Other than compositors which do frame-pacing I expect the main usage of either of these is mesa.

Okay, so it looks like we already agreed that having a way to bump frequency from userspace is acceptable. either because there are already other ways that you can waste power and because this already acceptable in the CPU world.

But why we are doing this in hidden ways then?

Why can't we have this hint per context that is getting executed? (either with a boost-context flag or with some low/med/max or '-1' to '1' value like the latency priority)?

I don't like the waitboost because this heurisitic fails in some media cases. I don't like the global setting because we might be alternating a top-priority with low-priority cases...

So, why not something per context in execution?

Quoted 156 lines · Rodrigo Vivi

Quoted 136 lines · Rob Clark

Quoted 126 lines · Luben Tuikov

Quoted 124 lines · Tvrtko Ursulin

Quoted 89 lines · Pekka Paalanen

Quoted 86 lines · On Fri, 24 Feb 2023 10:50:51 +0000 Tvrtko Ursulin…

Quoted 44 lines · Pekka Paalanen

Quoted 34 lines · On Fri, 24 Feb 2023 09:41:46 +0000 Tvrtko Ursulin…

Quoted 29 lines · Pekka Paalanen

Quoted 6 lines · On Thu, 23 Feb 2023 10:51:48 -0800 Rob Clark <[email protected]> wrote:

Quoted 5 lines · Pekka Paalanen

Quoted 1 line · On Wed, 22 Feb 2023 07:37:26 -0800 Rob Clark <[email protected]> wrote:

On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen <[email protected]> wrote:

...

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

On another matter, if the application uses SET_DEADLINE with one
timestamp, and the compositor uses SET_DEADLINE on the same thing with
another timestamp, what should happen?

The expectation is that many deadline hints can be set on a fence.
The fence signaller should track the soonest deadline.

You need to document that as UAPI, since it is observable to userspace. It would be bad if drivers or subsystems would differ in behaviour.

Quoted 5 lines

It is in the end a hint. It is about giving the driver more information so that it can make better choices. But the driver is even free to ignore it. So maybe "expectation" is too strong of a word. Rather, any other behavior doesn't really make sense. But it could end up being dictated by how the hw and/or fw works.

Quoted 3 lines

It will stop being a hint once it has been implemented and used in the wild long enough. The kernel userspace regression rules make sure of that.

Quoted 6 lines

Yeah, tricky and maybe a gray area in this case. I think we eluded elsewhere in the thread that renaming the thing might be an option.

So maybe instead of deadline, which is a very strong word, use something along the lines of "present time hint", or "signalled time hint"? Maybe reads clumsy. Just throwing some ideas for a start.

Quoted 8 lines

You can try, but I fear that if it ever changes behaviour and someone notices that, it's labelled as a kernel regression. I don't think documentation has ever been the authoritative definition of UABI in Linux, it just guides drivers and userspace towards a common understanding and common usage patterns.

So even if the UABI contract is not documented (ugh), you need to be prepared to set the UABI contract through kernel implementation.

To be the devil's advocate it probably wouldn't be an ABI regression but just an regression. Same way as what nice(2) priorities mean hasn't always been the same over the years, I don't think there is a strict contract.

Having said that, it may be different with latency sensitive stuff such as UIs though since it is very observable and can be very painful to users.

Quoted 9 lines

If you do not document the UABI contract, then different drivers are likely to implement it differently, leading to differing behaviour. Also userspace will invent wild ways to abuse the UABI if there is no documentation guiding it on proper use. If userspace or end users observe different behaviour, that's bad even if it's not a regression.

I don't like the situation either, but it is what it is. UABI stability trumps everything regardless of whether it was documented or not.

Quoted 8 lines

I bet userspace is going to use this as a "make it faster, make it hotter" button. I would not be surprised if someone wrote a LD_PRELOAD library that stamps any and all fences with an expired deadline to just squeeze out a little more through some weird side-effect.

Well, that's hopefully overboard in scaring, but in the end, I would like to see UABI documented so I can have a feeling of what it is for and how it was intended to be used. That's all.

We share the same concern. If you read elsewhere in these threads you will notice I have been calling this an "arms race". If the ability to make yourself go faster does not required additional privilege I also worry everyone will do it at which point it becomes pointless. So yes, I do share this concern about exposing any of this as an unprivileged uapi.

Quoted 7 lines

Quoted 2 lines

Is it possible to limit access to only compositors in some sane way? Sounds tricky when dma-fence should be disconnected from DRM..

Maybe it's not that bad in this particular case, because we are talking only about boosting GPU clocks which benefits everyone (except battery life) and it does not penalize other programs like e.g. job priorities do.

Apart from efficiency that you mentioned, which does not always favor higher clocks, sometimes thermal budget is also shared between CPU and GPU. So more GPU clocks can mean fewer CPU clocks. It's really hard to make optimal choices without the full coordination between both schedulers.

But that is even not the main point, which is that if everyone sets the immediate deadline then having the deadline API is a bit pointless. For instance there is a reason negative nice needs CAP_SYS_ADMIN.

However Rob has also pointed out the existence of uclamp.min via sched_setattr which is unprivileged and can influence frequency selection in the CPU world, so I conceded on that point. If CPU world has accepted it so can we I guess.

So IMO we are back to whether we can agree defining it is a hint is good enough, be in via the name of the ioctl/flag itself or via documentation.

Quoted 4 lines

Drivers are not going to use the deadline for scheduling priorities, right? I don't recall seeing any mention of that.

...right?

I wouldn't have thought it would be beneficial to preclude that, or assume what drivers would do with the info to begin with.

Quoted 8 lines

Quoted 6 lines

For instance in i915 we almost had a deadline based scheduler which was much fairer than the current priority sorted fifo and in an ideal world we would either revive or re-implement that idea. In which case considering the fence deadline would naturally slot in and give true integration with compositor deadlines (not just boost clocks and pray it helps).

How is user-space to decide whether to use ioctl(SET_DEADLINE) or poll(POLLPRI)?

Quoted 6 lines

Implementation of blocking gl/vk/cl APIs, like glFinish() would use poll(POLLPRI). It could also set an immediate deadline and then call poll() without POLLPRI.

Other than compositors which do frame-pacing I expect the main usage of either of these is mesa.

Okay, so it looks like we already agreed that having a way to bump frequency from userspace is acceptable. either because there are already other ways that you can waste power and because this already acceptable in the CPU world.

But why we are doing this in hidden ways then?

Why can't we have this hint per context that is getting executed? (either with a boost-context flag or with some low/med/max or '-1' to '1' value like the latency priority)?

Quoted 6 lines

I don't like the waitboost because this heurisitic fails in some media cases. I don't like the global setting because we might be alternating a top-priority with low-priority cases...

So, why not something per context in execution?

It needs to be finer granularity than per-context, because not all waits should trigger boosting. For example, virglrenderer ends up with a thread polling unsignaled fences to know when to signal an interrupt to the guest virtgpu. This alone shouldn't trigger boosting. (We also wouldn't want to completely disable boosting for virglrenderer.) Or the usermode driver could be waiting on a fence to know when to do some cleanup.

That is not to say that there isn't room for per-context flags to disable/enable boosting for fences created by that context, meaning it could be an AND operation for i915 if it needs to be.

BR,
-R

Quoted 173 lines · Rob Clark

Quoted 156 lines · Rodrigo Vivi

Quoted 136 lines · Rob Clark

Quoted 126 lines · Luben Tuikov

Quoted 124 lines · Tvrtko Ursulin

Quoted 89 lines · Pekka Paalanen

Quoted 86 lines · On Fri, 24 Feb 2023 10:50:51 +0000 Tvrtko Ursulin…

Quoted 44 lines · Pekka Paalanen

Quoted 34 lines · On Fri, 24 Feb 2023 09:41:46 +0000 Tvrtko Ursulin…

Quoted 29 lines · Pekka Paalanen

Quoted 6 lines · On Thu, 23 Feb 2023 10:51:48 -0800 Rob Clark <[email protected]> wrote:

Quoted 5 lines · Pekka Paalanen

On Wed, 22 Feb 2023 07:37:26 -0800
Rob Clark <[email protected]> wrote:

On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen <[email protected]> wrote:

...

Quoted 11 lines

Quoted 10 lines

On another matter, if the application uses SET_DEADLINE with one
timestamp, and the compositor uses SET_DEADLINE on the same thing with
another timestamp, what should happen?

The expectation is that many deadline hints can be set on a fence.
The fence signaller should track the soonest deadline.

You need to document that as UAPI, since it is observable to userspace.
It would be bad if drivers or subsystems would differ in behaviour.

Quoted 5 lines

It is in the end a hint. It is about giving the driver more information so that it can make better choices. But the driver is even free to ignore it. So maybe "expectation" is too strong of a word. Rather, any other behavior doesn't really make sense. But it could end up being dictated by how the hw and/or fw works.

Quoted 3 lines

It will stop being a hint once it has been implemented and used in the wild long enough. The kernel userspace regression rules make sure of that.

Quoted 6 lines

Yeah, tricky and maybe a gray area in this case. I think we eluded elsewhere in the thread that renaming the thing might be an option.

So maybe instead of deadline, which is a very strong word, use something along the lines of "present time hint", or "signalled time hint"? Maybe reads clumsy. Just throwing some ideas for a start.

Quoted 8 lines

You can try, but I fear that if it ever changes behaviour and someone notices that, it's labelled as a kernel regression. I don't think documentation has ever been the authoritative definition of UABI in Linux, it just guides drivers and userspace towards a common understanding and common usage patterns.

So even if the UABI contract is not documented (ugh), you need to be prepared to set the UABI contract through kernel implementation.

To be the devil's advocate it probably wouldn't be an ABI regression but just an regression. Same way as what nice(2) priorities mean hasn't always been the same over the years, I don't think there is a strict contract.

Having said that, it may be different with latency sensitive stuff such as UIs though since it is very observable and can be very painful to users.

Quoted 9 lines

If you do not document the UABI contract, then different drivers are likely to implement it differently, leading to differing behaviour. Also userspace will invent wild ways to abuse the UABI if there is no documentation guiding it on proper use. If userspace or end users observe different behaviour, that's bad even if it's not a regression.

I don't like the situation either, but it is what it is. UABI stability trumps everything regardless of whether it was documented or not.

Quoted 8 lines

I bet userspace is going to use this as a "make it faster, make it hotter" button. I would not be surprised if someone wrote a LD_PRELOAD library that stamps any and all fences with an expired deadline to just squeeze out a little more through some weird side-effect.

Well, that's hopefully overboard in scaring, but in the end, I would like to see UABI documented so I can have a feeling of what it is for and how it was intended to be used. That's all.

We share the same concern. If you read elsewhere in these threads you will notice I have been calling this an "arms race". If the ability to make yourself go faster does not required additional privilege I also worry everyone will do it at which point it becomes pointless. So yes, I do share this concern about exposing any of this as an unprivileged uapi.

Quoted 7 lines

Quoted 2 lines

Is it possible to limit access to only compositors in some sane way? Sounds tricky when dma-fence should be disconnected from DRM..

Maybe it's not that bad in this particular case, because we are talking only about boosting GPU clocks which benefits everyone (except battery life) and it does not penalize other programs like e.g. job priorities do.

Apart from efficiency that you mentioned, which does not always favor higher clocks, sometimes thermal budget is also shared between CPU and GPU. So more GPU clocks can mean fewer CPU clocks. It's really hard to make optimal choices without the full coordination between both schedulers.

But that is even not the main point, which is that if everyone sets the immediate deadline then having the deadline API is a bit pointless. For instance there is a reason negative nice needs CAP_SYS_ADMIN.

However Rob has also pointed out the existence of uclamp.min via sched_setattr which is unprivileged and can influence frequency selection in the CPU world, so I conceded on that point. If CPU world has accepted it so can we I guess.

So IMO we are back to whether we can agree defining it is a hint is good enough, be in via the name of the ioctl/flag itself or via documentation.

Quoted 4 lines

Drivers are not going to use the deadline for scheduling priorities, right? I don't recall seeing any mention of that.

...right?

I wouldn't have thought it would be beneficial to preclude that, or assume what drivers would do with the info to begin with.

Quoted 8 lines

Quoted 6 lines

For instance in i915 we almost had a deadline based scheduler which was much fairer than the current priority sorted fifo and in an ideal world we would either revive or re-implement that idea. In which case considering the fence deadline would naturally slot in and give true integration with compositor deadlines (not just boost clocks and pray it helps).

How is user-space to decide whether to use ioctl(SET_DEADLINE) or poll(POLLPRI)?

Quoted 6 lines

Implementation of blocking gl/vk/cl APIs, like glFinish() would use poll(POLLPRI). It could also set an immediate deadline and then call poll() without POLLPRI.

Other than compositors which do frame-pacing I expect the main usage of either of these is mesa.

Okay, so it looks like we already agreed that having a way to bump frequency from userspace is acceptable. either because there are already other ways that you can waste power and because this already acceptable in the CPU world.

But why we are doing this in hidden ways then?

Why can't we have this hint per context that is getting executed? (either with a boost-context flag or with some low/med/max or '-1' to '1' value like the latency priority)?

Quoted 6 lines

I don't like the waitboost because this heurisitic fails in some media cases. I don't like the global setting because we might be alternating a top-priority with low-priority cases...

So, why not something per context in execution?

It needs to be finer granularity than per-context, because not all waits should trigger boosting. For example, virglrenderer ends up with a thread polling unsignaled fences to know when to signal an interrupt to the guest virtgpu. This alone shouldn't trigger boosting. (We also wouldn't want to completely disable boosting for virglrenderer.) Or the usermode driver could be waiting on a fence to know when to do some cleanup.

Quoted 3 lines

That is not to say that there isn't room for per-context flags to disable/enable boosting for fences created by that context, meaning it could be an AND operation for i915 if it needs to be.

First of all, I believe that the fence deadline hint is a good idea. With that being said, I also don't think it is sufficient in a lot of cases.

The one thing I was alluding to before and that Pekka mentioned as well is that mutter for example has a problem where we're missing the deadline consistently because the clocks don't ramp up fast enough and there is a MR which is just trying to keep the GPU busy to avoid this.

It would be much better if the kernel could make sure the clocks are all ramped up when we start submitting work. In the compositor we actually have a lot of information that *should* influence clocks. We know when we're going to start submitting work and when the deadline for that work is beforehand. We know which windows are visible, and which one should have the highest priority.

We know when there are
input events which actually matter.

We know when the deadline for
client work is.

In the future we also want to make sure clients know beforehand when they should start their work and when the deadline is but that's all very much WIP in both wayland and vulkan.

There are two issues:

1. The compositor has no way to communicate any of that information to the kernel.
2. The only connection to client work the compositor has is a fence to the last bit of work that must be done before the deadline after a wl_surface.commit.

So in both cases a fence is just not the right primitive for us. We need to be able to provide per-context/queue information for work that will happen in the future and we need a way to refer to a context/queue generically and over IPC to boost the clocks of the device that a client is actually using and maybe even give priority.

But like I said, having a per-fence deadline is probably still a good idea and doesn't conflict with any of the more coarse information.

Quoted 183 lines · Sebastian Wick

Quoted 173 lines · Rob Clark

Quoted 156 lines · Rodrigo Vivi

Quoted 136 lines · Rob Clark

Quoted 126 lines · Luben Tuikov

Quoted 124 lines · Tvrtko Ursulin

Quoted 89 lines · Pekka Paalanen

Quoted 86 lines · On Fri, 24 Feb 2023 10:50:51 +0000 Tvrtko Ursulin…

Quoted 44 lines · Pekka Paalanen

Quoted 34 lines · On Fri, 24 Feb 2023 09:41:46 +0000 Tvrtko Ursulin…

Quoted 29 lines · Pekka Paalanen

Quoted 6 lines · On Thu, 23 Feb 2023 10:51:48 -0800 Rob Clark <[email protected]> wrote:

On Thu, Feb 23, 2023 at 1:38 AM Pekka Paalanen <[email protected]> wrote:

On Wed, 22 Feb 2023 07:37:26 -0800
Rob Clark <[email protected]> wrote:

On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen <[email protected]> wrote:

...

Quoted 16 lines

On another matter, if the application uses SET_DEADLINE with one
timestamp, and the compositor uses SET_DEADLINE on the same thing with
another timestamp, what should happen?

The expectation is that many deadline hints can be set on a fence.
The fence signaller should track the soonest deadline.

You need to document that as UAPI, since it is observable to userspace.
It would be bad if drivers or subsystems would differ in behaviour.

It is in the end a hint. It is about giving the driver more
information so that it can make better choices. But the driver is
even free to ignore it. So maybe "expectation" is too strong of a
word. Rather, any other behavior doesn't really make sense. But it
could end up being dictated by how the hw and/or fw works.

Quoted 3 lines

It will stop being a hint once it has been implemented and used in the wild long enough. The kernel userspace regression rules make sure of that.

Quoted 6 lines

Yeah, tricky and maybe a gray area in this case. I think we eluded elsewhere in the thread that renaming the thing might be an option.

So maybe instead of deadline, which is a very strong word, use something along the lines of "present time hint", or "signalled time hint"? Maybe reads clumsy. Just throwing some ideas for a start.

Quoted 8 lines

You can try, but I fear that if it ever changes behaviour and someone notices that, it's labelled as a kernel regression. I don't think documentation has ever been the authoritative definition of UABI in Linux, it just guides drivers and userspace towards a common understanding and common usage patterns.

So even if the UABI contract is not documented (ugh), you need to be prepared to set the UABI contract through kernel implementation.

To be the devil's advocate it probably wouldn't be an ABI regression but just an regression. Same way as what nice(2) priorities mean hasn't always been the same over the years, I don't think there is a strict contract.

Having said that, it may be different with latency sensitive stuff such as UIs though since it is very observable and can be very painful to users.

Quoted 9 lines

If you do not document the UABI contract, then different drivers are likely to implement it differently, leading to differing behaviour. Also userspace will invent wild ways to abuse the UABI if there is no documentation guiding it on proper use. If userspace or end users observe different behaviour, that's bad even if it's not a regression.

I don't like the situation either, but it is what it is. UABI stability trumps everything regardless of whether it was documented or not.

Quoted 8 lines

I bet userspace is going to use this as a "make it faster, make it hotter" button. I would not be surprised if someone wrote a LD_PRELOAD library that stamps any and all fences with an expired deadline to just squeeze out a little more through some weird side-effect.

Well, that's hopefully overboard in scaring, but in the end, I would like to see UABI documented so I can have a feeling of what it is for and how it was intended to be used. That's all.

We share the same concern. If you read elsewhere in these threads you will notice I have been calling this an "arms race". If the ability to make yourself go faster does not required additional privilege I also worry everyone will do it at which point it becomes pointless. So yes, I do share this concern about exposing any of this as an unprivileged uapi.

Quoted 7 lines

Quoted 2 lines

Is it possible to limit access to only compositors in some sane way? Sounds tricky when dma-fence should be disconnected from DRM..

Maybe it's not that bad in this particular case, because we are talking only about boosting GPU clocks which benefits everyone (except battery life) and it does not penalize other programs like e.g. job priorities do.

Apart from efficiency that you mentioned, which does not always favor higher clocks, sometimes thermal budget is also shared between CPU and GPU. So more GPU clocks can mean fewer CPU clocks. It's really hard to make optimal choices without the full coordination between both schedulers.

But that is even not the main point, which is that if everyone sets the immediate deadline then having the deadline API is a bit pointless. For instance there is a reason negative nice needs CAP_SYS_ADMIN.

However Rob has also pointed out the existence of uclamp.min via sched_setattr which is unprivileged and can influence frequency selection in the CPU world, so I conceded on that point. If CPU world has accepted it so can we I guess.

So IMO we are back to whether we can agree defining it is a hint is good enough, be in via the name of the ioctl/flag itself or via documentation.

Quoted 4 lines

Drivers are not going to use the deadline for scheduling priorities, right? I don't recall seeing any mention of that.

...right?

I wouldn't have thought it would be beneficial to preclude that, or assume what drivers would do with the info to begin with.

Quoted 8 lines

Quoted 6 lines

For instance in i915 we almost had a deadline based scheduler which was much fairer than the current priority sorted fifo and in an ideal world we would either revive or re-implement that idea. In which case considering the fence deadline would naturally slot in and give true integration with compositor deadlines (not just boost clocks and pray it helps).

How is user-space to decide whether to use ioctl(SET_DEADLINE) or poll(POLLPRI)?

Quoted 6 lines

Implementation of blocking gl/vk/cl APIs, like glFinish() would use poll(POLLPRI). It could also set an immediate deadline and then call poll() without POLLPRI.

Other than compositors which do frame-pacing I expect the main usage of either of these is mesa.

Okay, so it looks like we already agreed that having a way to bump frequency from userspace is acceptable. either because there are already other ways that you can waste power and because this already acceptable in the CPU world.

But why we are doing this in hidden ways then?

Why can't we have this hint per context that is getting executed? (either with a boost-context flag or with some low/med/max or '-1' to '1' value like the latency priority)?

Quoted 6 lines

I don't like the waitboost because this heurisitic fails in some media cases. I don't like the global setting because we might be alternating a top-priority with low-priority cases...

So, why not something per context in execution?

It needs to be finer granularity than per-context, because not all waits should trigger boosting. For example, virglrenderer ends up with a thread polling unsignaled fences to know when to signal an interrupt to the guest virtgpu. This alone shouldn't trigger boosting. (We also wouldn't want to completely disable boosting for virglrenderer.) Or the usermode driver could be waiting on a fence to know when to do some cleanup.

Quoted 3 lines

That is not to say that there isn't room for per-context flags to disable/enable boosting for fences created by that context, meaning it could be an AND operation for i915 if it needs to be.

First of all, I believe that the fence deadline hint is a good idea. With that being said, I also don't think it is sufficient in a lot of cases.

Quoted 4 lines

The one thing I was alluding to before and that Pekka mentioned as well is that mutter for example has a problem where we're missing the deadline consistently because the clocks don't ramp up fast enough and there is a MR which is just trying to keep the GPU busy to avoid this.

the dynamic double/triple buffer thing?

Quoted 6 lines

It would be much better if the kernel could make sure the clocks are all ramped up when we start submitting work. In the compositor we actually have a lot of information that *should* influence clocks. We know when we're going to start submitting work and when the deadline for that work is beforehand. We know which windows are visible, and which one should have the highest priority.

This sounds like something orthogonal.. something for cgroups? Ie. android moves visible/foreground apps to a different cgroup to given them higher priority. Tvrtko had a patchset to add drm cgroup support..

Quoted 2 lines

We know when there are
input events which actually matter.

This I see input as a different boost source for the driver. (Ie. one boost signal is missing fence deadlines, another is input events, etc.)

We end up using downstream input-handlers on the kernel side for this. Partially for the freq boost (but mostly not, UI interactive workloads like touchscreen scrolling don't generally need high GPU freqs, they are more memory bandwidth limited if they are limited by anything).. really the reason here is to get a head-start on the ~2ms that it takes to power up the GPU if it is suspended.

But this is not quite perfect, since for example some keys should be handled on key-down but others on key-up.

But again, this is something different from fence deadlines. I'm interested in proposals because we do need something for this. But I think it is something is orthogonal to this series. For input, we want the kernel to know long before userspace is ready to submit rendering.

Quoted 7 lines

We know when the deadline for
client work is.

In the future we also want to make sure clients know beforehand when they should start their work and when the deadline is but that's all very much WIP in both wayland and vulkan.

Quoted 7 lines

There are two issues:

1. The compositor has no way to communicate any of that information to the kernel.
2. The only connection to client work the compositor has is a fence to the last bit of work that must be done before the deadline after a wl_surface.commit.

If the client isn't using multiple GPUs, a single fence should be sufficient. And even if it is, well we still have all the dependency information on the kernel side. Ie. drm/sched knows what fences it is waiting on if it is waiting to schedule the work associated with the last fence. It would otherwise require drm/sched to be a bit more tricky than it is so far in this series.

But I think the normal dual-gpu case, the app is only dealing with a single GPU?

Quoted 8 lines

So in both cases a fence is just not the right primitive for us. We need to be able to provide per-context/queue information for work that will happen in the future and we need a way to refer to a context/queue generically and over IPC to boost the clocks of the device that a client is actually using and maybe even give priority.

But like I said, having a per-fence deadline is probably still a good idea and doesn't conflict with any of the more coarse information.

Yeah, I think the thing is you need multiple things, and this is only one of them ;-)

BR,
-R

Quoted 186 lines · Rob Clark

Quoted 183 lines · Sebastian Wick

Quoted 173 lines · Rob Clark

Quoted 156 lines · Rodrigo Vivi

Quoted 136 lines · Rob Clark

Quoted 126 lines · Luben Tuikov

Quoted 124 lines · Tvrtko Ursulin

Quoted 89 lines · Pekka Paalanen

Quoted 86 lines · On Fri, 24 Feb 2023 10:50:51 +0000 Tvrtko Ursulin…

Quoted 44 lines · Pekka Paalanen

Quoted 34 lines · On Fri, 24 Feb 2023 09:41:46 +0000 Tvrtko Ursulin…

Quoted 32 lines · Pekka Paalanen

On Thu, 23 Feb 2023 10:51:48 -0800
Rob Clark <[email protected]> wrote:

On Thu, Feb 23, 2023 at 1:38 AM Pekka Paalanen <[email protected]> wrote:

On Wed, 22 Feb 2023 07:37:26 -0800
Rob Clark <[email protected]> wrote:

On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen <[email protected]> wrote:

...

On another matter, if the application uses SET_DEADLINE with one
timestamp, and the compositor uses SET_DEADLINE on the same thing with
another timestamp, what should happen?

The expectation is that many deadline hints can be set on a fence.
The fence signaller should track the soonest deadline.

You need to document that as UAPI, since it is observable to userspace.
It would be bad if drivers or subsystems would differ in behaviour.

It is in the end a hint. It is about giving the driver more
information so that it can make better choices. But the driver is
even free to ignore it. So maybe "expectation" is too strong of a
word. Rather, any other behavior doesn't really make sense. But it
could end up being dictated by how the hw and/or fw works.

It will stop being a hint once it has been implemented and used in the
wild long enough. The kernel userspace regression rules make sure of
that.

Quoted 6 lines

Yeah, tricky and maybe a gray area in this case. I think we eluded elsewhere in the thread that renaming the thing might be an option.

So maybe instead of deadline, which is a very strong word, use something along the lines of "present time hint", or "signalled time hint"? Maybe reads clumsy. Just throwing some ideas for a start.

Quoted 8 lines

You can try, but I fear that if it ever changes behaviour and someone notices that, it's labelled as a kernel regression. I don't think documentation has ever been the authoritative definition of UABI in Linux, it just guides drivers and userspace towards a common understanding and common usage patterns.

So even if the UABI contract is not documented (ugh), you need to be prepared to set the UABI contract through kernel implementation.

To be the devil's advocate it probably wouldn't be an ABI regression but just an regression. Same way as what nice(2) priorities mean hasn't always been the same over the years, I don't think there is a strict contract.

Having said that, it may be different with latency sensitive stuff such as UIs though since it is very observable and can be very painful to users.

Quoted 9 lines

If you do not document the UABI contract, then different drivers are likely to implement it differently, leading to differing behaviour. Also userspace will invent wild ways to abuse the UABI if there is no documentation guiding it on proper use. If userspace or end users observe different behaviour, that's bad even if it's not a regression.

I don't like the situation either, but it is what it is. UABI stability trumps everything regardless of whether it was documented or not.

Quoted 8 lines

I bet userspace is going to use this as a "make it faster, make it hotter" button. I would not be surprised if someone wrote a LD_PRELOAD library that stamps any and all fences with an expired deadline to just squeeze out a little more through some weird side-effect.

Well, that's hopefully overboard in scaring, but in the end, I would like to see UABI documented so I can have a feeling of what it is for and how it was intended to be used. That's all.

We share the same concern. If you read elsewhere in these threads you will notice I have been calling this an "arms race". If the ability to make yourself go faster does not required additional privilege I also worry everyone will do it at which point it becomes pointless. So yes, I do share this concern about exposing any of this as an unprivileged uapi.

Quoted 7 lines

Quoted 2 lines

Is it possible to limit access to only compositors in some sane way? Sounds tricky when dma-fence should be disconnected from DRM..

Maybe it's not that bad in this particular case, because we are talking only about boosting GPU clocks which benefits everyone (except battery life) and it does not penalize other programs like e.g. job priorities do.

Apart from efficiency that you mentioned, which does not always favor higher clocks, sometimes thermal budget is also shared between CPU and GPU. So more GPU clocks can mean fewer CPU clocks. It's really hard to make optimal choices without the full coordination between both schedulers.

But that is even not the main point, which is that if everyone sets the immediate deadline then having the deadline API is a bit pointless. For instance there is a reason negative nice needs CAP_SYS_ADMIN.

However Rob has also pointed out the existence of uclamp.min via sched_setattr which is unprivileged and can influence frequency selection in the CPU world, so I conceded on that point. If CPU world has accepted it so can we I guess.

So IMO we are back to whether we can agree defining it is a hint is good enough, be in via the name of the ioctl/flag itself or via documentation.

Quoted 4 lines

Drivers are not going to use the deadline for scheduling priorities, right? I don't recall seeing any mention of that.

...right?

I wouldn't have thought it would be beneficial to preclude that, or assume what drivers would do with the info to begin with.

Quoted 8 lines

Quoted 6 lines

For instance in i915 we almost had a deadline based scheduler which was much fairer than the current priority sorted fifo and in an ideal world we would either revive or re-implement that idea. In which case considering the fence deadline would naturally slot in and give true integration with compositor deadlines (not just boost clocks and pray it helps).

How is user-space to decide whether to use ioctl(SET_DEADLINE) or poll(POLLPRI)?

Quoted 6 lines

Implementation of blocking gl/vk/cl APIs, like glFinish() would use poll(POLLPRI). It could also set an immediate deadline and then call poll() without POLLPRI.

Other than compositors which do frame-pacing I expect the main usage of either of these is mesa.

Okay, so it looks like we already agreed that having a way to bump frequency from userspace is acceptable. either because there are already other ways that you can waste power and because this already acceptable in the CPU world.

But why we are doing this in hidden ways then?

Why can't we have this hint per context that is getting executed? (either with a boost-context flag or with some low/med/max or '-1' to '1' value like the latency priority)?

Quoted 6 lines

I don't like the waitboost because this heurisitic fails in some media cases. I don't like the global setting because we might be alternating a top-priority with low-priority cases...

So, why not something per context in execution?

It needs to be finer granularity than per-context, because not all waits should trigger boosting. For example, virglrenderer ends up with a thread polling unsignaled fences to know when to signal an interrupt to the guest virtgpu. This alone shouldn't trigger boosting. (We also wouldn't want to completely disable boosting for virglrenderer.) Or the usermode driver could be waiting on a fence to know when to do some cleanup.

Quoted 3 lines

That is not to say that there isn't room for per-context flags to disable/enable boosting for fences created by that context, meaning it could be an AND operation for i915 if it needs to be.

First of all, I believe that the fence deadline hint is a good idea. With that being said, I also don't think it is sufficient in a lot of cases.

Quoted 6 lines

Quoted 4 lines

The one thing I was alluding to before and that Pekka mentioned as well is that mutter for example has a problem where we're missing the deadline consistently because the clocks don't ramp up fast enough and there is a MR which is just trying to keep the GPU busy to avoid this.

the dynamic double/triple buffer thing?

Yes

Quoted 7 lines

Quoted 6 lines

It would be much better if the kernel could make sure the clocks are all ramped up when we start submitting work. In the compositor we actually have a lot of information that *should* influence clocks. We know when we're going to start submitting work and when the deadline for that work is beforehand. We know which windows are visible, and which one should have the highest priority.

Quoted 4 lines

This sounds like something orthogonal.. something for cgroups? Ie. android moves visible/foreground apps to a different cgroup to given them higher priority. Tvrtko had a patchset to add drm cgroup support..

For the priority stuff, yes, probably. The visibility information on the other hand could be used to determine if we want to ramp up the GPU in the first place.

Quoted 7 lines

Quoted 2 lines

We know when there are
input events which actually matter.

This I see input as a different boost source for the driver. (Ie. one boost signal is missing fence deadlines, another is input events, etc.)

Quoted 6 lines

We end up using downstream input-handlers on the kernel side for this. Partially for the freq boost (but mostly not, UI interactive workloads like touchscreen scrolling don't generally need high GPU freqs, they are more memory bandwidth limited if they are limited by anything).. really the reason here is to get a head-start on the ~2ms that it takes to power up the GPU if it is suspended.

Right, but one of my main points I want to make here is that we could get the head-start not only in response to input events but also for the GPU work the compositor submits and in the future also to GPU work that clients commit. Except that we don't have a way to tell the kernel about it.

Quoted 8 lines

But this is not quite perfect, since for example some keys should be handled on key-down but others on key-up.

But again, this is something different from fence deadlines. I'm interested in proposals because we do need something for this. But I think it is something is orthogonal to this series. For input, we want the kernel to know long before userspace is ready to submit rendering.

We can do that in the compositor! Input events are really not something you should care about in the kernel. Input itself is also not the only indication of incoming animated content. Some network packets arriving could equally well result in the same situation.

Quoted 22 lines

Quoted 7 lines

We know when the deadline for
client work is.

In the future we also want to make sure clients know beforehand when they should start their work and when the deadline is but that's all very much WIP in both wayland and vulkan.

Quoted 7 lines

There are two issues:

1. The compositor has no way to communicate any of that information to the kernel.
2. The only connection to client work the compositor has is a fence to the last bit of work that must be done before the deadline after a wl_surface.commit.

If the client isn't using multiple GPUs, a single fence should be sufficient. And even if it is, well we still have all the dependency information on the kernel side. Ie. drm/sched knows what fences it is waiting on if it is waiting to schedule the work associated with the last fence. It would otherwise require drm/sched to be a bit more tricky than it is so far in this series.

Quoted 1 line

But I think the normal dual-gpu case, the app is only dealing with a single GPU?

We generally don't know which GPU a client uses though. We know which one we're using and tell the client that the buffer should be compatible with it but that's the extent of information we have, until we get a fence but that fence usually gets to the compositor pretty late. Way too late for the compositor to tell the kernel to ramp up the GPU and still have an impact.

It also seems like we're moving away from tracking execution dependencies with fences when we're switching to user mode fences.

Quoted 15 lines

Quoted 8 lines

So in both cases a fence is just not the right primitive for us. We need to be able to provide per-context/queue information for work that will happen in the future and we need a way to refer to a context/queue generically and over IPC to boost the clocks of the device that a client is actually using and maybe even give priority.

But like I said, having a per-fence deadline is probably still a good idea and doesn't conflict with any of the more coarse information.

Yeah, I think the thing is you need multiple things, and this is only one of them ;-)

BR,
-R

Quoted 204 lines · Sebastian Wick

Quoted 186 lines · Rob Clark

Quoted 183 lines · Sebastian Wick

Quoted 173 lines · Rob Clark

Quoted 156 lines · Rodrigo Vivi

Quoted 136 lines · Rob Clark

Quoted 126 lines · Luben Tuikov

Quoted 124 lines · Tvrtko Ursulin

Quoted 89 lines · Pekka Paalanen

Quoted 86 lines · On Fri, 24 Feb 2023 10:50:51 +0000 Tvrtko Ursulin…

Quoted 44 lines · Pekka Paalanen

Quoted 40 lines · On Fri, 24 Feb 2023 09:41:46 +0000 Tvrtko Ursulin…

On 24/02/2023 09:26, Pekka Paalanen wrote:
On Thu, 23 Feb 2023 10:51:48 -0800
Rob Clark <[email protected]> wrote:

On Thu, Feb 23, 2023 at 1:38 AM Pekka Paalanen <[email protected]> wrote:

On Wed, 22 Feb 2023 07:37:26 -0800
Rob Clark <[email protected]> wrote:

On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen <[email protected]> wrote:

...

On another matter, if the application uses SET_DEADLINE with one
timestamp, and the compositor uses SET_DEADLINE on the same thing with
another timestamp, what should happen?

The expectation is that many deadline hints can be set on a fence.
The fence signaller should track the soonest deadline.

You need to document that as UAPI, since it is observable to userspace.
It would be bad if drivers or subsystems would differ in behaviour.

It is in the end a hint. It is about giving the driver more
information so that it can make better choices. But the driver is
even free to ignore it. So maybe "expectation" is too strong of a
word. Rather, any other behavior doesn't really make sense. But it
could end up being dictated by how the hw and/or fw works.

It will stop being a hint once it has been implemented and used in the
wild long enough. The kernel userspace regression rules make sure of
that.

Yeah, tricky and maybe a gray area in this case. I think we eluded
elsewhere in the thread that renaming the thing might be an option.

So maybe instead of deadline, which is a very strong word, use something
along the lines of "present time hint", or "signalled time hint"? Maybe
reads clumsy. Just throwing some ideas for a start.

Quoted 8 lines

You can try, but I fear that if it ever changes behaviour and someone notices that, it's labelled as a kernel regression. I don't think documentation has ever been the authoritative definition of UABI in Linux, it just guides drivers and userspace towards a common understanding and common usage patterns.

So even if the UABI contract is not documented (ugh), you need to be prepared to set the UABI contract through kernel implementation.

To be the devil's advocate it probably wouldn't be an ABI regression but just an regression. Same way as what nice(2) priorities mean hasn't always been the same over the years, I don't think there is a strict contract.

Having said that, it may be different with latency sensitive stuff such as UIs though since it is very observable and can be very painful to users.

Quoted 9 lines

If you do not document the UABI contract, then different drivers are likely to implement it differently, leading to differing behaviour. Also userspace will invent wild ways to abuse the UABI if there is no documentation guiding it on proper use. If userspace or end users observe different behaviour, that's bad even if it's not a regression.

I don't like the situation either, but it is what it is. UABI stability trumps everything regardless of whether it was documented or not.

Quoted 8 lines

I bet userspace is going to use this as a "make it faster, make it hotter" button. I would not be surprised if someone wrote a LD_PRELOAD library that stamps any and all fences with an expired deadline to just squeeze out a little more through some weird side-effect.

Well, that's hopefully overboard in scaring, but in the end, I would like to see UABI documented so I can have a feeling of what it is for and how it was intended to be used. That's all.

We share the same concern. If you read elsewhere in these threads you will notice I have been calling this an "arms race". If the ability to make yourself go faster does not required additional privilege I also worry everyone will do it at which point it becomes pointless. So yes, I do share this concern about exposing any of this as an unprivileged uapi.

Quoted 7 lines

Quoted 2 lines

Is it possible to limit access to only compositors in some sane way? Sounds tricky when dma-fence should be disconnected from DRM..

Maybe it's not that bad in this particular case, because we are talking only about boosting GPU clocks which benefits everyone (except battery life) and it does not penalize other programs like e.g. job priorities do.

Apart from efficiency that you mentioned, which does not always favor higher clocks, sometimes thermal budget is also shared between CPU and GPU. So more GPU clocks can mean fewer CPU clocks. It's really hard to make optimal choices without the full coordination between both schedulers.

But that is even not the main point, which is that if everyone sets the immediate deadline then having the deadline API is a bit pointless. For instance there is a reason negative nice needs CAP_SYS_ADMIN.

However Rob has also pointed out the existence of uclamp.min via sched_setattr which is unprivileged and can influence frequency selection in the CPU world, so I conceded on that point. If CPU world has accepted it so can we I guess.

So IMO we are back to whether we can agree defining it is a hint is good enough, be in via the name of the ioctl/flag itself or via documentation.

Quoted 4 lines

Drivers are not going to use the deadline for scheduling priorities, right? I don't recall seeing any mention of that.

...right?

I wouldn't have thought it would be beneficial to preclude that, or assume what drivers would do with the info to begin with.

Quoted 8 lines

Quoted 6 lines

For instance in i915 we almost had a deadline based scheduler which was much fairer than the current priority sorted fifo and in an ideal world we would either revive or re-implement that idea. In which case considering the fence deadline would naturally slot in and give true integration with compositor deadlines (not just boost clocks and pray it helps).

How is user-space to decide whether to use ioctl(SET_DEADLINE) or poll(POLLPRI)?

Quoted 6 lines

Implementation of blocking gl/vk/cl APIs, like glFinish() would use poll(POLLPRI). It could also set an immediate deadline and then call poll() without POLLPRI.

Other than compositors which do frame-pacing I expect the main usage of either of these is mesa.

Okay, so it looks like we already agreed that having a way to bump frequency from userspace is acceptable. either because there are already other ways that you can waste power and because this already acceptable in the CPU world.

But why we are doing this in hidden ways then?

Why can't we have this hint per context that is getting executed? (either with a boost-context flag or with some low/med/max or '-1' to '1' value like the latency priority)?

Quoted 6 lines

I don't like the waitboost because this heurisitic fails in some media cases. I don't like the global setting because we might be alternating a top-priority with low-priority cases...

So, why not something per context in execution?

It needs to be finer granularity than per-context, because not all waits should trigger boosting. For example, virglrenderer ends up with a thread polling unsignaled fences to know when to signal an interrupt to the guest virtgpu. This alone shouldn't trigger boosting. (We also wouldn't want to completely disable boosting for virglrenderer.) Or the usermode driver could be waiting on a fence to know when to do some cleanup.

Quoted 3 lines

That is not to say that there isn't room for per-context flags to disable/enable boosting for fences created by that context, meaning it could be an AND operation for i915 if it needs to be.

First of all, I believe that the fence deadline hint is a good idea. With that being said, I also don't think it is sufficient in a lot of cases.

Quoted 6 lines

Quoted 4 lines

The one thing I was alluding to before and that Pekka mentioned as well is that mutter for example has a problem where we're missing the deadline consistently because the clocks don't ramp up fast enough and there is a MR which is just trying to keep the GPU busy to avoid this.

the dynamic double/triple buffer thing?

Yes

Quoted 7 lines

Quoted 6 lines

It would be much better if the kernel could make sure the clocks are all ramped up when we start submitting work. In the compositor we actually have a lot of information that *should* influence clocks. We know when we're going to start submitting work and when the deadline for that work is beforehand. We know which windows are visible, and which one should have the highest priority.

Quoted 8 lines

Quoted 4 lines

This sounds like something orthogonal.. something for cgroups? Ie. android moves visible/foreground apps to a different cgroup to given them higher priority. Tvrtko had a patchset to add drm cgroup support..

For the priority stuff, yes, probably. The visibility information on the other hand could be used to determine if we want to ramp up the GPU in the first place.

Right, but I think that we could have multiple cgroup based knobs, one that adjusts priority and one that limits/disables deadline based boost? This way the compositor could setup different policies for visible vs hidden apps influencing both how much time they get on the GPU and boost.

Quoted 29 lines

Quoted 7 lines

Quoted 2 lines

We know when there are
input events which actually matter.

This I see input as a different boost source for the driver. (Ie. one boost signal is missing fence deadlines, another is input events, etc.)

Quoted 6 lines

We end up using downstream input-handlers on the kernel side for this. Partially for the freq boost (but mostly not, UI interactive workloads like touchscreen scrolling don't generally need high GPU freqs, they are more memory bandwidth limited if they are limited by anything).. really the reason here is to get a head-start on the ~2ms that it takes to power up the GPU if it is suspended.

Right, but one of my main points I want to make here is that we could get the head-start not only in response to input events but also for the GPU work the compositor submits and in the future also to GPU work that clients commit. Except that we don't have a way to tell the kernel about it.

Quoted 8 lines

But this is not quite perfect, since for example some keys should be handled on key-down but others on key-up.

But again, this is something different from fence deadlines. I'm interested in proposals because we do need something for this. But I think it is something is orthogonal to this series. For input, we want the kernel to know long before userspace is ready to submit rendering.

Quoted 4 lines

We can do that in the compositor! Input events are really not something you should care about in the kernel. Input itself is also not the only indication of incoming animated content. Some network packets arriving could equally well result in the same situation.

We do use input boost not just for GPU freq, but also for CPU freq. It seems like triggering it from the kernel could happen somewhat sooner than userspace. (But just speculation.)

I'm not sure network packets count. Or at least compared to a touch interface, the user won't perceive the lag nearly as much, compared to whether or not the UI tracks their fingertips.

Quoted 22 lines

Quoted 22 lines

Quoted 7 lines

We know when the deadline for
client work is.

In the future we also want to make sure clients know beforehand when they should start their work and when the deadline is but that's all very much WIP in both wayland and vulkan.

Quoted 7 lines

There are two issues:

1. The compositor has no way to communicate any of that information to the kernel.
2. The only connection to client work the compositor has is a fence to the last bit of work that must be done before the deadline after a wl_surface.commit.

If the client isn't using multiple GPUs, a single fence should be sufficient. And even if it is, well we still have all the dependency information on the kernel side. Ie. drm/sched knows what fences it is waiting on if it is waiting to schedule the work associated with the last fence. It would otherwise require drm/sched to be a bit more tricky than it is so far in this series.

Quoted 8 lines

Quoted 1 line

But I think the normal dual-gpu case, the app is only dealing with a single GPU?

We generally don't know which GPU a client uses though. We know which one we're using and tell the client that the buffer should be compatible with it but that's the extent of information we have, until we get a fence but that fence usually gets to the compositor pretty late. Way too late for the compositor to tell the kernel to ramp up the GPU and still have an impact.

Are you sure about that? I'd have expected the app to hand off fence+buffer to the compositor pretty quickly after rendering is submitted. This is what I've seen elsewhere.

Quoted 2 lines

It also seems like we're moving away from tracking execution dependencies with fences when we're switching to user mode fences.

I suppose there are two cases..

1) dependent fences all from the same device.. no prob, the right driver gets the signal that it needs to clk up, and figures out the rest on it's own

2) dependent fences from different devices.. I suppose if device B is waiting for a fence from device A before it can make forward progress, why not express this as a deadline hint on A's fence? (But by the time you have this problem, you are probably not really caring about power consumption, so meh..)

BR,
-R

Quoted 16 lines

Quoted 15 lines

Quoted 8 lines

So in both cases a fence is just not the right primitive for us. We need to be able to provide per-context/queue information for work that will happen in the future and we need a way to refer to a context/queue generically and over IPC to boost the clocks of the device that a client is actually using and maybe even give priority.

But like I said, having a per-fence deadline is probably still a good idea and doesn't conflict with any of the more coarse information.

Yeah, I think the thing is you need multiple things, and this is only one of them ;-)

BR,
-R

Quoted 216 lines · Rob Clark

Quoted 204 lines · Sebastian Wick

Quoted 186 lines · Rob Clark

Quoted 183 lines · Sebastian Wick

Quoted 173 lines · Rob Clark

Quoted 156 lines · Rodrigo Vivi

Quoted 136 lines · Rob Clark

Quoted 126 lines · Luben Tuikov

Quoted 124 lines · Tvrtko Ursulin

Quoted 89 lines · Pekka Paalanen

Quoted 86 lines · On Fri, 24 Feb 2023 10:50:51 +0000 Tvrtko Ursulin…

Quoted 52 lines · Pekka Paalanen

On Fri, 24 Feb 2023 09:41:46 +0000
Tvrtko Ursulin <[email protected]> wrote:

On 24/02/2023 09:26, Pekka Paalanen wrote:
On Thu, 23 Feb 2023 10:51:48 -0800
Rob Clark <[email protected]> wrote:

On Thu, Feb 23, 2023 at 1:38 AM Pekka Paalanen <[email protected]> wrote:

On Wed, 22 Feb 2023 07:37:26 -0800
Rob Clark <[email protected]> wrote:

On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen <[email protected]> wrote:

...

On another matter, if the application uses SET_DEADLINE with one
timestamp, and the compositor uses SET_DEADLINE on the same thing with
another timestamp, what should happen?

The expectation is that many deadline hints can be set on a fence.
The fence signaller should track the soonest deadline.

You need to document that as UAPI, since it is observable to userspace.
It would be bad if drivers or subsystems would differ in behaviour.

It is in the end a hint. It is about giving the driver more
information so that it can make better choices. But the driver is
even free to ignore it. So maybe "expectation" is too strong of a
word. Rather, any other behavior doesn't really make sense. But it
could end up being dictated by how the hw and/or fw works.

It will stop being a hint once it has been implemented and used in the
wild long enough. The kernel userspace regression rules make sure of
that.

Yeah, tricky and maybe a gray area in this case. I think we eluded
elsewhere in the thread that renaming the thing might be an option.

So maybe instead of deadline, which is a very strong word, use something
along the lines of "present time hint", or "signalled time hint"? Maybe
reads clumsy. Just throwing some ideas for a start.

You can try, but I fear that if it ever changes behaviour and
someone notices that, it's labelled as a kernel regression. I don't
think documentation has ever been the authoritative definition of UABI
in Linux, it just guides drivers and userspace towards a common
understanding and common usage patterns.

So even if the UABI contract is not documented (ugh), you need to be
prepared to set the UABI contract through kernel implementation.

To be the devil's advocate it probably wouldn't be an ABI regression but just an regression. Same way as what nice(2) priorities mean hasn't always been the same over the years, I don't think there is a strict contract.

Having said that, it may be different with latency sensitive stuff such as UIs though since it is very observable and can be very painful to users.

Quoted 17 lines

If you do not document the UABI contract, then different drivers are
likely to implement it differently, leading to differing behaviour.
Also userspace will invent wild ways to abuse the UABI if there is no
documentation guiding it on proper use. If userspace or end users
observe different behaviour, that's bad even if it's not a regression.

I don't like the situation either, but it is what it is. UABI stability
trumps everything regardless of whether it was documented or not.

I bet userspace is going to use this as a "make it faster, make it
hotter" button. I would not be surprised if someone wrote a LD_PRELOAD
library that stamps any and all fences with an expired deadline to
just squeeze out a little more through some weird side-effect.

Well, that's hopefully overboard in scaring, but in the end, I would
like to see UABI documented so I can have a feeling of what it is for
and how it was intended to be used. That's all.

We share the same concern. If you read elsewhere in these threads you will notice I have been calling this an "arms race". If the ability to make yourself go faster does not required additional privilege I also worry everyone will do it at which point it becomes pointless. So yes, I do share this concern about exposing any of this as an unprivileged uapi.

Quoted 7 lines

Quoted 2 lines

Is it possible to limit access to only compositors in some sane way? Sounds tricky when dma-fence should be disconnected from DRM..

Maybe it's not that bad in this particular case, because we are talking only about boosting GPU clocks which benefits everyone (except battery life) and it does not penalize other programs like e.g. job priorities do.

Apart from efficiency that you mentioned, which does not always favor higher clocks, sometimes thermal budget is also shared between CPU and GPU. So more GPU clocks can mean fewer CPU clocks. It's really hard to make optimal choices without the full coordination between both schedulers.

But that is even not the main point, which is that if everyone sets the immediate deadline then having the deadline API is a bit pointless. For instance there is a reason negative nice needs CAP_SYS_ADMIN.

However Rob has also pointed out the existence of uclamp.min via sched_setattr which is unprivileged and can influence frequency selection in the CPU world, so I conceded on that point. If CPU world has accepted it so can we I guess.

So IMO we are back to whether we can agree defining it is a hint is good enough, be in via the name of the ioctl/flag itself or via documentation.

Quoted 4 lines

Drivers are not going to use the deadline for scheduling priorities, right? I don't recall seeing any mention of that.

...right?

I wouldn't have thought it would be beneficial to preclude that, or assume what drivers would do with the info to begin with.

Quoted 8 lines

Quoted 6 lines

For instance in i915 we almost had a deadline based scheduler which was much fairer than the current priority sorted fifo and in an ideal world we would either revive or re-implement that idea. In which case considering the fence deadline would naturally slot in and give true integration with compositor deadlines (not just boost clocks and pray it helps).

How is user-space to decide whether to use ioctl(SET_DEADLINE) or poll(POLLPRI)?

Quoted 6 lines

Implementation of blocking gl/vk/cl APIs, like glFinish() would use poll(POLLPRI). It could also set an immediate deadline and then call poll() without POLLPRI.

Other than compositors which do frame-pacing I expect the main usage of either of these is mesa.

Okay, so it looks like we already agreed that having a way to bump frequency from userspace is acceptable. either because there are already other ways that you can waste power and because this already acceptable in the CPU world.

But why we are doing this in hidden ways then?

Why can't we have this hint per context that is getting executed? (either with a boost-context flag or with some low/med/max or '-1' to '1' value like the latency priority)?

Quoted 6 lines

I don't like the waitboost because this heurisitic fails in some media cases. I don't like the global setting because we might be alternating a top-priority with low-priority cases...

So, why not something per context in execution?

It needs to be finer granularity than per-context, because not all waits should trigger boosting. For example, virglrenderer ends up with a thread polling unsignaled fences to know when to signal an interrupt to the guest virtgpu. This alone shouldn't trigger boosting. (We also wouldn't want to completely disable boosting for virglrenderer.) Or the usermode driver could be waiting on a fence to know when to do some cleanup.

Quoted 3 lines

That is not to say that there isn't room for per-context flags to disable/enable boosting for fences created by that context, meaning it could be an AND operation for i915 if it needs to be.

First of all, I believe that the fence deadline hint is a good idea. With that being said, I also don't think it is sufficient in a lot of cases.

Quoted 6 lines

Quoted 4 lines

The one thing I was alluding to before and that Pekka mentioned as well is that mutter for example has a problem where we're missing the deadline consistently because the clocks don't ramp up fast enough and there is a MR which is just trying to keep the GPU busy to avoid this.

the dynamic double/triple buffer thing?

Yes

Quoted 7 lines

Quoted 6 lines

It would be much better if the kernel could make sure the clocks are all ramped up when we start submitting work. In the compositor we actually have a lot of information that *should* influence clocks. We know when we're going to start submitting work and when the deadline for that work is beforehand. We know which windows are visible, and which one should have the highest priority.

Quoted 8 lines

Quoted 4 lines

This sounds like something orthogonal.. something for cgroups? Ie. android moves visible/foreground apps to a different cgroup to given them higher priority. Tvrtko had a patchset to add drm cgroup support..

For the priority stuff, yes, probably. The visibility information on the other hand could be used to determine if we want to ramp up the GPU in the first place.

Quoted 5 lines

Right, but I think that we could have multiple cgroup based knobs, one that adjusts priority and one that limits/disables deadline based boost? This way the compositor could setup different policies for visible vs hidden apps influencing both how much time they get on the GPU and boost.

I'm not sure if a negative control really makes sense. There are probably timing sensitive use cases where the result doesn't show up on the local screen and penalizing them when they are not focused or hidden might also not be the best idea.

Quoted 34 lines

Quoted 33 lines

Quoted 7 lines

Quoted 2 lines

We know when there are
input events which actually matter.

This I see input as a different boost source for the driver. (Ie. one boost signal is missing fence deadlines, another is input events, etc.)

Quoted 6 lines

We end up using downstream input-handlers on the kernel side for this. Partially for the freq boost (but mostly not, UI interactive workloads like touchscreen scrolling don't generally need high GPU freqs, they are more memory bandwidth limited if they are limited by anything).. really the reason here is to get a head-start on the ~2ms that it takes to power up the GPU if it is suspended.

Right, but one of my main points I want to make here is that we could get the head-start not only in response to input events but also for the GPU work the compositor submits and in the future also to GPU work that clients commit. Except that we don't have a way to tell the kernel about it.

Quoted 8 lines

But this is not quite perfect, since for example some keys should be handled on key-down but others on key-up.

But again, this is something different from fence deadlines. I'm interested in proposals because we do need something for this. But I think it is something is orthogonal to this series. For input, we want the kernel to know long before userspace is ready to submit rendering.

We can do that in the compositor! Input events are really not something you should care about in the kernel. Input itself is also not the only indication of incoming animated content. Some network packets arriving could equally well result in the same situation.

Quoted 3 lines

We do use input boost not just for GPU freq, but also for CPU freq. It seems like triggering it from the kernel could happen somewhat sooner than userspace. (But just speculation.)

Technically it has to be sooner but I doubt it makes any difference. Getting a lot of false-positives on the other hand does make a difference.

Quoted 3 lines

I'm not sure network packets count. Or at least compared to a touch interface, the user won't perceive the lag nearly as much, compared to whether or not the UI tracks their fingertips.

Sure, stutter in interactive cases is especially awful but stutter is awful in general. If we can solve it in all cases we should do so.

Quoted 31 lines

Quoted 22 lines

Quoted 22 lines

Quoted 7 lines

We know when the deadline for
client work is.

In the future we also want to make sure clients know beforehand when they should start their work and when the deadline is but that's all very much WIP in both wayland and vulkan.

Quoted 7 lines

There are two issues:

1. The compositor has no way to communicate any of that information to the kernel.
2. The only connection to client work the compositor has is a fence to the last bit of work that must be done before the deadline after a wl_surface.commit.

If the client isn't using multiple GPUs, a single fence should be sufficient. And even if it is, well we still have all the dependency information on the kernel side. Ie. drm/sched knows what fences it is waiting on if it is waiting to schedule the work associated with the last fence. It would otherwise require drm/sched to be a bit more tricky than it is so far in this series.

Quoted 8 lines

Quoted 1 line

But I think the normal dual-gpu case, the app is only dealing with a single GPU?

We generally don't know which GPU a client uses though. We know which one we're using and tell the client that the buffer should be compatible with it but that's the extent of information we have, until we get a fence but that fence usually gets to the compositor pretty late. Way too late for the compositor to tell the kernel to ramp up the GPU and still have an impact.

Quoted 3 lines

Are you sure about that? I'd have expected the app to hand off fence+buffer to the compositor pretty quickly after rendering is submitted. This is what I've seen elsewhere.

After rendering is submitted it is already too late if the GPU takes 2ms to wake up. And if there is no rendering submitted there is no fence and if there is no fence it has no deadline. If we want to solve that we also need some kind of 'work will be submitted to the queue starting from this time' hint.

Quoted 5 lines

Quoted 2 lines

It also seems like we're moving away from tracking execution dependencies with fences when we're switching to user mode fences.

I suppose there are two cases..

Quoted 3 lines

1) dependent fences all from the same device.. no prob, the right driver gets the signal that it needs to clk up, and figures out the rest on it's own

AFAIU with user mode fences it's impossible for the kernel to figure out what work depends on it and it might never signal. The whole deadline on fences thing breaks down with user mode fences.

Quoted 26 lines

2) dependent fences from different devices.. I suppose if device B is waiting for a fence from device A before it can make forward progress, why not express this as a deadline hint on A's fence? (But by the time you have this problem, you are probably not really caring about power consumption, so meh..)

BR,
-R

Quoted 16 lines

Quoted 15 lines

Quoted 8 lines

So in both cases a fence is just not the right primitive for us. We need to be able to provide per-context/queue information for work that will happen in the future and we need a way to refer to a context/queue generically and over IPC to boost the clocks of the device that a client is actually using and maybe even give priority.

But like I said, having a per-fence deadline is probably still a good idea and doesn't conflict with any of the more coarse information.

Yeah, I think the thing is you need multiple things, and this is only one of them ;-)

BR,
-R

Quoted 156 lines · Rodrigo Vivi

Quoted 136 lines · Rob Clark

Quoted 126 lines · Luben Tuikov

Quoted 124 lines · Tvrtko Ursulin

Quoted 89 lines · Pekka Paalanen

Quoted 86 lines · On Fri, 24 Feb 2023 10:50:51 +0000 Tvrtko Ursulin…

Quoted 44 lines · Pekka Paalanen

Quoted 34 lines · On Fri, 24 Feb 2023 09:41:46 +0000 Tvrtko Ursulin…

Quoted 29 lines · Pekka Paalanen

Quoted 6 lines · On Thu, 23 Feb 2023 10:51:48 -0800 Rob Clark <[email protected]> wrote:

Quoted 5 lines · Pekka Paalanen

Quoted 1 line · On Wed, 22 Feb 2023 07:37:26 -0800 Rob Clark <[email protected]> wrote:

On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen <[email protected]> wrote:

...

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

On another matter, if the application uses SET_DEADLINE with one
timestamp, and the compositor uses SET_DEADLINE on the same thing with
another timestamp, what should happen?

The expectation is that many deadline hints can be set on a fence.
The fence signaller should track the soonest deadline.

You need to document that as UAPI, since it is observable to userspace. It would be bad if drivers or subsystems would differ in behaviour.

Quoted 5 lines

It is in the end a hint. It is about giving the driver more information so that it can make better choices. But the driver is even free to ignore it. So maybe "expectation" is too strong of a word. Rather, any other behavior doesn't really make sense. But it could end up being dictated by how the hw and/or fw works.

Quoted 3 lines

It will stop being a hint once it has been implemented and used in the wild long enough. The kernel userspace regression rules make sure of that.

Quoted 6 lines

Yeah, tricky and maybe a gray area in this case. I think we eluded elsewhere in the thread that renaming the thing might be an option.

So maybe instead of deadline, which is a very strong word, use something along the lines of "present time hint", or "signalled time hint"? Maybe reads clumsy. Just throwing some ideas for a start.

Quoted 8 lines

You can try, but I fear that if it ever changes behaviour and someone notices that, it's labelled as a kernel regression. I don't think documentation has ever been the authoritative definition of UABI in Linux, it just guides drivers and userspace towards a common understanding and common usage patterns.

So even if the UABI contract is not documented (ugh), you need to be prepared to set the UABI contract through kernel implementation.

To be the devil's advocate it probably wouldn't be an ABI regression but just an regression. Same way as what nice(2) priorities mean hasn't always been the same over the years, I don't think there is a strict contract.

Having said that, it may be different with latency sensitive stuff such as UIs though since it is very observable and can be very painful to users.

Quoted 9 lines

If you do not document the UABI contract, then different drivers are likely to implement it differently, leading to differing behaviour. Also userspace will invent wild ways to abuse the UABI if there is no documentation guiding it on proper use. If userspace or end users observe different behaviour, that's bad even if it's not a regression.

I don't like the situation either, but it is what it is. UABI stability trumps everything regardless of whether it was documented or not.

Quoted 8 lines

I bet userspace is going to use this as a "make it faster, make it hotter" button. I would not be surprised if someone wrote a LD_PRELOAD library that stamps any and all fences with an expired deadline to just squeeze out a little more through some weird side-effect.

Well, that's hopefully overboard in scaring, but in the end, I would like to see UABI documented so I can have a feeling of what it is for and how it was intended to be used. That's all.

We share the same concern. If you read elsewhere in these threads you will notice I have been calling this an "arms race". If the ability to make yourself go faster does not required additional privilege I also worry everyone will do it at which point it becomes pointless. So yes, I do share this concern about exposing any of this as an unprivileged uapi.

Quoted 7 lines

Quoted 2 lines

Is it possible to limit access to only compositors in some sane way? Sounds tricky when dma-fence should be disconnected from DRM..

Maybe it's not that bad in this particular case, because we are talking only about boosting GPU clocks which benefits everyone (except battery life) and it does not penalize other programs like e.g. job priorities do.

Apart from efficiency that you mentioned, which does not always favor higher clocks, sometimes thermal budget is also shared between CPU and GPU. So more GPU clocks can mean fewer CPU clocks. It's really hard to make optimal choices without the full coordination between both schedulers.

But that is even not the main point, which is that if everyone sets the immediate deadline then having the deadline API is a bit pointless. For instance there is a reason negative nice needs CAP_SYS_ADMIN.

However Rob has also pointed out the existence of uclamp.min via sched_setattr which is unprivileged and can influence frequency selection in the CPU world, so I conceded on that point. If CPU world has accepted it so can we I guess.

So IMO we are back to whether we can agree defining it is a hint is good enough, be in via the name of the ioctl/flag itself or via documentation.

Quoted 4 lines

Drivers are not going to use the deadline for scheduling priorities, right? I don't recall seeing any mention of that.

...right?

I wouldn't have thought it would be beneficial to preclude that, or assume what drivers would do with the info to begin with.

Quoted 8 lines

Quoted 6 lines

For instance in i915 we almost had a deadline based scheduler which was much fairer than the current priority sorted fifo and in an ideal world we would either revive or re-implement that idea. In which case considering the fence deadline would naturally slot in and give true integration with compositor deadlines (not just boost clocks and pray it helps).

How is user-space to decide whether to use ioctl(SET_DEADLINE) or poll(POLLPRI)?

Quoted 6 lines

Implementation of blocking gl/vk/cl APIs, like glFinish() would use poll(POLLPRI). It could also set an immediate deadline and then call poll() without POLLPRI.

Other than compositors which do frame-pacing I expect the main usage of either of these is mesa.

Okay, so it looks like we already agreed that having a way to bump frequency from userspace is acceptable. either because there are already other ways that you can waste power and because this already acceptable in the CPU world.

But why we are doing this in hidden ways then?

Why can't we have this hint per context that is getting executed? (either with a boost-context flag or with some low/med/max or '-1' to '1' value like the latency priority)?

Quoted 6 lines

I don't like the waitboost because this heurisitic fails in some media cases. I don't like the global setting because we might be alternating a top-priority with low-priority cases...

So, why not something per context in execution?

It needs to be finer granularity than per-context, because not all waits should trigger boosting. For example, virglrenderer ends up with a thread polling unsignaled fences to know when to signal an interrupt to the guest virtgpu. This alone shouldn't trigger boosting. (We also wouldn't want to completely disable boosting for virglrenderer.) Or the usermode driver could be waiting on a fence to know when to do some cleanup.

That is not to say that there isn't room for per-context flags to disable/enable boosting for fences created by that context, meaning it could be an AND operation for i915 if it needs to be.

BR,
-R