diff mbox

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

Message ID 1374833372-21926-1-git-send-email-dh.herrmann@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

David Herrmann July 26, 2013, 10:09 a.m. UTC 
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>
---
 drivers/gpu/drm/drm_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Daniel Vetter July 26, 2013, 8:15 p.m. UTC | #1 
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

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
>
Sedat Dilek July 30, 2013, 7:41 a.m. UTC | #2 
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?

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
Sedat Dilek July 30, 2013, 7:52 a.m. UTC | #3 
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 ]

/**
 * 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
David Herrmann July 31, 2013, 4:46 p.m. UTC | #4 
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.

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
Sedat Dilek July 31, 2013, 5:13 p.m. UTC | #5 
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
diff mbox

Patch

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);