Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <da01c5b0-dd2d-1bcf-353c-76fea5015504@quicinc.com>
Date:   Thu, 15 Dec 2022 12:30:47 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.2
Subject: Re: [PATCH v2] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if
 irq is not for aux transfer
Content-Language: en-US
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Kuogee Hsieh <quic_khsieh@quicinc.com>, <robdclark@gmail.com>,
        <sean@poorly.run>, <swboyd@chromium.org>, <dianders@chromium.org>,
        <vkoul@kernel.org>, <daniel@ffwll.ch>, <airlied@gmail.com>,
        <agross@kernel.org>, <andersson@kernel.org>
CC:     <quic_sbillaka@quicinc.com>, <freedreno@lists.freedesktop.org>,
        <dri-devel@lists.freedesktop.org>, <linux-arm-msm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <1671129159-31105-1-git-send-email-quic_khsieh@quicinc.com>
 <ca15a54b-8040-5e4f-a78e-12b7f8a554e1@linaro.org>
From:   Abhinav Kumar <quic_abhinavk@quicinc.com>
In-Reply-To: <ca15a54b-8040-5e4f-a78e-12b7f8a554e1@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 12/15/2022 10:46 AM, Dmitry Baryshkov wrote:
> On 15/12/2022 20:32, Kuogee Hsieh 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 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
> 
> Usually it's a single dash.
> 
>>
>> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> 
> There should be no empty lines between the tags.
> 
>> Tested-by: Douglas Anderson <dianders@chromium.org>
>> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_aux.c | 87 
>> +++++++++++++++++++++++++++++------------
>>   1 file changed, 63 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>> index d030a93..f31e5c1 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>> @@ -162,45 +162,78 @@ static ssize_t dp_aux_cmd_fifo_rx(struct 
>> dp_aux_private *aux,
>>       return i;
>>   }
>> -static void dp_aux_native_handler(struct dp_aux_private *aux, u32 isr)
>> +static irqreturn_t dp_aux_native_handler(struct dp_aux_private *aux, 
>> u32 isr)
>>   {
>> -    if (isr & DP_INTR_AUX_I2C_DONE)
>> +    irqreturn_t ret = IRQ_NONE;
>> +
>> +    if (isr & DP_INTR_AUX_I2C_DONE) {
>>           aux->aux_error_num = DP_AUX_ERR_NONE;
>> -    else if (isr & DP_INTR_WRONG_ADDR)
>> +        ret = IRQ_HANDLED;
>> +    } else if (isr & DP_INTR_WRONG_ADDR) {
>>           aux->aux_error_num = DP_AUX_ERR_ADDR;
>> -    else if (isr & DP_INTR_TIMEOUT)
>> +        ret = IRQ_HANDLED;
>> +    } else if (isr & DP_INTR_TIMEOUT) {
>>           aux->aux_error_num = DP_AUX_ERR_TOUT;
>> -    if (isr & DP_INTR_NACK_DEFER)
>> +        ret = IRQ_HANDLED;
>> +    }
>> +
>> +    if (isr & DP_INTR_NACK_DEFER) {
>>           aux->aux_error_num = DP_AUX_ERR_NACK;
>> +        ret = IRQ_HANDLED;
>> +    }
>> +
>>       if (isr & DP_INTR_AUX_ERROR) {
>>           aux->aux_error_num = DP_AUX_ERR_PHY;
>>           dp_catalog_aux_clear_hw_interrupts(aux->catalog);
>> +        ret = IRQ_HANDLED;
>>       }
>> +
>> +    return ret;
>>   }
>> -static void dp_aux_i2c_handler(struct dp_aux_private *aux, u32 isr)
>> +static irqreturn_t dp_aux_i2c_handler(struct dp_aux_private *aux, u32 
>> isr)
>>   {
>> +    irqreturn_t ret = IRQ_NONE;
>> +
>>       if (isr & DP_INTR_AUX_I2C_DONE) {
>>           if (isr & (DP_INTR_I2C_NACK | DP_INTR_I2C_DEFER))
>>               aux->aux_error_num = DP_AUX_ERR_NACK;
>>           else
>>               aux->aux_error_num = DP_AUX_ERR_NONE;
>> -    } else {
>> -        if (isr & DP_INTR_WRONG_ADDR)
>> -            aux->aux_error_num = DP_AUX_ERR_ADDR;
>> -        else if (isr & DP_INTR_TIMEOUT)
>> -            aux->aux_error_num = DP_AUX_ERR_TOUT;
>> -        if (isr & DP_INTR_NACK_DEFER)
>> -            aux->aux_error_num = DP_AUX_ERR_NACK_DEFER;
>> -        if (isr & DP_INTR_I2C_NACK)
>> -            aux->aux_error_num = DP_AUX_ERR_NACK;
>> -        if (isr & DP_INTR_I2C_DEFER)
>> -            aux->aux_error_num = DP_AUX_ERR_DEFER;
>> -        if (isr & DP_INTR_AUX_ERROR) {
>> -            aux->aux_error_num = DP_AUX_ERR_PHY;
>> -            dp_catalog_aux_clear_hw_interrupts(aux->catalog);
>> -        }
>> +
>> +        return IRQ_HANDLED;
>> +    }
>> +
>> +    if (isr & DP_INTR_WRONG_ADDR) {
>> +        aux->aux_error_num = DP_AUX_ERR_ADDR;
>> +        ret = IRQ_HANDLED;
>> +    } else if (isr & DP_INTR_TIMEOUT) {
>> +        aux->aux_error_num = DP_AUX_ERR_TOUT;
>> +        ret = IRQ_HANDLED;
>>       }
>> +
>> +    if (isr & DP_INTR_NACK_DEFER) {
>> +        aux->aux_error_num = DP_AUX_ERR_NACK_DEFER;
>> +        ret = IRQ_HANDLED;
>> +    }
>> +
>> +    if (isr & DP_INTR_I2C_NACK) {
>> +        aux->aux_error_num = DP_AUX_ERR_NACK;
>> +        ret = IRQ_HANDLED;
>> +    }
>> +
>> +    if (isr & DP_INTR_I2C_DEFER) {
>> +        aux->aux_error_num = DP_AUX_ERR_DEFER;
>> +        ret = IRQ_HANDLED;
>> +    }
>> +
>> +    if (isr & DP_INTR_AUX_ERROR) {
>> +        aux->aux_error_num = DP_AUX_ERR_PHY;
>> +        dp_catalog_aux_clear_hw_interrupts(aux->catalog);
>> +        ret = IRQ_HANDLED;
>> +    }
>> +
>> +    return ret;
>>   }
>>   static void dp_aux_update_offset_and_segment(struct dp_aux_private 
>> *aux,
>> @@ -413,6 +446,7 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)
>>   {
>>       u32 isr;
>>       struct dp_aux_private *aux;
>> +    irqreturn_t ret = IRQ_NONE;
> 
> No need to assign a value here. It will be overwritten in both of code 
> branches.
> 
>>       if (!dp_aux) {
>>           DRM_ERROR("invalid input\n");
>> @@ -423,15 +457,20 @@ 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;
>> +
> 
> A separate commit please.
> 
>>       if (!aux->cmd_busy)
>>           return;
>>       if (aux->native)
>> -        dp_aux_native_handler(aux, isr);
>> +        ret = dp_aux_native_handler(aux, isr);
>>       else
>> -        dp_aux_i2c_handler(aux, isr);
>> +        ret = dp_aux_i2c_handler(aux, isr);
>> -    complete(&aux->comp);
>> +    if (ret == IRQ_HANDLED)
>> +        complete(&aux->comp);
> 
> Can you just move the complete() into the individual handling functions? 
> Then you won't have to return the error code from dp_aux_*_handler() at 
> all. You can check `isr' in that function and call complete if there was 
> any error.
> 
> Also could you please describe, why is it necessary to complete() 
> condition at all? Judging from your commit message the `if (!isr) 
> return;' part should be enough.
> 

Yes, and thats why I wrote that from a functionality standpoint, the 
original v1 posted was enough to fix this issue.

But, this version is better because of the irq return value.

So, if IRQ_HANDLED Vs IRQ_NONE handling is better with this.

>>   }
>>   void dp_aux_reconfig(struct drm_dp_aux *dp_aux)
>