Re: [PATCH 37/55] drm/i915: Introduce i915_gem_active for request tracking

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

On ma, 2016-07-25 at 18:32 +0100, Chris Wilson wrote:
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -155,10 +155,10 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>  		   obj->base.write_domain);
>  	for_each_engine_id(engine, dev_priv, id)
>  		seq_printf(m, "%x ",
> -				i915_gem_request_get_seqno(obj->last_read_req[id]));
> +			   i915_gem_request_get_seqno(obj->last_read[id].request));

I hate i915_gem_request_get_seqno already, it's just NULL protection,
but subject to different patch. Although, I see you got rid
of i915_gem_request_get_engine already.

> @@ -2383,10 +2383,10 @@ 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_req == NULL);
> -	GEM_BUG_ON(!(obj->active & intel_engine_flag(obj->last_write_req->engine)));
> +	GEM_BUG_ON(!obj->last_write.request);
> +	GEM_BUG_ON(!(obj->active & intel_engine_flag(obj->last_write.request->engine)));

Over 80 ch line.

intel_engine_flag seems rather dull, there's also this thing called
BIT(), but again subject to another series.

> @@ -2395,13 +2395,13 @@ i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int idx)
>  {
>  	struct i915_vma *vma;
>  
> -	GEM_BUG_ON(obj->last_read_req[idx] == NULL);
> +	GEM_BUG_ON(!obj->last_read[idx].request);
>  	GEM_BUG_ON(!(obj->active & (1 << idx)));

BIT(idx)?
 
> +/* We treat requests as fences. This is not be to confused with our
> + * "fence registers" but pipeline synchronisation objects ala GL_ARB_sync.
> + * We use the fences to synchronize access from the CPU with activity on the
> + * GPU, for example, we should not rewrite an object's PTE whilst the GPU
> + * is reading them. We also track fences at a higher level to provide
> + * implicit synchronisation around GEM objects, e.g. set-domain will wait
> + * for outstanding GPU rendering before marking the object ready for CPU
> + * access, or a pageflip will wait until the GPU is complete before showing
> + * the frame on the scanout.
> + *
> + * In order to use a fence, the object must track the fence it needs to
> + * serialise with. For example, GEM objects want to track both read and
> + * write access so that we can perform concurrent read operations between
> + * the CPU and GPU engines, as well as waiting for all rendering to
> + * complete, or waiting for the last GPU user of a "fence register". The
> + * object then embeds a @i915_gem_active to track the most recent (in
> + * retirment order) request relevant for the desired mode of access.
> + * The @i915_gem_active is updated with i915_gem_request_mark_active() to
> + * track the most recent fence request, typically this is done as part of
> + * i915_vma_move_to_active().
> + *
> + * When the @i915_gem_active completes (is retired), it will
> + * signal its completion to the owner through a callback as well as mark
> + * itself as idle (i915_gem_active.request == NULL). The owner
> + * can then perform any action, such as delayed freeing of an active
> + * resource including itself.
> + */
> +struct i915_gem_active {

Not sure if this is a good descriptive struct name. Would not this be
in the sync terminology a fence? active.request reads nicely though.

> +	struct drm_i915_gem_request *request;
> +};
> +
> +static inline void
> +i915_gem_active_set(struct i915_gem_active *active,
> +		    struct drm_i915_gem_request *request)
> +{
> +	i915_gem_request_assign(&active->request, request);
> +}
> +
> +#define for_each_active(mask, idx) \
> +	for (; mask ? idx = ffs(mask) - 1, 1 : 0; mask &= ~(1 << idx))

BIT()

> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11378,7 +11378,7 @@ static bool use_mmio_flip(struct intel_engine_cs *engine,
>  	if (resv && !reservation_object_test_signaled_rcu(resv, false))
>  		return true;
>  
> -	return engine != i915_gem_request_get_engine(obj->last_write_req);
> +	return engine != i915_gem_request_get_engine(obj->last_write.request);

What's been the obsession with NULL protecting simple accessor
functions? Makes the code look overly complicated. One more function to
nuke.

The changes itself make sense, with above tweaks;

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