On Tue, Feb 06, 2024 at 12:47:22AM +0200, Ville Syrjälä wrote:
> On Tue, Jan 23, 2024 at 12:28:42PM +0200, Imre Deak wrote:
> > +static int check_inherited_tunnel_state(struct intel_atomic_state *state,
> > + struct intel_dp *intel_dp,
> > + const struct intel_digital_connector_state *old_conn_state)
> > +{
> > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > + struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > + const struct intel_connector *connector =
> > + to_intel_connector(old_conn_state->base.connector);
> > + struct intel_crtc *old_crtc;
> > + const struct intel_crtc_state *old_crtc_state;
> > +
> > + /*
> > + * If a BWA tunnel gets detected only after the corresponding
> > + * connector got enabled already without a BWA tunnel, or a different
> > + * BWA tunnel (which was removed meanwhile) the old CRTC state won't
> > + * contain the state of the current tunnel. This tunnel still has a
> > + * reserved BW, which needs to be released, add the state for such
> > + * inherited tunnels separately only to this atomic state.
> > + */
> > + if (!intel_dp_tunnel_bw_alloc_is_enabled(intel_dp))
> > + return 0;
> > +
> > + if (!old_conn_state->base.crtc)
> > + return 0;
> > +
> > + old_crtc = to_intel_crtc(old_conn_state->base.crtc);
> > + old_crtc_state = intel_atomic_get_old_crtc_state(state, old_crtc);
> > +
> > + if (!old_crtc_state->hw.active ||
> > + old_crtc_state->dp_tunnel_ref.tunnel == intel_dp->tunnel)
> > + return 0;
> > +
> > + drm_dbg_kms(&i915->drm,
> > + "[DPTUN %s][CONNECTOR:%d:%s][ENCODER:%d:%s][CRTC:%d:%s] Adding state for inherited tunnel %p\n",
> > + drm_dp_tunnel_name(intel_dp->tunnel),
> > + connector->base.base.id,
> > + connector->base.name,
> > + encoder->base.base.id,
> > + encoder->base.name,
> > + old_crtc->base.base.id,
> > + old_crtc->base.name,
> > + intel_dp->tunnel);
> > +
> > + return add_inherited_tunnel_state(state, intel_dp->tunnel, old_crtc);
>
> I still strongly dislike this "tunnels are magically created by detect
> behind our back" approach. IMO in an ideal world we'd only ever create the
> tunnels during modeset/sanitize. What was the reason that didn't work again?
> I think you explained it to me in person at least once already, but can't
> remember anymore...
The tunnel information, describing which group the tunnel belongs to and
so how much BW it can use is needed already during detect time: to
filter the connectors' mode list during connector probing and to
pass/fail an atomic check of connectors that go through a tunnel/group
based on the modes the connectors use, the BW these require vs. the
available BW of the tunnel group.
The atomic state for the tunnel - with the required BW through it - is
only created/added during a modeset.
> --
> Ville Syrjälä
> Intel
