Re: [Nouveau] [PATCH 00/14] Rename dma-buf-map
On Tue, Feb 01, 2022 at 08:46:15AM +0100, Christian König wrote: Am 01.02.22 um 01:36 schrieb Lucas De Marchi: On Fri, Jan 28, 2022 at 10:48:42AM +0100, Christian König wrote: Am 28.01.22 um 10:40 schrieb Lucas De Marchi: On Fri, Jan 28, 2022 at 10:22:00AM +0100, Christian König wrote: Am 28.01.22 um 10:12 schrieb Lucas De Marchi: On Fri, Jan 28, 2022 at 09:41:14AM +0100, Christian König wrote: Rule #1 is to never ever break the build. Because of this all those patches needs to be squashed into a single one as far as I can see. what config are you building on? Well I'm not building at all, I'm just looking at the patches as an engineer with 25 years of experience with Linux patches. Just take a look at patch number 2: -static int fastrpc_vmap(struct dma_buf *dmabuf, struct dma_buf_map *map) +static int fastrpc_vmap(struct dma_buf *dmabuf, struct iosys_map *map) You are changing the functions signature without changing any of the callers. At bare minimum that causes a warning and on runtime this only works by coincident now because the structure pointers just happen to have the same layout. This is not something we usually do. you missed the magic/hack on patch 1: 1) dma-buf-map.h includes iosys-map.h _at the end_ 2) iosys-map.h includes dma-buf-map.h at the beginning and initially does a "define iosys_map dma_buf_map". So, it doesn't work by coincidence, It's because it was done to allow converting it piecemeal. Oh, my. Please never do stuff like that again. It's not uncommon approach to be required by other subsystems. Even drm-intel already used similar approach for macro conversions crossing drm-intel-next and drm-intel-gt-next branches recently. As I said, I don't mind one way or the other. The key point is that you seemed to have a misunderstanding why we separate changes into functional independent patches. The goal of that is *not* to reduce the number of lines in a patch, but rather to reduce the complexity of the review. When you do an automated renamed with a cocci or sed script you can have a 100k line patch as result, which is perfectly fine to send out like this as long as you include the script/commands used to autogenerate the patch. The background is that everybody on the planet can generate the patch with those commands himself and see if the results matches your patch no, as I said in the cover letter there were tweaks needed. or not. The maintainer of the component can then just puts an Acked-by on the patch and move on, but separating the patch causes additional work for both you as well as the reviewers. Separating the change into individual patches as much as possible is nice to have when you do a functional change and want or need a review from each individual driver maintainer. This is usually the default case, so sticking with separated changes as much as possible is usually still the best practice. Not sure if I should continue replying on why I split these specific patches. I even mentioned in this cover letter about squashing everything in a single patch, and I'm fine with that. Anyway, there are other reasons to split the patches when it crosses branches you don't seem to acknowledge. It's harder for maintainers of the specific branches to review/ack only the changes on their part. It's harder to find the best timing to merge it. The mega patch doesn't apply to *any* specific branch, potentially leaving silent conflicts behind. You may notice the patch was split by branch boundary for these very reasons. If maintainers prefer to have a single patch, I'm fine, I had already said that. Before I go and respin this into a single mega patch, I'd like to gather some feedback on the following topics: 1) Daniel Vetter and Thomas Zimmermann seemed to be ok with staying with the current name, dma_buf_map, while you prefer it renamed. Or at least not make the rename a pre-requisite for the API additions in https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20220126203702.1784589-1-lucas.demarchi%40intel.com%2Fdata=04%7C01%7Cchristian.koenig%40amd.com%7C01142fa3ce484040ade008d9e51aef5d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637792726123940514%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=ieMZ9Jiwo%2FQpT5kyyQgHNlepiusN%2Fkfff1Op6TVQ%2BaA%3Dreserved=0 One thing I like about the rename is that it makes clear the separation between this small shim and dma-buf. There are also some APIs that are really dma-buf API (e.g. dma_buf_map_attachment()), but if you don't look carefully you may think it's from dma_buf_map. Exactly that's the reason why I see this rename as mandatory. Adding the functionality goes beyond the inter driver interface DMA-buf provides into driver internal territory and I want to make sure that people understand just from the name alone that this is not part of DMA-buf but rather an
Re: [Nouveau] [PATCH 00/14] Rename dma-buf-map
On Fri, Jan 28, 2022 at 10:48:42AM +0100, Christian König wrote: Am 28.01.22 um 10:40 schrieb Lucas De Marchi: On Fri, Jan 28, 2022 at 10:22:00AM +0100, Christian König wrote: Am 28.01.22 um 10:12 schrieb Lucas De Marchi: On Fri, Jan 28, 2022 at 09:41:14AM +0100, Christian König wrote: Rule #1 is to never ever break the build. Because of this all those patches needs to be squashed into a single one as far as I can see. what config are you building on? Well I'm not building at all, I'm just looking at the patches as an engineer with 25 years of experience with Linux patches. Just take a look at patch number 2: -static int fastrpc_vmap(struct dma_buf *dmabuf, struct dma_buf_map *map) +static int fastrpc_vmap(struct dma_buf *dmabuf, struct iosys_map *map) You are changing the functions signature without changing any of the callers. At bare minimum that causes a warning and on runtime this only works by coincident now because the structure pointers just happen to have the same layout. This is not something we usually do. you missed the magic/hack on patch 1: 1) dma-buf-map.h includes iosys-map.h _at the end_ 2) iosys-map.h includes dma-buf-map.h at the beginning and initially does a "define iosys_map dma_buf_map". So, it doesn't work by coincidence, It's because it was done to allow converting it piecemeal. Oh, my. Please never do stuff like that again. It's not uncommon approach to be required by other subsystems. Even drm-intel already used similar approach for macro conversions crossing drm-intel-next and drm-intel-gt-next branches recently. As I said, I don't mind one way or the other. Before I go and respin this into a single mega patch, I'd like to gather some feedback on the following topics: 1) Daniel Vetter and Thomas Zimmermann seemed to be ok with staying with the current name, dma_buf_map, while you prefer it renamed. Or at least not make the rename a pre-requisite for the API additions in https://lore.kernel.org/all/20220126203702.1784589-1-lucas.demar...@intel.com/ One thing I like about the rename is that it makes clear the separation between this small shim and dma-buf. There are also some APIs that are really dma-buf API (e.g. dma_buf_map_attachment()), but if you don't look carefully you may think it's from dma_buf_map. 2) If renaming, would it still keep the same entry in MAINTAINERS? Thomas suggested drivers core, but this all seem to be used mainly on drm/, with just one exception. 3) If renaming, do we have another preferred name? thanks Lucas De Marchi But as I said, I don't really have a preference. When crossing subsystems one thing that is hard is that different people have different preferences on these things. At least squashing now is much easier than if I had to split it Try to imagine how much complain I received on going the other way in 25985edcedea6396277003854657b5f3cb31a628 with 2463 files changed, 4252 insertions(+), 4252 deletions(-) Well exactly that is perfectly fine. What you do here is applying your personal hack which is absolutely not welcomed. Regards, Christian. :) Lucas De Marchi Regards, Christian. I built this series, full config with CONFIG_COMPILE_TEST and doing: git rebase -i -x "make -j$(nproc)" I split these patches in a way that wouldn't break the build on purpose. There were a couple that I couldn't build without cross compiling: tegra and rockchip. The others were ok. I'm not really against squashing everything in one to merge, though. It will be hard on the conflicts later, but should get the job done much quicker. Lucas De Marchi Regards, Christian. Am 28.01.22 um 09:36 schrieb Lucas De Marchi: Motivation for this started in https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20220126203702.1784589-1-lucas.demarchi%40intel.com%2Fdata=04%7C01%7Cchristian.koenig%40amd.com%7C635084a520994d35a16e08d9e2423319%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637789596221829397%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=ruHpD3DbyyqQuZIFEQU%2B2RH31OwsdFnn1v7N4z75U0Y%3Dreserved=0 when trying to extend the dma-buf-map API to cover new use cases: help a single driver with allocations and sharing code paths for IO and system memory. I'm leaving the API additions aside and first renaming the interface as requested. There are already some users in tree outside the context of dma-buf importer/exporter. So before extending the API, let's dissociate it from dma-buf. The iosys-map.h is introduced in the first patch in a way that allows the conversion of each driver to happen separately. After all the conversions are done we can remove the old one, which is the last patch. Another possible way is to squash everything and merge together, but I believe this would make much harder for review. The conversion was done with the following semantic patch: @r1@ @@ - struct
Re: [Nouveau] [PATCH 00/14] Rename dma-buf-map
Hi Am 01.02.22 um 08:46 schrieb Christian König: [..] 2) If renaming, would it still keep the same entry in MAINTAINERS? Thomas suggested drivers core, but this all seem to be used mainly on drm/, with just one exception. I would just add a complete new entry for this and use Thomas as maintainer (with his permission of course) and dri as mailing list. Sure, no problem. Best regards Thomas 3) If renaming, do we have another preferred name? Nope, as Daniel said the name itself is only bikesheed. What is important is that we see this as functionality separated from the inter driver interface. Regards, Christian. thanks Lucas De Marchi But as I said, I don't really have a preference. When crossing subsystems one thing that is hard is that different people have different preferences on these things. At least squashing now is much easier than if I had to split it Try to imagine how much complain I received on going the other way in 25985edcedea6396277003854657b5f3cb31a628 with 2463 files changed, 4252 insertions(+), 4252 deletions(-) Well exactly that is perfectly fine. What you do here is applying your personal hack which is absolutely not welcomed. Regards, Christian. :) Lucas De Marchi Regards, Christian. I built this series, full config with CONFIG_COMPILE_TEST and doing: git rebase -i -x "make -j$(nproc)" I split these patches in a way that wouldn't break the build on purpose. There were a couple that I couldn't build without cross compiling: tegra and rockchip. The others were ok. I'm not really against squashing everything in one to merge, though. It will be hard on the conflicts later, but should get the job done much quicker. Lucas De Marchi Regards, Christian. Am 28.01.22 um 09:36 schrieb Lucas De Marchi: Motivation for this started in https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20220126203702.1784589-1-lucas.demarchi%40intel.com%2Fdata=04%7C01%7Cchristian.koenig%40amd.com%7C01142fa3ce484040ade008d9e51aef5d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637792726123940514%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=p8rR28Hn0yMTbwy%2F7bpiGyG9fAu9kG1VUzX2MF44mcs%3Dreserved=0 when trying to extend the dma-buf-map API to cover new use cases: help a single driver with allocations and sharing code paths for IO and system memory. I'm leaving the API additions aside and first renaming the interface as requested. There are already some users in tree outside the context of dma-buf importer/exporter. So before extending the API, let's dissociate it from dma-buf. The iosys-map.h is introduced in the first patch in a way that allows the conversion of each driver to happen separately. After all the conversions are done we can remove the old one, which is the last patch. Another possible way is to squash everything and merge together, but I believe this would make much harder for review. The conversion was done with the following semantic patch: @r1@ @@ - struct dma_buf_map + struct iosys_map @r2@ @@ ( - DMA_BUF_MAP_INIT_VADDR + IOSYS_MAP_INIT_VADDR | - dma_buf_map_set_vaddr + iosys_map_set_vaddr | - dma_buf_map_set_vaddr_iomem + iosys_map_set_vaddr_iomem | - dma_buf_map_is_equal + iosys_map_is_equal | - dma_buf_map_is_null + iosys_map_is_null | - dma_buf_map_is_set + iosys_map_is_set | - dma_buf_map_clear + iosys_map_clear | - dma_buf_map_memcpy_to + iosys_map_memcpy_to | - dma_buf_map_incr + iosys_map_incr ) @@ @@ - #include + #include and then some files had their includes adjusted so we can build everything on each commit in this series. Also some comments were update to remove mentions to dma-buf-map. Simply doing a sed to rename didn't work as dma-buf has some APIs using the dma_buf_map prefix. Once finalized, I think most of this, if not all, could go through the drm-misc-next branch. I split i915, msm, nouveau, and radeon in their own patches in case it's preferred to take those through their own trees. Lucas De Marchi Lucas De Marchi (14): iosys-map: Introduce renamed dma-buf-map misc: fastrpc: Replace dma-buf-map with iosys-map dma-buf: Replace dma-buf-map with iosys-map media: Replace dma-buf-map with iosys-map drm/ttm: Replace dma-buf-map with iosys-map drm: Replace dma-buf-map with iosys-map in drivers drm/i915: Replace dma-buf-map with iosys-map drm/msm: Replace dma-buf-map with iosys-map drm/nouveau: Replace dma-buf-map with iosys-map drm/tegra: Replace dma-buf-map with iosys-map drm/radeon: Replace dma-buf-map with iosys-map drm: Replace dma-buf-map with iosys-map in common code Documentation: Refer to iosys-map instead of dma-buf-map dma-buf-map: Remove API in favor of iosys-map Documentation/driver-api/dma-buf.rst
Re: [Nouveau] [PATCH 00/14] Rename dma-buf-map
On Fri, Jan 28, 2022 at 10:22:00AM +0100, Christian König wrote: Am 28.01.22 um 10:12 schrieb Lucas De Marchi: On Fri, Jan 28, 2022 at 09:41:14AM +0100, Christian König wrote: Rule #1 is to never ever break the build. Because of this all those patches needs to be squashed into a single one as far as I can see. what config are you building on? Well I'm not building at all, I'm just looking at the patches as an engineer with 25 years of experience with Linux patches. Just take a look at patch number 2: -static int fastrpc_vmap(struct dma_buf *dmabuf, struct dma_buf_map *map) +static int fastrpc_vmap(struct dma_buf *dmabuf, struct iosys_map *map) You are changing the functions signature without changing any of the callers. At bare minimum that causes a warning and on runtime this only works by coincident now because the structure pointers just happen to have the same layout. This is not something we usually do. you missed the magic/hack on patch 1: 1) dma-buf-map.h includes iosys-map.h _at the end_ 2) iosys-map.h includes dma-buf-map.h at the beginning and initially does a "define iosys_map dma_buf_map". So, it doesn't work by coincidence, It's because it was done to allow converting it piecemeal. But as I said, I don't really have a preference. When crossing subsystems one thing that is hard is that different people have different preferences on these things. At least squashing now is much easier than if I had to split it Try to imagine how much complain I received on going the other way in 25985edcedea6396277003854657b5f3cb31a628 with 2463 files changed, 4252 insertions(+), 4252 deletions(-) :) Lucas De Marchi Regards, Christian. I built this series, full config with CONFIG_COMPILE_TEST and doing: git rebase -i -x "make -j$(nproc)" I split these patches in a way that wouldn't break the build on purpose. There were a couple that I couldn't build without cross compiling: tegra and rockchip. The others were ok. I'm not really against squashing everything in one to merge, though. It will be hard on the conflicts later, but should get the job done much quicker. Lucas De Marchi Regards, Christian. Am 28.01.22 um 09:36 schrieb Lucas De Marchi: Motivation for this started in https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20220126203702.1784589-1-lucas.demarchi%40intel.com%2Fdata=04%7C01%7Cchristian.koenig%40amd.com%7C15bd6767b2fb4b2c027e08d9e23e46af%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637789579371467295%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=yAllbKjW29SsXA0CMrhK%2BDDvZ1A5CrSptshmsh5vYNQ%3Dreserved=0 when trying to extend the dma-buf-map API to cover new use cases: help a single driver with allocations and sharing code paths for IO and system memory. I'm leaving the API additions aside and first renaming the interface as requested. There are already some users in tree outside the context of dma-buf importer/exporter. So before extending the API, let's dissociate it from dma-buf. The iosys-map.h is introduced in the first patch in a way that allows the conversion of each driver to happen separately. After all the conversions are done we can remove the old one, which is the last patch. Another possible way is to squash everything and merge together, but I believe this would make much harder for review. The conversion was done with the following semantic patch: @r1@ @@ - struct dma_buf_map + struct iosys_map @r2@ @@ ( - DMA_BUF_MAP_INIT_VADDR + IOSYS_MAP_INIT_VADDR | - dma_buf_map_set_vaddr + iosys_map_set_vaddr | - dma_buf_map_set_vaddr_iomem + iosys_map_set_vaddr_iomem | - dma_buf_map_is_equal + iosys_map_is_equal | - dma_buf_map_is_null + iosys_map_is_null | - dma_buf_map_is_set + iosys_map_is_set | - dma_buf_map_clear + iosys_map_clear | - dma_buf_map_memcpy_to + iosys_map_memcpy_to | - dma_buf_map_incr + iosys_map_incr ) @@ @@ - #include + #include and then some files had their includes adjusted so we can build everything on each commit in this series. Also some comments were update to remove mentions to dma-buf-map. Simply doing a sed to rename didn't work as dma-buf has some APIs using the dma_buf_map prefix. Once finalized, I think most of this, if not all, could go through the drm-misc-next branch. I split i915, msm, nouveau, and radeon in their own patches in case it's preferred to take those through their own trees. Lucas De Marchi Lucas De Marchi (14): iosys-map: Introduce renamed dma-buf-map misc: fastrpc: Replace dma-buf-map with iosys-map dma-buf: Replace dma-buf-map with iosys-map media: Replace dma-buf-map with iosys-map drm/ttm: Replace dma-buf-map with iosys-map drm: Replace dma-buf-map with iosys-map in drivers drm/i915: Replace dma-buf-map with
Re: [Nouveau] [PATCH 00/14] Rename dma-buf-map
On Fri, Jan 28, 2022 at 09:41:14AM +0100, Christian König wrote: Rule #1 is to never ever break the build. Because of this all those patches needs to be squashed into a single one as far as I can see. what config are you building on? I built this series, full config with CONFIG_COMPILE_TEST and doing: git rebase -i -x "make -j$(nproc)" I split these patches in a way that wouldn't break the build on purpose. There were a couple that I couldn't build without cross compiling: tegra and rockchip. The others were ok. I'm not really against squashing everything in one to merge, though. It will be hard on the conflicts later, but should get the job done much quicker. Lucas De Marchi Regards, Christian. Am 28.01.22 um 09:36 schrieb Lucas De Marchi: Motivation for this started in https://lore.kernel.org/lkml/20220126203702.1784589-1-lucas.demar...@intel.com/ when trying to extend the dma-buf-map API to cover new use cases: help a single driver with allocations and sharing code paths for IO and system memory. I'm leaving the API additions aside and first renaming the interface as requested. There are already some users in tree outside the context of dma-buf importer/exporter. So before extending the API, let's dissociate it from dma-buf. The iosys-map.h is introduced in the first patch in a way that allows the conversion of each driver to happen separately. After all the conversions are done we can remove the old one, which is the last patch. Another possible way is to squash everything and merge together, but I believe this would make much harder for review. The conversion was done with the following semantic patch: @r1@ @@ - struct dma_buf_map + struct iosys_map @r2@ @@ ( - DMA_BUF_MAP_INIT_VADDR + IOSYS_MAP_INIT_VADDR | - dma_buf_map_set_vaddr + iosys_map_set_vaddr | - dma_buf_map_set_vaddr_iomem + iosys_map_set_vaddr_iomem | - dma_buf_map_is_equal + iosys_map_is_equal | - dma_buf_map_is_null + iosys_map_is_null | - dma_buf_map_is_set + iosys_map_is_set | - dma_buf_map_clear + iosys_map_clear | - dma_buf_map_memcpy_to + iosys_map_memcpy_to | - dma_buf_map_incr + iosys_map_incr ) @@ @@ - #include + #include and then some files had their includes adjusted so we can build everything on each commit in this series. Also some comments were update to remove mentions to dma-buf-map. Simply doing a sed to rename didn't work as dma-buf has some APIs using the dma_buf_map prefix. Once finalized, I think most of this, if not all, could go through the drm-misc-next branch. I split i915, msm, nouveau, and radeon in their own patches in case it's preferred to take those through their own trees. Lucas De Marchi Lucas De Marchi (14): iosys-map: Introduce renamed dma-buf-map misc: fastrpc: Replace dma-buf-map with iosys-map dma-buf: Replace dma-buf-map with iosys-map media: Replace dma-buf-map with iosys-map drm/ttm: Replace dma-buf-map with iosys-map drm: Replace dma-buf-map with iosys-map in drivers drm/i915: Replace dma-buf-map with iosys-map drm/msm: Replace dma-buf-map with iosys-map drm/nouveau: Replace dma-buf-map with iosys-map drm/tegra: Replace dma-buf-map with iosys-map drm/radeon: Replace dma-buf-map with iosys-map drm: Replace dma-buf-map with iosys-map in common code Documentation: Refer to iosys-map instead of dma-buf-map dma-buf-map: Remove API in favor of iosys-map Documentation/driver-api/dma-buf.rst | 4 +- Documentation/gpu/todo.rst| 20 +- MAINTAINERS | 2 +- drivers/dma-buf/dma-buf.c | 22 +- drivers/dma-buf/heaps/cma_heap.c | 10 +- drivers/dma-buf/heaps/system_heap.c | 10 +- drivers/gpu/drm/ast/ast_drv.h | 2 +- drivers/gpu/drm/ast/ast_mode.c| 8 +- drivers/gpu/drm/drm_cache.c | 18 +- drivers/gpu/drm/drm_client.c | 9 +- drivers/gpu/drm/drm_fb_helper.c | 12 +- drivers/gpu/drm/drm_gem.c | 12 +- drivers/gpu/drm/drm_gem_cma_helper.c | 9 +- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 16 +- drivers/gpu/drm/drm_gem_shmem_helper.c| 15 +- drivers/gpu/drm/drm_gem_ttm_helper.c | 4 +- drivers/gpu/drm/drm_gem_vram_helper.c | 25 +- drivers/gpu/drm/drm_internal.h| 6 +- drivers/gpu/drm/drm_mipi_dbi.c| 8 +- drivers/gpu/drm/drm_prime.c | 4 +- drivers/gpu/drm/etnaviv/etnaviv_drv.h | 2 +- drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 8 +- drivers/gpu/drm/gud/gud_pipe.c| 4 +-
