Re: [Intel-gfx] [PATCH 10/11] drm/i915: Use a preemption timeout to enforce interactivity

[Tvrtko Ursulin]

On 27/03/2018 09:39, Chris Wilson wrote:

Quoting Tvrtko Ursulin (2018-03-27 09:35:17)


On 26/03/2018 12:50, Chris Wilson wrote:

Use a liberal timeout of 20ms to ensure that the rendering for an
interactive pageflip is started in a timely fashion, and that
user interaction is not blocked by GPU, or CPU, hogs. This is at the cost
of resetting whoever was blocking the preemption, likely leading to that
context/process being banned from submitting future requests.

Signed-off-by: Chris Wilson 
---
   drivers/gpu/drm/i915/i915_drv.h  |  3 ++-
   drivers/gpu/drm/i915/i915_gem.c  | 18 ++
   drivers/gpu/drm/i915/intel_display.c | 18 +-
   3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 800230ba1c3b..87388feb973d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3150,8 +3150,9 @@ int i915_gem_object_wait(struct drm_i915_gem_object *obj,
struct intel_rps_client *rps);
   int i915_gem_object_wait_priority(struct drm_i915_gem_object *obj,
 unsigned int flags,
-   int priority);
+   int priority, unsigned int timeout);
   #define I915_PRIORITY_DISPLAY I915_PRIORITY_MAX
+#define I915_PREEMPTION_TIMEOUT_DISPLAY (20 * 1000 * 1000) /* 20 ms */
   
   int __must_check

   i915_gem_object_set_to_wc_domain(struct drm_i915_gem_object *obj, bool 
write);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 675d6bb59337..252c6b58e739 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -469,7 +469,8 @@ i915_gem_object_wait_reservation(struct reservation_object 
*resv,
   return timeout;
   }
   
-static void __fence_set_priority(struct dma_fence *fence, int prio)

+static void __fence_set_priority(struct dma_fence *fence,
+  int prio, unsigned int timeout)
   {
   struct i915_request *rq;
   struct intel_engine_cs *engine;
@@ -482,11 +483,12 @@ static void __fence_set_priority(struct dma_fence *fence, 
int prio)
   
   rcu_read_lock();

   if (engine->schedule)
- engine->schedule(rq, prio, 0);
+ engine->schedule(rq, prio, timeout);
   rcu_read_unlock();
   }
   
-static void fence_set_priority(struct dma_fence *fence, int prio)

+static void fence_set_priority(struct dma_fence *fence,
+int prio, unsigned int timeout)
   {
   /* Recurse once into a fence-array */
   if (dma_fence_is_array(fence)) {
@@ -494,16 +496,16 @@ static void fence_set_priority(struct dma_fence *fence, 
int prio)
   int i;
   
   for (i = 0; i < array->num_fences; i++)

- __fence_set_priority(array->fences[i], prio);
+ __fence_set_priority(array->fences[i], prio, timeout);
   } else {
- __fence_set_priority(fence, prio);
+ __fence_set_priority(fence, prio, timeout);
   }
   }
   
   int

   i915_gem_object_wait_priority(struct drm_i915_gem_object *obj,
 unsigned int flags,
-   int prio)
+   int prio, unsigned int timeout)
   {
   struct dma_fence *excl;
   
@@ -518,7 +520,7 @@ i915_gem_object_wait_priority(struct drm_i915_gem_object *obj,

   return ret;
   
   for (i = 0; i < count; i++) {

- fence_set_priority(shared[i], prio);
+ fence_set_priority(shared[i], prio, timeout);
   dma_fence_put(shared[i]);
   }
   
@@ -528,7 +530,7 @@ i915_gem_object_wait_priority(struct drm_i915_gem_object *obj,

   }
   
   if (excl) {

- fence_set_priority(excl, prio);
+ fence_set_priority(excl, prio, timeout);
   dma_fence_put(excl);
   }
   return 0;
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 4c30c7c04f9c..830f2d4e540f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12786,7 +12786,23 @@ intel_prepare_plane_fb(struct drm_plane *plane,
   
   ret = intel_plane_pin_fb(to_intel_plane_state(new_state));
   
- i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DISPLAY);

+ /*
+  * Reschedule our dependencies, and ensure we run within a timeout.
+  *
+  * Note that if the timeout is exceeded, then whoever was running that
+  * prevented us from acquiring the GPU is declared rogue and reset. An
+  * unresponsive process will then be banned in order to preserve
+  * interactivity. Since this can be seen as a bit heavy-handed, we
+  * select a timeout for when the dropped frames start to become a
+  * noticeable nuisance for the 

Re: [Intel-gfx] [PATCH 10/11] drm/i915: Use a preemption timeout to enforce interactivity

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-03-27 09:35:17)
> 
> On 26/03/2018 12:50, Chris Wilson wrote:
> > Use a liberal timeout of 20ms to ensure that the rendering for an
> > interactive pageflip is started in a timely fashion, and that
> > user interaction is not blocked by GPU, or CPU, hogs. This is at the cost
> > of resetting whoever was blocking the preemption, likely leading to that
> > context/process being banned from submitting future requests.
> > 
> > Signed-off-by: Chris Wilson 
> > ---
> >   drivers/gpu/drm/i915/i915_drv.h  |  3 ++-
> >   drivers/gpu/drm/i915/i915_gem.c  | 18 ++
> >   drivers/gpu/drm/i915/intel_display.c | 18 +-
> >   3 files changed, 29 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 800230ba1c3b..87388feb973d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3150,8 +3150,9 @@ int i915_gem_object_wait(struct drm_i915_gem_object 
> > *obj,
> >struct intel_rps_client *rps);
> >   int i915_gem_object_wait_priority(struct drm_i915_gem_object *obj,
> > unsigned int flags,
> > -   int priority);
> > +   int priority, unsigned int timeout);
> >   #define I915_PRIORITY_DISPLAY I915_PRIORITY_MAX
> > +#define I915_PREEMPTION_TIMEOUT_DISPLAY (20 * 1000 * 1000) /* 20 ms */
> >   
> >   int __must_check
> >   i915_gem_object_set_to_wc_domain(struct drm_i915_gem_object *obj, bool 
> > write);
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> > b/drivers/gpu/drm/i915/i915_gem.c
> > index 675d6bb59337..252c6b58e739 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -469,7 +469,8 @@ i915_gem_object_wait_reservation(struct 
> > reservation_object *resv,
> >   return timeout;
> >   }
> >   
> > -static void __fence_set_priority(struct dma_fence *fence, int prio)
> > +static void __fence_set_priority(struct dma_fence *fence,
> > +  int prio, unsigned int timeout)
> >   {
> >   struct i915_request *rq;
> >   struct intel_engine_cs *engine;
> > @@ -482,11 +483,12 @@ static void __fence_set_priority(struct dma_fence 
> > *fence, int prio)
> >   
> >   rcu_read_lock();
> >   if (engine->schedule)
> > - engine->schedule(rq, prio, 0);
> > + engine->schedule(rq, prio, timeout);
> >   rcu_read_unlock();
> >   }
> >   
> > -static void fence_set_priority(struct dma_fence *fence, int prio)
> > +static void fence_set_priority(struct dma_fence *fence,
> > +int prio, unsigned int timeout)
> >   {
> >   /* Recurse once into a fence-array */
> >   if (dma_fence_is_array(fence)) {
> > @@ -494,16 +496,16 @@ static void fence_set_priority(struct dma_fence 
> > *fence, int prio)
> >   int i;
> >   
> >   for (i = 0; i < array->num_fences; i++)
> > - __fence_set_priority(array->fences[i], prio);
> > + __fence_set_priority(array->fences[i], prio, timeout);
> >   } else {
> > - __fence_set_priority(fence, prio);
> > + __fence_set_priority(fence, prio, timeout);
> >   }
> >   }
> >   
> >   int
> >   i915_gem_object_wait_priority(struct drm_i915_gem_object *obj,
> > unsigned int flags,
> > -   int prio)
> > +   int prio, unsigned int timeout)
> >   {
> >   struct dma_fence *excl;
> >   
> > @@ -518,7 +520,7 @@ i915_gem_object_wait_priority(struct 
> > drm_i915_gem_object *obj,
> >   return ret;
> >   
> >   for (i = 0; i < count; i++) {
> > - fence_set_priority(shared[i], prio);
> > + fence_set_priority(shared[i], prio, timeout);
> >   dma_fence_put(shared[i]);
> >   }
> >   
> > @@ -528,7 +530,7 @@ i915_gem_object_wait_priority(struct 
> > drm_i915_gem_object *obj,
> >   }
> >   
> >   if (excl) {
> > - fence_set_priority(excl, prio);
> > + fence_set_priority(excl, prio, timeout);
> >   dma_fence_put(excl);
> >   }
> >   return 0;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 4c30c7c04f9c..830f2d4e540f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12786,7 +12786,23 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> >   
> >   ret = intel_plane_pin_fb(to_intel_plane_state(new_state));
> >   
> > - i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DISPLAY);
> > + /*
> > +  * Reschedule our dependencies, and ensure we run within a timeout.
> > +  *
> > +  * Note that if the timeout is 

Re: [Intel-gfx] [PATCH 10/11] drm/i915: Use a preemption timeout to enforce interactivity

[Tvrtko Ursulin]

On 26/03/2018 12:50, Chris Wilson wrote:

Use a liberal timeout of 20ms to ensure that the rendering for an
interactive pageflip is started in a timely fashion, and that
user interaction is not blocked by GPU, or CPU, hogs. This is at the cost
of resetting whoever was blocking the preemption, likely leading to that
context/process being banned from submitting future requests.

Signed-off-by: Chris Wilson 
---
  drivers/gpu/drm/i915/i915_drv.h  |  3 ++-
  drivers/gpu/drm/i915/i915_gem.c  | 18 ++
  drivers/gpu/drm/i915/intel_display.c | 18 +-
  3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 800230ba1c3b..87388feb973d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3150,8 +3150,9 @@ int i915_gem_object_wait(struct drm_i915_gem_object *obj,
 struct intel_rps_client *rps);
  int i915_gem_object_wait_priority(struct drm_i915_gem_object *obj,
  unsigned int flags,
- int priority);
+ int priority, unsigned int timeout);
  #define I915_PRIORITY_DISPLAY I915_PRIORITY_MAX
+#define I915_PREEMPTION_TIMEOUT_DISPLAY (20 * 1000 * 1000) /* 20 ms */
  
  int __must_check

  i915_gem_object_set_to_wc_domain(struct drm_i915_gem_object *obj, bool write);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 675d6bb59337..252c6b58e739 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -469,7 +469,8 @@ i915_gem_object_wait_reservation(struct reservation_object 
*resv,
return timeout;
  }
  
-static void __fence_set_priority(struct dma_fence *fence, int prio)

+static void __fence_set_priority(struct dma_fence *fence,
+int prio, unsigned int timeout)
  {
struct i915_request *rq;
struct intel_engine_cs *engine;
@@ -482,11 +483,12 @@ static void __fence_set_priority(struct dma_fence *fence, 
int prio)
  
  	rcu_read_lock();

if (engine->schedule)
-   engine->schedule(rq, prio, 0);
+   engine->schedule(rq, prio, timeout);
rcu_read_unlock();
  }
  
-static void fence_set_priority(struct dma_fence *fence, int prio)

+static void fence_set_priority(struct dma_fence *fence,
+  int prio, unsigned int timeout)
  {
/* Recurse once into a fence-array */
if (dma_fence_is_array(fence)) {
@@ -494,16 +496,16 @@ static void fence_set_priority(struct dma_fence *fence, 
int prio)
int i;
  
  		for (i = 0; i < array->num_fences; i++)

-   __fence_set_priority(array->fences[i], prio);
+   __fence_set_priority(array->fences[i], prio, timeout);
} else {
-   __fence_set_priority(fence, prio);
+   __fence_set_priority(fence, prio, timeout);
}
  }
  
  int

  i915_gem_object_wait_priority(struct drm_i915_gem_object *obj,
  unsigned int flags,
- int prio)
+ int prio, unsigned int timeout)
  {
struct dma_fence *excl;
  
@@ -518,7 +520,7 @@ i915_gem_object_wait_priority(struct drm_i915_gem_object *obj,

return ret;
  
  		for (i = 0; i < count; i++) {

-   fence_set_priority(shared[i], prio);
+   fence_set_priority(shared[i], prio, timeout);
dma_fence_put(shared[i]);
}
  
@@ -528,7 +530,7 @@ i915_gem_object_wait_priority(struct drm_i915_gem_object *obj,

}
  
  	if (excl) {

-   fence_set_priority(excl, prio);
+   fence_set_priority(excl, prio, timeout);
dma_fence_put(excl);
}
return 0;
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 4c30c7c04f9c..830f2d4e540f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12786,7 +12786,23 @@ intel_prepare_plane_fb(struct drm_plane *plane,
  
  	ret = intel_plane_pin_fb(to_intel_plane_state(new_state));
  
-	i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DISPLAY);

+   /*
+* Reschedule our dependencies, and ensure we run within a timeout.
+*
+* Note that if the timeout is exceeded, then whoever was running that
+* prevented us from acquiring the GPU is declared rogue and reset. An
+* unresponsive process will then be banned in order to preserve
+* interactivity. Since this can be seen as a bit heavy-handed, we
+* select a timeout for when the dropped frames start to become a
+* noticeable nuisance for the user (20 ms, i.e. preemption was blocked
+* for more than a frame). Note, this