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

[deathsimple@xxxxxxxxxxx (Christian König)]

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.

Christian.

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