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

[Joonas Lahtinen]

On ti, 2016-07-26 at 09:28 +0100, Chris Wilson wrote:
> 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.

As noticed already in later patch the lines get removed,

Reviewed-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>

> -Chris
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation