[PATCH 15/15] drm/amdgpu/vcn: update work handler for per instance powergating

[Alex Deucher]

On Fri, Nov 15, 2024 at 7:27 AM Christian König
<ckoenig.leichtzumerken at gmail.com> wrote:
>
> Am 13.11.24 um 22:44 schrieb Alex Deucher:
> > Only gate/ungate the relevant instances.
>
> That won't work, that is the whole problem why we started this series in
> the first place.

Why won't it work?  From my perspective, it was not that it wouldn't
work, but that it did not align well with the original abstraction.  I
assumed it would be easier and cleaner to split the instances as one
instance per IP block, but in doing that we also end up with a lot of
messy corner cases we had missed.  To solve them we end up with sort
of a hybrid where we have a mix of per IP block logic and global IP
specific logic.  Whis is kind of what we have now, but requires a lot
of churn and chances for regressions.

I think we could still go this route, but we need to take a different
path to get there.  We need to keep the multiple instances per IP
block in tact up until the last minute.  So, in the case of VCN, I
think it should work like this, building on this patch set:
1. Keep the current multiple instances per IP block.
2. Split the core vcn helper code in amdgpu_vcn.c to be per instance.
Carefully split the state in adev->vcn so that all of the instance
specific logic is stored in the vcn_instance structures.
3. Figure out how to move the IP specific harvesting logic out of the
IP specific code.  Maybe this means adding a new IP block callback
get_harvest_info.  This could then get called early to mark whether an
instance is harvested or not.
4. Figure out how to handle VCN specific sysfs files.  Presumably they
should be per instance.
5. Figure out how to deal with some of the interrupt corner cases
where on client id and src id, but the instance comes from the IV
cookie.
6. Figure out how best to deal with shared firmwares used by multiple
instances.  Worst case, we can just duplicate the firmware per
instance.  Some VCN IP versions have different firmware per instance,
some use the same firmware per instance.
7. Update all of the IP_VERSION checks to check the appropriate IP
version for each instance.  Currently we always check instance 0 and
make sure all of the instances are properly populated in that table.
There are cases today where only instance 0 of the IP version
information is populated even though there are multiple instances of
the IP.
8. Store the number of instances for each IP type in some common place
in adev.  Update the IP discovery code and IP specific instance
handling to update the core number of instances.
9. Add an ip priv pointer to ip_block structure.
10. Use the core number of instances for each IP type in
amdgpu_discovery_set_mm_ip_blocks() to add the IP blocks, one per
instance.  Hang the vcn_instance structures off of the ip block
structure priv pointer. Drop the VCN set powergating instance
calbacks.

>
> Regards,
> Christian.
>
> >
> > Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> > index 3383e4146c6a..24e9a3126763 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> > @@ -418,8 +418,8 @@ static void amdgpu_vcn_idle_work_handler(struct work_struct *work)
> >       fences += fence[i];
> >
> >       if (!fences && !atomic_read(&vcn_inst->total_submission_cnt)) {
> > -             amdgpu_device_ip_set_powergating_state(adev, AMD_IP_BLOCK_TYPE_VCN,
> > -                                                    AMD_PG_STATE_GATE);
> > +             amdgpu_device_ip_set_powergating_state_inst(adev, AMD_IP_BLOCK_TYPE_VCN,
> > +                                                         AMD_PG_STATE_GATE, i);
> >               r = amdgpu_dpm_switch_power_profile(adev, PP_SMC_POWER_PROFILE_VIDEO,
> >                                                   false);
> >               if (r)
> > @@ -444,8 +444,8 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
> >       }
> >
> >       mutex_lock(&adev->vcn.inst[ring->me].vcn_pg_lock);
> > -     amdgpu_device_ip_set_powergating_state(adev, AMD_IP_BLOCK_TYPE_VCN,
> > -                                            AMD_PG_STATE_UNGATE);
> > +     amdgpu_device_ip_set_powergating_state_inst(adev, AMD_IP_BLOCK_TYPE_VCN,
> > +                                                 AMD_PG_STATE_UNGATE, ring->me);
> >
> >       /* Only set DPG pause for VCN3 or below, VCN4 and above will be handled by FW */
> >       if (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG &&
>