Hi Tomi,
Thank you for the patch.
On Thu, Apr 21, 2022 at 05:34:48PM +0300, Tomi Valkeinen wrote:
> Instead of handling the WDMA START and END interrupts separately, we
> need to handle both at the same time to better manage the inherent race
> conditions related to CAL interrupts.
>
> Change the code so that we have a single function,
> cal_irq_handle_wdma(), which gets two booleans, start and end, as
> parameters, which allows us to manage the race conditions in the
> following patch.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/platform/ti/cal/cal.c | 59 ++++++++++++-----------------
> 1 file changed, 25 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/media/platform/ti/cal/cal.c b/drivers/media/platform/ti/cal/cal.c
> index 783ce5a8cd79..e4355f266c58 100644
> --- a/drivers/media/platform/ti/cal/cal.c
> +++ b/drivers/media/platform/ti/cal/cal.c
> @@ -668,22 +668,33 @@ static inline void cal_irq_wdma_end(struct cal_ctx *ctx)
> }
> }
>
> +static void cal_irq_handle_wdma(struct cal_ctx *ctx, bool start, bool end)
> +{
> + if (end)
> + cal_irq_wdma_end(ctx);
> +
> + if (start)
> + cal_irq_wdma_start(ctx);
> +}
> +
> static irqreturn_t cal_irq(int irq_cal, void *data)
> {
> struct cal_dev *cal = data;
> - u32 status;
> -
> - status = cal_read(cal, CAL_HL_IRQSTATUS(0));
> - if (status) {
> - unsigned int i;
> + u32 status[3];
> + unsigned int i;
>
> - cal_write(cal, CAL_HL_IRQSTATUS(0), status);
> + for (i = 0; i < 3; ++i) {
> + status[i] = cal_read(cal, CAL_HL_IRQSTATUS(i));
> + if (status[i])
> + cal_write(cal, CAL_HL_IRQSTATUS(i), status[i]);
> + }
>
> - if (status & CAL_HL_IRQ_OCPO_ERR_MASK)
> + if (status[0]) {
> + if (status[0] & CAL_HL_IRQ_OCPO_ERR_MASK)
> dev_err_ratelimited(cal->dev, "OCPO ERROR\n");
>
> for (i = 0; i < cal->data->num_csi2_phy; ++i) {
> - if (status & CAL_HL_IRQ_CIO_MASK(i)) {
> + if (status[0] & CAL_HL_IRQ_CIO_MASK(i)) {
> u32 cio_stat = cal_read(cal,
> CAL_CSI2_COMPLEXIO_IRQSTATUS(i));
>
> @@ -694,7 +705,7 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
> cio_stat);
> }
>
> - if (status & CAL_HL_IRQ_VC_MASK(i)) {
> + if (status[0] & CAL_HL_IRQ_VC_MASK(i)) {
> u32 vc_stat = cal_read(cal, CAL_CSI2_VC_IRQSTATUS(i));
>
> dev_err_ratelimited(cal->dev,
> @@ -706,32 +717,12 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
> }
> }
>
> - /* Check which DMA just finished */
> - status = cal_read(cal, CAL_HL_IRQSTATUS(1));
> - if (status) {
> - unsigned int i;
> -
> - /* Clear Interrupt status */
> - cal_write(cal, CAL_HL_IRQSTATUS(1), status);
> -
> - for (i = 0; i < cal->num_contexts; ++i) {
> - if (status & CAL_HL_IRQ_WDMA_END_MASK(i))
> - cal_irq_wdma_end(cal->ctx[i]);
> - }
> - }
> -
> - /* Check which DMA just started */
> - status = cal_read(cal, CAL_HL_IRQSTATUS(2));
> - if (status) {
> - unsigned int i;
> -
> - /* Clear Interrupt status */
> - cal_write(cal, CAL_HL_IRQSTATUS(2), status);
> + for (i = 0; i < cal->num_contexts; ++i) {
> + bool end = !!(status[1] & CAL_HL_IRQ_WDMA_END_MASK(i));
> + bool start = !!(status[2] & CAL_HL_IRQ_WDMA_START_MASK(i));
>
> - for (i = 0; i < cal->num_contexts; ++i) {
> - if (status & CAL_HL_IRQ_WDMA_START_MASK(i))
> - cal_irq_wdma_start(cal->ctx[i]);
> - }
> + if (start || end)
> + cal_irq_handle_wdma(cal->ctx[i], start, end);
> }
>
> return IRQ_HANDLED;
--
Regards,
Laurent Pinchart
