RE: [PATCH 2/3] drm/amdgpu/vcn: Deduplicate indirect SRAM checking code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[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.

> >         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
> >




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux