[PATCH v2] drm/gem: fix mmap vma size calculations

[Sedat Dilek]

On Tue, Jul 30, 2013 at 9:41 AM, Sedat Dilek <sedat.dilek at gmail.com> wrote:
> On Fri, Jul 26, 2013 at 10:15 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
>> On Fri, Jul 26, 2013 at 12:09:32PM +0200, David Herrmann wrote:
>>> The VMA manager is page-size based so drm_vma_node_size() returns the size
>>> in pages. However, drm_gem_mmap_obj() requires the size in bytes. Apply
>>> PAGE_SHIFT so we no longer get EINVAL during mmaps due to too small
>>> buffers.
>>>
>>> This bug was introduced in commit:
>>>   0de23977cfeb5b357ec884ba15417ae118ff9e9b
>>>   "drm/gem: convert to new unified vma manager"
>>>
>>> Fixes i915 gtt mmap failure reported by Sedat Dilek in:
>>>   Re: linux-next: Tree for Jul 25 [ call-trace: drm | drm-intel related? ]
>>>
>>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>> Signed-off-by: David Herrmann <dh.herrmann at gmail.com>
>>> Reported-by: Sedat Dilek <sedat.dilek at gmail.com>
>>> Tested-by: Sedat Dilek <sedat.dilek at gmail.com>
>>
>> I remember that I even checked whether v4 was consistent with pages vs.
>> bytes ;-) So
>>
>
> Hi David, Daniel, and Dave,
>
> Just reading quickly "drm: add unified vma offset manager"... is that
> below in the docs?
>
> "The VMA manager is page-size based so drm_vma_node_size() returns the size
> in pages. However, drm_gem_mmap_obj() requires the size in bytes. Apply
> PAGE_SHIFT so we no longer get EINVAL during mmaps due to too small
> buffers."
>
> If not, do you mind adding it?
>

To quote this two:

[ include/drm/drm_vma_manager.h ]

/**
 * drm_vma_node_size() - Return size (page-based)
 * @node: Node to inspect
 *
 * Return the size as number of pages for the given node. This is the same size
 * that was passed to drm_vma_offset_add(). If no offset is allocated for the
 * node, this is 0.
 *
 * RETURNS:
 * Size of @node as number of pages. 0 if the node does not have an offset
 * allocated.
 */

[ drivers/gpu/drm/drm_gem.c ]

/**
 * drm_gem_mmap - memory map routine for GEM objects
 * @filp: DRM file pointer
 * @vma: VMA for the area to be mapped
 *
 * If a driver supports GEM object mapping, mmap calls on the DRM file
 * descriptor will end up here.
 *
 * Look up the GEM object based on the offset passed in (vma->vm_pgoff will
 * contain the fake offset we created when the GTT map ioctl was called on
 * the object) and map it with a call to drm_gem_mmap_obj().
 */

- Sedat -

> Thanks in advance!
>
> - Sedat -
>
>> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>>
>> on this little fixup.
>> -Daniel
>>
>>> ---
>>>  drivers/gpu/drm/drm_gem.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>>> index 3613b50..1f76572 100644
>>> --- a/drivers/gpu/drm/drm_gem.c
>>> +++ b/drivers/gpu/drm/drm_gem.c
>>> @@ -666,7 +666,7 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>>>       }
>>>
>>>       obj = container_of(node, struct drm_gem_object, vma_node);
>>> -     ret = drm_gem_mmap_obj(obj, drm_vma_node_size(node), vma);
>>> +     ret = drm_gem_mmap_obj(obj, drm_vma_node_size(node) << PAGE_SHIFT, vma);
>>>
>>>       mutex_unlock(&dev->struct_mutex);
>>>
>>> --
>>> 1.8.3.4
>>>
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch