[RFC] Export alignment requirements for buffers

[RFC] Export alignment requirements for buffers

[Hans Verkuil]

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
#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.

These flags can be set for every vb2 dma implementation:

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.

The flags field can be extended with things like OPAQUE if the buffers cannot be
mmapped (since they are in protected memory).

Comments? Alternative solutions?

Regards,

	Hans

Re: [RFC] Export alignment requirements for buffers

[Sakari Ailus]

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.

> #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.

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.

-- 
Regards,

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

Re: [RFC] Export alignment requirements for buffers

[Hans Verkuil]

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?

> 
>> #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.

> 
> 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.

Regards,

	Hans

Re: [RFC] Export alignment requirements for buffers

[Hans Verkuil]

On 06/01/2015 11:44 AM, 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
> #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.
> 
> These flags can be set for every vb2 dma implementation:
> 
> 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.
> 
> The flags field can be extended with things like OPAQUE if the buffers cannot be
> mmapped (since they are in protected memory).
> 
> Comments? Alternative solutions?

I realized that we need this information for each plane. v4l2_requestbuffers does
not have enough room for that so for now I will do a test implementation using
v4l2_create_buffers only. Perhaps later requestbuffers can just expose the 'worst
case' alignment requirements which would be fine in most (and currently AFAIK all)
cases.

Regards,

	Hans

Re: [RFC] Export alignment requirements for buffers

[Laurent Pinchart]

Hi Hans,

On Monday 01 June 2015 11:44:51 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.

Doesn't the first issue also apply to DMABUF ?

As you already know, I'm not a big fan of USERPTR when used for sharing 
buffers between devices. DMABUF is a much better choice there. USERPTR can 
still be helpful for other use cases though. One of them that comes to my mind 
is to capturing directly buffers allocated by a software codec (or another 
similar userspace library) that doesn't support externally allocated memory 
(although the core issue there would be the library design).

Anyway, the problem of conveying memory constraints is identical in the DMABUF 
case, so a solution is needed.

> 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
> #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.
> 
> These flags can be set for every vb2 dma implementation:
> 
> 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.
> 
> The flags field can be extended with things like OPAQUE if the buffers
> cannot be mmapped (since they are in protected memory).
> 
> Comments? Alternative solutions?

The question of conveying memory constraints has been raised several times in 
the past, without any solutions being merged. The work has been revived 
recently, see http://lists.freedesktop.org/archives/dri-devel/2015-February/076862.html for instance.

Even if the API proposed here is specific to V4L2, and even if the patch set I 
linked to addresses a different problem, I believe it would be wise to widen 
the audience to make sure the solutions we come up with are at least aligned 
between subsystems. I've thus CC'ed Sumit to this e-mail as a start.

-- 
Regards,

Laurent Pinchart

Re: [RFC] Export alignment requirements for buffers

[Sakari Ailus]

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@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC] Export alignment requirements for buffers

[Sumit Semwal]

Hi Laurent,

Thanks for looping me into this email chain, and apologies about not
responding earlier; it just got lost in the barrage of things.

On 1 June 2015 at 21:20, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Hans,
>
> On Monday 01 June 2015 11:44:51 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.
>
> Doesn't the first issue also apply to DMABUF ?
>
> As you already know, I'm not a big fan of USERPTR when used for sharing
> buffers between devices. DMABUF is a much better choice there. USERPTR can
> still be helpful for other use cases though. One of them that comes to my mind
> is to capturing directly buffers allocated by a software codec (or another
> similar userspace library) that doesn't support externally allocated memory
> (although the core issue there would be the library design).
>
> Anyway, the problem of conveying memory constraints is identical in the DMABUF
> case, so a solution is needed.

Yes, +1 on that.

The problem posed here is similar in the sense of requirement of
conveying memory constraints, but the key is probably the reverse
direction - the userspace here *needs* to share constraints of
_buffers_ obtained from elsewhere.

>
>> 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
>> #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.
>>
>> These flags can be set for every vb2 dma implementation:
>>
>> 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.
>>
>> The flags field can be extended with things like OPAQUE if the buffers
>> cannot be mmapped (since they are in protected memory).
>>
>> Comments? Alternative solutions?
>
> The question of conveying memory constraints has been raised several times in
> the past, without any solutions being merged. The work has been revived
> recently, see http://lists.freedesktop.org/archives/dri-devel/2015-February/076862.html for instance.
>
> Even if the API proposed here is specific to V4L2, and even if the patch set I
> linked to addresses a different problem, I believe it would be wise to widen
> the audience to make sure the solutions we come up with are at least aligned
> between subsystems. I've thus CC'ed Sumit to this e-mail as a start.
>

So I've been (trying to) work out some way of conveying memory
constraints between devices, and our idea was to get that added to
struct device->dma_parameters; that ways, the userspace doesn't have
to necessarily know of the constraints, and some negotiation can
happen in the kernel itself. It doesn't at the moment concern with
taking care of sharing those back to userspace, but I think that's an
orthogonal thing to handle.

Absolutely, it'd be great if we can come up with some aligned way of
conveying these constraints.
> --
> Regards,
>
> Laurent Pinchart
>



-- 
Thanks and regards,

Sumit Semwal
Kernel SubTeam Lead - Linaro Mobile Group
Linaro.org │ Open source software for ARM SoCs