On Mon, Jul 21, 2025 at 08:47:56PM +0530, Borah, Chaitanya Kumar wrote:
>
> 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.
Is the connector for the TC3/4 ports get added? That would be incorrect,
as WCL doesn't have these ports. The VBT on WCL should indicate only
TC1/2 as present, but if the above is true that's probably not the case.
Please provide a full dmesg.
Regardless of the above, could you please check what happens with the
above PHY power request/state flags if you set the request flag to 1 in
the PORT_AUX_CTL_USBC3/4 registers?
>
> [ 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
> > > > > > > >
