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

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

On 11/12/2014 05:02 PM, Daniel Vetter wrote:
> On Thu, Nov 06, 2014 at 02:39:25PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Things like reliable GGTT mmaps and mirrored 2d-on-3d display will need
>> to map objects into the same address space multiple times.
>>
>> This also means that objects now can have multiple VMA entries.
>>
>> Added a GGTT view concept and linked it with the VMA to distinguish betwen
>> multiple instances per address space.
>>
>> New objects and GEM functions which do not take this new view as a parameter
>> assume the default of zero (I915_GGTT_VIEW_NORMAL) which preserves the
>> previous behaviour.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Let's hope I've picked the latest version, not sure. Please either have
> in-patch changelogs or coverletters with what's new to help confused
> maintainers ;-)

I had a cover letter and version when it was a patch series _and_ the 
concept split into multiple VMA and GGTT view. Once only one patch 
remained from the patch series and it was redesigned to kind of merge 
two concepts into one simplified approach I did not think cover letter 
makes sense any more. Will definitely version as this design goes forward.

> Looks good overal, just a few things around the lifetime rules for sg
> tables and related stuff that we've discussed offline. And one nit.
>
> I think with these addressed this is ready for detailed review and merging
> (also applies to the -internal series).

Excellent, thank you! Just one question below:

>> -int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj)
>> +int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj,
>> +				const struct i915_ggtt_view *view)
>>   {
>> -	if (obj->has_dma_mapping)
>> +	if (obj->has_dma_mapping || view->type != I915_GGTT_VIEW_NORMAL)
>
> This check and the one for finish_gtt are wrong. prepare/finish_gtt set up
> the dma_addr fields in the sg table (if available also the iommu mappings
> on big core). And that _must_ be done when the first view shows up, no
> matter which one it is. E.g. userspace might display a rotate buffer right
> aways without even rendering or writing into it (i.e. leaving it black).

The plan was to ensure that the "normal" view is always there first. 
Otherwise we can't "steal" DMA addresses and the other views do not have 
a proper SG table to generate DMA addresses from. So the internal code 
was doing that. Am I missing something?

> The change around finish_gtt is actually a leak since if the last view
> around is the rotate one (which can happen if userspace drops the buffer
> but leaves it displayed) we won't clean up (and so leak) the iommu
> mapping.

So I somehow need to ensure finish_gtt on a "normal" view is done last, 
correct? Move it from VMA destructon to object destruction? Would this 
have any implications for the shrinker?

Thanks,

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