Re: [PATCH 2/4] drm/i915: allow stolen, pre-allocated objects to avoid GTT allocation v2

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Wed, 8 May 2013 19:12:49 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> On Wed, May 08, 2013 at 10:45:14AM -0700, Jesse Barnes wrote:
> > In some cases, we may not need GTT address space allocated to a stolen
> > object, so allow passing -1 to the preallocated function to indicate as
> > much.
> > 
> > v2: remove BUG_ON(gtt_offset & 4095) now that -1 is allowed (Ville)
> > 
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_stolen.c |    5 ++++-
> >  drivers/gpu/drm/i915/intel_pm.c        |    2 +-
> >  2 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > index 913994c..89cbfab 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > @@ -339,7 +339,6 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> >  
> >  	/* KISS and expect everything to be page-aligned */
> >  	BUG_ON(stolen_offset & 4095);
> > -	BUG_ON(gtt_offset & 4095);
> >  	BUG_ON(size & 4095);
> >  
> >  	if (WARN_ON(size == 0))
> > @@ -360,6 +359,10 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> >  		return NULL;
> >  	}
> >  
> > +	/* Some objects just need physical mem from stolen space */
> > +	if (gtt_offset == -1)
> > +		return obj;
> > +
> >  	/* To simplify the initialisation sequence between KMS and GTT,
> >  	 * we allow construction of the stolen object prior to
> >  	 * setting up the GTT space. The actual reservation will occur
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index e60cd3e..081194d 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2877,7 +2877,7 @@ static void valleyview_setup_pctx(struct drm_device *dev)
> >  		pcbr_offset = (pcbr & (~4095)) - dev_priv->mm.stolen_base;
> >  		pctx = i915_gem_object_create_stolen_for_preallocated(dev_priv->dev,
> >  								      pcbr_offset,
> > -								      pcbr_offset,
> > +								      -1,
> >  								      pctx_size);
> >  		goto out;
> >  	}
> 
> I'm not asking for any changes, just want clarification.
> 
> Even the _i915_gem_object_create_stolen() call isn't very useful
> in this case. You really don't even want a gem object in this case, just
> to take it out of the stolen mm, so a simple call to drm_mm_create_block
> would be sufficient.

Yeah, but outside of stolen.c I thought that would be a bit
underhanded.  We don't need the object really, but it doesn't hurt to
have it either, and it fits well for the case where we might need to
allocate it.

> In the case where we setup the powerctx ourselves, you really just need
> to carve it out of the allocator too, because once created, we never
> want to touch the thing... evicting it is out of the question as well.
> Therefore, similar to the previous case, all you really need is
> drm_mm_get_block().
> 
> Certainly for the second case, it's much cleaner to use the existing
> APIs. It just showed up in my review that some of our object state
> doesn't make sense for the power context (read/write domains, cache
> level, has_dma_mapping, map_and_fenceable)
> 
> I'd be happy with a True/False for the first two paragraphs, and an
> ignore on the third.

Yeah I'd be ok moving over to using something else for the power stuff
(and anything else that's a one time alloc, don't touch sort of
object), but it would be nicer if we had an API to do that, ideally
self-documenting.

-- 
Jesse Barnes, Intel Open Source Technology Center