Re: [PATCH] drm/amdkfd: correct the SVM DMA device unmap direction

[Christian König <christian.koenig@xxxxxxx>]

Am 05.11.24 um 17:34 schrieb Felix Kuehling:
> On 2024-11-05 06:04, Christian König wrote:
> > Am 05.11.24 um 03:33 schrieb Prike Liang:
> > > The SVM DMA device unmap direction should be same as
> > > the DMA map process.
> >
> > At least of hand that looks like it's only papering over a major 
> > problem.
> >
> > Why are DMA ranges for SVM mapped with a direction in the first 
> > place? That is usually not something we should do.
>
> These are DMA mappings of system memory pages. I guess we're creating 
> DMA mappings only for the access required for the migration, which is 
> not bidirectional. I see we do something similar for userptr mappings 
> depending on whether the GPU mapping is read-only or read-write. Is 
> that wrong for userptrs as well?

I think so, yes. The DMA directions are there to make explicit CPU cache 
management and bounce buffers possible.

Since we shouldn't need or even want either for a cache coherent PCIe 
device we should probably always use BIDIRECTIONAL.

Regards,
Christian.

> Regards,
>   Felix
>
>
> > Regards,
> > Christian.
> >
> > > Signed-off-by: Prike Liang <Prike.Liang@xxxxxxx>
> > > ---
> > >   drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 4 ++--
> > >   drivers/gpu/drm/amd/amdkfd/kfd_svm.c     | 6 +++---
> > >   drivers/gpu/drm/amd/amdkfd/kfd_svm.h     | 3 ++-
> > >   3 files changed, 7 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c 
> > > b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
> > > index eacfeb32f35d..9d83bb9dd004 100644
> > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
> > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
> > > @@ -445,7 +445,7 @@ svm_migrate_vma_to_vram(struct kfd_node *node, 
> > > struct svm_range *prange,
> > >       pr_debug("successful/cpages/npages 0x%lx/0x%lx/0x%lx\n",
> > >                mpages, cpages, migrate.npages);
> > >   -    svm_range_dma_unmap_dev(adev->dev, scratch, 0, npages);
> > > +    svm_range_dma_unmap_dev(adev->dev, scratch, 0, npages, 
> > > DMA_TO_DEVICE);
> > >     out_free:
> > >       kvfree(buf);
> > > @@ -750,7 +750,7 @@ svm_migrate_vma_to_ram(struct kfd_node *node, 
> > > struct svm_range *prange,
> > >       svm_migrate_copy_done(adev, mfence);
> > >       migrate_vma_finalize(&migrate);
> > >   -    svm_range_dma_unmap_dev(adev->dev, scratch, 0, npages);
> > > +    svm_range_dma_unmap_dev(adev->dev, scratch, 0, npages, 
> > > DMA_FROM_DEVICE);
> > >     out_free:
> > >       kvfree(buf);
> > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c 
> > > b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> > > index 3e2911895c74..c21485fe6cbb 100644
> > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> > > @@ -233,9 +233,9 @@ svm_range_dma_map(struct svm_range *prange, 
> > > unsigned long *bitmap,
> > >   }
> > >     void svm_range_dma_unmap_dev(struct device *dev, dma_addr_t 
> > > *dma_addr,
> > > -             unsigned long offset, unsigned long npages)
> > > +             unsigned long offset, unsigned long npages,
> > > +                enum dma_data_direction dir)
> > >   {
> > > -    enum dma_data_direction dir = DMA_BIDIRECTIONAL;
> > >       int i;
> > >         if (!dma_addr)
> > > @@ -272,7 +272,7 @@ void svm_range_dma_unmap(struct svm_range *prange)
> > >           }
> > >           dev = &pdd->dev->adev->pdev->dev;
> > >   -        svm_range_dma_unmap_dev(dev, dma_addr, 0, prange->npages);
> > > +        svm_range_dma_unmap_dev(dev, dma_addr, 0, prange->npages, 
> > > DMA_BIDIRECTIONAL);
> > >       }
> > >   }
> > >   diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h 
> > > b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> > > index bddd24f04669..5370d68bc5b2 100644
> > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> > > @@ -182,7 +182,8 @@ void svm_range_add_list_work(struct 
> > > svm_range_list *svms,
> > >                    enum svm_work_list_ops op);
> > >   void schedule_deferred_list_work(struct svm_range_list *svms);
> > >   void svm_range_dma_unmap_dev(struct device *dev, dma_addr_t 
> > > *dma_addr,
> > > -             unsigned long offset, unsigned long npages);
> > > +             unsigned long offset, unsigned long npages,
> > > +             enum dma_data_direction dir);
> > >   void svm_range_dma_unmap(struct svm_range *prange);
> > >   int svm_range_get_info(struct kfd_process *p, uint32_t 
> > > *num_svm_ranges,
> > >                  uint64_t *svm_priv_data_size);