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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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.lahtinen@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