| Message ID | 1350995014-4392-1-git-send-email-deathsimple@vodafone.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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.
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; +} + +/** * radeon_check_arguments - validate module params * * @rdev: radeon_device pointer @@ -845,52 +858,26 @@ static unsigned int radeon_vga_set_decode(void *cookie, bool state) static void radeon_check_arguments(struct radeon_device *rdev) { /* vramlimit must be a power of two */ - switch (radeon_vram_limit) { - case 0: - case 4: - case 8: - case 16: - case 32: - case 64: - case 128: - case 256: - case 512: - case 1024: - case 2048: - case 4096: - break; - default: + if (!radeon_ckeck_pot_argument(radeon_vram_limit)) { dev_warn(rdev->dev, "vram limit (%d) must be a power of 2\n", radeon_vram_limit); radeon_vram_limit = 0; - break; } - radeon_vram_limit = radeon_vram_limit << 20; + radeon_vram_limit = (uint64_t)radeon_vram_limit << 20; + /* gtt size must be power of two and greater or equal to 32M */ - switch (radeon_gart_size) { - case 4: - case 8: - case 16: + if (radeon_gart_size < 32) { dev_warn(rdev->dev, "gart size (%d) too small forcing to 512M\n", radeon_gart_size); radeon_gart_size = 512; - break; - case 32: - case 64: - case 128: - case 256: - case 512: - case 1024: - case 2048: - case 4096: - break; - default: + + } else if (!radeon_ckeck_pot_argument(radeon_gart_size)) { dev_warn(rdev->dev, "gart size (%d) must be a power of 2\n", radeon_gart_size); radeon_gart_size = 512; - break; } - rdev->mc.gtt_size = radeon_gart_size * 1024 * 1024; + rdev->mc.gtt_size = (uint64_t)radeon_gart_size << 20; + /* AGP mode can only be -1, 1, 2, 4, 8 */ switch (radeon_agpmode) { case -1:
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(-)