public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/msm: restore GEM-related IOCTLs for KMS devices
@ 2026-03-04 13:34 Dmitry Baryshkov
  2026-03-04 14:57 ` Rob Clark
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Dmitry Baryshkov @ 2026-03-04 13:34 UTC (permalink / raw)
  To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
	Sean Paul, Marijn Suijten, David Airlie, Simona Vetter
  Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel

The MSM GBM backend uses MSM_GEM_NEW to allocate GEM buffers from the
KMS driver, imports them into the GPU driver (msm or kgsl) and then
uses them for rendering / blending. Commit 98f11fd1cf92 ("drm/msm: Take
the ioctls away from the KMS-only driver") dropped all IOCTLs from the
MSM KMS devices, pointing out the need to use dumb buffers, however dumb
buffers should not be used by the GPU for rendering. Restore GEM-related
IOCTLs for the KMS devices.

Fixes: 98f11fd1cf92 ("drm/msm: Take the ioctls away from the KMS-only driver")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
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.
---
 drivers/gpu/drm/msm/msm_drv.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index ed2a61c66ac9..f82c9a36a166 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -799,6 +799,14 @@ static const struct drm_ioctl_desc msm_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(MSM_VM_BIND,      msm_ioctl_vm_bind,      DRM_RENDER_ALLOW),
 };
 
+static const struct drm_ioctl_desc msm_kms_ioctls[] = {
+	DRM_IOCTL_DEF_DRV(MSM_GEM_NEW,      msm_ioctl_gem_new,      DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(MSM_GEM_INFO,     msm_ioctl_gem_info,     DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_PREP, msm_ioctl_gem_cpu_prep, DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_FINI, msm_ioctl_gem_cpu_fini, DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(MSM_GEM_MADVISE,  msm_ioctl_gem_madvise,  DRM_RENDER_ALLOW),
+};
+
 static void msm_show_fdinfo(struct drm_printer *p, struct drm_file *file)
 {
 	struct drm_device *dev = file->minor->dev;
@@ -868,6 +876,8 @@ static const struct drm_driver msm_kms_driver = {
 #endif
 	MSM_FBDEV_DRIVER_OPS,
 	.show_fdinfo        = msm_show_fdinfo,
+	.ioctls             = msm_kms_ioctls,
+	.num_ioctls         = ARRAY_SIZE(msm_kms_ioctls),
 	.fops               = &fops,
 	.name               = "msm-kms",
 	.desc               = "MSM Snapdragon DRM",

---
base-commit: ac47870fd795549f03d57e0879fc730c79119f4b
change-id: 20260304-msm-restore-ioctls-2a1792311a09

Best regards,
-- 
With best wishes
Dmitry


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] drm/msm: restore GEM-related IOCTLs for KMS devices
  2026-03-04 13:34 [PATCH] drm/msm: restore GEM-related IOCTLs for KMS devices Dmitry Baryshkov
@ 2026-03-04 14:57 ` Rob Clark
  2026-03-04 14:59   ` Rob Clark
  2026-03-04 15:57 ` Rob Clark
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Rob Clark @ 2026-03-04 14:57 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang, Sean Paul,
	Marijn Suijten, David Airlie, Simona Vetter, linux-arm-msm,
	dri-devel, freedreno, linux-kernel

On Wed, Mar 4, 2026 at 5:34 AM Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com> wrote:
>
> The MSM GBM backend uses MSM_GEM_NEW to allocate GEM buffers from the
> KMS driver, imports them into the GPU driver (msm or kgsl) and then
> uses them for rendering / blending. Commit 98f11fd1cf92 ("drm/msm: Take
> the ioctls away from the KMS-only driver") dropped all IOCTLs from the
> MSM KMS devices, pointing out the need to use dumb buffers, however dumb
> buffers should not be used by the GPU for rendering. Restore GEM-related
> IOCTLs for the KMS devices.
>
> Fixes: 98f11fd1cf92 ("drm/msm: Take the ioctls away from the KMS-only driver")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
> 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.

That is problematic in kmsro.. (but also unsure to what degree kmsro
ever got used "in production".. the x86 drivers don't use it.  Android
and chromeos didn't use it.  Etc.)

BR,
-R

> ---
>  drivers/gpu/drm/msm/msm_drv.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index ed2a61c66ac9..f82c9a36a166 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -799,6 +799,14 @@ static const struct drm_ioctl_desc msm_ioctls[] = {
>         DRM_IOCTL_DEF_DRV(MSM_VM_BIND,      msm_ioctl_vm_bind,      DRM_RENDER_ALLOW),
>  };
>
> +static const struct drm_ioctl_desc msm_kms_ioctls[] = {
> +       DRM_IOCTL_DEF_DRV(MSM_GEM_NEW,      msm_ioctl_gem_new,      DRM_RENDER_ALLOW),
> +       DRM_IOCTL_DEF_DRV(MSM_GEM_INFO,     msm_ioctl_gem_info,     DRM_RENDER_ALLOW),
> +       DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_PREP, msm_ioctl_gem_cpu_prep, DRM_RENDER_ALLOW),
> +       DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_FINI, msm_ioctl_gem_cpu_fini, DRM_RENDER_ALLOW),
> +       DRM_IOCTL_DEF_DRV(MSM_GEM_MADVISE,  msm_ioctl_gem_madvise,  DRM_RENDER_ALLOW),
> +};
> +
>  static void msm_show_fdinfo(struct drm_printer *p, struct drm_file *file)
>  {
>         struct drm_device *dev = file->minor->dev;
> @@ -868,6 +876,8 @@ static const struct drm_driver msm_kms_driver = {
>  #endif
>         MSM_FBDEV_DRIVER_OPS,
>         .show_fdinfo        = msm_show_fdinfo,
> +       .ioctls             = msm_kms_ioctls,
> +       .num_ioctls         = ARRAY_SIZE(msm_kms_ioctls),
>         .fops               = &fops,
>         .name               = "msm-kms",
>         .desc               = "MSM Snapdragon DRM",
>
> ---
> base-commit: ac47870fd795549f03d57e0879fc730c79119f4b
> change-id: 20260304-msm-restore-ioctls-2a1792311a09
>
> Best regards,
> --
> With best wishes
> Dmitry
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] drm/msm: restore GEM-related IOCTLs for KMS devices
  2026-03-04 14:57 ` Rob Clark
@ 2026-03-04 14:59   ` Rob Clark
  2026-03-05  1:03     ` Dmitry Baryshkov
  0 siblings, 1 reply; 8+ messages in thread
From: Rob Clark @ 2026-03-04 14:59 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang, Sean Paul,
	Marijn Suijten, David Airlie, Simona Vetter, linux-arm-msm,
	dri-devel, freedreno, linux-kernel

On Wed, Mar 4, 2026 at 6:57 AM Rob Clark <rob.clark@oss.qualcomm.com> wrote:
>
> On Wed, Mar 4, 2026 at 5:34 AM Dmitry Baryshkov
> <dmitry.baryshkov@oss.qualcomm.com> wrote:
> >
> > The MSM GBM backend uses MSM_GEM_NEW to allocate GEM buffers from the
> > KMS driver, imports them into the GPU driver (msm or kgsl) and then
> > uses them for rendering / blending. Commit 98f11fd1cf92 ("drm/msm: Take
> > the ioctls away from the KMS-only driver") dropped all IOCTLs from the
> > MSM KMS devices, pointing out the need to use dumb buffers, however dumb
> > buffers should not be used by the GPU for rendering. Restore GEM-related
> > IOCTLs for the KMS devices.
> >
> > Fixes: 98f11fd1cf92 ("drm/msm: Take the ioctls away from the KMS-only driver")
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > ---
> > 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.
>
> That is problematic in kmsro.. (but also unsure to what degree kmsro
> ever got used "in production".. the x86 drivers don't use it.  Android
> and chromeos didn't use it.  Etc.)

(also, allocate from the gpu render node)

BR,
-R

> BR,
> -R
>
> > ---
> >  drivers/gpu/drm/msm/msm_drv.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index ed2a61c66ac9..f82c9a36a166 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -799,6 +799,14 @@ static const struct drm_ioctl_desc msm_ioctls[] = {
> >         DRM_IOCTL_DEF_DRV(MSM_VM_BIND,      msm_ioctl_vm_bind,      DRM_RENDER_ALLOW),
> >  };
> >
> > +static const struct drm_ioctl_desc msm_kms_ioctls[] = {
> > +       DRM_IOCTL_DEF_DRV(MSM_GEM_NEW,      msm_ioctl_gem_new,      DRM_RENDER_ALLOW),
> > +       DRM_IOCTL_DEF_DRV(MSM_GEM_INFO,     msm_ioctl_gem_info,     DRM_RENDER_ALLOW),
> > +       DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_PREP, msm_ioctl_gem_cpu_prep, DRM_RENDER_ALLOW),
> > +       DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_FINI, msm_ioctl_gem_cpu_fini, DRM_RENDER_ALLOW),
> > +       DRM_IOCTL_DEF_DRV(MSM_GEM_MADVISE,  msm_ioctl_gem_madvise,  DRM_RENDER_ALLOW),
> > +};
> > +
> >  static void msm_show_fdinfo(struct drm_printer *p, struct drm_file *file)
> >  {
> >         struct drm_device *dev = file->minor->dev;
> > @@ -868,6 +876,8 @@ static const struct drm_driver msm_kms_driver = {
> >  #endif
> >         MSM_FBDEV_DRIVER_OPS,
> >         .show_fdinfo        = msm_show_fdinfo,
> > +       .ioctls             = msm_kms_ioctls,
> > +       .num_ioctls         = ARRAY_SIZE(msm_kms_ioctls),
> >         .fops               = &fops,
> >         .name               = "msm-kms",
> >         .desc               = "MSM Snapdragon DRM",
> >
> > ---
> > base-commit: ac47870fd795549f03d57e0879fc730c79119f4b
> > change-id: 20260304-msm-restore-ioctls-2a1792311a09
> >
> > Best regards,
> > --
> > With best wishes
> > Dmitry
> >

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] drm/msm: restore GEM-related IOCTLs for KMS devices
  2026-03-04 13:34 [PATCH] drm/msm: restore GEM-related IOCTLs for KMS devices Dmitry Baryshkov
  2026-03-04 14:57 ` Rob Clark
@ 2026-03-04 15:57 ` Rob Clark
  2026-03-05  0:44   ` Dmitry Baryshkov
  2026-03-05  3:25 ` Claude review: " Claude Code Review Bot
  2026-03-05  3:25 ` Claude Code Review Bot
  3 siblings, 1 reply; 8+ messages in thread
From: Rob Clark @ 2026-03-04 15:57 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang, Sean Paul,
	Marijn Suijten, David Airlie, Simona Vetter, linux-arm-msm,
	dri-devel, freedreno, linux-kernel

On Wed, Mar 4, 2026 at 5:34 AM Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com> wrote:
>
> The MSM GBM backend uses MSM_GEM_NEW to allocate GEM buffers from the
> KMS driver, imports them into the GPU driver (msm or kgsl) and then
> uses them for rendering / blending. Commit 98f11fd1cf92 ("drm/msm: Take
> the ioctls away from the KMS-only driver") dropped all IOCTLs from the
> MSM KMS devices, pointing out the need to use dumb buffers, however dumb
> buffers should not be used by the GPU for rendering. Restore GEM-related
> IOCTLs for the KMS devices.

We shouldn't be exposing DRIVER_RENDER on KMS-only nodes.  If msm gbm
is allocating from a primary node, that is problematic, since there is
no guarantee that anything but the compositor has access to the
primary node.

BR,
-R

>
> Fixes: 98f11fd1cf92 ("drm/msm: Take the ioctls away from the KMS-only driver")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
> 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.
> ---
>  drivers/gpu/drm/msm/msm_drv.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index ed2a61c66ac9..f82c9a36a166 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -799,6 +799,14 @@ static const struct drm_ioctl_desc msm_ioctls[] = {
>         DRM_IOCTL_DEF_DRV(MSM_VM_BIND,      msm_ioctl_vm_bind,      DRM_RENDER_ALLOW),
>  };
>
> +static const struct drm_ioctl_desc msm_kms_ioctls[] = {
> +       DRM_IOCTL_DEF_DRV(MSM_GEM_NEW,      msm_ioctl_gem_new,      DRM_RENDER_ALLOW),
> +       DRM_IOCTL_DEF_DRV(MSM_GEM_INFO,     msm_ioctl_gem_info,     DRM_RENDER_ALLOW),
> +       DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_PREP, msm_ioctl_gem_cpu_prep, DRM_RENDER_ALLOW),
> +       DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_FINI, msm_ioctl_gem_cpu_fini, DRM_RENDER_ALLOW),
> +       DRM_IOCTL_DEF_DRV(MSM_GEM_MADVISE,  msm_ioctl_gem_madvise,  DRM_RENDER_ALLOW),
> +};
> +
>  static void msm_show_fdinfo(struct drm_printer *p, struct drm_file *file)
>  {
>         struct drm_device *dev = file->minor->dev;
> @@ -868,6 +876,8 @@ static const struct drm_driver msm_kms_driver = {
>  #endif
>         MSM_FBDEV_DRIVER_OPS,
>         .show_fdinfo        = msm_show_fdinfo,
> +       .ioctls             = msm_kms_ioctls,
> +       .num_ioctls         = ARRAY_SIZE(msm_kms_ioctls),
>         .fops               = &fops,
>         .name               = "msm-kms",
>         .desc               = "MSM Snapdragon DRM",
>
> ---
> base-commit: ac47870fd795549f03d57e0879fc730c79119f4b
> change-id: 20260304-msm-restore-ioctls-2a1792311a09
>
> Best regards,
> --
> With best wishes
> Dmitry
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] drm/msm: restore GEM-related IOCTLs for KMS devices
  2026-03-04 15:57 ` Rob Clark
@ 2026-03-05  0:44   ` Dmitry Baryshkov
  0 siblings, 0 replies; 8+ messages in thread
From: Dmitry Baryshkov @ 2026-03-05  0:44 UTC (permalink / raw)
  To: Rob Clark
  Cc: Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang, Sean Paul,
	Marijn Suijten, David Airlie, Simona Vetter, linux-arm-msm,
	dri-devel, freedreno, linux-kernel

On Wed, Mar 04, 2026 at 07:57:34AM -0800, Rob Clark wrote:
> On Wed, Mar 4, 2026 at 5:34 AM Dmitry Baryshkov
> <dmitry.baryshkov@oss.qualcomm.com> wrote:
> >
> > The MSM GBM backend uses MSM_GEM_NEW to allocate GEM buffers from the
> > KMS driver, imports them into the GPU driver (msm or kgsl) and then
> > uses them for rendering / blending. Commit 98f11fd1cf92 ("drm/msm: Take
> > the ioctls away from the KMS-only driver") dropped all IOCTLs from the
> > MSM KMS devices, pointing out the need to use dumb buffers, however dumb
> > buffers should not be used by the GPU for rendering. Restore GEM-related
> > IOCTLs for the KMS devices.
> 
> We shouldn't be exposing DRIVER_RENDER on KMS-only nodes.  If msm gbm
> is allocating from a primary node, that is problematic, since there is
> no guarantee that anything but the compositor has access to the
> primary node.

We are not exposing DRIVER_RENDER for KMS-only nodes. I'll check with
the correponding colleagues, what exactly is their use case, if the
allocation is happening in the compositor or in the app. In the latter
case, I agree, the buffers should be allocated from the GPU.

> 
> BR,
> -R
> 
> >
> > Fixes: 98f11fd1cf92 ("drm/msm: Take the ioctls away from the KMS-only driver")
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > ---
> > 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.
> > ---
> >  drivers/gpu/drm/msm/msm_drv.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index ed2a61c66ac9..f82c9a36a166 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -799,6 +799,14 @@ static const struct drm_ioctl_desc msm_ioctls[] = {
> >         DRM_IOCTL_DEF_DRV(MSM_VM_BIND,      msm_ioctl_vm_bind,      DRM_RENDER_ALLOW),
> >  };
> >
> > +static const struct drm_ioctl_desc msm_kms_ioctls[] = {
> > +       DRM_IOCTL_DEF_DRV(MSM_GEM_NEW,      msm_ioctl_gem_new,      DRM_RENDER_ALLOW),
> > +       DRM_IOCTL_DEF_DRV(MSM_GEM_INFO,     msm_ioctl_gem_info,     DRM_RENDER_ALLOW),
> > +       DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_PREP, msm_ioctl_gem_cpu_prep, DRM_RENDER_ALLOW),
> > +       DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_FINI, msm_ioctl_gem_cpu_fini, DRM_RENDER_ALLOW),
> > +       DRM_IOCTL_DEF_DRV(MSM_GEM_MADVISE,  msm_ioctl_gem_madvise,  DRM_RENDER_ALLOW),
> > +};
> > +
> >  static void msm_show_fdinfo(struct drm_printer *p, struct drm_file *file)
> >  {
> >         struct drm_device *dev = file->minor->dev;
> > @@ -868,6 +876,8 @@ static const struct drm_driver msm_kms_driver = {
> >  #endif
> >         MSM_FBDEV_DRIVER_OPS,
> >         .show_fdinfo        = msm_show_fdinfo,
> > +       .ioctls             = msm_kms_ioctls,
> > +       .num_ioctls         = ARRAY_SIZE(msm_kms_ioctls),
> >         .fops               = &fops,
> >         .name               = "msm-kms",
> >         .desc               = "MSM Snapdragon DRM",
> >
> > ---
> > base-commit: ac47870fd795549f03d57e0879fc730c79119f4b
> > change-id: 20260304-msm-restore-ioctls-2a1792311a09
> >
> > Best regards,
> > --
> > With best wishes
> > Dmitry
> >

-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] drm/msm: restore GEM-related IOCTLs for KMS devices
  2026-03-04 14:59   ` Rob Clark
@ 2026-03-05  1:03     ` Dmitry Baryshkov
  0 siblings, 0 replies; 8+ messages in thread
From: Dmitry Baryshkov @ 2026-03-05  1:03 UTC (permalink / raw)
  To: Rob Clark
  Cc: Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang, Sean Paul,
	Marijn Suijten, David Airlie, Simona Vetter, linux-arm-msm,
	dri-devel, freedreno, linux-kernel

On Wed, Mar 04, 2026 at 06:59:42AM -0800, Rob Clark wrote:
> On Wed, Mar 4, 2026 at 6:57 AM Rob Clark <rob.clark@oss.qualcomm.com> wrote:
> >
> > On Wed, Mar 4, 2026 at 5:34 AM Dmitry Baryshkov
> > <dmitry.baryshkov@oss.qualcomm.com> wrote:
> > >
> > > The MSM GBM backend uses MSM_GEM_NEW to allocate GEM buffers from the
> > > KMS driver, imports them into the GPU driver (msm or kgsl) and then
> > > uses them for rendering / blending. Commit 98f11fd1cf92 ("drm/msm: Take
> > > the ioctls away from the KMS-only driver") dropped all IOCTLs from the
> > > MSM KMS devices, pointing out the need to use dumb buffers, however dumb
> > > buffers should not be used by the GPU for rendering. Restore GEM-related
> > > IOCTLs for the KMS devices.
> > >
> > > Fixes: 98f11fd1cf92 ("drm/msm: Take the ioctls away from the KMS-only driver")
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > > ---
> > > 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.
> >
> > That is problematic in kmsro.. (but also unsure to what degree kmsro
> > ever got used "in production".. the x86 drivers don't use it.  Android
> > and chromeos didn't use it.  Etc.)
> 
> (also, allocate from the gpu render node)

I think Asahi and VC4 allocate buffers from the GPU node and then import
them on the render side, but unfortunately iMX IPU driver doesn't seem
to support PRIME_FD_TO_HANDLE.

-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Claude review: drm/msm: restore GEM-related IOCTLs for KMS devices
  2026-03-04 13:34 [PATCH] drm/msm: restore GEM-related IOCTLs for KMS devices Dmitry Baryshkov
                   ` (2 preceding siblings ...)
  2026-03-05  3:25 ` Claude review: " Claude Code Review Bot
@ 2026-03-05  3:25 ` Claude Code Review Bot
  3 siblings, 0 replies; 8+ messages in thread
From: Claude Code Review Bot @ 2026-03-05  3:25 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/msm: restore GEM-related IOCTLs for KMS devices
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Patches: 6
Reviewed: 2026-03-05T13:25:16.724865

---

This is a single-patch fix by Dmitry Baryshkov that restores a subset of GEM-related IOCTLs to the MSM KMS-only driver, which were removed by commit 98f11fd1cf92. The motivation is that the MSM GBM backend needs `MSM_GEM_NEW` to allocate buffers from the KMS device and then imports them into the GPU driver for rendering — dumb buffers should not be used for GPU rendering.

The patch is technically correct in its implementation (designated initializers in `DRM_IOCTL_DEF_DRV` handle array indexing properly). However, there are a few points worth discussing.

**Overall assessment:** Reasonable fix for a real regression, but some design questions remain about whether this is the right subset of ioctls and whether `MSM_INFO_GET_IOVA` / `MSM_INFO_SET_IOVA` sub-commands within `GEM_INFO` will behave correctly without a GPU.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Claude review: drm/msm: restore GEM-related IOCTLs for KMS devices
  2026-03-04 13:34 [PATCH] drm/msm: restore GEM-related IOCTLs for KMS devices Dmitry Baryshkov
  2026-03-04 14:57 ` Rob Clark
  2026-03-04 15:57 ` Rob Clark
@ 2026-03-05  3:25 ` Claude Code Review Bot
  2026-03-05  3:25 ` Claude Code Review Bot
  3 siblings, 0 replies; 8+ messages in thread
From: Claude Code Review Bot @ 2026-03-05  3:25 UTC (permalink / raw)
  To: dri-devel-reviews

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] = { ... }
```
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-filled gaps. `ARRAY_SIZE()` will correctly yield 9. This is fine.

**Concern 1: `MSM_INFO_GET_IOVA` and `MSM_INFO_SET_IOVA` on KMS-only devices**

The `msm_ioctl_gem_info` handler is exposed via `MSM_GEM_INFO` in the new table. Some of its sub-commands (`MSM_INFO_GET_IOVA`, `MSM_INFO_SET_IOVA`) call `msm_ioctl_gem_info_iova()` and `msm_ioctl_gem_info_set_iova()` respectively, 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 return `-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 = 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 `drm_gem_prime_import` will be used instead of `msm_gem_prime_import`. This may or may not matter depending on the GBM backend workflow (which exports from KMS and imports into the GPU driver, not the reverse). But it's worth noting 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. If kmsro already uses dumb buffers successfully for rendering on MSM, then the original commit 98f11fd1cf92 may have been intentional in pushing towards that path. The patch could benefit from clarifying which specific use case is broken — 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 seems 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 whether 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-03-05  3:25 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-04 13:34 [PATCH] drm/msm: restore GEM-related IOCTLs for KMS devices Dmitry Baryshkov
2026-03-04 14:57 ` Rob Clark
2026-03-04 14:59   ` Rob Clark
2026-03-05  1:03     ` Dmitry Baryshkov
2026-03-04 15:57 ` Rob Clark
2026-03-05  0:44   ` Dmitry Baryshkov
2026-03-05  3:25 ` Claude review: " Claude Code Review Bot
2026-03-05  3:25 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox