Re: [PATCH 6/6] drm/i915: Avoid allocating a vmap arena for a single page

[Dave Gordon <david.s.gordon@intel.com>]

On 06/04/16 11:05, Chris Wilson wrote:
> On Wed, Apr 06, 2016 at 10:49:36AM +0100, Tvrtko Ursulin wrote:
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>> index 985f067c1f0e..dc8e1b76c896 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -2233,7 +2233,10 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
>>>   	list_del(&obj->global_list);
>>>
>>>   	if (obj->vmapping) {
>>> -		vunmap(obj->vmapping);
>>> +		if (obj->base.size == PAGE_SIZE)
>>> +			kunmap(kmap_to_page(obj->vmapping));
>>> +		else
>>> +			vunmap(obj->vmapping);
>>
>> Can't think of a reason why it would be better but there is also
>> is_vmalloc_addr(addr) as used by kvfree.
>
> For consistency with the shrinker (see below).

What I don't like here is the repetition (and correlation) of the 
PAGE_SIZE test, which has to be kept in sync with the corresponding one 
at the point where the mapping was set up. If we're going to overload 
the same field to store two different types of mapping, there should be 
an explicit flag to say which we chose. Or failing that, then actually 
test the mapping itself (as in is_vmalloc_addr()).

>>>   		obj->vmapping = NULL;
>>>   	}
>>>
>>> @@ -2416,15 +2419,22 @@ void *i915_gem_object_pin_vmap(struct drm_i915_gem_object *obj)
>>>   	i915_gem_object_pin_pages(obj);
>>>
>>>   	if (obj->vmapping == NULL) {
>>> -		struct sg_page_iter sg_iter;
>>>   		struct page **pages;
>>> -		int n;
>>>
>>> -		n = obj->base.size >> PAGE_SHIFT;
>>> -		pages = drm_malloc_gfp(n, sizeof(*pages), GFP_TEMPORARY);
>>> +		pages = NULL;
>>> +		if (obj->base.size == PAGE_SIZE)
>>> +			obj->vmapping = kmap(sg_page(obj->pages->sgl));
>>> +		else
>>> +			pages = drm_malloc_gfp(obj->base.size >> PAGE_SHIFT,
>>> +					       sizeof(*pages),
>>> +					       GFP_TEMPORARY);
>>>   		if (pages != NULL) {
>>> +			struct sg_page_iter sg_iter;
>>> +			int n;
>>> +
>>>   			n = 0;
>>> -			for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0)
>>> +			for_each_sg_page(obj->pages->sgl, &sg_iter,
>>> +					 obj->pages->nents, 0)
>>>   				pages[n++] = sg_page_iter_page(&sg_iter);
>>>
>>>   			obj->vmapping = vmap(pages, n, 0, PAGE_KERNEL);
>>>
>>
>> Two problems I can spot are:
>>
>> 1. Callers of pin_vmap now don't know which kind of address they are
>> getting. Maybe call it pin_kvmap or something? Just mention in
>> kerneldoc could be enough.
>
> I think just mention, and we can rename this to i915_gem_object_pin_map().
> Hmm. I liked the pin in the name since it ties to to pin_pages (later
> I plan to change that to get_pages and get_vmap/get_map as the pin
> becomes implicit).
>
>> 2. Shrinker will try to kick out kmapped objects because they have
>> obj->vmapping set.
>
> Not caring that much since the vmap_purge is very heavy weight, but we
> can apply is_vmalloc_addr() to the shrinker.
>
> Ok, happy to call this obj->mapping and i915_gem_object_pin_map() ?
> -Chris

Quite happy with the rename, and returning either type (a (virtual) 
address is just an address), but not with the implementation repeating 
the decision code.

.Dave.

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