| Message ID | 1457706567-14271-1-git-send-email-deathsimple@vodafone.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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.
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.
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
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
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; }