From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Christian_K=F6nig?= Subject: Re: [PATCH 1/4] drm/radeon: fix and simplify pot argument checks Date: Tue, 23 Oct 2012 15:40:45 +0200 Message-ID: <50869E5D.1060103@vodafone.de> References: <1350995014-4392-1-git-send-email-deathsimple@vodafone.de> <1350996241.11117.19.camel@thor.local> 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 941709F51A for ; Tue, 23 Oct 2012 06:40:50 -0700 (PDT) In-Reply-To: <1350996241.11117.19.camel@thor.local> 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: =?ISO-8859-1?Q?Michel_D=E4nzer?= Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On 23.10.2012 14:44, Michel D=E4nzer wrote: > On Die, 2012-10-23 at 14:23 +0200, Christian K=F6nig wrote: >> GART and VRAM size limits need to be a power of two. >> Fix values greater than 1GB and simplify those checks a bit. >> >> Signed-off-by: Christian K=F6nig >> --- >> drivers/gpu/drm/radeon/radeon_device.c | 55 ++++++++++++------------= -------- >> 1 file changed, 21 insertions(+), 34 deletions(-) >> >> diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/ra= deon/radeon_device.c >> index bd13ca0..3277aa1 100644 >> --- a/drivers/gpu/drm/radeon/radeon_device.c >> +++ b/drivers/gpu/drm/radeon/radeon_device.c >> @@ -835,6 +835,19 @@ static unsigned int radeon_vga_set_decode(void *coo= kie, bool state) >> } >> = >> /** >> + * radeon_check_pot_argument - check that argument is a power of two >> + * >> + * @arg: value to check >> + * >> + * Validates that a certain argument is a power of two (all asics). >> + * Returns true if argument is valid. >> + */ >> +static bool radeon_ckeck_pot_argument(int arg) >> +{ >> + return (arg & ((1 << __fls(arg)) - 1)) =3D=3D 0; >> +} > This could be simplified as > > return (arg & (arg - 1)) =3D=3D 0; Fixed. > > >> - radeon_vram_limit =3D radeon_vram_limit << 20; >> + radeon_vram_limit =3D (uint64_t)radeon_vram_limit << 20; > Does this cast have any effect without also changing the type of > radeon_vram_limit to something other than int? If the point is to allow > the shift to set the sign bit, I think casting to unsigned int or > uint32_t would be slightly less confusing, but a comment is probably > warranted anyway to prevent this from getting broken accidentally. Well, I actually focused on the GART limit instead, and just tried to = fix that in the same way. But wait a moment, modifying the radeon_vmram_limit here is quite faulty = anyway, cause this code gets executed for each card in the system. So = the vram limit won't work for the second card any more (and might = produce quite unusable results). Going to fix that. > > > The commit message of patch 2 has a typo (vzmalloc instead of vzalloc), > other than that patches 2-4 are > > Reviewed-by: Michel D=E4nzer Thanks for the review, going to send out a v2 of the patchset soon. Christian.