[PATCH] drm/i915/display: Skip unavailable power wells based on pipe mask
Imre Deak
imre.deak at intel.com
Sat Jul 19 10:37:56 UTC 2025
On Fri, Jul 18, 2025 at 05:16:20PM -0300, Gustavo Sousa wrote:
> 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.deak at intel.com>
> >> >> >Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah at 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.
Ok, so on WCL TGL_DFSM_PIPE_D_DISABLE is always set. I wonder if it'd
make sense to add an assert for this somewhere
(__intel_display_device_info_runtime_init() ?), so that the assumptions
on this elsewhere are always correct.
Would it make sense to clarify on the power well bspec page that PW_D is
not present on WCL?
I presume the "Block Diagram"s, like the one at index/74286, show what
pipes/DDIs are present in the IP, which may be fused off or not
wired/enabled at all like DDI TC3/4, for all of which the HW power wells
exist and so the driver must register a power well for them regardless
of the fused state or never being wired/enabled.
DDI TC3/4 are never wired on WCL, so I'd still confirm the above, i.e.
do the PORT_AUX_CTL_USBC3/4 registers exist indeed (they do based on the
spec) and do the 'PHY Power Request' / 'PHY Power State' flags get
updated if the request bit is set/cleared as expected?
> * 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;
Agreed, I'd also prefer (1). I'd still check if registering the
AUX_TC3/4 power wells is correct based on the above.
Apart from all the above, something that could be done as a follow-up:
POWER_DOMAIN_INIT should be removed eventually. It was initially added
to make sure that all HW blocks accessed during HW readout are powered.
By now all of these accesses should get an explicit power reference, so
POWER_DOMAIN_INIT isn't reqiured for that any more.
The HW readout during driver loading/resume still needs to hold
POWER_DOMAIN_INIT though (in intel_modeset_setup_hw_state()), because
w/o that the explicit power references get/put during readout would
incorrectly disable a power well inherited by the driver in the enabled
state for an enabled pipe/port etc. So this needs another solution
before POWER_DOMAIN_INIT could be removed.
> 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
> >> >> >
More information about the Intel-xe
mailing list