[PATCH 1/2] drm/amdgpu/gfx: fix MEC interrupt enablement for pipes != 0
Andres Rodriguez
andresx7 at gmail.com
Sat Jun 10 04:00:12 UTC 2017
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,
>>>
More information about the amd-gfx
mailing list