[i915][regression][bisected] Commit 3d3721ccb18a broke the detection of HP Lap Dock external display with DisplayPort over USB Type-C

Hi @lyudess, Hi @jani, Hi @seanpaul

After switching from Linux 5.9 to 5.10, my external HP Lap Dock display is not detected at all anymore on my Intel (SKL) laptop. I've been able to detect the source of the issue being commit 3d3721ccb18a:

$ git bisect good
3d3721ccb18a3dcec874c44120e2df7ec1c1db99 is the first bad commit
commit 3d3721ccb18a3dcec874c44120e2df7ec1c1db99
Author: Lyude Paul <lyude@redhat.com>
Date:   Wed Aug 26 14:24:49 2020 -0400

    drm/i915/dp: Extract drm_dp_read_downstream_info()
    
    We're going to be doing the same probing process in nouveau for
    determining downstream DP port capabilities, so let's deduplicate the
    work by moving i915's code for handling this into a shared helper:
    drm_dp_read_downstream_info().
    
    Note that when we do this, we also do make some functional changes while
    we're at it:
    * We always clear the downstream port info before trying to read it,
      just to make things easier for the caller
    * We skip reading downstream port info if the DPCD indicates that we
      don't support downstream port info
    * We only read as many bytes as needed for the reported number of
      downstream ports, no sense in reading the whole thing every time
    
    v2:
    * Fixup logic for calculating the downstream port length to account for
      the fact that downstream port caps can be either 1 byte or 4 bytes
      long. We can actually skip fixing the max_clock/max_bpc helpers here
      since they all check for DP_DETAILED_CAP_INFO_AVAILABLE anyway.
    * Fix ret code check for drm_dp_dpcd_read
    v5:
    * Change name from drm_dp_downstream_read_info() to
      drm_dp_read_downstream_info()
    * Also, add "See Also" sections for the various downstream info
      functions (drm_dp_read_downstream_info(), drm_dp_downstream_max_clock(),
      drm_dp_downstream_max_bpc())
    
    Reviewed-by: Sean Paul <sean@poorly.run>
    Signed-off-by: Lyude Paul <lyude@redhat.com>
    Reviewed-by: Jani Nikula <jani.nikula@intel.com>
    Link: https://patchwork.freedesktop.org/patch/msgid/20200826182456.322681-14-lyude@redhat.com

 drivers/gpu/drm/drm_dp_helper.c         | 62 +++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/display/intel_dp.c | 14 ++------
 include/drm/drm_dp_helper.h             |  3 ++
 3 files changed, 65 insertions(+), 14 deletions(-)

In the previous code, intel_dp_get_dpcd was returning true:

static bool
intel_dp_get_dpcd(struct intel_dp *intel_dp)
{
	[...]

	if (!drm_dp_is_branch(intel_dp->dpcd))
		return true; /* native DP sink */

	if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
		return true; /* no per-port downstream info */

	if (drm_dp_dpcd_read(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
			     intel_dp->downstream_ports,
			     DP_MAX_DOWNSTREAM_PORTS) < 0)
		return false; /* downstream port status fetch failed */

	return true;
}

as the drm_dp_dpcd_read(..., DP_MAX_DOWNSTREAM_PORTS) call was returning 16.

Now with the new drm_dp_downstream_port_count and drm_dp_read_downstream_info functions:

static u8 drm_dp_downstream_port_count(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
{
	u8 port_count = dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_PORT_COUNT_MASK;

	if (dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DETAILED_CAP_INFO_AVAILABLE && port_count > 4)
		port_count = 4;

	return port_count;
}

int drm_dp_read_downstream_info(struct drm_dp_aux *aux,
				const u8 dpcd[DP_RECEIVER_CAP_SIZE],
				u8 downstream_ports[DP_MAX_DOWNSTREAM_PORTS])
{
	int ret;
	u8 len;

	memset(downstream_ports, 0, DP_MAX_DOWNSTREAM_PORTS);

	/* No downstream info to read */
	if (!drm_dp_is_branch(dpcd) ||
	    dpcd[DP_DPCD_REV] < DP_DPCD_REV_10 ||
	    !(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
		return 0;

	len = drm_dp_downstream_port_count(dpcd);
	if (dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DETAILED_CAP_INFO_AVAILABLE)
		len *= 4;

	ret = drm_dp_dpcd_read(aux, DP_DOWNSTREAM_PORT_0, downstream_ports, len);
	if (ret < 0)
               return ret;
	[...]
}

len gets set to 0 (weirdly?) and the drm_dp_dpcd_read(..., len) call now returns -5 instead so intel_dp_get_dpcd now return false and the monitor doesn't get detected as a result :-(

static bool
intel_dp_get_dpcd(struct intel_dp *intel_dp)
{
	[...]

	return drm_dp_read_downstream_info(&intel_dp->aux, intel_dp->dpcd,
					   intel_dp->downstream_ports) == 0;
}

I don't know why the last parameter to drm_dp_dpcd_read was changed from DP_MAX_DOWNSTREAM_PORTS to a dynamically computed length in commit 3d3721ccb18a and why/how this length can be 0 in my case but that seems to be the source of this regression.

Testing with the latest Linus 12-rc8 tree , reverting to the original intel_dp_get_dpcd function works so the regression is really limited to this specific commit.

I've been able to get so far in my investigation by adding a few DRM_DEBUG_KMS calls, let me know if you need anything else to work on a fix for this regression, and I'll be happy to check and confirm it.

Thank you, Jérôme