[Intel-gfx] [PATCH 38/55] drm/i915: Prepare i915_gem_active for annotations

[Chris Wilson]

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

Not touching the long lines in intermediate patches for code that will
be deleted.

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

What I always had was requests for member names, rq for locals. But
apparently rq was too much like run-queue, which is how we use the
requests! In userspace I always used requests for members and rq for
locals.

Where I have been adding functions, I have been using request as the
local as well. Where I have been inheriting the ugly name, it has mostly
stuck.
 
> > +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.

They are (or will be) only used in a couple of places where the NULL
guard is required.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre