* [PATCH] drm/bridge: adv7511: Reset registers on hotplug @ 2018-07-03 16:56 Sean Paul 2018-07-03 18:58 ` Rob Clark 0 siblings, 1 reply; 8+ messages in thread From: Sean Paul @ 2018-07-03 16:56 UTC (permalink / raw) To: dri-devel, robdclark, linux-arm-msm; +Cc: Laurent Pinchart The bridge loses its hw state when the cable is unplugged. If we detect this case in the hpd handler, reset its state. Reported-by: Rob Clark <robdclark@gmail.com> Signed-off-by: Sean Paul <seanpaul@chromium.org> --- This is the follow-up to "drm/msm: Disable mdp5 crtc when there are no active planes". I incorrectly assumed the modeset was required from the msm side, but Rob pointed out that he thought it might be a bridge issue. drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 73021b388e12..dd3ff2f2cdce 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -429,6 +429,18 @@ static void adv7511_hpd_work(struct work_struct *work) else status = connector_status_disconnected; + /* + * The bridge resets its registers on unplug. So when we get a plug + * event and we're already supposed to be powered, cycle the bridge to + * restore its state. + */ + if (status == connector_status_connected && + adv7511->connector.status == connector_status_disconnected && + adv7511->powered) { + regcache_mark_dirty(adv7511->regmap); + adv7511_power_on(adv7511); + } + if (adv7511->connector.status != status) { adv7511->connector.status = status; if (status == connector_status_disconnected) -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/bridge: adv7511: Reset registers on hotplug 2018-07-03 16:56 [PATCH] drm/bridge: adv7511: Reset registers on hotplug Sean Paul @ 2018-07-03 18:58 ` Rob Clark 2018-07-03 19:03 ` Sean Paul 2018-07-04 13:23 ` Archit Taneja 0 siblings, 2 replies; 8+ messages in thread From: Rob Clark @ 2018-07-03 18:58 UTC (permalink / raw) To: Sean Paul; +Cc: linux-arm-msm, Laurent Pinchart, dri-devel On Tue, Jul 3, 2018 at 12:56 PM, Sean Paul <seanpaul@chromium.org> wrote: > The bridge loses its hw state when the cable is unplugged. If we detect > this case in the hpd handler, reset its state. > > Reported-by: Rob Clark <robdclark@gmail.com> > Signed-off-by: Sean Paul <seanpaul@chromium.org> Tested-by: Rob Clark <robdclark@gmail.com> > --- > This is the follow-up to "drm/msm: Disable mdp5 crtc when there are no > active planes". I incorrectly assumed the modeset was required from the > msm side, but Rob pointed out that he thought it might be a bridge > issue. To elaborate, this is an atomic userspace (weston), when it sees the display disconnected, it removes the planes from the CRTC but does not disable the CRTC. So drm/msm sets up the LM to scanout a solid color, and leaves the encoder and dsi cranking along happily. When replugging the display, the atomic helpers see the CRTC is still active and (correctly) doesn't do a full modeset. So the bridge is not re-configured/re-enabled. Arguably this perhaps isn't what weston *wanted* to do, but in the end the bug is in the bridge. We could possibly set the connector link_status to BAD as an alternative. But at least the build of weston I'm using atm doesn't handle the link_status propery, this approach requires each atomic userspace to handle this case. Compared to Sean's approach which is transparent to userspace, which seems attractive. BR, -R > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index 73021b388e12..dd3ff2f2cdce 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -429,6 +429,18 @@ static void adv7511_hpd_work(struct work_struct *work) > else > status = connector_status_disconnected; > > + /* > + * The bridge resets its registers on unplug. So when we get a plug > + * event and we're already supposed to be powered, cycle the bridge to > + * restore its state. > + */ > + if (status == connector_status_connected && > + adv7511->connector.status == connector_status_disconnected && > + adv7511->powered) { > + regcache_mark_dirty(adv7511->regmap); > + adv7511_power_on(adv7511); > + } > + > if (adv7511->connector.status != status) { > adv7511->connector.status = status; > if (status == connector_status_disconnected) > -- > Sean Paul, Software Engineer, Google / Chromium OS > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/bridge: adv7511: Reset registers on hotplug 2018-07-03 18:58 ` Rob Clark @ 2018-07-03 19:03 ` Sean Paul 2018-07-04 13:23 ` Archit Taneja 1 sibling, 0 replies; 8+ messages in thread From: Sean Paul @ 2018-07-03 19:03 UTC (permalink / raw) To: Rob Clark; +Cc: linux-arm-msm, Laurent Pinchart, dri-devel On Tue, Jul 3, 2018 at 2:58 PM Rob Clark <robdclark@gmail.com> wrote: > > On Tue, Jul 3, 2018 at 12:56 PM, Sean Paul <seanpaul@chromium.org> wrote: > > The bridge loses its hw state when the cable is unplugged. If we detect > > this case in the hpd handler, reset its state. > > > > Reported-by: Rob Clark <robdclark@gmail.com> > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > Tested-by: Rob Clark <robdclark@gmail.com> > > > --- > > This is the follow-up to "drm/msm: Disable mdp5 crtc when there are no > > active planes". I incorrectly assumed the modeset was required from the > > msm side, but Rob pointed out that he thought it might be a bridge > > issue. > > To elaborate, this is an atomic userspace (weston), when it sees the > display disconnected, it removes the planes from the CRTC but does not > disable the CRTC. So drm/msm sets up the LM to scanout a solid color, > and leaves the encoder and dsi cranking along happily. When > replugging the display, the atomic helpers see the CRTC is still > active and (correctly) doesn't do a full modeset. So the bridge is > not re-configured/re-enabled. > > Arguably this perhaps isn't what weston *wanted* to do, but in the end > the bug is in the bridge. > Fwiw, I've also filed https://gitlab.freedesktop.org/wayland/weston/issues/123 against weston so they can save some power on disconnected displays. But yeah, multiple pieces all working against each other here. Sean > We could possibly set the connector link_status to BAD as an > alternative. But at least the build of weston I'm using atm doesn't > handle the link_status propery, this approach requires each atomic > userspace to handle this case. Compared to Sean's approach which is > transparent to userspace, which seems attractive. > > BR, > -R > > > > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > index 73021b388e12..dd3ff2f2cdce 100644 > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > @@ -429,6 +429,18 @@ static void adv7511_hpd_work(struct work_struct *work) > > else > > status = connector_status_disconnected; > > > > + /* > > + * The bridge resets its registers on unplug. So when we get a plug > > + * event and we're already supposed to be powered, cycle the bridge to > > + * restore its state. > > + */ > > + if (status == connector_status_connected && > > + adv7511->connector.status == connector_status_disconnected && > > + adv7511->powered) { > > + regcache_mark_dirty(adv7511->regmap); > > + adv7511_power_on(adv7511); > > + } > > + > > if (adv7511->connector.status != status) { > > adv7511->connector.status = status; > > if (status == connector_status_disconnected) > > -- > > Sean Paul, Software Engineer, Google / Chromium OS > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/bridge: adv7511: Reset registers on hotplug 2018-07-03 18:58 ` Rob Clark 2018-07-03 19:03 ` Sean Paul @ 2018-07-04 13:23 ` Archit Taneja 2018-07-04 14:06 ` Rob Clark 2018-07-09 16:40 ` Sean Paul 1 sibling, 2 replies; 8+ messages in thread From: Archit Taneja @ 2018-07-04 13:23 UTC (permalink / raw) To: Rob Clark, Sean Paul; +Cc: linux-arm-msm, Laurent Pinchart, dri-devel On Wednesday 04 July 2018 12:28 AM, Rob Clark wrote: > On Tue, Jul 3, 2018 at 12:56 PM, Sean Paul <seanpaul@chromium.org> wrote: >> The bridge loses its hw state when the cable is unplugged. If we detect >> this case in the hpd handler, reset its state. >> >> Reported-by: Rob Clark <robdclark@gmail.com> >> Signed-off-by: Sean Paul <seanpaul@chromium.org> > > Tested-by: Rob Clark <robdclark@gmail.com> > >> --- >> This is the follow-up to "drm/msm: Disable mdp5 crtc when there are no >> active planes". I incorrectly assumed the modeset was required from the >> msm side, but Rob pointed out that he thought it might be a bridge >> issue. > > To elaborate, this is an atomic userspace (weston), when it sees the > display disconnected, it removes the planes from the CRTC but does not > disable the CRTC. So drm/msm sets up the LM to scanout a solid color, > and leaves the encoder and dsi cranking along happily. When > replugging the display, the atomic helpers see the CRTC is still > active and (correctly) doesn't do a full modeset. So the bridge is > not re-configured/re-enabled. The adv7511 connector's detect() op should have re-configured the registers. I'm assuming it was never called after the cable is plugged in again in the wetson usecase? With this patch, in the case of fb emulation/X, I'm wondering if we will end up trying to re-enable ADV7511 twice. First, in the new code in adv7511_hpd_work(), and later in adv7511_detect() (i.e, the connector's detect op). I'm guessing the 'hpd' variable in the check in adv7511_detect() below should ideally be false, and avoid us trying to configure the registers again, but I'm not entirely sure. if (status == connector_status_connected && hpd && adv7511->powered) { regcache_mark_dirty(adv7511->regmap); ... In any case: Reviewed-by: Archit Taneja <architt@codeaurora.org> > > Arguably this perhaps isn't what weston *wanted* to do, but in the end > the bug is in the bridge. > > We could possibly set the connector link_status to BAD as an > alternative. But at least the build of weston I'm using atm doesn't > handle the link_status propery, this approach requires each atomic > userspace to handle this case. Compared to Sean's approach which is > transparent to userspace, which seems attractive. > > BR, > -R > >> >> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> index 73021b388e12..dd3ff2f2cdce 100644 >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> @@ -429,6 +429,18 @@ static void adv7511_hpd_work(struct work_struct *work) >> else >> status = connector_status_disconnected; >> >> + /* >> + * The bridge resets its registers on unplug. So when we get a plug >> + * event and we're already supposed to be powered, cycle the bridge to >> + * restore its state. >> + */ >> + if (status == connector_status_connected && >> + adv7511->connector.status == connector_status_disconnected && >> + adv7511->powered) { >> + regcache_mark_dirty(adv7511->regmap); >> + adv7511_power_on(adv7511); >> + } >> + >> if (adv7511->connector.status != status) { >> adv7511->connector.status = status; >> if (status == connector_status_disconnected) >> -- >> Sean Paul, Software Engineer, Google / Chromium OS >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/bridge: adv7511: Reset registers on hotplug 2018-07-04 13:23 ` Archit Taneja @ 2018-07-04 14:06 ` Rob Clark 2018-07-04 14:15 ` Daniel Vetter 2018-07-09 16:40 ` Sean Paul 1 sibling, 1 reply; 8+ messages in thread From: Rob Clark @ 2018-07-04 14:06 UTC (permalink / raw) To: Archit Taneja; +Cc: linux-arm-msm, Laurent Pinchart, dri-devel On Wed, Jul 4, 2018 at 9:23 AM, Archit Taneja <architt@codeaurora.org> wrote: > > > On Wednesday 04 July 2018 12:28 AM, Rob Clark wrote: >> >> On Tue, Jul 3, 2018 at 12:56 PM, Sean Paul <seanpaul@chromium.org> wrote: >>> >>> The bridge loses its hw state when the cable is unplugged. If we detect >>> this case in the hpd handler, reset its state. >>> >>> Reported-by: Rob Clark <robdclark@gmail.com> >>> Signed-off-by: Sean Paul <seanpaul@chromium.org> >> >> >> Tested-by: Rob Clark <robdclark@gmail.com> >> >>> --- >>> This is the follow-up to "drm/msm: Disable mdp5 crtc when there are no >>> active planes". I incorrectly assumed the modeset was required from the >>> msm side, but Rob pointed out that he thought it might be a bridge >>> issue. >> >> >> To elaborate, this is an atomic userspace (weston), when it sees the >> display disconnected, it removes the planes from the CRTC but does not >> disable the CRTC. So drm/msm sets up the LM to scanout a solid color, >> and leaves the encoder and dsi cranking along happily. When >> replugging the display, the atomic helpers see the CRTC is still >> active and (correctly) doesn't do a full modeset. So the bridge is >> not re-configured/re-enabled. > > > The adv7511 connector's detect() op should have re-configured the > registers. I'm assuming it was never called after the cable is > plugged in again in the wetson usecase? > > With this patch, in the case of fb emulation/X, I'm wondering if > we will end up trying to re-enable ADV7511 twice. First, in the new code > in adv7511_hpd_work(), and later in adv7511_detect() (i.e, the > connector's detect op). > jfwiw, fbcon and things using legacy SETCRTC end up triggering a full modeset, which somehow papers over the issue (because we go thru the bridge disable/enable sequence). So now there probably is a double power cycle sequence, although it doesn't seem to be causing any harm (fbcon and x11 still seem happy) BR, -R > I'm guessing the 'hpd' variable in the check in adv7511_detect() below > should ideally be false, and avoid us trying to configure the registers > again, but I'm not entirely sure. > > if (status == connector_status_connected && hpd && adv7511->powered) { > regcache_mark_dirty(adv7511->regmap); > ... > > In any case: > > Reviewed-by: Archit Taneja <architt@codeaurora.org> > >> >> Arguably this perhaps isn't what weston *wanted* to do, but in the end >> the bug is in the bridge. >> >> We could possibly set the connector link_status to BAD as an >> alternative. But at least the build of weston I'm using atm doesn't >> handle the link_status propery, this approach requires each atomic >> userspace to handle this case. Compared to Sean's approach which is >> transparent to userspace, which seems attractive. >> >> BR, >> -R >> >>> >>> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>> index 73021b388e12..dd3ff2f2cdce 100644 >>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>> @@ -429,6 +429,18 @@ static void adv7511_hpd_work(struct work_struct >>> *work) >>> else >>> status = connector_status_disconnected; >>> >>> + /* >>> + * The bridge resets its registers on unplug. So when we get a >>> plug >>> + * event and we're already supposed to be powered, cycle the >>> bridge to >>> + * restore its state. >>> + */ >>> + if (status == connector_status_connected && >>> + adv7511->connector.status == connector_status_disconnected && >>> + adv7511->powered) { >>> + regcache_mark_dirty(adv7511->regmap); >>> + adv7511_power_on(adv7511); >>> + } >>> + >>> if (adv7511->connector.status != status) { >>> adv7511->connector.status = status; >>> if (status == connector_status_disconnected) >>> -- >>> Sean Paul, Software Engineer, Google / Chromium OS >>> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" >> in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/bridge: adv7511: Reset registers on hotplug 2018-07-04 14:06 ` Rob Clark @ 2018-07-04 14:15 ` Daniel Vetter 2018-07-04 14:53 ` Rob Clark 0 siblings, 1 reply; 8+ messages in thread From: Daniel Vetter @ 2018-07-04 14:15 UTC (permalink / raw) To: Rob Clark; +Cc: linux-arm-msm, Laurent Pinchart, dri-devel On Wed, Jul 4, 2018 at 4:06 PM, Rob Clark <robdclark@gmail.com> wrote: > On Wed, Jul 4, 2018 at 9:23 AM, Archit Taneja <architt@codeaurora.org> wrote: >> >> >> On Wednesday 04 July 2018 12:28 AM, Rob Clark wrote: >>> >>> On Tue, Jul 3, 2018 at 12:56 PM, Sean Paul <seanpaul@chromium.org> wrote: >>>> >>>> The bridge loses its hw state when the cable is unplugged. If we detect >>>> this case in the hpd handler, reset its state. >>>> >>>> Reported-by: Rob Clark <robdclark@gmail.com> >>>> Signed-off-by: Sean Paul <seanpaul@chromium.org> >>> >>> >>> Tested-by: Rob Clark <robdclark@gmail.com> >>> >>>> --- >>>> This is the follow-up to "drm/msm: Disable mdp5 crtc when there are no >>>> active planes". I incorrectly assumed the modeset was required from the >>>> msm side, but Rob pointed out that he thought it might be a bridge >>>> issue. >>> >>> >>> To elaborate, this is an atomic userspace (weston), when it sees the >>> display disconnected, it removes the planes from the CRTC but does not >>> disable the CRTC. So drm/msm sets up the LM to scanout a solid color, >>> and leaves the encoder and dsi cranking along happily. When >>> replugging the display, the atomic helpers see the CRTC is still >>> active and (correctly) doesn't do a full modeset. So the bridge is >>> not re-configured/re-enabled. >> >> >> The adv7511 connector's detect() op should have re-configured the >> registers. I'm assuming it was never called after the cable is >> plugged in again in the wetson usecase? >> >> With this patch, in the case of fb emulation/X, I'm wondering if >> we will end up trying to re-enable ADV7511 twice. First, in the new code >> in adv7511_hpd_work(), and later in adv7511_detect() (i.e, the >> connector's detect op). >> > > jfwiw, fbcon and things using legacy SETCRTC end up triggering a full > modeset, which somehow papers over the issue (because we go thru the > bridge disable/enable sequence). So now there probably is a double > power cycle sequence, although it doesn't seem to be causing any harm > (fbcon and x11 still seem happy) Hm that's strange. Legacy setcrtc on atomic drivers should do the exact same opportunistic modeset avoidance as an atomic flip. The only difference is around the automagic resetting of the link_status, but msm doesn't use that. I suspect the real reason here is that fbcon and X aren't broken, and properly shut down the disconnected output before trying to re-enable it. This shouldn't have anything to do with atomic vs. legacy ioctl paths. Also, in-kernel fbcon is 100% atomic nowadays, it doesn't use any of the legacy entry points anymore. -Daniel > > BR, > -R > >> I'm guessing the 'hpd' variable in the check in adv7511_detect() below >> should ideally be false, and avoid us trying to configure the registers >> again, but I'm not entirely sure. >> >> if (status == connector_status_connected && hpd && adv7511->powered) { >> regcache_mark_dirty(adv7511->regmap); >> ... >> >> In any case: >> >> Reviewed-by: Archit Taneja <architt@codeaurora.org> >> >>> >>> Arguably this perhaps isn't what weston *wanted* to do, but in the end >>> the bug is in the bridge. >>> >>> We could possibly set the connector link_status to BAD as an >>> alternative. But at least the build of weston I'm using atm doesn't >>> handle the link_status propery, this approach requires each atomic >>> userspace to handle this case. Compared to Sean's approach which is >>> transparent to userspace, which seems attractive. >>> >>> BR, >>> -R >>> >>>> >>>> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 ++++++++++++ >>>> 1 file changed, 12 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>>> index 73021b388e12..dd3ff2f2cdce 100644 >>>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>>> @@ -429,6 +429,18 @@ static void adv7511_hpd_work(struct work_struct >>>> *work) >>>> else >>>> status = connector_status_disconnected; >>>> >>>> + /* >>>> + * The bridge resets its registers on unplug. So when we get a >>>> plug >>>> + * event and we're already supposed to be powered, cycle the >>>> bridge to >>>> + * restore its state. >>>> + */ >>>> + if (status == connector_status_connected && >>>> + adv7511->connector.status == connector_status_disconnected && >>>> + adv7511->powered) { >>>> + regcache_mark_dirty(adv7511->regmap); >>>> + adv7511_power_on(adv7511); >>>> + } >>>> + >>>> if (adv7511->connector.status != status) { >>>> adv7511->connector.status = status; >>>> if (status == connector_status_disconnected) >>>> -- >>>> Sean Paul, Software Engineer, Google / Chromium OS >>>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" >>> in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >> > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/bridge: adv7511: Reset registers on hotplug 2018-07-04 14:15 ` Daniel Vetter @ 2018-07-04 14:53 ` Rob Clark 0 siblings, 0 replies; 8+ messages in thread From: Rob Clark @ 2018-07-04 14:53 UTC (permalink / raw) To: Daniel Vetter; +Cc: linux-arm-msm, Laurent Pinchart, dri-devel On Wed, Jul 4, 2018 at 10:15 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Jul 4, 2018 at 4:06 PM, Rob Clark <robdclark@gmail.com> wrote: >> On Wed, Jul 4, 2018 at 9:23 AM, Archit Taneja <architt@codeaurora.org> wrote: >>> >>> >>> On Wednesday 04 July 2018 12:28 AM, Rob Clark wrote: >>>> >>>> On Tue, Jul 3, 2018 at 12:56 PM, Sean Paul <seanpaul@chromium.org> wrote: >>>>> >>>>> The bridge loses its hw state when the cable is unplugged. If we detect >>>>> this case in the hpd handler, reset its state. >>>>> >>>>> Reported-by: Rob Clark <robdclark@gmail.com> >>>>> Signed-off-by: Sean Paul <seanpaul@chromium.org> >>>> >>>> >>>> Tested-by: Rob Clark <robdclark@gmail.com> >>>> >>>>> --- >>>>> This is the follow-up to "drm/msm: Disable mdp5 crtc when there are no >>>>> active planes". I incorrectly assumed the modeset was required from the >>>>> msm side, but Rob pointed out that he thought it might be a bridge >>>>> issue. >>>> >>>> >>>> To elaborate, this is an atomic userspace (weston), when it sees the >>>> display disconnected, it removes the planes from the CRTC but does not >>>> disable the CRTC. So drm/msm sets up the LM to scanout a solid color, >>>> and leaves the encoder and dsi cranking along happily. When >>>> replugging the display, the atomic helpers see the CRTC is still >>>> active and (correctly) doesn't do a full modeset. So the bridge is >>>> not re-configured/re-enabled. >>> >>> >>> The adv7511 connector's detect() op should have re-configured the >>> registers. I'm assuming it was never called after the cable is >>> plugged in again in the wetson usecase? >>> >>> With this patch, in the case of fb emulation/X, I'm wondering if >>> we will end up trying to re-enable ADV7511 twice. First, in the new code >>> in adv7511_hpd_work(), and later in adv7511_detect() (i.e, the >>> connector's detect op). >>> >> >> jfwiw, fbcon and things using legacy SETCRTC end up triggering a full >> modeset, which somehow papers over the issue (because we go thru the >> bridge disable/enable sequence). So now there probably is a double >> power cycle sequence, although it doesn't seem to be causing any harm >> (fbcon and x11 still seem happy) > > Hm that's strange. Legacy setcrtc on atomic drivers should do the > exact same opportunistic modeset avoidance as an atomic flip. The only > difference is around the automagic resetting of the link_status, but > msm doesn't use that. I suspect the real reason here is that fbcon and > X aren't broken, and properly shut down the disconnected output before > trying to re-enable it. This shouldn't have anything to do with atomic > vs. legacy ioctl paths. yes, probably it is because they shut down the CRTC on the disconnect, but userspace using ATOMIC ioctl has to do that explicitly (and weston does not). So the only connection to ATOMIC vs legacy ioctl paths is that atomic lets userspace not shut down the CRTC. Fwiw, I did have a patch to make adv75xx use the link_status (it is really the bridge, not msm, which should be doing this, since it is 100% about the bridge). But since that relies on userspace support (which weston does not have yet), and Sean's approach actually seems to work out ok, I kinda like his approach better. BR, -R > Also, in-kernel fbcon is 100% atomic nowadays, it doesn't use any of > the legacy entry points anymore. > -Daniel > >> >> BR, >> -R >> >>> I'm guessing the 'hpd' variable in the check in adv7511_detect() below >>> should ideally be false, and avoid us trying to configure the registers >>> again, but I'm not entirely sure. >>> >>> if (status == connector_status_connected && hpd && adv7511->powered) { >>> regcache_mark_dirty(adv7511->regmap); >>> ... >>> >>> In any case: >>> >>> Reviewed-by: Archit Taneja <architt@codeaurora.org> >>> >>>> >>>> Arguably this perhaps isn't what weston *wanted* to do, but in the end >>>> the bug is in the bridge. >>>> >>>> We could possibly set the connector link_status to BAD as an >>>> alternative. But at least the build of weston I'm using atm doesn't >>>> handle the link_status propery, this approach requires each atomic >>>> userspace to handle this case. Compared to Sean's approach which is >>>> transparent to userspace, which seems attractive. >>>> >>>> BR, >>>> -R >>>> >>>>> >>>>> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 ++++++++++++ >>>>> 1 file changed, 12 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>>>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>>>> index 73021b388e12..dd3ff2f2cdce 100644 >>>>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>>>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>>>> @@ -429,6 +429,18 @@ static void adv7511_hpd_work(struct work_struct >>>>> *work) >>>>> else >>>>> status = connector_status_disconnected; >>>>> >>>>> + /* >>>>> + * The bridge resets its registers on unplug. So when we get a >>>>> plug >>>>> + * event and we're already supposed to be powered, cycle the >>>>> bridge to >>>>> + * restore its state. >>>>> + */ >>>>> + if (status == connector_status_connected && >>>>> + adv7511->connector.status == connector_status_disconnected && >>>>> + adv7511->powered) { >>>>> + regcache_mark_dirty(adv7511->regmap); >>>>> + adv7511_power_on(adv7511); >>>>> + } >>>>> + >>>>> if (adv7511->connector.status != status) { >>>>> adv7511->connector.status = status; >>>>> if (status == connector_status_disconnected) >>>>> -- >>>>> Sean Paul, Software Engineer, Google / Chromium OS >>>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" >>>> in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>> >>> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/bridge: adv7511: Reset registers on hotplug 2018-07-04 13:23 ` Archit Taneja 2018-07-04 14:06 ` Rob Clark @ 2018-07-09 16:40 ` Sean Paul 1 sibling, 0 replies; 8+ messages in thread From: Sean Paul @ 2018-07-09 16:40 UTC (permalink / raw) To: Archit Taneja; +Cc: linux-arm-msm, dri-devel, Laurent Pinchart On Wed, Jul 04, 2018 at 06:53:02PM +0530, Archit Taneja wrote: > > > On Wednesday 04 July 2018 12:28 AM, Rob Clark wrote: > > On Tue, Jul 3, 2018 at 12:56 PM, Sean Paul <seanpaul@chromium.org> wrote: > > > The bridge loses its hw state when the cable is unplugged. If we detect > > > this case in the hpd handler, reset its state. > > > > > > Reported-by: Rob Clark <robdclark@gmail.com> > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > Tested-by: Rob Clark <robdclark@gmail.com> > > > > > --- > > > This is the follow-up to "drm/msm: Disable mdp5 crtc when there are no > > > active planes". I incorrectly assumed the modeset was required from the > > > msm side, but Rob pointed out that he thought it might be a bridge > > > issue. > > > > To elaborate, this is an atomic userspace (weston), when it sees the > > display disconnected, it removes the planes from the CRTC but does not > > disable the CRTC. So drm/msm sets up the LM to scanout a solid color, > > and leaves the encoder and dsi cranking along happily. When > > replugging the display, the atomic helpers see the CRTC is still > > active and (correctly) doesn't do a full modeset. So the bridge is > > not re-configured/re-enabled. > > The adv7511 connector's detect() op should have re-configured the > registers. I'm assuming it was never called after the cable is > plugged in again in the wetson usecase? Right. detect() isn't guaranteed to be called on connection (it just usually is). > > With this patch, in the case of fb emulation/X, I'm wondering if > we will end up trying to re-enable ADV7511 twice. First, in the new code > in adv7511_hpd_work(), and later in adv7511_detect() (i.e, the > connector's detect op). > > I'm guessing the 'hpd' variable in the check in adv7511_detect() below > should ideally be false, and avoid us trying to configure the registers > again, but I'm not entirely sure. It's entirely possible to call detect() multiple times per connection (ie, run modetest a bunch), so that hpd check is pretty critical for even non-hotplug cases. > > if (status == connector_status_connected && hpd && adv7511->powered) { > regcache_mark_dirty(adv7511->regmap); > ... > > In any case: > > Reviewed-by: Archit Taneja <architt@codeaurora.org> Thanks for your review, I'll apply to -misc-fixes. Sean > > > > > Arguably this perhaps isn't what weston *wanted* to do, but in the end > > the bug is in the bridge. > > > > We could possibly set the connector link_status to BAD as an > > alternative. But at least the build of weston I'm using atm doesn't > > handle the link_status propery, this approach requires each atomic > > userspace to handle this case. Compared to Sean's approach which is > > transparent to userspace, which seems attractive. > > > > BR, > > -R > > > > > > > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > index 73021b388e12..dd3ff2f2cdce 100644 > > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > @@ -429,6 +429,18 @@ static void adv7511_hpd_work(struct work_struct *work) > > > else > > > status = connector_status_disconnected; > > > > > > + /* > > > + * The bridge resets its registers on unplug. So when we get a plug > > > + * event and we're already supposed to be powered, cycle the bridge to > > > + * restore its state. > > > + */ > > > + if (status == connector_status_connected && > > > + adv7511->connector.status == connector_status_disconnected && > > > + adv7511->powered) { > > > + regcache_mark_dirty(adv7511->regmap); > > > + adv7511_power_on(adv7511); > > > + } > > > + > > > if (adv7511->connector.status != status) { > > > adv7511->connector.status = status; > > > if (status == connector_status_disconnected) > > > -- > > > Sean Paul, Software Engineer, Google / Chromium OS > > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-07-09 16:40 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-07-03 16:56 [PATCH] drm/bridge: adv7511: Reset registers on hotplug Sean Paul 2018-07-03 18:58 ` Rob Clark 2018-07-03 19:03 ` Sean Paul 2018-07-04 13:23 ` Archit Taneja 2018-07-04 14:06 ` Rob Clark 2018-07-04 14:15 ` Daniel Vetter 2018-07-04 14:53 ` Rob Clark 2018-07-09 16:40 ` Sean Paul
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox