[PATCH 3/3] drm/amdgpu: Add support for querying the max ibs in a submission.
Bas Nieuwenhuizen
bas at basnieuwenhuizen.nl
Sun Apr 9 15:59:11 UTC 2023
On Sun, Apr 9, 2023, 5:50 PM Timur Kristóf <timur.kristof at gmail.com> wrote:
>
>
> Christian König <ckoenig.leichtzumerken at gmail.com> 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 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.
>>
>
>
> Can we solve this in userspace by letting mesa pass the struct size to
> libdrm as well? Or would that create other compatibility issues?
>
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.
>
>
>
>> 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;
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20230409/dd8848b0/attachment-0001.htm>
More information about the amd-gfx
mailing list