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

[Sedat Dilek <sedat.dilek@gmail.com>]

On Wed, Jul 31, 2013 at 6:46 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Tue, Jul 30, 2013 at 9:52 AM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
>> On Tue, Jul 30, 2013 at 9:41 AM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
>>> On Fri, Jul 26, 2013 at 10:15 PM, Daniel Vetter <daniel@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@ffwll.ch>
>>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>>>>> Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
>>>>> Tested-by: Sedat Dilek <sedat.dilek@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 ]
>
> It's actually "drm_gem_mmap_obj" which we have to look at and it says:
>   * @obj_size: the object size to be mapped, in bytes
>
> So both are already documented.
>

Good. I had only a quick view, you are the expert.

> Regards
> David
>
>> /**
>>  * 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@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