[PATCH] drm/amdgpu: allow write access to mapped userptrs

[PATCH] drm/amdgpu: allow write access to mapped userptrs

[Christian König]

From: Christian König <christian.koenig@amd.com>

With the updated MMU notifier we should also be able to
handle the writeback case correctly.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 7f6b4d9..5d280f6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -258,12 +258,10 @@ int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
 	    AMDGPU_GEM_USERPTR_REGISTER))
 		return -EINVAL;
 
-	if (!(args->flags & AMDGPU_GEM_USERPTR_READONLY) && (
-	     !(args->flags & AMDGPU_GEM_USERPTR_ANONONLY) ||
-	     !(args->flags & AMDGPU_GEM_USERPTR_REGISTER))) {
+	if (!(args->flags & AMDGPU_GEM_USERPTR_READONLY) &&
+	     !(args->flags & AMDGPU_GEM_USERPTR_REGISTER)) {
 
-		/* if we want to write to it we must require anonymous
-		   memory and install a MMU notifier */
+		/* if we want to write to it we must install a MMU notifier */
 		return -EACCES;
 	}
 
-- 
2.5.0

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

Re: [PATCH] drm/amdgpu: allow write access to mapped userptrs

[Alex Deucher]

On Fri, Mar 11, 2016 at 9:29 AM, Christian König
<deathsimple@vodafone.de> wrote:
> From: Christian König <christian.koenig@amd.com>
>
> With the updated MMU notifier we should also be able to
> handle the writeback case correctly.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>

Applied.  thanks!

Alex

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 7f6b4d9..5d280f6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -258,12 +258,10 @@ int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>             AMDGPU_GEM_USERPTR_REGISTER))
>                 return -EINVAL;
>
> -       if (!(args->flags & AMDGPU_GEM_USERPTR_READONLY) && (
> -            !(args->flags & AMDGPU_GEM_USERPTR_ANONONLY) ||
> -            !(args->flags & AMDGPU_GEM_USERPTR_REGISTER))) {
> +       if (!(args->flags & AMDGPU_GEM_USERPTR_READONLY) &&
> +            !(args->flags & AMDGPU_GEM_USERPTR_REGISTER)) {
>
> -               /* if we want to write to it we must require anonymous
> -                  memory and install a MMU notifier */
> +               /* if we want to write to it we must install a MMU notifier */
>                 return -EACCES;
>         }
>
> --
> 2.5.0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/amdgpu: allow write access to mapped userptrs

[Jerome Glisse]

On Fri, Mar 11, 2016 at 3:29 PM, Christian König
<deathsimple@vodafone.de> wrote:
> From: Christian König <christian.koenig@amd.com>
>
> With the updated MMU notifier we should also be able to
> handle the writeback case correctly.
>

Out of curiosity what are you refering too ? I do not see anything
special on amdgpu_mn.c logs and i do not see why you could not use
then for write before.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/amdgpu: allow write access to mapped userptrs

[Christian König]

Am 17.03.2016 um 20:18 schrieb Jerome Glisse:
> On Fri, Mar 11, 2016 at 3:29 PM, Christian König
> <deathsimple@vodafone.de> wrote:
>> From: Christian König <christian.koenig@amd.com>
>>
>> With the updated MMU notifier we should also be able to
>> handle the writeback case correctly.
>>
> Out of curiosity what are you refering too ? I do not see anything
> special on amdgpu_mn.c logs and i do not see why you could not use
> then for write before.

We moved the get_user_pages() outside of reserving the BO and tested 
that quite extensively.

And don't ask me why that shouldn't have worked. It was you who gave the 
advise to not allow it.

I think the rational was something like that the writeback code disables 
CPU writes, compute a CRC and start to write the page to disk. When the 
GPU could write to the page in that moment the CRC won't match any more 
and we would get errors reported from the disk driver.

But I think that the MMU notifier should catch that case as well.

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

Re: [PATCH] drm/amdgpu: allow write access to mapped userptrs

[Jerome Glisse]

On Thu, Mar 17, 2016 at 08:26:36PM +0100, Christian König wrote:
> Am 17.03.2016 um 20:18 schrieb Jerome Glisse:
> >On Fri, Mar 11, 2016 at 3:29 PM, Christian König
> ><deathsimple@vodafone.de> wrote:
> >>From: Christian König <christian.koenig@amd.com>
> >>
> >>With the updated MMU notifier we should also be able to
> >>handle the writeback case correctly.
> >>
> >Out of curiosity what are you refering too ? I do not see anything
> >special on amdgpu_mn.c logs and i do not see why you could not use
> >then for write before.
> 
> We moved the get_user_pages() outside of reserving the BO and tested that
> quite extensively.
> 
> And don't ask me why that shouldn't have worked. It was you who gave the
> advise to not allow it.
> 
> I think the rational was something like that the writeback code disables CPU
> writes, compute a CRC and start to write the page to disk. When the GPU
> could write to the page in that moment the CRC won't match any more and we
> would get errors reported from the disk driver.
> 
> But I think that the MMU notifier should catch that case as well.

So to make sure we talk about same code i am looking at drm-next-4.6 :
https://cgit.freedesktop.org/~agd5f/linux/tree/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c?h=drm-next-4.6&id=390be2824fa4211c2e973c69b72e04000559bba3

In which you only register invalidate_range_start and it is not enough
to handle page write back (see mm/rmap.c page_mkclean_one() call from
page_mkclean()). So this patch is kind of dangerous as it could end up
with filesystem corruption (depending on fs and how unlucky you are).

So you either need to add invalidate_page() callback to mmu_notifier or
at very least reinstate the AMDGPU_GEM_USERPTR_ANONONLY flag.

Cheers,
Jérôme
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/amdgpu: allow write access to mapped userptrs

[Christian König]

Am 18.03.2016 um 11:02 schrieb Jerome Glisse:
> On Thu, Mar 17, 2016 at 08:26:36PM +0100, Christian König wrote:
>> Am 17.03.2016 um 20:18 schrieb Jerome Glisse:
>>> On Fri, Mar 11, 2016 at 3:29 PM, Christian König
>>> <deathsimple@vodafone.de> wrote:
>>>> From: Christian König <christian.koenig@amd.com>
>>>>
>>>> With the updated MMU notifier we should also be able to
>>>> handle the writeback case correctly.
>>>>
>>> Out of curiosity what are you refering too ? I do not see anything
>>> special on amdgpu_mn.c logs and i do not see why you could not use
>>> then for write before.
>> We moved the get_user_pages() outside of reserving the BO and tested that
>> quite extensively.
>>
>> And don't ask me why that shouldn't have worked. It was you who gave the
>> advise to not allow it.
>>
>> I think the rational was something like that the writeback code disables CPU
>> writes, compute a CRC and start to write the page to disk. When the GPU
>> could write to the page in that moment the CRC won't match any more and we
>> would get errors reported from the disk driver.
>>
>> But I think that the MMU notifier should catch that case as well.
> So to make sure we talk about same code i am looking at drm-next-4.6 :
> https://cgit.freedesktop.org/~agd5f/linux/tree/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c?h=drm-next-4.6&id=390be2824fa4211c2e973c69b72e04000559bba3
>
> In which you only register invalidate_range_start and it is not enough
> to handle page write back (see mm/rmap.c page_mkclean_one() call from
> page_mkclean()). So this patch is kind of dangerous as it could end up
> with filesystem corruption (depending on fs and how unlucky you are).
>
> So you either need to add invalidate_page() callback to mmu_notifier or
> at very least reinstate the AMDGPU_GEM_USERPTR_ANONONLY flag.

Oh, crap. Yeah thanks for the info. I will be looking into that.

Either I can come up with a quick solution or we need to revert that 
once more.

Regards,
Christian.

>
> Cheers,
> Jérôme

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