[Intel-gfx] [PATCH 37/55] drm/i915: Introduce i915_gem_active for request tracking

[Chris Wilson]

On Tue, Jul 26, 2016 at 11:23:44AM +0300, Joonas Lahtinen wrote:
> 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.

Don't care in this patch.

> 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)?

Not in this patch.
>  
> > +/* 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.

No. The active.request is the fence, this keeps track of the most recent
fence we are interested in.

> > +	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.

That was against my wishes. This patch was only to introduce the new
struct, actually making use of it comes later.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre