Re: [PATCH 1/4] drm/radeon: fix and simplify pot argument checks

["Michel Dänzer" <michel@daenzer.net>]

On Die, 2012-10-23 at 14:23 +0200, Christian König 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önig <deathsimple@vodafone.de>
> ---
>  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/radeon/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 *cookie, 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)) == 0;
> +}

This could be simplified as

return (arg & (arg - 1)) == 0;


> -	radeon_vram_limit = radeon_vram_limit << 20;
> +	radeon_vram_limit = (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.


The commit message of patch 2 has a typo (vzmalloc instead of vzalloc),
other than that patches 2-4 are

Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer