Quoting Imre Deak (2025-07-18 14:17:09-03:00)
>On Fri, Jul 18, 2025 at 01:33:26PM -0300, Gustavo Sousa wrote:
>> 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.
>
>Yes, these power wells do support other functionalities as well and so
>they must be registered unconditionally. pipe_mask would still be
>correctly indicating that this is the power well for the pipes in the
>mask; these power wells wouldn't be skipped either during registration,
>since that logic must use a platform pipe power well mask vs. a
>non-fused-off pipe mask.
Yep. I think that works for the platforms that we have today. That said,
this whole thing started because I had the impression that pipe D was
fused-off and that power wells for fused-off pipes should not be
touched.
It turns out I was wrong in both cases:
* I just got confirmation from hardware team that WCL does not have
pipe D neither power well D.
* As you explained in a previous reply, the driver needs to deal with
power wells of fused-off pipes to ensure those get properly powered
off in case whatever was controlling display before the driver takes
control let them enabled.
So, I guess we could either
(1) go back to Chaitanya's original patch;
(2) or tweak this patch to use a mask of pipes supported by the
display IP instead of non-fused-off ones.
I personally would prefer (1), since then we would make the presence of
power wells and mapping more explicit in the code; but I wouldn't be
against (2).
--
Gustavo Sousa
>
>> >> * 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
>> >> >
