Re: [PATCH 18/28] media: ti-vpe: cal: add 'use_pix_proc' field

[Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>]

On 18/04/2021 16:00, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> On Mon, Apr 12, 2021 at 02:34:47PM +0300, Tomi Valkeinen wrote:
> > We already have functions to reserve and release a pix proc unit, but we
> > always reserve such and the code expects the pix proc unit to be used.
> >
> > Add a new field, 'use_pix_proc', to indicate if the pix prox unit has
> > been reserved and should be used. Use the flag to skip programming pix
> > proc unit when not needed.
> >
> > Note that we still always set the use_pix_proc flag to true.
> >
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> > ---
> >   drivers/media/platform/ti-vpe/cal.c | 10 +++++++---
> >   drivers/media/platform/ti-vpe/cal.h |  2 ++
> >   2 files changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> > index e397f59d3bbc..a1d173bd4613 100644
> > --- a/drivers/media/platform/ti-vpe/cal.c
> > +++ b/drivers/media/platform/ti-vpe/cal.c
> > @@ -473,13 +473,15 @@ int cal_ctx_prepare(struct cal_ctx *ctx)
> >   	}
> >   
> >   	ctx->pix_proc = ret;
> > +	ctx->use_pix_proc = true;
> >   
> >   	return 0;
> >   }
> >   
> >   void cal_ctx_unprepare(struct cal_ctx *ctx)
> >   {
> > -	cal_release_pix_proc(ctx->cal, ctx->pix_proc);
> > +	if (ctx->use_pix_proc)
> > +		cal_release_pix_proc(ctx->cal, ctx->pix_proc);
> >   }
> >   
> >   void cal_ctx_start(struct cal_ctx *ctx)
> > @@ -489,7 +491,8 @@ void cal_ctx_start(struct cal_ctx *ctx)
> >   
> >   	/* Configure the CSI-2, pixel processing and write DMA contexts. */
> >   	cal_ctx_csi2_config(ctx);
> > -	cal_ctx_pix_proc_config(ctx);
> > +	if (ctx->use_pix_proc)
> > +		cal_ctx_pix_proc_config(ctx);
> >   	cal_ctx_wr_dma_config(ctx);
> >   
> >   	/* Enable IRQ_WDMA_END and IRQ_WDMA_START. */
> > @@ -530,7 +533,8 @@ void cal_ctx_stop(struct cal_ctx *ctx)
> >   	cal_write(ctx->cal, CAL_CSI2_CTX(ctx->phy->instance, ctx->ppi_ctx), 0);
> >   
> >   	/* Disable pix proc */
> > -	cal_write(ctx->cal, CAL_PIX_PROC(ctx->pix_proc), 0);
> > +	if (ctx->use_pix_proc)
> > +		cal_write(ctx->cal, CAL_PIX_PROC(ctx->pix_proc), 0);
> >   }
> >   
> >   /* ------------------------------------------------------------------
> > diff --git a/drivers/media/platform/ti-vpe/cal.h b/drivers/media/platform/ti-vpe/cal.h
> > index 01e05e46e48d..409b7276a1fa 100644
> > --- a/drivers/media/platform/ti-vpe/cal.h
> > +++ b/drivers/media/platform/ti-vpe/cal.h
> > @@ -223,6 +223,8 @@ struct cal_ctx {
> >   	u8			cport;
> >   	u8			ppi_ctx;
> >   	u8			pix_proc;
> > +
> > +	bool use_pix_proc;
>
> Indentation ?
>
> How about turning pix_proc to a signed value, and using -1 to indicate
> it's not used ?

I considered that. But then, instead of "if (ctx->use_pix_proc)" I would 
have "if (ctx->use_pix_proc >= 0)". I think the former is better. And 
having a non-zero default value always makes me a bit uneasy.

 Tomi