Re: [PATCH 40/55] drm/i915: Refactor blocking waits

Joonas Lahtinen
2016-07-27T06:04:05Z

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@chris-wilson.co.uk>
> ---
>  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.

> - 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)...

> + 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?

With above addressed,

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation