[PATCH] drm/panfrost: Give name to anonymous coredump object union

[PATCH] drm/panfrost: Give name to anonymous coredump object union

[Adrián Larumbe]

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(-)

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 */
-- 
2.37.0

Re: [PATCH] drm/panfrost: Give name to anonymous coredump object union

[Alyssa Rosenzweig]

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++.

Re: [PATCH] drm/panfrost: Give name to anonymous coredump object union

[Steven Price]

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];
-- 
2.34.1

Re: [PATCH] drm/panfrost: Give name to anonymous coredump object union

[Adrián Larumbe]

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

Re: [PATCH] drm/panfrost: Give name to anonymous coredump object union

[Steven Price]

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

Re: [PATCH] drm/panfrost: Give name to anonymous coredump object union

[Alyssa Rosenzweig]

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

[PATCH 1/2] drm/panfrost: Remove type name from internal structs

[Adrián Larumbe]

From: Steven Price <steven.price@arm.com>

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.37.0

[PATCH 2/2] drm/panfrost: replace endian-specific types with generic ones

[Adrián Larumbe]

__le32 and __l64 endian-specific types aren't portable and not available on
FreeBSD, for which there's a uAPI compatible reimplementation of Panfrost.

Replace these specific types with more generic unsigned ones, to prevent
FreeBSD Mesa build errors.

Bug: https://gitlab.freedesktop.org/mesa/mesa/-/issues/7252
Fixes: 730c2bf4ad39 ("drm/panfrost: Add support for devcoredump")
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 include/uapi/drm/panfrost_drm.h | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
index bd77254be121..c1a10a9366a9 100644
--- a/include/uapi/drm/panfrost_drm.h
+++ b/include/uapi/drm/panfrost_drm.h
@@ -236,24 +236,24 @@ struct drm_panfrost_madvise {
 #define PANFROSTDUMP_BUF_TRAILER (PANFROSTDUMP_BUF_BO + 1)
 
 struct panfrost_dump_object_header {
-	__le32 magic;
-	__le32 type;
-	__le32 file_size;
-	__le32 file_offset;
+	__u32 magic;
+	__u32 type;
+	__u32 file_size;
+	__u32 file_offset;
 
 	union {
 		struct {
-			__le64 jc;
-			__le32 gpu_id;
-			__le32 major;
-			__le32 minor;
-			__le64 nbos;
+			__u64 jc;
+			__u32 gpu_id;
+			__u32 major;
+			__u32 minor;
+			__u64 nbos;
 		} reghdr;
 
 		struct {
-			__le32 valid;
-			__le64 iova;
-			__le32 data[2];
+			__u32 valid;
+			__u64 iova;
+			__u32 data[2];
 		} bomap;
 
 		/*
@@ -261,14 +261,14 @@ struct panfrost_dump_object_header {
 		 * with new fields and also keep it 512-byte aligned
 		 */
 
-		__le32 sizer[496];
+		__u32 sizer[496];
 	};
 };
 
 /* Registers object, an array of these */
 struct panfrost_dump_registers {
-	__le32 reg;
-	__le32 value;
+	__u32 reg;
+	__u32 value;
 };
 
 #if defined(__cplusplus)
-- 
2.37.0

Re: [PATCH 2/2] drm/panfrost: replace endian-specific types with generic ones

[Alyssa Rosenzweig]

Tentative r-b, but we *do* need to make a decision on how we want to
handle endianness. I don't have strong feelings but the results of that
discussion should go in the commit message.

On Tue, Sep 20, 2022 at 10:15:45PM +0100, Adri??n Larumbe wrote:
> __le32 and __l64 endian-specific types aren't portable and not available on
> FreeBSD, for which there's a uAPI compatible reimplementation of Panfrost.
> 
> Replace these specific types with more generic unsigned ones, to prevent
> FreeBSD Mesa build errors.
> 
> Bug: https://gitlab.freedesktop.org/mesa/mesa/-/issues/7252
> Fixes: 730c2bf4ad39 ("drm/panfrost: Add support for devcoredump")
> Signed-off-by: Adri??n Larumbe <adrian.larumbe@collabora.com>
> ---
>  include/uapi/drm/panfrost_drm.h | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
> index bd77254be121..c1a10a9366a9 100644
> --- a/include/uapi/drm/panfrost_drm.h
> +++ b/include/uapi/drm/panfrost_drm.h
> @@ -236,24 +236,24 @@ struct drm_panfrost_madvise {
>  #define PANFROSTDUMP_BUF_TRAILER (PANFROSTDUMP_BUF_BO + 1)
>  
>  struct panfrost_dump_object_header {
> -	__le32 magic;
> -	__le32 type;
> -	__le32 file_size;
> -	__le32 file_offset;
> +	__u32 magic;
> +	__u32 type;
> +	__u32 file_size;
> +	__u32 file_offset;
>  
>  	union {
>  		struct {
> -			__le64 jc;
> -			__le32 gpu_id;
> -			__le32 major;
> -			__le32 minor;
> -			__le64 nbos;
> +			__u64 jc;
> +			__u32 gpu_id;
> +			__u32 major;
> +			__u32 minor;
> +			__u64 nbos;
>  		} reghdr;
>  
>  		struct {
> -			__le32 valid;
> -			__le64 iova;
> -			__le32 data[2];
> +			__u32 valid;
> +			__u64 iova;
> +			__u32 data[2];
>  		} bomap;
>  
>  		/*
> @@ -261,14 +261,14 @@ struct panfrost_dump_object_header {
>  		 * with new fields and also keep it 512-byte aligned
>  		 */
>  
> -		__le32 sizer[496];
> +		__u32 sizer[496];
>  	};
>  };
>  
>  /* Registers object, an array of these */
>  struct panfrost_dump_registers {
> -	__le32 reg;
> -	__le32 value;
> +	__u32 reg;
> +	__u32 value;
>  };
>  
>  #if defined(__cplusplus)
> -- 
> 2.37.0
> 

Re: [PATCH 2/2] drm/panfrost: replace endian-specific types with generic ones

[Steven Price]

On 20/09/2022 23:13, Alyssa Rosenzweig wrote:
> Tentative r-b, but we *do* need to make a decision on how we want to
> handle endianness. I don't have strong feelings but the results of that
> discussion should go in the commit message.

Linux currently treats the dump objects specially - the headers are
little endian. All the other (Panfrost) DRM structures are native endian
(although I doubt anyone has tested it so I expect bugs). I've no
particularly strong views on this, but since the dump objects are likely
to be saved to disk and transferred between computers it makes sense to
fix the endianness for those. The __le types currently mean sparse can
warn if we screw up in the kernel, so it would be a shame to lose that
type checking.

Another option would be to extend the list of typedefs in
include/uapi/drm/drm.h to include the __le types. We'd need wider buy-in
for that change though.

Finally etnaviv 'solves' the issue by not including the dump structures
in the UABI header...

Or of course we could just actually use native endian and detect from
the magic which endian is in use. That would require ripping out the
cpu_to_lexx() calls in Linux and making the user space tool more
intelligent. I'm happy with that, but it's pushing the complexity onto Mesa.

Steve

> On Tue, Sep 20, 2022 at 10:15:45PM +0100, Adri??n Larumbe wrote:
>> __le32 and __l64 endian-specific types aren't portable and not available on
>> FreeBSD, for which there's a uAPI compatible reimplementation of Panfrost.
>>
>> Replace these specific types with more generic unsigned ones, to prevent
>> FreeBSD Mesa build errors.
>>
>> Bug: https://gitlab.freedesktop.org/mesa/mesa/-/issues/7252
>> Fixes: 730c2bf4ad39 ("drm/panfrost: Add support for devcoredump")
>> Signed-off-by: Adri??n Larumbe <adrian.larumbe@collabora.com>
>> ---
>>  include/uapi/drm/panfrost_drm.h | 30 +++++++++++++++---------------
>>  1 file changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
>> index bd77254be121..c1a10a9366a9 100644
>> --- a/include/uapi/drm/panfrost_drm.h
>> +++ b/include/uapi/drm/panfrost_drm.h
>> @@ -236,24 +236,24 @@ struct drm_panfrost_madvise {
>>  #define PANFROSTDUMP_BUF_TRAILER (PANFROSTDUMP_BUF_BO + 1)
>>  
>>  struct panfrost_dump_object_header {
>> -	__le32 magic;
>> -	__le32 type;
>> -	__le32 file_size;
>> -	__le32 file_offset;
>> +	__u32 magic;
>> +	__u32 type;
>> +	__u32 file_size;
>> +	__u32 file_offset;
>>  
>>  	union {
>>  		struct {
>> -			__le64 jc;
>> -			__le32 gpu_id;
>> -			__le32 major;
>> -			__le32 minor;
>> -			__le64 nbos;
>> +			__u64 jc;
>> +			__u32 gpu_id;
>> +			__u32 major;
>> +			__u32 minor;
>> +			__u64 nbos;
>>  		} reghdr;
>>  
>>  		struct {
>> -			__le32 valid;
>> -			__le64 iova;
>> -			__le32 data[2];
>> +			__u32 valid;
>> +			__u64 iova;
>> +			__u32 data[2];
>>  		} bomap;
>>  
>>  		/*
>> @@ -261,14 +261,14 @@ struct panfrost_dump_object_header {
>>  		 * with new fields and also keep it 512-byte aligned
>>  		 */
>>  
>> -		__le32 sizer[496];
>> +		__u32 sizer[496];
>>  	};
>>  };
>>  
>>  /* Registers object, an array of these */
>>  struct panfrost_dump_registers {
>> -	__le32 reg;
>> -	__le32 value;
>> +	__u32 reg;
>> +	__u32 value;
>>  };
>>  
>>  #if defined(__cplusplus)
>> -- 
>> 2.37.0
>>

Re: [PATCH 2/2] drm/panfrost: replace endian-specific types with generic ones

[Robin Murphy]

On 2022-09-21 09:48, Steven Price wrote:
> On 20/09/2022 23:13, Alyssa Rosenzweig wrote:
>> Tentative r-b, but we *do* need to make a decision on how we want to
>> handle endianness. I don't have strong feelings but the results of that
>> discussion should go in the commit message.
> 
> Linux currently treats the dump objects specially - the headers are
> little endian. All the other (Panfrost) DRM structures are native endian
> (although I doubt anyone has tested it so I expect bugs).

If there can be *any* native-endian data included in the dump, then the 
original endianness needs to be recorded to be able to analyse it 
correctly anyway. The dumping code can't know the granularity at which 
arbitrary BOs may or may not need to be byteswapped to make everything 
consistently LE.

> I've no
> particularly strong views on this, but since the dump objects are likely
> to be saved to disk and transferred between computers it makes sense to
> fix the endianness for those. The __le types currently mean sparse can
> warn if we screw up in the kernel, so it would be a shame to lose that
> type checking.
> 
> Another option would be to extend the list of typedefs in
> include/uapi/drm/drm.h to include the __le types. We'd need wider buy-in
> for that change though.
> 
> Finally etnaviv 'solves' the issue by not including the dump structures
> in the UABI header...
> 
> Or of course we could just actually use native endian and detect from
> the magic which endian is in use. That would require ripping out the
> cpu_to_lexx() calls in Linux and making the user space tool more
> intelligent. I'm happy with that, but it's pushing the complexity onto Mesa.

If there's a clearly identifiable header, then I'd say making the whole 
dump native-endian is probably the way to go. Unless and until anyone 
actually demands to be able to do cross-endian post-mortem GPU 
debugging, the realistic extent of the complexity in Mesa is that it 
doesn't recognise the foreign dump format and gives up, which I assume 
is already implemented :)

Cheers,
Robin.

> 
> Steve
> 
>> On Tue, Sep 20, 2022 at 10:15:45PM +0100, Adri??n Larumbe wrote:
>>> __le32 and __l64 endian-specific types aren't portable and not available on
>>> FreeBSD, for which there's a uAPI compatible reimplementation of Panfrost.
>>>
>>> Replace these specific types with more generic unsigned ones, to prevent
>>> FreeBSD Mesa build errors.
>>>
>>> Bug: https://gitlab.freedesktop.org/mesa/mesa/-/issues/7252
>>> Fixes: 730c2bf4ad39 ("drm/panfrost: Add support for devcoredump")
>>> Signed-off-by: Adri??n Larumbe <adrian.larumbe@collabora.com>
>>> ---
>>>   include/uapi/drm/panfrost_drm.h | 30 +++++++++++++++---------------
>>>   1 file changed, 15 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
>>> index bd77254be121..c1a10a9366a9 100644
>>> --- a/include/uapi/drm/panfrost_drm.h
>>> +++ b/include/uapi/drm/panfrost_drm.h
>>> @@ -236,24 +236,24 @@ struct drm_panfrost_madvise {
>>>   #define PANFROSTDUMP_BUF_TRAILER (PANFROSTDUMP_BUF_BO + 1)
>>>   
>>>   struct panfrost_dump_object_header {
>>> -	__le32 magic;
>>> -	__le32 type;
>>> -	__le32 file_size;
>>> -	__le32 file_offset;
>>> +	__u32 magic;
>>> +	__u32 type;
>>> +	__u32 file_size;
>>> +	__u32 file_offset;
>>>   
>>>   	union {
>>>   		struct {
>>> -			__le64 jc;
>>> -			__le32 gpu_id;
>>> -			__le32 major;
>>> -			__le32 minor;
>>> -			__le64 nbos;
>>> +			__u64 jc;
>>> +			__u32 gpu_id;
>>> +			__u32 major;
>>> +			__u32 minor;
>>> +			__u64 nbos;
>>>   		} reghdr;
>>>   
>>>   		struct {
>>> -			__le32 valid;
>>> -			__le64 iova;
>>> -			__le32 data[2];
>>> +			__u32 valid;
>>> +			__u64 iova;
>>> +			__u32 data[2];
>>>   		} bomap;
>>>   
>>>   		/*
>>> @@ -261,14 +261,14 @@ struct panfrost_dump_object_header {
>>>   		 * with new fields and also keep it 512-byte aligned
>>>   		 */
>>>   
>>> -		__le32 sizer[496];
>>> +		__u32 sizer[496];
>>>   	};
>>>   };
>>>   
>>>   /* Registers object, an array of these */
>>>   struct panfrost_dump_registers {
>>> -	__le32 reg;
>>> -	__le32 value;
>>> +	__u32 reg;
>>> +	__u32 value;
>>>   };
>>>   
>>>   #if defined(__cplusplus)
>>> -- 
>>> 2.37.0
>>>
> 

Re: [PATCH 2/2] drm/panfrost: replace endian-specific types with generic ones

[Alyssa Rosenzweig]

> > Or of course we could just actually use native endian and detect from
> > the magic which endian is in use. That would require ripping out the
> > cpu_to_lexx() calls in Linux and making the user space tool more
> > intelligent. I'm happy with that, but it's pushing the complexity onto Mesa.
> 
> If there's a clearly identifiable header, then I'd say making the whole dump
> native-endian is probably the way to go. Unless and until anyone actually
> demands to be able to do cross-endian post-mortem GPU debugging, the
> realistic extent of the complexity in Mesa is that it doesn't recognise the
> foreign dump format and gives up, which I assume is already implemented :)

+1 to this solution. Gets the complexity out of both kernel and Mesa,
and in the vanishingly unlikely scenario that we need this
functionality, we can add it to Mesa without kernel changes. As mesa
panfrost maintainer I'll take those odds :+1: