From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932521AbeCLVGA (ORCPT ); Mon, 12 Mar 2018 17:06:00 -0400 Received: from mga12.intel.com ([192.55.52.136]:5795 "EHLO mga12.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932372AbeCLVF7 (ORCPT ); Mon, 12 Mar 2018 17:05:59 -0400 X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,463,1515484800"; d="scan'208";a="23892710" Date: Mon, 12 Mar 2018 23:05:55 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Lyude Paul Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Manasi Navare , Jani Nikula , Joonas Lahtinen , Rodrigo Vivi , David Airlie , linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 2/5] drm/i915: Only use one link bw config for MST topologies Message-ID: <20180312210555.GS5453@intel.com> References: <20180308232421.14049-1-lyude@redhat.com> <20180309213232.19855-1-lyude@redhat.com> <20180309213232.19855-2-lyude@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180309213232.19855-2-lyude@redhat.com> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Mar 09, 2018 at 04:32:28PM -0500, Lyude Paul wrote: > When a DP MST link needs retraining, sometimes the hub will detect that > the current link bw config is impossible and will update it's RX caps in > the DPCD to reflect the new maximum link rate. Currently, we make the > assumption that the RX caps in the dpcd will never change like this. > This means if the sink changes it's RX caps after we've already set up > an MST link and we attempt to add or remove another sink from the > topology, we could put ourselves into an invalid state where we've tried > to configure different sinks on the same MST topology with different > link rates. We could also run into this situation if a sink reports a > higher link rate after suspend, usually from us having trained it with a > fallback bw configuration before suspending. > > So: "lock" the bw config by only using the max DP link rate/lane count > on the first modeset for an MST topology. For every modeset following, > we instead use the last configured link bw for this topology. We only > unlock the bw config when we've detected a new MST sink. > > Signed-off-by: Lyude Paul > Cc: Manasi Navare > Cc: Ville Syrjälä > --- > drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++-- > drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++------- > drivers/gpu/drm/i915/intel_drv.h | 6 ++++++ > 3 files changed, 30 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 5abf0c95725a..5645a194de92 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3871,18 +3871,25 @@ intel_dp_can_mst(struct intel_dp *intel_dp) > static void > intel_dp_configure_mst(struct intel_dp *intel_dp) > { > + bool was_mst; > + > if (!i915_modparams.enable_dp_mst) > return; > > if (!intel_dp->can_mst) > return; > > + was_mst = intel_dp->is_mst; > intel_dp->is_mst = intel_dp_can_mst(intel_dp); > > - if (intel_dp->is_mst) > + if (intel_dp->is_mst) { > DRM_DEBUG_KMS("Sink is MST capable\n"); > - else > + > + if (!was_mst) > + intel_dp->mst_bw_locked = false; > + } else { > DRM_DEBUG_KMS("Sink is not MST capable\n"); > + } > > drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, > intel_dp->is_mst); > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > index c3de0918ee13..c0553456b18e 100644 > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > @@ -42,7 +42,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder, > to_intel_connector(conn_state->connector); > struct drm_atomic_state *state = pipe_config->base.state; > int bpp; > - int lane_count, slots; > + int lane_count, link_rate, slots; > const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode; > int mst_pbn; > bool reduce_m_n = drm_dp_has_quirk(&intel_dp->desc, > @@ -56,16 +56,22 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder, > bpp); > } > /* > - * for MST we always configure max link bw - the spec doesn't > - * seem to suggest we should do otherwise. > + * for MST we always configure max link bw if we don't know better - > + * the spec doesn't seem to suggest we should do otherwise. But, > + * ensure it always stays consistent with the rest of this hub's > + * state. > */ > - lane_count = intel_dp_max_lane_count(intel_dp); > + if (intel_dp->mst_bw_locked) { > + lane_count = intel_dp->lane_count; > + link_rate = intel_dp->link_rate; This feels like something we should be tracking in the MST state. > + } else { > + lane_count = intel_dp_max_lane_count(intel_dp); > + link_rate = intel_dp_max_link_rate(intel_dp); > + } > > pipe_config->lane_count = lane_count; > - > pipe_config->pipe_bpp = bpp; > - > - pipe_config->port_clock = intel_dp_max_link_rate(intel_dp); > + pipe_config->port_clock = link_rate; > > if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, connector->port)) > pipe_config->has_audio = true; > @@ -221,6 +227,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder, > connector->encoder = encoder; > intel_mst->connector = connector; > > + intel_dp->mst_bw_locked = true; > + > DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links); > > drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, true); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 3f19dc80997f..fc338529e918 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1107,6 +1107,12 @@ struct intel_dp { > bool can_mst; /* this port supports mst */ > bool is_mst; > int active_mst_links; > + /* Set when we've already decided on a link bw for mst, to prevent us > + * from setting different link bandwiths if the hub tries to confuse > + * us by changing it later > + */ > + bool mst_bw_locked; > + > /* connector directly attached - won't be use for modeset in mst world */ > struct intel_connector *attached_connector; > > -- > 2.14.3 -- Ville Syrjälä Intel OTC