Re: [PATCH 19/28] media: ti-vpe: cal: add cal_ctx_wr_dma_enable and fix a race

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

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