Re: [PATCH 3/3] drm/amdgpu: enable paging queue based on FW version

["Yang, Philip" <Philip.Yang-5C7GfCeVMHo@public.gmane.org>]

On 2018-11-19 3:57 p.m., Deucher, Alexander wrote:
>> -----Original Message-----
>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>> Yang, Philip
>> Sent: Monday, November 19, 2018 3:20 PM
>> To: amd-gfx@lists.freedesktop.org
>> Cc: Yang, Philip <Philip.Yang@amd.com>
>> Subject: [PATCH 3/3] drm/amdgpu: enable paging queue based on FW
>> version
>>
>> Based SDMA fw version to enable has_page_queue support. Have to move
>> sdma_v4_0_init_microcode from sw_init to early_init, to load firmware and
>> init fw_version before set_ring/buffer/vm_pte_funcs use it.
>>
>> Change-Id: Ife5d4659d28bc2a7012b48947b27e929749d87c1
>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 46 +++++++++++++++++----
>> -----
>>   1 file changed, 30 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>> b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>> index 4d873fd3242c..0a3b68dd49a0 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>> @@ -1447,23 +1447,44 @@ static void
>> sdma_v4_0_ring_emit_reg_wait(struct amdgpu_ring *ring, uint32_t reg,
>>   	sdma_v4_0_wait_reg_mem(ring, 0, 0, reg, 0, val, mask, 10);  }
>>
>> +static bool sdma_v4_0_fw_support_paging_queue(struct amdgpu_device
>> +*adev) {
>> +	uint fw_version = adev->sdma.instance[0].fw_version;
>> +
>> +	switch (adev->asic_type) {
>> +	case CHIP_VEGA10:
>> +		return fw_version >= 430;
>> +	case CHIP_VEGA12:
>> +		return fw_version >= 31;
>> +	case CHIP_VEGA20:
>> +		return fw_version >= 115;
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>>   static int sdma_v4_0_early_init(void *handle)  {
>>   	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>> +	int r;
>>
>> -	if (adev->asic_type == CHIP_RAVEN) {
>> +	if (adev->asic_type == CHIP_RAVEN)
>>   		adev->sdma.num_instances = 1;
>> -		adev->sdma.has_page_queue = false;
>> -	} else {
>> +	else
>>   		adev->sdma.num_instances = 2;
>> -		/* TODO: Page queue breaks driver reload under SRIOV */
>> -		if ((adev->asic_type == CHIP_VEGA10) &&
>> amdgpu_sriov_vf((adev)))
>> -			adev->sdma.has_page_queue = false;
>> -		else if (adev->asic_type != CHIP_VEGA20 &&
>> -				adev->asic_type != CHIP_VEGA12)
>> -			adev->sdma.has_page_queue = true;
>> +
>> +	r = sdma_v4_0_init_microcode(adev);
>> +	if (r) {
>> +		DRM_ERROR("Failed to load sdma firmware!\n");
>> +		return r;
> I think this should be ok.  As long as you've verified that sdam_v4_0_init_microcode() doesn't depend on any other init from another module like psp.  I took a quick look at the code and it seems like we should be ok.
> Acked-by: Alex Deucher <alexander.deucher@amd.com>
sdma_v4_0_init_microcode() reads fw binary file and setup fw version etc 
data structure. It doesn't depend on other init so it is fine to move it 
from sw_init() to early_init(). sdma_v4_0_load_microcode() will start fw 
from sdma_v4_0_start() which depends on psp, gmc etc. This happens after 
early_init()->sw_init()->hw_init().

Philip
>
>>   	}
>>
>> +	/* TODO: Page queue breaks driver reload under SRIOV */
>> +	if ((adev->asic_type == CHIP_VEGA10) &&
>> amdgpu_sriov_vf((adev)))
>> +		adev->sdma.has_page_queue = false;
>> +	else if (sdma_v4_0_fw_support_paging_queue(adev))
>> +		adev->sdma.has_page_queue = true;
>> +
>>   	sdma_v4_0_set_ring_funcs(adev);
>>   	sdma_v4_0_set_buffer_funcs(adev);
>>   	sdma_v4_0_set_vm_pte_funcs(adev);
>> @@ -1472,7 +1493,6 @@ static int sdma_v4_0_early_init(void *handle)
>>   	return 0;
>>   }
>>
>> -
>>   static int sdma_v4_0_sw_init(void *handle)  {
>>   	struct amdgpu_ring *ring;
>> @@ -1491,12 +1511,6 @@ static int sdma_v4_0_sw_init(void *handle)
>>   	if (r)
>>   		return r;
>>
>> -	r = sdma_v4_0_init_microcode(adev);
>> -	if (r) {
>> -		DRM_ERROR("Failed to load sdma firmware!\n");
>> -		return r;
>> -	}
>> -
>>   	for (i = 0; i < adev->sdma.num_instances; i++) {
>>   		ring = &adev->sdma.instance[i].ring;
>>   		ring->ring_obj = NULL;
>> --
>> 2.17.1
>>
>> _______________________________________________
>> 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