[Public]
> -----Original Message----- > From: Alex Deucher <alexdeuc...@gmail.com> > Sent: Monday, January 16, 2023 15:42 > To: Guilherme G. Piccoli <gpicc...@igalia.com> > Cc: amd-...@lists.freedesktop.org; Jiang, Sonny <sonny.ji...@amd.com>; > ker...@gpiccoli.net; Pan, Xinhui <xinhui....@amd.com>; dri- > de...@lists.freedesktop.org; Lazar, Lijo <lijo.la...@amd.com>; Limonciello, > Mario <mario.limoncie...@amd.com>; kernel-...@igalia.com; Deucher, > Alexander <alexander.deuc...@amd.com>; Zhu, James > <james....@amd.com>; Liu, Leo <leo....@amd.com>; Koenig, Christian > <christian.koe...@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 > <gpicc...@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....@amd.com> > > Cc: Lazar Lijo <lijo.la...@amd.com> > > Cc: Leo Liu <leo....@amd.com> > > Cc: Mario Limonciello <mario.limoncie...@amd.com> > > Cc: Sonny Jiang <sonny.ji...@amd.com> > > Signed-off-by: Guilherme G. Piccoli <gpicc...@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 > >