Re: [RFC PATCH 11/11] drm/bridge: ti-sn65dsi86: Support hotplug detection

[Doug Anderson <dianders@chromium.org>]

Hi,

On Wed, Feb 23, 2022 at 9:43 AM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Hi All,
>
> I'm working to respin the remainder of these patches, now that I have
> IRQ based HPD working on the SN65DSI86, and the (non-eDP) mode is used
> for Renesas R-Car boards.
>
> Quoting Doug Anderson (2021-06-24 00:51:12)
> > Hi,
> >
> > On Wed, Jun 23, 2021 at 4:26 PM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > > > @@ -1365,7 +1384,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
> > > > >
> > > > >         pdata->bridge.funcs = &ti_sn_bridge_funcs;
> > > > >         pdata->bridge.of_node = client->dev.of_node;
> > > > > -       pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> > > > > +       pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT)
> > > >
> > > > Checking for "no_hpd" here is not the right test IIUC. You want to
> > > > check for eDP vs. DP (AKA whether a panel is downstream of you or a
> > > > connector). Specifically if downstream of you is a panel then (I
> > > > believe) HPD won't assert until you turn on the panel and you won't
> > > > turn on the panel (which happens in pre_enable, right?) until HPD
> > > > fires, so you've got a chicken-and-egg problem. If downstream of you
> > > > is a connector, though, then by definition HPD has to just work
> > > > without pre_enable running so then you're OK.
> > >
> > > Agreed. It's even more true now that your rework has landed, as in the
> > > eDP case EDID is handled by the panel driver. I'll rework this.
> > >
> > > Should I also condition setting HPD_DISABLE to the presence of a panel
> > > then ? I could drop of_property_read_bool() and set
> > >
> > >         pdata->no_hpd = !!panel;
> > >
> > > > I guess then you'd need to figure out what to do if someone wants to
> > > > use "HPD" on eDP. Do you need to put a polling loop in pre_enable
> > > > then? Or you could just punt not support this case until someone needs
> > > > it.
> > >
> > > I think I'll stop short of saving the world this time, yes :-) We'll see
> > > what to do when this case arises.
> >
> > How about as a compromise you still call of_property_read_bool() but
> > add some type of warning in the logs if someone didn't set "no-hpd"
> > but they have a panel?
>
>
> Would a mix of the two work well?
>
> What about:
>
>         pdata->no_hpd = of_property_read_bool(np, "no-hpd");
>         if (panel && !pdata->no_hpd) {
>                 DRM_ERROR("Panels will not function with HPD. Enforcing no-hpd\n");
>                 pdata->no_hpd = true;
>         }
>
> That leaves it still optional to DP connectors, but enforced on eDP?

Yeah, that's fine with me. Nits would be to use "warn" instead of
"error" since this isn't fatal and use the non-SHOUTING versions of
the prints since the SHOUTING versions are deprecated.


> > > > > +                         | DRM_BRIDGE_OP_EDID;
> > > >
> > > > IMO somewhere in here if HPD is being used like this you should throw
> > > > in a call to pm_runtime_get_sync(). I guess in your solution the
> > > > regulators (for the bridge, not the panel) and enable pin are just
> > > > left on all the time,
> > >
> > > Correct, on my development board the SN65DSI86 is on all the time, I
> > > can't control that.
> > >
> > > > but plausibly someone might want to build a
> > > > system to use HPD and also have the enable pin and/or regulators
> > > > controlled by this driver, right?
> > >
> > > True. DRM doesn't make this very easy, as, as far as I can tell, there's
> > > no standard infrastructure for userspace to register an interest in HPD
> > > that could be notified to bridges. I think it should be fixable, but
> > > it's out of scope for this series :-) Should I still add a
> > > pm_runtime_get_sync() at probe time, or leave this to be addressed by
> > > someone who will need to implement power control ?
> >
> > IMO if you've detected you're running in DP mode you should add the
> > pm_runtime_get_sync() in probe to keep it powered all the time and
> > that seems the simplest. Technically I guess that's not really
> > required since you're polling and you could power off between polls,
> > but then you'd have to re-init a bunch of your state each time you
> > polled too. If you ever transitioned to using an IRQ for HPD then
> > you'd have to keep it always powered anyway.
>
>
> Hrm ... that's precisely what I've done. It's not IRQ based HPD...
>
> So would you like to see something like this during
> ti_sn_bridge_probe()?
>
>         /* The device must remain powered up for HPD to be supported. */
>         if (!pdata->no_hpd)
>                 pm_runtime_get_sync(pdata->dev);

Yeah, seems reasonable. Probably you'd want to add a devm action to put it too?

-Doug