RE: [PATCH 13/23] drm/amdgpu/mxgpu: add support for mailbox communication

["Liu, Shaoyun" <Shaoyun.Liu-5C7GfCeVMHo@public.gmane.org>]

Hi, 
I didn't check others changes in the  series.  For these two specific changes regarding  the mailbox communication , I have  two concerns 
1. This code should only works on SRIOV enabled VF , in your code seems you use  adev->priv_data  for the check , I would like to have  some more meaningful name than priv_data if possible . 
2. How you handle the  asic specific  MAILBOX programming ?   ex, for Vega10 support . 

Regards
Shaoyun.liu

-----Original Message-----
From: Yu, Xiangliang 
Sent: Tuesday, December 20, 2016 1:06 AM
To: Alex Deucher
Cc: amd-gfx list; dl.SRDC_SW_GPUVirtualization; Liu, Shaoyun
Subject: RE: [PATCH 13/23] drm/amdgpu/mxgpu: add support for mailbox communication


> -----Original Message-----
> From: Alex Deucher [mailto:alexdeucher@gmail.com]
> Sent: Tuesday, December 20, 2016 7:25 AM
> To: Yu, Xiangliang <Xiangliang.Yu@amd.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>; 
> dl.SRDC_SW_GPUVirtualization <dl.SRDC_SW_GPUVirtualization@amd.com>; 
> Liu, Shaoyun <Shaoyun.Liu@amd.com>
> Subject: Re: [PATCH 13/23] drm/amdgpu/mxgpu: add support for mailbox 
> communication
> 
> On Sat, Dec 17, 2016 at 11:16 AM, Xiangliang Yu 
> <Xiangliang.Yu@amd.com>
> wrote:
> > GPU guest driver send mailbox messages to hyperverisor to request 
> > full access to all of registers and release it when access is done.
> >
> > Signed-off-by: Xiangliang Yu <Xiangliang.Yu@amd.com>
> > Signed-off-by: shaoyunl <Shaoyun.Liu@amd.com>
> > ---
> >  drivers/gpu/drm/amd/mxgpu/amd_mxgpu.h |   3 +
> >  drivers/gpu/drm/amd/mxgpu/mxgpu_mb.c  | 128
> > ++++++++++++++++++++++++++++++++++
> >  2 files changed, 131 insertions(+)
> >  create mode 100644 drivers/gpu/drm/amd/mxgpu/mxgpu_mb.c
> >
> > diff --git a/drivers/gpu/drm/amd/mxgpu/amd_mxgpu.h
> > b/drivers/gpu/drm/amd/mxgpu/amd_mxgpu.h
> > index cc3123b..54e7b31 100644
> > --- a/drivers/gpu/drm/amd/mxgpu/amd_mxgpu.h
> > +++ b/drivers/gpu/drm/amd/mxgpu/amd_mxgpu.h
> > @@ -85,4 +85,7 @@ extern void amd_xgpu_free(struct amd_xgpu *xgpu);
> >
> >  extern int xgpu_allocate_csa(struct amd_xgpu *xgpu);  extern void 
> > xgpu_destroy_csa(struct amd_xgpu_csa *csa);
> > +
> > +extern int xgpu_request_full_gpu_access(struct amdgpu_device *adev, 
> > +bool init); extern int xgpu_release_full_gpu_access(struct
> > +amdgpu_device *adev, bool init);
> >  #endif
> > diff --git a/drivers/gpu/drm/amd/mxgpu/mxgpu_mb.c
> > b/drivers/gpu/drm/amd/mxgpu/mxgpu_mb.c
> > new file mode 100644
> > index 0000000..b74b501
> > --- /dev/null
> > +++ b/drivers/gpu/drm/amd/mxgpu/mxgpu_mb.c
> > @@ -0,0 +1,128 @@
> > +/*
> > + * Copyright 2016 Advanced Micro Devices, Inc.
> > + *
> > + * Permission is hereby granted, free of charge, to any person 
> > +obtaining a
> > + * copy of this software and associated documentation files (the 
> > +"Software"),
> > + * to deal in the Software without restriction, including without 
> > +limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, 
> > +sublicense,
> > + * and/or sell copies of the Software, and to permit persons to 
> > +whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be 
> > +included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
> KIND,
> > +EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
> > +MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN
> NO EVENT
> > +SHALL
> > + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, 
> > +DAMAGES OR
> > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR 
> > +OTHERWISE,
> > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR
> THE USE
> > +OR
> > + * OTHER DEALINGS IN THE SOFTWARE.
> > + *
> > + * Authors: Xiangliang.Yu@amd.com
> > + */
> > +#include "amd_mxgpu.h"
> > +#include "bif/bif_5_0_d.h"
> > +#include "bif/bif_5_0_sh_mask.h"
> > +
> > +/* mailbox messages request */
> > +enum idh_request {
> > +       IDH_REQ_GPU_INIT_ACCESS = 1,
> > +       IDH_REL_GPU_INIT_ACCESS,
> > +       IDH_REQ_GPU_FINI_ACCESS,
> > +       IDH_REL_GPU_FINI_ACCESS,
> > +       IDH_REQ_GPU_RESET_ACCESS
> > +};
> > +
> > +/* mailbox messages data */
> > +enum idh_event {
> > +       IDH_CLR_MSG_BUF = 0,
> > +       IDH_READY_TO_ACCESS_GPU,
> > +       IDH_FLR_NOTIFICATION,
> > +       IDH_FLR_NOTIFICATION_CMPL
> > +};
> > +
> > +/* mailbox state */
> > +enum mail_state {
> > +       AMDGPU_START_INIT = 1,
> > +       AMDGPU_END_INIT,
> > +       AMDGPU_START_FINI,
> > +       AMDGPU_END_FINI,
> > +};
> > +
> > +/*
> > + * Mailbox communicate between PF and VF  */ static int 
> > +xgpu_request_reg_access(struct amdgpu_device *adev,
> > +                                  enum mail_state state) {
> > +       u32 temp, timeout = 150000;
> > +       u32 req = IDH_REQ_GPU_INIT_ACCESS;
> > +
> > +       if (state == AMDGPU_START_FINI)
> > +               req = IDH_REQ_GPU_FINI_ACCESS;
> > +       else if (state != AMDGPU_START_INIT)
> > +               return -EINVAL;
> > +
> > +       WREG32(mmMAILBOX_MSGBUF_TRN_DW0, req);
> > +       WREG32(mmMAILBOX_CONTROL,
> > +                       REG_SET_FIELD(RREG32(mmMAILBOX_CONTROL),
> > +                       MAILBOX_CONTROL, TRN_MSG_VALID, 1));
> > +
> > +       /* wait respond from PF */
> > +       while (true) {
> > +               temp = RREG32(mmMAILBOX_MSGBUF_RCV_DW0);
> > +               if (temp == IDH_READY_TO_ACCESS_GPU)
> > +                       break;
> > +               if (timeout == 0) {
> > +                       pr_err("amdgpu: VF request GPU access failed(0x%x) \
> > +                               in state %d\n", temp, state);
> > +                       return -ETIME;
> > +               }
> > +               msleep(20);
> > +               timeout -= 20;
> > +       }
> > +       return 0;
> > +}
> > +
> > +static int xgpu_release_reg_access(struct amdgpu_device *adev,
> > +                                  enum mail_state state) {
> > +       u32 rel = IDH_REL_GPU_INIT_ACCESS;
> > +
> > +       if (state == AMDGPU_END_FINI)
> > +               rel = IDH_REL_GPU_FINI_ACCESS;
> > +       else if (state != AMDGPU_END_INIT)
> > +               return -EINVAL;
> > +
> > +       WREG32(mmMAILBOX_MSGBUF_TRN_DW0, rel);
> > +       WREG32(mmMAILBOX_CONTROL,
> > +               REG_SET_FIELD(RREG32(mmMAILBOX_CONTROL),
> > +                       MAILBOX_CONTROL, TRN_MSG_VALID, 1));
> > +
> > +       return 0;
> > +}
> > +
> 
> These are gfx8 specific.  they should be handled in a gfx8 specific code.
> mxgpu_mb.c should only contain asic independent code.

I don't think so. The mailbox is only used by virtualization, and I'd like to put it into virtualization component.  

As I said in cover letter, the main aim of the series is put all virtualization code together and manage different virtualization chips and features. Do not want to mix it into amdgpu components. Otherwise the series will lost meaning.

> 
> Alex
> 
> 
> > +int xgpu_request_full_gpu_access(struct amdgpu_device *adev, bool
> > +init) {
> > +       int r;
> > +
> > +       if (init)
> > +               r = xgpu_request_reg_access(adev, AMDGPU_START_INIT);
> > +       else
> > +               r = xgpu_request_reg_access(adev, 
> > + AMDGPU_START_FINI);
> > +
> > +       return r;
> > +}
> > +
> > +int xgpu_release_full_gpu_access(struct amdgpu_device *adev, bool
> > +init) {
> > +       int r;
> > +
> > +       if (init)
> > +               r = xgpu_release_reg_access(adev, AMDGPU_END_INIT);
> > +       else
> > +               r = xgpu_release_reg_access(adev, AMDGPU_END_FINI);
> > +
> > +       return r;
> > +}
> > --
> > 2.7.4
> >
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx