Messages in this thread | | | Date | Thu, 27 Jan 2022 10:21:53 +0100 | Subject | Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map | From | Christian König <> |
| |
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 >>
| |