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

[Alex Deucher]

2012/1/6 Christian König <deathsimple at vodafone.de>:
> On 06.01.2012 15:12, Alex Deucher wrote:
>>
>> 2012/1/6 Christian König<deathsimple at vodafone.de>:
>>>
>>> On -10.01.-28163 20:59, alexdeucher at 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.

I just went ahead and dropped it for now.  I'm open too suggestions
for better approaches though.

Alex

>
> Christian.