On 7/19/2025 4:07 PM, Imre Deak wrote:
On Fri, Jul 18, 2025 at 05:16:20PM -0300, Gustavo Sousa wrote:
...
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.
I had a version of my first patch that excluded AUX_TC3/4.
That lead to the following warning. I need to dig deeper to understand
the failure but it might be more obvious to one of you.
At the time I just assumed that these power wells are actually present.
[ 4.223342] ------------[ cut here ]------------
[ 4.223343] xe 0000:00:02.0: [drm] drm_WARN_ON(aux_powered)
[ 4.223367] WARNING: CPU: 0 PID: 145 at
drivers/gpu/drm/i915/display/intel_tc.c:1447
intel_tc_port_reset_mode+0x135/0x350 [xe]
[ 4.223583] Modules linked in: xe(+) drm_ttm_helper ttm
drm_suballoc_helper cec rc_core drm_buddy gpu_sched drm_gpuvm drm_exec
drm_gpusvm drm_display_helper mac_hid video wmi pinctrl_intel_platform
pinctrl_intel pwm_lpss
...
[ 4.223801] Call Trace:
[ 4.223803] <TASK>
[ 4.223806] __intel_tc_port_lock+0xdf/0x130 [xe]
[ 4.223943] intel_tc_port_lock+0x1e/0x30 [xe]
[ 4.224062] intel_digital_port_connected+0x33/0xa0 [xe]
[ 4.224258] intel_dp_detect+0xef/0x860 [xe]
[ 4.224474] ? ww_mutex_lock+0xfd/0x110
[ 4.224482] detect_connector_status+0x1e/0x50
[ 4.224490] drm_helper_probe_detect+0x4b/0x80
[ 4.224495] drm_helper_probe_single_connector_modes+0x3f4/0x680
[ 4.224502] drm_client_modeset_probe+0x251/0x1a40
[ 4.224506] ? kmem_cache_alloc_lru_noprof+0x2cd/0x3b0
[ 4.224512] ? __d_alloc+0x2e/0x1f0
[ 4.224519] ? __kmalloc_node_track_caller_noprof+0x347/0x4c0
[ 4.224524] __drm_fb_helper_initial_config_and_unlock+0x3e/0x560
[ 4.224529] ? kstrdup+0x3c/0x70
[ 4.224533] ? kstrdup+0x52/0x70
[ 4.224536] drm_fb_helper_initial_config+0x36/0x40
[ 4.224540] drm_fbdev_client_hotplug+0x76/0xc0
[ 4.224543] drm_client_register+0x68/0xb0
[ 4.224549] drm_fbdev_client_setup+0xe8/0x1d0
[ 4.224552] drm_client_setup+0x5b/0x80
[ 4.224555] drm_client_setup_with_color_mode+0x29/0x40
[ 4.224557] intel_fbdev_setup+0x20f/0x4c0 [xe]
[ 4.224699] intel_display_driver_register+0xb9/0x100 [xe]
[ 4.224905] ? __pfx___drm_printfn_dbg+0x10/0x10
[ 4.224909] ? intel_display_driver_register+0x32/0x100 [xe]
[ 4.225108] xe_display_register+0x2c/0x40 [xe]
[ 4.225321] xe_device_probe+0x4af/0x580 [xe]
[ 4.225467] xe_pci_probe+0x9a2/0xcd0 [xe]
[ 4.225653] local_pci_probe+0x4c/0xb0
[ 4.225657] pci_device_probe+0xdb/0x230
[ 4.225660] really_probe+0xe2/0x390
[ 4.225665] __driver_probe_device+0x7e/0x160
[ 4.225669] driver_probe_device+0x23/0xa0
[ 4.225673] __driver_attach+0xe8/0x1e0
[ 4.225677] ? __pfx___driver_attach+0x10/0x10
[ 4.225681] bus_for_each_dev+0x7d/0xd0
[ 4.225684] driver_attach+0x22/0x30
[ 4.225687] bus_add_driver+0x118/0x240
[ 4.225691] driver_register+0x68/0x130
[ 4.225695] __pci_register_driver+0x65/0x70
[ 4.225697] xe_register_pci_driver+0x27/0x30 [xe]
[ 4.225875] xe_init+0x35/0x90 [xe]
[ 4.226005] ? __pfx_xe_init+0x10/0x10 [xe]
[ 4.226131] do_one_initcall+0x49/0x330
[ 4.226137] do_init_module+0x6a/0x2a0
[ 4.226141] load_module+0x21e6/0x22b0
[ 4.226145] ? kernel_read_file+0x240/0x2c0
[ 4.226150] init_module_from_file+0x9b/0xe0
[ 4.226153] ? init_module_from_file+0x9b/0xe0
[ 4.226156] idempotent_init_module+0x170/0x270
[ 4.226159] __x64_sys_finit_module+0x6f/0xe0
[ 4.226162] x64_sys_call+0x1b7a/0x2150
[ 4.226165] do_syscall_64+0x56/0x860
[ 4.226169] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 4.226172] RIP: 0033:0x7fab2003b25d
...
[ 4.226190] ---[ end trace 0000000000000000 ]---
- Chaitanya
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
