Re: [PATCH 1/3] drm/radeon: GPU virtual memory support v22

["Christian König" <deathsimple@vodafone.de>]

On 06.01.2012 15:12, Alex Deucher wrote:
> 2012/1/6 Christian König<deathsimple@vodafone.de>:
>> On -10.01.-28163 20:59, alexdeucher@gmail.com wrote:
>> [SNIP]
>>
>>>   #define RADEON_CHUNK_ID_RELOCS        0x01
>>>   #define RADEON_CHUNK_ID_IB    0x02
>>>   #define RADEON_CHUNK_ID_FLAGS 0x03
>>>
>>>   /* The first dword of RADEON_CHUNK_ID_FLAGS is a uint32 of these flags:
>>> */
>>>   #define RADEON_CS_KEEP_TILING_FLAGS 0x01
>>> +#define RADEON_CS_USE_VM            0x02
>>> +/* The second dword of RADEON_CHUNK_ID_FLAGS is a uint32 that sets the
>>> ring type */
>>> +#define RADEON_CS_RING_GFX          0
>>> +#define RADEON_CS_RING_COMPUTE      1
>>> +/* The third dword of RADEON_CHUNK_ID_FLAGS is a sint32 that sets the
>>> priority */
>>> +/* 0 = normal, + = higher priority, - = lower priority */
>>> +struct drm_radeon_cs_ring_priority {
>>> +       int32_t                 priority;
>>> +};
>> Sorry, missed that one before, the "struct drm_radeon_cs_ring_priority" is
>> pretty much pointless.
> I didn't have a struct for priority before, but someone suggested it
> would be clearer what the 3rd dword was used for if I added a struct.
> Basically just treat the third flag dword as a signed int32.
Yeah that suggestion came from me, but what I had in mind was a 
structure for the whole flags chunk, instead of just the priority field.

>> My comment was going more into that direction:
>> struct drm_radeon_cs_flags {
>>     uint32_t      flags;
>>     uint32_t      ring_type;
>>     int32_t        priority;
>> };
>>
>> Anyway, the patch is finally committed, but I think we should fix (remove?)
>> that before it goes further upstream.
> Would there be any issues if we need to extend the stuct to add an
> extra field later?  Also, not all of the fields are necessary.  You
> can just allocate 1 dword if you just need the flags dword.  I've fine
> to just drop the struct.
>
Well, using structs as interface definitions has always suffered from 
those problems, e. g. for compatibility you can only extend it at the 
end, members might have ambiguous sizes, members might be packed 
differently.....  I'm still just prefering them because they are machine 
readable, instead of human readable comments, and machines tend to 
produce less bugs than humans while calculations memory offsets...

But I don't have enough experience with kernel interfaces to favor one 
style of coding over another, so if you think it's ok to drop it then 
just go ahead. I just wanted to note that there was a misunderstanding.

Christian.