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

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

On 23.10.2012 14:44, Michel Dänzer wrote:
> 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;

Fixed.

>
>
>> -	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.
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änzer <michel.daenzer@amd.com>
Thanks for the review, going to send out a v2 of the patchset soon.

Christian.