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(&obj->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 ? &args->timeout_ns : NULL;
> - ret = i915_gem_active_wait_unlocked(&obj->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;
> }
>
<SNIP>
>
> @@ -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(&obj->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(&dev_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 <joonas.lahti...@linux.intel.com>
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
