* Re: [RFC PATCH] habanalabs: goya: make use of dma_mmap_coherent
[not found] <20200817134752.11344-1-hdanton@sina.com>
@ 2020-08-22 16:10 ` Oded Gabbay
[not found] ` <20200822233242.7840-1-hdanton@sina.com>
0 siblings, 1 reply; 4+ messages in thread
From: Oded Gabbay @ 2020-08-22 16:10 UTC (permalink / raw)
To: Hillf Danton; +Cc: Christoph Hellwig, LKML, SW_Drivers, Zhang Li, Ding Z Nan
On Mon, Aug 17, 2020 at 4:48 PM Hillf Danton <hdanton@sina.com> wrote:
>
>
> Add dma_mmap_coherent() to match dma_alloc_coherent(), see the Link tag
> for why.
>
> Link: https://lore.kernel.org/lkml/20200609091727.GA23814@lst.de/
> Signed-off-by: Hillf Danton <hdanton@sina.com>
> ---
>
> --- 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/goya/goya.c
> +++ b/drivers/misc/habanalabs/goya/goya.c
> @@ -2642,15 +2642,14 @@ 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);
>
> --- 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;
>
Hi Hillf,
Could you please also fix gaudi_cb_mmap() and resend this patch ?
Thanks,
Oded
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] habanalabs: goya: make use of dma_mmap_coherent
[not found] ` <20200822233242.7840-1-hdanton@sina.com>
@ 2020-08-23 7:01 ` Oded Gabbay
[not found] ` <20200823081913.6132-1-hdanton@sina.com>
0 siblings, 1 reply; 4+ messages in thread
From: Oded Gabbay @ 2020-08-23 7:01 UTC (permalink / raw)
To: Hillf Danton; +Cc: Christoph Hellwig, LKML, SW_Drivers, Zhang Li, Ding Z Nan
On Sun, Aug 23, 2020 at 2:32 AM Hillf Danton <hdanton@sina.com> wrote:
>
>
> On Sat, 22 Aug 2020 19:10:07 +0300 Oded Gabbay wrote:
> > On Mon, Aug 17, 2020 at 4:48 PM Hillf Danton <hdanton@sina.com> wrote:
> > >
> > >
> > > Add dma_mmap_coherent() to match dma_alloc_coherent(), see the Link tag
> > > for why.
> > >
> > > Link: https://lore.kernel.org/lkml/20200609091727.GA23814@lst.de/
> > > Signed-off-by: Hillf Danton <hdanton@sina.com>
> > > ---
> > >
> > > --- 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/goya/goya.c
> > > +++ b/drivers/misc/habanalabs/goya/goya.c
> > > @@ -2642,15 +2642,14 @@ 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);
> > >
> > > --- 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;
> > >
> >
> > Hi Hillf,
> > Could you please also fix gaudi_cb_mmap() and resend this patch ?
>
> ---8<---
> From: Hillf Danton <hdanton@sina.com>
> 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@lst.de/
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Zhang Li <li.zhang@bitmain.com>
> Cc: Ding Z Nan <oshack@hotmail.com>
> Signed-off-by: Hillf Danton <hdanton@sina.com>
> ---
>
> --- 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/goya/goya.c
> +++ b/drivers/misc/habanalabs/goya/goya.c
> @@ -2642,15 +2642,14 @@ 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);
>
> --- 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/gaudi/gaudi.c
> +++ b/drivers/misc/habanalabs/gaudi/gaudi.c
> @@ -3063,15 +3063,14 @@ 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);
>
> --
>
Thanks!
One more small thing, can you please change the error message content
from "remap_pfn_range error" to "dma_mmap_coherent error" ?
Oded
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] habanalabs: goya: make use of dma_mmap_coherent
[not found] ` <20200823081913.6132-1-hdanton@sina.com>
@ 2020-08-23 11:01 ` Oded Gabbay
[not found] ` <20200825010509.10660-1-hdanton@sina.com>
0 siblings, 1 reply; 4+ messages in thread
From: Oded Gabbay @ 2020-08-23 11:01 UTC (permalink / raw)
To: Hillf Danton; +Cc: Christoph Hellwig, LKML, SW_Drivers, Zhang Li, Ding Z Nan
On Sun, Aug 23, 2020 at 11:19 AM Hillf Danton <hdanton@sina.com> 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@sina.com>
> 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@lst.de/
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Zhang Li <li.zhang@bitmain.com>
> Cc: Ding Z Nan <oshack@hotmail.com>
> Signed-off-by: Hillf Danton <hdanton@sina.com>
> ---
>
> --- 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:
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
Bottom line, I can't take this patch until we find what's wrong.
Oded
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] habanalabs: goya: make use of dma_mmap_coherent
[not found] ` <20200825010509.10660-1-hdanton@sina.com>
@ 2020-08-29 14:44 ` Oded Gabbay
0 siblings, 0 replies; 4+ messages in thread
From: Oded Gabbay @ 2020-08-29 14:44 UTC (permalink / raw)
To: Hillf Danton; +Cc: Christoph Hellwig, LKML, SW_Drivers, Zhang Li, Ding Z Nan
On Tue, Aug 25, 2020 at 4:05 AM Hillf Danton <hdanton@sina.com> 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@sina.com> 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@sina.com>
> > > > 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@lst.de/
> > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > Cc: Zhang Li <li.zhang@bitmain.com>
> > > > Cc: Ding Z Nan <oshack@hotmail.com>
> > > > Signed-off-by: Hillf Danton <hdanton@sina.com>
> > > > ---
> > > >
> > > > --- 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-08-29 14:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200817134752.11344-1-hdanton@sina.com>
2020-08-22 16:10 ` [RFC PATCH] habanalabs: goya: make use of dma_mmap_coherent Oded Gabbay
[not found] ` <20200822233242.7840-1-hdanton@sina.com>
2020-08-23 7:01 ` Oded Gabbay
[not found] ` <20200823081913.6132-1-hdanton@sina.com>
2020-08-23 11:01 ` Oded Gabbay
[not found] ` <20200825010509.10660-1-hdanton@sina.com>
2020-08-29 14:44 ` Oded Gabbay
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).