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

[Yu, Xiangliang]

> -----Original Message-----
> From: Christian König [mailto:deathsimple at vodafone.de]
> Sent: Tuesday, January 10, 2017 9:59 PM
> To: Yu, Xiangliang <Xiangliang.Yu at amd.com>; amd-
> gfx at lists.freedesktop.org
> Subject: Re: [V2 05/11] drm/amdgpu/virt: add high level interfaces for virt
> 
> Am 10.01.2017 um 14:33 schrieb Yu, Xiangliang:
> >> -----Original Message-----
> >> From: Christian König [mailto:deathsimple at vodafone.de]
> >> Sent: Tuesday, January 10, 2017 9:12 PM
> >> To: Yu, Xiangliang <Xiangliang.Yu at amd.com>; amd-
> >> gfx at lists.freedesktop.org
> >> Subject: Re: [V2 05/11] drm/amdgpu/virt: add high level interfaces
> >> for virt
> >>
> >> Am 10.01.2017 um 11:00 schrieb Xiangliang Yu:
> >>> 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>
> >>> ---
> >>>    drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 57
> >> ++++++++++++++++++++++++++++++++
> >>>    drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h | 15 +++++++++
> >>>    2 files changed, 72 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> >>> index 6520a4e..f32a789 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> >>> @@ -84,3 +84,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);
> >> I would be conservative here and request full GPU access first and
> >> then clear AMDGPU_SRIOV_CAPS_RUNTIME.
> >>
> >> Just in case the function is called concurrently with another thread
> >> checking the caps.
> > It can't be used in parallel, the problem you said shouldn't be appear.
> >
> >> On the other hand req_full_gpu() could need the flag to handle
> >> register reads/writes correctly, but in this case I would question if
> >> we shouldn't add special macros for this.
> > We must clear RUNTIME flag so that can read/write registers with mmio,
> > Otherwise driver will hang.
> 
> Yeah, that's what I expected. Would be nice if we could better split that, e.g.
> have explicit macros for direct register write.
> 
> But that's really only nice to have, patch is ok as it is as far as I can see.

I think it is also very clear. Actually, I have a try but it is not good choice.
If anyone have good idea, can change it later.

> >
> >>
> >>
> >>> +	}
> >>> +
> >>> +	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 24f0590..3f8fc0f 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
> >>> @@ -29,11 +29,23 @@
> >>>    #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		val_offs;
> >>>    	struct mutex		lock;
> >>> +
> >>> +	const struct amdgpu_virt_ops	*ops;
> >>>    };
> >>>
> >>>    #define amdgpu_sriov_enabled(adev) \ @@ -63,5 +75,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
>