[Intel-gfx] [PATCH 12/27] drm/i915: Move request runtime management onto gt

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-09-25 11:57:53)
> 
> On 25/09/2019 11:01, Chris Wilson wrote:
> > @@ -423,6 +424,7 @@ void i915_gem_object_release_mmap(struct drm_i915_gem_object *obj)
> >   static int create_mmap_offset(struct drm_i915_gem_object *obj)
> >   {
> >       struct drm_i915_private *i915 = to_i915(obj->base.dev);
> > +     struct intel_gt *gt = &i915->gt;
> >       int err;
> >   
> >       err = drm_gem_create_mmap_offset(&obj->base);
> > @@ -431,7 +433,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, MAX_SCHEDULE_TIMEOUT);
> > +             err = intel_gt_wait_for_idle(gt, MAX_SCHEDULE_TIMEOUT);
> 
> It may be useful to keep the i915_gem_wait_for_idle as trivial wrapper 
> which can then call intel_gt_wait_for_idle. Keeps the separation of GEM 
> and GT better, but also..
> >               if (err)
> >                       break;
> >   
> > @@ -440,7 +442,7 @@ static int create_mmap_offset(struct drm_i915_gem_object *obj)
> >               if (!err)
> >                       break;
> >   
> > -     } while (flush_delayed_work(&i915->gem.retire_work));
> > +     } while (flush_delayed_work(&gt->requests.retire_work));
> 
> .. here could be considered a layering violation that GEM is peeking 
> into GT internals like this. Shall we have another wrapper like 
> i915_gem_retire_requests_sync which would call intel_gt_retire_requests 
> and new intel_gt_flush_retire, or something?

This is the most complicated case because we're managing a global
resource. Actually this loop can be eliminated, as it was only trying to
avoid taking struct_mutex here (using the worker instead). Now that
we can call retire directly, it can be reduced.

After which I think the point is moot as the path forward is clearer.

> > @@ -180,8 +178,6 @@ struct drm_i915_private *mock_gem_device(void)
> >   
> >       mock_init_contexts(i915);
> >   
> > -     INIT_DELAYED_WORK(&i915->gem.retire_work, mock_retire_work_handler);
> > -
> >       i915->gt.awake = -1;
> >   
> >       intel_timelines_init(i915);
> > 
> 
> I am still slightly uneasy about having requests, which I see as a GEM 
> concept, be retired from GT, but okay, it's not the most important issue 
> at the moment.

requests are not a GEM concept, they belong in sched/ :)
-Chris