Christian König <ckoenig.leichtzumer...@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.leichtzumer...@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?
> 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 <b...@basnieuwenhuizen.nl>
> >>> Cc: David Airlie <airl...@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;
>
>
