[PATCH] drm/amdgpu/vcn: fix idle work handler for VCN 2.5

[Alex Deucher]

On Tue, Mar 4, 2025 at 4:00 PM Boyuan Zhang <Boyuan.Zhang at amd.com> wrote:
>
>
> On 2025-03-04 11:22, Alex Deucher wrote:
> > VCN 2.5 uses the PG callback to enable VCN DPM which is
> > a global state.  As such, we need to make sure all instances
> > are in the same state.
> >
> > v2: switch to a ref count (Lijo)
> >
> > Fixes: 4ce4fe27205c ("drm/amdgpu/vcn: use per instance callbacks for idle work handler")
> > Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h |  4 +++
> >   drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c   | 41 +++++++++++++++++++------
> >   2 files changed, 36 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> > index 26c9c2d90f455..3bc4fe4aeb481 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> > @@ -358,6 +358,10 @@ struct amdgpu_vcn {
> >
> >       bool                    per_inst_fw;
> >       unsigned                fw_version;
> > +     /* VCN 2.5 global PG handling */
> > +     struct mutex            global_pg_lock;
> > +     unsigned int            global_pg_count;
> > +     enum amd_powergating_state global_pg_state;
> >   };
> >
> >   struct amdgpu_fw_shared_rb_ptrs_struct {
> > diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
> > index dff1a88590363..972f0842ea47b 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
> > @@ -172,6 +172,8 @@ static int vcn_v2_5_sw_init(struct amdgpu_ip_block *ip_block)
> >       uint32_t *ptr;
> >       struct amdgpu_device *adev = ip_block->adev;
> >
> > +     mutex_init(&adev->vcn.global_pg_lock);
> > +
> >       for (j = 0; j < adev->vcn.num_vcn_inst; j++) {
> >               volatile struct amdgpu_fw_shared *fw_shared;
> >
> > @@ -1853,21 +1855,42 @@ static int vcn_v2_5_set_pg_state(struct amdgpu_vcn_inst *vinst,
> >                                enum amd_powergating_state state)
> >   {
> >       struct amdgpu_device *adev = vinst->adev;
> > -     int ret;
> > +     struct amdgpu_vcn_inst *v;
> > +     int ret = 0, i;
> >
> >       if (amdgpu_sriov_vf(adev))
> >               return 0;
> >
> > -     if (state == vinst->cur_state)
> > -             return 0;
> > +     mutex_lock(&adev->vcn.global_pg_lock);
> > +     if (state == AMD_PG_STATE_GATE) {
> > +             if (adev->vcn.global_pg_count == 0)
> > +                     goto unlock;
> > +             adev->vcn.global_pg_count--;
> > +             if (adev->vcn.global_pg_count == 0 &&
> > +                 adev->vcn.global_pg_state == AMD_PG_STATE_UNGATE) {
> > +                     for (i = 0; i < adev->vcn.num_vcn_inst; ++i) {
> > +                             v = &adev->vcn.inst[i];
> > +
> > +                             ret = vcn_v2_5_stop(v);
> > +                     }
> > +                     adev->vcn.global_pg_state = AMD_PG_STATE_GATE;
> > +             }
> > +     } else {
> > +             if (adev->vcn.global_pg_count == 0 &&
> > +                 adev->vcn.global_pg_state == AMD_PG_STATE_GATE) {
> > +                     for (i = 0; i < adev->vcn.num_vcn_inst; ++i) {
> > +                             v = &adev->vcn.inst[i];
> >
> > -     if (state == AMD_PG_STATE_GATE)
> > -             ret = vcn_v2_5_stop(vinst);
> > -     else
> > -             ret = vcn_v2_5_start(vinst);
> > +                             ret = vcn_v2_5_start(v);
> > +                     }
> > +                     adev->vcn.global_pg_state = AMD_PG_STATE_UNGATE;
> > +             }
> > +             adev->vcn.global_pg_count++;
> > +     }
> >
> > -     if (!ret)
> > -             vinst->cur_state = state;
> > +unlock:
> > +     vinst->cur_state = state;
>
>
> I guess we don't need to bother this per instant (vinst->cur_state) at
> all in this case? Other than this, this patch is

I figured it would be good to keep it up to date just in case, but
it's not strictly required.

>
> Reviewed-by: Boyuan Zhang <Boyuan.Zhang at amd.com>

Thanks,

Alex

>
>
> > +     mutex_unlock(&adev->vcn.global_pg_lock);
> >
> >       return ret;
> >   }