[PATCH 1/2] drm/amd/amdgpu/vcn_v2_0: Mark RB commands as KMD commands

[PATCH 1/2] drm/amd/amdgpu/vcn_v2_0: Mark RB commands as KMD commands

[Thai, Thong]

Sets the CMD_SOURCE bit for VCN 2.0 Decoder Ring Buffer commands. This
bit was previously set by the RBC HW on older firmware. Newer firmware
uses a SW RBC and this bit has to be set by the driver.

Signed-off-by: Thong Thai <thong.thai@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h |  1 +
 drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c   | 12 ++++++------
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
index 5e2453ee6b29..4d3bf4adf1eb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
@@ -30,6 +30,7 @@
 #define AMDGPU_VCN_FIRMWARE_OFFSET	256
 #define AMDGPU_VCN_MAX_ENC_RINGS	3
 
+#define VCN_DEC_KMD_CMD			0x80000000
 #define VCN_DEC_CMD_FENCE		0x00000000
 #define VCN_DEC_CMD_TRAP		0x00000001
 #define VCN_DEC_CMD_WRITE_REG		0x00000004
diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
index bc9726787c97..7091aef95ff0 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
@@ -1488,7 +1488,7 @@ static void vcn_v2_0_dec_ring_insert_start(struct amdgpu_ring *ring)
 	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_DATA0_INTERNAL_OFFSET, 0));
 	amdgpu_ring_write(ring, 0);
 	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
-	amdgpu_ring_write(ring, VCN_DEC_CMD_PACKET_START << 1);
+	amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_PACKET_START << 1));
 }
 
 /**
@@ -1501,7 +1501,7 @@ static void vcn_v2_0_dec_ring_insert_start(struct amdgpu_ring *ring)
 static void vcn_v2_0_dec_ring_insert_end(struct amdgpu_ring *ring)
 {
 	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
-	amdgpu_ring_write(ring, VCN_DEC_CMD_PACKET_END << 1);
+	amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_PACKET_END << 1));
 }
 
 /**
@@ -1546,7 +1546,7 @@ static void vcn_v2_0_dec_ring_emit_fence(struct amdgpu_ring *ring, u64 addr, u64
 	amdgpu_ring_write(ring, upper_32_bits(addr) & 0xff);
 
 	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
-	amdgpu_ring_write(ring, VCN_DEC_CMD_FENCE << 1);
+	amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_FENCE << 1));
 
 	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_DATA0_INTERNAL_OFFSET, 0));
 	amdgpu_ring_write(ring, 0);
@@ -1556,7 +1556,7 @@ static void vcn_v2_0_dec_ring_emit_fence(struct amdgpu_ring *ring, u64 addr, u64
 
 	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
 
-	amdgpu_ring_write(ring, VCN_DEC_CMD_TRAP << 1);
+	amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_TRAP << 1));
 }
 
 /**
@@ -1600,7 +1600,7 @@ static void vcn_v2_0_dec_ring_emit_reg_wait(struct amdgpu_ring *ring,
 
 	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
 
-	amdgpu_ring_write(ring, VCN_DEC_CMD_REG_READ_COND_WAIT << 1);
+	amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_REG_READ_COND_WAIT << 1));
 }
 
 static void vcn_v2_0_dec_ring_emit_vm_flush(struct amdgpu_ring *ring,
@@ -1629,7 +1629,7 @@ static void vcn_v2_0_dec_ring_emit_wreg(struct amdgpu_ring *ring,
 
 	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
 
-	amdgpu_ring_write(ring, VCN_DEC_CMD_WRITE_REG << 1);
+	amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_WRITE_REG << 1));
 }
 
 /**
-- 
2.17.1

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

[PATCH 2/2] drm/amd/amdgpu/vcn_v2_0: Move VCN 2.0 specific dec ring test to vcn_v2_0

[Thai, Thong]

VCN 2.0 firmware now requires a packet start command to be sent before
any other decode ring buffer command.

Signed-off-by: Thong Thai <thong.thai@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c | 32 ++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
index 7091aef95ff0..4b00481cf9e0 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
@@ -2082,6 +2082,36 @@ static int vcn_v2_0_process_interrupt(struct amdgpu_device *adev,
 	return 0;
 }
 
+int vcn_v2_0_dec_ring_test_ring(struct amdgpu_ring *ring)
+{
+	struct amdgpu_device *adev = ring->adev;
+	uint32_t tmp = 0;
+	unsigned i;
+	int r;
+
+	WREG32(adev->vcn.external.scratch9, 0xCAFEDEAD);
+	r = amdgpu_ring_alloc(ring, 3);
+	if (r)
+		return r;
+	amdgpu_ring_write(ring, PACKET0(adev->vcn.internal.cmd, 0));
+	amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_PACKET_START << 1));
+	amdgpu_ring_write(ring, PACKET0(adev->vcn.internal.scratch9, 0));
+	amdgpu_ring_write(ring, 0xDEADBEEF);
+	amdgpu_ring_commit(ring);
+	for (i = 0; i < adev->usec_timeout; i++) {
+		tmp = RREG32(adev->vcn.external.scratch9);
+		if (tmp == 0xDEADBEEF)
+			break;
+		DRM_UDELAY(1);
+	}
+
+	if (i >= adev->usec_timeout)
+		r = -ETIMEDOUT;
+
+	return r;
+}
+
+
 static int vcn_v2_0_set_powergating_state(void *handle,
 					  enum amd_powergating_state state)
 {
@@ -2145,7 +2175,7 @@ static const struct amdgpu_ring_funcs vcn_v2_0_dec_ring_vm_funcs = {
 	.emit_ib = vcn_v2_0_dec_ring_emit_ib,
 	.emit_fence = vcn_v2_0_dec_ring_emit_fence,
 	.emit_vm_flush = vcn_v2_0_dec_ring_emit_vm_flush,
-	.test_ring = amdgpu_vcn_dec_ring_test_ring,
+	.test_ring = vcn_v2_0_dec_ring_test_ring,
 	.test_ib = amdgpu_vcn_dec_ring_test_ib,
 	.insert_nop = vcn_v2_0_dec_ring_insert_nop,
 	.insert_start = vcn_v2_0_dec_ring_insert_start,
-- 
2.17.1

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

[PATCH v2] drm/amd/amdgpu/vcn_v2_0: Mark RB commands as KMD commands

[Thai, Thong]

Sets the CMD_SOURCE bit for VCN 2.0 Decoder Ring Buffer commands. This
bit was previously set by the RBC HW on older firmware. Newer firmware
uses a SW RBC and this bit has to be set by the driver.

Signed-off-by: Thong Thai <thong.thai@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h |  1 +
 drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c   | 44 +++++++++++++++++++++----
 2 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
index 5e2453ee6b29..4d3bf4adf1eb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
@@ -30,6 +30,7 @@
 #define AMDGPU_VCN_FIRMWARE_OFFSET	256
 #define AMDGPU_VCN_MAX_ENC_RINGS	3
 
+#define VCN_DEC_KMD_CMD			0x80000000
 #define VCN_DEC_CMD_FENCE		0x00000000
 #define VCN_DEC_CMD_TRAP		0x00000001
 #define VCN_DEC_CMD_WRITE_REG		0x00000004
diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
index bc9726787c97..de928f1b1528 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
@@ -1488,7 +1488,7 @@ static void vcn_v2_0_dec_ring_insert_start(struct amdgpu_ring *ring)
 	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_DATA0_INTERNAL_OFFSET, 0));
 	amdgpu_ring_write(ring, 0);
 	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
-	amdgpu_ring_write(ring, VCN_DEC_CMD_PACKET_START << 1);
+	amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_PACKET_START << 1));
 }
 
 /**
@@ -1501,7 +1501,7 @@ static void vcn_v2_0_dec_ring_insert_start(struct amdgpu_ring *ring)
 static void vcn_v2_0_dec_ring_insert_end(struct amdgpu_ring *ring)
 {
 	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
-	amdgpu_ring_write(ring, VCN_DEC_CMD_PACKET_END << 1);
+	amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_PACKET_END << 1));
 }
 
 /**
@@ -1546,7 +1546,7 @@ static void vcn_v2_0_dec_ring_emit_fence(struct amdgpu_ring *ring, u64 addr, u64
 	amdgpu_ring_write(ring, upper_32_bits(addr) & 0xff);
 
 	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
-	amdgpu_ring_write(ring, VCN_DEC_CMD_FENCE << 1);
+	amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_FENCE << 1));
 
 	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_DATA0_INTERNAL_OFFSET, 0));
 	amdgpu_ring_write(ring, 0);
@@ -1556,7 +1556,7 @@ static void vcn_v2_0_dec_ring_emit_fence(struct amdgpu_ring *ring, u64 addr, u64
 
 	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
 
-	amdgpu_ring_write(ring, VCN_DEC_CMD_TRAP << 1);
+	amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_TRAP << 1));
 }
 
 /**
@@ -1600,7 +1600,7 @@ static void vcn_v2_0_dec_ring_emit_reg_wait(struct amdgpu_ring *ring,
 
 	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
 
-	amdgpu_ring_write(ring, VCN_DEC_CMD_REG_READ_COND_WAIT << 1);
+	amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_REG_READ_COND_WAIT << 1));
 }
 
 static void vcn_v2_0_dec_ring_emit_vm_flush(struct amdgpu_ring *ring,
@@ -1629,7 +1629,7 @@ static void vcn_v2_0_dec_ring_emit_wreg(struct amdgpu_ring *ring,
 
 	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
 
-	amdgpu_ring_write(ring, VCN_DEC_CMD_WRITE_REG << 1);
+	amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_WRITE_REG << 1));
 }
 
 /**
@@ -2082,6 +2082,36 @@ static int vcn_v2_0_process_interrupt(struct amdgpu_device *adev,
 	return 0;
 }
 
+int vcn_v2_0_dec_ring_test_ring(struct amdgpu_ring *ring)
+{
+	struct amdgpu_device *adev = ring->adev;
+	uint32_t tmp = 0;
+	unsigned i;
+	int r;
+
+	WREG32(adev->vcn.external.scratch9, 0xCAFEDEAD);
+	r = amdgpu_ring_alloc(ring, 4);
+	if (r)
+		return r;
+	amdgpu_ring_write(ring, PACKET0(adev->vcn.internal.cmd, 0));
+	amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_PACKET_START << 1));
+	amdgpu_ring_write(ring, PACKET0(adev->vcn.internal.scratch9, 0));
+	amdgpu_ring_write(ring, 0xDEADBEEF);
+	amdgpu_ring_commit(ring);
+	for (i = 0; i < adev->usec_timeout; i++) {
+		tmp = RREG32(adev->vcn.external.scratch9);
+		if (tmp == 0xDEADBEEF)
+			break;
+		DRM_UDELAY(1);
+	}
+
+	if (i >= adev->usec_timeout)
+		r = -ETIMEDOUT;
+
+	return r;
+}
+
+
 static int vcn_v2_0_set_powergating_state(void *handle,
 					  enum amd_powergating_state state)
 {
@@ -2145,7 +2175,7 @@ static const struct amdgpu_ring_funcs vcn_v2_0_dec_ring_vm_funcs = {
 	.emit_ib = vcn_v2_0_dec_ring_emit_ib,
 	.emit_fence = vcn_v2_0_dec_ring_emit_fence,
 	.emit_vm_flush = vcn_v2_0_dec_ring_emit_vm_flush,
-	.test_ring = amdgpu_vcn_dec_ring_test_ring,
+	.test_ring = vcn_v2_0_dec_ring_test_ring,
 	.test_ib = amdgpu_vcn_dec_ring_test_ib,
 	.insert_nop = vcn_v2_0_dec_ring_insert_nop,
 	.insert_start = vcn_v2_0_dec_ring_insert_start,
-- 
2.17.1

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

Re: [PATCH 1/2] drm/amd/amdgpu/vcn_v2_0: Mark RB commands as KMD commands

[Christian König]

Am 25.07.19 um 17:52 schrieb Thai, Thong:
> Sets the CMD_SOURCE bit for VCN 2.0 Decoder Ring Buffer commands. This
> bit was previously set by the RBC HW on older firmware. Newer firmware
> uses a SW RBC and this bit has to be set by the driver.

Mhm, another question came to my mind: Would it now be possible for user 
space to set this flag and and gain access to the kernel driver commands?

Cause that could be a security problem.

Christian.

>
> Signed-off-by: Thong Thai <thong.thai@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h |  1 +
>   drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c   | 12 ++++++------
>   2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> index 5e2453ee6b29..4d3bf4adf1eb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> @@ -30,6 +30,7 @@
>   #define AMDGPU_VCN_FIRMWARE_OFFSET	256
>   #define AMDGPU_VCN_MAX_ENC_RINGS	3
>   
> +#define VCN_DEC_KMD_CMD			0x80000000
>   #define VCN_DEC_CMD_FENCE		0x00000000
>   #define VCN_DEC_CMD_TRAP		0x00000001
>   #define VCN_DEC_CMD_WRITE_REG		0x00000004
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
> index bc9726787c97..7091aef95ff0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
> @@ -1488,7 +1488,7 @@ static void vcn_v2_0_dec_ring_insert_start(struct amdgpu_ring *ring)
>   	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_DATA0_INTERNAL_OFFSET, 0));
>   	amdgpu_ring_write(ring, 0);
>   	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
> -	amdgpu_ring_write(ring, VCN_DEC_CMD_PACKET_START << 1);
> +	amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_PACKET_START << 1));
>   }
>   
>   /**
> @@ -1501,7 +1501,7 @@ static void vcn_v2_0_dec_ring_insert_start(struct amdgpu_ring *ring)
>   static void vcn_v2_0_dec_ring_insert_end(struct amdgpu_ring *ring)
>   {
>   	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
> -	amdgpu_ring_write(ring, VCN_DEC_CMD_PACKET_END << 1);
> +	amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_PACKET_END << 1));
>   }
>   
>   /**
> @@ -1546,7 +1546,7 @@ static void vcn_v2_0_dec_ring_emit_fence(struct amdgpu_ring *ring, u64 addr, u64
>   	amdgpu_ring_write(ring, upper_32_bits(addr) & 0xff);
>   
>   	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
> -	amdgpu_ring_write(ring, VCN_DEC_CMD_FENCE << 1);
> +	amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_FENCE << 1));
>   
>   	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_DATA0_INTERNAL_OFFSET, 0));
>   	amdgpu_ring_write(ring, 0);
> @@ -1556,7 +1556,7 @@ static void vcn_v2_0_dec_ring_emit_fence(struct amdgpu_ring *ring, u64 addr, u64
>   
>   	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
>   
> -	amdgpu_ring_write(ring, VCN_DEC_CMD_TRAP << 1);
> +	amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_TRAP << 1));
>   }
>   
>   /**
> @@ -1600,7 +1600,7 @@ static void vcn_v2_0_dec_ring_emit_reg_wait(struct amdgpu_ring *ring,
>   
>   	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
>   
> -	amdgpu_ring_write(ring, VCN_DEC_CMD_REG_READ_COND_WAIT << 1);
> +	amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_REG_READ_COND_WAIT << 1));
>   }
>   
>   static void vcn_v2_0_dec_ring_emit_vm_flush(struct amdgpu_ring *ring,
> @@ -1629,7 +1629,7 @@ static void vcn_v2_0_dec_ring_emit_wreg(struct amdgpu_ring *ring,
>   
>   	amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
>   
> -	amdgpu_ring_write(ring, VCN_DEC_CMD_WRITE_REG << 1);
> +	amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_WRITE_REG << 1));
>   }
>   
>   /**

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

Re: [PATCH 1/2] drm/amd/amdgpu/vcn_v2_0: Mark RB commands as KMD commands

[Thai, Thong]

[-- Attachment #1.1: Type: text/plain, Size: 4692 bytes --]

Well, not through this particular piece of code, since this explicitly sets it. But I would imagine someone could set the bit in userspace and insert KMD commands in the BO as part of some IB instructions - I’ll have a look.

________________________________
From: Christian König <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Sent: Friday, July 26, 2019 3:17:19 AM
To: Thai, Thong <Thong.Thai-5C7GfCeVMHo@public.gmane.org>; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org <amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 1/2] drm/amd/amdgpu/vcn_v2_0: Mark RB commands as KMD commands

Am 25.07.19 um 17:52 schrieb Thai, Thong:
> Sets the CMD_SOURCE bit for VCN 2.0 Decoder Ring Buffer commands. This
> bit was previously set by the RBC HW on older firmware. Newer firmware
> uses a SW RBC and this bit has to be set by the driver.

Mhm, another question came to my mind: Would it now be possible for user
space to set this flag and and gain access to the kernel driver commands?

Cause that could be a security problem.

Christian.

>
> Signed-off-by: Thong Thai <thong.thai-5C7GfCeVMHo@public.gmane.org>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h |  1 +
>   drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c   | 12 ++++++------
>   2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> index 5e2453ee6b29..4d3bf4adf1eb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> @@ -30,6 +30,7 @@
>   #define AMDGPU_VCN_FIRMWARE_OFFSET  256
>   #define AMDGPU_VCN_MAX_ENC_RINGS    3
>
> +#define VCN_DEC_KMD_CMD                      0x80000000
>   #define VCN_DEC_CMD_FENCE           0x00000000
>   #define VCN_DEC_CMD_TRAP            0x00000001
>   #define VCN_DEC_CMD_WRITE_REG               0x00000004
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
> index bc9726787c97..7091aef95ff0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
> @@ -1488,7 +1488,7 @@ static void vcn_v2_0_dec_ring_insert_start(struct amdgpu_ring *ring)
>        amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_DATA0_INTERNAL_OFFSET, 0));
>        amdgpu_ring_write(ring, 0);
>        amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
> -     amdgpu_ring_write(ring, VCN_DEC_CMD_PACKET_START << 1);
> +     amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_PACKET_START << 1));
>   }
>
>   /**
> @@ -1501,7 +1501,7 @@ static void vcn_v2_0_dec_ring_insert_start(struct amdgpu_ring *ring)
>   static void vcn_v2_0_dec_ring_insert_end(struct amdgpu_ring *ring)
>   {
>        amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
> -     amdgpu_ring_write(ring, VCN_DEC_CMD_PACKET_END << 1);
> +     amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_PACKET_END << 1));
>   }
>
>   /**
> @@ -1546,7 +1546,7 @@ static void vcn_v2_0_dec_ring_emit_fence(struct amdgpu_ring *ring, u64 addr, u64
>        amdgpu_ring_write(ring, upper_32_bits(addr) & 0xff);
>
>        amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
> -     amdgpu_ring_write(ring, VCN_DEC_CMD_FENCE << 1);
> +     amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_FENCE << 1));
>
>        amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_DATA0_INTERNAL_OFFSET, 0));
>        amdgpu_ring_write(ring, 0);
> @@ -1556,7 +1556,7 @@ static void vcn_v2_0_dec_ring_emit_fence(struct amdgpu_ring *ring, u64 addr, u64
>
>        amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
>
> -     amdgpu_ring_write(ring, VCN_DEC_CMD_TRAP << 1);
> +     amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_TRAP << 1));
>   }
>
>   /**
> @@ -1600,7 +1600,7 @@ static void vcn_v2_0_dec_ring_emit_reg_wait(struct amdgpu_ring *ring,
>
>        amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
>
> -     amdgpu_ring_write(ring, VCN_DEC_CMD_REG_READ_COND_WAIT << 1);
> +     amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_REG_READ_COND_WAIT << 1));
>   }
>
>   static void vcn_v2_0_dec_ring_emit_vm_flush(struct amdgpu_ring *ring,
> @@ -1629,7 +1629,7 @@ static void vcn_v2_0_dec_ring_emit_wreg(struct amdgpu_ring *ring,
>
>        amdgpu_ring_write(ring, PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
>
> -     amdgpu_ring_write(ring, VCN_DEC_CMD_WRITE_REG << 1);
> +     amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_WRITE_REG << 1));
>   }
>
>   /**


[-- Attachment #1.2: Type: text/html, Size: 7619 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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

Re: [PATCH 1/2] drm/amd/amdgpu/vcn_v2_0: Mark RB commands as KMD commands

[Christian König]

[-- Attachment #1.1: Type: text/plain, Size: 5324 bytes --]

Good point, this patch is Reviewed-by: Christian König 
<christian.koenig-5C7GfCeVMHo@public.gmane.org>.

But please ping the firmware guys if that really could be an issue,
Christian.

Am 26.07.19 um 12:45 schrieb Thai, Thong:
> Well, not through this particular piece of code, since this explicitly 
> sets it. But I would imagine someone could set the bit in userspace 
> and insert KMD commands in the BO as part of some IB instructions - 
> I’ll have a look.
>
> ------------------------------------------------------------------------
> *From:* Christian König <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> *Sent:* Friday, July 26, 2019 3:17:19 AM
> *To:* Thai, Thong <Thong.Thai-5C7GfCeVMHo@public.gmane.org>; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org 
> <amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
> *Subject:* Re: [PATCH 1/2] drm/amd/amdgpu/vcn_v2_0: Mark RB commands 
> as KMD commands
> Am 25.07.19 um 17:52 schrieb Thai, Thong:
> > Sets the CMD_SOURCE bit for VCN 2.0 Decoder Ring Buffer commands. This
> > bit was previously set by the RBC HW on older firmware. Newer firmware
> > uses a SW RBC and this bit has to be set by the driver.
>
> Mhm, another question came to my mind: Would it now be possible for user
> space to set this flag and and gain access to the kernel driver commands?
>
> Cause that could be a security problem.
>
> Christian.
>
> >
> > Signed-off-by: Thong Thai <thong.thai-5C7GfCeVMHo@public.gmane.org>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h |  1 +
> >   drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c   | 12 ++++++------
> >   2 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> > index 5e2453ee6b29..4d3bf4adf1eb 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> > @@ -30,6 +30,7 @@
> >   #define AMDGPU_VCN_FIRMWARE_OFFSET  256
> >   #define AMDGPU_VCN_MAX_ENC_RINGS    3
> >
> > +#define VCN_DEC_KMD_CMD 0x80000000
> >   #define VCN_DEC_CMD_FENCE           0x00000000
> >   #define VCN_DEC_CMD_TRAP            0x00000001
> >   #define VCN_DEC_CMD_WRITE_REG 0x00000004
> > diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c 
> b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
> > index bc9726787c97..7091aef95ff0 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
> > @@ -1488,7 +1488,7 @@ static void 
> vcn_v2_0_dec_ring_insert_start(struct amdgpu_ring *ring)
> >        amdgpu_ring_write(ring, 
> PACKET0(mmUVD_GPCOM_VCPU_DATA0_INTERNAL_OFFSET, 0));
> >        amdgpu_ring_write(ring, 0);
> >        amdgpu_ring_write(ring, 
> PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
> > -     amdgpu_ring_write(ring, VCN_DEC_CMD_PACKET_START << 1);
> > +     amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | 
> (VCN_DEC_CMD_PACKET_START << 1));
> >   }
> >
> >   /**
> > @@ -1501,7 +1501,7 @@ static void 
> vcn_v2_0_dec_ring_insert_start(struct amdgpu_ring *ring)
> >   static void vcn_v2_0_dec_ring_insert_end(struct amdgpu_ring *ring)
> >   {
> >        amdgpu_ring_write(ring, 
> PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
> > -     amdgpu_ring_write(ring, VCN_DEC_CMD_PACKET_END << 1);
> > +     amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | 
> (VCN_DEC_CMD_PACKET_END << 1));
> >   }
> >
> >   /**
> > @@ -1546,7 +1546,7 @@ static void 
> vcn_v2_0_dec_ring_emit_fence(struct amdgpu_ring *ring, u64 addr, u64
> >        amdgpu_ring_write(ring, upper_32_bits(addr) & 0xff);
> >
> >        amdgpu_ring_write(ring, 
> PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
> > -     amdgpu_ring_write(ring, VCN_DEC_CMD_FENCE << 1);
> > +     amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_FENCE 
> << 1));
> >
> >        amdgpu_ring_write(ring, 
> PACKET0(mmUVD_GPCOM_VCPU_DATA0_INTERNAL_OFFSET, 0));
> >        amdgpu_ring_write(ring, 0);
> > @@ -1556,7 +1556,7 @@ static void 
> vcn_v2_0_dec_ring_emit_fence(struct amdgpu_ring *ring, u64 addr, u64
> >
> >        amdgpu_ring_write(ring, 
> PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
> >
> > -     amdgpu_ring_write(ring, VCN_DEC_CMD_TRAP << 1);
> > +     amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | (VCN_DEC_CMD_TRAP << 
> 1));
> >   }
> >
> >   /**
> > @@ -1600,7 +1600,7 @@ static void 
> vcn_v2_0_dec_ring_emit_reg_wait(struct amdgpu_ring *ring,
> >
> >        amdgpu_ring_write(ring, 
> PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
> >
> > -     amdgpu_ring_write(ring, VCN_DEC_CMD_REG_READ_COND_WAIT << 1);
> > +     amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | 
> (VCN_DEC_CMD_REG_READ_COND_WAIT << 1));
> >   }
> >
> >   static void vcn_v2_0_dec_ring_emit_vm_flush(struct amdgpu_ring *ring,
> > @@ -1629,7 +1629,7 @@ static void vcn_v2_0_dec_ring_emit_wreg(struct 
> amdgpu_ring *ring,
> >
> >        amdgpu_ring_write(ring, 
> PACKET0(mmUVD_GPCOM_VCPU_CMD_INTERNAL_OFFSET, 0));
> >
> > -     amdgpu_ring_write(ring, VCN_DEC_CMD_WRITE_REG << 1);
> > +     amdgpu_ring_write(ring, VCN_DEC_KMD_CMD | 
> (VCN_DEC_CMD_WRITE_REG << 1));
> >   }
> >
> >   /**
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


[-- Attachment #1.2: Type: text/html, Size: 9735 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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