[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 <chris at chris-wilson.co.uk>
> > ---
> >   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 is only a timeout for a delay in
> > +      * preempting the current request in order to run our dependency chain,
> > +      * our dependency chain may itself take a long time to run to completion
> > +      * before we can present the framebuffer.
> > +      */
> > +     i915_gem_object_wait_priority(obj, 0,
> > +                                   I915_PRIORITY_DISPLAY,
> > +                                   I915_PREEMPTION_TIMEOUT_DISPLAY);
> 
> API signature changes to allow timeouts are fine, but I think this hunk 
> should be separate patch at the end of the series. (Since it has the 
> potential to make things which used to work, albeit stutteringly (?), 
> start crashing.

It is at the end as a separate patch. What am I missing?
(The only thing after it is exposing a param to userspace.)
-Chris