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