[PATCH v4 0/8] Adding driver-private objects to atomic state

[PATCH v4 0/8] Adding driver-private objects to atomic state

[Dhinakaran Pandiyan]

Link bandwidth is a shared resource between multiple displays in DP MST
configurations. 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 when multiple CRTC's and
connectors are involved.

Managing shared resources like DP MST link bandwidth in the driver's
subclassed atomic_state will result in duplicating the code in each atomic
modesetting driver. But adding objects like DP MST link bandwidth to the
DRM core's drm_atomic_state would mean that an object that is not a core
modesetting object like connector, CRTC or a plane will be modified by the
helper functions for swapping and clearing state. So, this series
introduces void * type driver-private objects in drm_atomic_state and adds
helper functions that operate on these private objects. Drivers can then
implement object-specific functions to swap and clear states.
The advantage of having void * for these objects in drm_atomic_state is
that objects of different types can be managed in the same state array.

This rebased version includes a few minor changes -
1) Used for_each_oldnew_connector_in_state() macro (7/8)
2) Added a WARN_ON() to check for connection_mutex (5/8)
3) Alignment fix. (4/8)

Pandiyan, Dhinakaran (8):
  drm/dp: Kill total_pbn and total_slots in struct
    drm_dp_mst_topology_mgr
  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 to track available link bandwidth
  drm/dp: Add DP MST helpers to atomically find and release vcpi slots
  drm: Connector helper function to release resources
  drm/dp: Track MST link bandwidth

 drivers/gpu/drm/drm_atomic.c             |  68 ++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c      |  24 ++++
 drivers/gpu/drm/drm_dp_mst_topology.c    | 185 ++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_dp_mst.c      |  42 +++++--
 drivers/gpu/drm/nouveau/nv50_display.c   |   3 +-
 drivers/gpu/drm/radeon/radeon_dp_mst.c   |   4 +-
 include/drm/drm_atomic.h                 |  93 ++++++++++++++++
 include/drm/drm_dp_mst_helper.h          |  33 ++++--
 include/drm/drm_modeset_helper_vtables.h |  13 +++
 9 files changed, 427 insertions(+), 38 deletions(-)

-- 
2.7.4

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

[PATCH v4 1/8] drm/dp: Kill total_pbn and total_slots in struct drm_dp_mst_topology_mgr

[Dhinakaran Pandiyan]

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

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 next 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.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Harry Wentland <Harry.wentland@amd.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 5 ++---
 include/drm/drm_dp_mst_helper.h       | 9 +--------
 2 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index f2cc375..66a611a 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2042,9 +2042,8 @@ 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;
+		/* max. time slots - one slot for MTP header */
+		mgr->avail_slots = 63;
 
 		/* add initial branch device at LCT 1 */
 		mstb = drm_dp_add_mst_branch_device(1, NULL);
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index f4b4d15..1a7e0f4 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -479,18 +479,11 @@ 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 &drm_dp_mst_branch.txslost and
-- 
2.7.4

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

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

[Dhinakaran Pandiyan]

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

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, 63. So, let's make that check obvious and remove
avail_slots. While at it, make debug messages more descriptive.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Harry Wentland <Harry.wentland@amd.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 15 ++++++++-------
 include/drm/drm_dp_mst_helper.h       |  5 -----
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 66a611a..2e2af13 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2042,9 +2042,6 @@ 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->avail_slots = 63;
-
 		/* add initial branch device at LCT 1 */
 		mstb = drm_dp_add_mst_branch_device(1, NULL);
 		if (mstb == NULL) {
@@ -2474,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;
 }
@@ -2488,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;
@@ -2527,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 1a7e0f4..d836511 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -481,11 +481,6 @@ struct drm_dp_mst_topology_mgr {
 	int pbn_div;
 
 	/**
-	 * @avail_slots: Still available slots that can be allocated.
-	 */
-	int avail_slots;
-
-	/**
 	 * @qlock: protects @tx_msg_downq, the &drm_dp_mst_branch.txslost and
 	 * &drm_dp_sideband_msg_tx.state once they are queued
 	 */
-- 
2.7.4

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

[PATCH v4 3/8] drm/dp: Split drm_dp_mst_allocate_vcpi

[Dhinakaran Pandiyan]

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

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>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c  | 21 ++++++++++-----------
 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        |  3 ++-
 5 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 2e2af13..d3fc7e4 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,8 @@ 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 +2513,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 +2532,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 094cbdc..c1f62eb 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -149,7 +149,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.
@@ -165,7 +164,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 7ad1ee5..418872b 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -2895,7 +2895,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 d836511..5b02476 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -567,7 +567,8 @@ 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 v4 4/8] drm: Add driver-private objects to atomic state

[Dhinakaran Pandiyan]

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

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.

v2: Added docs and new iterator to filter private objects (Daniel)
v3: Macro alignment (Chris)

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Harry Wentland <Harry.wentland@amd.com>
Acked-by: Harry Wentland <harry.wentland@amd.com>
Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_atomic.c        | 68 +++++++++++++++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c |  5 ++
 include/drm/drm_atomic.h            | 93 +++++++++++++++++++++++++++++++++++++
 3 files changed, 166 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 9b892af..d89be5c 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 *private_obj = state->private_objs[i].obj;
+		void *obj_state = state->private_objs[i].obj_state;
+
+		if (!private_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);
 
@@ -978,6 +993,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
 }
 
 /**
+ * drm_atomic_get_private_obj_state - get private object state
+ * @state: global atomic state
+ * @obj: private object to get the state for
+ * @funcs: pointer to the struct of function pointers that identify the object
+ * type
+ *
+ * This function returns the private object state for the given private object,
+ * allocating the state if needed. It does not grab any locks as the caller is
+ * expected to care of any required locking.
+ *
+ * RETURNS:
+ *
+ * Either the allocated state or the error code encoded into a pointer.
+ */
+void *
+drm_atomic_get_private_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_private_obj_state);
+
+/**
  * drm_atomic_get_connector_state - get connector state
  * @state: global atomic state object
  * @connector: connector to get state object for
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 4e26b73..1403334 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2001,6 +2001,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 	struct drm_plane *plane;
 	struct drm_plane_state *old_plane_state, *new_plane_state;
 	struct drm_crtc_commit *commit;
+	void *obj, *obj_state;
+	const struct drm_private_state_funcs *funcs;
 
 	if (stall) {
 		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
@@ -2061,6 +2063,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 		state->planes[i].state = old_plane_state;
 		plane->state = new_plane_state;
 	}
+
+	__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 0147a04..c17da39 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -155,6 +155,53 @@ struct __drm_connnectors_state {
 };
 
 /**
+ * struct drm_private_state_funcs - atomic state functions for private objects
+ *
+ * These hooks are used by atomic helpers to create, swap and destroy states of
+ * private objects. The structure itself is used as a vtable to identify the
+ * associated private object type. Each private object type that needs to be
+ * added to the atomic states is expected to have an implementation of these
+ * hooks and pass a pointer to it's drm_private_state_funcs struct to
+ * drm_atomic_get_private_obj_state().
+ */
+struct drm_private_state_funcs {
+	/**
+	 * @duplicate_state:
+	 *
+	 * Duplicate the current state of the private object and return it. It
+	 * is an error to call this before obj->state has been initialized.
+	 *
+	 * RETURNS:
+	 *
+	 * Duplicated atomic state or NULL when obj->state is not
+	 * initialized or allocation failed.
+	 */
+	void *(*duplicate_state)(struct drm_atomic_state *state, void *obj);
+
+	/**
+	 * @swap_state:
+	 *
+	 * This function swaps the existing state of a private object @obj with
+	 * it's newly created state, the pointer to which is passed as
+	 * @obj_state_ptr.
+	 */
+	void (*swap_state)(void *obj, void **obj_state_ptr);
+
+	/**
+	 * @destroy_state:
+	 *
+	 * Frees the private object state created with @duplicate_state.
+	 */
+	void (*destroy_state)(void *obj_state);
+};
+
+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)
  * @dev: parent DRM device
@@ -165,6 +212,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 {
@@ -178,6 +227,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;
 
@@ -270,6 +321,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_private_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
@@ -598,6 +654,43 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
 		for_each_if (plane)
 
 /**
+ * __for_each_private_obj - iterate over all private objects
+ * @__state: the atomic state
+ *
+ * This macro iterates over the array containing private object data in atomic
+ * 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)
+
+/**
+ * for_each_private_obj - iterate over a specify type of private object
+ * @__state: the atomic state
+ * @obj_funcs: function table to filter the private objects
+ *
+ * This macro iterates over the private objects state array while filtering the
+ * objects based on the vfunc table that is passed as @obj_funcs. New macros
+ * can be created by passing in the vfunc table associated with a specific
+ * private object.
+ */
+#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)
+
+/**
  * 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 v4 4/8] drm: Add driver-private objects to atomic state

[Maarten Lankhorst]

Op 16-03-17 om 08:10 schreef Dhinakaran Pandiyan:
> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>
> 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.
>
> v2: Added docs and new iterator to filter private objects (Daniel)
> v3: Macro alignment (Chris)
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Harry Wentland <Harry.wentland@amd.com>
> Acked-by: Harry Wentland <harry.wentland@amd.com>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Mostly looks good, but too many null checks. I think it's best to get rid of them all
by freeing state->driver_private in default_clear() or setting num_private_objs to 0.
It would remove the need for all null checks I think..

~Maarten

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

Re: [PATCH v4 4/8] drm: Add driver-private objects to atomic state

[Pandiyan, Dhinakaran]

On Wed, 2017-03-22 at 11:00 +0100, Maarten Lankhorst wrote:
> Op 16-03-17 om 08:10 schreef Dhinakaran Pandiyan:
> > From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> >
> > 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.
> >
> > v2: Added docs and new iterator to filter private objects (Daniel)
> > v3: Macro alignment (Chris)
> >
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Archit Taneja <architt@codeaurora.org>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Harry Wentland <Harry.wentland@amd.com>
> > Acked-by: Harry Wentland <harry.wentland@amd.com>
> > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> Mostly looks good, but too many null checks. I think it's best to get rid of them all
> by freeing state->driver_private in default_clear() or setting num_private_objs to 0.
> It would remove the need for all null checks I think..
> 
> ~Maarten
> 

Did you mean the NULL checks in this loop inside
drm_atomic_get_private_obj_state()

+       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;

and the fact that I am not setting num_private_objs = 0 in
drm_atomic_state_default_clear() ?

-DK
> _______________________________________________
> 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: [Intel-gfx] [PATCH v4 4/8] drm: Add driver-private objects to atomic state

[Maarten Lankhorst]

Op 22-03-17 om 22:05 schreef Pandiyan, Dhinakaran:
> On Wed, 2017-03-22 at 11:00 +0100, Maarten Lankhorst wrote:
>> Op 16-03-17 om 08:10 schreef Dhinakaran Pandiyan:
>>> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>>>
>>> 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.
>>>
>>> v2: Added docs and new iterator to filter private objects (Daniel)
>>> v3: Macro alignment (Chris)
>>>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: Archit Taneja <architt@codeaurora.org>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Harry Wentland <Harry.wentland@amd.com>
>>> Acked-by: Harry Wentland <harry.wentland@amd.com>
>>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> Mostly looks good, but too many null checks. I think it's best to get rid of them all
>> by freeing state->driver_private in default_clear() or setting num_private_objs to 0.
>> It would remove the need for all null checks I think..
>>
>> ~Maarten
>>
> Did you mean the NULL checks in this loop inside
> drm_atomic_get_private_obj_state()
>
> +       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;
>
> and the fact that I am not setting num_private_objs = 0 in
> drm_atomic_state_default_clear() ?
All of them, the NULL check in default_clear goes away, same for get_private_obj_state, and __for_each_private_obj
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 4/8] drm: Add driver-private objects to atomic state

[Dhinakaran Pandiyan]

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

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.

v4: Avoid redundant NULL checks when private_objs array is empty (Maarten)
v3: Macro alignment (Chris)
v2: Added docs and new iterator to filter private objects (Daniel)

Acked-by: Harry Wentland <harry.wentland@amd.com>
Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_atomic.c        | 69 +++++++++++++++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c |  5 ++
 include/drm/drm_atomic.h            | 93 +++++++++++++++++++++++++++++++++++++
 3 files changed, 167 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 9b892af..e590148 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,21 @@ 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 *private_obj = state->private_objs[i].obj;
+		void *obj_state = state->private_objs[i].obj_state;
+
+		if (!private_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;
+	}
+	state->num_private_objs = 0;
+
 }
 EXPORT_SYMBOL(drm_atomic_state_default_clear);
 
@@ -978,6 +994,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
 }
 
 /**
+ * drm_atomic_get_private_obj_state - get private object state
+ * @state: global atomic state
+ * @obj: private object to get the state for
+ * @funcs: pointer to the struct of function pointers that identify the object
+ * type
+ *
+ * This function returns the private object state for the given private object,
+ * allocating the state if needed. It does not grab any locks as the caller is
+ * expected to care of any required locking.
+ *
+ * RETURNS:
+ *
+ * Either the allocated state or the error code encoded into a pointer.
+ */
+void *
+drm_atomic_get_private_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_private_obj_state);
+
+/**
  * drm_atomic_get_connector_state - get connector state
  * @state: global atomic state object
  * @connector: connector to get state object for
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 4e26b73..1403334 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2001,6 +2001,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 	struct drm_plane *plane;
 	struct drm_plane_state *old_plane_state, *new_plane_state;
 	struct drm_crtc_commit *commit;
+	void *obj, *obj_state;
+	const struct drm_private_state_funcs *funcs;
 
 	if (stall) {
 		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
@@ -2061,6 +2063,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 		state->planes[i].state = old_plane_state;
 		plane->state = new_plane_state;
 	}
+
+	__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 0147a04..c17da39 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -155,6 +155,53 @@ struct __drm_connnectors_state {
 };
 
 /**
+ * struct drm_private_state_funcs - atomic state functions for private objects
+ *
+ * These hooks are used by atomic helpers to create, swap and destroy states of
+ * private objects. The structure itself is used as a vtable to identify the
+ * associated private object type. Each private object type that needs to be
+ * added to the atomic states is expected to have an implementation of these
+ * hooks and pass a pointer to it's drm_private_state_funcs struct to
+ * drm_atomic_get_private_obj_state().
+ */
+struct drm_private_state_funcs {
+	/**
+	 * @duplicate_state:
+	 *
+	 * Duplicate the current state of the private object and return it. It
+	 * is an error to call this before obj->state has been initialized.
+	 *
+	 * RETURNS:
+	 *
+	 * Duplicated atomic state or NULL when obj->state is not
+	 * initialized or allocation failed.
+	 */
+	void *(*duplicate_state)(struct drm_atomic_state *state, void *obj);
+
+	/**
+	 * @swap_state:
+	 *
+	 * This function swaps the existing state of a private object @obj with
+	 * it's newly created state, the pointer to which is passed as
+	 * @obj_state_ptr.
+	 */
+	void (*swap_state)(void *obj, void **obj_state_ptr);
+
+	/**
+	 * @destroy_state:
+	 *
+	 * Frees the private object state created with @duplicate_state.
+	 */
+	void (*destroy_state)(void *obj_state);
+};
+
+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)
  * @dev: parent DRM device
@@ -165,6 +212,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 {
@@ -178,6 +227,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;
 
@@ -270,6 +321,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_private_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
@@ -598,6 +654,43 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
 		for_each_if (plane)
 
 /**
+ * __for_each_private_obj - iterate over all private objects
+ * @__state: the atomic state
+ *
+ * This macro iterates over the array containing private object data in atomic
+ * 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)
+
+/**
+ * for_each_private_obj - iterate over a specify type of private object
+ * @__state: the atomic state
+ * @obj_funcs: function table to filter the private objects
+ *
+ * This macro iterates over the private objects state array while filtering the
+ * objects based on the vfunc table that is passed as @obj_funcs. New macros
+ * can be created by passing in the vfunc table associated with a specific
+ * private object.
+ */
+#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)
+
+/**
  * 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 v5 4/8] drm: Add driver-private objects to atomic state

[Maarten Lankhorst]

Hey,

There are still 2 unnecessary NULL checks, afaict.

Op 22-03-17 om 23:30 schreef Dhinakaran Pandiyan:
> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>
> 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.
>
> v4: Avoid redundant NULL checks when private_objs array is empty (Maarten)
> v3: Macro alignment (Chris)
> v2: Added docs and new iterator to filter private objects (Daniel)
>
> Acked-by: Harry Wentland <harry.wentland@amd.com>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        | 69 +++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c |  5 ++
>  include/drm/drm_atomic.h            | 93 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 167 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 9b892af..e590148 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,21 @@ 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 *private_obj = state->private_objs[i].obj;
> +		void *obj_state = state->private_objs[i].obj_state;
> +
> +		if (!private_obj)
> +			continue;
^This one.
> +		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;
> +	}
> +	state->num_private_objs = 0;
> +
>  }
>  EXPORT_SYMBOL(drm_atomic_state_default_clear);
>  
> @@ -978,6 +994,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
>  }
>  
>  /**
> + * drm_atomic_get_private_obj_state - get private object state
> + * @state: global atomic state
> + * @obj: private object to get the state for
> + * @funcs: pointer to the struct of function pointers that identify the object
> + * type
> + *
> + * This function returns the private object state for the given private object,
> + * allocating the state if needed. It does not grab any locks as the caller is
> + * expected to care of any required locking.
> + *
> + * RETURNS:
> + *
> + * Either the allocated state or the error code encoded into a pointer.
> + */
> +void *
> +drm_atomic_get_private_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_private_obj_state);
> +
> +/**
>   * drm_atomic_get_connector_state - get connector state
>   * @state: global atomic state object
>   * @connector: connector to get state object for
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 4e26b73..1403334 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2001,6 +2001,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  	struct drm_plane *plane;
>  	struct drm_plane_state *old_plane_state, *new_plane_state;
>  	struct drm_crtc_commit *commit;
> +	void *obj, *obj_state;
> +	const struct drm_private_state_funcs *funcs;
>  
>  	if (stall) {
>  		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> @@ -2061,6 +2063,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  		state->planes[i].state = old_plane_state;
>  		plane->state = new_plane_state;
>  	}
> +
> +	__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 0147a04..c17da39 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -155,6 +155,53 @@ struct __drm_connnectors_state {
>  };
>  
>  /**
> + * struct drm_private_state_funcs - atomic state functions for private objects
> + *
> + * These hooks are used by atomic helpers to create, swap and destroy states of
> + * private objects. The structure itself is used as a vtable to identify the
> + * associated private object type. Each private object type that needs to be
> + * added to the atomic states is expected to have an implementation of these
> + * hooks and pass a pointer to it's drm_private_state_funcs struct to
> + * drm_atomic_get_private_obj_state().
> + */
> +struct drm_private_state_funcs {
> +	/**
> +	 * @duplicate_state:
> +	 *
> +	 * Duplicate the current state of the private object and return it. It
> +	 * is an error to call this before obj->state has been initialized.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * Duplicated atomic state or NULL when obj->state is not
> +	 * initialized or allocation failed.
> +	 */
> +	void *(*duplicate_state)(struct drm_atomic_state *state, void *obj);
> +
> +	/**
> +	 * @swap_state:
> +	 *
> +	 * This function swaps the existing state of a private object @obj with
> +	 * it's newly created state, the pointer to which is passed as
> +	 * @obj_state_ptr.
> +	 */
> +	void (*swap_state)(void *obj, void **obj_state_ptr);
> +
> +	/**
> +	 * @destroy_state:
> +	 *
> +	 * Frees the private object state created with @duplicate_state.
> +	 */
> +	void (*destroy_state)(void *obj_state);
> +};
> +
> +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)
>   * @dev: parent DRM device
> @@ -165,6 +212,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 {
> @@ -178,6 +227,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;
>  
> @@ -270,6 +321,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_private_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
> @@ -598,6 +654,43 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
>  		for_each_if (plane)
>  
>  /**
> + * __for_each_private_obj - iterate over all private objects
> + * @__state: the atomic state
> + *
> + * This macro iterates over the array containing private object data in atomic
> + * 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)
^This.
> +/**
> + * for_each_private_obj - iterate over a specify type of private object
> + * @__state: the atomic state
> + * @obj_funcs: function table to filter the private objects
> + *
> + * This macro iterates over the private objects state array while filtering the
> + * objects based on the vfunc table that is passed as @obj_funcs. New macros
> + * can be created by passing in the vfunc table associated with a specific
> + * private object.
> + */
> +#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)
> +
> +/**
>   * drm_atomic_crtc_needs_modeset - compute combined modeset need
>   * @state: &drm_crtc_state for the CRTC
>   *

~Maarten

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

Re: [PATCH v5 4/8] drm: Add driver-private objects to atomic state

[Daniel Vetter]

On Wed, Mar 22, 2017 at 03:30:49PM -0700, Dhinakaran Pandiyan wrote:
> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> 
> 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.
> 
> v4: Avoid redundant NULL checks when private_objs array is empty (Maarten)
> v3: Macro alignment (Chris)
> v2: Added docs and new iterator to filter private objects (Daniel)
> 
> Acked-by: Harry Wentland <harry.wentland@amd.com>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        | 69 +++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c |  5 ++
>  include/drm/drm_atomic.h            | 93 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 167 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 9b892af..e590148 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,21 @@ 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 *private_obj = state->private_objs[i].obj;
> +		void *obj_state = state->private_objs[i].obj_state;
> +
> +		if (!private_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;
> +	}
> +	state->num_private_objs = 0;

Here we set num_private_objs = 0;

> +
>  }
>  EXPORT_SYMBOL(drm_atomic_state_default_clear);
>  
> @@ -978,6 +994,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
>  }
>  
>  /**
> + * drm_atomic_get_private_obj_state - get private object state
> + * @state: global atomic state
> + * @obj: private object to get the state for
> + * @funcs: pointer to the struct of function pointers that identify the object
> + * type
> + *
> + * This function returns the private object state for the given private object,
> + * allocating the state if needed. It does not grab any locks as the caller is
> + * expected to care of any required locking.
> + *
> + * RETURNS:
> + *
> + * Either the allocated state or the error code encoded into a pointer.
> + */
> +void *
> +drm_atomic_get_private_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);

But here we unconditionally realloc to a presumably smaller size. If you
look at drm_atomic_state->num_connector (which also does dynamic array
realloc), that one works a bit differently (and hence needs these NULL
checks).

I think aligning with how we do things with connectors, for consistency
(no other reason really) would be good.

Just noticed this while reading Maarten's review, which seems to go even
farther away from how we handle this for connectors.
-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 v5 4/8] drm: Add driver-private objects to atomic state

[Maarten Lankhorst]

Op 27-03-17 om 08:38 schreef Daniel Vetter:
> On Wed, Mar 22, 2017 at 03:30:49PM -0700, Dhinakaran Pandiyan wrote:
>> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>>
>> 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.
>>
>> v4: Avoid redundant NULL checks when private_objs array is empty (Maarten)
>> v3: Macro alignment (Chris)
>> v2: Added docs and new iterator to filter private objects (Daniel)
>>
>> Acked-by: Harry Wentland <harry.wentland@amd.com>
>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> ---
>>  drivers/gpu/drm/drm_atomic.c        | 69 +++++++++++++++++++++++++++
>>  drivers/gpu/drm/drm_atomic_helper.c |  5 ++
>>  include/drm/drm_atomic.h            | 93 +++++++++++++++++++++++++++++++++++++
>>  3 files changed, 167 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 9b892af..e590148 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,21 @@ 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 *private_obj = state->private_objs[i].obj;
>> +		void *obj_state = state->private_objs[i].obj_state;
>> +
>> +		if (!private_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;
>> +	}
>> +	state->num_private_objs = 0;
> Here we set num_private_objs = 0;
>
>> +
>>  }
>>  EXPORT_SYMBOL(drm_atomic_state_default_clear);
>>  
>> @@ -978,6 +994,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
>>  }
>>  
>>  /**
>> + * drm_atomic_get_private_obj_state - get private object state
>> + * @state: global atomic state
>> + * @obj: private object to get the state for
>> + * @funcs: pointer to the struct of function pointers that identify the object
>> + * type
>> + *
>> + * This function returns the private object state for the given private object,
>> + * allocating the state if needed. It does not grab any locks as the caller is
>> + * expected to care of any required locking.
>> + *
>> + * RETURNS:
>> + *
>> + * Either the allocated state or the error code encoded into a pointer.
>> + */
>> +void *
>> +drm_atomic_get_private_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);
> But here we unconditionally realloc to a presumably smaller size. If you
> look at drm_atomic_state->num_connector (which also does dynamic array
> realloc), that one works a bit differently (and hence needs these NULL
> checks).
>
> I think aligning with how we do things with connectors, for consistency
> (no other reason really) would be good.
>
> Just noticed this while reading Maarten's review, which seems to go even
> farther away from how we handle this for connectors.
> -Daniel

Connectors are handled differently, because there's a fixed number of connectors and each
connector is assigned to its slot at state->connectors[drm_connector_index];

For private objects this is not the case, there's no way to put them in a fixed index,
so the array is resized and reallocated as needed. If you care about the realloc to a smaller
size, add a separate variable max_private_objs and multiply its size by 2 every time it's
not big enough. This also changes get_private_obj_state from O(n²) to O(n log(n)).

I don't propose you should though, because N is small enough and the increased complexity
isn't worth the decreased readability. So just set num to zero and don't worry about null
checks. :)

~Maarten


~Maarten

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

Re: [PATCH v5 4/8] drm: Add driver-private objects to atomic state

[Daniel Vetter]

On Mon, Mar 27, 2017 at 10:28:42AM +0200, Maarten Lankhorst wrote:
> Op 27-03-17 om 08:38 schreef Daniel Vetter:
> > On Wed, Mar 22, 2017 at 03:30:49PM -0700, Dhinakaran Pandiyan wrote:
> >> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> >>
> >> 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.
> >>
> >> v4: Avoid redundant NULL checks when private_objs array is empty (Maarten)
> >> v3: Macro alignment (Chris)
> >> v2: Added docs and new iterator to filter private objects (Daniel)
> >>
> >> Acked-by: Harry Wentland <harry.wentland@amd.com>
> >> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >> ---
> >>  drivers/gpu/drm/drm_atomic.c        | 69 +++++++++++++++++++++++++++
> >>  drivers/gpu/drm/drm_atomic_helper.c |  5 ++
> >>  include/drm/drm_atomic.h            | 93 +++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 167 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> index 9b892af..e590148 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,21 @@ 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 *private_obj = state->private_objs[i].obj;
> >> +		void *obj_state = state->private_objs[i].obj_state;
> >> +
> >> +		if (!private_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;
> >> +	}
> >> +	state->num_private_objs = 0;
> > Here we set num_private_objs = 0;
> >
> >> +
> >>  }
> >>  EXPORT_SYMBOL(drm_atomic_state_default_clear);
> >>  
> >> @@ -978,6 +994,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
> >>  }
> >>  
> >>  /**
> >> + * drm_atomic_get_private_obj_state - get private object state
> >> + * @state: global atomic state
> >> + * @obj: private object to get the state for
> >> + * @funcs: pointer to the struct of function pointers that identify the object
> >> + * type
> >> + *
> >> + * This function returns the private object state for the given private object,
> >> + * allocating the state if needed. It does not grab any locks as the caller is
> >> + * expected to care of any required locking.
> >> + *
> >> + * RETURNS:
> >> + *
> >> + * Either the allocated state or the error code encoded into a pointer.
> >> + */
> >> +void *
> >> +drm_atomic_get_private_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);
> > But here we unconditionally realloc to a presumably smaller size. If you
> > look at drm_atomic_state->num_connector (which also does dynamic array
> > realloc), that one works a bit differently (and hence needs these NULL
> > checks).
> >
> > I think aligning with how we do things with connectors, for consistency
> > (no other reason really) would be good.
> >
> > Just noticed this while reading Maarten's review, which seems to go even
> > farther away from how we handle this for connectors.
> > -Daniel
> 
> Connectors are handled differently, because there's a fixed number of connectors and each
> connector is assigned to its slot at state->connectors[drm_connector_index];
> 
> For private objects this is not the case, there's no way to put them in a fixed index,
> so the array is resized and reallocated as needed. If you care about the realloc to a smaller
> size, add a separate variable max_private_objs and multiply its size by 2 every time it's
> not big enough. This also changes get_private_obj_state from O(n²) to O(n log(n)).
> 
> I don't propose you should though, because N is small enough and the increased complexity
> isn't worth the decreased readability. So just set num to zero and don't worry about null
> checks. :)

Hm, in that case shouldn't we also kfree the allocation in default_clear?
Makes no sense resetting to 0 and not freeing, when we do an
unconditional krealloc afterwards. That's the part that confused me ...
I'm not worried about the realloc overahead (and that's easy to fix
indeed).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 4/8] drm: Add driver-private objects to atomic state

[Maarten Lankhorst]

Op 27-03-17 om 10:31 schreef Daniel Vetter:
> On Mon, Mar 27, 2017 at 10:28:42AM +0200, Maarten Lankhorst wrote:
>> Op 27-03-17 om 08:38 schreef Daniel Vetter:
>>> On Wed, Mar 22, 2017 at 03:30:49PM -0700, Dhinakaran Pandiyan wrote:
>>>> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>>>>
>>>> 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.
>>>>
>>>> v4: Avoid redundant NULL checks when private_objs array is empty (Maarten)
>>>> v3: Macro alignment (Chris)
>>>> v2: Added docs and new iterator to filter private objects (Daniel)
>>>>
>>>> Acked-by: Harry Wentland <harry.wentland@amd.com>
>>>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_atomic.c        | 69 +++++++++++++++++++++++++++
>>>>  drivers/gpu/drm/drm_atomic_helper.c |  5 ++
>>>>  include/drm/drm_atomic.h            | 93 +++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 167 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>>> index 9b892af..e590148 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,21 @@ 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 *private_obj = state->private_objs[i].obj;
>>>> +		void *obj_state = state->private_objs[i].obj_state;
>>>> +
>>>> +		if (!private_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;
>>>> +	}
>>>> +	state->num_private_objs = 0;
>>> Here we set num_private_objs = 0;
>>>
>>>> +
>>>>  }
>>>>  EXPORT_SYMBOL(drm_atomic_state_default_clear);
>>>>  
>>>> @@ -978,6 +994,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
>>>>  }
>>>>  
>>>>  /**
>>>> + * drm_atomic_get_private_obj_state - get private object state
>>>> + * @state: global atomic state
>>>> + * @obj: private object to get the state for
>>>> + * @funcs: pointer to the struct of function pointers that identify the object
>>>> + * type
>>>> + *
>>>> + * This function returns the private object state for the given private object,
>>>> + * allocating the state if needed. It does not grab any locks as the caller is
>>>> + * expected to care of any required locking.
>>>> + *
>>>> + * RETURNS:
>>>> + *
>>>> + * Either the allocated state or the error code encoded into a pointer.
>>>> + */
>>>> +void *
>>>> +drm_atomic_get_private_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);
>>> But here we unconditionally realloc to a presumably smaller size. If you
>>> look at drm_atomic_state->num_connector (which also does dynamic array
>>> realloc), that one works a bit differently (and hence needs these NULL
>>> checks).
>>>
>>> I think aligning with how we do things with connectors, for consistency
>>> (no other reason really) would be good.
>>>
>>> Just noticed this while reading Maarten's review, which seems to go even
>>> farther away from how we handle this for connectors.
>>> -Daniel
>> Connectors are handled differently, because there's a fixed number of connectors and each
>> connector is assigned to its slot at state->connectors[drm_connector_index];
>>
>> For private objects this is not the case, there's no way to put them in a fixed index,
>> so the array is resized and reallocated as needed. If you care about the realloc to a smaller
>> size, add a separate variable max_private_objs and multiply its size by 2 every time it's
>> not big enough. This also changes get_private_obj_state from O(n²) to O(n log(n)).
>>
>> I don't propose you should though, because N is small enough and the increased complexity
>> isn't worth the decreased readability. So just set num to zero and don't worry about null
>> checks. :)
> Hm, in that case shouldn't we also kfree the allocation in default_clear?
> Makes no sense resetting to 0 and not freeing, when we do an
> unconditional krealloc afterwards. That's the part that confused me ...
> I'm not worried about the realloc overahead (and that's easy to fix
> indeed).
> -Daniel

We might as well, strictly speaking not needed but probably reduces confusion. :)

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

Re: [PATCH v5 4/8] drm: Add driver-private objects to atomic state

[Pandiyan, Dhinakaran]

On Mon, 2017-03-27 at 10:35 +0200, Maarten Lankhorst wrote:
> Op 27-03-17 om 10:31 schreef Daniel Vetter:
> > On Mon, Mar 27, 2017 at 10:28:42AM +0200, Maarten Lankhorst wrote:
> >> Op 27-03-17 om 08:38 schreef Daniel Vetter:
> >>> On Wed, Mar 22, 2017 at 03:30:49PM -0700, Dhinakaran Pandiyan wrote:
> >>>> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> >>>>
> >>>> 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.
> >>>>
> >>>> v4: Avoid redundant NULL checks when private_objs array is empty (Maarten)
> >>>> v3: Macro alignment (Chris)
> >>>> v2: Added docs and new iterator to filter private objects (Daniel)
> >>>>
> >>>> Acked-by: Harry Wentland <harry.wentland@amd.com>
> >>>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/drm_atomic.c        | 69 +++++++++++++++++++++++++++
> >>>>  drivers/gpu/drm/drm_atomic_helper.c |  5 ++
> >>>>  include/drm/drm_atomic.h            | 93 +++++++++++++++++++++++++++++++++++++
> >>>>  3 files changed, 167 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >>>> index 9b892af..e590148 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,21 @@ 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 *private_obj = state->private_objs[i].obj;
> >>>> +		void *obj_state = state->private_objs[i].obj_state;
> >>>> +
> >>>> +		if (!private_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;
> >>>> +	}
> >>>> +	state->num_private_objs = 0;
> >>> Here we set num_private_objs = 0;
> >>>
> >>>> +
> >>>>  }
> >>>>  EXPORT_SYMBOL(drm_atomic_state_default_clear);
> >>>>  
> >>>> @@ -978,6 +994,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
> >>>>  }
> >>>>  
> >>>>  /**
> >>>> + * drm_atomic_get_private_obj_state - get private object state
> >>>> + * @state: global atomic state
> >>>> + * @obj: private object to get the state for
> >>>> + * @funcs: pointer to the struct of function pointers that identify the object
> >>>> + * type
> >>>> + *
> >>>> + * This function returns the private object state for the given private object,
> >>>> + * allocating the state if needed. It does not grab any locks as the caller is
> >>>> + * expected to care of any required locking.
> >>>> + *
> >>>> + * RETURNS:
> >>>> + *
> >>>> + * Either the allocated state or the error code encoded into a pointer.
> >>>> + */
> >>>> +void *
> >>>> +drm_atomic_get_private_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);
> >>> But here we unconditionally realloc to a presumably smaller size. If you
> >>> look at drm_atomic_state->num_connector (which also does dynamic array
> >>> realloc), that one works a bit differently (and hence needs these NULL
> >>> checks).
> >>>
> >>> I think aligning with how we do things with connectors, for consistency
> >>> (no other reason really) would be good.
> >>>
> >>> Just noticed this while reading Maarten's review, which seems to go even
> >>> farther away from how we handle this for connectors.
> >>> -Daniel
> >> Connectors are handled differently, because there's a fixed number of connectors and each
> >> connector is assigned to its slot at state->connectors[drm_connector_index];
> >>
> >> For private objects this is not the case, there's no way to put them in a fixed index,
> >> so the array is resized and reallocated as needed. If you care about the realloc to a smaller
> >> size, add a separate variable max_private_objs and multiply its size by 2 every time it's
> >> not big enough. This also changes get_private_obj_state from O(n²) to O(n log(n)).
> >>
> >> I don't propose you should though, because N is small enough and the increased complexity
> >> isn't worth the decreased readability. So just set num to zero and don't worry about null
> >> checks. :)
> > Hm, in that case shouldn't we also kfree the allocation in default_clear?
> > Makes no sense resetting to 0 and not freeing, when we do an
> > unconditional krealloc afterwards. That's the part that confused me ...
> > I'm not worried about the realloc overahead (and that's easy to fix
> > indeed).
> > -Daniel
> 
> We might as well, strictly speaking not needed but probably reduces confusion. :)
> 


kfree'ing the state->private_objs array in
drm_atomic_state_default_clear() does not seem consistent with what we
do for crtcs, planes and connectors. default_release() seems to be the
function that frees memory for state arrays. We could just go with v4,
where state->num_private_objs is not reset. As 'n' is small, the NULL
checks shouldn't be costly. I can look at optimizing realloc's once we
have more consumers for private_objs?


-DK
> _______________________________________________
> 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

[PATCH v4 5/8] drm/dp: Introduce MST topology state to track available link bandwidth

[Dhinakaran Pandiyan]

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

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 encapsulate 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.

v3: WARN_ON() if connection_mutex is not held (Archit)
v2: Included kernel doc, moved state initialization and switched to
kmemdup() for allocation (Daniel)

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Harry Wentland <Harry.wentland@amd.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 75 +++++++++++++++++++++++++++++++++++
 include/drm/drm_dp_mst_helper.h       | 20 ++++++++++
 2 files changed, 95 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index d3fc7e4..0ad0baa 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2936,6 +2936,69 @@ static void drm_dp_destroy_connector_work(struct work_struct *work)
 		(*mgr->cbs->hotplug)(mgr);
 }
 
+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 = kmemdup(mgr->state, sizeof(*new_mst_state), GFP_KERNEL);
+	if (new_mst_state)
+		new_mst_state->state = state;
+	return new_mst_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_destroy_state(void *obj_state)
+{
+	kfree(obj_state);
+}
+
+static const struct drm_private_state_funcs mst_state_funcs = {
+	.duplicate_state = drm_dp_mst_duplicate_state,
+	.swap_state = drm_dp_mst_swap_state,
+	.destroy_state = drm_dp_mst_destroy_state,
+};
+
+/**
+ * drm_atomic_get_mst_topology_state: get MST topology state
+ *
+ * @state: global atomic state
+ * @mgr: MST topology manager, also the private object in this case
+ *
+ * This function wraps drm_atomic_get_priv_obj_state() passing in the MST atomic
+ * state vtable so that the private object state returned is that of a MST
+ * topology object. Also, drm_atomic_get_private_obj_state() expects the caller
+ * to care of the locking, so warn if don't hold the connection_mutex.
+ *
+ * RETURNS:
+ *
+ * The MST topology state or error pointer.
+ */
+struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
+								    struct drm_dp_mst_topology_mgr *mgr)
+{
+	struct drm_device *dev = mgr->dev;
+
+	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
+	return drm_atomic_get_private_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
@@ -2980,6 +3043,15 @@ 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;
+
+	/* max. time slots - one slot for MTP header */
+	mgr->state->avail_slots = 63;
+	mgr->funcs = &mst_state_funcs;
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_dp_mst_topology_mgr_init);
@@ -3000,6 +3072,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 5b02476..0b371df 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 &drm_dp_mst_branch.txslost and
 	 * &drm_dp_sideband_msg_tx.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 v4 6/8] drm/dp: Add DP MST helpers to atomically find and release vcpi slots

[Dhinakaran Pandiyan]

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

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().

v2:
Added checks for verifying the port reference is valid
Moved get_mst_topology_state() into the helpers (Daniel)
Changed find_vcpi_slots() to not depend on current allocation

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Harry Wentland <Harry.wentland@amd.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 75 +++++++++++++++++++++++++++++++++++
 include/drm/drm_dp_mst_helper.h       |  6 +++
 2 files changed, 81 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 0ad0baa..9f3954e 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2498,6 +2498,81 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
 }
 
 /**
+ * drm_dp_atomic_find_vcpi_slots() - Find and add vcpi slots to the state
+ * @state: global atomic state
+ * @mgr: MST topology manager for the port
+ * @port: port to find vcpi slots for
+ * @pbn: bandwidth required for the mode in PBN
+ *
+ * RETURNS:
+ * Total slots in the atomic state assigned for this port or error
+ */
+int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
+				  struct drm_dp_mst_topology_mgr *mgr,
+				  struct drm_dp_mst_port *port, int pbn)
+{
+	struct drm_dp_mst_topology_state *topology_state;
+	int req_slots;
+
+	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
+	if (topology_state == NULL)
+		return -ENOMEM;
+
+	port = drm_dp_get_validated_port_ref(mgr, port);
+	if (port == NULL)
+		return -EINVAL;
+	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
+	DRM_DEBUG_KMS("vcpi slots req=%d, avail=%d\n",
+			req_slots, topology_state->avail_slots);
+
+	if (req_slots > topology_state->avail_slots) {
+		drm_dp_put_port(port);
+		return -ENOSPC;
+	}
+
+	topology_state->avail_slots -= req_slots;
+	DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state->avail_slots);
+
+	drm_dp_put_port(port);
+	return req_slots;
+}
+EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
+
+/**
+ * drm_dp_atomic_release_vcpi_slots() - Release allocated vcpi slots
+ * @state: global atomic state
+ * @mgr: MST topology manager for the port
+ * @port: port to release the vcpi slots for
+ *
+ * RETURNS:
+ * Number of slots released from the atomic state for this port
+ */
+int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
+				     struct drm_dp_mst_topology_mgr *mgr,
+				     struct drm_dp_mst_port *port)
+{
+	struct drm_dp_mst_topology_state *topology_state;
+	int curr_slots;
+
+	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
+	if (topology_state == NULL)
+		return -ENOMEM;
+
+	port = drm_dp_get_validated_port_ref(mgr, port);
+	if (port == NULL)
+		return -EINVAL;
+
+	curr_slots = port->vcpi.num_slots;
+	topology_state->avail_slots += curr_slots;
+	DRM_DEBUG_KMS("vcpi slots released=%d, avail=%d\n",
+			curr_slots, topology_state->avail_slots);
+
+	drm_dp_put_port(port);
+	return curr_slots;
+}
+EXPORT_SYMBOL(drm_dp_atomic_release_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 0b371df..64e7dac 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -615,5 +615,11 @@ 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_atomic_state *state,
+				  struct drm_dp_mst_topology_mgr *mgr,
+				  struct drm_dp_mst_port *port, int pbn);
+int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
+				     struct drm_dp_mst_topology_mgr *mgr,
+				     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 v4 7/8] drm: Connector helper function to release resources

[Dhinakaran Pandiyan]

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

Having a ->atomic_release callback is useful to release shared resources
that get allocated in compute_config(). This function is expected to be
called in the atomic_check() phase before new resources are acquired.

v3: Use the new 'for_each_oldnew_connector_in_state()' macro.
v2: Moved the caller hunk to this patch (Daniel)

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Harry Wentland <Harry.wentland@amd.com>
Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c      | 19 +++++++++++++++++++
 include/drm/drm_modeset_helper_vtables.h | 13 +++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 1403334..c6c8397 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -586,6 +586,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		}
 	}
 
+	for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) {
+		const struct drm_connector_helper_funcs *conn_funcs;
+		struct drm_crtc_state *crtc_state;
+
+		conn_funcs = connector->helper_private;
+		if (!conn_funcs->atomic_release)
+			continue;
+
+		if (!old_connector_state->crtc)
+			continue;
+
+		crtc_state = drm_atomic_get_existing_crtc_state(state, old_connector_state->crtc);
+
+		if (crtc_state->connectors_changed ||
+		    crtc_state->mode_changed ||
+		    (crtc_state->active_changed && !crtc_state->active))
+			conn_funcs->atomic_release(connector, new_connector_state);
+	}
+
 	return mode_fixup(state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 091c422..394ec0c 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -836,6 +836,19 @@ 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.
+	 *
+	 * 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 v4 7/8] drm: Connector helper function to release resources

[Daniel Vetter]

On Thu, Mar 16, 2017 at 12:10:30AM -0700, Dhinakaran Pandiyan wrote:
> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> 
> Having a ->atomic_release callback is useful to release shared resources
> that get allocated in compute_config(). This function is expected to be
> called in the atomic_check() phase before new resources are acquired.
> 
> v3: Use the new 'for_each_oldnew_connector_in_state()' macro.
> v2: Moved the caller hunk to this patch (Daniel)
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Harry Wentland <Harry.wentland@amd.com>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c      | 19 +++++++++++++++++++
>  include/drm/drm_modeset_helper_vtables.h | 13 +++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 1403334..c6c8397 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -586,6 +586,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  		}
>  	}
>  
> +	for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) {
> +		const struct drm_connector_helper_funcs *conn_funcs;
> +		struct drm_crtc_state *crtc_state;
> +
> +		conn_funcs = connector->helper_private;
> +		if (!conn_funcs->atomic_release)
> +			continue;
> +
> +		if (!old_connector_state->crtc)
> +			continue;
> +
> +		crtc_state = drm_atomic_get_existing_crtc_state(state, old_connector_state->crtc);
> +
> +		if (crtc_state->connectors_changed ||
> +		    crtc_state->mode_changed ||
> +		    (crtc_state->active_changed && !crtc_state->active))
> +			conn_funcs->atomic_release(connector, new_connector_state);
> +	}
> +
>  	return mode_fixup(state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 091c422..394ec0c 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -836,6 +836,19 @@ 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.
> +	 *
> +	 * NOTE:
> +	 *
> +	 * This function is called in the check phase of an atomic update.

"is _unconditionally_ called" I think that would be a useful
clarification. Also, probably good to state that this is called before any
atomic_check calls.

With those two nits addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +	 */
> +	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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 7/8] drm: Connector helper function to release resources

[Dhinakaran Pandiyan]

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

Having an ->atomic_release callback is useful to release shared resources
that get allocated in compute_config(). This function is expected to be
called in the atomic_check() phase before new resources are acquired.

v4: Document that the function is conditionally called and before
other atomic_checks() (Daniel)
v3: Use the new 'for_each_oldnew_connector_in_state()' macro.
v2: Moved the caller hunk to this patch (Daniel)

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c      | 19 +++++++++++++++++++
 include/drm/drm_modeset_helper_vtables.h | 16 ++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 1403334..c6c8397 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -586,6 +586,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		}
 	}
 
+	for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) {
+		const struct drm_connector_helper_funcs *conn_funcs;
+		struct drm_crtc_state *crtc_state;
+
+		conn_funcs = connector->helper_private;
+		if (!conn_funcs->atomic_release)
+			continue;
+
+		if (!old_connector_state->crtc)
+			continue;
+
+		crtc_state = drm_atomic_get_existing_crtc_state(state, old_connector_state->crtc);
+
+		if (crtc_state->connectors_changed ||
+		    crtc_state->mode_changed ||
+		    (crtc_state->active_changed && !crtc_state->active))
+			conn_funcs->atomic_release(connector, new_connector_state);
+	}
+
 	return mode_fixup(state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 091c422..84e80aa 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -836,6 +836,22 @@ 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 conditionally called to release shared resources
+	 * when the attached CRTC's mode changes or it's connectors change or
+	 * becomes inactive. It is called before the corresponding
+	 * &drm_crtc_helper_funcs.atomic_check or
+	 * &drm_crtc_helper_funcs.mode_fixup hooks are called.
+	 *
+	 * 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 v4 8/8] drm/dp: Track MST link bandwidth

[Dhinakaran Pandiyan]

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

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

v2:
Squashed atomic_release() implementation and caller (Daniel)
Fixed logic for connector-crtc switching case (Daniel)
Fixed logic for suspend-resume case.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Harry Wentland <Harry.wentland@amd.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 38 ++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index c1f62eb..a8f40fa 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -39,9 +39,9 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	struct intel_dp *intel_dp = &intel_dig_port->dp;
 	struct intel_connector *connector =
 		to_intel_connector(conn_state->connector);
-	struct drm_atomic_state *state;
+	struct drm_atomic_state *state = pipe_config->base.state;
 	int bpp;
-	int lane_count, slots;
+	int lane_count, slots = 0;
 	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
 	int mst_pbn;
 
@@ -57,30 +57,53 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	 * seem to suggest we should do otherwise.
 	 */
 	lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
-
 	pipe_config->lane_count = lane_count;
 
 	pipe_config->pipe_bpp = bpp;
 	pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);
 
-	state = pipe_config->base.state;
-
 	if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, connector->port))
 		pipe_config->has_audio = true;
-	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
 
+	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
 	pipe_config->pbn = mst_pbn;
-	slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
 
 	intel_link_compute_m_n(bpp, lane_count,
 			       adjusted_mode->crtc_clock,
 			       pipe_config->port_clock,
 			       &pipe_config->dp_m_n);
 
+	if (pipe_config->base.active) {
+		slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr,
+					      connector->port, mst_pbn);
+		if (slots < 0) {
+			DRM_DEBUG_KMS("failed finding vcpi slots:%d\n", slots);
+			return false;
+		}
+	}
 	pipe_config->dp_m_n.tu = slots;
 
 	return true;
+}
+
+static 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_encoder *encoder;
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct drm_atomic_state *state = conn_state->state;
+	int slots;
+
+	encoder = connector->state->best_encoder;
+	intel_mst = enc_to_mst(encoder);
+	mgr = &intel_mst->primary->dp.mst_mgr;
 
+	slots = drm_dp_atomic_release_vcpi_slots(state, mgr,
+						 intel_connector->port);
+	if (slots < 0)
+		DRM_DEBUG_KMS("failed releasing vcpi slots:%d\n", slots);
 }
 
 static void intel_mst_disable_dp(struct intel_encoder *encoder,
@@ -387,6 +410,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 v4 8/8] drm/dp: Track MST link bandwidth

[Maarten Lankhorst]

Op 16-03-17 om 08:10 schreef Dhinakaran Pandiyan:
> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>
> Use the added helpers to track MST link bandwidth for atomic modesets.
> Link bw is acquired in the ->atomic_check() phase when CRTCs are being
> enabled with drm_atomic_find_vcpi_slots() instead of drm_find_vcpi_slots().
> Similarly, link bw is released during ->atomic_check() with the connector
> helper callback ->atomic_release() when CRTCs are disabled.
>
> v2:
> Squashed atomic_release() implementation and caller (Daniel)
> Fixed logic for connector-crtc switching case (Daniel)
> Fixed logic for suspend-resume case.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Harry Wentland <Harry.wentland@amd.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 38 ++++++++++++++++++++++++++++++-------
>  1 file changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index c1f62eb..a8f40fa 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -39,9 +39,9 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  	struct intel_dp *intel_dp = &intel_dig_port->dp;
>  	struct intel_connector *connector =
>  		to_intel_connector(conn_state->connector);
> -	struct drm_atomic_state *state;
> +	struct drm_atomic_state *state = pipe_config->base.state;
>  	int bpp;
> -	int lane_count, slots;
> +	int lane_count, slots = 0;
>  	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>  	int mst_pbn;
>  
> @@ -57,30 +57,53 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  	 * seem to suggest we should do otherwise.
>  	 */
>  	lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
> -
>  	pipe_config->lane_count = lane_count;
>  
>  	pipe_config->pipe_bpp = bpp;
>  	pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);
>  
> -	state = pipe_config->base.state;
> -
>  	if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, connector->port))
>  		pipe_config->has_audio = true;
> -	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
>  
> +	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
>  	pipe_config->pbn = mst_pbn;
> -	slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
>  
>  	intel_link_compute_m_n(bpp, lane_count,
>  			       adjusted_mode->crtc_clock,
>  			       pipe_config->port_clock,
>  			       &pipe_config->dp_m_n);
>  
> +	if (pipe_config->base.active) {
> +		slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr,
> +					      connector->port, mst_pbn);
> +		if (slots < 0) {
> +			DRM_DEBUG_KMS("failed finding vcpi slots:%d\n", slots);
> +			return false;
> +		}
> +	}
>  	pipe_config->dp_m_n.tu = slots;
>  
>  	return true;
> +}
> +
> +static 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_encoder *encoder;
> +	struct intel_connector *intel_connector = to_intel_connector(connector);
> +	struct drm_atomic_state *state = conn_state->state;
> +	int slots;
> +
> +	encoder = connector->state->best_encoder;
> +	intel_mst = enc_to_mst(encoder);
> +	mgr = &intel_mst->primary->dp.mst_mgr;
>  
> +	slots = drm_dp_atomic_release_vcpi_slots(state, mgr,
> +						 intel_connector->port);
> +	if (slots < 0)
> +		DRM_DEBUG_KMS("failed releasing vcpi slots:%d\n", slots);
>  }
>  
>  static void intel_mst_disable_dp(struct intel_encoder *encoder,
> @@ -387,6 +410,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)

Is there any issue into attempting to release vcpi slots when they're already released? If not, for patches 1-3 5-8

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 8/8] drm/dp: Track MST link bandwidth

[Pandiyan, Dhinakaran]

On Wed, 2017-03-22 at 13:30 +0100, Maarten Lankhorst wrote:
> Op 16-03-17 om 08:10 schreef Dhinakaran Pandiyan:
> > From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> >
> > Use the added helpers to track MST link bandwidth for atomic modesets.
> > Link bw is acquired in the ->atomic_check() phase when CRTCs are being
> > enabled with drm_atomic_find_vcpi_slots() instead of drm_find_vcpi_slots().
> > Similarly, link bw is released during ->atomic_check() with the connector
> > helper callback ->atomic_release() when CRTCs are disabled.
> >
> > v2:
> > Squashed atomic_release() implementation and caller (Daniel)
> > Fixed logic for connector-crtc switching case (Daniel)
> > Fixed logic for suspend-resume case.
> >
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Archit Taneja <architt@codeaurora.org>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Harry Wentland <Harry.wentland@amd.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 38 ++++++++++++++++++++++++++++++-------
> >  1 file changed, 31 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index c1f62eb..a8f40fa 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -39,9 +39,9 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >  	struct intel_dp *intel_dp = &intel_dig_port->dp;
> >  	struct intel_connector *connector =
> >  		to_intel_connector(conn_state->connector);
> > -	struct drm_atomic_state *state;
> > +	struct drm_atomic_state *state = pipe_config->base.state;
> >  	int bpp;
> > -	int lane_count, slots;
> > +	int lane_count, slots = 0;
> >  	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> >  	int mst_pbn;
> >  
> > @@ -57,30 +57,53 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >  	 * seem to suggest we should do otherwise.
> >  	 */
> >  	lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
> > -
> >  	pipe_config->lane_count = lane_count;
> >  
> >  	pipe_config->pipe_bpp = bpp;
> >  	pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);
> >  
> > -	state = pipe_config->base.state;
> > -
> >  	if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, connector->port))
> >  		pipe_config->has_audio = true;
> > -	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
> >  
> > +	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
> >  	pipe_config->pbn = mst_pbn;
> > -	slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
> >  
> >  	intel_link_compute_m_n(bpp, lane_count,
> >  			       adjusted_mode->crtc_clock,
> >  			       pipe_config->port_clock,
> >  			       &pipe_config->dp_m_n);
> >  
> > +	if (pipe_config->base.active) {
> > +		slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr,
> > +					      connector->port, mst_pbn);
> > +		if (slots < 0) {
> > +			DRM_DEBUG_KMS("failed finding vcpi slots:%d\n", slots);
> > +			return false;
> > +		}
> > +	}
> >  	pipe_config->dp_m_n.tu = slots;
> >  
> >  	return true;
> > +}
> > +
> > +static 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_encoder *encoder;
> > +	struct intel_connector *intel_connector = to_intel_connector(connector);
> > +	struct drm_atomic_state *state = conn_state->state;
> > +	int slots;
> > +
> > +	encoder = connector->state->best_encoder;
> > +	intel_mst = enc_to_mst(encoder);
> > +	mgr = &intel_mst->primary->dp.mst_mgr;
> >  
> > +	slots = drm_dp_atomic_release_vcpi_slots(state, mgr,
> > +						 intel_connector->port);
> > +	if (slots < 0)
> > +		DRM_DEBUG_KMS("failed releasing vcpi slots:%d\n", slots);
> >  }
> >  
> >  static void intel_mst_disable_dp(struct intel_encoder *encoder,
> > @@ -387,6 +410,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)
> 
> Is there any issue into attempting to release vcpi slots when they're already released? If not, for patches 1-3 5-8
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 

Thanks for the review.

If we already had an atomic commit that released the slots, then
vcpi.num_slots in 'struct drm_dp_mst_port' would have been reset to 0.
The second time, release_vcpi_slots() is called,  the topology_state
slot tracking won't be updated because it gets the current allocation
from per-port structure. So, it should work fine.

-DK

> _______________________________________________
> 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 v4 8/8] drm/dp: Track MST link bandwidth

[Daniel Vetter]

On Wed, Mar 22, 2017 at 01:30:30PM +0100, Maarten Lankhorst wrote:
> Op 16-03-17 om 08:10 schreef Dhinakaran Pandiyan:
> Is there any issue into attempting to release vcpi slots when they're already released? If not, for patches 1-3 5-8
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Merged patches 1-3 to drm-misc-next, thanks.
-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