On Fri, Jul 29, 2016 at 11:36:40AM -0700, Jim Bride wrote:
> On Fri, Jul 29, 2016 at 02:36:23PM +0300, Ville Syrjälä wrote:
> > On Fri, Jul 29, 2016 at 11:22:32AM +0200, Daniel Vetter wrote:
> > > On Thu, Jul 28, 2016 at 05:50:41PM +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > >
> > > > s/active_mst_links/active_streams/ and use it also for SST. We can then
> > > > use this information in the hpd handling to see if the link is active
> > > > or not, and thus whether we may need to retrain.
> > > >
> > > > Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@xxxxxxxxx>
> > > > Cc: Jim Bride <jim.bride@xxxxxxxxxxxxxxx>
> > > > Cc: Manasi D Navare <manasi.d.navare@xxxxxxxxx>
> > > > Cc: Durgadoss R <durgadoss.r@xxxxxxxxx>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > Hm, more state not in the state structures, and not cross-checked :( Ben
> > > Skeggs just pinged me on this, and one of the ideas he's now looking into
> > > is a separate state array for mst ports to fully keep track of this. Kinda
> > > like we keep track of the shared dplls.
> >
> > Yeah it's a bit a mess. The other option I've outlined before might be a
> > fake crtc to drive the primary, and then use atomic enable/disable it at
> > the right time depending the state the of the actual MST streams.
Was more thinking of tracking a intel_dp_mst_port_state per mst capable
port. And then treating it similarly to how we treat the shared dpll,
including state readout, tracking, checking and all that. Fake crtc seems
like abusing the wrong concept.
But tbh I'm not entirely sure what really makes the most sense, maybe
better to keep on stitching stuff together to at least fix the bugs for
now.
> > Speaking of atomic/mst, I'm not sure our link retraining is really good
> > enough for MST. IIRC I saw a note in the spec that the payloads and
> > whatnot might get deallocated by the sink/hub if the link drops. I think
> > I have to re-read the spec a few times to make sure, but if that's the
> > case then we'd have to pimp up the link retraining to be MST aware. But,
> > I had an alternative idea recently; What if we just force a modeset on
> > all the pipes on that port? IIRC the spec even says that for link
> > retraining we should really be doing more or less the full modeset
> > sequence. It should also get rid of the FIFO underruns we now get every
> > time we retrain the link. Any thoughts?
>
> If we ensure that we train the link at the widest and fastest configuration
> possible in the first place (which is how I interpret what the DP spec says
> to do for link training) then we shouldn't be in a situation where we need
> to retrain the main link unless the link is lost, in which case all of the
> pipes & payloads associated with the link will need to be re-configured
> anyhow (since there's no guarantee that the link bandwidth will be the same
> once the re-training was done.) This seems like a simpler solution, IMHO.
We already link train the widest/faster link for mst. And this is about
retraining when the link is lost.
One question I have on this plan is what happens for a full unplug. Atm I
think we expect userspace to do a full modeset and we won't reconnect the
current state to the replugged hw. This is a bit different from all other
connectors (and dp sst) where an accidental cable trip doesn't result in a
black screen (assuming userspace doesn't fix things up on its own).
But reconnecting our internal dp objects with the replugged physical ones
might be tricky, so not sure this is a feasible plane. Besides this it
makes sense.
-Daniel
>
> Jim
>
>
> > >
> > > Again just ideas, code looks correct.
> > >
> > > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > >
> > > > ---
> > > > drivers/gpu/drm/i915/intel_ddi.c | 10 ++++++++++
> > > > drivers/gpu/drm/i915/intel_dp.c | 8 +++++++-
> > > > drivers/gpu/drm/i915/intel_dp_mst.c | 16 ++++++++--------
> > > > drivers/gpu/drm/i915/intel_drv.h | 2 +-
> > > > 4 files changed, 26 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > > index 3b3a0a808477..bc188ee6e37f 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > @@ -1616,6 +1616,9 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > > >
> > > > intel_ddi_init_dp_buf_reg(intel_encoder);
> > > >
> > > > + WARN_ON(intel_dp->active_streams != 0);
> > > > + intel_dp->active_streams++;
> > > > +
> > > > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > > > intel_dp_start_link_train(intel_dp);
> > > > if (port != PORT_A || INTEL_INFO(dev_priv)->gen >= 9)
> > > > @@ -1733,6 +1736,13 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
> > > > intel_psr_disable(intel_dp);
> > > > intel_edp_backlight_off(intel_dp);
> > > > }
> > > > +
> > > > + if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) {
> > > > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > > +
> > > > + intel_dp->active_streams--;
> > > > + WARN_ON(intel_dp->active_streams != 0);
> > > > + }
> > > > }
> > > >
> > > > bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 0096c651c21f..3a9c5d3b5c66 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -2685,6 +2685,9 @@ static void intel_enable_dp(struct intel_encoder *encoder)
> > > > lane_mask);
> > > > }
> > > >
> > > > + WARN_ON(intel_dp->active_streams != 0);
> > > > + intel_dp->active_streams++;
> > > > +
> > > > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > > > intel_dp_start_link_train(intel_dp);
> > > > intel_dp_stop_link_train(intel_dp);
> > > > @@ -3344,6 +3347,9 @@ intel_dp_link_down(struct intel_dp *intel_dp)
> > > >
> > > > DRM_DEBUG_KMS("\n");
> > > >
> > > > + intel_dp->active_streams--;
> > > > + WARN_ON(intel_dp->active_streams != 0);
> > > > +
> > > > if ((IS_GEN7(dev) && port == PORT_A) ||
> > > > (HAS_PCH_CPT(dev) && port != PORT_A)) {
> > > > DP &= ~DP_LINK_TRAIN_MASK_CPT;
> > > > @@ -3826,7 +3832,7 @@ go_again:
> > > > if (bret == true) {
> > > >
> > > > /* check link status - esi[10] = 0x200c */
> > > > - if (intel_dp->active_mst_links &&
> > > > + if (intel_dp->active_streams &&
> > > > !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> > > > DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> > > > intel_dp_start_link_train(intel_dp);
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > index 68a005d729e9..857cfa6928b3 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > @@ -99,7 +99,7 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder)
> > > > struct intel_dp *intel_dp = &intel_dig_port->dp;
> > > > int ret;
> > > >
> > > > - DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> > > > + DRM_DEBUG_KMS("%d\n", intel_dp->active_streams);
> > > >
> > > > drm_dp_mst_reset_vcpi_slots(&intel_dp->mst_mgr, intel_mst->connector->port);
> > > >
> > > > @@ -115,7 +115,7 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder)
> > > > struct intel_digital_port *intel_dig_port = intel_mst->primary;
> > > > struct intel_dp *intel_dp = &intel_dig_port->dp;
> > > >
> > > > - DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> > > > + DRM_DEBUG_KMS("%d\n", intel_dp->active_streams);
> > > >
> > > > /* this can fail */
> > > > drm_dp_check_act_status(&intel_dp->mst_mgr);
> > > > @@ -124,10 +124,10 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder)
> > > >
> > > > drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, intel_mst->connector->port);
> > > >
> > > > - intel_dp->active_mst_links--;
> > > > + intel_dp->active_streams--;
> > > >
> > > > intel_mst->connector = NULL;
> > > > - if (intel_dp->active_mst_links == 0) {
> > > > + if (intel_dp->active_streams == 0) {
> > > > intel_dig_port->base.post_disable(&intel_dig_port->base);
> > > > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > > > }
> > > > @@ -165,11 +165,11 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
> > > > */
> > > > found->encoder = encoder;
> > > >
> > > > - DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> > > > + DRM_DEBUG_KMS("%d\n", intel_dp->active_streams);
> > > >
> > > > intel_mst->connector = found;
> > > >
> > > > - if (intel_dp->active_mst_links == 0) {
> > > > + if (intel_dp->active_streams == 0) {
> > > > intel_prepare_ddi_buffer(&intel_dig_port->base);
> > > >
> > > > intel_ddi_clk_select(&intel_dig_port->base, intel_crtc->config);
> > > > @@ -193,7 +193,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
> > > > }
> > > >
> > > >
> > > > - intel_dp->active_mst_links++;
> > > > + intel_dp->active_streams++;
> > > > temp = I915_READ(DP_TP_STATUS(port));
> > > > I915_WRITE(DP_TP_STATUS(port), temp);
> > > >
> > > > @@ -210,7 +210,7 @@ static void intel_mst_enable_dp(struct intel_encoder *encoder)
> > > > enum port port = intel_dig_port->port;
> > > > int ret;
> > > >
> > > > - DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> > > > + DRM_DEBUG_KMS("%d\n", intel_dp->active_streams);
> > > >
> > > > if (intel_wait_for_register(dev_priv,
> > > > DP_TP_STATUS(port),
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 70d7f33d6747..7fef18288aa2 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -894,7 +894,7 @@ struct intel_dp {
> > > >
> > > > bool can_mst; /* this port supports mst */
> > > > bool is_mst;
> > > > - int active_mst_links;
> > > > + int active_streams; /* number of active streams (for SST and MST both) */
> > > > /* connector directly attached - won't be use for modeset in mst world */
> > > > struct intel_connector *attached_connector;
> > > >
> > > > --
> > > > 2.7.4
> > > >
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
