On Wed, Jul 27, 2016 at 10:40:14AM +0300, Joonas Lahtinen wrote:
> On ma, 2016-07-25 at 18:32 +0100, Chris Wilson wrote:
> > static void
> > -i915_gem_object_retire__write(struct drm_i915_gem_object *obj)
> > +i915_gem_object_retire__fence(struct i915_gem_active *active,
> > + struct drm_i915_gem_request *req)
> > {
> > - GEM_BUG_ON(!__i915_gem_active_is_busy(&obj->last_write));
> > - GEM_BUG_ON(!(obj->active &
> > - intel_engine_flag(i915_gem_active_get_engine(&obj->last_write,
> > - &obj->base.dev->struct_mutex))));
> > +}
> >
>
> An empty function? Could have at least a comment why currently empty.
To avoid the branch inside the retirement loop for an infrequent call.
>
> > - i915_gem_active_set(&obj->last_write, NULL);
> > - intel_fb_obj_flush(obj, true, ORIGIN_CS);
> > +static void
> > +i915_gem_object_retire__write(struct i915_gem_active *active,
> > + struct drm_i915_gem_request *request)
> > +{
> > + intel_fb_obj_flush(container_of(active,
> > + struct drm_i915_gem_object,
> > + last_write),
>
> Add a function, manual container_of are horrible. And do it in the
> beginning of a function as a separate line, too.
>
> > + true,
> > + ORIGIN_CS);
> > }
> >
> > static void
> > -i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int idx)
> > +i915_gem_object_retire__read(struct i915_gem_active *active,
> > + struct drm_i915_gem_request *request)
> > {
> > - struct intel_engine_cs *engine;
> > + int idx = request->engine->id;
> > + struct drm_i915_gem_object *obj =
> > + container_of(active, struct drm_i915_gem_object, last_read[idx]);
>
> Ditto.
No.
>
> > struct i915_vma *vma;
> >
> > - GEM_BUG_ON(!__i915_gem_active_is_busy(&obj->last_read[idx]));
> > - GEM_BUG_ON(!(obj->active & (1 << idx)));
> > -
> > - list_del_init(&obj->engine_list[idx]);
> > - i915_gem_active_set(&obj->last_read[idx], NULL);
> > -
> > - engine = i915_gem_active_get_engine(&obj->last_write,
> > - &obj->base.dev->struct_mutex);
> > - if (engine && engine->id == idx)
> > - i915_gem_object_retire__write(obj);
> > + GEM_BUG_ON((obj->active & (1 << idx)) == 0);
>
> BIT() or maybe even ENGINE_MASK() when we have such a beauty. Or do you
> intend to make this about something else but engines eventually?
Yes. You will get to those patches eventually.
>
> >
> > obj->active &= ~(1 << idx);
> > if (obj->active)
> > @@ -2419,15 +2384,13 @@ i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int idx)
> > * so that we don't steal from recently used but inactive objects
> > * (unless we are forced to ofc!)
> > */
> > - list_move_tail(&obj->global_list,
> > - &to_i915(obj->base.dev)->mm.bound_list);
> > + list_move_tail(&obj->global_list, &request->i915->mm.bound_list);
>
> As a follow-up s/global_list/global_link/?
>
> > void i915_gem_retire_requests(struct drm_i915_private *dev_priv)
> > @@ -2818,8 +2742,7 @@ out:
> > }
> >
> > static int
> > -__i915_gem_object_sync(struct drm_i915_gem_object *obj,
> > - struct drm_i915_gem_request *to,
> > +__i915_gem_object_sync(struct drm_i915_gem_request *to,
> > struct drm_i915_gem_request *from)
> > {
> > int ret;
> > @@ -2827,9 +2750,6 @@ __i915_gem_object_sync(struct drm_i915_gem_object *obj,
> > if (to->engine == from->engine)
> > return 0;
> >
> > - if (i915_gem_request_completed(from))
> > - return 0;
> > -
>
> Why remove the early exit?
I made it redundant. There is also a small improvement by using the
sema-seqno tracking (which actually applies equally to the bdw
!semaphore path, more on that later).
> > @@ -172,6 +176,24 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request)
> > */
> > request->ring->last_retired_head = request->postfix;
> >
> > + /* Walk through the active list, calling retire on each. This allows
> > + * objects to track their GPU activity and mark themselves as idle
> > + * when their *last* active request is completed (updating state
> > + * tracking lists for eviction, active references for GEM, etc).
> > + *
> > + * As the ->retire() may free the node, we decouple it first and
> > + * pass along the auxiliary information (to avoid dereferencing
> > + * the node after the callback).
> > + */
> > + list_for_each_entry_safe(active, next, &request->active_list, link) {
> > + prefetchw(next);
>
> Would not this be an improvement to go to list_for_each_entry{,_safe}
> rather?
It's been tried before. It's not a universal improvement. This loop is
one of the top functions in the profiles when handling large request
objects, with perf implying that the memory loads are where our time
goes..
> > +
> > + INIT_LIST_HEAD(&active->link);
> > + active->__request = NULL;
> > +
> > + active->retire(active, request);
> > + }
> > +
> > i915_gem_request_remove_from_client(request);
> >
> > if (request->previous_context) {
> >
> > @@ -705,10 +723,13 @@ int i915_wait_request(struct drm_i915_gem_request *req)
> > {
> > int ret;
> >
> > - GEM_BUG_ON(!req);
> > lockdep_assert_held(&req->i915->drm.struct_mutex);
> > + GEM_BUG_ON(list_empty(&req->link));
>
> Humm, why no waiting on requests without the tracker object? Or then
> need to use __i915_wait_request? Kerneldoc might be useful.
?
-BUG_ON(!req) we declare it as non-null and a NULL pointer dereference
oops isn't much harder than a BUG to pinpoint.
+list_empty() is an assertion against waiting on a retired request.
> > i915_gem_active_peek(const struct i915_gem_active *active, struct mutex *mutex)
> > {
> > - return active->__request;
> > + struct drm_i915_gem_request *request;
> > +
> > + request = active->__request;
> > + if (!request || i915_gem_request_completed(request))
> > + return NULL;
>
> I see early exit was kinda migrated here.
Kinda, but this is also a big difference in behaviour.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
