On 18/04/2021 15:59, Laurent Pinchart wrote:
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@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.
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.
+
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.
Tomi
