On Mon, Jan 16, 2023 at 4:49 PM Limonciello, Mario
<Mario.Limonciello@xxxxxxx> wrote:
>
> [Public]
>
>
>
> > -----Original Message-----
> > From: Alex Deucher <alexdeucher@xxxxxxxxx>
> > Sent: Monday, January 16, 2023 15:42
> > To: Guilherme G. Piccoli <gpiccoli@xxxxxxxxxx>
> > Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Jiang, Sonny <Sonny.Jiang@xxxxxxx>;
> > kernel@xxxxxxxxxxxx; Pan, Xinhui <Xinhui.Pan@xxxxxxx>; dri-
> > devel@xxxxxxxxxxxxxxxxxxxxx; Lazar, Lijo <Lijo.Lazar@xxxxxxx>; Limonciello,
> > Mario <Mario.Limonciello@xxxxxxx>; kernel-dev@xxxxxxxxxx; Deucher,
> > Alexander <Alexander.Deucher@xxxxxxx>; Zhu, James
> > <James.Zhu@xxxxxxx>; Liu, Leo <Leo.Liu@xxxxxxx>; Koenig, Christian
> > <Christian.Koenig@xxxxxxx>
> > 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@xxxxxxxxxx> 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@xxxxxxx>
> > > Cc: Lazar Lijo <Lijo.Lazar@xxxxxxx>
> > > Cc: Leo Liu <leo.liu@xxxxxxx>
> > > Cc: Mario Limonciello <mario.limonciello@xxxxxxx>
> > > Cc: Sonny Jiang <sonny.jiang@xxxxxxx>
> > > Signed-off-by: Guilherme G. Piccoli <gpiccoli@xxxxxxxxxx>
> > > ---
> > >
> > >
> > > 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?
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
> > >
