[PATCH 3/3] drm/amdgpu: Add support for querying the max ibs in a submission.

[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?

- 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;
>