Message-ID: <9daab4a1-0282-4168-becc-f5563f49f213@amd.com>
Date:   Wed, 15 Feb 2023 14:56:09 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: Question: partial transfers of DMABUFs
Content-Language: en-US
To:     Paul Cercueil <paul@crapouillou.net>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Vinod Koul <vkoul@kernel.org>
Cc:     linux-media@vger.kernel.org, dmaengine@vger.kernel.org,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
References: <53ea4d2db570d3ca514a69015488bd5b849a5193.camel@crapouillou.net>
 <c5fe8089-49e9-2bec-eac5-e8a035412cf3@amd.com>
 <db4d83f3c3c8dc4f3ef5f850fe4c1c04aa648d47.camel@crapouillou.net>
 <bf3771bc-bbdc-8a9f-b9e7-425812a66ea9@amd.com>
 <0a128bd44ee7f33369b19b1e984c9e6ffc926ae4.camel@crapouillou.net>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <0a128bd44ee7f33369b19b1e984c9e6ffc926ae4.camel@crapouillou.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VzhXYlRhL1NwbmpSbkN1akIvbEk1Z2dXb2lYL1hTeEZFSi9JUk1TNzhHdDZF?=
 =?utf-8?B?WVVMb3Z6WVlMRzBvbjlUVmR0MHBSTERmY3FMZ0JnQnQ1VFcvbm1SYkphdzhI?=
 =?utf-8?B?aEVmb21saEFqR1VMYUdZaXI4SVB3czVwaXRab3FZWEFVdDJVZ1JabVg2Z0lo?=
 =?utf-8?B?UldzaEhEQmp5T2tlUXFFNFhuVTVSWitBWTY3aDRuOWdXQldBLzBjN3dRYzFM?=
 =?utf-8?B?eVdZUkZXRnBheTZkWG1Oa1lKYlN2bmU1bnZnbGQ4MmtmZXdicXF6N0ZFVEdP?=
 =?utf-8?B?QzJwdkpCNmpxNXZtSnIxbjZldWh6OHRYRHVKWVFidzRnT3JldUpWZ0wveDhl?=
 =?utf-8?B?b3EyUjFoTFRkOHh3MzBOWTkyM2ROUklZSmF3eWE2bUxDdldKNnhma2p1SjNw?=
 =?utf-8?B?YzJjK3RDV2FpQUxhR29IWmk1NHVicGhtdi9weHZkRWRNWURBUjF0NnNxcU9h?=
 =?utf-8?B?bU1hWStMQXp0eUtMZUcrcS9Jak4xUk1CTzhZOFJaazBmV2krMHl1UE5sN0Fw?=
 =?utf-8?B?MmEvdlZCSzVZbFJYekdVS3ltSGY2TzVhSEhncW52SmRDUXc1cDBrVVgwOXJa?=
 =?utf-8?B?RmxGWFRaZDJOMGg3T2l1VFlVb3cwb0RPK0E4N1pJWWdvQ0J1TnNXUGdrT0FZ?=
 =?utf-8?B?M3pYQnhWNFhhV1hVV0gwcVVPdkNCNVEvbDJ5ZDdaNEdMMGVGd0hyUVlmNEU2?=
 =?utf-8?B?aVR6cGdaN0tINXJPUE9heDFtTTFmemFBSlNRdW9wS3BuZExQdmtSNnlLZkt2?=
 =?utf-8?B?U0FqbWplbk45RzQxaWtGM29pdFNSQXd1SEdobW1SZ1hicnpLNFN2Ty9EY1pY?=
 =?utf-8?B?SU9YeEJHQWxFSEp2UTdZZEpxRFRTb3k2UlhMMGZFN0N6bHBZTnJlUHlvS1Rn?=
 =?utf-8?B?SXh2SmdJWjc5Z3JINUpmTTg1emU3NzZyNkVBNGIrNUNrSjAyUUhPVzB1bndv?=
 =?utf-8?B?VW1MMXUxUUlmMklEVms0MkxReTduY0hNcUxPYjRUUmlsZEZWd2NpOE0rNTRI?=
 =?utf-8?B?YlpZeUk3YUlSL1o1MFVIZi9tRkNuWXU3aHoyc2tjMGY1UGk0d2FlT3k4WlZt?=
 =?utf-8?B?bXVHckFBRTRKcXFNSXpzTlV2QjRBN0E5eitmbHdKTDN3UWlXczZCQmlYdkx0?=
 =?utf-8?B?eU85VFpyeFJQQmdwY0dxc1FwNzRhbkFxZzd2V2Y0MW9XbTNkNTlsSmhhTkt4?=
 =?utf-8?B?TGdkZmlocjhzRldEWDhrMjJpNzFtWG1sWnEza1JoRnFISnVmVWEycEhoTDBX?=
 =?utf-8?B?Wm0rN1YweHU2S09YRWNxVExOK1B2SnhJOGdXMHJxdlJnNHcxdmxocFhGaGVx?=
 =?utf-8?B?M2YwMnlKK0IybW9UV0J3MWdML2U1d1ZkTlROWUNnZnNGYWVGUTBtYThveWlW?=
 =?utf-8?B?bUlLN2wrc3pDRk5nTUhHR2JNbkNNR1VRdlprS1QzY1EyM2QvcXZndGFnSkcy?=
 =?utf-8?B?VktyLzl4aEdaZHcxQVFydlZ5OHJnRk1VSURsN2hQaDc1anZ6UUNEOERPelFt?=
 =?utf-8?B?UENibm10cms2bko3aVlrWWJSbEU0SjFuOXdtRjBZdWFncjdoNW05cWh5Z2dm?=
 =?utf-8?B?WFNpUDRoY25URWNkeXNEMjJSRmpVSjR5NENKYWFkbG5WbkpyN3lXRHVQcTVG?=
 =?utf-8?B?a28vSWt3WXFhN1BlNTlQQ3VmZ2JEMG1FeTRMQkJYTTdKcmw0T0h1eEltLzFL?=
 =?utf-8?B?UzU4Y1Z4QTJSVTBzTVNpS0dhZnByNGk4ZjA0Z2VJQm1tWHJTR0g3bFNQWVgz?=
 =?utf-8?B?cDRVY3U5OVd0ZGRrTVFZYVIzN2wvQytBZ3dnSkZqdnY3cURZUzZFcE5DZzhJ?=
 =?utf-8?B?YmJ2K2xlTTVBejg2N1o0Z3NIRzdnc20wbUZiUGp1VXU3aGRhMXRWVEV5UGp6?=
 =?utf-8?B?bHJrbW5UbEtzTEZYRXA0NzEvZk9LeVhrZlo5cnJ0eUNPR3RzMzlibkllMTha?=
 =?utf-8?B?dCszNC9wZmNTMmt0a082aHl4SlBQYWRpc1prQnYxcGpIMXlGMzYzeDZ0cEFh?=
 =?utf-8?B?d29uNWtKUnhlVjkzbjZqYkdBVDBDdlJCM01hYkU4ZkQ3YkxlODR5VnlZQ0VO?=
 =?utf-8?B?WG04WWVqRzlkNmlxbU01bmd6akRTUklJSWlReUJBeXowRmV6UkVJaTF5SXlW?=
 =?utf-8?B?UDBmTElTU1hFNjB0QVErWGZJVllYR2ZoSjliK3ZQcWJkaDQxUUovdlpMSHh4?=
 =?utf-8?Q?hAdGuYJBNg9DqnRfHmVhvfF0G1rp3r1Xnp2Uk4tS0yP3?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 76b86c2d-ad3c-4876-eae1-08db0f5c66a2
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Feb 2023 13:56:13.7980
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Q884Juw0gpPgZIBCFtkQ+sV67qiOESjZOcqGu6fC2F7IgwvcgcK+P6t9wNAci7sF
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR12MB5370
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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