diff mbox series

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

Message ID 20240905111338.95714-2-mary.guillemard@collabora.com (mailing list archive)
State New, archived
Headers show
Series drm/panthor: Expose realtime group priority and allowed priorites to userspace | expand

Commit Message

Mary Guillemard Sept. 5, 2024, 11:13 a.m. UTC
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(-)

Comments

Mihail Atanassov Sept. 5, 2024, 1:54 p.m. UTC | #1
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.

>   };
>   
>   /**
Steven Price Sept. 5, 2024, 3:41 p.m. UTC | #2
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).

Steve

>>   };
>>     /**
>
Steven Price Sept. 5, 2024, 3:41 p.m. UTC | #3
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>

Reviewed-by: Steven Price <steven.price@arm.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,
>  };
>  
>  /**
Mihail Atanassov Sept. 5, 2024, 4:22 p.m. UTC | #4
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
> 
>>>    };
>>>      /**
>>
>
diff mbox series

Patch

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,
 };
 
 /**