On 10.09.2025 16:42, Steven Price wrote:
> On 04/09/2025 01:08, Adrián Larumbe wrote:
> > From: Boris Brezillon <boris.brezil...@collabora.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.brezil...@collabora.com>
> > Signed-off-by: Adrián Larumbe <adrian.laru...@collabora.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
