Quoting Imre Deak (2025-07-18 12:54:11-03:00)
>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.
I mean, that *exclusively* powers the pipe(s).
As an example, bdw_pwdoms_display appears to be responsible not only for
pipe B and C, but also ddi lanes and audio, for example.
>
>> * 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
Ah, I see. Yeah, that makes sense. Thanks for the details!
Well, although Bspec overview page tells that WCL's display has only
pipes A, B and C, the page specific for power wells still lists power
well D. So I'm wondering if WCL display just has pipe D fused off and
the power well still exists or if power well D being listed in Bspec is
just a documentation mistake. I'll check with the hardware team.
>
>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;
>}
Well, if power well D does not exist indeed (i.e. not a case of pipe D
fused-off), we need either this above or maybe go back to Chaitanya's
original patch.
I think I prefer the original patch, making the power well mapping
explicit.
--
Gustavo Sousa
>
>> --
>> 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
>> >
