On 06/05/2022 00:24, Doug Anderson wrote:
Hi,
On Thu, May 5, 2022 at 1:56 PM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
On Thu, 5 May 2022 at 23:21, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
Hi,
On Thu, May 5, 2022 at 1:10 PM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
On Thu, 5 May 2022 at 18:53, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
Hi,
On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä
<ville.syrjala@xxxxxxxxxxxxxxx> wrote:
On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
Hi,
On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
<ville.syrjala@xxxxxxxxxxxxxxx> wrote:
On Wed, May 04, 2022 at 02:10:08PM -0400, Lyude Paul wrote:
On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote:
Hi,
On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä
<ville.syrjala@xxxxxxxxxxxxxxx> wrote:
On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote:
When doing DP AUX transfers there are two actors that need to be
powered in order for the DP AUX transfer to work: the DP source and
the DP sync. Commit bacbab58f09d ("drm: Mention the power state
requirement on side-channel operations") added some documentation
saying that the DP source is required to power itself up (if needed)
to do AUX transfers. However, that commit doesn't talk anything about
the DP sink.
For full fledged DP the sink isn't really a problem. It's expected
that if an external DP monitor isn't plugged in that attempting to do
AUX transfers won't work. It's also expected that if a DP monitor is
plugged in (and thus asserting HPD) that it AUX transfers will work.
When we're looking at eDP, however, things are less obvious. Let's add
some documentation about expectations. Here's what we'll say:
1. We don't expect the DP AUX transfer function to power on an eDP
panel. If an eDP panel is physically connected but powered off then it
makes sense for the transfer to fail.
I don't agree with this. I think the panel should just get powred up
for AUX transfers.
That's definitely a fair thing to think about and I have at times
thought about trying to make it work that way. It always ends up
hitting a roadblock.
How do you even probe the panel initially if you can't power it on
without doing some kind of full modeset/etc.?
It's not that we can't power it on without a full modeset. It' that at
panel probe time all the DRM components haven't been hooked together
yet, so the bridge chain isn't available yet. The panel can power
itself on, though. This is why the documentation I added says: "if a
panel driver is initiating a DP AUX transfer it may power itself up
however it wants"
The biggest roadblock that I recall is that to make this work then
you'd have to somehow ensure that the bridge chain's pre_enable() call
was made as part of the AUX transfer, right? Since the transfer
function can be called in any context at all, we have to coordinate
this with DRM. If, for instance, DRM is mid way through powering the
panel down then we need to wait for DRM to fully finish powering down,
then we need to power the panel back up. I don't believe that we can
just force the panel to stay on if DRM is turning it off because of
panel power sequencing requirements. At least I know it would have the
potential to break "samsung-atna33xc20.c" which absolutely needs to
see the panel power off after it's been disabled.
We also, I believe, need to handle the fact that the bridge chain may
not have even been created yet. We do AUX transfers to read the EDID
and also to setup the backlight in the probe function of panel-edp. At
that point the panel hasn't been linked into the chain. We had _long_
discussions [1] about moving these out of probe and decided that we
could move the EDID read to be later but that it was going to really
ugly to move the AUX backlight later. The backlight would end up
popping up at some point in time later (the first call to panel
prepare() or maybe get_modes()) and that seemed weird.
[1]
https://lore.kernel.org/lkml/CAD=FV=U5-sTDLYdkeJWLAOG-0wgxR49VxtwUyUO7z2PuibLGsg@xxxxxxxxxxxxxx/
Otherwise you can't trust that eg. the /dev/aux
stuff is actually usable.
Yeah, it's been on my mind to talk more about /dev/aux. I think
/dev/aux has some problems, at least with eDP. Specifically:
1. Even if we somehow figure out how to power the panel on as part of
the aux transfer, we actually _still_ not guaranteed to be able to
talk to it as far as I understand. My colleague reported to me that on
a system he was working with that had PSR (panel self refresh) that
when the panel was powered on but in PSR mode that it wouldn't talk
over AUX. Assuming that this is correct then I guess we'd also have to
do even more coordination with DRM to exit PSR and block future
transitions of PSR. (NOTE: it's always possible that my colleague ran
into some other bug and that panels are _supposed_ to be able to talk
in PSR. If you think this is the case, I can always try to dig more).
TBH - the coordination with drm I don't think would be the difficult part, as
we'd just need to add some sort of property (ideally invisible to userspace)
that can be used in an atomic commit to disable PSR - similar to how we enable
CRC readback from sysfs in the majority of DRM drivers. That being said
though, I think we can just leave the work of solving this problem up to
whoever ends up needing this to work.
The driver should just disable/prevent PSR when doing AUX if the hardware
can't guarantee the PSR and AUX won't interfere with each other.
OK, fair enough. If we can solve the PSR problem that would be great.
For i915 we have no problems with powering the panel on for AUX, but
there is still a race with PSR vs. AUX because both use the same hardware
internally. I've been nagging at people to fix this for i915 but I don't
think it still got done :( Originally we were supposed to get a hardware
mutex for this but that plan got scrapped for some reason.
I haven't looked at the i915 DRM code much, but my understanding is
that it's more of an "all in one" approach. The one driver pretty much
handles everything itself. That means that powering the panel up isn't
too hard. Is that right?
Yeah, we don't have too many "helpful" abstractions in the way ;)
for userspace to be mucking with /dev/aux. For DP's case I guess
/dev/aux is essentially enabling userspace drivers to do things like
update firmware on DP monitors or play with the backlight. I guess we
decided that we didn't want to add drivers in the kernel to handle
this type of stuff so we left it for userspace? For eDP, though, there
The main reason DP AUX got exposed to userspace in the first place was for
usecases like fwupd,
My memory says the original reason was debugging. Or at least I had
no idea fwupd had started to use this until I saw some weird looking
DPCD addresses in some debug log.
But I suppose it's possible there were already plans for firmware
updates and whatnot and it just wasn't being discussed when this was
being developed.
If it's just for debugging, I'd argue that leaving it as-is should be
fine. Someone poking around with their system can find a way to make
sure that the panel stays on.
That could require altering the state of the system quite a bit, which
may defeat the purpose.
It does? In my experience you just need to make sure that the panel is
turned on. ...or are you saying that you'd use this for debugging a
case where the system isn't probing properly?
If things are truly in bad shape, at least on boards using device tree
it's easy to tweak the device tree to force a regulator to stay on. I
suppose we could also add a "debugfs" entry for the panel that also
forces it to be powered on.
At least I would not be willing to accept such
a limitation.
Hmm, so where does that leave us? Are you against landing this patch?
I've done a lot of cleanups recently and I just don't think I have the
time to rework all the AUX transfer functions and figure out how to
power the panel. It also seems like a lot of added complexity for a
debug path.
If my 2c counts, I support landing this patch. It clearly documents
current behaviour and expectations.
If that helps,
Acked-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
As for the /dev/aux, question, I think we can make the following plan work:
- Document that eDP panel power up can be handled by using the
pm_runtime API (which is the case for both panel-edp and atna33xc20)).
I think this is a sensible requirement anyway. And both panels show
how to handle different poweron/poweroff timings.
- Make drm_dp_aux_dev_get_by_minor() pm_runtime_get() the attached panel.
This matches what you suggested previously, but I still think it has a
potential problem as I talked about in the my previous (very long)
reply [1]. The relevant part was:
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.
Specifically, I think you could get in trouble if you did:
a) drm wants to power down the panel.
b) drm calls the panel's disable() function
c) we start an aux transfer and grab a runtime pm reference
d) drm calls the panel's unprepare() function => atana33xc20_unprepare()
e) atana33xc20_unprepare()'s pm_runtime_put_sync_suspend() _won't_
power off the panel (we still have the reference from step c), even
though it needs to.
f) we'll finish an aux transfer and, presumably, call
pm_runtime_put_autosuspend()
g) drm wants to power the panel back up
h) drm calls the panel's prepare() function, but power wasn't properly cycled
Why? We'd need to extend the prepare() function with the flag
data_was_on, which is set in the enable() and cleared in the suspend
path. If we see this flag in the prepare() callback, we should
forcibly power cycle the panel by toggling the regulator. Yes, this
will cause additional wait.
Another option might be to toggle the autosuspend support. Let the
disable() call pm_runtime_dont_autosuspend() (which would turn all
autosuspend calls into plain pm_runtime_put()) and allow it again in
the resume(). I'm not 100% sure that this will work, but it looks like
it should.
It turns out, though, that we _want_ autosuspend sometimes, even when
the panel is disabled. Specifically, if the panel is disabled and then
atana33xc20_get_modes() gets called then we _don't_ want to fully
power the panel off. It's expected that there will be a future call to
prepare() soon after the get_modes() and we don't want a full power
cycle (500 ms) there. This mechanism is also fully allowed by the eDP
spec. The only time we _need_ a full power cycle is after disable().
Yes. That's why I proposed to put pm_runtime_dont_autosuspend() call to
the disable(). So that after get_modes() there will be no power cycle,
but only after disable() call.
In any case, we can come up with complex ways to solve this, but I'm
still just not convinced that it's worth it. There's no valid use case
other than debugging. IMO if we're poking around and want to read DP
registers while the panel is on then it works just fine. A user doing
this can ensure that the panel is on while poking. Certainly I've done
that and it wasn't a big imposition.
If someone wants to submit patches to attempt this then I'm happy to
test them, but I feel like it's adding complexity for very little
value. The way it works now is simple / understandable and mathes my
intuition from other busses, like i2c. The bus is just responsible for
powering itself, not the devices on the bus. It has also long been
documented since commit 83127f67e450 ("drm/panel: Flesh out
kerneldoc") in 2016 that the way to turn on a panel for communication
over the command bus is via drm_panel_prepare(). I don't think we need
to change this.
Yes.
The second option leaves a possible window for the panel issues if the
userspace AUX transfer is ongoing while the panel is being unprepared
and then prepared again. In this case it will never be power cycled.
However after some thought I think this is correct. You wouldn't like
to power cycle a panel while you are e.g. updating the firmware.
As per my earlier responses, nothing we are doing here solves the
firmware update anyway. Even if we automatically power the panel for
the duration of a single aux transfer, nothing prevents the panel from
turning off between transfers. There's no API to "keep the power on
until I say stop". You certainly wouldn't want a panel to turn off
midway through a firmware update. IMO if we want to use this for
firmware update, we either need an special way to force the panel on
(in which case, we don't need to worry about it in the aux transfer
function) or (better IMO) we need to manage the firmware update in the
panel driver and prevent some type of sysfs interface to provide the
new firmware and kick off the update. Presumably having this managed
by the panel driver would be best because the panel driver could know
to, for instance, re-read the EDID after the firmware update took
place.
That's why I suggested pm_runtime_get()'ting the panel from
drm_dp_aux_dev_get_by_minor(). This way the whole userspace session will
be protected.
But as you said, this becomes more and more complex with little added value.
--
With best wishes
Dmitry
