Messages in this thread | | | Date | Thu, 11 May 2023 17:16:48 -0700 | Subject | Re: [PATCH v1 1/2] drm/msm/dp: enable HDP plugin/unplugged interrupts to hpd_enable/disable | From | Kuogee Hsieh <> |
| |
On 5/11/2023 8:57 AM, Dmitry Baryshkov wrote: > On 11/05/2023 18:53, Bjorn Andersson wrote: >> On Thu, May 11, 2023 at 07:24:46AM +0300, Dmitry Baryshkov wrote: >>> On Wed, 10 May 2023 at 23:31, Kuogee Hsieh <quic_khsieh@quicinc.com> >>> wrote: >>>> >>>> The internal_hpd flag was introduced to handle external DP HPD >>>> derived from GPIO >>>> pinmuxed into DP controller. HPD plug/unplug interrupts cannot be >>>> enabled until >>>> internal_hpd flag is set to true. >>>> At both bootup and resume time, the DP driver will enable external DP >>>> plugin interrupts and handle plugin interrupt accordingly. >>>> Unfortunately >>>> dp_bridge_hpd_enable() bridge ops function was called to set >>>> internal_hpd >>>> flag to true later than where DP driver expected during bootup time. >>>> >>>> This causes external DP plugin event to not get detected and >>>> display stays blank. >>>> Move enabling HDP plugin/unplugged interrupts to >>>> dp_bridge_hpd_enable()/disable() to >>>> set internal_hpd to true along with enabling HPD plugin/unplugged >>>> interrupts >>>> simultaneously to avoid timing issue during bootup and resume. >>>> >>>> Fixes: cd198caddea7 ("drm/msm/dp: Rely on hpd_enable/disable >>>> callbacks") >>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >>> >>> Thanks for debugging this! >>> >>> However after looking at the driver I think there is more than this. >>> >>> We have several other places gated on internal_hpd flag, where we do >>> not have a strict ordering of events. >>> I see that dp_hpd_plug_handle() and dp_hpd_unplug_handle() also toggle >>> DP_DP_IRQ_HPD_INT_MASK and DP_DP_HPD_REPLUG_INT_MASK depending on >>> internal_hpd. Can we toggle all 4 interrupts from the >>> hpd_enable/hpd_disable functions? If we can do it, then I think we can >>> drop the internal_hpd flag completely. >>> >> >> Yes, that's what I believe the DRM framework intend us to do. >> >> The problem, and reason why I didn't do tat in my series, was that in >> order to update the INT_MASKs you need to clock the IP-block and that's >> done elsewhere. >> >> So, for the internal_hpd case, it seems appropriate to pm_runtime_get() >> in hpd_enable() and unmask the HPD interrupts, and mask the interrupts >> and pm_runtime_put() in hpd_disable(). >> >> >> But for edp and external HPD-signal we also need to make sure power is >> on while something is connected... > > I think this is already handled by the existing code, see calls to the > dp_display_host_init(). > >> >>> I went on and checked other places where it is used: >>> - dp_hpd_unplug_handle(), guarding DP_DP_HPD_PLUG_INT_MASK toggling. I >>> think we can drop these two calls completely. The function is under >>> the event_mutex protection, so other events can not interfere. >>> - dp_bridge_hpd_notify(). What is the point of this check? If some >>> other party informs us of the HPD event, we'd better handle it instead >>> of dropping it. Correct? In other words, I'd prefer seeing the >>> hpd_event_thread removal. Instead of that I think that on >>> HPD/plug/unplug/etc. IRQ the driver should call into the drm stack, >>> then the hpd_notify call should process those events. >>> 1) DP with GPIO: No downstream drm_bridge are connected, is_edp = false and internal HPD-logic is in used (internal_hpd = true). Power needs to be on at all times etc.
2) DP without GPIO: Downstream drm_bridge connected, is_edp = false and internal HPD-logic should not be used/enabled (internal_hpd = false). Power doesn't need to be on unless hpd_notify is invoked to tell us that there's something connected...
- dp_bridge_hpd_notify(). What is the point of this check? <== i have below two questions,
1) can you explain when/what this dp_bridge_hpd_notify() will be called?
2) is dp_bridge_hpd_notify() only will be called at above case #2? and it will not be used by case #1?
>> >> I agree, that seems to be what's expected of us from the DRM framework. >> >> Regards, >> Bjorn >> >>> >>>> --- >>>> drivers/gpu/drm/msm/dp/dp_display.c | 27 ++++++++++++++------------- >>>> 1 file changed, 14 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >>>> b/drivers/gpu/drm/msm/dp/dp_display.c >>>> index 3e13acdf..71aa944 100644 >>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>>> @@ -1088,13 +1088,6 @@ static void dp_display_config_hpd(struct >>>> dp_display_private *dp) >>>> dp_display_host_init(dp); >>>> dp_catalog_ctrl_hpd_config(dp->catalog); >>>> >>>> - /* Enable plug and unplug interrupts only if requested */ >>>> - if (dp->dp_display.internal_hpd) >>>> - dp_catalog_hpd_config_intr(dp->catalog, >>>> - DP_DP_HPD_PLUG_INT_MASK | >>>> - DP_DP_HPD_UNPLUG_INT_MASK, >>>> - true); >>>> - >>>> /* Enable interrupt first time >>>> * we are leaving dp clocks on during disconnect >>>> * and never disable interrupt >>>> @@ -1396,12 +1389,6 @@ static int dp_pm_resume(struct device *dev) >>>> >>>> dp_catalog_ctrl_hpd_config(dp->catalog); >>>> >>>> - if (dp->dp_display.internal_hpd) >>>> - dp_catalog_hpd_config_intr(dp->catalog, >>>> - DP_DP_HPD_PLUG_INT_MASK | >>>> - DP_DP_HPD_UNPLUG_INT_MASK, >>>> - true); >>>> - >>>> if (dp_catalog_link_is_connected(dp->catalog)) { >>>> /* >>>> * set sink to normal operation mode -- D0 >>>> @@ -1801,15 +1788,29 @@ void dp_bridge_hpd_enable(struct drm_bridge >>>> *bridge) >>>> { >>>> struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge); >>>> struct msm_dp *dp_display = dp_bridge->dp_display; >>>> + struct dp_display_private *dp; >>>> + >>>> + dp = container_of(dp_display, struct dp_display_private, >>>> dp_display); >>>> >>>> dp_display->internal_hpd = true; >>>> + dp_catalog_hpd_config_intr(dp->catalog, >>>> + DP_DP_HPD_PLUG_INT_MASK | >>>> + DP_DP_HPD_UNPLUG_INT_MASK, >>>> + true); >>>> } >>>> >>>> void dp_bridge_hpd_disable(struct drm_bridge *bridge) >>>> { >>>> struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge); >>>> struct msm_dp *dp_display = dp_bridge->dp_display; >>>> + struct dp_display_private *dp; >>>> + >>>> + dp = container_of(dp_display, struct dp_display_private, >>>> dp_display); >>>> >>>> + dp_catalog_hpd_config_intr(dp->catalog, >>>> + DP_DP_HPD_PLUG_INT_MASK | >>>> + DP_DP_HPD_UNPLUG_INT_MASK, >>>> + false); >>>> dp_display->internal_hpd = false; >>>> } >>> >>> -- >>> With best wishes >>> Dmitry >
| |