| Message ID | 20210412113457.328012-11-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Superseded |
| 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:39PM +0300, Tomi Valkeinen wrote: > CAL has 4 pixel processing contexts (PIX PROC) of which the driver > currently uses pix proc 0 for PHY0, and pix proc 1 for PHY1 (as the > driver supports only a single source per PHY). > > Add a pix_proc field to cal_ctx to allow us to use different pix proc > contexts in the future > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/platform/ti-vpe/cal.c | 9 +++++---- > drivers/media/platform/ti-vpe/cal.h | 1 + > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c > index c550eeb27e79..3dc83c66fd96 100644 > --- a/drivers/media/platform/ti-vpe/cal.c > +++ b/drivers/media/platform/ti-vpe/cal.c > @@ -355,16 +355,16 @@ static void cal_ctx_pix_proc_config(struct cal_ctx *ctx) > break; > } > > - val = cal_read(ctx->cal, CAL_PIX_PROC(ctx->index)); > + val = cal_read(ctx->cal, CAL_PIX_PROC(ctx->pix_proc)); > cal_set_field(&val, extract, CAL_PIX_PROC_EXTRACT_MASK); > cal_set_field(&val, CAL_PIX_PROC_DPCMD_BYPASS, CAL_PIX_PROC_DPCMD_MASK); > cal_set_field(&val, CAL_PIX_PROC_DPCME_BYPASS, CAL_PIX_PROC_DPCME_MASK); > cal_set_field(&val, pack, CAL_PIX_PROC_PACK_MASK); > cal_set_field(&val, ctx->cport, CAL_PIX_PROC_CPORT_MASK); > cal_set_field(&val, 1, CAL_PIX_PROC_EN_MASK); > - cal_write(ctx->cal, CAL_PIX_PROC(ctx->index), val); > - ctx_dbg(3, ctx, "CAL_PIX_PROC(%d) = 0x%08x\n", ctx->index, > - cal_read(ctx->cal, CAL_PIX_PROC(ctx->index))); > + cal_write(ctx->cal, CAL_PIX_PROC(ctx->pix_proc), val); > + ctx_dbg(3, ctx, "CAL_PIX_PROC(%d) = 0x%08x\n", ctx->pix_proc, While at it, you could s/%d/%u/ > + cal_read(ctx->cal, CAL_PIX_PROC(ctx->pix_proc))); And should we use val here instead of reading the value back ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > } > > static void cal_ctx_wr_dma_config(struct cal_ctx *ctx) > @@ -857,6 +857,7 @@ static struct cal_ctx *cal_ctx_create(struct cal_dev *cal, int inst) > ctx->index = 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 6eb63268f916..03c71763f1a5 100644 > --- a/drivers/media/platform/ti-vpe/cal.h > +++ b/drivers/media/platform/ti-vpe/cal.h > @@ -220,6 +220,7 @@ struct cal_ctx { > u8 index; > u8 cport; > u8 ppi_ctx; > + u8 pix_proc; > }; > > extern unsigned int cal_debug; -- Regards, Laurent Pinchart
And another comment. On Sun, Apr 18, 2021 at 03:20:29PM +0300, Laurent Pinchart wrote: > On Mon, Apr 12, 2021 at 02:34:39PM +0300, Tomi Valkeinen wrote: > > CAL has 4 pixel processing contexts (PIX PROC) of which the driver > > currently uses pix proc 0 for PHY0, and pix proc 1 for PHY1 (as the > > driver supports only a single source per PHY). > > > > Add a pix_proc field to cal_ctx to allow us to use different pix proc > > contexts in the future > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > --- > > drivers/media/platform/ti-vpe/cal.c | 9 +++++---- > > drivers/media/platform/ti-vpe/cal.h | 1 + > > 2 files changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c > > index c550eeb27e79..3dc83c66fd96 100644 > > --- a/drivers/media/platform/ti-vpe/cal.c > > +++ b/drivers/media/platform/ti-vpe/cal.c > > @@ -355,16 +355,16 @@ static void cal_ctx_pix_proc_config(struct cal_ctx *ctx) > > break; > > } > > > > - val = cal_read(ctx->cal, CAL_PIX_PROC(ctx->index)); > > + val = cal_read(ctx->cal, CAL_PIX_PROC(ctx->pix_proc)); > > cal_set_field(&val, extract, CAL_PIX_PROC_EXTRACT_MASK); > > cal_set_field(&val, CAL_PIX_PROC_DPCMD_BYPASS, CAL_PIX_PROC_DPCMD_MASK); > > cal_set_field(&val, CAL_PIX_PROC_DPCME_BYPASS, CAL_PIX_PROC_DPCME_MASK); > > cal_set_field(&val, pack, CAL_PIX_PROC_PACK_MASK); > > cal_set_field(&val, ctx->cport, CAL_PIX_PROC_CPORT_MASK); > > cal_set_field(&val, 1, CAL_PIX_PROC_EN_MASK); > > - cal_write(ctx->cal, CAL_PIX_PROC(ctx->index), val); > > - ctx_dbg(3, ctx, "CAL_PIX_PROC(%d) = 0x%08x\n", ctx->index, > > - cal_read(ctx->cal, CAL_PIX_PROC(ctx->index))); > > + cal_write(ctx->cal, CAL_PIX_PROC(ctx->pix_proc), val); > > + ctx_dbg(3, ctx, "CAL_PIX_PROC(%d) = 0x%08x\n", ctx->pix_proc, > > While at it, you could s/%d/%u/ > > > + cal_read(ctx->cal, CAL_PIX_PROC(ctx->pix_proc))); > > And should we use val here instead of reading the value back ? Given that these patterns are common in the driver, a separate patch to address them all (if desired) would likely be best. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > } > > > > static void cal_ctx_wr_dma_config(struct cal_ctx *ctx) > > @@ -857,6 +857,7 @@ static struct cal_ctx *cal_ctx_create(struct cal_dev *cal, int inst) > > ctx->index = 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 6eb63268f916..03c71763f1a5 100644 > > --- a/drivers/media/platform/ti-vpe/cal.h > > +++ b/drivers/media/platform/ti-vpe/cal.h > > @@ -220,6 +220,7 @@ struct cal_ctx { > > u8 index; > > u8 cport; > > u8 ppi_ctx; > > + u8 pix_proc; > > }; > > > > extern unsigned int cal_debug; -- Regards, Laurent Pinchart
On 18/04/2021 15:20, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Mon, Apr 12, 2021 at 02:34:39PM +0300, Tomi Valkeinen wrote: >> CAL has 4 pixel processing contexts (PIX PROC) of which the driver >> currently uses pix proc 0 for PHY0, and pix proc 1 for PHY1 (as the >> driver supports only a single source per PHY). >> >> Add a pix_proc field to cal_ctx to allow us to use different pix proc >> contexts in the future >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/platform/ti-vpe/cal.c | 9 +++++---- >> drivers/media/platform/ti-vpe/cal.h | 1 + >> 2 files changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c >> index c550eeb27e79..3dc83c66fd96 100644 >> --- a/drivers/media/platform/ti-vpe/cal.c >> +++ b/drivers/media/platform/ti-vpe/cal.c >> @@ -355,16 +355,16 @@ static void cal_ctx_pix_proc_config(struct cal_ctx *ctx) >> break; >> } >> >> - val = cal_read(ctx->cal, CAL_PIX_PROC(ctx->index)); >> + val = cal_read(ctx->cal, CAL_PIX_PROC(ctx->pix_proc)); >> cal_set_field(&val, extract, CAL_PIX_PROC_EXTRACT_MASK); >> cal_set_field(&val, CAL_PIX_PROC_DPCMD_BYPASS, CAL_PIX_PROC_DPCMD_MASK); >> cal_set_field(&val, CAL_PIX_PROC_DPCME_BYPASS, CAL_PIX_PROC_DPCME_MASK); >> cal_set_field(&val, pack, CAL_PIX_PROC_PACK_MASK); >> cal_set_field(&val, ctx->cport, CAL_PIX_PROC_CPORT_MASK); >> cal_set_field(&val, 1, CAL_PIX_PROC_EN_MASK); >> - cal_write(ctx->cal, CAL_PIX_PROC(ctx->index), val); >> - ctx_dbg(3, ctx, "CAL_PIX_PROC(%d) = 0x%08x\n", ctx->index, >> - cal_read(ctx->cal, CAL_PIX_PROC(ctx->index))); >> + cal_write(ctx->cal, CAL_PIX_PROC(ctx->pix_proc), val); >> + ctx_dbg(3, ctx, "CAL_PIX_PROC(%d) = 0x%08x\n", ctx->pix_proc, > > While at it, you could s/%d/%u/ > >> + cal_read(ctx->cal, CAL_PIX_PROC(ctx->pix_proc))); > > And should we use val here instead of reading the value back ? I think it's usually good to read the value from the register. It'll show what the register value really is, not what we thought we wrote. Usually those two should be the same, but in some cases there are read-only fields or non-writeable areas. That said, I'm not sure if any of the registers printed in cal.c have any of those fields, so... I don't feel strongly either way, but I also don't see benefit in doing the change (verifying there are no read-only fields, and fixing the conflicts). I can change this and the other similar cases if you have a reason why it's better =). Tomi
diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c index c550eeb27e79..3dc83c66fd96 100644 --- a/drivers/media/platform/ti-vpe/cal.c +++ b/drivers/media/platform/ti-vpe/cal.c @@ -355,16 +355,16 @@ static void cal_ctx_pix_proc_config(struct cal_ctx *ctx) break; } - val = cal_read(ctx->cal, CAL_PIX_PROC(ctx->index)); + val = cal_read(ctx->cal, CAL_PIX_PROC(ctx->pix_proc)); cal_set_field(&val, extract, CAL_PIX_PROC_EXTRACT_MASK); cal_set_field(&val, CAL_PIX_PROC_DPCMD_BYPASS, CAL_PIX_PROC_DPCMD_MASK); cal_set_field(&val, CAL_PIX_PROC_DPCME_BYPASS, CAL_PIX_PROC_DPCME_MASK); cal_set_field(&val, pack, CAL_PIX_PROC_PACK_MASK); cal_set_field(&val, ctx->cport, CAL_PIX_PROC_CPORT_MASK); cal_set_field(&val, 1, CAL_PIX_PROC_EN_MASK); - cal_write(ctx->cal, CAL_PIX_PROC(ctx->index), val); - ctx_dbg(3, ctx, "CAL_PIX_PROC(%d) = 0x%08x\n", ctx->index, - cal_read(ctx->cal, CAL_PIX_PROC(ctx->index))); + cal_write(ctx->cal, CAL_PIX_PROC(ctx->pix_proc), val); + ctx_dbg(3, ctx, "CAL_PIX_PROC(%d) = 0x%08x\n", ctx->pix_proc, + cal_read(ctx->cal, CAL_PIX_PROC(ctx->pix_proc))); } static void cal_ctx_wr_dma_config(struct cal_ctx *ctx) @@ -857,6 +857,7 @@ static struct cal_ctx *cal_ctx_create(struct cal_dev *cal, int inst) ctx->index = 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 6eb63268f916..03c71763f1a5 100644 --- a/drivers/media/platform/ti-vpe/cal.h +++ b/drivers/media/platform/ti-vpe/cal.h @@ -220,6 +220,7 @@ struct cal_ctx { u8 index; u8 cport; u8 ppi_ctx; + u8 pix_proc; }; extern unsigned int cal_debug;
CAL has 4 pixel processing contexts (PIX PROC) of which the driver currently uses pix proc 0 for PHY0, and pix proc 1 for PHY1 (as the driver supports only a single source per PHY). Add a pix_proc field to cal_ctx to allow us to use different pix proc contexts in the future Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/platform/ti-vpe/cal.c | 9 +++++---- drivers/media/platform/ti-vpe/cal.h | 1 + 2 files changed, 6 insertions(+), 4 deletions(-)