[PATCH v5 4/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Doug Anderson]

Hi,

On Tue, Aug 30, 2022 at 9:11 AM Tomi Valkeinen
<tomi.valkeinen at ideasonboard.com> wrote:
>
> >>> eDP the _correct_ implementation for detect is to always return true.
> >>> Yes, there is a line called HPD for eDP and yes that line is used for
> >>> full DisplayPort for detecting a display. For eDP, though, HPD does
> >>> not detect the presence of a display. A display is always there.
> >>
> >> But for eDP it still signals the actual availability of the display,
> >> similarly to DP, doesn't it? You can't communicate with the monitor or
> >> read the EDID until you get the HPD.
> >
> > It signals that the display has finished booting, _not_ whether the
> > display is there. The display is always there.
> >
> > There are simply two concepts:
> > 1. Is a display there?
> > 2. Can I talk to the display?
> >
> > I assert that the way that "detect" is used in the DRM core is for #1.
>
> Why is that? Can you point to any specific piece of code?
>
> I didn't look it closely, but I believe in my testing I saw that the
> framework expects to be able to read EDID after detect() reports that
> the display is connected. And if EDID read fails, then you get only the
> default modes, even if the display was ready very soon afterwards. If so
> that hints more towards 2.

I guess it's mostly the chicken and egg problem. In your model, how
does the panel get turned on? Let's say that the eDP panel is off at
bootup, right? So "HPD" will not be high. detect() will say that
nothing is there. Since nothing is there, nobody will ever try to call
get_edid() nor will they ever try turning on the panel. ...and since
nobody turns on the panel HPD will never be high.

Now let's imagine that there is some rule to turn the panel on once at
bootup. Great, you'll see the panel at bootup. ...but then what
happens when you go through a modeset or suspend/resume or similar?
We'll turn the panel off as part of the modeset, HPD will go low, and
it will look like the panel is gone forever.

This is why detect() has to always say that an eDP panel is present.
If this isn't true the panel will never be turned on because we'll
never know it's there.


> > In theory one could try to conflate the two. Everyone keeps trying
>
> I agree here, they are not the same.
>
> > until they think about it more. Probably because the signal is named
> > HPD and everyone reads that as "hot plug detect". Worst. Name. Ever.
> > In any case, here lies dragons. Specifically if you conflate these two
> > concepts then when do you know to provide power to the display?
> > Remember, you can't detect the display until it's powered. ...but why
> > would you power it if you thought it wasn't there? You could power it
> > once at bootup, but then if someone turns the display off how will you
> > ever know that you can power it back on? It'll look like the display
> > was removed...
>
> But here's my question: if detect() tells whether the display is
> physically there, why do we need it?
>
> If the display is not hot-pluggable, then, as you say, it's always
> there, and detect() is unnecessary. The panel driver always assumes the
> panel is there and will power it up. So detect is not really needed.

Right. I conflated these two, sorry. Having detect() unimplemented and
having it always return true are the same thing and the DRM core
treats them the same as far as I'm aware.


> > It gets down to making sure things are powered. If the eDP controller
> > implements get_edid() then the eDP controller needs to know how to
> > power on the panel in response to that get_edid(). Remember, this is
> > eDP and we have to _always_ say the panel is there even when HPD
> > hasn't been asserted. See the above rant^H^H^H^H explanation. While
> > it's possible to have the eDP controller call down the bridge chain to
> > power the panel temporarily for get_edid() (early patches of mine did
> > that), in the end we decided it made more sense to have this driven by
> > the panel driver.
>
> I agree here, the panel driver has to drive the process. That's actually
> how I designed the old omapfb display subsystem (well, DP didn't exist
> then), everything originated from the display driver, not the crtc side.
>
> However, my argument is that someone, be it the display or the source
> driver, should offer detect() and get_edid(),

If you implement get_edid() I believe it won't be the end of the world
because the panel's version will be picked first. However, it feels
clearer to me to not implement it if it's not going to be used / won't
work for eDP.


> and afaics it makes sense
> for detect() to report whether the display is ready or not (usually HPD
> if it is connected, but could be via some other means).

I think detect() is actually harmful for eDP, as per my argument
above. If we detect the panel is "gone" then we'll turn off the power
to the panel. We'll never detect the panel again and we'll never again
have a reason to power the panel on.


> However, I have to say this is perhaps sidetracking this patch =). I can
> drop the comment in question from the description as it's somewhat
> irrelevant wrt. this patch.

Sounds good!

-Doug