Re: [RFC PATCH 1/6] drm/dp: Helpers to make it easier for drivers to use DP AUX bus properly

[Doug Anderson]

Hi,

On Fri, Apr 15, 2022 at 3:45 PM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
>
> On Sat, 16 Apr 2022 at 00:13, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > On Thu, Apr 14, 2022 at 5:47 PM Dmitry Baryshkov
> > <dmitry.baryshkov@xxxxxxxxxx> wrote:
> > >
> > > On 09/04/2022 05:36, Douglas Anderson wrote:
> > > > As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
> > > > patch and also in the past in commit a1e3667a9835 ("drm/bridge:
> > > > ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
> > > > DP AUX bus properly we really need two "struct device"s. One "struct
> > > > device" is in charge of providing the DP AUX bus and the other is
> > > > where we'll try to get a reference to the newly probed endpoint
> > > > devices.
> > > >
> > > > In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
> > > > is already broken up into several "struct devices" anyway because it
> > > > also provides a PWM and some GPIOs. Adding one more wasn't that
> > > > difficult / ugly.
> > > >
> > > > When I tried to do the same solution in parade-ps8640, it felt like I
> > > > was copying too much boilerplate code. I made the realization that I
> > > > didn't _really_ need a separate "driver" for each person that wanted
> > > > to do the same thing. By putting all the "driver" related code in a
> > > > common place then we could save a bit of hassle. This change
> > > > effectively adds a new "ep_client" driver that can be used by
> > > > anyone. The devices instantiated by this driver will just call through
> > > > to the probe/remove/shutdown calls provided.
> > > >
> > > > At the moment, the "ep_client" driver is backed by the Linux auxiliary
> > > > bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
> > > > want to expose this to clients, though, so as far as clients are
> > > > concerned they get a vanilla "struct device".
> > >
> > > I have been thinking about your approach for quite some time. I think
> > > that enforcing a use of auxilliary device is an overkill. What do we
> > > really need is the the set callbacks in the bus struct or a notifier. We
> > > have to notify the aux_bus controller side that the client has been
> > > probed successfully or that the client is going to be removed.
> >
> > It seems like these new callbacks would be nearly the same as the
> > probe/remove callbacks in my proposal except:
> >
> > * They rely on there being exactly 1 AUX device, or we make it a rule
> > that we wait for all AUX devices to probe (?)
>
> Is the backlight a separate device on an AUX bus? Judging from
> drm_panel_dp_aux_backlight(), it isn't. I assumed that aux bus is just
> a point-to-point bus, so there is always a single client.

Define "device". ;-)

It's a seperate "struct device" from a Linux point of view since it's
a backlight class device. Certainly it's highly correlated to the
display, but one can conceptually think of them as different devices,
sorta. ;-)

I actually dug a tiny bit more into the whole "touchscreen over aux".
I guess DP 1.2 has a standard of "USB over DP AUX". No idea how that
would be modeled, of course.

I guess the summary is that I'm OK w/ changing it to assume one device
for now, but I'm still not sure it's compelling to move to normal
callbacks. The API for callbacks is pretty much the same as the one I
proposed and IMO leaving it the way it is (with an extra struct
device) doesn't really add much complexity and has a few (small) nice
benefits.


> > * We need to come up with a system for detecting when everything
> > probes or is going to be removed, though that's probably not too hard.
> > I guess the DP AUX bus could just replace the panel's probe function
> > with its own and essentially "tail patch" it. I guess it could "head
> > patch" the remove call? ...or is there some better way you were
> > thinking of knowing when all our children probed?
> >
> > * The callback on the aux bus controller side would not be able to
> > DEFER. In other words trying to acquire a reference to the panel can
> > always be the last thing we do so we know there can be no reasons to
> > defer after. This should be doable, but at least in the ps8640 case it
> > will require changing the code a bit. I notice that today it actually
> > tries to get the panel side _before_ it gets the MIPI side and it
> > potentially can return -EPROBE_DEFER if it can't find the MIPI side. I
> > guess I have a niggling feeling that we'll find some reason in the
> > future that we can't be last, but we can probably ignore that. ;-)
> >
> > I can switch this all to normal callbacks if that's what everyone
> > wants, but it doesn't feel significantly cleaner to me and does seem
> > to have some (small) downsides.
> >
> >
> > > And this
> > > approach would make driver's life easier, since e.g. the bus code can
> > > pm_get the EP device before calling callbacks/notifiers and
> > > pm_put_autosuspend it afterwards.
> >
> > Not sure about doing the pm calls on behalf of the EP device. What's
> > the goal there?
>
> I think any driver can pm_runtime_get another device. The goal is to
> let the 'post_probe' callback to power up the panel, read the EDID,
> etc.

Right. I was hoping to keep this as a separate discussion since I
think it's largely unrelated to the probe ordering issue, but we can
talk about it here if you want.

There are a lot of open questions here and it's definitely hard to
wrap your head around all of it. Maybe I'll just spam some thoughts
and see if they all make sense together...

1. At the moment, there's no guarantee that a DP AUX Endpoint (AKA
panel) will use pm_runtime() to power itself up enough to do an AUX
transfer. At the moment the two eDP panels drivers I'm aware of use
pm_runtime, but that's actually a fairly new behavior. I guess we'd
have to codify it as "required" if we were going to rely on it.

2. In general, panels have powered themselves enough to read the EDID
in their prepare() stage, which is equivalent to the bridge's
pre_enable(). During some of my early patches to try to support EDID
reading in ti-sn65dsi86 I actually relied upon it. It was like that in
v3 [1]. Personally I see this as the "official" interface to power on
the panel from the DP controller. As such I'm not sure we need to add
pm_runtime() as an equivalent option.

3. In the cover letter of v4 of my ti-sn65dsi86 EDID patch series I
talked about why I switched to having EDID reading driven by the panel
instead of powering on the panel (via pre_enable) and reading the EDID
in the controller. One reason talked about there is that the "generic"
eDP panel driver actually needs the EDID, or at least enough of it to
get the panel ID, so that it can adjust its power sequence timings. If
the EDID reading is completely handled by the DP driver and the panel
can't do it then we'd need to figure out how to communicate it back.

4. In general, panels can be pretty persnickety about their power
sequencing. As far as I've been able to tell, the official spec
provides two things you can do:

4a) You can power the panel up enough to do AUX transfers and then
power it back off.

4b) You can power the panel up enough to do AUX transfers, then finish
powering it all the way up (turn on screen, backlight, etc). When you
turn the screen off, if you follow the spec strictly, you're also
_required_ to fully power the panel off. In other words, remove _all_
power from the display including any power that would be needed to do
AUX transfers.

Now the generic eDP panel code doesn't currently follow the
"strict"ness of the spec and I'm not actually sure if that's how the
spec is intended to be interpreted anyway. There are two timing
diagrams, though. One for "aux transfer only" and the other for
"normal system operation". In the "normal system operation" the
diagram doesn't allow for the backlight to ever go off and on again.

Now, despite the fact that the generic eDP panel code doesn't follow
the "strict"ness I just described, the _other_ DP panel I worked on
recently (samsung-atna33xc20) does. In testing we found that this
panel would sometimes (like 1 in 20 times?) crash if you ever stopped
outputting data to the display and then started again. You absolutely
needed to fully power cycle the display each time. I tried to document
this to the best of my ability in atana33xc20_unprepare(). There's
also a WARN_ON() in atana33xc20_enable() trying to detect if someone
is doing something the panel driver doesn't expect. I've also been
trying to keep my eyes out to see if we need to do the same thing in
generic eDP panel code, either for everyone or via some type of
per-panel quirk. There's definitely a good reason to avoid the extra
cycling if possible since powering panels off and on again often
requires hundreds of milliseconds of delay in order to meet timing
diagrams. ...and we do this if we ever change panel "modes".

...OK, so why does this all matter? I guess my point here is I worry a
little bit about saying that the DP controller code can willy nilly
request the panel to be powered whenever it wants. If the DP
controller was trying to hold the panel powered and then we _needed_
to power the panel off then that would be bad. It doesn't mean we
can't be careful about it, of course...

Said another way, in my mental model these three sequences are allowed:

s1) prepare, unprepare
s2) prepare, enable, disable, unprepare
s3) prepare, enable, disable, unprepare, prepare, enable, disable, unprepare

...and this sequence is _not_ allowed:

s4) prepare, enable, disable, enable, disable, unprepare

...and, in my mind, it's up to the panel driver to know whether in
sequence s3) it has to _force_ power off between the unprepare and a
prepare.

If pm_runtime() officially replaces prepare/unprepare then it's less
obvious (in my mind) that we have to coordinate with enable().

5. In general I've been asserting that it should be up to the panel to
power things on and drive all AUX transactions. ...but clearly my
model isn't reality. We certainly do AUX transactions from the DP
driver because the DP driver needs to know things about the connected
device, like the number of lanes it has, the version of eDP it
supports, and the available bit rates to name a few. Those things all
work today by relying on the fact that pre-enable powers the panel on.
It's pretty easy to say that reading the EDID (and I guess AUX
backlight) is the odd one out. So right now I guess my model is:

5a) If the panel code wants to access the AUX bus it can do so by
powering itself on and then just doing an AUX transaction and assuming
that the provider of the AUX bus can power itself on as needed.

5b) If the DP code wants to access the AUX bus it should make sure
that the next bridge's pre_enable() has been called. It can then
assume that the device is powered on until the next bridge's
post_disable() has been called.

So I guess tl;dr: I'm not really a huge fan of the DP driver powering
the panel on by doing a pm_runtime_get() on it. I'd prefer to keep
with the interface that we have to pre_enable() the panel to turn it
on.


[1] https://lore.kernel.org/r/20210402152701.v3.8.Ied721dc895156046ac523baa55a71da241cd09c7@changeid/
[2] https://lore.kernel.org/r/20210416223950.3586967-1-dianders@xxxxxxxxxxxx/


> BTW: as I'm slowly diving into DP vs eDP differences. Do we need to
> write the EDID checksum like we do for DP?

Write the EDID checksum? I don't know what that means. You mean
dp_panel_get_edid_checksum()? I'm not 100% sure, a quick glance seems
to make me feel it has to do with DP compliance testing? I can dig
more if need be. The generic EDID reading code already calculates the
checksum, so unless you're doing some funny business you shouldn't
need to check it again...


> Do you have any good summary for eDP vs DP differences?

I don't. :( Mostly stuff here is me trying to grok bits out of what
existing drivers were doing and trying to cross reference it with the
eDP spec that I have (which I don't believe I can share,
unfortunately).

-Doug