Hi Hans, On Mon, Jun 01, 2015 at 01:02:26PM +0200, Hans Verkuil wrote: > On 06/01/2015 12:44 PM, Sakari Ailus wrote: > > Hi Hans, > > > > Thanks for the RFC. > > > > On Mon, Jun 01, 2015 at 11:44:51AM +0200, Hans Verkuil wrote: > >> One of the things that is really irritating is the fact that drivers that > >> use contig-dma sometimes want to support USERPTR, allowing applications to > >> pass pointers to the driver that point to physically contiguous memory that > >> was somehow obtained, and that userspace has no way of knowing whether the > >> driver has this requirement or not. > >> > >> A related issue is that, depending on the DMA engine, the user pointer might > >> have some alignment requirements (page aligned, or at minimum 16 bytes aligned) > >> that userspace has no way of knowing. > >> > >> The same alignment issue is present also for dma-buf. > >> > >> I propose to take one reserved field from struct v4l2_create_buffers and > >> from struct v4l2_requestbuffers and change it to this: > >> > >> __u32 flags; > >> > >> #define V4L2_REQBUFS_FL_ALIGNMENT_MSK 0x3f > > > > How about V4L2_REQBUFS_FL_ALIGN_MASK instead? It's shorter, and that msk > > part looks odd to me. > > Hmm, how to do this. Currently it masks out 6 bits which form the power-of-two > that determines the alignment. How about this: > > #define V4L2_REQBUFS_FL_ALIGN_EXP(flags) ((flags) & 0x3f) > #define V4L2_REQBUFS_FL_ALIGN_MASK(flags) ((1ULL << (flags & 0x3f)) - 1) > > That gives you both mask and the exponent. Better names are welcome :-) > ALIGN_PWR? PWR2? ALIGN_AT_BIT? I think ALIGN_EXP and ALIGN_MASK are good. > > > > >> #define V4L2_REQBUFS_FL_PHYS_CONTIG (1 << 31) > >> > >> Where the alignment is a power of 2 (and if 0 the alignment is unknown). The max > >> is 2^63, which should be enough for the foreseeable future :-) > >> > >> If the physically contiguous flag is set, then the buffer must be physically > >> contiguous. > > > > Both only apply to userptr buffers. I guess saying this in documentation > > only is enough. > > I don't follow you. Perhaps there is some confusion here? The flags field is set > by the driver, not by userspace. So PHYS_CONTIG applies to any type of buffer if > the driver uses dma-contig. And the alignment is valid for all drivers as well. What I meant was that this is mostly relevant for userptr buffers, mmap buffers are allocated by the driver as well as dma-buf buffers are, so the user has no (or little?) use for this information on those buffer types. > > > > > The approach looks good to me. > > > >> dma-contig: the PHYS_CONTIG flag is always set and the alignment is (unless overridden > >> by the driver) page aligned. > >> > >> dma-sg: the PHYS_CONTIG flag is 0 and the alignment will depend on the driver DMA > >> implementation. Note: malloc will align the buffer to 8 bytes on a 32 bit OS and 16 bytes > >> on a 64 bit OS. > >> > >> vmalloc: PHYS_CONFIG is 0 and the alignment should be 3 (2^3 == 8) for 32 bit and > >> 4 (2^4=16) for 64 bit OS. This matches malloc() which will align the buffer to > >> 8 bytes on a 32 bit OS and 16 bytes on a 64 bit OS. > > > > Ack. Many dma-sg drivers actually can handle physically contiguous memory > > since they're behind an IOMMU; the drivers can then set the flag if needed. > > All dma-sg drivers can handle phys contig memory since that's just one DMA descriptor. > > The flag is meant to say that the buffer *has* to be phys contig, not that it might > be. So dma-sg drivers will not set it, since they don't have that requirement. I meant to say drivers using dma-contig, not dma-sg. -- regards, Sakari Ailus e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx -- 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
