| Message ID | 20220824130034.196041-2-tomi.valkeinen@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: ti-sn65dsi86: Basic DP support | expand |
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)
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>
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 --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)