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

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

Hi Tomi,

On Mon, Apr 19, 2021 at 02:45:53PM +0300, Tomi Valkeinen wrote:
> On 18/04/2021 15:59, Laurent Pinchart wrote:
> > 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.
> 
> Earlier than what? Even before cal_start_streaming()? I guess we could 
> do this in cal_vb2_ioctl_reqbufs and cal_vb2_ioctl_create_bufs, but then 
> I'm not sure where to free the pix proc.

I'm trying to recall what I meant :-) There are only two options really,
at stream start (that's the latest point at which we need to guarantee a
valid pipeline), or when configuring routing (either through link setup,
or internal subdev routing). I'm not sure if the second option is even
possible. Buffer allocation isn't the right place.

> > 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.
> 
> For now cal_reserve_pix_proc() will always succeed, as we have 4 pix 
> procs but only ever allocate 2 at the same time. If there is a better 
> place to do this for multiplexed streams, which is not available yet at 
> this patch, then I agree, we could just do this in cal_ctx_create until 
> we have that better place available.

Maybe that's what I meant :-) As I can't really recall, it probably
doesn't matter much.

> >> +
> >>   	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.
> 
> The name has a typo, and should actually be 'reserved_pix_proc_mask'. 
> Doesn't 'used' mean that something was used, but may not be used 
> anymore? So... in_use_pix_proc_mask? 'active'? I don't know, I like 
> 'reserved' best here.

"reserved" is indeed better than "reserve". I may still like "used"
better, but I don't care much. Or you could flip it, and have
"free_pix_proc_mask". Up to you.

-- 
Regards,

Laurent Pinchart