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

[Ben Widawsky <ben@bwidawsk.net>]

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.

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.

-- 
Ben Widawsky, Intel Open Source Technology Center