On Wed, Jul 27, 2016 at 09:04:03AM +0300, Joonas Lahtinen wrote:
> On ma, 2016-07-25 at 18:32 +0100, Chris Wilson wrote:
> > Tidy up the for loops that handle waiting for read/write vs read-only
> > access.
> >
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_gem.c | 158 +++++++++++++++++++---------------------
> > 1 file changed, 75 insertions(+), 83 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 3f6b69dcaccb..2d86a0c3f295 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1339,6 +1339,23 @@ put_rpm:
> > return ret;
> > }
> >
> > +static void
> > +i915_gem_object_retire_request(struct drm_i915_gem_object *obj,
> > + struct drm_i915_gem_request *req)
> > +{
> > + int idx = req->engine->id;
> > +
> > + if (i915_gem_active_peek(&obj->last_read[idx],
> > + &obj->base.dev->struct_mutex) == req)
> > + i915_gem_object_retire__read(obj, idx);
> > + else if (i915_gem_active_peek(&obj->last_write,
> > + &obj->base.dev->struct_mutex) == req)
> > + i915_gem_object_retire__write(obj);
>
> If these functions will use same mutex (be it different than
> struct_mutex) in all invocations, I'd make an alias for it.
>
> > +
> > + if (!i915_reset_in_progress(&req->i915->gpu_error))
> > + i915_gem_request_retire_upto(req);
> > +}
> > +
> > /**
> > * Ensures that all rendering to the object has completed and the object is
> > * safe to unbind from the GTT or access from the CPU.
> > @@ -1349,39 +1366,34 @@ int
> > i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj,
> > bool readonly)
> > {
> > - struct drm_i915_gem_request *request;
> > struct reservation_object *resv;
> > - int ret, i;
> > + struct i915_gem_active *active;
> > + unsigned long active_mask;
> > + int idx, ret;
> >
>
> We could do early exit here based on the active_mask, like with the
> nonblocking version.
Nope. Because here we have to worry about third parties, we can't exit
early.
> > - if (readonly) {
> > - request = i915_gem_active_peek(&obj->last_write,
> > - &obj->base.dev->struct_mutex);
> > - if (request) {
> > - ret = i915_wait_request(request);
> > - if (ret)
> > - return ret;
> > + lockdep_assert_held(&obj->base.dev->struct_mutex);
> >
> > - i = request->engine->id;
> > - if (i915_gem_active_peek(&obj->last_read[i],
> > - &obj->base.dev->struct_mutex) == request)
> > - i915_gem_object_retire__read(obj, i);
> > - else
> > - i915_gem_object_retire__write(obj);
> > - }
> > + if (!readonly) {
>
> Not sure why not just swap and keep this if (readonly)...
Consistency. The others did !readonly, and this was the odd one out.
>
> > + active = obj->last_read;
> > + active_mask = obj->active;
> > } else {
> > - for (i = 0; i < I915_NUM_ENGINES; i++) {
> > - request = i915_gem_active_peek(&obj->last_read[i],
> > - &obj->base.dev->struct_mutex);
> > - if (!request)
> > - continue;
> > + active_mask = 1;
>
> Wouldn't we have RENDER_RING define for this and other instances?
?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
