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 aplug
* event and we're already supposed to be powered, cycle thebridge 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