[PATCH 1/2] drm/amdgpu/gfx: fix MEC interrupt enablement for pipes != 0

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 2017-06-09 11:16 PM, Deucher, Alexander wrote:
>> -----Original Message-----
>> From: Andres Rodriguez [mailto:andresx7 at gmail.com]
>> Sent: Friday, June 09, 2017 7:49 PM
>> To: Alex Deucher; amd-gfx at lists.freedesktop.org
>> Cc: Deucher, Alexander
>> Subject: Re: [PATCH 1/2] drm/amdgpu/gfx: fix MEC interrupt enablement for
>> pipes != 0
>>
>>
>> I'm a little curious about the failures test cases. Is it related to a
>> specific ASIC?
> 
> Terrible performance on a range of gfx8 parts. >>
>> Using CPC_INT_CNTL seemed to be working well for me on polaris10 (I was
>> getting terrible perf on pipes 1+ without the original patch).
> 
> Weird,  not sure.  Maybe the indexing works on Polaris.
> 

Maybe it only works on some parts and not others?

This is also how current kfd and ROCM enable interrupts:

https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/radeon/radeon_kfd.c#L441

https://github.com/RadeonOpenCompute/ROCK-Kernel-Driver/blob/master/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c#L328

Might want to keep an eye on that if kfd ends up supporting those ASICs.

Regards,
Andres

> Alex
> 
>>
>> Regards,
>> Andres
>>
>> On 2017-06-09 08:49 AM, Alex Deucher wrote:
>>> The interrupt registers are not indexed.
>>>
>>> Fixes: 763a47b8e (drm/amdgpu: teach amdgpu how to enable interrupts
>> for any pipe v3)
>>> Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 57
>> +++++++++++++++++++++++----------
>>>    drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 57
>> +++++++++++++++++++++++----------
>>>    drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 59
>> +++++++++++++++++++++++++----------
>>>    3 files changed, 124 insertions(+), 49 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>> b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>>> index e30c7d0..fb0a94c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>>> @@ -5015,28 +5015,51 @@ static void
>> gfx_v7_0_set_compute_eop_interrupt_state(struct amdgpu_device *adev,
>>>    						     int me, int pipe,
>>>    						     enum
>> amdgpu_interrupt_state state)
>>>    {
>>> -	/* Me 0 is for graphics and Me 2 is reserved for HW scheduling
>>> -	 * So we should only really be configuring ME 1 i.e. MEC0
>>> +	u32 mec_int_cntl, mec_int_cntl_reg;
>>> +
>>> +	/*
>>> +	 * amdgpu controls only the first MEC. That's why this function only
>>> +	 * handles the setting of interrupts for this specific MEC. All other
>>> +	 * pipes' interrupts are set by amdkfd.
>>>    	 */
>>> -	if (me != 1) {
>>> -		DRM_ERROR("Ignoring request to enable interrupts for
>> invalid me:%d\n", me);
>>> -		return;
>>> -	}
>>>
>>> -	if (pipe >= adev->gfx.mec.num_pipe_per_mec) {
>>> -		DRM_ERROR("Ignoring request to enable interrupts for
>> invalid "
>>> -				"me:%d pipe:%d\n", pipe, me);
>>> +	if (me == 1) {
>>> +		switch (pipe) {
>>> +		case 0:
>>> +			mec_int_cntl_reg = mmCP_ME1_PIPE0_INT_CNTL;
>>> +			break;
>>> +		case 1:
>>> +			mec_int_cntl_reg = mmCP_ME1_PIPE1_INT_CNTL;
>>> +			break;
>>> +		case 2:
>>> +			mec_int_cntl_reg = mmCP_ME1_PIPE2_INT_CNTL;
>>> +			break;
>>> +		case 3:
>>> +			mec_int_cntl_reg = mmCP_ME1_PIPE3_INT_CNTL;
>>> +			break;
>>> +		default:
>>> +			DRM_DEBUG("invalid pipe %d\n", pipe);
>>> +			return;
>>> +		}
>>> +	} else {
>>> +		DRM_DEBUG("invalid me %d\n", me);
>>>    		return;
>>>    	}
>>>
>>> -	mutex_lock(&adev->srbm_mutex);
>>> -	cik_srbm_select(adev, me, pipe, 0, 0);
>>> -
>>> -	WREG32_FIELD(CPC_INT_CNTL, TIME_STAMP_INT_ENABLE,
>>> -			state == AMDGPU_IRQ_STATE_DISABLE ? 0 : 1);
>>> -
>>> -	cik_srbm_select(adev, 0, 0, 0, 0);
>>> -	mutex_unlock(&adev->srbm_mutex);
>>> +	switch (state) {
>>> +	case AMDGPU_IRQ_STATE_DISABLE:
>>> +		mec_int_cntl = RREG32(mec_int_cntl_reg);
>>> +		mec_int_cntl &=
>> ~CP_INT_CNTL_RING0__TIME_STAMP_INT_ENABLE_MASK;
>>> +		WREG32(mec_int_cntl_reg, mec_int_cntl);
>>> +		break;
>>> +	case AMDGPU_IRQ_STATE_ENABLE:
>>> +		mec_int_cntl = RREG32(mec_int_cntl_reg);
>>> +		mec_int_cntl |=
>> CP_INT_CNTL_RING0__TIME_STAMP_INT_ENABLE_MASK;
>>> +		WREG32(mec_int_cntl_reg, mec_int_cntl);
>>> +		break;
>>> +	default:
>>> +		break;
>>> +	}
>>>    }
>>>
>>>    static int gfx_v7_0_set_priv_reg_fault_state(struct amdgpu_device
>> *adev,
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>> b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>>> index 6e541af..1a75ab1 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>>> @@ -6610,26 +6610,51 @@ static void
>> gfx_v8_0_set_compute_eop_interrupt_state(struct amdgpu_device *adev,
>>>    						     int me, int pipe,
>>>    						     enum
>> amdgpu_interrupt_state state)
>>>    {
>>> -	/* Me 0 is reserved for graphics */
>>> -	if (me < 1 || me > adev->gfx.mec.num_mec) {
>>> -		DRM_ERROR("Ignoring request to enable interrupts for
>> invalid me:%d\n", me);
>>> -		return;
>>> -	}
>>> +	u32 mec_int_cntl, mec_int_cntl_reg;
>>>
>>> -	if (pipe >= adev->gfx.mec.num_pipe_per_mec) {
>>> -		DRM_ERROR("Ignoring request to enable interrupts for
>> invalid "
>>> -				"me:%d pipe:%d\n", pipe, me);
>>> +	/*
>>> +	 * amdgpu controls only the first MEC. That's why this function only
>>> +	 * handles the setting of interrupts for this specific MEC. All other
>>> +	 * pipes' interrupts are set by amdkfd.
>>> +	 */
>>> +
>>> +	if (me == 1) {
>>> +		switch (pipe) {
>>> +		case 0:
>>> +			mec_int_cntl_reg = mmCP_ME1_PIPE0_INT_CNTL;
>>> +			break;
>>> +		case 1:
>>> +			mec_int_cntl_reg = mmCP_ME1_PIPE1_INT_CNTL;
>>> +			break;
>>> +		case 2:
>>> +			mec_int_cntl_reg = mmCP_ME1_PIPE2_INT_CNTL;
>>> +			break;
>>> +		case 3:
>>> +			mec_int_cntl_reg = mmCP_ME1_PIPE3_INT_CNTL;
>>> +			break;
>>> +		default:
>>> +			DRM_DEBUG("invalid pipe %d\n", pipe);
>>> +			return;
>>> +		}
>>> +	} else {
>>> +		DRM_DEBUG("invalid me %d\n", me);
>>>    		return;
>>>    	}
>>>
>>> -	mutex_lock(&adev->srbm_mutex);
>>> -	vi_srbm_select(adev, me, pipe, 0, 0);
>>> -
>>> -	WREG32_FIELD(CPC_INT_CNTL, TIME_STAMP_INT_ENABLE,
>>> -			state == AMDGPU_IRQ_STATE_DISABLE ? 0 : 1);
>>> -
>>> -	vi_srbm_select(adev, 0, 0, 0, 0);
>>> -	mutex_unlock(&adev->srbm_mutex);
>>> +	switch (state) {
>>> +	case AMDGPU_IRQ_STATE_DISABLE:
>>> +		mec_int_cntl = RREG32(mec_int_cntl_reg);
>>> +		mec_int_cntl &=
>> ~CP_INT_CNTL_RING0__TIME_STAMP_INT_ENABLE_MASK;
>>> +		WREG32(mec_int_cntl_reg, mec_int_cntl);
>>> +		break;
>>> +	case AMDGPU_IRQ_STATE_ENABLE:
>>> +		mec_int_cntl = RREG32(mec_int_cntl_reg);
>>> +		mec_int_cntl |=
>> CP_INT_CNTL_RING0__TIME_STAMP_INT_ENABLE_MASK;
>>> +		WREG32(mec_int_cntl_reg, mec_int_cntl);
>>> +		break;
>>> +	default:
>>> +		break;
>>> +	}
>>>    }
>>>
>>>    static int gfx_v8_0_set_priv_reg_fault_state(struct amdgpu_device
>> *adev,
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>>> index 375620a..e9dd2c1 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>>> @@ -3982,26 +3982,53 @@ static void
>> gfx_v9_0_set_compute_eop_interrupt_state(struct amdgpu_device *adev,
>>>    						     int me, int pipe,
>>>    						     enum
>> amdgpu_interrupt_state state)
>>>    {
>>> -	/* Me 0 is reserved for graphics */
>>> -	if (me < 1 || me > adev->gfx.mec.num_mec) {
>>> -		DRM_ERROR("Ignoring request to enable interrupts for
>> invalid me:%d\n", me);
>>> -		return;
>>> -	}
>>> +	u32 mec_int_cntl, mec_int_cntl_reg;
>>>
>>> -	if (pipe >= adev->gfx.mec.num_pipe_per_mec) {
>>> -		DRM_ERROR("Ignoring request to enable interrupts for
>> invalid "
>>> -				"me:%d pipe:%d\n", pipe, me);
>>> +	/*
>>> +	 * amdgpu controls only the first MEC. That's why this function only
>>> +	 * handles the setting of interrupts for this specific MEC. All other
>>> +	 * pipes' interrupts are set by amdkfd.
>>> +	 */
>>> +
>>> +	if (me == 1) {
>>> +		switch (pipe) {
>>> +		case 0:
>>> +			mec_int_cntl_reg = SOC15_REG_OFFSET(GC, 0,
>> mmCP_ME1_PIPE0_INT_CNTL);
>>> +			break;
>>> +		case 1:
>>> +			mec_int_cntl_reg = SOC15_REG_OFFSET(GC, 0,
>> mmCP_ME1_PIPE1_INT_CNTL);
>>> +			break;
>>> +		case 2:
>>> +			mec_int_cntl_reg = SOC15_REG_OFFSET(GC, 0,
>> mmCP_ME1_PIPE2_INT_CNTL);
>>> +			break;
>>> +		case 3:
>>> +			mec_int_cntl_reg = SOC15_REG_OFFSET(GC, 0,
>> mmCP_ME1_PIPE3_INT_CNTL);
>>> +			break;
>>> +		default:
>>> +			DRM_DEBUG("invalid pipe %d\n", pipe);
>>> +			return;
>>> +		}
>>> +	} else {
>>> +		DRM_DEBUG("invalid me %d\n", me);
>>>    		return;
>>>    	}
>>>
>>> -	mutex_lock(&adev->srbm_mutex);
>>> -	soc15_grbm_select(adev, me, pipe, 0, 0);
>>> -
>>> -	WREG32_FIELD(CPC_INT_CNTL, TIME_STAMP_INT_ENABLE,
>>> -			state == AMDGPU_IRQ_STATE_DISABLE ? 0 : 1);
>>> -
>>> -	soc15_grbm_select(adev, 0, 0, 0, 0);
>>> -	mutex_unlock(&adev->srbm_mutex);
>>> +	switch (state) {
>>> +	case AMDGPU_IRQ_STATE_DISABLE:
>>> +		mec_int_cntl = RREG32(mec_int_cntl_reg);
>>> +		mec_int_cntl = REG_SET_FIELD(mec_int_cntl,
>> CP_ME1_PIPE0_INT_CNTL,
>>> +					     TIME_STAMP_INT_ENABLE, 0);
>>> +		WREG32(mec_int_cntl_reg, mec_int_cntl);
>>> +		break;
>>> +	case AMDGPU_IRQ_STATE_ENABLE:
>>> +		mec_int_cntl = RREG32(mec_int_cntl_reg);
>>> +		mec_int_cntl = REG_SET_FIELD(mec_int_cntl,
>> CP_ME1_PIPE0_INT_CNTL,
>>> +					     TIME_STAMP_INT_ENABLE, 1);
>>> +		WREG32(mec_int_cntl_reg, mec_int_cntl);
>>> +		break;
>>> +	default:
>>> +		break;
>>> +	}
>>>    }
>>>
>>>    static int gfx_v9_0_set_priv_reg_fault_state(struct amdgpu_device
>> *adev,
>>>


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux