[PATCH 3/3] drm/amdgpu: Add support for querying the max ibs in a submission.
Christian König
ckoenig.leichtzumerken at gmail.com
Sun Apr 9 15:38:13 UTC 2023
Am 09.04.23 um 17:32 schrieb Bas Nieuwenhuizen:
> On Sun, Apr 9, 2023 at 5:30 PM Christian König
> <ckoenig.leichtzumerken at gmail.com> 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.
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 at basnieuwenhuizen.nl>
>>> Cc: David Airlie <airlied at gmail.com>
>>> ---
>>> 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;
More information about the amd-gfx
mailing list