[PATCH] drm/bridge: adv7511: Reset registers on hotplug

Wed Jul 4 14:15:28 UTC 2018

On Wed, Jul 4, 2018 at 4:06 PM, Rob Clark <robdclark at gmail.com> wrote:
> On Wed, Jul 4, 2018 at 9:23 AM, Archit Taneja <architt at 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 at 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 at gmail.com>
>>>> Signed-off-by: Sean Paul <seanpaul at chromium.org>
>>>
>>>
>>> Tested-by: Rob Clark <robdclark at 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 at 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 at vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel at 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