| Message ID | 20220920211545.1017355-2-adrian.larumbe@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/panfrost: Remove type name from internal structs | expand |
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 >
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 >>
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 >>> >
> > 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:
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)
__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(-)