<div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Apr 9, 2023, 5:50 PM Timur Kristóf <<a href="mailto:timur.kristof@gmail.com">timur.kristof@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Christian König <<a href="mailto:ckoenig.leichtzumerken@gmail.com" target="_blank" rel="noreferrer">ckoenig.leichtzumerken@gmail.com</a>> ezt írta (időpont: 2023. ápr. 9., Vas 17:38):<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Am 09.04.23 um 17:32 schrieb Bas Nieuwenhuizen:<br>
> On Sun, Apr 9, 2023 at 5:30 PM Christian König<br>
> <<a href="mailto:ckoenig.leichtzumerken@gmail.com" rel="noreferrer noreferrer" target="_blank">ckoenig.leichtzumerken@gmail.com</a>> wrote:<br>
>> Am 09.04.23 um 16:44 schrieb Bas Nieuwenhuizen:<br>
>>> We need to introduce a new version of the info struct because<br>
>>> libdrm_amdgpu forgot any versioning info in amdgpu_query_hw_ip_info<br>
>>> so the mesa<->libdrm_amdgpu interface can't handle increasing the<br>
>>> size.<br>
>> Those are not forgotten, but simply unnecessary.<br>
>><br>
>> The size of the input output structures are given to the IOCTL in bytes<br>
>> and additional bytes should be filled with zeros.<br>
> At the ioctl side, yes, but it is libdrm_amdgpu filling in the size,<br>
> while passing in the struct directly from the client (mesa or<br>
> whatever). So if we have new libdrm_amdgpu and old mesa, then mesa<br>
> allocates N bytes on the stack and libdrm_amdgpu happily tells the<br>
> kernel in the ioctl "this struct is N+8 bytes long" which would cause<br>
> corruption?<br>
<br>
WTF? This has a wrapper in libdrm? Well then that's indeed horrible broken.<br>
<br>
In this case please define the new structure as extension of the old <br>
one. E.g. something like:<br>
<br>
struct drm_amdgpu_info_hw_ip2 {<br>
struct drm_amdgpu_info_hw_ip base;<br>
....<br>
};<br>
<br>
This way we can make it clear that this is an extension.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto">Can we solve this in userspace by letting mesa pass the struct size to libdrm as well? Or would that create other compatibility issues?</div></div></blockquote></div></div><div dir="auto"><br></div><div dir="auto">That is what the new wrapper in my libdrm patch does, but we still need the new struct to deal with the old broken wrapper.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks,<br>
Christian.<br>
<br>
><br>
> - Bas<br>
><br>
>> So you should be able to extend the structures at the end without<br>
>> breaking anything.<br>
>><br>
>> Regards,<br>
>> Christian.<br>
>><br>
>>> This info would be used by radv to figure out when we need to<br>
>>> split a submission into multiple submissions. radv currently has<br>
>>> a limit of 192 which seems to work for most gfx submissions, but<br>
>>> is way too high for e.g. compute or sdma.<br>
>>><br>
>>> Because the kernel handling is just fine we can just use the v2<br>
>>> everywhere and have the return_size do the versioning for us,<br>
>>> with userspace interpreting 0 as unknown.<br>
>>><br>
>>> Userspace implementation:<br>
>>> <a href="https://gitlab.freedesktop.org/bnieuwenhuizen/drm/-/tree/ib-rejection" rel="noreferrer noreferrer noreferrer" target="_blank">https://gitlab.freedesktop.org/bnieuwenhuizen/drm/-/tree/ib-rejection</a><br>
>>> <a href="https://gitlab.freedesktop.org/bnieuwenhuizen/mesa/-/tree/ib-rejection" rel="noreferrer noreferrer noreferrer" target="_blank">https://gitlab.freedesktop.org/bnieuwenhuizen/mesa/-/tree/ib-rejection</a><br>
>>><br>
>>> Link: <a href="https://gitlab.freedesktop.org/drm/amd/-/issues/2498" rel="noreferrer noreferrer noreferrer" target="_blank">https://gitlab.freedesktop.org/drm/amd/-/issues/2498</a><br>
>>> Signed-off-by: Bas Nieuwenhuizen <<a href="mailto:bas@basnieuwenhuizen.nl" rel="noreferrer noreferrer" target="_blank">bas@basnieuwenhuizen.nl</a>><br>
>>> Cc: David Airlie <<a href="mailto:airlied@gmail.com" rel="noreferrer noreferrer" target="_blank">airlied@gmail.com</a>><br>
>>> ---<br>
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 7 ++++--<br>
>>> include/uapi/drm/amdgpu_drm.h | 29 +++++++++++++++++++++++++<br>
>>> 2 files changed, 34 insertions(+), 2 deletions(-)<br>
>>><br>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c<br>
>>> index 89689b940493..c7e815c2733e 100644<br>
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c<br>
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c<br>
>>> @@ -360,7 +360,7 @@ static int amdgpu_firmware_info(struct drm_amdgpu_info_firmware *fw_info,<br>
>>><br>
>>> static int amdgpu_hw_ip_info(struct amdgpu_device *adev,<br>
>>> struct drm_amdgpu_info *info,<br>
>>> - struct drm_amdgpu_info_hw_ip *result)<br>
>>> + struct drm_amdgpu_info_hw_ip2 *result)<br>
>>> {<br>
>>> uint32_t ib_start_alignment = 0;<br>
>>> uint32_t ib_size_alignment = 0;<br>
>>> @@ -431,6 +431,7 @@ static int amdgpu_hw_ip_info(struct amdgpu_device *adev,<br>
>>> return -EINVAL;<br>
>>> }<br>
>>><br>
>>> + result->max_ibs = UINT_MAX;<br>
>>> for (i = 0; i < adev->num_rings; ++i) {<br>
>>> /* Note that this uses that ring types alias the equivalent<br>
>>> * HW IP exposes to userspace.<br>
>>> @@ -438,6 +439,8 @@ static int amdgpu_hw_ip_info(struct amdgpu_device *adev,<br>
>>> if (adev->rings[i]->funcs->type == info->query_hw_ip.type &&<br>
>>> adev->rings[i]->sched.ready) {<br>
>>> ++num_rings;<br>
>>> + result->max_ibs = min(result->max_ibs,<br>
>>> + adev->rings[i]->max_ibs);<br>
>>> }<br>
>>> }<br>
>>><br>
>>> @@ -536,7 +539,7 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)<br>
>>> }<br>
>>> return copy_to_user(out, &ui32, min(size, 4u)) ? -EFAULT : 0;<br>
>>> case AMDGPU_INFO_HW_IP_INFO: {<br>
>>> - struct drm_amdgpu_info_hw_ip ip = {};<br>
>>> + struct drm_amdgpu_info_hw_ip2 ip = {};<br>
>>> int ret;<br>
>>><br>
>>> ret = amdgpu_hw_ip_info(adev, info, &ip);<br>
>>> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h<br>
>>> index b6eb90df5d05..45e5ae546d19 100644<br>
>>> --- a/include/uapi/drm/amdgpu_drm.h<br>
>>> +++ b/include/uapi/drm/amdgpu_drm.h<br>
>>> @@ -1128,6 +1128,9 @@ struct drm_amdgpu_info_device {<br>
>>> __u32 enabled_rb_pipes_mask_hi;<br>
>>> };<br>
>>><br>
>>> +/* The size of this struct cannot be increased for compatibility reasons, use<br>
>>> + * struct drm_amdgpu_info_hw_ip2 instead.<br>
>>> + */<br>
>>> struct drm_amdgpu_info_hw_ip {<br>
>>> /** Version of h/w IP */<br>
>>> __u32 hw_ip_version_major;<br>
>>> @@ -1144,6 +1147,32 @@ struct drm_amdgpu_info_hw_ip {<br>
>>> __u32 ip_discovery_version;<br>
>>> };<br>
>>><br>
>>> +/* The prefix fields of this are intentionally the same as those of<br>
>>> + * struct drm_amdgpu_info_hw_ip. The struct has a v2 to resolve a lack of<br>
>>> + * versioning on the libdrm_amdgpu side.<br>
>>> + */<br>
>>> +struct drm_amdgpu_info_hw_ip2 {<br>
>>> + /** Version of h/w IP */<br>
>>> + __u32 hw_ip_version_major;<br>
>>> + __u32 hw_ip_version_minor;<br>
>>> + /** Capabilities */<br>
>>> + __u64 capabilities_flags;<br>
>>> + /** command buffer address start alignment*/<br>
>>> + __u32 ib_start_alignment;<br>
>>> + /** command buffer size alignment*/<br>
>>> + __u32 ib_size_alignment;<br>
>>> + /** Bitmask of available rings. Bit 0 means ring 0, etc. */<br>
>>> + __u32 available_rings;<br>
>>> + /** version info: bits 23:16 major, 15:8 minor, 7:0 revision */<br>
>>> + __u32 ip_discovery_version;<br>
>>> + /** The maximum number of IBs one can submit in a single submission<br>
>>> + * ioctl. (When using gang submit: this is per IP type)<br>
>>> + */<br>
>>> + __u32 max_ibs;<br>
>>> + /** padding to 64bit for arch differences */<br>
>>> + __u32 pad;<br>
>>> +};<br>
>>> +<br>
>>> struct drm_amdgpu_info_num_handles {<br>
>>> /** Max handles as supported by firmware for UVD */<br>
>>> __u32 uvd_max_handles;<br>
<br>
</blockquote></div></div></div>
</blockquote></div></div></div>