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 <[email protected]>
---
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;
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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 <[email protected]>
> ---
> 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 <[email protected]>
>> ---
>> 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 <[email protected]> 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 <[email protected]>
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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > > ---
> > > 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 <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]> 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 <[email protected]>
>>
>> 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
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 <[email protected]>
>>> 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 <[email protected]>
>>>
>>> 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 <[email protected]> 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 <[email protected]>
> >>> 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 <[email protected]>
> >>>
> >>> 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
> >
--
With best wishes
Dmitry
Quoting Dmitry Baryshkov (2023-05-11 17:54:19)
> On Fri, 12 May 2023 at 03:16, Kuogee Hsieh <[email protected]> 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 <[email protected]> wrote:
>
> Quoting Dmitry Baryshkov (2023-05-11 17:54:19)
> > On Fri, 12 May 2023 at 03:16, Kuogee Hsieh <[email protected]> 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).
--
With best wishes
Dmitry