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

[Gustavo Sousa <gustavo.sousa@intel.com>]

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@intel.com>
>Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@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?

* 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_*.

--
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
>