| Message ID | 20210412113457.328012-19-tomi.valkeinen@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ti-vpe: cal: prepare for multistream support | expand |
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@ideasonboard.com> > --- > 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 ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > }; > > extern unsigned int cal_debug;
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@ideasonboard.com> >> --- >> 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
Hi Tomi, On Mon, Apr 19, 2021 at 02:53:56PM +0300, Tomi Valkeinen wrote: > On 18/04/2021 16:00, Laurent Pinchart wrote: > > 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@ideasonboard.com> > >> --- > >> 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. I personally feel more uneasy when having a bool that qualifies whether another field is valid or not :-) Probably because I then never know what to set the other field to when setting the bool to false. It could just be me, it doesn't matter much.
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; }; extern unsigned int cal_debug;
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@ideasonboard.com> --- drivers/media/platform/ti-vpe/cal.c | 10 +++++++--- drivers/media/platform/ti-vpe/cal.h | 2 ++ 2 files changed, 9 insertions(+), 3 deletions(-)