On Wed, 8 May 2013 19:12:49 -0700 Ben Widawsky <ben at 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 at chris-wilson.co.uk> > > Signed-off-by: Jesse Barnes <jbarnes at 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
