Re: DMA direct mapping fix for 5.4 and earlier stable branches
On Tue, Feb 09, 2021 at 12:45:11PM +, Robin Murphy wrote: > It's not a bug, it's a fundamental design failure. dma_get_sgtable() has > only ever sort-of-worked for DMA buffers that come from CMA or regular page > allocations. In particular, a "no-map" DMA pool is not backed by kernel > memory, so does not have any corresponding page structs, so it's impossible > to generate a *valid* scatterlist to represent memory from that pool, > regardless of what you might get away with provided you don't poke too hard > at it. > > It is not a good API... Yes, I don't think anyone should add new users of the API. That being said the commit he is trying to backport fixes a bug in the implementation that at least in theory could also affect in-tree drivers.
Re: DMA direct mapping fix for 5.4 and earlier stable branches
On 2021-02-09 12:36, Sumit Garg wrote:
Hi Christoph,
On Tue, 9 Feb 2021 at 15:06, Christoph Hellwig wrote:
On Tue, Feb 09, 2021 at 10:23:12AM +0100, Greg KH wrote:
From the view point of ZeroCopy using DMABUF, is 5.4 not
mature enough, and is 5.10 enough mature ?
This is the most important point for judging migration.
How do you judge "mature"?
And again, if a feature isn't present in a specific kernel version, why
would you think that it would be a viable solution for you to use?
I'm pretty sure dma_get_sgtable has been around much longer and was
supposed to work, but only really did work properly for arm32, and
for platforms with coherent DMA. I bet he is using non-coherent arm64,
It's an arm64 platform using coherent DMA where device coherent DMA
memory pool is defined in the DT as follows:
reserved-memory {
#address-cells = <2>;
#size-cells = <2>;
ranges;
encbuffer: encbuffer@0xb000 {
compatible = "shared-dma-pool";
reg = <0 0xb000 0 0x0800>; // this
area used with dma-coherent
no-map;
};
};
Device is dma-coherent as per following DT property:
codec {
compatible = "socionext,uniphier-pxs3-codec";
memory-region = <>;
dma-coherent;
};
And call chain to create device coherent DMA pool is as follows:
rmem_dma_device_init();
dma_init_coherent_memory();
memremap();
ioremap_wc();
which simply maps coherent DMA memory into vmalloc space on arm64.
The thing I am unclear is why this is called a new feature rather than
a bug in dma_common_get_sgtable() which is failing to handle vmalloc
addresses? While at the same time DMA debug APIs specifically handle
vmalloc addresses [1].
It's not a bug, it's a fundamental design failure. dma_get_sgtable() has
only ever sort-of-worked for DMA buffers that come from CMA or regular
page allocations. In particular, a "no-map" DMA pool is not backed by
kernel memory, so does not have any corresponding page structs, so it's
impossible to generate a *valid* scatterlist to represent memory from
that pool, regardless of what you might get away with provided you don't
poke too hard at it.
It is not a good API...
Robin.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/kernel/dma/debug.c?h=linux-5.4.y#n1462
-Sumit
and it would be broken for other drivers there as well if people did
test them, which they apparently so far did not.
Re: DMA direct mapping fix for 5.4 and earlier stable branches
Hi Christoph,
On Tue, 9 Feb 2021 at 15:06, Christoph Hellwig wrote:
>
> On Tue, Feb 09, 2021 at 10:23:12AM +0100, Greg KH wrote:
> > > From the view point of ZeroCopy using DMABUF, is 5.4 not
> > > mature enough, and is 5.10 enough mature ?
> > > This is the most important point for judging migration.
> >
> > How do you judge "mature"?
> >
> > And again, if a feature isn't present in a specific kernel version, why
> > would you think that it would be a viable solution for you to use?
>
> I'm pretty sure dma_get_sgtable has been around much longer and was
> supposed to work, but only really did work properly for arm32, and
> for platforms with coherent DMA. I bet he is using non-coherent arm64,
It's an arm64 platform using coherent DMA where device coherent DMA
memory pool is defined in the DT as follows:
reserved-memory {
#address-cells = <2>;
#size-cells = <2>;
ranges;
encbuffer: encbuffer@0xb000 {
compatible = "shared-dma-pool";
reg = <0 0xb000 0 0x0800>; // this
area used with dma-coherent
no-map;
};
};
Device is dma-coherent as per following DT property:
codec {
compatible = "socionext,uniphier-pxs3-codec";
memory-region = <>;
dma-coherent;
};
And call chain to create device coherent DMA pool is as follows:
rmem_dma_device_init();
dma_init_coherent_memory();
memremap();
ioremap_wc();
which simply maps coherent DMA memory into vmalloc space on arm64.
The thing I am unclear is why this is called a new feature rather than
a bug in dma_common_get_sgtable() which is failing to handle vmalloc
addresses? While at the same time DMA debug APIs specifically handle
vmalloc addresses [1].
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/kernel/dma/debug.c?h=linux-5.4.y#n1462
-Sumit
> and it would be broken for other drivers there as well if people did
> test them, which they apparently so far did not.
Re: DMA direct mapping fix for 5.4 and earlier stable branches
On Tue, Feb 09, 2021 at 10:19:07AM +, obayashi.yoshim...@socionext.com wrote: > > How do you judge "mature"? > > My basic criteria are > * Function is exist, but bug fix is necessary: "mature" > * Function extension is necessary: "immature" > > > And again, if a feature isn't present in a specific kernel version, why > > would you think that it would be > > a viable solution for you to use? > > So, "a feature isn't present in a specific kernel version", > also means "immature" according to my criteria. Great, please use the 5.10 or newer kernel release then. thanks, greg k-h
RE: DMA direct mapping fix for 5.4 and earlier stable branches
> How do you judge "mature"? My basic criteria are * Function is exist, but bug fix is necessary: "mature" * Function extension is necessary: "immature" > And again, if a feature isn't present in a specific kernel version, why would > you think that it would be > a viable solution for you to use? So, "a feature isn't present in a specific kernel version", also means "immature" according to my criteria. Regards. > -Original Message- > From: Greg KH > Sent: Tuesday, February 9, 2021 6:23 PM > To: Obayashi, Yoshimasa/尾林 善正 > Cc: sumit.g...@linaro.org; h...@lst.de; m.szyprow...@samsung.com; > robin.mur...@arm.com; > io...@lists.linux-foundation.org; linux-kernel@vger.kernel.org; > sta...@vger.kernel.org; > daniel.thomp...@linaro.org > Subject: Re: DMA direct mapping fix for 5.4 and earlier stable branches > > On Tue, Feb 09, 2021 at 09:05:40AM +, obayashi.yoshim...@socionext.com > wrote: > > > > As the drivers are currently under development and Socionext has > > > > chosen 5.4 stable kernel for their development. So I will let > > > > Obayashi-san answer this if it's possible for them to migrate to > > > > 5.10 instead? > > > > We have started this development project from last August, so we > > have selected 5.4 as most recent and longest lifetime LTS version at > > that time. > > > > And we have already finished to develop other device drivers, and > > Video converter and CODEC drivers are now in development. > > > > > Why pick a kernel that doesn not support the features they require? > > > That seems very odd and unwise. > > > > From the view point of ZeroCopy using DMABUF, is 5.4 not mature > > enough, and is 5.10 enough mature ? > > This is the most important point for judging migration. > > How do you judge "mature"? > > And again, if a feature isn't present in a specific kernel version, why would > you think that it would be > a viable solution for you to use? > > good luck! > > greg k-h
Re: DMA direct mapping fix for 5.4 and earlier stable branches
On Tue, Feb 09, 2021 at 10:23:12AM +0100, Greg KH wrote: > > From the view point of ZeroCopy using DMABUF, is 5.4 not > > mature enough, and is 5.10 enough mature ? > > This is the most important point for judging migration. > > How do you judge "mature"? > > And again, if a feature isn't present in a specific kernel version, why > would you think that it would be a viable solution for you to use? I'm pretty sure dma_get_sgtable has been around much longer and was supposed to work, but only really did work properly for arm32, and for platforms with coherent DMA. I bet he is using non-coherent arm64, and it would be broken for other drivers there as well if people did test them, which they apparently so far did not.
Re: DMA direct mapping fix for 5.4 and earlier stable branches
On Tue, Feb 09, 2021 at 09:05:40AM +, obayashi.yoshim...@socionext.com wrote: > > > As the drivers are currently under development and Socionext has > > > chosen 5.4 stable kernel for their development. So I will let > > > Obayashi-san answer this if it's possible for them to migrate to 5.10 > > > instead? > > We have started this development project from last August, > so we have selected 5.4 as most recent and longest lifetime LTS > version at that time. > > And we have already finished to develop other device drivers, > and Video converter and CODEC drivers are now in development. > > > Why pick a kernel that doesn not support the features they require? > > That seems very odd and unwise. > > From the view point of ZeroCopy using DMABUF, is 5.4 not > mature enough, and is 5.10 enough mature ? > This is the most important point for judging migration. How do you judge "mature"? And again, if a feature isn't present in a specific kernel version, why would you think that it would be a viable solution for you to use? good luck! greg k-h
RE: DMA direct mapping fix for 5.4 and earlier stable branches
> > As the drivers are currently under development and Socionext has > > chosen 5.4 stable kernel for their development. So I will let > > Obayashi-san answer this if it's possible for them to migrate to 5.10 > > instead? We have started this development project from last August, so we have selected 5.4 as most recent and longest lifetime LTS version at that time. And we have already finished to develop other device drivers, and Video converter and CODEC drivers are now in development. > Why pick a kernel that doesn not support the features they require? > That seems very odd and unwise. From the view point of ZeroCopy using DMABUF, is 5.4 not mature enough, and is 5.10 enough mature ? This is the most important point for judging migration. Regards. > -Original Message- > From: Greg Kroah-Hartman > Sent: Tuesday, February 9, 2021 5:04 PM > To: Sumit Garg > Cc: Obayashi, Yoshimasa/尾林 善正 ; h...@lst.de; > m.szyprow...@samsung.com; robin.mur...@arm.com; > io...@lists.linux-foundation.org; Linux Kernel Mailing > List ; stable ; Daniel > Thompson > > Subject: Re: DMA direct mapping fix for 5.4 and earlier stable branches > > On Tue, Feb 09, 2021 at 01:28:47PM +0530, Sumit Garg wrote: > > Thanks Greg for your response. > > > > On Tue, 9 Feb 2021 at 12:28, Greg Kroah-Hartman > > wrote: > > > > > > On Tue, Feb 09, 2021 at 11:39:25AM +0530, Sumit Garg wrote: > > > > Hi Christoph, Greg, > > > > > > > > Currently we are observing an incorrect address translation > > > > corresponding to DMA direct mapping methods on 5.4 stable kernel > > > > while sharing dmabuf from one device to another where both devices > > > > have their own coherent DMA memory pools. > > > > > > What devices have this problem? > > > > The problem is seen with V4L2 device drivers which are currently under > > development for UniPhier PXs3 Reference Board from Socionext [1]. > > Ok, so it's not even a driver in the 5.4 kernel today, so there's nothing I > can do here as there is no regression > of the existing source tree. > > > Following is brief description of the test framework: > > > > The issue is observed while trying to construct a Gstreamer pipeline > > leveraging hardware video converter engine (VPE device) and hardware > > video encode/decode engine (CODEC device) where we use dmabuf > > framework for Zero-Copy. > > > > Example GStreamer pipeline is: > > gst-launch-1.0 -v -e videotestsrc \ > > > ! video/x-raw, width=480, height=270, format=NV15 \ ! v4l2convert > > > device=/dev/vpe0 capture-io-mode=dmabuf-import \ ! video/x-raw, > > > width=480, height=270, format=NV12 \ ! v4l2h265enc > > > device=/dev/codec0 output-io-mode=dmabuf \ ! video/x-h265, > > > format=byte-stream, width=480, height=270 \ ! filesink > > > location=out.hevc > > > > Using GStreamer's V4L2 plugin, > > - v4l2convert controls VPE driver, > > - v4l2h265enc controls CODEC driver. > > > > In the above pipeline, VPE driver imports CODEC driver's DMABUF for > > Zero-Copy. > > > > [1] arch/arm64/boot/dts/socionext/uniphier-pxs3-ref.dts > > > > > And why can't then just use 5.10 to > > > solve this issue as that problem has always been present for them, > > > right? > > > > As the drivers are currently under development and Socionext has > > chosen 5.4 stable kernel for their development. So I will let > > Obayashi-san answer this if it's possible for them to migrate to 5.10 > > instead? > > Why pick a kernel that doesn not support the features they require? > That seems very odd and unwise. > > > BTW, this problem belongs to the common code, so others may experience > > this issue as well. > > Then they should move to 5.10 or newer as this just doesn't work on older > kernels, right? > > > > > I am able to root cause this issue which is caused by incorrect > > > > virt to phys translation for addresses belonging to vmalloc space > > > > using virt_to_page(). But while looking at the mainline kernel, > > > > this patch [1] changes address translation from virt->to->phys to > > > > dma->to->phys which fixes the issue observed on 5.4 stable kernel > > > > as well (minimal fix [2]). > > > > > > > > So I would like to seek your suggestion for backport to stable > > > > kernels > > > > (5.4 or earlier) as to whether we should backport the complete > > > > mainline commit [1] or we shoul
Re: DMA direct mapping fix for 5.4 and earlier stable branches
On Tue, Feb 09, 2021 at 01:28:47PM +0530, Sumit Garg wrote: > Thanks Greg for your response. > > On Tue, 9 Feb 2021 at 12:28, Greg Kroah-Hartman > wrote: > > > > On Tue, Feb 09, 2021 at 11:39:25AM +0530, Sumit Garg wrote: > > > Hi Christoph, Greg, > > > > > > Currently we are observing an incorrect address translation > > > corresponding to DMA direct mapping methods on 5.4 stable kernel while > > > sharing dmabuf from one device to another where both devices have > > > their own coherent DMA memory pools. > > > > What devices have this problem? > > The problem is seen with V4L2 device drivers which are currently under > development for UniPhier PXs3 Reference Board from Socionext [1]. Ok, so it's not even a driver in the 5.4 kernel today, so there's nothing I can do here as there is no regression of the existing source tree. > Following is brief description of the test framework: > > The issue is observed while trying to construct a Gstreamer pipeline > leveraging hardware video converter engine (VPE device) and hardware > video encode/decode engine (CODEC device) where we use dmabuf > framework for Zero-Copy. > > Example GStreamer pipeline is: > gst-launch-1.0 -v -e videotestsrc \ > > ! video/x-raw, width=480, height=270, format=NV15 \ > > ! v4l2convert device=/dev/vpe0 capture-io-mode=dmabuf-import \ > > ! video/x-raw, width=480, height=270, format=NV12 \ > > ! v4l2h265enc device=/dev/codec0 output-io-mode=dmabuf \ > > ! video/x-h265, format=byte-stream, width=480, height=270 \ > > ! filesink location=out.hevc > > Using GStreamer's V4L2 plugin, > - v4l2convert controls VPE driver, > - v4l2h265enc controls CODEC driver. > > In the above pipeline, VPE driver imports CODEC driver's DMABUF for Zero-Copy. > > [1] arch/arm64/boot/dts/socionext/uniphier-pxs3-ref.dts > > > And why can't then just use 5.10 to > > solve this issue as that problem has always been present for them, > > right? > > As the drivers are currently under development and Socionext has > chosen 5.4 stable kernel for their development. So I will let > Obayashi-san answer this if it's possible for them to migrate to 5.10 > instead? Why pick a kernel that doesn not support the features they require? That seems very odd and unwise. > BTW, this problem belongs to the common code, so others may experience > this issue as well. Then they should move to 5.10 or newer as this just doesn't work on older kernels, right? > > > I am able to root cause this issue which is caused by incorrect virt > > > to phys translation for addresses belonging to vmalloc space using > > > virt_to_page(). But while looking at the mainline kernel, this patch > > > [1] changes address translation from virt->to->phys to dma->to->phys > > > which fixes the issue observed on 5.4 stable kernel as well (minimal > > > fix [2]). > > > > > > So I would like to seek your suggestion for backport to stable kernels > > > (5.4 or earlier) as to whether we should backport the complete > > > mainline commit [1] or we should just apply the minimal fix [2]? > > > > Whenever you try to create a "minimal" fix, 90% of the time it is wrong > > and does not work and I end up having to deal with the mess. > > I agree with your concerns for having to apply a non-mainline commit > onto a stable kernel. > > > What > > prevents you from doing the real thing here? Are the patches to big? > > > > IMHO, yes the mainline patch is big enough to touch multiple > architectures. But if that's the only way preferred then I can > backport the mainline patch instead. > > > And again, why not just use 5.10 for this hardware? What hardware is > > it? > > > > Please see my response above. If a feature in the kernel was not present on older kernels, trying to shoe-horn it into them is not wise at all. You pick a kernel version to reflect the features/options that you require, and it sounds like 5.4 just will not work for them, so to stick with that would be quite foolish. Just move to 5.10, much simpler! thanks, greg k-h
Re: DMA direct mapping fix for 5.4 and earlier stable branches
Thanks Greg for your response. On Tue, 9 Feb 2021 at 12:28, Greg Kroah-Hartman wrote: > > On Tue, Feb 09, 2021 at 11:39:25AM +0530, Sumit Garg wrote: > > Hi Christoph, Greg, > > > > Currently we are observing an incorrect address translation > > corresponding to DMA direct mapping methods on 5.4 stable kernel while > > sharing dmabuf from one device to another where both devices have > > their own coherent DMA memory pools. > > What devices have this problem? The problem is seen with V4L2 device drivers which are currently under development for UniPhier PXs3 Reference Board from Socionext [1]. Following is brief description of the test framework: The issue is observed while trying to construct a Gstreamer pipeline leveraging hardware video converter engine (VPE device) and hardware video encode/decode engine (CODEC device) where we use dmabuf framework for Zero-Copy. Example GStreamer pipeline is: gst-launch-1.0 -v -e videotestsrc \ > ! video/x-raw, width=480, height=270, format=NV15 \ > ! v4l2convert device=/dev/vpe0 capture-io-mode=dmabuf-import \ > ! video/x-raw, width=480, height=270, format=NV12 \ > ! v4l2h265enc device=/dev/codec0 output-io-mode=dmabuf \ > ! video/x-h265, format=byte-stream, width=480, height=270 \ > ! filesink location=out.hevc Using GStreamer's V4L2 plugin, - v4l2convert controls VPE driver, - v4l2h265enc controls CODEC driver. In the above pipeline, VPE driver imports CODEC driver's DMABUF for Zero-Copy. [1] arch/arm64/boot/dts/socionext/uniphier-pxs3-ref.dts > And why can't then just use 5.10 to > solve this issue as that problem has always been present for them, > right? As the drivers are currently under development and Socionext has chosen 5.4 stable kernel for their development. So I will let Obayashi-san answer this if it's possible for them to migrate to 5.10 instead? BTW, this problem belongs to the common code, so others may experience this issue as well. > > > I am able to root cause this issue which is caused by incorrect virt > > to phys translation for addresses belonging to vmalloc space using > > virt_to_page(). But while looking at the mainline kernel, this patch > > [1] changes address translation from virt->to->phys to dma->to->phys > > which fixes the issue observed on 5.4 stable kernel as well (minimal > > fix [2]). > > > > So I would like to seek your suggestion for backport to stable kernels > > (5.4 or earlier) as to whether we should backport the complete > > mainline commit [1] or we should just apply the minimal fix [2]? > > Whenever you try to create a "minimal" fix, 90% of the time it is wrong > and does not work and I end up having to deal with the mess. I agree with your concerns for having to apply a non-mainline commit onto a stable kernel. > What > prevents you from doing the real thing here? Are the patches to big? > IMHO, yes the mainline patch is big enough to touch multiple architectures. But if that's the only way preferred then I can backport the mainline patch instead. > And again, why not just use 5.10 for this hardware? What hardware is > it? > Please see my response above. -Sumit > thanks, > > greg k-h
Re: DMA direct mapping fix for 5.4 and earlier stable branches
On Tue, Feb 09, 2021 at 11:39:25AM +0530, Sumit Garg wrote: > Hi Christoph, Greg, > > Currently we are observing an incorrect address translation > corresponding to DMA direct mapping methods on 5.4 stable kernel while > sharing dmabuf from one device to another where both devices have > their own coherent DMA memory pools. What devices have this problem? And why can't then just use 5.10 to solve this issue as that problem has always been present for them, right? > I am able to root cause this issue which is caused by incorrect virt > to phys translation for addresses belonging to vmalloc space using > virt_to_page(). But while looking at the mainline kernel, this patch > [1] changes address translation from virt->to->phys to dma->to->phys > which fixes the issue observed on 5.4 stable kernel as well (minimal > fix [2]). > > So I would like to seek your suggestion for backport to stable kernels > (5.4 or earlier) as to whether we should backport the complete > mainline commit [1] or we should just apply the minimal fix [2]? Whenever you try to create a "minimal" fix, 90% of the time it is wrong and does not work and I end up having to deal with the mess. What prevents you from doing the real thing here? Are the patches to big? And again, why not just use 5.10 for this hardware? What hardware is it? thanks, greg k-h
DMA direct mapping fix for 5.4 and earlier stable branches
Hi Christoph, Greg,
Currently we are observing an incorrect address translation
corresponding to DMA direct mapping methods on 5.4 stable kernel while
sharing dmabuf from one device to another where both devices have
their own coherent DMA memory pools.
I am able to root cause this issue which is caused by incorrect virt
to phys translation for addresses belonging to vmalloc space using
virt_to_page(). But while looking at the mainline kernel, this patch
[1] changes address translation from virt->to->phys to dma->to->phys
which fixes the issue observed on 5.4 stable kernel as well (minimal
fix [2]).
So I would like to seek your suggestion for backport to stable kernels
(5.4 or earlier) as to whether we should backport the complete
mainline commit [1] or we should just apply the minimal fix [2]?
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=34dc0ea6bc960f1f57b2148f01a3f4da23f87013
[2] minimal fix required for 5.4 stable kernel:
commit bb0b3ff6e54d78370b6b0c04426f0d9192f31795
Author: Sumit Garg
Date: Wed Feb 3 13:08:37 2021 +0530
dma-mapping: Fix common get_sgtable and mmap methods
Currently common get_sgtable and mmap methods can only handle normal
kernel addresses leading to incorrect handling of vmalloc addresses which
is common means for DMA coherent memory mapping.
So instead of cpu_addr, directly decode the physical address from
dma_addr and
hence decode corresponding page and pfn values. In this way we can handle
normal kernel addresses as well as vmalloc addresses.
This fix is inspired from following mainline commit:
34dc0ea6bc96 ("dma-direct: provide mmap and get_sgtable method overrides")
This fixes an issue observed during dmabuf sharing from one device to
another where both devices have their own coherent DMA memory pools.
Signed-off-by: Sumit Garg
diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
index 8682a53..034bbae 100644
--- a/kernel/dma/mapping.c
+++ b/kernel/dma/mapping.c
@@ -127,7 +127,7 @@ int dma_common_get_sgtable(struct device *dev,
struct sg_table *sgt,
return -ENXIO;
page = pfn_to_page(pfn);
} else {
- page = virt_to_page(cpu_addr);
+ page = pfn_to_page(PHYS_PFN(dma_to_phys(dev, dma_addr)));
}
ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
@@ -214,7 +214,7 @@ int dma_common_mmap(struct device *dev, struct
vm_area_struct *vma,
if (!pfn_valid(pfn))
return -ENXIO;
} else {
- pfn = page_to_pfn(virt_to_page(cpu_addr));
+ pfn = PHYS_PFN(dma_to_phys(dev, dma_addr));
}
return remap_pfn_range(vma, vma->vm_start, pfn + vma->vm_pgoff,
