Re: [PATCH 3/3] videobuf2: replace a layering violation with dma_map_resource
Hi Marek, can chance you could retest the v2 version? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 3/3] videobuf2: replace a layering violation with dma_map_resource
vb2_dc_get_userptr pokes into arm direct mapping details to get the
resemblance of a dma address for a a physical address that does is
not backed by a page struct. Not only is this not portable to other
architectures with dma direct mapping offsets, but also not to uses
of IOMMUs of any kind. Switch to the proper dma_map_resource /
dma_unmap_resource interface instead.
Signed-off-by: Christoph Hellwig
Acked-by: Mauro Carvalho Chehab
---
.../common/videobuf2/videobuf2-dma-contig.c | 41 ---
1 file changed, 9 insertions(+), 32 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
index aff0ab7bf83d..82389aead6ed 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
@@ -439,42 +439,14 @@ static void vb2_dc_put_userptr(void *buf_priv)
set_page_dirty_lock(pages[i]);
sg_free_table(sgt);
kfree(sgt);
+ } else {
+ dma_unmap_resource(buf->dev, buf->dma_addr, buf->size,
+ buf->dma_dir, 0);
}
vb2_destroy_framevec(buf->vec);
kfree(buf);
}
-/*
- * For some kind of reserved memory there might be no struct page available,
- * so all that can be done to support such 'pages' is to try to convert
- * pfn to dma address or at the last resort just assume that
- * dma address == physical address (like it has been assumed in earlier version
- * of videobuf2-dma-contig
- */
-
-#ifdef __arch_pfn_to_dma
-static inline dma_addr_t vb2_dc_pfn_to_dma(struct device *dev, unsigned long
pfn)
-{
- return (dma_addr_t)__arch_pfn_to_dma(dev, pfn);
-}
-#elif defined(__pfn_to_bus)
-static inline dma_addr_t vb2_dc_pfn_to_dma(struct device *dev, unsigned long
pfn)
-{
- return (dma_addr_t)__pfn_to_bus(pfn);
-}
-#elif defined(__pfn_to_phys)
-static inline dma_addr_t vb2_dc_pfn_to_dma(struct device *dev, unsigned long
pfn)
-{
- return (dma_addr_t)__pfn_to_phys(pfn);
-}
-#else
-static inline dma_addr_t vb2_dc_pfn_to_dma(struct device *dev, unsigned long
pfn)
-{
- /* really, we cannot do anything better at this point */
- return (dma_addr_t)(pfn) << PAGE_SHIFT;
-}
-#endif
-
static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
unsigned long size, enum dma_data_direction dma_dir)
{
@@ -528,7 +500,12 @@ static void *vb2_dc_get_userptr(struct device *dev,
unsigned long vaddr,
for (i = 1; i < n_pages; i++)
if (nums[i-1] + 1 != nums[i])
goto fail_pfnvec;
- buf->dma_addr = vb2_dc_pfn_to_dma(buf->dev, nums[0]);
+ buf->dma_addr = dma_map_resource(buf->dev,
+ __pfn_to_phys(nums[0]), size, buf->dma_dir, 0);
+ if (dma_mapping_error(buf->dev, buf->dma_addr)) {
+ ret = -ENOMEM;
+ goto fail_pfnvec;
+ }
goto out;
}
--
2.20.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/3] videobuf2: replace a layering violation with dma_map_resource
Hi Christoph,
On 2019-01-17 18:21, Christoph Hellwig wrote:
> On Mon, Jan 14, 2019 at 01:42:26PM +0100, Marek Szyprowski wrote:
>> On 2019-01-11 19:17, Christoph Hellwig wrote:
>>> vb2_dc_get_userptr pokes into arm direct mapping details to get the
>>> resemblance of a dma address for a a physical address that does is
>>> not backed by a page struct. Not only is this not portable to other
>>> architectures with dma direct mapping offsets, but also not to uses
>>> of IOMMUs of any kind. Switch to the proper dma_map_resource /
>>> dma_unmap_resource interface instead.
>>>
>>> Signed-off-by: Christoph Hellwig
>> There are checks for IOMMU presence in other places in vb2-dma-contig,
>> so it was used only when no IOMMU is available, but I agree that the
>> hacky code should be replaced by a generic code if possible.
>>
>> Tested-by: Marek Szyprowski
>>
>> V4L2 pipeline works fine on older Exynos-based boards with CMA and IOMMU
>> disabled.
> Do you know if these rely on the offsets? E.g. would they still work
> with the patch below applied on top. That would keep the map_resource
> semantics as-is as solve the issue pointed out by Robin for now.
AFAIK that code was only used for sharing buffers between hardware
modules that are a part of the same SoC, usually implemented as platform
devices. AFAIR this never worked for devices on different buses. So far
I wasn't aware on ANY which would require an offset for the DMA access.
The first version of videobuf2-dc code even incorrectly used paddr
instead of dma_addr as a buffer 'address' returned to the client
drivers, because in case of those SoC this was exactly the same (see
commits 472af2b05bdefcaee7e754e22cbf131110017ad6 and
ba7fcb0c954921534707f08ebc4d8beeb2eb17e7).
> If not I can only think of a flag to bypass the offseting for now, but
> that would be pretty ugly. Or go for the long-term solution of
> discovering the relationship between the two devices, as done by the
> PCIe P2P code..
>
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 8e0359b04957..25bd19974223 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -359,7 +359,7 @@ EXPORT_SYMBOL(dma_direct_map_sg);
> dma_addr_t dma_direct_map_resource(struct device *dev, phys_addr_t paddr,
> size_t size, enum dma_data_direction dir, unsigned long attrs)
> {
> - dma_addr_t dma_addr = phys_to_dma(dev, paddr);
> + dma_addr_t dma_addr = paddr;
>
> if (unlikely(!dma_direct_possible(dev, dma_addr, size))) {
> report_addr(dev, dma_addr, size);
>
>
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/3] videobuf2: replace a layering violation with dma_map_resource
On Mon, Jan 14, 2019 at 01:42:26PM +0100, Marek Szyprowski wrote:
> Hi Christoph,
>
> On 2019-01-11 19:17, Christoph Hellwig wrote:
> > vb2_dc_get_userptr pokes into arm direct mapping details to get the
> > resemblance of a dma address for a a physical address that does is
> > not backed by a page struct. Not only is this not portable to other
> > architectures with dma direct mapping offsets, but also not to uses
> > of IOMMUs of any kind. Switch to the proper dma_map_resource /
> > dma_unmap_resource interface instead.
> >
> > Signed-off-by: Christoph Hellwig
>
> There are checks for IOMMU presence in other places in vb2-dma-contig,
> so it was used only when no IOMMU is available, but I agree that the
> hacky code should be replaced by a generic code if possible.
>
> Tested-by: Marek Szyprowski
>
> V4L2 pipeline works fine on older Exynos-based boards with CMA and IOMMU
> disabled.
Do you know if these rely on the offsets? E.g. would they still work
with the patch below applied on top. That would keep the map_resource
semantics as-is as solve the issue pointed out by Robin for now.
If not I can only think of a flag to bypass the offseting for now, but
that would be pretty ugly. Or go for the long-term solution of
discovering the relationship between the two devices, as done by the
PCIe P2P code..
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 8e0359b04957..25bd19974223 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -359,7 +359,7 @@ EXPORT_SYMBOL(dma_direct_map_sg);
dma_addr_t dma_direct_map_resource(struct device *dev, phys_addr_t paddr,
size_t size, enum dma_data_direction dir, unsigned long attrs)
{
- dma_addr_t dma_addr = phys_to_dma(dev, paddr);
+ dma_addr_t dma_addr = paddr;
if (unlikely(!dma_direct_possible(dev, dma_addr, size))) {
report_addr(dev, dma_addr, size);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/3] videobuf2: replace a layering violation with dma_map_resource
Hi Christoph,
On 2019-01-11 19:17, Christoph Hellwig wrote:
> vb2_dc_get_userptr pokes into arm direct mapping details to get the
> resemblance of a dma address for a a physical address that does is
> not backed by a page struct. Not only is this not portable to other
> architectures with dma direct mapping offsets, but also not to uses
> of IOMMUs of any kind. Switch to the proper dma_map_resource /
> dma_unmap_resource interface instead.
>
> Signed-off-by: Christoph Hellwig
There are checks for IOMMU presence in other places in vb2-dma-contig,
so it was used only when no IOMMU is available, but I agree that the
hacky code should be replaced by a generic code if possible.
Tested-by: Marek Szyprowski
V4L2 pipeline works fine on older Exynos-based boards with CMA and IOMMU
disabled.
> ---
> .../common/videobuf2/videobuf2-dma-contig.c | 41 ---
> 1 file changed, 9 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> index aff0ab7bf83d..82389aead6ed 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> @@ -439,42 +439,14 @@ static void vb2_dc_put_userptr(void *buf_priv)
> set_page_dirty_lock(pages[i]);
> sg_free_table(sgt);
> kfree(sgt);
> + } else {
> + dma_unmap_resource(buf->dev, buf->dma_addr, buf->size,
> +buf->dma_dir, 0);
> }
> vb2_destroy_framevec(buf->vec);
> kfree(buf);
> }
>
> -/*
> - * For some kind of reserved memory there might be no struct page available,
> - * so all that can be done to support such 'pages' is to try to convert
> - * pfn to dma address or at the last resort just assume that
> - * dma address == physical address (like it has been assumed in earlier
> version
> - * of videobuf2-dma-contig
> - */
> -
> -#ifdef __arch_pfn_to_dma
> -static inline dma_addr_t vb2_dc_pfn_to_dma(struct device *dev, unsigned long
> pfn)
> -{
> - return (dma_addr_t)__arch_pfn_to_dma(dev, pfn);
> -}
> -#elif defined(__pfn_to_bus)
> -static inline dma_addr_t vb2_dc_pfn_to_dma(struct device *dev, unsigned long
> pfn)
> -{
> - return (dma_addr_t)__pfn_to_bus(pfn);
> -}
> -#elif defined(__pfn_to_phys)
> -static inline dma_addr_t vb2_dc_pfn_to_dma(struct device *dev, unsigned long
> pfn)
> -{
> - return (dma_addr_t)__pfn_to_phys(pfn);
> -}
> -#else
> -static inline dma_addr_t vb2_dc_pfn_to_dma(struct device *dev, unsigned long
> pfn)
> -{
> - /* really, we cannot do anything better at this point */
> - return (dma_addr_t)(pfn) << PAGE_SHIFT;
> -}
> -#endif
> -
> static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
> unsigned long size, enum dma_data_direction dma_dir)
> {
> @@ -528,7 +500,12 @@ static void *vb2_dc_get_userptr(struct device *dev,
> unsigned long vaddr,
> for (i = 1; i < n_pages; i++)
> if (nums[i-1] + 1 != nums[i])
> goto fail_pfnvec;
> - buf->dma_addr = vb2_dc_pfn_to_dma(buf->dev, nums[0]);
> + buf->dma_addr = dma_map_resource(buf->dev,
> + __pfn_to_phys(nums[0]), size, buf->dma_dir, 0);
> + if (dma_mapping_error(buf->dev, buf->dma_addr)) {
> + ret = -ENOMEM;
> + goto fail_pfnvec;
> + }
> goto out;
> }
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/3] videobuf2: replace a layering violation with dma_map_resource
On Mon, Jan 14, 2019 at 09:04:56AM -0200, Mauro Carvalho Chehab wrote: > It would be good if you could later send us a stable branch where > you merged, in order to allow us to run some tests with the new > DMA mapping patchset and be sure that it won't cause regressions > to videobuf2. I can do that, but the series as sent should be testable, results welcome! ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/3] videobuf2: replace a layering violation with dma_map_resource
Em Mon, 14 Jan 2019 11:31:39 +0100 Christoph Hellwig escreveu: > On Fri, Jan 11, 2019 at 05:54:16PM -0200, Mauro Carvalho Chehab wrote: > > Em Fri, 11 Jan 2019 19:17:31 +0100 > > Christoph Hellwig escreveu: > > > > > vb2_dc_get_userptr pokes into arm direct mapping details to get the > > > resemblance of a dma address for a a physical address that does is > > > not backed by a page struct. Not only is this not portable to other > > > architectures with dma direct mapping offsets, but also not to uses > > > of IOMMUs of any kind. Switch to the proper dma_map_resource / > > > dma_unmap_resource interface instead. > > > > Makes sense to me. I'm assuming that you'll be pushing it together > > with other mm patches, so: > > Not really mm, but rather DMA mapping, but yes, I'd love to take it > all together. Ah, OK! Anyway, feel free to place it altogether. It would be good if you could later send us a stable branch where you merged, in order to allow us to run some tests with the new DMA mapping patchset and be sure that it won't cause regressions to videobuf2. Thank you! Mauro ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/3] videobuf2: replace a layering violation with dma_map_resource
On Fri, Jan 11, 2019 at 05:54:16PM -0200, Mauro Carvalho Chehab wrote: > Em Fri, 11 Jan 2019 19:17:31 +0100 > Christoph Hellwig escreveu: > > > vb2_dc_get_userptr pokes into arm direct mapping details to get the > > resemblance of a dma address for a a physical address that does is > > not backed by a page struct. Not only is this not portable to other > > architectures with dma direct mapping offsets, but also not to uses > > of IOMMUs of any kind. Switch to the proper dma_map_resource / > > dma_unmap_resource interface instead. > > Makes sense to me. I'm assuming that you'll be pushing it together > with other mm patches, so: Not really mm, but rather DMA mapping, but yes, I'd love to take it all together. Thanks! ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/3] videobuf2: replace a layering violation with dma_map_resource
Em Fri, 11 Jan 2019 19:17:31 +0100
Christoph Hellwig escreveu:
> vb2_dc_get_userptr pokes into arm direct mapping details to get the
> resemblance of a dma address for a a physical address that does is
> not backed by a page struct. Not only is this not portable to other
> architectures with dma direct mapping offsets, but also not to uses
> of IOMMUs of any kind. Switch to the proper dma_map_resource /
> dma_unmap_resource interface instead.
Makes sense to me. I'm assuming that you'll be pushing it together
with other mm patches, so:
Acked-by: Mauro Carvalho Chehab
>
> Signed-off-by: Christoph Hellwig
> ---
> .../common/videobuf2/videobuf2-dma-contig.c | 41 ---
> 1 file changed, 9 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> index aff0ab7bf83d..82389aead6ed 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> @@ -439,42 +439,14 @@ static void vb2_dc_put_userptr(void *buf_priv)
> set_page_dirty_lock(pages[i]);
> sg_free_table(sgt);
> kfree(sgt);
> + } else {
> + dma_unmap_resource(buf->dev, buf->dma_addr, buf->size,
> +buf->dma_dir, 0);
> }
> vb2_destroy_framevec(buf->vec);
> kfree(buf);
> }
>
> -/*
> - * For some kind of reserved memory there might be no struct page available,
> - * so all that can be done to support such 'pages' is to try to convert
> - * pfn to dma address or at the last resort just assume that
> - * dma address == physical address (like it has been assumed in earlier
> version
> - * of videobuf2-dma-contig
> - */
> -
> -#ifdef __arch_pfn_to_dma
> -static inline dma_addr_t vb2_dc_pfn_to_dma(struct device *dev, unsigned long
> pfn)
> -{
> - return (dma_addr_t)__arch_pfn_to_dma(dev, pfn);
> -}
> -#elif defined(__pfn_to_bus)
> -static inline dma_addr_t vb2_dc_pfn_to_dma(struct device *dev, unsigned long
> pfn)
> -{
> - return (dma_addr_t)__pfn_to_bus(pfn);
> -}
> -#elif defined(__pfn_to_phys)
> -static inline dma_addr_t vb2_dc_pfn_to_dma(struct device *dev, unsigned long
> pfn)
> -{
> - return (dma_addr_t)__pfn_to_phys(pfn);
> -}
> -#else
> -static inline dma_addr_t vb2_dc_pfn_to_dma(struct device *dev, unsigned long
> pfn)
> -{
> - /* really, we cannot do anything better at this point */
> - return (dma_addr_t)(pfn) << PAGE_SHIFT;
> -}
> -#endif
> -
> static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
> unsigned long size, enum dma_data_direction dma_dir)
> {
> @@ -528,7 +500,12 @@ static void *vb2_dc_get_userptr(struct device *dev,
> unsigned long vaddr,
> for (i = 1; i < n_pages; i++)
> if (nums[i-1] + 1 != nums[i])
> goto fail_pfnvec;
> - buf->dma_addr = vb2_dc_pfn_to_dma(buf->dev, nums[0]);
> + buf->dma_addr = dma_map_resource(buf->dev,
> + __pfn_to_phys(nums[0]), size, buf->dma_dir, 0);
> + if (dma_mapping_error(buf->dev, buf->dma_addr)) {
> + ret = -ENOMEM;
> + goto fail_pfnvec;
> + }
> goto out;
> }
>
Thanks,
Mauro
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 3/3] videobuf2: replace a layering violation with dma_map_resource
vb2_dc_get_userptr pokes into arm direct mapping details to get the
resemblance of a dma address for a a physical address that does is
not backed by a page struct. Not only is this not portable to other
architectures with dma direct mapping offsets, but also not to uses
of IOMMUs of any kind. Switch to the proper dma_map_resource /
dma_unmap_resource interface instead.
Signed-off-by: Christoph Hellwig
---
.../common/videobuf2/videobuf2-dma-contig.c | 41 ---
1 file changed, 9 insertions(+), 32 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
index aff0ab7bf83d..82389aead6ed 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
@@ -439,42 +439,14 @@ static void vb2_dc_put_userptr(void *buf_priv)
set_page_dirty_lock(pages[i]);
sg_free_table(sgt);
kfree(sgt);
+ } else {
+ dma_unmap_resource(buf->dev, buf->dma_addr, buf->size,
+ buf->dma_dir, 0);
}
vb2_destroy_framevec(buf->vec);
kfree(buf);
}
-/*
- * For some kind of reserved memory there might be no struct page available,
- * so all that can be done to support such 'pages' is to try to convert
- * pfn to dma address or at the last resort just assume that
- * dma address == physical address (like it has been assumed in earlier version
- * of videobuf2-dma-contig
- */
-
-#ifdef __arch_pfn_to_dma
-static inline dma_addr_t vb2_dc_pfn_to_dma(struct device *dev, unsigned long
pfn)
-{
- return (dma_addr_t)__arch_pfn_to_dma(dev, pfn);
-}
-#elif defined(__pfn_to_bus)
-static inline dma_addr_t vb2_dc_pfn_to_dma(struct device *dev, unsigned long
pfn)
-{
- return (dma_addr_t)__pfn_to_bus(pfn);
-}
-#elif defined(__pfn_to_phys)
-static inline dma_addr_t vb2_dc_pfn_to_dma(struct device *dev, unsigned long
pfn)
-{
- return (dma_addr_t)__pfn_to_phys(pfn);
-}
-#else
-static inline dma_addr_t vb2_dc_pfn_to_dma(struct device *dev, unsigned long
pfn)
-{
- /* really, we cannot do anything better at this point */
- return (dma_addr_t)(pfn) << PAGE_SHIFT;
-}
-#endif
-
static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
unsigned long size, enum dma_data_direction dma_dir)
{
@@ -528,7 +500,12 @@ static void *vb2_dc_get_userptr(struct device *dev,
unsigned long vaddr,
for (i = 1; i < n_pages; i++)
if (nums[i-1] + 1 != nums[i])
goto fail_pfnvec;
- buf->dma_addr = vb2_dc_pfn_to_dma(buf->dev, nums[0]);
+ buf->dma_addr = dma_map_resource(buf->dev,
+ __pfn_to_phys(nums[0]), size, buf->dma_dir, 0);
+ if (dma_mapping_error(buf->dev, buf->dma_addr)) {
+ ret = -ENOMEM;
+ goto fail_pfnvec;
+ }
goto out;
}
--
2.20.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
