bitfield structures

[Jerome Glisse]

On Thu, Oct 16, 2014 at 10:14:27AM -0400, Alex Deucher wrote:
> As part of the amdgpu transition, we are moving to using database
> generated register and packet headers.  We have a number of options
> for formatting, some of which involve bitfields (don't worry there
> will also be shift/mask style headers as well which is mainly what we
> use in the code).  I think these formats are cleaner for a number of
> cases, however, as far as I know, C does not define the ordering of
> bits within bitfields.  That said, every compiler I've used seems to
> do what you would expect.  It makes coding a lot cleaner as less
> error-prone in certain cases.  Here are a couple of example of what
> I'm talking about:
> 
> A register example:
> 
> union GRPH_SWAP_CNTL {
>         struct {
> #if BIG_ENDIAN
>                 unsigned int                                 : 20;
>                 unsigned int             GRPH_ALPHA_CROSSBAR : 2;
>                 unsigned int              GRPH_BLUE_CROSSBAR : 2;
>                 unsigned int             GRPH_GREEN_CROSSBAR : 2;
>                 unsigned int               GRPH_RED_CROSSBAR : 2;
>                 unsigned int                                 : 2;
>                 unsigned int                GRPH_ENDIAN_SWAP : 2;
> #else
>                 unsigned int                GRPH_ENDIAN_SWAP : 2;
>                 unsigned int                                 : 2;
>                 unsigned int               GRPH_RED_CROSSBAR : 2;
>                 unsigned int             GRPH_GREEN_CROSSBAR : 2;
>                 unsigned int              GRPH_BLUE_CROSSBAR : 2;
>                 unsigned int             GRPH_ALPHA_CROSSBAR : 2;
>                 unsigned int                                 : 20;
> #endif
>         } bitfields, bits;
>         unsigned int u32All;
>         signed int i32All;
>         float f32All;
> };
> 
> A packet example:
> 
> typedef union PM4_TYPE_3_HEADER
> {
>     struct
>     {
> #if BIG_ENDIAN
>         unsigned int type      : 2; ///< packet identifier. It should
> be 3 for type 3 packets
>         unsigned int count     : 14;///< number of DWORDs - 1 in the
> information body.
>         unsigned int opcode    : 8; ///< IT opcode
>         unsigned int reserved1 : 6; ///< reserved
>         unsigned int shaderType: 1; ///< 0: Graphics, 1: Compute Shader
>         unsigned int predicate : 1; ///< predicated version of packet when set
> #else
>         unsigned int predicate : 1; ///< predicated version of packet when set
>         unsigned int shaderType: 1; ///< 0: Graphics, 1: Compute Shader
>         unsigned int reserved1 : 6; ///< reserved
>         unsigned int opcode    : 8; ///< IT opcode
>         unsigned int count     : 14;///< number of DWORDs - 1 in the
> information body.
>         unsigned int type      : 2; ///< packet identifier. It should
> be 3 for type 3 packets
> #endif
>     };
>     unsigned int u32All;
> } PM4_TYPE_3_HEADER;
> 
> //--------------------MEM_SEMAPHORE--------------------
> 
> enum MEM_SEMAPHORE_signal_type_enum {
> signal_type_mem_semaphore_SIGNAL_TYPE_INCREMENT_0 = 0,
> signal_type_mem_semaphore_SIGNAL_TYPE_WRITE_1 = 1 };
> enum MEM_SEMAPHORE_client_code_enum { client_code_mem_semaphore_CP_0 =
> 0, client_code_mem_semaphore_CB_1 = 1, client_code_mem_semaphore_DB_2
> = 2, client_code_mem_semaphore_RESERVED_3 = 3 };
> enum MEM_SEMAPHORE_sem_sel_enum {
> sem_sel_mem_semaphore_SIGNAL_SEMAPHORE_6 = 6,
> sem_sel_mem_semaphore_WAIT_SEMAPHORE_7 = 7 };
> 
> typedef struct _PM4_MEM_SEMAPHORE
> {
>     union
>     {
>         PM4_TYPE_3_HEADER   header;            ///header
>         unsigned int        ordinal1;
>     };
> 
>     union
>     {
>         struct
>     {
> #if BIG_ENDIAN
>             unsigned int address_lo:29;
>             unsigned int reserved1:3;
> #else
>             unsigned int reserved1:3;
>             unsigned int address_lo:29;
> #endif
>         } bitfields2;
>         unsigned int ordinal2;
>     };
> 
>     union
>     {
>         struct
>     {
> #if BIG_ENDIAN
>             MEM_SEMAPHORE_sem_sel_enum sem_sel:3;
>             unsigned int reserved4:3;
>             MEM_SEMAPHORE_client_code_enum client_code:2;

Do the enum really works with bitfield ? I am just lazy to go check c spec.

>             unsigned int reserved3:3;
>             MEM_SEMAPHORE_signal_type_enum signal_type:1;
>             unsigned int reserved2:3;
>             unsigned int use_mailbox:1;
>             unsigned int address_hi:16;
> #else
>             unsigned int address_hi:16;
>             unsigned int use_mailbox:1;
>             unsigned int reserved2:3;
>             MEM_SEMAPHORE_signal_type_enum signal_type:1;
>             unsigned int reserved3:3;
>             MEM_SEMAPHORE_client_code_enum client_code:2;
>             unsigned int reserved4:3;
>             MEM_SEMAPHORE_sem_sel_enum sem_sel:3;
> #endif
>         } bitfields3;
>         unsigned int ordinal3;
>     };
> 
> }  PM4MEM_SEMAPHORE, *PPM4MEM_SEMAPHORE;
> 
> Are there any strong objections to these sorts of structures?

I personally do not have any objection to using that though i would
rather avoid things like u32All (forgot the name for this kind of
variable naming convention) so just use all ie GRPH_SWAP_CNTL.all

Also using uint32_t instead of unsigned int would be saffer in my
mind even thought all arch we care about define unsigned int as
32 bits.

Finaly if you want to use that then the define with shift and mask
is pointless and risky, i rather have one and only one representation
than two different one that might diverge.

Cheers,
Jérôme

> Thanks,
> 
> Alex
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel