| Message ID | 20171019133721.11794-9-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 19-10-17 om 15:37 schreef Ville Syrjala: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Pass an old crtc state to intel_ddi_post_disable() from the MST code. > > Note that this crtc state won't necessaitly match the one that was > passed to intel_ddi_pre_enable() if the first stream to be enabled isn't > the last stream to be disabled. But this is fine since the states should > be identical in every important way. This does mean people frobbing > the DDI pre_enable/post_disable hooks have to pay attention in what > parts of the state they consult. > > The alternative would be to inline the relevant code into the MST code. > That is actually what we used to do for pre_enable before > commit e081c8463ac9 ("drm/i915: Remove duplicate DDI enabling logic > from MST path"). For post_disable we've always called the DDI hook. I woudl rather have it NULL throughout the entire disable sequence, it's going to be dangerous anyway, and something dangerous shouldn't be easy. The fact that you have to think Also there's an unrelated hunk in intel_ddi_pre_enable?
On Fri, Oct 27, 2017 at 01:39:00PM +0200, Maarten Lankhorst wrote: > Op 19-10-17 om 15:37 schreef Ville Syrjala: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Pass an old crtc state to intel_ddi_post_disable() from the MST code. > > > > Note that this crtc state won't necessaitly match the one that was > > passed to intel_ddi_pre_enable() if the first stream to be enabled isn't > > the last stream to be disabled. But this is fine since the states should > > be identical in every important way. This does mean people frobbing > > the DDI pre_enable/post_disable hooks have to pay attention in what > > parts of the state they consult. > > > > The alternative would be to inline the relevant code into the MST code. > > That is actually what we used to do for pre_enable before > > commit e081c8463ac9 ("drm/i915: Remove duplicate DDI enabling logic > > from MST path"). For post_disable we've always called the DDI hook. > > I woudl rather have it NULL throughout the entire disable sequence, it's going to be > dangerous anyway, and something dangerous shouldn't be easy. The fact that you have > to think It makes for really ugly code as the enable and disable don't look at all like each other when one has the NULL and the other doesn't. If it's dangerous for disable, then it's equally dangerous for enable. So I'd just go with whatever makes the code suck the least, which IMO is passing a crtc state to both. Probably the proper fix would be some kind of encoder state which would be assigned to mst->primary. But that's going to require actual design work. > > Also there's an unrelated hunk in intel_ddi_pre_enable? Apparently I wanted to get rid of the local pipe variable.
Op 27-10-17 om 13:49 schreef Ville Syrjälä: > On Fri, Oct 27, 2017 at 01:39:00PM +0200, Maarten Lankhorst wrote: >> Op 19-10-17 om 15:37 schreef Ville Syrjala: >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> >>> Pass an old crtc state to intel_ddi_post_disable() from the MST code. >>> >>> Note that this crtc state won't necessaitly match the one that was >>> passed to intel_ddi_pre_enable() if the first stream to be enabled isn't >>> the last stream to be disabled. But this is fine since the states should >>> be identical in every important way. This does mean people frobbing >>> the DDI pre_enable/post_disable hooks have to pay attention in what >>> parts of the state they consult. >>> >>> The alternative would be to inline the relevant code into the MST code. >>> That is actually what we used to do for pre_enable before >>> commit e081c8463ac9 ("drm/i915: Remove duplicate DDI enabling logic >>> from MST path"). For post_disable we've always called the DDI hook. >> I woudl rather have it NULL throughout the entire disable sequence, it's going to be >> dangerous anyway, and something dangerous shouldn't be easy. The fact that you have >> to think > It makes for really ugly code as the enable and disable don't look at > all like each other when one has the NULL and the other doesn't. If it's > dangerous for disable, then it's equally dangerous for enable. So I'd > just go with whatever makes the code suck the least, which IMO is > passing a crtc state to both. > > Probably the proper fix would be some kind of encoder state which would > be assigned to mst->primary. But that's going to require actual design > work. Yeah, I just think it's dangerous regardless, I wish we could do something more safe about it, but this will have to do for now. Theoretically we could add encoder state, but that probably requires a massive split in crtc_state. Not sure what's wisdom here. :/
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index a8ae95e4c6f2..0123c4753cd4 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2210,11 +2210,15 @@ static void intel_ddi_pre_enable(struct intel_encoder *encoder, { struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); - enum pipe pipe = crtc->pipe; + + /* + * conn_state is NULL when called from DP_MST. The main connector + * associated with this port is never bound to a crtc for MST. + */ WARN_ON(crtc_state->has_pch_encoder); - intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true); + intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true); if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) intel_ddi_pre_enable_hdmi(encoder, crtc_state, conn_state); @@ -2252,12 +2256,7 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder, struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); struct intel_dp *intel_dp = &dig_port->dp; - /* - * old_crtc_state and old_conn_state are NULL when called from - * DP_MST. The main connector associated with this port is never - * bound to a crtc for MST. - */ - bool is_mst = !old_crtc_state; + bool is_mst = intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST); /* * Power down sink before disabling the port, otherwise we end @@ -2301,12 +2300,11 @@ static void intel_ddi_post_disable(struct intel_encoder *encoder, const struct drm_connector_state *old_conn_state) { /* - * old_crtc_state and old_conn_state are NULL when called from - * DP_MST. The main connector associated with this port is never - * bound to a crtc for MST. + * old_conn_state is NULL when called from DP_MST. The main connector + * associated with this port is never bound to a crtc for MST. */ - if (old_crtc_state && - intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_HDMI)) + + if (intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_HDMI)) intel_ddi_post_disable_hdmi(encoder, old_crtc_state, old_conn_state); else diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index 9be6f32bb100..cdffe7e93a71 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -172,10 +172,10 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder, intel_dp->active_mst_links--; intel_mst->connector = NULL; - if (intel_dp->active_mst_links == 0) { + if (intel_dp->active_mst_links == 0) intel_dig_port->base.post_disable(&intel_dig_port->base, - NULL, NULL); - } + old_crtc_state, NULL); + DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links); }