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

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

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