[Intel-gfx] [PATCH 1/2] drm/i915: Infrastructure for supporting different GGTT views per object

[Tvrtko Ursulin]

On 12/10/2014 09:16 AM, Daniel Vetter wrote:
> On Tue, Dec 09, 2014 at 03:23:35PM +0000, Michel Thierry wrote:
>> We also need a _vma_view version of i915_gem_obj_bound;
>> i915_gem_object_ggtt_unpin checks if the obj is ggtt_bound and it could be
>> that the normal view is gone but a different view is still active (it is
>> also used in gpu_error and debug_fs, but I don't think it's a problem
>> there).
>
> Where did you see the need for the new obj_bound variant? Probably best to
> reply to the patch newly with just the relevant part quoted.

It is not in the patch but in the i915_gem_object_ggtt_unpin. Which is:

i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
{
         struct i915_vma *vma = i915_gem_obj_to_ggtt(obj);

         BUG_ON(!vma);
         BUG_ON(vma->pin_count == 0);
         BUG_ON(!i915_gem_obj_ggtt_bound(obj));

         if (--vma->pin_count == 0)
                 obj->pin_mappable = false;
}

The concern is the mismatch in semantics between i915_gem_obj_to_ggtt 
and i915_gem_obj_ggtt_bound. Former implies normal VMA while the latter 
hasn't been touched so it returns true on _any_ GGTT bound VMA.

I don't think this BUG_ON can trigger since normal VMA exists by the 
virtue of BUG_ON(!vma), but I do agree that there is a mismatch in 
"documentation" (BUG_ONs). So making i915_gem_obj_ggtt_bound also imply 
a normal view would be correct it seems.

Regards,

Tvrtko