Message ID | 20200313164831.5980-2-ville.syrjala@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/i915: Port sync for skl+ | expand |
This will not work for MST, here a example Previous state: MST master pipe A MST slave pipe B New state: Pipe A being disabled On drm_atomic_helper_check_modeset() both intel_crtc_states will be added to the state with crtc_state->uapi.mode_changed=true. Then on the regular for_each_oldnew_intel_crtc_in_state() loop config of CRTC B will have mst_master_transcoder=INVALID_TRANSCODER that differs from TRANSCODER_A and will keep mode_changed set. Then on the config_late loop it will skip the interation on the needs_modeset() check keeping CRTC B with mst_master_transcoder=INVALID_TRANSCODER. That would be caugh by CI if this tests were merged and the TGL machine with MST is still on: https://patchwork.freedesktop.org/series/72211/ On Fri, 2020-03-13 at 18:48 +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Use the recently introduced encoder .compute_config_late() hook to > do the MST master transcoder assignment. Avoids having to do it > in a funny way before we know the CPU transcoder of each pipe. > > And now we can also properly use hw.active instead of uapi.active > since it too has been calculated earlier for everyone. > > Cc: José Roberto de Souza <jose.souza@intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp_mst.c | 98 +++++++++++------ > ---- > 1 file changed, 51 insertions(+), 47 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index 44f3fd251ca1..b9afc1135b9b 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -88,56 +88,10 @@ static int > intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > return 0; > } > > -/* > - * Iterate over all connectors and return the smallest transcoder in > the MST > - * stream > - */ > -static enum transcoder > -intel_dp_mst_master_trans_compute(struct intel_atomic_state *state, > - struct intel_dp *mst_port) > -{ > - struct drm_i915_private *dev_priv = to_i915(state->base.dev); > - struct intel_digital_connector_state *conn_state; > - struct intel_connector *connector; > - enum pipe ret = I915_MAX_PIPES; > - int i; > - > - if (INTEL_GEN(dev_priv) < 12) > - return INVALID_TRANSCODER; > - > - for_each_new_intel_connector_in_state(state, connector, > conn_state, i) { > - struct intel_crtc_state *crtc_state; > - struct intel_crtc *crtc; > - > - if (connector->mst_port != mst_port || !conn_state- > >base.crtc) > - continue; > - > - crtc = to_intel_crtc(conn_state->base.crtc); > - crtc_state = intel_atomic_get_new_crtc_state(state, > crtc); > - if (!crtc_state->uapi.active) > - continue; > - > - /* > - * Using crtc->pipe because crtc_state->cpu_transcoder > is > - * computed, so others CRTCs could have non-computed > - * cpu_transcoder > - */ > - if (crtc->pipe < ret) > - ret = crtc->pipe; > - } > - > - if (ret == I915_MAX_PIPES) > - return INVALID_TRANSCODER; > - > - /* Simple cast works because TGL don't have a eDP transcoder */ > - return (enum transcoder)ret; > -} > - > static int intel_dp_mst_compute_config(struct intel_encoder > *encoder, > struct intel_crtc_state > *pipe_config, > struct drm_connector_state > *conn_state) > { > - struct intel_atomic_state *state = > to_intel_atomic_state(conn_state->state); > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder); > struct intel_dp *intel_dp = &intel_mst->primary->dp; > @@ -201,7 +155,56 @@ static int intel_dp_mst_compute_config(struct > intel_encoder *encoder, > > intel_ddi_compute_min_voltage_level(dev_priv, pipe_config); > > - pipe_config->mst_master_transcoder = > intel_dp_mst_master_trans_compute(state, intel_dp); > + return 0; > +} > + > +/* > + * Iterate over all connectors and return a mask of > + * all CPU transcoders streaming over the same DP link. > + */ > +static unsigned int > +intel_dp_mst_transcoder_mask(struct intel_atomic_state *state, > + struct intel_dp *mst_port) > +{ > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + const struct intel_digital_connector_state *conn_state; > + struct intel_connector *connector; > + u8 transcoders = 0; > + int i; > + > + if (INTEL_GEN(dev_priv) < 12) > + return 0; > + > + for_each_new_intel_connector_in_state(state, connector, > conn_state, i) { > + const struct intel_crtc_state *crtc_state; > + struct intel_crtc *crtc; > + > + if (connector->mst_port != mst_port || !conn_state- > >base.crtc) > + continue; > + > + crtc = to_intel_crtc(conn_state->base.crtc); > + crtc_state = intel_atomic_get_new_crtc_state(state, > crtc); > + > + if (!crtc_state->hw.active) > + continue; > + > + transcoders |= BIT(crtc_state->cpu_transcoder); > + } > + > + return transcoders; > +} > + > +static int intel_dp_mst_compute_config_late(struct intel_encoder > *encoder, > + struct intel_crtc_state > *crtc_state, > + struct drm_connector_state > *conn_state) > +{ > + struct intel_atomic_state *state = > to_intel_atomic_state(conn_state->state); > + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder); > + struct intel_dp *intel_dp = &intel_mst->primary->dp; > + > + /* lowest numbered transcoder will be designated master */ > + crtc_state->mst_master_transcoder = > + ffs(intel_dp_mst_transcoder_mask(state, intel_dp)) - 1; > > return 0; > } > @@ -786,6 +789,7 @@ intel_dp_create_fake_mst_encoder(struct > intel_digital_port *intel_dig_port, enum > intel_encoder->pipe_mask = ~0; > > intel_encoder->compute_config = intel_dp_mst_compute_config; > + intel_encoder->compute_config_late = > intel_dp_mst_compute_config_late; > intel_encoder->disable = intel_mst_disable_dp; > intel_encoder->post_disable = intel_mst_post_disable_dp; > intel_encoder->pre_pll_enable = intel_mst_pre_pll_enable_dp;
Never mind, read the code again it will work. Reviewed-by: José Roberto de Souza <jose.souza@intel.com> On Fri, 2020-03-20 at 23:37 +0000, Souza, Jose wrote: > This will not work for MST, here a example > > Previous state: > MST master pipe A > MST slave pipe B > > New state: > Pipe A being disabled > > On drm_atomic_helper_check_modeset() both intel_crtc_states will be > added to the state with crtc_state->uapi.mode_changed=true. > Then on the regular for_each_oldnew_intel_crtc_in_state() loop config > of CRTC B will have mst_master_transcoder=INVALID_TRANSCODER that > differs from TRANSCODER_A and will keep mode_changed set. > Then on the config_late loop it will skip the interation on the > needs_modeset() check keeping CRTC B with > mst_master_transcoder=INVALID_TRANSCODER. > > That would be caugh by CI if this tests were merged and the TGL > machine > with MST is still on: https://patchwork.freedesktop.org/series/72211/ > > On Fri, 2020-03-13 at 18:48 +0200, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Use the recently introduced encoder .compute_config_late() hook to > > do the MST master transcoder assignment. Avoids having to do it > > in a funny way before we know the CPU transcoder of each pipe. > > > > And now we can also properly use hw.active instead of uapi.active > > since it too has been calculated earlier for everyone. > > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 98 +++++++++++------ > > ---- > > 1 file changed, 51 insertions(+), 47 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > index 44f3fd251ca1..b9afc1135b9b 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > @@ -88,56 +88,10 @@ static int > > intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > > return 0; > > } > > > > -/* > > - * Iterate over all connectors and return the smallest transcoder > > in > > the MST > > - * stream > > - */ > > -static enum transcoder > > -intel_dp_mst_master_trans_compute(struct intel_atomic_state > > *state, > > - struct intel_dp *mst_port) > > -{ > > - struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > - struct intel_digital_connector_state *conn_state; > > - struct intel_connector *connector; > > - enum pipe ret = I915_MAX_PIPES; > > - int i; > > - > > - if (INTEL_GEN(dev_priv) < 12) > > - return INVALID_TRANSCODER; > > - > > - for_each_new_intel_connector_in_state(state, connector, > > conn_state, i) { > > - struct intel_crtc_state *crtc_state; > > - struct intel_crtc *crtc; > > - > > - if (connector->mst_port != mst_port || !conn_state- > > > base.crtc) > > - continue; > > - > > - crtc = to_intel_crtc(conn_state->base.crtc); > > - crtc_state = intel_atomic_get_new_crtc_state(state, > > crtc); > > - if (!crtc_state->uapi.active) > > - continue; > > - > > - /* > > - * Using crtc->pipe because crtc_state->cpu_transcoder > > is > > - * computed, so others CRTCs could have non-computed > > - * cpu_transcoder > > - */ > > - if (crtc->pipe < ret) > > - ret = crtc->pipe; > > - } > > - > > - if (ret == I915_MAX_PIPES) > > - return INVALID_TRANSCODER; > > - > > - /* Simple cast works because TGL don't have a eDP transcoder */ > > - return (enum transcoder)ret; > > -} > > - > > static int intel_dp_mst_compute_config(struct intel_encoder > > *encoder, > > struct intel_crtc_state > > *pipe_config, > > struct drm_connector_state > > *conn_state) > > { > > - struct intel_atomic_state *state = > > to_intel_atomic_state(conn_state->state); > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder); > > struct intel_dp *intel_dp = &intel_mst->primary->dp; > > @@ -201,7 +155,56 @@ static int intel_dp_mst_compute_config(struct > > intel_encoder *encoder, > > > > intel_ddi_compute_min_voltage_level(dev_priv, pipe_config); > > > > - pipe_config->mst_master_transcoder = > > intel_dp_mst_master_trans_compute(state, intel_dp); > > + return 0; > > +} > > + > > +/* > > + * Iterate over all connectors and return a mask of > > + * all CPU transcoders streaming over the same DP link. > > + */ > > +static unsigned int > > +intel_dp_mst_transcoder_mask(struct intel_atomic_state *state, > > + struct intel_dp *mst_port) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > + const struct intel_digital_connector_state *conn_state; > > + struct intel_connector *connector; > > + u8 transcoders = 0; > > + int i; > > + > > + if (INTEL_GEN(dev_priv) < 12) > > + return 0; > > + > > + for_each_new_intel_connector_in_state(state, connector, > > conn_state, i) { > > + const struct intel_crtc_state *crtc_state; > > + struct intel_crtc *crtc; > > + > > + if (connector->mst_port != mst_port || !conn_state- > > > base.crtc) > > + continue; > > + > > + crtc = to_intel_crtc(conn_state->base.crtc); > > + crtc_state = intel_atomic_get_new_crtc_state(state, > > crtc); > > + > > + if (!crtc_state->hw.active) > > + continue; > > + > > + transcoders |= BIT(crtc_state->cpu_transcoder); > > + } > > + > > + return transcoders; > > +} > > + > > +static int intel_dp_mst_compute_config_late(struct intel_encoder > > *encoder, > > + struct intel_crtc_state > > *crtc_state, > > + struct drm_connector_state > > *conn_state) > > +{ > > + struct intel_atomic_state *state = > > to_intel_atomic_state(conn_state->state); > > + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder); > > + struct intel_dp *intel_dp = &intel_mst->primary->dp; > > + > > + /* lowest numbered transcoder will be designated master */ > > + crtc_state->mst_master_transcoder = > > + ffs(intel_dp_mst_transcoder_mask(state, intel_dp)) - 1; > > > > return 0; > > } > > @@ -786,6 +789,7 @@ intel_dp_create_fake_mst_encoder(struct > > intel_digital_port *intel_dig_port, enum > > intel_encoder->pipe_mask = ~0; > > > > intel_encoder->compute_config = intel_dp_mst_compute_config; > > + intel_encoder->compute_config_late = > > intel_dp_mst_compute_config_late; > > intel_encoder->disable = intel_mst_disable_dp; > > intel_encoder->post_disable = intel_mst_post_disable_dp; > > intel_encoder->pre_pll_enable = intel_mst_pre_pll_enable_dp; > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index 44f3fd251ca1..b9afc1135b9b 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -88,56 +88,10 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, return 0; } -/* - * Iterate over all connectors and return the smallest transcoder in the MST - * stream - */ -static enum transcoder -intel_dp_mst_master_trans_compute(struct intel_atomic_state *state, - struct intel_dp *mst_port) -{ - struct drm_i915_private *dev_priv = to_i915(state->base.dev); - struct intel_digital_connector_state *conn_state; - struct intel_connector *connector; - enum pipe ret = I915_MAX_PIPES; - int i; - - if (INTEL_GEN(dev_priv) < 12) - return INVALID_TRANSCODER; - - for_each_new_intel_connector_in_state(state, connector, conn_state, i) { - struct intel_crtc_state *crtc_state; - struct intel_crtc *crtc; - - if (connector->mst_port != mst_port || !conn_state->base.crtc) - continue; - - crtc = to_intel_crtc(conn_state->base.crtc); - crtc_state = intel_atomic_get_new_crtc_state(state, crtc); - if (!crtc_state->uapi.active) - continue; - - /* - * Using crtc->pipe because crtc_state->cpu_transcoder is - * computed, so others CRTCs could have non-computed - * cpu_transcoder - */ - if (crtc->pipe < ret) - ret = crtc->pipe; - } - - if (ret == I915_MAX_PIPES) - return INVALID_TRANSCODER; - - /* Simple cast works because TGL don't have a eDP transcoder */ - return (enum transcoder)ret; -} - static int intel_dp_mst_compute_config(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config, struct drm_connector_state *conn_state) { - struct intel_atomic_state *state = to_intel_atomic_state(conn_state->state); struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder); struct intel_dp *intel_dp = &intel_mst->primary->dp; @@ -201,7 +155,56 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder, intel_ddi_compute_min_voltage_level(dev_priv, pipe_config); - pipe_config->mst_master_transcoder = intel_dp_mst_master_trans_compute(state, intel_dp); + return 0; +} + +/* + * Iterate over all connectors and return a mask of + * all CPU transcoders streaming over the same DP link. + */ +static unsigned int +intel_dp_mst_transcoder_mask(struct intel_atomic_state *state, + struct intel_dp *mst_port) +{ + struct drm_i915_private *dev_priv = to_i915(state->base.dev); + const struct intel_digital_connector_state *conn_state; + struct intel_connector *connector; + u8 transcoders = 0; + int i; + + if (INTEL_GEN(dev_priv) < 12) + return 0; + + for_each_new_intel_connector_in_state(state, connector, conn_state, i) { + const struct intel_crtc_state *crtc_state; + struct intel_crtc *crtc; + + if (connector->mst_port != mst_port || !conn_state->base.crtc) + continue; + + crtc = to_intel_crtc(conn_state->base.crtc); + crtc_state = intel_atomic_get_new_crtc_state(state, crtc); + + if (!crtc_state->hw.active) + continue; + + transcoders |= BIT(crtc_state->cpu_transcoder); + } + + return transcoders; +} + +static int intel_dp_mst_compute_config_late(struct intel_encoder *encoder, + struct intel_crtc_state *crtc_state, + struct drm_connector_state *conn_state) +{ + struct intel_atomic_state *state = to_intel_atomic_state(conn_state->state); + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder); + struct intel_dp *intel_dp = &intel_mst->primary->dp; + + /* lowest numbered transcoder will be designated master */ + crtc_state->mst_master_transcoder = + ffs(intel_dp_mst_transcoder_mask(state, intel_dp)) - 1; return 0; } @@ -786,6 +789,7 @@ intel_dp_create_fake_mst_encoder(struct intel_digital_port *intel_dig_port, enum intel_encoder->pipe_mask = ~0; intel_encoder->compute_config = intel_dp_mst_compute_config; + intel_encoder->compute_config_late = intel_dp_mst_compute_config_late; intel_encoder->disable = intel_mst_disable_dp; intel_encoder->post_disable = intel_mst_post_disable_dp; intel_encoder->pre_pll_enable = intel_mst_pre_pll_enable_dp;