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

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

On 05/04/16 13:57, Chris Wilson wrote:
> If we want a contiguous mapping of a single page sized object, we can
> forgo using vmap() and just use a regular kmap(). Note that this is only
> suitable if the desired pgprot_t is comptabitible.

You noticed the spelling already. :)

>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 22 ++++++++++++++++------
>   1 file changed, 16 insertions(+), 6 deletions(-)
>
> 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.

>   		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.

2. Shrinker will try to kick out kmapped objects because they have 
obj->vmapping set.

Regards,

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