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

[Rob Clark]

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)

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
>>
>