Re: [PATCH v2 1/2] drm/i915: Add i915_gem_object_vmap to map GEM object to virtual space

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

On 19/02/16 11:07, Tvrtko Ursulin wrote:
>
> On 18/02/16 18:31, yu.dai@intel.com wrote:
>> From: Alex Dai <yu.dai@intel.com>
>>
>> There are several places inside driver where a GEM object is mapped to
>> kernel virtual space. The mapping is either done for the whole object
>> or certain page range of it.
>>
>> This patch introduces a function i915_gem_object_vmap to do such job.
>>
>> v2: Use obj->pages->nents for iteration within i915_gem_object_vmap;
>>      break when it finishes all desired pages. The caller need to pass
>>      in actual page number. (Tvrtko Ursulin)
>
> Look OK to me. Just one more thing, it would be good to add a WARN_ON
> and bail out if pages are not pinned. Just because the function is now
> public and that is a bit stronger than kerneldoc.
>
> With that added:
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Have to retract this due thing I noticed in Dave's repost of this patch, 
below:

> Regards,
>
> Tvrtko
>
>
>> Signed-off-by: Alex Dai <yu.dai@intel.com>
>> Cc: Dave Gordon <david.s.gordon@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Alex Dai <yu.dai@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_cmd_parser.c  | 28 +-------------------
>>   drivers/gpu/drm/i915/i915_drv.h         |  3 +++
>>   drivers/gpu/drm/i915/i915_gem.c         | 47
>> +++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/i915_gem_dmabuf.c  | 16 +++--------
>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 24 ++---------------
>>   5 files changed, 56 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c
>> b/drivers/gpu/drm/i915/i915_cmd_parser.c
>> index 814d894..915e8c1 100644
>> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
>> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
>> @@ -863,37 +863,11 @@ find_reg(const struct drm_i915_reg_descriptor
>> *table,
>>   static u32 *vmap_batch(struct drm_i915_gem_object *obj,
>>                  unsigned start, unsigned len)
>>   {
>> -    int i;
>> -    void *addr = NULL;
>> -    struct sg_page_iter sg_iter;
>>       int first_page = start >> PAGE_SHIFT;
>>       int last_page = (len + start + 4095) >> PAGE_SHIFT;
>>       int npages = last_page - first_page;
>> -    struct page **pages;
>> -
>> -    pages = drm_malloc_ab(npages, sizeof(*pages));
>> -    if (pages == NULL) {
>> -        DRM_DEBUG_DRIVER("Failed to get space for pages\n");
>> -        goto finish;
>> -    }
>> -
>> -    i = 0;
>> -    for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents,
>> first_page) {
>> -        pages[i++] = sg_page_iter_page(&sg_iter);
>> -        if (i == npages)
>> -            break;
>> -    }
>> -
>> -    addr = vmap(pages, i, 0, PAGE_KERNEL);
>> -    if (addr == NULL) {
>> -        DRM_DEBUG_DRIVER("Failed to vmap pages\n");
>> -        goto finish;
>> -    }
>>
>> -finish:
>> -    if (pages)
>> -        drm_free_large(pages);
>> -    return (u32*)addr;
>> +    return (u32*)i915_gem_object_vmap(obj, first_page, npages);
>>   }
>>
>>   /* Returns a vmap'd pointer to dest_obj, which the caller must unmap */
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 6644c2e..5b00a6a 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2899,6 +2899,9 @@ struct drm_i915_gem_object
>> *i915_gem_object_create_from_data(
>>           struct drm_device *dev, const void *data, size_t size);
>>   void i915_gem_free_object(struct drm_gem_object *obj);
>>   void i915_gem_vma_destroy(struct i915_vma *vma);
>> +void *i915_gem_object_vmap(struct drm_i915_gem_object *obj,
>> +               unsigned int first,
>> +               unsigned int npages);
>>
>>   /* Flags used by pin/bind&friends. */
>>   #define PIN_MAPPABLE    (1<<0)
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index f68f346..4bc0ce7 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -5356,3 +5356,50 @@ fail:
>>       drm_gem_object_unreference(&obj->base);
>>       return ERR_PTR(ret);
>>   }
>> +
>> +/**
>> + * i915_gem_object_vmap - map a GEM obj into kernel virtual space
>> + * @obj: the GEM obj to be mapped
>> + * @first: index of the first page where mapping starts
>> + * @npages: how many pages to be mapped, starting from first page
>> + *
>> + * Map a given page range of GEM obj into kernel virtual space. The
>> caller must
>> + * make sure the associated pages are gathered and pinned before
>> calling this
>> + * function. vunmap should be called after use.
>> + *
>> + * NULL will be returned if fails.
>> + */
>> +void *i915_gem_object_vmap(struct drm_i915_gem_object *obj,
>> +               unsigned int first,
>> +               unsigned int npages)
>> +{
>> +    struct sg_page_iter sg_iter;
>> +    struct page **pages;
>> +    void *addr;
>> +    int i;
>> +
>> +    if (first + npages > obj->pages->nents) {
>> +        DRM_DEBUG_DRIVER("Invalid page count\n");

nents can be equal or less than number of pages in an object so this can 
fail the attempt incorrectly. Should check against obj->base.size >> 
PAGE_SHIFT instead.

Regards,

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