diff mbox series

[3/4] drm/sched: Remove to_drm_sched_job internal helper

Message ID 20250120165240.9105-4-tvrtko.ursulin@igalia.com (mailing list archive)
State New
Headers show
Series A bit of struct drm_sched_job cleanup | expand

Commit Message

Tvrtko Ursulin Jan. 20, 2025, 4:52 p.m. UTC
The code assumes queue node is the first element in struct
drm_sched_job. Since this is not documented it can be very fragile so lets
just remove the internal helper and explicitly check for "nothing
dequeued", before converting the node to a sched job.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
---
 drivers/gpu/drm/scheduler/sched_entity.c | 18 +++++++++---------
 drivers/gpu/drm/scheduler/sched_main.c   | 10 +++++-----
 2 files changed, 14 insertions(+), 14 deletions(-)

Comments

Danilo Krummrich Jan. 20, 2025, 5:17 p.m. UTC | #1
On Mon, Jan 20, 2025 at 04:52:39PM +0000, Tvrtko Ursulin wrote:
> The code assumes queue node is the first element in struct
> drm_sched_job.

I'd add that this assumption lies in doing the NULL check after the
container_of(). Without saying that, it might not be that obvious.

> Since this is not documented it can be very fragile so lets
> just remove the internal helper and explicitly check for "nothing
> dequeued", before converting the node to a sched job.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Philipp Stanner <phasta@kernel.org>
> ---
>  drivers/gpu/drm/scheduler/sched_entity.c | 18 +++++++++---------
>  drivers/gpu/drm/scheduler/sched_main.c   | 10 +++++-----
>  2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 7c0d266a89ef..8992bb432ec6 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -30,9 +30,6 @@
>  
>  #include "gpu_scheduler_trace.h"
>  
> -#define to_drm_sched_job(sched_job)		\
> -		container_of((sched_job), struct drm_sched_job, queue_node)
> -
>  /**
>   * drm_sched_entity_init - Init a context entity used by scheduler when
>   * submit to HW ring.
> @@ -476,11 +473,14 @@ drm_sched_job_dependency(struct drm_sched_job *job,
>  struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
>  {
>  	struct drm_sched_job *sched_job;
> +	struct spsc_node *node;
>  
> -	sched_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
> -	if (!sched_job)
> +	node = spsc_queue_peek(&entity->job_queue);
> +	if (!node)
>  		return NULL;
>  
> +	sched_job = container_of(node, typeof(*sched_job), queue_node);

So, here you have this pattern for a valid used case and even twice. I think you
should add drm_sched_entity_peek_job() instead and document what the
preconditions are to be allowed to peek given it's an spsc queue.

> +
>  	while ((entity->dependency =
>  			drm_sched_job_dependency(sched_job, entity))) {
>  		trace_drm_sched_job_wait_dep(sched_job, entity->dependency);
> @@ -511,10 +511,10 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
>  	 * the timestamp of the next job, if any.
>  	 */
>  	if (drm_sched_policy == DRM_SCHED_POLICY_FIFO) {
> -		struct drm_sched_job *next;
> -
> -		next = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
> -		if (next) {
> +		node = spsc_queue_peek(&entity->job_queue);
> +		if (node) {
> +			struct drm_sched_job *next =
> +				container_of(node, typeof(*next), queue_node);
>  			struct drm_sched_rq *rq;
>  
>  			spin_lock(&entity->lock);
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index a48be16ab84f..66eee6372253 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -87,9 +87,6 @@ static struct lockdep_map drm_sched_lockdep_map = {
>  };
>  #endif
>  
> -#define to_drm_sched_job(sched_job)		\
> -		container_of((sched_job), struct drm_sched_job, queue_node)
> -
>  int drm_sched_policy = DRM_SCHED_POLICY_FIFO;
>  
>  /**
> @@ -122,11 +119,14 @@ static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
>  				struct drm_sched_entity *entity)
>  {
>  	struct drm_sched_job *s_job;
> +	struct spsc_node *node;
>  
> -	s_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
> -	if (!s_job)
> +	node = spsc_queue_peek(&entity->job_queue);
> +	if (!node)
>  		return false;
>  
> +	s_job = container_of(node, typeof(*s_job), queue_node);
> +
>  	/* If a job exceeds the credit limit, truncate it to the credit limit
>  	 * itself to guarantee forward progress.
>  	 */
> -- 
> 2.47.1
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 7c0d266a89ef..8992bb432ec6 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -30,9 +30,6 @@ 
 
 #include "gpu_scheduler_trace.h"
 
-#define to_drm_sched_job(sched_job)		\
-		container_of((sched_job), struct drm_sched_job, queue_node)
-
 /**
  * drm_sched_entity_init - Init a context entity used by scheduler when
  * submit to HW ring.
@@ -476,11 +473,14 @@  drm_sched_job_dependency(struct drm_sched_job *job,
 struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
 {
 	struct drm_sched_job *sched_job;
+	struct spsc_node *node;
 
-	sched_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
-	if (!sched_job)
+	node = spsc_queue_peek(&entity->job_queue);
+	if (!node)
 		return NULL;
 
+	sched_job = container_of(node, typeof(*sched_job), queue_node);
+
 	while ((entity->dependency =
 			drm_sched_job_dependency(sched_job, entity))) {
 		trace_drm_sched_job_wait_dep(sched_job, entity->dependency);
@@ -511,10 +511,10 @@  struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
 	 * the timestamp of the next job, if any.
 	 */
 	if (drm_sched_policy == DRM_SCHED_POLICY_FIFO) {
-		struct drm_sched_job *next;
-
-		next = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
-		if (next) {
+		node = spsc_queue_peek(&entity->job_queue);
+		if (node) {
+			struct drm_sched_job *next =
+				container_of(node, typeof(*next), queue_node);
 			struct drm_sched_rq *rq;
 
 			spin_lock(&entity->lock);
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index a48be16ab84f..66eee6372253 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -87,9 +87,6 @@  static struct lockdep_map drm_sched_lockdep_map = {
 };
 #endif
 
-#define to_drm_sched_job(sched_job)		\
-		container_of((sched_job), struct drm_sched_job, queue_node)
-
 int drm_sched_policy = DRM_SCHED_POLICY_FIFO;
 
 /**
@@ -122,11 +119,14 @@  static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
 				struct drm_sched_entity *entity)
 {
 	struct drm_sched_job *s_job;
+	struct spsc_node *node;
 
-	s_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
-	if (!s_job)
+	node = spsc_queue_peek(&entity->job_queue);
+	if (!node)
 		return false;
 
+	s_job = container_of(node, typeof(*s_job), queue_node);
+
 	/* If a job exceeds the credit limit, truncate it to the credit limit
 	 * itself to guarantee forward progress.
 	 */