[PATCH v3 2/4] drm/i915: refactor i915_gem_object_pin_map()

Chris Wilson chris at chris-wilson.co.uk
Wed Apr 20 20:02:27 UTC 2016


On Wed, Apr 20, 2016 at 03:35:45PM +0100, 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)
> 
> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 58 ++++++++++++++++++++++++++---------------
>  1 file changed, 37 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 261a3ef..98b6486 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2388,6 +2388,42 @@ 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 scatterlist *sg = obj->pages->sgl;
> +	struct sg_page_iter sg_iter;
> +	struct page **pages;
> +	unsigned long i = 0;
> +	void *addr = NULL;
> +
> +	/* A single page can always be kmapped */
> +	if (n_pages == 1)
> +		return kmap(sg_page(sg));
> +
> +	pages = drm_malloc_gfp(n_pages, sizeof(*pages), GFP_TEMPORARY);
> +	if (pages == NULL) {
> +		DRM_DEBUG_DRIVER("Failed to get space for pages\n");

The oom and subsequent ENOMEM to userspace (if it survives) is not enough?

> +		return NULL;
> +	}
> +

Please put the loop counter here so I can see it is correct.

> +	for_each_sg_page(sg, &sg_iter, n_pages, 0)

This must be nents. Ah. I see why you needed so much error detection
now.

A simpler iterator for this case would be
https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=tasklet&id=9db86845416ffc8c9d41fc187b0de6fd015cb319

> +		pages[i++] = sg_page_iter_page(&sg_iter);
> +
> +	/* Check that we have the expected number of pages */
> +	if (!WARN_ON(i != n_pages))

We've already trampled over the end of the buffer because of code we
wrote (for_each_sg_page) is broken? Job for ubsan/kasan/kmemcheck to
identify the culprit, because it is very unlikely to be here.

> +		addr = vmap(pages, n_pages, 0, PAGE_KERNEL);
> +
> +	if (addr == NULL)
> +		DRM_DEBUG_DRIVER("Failed to vmap pages\n");

More debug after lower level WARNs.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre


More information about the Intel-gfx-trybot mailing list