[Intel-gfx] [RFC 1/2] drm/i915: Move ioremap_wc tracking onto VMA

[Tvrtko Ursulin]

On 13/04/16 10:04, Chris Wilson wrote:
> On Wed, Apr 13, 2016 at 09:45:03AM +0100, Tvrtko Ursulin wrote:
>>
>> On 12/04/16 17:21, Chris Wilson wrote:
>>> On Tue, Apr 12, 2016 at 04:56:51PM +0100, Tvrtko Ursulin wrote:
>>>> From: Chris Wilson <chris at chris-wilson.co.uk>
>>>>
>>>> By tracking the iomapping on the VMA itself, we can share that area
>>>> between multiple users. Also by only revoking the iomapping upon
>>>> unbinding from the mappable portion of the GGTT, we can keep that iomap
>>>> across multiple invocations (e.g. execlists context pinning).
>>>>
>>>> v2:
>>>>    * Rebase on nightly;
>>>>    * added kerneldoc. (Tvrtko Ursulin)
>>>>
>>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_gem.c     |  2 ++
>>>>   drivers/gpu/drm/i915/i915_gem_gtt.c | 38 +++++++++++++++++++++++++++++++++++++
>>>>   drivers/gpu/drm/i915/i915_gem_gtt.h | 38 +++++++++++++++++++++++++++++++++++++
>>>>   drivers/gpu/drm/i915/intel_fbdev.c  |  8 +++-----
>>>>   4 files changed, 81 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>>> index b37ffea8b458..6a485630595e 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>> @@ -3393,6 +3393,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 c5cb04907525..b2a8a14e8dcb 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>> @@ -3626,3 +3626,41 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
>>>>   		return obj->base.size;
>>>>   	}
>>>>   }
>>>> +
>>>> +void *i915_vma_iomap(struct drm_i915_private *dev_priv,
>>>> +		     struct i915_vma *vma)
>>>> +{
>>>> +	struct drm_i915_gem_object *obj = vma->obj;
>>>> +	struct i915_ggtt *ggtt = &dev_priv->ggtt;
>>>> +
>>>> +	if (WARN_ON(!obj->map_and_fenceable))
>>>> +		return ERR_PTR(-ENODEV);
>>>> +
>>>> +	BUG_ON(!vma->is_ggtt);
>>>> +	BUG_ON(vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL);
>>>> +	BUG_ON((vma->bound & GLOBAL_BIND) == 0);
>>>> +
>>>> +	if (vma->iomap == NULL) {
>>>> +		void *ptr;
>>>
>>> We could extract ggtt from the is_ggtt vma->vm that would remove the
>>> dev_priv parameter and make the callers a bit tidier.
>>>
>>> static inline struct i915_ggtt *to_ggtt(struct i915_address_space *vm)
>>> {
>>> 	BUG_ON(!i915_is_ggtt(vm));
>>> 	return container_of(vm, struct i915_ggtt, base);
>>> }
>>>
>>>> +
>>>> +		ptr = ioremap_wc(ggtt->mappable_base + vma->node.start,
>>>> +				 obj->base.size);
>>>> +		if (ptr == NULL) {
>>>> +			int ret;
>>>> +
>>>> +			/* Too many areas already allocated? */
>>>> +			ret = i915_gem_evict_vm(vma->vm, true);
>>>> +			if (ret)
>>>> +				return ERR_PTR(ret);
>>>> +
>>>> +			ptr = ioremap_wc(ggtt->mappable_base + vma->node.start,
>>>> +					 obj->base.size);
>>>
>>> No, we really don't want to create a new ioremap for every caller when
>>> we already have one, ggtt->mappable. Hence,
>>>      io-mapping: Specify mapping size for io_mapping_map_wc
>>> being its preceeding patch. The difference is huge on Braswell.
>>
>> I was following the principle of least change for this one since it
>> does remain the same in that respect as the current code. Goal was
>> to unblock the GuC early unpin / execlist no retired queue series
>> and not get into the trap of inflating the dependencies too much. I
>> thought to add this and default context cleanup but you keep adding
>> things to the pile. :)
>
> Tip of the iceberg. Only we have lots of icebergs. And the occasional
> dragon.
>
>> I'll have a look at your io_mapping stuff to see how much it would
>> add to the series. Remember I said I'll add the stable ctx id
>> patches as well. Do we need to come up with a plan here?
>
> We more or less own io_mapping, for this it is just one patch to add a
> pass-through parameter to ioremap_wc that's required for 32bit.
>
> I could mutter about the patch before this being quite a major
> bugfix...
>
>> I could have two separate series to simplify dependencies a bit:
>>
>>   1. GuC premature unpin and
>>   2. execlist no retired queue.
>>
>> The stack for the first one could look like (not as patches but
>> conceptually):
>>
>>   1. default context cleanup
>>   2. any io_mapping patches of yours
>>   3. i915_vma_iomap or WC pin_map as you suggested in the other reply.
>>   4. previous / pinned context
>>
>> Execlist no retired queue would then be:
>>
>>   1. stable ctx id
>>   2. removal of i915_gem_request_unreference__unlocked
>>   3. execlist no retired queue
>>
>> If I did not forget about something.
>>
>> At any point in any series we could add things like
>> i915_gem_request.c or those patches which move around the context
>> init.
>
> Could I see you some drm_mm.c patches after that?

If I am competent enough to help with them sure.

Does this mean above sounds like a plan to you? Two series containing 
the listed items?

Regards,

Tvrtko