[PATCH v3 4/9] drm/rockchip/rockchip_drm_gem.c: Convert to use vm_insert_range

[PATCH v3 4/9] drm/rockchip/rockchip_drm_gem.c: Convert to use vm_insert_range

[Souptick Joarder]

Convert to use vm_insert_range() to map range of kernel
memory to user vma.

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Acked-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
index a8db758..2cb83bb 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
@@ -221,26 +221,10 @@ static int rockchip_drm_gem_object_mmap_iommu(struct drm_gem_object *obj,
 					      struct vm_area_struct *vma)
 {
 	struct rockchip_gem_object *rk_obj = to_rockchip_obj(obj);
-	unsigned int i, count = obj->size >> PAGE_SHIFT;
 	unsigned long user_count = vma_pages(vma);
-	unsigned long uaddr = vma->vm_start;
-	unsigned long offset = vma->vm_pgoff;
-	unsigned long end = user_count + offset;
-	int ret;
-
-	if (user_count == 0)
-		return -ENXIO;
-	if (end > count)
-		return -ENXIO;
 
-	for (i = offset; i < end; i++) {
-		ret = vm_insert_page(vma, uaddr, rk_obj->pages[i]);
-		if (ret)
-			return ret;
-		uaddr += PAGE_SIZE;
-	}
-
-	return 0;
+	return vm_insert_range(vma, vma->vm_start, rk_obj->pages,
+				user_count);
 }
 
 static int rockchip_drm_gem_object_mmap_dma(struct drm_gem_object *obj,
-- 
1.9.1

Re: [PATCH v3 4/9] drm/rockchip/rockchip_drm_gem.c: Convert to use vm_insert_range

[Robin Murphy]

On 06/12/2018 18:42, Souptick Joarder wrote:
> Convert to use vm_insert_range() to map range of kernel
> memory to user vma.
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Acked-by: Heiko Stuebner <heiko@sntech.de>
> ---
>   drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 20 ++------------------
>   1 file changed, 2 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> index a8db758..2cb83bb 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> @@ -221,26 +221,10 @@ static int rockchip_drm_gem_object_mmap_iommu(struct drm_gem_object *obj,
>   					      struct vm_area_struct *vma)
>   {
>   	struct rockchip_gem_object *rk_obj = to_rockchip_obj(obj);
> -	unsigned int i, count = obj->size >> PAGE_SHIFT;
>   	unsigned long user_count = vma_pages(vma);
> -	unsigned long uaddr = vma->vm_start;
> -	unsigned long offset = vma->vm_pgoff;
> -	unsigned long end = user_count + offset;
> -	int ret;
> -
> -	if (user_count == 0)
> -		return -ENXIO;
> -	if (end > count)
> -		return -ENXIO;
>   
> -	for (i = offset; i < end; i++) {
> -		ret = vm_insert_page(vma, uaddr, rk_obj->pages[i]);
> -		if (ret)
> -			return ret;
> -		uaddr += PAGE_SIZE;
> -	}
> -
> -	return 0;
> +	return vm_insert_range(vma, vma->vm_start, rk_obj->pages,
> +				user_count);

We're losing vm_pgoff handling here, which given the implication in 
57de50af162b, may well be a regression for at least some combination of 
GPU and userspace driver (I assume that commit was in the context of 
some version of the Arm Mali driver, possibly on RK3288).

Robin.

>   }
>   
>   static int rockchip_drm_gem_object_mmap_dma(struct drm_gem_object *obj,
> 

Re: [PATCH v3 4/9] drm/rockchip/rockchip_drm_gem.c: Convert to use vm_insert_range

[Souptick Joarder]

On Fri, Dec 7, 2018 at 8:20 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 06/12/2018 18:42, Souptick Joarder wrote:
> > Convert to use vm_insert_range() to map range of kernel
> > memory to user vma.
> >
> > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > Tested-by: Heiko Stuebner <heiko@sntech.de>
> > Acked-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> >   drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 20 ++------------------
> >   1 file changed, 2 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> > index a8db758..2cb83bb 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> > @@ -221,26 +221,10 @@ static int rockchip_drm_gem_object_mmap_iommu(struct drm_gem_object *obj,
> >                                             struct vm_area_struct *vma)
> >   {
> >       struct rockchip_gem_object *rk_obj = to_rockchip_obj(obj);
> > -     unsigned int i, count = obj->size >> PAGE_SHIFT;
> >       unsigned long user_count = vma_pages(vma);
> > -     unsigned long uaddr = vma->vm_start;
> > -     unsigned long offset = vma->vm_pgoff;
> > -     unsigned long end = user_count + offset;
> > -     int ret;
> > -
> > -     if (user_count == 0)
> > -             return -ENXIO;
> > -     if (end > count)
> > -             return -ENXIO;
> >
> > -     for (i = offset; i < end; i++) {
> > -             ret = vm_insert_page(vma, uaddr, rk_obj->pages[i]);
> > -             if (ret)
> > -                     return ret;
> > -             uaddr += PAGE_SIZE;
> > -     }
> > -
> > -     return 0;
> > +     return vm_insert_range(vma, vma->vm_start, rk_obj->pages,
> > +                             user_count);
>
> We're losing vm_pgoff handling here, which given the implication in
> 57de50af162b, may well be a regression for at least some combination of
> GPU and userspace driver (I assume that commit was in the context of
> some version of the Arm Mali driver, possibly on RK3288).

In commit  57de50af162b, vma->vm_pgoff = 0 for GEM mmap handler context
and removing it from common path which means if call stack looks like
rockchip_gem_mmap_buf() -> rockchip_drm_gem_object_mmap() ->
rockchip_drm_gem_object_mmap_iommu(), then we might have a non zero
vma->vm_pgoff context which is not handled.

This is the problem you are pointing ? right ?

Re: [PATCH v3 4/9] drm/rockchip/rockchip_drm_gem.c: Convert to use vm_insert_range

[Robin Murphy]

On 2018-12-07 8:30 pm, Souptick Joarder wrote:
> On Fri, Dec 7, 2018 at 8:20 PM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 06/12/2018 18:42, Souptick Joarder wrote:
>>> Convert to use vm_insert_range() to map range of kernel
>>> memory to user vma.
>>>
>>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
>>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>>> Acked-by: Heiko Stuebner <heiko@sntech.de>
>>> ---
>>>    drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 20 ++------------------
>>>    1 file changed, 2 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
>>> index a8db758..2cb83bb 100644
>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
>>> @@ -221,26 +221,10 @@ static int rockchip_drm_gem_object_mmap_iommu(struct drm_gem_object *obj,
>>>                                              struct vm_area_struct *vma)
>>>    {
>>>        struct rockchip_gem_object *rk_obj = to_rockchip_obj(obj);
>>> -     unsigned int i, count = obj->size >> PAGE_SHIFT;
>>>        unsigned long user_count = vma_pages(vma);
>>> -     unsigned long uaddr = vma->vm_start;
>>> -     unsigned long offset = vma->vm_pgoff;
>>> -     unsigned long end = user_count + offset;
>>> -     int ret;
>>> -
>>> -     if (user_count == 0)
>>> -             return -ENXIO;
>>> -     if (end > count)
>>> -             return -ENXIO;
>>>
>>> -     for (i = offset; i < end; i++) {
>>> -             ret = vm_insert_page(vma, uaddr, rk_obj->pages[i]);
>>> -             if (ret)
>>> -                     return ret;
>>> -             uaddr += PAGE_SIZE;
>>> -     }
>>> -
>>> -     return 0;
>>> +     return vm_insert_range(vma, vma->vm_start, rk_obj->pages,
>>> +                             user_count);
>>
>> We're losing vm_pgoff handling here, which given the implication in
>> 57de50af162b, may well be a regression for at least some combination of
>> GPU and userspace driver (I assume that commit was in the context of
>> some version of the Arm Mali driver, possibly on RK3288).
> 
> In commit  57de50af162b, vma->vm_pgoff = 0 for GEM mmap handler context
> and removing it from common path which means if call stack looks like
> rockchip_gem_mmap_buf() -> rockchip_drm_gem_object_mmap() ->
> rockchip_drm_gem_object_mmap_iommu(), then we might have a non zero
> vma->vm_pgoff context which is not handled.
> 
> This is the problem you are pointing ? right ?

Exactly - if unconditionally zeroing the offset in the PRIME mmap() path 
was a problem, then the implication is that there are callers of that 
path who expect the offset to be honoured here.

Robin.