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.nav...@intel.com>
>> Cc: Jani Nikula <jani.nik...@linux.intel.com>
>> Cc: Ville Syrjala <ville.syrj...@linux.intel.com>
>> Cc: Cooper Chiou <cooper.ch...@intel.com>
>> Cc: Lyude Paul <ly...@redhat.com>
>> Signed-off-by: Lee Shawn C <shawn.c....@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
