[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

✓ Fi.CI.BAT: success for Introduce DP MST Topology state (rev2)

[Patchwork]

== Series Details ==

Series: Introduce DP MST Topology state (rev2)
URL   : https://patchwork.freedesktop.org/series/17303/
State : success

== Summary ==

Series 17303v2 Introduce DP MST Topology state
https://patchwork.freedesktop.org/api/1.0/series/17303/revisions/2/mbox/


fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:207  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:82   pass:69   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:222  dwarn:3   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:246  pass:214  dwarn:0   dfail:0   fail:0   skip:32 

47cdc87bc9856ca063e65af0c427bd875c1c3c36 drm-tip: 2017y-01m-03d-20h-53m-04s UTC integration manifest
c238d61 drm/i915/dp: Track available DP MST vcpi time slots
da5bef2 drm/dp: Add DP MST helpers to atomically find and release vcpi slots
cfbd701 drm/dp: Introduce DP MST topology manager state to track DP link bw
62fd297 drm/dp: Split drm_dp_mst_allocate_vcpi
46f1ddd drm/dp: Kill unused MST vcpi slot availability tracking
a42457e drm/dp: Store drm_device in MST topology manager

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3426/
_______________________________________________
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

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

[Pandiyan, Dhinakaran]

On Thu, 2017-01-05 at 09:24 +0100, Daniel Vetter wrote:
> 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;

Thanks for the sketch Daniel, I have a couple of questions.
Should this be void *obj and void *obj_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))
> +

So, this macro iterates through a specific type of private obj in the
array. You are not implying that drm_atomic_helper_swap_state is
expected to use this, right? If we don't do
"((__state)->private_objs[__i].funcs == (funcs))", we could swap the
state for all private objs.



>  /**
>   * 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);
> }
> 

I like this idea, except for the part we have to do a linear search for
lookups.

-DK

> 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

_______________________________________________
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 Sat, Jan 07, 2017 at 12:35:36AM +0000, Pandiyan, Dhinakaran wrote:
> On Thu, 2017-01-05 at 09:24 +0100, Daniel Vetter wrote:
> > 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;
> 
> Thanks for the sketch Daniel, I have a couple of questions.
> Should this be void *obj and void *obj_state? 

Yes :-)

> 
> > +	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))
> > +
> 
> So, this macro iterates through a specific type of private obj in the
> array. You are not implying that drm_atomic_helper_swap_state is
> expected to use this, right? If we don't do
> "((__state)->private_objs[__i].funcs == (funcs))", we could swap the
> state for all private objs.

Yes, swap state should go through all of them and not filter for a
specific funcs. Probably best to have an internal/dangerous version with

#define __drm_for_each_private_obj(__state, obj, obj_state, __i, __funcs)
	for ((__i) = 0;							\
	     (__i) < (__state)->num_private_objs &&			\
	     ((obj) = (__state)->private_objs[__i].ptr,			\
	     (__funcs) =	(__state)->private_objs[__i].funcs,	\
	     (obj_state) = (__state)->private_objs[__i].state, 1); 	\
	     (__i)++)							\
		for_each_if (__funcs)

i.e. instead of filtering for funcs, assign the provided funcs pointer.
Then swap_states and the cleanup functions could use that.


> 
> 
> 
> >  /**
> >   * 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);
> > }
> > 
> 
> I like this idea, except for the part we have to do a linear search for
> lookups.

That's not an issue, since there's very few objects. And if it ever
becomes an issue, we can easily add a hashtable or something for the void
* -> entry lookup to speed it up.

But a good thing to note, please add a comment to the commit message.
-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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 17118 bytes --]

Hi Dhinakaran,

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on v4.10-rc1 next-20161224]
[cannot apply to drm/drm-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Dhinakaran-Pandiyan/Introduce-DP-MST-Topology-state/20161230-151546
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   make[3]: warning: jobserver unavailable: using -j1.  Add '+' to parent make rule.
   include/linux/init.h:1: warning: no structured comments found
   include/linux/workqueue.h:392: warning: No description found for parameter '...'
   include/linux/workqueue.h:392: warning: Excess function parameter 'args' description in 'alloc_workqueue'
   include/linux/workqueue.h:413: warning: No description found for parameter '...'
   include/linux/workqueue.h:413: warning: Excess function parameter 'args' description in 'alloc_ordered_workqueue'
   include/linux/kthread.h:26: warning: No description found for parameter '...'
   kernel/sys.c:1: warning: no structured comments found
   drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found
   include/sound/core.h:324: warning: No description found for parameter '...'
   include/sound/core.h:335: warning: No description found for parameter '...'
   include/sound/core.h:388: warning: No description found for parameter '...'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'load'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'firstopen'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'open'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'preclose'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'postclose'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'lastclose'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'unload'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'dma_ioctl'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'dma_quiescent'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'context_dtor'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'set_busid'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'irq_handler'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'irq_preinstall'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'irq_postinstall'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'irq_uninstall'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'debugfs_init'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'debugfs_cleanup'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'gem_open_object'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'gem_close_object'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'prime_handle_to_fd'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'prime_fd_to_handle'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'gem_prime_export'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'gem_prime_import'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'gem_prime_pin'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'gem_prime_unpin'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'gem_prime_res_obj'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'gem_prime_get_sg_table'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'gem_prime_import_sg_table'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'gem_prime_vmap'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'gem_prime_vunmap'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'gem_prime_mmap'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'vgaarb_irq'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'gem_vm_ops'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'major'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'minor'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'patchlevel'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'name'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'desc'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'date'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'driver_features'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'dev_priv_size'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'ioctls'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'num_ioctls'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'fops'
   include/drm/drm_drv.h:409: warning: No description found for parameter 'legacy_dev_list'
>> include/drm/drm_atomic.h:197: warning: No description found for parameter 'num_mst_topologies'
>> include/drm/drm_atomic.h:197: warning: No description found for parameter 'dp_mst_topologies'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'i915'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'pid'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'name'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'hw_id'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'priority'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'ggtt_alignment'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'ggtt_offset_bias'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'engine[I915_NUM_ENGINES]'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'ring_size'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'desc_template'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'status_notifier'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'execlists_force_single_submission'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'closed'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'bannable'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'banned'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'guilty_count'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'active_count'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'ban_score'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: Excess struct/union/enum/typedef member 'hang_stats' description in 'i915_gem_context'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: Excess struct/union/enum/typedef member 'legacy_hw_ctx' description in 'i915_gem_context'
   drivers/gpu/drm/i915/i915_drv.h:1203: warning: cannot understand function prototype: 'enum drrs_refresh_rate_type '
   drivers/gpu/drm/i915/i915_drv.h:3334: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_drv.h:3334: warning: No description found for parameter 'type'
   drivers/gpu/drm/i915/i915_drv.h:3346: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'i915'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'pid'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'name'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'hw_id'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'priority'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'ggtt_alignment'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'ggtt_offset_bias'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'engine[I915_NUM_ENGINES]'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'ring_size'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'desc_template'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'status_notifier'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'execlists_force_single_submission'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'closed'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'bannable'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'banned'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'guilty_count'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'active_count'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'ban_score'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: Excess struct/union/enum/typedef member 'hang_stats' description in 'i915_gem_context'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: Excess struct/union/enum/typedef member 'legacy_hw_ctx' description in 'i915_gem_context'
   drivers/gpu/drm/i915/i915_drv.h:1203: warning: cannot understand function prototype: 'enum drrs_refresh_rate_type '
   drivers/gpu/drm/i915/i915_drv.h:3334: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_drv.h:3334: warning: No description found for parameter 'type'
   drivers/gpu/drm/i915/i915_drv.h:3346: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'i915'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'pid'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'name'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'hw_id'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'priority'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'ggtt_alignment'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'ggtt_offset_bias'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'engine[I915_NUM_ENGINES]'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'ring_size'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'desc_template'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'status_notifier'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'execlists_force_single_submission'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'closed'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'bannable'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'banned'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'guilty_count'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'active_count'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: No description found for parameter 'ban_score'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: Excess struct/union/enum/typedef member 'hang_stats' description in 'i915_gem_context'
   drivers/gpu/drm/i915/i915_drv.h:1119: warning: Excess struct/union/enum/typedef member 'legacy_hw_ctx' description in 'i915_gem_context'
   drivers/gpu/drm/i915/i915_drv.h:1203: warning: cannot understand function prototype: 'enum drrs_refresh_rate_type '
   drivers/gpu/drm/i915/i915_drv.h:3334: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_drv.h:3334: warning: No description found for parameter 'type'
   drivers/gpu/drm/i915/i915_drv.h:3346: warning: No description found for parameter 'obj'
   include/media/media-entity.h:1054: warning: No description found for parameter '...'
   include/net/mac80211.h:3207: ERROR: Unexpected indentation.
   include/net/mac80211.h:3210: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/net/mac80211.h:3212: ERROR: Unexpected indentation.
   include/net/mac80211.h:3213: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/net/mac80211.h:1772: ERROR: Unexpected indentation.
   include/net/mac80211.h:1776: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/sched/fair.c:7259: WARNING: Inline emphasis start-string without end-string.
   kernel/time/timer.c:1240: ERROR: Unexpected indentation.
   kernel/time/timer.c:1242: ERROR: Unexpected indentation.
   kernel/time/timer.c:1243: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:121: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:124: ERROR: Unexpected indentation.
   include/linux/wait.h:126: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/time/hrtimer.c:1021: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/signal.c:317: WARNING: Inline literal start-string without end-string.
   drivers/base/firmware_class.c:1348: WARNING: Bullet list ends without a blank line; unexpected unindent.
   drivers/message/fusion/mptbase.c:5054: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/tty/serial/serial_core.c:1893: WARNING: Definition list ends without a blank line; unexpected unindent.
   include/linux/spi/spi.h:369: ERROR: Unexpected indentation.
   WARNING: dvipng command 'dvipng' cannot be run (needed for math display), check the imgmath_dvipng setting

vim +/num_mst_topologies +197 include/drm/drm_atomic.h

43968d7b Daniel Vetter       2016-09-21  181  	struct __drm_crtcs_state *crtcs;
43968d7b Daniel Vetter       2016-09-21  182  	int num_connector;
43968d7b Daniel Vetter       2016-09-21  183  	struct __drm_connnectors_state *connectors;
051ada4a Dhinakaran Pandiyan 2016-12-29  184  	int num_mst_topologies;
051ada4a Dhinakaran Pandiyan 2016-12-29  185  	struct __drm_dp_mst_topology_state *dp_mst_topologies;
43968d7b Daniel Vetter       2016-09-21  186  
43968d7b Daniel Vetter       2016-09-21  187  	struct drm_modeset_acquire_ctx *acquire_ctx;
43968d7b Daniel Vetter       2016-09-21  188  
43968d7b Daniel Vetter       2016-09-21  189  	/**
43968d7b Daniel Vetter       2016-09-21  190  	 * @commit_work:
43968d7b Daniel Vetter       2016-09-21  191  	 *
43968d7b Daniel Vetter       2016-09-21  192  	 * Work item which can be used by the driver or helpers to execute the
43968d7b Daniel Vetter       2016-09-21  193  	 * commit without blocking.
43968d7b Daniel Vetter       2016-09-21  194  	 */
43968d7b Daniel Vetter       2016-09-21  195  	struct work_struct commit_work;
43968d7b Daniel Vetter       2016-09-21  196  };
43968d7b Daniel Vetter       2016-09-21 @197  
3b24f7d6 Daniel Vetter       2016-06-08  198  void drm_crtc_commit_put(struct drm_crtc_commit *commit);
3b24f7d6 Daniel Vetter       2016-06-08  199  static inline void drm_crtc_commit_get(struct drm_crtc_commit *commit)
3b24f7d6 Daniel Vetter       2016-06-08  200  {
3b24f7d6 Daniel Vetter       2016-06-08  201  	kref_get(&commit->ref);
3b24f7d6 Daniel Vetter       2016-06-08  202  }
3b24f7d6 Daniel Vetter       2016-06-08  203  
cc4ceb48 Daniel Vetter       2014-07-25  204  struct drm_atomic_state * __must_check
cc4ceb48 Daniel Vetter       2014-07-25  205  drm_atomic_state_alloc(struct drm_device *dev);

:::::: The code at line 197 was first introduced by commit
:::::: 43968d7b806d7a7e021261294c583a216fddf0e5 drm: Extract drm_plane.[hc]

:::::: TO: Daniel Vetter <daniel.vetter@ffwll.ch>
:::::: CC: Sean Paul <seanpaul@chromium.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6425 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
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              | 11 ++++++
 include/drm/drm_dp_mst_helper.h       | 13 +++++++
 5 files changed, 110 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index b1b5401..5f2fc96 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 799c156..0c0e497 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 b0ebe0f..cb91b5a 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)
@@ -177,6 +182,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 +257,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