[PATCH 0/6] Introduce DP MST Topology state

[PATCH 0/6] Introduce DP MST Topology state

[Dhinakaran Pandiyan]

Link bandwidth is shared between multiple display streams in DP MST
configurations. The DP MST topology manager structure maintains the shared
link bandwidth for a primary link directly connected to the GPU. For atomic
modesetting drivers, checking if there is sufficient link bandwidth for a
mode needs to be done during the atomic_check phase to avoid failed
modesets.

This series adds dp_mst_topology_state to track available link bw for
atomic modesets and MST helpers to find and release link bw in terms of
vcpi time slots. Using the new helpers is optional and the changes should
not affect drivers that don't support atomic modesetting. I have made
changes to i915 to use the new helpers, but this should be applicable to
nouveau as well.

Patches 1-3/6 include cleanups and refactoring.
Patch 4/6 adds the MST topology state, 5/6 adds helpers to alter the state
and 6/6 contains i915 changes to use the helpers .

Dhinakaran Pandiyan (6):
  drm/dp: Store drm_device in MST topology manager
  drm/dp: Kill unused MST vcpi slot availability tracking
  drm/dp: Split drm_dp_mst_allocate_vcpi
  drm/dp: Introduce DP MST topology manager state to track DP link bw
  drm/dp: Add DP MST helpers to atomically find and release vcpi slots
  drm/i915/dp: Track available DP MST vcpi time slots

 drivers/gpu/drm/drm_atomic.c           | 66 +++++++++++++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c    | 10 ++++
 drivers/gpu/drm/drm_dp_mst_topology.c  | 89 ++++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_display.c   | 39 ++++++++++++++-
 drivers/gpu/drm/i915/intel_dp_mst.c    | 42 ++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h       |  3 ++
 drivers/gpu/drm/nouveau/nv50_display.c |  5 +-
 drivers/gpu/drm/radeon/radeon_dp_mst.c |  5 +-
 include/drm/drm_atomic.h               | 11 +++++
 include/drm/drm_dp_mst_helper.h        | 35 ++++++++-----
 10 files changed, 263 insertions(+), 42 deletions(-)

-- 
2.7.4

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

[PATCH 1/6] drm/dp: Store drm_device in MST topology manager

[Dhinakaran Pandiyan]

struct drm_dp_mst_topology_mgr currently stores a pointer to struct dev.
Changing this to instead hold a pointer to drm_device is more useful as it
can give us access to DRM structures from the topology manager. This
also makes it consistent with other DRM structures like drm_crtc,
drm_connector etc.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c  | 6 +++---
 drivers/gpu/drm/i915/intel_dp_mst.c    | 3 ++-
 drivers/gpu/drm/nouveau/nv50_display.c | 2 +-
 drivers/gpu/drm/radeon/radeon_dp_mst.c | 2 +-
 include/drm/drm_dp_mst_helper.h        | 7 +++++--
 5 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index aa64448..122a1b0 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1086,7 +1086,7 @@ static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
 }
 
 static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,
-			    struct device *dev,
+			    struct drm_device *dev,
 			    struct drm_dp_link_addr_reply_port *port_msg)
 {
 	struct drm_dp_mst_port *port;
@@ -1104,7 +1104,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,
 		port->port_num = port_msg->port_number;
 		port->mgr = mstb->mgr;
 		port->aux.name = "DPMST";
-		port->aux.dev = dev;
+		port->aux.dev = dev->dev;
 		created = true;
 	} else {
 		old_pdt = port->pdt;
@@ -2949,7 +2949,7 @@ static void drm_dp_destroy_connector_work(struct work_struct *work)
  * Return 0 for success, or negative error code on failure
  */
 int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
-				 struct device *dev, struct drm_dp_aux *aux,
+				 struct drm_device *dev, struct drm_dp_aux *aux,
 				 int max_dpcd_transaction_bytes,
 				 int max_payloads, int conn_base_id)
 {
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 205fe47..38e3ca2 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -587,7 +587,8 @@ intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_ba
 
 	/* create encoders */
 	intel_dp_create_fake_mst_encoders(intel_dig_port);
-	ret = drm_dp_mst_topology_mgr_init(&intel_dp->mst_mgr, dev->dev, &intel_dp->aux, 16, 3, conn_base_id);
+	ret = drm_dp_mst_topology_mgr_init(&intel_dp->mst_mgr, dev,
+					   &intel_dp->aux, 16, 3, conn_base_id);
 	if (ret) {
 		intel_dp->can_mst = false;
 		return ret;
diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
index cb85cb7..452da48 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -3417,7 +3417,7 @@ nv50_mstm_new(struct nouveau_encoder *outp, struct drm_dp_aux *aux, int aux_max,
 	mstm->outp = outp;
 	mstm->mgr.cbs = &nv50_mstm;
 
-	ret = drm_dp_mst_topology_mgr_init(&mstm->mgr, dev->dev, aux, aux_max,
+	ret = drm_dp_mst_topology_mgr_init(&mstm->mgr, dev, aux, aux_max,
 					   max_payloads, conn_base_id);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c
index 6d1237d..7d5ada3 100644
--- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
+++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
@@ -667,7 +667,7 @@ radeon_dp_mst_init(struct radeon_connector *radeon_connector)
 		return 0;
 
 	radeon_connector->mst_mgr.cbs = &mst_cbs;
-	return drm_dp_mst_topology_mgr_init(&radeon_connector->mst_mgr, dev->dev,
+	return drm_dp_mst_topology_mgr_init(&radeon_connector->mst_mgr, dev,
 					    &radeon_connector->ddc_bus->aux, 16, 6,
 					    radeon_connector->base.base.id);
 }
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 0032076..27f3e99 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -414,7 +414,7 @@ struct drm_dp_mst_topology_mgr {
 	/**
 	 * @dev: device pointer for adding i2c devices etc.
 	 */
-	struct device *dev;
+	struct drm_device *dev;
 	/**
 	 * @cbs: callbacks for connector addition and destruction.
 	 */
@@ -556,7 +556,10 @@ struct drm_dp_mst_topology_mgr {
 	struct work_struct destroy_connector_work;
 };
 
-int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr, struct device *dev, struct drm_dp_aux *aux, int max_dpcd_transaction_bytes, int max_payloads, int conn_base_id);
+int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
+				 struct drm_device *dev, struct drm_dp_aux *aux,
+				 int max_dpcd_transaction_bytes,
+				 int max_payloads, int conn_base_id);
 
 void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr);
 
-- 
2.7.4

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

[PATCH 2/6] drm/dp: Kill unused MST vcpi slot availability tracking

[Dhinakaran Pandiyan]

The avail_slots member in the MST topology manager is never updated to
reflect the available vcpi slots. The check is effectively against
total_slots. So, let's make that check obvious. Secondly, since the total
vcpi time slots is always 63 and does not depend on the link BW, remove
total_slots from MST topology manager struct. The third change is to
remove total_pbn which is hardcoded to 2560. The total PBN that the
topology manager allocates from depends on the link rate and is not a
constant. So, fix this by removing the total_pbn member itself.

Finally, make debug messages more informative.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 14 ++++++--------
 include/drm/drm_dp_mst_helper.h       | 12 ------------
 2 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 122a1b0..5df00ae 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2042,10 +2042,6 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
 			goto out_unlock;
 		}
 
-		mgr->total_pbn = 2560;
-		mgr->total_slots = DIV_ROUND_UP(mgr->total_pbn, mgr->pbn_div);
-		mgr->avail_slots = mgr->total_slots;
-
 		/* add initial branch device at LCT 1 */
 		mstb = drm_dp_add_mst_branch_device(1, NULL);
 		if (mstb == NULL) {
@@ -2475,7 +2471,8 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
 
 	num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
 
-	if (num_slots > mgr->avail_slots)
+	/* max. time slots - one slot for MTP header */
+	if (num_slots > 63)
 		return -ENOSPC;
 	return num_slots;
 }
@@ -2489,7 +2486,8 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
 
 	num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
 
-	if (num_slots > mgr->avail_slots)
+	/* max. time slots - one slot for MTP header */
+	if (num_slots > 63)
 		return -ENOSPC;
 
 	vcpi->pbn = pbn;
@@ -2528,10 +2526,10 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp
 
 	ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn);
 	if (ret) {
-		DRM_DEBUG_KMS("failed to init vcpi %d %d %d\n", DIV_ROUND_UP(pbn, mgr->pbn_div), mgr->avail_slots, ret);
+		DRM_DEBUG_KMS("failed to init vcpi slots=%d max=63 ret=%d\n", DIV_ROUND_UP(pbn, mgr->pbn_div), ret);
 		goto out;
 	}
-	DRM_DEBUG_KMS("initing vcpi for %d %d\n", pbn, port->vcpi.num_slots);
+	DRM_DEBUG_KMS("initing vcpi for pbn=%d slots=%d\n", pbn, port->vcpi.num_slots);
 	*slots = port->vcpi.num_slots;
 
 	drm_dp_put_port(port);
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 27f3e99..b0f4a09 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -479,18 +479,6 @@ struct drm_dp_mst_topology_mgr {
 	 * @pbn_div: PBN to slots divisor.
 	 */
 	int pbn_div;
-	/**
-	 * @total_slots: Total slots that can be allocated.
-	 */
-	int total_slots;
-	/**
-	 * @avail_slots: Still available slots that can be allocated.
-	 */
-	int avail_slots;
-	/**
-	 * @total_pbn: Total PBN count.
-	 */
-	int total_pbn;
 
 	/**
 	 * @qlock: protects @tx_msg_downq, the tx_slots in struct
-- 
2.7.4

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

[PATCH 3/6] drm/dp: Split drm_dp_mst_allocate_vcpi

[Dhinakaran Pandiyan]

drm_dp_mst_allocate_vcpi() apart from setting up the vcpi structure,
also finds if there are enough slots available. This check is a duplicate
of that implemented in drm_dp_mst_find_vcpi_slots(). Let's move this check
out and reuse the existing drm_dp_mst_find_vcpi_slots() function to check
if there are enough vcpi slots before allocating them.

This brings the check to one place. Additionally drivers that will use MST
state tracking for atomic modesets can uses the atomic version of
find_vcpi_slots() and reuse drm_dp_mst_allocate_vcpi()

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c  | 20 +++++++++-----------
 drivers/gpu/drm/i915/intel_dp_mst.c    |  3 +--
 drivers/gpu/drm/nouveau/nv50_display.c |  3 ++-
 drivers/gpu/drm/radeon/radeon_dp_mst.c |  3 ++-
 include/drm/drm_dp_mst_helper.h        |  2 +-
 5 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 5df00ae..d42a6c0 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2479,20 +2479,17 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
 EXPORT_SYMBOL(drm_dp_find_vcpi_slots);
 
 static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
-			    struct drm_dp_vcpi *vcpi, int pbn)
+			    struct drm_dp_vcpi *vcpi, int pbn, int slots)
 {
-	int num_slots;
 	int ret;
 
-	num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
-
 	/* max. time slots - one slot for MTP header */
-	if (num_slots > 63)
+	if (slots > 63)
 		return -ENOSPC;
 
 	vcpi->pbn = pbn;
-	vcpi->aligned_pbn = num_slots * mgr->pbn_div;
-	vcpi->num_slots = num_slots;
+	vcpi->aligned_pbn = slots * mgr->pbn_div;
+	vcpi->num_slots = slots;
 
 	ret = drm_dp_mst_assign_payload_id(mgr, vcpi);
 	if (ret < 0)
@@ -2507,7 +2504,7 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
  * @pbn: payload bandwidth number to request
  * @slots: returned number of slots for this PBN.
  */
-bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port, int pbn, int *slots)
+bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port, int pbn, int slots)
 {
 	int ret;
 
@@ -2515,22 +2512,23 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp
 	if (!port)
 		return false;
 
+	if (slots < 0)
+		return false;
+
 	if (port->vcpi.vcpi > 0) {
 		DRM_DEBUG_KMS("payload: vcpi %d already allocated for pbn %d - requested pbn %d\n", port->vcpi.vcpi, port->vcpi.pbn, pbn);
 		if (pbn == port->vcpi.pbn) {
-			*slots = port->vcpi.num_slots;
 			drm_dp_put_port(port);
 			return true;
 		}
 	}
 
-	ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn);
+	ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);
 	if (ret) {
 		DRM_DEBUG_KMS("failed to init vcpi slots=%d max=63 ret=%d\n", DIV_ROUND_UP(pbn, mgr->pbn_div), ret);
 		goto out;
 	}
 	DRM_DEBUG_KMS("initing vcpi for pbn=%d slots=%d\n", pbn, port->vcpi.num_slots);
-	*slots = port->vcpi.num_slots;
 
 	drm_dp_put_port(port);
 	return true;
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 38e3ca2..02a1e2c 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -147,7 +147,6 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
 		to_intel_connector(conn_state->connector);
 	int ret;
 	uint32_t temp;
-	int slots;
 
 	/* MST encoders are bound to a crtc, not to a connector,
 	 * force the mapping here for get_hw_state.
@@ -177,7 +176,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
 
 	ret = drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr,
 				       connector->port,
-				       pipe_config->pbn, &slots);
+				       pipe_config->pbn, pipe_config->dp_m_n.tu);
 	if (ret == false) {
 		DRM_ERROR("failed to allocate vcpi\n");
 		return;
diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
index 452da48..91a4875 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -2959,7 +2959,8 @@ nv50_msto_enable(struct drm_encoder *encoder)
 	if (WARN_ON(!mstc))
 		return;
 
-	r = drm_dp_mst_allocate_vcpi(&mstm->mgr, mstc->port, mstc->pbn, &slots);
+	slots = drm_dp_find_vcpi_slots(&mstm->mgr, mstc->pbn);
+	r = drm_dp_mst_allocate_vcpi(&mstm->mgr, mstc->port, mstc->pbn, slots);
 	WARN_ON(!r);
 
 	if (mstm->outp->dcb->sorconf.link & 1)
diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c
index 7d5ada3..5ce34af 100644
--- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
+++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
@@ -453,9 +453,10 @@ radeon_mst_encoder_dpms(struct drm_encoder *encoder, int mode)
 		DRM_DEBUG_KMS("dig encoder is %d %d %d\n", dig_enc->dig_encoder,
 			      dig_enc->linkb, radeon_crtc->crtc_id);
 
+		slots = drm_dp_find_vcpi_slots(&radeon_connector->mst_port->mst_mgr, mst_enc->pbn);
 		ret = drm_dp_mst_allocate_vcpi(&radeon_connector->mst_port->mst_mgr,
 					       radeon_connector->port,
-					       mst_enc->pbn, &slots);
+					       mst_enc->pbn, slots);
 		ret = drm_dp_update_payload_part1(&radeon_connector->mst_port->mst_mgr);
 
 		radeon_dp_mst_set_be_cntl(primary, mst_enc,
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index b0f4a09..98d3c73 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -568,7 +568,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_
 int drm_dp_calc_pbn_mode(int clock, int bpp);
 
 
-bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port, int pbn, int *slots);
+bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port, int pbn, int slots);
 
 int drm_dp_mst_get_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);
 
-- 
2.7.4

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

[PATCH 4/6] drm/dp: Introduce DP MST topology manager state to track DP link bw

[Dhinakaran Pandiyan]

Link bandwidth is shared between multiple display streams in DP MST
configurations. The DP MST topology manager structure maintains the shared
link bandwidth for a primary link directly connected to the GPU. For atomic
modesetting drivers, checking if there is sufficient link bandwidth for a
mode needs to be done during the atomic_check phase to avoid failed
modesets. Let's encsapsulate the available link bw information in a state
structure so that bw can be allocated and released atomically for each of
the ports sharing the primary link.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_atomic.c          | 66 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c   | 10 ++++++
 drivers/gpu/drm/drm_dp_mst_topology.c | 10 ++++++
 include/drm/drm_atomic.h              | 13 +++++++
 include/drm/drm_dp_mst_helper.h       | 13 +++++++
 5 files changed, 112 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 681d5f9..b8e2cea 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -31,6 +31,7 @@
 #include <drm/drm_mode.h>
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_print.h>
+#include <drm/drm_dp_mst_helper.h>
 #include <linux/sync_file.h>
 
 #include "drm_crtc_internal.h"
@@ -62,6 +63,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state)
 	kfree(state->connectors);
 	kfree(state->crtcs);
 	kfree(state->planes);
+	kfree(state->dp_mst_topologies);
 }
 EXPORT_SYMBOL(drm_atomic_state_default_release);
 
@@ -189,6 +191,18 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
 		state->planes[i].ptr = NULL;
 		state->planes[i].state = NULL;
 	}
+
+	for (i = 0; i < state->num_mst_topologies; i++) {
+		struct drm_dp_mst_topology_mgr *mgr = state->dp_mst_topologies[i].ptr;
+
+		if (!mgr)
+			continue;
+
+		kfree(state->dp_mst_topologies[i].state);
+		state->dp_mst_topologies[i].ptr = NULL;
+		state->dp_mst_topologies[i].state = NULL;
+	}
+
 }
 EXPORT_SYMBOL(drm_atomic_state_default_clear);
 
@@ -981,6 +995,58 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
 		plane->funcs->atomic_print_state(p, state);
 }
 
+struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
+		struct drm_dp_mst_topology_mgr *mgr)
+{
+
+	int ret, i;
+	size_t new_size;
+	struct __drm_dp_mst_topology_state *new_arr;
+	struct drm_dp_mst_topology_state *new_mst_state;
+	int num_topologies;
+	struct drm_mode_config *config = &mgr->dev->mode_config;
+
+	WARN_ON(!state->acquire_ctx);
+
+	ret = drm_modeset_lock(&config->connection_mutex, state->acquire_ctx);
+	if (ret)
+		return ERR_PTR(ret);
+
+	for (i = 0; i < state->num_mst_topologies; i++) {
+		if (mgr == state->dp_mst_topologies[i].ptr &&
+		    state->dp_mst_topologies[i].state) {
+			return state->dp_mst_topologies[i].state;
+		}
+	}
+
+	num_topologies = state->num_mst_topologies + 1;
+	new_size = sizeof(*state->dp_mst_topologies) * num_topologies;
+	new_arr = krealloc(state->dp_mst_topologies, new_size, GFP_KERNEL);
+	if (!new_arr)
+		return ERR_PTR(-ENOMEM);
+
+	state->dp_mst_topologies = new_arr;
+	memset(&state->dp_mst_topologies[state->num_mst_topologies], 0,
+		sizeof(*state->dp_mst_topologies));
+
+	new_mst_state = kmalloc(sizeof(*mgr->state), GFP_KERNEL);
+	if (!new_mst_state)
+		return ERR_PTR(-ENOMEM);
+
+	new_mst_state->avail_slots = mgr->state->avail_slots;
+	state->dp_mst_topologies[state->num_mst_topologies].state = new_mst_state;
+	state->dp_mst_topologies[state->num_mst_topologies].ptr = mgr;
+	state->num_mst_topologies = num_topologies;
+	new_mst_state->mgr = mgr;
+	mgr->state->state = state;
+
+	DRM_DEBUG_ATOMIC("Added [MST Topology w/ base connector:%d] %p state to %p\n",
+			 mgr->conn_base_id, new_mst_state, state);
+
+	return new_mst_state;
+}
+EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
+
 /**
  * drm_atomic_get_connector_state - get connector state
  * @state: global atomic state object
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 5e52244..1cd38b7 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -30,6 +30,7 @@
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_dp_mst_helper.h>
 #include <linux/dma-fence.h>
 
 #include "drm_crtc_internal.h"
@@ -1992,6 +1993,15 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 		connector->state->state = NULL;
 	}
 
+	for (i = 0; i < state->num_mst_topologies; i++) {
+		struct drm_dp_mst_topology_mgr *mgr;
+
+		mgr = state->dp_mst_topologies[i].ptr;
+		mgr->state->state = state;
+		swap(state->dp_mst_topologies[i].state, mgr->state);
+		mgr->state->state = NULL;
+	}
+
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		crtc->state->state = state;
 		swap(state->crtcs[i].state, crtc->state);
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index d42a6c0..1be19e1 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2042,6 +2042,9 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
 			goto out_unlock;
 		}
 
+		/* max. time slots - one slot for MTP header */
+		mgr->state->avail_slots = 63;
+
 		/* add initial branch device at LCT 1 */
 		mstb = drm_dp_add_mst_branch_device(1, NULL);
 		if (mstb == NULL) {
@@ -2973,6 +2976,11 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
 	mgr->proposed_vcpis = kcalloc(max_payloads, sizeof(struct drm_dp_vcpi *), GFP_KERNEL);
 	if (!mgr->proposed_vcpis)
 		return -ENOMEM;
+	mgr->state = kzalloc(sizeof(*mgr->state), GFP_KERNEL);
+	if (!mgr->state)
+		return -ENOMEM;
+	mgr->state->mgr = mgr;
+
 	set_bit(0, &mgr->payload_mask);
 	if (test_calc_pbn_mode() < 0)
 		DRM_ERROR("MST PBN self-test failed\n");
@@ -2995,6 +3003,8 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr)
 	kfree(mgr->proposed_vcpis);
 	mgr->proposed_vcpis = NULL;
 	mutex_unlock(&mgr->payload_lock);
+	kfree(mgr->state);
+	mgr->state = NULL;
 	mgr->dev = NULL;
 	mgr->aux = NULL;
 }
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index fd2d971..7ac5ed6 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -153,6 +153,11 @@ struct __drm_connnectors_state {
 	struct drm_connector_state *state;
 };
 
+struct __drm_dp_mst_topology_state {
+	struct drm_dp_mst_topology_mgr *ptr;
+	struct drm_dp_mst_topology_state *state;
+};
+
 /**
  * struct drm_atomic_state - the global state object for atomic updates
  * @ref: count of all references to this state (will not be freed until zero)
@@ -164,6 +169,8 @@ struct __drm_connnectors_state {
  * @crtcs: pointer to array of CRTC pointers
  * @num_connector: size of the @connectors and @connector_states arrays
  * @connectors: pointer to array of structures with per-connector data
+ * @num_mst_topologies: size of the @dp_mst_topologies array
+ * @dp_mst_topologies: pointer to array of structures with per-MST topology manager data
  * @acquire_ctx: acquire context for this atomic modeset state update
  */
 struct drm_atomic_state {
@@ -177,6 +184,8 @@ struct drm_atomic_state {
 	struct __drm_crtcs_state *crtcs;
 	int num_connector;
 	struct __drm_connnectors_state *connectors;
+	int num_mst_topologies;
+	struct __drm_dp_mst_topology_state *dp_mst_topologies;
 
 	struct drm_modeset_acquire_ctx *acquire_ctx;
 
@@ -250,6 +259,10 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
 		struct drm_connector_state *state, struct drm_property *property,
 		uint64_t val);
 
+struct drm_dp_mst_topology_state * __must_check
+drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
+				  struct drm_dp_mst_topology_mgr *mgr);
+
 /**
  * drm_atomic_get_existing_crtc_state - get crtc state, if it exists
  * @state: global atomic state object
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 98d3c73..0a9bf20 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -403,6 +403,12 @@ struct drm_dp_payload {
 	int vcpi;
 };
 
+struct drm_dp_mst_topology_state {
+	int avail_slots;
+	struct drm_atomic_state *state;
+	struct drm_dp_mst_topology_mgr *mgr;
+};
+
 /**
  * struct drm_dp_mst_topology_mgr - DisplayPort MST manager
  *
@@ -481,6 +487,11 @@ struct drm_dp_mst_topology_mgr {
 	int pbn_div;
 
 	/**
+	 *  @state: MST topology manager state for atomic modesetting drivers
+	 */
+	struct drm_dp_mst_topology_state *state;
+
+	/**
 	 * @qlock: protects @tx_msg_downq, the tx_slots in struct
 	 * &drm_dp_mst_branch and txmsg->state once they are queued
 	 */
@@ -596,4 +607,6 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
 
 void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
 int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr);
+struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
+					struct drm_dp_mst_topology_mgr *mgr);
 #endif
-- 
2.7.4

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

[PATCH 5/6] drm/dp: Add DP MST helpers to atomically find and release vcpi slots

[Dhinakaran Pandiyan]

drm_dp_atomic_find_vcpi_slots() should be called from ->atomic_check() to
check there are sufficient vcpi slots for a mode and to add that to the
state. This should be followed by a call to drm_dp_mst_allocate_vcpi()
in ->atomic_commit() to initialize a struct vcpi for the port.

drm_dp_atomic_release_vcpi_slots() should be called from
->atomic_check() to release a port's vcpi slot allocation from the
state.

Drivers that do not make use of this atomic helper are expected to call
drm_dp_find_vcpi_slots() instead before calling
drm_dp_mst_allocate_vcpi().

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 43 +++++++++++++++++++++++++++++++++++
 include/drm/drm_dp_mst_helper.h       |  5 ++++
 2 files changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 1be19e1..737d61f 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2501,6 +2501,49 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
 }
 
 /**
+ * drm_dp_atomic_release_vcpi_slots() - Release allocated vcpi slots from the state
+ * @topology_state: MST topology state
+ * @port: port to release the vcpi slots for
+ */
+int drm_dp_atomic_release_vcpi_slots(struct drm_dp_mst_topology_state *topology_state,
+				     struct drm_dp_mst_port *port)
+{
+	int alloc = drm_dp_mst_get_vcpi_slots(topology_state->mgr, port);
+
+	topology_state->avail_slots += alloc;
+	DRM_DEBUG_KMS("vcpi slots released=%d, avail=%d\n",
+			alloc, topology_state->avail_slots);
+	return alloc;
+}
+EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
+
+/**
+ * drm_dp_atomic_find_vcpi_slots() - Find and add vcpi slots to the state
+ * @topology_state: MST topology state
+ * @port: port to find vcpi slots for
+ * @pbn: bandwidth required for the mode in PBN
+ */
+int drm_dp_atomic_find_vcpi_slots(struct drm_dp_mst_topology_state *topology_state,
+				  struct drm_dp_mst_port *port, int pbn)
+{
+	int num_slots, curr_alloc;
+	struct drm_dp_mst_topology_mgr *mgr = topology_state->mgr;
+
+	num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
+	curr_alloc = drm_dp_mst_get_vcpi_slots(mgr, port);
+	DRM_DEBUG_KMS("vcpi slots new=%d, curr=%d, avail=%d\n",
+			num_slots, curr_alloc, topology_state->avail_slots);
+
+	if (num_slots - curr_alloc > topology_state->avail_slots)
+		return -ENOSPC;
+
+	topology_state->avail_slots -= (num_slots - curr_alloc);
+	DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state->avail_slots);
+	return num_slots;
+}
+EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
+
+/**
  * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
  * @mgr: manager for this port
  * @port: port to allocate a virtual channel for.
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 0a9bf20..90395d8 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -609,4 +609,9 @@ void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
 int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr);
 struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
 					struct drm_dp_mst_topology_mgr *mgr);
+int drm_dp_atomic_find_vcpi_slots(struct drm_dp_mst_topology_state *topology_state,
+				  struct drm_dp_mst_port *port, int pbn);
+int drm_dp_atomic_release_vcpi_slots(struct drm_dp_mst_topology_state *topology_state,
+				     struct drm_dp_mst_port *port);
+
 #endif
-- 
2.7.4

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

[PATCH 6/6] drm/i915/dp: Track available DP MST vcpi time slots

[Dhinakaran Pandiyan]

Make use of the added MST helpers to find, allocate and release link bw
for atomic modesets.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_dp_mst.c  | 36 ++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  3 +++
 3 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ab72858..71e2ac7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14072,6 +14072,40 @@ static int calc_watermark_data(struct drm_atomic_state *state)
 	return 0;
 }
 
+static int intel_mst_clear_config(struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	struct drm_connector *connector;
+	struct drm_connector_state *connector_state;
+	int i, j;
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (!crtc_state->active_changed || crtc_state->active)
+			continue;
+
+		for_each_connector_in_state(state, connector, connector_state, j) {
+			struct intel_encoder *encoder;
+			struct drm_crtc *curr_crtc;
+			int slots;
+
+			encoder = to_intel_encoder(connector->state->best_encoder);
+			if (encoder->type != INTEL_OUTPUT_DP_MST)
+				continue;
+
+			curr_crtc = connector->state->crtc;
+			if (curr_crtc && crtc == curr_crtc) {
+				slots = to_intel_crtc_state(crtc->state)->dp_m_n.tu;
+				return intel_dp_mst_reset_vcpi(encoder,
+							       connector_state,
+							       slots);
+			}
+		}
+	}
+
+	return 0;
+}
+
 /**
  * intel_atomic_check - validate state object
  * @dev: drm device
@@ -14142,8 +14176,11 @@ static int intel_atomic_check(struct drm_device *dev,
 	}
 
 	if (any_ms) {
-		ret = intel_modeset_checks(state);
+		ret = intel_mst_clear_config(state);
+		if (ret)
+			return ret;
 
+		ret = intel_modeset_checks(state);
 		if (ret)
 			return ret;
 	} else {
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 02a1e2c..331909b 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -44,6 +44,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	int lane_count, slots;
 	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
 	int mst_pbn;
+	struct drm_dp_mst_topology_state *topology_state;
 
 	pipe_config->has_pch_encoder = false;
 	bpp = 24;
@@ -65,7 +66,18 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	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);
+
+	topology_state = drm_atomic_get_mst_topology_state(state,
+							   &intel_dp->mst_mgr);
+	if (topology_state == NULL)
+		return false;
+
+	slots = drm_dp_atomic_find_vcpi_slots(topology_state, connector->port,
+					      mst_pbn);
+	if (slots < 0) {
+		DRM_DEBUG_KMS("not enough link bw for this mode\n");
+		return false;
+	}
 
 	intel_link_compute_m_n(bpp, lane_count,
 			       adjusted_mode->crtc_clock,
@@ -78,6 +90,28 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 
 }
 
+int intel_dp_mst_reset_vcpi(struct intel_encoder *encoder,
+			     struct drm_connector_state *conn_state, int slots)
+{
+	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
+	struct drm_dp_mst_topology_mgr *mgr  = &intel_mst->primary->dp.mst_mgr;
+	struct drm_dp_mst_topology_state *topology_state;
+	struct intel_connector *connector =
+		to_intel_connector(conn_state->connector);
+	int released;
+
+	topology_state = drm_atomic_get_mst_topology_state(conn_state->state, mgr);
+	if (IS_ERR(topology_state))
+		return PTR_ERR(topology_state);
+
+	released = drm_dp_atomic_release_vcpi_slots(topology_state, connector->port);
+
+	if (WARN_ON(released != slots))
+		return -EINVAL;
+
+	return 0;
+}
+
 static void intel_mst_disable_dp(struct intel_encoder *encoder,
 				 struct intel_crtc_state *old_crtc_state,
 				 struct drm_connector_state *old_conn_state)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 6b02dac..ea2e41e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1487,6 +1487,9 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
 /* intel_dp_mst.c */
 int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
 void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port);
+int intel_dp_mst_reset_vcpi(struct intel_encoder *encoder,
+			     struct drm_connector_state *conn_state,
+			     int slots);
 /* intel_dsi.c */
 void intel_dsi_init(struct drm_i915_private *dev_priv);
 
-- 
2.7.4

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

Re: [PATCH 6/6] drm/i915/dp: Track available DP MST vcpi time slots

[Daniel Vetter]

On Tue, Jan 03, 2017 at 01:01:51PM -0800, Dhinakaran Pandiyan wrote:
> Make use of the added MST helpers to find, allocate and release link bw
> for atomic modesets.
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_dp_mst.c  | 36 ++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |  3 +++
>  3 files changed, 76 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ab72858..71e2ac7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14072,6 +14072,40 @@ static int calc_watermark_data(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> +static int intel_mst_clear_config(struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	struct drm_connector *connector;
> +	struct drm_connector_state *connector_state;
> +	int i, j;
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		if (!crtc_state->active_changed || crtc_state->active)
> +			continue;

I don't think the double-loop is needed.

> +
> +		for_each_connector_in_state(state, connector, connector_state, j) {
> +			struct intel_encoder *encoder;
> +			struct drm_crtc *curr_crtc;
> +			int slots;
> +
> +			encoder = to_intel_encoder(connector->state->best_encoder);
> +			if (encoder->type != INTEL_OUTPUT_DP_MST)
> +				continue;
> +
> +			curr_crtc = connector->state->crtc;
> +			if (curr_crtc && crtc == curr_crtc) {
> +				slots = to_intel_crtc_state(crtc->state)->dp_m_n.tu;
> +				return intel_dp_mst_reset_vcpi(encoder,
> +							       connector_state,
> +							       slots);
> +			}

Hm, I think it might be useful to have a generic atomic_release function
for connectors in the core atomic helpers. E.g. something like the below:


diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index b4dfd1e1a4f0..ce55e87b50e5 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -385,8 +385,12 @@ mode_fixup(struct drm_atomic_state *state)
 
 		WARN_ON(!!conn_state->best_encoder != !!conn_state->crtc);
 
-		if (!conn_state->crtc || !conn_state->best_encoder)
+		if (!conn_state->crtc || !conn_state->best_encoder) {
+			if (connector->helper_private->atomic_release)
+				connector->helper_private->atomic_release(connector,
+									  conn_state);
 			continue;
+		}
 
 		crtc_state = drm_atomic_get_existing_crtc_state(state,
 								conn_state->crtc);


I think we'll have other connectors in the future where we need to drop
shared resources in a similar fashion.

Cheers, Daniel


> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * intel_atomic_check - validate state object
>   * @dev: drm device
> @@ -14142,8 +14176,11 @@ static int intel_atomic_check(struct drm_device *dev,
>  	}
>  
>  	if (any_ms) {
> -		ret = intel_modeset_checks(state);
> +		ret = intel_mst_clear_config(state);
> +		if (ret)
> +			return ret;
>  
> +		ret = intel_modeset_checks(state);
>  		if (ret)
>  			return ret;
>  	} else {
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 02a1e2c..331909b 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -44,6 +44,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  	int lane_count, slots;
>  	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>  	int mst_pbn;
> +	struct drm_dp_mst_topology_state *topology_state;
>  
>  	pipe_config->has_pch_encoder = false;
>  	bpp = 24;
> @@ -65,7 +66,18 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  	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);
> +
> +	topology_state = drm_atomic_get_mst_topology_state(state,
> +							   &intel_dp->mst_mgr);
> +	if (topology_state == NULL)
> +		return false;
> +
> +	slots = drm_dp_atomic_find_vcpi_slots(topology_state, connector->port,
> +					      mst_pbn);
> +	if (slots < 0) {
> +		DRM_DEBUG_KMS("not enough link bw for this mode\n");
> +		return false;
> +	}
>  
>  	intel_link_compute_m_n(bpp, lane_count,
>  			       adjusted_mode->crtc_clock,
> @@ -78,6 +90,28 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  
>  }
>  
> +int intel_dp_mst_reset_vcpi(struct intel_encoder *encoder,
> +			     struct drm_connector_state *conn_state, int slots)
> +{
> +	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
> +	struct drm_dp_mst_topology_mgr *mgr  = &intel_mst->primary->dp.mst_mgr;
> +	struct drm_dp_mst_topology_state *topology_state;
> +	struct intel_connector *connector =
> +		to_intel_connector(conn_state->connector);
> +	int released;
> +
> +	topology_state = drm_atomic_get_mst_topology_state(conn_state->state, mgr);
> +	if (IS_ERR(topology_state))
> +		return PTR_ERR(topology_state);
> +
> +	released = drm_dp_atomic_release_vcpi_slots(topology_state, connector->port);
> +
> +	if (WARN_ON(released != slots))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static void intel_mst_disable_dp(struct intel_encoder *encoder,
>  				 struct intel_crtc_state *old_crtc_state,
>  				 struct drm_connector_state *old_conn_state)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6b02dac..ea2e41e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1487,6 +1487,9 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
>  /* intel_dp_mst.c */
>  int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
>  void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port);
> +int intel_dp_mst_reset_vcpi(struct intel_encoder *encoder,
> +			     struct drm_connector_state *conn_state,
> +			     int slots);
>  /* intel_dsi.c */
>  void intel_dsi_init(struct drm_i915_private *dev_priv);
>  
> -- 
> 2.7.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/6] drm/dp: Introduce DP MST topology manager state to track DP link bw

[Daniel Vetter]

On Tue, Jan 03, 2017 at 01:01:49PM -0800, Dhinakaran Pandiyan wrote:
> Link bandwidth is shared between multiple display streams in DP MST
> configurations. The DP MST topology manager structure maintains the shared
> link bandwidth for a primary link directly connected to the GPU. For atomic
> modesetting drivers, checking if there is sufficient link bandwidth for a
> mode needs to be done during the atomic_check phase to avoid failed
> modesets. Let's encsapsulate the available link bw information in a state
> structure so that bw can be allocated and released atomically for each of
> the ports sharing the primary link.
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Overall issue with the patch is that dp helpers now have 2 places where
available_slots is stored: One for atomic drivers in ->state, and the
legacy one. I think it'd be good to rework the legacy codepaths (i.e.
drm_dp_find_vcpi_slots) to use mgr->state->avail_slots, and remove
mgr->avail_slots entirely.

Another design concern below, but in principle this looks like what we
need.
-Daniel


> ---
>  drivers/gpu/drm/drm_atomic.c          | 66 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c   | 10 ++++++
>  drivers/gpu/drm/drm_dp_mst_topology.c | 10 ++++++
>  include/drm/drm_atomic.h              | 13 +++++++
>  include/drm/drm_dp_mst_helper.h       | 13 +++++++
>  5 files changed, 112 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 681d5f9..b8e2cea 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -31,6 +31,7 @@
>  #include <drm/drm_mode.h>
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_print.h>
> +#include <drm/drm_dp_mst_helper.h>
>  #include <linux/sync_file.h>
>  
>  #include "drm_crtc_internal.h"
> @@ -62,6 +63,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state)
>  	kfree(state->connectors);
>  	kfree(state->crtcs);
>  	kfree(state->planes);
> +	kfree(state->dp_mst_topologies);
>  }
>  EXPORT_SYMBOL(drm_atomic_state_default_release);
>  
> @@ -189,6 +191,18 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>  		state->planes[i].ptr = NULL;
>  		state->planes[i].state = NULL;
>  	}
> +
> +	for (i = 0; i < state->num_mst_topologies; i++) {
> +		struct drm_dp_mst_topology_mgr *mgr = state->dp_mst_topologies[i].ptr;
> +
> +		if (!mgr)
> +			continue;
> +
> +		kfree(state->dp_mst_topologies[i].state);
> +		state->dp_mst_topologies[i].ptr = NULL;
> +		state->dp_mst_topologies[i].state = NULL;
> +	}
> +
>  }
>  EXPORT_SYMBOL(drm_atomic_state_default_clear);
>  
> @@ -981,6 +995,58 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
>  		plane->funcs->atomic_print_state(p, state);
>  }
>  
> +struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
> +		struct drm_dp_mst_topology_mgr *mgr)
> +{
> +
> +	int ret, i;
> +	size_t new_size;
> +	struct __drm_dp_mst_topology_state *new_arr;
> +	struct drm_dp_mst_topology_state *new_mst_state;
> +	int num_topologies;
> +	struct drm_mode_config *config = &mgr->dev->mode_config;
> +
> +	WARN_ON(!state->acquire_ctx);
> +
> +	ret = drm_modeset_lock(&config->connection_mutex, state->acquire_ctx);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	for (i = 0; i < state->num_mst_topologies; i++) {
> +		if (mgr == state->dp_mst_topologies[i].ptr &&
> +		    state->dp_mst_topologies[i].state) {
> +			return state->dp_mst_topologies[i].state;
> +		}
> +	}
> +
> +	num_topologies = state->num_mst_topologies + 1;
> +	new_size = sizeof(*state->dp_mst_topologies) * num_topologies;
> +	new_arr = krealloc(state->dp_mst_topologies, new_size, GFP_KERNEL);
> +	if (!new_arr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	state->dp_mst_topologies = new_arr;
> +	memset(&state->dp_mst_topologies[state->num_mst_topologies], 0,
> +		sizeof(*state->dp_mst_topologies));
> +
> +	new_mst_state = kmalloc(sizeof(*mgr->state), GFP_KERNEL);
> +	if (!new_mst_state)
> +		return ERR_PTR(-ENOMEM);
> +
> +	new_mst_state->avail_slots = mgr->state->avail_slots;
> +	state->dp_mst_topologies[state->num_mst_topologies].state = new_mst_state;
> +	state->dp_mst_topologies[state->num_mst_topologies].ptr = mgr;
> +	state->num_mst_topologies = num_topologies;
> +	new_mst_state->mgr = mgr;
> +	mgr->state->state = state;
> +
> +	DRM_DEBUG_ATOMIC("Added [MST Topology w/ base connector:%d] %p state to %p\n",
> +			 mgr->conn_base_id, new_mst_state, state);
> +
> +	return new_mst_state;
> +}
> +EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
> +
>  /**
>   * drm_atomic_get_connector_state - get connector state
>   * @state: global atomic state object
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 5e52244..1cd38b7 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -30,6 +30,7 @@
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_dp_mst_helper.h>
>  #include <linux/dma-fence.h>
>  
>  #include "drm_crtc_internal.h"
> @@ -1992,6 +1993,15 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  		connector->state->state = NULL;
>  	}
>  
> +	for (i = 0; i < state->num_mst_topologies; i++) {
> +		struct drm_dp_mst_topology_mgr *mgr;
> +
> +		mgr = state->dp_mst_topologies[i].ptr;
> +		mgr->state->state = state;
> +		swap(state->dp_mst_topologies[i].state, mgr->state);
> +		mgr->state->state = NULL;
> +	}

I'm not sure we want to tie the dp mst helpers that closely with the
atomic machinery, otoh it makes things a lot easier. My concern is that
drm_atomic_state is a core structure, whereas the dp_mst stuff is a
helper, which means we have a control inversion here compared to the
normal pattern.

> +
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  		crtc->state->state = state;
>  		swap(state->crtcs[i].state, crtc->state);
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index d42a6c0..1be19e1 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2042,6 +2042,9 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
>  			goto out_unlock;
>  		}
>  
> +		/* max. time slots - one slot for MTP header */
> +		mgr->state->avail_slots = 63;
> +
>  		/* add initial branch device at LCT 1 */
>  		mstb = drm_dp_add_mst_branch_device(1, NULL);
>  		if (mstb == NULL) {
> @@ -2973,6 +2976,11 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
>  	mgr->proposed_vcpis = kcalloc(max_payloads, sizeof(struct drm_dp_vcpi *), GFP_KERNEL);
>  	if (!mgr->proposed_vcpis)
>  		return -ENOMEM;
> +	mgr->state = kzalloc(sizeof(*mgr->state), GFP_KERNEL);
> +	if (!mgr->state)
> +		return -ENOMEM;
> +	mgr->state->mgr = mgr;
> +
>  	set_bit(0, &mgr->payload_mask);
>  	if (test_calc_pbn_mode() < 0)
>  		DRM_ERROR("MST PBN self-test failed\n");
> @@ -2995,6 +3003,8 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr)
>  	kfree(mgr->proposed_vcpis);
>  	mgr->proposed_vcpis = NULL;
>  	mutex_unlock(&mgr->payload_lock);
> +	kfree(mgr->state);
> +	mgr->state = NULL;
>  	mgr->dev = NULL;
>  	mgr->aux = NULL;
>  }
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index fd2d971..7ac5ed6 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -153,6 +153,11 @@ struct __drm_connnectors_state {
>  	struct drm_connector_state *state;
>  };
>  
> +struct __drm_dp_mst_topology_state {
> +	struct drm_dp_mst_topology_mgr *ptr;
> +	struct drm_dp_mst_topology_state *state;

One way to fix that control inversion I mentioned above is to use void*
pionters here, and then have callbacks for atomic_destroy and swap_state
on top. A bit more shuffling, but we could then use that for other driver
private objects.

Other option is to stuff it into intel_atomic_state.

> +};
> +
>  /**
>   * struct drm_atomic_state - the global state object for atomic updates
>   * @ref: count of all references to this state (will not be freed until zero)
> @@ -164,6 +169,8 @@ struct __drm_connnectors_state {
>   * @crtcs: pointer to array of CRTC pointers
>   * @num_connector: size of the @connectors and @connector_states arrays
>   * @connectors: pointer to array of structures with per-connector data
> + * @num_mst_topologies: size of the @dp_mst_topologies array
> + * @dp_mst_topologies: pointer to array of structures with per-MST topology manager data
>   * @acquire_ctx: acquire context for this atomic modeset state update
>   */
>  struct drm_atomic_state {
> @@ -177,6 +184,8 @@ struct drm_atomic_state {
>  	struct __drm_crtcs_state *crtcs;
>  	int num_connector;
>  	struct __drm_connnectors_state *connectors;
> +	int num_mst_topologies;
> +	struct __drm_dp_mst_topology_state *dp_mst_topologies;
>  
>  	struct drm_modeset_acquire_ctx *acquire_ctx;
>  
> @@ -250,6 +259,10 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
>  		struct drm_connector_state *state, struct drm_property *property,
>  		uint64_t val);
>  
> +struct drm_dp_mst_topology_state * __must_check
> +drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
> +				  struct drm_dp_mst_topology_mgr *mgr);
> +
>  /**
>   * drm_atomic_get_existing_crtc_state - get crtc state, if it exists
>   * @state: global atomic state object
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 98d3c73..0a9bf20 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -403,6 +403,12 @@ struct drm_dp_payload {
>  	int vcpi;
>  };
>  
> +struct drm_dp_mst_topology_state {
> +	int avail_slots;
> +	struct drm_atomic_state *state;
> +	struct drm_dp_mst_topology_mgr *mgr;
> +};
> +
>  /**
>   * struct drm_dp_mst_topology_mgr - DisplayPort MST manager
>   *
> @@ -481,6 +487,11 @@ struct drm_dp_mst_topology_mgr {
>  	int pbn_div;
>  
>  	/**
> +	 *  @state: MST topology manager state for atomic modesetting drivers
> +	 */
> +	struct drm_dp_mst_topology_state *state;
> +
> +	/**
>  	 * @qlock: protects @tx_msg_downq, the tx_slots in struct
>  	 * &drm_dp_mst_branch and txmsg->state once they are queued
>  	 */
> @@ -596,4 +607,6 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
>  
>  void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
>  int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr);
> +struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
> +					struct drm_dp_mst_topology_mgr *mgr);
>  #endif
> -- 
> 2.7.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/6] Introduce DP MST Topology state

[Daniel Vetter]

On Tue, Jan 03, 2017 at 01:01:45PM -0800, Dhinakaran Pandiyan wrote:
> Link bandwidth is shared between multiple display streams in DP MST
> configurations. The DP MST topology manager structure maintains the shared
> link bandwidth for a primary link directly connected to the GPU. For atomic
> modesetting drivers, checking if there is sufficient link bandwidth for a
> mode needs to be done during the atomic_check phase to avoid failed
> modesets.
> 
> This series adds dp_mst_topology_state to track available link bw for
> atomic modesets and MST helpers to find and release link bw in terms of
> vcpi time slots. Using the new helpers is optional and the changes should
> not affect drivers that don't support atomic modesetting. I have made
> changes to i915 to use the new helpers, but this should be applicable to
> nouveau as well.
> 
> Patches 1-3/6 include cleanups and refactoring.
> Patch 4/6 adds the MST topology state, 5/6 adds helpers to alter the state
> and 6/6 contains i915 changes to use the helpers .

Please also cc amdgpu and nouveau maintainers on these patches, they both
will need it.
-Daniel

> 
> Dhinakaran Pandiyan (6):
>   drm/dp: Store drm_device in MST topology manager
>   drm/dp: Kill unused MST vcpi slot availability tracking
>   drm/dp: Split drm_dp_mst_allocate_vcpi
>   drm/dp: Introduce DP MST topology manager state to track DP link bw
>   drm/dp: Add DP MST helpers to atomically find and release vcpi slots
>   drm/i915/dp: Track available DP MST vcpi time slots
> 
>  drivers/gpu/drm/drm_atomic.c           | 66 +++++++++++++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c    | 10 ++++
>  drivers/gpu/drm/drm_dp_mst_topology.c  | 89 ++++++++++++++++++++++++++--------
>  drivers/gpu/drm/i915/intel_display.c   | 39 ++++++++++++++-
>  drivers/gpu/drm/i915/intel_dp_mst.c    | 42 ++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h       |  3 ++
>  drivers/gpu/drm/nouveau/nv50_display.c |  5 +-
>  drivers/gpu/drm/radeon/radeon_dp_mst.c |  5 +-
>  include/drm/drm_atomic.h               | 11 +++++
>  include/drm/drm_dp_mst_helper.h        | 35 ++++++++-----
>  10 files changed, 263 insertions(+), 42 deletions(-)
> 
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/6] drm/dp: Introduce DP MST topology manager state to track DP link bw

[Pandiyan, Dhinakaran]

On Wed, 2017-01-04 at 10:33 +0100, Daniel Vetter wrote:
> On Tue, Jan 03, 2017 at 01:01:49PM -0800, Dhinakaran Pandiyan wrote:
> > Link bandwidth is shared between multiple display streams in DP MST
> > configurations. The DP MST topology manager structure maintains the shared
> > link bandwidth for a primary link directly connected to the GPU. For atomic
> > modesetting drivers, checking if there is sufficient link bandwidth for a
> > mode needs to be done during the atomic_check phase to avoid failed
> > modesets. Let's encsapsulate the available link bw information in a state
> > structure so that bw can be allocated and released atomically for each of
> > the ports sharing the primary link.
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> Overall issue with the patch is that dp helpers now have 2 places where
> available_slots is stored: One for atomic drivers in ->state, and the
> legacy one. I think it'd be good to rework the legacy codepaths (i.e.
> drm_dp_find_vcpi_slots) to use mgr->state->avail_slots, and remove
> mgr->avail_slots entirely.

PATCH 2/6 does that. mgr->avail_slots is not updated in the legacy code
path, so the check turns out to be against mgr->total_slots. So, I did
just that, albeit explicitly. 

-DK

> 
> Another design concern below, but in principle this looks like what we
> need.
> -Daniel
> 
> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c          | 66 +++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_atomic_helper.c   | 10 ++++++
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 10 ++++++
> >  include/drm/drm_atomic.h              | 13 +++++++
> >  include/drm/drm_dp_mst_helper.h       | 13 +++++++
> >  5 files changed, 112 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 681d5f9..b8e2cea 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -31,6 +31,7 @@
> >  #include <drm/drm_mode.h>
> >  #include <drm/drm_plane_helper.h>
> >  #include <drm/drm_print.h>
> > +#include <drm/drm_dp_mst_helper.h>
> >  #include <linux/sync_file.h>
> >  
> >  #include "drm_crtc_internal.h"
> > @@ -62,6 +63,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state)
> >  	kfree(state->connectors);
> >  	kfree(state->crtcs);
> >  	kfree(state->planes);
> > +	kfree(state->dp_mst_topologies);
> >  }
> >  EXPORT_SYMBOL(drm_atomic_state_default_release);
> >  
> > @@ -189,6 +191,18 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> >  		state->planes[i].ptr = NULL;
> >  		state->planes[i].state = NULL;
> >  	}
> > +
> > +	for (i = 0; i < state->num_mst_topologies; i++) {
> > +		struct drm_dp_mst_topology_mgr *mgr = state->dp_mst_topologies[i].ptr;
> > +
> > +		if (!mgr)
> > +			continue;
> > +
> > +		kfree(state->dp_mst_topologies[i].state);
> > +		state->dp_mst_topologies[i].ptr = NULL;
> > +		state->dp_mst_topologies[i].state = NULL;
> > +	}
> > +
> >  }
> >  EXPORT_SYMBOL(drm_atomic_state_default_clear);
> >  
> > @@ -981,6 +995,58 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
> >  		plane->funcs->atomic_print_state(p, state);
> >  }
> >  
> > +struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
> > +		struct drm_dp_mst_topology_mgr *mgr)
> > +{
> > +
> > +	int ret, i;
> > +	size_t new_size;
> > +	struct __drm_dp_mst_topology_state *new_arr;
> > +	struct drm_dp_mst_topology_state *new_mst_state;
> > +	int num_topologies;
> > +	struct drm_mode_config *config = &mgr->dev->mode_config;
> > +
> > +	WARN_ON(!state->acquire_ctx);
> > +
> > +	ret = drm_modeset_lock(&config->connection_mutex, state->acquire_ctx);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	for (i = 0; i < state->num_mst_topologies; i++) {
> > +		if (mgr == state->dp_mst_topologies[i].ptr &&
> > +		    state->dp_mst_topologies[i].state) {
> > +			return state->dp_mst_topologies[i].state;
> > +		}
> > +	}
> > +
> > +	num_topologies = state->num_mst_topologies + 1;
> > +	new_size = sizeof(*state->dp_mst_topologies) * num_topologies;
> > +	new_arr = krealloc(state->dp_mst_topologies, new_size, GFP_KERNEL);
> > +	if (!new_arr)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	state->dp_mst_topologies = new_arr;
> > +	memset(&state->dp_mst_topologies[state->num_mst_topologies], 0,
> > +		sizeof(*state->dp_mst_topologies));
> > +
> > +	new_mst_state = kmalloc(sizeof(*mgr->state), GFP_KERNEL);
> > +	if (!new_mst_state)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	new_mst_state->avail_slots = mgr->state->avail_slots;
> > +	state->dp_mst_topologies[state->num_mst_topologies].state = new_mst_state;
> > +	state->dp_mst_topologies[state->num_mst_topologies].ptr = mgr;
> > +	state->num_mst_topologies = num_topologies;
> > +	new_mst_state->mgr = mgr;
> > +	mgr->state->state = state;
> > +
> > +	DRM_DEBUG_ATOMIC("Added [MST Topology w/ base connector:%d] %p state to %p\n",
> > +			 mgr->conn_base_id, new_mst_state, state);
> > +
> > +	return new_mst_state;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
> > +
> >  /**
> >   * drm_atomic_get_connector_state - get connector state
> >   * @state: global atomic state object
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 5e52244..1cd38b7 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -30,6 +30,7 @@
> >  #include <drm/drm_plane_helper.h>
> >  #include <drm/drm_crtc_helper.h>
> >  #include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_dp_mst_helper.h>
> >  #include <linux/dma-fence.h>
> >  
> >  #include "drm_crtc_internal.h"
> > @@ -1992,6 +1993,15 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> >  		connector->state->state = NULL;
> >  	}
> >  
> > +	for (i = 0; i < state->num_mst_topologies; i++) {
> > +		struct drm_dp_mst_topology_mgr *mgr;
> > +
> > +		mgr = state->dp_mst_topologies[i].ptr;
> > +		mgr->state->state = state;
> > +		swap(state->dp_mst_topologies[i].state, mgr->state);
> > +		mgr->state->state = NULL;
> > +	}
> 
> I'm not sure we want to tie the dp mst helpers that closely with the
> atomic machinery, otoh it makes things a lot easier. My concern is that
> drm_atomic_state is a core structure, whereas the dp_mst stuff is a
> helper, which means we have a control inversion here compared to the
> normal pattern.
> 
> > +
> >  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> >  		crtc->state->state = state;
> >  		swap(state->crtcs[i].state, crtc->state);
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index d42a6c0..1be19e1 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2042,6 +2042,9 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
> >  			goto out_unlock;
> >  		}
> >  
> > +		/* max. time slots - one slot for MTP header */
> > +		mgr->state->avail_slots = 63;
> > +
> >  		/* add initial branch device at LCT 1 */
> >  		mstb = drm_dp_add_mst_branch_device(1, NULL);
> >  		if (mstb == NULL) {
> > @@ -2973,6 +2976,11 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
> >  	mgr->proposed_vcpis = kcalloc(max_payloads, sizeof(struct drm_dp_vcpi *), GFP_KERNEL);
> >  	if (!mgr->proposed_vcpis)
> >  		return -ENOMEM;
> > +	mgr->state = kzalloc(sizeof(*mgr->state), GFP_KERNEL);
> > +	if (!mgr->state)
> > +		return -ENOMEM;
> > +	mgr->state->mgr = mgr;
> > +
> >  	set_bit(0, &mgr->payload_mask);
> >  	if (test_calc_pbn_mode() < 0)
> >  		DRM_ERROR("MST PBN self-test failed\n");
> > @@ -2995,6 +3003,8 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr)
> >  	kfree(mgr->proposed_vcpis);
> >  	mgr->proposed_vcpis = NULL;
> >  	mutex_unlock(&mgr->payload_lock);
> > +	kfree(mgr->state);
> > +	mgr->state = NULL;
> >  	mgr->dev = NULL;
> >  	mgr->aux = NULL;
> >  }
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index fd2d971..7ac5ed6 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -153,6 +153,11 @@ struct __drm_connnectors_state {
> >  	struct drm_connector_state *state;
> >  };
> >  
> > +struct __drm_dp_mst_topology_state {
> > +	struct drm_dp_mst_topology_mgr *ptr;
> > +	struct drm_dp_mst_topology_state *state;
> 
> One way to fix that control inversion I mentioned above is to use void*
> pionters here, and then have callbacks for atomic_destroy and swap_state
> on top. A bit more shuffling, but we could then use that for other driver
> private objects.
> 
> Other option is to stuff it into intel_atomic_state.
> 
> > +};
> > +
> >  /**
> >   * struct drm_atomic_state - the global state object for atomic updates
> >   * @ref: count of all references to this state (will not be freed until zero)
> > @@ -164,6 +169,8 @@ struct __drm_connnectors_state {
> >   * @crtcs: pointer to array of CRTC pointers
> >   * @num_connector: size of the @connectors and @connector_states arrays
> >   * @connectors: pointer to array of structures with per-connector data
> > + * @num_mst_topologies: size of the @dp_mst_topologies array
> > + * @dp_mst_topologies: pointer to array of structures with per-MST topology manager data
> >   * @acquire_ctx: acquire context for this atomic modeset state update
> >   */
> >  struct drm_atomic_state {
> > @@ -177,6 +184,8 @@ struct drm_atomic_state {
> >  	struct __drm_crtcs_state *crtcs;
> >  	int num_connector;
> >  	struct __drm_connnectors_state *connectors;
> > +	int num_mst_topologies;
> > +	struct __drm_dp_mst_topology_state *dp_mst_topologies;
> >  
> >  	struct drm_modeset_acquire_ctx *acquire_ctx;
> >  
> > @@ -250,6 +259,10 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
> >  		struct drm_connector_state *state, struct drm_property *property,
> >  		uint64_t val);
> >  
> > +struct drm_dp_mst_topology_state * __must_check
> > +drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
> > +				  struct drm_dp_mst_topology_mgr *mgr);
> > +
> >  /**
> >   * drm_atomic_get_existing_crtc_state - get crtc state, if it exists
> >   * @state: global atomic state object
> > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> > index 98d3c73..0a9bf20 100644
> > --- a/include/drm/drm_dp_mst_helper.h
> > +++ b/include/drm/drm_dp_mst_helper.h
> > @@ -403,6 +403,12 @@ struct drm_dp_payload {
> >  	int vcpi;
> >  };
> >  
> > +struct drm_dp_mst_topology_state {
> > +	int avail_slots;
> > +	struct drm_atomic_state *state;
> > +	struct drm_dp_mst_topology_mgr *mgr;
> > +};
> > +
> >  /**
> >   * struct drm_dp_mst_topology_mgr - DisplayPort MST manager
> >   *
> > @@ -481,6 +487,11 @@ struct drm_dp_mst_topology_mgr {
> >  	int pbn_div;
> >  
> >  	/**
> > +	 *  @state: MST topology manager state for atomic modesetting drivers
> > +	 */
> > +	struct drm_dp_mst_topology_state *state;
> > +
> > +	/**
> >  	 * @qlock: protects @tx_msg_downq, the tx_slots in struct
> >  	 * &drm_dp_mst_branch and txmsg->state once they are queued
> >  	 */
> > @@ -596,4 +607,6 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
> >  
> >  void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
> >  int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr);
> > +struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
> > +					struct drm_dp_mst_topology_mgr *mgr);
> >  #endif
> > -- 
> > 2.7.4
> > 
> 

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

Re: [PATCH 4/6] drm/dp: Introduce DP MST topology manager state to track DP link bw

[Pandiyan, Dhinakaran]

On Wed, 2017-01-04 at 19:20 +0000, Pandiyan, Dhinakaran wrote:
> On Wed, 2017-01-04 at 10:33 +0100, Daniel Vetter wrote:
> > On Tue, Jan 03, 2017 at 01:01:49PM -0800, Dhinakaran Pandiyan wrote:
> > > Link bandwidth is shared between multiple display streams in DP MST
> > > configurations. The DP MST topology manager structure maintains the shared
> > > link bandwidth for a primary link directly connected to the GPU. For atomic
> > > modesetting drivers, checking if there is sufficient link bandwidth for a
> > > mode needs to be done during the atomic_check phase to avoid failed
> > > modesets. Let's encsapsulate the available link bw information in a state
> > > structure so that bw can be allocated and released atomically for each of
> > > the ports sharing the primary link.
> > > 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > 
> > Overall issue with the patch is that dp helpers now have 2 places where
> > available_slots is stored: One for atomic drivers in ->state, and the
> > legacy one. I think it'd be good to rework the legacy codepaths (i.e.
> > drm_dp_find_vcpi_slots) to use mgr->state->avail_slots, and remove
> > mgr->avail_slots entirely.
> 
> PATCH 2/6 does that. mgr->avail_slots is not updated in the legacy code
> path, so the check turns out to be against mgr->total_slots. So, I did
> just that, albeit explicitly. 
> 
> -DK
> 
> > 
> > Another design concern below, but in principle this looks like what we
> > need.
> > -Daniel
> > 
> > 
> > > ---
> > >  drivers/gpu/drm/drm_atomic.c          | 66 +++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/drm_atomic_helper.c   | 10 ++++++
> > >  drivers/gpu/drm/drm_dp_mst_topology.c | 10 ++++++
> > >  include/drm/drm_atomic.h              | 13 +++++++
> > >  include/drm/drm_dp_mst_helper.h       | 13 +++++++
> > >  5 files changed, 112 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index 681d5f9..b8e2cea 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -31,6 +31,7 @@
> > >  #include <drm/drm_mode.h>
> > >  #include <drm/drm_plane_helper.h>
> > >  #include <drm/drm_print.h>
> > > +#include <drm/drm_dp_mst_helper.h>
> > >  #include <linux/sync_file.h>
> > >  
> > >  #include "drm_crtc_internal.h"
> > > @@ -62,6 +63,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state)
> > >  	kfree(state->connectors);
> > >  	kfree(state->crtcs);
> > >  	kfree(state->planes);
> > > +	kfree(state->dp_mst_topologies);
> > >  }
> > >  EXPORT_SYMBOL(drm_atomic_state_default_release);
> > >  
> > > @@ -189,6 +191,18 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> > >  		state->planes[i].ptr = NULL;
> > >  		state->planes[i].state = NULL;
> > >  	}
> > > +
> > > +	for (i = 0; i < state->num_mst_topologies; i++) {
> > > +		struct drm_dp_mst_topology_mgr *mgr = state->dp_mst_topologies[i].ptr;
> > > +
> > > +		if (!mgr)
> > > +			continue;
> > > +
> > > +		kfree(state->dp_mst_topologies[i].state);
> > > +		state->dp_mst_topologies[i].ptr = NULL;
> > > +		state->dp_mst_topologies[i].state = NULL;
> > > +	}
> > > +
> > >  }
> > >  EXPORT_SYMBOL(drm_atomic_state_default_clear);
> > >  
> > > @@ -981,6 +995,58 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
> > >  		plane->funcs->atomic_print_state(p, state);
> > >  }
> > >  
> > > +struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
> > > +		struct drm_dp_mst_topology_mgr *mgr)
> > > +{
> > > +
> > > +	int ret, i;
> > > +	size_t new_size;
> > > +	struct __drm_dp_mst_topology_state *new_arr;
> > > +	struct drm_dp_mst_topology_state *new_mst_state;
> > > +	int num_topologies;
> > > +	struct drm_mode_config *config = &mgr->dev->mode_config;
> > > +
> > > +	WARN_ON(!state->acquire_ctx);
> > > +
> > > +	ret = drm_modeset_lock(&config->connection_mutex, state->acquire_ctx);
> > > +	if (ret)
> > > +		return ERR_PTR(ret);
> > > +
> > > +	for (i = 0; i < state->num_mst_topologies; i++) {
> > > +		if (mgr == state->dp_mst_topologies[i].ptr &&
> > > +		    state->dp_mst_topologies[i].state) {
> > > +			return state->dp_mst_topologies[i].state;
> > > +		}
> > > +	}
> > > +
> > > +	num_topologies = state->num_mst_topologies + 1;
> > > +	new_size = sizeof(*state->dp_mst_topologies) * num_topologies;
> > > +	new_arr = krealloc(state->dp_mst_topologies, new_size, GFP_KERNEL);
> > > +	if (!new_arr)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	state->dp_mst_topologies = new_arr;
> > > +	memset(&state->dp_mst_topologies[state->num_mst_topologies], 0,
> > > +		sizeof(*state->dp_mst_topologies));
> > > +
> > > +	new_mst_state = kmalloc(sizeof(*mgr->state), GFP_KERNEL);
> > > +	if (!new_mst_state)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	new_mst_state->avail_slots = mgr->state->avail_slots;
> > > +	state->dp_mst_topologies[state->num_mst_topologies].state = new_mst_state;
> > > +	state->dp_mst_topologies[state->num_mst_topologies].ptr = mgr;
> > > +	state->num_mst_topologies = num_topologies;
> > > +	new_mst_state->mgr = mgr;
> > > +	mgr->state->state = state;
> > > +
> > > +	DRM_DEBUG_ATOMIC("Added [MST Topology w/ base connector:%d] %p state to %p\n",
> > > +			 mgr->conn_base_id, new_mst_state, state);
> > > +
> > > +	return new_mst_state;
> > > +}
> > > +EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
> > > +
> > >  /**
> > >   * drm_atomic_get_connector_state - get connector state
> > >   * @state: global atomic state object
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 5e52244..1cd38b7 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -30,6 +30,7 @@
> > >  #include <drm/drm_plane_helper.h>
> > >  #include <drm/drm_crtc_helper.h>
> > >  #include <drm/drm_atomic_helper.h>
> > > +#include <drm/drm_dp_mst_helper.h>
> > >  #include <linux/dma-fence.h>
> > >  
> > >  #include "drm_crtc_internal.h"
> > > @@ -1992,6 +1993,15 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> > >  		connector->state->state = NULL;
> > >  	}
> > >  
> > > +	for (i = 0; i < state->num_mst_topologies; i++) {
> > > +		struct drm_dp_mst_topology_mgr *mgr;
> > > +
> > > +		mgr = state->dp_mst_topologies[i].ptr;
> > > +		mgr->state->state = state;
> > > +		swap(state->dp_mst_topologies[i].state, mgr->state);
> > > +		mgr->state->state = NULL;
> > > +	}
> > 
> > I'm not sure we want to tie the dp mst helpers that closely with the
> > atomic machinery, otoh it makes things a lot easier. My concern is that
> > drm_atomic_state is a core structure, whereas the dp_mst stuff is a
> > helper, which means we have a control inversion here compared to the
> > normal pattern.
> > 
> > > +
> > >  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > >  		crtc->state->state = state;
> > >  		swap(state->crtcs[i].state, crtc->state);
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index d42a6c0..1be19e1 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -2042,6 +2042,9 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
> > >  			goto out_unlock;
> > >  		}
> > >  
> > > +		/* max. time slots - one slot for MTP header */
> > > +		mgr->state->avail_slots = 63;
> > > +
> > >  		/* add initial branch device at LCT 1 */
> > >  		mstb = drm_dp_add_mst_branch_device(1, NULL);
> > >  		if (mstb == NULL) {
> > > @@ -2973,6 +2976,11 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
> > >  	mgr->proposed_vcpis = kcalloc(max_payloads, sizeof(struct drm_dp_vcpi *), GFP_KERNEL);
> > >  	if (!mgr->proposed_vcpis)
> > >  		return -ENOMEM;
> > > +	mgr->state = kzalloc(sizeof(*mgr->state), GFP_KERNEL);
> > > +	if (!mgr->state)
> > > +		return -ENOMEM;
> > > +	mgr->state->mgr = mgr;
> > > +
> > >  	set_bit(0, &mgr->payload_mask);
> > >  	if (test_calc_pbn_mode() < 0)
> > >  		DRM_ERROR("MST PBN self-test failed\n");
> > > @@ -2995,6 +3003,8 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr)
> > >  	kfree(mgr->proposed_vcpis);
> > >  	mgr->proposed_vcpis = NULL;
> > >  	mutex_unlock(&mgr->payload_lock);
> > > +	kfree(mgr->state);
> > > +	mgr->state = NULL;
> > >  	mgr->dev = NULL;
> > >  	mgr->aux = NULL;
> > >  }
> > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > > index fd2d971..7ac5ed6 100644
> > > --- a/include/drm/drm_atomic.h
> > > +++ b/include/drm/drm_atomic.h
> > > @@ -153,6 +153,11 @@ struct __drm_connnectors_state {
> > >  	struct drm_connector_state *state;
> > >  };
> > >  
> > > +struct __drm_dp_mst_topology_state {
> > > +	struct drm_dp_mst_topology_mgr *ptr;
> > > +	struct drm_dp_mst_topology_state *state;
> > 
> > One way to fix that control inversion I mentioned above is to use void*
> > pionters here, and then have callbacks for atomic_destroy and swap_state
> > on top. A bit more shuffling, but we could then use that for other driver
> > private objects.
> > 
> > Other option is to stuff it into intel_atomic_state.
> > 

Hmm... I think I understand what you are saying. The core atomic
functions like swap_state should not be able alter the topology
manager's current state?

Did you mean something like this - https://paste.ubuntu.com/23743485/ ?

Do we need the destroy callback too? drm_atomic_state_default_clear()
does not have to dereference drm_dp_mst_topology_mgr.



Moving this to intel_atomic_state would be mean that nouveau will
require a re-implementation. Is that right?


-DK


> > > +};
> > > +
> > >  /**
> > >   * struct drm_atomic_state - the global state object for atomic updates
> > >   * @ref: count of all references to this state (will not be freed until zero)
> > > @@ -164,6 +169,8 @@ struct __drm_connnectors_state {
> > >   * @crtcs: pointer to array of CRTC pointers
> > >   * @num_connector: size of the @connectors and @connector_states arrays
> > >   * @connectors: pointer to array of structures with per-connector data
> > > + * @num_mst_topologies: size of the @dp_mst_topologies array
> > > + * @dp_mst_topologies: pointer to array of structures with per-MST topology manager data
> > >   * @acquire_ctx: acquire context for this atomic modeset state update
> > >   */
> > >  struct drm_atomic_state {
> > > @@ -177,6 +184,8 @@ struct drm_atomic_state {
> > >  	struct __drm_crtcs_state *crtcs;
> > >  	int num_connector;
> > >  	struct __drm_connnectors_state *connectors;
> > > +	int num_mst_topologies;
> > > +	struct __drm_dp_mst_topology_state *dp_mst_topologies;
> > >  
> > >  	struct drm_modeset_acquire_ctx *acquire_ctx;
> > >  
> > > @@ -250,6 +259,10 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
> > >  		struct drm_connector_state *state, struct drm_property *property,
> > >  		uint64_t val);
> > >  
> > > +struct drm_dp_mst_topology_state * __must_check
> > > +drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
> > > +				  struct drm_dp_mst_topology_mgr *mgr);
> > > +
> > >  /**
> > >   * drm_atomic_get_existing_crtc_state - get crtc state, if it exists
> > >   * @state: global atomic state object
> > > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> > > index 98d3c73..0a9bf20 100644
> > > --- a/include/drm/drm_dp_mst_helper.h
> > > +++ b/include/drm/drm_dp_mst_helper.h
> > > @@ -403,6 +403,12 @@ struct drm_dp_payload {
> > >  	int vcpi;
> > >  };
> > >  
> > > +struct drm_dp_mst_topology_state {
> > > +	int avail_slots;
> > > +	struct drm_atomic_state *state;
> > > +	struct drm_dp_mst_topology_mgr *mgr;
> > > +};
> > > +
> > >  /**
> > >   * struct drm_dp_mst_topology_mgr - DisplayPort MST manager
> > >   *
> > > @@ -481,6 +487,11 @@ struct drm_dp_mst_topology_mgr {
> > >  	int pbn_div;
> > >  
> > >  	/**
> > > +	 *  @state: MST topology manager state for atomic modesetting drivers
> > > +	 */
> > > +	struct drm_dp_mst_topology_state *state;
> > > +
> > > +	/**
> > >  	 * @qlock: protects @tx_msg_downq, the tx_slots in struct
> > >  	 * &drm_dp_mst_branch and txmsg->state once they are queued
> > >  	 */
> > > @@ -596,4 +607,6 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
> > >  
> > >  void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
> > >  int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr);
> > > +struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
> > > +					struct drm_dp_mst_topology_mgr *mgr);
> > >  #endif
> > > -- 
> > > 2.7.4
> > > 
> > 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 4/6] drm/dp: Introduce DP MST topology manager state to track DP link bw

[Daniel Vetter]

On Thu, Jan 05, 2017 at 03:54:54AM +0000, Pandiyan, Dhinakaran wrote:
> On Wed, 2017-01-04 at 19:20 +0000, Pandiyan, Dhinakaran wrote:
> > On Wed, 2017-01-04 at 10:33 +0100, Daniel Vetter wrote:
> > > On Tue, Jan 03, 2017 at 01:01:49PM -0800, Dhinakaran Pandiyan wrote:
> > > > Link bandwidth is shared between multiple display streams in DP MST
> > > > configurations. The DP MST topology manager structure maintains the shared
> > > > link bandwidth for a primary link directly connected to the GPU. For atomic
> > > > modesetting drivers, checking if there is sufficient link bandwidth for a
> > > > mode needs to be done during the atomic_check phase to avoid failed
> > > > modesets. Let's encsapsulate the available link bw information in a state
> > > > structure so that bw can be allocated and released atomically for each of
> > > > the ports sharing the primary link.
> > > > 
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > 
> > > Overall issue with the patch is that dp helpers now have 2 places where
> > > available_slots is stored: One for atomic drivers in ->state, and the
> > > legacy one. I think it'd be good to rework the legacy codepaths (i.e.
> > > drm_dp_find_vcpi_slots) to use mgr->state->avail_slots, and remove
> > > mgr->avail_slots entirely.
> > 
> > PATCH 2/6 does that. mgr->avail_slots is not updated in the legacy code
> > path, so the check turns out to be against mgr->total_slots. So, I did
> > just that, albeit explicitly. 

Ah right, I missed that.

> > > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > > > index fd2d971..7ac5ed6 100644
> > > > --- a/include/drm/drm_atomic.h
> > > > +++ b/include/drm/drm_atomic.h
> > > > @@ -153,6 +153,11 @@ struct __drm_connnectors_state {
> > > >  	struct drm_connector_state *state;
> > > >  };
> > > >  
> > > > +struct __drm_dp_mst_topology_state {
> > > > +	struct drm_dp_mst_topology_mgr *ptr;
> > > > +	struct drm_dp_mst_topology_state *state;
> > > 
> > > One way to fix that control inversion I mentioned above is to use void*
> > > pionters here, and then have callbacks for atomic_destroy and swap_state
> > > on top. A bit more shuffling, but we could then use that for other driver
> > > private objects.
> > > 
> > > Other option is to stuff it into intel_atomic_state.
> 
> Hmm... I think I understand what you are saying. The core atomic
> functions like swap_state should not be able alter the topology
> manager's current state?
> 
> Did you mean something like this - https://paste.ubuntu.com/23743485/ ?

Not quite yet, here's what I had in mind as a sketch:

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 2e28fdca9c3d..6ce704b1e900 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -154,6 +154,17 @@ struct __drm_connnectors_state {
 	struct drm_connector_state *state;
 };
 
+struct drm_private_state_funcs {
+	void (*swap)(void *obj, void *state);
+	void (*destroy_state)(void *state);
+};
+
+struct __drm_private_obj_state {
+	struct obj *ptr;
+	struct obj_state *state;
+	struct drm_private_state_funcs *funcs;
+}
+
 /**
  * struct drm_atomic_state - the global state object for atomic updates
  * @ref: count of all references to this state (will not be freed until zero)
@@ -178,6 +189,8 @@ struct drm_atomic_state {
 	struct __drm_crtcs_state *crtcs;
 	int num_connector;
 	struct __drm_connnectors_state *connectors;
+	int num_private_objs;
+	struct __drm_private_obj_state *private_objs;
 
 	struct drm_modeset_acquire_ctx *acquire_ctx;
 
@@ -414,6 +427,19 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
 	     (__i)++)							\
 		for_each_if (plane_state)
 
+/* The magic here is that if obj and obj_state have the right type, then this
+ * will automatically cast to the right type. Since we allow any kind of private
+ * object mixed into the same array, runtime type casting is done using the
+ * funcs pointer.
+ */
+#define for_each_private_obj(__state, obj, obj_state, __i, funcs)
+	for ((__i) = 0;							\
+	     (__i) < (__state)->num_private_objs &&				\
+	     ((obj) = (__state)->private_objs[__i].ptr,			\
+	     (obj_state) = (__state)->private_objs[__i].state, 1); 	\
+	     (__i)++)							\
+		for_each_if ((__state)->private_objs[__i].funcs == (funcs))
+
 /**
  * drm_atomic_crtc_needs_modeset - compute combined modeset need
  * @state: &drm_crtc_state for the CRTC

I didn't sketch helper functions for looking up/inserting objects, and ofc
we'd need the boilerplat for cleanup/swap, but I hope that part is clear.

If we add a duplicate_state callback to drm_private_state_funcs then we
might even be able to abstract away the entire lookup-or-dupliocate logic
into a helper, and all that's left for a specific implementation would be

struct drm_dp_mst_topology_state*
drm_dp_mst_get_atomic_state(struct drm_atomic_state *state,
			    struct dp_mst_topology_mgr *mgr)
{
	return drm_atomic_get_private_state(state, mgr, mgr->state,
					    &dp_mst_state_funcs);
}

The upside of going to all this trouble is that we could reuse this for
all the other driver private state, like dpll, or wm or whatever. And we
wouldn't need to type the same boring boilerplate for all of them, since
that would all be hidden. And since I expect that there will be more&more
use-cases and needs for driver private atomic state for all the fancy
features coming down the pipeline, this might be worth it. But not yet
sure.

> Do we need the destroy callback too? drm_atomic_state_default_clear()
> does not have to dereference drm_dp_mst_topology_mgr.
> 
> Moving this to intel_atomic_state would be mean that nouveau will
> require a re-implementation. Is that right?

Yeah, that's the downside, and I think we could also make other
driver-private objects a bit easier to handle.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx