| Message ID | 20230903222432.2894093-1-dmitry.baryshkov@linaro.org |
|---|---|
| State | Accepted |
| Commit | bfcc3d8f94f4cb7b97cd666367ffc729342d30c0 |
| Headers | show |
| Series | drm/msm/dp: support setting the DP subconnector type | expand |
Quoting Dmitry Baryshkov (2023-09-03 15:24:32) > diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c > index 97ba41593820..1cb54f26f5aa 100644 > --- a/drivers/gpu/drm/msm/dp/dp_panel.c > +++ b/drivers/gpu/drm/msm/dp/dp_panel.c > @@ -162,6 +162,11 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel, > } > } > > + rc = drm_dp_read_downstream_info(panel->aux, dp_panel->dpcd, > + dp_panel->downstream_ports); > + if (rc) > + return rc; I haven't been able to test it yet, but I think with an apple dongle we'll never populate the 'downstream_ports' member if the HDMI cable is not connected when this runs. That's because this function bails out early before trying to read the downstream ports when there isn't a sink. Perhaps we need to read it again when an hpd_irq comes in, or we need to read it before bailing out from here?
On 08/09/2023 00:34, Stephen Boyd wrote: > Quoting Dmitry Baryshkov (2023-09-03 15:24:32) >> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c >> index 97ba41593820..1cb54f26f5aa 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_panel.c >> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c >> @@ -162,6 +162,11 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel, >> } >> } >> >> + rc = drm_dp_read_downstream_info(panel->aux, dp_panel->dpcd, >> + dp_panel->downstream_ports); >> + if (rc) >> + return rc; > > I haven't been able to test it yet, but I think with an apple dongle > we'll never populate the 'downstream_ports' member if the HDMI cable is > not connected when this runs. That's because this function bails out > early before trying to read the downstream ports when there isn't a > sink. Perhaps we need to read it again when an hpd_irq comes in, or we > need to read it before bailing out from here? I don't have an Apple dongle here. But I'll run a check with first connecting the dongle and plugging the HDMI afterwards. However my expectation based on my previous tests is that we only get here when the actual display is connected.
Quoting Dmitry Baryshkov (2023-09-07 14:48:54) > On 08/09/2023 00:34, Stephen Boyd wrote: > > Quoting Dmitry Baryshkov (2023-09-03 15:24:32) > >> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c > >> index 97ba41593820..1cb54f26f5aa 100644 > >> --- a/drivers/gpu/drm/msm/dp/dp_panel.c > >> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c > >> @@ -162,6 +162,11 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel, > >> } > >> } > >> > >> + rc = drm_dp_read_downstream_info(panel->aux, dp_panel->dpcd, > >> + dp_panel->downstream_ports); > >> + if (rc) > >> + return rc; > > > > I haven't been able to test it yet, but I think with an apple dongle > > we'll never populate the 'downstream_ports' member if the HDMI cable is > > not connected when this runs. That's because this function bails out > > early before trying to read the downstream ports when there isn't a > > sink. Perhaps we need to read it again when an hpd_irq comes in, or we > > need to read it before bailing out from here? > > I don't have an Apple dongle here. But I'll run a check with first > connecting the dongle and plugging the HDMI afterwards. > > However my expectation based on my previous tests is that we only get > here when the actual display is connected. > We get here when HPD is high. With an apple dongle, hpd is high when just the dongle is plugged in. That calls dp_display_process_hpd_high() which calls dp_panel_read_sink_caps(), but that returns with an error (-ENOTCONN) and then we wait for something to change. When the HDMI cable is plugged in (i.e. an actual display) we get an irq_hpd. That causes dp_irq_hpd_handle() to call dp_display_usbpd_attention_cb() which calls dp_link_process_request() that sees 'sink_request & DS_PORT_STATUS_CHANGED' and thus calls dp_display_handle_port_ststus_changed() (that has a typo right?) which hits the else condition and calls dp_display_process_hpd_high(). So yes? We will eventually call dp_panel_read_sink_caps() again, and this time not bail out early. It's probably fine.
Quoting Dmitry Baryshkov (2023-09-03 15:24:32) > Read the downstream port info and set the subconnector type accordingly. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org>
On Mon, 04 Sep 2023 01:24:32 +0300, Dmitry Baryshkov wrote: > Read the downstream port info and set the subconnector type accordingly. > > Applied, thanks! [1/1] drm/msm/dp: support setting the DP subconnector type https://gitlab.freedesktop.org/lumag/msm/-/commit/631557f9741b Best regards,
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 96bbf6fec2f1..8abeae658200 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -372,8 +372,12 @@ static int dp_display_send_hpd_notification(struct dp_display_private *dp, } /* reset video pattern flag on disconnect */ - if (!hpd) + if (!hpd) { dp->panel->video_test = false; + drm_dp_set_subconnector_property(dp->dp_display.connector, + connector_status_disconnected, + dp->panel->dpcd, dp->panel->downstream_ports); + } dp->dp_display.is_connected = hpd; @@ -401,6 +405,9 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp) dp_link_process_request(dp->link); + drm_dp_set_subconnector_property(dp->dp_display.connector, connector_status_connected, + dp->panel->dpcd, dp->panel->downstream_ports); + edid = dp->panel->edid; dp->dp_display.psr_supported = dp->panel->psr_cap.version && psr_enabled; diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c index 97ba41593820..1cb54f26f5aa 100644 --- a/drivers/gpu/drm/msm/dp/dp_panel.c +++ b/drivers/gpu/drm/msm/dp/dp_panel.c @@ -162,6 +162,11 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel, } } + rc = drm_dp_read_downstream_info(panel->aux, dp_panel->dpcd, + dp_panel->downstream_ports); + if (rc) + return rc; + kfree(dp_panel->edid); dp_panel->edid = NULL; diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h index 3cb1f8dcfd3b..a0dfc579c5f9 100644 --- a/drivers/gpu/drm/msm/dp/dp_panel.h +++ b/drivers/gpu/drm/msm/dp/dp_panel.h @@ -36,6 +36,7 @@ struct dp_panel_psr { struct dp_panel { /* dpcd raw data */ u8 dpcd[DP_RECEIVER_CAP_SIZE]; + u8 downstream_ports[DP_MAX_DOWNSTREAM_PORTS]; struct dp_link_info link_info; struct drm_dp_desc desc;
Read the downstream port info and set the subconnector type accordingly. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- Dependencies: https://patchwork.freedesktop.org/series/123221/ --- drivers/gpu/drm/msm/dp/dp_display.c | 9 ++++++++- drivers/gpu/drm/msm/dp/dp_panel.c | 5 +++++ drivers/gpu/drm/msm/dp/dp_panel.h | 1 + 3 files changed, 14 insertions(+), 1 deletion(-)