Question: partial transfers of DMABUFs

Question: partial transfers of DMABUFs

[Paul Cercueil]

Hi,

I am working on adding support for DMABUFs in the IIO subsystem.

One thing we want there, is the ability to specify the number of bytes
to transfer (while still defaulting to the DMABUF size).

Since dma_buf_map_attachment() returns a sg_table, I basically have two
options, and I can't decide which one is the best (or the less ugly):

- Either I add a new API function similar to dmaengine_prep_slave_sg(),
which still takes a scatterlist as argument but also takes the number
of bytes as argument;

- Or I add a function to duplicate the scatterlist and then shrink it
manually, which doesn't sound like a good idea either.

What would be the recommended way?

Cheers,
-Paul

Question: partial transfers of DMABUFs

[Paul Cercueil]

Hi,

I am working on adding support for DMABUFs in the IIO subsystem.

One thing we want there, is the ability to specify the number of bytes
to transfer (while still defaulting to the DMABUF size).

Since dma_buf_map_attachment() returns a sg_table, I basically have two
options, and I can't decide which one is the best (or the less ugly):

- Either I add a new API function similar to dmaengine_prep_slave_sg(),
which still takes a scatterlist as argument but also takes the number
of bytes as argument;

- Or I add a function to duplicate the scatterlist and then shrink it
manually, which doesn't sound like a good idea either.

What would be the recommended way?

Cheers,
-Paul

Re: Question: partial transfers of DMABUFs

[Maarten Lankhorst]

Hey,

On 2023-02-15 11:48, Paul Cercueil wrote:
> Hi,
>
> I am working on adding support for DMABUFs in the IIO subsystem.
>
> One thing we want there, is the ability to specify the number of bytes
> to transfer (while still defaulting to the DMABUF size).
>
> Since dma_buf_map_attachment() returns a sg_table, I basically have two
> options, and I can't decide which one is the best (or the less ugly):
>
> - Either I add a new API function similar to dmaengine_prep_slave_sg(),
> which still takes a scatterlist as argument but also takes the number
> of bytes as argument;
>
> - Or I add a function to duplicate the scatterlist and then shrink it
> manually, which doesn't sound like a good idea either.
>
> What would be the recommended way?

Does this need an api change? If you create a DMA-BUF of size X, it has 
to be of size X. You can pad with a dummy page probably if you know it 
in advance. But after it has been imported, it cannot change size.

You don´t have to write the entire dma-buf either, so if you want to 
create a 1GB buf and only use the first 4K, that is allowed. The 
contents of  the remainder of the DMA-BUF are undefined. It's up to 
userspace to assign a meaning to it.

I think I'm missing something here that makes the whole question m,ake 
more sense.

~Maarten

Re: Question: partial transfers of DMABUFs

[Paul Cercueil]

Hi Maarten,

Le mercredi 15 février 2023 à 12:30 +0100, Maarten Lankhorst a écrit :
> Hey,
> 
> On 2023-02-15 11:48, Paul Cercueil wrote:
> > Hi,
> > 
> > I am working on adding support for DMABUFs in the IIO subsystem.
> > 
> > One thing we want there, is the ability to specify the number of
> > bytes
> > to transfer (while still defaulting to the DMABUF size).
> > 
> > Since dma_buf_map_attachment() returns a sg_table, I basically have
> > two
> > options, and I can't decide which one is the best (or the less
> > ugly):
> > 
> > - Either I add a new API function similar to
> > dmaengine_prep_slave_sg(),
> > which still takes a scatterlist as argument but also takes the
> > number
> > of bytes as argument;
> > 
> > - Or I add a function to duplicate the scatterlist and then shrink
> > it
> > manually, which doesn't sound like a good idea either.
> > 
> > What would be the recommended way?
> 
> Does this need an api change? If you create a DMA-BUF of size X, it
> has 
> to be of size X. You can pad with a dummy page probably if you know
> it 
> in advance. But after it has been imported, it cannot change size.

Yes, the sizes are fixed.

> You don´t have to write the entire dma-buf either, so if you want to 
> create a 1GB buf and only use the first 4K, that is allowed. The 
> contents of  the remainder of the DMA-BUF are undefined. It's up to 
> userspace to assign a meaning to it.
> 
> I think I'm missing something here that makes the whole question
> m,ake 
> more sense.

I want my userspace to be able to specify how much of the DMABUF is to
be read from or written to.

So in my new "dmabuf enqueue" IOCTL that I want to add to IIO, I added
a parameter to specify the number of bytes to transfer (where 0 means
the whole buffer).

The problem I have now, is that the current dmaengine core does not
have a API function that takes a scatterlist (returned by
dma_map_attachment()) and a transfer size in bytes, it will always
transfer the whole scatterlist.

So my two options would be to add a new API function to support
specifying a bytes count, or add a mechanism to duplicate a
scatterlist, so that I can tweak it to the right size.

> ~Maarten

Cheers,
-Paul

Re: Question: partial transfers of DMABUFs

[Maarten Lankhorst]

Hey,

On 2023-02-15 12:47, Paul Cercueil wrote:
> Hi Maarten,
>
> Le mercredi 15 février 2023 à 12:30 +0100, Maarten Lankhorst a écrit :
>> Hey,
>>
>> On 2023-02-15 11:48, Paul Cercueil wrote:
>>> Hi,
>>>
>>> I am working on adding support for DMABUFs in the IIO subsystem.
>>>
>>> One thing we want there, is the ability to specify the number of
>>> bytes
>>> to transfer (while still defaulting to the DMABUF size).
>>>
>>> Since dma_buf_map_attachment() returns a sg_table, I basically have
>>> two
>>> options, and I can't decide which one is the best (or the less
>>> ugly):
>>>
>>> - Either I add a new API function similar to
>>> dmaengine_prep_slave_sg(),
>>> which still takes a scatterlist as argument but also takes the
>>> number
>>> of bytes as argument;
>>>
>>> - Or I add a function to duplicate the scatterlist and then shrink
>>> it
>>> manually, which doesn't sound like a good idea either.
>>>
>>> What would be the recommended way?
>> Does this need an api change? If you create a DMA-BUF of size X, it
>> has
>> to be of size X. You can pad with a dummy page probably if you know
>> it
>> in advance. But after it has been imported, it cannot change size.
> Yes, the sizes are fixed.
>
>> You don´t have to write the entire dma-buf either, so if you want to
>> create a 1GB buf and only use the first 4K, that is allowed. The
>> contents of  the remainder of the DMA-BUF are undefined. It's up to
>> userspace to assign a meaning to it.
>>
>> I think I'm missing something here that makes the whole question
>> m,ake
>> more sense.
> I want my userspace to be able to specify how much of the DMABUF is to
> be read from or written to.
>
> So in my new "dmabuf enqueue" IOCTL that I want to add to IIO, I added
> a parameter to specify the number of bytes to transfer (where 0 means
> the whole buffer).
>
> The problem I have now, is that the current dmaengine core does not
> have a API function that takes a scatterlist (returned by
> dma_map_attachment()) and a transfer size in bytes, it will always
> transfer the whole scatterlist.
>
> So my two options would be to add a new API function to support
> specifying a bytes count, or add a mechanism to duplicate a
> scatterlist, so that I can tweak it to the right size.

This doesn't have to happen through DMA-BUF. Presumably you are both the 
importer and the exporter, so after you know how much is read, you can 
tell this to the importer that X number of bytes can be read from DMA-BUF Y.

In your case, when enqueing you will get a full SG list, but if you know 
only X bytes are read/written you only have to map the first X bytes to 
your IIO device. The rest of the SG list could be ignored safely.

~Maarten

Re: Question: partial transfers of DMABUFs

[Paul Cercueil]

Hi Maarten,

Le mercredi 15 février 2023 à 12:52 +0100, Maarten Lankhorst a écrit :
> Hey,
> 
> On 2023-02-15 12:47, Paul Cercueil wrote:
> > Hi Maarten,
> > 
> > Le mercredi 15 février 2023 à 12:30 +0100, Maarten Lankhorst a
> > écrit :
> > > Hey,
> > > 
> > > On 2023-02-15 11:48, Paul Cercueil wrote:
> > > > Hi,
> > > > 
> > > > I am working on adding support for DMABUFs in the IIO
> > > > subsystem.
> > > > 
> > > > One thing we want there, is the ability to specify the number
> > > > of
> > > > bytes
> > > > to transfer (while still defaulting to the DMABUF size).
> > > > 
> > > > Since dma_buf_map_attachment() returns a sg_table, I basically
> > > > have
> > > > two
> > > > options, and I can't decide which one is the best (or the less
> > > > ugly):
> > > > 
> > > > - Either I add a new API function similar to
> > > > dmaengine_prep_slave_sg(),
> > > > which still takes a scatterlist as argument but also takes the
> > > > number
> > > > of bytes as argument;
> > > > 
> > > > - Or I add a function to duplicate the scatterlist and then
> > > > shrink
> > > > it
> > > > manually, which doesn't sound like a good idea either.
> > > > 
> > > > What would be the recommended way?
> > > Does this need an api change? If you create a DMA-BUF of size X,
> > > it
> > > has
> > > to be of size X. You can pad with a dummy page probably if you
> > > know
> > > it
> > > in advance. But after it has been imported, it cannot change
> > > size.
> > Yes, the sizes are fixed.
> > 
> > > You don´t have to write the entire dma-buf either, so if you want
> > > to
> > > create a 1GB buf and only use the first 4K, that is allowed. The
> > > contents of  the remainder of the DMA-BUF are undefined. It's up
> > > to
> > > userspace to assign a meaning to it.
> > > 
> > > I think I'm missing something here that makes the whole question
> > > m,ake
> > > more sense.
> > I want my userspace to be able to specify how much of the DMABUF is
> > to
> > be read from or written to.
> > 
> > So in my new "dmabuf enqueue" IOCTL that I want to add to IIO, I
> > added
> > a parameter to specify the number of bytes to transfer (where 0
> > means
> > the whole buffer).
> > 
> > The problem I have now, is that the current dmaengine core does not
> > have a API function that takes a scatterlist (returned by
> > dma_map_attachment()) and a transfer size in bytes, it will always
> > transfer the whole scatterlist.
> > 
> > So my two options would be to add a new API function to support
> > specifying a bytes count, or add a mechanism to duplicate a
> > scatterlist, so that I can tweak it to the right size.
> 
> This doesn't have to happen through DMA-BUF. Presumably you are both
> the 
> importer and the exporter, so after you know how much is read, you
> can 
> tell this to the importer that X number of bytes can be read from
> DMA-BUF Y.

Yes, I do that already as it is an argument in my ioctl.

> In your case, when enqueing you will get a full SG list, but if you
> know 
> only X bytes are read/written you only have to map the first X bytes
> to 
> your IIO device. The rest of the SG list could be ignored safely.

Yes. But I don't know how to "ignore the rest of the SG list".

- dma_buf_map_attachment() does not have a parameter to specify that I
only need the first X bytes mapped;

- if I map the whole thing, dmaengine_prep_slave_sg() does not have an
option to specify that I only want the first X bytes transferred.

-Paul

Re: Question: partial transfers of DMABUFs

[Maarten Lankhorst]

On 2023-02-15 13:00, Paul Cercueil wrote:
> Hi Maarten,
>
> Le mercredi 15 février 2023 à 12:52 +0100, Maarten Lankhorst a écrit :
>> Hey,
>>
>> On 2023-02-15 12:47, Paul Cercueil wrote:
>>> Hi Maarten,
>>>
>>> Le mercredi 15 février 2023 à 12:30 +0100, Maarten Lankhorst a
>>> écrit :
>>>> Hey,
>>>>
>>>> On 2023-02-15 11:48, Paul Cercueil wrote:
>>>>> Hi,
>>>>>
>>>>> I am working on adding support for DMABUFs in the IIO
>>>>> subsystem.
>>>>>
>>>>> One thing we want there, is the ability to specify the number
>>>>> of
>>>>> bytes
>>>>> to transfer (while still defaulting to the DMABUF size).
>>>>>
>>>>> Since dma_buf_map_attachment() returns a sg_table, I basically
>>>>> have
>>>>> two
>>>>> options, and I can't decide which one is the best (or the less
>>>>> ugly):
>>>>>
>>>>> - Either I add a new API function similar to
>>>>> dmaengine_prep_slave_sg(),
>>>>> which still takes a scatterlist as argument but also takes the
>>>>> number
>>>>> of bytes as argument;
>>>>>
>>>>> - Or I add a function to duplicate the scatterlist and then
>>>>> shrink
>>>>> it
>>>>> manually, which doesn't sound like a good idea either.
>>>>>
>>>>> What would be the recommended way?
>>>> Does this need an api change? If you create a DMA-BUF of size X,
>>>> it
>>>> has
>>>> to be of size X. You can pad with a dummy page probably if you
>>>> know
>>>> it
>>>> in advance. But after it has been imported, it cannot change
>>>> size.
>>> Yes, the sizes are fixed.
>>>
>>>> You don´t have to write the entire dma-buf either, so if you want
>>>> to
>>>> create a 1GB buf and only use the first 4K, that is allowed. The
>>>> contents of  the remainder of the DMA-BUF are undefined. It's up
>>>> to
>>>> userspace to assign a meaning to it.
>>>>
>>>> I think I'm missing something here that makes the whole question
>>>> m,ake
>>>> more sense.
>>> I want my userspace to be able to specify how much of the DMABUF is
>>> to
>>> be read from or written to.
>>>
>>> So in my new "dmabuf enqueue" IOCTL that I want to add to IIO, I
>>> added
>>> a parameter to specify the number of bytes to transfer (where 0
>>> means
>>> the whole buffer).
>>>
>>> The problem I have now, is that the current dmaengine core does not
>>> have a API function that takes a scatterlist (returned by
>>> dma_map_attachment()) and a transfer size in bytes, it will always
>>> transfer the whole scatterlist.
>>>
>>> So my two options would be to add a new API function to support
>>> specifying a bytes count, or add a mechanism to duplicate a
>>> scatterlist, so that I can tweak it to the right size.
>> This doesn't have to happen through DMA-BUF. Presumably you are both
>> the
>> importer and the exporter, so after you know how much is read, you
>> can
>> tell this to the importer that X number of bytes can be read from
>> DMA-BUF Y.
> Yes, I do that already as it is an argument in my ioctl.
>
>> In your case, when enqueing you will get a full SG list, but if you
>> know
>> only X bytes are read/written you only have to map the first X bytes
>> to
>> your IIO device. The rest of the SG list could be ignored safely.
> Yes. But I don't know how to "ignore the rest of the SG list".
>
> - dma_buf_map_attachment() does not have a parameter to specify that I
> only need the first X bytes mapped;
>
> - if I map the whole thing, dmaengine_prep_slave_sg() does not have an
> option to specify that I only want the first X bytes transferred.

sg_split apppears to allow you to split it? I'm not 100% sure whether it 
leaves the original SG untouched, but you can try to put it in between 
those 2 calls to get a smaller SG to pass to prep_slave_sg.

~Maarten

Re: Question: partial transfers of DMABUFs

[Christian König]

Hi Paul,

Am 15.02.23 um 11:48 schrieb Paul Cercueil:
> Hi,
>
> I am working on adding support for DMABUFs in the IIO subsystem.
>
> One thing we want there, is the ability to specify the number of bytes
> to transfer (while still defaulting to the DMABUF size).
>
> Since dma_buf_map_attachment() returns a sg_table,

Please don't assume that this is an sg_table. We just used it as 
container for DMA addresses, but this has proven to be a mistake.

There is work underway to replace the sg_table with (for example) just 
an array of DMA addresses.

> I basically have two options, and I can't decide which one is the best (or the less ugly):
>
> - Either I add a new API function similar to dmaengine_prep_slave_sg(),
> which still takes a scatterlist as argument but also takes the number
> of bytes as argument;
>
> - Or I add a function to duplicate the scatterlist and then shrink it
> manually, which doesn't sound like a good idea either.
>
> What would be the recommended way?

I strongly recommend to come up with a new function which only takes DMA 
addresses and separate segment length.

Regards,
Christian.

>
> Cheers,
> -Paul

Re: Question: partial transfers of DMABUFs

[Christian König]

Hi Paul,

Am 15.02.23 um 11:48 schrieb Paul Cercueil:
> Hi,
>
> I am working on adding support for DMABUFs in the IIO subsystem.
>
> One thing we want there, is the ability to specify the number of bytes
> to transfer (while still defaulting to the DMABUF size).
>
> Since dma_buf_map_attachment() returns a sg_table,

Please don't assume that this is an sg_table. We just used it as 
container for DMA addresses, but this has proven to be a mistake.

There is work underway to replace the sg_table with (for example) just 
an array of DMA addresses.

> I basically have two options, and I can't decide which one is the best (or the less ugly):
>
> - Either I add a new API function similar to dmaengine_prep_slave_sg(),
> which still takes a scatterlist as argument but also takes the number
> of bytes as argument;
>
> - Or I add a function to duplicate the scatterlist and then shrink it
> manually, which doesn't sound like a good idea either.
>
> What would be the recommended way?

I strongly recommend to come up with a new function which only takes DMA 
addresses and separate segment length.

Regards,
Christian.

>
> Cheers,
> -Paul

Re: Question: partial transfers of DMABUFs

[Paul Cercueil]

Le mercredi 15 février 2023 à 13:13 +0100, Maarten Lankhorst a écrit :
> 
> On 2023-02-15 13:00, Paul Cercueil wrote:
> > Hi Maarten,
> > 
> > Le mercredi 15 février 2023 à 12:52 +0100, Maarten Lankhorst a
> > écrit :
> > > Hey,
> > > 
> > > On 2023-02-15 12:47, Paul Cercueil wrote:
> > > > Hi Maarten,
> > > > 
> > > > Le mercredi 15 février 2023 à 12:30 +0100, Maarten Lankhorst a
> > > > écrit :
> > > > > Hey,
> > > > > 
> > > > > On 2023-02-15 11:48, Paul Cercueil wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > I am working on adding support for DMABUFs in the IIO
> > > > > > subsystem.
> > > > > > 
> > > > > > One thing we want there, is the ability to specify the
> > > > > > number
> > > > > > of
> > > > > > bytes
> > > > > > to transfer (while still defaulting to the DMABUF size).
> > > > > > 
> > > > > > Since dma_buf_map_attachment() returns a sg_table, I
> > > > > > basically
> > > > > > have
> > > > > > two
> > > > > > options, and I can't decide which one is the best (or the
> > > > > > less
> > > > > > ugly):
> > > > > > 
> > > > > > - Either I add a new API function similar to
> > > > > > dmaengine_prep_slave_sg(),
> > > > > > which still takes a scatterlist as argument but also takes
> > > > > > the
> > > > > > number
> > > > > > of bytes as argument;
> > > > > > 
> > > > > > - Or I add a function to duplicate the scatterlist and then
> > > > > > shrink
> > > > > > it
> > > > > > manually, which doesn't sound like a good idea either.
> > > > > > 
> > > > > > What would be the recommended way?
> > > > > Does this need an api change? If you create a DMA-BUF of size
> > > > > X,
> > > > > it
> > > > > has
> > > > > to be of size X. You can pad with a dummy page probably if
> > > > > you
> > > > > know
> > > > > it
> > > > > in advance. But after it has been imported, it cannot change
> > > > > size.
> > > > Yes, the sizes are fixed.
> > > > 
> > > > > You don´t have to write the entire dma-buf either, so if you
> > > > > want
> > > > > to
> > > > > create a 1GB buf and only use the first 4K, that is allowed.
> > > > > The
> > > > > contents of  the remainder of the DMA-BUF are undefined. It's
> > > > > up
> > > > > to
> > > > > userspace to assign a meaning to it.
> > > > > 
> > > > > I think I'm missing something here that makes the whole
> > > > > question
> > > > > m,ake
> > > > > more sense.
> > > > I want my userspace to be able to specify how much of the
> > > > DMABUF is
> > > > to
> > > > be read from or written to.
> > > > 
> > > > So in my new "dmabuf enqueue" IOCTL that I want to add to IIO,
> > > > I
> > > > added
> > > > a parameter to specify the number of bytes to transfer (where 0
> > > > means
> > > > the whole buffer).
> > > > 
> > > > The problem I have now, is that the current dmaengine core does
> > > > not
> > > > have a API function that takes a scatterlist (returned by
> > > > dma_map_attachment()) and a transfer size in bytes, it will
> > > > always
> > > > transfer the whole scatterlist.
> > > > 
> > > > So my two options would be to add a new API function to support
> > > > specifying a bytes count, or add a mechanism to duplicate a
> > > > scatterlist, so that I can tweak it to the right size.
> > > This doesn't have to happen through DMA-BUF. Presumably you are
> > > both
> > > the
> > > importer and the exporter, so after you know how much is read,
> > > you
> > > can
> > > tell this to the importer that X number of bytes can be read from
> > > DMA-BUF Y.
> > Yes, I do that already as it is an argument in my ioctl.
> > 
> > > In your case, when enqueing you will get a full SG list, but if
> > > you
> > > know
> > > only X bytes are read/written you only have to map the first X
> > > bytes
> > > to
> > > your IIO device. The rest of the SG list could be ignored safely.
> > Yes. But I don't know how to "ignore the rest of the SG list".
> > 
> > - dma_buf_map_attachment() does not have a parameter to specify
> > that I
> > only need the first X bytes mapped;
> > 
> > - if I map the whole thing, dmaengine_prep_slave_sg() does not have
> > an
> > option to specify that I only want the first X bytes transferred.
> 
> sg_split apppears to allow you to split it? I'm not 100% sure whether
> it 
> leaves the original SG untouched, but you can try to put it in
> between 
> those 2 calls to get a smaller SG to pass to prep_slave_sg.

I overlooked sg_split. It looks like it could work for me.

Thanks!

Cheers,
-Paul

Re: Question: partial transfers of DMABUFs

[Paul Cercueil]

Hi Christian,

Le mercredi 15 février 2023 à 13:58 +0100, Christian König a écrit :
> Hi Paul,
> 
> Am 15.02.23 um 11:48 schrieb Paul Cercueil:
> > Hi,
> > 
> > I am working on adding support for DMABUFs in the IIO subsystem.
> > 
> > One thing we want there, is the ability to specify the number of
> > bytes
> > to transfer (while still defaulting to the DMABUF size).
> > 
> > Since dma_buf_map_attachment() returns a sg_table,
> 
> Please don't assume that this is an sg_table. We just used it as 
> container for DMA addresses, but this has proven to be a mistake.

TL/DR, why was it a mistake? Just curious.

> There is work underway to replace the sg_table with (for example)
> just 
> an array of DMA addresses.

Ok, so I believe at some point we will need an equivalent of
dmaengine_prep_slave_sg() which takes an array of DMA addresses.

> > I basically have two options, and I can't decide which one is the
> > best (or the less ugly):
> > 
> > - Either I add a new API function similar to
> > dmaengine_prep_slave_sg(),
> > which still takes a scatterlist as argument but also takes the
> > number
> > of bytes as argument;
> > 
> > - Or I add a function to duplicate the scatterlist and then shrink
> > it
> > manually, which doesn't sound like a good idea either.
> > 
> > What would be the recommended way?
> 
> I strongly recommend to come up with a new function which only takes
> DMA 
> addresses and separate segment length.

Alright, thanks for your input.

So I would add a new dma_device.dma_prep_slave_dma_array() callback
with a corresponding API function, and then the drivers can be
converted from using .dma_prep_slave_sg() to this new function in due
time.

Vinod, that works for you?

Cheers,
-Paul

Re: Question: partial transfers of DMABUFs

[Paul Cercueil]

Hi Christian,

Le mercredi 15 février 2023 à 13:58 +0100, Christian König a écrit :
> Hi Paul,
> 
> Am 15.02.23 um 11:48 schrieb Paul Cercueil:
> > Hi,
> > 
> > I am working on adding support for DMABUFs in the IIO subsystem.
> > 
> > One thing we want there, is the ability to specify the number of
> > bytes
> > to transfer (while still defaulting to the DMABUF size).
> > 
> > Since dma_buf_map_attachment() returns a sg_table,
> 
> Please don't assume that this is an sg_table. We just used it as 
> container for DMA addresses, but this has proven to be a mistake.

TL/DR, why was it a mistake? Just curious.

> There is work underway to replace the sg_table with (for example)
> just 
> an array of DMA addresses.

Ok, so I believe at some point we will need an equivalent of
dmaengine_prep_slave_sg() which takes an array of DMA addresses.

> > I basically have two options, and I can't decide which one is the
> > best (or the less ugly):
> > 
> > - Either I add a new API function similar to
> > dmaengine_prep_slave_sg(),
> > which still takes a scatterlist as argument but also takes the
> > number
> > of bytes as argument;
> > 
> > - Or I add a function to duplicate the scatterlist and then shrink
> > it
> > manually, which doesn't sound like a good idea either.
> > 
> > What would be the recommended way?
> 
> I strongly recommend to come up with a new function which only takes
> DMA 
> addresses and separate segment length.

Alright, thanks for your input.

So I would add a new dma_device.dma_prep_slave_dma_array() callback
with a corresponding API function, and then the drivers can be
converted from using .dma_prep_slave_sg() to this new function in due
time.

Vinod, that works for you?

Cheers,
-Paul

Re: Question: partial transfers of DMABUFs

[Christian König]

Am 15.02.23 um 14:24 schrieb Paul Cercueil:
> Hi Christian,
>
> Le mercredi 15 février 2023 à 13:58 +0100, Christian König a écrit :
>> Hi Paul,
>>
>> Am 15.02.23 um 11:48 schrieb Paul Cercueil:
>>> Hi,
>>>
>>> I am working on adding support for DMABUFs in the IIO subsystem.
>>>
>>> One thing we want there, is the ability to specify the number of
>>> bytes
>>> to transfer (while still defaulting to the DMABUF size).
>>>
>>> Since dma_buf_map_attachment() returns a sg_table,
>> Please don't assume that this is an sg_table. We just used it as
>> container for DMA addresses, but this has proven to be a mistake.
> TL/DR, why was it a mistake? Just curious.

The sg_table should have just contained DMA addresses, but we had 
multiple people who tried to use the pages instead.

This works to some extend, but goes boom as soon as somebody messes with 
the pages reference counts or tries to map it into an address space or 
something like that.

We got so far that we now intentionally mangle the page addresses in the 
sg_table to prevent people from using it: 
https://elixir.bootlin.com/linux/latest/source/drivers/dma-buf/dma-buf.c#L763

>> There is work underway to replace the sg_table with (for example)
>> just
>> an array of DMA addresses.
> Ok, so I believe at some point we will need an equivalent of
> dmaengine_prep_slave_sg() which takes an array of DMA addresses.

Well we will probably come up with a new container for this, but yeah.

Regards,
Christian.

>
>>> I basically have two options, and I can't decide which one is the
>>> best (or the less ugly):
>>>
>>> - Either I add a new API function similar to
>>> dmaengine_prep_slave_sg(),
>>> which still takes a scatterlist as argument but also takes the
>>> number
>>> of bytes as argument;
>>>
>>> - Or I add a function to duplicate the scatterlist and then shrink
>>> it
>>> manually, which doesn't sound like a good idea either.
>>>
>>> What would be the recommended way?
>> I strongly recommend to come up with a new function which only takes
>> DMA
>> addresses and separate segment length.
> Alright, thanks for your input.
>
> So I would add a new dma_device.dma_prep_slave_dma_array() callback
> with a corresponding API function, and then the drivers can be
> converted from using .dma_prep_slave_sg() to this new function in due
> time.
>
> Vinod, that works for you?
>
> Cheers,
> -Paul

Re: Question: partial transfers of DMABUFs

[Christian König]

Am 15.02.23 um 14:24 schrieb Paul Cercueil:
> Hi Christian,
>
> Le mercredi 15 février 2023 à 13:58 +0100, Christian König a écrit :
>> Hi Paul,
>>
>> Am 15.02.23 um 11:48 schrieb Paul Cercueil:
>>> Hi,
>>>
>>> I am working on adding support for DMABUFs in the IIO subsystem.
>>>
>>> One thing we want there, is the ability to specify the number of
>>> bytes
>>> to transfer (while still defaulting to the DMABUF size).
>>>
>>> Since dma_buf_map_attachment() returns a sg_table,
>> Please don't assume that this is an sg_table. We just used it as
>> container for DMA addresses, but this has proven to be a mistake.
> TL/DR, why was it a mistake? Just curious.

The sg_table should have just contained DMA addresses, but we had 
multiple people who tried to use the pages instead.

This works to some extend, but goes boom as soon as somebody messes with 
the pages reference counts or tries to map it into an address space or 
something like that.

We got so far that we now intentionally mangle the page addresses in the 
sg_table to prevent people from using it: 
https://elixir.bootlin.com/linux/latest/source/drivers/dma-buf/dma-buf.c#L763

>> There is work underway to replace the sg_table with (for example)
>> just
>> an array of DMA addresses.
> Ok, so I believe at some point we will need an equivalent of
> dmaengine_prep_slave_sg() which takes an array of DMA addresses.

Well we will probably come up with a new container for this, but yeah.

Regards,
Christian.

>
>>> I basically have two options, and I can't decide which one is the
>>> best (or the less ugly):
>>>
>>> - Either I add a new API function similar to
>>> dmaengine_prep_slave_sg(),
>>> which still takes a scatterlist as argument but also takes the
>>> number
>>> of bytes as argument;
>>>
>>> - Or I add a function to duplicate the scatterlist and then shrink
>>> it
>>> manually, which doesn't sound like a good idea either.
>>>
>>> What would be the recommended way?
>> I strongly recommend to come up with a new function which only takes
>> DMA
>> addresses and separate segment length.
> Alright, thanks for your input.
>
> So I would add a new dma_device.dma_prep_slave_dma_array() callback
> with a corresponding API function, and then the drivers can be
> converted from using .dma_prep_slave_sg() to this new function in due
> time.
>
> Vinod, that works for you?
>
> Cheers,
> -Paul

Re: Question: partial transfers of DMABUFs

[Paul Cercueil]

Le mercredi 15 février 2023 à 14:46 +0100, Christian König a écrit :
> Am 15.02.23 um 14:24 schrieb Paul Cercueil:
> > Hi Christian,
> > 
> > Le mercredi 15 février 2023 à 13:58 +0100, Christian König a
> > écrit :
> > > Hi Paul,
> > > 
> > > Am 15.02.23 um 11:48 schrieb Paul Cercueil:
> > > > Hi,
> > > > 
> > > > I am working on adding support for DMABUFs in the IIO
> > > > subsystem.
> > > > 
> > > > One thing we want there, is the ability to specify the number
> > > > of
> > > > bytes
> > > > to transfer (while still defaulting to the DMABUF size).
> > > > 
> > > > Since dma_buf_map_attachment() returns a sg_table,
> > > Please don't assume that this is an sg_table. We just used it as
> > > container for DMA addresses, but this has proven to be a mistake.
> > TL/DR, why was it a mistake? Just curious.
> 
> The sg_table should have just contained DMA addresses, but we had 
> multiple people who tried to use the pages instead.
> 
> This works to some extend, but goes boom as soon as somebody messes
> with 
> the pages reference counts or tries to map it into an address space
> or 
> something like that.
> 
> We got so far that we now intentionally mangle the page addresses in
> the 
> sg_table to prevent people from using it: 
> https://elixir.bootlin.com/linux/latest/source/drivers/dma-buf/dma-buf.c#L763

Isn't that breaking the chains though? I'd expect page_link to be
mangled only if !sg_is_chain(sg).

> > > There is work underway to replace the sg_table with (for example)
> > > just
> > > an array of DMA addresses.
> > Ok, so I believe at some point we will need an equivalent of
> > dmaengine_prep_slave_sg() which takes an array of DMA addresses.
> 
> Well we will probably come up with a new container for this, but
> yeah.

Understood.

You said there was work underway, could you point me to the
corresponding mailing list threads and/or code?

> Regards,
> Christian.

Cheers,
-Paul

> > 
> > > > I basically have two options, and I can't decide which one is
> > > > the
> > > > best (or the less ugly):
> > > > 
> > > > - Either I add a new API function similar to
> > > > dmaengine_prep_slave_sg(),
> > > > which still takes a scatterlist as argument but also takes the
> > > > number
> > > > of bytes as argument;
> > > > 
> > > > - Or I add a function to duplicate the scatterlist and then
> > > > shrink
> > > > it
> > > > manually, which doesn't sound like a good idea either.
> > > > 
> > > > What would be the recommended way?
> > > I strongly recommend to come up with a new function which only
> > > takes
> > > DMA
> > > addresses and separate segment length.
> > Alright, thanks for your input.
> > 
> > So I would add a new dma_device.dma_prep_slave_dma_array() callback
> > with a corresponding API function, and then the drivers can be
> > converted from using .dma_prep_slave_sg() to this new function in
> > due
> > time.
> > 
> > Vinod, that works for you?
> > 
> > Cheers,
> > -Paul
> 

Re: Question: partial transfers of DMABUFs

[Paul Cercueil]

Le mercredi 15 février 2023 à 14:46 +0100, Christian König a écrit :
> Am 15.02.23 um 14:24 schrieb Paul Cercueil:
> > Hi Christian,
> > 
> > Le mercredi 15 février 2023 à 13:58 +0100, Christian König a
> > écrit :
> > > Hi Paul,
> > > 
> > > Am 15.02.23 um 11:48 schrieb Paul Cercueil:
> > > > Hi,
> > > > 
> > > > I am working on adding support for DMABUFs in the IIO
> > > > subsystem.
> > > > 
> > > > One thing we want there, is the ability to specify the number
> > > > of
> > > > bytes
> > > > to transfer (while still defaulting to the DMABUF size).
> > > > 
> > > > Since dma_buf_map_attachment() returns a sg_table,
> > > Please don't assume that this is an sg_table. We just used it as
> > > container for DMA addresses, but this has proven to be a mistake.
> > TL/DR, why was it a mistake? Just curious.
> 
> The sg_table should have just contained DMA addresses, but we had 
> multiple people who tried to use the pages instead.
> 
> This works to some extend, but goes boom as soon as somebody messes
> with 
> the pages reference counts or tries to map it into an address space
> or 
> something like that.
> 
> We got so far that we now intentionally mangle the page addresses in
> the 
> sg_table to prevent people from using it: 
> https://elixir.bootlin.com/linux/latest/source/drivers/dma-buf/dma-buf.c#L763

Isn't that breaking the chains though? I'd expect page_link to be
mangled only if !sg_is_chain(sg).

> > > There is work underway to replace the sg_table with (for example)
> > > just
> > > an array of DMA addresses.
> > Ok, so I believe at some point we will need an equivalent of
> > dmaengine_prep_slave_sg() which takes an array of DMA addresses.
> 
> Well we will probably come up with a new container for this, but
> yeah.

Understood.

You said there was work underway, could you point me to the
corresponding mailing list threads and/or code?

> Regards,
> Christian.

Cheers,
-Paul

> > 
> > > > I basically have two options, and I can't decide which one is
> > > > the
> > > > best (or the less ugly):
> > > > 
> > > > - Either I add a new API function similar to
> > > > dmaengine_prep_slave_sg(),
> > > > which still takes a scatterlist as argument but also takes the
> > > > number
> > > > of bytes as argument;
> > > > 
> > > > - Or I add a function to duplicate the scatterlist and then
> > > > shrink
> > > > it
> > > > manually, which doesn't sound like a good idea either.
> > > > 
> > > > What would be the recommended way?
> > > I strongly recommend to come up with a new function which only
> > > takes
> > > DMA
> > > addresses and separate segment length.
> > Alright, thanks for your input.
> > 
> > So I would add a new dma_device.dma_prep_slave_dma_array() callback
> > with a corresponding API function, and then the drivers can be
> > converted from using .dma_prep_slave_sg() to this new function in
> > due
> > time.
> > 
> > Vinod, that works for you?
> > 
> > Cheers,
> > -Paul
> 

Re: Question: partial transfers of DMABUFs

[Christian König]

Am 15.02.23 um 14:52 schrieb Paul Cercueil:
> Le mercredi 15 février 2023 à 14:46 +0100, Christian König a écrit :
>> Am 15.02.23 um 14:24 schrieb Paul Cercueil:
>>> Hi Christian,
>>>
>>> Le mercredi 15 février 2023 à 13:58 +0100, Christian König a
>>> écrit :
>>>> Hi Paul,
>>>>
>>>> Am 15.02.23 um 11:48 schrieb Paul Cercueil:
>>>>> Hi,
>>>>>
>>>>> I am working on adding support for DMABUFs in the IIO
>>>>> subsystem.
>>>>>
>>>>> One thing we want there, is the ability to specify the number
>>>>> of
>>>>> bytes
>>>>> to transfer (while still defaulting to the DMABUF size).
>>>>>
>>>>> Since dma_buf_map_attachment() returns a sg_table,
>>>> Please don't assume that this is an sg_table. We just used it as
>>>> container for DMA addresses, but this has proven to be a mistake.
>>> TL/DR, why was it a mistake? Just curious.
>> The sg_table should have just contained DMA addresses, but we had
>> multiple people who tried to use the pages instead.
>>
>> This works to some extend, but goes boom as soon as somebody messes
>> with
>> the pages reference counts or tries to map it into an address space
>> or
>> something like that.
>>
>> We got so far that we now intentionally mangle the page addresses in
>> the
>> sg_table to prevent people from using it:
>> https://elixir.bootlin.com/linux/latest/source/drivers/dma-buf/dma-buf.c#L763
> Isn't that breaking the chains though? I'd expect page_link to be
> mangled only if !sg_is_chain(sg).

Those are filtered out by for_each_sgtable_sg if I'm not completely 
mistaken.

>>>> There is work underway to replace the sg_table with (for example)
>>>> just
>>>> an array of DMA addresses.
>>> Ok, so I believe at some point we will need an equivalent of
>>> dmaengine_prep_slave_sg() which takes an array of DMA addresses.
>> Well we will probably come up with a new container for this, but
>> yeah.
> Understood.
>
> You said there was work underway, could you point me to the
> corresponding mailing list threads and/or code?

That's not really released yet. We just discussed it a bit when Daniel 
added the sg_table mangling after this went boom for the third time so :)

Just use git blame to find the patch of the mangling and read up on the 
mailing list discussion around that.

Regards,
Christian.

>
>> Regards,
>> Christian.
> Cheers,
> -Paul
>
>>>>> I basically have two options, and I can't decide which one is
>>>>> the
>>>>> best (or the less ugly):
>>>>>
>>>>> - Either I add a new API function similar to
>>>>> dmaengine_prep_slave_sg(),
>>>>> which still takes a scatterlist as argument but also takes the
>>>>> number
>>>>> of bytes as argument;
>>>>>
>>>>> - Or I add a function to duplicate the scatterlist and then
>>>>> shrink
>>>>> it
>>>>> manually, which doesn't sound like a good idea either.
>>>>>
>>>>> What would be the recommended way?
>>>> I strongly recommend to come up with a new function which only
>>>> takes
>>>> DMA
>>>> addresses and separate segment length.
>>> Alright, thanks for your input.
>>>
>>> So I would add a new dma_device.dma_prep_slave_dma_array() callback
>>> with a corresponding API function, and then the drivers can be
>>> converted from using .dma_prep_slave_sg() to this new function in
>>> due
>>> time.
>>>
>>> Vinod, that works for you?
>>>
>>> Cheers,
>>> -Paul

Re: Question: partial transfers of DMABUFs

[Christian König]

Am 15.02.23 um 14:52 schrieb Paul Cercueil:
> Le mercredi 15 février 2023 à 14:46 +0100, Christian König a écrit :
>> Am 15.02.23 um 14:24 schrieb Paul Cercueil:
>>> Hi Christian,
>>>
>>> Le mercredi 15 février 2023 à 13:58 +0100, Christian König a
>>> écrit :
>>>> Hi Paul,
>>>>
>>>> Am 15.02.23 um 11:48 schrieb Paul Cercueil:
>>>>> Hi,
>>>>>
>>>>> I am working on adding support for DMABUFs in the IIO
>>>>> subsystem.
>>>>>
>>>>> One thing we want there, is the ability to specify the number
>>>>> of
>>>>> bytes
>>>>> to transfer (while still defaulting to the DMABUF size).
>>>>>
>>>>> Since dma_buf_map_attachment() returns a sg_table,
>>>> Please don't assume that this is an sg_table. We just used it as
>>>> container for DMA addresses, but this has proven to be a mistake.
>>> TL/DR, why was it a mistake? Just curious.
>> The sg_table should have just contained DMA addresses, but we had
>> multiple people who tried to use the pages instead.
>>
>> This works to some extend, but goes boom as soon as somebody messes
>> with
>> the pages reference counts or tries to map it into an address space
>> or
>> something like that.
>>
>> We got so far that we now intentionally mangle the page addresses in
>> the
>> sg_table to prevent people from using it:
>> https://elixir.bootlin.com/linux/latest/source/drivers/dma-buf/dma-buf.c#L763
> Isn't that breaking the chains though? I'd expect page_link to be
> mangled only if !sg_is_chain(sg).

Those are filtered out by for_each_sgtable_sg if I'm not completely 
mistaken.

>>>> There is work underway to replace the sg_table with (for example)
>>>> just
>>>> an array of DMA addresses.
>>> Ok, so I believe at some point we will need an equivalent of
>>> dmaengine_prep_slave_sg() which takes an array of DMA addresses.
>> Well we will probably come up with a new container for this, but
>> yeah.
> Understood.
>
> You said there was work underway, could you point me to the
> corresponding mailing list threads and/or code?

That's not really released yet. We just discussed it a bit when Daniel 
added the sg_table mangling after this went boom for the third time so :)

Just use git blame to find the patch of the mangling and read up on the 
mailing list discussion around that.

Regards,
Christian.

>
>> Regards,
>> Christian.
> Cheers,
> -Paul
>
>>>>> I basically have two options, and I can't decide which one is
>>>>> the
>>>>> best (or the less ugly):
>>>>>
>>>>> - Either I add a new API function similar to
>>>>> dmaengine_prep_slave_sg(),
>>>>> which still takes a scatterlist as argument but also takes the
>>>>> number
>>>>> of bytes as argument;
>>>>>
>>>>> - Or I add a function to duplicate the scatterlist and then
>>>>> shrink
>>>>> it
>>>>> manually, which doesn't sound like a good idea either.
>>>>>
>>>>> What would be the recommended way?
>>>> I strongly recommend to come up with a new function which only
>>>> takes
>>>> DMA
>>>> addresses and separate segment length.
>>> Alright, thanks for your input.
>>>
>>> So I would add a new dma_device.dma_prep_slave_dma_array() callback
>>> with a corresponding API function, and then the drivers can be
>>> converted from using .dma_prep_slave_sg() to this new function in
>>> due
>>> time.
>>>
>>> Vinod, that works for you?
>>>
>>> Cheers,
>>> -Paul