Re: [PATCH] drm/amdgpu: add automatic per asic settings for gart_size

["Christian König" <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>]

Am 21.08.2017 um 22:48 schrieb Felix Kuehling:
> On 2017-08-21 12:00 PM, Alex Deucher wrote:
>> We need a larger gart for asics that do not support GPUVM on all
>> engines (e.g., MM) to make sure we have enough space for all
>> gtt buffers in physical mode.  Change the default size based on
>> the asic type.
>>
>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  2 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  4 ++--
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  4 ++--
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c   | 16 +++++++++++++++-
>>   4 files changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index ffb98bb..97d50cd 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -77,7 +77,7 @@
>>   extern int amdgpu_modeset;
>>   extern int amdgpu_vram_limit;
>>   extern int amdgpu_vis_vram_limit;
>> -extern unsigned amdgpu_gart_size;
>> +extern int amdgpu_gart_size;
>>   extern int amdgpu_gtt_size;
>>   extern int amdgpu_moverate;
>>   extern int amdgpu_benchmarking;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 1e66eda8..3c062a7 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -1063,11 +1063,11 @@ static void amdgpu_check_arguments(struct amdgpu_device *adev)
>>   		amdgpu_sched_jobs = roundup_pow_of_two(amdgpu_sched_jobs);
>>   	}
>>   
>> -	if (amdgpu_gart_size < 32) {
>> +	if (amdgpu_gart_size != -1 && amdgpu_gart_size < 32) {
>>   		/* gart size must be greater or equal to 32M */
>>   		dev_warn(adev->dev, "gart size (%d) too small\n",
>>   			 amdgpu_gart_size);
>> -		amdgpu_gart_size = 32;
>> +		amdgpu_gart_size = -1;
>>   	}
>>   
>>   	if (amdgpu_gtt_size != -1 && amdgpu_gtt_size < 32) {
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index 3eafe39..3f2143d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -76,7 +76,7 @@
>>   
>>   int amdgpu_vram_limit = 0;
>>   int amdgpu_vis_vram_limit = 0;
>> -unsigned amdgpu_gart_size = 256;
>> +int amdgpu_gart_size = -1; /* auto */
>>   int amdgpu_gtt_size = -1; /* auto */
>>   int amdgpu_moverate = -1; /* auto */
>>   int amdgpu_benchmarking = 0;
>> @@ -129,7 +129,7 @@ module_param_named(vramlimit, amdgpu_vram_limit, int, 0600);
>>   MODULE_PARM_DESC(vis_vramlimit, "Restrict visible VRAM for testing, in megabytes");
>>   module_param_named(vis_vramlimit, amdgpu_vis_vram_limit, int, 0444);
>>   
>> -MODULE_PARM_DESC(gartsize, "Size of PCIE/IGP gart to setup in megabytes (32, 64, etc.)");
>> +MODULE_PARM_DESC(gartsize, "Size of gart to setup in megabytes (32, 64, etc., -1=auto)");
> I would spell GART in capitals here.
>
>>   module_param_named(gartsize, amdgpu_gart_size, uint, 0600);
>>   
>>   MODULE_PARM_DESC(gttsize, "Size of the GTT domain in megabytes (-1 = auto)");
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>> index 94c1e2e..b9b9f68 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>> @@ -65,7 +65,21 @@
>>    */
>>   void amdgpu_gart_set_defaults(struct amdgpu_device *adev)
>>   {
>> -	adev->mc.gart_size = (uint64_t)amdgpu_gart_size << 20;
>> +	u64 gart_size;
>> +
>> +	if (amdgpu_gart_size == -1) {
>> +		/* make the GART larger for chips that
>> +		 * dont' support VM for all rings
>> +		 */
>> +		if (adev->asic_type <= CHIP_STONEY)
> I guess that means Polaris10 and later support VM for multimedia? I
> needed to go look at amd_shared.h to find that out. It would be more
> obvious like this:
>
>      if (adev->asic_type < CHIP_POLARIS10) ...
>
> Other than that, this patch is Reviewed-by: Felix Kuehling
> <Felix.Kuehling@amd.com>

Yeah, the code in uvd_v6_0.c to decide what to do is:
> static void uvd_v6_0_set_ring_funcs(struct amdgpu_device *adev)
> {
>         if (adev->asic_type >= CHIP_POLARIS10) {
>                 adev->uvd.ring.funcs = &uvd_v6_0_ring_vm_funcs;
>                 DRM_INFO("UVD is enabled in VM mode\n");
>         } else {
>                 adev->uvd.ring.funcs = &uvd_v6_0_ring_phys_funcs;
>                 DRM_INFO("UVD is enabled in physical mode\n");
>         }
> }

Not 100% sure about VCE, but Monk also pointed out internally that we 
should put that in a single place.

A check in amdgpu_uvd.h and one in amdgpu_vce.h sound reasonable to me.

Christian.

>
> Regards,
>    Felix
>
>> +			gart_size = 1024;
>> +		else
>> +			gart_size = 256;
>> +	} else {
>> +		gart_size = amdgpu_gart_size;
>> +	}
>> +
>> +	adev->mc.gart_size = gart_size << 20;
>>   }
>>   
>>   /**
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx