On Fri, Nov 15, 2024 at 7:27 AM Christian König <ckoenig.leichtzumerken@xxxxxxxxx> 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@xxxxxxx> > > --- > > 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 && >