[PATCH v2 0/2] media: vsp1: Detect display list wrong usage patterns

[PATCH v2 0/2] media: vsp1: Detect display list wrong usage patterns

[Jacopo Mondi]

The VSP1 library driver offers support for programming the VSP
using display lists.

Display lists are created in a pool and users can get them and use them
to program the VSP. Once done the display list shall be returned to
the display list pool.

The correct management of the display list pool is left to the user
of the helpers, and it's helpful to add a few checks to detect invalid
usage patterns, such as a double release or a non-returned display list.

Add two counters to detect double releases of a display list, and a
counter to the display list to make sure that once it is reset all the
display lists have been returned.

Tested with vsp-tests
170 tests: 165 passed, 0 failed, 5 skipped

However I got a (hopefully unrelated) warning:

[  795.547528] [<000000007d841fd6>] vsp1_irq_handler
[  795.552448] Disabling IRQ #43
[  795.653324] vsp1 fea20000.vsp: Underrun occurred at WPF1 (total underruns 1)

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
---
Changes in v2:
- Use a boolean to keep track of DL state
- minor changes
- Link to v1: https://lore.kernel.org/r/20250529-vsp1_dl_list_count-v1-0-40c6d0e20592@ideasonboard.com

---
Jacopo Mondi (2):
      media: vsp1: vsp1_dl: Detect double list release
      media: vsp1: vsp1_dl: Count display lists

 drivers/media/platform/renesas/vsp1/vsp1_dl.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
---
base-commit: 4d2c3d70799f5eb210003613766bbd113bbebc1a
change-id: 20250529-vsp1_dl_list_count-156d27a2d5b1

Best regards,
-- 
Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>

[PATCH v2 1/2] media: vsp1: vsp1_dl: Detect double list release

[Jacopo Mondi]

In order to detect invalid usage pattern such as double list_put()
calls, add an 'allocated' flag to each display list. Set the flag
whenever a list is get() and clear it when the list is put(). Warn if a
list not marked as allocated is returned to the pool of available
display lists.

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
---
 drivers/media/platform/renesas/vsp1/vsp1_dl.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/platform/renesas/vsp1/vsp1_dl.c b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
index bb8228b198249943399719b5f37c337fc43eed5b..18617cbb168703b851a9b437fa62f18425934c68 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
@@ -176,6 +176,7 @@ struct vsp1_dl_cmd_pool {
  * @bodies: list of extra display list bodies
  * @pre_cmd: pre command to be issued through extended dl header
  * @post_cmd: post command to be issued through extended dl header
+ * @allocated: flag to detect double list release
  * @has_chain: if true, indicates that there's a partition chain
  * @chain: entry in the display list partition chain
  * @flags: display list flags, a combination of VSP1_DL_FRAME_END_*
@@ -194,6 +195,8 @@ struct vsp1_dl_list {
 	struct vsp1_dl_ext_cmd *pre_cmd;
 	struct vsp1_dl_ext_cmd *post_cmd;
 
+	bool allocated;
+
 	bool has_chain;
 	struct list_head chain;
 
@@ -617,6 +620,7 @@ struct vsp1_dl_list *vsp1_dl_list_get(struct vsp1_dl_manager *dlm)
 		 * display list can assert list_empty() if it is not in a chain.
 		 */
 		INIT_LIST_HEAD(&dl->chain);
+		dl->allocated = true;
 	}
 
 	spin_unlock_irqrestore(&dlm->lock, flags);
@@ -657,6 +661,13 @@ static void __vsp1_dl_list_put(struct vsp1_dl_list *dl)
 	 */
 	dl->body0->num_entries = 0;
 
+	/*
+	 * Return the display list to the 'free' pool. If the list had already
+	 * been returned be loud about it.
+	 */
+	WARN_ON_ONCE(!dl->allocated);
+	dl->allocated = false;
+
 	list_add_tail(&dl->list, &dl->dlm->free);
 }
 

-- 
2.49.0

Re: [PATCH v2 1/2] media: vsp1: vsp1_dl: Detect double list release

[Laurent Pinchart]

Hi Jacopo,

On Mon, Jun 16, 2025 at 06:30:37PM +0200, Jacopo Mondi wrote:
> In order to detect invalid usage pattern such as double list_put()
> calls, add an 'allocated' flag to each display list. Set the flag
> whenever a list is get() and clear it when the list is put(). Warn if a
> list not marked as allocated is returned to the pool of available
> display lists.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/media/platform/renesas/vsp1/vsp1_dl.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_dl.c b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> index bb8228b198249943399719b5f37c337fc43eed5b..18617cbb168703b851a9b437fa62f18425934c68 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> @@ -176,6 +176,7 @@ struct vsp1_dl_cmd_pool {
>   * @bodies: list of extra display list bodies
>   * @pre_cmd: pre command to be issued through extended dl header
>   * @post_cmd: post command to be issued through extended dl header
> + * @allocated: flag to detect double list release
>   * @has_chain: if true, indicates that there's a partition chain
>   * @chain: entry in the display list partition chain
>   * @flags: display list flags, a combination of VSP1_DL_FRAME_END_*
> @@ -194,6 +195,8 @@ struct vsp1_dl_list {
>  	struct vsp1_dl_ext_cmd *pre_cmd;
>  	struct vsp1_dl_ext_cmd *post_cmd;
>  
> +	bool allocated;
> +
>  	bool has_chain;
>  	struct list_head chain;
>  
> @@ -617,6 +620,7 @@ struct vsp1_dl_list *vsp1_dl_list_get(struct vsp1_dl_manager *dlm)
>  		 * display list can assert list_empty() if it is not in a chain.
>  		 */
>  		INIT_LIST_HEAD(&dl->chain);
> +		dl->allocated = true;
>  	}
>  
>  	spin_unlock_irqrestore(&dlm->lock, flags);
> @@ -657,6 +661,13 @@ static void __vsp1_dl_list_put(struct vsp1_dl_list *dl)
>  	 */
>  	dl->body0->num_entries = 0;
>  
> +	/*
> +	 * Return the display list to the 'free' pool. If the list had already
> +	 * been returned be loud about it.
> +	 */
> +	WARN_ON_ONCE(!dl->allocated);
> +	dl->allocated = false;
> +
>  	list_add_tail(&dl->list, &dl->dlm->free);
>  }
>  

-- 
Regards,

Laurent Pinchart

[PATCH v2 2/2] media: vsp1: vsp1_dl: Count display lists

[Jacopo Mondi]

To detect leaks of display lists, store in the display list manager the
number of allocated display lists when the manager is created and verify
that when the display manager is reset the same number of lists is
allocated.

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
---
 drivers/media/platform/renesas/vsp1/vsp1_dl.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/media/platform/renesas/vsp1/vsp1_dl.c b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
index 18617cbb168703b851a9b437fa62f18425934c68..3713730c6ad8739935851e4da464fc8f23da6180 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
@@ -215,6 +215,7 @@ struct vsp1_dl_list {
  * @pending: list waiting to be queued to the hardware
  * @pool: body pool for the display list bodies
  * @cmdpool: commands pool for extended display list
+ * @list_count: number of allocated display lists
  */
 struct vsp1_dl_manager {
 	unsigned int index;
@@ -229,6 +230,8 @@ struct vsp1_dl_manager {
 
 	struct vsp1_dl_body_pool *pool;
 	struct vsp1_dl_cmd_pool *cmdpool;
+
+	size_t list_count;
 };
 
 /* -----------------------------------------------------------------------------
@@ -1078,6 +1081,7 @@ void vsp1_dlm_setup(struct vsp1_device *vsp1)
 void vsp1_dlm_reset(struct vsp1_dl_manager *dlm)
 {
 	unsigned long flags;
+	size_t list_count;
 
 	spin_lock_irqsave(&dlm->lock, flags);
 
@@ -1085,8 +1089,11 @@ void vsp1_dlm_reset(struct vsp1_dl_manager *dlm)
 	__vsp1_dl_list_put(dlm->queued);
 	__vsp1_dl_list_put(dlm->pending);
 
+	list_count = list_count_nodes(&dlm->free);
 	spin_unlock_irqrestore(&dlm->lock, flags);
 
+	WARN_ON_ONCE(list_count != dlm->list_count);
+
 	dlm->active = NULL;
 	dlm->queued = NULL;
 	dlm->pending = NULL;
@@ -1155,6 +1162,7 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1,
 
 		list_add_tail(&dl->list, &dlm->free);
 	}
+	dlm->list_count = prealloc;
 
 	if (vsp1_feature(vsp1, VSP1_HAS_EXT_DL)) {
 		dlm->cmdpool = vsp1_dl_cmd_pool_create(vsp1,

-- 
2.49.0

Re: [PATCH v2 2/2] media: vsp1: vsp1_dl: Count display lists

[Laurent Pinchart]

On Mon, Jun 16, 2025 at 06:30:38PM +0200, Jacopo Mondi wrote:
> To detect leaks of display lists, store in the display list manager the
> number of allocated display lists when the manager is created and verify
> that when the display manager is reset the same number of lists is
> allocated.

"is available in the free list".

> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>

I'll update the above when applying.

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/media/platform/renesas/vsp1/vsp1_dl.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_dl.c b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> index 18617cbb168703b851a9b437fa62f18425934c68..3713730c6ad8739935851e4da464fc8f23da6180 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> @@ -215,6 +215,7 @@ struct vsp1_dl_list {
>   * @pending: list waiting to be queued to the hardware
>   * @pool: body pool for the display list bodies
>   * @cmdpool: commands pool for extended display list
> + * @list_count: number of allocated display lists
>   */
>  struct vsp1_dl_manager {
>  	unsigned int index;
> @@ -229,6 +230,8 @@ struct vsp1_dl_manager {
>  
>  	struct vsp1_dl_body_pool *pool;
>  	struct vsp1_dl_cmd_pool *cmdpool;
> +
> +	size_t list_count;
>  };
>  
>  /* -----------------------------------------------------------------------------
> @@ -1078,6 +1081,7 @@ void vsp1_dlm_setup(struct vsp1_device *vsp1)
>  void vsp1_dlm_reset(struct vsp1_dl_manager *dlm)
>  {
>  	unsigned long flags;
> +	size_t list_count;
>  
>  	spin_lock_irqsave(&dlm->lock, flags);
>  
> @@ -1085,8 +1089,11 @@ void vsp1_dlm_reset(struct vsp1_dl_manager *dlm)
>  	__vsp1_dl_list_put(dlm->queued);
>  	__vsp1_dl_list_put(dlm->pending);
>  
> +	list_count = list_count_nodes(&dlm->free);
>  	spin_unlock_irqrestore(&dlm->lock, flags);
>  
> +	WARN_ON_ONCE(list_count != dlm->list_count);
> +
>  	dlm->active = NULL;
>  	dlm->queued = NULL;
>  	dlm->pending = NULL;
> @@ -1155,6 +1162,7 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1,
>  
>  		list_add_tail(&dl->list, &dlm->free);
>  	}
> +	dlm->list_count = prealloc;
>  
>  	if (vsp1_feature(vsp1, VSP1_HAS_EXT_DL)) {
>  		dlm->cmdpool = vsp1_dl_cmd_pool_create(vsp1,

-- 
Regards,

Laurent Pinchart