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@xxxxxxxxxxxxxxx>
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
