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

["Christian König" <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Am 19.11.18 um 22:16 schrieb Yang, Philip:
> 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().

Yeah, that sounds like it should work.

Patches #1 and #2 are Reviewed-by: Christian König 
<christian.koenig@amd.com>

Patch #3 is Acked-by: Christian König <christian.koenig@amd.com>

Thanks for taking care of that,
Christian.


>
> 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

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