Message-ID: <9daab4a1-0282-4168-becc-f5563f49f213@amd.com>
Date:   Wed, 15 Feb 2023 14:56:09 +0100
From:   Christian König <christian.koenig@....com>
To:     Paul Cercueil <paul@...pouillou.net>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Vinod Koul <vkoul@...nel.org>
Cc:     linux-media@...r.kernel.org, dmaengine@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: Question: partial transfers of DMABUFs

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

Powered by blists - more mailing lists