[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 at 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
>>>
>