| Message ID | 20220213022648.495895-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] drm/bridge: ti-sn65dsi83: Check link status register after enabling the bridge | expand |
On Sun, 13 Feb 2022 at 03:27, Marek Vasut <marex@denx.de> wrote: > > In rare cases, the bridge may not start up correctly, which usually > leads to no display output. In case this happens, warn about it in > the kernel log. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Jagan Teki <jagan@amarulasolutions.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Robert Foss <robert.foss@linaro.org> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: dri-devel@lists.freedesktop.org > --- > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > index 19daaddd29a41..1d7c154ea1d79 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > @@ -488,6 +488,11 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge, > /* Clear all errors that got asserted during initialization. */ > regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); > regmap_write(ctx->regmap, REG_IRQ_STAT, pval); > + > + usleep_range(10000, 12000); > + regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); > + if (pval) > + dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval); > } > > static void sn65dsi83_atomic_disable(struct drm_bridge *bridge, > -- > 2.34.1 > Reviewed-by: Robert Foss <robert.foss@linaro.org>
On Thu, 31 Mar 2022 at 17:32, Robert Foss <robert.foss@linaro.org> wrote: > > On Sun, 13 Feb 2022 at 03:27, Marek Vasut <marex@denx.de> wrote: > > > > In rare cases, the bridge may not start up correctly, which usually > > leads to no display output. In case this happens, warn about it in > > the kernel log. > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Jagan Teki <jagan@amarulasolutions.com> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Cc: Linus Walleij <linus.walleij@linaro.org> > > Cc: Robert Foss <robert.foss@linaro.org> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Cc: dri-devel@lists.freedesktop.org > > --- > > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > index 19daaddd29a41..1d7c154ea1d79 100644 > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > @@ -488,6 +488,11 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge, > > /* Clear all errors that got asserted during initialization. */ > > regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); > > regmap_write(ctx->regmap, REG_IRQ_STAT, pval); > > + > > + usleep_range(10000, 12000); > > + regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); > > + if (pval) > > + dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval); > > } > > > > static void sn65dsi83_atomic_disable(struct drm_bridge *bridge, > > -- > > 2.34.1 > > > > Reviewed-by: Robert Foss <robert.foss@linaro.org> Applied to drm-misc-next.
Hi Marek, Thank you for the patch. On Sun, Feb 13, 2022 at 03:26:48AM +0100, Marek Vasut wrote: > In rare cases, the bridge may not start up correctly, which usually > leads to no display output. In case this happens, warn about it in > the kernel log. Do you know what this is caused by ? It's a bit annoying to add a 10+ms delay at start time just to be notified of rare cases. > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Jagan Teki <jagan@amarulasolutions.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Robert Foss <robert.foss@linaro.org> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: dri-devel@lists.freedesktop.org > --- > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > index 19daaddd29a41..1d7c154ea1d79 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > @@ -488,6 +488,11 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge, > /* Clear all errors that got asserted during initialization. */ > regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); > regmap_write(ctx->regmap, REG_IRQ_STAT, pval); > + > + usleep_range(10000, 12000); > + regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); > + if (pval) > + dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval); > } > > static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
On 4/5/22 13:42, Laurent Pinchart wrote: > Hi Marek, Hi, > Thank you for the patch. > > On Sun, Feb 13, 2022 at 03:26:48AM +0100, Marek Vasut wrote: >> In rare cases, the bridge may not start up correctly, which usually >> leads to no display output. In case this happens, warn about it in >> the kernel log. > > Do you know what this is caused by ? It's a bit annoying to add a 10+ms > delay at start time just to be notified of rare cases. Could be anything, broken DSI bridge driver, misconfigured DSI link, etc. This at least informs the user that the bridge failed to come up, before this patch, there was no information about such failure.
Hi Laurent On Tue, 5 Apr 2022 at 12:42, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Marek, > > Thank you for the patch. > > On Sun, Feb 13, 2022 at 03:26:48AM +0100, Marek Vasut wrote: > > In rare cases, the bridge may not start up correctly, which usually > > leads to no display output. In case this happens, warn about it in > > the kernel log. > > Do you know what this is caused by ? It's a bit annoying to add a 10+ms > delay at start time just to be notified of rare cases. The datasheet [1] section 7.4.2 Initialization Sequence states in step 2 "After power is applied and stable, the DSI CLK lanes MUST be in HS state and the DSI data lanes MUST be driven to LP11 state" Data lanes shouldn't go to HS until step 8 after the DSI83 has been configured. Configuration from the driver is being done from atomic_enable, therefore the data lanes are likely in HS mode and sending video, not LP11. Deviate from the specified initialisation sequence at your peril! The SN65DSI8[3|4|5] is one of the DSI devices that I'd been looking at with the DSI ordering patches [2] so that we could initialise it in the way specified in the datasheet. I've had no responses to v2 of those patches though. Dave [1] https://www.ti.com/lit/ds/symlink/sn65dsi83.pdf [2] https://patchwork.freedesktop.org/series/100252/#rev2 > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Jagan Teki <jagan@amarulasolutions.com> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Cc: Linus Walleij <linus.walleij@linaro.org> > > Cc: Robert Foss <robert.foss@linaro.org> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Cc: dri-devel@lists.freedesktop.org > > --- > > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > index 19daaddd29a41..1d7c154ea1d79 100644 > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > @@ -488,6 +488,11 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge, > > /* Clear all errors that got asserted during initialization. */ > > regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); > > regmap_write(ctx->regmap, REG_IRQ_STAT, pval); > > + > > + usleep_range(10000, 12000); > > + regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); > > + if (pval) > > + dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval); > > } > > > > static void sn65dsi83_atomic_disable(struct drm_bridge *bridge, > > -- > Regards, > > Laurent Pinchart
Hi Dave, On Tue, Apr 05, 2022 at 01:00:28PM +0100, Dave Stevenson wrote: > On Tue, 5 Apr 2022 at 12:42, Laurent Pinchart wrote: > > On Sun, Feb 13, 2022 at 03:26:48AM +0100, Marek Vasut wrote: > > > In rare cases, the bridge may not start up correctly, which usually > > > leads to no display output. In case this happens, warn about it in > > > the kernel log. > > > > Do you know what this is caused by ? It's a bit annoying to add a 10+ms > > delay at start time just to be notified of rare cases. > > The datasheet [1] section 7.4.2 Initialization Sequence states in step 2 > "After power is applied and stable, the DSI CLK lanes MUST be in HS > state and the DSI data lanes MUST be driven > to LP11 state" > Data lanes shouldn't go to HS until step 8 after the DSI83 has been configured. > > Configuration from the driver is being done from atomic_enable, > therefore the data lanes are likely in HS mode and sending video, not > LP11. > > Deviate from the specified initialisation sequence at your peril! > > The SN65DSI8[3|4|5] is one of the DSI devices that I'd been looking at > with the DSI ordering patches [2] so that we could initialise it in > the way specified in the datasheet. I've had no responses to v2 of > those patches though. Sounds like I need to review that :-) It's still in my queue, I'll try to push it to the top. Do you think this patch could then be reverted ? > [1] https://www.ti.com/lit/ds/symlink/sn65dsi83.pdf > [2] https://patchwork.freedesktop.org/series/100252/#rev2 > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > > Cc: Jagan Teki <jagan@amarulasolutions.com> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Cc: Linus Walleij <linus.walleij@linaro.org> > > > Cc: Robert Foss <robert.foss@linaro.org> > > > Cc: Sam Ravnborg <sam@ravnborg.org> > > > Cc: dri-devel@lists.freedesktop.org > > > --- > > > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > index 19daaddd29a41..1d7c154ea1d79 100644 > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > @@ -488,6 +488,11 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge, > > > /* Clear all errors that got asserted during initialization. */ > > > regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); > > > regmap_write(ctx->regmap, REG_IRQ_STAT, pval); > > > + > > > + usleep_range(10000, 12000); > > > + regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); > > > + if (pval) > > > + dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval); > > > } > > > > > > static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
On Tue, 5 Apr 2022 at 14:08, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Dave, > > On Tue, Apr 05, 2022 at 01:00:28PM +0100, Dave Stevenson wrote: > > On Tue, 5 Apr 2022 at 12:42, Laurent Pinchart wrote: > > > On Sun, Feb 13, 2022 at 03:26:48AM +0100, Marek Vasut wrote: > > > > In rare cases, the bridge may not start up correctly, which usually > > > > leads to no display output. In case this happens, warn about it in > > > > the kernel log. > > > > > > Do you know what this is caused by ? It's a bit annoying to add a 10+ms > > > delay at start time just to be notified of rare cases. > > > > The datasheet [1] section 7.4.2 Initialization Sequence states in step 2 > > "After power is applied and stable, the DSI CLK lanes MUST be in HS > > state and the DSI data lanes MUST be driven > > to LP11 state" > > Data lanes shouldn't go to HS until step 8 after the DSI83 has been configured. > > > > Configuration from the driver is being done from atomic_enable, > > therefore the data lanes are likely in HS mode and sending video, not > > LP11. > > > > Deviate from the specified initialisation sequence at your peril! > > > > The SN65DSI8[3|4|5] is one of the DSI devices that I'd been looking at > > with the DSI ordering patches [2] so that we could initialise it in > > the way specified in the datasheet. I've had no responses to v2 of > > those patches though. > > Sounds like I need to review that :-) It's still in my queue, I'll try > to push it to the top. > > Do you think this patch could then be reverted ? If we can initialise the DSI host before the bridge for the pre_enable, then all the configuration moves to the atomic_pre_enable and there should be no need to have the delay. I can't 100% guarantee that, but one of the folks on the Pi forums is using [1] which does that, and is reporting it working well. (He's also using the DSI85 to take 2 DSI links and drive 2 LVDS single link panels) Dave [1] https://github.com/6by9/linux/commits/rpi-5.15.y-sn65dsi8x > > [1] https://www.ti.com/lit/ds/symlink/sn65dsi83.pdf > > [2] https://patchwork.freedesktop.org/series/100252/#rev2 > > > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > > > Cc: Jagan Teki <jagan@amarulasolutions.com> > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Cc: Linus Walleij <linus.walleij@linaro.org> > > > > Cc: Robert Foss <robert.foss@linaro.org> > > > > Cc: Sam Ravnborg <sam@ravnborg.org> > > > > Cc: dri-devel@lists.freedesktop.org > > > > --- > > > > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > index 19daaddd29a41..1d7c154ea1d79 100644 > > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > @@ -488,6 +488,11 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge, > > > > /* Clear all errors that got asserted during initialization. */ > > > > regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); > > > > regmap_write(ctx->regmap, REG_IRQ_STAT, pval); > > > > + > > > > + usleep_range(10000, 12000); > > > > + regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); > > > > + if (pval) > > > > + dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval); > > > > } > > > > > > > > static void sn65dsi83_atomic_disable(struct drm_bridge *bridge, > > -- > Regards, > > Laurent Pinchart
On 4/5/22 15:25, Dave Stevenson wrote: > On Tue, 5 Apr 2022 at 14:08, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: >> >> Hi Dave, >> >> On Tue, Apr 05, 2022 at 01:00:28PM +0100, Dave Stevenson wrote: >>> On Tue, 5 Apr 2022 at 12:42, Laurent Pinchart wrote: >>>> On Sun, Feb 13, 2022 at 03:26:48AM +0100, Marek Vasut wrote: >>>>> In rare cases, the bridge may not start up correctly, which usually >>>>> leads to no display output. In case this happens, warn about it in >>>>> the kernel log. >>>> >>>> Do you know what this is caused by ? It's a bit annoying to add a 10+ms >>>> delay at start time just to be notified of rare cases. >>> >>> The datasheet [1] section 7.4.2 Initialization Sequence states in step 2 >>> "After power is applied and stable, the DSI CLK lanes MUST be in HS >>> state and the DSI data lanes MUST be driven >>> to LP11 state" >>> Data lanes shouldn't go to HS until step 8 after the DSI83 has been configured. >>> >>> Configuration from the driver is being done from atomic_enable, >>> therefore the data lanes are likely in HS mode and sending video, not >>> LP11. >>> >>> Deviate from the specified initialisation sequence at your peril! >>> >>> The SN65DSI8[3|4|5] is one of the DSI devices that I'd been looking at >>> with the DSI ordering patches [2] so that we could initialise it in >>> the way specified in the datasheet. I've had no responses to v2 of >>> those patches though. >> >> Sounds like I need to review that :-) It's still in my queue, I'll try >> to push it to the top. >> >> Do you think this patch could then be reverted ? > > If we can initialise the DSI host before the bridge for the > pre_enable, then all the configuration moves to the atomic_pre_enable > and there should be no need to have the delay. > > I can't 100% guarantee that, but one of the folks on the Pi forums is > using [1] which does that, and is reporting it working well. (He's > also using the DSI85 to take 2 DSI links and drive 2 LVDS single link > panels) It seems to me that checking whether the bridge got correctly initialized is orthogonal to the aforementioned patchset though ?
Hi Marek. On Tue, 5 Apr 2022 at 14:49, Marek Vasut <marex@denx.de> wrote: > > On 4/5/22 15:25, Dave Stevenson wrote: > > On Tue, 5 Apr 2022 at 14:08, Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > >> > >> Hi Dave, > >> > >> On Tue, Apr 05, 2022 at 01:00:28PM +0100, Dave Stevenson wrote: > >>> On Tue, 5 Apr 2022 at 12:42, Laurent Pinchart wrote: > >>>> On Sun, Feb 13, 2022 at 03:26:48AM +0100, Marek Vasut wrote: > >>>>> In rare cases, the bridge may not start up correctly, which usually > >>>>> leads to no display output. In case this happens, warn about it in > >>>>> the kernel log. > >>>> > >>>> Do you know what this is caused by ? It's a bit annoying to add a 10+ms > >>>> delay at start time just to be notified of rare cases. > >>> > >>> The datasheet [1] section 7.4.2 Initialization Sequence states in step 2 > >>> "After power is applied and stable, the DSI CLK lanes MUST be in HS > >>> state and the DSI data lanes MUST be driven > >>> to LP11 state" > >>> Data lanes shouldn't go to HS until step 8 after the DSI83 has been configured. > >>> > >>> Configuration from the driver is being done from atomic_enable, > >>> therefore the data lanes are likely in HS mode and sending video, not > >>> LP11. > >>> > >>> Deviate from the specified initialisation sequence at your peril! > >>> > >>> The SN65DSI8[3|4|5] is one of the DSI devices that I'd been looking at > >>> with the DSI ordering patches [2] so that we could initialise it in > >>> the way specified in the datasheet. I've had no responses to v2 of > >>> those patches though. > >> > >> Sounds like I need to review that :-) It's still in my queue, I'll try > >> to push it to the top. > >> > >> Do you think this patch could then be reverted ? > > > > If we can initialise the DSI host before the bridge for the > > pre_enable, then all the configuration moves to the atomic_pre_enable > > and there should be no need to have the delay. > > > > I can't 100% guarantee that, but one of the folks on the Pi forums is > > using [1] which does that, and is reporting it working well. (He's > > also using the DSI85 to take 2 DSI links and drive 2 LVDS single link > > panels) > > It seems to me that checking whether the bridge got correctly > initialized is orthogonal to the aforementioned patchset though ? It's the delay that is ugly. Put the check in atomic_enable which will be slightly later than configuration in pre_enable? Check that sufficient jiffies have passed if you needed. Or wire up the IRQ line from the SN65DSI83 rather than polling the IRQ Status register. Delayed workqueue if the IRQ isn't wired up. If I read it right your log message is triggered by any bit being set in REG_IRQ_STAT. So an inconveniently timed correctable DSI error will set bit 4 and log the error even though it's been corrected. Likewise bit 7 / CHA_SYNCH_ERR could get triggered by an H or V sync packet being received in that 10-12ms window (we're in atomic_enable, so video is already running). If it's the PLL being unlocked that is the issue then it should only be checking bit 0. Or possibly reading the actual PLL lock status from REG_RC_LVDS_PLL_PLL_EN_STAT. Although as we've already checked that the PLL is locked via REG_RC_LVDS_PLL_PLL_EN_STAT earlier in the atomic_enable, I'm now a little confused as to the condition you are actually wanting to detect. Dave
On 4/5/22 16:20, Dave Stevenson wrote: Hi, >>> If we can initialise the DSI host before the bridge for the >>> pre_enable, then all the configuration moves to the atomic_pre_enable >>> and there should be no need to have the delay. >>> >>> I can't 100% guarantee that, but one of the folks on the Pi forums is >>> using [1] which does that, and is reporting it working well. (He's >>> also using the DSI85 to take 2 DSI links and drive 2 LVDS single link >>> panels) >> >> It seems to me that checking whether the bridge got correctly >> initialized is orthogonal to the aforementioned patchset though ? > > It's the delay that is ugly. You do need to wait a little after the initialization and before checking the status, so that delay is not going away no matter how you frob with the DSI bridge. > Put the check in atomic_enable which will be slightly later than > configuration in pre_enable? Check that sufficient jiffies have passed > if you needed. > Or wire up the IRQ line from the SN65DSI83 rather than polling the IRQ > Status register. Delayed workqueue if the IRQ isn't wired up. Are you able to do such deferred non-atomic operations in atomic_enable callback ? > If I read it right your log message is triggered by any bit being set > in REG_IRQ_STAT. So an inconveniently timed correctable DSI error will > set bit 4 and log the error even though it's been corrected. Likewise > bit 7 / CHA_SYNCH_ERR could get triggered by an H or V sync packet > being received in that 10-12ms window (we're in atomic_enable, so > video is already running). There should be no bits set in the IRQ_STAT register if everything works as it should. > If it's the PLL being unlocked that is the issue then it should only > be checking bit 0. Or possibly reading the actual PLL lock status from > REG_RC_LVDS_PLL_PLL_EN_STAT. Although as we've already checked that > the PLL is locked via REG_RC_LVDS_PLL_PLL_EN_STAT earlier in the > atomic_enable, I'm now a little confused as to the condition you are > actually wanting to detect. Any outstanding errors which are currently hidden and only show up sporadically at the worst possible moment. [...]
On Tue, 5 Apr 2022 at 15:48, Marek Vasut <marex@denx.de> wrote: > > On 4/5/22 16:20, Dave Stevenson wrote: > > Hi, > > >>> If we can initialise the DSI host before the bridge for the > >>> pre_enable, then all the configuration moves to the atomic_pre_enable > >>> and there should be no need to have the delay. > >>> > >>> I can't 100% guarantee that, but one of the folks on the Pi forums is > >>> using [1] which does that, and is reporting it working well. (He's > >>> also using the DSI85 to take 2 DSI links and drive 2 LVDS single link > >>> panels) > >> > >> It seems to me that checking whether the bridge got correctly > >> initialized is orthogonal to the aforementioned patchset though ? > > > > It's the delay that is ugly. > > You do need to wait a little after the initialization and before > checking the status, so that delay is not going away no matter how you > frob with the DSI bridge. > > > Put the check in atomic_enable which will be slightly later than > > configuration in pre_enable? Check that sufficient jiffies have passed > > if you needed. > > Or wire up the IRQ line from the SN65DSI83 rather than polling the IRQ > > Status register. Delayed workqueue if the IRQ isn't wired up. > > Are you able to do such deferred non-atomic operations in atomic_enable > callback ? atomic_enable returns void so you can't fail the atomic_enable. All you're doing is reading a register and potentially logging an error - surely that can be done from any context. > > If I read it right your log message is triggered by any bit being set > > in REG_IRQ_STAT. So an inconveniently timed correctable DSI error will > > set bit 4 and log the error even though it's been corrected. Likewise > > bit 7 / CHA_SYNCH_ERR could get triggered by an H or V sync packet > > being received in that 10-12ms window (we're in atomic_enable, so > > video is already running). > > There should be no bits set in the IRQ_STAT register if everything works > as it should. On a perfect link, yes. Looking at the top 4 bits. Bit 4 CHA_COR_ECC_ERR When the DSI channel A packet processor detects a correctable ECC error, this bit is set. The error is corrected, so why do we care? The display is still working. Bit 5 CHA_UNC_ECC_ERR When the DSI channel A packet processor detects an uncorrectable ECC error, this bit is set; Bit 6 CHA_CRC_ERR When the DSI channel A packet processor detects a data stream CRC error, this bit is set It doesn't say what behaviour the DSI83 will take under these circumstances, but shouldn't be fatal to the display. Bit 7 CHA_SYNCH_ERR When the DSI channel A packet processor detects an HS or VS synchronization error, that is, an unexpected sync packet; this bit is set. It's happened, but shouldn't be fatal, so why do we care? The display should pick up correctly at the start of the next frame. As I've already said, we're in atomic_enable and video is therefore running, this is highly plausibly going to happen. We've delayed for 4-5ms in sn65dsi83_atomic_enable, so we're a third of the way through a frame at 60fps. The chances of seeing a VS or HS packet at an unexpected time is therefore high. Bits 2 & 3 look equally inconsequential. Bit 0 as PLL unlock would cause grief. > > If it's the PLL being unlocked that is the issue then it should only > > be checking bit 0. Or possibly reading the actual PLL lock status from > > REG_RC_LVDS_PLL_PLL_EN_STAT. Although as we've already checked that > > the PLL is locked via REG_RC_LVDS_PLL_PLL_EN_STAT earlier in the > > atomic_enable, I'm now a little confused as to the condition you are > > actually wanting to detect. > > Any outstanding errors which are currently hidden and only show up > sporadically at the worst possible moment. If you were constantly looking at the status then that would be reasonable. To be looking during one specific 10-12ms period of time for some fairly generic status values seems a little redundant, and a waste of time in delaying the atomic_enable. It'll be interesting to see if these events just go away when the initialisation sequence specified in the datasheet is being followed. Let's leave the debate until then, as it's currently fairly arbitrary. Dave
On 4/5/22 17:24, Dave Stevenson wrote: Hi, >>>>> If we can initialise the DSI host before the bridge for the >>>>> pre_enable, then all the configuration moves to the atomic_pre_enable >>>>> and there should be no need to have the delay. >>>>> >>>>> I can't 100% guarantee that, but one of the folks on the Pi forums is >>>>> using [1] which does that, and is reporting it working well. (He's >>>>> also using the DSI85 to take 2 DSI links and drive 2 LVDS single link >>>>> panels) >>>> >>>> It seems to me that checking whether the bridge got correctly >>>> initialized is orthogonal to the aforementioned patchset though ? >>> >>> It's the delay that is ugly. >> >> You do need to wait a little after the initialization and before >> checking the status, so that delay is not going away no matter how you >> frob with the DSI bridge. >> >>> Put the check in atomic_enable which will be slightly later than >>> configuration in pre_enable? Check that sufficient jiffies have passed >>> if you needed. >>> Or wire up the IRQ line from the SN65DSI83 rather than polling the IRQ >>> Status register. Delayed workqueue if the IRQ isn't wired up. >> >> Are you able to do such deferred non-atomic operations in atomic_enable >> callback ? > > atomic_enable returns void so you can't fail the atomic_enable. > All you're doing is reading a register and potentially logging an > error - surely that can be done from any context. > >>> If I read it right your log message is triggered by any bit being set >>> in REG_IRQ_STAT. So an inconveniently timed correctable DSI error will >>> set bit 4 and log the error even though it's been corrected. Likewise >>> bit 7 / CHA_SYNCH_ERR could get triggered by an H or V sync packet >>> being received in that 10-12ms window (we're in atomic_enable, so >>> video is already running). >> >> There should be no bits set in the IRQ_STAT register if everything works >> as it should. > > On a perfect link, yes. If your hardware is broken, then you really do want to know about it. > Looking at the top 4 bits. > > Bit 4 > CHA_COR_ECC_ERR > When the DSI channel A packet processor detects a correctable ECC > error, this bit is set. > > The error is corrected, so why do we care? The display is still working. If you get a lot of those correctable errors, your display might not work at all. So we do care. > Bit 5 > CHA_UNC_ECC_ERR > When the DSI channel A packet processor detects an uncorrectable ECC > error, this bit is set; > Bit 6 > CHA_CRC_ERR > When the DSI channel A packet processor detects a data stream CRC error, > this bit is set > > It doesn't say what behaviour the DSI83 will take under these > circumstances, but shouldn't be fatal to the display. See above, it is an error, hardware is broken, you want to know about this and fix the hardware. > Bit 7 > CHA_SYNCH_ERR > When the DSI channel A packet processor detects an HS or VS > synchronization error, that is, an unexpected sync packet; this bit is > set. > > It's happened, but shouldn't be fatal, so why do we care? The display > should pick up correctly at the start of the next frame. Or maybe it won't because of noise on the DSI link, fix the hardware. Sorry, I am not happy about hiding errors and trying to somehow justify they are OK. They are not, the hardware is likely broken and it should be fixed, that is the right way to handle these errors. > As I've already said, we're in atomic_enable and video is therefore > running, this is highly plausibly going to happen. We've delayed for > 4-5ms in sn65dsi83_atomic_enable, so we're a third of the way through > a frame at 60fps. The chances of seeing a VS or HS packet at an > unexpected time is therefore high. > > Bits 2 & 3 look equally inconsequential. > > Bit 0 as PLL unlock would cause grief. > >>> If it's the PLL being unlocked that is the issue then it should only >>> be checking bit 0. Or possibly reading the actual PLL lock status from >>> REG_RC_LVDS_PLL_PLL_EN_STAT. Although as we've already checked that >>> the PLL is locked via REG_RC_LVDS_PLL_PLL_EN_STAT earlier in the >>> atomic_enable, I'm now a little confused as to the condition you are >>> actually wanting to detect. >> >> Any outstanding errors which are currently hidden and only show up >> sporadically at the worst possible moment. > > If you were constantly looking at the status then that would be reasonable. > To be looking during one specific 10-12ms period of time for some > fairly generic status values seems a little redundant, and a waste of > time in delaying the atomic_enable. Sorry, I disagree. I think 10ms extra time in atomic_enable() is a good trade-off for knowing about possible hardware problems sooner rather than later. > It'll be interesting to see if these events just go away when the > initialisation sequence specified in the datasheet is being followed. > Let's leave the debate until then, as it's currently fairly arbitrary. No, your patch series is orthogonal to this patch.
Am 05.04.22 um 23:36 schrieb Marek Vasut: > On 4/5/22 17:24, Dave Stevenson wrote: > > Hi, > >>>>>> If we can initialise the DSI host before the bridge for the >>>>>> pre_enable, then all the configuration moves to the atomic_pre_enable >>>>>> and there should be no need to have the delay. >>>>>> >>>>>> I can't 100% guarantee that, but one of the folks on the Pi forums is >>>>>> using [1] which does that, and is reporting it working well. (He's >>>>>> also using the DSI85 to take 2 DSI links and drive 2 LVDS single link >>>>>> panels) >>>>> >>>>> It seems to me that checking whether the bridge got correctly >>>>> initialized is orthogonal to the aforementioned patchset though ? >>>> >>>> It's the delay that is ugly. >>> >>> You do need to wait a little after the initialization and before >>> checking the status, so that delay is not going away no matter how you >>> frob with the DSI bridge. >>> >>>> Put the check in atomic_enable which will be slightly later than >>>> configuration in pre_enable? Check that sufficient jiffies have passed >>>> if you needed. >>>> Or wire up the IRQ line from the SN65DSI83 rather than polling the IRQ >>>> Status register. Delayed workqueue if the IRQ isn't wired up. >>> >>> Are you able to do such deferred non-atomic operations in atomic_enable >>> callback ? >> >> atomic_enable returns void so you can't fail the atomic_enable. >> All you're doing is reading a register and potentially logging an >> error - surely that can be done from any context. >> >>>> If I read it right your log message is triggered by any bit being set >>>> in REG_IRQ_STAT. So an inconveniently timed correctable DSI error will >>>> set bit 4 and log the error even though it's been corrected. Likewise >>>> bit 7 / CHA_SYNCH_ERR could get triggered by an H or V sync packet >>>> being received in that 10-12ms window (we're in atomic_enable, so >>>> video is already running). >>> >>> There should be no bits set in the IRQ_STAT register if everything works >>> as it should. >> >> On a perfect link, yes. > > If your hardware is broken, then you really do want to know about it. > >> Looking at the top 4 bits. >> >> Bit 4 >> CHA_COR_ECC_ERR >> When the DSI channel A packet processor detects a correctable ECC >> error, this bit is set. >> >> The error is corrected, so why do we care? The display is still working. > > If you get a lot of those correctable errors, your display might not > work at all. So we do care. > >> Bit 5 >> CHA_UNC_ECC_ERR >> When the DSI channel A packet processor detects an uncorrectable ECC >> error, this bit is set; >> Bit 6 >> CHA_CRC_ERR >> When the DSI channel A packet processor detects a data stream CRC error, >> this bit is set >> >> It doesn't say what behaviour the DSI83 will take under these >> circumstances, but shouldn't be fatal to the display. > > See above, it is an error, hardware is broken, you want to know about > this and fix the hardware. > >> Bit 7 >> CHA_SYNCH_ERR >> When the DSI channel A packet processor detects an HS or VS >> synchronization error, that is, an unexpected sync packet; this bit is >> set. >> >> It's happened, but shouldn't be fatal, so why do we care? The display >> should pick up correctly at the start of the next frame. > > Or maybe it won't because of noise on the DSI link, fix the hardware. > > Sorry, I am not happy about hiding errors and trying to somehow justify > they are OK. They are not, the hardware is likely broken and it should > be fixed, that is the right way to handle these errors. > >> As I've already said, we're in atomic_enable and video is therefore >> running, this is highly plausibly going to happen. We've delayed for >> 4-5ms in sn65dsi83_atomic_enable, so we're a third of the way through >> a frame at 60fps. The chances of seeing a VS or HS packet at an >> unexpected time is therefore high. >> >> Bits 2 & 3 look equally inconsequential. >> >> Bit 0 as PLL unlock would cause grief. >> >>>> If it's the PLL being unlocked that is the issue then it should only >>>> be checking bit 0. Or possibly reading the actual PLL lock status from >>>> REG_RC_LVDS_PLL_PLL_EN_STAT. Although as we've already checked that >>>> the PLL is locked via REG_RC_LVDS_PLL_PLL_EN_STAT earlier in the >>>> atomic_enable, I'm now a little confused as to the condition you are >>>> actually wanting to detect. >>> >>> Any outstanding errors which are currently hidden and only show up >>> sporadically at the worst possible moment. >> >> If you were constantly looking at the status then that would be >> reasonable. >> To be looking during one specific 10-12ms period of time for some >> fairly generic status values seems a little redundant, and a waste of >> time in delaying the atomic_enable. > > Sorry, I disagree. I think 10ms extra time in atomic_enable() is a good > trade-off for knowing about possible hardware problems sooner rather > than later. Isn't the delay at this point even required (regardless of the debugging information), as the init sequence in the datasheet mentions a 10 ms delay after issuing a soft reset and before the DSI stream is enabled? Or is this handled elsewhere? > >> It'll be interesting to see if these events just go away when the >> initialisation sequence specified in the datasheet is being followed. >> Let's leave the debate until then, as it's currently fairly arbitrary. > > No, your patch series is orthogonal to this patch. > >
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 19daaddd29a41..1d7c154ea1d79 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -488,6 +488,11 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge, /* Clear all errors that got asserted during initialization. */ regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); regmap_write(ctx->regmap, REG_IRQ_STAT, pval); + + usleep_range(10000, 12000); + regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); + if (pval) + dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval); } static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
In rare cases, the bridge may not start up correctly, which usually leads to no display output. In case this happens, warn about it in the kernel log. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Jagan Teki <jagan@amarulasolutions.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Robert Foss <robert.foss@linaro.org> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: dri-devel@lists.freedesktop.org --- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 +++++ 1 file changed, 5 insertions(+)