[PATCH 2/3] drm/amdgpu/vcn: Deduplicate indirect SRAM checking code
Limonciello, Mario
Mario.Limonciello at amd.com
Mon Jan 16 21:57:16 UTC 2023
[Public]
> -----Original Message-----
> From: Alex Deucher <alexdeucher at gmail.com>
> Sent: Monday, January 16, 2023 15:54
> To: Limonciello, Mario <Mario.Limonciello at amd.com>
> Cc: Guilherme G. Piccoli <gpiccoli at igalia.com>; 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>; 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:49 PM Limonciello, Mario
> <Mario.Limonciello at amd.com> wrote:
> >
> > [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.
>
>
> Maybe it would be better to just drop the case statement and just
> always set this condition. I don't think there is a case where we
> don't set it?
Yeah, it wasn't immediately apparent at the time of the rework - but you're right.
Goodbye ~75 LOC.
>
> Alex
>
> >
> > > > 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