[PATCH] drm/i915/display: Skip unavailable power wells based on pipe mask

[Imre Deak]

On Thu, Jul 17, 2025 at 09:02:45AM -0300, Gustavo Sousa wrote:
> Quoting Chaitanya Kumar Borah (2025-07-17 02:16:03-03:00)
> >Some power wells are only relevant for certain display pipes. Add a check
> >to ensure we only allocate and initialize power wells whose associated
> >pipes are available on the platform.
> >
> >This avoids unnecessary mapping of power wells, particularly when platforms
> >support a subset of pipes described in the power well descriptors.
> >
> >Suggested-by: Imre Deak <imre.deak at intel.com>
> >Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah at intel.com>
> >---
> > .../i915/display/intel_display_power_map.c    | 19 +++++++++++++++++--
> > 1 file changed, 17 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/display/intel_display_power_map.c b/drivers/gpu/drm/i915/display/intel_display_power_map.c
> >index 77268802b55e..ca73e4084354 100644
> >--- a/drivers/gpu/drm/i915/display/intel_display_power_map.c
> >+++ b/drivers/gpu/drm/i915/display/intel_display_power_map.c
> >@@ -1748,6 +1748,16 @@ static void init_power_well_domains(const struct i915_power_well_instance *inst,
> >                 for_each_power_well_instance_in_desc_list((_descs)->list, (_descs)->count, \
> >                                                           (_desc), (_inst))
> > 
> >+static bool
> >+is_power_well_available(struct intel_display *display, const struct i915_power_well_desc *desc)
> >+{
> >+        if (desc->irq_pipe_mask &&
> >+            !(desc->irq_pipe_mask & DISPLAY_RUNTIME_INFO(display)->pipe_mask))
> 
> According to irq_pipe_mask's documentation, that member contains a "mask
> of pipes whose IRQ logic is backed by the pw". I think we are
> overloading the meaning of that field with this logic.
> 
> * Do we have guarantees that irq_pipe_mask will always be associated
>   with the power well that powers the pipe?

It is the case on all the platforms and so it also provides the required
way to identify the power well for a particular pipe. irq_pipe_mask
could be renamed to pipe_mask accordingly.

> * If the power well that has irq_pipe_mask is also used to power
>   something else than the pipes, we could have issues if pipes in that
>   mask are fused off.
>
> I'm leaning more toward a solution that makes POWER_DOMAIN_INIT map to
> POWER_DOMAIN_PIPE_* based on DISPLAY_RUNTIME_INFO(display)->pipe_mask. I
> have some idea of how to do that without rewriting code to use a
> hierarchical structure (which IMO would be ideal, but takes more
> effort).
> 
> The idea is to, during runtime and initialization of the mapping, set
> the bit respective to POWER_DOMAIN_INIT in each power well that has the
> bit for POWER_DOMAIN_PIPE_* set for non-fused off pipes. That would
> also require removing the POWER_DOMAIN_INIT from the static mapping for
> power wells directly responsible for POWER_DOMAIN_PIPE_*.

Power wells that don't exist on a platform shouldn't be registered in
the first place, so it's not enough to only remove them from the power
well->domain mapping, while still registering the power well. Otherwise
these non-existant power wells would still be accessed while disabling
any unused power well during driver loading/resume. Also these power
wells non-existant on a platform would be incorrectly listed in debugfs
and other state dumps.

However, I realized that pipe power wells that do exist on a platform,
but for which the corresponing pipe is fused off (for instance pipe
A/B/C on WCL) we still need to register the power well. On some
platforms at least such power wells may be enabled after HW reset/by
BIOS and so these still need to be checked and disabled if needed during
driver loading/resume. I.e. instead of the above

DISPLAY_RUNTIME_INFO(display)->pipe_mask

something like the following should be used:

u8 pipe_pw_mask(display)
{
	if (DISPLAY_VERx100(display) == 3002)
		return BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C);

	return BIT(I915_MAX_PIPES + 1) - 1;
}

> --
> Gustavo Sousa
> 
> >+                return false;
> >+
> >+        return true;
> >+}
> >+
> > static int
> > __set_power_wells(struct i915_power_domains *power_domains,
> >                   const struct i915_power_well_desc_list *power_well_descs,
> >@@ -1763,8 +1773,10 @@ __set_power_wells(struct i915_power_domains *power_domains,
> >         int power_well_count = 0;
> >         int plt_idx = 0;
> > 
> >-        for_each_power_well_instance(power_well_descs, power_well_descs_sz, desc_list, desc, inst)
> >-                power_well_count++;
> >+        for_each_power_well_instance(power_well_descs, power_well_descs_sz, desc_list, desc, inst) {
> >+                if (is_power_well_available(display, desc))
> >+                        power_well_count++;
> >+        }
> > 
> >         power_domains->power_well_count = power_well_count;
> >         power_domains->power_wells =
> >@@ -1778,6 +1790,9 @@ __set_power_wells(struct i915_power_domains *power_domains,
> >                 struct i915_power_well *pw = &power_domains->power_wells[plt_idx];
> >                 enum i915_power_well_id id = inst->id;
> > 
> >+                if (!is_power_well_available(display, desc))
> >+                        continue;
> >+
> >                 pw->desc = desc;
> >                 drm_WARN_ON(display->drm,
> >                             overflows_type(inst - desc->instances->list, pw->instance_idx));
> >-- 
> >2.25.1
> >