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

[PATCH 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>
---
Jacopo Mondi (2):
      media: vsp1: vsp1_dl: Add usage counter
      media: vsp1: vsp1_dl: Count display lists

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

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

[PATCH 1/2] media: vsp1: vsp1_dl: Add usage counter

[Jacopo Mondi]

In order to detect invalid usage pattern such as double list_put()
calls, add a usage counter to each display list. Increment it whenever
a list is get() and decrement it when the list is put(). Warn if the
usage counter goes below 0.

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

diff --git a/drivers/media/platform/renesas/vsp1/vsp1_dl.c b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
index bb8228b19824..8a3c0274a163 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
@@ -179,6 +179,7 @@ struct vsp1_dl_cmd_pool {
  * @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_*
+ * @usage: usage counter to detect double list free
  */
 struct vsp1_dl_list {
 	struct list_head list;
@@ -198,6 +199,7 @@ struct vsp1_dl_list {
 	struct list_head chain;
 
 	unsigned int flags;
+	int usage;
 };
 
 /**
@@ -617,6 +619,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->usage++;
 	}
 
 	spin_unlock_irqrestore(&dlm->lock, flags);
@@ -657,6 +660,10 @@ static void __vsp1_dl_list_put(struct vsp1_dl_list *dl)
 	 */
 	dl->body0->num_entries = 0;
 
+	/* decrement usage count to detect invalid usage pattern. */
+	if (WARN_ON_ONCE(--dl->usage < 0))
+		dl->usage = 0;
+
 	list_add_tail(&dl->list, &dl->dlm->free);
 }
 

-- 
2.49.0

Re: [PATCH 1/2] media: vsp1: vsp1_dl: Add usage counter

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, May 29, 2025 at 06:36:30PM +0200, Jacopo Mondi wrote:
> In order to detect invalid usage pattern such as double list_put()
> calls, add a usage counter to each display list. Increment it whenever
> a list is get() and decrement it when the list is put(). Warn if the
> usage counter goes below 0.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/renesas/vsp1/vsp1_dl.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_dl.c b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> index bb8228b19824..8a3c0274a163 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> @@ -179,6 +179,7 @@ struct vsp1_dl_cmd_pool {
>   * @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_*
> + * @usage: usage counter to detect double list free
>   */
>  struct vsp1_dl_list {
>  	struct list_head list;
> @@ -198,6 +199,7 @@ struct vsp1_dl_list {
>  	struct list_head chain;
>  
>  	unsigned int flags;
> +	int usage;

If you have no objection, I'd like to include "count" in the name. This
could be usage_count, ref_count or refcount.

>  };
>  
>  /**
> @@ -617,6 +619,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->usage++;

The usage count will never go above 1, as there's not real reference
counting for display lists. Better than including "count" in the field
name, we could replace

	int usage;

with

	bool allocated;

(right after has_chain to avoid increasing the structure size).

>  	}
>  
>  	spin_unlock_irqrestore(&dlm->lock, flags);
> @@ -657,6 +660,10 @@ static void __vsp1_dl_list_put(struct vsp1_dl_list *dl)
>  	 */
>  	dl->body0->num_entries = 0;
>  
> +	/* decrement usage count to detect invalid usage pattern. */

s/decrement/Decrement/

> +	if (WARN_ON_ONCE(--dl->usage < 0))
> +		dl->usage = 0;
> +
>  	list_add_tail(&dl->list, &dl->dlm->free);
>  }
>  

-- 
Regards,

Laurent Pinchart

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

[Jacopo Mondi]

To detect invalid usage patterns of the display list helpers, store
in the display list manager the number of available display lists
when the manager is created and verify that when the display manager
is reset the same number of lists is available.

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 8a3c0274a163..5c4eeb65216f 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
@@ -214,6 +214,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: display list counter
  */
 struct vsp1_dl_manager {
 	unsigned int index;
@@ -228,6 +229,8 @@ struct vsp1_dl_manager {
 
 	struct vsp1_dl_body_pool *pool;
 	struct vsp1_dl_cmd_pool *cmdpool;
+
+	size_t list_count;
 };
 
 /* -----------------------------------------------------------------------------
@@ -1073,7 +1076,9 @@ void vsp1_dlm_setup(struct vsp1_device *vsp1)
 
 void vsp1_dlm_reset(struct vsp1_dl_manager *dlm)
 {
+	size_t dlm_list_count;
 	unsigned long flags;
+	size_t list_count;
 
 	spin_lock_irqsave(&dlm->lock, flags);
 
@@ -1081,8 +1086,13 @@ 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);
+	dlm_list_count = dlm->list_count;
+
 	spin_unlock_irqrestore(&dlm->lock, flags);
 
+	WARN_ON_ONCE(list_count != dlm_list_count);
+
 	dlm->active = NULL;
 	dlm->queued = NULL;
 	dlm->pending = NULL;
@@ -1150,6 +1160,7 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1,
 				      + sizeof(*dl->header);
 
 		list_add_tail(&dl->list, &dlm->free);
+		dlm->list_count = list_count_nodes(&dlm->free);
 	}
 
 	if (vsp1_feature(vsp1, VSP1_HAS_EXT_DL)) {

-- 
2.49.0

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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, May 29, 2025 at 06:36:31PM +0200, Jacopo Mondi wrote:
> To detect invalid usage patterns of the display list helpers, store

We can be more precise:

"To detect leaks of display lists, ..."

> in the display list manager the number of available display lists

s/available/allocated/

> when the manager is created and verify that when the display manager
> is reset the same number of lists is available.
> 
> 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 8a3c0274a163..5c4eeb65216f 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> @@ -214,6 +214,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: display list counter

"number of allocated display lists"

>   */
>  struct vsp1_dl_manager {
>  	unsigned int index;
> @@ -228,6 +229,8 @@ struct vsp1_dl_manager {
>  
>  	struct vsp1_dl_body_pool *pool;
>  	struct vsp1_dl_cmd_pool *cmdpool;
> +
> +	size_t list_count;
>  };
>  
>  /* -----------------------------------------------------------------------------
> @@ -1073,7 +1076,9 @@ void vsp1_dlm_setup(struct vsp1_device *vsp1)
>  
>  void vsp1_dlm_reset(struct vsp1_dl_manager *dlm)
>  {
> +	size_t dlm_list_count;
>  	unsigned long flags;
> +	size_t list_count;
>  
>  	spin_lock_irqsave(&dlm->lock, flags);
>  
> @@ -1081,8 +1086,13 @@ 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);
> +	dlm_list_count = dlm->list_count;

dlm->list_count is not documented as protected by the lock. I don't
think that's an oversight, as it can only be set when the dlm is
created. You can drop the dlm_list_count variable and use
dlm->list_count below.

> +
>  	spin_unlock_irqrestore(&dlm->lock, flags);
>  
> +	WARN_ON_ONCE(list_count != dlm_list_count);
> +
>  	dlm->active = NULL;
>  	dlm->queued = NULL;
>  	dlm->pending = NULL;
> @@ -1150,6 +1160,7 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1,
>  				      + sizeof(*dl->header);
>  
>  		list_add_tail(&dl->list, &dlm->free);
> +		dlm->list_count = list_count_nodes(&dlm->free);

Does this need to be done inside the loop, can't you just write

	dlm->list_count = prealloc;

after the loop ?

>  	}
>  
>  	if (vsp1_feature(vsp1, VSP1_HAS_EXT_DL)) {

-- 
Regards,

Laurent Pinchart

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

[Jacopo Mondi]

Hi Laurent

On Mon, Jun 16, 2025 at 04:31:34PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, May 29, 2025 at 06:36:31PM +0200, Jacopo Mondi wrote:
> > To detect invalid usage patterns of the display list helpers, store
>
> We can be more precise:
>
> "To detect leaks of display lists, ..."
>
> > in the display list manager the number of available display lists
>
> s/available/allocated/
>
> > when the manager is created and verify that when the display manager
> > is reset the same number of lists is available.
> >
> > 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 8a3c0274a163..5c4eeb65216f 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> > @@ -214,6 +214,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: display list counter
>
> "number of allocated display lists"
>
> >   */
> >  struct vsp1_dl_manager {
> >  	unsigned int index;
> > @@ -228,6 +229,8 @@ struct vsp1_dl_manager {
> >
> >  	struct vsp1_dl_body_pool *pool;
> >  	struct vsp1_dl_cmd_pool *cmdpool;
> > +
> > +	size_t list_count;
> >  };
> >
> >  /* -----------------------------------------------------------------------------
> > @@ -1073,7 +1076,9 @@ void vsp1_dlm_setup(struct vsp1_device *vsp1)
> >
> >  void vsp1_dlm_reset(struct vsp1_dl_manager *dlm)
> >  {
> > +	size_t dlm_list_count;
> >  	unsigned long flags;
> > +	size_t list_count;
> >
> >  	spin_lock_irqsave(&dlm->lock, flags);
> >
> > @@ -1081,8 +1086,13 @@ 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);
> > +	dlm_list_count = dlm->list_count;
>
> dlm->list_count is not documented as protected by the lock. I don't
> think that's an oversight, as it can only be set when the dlm is
> created. You can drop the dlm_list_count variable and use
> dlm->list_count below.
>
Ack

> > +
> >  	spin_unlock_irqrestore(&dlm->lock, flags);
> >
> > +	WARN_ON_ONCE(list_count != dlm_list_count);
> > +
> >  	dlm->active = NULL;
> >  	dlm->queued = NULL;
> >  	dlm->pending = NULL;
> > @@ -1150,6 +1160,7 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1,
> >  				      + sizeof(*dl->header);
> >
> >  		list_add_tail(&dl->list, &dlm->free);
> > +		dlm->list_count = list_count_nodes(&dlm->free);
>
> Does this need to be done inside the loop, can't you just write
>
> 	dlm->list_count = prealloc;
>
> after the loop ?
>

Oh indeed, this was stupid. Thanks for catching this

> >  	}
> >
> >  	if (vsp1_feature(vsp1, VSP1_HAS_EXT_DL)) {
>
> --
> Regards,
>
> Laurent Pinchart