[PATCH 2/3] drm/amdgpu/vcn: Deduplicate indirect SRAM checking code
Limonciello, Mario
Mario.Limonciello at amd.com
Mon Jan 16 21:49:04 UTC 2023
[Public]
> -----Original Message-----
> From: Alex Deucher <alexdeucher at gmail.com>
> Sent: Monday, January 16, 2023 15:42
> To: Guilherme G. Piccoli <gpiccoli at igalia.com>
> Cc: amd-gfx at lists.freedesktop.org; Jiang, Sonny <Sonny.Jiang at amd.com>;
> kernel at gpiccoli.net; Pan, Xinhui <Xinhui.Pan at amd.com>; dri-
> devel at lists.freedesktop.org; Lazar, Lijo <Lijo.Lazar at amd.com>; Limonciello,
> Mario <Mario.Limonciello at amd.com>; kernel-dev at igalia.com; Deucher,
> Alexander <Alexander.Deucher at amd.com>; Zhu, James
> <James.Zhu at amd.com>; Liu, Leo <Leo.Liu at amd.com>; Koenig, Christian
> <Christian.Koenig at amd.com>
> Subject: Re: [PATCH 2/3] drm/amdgpu/vcn: Deduplicate indirect SRAM
> checking code
>
> On Mon, Jan 16, 2023 at 4:20 PM Guilherme G. Piccoli
> <gpiccoli at igalia.com> wrote:
> >
> > Currently both conditionals checked to enable indirect SRAM are
> > repeated for all HW/IP models. Let's just simplify it a bit so
> > code is more compact and readable.
> >
> > While at it, add the legacy names as a comment per case block, to
> > help whoever is reading the code and doesn't have much experience
> > with the name/number mapping.
> >
> > Cc: James Zhu <James.Zhu at amd.com>
> > Cc: Lazar Lijo <Lijo.Lazar at amd.com>
> > Cc: Leo Liu <leo.liu at amd.com>
> > Cc: Mario Limonciello <mario.limonciello at amd.com>
> > Cc: Sonny Jiang <sonny.jiang at amd.com>
> > Signed-off-by: Guilherme G. Piccoli <gpiccoli at igalia.com>
> > ---
> >
> >
> > Hi folks, first of all thanks in advance for reviews/comments!
> >
> > This work is based on agd5f/amd-staging-drm-next branch - there is this
> > patch from Mario that refactored the amdgpu_vcn.c, and since it dropped
> > the legacy names from the switch cases, I've decided to also include them
> > here as comments.
> >
> > I'm not sure if that's a good idea, feels good for somebody not so
> > used to the code read the codenames instead of purely numbers, but
> > if you wanna move away from the legacy names for good, lemme know
> > and I can rework without these comments.
> >
> > Cheers,
> >
> >
> > Guilherme
> >
> >
> > drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 84 +++++--------------------
> > 1 file changed, 16 insertions(+), 68 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> > index 1b1a3c9e1863..1f880e162d9d 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> > @@ -111,78 +111,26 @@ int amdgpu_vcn_sw_init(struct amdgpu_device
> *adev)
> > atomic_set(&adev->vcn.inst[i].dpg_enc_submission_cnt, 0);
> >
> > switch (adev->ip_versions[UVD_HWIP][0]) {
> > - case IP_VERSION(1, 0, 0):
> > - case IP_VERSION(1, 0, 1):
> > - case IP_VERSION(2, 5, 0):
> > - if ((adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) &&
> > - (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG))
> > - adev->vcn.indirect_sram = true;
> > - break;
> > - case IP_VERSION(2, 2, 0):
> > - if ((adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) &&
> > - (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG))
> > - adev->vcn.indirect_sram = true;
> > - break;
> > - case IP_VERSION(2, 6, 0):
> > - if ((adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) &&
> > - (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG))
> > - adev->vcn.indirect_sram = true;
> > - break;
> > - case IP_VERSION(2, 0, 0):
> > - if ((adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) &&
> > - (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG))
> > - adev->vcn.indirect_sram = true;
> > - break;
> > - case IP_VERSION(2, 0, 2):
> > - if ((adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) &&
> > - (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG))
> > - adev->vcn.indirect_sram = true;
> > - break;
> > - case IP_VERSION(3, 0, 0):
> > - case IP_VERSION(3, 0, 64):
> > - case IP_VERSION(3, 0, 192):
> > - if ((adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) &&
> > - (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG))
> > - adev->vcn.indirect_sram = true;
> > - break;
> > - case IP_VERSION(3, 0, 2):
> > - if ((adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) &&
> > - (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG))
> > - adev->vcn.indirect_sram = true;
> > - break;
> > - case IP_VERSION(3, 0, 16):
> > - if ((adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) &&
> > - (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG))
> > - adev->vcn.indirect_sram = true;
> > - break;
> > - case IP_VERSION(3, 0, 33):
> > - if ((adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) &&
> > - (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG))
> > - adev->vcn.indirect_sram = true;
> > - break;
> > - case IP_VERSION(3, 1, 1):
> > - if ((adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) &&
> > - (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG))
> > - adev->vcn.indirect_sram = true;
> > - break;
> > + case IP_VERSION(1, 0, 0): /* Raven (1/2) / Picasso */
> > + case IP_VERSION(1, 0, 1): /* Raven (1/2) / Picasso */
> > + case IP_VERSION(2, 0, 0): /* Navi10 */
> > + case IP_VERSION(2, 0, 2): /* Navi12 / Navi14 */
> > + case IP_VERSION(2, 2, 0): /* Renoir / Green Sardine */
> > + case IP_VERSION(2, 5, 0): /* Arcturus */
> > + case IP_VERSION(2, 6, 0): /* Aldebaran */
> > + case IP_VERSION(3, 0, 0): /* Sienna Cichlid / Navy Flounder */
> > + case IP_VERSION(3, 0, 2): /* Vangogh */
> > + case IP_VERSION(3, 0, 64): /* Sienna Cichlid / Navy Flounder */
> > + case IP_VERSION(3, 0, 16): /* Dimgray Cavefish */
> > + case IP_VERSION(3, 0, 33): /* Beige Goby */
> > + case IP_VERSION(3, 0, 192): /* Sienna Cichlid / Navy Flounder */
> > + case IP_VERSION(3, 1, 1): /* Yellow Carp */
The problem with adding a comment about which platform it's in is that
this will probably go stale. Some IP blocks are re-used in multiple ASICs.
VCN 3, 1, 1 is a great example. This is used in both Yellow Carp (Rembrandt) as well
as Mendocino. I would think a better approach would be to have a single point
of documentation somewhere in the source tree that we can update after new
ASICs launch that could act as a decoder ring.
It's effort to remember to update it, but it's at least a single point of failure.
> > case IP_VERSION(3, 1, 2):
> > - if ((adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) &&
> > - (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG))
> > - adev->vcn.indirect_sram = true;
> > - break;
> > - case IP_VERSION(4, 0, 0):
> > - if ((adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) &&
> > - (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG))
> > - adev->vcn.indirect_sram = true;
> > - break;
> > + case IP_VERSION(4, 0, 0): /* Vega10 */
>
> This comment is incorrect. Vega10 does not have VCN (it has UVD and VCE).
>
> Alex
>
>
> Alex
>
> > case IP_VERSION(4, 0, 2):
> > - if ((adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) &&
> > - (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG))
> > - adev->vcn.indirect_sram = true;
> > - break;
> > case IP_VERSION(4, 0, 4):
> > if ((adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) &&
> > - (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG))
> > + (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG))
> > adev->vcn.indirect_sram = true;
> > break;
> > default:
> > --
> > 2.39.0
> >
More information about the dri-devel
mailing list