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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 11/03/2014 03:58 PM, Daniel Vetter wrote:
> On Thu, Oct 30, 2014 at 04:39:36PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@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.

I actually thought I will need semi-persistent view for this in the 
future patch which get_pages()/put_pages() caters for.

More precisely it would be for holding onto created pages during view 
creation across the i915_gem_gtt_prepare_object and i915_vma_bind in 
i915_gem_object_bind_to_vm.

Also, ->put_pages looks much neater to me from i915_gem_vma_destroy 
rather than leaking the knowledge of internal implementation. That is of 
course if you will allow the above justification for making the 
alternative view at least semi-persistent.

As additional bonus it has the same semantics to GEM get/put_pages.

>> +}
>> 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.

So you suggest to imply VMA id to be equal to GGTT view type and 
VMA_ID_DEFAULT to be fixed to I915_GGTT_VIEW_NORMAL? Is it not a 
logistics/maintenance problem to sync the two then?

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

Yes I left that for later. I mean, when at stage to be talking about 
such detail it is a happy place.

>> +	struct sg_table *pages;
>
> This seems unused - leftover from a previous patch which kept the sgtable
> around?

Yes, survived refactoring somehow, shouldn't be here.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx