From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/msm: restore GEM-related IOCTLs for KMS devices Date: Thu, 05 Mar 2026 13:25:16 +1000 Message-ID: In-Reply-To: <20260304-msm-restore-ioctls-v1-1-b28f9231fcd2@oss.qualcomm.com> References: <20260304-msm-restore-ioctls-v1-1-b28f9231fcd2@oss.qualcomm.com> <20260304-msm-restore-ioctls-v1-1-b28f9231fcd2@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Correctness of ioctl table indexing: OK** The `DRM_IOCTL_DEF_DRV` macro uses designated initializers: ```c [DRM_IOCTL_NR(DRM_IOCTL_##ioctl) - DRM_COMMAND_BASE] =3D { ... } ``` So entries in `msm_kms_ioctls[]` will be placed at their correct indices (e= .g., `MSM_GEM_NEW` at index 2, `MSM_GEM_MADVISE` at index 8) with zero-fill= ed gaps. `ARRAY_SIZE()` will correctly yield 9. This is fine. **Concern 1: `MSM_INFO_GET_IOVA` and `MSM_INFO_SET_IOVA` on KMS-only device= s** The `msm_ioctl_gem_info` handler is exposed via `MSM_GEM_INFO` in the new t= able. Some of its sub-commands (`MSM_INFO_GET_IOVA`, `MSM_INFO_SET_IOVA`) c= all `msm_ioctl_gem_info_iova()` and `msm_ioctl_gem_info_set_iova()` respect= ively, which check `if (!priv->gpu) return -EINVAL` (lines 428 and 452). On= a KMS-only device, `priv->gpu` will be NULL, so these sub-commands will re= turn `-EINVAL`, which is safe but could be confusing. This is probably fine= since the other sub-commands (`MSM_INFO_GET_OFFSET`, `MSM_INFO_GET_FLAGS`,= metadata, name) all work without a GPU. Worth confirming this matches the = intended use case though. **Concern 2: Missing `gem_prime_import` in `msm_kms_driver`** The combined `msm_driver` has `.gem_prime_import =3D msm_gem_prime_import` = but `msm_kms_driver` does not. If userspace allocates a GEM buffer via `MSM= _GEM_NEW` on the KMS device and then exports/re-imports it, the default `dr= m_gem_prime_import` will be used instead of `msm_gem_prime_import`. This ma= y or may not matter depending on the GBM backend workflow (which exports fr= om KMS and imports into the GPU driver, not the reverse). But it's worth no= ting that there's an asymmetry here. **Concern 3: Commit message notes raise a question** The author's note: > Note, here I assume that dumb buffers generally should not be used for > rendering. That doesn't seem to be complete truth as Mesa kmsro on MSM > devices uses DRM_IOCTL_MODE_CREATE_DUMB to create buffers for resources. This is an honest acknowledgment that weakens the justification somewhat. I= f kmsro already uses dumb buffers successfully for rendering on MSM, then t= he original commit 98f11fd1cf92 may have been intentional in pushing toward= s that path. The patch could benefit from clarifying which specific use cas= e is broken =E2=80=94 is it specifically the MSM GBM backend (as opposed to= kmsro), and on which platforms/configurations? **Minor: No `MSM_GET_PARAM` / `MSM_SET_PARAM`** The patch deliberately omits `MSM_GET_PARAM` and `MSM_SET_PARAM`. This seem= s intentional since those are GPU-related queries, but userspace that calls= `MSM_GEM_NEW` might also query params to determine supported flags or GPU = capabilities. If the GBM backend needs param queries too, this will need a = follow-up. **Summary:** The patch is technically sound and addresses a real regression= . The main open question is whether this is the right set of ioctls and whe= ther the commit message should more precisely identify the broken use case.= The `!priv->gpu` guards in `gem_info` sub-commands provide adequate safety= for the KMS-only case. --- Generated by Claude Code Patch Reviewer