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