| Message ID | 20200106174156.11081-1-shawn.c.lee@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dp/mst : Get clock rate from sink's available PBN | expand |
Hi Lee,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v5.5-rc5 next-20200106]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Lee-Shawn-C/drm-i915-dp-mst-Get-clock-rate-from-sink-s-available-PBN/20200106-190912
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-defconfig (attached as .config)
compiler: gcc-7 (Debian 7.5.0-3) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
ld: drivers/gpu/drm/i915/display/intel_dp_mst.o: in function `intel_dp_mst_mode_valid':
>> intel_dp_mst.c:(.text+0x385): undefined reference to `__udivdi3'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
On Tue, Jan 07, 2020 at 01:41:56AM +0800, Lee Shawn C wrote: > Driver report physcial bandwidth for max dot clock rate. > It would caused compatibility issue sometimes when physical > bandwidth exceed MST hub output ability. > > For example, here is a MST hub with HDMI 1.4 and DP 1.2 output. > And source have DP 1.2 output capability. Connect a HDMI 2.0 > display then source will retrieve EDID from external monitor. > Driver got max resolution was 4k@60fps. DP 1.2 can support > this resolution because it did not surpass max physical bandwidth. > After modeset, source output display data but MST hub can't > output HDMI properly due to it already over HDMI 1.4 spec. > > Apply this calculation, source calcualte max dot clock according > to available PBN. Driver will remove the mode that over current > clock rate. And external display can works normally. > > 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> > > Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp_mst.c | 27 ++++++++++++++++++--- > 1 file changed, 24 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index 3b066c63816d..eaa440165ad2 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -550,6 +550,27 @@ static int intel_dp_mst_get_modes(struct drm_connector *connector) > return intel_dp_mst_get_ddc_modes(connector); > } > > +static int > +intel_dp_mst_downstream_max_dotclock(struct drm_connector *connector) > +{ > + struct intel_connector *intel_connector = to_intel_connector(connector); > + struct intel_dp *intel_dp = intel_connector->mst_port; > + struct drm_dp_mst_port *port; > + u64 clock_rate = 0; > + > + if (intel_dp->mst_mgr.mst_primary) > + list_for_each_entry(port, &intel_dp->mst_mgr.mst_primary->ports, next) > + if (port->connector == connector) { > + clock_rate = ((u64)port->available_pbn * (54 * 8 * 1000 * 1000)) / (64 * 1006); IIRC avaible pbn is soime kind of dynamic "how much bw we have left currently" so we don't want to use it for this purpose. If we really wanted to do this we'd have to refilter the modelist and generate hotplugs whenever the bw allocations change. In the current design what should happens is that we check that we have enough bw when doing the modeset, and if that fails userspace is supposed to handle the situation in some graceful manner. Also locking totally missing. So nak. > + > + // FIXME: We should used pipe bpp to do this calculation. > + // But can't retrieve bpp setting from drm_connector. > + return (int)(clock_rate / 24); > + } > + > + return to_i915(connector->dev)->max_dotclk_freq; > +} > + > static enum drm_mode_status > intel_dp_mst_mode_valid(struct drm_connector *connector, > struct drm_display_mode *mode) > @@ -557,8 +578,7 @@ intel_dp_mst_mode_valid(struct drm_connector *connector, > struct drm_i915_private *dev_priv = to_i915(connector->dev); > struct intel_connector *intel_connector = to_intel_connector(connector); > struct intel_dp *intel_dp = intel_connector->mst_port; > - int max_dotclk = to_i915(connector->dev)->max_dotclk_freq; > - int max_rate, mode_rate, max_lanes, max_link_clock; > + int max_rate, mode_rate, max_lanes, max_link_clock, max_dotclk; > > if (drm_connector_is_unregistered(connector)) > return MODE_ERROR; > @@ -572,7 +592,8 @@ 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 */ > + max_dotclk = intel_dp_mst_downstream_max_dotclock(connector); > + > if (mode->clock < 10000) > return MODE_CLOCK_LOW; > > -- > 2.17.1
On Jan. 8, 2020, 3:15 p.m, Ville Syrjala wrote: >On Tue, Jan 07, 2020 at 01:41:56AM +0800, Lee Shawn C wrote: >> Driver report physcial bandwidth for max dot clock rate. >> It would caused compatibility issue sometimes when physical >> bandwidth exceed MST hub output ability. >> >> For example, here is a MST hub with HDMI 1.4 and DP 1.2 output. >> And source have DP 1.2 output capability. Connect a HDMI 2.0 >> display then source will retrieve EDID from external monitor. >> Driver got max resolution was 4k@60fps. DP 1.2 can support >> this resolution because it did not surpass max physical bandwidth. >> After modeset, source output display data but MST hub can't >> output HDMI properly due to it already over HDMI 1.4 spec. >> >> Apply this calculation, source calcualte max dot clock according >> to available PBN. Driver will remove the mode that over current >> clock rate. And external display can works normally. >> >> 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> >> >> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_dp_mst.c | 27 ++++++++++++++++++--- >> 1 file changed, 24 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c >> index 3b066c63816d..eaa440165ad2 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c >> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c >> @@ -550,6 +550,27 @@ static int intel_dp_mst_get_modes(struct drm_connector *connector) >> return intel_dp_mst_get_ddc_modes(connector); >> } >> >> +static int >> +intel_dp_mst_downstream_max_dotclock(struct drm_connector *connector) >> +{ >> + struct intel_connector *intel_connector = to_intel_connector(connector); >> + struct intel_dp *intel_dp = intel_connector->mst_port; >> + struct drm_dp_mst_port *port; >> + u64 clock_rate = 0; >> + >> + if (intel_dp->mst_mgr.mst_primary) >> + list_for_each_entry(port, &intel_dp->mst_mgr.mst_primary->ports, next) >> + if (port->connector == connector) { >> + clock_rate = ((u64)port->available_pbn * (54 * 8 * 1000 * 1000)) / (64 * 1006); > >IIRC avaible pbn is soime kind of dynamic "how much bw we have left >currently" so we don't want to use it for this purpose. If we really >wanted to do this we'd have to refilter the modelist and generate >hotplugs whenever the bw allocations change. > >In the current design what should happens is that we check that we >have enough bw when doing the modeset, and if that fails userspace >is supposed to handle the situation in some graceful manner. > >Also locking totally missing. > >So nak. > Thanks for comments! In my opinion, branch device (MST hub) would tell source driver the available PBN for each extended port it owns. And source driver will renew it everytime after HPD coming. That's why we should refer the PBN report by MST branch to calculate dot clock rate and make sure sink can support the resolution while creating mode list. >> + >> + // FIXME: We should used pipe bpp to do this calculation. >> + // But can't retrieve bpp setting from drm_connector. >> + return (int)(clock_rate / 24); >> + } >> + >> + return to_i915(connector->dev)->max_dotclk_freq; >> +} >> + Here try to get the max dot clock according to PBN value from sink. If driver can't find it, will return the orginal max_dotclk_freq. >> static enum drm_mode_status >> intel_dp_mst_mode_valid(struct drm_connector *connector, >> struct drm_display_mode *mode) >> @@ -557,8 +578,7 @@ intel_dp_mst_mode_valid(struct drm_connector *connector, >> struct drm_i915_private *dev_priv = to_i915(connector->dev); >> struct intel_connector *intel_connector = to_intel_connector(connector); >> struct intel_dp *intel_dp = intel_connector->mst_port; >> - int max_dotclk = to_i915(connector->dev)->max_dotclk_freq; >> - int max_rate, mode_rate, max_lanes, max_link_clock; >> + int max_rate, mode_rate, max_lanes, max_link_clock, max_dotclk; >> >> if (drm_connector_is_unregistered(connector)) >> return MODE_ERROR; >> @@ -572,7 +592,8 @@ 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 */ >> + max_dotclk = intel_dp_mst_downstream_max_dotclock(connector); >> + Then driver would the clock rate exceed max_dotclk or not to create mode list. if (mode_rate > max_rate || mode->clock > max_dotclk) return MODE_CLOCK_HIGH; When connect to MST hub with HDMI 1.4 output. If user connect the monitor with HDMI 2.0 capability. EDID from sink shows it can meet DP 1.2 requirement and source set the prefer resolution (4k@60fps) to display. But MST hub can't output the video data via its HDMI port due to this resolution already exceed its ability. This change may increase source driver compatibility at MST mode. Please give us advice if more concern. >> if (mode->clock < 10000) >> return MODE_CLOCK_LOW; >> >> -- >> 2.17.1
On Tue, 2020-01-07 at 01:41 +0800, Lee Shawn C wrote: > Driver report physcial bandwidth for max dot clock rate. > It would caused compatibility issue sometimes when physical > bandwidth exceed MST hub output ability. > > For example, here is a MST hub with HDMI 1.4 and DP 1.2 output. > And source have DP 1.2 output capability. Connect a HDMI 2.0 > display then source will retrieve EDID from external monitor. > Driver got max resolution was 4k@60fps. DP 1.2 can support > this resolution because it did not surpass max physical bandwidth. > After modeset, source output display data but MST hub can't > output HDMI properly due to it already over HDMI 1.4 spec. > > Apply this calculation, source calcualte max dot clock according > to available PBN. Driver will remove the mode that over current > clock rate. And external display can works normally. > > 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> > > Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp_mst.c | 27 ++++++++++++++++++--- > 1 file changed, 24 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index 3b066c63816d..eaa440165ad2 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -550,6 +550,27 @@ static int intel_dp_mst_get_modes(struct drm_connector > *connector) > return intel_dp_mst_get_ddc_modes(connector); > } > > +static int > +intel_dp_mst_downstream_max_dotclock(struct drm_connector *connector) > +{ > + struct intel_connector *intel_connector = > to_intel_connector(connector); > + struct intel_dp *intel_dp = intel_connector->mst_port; > + struct drm_dp_mst_port *port; > + u64 clock_rate = 0; > + > + if (intel_dp->mst_mgr.mst_primary) > + list_for_each_entry(port, &intel_dp->mst_mgr.mst_primary- > >ports, next) > + if (port->connector == connector) { > + clock_rate = ((u64)port->available_pbn * (54 * > 8 * 1000 * 1000)) / (64 * 1006); > + > + // FIXME: We should used pipe bpp to do this > calculation. > + // But can't retrieve bpp setting from > drm_connector. > + return (int)(clock_rate / 24); > + } > + > + return to_i915(connector->dev)->max_dotclk_freq; > +} Hi! So-there's no need to loop through the ports like this, just use the drm_dp_mst_port struct that's associated with intel_connector->port directly (feel free to change the declaration to `struct drm_dp_mst_port *port` instead of `void *port`, it's not illegal to dereference it anymore I promise! Additionally - you don't want to use pipe_bpp here at all. My advice is to use the hard-coded bpc we currently have for MST. Once you guys have retry logic to dynamically select the bpc depending on the available bandwidth, I'd move this check over to using the smallest possible BPC reported by the connector's display_info. Remember we're checking if -any- variant of each mode is somehow possible, it's ok and expected for modes to potentially fail at higher BPCs. Anyway-this looks fine otherwise, but like Ville mentioned available_pbn isn't the thing that we want to be using here. I've got support for using full_pbn instead and that should be pushed sometime today or tommorrow (dealing with some topic branch weirdness with dim right now). This is the patch series, jfyi: https://patchwork.freedesktop.org/series/74295/ Also-feel free to write a drm helper to do these mode_valid checks for mst, if it's feasible and not overkill > + > static enum drm_mode_status > intel_dp_mst_mode_valid(struct drm_connector *connector, > struct drm_display_mode *mode) > @@ -557,8 +578,7 @@ intel_dp_mst_mode_valid(struct drm_connector *connector, > struct drm_i915_private *dev_priv = to_i915(connector->dev); > struct intel_connector *intel_connector = > to_intel_connector(connector); > struct intel_dp *intel_dp = intel_connector->mst_port; > - int max_dotclk = to_i915(connector->dev)->max_dotclk_freq; > - int max_rate, mode_rate, max_lanes, max_link_clock; > + int max_rate, mode_rate, max_lanes, max_link_clock, max_dotclk; > > if (drm_connector_is_unregistered(connector)) > return MODE_ERROR; > @@ -572,7 +592,8 @@ 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 */ > + max_dotclk = intel_dp_mst_downstream_max_dotclock(connector); > + > if (mode->clock < 10000) > return MODE_CLOCK_LOW; >
On Wed, 2020-03-11, Lyude Paul wrote: >On Tue, 2020-01-07 at 01:41 +0800, Lee Shawn C wrote: >> Driver report physcial bandwidth for max dot clock rate. >> It would caused compatibility issue sometimes when physical bandwidth >> exceed MST hub output ability. >> >> For example, here is a MST hub with HDMI 1.4 and DP 1.2 output. >> And source have DP 1.2 output capability. Connect a HDMI 2.0 display >> then source will retrieve EDID from external monitor. >> Driver got max resolution was 4k@60fps. DP 1.2 can support this >> resolution because it did not surpass max physical bandwidth. >> After modeset, source output display data but MST hub can't output >> HDMI properly due to it already over HDMI 1.4 spec. >> >> Apply this calculation, source calcualte max dot clock according to >> available PBN. Driver will remove the mode that over current clock >> rate. And external display can works normally. >> >> 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> >> >> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_dp_mst.c | 27 >> ++++++++++++++++++--- >> 1 file changed, 24 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c >> b/drivers/gpu/drm/i915/display/intel_dp_mst.c >> index 3b066c63816d..eaa440165ad2 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c >> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c >> @@ -550,6 +550,27 @@ static int intel_dp_mst_get_modes(struct >> drm_connector >> *connector) >> return intel_dp_mst_get_ddc_modes(connector); >> } >> >> +static int >> +intel_dp_mst_downstream_max_dotclock(struct drm_connector *connector) >> +{ >> + struct intel_connector *intel_connector = >> to_intel_connector(connector); >> + struct intel_dp *intel_dp = intel_connector->mst_port; >> + struct drm_dp_mst_port *port; >> + u64 clock_rate = 0; >> + >> + if (intel_dp->mst_mgr.mst_primary) >> + list_for_each_entry(port, &intel_dp->mst_mgr.mst_primary- >> >ports, next) >> + if (port->connector == connector) { >> + clock_rate = ((u64)port->available_pbn * (54 * >> 8 * 1000 * 1000)) / (64 * 1006); >> + >> + // FIXME: We should used pipe bpp to do this >> calculation. >> + // But can't retrieve bpp setting from >> drm_connector. >> + return (int)(clock_rate / 24); >> + } >> + >> + return to_i915(connector->dev)->max_dotclk_freq; >> +} > >Hi! So-there's no need to loop through the ports like this, just use the drm_dp_mst_port struct that's associated with intel_connector->port directly (feel free to change the declaration to `struct drm_dp_mst_port *port` instead of `void *port`, it's not illegal to dereference it anymore I promise! > >Additionally - you don't want to use pipe_bpp here at all. My advice is to use the hard-coded bpc we currently have for MST. Once you guys have retry logic to dynamically select the bpc depending on the available bandwidth, I'd move this check over to using the smallest possible BPC reported by the connector's display_info. Remember we're checking if -any- variant of each mode is somehow possible, it's ok and expected for modes to potentially fail at higher BPCs. > >Anyway-this looks fine otherwise, but like Ville mentioned available_pbn isn't the thing that we want to be using here. I've got support for using full_pbn instead and that should be pushed sometime today or tommorrow (dealing with some topic branch weirdness with dim right now). This is the patch series, >jfyi: > >https://patchwork.freedesktop.org/series/74295/ > >Also-feel free to write a drm helper to do these mode_valid checks for mst, if it's feasible and not overkill > Thanks! I will refer the change from patch series you mentioned. Hardcode bpc to 24 and use full_pbn instead of available_pbn. BTW, this patch series still on specific branch (topic/mst-bw-check-fixes-for-airlied) and not merge to drm branch yet. It would be better to wait the patches merged into drm branch. After that, I can commit new patch to fix issue. Any comment? >> + >> static enum drm_mode_status >> intel_dp_mst_mode_valid(struct drm_connector *connector, >> struct drm_display_mode *mode) >> @@ -557,8 +578,7 @@ intel_dp_mst_mode_valid(struct drm_connector *connector, >> struct drm_i915_private *dev_priv = to_i915(connector->dev); >> struct intel_connector *intel_connector = >> to_intel_connector(connector); >> struct intel_dp *intel_dp = intel_connector->mst_port; >> - int max_dotclk = to_i915(connector->dev)->max_dotclk_freq; >> - int max_rate, mode_rate, max_lanes, max_link_clock; >> + int max_rate, mode_rate, max_lanes, max_link_clock, max_dotclk; >> >> if (drm_connector_is_unregistered(connector)) >> return MODE_ERROR; >> @@ -572,7 +592,8 @@ 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 */ >> + max_dotclk = intel_dp_mst_downstream_max_dotclock(connector); >> + >> if (mode->clock < 10000) >> return MODE_CLOCK_LOW; >> >-- >Cheers, > Lyude Paul (she/her) > Associate Software Engineer at Red Hat > Best regards, Shawn
On Mon, 2020-03-16 at 13:56 +0000, Lee, Shawn C wrote: > On Wed, 2020-03-11, Lyude Paul wrote: > > On Tue, 2020-01-07 at 01:41 +0800, Lee Shawn C wrote: > > > Driver report physcial bandwidth for max dot clock rate. > > > It would caused compatibility issue sometimes when physical bandwidth > > > exceed MST hub output ability. > > > > > > For example, here is a MST hub with HDMI 1.4 and DP 1.2 output. > > > And source have DP 1.2 output capability. Connect a HDMI 2.0 display > > > then source will retrieve EDID from external monitor. > > > Driver got max resolution was 4k@60fps. DP 1.2 can support this > > > resolution because it did not surpass max physical bandwidth. > > > After modeset, source output display data but MST hub can't output > > > HDMI properly due to it already over HDMI 1.4 spec. > > > > > > Apply this calculation, source calcualte max dot clock according to > > > available PBN. Driver will remove the mode that over current clock > > > rate. And external display can works normally. > > > > > > 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> > > > > > > Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 27 > > > ++++++++++++++++++--- > > > 1 file changed, 24 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > index 3b066c63816d..eaa440165ad2 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > @@ -550,6 +550,27 @@ static int intel_dp_mst_get_modes(struct > > > drm_connector > > > *connector) > > > return intel_dp_mst_get_ddc_modes(connector); > > > } > > > > > > +static int > > > +intel_dp_mst_downstream_max_dotclock(struct drm_connector *connector) > > > +{ > > > + struct intel_connector *intel_connector = > > > to_intel_connector(connector); > > > + struct intel_dp *intel_dp = intel_connector->mst_port; > > > + struct drm_dp_mst_port *port; > > > + u64 clock_rate = 0; > > > + > > > + if (intel_dp->mst_mgr.mst_primary) > > > + list_for_each_entry(port, &intel_dp->mst_mgr.mst_primary- > > > > ports, next) > > > + if (port->connector == connector) { > > > + clock_rate = ((u64)port->available_pbn * (54 * > > > 8 * 1000 * 1000)) / (64 * 1006); > > > + > > > + // FIXME: We should used pipe bpp to do this > > > calculation. > > > + // But can't retrieve bpp setting from > > > drm_connector. > > > + return (int)(clock_rate / 24); > > > + } > > > + > > > + return to_i915(connector->dev)->max_dotclk_freq; > > > +} > > > > Hi! So-there's no need to loop through the ports like this, just use the > > drm_dp_mst_port struct that's associated with intel_connector->port > > directly (feel free to change the declaration to `struct drm_dp_mst_port > > *port` instead of `void *port`, it's not illegal to dereference it anymore > > I promise! > > > > Additionally - you don't want to use pipe_bpp here at all. My advice is to > > use the hard-coded bpc we currently have for MST. Once you guys have retry > > logic to dynamically select the bpc depending on the available bandwidth, > > I'd move this check over to using the smallest possible BPC reported by > > the connector's display_info. Remember we're checking if -any- variant of > > each mode is somehow possible, it's ok and expected for modes to > > potentially fail at higher BPCs. > > > > Anyway-this looks fine otherwise, but like Ville mentioned available_pbn > > isn't the thing that we want to be using here. I've got support for using > > full_pbn instead and that should be pushed sometime today or tommorrow > > (dealing with some topic branch weirdness with dim right now). This is the > > patch series, > > jfyi: > > > > https://patchwork.freedesktop.org/series/74295/ > > > > Also-feel free to write a drm helper to do these mode_valid checks for > > mst, if it's feasible and not overkill > > > > Thanks! I will refer the change from patch series you mentioned. Hardcode > bpc to 24 and use full_pbn instead of available_pbn. > > BTW, this patch series still on specific branch (topic/mst-bw-check-fixes- > for-airlied) and not merge to drm branch yet. > It would be better to wait the patches merged into drm branch. After that, I > can commit new patch to fix issue. Any comment? jfyi the patch should be upstream now. So feel free to send a new patch (also make sure to cc me so I can review it!) > > > > + > > > static enum drm_mode_status > > > intel_dp_mst_mode_valid(struct drm_connector *connector, > > > struct drm_display_mode *mode) > > > @@ -557,8 +578,7 @@ intel_dp_mst_mode_valid(struct drm_connector > > > *connector, > > > struct drm_i915_private *dev_priv = to_i915(connector->dev); > > > struct intel_connector *intel_connector = > > > to_intel_connector(connector); > > > struct intel_dp *intel_dp = intel_connector->mst_port; > > > - int max_dotclk = to_i915(connector->dev)->max_dotclk_freq; > > > - int max_rate, mode_rate, max_lanes, max_link_clock; > > > + int max_rate, mode_rate, max_lanes, max_link_clock, max_dotclk; > > > > > > if (drm_connector_is_unregistered(connector)) > > > return MODE_ERROR; > > > @@ -572,7 +592,8 @@ 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 */ > > > + max_dotclk = intel_dp_mst_downstream_max_dotclock(connector); > > > + > > > if (mode->clock < 10000) > > > return MODE_CLOCK_LOW; > > > > > -- > > Cheers, > > Lyude Paul (she/her) > > Associate Software Engineer at Red Hat > > > > 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 3b066c63816d..eaa440165ad2 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -550,6 +550,27 @@ static int intel_dp_mst_get_modes(struct drm_connector *connector) return intel_dp_mst_get_ddc_modes(connector); } +static int +intel_dp_mst_downstream_max_dotclock(struct drm_connector *connector) +{ + struct intel_connector *intel_connector = to_intel_connector(connector); + struct intel_dp *intel_dp = intel_connector->mst_port; + struct drm_dp_mst_port *port; + u64 clock_rate = 0; + + if (intel_dp->mst_mgr.mst_primary) + list_for_each_entry(port, &intel_dp->mst_mgr.mst_primary->ports, next) + if (port->connector == connector) { + clock_rate = ((u64)port->available_pbn * (54 * 8 * 1000 * 1000)) / (64 * 1006); + + // FIXME: We should used pipe bpp to do this calculation. + // But can't retrieve bpp setting from drm_connector. + return (int)(clock_rate / 24); + } + + return to_i915(connector->dev)->max_dotclk_freq; +} + static enum drm_mode_status intel_dp_mst_mode_valid(struct drm_connector *connector, struct drm_display_mode *mode) @@ -557,8 +578,7 @@ intel_dp_mst_mode_valid(struct drm_connector *connector, struct drm_i915_private *dev_priv = to_i915(connector->dev); struct intel_connector *intel_connector = to_intel_connector(connector); struct intel_dp *intel_dp = intel_connector->mst_port; - int max_dotclk = to_i915(connector->dev)->max_dotclk_freq; - int max_rate, mode_rate, max_lanes, max_link_clock; + int max_rate, mode_rate, max_lanes, max_link_clock, max_dotclk; if (drm_connector_is_unregistered(connector)) return MODE_ERROR; @@ -572,7 +592,8 @@ 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 */ + max_dotclk = intel_dp_mst_downstream_max_dotclock(connector); + if (mode->clock < 10000) return MODE_CLOCK_LOW;
Driver report physcial bandwidth for max dot clock rate. It would caused compatibility issue sometimes when physical bandwidth exceed MST hub output ability. For example, here is a MST hub with HDMI 1.4 and DP 1.2 output. And source have DP 1.2 output capability. Connect a HDMI 2.0 display then source will retrieve EDID from external monitor. Driver got max resolution was 4k@60fps. DP 1.2 can support this resolution because it did not surpass max physical bandwidth. After modeset, source output display data but MST hub can't output HDMI properly due to it already over HDMI 1.4 spec. Apply this calculation, source calcualte max dot clock according to available PBN. Driver will remove the mode that over current clock rate. And external display can works normally. 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> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com> --- drivers/gpu/drm/i915/display/intel_dp_mst.c | 27 ++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-)