Messages in this thread | | | Date | Thu, 11 May 2023 18:57:59 +0300 | Subject | Re: [PATCH v1 1/2] drm/msm/dp: enable HDP plugin/unplugged interrupts to hpd_enable/disable | From | Dmitry Baryshkov <> |
| |
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. >> > > 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
-- With best wishes Dmitry
| |