| Message ID | 20210412113457.328012-20-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:48PM +0300, Tomi Valkeinen wrote: > I have not noticed any errors due to this, but the DMA configuration > looks racy. Setting the DMA mode bitfield in CAL_WR_DMA_CTRL supposedly > enables the DMA. However, the driver currently a) continues configuring > the DMA after setting the mode, and b) enables the DMA interrupts only > after setting the mode. > > This probably doesn't cause any issues as there should be no data coming > in to the DMA yet, but it's still better to fix this. > > Add a new function, cal_ctx_wr_dma_enable(), to set the DMA mode field, > and call that function only after the DMA config and the irq enabling > has been done. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/platform/ti-vpe/cal.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c > index a1d173bd4613..0fef892854ef 100644 > --- a/drivers/media/platform/ti-vpe/cal.c > +++ b/drivers/media/platform/ti-vpe/cal.c > @@ -409,8 +409,6 @@ static void cal_ctx_wr_dma_config(struct cal_ctx *ctx) > CAL_WR_DMA_CTRL_YSIZE_MASK); > cal_set_field(&val, CAL_WR_DMA_CTRL_DTAG_PIX_DAT, > CAL_WR_DMA_CTRL_DTAG_MASK); > - cal_set_field(&val, CAL_WR_DMA_CTRL_MODE_CONST, > - CAL_WR_DMA_CTRL_MODE_MASK); > cal_set_field(&val, CAL_WR_DMA_CTRL_PATTERN_LINEAR, > CAL_WR_DMA_CTRL_PATTERN_MASK); > cal_set_field(&val, 1, CAL_WR_DMA_CTRL_STALL_RD_MASK); > @@ -442,6 +440,15 @@ void cal_ctx_set_dma_addr(struct cal_ctx *ctx, dma_addr_t addr) > cal_write(ctx->cal, CAL_WR_DMA_ADDR(ctx->dma_ctx), addr); > } > > +static void cal_ctx_wr_dma_enable(struct cal_ctx *ctx) > +{ > + u32 val = cal_read(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx)); > + > + cal_set_field(&val, CAL_WR_DMA_CTRL_MODE_CONST, > + CAL_WR_DMA_CTRL_MODE_MASK); > + cal_write(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx), val); > +} > + > static void cal_ctx_wr_dma_disable(struct cal_ctx *ctx) > { > u32 val = cal_read(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx)); > @@ -500,6 +507,8 @@ void cal_ctx_start(struct cal_ctx *ctx) > CAL_HL_IRQ_WDMA_END_MASK(ctx->dma_ctx)); > cal_write(ctx->cal, CAL_HL_IRQENABLE_SET(2), > CAL_HL_IRQ_WDMA_START_MASK(ctx->dma_ctx)); > + > + cal_ctx_wr_dma_enable(ctx); > } You could also move the IRQ enabling before the call to cal_ctx_wr_dma_config(), and reorder the configuration in cal_ctx_wr_dma_config() to write CAL_WR_DMA_CTRL last. That would save a read-modify-write cycle. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > void cal_ctx_stop(struct cal_ctx *ctx)
On 18/04/2021 16:04, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Mon, Apr 12, 2021 at 02:34:48PM +0300, Tomi Valkeinen wrote: >> I have not noticed any errors due to this, but the DMA configuration >> looks racy. Setting the DMA mode bitfield in CAL_WR_DMA_CTRL supposedly >> enables the DMA. However, the driver currently a) continues configuring >> the DMA after setting the mode, and b) enables the DMA interrupts only >> after setting the mode. >> >> This probably doesn't cause any issues as there should be no data coming >> in to the DMA yet, but it's still better to fix this. >> >> Add a new function, cal_ctx_wr_dma_enable(), to set the DMA mode field, >> and call that function only after the DMA config and the irq enabling >> has been done. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/platform/ti-vpe/cal.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c >> index a1d173bd4613..0fef892854ef 100644 >> --- a/drivers/media/platform/ti-vpe/cal.c >> +++ b/drivers/media/platform/ti-vpe/cal.c >> @@ -409,8 +409,6 @@ static void cal_ctx_wr_dma_config(struct cal_ctx *ctx) >> CAL_WR_DMA_CTRL_YSIZE_MASK); >> cal_set_field(&val, CAL_WR_DMA_CTRL_DTAG_PIX_DAT, >> CAL_WR_DMA_CTRL_DTAG_MASK); >> - cal_set_field(&val, CAL_WR_DMA_CTRL_MODE_CONST, >> - CAL_WR_DMA_CTRL_MODE_MASK); >> cal_set_field(&val, CAL_WR_DMA_CTRL_PATTERN_LINEAR, >> CAL_WR_DMA_CTRL_PATTERN_MASK); >> cal_set_field(&val, 1, CAL_WR_DMA_CTRL_STALL_RD_MASK); >> @@ -442,6 +440,15 @@ void cal_ctx_set_dma_addr(struct cal_ctx *ctx, dma_addr_t addr) >> cal_write(ctx->cal, CAL_WR_DMA_ADDR(ctx->dma_ctx), addr); >> } >> >> +static void cal_ctx_wr_dma_enable(struct cal_ctx *ctx) >> +{ >> + u32 val = cal_read(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx)); >> + >> + cal_set_field(&val, CAL_WR_DMA_CTRL_MODE_CONST, >> + CAL_WR_DMA_CTRL_MODE_MASK); >> + cal_write(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx), val); >> +} >> + >> static void cal_ctx_wr_dma_disable(struct cal_ctx *ctx) >> { >> u32 val = cal_read(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx)); >> @@ -500,6 +507,8 @@ void cal_ctx_start(struct cal_ctx *ctx) >> CAL_HL_IRQ_WDMA_END_MASK(ctx->dma_ctx)); >> cal_write(ctx->cal, CAL_HL_IRQENABLE_SET(2), >> CAL_HL_IRQ_WDMA_START_MASK(ctx->dma_ctx)); >> + >> + cal_ctx_wr_dma_enable(ctx); >> } > > You could also move the IRQ enabling before the call to > cal_ctx_wr_dma_config(), and reorder the configuration in > cal_ctx_wr_dma_config() to write CAL_WR_DMA_CTRL last. That would save a > read-modify-write cycle. Yes, I did that initially, but then ended up with a separate dma_enable call for clarity (I find it a bit misleading that cal_ctx_wr_dma_config would start the dma) and to have matching dma_enable and dma_disable calls. Tomi
diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c index a1d173bd4613..0fef892854ef 100644 --- a/drivers/media/platform/ti-vpe/cal.c +++ b/drivers/media/platform/ti-vpe/cal.c @@ -409,8 +409,6 @@ static void cal_ctx_wr_dma_config(struct cal_ctx *ctx) CAL_WR_DMA_CTRL_YSIZE_MASK); cal_set_field(&val, CAL_WR_DMA_CTRL_DTAG_PIX_DAT, CAL_WR_DMA_CTRL_DTAG_MASK); - cal_set_field(&val, CAL_WR_DMA_CTRL_MODE_CONST, - CAL_WR_DMA_CTRL_MODE_MASK); cal_set_field(&val, CAL_WR_DMA_CTRL_PATTERN_LINEAR, CAL_WR_DMA_CTRL_PATTERN_MASK); cal_set_field(&val, 1, CAL_WR_DMA_CTRL_STALL_RD_MASK); @@ -442,6 +440,15 @@ void cal_ctx_set_dma_addr(struct cal_ctx *ctx, dma_addr_t addr) cal_write(ctx->cal, CAL_WR_DMA_ADDR(ctx->dma_ctx), addr); } +static void cal_ctx_wr_dma_enable(struct cal_ctx *ctx) +{ + u32 val = cal_read(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx)); + + cal_set_field(&val, CAL_WR_DMA_CTRL_MODE_CONST, + CAL_WR_DMA_CTRL_MODE_MASK); + cal_write(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx), val); +} + static void cal_ctx_wr_dma_disable(struct cal_ctx *ctx) { u32 val = cal_read(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx)); @@ -500,6 +507,8 @@ void cal_ctx_start(struct cal_ctx *ctx) CAL_HL_IRQ_WDMA_END_MASK(ctx->dma_ctx)); cal_write(ctx->cal, CAL_HL_IRQENABLE_SET(2), CAL_HL_IRQ_WDMA_START_MASK(ctx->dma_ctx)); + + cal_ctx_wr_dma_enable(ctx); } void cal_ctx_stop(struct cal_ctx *ctx)
I have not noticed any errors due to this, but the DMA configuration looks racy. Setting the DMA mode bitfield in CAL_WR_DMA_CTRL supposedly enables the DMA. However, the driver currently a) continues configuring the DMA after setting the mode, and b) enables the DMA interrupts only after setting the mode. This probably doesn't cause any issues as there should be no data coming in to the DMA yet, but it's still better to fix this. Add a new function, cal_ctx_wr_dma_enable(), to set the DMA mode field, and call that function only after the DMA config and the irq enabling has been done. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/platform/ti-vpe/cal.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)