[PATCH] exynos: Put a stop to the userptr heresy.

2014-07-10 Thread Daniel Vetter 
On Thu, Jul 10, 2014 at 12:39:24AM +0900, Inki Dae wrote:
> On 2014? 07? 09? 18:23, Daniel Vetter wrote:
> > On Tue, Jul 8, 2014 at 6:20 PM, Inki Dae  wrote:
> >> 2014-07-08 22:37 GMT+09:00 Daniel Vetter :
> >>> On Wed, Jul 02, 2014 at 11:25:19AM -0400, Jerome Glisse wrote:
>  Anyway as this is upstream i guess you can keep it. This is just an 
>  horrible
>  API that allow to circumvant any limit set by memcg for page locking and 
>  all.
>  But anyway GPU driver never played in the same ballpark as other driver.
> >>>
> >>> I agree that exynos userptr as-is should be removed since as opposed to
> >>> the i915 implementation it doesn't play nice with the core mm
> >>
> >> Can you give me more details why you think so?
> > 
> >>From a very quick look there's two pieces:
> > - The implementation with the vma tricks looks _really_ scary. You'd
> > need to have Al Viro's opinion on it though.
> 
> You mean that it checks VM_DONTCOPY flag before copying vma? If so, I
> really forgot it.
> 
> > - If I'm reading the code correctly userspace can pin unlimted amounts
> > of memory, but I've gotten a bit lost in the code. In i915 we have
> 
> Not so. g2d driver is checking if user-requested buffer size is more
> than maximum capacity of g2d dma. So it can never pin unlimited amounts
> of memory.

Ah I didn't spot this check. I guess if the g2d dma size is a natural
limit then we're fine. Nowadays on recent i915 hw/sw we can use all of
system memory, so can't rely on some hw limit to limit the amount of
memory userspace can mlock. But if g2d has that (and there's enough memory
left for the kernel to not fall over) you should be fine.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] exynos: Put a stop to the userptr heresy.

2014-07-10 Thread Inki Dae 
On 2014? 07? 09? 18:23, Daniel Vetter wrote:
> On Tue, Jul 8, 2014 at 6:20 PM, Inki Dae  wrote:
>> 2014-07-08 22:37 GMT+09:00 Daniel Vetter :
>>> On Wed, Jul 02, 2014 at 11:25:19AM -0400, Jerome Glisse wrote:
 Anyway as this is upstream i guess you can keep it. This is just an 
 horrible
 API that allow to circumvant any limit set by memcg for page locking and 
 all.
 But anyway GPU driver never played in the same ballpark as other driver.
>>>
>>> I agree that exynos userptr as-is should be removed since as opposed to
>>> the i915 implementation it doesn't play nice with the core mm
>>
>> Can you give me more details why you think so?
> 
>>From a very quick look there's two pieces:
> - The implementation with the vma tricks looks _really_ scary. You'd
> need to have Al Viro's opinion on it though.

You mean that it checks VM_DONTCOPY flag before copying vma? If so, I
really forgot it.

> - If I'm reading the code correctly userspace can pin unlimted amounts
> of memory, but I've gotten a bit lost in the code. In i915 we have

Not so. g2d driver is checking if user-requested buffer size is more
than maximum capacity of g2d dma. So it can never pin unlimited amounts
of memory.

> shrinkers and mmu notifier to make sure that if the vm needs this
> memory again we'll make it available.

I am not familiar to mmu notifier. I will look into how i915 driver
handle it.

Thanks for comments,
Inki Dae

> -Daniel
>

[PATCH] exynos: Put a stop to the userptr heresy.

2014-07-09 Thread Daniel Vetter 
On Tue, Jul 8, 2014 at 6:20 PM, Inki Dae  wrote:
> 2014-07-08 22:37 GMT+09:00 Daniel Vetter :
>> On Wed, Jul 02, 2014 at 11:25:19AM -0400, Jerome Glisse wrote:
>>> Anyway as this is upstream i guess you can keep it. This is just an horrible
>>> API that allow to circumvant any limit set by memcg for page locking and 
>>> all.
>>> But anyway GPU driver never played in the same ballpark as other driver.
>>
>> I agree that exynos userptr as-is should be removed since as opposed to
>> the i915 implementation it doesn't play nice with the core mm
>
> Can you give me more details why you think so?

>From a very quick look there's two pieces:
- The implementation with the vma tricks looks _really_ scary. You'd
need to have Al Viro's opinion on it though.
- If I'm reading the code correctly userspace can pin unlimted amounts
of memory, but I've gotten a bit lost in the code. In i915 we have
shrinkers and mmu notifier to make sure that if the vm needs this
memory again we'll make it available.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] exynos: Put a stop to the userptr heresy.

2014-07-09 Thread Inki Dae 
2014-07-08 22:37 GMT+09:00 Daniel Vetter :
> On Wed, Jul 02, 2014 at 11:25:19AM -0400, Jerome Glisse wrote:
>> Anyway as this is upstream i guess you can keep it. This is just an horrible
>> API that allow to circumvant any limit set by memcg for page locking and all.
>> But anyway GPU driver never played in the same ballpark as other driver.
>
> I agree that exynos userptr as-is should be removed since as opposed to
> the i915 implementation it doesn't play nice with the core mm

Can you give me more details why you think so?

> and allos
> unpriviledged userspace to exceed all mlock limits.

we can make the userptr ioctl to have master permission for the meantime.

Thanks,
Inki Dae

>
> I've taken considerable amounts of internal heat for rejecting i915
> userptr until the mmu notifier stuff was implemented and so really want
> exynos to fix this up asap.
>
> Dave?
>
> Thanks, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] exynos: Put a stop to the userptr heresy.

2014-07-08 Thread Daniel Vetter 
On Wed, Jul 02, 2014 at 11:25:19AM -0400, Jerome Glisse wrote:
> Anyway as this is upstream i guess you can keep it. This is just an horrible
> API that allow to circumvant any limit set by memcg for page locking and all.
> But anyway GPU driver never played in the same ballpark as other driver.

I agree that exynos userptr as-is should be removed since as opposed to
the i915 implementation it doesn't play nice with the core mm and allos
unpriviledged userspace to exceed all mlock limits.

I've taken considerable amounts of internal heat for rejecting i915
userptr until the mmu notifier stuff was implemented and so really want
exynos to fix this up asap.

Dave?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] exynos: Put a stop to the userptr heresy.

2014-07-03 Thread Inki Dae 
2014-07-02 0:03 GMT+09:00 Jerome Glisse :
> On Tue, Jul 01, 2014 at 05:55:25PM +0900, Inki Dae wrote:
>>
>> Hi Jerome,
>>
>>
>> On 2014? 07? 01? 06:46, j.glisse at gmail.com wrote:
>> > From: Jerome Glisse 
>> >
>> > get_user_pages gives no garanty that page it returns are still
>> > the one backing the vma by the time it returns. Thus any ioctl
>>
>> One of pages from get_user_pages() could be migrated? I think all the
>> pages are pinned so that they cannot be migrated. If not such case, is
>> there other case I missed?
>
> I thought the ksm or gdb path were forcing unmap and replace but they
> still abide by the pin so while page will not be replace you are still
> violating few things. First you are pining memory without any kind of
> accouting from memcg point of view hence userspace can use that as a
> vector of attack to deplete memory.

Hm... right. The attack vector could deplete of system memory because
g2d are pinning all pages from get_user_pages(). But all processes
cannot do it: g2d ioctls can be used by process authorized by master
or root process. Of course, if more strict rule is required, we can
limit the size of memory to be used by g2d.

>
> You also ignore any munmap that might happen, any munmap will not care
> about the page being pin or not. The vma will disappear.

The vma would mean userspace. Physical pages will be freed once g2d
dma releases them. The important thing is that the pages from
get_user_pages() are only valid while g2d dma is running so once the
dma operation is completed the pages will be freed. And also g2d
driver doesn't provide any interface that user process can access the
pages. Give me more details what is the problem?

>
> Nor you respect the page being remapped read only for page write back
> allowing the gpu to write to the page while I/O is in progress.

You mean that it is possible for the g2d dma to access the pages for
write while I/O operation: at this moment, the pages have read only
attribute?

>
>>
>> One more thing, do you think this issue cannot be resolved so you are
>> trying to remove userptr feature from g2d driver?
>>
>
> I am just thinking that this is a somewhat evil api, i understand how
> nice it is for things like zero copy upload/download. But it allows to
> go around some of the memory policy set by other part of the kernel.
>
> But i guess that given you map/unmap accross cmd ioctl that is close
> to the direct-io usage pattern and thus can be forgiven.
>
> Note that if you were to start using GUP-fast then you will also have
> to consider the mmu_notifier range invalidation.

s/GUP-fast/GPU-fast.

Hm.. I think the page tables, one is for user space and other is for
device space, are different each other: they - cpu and dma - use fully
separated page table in case of Exynos drm. So I don't understand why
g2d driver have to consider the mmu_nitifier or relevant things. If
there is my missing point, can you give me more details?

Thanks,
Inki Dae

>
> Cheers,
> J?r?me
>
>> Thanks,
>> Inki Dae
>>
>> > that rely on this behavior is broken and rely on pure luck. To
>> > avoid any false hope from userspace stop such useage by simply
>> > flat out returning -EFAULT. Better to have a reliable behavior
>> > than to depend on pure luck and currently observed behavior of
>> > mm code.
>> >
>> > Note this was not even compile tested but i think i did update
>> > all places.
>> >
>> > Signed-off-by: J?r?me Glisse 
>> > ---
>> >  drivers/gpu/drm/exynos/exynos_drm_drv.h |   1 -
>> >  drivers/gpu/drm/exynos/exynos_drm_g2d.c | 277 
>> > +---
>> >  drivers/gpu/drm/exynos/exynos_drm_gem.c |  60 ---
>> >  drivers/gpu/drm/exynos/exynos_drm_gem.h |  20 ---
>> >  4 files changed, 3 insertions(+), 355 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h 
>> > b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> > index 36535f3..7b55e89 100644
>> > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> > @@ -233,7 +233,6 @@ struct exynos_drm_g2d_private {
>> > struct device   *dev;
>> > struct list_headinuse_cmdlist;
>> > struct list_headevent_list;
>> > -   struct list_headuserptr_list;
>> >  };
>> >
>> >  struct exynos_drm_ipp_private {
>> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c 
>> > b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
>> > index 8001587..d0be6dc 100644
>> > --- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
>> > +++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
>> > @@ -118,9 +118,6 @@
>> >  #define G2D_CMDLIST_POOL_SIZE  (G2D_CMDLIST_SIZE * 
>> > G2D_CMDLIST_NUM)
>> >  #define G2D_CMDLIST_DATA_NUM   (G2D_CMDLIST_SIZE / 
>> > sizeof(u32) - 2)
>> >
>> > -/* maximum buffer pool size of userptr is 64MB as default */
>> > -#define MAX_POOL   (64 * 1024 * 1024)
>> > -
>> >  enum {
>> > BUF_TYPE_GEM = 1,
>> > BUF_TYPE_USERPTR,
>> > @@ -185,19 +182,6 @@ struct

[PATCH] exynos: Put a stop to the userptr heresy.

2014-07-02 Thread Jerome Glisse 
On Thu, Jul 03, 2014 at 12:09:25AM +0900, Inki Dae wrote:
> 2014-07-02 0:03 GMT+09:00 Jerome Glisse :
> > On Tue, Jul 01, 2014 at 05:55:25PM +0900, Inki Dae wrote:
> >>
> >> Hi Jerome,
> >>
> >>
> >> On 2014? 07? 01? 06:46, j.glisse at gmail.com wrote:
> >> > From: Jerome Glisse 
> >> >
> >> > get_user_pages gives no garanty that page it returns are still
> >> > the one backing the vma by the time it returns. Thus any ioctl
> >>
> >> One of pages from get_user_pages() could be migrated? I think all the
> >> pages are pinned so that they cannot be migrated. If not such case, is
> >> there other case I missed?
> >
> > I thought the ksm or gdb path were forcing unmap and replace but they
> > still abide by the pin so while page will not be replace you are still
> > violating few things. First you are pining memory without any kind of
> > accouting from memcg point of view hence userspace can use that as a
> > vector of attack to deplete memory.
> 
> Hm... right. The attack vector could deplete of system memory because
> g2d are pinning all pages from get_user_pages(). But all processes
> cannot do it: g2d ioctls can be used by process authorized by master
> or root process. Of course, if more strict rule is required, we can
> limit the size of memory to be used by g2d.

Well i just wanted to stress that pining page that are on the lru do not
have same effect for mm point of view a page that are part of regular gem
object for the gpu.


> 
> >
> > You also ignore any munmap that might happen, any munmap will not care
> > about the page being pin or not. The vma will disappear.
> 
> The vma would mean userspace. Physical pages will be freed once g2d
> dma releases them. The important thing is that the pages from
> get_user_pages() are only valid while g2d dma is running so once the
> dma operation is completed the pages will be freed. And also g2d
> driver doesn't provide any interface that user process can access the
> pages. Give me more details what is the problem?
> 
> >
> > Nor you respect the page being remapped read only for page write back
> > allowing the gpu to write to the page while I/O is in progress.
> 
> You mean that it is possible for the g2d dma to access the pages for
> write while I/O operation: at this moment, the pages have read only
> attribute?
> 
> >
> >>
> >> One more thing, do you think this issue cannot be resolved so you are
> >> trying to remove userptr feature from g2d driver?
> >>
> >
> > I am just thinking that this is a somewhat evil api, i understand how
> > nice it is for things like zero copy upload/download. But it allows to
> > go around some of the memory policy set by other part of the kernel.
> >
> > But i guess that given you map/unmap accross cmd ioctl that is close
> > to the direct-io usage pattern and thus can be forgiven.
> >
> > Note that if you were to start using GUP-fast then you will also have
> > to consider the mmu_notifier range invalidation.
> 
> s/GUP-fast/GPU-fast.
> 
> Hm.. I think the page tables, one is for user space and other is for
> device space, are different each other: they - cpu and dma - use fully
> separated page table in case of Exynos drm. So I don't understand why
> g2d driver have to consider the mmu_nitifier or relevant things. If
> there is my missing point, can you give me more details?

I was talking about get_user_pages_fast for which you have no garanty of
exclusion against fork, truncate, munmap, ...

Anyway as this is upstream i guess you can keep it. This is just an horrible
API that allow to circumvant any limit set by memcg for page locking and all.
But anyway GPU driver never played in the same ballpark as other driver.

Cheers,
J?r?me

[PATCH] exynos: Put a stop to the userptr heresy.

2014-07-01 Thread Inki Dae 

Hi Jerome,


On 2014? 07? 01? 06:46, j.glisse at gmail.com wrote:
> From: Jerome Glisse 
> 
> get_user_pages gives no garanty that page it returns are still
> the one backing the vma by the time it returns. Thus any ioctl

One of pages from get_user_pages() could be migrated? I think all the
pages are pinned so that they cannot be migrated. If not such case, is
there other case I missed?

One more thing, do you think this issue cannot be resolved so you are
trying to remove userptr feature from g2d driver?

Thanks,
Inki Dae

> that rely on this behavior is broken and rely on pure luck. To
> avoid any false hope from userspace stop such useage by simply
> flat out returning -EFAULT. Better to have a reliable behavior
> than to depend on pure luck and currently observed behavior of
> mm code.
> 
> Note this was not even compile tested but i think i did update
> all places.
> 
> Signed-off-by: J?r?me Glisse 
> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.h |   1 -
>  drivers/gpu/drm/exynos/exynos_drm_g2d.c | 277 
> +---
>  drivers/gpu/drm/exynos/exynos_drm_gem.c |  60 ---
>  drivers/gpu/drm/exynos/exynos_drm_gem.h |  20 ---
>  4 files changed, 3 insertions(+), 355 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h 
> b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index 36535f3..7b55e89 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -233,7 +233,6 @@ struct exynos_drm_g2d_private {
>   struct device   *dev;
>   struct list_headinuse_cmdlist;
>   struct list_headevent_list;
> - struct list_headuserptr_list;
>  };
>  
>  struct exynos_drm_ipp_private {
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c 
> b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> index 8001587..d0be6dc 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> @@ -118,9 +118,6 @@
>  #define G2D_CMDLIST_POOL_SIZE(G2D_CMDLIST_SIZE * 
> G2D_CMDLIST_NUM)
>  #define G2D_CMDLIST_DATA_NUM (G2D_CMDLIST_SIZE / sizeof(u32) - 2)
>  
> -/* maximum buffer pool size of userptr is 64MB as default */
> -#define MAX_POOL (64 * 1024 * 1024)
> -
>  enum {
>   BUF_TYPE_GEM = 1,
>   BUF_TYPE_USERPTR,
> @@ -185,19 +182,6 @@ struct drm_exynos_pending_g2d_event {
>   struct drm_exynos_g2d_event event;
>  };
>  
> -struct g2d_cmdlist_userptr {
> - struct list_headlist;
> - dma_addr_t  dma_addr;
> - unsigned long   userptr;
> - unsigned long   size;
> - struct page **pages;
> - unsigned intnpages;
> - struct sg_table *sgt;
> - struct vm_area_struct   *vma;
> - atomic_trefcount;
> - boolin_pool;
> - boolout_of_list;
> -};
>  struct g2d_cmdlist_node {
>   struct list_headlist;
>   struct g2d_cmdlist  *cmdlist;
> @@ -242,7 +226,6 @@ struct g2d_data {
>   struct kmem_cache   *runqueue_slab;
>  
>   unsigned long   current_pool;
> - unsigned long   max_pool;
>  };
>  
>  static int g2d_init_cmdlist(struct g2d_data *g2d)
> @@ -354,232 +337,6 @@ add_to_list:
>   list_add_tail(>event->base.link, _priv->event_list);
>  }
>  
> -static void g2d_userptr_put_dma_addr(struct drm_device *drm_dev,
> - unsigned long obj,
> - bool force)
> -{
> - struct g2d_cmdlist_userptr *g2d_userptr =
> - (struct g2d_cmdlist_userptr *)obj;
> -
> - if (!obj)
> - return;
> -
> - if (force)
> - goto out;
> -
> - atomic_dec(_userptr->refcount);
> -
> - if (atomic_read(_userptr->refcount) > 0)
> - return;
> -
> - if (g2d_userptr->in_pool)
> - return;
> -
> -out:
> - exynos_gem_unmap_sgt_from_dma(drm_dev, g2d_userptr->sgt,
> - DMA_BIDIRECTIONAL);
> -
> - exynos_gem_put_pages_to_userptr(g2d_userptr->pages,
> - g2d_userptr->npages,
> - g2d_userptr->vma);
> -
> - exynos_gem_put_vma(g2d_userptr->vma);
> -
> - if (!g2d_userptr->out_of_list)
> - list_del_init(_userptr->list);
> -
> - sg_free_table(g2d_userptr->sgt);
> - kfree(g2d_userptr->sgt);
> -
> - drm_free_large(g2d_userptr->pages);
> - kfree(g2d_userptr);
> -}
> -
> -static dma_addr_t *g2d_userptr_get_dma_addr(struct drm_device *drm_dev,
> - unsigned long userptr,
> - unsigned long size,
> - struct drm_file *filp,
> - unsigned long *obj)
> -{
> - struct drm_exynos_file_private

[PATCH] exynos: Put a stop to the userptr heresy.

2014-07-01 Thread Jerome Glisse 
On Tue, Jul 01, 2014 at 05:55:25PM +0900, Inki Dae wrote:
> 
> Hi Jerome,
> 
> 
> On 2014? 07? 01? 06:46, j.glisse at gmail.com wrote:
> > From: Jerome Glisse 
> >
> > get_user_pages gives no garanty that page it returns are still
> > the one backing the vma by the time it returns. Thus any ioctl
> 
> One of pages from get_user_pages() could be migrated? I think all the
> pages are pinned so that they cannot be migrated. If not such case, is
> there other case I missed?

I thought the ksm or gdb path were forcing unmap and replace but they
still abide by the pin so while page will not be replace you are still
violating few things. First you are pining memory without any kind of
accouting from memcg point of view hence userspace can use that as a
vector of attack to deplete memory.

You also ignore any munmap that might happen, any munmap will not care
about the page being pin or not. The vma will disappear.

Nor you respect the page being remapped read only for page write back
allowing the gpu to write to the page while I/O is in progress.

> 
> One more thing, do you think this issue cannot be resolved so you are
> trying to remove userptr feature from g2d driver?
> 

I am just thinking that this is a somewhat evil api, i understand how
nice it is for things like zero copy upload/download. But it allows to
go around some of the memory policy set by other part of the kernel.

But i guess that given you map/unmap accross cmd ioctl that is close
to the direct-io usage pattern and thus can be forgiven.

Note that if you were to start using GUP-fast then you will also have
to consider the mmu_notifier range invalidation.

Cheers,
J?r?me

> Thanks,
> Inki Dae
> 
> > that rely on this behavior is broken and rely on pure luck. To
> > avoid any false hope from userspace stop such useage by simply
> > flat out returning -EFAULT. Better to have a reliable behavior
> > than to depend on pure luck and currently observed behavior of
> > mm code.
> >
> > Note this was not even compile tested but i think i did update
> > all places.
> >
> > Signed-off-by: J?r?me Glisse 
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_drv.h |   1 -
> >  drivers/gpu/drm/exynos/exynos_drm_g2d.c | 277 
> > +---
> >  drivers/gpu/drm/exynos/exynos_drm_gem.c |  60 ---
> >  drivers/gpu/drm/exynos/exynos_drm_gem.h |  20 ---
> >  4 files changed, 3 insertions(+), 355 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h 
> > b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> > index 36535f3..7b55e89 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> > @@ -233,7 +233,6 @@ struct exynos_drm_g2d_private {
> > struct device   *dev;
> > struct list_headinuse_cmdlist;
> > struct list_headevent_list;
> > -   struct list_headuserptr_list;
> >  };
> >
> >  struct exynos_drm_ipp_private {
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c 
> > b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> > index 8001587..d0be6dc 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> > @@ -118,9 +118,6 @@
> >  #define G2D_CMDLIST_POOL_SIZE  (G2D_CMDLIST_SIZE * 
> > G2D_CMDLIST_NUM)
> >  #define G2D_CMDLIST_DATA_NUM   (G2D_CMDLIST_SIZE / sizeof(u32) 
> > - 2)
> >
> > -/* maximum buffer pool size of userptr is 64MB as default */
> > -#define MAX_POOL   (64 * 1024 * 1024)
> > -
> >  enum {
> > BUF_TYPE_GEM = 1,
> > BUF_TYPE_USERPTR,
> > @@ -185,19 +182,6 @@ struct drm_exynos_pending_g2d_event {
> > struct drm_exynos_g2d_event event;
> >  };
> >
> > -struct g2d_cmdlist_userptr {
> > -   struct list_headlist;
> > -   dma_addr_t  dma_addr;
> > -   unsigned long   userptr;
> > -   unsigned long   size;
> > -   struct page **pages;
> > -   unsigned intnpages;
> > -   struct sg_table *sgt;
> > -   struct vm_area_struct   *vma;
> > -   atomic_trefcount;
> > -   boolin_pool;
> > -   boolout_of_list;
> > -};
> >  struct g2d_cmdlist_node {
> > struct list_headlist;
> > struct g2d_cmdlist  *cmdlist;
> > @@ -242,7 +226,6 @@ struct g2d_data {
> > struct kmem_cache   *runqueue_slab;
> >
> > unsigned long   current_pool;
> > -   unsigned long   max_pool;
> >  };
> >
> >  static int g2d_init_cmdlist(struct g2d_data *g2d)
> > @@ -354,232 +337,6 @@ add_to_list:
> > list_add_tail(>event->base.link, _priv->event_list);
> >  }
> >
> > -static void g2d_userptr_put_dma_addr(struct drm_device *drm_dev,
> > -   unsigned long obj,
> > -   bool force)
> > -{
> > -   struct g2d_cmdlist_userptr *g2d_userptr =
> > -   (struct g2d_cmdlist_userptr *)obj;
> > -
>

[PATCH] exynos: Put a stop to the userptr heresy.

2014-06-30 Thread j.gli...@gmail.com 
From: Jerome Glisse 

get_user_pages gives no garanty that page it returns are still
the one backing the vma by the time it returns. Thus any ioctl
that rely on this behavior is broken and rely on pure luck. To
avoid any false hope from userspace stop such useage by simply
flat out returning -EFAULT. Better to have a reliable behavior
than to depend on pure luck and currently observed behavior of
mm code.

Note this was not even compile tested but i think i did update
all places.

Signed-off-by: J?r?me Glisse 
---
 drivers/gpu/drm/exynos/exynos_drm_drv.h |   1 -
 drivers/gpu/drm/exynos/exynos_drm_g2d.c | 277 +---
 drivers/gpu/drm/exynos/exynos_drm_gem.c |  60 ---
 drivers/gpu/drm/exynos/exynos_drm_gem.h |  20 ---
 4 files changed, 3 insertions(+), 355 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h 
b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index 36535f3..7b55e89 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -233,7 +233,6 @@ struct exynos_drm_g2d_private {
struct device   *dev;
struct list_headinuse_cmdlist;
struct list_headevent_list;
-   struct list_headuserptr_list;
 };

 struct exynos_drm_ipp_private {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c 
b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
index 8001587..d0be6dc 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
@@ -118,9 +118,6 @@
 #define G2D_CMDLIST_POOL_SIZE  (G2D_CMDLIST_SIZE * G2D_CMDLIST_NUM)
 #define G2D_CMDLIST_DATA_NUM   (G2D_CMDLIST_SIZE / sizeof(u32) - 2)

-/* maximum buffer pool size of userptr is 64MB as default */
-#define MAX_POOL   (64 * 1024 * 1024)
-
 enum {
BUF_TYPE_GEM = 1,
BUF_TYPE_USERPTR,
@@ -185,19 +182,6 @@ struct drm_exynos_pending_g2d_event {
struct drm_exynos_g2d_event event;
 };

-struct g2d_cmdlist_userptr {
-   struct list_headlist;
-   dma_addr_t  dma_addr;
-   unsigned long   userptr;
-   unsigned long   size;
-   struct page **pages;
-   unsigned intnpages;
-   struct sg_table *sgt;
-   struct vm_area_struct   *vma;
-   atomic_trefcount;
-   boolin_pool;
-   boolout_of_list;
-};
 struct g2d_cmdlist_node {
struct list_headlist;
struct g2d_cmdlist  *cmdlist;
@@ -242,7 +226,6 @@ struct g2d_data {
struct kmem_cache   *runqueue_slab;

unsigned long   current_pool;
-   unsigned long   max_pool;
 };

 static int g2d_init_cmdlist(struct g2d_data *g2d)
@@ -354,232 +337,6 @@ add_to_list:
list_add_tail(>event->base.link, _priv->event_list);
 }

-static void g2d_userptr_put_dma_addr(struct drm_device *drm_dev,
-   unsigned long obj,
-   bool force)
-{
-   struct g2d_cmdlist_userptr *g2d_userptr =
-   (struct g2d_cmdlist_userptr *)obj;
-
-   if (!obj)
-   return;
-
-   if (force)
-   goto out;
-
-   atomic_dec(_userptr->refcount);
-
-   if (atomic_read(_userptr->refcount) > 0)
-   return;
-
-   if (g2d_userptr->in_pool)
-   return;
-
-out:
-   exynos_gem_unmap_sgt_from_dma(drm_dev, g2d_userptr->sgt,
-   DMA_BIDIRECTIONAL);
-
-   exynos_gem_put_pages_to_userptr(g2d_userptr->pages,
-   g2d_userptr->npages,
-   g2d_userptr->vma);
-
-   exynos_gem_put_vma(g2d_userptr->vma);
-
-   if (!g2d_userptr->out_of_list)
-   list_del_init(_userptr->list);
-
-   sg_free_table(g2d_userptr->sgt);
-   kfree(g2d_userptr->sgt);
-
-   drm_free_large(g2d_userptr->pages);
-   kfree(g2d_userptr);
-}
-
-static dma_addr_t *g2d_userptr_get_dma_addr(struct drm_device *drm_dev,
-   unsigned long userptr,
-   unsigned long size,
-   struct drm_file *filp,
-   unsigned long *obj)
-{
-   struct drm_exynos_file_private *file_priv = filp->driver_priv;
-   struct exynos_drm_g2d_private *g2d_priv = file_priv->g2d_priv;
-   struct g2d_cmdlist_userptr *g2d_userptr;
-   struct g2d_data *g2d;
-   struct page **pages;
-   struct sg_table *sgt;
-   struct vm_area_struct *vma;
-   unsigned long start, end;
-   unsigned int npages, offset;
-   int ret;
-
-   if (!size) {
-   DRM_ERROR("invalid userptr size.\n");
-   return ERR_PTR(-EINVAL);
-   }
-
-   g2d =