[PATCH v2 0/9] Adding private objects to atomic state

[PATCH v2 0/9] Adding private objects to atomic 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.

But adding objects like DP MST link bandwidth to drm_atomic_state would
mean that a non-core object will be modified by the core helper functions
for swapping and clearing it's state. So, this series adds void * objects
to drm_atomic_state and helper functions that operate on void * types to
keep these objects and states private to the core. Drivers can then
implement specific functions to swap and clear states. The other advantage
having just void * for these objects in drm_atomic_state is that objects of
different types can be managed in the same state array.

This series also includes some necessary cleanups.

Dhinakaran Pandiyan (9):
  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: Add driver private objects to atomic state
  drm/dp: Introduce MST topology state
  drm/dp: Add DP MST helpers to atomically find and release vcpi slots
  drm: Connector helper function to release atomic state
  drm/dp: Release DP MST shared link bandwidth
  drm/dp: Track MST link bandwidth

 drivers/gpu/drm/drm_atomic.c             |  55 ++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c      |  15 +++-
 drivers/gpu/drm/drm_dp_mst_topology.c    | 143 ++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_dp_mst.c      |  48 ++++++++++-
 drivers/gpu/drm/nouveau/nv50_display.c   |   5 +-
 drivers/gpu/drm/radeon/radeon_dp_mst.c   |   6 +-
 include/drm/drm_atomic.h                 |  30 +++++++
 include/drm/drm_dp_mst_helper.h          |  37 +++++---
 include/drm/drm_modeset_helper_vtables.h |  15 ++++
 9 files changed, 313 insertions(+), 41 deletions(-)

-- 
2.7.4

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

[PATCH v2 1/9] 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
gives access to DRM structures. 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 v2 2/9] 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 descriptive.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 16 ++++++++--------
 include/drm/drm_dp_mst_helper.h       | 12 ------------
 2 files changed, 8 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..d9edd84 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,12 @@ 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 v2 3/9] 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 use 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    |  4 ++--
 drivers/gpu/drm/nouveau/nv50_display.c |  3 ++-
 drivers/gpu/drm/radeon/radeon_dp_mst.c |  4 +++-
 include/drm/drm_dp_mst_helper.h        |  2 +-
 5 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index d9edd84..b871d4e 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,16 +2512,18 @@ 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);
@@ -2532,7 +2531,6 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp
 	}
 	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..f51574f 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,8 @@ 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..6598306 100644
--- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
+++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
@@ -453,9 +453,11 @@ 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 v2 4/9] drm: Add driver private objects to atomic state

[Dhinakaran Pandiyan]

It is necessary to track states for objects other than connector, crtc
and plane for atomic modesets. But adding objects like DP MST link
bandwidth to drm_atomic_state would mean that a non-core object will be
modified by the core helper functions for swapping and clearing
it's state. So, lets add void * objects and helper functions that operate
on void * types to keep these objects and states private to the core.
Drivers can then implement specific functions to swap and clear states.
The other advantage having just void * for these objects in
drm_atomic_state is that objects of different types can be managed in the
same state array.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_atomic.c        | 55 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c |  6 ++++
 include/drm/drm_atomic.h            | 30 ++++++++++++++++++++
 3 files changed, 91 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 723392f..f3a71cc 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state)
 	kfree(state->connectors);
 	kfree(state->crtcs);
 	kfree(state->planes);
+	kfree(state->private_objs);
 }
 EXPORT_SYMBOL(drm_atomic_state_default_release);
 
@@ -184,6 +185,20 @@ 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_private_objs; i++) {
+		void *priv_obj = state->private_objs[i].obj;
+		void *obj_state = state->private_objs[i].obj_state;
+
+		if (!priv_obj)
+			continue;
+
+		state->private_objs[i].funcs->destroy_state(obj_state);
+		state->private_objs[i].obj = NULL;
+		state->private_objs[i].obj_state = NULL;
+		state->private_objs[i].funcs = NULL;
+	}
+
 }
 EXPORT_SYMBOL(drm_atomic_state_default_clear);
 
@@ -976,6 +991,46 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
 		plane->funcs->atomic_print_state(p, state);
 }
 
+
+void *
+drm_atomic_get_priv_obj_state(struct drm_atomic_state *state, void *obj,
+			      const struct drm_private_state_funcs *funcs)
+{
+	int index, num_objs, i;
+	size_t size;
+	struct __drm_private_objs_state *arr;
+
+	for (i = 0; i < state->num_private_objs; i++)
+		if (obj == state->private_objs[i].obj &&
+		    state->private_objs[i].obj_state)
+			return state->private_objs[i].obj_state;
+
+	num_objs = state->num_private_objs + 1;
+	size = sizeof(*state->private_objs) * num_objs;
+	arr = krealloc(state->private_objs, size, GFP_KERNEL);
+	if (!arr)
+		return ERR_PTR(-ENOMEM);
+
+	state->private_objs = arr;
+	index = state->num_private_objs;
+	memset(&state->private_objs[index], 0, sizeof(*state->private_objs));
+
+	state->private_objs[index].obj_state = funcs->duplicate_state(state, obj);
+	if (!state->private_objs[index].obj_state)
+		return ERR_PTR(-ENOMEM);
+
+	state->private_objs[index].obj = obj;
+	state->private_objs[index].funcs = funcs;
+	state->num_private_objs = num_objs;
+
+	DRM_DEBUG_ATOMIC("Added new private object state %p to %p\n",
+			 state->private_objs[index].obj_state, state);
+
+	return state->private_objs[index].obj_state;
+}
+EXPORT_SYMBOL(drm_atomic_get_priv_obj_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 1f0cd7e..dd34d21 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1977,6 +1977,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 	struct drm_plane *plane;
 	struct drm_plane_state *plane_state;
 	struct drm_crtc_commit *commit;
+	void *obj, *obj_state;
+	const struct drm_private_state_funcs *funcs;
 
 	if (stall) {
 		for_each_crtc_in_state(state, crtc, crtc_state, i) {
@@ -2025,6 +2027,10 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 		swap(state->planes[i].state, plane->state);
 		plane->state->state = NULL;
 	}
+
+	for_each_private_obj(state, obj, obj_state, i, funcs) {
+		funcs->swap_state(obj, &state->private_objs[i].obj_state);
+	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_swap_state);
 
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index f1cb2b0..80d6e21 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -153,6 +153,18 @@ struct __drm_connnectors_state {
 	struct drm_connector_state *state;
 };
 
+struct drm_private_state_funcs {
+	void (*swap_state)(void *obj, void **obj_state_ptr);
+	void (*destroy_state)(void *obj_state);
+	void *(*duplicate_state)(struct drm_atomic_state *state, void *obj);
+};
+
+struct __drm_private_objs_state {
+	void *obj;
+	void *obj_state;
+	const 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)
@@ -164,6 +176,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_private_objs: size of the @private_objs array
+ * @private_objs: pointer to array of private object pointers
  * @acquire_ctx: acquire context for this atomic modeset state update
  */
 struct drm_atomic_state {
@@ -177,6 +191,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_objs_state *private_objs;
 
 	struct drm_modeset_acquire_ctx *acquire_ctx;
 
@@ -269,6 +285,11 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
 		struct drm_connector_state *state, struct drm_property *property,
 		uint64_t val);
 
+void * __must_check
+drm_atomic_get_priv_obj_state(struct drm_atomic_state *state,
+			      void *obj,
+			      const struct drm_private_state_funcs *funcs);
+
 /**
  * drm_atomic_get_existing_crtc_state - get crtc state, if it exists
  * @state: global atomic state object
@@ -413,6 +434,15 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
 	     (__i)++)							\
 		for_each_if (plane_state)
 
+#define for_each_private_obj(__state, obj, obj_state, __i, __funcs)	\
+	for ((__i) = 0;							\
+	     (__i) < (__state)->num_private_objs &&			\
+	     ((obj) = (__state)->private_objs[__i].obj,			\
+	      (__funcs) = (__state)->private_objs[__i].funcs,		\
+	      (obj_state) = (__state)->private_objs[__i].obj_state, 1);	\
+	      (__i)++)							\
+		for_each_if (__funcs)
+
 /**
  * drm_atomic_crtc_needs_modeset - compute combined modeset need
  * @state: &drm_crtc_state for the CRTC
-- 
2.7.4

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

[PATCH v2 5/9] drm/dp: Introduce 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. Let's encsapsulate the available link bw information in a
private 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_dp_mst_topology.c | 60 +++++++++++++++++++++++++++++++++++
 include/drm/drm_dp_mst_helper.h       | 20 ++++++++++++
 2 files changed, 80 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index b871d4e..247286b 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2042,6 +2042,8 @@ 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) {
@@ -2935,6 +2937,55 @@ static void drm_dp_destroy_connector_work(struct work_struct *work)
 		(*mgr->cbs->hotplug)(mgr);
 }
 
+void drm_dp_mst_destroy_state(void *obj_state)
+{
+	kfree(obj_state);
+}
+
+void drm_dp_mst_swap_state(void *obj, void **obj_state_ptr)
+{
+	struct drm_dp_mst_topology_mgr *mgr = obj;
+	struct drm_dp_mst_topology_state **topology_state_ptr;
+
+	topology_state_ptr = (struct drm_dp_mst_topology_state **)obj_state_ptr;
+
+	mgr->state->state = (*topology_state_ptr)->state;
+	swap(*topology_state_ptr, mgr->state);
+	mgr->state->state = NULL;
+}
+
+void *drm_dp_mst_duplicate_state(struct drm_atomic_state *state, void *obj)
+{
+	struct drm_dp_mst_topology_mgr *mgr = obj;
+	struct drm_dp_mst_topology_state *new_mst_state;
+
+	if (WARN_ON(!mgr->state))
+		return NULL;
+
+	new_mst_state = kmalloc(sizeof(*new_mst_state), GFP_KERNEL);
+	if (new_mst_state) {
+		new_mst_state->avail_slots = mgr->state->avail_slots;
+		new_mst_state->mgr = mgr;
+		new_mst_state->state = state;
+	}
+
+	return new_mst_state;
+}
+
+static const struct drm_private_state_funcs mst_state_funcs = {
+	.swap_state = drm_dp_mst_swap_state,
+	.destroy_state = drm_dp_mst_destroy_state,
+	.duplicate_state = drm_dp_mst_duplicate_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)
+{
+		return drm_atomic_get_priv_obj_state(state, mgr,
+						     &mst_state_funcs);
+}
+EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
+
 /**
  * drm_dp_mst_topology_mgr_init - initialise a topology manager
  * @mgr: manager struct to initialise
@@ -2979,6 +3030,12 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
 	if (test_calc_pbn_mode() < 0)
 		DRM_ERROR("MST PBN self-test failed\n");
 
+	mgr->state = kzalloc(sizeof(*mgr->state), GFP_KERNEL);
+	if (mgr->state == NULL)
+		return -ENOMEM;
+	mgr->state->mgr = mgr;
+	mgr->funcs = &mst_state_funcs;
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_dp_mst_topology_mgr_init);
@@ -2999,6 +3056,9 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr)
 	mutex_unlock(&mgr->payload_lock);
 	mgr->dev = NULL;
 	mgr->aux = NULL;
+	kfree(mgr->state);
+	mgr->state = NULL;
+	mgr->funcs = NULL;
 }
 EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy);
 
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 98d3c73..8cbdbfa 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -24,6 +24,7 @@
 
 #include <linux/types.h>
 #include <drm/drm_dp_helper.h>
+#include <drm/drm_atomic.h>
 
 struct drm_dp_mst_branch;
 
@@ -403,6 +404,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 +488,16 @@ struct drm_dp_mst_topology_mgr {
 	int pbn_div;
 
 	/**
+	 * @state: State information for topology manager
+	 */
+	struct drm_dp_mst_topology_state *state;
+
+	/**
+	 * @funcs: Atomic helper callbacks
+	 */
+	const struct drm_private_state_funcs *funcs;
+
+	/**
 	 * @qlock: protects @tx_msg_downq, the tx_slots in struct
 	 * &drm_dp_mst_branch and txmsg->state once they are queued
 	 */
@@ -596,4 +613,7 @@ 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 v2 6/9] 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       |  4 ++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 247286b..abebb42 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2500,6 +2500,49 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
 }
 
 /**
+ * drm_dp_atomic_release_vcpi_slots() - Release allocated vcpi slots
+ * @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 req=%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 8cbdbfa..4d61269 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -615,5 +615,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 v2 7/9] drm: Connector helper function to release atomic state

[Dhinakaran Pandiyan]

Having a ->atomic_release callback is useful to release shared resources
that get allocated in compute_config().

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 include/drm/drm_modeset_helper_vtables.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 46f5b34..e41b18a 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -831,6 +831,21 @@ struct drm_connector_helper_funcs {
 	 */
 	struct drm_encoder *(*atomic_best_encoder)(struct drm_connector *connector,
 						   struct drm_connector_state *connector_state);
+
+	/**
+	 * @atomic_release:
+	 *
+	 * This function is used to release shared resources that were
+	 * previously acquired. For example, resources acquired in
+	 * encoder->compute_config() can be released by calling this function
+	 * from mode_fixup()
+	 *
+	 * NOTE:
+	 *
+	 * This function is called in the check phase of an atomic update.
+	 */
+	void (*atomic_release)(struct drm_connector *connector,
+			       struct drm_connector_state *connector_state);
 };
 
 /**
-- 
2.7.4

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

[PATCH v2 8/9] drm/dp: Release DP MST shared link bandwidth

[Dhinakaran Pandiyan]

Implement the ->atomic_release() callback to release the shared link
bandwidth that was originally acquired during compute_config()

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index f51574f..2f57a56 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -78,6 +78,33 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 
 }
 
+static void intel_dp_mst_atomic_release(struct drm_connector *connector,
+					struct drm_connector_state *conn_state)
+{
+	struct intel_dp_mst_encoder *intel_mst;
+	struct drm_dp_mst_topology_mgr *mgr;
+	struct drm_dp_mst_topology_state *topology_state;
+	struct drm_encoder *encoder;
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+
+	encoder = connector->state->best_encoder;
+	if (!encoder || to_intel_encoder(encoder)->type != INTEL_OUTPUT_DP_MST)
+		return;
+
+	intel_mst = enc_to_mst(encoder);
+	mgr = &intel_mst->primary->dp.mst_mgr;
+
+	topology_state = drm_atomic_get_mst_topology_state(conn_state->state,
+							   mgr);
+	if (IS_ERR(topology_state)) {
+		DRM_DEBUG_KMS("failed to create MST topology state %ld\n",
+				PTR_ERR(topology_state));
+		return;
+	}
+
+	drm_dp_atomic_release_vcpi_slots(topology_state, intel_connector->port);
+}
+
 static void intel_mst_disable_dp(struct intel_encoder *encoder,
 				 struct intel_crtc_state *old_crtc_state,
 				 struct drm_connector_state *old_conn_state)
@@ -401,6 +428,7 @@ static const struct drm_connector_helper_funcs intel_dp_mst_connector_helper_fun
 	.mode_valid = intel_dp_mst_mode_valid,
 	.atomic_best_encoder = intel_mst_atomic_best_encoder,
 	.best_encoder = intel_mst_best_encoder,
+	.atomic_release = intel_dp_mst_atomic_release,
 };
 
 static void intel_dp_mst_encoder_destroy(struct drm_encoder *encoder)
-- 
2.7.4

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

[PATCH v2 9/9] drm/dp: Track MST link bandwidth

[Dhinakaran Pandiyan]

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

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c |  9 ++++++++-
 drivers/gpu/drm/i915/intel_dp_mst.c | 13 ++++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index dd34d21..0d42173 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -385,8 +385,15 @@ 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) {
+			const struct drm_connector_helper_funcs *conn_funcs;
+
+			conn_funcs = connector->helper_private;
+			if (conn_funcs->atomic_release)
+				conn_funcs->atomic_release(connector,
+							   conn_state);
 			continue;
+		}
 
 		crtc_state = drm_atomic_get_existing_crtc_state(state,
 								conn_state->crtc);
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 2f57a56..ee5fedb 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,17 @@ 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,
-- 
2.7.4

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

Re: [Intel-gfx] [PATCH v2 1/9] drm/dp: Store drm_device in MST topology manager

[Dave Airlie]

On 25 January 2017 at 09:49, Dhinakaran Pandiyan
<dhinakaran.pandiyan@intel.com> wrote:
> 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
> gives access to DRM structures. This also makes it consistent with other
> DRM structures like drm_crtc, drm_connector etc.
>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Makes sense, can't think of any reason I initially didn't do this.

Reviewed-by: Dave Airlie <airlied@redhat.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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/9] drm/dp: Split drm_dp_mst_allocate_vcpi

[Dave Airlie]

On 25 January 2017 at 09:49, Dhinakaran Pandiyan
<dhinakaran.pandiyan@intel.com> wrote:
> 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 use the atomic version of
> find_vcpi_slots() and reuse drm_dp_mst_allocate_vcpi()
>

Also seem sane, at least for the core bits,

Reviewed-by: Dave Airlie <airlied@redhat.com>

> 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    |  4 ++--
>  drivers/gpu/drm/nouveau/nv50_display.c |  3 ++-
>  drivers/gpu/drm/radeon/radeon_dp_mst.c |  4 +++-
>  include/drm/drm_dp_mst_helper.h        |  2 +-
>  5 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index d9edd84..b871d4e 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,16 +2512,18 @@ 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);
> @@ -2532,7 +2531,6 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp
>         }
>         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..f51574f 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,8 @@ 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..6598306 100644
> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> @@ -453,9 +453,11 @@ 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/9] drm/dp: Kill unused MST vcpi slot availability tracking

[Daniel Vetter]

On Tue, Jan 24, 2017 at 03:49:30PM -0800, Dhinakaran Pandiyan wrote:
> 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 descriptive.

Ok, these are 3 different changes, and they need to be split up. Well, at
least in 2 patches, to get the functional change out of there:
- First hardcode avail_slots to 63, with the commit message explaining in
  detail why that's the right thing. You can already remove total_pbn and
  total_slots in that patch, since they will be unused. The commit message
  should have a reference to the DP spec to support that "it's always 63"
  claim.
- Then garbage-collect ->avail_slots in a 2nd patch.

If you smash these together it's a lot less obvious that this is not just
a pure refactoring.

Thanks, Daniel

> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 16 ++++++++--------
>  include/drm/drm_dp_mst_helper.h       | 12 ------------
>  2 files changed, 8 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..d9edd84 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,12 @@ 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
> 

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

Re: [PATCH v2 4/9] drm: Add driver private objects to atomic state

[Daniel Vetter]

On Tue, Jan 24, 2017 at 03:49:32PM -0800, Dhinakaran Pandiyan wrote:
> It is necessary to track states for objects other than connector, crtc
> and plane for atomic modesets. But adding objects like DP MST link
> bandwidth to drm_atomic_state would mean that a non-core object will be
> modified by the core helper functions for swapping and clearing
> it's state. So, lets add void * objects and helper functions that operate
> on void * types to keep these objects and states private to the core.
> Drivers can then implement specific functions to swap and clear states.
> The other advantage having just void * for these objects in
> drm_atomic_state is that objects of different types can be managed in the
> same state array.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

I think an overview DOC: section to explain how to subclass atomic state
and how to do private state objects would be great. But I can do that once
your patch has landed, since it'd be much more about the overall design of
atomic (and I promised to do that anyway).

Looks pretty good, a few nits below still. I'll also ask amd folks to
check this out, and I think Ville could use it for his cdclk stuff.

> ---
>  drivers/gpu/drm/drm_atomic.c        | 55 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c |  6 ++++
>  include/drm/drm_atomic.h            | 30 ++++++++++++++++++++
>  3 files changed, 91 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 723392f..f3a71cc 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state)
>  	kfree(state->connectors);
>  	kfree(state->crtcs);
>  	kfree(state->planes);
> +	kfree(state->private_objs);
>  }
>  EXPORT_SYMBOL(drm_atomic_state_default_release);
>  
> @@ -184,6 +185,20 @@ 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_private_objs; i++) {
> +		void *priv_obj = state->private_objs[i].obj;
> +		void *obj_state = state->private_objs[i].obj_state;
> +
> +		if (!priv_obj)
> +			continue;
> +
> +		state->private_objs[i].funcs->destroy_state(obj_state);
> +		state->private_objs[i].obj = NULL;
> +		state->private_objs[i].obj_state = NULL;
> +		state->private_objs[i].funcs = NULL;
> +	}
> +
>  }
>  EXPORT_SYMBOL(drm_atomic_state_default_clear);
>  
> @@ -976,6 +991,46 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
>  		plane->funcs->atomic_print_state(p, state);
>  }
>  
> +

Needs kerneldoc here.

> +void *
> +drm_atomic_get_priv_obj_state(struct drm_atomic_state *state, void *obj,

ocd bikeshed: priv_obj vs private_obj inconsistency here, please stick to
one (I don't care which one).

> +			      const struct drm_private_state_funcs *funcs)
> +{
> +	int index, num_objs, i;
> +	size_t size;
> +	struct __drm_private_objs_state *arr;
> +
> +	for (i = 0; i < state->num_private_objs; i++)
> +		if (obj == state->private_objs[i].obj &&
> +		    state->private_objs[i].obj_state)
> +			return state->private_objs[i].obj_state;
> +
> +	num_objs = state->num_private_objs + 1;
> +	size = sizeof(*state->private_objs) * num_objs;
> +	arr = krealloc(state->private_objs, size, GFP_KERNEL);
> +	if (!arr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	state->private_objs = arr;
> +	index = state->num_private_objs;
> +	memset(&state->private_objs[index], 0, sizeof(*state->private_objs));
> +
> +	state->private_objs[index].obj_state = funcs->duplicate_state(state, obj);
> +	if (!state->private_objs[index].obj_state)
> +		return ERR_PTR(-ENOMEM);

I wondered whether we need to allow other error codes than ENOMEM, e.g.
if duplicate_state needs to acquire a ww_mutex. But we can always acquire
the necessary locks outside of drm_atomic_get_priv_obj_state in some
helper/driver wrapper. So no big deal, but worth explaining that
drm_atomic_get_priv_obj_state won't acquire necessarily locsk (since it
doesn't know about them) in the kernel-doc.

> +
> +	state->private_objs[index].obj = obj;
> +	state->private_objs[index].funcs = funcs;
> +	state->num_private_objs = num_objs;
> +
> +	DRM_DEBUG_ATOMIC("Added new private object state %p to %p\n",
> +			 state->private_objs[index].obj_state, state);
> +
> +	return state->private_objs[index].obj_state;
> +}
> +EXPORT_SYMBOL(drm_atomic_get_priv_obj_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 1f0cd7e..dd34d21 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1977,6 +1977,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  	struct drm_plane *plane;
>  	struct drm_plane_state *plane_state;
>  	struct drm_crtc_commit *commit;
> +	void *obj, *obj_state;
> +	const struct drm_private_state_funcs *funcs;
>  
>  	if (stall) {
>  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> @@ -2025,6 +2027,10 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  		swap(state->planes[i].state, plane->state);
>  		plane->state->state = NULL;
>  	}
> +
> +	for_each_private_obj(state, obj, obj_state, i, funcs) {
> +		funcs->swap_state(obj, &state->private_objs[i].obj_state);
> +	}
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_swap_state);
>  
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index f1cb2b0..80d6e21 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -153,6 +153,18 @@ struct __drm_connnectors_state {
>  	struct drm_connector_state *state;
>  };
>  

This one needs kernel-doc too, since it's a struct that drivers/helpers
will need to implement. Please use the inline style and document expected
semantics in detail, like we do for other vfunc tables.

> +struct drm_private_state_funcs {

I also wonder whether we shouldn't give this a drm_atomic_ prefix ...

> +	void (*swap_state)(void *obj, void **obj_state_ptr);
> +	void (*destroy_state)(void *obj_state);
> +	void *(*duplicate_state)(struct drm_atomic_state *state, void *obj);
> +};
> +
> +struct __drm_private_objs_state {
> +	void *obj;
> +	void *obj_state;
> +	const 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)
> @@ -164,6 +176,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_private_objs: size of the @private_objs array
> + * @private_objs: pointer to array of private object pointers
>   * @acquire_ctx: acquire context for this atomic modeset state update
>   */
>  struct drm_atomic_state {
> @@ -177,6 +191,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_objs_state *private_objs;
>  
>  	struct drm_modeset_acquire_ctx *acquire_ctx;
>  
> @@ -269,6 +285,11 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
>  		struct drm_connector_state *state, struct drm_property *property,
>  		uint64_t val);
>  
> +void * __must_check
> +drm_atomic_get_priv_obj_state(struct drm_atomic_state *state,
> +			      void *obj,
> +			      const struct drm_private_state_funcs *funcs);
> +
>  /**
>   * drm_atomic_get_existing_crtc_state - get crtc state, if it exists
>   * @state: global atomic state object
> @@ -413,6 +434,15 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
>  	     (__i)++)							\
>  		for_each_if (plane_state)
>  
> +#define for_each_private_obj(__state, obj, obj_state, __i, __funcs)	\
> +	for ((__i) = 0;							\
> +	     (__i) < (__state)->num_private_objs &&			\
> +	     ((obj) = (__state)->private_objs[__i].obj,			\
> +	      (__funcs) = (__state)->private_objs[__i].funcs,		\
> +	      (obj_state) = (__state)->private_objs[__i].obj_state, 1);	\
> +	      (__i)++)							\
> +		for_each_if (__funcs)

You are not filtering for the function table here, which is important to
make sure that this can be used to only walk over objects with a given
vtable. With that we can then #define specific macros for e.g. mst:

struct drm_private_state_funcs mst_state_funcs;

#define for_each_mst_state(__state, obj, obj_state, __i, __funcs) \
	for_each_private_obj(__state, &mst_state_funcs, obj, obj_state, __i, __funcs)

I'd place the vfunc right after the state, since those are both input
parameters to the macro, and specify what exactly we're looping over. To
make this work you need something like:

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

Note the check to compare __funcs == obj_funcs.

With that other subsytem can the filter for their own objects only with
e.g.

#define intel_for_each_cdclk_state(__state, obj, obj_state, __i, __funcs) \
	for_each_private_obj(__state, &intel_cdclk_state_funcs, obj, obj_state, __i, __funcs)

Would be good to also then have kerneldoc for this iterator, to explain
how to use it.
-Daniel

>  /**
>   * drm_atomic_crtc_needs_modeset - compute combined modeset need
>   * @state: &drm_crtc_state for the CRTC
> -- 
> 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 v2 5/9] drm/dp: Introduce MST topology state

[Daniel Vetter]

On Tue, Jan 24, 2017 at 03:49:33PM -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
> private 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>

Yeah, I like how this neatly hides all the atomic state tracking now, and
just leaves the mst specific logic in the mst helpers.

> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 60 +++++++++++++++++++++++++++++++++++
>  include/drm/drm_dp_mst_helper.h       | 20 ++++++++++++
>  2 files changed, 80 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index b871d4e..247286b 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2042,6 +2042,8 @@ 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;

We can't reset atomic state on each hotplug, that would wreak the atomic
tracking. E.g. there might still be a pipe around running on this output,
and then we'd go above 63 slots when we release that one. You need to init
this once in drm_dp_mst_topology_mgr_init() and then make sure it's always
accurate.

>  		/* add initial branch device at LCT 1 */
>  		mstb = drm_dp_add_mst_branch_device(1, NULL);
>  		if (mstb == NULL) {
> @@ -2935,6 +2937,55 @@ static void drm_dp_destroy_connector_work(struct work_struct *work)
>  		(*mgr->cbs->hotplug)(mgr);
>  }
>  
> +void drm_dp_mst_destroy_state(void *obj_state)
> +{
> +	kfree(obj_state);
> +}
> +
> +void drm_dp_mst_swap_state(void *obj, void **obj_state_ptr)
> +{
> +	struct drm_dp_mst_topology_mgr *mgr = obj;
> +	struct drm_dp_mst_topology_state **topology_state_ptr;
> +
> +	topology_state_ptr = (struct drm_dp_mst_topology_state **)obj_state_ptr;
> +
> +	mgr->state->state = (*topology_state_ptr)->state;
> +	swap(*topology_state_ptr, mgr->state);
> +	mgr->state->state = NULL;
> +}
> +
> +void *drm_dp_mst_duplicate_state(struct drm_atomic_state *state, void *obj)
> +{
> +	struct drm_dp_mst_topology_mgr *mgr = obj;
> +	struct drm_dp_mst_topology_state *new_mst_state;
> +
> +	if (WARN_ON(!mgr->state))
> +		return NULL;
> +
> +	new_mst_state = kmalloc(sizeof(*new_mst_state), GFP_KERNEL);
> +	if (new_mst_state) {
> +		new_mst_state->avail_slots = mgr->state->avail_slots;
> +		new_mst_state->mgr = mgr;
> +		new_mst_state->state = state;
> +	}

kmemdup is less code :-)

> +
> +	return new_mst_state;
> +}
> +
> +static const struct drm_private_state_funcs mst_state_funcs = {
> +	.swap_state = drm_dp_mst_swap_state,
> +	.destroy_state = drm_dp_mst_destroy_state,
> +	.duplicate_state = drm_dp_mst_duplicate_state,
> +};
> +

Needs a few lines of kernel-doc here since it's an exported function.

Cheers, Daniel

> +struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
> +								    struct drm_dp_mst_topology_mgr *mgr)
> +{
> +		return drm_atomic_get_priv_obj_state(state, mgr,
> +						     &mst_state_funcs);
> +}
> +EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
> +
>  /**
>   * drm_dp_mst_topology_mgr_init - initialise a topology manager
>   * @mgr: manager struct to initialise
> @@ -2979,6 +3030,12 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
>  	if (test_calc_pbn_mode() < 0)
>  		DRM_ERROR("MST PBN self-test failed\n");
>  
> +	mgr->state = kzalloc(sizeof(*mgr->state), GFP_KERNEL);
> +	if (mgr->state == NULL)
> +		return -ENOMEM;
> +	mgr->state->mgr = mgr;
> +	mgr->funcs = &mst_state_funcs;
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_dp_mst_topology_mgr_init);
> @@ -2999,6 +3056,9 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr)
>  	mutex_unlock(&mgr->payload_lock);
>  	mgr->dev = NULL;
>  	mgr->aux = NULL;
> +	kfree(mgr->state);
> +	mgr->state = NULL;
> +	mgr->funcs = NULL;
>  }
>  EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy);
>  
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 98d3c73..8cbdbfa 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -24,6 +24,7 @@
>  
>  #include <linux/types.h>
>  #include <drm/drm_dp_helper.h>
> +#include <drm/drm_atomic.h>
>  
>  struct drm_dp_mst_branch;
>  
> @@ -403,6 +404,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 +488,16 @@ struct drm_dp_mst_topology_mgr {
>  	int pbn_div;
>  
>  	/**
> +	 * @state: State information for topology manager
> +	 */
> +	struct drm_dp_mst_topology_state *state;
> +
> +	/**
> +	 * @funcs: Atomic helper callbacks
> +	 */
> +	const struct drm_private_state_funcs *funcs;
> +
> +	/**
>  	 * @qlock: protects @tx_msg_downq, the tx_slots in struct
>  	 * &drm_dp_mst_branch and txmsg->state once they are queued
>  	 */
> @@ -596,4 +613,7 @@ 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 9/9] drm/dp: Track MST link bandwidth

[Daniel Vetter]

On Tue, Jan 24, 2017 at 03:49:37PM -0800, Dhinakaran Pandiyan wrote:
> Use the added helpers to track MST link bandwidth for atomic modesets.
> Link bw is acquired in the ->atomic_check() phase when CRTCs are being
> enabled with drm_atomic_find_vcpi_slots() instead of drm_find_vcpi_slots().
> Similarly, link bw is released during ->atomic_check() with the connector
> helper callback ->atomic_release() when CRTCs are disabled.
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c |  9 ++++++++-
>  drivers/gpu/drm/i915/intel_dp_mst.c | 13 ++++++++++++-
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index dd34d21..0d42173 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -385,8 +385,15 @@ 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) {
> +			const struct drm_connector_helper_funcs *conn_funcs;
> +
> +			conn_funcs = connector->helper_private;
> +			if (conn_funcs->atomic_release)
> +				conn_funcs->atomic_release(connector,
> +							   conn_state);

I wonder whether this won't surprise drivers: The way you coded this
release only gets called when the connector is fully disabled. But it does
not get called when you atomically switch it from one crtc to the other
(in which case you also might want to release resources, before allocating
new ones). I think that case of directly switching stuff is even a bug in
your current atomic tracking code ...

We also need to extract the release calls into a separate loop which runs
_before_ we allocate any resources. Otherwise if you do an atomic commit
where you disable connector2 and enable connector1 and they can't run both
at the same time it'll fail, because we'll release the vcpi for connector2
only after we've tried to get it for connnector1.

>  			continue;
> +		}
>  
>  		crtc_state = drm_atomic_get_existing_crtc_state(state,
>  								conn_state->crtc);

Misplaced hunk, this needs to be in the patch that adds the
->atomic_release callback.

> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 2f57a56..ee5fedb 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,17 @@ 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);

Can we merge the get_mst_topology_state and find_vcpi_slots functions
together? Would be less code in drivers ...

> +	if (slots < 0) {
> +		DRM_DEBUG_KMS("not enough link bw for this mode\n");
> +		return false;
> +	}

And then also stuff the error checking in there, and just have a return
-EINVAL when there's not enough bw.

I think this should be together with the previous patch, to wire up the
entire mst state tracking for i915 at the same time. Atm the previous
patch just wires up dead code, which is a bit backwards.
-Daniel

>  
>  	intel_link_compute_m_n(bpp, lane_count,
>  			       adjusted_mode->crtc_clock,
> -- 
> 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 v2 8/9] drm/dp: Release DP MST shared link bandwidth

[Daniel Vetter]

On Tue, Jan 24, 2017 at 03:49:36PM -0800, Dhinakaran Pandiyan wrote:
> Implement the ->atomic_release() callback to release the shared link
> bandwidth that was originally acquired during compute_config()
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index f51574f..2f57a56 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -78,6 +78,33 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  
>  }
>  
> +static void intel_dp_mst_atomic_release(struct drm_connector *connector,
> +					struct drm_connector_state *conn_state)
> +{
> +	struct intel_dp_mst_encoder *intel_mst;
> +	struct drm_dp_mst_topology_mgr *mgr;
> +	struct drm_dp_mst_topology_state *topology_state;
> +	struct drm_encoder *encoder;
> +	struct intel_connector *intel_connector = to_intel_connector(connector);
> +
> +	encoder = connector->state->best_encoder;
> +	if (!encoder || to_intel_encoder(encoder)->type != INTEL_OUTPUT_DP_MST)
> +		return;

NULL encoder should imo be caught in core. Type != DP_MST is impossible,
if you're unsure make it into a BUG_ON for testing.
> +
> +	intel_mst = enc_to_mst(encoder);
> +	mgr = &intel_mst->primary->dp.mst_mgr;
> +
> +	topology_state = drm_atomic_get_mst_topology_state(conn_state->state,
> +							   mgr);
> +	if (IS_ERR(topology_state)) {
> +		DRM_DEBUG_KMS("failed to create MST topology state %ld\n",
> +				PTR_ERR(topology_state));
> +		return;
> +	}
> +
> +	drm_dp_atomic_release_vcpi_slots(topology_state, intel_connector->port);

I think it'd be great to merge this together into 1 helper that both gets
the state and releases the vcpi, for less code in drivers.
> +}
> +
>  static void intel_mst_disable_dp(struct intel_encoder *encoder,
>  				 struct intel_crtc_state *old_crtc_state,
>  				 struct drm_connector_state *old_conn_state)
> @@ -401,6 +428,7 @@ static const struct drm_connector_helper_funcs intel_dp_mst_connector_helper_fun
>  	.mode_valid = intel_dp_mst_mode_valid,
>  	.atomic_best_encoder = intel_mst_atomic_best_encoder,
>  	.best_encoder = intel_mst_best_encoder,
> +	.atomic_release = intel_dp_mst_atomic_release,
>  };
>  
>  static void intel_dp_mst_encoder_destroy(struct drm_encoder *encoder)
> -- 
> 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 v2 7/9] drm: Connector helper function to release atomic state

[Daniel Vetter]

On Tue, Jan 24, 2017 at 03:49:35PM -0800, Dhinakaran Pandiyan wrote:
> Having a ->atomic_release callback is useful to release shared resources
> that get allocated in compute_config().
> 
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  include/drm/drm_modeset_helper_vtables.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 46f5b34..e41b18a 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -831,6 +831,21 @@ struct drm_connector_helper_funcs {
>  	 */
>  	struct drm_encoder *(*atomic_best_encoder)(struct drm_connector *connector,
>  						   struct drm_connector_state *connector_state);
> +
> +	/**
> +	 * @atomic_release:
> +	 *
> +	 * This function is used to release shared resources that were
> +	 * previously acquired. For example, resources acquired in
> +	 * encoder->compute_config() can be released by calling this function

@compute_config is the right way to do references within the same struct.

> +	 * from mode_fixup()

Same here.

Patch split up is a bit strange, hence why my review of the design is in
later patches.

Thanks, Daniel

> +	 *
> +	 * NOTE:
> +	 *
> +	 * This function is called in the check phase of an atomic update.
> +	 */
> +	void (*atomic_release)(struct drm_connector *connector,
> +			       struct drm_connector_state *connector_state);
>  };
>  
>  /**
> -- 
> 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 v2 3/9] drm/dp: Split drm_dp_mst_allocate_vcpi

[Pandiyan, Dhinakaran]

On Wed, 2017-01-25 at 10:31 +1000, Dave Airlie wrote:
> On 25 January 2017 at 09:49, Dhinakaran Pandiyan
> <dhinakaran.pandiyan@intel.com> wrote:
> > 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 use the atomic version of
> > find_vcpi_slots() and reuse drm_dp_mst_allocate_vcpi()
> >
> 
> Also seem sane, at least for the core bits,
> 
> Reviewed-by: Dave Airlie <airlied@redhat.com>
> 

Thanks for the review.

-DK


> > 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    |  4 ++--
> >  drivers/gpu/drm/nouveau/nv50_display.c |  3 ++-
> >  drivers/gpu/drm/radeon/radeon_dp_mst.c |  4 +++-
> >  include/drm/drm_dp_mst_helper.h        |  2 +-
> >  5 files changed, 17 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index d9edd84..b871d4e 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,16 +2512,18 @@ 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);
> > @@ -2532,7 +2531,6 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp
> >         }
> >         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..f51574f 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,8 @@ 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..6598306 100644
> > --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
> > +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> > @@ -453,9 +453,11 @@ 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
> _______________________________________________
> 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 v2 2/9] drm/dp: Kill unused MST vcpi slot availability tracking

[Pandiyan, Dhinakaran]

On Wed, 2017-01-25 at 06:43 +0100, Daniel Vetter wrote:
> On Tue, Jan 24, 2017 at 03:49:30PM -0800, Dhinakaran Pandiyan wrote:
> > 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 descriptive.
> 
> Ok, these are 3 different changes, and they need to be split up. Well, at
> least in 2 patches, to get the functional change out of there:
> - First hardcode avail_slots to 63, with the commit message explaining in
>   detail why that's the right thing. You can already remove total_pbn and
>   total_slots in that patch, since they will be unused. The commit message
>   should have a reference to the DP spec to support that "it's always 63"
>   claim.
> - Then garbage-collect ->avail_slots in a 2nd patch.
> 
> If you smash these together it's a lot less obvious that this is not just
> a pure refactoring.
> 
> Thanks, Daniel
> 

Makes sense, will do this in the next revision.

-DK
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 16 ++++++++--------
> >  include/drm/drm_dp_mst_helper.h       | 12 ------------
> >  2 files changed, 8 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..d9edd84 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,12 @@ 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

Re: [PATCH v2 4/9] drm: Add driver private objects to atomic state

[Pandiyan, Dhinakaran]

On Wed, 2017-01-25 at 06:59 +0100, Daniel Vetter wrote:
> On Tue, Jan 24, 2017 at 03:49:32PM -0800, Dhinakaran Pandiyan wrote:
> > It is necessary to track states for objects other than connector, crtc
> > and plane for atomic modesets. But adding objects like DP MST link
> > bandwidth to drm_atomic_state would mean that a non-core object will be
> > modified by the core helper functions for swapping and clearing
> > it's state. So, lets add void * objects and helper functions that operate
> > on void * types to keep these objects and states private to the core.
> > Drivers can then implement specific functions to swap and clear states.
> > The other advantage having just void * for these objects in
> > drm_atomic_state is that objects of different types can be managed in the
> > same state array.
> > 
> > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> I think an overview DOC: section to explain how to subclass atomic state
> and how to do private state objects would be great. But I can do that once
> your patch has landed, since it'd be much more about the overall design of
> atomic (and I promised to do that anyway).
> 
> Looks pretty good, a few nits below still. I'll also ask amd folks to
> check this out, and I think Ville could use it for his cdclk stuff.
> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c        | 55 +++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_atomic_helper.c |  6 ++++
> >  include/drm/drm_atomic.h            | 30 ++++++++++++++++++++
> >  3 files changed, 91 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 723392f..f3a71cc 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state)
> >  	kfree(state->connectors);
> >  	kfree(state->crtcs);
> >  	kfree(state->planes);
> > +	kfree(state->private_objs);
> >  }
> >  EXPORT_SYMBOL(drm_atomic_state_default_release);
> >  
> > @@ -184,6 +185,20 @@ 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_private_objs; i++) {
> > +		void *priv_obj = state->private_objs[i].obj;
> > +		void *obj_state = state->private_objs[i].obj_state;
> > +
> > +		if (!priv_obj)
> > +			continue;
> > +
> > +		state->private_objs[i].funcs->destroy_state(obj_state);
> > +		state->private_objs[i].obj = NULL;
> > +		state->private_objs[i].obj_state = NULL;
> > +		state->private_objs[i].funcs = NULL;
> > +	}
> > +
> >  }
> >  EXPORT_SYMBOL(drm_atomic_state_default_clear);
> >  
> > @@ -976,6 +991,46 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
> >  		plane->funcs->atomic_print_state(p, state);
> >  }
> >  
> > +
> 
> Needs kerneldoc here.
> 
> > +void *
> > +drm_atomic_get_priv_obj_state(struct drm_atomic_state *state, void *obj,
> 
> ocd bikeshed: priv_obj vs private_obj inconsistency here, please stick to
> one (I don't care which one).
> 
> > +			      const struct drm_private_state_funcs *funcs)
> > +{
> > +	int index, num_objs, i;
> > +	size_t size;
> > +	struct __drm_private_objs_state *arr;
> > +
> > +	for (i = 0; i < state->num_private_objs; i++)
> > +		if (obj == state->private_objs[i].obj &&
> > +		    state->private_objs[i].obj_state)
> > +			return state->private_objs[i].obj_state;
> > +
> > +	num_objs = state->num_private_objs + 1;
> > +	size = sizeof(*state->private_objs) * num_objs;
> > +	arr = krealloc(state->private_objs, size, GFP_KERNEL);
> > +	if (!arr)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	state->private_objs = arr;
> > +	index = state->num_private_objs;
> > +	memset(&state->private_objs[index], 0, sizeof(*state->private_objs));
> > +
> > +	state->private_objs[index].obj_state = funcs->duplicate_state(state, obj);
> > +	if (!state->private_objs[index].obj_state)
> > +		return ERR_PTR(-ENOMEM);
> 
> I wondered whether we need to allow other error codes than ENOMEM, e.g.
> if duplicate_state needs to acquire a ww_mutex. But we can always acquire
> the necessary locks outside of drm_atomic_get_priv_obj_state in some
> helper/driver wrapper. So no big deal, but worth explaining that
> drm_atomic_get_priv_obj_state won't acquire necessarily locsk (since it
> doesn't know about them) in the kernel-doc.
> 

Got it, will include that.

> > +
> > +	state->private_objs[index].obj = obj;
> > +	state->private_objs[index].funcs = funcs;
> > +	state->num_private_objs = num_objs;
> > +
> > +	DRM_DEBUG_ATOMIC("Added new private object state %p to %p\n",
> > +			 state->private_objs[index].obj_state, state);
> > +
> > +	return state->private_objs[index].obj_state;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_get_priv_obj_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 1f0cd7e..dd34d21 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1977,6 +1977,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> >  	struct drm_plane *plane;
> >  	struct drm_plane_state *plane_state;
> >  	struct drm_crtc_commit *commit;
> > +	void *obj, *obj_state;
> > +	const struct drm_private_state_funcs *funcs;
> >  
> >  	if (stall) {
> >  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > @@ -2025,6 +2027,10 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> >  		swap(state->planes[i].state, plane->state);
> >  		plane->state->state = NULL;
> >  	}
> > +
> > +	for_each_private_obj(state, obj, obj_state, i, funcs) {
> > +		funcs->swap_state(obj, &state->private_objs[i].obj_state);
> > +	}
> >  }
> >  EXPORT_SYMBOL(drm_atomic_helper_swap_state);
> >  
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index f1cb2b0..80d6e21 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -153,6 +153,18 @@ struct __drm_connnectors_state {
> >  	struct drm_connector_state *state;
> >  };
> >  
> 
> This one needs kernel-doc too, since it's a struct that drivers/helpers
> will need to implement. Please use the inline style and document expected
> semantics in detail, like we do for other vfunc tables.
> 
> > +struct drm_private_state_funcs {
> 
> I also wonder whether we shouldn't give this a drm_atomic_ prefix ...
> 
> > +	void (*swap_state)(void *obj, void **obj_state_ptr);
> > +	void (*destroy_state)(void *obj_state);
> > +	void *(*duplicate_state)(struct drm_atomic_state *state, void *obj);
> > +};
> > +
> > +struct __drm_private_objs_state {
> > +	void *obj;
> > +	void *obj_state;
> > +	const 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)
> > @@ -164,6 +176,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_private_objs: size of the @private_objs array
> > + * @private_objs: pointer to array of private object pointers
> >   * @acquire_ctx: acquire context for this atomic modeset state update
> >   */
> >  struct drm_atomic_state {
> > @@ -177,6 +191,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_objs_state *private_objs;
> >  
> >  	struct drm_modeset_acquire_ctx *acquire_ctx;
> >  
> > @@ -269,6 +285,11 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
> >  		struct drm_connector_state *state, struct drm_property *property,
> >  		uint64_t val);
> >  
> > +void * __must_check
> > +drm_atomic_get_priv_obj_state(struct drm_atomic_state *state,
> > +			      void *obj,
> > +			      const struct drm_private_state_funcs *funcs);
> > +
> >  /**
> >   * drm_atomic_get_existing_crtc_state - get crtc state, if it exists
> >   * @state: global atomic state object
> > @@ -413,6 +434,15 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
> >  	     (__i)++)							\
> >  		for_each_if (plane_state)
> >  
> > +#define for_each_private_obj(__state, obj, obj_state, __i, __funcs)	\
> > +	for ((__i) = 0;							\
> > +	     (__i) < (__state)->num_private_objs &&			\
> > +	     ((obj) = (__state)->private_objs[__i].obj,			\
> > +	      (__funcs) = (__state)->private_objs[__i].funcs,		\
> > +	      (obj_state) = (__state)->private_objs[__i].obj_state, 1);	\
> > +	      (__i)++)							\
> > +		for_each_if (__funcs)
> 
> You are not filtering for the function table here, which is important to
> make sure that this can be used to only walk over objects with a given
> vtable. With that we can then #define specific macros for e.g. mst:
> 
> struct drm_private_state_funcs mst_state_funcs;
> 
> #define for_each_mst_state(__state, obj, obj_state, __i, __funcs) \
> 	for_each_private_obj(__state, &mst_state_funcs, obj, obj_state, __i, __funcs)
> 
> I'd place the vfunc right after the state, since those are both input
> parameters to the macro, and specify what exactly we're looping over. To
> make this work you need something like:
> 
> #define for_each_private_obj(__state, obj_funcs, obj, obj_state, __i, __funcs)	\
> 	for ((__i) = 0;							\
> 	     (__i) < (__state)->num_private_objs &&			\
> 	     ((obj) = (__state)->private_objs[__i].obj,			\
> 	      (__funcs) = (__state)->private_objs[__i].funcs,		\
> 	      (obj_state) = (__state)->private_objs[__i].obj_state, 1);	\
> 	      (__i)++)							\
> 		for_each_if (__funcs == obj_funcs)
> 
> Note the check to compare __funcs == obj_funcs.
> 
> With that other subsytem can the filter for their own objects only with
> e.g.
> 
> #define intel_for_each_cdclk_state(__state, obj, obj_state, __i, __funcs) \
> 	for_each_private_obj(__state, &intel_cdclk_state_funcs, obj, obj_state, __i, __funcs)
> 
> Would be good to also then have kerneldoc for this iterator, to explain
> how to use it.
> -Daniel
> 

I see your point but we can't use this iterator in the swap_state()
helper if we do that. I have used it to swap states for all objects
using this version without filtering.

I guess, I can just code the iterator explicitly for swap_state() and
re-write the iterator with the filtering.

-DK
> >  /**
> >   * drm_atomic_crtc_needs_modeset - compute combined modeset need
> >   * @state: &drm_crtc_state for the CRTC
> > -- 
> > 2.7.4
> > 
> 

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

Re: [PATCH v2 8/9] drm/dp: Release DP MST shared link bandwidth

[Pandiyan, Dhinakaran]

On Wed, 2017-01-25 at 07:16 +0100, Daniel Vetter wrote:
> On Tue, Jan 24, 2017 at 03:49:36PM -0800, Dhinakaran Pandiyan wrote:
> > Implement the ->atomic_release() callback to release the shared link
> > bandwidth that was originally acquired during compute_config()
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index f51574f..2f57a56 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -78,6 +78,33 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >  
> >  }
> >  
> > +static void intel_dp_mst_atomic_release(struct drm_connector *connector,
> > +					struct drm_connector_state *conn_state)
> > +{
> > +	struct intel_dp_mst_encoder *intel_mst;
> > +	struct drm_dp_mst_topology_mgr *mgr;
> > +	struct drm_dp_mst_topology_state *topology_state;
> > +	struct drm_encoder *encoder;
> > +	struct intel_connector *intel_connector = to_intel_connector(connector);
> > +
> > +	encoder = connector->state->best_encoder;
> > +	if (!encoder || to_intel_encoder(encoder)->type != INTEL_OUTPUT_DP_MST)
> > +		return;
> 
> NULL encoder should imo be caught in core. Type != DP_MST is impossible,
> if you're unsure make it into a BUG_ON for testing.


I don't get why (type != INTEL_OUTPUT_DP_MST) is impossible. Is that
because we have the implementation for atomic_release() only for MST
connectors? But, that is only for now.

-DK
> > +
> > +	intel_mst = enc_to_mst(encoder);
> > +	mgr = &intel_mst->primary->dp.mst_mgr;
> > +
> > +	topology_state = drm_atomic_get_mst_topology_state(conn_state->state,
> > +							   mgr);
> > +	if (IS_ERR(topology_state)) {
> > +		DRM_DEBUG_KMS("failed to create MST topology state %ld\n",
> > +				PTR_ERR(topology_state));
> > +		return;
> > +	}
> > +
> > +	drm_dp_atomic_release_vcpi_slots(topology_state, intel_connector->port);
> 
> I think it'd be great to merge this together into 1 helper that both gets
> the state and releases the vcpi, for less code in drivers.

Agreed.

> > +}
> > +
> >  static void intel_mst_disable_dp(struct intel_encoder *encoder,
> >  				 struct intel_crtc_state *old_crtc_state,
> >  				 struct drm_connector_state *old_conn_state)
> > @@ -401,6 +428,7 @@ static const struct drm_connector_helper_funcs intel_dp_mst_connector_helper_fun
> >  	.mode_valid = intel_dp_mst_mode_valid,
> >  	.atomic_best_encoder = intel_mst_atomic_best_encoder,
> >  	.best_encoder = intel_mst_best_encoder,
> > +	.atomic_release = intel_dp_mst_atomic_release,
> >  };
> >  
> >  static void intel_dp_mst_encoder_destroy(struct drm_encoder *encoder)
> > -- 
> > 2.7.4
> > 
> 

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

Re: [PATCH v2 9/9] drm/dp: Track MST link bandwidth

[Pandiyan, Dhinakaran]

On Wed, 2017-01-25 at 07:15 +0100, Daniel Vetter wrote:
> On Tue, Jan 24, 2017 at 03:49:37PM -0800, Dhinakaran Pandiyan wrote:
> > Use the added helpers to track MST link bandwidth for atomic modesets.
> > Link bw is acquired in the ->atomic_check() phase when CRTCs are being
> > enabled with drm_atomic_find_vcpi_slots() instead of drm_find_vcpi_slots().
> > Similarly, link bw is released during ->atomic_check() with the connector
> > helper callback ->atomic_release() when CRTCs are disabled.
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c |  9 ++++++++-
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 13 ++++++++++++-
> >  2 files changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index dd34d21..0d42173 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -385,8 +385,15 @@ 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) {
> > +			const struct drm_connector_helper_funcs *conn_funcs;
> > +
> > +			conn_funcs = connector->helper_private;
> > +			if (conn_funcs->atomic_release)
> > +				conn_funcs->atomic_release(connector,
> > +							   conn_state);
> 
> I wonder whether this won't surprise drivers: The way you coded this
> release only gets called when the connector is fully disabled. But it does
> not get called when you atomically switch it from one crtc to the other
> (in which case you also might want to release resources, before allocating
> new ones). I think that case of directly switching stuff is even a bug in
> your current atomic tracking code ...
> 
> We also need to extract the release calls into a separate loop which runs
> _before_ we allocate any resources. Otherwise if you do an atomic commit
> where you disable connector2 and enable connector1 and they can't run both
> at the same time it'll fail, because we'll release the vcpi for connector2
> only after we've tried to get it for connnector1.
> 


Yeah, you are right. I thought of this problem and then forgot about it.
Is there any igt test to test the switching?


-DK
> >  			continue;
> > +		}
> >  
> >  		crtc_state = drm_atomic_get_existing_crtc_state(state,
> >  								conn_state->crtc);
> 
> Misplaced hunk, this needs to be in the patch that adds the
> ->atomic_release callback.
> 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 2f57a56..ee5fedb 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,17 @@ 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);
> 
> Can we merge the get_mst_topology_state and find_vcpi_slots functions
> together? Would be less code in drivers ...
> 
> > +	if (slots < 0) {
> > +		DRM_DEBUG_KMS("not enough link bw for this mode\n");
> > +		return false;
> > +	}
> 
> And then also stuff the error checking in there, and just have a return
> -EINVAL when there's not enough bw.
> 
> I think this should be together with the previous patch, to wire up the
> entire mst state tracking for i915 at the same time. Atm the previous
> patch just wires up dead code, which is a bit backwards.
> -Daniel
> 
> >  
> >  	intel_link_compute_m_n(bpp, lane_count,
> >  			       adjusted_mode->crtc_clock,
> > -- 
> > 2.7.4
> > 
> 

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

Re: [PATCH v2 7/9] drm: Connector helper function to release atomic state

[Pandiyan, Dhinakaran]

On Wed, 2017-01-25 at 07:18 +0100, Daniel Vetter wrote:
> On Tue, Jan 24, 2017 at 03:49:35PM -0800, Dhinakaran Pandiyan wrote:
> > Having a ->atomic_release callback is useful to release shared resources
> > that get allocated in compute_config().
> > 
> > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  include/drm/drm_modeset_helper_vtables.h | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > index 46f5b34..e41b18a 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -831,6 +831,21 @@ struct drm_connector_helper_funcs {
> >  	 */
> >  	struct drm_encoder *(*atomic_best_encoder)(struct drm_connector *connector,
> >  						   struct drm_connector_state *connector_state);
> > +
> > +	/**
> > +	 * @atomic_release:
> > +	 *
> > +	 * This function is used to release shared resources that were
> > +	 * previously acquired. For example, resources acquired in
> > +	 * encoder->compute_config() can be released by calling this function
> 
> @compute_config is the right way to do references within the same struct.
> 
> > +	 * from mode_fixup()
> 
> Same here.
> 
> Patch split up is a bit strange, hence why my review of the design is in
> later patches.
> 
> Thanks, Daniel
> 


I'll squash them appropriately, thanks for the review.

-DK
> > +	 *
> > +	 * NOTE:
> > +	 *
> > +	 * This function is called in the check phase of an atomic update.
> > +	 */
> > +	void (*atomic_release)(struct drm_connector *connector,
> > +			       struct drm_connector_state *connector_state);
> >  };
> >  
> >  /**
> > -- 
> > 2.7.4
> > 
> 

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

Re: [PATCH v2 4/9] drm: Add driver private objects to atomic state

[Harry Wentland]

On 2017-01-25 12:59 AM, Daniel Vetter wrote:
> On Tue, Jan 24, 2017 at 03:49:32PM -0800, Dhinakaran Pandiyan wrote:
>> It is necessary to track states for objects other than connector, crtc
>> and plane for atomic modesets. But adding objects like DP MST link
>> bandwidth to drm_atomic_state would mean that a non-core object will be
>> modified by the core helper functions for swapping and clearing
>> it's state. So, lets add void * objects and helper functions that operate
>> on void * types to keep these objects and states private to the core.
>> Drivers can then implement specific functions to swap and clear states.
>> The other advantage having just void * for these objects in
>> drm_atomic_state is that objects of different types can be managed in the
>> same state array.
>>
>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> I think an overview DOC: section to explain how to subclass atomic state
> and how to do private state objects would be great. But I can do that once
> your patch has landed, since it'd be much more about the overall design of
> atomic (and I promised to do that anyway).
> 
> Looks pretty good, a few nits below still. I'll also ask amd folks to
> check this out, and I think Ville could use it for his cdclk stuff.

Thanks for pinging me again about this. This should help with attaching
our context to atomic_state in a clean fashion. I hope to show some
patches of it soon after I rebase on top of drm-next + these patches.

> 
>> ---
>>  drivers/gpu/drm/drm_atomic.c        | 55 +++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/drm_atomic_helper.c |  6 ++++
>>  include/drm/drm_atomic.h            | 30 ++++++++++++++++++++
>>  3 files changed, 91 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 723392f..f3a71cc 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state)
>>  	kfree(state->connectors);
>>  	kfree(state->crtcs);
>>  	kfree(state->planes);
>> +	kfree(state->private_objs);
>>  }
>>  EXPORT_SYMBOL(drm_atomic_state_default_release);
>>  
>> @@ -184,6 +185,20 @@ 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_private_objs; i++) {
>> +		void *priv_obj = state->private_objs[i].obj;
>> +		void *obj_state = state->private_objs[i].obj_state;
>> +
>> +		if (!priv_obj)
>> +			continue;
>> +
>> +		state->private_objs[i].funcs->destroy_state(obj_state);
>> +		state->private_objs[i].obj = NULL;
>> +		state->private_objs[i].obj_state = NULL;
>> +		state->private_objs[i].funcs = NULL;
>> +	}
>> +
>>  }
>>  EXPORT_SYMBOL(drm_atomic_state_default_clear);
>>  
>> @@ -976,6 +991,46 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
>>  		plane->funcs->atomic_print_state(p, state);
>>  }
>>  
>> +
> 
> Needs kerneldoc here.
> 
>> +void *
>> +drm_atomic_get_priv_obj_state(struct drm_atomic_state *state, void *obj,
> 
> ocd bikeshed: priv_obj vs private_obj inconsistency here, please stick to
> one (I don't care which one).
> 
>> +			      const struct drm_private_state_funcs *funcs)
>> +{
>> +	int index, num_objs, i;
>> +	size_t size;
>> +	struct __drm_private_objs_state *arr;
>> +
>> +	for (i = 0; i < state->num_private_objs; i++)
>> +		if (obj == state->private_objs[i].obj &&
>> +		    state->private_objs[i].obj_state)
>> +			return state->private_objs[i].obj_state;
>> +
>> +	num_objs = state->num_private_objs + 1;
>> +	size = sizeof(*state->private_objs) * num_objs;
>> +	arr = krealloc(state->private_objs, size, GFP_KERNEL);
>> +	if (!arr)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	state->private_objs = arr;
>> +	index = state->num_private_objs;
>> +	memset(&state->private_objs[index], 0, sizeof(*state->private_objs));
>> +
>> +	state->private_objs[index].obj_state = funcs->duplicate_state(state, obj);
>> +	if (!state->private_objs[index].obj_state)
>> +		return ERR_PTR(-ENOMEM);
> 
> I wondered whether we need to allow other error codes than ENOMEM, e.g.
> if duplicate_state needs to acquire a ww_mutex. But we can always acquire
> the necessary locks outside of drm_atomic_get_priv_obj_state in some
> helper/driver wrapper. So no big deal, but worth explaining that
> drm_atomic_get_priv_obj_state won't acquire necessarily locsk (since it
> doesn't know about them) in the kernel-doc.
> 
>> +
>> +	state->private_objs[index].obj = obj;
>> +	state->private_objs[index].funcs = funcs;
>> +	state->num_private_objs = num_objs;
>> +
>> +	DRM_DEBUG_ATOMIC("Added new private object state %p to %p\n",
>> +			 state->private_objs[index].obj_state, state);
>> +
>> +	return state->private_objs[index].obj_state;
>> +}
>> +EXPORT_SYMBOL(drm_atomic_get_priv_obj_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 1f0cd7e..dd34d21 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1977,6 +1977,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>>  	struct drm_plane *plane;
>>  	struct drm_plane_state *plane_state;
>>  	struct drm_crtc_commit *commit;
>> +	void *obj, *obj_state;
>> +	const struct drm_private_state_funcs *funcs;
>>  
>>  	if (stall) {
>>  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> @@ -2025,6 +2027,10 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>>  		swap(state->planes[i].state, plane->state);
>>  		plane->state->state = NULL;
>>  	}
>> +
>> +	for_each_private_obj(state, obj, obj_state, i, funcs) {
>> +		funcs->swap_state(obj, &state->private_objs[i].obj_state);
>> +	}
>>  }
>>  EXPORT_SYMBOL(drm_atomic_helper_swap_state);
>>  
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index f1cb2b0..80d6e21 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -153,6 +153,18 @@ struct __drm_connnectors_state {
>>  	struct drm_connector_state *state;
>>  };
>>  
> 
> This one needs kernel-doc too, since it's a struct that drivers/helpers
> will need to implement. Please use the inline style and document expected
> semantics in detail, like we do for other vfunc tables.
> 

Kerneldocs would definitely help.

>> +struct drm_private_state_funcs {
> 
> I also wonder whether we shouldn't give this a drm_atomic_ prefix ...

I think leaving the atomic prefix out is more consistent with the other
atomic state objects (drm_crtc_state, etc). drm_xyz_state seems to apply
atomic.

I don't have a strong preference either way, though.

Harry

> 
>> +	void (*swap_state)(void *obj, void **obj_state_ptr);
>> +	void (*destroy_state)(void *obj_state);
>> +	void *(*duplicate_state)(struct drm_atomic_state *state, void *obj);
>> +};
>> +
>> +struct __drm_private_objs_state {
>> +	void *obj;
>> +	void *obj_state;
>> +	const 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)
>> @@ -164,6 +176,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_private_objs: size of the @private_objs array
>> + * @private_objs: pointer to array of private object pointers
>>   * @acquire_ctx: acquire context for this atomic modeset state update
>>   */
>>  struct drm_atomic_state {
>> @@ -177,6 +191,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_objs_state *private_objs;
>>  
>>  	struct drm_modeset_acquire_ctx *acquire_ctx;
>>  
>> @@ -269,6 +285,11 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
>>  		struct drm_connector_state *state, struct drm_property *property,
>>  		uint64_t val);
>>  
>> +void * __must_check
>> +drm_atomic_get_priv_obj_state(struct drm_atomic_state *state,
>> +			      void *obj,
>> +			      const struct drm_private_state_funcs *funcs);
>> +
>>  /**
>>   * drm_atomic_get_existing_crtc_state - get crtc state, if it exists
>>   * @state: global atomic state object
>> @@ -413,6 +434,15 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
>>  	     (__i)++)							\
>>  		for_each_if (plane_state)
>>  
>> +#define for_each_private_obj(__state, obj, obj_state, __i, __funcs)	\
>> +	for ((__i) = 0;							\
>> +	     (__i) < (__state)->num_private_objs &&			\
>> +	     ((obj) = (__state)->private_objs[__i].obj,			\
>> +	      (__funcs) = (__state)->private_objs[__i].funcs,		\
>> +	      (obj_state) = (__state)->private_objs[__i].obj_state, 1);	\
>> +	      (__i)++)							\
>> +		for_each_if (__funcs)
> 
> You are not filtering for the function table here, which is important to
> make sure that this can be used to only walk over objects with a given
> vtable. With that we can then #define specific macros for e.g. mst:
> 
> struct drm_private_state_funcs mst_state_funcs;
> 
> #define for_each_mst_state(__state, obj, obj_state, __i, __funcs) \
> 	for_each_private_obj(__state, &mst_state_funcs, obj, obj_state, __i, __funcs)
> 
> I'd place the vfunc right after the state, since those are both input
> parameters to the macro, and specify what exactly we're looping over. To
> make this work you need something like:
> 
> #define for_each_private_obj(__state, obj_funcs, obj, obj_state, __i, __funcs)	\
> 	for ((__i) = 0;							\
> 	     (__i) < (__state)->num_private_objs &&			\
> 	     ((obj) = (__state)->private_objs[__i].obj,			\
> 	      (__funcs) = (__state)->private_objs[__i].funcs,		\
> 	      (obj_state) = (__state)->private_objs[__i].obj_state, 1);	\
> 	      (__i)++)							\
> 		for_each_if (__funcs == obj_funcs)
> 
> Note the check to compare __funcs == obj_funcs.
> 
> With that other subsytem can the filter for their own objects only with
> e.g.
> 
> #define intel_for_each_cdclk_state(__state, obj, obj_state, __i, __funcs) \
> 	for_each_private_obj(__state, &intel_cdclk_state_funcs, obj, obj_state, __i, __funcs)
> 
> Would be good to also then have kerneldoc for this iterator, to explain
> how to use it.
> -Daniel
> 
>>  /**
>>   * drm_atomic_crtc_needs_modeset - compute combined modeset need
>>   * @state: &drm_crtc_state for the CRTC
>> -- 
>> 2.7.4
>>
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 4/9] drm: Add driver private objects to atomic state

[Daniel Vetter]

On Wed, Jan 25, 2017 at 08:47:09PM +0000, Pandiyan, Dhinakaran wrote:
> On Wed, 2017-01-25 at 06:59 +0100, Daniel Vetter wrote:
> > On Tue, Jan 24, 2017 at 03:49:32PM -0800, Dhinakaran Pandiyan wrote:
> > > +#define for_each_private_obj(__state, obj, obj_state, __i, __funcs)	\
> > > +	for ((__i) = 0;							\
> > > +	     (__i) < (__state)->num_private_objs &&			\
> > > +	     ((obj) = (__state)->private_objs[__i].obj,			\
> > > +	      (__funcs) = (__state)->private_objs[__i].funcs,		\
> > > +	      (obj_state) = (__state)->private_objs[__i].obj_state, 1);	\
> > > +	      (__i)++)							\
> > > +		for_each_if (__funcs)
> > 
> > You are not filtering for the function table here, which is important to
> > make sure that this can be used to only walk over objects with a given
> > vtable. With that we can then #define specific macros for e.g. mst:
> > 
> > struct drm_private_state_funcs mst_state_funcs;
> > 
> > #define for_each_mst_state(__state, obj, obj_state, __i, __funcs) \
> > 	for_each_private_obj(__state, &mst_state_funcs, obj, obj_state, __i, __funcs)
> > 
> > I'd place the vfunc right after the state, since those are both input
> > parameters to the macro, and specify what exactly we're looping over. To
> > make this work you need something like:
> > 
> > #define for_each_private_obj(__state, obj_funcs, obj, obj_state, __i, __funcs)	\
> > 	for ((__i) = 0;							\
> > 	     (__i) < (__state)->num_private_objs &&			\
> > 	     ((obj) = (__state)->private_objs[__i].obj,			\
> > 	      (__funcs) = (__state)->private_objs[__i].funcs,		\
> > 	      (obj_state) = (__state)->private_objs[__i].obj_state, 1);	\
> > 	      (__i)++)							\
> > 		for_each_if (__funcs == obj_funcs)
> > 
> > Note the check to compare __funcs == obj_funcs.
> > 
> > With that other subsytem can the filter for their own objects only with
> > e.g.
> > 
> > #define intel_for_each_cdclk_state(__state, obj, obj_state, __i, __funcs) \
> > 	for_each_private_obj(__state, &intel_cdclk_state_funcs, obj, obj_state, __i, __funcs)
> > 
> > Would be good to also then have kerneldoc for this iterator, to explain
> > how to use it.
> > -Daniel
> > 
> 
> I see your point but we can't use this iterator in the swap_state()
> helper if we do that. I have used it to swap states for all objects
> using this version without filtering.
> 
> I guess, I can just code the iterator explicitly for swap_state() and
> re-write the iterator with the filtering.

For swap states I'd use a raw iterator with a __ prefix, which does not
have the vtable check. So yes, you need two I think.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/9] drm: Add driver private objects to atomic state

[Daniel Vetter]

On Wed, Jan 25, 2017 at 04:33:16PM -0500, Harry Wentland wrote:
> On 2017-01-25 12:59 AM, Daniel Vetter wrote:
> > On Tue, Jan 24, 2017 at 03:49:32PM -0800, Dhinakaran Pandiyan wrote:
> >> +struct drm_private_state_funcs {
> > 
> > I also wonder whether we shouldn't give this a drm_atomic_ prefix ...
> 
> I think leaving the atomic prefix out is more consistent with the other
> atomic state objects (drm_crtc_state, etc). drm_xyz_state seems to apply
> atomic.
> 
> I don't have a strong preference either way, though.

Yeah, makes sense ... I suggested the atomic prefix since it's (at least
right now) only used for atomic updates. But just calling them
drm_private_{state,obj} makes sense too.
-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: [Intel-gfx] [PATCH v2 8/9] drm/dp: Release DP MST shared link bandwidth

[Daniel Vetter]

On Wed, Jan 25, 2017 at 08:53:18PM +0000, Pandiyan, Dhinakaran wrote:
> On Wed, 2017-01-25 at 07:16 +0100, Daniel Vetter wrote:
> > On Tue, Jan 24, 2017 at 03:49:36PM -0800, Dhinakaran Pandiyan wrote:
> > > Implement the ->atomic_release() callback to release the shared link
> > > bandwidth that was originally acquired during compute_config()
> > > 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp_mst.c | 28 ++++++++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > index f51574f..2f57a56 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > @@ -78,6 +78,33 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> > >  
> > >  }
> > >  
> > > +static void intel_dp_mst_atomic_release(struct drm_connector *connector,
> > > +					struct drm_connector_state *conn_state)
> > > +{
> > > +	struct intel_dp_mst_encoder *intel_mst;
> > > +	struct drm_dp_mst_topology_mgr *mgr;
> > > +	struct drm_dp_mst_topology_state *topology_state;
> > > +	struct drm_encoder *encoder;
> > > +	struct intel_connector *intel_connector = to_intel_connector(connector);
> > > +
> > > +	encoder = connector->state->best_encoder;
> > > +	if (!encoder || to_intel_encoder(encoder)->type != INTEL_OUTPUT_DP_MST)
> > > +		return;
> > 
> > NULL encoder should imo be caught in core. Type != DP_MST is impossible,
> > if you're unsure make it into a BUG_ON for testing.
> 
> 
> I don't get why (type != INTEL_OUTPUT_DP_MST) is impossible. Is that
> because we have the implementation for atomic_release() only for MST
> connectors? But, that is only for now.

Yes. And if we add atomic_release for other encoders, then ofcourse we
will not reuse the mst one there. See all the other intel_dp_mst_* hooks
in the same vfunc table, those also don't check for the encoder type. This
would be like doing a runtime type check in C++ for the "this" pointer :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 9/9] drm/dp: Track MST link bandwidth

[Daniel Vetter]

On Wed, Jan 25, 2017 at 09:00:02PM +0000, Pandiyan, Dhinakaran wrote:
> On Wed, 2017-01-25 at 07:15 +0100, Daniel Vetter wrote:
> > On Tue, Jan 24, 2017 at 03:49:37PM -0800, Dhinakaran Pandiyan wrote:
> > > Use the added helpers to track MST link bandwidth for atomic modesets.
> > > Link bw is acquired in the ->atomic_check() phase when CRTCs are being
> > > enabled with drm_atomic_find_vcpi_slots() instead of drm_find_vcpi_slots().
> > > Similarly, link bw is released during ->atomic_check() with the connector
> > > helper callback ->atomic_release() when CRTCs are disabled.
> > > 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c |  9 ++++++++-
> > >  drivers/gpu/drm/i915/intel_dp_mst.c | 13 ++++++++++++-
> > >  2 files changed, 20 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index dd34d21..0d42173 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -385,8 +385,15 @@ 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) {
> > > +			const struct drm_connector_helper_funcs *conn_funcs;
> > > +
> > > +			conn_funcs = connector->helper_private;
> > > +			if (conn_funcs->atomic_release)
> > > +				conn_funcs->atomic_release(connector,
> > > +							   conn_state);
> > 
> > I wonder whether this won't surprise drivers: The way you coded this
> > release only gets called when the connector is fully disabled. But it does
> > not get called when you atomically switch it from one crtc to the other
> > (in which case you also might want to release resources, before allocating
> > new ones). I think that case of directly switching stuff is even a bug in
> > your current atomic tracking code ...
> > 
> > We also need to extract the release calls into a separate loop which runs
> > _before_ we allocate any resources. Otherwise if you do an atomic commit
> > where you disable connector2 and enable connector1 and they can't run both
> > at the same time it'll fail, because we'll release the vcpi for connector2
> > only after we've tried to get it for connnector1.
> > 
> 
> 
> Yeah, you are right. I thought of this problem and then forgot about it.
> Is there any igt test to test the switching?

Not sure we have any direct mode-switching tests. Please chat with Mika
Kahola and make sure we do have that test coverage. Mika is working on
atomic test coverage, and having this would be good in general (not just
for mst).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 7/9] drm: Connector helper function to release atomic state

[Pandiyan, Dhinakaran]

On Wed, 2017-01-25 at 07:18 +0100, Daniel Vetter wrote:
> On Tue, Jan 24, 2017 at 03:49:35PM -0800, Dhinakaran Pandiyan wrote:
> > Having a ->atomic_release callback is useful to release shared resources
> > that get allocated in compute_config().
> > 
> > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  include/drm/drm_modeset_helper_vtables.h | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > index 46f5b34..e41b18a 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -831,6 +831,21 @@ struct drm_connector_helper_funcs {
> >  	 */
> >  	struct drm_encoder *(*atomic_best_encoder)(struct drm_connector *connector,
> >  						   struct drm_connector_state *connector_state);
> > +
> > +	/**
> > +	 * @atomic_release:
> > +	 *
> > +	 * This function is used to release shared resources that were
> > +	 * previously acquired. For example, resources acquired in
> > +	 * encoder->compute_config() can be released by calling this function
> 
> @compute_config is the right way to do references within the same struct.

compute_config is not in the same structure, which made me realize I
should not be referring to compute_config() at all, as it is a helper
for struct intel_encoder. 


-DK
> 
> > +	 * from mode_fixup()
> 
> Same here.
> 
> Patch split up is a bit strange, hence why my review of the design is in
> later patches.
> 
> Thanks, Daniel
> 
> > +	 *
> > +	 * NOTE:
> > +	 *
> > +	 * This function is called in the check phase of an atomic update.
> > +	 */
> > +	void (*atomic_release)(struct drm_connector *connector,
> > +			       struct drm_connector_state *connector_state);
> >  };
> >  
> >  /**
> > -- 
> > 2.7.4
> > 
> 

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