Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 125 additions & 7 deletions src/common/displayport/src/dp_connectorimpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1395,6 +1395,125 @@ bool ConnectorImpl::compoundQueryAttachMST(Group * target,

if (compoundQueryAttachMSTIsDscPossible(target, modesetParams, pDscParams))
{
//
// Before entering the DSC path, check if the mode can be supported
// without compression. The SST path (compoundQueryAttachSST) already
// does this correctly: it only enables DSC when willLinkSupportModeSST
// fails. For MST, perform an equivalent pre-check using PBN to avoid
// unnecessary DSC on links with sufficient bandwidth.
//
// Unnecessary DSC adds link training complexity and can cause
// instability through MST hubs and USB-C docks, manifesting as
// spurious HPD short pulses and DPCD AUX channel failures during
// DSC capability negotiation.
//
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the related issue opened,

This causes instability through MST hubs and USB-C docks that don't handle DSC negotiation well, manifesting as spurious HPD short pulses, DPCD AUX channel failures, and intermittent display disconnection.

Monitor intermittently shows "input signal out of range" and disconnects

For some of these, I am not sure how DSC would be related. DSC shouldn't affect DP AUX or HPD. I suspect the "input signal out of range" seen on the monitor is likely due to the a link assessment failure that programs the wrong compression level supported by the sink and this change works around that programming error. Would you be willing to share a log collection using nvidia-bug-report.sh with nvidia_modeset.debug=1, so we can inspect the dplib logging and understand the failure with the display?

As for why we always opportunistically enable DSC in a MST configuration, that is done for a simple software management policy since in most MST setups, DSC will be required.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point on the DSC/AUX/HPD relationship — you may be right that the root cause is a link assessment failure programming the wrong compression level, and skipping DSC just sidesteps it. I'll collect a nvidia-bug-report.sh with nvidia_modeset.debug=1 and share it so we can inspect the dplib logging.

Regarding the opportunistic DSC policy for MST: understood. This change preserves that — it only skips DSC when the uncompressed mode passes the full compoundQueryAttachMSTGeneric validation (local link PBN, watermark, and per-device intermediate branch timeslots). If any check fails, it falls through to the DSC path as before.

Since the original submission, I've added two additional commits to fix bugs in the speculative pre-check:

  1. Fall through to DSC on intermediate hop failure (e91a9fb) — the original pre-check returned the generic result unconditionally, which meant if an intermediate branch had insufficient timeslots, DSC was never tried. Now it only returns on success; failure falls through to DSC.

  2. Save/restore compound query state (223a169) — the speculative compoundQueryAttachMSTGeneric call mutates compoundQueryResult and per-device compound_query_state. On failure, compoundQueryResult was poisoned (permanently false), and intermediate devices retained stale uncompressed timeslot allocations via bandwidthAllocatedForIndex. The DSC path would then either skip those devices or over-count their timeslots. Fixed by saving all state before the speculative call and restoring on failure. Also added an overflow guard — if the MST topology has more devices than the save array can hold, the speculative pre-check is skipped entirely.

The branch now has 3 clean DP commits and the BTF change has been moved to #1038.

bool bForceDsc = pDscParams &&
(pDscParams->forceDsc == DSC_FORCE_ENABLE);
bool bDscDualRequested =
(modesetParams.modesetInfo.mode == DSC_DUAL);

if (!bForceDsc && !bDscDualRequested)
{
unsigned base_pbn, slots, slots_pbn;
localInfo.lc.pbnRequired(localInfo.localModesetInfo,
base_pbn, slots, slots_pbn);
if (compoundQueryLocalLinkPBN + slots_pbn <=
localInfo.lc.pbnTotal())
{
//
// The uncompressed mode fits within available local link PBN.
// Speculatively try the full generic validation (watermark,
// per-device bandwidth at intermediate MST branches). If it
// succeeds, DSC is unnecessary.
//
// compoundQueryAttachMSTGeneric mutates connector and
// per-device state (compoundQueryResult, compoundQueryLocalLinkPBN,
// and device compound_query_state). Since this is a speculative
// check, save the state before and restore it on failure so the
// DSC path starts from a clean slate.
//
bool savedCompoundQueryResult = compoundQueryResult;

//
// Save per-device compound_query_state for all devices in this
// group's MST path. On failure, intermediate branch devices may
// retain stale timeslot allocations from the uncompressed attempt
// which would cause the DSC path to either skip them (via
// bandwidthAllocatedForIndex) or over-count their timeslots.
//
struct {
DeviceImpl *dev;
unsigned timeslots_used_by_query;
unsigned bandwidthAllocatedForIndex;
} savedDevState[63];
unsigned savedDevCount = 0;
bool savedDevOverflow = false;

for (Device * d = target->enumDevices(0);
d && !savedDevOverflow;
d = target->enumDevices(d))
{
DeviceImpl * tail = (DeviceImpl *)d;
while (tail && tail->getParent())
{
if (savedDevCount >= 63)
{
savedDevOverflow = true;
break;
}
savedDevState[savedDevCount].dev = tail;
savedDevState[savedDevCount].timeslots_used_by_query =
tail->bandwidth.compound_query_state.timeslots_used_by_query;
savedDevState[savedDevCount].bandwidthAllocatedForIndex =
tail->bandwidth.compound_query_state.bandwidthAllocatedForIndex;
savedDevCount++;
tail = (DeviceImpl*)tail->getParent();
}
}

//
// If we couldn't save all device state (extremely deep MST
// topology), skip the speculative pre-check to avoid partial
// state restoration on failure.
//
if (savedDevOverflow)
goto skipPreCheck;

if (compoundQueryAttachMSTGeneric(target, modesetParams,
&localInfo, pDscParams,
pErrorCode))
{
return true;
}

//
// Generic validation failed — restore all state so the DSC
// path runs on a clean slate.
//
// compoundQueryLocalLinkPBN is already rolled back by
// compoundQueryAttachMSTGeneric on failure.
//
compoundQueryResult = savedCompoundQueryResult;
for (unsigned idx = 0; idx < savedDevCount; idx++)
{
savedDevState[idx].dev->bandwidth.compound_query_state
.timeslots_used_by_query =
savedDevState[idx].timeslots_used_by_query;
savedDevState[idx].dev->bandwidth.compound_query_state
.bandwidthAllocatedForIndex =
savedDevState[idx].bandwidthAllocatedForIndex;
}

if (pErrorCode)
*pErrorCode = DP_IMP_ERROR_NONE;
skipPreCheck:
}
}

//
// DSC is required: either forced by client, DSC_DUAL requested,
// or local link bandwidth is insufficient for uncompressed mode.
//
unsigned int forceDscBitsPerPixelX16 = pDscParams->bitsPerPixelX16;
result = compoundQueryAttachMSTDsc(target, modesetParams, &localInfo,
pDscParams, pErrorCode);
Expand All @@ -1406,13 +1525,12 @@ bool ConnectorImpl::compoundQueryAttachMST(Group * target,
compoundQueryResult = compoundQueryAttachMSTGeneric(target, modesetParams, &localInfo,
pDscParams, pErrorCode);
//
// compoundQueryAttachMST Generic might fail due to the insufficient bandwidth ,
// We only check whether bpp can be fit in the available bandwidth based on the tranied link config in compoundQueryAttachMSTDsc function.
// There might be cases where the default 10 bpp might fit in the available bandwidth based on the trained link config,
// however, the bandwidth might be insufficient at the actual bottleneck link between source and sink to drive the mode, causing CompoundQueryAttachMSTGeneric to fail.
// Incase of CompoundQueryAttachMSTGeneric failure, instead of returning false, check whether the mode can be supported with the max dsc compression bpp
// and return true if it can be supported.

// compoundQueryAttachMSTGeneric might fail due to insufficient bandwidth
// at a bottleneck link between source and sink. The default 10 bpp might
// fit based on the trained link config, but the actual available bandwidth
// at intermediate MST branches may be lower. If so, retry with max DSC
// compression (8 bpp) to check if that can support the mode.
//
if (!compoundQueryResult && forceDscBitsPerPixelX16 == 0U)
{
pDscParams->bitsPerPixelX16 = MAX_DSC_COMPRESSION_BPPX16;
Expand Down