[Intel-gfx] [RFC 3/5] drm/i915: Infrastructure for supporting different GGTT views per object

[Daniel Vetter]

On Thu, Oct 30, 2014 at 04:39:36PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> Things like reliable GGTT mappings and mirrored 3d display will need to be
> to map the same object twice into the GGTT.
> 
> Add a ggtt_view field per VMA and select the page view based on the type
> of the view.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>

lgtm overall, some comments for polish in the crucial parts below.

> @@ -2189,3 +2191,21 @@ i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
>  
>  	return vma;
>  }
> +
> +void i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> +			u32 flags)
> +{
> +	struct sg_table *pages;
> +
> +	if (vma->ggtt_view.get_pages)
> +		pages = vma->ggtt_view.get_pages(&vma->ggtt_view, vma->obj);
> +	else
> +		pages = vma->obj->pages;
> +
> +	if (pages && !IS_ERR(pages)) {
> +		vma->bind_vma(vma, pages, cache_level, flags);
> +
> +		if (vma->ggtt_view.put_pages)
> +			vma->ggtt_view.put_pages(&vma->ggtt_view);
> +	}

tbh this looks a bit like overboarding generalism to me ;-) Imo
- drop the ->put_pages callback and just free the sg table if you have
  one.
- drop teh ->get_pages callbacks and replace it with a new
  i915_vma_shuffle_pages or similar you'll call for non-standard ggtt
  views. Two reasons: shuffle is a more accurate name than get since this
  version doesn't grab it's own page references any more, and with
  currently just one internal user for this a vtable is serious overkill.

> +}
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 66bc44b..cbaddda 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -109,6 +109,23 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t;
>  #define GEN8_PPAT_ELLC_OVERRIDE		(0<<2)
>  #define GEN8_PPAT(i, x)			((uint64_t) (x) << ((i) * 8))
>  
> +enum i915_ggtt_view_type {
> +	I915_GGTT_VIEW_NORMAL = 0,
> +};
> +
> +struct i915_ggtt_view {
> +	enum i915_ggtt_view_type type;
> +	unsigned int vma_id;

Imo vma_id and ggtt_view_type are fairly redundant. If you _really_ want
to make this super-generic (i.e. for ppgtt) just call it gtt_view instead
fo ggtt_view. But I wouldn't bother.

Removing vma_id also removes the int, which I'd ask you to replace with an
explicit enum anyway ;-)

> +	struct sg_table *pages;

This seems unused - leftover from a previous patch which kept the sgtable
around?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch