diff mbox series

[v2,11/11] drm/i915/dp_mst: Enable HBLANK expansion quirk for UHBR rates

Message ID 20240416221010.376865-12-imre.deak@intel.com (mailing list archive)
State New, archived
Headers show
Series [v2,01/11] drm/i915/dp: Fix DSC line buffer depth programming | expand

Commit Message

Imre Deak April 16, 2024, 10:10 p.m. UTC 
Enabling the 5k@60Hz uncompressed mode on the MediaTek/Dell U3224KBA
monitor results in a blank screen, at least on MTL platforms on UHBR
link rates with some (<30) uncompressed bpp values. Enabling compression
fixes the problem, so do that for now. Windows enables DSC always if the
sink supports it and forcing it to enable the mode without compression
leads to the same problem above (which suggests a panel issue with
uncompressed mode).

The same 5k mode on non-UHBR link rates is not affected and lower
resolution modes are not affected either. The problem is similar to the
one fixed by the HBLANK expansion quirk on Synaptics hubs, with the
difference that the problematic mode has a longer HBLANK duration. Also
the monitor doesn't report supporting HBLANK expansion; either its
internal MST hub does the expansion internally - similarly to the
Synaptics hub - or the issue has another root cause, but still related
to the mode's short HBLANK duration. Enable the quirk for the monitor
adjusting the detection for the above differences.

Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_helper.c     |  2 ++
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 22 +++++++++++++++++----
 2 files changed, 20 insertions(+), 4 deletions(-)

Comments

Jani Nikula April 17, 2024, 9:39 a.m. UTC | #1 
On Wed, 17 Apr 2024, Imre Deak <imre.deak@intel.com> wrote:
> Enabling the 5k@60Hz uncompressed mode on the MediaTek/Dell U3224KBA
> monitor results in a blank screen, at least on MTL platforms on UHBR
> link rates with some (<30) uncompressed bpp values. Enabling compression
> fixes the problem, so do that for now. Windows enables DSC always if the
> sink supports it and forcing it to enable the mode without compression
> leads to the same problem above (which suggests a panel issue with
> uncompressed mode).
>
> The same 5k mode on non-UHBR link rates is not affected and lower
> resolution modes are not affected either. The problem is similar to the
> one fixed by the HBLANK expansion quirk on Synaptics hubs, with the
> difference that the problematic mode has a longer HBLANK duration. Also
> the monitor doesn't report supporting HBLANK expansion; either its
> internal MST hub does the expansion internally - similarly to the
> Synaptics hub - or the issue has another root cause, but still related
> to the mode's short HBLANK duration. Enable the quirk for the monitor
> adjusting the detection for the above differences.
>
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_helper.c     |  2 ++
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 22 +++++++++++++++++----
>  2 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index 023907da98581..79a615667aab1 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -2281,6 +2281,8 @@ static const struct dpcd_quirk dpcd_quirk_list[] = {
>  	{ OUI(0x90, 0xCC, 0x24), DEVICE_ID_ANY, true, BIT(DP_DPCD_QUIRK_DSC_WITHOUT_VIRTUAL_DPCD) },
>  	/* Synaptics DP1.4 MST hubs require DSC for some modes on which it applies HBLANK expansion. */
>  	{ OUI(0x90, 0xCC, 0x24), DEVICE_ID_ANY, true, BIT(DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC) },
> +	/* MediaTek panels (at least in U3224KBA) require DSC for modes with a short HBLANK on UHBR links. */
> +	{ OUI(0x00, 0x0C, 0xE7), DEVICE_ID_ANY, false, BIT(DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC) },
>  	/* Apple MacBookPro 2017 15 inch eDP Retina panel reports too low DP_MAX_LINK_RATE */
>  	{ OUI(0x00, 0x10, 0xfa), DEVICE_ID(101, 68, 21, 101, 98, 97), false, BIT(DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS) },
>  };
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index fb5e167c3c659..71b01f7631919 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -421,15 +421,22 @@ static int mode_hblank_period_ns(const struct drm_display_mode *mode)
>  
>  static bool
>  hblank_expansion_quirk_needs_dsc(const struct intel_connector *connector,
> -				 const struct intel_crtc_state *crtc_state)
> +				 const struct intel_crtc_state *crtc_state,
> +				 const struct link_config_limits *limits)
>  {
>  	const struct drm_display_mode *adjusted_mode =
>  		&crtc_state->hw.adjusted_mode;
> +	bool is_uhbr_sink = connector->mst_port &&
> +			    drm_dp_uhbr_channel_coding_supported(connector->mst_port->dpcd);

Why do you combine connector->mst_port to "is uhbr sink"? I think it's
confusing.

> +	int hblank_limit = is_uhbr_sink ? 500 : 300;
>  
>  	if (!connector->dp.dsc_hblank_expansion_quirk)
>  		return false;
>  
> -	if (mode_hblank_period_ns(adjusted_mode) > 300)
> +	if (is_uhbr_sink && !drm_dp_is_uhbr_rate(limits->max_rate))

I'm not saying that's not correct, but I find that condition a bit
surprising. "This does not apply to sinks capable of 128b/132b, but not
running at UHBR."

IOW, this applies to sinks not capable of 128b/132b, and sinks capable
of 128b/132b and running at UHBR.

A head scratcher.

> +		return false;
> +
> +	if (mode_hblank_period_ns(adjusted_mode) > hblank_limit)
>  		return false;
>  
>  	return true;
> @@ -445,7 +452,7 @@ adjust_limits_for_dsc_hblank_expansion_quirk(const struct intel_connector *conne
>  	const struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	int min_bpp_x16 = limits->link.min_bpp_x16;
>  
> -	if (!hblank_expansion_quirk_needs_dsc(connector, crtc_state))
> +	if (!hblank_expansion_quirk_needs_dsc(connector, crtc_state, limits))
>  		return true;
>  
>  	if (!dsc) {
> @@ -1604,7 +1611,14 @@ static bool detect_dsc_hblank_expansion_quirk(const struct intel_connector *conn
>  			      DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC))
>  		return false;
>  
> -	if (!(dpcd[DP_RECEIVE_PORT_0_CAP_0] & DP_HBLANK_EXPANSION_CAPABLE))
> +	/*
> +	 * UHBR (MST sink) devices requiring this quirk doesn't advertise the

What are you trying to say with "UHBR (MST sink)"? We've (read: I) have
been confused by this in the past, and casually equating UHBR and MST
isn't helping.

BR,
Jani.


> +	 * HBLANK expansion support. Presuming that they perform HBLANK
> +	 * expansion internally, or are affected by this issue on modes with a
> +	 * short HBLANK for other reasons.
> +	 */
> +	if (!drm_dp_uhbr_channel_coding_supported(dpcd) &&
> +	    !(dpcd[DP_RECEIVE_PORT_0_CAP_0] & DP_HBLANK_EXPANSION_CAPABLE))
>  		return false;
>  
>  	drm_dbg_kms(&i915->drm,
Imre Deak April 17, 2024, 11:46 a.m. UTC | #2 
On Wed, Apr 17, 2024 at 12:39:40PM +0300, Jani Nikula wrote:
> On Wed, 17 Apr 2024, Imre Deak <imre.deak@intel.com> wrote:
> > Enabling the 5k@60Hz uncompressed mode on the MediaTek/Dell U3224KBA
> > monitor results in a blank screen, at least on MTL platforms on UHBR
> > link rates with some (<30) uncompressed bpp values. Enabling compression
> > fixes the problem, so do that for now. Windows enables DSC always if the
> > sink supports it and forcing it to enable the mode without compression
> > leads to the same problem above (which suggests a panel issue with
> > uncompressed mode).
> >
> > The same 5k mode on non-UHBR link rates is not affected and lower
> > resolution modes are not affected either. The problem is similar to the
> > one fixed by the HBLANK expansion quirk on Synaptics hubs, with the
> > difference that the problematic mode has a longer HBLANK duration. Also
> > the monitor doesn't report supporting HBLANK expansion; either its
> > internal MST hub does the expansion internally - similarly to the
> > Synaptics hub - or the issue has another root cause, but still related
> > to the mode's short HBLANK duration. Enable the quirk for the monitor
> > adjusting the detection for the above differences.
> >
> > Cc: dri-devel@lists.freedesktop.org
> > Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/display/drm_dp_helper.c     |  2 ++
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 22 +++++++++++++++++----
> >  2 files changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> > index 023907da98581..79a615667aab1 100644
> > --- a/drivers/gpu/drm/display/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> > @@ -2281,6 +2281,8 @@ static const struct dpcd_quirk dpcd_quirk_list[] = {
> >  	{ OUI(0x90, 0xCC, 0x24), DEVICE_ID_ANY, true, BIT(DP_DPCD_QUIRK_DSC_WITHOUT_VIRTUAL_DPCD) },
> >  	/* Synaptics DP1.4 MST hubs require DSC for some modes on which it applies HBLANK expansion. */
> >  	{ OUI(0x90, 0xCC, 0x24), DEVICE_ID_ANY, true, BIT(DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC) },
> > +	/* MediaTek panels (at least in U3224KBA) require DSC for modes with a short HBLANK on UHBR links. */
> > +	{ OUI(0x00, 0x0C, 0xE7), DEVICE_ID_ANY, false, BIT(DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC) },
> >  	/* Apple MacBookPro 2017 15 inch eDP Retina panel reports too low DP_MAX_LINK_RATE */
> >  	{ OUI(0x00, 0x10, 0xfa), DEVICE_ID(101, 68, 21, 101, 98, 97), false, BIT(DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS) },
> >  };
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index fb5e167c3c659..71b01f7631919 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -421,15 +421,22 @@ static int mode_hblank_period_ns(const struct drm_display_mode *mode)
> >  
> >  static bool
> >  hblank_expansion_quirk_needs_dsc(const struct intel_connector *connector,
> > -				 const struct intel_crtc_state *crtc_state)
> > +				 const struct intel_crtc_state *crtc_state,
> > +				 const struct link_config_limits *limits)
> >  {
> >  	const struct drm_display_mode *adjusted_mode =
> >  		&crtc_state->hw.adjusted_mode;
> > +	bool is_uhbr_sink = connector->mst_port &&
> > +			    drm_dp_uhbr_channel_coding_supported(connector->mst_port->dpcd);
> 
> Why do you combine connector->mst_port to "is uhbr sink"? I think it's
> confusing.

It is a way to get the DPCD of the root port, to determine if it
supports UHBR.

> > +	int hblank_limit = is_uhbr_sink ? 500 : 300;
> >  
> >  	if (!connector->dp.dsc_hblank_expansion_quirk)
> >  		return false;
> >  
> > -	if (mode_hblank_period_ns(adjusted_mode) > 300)
> > +	if (is_uhbr_sink && !drm_dp_is_uhbr_rate(limits->max_rate))
> 
> I'm not saying that's not correct, but I find that condition a bit
> surprising. "This does not apply to sinks capable of 128b/132b, but not
> running at UHBR."
> 
> IOW, this applies to sinks not capable of 128b/132b, and sinks capable
> of 128b/132b and running at UHBR.

Yes, on the particular monitor I tested and enabled the quirk for - DELL
U3224KBA - all the modes work fine in decompressed mode on non-UHBR link
rates, so it remains possible to enable those modes without DSC on non-UHBR
link rates.

> A head scratcher.
> 
> > +		return false;
> > +
> > +	if (mode_hblank_period_ns(adjusted_mode) > hblank_limit)
> >  		return false;
> >  
> >  	return true;
> > @@ -445,7 +452,7 @@ adjust_limits_for_dsc_hblank_expansion_quirk(const struct intel_connector *conne
> >  	const struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >  	int min_bpp_x16 = limits->link.min_bpp_x16;
> >  
> > -	if (!hblank_expansion_quirk_needs_dsc(connector, crtc_state))
> > +	if (!hblank_expansion_quirk_needs_dsc(connector, crtc_state, limits))
> >  		return true;
> >  
> >  	if (!dsc) {
> > @@ -1604,7 +1611,14 @@ static bool detect_dsc_hblank_expansion_quirk(const struct intel_connector *conn
> >  			      DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC))
> >  		return false;
> >  
> > -	if (!(dpcd[DP_RECEIVE_PORT_0_CAP_0] & DP_HBLANK_EXPANSION_CAPABLE))
> > +	/*
> > +	 * UHBR (MST sink) devices requiring this quirk doesn't advertise the
> 
> What are you trying to say with "UHBR (MST sink)"? We've (read: I) have
> been confused by this in the past, and casually equating UHBR and MST
> isn't helping.

"MST sink" above is a distinction vs. the MST hubs the quirk is also
enabled for, the latter ones setting the HBLANK expansion cap flag,
while the former one doesn't.

> BR,
> Jani.
> 
> 
> > +	 * HBLANK expansion support. Presuming that they perform HBLANK
> > +	 * expansion internally, or are affected by this issue on modes with a
> > +	 * short HBLANK for other reasons.
> > +	 */
> > +	if (!drm_dp_uhbr_channel_coding_supported(dpcd) &&
> > +	    !(dpcd[DP_RECEIVE_PORT_0_CAP_0] & DP_HBLANK_EXPANSION_CAPABLE))
> >  		return false;
> >  
> >  	drm_dbg_kms(&i915->drm,
> 
> -- 
> Jani Nikula, Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
index 023907da98581..79a615667aab1 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -2281,6 +2281,8 @@  static const struct dpcd_quirk dpcd_quirk_list[] = {
 	{ OUI(0x90, 0xCC, 0x24), DEVICE_ID_ANY, true, BIT(DP_DPCD_QUIRK_DSC_WITHOUT_VIRTUAL_DPCD) },
 	/* Synaptics DP1.4 MST hubs require DSC for some modes on which it applies HBLANK expansion. */
 	{ OUI(0x90, 0xCC, 0x24), DEVICE_ID_ANY, true, BIT(DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC) },
+	/* MediaTek panels (at least in U3224KBA) require DSC for modes with a short HBLANK on UHBR links. */
+	{ OUI(0x00, 0x0C, 0xE7), DEVICE_ID_ANY, false, BIT(DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC) },
 	/* Apple MacBookPro 2017 15 inch eDP Retina panel reports too low DP_MAX_LINK_RATE */
 	{ OUI(0x00, 0x10, 0xfa), DEVICE_ID(101, 68, 21, 101, 98, 97), false, BIT(DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS) },
 };
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index fb5e167c3c659..71b01f7631919 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -421,15 +421,22 @@  static int mode_hblank_period_ns(const struct drm_display_mode *mode)
 
 static bool
 hblank_expansion_quirk_needs_dsc(const struct intel_connector *connector,
-				 const struct intel_crtc_state *crtc_state)
+				 const struct intel_crtc_state *crtc_state,
+				 const struct link_config_limits *limits)
 {
 	const struct drm_display_mode *adjusted_mode =
 		&crtc_state->hw.adjusted_mode;
+	bool is_uhbr_sink = connector->mst_port &&
+			    drm_dp_uhbr_channel_coding_supported(connector->mst_port->dpcd);
+	int hblank_limit = is_uhbr_sink ? 500 : 300;
 
 	if (!connector->dp.dsc_hblank_expansion_quirk)
 		return false;
 
-	if (mode_hblank_period_ns(adjusted_mode) > 300)
+	if (is_uhbr_sink && !drm_dp_is_uhbr_rate(limits->max_rate))
+		return false;
+
+	if (mode_hblank_period_ns(adjusted_mode) > hblank_limit)
 		return false;
 
 	return true;
@@ -445,7 +452,7 @@  adjust_limits_for_dsc_hblank_expansion_quirk(const struct intel_connector *conne
 	const struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	int min_bpp_x16 = limits->link.min_bpp_x16;
 
-	if (!hblank_expansion_quirk_needs_dsc(connector, crtc_state))
+	if (!hblank_expansion_quirk_needs_dsc(connector, crtc_state, limits))
 		return true;
 
 	if (!dsc) {
@@ -1604,7 +1611,14 @@  static bool detect_dsc_hblank_expansion_quirk(const struct intel_connector *conn
 			      DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC))
 		return false;
 
-	if (!(dpcd[DP_RECEIVE_PORT_0_CAP_0] & DP_HBLANK_EXPANSION_CAPABLE))
+	/*
+	 * UHBR (MST sink) devices requiring this quirk doesn't advertise the
+	 * HBLANK expansion support. Presuming that they perform HBLANK
+	 * expansion internally, or are affected by this issue on modes with a
+	 * short HBLANK for other reasons.
+	 */
+	if (!drm_dp_uhbr_channel_coding_supported(dpcd) &&
+	    !(dpcd[DP_RECEIVE_PORT_0_CAP_0] & DP_HBLANK_EXPANSION_CAPABLE))
 		return false;
 
 	drm_dbg_kms(&i915->drm,