diff mbox series

[v5,1/4] drm/bridge: ti-sn65dsi86: check AUX errors better

Message ID 20220824130034.196041-2-tomi.valkeinen@ideasonboard.com (mailing list archive)
State Superseded
Delegated to: Kieran Bingham
Headers show
Series drm/bridge: ti-sn65dsi86: Basic DP support | expand

Commit Message

Tomi Valkeinen Aug. 24, 2022, 1 p.m. UTC
From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

The driver does not check AUX_IRQ_STATUS_NAT_I2C_FAIL bit at all when
sending AUX transfers, which leads to the driver not returning an error.

Add the check.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Laurent Pinchart Aug. 27, 2022, 1:04 a.m. UTC | #1
Hi Tomi,

Thank you for the patch.

On Wed, Aug 24, 2022 at 04:00:31PM +0300, Tomi Valkeinen wrote:
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> The driver does not check AUX_IRQ_STATUS_NAT_I2C_FAIL bit at all when
> sending AUX transfers, which leads to the driver not returning an error.
> 
> Add the check.

That looks right.

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 90bbabde1595..ba84215c1511 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -582,6 +582,11 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
>  		goto exit;
>  	}
>  
> +	if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) {
> +		ret = -EIO;
> +		goto exit;
> +	}
> +
>  	if (val & AUX_IRQ_STATUS_AUX_SHORT) {
>  		ret = regmap_read(pdata->regmap, SN_AUX_LENGTH_REG, &len);
>  		if (ret)
Doug Anderson Aug. 29, 2022, 5:15 p.m. UTC | #2
Hi,

On Wed, Aug 24, 2022 at 6:01 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>
> The driver does not check AUX_IRQ_STATUS_NAT_I2C_FAIL bit at all when
> sending AUX transfers,

It doesn't? What about a few lines down from where your patch modifies
that reads:

  else if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) {

That seems like it's checking that bit?


> which leads to the driver not returning an error.

Right that it doesn't return an error. I guess the question is: should
it? Right now it sets the proper reply (DP_AUX_I2C_REPLY_NACK or
DP_AUX_NATIVE_REPLY_NACK) and returns 0. Is it supposed to be
returning an error code? What problem are you fixing?

In commit 982f589bde7a ("drm/bridge: ti-sn65dsi86: Update reply on aux
failures"), at least, we thought that returning "0" and setting the
"reply" was the correct thing to do (though we didn't have any good
setup to test all the error paths).

...and looking through the code at drm_dp_i2c_do_msg(), I see that it
only ever looks at DP_AUX_I2C_REPLY_NACK if "ret" was 0.

So I guess I would say:

1. Your patch doesn't seem right to me.

2. If your patch is actually fixing a problem, you should at least
modify it so that it doesn't create dead code (the old handling of
AUX_IRQ_STATUS_NAT_I2C_FAIL is no longer reachable after your patch.

Naked-by: Douglas Anderson <dianders@chromium.org>
Tomi Valkeinen Aug. 30, 2022, 9:44 a.m. UTC | #3
Hi,

On 29/08/2022 20:15, Doug Anderson wrote:
> Hi,
> 
> On Wed, Aug 24, 2022 at 6:01 AM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>>
>> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>>
>> The driver does not check AUX_IRQ_STATUS_NAT_I2C_FAIL bit at all when
>> sending AUX transfers,
> 
> It doesn't? What about a few lines down from where your patch modifies
> that reads:
> 
>    else if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) {
> 
> That seems like it's checking that bit?

You're right, the patch is obviously broken.

>> which leads to the driver not returning an error.
> 
> Right that it doesn't return an error. I guess the question is: should
> it? Right now it sets the proper reply (DP_AUX_I2C_REPLY_NACK or
> DP_AUX_NATIVE_REPLY_NACK) and returns 0. Is it supposed to be
> returning an error code? What problem are you fixing?

I encountered a problem where the monitor was not replying properly and 
the driver was just getting AUX_IRQ_STATUS_NAT_I2C_FAIL errors, but the 
drm_dp_dpcd_read functions were not returning an error and the driver 
didn't understand that none of the transactions are actually going through.

Of course, now that I try I'm unable to reproduce the situation, I can 
only see AUX_IRQ_STATUS_AUX_RPLY_TOUT when something is not right, never 
AUX_IRQ_STATUS_NAT_I2C_FAIL.

Looking at the code, AUX_IRQ_STATUS_NAT_I2C_FAIL sets the len to 0, so 
that should cause a failure when the driver compares the return value of 
drm_dp_dpcd_read to the expected number of bytes. So I have trouble 
understanding the behavior I saw.

I did have WIP IRQ code in my branch at that time, though, and I'm now 
kind of suspecting that code, as it also somehow triggers the DSI RX 
issues I mention in the other mail.

> In commit 982f589bde7a ("drm/bridge: ti-sn65dsi86: Update reply on aux
> failures"), at least, we thought that returning "0" and setting the
> "reply" was the correct thing to do (though we didn't have any good
> setup to test all the error paths).
> 
> ...and looking through the code at drm_dp_i2c_do_msg(), I see that it
> only ever looks at DP_AUX_I2C_REPLY_NACK if "ret" was 0.
> 
> So I guess I would say:
> 
> 1. Your patch doesn't seem right to me.
> 
> 2. If your patch is actually fixing a problem, you should at least
> modify it so that it doesn't create dead code (the old handling of
> AUX_IRQ_STATUS_NAT_I2C_FAIL is no longer reachable after your patch.
> 
> Naked-by: Douglas Anderson <dianders@chromium.org>

I'll drop this patch.

  Tomi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 90bbabde1595..ba84215c1511 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -582,6 +582,11 @@  static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 		goto exit;
 	}
 
+	if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) {
+		ret = -EIO;
+		goto exit;
+	}
+
 	if (val & AUX_IRQ_STATUS_AUX_SHORT) {
 		ret = regmap_read(pdata->regmap, SN_AUX_LENGTH_REG, &len);
 		if (ret)