Am 29.09.20 um 13:09 schrieb Christian König: > Am 29.09.20 um 11:14 schrieb Daniel Vetter: >> On Mon, Sep 28, 2020 at 01:22:13PM +0200, Christian König wrote: >>> Am 28.09.20 um 09:37 schrieb Thomas Zimmermann: >>>> Hi >>>> >>>> Am 28.09.20 um 08:50 schrieb Christian König: >>>>> Am 27.09.20 um 21:16 schrieb Sam Ravnborg: >>>>>> Hi Thomas. >>>>>> >>>>>>>> struct simap { >>>>>>>>           union { >>>>>>>>                   void __iomem *vaddr_iomem; >>>>>>>>                   void *vaddr; >>>>>>>>           }; >>>>>>>>           bool is_iomem; >>>>>>>> }; >>>>>>>> >>>>>>>> Where simap is a shorthand for system_iomem_map >>>>>>>> And it could al be stuffed into a include/linux/simap.h file. >>>>>>>> >>>>>>>> Not totally sold on the simap name - but wanted to come up with >>>>>>>> something. >>>>>>> Yes. Others, myself included, have suggested to use a name that >>>>>>> does not >>>>>>> imply a connection to the dma-buf framework, but no one has come >>>>>>> up with >>>>>>>     a good name. >>>>>>> >>>>>>> I strongly dislike simap, as it's entirely non-obvious what it does. >>>>>>> dma-buf-map is not actually wrong. The structures represents the >>>>>>> mapping >>>>>>> of a dma-able buffer in most cases. >>>>>>> >>>>>>>> With this approach users do not have to pull in dma-buf to use >>>>>>>> it and >>>>>>>> users will not confuse that this is only for dma-buf usage. >>>>>>> There's no need to enable dma-buf. It's all in the header file >>>>>>> without >>>>>>> dependencies on dma-buf. It's really just the name. >>>>>>> >>>>>>> But there's something else to take into account. The whole issue >>>>>>> here is >>>>>>> that the buffer is disconnected from its originating driver, so >>>>>>> we don't >>>>>>> know which kind of memory ops we have to use. Thinking about it, I >>>>>>> realized that no one else seemed to have this problem until now. >>>>>>> Otherwise there would be a solution already. So maybe the dma-buf >>>>>>> framework *is* the native use case for this data structure. >>>>>> We have at least: >>>>>> linux/fb.h: >>>>>>       union { >>>>>>           char __iomem *screen_base;      /* Virtual address */ >>>>>>           char *screen_buffer; >>>>>>       }; >>>>>> >>>>>> Which solve more or less the same problem. >>>> I thought this was for convenience. The important is_iomem bit is >>>> missing. >>>> >>>>> I also already noted that in TTM we have exactly the same problem >>>>> and a >>>>> whole bunch of helpers to allow operations on those pointers. >>>> How do you call this within TTM? >>> ttm_bus_placement, but I really don't like that name. >>> >>>> The data structure represents a pointer to either system or I/O memory, >>>> but not necessatrily device memory. It contains raw data. That would >>>> give something like >>>> >>>>     struct databuf_map >>>>     struct databuf_ptr >>>>     struct dbuf_map >>>>     struct dbuf_ptr >>>> >>>> My favorite would be dbuf_ptr. It's short and the API names would make >>>> sense: dbuf_ptr_clear() for clearing, dbuf_ptr_set_vaddr() to set an >>>> address, dbuf_ptr_incr() to increment, etc. Also, the _ptr indicates >>>> that it's a single address; not an offset with length. >>> Puh, no idea. All of that doesn't sound like it 100% hits the underlying >>> meaning of the structure. >> Imo first let's merge this and then second with more users we might come >> up with a better name. And cocci is fairly good at large-scale rename, to >> the point where we manged to rename struct fence to struct dma_fence. > > Agreed, renaming things later on is easy if somebody comes up with > something better. > > But blocking a necessary technical change just because of the naming is > usually not a good idea. OK, merged now. Best regards Thomas > > Christian. > >> >> Also this entire thread here imo shows that we haven't yet figured out >> the >> perfect name anyway, and I don't think it's worth it to hold this up >> because of this bikeshed :-) >> >> Cheers, Daniel >> >>> Christian. >>> >>>> Best regards >>>> Thomas >>>> >>>>> Christian. >>>>> >>>>>>> Anyway, if a better name than dma-buf-map comes in, I'm willing to >>>>>>> rename the thing. Otherwise I intend to merge the patchset by the >>>>>>> end of >>>>>>> the week. >>>>>> Well, the main thing is that I think this shoud be moved away from >>>>>> dma-buf. But if indeed dma-buf is the only relevant user in drm then >>>>>> I am totally fine with the current naming. >>>>>> >>>>>> One alternative named that popped up in my head: struct sys_io_map {} >>>>>> But again, if this is kept in dma-buf then I am fine with the current >>>>>> naming. >>>>>> >>>>>> In other words, if you continue to think this is mostly a dma-buf >>>>>> thing all three patches are: >>>>>> Acked-by: Sam Ravnborg >>>>>> >>>>>>       Sam >>>>> _______________________________________________ >>>>> dri-devel mailing list >>>>> dri-devel@lists.freedesktop.org >>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=02%7C01%7Cchristian.koenig%40amd.com%7C71c630a7ca1d48476eed08d864581e4f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637369676925032848&sdata=CsekzASvj2lY%2B74FIiLc9B5QG7AHma8B2M5y8Cassj4%3D&reserved=0 >>>>> > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer