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

Joonas Lahtinen
2016-07-26T08:50:26Z

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