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

[Mihail Atanassov <mihail.atanassov@arm.com>]

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@collabora.com>
>>> ---
>>>    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@arm.com>