* [PATCH v2] drm/amd/amdgpu: Use IP discovery data to determine VCN enablement instead of MMSCH
@ 2021-06-15 7:46 Peng Ju Zhou
2021-06-15 19:28 ` Alex Deucher
0 siblings, 1 reply; 3+ messages in thread
From: Peng Ju Zhou @ 2021-06-15 7:46 UTC (permalink / raw)
To: amd-gfx; +Cc: Bokun Zhang
From: Bokun Zhang <Bokun.Zhang@amd.com>
In the past, we use MMSCH to determine whether a VCN is enabled or not.
This is not reliable since after a FLR, MMSCH may report junk data.
It is better to use IP discovery data.
Signed-off-by: Bokun Zhang <Bokun.Zhang@amd.com>
Signed-off-by: Peng Ju Zhou <PengJu.Zhou@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 8 +++
drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h | 3 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 23 ++++++++
drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 13 +++++
drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c | 52 +++++--------------
5 files changed, 60 insertions(+), 39 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index f949ed8bfd9e..e02405a24fe3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -373,6 +373,14 @@ int amdgpu_discovery_get_ip_version(struct amdgpu_device *adev, int hw_id, int n
return -EINVAL;
}
+
+int amdgpu_discovery_get_vcn_version(struct amdgpu_device *adev, int vcn_instance,
+ int *major, int *minor, int *revision)
+{
+ return amdgpu_discovery_get_ip_version(adev, VCN_HWID,
+ vcn_instance, major, minor, revision);
+}
+
void amdgpu_discovery_harvest_ip(struct amdgpu_device *adev)
{
struct binary_header *bhdr;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h
index 02e340cd3a38..48e6b88cfdfe 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h
@@ -32,6 +32,9 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev);
void amdgpu_discovery_harvest_ip(struct amdgpu_device *adev);
int amdgpu_discovery_get_ip_version(struct amdgpu_device *adev, int hw_id, int number_instance,
int *major, int *minor, int *revision);
+
+int amdgpu_discovery_get_vcn_version(struct amdgpu_device *adev, int vcn_instance,
+ int *major, int *minor, int *revision);
int amdgpu_discovery_get_gfx_info(struct amdgpu_device *adev);
#endif /* __AMDGPU_DISCOVERY__ */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
index 9492b505e69b..84b025405578 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
@@ -287,6 +287,29 @@ int amdgpu_vcn_sw_fini(struct amdgpu_device *adev)
return 0;
}
+bool amdgpu_vcn_is_disabled_vcn(struct amdgpu_device *adev, enum vcn_ring_type type, uint32_t vcn_instance)
+{
+ bool ret = false;
+
+ int major;
+ int minor;
+ int revision;
+
+ /* if cannot find IP data, then this VCN does not exist */
+ if (amdgpu_discovery_get_vcn_version(adev, vcn_instance, &major, &minor, &revision) != 0)
+ return true;
+
+ if ((type == VCN_ENCODE_RING) && (revision & VCN_BLOCK_ENCODE_DISABLE_MASK)) {
+ ret = true;
+ } else if ((type == VCN_DECODE_RING) && (revision & VCN_BLOCK_DECODE_DISABLE_MASK)) {
+ ret = true;
+ } else if ((type == VCN_UNIFIED_RING) && (revision & VCN_BLOCK_QUEUE_DISABLE_MASK)) {
+ ret = true;
+ }
+
+ return ret;
+}
+
int amdgpu_vcn_suspend(struct amdgpu_device *adev)
{
unsigned size;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
index bc76cab67697..d74c62b49795 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
@@ -280,6 +280,16 @@ struct amdgpu_vcn_decode_buffer {
uint32_t pad[30];
};
+#define VCN_BLOCK_ENCODE_DISABLE_MASK 0x80
+#define VCN_BLOCK_DECODE_DISABLE_MASK 0x40
+#define VCN_BLOCK_QUEUE_DISABLE_MASK 0xC0
+
+enum vcn_ring_type {
+ VCN_ENCODE_RING,
+ VCN_DECODE_RING,
+ VCN_UNIFIED_RING,
+};
+
int amdgpu_vcn_sw_init(struct amdgpu_device *adev);
int amdgpu_vcn_sw_fini(struct amdgpu_device *adev);
int amdgpu_vcn_suspend(struct amdgpu_device *adev);
@@ -287,6 +297,9 @@ int amdgpu_vcn_resume(struct amdgpu_device *adev);
void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring);
void amdgpu_vcn_ring_end_use(struct amdgpu_ring *ring);
+bool amdgpu_vcn_is_disabled_vcn(struct amdgpu_device *adev,
+ enum vcn_ring_type type, uint32_t vcn_instance);
+
int amdgpu_vcn_dec_ring_test_ring(struct amdgpu_ring *ring);
int amdgpu_vcn_dec_ring_test_ib(struct amdgpu_ring *ring, long timeout);
int amdgpu_vcn_dec_sw_ring_test_ring(struct amdgpu_ring *ring);
diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
index ce3c794c176f..a79ae86bc752 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
@@ -85,9 +85,12 @@ static void vcn_v3_0_enc_ring_set_wptr(struct amdgpu_ring *ring);
static int vcn_v3_0_early_init(void *handle)
{
struct amdgpu_device *adev = (struct amdgpu_device *)handle;
+ int i;
if (amdgpu_sriov_vf(adev)) {
- adev->vcn.num_vcn_inst = VCN_INSTANCES_SIENNA_CICHLID;
+ for (i = 0; i < VCN_INSTANCES_SIENNA_CICHLID; i++)
+ if (amdgpu_vcn_is_disabled_vcn(adev, VCN_DECODE_RING, i))
+ adev->vcn.num_vcn_inst++;
adev->vcn.harvest_config = 0;
adev->vcn.num_enc_rings = 1;
@@ -99,7 +102,6 @@ static int vcn_v3_0_early_init(void *handle)
} else {
if (adev->asic_type == CHIP_SIENNA_CICHLID) {
u32 harvest;
- int i;
adev->vcn.num_vcn_inst = VCN_INSTANCES_SIENNA_CICHLID;
for (i = 0; i < adev->vcn.num_vcn_inst; i++) {
@@ -154,7 +156,7 @@ static int vcn_v3_0_sw_init(void *handle)
adev->firmware.fw_size +=
ALIGN(le32_to_cpu(hdr->ucode_size_bytes), PAGE_SIZE);
- if (adev->vcn.num_vcn_inst == VCN_INSTANCES_SIENNA_CICHLID) {
+ if (adev->asic_type == CHIP_SIENNA_CICHLID) {
adev->firmware.ucode[AMDGPU_UCODE_ID_VCN1].ucode_id = AMDGPU_UCODE_ID_VCN1;
adev->firmware.ucode[AMDGPU_UCODE_ID_VCN1].fw = adev->vcn.fw;
adev->firmware.fw_size +=
@@ -324,19 +326,17 @@ static int vcn_v3_0_hw_init(void *handle)
continue;
ring = &adev->vcn.inst[i].ring_dec;
- if (ring->sched.ready) {
- ring->wptr = 0;
- ring->wptr_old = 0;
- vcn_v3_0_dec_ring_set_wptr(ring);
- }
+ ring->wptr = 0;
+ ring->wptr_old = 0;
+ vcn_v3_0_dec_ring_set_wptr(ring);
+ ring->sched.ready = true;
for (j = 0; j < adev->vcn.num_enc_rings; ++j) {
ring = &adev->vcn.inst[i].ring_enc[j];
- if (ring->sched.ready) {
- ring->wptr = 0;
- ring->wptr_old = 0;
- vcn_v3_0_enc_ring_set_wptr(ring);
- }
+ ring->wptr = 0;
+ ring->wptr_old = 0;
+ vcn_v3_0_enc_ring_set_wptr(ring);
+ ring->sched.ready = true;
}
}
} else {
@@ -1303,8 +1303,6 @@ static int vcn_v3_0_start_sriov(struct amdgpu_device *adev)
uint32_t table_size;
uint32_t size, size_dw;
- bool is_vcn_ready;
-
struct mmsch_v3_0_cmd_direct_write
direct_wt = { {0} };
struct mmsch_v3_0_cmd_direct_read_modify_write
@@ -1496,30 +1494,6 @@ static int vcn_v3_0_start_sriov(struct amdgpu_device *adev)
}
}
- /* 6, check each VCN's init_status
- * if it remains as 0, then this VCN is not assigned to current VF
- * do not start ring for this VCN
- */
- size = sizeof(struct mmsch_v3_0_init_header);
- table_loc = (uint32_t *)table->cpu_addr;
- memcpy(&header, (void *)table_loc, size);
-
- for (i = 0; i < adev->vcn.num_vcn_inst; i++) {
- if (adev->vcn.harvest_config & (1 << i))
- continue;
-
- is_vcn_ready = (header.inst[i].init_status == 1);
- if (!is_vcn_ready)
- DRM_INFO("VCN(%d) engine is disabled by hypervisor\n", i);
-
- ring = &adev->vcn.inst[i].ring_dec;
- ring->sched.ready = is_vcn_ready;
- for (j = 0; j < adev->vcn.num_enc_rings; ++j) {
- ring = &adev->vcn.inst[i].ring_enc[j];
- ring->sched.ready = is_vcn_ready;
- }
- }
-
return 0;
}
--
2.17.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] drm/amd/amdgpu: Use IP discovery data to determine VCN enablement instead of MMSCH
2021-06-15 7:46 [PATCH v2] drm/amd/amdgpu: Use IP discovery data to determine VCN enablement instead of MMSCH Peng Ju Zhou
@ 2021-06-15 19:28 ` Alex Deucher
2021-06-16 5:40 ` Zhou, Peng Ju
0 siblings, 1 reply; 3+ messages in thread
From: Alex Deucher @ 2021-06-15 19:28 UTC (permalink / raw)
To: Peng Ju Zhou; +Cc: Bokun Zhang, amd-gfx list
On Tue, Jun 15, 2021 at 3:46 AM Peng Ju Zhou <PengJu.Zhou@amd.com> wrote:
>
> From: Bokun Zhang <Bokun.Zhang@amd.com>
>
> In the past, we use MMSCH to determine whether a VCN is enabled or not.
> This is not reliable since after a FLR, MMSCH may report junk data.
>
> It is better to use IP discovery data.
>
> Signed-off-by: Bokun Zhang <Bokun.Zhang@amd.com>
> Signed-off-by: Peng Ju Zhou <PengJu.Zhou@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 8 +++
> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h | 3 ++
> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 23 ++++++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 13 +++++
> drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c | 52 +++++--------------
> 5 files changed, 60 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> index f949ed8bfd9e..e02405a24fe3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -373,6 +373,14 @@ int amdgpu_discovery_get_ip_version(struct amdgpu_device *adev, int hw_id, int n
> return -EINVAL;
> }
>
> +
> +int amdgpu_discovery_get_vcn_version(struct amdgpu_device *adev, int vcn_instance,
> + int *major, int *minor, int *revision)
> +{
> + return amdgpu_discovery_get_ip_version(adev, VCN_HWID,
> + vcn_instance, major, minor, revision);
> +}
> +
I think you can drop this wrapper and just call
amdgpu_discovery_get_ip_version() directly from
amdgpu_vcn_is_disabled_vcn().
> void amdgpu_discovery_harvest_ip(struct amdgpu_device *adev)
> {
> struct binary_header *bhdr;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h
> index 02e340cd3a38..48e6b88cfdfe 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h
> @@ -32,6 +32,9 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev);
> void amdgpu_discovery_harvest_ip(struct amdgpu_device *adev);
> int amdgpu_discovery_get_ip_version(struct amdgpu_device *adev, int hw_id, int number_instance,
> int *major, int *minor, int *revision);
> +
> +int amdgpu_discovery_get_vcn_version(struct amdgpu_device *adev, int vcn_instance,
> + int *major, int *minor, int *revision);
> int amdgpu_discovery_get_gfx_info(struct amdgpu_device *adev);
>
> #endif /* __AMDGPU_DISCOVERY__ */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> index 9492b505e69b..84b025405578 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> @@ -287,6 +287,29 @@ int amdgpu_vcn_sw_fini(struct amdgpu_device *adev)
> return 0;
> }
>
> +bool amdgpu_vcn_is_disabled_vcn(struct amdgpu_device *adev, enum vcn_ring_type type, uint32_t vcn_instance)
> +{
> + bool ret = false;
> +
> + int major;
> + int minor;
> + int revision;
> +
> + /* if cannot find IP data, then this VCN does not exist */
> + if (amdgpu_discovery_get_vcn_version(adev, vcn_instance, &major, &minor, &revision) != 0)
Just call amdgpu_discovery_get_ip_version() directly here.
> + return true;
> +
> + if ((type == VCN_ENCODE_RING) && (revision & VCN_BLOCK_ENCODE_DISABLE_MASK)) {
> + ret = true;
> + } else if ((type == VCN_DECODE_RING) && (revision & VCN_BLOCK_DECODE_DISABLE_MASK)) {
> + ret = true;
> + } else if ((type == VCN_UNIFIED_RING) && (revision & VCN_BLOCK_QUEUE_DISABLE_MASK)) {
> + ret = true;
> + }
> +
> + return ret;
> +}
> +
> int amdgpu_vcn_suspend(struct amdgpu_device *adev)
> {
> unsigned size;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> index bc76cab67697..d74c62b49795 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> @@ -280,6 +280,16 @@ struct amdgpu_vcn_decode_buffer {
> uint32_t pad[30];
> };
>
> +#define VCN_BLOCK_ENCODE_DISABLE_MASK 0x80
> +#define VCN_BLOCK_DECODE_DISABLE_MASK 0x40
> +#define VCN_BLOCK_QUEUE_DISABLE_MASK 0xC0
> +
> +enum vcn_ring_type {
> + VCN_ENCODE_RING,
> + VCN_DECODE_RING,
> + VCN_UNIFIED_RING,
> +};
> +
> int amdgpu_vcn_sw_init(struct amdgpu_device *adev);
> int amdgpu_vcn_sw_fini(struct amdgpu_device *adev);
> int amdgpu_vcn_suspend(struct amdgpu_device *adev);
> @@ -287,6 +297,9 @@ int amdgpu_vcn_resume(struct amdgpu_device *adev);
> void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring);
> void amdgpu_vcn_ring_end_use(struct amdgpu_ring *ring);
>
> +bool amdgpu_vcn_is_disabled_vcn(struct amdgpu_device *adev,
> + enum vcn_ring_type type, uint32_t vcn_instance);
> +
> int amdgpu_vcn_dec_ring_test_ring(struct amdgpu_ring *ring);
> int amdgpu_vcn_dec_ring_test_ib(struct amdgpu_ring *ring, long timeout);
> int amdgpu_vcn_dec_sw_ring_test_ring(struct amdgpu_ring *ring);
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
> index ce3c794c176f..a79ae86bc752 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
> @@ -85,9 +85,12 @@ static void vcn_v3_0_enc_ring_set_wptr(struct amdgpu_ring *ring);
> static int vcn_v3_0_early_init(void *handle)
> {
> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> + int i;
>
> if (amdgpu_sriov_vf(adev)) {
> - adev->vcn.num_vcn_inst = VCN_INSTANCES_SIENNA_CICHLID;
> + for (i = 0; i < VCN_INSTANCES_SIENNA_CICHLID; i++)
> + if (amdgpu_vcn_is_disabled_vcn(adev, VCN_DECODE_RING, i))
> + adev->vcn.num_vcn_inst++;
> adev->vcn.harvest_config = 0;
> adev->vcn.num_enc_rings = 1;
>
> @@ -99,7 +102,6 @@ static int vcn_v3_0_early_init(void *handle)
> } else {
> if (adev->asic_type == CHIP_SIENNA_CICHLID) {
> u32 harvest;
> - int i;
>
> adev->vcn.num_vcn_inst = VCN_INSTANCES_SIENNA_CICHLID;
> for (i = 0; i < adev->vcn.num_vcn_inst; i++) {
> @@ -154,7 +156,7 @@ static int vcn_v3_0_sw_init(void *handle)
> adev->firmware.fw_size +=
> ALIGN(le32_to_cpu(hdr->ucode_size_bytes), PAGE_SIZE);
>
> - if (adev->vcn.num_vcn_inst == VCN_INSTANCES_SIENNA_CICHLID) {
> + if (adev->asic_type == CHIP_SIENNA_CICHLID) {
Is this change safe? Will this break anything on harvested boards?
Other than these comments the patch looks good.
Alex
> adev->firmware.ucode[AMDGPU_UCODE_ID_VCN1].ucode_id = AMDGPU_UCODE_ID_VCN1;
> adev->firmware.ucode[AMDGPU_UCODE_ID_VCN1].fw = adev->vcn.fw;
> adev->firmware.fw_size +=
> @@ -324,19 +326,17 @@ static int vcn_v3_0_hw_init(void *handle)
> continue;
>
> ring = &adev->vcn.inst[i].ring_dec;
> - if (ring->sched.ready) {
> - ring->wptr = 0;
> - ring->wptr_old = 0;
> - vcn_v3_0_dec_ring_set_wptr(ring);
> - }
> + ring->wptr = 0;
> + ring->wptr_old = 0;
> + vcn_v3_0_dec_ring_set_wptr(ring);
> + ring->sched.ready = true;
>
> for (j = 0; j < adev->vcn.num_enc_rings; ++j) {
> ring = &adev->vcn.inst[i].ring_enc[j];
> - if (ring->sched.ready) {
> - ring->wptr = 0;
> - ring->wptr_old = 0;
> - vcn_v3_0_enc_ring_set_wptr(ring);
> - }
> + ring->wptr = 0;
> + ring->wptr_old = 0;
> + vcn_v3_0_enc_ring_set_wptr(ring);
> + ring->sched.ready = true;
> }
> }
> } else {
> @@ -1303,8 +1303,6 @@ static int vcn_v3_0_start_sriov(struct amdgpu_device *adev)
> uint32_t table_size;
> uint32_t size, size_dw;
>
> - bool is_vcn_ready;
> -
> struct mmsch_v3_0_cmd_direct_write
> direct_wt = { {0} };
> struct mmsch_v3_0_cmd_direct_read_modify_write
> @@ -1496,30 +1494,6 @@ static int vcn_v3_0_start_sriov(struct amdgpu_device *adev)
> }
> }
>
> - /* 6, check each VCN's init_status
> - * if it remains as 0, then this VCN is not assigned to current VF
> - * do not start ring for this VCN
> - */
> - size = sizeof(struct mmsch_v3_0_init_header);
> - table_loc = (uint32_t *)table->cpu_addr;
> - memcpy(&header, (void *)table_loc, size);
> -
> - for (i = 0; i < adev->vcn.num_vcn_inst; i++) {
> - if (adev->vcn.harvest_config & (1 << i))
> - continue;
> -
> - is_vcn_ready = (header.inst[i].init_status == 1);
> - if (!is_vcn_ready)
> - DRM_INFO("VCN(%d) engine is disabled by hypervisor\n", i);
> -
> - ring = &adev->vcn.inst[i].ring_dec;
> - ring->sched.ready = is_vcn_ready;
> - for (j = 0; j < adev->vcn.num_enc_rings; ++j) {
> - ring = &adev->vcn.inst[i].ring_enc[j];
> - ring->sched.ready = is_vcn_ready;
> - }
> - }
> -
> return 0;
> }
>
> --
> 2.17.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH v2] drm/amd/amdgpu: Use IP discovery data to determine VCN enablement instead of MMSCH
2021-06-15 19:28 ` Alex Deucher
@ 2021-06-16 5:40 ` Zhou, Peng Ju
0 siblings, 0 replies; 3+ messages in thread
From: Zhou, Peng Ju @ 2021-06-16 5:40 UTC (permalink / raw)
To: Alex Deucher; +Cc: Zhang, Bokun, amd-gfx list
[AMD Official Use Only]
Hi Alex
Update inline
> -----Original Message-----
> From: Alex Deucher <alexdeucher@gmail.com>
> Sent: Wednesday, June 16, 2021 3:29 AM
> To: Zhou, Peng Ju <PengJu.Zhou@amd.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>; Zhang, Bokun
> <Bokun.Zhang@amd.com>
> Subject: Re: [PATCH v2] drm/amd/amdgpu: Use IP discovery data to determine
> VCN enablement instead of MMSCH
>
> On Tue, Jun 15, 2021 at 3:46 AM Peng Ju Zhou <PengJu.Zhou@amd.com>
> wrote:
> >
> > From: Bokun Zhang <Bokun.Zhang@amd.com>
> >
> > In the past, we use MMSCH to determine whether a VCN is enabled or not.
> > This is not reliable since after a FLR, MMSCH may report junk data.
> >
> > It is better to use IP discovery data.
> >
> > Signed-off-by: Bokun Zhang <Bokun.Zhang@amd.com>
> > Signed-off-by: Peng Ju Zhou <PengJu.Zhou@amd.com>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 8 +++
> > drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h | 3 ++
> > drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 23 ++++++++
> > drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 13 +++++
> > drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c | 52 +++++--------------
> > 5 files changed, 60 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> > index f949ed8bfd9e..e02405a24fe3 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> > @@ -373,6 +373,14 @@ int amdgpu_discovery_get_ip_version(struct
> amdgpu_device *adev, int hw_id, int n
> > return -EINVAL;
> > }
> >
> > +
> > +int amdgpu_discovery_get_vcn_version(struct amdgpu_device *adev, int
> vcn_instance,
> > + int *major, int *minor, int
> > +*revision) {
> > + return amdgpu_discovery_get_ip_version(adev, VCN_HWID,
> > + vcn_instance, major,
> > +minor, revision); }
> > +
>
> I think you can drop this wrapper and just call
> amdgpu_discovery_get_ip_version() directly from
> amdgpu_vcn_is_disabled_vcn().
>
> > void amdgpu_discovery_harvest_ip(struct amdgpu_device *adev) {
> > struct binary_header *bhdr;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h
> > index 02e340cd3a38..48e6b88cfdfe 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.h
> > @@ -32,6 +32,9 @@ int amdgpu_discovery_reg_base_init(struct
> > amdgpu_device *adev); void amdgpu_discovery_harvest_ip(struct
> > amdgpu_device *adev); int amdgpu_discovery_get_ip_version(struct
> amdgpu_device *adev, int hw_id, int number_instance,
> > int *major, int *minor, int
> > *revision);
> > +
> > +int amdgpu_discovery_get_vcn_version(struct amdgpu_device *adev, int
> vcn_instance,
> > + int *major, int *minor, int
> > +*revision);
> > int amdgpu_discovery_get_gfx_info(struct amdgpu_device *adev);
> >
> > #endif /* __AMDGPU_DISCOVERY__ */
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> > index 9492b505e69b..84b025405578 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> > @@ -287,6 +287,29 @@ int amdgpu_vcn_sw_fini(struct amdgpu_device
> *adev)
> > return 0;
> > }
> >
> > +bool amdgpu_vcn_is_disabled_vcn(struct amdgpu_device *adev, enum
> > +vcn_ring_type type, uint32_t vcn_instance) {
> > + bool ret = false;
> > +
> > + int major;
> > + int minor;
> > + int revision;
> > +
> > + /* if cannot find IP data, then this VCN does not exist */
> > + if (amdgpu_discovery_get_vcn_version(adev, vcn_instance,
> > + &major, &minor, &revision) != 0)
>
> Just call amdgpu_discovery_get_ip_version() directly here.
If call amdgpu_discovery_get_ip_version() directly, we have to include header soc15_hw_ip.h
I have 2 concern:
1. amdgpu_vcn.c is ASIC independent file, but soc15_hw_ip.h is ASIC related file, Can we add the header to amdgpu_vcn.c?
2. From design perspective, is it better to use a wrapper than including a header file which may increase the size of hex file?
> > + return true;
> > +
> > + if ((type == VCN_ENCODE_RING) && (revision &
> VCN_BLOCK_ENCODE_DISABLE_MASK)) {
> > + ret = true;
> > + } else if ((type == VCN_DECODE_RING) && (revision &
> VCN_BLOCK_DECODE_DISABLE_MASK)) {
> > + ret = true;
> > + } else if ((type == VCN_UNIFIED_RING) && (revision &
> VCN_BLOCK_QUEUE_DISABLE_MASK)) {
> > + ret = true;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > int amdgpu_vcn_suspend(struct amdgpu_device *adev) {
> > unsigned size;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> > index bc76cab67697..d74c62b49795 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> > @@ -280,6 +280,16 @@ struct amdgpu_vcn_decode_buffer {
> > uint32_t pad[30];
> > };
> >
> > +#define VCN_BLOCK_ENCODE_DISABLE_MASK 0x80 #define
> > +VCN_BLOCK_DECODE_DISABLE_MASK 0x40 #define
> > +VCN_BLOCK_QUEUE_DISABLE_MASK 0xC0
> > +
> > +enum vcn_ring_type {
> > + VCN_ENCODE_RING,
> > + VCN_DECODE_RING,
> > + VCN_UNIFIED_RING,
> > +};
> > +
> > int amdgpu_vcn_sw_init(struct amdgpu_device *adev); int
> > amdgpu_vcn_sw_fini(struct amdgpu_device *adev); int
> > amdgpu_vcn_suspend(struct amdgpu_device *adev); @@ -287,6 +297,9 @@
> > int amdgpu_vcn_resume(struct amdgpu_device *adev); void
> > amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring); void
> > amdgpu_vcn_ring_end_use(struct amdgpu_ring *ring);
> >
> > +bool amdgpu_vcn_is_disabled_vcn(struct amdgpu_device *adev,
> > + enum vcn_ring_type type, uint32_t
> > +vcn_instance);
> > +
> > int amdgpu_vcn_dec_ring_test_ring(struct amdgpu_ring *ring); int
> > amdgpu_vcn_dec_ring_test_ib(struct amdgpu_ring *ring, long timeout);
> > int amdgpu_vcn_dec_sw_ring_test_ring(struct amdgpu_ring *ring); diff
> > --git a/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
> > b/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
> > index ce3c794c176f..a79ae86bc752 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
> > @@ -85,9 +85,12 @@ static void vcn_v3_0_enc_ring_set_wptr(struct
> > amdgpu_ring *ring); static int vcn_v3_0_early_init(void *handle) {
> > struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > + int i;
> >
> > if (amdgpu_sriov_vf(adev)) {
> > - adev->vcn.num_vcn_inst = VCN_INSTANCES_SIENNA_CICHLID;
> > + for (i = 0; i < VCN_INSTANCES_SIENNA_CICHLID; i++)
> > + if (amdgpu_vcn_is_disabled_vcn(adev, VCN_DECODE_RING, i))
> > + adev->vcn.num_vcn_inst++;
> > adev->vcn.harvest_config = 0;
> > adev->vcn.num_enc_rings = 1;
> >
> > @@ -99,7 +102,6 @@ static int vcn_v3_0_early_init(void *handle)
> > } else {
> > if (adev->asic_type == CHIP_SIENNA_CICHLID) {
> > u32 harvest;
> > - int i;
> >
> > adev->vcn.num_vcn_inst = VCN_INSTANCES_SIENNA_CICHLID;
> > for (i = 0; i < adev->vcn.num_vcn_inst; i++) {
> > @@ -154,7 +156,7 @@ static int vcn_v3_0_sw_init(void *handle)
> > adev->firmware.fw_size +=
> > ALIGN(le32_to_cpu(hdr->ucode_size_bytes),
> > PAGE_SIZE);
> >
> > - if (adev->vcn.num_vcn_inst == VCN_INSTANCES_SIENNA_CICHLID) {
> > + if (adev->asic_type == CHIP_SIENNA_CICHLID) {
>
> Is this change safe? Will this break anything on harvested boards?
> Other than these comments the patch looks good.
Will change it.
> Alex
>
> > adev->firmware.ucode[AMDGPU_UCODE_ID_VCN1].ucode_id =
> AMDGPU_UCODE_ID_VCN1;
> > adev->firmware.ucode[AMDGPU_UCODE_ID_VCN1].fw = adev-
> >vcn.fw;
> > adev->firmware.fw_size += @@ -324,19 +326,17
> > @@ static int vcn_v3_0_hw_init(void *handle)
> > continue;
> >
> > ring = &adev->vcn.inst[i].ring_dec;
> > - if (ring->sched.ready) {
> > - ring->wptr = 0;
> > - ring->wptr_old = 0;
> > - vcn_v3_0_dec_ring_set_wptr(ring);
> > - }
> > + ring->wptr = 0;
> > + ring->wptr_old = 0;
> > + vcn_v3_0_dec_ring_set_wptr(ring);
> > + ring->sched.ready = true;
> >
> > for (j = 0; j < adev->vcn.num_enc_rings; ++j) {
> > ring = &adev->vcn.inst[i].ring_enc[j];
> > - if (ring->sched.ready) {
> > - ring->wptr = 0;
> > - ring->wptr_old = 0;
> > - vcn_v3_0_enc_ring_set_wptr(ring);
> > - }
> > + ring->wptr = 0;
> > + ring->wptr_old = 0;
> > + vcn_v3_0_enc_ring_set_wptr(ring);
> > + ring->sched.ready = true;
> > }
> > }
> > } else {
> > @@ -1303,8 +1303,6 @@ static int vcn_v3_0_start_sriov(struct
> amdgpu_device *adev)
> > uint32_t table_size;
> > uint32_t size, size_dw;
> >
> > - bool is_vcn_ready;
> > -
> > struct mmsch_v3_0_cmd_direct_write
> > direct_wt = { {0} };
> > struct mmsch_v3_0_cmd_direct_read_modify_write
> > @@ -1496,30 +1494,6 @@ static int vcn_v3_0_start_sriov(struct
> amdgpu_device *adev)
> > }
> > }
> >
> > - /* 6, check each VCN's init_status
> > - * if it remains as 0, then this VCN is not assigned to current VF
> > - * do not start ring for this VCN
> > - */
> > - size = sizeof(struct mmsch_v3_0_init_header);
> > - table_loc = (uint32_t *)table->cpu_addr;
> > - memcpy(&header, (void *)table_loc, size);
> > -
> > - for (i = 0; i < adev->vcn.num_vcn_inst; i++) {
> > - if (adev->vcn.harvest_config & (1 << i))
> > - continue;
> > -
> > - is_vcn_ready = (header.inst[i].init_status == 1);
> > - if (!is_vcn_ready)
> > - DRM_INFO("VCN(%d) engine is disabled by hypervisor\n", i);
> > -
> > - ring = &adev->vcn.inst[i].ring_dec;
> > - ring->sched.ready = is_vcn_ready;
> > - for (j = 0; j < adev->vcn.num_enc_rings; ++j) {
> > - ring = &adev->vcn.inst[i].ring_enc[j];
> > - ring->sched.ready = is_vcn_ready;
> > - }
> > - }
> > -
> > return 0;
> > }
> >
> > --
> > 2.17.1
> >
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@lists.freedesktop.org
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> > s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-
> gfx&data=04%7C01%7CPe
> >
> ngJu.Zhou%40amd.com%7C03db0d21b4954eb34a0308d93033d2b5%7C3dd89
> 61fe4884
> >
> e608e11a82d994e183d%7C0%7C0%7C637593821403612572%7CUnknown%7C
> TWFpbGZsb
> >
> 3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%
> 3D%
> >
> 7C1000&sdata=CTur5LY8Nc8OlpJ5a1G4TlJe96CXEZ2%2BRpqaLc%2F%2FiP
> o%3D&
> > amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-06-16 5:40 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-15 7:46 [PATCH v2] drm/amd/amdgpu: Use IP discovery data to determine VCN enablement instead of MMSCH Peng Ju Zhou
2021-06-15 19:28 ` Alex Deucher
2021-06-16 5:40 ` Zhou, Peng Ju
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.