| Message ID | 1683750665-8764-2-git-send-email-quic_khsieh@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | enable HDP plugin/unplugged interrupts to hpd_enable/disable | expand |
Quoting Kuogee Hsieh (2023-05-10 13:31:04) > The internal_hpd flag was introduced to handle external DP HPD derived from GPIO > pinmuxed into DP controller. Was it? It looks more like it was done to differentiate between eDP and DP, because internal_hpd is set only if DRM_BRIDGE_OP_HPD is set on the bridge and we only set the bridge op if the connector type is DP. The assumption looks like if you have DP connector_type, you have the gpio pinmuxed for "dp_hot" mode, which isn't exactly true. We don't treat that gpio as an irq either, because it isn't. Instead the gpio is muxed to the mdss inside the SoC and then that generates an mdss interrupt that's combined with non-HPD things like "video ready". If that all follows, then I don't quite understand why we're setting internal_hpd to false at all at runtime. It should be set to true at some point, but ideally that point is during probe. > 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> > --- > 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 > @@ -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; Can we set internal_hpd to true during probe when we see that the hpd pinmux exists? Or do any of these bits toggle in the irq status register when the gpio isn't muxed to "dp_hot" or the controller is for eDP and it doesn't have any gpio connection internally? I'm wondering if we can get by with simply enabling the "dp_hot" pin interrupts (plug/unplug/replug/irq_hpd) unconditionally and not worrying about them if eDP is there (because the pin doesn't exist inside the SoC), or if DP HPD is being signalled out of band through type-c framework.
On 5/10/2023 4:55 PM, Stephen Boyd wrote: > Quoting Kuogee Hsieh (2023-05-10 13:31:04) >> The internal_hpd flag was introduced to handle external DP HPD derived from GPIO >> pinmuxed into DP controller. > > Was it? It looks more like it was done to differentiate between eDP and > DP, because internal_hpd is set only if DRM_BRIDGE_OP_HPD is set on the > bridge and we only set the bridge op if the connector type is DP. The > assumption looks like if you have DP connector_type, you have the gpio > pinmuxed for "dp_hot" mode, which isn't exactly true. We don't treat > that gpio as an irq either, because it isn't. Instead the gpio is muxed > to the mdss inside the SoC and then that generates an mdss interrupt > that's combined with non-HPD things like "video ready". > > If that all follows, then I don't quite understand why we're setting > internal_hpd to false at all at runtime. It should be set to true at > some point, but ideally that point is during probe. > Kuogee had the same thought originally but were not entirely sure of this part of the commit message in Bjorn's original commit which introduced these changes. "This difference is not appropriately represented by the "is_edp" boolean, but is properly represented by the frameworks invocation of the hpd_enable() and hpd_disable() callbacks. Switch the current condition to rely on these callbacks instead" Does this along with below documentation mean we should generate the hpd interrupts only after hpd_enable callback happens? " * Call &drm_bridge_funcs.hpd_enable if implemented and register the given @cb * and @data as hot plug notification callback. From now on the @cb will be * called with @data when an output status change is detected by the bridge, * until hot plug notification gets disabled with drm_bridge_hpd_disable(). " Bjorn, can you please clarify this? >> 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> >> --- >> 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 >> @@ -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; > > Can we set internal_hpd to true during probe when we see that the hpd > pinmux exists? Or do any of these bits toggle in the irq status register > when the gpio isn't muxed to "dp_hot" or the controller is for eDP and > it doesn't have any gpio connection internally? I'm wondering if we can > get by with simply enabling the "dp_hot" pin interrupts > (plug/unplug/replug/irq_hpd) unconditionally and not worrying about them > if eDP is there (because the pin doesn't exist inside the SoC), or if DP > HPD is being signalled out of band through type-c framework.
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. 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. > --- > 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
On Wed, May 10, 2023 at 04:55:04PM -0700, Stephen Boyd wrote: > Quoting Kuogee Hsieh (2023-05-10 13:31:04) > > The internal_hpd flag was introduced to handle external DP HPD derived from GPIO > > pinmuxed into DP controller. > > Was it? It looks more like it was done to differentiate between eDP and > DP, because internal_hpd is set only if DRM_BRIDGE_OP_HPD is set on the > bridge and we only set the bridge op if the connector type is DP. The > assumption looks like if you have DP connector_type, you have the gpio > pinmuxed for "dp_hot" mode, which isn't exactly true. We don't treat > that gpio as an irq either, because it isn't. Instead the gpio is muxed > to the mdss inside the SoC and then that generates an mdss interrupt > that's combined with non-HPD things like "video ready". > The purpose of "internal_hpd" is to indicate track if the internal HPD-logic should be used or not. > If that all follows, then I don't quite understand why we're setting > internal_hpd to false at all at runtime. It should be set to true at > some point, but ideally that point is during probe. > The DRM framework will invoke hpd_enable on the bridge furthest out that has OP_HPD. So in the case of HPD signal being pinmuxed into the HPD-logic, dp_bridge_hpd_enable() will be invoked. I therefor think the appropriate thing to do is to move the enablement of the internal HPD-logic to dp_bridge_hpd_enable(), further more I think the correct thing to do would be to tie the power state of the DP block to this (and to when the external hpd_notify events signals that there's something attached). > > 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> > > --- > > 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 > > @@ -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; > > Can we set internal_hpd to true during probe when we see that the hpd > pinmux exists? Or do any of these bits toggle in the irq status register > when the gpio isn't muxed to "dp_hot" or the controller is for eDP and > it doesn't have any gpio connection internally? I'm wondering if we can > get by with simply enabling the "dp_hot" pin interrupts > (plug/unplug/replug/irq_hpd) unconditionally and not worrying about them > if eDP is there (because the pin doesn't exist inside the SoC), or if DP > HPD is being signalled out of band through type-c framework. It seems logical to me that the panel driver should handle HPD signaling (or signal it always-asserted), in which case it also seems reasonable that hpd_enable() would not be invoked... I didn't go far enough into that rabbit hole though, but I think it would allow us to drop the is_edp flag (which isn't at all a property of the controller, but of what you have connected). I don't think we should peak into the pinmux settings to determine if the internal HPD logic should be enabled or not, when the DRM framework already has this callback to tell us "hey, you're the one doing HPD detection!". And as mentioned above, the continuation of this is to tie the power state to hpd_enable/hpd_disable/hpd_notify and thereby allow the DP block (and MMCX) to be powered down when nothing is connected (and we don't need to drive the HPD logic). Regards, Bjorn
On Wed, May 10, 2023 at 05:39:07PM -0700, Abhinav Kumar wrote: > > > On 5/10/2023 4:55 PM, Stephen Boyd wrote: > > Quoting Kuogee Hsieh (2023-05-10 13:31:04) > > > The internal_hpd flag was introduced to handle external DP HPD derived from GPIO > > > pinmuxed into DP controller. > > > > Was it? It looks more like it was done to differentiate between eDP and > > DP, because internal_hpd is set only if DRM_BRIDGE_OP_HPD is set on the > > bridge and we only set the bridge op if the connector type is DP. The > > assumption looks like if you have DP connector_type, you have the gpio > > pinmuxed for "dp_hot" mode, which isn't exactly true. We don't treat > > that gpio as an irq either, because it isn't. Instead the gpio is muxed > > to the mdss inside the SoC and then that generates an mdss interrupt > > that's combined with non-HPD things like "video ready". > > > > If that all follows, then I don't quite understand why we're setting > > internal_hpd to false at all at runtime. It should be set to true at > > some point, but ideally that point is during probe. > > > > Kuogee had the same thought originally but were not entirely sure of this > part of the commit message in Bjorn's original commit which introduced these > changes. > > "This difference is not appropriately represented by the "is_edp" > boolean, but is properly represented by the frameworks invocation of the > hpd_enable() and hpd_disable() callbacks. Switch the current condition > to rely on these callbacks instead" > > Does this along with below documentation mean we should generate the hpd > interrupts only after hpd_enable callback happens? > > " * Call &drm_bridge_funcs.hpd_enable if implemented and register the given > @cb > * and @data as hot plug notification callback. From now on the @cb will be > * called with @data when an output status change is detected by the bridge, > * until hot plug notification gets disabled with drm_bridge_hpd_disable(). > " > > Bjorn, can you please clarify this? > We currently have 3 cases: 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... 3) eDP with or without HPD signal and/or HPD gpio. Downstream drm_bridge/panel is connected, is_edp = true and internal HPD logic is short-circuited regardless of the panel providing HPD signal or not. In #1 dp_bridge_hpd_enable() will be invoked to indicate that the DP controller is expected to perform HPD handling. In #2 dp_bridge_hpd_enable() will _not_ be invoked, instead some downstream drm_bridge/panel will get the hpd_enable() callback and will be responsible to updating the HPD state of the chain, which will cause hpd_notify to be invoked. Note that #3 is based entirely on the controller, it has currently no relation to what is attached. It seems reasonable that this is just another case of #2 (perhaps just always reporting connector_status_connected?). Regards, Bjorn > > > 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> > > > --- > > > 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 > > > @@ -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; > > > > Can we set internal_hpd to true during probe when we see that the hpd > > pinmux exists? Or do any of these bits toggle in the irq status register > > when the gpio isn't muxed to "dp_hot" or the controller is for eDP and > > it doesn't have any gpio connection internally? I'm wondering if we can > > get by with simply enabling the "dp_hot" pin interrupts > > (plug/unplug/replug/irq_hpd) unconditionally and not worrying about them > > if eDP is there (because the pin doesn't exist inside the SoC), or if DP > > HPD is being signalled out of band through type-c framework.
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 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
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
Quoting Bjorn Andersson (2023-05-11 08:44:16) > On Wed, May 10, 2023 at 05:39:07PM -0700, Abhinav Kumar wrote: > > > > > > On 5/10/2023 4:55 PM, Stephen Boyd wrote: > > > Quoting Kuogee Hsieh (2023-05-10 13:31:04) > > > > The internal_hpd flag was introduced to handle external DP HPD derived from GPIO > > > > pinmuxed into DP controller. > > > > > > Was it? It looks more like it was done to differentiate between eDP and > > > DP, because internal_hpd is set only if DRM_BRIDGE_OP_HPD is set on the > > > bridge and we only set the bridge op if the connector type is DP. The > > > assumption looks like if you have DP connector_type, you have the gpio > > > pinmuxed for "dp_hot" mode, which isn't exactly true. We don't treat > > > that gpio as an irq either, because it isn't. Instead the gpio is muxed > > > to the mdss inside the SoC and then that generates an mdss interrupt > > > that's combined with non-HPD things like "video ready". > > > > > > If that all follows, then I don't quite understand why we're setting > > > internal_hpd to false at all at runtime. It should be set to true at > > > some point, but ideally that point is during probe. > > > > > > > Kuogee had the same thought originally but were not entirely sure of this > > part of the commit message in Bjorn's original commit which introduced these > > changes. > > > > "This difference is not appropriately represented by the "is_edp" > > boolean, but is properly represented by the frameworks invocation of the > > hpd_enable() and hpd_disable() callbacks. Switch the current condition > > to rely on these callbacks instead" > > > > Does this along with below documentation mean we should generate the hpd > > interrupts only after hpd_enable callback happens? > > > > " * Call &drm_bridge_funcs.hpd_enable if implemented and register the given > > @cb > > * and @data as hot plug notification callback. From now on the @cb will be > > * called with @data when an output status change is detected by the bridge, > > * until hot plug notification gets disabled with drm_bridge_hpd_disable(). > > " > > > > Bjorn, can you please clarify this? > > > > We currently have 3 cases: > > 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... > > 3) eDP with or without HPD signal and/or HPD gpio. Downstream > drm_bridge/panel is connected, is_edp = true and internal HPD logic is > short-circuited regardless of the panel providing HPD signal or not. Oh weird. I thought that the "is_edp" controller on sc7280 didn't have HPD hardware in the PHY (phy@aec2a00), hence all the logic to avoid using the HPD interrupts and bits there. What is "is_edp" about then? > > > In #1 dp_bridge_hpd_enable() will be invoked to indicate that the DP > controller is expected to perform HPD handling. In #2 > dp_bridge_hpd_enable() will _not_ be invoked, instead some downstream > drm_bridge/panel will get the hpd_enable() callback and will be > responsible to updating the HPD state of the chain, which will cause > hpd_notify to be invoked. > > > Note that #3 is based entirely on the controller, it has currently no > relation to what is attached. It seems reasonable that this is just > another case of #2 (perhaps just always reporting > connector_status_connected?). > Looking at drm_bridge_connector_detect() the default is to consider DRM_MODE_CONNECTOR_eDP as connector_status_connected. I wonder if panel_bridge_bridge_funcs can gain a 'detect' function and also set DRM_BRIDGE_OP_DETECT if the connector_type is DRM_MODE_CONNECTOR_eDP.
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 >
On Fri, 12 May 2023 at 03:16, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > > > 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? The call chain is drm_bridge_hpd_notify() -> drm_bridge_connector_hpd_notify() -> .hpd_notify() for all drm_bridge in chain One should add a call to drm_bridge_hpd_notify() when the hotplug event has been detected. Also please note the patch https://patchwork.freedesktop.org/patch/484432/ > > 2) is dp_bridge_hpd_notify() only will be called at above case #2? and > it will not be used by case #1? Once the driver calls drm_bridge_hpd_notify() in the hpd path, the hpd_notify callbacks will be called in case#1 too. BTW: I don't see drm_bridge_hpd_notify() or drm_kms_{,connector_}_hotplug_event() HPD notifications in the DP driver at all. This should be fixed. > > > > >> > >> 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 > >
Quoting Dmitry Baryshkov (2023-05-11 17:54:19) > On Fri, 12 May 2023 at 03:16, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > > 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? > > The call chain is drm_bridge_hpd_notify() -> > drm_bridge_connector_hpd_notify() -> .hpd_notify() for all drm_bridge > in chain > > One should add a call to drm_bridge_hpd_notify() when the hotplug > event has been detected. > > Also please note the patch https://patchwork.freedesktop.org/patch/484432/ > > > > > 2) is dp_bridge_hpd_notify() only will be called at above case #2? and > > it will not be used by case #1? > > Once the driver calls drm_bridge_hpd_notify() in the hpd path, the > hpd_notify callbacks will be called in case#1 too. > > BTW: I don't see drm_bridge_hpd_notify() or > drm_kms_{,connector_}_hotplug_event() HPD notifications in the DP > driver at all. This should be fixed. > Is dp_bridge_hpd_notify() being called by drm_helper_probe_single_connector_modes() when the connectors are detected? I see drm_helper_probe_detect() calls connector->funcs->detect() which I think calls drm_bridge_connector_funcs::drm_bridge_connector_hpd_notify() but I haven't confirmed yet. The 'detect' bridge is the DP bridge in msm driver if (!dp_display->is_edp) { bridge->ops = DRM_BRIDGE_OP_DETECT | so if the bridge_connector is being used then I think when fill_modes() is called we'll run the detect cycle and call the hpd_notify callbacks on the bridge chain.
On Fri, 12 May 2023 at 21:03, Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Dmitry Baryshkov (2023-05-11 17:54:19) > > On Fri, 12 May 2023 at 03:16, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > > > 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? > > > > The call chain is drm_bridge_hpd_notify() -> > > drm_bridge_connector_hpd_notify() -> .hpd_notify() for all drm_bridge > > in chain > > > > One should add a call to drm_bridge_hpd_notify() when the hotplug > > event has been detected. > > > > Also please note the patch https://patchwork.freedesktop.org/patch/484432/ > > > > > > > > 2) is dp_bridge_hpd_notify() only will be called at above case #2? and > > > it will not be used by case #1? > > > > Once the driver calls drm_bridge_hpd_notify() in the hpd path, the > > hpd_notify callbacks will be called in case#1 too. > > > > BTW: I don't see drm_bridge_hpd_notify() or > > drm_kms_{,connector_}_hotplug_event() HPD notifications in the DP > > driver at all. This should be fixed. > > > > Is dp_bridge_hpd_notify() being called by > drm_helper_probe_single_connector_modes() when the connectors are > detected? > > I see drm_helper_probe_detect() calls connector->funcs->detect() which I > think calls > drm_bridge_connector_funcs::drm_bridge_connector_hpd_notify() but I > haven't confirmed yet. The 'detect' bridge is the DP bridge in msm > driver > > if (!dp_display->is_edp) { > bridge->ops = > DRM_BRIDGE_OP_DETECT | > > so if the bridge_connector is being used then I think when fill_modes() > is called we'll run the detect cycle and call the hpd_notify callbacks > on the bridge chain. Yes. This call chain is correct. drm_helper_probe_single_connector_modes() -> drm_bridge_connector_detect() -> drm_bridge_connector_hpd_notify(). However on HPD events the DP driver doesn't call into the drm core (which I believe should be fixed).
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; }
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> --- drivers/gpu/drm/msm/dp/dp_display.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-)