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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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&data=04%7C01%7Cchristian.koenig%40amd.com%7Cb36def4a6ebd4879731c08d9e1753ccd%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637788715933199161%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=gwW05OaUq%2FxlBWnY%2FPuPfl0YDdKp5VTbllaSmn45nE8%3D&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





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux