| Message ID | 201104122306.34909.jkrzyszt@tis.icnet.pl (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Tue, Apr 12, 2011 at 11:06:34PM +0200, Janusz Krzysztofik wrote: > The patch tries to solve this regression by using > virt_to_phys(bus_to_virt(mem->dma_handle)) instead of problematic > virt_to_phys(mem->vaddr). Who says that DMA handles are bus addresses in the strictest sense? DMA handles on ARM are the bus address to program 'dev' with in order for it to access the memory mapped by dma_alloc_coherent(). On some ARM platforms, this bus address is dependent on 'dev' - such as platforms with more than one root PCI bus, and so bus_to_virt() just doesn't hack it. What is really needed is for this problem - the mapping of DMA coherent memory into userspace - to be solved with a proper arch API rather than all these horrible hacks which subsystems instead invent. That's something I tried to do with the dma_mmap_coherent() stuff but it was shot down by linux-arch as (iirc) PA-RISC objected to it. Hence why ARM only implements it. Maybe the video drivers should try to resurect the idea, maybe only allowing this support for architectures which provide dma_mmap_coherent(). -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 12 Apr 2011, Russell King - ARM Linux wrote: > On Tue, Apr 12, 2011 at 11:06:34PM +0200, Janusz Krzysztofik wrote: > > The patch tries to solve this regression by using > > virt_to_phys(bus_to_virt(mem->dma_handle)) instead of problematic > > virt_to_phys(mem->vaddr). > > Who says that DMA handles are bus addresses in the strictest sense? > > DMA handles on ARM are the bus address to program 'dev' with in order > for it to access the memory mapped by dma_alloc_coherent(). On some > ARM platforms, this bus address is dependent on 'dev' - such as platforms > with more than one root PCI bus, and so bus_to_virt() just doesn't > hack it. > > What is really needed is for this problem - the mapping of DMA coherent > memory into userspace - to be solved with a proper arch API rather than > all these horrible hacks which subsystems instead invent. That's > something I tried to do with the dma_mmap_coherent() stuff but it was > shot down by linux-arch as (iirc) PA-RISC objected to it. > > Hence why ARM only implements it. > > Maybe the video drivers should try to resurect the idea, maybe only > allowing this support for architectures which provide dma_mmap_coherent(). I do not know how this fits into the present discussion. Perhaps everyone who reads the above message is well aware of what is below. If so my comment below is superfluous. But just in case things are otherwise it might save someone a bit of trouble in trying to write something which will work "everywhere": If one is speaking here of architecture problems, there is the additional problem that some ARM systems might have not two PCI buses, but instead no PCI bus at all. Theodore Kilgore -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue 12 Apr 2011 at 23:40:11 Russell King - ARM Linux wrote: > On Tue, Apr 12, 2011 at 11:06:34PM +0200, Janusz Krzysztofik wrote: > > The patch tries to solve this regression by using > > virt_to_phys(bus_to_virt(mem->dma_handle)) instead of problematic > > virt_to_phys(mem->vaddr). > > Who says that DMA handles are bus addresses in the strictest sense? > > DMA handles on ARM are the bus address to program 'dev' with in order > for it to access the memory mapped by dma_alloc_coherent(). On some > ARM platforms, this bus address is dependent on 'dev' - such as > platforms with more than one root PCI bus, and so bus_to_virt() just > doesn't hack it. Taking into account that I'm just trying to fix a regression, and not invent a new, long term solution: are you able to name an ARM based board which a) is already supported in 2.6.39, b) is (or can be) equipped with a device supported by a V4L driver which uses videobuf- dma-config susbsystem, c) has a bus structure with which virt_to_phys(bus_to_virt(dma_handle)) is not equal dma_handle? If there is one, then I agree that my short-term fix is wrong. > What is really needed is for this problem - the mapping of DMA > coherent memory into userspace - to be solved with a proper arch API > rather than all these horrible hacks which subsystems instead > invent. That's something I tried to do with the dma_mmap_coherent() > stuff but it was shot down by linux-arch as (iirc) PA-RISC objected > to it. > > Hence why ARM only implements it. I thought so too, but missed the fact that PowerPC implements it actually, even defining the ARCH_HAS_DMA_MMAP_COHERENT symbol, which ARM doesn't so far. > Maybe the video drivers should try to resurect the idea, maybe only > allowing this support for architectures which provide > dma_mmap_coherent(). AFAICT, ARM implementation of dma_mmap_coherent() is not compatible with dma_declare_coherent_memory(), is it? If I'm wrong, please correct me, I'll get back to the idea presented in v1 of the fix. Otherwise, I think that switching to dma_mmap_coherent() is not an option for the videobuf-dma-contig subsystem as long as there is no good solution to the problem of dma_alloc_coherent() not guaranteed to succeed with high-order allocations at any time. Any chance for your already proposed (http://lists.infradead.org/pipermail/linux-arm-kernel/2010-December/036463.html, http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/036809.html), or perhaps a new, better solution ever finding its way to the mainline tree? Thanks, Janusz -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 13-04-2011 1:06, Janusz Krzysztofik wrote: > After switching from mem->dma_handle to virt_to_phys(mem->vaddr) used > for obtaining page frame number passed to remap_pfn_range() > (commit 35d9f510b67b10338161aba6229d4f55b4000f5b), videobuf-dma-contig Please specify the commit summary -- for the human readers. > stopped working on my ARM based board. The ARM architecture maintainer, > Russell King, confirmed that using something like > virt_to_phys(dma_alloc_coherent()) is not supported on ARM, and can be > broken on other architectures as well. The author of the change, Jiri > Slaby, also confirmed that his code may not work on all architectures. > The patch tries to solve this regression by using > virt_to_phys(bus_to_virt(mem->dma_handle)) instead of problematic > virt_to_phys(mem->vaddr). I think this should work even if those > translations would occure inaccurate for DMA addresses, since possible > errors introduced by both translations, performed in opposite > directions, should compensate. > Tested on ARM OMAP1 based Amstrad Delta board. > Signed-off-by: Janusz Krzysztofik<jkrzyszt@tis.icnet.pl> WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dnia ?roda 13 kwiecie? 2011 o 14:03:44 Sergei Shtylyov napisa?(a): > Hello. > > On 13-04-2011 1:06, Janusz Krzysztofik wrote: > > After switching from mem->dma_handle to virt_to_phys(mem->vaddr) > > used for obtaining page frame number passed to remap_pfn_range() > > (commit 35d9f510b67b10338161aba6229d4f55b4000f5b), > > videobuf-dma-contig > > Please specify the commit summary -- for the human readers. Hi, OK, I'll try to reword the summary using a more human friendly language as soon as I have signs that Mauro (who seemed to understand the message well enough) is willing to accept the code. Thanks, Janusz -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. Janusz Krzysztofik wrote: >>> After switching from mem->dma_handle to virt_to_phys(mem->vaddr) >>> used for obtaining page frame number passed to remap_pfn_range() >>> (commit 35d9f510b67b10338161aba6229d4f55b4000f5b), >>> videobuf-dma-contig >> Please specify the commit summary -- for the human readers. > Hi, > OK, I'll try to reword the summary using a more human friendly language > as soon as I have signs that Mauro (who seemed to understand the message > well enough) is willing to accept the code. I wasn't asking you to rework your summary but to specify the summary of the commit you've mentioned (in parens). > Thanks, > Janusz WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 13, 2011 at 12:52:31PM +0200, Janusz Krzysztofik wrote: > Taking into account that I'm just trying to fix a regression, and not > invent a new, long term solution: are you able to name an ARM based > board which a) is already supported in 2.6.39, b) is (or can be) > equipped with a device supported by a V4L driver which uses videobuf- > dma-config susbsystem, c) has a bus structure with which > virt_to_phys(bus_to_virt(dma_handle)) is not equal dma_handle? I have no idea - and why should whether someone can name something that may break be a justification to allow something which is technically wrong? Surely it should be the other way around - if its technically wrong and _may_ break something then it shouldn't be allowed. > I thought so too, but missed the fact that PowerPC implements it > actually, even defining the ARCH_HAS_DMA_MMAP_COHERENT symbol, which ARM > doesn't so far. So, there's no problem adding that symbol to ARM. > > Maybe the video drivers should try to resurect the idea, maybe only > > allowing this support for architectures which provide > > dma_mmap_coherent(). > > AFAICT, ARM implementation of dma_mmap_coherent() is not compatible with > dma_declare_coherent_memory(), is it? If I'm wrong, please correct me, > I'll get back to the idea presented in v1 of the fix. 1. dma_declare_coherent_memory() doesn't work on ARM for memory which already exists (its not permitted to ioremap() the kernel direct-mapped memory due to attribute aliasing issues.) 2. dma_declare_coherent_memory() totally bypasses the DMA allocator, and so dma_mmap_coherent() has no knowledge of how to map the memory. If we had a proper way to map DMA memory into userspace, then we wouldn't have these issues as the dma_declare_coherent_memory() would already support that. And actually, talking about dma_declare_coherent_memory(), you've just provided a reason why virt_to_phys(bus_to_virt(dma_handle)) won't work - dma_declare_coherent_memory() can be used to provide on-device memory where the virt/handle may not be mappable with bus_to_virt(). > Otherwise, I think that switching to dma_mmap_coherent() is not an > option for the videobuf-dma-contig subsystem as long as there is no good > solution to the problem of dma_alloc_coherent() not guaranteed to > succeed with high-order allocations at any time. Let me repeat: there is no official API or way to map DMA memory into userspace. Every way people try is a half-hacked up bodge which may or may not work for a limited subset of systems. Until someone (like you) puts some serious effort into persuading *everyone* that this feature is needed, things are just going to continue being bodged and fragile. All that's happening here is that you're changing one broken way which works for one subset with another broken way which works for a different subset of systems and architectures. What actually needs to happen is a _proper_ fix for this. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dnia ?roda 13 kwiecie? 2011 o 20:32:31 Russell King - ARM Linux napisa?(a): > On Wed, Apr 13, 2011 at 12:52:31PM +0200, Janusz Krzysztofik wrote: > > Taking into account that I'm just trying to fix a regression, and > > not invent a new, long term solution: are you able to name an ARM > > based board which a) is already supported in 2.6.39, b) is (or can > > be) equipped with a device supported by a V4L driver which uses > > videobuf- dma-config susbsystem, c) has a bus structure with which > > virt_to_phys(bus_to_virt(dma_handle)) is not equal dma_handle? > > I have no idea - and why should whether someone can name something > that may break be a justification to allow something which is > technically wrong? > > Surely it should be the other way around - if its technically wrong > and _may_ break something then it shouldn't be allowed. In theory - of course. In practice - couldn't we now, close to -rc3, relax the rules a little bit and stop bothering with something that may break in the future if it doesn't break on any board supported so far (I hope)? > > I thought so too, but missed the fact that PowerPC implements it > > actually, even defining the ARCH_HAS_DMA_MMAP_COHERENT symbol, > > which ARM doesn't so far. > > So, there's no problem adding that symbol to ARM. OK, I can provide a patch as soon as dma_mmap_coherent() really works for me. > > > Maybe the video drivers should try to resurect the idea, maybe > > > only allowing this support for architectures which provide > > > dma_mmap_coherent(). > > > > AFAICT, ARM implementation of dma_mmap_coherent() is not compatible > > with dma_declare_coherent_memory(), is it? If I'm wrong, please > > correct me, I'll get back to the idea presented in v1 of the fix. > > 1. dma_declare_coherent_memory() doesn't work on ARM for memory which > already exists (its not permitted to ioremap() the kernel > direct-mapped memory due to attribute aliasing issues.) But you had once inspired (http://lists.infradead.org/pipermail/linux-arm-kernel/2010-December/034879.html), then was supporting my attempts towards pushing ioremap() out of this function up to the caller (http://lists.infradead.org/pipermail/linux-arm-kernel/2010-December/036419.html), which would allow for doing sane preallocations via dma_coherent_alloc() and caching them back into dma_declare_coherent_memory() at boot time for later reuse mempry from that pool as DMA coherent. Now, should my attepmts succeded, we would still end up with the following: > 2. dma_declare_coherent_memory() totally bypasses the DMA allocator, Would it still, under your terms, if it could accept dma_alloc_coherent() obtained cpu addresses, not trying to ioremap() them? > and so dma_mmap_coherent() has no knowledge of how to map the > memory. I think it _could_ have a good knowledge if that memory was first preallocated with dma_alloc_coherent() at boot time, unless that memory was then fetched from that pool in smaller chunks. I think this is the reason it didn't work for me when I tried using this method with dma_mmap_coherent(). Am I missing something? > If we had a proper way to map DMA memory into userspace, then we > wouldn't have these issues as the dma_declare_coherent_memory() > would already support that. > > And actually, talking about dma_declare_coherent_memory(), you've > just provided a reason why virt_to_phys(bus_to_virt(dma_handle)) > won't work - dma_declare_coherent_memory() can be used to provide > on-device memory where the virt/handle may not be mappable with > bus_to_virt(). While I have no problems to agree with the principles, I can confirm that this _hack_ does coexist nicely with dma_declare_coherent_memory(), at least on my OMAP1 based board. It also coexists nicely with your WiP patches I mentioned before and you didn't quote here, so I provide the links again: http://lists.infradead.org/pipermail/linux-arm-kernel/2010-December/036461.html, http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/036809.html. OTOH, dma_mmap_coherent() didn't work for me when I tried using it on top of those patches. This doesn't mean I'm against a dma_mmap_coherent() based solution. I just think we can't afford switching to it _now_. > > Otherwise, I think that switching to dma_mmap_coherent() is not an > > option for the videobuf-dma-contig subsystem as long as there is no > > good solution to the problem of dma_alloc_coherent() not > > guaranteed to succeed with high-order allocations at any time. > > Let me repeat: there is no official API or way to map DMA memory into > userspace. Doesn't dma_mmap_coherent(), already available on 2 architectures, ARM and PPC, aim to provide the correct API? From the fact you didn't dispute v1 of my patch, which provided a dma_mmap_coherent() based code path for architectures supporting it, I would conclude this is a solution which might get your support, isn't it? However, I think that even if it was a _proper_ solution to the problem, it couldn't be accepted as a fix during the rc cycle for a simple reason: this would break all those boards (ab)using dma_declare_coherent_memory(). > Every way people try is a half-hacked up bodge which may > or may not work for a limited subset of systems. > > Until someone (like you) puts some serious effort into persuading > *everyone* that this feature is needed, things are just going to > continue being bodged and fragile. > > All that's happening here is that you're changing one broken way > which works for one subset with another broken way which works for a > different subset of systems and architectures. What actually needs > to happen is a _proper_ fix for this. I don't believe you really think it would possible to come out with a _proper_ solution to the problem which could still be accepted as a _fix_. We are close to -rc3. If I'm wrong, and you think this can still be fixed properly for this rc cycle, please share more of your idea and I'll try to do my best to implement it. Thanks, Janusz -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dnia ?roda 13 kwiecie? 2011 o 19:36:30 Sergei Shtylyov napisa?(a): > Hello. > > Janusz Krzysztofik wrote: > >>> After switching from mem->dma_handle to virt_to_phys(mem->vaddr) > >>> used for obtaining page frame number passed to remap_pfn_range() > >>> (commit 35d9f510b67b10338161aba6229d4f55b4000f5b), > >>> videobuf-dma-contig > >>> > >> Please specify the commit summary -- for the human readers. > > > > Hi, > > OK, I'll try to reword the summary using a more human friendly > > language as soon as I have signs that Mauro (who seemed to > > understand the message well enough) is willing to accept the code. > > I wasn't asking you to rework your summary but to specify the > summary of the commit you've mentioned (in parens). Ah, I see. How about just reordered wording: After commit 35d9f510b67b10338161aba6229d4f55b4000f5b (switching from mem->dma_handle to virt_to_phys(mem->vaddr) used for obtaining page frame number passed to remap_pfn_range()), .... Do you think this would be clear enough? Thanks, Janusz -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dnia ?roda 13 kwiecie? 2011 o 23:01:55 Janusz Krzysztofik napisa?(a): > Dnia ?roda 13 kwiecie? 2011 o 19:36:30 Sergei Shtylyov napisa?(a): > > Hello. > > > > Janusz Krzysztofik wrote: > > >>> After switching from mem->dma_handle to > > >>> virt_to_phys(mem->vaddr) used for obtaining page frame number > > >>> passed to remap_pfn_range() (commit > > >>> 35d9f510b67b10338161aba6229d4f55b4000f5b), > > >>> videobuf-dma-contig > > >>> > > >> Please specify the commit summary -- for the human readers. > > > > > > Hi, > > > OK, I'll try to reword the summary using a more human friendly > > > language as soon as I have signs that Mauro (who seemed to > > > understand the message well enough) is willing to accept the > > > code. > > > > > I wasn't asking you to rework your summary but to specify the > > > > summary of the commit you've mentioned (in parens). > > Ah, I see. How about just reordered wording: > > After commit 35d9f510b67b10338161aba6229d4f55b4000f5b (switching from > mem->dma_handle to virt_to_phys(mem->vaddr) used for obtaining page > frame number passed to remap_pfn_range()), .... > > Do you think this would be clear enough? Oh no, I probably missed your point again. You meant just quoting the commit original summary line, didn't you? Thanks, Janusz -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 13, 2011 at 10:56:39PM +0200, Janusz Krzysztofik wrote: > Dnia ?roda 13 kwiecie? 2011 o 20:32:31 Russell King - ARM Linux > napisa?(a): > > On Wed, Apr 13, 2011 at 12:52:31PM +0200, Janusz Krzysztofik wrote: > > > Taking into account that I'm just trying to fix a regression, and > > > not invent a new, long term solution: are you able to name an ARM > > > based board which a) is already supported in 2.6.39, b) is (or can > > > be) equipped with a device supported by a V4L driver which uses > > > videobuf- dma-config susbsystem, c) has a bus structure with which > > > virt_to_phys(bus_to_virt(dma_handle)) is not equal dma_handle? > > > > I have no idea - and why should whether someone can name something > > that may break be a justification to allow something which is > > technically wrong? > > > > Surely it should be the other way around - if its technically wrong > > and _may_ break something then it shouldn't be allowed. > > In theory - of course. In practice - couldn't we now, close to -rc3, > relax the rules a little bit and stop bothering with something that may > break in the future if it doesn't break on any board supported so far (I > hope)? If we are worried about closeness to -final, then what should happen is that the original commit is reverted; the "fix" for IOMMUs resulted in a regression for existing users which isn't trivial to resolve without risking possible breakage of other users. Do we even know whether bus_to_virt(iommu_bus_address) works? I suspect it doesn't, so by doing so you're already re-breaking the IOMMU case. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dnia czwartek 14 kwiecie? 2011 o 00:00:08 Russell King - ARM Linux napisa?(a): > On Wed, Apr 13, 2011 at 10:56:39PM +0200, Janusz Krzysztofik wrote: > > Dnia ?roda 13 kwiecie? 2011 o 20:32:31 Russell King - ARM Linux > > > > napisa?(a): > > > On Wed, Apr 13, 2011 at 12:52:31PM +0200, Janusz Krzysztofik wrote: > > > > Taking into account that I'm just trying to fix a regression, > > > > and not invent a new, long term solution: are you able to name > > > > an ARM based board which a) is already supported in 2.6.39, b) > > > > is (or can be) equipped with a device supported by a V4L > > > > driver which uses videobuf- dma-config susbsystem, c) has a > > > > bus structure with which virt_to_phys(bus_to_virt(dma_handle)) > > > > is not equal dma_handle? > > > > > > I have no idea - and why should whether someone can name > > > something that may break be a justification to allow something > > > which is technically wrong? > > > > > > Surely it should be the other way around - if its technically > > > wrong and _may_ break something then it shouldn't be allowed. > > > > In theory - of course. In practice - couldn't we now, close to > > -rc3, relax the rules a little bit and stop bothering with > > something that may break in the future if it doesn't break on any > > board supported so far (I hope)? > > If we are worried about closeness to -final, then what should happen > is that the original commit is reverted; the "fix" for IOMMUs > resulted in a regression for existing users which isn't trivial to > resolve without risking possible breakage of other users. > > Do we even know whether bus_to_virt(iommu_bus_address) works? I > suspect it doesn't, so by doing so you're already re-breaking the > IOMMU case. Hard to deny with only me having actually tested this dirty hack on one single board :) Thanks for your support, Janusz -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 14-04-2011 1:01, Janusz Krzysztofik wrote: >>>>> After switching from mem->dma_handle to virt_to_phys(mem->vaddr) >>>>> used for obtaining page frame number passed to remap_pfn_range() >>>>> (commit 35d9f510b67b10338161aba6229d4f55b4000f5b), >>>>> videobuf-dma-contig >>>> Please specify the commit summary -- for the human readers. >>> Hi, >>> OK, I'll try to reword the summary using a more human friendly >>> language as soon as I have signs that Mauro (who seemed to >>> understand the message well enough) is willing to accept the code. >> I wasn't asking you to rework your summary but to specify the >> summary of the commit you've mentioned (in parens). > Ah, I see. How about just reordered wording: > After commit 35d9f510b67b10338161aba6229d4f55b4000f5b (switching from > mem->dma_handle to virt_to_phys(mem->vaddr) used for obtaining page > frame number passed to remap_pfn_range()), .... > Do you think this would be clear enough? As I can see, the summary of that commit is "[media] V4L: videobuf, don't use dma addr as physical". That's what you should have inserted. > Thanks, > Janusz WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 14-04-2011 1:16, Janusz Krzysztofik wrote: >>> Janusz Krzysztofik wrote: >>>>>> After switching from mem->dma_handle to >>>>>> virt_to_phys(mem->vaddr) used for obtaining page frame number >>>>>> passed to remap_pfn_range() (commit >>>>>> 35d9f510b67b10338161aba6229d4f55b4000f5b), >>>>>> videobuf-dma-contig >>>>> Please specify the commit summary -- for the human readers. >>>> Hi, >>>> OK, I'll try to reword the summary using a more human friendly >>>> language as soon as I have signs that Mauro (who seemed to >>>> understand the message well enough) is willing to accept the >>>> code. >>> I wasn't asking you to rework your summary but to specify the >>> summary of the commit you've mentioned (in parens). >> Ah, I see. How about just reordered wording: >> After commit 35d9f510b67b10338161aba6229d4f55b4000f5b (switching from >> mem->dma_handle to virt_to_phys(mem->vaddr) used for obtaining page >> frame number passed to remap_pfn_range()), .... >> Do you think this would be clear enough? > Oh no, I probably missed your point again. > You meant just quoting the commit original summary line, didn't you? Yes. > Thanks, > Janusz WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- git/drivers/media/video/videobuf-dma-contig.c.orig 2011-04-09 00:38:45.000000000 +0200 +++ git/drivers/media/video/videobuf-dma-contig.c 2011-04-12 22:36:44.000000000 +0200 @@ -300,8 +300,8 @@ static int __videobuf_mmap_mapper(struct vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); retval = remap_pfn_range(vma, vma->vm_start, - PFN_DOWN(virt_to_phys(mem->vaddr)), - size, vma->vm_page_prot); + PFN_DOWN(virt_to_phys(bus_to_virt(mem->dma_handle))), + size, vma->vm_page_prot); if (retval) { dev_err(q->dev, "mmap: remap failed with error %d. ", retval); dma_free_coherent(q->dev, mem->size,
After switching from mem->dma_handle to virt_to_phys(mem->vaddr) used for obtaining page frame number passed to remap_pfn_range() (commit 35d9f510b67b10338161aba6229d4f55b4000f5b), videobuf-dma-contig stopped working on my ARM based board. The ARM architecture maintainer, Russell King, confirmed that using something like virt_to_phys(dma_alloc_coherent()) is not supported on ARM, and can be broken on other architectures as well. The author of the change, Jiri Slaby, also confirmed that his code may not work on all architectures. The patch tries to solve this regression by using virt_to_phys(bus_to_virt(mem->dma_handle)) instead of problematic virt_to_phys(mem->vaddr). I think this should work even if those translations would occure inaccurate for DMA addresses, since possible errors introduced by both translations, performed in opposite directions, should compensate. Tested on ARM OMAP1 based Amstrad Delta board. Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> --- v1 -> v2 changes: - drop dma_mmap_coherent() path, it may not work correctly for device memory preallocated with dma_declare_coherent_memory(). drivers/media/video/videobuf-dma-contig.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html