Re: [PATCH v4 1/2] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer

[Dmitry Baryshkov]

On Fri, 16 Dec 2022 at 00:53, Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx> wrote:
>
> There are 3 possible interrupt sources are handled by DP controller,
> HPDstatus, Controller state changes and Aux read/write transaction.
> At every irq, DP controller have to check isr status of every interrupt
> sources and service the interrupt if its isr status bits shows interrupts
> are pending. There is potential race condition may happen at current aux
> isr handler implementation since it is always complete dp_aux_cmd_fifo_tx()
> even irq is not for aux read or write transaction. This may cause aux read
> transaction return premature if host aux data read is in the middle of
> waiting for sink to complete transferring data to host while irq happen.
> This will cause host's receiving buffer contains unexpected data. This
> patch fixes this problem by checking aux isr and return immediately at
> aux isr handler if there are no any isr status bits set.
>
> Current there is a bug report regrading eDP edid corruption happen during
> system booting up. After lengthy debugging to found that that VIDEO_READY
> interrupt was continuously firing during system booting up which cause
> dp_aux_isr() to complete dp_aux_cmd_fifo_tx() prematurely to retrieve data
> from aux hardware buffer which is not yet contains complete data transfer
> from sink. This cause edid corruption.
>
> Follows are the signature at kernel logs when problem happen,
> EDID has corrupt header
> panel-simple-dp-aux aux-aea0000.edp: Couldn't identify panel via EDID
> panel-simple-dp-aux aux-aea0000.edp: error -EIO: Couldn't detect panel nor find a fallback
>
> Changes in v2:
> -- do complete if (ret == IRQ_HANDLED) ay dp-aux_isr()
> -- add more commit text
>
> Changes in v3:
> -- add Stephen suggested
> -- dp_aux_isr() return IRQ_XXX back to caller
> -- dp_ctrl_isr() return IRQ_XXX back to caller
>
> Changes in v4:
> -- split into two patches
>
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
>

The same feedback as the one I gave for v2 or v3:

There should be no empty lines between tags. And you still have one
empty line here.

With that fixed:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>

> Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
> Tested-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> ---
>  drivers/gpu/drm/msm/dp/dp_aux.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index d030a93..cc3efed 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -423,6 +423,10 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)
>
>         isr = dp_catalog_aux_get_irq(aux->catalog);
>
> +       /* no interrupts pending, return immediately */
> +       if (!isr)
> +               return;
> +
>         if (!aux->cmd_busy)
>                 return;
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>


-- 
With best wishes
Dmitry