Re: [Intel-gfx] [PATCH 03/18] drm/i915: Rearrange i915_wait_request() accounting with callers

[Joonas Lahtinen]

On ke, 2016-09-14 at 07:52 +0100, Chris Wilson wrote:
> +int
> +i915_gem_object_wait(struct drm_i915_gem_object *obj,
> +  unsigned int flags,
> +  long timeout,
> +  struct intel_rps_client *rps)
>  {
>  

[...]

> - return 0;
> + resv = i915_gem_object_get_dmabuf_resv(obj);
> + if (resv)
> + timeout = i915_gem_object_wait_reservation(resv,
> +    flags, timeout,
> +    rps);
> + return timeout < 0 ? timeout : timeout > 0 ? 0 : -ETIME;

Format this in a more readable manner eg.;

return timeout == 0 ? -ETIME :
       timeout < 0 ? timeout :
       0;

>  
>  static struct intel_rps_client *to_rps_client(struct drm_file *file)
> @@ -454,7 +542,13 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
>   /* We manually control the domain here and pretend that it
>    * remains coherent i.e. in the GTT domain, like shmem_pwrite.
>    */
> - ret = i915_gem_object_wait_rendering(obj, false);
> + lockdep_assert_held(>base.dev->struct_mutex);

Bump this before the comment to the beginning of function like
elsehwere.

> @@ -2804,17 +2923,21 @@ i915_gem_wait_ioctl(struct drm_device *dev, void 
> *data, struct drm_file *file)
>   if (!obj)
>   return -ENOENT;
>  
> - active = __I915_BO_ACTIVE(obj);
> - for_each_active(active, idx) {
> - s64 *timeout = args->timeout_ns >= 0 ? >timeout_ns : NULL;
> - ret = i915_gem_active_wait_unlocked(>last_read[idx],
> - I915_WAIT_INTERRUPTIBLE,
> - timeout, rps);
> - if (ret)
> - break;
> + start = ktime_get();
> +
> + ret = i915_gem_object_wait(obj,
> +    I915_WAIT_INTERRUPTIBLE | I915_WAIT_ALL,
> +    args->timeout_ns < 0 ? MAX_SCHEDULE_TIMEOUT 
> : nsecs_to_jiffies(args->timeout_ns),

Do break this line, plz.

Maybe just have long timeout = MAX_SCHEDULE_TIMEOUT; in the beginning
of file, then do if (args->timeout_ns >= 0) before the function, it
matches the after function if nicely.


> +    to_rps_client(file));
> +
> + if (args->timeout_ns > 0) {

And as we have this.

> + args->timeout_ns -= ktime_to_ns(ktime_sub(ktime_get(), start));
> + if (args->timeout_ns < 0)
> + args->timeout_ns = 0;
>   }
>  
>   i915_gem_object_put_unlocked(obj);
> +
>   return ret;
>  }
> 



> 
> @@ -3598,7 +3732,13 @@ i915_gem_object_set_to_cpu_domain(struct 
> drm_i915_gem_object *obj, bool write)
>   uint32_t old_write_domain, old_read_domains;
>   int ret;
>  
> - ret = i915_gem_object_wait_rendering(obj, !write);
> + lockdep_assert_held(>base.dev->struct_mutex);

I'd add a newline here like elsewhere.

> + ret = i915_gem_object_wait(obj,
> +    I915_WAIT_INTERRUPTIBLE |
> +    I915_WAIT_LOCKED |
> +    (write ? I915_WAIT_ALL : 0),
> +    MAX_SCHEDULE_TIMEOUT,
> +    NULL);
>   if (ret)
>   return ret;
>  
> @@ -3654,11 +3794,7 @@ i915_gem_ring_throttle(struct drm_device *dev, struct 
> drm_file *file)
>   struct drm_i915_file_private *file_priv = file->driver_priv;
>   unsigned long recent_enough = jiffies - DRM_I915_THROTTLE_JIFFIES;
>   struct drm_i915_gem_request *request, *target = NULL;
> - int ret;
> -
> - ret = i915_gem_wait_for_error(_priv->gpu_error);
> - if (ret)
> - return ret;

Unsure how this is related to the changes, need to explain in commit
message or I nominate this a lost hunk.

With those addressed,

Reviewed-by: Joonas Lahtinen 

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 03/18] drm/i915: Rearrange i915_wait_request() accounting with callers

[Chris Wilson]

Our low-level wait routine has evolved from our generic wait interface
that handled unlocked, RPS boosting, waits with time tracking. If we
push our GEM fence tracking to use reservation_objects (required for
handling multiple timelines), we lose the ability to pass the required
information down to i915_wait_request(). However, if we push the extra
functionality from i915_wait_request() to the individual callsites
(i915_gem_object_wait_rendering and i915_gem_wait_ioctl) that make use
of those extras, we can both simplify our low level wait and prepare for
extending the GEM interface for use of reservation_objects.

* This causes a temporary regression in the use of wait-ioctl as a busy
query -- it will fail to report immediately, but go into
i915_wait_request() before timing out.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/i915_drv.h |   7 +-
 drivers/gpu/drm/i915/i915_gem.c | 300 +++-
 drivers/gpu/drm/i915/i915_gem_request.c | 117 ++---
 drivers/gpu/drm/i915/i915_gem_request.h |  32 ++--
 drivers/gpu/drm/i915/i915_gem_userptr.c |  12 +-
 drivers/gpu/drm/i915/intel_display.c|  34 ++--
 drivers/gpu/drm/i915/intel_ringbuffer.c |  14 +-
 drivers/gpu/drm/i915/intel_ringbuffer.h |   3 +-
 8 files changed, 283 insertions(+), 236 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1e2dda88a483..af0212032b64 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3274,9 +3274,10 @@ int __must_check i915_gem_wait_for_idle(struct 
drm_i915_private *dev_priv,
 int __must_check i915_gem_suspend(struct drm_device *dev);
 void i915_gem_resume(struct drm_device *dev);
 int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf);
-int __must_check
-i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj,
-  bool readonly);
+int i915_gem_object_wait(struct drm_i915_gem_object *obj,
+unsigned int flags,
+long timeout,
+struct intel_rps_client *rps);
 int __must_check
 i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj,
  bool write);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c8bd02277b7d..d9214c9d31d2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -292,7 +292,12 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj)
 * must wait for all rendering to complete to the object (as unbinding
 * must anyway), and retire the requests.
 */
-   ret = i915_gem_object_wait_rendering(obj, false);
+   ret = i915_gem_object_wait(obj,
+  I915_WAIT_INTERRUPTIBLE |
+  I915_WAIT_LOCKED |
+  I915_WAIT_ALL,
+  MAX_SCHEDULE_TIMEOUT,
+  NULL);
if (ret)
return ret;
 
@@ -311,88 +316,171 @@ int i915_gem_object_unbind(struct drm_i915_gem_object 
*obj)
return ret;
 }
 
-/**
- * Ensures that all rendering to the object has completed and the object is
- * safe to unbind from the GTT or access from the CPU.
- * @obj: i915 gem object
- * @readonly: waiting for just read access or read-write access
- */
-int
-i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj,
-  bool readonly)
+static long
+i915_gem_object_wait_fence(struct fence *fence,
+  unsigned int flags,
+  long timeout,
+  struct intel_rps_client *rps)
 {
-   struct reservation_object *resv;
-   struct i915_gem_active *active;
-   unsigned long active_mask;
-   int idx;
+   struct drm_i915_gem_request *rq;
 
-   lockdep_assert_held(>base.dev->struct_mutex);
+   BUILD_BUG_ON(I915_WAIT_INTERRUPTIBLE != 0x1);
 
-   if (!readonly) {
-   active = obj->last_read;
-   active_mask = i915_gem_object_get_active(obj);
-   } else {
-   active_mask = 1;
-   active = >last_write;
+   if (test_bit(FENCE_FLAG_SIGNALED_BIT, >flags))
+   return timeout;
+
+   if (!fence_is_i915(fence))
+   return fence_wait_timeout(fence,
+ flags & I915_WAIT_INTERRUPTIBLE,
+ timeout);
+
+   rq = to_request(fence);
+   if (i915_gem_request_completed(rq))
+   goto out;
+
+   /* This client is about to stall waiting for the GPU. In many cases
+* this is undesirable and limits the throughput of the system, as
+* many clients cannot continue processing user input/output whilst
+* blocked. RPS autotuning may take tens of milliseconds to respond
+