Re: [Intel-gfx] [PATCH v7 4/4] drm/dp: Track MST link bandwidth

On Tue, 2017-04-25 at 09:51 +0200, Maarten Lankhorst wrote:

On 21-04-17 07:51, Dhinakaran Pandiyan wrote:

...

From: "Pandiyan, Dhinakaran" dhinakaran.pandiyan@intel.com

Use the added helpers to track MST link bandwidth for atomic modesets. Link bw is acquired in the ->atomic_check() phase when CRTCs are being enabled with drm_atomic_find_vcpi_slots() instead of drm_find_vcpi_slots(). Similarly, link bw is released during ->atomic_check() with the connector helper callback ->atomic_release() when CRTCs are disabled.

v5: Implement connector->atomic_check() in place of atomic_release() v4: Return an int from intel_dp_mst_atomic_release() (Maarten) v3: Handled the case where ->atomic_release() is called more than once during an atomic_check() for the same state. v2: Squashed atomic_release() implementation and caller (Daniel) Fixed logic for connector-crtc switching case (Daniel) Fixed logic for suspend-resume case.

Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Archit Taneja architt@codeaurora.org Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Harry Wentland Harry.wentland@amd.com Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com

---

drivers/gpu/drm/i915/intel_dp_mst.c | 57 +++++++++++++++++++++++++++++++++---- 1 file changed, 51 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index 5af22a7..20c557c 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -39,9 +39,9 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder, struct intel_dp *intel_dp = &intel_dig_port->dp; struct intel_connector *connector = to_intel_connector(conn_state->connector);

• struct drm_atomic_state *state;

• struct drm_atomic_state *state = pipe_config->base.state; int bpp;

• int lane_count, slots;

• int lane_count, slots = 0; const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode; int mst_pbn;

@@ -63,24 +63,68 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder, pipe_config->pipe_bpp = bpp; pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);

• state = pipe_config->base.state;
• if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, connector->port)) pipe_config->has_audio = true;
• mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);

• mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp); pipe_config->pbn = mst_pbn;

• slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);

  intel_link_compute_m_n(bpp, lane_count, adjusted_mode->crtc_clock, pipe_config->port_clock, &pipe_config->dp_m_n);

• if (pipe_config->base.active) {

Is this check needed? If so it needs a comment as to why.

I know that the output won't actually turn on, but if the crtc is configured vcpi slots should be allocated if possible.

Else the following will fail in the wrong place:

1. Configure CRTC with a valid mode and DP-MST connector, but set active=false. connectors_changed = true, mode_changed = true. Not enough VCPI slots to enable all crtc's at the same time now, but this returns OK.
2. Commit with active property set to true. There are too few vcpi slots to make this work, this atomic commit returns -EINVAL. This might happen on a different crtc than the newly configured one in stap 1, if they were all previously set to !active.

I gave this more thought, what you are recommending is not allocating vcpi for active false->true but instead doing it only for enable false->true. The problem I see is encoder->compute_config() is called to configure the link for a modeset with active=false->true transition. If we don't want to allocate vcpi slots in this case, we should not be doing other config. computations as well i.e., not call encoder->compute_config() at all for active_changed. This means we should complete this FIXME in intel_atomic_check()

/* FIXME: For only active_changed we shouldn't need to do any * state recomputation at all. */

Otoh if you want me to remove 'if (pipe_config->base.active)', we can do this -

a) allocate slots unconditionally in compute_config() intel_link_compute_m_n(bpp, lane_count, adjusted_mode->crtc_clock, pipe_config->port_clock, &pipe_config->dp_m_n);

+ slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr, + connector->port, mst_pbn); + if (slots < 0) { + DRM_DEBUG_KMS("failed finding vcpi slots:%d\n", slots); + return false; + } pipe_config->dp_m_n.tu = slots;

b) release slots on connectors_changed||mode_changed||active_changed.

+ if (drm_atomic_crtc_needs_modeset(crtc_state) && slots > 0) { + struct drm_dp_mst_topology_mgr *mgr; + struct drm_encoder *old_encoder; + + old_encoder = old_conn_state->best_encoder; + mgr = &enc_to_mst(old_encoder)->primary->dp.mst_mgr; + + ret = drm_dp_atomic_release_vcpi_slots(state, mgr, slots);

Full patch - https://pastebin.ubuntu.com/24469988/ This assumes encoder->compute_config() will be called only once for a state.

-DK

...

• slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr,
  

• 			      connector->port, mst_pbn);
  

• if (slots < 0) {
  

• 	DRM_DEBUG_KMS("failed finding vcpi slots:%d\n", slots);
  

• 	return false;
  

• }
  

• } pipe_config->dp_m_n.tu = slots;

  return true;

+}

+static inline bool release_resources(struct drm_crtc_state *crtc_state) +{

• return (crtc_state->connectors_changed ||

• crtc_state->mode_changed ||
  

• (crtc_state->active_changed && !crtc_state->active));
  

+}

+static int intel_dp_mst_atomic_check(struct drm_connector *connector,

• struct drm_connector_state *new_conn_state)
  

+{

• struct drm_atomic_state *state = new_conn_state->state;
• struct drm_connector_state *old_conn_state;
• struct drm_crtc *old_crtc;
• struct drm_crtc_state *crtc_state;
• int slots, ret = 0;
• old_conn_state = drm_atomic_get_old_connector_state(state, connector);
• old_crtc = old_conn_state->crtc;
• if (!old_crtc)

• return 0;
  

• crtc_state = drm_atomic_get_new_crtc_state(state, old_crtc);
• slots = to_intel_crtc_state(crtc_state)->dp_m_n.tu;
• if (release_resources(crtc_state) && slots > 0) {

• struct drm_dp_mst_topology_mgr *mgr;
  

• struct drm_encoder *old_encoder;
  

• old_encoder = old_conn_state->best_encoder;
  

• mgr = &enc_to_mst(old_encoder)->primary->dp.mst_mgr;
  

• ret = drm_dp_atomic_release_vcpi_slots(state, mgr, slots);
  

• if (ret)
  

• 	DRM_DEBUG_KMS("failed releasing %d vcpi slots:%d\n", slots, ret);
  

• else
  

• 	to_intel_crtc_state(crtc_state)->dp_m_n.tu = 0;
  

• }
• return ret;

}

static void intel_mst_disable_dp(struct intel_encoder *encoder, @@ -378,6 +422,7 @@ static const struct drm_connector_helper_funcs intel_dp_mst_connector_helper_fun .mode_valid = intel_dp_mst_mode_valid, .atomic_best_encoder = intel_mst_atomic_best_encoder, .best_encoder = intel_mst_best_encoder,

• .atomic_check = intel_dp_mst_atomic_check,

};

static void intel_dp_mst_encoder_destroy(struct drm_encoder *encoder)

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx