| Message ID | 20200417212408.19649-1-shawn.c.lee@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/mst: filter out the display mode exceed sink's capability | expand |
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. > + 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). > + 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 > + 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; >
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
On Thu, 2020-04-30 at 02:37 +0000, Lee, Shawn C wrote: > 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. Sorry this took me a little while to respond to, crunch time came up at work… Anyway-have you seen issues with full_pbn reporting on hubs? I've seen plenty of problems with available_pbn reporting, but the reason we switched over to full_pbn in the first place is because that seemed to be the one hubs reported reliably. We actually make the assumption full_pbn is always > 0 if something's connected in some places in the MST helpers, so if we've got cases of full_pbn lying as well on some hubs we might want to fix that. > > > > + 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 >
On Thu, 2020-05-07, Lyude Paul wrote: >On Thu, 2020-04-30 at 02:37 +0000, Lee, Shawn C wrote: >> 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. > >Sorry this took me a little while to respond to, crunch time came up at work... >Anyway-have you seen issues with full_pbn reporting on hubs? I've seen plenty >of problems with available_pbn reporting, but the reason we switched over to >full_pbn in the first place is because that seemed to be the one hubs reported >reliably. We actually make the assumption full_pbn is always > 0 if >something's connected in some places in the MST helpers, so if we've got cases >of full_pbn lying as well on some hubs we might want to fix that. > No, we don't see MST hub report full_pbn = 0 so far. Just like what you said. This is used to guarantee driver always have valid full_pbn for bandwidth calcuation. If not, driver just bypass the PBN comparison. >> >> > > + 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
On Thu, May 07, 2020 at 06:46:17PM -0400, Lyude Paul wrote: > On Thu, 2020-04-30 at 02:37 +0000, Lee, Shawn C wrote: > > 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. > > Sorry this took me a little while to respond to, crunch time came up at work… > Anyway-have you seen issues with full_pbn reporting on hubs? I've seen plenty > of problems with available_pbn reporting, but the reason we switched over to > full_pbn in the first place is because that seemed to be the one hubs reported > reliably. We actually make the assumption full_pbn is always > 0 if > something's connected in some places in the MST helpers, so if we've got cases > of full_pbn lying as well on some hubs we might want to fix that. We have at least one full_pbn==0 issue in ci: <4>[ 5.061345] WARNING: CPU: 0 PID: 376 at drivers/gpu/drm/drm_dp_mst_topology.c:4889 drm_dp_mst_atomic_check_mstb_bw_limit+0x193/0x1c0 https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8460/fi-kbl-7560u/boot0.txt Dunno if that's the MST device reporting zero or due to some other bug, but at least CI is currently borked on that machine.
On Thu, May 11, 2020, Ville Syrjälä wrote: >On Thu, May 07, 2020 at 06:46:17PM -0400, Lyude Paul wrote: >> On Thu, 2020-04-30 at 02:37 +0000, Lee, Shawn C wrote: >> > 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. >> >> Sorry this took me a little while to respond to, crunch time came up at work. >> Anyway-have you seen issues with full_pbn reporting on hubs? I've seen plenty >> of problems with available_pbn reporting, but the reason we switched over to >> full_pbn in the first place is because that seemed to be the one hubs reported >> reliably. We actually make the assumption full_pbn is always > 0 if >> something's connected in some places in the MST helpers, so if we've got cases >> of full_pbn lying as well on some hubs we might want to fix that. > >We have at least one full_pbn==0 issue in ci: ><4>[ 5.061345] WARNING: CPU: 0 PID: 376 at drivers/gpu/drm/drm_dp_mst_topology.c:4889 drm_dp_mst_atomic_check_mstb_bw_limit+0x193/0x1c0 > >https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8460/fi-kbl-7560u/boot0.txt > >Dunno if that's the MST device reporting zero or due to some other >bug, but at least CI is currently borked on that machine. Thanks for sharing! It will be good to keep checking full_pbn to avoid unexpected issue. BTW, I just upload patch v2. Could you please share your comments about that? Best regards, Shawn
On Tue, 2020-05-12 at 06:17 +0000, Lee, Shawn C wrote: > On Thu, May 11, 2020, Ville Syrjälä wrote: > > On Thu, May 07, 2020 at 06:46:17PM -0400, Lyude Paul wrote: > > > On Thu, 2020-04-30 at 02:37 +0000, Lee, Shawn C wrote: > > > > 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. > > > > > > Sorry this took me a little while to respond to, crunch time came up at > > > work. > > > Anyway-have you seen issues with full_pbn reporting on hubs? I've seen > > > plenty > > > of problems with available_pbn reporting, but the reason we switched > > > over to > > > full_pbn in the first place is because that seemed to be the one hubs > > > reported > > > reliably. We actually make the assumption full_pbn is always > 0 if > > > something's connected in some places in the MST helpers, so if we've got > > > cases > > > of full_pbn lying as well on some hubs we might want to fix that. > > > > We have at least one full_pbn==0 issue in ci: > > <4>[ 5.061345] WARNING: CPU: 0 PID: 376 at > > drivers/gpu/drm/drm_dp_mst_topology.c:4889 > > drm_dp_mst_atomic_check_mstb_bw_limit+0x193/0x1c0 > > > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8460/fi-kbl-7560u/boot0.txt > > Oh yikes-yeah, it's definitely reporting 0 PBN, that's a new one. Anyway-I'll take a look at this patch sometime today. I'd like to look more into that hub though, I'm almost certain that full_pbn = 0 is against the MST spec, but the hub does seem to be reporting that: <7>[ 5.002279] [drm:drm_dp_send_enum_path_resources] enum path resources 3: 0 0 Since hubs will set full_pbn based on the current downstream link training settings for the mst port though, I wonder if maybe this hub is just failing to do link training downstream or failing to detect a display downstream? Probably would be useful to know what kind of a hub this is, and what kind of displays are hooked up to it. Also, having drm DP enabled in those logs would be quite helpful as well > > Dunno if that's the MST device reporting zero or due to some other > > bug, but at least CI is currently borked on that machine. > > Thanks for sharing! It will be good to keep checking full_pbn to avoid > unexpected issue. > BTW, I just upload patch v2. Could you please share your comments about > that? > > Best regards, > Shawn >
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); + if (port->full_pbn) + ret = (drm_dp_calc_pbn_mode(clock, bpp, false) > port->full_pbn); + 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;
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(-)