<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
Am 09.04.23 um 17:59 schrieb Bas Nieuwenhuizen:<br>
<blockquote type="cite"
cite="mid:CAP+8YyEzB9UoLuTYjmHXDYdTOdE2mFeYN5CzhzpJ3O=VuHTH5g@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="auto">
<div>On Sun, Apr 9, 2023, 5:50 PM Timur Kristóf <<a
href="mailto:timur.kristof@gmail.com" moz-do-not-send="true"
class="moz-txt-link-freetext">timur.kristof@gmail.com</a>>
wrote:<br>
<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><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"
moz-do-not-send="true"
class="moz-txt-link-freetext">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"
moz-do-not-send="true"
class="moz-txt-link-freetext">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>
</blockquote>
<br>
Let me double check what exactly goes wrong here. I clearly remember
that while we designed this I've pointed out that structures can't
be extended when they are part of the libdrm C ABI.<br>
<br>
Because of this we added a general query function which takes an
enum and parameters what to querey and an out buffer with size for
the result.<br>
<br>
I'm really wondering why this isn't used for this information.<br>
<br>
Thanks,<br>
Christian.<br>
<br>
<blockquote type="cite"
cite="mid:CAP+8YyEzB9UoLuTYjmHXDYdTOdE2mFeYN5CzhzpJ3O=VuHTH5g@mail.gmail.com">
<div dir="auto">
<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" moz-do-not-send="true"
class="moz-txt-link-freetext">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" moz-do-not-send="true"
class="moz-txt-link-freetext">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" moz-do-not-send="true"
class="moz-txt-link-freetext">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"
moz-do-not-send="true"
class="moz-txt-link-freetext">bas@basnieuwenhuizen.nl</a>><br>
>>> Cc: David Airlie <<a
href="mailto:airlied@gmail.com" rel="noreferrer
noreferrer" target="_blank"
moz-do-not-send="true"
class="moz-txt-link-freetext">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>
</blockquote>
<br>
</body>
</html>