Re: [PATCH v2 25/26] omap3isp: Move to videobuf2

[Sakari Ailus <sakari.ailus@iki.fi>]

Hi Tim,

On Wed, Mar 18, 2015 at 03:58:07PM -0500, Tim Nordell wrote:
> Laurent -
> 
> On 03/18/15 14:49, Tim Nordell wrote:
> >Digging through to find who is responsible for assigning the virtual
> >addresses, I find that it's buried inside
> >arch/arm/mm/dma-mapping.c:__alloc_iova(...).  This call is called
> >individually for each entry in the scatter-gather table via
> >__map_sg_chunk from iommu_map_sg(...).  If this is supposed to allocate
> >a contiguous virtual memory region, it seems that __iommu_map_sg(...)
> >should be considering the full buffer range rather than parts of the
> >buffer at a time for the virtual allocation, similar to how
> >__iommu_create_mapping(...) works in the same file.
> >
> >- Tim
> >
> 
> I've confirmed that this code is the culprit for allocating a
> non-contiguous space (called via the dma_map_sg_attrs(...) back down
> in the videobuf2-dma-contig).  I've reworked it for testing so that
> it does an __alloc_iova(...) on the entire region rather than a
> chunk at a time, however, I don't think what I have locally is
> completely the right approach for the generic case since I think
> technically a given entry in the scatterlist could end up with the
> end of a page partially used (the per list entry ->offset and such).
> 
> Looks like code (the specific functions in mm/dma-mapping.c) in
> question was last touched in 2012 with a quick git-blame, but I
> don't know how long the OMAP 3 ISP code has been using this common
> code.  I'm guessing it's only been since the virtual memory manager
> internal to the IOMMU code was removed in July of last year.

I don't think omap3isp has been using this very long. A few minor versions
perhaps.

> Do you know if this common code is supposed to guarantee a
> physically contiguous memory region?  The documentation for the
> function doesn't indicate that it should, and it certainly doesn't
> as-is.  It seems like hitting this issue is highly dependent on the
> size of the buffer one is allocating.

I guess there aren't too many drivers that may map large areas of memory
pinned using get_user_pages() to IOMMU. If dma_map_sg() couldn't be used to
allocate virtually contiguous memory, then what could be? This looks like a
bug in __iommu_map_sg() to me.

Cc the iommu list.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk