[PATCH 00/66] Move to IP driven device enumeration

[Christian König]

Am 22.09.21 um 22:25 schrieb Alex Deucher:
> On Wed, Sep 22, 2021 at 3:54 AM Christian König
> <ckoenig.leichtzumerken at gmail.com> wrote:
>> [SNIP]
>> Comment for patch #32:
>>
>> Maybe adjust the commit subject, otherwise somebody could think it's a
>> revert the the previous patch.
> I was thinking I could just apply 31 independently of what happens to
> this patch set.  I meant to split it out as a separate bug fix patch,
> but it got lost in the other patches.

Good point. I suggest to just push it to amd-staging-drm-next ASAP and 
not consider it part of this set any more.

>> Comment on patch #51, #52 and #61:
>>
>> That looks just a little bit questionable. Could we clean that up or
>> would that be to much churn for little gain?
> What did you have in mind?  As I mentioned in the reply to Lijo, the
> IP discovery table uses a mix of separate HWIDs and multiple instances
> of the same HWID to handle instancing.

Patch #52 adds something like: "adev->ip_versions[hw_ip + 
ip->number_instance] =..."

While patch #61 then cleans that up towards: 
"adev->ip_versions[hw_ip][ip->number_instance] =..."

It would be nice to have some clean concept which handles all of the 
hw_ip oddies gracefully in the first place. And then add what you wrote 
to Lijo as comment somewhere as well.

Not a must have, but I think it would make things a bit cleaner and more 
review- and maintain-able.

>> Comment on patch #63:
>>
>>>                case IP_VERSION(7, 2, 0):
>>> -                     amdgpu_device_ip_block_add(adev, &uvd_v7_0_ip_block);
>>> +                     if (!(adev->asic_type == CHIP_VEGA20 && amdgpu_sriov_vf(adev)))
>>> +                             amdgpu_device_ip_block_add(adev, &uvd_v7_0_ip_block);
>> Checking the IP version and then the chip type looks questionable. I
>> have an idea where this comes from, but please confirm with a comment.
> It's just because SR-IOV is only enabled on certain asics and that is
> the way the old code looked.  I guess I could drop the asic_type
> checks.  We'd never end up in with amdgpu_sriov_vf() returning true on
> a asic without SR-IOV support in the first place.

Yeah, either that our just add something like "SRIOV is only enabled on 
certain asics" as comment.

Christian.

>
> Alex
>