Re: [Intel-gfx] [PATCH] drm/i915/mst: filter out the display mode exceed sink's capability

["Lee, Shawn C" <shawn.c.lee@intel.com>]

On Sat, 2020-04-25, Lyude Paul wrote:
>
>Hi! Sorry this took me a little while to get back to, I had a couple of MST regressions that I had to look into
>
>On Sat, 2020-04-18 at 05:24 +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.
>> 
>> 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 | 24 
>> ++++++++++++++++++++-
>>  1 file changed, 23 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 61605eb8c2af..68697f463dab 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> @@ -609,6 +609,26 @@ 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;
>> +	bool ret = false;
>> +
>> +	if (!mgr)
>> +		return ret;
>> +
>> +	mutex_lock(&mgr->lock);
>
>This isn't the right lock for this - mgr->lock protects the topology layout (e.g. drm_dp_mst_port.mstb, drm_dp_mst_port.next, and drm_dp_mst_branch.ports). port->full_pbn is protected under &drm_dp_mst_topology_mgr.base.lock (not drm_dp_mst_topology_mgr.lock), so you need to first add a version of .mode_valid() to struct drm_connector_helper_funcs that accepts a drm_modeset_acquire_ctx so you can use that to safely grab &drm_dp_mst_topology_mgr.base.lock.
>

Thanks for comments! I will correct the lock to protect port->full_pbn.

>> +	if (port->full_pbn)
>
>Also - there's no reason to check if (port->full_pbn) here, so long as you're holding the right locks this should always be populated by the time we create the drm_connector for the MST port (if it's not, that's a bug that needs to be fixed).
>

Just want to make sure full_pbn value is greater than zero. As you mention in another patch, it's hard to predict sink report full or available PBN properly.

>> +		ret = (drm_dp_calc_pbn_mode(clock, bpp, false) > port-
>> >full_pbn);
>
>Finally - I'd say we should probably have a separate patch to add a helper for calculating the PBN and checking it against port->full_pbn. Maybe something that looks like this:
>
>int drm_dp_mst_mode_valid(struct drm_dp_mst_port *port, struct drm_modeset_acquire_ctx *ctx, int clock, int bpp) {
>	int ret = MODE_VALID;
>	/* TODO: DSC support */
>
>	/* ...try grabbing locks here...*/
>	if (drm_dp_calc_pbn_mode(clock, bpp, false) > port->full_pbn)
>		ret = MODE_CLOCK_HIGH;
>
>	return ret;
>}
>
>That way we might be able to add some checks for DSC capable connectors later once I've migrated most of the DSC code from amdgpu into the MST helpers

This sounds good. DRM driver provide a public function call for different vendor to check the current mode exceed PBN's limitation or not.

>> +	mutex_unlock(&mgr->lock);
>> +
>> +	return ret;
>> +}
>> +
>>  static enum drm_mode_status
>>  intel_dp_mst_mode_valid(struct drm_connector *connector,
>>  			struct drm_display_mode *mode)
>> @@ -631,7 +651,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;
>>  
>--
>Cheers,
>	Lyude Paul (she/her)
>	Associate Software Engineer at Red Hat
>

Best regards,
Shawn
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx