Re: [PATCH 1/3] drm/i915: refactor i915_gem_object_pin_map()

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

On 16/05/16 16:19, Dave Gordon wrote:
> The recently-added i915_gem_object_pin_map() can be further optimised
> for "small" objects. To facilitate this, and simplify the error paths
> before adding the new code, this patch pulls out the "mapping" part of
> the operation (involving local allocations which must be undone before
> return) into its own subfunction.
>
> The next patch will then insert the new optimisation into the middle of
> the now-separated subfunction.
>
> This reorganisation will probably not affect the generated code, as the
> compiler will most likely inline it anyway, but it makes the logical
> structure a bit clearer and easier to modify.
>
> v2:
>      Restructure loop-over-pages & error check (Chris Wilson)
>
> v3:
>      Add page count to debug messages (Chris Wilson)
>      Convert WARN_ON() to GEM_BUG_ON()
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 59 ++++++++++++++++++++++++++---------------
>   1 file changed, 38 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index aff386e..bbec429 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2398,6 +2398,43 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj)
>   	return 0;
>   }
>
> +/* The 'mapping' part of i915_gem_object_pin_map() below */
> +static void *i915_gem_object_map(const struct drm_i915_gem_object *obj)
> +{
> +	unsigned long n_pages = obj->base.size >> PAGE_SHIFT;
> +	struct sg_table *sgt = obj->pages;
> +	struct sg_page_iter sg_iter;
> +	struct page **pages;
> +	unsigned long i = 0;
> +	void *addr = NULL;

Looks like this does not need to be initialized?

> +
> +	/* A single page can always be kmapped */
> +	if (n_pages == 1)
> +		return kmap(sg_page(sgt->sgl));
> +
> +	pages = drm_malloc_gfp(n_pages, sizeof(*pages), GFP_TEMPORARY);
> +	if (pages == NULL) {
> +		DRM_DEBUG_DRIVER("Failed to get space for %lu pointers\n",
> +				 n_pages);

Not terribly important but I think this is too low level functions to 
have debug logging. It will not add a lot of useful information, no call 
stack etc. And the callers are probably handling failures already and 
they would propagate somewhere from where it is already reported.

> +		return NULL;
> +	}
> +
> +	for_each_sg_page(sgt->sgl, &sg_iter, sgt->nents, 0)
> +		pages[i++] = sg_page_iter_page(&sg_iter);
> +
> +	/* Check that we have the expected number of pages */
> +	GEM_BUG_ON(i != n_pages);
> +
> +	addr = vmap(pages, n_pages, 0, PAGE_KERNEL);
> +	if (addr == NULL)
> +		DRM_DEBUG_DRIVER("Failed to vmap %lu pages\n", n_pages);

Same here. I mean, the only potential argument could be that this will 
tell the real reason which is otherwise lost in the NULL return code, 
but I am not sure it is worth it since it is so unlikely it would happen.

> +
> +	drm_free_large(pages);
> +
> +	return addr;
> +}
> +
> +/* get, pin, and map the pages of the object into kernel space */
>   void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj)
>   {
>   	int ret;
> @@ -2411,27 +2448,7 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj)
>   	i915_gem_object_pin_pages(obj);
>
>   	if (obj->mapping == NULL) {
> -		struct page **pages;
> -
> -		pages = NULL;
> -		if (obj->base.size == PAGE_SIZE)
> -			obj->mapping = 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)
> -				pages[n++] = sg_page_iter_page(&sg_iter);
> -
> -			obj->mapping = vmap(pages, n, 0, PAGE_KERNEL);
> -			drm_free_large(pages);
> -		}
> +		obj->mapping = i915_gem_object_map(obj);
>   		if (obj->mapping == NULL) {
>   			i915_gem_object_unpin_pages(obj);
>   			return ERR_PTR(-ENOMEM);
>

Otherwise looks fine to me.

Regards,

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