[PATCH] drm/ttm: make sure pool pages are cleared

[PATCH] drm/ttm: make sure pool pages are cleared

[Christian König]

The old implementation wasn't consistend on this.

But it looks like we depend on this so better bring it back.

Signed-off-by: Christian König <christian.koenig@amd.com>
Reported-and-tested-by: Mike Galbraith <efault@gmx.de>
Fixes: d099fc8f540a ("drm/ttm: new TT backend allocation pool v3")
---
 drivers/gpu/drm/ttm/ttm_pool.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
index 74bf1c84b637..6e27cb1bf48b 100644
--- a/drivers/gpu/drm/ttm/ttm_pool.c
+++ b/drivers/gpu/drm/ttm/ttm_pool.c
@@ -33,6 +33,7 @@
 
 #include <linux/module.h>
 #include <linux/dma-mapping.h>
+#include <linux/highmem.h>
 
 #ifdef CONFIG_X86
 #include <asm/set_memory.h>
@@ -218,6 +219,15 @@ static void ttm_pool_unmap(struct ttm_pool *pool, dma_addr_t dma_addr,
 /* Give pages into a specific pool_type */
 static void ttm_pool_type_give(struct ttm_pool_type *pt, struct page *p)
 {
+	unsigned int i, num_pages = 1 << pt->order;
+
+	for (i = 0; i < num_pages; ++i) {
+		if (PageHighMem(p))
+			clear_highpage(p + i);
+		else
+			clear_page(page_address(p + i));
+	}
+
 	spin_lock(&pt->lock);
 	list_add(&p->lru, &pt->pages);
 	spin_unlock(&pt->lock);
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ttm: make sure pool pages are cleared

[Daniel Vetter]

On Wed, Feb 10, 2021 at 5:05 PM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> The old implementation wasn't consistend on this.
>
> But it looks like we depend on this so better bring it back.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Reported-and-tested-by: Mike Galbraith <efault@gmx.de>
> Fixes: d099fc8f540a ("drm/ttm: new TT backend allocation pool v3")

Well I think in general there's a bit an issue in ttm with not
clearing stuff everywhere. So definitely in favour of clearing stuff.
Looking at the code this only fixes the clearing, at alloc time we're
still very optional with clearing. I think we should just set
__GFP_ZERO unconditionally there too.

With that: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/ttm/ttm_pool.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index 74bf1c84b637..6e27cb1bf48b 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> @@ -33,6 +33,7 @@
>
>  #include <linux/module.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/highmem.h>
>
>  #ifdef CONFIG_X86
>  #include <asm/set_memory.h>
> @@ -218,6 +219,15 @@ static void ttm_pool_unmap(struct ttm_pool *pool, dma_addr_t dma_addr,
>  /* Give pages into a specific pool_type */
>  static void ttm_pool_type_give(struct ttm_pool_type *pt, struct page *p)
>  {
> +       unsigned int i, num_pages = 1 << pt->order;
> +
> +       for (i = 0; i < num_pages; ++i) {
> +               if (PageHighMem(p))
> +                       clear_highpage(p + i);
> +               else
> +                       clear_page(page_address(p + i));
> +       }
> +
>         spin_lock(&pt->lock);
>         list_add(&p->lru, &pt->pages);
>         spin_unlock(&pt->lock);
> --
> 2.25.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ttm: make sure pool pages are cleared

[Christian König]

Am 10.02.21 um 19:15 schrieb Daniel Vetter:
> On Wed, Feb 10, 2021 at 5:05 PM Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
>> The old implementation wasn't consistend on this.
>>
>> But it looks like we depend on this so better bring it back.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Reported-and-tested-by: Mike Galbraith <efault@gmx.de>
>> Fixes: d099fc8f540a ("drm/ttm: new TT backend allocation pool v3")
> Well I think in general there's a bit an issue in ttm with not
> clearing stuff everywhere. So definitely in favour of clearing stuff.
> Looking at the code this only fixes the clearing, at alloc time we're
> still very optional with clearing. I think we should just set
> __GFP_ZERO unconditionally there too.

No, the alloc handling is actually correct.

We are clearing only when we allocate pages for userspace. Not for the 
kernel nor for eviction when pages are overwritten anyway.

The key point is that the old page pool was ignoring the flag for this 
in some code paths and I wasn't sure if that's still necessary or not.

Turned out it was necessary after all.

Thanks,
Christian.

>
> With that: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
>> ---
>>   drivers/gpu/drm/ttm/ttm_pool.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
>> index 74bf1c84b637..6e27cb1bf48b 100644
>> --- a/drivers/gpu/drm/ttm/ttm_pool.c
>> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
>> @@ -33,6 +33,7 @@
>>
>>   #include <linux/module.h>
>>   #include <linux/dma-mapping.h>
>> +#include <linux/highmem.h>
>>
>>   #ifdef CONFIG_X86
>>   #include <asm/set_memory.h>
>> @@ -218,6 +219,15 @@ static void ttm_pool_unmap(struct ttm_pool *pool, dma_addr_t dma_addr,
>>   /* Give pages into a specific pool_type */
>>   static void ttm_pool_type_give(struct ttm_pool_type *pt, struct page *p)
>>   {
>> +       unsigned int i, num_pages = 1 << pt->order;
>> +
>> +       for (i = 0; i < num_pages; ++i) {
>> +               if (PageHighMem(p))
>> +                       clear_highpage(p + i);
>> +               else
>> +                       clear_page(page_address(p + i));
>> +       }
>> +
>>          spin_lock(&pt->lock);
>>          list_add(&p->lru, &pt->pages);
>>          spin_unlock(&pt->lock);
>> --
>> 2.25.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ttm: make sure pool pages are cleared

[Daniel Vetter]

On Wed, Feb 10, 2021 at 09:23:52PM +0100, Christian König wrote:
> 
> 
> Am 10.02.21 um 19:15 schrieb Daniel Vetter:
> > On Wed, Feb 10, 2021 at 5:05 PM Christian König
> > <ckoenig.leichtzumerken@gmail.com> wrote:
> > > The old implementation wasn't consistend on this.
> > > 
> > > But it looks like we depend on this so better bring it back.
> > > 
> > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > > Reported-and-tested-by: Mike Galbraith <efault@gmx.de>
> > > Fixes: d099fc8f540a ("drm/ttm: new TT backend allocation pool v3")
> > Well I think in general there's a bit an issue in ttm with not
> > clearing stuff everywhere. So definitely in favour of clearing stuff.
> > Looking at the code this only fixes the clearing, at alloc time we're
> > still very optional with clearing. I think we should just set
> > __GFP_ZERO unconditionally there too.
> 
> No, the alloc handling is actually correct.
> 
> We are clearing only when we allocate pages for userspace. Not for the
> kernel nor for eviction when pages are overwritten anyway.
> 
> The key point is that the old page pool was ignoring the flag for this in
> some code paths and I wasn't sure if that's still necessary or not.
> 
> Turned out it was necessary after all.

Somehow my git grep went wrong and I didn't find the users. You're right,
and I learned a few things more :-)

I'm kinda wondering, should we perhaps move the clearing to the use side,
and then only do when required? Might allow us to save it quite a few
times when we're thrashing around buffers in/out of vram, at the cost of
moving it to the alloc side for other cases.

Just an idea.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ttm: make sure pool pages are cleared

[Christian König]

Am 11.02.21 um 16:58 schrieb Daniel Vetter:
> On Wed, Feb 10, 2021 at 09:23:52PM +0100, Christian König wrote:
>>
>> Am 10.02.21 um 19:15 schrieb Daniel Vetter:
>>> On Wed, Feb 10, 2021 at 5:05 PM Christian König
>>> <ckoenig.leichtzumerken@gmail.com> wrote:
>>>> The old implementation wasn't consistend on this.
>>>>
>>>> But it looks like we depend on this so better bring it back.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> Reported-and-tested-by: Mike Galbraith <efault@gmx.de>
>>>> Fixes: d099fc8f540a ("drm/ttm: new TT backend allocation pool v3")
>>> Well I think in general there's a bit an issue in ttm with not
>>> clearing stuff everywhere. So definitely in favour of clearing stuff.
>>> Looking at the code this only fixes the clearing, at alloc time we're
>>> still very optional with clearing. I think we should just set
>>> __GFP_ZERO unconditionally there too.
>> No, the alloc handling is actually correct.
>>
>> We are clearing only when we allocate pages for userspace. Not for the
>> kernel nor for eviction when pages are overwritten anyway.
>>
>> The key point is that the old page pool was ignoring the flag for this in
>> some code paths and I wasn't sure if that's still necessary or not.
>>
>> Turned out it was necessary after all.
> Somehow my git grep went wrong and I didn't find the users. You're right,
> and I learned a few things more :-)
>
> I'm kinda wondering, should we perhaps move the clearing to the use side,
> and then only do when required? Might allow us to save it quite a few
> times when we're thrashing around buffers in/out of vram, at the cost of
> moving it to the alloc side for other cases.

I was playing with that idea in my mind as well.

The key argument against it is that the pool can optimize by clearing on 
free instead of during allocation.

This way we also implement a bit heuristic in the pool and have a list 
of cleared and not cleared pages.

Regards,
Christian.

>
> Just an idea.
> -Daniel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel