[PATCH] drm/amdgpu: disable c-states on xgmi perfmons

Kim, Jonathan Jonathan.Kim at amd.com
Thu Oct 17 17:58:11 UTC 2019


Thanks for the catch Philip.  I must have missed this with the renoir warnings.
I sent fix.

Jon

-----Original Message-----
From: Yang, Philip <Philip.Yang at amd.com> 
Sent: Thursday, October 17, 2019 1:40 PM
To: Quan, Evan <Evan.Quan at amd.com>; Kim, Jonathan <Jonathan.Kim at amd.com>; amd-gfx at lists.freedesktop.org
Cc: Kuehling, Felix <Felix.Kuehling at amd.com>
Subject: Re: [PATCH] drm/amdgpu: disable c-states on xgmi perfmons

I got compiler warnings after update this morning, because the variables are not initialized in df_v3_6_set_df_cstate() return failed path.

  CC [M]  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.o
   CC [M]  drivers/gpu/drm/amd/amdgpu/gfxhub_v1_1.o
/home/yangp/git/compute_staging/kernel/drivers/gpu/drm/amd/amdgpu/df_v3_6.c:96:8: 
warning: return type defaults to ‘int’ [-Wreturn-type]
  static df_v3_6_set_df_cstate(struct amdgpu_device *adev, int allow)
         ^~~~~~~~~~~~~~~~~~~~~
/home/yangp/git/compute_staging/kernel/drivers/gpu/drm/amd/amdgpu/df_v3_6.c: 
In function ‘df_v3_6_pmc_get_count’:
/home/yangp/git/compute_staging/kernel/drivers/gpu/drm/amd/amdgpu/df_v3_6.c:564:22: 
warning: ‘hi_val’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    *count  = ((hi_val | 0ULL) << 32) | (lo_val | 0ULL);
               ~~~~~~~~^~~~~~~
/home/yangp/git/compute_staging/kernel/drivers/gpu/drm/amd/amdgpu/df_v3_6.c:564:47: 
warning: ‘lo_val’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    *count  = ((hi_val | 0ULL) << 32) | (lo_val | 0ULL);
                                        ~~~~~~~~^~~~~~~
   CC [M]  drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.o


Regards,
Philip

On 2019-10-16 10:54 p.m., Quan, Evan wrote:
> Reviewed-by: Evan Quan <evan.quan at amd.com>
> 
>> -----Original Message-----
>> From: Kim, Jonathan <Jonathan.Kim at amd.com>
>> Sent: 2019年10月17日 10:06
>> To: amd-gfx at lists.freedesktop.org
>> Cc: Kuehling, Felix <Felix.Kuehling at amd.com>; Quan, Evan 
>> <Evan.Quan at amd.com>; Kim, Jonathan <Jonathan.Kim at amd.com>; Kim, 
>> Jonathan <Jonathan.Kim at amd.com>
>> Subject: [PATCH] drm/amdgpu: disable c-states on xgmi perfmons
>>
>> read or writes to df registers when gpu df is in c-states will result 
>> in hang.  df c-states should be disabled prior to read or writes then 
>> re-enabled after read or writes.
>>
>> v2: use old powerplay routines for vega20
>>
>> Change-Id: I6d5a83e4fe13e29c73dfb03a94fe7c611e867fec
>> Signed-off-by: Jonathan Kim <Jonathan.Kim at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/df_v3_6.c | 36
>> +++++++++++++++++++++++++++-
>>   1 file changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
>> b/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
>> index 16fbd2bc8ad1..f403c62c944e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
>> @@ -93,6 +93,21 @@ const struct attribute_group 
>> *df_v3_6_attr_groups[] = {
>>   		NULL
>>   };
>>
>> +static df_v3_6_set_df_cstate(struct amdgpu_device *adev, int allow) 
>> +{
>> +	int r = 0;
>> +
>> +	if (is_support_sw_smu(adev)) {
>> +		r = smu_set_df_cstate(&adev->smu, allow);
>> +	} else if (adev->powerplay.pp_funcs
>> +			&& adev->powerplay.pp_funcs->set_df_cstate) {
>> +		r = adev->powerplay.pp_funcs->set_df_cstate(
>> +			adev->powerplay.pp_handle, allow);
>> +	}
>> +
>> +	return r;
>> +}
>> +
>>   static uint64_t df_v3_6_get_fica(struct amdgpu_device *adev,
>>   				 uint32_t ficaa_val)
>>   {
>> @@ -102,6 +117,9 @@ static uint64_t df_v3_6_get_fica(struct 
>> amdgpu_device *adev,
>>   	address = adev->nbio.funcs->get_pcie_index_offset(adev);
>>   	data = adev->nbio.funcs->get_pcie_data_offset(adev);
>>
>> +	if (df_v3_6_set_df_cstate(adev, DF_CSTATE_DISALLOW))
>> +		return 0xFFFFFFFFFFFFFFFF;
>> +
>>   	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
>>   	WREG32(address,
>> smnDF_PIE_AON_FabricIndirectConfigAccessAddress3);
>>   	WREG32(data, ficaa_val);
>> @@ -114,6 +132,8 @@ static uint64_t df_v3_6_get_fica(struct 
>> amdgpu_device *adev,
>>
>>   	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
>>
>> +	df_v3_6_set_df_cstate(adev, DF_CSTATE_ALLOW);
>> +
>>   	return (((ficadh_val & 0xFFFFFFFFFFFFFFFF) << 32) | ficadl_val);
>>   }
>>
>> @@ -125,6 +145,9 @@ static void df_v3_6_set_fica(struct amdgpu_device 
>> *adev, uint32_t ficaa_val,
>>   	address = adev->nbio.funcs->get_pcie_index_offset(adev);
>>   	data = adev->nbio.funcs->get_pcie_data_offset(adev);
>>
>> +	if (df_v3_6_set_df_cstate(adev, DF_CSTATE_DISALLOW))
>> +		return;
>> +
>>   	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
>>   	WREG32(address,
>> smnDF_PIE_AON_FabricIndirectConfigAccessAddress3);
>>   	WREG32(data, ficaa_val);
>> @@ -134,8 +157,9 @@ static void df_v3_6_set_fica(struct amdgpu_device 
>> *adev, uint32_t ficaa_val,
>>
>>   	WREG32(address,
>> smnDF_PIE_AON_FabricIndirectConfigAccessDataHi3);
>>   	WREG32(data, ficadh_val);
>> -
>>   	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
>> +
>> +	df_v3_6_set_df_cstate(adev, DF_CSTATE_ALLOW);
>>   }
>>
>>   /*
>> @@ -153,12 +177,17 @@ static void df_v3_6_perfmon_rreg(struct 
>> amdgpu_device *adev,
>>   	address = adev->nbio.funcs->get_pcie_index_offset(adev);
>>   	data = adev->nbio.funcs->get_pcie_data_offset(adev);
>>
>> +	if (df_v3_6_set_df_cstate(adev, DF_CSTATE_DISALLOW))
>> +		return;
>> +
>>   	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
>>   	WREG32(address, lo_addr);
>>   	*lo_val = RREG32(data);
>>   	WREG32(address, hi_addr);
>>   	*hi_val = RREG32(data);
>>   	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
>> +
>> +	df_v3_6_set_df_cstate(adev, DF_CSTATE_ALLOW);
>>   }
>>
>>   /*
>> @@ -175,12 +204,17 @@ static void df_v3_6_perfmon_wreg(struct 
>> amdgpu_device *adev, uint32_t lo_addr,
>>   	address = adev->nbio.funcs->get_pcie_index_offset(adev);
>>   	data = adev->nbio.funcs->get_pcie_data_offset(adev);
>>
>> +	if (df_v3_6_set_df_cstate(adev, DF_CSTATE_DISALLOW))
>> +		return;
>> +
>>   	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
>>   	WREG32(address, lo_addr);
>>   	WREG32(data, lo_val);
>>   	WREG32(address, hi_addr);
>>   	WREG32(data, hi_val);
>>   	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
>> +
>> +	df_v3_6_set_df_cstate(adev, DF_CSTATE_ALLOW);
>>   }
>>
>>   /* get the number of df counters available */
>> --
>> 2.17.1
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 


More information about the amd-gfx mailing list