Re: [RFC, v3 9/9] media: platform: Add Mediatek ISP P1 shared memory device
On Fri, Jul 26, 2019 at 8:59 PM Jungo Lin wrote:
>
> Hi Robin:
>
> On Fri, 2019-07-26 at 12:04 +0100, Robin Murphy wrote:
> > On 26/07/2019 08:42, Tomasz Figa wrote:
> > > On Fri, Jul 26, 2019 at 4:41 PM Christoph Hellwig
> > > wrote:
> > >>
> > >> On Fri, Jul 26, 2019 at 02:15:14PM +0900, Tomasz Figa wrote:
> > >>> Could you try dma_get_sgtable() with the SCP struct device and then
> > >>> dma_map_sg() with the P1 struct device?
> > >>
> > >> Please don't do that. dma_get_sgtable is a pretty broken API (see
> > >> the common near the arm implementation) and we should not add more
> > >> users of it. If you want a piece of memory that can be mapped to
> > >> multiple devices allocate it using alloc_pages and then just map
> > >> it to each device.
> > >
> > > Thanks for taking a look at this thread.
> > >
> > > Unfortunately that wouldn't work. We have a specific reserved memory
> > > pool that is the only memory area accessible to one of the devices.
> > > Any idea how to handle this?
> >
> > If it's reserved in the sense of being outside struct-page-backed
> > "kernel memory", then provided you have a consistent CPU physical
> > address it might be reasonable for other devices to access it via
> > dma_map_resource().
> >
> > Robin.
>
> Thank you for your suggestion.
>
> After revising to use dma_map_resource(), it is worked. Below is the
> current implementation. Pleas kindly help us to check if there is any
> misunderstanding.
>
> #define MTK_ISP_COMPOSER_MEM_SIZE 0x20
>
> /*
> * Allocate coherent reserved memory for SCP firmware usage.
> * The size of SCP composer's memory is fixed to 0x20
> * for the requirement of firmware.
> */
> ptr = dma_alloc_coherent(p1_dev->cam_dev.smem_dev,
> MTK_ISP_COMPOSER_MEM_SIZE, &addr,
> GFP_KERNEL);
> if (!ptr) {
> dev_err(dev, "failed to allocate compose memory\n");
> return -ENOMEM;
> }
> p1_dev->composer_scp_addr = addr;
> p1_dev->composer_virt_addr = ptr;
> dev_dbg(dev, "scp addr:%pad va:%pK\n", &addr, ptr);
>
> /*
> * This reserved memory is also be used by ISP P1 HW.
> * Need to get iova address for ISP P1 DMA.
> */
> addr = dma_map_resource(dev, addr, MTK_ISP_COMPOSER_MEM_SIZE,
> DMA_BIDIRECTIONAL, DMA_ATTR_SKIP_CPU_SYNC);
This is still incorrect, because addr is a DMA address, but the second
argument to dma_map_resource() is a physical address.
> if (dma_mapping_error(dev, addr)) {
> dev_err(dev, "Failed to map scp iova\n");
> ret = -ENOMEM;
> goto fail_free_mem;
> }
> p1_dev->composer_iova = addr;
> dev_info(dev, "scp iova addr:%pad\n", &addr);
>
> Moreover, appropriate Tomasz & Christoph's help on this issue.
Robin, the memory is specified using the reserved-memory DT binding
and managed by the coherent DMA pool framework. We can allocate from
it using dma_alloc_coherent(), which gives us a DMA address, not CPU
physial address (although in practice on this platform they are equal
numerically).
Best regards,
Tomasz
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC, v3 9/9] media: platform: Add Mediatek ISP P1 shared memory device
Hi Robin:
On Fri, 2019-07-26 at 12:04 +0100, Robin Murphy wrote:
> On 26/07/2019 08:42, Tomasz Figa wrote:
> > On Fri, Jul 26, 2019 at 4:41 PM Christoph Hellwig
> > wrote:
> >>
> >> On Fri, Jul 26, 2019 at 02:15:14PM +0900, Tomasz Figa wrote:
> >>> Could you try dma_get_sgtable() with the SCP struct device and then
> >>> dma_map_sg() with the P1 struct device?
> >>
> >> Please don't do that. dma_get_sgtable is a pretty broken API (see
> >> the common near the arm implementation) and we should not add more
> >> users of it. If you want a piece of memory that can be mapped to
> >> multiple devices allocate it using alloc_pages and then just map
> >> it to each device.
> >
> > Thanks for taking a look at this thread.
> >
> > Unfortunately that wouldn't work. We have a specific reserved memory
> > pool that is the only memory area accessible to one of the devices.
> > Any idea how to handle this?
>
> If it's reserved in the sense of being outside struct-page-backed
> "kernel memory", then provided you have a consistent CPU physical
> address it might be reasonable for other devices to access it via
> dma_map_resource().
>
> Robin.
Thank you for your suggestion.
After revising to use dma_map_resource(), it is worked. Below is the
current implementation. Pleas kindly help us to check if there is any
misunderstanding.
#define MTK_ISP_COMPOSER_MEM_SIZE 0x20
/*
* Allocate coherent reserved memory for SCP firmware usage.
* The size of SCP composer's memory is fixed to 0x20
* for the requirement of firmware.
*/
ptr = dma_alloc_coherent(p1_dev->cam_dev.smem_dev,
MTK_ISP_COMPOSER_MEM_SIZE, &addr, GFP_KERNEL);
if (!ptr) {
dev_err(dev, "failed to allocate compose memory\n");
return -ENOMEM;
}
p1_dev->composer_scp_addr = addr;
p1_dev->composer_virt_addr = ptr;
dev_dbg(dev, "scp addr:%pad va:%pK\n", &addr, ptr);
/*
* This reserved memory is also be used by ISP P1 HW.
* Need to get iova address for ISP P1 DMA.
*/
addr = dma_map_resource(dev, addr, MTK_ISP_COMPOSER_MEM_SIZE,
DMA_BIDIRECTIONAL, DMA_ATTR_SKIP_CPU_SYNC);
if (dma_mapping_error(dev, addr)) {
dev_err(dev, "Failed to map scp iova\n");
ret = -ENOMEM;
goto fail_free_mem;
}
p1_dev->composer_iova = addr;
dev_info(dev, "scp iova addr:%pad\n", &addr);
Moreover, appropriate Tomasz & Christoph's help on this issue.
Best regards,
Jungo
Re: [RFC, v3 9/9] media: platform: Add Mediatek ISP P1 shared memory device
On 26/07/2019 08:42, Tomasz Figa wrote: On Fri, Jul 26, 2019 at 4:41 PM Christoph Hellwig wrote: On Fri, Jul 26, 2019 at 02:15:14PM +0900, Tomasz Figa wrote: Could you try dma_get_sgtable() with the SCP struct device and then dma_map_sg() with the P1 struct device? Please don't do that. dma_get_sgtable is a pretty broken API (see the common near the arm implementation) and we should not add more users of it. If you want a piece of memory that can be mapped to multiple devices allocate it using alloc_pages and then just map it to each device. Thanks for taking a look at this thread. Unfortunately that wouldn't work. We have a specific reserved memory pool that is the only memory area accessible to one of the devices. Any idea how to handle this? If it's reserved in the sense of being outside struct-page-backed "kernel memory", then provided you have a consistent CPU physical address it might be reasonable for other devices to access it via dma_map_resource(). Robin.
Re: [RFC, v3 9/9] media: platform: Add Mediatek ISP P1 shared memory device
On Fri, Jul 26, 2019 at 4:41 PM Christoph Hellwig wrote: > > On Fri, Jul 26, 2019 at 02:15:14PM +0900, Tomasz Figa wrote: > > Could you try dma_get_sgtable() with the SCP struct device and then > > dma_map_sg() with the P1 struct device? > > Please don't do that. dma_get_sgtable is a pretty broken API (see > the common near the arm implementation) and we should not add more > users of it. If you want a piece of memory that can be mapped to > multiple devices allocate it using alloc_pages and then just map > it to each device. Thanks for taking a look at this thread. Unfortunately that wouldn't work. We have a specific reserved memory pool that is the only memory area accessible to one of the devices. Any idea how to handle this? Best regards, Tomasz
Re: [RFC, v3 9/9] media: platform: Add Mediatek ISP P1 shared memory device
On Fri, Jul 26, 2019 at 02:15:14PM +0900, Tomasz Figa wrote: > Could you try dma_get_sgtable() with the SCP struct device and then > dma_map_sg() with the P1 struct device? Please don't do that. dma_get_sgtable is a pretty broken API (see the common near the arm implementation) and we should not add more users of it. If you want a piece of memory that can be mapped to multiple devices allocate it using alloc_pages and then just map it to each device.
Re: [RFC, v3 9/9] media: platform: Add Mediatek ISP P1 shared memory device
On Tue, Jul 23, 2019 at 5:22 PM Jungo Lin wrote:
>
> Hi, Tomasz:
>
> On Tue, 2019-07-23 at 16:20 +0900, Tomasz Figa wrote:
> > Hi Jungo,
> >
> > On Fri, Jul 5, 2019 at 4:59 PM Jungo Lin wrote:
> > >
> > > Hi Tomasz:
> > >
> > > On Fri, 2019-07-05 at 13:22 +0900, Tomasz Figa wrote:
> > > > Hi Jungo,
> > > >
> > > > On Fri, Jul 5, 2019 at 12:33 PM Jungo Lin
> > > > wrote:
> > > > >
> > > > > Hi Tomasz,
> > >
> > > [snip]
> > >
> > > > > After applying your suggestion in SCP device driver, we could remove
> > > > > mtk_cam-smem.h/c. Currently, we use dma_alloc_coherent with SCP device
> > > > > to get SCP address. We could touch the buffer with this SCP address in
> > > > > SCP processor.
> > > > >
> > > > > After that, we use dma_map_page_attrs with P1 device which supports
> > > > > IOMMU domain to get IOVA address. For this address, we will assign
> > > > > it to our ISP HW device to proceed.
> > > > >
> > > > > Below is the snippet for ISP P1 compose buffer initialization.
> > > > >
> > > > > ptr = dma_alloc_coherent(p1_dev->cam_dev.smem_dev,
> > > > > MAX_COMPOSER_SIZE, &addr,
> > > > > GFP_KERNEL);
> > > > > if (!ptr) {
> > > > > dev_err(dev, "failed to allocate compose memory\n");
> > > > > return -ENOMEM;
> > > > > }
> > > > > isp_ctx->scp_mem_pa = addr;
> > > >
> > > > addr contains a DMA address, not a physical address. Could we call it
> > > > scp_mem_dma instead?
> > > >
> > > > > dev_dbg(dev, "scp addr:%pad\n", &addr);
> > > > >
> > > > > /* get iova address */
> > > > > addr = dma_map_page_attrs(dev, phys_to_page(addr), 0,
> > > >
> > > > addr is a DMA address, so phys_to_page() can't be called on it. The
> > > > simplest thing here would be to use dma_map_single() with ptr as the
> > > > CPU address expected.
> > > >
> > >
> > > We have changed to use ma_map_single() with ptr, but encounter IOMMU
> > > error. From the debug log of iommu_dma_map_page[3], we got
> > > 0x5480 instead of expected address: 0x5080[2].
> > > There is a address offset(0x400). If we change to use
> > > dma_map_page_attrs with phys_to_page(addr), the address is correct as we
> > > expected[2]. Do you have any suggestion on this issue? Do we miss
> > > something?
> >
> > Sorry for the late reply. Could you show me the code changes you made
> > to use dma_map_single()? It would sound like the virtual address
> > passed to dma_map_single() isn't correct.
> >
> > Best regards,
> > Tomasz
> >
>
>
> Please check the below code snippet in today's testing.
>
> p1_dev->cam_dev.smem_dev = &p1_dev->scp_pdev->dev;
> ptr = dma_alloc_coherent(p1_dev->cam_dev.smem_dev,
> MTK_ISP_COMPOSER_MEM_SIZE, &addr,
> GFP_KERNEL);
> if (!ptr) {
> dev_err(dev, "failed to allocate compose memory\n");
> return -ENOMEM;
> }
> p1_dev->composer_scp_addr = addr;
> p1_dev->composer_virt_addr = ptr;
> dev_info(dev, "scp addr:%pad va:%pK\n", &addr, ptr);
>
> /* get iova address */
> addr = dma_map_single(dev, ptr, MTK_ISP_COMPOSER_MEM_SIZE,
> DMA_BIDIRECTIONAL);
> if (dma_mapping_error(dev, addr)) {
> dma_free_coherent(p1_dev->cam_dev.smem_dev,
> MTK_ISP_COMPOSER_MEM_SIZE,
> ptr, p1_dev->composer_scp_addr);
> dev_err(dev, "Failed to map scp iova\n");
> ret = -ENOMEM;
> goto fail_free_mem;
> }
> p1_dev->composer_iova = addr;
> dev_info(dev, "scp iova addr:%pad\n", &addr);
>
> Moreover, below is extracted log[2].
>
> We guess the virtual address which is returned by dma_alloc_coherent
> function is not valid kernel logical address. It is actually returned by
> memremap() in dma_init_coherent_memory(). Moreover, dma_map_single()
> will call virt_to_page() function. For virt_to_page function, it
> requires a logical address[1].
>
> [1]https://www.oreilly.com/library/view/linux-device-drivers/0596005903/ch15.html
>
Indeed virt_to_page() works only with kernel LOWMEM addresses. Whether
virt_to_page() is the right thing to do in dma_map_single() is a good
question, but let's assume it was implemented like this for a reason.
However, you also can't call phys_to_page() on the DMA addresses
returned by dma_alloc_*() either. It works just by luck, because SCP
DMA addresses and CPU physical addresses are numerically the same.
Could you try dma_get_sgtable() with the SCP struct device and then
dma_map_sg() with the P1 struct device?
Best regards,
Tomasz
> [2]
> 322 [1.238269] mtk-cam-p1 1a006000.camisp: scp
> addr:0x5200 va:a3adc471
> 323 [1.239582] mtk-cam-p1 1a006000.camisp: scp iova
> addr:0xfde0
> 7716 [1.238963] mtk-cam-p1 1a006000.camisp: scp
> addr:0x5200
Re: [RFC, v3 9/9] media: platform: Add Mediatek ISP P1 shared memory device
Hi, Tomasz:
On Tue, 2019-07-23 at 16:20 +0900, Tomasz Figa wrote:
> Hi Jungo,
>
> On Fri, Jul 5, 2019 at 4:59 PM Jungo Lin wrote:
> >
> > Hi Tomasz:
> >
> > On Fri, 2019-07-05 at 13:22 +0900, Tomasz Figa wrote:
> > > Hi Jungo,
> > >
> > > On Fri, Jul 5, 2019 at 12:33 PM Jungo Lin wrote:
> > > >
> > > > Hi Tomasz,
> >
> > [snip]
> >
> > > > After applying your suggestion in SCP device driver, we could remove
> > > > mtk_cam-smem.h/c. Currently, we use dma_alloc_coherent with SCP device
> > > > to get SCP address. We could touch the buffer with this SCP address in
> > > > SCP processor.
> > > >
> > > > After that, we use dma_map_page_attrs with P1 device which supports
> > > > IOMMU domain to get IOVA address. For this address, we will assign
> > > > it to our ISP HW device to proceed.
> > > >
> > > > Below is the snippet for ISP P1 compose buffer initialization.
> > > >
> > > > ptr = dma_alloc_coherent(p1_dev->cam_dev.smem_dev,
> > > > MAX_COMPOSER_SIZE, &addr, GFP_KERNEL);
> > > > if (!ptr) {
> > > > dev_err(dev, "failed to allocate compose memory\n");
> > > > return -ENOMEM;
> > > > }
> > > > isp_ctx->scp_mem_pa = addr;
> > >
> > > addr contains a DMA address, not a physical address. Could we call it
> > > scp_mem_dma instead?
> > >
> > > > dev_dbg(dev, "scp addr:%pad\n", &addr);
> > > >
> > > > /* get iova address */
> > > > addr = dma_map_page_attrs(dev, phys_to_page(addr), 0,
> > >
> > > addr is a DMA address, so phys_to_page() can't be called on it. The
> > > simplest thing here would be to use dma_map_single() with ptr as the
> > > CPU address expected.
> > >
> >
> > We have changed to use ma_map_single() with ptr, but encounter IOMMU
> > error. From the debug log of iommu_dma_map_page[3], we got
> > 0x5480 instead of expected address: 0x5080[2].
> > There is a address offset(0x400). If we change to use
> > dma_map_page_attrs with phys_to_page(addr), the address is correct as we
> > expected[2]. Do you have any suggestion on this issue? Do we miss
> > something?
>
> Sorry for the late reply. Could you show me the code changes you made
> to use dma_map_single()? It would sound like the virtual address
> passed to dma_map_single() isn't correct.
>
> Best regards,
> Tomasz
>
Please check the below code snippet in today's testing.
p1_dev->cam_dev.smem_dev = &p1_dev->scp_pdev->dev;
ptr = dma_alloc_coherent(p1_dev->cam_dev.smem_dev,
MTK_ISP_COMPOSER_MEM_SIZE, &addr, GFP_KERNEL);
if (!ptr) {
dev_err(dev, "failed to allocate compose memory\n");
return -ENOMEM;
}
p1_dev->composer_scp_addr = addr;
p1_dev->composer_virt_addr = ptr;
dev_info(dev, "scp addr:%pad va:%pK\n", &addr, ptr);
/* get iova address */
addr = dma_map_single(dev, ptr, MTK_ISP_COMPOSER_MEM_SIZE,
DMA_BIDIRECTIONAL);
if (dma_mapping_error(dev, addr)) {
dma_free_coherent(p1_dev->cam_dev.smem_dev,
MTK_ISP_COMPOSER_MEM_SIZE,
ptr, p1_dev->composer_scp_addr);
dev_err(dev, "Failed to map scp iova\n");
ret = -ENOMEM;
goto fail_free_mem;
}
p1_dev->composer_iova = addr;
dev_info(dev, "scp iova addr:%pad\n", &addr);
Moreover, below is extracted log[2].
We guess the virtual address which is returned by dma_alloc_coherent
function is not valid kernel logical address. It is actually returned by
memremap() in dma_init_coherent_memory(). Moreover, dma_map_single()
will call virt_to_page() function. For virt_to_page function, it
requires a logical address[1].
[1]https://www.oreilly.com/library/view/linux-device-drivers/0596005903/ch15.html
[2]
322 [1.238269] mtk-cam-p1 1a006000.camisp: scp
addr:0x5200 va:a3adc471
323 [1.239582] mtk-cam-p1 1a006000.camisp: scp iova
addr:0xfde0
7716 [1.238963] mtk-cam-p1 1a006000.camisp: scp
addr:0x5200 va:42ec580f
7717 [1.240276] mtk-cam-p1 1a006000.camisp: scp iova
addr:0xfde0
15088 [1.239309] mtk-cam-p1 1a006000.camisp: scp
addr:0x5200 va:5e5b3462
15089 [1.240626] mtk-cam-p1 1a006000.camisp: scp iova
addr:0xfde0
Best regards,
Jungo
> >
> > [1]
> > [1.344786] __dma_alloc_from_coherent: 0x80 PAGE_SHIFT:12
> > device_base:0x5000 dma:0x5080
> > virt_base:ff801400 va:ff801480
> >
> > [1.346890] mtk-cam 1a00.camisp: scp addr:0x5080
> > va:ff801480
> >
> > [1.347864] iommu_dma_map_page:0x5480 offset:0
> > [1.348562] mtk-cam 1a00.camisp: iova addr:0xfde0
> >
> > [2]
> > [1.346738] __dma_alloc_from_coherent: 0x80 PAGE_SHIFT:12
> > dev
Re: [RFC,v3 9/9] media: platform: Add Mediatek ISP P1 shared memory device
Hi Jungo,
On Fri, Jul 5, 2019 at 4:59 PM Jungo Lin wrote:
>
> Hi Tomasz:
>
> On Fri, 2019-07-05 at 13:22 +0900, Tomasz Figa wrote:
> > Hi Jungo,
> >
> > On Fri, Jul 5, 2019 at 12:33 PM Jungo Lin wrote:
> > >
> > > Hi Tomasz,
>
> [snip]
>
> > > After applying your suggestion in SCP device driver, we could remove
> > > mtk_cam-smem.h/c. Currently, we use dma_alloc_coherent with SCP device
> > > to get SCP address. We could touch the buffer with this SCP address in
> > > SCP processor.
> > >
> > > After that, we use dma_map_page_attrs with P1 device which supports
> > > IOMMU domain to get IOVA address. For this address, we will assign
> > > it to our ISP HW device to proceed.
> > >
> > > Below is the snippet for ISP P1 compose buffer initialization.
> > >
> > > ptr = dma_alloc_coherent(p1_dev->cam_dev.smem_dev,
> > > MAX_COMPOSER_SIZE, &addr, GFP_KERNEL);
> > > if (!ptr) {
> > > dev_err(dev, "failed to allocate compose memory\n");
> > > return -ENOMEM;
> > > }
> > > isp_ctx->scp_mem_pa = addr;
> >
> > addr contains a DMA address, not a physical address. Could we call it
> > scp_mem_dma instead?
> >
> > > dev_dbg(dev, "scp addr:%pad\n", &addr);
> > >
> > > /* get iova address */
> > > addr = dma_map_page_attrs(dev, phys_to_page(addr), 0,
> >
> > addr is a DMA address, so phys_to_page() can't be called on it. The
> > simplest thing here would be to use dma_map_single() with ptr as the
> > CPU address expected.
> >
>
> We have changed to use ma_map_single() with ptr, but encounter IOMMU
> error. From the debug log of iommu_dma_map_page[3], we got
> 0x5480 instead of expected address: 0x5080[2].
> There is a address offset(0x400). If we change to use
> dma_map_page_attrs with phys_to_page(addr), the address is correct as we
> expected[2]. Do you have any suggestion on this issue? Do we miss
> something?
Sorry for the late reply. Could you show me the code changes you made
to use dma_map_single()? It would sound like the virtual address
passed to dma_map_single() isn't correct.
Best regards,
Tomasz
>
> [1]
> [1.344786] __dma_alloc_from_coherent: 0x80 PAGE_SHIFT:12
> device_base:0x5000 dma:0x5080
> virt_base:ff801400 va:ff801480
>
> [1.346890] mtk-cam 1a00.camisp: scp addr:0x5080
> va:ff801480
>
> [1.347864] iommu_dma_map_page:0x5480 offset:0
> [1.348562] mtk-cam 1a00.camisp: iova addr:0xfde0
>
> [2]
> [1.346738] __dma_alloc_from_coherent: 0x80 PAGE_SHIFT:12
> device_base:0x5000 dma:0x5080
> virt_base:ff801400 va:ff801480
> [1.348841] mtk-cam 1a00.camisp: scp addr:0x5080
> va:ff801480
> [1.349816] iommu_dma_map_page:0x5080 offset:0
> [1.350514] mtk-cam 1a00.camisp: iova addr:0xfde0
>
>
> [3]
> dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
> unsigned long offset, size_t size, int prot)
> {
> phys_addr_t phys = page_to_phys(page);
> pr_err("iommu_dma_map_page:%pa offset:%lu\n", &phys, offset);
>
> return __iommu_dma_map(dev, page_to_phys(page) + offset, size, prot,
> iommu_get_dma_domain(dev));
> }
>
> [snip]
>
> Best regards,
>
> Jungo
>
Re: [RFC,v3 9/9] media: platform: Add Mediatek ISP P1 shared memory device
Hi Tomasz:
On Fri, 2019-07-05 at 13:22 +0900, Tomasz Figa wrote:
> Hi Jungo,
>
> On Fri, Jul 5, 2019 at 12:33 PM Jungo Lin wrote:
> >
> > Hi Tomasz,
[snip]
> > After applying your suggestion in SCP device driver, we could remove
> > mtk_cam-smem.h/c. Currently, we use dma_alloc_coherent with SCP device
> > to get SCP address. We could touch the buffer with this SCP address in
> > SCP processor.
> >
> > After that, we use dma_map_page_attrs with P1 device which supports
> > IOMMU domain to get IOVA address. For this address, we will assign
> > it to our ISP HW device to proceed.
> >
> > Below is the snippet for ISP P1 compose buffer initialization.
> >
> > ptr = dma_alloc_coherent(p1_dev->cam_dev.smem_dev,
> > MAX_COMPOSER_SIZE, &addr, GFP_KERNEL);
> > if (!ptr) {
> > dev_err(dev, "failed to allocate compose memory\n");
> > return -ENOMEM;
> > }
> > isp_ctx->scp_mem_pa = addr;
>
> addr contains a DMA address, not a physical address. Could we call it
> scp_mem_dma instead?
>
> > dev_dbg(dev, "scp addr:%pad\n", &addr);
> >
> > /* get iova address */
> > addr = dma_map_page_attrs(dev, phys_to_page(addr), 0,
>
> addr is a DMA address, so phys_to_page() can't be called on it. The
> simplest thing here would be to use dma_map_single() with ptr as the
> CPU address expected.
>
We have changed to use ma_map_single() with ptr, but encounter IOMMU
error. From the debug log of iommu_dma_map_page[3], we got
0x5480 instead of expected address: 0x5080[2].
There is a address offset(0x400). If we change to use
dma_map_page_attrs with phys_to_page(addr), the address is correct as we
expected[2]. Do you have any suggestion on this issue? Do we miss
something?
[1]
[1.344786] __dma_alloc_from_coherent: 0x80 PAGE_SHIFT:12
device_base:0x5000 dma:0x5080
virt_base:ff801400 va:ff801480
[1.346890] mtk-cam 1a00.camisp: scp addr:0x5080
va:ff801480
[1.347864] iommu_dma_map_page:0x5480 offset:0
[1.348562] mtk-cam 1a00.camisp: iova addr:0xfde0
[2]
[1.346738] __dma_alloc_from_coherent: 0x80 PAGE_SHIFT:12
device_base:0x5000 dma:0x5080
virt_base:ff801400 va:ff801480
[1.348841] mtk-cam 1a00.camisp: scp addr:0x5080
va:ff801480
[1.349816] iommu_dma_map_page:0x5080 offset:0
[1.350514] mtk-cam 1a00.camisp: iova addr:0xfde0
[3]
dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
unsigned long offset, size_t size, int prot)
{
phys_addr_t phys = page_to_phys(page);
pr_err("iommu_dma_map_page:%pa offset:%lu\n", &phys, offset);
return __iommu_dma_map(dev, page_to_phys(page) + offset, size, prot,
iommu_get_dma_domain(dev));
}
[snip]
Best regards,
Jungo
Re: [RFC,v3 9/9] media: platform: Add Mediatek ISP P1 shared memory device
Hi, Tomasz:
On Fri, 2019-07-05 at 13:22 +0900, Tomasz Figa wrote:
> Hi Jungo,
>
> On Fri, Jul 5, 2019 at 12:33 PM Jungo Lin wrote:
> >
> > Hi Tomasz,
> >
> > On Mon, 2019-07-01 at 16:25 +0900, Tomasz Figa wrote:
> > > Hi Jungo,
> > >
> > > On Tue, Jun 11, 2019 at 11:53:44AM +0800, Jungo Lin wrote:
> > > > The purpose of this child device is to provide shared
> > > > memory management for exchanging tuning data between co-processor
> > > > and the Pass 1 unit of the camera ISP system, including cache
> > > > buffer handling.
> > > >
> > >
> > > Looks like we haven't really progressed on getting this replaced with
> > > something that doesn't require so much custom code. Let me propose
> > > something
> > > better then.
> > >
> > > We already have a reserved memory mode in DT. If it has a compatible
> > > string
> > > of "shared-dma-pool", it would be registered in the coherent DMA framework
> > > [1]. That would make it available for consumer devices to look-up.
> > >
> > > Now if we add a "memory-region" property to the SCP device node and point
> > > it
> > > to our reserved memory node, the SCP driver could look it up and hook to
> > > the
> > > DMA mapping API using of_reserved_mem_device_init_by_idx[2].
> > >
> > > That basically makes any dma_alloc_*(), dma_map_*(), etc. calls on the SCP
> > > struct device use the coherent DMA ops, which operate on the assigned
> > > memory
> > > pool. With that, the P1 driver could just directly use those calls to
> > > manage the memory, without any custom code.
> > >
> > > There is an example how this setup works in the s5p-mfc driver[3], but it
> > > needs to be noted that it creates child nodes, because it can have more
> > > than
> > > 1 DMA port, which may need its own memory pool. In our case, we wouldn't
> > > need child nodes and could just use the SCP device directly.
> > >
> > > [1]
> > > https://elixir.bootlin.com/linux/v5.2-rc7/source/kernel/dma/coherent.c#L345
> > > [2]
> > > https://elixir.bootlin.com/linux/v5.2-rc7/source/drivers/of/of_reserved_mem.c#L312
> > > [3]
> > > https://elixir.bootlin.com/linux/v5.2-rc7/source/drivers/media/platform/s5p-mfc/s5p_mfc.c#L1075
> > >
> > > Let me also post some specific comments below, in case we end up still
> > > needing any of the code.
> > >
> >
> > Thanks your suggestions.
> >
> > After applying your suggestion in SCP device driver, we could remove
> > mtk_cam-smem.h/c. Currently, we use dma_alloc_coherent with SCP device
> > to get SCP address. We could touch the buffer with this SCP address in
> > SCP processor.
> >
> > After that, we use dma_map_page_attrs with P1 device which supports
> > IOMMU domain to get IOVA address. For this address, we will assign
> > it to our ISP HW device to proceed.
> >
> > Below is the snippet for ISP P1 compose buffer initialization.
> >
> > ptr = dma_alloc_coherent(p1_dev->cam_dev.smem_dev,
> > MAX_COMPOSER_SIZE, &addr, GFP_KERNEL);
> > if (!ptr) {
> > dev_err(dev, "failed to allocate compose memory\n");
> > return -ENOMEM;
> > }
> > isp_ctx->scp_mem_pa = addr;
>
> addr contains a DMA address, not a physical address. Could we call it
> scp_mem_dma instead?
>
Ok, we will rename this.
> > dev_dbg(dev, "scp addr:%pad\n", &addr);
> >
> > /* get iova address */
> > addr = dma_map_page_attrs(dev, phys_to_page(addr), 0,
>
> addr is a DMA address, so phys_to_page() can't be called on it. The
> simplest thing here would be to use dma_map_single() with ptr as the
> CPU address expected.
>
Got it. We will revise to use dma_map_single() with ptr.
> > MAX_COMPOSER_SIZE, DMA_BIDIRECTIONAL,
> > DMA_ATTR_SKIP_CPU_SYNC);
> > if (dma_mapping_error(dev, addr)) {
> > isp_ctx->scp_mem_pa = 0;
>
> We also need to free the allocated memory.
>
Ok, we will add the dma_unmap_single to free the allocated memory.
> > dev_err(dev, "Failed to map scp iova\n");
> > return -ENOMEM;
> > }
> > isp_ctx->scp_mem_iova = addr;
> >
> > Moreover, we have another meta input buffer usage.
> > For this kind of buffer, it will be allocated by V4L2 framework
> > with dma_alloc_coherent with SCP device. In order to get IOVA,
> > we will add dma_map_page_attrs in vb2_ops' buf_init function.
> > In buf_cleanup function, we will call dma_unmap_page_attrs function.
>
> As per above, we don't have access to the struct page we want to map.
> We probably want to get the CPU VA using vb2_plane_vaddr() and call
> dma_map_single() instead.
>
Got it. We will revise this to use dma_map_single() with CPU VA which is
got from vb2_plane_vaddr() function.
> >
> > Based on these current implementation, do you think it is correct?
> > If we got any wrong, please let us know.
> >
> > Btw, we also DMA_ATTR_NO_KERNEL_MAPPING DMA attribte to
> > avoid dma_sy
Re: [RFC,v3 9/9] media: platform: Add Mediatek ISP P1 shared memory device
Hi Jungo,
On Fri, Jul 5, 2019 at 12:33 PM Jungo Lin wrote:
>
> Hi Tomasz,
>
> On Mon, 2019-07-01 at 16:25 +0900, Tomasz Figa wrote:
> > Hi Jungo,
> >
> > On Tue, Jun 11, 2019 at 11:53:44AM +0800, Jungo Lin wrote:
> > > The purpose of this child device is to provide shared
> > > memory management for exchanging tuning data between co-processor
> > > and the Pass 1 unit of the camera ISP system, including cache
> > > buffer handling.
> > >
> >
> > Looks like we haven't really progressed on getting this replaced with
> > something that doesn't require so much custom code. Let me propose something
> > better then.
> >
> > We already have a reserved memory mode in DT. If it has a compatible string
> > of "shared-dma-pool", it would be registered in the coherent DMA framework
> > [1]. That would make it available for consumer devices to look-up.
> >
> > Now if we add a "memory-region" property to the SCP device node and point it
> > to our reserved memory node, the SCP driver could look it up and hook to the
> > DMA mapping API using of_reserved_mem_device_init_by_idx[2].
> >
> > That basically makes any dma_alloc_*(), dma_map_*(), etc. calls on the SCP
> > struct device use the coherent DMA ops, which operate on the assigned memory
> > pool. With that, the P1 driver could just directly use those calls to
> > manage the memory, without any custom code.
> >
> > There is an example how this setup works in the s5p-mfc driver[3], but it
> > needs to be noted that it creates child nodes, because it can have more than
> > 1 DMA port, which may need its own memory pool. In our case, we wouldn't
> > need child nodes and could just use the SCP device directly.
> >
> > [1]
> > https://elixir.bootlin.com/linux/v5.2-rc7/source/kernel/dma/coherent.c#L345
> > [2]
> > https://elixir.bootlin.com/linux/v5.2-rc7/source/drivers/of/of_reserved_mem.c#L312
> > [3]
> > https://elixir.bootlin.com/linux/v5.2-rc7/source/drivers/media/platform/s5p-mfc/s5p_mfc.c#L1075
> >
> > Let me also post some specific comments below, in case we end up still
> > needing any of the code.
> >
>
> Thanks your suggestions.
>
> After applying your suggestion in SCP device driver, we could remove
> mtk_cam-smem.h/c. Currently, we use dma_alloc_coherent with SCP device
> to get SCP address. We could touch the buffer with this SCP address in
> SCP processor.
>
> After that, we use dma_map_page_attrs with P1 device which supports
> IOMMU domain to get IOVA address. For this address, we will assign
> it to our ISP HW device to proceed.
>
> Below is the snippet for ISP P1 compose buffer initialization.
>
> ptr = dma_alloc_coherent(p1_dev->cam_dev.smem_dev,
> MAX_COMPOSER_SIZE, &addr, GFP_KERNEL);
> if (!ptr) {
> dev_err(dev, "failed to allocate compose memory\n");
> return -ENOMEM;
> }
> isp_ctx->scp_mem_pa = addr;
addr contains a DMA address, not a physical address. Could we call it
scp_mem_dma instead?
> dev_dbg(dev, "scp addr:%pad\n", &addr);
>
> /* get iova address */
> addr = dma_map_page_attrs(dev, phys_to_page(addr), 0,
addr is a DMA address, so phys_to_page() can't be called on it. The
simplest thing here would be to use dma_map_single() with ptr as the
CPU address expected.
> MAX_COMPOSER_SIZE, DMA_BIDIRECTIONAL,
> DMA_ATTR_SKIP_CPU_SYNC);
> if (dma_mapping_error(dev, addr)) {
> isp_ctx->scp_mem_pa = 0;
We also need to free the allocated memory.
> dev_err(dev, "Failed to map scp iova\n");
> return -ENOMEM;
> }
> isp_ctx->scp_mem_iova = addr;
>
> Moreover, we have another meta input buffer usage.
> For this kind of buffer, it will be allocated by V4L2 framework
> with dma_alloc_coherent with SCP device. In order to get IOVA,
> we will add dma_map_page_attrs in vb2_ops' buf_init function.
> In buf_cleanup function, we will call dma_unmap_page_attrs function.
As per above, we don't have access to the struct page we want to map.
We probably want to get the CPU VA using vb2_plane_vaddr() and call
dma_map_single() instead.
>
> Based on these current implementation, do you think it is correct?
> If we got any wrong, please let us know.
>
> Btw, we also DMA_ATTR_NO_KERNEL_MAPPING DMA attribte to
> avoid dma_sync_sg_for_device. Othewise, it will hit the KE.
> Maybe we could not get the correct sg_table.
> Do you think it is a bug and need to fix?
I think DMA_ATTR_NO_KERNEL_MAPPING is good to have for all the buffers
that don't need to be accessed from the kernel anyway, to avoid
unnecessary kernel mapping operations. However, for coherent memory
pool, it doesn't change anything, because the memory always has a
kernel mapping. We also need the kernel virtual address for
dma_map_single(). Also the flag doesn't eliminate the need to do the
sync, e.g. if the userspace accesses the buffer.
Could y
