| Message ID | 20220912164413.2181937-1-adrian.larumbe@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panfrost: Give name to anonymous coredump object union | expand |
Have we checked that this actually fixes the Mesa build? If so, R-b. > Commit 730c2bf4ad39 ("drm/panfrost: Add support for devcoredump") > introduces one such union, breaking the Mesa build. > > Give it a name, and also rename pan_reg_hdr structure because it will > always be prefixed by the union name. > > Bug: https://gitlab.freedesktop.org/mesa/mesa/-/issues/7195 > > Signed-off-by: Adri??n Larumbe <adrian.larumbe@collabora.com> In Mesa, we would add a trailer "Fixes: 730c2bf4ad39 ("drm/panfrost: Add support for devcoredump")". If the kernel does the same (I don't remember), we should do that here, seeing as the panfrost uapi headers do need to build as C++.
On 12/09/2022 17:44, Adrián Larumbe wrote: > Building Mesa's Perfetto requires including the panfrost drm uAPI header in > C++ code, but the C++ compiler requires anonymous unions to have only > public non-static data members. > > Commit 730c2bf4ad39 ("drm/panfrost: Add support for devcoredump") > introduces one such union, breaking the Mesa build. > > Give it a name, and also rename pan_reg_hdr structure because it will > always be prefixed by the union name. > > Bug: https://gitlab.freedesktop.org/mesa/mesa/-/issues/7195 > > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com> Ouch! It's frustrating how C++ isn't quite a superset of C. However I think we can solve this with a simpler patch, I'd appreciate testing that this does indeed fix the build issues with Mesa with all supported compilers (I'm not so familiar with C++): ----8<---- From 492714a7dff0710ac5b8b457bcfe9ae52b458565 Mon Sep 17 00:00:00 2001 From: Steven Price <steven.price@arm.com> Date: Tue, 13 Sep 2022 09:37:55 +0100 Subject: [PATCH] drm/panfrost: Remove type name from internal structs The two structs internal to struct panfrost_dump_object_header were named, but sadly that is incompatible with C++, causing an error: "an anonymous union may only have public non-static data members". However nothing refers to struct pan_reg_hdr and struct pan_bomap_hdr and there's no need to export these definitions, so lets drop them. This fixes the C++ build error with the minimum change in userspace API. Bug: https://gitlab.freedesktop.org/mesa/mesa/-/issues/7195 Fixes: 730c2bf4ad39 ("drm/panfrost: Add support for devcoredump") Signed-off-by: Steven Price <steven.price@arm.com> --- include/uapi/drm/panfrost_drm.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h index eac87310b348..bd77254be121 100644 --- a/include/uapi/drm/panfrost_drm.h +++ b/include/uapi/drm/panfrost_drm.h @@ -242,7 +242,7 @@ struct panfrost_dump_object_header { __le32 file_offset; union { - struct pan_reg_hdr { + struct { __le64 jc; __le32 gpu_id; __le32 major; @@ -250,7 +250,7 @@ struct panfrost_dump_object_header { __le64 nbos; } reghdr; - struct pan_bomap_hdr { + struct { __le32 valid; __le64 iova; __le32 data[2];
Hi Steven, On 13.09.2022 09:45, Steven Price wrote: >On 12/09/2022 17:44, Adrián Larumbe wrote: >> Building Mesa's Perfetto requires including the panfrost drm uAPI header in >> C++ code, but the C++ compiler requires anonymous unions to have only >> public non-static data members. >> >> Commit 730c2bf4ad39 ("drm/panfrost: Add support for devcoredump") >> introduces one such union, breaking the Mesa build. >> >> Give it a name, and also rename pan_reg_hdr structure because it will >> always be prefixed by the union name. >> >> Bug: https://gitlab.freedesktop.org/mesa/mesa/-/issues/7195 >> >> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com> >Ouch! It's frustrating how C++ isn't quite a superset of C. However I >think we can solve this with a simpler patch, I'd appreciate testing >that this does indeed fix the build issues with Mesa with all supported >compilers (I'm not so familiar with C++): I just tested your changes on Mesa and they do fix the build. >----8<---- >From 492714a7dff0710ac5b8b457bcfe9ae52b458565 Mon Sep 17 00:00:00 2001 >From: Steven Price <steven.price@arm.com> >Date: Tue, 13 Sep 2022 09:37:55 +0100 >Subject: [PATCH] drm/panfrost: Remove type name from internal structs > >The two structs internal to struct panfrost_dump_object_header were >named, but sadly that is incompatible with C++, causing an error: "an >anonymous union may only have public non-static data members". > >However nothing refers to struct pan_reg_hdr and struct pan_bomap_hdr >and there's no need to export these definitions, so lets drop them. This >fixes the C++ build error with the minimum change in userspace API. > >Bug: https://gitlab.freedesktop.org/mesa/mesa/-/issues/7195 >Fixes: 730c2bf4ad39 ("drm/panfrost: Add support for devcoredump") >Signed-off-by: Steven Price <steven.price@arm.com> >--- > include/uapi/drm/panfrost_drm.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/include/uapi/drm/panfrost_drm.h >b/include/uapi/drm/panfrost_drm.h >index eac87310b348..bd77254be121 100644 >--- a/include/uapi/drm/panfrost_drm.h >+++ b/include/uapi/drm/panfrost_drm.h >@@ -242,7 +242,7 @@ struct panfrost_dump_object_header { > __le32 file_offset; > > union { >- struct pan_reg_hdr { >+ struct { > __le64 jc; > __le32 gpu_id; > __le32 major; >@@ -250,7 +250,7 @@ struct panfrost_dump_object_header { > __le64 nbos; > } reghdr; > >- struct pan_bomap_hdr { >+ struct { > __le32 valid; > __le64 iova; > __le32 data[2]; >-- >2.34.1
On 19/09/2022 07:44, Adrián Larumbe wrote: > Hi Steven, > > On 13.09.2022 09:45, Steven Price wrote: >> On 12/09/2022 17:44, Adrián Larumbe wrote: >>> Building Mesa's Perfetto requires including the panfrost drm uAPI header in >>> C++ code, but the C++ compiler requires anonymous unions to have only >>> public non-static data members. >>> >>> Commit 730c2bf4ad39 ("drm/panfrost: Add support for devcoredump") >>> introduces one such union, breaking the Mesa build. >>> >>> Give it a name, and also rename pan_reg_hdr structure because it will >>> always be prefixed by the union name. >>> >>> Bug: https://gitlab.freedesktop.org/mesa/mesa/-/issues/7195 >>> >>> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com> > >> Ouch! It's frustrating how C++ isn't quite a superset of C. However I >> think we can solve this with a simpler patch, I'd appreciate testing >> that this does indeed fix the build issues with Mesa with all supported >> compilers (I'm not so familiar with C++): > > I just tested your changes on Mesa and they do fix the build. Thanks Adrián! Alyssa: Could you give your R-b if you're happy with this change? It would be good to get this fixed before it hits -rc1. Thanks, Steve > >> ----8<---- >>From 492714a7dff0710ac5b8b457bcfe9ae52b458565 Mon Sep 17 00:00:00 2001 >> From: Steven Price <steven.price@arm.com> >> Date: Tue, 13 Sep 2022 09:37:55 +0100 >> Subject: [PATCH] drm/panfrost: Remove type name from internal structs >> >> The two structs internal to struct panfrost_dump_object_header were >> named, but sadly that is incompatible with C++, causing an error: "an >> anonymous union may only have public non-static data members". >> >> However nothing refers to struct pan_reg_hdr and struct pan_bomap_hdr >> and there's no need to export these definitions, so lets drop them. This >> fixes the C++ build error with the minimum change in userspace API. >> >> Bug: https://gitlab.freedesktop.org/mesa/mesa/-/issues/7195 >> Fixes: 730c2bf4ad39 ("drm/panfrost: Add support for devcoredump") >> Signed-off-by: Steven Price <steven.price@arm.com> >> --- >> include/uapi/drm/panfrost_drm.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/include/uapi/drm/panfrost_drm.h >> b/include/uapi/drm/panfrost_drm.h >> index eac87310b348..bd77254be121 100644 >> --- a/include/uapi/drm/panfrost_drm.h >> +++ b/include/uapi/drm/panfrost_drm.h >> @@ -242,7 +242,7 @@ struct panfrost_dump_object_header { >> __le32 file_offset; >> >> union { >> - struct pan_reg_hdr { >> + struct { >> __le64 jc; >> __le32 gpu_id; >> __le32 major; >> @@ -250,7 +250,7 @@ struct panfrost_dump_object_header { >> __le64 nbos; >> } reghdr; >> >> - struct pan_bomap_hdr { >> + struct { >> __le32 valid; >> __le64 iova; >> __le32 data[2]; >> -- >> 2.34.1
On Tue, Sep 20, 2022 at 02:26:52PM +0100, Steven Price wrote: > On 19/09/2022 07:44, Adri??n Larumbe wrote: > > Hi Steven, > > > > On 13.09.2022 09:45, Steven Price wrote: > >> On 12/09/2022 17:44, Adri??n Larumbe wrote: > >>> Building Mesa's Perfetto requires including the panfrost drm uAPI header in > >>> C++ code, but the C++ compiler requires anonymous unions to have only > >>> public non-static data members. > >>> > >>> Commit 730c2bf4ad39 ("drm/panfrost: Add support for devcoredump") > >>> introduces one such union, breaking the Mesa build. > >>> > >>> Give it a name, and also rename pan_reg_hdr structure because it will > >>> always be prefixed by the union name. > >>> > >>> Bug: https://gitlab.freedesktop.org/mesa/mesa/-/issues/7195 > >>> > >>> Signed-off-by: Adri??n Larumbe <adrian.larumbe@collabora.com> > > > >> Ouch! It's frustrating how C++ isn't quite a superset of C. However I > >> think we can solve this with a simpler patch, I'd appreciate testing > >> that this does indeed fix the build issues with Mesa with all supported > >> compilers (I'm not so familiar with C++): > > > > I just tested your changes on Mesa and they do fix the build. > > Thanks Adri??n! > > Alyssa: Could you give your R-b if you're happy with this change? It > would be good to get this fixed before it hits -rc1. R-b, however the issue isn't totally gone: in a separate but related issue, apparently the __le types aren't portable and the devcoredump support has now broken the panfrost (mesa) build for FreeBSD, which has a UAPI-compatible reimplementation of panfrost.ko ... Do we maybe want to change all the __le to u at the same time? If we have to break UAPI, better do it before the UAPI is actually merged. Panfrost is probably broken in far worse ways on big endian anyway. Or maybe we want to keep doing little-endian but in u32 containers and have conversions in the kernel for big-endian CPUs. Or maybe we want to just "we don't care about big endian, because you'll have worse problems", at a GPU level Mali hasn't supported big endian since Midgard so I doubt the recent DDKs would work on BE either. Anyway, ideally we'd solve both at once, and soon, so we don't have to revert the devcoredump stuff from mesa. Thanks, Alyssa
diff --git a/drivers/gpu/drm/panfrost/panfrost_dump.c b/drivers/gpu/drm/panfrost/panfrost_dump.c index 89056a1aac7d..e40b6eace187 100644 --- a/drivers/gpu/drm/panfrost/panfrost_dump.c +++ b/drivers/gpu/drm/panfrost/panfrost_dump.c @@ -177,11 +177,11 @@ void panfrost_core_dump(struct panfrost_job *job) * For now, we write the job identifier in the register dump header, * so that we can decode the entire dump later with pandecode */ - iter.hdr->reghdr.jc = cpu_to_le64(job->jc); - iter.hdr->reghdr.major = cpu_to_le32(PANFROSTDUMP_MAJOR); - iter.hdr->reghdr.minor = cpu_to_le32(PANFROSTDUMP_MINOR); - iter.hdr->reghdr.gpu_id = cpu_to_le32(pfdev->features.id); - iter.hdr->reghdr.nbos = cpu_to_le64(job->bo_count); + iter.hdr->pan_hdr.regs.jc = cpu_to_le64(job->jc); + iter.hdr->pan_hdr.regs.major = cpu_to_le32(PANFROSTDUMP_MAJOR); + iter.hdr->pan_hdr.regs.minor = cpu_to_le32(PANFROSTDUMP_MINOR); + iter.hdr->pan_hdr.regs.gpu_id = cpu_to_le32(pfdev->features.id); + iter.hdr->pan_hdr.regs.nbos = cpu_to_le64(job->bo_count); panfrost_core_dump_registers(&iter, pfdev, as_nr, slot); @@ -205,20 +205,20 @@ void panfrost_core_dump(struct panfrost_job *job) if (!bo->base.sgt) { dev_err(pfdev->dev, "Panfrost Dump: BO has no sgt, cannot dump\n"); - iter.hdr->bomap.valid = 0; + iter.hdr->pan_hdr.bomap.valid = 0; goto dump_header; } ret = drm_gem_shmem_vmap(&bo->base, &map); if (ret) { dev_err(pfdev->dev, "Panfrost Dump: couldn't map Buffer Object\n"); - iter.hdr->bomap.valid = 0; + iter.hdr->pan_hdr.bomap.valid = 0; goto dump_header; } WARN_ON(!mapping->active); - iter.hdr->bomap.data[0] = cpu_to_le32((bomap - bomap_start)); + iter.hdr->pan_hdr.bomap.data[0] = cpu_to_le32((bomap - bomap_start)); for_each_sgtable_page(bo->base.sgt, &page_iter, 0) { struct page *page = sg_page_iter_page(&page_iter); @@ -231,14 +231,14 @@ void panfrost_core_dump(struct panfrost_job *job) } } - iter.hdr->bomap.iova = cpu_to_le64(mapping->mmnode.start << PAGE_SHIFT); + iter.hdr->pan_hdr.bomap.iova = cpu_to_le64(mapping->mmnode.start << PAGE_SHIFT); vaddr = map.vaddr; memcpy(iter.data, vaddr, bo->base.base.size); drm_gem_shmem_vunmap(&bo->base, &map); - iter.hdr->bomap.valid = cpu_to_le32(1); + iter.hdr->pan_hdr.bomap.valid = cpu_to_le32(1); dump_header: panfrost_core_dump_header(&iter, PANFROSTDUMP_BUF_BO, iter.data + bo->base.base.size); diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h index eac87310b348..4da33e4d7e2c 100644 --- a/include/uapi/drm/panfrost_drm.h +++ b/include/uapi/drm/panfrost_drm.h @@ -248,7 +248,7 @@ struct panfrost_dump_object_header { __le32 major; __le32 minor; __le64 nbos; - } reghdr; + } regs; struct pan_bomap_hdr { __le32 valid; @@ -262,7 +262,7 @@ struct panfrost_dump_object_header { */ __le32 sizer[496]; - }; + } pan_hdr; }; /* Registers object, an array of these */
Building Mesa's Perfetto requires including the panfrost drm uAPI header in C++ code, but the C++ compiler requires anonymous unions to have only public non-static data members. Commit 730c2bf4ad39 ("drm/panfrost: Add support for devcoredump") introduces one such union, breaking the Mesa build. Give it a name, and also rename pan_reg_hdr structure because it will always be prefixed by the union name. Bug: https://gitlab.freedesktop.org/mesa/mesa/-/issues/7195 Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com> --- drivers/gpu/drm/panfrost/panfrost_dump.c | 20 ++++++++++---------- include/uapi/drm/panfrost_drm.h | 4 ++-- 2 files changed, 12 insertions(+), 12 deletions(-)