Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/msm/dp: inconsistent sink_count check in msm_dp_panel_read_link_caps() - [Medium] drm/msm/dp: uninitialized or stale dpcd used in msm_dp_irq_hpd_handle() --
commit 11ea4fa4c607d4520f894e5a7054fde29ed84c68 Author: Vivek Kumar <[email protected]> drm/msm/dp: allow eDP sinks without DP_SINK_COUNT This commit uses drm_dp_read_sink_count_cap() in msm_dp_link_parse_sink_status_field() and sets link->sink_count to 1 when the sink count capability is not available, allowing eDP sinks that report a zero DP_SINK_COUNT to probe and enable properly. > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > b/drivers/gpu/drm/msm/dp/dp_display.c > index bce641d7d8938..5a13982d7ed4e 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -296,7 +296,8 @@ static int msm_dp_display_process_hpd_high(struct > msm_dp_display_private *dp) > return -ETIMEDOUT; > } > > - msm_dp_link_process_request(dp->link); > + msm_dp_link_process_request(dp->link, connector, dp->panel->dpcd, > + &dp->panel->desc); > > if (!dp->msm_dp_display.is_edp) > drm_dp_set_subconnector_property(connector, > @@ -515,7 +516,8 @@ static int msm_dp_irq_hpd_handle(struct > msm_dp_display_private *dp) > dp->link->sink_count); > > /* check for any test request issued by sink */ > - rc = msm_dp_link_process_request(dp->link); > + rc = msm_dp_link_process_request(dp->link, dp->msm_dp_display.connector, > + dp->panel->dpcd, &dp->panel->desc); > if (!rc) { > sink_request = dp->link->sink_request; > drm_dbg_dp(dp->drm_dev, "sink_request=%d\n", sink_request); [Severity: Medium] Can msm_dp_irq_hpd_handle() pass uninitialized or stale dp->panel->dpcd and dp->panel->desc into msm_dp_link_process_request() here? In msm_dp_irq_hpd_handle(), msm_dp_link_process_request() is called before msm_dp_display_process_hpd_high() to determine whether DS_PORT_STATUS_CHANGED is set in dp->link->sink_request: rc = msm_dp_link_process_request(dp->link, dp->msm_dp_display.connector, dp->panel->dpcd, &dp->panel->desc); if (!rc) { sink_request = dp->link->sink_request; drm_dbg_dp(dp->drm_dev, "sink_request=%d\n", sink_request); if (sink_request & DS_PORT_STATUS_CHANGED) rc = msm_dp_display_process_hpd_high(dp); else rc = msm_dp_display_handle_irq_hpd(dp); } However, dp->panel->dpcd and dp->panel->desc are only populated inside msm_dp_panel_read_link_caps(), which is only called from msm_dp_display_process_hpd_high(). In contrast, msm_dp_bridge_detect() reads DPCD and desc into stack-local variables, and msm_dp_hpd_unplug_handle() does not clear dp->panel->dpcd, dp->panel->desc, or dp->link->sink_count. If an IRQ_HPD is handled before msm_dp_display_process_hpd_high() has run (for example, when a branch device is present at boot with sink_count == 0 so msm_dp_bridge_detect() returns connector_status_disconnected without calling msm_dp_display_process_hpd_high(), or when msm_dp_bridge_hpd_notify() sees ISR_IRQ_HPD_PULSE_COUNT before a plug pass), dp->panel->dpcd is all-zero. In that state, drm_dp_read_sink_count_cap() sees dpcd[DP_DPCD_REV] == 0 and dpcd[DP_DOWNSTREAMPORT_PRESENT] == 0, returns false, skips reading DP_SINK_COUNT over AUX, and forces link->msm_dp_link.sink_count = 1. Conversely, if a previous sink was unplugged, dp->panel->dpcd and dp->panel->desc retain the previous sink's values when msm_dp_irq_hpd_handle() runs prior to msm_dp_display_process_hpd_high(). > diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c > b/drivers/gpu/drm/msm/dp/dp_panel.c > index d3468b73fdc32..938152fe8013f 100644 > --- a/drivers/gpu/drm/msm/dp/dp_panel.c > +++ b/drivers/gpu/drm/msm/dp/dp_panel.c > @@ -346,6 +346,11 @@ int msm_dp_panel_read_link_caps(struct msm_dp_panel > *msm_dp_panel, > return rc; > } > > + rc = drm_dp_read_desc(panel->aux, &msm_dp_panel->desc, > + drm_dp_is_branch(msm_dp_panel->dpcd)); > + if (rc) > + memset(&msm_dp_panel->desc, 0, sizeof(msm_dp_panel->desc)); > + > bw_code = drm_dp_link_rate_to_bw_code(msm_dp_panel->link_info.rate); > if (!is_link_rate_valid(bw_code) || > !is_lane_count_valid(msm_dp_panel->link_info.num_lanes) > || [Severity: Medium] Should the branch check just below this in msm_dp_panel_read_link_caps() also use drm_dp_read_sink_count_cap()? Looking at drm_dp_read_sink_count_cap(): return connector->connector_type != DRM_MODE_CONNECTOR_eDP && dpcd[DP_DPCD_REV] >= DP_DPCD_REV_11 && dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT && !drm_dp_has_quirk(desc, DP_DPCD_QUIRK_NO_SINK_COUNT); Whenever drm_dp_is_branch(dpcd) is false (meaning DP_DWN_STRM_PORT_PRESENT is not set), drm_dp_read_sink_count_cap() already returns false unconditionally for both eDP and external DP without inspecting connector->connector_type or desc. The only case where connector->connector_type != DRM_MODE_CONNECTOR_eDP, dpcd[DP_DPCD_REV] >= DP_DPCD_REV_11, or !drm_dp_has_quirk(desc, DP_DPCD_QUIRK_NO_SINK_COUNT) makes a difference is when drm_dp_is_branch(dpcd) is true (such as an eDP bridge chip that sets DP_DWN_STRM_PORT_PRESENT, or a branch device with DP_DPCD_QUIRK_NO_SINK_COUNT). However, in msm_dp_display_process_hpd_high(), msm_dp_panel_read_link_caps() runs before msm_dp_link_process_request() and still checks: if (drm_dp_is_branch(msm_dp_panel->dpcd)) { count = drm_dp_read_sink_count(panel->aux); if (!count) { panel->link->sink_count = 0; return -ENOTCONN; } } For any branch device where drm_dp_read_sink_count_cap() is false and DP_SINK_COUNT is 0, wouldn't msm_dp_panel_read_link_caps() set panel->link->sink_count = 0 and return -ENOTCONN before msm_dp_link_process_request() is ever reached, causing both msm_dp_display_process_hpd_high() and msm_dp_display_prepare_link() to fail? [ ... ] > > > 完美 (wait - no extra text! Just end with a blank line). Let me make sure I don't output anything after the blank line! Let's review the whole output carefully. Wait: in my thought above I didn't put anything into the final text except what's before my thought? Wait—I didn't open a thought block before `完美`! WAIT A SECOND! I wrote that directly into the output stream! Oh no, I cannot undo what's already emitted, so I should just stop immediately. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
