linux-kernel - Re: [PATCH v2 3/4] drm/panfrost: Expose JM context IOCTLs to UM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <4sp5ary2xfd7yjqux4yex5vtb3gmcrhwsg5mnoeirubzf62eot@ydroedwvv6pb>
Date: Thu, 11 Sep 2025 20:34:57 +0100
From: Adrián Larumbe <adrian.larumbe@...labora.com>
To: Steven Price <steven.price@....com>
Cc: linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org, 
	Boris Brezillon <boris.brezillon@...labora.com>, kernel@...labora.com, Rob Herring <robh@...nel.org>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, 
	Simona Vetter <simona@...ll.ch>
Subject: Re: [PATCH v2 3/4] drm/panfrost: Expose JM context IOCTLs to UM

On 10.09.2025 16:42, Steven Price wrote:
> On 04/09/2025 01:08, Adrián Larumbe wrote:
> > From: Boris Brezillon <boris.brezillon@...labora.com>
> >
> > Minor revision of the driver must be bumped because this expands the
> > uAPI. On top of that, let user know the available priorities so that
> > they can create contexts with legal priority values.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@...labora.com>
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@...labora.com>
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_drv.c | 35 +++++++++++++++++++++++--
> >  1 file changed, 33 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > index 398c067457d9..02f704ec4961 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_runtime.h>
> >  #include <drm/panfrost_drm.h>
> > +#include <drm/drm_auth.h>
> >  #include <drm/drm_debugfs.h>
> >  #include <drm/drm_drv.h>
> >  #include <drm/drm_ioctl.h>
> > @@ -109,6 +110,15 @@ static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct
> >  #endif
> >  		break;
> >
> > +	case DRM_PANFROST_PARAM_ALLOWED_JM_CTX_PRIORITIES:
> > +		param->value = BIT(PANFROST_JM_CTX_PRIORITY_LOW) |
> > +			       BIT(PANFROST_JM_CTX_PRIORITY_MEDIUM);
> > +
> > +		/* High prio require CAP_SYS_NICE or DRM_MASTER */
> > +		if (capable(CAP_SYS_NICE) || drm_is_current_master(file))
>
> NIT: This is repeating the check in jm_ctx_prio_to_drm_sched_prio(). It
> would be nice to have this check in one place to ensure that the two
> cannot get out of sync.

I noticed some other drivers are performing the same check, e.g.

- amdgpu: in amdgpu_ctx_priority_permit()
- imagination: remap_priority
- panthor ...

Which makes me wonder, maybe adding a helper to drm_auth.c would be good in this case?
Anyway, that perhaps should be matter for a different patch .


> > +			param->value |= BIT(PANFROST_JM_CTX_PRIORITY_HIGH);
> > +		break;
> > +
> >  	default:
> >  		return -EINVAL;
> >  	}
> > @@ -295,8 +305,7 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
> >  			return -ENODEV;
> >  	}
> >
> > -	/* TODO: Use the default JM context until ctx management IOCTLs are exposed */
> > -	jm_ctx = panfrost_jm_ctx_from_handle(file, 0);
> > +	jm_ctx = panfrost_jm_ctx_from_handle(file, args->jm_ctx_handle);
>
> I'm not sure if this should go in this patch or the previous one, but:
> we need to add a check that the padding is 0. Personally I'd be tempted
> to put it in the previous patch and enforce that jm_ctx_handle is zeroed
> too (and drop that check here when you also remove the TODO).

Acked.

> >  	if (!jm_ctx) {
> >  		ret = -EINVAL;
> >  		goto out_put_syncout;
> > @@ -547,6 +556,24 @@ static int panfrost_ioctl_set_label_bo(struct drm_device *ddev, void *data,
> >  	return ret;
> >  }
> >
> > +static int panfrost_ioctl_jm_ctx_create(struct drm_device *dev, void *data,
> > +					struct drm_file *file)
> > +{
> > +	return panfrost_jm_ctx_create(file, data);
> > +}
> > +
> > +static int panfrost_ioctl_jm_ctx_destroy(struct drm_device *dev, void *data,
> > +					 struct drm_file *file)
> > +{
> > +	const struct drm_panfrost_jm_ctx_destroy *args = data;
> > +
> > +	/* We can't destroy the default context created when the file is opened. */
> > +	if (!args->handle)
> > +		return -EINVAL;
>
> Also need a check for padding being non-zero.
>
> Thanks,
> Steve
>
> > +
> > +	return panfrost_jm_ctx_destroy(file, args->handle);
> > +}
> > +
> >  int panfrost_unstable_ioctl_check(void)
> >  {
> >  	if (!unstable_ioctls)
> > @@ -614,6 +641,8 @@ static const struct drm_ioctl_desc panfrost_drm_driver_ioctls[] = {
> >  	PANFROST_IOCTL(PERFCNT_DUMP,	perfcnt_dump,	DRM_RENDER_ALLOW),
> >  	PANFROST_IOCTL(MADVISE,		madvise,	DRM_RENDER_ALLOW),
> >  	PANFROST_IOCTL(SET_LABEL_BO,	set_label_bo,	DRM_RENDER_ALLOW),
> > +	PANFROST_IOCTL(JM_CTX_CREATE,	jm_ctx_create,	DRM_RENDER_ALLOW),
> > +	PANFROST_IOCTL(JM_CTX_DESTROY,	jm_ctx_destroy,	DRM_RENDER_ALLOW),
> >  };
> >
> >  static void panfrost_gpu_show_fdinfo(struct panfrost_device *pfdev,
> > @@ -710,6 +739,8 @@ static void panfrost_debugfs_init(struct drm_minor *minor)
> >   * - 1.3 - adds JD_REQ_CYCLE_COUNT job requirement for SUBMIT
> >   *       - adds SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY queries
> >   * - 1.4 - adds SET_LABEL_BO
> > + * - 1.5 - adds JM_CTX_{CREATE,DESTROY} ioctls and extend SUBMIT to allow
> > + *	   context creation with configurable priorities/affinity
> >   */
> >  static const struct drm_driver panfrost_drm_driver = {
> >  	.driver_features	= DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ,

Adrian Larumbe