> -----Original Message----- > From: Liu, Monk > Sent: Wednesday, January 11, 2017 10:33 PM > To: Yu, Xiangliang <Xiangliang.Yu at amd.com>; amd- > gfx at lists.freedesktop.org > Subject: RE: [V3 05/11] drm/amdgpu/virt: add high level interfaces for virt > > See in lines > > > -----Original Message----- > From: Yu, Xiangliang > Sent: Wednesday, January 11, 2017 10:27 PM > To: Liu, Monk <Monk.Liu at amd.com>; amd-gfx at lists.freedesktop.org > Subject: RE: [V3 05/11] drm/amdgpu/virt: add high level interfaces for virt > > > This patch is not derived from my work (amd-sriov-4.3/4.6), please > > don't add my "signed-off-by: Monk Liu " line on it, Seems you add an > > upper layer interface to invoke on gpu_request/release routines, And I > > will also take role in the reviewing of it: > > > > + adev->virt.caps &= ~AMDGPU_SRIOV_CAPS_RUNTIME; > > + return virt->ops->req_full_gpu(adev, init); > > > > [ml]One nitpick , you need place " adev->virt.caps &= > > ~AMDGPU_SRIOV_CAPS_RUNTIME;" in the second line, > > I think it make more clear as it is not runtime when entering full access mode. > > [ml] NO, you didn't get my point, you should &= ~ > AMDGPU_SRIOV_CAPS_RUNTIME *after* virt->ops->req_full_gpu(adev, > init) Good catch. > > > > > + r = virt->ops->rel_full_gpu(adev, init); > > + adev->virt.caps |= AMDGPU_SRIOV_CAPS_RUNTIME; > > > > [ml] what about rel_full_gpu() failed ? and if rel_full_gpu is > > designed not allowed to fail then you should type it as non-return > > function. ( I remember in my first initial implementation on rel_gpu > > function it is is non-return > > function) > > I think it is more flexible, we can handle error according to return result. > > [ML] then please handle the error accordingly, but your patch just ignore the > return value . see it yourself I have no time to think of the design, maybe later. > > > > > > > + adev->virt.caps &= ~AMDGPU_SRIOV_CAPS_RUNTIME; > > + return virt->ops->reset_gpu(adev); > > + } > > > > [ML] " adev->virt.caps &= ~AMDGPU_SRIOV_CAPS_RUNTIME" should be > put > > to the second line as well, like: > > > > r = virt-ops->reset_gpu(adev); > > If (!r) > > adev->virt.caps &= ~AMDGPU_SRIOV_CAPS_RUNTIME; else > > BUG(); /* we shouldn't fail in requesting GPU_RESET, check > > hypervisor please */ return r; > > I think we can't hang out VM so rudely, need to print out error message and > do error handling to enhance the process later. > > [ML] you still not get my point, if reset_gpu() failed. You shouldn't "&= > ~AMDGPU_SRIOV_CAPS_RUNTIME", but you did it in prior to reset_gpu(), > isn't it ?? > > > > > BR Monk > > > > -----Original Message----- > > From: Xiangliang Yu [mailto:Xiangliang.Yu at amd.com] > > Sent: Wednesday, January 11, 2017 9:18 PM > > To: amd-gfx at lists.freedesktop.org > > Cc: Yu, Xiangliang <Xiangliang.Yu at amd.com>; Liu, Monk > > <Monk.Liu at amd.com> > > Subject: [V3 05/11] drm/amdgpu/virt: add high level interfaces for > > virt > > > > Add high level interfaces that is not relate to specific asic. So asic > > files just need to implement the interfaces to support virtualization. > > > > Signed-off-by: Xiangliang Yu <Xiangliang.Yu at amd.com> > > Signed-off-by: Monk Liu <Monk.Liu at amd.com> > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 57 > > ++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h | 14 ++++++++ > > 2 files changed, 71 insertions(+) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c > > index 7861b6b..4e7df8e 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c > > @@ -80,3 +80,60 @@ void amdgpu_virt_kiq_wreg(struct amdgpu_device > > *adev, uint32_t reg, uint32_t v) > > DRM_ERROR("wait for kiq fence error: %ld.\n", r); > > fence_put(f); > > } > > + > > +/** > > + * amdgpu_virt_request_full_gpu() - request full gpu access > > + * @amdgpu: amdgpu device. > > + * @init: is driver init time. > > + * When start to init/fini driver, first need to request full gpu access. > > + * Return: Zero if request success, otherwise will return error. > > + */ > > +int amdgpu_virt_request_full_gpu(struct amdgpu_device *adev, bool > > +init) { > > + struct amdgpu_virt *virt = &adev->virt; > > + > > + if (virt->ops && virt->ops->req_full_gpu) { > > + adev->virt.caps &= ~AMDGPU_SRIOV_CAPS_RUNTIME; > > + return virt->ops->req_full_gpu(adev, init); > > + } > > + > > + return 0; > > +} > > + > > +/** > > + * amdgpu_virt_release_full_gpu() - release full gpu access > > + * @amdgpu: amdgpu device. > > + * @init: is driver init time. > > + * When finishing driver init/fini, need to release full gpu access. > > + * Return: Zero if release success, otherwise will returen error. > > + */ > > +int amdgpu_virt_release_full_gpu(struct amdgpu_device *adev, bool > > +init) { > > + struct amdgpu_virt *virt = &adev->virt; > > + int r; > > + > > + if (virt->ops && virt->ops->rel_full_gpu) { > > + r = virt->ops->rel_full_gpu(adev, init); > > + adev->virt.caps |= AMDGPU_SRIOV_CAPS_RUNTIME; > > + return r; > > + } > > + return 0; > > +} > > + > > +/** > > + * amdgpu_virt_reset_gpu() - reset gpu > > + * @amdgpu: amdgpu device. > > + * Send reset command to GPU hypervisor to reset GPU that VM is using > > + * Return: Zero if reset success, otherwise will return error. > > + */ > > +int amdgpu_virt_reset_gpu(struct amdgpu_device *adev) { > > + struct amdgpu_virt *virt = &adev->virt; > > + > > + if (virt->ops && virt->ops->reset_gpu) { > > + adev->virt.caps &= ~AMDGPU_SRIOV_CAPS_RUNTIME; > > + return virt->ops->reset_gpu(adev); > > + } > > + > > + return 0; > > +} > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h > > index 2869980..45771b0 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h > > @@ -29,11 +29,22 @@ > > #define AMDGPU_SRIOV_CAPS_IS_VF (1 << 2) /* this GPU is a virtual > > function */ > > #define AMDGPU_PASSTHROUGH_MODE (1 << 3) /* thw whole GPU > is > > pass through for VM */ > > #define AMDGPU_SRIOV_CAPS_RUNTIME (1 << 4) /* is out of full > access > > mode */ > > + > > +/** > > + * struct amdgpu_virt_ops - amdgpu device virt operations */ struct > > +amdgpu_virt_ops { > > + int (*req_full_gpu)(struct amdgpu_device *adev, bool init); > > + int (*rel_full_gpu)(struct amdgpu_device *adev, bool init); > > + int (*reset_gpu)(struct amdgpu_device *adev); }; > > + > > /* GPU virtualization */ > > struct amdgpu_virt { > > uint32_t caps; > > uint32_t reg_val_offs; > > struct mutex lock; > > + const struct amdgpu_virt_ops *ops; > > }; > > > > #define amdgpu_sriov_enabled(adev) \ > > @@ -63,5 +74,8 @@ static inline bool is_virtual_machine(void) void > > amdgpu_virt_init_setting(struct amdgpu_device *adev); uint32_t > > amdgpu_virt_kiq_rreg(struct amdgpu_device *adev, uint32_t reg); void > > amdgpu_virt_kiq_wreg(struct amdgpu_device *adev, uint32_t reg, > > uint32_t v); > > +int amdgpu_virt_request_full_gpu(struct amdgpu_device *adev, bool > > +init); int amdgpu_virt_release_full_gpu(struct amdgpu_device *adev, > > +bool init); int amdgpu_virt_reset_gpu(struct amdgpu_device *adev); > > > > #endif > > -- > > 2.7.4
