From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Christian_K=F6nig?= Subject: Re: [PATCH 1/3] drm/radeon: GPU virtual memory support v22 Date: Fri, 06 Jan 2012 15:46:52 +0100 Message-ID: <4F07095C.8090407@vodafone.de> References: <1325819467-2216-1-git-send-email-alexdeucher@gmail.com> <4F06EEB7.2070901@vodafone.de> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from outgoing.email.vodafone.de (outgoing.email.vodafone.de [139.7.28.128]) by gabe.freedesktop.org (Postfix) with ESMTP id 731449E79A for ; Fri, 6 Jan 2012 06:46:59 -0800 (PST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Alex Deucher Cc: Alex Deucher , Jerome Glisse , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On 06.01.2012 15:12, Alex Deucher wrote: > 2012/1/6 Christian K=F6nig: >> 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 flag= s: >>> */ >>> #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 =3D normal, + =3D higher priority, - =3D 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 (remov= e?) >> 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.