From: "Zhou, Peng Ju" <PengJu.Zhou@amd.com>
To: Alex Deucher <alexdeucher@gmail.com>
Cc: "Zhang, Bokun" <Bokun.Zhang@amd.com>,
amd-gfx list <amd-gfx@lists.freedesktop.org>
Subject: RE: [PATCH v2] drm/amd/amdgpu: Use IP discovery data to determine VCN enablement instead of MMSCH
Date: Wed, 16 Jun 2021 05:40:26 +0000 [thread overview]
Message-ID: <DM8PR12MB54781D5962DF650411375AD3F80F9@DM8PR12MB5478.namprd12.prod.outlook.com> (raw)
In-Reply-To: <CADnq5_PBj7frnYown4AY3jcdu7LHCg6sEKeQ5cHU8U1U6Wb0VQ@mail.gmail.com>
[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
prev parent reply other threads:[~2021-06-16 5:40 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DM8PR12MB54781D5962DF650411375AD3F80F9@DM8PR12MB5478.namprd12.prod.outlook.com \
--to=pengju.zhou@amd.com \
--cc=Bokun.Zhang@amd.com \
--cc=alexdeucher@gmail.com \
--cc=amd-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.