Re: [PATCH 1/2] drm/panthor: Add PANTHOR_GROUP_PRIORITY_REALTIME group priority

[Mihail Atanassov]

On 05/09/2024 16:41, Steven Price wrote:
> Hi Mihail,
>
> On 05/09/2024 14:54, Mihail Atanassov wrote:
> > Hi Mary,
> >
> > On 05/09/2024 12:13, Mary Guillemard wrote:
> > > This adds a new value to drm_panthor_group_priority exposing the
> > > realtime priority to userspace.
> > >
> > > This is required to implement NV_context_priority_realtime in Mesa.
> > >
> > > Signed-off-by: Mary Guillemard <mary.guillemard@xxxxxxxxxxxxx>
> > > ---
> > >    drivers/gpu/drm/panthor/panthor_drv.c   | 2 +-
> > >    drivers/gpu/drm/panthor/panthor_sched.c | 2 --
> > >    include/uapi/drm/panthor_drm.h          | 7 +++++++
> > >    3 files changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c
> > > b/drivers/gpu/drm/panthor/panthor_drv.c
> > > index 0caf9e9a8c45..7b1db2adcb4c 100644
> > > --- a/drivers/gpu/drm/panthor/panthor_drv.c
> > > +++ b/drivers/gpu/drm/panthor/panthor_drv.c
> > > @@ -1041,7 +1041,7 @@ static int group_priority_permit(struct drm_file
> > > *file,
> > >                     u8 priority)
> > >    {
> > >        /* Ensure that priority is valid */
> > > -    if (priority > PANTHOR_GROUP_PRIORITY_HIGH)
> > > +    if (priority > PANTHOR_GROUP_PRIORITY_REALTIME)
> > >            return -EINVAL;
> > >          /* Medium priority and below are always allowed */
> > > diff --git a/drivers/gpu/drm/panthor/panthor_sched.c
> > > b/drivers/gpu/drm/panthor/panthor_sched.c
> > > index 91a31b70c037..86908ada7335 100644
> > > --- a/drivers/gpu/drm/panthor/panthor_sched.c
> > > +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> > > @@ -137,8 +137,6 @@ enum panthor_csg_priority {
> > >         * non-real-time groups. When such a group becomes executable,
> > >         * it will evict the group with the lowest non-rt priority if
> > >         * there's no free group slot available.
> > > -     *
> > > -     * Currently not exposed to userspace.
> > >         */
> > >        PANTHOR_CSG_PRIORITY_RT,
> > >    diff --git a/include/uapi/drm/panthor_drm.h
> > > b/include/uapi/drm/panthor_drm.h
> > > index 1fd8473548ac..011a555e4674 100644
> > > --- a/include/uapi/drm/panthor_drm.h
> > > +++ b/include/uapi/drm/panthor_drm.h
> > > @@ -720,6 +720,13 @@ enum drm_panthor_group_priority {
> > >         * Requires CAP_SYS_NICE or DRM_MASTER.
> > >         */
> > >        PANTHOR_GROUP_PRIORITY_HIGH,
> > > +
> > > +    /**
> > > +     * @PANTHOR_GROUP_PRIORITY_REALTIME: Realtime priority group.
> > > +     *
> > > +     * Requires CAP_SYS_NICE or DRM_MASTER.
> > > +     */
> > > +    PANTHOR_GROUP_PRIORITY_REALTIME,
> >
> > This is a uapi change, but doesn't have a corresponding driver version
> > bump in the same patch. You also document the addition of this enum
> > value in the next patch, which also is a tad wonky. If you reversed the
> > order of the patches, they'd make more sense IMO.
>
> You can't reverse the order because then the version bump would be
> before all the features were present. Generally we put the version bump
> at the end of a patch series because it's indicating to user space that
> the new features can be used. This way round if a bisect lands in the
> middle of the series then the new priority is there but won't be used
> because user space won't know about it (which is fine).

Ack.

> Steve
>
> > >    };
> > >      /**

--
Mihail Atanassov <mihail.atanassov@xxxxxxx>