Am 22.09.21 um 22:25 schrieb Alex Deucher:
On Wed, Sep 22, 2021 at 3:54 AM Christian König
<ckoenig.leichtzumerken@xxxxxxxxx> 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
