[PATCH v2] drm/amdgpu: add workarounds for Raven VCN TMZ issue

[PATCH v2] drm/amdgpu: add workarounds for Raven VCN TMZ issue

[Lang Yu]

It is a hardware issue that VCN can't handle a GTT
backing stored TMZ buffer on Raven.

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

v2:
 - Use patch_cs_in_place callback.

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 | 68 +++++++++++++++++++++++++++
 1 file changed, 68 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..810932abd3af 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,72 @@ 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 Raven VCN can't handle a GTT TMZ buffer.
+ * Move such a GTT TMZ buffer to VRAM domain before command submission.
+ */
+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);
+		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;
+
+	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 +1981,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 +
-- 
2.25.1

Re: [PATCH v2] drm/amdgpu: add workarounds for Raven VCN TMZ issue

[Christian König]

Am 08.03.22 um 04:39 schrieb Lang Yu:
> It is a hardware issue that VCN can't handle a GTT
> backing stored TMZ buffer on Raven.
>
> Move such a TMZ buffer to VRAM domain before command
> submission.
>
> v2:
>   - Use patch_cs_in_place callback.
>
> 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 | 68 +++++++++++++++++++++++++++
>   1 file changed, 68 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..810932abd3af 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,72 @@ 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 Raven VCN can't handle a GTT TMZ buffer.
> + * Move such a GTT TMZ buffer to VRAM domain before command submission.
> + */
> +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);
> +		return r;
> +	}

Well, exactly that won't work.

The message structure isn't TMZ protected because otherwise the driver 
won't be able to stitch it together.

What is TMZ protected are the surfaces the message structure is pointing 
to. So what you would need to do is to completely parse the structure 
and then move on the relevant buffers into VRAM.

Leo or James, can you help with that?

Regards,
Christian.

> +
> +	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;
> +
> +	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 +1981,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 +

Re: [PATCH v2] drm/amdgpu: add workarounds for Raven VCN TMZ issue

[Lang Yu]

On 03/08/ , Christian König wrote:
> 
> 
> Am 08.03.22 um 04:39 schrieb Lang Yu:
> > It is a hardware issue that VCN can't handle a GTT
> > backing stored TMZ buffer on Raven.
> > 
> > Move such a TMZ buffer to VRAM domain before command
> > submission.
> > 
> > v2:
> >   - Use patch_cs_in_place callback.
> > 
> > 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 | 68 +++++++++++++++++++++++++++
> >   1 file changed, 68 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..810932abd3af 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,72 @@ 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 Raven VCN can't handle a GTT TMZ buffer.
> > + * Move such a GTT TMZ buffer to VRAM domain before command submission.
> > + */
> > +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);
> > +		return r;
> > +	}
> 
> Well, exactly that won't work.
> 
> The message structure isn't TMZ protected because otherwise the driver won't
> be able to stitch it together.
> 
> What is TMZ protected are the surfaces the message structure is pointing to.
> So what you would need to do is to completely parse the structure and then
> move on the relevant buffers into VRAM.
> 
> Leo or James, can you help with that?

From my observations, when decoding secure contents, register
GPCOM_VCPU_DATA0 and GPCOM_VCPU_DATA1 are set to a TMZ buffer address.
And this way works when allocating TMZ buffers in GTT domain.

Regards,
Lang

> Regards,
> Christian.
> 
> > +
> > +	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;
> > +
> > +	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 +1981,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 +
> 

Re: [PATCH v2] drm/amdgpu: add workarounds for Raven VCN TMZ issue

[Christian König]

Am 08.03.22 um 08:33 schrieb Lang Yu:
> On 03/08/ , Christian König wrote:
>>
>> Am 08.03.22 um 04:39 schrieb Lang Yu:
>>> It is a hardware issue that VCN can't handle a GTT
>>> backing stored TMZ buffer on Raven.
>>>
>>> Move such a TMZ buffer to VRAM domain before command
>>> submission.
>>>
>>> v2:
>>>    - Use patch_cs_in_place callback.
>>>
>>> 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 | 68 +++++++++++++++++++++++++++
>>>    1 file changed, 68 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..810932abd3af 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,72 @@ 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 Raven VCN can't handle a GTT TMZ buffer.
>>> + * Move such a GTT TMZ buffer to VRAM domain before command submission.
>>> + */
>>> +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);
>>> +		return r;
>>> +	}
>> Well, exactly that won't work.
>>
>> The message structure isn't TMZ protected because otherwise the driver won't
>> be able to stitch it together.
>>
>> What is TMZ protected are the surfaces the message structure is pointing to.
>> So what you would need to do is to completely parse the structure and then
>> move on the relevant buffers into VRAM.
>>
>> Leo or James, can you help with that?
>  From my observations, when decoding secure contents, register
> GPCOM_VCPU_DATA0 and GPCOM_VCPU_DATA1 are set to a TMZ buffer address.
> And this way works when allocating TMZ buffers in GTT domain.

As far as I remember that's only the case for the decoding, encoding 
works by putting the addresses into the message buffer.

But could be that decoding is sufficient, Leo and James need to comment 
on this.

Regards,
Christian.

>
> Regards,
> Lang
>
>> Regards,
>> Christian.
>>
>>> +
>>> +	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;
>>> +
>>> +	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 +1981,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 +

Re: [PATCH v2] drm/amdgpu: add workarounds for Raven VCN TMZ issue

[Lang Yu]

On 03/08/ , Christian König wrote:
> Am 08.03.22 um 08:33 schrieb Lang Yu:
> > On 03/08/ , Christian König wrote:
> > > 
> > > Am 08.03.22 um 04:39 schrieb Lang Yu:
> > > > It is a hardware issue that VCN can't handle a GTT
> > > > backing stored TMZ buffer on Raven.
> > > > 
> > > > Move such a TMZ buffer to VRAM domain before command
> > > > submission.
> > > > 
> > > > v2:
> > > >    - Use patch_cs_in_place callback.
> > > > 
> > > > 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 | 68 +++++++++++++++++++++++++++
> > > >    1 file changed, 68 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..810932abd3af 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,72 @@ 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 Raven VCN can't handle a GTT TMZ buffer.
> > > > + * Move such a GTT TMZ buffer to VRAM domain before command submission.
> > > > + */
> > > > +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);
> > > > +		return r;
> > > > +	}
> > > Well, exactly that won't work.
> > > 
> > > The message structure isn't TMZ protected because otherwise the driver won't
> > > be able to stitch it together.
> > > 
> > > What is TMZ protected are the surfaces the message structure is pointing to.
> > > So what you would need to do is to completely parse the structure and then
> > > move on the relevant buffers into VRAM.
> > > 
> > > Leo or James, can you help with that?
> >  From my observations, when decoding secure contents, register
> > GPCOM_VCPU_DATA0 and GPCOM_VCPU_DATA1 are set to a TMZ buffer address.
> > And this way works when allocating TMZ buffers in GTT domain.
> 
> As far as I remember that's only the case for the decoding, encoding works
> by putting the addresses into the message buffer.
> 
> But could be that decoding is sufficient, Leo and James need to comment on
> this.

It seems that only decode needs TMZ buffers. Only observe si_vid_create_tmz_buffer() 
was called in rvcn_dec_message_decode() in src/gallium/drivers/radeon/radeon_vcn_dec.c.

Regards,
Lang

> Regards,
> Christian.
> 
> > 
> > Regards,
> > Lang
> > 
> > > Regards,
> > > Christian.
> > > 
> > > > +
> > > > +	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;
> > > > +
> > > > +	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 +1981,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 +
> 

Re: [PATCH v2] drm/amdgpu: add workarounds for Raven VCN TMZ issue

[Christian König]

Am 08.03.22 um 09:06 schrieb Lang Yu:
> On 03/08/ , Christian König wrote:
>> Am 08.03.22 um 08:33 schrieb Lang Yu:
>>> On 03/08/ , Christian König wrote:
>>>> Am 08.03.22 um 04:39 schrieb Lang Yu:
>>>>> It is a hardware issue that VCN can't handle a GTT
>>>>> backing stored TMZ buffer on Raven.
>>>>>
>>>>> Move such a TMZ buffer to VRAM domain before command
>>>>> submission.
>>>>>
>>>>> v2:
>>>>>     - Use patch_cs_in_place callback.
>>>>>
>>>>> 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 | 68 +++++++++++++++++++++++++++
>>>>>     1 file changed, 68 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..810932abd3af 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,72 @@ 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 Raven VCN can't handle a GTT TMZ buffer.
>>>>> + * Move such a GTT TMZ buffer to VRAM domain before command submission.
>>>>> + */
>>>>> +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);
>>>>> +		return r;
>>>>> +	}
>>>> Well, exactly that won't work.
>>>>
>>>> The message structure isn't TMZ protected because otherwise the driver won't
>>>> be able to stitch it together.
>>>>
>>>> What is TMZ protected are the surfaces the message structure is pointing to.
>>>> So what you would need to do is to completely parse the structure and then
>>>> move on the relevant buffers into VRAM.
>>>>
>>>> Leo or James, can you help with that?
>>>   From my observations, when decoding secure contents, register
>>> GPCOM_VCPU_DATA0 and GPCOM_VCPU_DATA1 are set to a TMZ buffer address.
>>> And this way works when allocating TMZ buffers in GTT domain.
>> As far as I remember that's only the case for the decoding, encoding works
>> by putting the addresses into the message buffer.
>>
>> But could be that decoding is sufficient, Leo and James need to comment on
>> this.
> It seems that only decode needs TMZ buffers. Only observe si_vid_create_tmz_buffer()
> was called in rvcn_dec_message_decode() in src/gallium/drivers/radeon/radeon_vcn_dec.c.

Mhm, good point. Let's wait for Leo and James to wake up, when we don't 
need encode support than that would makes things much easier.

Regards,
Christian.

>
> Regards,
> Lang
>
>> Regards,
>> Christian.
>>
>>> Regards,
>>> Lang
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> +
>>>>> +	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;
>>>>> +
>>>>> +	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 +1981,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 +

Re: [PATCH v2] drm/amdgpu: add workarounds for Raven VCN TMZ issue

[Alex Deucher]

On Tue, Mar 8, 2022 at 4:16 AM Christian König <christian.koenig@amd.com> wrote:
>
> Am 08.03.22 um 09:06 schrieb Lang Yu:
> > On 03/08/ , Christian König wrote:
> >> Am 08.03.22 um 08:33 schrieb Lang Yu:
> >>> On 03/08/ , Christian König wrote:
> >>>> Am 08.03.22 um 04:39 schrieb Lang Yu:
> >>>>> It is a hardware issue that VCN can't handle a GTT
> >>>>> backing stored TMZ buffer on Raven.
> >>>>>
> >>>>> Move such a TMZ buffer to VRAM domain before command
> >>>>> submission.
> >>>>>
> >>>>> v2:
> >>>>>     - Use patch_cs_in_place callback.
> >>>>>
> >>>>> 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 | 68 +++++++++++++++++++++++++++
> >>>>>     1 file changed, 68 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..810932abd3af 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,72 @@ 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 Raven VCN can't handle a GTT TMZ buffer.
> >>>>> + * Move such a GTT TMZ buffer to VRAM domain before command submission.
> >>>>> + */
> >>>>> +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);
> >>>>> +         return r;
> >>>>> + }
> >>>> Well, exactly that won't work.
> >>>>
> >>>> The message structure isn't TMZ protected because otherwise the driver won't
> >>>> be able to stitch it together.
> >>>>
> >>>> What is TMZ protected are the surfaces the message structure is pointing to.
> >>>> So what you would need to do is to completely parse the structure and then
> >>>> move on the relevant buffers into VRAM.
> >>>>
> >>>> Leo or James, can you help with that?
> >>>   From my observations, when decoding secure contents, register
> >>> GPCOM_VCPU_DATA0 and GPCOM_VCPU_DATA1 are set to a TMZ buffer address.
> >>> And this way works when allocating TMZ buffers in GTT domain.
> >> As far as I remember that's only the case for the decoding, encoding works
> >> by putting the addresses into the message buffer.
> >>
> >> But could be that decoding is sufficient, Leo and James need to comment on
> >> this.
> > It seems that only decode needs TMZ buffers. Only observe si_vid_create_tmz_buffer()
> > was called in rvcn_dec_message_decode() in src/gallium/drivers/radeon/radeon_vcn_dec.c.
>
> Mhm, good point. Let's wait for Leo and James to wake up, when we don't
> need encode support than that would makes things much easier.

If we don't support encrypted encode, we should add a check to prevent
it like we do for compute.

Alex

>
> Regards,
> Christian.
>
> >
> > Regards,
> > Lang
> >
> >> Regards,
> >> Christian.
> >>
> >>> Regards,
> >>> Lang
> >>>
> >>>> Regards,
> >>>> Christian.
> >>>>
> >>>>> +
> >>>>> + 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;
> >>>>> +
> >>>>> + 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 +1981,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 +
>

Re: [PATCH v2] drm/amdgpu: add workarounds for Raven VCN TMZ issue

[Leo Liu]

On 2022-03-08 04:16, Christian König wrote:
> Am 08.03.22 um 09:06 schrieb Lang Yu:
>> On 03/08/ , Christian König wrote:
>>> Am 08.03.22 um 08:33 schrieb Lang Yu:
>>>> On 03/08/ , Christian König wrote:
>>>>> Am 08.03.22 um 04:39 schrieb Lang Yu:
>>>>>> It is a hardware issue that VCN can't handle a GTT
>>>>>> backing stored TMZ buffer on Raven.
>>>>>>
>>>>>> Move such a TMZ buffer to VRAM domain before command
>>>>>> submission.
>>>>>>
>>>>>> v2:
>>>>>>     - Use patch_cs_in_place callback.
>>>>>>
>>>>>> 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 | 68 
>>>>>> +++++++++++++++++++++++++++
>>>>>>     1 file changed, 68 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..810932abd3af 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,72 @@ 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 Raven VCN can't handle a GTT TMZ 
>>>>>> buffer.
>>>>>> + * Move such a GTT TMZ buffer to VRAM domain before command 
>>>>>> submission.
>>>>>> + */
>>>>>> +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);
>>>>>> +        return r;
>>>>>> +    }
>>>>> Well, exactly that won't work.
>>>>>
>>>>> The message structure isn't TMZ protected because otherwise the 
>>>>> driver won't
>>>>> be able to stitch it together.
>>>>>
>>>>> What is TMZ protected are the surfaces the message structure is 
>>>>> pointing to.
>>>>> So what you would need to do is to completely parse the structure 
>>>>> and then
>>>>> move on the relevant buffers into VRAM.
>>>>>
>>>>> Leo or James, can you help with that?
>>>>   From my observations, when decoding secure contents, register
>>>> GPCOM_VCPU_DATA0 and GPCOM_VCPU_DATA1 are set to a TMZ buffer address.
>>>> And this way works when allocating TMZ buffers in GTT domain.
>>> As far as I remember that's only the case for the decoding, encoding 
>>> works
>>> by putting the addresses into the message buffer.
>>>
>>> But could be that decoding is sufficient, Leo and James need to 
>>> comment on
>>> this.
>> It seems that only decode needs TMZ buffers. Only observe 
>> si_vid_create_tmz_buffer()
>> was called in rvcn_dec_message_decode() in 
>> src/gallium/drivers/radeon/radeon_vcn_dec.c.
>
> Mhm, good point. Let's wait for Leo and James to wake up, when we 
> don't need encode support than that would makes things much easier.

For secure playback, the buffer required in TMZ are dpb, dt and ctx, for 
the rest esp. those for CPU access don't need that E.g. msg buffer, and 
bitstream buffer.

 From radeon_vcn_dec.c, you can see the buffer for dpb and ctx, and dt 
buffer frontend/va/surface is set to PIPE_BIND_PROTECTED.


Regards,

Leo



>
> Regards,
> Christian.
>
>>
>> Regards,
>> Lang
>>
>>> Regards,
>>> Christian.
>>>
>>>> Regards,
>>>> Lang
>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>> +
>>>>>> +    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;
>>>>>> +
>>>>>> +    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 +1981,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 +
>

Re: [PATCH v2] drm/amdgpu: add workarounds for Raven VCN TMZ issue

[Leo Liu]

On 2022-03-08 11:18, Leo Liu wrote:
>
> On 2022-03-08 04:16, Christian König wrote:
>> Am 08.03.22 um 09:06 schrieb Lang Yu:
>>> On 03/08/ , Christian König wrote:
>>>> Am 08.03.22 um 08:33 schrieb Lang Yu:
>>>>> On 03/08/ , Christian König wrote:
>>>>>> Am 08.03.22 um 04:39 schrieb Lang Yu:
>>>>>>> It is a hardware issue that VCN can't handle a GTT
>>>>>>> backing stored TMZ buffer on Raven.
>>>>>>>
>>>>>>> Move such a TMZ buffer to VRAM domain before command
>>>>>>> submission.
>>>>>>>
>>>>>>> v2:
>>>>>>>     - Use patch_cs_in_place callback.
>>>>>>>
>>>>>>> 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 | 68 
>>>>>>> +++++++++++++++++++++++++++
>>>>>>>     1 file changed, 68 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..810932abd3af 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,72 @@ 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 Raven VCN can't handle a GTT TMZ 
>>>>>>> buffer.
>>>>>>> + * Move such a GTT TMZ buffer to VRAM domain before command 
>>>>>>> submission.
>>>>>>> + */
>>>>>>> +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);
>>>>>>> +        return r;
>>>>>>> +    }
>>>>>> Well, exactly that won't work.
>>>>>>
>>>>>> The message structure isn't TMZ protected because otherwise the 
>>>>>> driver won't
>>>>>> be able to stitch it together.
>>>>>>
>>>>>> What is TMZ protected are the surfaces the message structure is 
>>>>>> pointing to.
>>>>>> So what you would need to do is to completely parse the structure 
>>>>>> and then
>>>>>> move on the relevant buffers into VRAM.
>>>>>>
>>>>>> Leo or James, can you help with that?
>>>>>   From my observations, when decoding secure contents, register
>>>>> GPCOM_VCPU_DATA0 and GPCOM_VCPU_DATA1 are set to a TMZ buffer 
>>>>> address.
>>>>> And this way works when allocating TMZ buffers in GTT domain.
>>>> As far as I remember that's only the case for the decoding, 
>>>> encoding works
>>>> by putting the addresses into the message buffer.
>>>>
>>>> But could be that decoding is sufficient, Leo and James need to 
>>>> comment on
>>>> this.
>>> It seems that only decode needs TMZ buffers. Only observe 
>>> si_vid_create_tmz_buffer()
>>> was called in rvcn_dec_message_decode() in 
>>> src/gallium/drivers/radeon/radeon_vcn_dec.c.
>>
>> Mhm, good point. Let's wait for Leo and James to wake up, when we 
>> don't need encode support than that would makes things much easier.
>
> For secure playback, the buffer required in TMZ are dpb, dt and ctx, 
> for the rest esp. those for CPU access don't need that E.g. msg 
> buffer, and bitstream buffer.
>
> From radeon_vcn_dec.c, you can see the buffer for dpb and ctx, and dt 
> buffer frontend/va/surface is set to PIPE_BIND_PROTECTED.
>
>
> Regards,
>
> Leo
>
For VCN1, due to performance reason, the msg and fb buffer was allocated 
into VRAM instead of GTT(for other HW), but those are not TMZ in order 
to have CPU access.


Regards,

Leo



>
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>> Lang
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Regards,
>>>>> Lang
>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>> +
>>>>>>> +    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;
>>>>>>> +
>>>>>>> +    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 +1981,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 +
>>

Re: [PATCH v2] drm/amdgpu: add workarounds for Raven VCN TMZ issue

[James Zhu]

[-- Attachment #1: Type: text/plain, Size: 8602 bytes --]

+BoYuan

I am not sure if we need add ENCRYPT check when allocate surface if is 
supposed to be TMZ protected . For example:

VAStatus
*vlVaHandleSurfaceAllocate*(vlVaDriver *drv, vlVaSurface *surface,
                           struct pipe_video_buffer *templat,
                           const uint64_t *modifiers,
                           unsigned int modifiers_count)
{
    struct pipe_surface **surfaces;
    unsigned i;

*    if (is_encrypted && **templat) {/***is_encrypted is pseudocode*/
*

***templat->bind |= PIPE_BIND_PROTECTED;**
*

*    }*
    if (modifiers_count > 0) {
       if (!drv->pipe->create_video_buffer_with_modifiers)
          return VA_STATUS_ERROR_ATTR_NOT_SUPPORTED;
       surface->buffer =
drv->pipe->create_video_buffer_with_modifiers(drv->pipe, templat,
                                                        modifiers,
modifiers_count);
    } else {
       surface->buffer = drv->pipe->create_video_buffer(drv->pipe, templat);
    }

Best Regards!

James

On 2022-03-08 11:30 a.m., Leo Liu wrote:
>
> On 2022-03-08 11:18, Leo Liu wrote:
>>
>> On 2022-03-08 04:16, Christian König wrote:
>>> Am 08.03.22 um 09:06 schrieb Lang Yu:
>>>> On 03/08/ , Christian König wrote:
>>>>> Am 08.03.22 um 08:33 schrieb Lang Yu:
>>>>>> On 03/08/ , Christian König wrote:
>>>>>>> Am 08.03.22 um 04:39 schrieb Lang Yu:
>>>>>>>> It is a hardware issue that VCN can't handle a GTT
>>>>>>>> backing stored TMZ buffer on Raven.
>>>>>>>>
>>>>>>>> Move such a TMZ buffer to VRAM domain before command
>>>>>>>> submission.
>>>>>>>>
>>>>>>>> v2:
>>>>>>>>     - Use patch_cs_in_place callback.
>>>>>>>>
>>>>>>>> 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 | 68 
>>>>>>>> +++++++++++++++++++++++++++
>>>>>>>>     1 file changed, 68 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..810932abd3af 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,72 @@ 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 Raven VCN can't handle a GTT 
>>>>>>>> TMZ buffer.
>>>>>>>> + * Move such a GTT TMZ buffer to VRAM domain before command 
>>>>>>>> submission.
>>>>>>>> + */
>>>>>>>> +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);
>>>>>>>> +        return r;
>>>>>>>> +    }
>>>>>>> Well, exactly that won't work.
>>>>>>>
>>>>>>> The message structure isn't TMZ protected because otherwise the 
>>>>>>> driver won't
>>>>>>> be able to stitch it together.
>>>>>>>
>>>>>>> What is TMZ protected are the surfaces the message structure is 
>>>>>>> pointing to.
>>>>>>> So what you would need to do is to completely parse the 
>>>>>>> structure and then
>>>>>>> move on the relevant buffers into VRAM.
>>>>>>>
>>>>>>> Leo or James, can you help with that?
>>>>>>   From my observations, when decoding secure contents, register
>>>>>> GPCOM_VCPU_DATA0 and GPCOM_VCPU_DATA1 are set to a TMZ buffer 
>>>>>> address.
>>>>>> And this way works when allocating TMZ buffers in GTT domain.
>>>>> As far as I remember that's only the case for the decoding, 
>>>>> encoding works
>>>>> by putting the addresses into the message buffer.
>>>>>
>>>>> But could be that decoding is sufficient, Leo and James need to 
>>>>> comment on
>>>>> this.
>>>> It seems that only decode needs TMZ buffers. Only observe 
>>>> si_vid_create_tmz_buffer()
>>>> was called in rvcn_dec_message_decode() in 
>>>> src/gallium/drivers/radeon/radeon_vcn_dec.c.
>>>
>>> Mhm, good point. Let's wait for Leo and James to wake up, when we 
>>> don't need encode support than that would makes things much easier.
>>
>> For secure playback, the buffer required in TMZ are dpb, dt and ctx, 
>> for the rest esp. those for CPU access don't need that E.g. msg 
>> buffer, and bitstream buffer.
>>
>> From radeon_vcn_dec.c, you can see the buffer for dpb and ctx, and dt 
>> buffer frontend/va/surface is set to PIPE_BIND_PROTECTED.
>>
>>
>> Regards,
>>
>> Leo
>>
> For VCN1, due to performance reason, the msg and fb buffer was 
> allocated into VRAM instead of GTT(for other HW), but those are not 
> TMZ in order to have CPU access.
>
>
> Regards,
>
> Leo
>
>
>
>>
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Regards,
>>>> Lang
>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>> Regards,
>>>>>> Lang
>>>>>>
>>>>>>> Regards,
>>>>>>> Christian.
>>>>>>>
>>>>>>>> +
>>>>>>>> +    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;
>>>>>>>> +
>>>>>>>> +    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 +1981,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 +
>>>

[-- Attachment #2: Type: text/html, Size: 18766 bytes --]

Re: [PATCH v2] drm/amdgpu: add workarounds for Raven VCN TMZ issue

[Lang Yu]

Ping.

On 03/08/ , Leo Liu wrote:
> 
> On 2022-03-08 11:18, Leo Liu wrote:
> > 
> > On 2022-03-08 04:16, Christian König wrote:
> > > Am 08.03.22 um 09:06 schrieb Lang Yu:
> > > > On 03/08/ , Christian König wrote:
> > > > > Am 08.03.22 um 08:33 schrieb Lang Yu:
> > > > > > On 03/08/ , Christian König wrote:
> > > > > > > Am 08.03.22 um 04:39 schrieb Lang Yu:
> > > > > > > > It is a hardware issue that VCN can't handle a GTT
> > > > > > > > backing stored TMZ buffer on Raven.
> > > > > > > > 
> > > > > > > > Move such a TMZ buffer to VRAM domain before command
> > > > > > > > submission.
> > > > > > > > 
> > > > > > > > v2:
> > > > > > > >     - Use patch_cs_in_place callback.
> > > > > > > > 
> > > > > > > > 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 | 68
> > > > > > > > +++++++++++++++++++++++++++
> > > > > > > >     1 file changed, 68 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..810932abd3af 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,72 @@ 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 Raven VCN can't
> > > > > > > > handle a GTT TMZ buffer.
> > > > > > > > + * Move such a GTT TMZ buffer to VRAM domain
> > > > > > > > before command submission.
> > > > > > > > + */
> > > > > > > > +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);
> > > > > > > > +        return r;
> > > > > > > > +    }
> > > > > > > Well, exactly that won't work.
> > > > > > > 
> > > > > > > The message structure isn't TMZ protected because
> > > > > > > otherwise the driver won't
> > > > > > > be able to stitch it together.
> > > > > > > 
> > > > > > > What is TMZ protected are the surfaces the message
> > > > > > > structure is pointing to.
> > > > > > > So what you would need to do is to completely parse
> > > > > > > the structure and then
> > > > > > > move on the relevant buffers into VRAM.
> > > > > > > 
> > > > > > > Leo or James, can you help with that?
> > > > > >   From my observations, when decoding secure contents, register
> > > > > > GPCOM_VCPU_DATA0 and GPCOM_VCPU_DATA1 are set to a TMZ
> > > > > > buffer address.
> > > > > > And this way works when allocating TMZ buffers in GTT domain.
> > > > > As far as I remember that's only the case for the decoding,
> > > > > encoding works
> > > > > by putting the addresses into the message buffer.
> > > > > 
> > > > > But could be that decoding is sufficient, Leo and James need
> > > > > to comment on
> > > > > this.
> > > > It seems that only decode needs TMZ buffers. Only observe
> > > > si_vid_create_tmz_buffer()
> > > > was called in rvcn_dec_message_decode() in
> > > > src/gallium/drivers/radeon/radeon_vcn_dec.c.
> > > 
> > > Mhm, good point. Let's wait for Leo and James to wake up, when we
> > > don't need encode support than that would makes things much easier.
> > 
> > For secure playback, the buffer required in TMZ are dpb, dt and ctx, for
> > the rest esp. those for CPU access don't need that E.g. msg buffer, and
> > bitstream buffer.
> > 
> > From radeon_vcn_dec.c, you can see the buffer for dpb and ctx, and dt
> > buffer frontend/va/surface is set to PIPE_BIND_PROTECTED.
> > 
> > 
> > Regards,
> > 
> > Leo
> > 
> For VCN1, due to performance reason, the msg and fb buffer was allocated
> into VRAM instead of GTT(for other HW), but those are not TMZ in order to
> have CPU access.
> 
> 
> Regards,
> 
> Leo
> 
> 
> 
> > 
> > 
> > > 
> > > Regards,
> > > Christian.
> > > 
> > > > 
> > > > Regards,
> > > > Lang
> > > > 
> > > > > Regards,
> > > > > Christian.
> > > > > 
> > > > > > Regards,
> > > > > > Lang
> > > > > > 
> > > > > > > Regards,
> > > > > > > Christian.
> > > > > > > 
> > > > > > > > +
> > > > > > > > +    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;
> > > > > > > > +
> > > > > > > > +    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 +1981,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 +
> > > 

Re: [PATCH v2] drm/amdgpu: add workarounds for Raven VCN TMZ issue

[Christian König]

Leo you didn't answered the question if we need TMZ for encode as well.

Regards,
Christian.

Am 10.03.22 um 09:45 schrieb Lang Yu:
> Ping.
>
> On 03/08/ , Leo Liu wrote:
>> On 2022-03-08 11:18, Leo Liu wrote:
>>> On 2022-03-08 04:16, Christian König wrote:
>>>> Am 08.03.22 um 09:06 schrieb Lang Yu:
>>>>> On 03/08/ , Christian König wrote:
>>>>>> Am 08.03.22 um 08:33 schrieb Lang Yu:
>>>>>>> On 03/08/ , Christian König wrote:
>>>>>>>> Am 08.03.22 um 04:39 schrieb Lang Yu:
>>>>>>>>> It is a hardware issue that VCN can't handle a GTT
>>>>>>>>> backing stored TMZ buffer on Raven.
>>>>>>>>>
>>>>>>>>> Move such a TMZ buffer to VRAM domain before command
>>>>>>>>> submission.
>>>>>>>>>
>>>>>>>>> v2:
>>>>>>>>>      - Use patch_cs_in_place callback.
>>>>>>>>>
>>>>>>>>> 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 | 68
>>>>>>>>> +++++++++++++++++++++++++++
>>>>>>>>>      1 file changed, 68 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..810932abd3af 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,72 @@ 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 Raven VCN can't
>>>>>>>>> handle a GTT TMZ buffer.
>>>>>>>>> + * Move such a GTT TMZ buffer to VRAM domain
>>>>>>>>> before command submission.
>>>>>>>>> + */
>>>>>>>>> +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);
>>>>>>>>> +        return r;
>>>>>>>>> +    }
>>>>>>>> Well, exactly that won't work.
>>>>>>>>
>>>>>>>> The message structure isn't TMZ protected because
>>>>>>>> otherwise the driver won't
>>>>>>>> be able to stitch it together.
>>>>>>>>
>>>>>>>> What is TMZ protected are the surfaces the message
>>>>>>>> structure is pointing to.
>>>>>>>> So what you would need to do is to completely parse
>>>>>>>> the structure and then
>>>>>>>> move on the relevant buffers into VRAM.
>>>>>>>>
>>>>>>>> Leo or James, can you help with that?
>>>>>>>    From my observations, when decoding secure contents, register
>>>>>>> GPCOM_VCPU_DATA0 and GPCOM_VCPU_DATA1 are set to a TMZ
>>>>>>> buffer address.
>>>>>>> And this way works when allocating TMZ buffers in GTT domain.
>>>>>> As far as I remember that's only the case for the decoding,
>>>>>> encoding works
>>>>>> by putting the addresses into the message buffer.
>>>>>>
>>>>>> But could be that decoding is sufficient, Leo and James need
>>>>>> to comment on
>>>>>> this.
>>>>> It seems that only decode needs TMZ buffers. Only observe
>>>>> si_vid_create_tmz_buffer()
>>>>> was called in rvcn_dec_message_decode() in
>>>>> src/gallium/drivers/radeon/radeon_vcn_dec.c.
>>>> Mhm, good point. Let's wait for Leo and James to wake up, when we
>>>> don't need encode support than that would makes things much easier.
>>> For secure playback, the buffer required in TMZ are dpb, dt and ctx, for
>>> the rest esp. those for CPU access don't need that E.g. msg buffer, and
>>> bitstream buffer.
>>>
>>>  From radeon_vcn_dec.c, you can see the buffer for dpb and ctx, and dt
>>> buffer frontend/va/surface is set to PIPE_BIND_PROTECTED.
>>>
>>>
>>> Regards,
>>>
>>> Leo
>>>
>> For VCN1, due to performance reason, the msg and fb buffer was allocated
>> into VRAM instead of GTT(for other HW), but those are not TMZ in order to
>> have CPU access.
>>
>>
>> Regards,
>>
>> Leo
>>
>>
>>
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Regards,
>>>>> Lang
>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>> Regards,
>>>>>>> Lang
>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Christian.
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +    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;
>>>>>>>>> +
>>>>>>>>> +    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 +1981,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 +

Re: [PATCH v2] drm/amdgpu: add workarounds for Raven VCN TMZ issue

[Leo Liu]

No need for encode. Encrypting uses TEE/TA to convert clear bitstream to 
encrypted bitstream, and has nothing to do with VCN encode and tmz.

Regards,

Leo


On 2022-03-10 04:53, Christian König wrote:
> Leo you didn't answered the question if we need TMZ for encode as well.
>
> Regards,
> Christian.
>
> Am 10.03.22 um 09:45 schrieb Lang Yu:
>> Ping.
>>
>> On 03/08/ , Leo Liu wrote:
>>> On 2022-03-08 11:18, Leo Liu wrote:
>>>> On 2022-03-08 04:16, Christian König wrote:
>>>>> Am 08.03.22 um 09:06 schrieb Lang Yu:
>>>>>> On 03/08/ , Christian König wrote:
>>>>>>> Am 08.03.22 um 08:33 schrieb Lang Yu:
>>>>>>>> On 03/08/ , Christian König wrote:
>>>>>>>>> Am 08.03.22 um 04:39 schrieb Lang Yu:
>>>>>>>>>> It is a hardware issue that VCN can't handle a GTT
>>>>>>>>>> backing stored TMZ buffer on Raven.
>>>>>>>>>>
>>>>>>>>>> Move such a TMZ buffer to VRAM domain before command
>>>>>>>>>> submission.
>>>>>>>>>>
>>>>>>>>>> v2:
>>>>>>>>>>      - Use patch_cs_in_place callback.
>>>>>>>>>>
>>>>>>>>>> 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 | 68
>>>>>>>>>> +++++++++++++++++++++++++++
>>>>>>>>>>      1 file changed, 68 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..810932abd3af 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,72 @@ 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 Raven VCN can't
>>>>>>>>>> handle a GTT TMZ buffer.
>>>>>>>>>> + * Move such a GTT TMZ buffer to VRAM domain
>>>>>>>>>> before command submission.
>>>>>>>>>> + */
>>>>>>>>>> +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);
>>>>>>>>>> +        return r;
>>>>>>>>>> +    }
>>>>>>>>> Well, exactly that won't work.
>>>>>>>>>
>>>>>>>>> The message structure isn't TMZ protected because
>>>>>>>>> otherwise the driver won't
>>>>>>>>> be able to stitch it together.
>>>>>>>>>
>>>>>>>>> What is TMZ protected are the surfaces the message
>>>>>>>>> structure is pointing to.
>>>>>>>>> So what you would need to do is to completely parse
>>>>>>>>> the structure and then
>>>>>>>>> move on the relevant buffers into VRAM.
>>>>>>>>>
>>>>>>>>> Leo or James, can you help with that?
>>>>>>>>    From my observations, when decoding secure contents, register
>>>>>>>> GPCOM_VCPU_DATA0 and GPCOM_VCPU_DATA1 are set to a TMZ
>>>>>>>> buffer address.
>>>>>>>> And this way works when allocating TMZ buffers in GTT domain.
>>>>>>> As far as I remember that's only the case for the decoding,
>>>>>>> encoding works
>>>>>>> by putting the addresses into the message buffer.
>>>>>>>
>>>>>>> But could be that decoding is sufficient, Leo and James need
>>>>>>> to comment on
>>>>>>> this.
>>>>>> It seems that only decode needs TMZ buffers. Only observe
>>>>>> si_vid_create_tmz_buffer()
>>>>>> was called in rvcn_dec_message_decode() in
>>>>>> src/gallium/drivers/radeon/radeon_vcn_dec.c.
>>>>> Mhm, good point. Let's wait for Leo and James to wake up, when we
>>>>> don't need encode support than that would makes things much easier.
>>>> For secure playback, the buffer required in TMZ are dpb, dt and 
>>>> ctx, for
>>>> the rest esp. those for CPU access don't need that E.g. msg buffer, 
>>>> and
>>>> bitstream buffer.
>>>>
>>>>  From radeon_vcn_dec.c, you can see the buffer for dpb and ctx, and dt
>>>> buffer frontend/va/surface is set to PIPE_BIND_PROTECTED.
>>>>
>>>>
>>>> Regards,
>>>>
>>>> Leo
>>>>
>>> For VCN1, due to performance reason, the msg and fb buffer was 
>>> allocated
>>> into VRAM instead of GTT(for other HW), but those are not TMZ in 
>>> order to
>>> have CPU access.
>>>
>>>
>>> Regards,
>>>
>>> Leo
>>>
>>>
>>>
>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>> Regards,
>>>>>> Lang
>>>>>>
>>>>>>> Regards,
>>>>>>> Christian.
>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Lang
>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Christian.
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +    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;
>>>>>>>>>> +
>>>>>>>>>> +    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 +1981,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 +
>

Re: [PATCH v2] drm/amdgpu: add workarounds for Raven VCN TMZ issue

[Christian König]

Ok, thanks.

Lang is that case your patch should work fine.

Just add another patch with a check for the encode case to reject any CS 
with TMZ buffers in it.

Thanks,
Christian.

Am 10.03.22 um 15:17 schrieb Leo Liu:
> No need for encode. Encrypting uses TEE/TA to convert clear bitstream 
> to encrypted bitstream, and has nothing to do with VCN encode and tmz.
>
> Regards,
>
> Leo
>
>
> On 2022-03-10 04:53, Christian König wrote:
>> Leo you didn't answered the question if we need TMZ for encode as well.
>>
>> Regards,
>> Christian.
>>
>> Am 10.03.22 um 09:45 schrieb Lang Yu:
>>> Ping.
>>>
>>> On 03/08/ , Leo Liu wrote:
>>>> On 2022-03-08 11:18, Leo Liu wrote:
>>>>> On 2022-03-08 04:16, Christian König wrote:
>>>>>> Am 08.03.22 um 09:06 schrieb Lang Yu:
>>>>>>> On 03/08/ , Christian König wrote:
>>>>>>>> Am 08.03.22 um 08:33 schrieb Lang Yu:
>>>>>>>>> On 03/08/ , Christian König wrote:
>>>>>>>>>> Am 08.03.22 um 04:39 schrieb Lang Yu:
>>>>>>>>>>> It is a hardware issue that VCN can't handle a GTT
>>>>>>>>>>> backing stored TMZ buffer on Raven.
>>>>>>>>>>>
>>>>>>>>>>> Move such a TMZ buffer to VRAM domain before command
>>>>>>>>>>> submission.
>>>>>>>>>>>
>>>>>>>>>>> v2:
>>>>>>>>>>>      - Use patch_cs_in_place callback.
>>>>>>>>>>>
>>>>>>>>>>> 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 | 68
>>>>>>>>>>> +++++++++++++++++++++++++++
>>>>>>>>>>>      1 file changed, 68 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..810932abd3af 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,72 @@ 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 Raven VCN can't
>>>>>>>>>>> handle a GTT TMZ buffer.
>>>>>>>>>>> + * Move such a GTT TMZ buffer to VRAM domain
>>>>>>>>>>> before command submission.
>>>>>>>>>>> + */
>>>>>>>>>>> +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);
>>>>>>>>>>> +        return r;
>>>>>>>>>>> +    }
>>>>>>>>>> Well, exactly that won't work.
>>>>>>>>>>
>>>>>>>>>> The message structure isn't TMZ protected because
>>>>>>>>>> otherwise the driver won't
>>>>>>>>>> be able to stitch it together.
>>>>>>>>>>
>>>>>>>>>> What is TMZ protected are the surfaces the message
>>>>>>>>>> structure is pointing to.
>>>>>>>>>> So what you would need to do is to completely parse
>>>>>>>>>> the structure and then
>>>>>>>>>> move on the relevant buffers into VRAM.
>>>>>>>>>>
>>>>>>>>>> Leo or James, can you help with that?
>>>>>>>>>    From my observations, when decoding secure contents, register
>>>>>>>>> GPCOM_VCPU_DATA0 and GPCOM_VCPU_DATA1 are set to a TMZ
>>>>>>>>> buffer address.
>>>>>>>>> And this way works when allocating TMZ buffers in GTT domain.
>>>>>>>> As far as I remember that's only the case for the decoding,
>>>>>>>> encoding works
>>>>>>>> by putting the addresses into the message buffer.
>>>>>>>>
>>>>>>>> But could be that decoding is sufficient, Leo and James need
>>>>>>>> to comment on
>>>>>>>> this.
>>>>>>> It seems that only decode needs TMZ buffers. Only observe
>>>>>>> si_vid_create_tmz_buffer()
>>>>>>> was called in rvcn_dec_message_decode() in
>>>>>>> src/gallium/drivers/radeon/radeon_vcn_dec.c.
>>>>>> Mhm, good point. Let's wait for Leo and James to wake up, when we
>>>>>> don't need encode support than that would makes things much easier.
>>>>> For secure playback, the buffer required in TMZ are dpb, dt and 
>>>>> ctx, for
>>>>> the rest esp. those for CPU access don't need that E.g. msg 
>>>>> buffer, and
>>>>> bitstream buffer.
>>>>>
>>>>>  From radeon_vcn_dec.c, you can see the buffer for dpb and ctx, 
>>>>> and dt
>>>>> buffer frontend/va/surface is set to PIPE_BIND_PROTECTED.
>>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> Leo
>>>>>
>>>> For VCN1, due to performance reason, the msg and fb buffer was 
>>>> allocated
>>>> into VRAM instead of GTT(for other HW), but those are not TMZ in 
>>>> order to
>>>> have CPU access.
>>>>
>>>>
>>>> Regards,
>>>>
>>>> Leo
>>>>
>>>>
>>>>
>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>> Regards,
>>>>>>> Lang
>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Christian.
>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Lang
>>>>>>>>>
>>>>>>>>>> Regards,
>>>>>>>>>> Christian.
>>>>>>>>>>
>>>>>>>>>>> +
>>>>>>>>>>> +    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;
>>>>>>>>>>> +
>>>>>>>>>>> +    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 +1981,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 +
>>

Re: [PATCH v2] drm/amdgpu: add workarounds for Raven VCN TMZ issue

[Lang Yu]

On 03/10/ , Christian König wrote:
> Ok, thanks.
> 
> Lang is that case your patch should work fine.
> 
> Just add another patch with a check for the encode case to reject any CS
> with TMZ buffers in it.

Only VCN decode ring is cared in this patch. For encode ring
(AMDGPU_HW_IP_VCN_ENC and AMDGPU_HW_IP_VCN_JPEG), is it fine 
we just reject secure IBs in amdgpu_ib_schedule like compute ring?

Regards,
Lang

> Thanks,
> Christian.
> 
> Am 10.03.22 um 15:17 schrieb Leo Liu:
> > No need for encode. Encrypting uses TEE/TA to convert clear bitstream to
> > encrypted bitstream, and has nothing to do with VCN encode and tmz.
> > 
> > Regards,
> > 
> > Leo
> > 
> > 
> > On 2022-03-10 04:53, Christian König wrote:
> > > Leo you didn't answered the question if we need TMZ for encode as well.
> > > 
> > > Regards,
> > > Christian.
> > > 
> > > Am 10.03.22 um 09:45 schrieb Lang Yu:
> > > > Ping.
> > > > 
> > > > On 03/08/ , Leo Liu wrote:
> > > > > On 2022-03-08 11:18, Leo Liu wrote:
> > > > > > On 2022-03-08 04:16, Christian König wrote:
> > > > > > > Am 08.03.22 um 09:06 schrieb Lang Yu:
> > > > > > > > On 03/08/ , Christian König wrote:
> > > > > > > > > Am 08.03.22 um 08:33 schrieb Lang Yu:
> > > > > > > > > > On 03/08/ , Christian König wrote:
> > > > > > > > > > > Am 08.03.22 um 04:39 schrieb Lang Yu:
> > > > > > > > > > > > It is a hardware issue that VCN can't handle a GTT
> > > > > > > > > > > > backing stored TMZ buffer on Raven.
> > > > > > > > > > > > 
> > > > > > > > > > > > Move such a TMZ buffer to VRAM domain before command
> > > > > > > > > > > > submission.
> > > > > > > > > > > > 
> > > > > > > > > > > > v2:
> > > > > > > > > > > >      - Use patch_cs_in_place callback.
> > > > > > > > > > > > 
> > > > > > > > > > > > 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 | 68
> > > > > > > > > > > > +++++++++++++++++++++++++++
> > > > > > > > > > > >      1 file changed, 68 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..810932abd3af 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,72 @@ 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 Raven VCN can't
> > > > > > > > > > > > handle a GTT TMZ buffer.
> > > > > > > > > > > > + * Move such a GTT TMZ buffer to VRAM domain
> > > > > > > > > > > > before command submission.
> > > > > > > > > > > > + */
> > > > > > > > > > > > +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);
> > > > > > > > > > > > +        return r;
> > > > > > > > > > > > +    }
> > > > > > > > > > > Well, exactly that won't work.
> > > > > > > > > > > 
> > > > > > > > > > > The message structure isn't TMZ protected because
> > > > > > > > > > > otherwise the driver won't
> > > > > > > > > > > be able to stitch it together.
> > > > > > > > > > > 
> > > > > > > > > > > What is TMZ protected are the surfaces the message
> > > > > > > > > > > structure is pointing to.
> > > > > > > > > > > So what you would need to do is to completely parse
> > > > > > > > > > > the structure and then
> > > > > > > > > > > move on the relevant buffers into VRAM.
> > > > > > > > > > > 
> > > > > > > > > > > Leo or James, can you help with that?
> > > > > > > > > >    From my observations, when decoding secure contents, register
> > > > > > > > > > GPCOM_VCPU_DATA0 and GPCOM_VCPU_DATA1 are set to a TMZ
> > > > > > > > > > buffer address.
> > > > > > > > > > And this way works when allocating TMZ buffers in GTT domain.
> > > > > > > > > As far as I remember that's only the case for the decoding,
> > > > > > > > > encoding works
> > > > > > > > > by putting the addresses into the message buffer.
> > > > > > > > > 
> > > > > > > > > But could be that decoding is sufficient, Leo and James need
> > > > > > > > > to comment on
> > > > > > > > > this.
> > > > > > > > It seems that only decode needs TMZ buffers. Only observe
> > > > > > > > si_vid_create_tmz_buffer()
> > > > > > > > was called in rvcn_dec_message_decode() in
> > > > > > > > src/gallium/drivers/radeon/radeon_vcn_dec.c.
> > > > > > > Mhm, good point. Let's wait for Leo and James to wake up, when we
> > > > > > > don't need encode support than that would makes things much easier.
> > > > > > For secure playback, the buffer required in TMZ are dpb,
> > > > > > dt and ctx, for
> > > > > > the rest esp. those for CPU access don't need that E.g.
> > > > > > msg buffer, and
> > > > > > bitstream buffer.
> > > > > > 
> > > > > >  From radeon_vcn_dec.c, you can see the buffer for dpb
> > > > > > and ctx, and dt
> > > > > > buffer frontend/va/surface is set to PIPE_BIND_PROTECTED.
> > > > > > 
> > > > > > 
> > > > > > Regards,
> > > > > > 
> > > > > > Leo
> > > > > > 
> > > > > For VCN1, due to performance reason, the msg and fb buffer
> > > > > was allocated
> > > > > into VRAM instead of GTT(for other HW), but those are not
> > > > > TMZ in order to
> > > > > have CPU access.
> > > > > 
> > > > > 
> > > > > Regards,
> > > > > 
> > > > > Leo
> > > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > > Regards,
> > > > > > > Christian.
> > > > > > > 
> > > > > > > > Regards,
> > > > > > > > Lang
> > > > > > > > 
> > > > > > > > > Regards,
> > > > > > > > > Christian.
> > > > > > > > > 
> > > > > > > > > > Regards,
> > > > > > > > > > Lang
> > > > > > > > > > 
> > > > > > > > > > > Regards,
> > > > > > > > > > > Christian.
> > > > > > > > > > > 
> > > > > > > > > > > > +
> > > > > > > > > > > > +    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;
> > > > > > > > > > > > +
> > > > > > > > > > > > +    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 +1981,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 +
> > > 
> 

Re: [PATCH v2] drm/amdgpu: add workarounds for Raven VCN TMZ issue

[Alex Deucher]

On Thu, Mar 10, 2022 at 9:32 PM Lang Yu <Lang.Yu@amd.com> wrote:
>
> On 03/10/ , Christian König wrote:
> > Ok, thanks.
> >
> > Lang is that case your patch should work fine.
> >
> > Just add another patch with a check for the encode case to reject any CS
> > with TMZ buffers in it.
>
> Only VCN decode ring is cared in this patch. For encode ring
> (AMDGPU_HW_IP_VCN_ENC and AMDGPU_HW_IP_VCN_JPEG), is it fine
> we just reject secure IBs in amdgpu_ib_schedule like compute ring?
>

Yes,  correct.

Alex

> Regards,
> Lang
>
> > Thanks,
> > Christian.
> >
> > Am 10.03.22 um 15:17 schrieb Leo Liu:
> > > No need for encode. Encrypting uses TEE/TA to convert clear bitstream to
> > > encrypted bitstream, and has nothing to do with VCN encode and tmz.
> > >
> > > Regards,
> > >
> > > Leo
> > >
> > >
> > > On 2022-03-10 04:53, Christian König wrote:
> > > > Leo you didn't answered the question if we need TMZ for encode as well.
> > > >
> > > > Regards,
> > > > Christian.
> > > >
> > > > Am 10.03.22 um 09:45 schrieb Lang Yu:
> > > > > Ping.
> > > > >
> > > > > On 03/08/ , Leo Liu wrote:
> > > > > > On 2022-03-08 11:18, Leo Liu wrote:
> > > > > > > On 2022-03-08 04:16, Christian König wrote:
> > > > > > > > Am 08.03.22 um 09:06 schrieb Lang Yu:
> > > > > > > > > On 03/08/ , Christian König wrote:
> > > > > > > > > > Am 08.03.22 um 08:33 schrieb Lang Yu:
> > > > > > > > > > > On 03/08/ , Christian König wrote:
> > > > > > > > > > > > Am 08.03.22 um 04:39 schrieb Lang Yu:
> > > > > > > > > > > > > It is a hardware issue that VCN can't handle a GTT
> > > > > > > > > > > > > backing stored TMZ buffer on Raven.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Move such a TMZ buffer to VRAM domain before command
> > > > > > > > > > > > > submission.
> > > > > > > > > > > > >
> > > > > > > > > > > > > v2:
> > > > > > > > > > > > >      - Use patch_cs_in_place callback.
> > > > > > > > > > > > >
> > > > > > > > > > > > > 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 | 68
> > > > > > > > > > > > > +++++++++++++++++++++++++++
> > > > > > > > > > > > >      1 file changed, 68 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..810932abd3af 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,72 @@ 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 Raven VCN can't
> > > > > > > > > > > > > handle a GTT TMZ buffer.
> > > > > > > > > > > > > + * Move such a GTT TMZ buffer to VRAM domain
> > > > > > > > > > > > > before command submission.
> > > > > > > > > > > > > + */
> > > > > > > > > > > > > +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);
> > > > > > > > > > > > > +        return r;
> > > > > > > > > > > > > +    }
> > > > > > > > > > > > Well, exactly that won't work.
> > > > > > > > > > > >
> > > > > > > > > > > > The message structure isn't TMZ protected because
> > > > > > > > > > > > otherwise the driver won't
> > > > > > > > > > > > be able to stitch it together.
> > > > > > > > > > > >
> > > > > > > > > > > > What is TMZ protected are the surfaces the message
> > > > > > > > > > > > structure is pointing to.
> > > > > > > > > > > > So what you would need to do is to completely parse
> > > > > > > > > > > > the structure and then
> > > > > > > > > > > > move on the relevant buffers into VRAM.
> > > > > > > > > > > >
> > > > > > > > > > > > Leo or James, can you help with that?
> > > > > > > > > > >    From my observations, when decoding secure contents, register
> > > > > > > > > > > GPCOM_VCPU_DATA0 and GPCOM_VCPU_DATA1 are set to a TMZ
> > > > > > > > > > > buffer address.
> > > > > > > > > > > And this way works when allocating TMZ buffers in GTT domain.
> > > > > > > > > > As far as I remember that's only the case for the decoding,
> > > > > > > > > > encoding works
> > > > > > > > > > by putting the addresses into the message buffer.
> > > > > > > > > >
> > > > > > > > > > But could be that decoding is sufficient, Leo and James need
> > > > > > > > > > to comment on
> > > > > > > > > > this.
> > > > > > > > > It seems that only decode needs TMZ buffers. Only observe
> > > > > > > > > si_vid_create_tmz_buffer()
> > > > > > > > > was called in rvcn_dec_message_decode() in
> > > > > > > > > src/gallium/drivers/radeon/radeon_vcn_dec.c.
> > > > > > > > Mhm, good point. Let's wait for Leo and James to wake up, when we
> > > > > > > > don't need encode support than that would makes things much easier.
> > > > > > > For secure playback, the buffer required in TMZ are dpb,
> > > > > > > dt and ctx, for
> > > > > > > the rest esp. those for CPU access don't need that E.g.
> > > > > > > msg buffer, and
> > > > > > > bitstream buffer.
> > > > > > >
> > > > > > >  From radeon_vcn_dec.c, you can see the buffer for dpb
> > > > > > > and ctx, and dt
> > > > > > > buffer frontend/va/surface is set to PIPE_BIND_PROTECTED.
> > > > > > >
> > > > > > >
> > > > > > > Regards,
> > > > > > >
> > > > > > > Leo
> > > > > > >
> > > > > > For VCN1, due to performance reason, the msg and fb buffer
> > > > > > was allocated
> > > > > > into VRAM instead of GTT(for other HW), but those are not
> > > > > > TMZ in order to
> > > > > > have CPU access.
> > > > > >
> > > > > >
> > > > > > Regards,
> > > > > >
> > > > > > Leo
> > > > > >
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > > Regards,
> > > > > > > > Christian.
> > > > > > > >
> > > > > > > > > Regards,
> > > > > > > > > Lang
> > > > > > > > >
> > > > > > > > > > Regards,
> > > > > > > > > > Christian.
> > > > > > > > > >
> > > > > > > > > > > Regards,
> > > > > > > > > > > Lang
> > > > > > > > > > >
> > > > > > > > > > > > Regards,
> > > > > > > > > > > > Christian.
> > > > > > > > > > > >
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +    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;
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +    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 +1981,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 +
> > > >
> >