[PATCH v4 5/5] drm/msm/dp: Implement hpd_notify()
Kuogee Hsieh
quic_khsieh at quicinc.com
Mon May 2 22:49:16 UTC 2022
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 at 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_ */
More information about the dri-devel
mailing list