Messages in this thread Patch in this message | | | Date | Wed, 12 Jan 2022 14:17:54 -0800 | Subject | Re: [PATCH v11 1/4] drm/msm/dp: do not initialize phy until plugin interrupt received | From | Kuogee Hsieh <> |
| |
On 1/12/2022 12:00 PM, Stephen Boyd wrote: > Quoting Kuogee Hsieh (2022-01-11 10:43:23) >> Current DP drivers have regulators, clocks, irq and phy are grouped >> together within a function and executed not in a symmetric manner. >> This increase difficulty of code maintenance and limited code scalability. >> This patch divides the driver life cycle of operation into four states, >> resume (including booting up), dongle plugin, dongle unplugged and suspend. >> Regulators, core clocks and irq are grouped together and enabled at resume >> (or booting up) so that the DP controller is armed and ready to receive HPD >> plugin interrupts. HPD plugin interrupt is generated when a dongle plugs >> into DUT (device under test). Once HPD plugin interrupt is received, DP >> controller will initialize phy so that dpcd read/write will function and >> following link training can be proceeded successfully. DP phy will be >> disabled after main link is teared down at end of unplugged HPD interrupt >> handle triggered by dongle unplugged out of DUT. Finally regulators, code >> clocks and irq are disabled at corresponding suspension.
0) Please note that dongles are behavior differently.
1) Apple dongle will generate plug-in interrupt only if no hdmi monitor atatched to dongle. it will generate irq-hpd interrupt once hdmi monitor connect to dongle later.
2) Apple dongle will generate plugged-in interrupt followed by irq-hpd interrupt if dongle has hdmi monitor attached when connects to DUT.
3) other dongle will not generate plug-in interrupt unless dongle has hdmi monitor attached when connects to DUT. It only generate plug-in interrupt only and no irq-hpd interrupt generated on this case.
4) Note: phy_initialized only associated with plugged-in interrupt
5) irq-hpd interrupt must happen after plugged-in interrupt and before unplugged interrupt
I will fill up below question with Apple dongle case with the order of event happen timing.
> I'll write out the various scenarios > > #1 > dongle plugged in with HDMI cable attached > driver probe
1) driver probe ==> core_initialized = false; phy_initialized = false; 2) dp_display_host_init() ==> core_initialized = true; 3) generate plugged-in interrupt triggers handler 4) dp_display_phy_init() ==> phy_initialized = true;
> > > #2 > dongle unplugged > driver probe
1) driver probe ==> core_initialized = false; phy_initialized = false; 2) dp_display_host_init() ==> core_initialized = true;
> > #3 > dongle plugged in without HDMI cable attached > driver probe
1) driver probe ==> core_initialized = false; phy_initialized = false; 2) dp_display_host_init() ==> core_initialized = true; 3) generate plug-in interrupt triggers handler 4) dp_display_phy_init() ==> phy_initialized = true;
Note: same as case #1 > #4 > driver probe > dongle plugged in without HDMI cable attached
1) driver probe ==> core_initialized = false; phy_initialized = false; 2) dp_display_host_init() ==> core_initialized = true; 3) dongle plugged in 4) generate plug-in interrupt triggers handler 5) dp_display_phy_init() ==> phy_initialized = true;
> > > #5 > driver probe > dongle plugged in with HDMI cable attached
1) driver probe ==> core_initialized = false; phy_initialized = false; 2) dp_display_host_init() ==> core_initialized = true; 3) dongle plugged in 4) generate plug-in interrupt trigger handler 5) dp_display_phy_init() ==> phy_initialized = true;
Note: same as case #4
> > #6 > driver probe > dongle plugged in > suspend > resume
1) driver probe ==> core_initialized = false; phy_initialized = false; 2) dp_display_host_init() ==> core_initialized = true; 3) dongle plug in 4) generate plug-in interrupt triggers handler 5) dp_display_phy_init() ==> phy_initialized = true; 6) suspend 7) dp_display_host_deinit() ==> core_initialized = false; 8) dp_display_host_phy_exit() ==> phy_initialize = false; 9) resume 10) dp_display_host_init() ==> core_initialized = true; 11) generate plug-in interrupt 12) dp_display_phy_init() ==> phy_initialize = true;
> > #7 > driver probe > dongle plugged in > suspend > dongle unplugged > resume
1) driver probe ==> core_initialized = false; phy_initialized = false; 2) dp_display_host_init() ==> core_initialized = true; 3) dongle plugged in 4) generate plug-in interrupt triggers handler 5) dp_display_phy_init() ==> phy_initialized = true; 6) suspend 7) dp_display_host_deinit() ==> core_initialized = false; 8) dp_display_host_phy_exit() ==> phy_initialize = false; 9) dongle unplugged 10) resume 11) dp_display_host_init() ==> core_initialized = true;
#8 driver probe dongle plugged in without HDMI cable attached suspend resume
1) driver probe ==> core_initialized = false; phy_initialized = false; 2) dp_display_host_init() ==> core_initialized = true; 3) dongle plug in 4) generate plug-in interrupt triggers handler 5) dp_display_phy_init() ==> phy_initialized = true; 6) suspend 7) dp_display_host_deinit() ==> core_initialized = false; 8) dp_display_host_phy_exit() ==> phy_initialize = false; 9) resume 10) dp_display_host_init() ==> core_initialized = true; 11) generate plug-in interrupt 12) dp_display_phy_init() ==> phy_initialize = true;
NOTE: same case #6
#9 driver probe dongle plugged in without HDMI cable attached suspend HDMI cable attached during suspend resume
1) driver probe ==> core_initialized = false; phy_initialized = false; 2) dp_display_host_init() ==> core_initialized = true; 3) dongle plugged in 4) generate plug-in interrupt triggers handler 5) dp_display_phy_init() ==> phy_initialized = true; 6) suspend 7) dp_display_host_deinit() ==> core_initialized = false; 8) dp_display_host_phy_exit() ==> phy_initialize = false; 9) HDMI cable attached 10) resume 11) dp_display_host_init() ==> core_initialized = true; 12) generate plug-in interrupt 13) dp_display_phy_init() ==> phy_initialize = true;
What's the state of the phy and core initialized variable at the end of each of these scenarios? Please fill out the truth table.
+-----------------+------------------------ | false | true | +-----------------+------------------------ phy_initialized | | | +-----------------+------------------------ core_initialized | | #1, | +-----------------+------------------------
I guess we also need eDP scenarios, but that's probably simpler
#10 eDP panel connected driver probe
1) driver probe ==> core_initialized = false; phy_initialized = false; 2) dp_display_host_init() ==> core_initialized = true; 3) generate plug-in interrupt triggers handler 4) dp_display_phy_init() ==> phy_initialized = true;
#11 eDP panel disconnected driver probe
NOTE: eDP panel can not be disconnected
#12 eDP panel disconnected driver probe suspend resume
NOTE: assume edp panel connected 1) driver probe ==> core_initialized = false; phy_initialized = false; 2) dp_display_host_init() ==> core_initialized = true; 3) generate plug-in interrupt triggers handler 4) dp_display_phy_init() ==> phy_initialized = true; 5) suspend 6) dp_display_host_deinit() ==> core_initialized = false; 7) dp_display_host_phy_exit() ==> phy_initialize = false; 8) resume 9) dp_display_host_init() ==> core_initialized = true; 10) generate plug-in interrupt 11) dp_display_phy_init() ==> phy_initialize = true;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c >> index 7cc4d21..f6bb4bc 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> @@ -83,6 +83,7 @@ struct dp_display_private { >> >> /* state variables */ >> bool core_initialized; >> + bool phy_initialized; >> bool hpd_irq_on; >> bool audio_supported; >> >> @@ -372,21 +373,46 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp) >> return rc; >> } >> >> -static void dp_display_host_init(struct dp_display_private *dp, int reset) >> +static void dp_display_host_phy_init(struct dp_display_private *dp) >> { >> - bool flip = false; >> + DRM_DEBUG_DP("core_init=%d phy_init=%d\n", >> + dp->core_initialized, dp->phy_initialized); >> >> + if (!dp->phy_initialized) { >> + dp_ctrl_phy_init(dp->ctrl); >> + dp->phy_initialized = true; >> + } >> +} >> + >> +static void dp_display_host_phy_exit(struct dp_display_private *dp) >> +{ >> + DRM_DEBUG_DP("core_init=%d phy_init=%d\n", >> + dp->core_initialized, dp->phy_initialized); >> + >> + if (dp->phy_initialized) { >> + dp_ctrl_phy_exit(dp->ctrl); >> + dp->phy_initialized = false; >> + } >> +} >> + >> +static void dp_display_host_init(struct dp_display_private *dp) >> +{ >> DRM_DEBUG_DP("core_initialized=%d\n", dp->core_initialized); >> if (dp->core_initialized) { >> DRM_DEBUG_DP("DP core already initialized\n"); >> return; >> } >> >> - if (dp->usbpd->orientation == ORIENTATION_CC2) >> - flip = true; >> + dp_power_init(dp->power, false); >> + dp_ctrl_reset_irq_ctrl(dp->ctrl, true); >> + >> + /* >> + * eDP is the embedded primary display and has its own phy >> + * initialize phy immediately > Question still stands why we can't wait for hpd high from the eDP panel. > Also, I think "has its own phy" means that it's not part of a combo > USB+DP phy? Can you please clarify? > >> + */ >> + if (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP) >> + dp_display_host_phy_init(dp); >> >> - dp_power_init(dp->power, flip); >> - dp_ctrl_host_init(dp->ctrl, flip, reset); >> dp_aux_init(dp->aux); >> dp->core_initialized = true; >> } >> @@ -1306,20 +1330,23 @@ static int dp_pm_resume(struct device *dev) >> dp->hpd_state = ST_DISCONNECTED; >> >> /* turn on dp ctrl/phy */ >> - dp_display_host_init(dp, true); >> + dp_display_host_init(dp); >> >> dp_catalog_ctrl_hpd_config(dp->catalog); >> >> - /* >> - * set sink to normal operation mode -- D0 >> - * before dpcd read >> - */ >> - dp_link_psm_config(dp->link, &dp->panel->link_info, false); >> >> if (dp_catalog_link_is_connected(dp->catalog)) { >> + /* >> + * set sink to normal operation mode -- D0 >> + * before dpcd read >> + */ >> + dp_display_host_phy_init(dp); >> + dp_link_psm_config(dp->link, &dp->panel->link_info, false); >> sink_count = drm_dp_read_sink_count(dp->aux); >> if (sink_count < 0) >> sink_count = 0; >> + >> + dp_display_host_phy_exit(dp); > Why is the phy exited on resume when the link is still connected? Is > this supposed to be done only when the sink_count is 0? And how does > this interact with eDP where the phy is initialized by the call to > dp_display_host_init() earlier in this function. > >> } >> >> dp->link->sink_count = sink_count; >> @@ -1366,6 +1393,8 @@ static int dp_pm_suspend(struct device *dev) >> dp_display_host_deinit(dp); >> } >> >> + dp_display_host_phy_exit(dp); >> + >> dp->hpd_state = ST_SUSPENDED; >> >> /* host_init will be called at pm_resume */
| |