Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] The patch exports and directly calls the low-level `drm_dp_mst_dpcd_read` and `drm_dp_mst_dpcd_write` functions instead of using the standard DPCD access APIs. --
commit 79505a4fb76cee58bfa580901d6c993fd41628e1 Author: Xiao Lu <[email protected]> drm/i915/display: Enable VRR/HDR over MST This commit enables VRR and HDR metadata over MST. It reads the MSA timing parameter ignore capability from the sink's virtual DPCD and updates the VSC SDP, HDR metadata, and AS-SDP computations. > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c > b/drivers/gpu/drm/display/drm_dp_mst_topology.c > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c > @@ -2164,6 +2164,7 @@ ssize_t drm_dp_mst_dpcd_read(struct drm_dp_aux *aux, > return drm_dp_send_dpcd_read(port->mgr, port, > offset, size, buffer); > } > +EXPORT_SYMBOL(drm_dp_mst_dpcd_read); [Severity: Low] Is it necessary to export drm_dp_mst_dpcd_read directly to the core DRM layer? The standard helpers like drm_dp_dpcd_read already check aux->is_remote and route to the MST equivalents. Using the standard API preserves the built-in drm_dp_dump_access debugging trace. > @@ -2187,6 +2188,7 @@ ssize_t drm_dp_mst_dpcd_write(struct drm_dp_aux *aux, > return drm_dp_send_dpcd_write(port->mgr, port, > offset, size, buffer); > } > +EXPORT_SYMBOL(drm_dp_mst_dpcd_write); [Severity: Low] Similarly, does drm_dp_mst_dpcd_write need to be exported here, or could we rely on the standard drm_dp_dpcd_write API instead? > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c [ ... ] > @@ -1157,6 +1160,24 @@ static void mst_stream_post_disable(struct > intel_atomic_state *state, > * Power down mst path before disabling the port, otherwise we end > * up getting interrupts from the sink upon detecting link loss. > */ > + if (old_crtc_state->vrr.enable) { > + u8 val; > + ssize_t ret; > + > + ret = drm_dp_mst_dpcd_read(&connector->mst.port->aux, > + DP_DOWNSPREAD_CTRL, &val, 1); [Severity: Low] Can we use drm_dp_dpcd_read here in mst_stream_post_disable instead of calling the low-level drm_dp_mst_dpcd_read function? Bypassing the standard API skips the DPCD access debug tracing that is printed when DRM_UT_DP is enabled, which can make MST debugging more difficult. > + if (ret < 0) { > + drm_dbg_kms(display->drm, > + "[CONNECTOR:%d:%s] Failed to read > DP_DOWNSPREAD_CTRL: %zd\n", > + connector->base.base.id, > + connector->base.name, ret); > + } else { > + val &= ~DP_MSA_TIMING_PAR_IGNORE_EN; > + drm_dp_mst_dpcd_write(&connector->mst.port->aux, > + DP_DOWNSPREAD_CTRL, &val, 1); [Severity: Low] Does this also apply to the write operation? Calling drm_dp_dpcd_write instead of drm_dp_mst_dpcd_write would ensure the sideband transactions are logged consistently with the rest of the DPCD accesses. [ ... ] > @@ -1274,6 +1295,24 @@ static void mst_stream_pre_enable(struct > intel_atomic_state *state, > > drm_dp_send_power_updown_phy(&intel_dp->mst.mgr, connector->mst.port, > true); > > + if (pipe_config->vrr.enable) { > + u8 val; > + ssize_t ret; > + > + ret = drm_dp_mst_dpcd_read(&connector->mst.port->aux, > + DP_DOWNSPREAD_CTRL, &val, 1); > + if (ret < 0) { > + drm_dbg_kms(display->drm, > + "[CONNECTOR:%d:%s] Failed to read > DP_DOWNSPREAD_CTRL: %zd\n", > + connector->base.base.id, > + connector->base.name, ret); > + } else { > + val |= DP_MSA_TIMING_PAR_IGNORE_EN; > + drm_dp_mst_dpcd_write(&connector->mst.port->aux, > + DP_DOWNSPREAD_CTRL, &val, 1); [Severity: Low] For consistency, could the standard read and write helpers be used here in mst_stream_pre_enable as well, avoiding the need to export the low-level MST topology functions? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
