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 at gmail.com] > Sent: Tuesday, December 20, 2016 7:25 AM > To: Yu, Xiangliang <Xiangliang.Yu at amd.com> > Cc: amd-gfx list <amd-gfx at lists.freedesktop.org>; > dl.SRDC_SW_GPUVirtualization <dl.SRDC_SW_GPUVirtualization at amd.com>; > Liu, Shaoyun <Shaoyun.Liu at 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 at 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 at amd.com> > > Signed-off-by: shaoyunl <Shaoyun.Liu at 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 at 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 at lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
