Re: [PATCH 17/28] media: ti-vpe: cal: allocate pix proc dynamically

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Tomi,

Thank you for the patch.

On Mon, Apr 12, 2021 at 02:34:46PM +0300, Tomi Valkeinen wrote:
> CAL has 4 pixel processing units but the units are not needed e.g. for
> metadata. As we could be capturing 4 pixel streams and 4 metadata
> streams, i.e. using 8 DMA contexts, we cannot assign a pixel processing
> unit to every DMA context. Instead we need to reserve a pixel processing
> unit only when needed.
> 
> Add functions to reserve and release a pix proc unit, and use them in
> cal_ctx_prepare/unprepare. Note that for the time being we still always
> reserve a pix proc unit.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/platform/ti-vpe/cal.c | 44 +++++++++++++++++++++++++++--
>  drivers/media/platform/ti-vpe/cal.h |  2 ++
>  2 files changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index a6ca341c98bd..e397f59d3bbc 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -290,6 +290,37 @@ void cal_quickdump_regs(struct cal_dev *cal)
>   * ------------------------------------------------------------------
>   */
>  
> +#define CAL_MAX_PIX_PROC 4
> +
> +static int cal_reserve_pix_proc(struct cal_dev *cal)
> +{
> +	unsigned long ret;
> +
> +	spin_lock(&cal->v4l2_dev.lock);
> +
> +	ret = find_first_zero_bit(&cal->reserve_pix_proc_mask, CAL_MAX_PIX_PROC);
> +
> +	if (ret == CAL_MAX_PIX_PROC) {
> +		spin_unlock(&cal->v4l2_dev.lock);
> +		return -ENOSPC;
> +	}
> +
> +	cal->reserve_pix_proc_mask |= BIT(ret);
> +
> +	spin_unlock(&cal->v4l2_dev.lock);
> +
> +	return ret;
> +}
> +
> +static void cal_release_pix_proc(struct cal_dev *cal, unsigned int pix_proc_num)
> +{
> +	spin_lock(&cal->v4l2_dev.lock);
> +
> +	cal->reserve_pix_proc_mask &= ~BIT(pix_proc_num);
> +
> +	spin_unlock(&cal->v4l2_dev.lock);
> +}
> +
>  static void cal_ctx_csi2_config(struct cal_ctx *ctx)
>  {
>  	u32 val;
> @@ -433,12 +464,22 @@ static bool cal_ctx_wr_dma_stopped(struct cal_ctx *ctx)
>  
>  int cal_ctx_prepare(struct cal_ctx *ctx)
>  {
> +	int ret;
> +
> +	ret = cal_reserve_pix_proc(ctx->cal);
> +	if (ret < 0) {
> +		ctx_err(ctx, "Failed to reserve pix proc: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ctx->pix_proc = ret;

I wonder if this is the right place to allocate a context, it may be
better to reject invalid pipeline configurations earlier on. It's fine
for now, we can revisit this in subsequent patches, with the full
multiplexed streams implementation.

Another option would be to allocate the context in cal_ctx_create() for
now, with a call to cal_reserve_pix_proc(), and move it later in the
context of the patch series that implements support for multiplexed
streams.

> +
>  	return 0;
>  }
>  
>  void cal_ctx_unprepare(struct cal_ctx *ctx)
>  {
> -
> +	cal_release_pix_proc(ctx->cal, ctx->pix_proc);
>  }
>  
>  void cal_ctx_start(struct cal_ctx *ctx)
> @@ -872,7 +913,6 @@ static struct cal_ctx *cal_ctx_create(struct cal_dev *cal, int inst)
>  	ctx->dma_ctx = inst;
>  	ctx->ppi_ctx = inst;
>  	ctx->cport = inst;
> -	ctx->pix_proc = inst;
>  
>  	ret = cal_ctx_v4l2_init(ctx);
>  	if (ret)
> diff --git a/drivers/media/platform/ti-vpe/cal.h b/drivers/media/platform/ti-vpe/cal.h
> index c34b843d2019..01e05e46e48d 100644
> --- a/drivers/media/platform/ti-vpe/cal.h
> +++ b/drivers/media/platform/ti-vpe/cal.h
> @@ -188,6 +188,8 @@ struct cal_dev {
>  	struct media_device	mdev;
>  	struct v4l2_device	v4l2_dev;
>  	struct v4l2_async_notifier notifier;
> +
> +	unsigned long reserve_pix_proc_mask;

I would have named this used_pix_proc_mask.

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

>  };
>  
>  /*

-- 
Regards,

Laurent Pinchart