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

[Archit Taneja <architt@codeaurora.org>]

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