Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map

From: "Christian König" <christian.koenig@amd.com>
To: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: linaro-mm-sig@lists.linaro.org, intel-gfx@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linux-media@vger.kernel.org,
	Thomas Zimmermann <tzimmermann@suse.de>
Subject: Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map
Date: Thu, 27 Jan 2022 10:21:53 +0100	[thread overview]
Message-ID: <69c260e4-f4b8-e51c-4930-e7aaf9bec2b9@amd.com> (raw)
In-Reply-To: <20220127091247.3uqi5zhesqtecbsw@ldmartin-desk2>

Am 27.01.22 um 10:12 schrieb Lucas De Marchi:
> On Thu, Jan 27, 2022 at 09:55:05AM +0100, Christian König wrote:
>> Am 27.01.22 um 09:18 schrieb Lucas De Marchi:
>>> On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
>>>> Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
>>>>> On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König wrote:
>>>>>> Am 26.01.22 um 21:36 schrieb Lucas De Marchi:
>>>>>>> [SNIP]
>>>>> humn... not sure if I was  clear. There is no importer and 
>>>>> exporter here.
>>>>
>>>> Yeah, and exactly that's what I'm pointing out as problem here.
>>>>
>>>> You are using the inter driver framework for something internal to 
>>>> the driver. That is an absolutely clear NAK!
>>>>
>>>> We could discuss that, but you guys are just sending around patches 
>>>> to do this without any consensus that this is a good idea.
>>>
>>> s/you guys/you/ if you have to blame anyone - I'm the only s-o-b in
>>> these patches. I'm sending these to _build consensus_ on what may be 
>>> a good
>>> use for it showing a real problem it's helping to fix.
>>
>> Well a cover letter would have been helpful, my impression was that 
>> you have a larger set and just want to upstream some minor DMA-buf 
>> changes necessary for it.
>
> I missed adding this sentence to the cover letter, as my impression 
> was that
> dma-buf-map was already used outside inter-driver framework. But there
> is actually a cover letter:
>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20220126203702.1784589-1-lucas.demarchi%40intel.com%2F&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7Cb36def4a6ebd4879731c08d9e1753ccd%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637788715933199161%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=gwW05OaUq%2FxlBWnY%2FPuPfl0YDdKp5VTbllaSmn45nE8%3D&amp;reserved=0 
>
>
> And looking at it now, it seems I missed adding Thomas Zimmermann to Cc.
>
>>
>> Now I know why people are bugging me all the time to add cover 
>> letters to add more context to my sets.
>>
>>>
>>> From its documentation:
>>>
>>>  * The type :c:type:`struct dma_buf_map <dma_buf_map>` and its 
>>> helpers are
>>>  * actually independent from the dma-buf infrastructure. When 
>>> sharing buffers
>>>  * among devices, drivers have to know the location of the memory to 
>>> access
>>>  * the buffers in a safe way. :c:type:`struct dma_buf_map 
>>> <dma_buf_map>`
>>>  * solves this problem for dma-buf and its users. If other drivers or
>>>  * sub-systems require similar functionality, the type could be 
>>> generalized
>>>  * and moved to a more prominent header file.
>>>
>>> if there is no consensus and a better alternative, I'm perfectly 
>>> fine in
>>> throwing it out and using the better approach.
>>
>> When Thomas Zimmermann upstreamed the dma_buf_map work we had a 
>> discussion if that shouldn't be independent of the DMA-buf framework.
>>
>> The consensus was that as soon as we have more widely use for it this 
>> should be made independent. So basically that is what's happening now.
>>
>> I suggest the following approach:
>> 1. Find a funky name for this, something like iomem_, kiomap_ or 
>> similar.
>
> iosys_map?

Works for me.

>
>> 2. Separate this from all you driver dependent work and move the 
>> dma_buf_map structure out of DMA-buf into this new whatever_ prefix.
>
> should this be a follow up to the driver work or a prerequisite?

Prerequisite. Structural changes like this always separate to the 
actually work switching over to them because the later needs a much 
fewer audience for review.

Regards,
Christian.

>
> thanks
> Lucas De Marchi
>
>> 3. Ping Thomas, LKML, me and probably a couple of other core people 
>> if this is the right idea or not.
>> 4. Work on dropping the map parameter from dma_buf_vunmap(). This is 
>> basically why we can't modify the pointers returned from 
>> dma_buf_vmap() and has already cause a few problems with 
>> dma_buf_map_incr().
>>
>> Regards,
>> Christian.
>>
>>>
>>> Lucas De Marchi
>>