Re: [PATCH] drm/amd/amdgpu: workaround for UVD busy status

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

Am 14.08.19 um 13:11 schrieb Kenneth Feng:
> On Vega20, tools depends on UVD_STATUS.VCPU_REPORT bit0
> to decide if UVD instances are in busy state or idle state.
> This workaround fixes the issue that tools always fetch the
> UVD instances state as busy state no matter if there is a UVD work.

Well which tools are using that?

Over all looks like the wrong approach to me since you modify the common 
code to call back into the IP specific code again.

So without further explanation this is a NAK.

Christian.

> Signed-off-by: Kenneth Feng <kenneth.feng@xxxxxxx>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  3 +++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c    | 30 ++++++++++++++++++++++++++++--
>   drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c      | 19 +++++++++++++++++++
>   drivers/gpu/drm/amd/include/amd_shared.h   |  1 +
>   5 files changed, 74 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 4d096ff..6e5a41b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -259,6 +259,9 @@ int amdgpu_device_ip_set_clockgating_state(void *dev,
>   int amdgpu_device_ip_set_powergating_state(void *dev,
>   					   enum amd_ip_block_type block_type,
>   					   enum amd_powergating_state state);
> +int amdgpu_device_ip_set_instance_state(void *dev,
> +					   enum amd_ip_block_type block_type,
> +					   bool busy_state, int inst);
>   void amdgpu_device_ip_get_clockgating_state(struct amdgpu_device *adev,
>   					    u32 *flags);
>   int amdgpu_device_ip_wait_for_idle(struct amdgpu_device *adev,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 93ed3cb..e65e251 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1127,6 +1127,29 @@ int amdgpu_device_ip_set_powergating_state(void *dev,
>   	return r;
>   }
>   
> +int amdgpu_device_ip_set_instance_state(void *dev,
> +					   enum amd_ip_block_type block_type,
> +					   bool busy_state, int inst)
> +{
> +	struct amdgpu_device *adev = dev;
> +	int i, r = 0;
> +
> +	for (i = 0; i < adev->num_ip_blocks; i++) {
> +		if (!adev->ip_blocks[i].status.valid)
> +			continue;
> +		if (adev->ip_blocks[i].version->type != block_type)
> +			continue;
> +		if (!adev->ip_blocks[i].version->funcs->set_instance_state)
> +			continue;
> +		r = adev->ip_blocks[i].version->funcs->set_instance_state(
> +			(void *)adev, busy_state, inst);
> +		if (r)
> +			DRM_ERROR("set_instance_state of IP block <%s> failed %d\n",
> +				  adev->ip_blocks[i].version->funcs->name, r);
> +	}
> +	return r;
> +}
> +
>   /**
>    * amdgpu_device_ip_get_clockgating_state - get the CG state
>    *
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> index 4e5d13e4..d3496ab 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> @@ -1180,13 +1180,22 @@ static void amdgpu_uvd_idle_work_handler(struct work_struct *work)
>   	struct amdgpu_device *adev =
>   		container_of(work, struct amdgpu_device, uvd.idle_work.work);
>   	unsigned fences = 0, i, j;
> +	unsigned *fences_inst = (unsigned *)kzalloc(adev->uvd.num_uvd_inst*sizeof(unsigned), GFP_KERNEL);
>   
>   	for (i = 0; i < adev->uvd.num_uvd_inst; ++i) {
>   		if (adev->uvd.harvest_config & (1 << i))
>   			continue;
> -		fences += amdgpu_fence_count_emitted(&adev->uvd.inst[i].ring);
> +		fences_inst[i] = amdgpu_fence_count_emitted(&adev->uvd.inst[i].ring);
> +		fences += fences_inst[i];
>   		for (j = 0; j < adev->uvd.num_enc_rings; ++j) {
> -			fences += amdgpu_fence_count_emitted(&adev->uvd.inst[i].ring_enc[j]);
> +			fences_inst[i] += amdgpu_fence_count_emitted(&adev->uvd.inst[i].ring_enc[j]);
> +			fences += fences_inst[i];
> +		}
> +		/* workaround for the tools to show UVD busy status */
> +		if (fences_inst[i] == 0) {
> +			amdgpu_device_ip_set_instance_state(adev, AMD_IP_BLOCK_TYPE_UVD,
> +							       false, i);
> +			printk("Kenneth - set instance idle!\n");
>   		}
>   	}
>   
> @@ -1210,10 +1219,27 @@ void amdgpu_uvd_ring_begin_use(struct amdgpu_ring *ring)
>   {
>   	struct amdgpu_device *adev = ring->adev;
>   	bool set_clocks;
> +	unsigned i,j;
> +	unsigned *fences_inst = (unsigned *)kzalloc(adev->uvd.num_uvd_inst*sizeof(unsigned), GFP_KERNEL);
>   
>   	if (amdgpu_sriov_vf(adev))
>   		return;
>   
> +	for (i = 0; i < adev->uvd.num_uvd_inst; ++i) {
> +		printk("Kenneth - run after set_clock!\n");
> +		if (adev->uvd.harvest_config & (1 << i))
> +			continue;
> +		fences_inst[i] = amdgpu_fence_count_emitted(&adev->uvd.inst[i].ring);
> +		for (j = 0; j < adev->uvd.num_enc_rings; ++j)
> +			fences_inst[i] += amdgpu_fence_count_emitted(&adev->uvd.inst[i].ring_enc[j]);
> +		/* workaround for the tools to show UVD busy */
> +		if (fences_inst[i] != 0) {
> +			amdgpu_device_ip_set_instance_state(adev, AMD_IP_BLOCK_TYPE_UVD,
> +						       true, i);
> +			printk("Kenneth - run in setting instance state!\n");
> +		}
> +	}
> +
>   	set_clocks = !cancel_delayed_work_sync(&adev->uvd.idle_work);
>   	if (set_clocks) {
>   		if (adev->pm.dpm_enabled) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c b/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
> index 2f3d4e8..bf0f33a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
> @@ -1738,6 +1738,24 @@ static int uvd_v7_0_set_clockgating_state(void *handle,
>   	return 0;
>   }
>   
> +/* workaround for tools to fetch the UVD busy status */
> +static int uvd_v7_0_set_instance_state(void *handle, bool busy_state, int inst)
> +{
> +	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> +
> +	if(inst >= adev->uvd.num_uvd_inst)
> +		return -EINVAL;
> +
> +	if(busy_state)
> +		WREG32_P(SOC15_REG_OFFSET(UVD, inst, mmUVD_STATUS), (1 << UVD_STATUS__VCPU_REPORT__SHIFT),
> +				~(1 << UVD_STATUS__VCPU_REPORT__SHIFT));
> +	else
> +		WREG32_P(SOC15_REG_OFFSET(UVD, inst, mmUVD_STATUS), 0,
> +				~(1 << UVD_STATUS__VCPU_REPORT__SHIFT));
> +
> +	return 0;
> +}
> +
>   const struct amd_ip_funcs uvd_v7_0_ip_funcs = {
>   	.name = "uvd_v7_0",
>   	.early_init = uvd_v7_0_early_init,
> @@ -1756,6 +1774,7 @@ const struct amd_ip_funcs uvd_v7_0_ip_funcs = {
>   	.post_soft_reset = NULL /* uvd_v7_0_post_soft_reset */,
>   	.set_clockgating_state = uvd_v7_0_set_clockgating_state,
>   	.set_powergating_state = NULL /* uvd_v7_0_set_powergating_state */,
> +	.set_instance_state = uvd_v7_0_set_instance_state,
>   };
>   
>   static const struct amdgpu_ring_funcs uvd_v7_0_ring_vm_funcs = {
> diff --git a/drivers/gpu/drm/amd/include/amd_shared.h b/drivers/gpu/drm/amd/include/amd_shared.h
> index 779c9e7..8cc54c9 100644
> --- a/drivers/gpu/drm/amd/include/amd_shared.h
> +++ b/drivers/gpu/drm/amd/include/amd_shared.h
> @@ -198,6 +198,7 @@ struct amd_ip_funcs {
>   	void (*get_clockgating_state)(void *handle, u32 *flags);
>   	/** @enable_umd_pstate: enable UMD powerstate */
>   	int (*enable_umd_pstate)(void *handle, enum amd_dpm_forced_level *level);
> +	int (*set_instance_state)(void *handle, bool busy_state, int inst);
>   };
>   
>   

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx