Re: [PATCH v3] drm/amdgpu: add workarounds for VCN TMZ issue on CHIP_RAVEN

[Paul Menzel <pmenzel@molgen.mpg.de>]

Dear Lang,


Am 14.03.22 um 03:45 schrieb Lang Yu:

Thank you for your patch. A shorter commit message summary would be:

 > drm/amdgpu: Work around VNC TMZ issue on CHIP_RAVEN

> It is a hardware issue that VCN can't handle a GTT
> backing stored TMZ buffer on CHIP_RAVEN series ASIC.

Where is that documented, and how can this be reproduced?

> Move such a TMZ buffer to VRAM domain before command
> submission as a wrokaround.

workaround

Please use a text width of 75 characters per line.

> v2:
>   - Use patch_cs_in_place callback.
> 
> v3:
>   - Bail out early if unsecure IBs.
> 
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Lang Yu <Lang.Yu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 71 +++++++++++++++++++++++++++
>   1 file changed, 71 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> index 7bbb9ba6b80b..1ac9e06d3a4d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> @@ -24,6 +24,7 @@
>   #include <linux/firmware.h>
>   
>   #include "amdgpu.h"
> +#include "amdgpu_cs.h"
>   #include "amdgpu_vcn.h"
>   #include "amdgpu_pm.h"
>   #include "soc15.h"
> @@ -1905,6 +1906,75 @@ static const struct amd_ip_funcs vcn_v1_0_ip_funcs = {
>   	.set_powergating_state = vcn_v1_0_set_powergating_state,
>   };
>   
> +/*
> + * It is a hardware issue that VCN can't handle a GTT TMZ buffer on
> + * CHIP_RAVEN series ASIC. Move such a GTT TMZ buffer to VRAM domain
> + * before command submission as a workaround.
> + */
> +static int vcn_v1_0_validate_bo(struct amdgpu_cs_parser *parser,
> +				struct amdgpu_job *job,
> +				uint64_t addr)
> +{
> +	struct ttm_operation_ctx ctx = { false, false };
> +	struct amdgpu_fpriv *fpriv = parser->filp->driver_priv;
> +	struct amdgpu_vm *vm = &fpriv->vm;
> +	struct amdgpu_bo_va_mapping *mapping;
> +	struct amdgpu_bo *bo;
> +	int r;
> +
> +	addr &= AMDGPU_GMC_HOLE_MASK;
> +	if (addr & 0x7) {
> +		DRM_ERROR("VCN messages must be 8 byte aligned!\n");
> +		return -EINVAL;
> +	}
> +
> +	mapping = amdgpu_vm_bo_lookup_mapping(vm, addr/AMDGPU_GPU_PAGE_SIZE);
> +	if (!mapping || !mapping->bo_va || !mapping->bo_va->base.bo)
> +		return -EINVAL;
> +
> +	bo = mapping->bo_va->base.bo;
> +	if (!(bo->flags & AMDGPU_GEM_CREATE_ENCRYPTED))
> +		return 0;
> +
> +	amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_VRAM);
> +	r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
> +	if (r) {
> +		DRM_ERROR("Failed validating the VCN message BO (%d)!\n", r);

to validate

> +		return r;
> +	}
> +
> +	return r;
> +}
> +
> +static int vcn_v1_0_ring_patch_cs_in_place(struct amdgpu_cs_parser *p,
> +					   struct amdgpu_job *job,
> +					   struct amdgpu_ib *ib)
> +{
> +	uint32_t msg_lo = 0, msg_hi = 0;
> +	int i, r;

Use unsigned int for the counter variable?

> +
> +	if (!(ib->flags & AMDGPU_IB_FLAGS_SECURE))
> +		return 0;
> +
> +	for (i = 0; i < ib->length_dw; i += 2) {
> +		uint32_t reg = amdgpu_ib_get_value(ib, i);
> +		uint32_t val = amdgpu_ib_get_value(ib, i + 1);
> +
> +		if (reg == PACKET0(p->adev->vcn.internal.data0, 0)) {
> +			msg_lo = val;
> +		} else if (reg == PACKET0(p->adev->vcn.internal.data1, 0)) {
> +			msg_hi = val;
> +		} else if (reg == PACKET0(p->adev->vcn.internal.cmd, 0)) {
> +			r = vcn_v1_0_validate_bo(p, job,
> +						 ((u64)msg_hi) << 32 | msg_lo);
> +			if (r)
> +				return r;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>   static const struct amdgpu_ring_funcs vcn_v1_0_dec_ring_vm_funcs = {
>   	.type = AMDGPU_RING_TYPE_VCN_DEC,
>   	.align_mask = 0xf,
> @@ -1914,6 +1984,7 @@ static const struct amdgpu_ring_funcs vcn_v1_0_dec_ring_vm_funcs = {
>   	.get_rptr = vcn_v1_0_dec_ring_get_rptr,
>   	.get_wptr = vcn_v1_0_dec_ring_get_wptr,
>   	.set_wptr = vcn_v1_0_dec_ring_set_wptr,
> +	.patch_cs_in_place = vcn_v1_0_ring_patch_cs_in_place,
>   	.emit_frame_size =
>   		6 + 6 + /* hdp invalidate / flush */
>   		SOC15_FLUSH_GPU_TLB_NUM_WREG * 6 +


Kind regards,

Paul