On Thu, 31 Oct 2024 at 08:39, Tomi Valkeinen
[off-list ref] wrote:

Hi,

On 30/10/2024 22:04, Kevin Hilman wrote:

quoted

Tomi Valkeinen [off-list ref] writes:

quoted

At the moment the driver sets the power state of all the PDs it creates
to off, regardless of the actual HW state. This has two drawbacks:

1) The kernel cannot disable unused PDs automatically for power saving,
    as it thinks they are off already

2) A more specific case (but perhaps applicable to other scenarios
    also): bootloader enabled splash-screen cannot be kept on the screen.

The issue in 2) is that the driver framework automatically enables the
device's PD before calling probe() and disables it after the probe().
This means that when the display subsystem (DSS) driver probes, but e.g.
fails due to deferred probing, the DSS PD gets turned off and the driver
cannot do anything to affect that.

Solving the 2) requires more changes to actually keep the PD on during
the boot, but a prerequisite for it is to have the correct power state
for the PD.

The downside with this patch is that it takes time to call the 'is_on'
op, and we need to call it for each PD. In my tests with AM62 SK, using
defconfig, I see an increase from ~3.5ms to ~7ms. However, the added
feature is valuable, so in my opinion it's worth it.

The performance could probably be improved with a new firmware API which
returns the power states of all the PDs.

Agreed.  I think we have to pay this performance price for correctness,
and we can optimizie it later with improvements to the SCI firmware and
a new API.

quoted

There's also a related HW issue at play here: if the DSS IP is enabled
and active, and its PD is turned off without first disabling the DSS
display outputs, the DSS IP will hang and causes the kernel to halt if
and when the DSS driver accesses the DSS registers the next time.

Ouch.

quoted

With the current upstream kernel, with this patch applied, this means
that if the bootloader enables the display, and the DSS driver is
compiled as a module, the kernel will at some point disable unused PDs,
including the DSS PD. When the DSS module is later loaded, it will hang
the kernel.

The same issue is already there, even without this patch, as the DSS
driver may hit deferred probing, which causes the PD to be turned off,
and leading to kernel halt when the DSS driver is probed again. This
issue has been made quite rare with some arrangements in the DSS
driver's probe, but it's still there.

So, because of the DSS hang issues, I think this patch is still an RFC.

Like you said, I think that DSS hang is an issue independently of this
patch, so it shouldn't hold this up IMO.

In current upstream, if the bootloader has enabled the display, we most
likely won't hit the DSS hang issue as the PD will stay on until the DSS
driver has had a chance to probe, and the driver takes actions to avoid
the hang issue.

With this patch applied, the PD may be turned off before the DSS driver
has had a chance to probe, causing the board to hang when the DSS driver
probes the first time.

That's why I'm a bit hesitant to apply this. It could mean that for some
people their board stops booting.

I'm not even sure what would be the perfect fix for this hang problem...

We could have some built-in early boot code which checks if the DSS is
enabled, and disables it, so that the hang issue won't happen. But
that's not good if we try to keep the boot splash on the screen until
the userspace takes over.

Alternatively we could, somehow, mark the DSS powerdomain to be handled
in a special way: if the PD is enabled at boot time, it will be kept
enabled until the DSS driver (somehow) changes the PD back to normal
operation (and if DSS driver is never loaded, PD will stay on).

This option is kind of what I am working on. Although, the goal is to
keep the code generic, so ideally we should not need any changes in
the DSS driver to make this work. Let's see.

That said, it sounds like we should defer $subject patch until we have
a solution for the above, right?

[...]

Kind regards
Uffe