[Intel-gfx] [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests

[Chris Wilson]

On Tue, Jan 12, 2016 at 02:50:28PM +0100, Daniel Vetter wrote:
> On Thu, Jan 07, 2016 at 10:20:50AM +0000, Dave Gordon wrote:
> > There are a number of places where the driver needs a request, but isn't
> > working on behalf of any specific user or in a specific context. At
> > present, we associate them with the per-engine default context. A future
> > patch will abolish those per-engine context pointers; but we can already
> > eliminate a lot of the references to them, just by making the allocator
> > allow NULL as a shorthand for "an appropriate context for this ring",
> > which will mean that the callers don't need to know anything about how
> > the "appropriate context" is found (e.g. per-ring vs per-device, etc).
> > 
> > So this patch renames the existing i915_gem_request_alloc(), and makes
> > it local (static inline), and replaces it with a wrapper that provides
> > a default if the context is NULL, and also has a nicer calling
> > convention (doesn't require a pointer to an output parameter). Then we
> > change all callers to use the new convention:
> > OLD:
> > 	err = i915_gem_request_alloc(ring, user_ctx, &req);
> > 	if (err) ...
> > NEW:
> > 	req = i915_gem_request_alloc(ring, user_ctx);
> > 	if (IS_ERR(req)) ...
> > OLD:
> > 	err = i915_gem_request_alloc(ring, ring->default_context, &req);
> > 	if (err) ...
> > NEW:
> > 	req = i915_gem_request_alloc(ring, NULL);
> > 	if (IS_ERR(req)) ...
> > 
> > Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h            |  6 ++--
> >  drivers/gpu/drm/i915/i915_gem.c            | 55 +++++++++++++++++++++++-------
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 14 +++++---
> >  drivers/gpu/drm/i915/intel_display.c       |  6 ++--
> >  drivers/gpu/drm/i915/intel_lrc.c           |  9 +++--
> >  drivers/gpu/drm/i915/intel_overlay.c       | 24 ++++++-------
> >  6 files changed, 74 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index c6dd4db..c2b000a 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2260,9 +2260,9 @@ struct drm_i915_gem_request {
> >  
> >  };
> >  
> > -int i915_gem_request_alloc(struct intel_engine_cs *ring,
> > -			   struct intel_context *ctx,
> > -			   struct drm_i915_gem_request **req_out);
> > +struct drm_i915_gem_request * __must_check
> > +i915_gem_request_alloc(struct intel_engine_cs *engine,
> > +		       struct intel_context *ctx);
> >  void i915_gem_request_cancel(struct drm_i915_gem_request *req);
> >  void i915_gem_request_free(struct kref *req_ref);
> >  int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 6c60e04..c908ed1 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2688,9 +2688,10 @@ void i915_gem_request_free(struct kref *req_ref)
> >  	kmem_cache_free(req->i915->requests, req);
> >  }
> >  
> > -int i915_gem_request_alloc(struct intel_engine_cs *ring,
> > -			   struct intel_context *ctx,
> > -			   struct drm_i915_gem_request **req_out)
> > +static inline int
> > +__i915_gem_request_alloc(struct intel_engine_cs *ring,
> > +			 struct intel_context *ctx,
> > +			 struct drm_i915_gem_request **req_out)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(ring->dev);
> >  	struct drm_i915_gem_request *req;
> > @@ -2753,6 +2754,31 @@ err:
> >  	return ret;
> >  }
> >  
> > +/**
> > + * i915_gem_request_alloc - allocate a request structure
> > + *
> > + * @engine: engine that we wish to issue the request on.
> > + * @ctx: context that the request will be associated with.
> > + *       This can be NULL if the request is not directly related to
> > + *       any specific user context, in which case this function will
> > + *       choose an appropriate context to use.
> > + *
> > + * Returns a pointer to the allocated request if successful,
> > + * or an error code if not.
> > + */
> > +struct drm_i915_gem_request *
> > +i915_gem_request_alloc(struct intel_engine_cs *engine,
> > +		       struct intel_context *ctx)
> > +{
> > +	struct drm_i915_gem_request *req;
> > +	int err;
> > +
> > +	if (ctx == NULL)
> > +		ctx = engine->default_context;
> 
> I think NULL vs. explicit dev_priv->kernel_context (or whatever it is) is
> semantically equivalent enough that it doesn't matter. And we can easily
> sed this (or an entire patch series for easy rebasing) if we change our
> minds.

But we were removing the engine->default_context as it complicated the
rest of the code. I strongly prefer keeping the contexts explicit as
context separation should be first and foremost in the driver.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre