[PATCH 3/3] drm/amdgpu: Add support for querying the max ibs in a submission.
Christian König
ckoenig.leichtzumerken at gmail.com
Mon Apr 10 15:10:36 UTC 2023
Am 09.04.23 um 17:59 schrieb Bas Nieuwenhuizen:
> 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.
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.
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.
I'm really wondering why this isn't used for this information.
Thanks,
Christian.
>
>
>
>
>
> 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/20230410/6d65e697/attachment-0001.htm>
More information about the amd-gfx
mailing list