| Message ID | 20220502165316.4167199-6-bjorn.andersson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/dp: implement HPD notifications handling | expand |
On 5/2/2022 9:53 AM, Bjorn Andersson wrote: > The Qualcomm DisplayPort driver contains traces of the necessary > plumbing to hook up USB HPD, in the form of the dp_hpd module and the > dp_usbpd_cb struct. Use this as basis for implementing the > hpd_notify() callback, by amending the dp_hpd module with the > missing logic. > > Overall the solution is similar to what's done downstream, but upstream > all the code to disect the HPD notification lives on the calling side of > drm_connector_oob_hotplug_event(). > > drm_connector_oob_hotplug_event() performs the lookup of the > drm_connector based on fwnode, hence the need to assign the fwnode in > dp_drm_connector_init(). > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > > Changes since v3: > - Implements hpd_notify instead of oob_hotplug_event > - Rebased on new cleanup patch from Dmitry > - Set hpd_state to ST_MAINLINK_READY when dp_display_usbpd_configure() succeeds > > drivers/gpu/drm/msm/dp/dp_display.c | 26 ++++++++++++++++++++++++++ > drivers/gpu/drm/msm/dp/dp_display.h | 1 + > drivers/gpu/drm/msm/dp/dp_drm.c | 3 +++ > drivers/gpu/drm/msm/dp/dp_drm.h | 2 ++ > 4 files changed, 32 insertions(+) > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index b447446d75e9..080294ac6144 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -83,6 +83,8 @@ struct dp_display_private { > bool hpd_irq_on; > bool audio_supported; > > + bool connected; > + > struct drm_device *drm_dev; > struct platform_device *pdev; > struct dentry *root; > @@ -1271,6 +1273,7 @@ static int dp_display_probe(struct platform_device *pdev) > if (!desc) > return -EINVAL; > > + dp->dp_display.dev = &pdev->dev; > dp->pdev = pdev; > dp->name = "drm_dp"; > dp->dp_display.connector_type = desc->connector_type; > @@ -1760,3 +1763,26 @@ void dp_bridge_mode_set(struct drm_bridge *drm_bridge, > dp_display->dp_mode.h_active_low = > !!(dp_display->dp_mode.drm_mode.flags & DRM_MODE_FLAG_NHSYNC); > } > + > +void dp_bridge_hpd_notify(struct drm_bridge *bridge, > + enum drm_connector_status status) > +{ > + struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge); > + struct msm_dp *dp = dp_bridge->dp_display; > + struct dp_display_private *dp_display = container_of(dp, struct dp_display_private, dp_display); > + int ret; > + > + drm_dbg_dp(dp_display->drm_dev, "status: %d connected: %d\n", status, dp_display->connected); > + > + if (!dp_display->connected && status == connector_status_connected) { > + dp_display->connected = true; > + ret = dp_display_usbpd_configure(dp_display); > + if (!ret) > + dp_display->hpd_state = ST_MAINLINK_READY; > + } else if (status != connector_status_connected) { > + dp_display->connected = false; > + dp_display_notify_disconnect(dp_display); > + } else { > + dp_display_usbpd_attention(dp_display); > + } > +} I would assume dp_bridge_hpd_notify() will server same purpose as dp_display_irq_handler() if hpd_notification is enabled. In that case, should dp_bridge_hpd_notify() add EV_HPD_PLUG_INT/EV_IRQ_HPD_INT/EV_HPD_UNPLUG_INT into event q to kick off corresponding dp_hpd_plug_handle()/dp_irq_hpd_handle()/dp_hpd_unplug_handle()? By the way, I am going to test this patch out. Any patches I have to pull in before apply this serial patches? > diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h > index 4f9fe4d7610b..2d2614bc5a14 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.h > +++ b/drivers/gpu/drm/msm/dp/dp_display.h > @@ -11,6 +11,7 @@ > #include "disp/msm_disp_snapshot.h" > > struct msm_dp { > + struct device *dev; > struct drm_device *drm_dev; > struct device *codec_dev; > struct drm_bridge *bridge; > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c > index 62d58b9c4647..821cfd37b1fb 100644 > --- a/drivers/gpu/drm/msm/dp/dp_drm.c > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c > @@ -68,6 +68,7 @@ static const struct drm_bridge_funcs dp_bridge_ops = { > .mode_valid = dp_bridge_mode_valid, > .get_modes = dp_bridge_get_modes, > .detect = dp_bridge_detect, > + .hpd_notify = dp_bridge_hpd_notify, > }; > > struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, > @@ -138,6 +139,8 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display) > if (IS_ERR(connector)) > return connector; > > + connector->fwnode = fwnode_handle_get(dev_fwnode(dp_display->dev)); > + > drm_connector_attach_encoder(connector, dp_display->encoder); > > return connector; > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h > index f4b1ed1e24f7..3b7480a86844 100644 > --- a/drivers/gpu/drm/msm/dp/dp_drm.h > +++ b/drivers/gpu/drm/msm/dp/dp_drm.h > @@ -32,5 +32,7 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge, > void dp_bridge_mode_set(struct drm_bridge *drm_bridge, > const struct drm_display_mode *mode, > const struct drm_display_mode *adjusted_mode); > +void dp_bridge_hpd_notify(struct drm_bridge *bridge, > + enum drm_connector_status status); > > #endif /* _DP_DRM_H_ */
On Mon 02 May 13:59 PDT 2022, Kuogee Hsieh wrote: > > On 5/2/2022 9:53 AM, Bjorn Andersson wrote: > > The Qualcomm DisplayPort driver contains traces of the necessary > > plumbing to hook up USB HPD, in the form of the dp_hpd module and the > > dp_usbpd_cb struct. Use this as basis for implementing the > > hpd_notify() callback, by amending the dp_hpd module with the > > missing logic. > > > > Overall the solution is similar to what's done downstream, but upstream > > all the code to disect the HPD notification lives on the calling side of > > drm_connector_oob_hotplug_event(). > > > > drm_connector_oob_hotplug_event() performs the lookup of the > > drm_connector based on fwnode, hence the need to assign the fwnode in > > dp_drm_connector_init(). > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > > > Changes since v3: > > - Implements hpd_notify instead of oob_hotplug_event > > - Rebased on new cleanup patch from Dmitry > > - Set hpd_state to ST_MAINLINK_READY when dp_display_usbpd_configure() succeeds > > > > drivers/gpu/drm/msm/dp/dp_display.c | 26 ++++++++++++++++++++++++++ > > drivers/gpu/drm/msm/dp/dp_display.h | 1 + > > drivers/gpu/drm/msm/dp/dp_drm.c | 3 +++ > > drivers/gpu/drm/msm/dp/dp_drm.h | 2 ++ > > 4 files changed, 32 insertions(+) > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > > index b447446d75e9..080294ac6144 100644 > > --- a/drivers/gpu/drm/msm/dp/dp_display.c > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > > @@ -83,6 +83,8 @@ struct dp_display_private { > > bool hpd_irq_on; > > bool audio_supported; > > + bool connected; > > + > > struct drm_device *drm_dev; > > struct platform_device *pdev; > > struct dentry *root; > > @@ -1271,6 +1273,7 @@ static int dp_display_probe(struct platform_device *pdev) > > if (!desc) > > return -EINVAL; > > + dp->dp_display.dev = &pdev->dev; > > dp->pdev = pdev; > > dp->name = "drm_dp"; > > dp->dp_display.connector_type = desc->connector_type; > > @@ -1760,3 +1763,26 @@ void dp_bridge_mode_set(struct drm_bridge *drm_bridge, > > dp_display->dp_mode.h_active_low = > > !!(dp_display->dp_mode.drm_mode.flags & DRM_MODE_FLAG_NHSYNC); > > } > > + > > +void dp_bridge_hpd_notify(struct drm_bridge *bridge, > > + enum drm_connector_status status) > > +{ > > + struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge); > > + struct msm_dp *dp = dp_bridge->dp_display; > > + struct dp_display_private *dp_display = container_of(dp, struct dp_display_private, dp_display); > > + int ret; > > + > > + drm_dbg_dp(dp_display->drm_dev, "status: %d connected: %d\n", status, dp_display->connected); > > + > > + if (!dp_display->connected && status == connector_status_connected) { > > + dp_display->connected = true; > > + ret = dp_display_usbpd_configure(dp_display); > > + if (!ret) > > + dp_display->hpd_state = ST_MAINLINK_READY; > > + } else if (status != connector_status_connected) { > > + dp_display->connected = false; > > + dp_display_notify_disconnect(dp_display); > > + } else { > > + dp_display_usbpd_attention(dp_display); > > + } > > +} > > I would assume dp_bridge_hpd_notify() will server same purpose as > dp_display_irq_handler() if hpd_notification is enabled. > I agree with this statement. > In that case, should dp_bridge_hpd_notify() add > EV_HPD_PLUG_INT/EV_IRQ_HPD_INT/EV_HPD_UNPLUG_INT > I tried this originally, but couldn't get it to work and expected that as the downstream driver doesn't do this, there was some good reason for me not to do it either. > into event q to kick off corresponding > dp_hpd_plug_handle()/dp_irq_hpd_handle()/dp_hpd_unplug_handle()? > But since then the driver has been cleaned up significantly, so I decided to give it a test again. Unfortunately it still doesn't work, but now it's easier to trace. Replacing the 3 cases with relevant calls to dp_add_event() results in us inserting a EV_HPD_UNPLUG_INT event really early, before things has been brought up. This will result in dp_hpd_unplug_handle() trying to disable the dp_catalog_hpd_config_intr(), which will crash as the hardware isn't yet clocked up. Further more, this points out the main difference between the normal HPD code and the USB HPD code; dp_catalog_hpd_config_intr() will enable the plug/unplug interrupts, which it shouldn't do for USB-controlled. So it seems we need two code paths after all. > By the way, I am going to test this patch out. > > Any patches I have to pull in before apply this serial patches? > The patches applies on Dmitry's msm-next-staging, which I've merged on top of linux-next together with a number of pending patches to get the DPU up on SM8350 and a pmic_glink driver which I'm about to post. But to validate that it doesn't affect your non-USB case, Dmitry's branch should be sufficient. Thanks, Bjorn > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h > > index 4f9fe4d7610b..2d2614bc5a14 100644 > > --- a/drivers/gpu/drm/msm/dp/dp_display.h > > +++ b/drivers/gpu/drm/msm/dp/dp_display.h > > @@ -11,6 +11,7 @@ > > #include "disp/msm_disp_snapshot.h" > > struct msm_dp { > > + struct device *dev; > > struct drm_device *drm_dev; > > struct device *codec_dev; > > struct drm_bridge *bridge; > > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c > > index 62d58b9c4647..821cfd37b1fb 100644 > > --- a/drivers/gpu/drm/msm/dp/dp_drm.c > > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c > > @@ -68,6 +68,7 @@ static const struct drm_bridge_funcs dp_bridge_ops = { > > .mode_valid = dp_bridge_mode_valid, > > .get_modes = dp_bridge_get_modes, > > .detect = dp_bridge_detect, > > + .hpd_notify = dp_bridge_hpd_notify, > > }; > > struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, > > @@ -138,6 +139,8 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display) > > if (IS_ERR(connector)) > > return connector; > > + connector->fwnode = fwnode_handle_get(dev_fwnode(dp_display->dev)); > > + > > drm_connector_attach_encoder(connector, dp_display->encoder); > > return connector; > > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h > > index f4b1ed1e24f7..3b7480a86844 100644 > > --- a/drivers/gpu/drm/msm/dp/dp_drm.h > > +++ b/drivers/gpu/drm/msm/dp/dp_drm.h > > @@ -32,5 +32,7 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge, > > void dp_bridge_mode_set(struct drm_bridge *drm_bridge, > > const struct drm_display_mode *mode, > > const struct drm_display_mode *adjusted_mode); > > +void dp_bridge_hpd_notify(struct drm_bridge *bridge, > > + enum drm_connector_status status); > > #endif /* _DP_DRM_H_ */
On 5/2/2022 3:29 PM, Bjorn Andersson wrote: > On Mon 02 May 13:59 PDT 2022, Kuogee Hsieh wrote: > >> On 5/2/2022 9:53 AM, Bjorn Andersson wrote: >>> The Qualcomm DisplayPort driver contains traces of the necessary >>> plumbing to hook up USB HPD, in the form of the dp_hpd module and the >>> dp_usbpd_cb struct. Use this as basis for implementing the >>> hpd_notify() callback, by amending the dp_hpd module with the >>> missing logic. >>> >>> Overall the solution is similar to what's done downstream, but upstream >>> all the code to disect the HPD notification lives on the calling side of >>> drm_connector_oob_hotplug_event(). >>> >>> drm_connector_oob_hotplug_event() performs the lookup of the >>> drm_connector based on fwnode, hence the need to assign the fwnode in >>> dp_drm_connector_init(). >>> >>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> >>> --- >>> >>> Changes since v3: >>> - Implements hpd_notify instead of oob_hotplug_event >>> - Rebased on new cleanup patch from Dmitry >>> - Set hpd_state to ST_MAINLINK_READY when dp_display_usbpd_configure() succeeds >>> >>> drivers/gpu/drm/msm/dp/dp_display.c | 26 ++++++++++++++++++++++++++ >>> drivers/gpu/drm/msm/dp/dp_display.h | 1 + >>> drivers/gpu/drm/msm/dp/dp_drm.c | 3 +++ >>> drivers/gpu/drm/msm/dp/dp_drm.h | 2 ++ >>> 4 files changed, 32 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c >>> index b447446d75e9..080294ac6144 100644 >>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>> @@ -83,6 +83,8 @@ struct dp_display_private { >>> bool hpd_irq_on; >>> bool audio_supported; >>> + bool connected; >>> + >>> struct drm_device *drm_dev; >>> struct platform_device *pdev; >>> struct dentry *root; >>> @@ -1271,6 +1273,7 @@ static int dp_display_probe(struct platform_device *pdev) >>> if (!desc) >>> return -EINVAL; >>> + dp->dp_display.dev = &pdev->dev; >>> dp->pdev = pdev; >>> dp->name = "drm_dp"; >>> dp->dp_display.connector_type = desc->connector_type; >>> @@ -1760,3 +1763,26 @@ void dp_bridge_mode_set(struct drm_bridge *drm_bridge, >>> dp_display->dp_mode.h_active_low = >>> !!(dp_display->dp_mode.drm_mode.flags & DRM_MODE_FLAG_NHSYNC); >>> } >>> + >>> +void dp_bridge_hpd_notify(struct drm_bridge *bridge, >>> + enum drm_connector_status status) >>> +{ >>> + struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge); >>> + struct msm_dp *dp = dp_bridge->dp_display; >>> + struct dp_display_private *dp_display = container_of(dp, struct dp_display_private, dp_display); >>> + int ret; >>> + >>> + drm_dbg_dp(dp_display->drm_dev, "status: %d connected: %d\n", status, dp_display->connected); >>> + >>> + if (!dp_display->connected && status == connector_status_connected) { >>> + dp_display->connected = true; >>> + ret = dp_display_usbpd_configure(dp_display); >>> + if (!ret) >>> + dp_display->hpd_state = ST_MAINLINK_READY; >>> + } else if (status != connector_status_connected) { >>> + dp_display->connected = false; >>> + dp_display_notify_disconnect(dp_display); >>> + } else { >>> + dp_display_usbpd_attention(dp_display); >>> + } >>> +} >> I would assume dp_bridge_hpd_notify() will server same purpose as >> dp_display_irq_handler() if hpd_notification is enabled. >> > I agree with this statement. > >> In that case, should dp_bridge_hpd_notify() add >> EV_HPD_PLUG_INT/EV_IRQ_HPD_INT/EV_HPD_UNPLUG_INT >> > I tried this originally, but couldn't get it to work and expected that > as the downstream driver doesn't do this, there was some good reason for > me not to do it either. > >> into event q to kick off corresponding >> dp_hpd_plug_handle()/dp_irq_hpd_handle()/dp_hpd_unplug_handle()? >> > But since then the driver has been cleaned up significantly, so I > decided to give it a test again. > Unfortunately it still doesn't work, but now it's easier to trace. > > Replacing the 3 cases with relevant calls to dp_add_event() results in > us inserting a EV_HPD_UNPLUG_INT event really early, before things has > been brought up. This will result in dp_hpd_unplug_handle() trying to > disable the dp_catalog_hpd_config_intr(), which will crash as the > hardware isn't yet clocked up. > > Further more, this points out the main difference between the normal HPD > code and the USB HPD code; dp_catalog_hpd_config_intr() will enable the > plug/unplug interrupts, which it shouldn't do for USB-controlled. > > > So it seems we need two code paths after all. > >> By the way, I am going to test this patch out. >> >> Any patches I have to pull in before apply this serial patches? >> > The patches applies on Dmitry's msm-next-staging, which I've merged on > top of linux-next together with a number of pending patches to get the > DPU up on SM8350 and a pmic_glink driver which I'm about to post. > > But to validate that it doesn't affect your non-USB case, Dmitry's > branch should be sufficient. > > Thanks, > Bjorn Hi Bjorn, Which release image you had flashed? I have ChromeOS-test-R100-14526.69.0-trogdor.tar flashed. 1) Is this will work? 2) how about EC? do I need to upgrade EC image? Thanks, kuogee >>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h >>> index 4f9fe4d7610b..2d2614bc5a14 100644 >>> --- a/drivers/gpu/drm/msm/dp/dp_display.h >>> +++ b/drivers/gpu/drm/msm/dp/dp_display.h >>> @@ -11,6 +11,7 @@ >>> #include "disp/msm_disp_snapshot.h" >>> struct msm_dp { >>> + struct device *dev; >>> struct drm_device *drm_dev; >>> struct device *codec_dev; >>> struct drm_bridge *bridge; >>> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c >>> index 62d58b9c4647..821cfd37b1fb 100644 >>> --- a/drivers/gpu/drm/msm/dp/dp_drm.c >>> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c >>> @@ -68,6 +68,7 @@ static const struct drm_bridge_funcs dp_bridge_ops = { >>> .mode_valid = dp_bridge_mode_valid, >>> .get_modes = dp_bridge_get_modes, >>> .detect = dp_bridge_detect, >>> + .hpd_notify = dp_bridge_hpd_notify, >>> }; >>> struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, >>> @@ -138,6 +139,8 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display) >>> if (IS_ERR(connector)) >>> return connector; >>> + connector->fwnode = fwnode_handle_get(dev_fwnode(dp_display->dev)); >>> + >>> drm_connector_attach_encoder(connector, dp_display->encoder); >>> return connector; >>> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h >>> index f4b1ed1e24f7..3b7480a86844 100644 >>> --- a/drivers/gpu/drm/msm/dp/dp_drm.h >>> +++ b/drivers/gpu/drm/msm/dp/dp_drm.h >>> @@ -32,5 +32,7 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge, >>> void dp_bridge_mode_set(struct drm_bridge *drm_bridge, >>> const struct drm_display_mode *mode, >>> const struct drm_display_mode *adjusted_mode); >>> +void dp_bridge_hpd_notify(struct drm_bridge *bridge, >>> + enum drm_connector_status status); >>> #endif /* _DP_DRM_H_ */
On Mon 02 May 15:29 PDT 2022, Bjorn Andersson wrote: > On Mon 02 May 13:59 PDT 2022, Kuogee Hsieh wrote: > > > > > On 5/2/2022 9:53 AM, Bjorn Andersson wrote: > > > The Qualcomm DisplayPort driver contains traces of the necessary > > > plumbing to hook up USB HPD, in the form of the dp_hpd module and the > > > dp_usbpd_cb struct. Use this as basis for implementing the > > > hpd_notify() callback, by amending the dp_hpd module with the > > > missing logic. > > > > > > Overall the solution is similar to what's done downstream, but upstream > > > all the code to disect the HPD notification lives on the calling side of > > > drm_connector_oob_hotplug_event(). > > > > > > drm_connector_oob_hotplug_event() performs the lookup of the > > > drm_connector based on fwnode, hence the need to assign the fwnode in > > > dp_drm_connector_init(). > > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > --- > > > > > > Changes since v3: > > > - Implements hpd_notify instead of oob_hotplug_event > > > - Rebased on new cleanup patch from Dmitry > > > - Set hpd_state to ST_MAINLINK_READY when dp_display_usbpd_configure() succeeds > > > > > > drivers/gpu/drm/msm/dp/dp_display.c | 26 ++++++++++++++++++++++++++ > > > drivers/gpu/drm/msm/dp/dp_display.h | 1 + > > > drivers/gpu/drm/msm/dp/dp_drm.c | 3 +++ > > > drivers/gpu/drm/msm/dp/dp_drm.h | 2 ++ > > > 4 files changed, 32 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > > > index b447446d75e9..080294ac6144 100644 > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > > > @@ -83,6 +83,8 @@ struct dp_display_private { > > > bool hpd_irq_on; > > > bool audio_supported; > > > + bool connected; > > > + > > > struct drm_device *drm_dev; > > > struct platform_device *pdev; > > > struct dentry *root; > > > @@ -1271,6 +1273,7 @@ static int dp_display_probe(struct platform_device *pdev) > > > if (!desc) > > > return -EINVAL; > > > + dp->dp_display.dev = &pdev->dev; > > > dp->pdev = pdev; > > > dp->name = "drm_dp"; > > > dp->dp_display.connector_type = desc->connector_type; > > > @@ -1760,3 +1763,26 @@ void dp_bridge_mode_set(struct drm_bridge *drm_bridge, > > > dp_display->dp_mode.h_active_low = > > > !!(dp_display->dp_mode.drm_mode.flags & DRM_MODE_FLAG_NHSYNC); > > > } > > > + > > > +void dp_bridge_hpd_notify(struct drm_bridge *bridge, > > > + enum drm_connector_status status) > > > +{ > > > + struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge); > > > + struct msm_dp *dp = dp_bridge->dp_display; > > > + struct dp_display_private *dp_display = container_of(dp, struct dp_display_private, dp_display); > > > + int ret; > > > + > > > + drm_dbg_dp(dp_display->drm_dev, "status: %d connected: %d\n", status, dp_display->connected); > > > + > > > + if (!dp_display->connected && status == connector_status_connected) { > > > + dp_display->connected = true; > > > + ret = dp_display_usbpd_configure(dp_display); > > > + if (!ret) > > > + dp_display->hpd_state = ST_MAINLINK_READY; > > > + } else if (status != connector_status_connected) { > > > + dp_display->connected = false; > > > + dp_display_notify_disconnect(dp_display); > > > + } else { > > > + dp_display_usbpd_attention(dp_display); > > > + } > > > +} > > > > I would assume dp_bridge_hpd_notify() will server same purpose as > > dp_display_irq_handler() if hpd_notification is enabled. > > > > I agree with this statement. > > > In that case, should dp_bridge_hpd_notify() add > > EV_HPD_PLUG_INT/EV_IRQ_HPD_INT/EV_HPD_UNPLUG_INT > > > > I tried this originally, but couldn't get it to work and expected that > as the downstream driver doesn't do this, there was some good reason for > me not to do it either. > > > into event q to kick off corresponding > > dp_hpd_plug_handle()/dp_irq_hpd_handle()/dp_hpd_unplug_handle()? > > > > But since then the driver has been cleaned up significantly, so I > decided to give it a test again. > Unfortunately it still doesn't work, but now it's easier to trace. > > Replacing the 3 cases with relevant calls to dp_add_event() results in > us inserting a EV_HPD_UNPLUG_INT event really early, before things has > been brought up. This will result in dp_hpd_unplug_handle() trying to > disable the dp_catalog_hpd_config_intr(), which will crash as the > hardware isn't yet clocked up. > As I sent that I realized and have now confirmed that if I get a HPD connect before dp_display_host_init() things will not be in an appropriate state to either and the board will crash... I will dig a little bit more to see what we can do about this. Regards, Bjorn > Further more, this points out the main difference between the normal HPD > code and the USB HPD code; dp_catalog_hpd_config_intr() will enable the > plug/unplug interrupts, which it shouldn't do for USB-controlled. > > > So it seems we need two code paths after all. > > > By the way, I am going to test this patch out. > > > > Any patches I have to pull in before apply this serial patches? > > > > The patches applies on Dmitry's msm-next-staging, which I've merged on > top of linux-next together with a number of pending patches to get the > DPU up on SM8350 and a pmic_glink driver which I'm about to post. > > But to validate that it doesn't affect your non-USB case, Dmitry's > branch should be sufficient. > > Thanks, > Bjorn > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h > > > index 4f9fe4d7610b..2d2614bc5a14 100644 > > > --- a/drivers/gpu/drm/msm/dp/dp_display.h > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.h > > > @@ -11,6 +11,7 @@ > > > #include "disp/msm_disp_snapshot.h" > > > struct msm_dp { > > > + struct device *dev; > > > struct drm_device *drm_dev; > > > struct device *codec_dev; > > > struct drm_bridge *bridge; > > > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c > > > index 62d58b9c4647..821cfd37b1fb 100644 > > > --- a/drivers/gpu/drm/msm/dp/dp_drm.c > > > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c > > > @@ -68,6 +68,7 @@ static const struct drm_bridge_funcs dp_bridge_ops = { > > > .mode_valid = dp_bridge_mode_valid, > > > .get_modes = dp_bridge_get_modes, > > > .detect = dp_bridge_detect, > > > + .hpd_notify = dp_bridge_hpd_notify, > > > }; > > > struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, > > > @@ -138,6 +139,8 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display) > > > if (IS_ERR(connector)) > > > return connector; > > > + connector->fwnode = fwnode_handle_get(dev_fwnode(dp_display->dev)); > > > + > > > drm_connector_attach_encoder(connector, dp_display->encoder); > > > return connector; > > > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h > > > index f4b1ed1e24f7..3b7480a86844 100644 > > > --- a/drivers/gpu/drm/msm/dp/dp_drm.h > > > +++ b/drivers/gpu/drm/msm/dp/dp_drm.h > > > @@ -32,5 +32,7 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge, > > > void dp_bridge_mode_set(struct drm_bridge *drm_bridge, > > > const struct drm_display_mode *mode, > > > const struct drm_display_mode *adjusted_mode); > > > +void dp_bridge_hpd_notify(struct drm_bridge *bridge, > > > + enum drm_connector_status status); > > > #endif /* _DP_DRM_H_ */
On Mon 02 May 15:49 PDT 2022, Kuogee Hsieh wrote: > > On 5/2/2022 3:29 PM, Bjorn Andersson wrote: > > On Mon 02 May 13:59 PDT 2022, Kuogee Hsieh wrote: > > > > > On 5/2/2022 9:53 AM, Bjorn Andersson wrote: > > > > The Qualcomm DisplayPort driver contains traces of the necessary > > > > plumbing to hook up USB HPD, in the form of the dp_hpd module and the > > > > dp_usbpd_cb struct. Use this as basis for implementing the > > > > hpd_notify() callback, by amending the dp_hpd module with the > > > > missing logic. > > > > > > > > Overall the solution is similar to what's done downstream, but upstream > > > > all the code to disect the HPD notification lives on the calling side of > > > > drm_connector_oob_hotplug_event(). > > > > > > > > drm_connector_oob_hotplug_event() performs the lookup of the > > > > drm_connector based on fwnode, hence the need to assign the fwnode in > > > > dp_drm_connector_init(). > > > > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > > --- > > > > > > > > Changes since v3: > > > > - Implements hpd_notify instead of oob_hotplug_event > > > > - Rebased on new cleanup patch from Dmitry > > > > - Set hpd_state to ST_MAINLINK_READY when dp_display_usbpd_configure() succeeds > > > > > > > > drivers/gpu/drm/msm/dp/dp_display.c | 26 ++++++++++++++++++++++++++ > > > > drivers/gpu/drm/msm/dp/dp_display.h | 1 + > > > > drivers/gpu/drm/msm/dp/dp_drm.c | 3 +++ > > > > drivers/gpu/drm/msm/dp/dp_drm.h | 2 ++ > > > > 4 files changed, 32 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > > > > index b447446d75e9..080294ac6144 100644 > > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c > > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > > > > @@ -83,6 +83,8 @@ struct dp_display_private { > > > > bool hpd_irq_on; > > > > bool audio_supported; > > > > + bool connected; > > > > + > > > > struct drm_device *drm_dev; > > > > struct platform_device *pdev; > > > > struct dentry *root; > > > > @@ -1271,6 +1273,7 @@ static int dp_display_probe(struct platform_device *pdev) > > > > if (!desc) > > > > return -EINVAL; > > > > + dp->dp_display.dev = &pdev->dev; > > > > dp->pdev = pdev; > > > > dp->name = "drm_dp"; > > > > dp->dp_display.connector_type = desc->connector_type; > > > > @@ -1760,3 +1763,26 @@ void dp_bridge_mode_set(struct drm_bridge *drm_bridge, > > > > dp_display->dp_mode.h_active_low = > > > > !!(dp_display->dp_mode.drm_mode.flags & DRM_MODE_FLAG_NHSYNC); > > > > } > > > > + > > > > +void dp_bridge_hpd_notify(struct drm_bridge *bridge, > > > > + enum drm_connector_status status) > > > > +{ > > > > + struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge); > > > > + struct msm_dp *dp = dp_bridge->dp_display; > > > > + struct dp_display_private *dp_display = container_of(dp, struct dp_display_private, dp_display); > > > > + int ret; > > > > + > > > > + drm_dbg_dp(dp_display->drm_dev, "status: %d connected: %d\n", status, dp_display->connected); > > > > + > > > > + if (!dp_display->connected && status == connector_status_connected) { > > > > + dp_display->connected = true; > > > > + ret = dp_display_usbpd_configure(dp_display); > > > > + if (!ret) > > > > + dp_display->hpd_state = ST_MAINLINK_READY; > > > > + } else if (status != connector_status_connected) { > > > > + dp_display->connected = false; > > > > + dp_display_notify_disconnect(dp_display); > > > > + } else { > > > > + dp_display_usbpd_attention(dp_display); > > > > + } > > > > +} > > > I would assume dp_bridge_hpd_notify() will server same purpose as > > > dp_display_irq_handler() if hpd_notification is enabled. > > > > > I agree with this statement. > > > > > In that case, should dp_bridge_hpd_notify() add > > > EV_HPD_PLUG_INT/EV_IRQ_HPD_INT/EV_HPD_UNPLUG_INT > > > > > I tried this originally, but couldn't get it to work and expected that > > as the downstream driver doesn't do this, there was some good reason for > > me not to do it either. > > > > > into event q to kick off corresponding > > > dp_hpd_plug_handle()/dp_irq_hpd_handle()/dp_hpd_unplug_handle()? > > > > > But since then the driver has been cleaned up significantly, so I > > decided to give it a test again. > > Unfortunately it still doesn't work, but now it's easier to trace. > > > > Replacing the 3 cases with relevant calls to dp_add_event() results in > > us inserting a EV_HPD_UNPLUG_INT event really early, before things has > > been brought up. This will result in dp_hpd_unplug_handle() trying to > > disable the dp_catalog_hpd_config_intr(), which will crash as the > > hardware isn't yet clocked up. > > > > Further more, this points out the main difference between the normal HPD > > code and the USB HPD code; dp_catalog_hpd_config_intr() will enable the > > plug/unplug interrupts, which it shouldn't do for USB-controlled. > > > > > > So it seems we need two code paths after all. > > > > > By the way, I am going to test this patch out. > > > > > > Any patches I have to pull in before apply this serial patches? > > > > > The patches applies on Dmitry's msm-next-staging, which I've merged on > > top of linux-next together with a number of pending patches to get the > > DPU up on SM8350 and a pmic_glink driver which I'm about to post. > > > > But to validate that it doesn't affect your non-USB case, Dmitry's > > branch should be sufficient. > > > > Thanks, > > Bjorn > > Hi Bjorn, > > Which release image you had flashed? > > I have ChromeOS-test-R100-14526.69.0-trogdor.tar flashed. > > 1) Is this will work? > > 2) how about EC? do I need to upgrade EC image? > I'm not aware of the state of EC firmware for Trogdor for invoking the USB Type-C code path. As of today Trogdor relies on the EC signalling HPD using a GPIO. I'm testing this on SM8350 and SC8180X using pmic_glink. Also, going forward we will have to continue supporting both hardware IRQ and this oob based HPD interrupts (as both combinations are valid). Regards, Bjorn > Thanks, > > kuogee > > > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h > > > > index 4f9fe4d7610b..2d2614bc5a14 100644 > > > > --- a/drivers/gpu/drm/msm/dp/dp_display.h > > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.h > > > > @@ -11,6 +11,7 @@ > > > > #include "disp/msm_disp_snapshot.h" > > > > struct msm_dp { > > > > + struct device *dev; > > > > struct drm_device *drm_dev; > > > > struct device *codec_dev; > > > > struct drm_bridge *bridge; > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c > > > > index 62d58b9c4647..821cfd37b1fb 100644 > > > > --- a/drivers/gpu/drm/msm/dp/dp_drm.c > > > > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c > > > > @@ -68,6 +68,7 @@ static const struct drm_bridge_funcs dp_bridge_ops = { > > > > .mode_valid = dp_bridge_mode_valid, > > > > .get_modes = dp_bridge_get_modes, > > > > .detect = dp_bridge_detect, > > > > + .hpd_notify = dp_bridge_hpd_notify, > > > > }; > > > > struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, > > > > @@ -138,6 +139,8 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display) > > > > if (IS_ERR(connector)) > > > > return connector; > > > > + connector->fwnode = fwnode_handle_get(dev_fwnode(dp_display->dev)); > > > > + > > > > drm_connector_attach_encoder(connector, dp_display->encoder); > > > > return connector; > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h > > > > index f4b1ed1e24f7..3b7480a86844 100644 > > > > --- a/drivers/gpu/drm/msm/dp/dp_drm.h > > > > +++ b/drivers/gpu/drm/msm/dp/dp_drm.h > > > > @@ -32,5 +32,7 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge, > > > > void dp_bridge_mode_set(struct drm_bridge *drm_bridge, > > > > const struct drm_display_mode *mode, > > > > const struct drm_display_mode *adjusted_mode); > > > > +void dp_bridge_hpd_notify(struct drm_bridge *bridge, > > > > + enum drm_connector_status status); > > > > #endif /* _DP_DRM_H_ */
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index b447446d75e9..080294ac6144 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -83,6 +83,8 @@ struct dp_display_private { bool hpd_irq_on; bool audio_supported; + bool connected; + struct drm_device *drm_dev; struct platform_device *pdev; struct dentry *root; @@ -1271,6 +1273,7 @@ static int dp_display_probe(struct platform_device *pdev) if (!desc) return -EINVAL; + dp->dp_display.dev = &pdev->dev; dp->pdev = pdev; dp->name = "drm_dp"; dp->dp_display.connector_type = desc->connector_type; @@ -1760,3 +1763,26 @@ void dp_bridge_mode_set(struct drm_bridge *drm_bridge, dp_display->dp_mode.h_active_low = !!(dp_display->dp_mode.drm_mode.flags & DRM_MODE_FLAG_NHSYNC); } + +void dp_bridge_hpd_notify(struct drm_bridge *bridge, + enum drm_connector_status status) +{ + struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge); + struct msm_dp *dp = dp_bridge->dp_display; + struct dp_display_private *dp_display = container_of(dp, struct dp_display_private, dp_display); + int ret; + + drm_dbg_dp(dp_display->drm_dev, "status: %d connected: %d\n", status, dp_display->connected); + + if (!dp_display->connected && status == connector_status_connected) { + dp_display->connected = true; + ret = dp_display_usbpd_configure(dp_display); + if (!ret) + dp_display->hpd_state = ST_MAINLINK_READY; + } else if (status != connector_status_connected) { + dp_display->connected = false; + dp_display_notify_disconnect(dp_display); + } else { + dp_display_usbpd_attention(dp_display); + } +} diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h index 4f9fe4d7610b..2d2614bc5a14 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.h +++ b/drivers/gpu/drm/msm/dp/dp_display.h @@ -11,6 +11,7 @@ #include "disp/msm_disp_snapshot.h" struct msm_dp { + struct device *dev; struct drm_device *drm_dev; struct device *codec_dev; struct drm_bridge *bridge; diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c index 62d58b9c4647..821cfd37b1fb 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -68,6 +68,7 @@ static const struct drm_bridge_funcs dp_bridge_ops = { .mode_valid = dp_bridge_mode_valid, .get_modes = dp_bridge_get_modes, .detect = dp_bridge_detect, + .hpd_notify = dp_bridge_hpd_notify, }; struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, @@ -138,6 +139,8 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display) if (IS_ERR(connector)) return connector; + connector->fwnode = fwnode_handle_get(dev_fwnode(dp_display->dev)); + drm_connector_attach_encoder(connector, dp_display->encoder); return connector; diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h index f4b1ed1e24f7..3b7480a86844 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.h +++ b/drivers/gpu/drm/msm/dp/dp_drm.h @@ -32,5 +32,7 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge, void dp_bridge_mode_set(struct drm_bridge *drm_bridge, const struct drm_display_mode *mode, const struct drm_display_mode *adjusted_mode); +void dp_bridge_hpd_notify(struct drm_bridge *bridge, + enum drm_connector_status status); #endif /* _DP_DRM_H_ */
The Qualcomm DisplayPort driver contains traces of the necessary plumbing to hook up USB HPD, in the form of the dp_hpd module and the dp_usbpd_cb struct. Use this as basis for implementing the hpd_notify() callback, by amending the dp_hpd module with the missing logic. Overall the solution is similar to what's done downstream, but upstream all the code to disect the HPD notification lives on the calling side of drm_connector_oob_hotplug_event(). drm_connector_oob_hotplug_event() performs the lookup of the drm_connector based on fwnode, hence the need to assign the fwnode in dp_drm_connector_init(). Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- Changes since v3: - Implements hpd_notify instead of oob_hotplug_event - Rebased on new cleanup patch from Dmitry - Set hpd_state to ST_MAINLINK_READY when dp_display_usbpd_configure() succeeds drivers/gpu/drm/msm/dp/dp_display.c | 26 ++++++++++++++++++++++++++ drivers/gpu/drm/msm/dp/dp_display.h | 1 + drivers/gpu/drm/msm/dp/dp_drm.c | 3 +++ drivers/gpu/drm/msm/dp/dp_drm.h | 2 ++ 4 files changed, 32 insertions(+)