[V3 05/11] drm/amdgpu/virt: add high level interfaces for virt

[Yu, Xiangliang]

> -----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