Re: [PATCH] drm/i915: Move ioremap_wc tracking onto VMA

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Move ioremap_wc tracking onto VMA
Date: Mon, 18 Apr 2016 16:27:32 +0100	[thread overview]
Message-ID: <5714FCE4.8090803@linux.intel.com> (raw)
In-Reply-To: <20160418152243.GL18964@nuc-i3427.alporthouse.com>

On 18/04/16 16:22, Chris Wilson wrote:
> On Mon, Apr 18, 2016 at 04:08:55PM +0100, Tvrtko Ursulin wrote:
>> On 18/04/16 15:54, Chris Wilson wrote:
>>> @@ -3378,6 +3378,8 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait)
>>>   		ret = i915_gem_object_put_fence(obj);
>>>   		if (ret)
>>>   			return ret;
>>> +
>>> +		i915_vma_iounmap(vma);
>>>   	}
>>>
>>>   	trace_i915_vma_unbind(vma);
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> index b3af2e808b49..e185dff358e1 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> @@ -3634,3 +3634,29 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
>>>   		return obj->base.size;
>>>   	}
>>>   }
>>> +
>>> +void *i915_vma_pin_iomap(struct i915_vma *vma)
>>> +{
>>> +	void *ptr;
>>
>> Just realized we should mark the pointers with __iomem here.
>
> The problem is that we need to lose the annotation at some point as we
> mix access via ring->virtual_start. For correctness, we should report
> __iomem here and discard it later.
>
>>> +/**
>>> + * i915_vma_iounmap - unmaps the VMA
>>> + * @vma: VMA to unmap
>>> + *
>>> + * Unmaps the previously iomapped VMA using iounmap.
>>> + *
>>> + * Users of i915_vma_pin_iomap() should not manually unmap by calling this
>>> + * function but should call i915_vma_unpin_iomap() instead.
>>> + *
>>> + * Callers must hold the struct_mutex.
>>> + */
>>> +static inline void i915_vma_iounmap(struct i915_vma *vma)
>>> +{
>>> +	lockdep_assert_held(&vma->vm->dev->struct_mutex);
>>> +	GEM_BUG_ON(vma->pin_count);
>>> +
>>> +	if (vma->iomap == NULL)
>>> +		return;
>>> +
>>> +	io_mapping_unmap(vma->iomap);
>>> +	vma->iomap = NULL;
>>> +}
>>
>> It would be best to hide this near the unbind code. Alternatively
>> maybe put a louder warning in the kerneldoc? Just so it is a bit
>> more distinguishable from the GEM exported API for this feature.
>
> I had the same thought, then decided to stick with the
> i915_vma_iounmap() call in i915_vma_unbind(). Maybe __i915_vma_iounmap?

__i915_vma_ionmap in i915_gem_gtt.h or in i915_gem.c ?

Regards,

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