From: Manasi Navare <manasi.d.navare@intel.com> To: Lyude Paul <lyude@redhat.com> Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, "Ville Syrjälä" <ville.syrjala@linux.intel.com>, "Gustavo Padovan" <gustavo@padovan.org>, "Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>, "Sean Paul" <seanpaul@chromium.org>, "David Airlie" <airlied@linux.ie>, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 3/5] drm/dp_mst: Add drm_dp_mst_topology_mgr_lower_link_rate() Date: Mon, 12 Mar 2018 14:28:14 -0700 [thread overview] Message-ID: <20180312212814.GB3022@intel.com> (raw) In-Reply-To: <20180309213232.19855-3-lyude@redhat.com> On Fri, Mar 09, 2018 at 04:32:29PM -0500, Lyude Paul wrote: > Unlike SST, MST can have far more then a single display hooked up on a > single port. What this also means however, is that if the DisplayPort > link to the top-level MST branch device becomes unstable then every > single branch device also has an unstable link. Additionally, MST has a > few more steps that must be taken in order to properly retrain the > entire topology under a lower link rate. Since the VCID allocations for > each mstb is determined based off the link rate for the top of the > topology, we also have to recalculate all of the VCID allocations for > the downstream ports as well to ensure that we still have enough link > bandwidth to drive each mstb. > > Additionally, since we have multiple downstream connectors per port, > setting the link status of the parent mstb's port to bad isn't enough: > all of the downstream mstb ports have to have their link status set to > bad. > > This basically follows the same paradigm that our DP link status logic > in DRM does, in that we simply tell userspace that all of the mstb ports > need retraining and additionally applying the new lower bandwidth > constraints to all of the atomic commits on the topology that follow. > > Since the work of figuring out which connectors live downstream on an > MST topology and updating their link status is something that any driver > supporting MST is going to need to do in order to retrain MST links > properly, we add the drm_dp_mst_topology_mgr_lower_link_rate() helper > which simply recalculates the pbn_div for a given mst topology, then > marks the link status of all connectors living in that topology as bad. > We'll make use of it in i915 later in this series. > > Signed-off-by: Lyude Paul <lyude@redhat.com> > Cc: Manasi Navare <manasi.d.navare@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 73 ++++++++++++++++++++++++++++++++++- > include/drm/drm_dp_mst_helper.h | 3 ++ > 2 files changed, 75 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index 6fac4129e6a2..0d6604500b29 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -2076,7 +2076,7 @@ static bool drm_dp_get_vc_payload_bw(int dp_link_bw, > { > switch (dp_link_bw) { > default: > - DRM_DEBUG_KMS("invalid link bandwidth in DPCD: %x (link count: %d)\n", > + DRM_DEBUG_KMS("invalid link bandwidth: %x (link count: %d)\n", > dp_link_bw, dp_link_count); > return false; > > @@ -2096,6 +2096,77 @@ static bool drm_dp_get_vc_payload_bw(int dp_link_bw, > return true; > } > > +static void drm_dp_set_mstb_link_status(struct drm_dp_mst_branch *mstb, > + enum drm_link_status status) > +{ > + struct drm_dp_mst_branch *rmstb; > + struct drm_dp_mst_port *port; > + > + list_for_each_entry(port, &mstb->ports, next) { > + rmstb = drm_dp_get_validated_mstb_ref(mstb->mgr, port->mstb); > + if (rmstb) { > + drm_dp_set_mstb_link_status(rmstb, status); > + drm_dp_put_mst_branch_device(rmstb); > + } > + > + if (port->connector) > + port->connector->state->link_status = status; > + } > +} > + > +/** > + * drm_dp_mst_topology_mgr_lower_link_rate() - Override the DP link bw/count > + * for all connectors in a given MST topology > + * @mgr: manager to set state for > + * @dp_link_bw: The new DP link bandwidth I would rather call this argument as dp_link_rate since thats what we are passing and not dp_link_bw. > + * @dp_link_count: The new DP link count > + * > + * This is called by the driver when it detects that the current DP link for > + * the given topology manager is unstable, and needs to be retrained at a > + * lower link rate. > + * > + * This takes care of updating the link status on all downstream connectors > + * along with recalculating the VC payloads. The driver should send a hotplug > + * event after calling this function to notify userspace of the link status > + * change. > + * > + * RETURNS: > + * > + * True for success, or negative error code on failure. > + */ > +int drm_dp_mst_topology_mgr_lower_link_rate(struct drm_dp_mst_topology_mgr *mgr, > + int dp_link_bw, int dp_link_count) > +{ > + struct drm_device *dev = mgr->dev; > + struct drm_dp_mst_branch *mst_primary; > + int new_pbn_div; > + int ret = 0; > + > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > + > + if (!drm_dp_get_vc_payload_bw(drm_dp_link_rate_to_bw_code(dp_link_bw), This explains my comment above, we convert link rate to link_bw here so better call that link_rate to begin with > + dp_link_count, &new_pbn_div)) { > + ret = -EINVAL; > + goto out; > + } > + > + mst_primary = drm_dp_get_validated_mstb_ref(mgr, mgr->mst_primary); > + if (!mst_primary) > + goto out; > + > + DRM_DEBUG_KMS("MST link failed to retrain, lowering pbn_div to %d\n", > + new_pbn_div); This debug message seems a little misleading since we are saying that it failed to train link or retrain at the same params and now we are retraining with fallback values. Clarify this a bit more in this message like "MST link training failed or retrain at same params failed"..? Manasi > + mgr->pbn_div = new_pbn_div; > + > + drm_dp_set_mstb_link_status(mst_primary, DRM_MODE_LINK_STATUS_BAD); > + > + drm_dp_put_mst_branch_device(mst_primary); > +out: > + drm_modeset_unlock(&dev->mode_config.connection_mutex); > + return ret; > +} > +EXPORT_SYMBOL(drm_dp_mst_topology_mgr_lower_link_rate); > + > /** > * drm_dp_mst_topology_mgr_set_mst() - Set the MST state for a topology manager > * @mgr: manager to set state for > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h > index 7f78d26a0766..6261ec43a2c0 100644 > --- a/include/drm/drm_dp_mst_helper.h > +++ b/include/drm/drm_dp_mst_helper.h > @@ -634,4 +634,7 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state, > int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr *mgr, > struct drm_dp_mst_port *port, bool power_up); > > +int drm_dp_mst_topology_mgr_lower_link_rate(struct drm_dp_mst_topology_mgr *mgr, > + int dp_link_bw, int dp_link_count); > + > #endif > -- > 2.14.3 >
WARNING: multiple messages have this Message-ID (diff)
From: Manasi Navare <manasi.d.navare@intel.com> To: Lyude Paul <lyude@redhat.com> Cc: David Airlie <airlied@linux.ie>, intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH v3 3/5] drm/dp_mst: Add drm_dp_mst_topology_mgr_lower_link_rate() Date: Mon, 12 Mar 2018 14:28:14 -0700 [thread overview] Message-ID: <20180312212814.GB3022@intel.com> (raw) In-Reply-To: <20180309213232.19855-3-lyude@redhat.com> On Fri, Mar 09, 2018 at 04:32:29PM -0500, Lyude Paul wrote: > Unlike SST, MST can have far more then a single display hooked up on a > single port. What this also means however, is that if the DisplayPort > link to the top-level MST branch device becomes unstable then every > single branch device also has an unstable link. Additionally, MST has a > few more steps that must be taken in order to properly retrain the > entire topology under a lower link rate. Since the VCID allocations for > each mstb is determined based off the link rate for the top of the > topology, we also have to recalculate all of the VCID allocations for > the downstream ports as well to ensure that we still have enough link > bandwidth to drive each mstb. > > Additionally, since we have multiple downstream connectors per port, > setting the link status of the parent mstb's port to bad isn't enough: > all of the downstream mstb ports have to have their link status set to > bad. > > This basically follows the same paradigm that our DP link status logic > in DRM does, in that we simply tell userspace that all of the mstb ports > need retraining and additionally applying the new lower bandwidth > constraints to all of the atomic commits on the topology that follow. > > Since the work of figuring out which connectors live downstream on an > MST topology and updating their link status is something that any driver > supporting MST is going to need to do in order to retrain MST links > properly, we add the drm_dp_mst_topology_mgr_lower_link_rate() helper > which simply recalculates the pbn_div for a given mst topology, then > marks the link status of all connectors living in that topology as bad. > We'll make use of it in i915 later in this series. > > Signed-off-by: Lyude Paul <lyude@redhat.com> > Cc: Manasi Navare <manasi.d.navare@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 73 ++++++++++++++++++++++++++++++++++- > include/drm/drm_dp_mst_helper.h | 3 ++ > 2 files changed, 75 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index 6fac4129e6a2..0d6604500b29 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -2076,7 +2076,7 @@ static bool drm_dp_get_vc_payload_bw(int dp_link_bw, > { > switch (dp_link_bw) { > default: > - DRM_DEBUG_KMS("invalid link bandwidth in DPCD: %x (link count: %d)\n", > + DRM_DEBUG_KMS("invalid link bandwidth: %x (link count: %d)\n", > dp_link_bw, dp_link_count); > return false; > > @@ -2096,6 +2096,77 @@ static bool drm_dp_get_vc_payload_bw(int dp_link_bw, > return true; > } > > +static void drm_dp_set_mstb_link_status(struct drm_dp_mst_branch *mstb, > + enum drm_link_status status) > +{ > + struct drm_dp_mst_branch *rmstb; > + struct drm_dp_mst_port *port; > + > + list_for_each_entry(port, &mstb->ports, next) { > + rmstb = drm_dp_get_validated_mstb_ref(mstb->mgr, port->mstb); > + if (rmstb) { > + drm_dp_set_mstb_link_status(rmstb, status); > + drm_dp_put_mst_branch_device(rmstb); > + } > + > + if (port->connector) > + port->connector->state->link_status = status; > + } > +} > + > +/** > + * drm_dp_mst_topology_mgr_lower_link_rate() - Override the DP link bw/count > + * for all connectors in a given MST topology > + * @mgr: manager to set state for > + * @dp_link_bw: The new DP link bandwidth I would rather call this argument as dp_link_rate since thats what we are passing and not dp_link_bw. > + * @dp_link_count: The new DP link count > + * > + * This is called by the driver when it detects that the current DP link for > + * the given topology manager is unstable, and needs to be retrained at a > + * lower link rate. > + * > + * This takes care of updating the link status on all downstream connectors > + * along with recalculating the VC payloads. The driver should send a hotplug > + * event after calling this function to notify userspace of the link status > + * change. > + * > + * RETURNS: > + * > + * True for success, or negative error code on failure. > + */ > +int drm_dp_mst_topology_mgr_lower_link_rate(struct drm_dp_mst_topology_mgr *mgr, > + int dp_link_bw, int dp_link_count) > +{ > + struct drm_device *dev = mgr->dev; > + struct drm_dp_mst_branch *mst_primary; > + int new_pbn_div; > + int ret = 0; > + > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > + > + if (!drm_dp_get_vc_payload_bw(drm_dp_link_rate_to_bw_code(dp_link_bw), This explains my comment above, we convert link rate to link_bw here so better call that link_rate to begin with > + dp_link_count, &new_pbn_div)) { > + ret = -EINVAL; > + goto out; > + } > + > + mst_primary = drm_dp_get_validated_mstb_ref(mgr, mgr->mst_primary); > + if (!mst_primary) > + goto out; > + > + DRM_DEBUG_KMS("MST link failed to retrain, lowering pbn_div to %d\n", > + new_pbn_div); This debug message seems a little misleading since we are saying that it failed to train link or retrain at the same params and now we are retraining with fallback values. Clarify this a bit more in this message like "MST link training failed or retrain at same params failed"..? Manasi > + mgr->pbn_div = new_pbn_div; > + > + drm_dp_set_mstb_link_status(mst_primary, DRM_MODE_LINK_STATUS_BAD); > + > + drm_dp_put_mst_branch_device(mst_primary); > +out: > + drm_modeset_unlock(&dev->mode_config.connection_mutex); > + return ret; > +} > +EXPORT_SYMBOL(drm_dp_mst_topology_mgr_lower_link_rate); > + > /** > * drm_dp_mst_topology_mgr_set_mst() - Set the MST state for a topology manager > * @mgr: manager to set state for > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h > index 7f78d26a0766..6261ec43a2c0 100644 > --- a/include/drm/drm_dp_mst_helper.h > +++ b/include/drm/drm_dp_mst_helper.h > @@ -634,4 +634,7 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state, > int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr *mgr, > struct drm_dp_mst_port *port, bool power_up); > > +int drm_dp_mst_topology_mgr_lower_link_rate(struct drm_dp_mst_topology_mgr *mgr, > + int dp_link_bw, int dp_link_count); > + > #endif > -- > 2.14.3 > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2018-03-12 21:25 UTC|newest] Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-03-08 23:24 [PATCH 0/6] Implement proper MST fallback retraining in i915 Lyude Paul 2018-03-08 23:24 ` Lyude Paul 2018-03-08 23:24 ` [PATCH 1/6] drm/i915: Remove unused DP_LINK_CHECK_TIMEOUT Lyude Paul 2018-03-08 23:24 ` Lyude Paul 2018-03-09 6:59 ` Manasi Navare 2018-03-08 23:24 ` [PATCH 2/6] drm/i915: Move DP modeset retry work into intel_dp Lyude Paul 2018-03-08 23:24 ` Lyude Paul 2018-03-08 23:41 ` [PATCH v2 " Lyude Paul 2018-03-08 23:41 ` Lyude Paul 2018-03-09 7:25 ` Manasi Navare 2018-03-09 7:25 ` Manasi Navare 2018-03-08 23:24 ` [PATCH 3/6] drm/i915: Only use one link bw config for MST topologies Lyude Paul 2018-03-08 23:24 ` Lyude Paul 2018-03-08 23:24 ` [PATCH 4/6] drm/dp_mst: Add drm_dp_mst_topology_mgr_lower_link_rate() Lyude Paul 2018-03-08 23:24 ` Lyude Paul 2018-03-08 23:24 ` [PATCH 5/6] drm/dp_mst: Add drm_atomic_dp_mst_retrain_topology() Lyude Paul 2018-03-08 23:24 ` Lyude Paul 2018-03-08 23:24 ` [PATCH 6/6] drm/i915: Implement proper fallback training for MST Lyude Paul 2018-03-08 23:24 ` Lyude Paul 2018-03-09 21:32 ` [PATCH v3 1/5] drm/i915: Move DP modeset retry work into intel_dp Lyude Paul 2018-03-09 21:32 ` Lyude Paul 2018-03-09 21:32 ` [PATCH v3 2/5] drm/i915: Only use one link bw config for MST topologies Lyude Paul 2018-03-09 21:32 ` Lyude Paul 2018-03-12 20:45 ` Manasi Navare 2018-03-12 20:45 ` Manasi Navare 2018-03-13 23:18 ` Lyude Paul 2018-03-12 21:05 ` Ville Syrjälä 2018-03-09 21:32 ` [PATCH v3 3/5] drm/dp_mst: Add drm_dp_mst_topology_mgr_lower_link_rate() Lyude Paul 2018-03-09 21:32 ` Lyude Paul 2018-03-12 21:28 ` Manasi Navare [this message] 2018-03-12 21:28 ` Manasi Navare 2018-03-09 21:32 ` [PATCH v3 4/5] drm/dp_mst: Add drm_atomic_dp_mst_retrain_topology() Lyude Paul 2018-03-09 21:32 ` [PATCH v3 5/5] drm/i915: Implement proper fallback training for MST Lyude Paul 2018-03-09 21:32 ` Lyude Paul 2018-03-12 21:12 ` Ville Syrjälä 2018-03-12 21:12 ` Ville Syrjälä 2018-03-12 22:05 ` Manasi Navare 2018-03-12 22:05 ` Manasi Navare 2018-03-12 22:16 ` Lyude Paul 2018-03-12 22:16 ` Lyude Paul 2018-03-12 21:01 ` [PATCH v3 1/5] drm/i915: Move DP modeset retry work into intel_dp Ville Syrjälä 2018-03-12 21:01 ` Ville Syrjälä 2018-03-13 23:24 ` Lyude Paul 2018-03-13 23:24 ` Lyude Paul 2018-03-14 17:28 ` Ville Syrjälä 2018-03-14 17:28 ` Ville Syrjälä
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180312212814.GB3022@intel.com \ --to=manasi.d.navare@intel.com \ --cc=airlied@linux.ie \ --cc=dri-devel@lists.freedesktop.org \ --cc=gustavo@padovan.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ --cc=lyude@redhat.com \ --cc=maarten.lankhorst@linux.intel.com \ --cc=seanpaul@chromium.org \ --cc=ville.syrjala@linux.intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.