[PATCH RESEND] drm/via: use get_user_pages_unlocked()

[PATCH RESEND] drm/via: use get_user_pages_unlocked()

[Lorenzo Stoakes]

Moving from get_user_pages() to get_user_pages_unlocked() simplifies the code
and takes advantage of VM_FAULT_RETRY functionality when faulting in pages.

Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
---
 drivers/gpu/drm/via/via_dmablit.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/via/via_dmablit.c b/drivers/gpu/drm/via/via_dmablit.c
index 1a3ad769f8c8..98aae9809249 100644
--- a/drivers/gpu/drm/via/via_dmablit.c
+++ b/drivers/gpu/drm/via/via_dmablit.c
@@ -238,13 +238,9 @@ via_lock_all_dma_pages(drm_via_sg_info_t *vsg,  drm_via_dmablit_t *xfer)
 	vsg->pages = vzalloc(sizeof(struct page *) * vsg->num_pages);
 	if (NULL == vsg->pages)
 		return -ENOMEM;
-	down_read(&current->mm->mmap_sem);
-	ret = get_user_pages((unsigned long)xfer->mem_addr,
-			     vsg->num_pages,
-			     (vsg->direction == DMA_FROM_DEVICE) ? FOLL_WRITE : 0,
-			     vsg->pages, NULL);
-
-	up_read(&current->mm->mmap_sem);
+	ret = get_user_pages_unlocked((unsigned long)xfer->mem_addr,
+			vsg->num_pages, vsg->pages,
+			(vsg->direction == DMA_FROM_DEVICE) ? FOLL_WRITE : 0);
 	if (ret != vsg->num_pages) {
 		if (ret < 0)
 			return ret;
-- 
2.11.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH RESEND] drm/via: use get_user_pages_unlocked()

[Daniel Vetter]

On Mon, Feb 27, 2017 at 09:50:08PM +0000, Lorenzo Stoakes wrote:
> Moving from get_user_pages() to get_user_pages_unlocked() simplifies the code
> and takes advantage of VM_FAULT_RETRY functionality when faulting in pages.
> 
> Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>

Queued for 4.12, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/via/via_dmablit.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/via/via_dmablit.c b/drivers/gpu/drm/via/via_dmablit.c
> index 1a3ad769f8c8..98aae9809249 100644
> --- a/drivers/gpu/drm/via/via_dmablit.c
> +++ b/drivers/gpu/drm/via/via_dmablit.c
> @@ -238,13 +238,9 @@ via_lock_all_dma_pages(drm_via_sg_info_t *vsg,  drm_via_dmablit_t *xfer)
>  	vsg->pages = vzalloc(sizeof(struct page *) * vsg->num_pages);
>  	if (NULL == vsg->pages)
>  		return -ENOMEM;
> -	down_read(&current->mm->mmap_sem);
> -	ret = get_user_pages((unsigned long)xfer->mem_addr,
> -			     vsg->num_pages,
> -			     (vsg->direction == DMA_FROM_DEVICE) ? FOLL_WRITE : 0,
> -			     vsg->pages, NULL);
> -
> -	up_read(&current->mm->mmap_sem);
> +	ret = get_user_pages_unlocked((unsigned long)xfer->mem_addr,
> +			vsg->num_pages, vsg->pages,
> +			(vsg->direction == DMA_FROM_DEVICE) ? FOLL_WRITE : 0);
>  	if (ret != vsg->num_pages) {
>  		if (ret < 0)
>  			return ret;
> -- 
> 2.11.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH RESEND] drm/via: use get_user_pages_unlocked()

[Al Viro]

On Tue, Feb 28, 2017 at 10:01:10AM +0100, Daniel Vetter wrote:

> > +	ret = get_user_pages_unlocked((unsigned long)xfer->mem_addr,
> > +			vsg->num_pages, vsg->pages,
> > +			(vsg->direction == DMA_FROM_DEVICE) ? FOLL_WRITE : 0);

Umm...  Why not
	ret = get_user_pages_fast((unsigned long)xfer->mem_addr,
			vsg->num_pages,
			vsg->direction == DMA_FROM_DEVICE,
			vsg->pages);

IOW, do you really need a warranty that ->mmap_sem will be grabbed and
released?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH RESEND] drm/via: use get_user_pages_unlocked()

[Lorenzo Stoakes]

On 28 February 2017 at 19:35, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Feb 28, 2017 at 10:01:10AM +0100, Daniel Vetter wrote:
>
>> > +   ret = get_user_pages_unlocked((unsigned long)xfer->mem_addr,
>> > +                   vsg->num_pages, vsg->pages,
>> > +                   (vsg->direction == DMA_FROM_DEVICE) ? FOLL_WRITE : 0);
>
> Umm...  Why not
>         ret = get_user_pages_fast((unsigned long)xfer->mem_addr,
>                         vsg->num_pages,
>                         vsg->direction == DMA_FROM_DEVICE,
>                         vsg->pages);
>
> IOW, do you really need a warranty that ->mmap_sem will be grabbed and
> released?

Daniel will be better placed to answer in this specific case, but more
generally is there any reason why we can't just use
get_user_pages_fast() in all such cases? These patches were simply a
mechanical/cautious replacement for code that is more or less exactly
equivalent but if this would make sense perhaps it'd be worth using
gup_fast() where possible?

-- 
Lorenzo Stoakes
https://ljs.io

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH RESEND] drm/via: use get_user_pages_unlocked()

[Daniel Vetter]

On Tue, Feb 28, 2017 at 08:28:08PM +0000, Lorenzo Stoakes wrote:
> On 28 February 2017 at 19:35, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Tue, Feb 28, 2017 at 10:01:10AM +0100, Daniel Vetter wrote:
> >
> >> > +   ret = get_user_pages_unlocked((unsigned long)xfer->mem_addr,
> >> > +                   vsg->num_pages, vsg->pages,
> >> > +                   (vsg->direction == DMA_FROM_DEVICE) ? FOLL_WRITE : 0);
> >
> > Umm...  Why not
> >         ret = get_user_pages_fast((unsigned long)xfer->mem_addr,
> >                         vsg->num_pages,
> >                         vsg->direction == DMA_FROM_DEVICE,
> >                         vsg->pages);
> >
> > IOW, do you really need a warranty that ->mmap_sem will be grabbed and
> > released?
> 
> Daniel will be better placed to answer in this specific case, but more
> generally is there any reason why we can't just use
> get_user_pages_fast() in all such cases? These patches were simply a
> mechanical/cautious replacement for code that is more or less exactly
> equivalent but if this would make sense perhaps it'd be worth using
> gup_fast() where possible?

I have no idea. drm/via is unmaintained, it's a dri1 racy driver with
problems probably everywhere, and I'm not sure we even have someone left
who cares (there's an out-of-tree kms conversion of via, but it's stuck
since years).

In short, it's the drm dungeons and the only reason I merge patches is to
give people an easy target for test driving the patch submission process
to dri-devel. And to avoid drm being a blocker for tree-wide refactorings.
Otherwise 0 reasons to change anything here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>