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