[Intel-gfx] [PATCH 19/21] drm/i915: Merge wait_for_timelines with retire_request

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-09-24 16:57:12)
> 
> On 02/09/2019 05:03, Chris Wilson wrote:
> > wait_for_timelines is essentially the same loop as retiring requests
> > (with an extra), so merge the two into one routine.
> 
> Extra suspense! :)
> 
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/gem/i915_gem_mman.c      |  4 +-
> >   drivers/gpu/drm/i915/gem/i915_gem_pm.c        |  6 +-
> >   .../drm/i915/gem/selftests/i915_gem_context.c |  4 +-
> >   drivers/gpu/drm/i915/gt/selftest_timeline.c   |  2 +-
> >   drivers/gpu/drm/i915/i915_debugfs.c           |  6 +-
> >   drivers/gpu/drm/i915/i915_drv.h               |  3 +-
> >   drivers/gpu/drm/i915/i915_gem.c               | 68 ++-----------------
> >   drivers/gpu/drm/i915/i915_gem_evict.c         | 12 ++--
> >   drivers/gpu/drm/i915/i915_gem_gtt.c           |  2 +-
> >   drivers/gpu/drm/i915/i915_request.c           | 21 +++++-
> >   drivers/gpu/drm/i915/i915_request.h           |  3 +-
> >   .../gpu/drm/i915/selftests/igt_flush_test.c   |  4 +-
> >   .../gpu/drm/i915/selftests/igt_live_test.c    |  4 +-
> >   .../gpu/drm/i915/selftests/mock_gem_device.c  |  2 +-
> >   14 files changed, 42 insertions(+), 99 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > index 9a8c307c5aeb..761ab0076a6a 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > @@ -429,9 +429,7 @@ static int create_mmap_offset(struct drm_i915_gem_object *obj)
> >   
> >       /* Attempt to reap some mmap space from dead objects */
> >       do {
> > -             err = i915_gem_wait_for_idle(i915,
> > -                                          I915_WAIT_INTERRUPTIBLE,
> > -                                          MAX_SCHEDULE_TIMEOUT);
> > +             err = i915_gem_wait_for_idle(i915, MAX_SCHEDULE_TIMEOUT);
> >               if (err)
> >                       break;
> >   
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> > index e83eed8fa452..afbcf9219267 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> > @@ -25,7 +25,7 @@ static void retire_work_handler(struct work_struct *work)
> >       struct drm_i915_private *i915 =
> >               container_of(work, typeof(*i915), gem.retire_work.work);
> >   
> > -     i915_retire_requests(i915);
> > +     i915_retire_requests(i915, 0);
> 
> Majority of callers end up with ", 0" which looks a bit aesthetically 
> not pleasing. How about you add __i915_retire_requests(i915, timeout) 
> instead for those few callers which need it? Or 
> i915_retire_requests_wait/sync/timeout?
> 
> >   
> >       queue_delayed_work(i915->wq,
> >                          &i915->gem.retire_work,
> > @@ -59,9 +59,7 @@ static bool switch_to_kernel_context_sync(struct intel_gt *gt)
> >   {
> >       bool result = !intel_gt_is_wedged(gt);
> >   
> > -     if (i915_gem_wait_for_idle(gt->i915,
> > -                                I915_WAIT_FOR_IDLE_BOOST,
> > -                                I915_GEM_IDLE_TIMEOUT) == -ETIME) {
> > +     if (i915_gem_wait_for_idle(gt->i915, I915_GEM_IDLE_TIMEOUT) == -ETIME) {
> 
> This now ends up interruptible sleep on a driver load path (and probably 
> others) and I am not sure if that is okay. How about we keep the 
> interruptible annotation?

SIGINT during module load should be returned to the user, no?

The only one that is a pain is the broken vtd.

> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 60676de059a7..2b7a4d49b2e6 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -2524,7 +2524,7 @@ void i915_gem_gtt_finish_pages(struct drm_i915_gem_object *obj,
> >       struct i915_ggtt *ggtt = &dev_priv->ggtt;
> >   
> >       if (unlikely(ggtt->do_idle_maps)) {
> > -             if (i915_gem_wait_for_idle(dev_priv, 0, MAX_SCHEDULE_TIMEOUT)) {
> > +             if (i915_retire_requests(dev_priv, MAX_SCHEDULE_TIMEOUT)) {
> 
> Why this couldn't state i915_gem_wait_for_idle?

But there is no i915_gem_wait_for_idle, calling into GEM userspace layer
from here is a layering violation. (You might think this says
i915_gem_gtt.c, but it lies.)