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@xxxxxxxxxxxxxxxx>
> >> ---
> >> 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
