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@xxxxxxxxxxxxxxxxxx>
> > ---
> > 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.)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
