Re: [PATCH 09/66] drm/amdgpu/sdma5.2: convert to IP version checking

["Lazar, Lijo" <lijo.lazar@amd.com>]

On 9/22/2021 11:38 AM, Chen, Guchun wrote:
> [Public]
> 
>> +	switch (adev->ip_versions[SDMA0_HWIP]) {
>> +	case IP_VERSION(5, 2, 0):
>>    		adev->sdma.num_instances = 4;
>>    		break;
> Isn't the instance count also expected from discovery table?
> 
> This will be addressed in patch 54 of the series.
> 

Thanks Guchun! Saw that after a while :)

Alex,

Please ignore the comment. Apart from a few comments here and there, the 
patch series in general looks good to me.

Thanks,
Lijo

> Regards,
> Guchun
> 
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Lazar, Lijo
> Sent: Wednesday, September 22, 2021 1:56 PM
> To: Deucher, Alexander <Alexander.Deucher@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 09/66] drm/amdgpu/sdma5.2: convert to IP version checking
> 
> 
> 
> On 9/21/2021 11:36 PM, Alex Deucher wrote:
>> Use IP versions rather than asic_type to differentiate IP version
>> specific features.
>>
>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c | 48 +++++++++++++-------------
>>    1 file changed, 24 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
>> b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
>> index e4a96e7e386d..c5252f12eee9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
>> @@ -136,23 +136,23 @@ static int sdma_v5_2_init_microcode(struct
>> amdgpu_device *adev)
>>    
>>    	DRM_DEBUG("\n");
>>    
>> -	switch (adev->asic_type) {
>> -	case CHIP_SIENNA_CICHLID:
>> +	switch (adev->ip_versions[SDMA0_HWIP]) {
>> +	case IP_VERSION(5, 2, 0):
>>    		chip_name = "sienna_cichlid";
>>    		break;
>> -	case CHIP_NAVY_FLOUNDER:
>> +	case IP_VERSION(5, 2, 2):
>>    		chip_name = "navy_flounder";
>>    		break;
>> -	case CHIP_VANGOGH:
>> +	case IP_VERSION(5, 2, 1):
>>    		chip_name = "vangogh";
>>    		break;
>> -	case CHIP_DIMGREY_CAVEFISH:
>> +	case IP_VERSION(5, 2, 4):
>>    		chip_name = "dimgrey_cavefish";
>>    		break;
>> -	case CHIP_BEIGE_GOBY:
>> +	case IP_VERSION(5, 2, 5):
>>    		chip_name = "beige_goby";
>>    		break;
>> -	case CHIP_YELLOW_CARP:
>> +	case IP_VERSION(5, 2, 3):
>>    		chip_name = "yellow_carp";
>>    		break;
>>    	default:
>> @@ -174,7 +174,7 @@ static int sdma_v5_2_init_microcode(struct amdgpu_device *adev)
>>    		       (void *)&adev->sdma.instance[0],
>>    		       sizeof(struct amdgpu_sdma_instance));
>>    
>> -	if (amdgpu_sriov_vf(adev) && (adev->asic_type == CHIP_SIENNA_CICHLID))
>> +	if (amdgpu_sriov_vf(adev) && (adev->ip_versions[SDMA0_HWIP] ==
>> +IP_VERSION(5, 2, 0)))
>>    		return 0;
>>    
>>    	DRM_DEBUG("psp_load == '%s'\n",
>> @@ -1209,17 +1209,17 @@ static int sdma_v5_2_early_init(void *handle)
>>    {
>>    	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>    
>> -	switch (adev->asic_type) {
>> -	case CHIP_SIENNA_CICHLID:
>> +	switch (adev->ip_versions[SDMA0_HWIP]) {
>> +	case IP_VERSION(5, 2, 0):
>>    		adev->sdma.num_instances = 4;
>>    		break;
> Isn't the instance count also expected from discovery table?
> 
> Thanks,
> Lijo
> 
>> -	case CHIP_NAVY_FLOUNDER:
>> -	case CHIP_DIMGREY_CAVEFISH:
>> +	case IP_VERSION(5, 2, 2):
>> +	case IP_VERSION(5, 2, 4):
>>    		adev->sdma.num_instances = 2;
>>    		break;
>> -	case CHIP_VANGOGH:
>> -	case CHIP_BEIGE_GOBY:
>> -	case CHIP_YELLOW_CARP:
>> +	case IP_VERSION(5, 2, 1):
>> +	case IP_VERSION(5, 2, 5):
>> +	case IP_VERSION(5, 2, 3):
>>    		adev->sdma.num_instances = 1;
>>    		break;
>>    	default:
>> @@ -1547,7 +1547,7 @@ static void
>> sdma_v5_2_update_medium_grain_clock_gating(struct amdgpu_device *ade
>>    
>>    	for (i = 0; i < adev->sdma.num_instances; i++) {
>>    
>> -		if (adev->sdma.instance[i].fw_version < 70 && adev->asic_type == CHIP_VANGOGH)
>> +		if (adev->sdma.instance[i].fw_version < 70 &&
>> +adev->ip_versions[SDMA0_HWIP] == IP_VERSION(5, 2, 1))
>>    			adev->cg_flags &= ~AMD_CG_SUPPORT_SDMA_MGCG;
>>    
>>    		if (enable && (adev->cg_flags & AMD_CG_SUPPORT_SDMA_MGCG)) { @@
>> -1584,7 +1584,7 @@ static void
>> sdma_v5_2_update_medium_grain_light_sleep(struct amdgpu_device *adev
>>    
>>    	for (i = 0; i < adev->sdma.num_instances; i++) {
>>    
>> -		if (adev->sdma.instance[i].fw_version < 70 && adev->asic_type == CHIP_VANGOGH)
>> +		if (adev->sdma.instance[i].fw_version < 70 &&
>> +adev->ip_versions[SDMA0_HWIP] == IP_VERSION(5, 2, 1))
>>    			adev->cg_flags &= ~AMD_CG_SUPPORT_SDMA_LS;
>>    
>>    		if (enable && (adev->cg_flags & AMD_CG_SUPPORT_SDMA_LS)) { @@
>> -1613,13 +1613,13 @@ static int sdma_v5_2_set_clockgating_state(void *handle,
>>    	if (amdgpu_sriov_vf(adev))
>>    		return 0;
>>    
>> -	switch (adev->asic_type) {
>> -	case CHIP_SIENNA_CICHLID:
>> -	case CHIP_NAVY_FLOUNDER:
>> -	case CHIP_VANGOGH:
>> -	case CHIP_DIMGREY_CAVEFISH:
>> -	case CHIP_BEIGE_GOBY:
>> -	case CHIP_YELLOW_CARP:
>> +	switch (adev->ip_versions[SDMA0_HWIP]) {
>> +	case IP_VERSION(5, 2, 0):
>> +	case IP_VERSION(5, 2, 2):
>> +	case IP_VERSION(5, 2, 1):
>> +	case IP_VERSION(5, 2, 4):
>> +	case IP_VERSION(5, 2, 5):
>> +	case IP_VERSION(5, 2, 3):
>>    		sdma_v5_2_update_medium_grain_clock_gating(adev,
>>    				state == AMD_CG_STATE_GATE);
>>    		sdma_v5_2_update_medium_grain_light_sleep(adev,
>>