Re: [PATCH 42/55] drm/i915: Remove obsolete i915_gem_object_flush_active()

Joonas Lahtinen
2016-07-26T09:31:32Z

On ma, 2016-07-25 at 18:32 +0100, Chris Wilson wrote:
> Since we track requests, and requests are always added to the GPU fully
> formed, we never have to flush the incomplete request and know that the
> given request will eventually complete without any further action on our
> part.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

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

This series will need plenty of CI regression and benchmark testing...

Regards, Joonas

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 59 +++--------------------------------------
>  1 file changed, 3 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e5be4eb0a397..3bff6b560f97 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2746,35 +2746,6 @@ out_rearm:
>  }
>
>  /**
> - * Ensures that an object will eventually get non-busy by flushing any required
> - * write domains, emitting any outstanding lazy request and retiring and
> - * completed requests.
> - * @obj: object to flush
> - */
> -static int
> -i915_gem_object_flush_active(struct drm_i915_gem_object *obj)
> -{
> - int i;
> -
> - if (!obj->active)
> - return 0;
> -
> - for (i = 0; i < I915_NUM_ENGINES; i++) {
> - struct drm_i915_gem_request *req;
> -
> - req = i915_gem_active_peek(&obj->last_read[i],
> -    &obj->base.dev->struct_mutex);
> - if (req == NULL)
> - continue;
> -
> - if (i915_gem_request_completed(req))
> - i915_gem_object_retire__read(obj, i);
> - }
> -
> - return 0;
> -}
> -
> -/**
>   * i915_gem_wait_ioctl - implements DRM_IOCTL_I915_GEM_WAIT
>   * @dev: drm device pointer
>   * @data: ioctl data blob
> @@ -2820,24 +2791,9 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>   return -ENOENT;
>   }
>
> - /* Need to make sure the object gets inactive eventually. */
> - ret = i915_gem_object_flush_active(obj);
> - if (ret)
> - goto out;
> -
>   if (!obj->active)
>   goto out;
>
> - /* Do this after OLR check to make sure we make forward progress polling
> -  * on this IOCTL with a timeout == 0 (like busy ioctl)
> -  */
> - if (args->timeout_ns == 0) {
> - ret = -ETIME;
> - goto out;
> - }
> -
> - i915_gem_object_put(obj);
> -
>   for (i = 0; i < I915_NUM_ENGINES; i++) {
>   struct drm_i915_gem_request *req;
>
> @@ -2847,6 +2803,8 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>   requests[n++] = req;
>   }
>
> +out:
> + i915_gem_object_put(obj);
>   mutex_unlock(&dev->struct_mutex);
>
>   for (i = 0; i < n; i++) {
> @@ -2857,11 +2815,6 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>   i915_gem_request_put(requests[i]);
>   }
>   return ret;
> -
> -out:
> - i915_gem_object_put(obj);
> - mutex_unlock(&dev->struct_mutex);
> - return ret;
>  }
>
>  static int
> @@ -4036,13 +3989,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
>
>   /* Count all active objects as busy, even if they are currently not used
>    * by the gpu. Users of this interface expect objects to eventually
> -  * become non-busy without any further actions, therefore emit any
> -  * necessary flushes here.
> +  * become non-busy without any further actions.
>    */
> - ret = i915_gem_object_flush_active(obj);
> - if (ret)
> - goto unref;
> -
>   args->busy = 0;
>   if (obj->active) {
>   struct drm_i915_gem_request *req;
> @@ -4060,7 +4008,6 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
>   args->busy |= req->engine->exec_id;
>   }
>
> -unref:
>   i915_gem_object_put(obj);
>  unlock:
>   mutex_unlock(&dev->struct_mutex);
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation