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

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

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