[Intel-gfx] [PATCH 1/3] drm/i915: refactor i915_gem_object_pin_map()

[Dave Gordon]

On 17/05/16 10:22, Tvrtko Ursulin wrote:
>
> 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 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 | 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?

OK. BTW the compiler didn't actually generate any code for this :)

>> +
>> +    /* 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.

OK, debugging removed, new version posted.

.Dave.

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