[v7,08/16] intel-ipu3: css: Add dma buff pool utility functions

Commit Message

Zhi, Yong Oct. 29, 2018, 10:23 p.m. UTC

The pools are used to store previous parameters set by
user with the parameter queue. Due to pipelining,
there needs to be multiple sets (up to four)
of parameters which are queued in a host-to-sp queue.

Signed-off-by: Yong Zhi <[e-mail]>
---
 drivers/media/pci/intel/ipu3/ipu3-css-pool.c | 136 +++++++++++++++++++++++++++
 drivers/media/pci/intel/ipu3/ipu3-css-pool.h |  56 +++++++++++
 2 files changed, 192 insertions(+)
 create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-pool.c
 create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-pool.h

Comments

Sakari Ailus Nov. 8, 2018, 3:36 p.m. UTC | #1

Hi Yong,

On Mon, Oct 29, 2018 at 03:23:02PM -0700, Yong Zhi wrote:
> The pools are used to store previous parameters set by
> user with the parameter queue. Due to pipelining,
> there needs to be multiple sets (up to four)
> of parameters which are queued in a host-to-sp queue.
> 
> Signed-off-by: Yong Zhi <[e-mail]>
> ---
>  drivers/media/pci/intel/ipu3/ipu3-css-pool.c | 136 +++++++++++++++++++++++++++
>  drivers/media/pci/intel/ipu3/ipu3-css-pool.h |  56 +++++++++++
>  2 files changed, 192 insertions(+)
>  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-pool.c
>  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-css-pool.c b/drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> new file mode 100644
> index 0000000..eab41c3
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> @@ -0,0 +1,136 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2018 Intel Corporation
> +
> +#include <linux/device.h>
> +
> +#include "ipu3.h"
> +#include "ipu3-css-pool.h"
> +#include "ipu3-dmamap.h"
> +
> +int ipu3_css_dma_buffer_resize(struct imgu_device *imgu,
> +			       struct ipu3_css_map *map, size_t size)
> +{
> +	if (map->size < size && map->vaddr) {
> +		dev_warn(&imgu->pci_dev->dev, "dma buf resized from %zu to %zu",
> +			 map->size, size);
> +
> +		ipu3_dmamap_free(imgu, map);
> +		if (!ipu3_dmamap_alloc(imgu, map, size))
> +			return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +void ipu3_css_pool_cleanup(struct imgu_device *imgu, struct ipu3_css_pool *pool)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < IPU3_CSS_POOL_SIZE; i++)
> +		ipu3_dmamap_free(imgu, &pool->entry[i].param);
> +}
> +
> +int ipu3_css_pool_init(struct imgu_device *imgu, struct ipu3_css_pool *pool,
> +		       size_t size)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < IPU3_CSS_POOL_SIZE; i++) {
> +		/*
> +		 * entry[i].framenum is initialized to INT_MIN so that
> +		 * ipu3_css_pool_check() can treat it as usesable slot.
> +		 */
> +		pool->entry[i].framenum = INT_MIN;
> +
> +		if (size == 0) {
> +			pool->entry[i].param.vaddr = NULL;
> +			continue;
> +		}
> +
> +		if (!ipu3_dmamap_alloc(imgu, &pool->entry[i].param, size))
> +			goto fail;
> +	}
> +
> +	pool->last = IPU3_CSS_POOL_SIZE;
> +
> +	return 0;
> +
> +fail:
> +	ipu3_css_pool_cleanup(imgu, pool);
> +	return -ENOMEM;
> +}
> +
> +/*
> + * Check that the following call to pool_get succeeds.
> + * Return negative on error.
> + */
> +static int ipu3_css_pool_check(struct ipu3_css_pool *pool, long framenum)
> +{
> +	/* Get the oldest entry */
> +	int n = (pool->last + 1) % IPU3_CSS_POOL_SIZE;
> +	long diff = framenum - pool->entry[n].framenum;
> +
> +	/* if framenum wraps around and becomes smaller than entry n */
> +	if (diff < 0)
> +		diff += LONG_MAX;

Have you tested the wrap-around? As a result, the value of the diff is
between -1 and LONG_MAX - 1 (without considering more than just the two
lines above). Is that intended?

You seem to be using different types for the frame number; sometimes int,
sometimes long. Could you align that, preferrably to an unsigned type? u32
would probably be a sound choice.

The entry (index to pool->entry array) should be unsigned as well.

> +
> +	/*
> +	 * pool->entry[n].framenum stores the frame number where that
> +	 * entry was allocated. If that was allocated more than POOL_SIZE
> +	 * frames back, it is old enough that we know it is no more in
> +	 * use by firmware.
> +	 */
> +	if (diff > IPU3_CSS_POOL_SIZE)
> +		return n;
> +
> +	return -ENOSPC;
> +}
> +
> +/*
> + * Allocate a new parameter from pool at frame number `framenum'.
> + * Release the oldest entry in the pool to make space for the new entry.
> + * Return negative on error.
> + */
> +int ipu3_css_pool_get(struct ipu3_css_pool *pool, long framenum)
> +{
> +	int n = ipu3_css_pool_check(pool, framenum);
> +
> +	if (n < 0)
> +		return n;
> +
> +	pool->entry[n].framenum = framenum;
> +	pool->last = n;
> +
> +	return n;
> +}
> +
> +/*
> + * Undo, for all practical purposes, the effect of pool_get().
> + */
> +void ipu3_css_pool_put(struct ipu3_css_pool *pool)
> +{
> +	pool->entry[pool->last].framenum = INT_MIN;
> +	pool->last = (pool->last + IPU3_CSS_POOL_SIZE - 1) % IPU3_CSS_POOL_SIZE;
> +}
> +
> +/**
> + * ipu3_css_pool_last - Retrieve the nth pool entry from last
> + *
> + * @pool: a pointer to &struct ipu3_css_pool.
> + * @n: the distance to the last index.
> + *
> + * Return: The nth entry from last or null map to indicate no frame stored.
> + */
> +const struct ipu3_css_map *
> +ipu3_css_pool_last(struct ipu3_css_pool *pool, unsigned int n)
> +{
> +	static const struct ipu3_css_map null_map = { 0 };
> +	int i = (pool->last + IPU3_CSS_POOL_SIZE - n) % IPU3_CSS_POOL_SIZE;
> +
> +	WARN_ON(n >= IPU3_CSS_POOL_SIZE);
> +
> +	if (pool->entry[i].framenum < 0)
> +		return &null_map;
> +
> +	return &pool->entry[i].param;
> +}
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-css-pool.h b/drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> new file mode 100644
> index 0000000..71e48d1
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2018 Intel Corporation */
> +
> +#ifndef __IPU3_UTIL_H
> +#define __IPU3_UTIL_H
> +
> +struct device;
> +struct imgu_device;
> +
> +#define IPU3_CSS_POOL_SIZE		4
> +
> +/**
> + * ipu3_css_map - store DMA mapping info for buffer
> + *
> + * @size:		size of the buffer in bytes.
> + * @vaddr:		kernel virtual address.
> + * @daddr:		iova dma address to access IPU3.
> + * @vma:		private, a pointer to &struct vm_struct,
> + *			used for ipu3_dmamap_free.
> + */
> +struct ipu3_css_map {
> +	size_t size;
> +	void *vaddr;
> +	dma_addr_t daddr;
> +	struct vm_struct *vma;
> +};
> +
> +/**
> + * ipu3_css_pool - circular buffer pool definition
> + *
> + * @entry:		array with IPU3_CSS_POOL_SIZE elements.
> + * @entry.param:	a &struct ipu3_css_map for storing the mem mapping.
> + * @entry.framenum:	the css frame number, used to determine if the entry
> + *			is old enough to be recycled.
> + * @last:		write pointer, initialized to IPU3_CSS_POOL_SIZE.
> + */
> +struct ipu3_css_pool {
> +	struct {
> +		struct ipu3_css_map param;
> +		long framenum;
> +	} entry[IPU3_CSS_POOL_SIZE];
> +	unsigned int last; /* Latest entry */
> +};
> +
> +int ipu3_css_dma_buffer_resize(struct imgu_device *imgu,
> +			       struct ipu3_css_map *map, size_t size);
> +void ipu3_css_pool_cleanup(struct imgu_device *imgu,
> +			   struct ipu3_css_pool *pool);
> +int ipu3_css_pool_init(struct imgu_device *imgu, struct ipu3_css_pool *pool,
> +		       size_t size);
> +int ipu3_css_pool_get(struct ipu3_css_pool *pool, long framenum);
> +void ipu3_css_pool_put(struct ipu3_css_pool *pool);
> +const struct ipu3_css_map *ipu3_css_pool_last(struct ipu3_css_pool *pool,
> +					      unsigned int last);
> +
> +#endif

Zhi, Yong Nov. 9, 2018, 11:16 p.m. UTC | #2

Hi, Sakari,

> -----Original Message-----
> From: Sakari Ailus [mailto:[e-mail]]
> Sent: Thursday, November 8, 2018 9:36 AM
> To: Zhi, Yong <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Mani, Rajmohan
> <[e-mail]>; Zheng, Jian Xu <[e-mail]>; Hu,
> Jerry W <[e-mail]>; Toivonen, Tuukka
> <[e-mail]>; Qiu, Tian Shu <[e-mail]>; Cao,
> Bingbu <[e-mail]>
> Subject: Re: [PATCH v7 08/16] intel-ipu3: css: Add dma buff pool utility
> functions
> 
> Hi Yong,
> 
> On Mon, Oct 29, 2018 at 03:23:02PM -0700, Yong Zhi wrote:
> > The pools are used to store previous parameters set by user with the
> > parameter queue. Due to pipelining, there needs to be multiple sets
> > (up to four) of parameters which are queued in a host-to-sp queue.
> >
> > Signed-off-by: Yong Zhi <[e-mail]>
> > ---
> >  drivers/media/pci/intel/ipu3/ipu3-css-pool.c | 136
> > +++++++++++++++++++++++++++
> > drivers/media/pci/intel/ipu3/ipu3-css-pool.h |  56 +++++++++++
> >  2 files changed, 192 insertions(+)
> >  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> >  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> > b/drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> > new file mode 100644
> > index 0000000..eab41c3
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> > @@ -0,0 +1,136 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (C) 2018 Intel Corporation
> > +
> > +#include <linux/device.h>
> > +
> > +#include "ipu3.h"
> > +#include "ipu3-css-pool.h"
> > +#include "ipu3-dmamap.h"
> > +
> > +int ipu3_css_dma_buffer_resize(struct imgu_device *imgu,
> > +			       struct ipu3_css_map *map, size_t size) {
> > +	if (map->size < size && map->vaddr) {
> > +		dev_warn(&imgu->pci_dev->dev, "dma buf resized from %zu
> to %zu",
> > +			 map->size, size);
> > +
> > +		ipu3_dmamap_free(imgu, map);
> > +		if (!ipu3_dmamap_alloc(imgu, map, size))
> > +			return -ENOMEM;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +void ipu3_css_pool_cleanup(struct imgu_device *imgu, struct
> > +ipu3_css_pool *pool) {
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < IPU3_CSS_POOL_SIZE; i++)
> > +		ipu3_dmamap_free(imgu, &pool->entry[i].param); }
> > +
> > +int ipu3_css_pool_init(struct imgu_device *imgu, struct ipu3_css_pool
> *pool,
> > +		       size_t size)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < IPU3_CSS_POOL_SIZE; i++) {
> > +		/*
> > +		 * entry[i].framenum is initialized to INT_MIN so that
> > +		 * ipu3_css_pool_check() can treat it as usesable slot.
> > +		 */
> > +		pool->entry[i].framenum = INT_MIN;
> > +
> > +		if (size == 0) {
> > +			pool->entry[i].param.vaddr = NULL;
> > +			continue;
> > +		}
> > +
> > +		if (!ipu3_dmamap_alloc(imgu, &pool->entry[i].param, size))
> > +			goto fail;
> > +	}
> > +
> > +	pool->last = IPU3_CSS_POOL_SIZE;
> > +
> > +	return 0;
> > +
> > +fail:
> > +	ipu3_css_pool_cleanup(imgu, pool);
> > +	return -ENOMEM;
> > +}
> > +
> > +/*
> > + * Check that the following call to pool_get succeeds.
> > + * Return negative on error.
> > + */
> > +static int ipu3_css_pool_check(struct ipu3_css_pool *pool, long
> > +framenum) {
> > +	/* Get the oldest entry */
> > +	int n = (pool->last + 1) % IPU3_CSS_POOL_SIZE;
> > +	long diff = framenum - pool->entry[n].framenum;
> > +
> > +	/* if framenum wraps around and becomes smaller than entry n */
> > +	if (diff < 0)
> > +		diff += LONG_MAX;
> 
> Have you tested the wrap-around? As a result, the value of the diff is
> between -1 and LONG_MAX - 1 (without considering more than just the two
> lines above). Is that intended?
> 

Yes, I simulated wrap-around using a smaller limit in v5.

> You seem to be using different types for the frame number; sometimes int,
> sometimes long. Could you align that, preferrably to an unsigned type? u32
> would probably be a sound choice.
> 

Will use u32 at places except entry.framenum, which is initialized to INT_MIN. This is because the frame is counted from 0 at stream start, and the entry.framenum must be smaller enough for the ipu3_css_pool_check() to not return -ENOSPC.

> The entry (index to pool->entry array) should be unsigned as well.

Ack, will refactor the code a little to make this function cleaner.

Thanks,

Yong
> 
> > +
> > +	/*
> > +	 * pool->entry[n].framenum stores the frame number where that
> > +	 * entry was allocated. If that was allocated more than POOL_SIZE
> > +	 * frames back, it is old enough that we know it is no more in
> > +	 * use by firmware.
> > +	 */
> > +	if (diff > IPU3_CSS_POOL_SIZE)
> > +		return n;
> > +
> > +	return -ENOSPC;
> > +}
> > +
> > +/*
> > + * Allocate a new parameter from pool at frame number `framenum'.
> > + * Release the oldest entry in the pool to make space for the new entry.
> > + * Return negative on error.
> > + */
> > +int ipu3_css_pool_get(struct ipu3_css_pool *pool, long framenum) {
> > +	int n = ipu3_css_pool_check(pool, framenum);
> > +
> > +	if (n < 0)
> > +		return n;
> > +
> > +	pool->entry[n].framenum = framenum;
> > +	pool->last = n;
> > +
> > +	return n;
> > +}
> > +
> > +/*
> > + * Undo, for all practical purposes, the effect of pool_get().
> > + */
> > +void ipu3_css_pool_put(struct ipu3_css_pool *pool) {
> > +	pool->entry[pool->last].framenum = INT_MIN;
> > +	pool->last = (pool->last + IPU3_CSS_POOL_SIZE - 1) %
> > +IPU3_CSS_POOL_SIZE; }
> > +
> > +/**
> > + * ipu3_css_pool_last - Retrieve the nth pool entry from last
> > + *
> > + * @pool: a pointer to &struct ipu3_css_pool.
> > + * @n: the distance to the last index.
> > + *
> > + * Return: The nth entry from last or null map to indicate no frame stored.
> > + */
> > +const struct ipu3_css_map *
> > +ipu3_css_pool_last(struct ipu3_css_pool *pool, unsigned int n) {
> > +	static const struct ipu3_css_map null_map = { 0 };
> > +	int i = (pool->last + IPU3_CSS_POOL_SIZE - n) % IPU3_CSS_POOL_SIZE;
> > +
> > +	WARN_ON(n >= IPU3_CSS_POOL_SIZE);
> > +
> > +	if (pool->entry[i].framenum < 0)
> > +		return &null_map;
> > +
> > +	return &pool->entry[i].param;
> > +}
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> > b/drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> > new file mode 100644
> > index 0000000..71e48d1
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> > @@ -0,0 +1,56 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright (C) 2018 Intel Corporation */
> > +
> > +#ifndef __IPU3_UTIL_H
> > +#define __IPU3_UTIL_H
> > +
> > +struct device;
> > +struct imgu_device;
> > +
> > +#define IPU3_CSS_POOL_SIZE		4
> > +
> > +/**
> > + * ipu3_css_map - store DMA mapping info for buffer
> > + *
> > + * @size:		size of the buffer in bytes.
> > + * @vaddr:		kernel virtual address.
> > + * @daddr:		iova dma address to access IPU3.
> > + * @vma:		private, a pointer to &struct vm_struct,
> > + *			used for ipu3_dmamap_free.
> > + */
> > +struct ipu3_css_map {
> > +	size_t size;
> > +	void *vaddr;
> > +	dma_addr_t daddr;
> > +	struct vm_struct *vma;
> > +};
> > +
> > +/**
> > + * ipu3_css_pool - circular buffer pool definition
> > + *
> > + * @entry:		array with IPU3_CSS_POOL_SIZE elements.
> > + * @entry.param:	a &struct ipu3_css_map for storing the mem
> mapping.
> > + * @entry.framenum:	the css frame number, used to determine if
> the entry
> > + *			is old enough to be recycled.
> > + * @last:		write pointer, initialized to IPU3_CSS_POOL_SIZE.
> > + */
> > +struct ipu3_css_pool {
> > +	struct {
> > +		struct ipu3_css_map param;
> > +		long framenum;
> > +	} entry[IPU3_CSS_POOL_SIZE];
> > +	unsigned int last; /* Latest entry */ };
> > +
> > +int ipu3_css_dma_buffer_resize(struct imgu_device *imgu,
> > +			       struct ipu3_css_map *map, size_t size); void
> > +ipu3_css_pool_cleanup(struct imgu_device *imgu,
> > +			   struct ipu3_css_pool *pool);
> > +int ipu3_css_pool_init(struct imgu_device *imgu, struct ipu3_css_pool
> *pool,
> > +		       size_t size);
> > +int ipu3_css_pool_get(struct ipu3_css_pool *pool, long framenum);
> > +void ipu3_css_pool_put(struct ipu3_css_pool *pool); const struct
> > +ipu3_css_map *ipu3_css_pool_last(struct ipu3_css_pool *pool,
> > +					      unsigned int last);
> > +
> > +#endif
> 
> --
> Regards,
> 
> Sakari Ailus
> [e-mail]

Sakari Ailus Nov. 12, 2018, 9:21 a.m. UTC | #3

Hi Yong,

On Fri, Nov 09, 2018 at 11:16:44PM +0000, Zhi, Yong wrote:
> Hi, Sakari,
> 
> > -----Original Message-----
> > From: Sakari Ailus [mailto:[e-mail]]
> > Sent: Thursday, November 8, 2018 9:36 AM
> > To: Zhi, Yong <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; Mani, Rajmohan
> > <[e-mail]>; Zheng, Jian Xu <[e-mail]>; Hu,
> > Jerry W <[e-mail]>; Toivonen, Tuukka
> > <[e-mail]>; Qiu, Tian Shu <[e-mail]>; Cao,
> > Bingbu <[e-mail]>
> > Subject: Re: [PATCH v7 08/16] intel-ipu3: css: Add dma buff pool utility
> > functions
> > 
> > Hi Yong,
> > 
> > On Mon, Oct 29, 2018 at 03:23:02PM -0700, Yong Zhi wrote:
> > > The pools are used to store previous parameters set by user with the
> > > parameter queue. Due to pipelining, there needs to be multiple sets
> > > (up to four) of parameters which are queued in a host-to-sp queue.
> > >
> > > Signed-off-by: Yong Zhi <[e-mail]>
> > > ---
> > >  drivers/media/pci/intel/ipu3/ipu3-css-pool.c | 136
> > > +++++++++++++++++++++++++++
> > > drivers/media/pci/intel/ipu3/ipu3-css-pool.h |  56 +++++++++++
> > >  2 files changed, 192 insertions(+)
> > >  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> > >  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> > >
> > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> > > b/drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> > > new file mode 100644
> > > index 0000000..eab41c3
> > > --- /dev/null
> > > +++ b/drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> > > @@ -0,0 +1,136 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +// Copyright (C) 2018 Intel Corporation
> > > +
> > > +#include <linux/device.h>
> > > +
> > > +#include "ipu3.h"
> > > +#include "ipu3-css-pool.h"
> > > +#include "ipu3-dmamap.h"
> > > +
> > > +int ipu3_css_dma_buffer_resize(struct imgu_device *imgu,
> > > +			       struct ipu3_css_map *map, size_t size) {
> > > +	if (map->size < size && map->vaddr) {
> > > +		dev_warn(&imgu->pci_dev->dev, "dma buf resized from %zu
> > to %zu",
> > > +			 map->size, size);
> > > +
> > > +		ipu3_dmamap_free(imgu, map);
> > > +		if (!ipu3_dmamap_alloc(imgu, map, size))
> > > +			return -ENOMEM;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void ipu3_css_pool_cleanup(struct imgu_device *imgu, struct
> > > +ipu3_css_pool *pool) {
> > > +	unsigned int i;
> > > +
> > > +	for (i = 0; i < IPU3_CSS_POOL_SIZE; i++)
> > > +		ipu3_dmamap_free(imgu, &pool->entry[i].param); }
> > > +
> > > +int ipu3_css_pool_init(struct imgu_device *imgu, struct ipu3_css_pool
> > *pool,
> > > +		       size_t size)
> > > +{
> > > +	unsigned int i;
> > > +
> > > +	for (i = 0; i < IPU3_CSS_POOL_SIZE; i++) {
> > > +		/*
> > > +		 * entry[i].framenum is initialized to INT_MIN so that
> > > +		 * ipu3_css_pool_check() can treat it as usesable slot.
> > > +		 */
> > > +		pool->entry[i].framenum = INT_MIN;
> > > +
> > > +		if (size == 0) {
> > > +			pool->entry[i].param.vaddr = NULL;
> > > +			continue;
> > > +		}
> > > +
> > > +		if (!ipu3_dmamap_alloc(imgu, &pool->entry[i].param, size))
> > > +			goto fail;
> > > +	}
> > > +
> > > +	pool->last = IPU3_CSS_POOL_SIZE;
> > > +
> > > +	return 0;
> > > +
> > > +fail:
> > > +	ipu3_css_pool_cleanup(imgu, pool);
> > > +	return -ENOMEM;
> > > +}
> > > +
> > > +/*
> > > + * Check that the following call to pool_get succeeds.
> > > + * Return negative on error.
> > > + */
> > > +static int ipu3_css_pool_check(struct ipu3_css_pool *pool, long
> > > +framenum) {
> > > +	/* Get the oldest entry */
> > > +	int n = (pool->last + 1) % IPU3_CSS_POOL_SIZE;
> > > +	long diff = framenum - pool->entry[n].framenum;
> > > +
> > > +	/* if framenum wraps around and becomes smaller than entry n */
> > > +	if (diff < 0)
> > > +		diff += LONG_MAX;
> > 
> > Have you tested the wrap-around? As a result, the value of the diff is
> > between -1 and LONG_MAX - 1 (without considering more than just the two
> > lines above). Is that intended?
> > 
> 
> Yes, I simulated wrap-around using a smaller limit in v5.
> 
> > You seem to be using different types for the frame number; sometimes int,
> > sometimes long. Could you align that, preferrably to an unsigned type? u32
> > would probably be a sound choice.
> > 
> 
> Will use u32 at places except entry.framenum, which is initialized to
> INT_MIN. This is because the frame is counted from 0 at stream start, and
> the entry.framenum must be smaller enough for the ipu3_css_pool_check()
> to not return -ENOSPC.

You could use another field to tell whether an entry is valid or not.
That'd simplify the code, as well as remove the need to cap the frame
number to an arbitrary value.

Patch

diff --git a/drivers/media/pci/intel/ipu3/ipu3-css-pool.c b/drivers/media/pci/intel/ipu3/ipu3-css-pool.c
new file mode 100644
index 0000000..eab41c3
--- /dev/null
+++ b/drivers/media/pci/intel/ipu3/ipu3-css-pool.c
@@ -0,0 +1,136 @@ 
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 Intel Corporation
+
+#include <linux/device.h>
+
+#include "ipu3.h"
+#include "ipu3-css-pool.h"
+#include "ipu3-dmamap.h"
+
+int ipu3_css_dma_buffer_resize(struct imgu_device *imgu,
+			       struct ipu3_css_map *map, size_t size)
+{
+	if (map->size < size && map->vaddr) {
+		dev_warn(&imgu->pci_dev->dev, "dma buf resized from %zu to %zu",
+			 map->size, size);
+
+		ipu3_dmamap_free(imgu, map);
+		if (!ipu3_dmamap_alloc(imgu, map, size))
+			return -ENOMEM;
+	}
+
+	return 0;
+}
+
+void ipu3_css_pool_cleanup(struct imgu_device *imgu, struct ipu3_css_pool *pool)
+{
+	unsigned int i;
+
+	for (i = 0; i < IPU3_CSS_POOL_SIZE; i++)
+		ipu3_dmamap_free(imgu, &pool->entry[i].param);
+}
+
+int ipu3_css_pool_init(struct imgu_device *imgu, struct ipu3_css_pool *pool,
+		       size_t size)
+{
+	unsigned int i;
+
+	for (i = 0; i < IPU3_CSS_POOL_SIZE; i++) {
+		/*
+		 * entry[i].framenum is initialized to INT_MIN so that
+		 * ipu3_css_pool_check() can treat it as usesable slot.
+		 */
+		pool->entry[i].framenum = INT_MIN;
+
+		if (size == 0) {
+			pool->entry[i].param.vaddr = NULL;
+			continue;
+		}
+
+		if (!ipu3_dmamap_alloc(imgu, &pool->entry[i].param, size))
+			goto fail;
+	}
+
+	pool->last = IPU3_CSS_POOL_SIZE;
+
+	return 0;
+
+fail:
+	ipu3_css_pool_cleanup(imgu, pool);
+	return -ENOMEM;
+}
+
+/*
+ * Check that the following call to pool_get succeeds.
+ * Return negative on error.
+ */
+static int ipu3_css_pool_check(struct ipu3_css_pool *pool, long framenum)
+{
+	/* Get the oldest entry */
+	int n = (pool->last + 1) % IPU3_CSS_POOL_SIZE;
+	long diff = framenum - pool->entry[n].framenum;
+
+	/* if framenum wraps around and becomes smaller than entry n */
+	if (diff < 0)
+		diff += LONG_MAX;
+
+	/*
+	 * pool->entry[n].framenum stores the frame number where that
+	 * entry was allocated. If that was allocated more than POOL_SIZE
+	 * frames back, it is old enough that we know it is no more in
+	 * use by firmware.
+	 */
+	if (diff > IPU3_CSS_POOL_SIZE)
+		return n;
+
+	return -ENOSPC;
+}
+
+/*
+ * Allocate a new parameter from pool at frame number `framenum'.
+ * Release the oldest entry in the pool to make space for the new entry.
+ * Return negative on error.
+ */
+int ipu3_css_pool_get(struct ipu3_css_pool *pool, long framenum)
+{
+	int n = ipu3_css_pool_check(pool, framenum);
+
+	if (n < 0)
+		return n;
+
+	pool->entry[n].framenum = framenum;
+	pool->last = n;
+
+	return n;
+}
+
+/*
+ * Undo, for all practical purposes, the effect of pool_get().
+ */
+void ipu3_css_pool_put(struct ipu3_css_pool *pool)
+{
+	pool->entry[pool->last].framenum = INT_MIN;
+	pool->last = (pool->last + IPU3_CSS_POOL_SIZE - 1) % IPU3_CSS_POOL_SIZE;
+}
+
+/**
+ * ipu3_css_pool_last - Retrieve the nth pool entry from last
+ *
+ * @pool: a pointer to &struct ipu3_css_pool.
+ * @n: the distance to the last index.
+ *
+ * Return: The nth entry from last or null map to indicate no frame stored.
+ */
+const struct ipu3_css_map *
+ipu3_css_pool_last(struct ipu3_css_pool *pool, unsigned int n)
+{
+	static const struct ipu3_css_map null_map = { 0 };
+	int i = (pool->last + IPU3_CSS_POOL_SIZE - n) % IPU3_CSS_POOL_SIZE;
+
+	WARN_ON(n >= IPU3_CSS_POOL_SIZE);
+
+	if (pool->entry[i].framenum < 0)
+		return &null_map;
+
+	return &pool->entry[i].param;
+}
diff --git a/drivers/media/pci/intel/ipu3/ipu3-css-pool.h b/drivers/media/pci/intel/ipu3/ipu3-css-pool.h
new file mode 100644
index 0000000..71e48d1
--- /dev/null
+++ b/drivers/media/pci/intel/ipu3/ipu3-css-pool.h
@@ -0,0 +1,56 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2018 Intel Corporation */
+
+#ifndef __IPU3_UTIL_H
+#define __IPU3_UTIL_H
+
+struct device;
+struct imgu_device;
+
+#define IPU3_CSS_POOL_SIZE		4
+
+/**
+ * ipu3_css_map - store DMA mapping info for buffer
+ *
+ * @size:		size of the buffer in bytes.
+ * @vaddr:		kernel virtual address.
+ * @daddr:		iova dma address to access IPU3.
+ * @vma:		private, a pointer to &struct vm_struct,
+ *			used for ipu3_dmamap_free.
+ */
+struct ipu3_css_map {
+	size_t size;
+	void *vaddr;
+	dma_addr_t daddr;
+	struct vm_struct *vma;
+};
+
+/**
+ * ipu3_css_pool - circular buffer pool definition
+ *
+ * @entry:		array with IPU3_CSS_POOL_SIZE elements.
+ * @entry.param:	a &struct ipu3_css_map for storing the mem mapping.
+ * @entry.framenum:	the css frame number, used to determine if the entry
+ *			is old enough to be recycled.
+ * @last:		write pointer, initialized to IPU3_CSS_POOL_SIZE.
+ */
+struct ipu3_css_pool {
+	struct {
+		struct ipu3_css_map param;
+		long framenum;
+	} entry[IPU3_CSS_POOL_SIZE];
+	unsigned int last; /* Latest entry */
+};
+
+int ipu3_css_dma_buffer_resize(struct imgu_device *imgu,
+			       struct ipu3_css_map *map, size_t size);
+void ipu3_css_pool_cleanup(struct imgu_device *imgu,
+			   struct ipu3_css_pool *pool);
+int ipu3_css_pool_init(struct imgu_device *imgu, struct ipu3_css_pool *pool,
+		       size_t size);
+int ipu3_css_pool_get(struct ipu3_css_pool *pool, long framenum);
+void ipu3_css_pool_put(struct ipu3_css_pool *pool);
+const struct ipu3_css_map *ipu3_css_pool_last(struct ipu3_css_pool *pool,
+					      unsigned int last);
+
+#endif