Re: [RFC PATCH] habanalabs: goya: make use of dma_mmap_coherent
From: Oded Gabbay
Date: Sat Aug 29 2020 - 10:44:42 EST
On Tue, Aug 25, 2020 at 4:05 AM Hillf Danton <hdanton@xxxxxxxx> wrote:
>
>
> On Mon, Aug 24, 2020 at 09:29 Hillf Danton wrote:
> > On Sun, 23 Aug 2020 14:01:23 +0300 Oded Gabbay wrote:
> > > On Sun, Aug 23, 2020 at 11:19 AM Hillf Danton <hdanton@xxxxxxxx> wrote:
> > > > On Sun, 23 Aug 2020 10:01:59 +0300 Oded Gabbay wrote:
> > > > >
> > > > > One more small thing, can you please change the error message content
> > > > > from "remap_pfn_range error" to "dma_mmap_coherent error" ?
> > > >
> > > > My fault.
> > > >
> > > > ---8<---
> > > > From: Hillf Danton <hdanton@xxxxxxxx>
> > > > Subject: [PATCH] habanalabs: make use of dma_mmap_coherent
> > > >
> > > > Add dma_mmap_coherent() for goya and gaudi to match their use of
> > > > dma_alloc_coherent(), see the Link tag for why.
> > > >
> > > > Link: https://lore.kernel.org/lkml/20200609091727.GA23814@xxxxxx/
> > > > Cc: Christoph Hellwig <hch@xxxxxx>
> > > > Cc: Zhang Li <li.zhang@xxxxxxxxxxx>
> > > > Cc: Ding Z Nan <oshack@xxxxxxxxxxx>
> > > > Signed-off-by: Hillf Danton <hdanton@xxxxxxxx>
> > > > ---
> > > >
> > > > --- a/drivers/misc/habanalabs/common/habanalabs.h
> > > > +++ b/drivers/misc/habanalabs/common/habanalabs.h
> > > > @@ -679,7 +679,7 @@ struct hl_asic_funcs {
> > > > int (*suspend)(struct hl_device *hdev);
> > > > int (*resume)(struct hl_device *hdev);
> > > > int (*cb_mmap)(struct hl_device *hdev, struct vm_area_struct *vma,
> > > > - u64 kaddress, phys_addr_t paddress, u32 size);
> > > > + void *cpu_addr, dma_addr_t dma_addr, size_t size);
> > > > void (*ring_doorbell)(struct hl_device *hdev, u32 hw_queue_id, u32 pi);
> > > > void (*pqe_write)(struct hl_device *hdev, __le64 *pqe,
> > > > struct hl_bd *bd);
> > > > --- a/drivers/misc/habanalabs/common/command_buffer.c
> > > > +++ b/drivers/misc/habanalabs/common/command_buffer.c
> > > > @@ -299,7 +299,6 @@ int hl_cb_mmap(struct hl_fpriv *hpriv, s
> > > > {
> > > > struct hl_device *hdev = hpriv->hdev;
> > > > struct hl_cb *cb;
> > > > - phys_addr_t address;
> > > > u32 handle;
> > > > int rc;
> > > >
> > > > @@ -344,12 +343,8 @@ int hl_cb_mmap(struct hl_fpriv *hpriv, s
> > > >
> > > > vma->vm_private_data = cb;
> > > >
> > > > - /* Calculate address for CB */
> > > > - address = virt_to_phys((void *) (uintptr_t) cb->kernel_address);
> > > > -
> > > > - rc = hdev->asic_funcs->cb_mmap(hdev, vma, cb->kernel_address,
> > > > - address, cb->size);
> > > > -
> > > > + rc = hdev->asic_funcs->cb_mmap(hdev, vma, (void *) cb->kernel_address,
> > > > + cb->bus_address, cb->size);
> > > > if (rc) {
> > > > spin_lock(&cb->lock);
> > > > cb->mmap = false;
> > > > --- a/drivers/misc/habanalabs/goya/goya.c
> > > > +++ b/drivers/misc/habanalabs/goya/goya.c
> > > > @@ -2642,17 +2642,16 @@ int goya_resume(struct hl_device *hdev)
> > > > }
> > > >
> > > > static int goya_cb_mmap(struct hl_device *hdev, struct vm_area_struct *vma,
> > > > - u64 kaddress, phys_addr_t paddress, u32 size)
> > > > + void *cpu_addr, dma_addr_t dma_addr, size_t size)
> > > > {
> > > > int rc;
> > > >
> > > > vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP |
> > > > VM_DONTCOPY | VM_NORESERVE;
> > > >
> > > > - rc = remap_pfn_range(vma, vma->vm_start, paddress >> PAGE_SHIFT,
> > > > - size, vma->vm_page_prot);
> > > > + rc = dma_mmap_coherent(hdev->dev, vma, cpu_addr, dma_addr, size);
> > > > if (rc)
> > > > - dev_err(hdev->dev, "remap_pfn_range error %d", rc);
> > > > + dev_err(hdev->dev, "dma_mmap_coherent error %d", rc);
> > > >
> > > > return rc;
> > > > }
> > > > --- a/drivers/misc/habanalabs/gaudi/gaudi.c
> > > > +++ b/drivers/misc/habanalabs/gaudi/gaudi.c
> > > > @@ -3063,17 +3063,16 @@ static int gaudi_resume(struct hl_device
> > > > }
> > > >
> > > > static int gaudi_cb_mmap(struct hl_device *hdev, struct vm_area_struct *vma,
> > > > - u64 kaddress, phys_addr_t paddress, u32 size)
> > > > + void *cpu_addr, dma_addr_t dma_addr, size_t size)
> > > > {
> > > > int rc;
> > > >
> > > > vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP |
> > > > VM_DONTCOPY | VM_NORESERVE;
> > > >
> > > > - rc = remap_pfn_range(vma, vma->vm_start, paddress >> PAGE_SHIFT,
> > > > - size, vma->vm_page_prot);
> > > > + rc = dma_mmap_coherent(hdev->dev, vma, cpu_addr, dma_addr, size);
> > > > if (rc)
> > > > - dev_err(hdev->dev, "remap_pfn_range error %d", rc);
> > > > + dev_err(hdev->dev, "dma_mmap_coherent error %d", rc);
> > > >
> > > > return rc;
> > > > }
> > > > --
> > > >
> > >
> > > Hi Hillf,
> > > I took your patch and ran a very basic test with it and unfortunately it breaks.
> > > There are two bugs which I fixed:
> >
> > Thanks for your fix.
> >
> > > 1. Need to pass "&hdev->pdev->dev" instead of "hdev->dev" to
> > > dma_mmap_coherent because we use the PCI dev, not our driver's device.
> > > 2. Need to pass "dma_addr - HOST_PHYS_BASE" instead of "dma_addr" to
> > > dma_mmap_coherent because goya/gaudi alloc function adds
> > > HOST_PHYS_BASE
> > >
> > > But even after those fixes, I still get "-ENXIO" as a return value each time.
> > > I added prints in the driver to make sure I called dma_mmap_coherent
> > > with the exact same parameters as dma_alloc_coherent was called and I
> > > saw they were the same.
> > > e.g.
> > > habanalabs hl0: alloc CB:
> > > habanalabs hl0: kernel address == ffff9e6124691000
> > > habanalabs hl0: bus address == 0xa24691000
> > > habanalabs hl0: size == 0x1000
> > > habanalabs hl0: mapping CB:
> > > habanalabs hl0: user address == 0x7f674c4d9000
> > > habanalabs hl0: kernel address == ffff9e6124691000
> > > habanalabs hl0: bus address == 0xa24691000
> > > habanalabs hl0: vm_flags == 0x42644fb
> > > habanalabs hl0: size == 0x1000
> > > habanalabs hl0: vm_page_prot == 0x8000000000000027
> >
> > 1) it's a small mapping as the above size shows.
> >
> > 2) there is a boundary check in dma_direct_mmap(),
> >
> > if (vma->vm_pgoff >= count || user_count > count - vma->vm_pgoff)
> > return -ENXIO;
> >
> > and I'm wondering if we didn't go across it, e.g vm_pgoff isn't zero.
>
> 3) the grep output seems to have something to say.
>
> _>grep -n idr_ linux/drivers/misc/habanalabs/common/command_buffer.c
> 168: rc = idr_alloc(&mgr->cb_handles, cb, 1, 0, GFP_ATOMIC);
> 215: cb = idr_find(&mgr->cb_handles, handle);
> 217: idr_remove(&mgr->cb_handles, handle);
> 376: cb = idr_find(&mgr->cb_handles, handle);
> 401: idr_init(&mgr->cb_handles);
> 412: idr_for_each_entry(idp, cb, id) {
> 419: idr_destroy(&mgr->cb_handles);
>
> First, the key managed by idr is never zero as the alloc method feeds
> back keys that are bigger than one.
>
> Second, no mmap callback, which is the target of this patch, will be
> invoked for a command buffer that cann't be successfully found in idr
> with vma->vm_pgoff used as the key in hl_cb_mmap().
>
> handle = vma->vm_pgoff;
>
> /* reference was taken here */
> cb = hl_cb_get(hdev, &hpriv->cb_mgr, handle);
> if (!cb) {
> dev_err(hdev->dev,
> "CB mmap failed, no match to handle 0x%x\n", handle);
> return -EINVAL;
> }
>
> IOW vm_pgoff is reused by habana as idr key.
>
> Finally, the helper in the dma core is trying to fix the reuse and
> got your tests broken.
> >
> > Hillf
> > >
> > > Bottom line, I can't take this patch until we find what's wrong.
> > > Oded
>
Hi Hillf,
Thanks for the detailed analysis.
With your findings, I was able to make your patch work by clearing the
vm_pgoff after extracting from it the idr handle.
It also gave me the chance to do some nice refactoring of the mmap
function in the driver.
So I'll add your patch after my two patches which do the refactoring +
clearing the handle.
Thanks so much for your time and contribution :)
Oded