Re: [PATCH 38/55] drm/i915: Prepare i915_gem_active for annotations

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

On ma, 2016-07-25 at 18:32 +0100, Chris Wilson wrote:
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 98dc97c8c2bf..b8d541f212ff 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1349,27 +1349,30 @@ int
>  i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj,
>  			       bool readonly)
>  {
> +	struct drm_i915_gem_request *request;

'req' is rather de facto. One bad name is better than two names of any
grade. I see much of your new code is with *request, which direction
should we have?

>  
> @@ -2383,8 +2386,8 @@ void i915_vma_move_to_active(struct i915_vma *vma,
>  static void
>  i915_gem_object_retire__write(struct drm_i915_gem_object *obj)
>  {
> -	GEM_BUG_ON(!obj->last_write.request);
> -	GEM_BUG_ON(!(obj->active & intel_engine_flag(obj->last_write.request->engine)));
> +	GEM_BUG_ON(!__i915_gem_active_is_busy(&obj->last_write));
> +	GEM_BUG_ON(!(obj->active & intel_engine_flag(i915_gem_active_get_engine(&obj->last_write))));

Already mentioned in previous, long line. You added new functions to
_gem_active which do useful stuff, then you could nuke the dummy ones?

> @@ -2621,7 +2626,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *engine)
>  				       struct drm_i915_gem_object,
>  				       engine_list[engine->id]);
>  
> -		if (!list_empty(&obj->last_read[engine->id].request->list))
> +		if (!list_empty(&i915_gem_active_peek(&obj->last_read[engine->id])->list))

Long line.

>  			break;
>  
>  		i915_gem_object_retire__read(obj, engine->id);
> @@ -2754,7 +2759,7 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj)
>  	for (i = 0; i < I915_NUM_ENGINES; i++) {
>  		struct drm_i915_gem_request *req;
>  
> -		req = obj->last_read[i].request;
> +		req = i915_gem_active_peek(&obj->last_read[i]);
>  		if (req == NULL)
>  			continue;
>  
> @@ -2794,7 +2799,7 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>  {
>  	struct drm_i915_gem_wait *args = data;
>  	struct drm_i915_gem_object *obj;
> -	struct drm_i915_gem_request *req[I915_NUM_ENGINES];
> +	struct drm_i915_gem_request *requests[I915_NUM_ENGINES];

I think this answers my previous question.

<SNIP>

> +		struct drm_i915_gem_request *req;
>  
>  
>  	n = 0;
>  	if (readonly) {
> -		if (obj->last_write.request)
> -			req[n++] = obj->last_write.request;
> +		struct drm_i915_gem_request *req;
> +
> +		req = i915_gem_active_peek(&obj->last_write);
> +		if (req)
> +			requests[n++] = req;
>  	} else {
> -		for (i = 0; i < I915_NUM_ENGINES; i++)
> -			if (obj->last_read[i].request)
> -				req[n++] = obj->last_read[i].request;
> +		for (i = 0; i < I915_NUM_ENGINES; i++) {
> +			struct drm_i915_gem_request *req;

But some consistency is lacking with dem names. How's it going to be?

> +static inline uint32_t
> +i915_gem_active_get_seqno(const struct i915_gem_active *active)
> +{
> +	return i915_gem_request_get_seqno(i915_gem_active_peek(active));

Nuke the i915_gem_request_get_seqno wrapper, it's insanity. Now or an
another patch.

> +}
> +
> +static inline struct intel_engine_cs *
> +i915_gem_active_get_engine(const struct i915_gem_active *active)
> +{
> +	return i915_gem_request_get_engine(i915_gem_active_peek(active));

Ditto.

With the nitpicking,

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