On Wed, Sep 12, 2018 at 10:06 PM Felix Kuehling <felix.kuehling at amd.com> wrote:
>
> On 2018-09-12 09:55 PM, Alex Deucher wrote:
> > On Wed, Sep 12, 2018 at 9:45 PM Felix Kuehling <Felix.Kuehling at amd.com> wrote:
> >> From: Emily Deng <Emily.Deng at amd.com>
> >>
> >> Correct the format
> >>
> >> For vega10 sriov, the sdma doorbell must be fixed as follow to keep the
> >> same setting with host driver, or it will happen conflicts.
> >>
> >> Signed-off-by: Emily Deng <Emily.Deng at amd.com>
> >> Acked-by: Alex Deucher <alexander.deucher at amd.com>
> >> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
> >> ---
> >> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 9 +++++++++
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 27 +++++++++++++++++++--------
> >> drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 12 +++++++++---
> >> 3 files changed, 37 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >> index afa9e77..e60de88 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >> @@ -420,6 +420,15 @@ typedef enum _AMDGPU_DOORBELL64_ASSIGNMENT
> >> AMDGPU_DOORBELL64_sDMA_ENGINE1 = 0xE8,
> >> AMDGPU_DOORBELL64_sDMA_HI_PRI_ENGINE1 = 0xE9,
> >>
> >> + /* For vega10 sriov, the sdma doorbell must be fixed as follow
> >> + * to keep the same setting with host driver, or it will
> >> + * happen conflicts
> >> + */
> >> + AMDGPU_VEGA10_DOORBELL64_sDMA_ENGINE0 = 0xF0,
> >> + AMDGPU_VEGA10_DOORBELL64_sDMA_HI_PRI_ENGINE0 = 0xF1,
> >> + AMDGPU_VEGA10_DOORBELL64_sDMA_ENGINE1 = 0xF2,
> >> + AMDGPU_VEGA10_DOORBELL64_sDMA_HI_PRI_ENGINE1 = 0xF3,
> >> +
> >> /* Interrupt handler */
> >> AMDGPU_DOORBELL64_IH = 0xF4, /* For legacy interrupt ring buffer */
> >> AMDGPU_DOORBELL64_IH_RING1 = 0xF5, /* For page migration request log */
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> >> index bf0b012..7a165a9 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> >> @@ -181,14 +181,25 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
> >> * process in case of 64-bit doorbells so we
> >> * can use each doorbell assignment twice.
> >> */
> >> - gpu_resources.sdma_doorbell[0][i] =
> >> - AMDGPU_DOORBELL64_sDMA_ENGINE0 + (i >> 1);
> >> - gpu_resources.sdma_doorbell[0][i+1] =
> >> - AMDGPU_DOORBELL64_sDMA_ENGINE0 + 0x200 + (i >> 1);
> >> - gpu_resources.sdma_doorbell[1][i] =
> >> - AMDGPU_DOORBELL64_sDMA_ENGINE1 + (i >> 1);
> >> - gpu_resources.sdma_doorbell[1][i+1] =
> >> - AMDGPU_DOORBELL64_sDMA_ENGINE1 + 0x200 + (i >> 1);
> >> + if (adev->asic_type == CHIP_VEGA10) {
> >> + gpu_resources.sdma_doorbell[0][i] =
> >> + AMDGPU_VEGA10_DOORBELL64_sDMA_ENGINE0 + (i >> 1);
> >> + gpu_resources.sdma_doorbell[0][i+1] =
> >> + AMDGPU_VEGA10_DOORBELL64_sDMA_ENGINE0 + 0x200 + (i >> 1);
> >> + gpu_resources.sdma_doorbell[1][i] =
> >> + AMDGPU_VEGA10_DOORBELL64_sDMA_ENGINE1 + (i >> 1);
> >> + gpu_resources.sdma_doorbell[1][i+1] =
> >> + AMDGPU_VEGA10_DOORBELL64_sDMA_ENGINE1 + 0x200 + (i >> 1);
> >> + } else {
> >> + gpu_resources.sdma_doorbell[0][i] =
> >> + AMDGPU_DOORBELL64_sDMA_ENGINE0 + (i >> 1);
> >> + gpu_resources.sdma_doorbell[0][i+1] =
> >> + AMDGPU_DOORBELL64_sDMA_ENGINE0 + 0x200 + (i >> 1);
> >> + gpu_resources.sdma_doorbell[1][i] =
> >> + AMDGPU_DOORBELL64_sDMA_ENGINE1 + (i >> 1);
> >> + gpu_resources.sdma_doorbell[1][i+1] =
> >> + AMDGPU_DOORBELL64_sDMA_ENGINE1 + 0x200 + (i >> 1);
> >> + }
> > It would probably make more sense to reverse the conditions here so we
> > retain the old behavior for all previous non-vega20 asics rather than
> > just vega10. E.g.,
> >
> > if (vega20) {
> > // use new doorbell mapping
> > } else {
> > // use the old doorbell mapping
> > }
> >
> > Same thing below.
>
> This code is only applicable to GFXv9 and later GPUs with 64-bit
> doorbells. It does not apply to GFXv8 and older GPUs anyway. The new
> enum names AMDGPU_VEGA10_DOORBELL64_... also imply that we preserve the
> old behaviour on Vega10 only. I think the assumption is that future GPUs
> will also need bigger doorbell assignments.
>
> Do any of the other GFXv9 GPUs support SRIOV? (Vega12, Raven, etc.)
Not at the moment, but if we do end up doing so, we'll need to
remember to change the code. Not a big deal either way, just thought
it was worth mentioning.
Alex
>
> Regards,
> Felix
>
> >
> > Alex
> >
> >> }
> >> /* Doorbells 0x0e0-0ff and 0x2e0-2ff are reserved for
> >> * SDMA, IH and VCN. So don't use them for the CP.
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> >> index df13840..6265361 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> >> @@ -1299,9 +1299,15 @@ static int sdma_v4_0_sw_init(void *handle)
> >> DRM_INFO("use_doorbell being set to: [%s]\n",
> >> ring->use_doorbell?"true":"false");
> >>
> >> - ring->doorbell_index = (i == 0) ?
> >> - (AMDGPU_DOORBELL64_sDMA_ENGINE0 << 1) //get DWORD offset
> >> - : (AMDGPU_DOORBELL64_sDMA_ENGINE1 << 1); // get DWORD offset
> >> + if (adev->asic_type == CHIP_VEGA10)
> >> + ring->doorbell_index = (i == 0) ?
> >> + (AMDGPU_VEGA10_DOORBELL64_sDMA_ENGINE0 << 1) //get DWORD offset
> >> + : (AMDGPU_VEGA10_DOORBELL64_sDMA_ENGINE1 << 1); // get DWORD offset
> >> + else
> >> + ring->doorbell_index = (i == 0) ?
> >> + (AMDGPU_DOORBELL64_sDMA_ENGINE0 << 1) //get DWORD offset
> >> + : (AMDGPU_DOORBELL64_sDMA_ENGINE1 << 1); // get DWORD offset
> >> +
> >>
> >> sprintf(ring->name, "sdma%d", i);
> >> r = amdgpu_ring_init(adev, ring, 1024,
> >> --
> >> 2.7.4
> >>
> >> _______________________________________________
> >> amd-gfx mailing list
> >> amd-gfx at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
