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