[v3] drm/i915/mst: filter out the display mode exceed sink's capability
diff mbox series

Message ID 20200519035656.4885-1-shawn.c.lee@intel.com
State New
Headers show
Series
  • [v3] drm/i915/mst: filter out the display mode exceed sink's capability
Related show

Commit Message

Lee, Shawn C May 19, 2020, 3:56 a.m. UTC
So far, max dot clock rate for MST mode rely on physcial
bandwidth limitation. It would caused compatibility issue
if source display resolution exceed MST hub output ability.

For example, source DUT had DP 1.2 output capability.
And MST docking just support HDMI 1.4 spec. When a HDMI 2.0
monitor connected. Source would retrieve EDID from external
and get max resolution 4k@60fps. DP 1.2 can support 4K@60fps
because it did not surpass DP physical bandwidth limitation.
Do modeset to 4k@60fps, source output display data but MST
docking can't output HDMI properly due to this resolution
already over HDMI 1.4 spec.

Refer to commit <fcf463807596> ("drm/dp_mst: Use full_pbn
instead of available_pbn for bandwidth checks").
Source driver should refer to full_pbn to evaluate sink
output capability. And filter out the resolution surpass
sink output limitation.

v2: Using mgr->base.lock to protect full_pbn.
v3: Add ctx lock.

Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Cooper Chiou <cooper.chiou@intel.com>
Cc: Lyude Paul <lyude@redhat.com>
Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 30 ++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

Comments

Lyude Paul May 22, 2020, 6:35 p.m. UTC | #1
On Tue, 2020-05-19 at 11:56 +0800, Lee Shawn C wrote:
> So far, max dot clock rate for MST mode rely on physcial
> bandwidth limitation. It would caused compatibility issue
> if source display resolution exceed MST hub output ability.
> 
> For example, source DUT had DP 1.2 output capability.
> And MST docking just support HDMI 1.4 spec. When a HDMI 2.0
> monitor connected. Source would retrieve EDID from external
> and get max resolution 4k@60fps. DP 1.2 can support 4K@60fps
> because it did not surpass DP physical bandwidth limitation.
> Do modeset to 4k@60fps, source output display data but MST
> docking can't output HDMI properly due to this resolution
> already over HDMI 1.4 spec.
> 
> Refer to commit <fcf463807596> ("drm/dp_mst: Use full_pbn
> instead of available_pbn for bandwidth checks").
> Source driver should refer to full_pbn to evaluate sink
> output capability. And filter out the resolution surpass
> sink output limitation.
> 
> v2: Using mgr->base.lock to protect full_pbn.
> v3: Add ctx lock.
> 
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Cooper Chiou <cooper.chiou@intel.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 30 ++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 74559379384a..6b6f7eef5b68 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -611,6 +611,32 @@ static int intel_dp_mst_get_modes(struct drm_connector
> *connector)
>  	return intel_dp_mst_get_ddc_modes(connector);
>  }
>  
> +static bool
> +intel_dp_mst_mode_clock_exceed_pbn_bandwidth(struct drm_connector *connector,
> int clock, int bpp)
> +{
> +	struct intel_connector *intel_connector = to_intel_connector(connector);
> +	struct intel_dp *intel_dp = intel_connector->mst_port;
> +	struct drm_dp_mst_topology_mgr *mgr = &intel_dp->mst_mgr;
> +	struct drm_dp_mst_port *port = (to_intel_connector(connector))->port;
> +	struct drm_modeset_acquire_ctx ctx;
> +	bool ret = false;
> +
> +	if (!mgr)
> +		return ret;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +	drm_modeset_lock(&mgr->base.lock, &ctx);
> +	if (port->full_pbn)
> +		ret = (drm_dp_calc_pbn_mode(clock, bpp, false) > port-
> >full_pbn);
> +	drm_modeset_unlock(&mgr->base.lock);
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +
> +	return ret;
> +}

Sorry to do this, but this still isn't right :s, and will likely end up
deadlocking. I'm going to write up a fixed version of this patch and add you in
the Co-developed-by: tag, shouldn't take too long for me to do so I can send it
out today
> +
>  static enum drm_mode_status
>  intel_dp_mst_mode_valid(struct drm_connector *connector,
>  			struct drm_display_mode *mode)
> @@ -633,7 +659,9 @@ intel_dp_mst_mode_valid(struct drm_connector *connector,
>  	max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes);
>  	mode_rate = intel_dp_link_required(mode->clock, 18);
>  
> -	/* TODO - validate mode against available PBN for link */
> +	if (intel_dp_mst_mode_clock_exceed_pbn_bandwidth(connector, mode->clock, 
> 24))
> +		return MODE_CLOCK_HIGH;
> +
>  	if (mode->clock < 10000)
>  		return MODE_CLOCK_LOW;
>
Lyude Paul May 22, 2020, 6:39 p.m. UTC | #2
On Fri, 2020-05-22 at 14:35 -0400, Lyude Paul wrote:
> 
> On Tue, 2020-05-19 at 11:56 +0800, Lee Shawn C wrote:
> > So far, max dot clock rate for MST mode rely on physcial
> > bandwidth limitation. It would caused compatibility issue
> > if source display resolution exceed MST hub output ability.
> > 
> > For example, source DUT had DP 1.2 output capability.
> > And MST docking just support HDMI 1.4 spec. When a HDMI 2.0
> > monitor connected. Source would retrieve EDID from external
> > and get max resolution 4k@60fps. DP 1.2 can support 4K@60fps
> > because it did not surpass DP physical bandwidth limitation.
> > Do modeset to 4k@60fps, source output display data but MST
> > docking can't output HDMI properly due to this resolution
> > already over HDMI 1.4 spec.
> > 
> > Refer to commit <fcf463807596> ("drm/dp_mst: Use full_pbn
> > instead of available_pbn for bandwidth checks").
> > Source driver should refer to full_pbn to evaluate sink
> > output capability. And filter out the resolution surpass
> > sink output limitation.
> > 
> > v2: Using mgr->base.lock to protect full_pbn.
> > v3: Add ctx lock.
> > 
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Cooper Chiou <cooper.chiou@intel.com>
> > Cc: Lyude Paul <lyude@redhat.com>
> > Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 30 ++++++++++++++++++++-
> >  1 file changed, 29 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 74559379384a..6b6f7eef5b68 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -611,6 +611,32 @@ static int intel_dp_mst_get_modes(struct drm_connector
> > *connector)
> >  	return intel_dp_mst_get_ddc_modes(connector);
> >  }
> >  
> > +static bool
> > +intel_dp_mst_mode_clock_exceed_pbn_bandwidth(struct drm_connector
> > *connector,
> > int clock, int bpp)
> > +{
> > +	struct intel_connector *intel_connector = to_intel_connector(connector);
> > +	struct intel_dp *intel_dp = intel_connector->mst_port;
> > +	struct drm_dp_mst_topology_mgr *mgr = &intel_dp->mst_mgr;
> > +	struct drm_dp_mst_port *port = (to_intel_connector(connector))->port;
> > +	struct drm_modeset_acquire_ctx ctx;
> > +	bool ret = false;
> > +
> > +	if (!mgr)
> > +		return ret;
> > +
> > +	drm_modeset_acquire_init(&ctx, 0);
> > +
> > +	drm_modeset_lock(&mgr->base.lock, &ctx);
> > +	if (port->full_pbn)
> > +		ret = (drm_dp_calc_pbn_mode(clock, bpp, false) > port-
> > > full_pbn);
> > +	drm_modeset_unlock(&mgr->base.lock);
> > +
> > +	drm_modeset_drop_locks(&ctx);
> > +	drm_modeset_acquire_fini(&ctx);
> > +
> > +	return ret;
> > +}
> 
> Sorry to do this, but this still isn't right :s, and will likely end up
> deadlocking. I'm going to write up a fixed version of this patch and add you
> in
> the Co-developed-by: tag, shouldn't take too long for me to do so I can send
> it
> out today

Completely forgot to actually explain what was wrong here - this creates an
acquisition context that would be separate from the caller's lock acquisition
context. So, drm_helper_probe_single_connector_modes() would grab
connection_mutex before calling the hook, and since it's not using an
acquisition context we could potentially deadlock when locking &mgr->base.lock
if another thread locked &mgr->base.lock before connection_mutex. If we add a
lock acquisition context to drm_helper_probe_single_connector_modes and use that
to grab &mgr->base.lock, the ww-mutex code can detect the deadlock and give us a
chance to back off and try again.

anyway-i'll send out the new version of the patch asap

> > +
> >  static enum drm_mode_status
> >  intel_dp_mst_mode_valid(struct drm_connector *connector,
> >  			struct drm_display_mode *mode)
> > @@ -633,7 +659,9 @@ intel_dp_mst_mode_valid(struct drm_connector *connector,
> >  	max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes);
> >  	mode_rate = intel_dp_link_required(mode->clock, 18);
> >  
> > -	/* TODO - validate mode against available PBN for link */
> > +	if (intel_dp_mst_mode_clock_exceed_pbn_bandwidth(connector, mode-
> > >clock, 
> > 24))
> > +		return MODE_CLOCK_HIGH;
> > +
> >  	if (mode->clock < 10000)
> >  		return MODE_CLOCK_LOW;
> >
Lee, Shawn C May 25, 2020, 4:59 a.m. UTC | #3
On Fri, 2020-05-22, 06:35 p.m, Lyude Paul wrote:
>On Fri, 2020-05-22 at 14:35 -0400, Lyude Paul wrote:
>> 
>> On Tue, 2020-05-19 at 11:56 +0800, Lee Shawn C wrote:
>> > So far, max dot clock rate for MST mode rely on physcial bandwidth 
>> > limitation. It would caused compatibility issue if source display 
>> > resolution exceed MST hub output ability.
>> > 
>> > For example, source DUT had DP 1.2 output capability.
>> > And MST docking just support HDMI 1.4 spec. When a HDMI 2.0 monitor 
>> > connected. Source would retrieve EDID from external and get max 
>> > resolution 4k@60fps. DP 1.2 can support 4K@60fps because it did not 
>> > surpass DP physical bandwidth limitation.
>> > Do modeset to 4k@60fps, source output display data but MST docking 
>> > can't output HDMI properly due to this resolution already over HDMI 
>> > 1.4 spec.
>> > 
>> > Refer to commit <fcf463807596> ("drm/dp_mst: Use full_pbn instead of 
>> > available_pbn for bandwidth checks").
>> > Source driver should refer to full_pbn to evaluate sink output 
>> > capability. And filter out the resolution surpass sink output 
>> > limitation.
>> > 
>> > v2: Using mgr->base.lock to protect full_pbn.
>> > v3: Add ctx lock.
>> > 
>> > Cc: Manasi Navare <manasi.d.navare@intel.com>
>> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> > Cc: Cooper Chiou <cooper.chiou@intel.com>
>> > Cc: Lyude Paul <lyude@redhat.com>
>> > Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 30 
>> > ++++++++++++++++++++-
>> >  1 file changed, 29 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> > index 74559379384a..6b6f7eef5b68 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> > @@ -611,6 +611,32 @@ static int intel_dp_mst_get_modes(struct 
>> > drm_connector
>> > *connector)
>> >  	return intel_dp_mst_get_ddc_modes(connector);
>> >  }
>> >  
>> > +static bool
>> > +intel_dp_mst_mode_clock_exceed_pbn_bandwidth(struct drm_connector
>> > *connector,
>> > int clock, int bpp)
>> > +{
>> > +	struct intel_connector *intel_connector = to_intel_connector(connector);
>> > +	struct intel_dp *intel_dp = intel_connector->mst_port;
>> > +	struct drm_dp_mst_topology_mgr *mgr = &intel_dp->mst_mgr;
>> > +	struct drm_dp_mst_port *port = (to_intel_connector(connector))->port;
>> > +	struct drm_modeset_acquire_ctx ctx;
>> > +	bool ret = false;
>> > +
>> > +	if (!mgr)
>> > +		return ret;
>> > +
>> > +	drm_modeset_acquire_init(&ctx, 0);
>> > +
>> > +	drm_modeset_lock(&mgr->base.lock, &ctx);
>> > +	if (port->full_pbn)
>> > +		ret = (drm_dp_calc_pbn_mode(clock, bpp, false) > port-
>> > > full_pbn);
>> > +	drm_modeset_unlock(&mgr->base.lock);
>> > +
>> > +	drm_modeset_drop_locks(&ctx);
>> > +	drm_modeset_acquire_fini(&ctx);
>> > +
>> > +	return ret;
>> > +}
>> 
>> Sorry to do this, but this still isn't right :s, and will likely end 
>> up deadlocking. I'm going to write up a fixed version of this patch 
>> and add you in the Co-developed-by: tag, shouldn't take too long for 
>> me to do so I can send it out today
>
>Completely forgot to actually explain what was wrong here - this creates an acquisition context that would be separate from the caller's lock acquisition context. So, drm_helper_probe_single_connector_modes() would grab connection_mutex before calling the hook, and since it's not using an acquisition context we could potentially deadlock when locking &mgr->base.lock if another thread locked &mgr->base.lock before connection_mutex. If we add a lock acquisition context to drm_helper_probe_single_connector_modes and use that to grab &mgr->base.lock, the ww-mutex code can detect the deadlock and give us a chance to back off and try again.
>
>anyway-i'll send out the new version of the patch asap
>

I already test these 2 patches from https://patchwork.freedesktop.org/series/77573/.

Here is my test env.
1. DUT with DP 1.2 support max 4k@60fps output.                 
2. MST hub with HDMI 1.4 support max 4k@30fps output.           
3. BenQ monitor support HDMI 2.0 max 4k@60fps input.            
                                                                
WIth these 2 changes, DUT works meet our expection.             
The resolution from EDID that over 4K@30fps will not be support.

>> > +
>> >  static enum drm_mode_status
>> >  intel_dp_mst_mode_valid(struct drm_connector *connector,
>> >  			struct drm_display_mode *mode)
>> > @@ -633,7 +659,9 @@ intel_dp_mst_mode_valid(struct drm_connector *connector,
>> >  	max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes);
>> >  	mode_rate = intel_dp_link_required(mode->clock, 18);
>> >  
>> > -	/* TODO - validate mode against available PBN for link */
>> > +	if (intel_dp_mst_mode_clock_exceed_pbn_bandwidth(connector, mode-
>> > >clock,
>> > 24))
>> > +		return MODE_CLOCK_HIGH;
>> > +
>> >  	if (mode->clock < 10000)
>> >  		return MODE_CLOCK_LOW;
>> >  
>

Best regards,
Shawn

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 74559379384a..6b6f7eef5b68 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -611,6 +611,32 @@  static int intel_dp_mst_get_modes(struct drm_connector *connector)
 	return intel_dp_mst_get_ddc_modes(connector);
 }
 
+static bool
+intel_dp_mst_mode_clock_exceed_pbn_bandwidth(struct drm_connector *connector, int clock, int bpp)
+{
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct intel_dp *intel_dp = intel_connector->mst_port;
+	struct drm_dp_mst_topology_mgr *mgr = &intel_dp->mst_mgr;
+	struct drm_dp_mst_port *port = (to_intel_connector(connector))->port;
+	struct drm_modeset_acquire_ctx ctx;
+	bool ret = false;
+
+	if (!mgr)
+		return ret;
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+	drm_modeset_lock(&mgr->base.lock, &ctx);
+	if (port->full_pbn)
+		ret = (drm_dp_calc_pbn_mode(clock, bpp, false) > port->full_pbn);
+	drm_modeset_unlock(&mgr->base.lock);
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+
+	return ret;
+}
+
 static enum drm_mode_status
 intel_dp_mst_mode_valid(struct drm_connector *connector,
 			struct drm_display_mode *mode)
@@ -633,7 +659,9 @@  intel_dp_mst_mode_valid(struct drm_connector *connector,
 	max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes);
 	mode_rate = intel_dp_link_required(mode->clock, 18);
 
-	/* TODO - validate mode against available PBN for link */
+	if (intel_dp_mst_mode_clock_exceed_pbn_bandwidth(connector, mode->clock, 24))
+		return MODE_CLOCK_HIGH;
+
 	if (mode->clock < 10000)
 		return MODE_CLOCK_LOW;