> -----Original Message----- > From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf > Of Tom St Denis > Sent: Friday, June 09, 2017 9:19 AM > To: amd-gfx at lists.freedesktop.org > Subject: Re: [PATCH 1/2] drm/amdgpu/gfx: fix MEC interrupt enablement for > pipes != 0 > > On 09/06/17 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; > > + } > > > Could use WREG32_FIELD() here to simplify these. That doesn't work if you have variable register names as we do in this case. Alex > > Cheers, > Tom > _______________________________________________ > amd-gfx mailing list > amd-gfx at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
