Hello,
On 2015-06-03 03:22, Laurent Pinchart wrote:
On Monday 01 June 2015 14:14:17 Marek Szyprowski wrote:
If device has no DMA max_seg_size set, we assume that there is no limit
and it is safe to force it to use DMA_BIT_MASK(32) as max_seg_size to
let DMA-mapping API always create contiguous mappings in DMA address
space. This is essential for all devices, which use dma-contig
videobuf2 memory allocator.
Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
---
drivers/media/v4l2-core/videobuf2-dma-contig.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
b/drivers/media/v4l2-core/videobuf2-dma-contig.c index
644dec73d220..9d7c1814b0f3 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -862,6 +862,23 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);
void *vb2_dma_contig_init_ctx(struct device *dev)
{
struct vb2_dc_conf *conf;
+ int err;
+
+ /*
+ * if device has no max_seg_size set, we assume that there is no limit
+ * and force it to DMA_BIT_MASK(32) to always use contiguous mappings
+ * in DMA address space
+ */
+ if (!dev->dma_parms) {
+ dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL);
I was checking how dma_parms was usually allocated and freed, and was shocked
to find that the memory is never freed. OK, actually not shocked, I had a bad
feeling about it already, but it's still not good :-/
This goes beyond the scope of this patch, but I think we need to clean up
dma_parms. The structure is 8 bytes long on 32-bit systems and 16 bytes long
on 64-bit systems. I wonder if it's really worth it to allocate it separately
from struct device. It might if we moved more DMA-related fields to struct
device_dma_parameters but that hasn't happened since 2008 when the structure
was introduced (yes that's more than 7 years ago).
If we consider it's worth it (and I believe Josh Triplett might, in the
context of the Linux kernel tinification project), we should at least handle
allocation and free of the field coherently across drivers.
Right, the whole dma_params approach looks like some unfinished thing.
Maybe it
would be better to remove it completely instead of having separate structure
just for 2 values? This will solve the allocation/freeing issue as well.
+ if (!dev->dma_parms)
+ return ERR_PTR(-ENOMEM);
+ }
+ if (dma_get_max_seg_size(dev) < DMA_BIT_MASK(32)) {
+ err = dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
What if the device has set a maximum segment size smaller than 4GB because of
hardware limitations ?
Then it looks that it will make more sense to set max_seg_size only when dma
params structure has been allocated and keep the old value otherwise.
I also wonder whether this is the correct place to solve the issue.
Frankly I don't see any good place for this code, especially if you consider
the default 64kB value and the fact that some code relies on it, so you
cannot
change it easily in generic code.
Why is the
default value returned by dma_get_max_seg_size() set to 64kB ?
I really have no idea why the default value is 64kB, but it looks that
there are
drivers that rely on this value:
https://www.marc.info/?l=linux-arm-kernel&m=141692087708203&w=2
+ if (err)
+ return ERR_PTR(err);
+ }
conf = kzalloc(sizeof *conf, GFP_KERNEL);
if (!conf)
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
