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

[PATCH v3 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 version
	1) splits and squashes patches
	2) adds documentation
	3) fixes vcpi slot accounting logic for suspend-resume and
	connector switching

Dhinakaran Pandiyan (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    | 181 ++++++++++++++++++++++++++++---
 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                 |  91 ++++++++++++++++
 include/drm/drm_dp_mst_helper.h          |  33 ++++--
 include/drm/drm_modeset_helper_vtables.h |  13 +++
 9 files changed, 421 insertions(+), 38 deletions(-)

-- 
2.7.4

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

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

[Dhinakaran Pandiyan]

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.

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 122a1b0..cf4b866 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 v3 2/8] drm/dp: Kill unused MST vcpi slot availability tracking

[Dhinakaran Pandiyan]

The avail_slots member in the MST topology manager is never updated to
reflect the available vcpi slots. The check is effectively against
total slots, 63. So, let's make that check obvious and remove
avail_slots. While at it, make debug messages more descriptive.

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 cf4b866..d9edd84 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 v3 3/8] drm/dp: Split drm_dp_mst_allocate_vcpi

[Dhinakaran Pandiyan]

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

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

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
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 d9edd84..37482b7 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 6a85d38..9b9dda8 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.
@@ -179,7 +178,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 cd75413..6bfdf47 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 v3 4/8] drm: Add driver-private objects to atomic state

[Dhinakaran Pandiyan]

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

v2: Added docs and new iterator to filter private objects (Daniel)

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            | 91 +++++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index a567310..1a9ffe8 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);
 
@@ -974,6 +989,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 3a4383f..8795088 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1983,6 +1983,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 	struct drm_plane *plane;
 	struct drm_plane_state *plane_state;
 	struct drm_crtc_commit *commit;
+	void *obj, *obj_state;
+	const struct drm_private_state_funcs *funcs;
 
 	if (stall) {
 		for_each_crtc_in_state(state, crtc, crtc_state, i) {
@@ -2031,6 +2033,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 		swap(state->planes[i].state, plane->state);
 		plane->state->state = NULL;
 	}
+
+	__for_each_private_obj(state, obj, obj_state, i, funcs)
+		funcs->swap_state(obj, &state->private_objs[i].obj_state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_swap_state);
 
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 052ab16..cafa404 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
@@ -415,6 +471,41 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
 		for_each_if (plane_state)
 
 /**
+ * __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 v3 4/8] drm: Add driver-private objects to atomic state

[Chris Wilson]

On Wed, Feb 08, 2017 at 10:38:07PM -0800, Dhinakaran Pandiyan wrote:
> +#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);	\

Align to ( and put the trailing 1 on its own line so it stands out.

	     (__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
>   *

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Pandiyan, Dhinakaran]

On Thu, 2017-02-09 at 08:08 +0000, Chris Wilson wrote:
> On Wed, Feb 08, 2017 at 10:38:07PM -0800, Dhinakaran Pandiyan wrote:
> > +#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);	\
> 
> Align to ( and put the trailing 1 on its own line so it stands out.

Sure, will do that. Looks like I have to change other macros in that
file too.

-DK
> 
> 	     (__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
> >   *
> 

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

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

[Archit Taneja]

Hi,

On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:
> It is necessary to track states for objects other than connector, crtc
> and plane for atomic modesets. But adding objects like DP MST link
> bandwidth to drm_atomic_state would mean that a non-core object will be
> modified by the core helper functions for swapping and clearing
> it's state. So, lets add void * objects and helper functions that operate
> on void * types to keep these objects and states private to the core.
> Drivers can then implement specific functions to swap and clear states.
> The other advantage having just void * for these objects in
> drm_atomic_state is that objects of different types can be managed in the
> same state array.
>
> v2: Added docs and new iterator to filter private objects (Daniel)
>
> 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            | 91 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 164 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index a567310..1a9ffe8 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);
>
> @@ -974,6 +989,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;

Comparing this func to drm_atomic_get_plane_state/drm_atomic_get_crtc_state, it
doesn't seem to call drm_modeset_lock if the obj_state doesn't already exist. I
don't understand the locking stuff toowell, I just noticed this difference when
comparing this approach with what is done in the msm kms driver (where we
have subclassed drm_atomic_state to msm_kms_state).

Thanks,
Archit

> +
> +	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 3a4383f..8795088 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1983,6 +1983,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  	struct drm_plane *plane;
>  	struct drm_plane_state *plane_state;
>  	struct drm_crtc_commit *commit;
> +	void *obj, *obj_state;
> +	const struct drm_private_state_funcs *funcs;
>
>  	if (stall) {
>  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> @@ -2031,6 +2033,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  		swap(state->planes[i].state, plane->state);
>  		plane->state->state = NULL;
>  	}
> +
> +	__for_each_private_obj(state, obj, obj_state, i, funcs)
> +		funcs->swap_state(obj, &state->private_objs[i].obj_state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_swap_state);
>
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 052ab16..cafa404 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
> @@ -415,6 +471,41 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
>  		for_each_if (plane_state)
>
>  /**
> + * __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
>   *
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Pandiyan, Dhinakaran]

On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
> Hi,
> 
> On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:
> > It is necessary to track states for objects other than connector, crtc
> > and plane for atomic modesets. But adding objects like DP MST link
> > bandwidth to drm_atomic_state would mean that a non-core object will be
> > modified by the core helper functions for swapping and clearing
> > it's state. So, lets add void * objects and helper functions that operate
> > on void * types to keep these objects and states private to the core.
> > Drivers can then implement specific functions to swap and clear states.
> > The other advantage having just void * for these objects in
> > drm_atomic_state is that objects of different types can be managed in the
> > same state array.
> >
> > v2: Added docs and new iterator to filter private objects (Daniel)
> >
> > 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            | 91 +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 164 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index a567310..1a9ffe8 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);
> >
> > @@ -974,6 +989,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;
> 
> Comparing this func to drm_atomic_get_plane_state/drm_atomic_get_crtc_state, it
> doesn't seem to call drm_modeset_lock if the obj_state doesn't already exist. I
> don't understand the locking stuff toowell, I just noticed this difference when
> comparing this approach with what is done in the msm kms driver (where we
> have subclassed drm_atomic_state to msm_kms_state).
> 
> Thanks,
> Archit
> 


The caller is expected to take care of any required locking. The
driver-private objects are opaque from core's pov, so the core is not
aware of necessary locks for that object type.

-DK 

> > +
> > +	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 3a4383f..8795088 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1983,6 +1983,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> >  	struct drm_plane *plane;
> >  	struct drm_plane_state *plane_state;
> >  	struct drm_crtc_commit *commit;
> > +	void *obj, *obj_state;
> > +	const struct drm_private_state_funcs *funcs;
> >
> >  	if (stall) {
> >  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > @@ -2031,6 +2033,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> >  		swap(state->planes[i].state, plane->state);
> >  		plane->state->state = NULL;
> >  	}
> > +
> > +	__for_each_private_obj(state, obj, obj_state, i, funcs)
> > +		funcs->swap_state(obj, &state->private_objs[i].obj_state);
> >  }
> >  EXPORT_SYMBOL(drm_atomic_helper_swap_state);
> >
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index 052ab16..cafa404 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
> > @@ -415,6 +471,41 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
> >  		for_each_if (plane_state)
> >
> >  /**
> > + * __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
> >   *
> >
> 

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

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

[Archit Taneja]

On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote:
> On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
>> Hi,
>>
>> On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:
>>> It is necessary to track states for objects other than connector, crtc
>>> and plane for atomic modesets. But adding objects like DP MST link
>>> bandwidth to drm_atomic_state would mean that a non-core object will be
>>> modified by the core helper functions for swapping and clearing
>>> it's state. So, lets add void * objects and helper functions that operate
>>> on void * types to keep these objects and states private to the core.
>>> Drivers can then implement specific functions to swap and clear states.
>>> The other advantage having just void * for these objects in
>>> drm_atomic_state is that objects of different types can be managed in the
>>> same state array.
>>>
>>> v2: Added docs and new iterator to filter private objects (Daniel)
>>>
>>> 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            | 91 +++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 164 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>> index a567310..1a9ffe8 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);
>>>
>>> @@ -974,6 +989,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;
>>
>> Comparing this func to drm_atomic_get_plane_state/drm_atomic_get_crtc_state, it
>> doesn't seem to call drm_modeset_lock if the obj_state doesn't already exist. I
>> don't understand the locking stuff toowell, I just noticed this difference when
>> comparing this approach with what is done in the msm kms driver (where we
>> have subclassed drm_atomic_state to msm_kms_state).
>>
>> Thanks,
>> Archit
>>
>
>
> The caller is expected to take care of any required locking. The
> driver-private objects are opaque from core's pov, so the core is not
> aware of necessary locks for that object type.

I had a look at the rest of the series, and I couldn't easily understand
whether the caller code protects the MST related driver private state. Is
it expected to be protect via the drm_mode_config.connection_mutex lock?

Thanks,
Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Pandiyan, Dhinakaran]

On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote:
> 
> On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote:
> > On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
> >> Hi,
> >>
> >> On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:
> >>> It is necessary to track states for objects other than connector, crtc
> >>> and plane for atomic modesets. But adding objects like DP MST link
> >>> bandwidth to drm_atomic_state would mean that a non-core object will be
> >>> modified by the core helper functions for swapping and clearing
> >>> it's state. So, lets add void * objects and helper functions that operate
> >>> on void * types to keep these objects and states private to the core.
> >>> Drivers can then implement specific functions to swap and clear states.
> >>> The other advantage having just void * for these objects in
> >>> drm_atomic_state is that objects of different types can be managed in the
> >>> same state array.
> >>>
> >>> v2: Added docs and new iterator to filter private objects (Daniel)
> >>>
> >>> 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            | 91 +++++++++++++++++++++++++++++++++++++
> >>>  3 files changed, 164 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >>> index a567310..1a9ffe8 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);
> >>>
> >>> @@ -974,6 +989,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;
> >>
> >> Comparing this func to drm_atomic_get_plane_state/drm_atomic_get_crtc_state, it
> >> doesn't seem to call drm_modeset_lock if the obj_state doesn't already exist. I
> >> don't understand the locking stuff toowell, I just noticed this difference when
> >> comparing this approach with what is done in the msm kms driver (where we
> >> have subclassed drm_atomic_state to msm_kms_state).
> >>
> >> Thanks,
> >> Archit
> >>
> >
> >
> > The caller is expected to take care of any required locking. The
> > driver-private objects are opaque from core's pov, so the core is not
> > aware of necessary locks for that object type.
> 
> I had a look at the rest of the series, and I couldn't easily understand
> whether the caller code protects the MST related driver private state. Is
> it expected to be protect via the drm_mode_config.connection_mutex lock?
> 
> Thanks,
> Archit
> 

That's right, the connection_mutex takes care of the locking for the MST
private state. I can add that as a comment to the caller's (MST helper)
kernel doc with a

WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); 
 

-DK

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

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

[Archit Taneja]

On 02/22/2017 05:31 AM, Pandiyan, Dhinakaran wrote:
> On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote:
>>
>> On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote:
>>> On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
>>>> Hi,
>>>>
>>>> On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:
>>>>> It is necessary to track states for objects other than connector, crtc
>>>>> and plane for atomic modesets. But adding objects like DP MST link
>>>>> bandwidth to drm_atomic_state would mean that a non-core object will be
>>>>> modified by the core helper functions for swapping and clearing
>>>>> it's state. So, lets add void * objects and helper functions that operate
>>>>> on void * types to keep these objects and states private to the core.
>>>>> Drivers can then implement specific functions to swap and clear states.
>>>>> The other advantage having just void * for these objects in
>>>>> drm_atomic_state is that objects of different types can be managed in the
>>>>> same state array.
>>>>>
>>>>> v2: Added docs and new iterator to filter private objects (Daniel)
>>>>>
>>>>> 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            | 91 +++++++++++++++++++++++++++++++++++++
>>>>>  3 files changed, 164 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>>>> index a567310..1a9ffe8 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);
>>>>>
>>>>> @@ -974,6 +989,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;
>>>>
>>>> Comparing this func to drm_atomic_get_plane_state/drm_atomic_get_crtc_state, it
>>>> doesn't seem to call drm_modeset_lock if the obj_state doesn't already exist. I
>>>> don't understand the locking stuff toowell, I just noticed this difference when
>>>> comparing this approach with what is done in the msm kms driver (where we
>>>> have subclassed drm_atomic_state to msm_kms_state).
>>>>
>>>> Thanks,
>>>> Archit
>>>>
>>>
>>>
>>> The caller is expected to take care of any required locking. The
>>> driver-private objects are opaque from core's pov, so the core is not
>>> aware of necessary locks for that object type.
>>
>> I had a look at the rest of the series, and I couldn't easily understand
>> whether the caller code protects the MST related driver private state. Is
>> it expected to be protect via the drm_mode_config.connection_mutex lock?
>>
>> Thanks,
>> Archit
>>
>
> That's right, the connection_mutex takes care of the locking for the MST
> private state. I can add that as a comment to the caller's (MST helper)
> kernel doc with a
>
> WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));

That would be nice to have.

In the comment: "It does not grab any locks as the caller is expected to
care of any required locking.", you could maybe be a bit more specific
and rephrase it as "the caller needs to grab the &drm_modeset_lock
responsible for protecting the private object state"

Thanks,
Archit

>
>
> -DK
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Pandiyan, Dhinakaran]

On Wed, 2017-02-22 at 09:59 +0530, Archit Taneja wrote:
> 
> On 02/22/2017 05:31 AM, Pandiyan, Dhinakaran wrote:
> > On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote:
> >>
> >> On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote:
> >>> On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
> >>>> Hi,
> >>>>
> >>>> On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:
> >>>>> It is necessary to track states for objects other than connector, crtc
> >>>>> and plane for atomic modesets. But adding objects like DP MST link
> >>>>> bandwidth to drm_atomic_state would mean that a non-core object will be
> >>>>> modified by the core helper functions for swapping and clearing
> >>>>> it's state. So, lets add void * objects and helper functions that operate
> >>>>> on void * types to keep these objects and states private to the core.
> >>>>> Drivers can then implement specific functions to swap and clear states.
> >>>>> The other advantage having just void * for these objects in
> >>>>> drm_atomic_state is that objects of different types can be managed in the
> >>>>> same state array.
> >>>>>
> >>>>> v2: Added docs and new iterator to filter private objects (Daniel)
> >>>>>
> >>>>> 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            | 91 +++++++++++++++++++++++++++++++++++++
> >>>>>  3 files changed, 164 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >>>>> index a567310..1a9ffe8 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);
> >>>>>
> >>>>> @@ -974,6 +989,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;
> >>>>
> >>>> Comparing this func to drm_atomic_get_plane_state/drm_atomic_get_crtc_state, it
> >>>> doesn't seem to call drm_modeset_lock if the obj_state doesn't already exist. I
> >>>> don't understand the locking stuff toowell, I just noticed this difference when
> >>>> comparing this approach with what is done in the msm kms driver (where we
> >>>> have subclassed drm_atomic_state to msm_kms_state).
> >>>>
> >>>> Thanks,
> >>>> Archit
> >>>>
> >>>
> >>>
> >>> The caller is expected to take care of any required locking. The
> >>> driver-private objects are opaque from core's pov, so the core is not
> >>> aware of necessary locks for that object type.
> >>
> >> I had a look at the rest of the series, and I couldn't easily understand
> >> whether the caller code protects the MST related driver private state. Is
> >> it expected to be protect via the drm_mode_config.connection_mutex lock?
> >>
> >> Thanks,
> >> Archit
> >>
> >
> > That's right, the connection_mutex takes care of the locking for the MST
> > private state. I can add that as a comment to the caller's (MST helper)
> > kernel doc with a
> >
> > WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> 
> That would be nice to have.
> 
> In the comment: "It does not grab any locks as the caller is expected to
> care of any required locking.", you could maybe be a bit more specific
> and rephrase it as "the caller needs to grab the &drm_modeset_lock
> responsible for protecting the private object state"
> 
> Thanks,
> Archit
> 

The core leaves it to the drivers to choose the private-object types to
add. So, I believe the core should do not mandate the use of
modeset_locks. MST happens to be one example where connection_mutex is
the appropriate lock.

-DK

> >
> >
> > -DK
> >
> 

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

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

[Daniel Vetter]

On Wed, Feb 22, 2017 at 12:01:12AM +0000, Pandiyan, Dhinakaran wrote:
> On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote:
> > 
> > On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote:
> > > On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
> > >> Comparing this func to drm_atomic_get_plane_state/drm_atomic_get_crtc_state, it
> > >> doesn't seem to call drm_modeset_lock if the obj_state doesn't already exist. I
> > >> don't understand the locking stuff toowell, I just noticed this difference when
> > >> comparing this approach with what is done in the msm kms driver (where we
> > >> have subclassed drm_atomic_state to msm_kms_state).
> > >>
> > >> Thanks,
> > >> Archit
> > >>
> > >
> > >
> > > The caller is expected to take care of any required locking. The
> > > driver-private objects are opaque from core's pov, so the core is not
> > > aware of necessary locks for that object type.
> > 
> > I had a look at the rest of the series, and I couldn't easily understand
> > whether the caller code protects the MST related driver private state. Is
> > it expected to be protect via the drm_mode_config.connection_mutex lock?
> > 
> > Thanks,
> > Archit
> > 
> 
> That's right, the connection_mutex takes care of the locking for the MST
> private state. I can add that as a comment to the caller's (MST helper)
> kernel doc with a
> 
> WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); 

Please don't add this as a comment, but as real code so it is checked at
runtime :-) Personally I wouldn't mention locking rules in kernel-doc,
that part tends to get outdated fast. Better to enforce with
runtime-checks.
-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 v3 4/8] drm: Add driver-private objects to atomic state

[Pandiyan, Dhinakaran]

On Sun, 2017-02-26 at 20:57 +0100, Daniel Vetter wrote:
> On Wed, Feb 22, 2017 at 12:01:12AM +0000, Pandiyan, Dhinakaran wrote:
> > On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote:
> > > 
> > > On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote:
> > > > On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
> > > >> Comparing this func to drm_atomic_get_plane_state/drm_atomic_get_crtc_state, it
> > > >> doesn't seem to call drm_modeset_lock if the obj_state doesn't already exist. I
> > > >> don't understand the locking stuff toowell, I just noticed this difference when
> > > >> comparing this approach with what is done in the msm kms driver (where we
> > > >> have subclassed drm_atomic_state to msm_kms_state).
> > > >>
> > > >> Thanks,
> > > >> Archit
> > > >>
> > > >
> > > >
> > > > The caller is expected to take care of any required locking. The
> > > > driver-private objects are opaque from core's pov, so the core is not
> > > > aware of necessary locks for that object type.
> > > 
> > > I had a look at the rest of the series, and I couldn't easily understand
> > > whether the caller code protects the MST related driver private state. Is
> > > it expected to be protect via the drm_mode_config.connection_mutex lock?
> > > 
> > > Thanks,
> > > Archit
> > > 
> > 
> > That's right, the connection_mutex takes care of the locking for the MST
> > private state. I can add that as a comment to the caller's (MST helper)
> > kernel doc with a
> > 
> > WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); 
> 
> Please don't add this as a comment, but as real code so it is checked at
> runtime :-) Personally I wouldn't mention locking rules in kernel-doc,
> that part tends to get outdated fast. Better to enforce with
> runtime-checks.
> -Daniel

That's what I meant but evidently didn't type it that way:) I was going
to add that the connection_mutex does the locking for MST state as a
comment and put the WARN_ON() in code.

-DK

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

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

[Pandiyan, Dhinakaran]

IRC acked by Harry Wentland


"<hwentlan> dhnkrn, the patch for driver-private atomic state object
makes sense to me.  Didn't realize that's the same one from early
February. Feel free to add my Acked-by"


-DK
On Wed, 2017-02-08 at 22:38 -0800, Dhinakaran Pandiyan wrote:
> It is necessary to track states for objects other than connector, crtc
> and plane for atomic modesets. But adding objects like DP MST link
> bandwidth to drm_atomic_state would mean that a non-core object will be
> modified by the core helper functions for swapping and clearing
> it's state. So, lets add void * objects and helper functions that operate
> on void * types to keep these objects and states private to the core.
> Drivers can then implement specific functions to swap and clear states.
> The other advantage having just void * for these objects in
> drm_atomic_state is that objects of different types can be managed in the
> same state array.
> 
> v2: Added docs and new iterator to filter private objects (Daniel)
> 
> 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            | 91 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 164 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index a567310..1a9ffe8 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);
>  
> @@ -974,6 +989,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 3a4383f..8795088 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1983,6 +1983,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  	struct drm_plane *plane;
>  	struct drm_plane_state *plane_state;
>  	struct drm_crtc_commit *commit;
> +	void *obj, *obj_state;
> +	const struct drm_private_state_funcs *funcs;
>  
>  	if (stall) {
>  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> @@ -2031,6 +2033,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  		swap(state->planes[i].state, plane->state);
>  		plane->state->state = NULL;
>  	}
> +
> +	__for_each_private_obj(state, obj, obj_state, i, funcs)
> +		funcs->swap_state(obj, &state->private_objs[i].obj_state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_swap_state);
>  
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 052ab16..cafa404 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
> @@ -415,6 +471,41 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
>  		for_each_if (plane_state)
>  
>  /**
> + * __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
>   *

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

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

[Dhinakaran Pandiyan]

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

v2: Included kernel doc, moved state initialization and switched to
kmemdup() for allocation (Daniel)

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

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 37482b7..a891c36 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2936,6 +2936,65 @@ 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.
+ *
+ * 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)
+{
+		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 +3039,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 +3068,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 v3 6/8] drm/dp: Add DP MST helpers to atomically find and release vcpi slots

[Dhinakaran Pandiyan]

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

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

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

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

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 a891c36..93de257 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 v3 7/8] drm: Connector helper function to release resources

[Dhinakaran Pandiyan]

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.

v2: Moved the caller hunk to this patch (Daniel)

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 8795088..92bd741 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		}
 	}
 
+	for_each_connector_in_state(state, connector, 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 (!connector->state->crtc)
+			continue;
+
+		crtc_state = drm_atomic_get_existing_crtc_state(state, connector->state->crtc);
+
+		if (crtc_state->connectors_changed ||
+		    crtc_state->mode_changed ||
+		    (crtc_state->active_changed && !crtc_state->active))
+			conn_funcs->atomic_release(connector, 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 v3 7/8] drm: Connector helper function to release resources

[Lankhorst, Maarten]

Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
> 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.
> 
> v2: Moved the caller hunk to this patch (Daniel)
> 
> 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 8795088..92bd741 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct
> drm_device *dev,
>  		}
>  	}
>  
> +	for_each_connector_in_state(state, connector,
> 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 (!connector->state->crtc)
> +			continue;
> +
> +		crtc_state =
> drm_atomic_get_existing_crtc_state(state, connector->state->crtc);
> +
> +		if (crtc_state->connectors_changed ||
> +		    crtc_state->mode_changed ||
> +		    (crtc_state->active_changed && !crtc_state-
> >active))
> +			conn_funcs->atomic_release(connector,
> connector_state);
> +	}

Could we deal with the VCPI state separately in intel_modeset_checks,
like we do with dpll?

Maybe implementing the relevant VCPI state could be done as an atomic
helper function too, so other atomic drivers can just plug it in.

Not sure how doable this is, but if it's not too hard, then it's
probably cleaner :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Pandiyan, Dhinakaran]

On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote:
> Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
> > 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.
> > 
> > v2: Moved the caller hunk to this patch (Daniel)
> > 
> > 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 8795088..92bd741 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct
> > drm_device *dev,
> >  		}
> >  	}
> >  
> > +	for_each_connector_in_state(state, connector,
> > 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 (!connector->state->crtc)
> > +			continue;
> > +
> > +		crtc_state =
> > drm_atomic_get_existing_crtc_state(state, connector->state->crtc);
> > +
> > +		if (crtc_state->connectors_changed ||
> > +		    crtc_state->mode_changed ||
> > +		    (crtc_state->active_changed && !crtc_state-
> > >active))
> > +			conn_funcs->atomic_release(connector,
> > connector_state);
> > +	}
> 
> Could we deal with the VCPI state separately in intel_modeset_checks,
> like we do with dpll?

We'd want to release the VCPI slots before they are acquired in
->compute_config(). intel_modeset_checks() will be too late to release
them. Are you suggesting both acquiring and releasing slots should be
done in intel_modeset_checks()?


> 
> Maybe implementing the relevant VCPI state could be done as an atomic
> helper function too, so other atomic drivers can just plug it in.
> 
The idea was to reduce boilerplate in the drivers and use the
private_obj state for different object types.


> Not sure how doable this is, but if it's not too hard, then it's
> probably cleaner :)
> _______________________________________________
> 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 v3 7/8] drm: Connector helper function to release resources

[Lankhorst, Maarten]

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

Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]:
> On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote:
> > 
> > Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
> > > 
> > > 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.
> > > 
> > > v2: Moved the caller hunk to this patch (Daniel)
> > > 
> > > 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 8795088..92bd741 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct
> > > drm_device *dev,
> > >  		}
> > >  	}
> > >  
> > > +	for_each_connector_in_state(state, connector,
> > > 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 (!connector->state->crtc)
> > > +			continue;
> > > +
> > > +		crtc_state =
> > > drm_atomic_get_existing_crtc_state(state, connector->state-
> > > >crtc);
> > > +
> > > +		if (crtc_state->connectors_changed ||
> > > +		    crtc_state->mode_changed ||
> > > +		    (crtc_state->active_changed && !crtc_state-
> > > > 
> > > > active))
> > > +			conn_funcs->atomic_release(connector,
> > > connector_state);
> > > +	}
> > 
> > Could we deal with the VCPI state separately in
> > intel_modeset_checks,
> > like we do with dpll?
> 
> We'd want to release the VCPI slots before they are acquired in
> ->compute_config(). intel_modeset_checks() will be too late to
> release
> them. Are you suggesting both acquiring and releasing slots should be
> done in intel_modeset_checks()?

That makes things a bit more nasty. Maybe add a
conn_funcs->atomic_check that always gets called, something like I did
below?

I'd love to use it for some atomic connector properties too.
> > 
> > 
> > Maybe implementing the relevant VCPI state could be done as an
> > atomic
> > helper function too, so other atomic drivers can just plug it in.
> > 
> The idea was to reduce boilerplate in the drivers and use the
> private_obj state for different object types.
> 
> 
> > 
> > Not sure how doable this is, but if it's not too hard, then it's
> > probably cleaner :)
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-patch; name="patch", Size: 1579 bytes --]

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 39cbacd8cd07..1e5f0a95c685 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -381,11 +381,24 @@ mode_fixup(struct drm_atomic_state *state)
 
 	for_each_new_connector_in_state(state, connector, conn_state, i) {
 		const struct drm_encoder_helper_funcs *funcs;
+		const struct drm_connector_helper_funcs *conn_funcs;
 		struct drm_encoder *encoder;
 
+		conn_funcs = connector->helper_private;
+
 		WARN_ON(!!conn_state->best_encoder != !!conn_state->crtc);
 
-		if (!conn_state->crtc || !conn_state->best_encoder)
+		if (!conn_state->crtc || !conn_state->best_encoder) {
+			if (conn_funcs && conn_funcs->atomic_check) {
+				ret = conn_funcs->atomic_check(connector, conn_state);
+
+				if (ret) {
+					DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] check failed\n",
+							 connector->base.id, connector->name);
+					return ret;
+				}
+			}
+
 			continue;
 
 		crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
@@ -404,7 +417,15 @@ mode_fixup(struct drm_atomic_state *state)
 			return -EINVAL;
 		}
 
-		if (funcs && funcs->atomic_check) {
+		if (conn_funcs && conn_funcs->atomic_check) {
+			ret = conn_funcs->atomic_check(connector, conn_state);
+
+			if (ret) {
+				DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] check failed\n",
+						  connector->base.id, connector->name);
+				return ret;
+			}
+		} else if (funcs && funcs->atomic_check) {
 			ret = funcs->atomic_check(encoder, crtc_state,
 						  conn_state);
 			if (ret) {

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

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

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

[Pandiyan, Dhinakaran]

On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
> Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]:
> > On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote:
> > > 
> > > Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
> > > > 
> > > > 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.
> > > > 
> > > > v2: Moved the caller hunk to this patch (Daniel)
> > > > 
> > > > 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 8795088..92bd741 100644
> > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct
> > > > drm_device *dev,
> > > >  		}
> > > >  	}
> > > >  
> > > > +	for_each_connector_in_state(state, connector,
> > > > 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 (!connector->state->crtc)
> > > > +			continue;
> > > > +
> > > > +		crtc_state =
> > > > drm_atomic_get_existing_crtc_state(state, connector->state-
> > > > >crtc);
> > > > +
> > > > +		if (crtc_state->connectors_changed ||
> > > > +		    crtc_state->mode_changed ||
> > > > +		    (crtc_state->active_changed && !crtc_state-
> > > > > 
> > > > > active))
> > > > +			conn_funcs->atomic_release(connector,
> > > > connector_state);
> > > > +	}
> > > 
> > > Could we deal with the VCPI state separately in
> > > intel_modeset_checks,
> > > like we do with dpll?
> > 
> > We'd want to release the VCPI slots before they are acquired in
> > ->compute_config(). intel_modeset_checks() will be too late to
> > release
> > them. Are you suggesting both acquiring and releasing slots should be
> > done in intel_modeset_checks()?
> 
> That makes things a bit more nasty. Maybe add a
> conn_funcs->atomic_check that always gets called, something like I did
> below?
> 
> I'd love to use it for some atomic connector properties too.


Adding and unconditionally calling conn_funcs->atomic_check() should be
doable. It also follows the pattern we have for encoders and CRTCs. But
I'll have to move the connector->state->crtc state checks inside the
function.

-DK
> > > 
> > > 
> > > Maybe implementing the relevant VCPI state could be done as an
> > > atomic
> > > helper function too, so other atomic drivers can just plug it in.
> > > 
> > The idea was to reduce boilerplate in the drivers and use the
> > private_obj state for different object types.
> > 
> > 
> > > 
> > > Not sure how doable this is, but if it's not too hard, then it's
> > > probably cleaner :)
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Pandiyan, Dhinakaran]

On Mon, 2017-02-13 at 21:26 +0000, Pandiyan, Dhinakaran wrote:
> On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
> > Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]:
> > > On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote:
> > > > 
> > > > Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
> > > > > 
> > > > > 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.
> > > > > 
> > > > > v2: Moved the caller hunk to this patch (Daniel)
> > > > > 
> > > > > 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 8795088..92bd741 100644
> > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct
> > > > > drm_device *dev,
> > > > >  		}
> > > > >  	}
> > > > >  
> > > > > +	for_each_connector_in_state(state, connector,
> > > > > 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 (!connector->state->crtc)
> > > > > +			continue;
> > > > > +
> > > > > +		crtc_state =
> > > > > drm_atomic_get_existing_crtc_state(state, connector->state-
> > > > > >crtc);
> > > > > +
> > > > > +		if (crtc_state->connectors_changed ||
> > > > > +		    crtc_state->mode_changed ||
> > > > > +		    (crtc_state->active_changed && !crtc_state-
> > > > > > 
> > > > > > active))
> > > > > +			conn_funcs->atomic_release(connector,
> > > > > connector_state);
> > > > > +	}
> > > > 
> > > > Could we deal with the VCPI state separately in
> > > > intel_modeset_checks,
> > > > like we do with dpll?
> > > 
> > > We'd want to release the VCPI slots before they are acquired in
> > > ->compute_config(). intel_modeset_checks() will be too late to
> > > release
> > > them. Are you suggesting both acquiring and releasing slots should be
> > > done in intel_modeset_checks()?
> > 
> > That makes things a bit more nasty. Maybe add a
> > conn_funcs->atomic_check that always gets called, something like I did
> > below?
> > 
> > I'd love to use it for some atomic connector properties too.
> 
> 
> Adding and unconditionally calling conn_funcs->atomic_check() should be
> doable. It also follows the pattern we have for encoders and CRTCs. But
> I'll have to move the connector->state->crtc state checks inside the
> function.
> 
> -DK


This is what I mean -https://pastebin.ubuntu.com/23991405/
But, I do have one concern with calling this conn_func->atomic_check().
We are not validating the new connector_state like atomic_check() seems
to do generally but only cleaning up vcpi resources for compute_config()
to later acquire. Let me know if I am wrong in my understanding what
atomic_check() is expected to do.


-DK
> > > > 
> > > > 
> > > > Maybe implementing the relevant VCPI state could be done as an
> > > > atomic
> > > > helper function too, so other atomic drivers can just plug it in.
> > > > 
> > > The idea was to reduce boilerplate in the drivers and use the
> > > private_obj state for different object types.
> > > 
> > > 
> > > > 
> > > > Not sure how doable this is, but if it's not too hard, then it's
> > > > probably cleaner :)
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Lankhorst, Maarten]

Pandiyan, Dhinakaran schreef op ma 13-02-2017 om 22:48 [+0000]:
> On Mon, 2017-02-13 at 21:26 +0000, Pandiyan, Dhinakaran wrote:
> > 
> > On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
> > > 
> > > Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]:
> > > > 
> > > > On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote:
> > > > > 
> > > > > 
> > > > > Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-
> > > > > 0800]:
> > > > > > 
> > > > > > 
> > > > > > 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.
> > > > > > 
> > > > > > v2: Moved the caller hunk to this patch (Daniel)
> > > > > > 
> > > > > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@int
> > > > > > el.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 8795088..92bd741 100644
> > > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct
> > > > > > drm_device *dev,
> > > > > >  		}
> > > > > >  	}
> > > > > >  
> > > > > > +	for_each_connector_in_state(state, connector,
> > > > > > 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 (!connector->state->crtc)
> > > > > > +			continue;
> > > > > > +
> > > > > > +		crtc_state =
> > > > > > drm_atomic_get_existing_crtc_state(state, connector->state-
> > > > > > > 
> > > > > > > crtc);
> > > > > > +
> > > > > > +		if (crtc_state->connectors_changed ||
> > > > > > +		    crtc_state->mode_changed ||
> > > > > > +		    (crtc_state->active_changed &&
> > > > > > !crtc_state-
> > > > > > > 
> > > > > > > 
> > > > > > > active))
> > > > > > +			conn_funcs-
> > > > > > >atomic_release(connector,
> > > > > > connector_state);
> > > > > > +	}
> > > > > 
> > > > > Could we deal with the VCPI state separately in
> > > > > intel_modeset_checks,
> > > > > like we do with dpll?
> > > > 
> > > > We'd want to release the VCPI slots before they are acquired in
> > > > ->compute_config(). intel_modeset_checks() will be too late to
> > > > release
> > > > them. Are you suggesting both acquiring and releasing slots
> > > > should be
> > > > done in intel_modeset_checks()?
> > > 
> > > That makes things a bit more nasty. Maybe add a
> > > conn_funcs->atomic_check that always gets called, something like
> > > I did
> > > below?
> > > 
> > > I'd love to use it for some atomic connector properties too.
> > 
> > 
> > Adding and unconditionally calling conn_funcs->atomic_check()
> > should be
> > doable. It also follows the pattern we have for encoders and CRTCs.
> > But
> > I'll have to move the connector->state->crtc state checks inside
> > the
> > function.
> > 
> > -DK
> 
> 
> This is what I mean -https://pastebin.ubuntu.com/23991405/
> But, I do have one concern with calling this conn_func-
> >atomic_check().
> We are not validating the new connector_state like atomic_check()
> seems
> to do generally but only cleaning up vcpi resources for
> compute_config()
> to later acquire. Let me know if I am wrong in my understanding what
> atomic_check() is expected to do.

Yeah looks good. I think it makes sense to have such a validation
function. There may not be much in it now but that could change when
i915 connector properties are made atomic. :)

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

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

[Daniel Vetter]

On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran
<dhinakaran.pandiyan@intel.com> wrote:
> On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
>> Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]:
>> > On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote:
>> > >
>> > > Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
>> > > >
>> > > > 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.
>> > > >
>> > > > v2: Moved the caller hunk to this patch (Daniel)
>> > > >
>> > > > 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 8795088..92bd741 100644
>> > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
>> > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> > > > @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct
>> > > > drm_device *dev,
>> > > >                 }
>> > > >         }
>> > > >
>> > > > +       for_each_connector_in_state(state, connector,
>> > > > 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 (!connector->state->crtc)
>> > > > +                       continue;
>> > > > +
>> > > > +               crtc_state =
>> > > > drm_atomic_get_existing_crtc_state(state, connector->state-
>> > > > >crtc);
>> > > > +
>> > > > +               if (crtc_state->connectors_changed ||
>> > > > +                   crtc_state->mode_changed ||
>> > > > +                   (crtc_state->active_changed && !crtc_state-
>> > > > >
>> > > > > active))
>> > > > +                       conn_funcs->atomic_release(connector,
>> > > > connector_state);
>> > > > +       }
>> > >
>> > > Could we deal with the VCPI state separately in
>> > > intel_modeset_checks,
>> > > like we do with dpll?
>> >
>> > We'd want to release the VCPI slots before they are acquired in
>> > ->compute_config(). intel_modeset_checks() will be too late to
>> > release
>> > them. Are you suggesting both acquiring and releasing slots should be
>> > done in intel_modeset_checks()?
>>
>> That makes things a bit more nasty. Maybe add a
>> conn_funcs->atomic_check that always gets called, something like I did
>> below?
>>
>> I'd love to use it for some atomic connector properties too.
>
>
> Adding and unconditionally calling conn_funcs->atomic_check() should be
> doable. It also follows the pattern we have for encoders and CRTCs. But
> I'll have to move the connector->state->crtc state checks inside the
> function.

Adding ->atomic_check that's unconditionally called sounds troubling,
because all the other ->atomic_check functions are _only_ called when
enabling stuff. ->atomic_release sounds much better to me, and from a
helper pov DK's patch above is the right place.

If that place doesn't work for i915.ko, then we need our own callback
(like we already have with e.g. ->compute_config, we could do a
->release_config). But if it's just cosmetics, then I don't see the
reason why we need to change this. On that issue: How exactly does our
compute_config work if we haven't updated the routing (using the above
helper) yet? That sounds like a pretty big misdesign on our side ...
-Daniel


>
> -DK
>> > >
>> > >
>> > > Maybe implementing the relevant VCPI state could be done as an
>> > > atomic
>> > > helper function too, so other atomic drivers can just plug it in.
>> > >
>> > The idea was to reduce boilerplate in the drivers and use the
>> > private_obj state for different object types.
>> >
>> >
>> > >
>> > > Not sure how doable this is, but if it's not too hard, then it's
>> > > probably cleaner :)
>> > > _______________________________________________
>> > > 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
>



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Pandiyan, Dhinakaran]

On Tue, 2017-02-14 at 20:51 +0100, Daniel Vetter wrote:
> On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com> wrote:
> > On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
> >> Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]:
> >> > On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote:
> >> > >
> >> > > Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
> >> > > >
> >> > > > 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.
> >> > > >
> >> > > > v2: Moved the caller hunk to this patch (Daniel)
> >> > > >
> >> > > > 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 8795088..92bd741 100644
> >> > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> > > > @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct
> >> > > > drm_device *dev,
> >> > > >                 }
> >> > > >         }
> >> > > >
> >> > > > +       for_each_connector_in_state(state, connector,
> >> > > > 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 (!connector->state->crtc)
> >> > > > +                       continue;
> >> > > > +
> >> > > > +               crtc_state =
> >> > > > drm_atomic_get_existing_crtc_state(state, connector->state-
> >> > > > >crtc);
> >> > > > +
> >> > > > +               if (crtc_state->connectors_changed ||
> >> > > > +                   crtc_state->mode_changed ||
> >> > > > +                   (crtc_state->active_changed && !crtc_state-
> >> > > > >
> >> > > > > active))
> >> > > > +                       conn_funcs->atomic_release(connector,
> >> > > > connector_state);
> >> > > > +       }
> >> > >
> >> > > Could we deal with the VCPI state separately in
> >> > > intel_modeset_checks,
> >> > > like we do with dpll?
> >> >
> >> > We'd want to release the VCPI slots before they are acquired in
> >> > ->compute_config(). intel_modeset_checks() will be too late to
> >> > release
> >> > them. Are you suggesting both acquiring and releasing slots should be
> >> > done in intel_modeset_checks()?
> >>
> >> That makes things a bit more nasty. Maybe add a
> >> conn_funcs->atomic_check that always gets called, something like I did
> >> below?
> >>
> >> I'd love to use it for some atomic connector properties too.
> >
> >
> > Adding and unconditionally calling conn_funcs->atomic_check() should be
> > doable. It also follows the pattern we have for encoders and CRTCs. But
> > I'll have to move the connector->state->crtc state checks inside the
> > function.
> 
> Adding ->atomic_check that's unconditionally called sounds troubling,
> because all the other ->atomic_check functions are _only_ called when
> enabling stuff. ->atomic_release sounds much better to me, and from a
> helper pov DK's patch above is the right place.
> 
> If that place doesn't work for i915.ko, then we need our own callback
> (like we already have with e.g. ->compute_config, we could do a
> ->release_config). But if it's just cosmetics, then I don't see the
> reason why we need to change this. On that issue: How exactly does our
> compute_config work if we haven't updated the routing (using the above
> helper) yet? That sounds like a pretty big misdesign on our side ...
> -Daniel
> 
Are you referring to the drm_atomic_helper_check_modeset() helper? We do
call it to update connector routing before compute_config .

-DK

> >
> > -DK
> >> > >
> >> > >
> >> > > Maybe implementing the relevant VCPI state could be done as an
> >> > > atomic
> >> > > helper function too, so other atomic drivers can just plug it in.
> >> > >
> >> > The idea was to reduce boilerplate in the drivers and use the
> >> > private_obj state for different object types.
> >> >
> >> >
> >> > >
> >> > > Not sure how doable this is, but if it's not too hard, then it's
> >> > > probably cleaner :)
> >> > > _______________________________________________
> >> > > Intel-gfx mailing list
> >> > > Intel-gfx@lists.freedesktop.org
> >> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> >
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> 
> 
> 

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

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

[Lankhorst, Maarten]

[-- Attachment #1.1: Type: text/plain, Size: 5762 bytes --]

Daniel Vetter schreef op di 14-02-2017 om 20:51 [+0100]:
> On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com> wrote:
> > On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
> > > Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]:
> > > > On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote:
> > > > > 
> > > > > Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-
> > > > > 0800]:
> > > > > > 
> > > > > > 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.
> > > > > > 
> > > > > > v2: Moved the caller hunk to this patch (Daniel)
> > > > > > 
> > > > > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@int
> > > > > > el.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 8795088..92bd741 100644
> > > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct
> > > > > > drm_device *dev,
> > > > > >                 }
> > > > > >         }
> > > > > > 
> > > > > > +       for_each_connector_in_state(state, connector,
> > > > > > 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 (!connector->state->crtc)
> > > > > > +                       continue;
> > > > > > +
> > > > > > +               crtc_state =
> > > > > > drm_atomic_get_existing_crtc_state(state, connector->state-
> > > > > > > crtc);
> > > > > > 
> > > > > > +
> > > > > > +               if (crtc_state->connectors_changed ||
> > > > > > +                   crtc_state->mode_changed ||
> > > > > > +                   (crtc_state->active_changed &&
> > > > > > !crtc_state-
> > > > > > > 
> > > > > > > active))
> > > > > > 
> > > > > > +                       conn_funcs-
> > > > > > >atomic_release(connector,
> > > > > > connector_state);
> > > > > > +       }
> > > > > 
> > > > > Could we deal with the VCPI state separately in
> > > > > intel_modeset_checks,
> > > > > like we do with dpll?
> > > > 
> > > > We'd want to release the VCPI slots before they are acquired in
> > > > ->compute_config(). intel_modeset_checks() will be too late to
> > > > release
> > > > them. Are you suggesting both acquiring and releasing slots
> > > > should be
> > > > done in intel_modeset_checks()?
> > > 
> > > That makes things a bit more nasty. Maybe add a
> > > conn_funcs->atomic_check that always gets called, something like
> > > I did
> > > below?
> > > 
> > > I'd love to use it for some atomic connector properties too.
> > 
> > 
> > Adding and unconditionally calling conn_funcs->atomic_check()
> > should be
> > doable. It also follows the pattern we have for encoders and CRTCs.
> > But
> > I'll have to move the connector->state->crtc state checks inside
> > the
> > function.
> 
> Adding ->atomic_check that's unconditionally called sounds troubling,
> because all the other ->atomic_check functions are _only_ called when
> enabling stuff. ->atomic_release sounds much better to me, and from a
> helper pov DK's patch above is the right place.

Having an atomic check would be nice for implementing connector
properties. Some of them may need to be validated regardless of crtc.

I would really like to be able to do the validation in atomic_check
instead of during the set_property callback. The state is not
completely valid at that point yet, so this would be a logical place.

> If that place doesn't work for i915.ko, then we need our own callback
> (like we already have with e.g. ->compute_config, we could do a
> ->release_config). But if it's just cosmetics, then I don't see the
> reason why we need to change this. On that issue: How exactly does
> our
> compute_config work if we haven't updated the routing (using the
> above
> helper) yet? That sounds like a pretty big misdesign on our side ...
> -Daniel
> 
> 
> > 
> > -DK
> > > > > 
> > > > > 
> > > > > Maybe implementing the relevant VCPI state could be done as
> > > > > an
> > > > > atomic
> > > > > helper function too, so other atomic drivers can just plug it
> > > > > in.
> > > > > 
> > > > 
> > > > The idea was to reduce boilerplate in the drivers and use the
> > > > private_obj state for different object types.
> > > > 
> > > > 
> > > > > 
> > > > > Not sure how doable this is, but if it's not too hard, then
> > > > > it's
> > > > > probably cleaner :)

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3282 bytes --]

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

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

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

[Pandiyan, Dhinakaran]

On Thu, 2017-02-16 at 09:09 +0000, Lankhorst, Maarten wrote:
> Daniel Vetter schreef op di 14-02-2017 om 20:51 [+0100]:
> > On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran
> > <dhinakaran.pandiyan@intel.com> wrote:
> > > On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
> > > > Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]:
> > > > > On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote:
> > > > > > 
> > > > > > Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-
> > > > > > 0800]:
> > > > > > > 
> > > > > > > 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.
> > > > > > > 
> > > > > > > v2: Moved the caller hunk to this patch (Daniel)
> > > > > > > 
> > > > > > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@int
> > > > > > > el.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 8795088..92bd741 100644
> > > > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct
> > > > > > > drm_device *dev,
> > > > > > >                 }
> > > > > > >         }
> > > > > > > 
> > > > > > > +       for_each_connector_in_state(state, connector,
> > > > > > > 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 (!connector->state->crtc)
> > > > > > > +                       continue;
> > > > > > > +
> > > > > > > +               crtc_state =
> > > > > > > drm_atomic_get_existing_crtc_state(state, connector->state-
> > > > > > > > crtc);
> > > > > > > 
> > > > > > > +
> > > > > > > +               if (crtc_state->connectors_changed ||
> > > > > > > +                   crtc_state->mode_changed ||
> > > > > > > +                   (crtc_state->active_changed &&
> > > > > > > !crtc_state-
> > > > > > > > 
> > > > > > > > active))
> > > > > > > 
> > > > > > > +                       conn_funcs-
> > > > > > > >atomic_release(connector,
> > > > > > > connector_state);
> > > > > > > +       }
> > > > > > 
> > > > > > Could we deal with the VCPI state separately in
> > > > > > intel_modeset_checks,
> > > > > > like we do with dpll?
> > > > > 
> > > > > We'd want to release the VCPI slots before they are acquired in
> > > > > ->compute_config(). intel_modeset_checks() will be too late to
> > > > > release
> > > > > them. Are you suggesting both acquiring and releasing slots
> > > > > should be
> > > > > done in intel_modeset_checks()?
> > > > 
> > > > That makes things a bit more nasty. Maybe add a
> > > > conn_funcs->atomic_check that always gets called, something like
> > > > I did
> > > > below?
> > > > 
> > > > I'd love to use it for some atomic connector properties too.
> > > 
> > > 
> > > Adding and unconditionally calling conn_funcs->atomic_check()
> > > should be
> > > doable. It also follows the pattern we have for encoders and CRTCs.
> > > But
> > > I'll have to move the connector->state->crtc state checks inside
> > > the
> > > function.
> > 
> > Adding ->atomic_check that's unconditionally called sounds troubling,
> > because all the other ->atomic_check functions are _only_ called when
> > enabling stuff. ->atomic_release sounds much better to me, and from a
> > helper pov DK's patch above is the right place.
> 
> Having an atomic check would be nice for implementing connector
> properties. Some of them may need to be validated regardless of crtc.
> 

Can we add this later when we need state validation that is appropriate
for an ->atomic_check()? 

-DK

> I would really like to be able to do the validation in atomic_check
> instead of during the set_property callback. The state is not
> completely valid at that point yet, so this would be a logical place.
> 
> > If that place doesn't work for i915.ko, then we need our own callback
> > (like we already have with e.g. ->compute_config, we could do a
> > ->release_config). But if it's just cosmetics, then I don't see the
> > reason why we need to change this. On that issue: How exactly does
> > our
> > compute_config work if we haven't updated the routing (using the
> > above
> > helper) yet? That sounds like a pretty big misdesign on our side ...
> > -Daniel
> > 
> > 
> > > 
> > > -DK
> > > > > > 
> > > > > > 
> > > > > > Maybe implementing the relevant VCPI state could be done as
> > > > > > an
> > > > > > atomic
> > > > > > helper function too, so other atomic drivers can just plug it
> > > > > > in.
> > > > > > 
> > > > > 
> > > > > The idea was to reduce boilerplate in the drivers and use the
> > > > > private_obj state for different object types.
> > > > > 
> > > > > 
> > > > > > 
> > > > > > Not sure how doable this is, but if it's not too hard, then
> > > > > > it's
> > > > > > probably cleaner :)
> _______________________________________________
> 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 v3 7/8] drm: Connector helper function to release resources

[Daniel Vetter]

On Fri, Feb 24, 2017 at 12:52:53AM +0000, Pandiyan, Dhinakaran wrote:
> On Thu, 2017-02-16 at 09:09 +0000, Lankhorst, Maarten wrote:
> > Daniel Vetter schreef op di 14-02-2017 om 20:51 [+0100]:
> > > On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran
> > > <dhinakaran.pandiyan@intel.com> wrote:
> > > > On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
> > > > > Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]:
> > > > > > > Could we deal with the VCPI state separately in
> > > > > > > intel_modeset_checks,
> > > > > > > like we do with dpll?
> > > > > > 
> > > > > > We'd want to release the VCPI slots before they are acquired in
> > > > > > ->compute_config(). intel_modeset_checks() will be too late to
> > > > > > release
> > > > > > them. Are you suggesting both acquiring and releasing slots
> > > > > > should be
> > > > > > done in intel_modeset_checks()?
> > > > > 
> > > > > That makes things a bit more nasty. Maybe add a
> > > > > conn_funcs->atomic_check that always gets called, something like
> > > > > I did
> > > > > below?
> > > > > 
> > > > > I'd love to use it for some atomic connector properties too.
> > > > 
> > > > 
> > > > Adding and unconditionally calling conn_funcs->atomic_check()
> > > > should be
> > > > doable. It also follows the pattern we have for encoders and CRTCs.
> > > > But
> > > > I'll have to move the connector->state->crtc state checks inside
> > > > the
> > > > function.
> > > 
> > > Adding ->atomic_check that's unconditionally called sounds troubling,
> > > because all the other ->atomic_check functions are _only_ called when
> > > enabling stuff. ->atomic_release sounds much better to me, and from a
> > > helper pov DK's patch above is the right place.
> > 
> > Having an atomic check would be nice for implementing connector
> > properties. Some of them may need to be validated regardless of crtc.
> > 
> 
> Can we add this later when we need state validation that is appropriate
> for an ->atomic_check()? 

+1 on not solving problems we don't have yet :-) If we'd write code for
every eventuality that we can come up with, we'd get nothing done. And
ime, such unused code will also be full of bugs.

Discussing issues like this is still good and useful, just to make sure we
have a coherent plan for the eventual future, once it happens.
-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 v3 7/8] drm: Connector helper function to release resources

[Lankhorst, Maarten]

Daniel Vetter schreef op zo 26-02-2017 om 21:00 [+0100]:
> On Fri, Feb 24, 2017 at 12:52:53AM +0000, Pandiyan, Dhinakaran wrote:
> > 
> > On Thu, 2017-02-16 at 09:09 +0000, Lankhorst, Maarten wrote:
> > > 
> > > Daniel Vetter schreef op di 14-02-2017 om 20:51 [+0100]:
> > > > 
> > > > On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran
> > > > <dhinakaran.pandiyan@intel.com> wrote:
> > > > > 
> > > > > On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
> > > > > > 
> > > > > > Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55
> > > > > > [+0000]:
> > > > > > > 
> > > > > > > > 
> > > > > > > > Could we deal with the VCPI state separately in
> > > > > > > > intel_modeset_checks,
> > > > > > > > like we do with dpll?
> > > > > > > 
> > > > > > > We'd want to release the VCPI slots before they are
> > > > > > > acquired in
> > > > > > > ->compute_config(). intel_modeset_checks() will be too
> > > > > > > late to
> > > > > > > release
> > > > > > > them. Are you suggesting both acquiring and releasing
> > > > > > > slots
> > > > > > > should be
> > > > > > > done in intel_modeset_checks()?
> > > > > > 
> > > > > > That makes things a bit more nasty. Maybe add a
> > > > > > conn_funcs->atomic_check that always gets called, something
> > > > > > like
> > > > > > I did
> > > > > > below?
> > > > > > 
> > > > > > I'd love to use it for some atomic connector properties
> > > > > > too.
> > > > > 
> > > > > 
> > > > > Adding and unconditionally calling conn_funcs->atomic_check()
> > > > > should be
> > > > > doable. It also follows the pattern we have for encoders and
> > > > > CRTCs.
> > > > > But
> > > > > I'll have to move the connector->state->crtc state checks
> > > > > inside
> > > > > the
> > > > > function.
> > > > 
> > > > Adding ->atomic_check that's unconditionally called sounds
> > > > troubling,
> > > > because all the other ->atomic_check functions are _only_
> > > > called when
> > > > enabling stuff. ->atomic_release sounds much better to me, and
> > > > from a
> > > > helper pov DK's patch above is the right place.
> > > 
> > > Having an atomic check would be nice for implementing connector
> > > properties. Some of them may need to be validated regardless of
> > > crtc.
> > > 
> > 
> > Can we add this later when we need state validation that is
> > appropriate
> > for an ->atomic_check()? 
> 
> +1 on not solving problems we don't have yet :-) If we'd write code
> for
> every eventuality that we can come up with, we'd get nothing done.
> And
> ime, such unused code will also be full of bugs.
> 
> Discussing issues like this is still good and useful, just to make
> sure we
> have a coherent plan for the eventual future, once it happens.

Near future, I'm working on converting i915 specific connector
properties to atomic, and it would be nice if I had a connector atomic
check function like this. :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dhinakaran Pandiyan]

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

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

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 9b9dda8..7aec0c7 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,
@@ -401,6 +424,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

✓ Fi.CI.BAT: success for Adding driver-private objects to atomic state

[Patchwork]

== Series Details ==

Series: Adding driver-private objects to atomic state
URL   : https://patchwork.freedesktop.org/series/19355/
State : success

== Summary ==

Series 19355v1 Adding driver-private objects to atomic state
https://patchwork.freedesktop.org/api/1.0/series/19355/revisions/1/mbox/


fi-bdw-5557u     total:252  pass:238  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:252  pass:213  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:252  pass:230  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:252  pass:225  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:252  pass:221  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:252  pass:233  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:252  pass:233  dwarn:0   dfail:0   fail:0   skip:19 
fi-ilk-650       total:252  pass:199  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:252  pass:231  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:252  pass:231  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:252  pass:229  dwarn:0   dfail:0   fail:2   skip:21 
fi-skl-6260u     total:252  pass:239  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:252  pass:232  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:252  pass:227  dwarn:4   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:252  pass:239  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:252  pass:221  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:252  pass:220  dwarn:0   dfail:0   fail:0   skip:32 

29b6014fdfab0a48deb05fc5cc5b33ba00042118 drm-tip: 2017y-02m-08d-21h-01m-50s UTC integration manifest
800bfe4 drm/dp: Track MST link bandwidth
f5dcddf drm: Connector helper function to release resources
0320fb9 drm/dp: Add DP MST helpers to atomically find and release vcpi slots
8b37401 drm/dp: Introduce MST topology state to track available link bandwidth
01d509c drm: Add driver-private objects to atomic state
058eb4f drm/dp: Split drm_dp_mst_allocate_vcpi
5e7de0e drm/dp: Kill unused MST vcpi slot availability tracking
4d665e1 drm/dp: Kill total_pbn and total_slots in struct drm_dp_mst_topology_mgr

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3746/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v7 0/4] Adding driver-private objects to atomic state

[Dhinakaran Pandiyan]

Changes in this version:
Used connector->atomic_check() to release vcpi slots instead of the
atomic_release() callback.

This series introduces void * type driver-private objects in core and adds
helper functions that operate on these private objects. Drivers need to
implement object-specific functions to swap and clear object states. The
advantage of having void * for these objects in the core is objects of different
types can be managed in the same atomic state array. The series implements
DP-MST link bw tracking using the driver-private object infrastructure.

Pandiyan, Dhinakaran (4):
  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/dp: Track MST link bandwidth

 drivers/gpu/drm/drm_atomic.c          |  65 +++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c   |   5 ++
 drivers/gpu/drm/drm_dp_mst_topology.c | 150 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp_mst.c   |  57 +++++++++++--
 include/drm/drm_atomic.h              | 101 +++++++++++++++++++++++
 include/drm/drm_dp_mst_helper.h       |  26 ++++++
 6 files changed, 398 insertions(+), 6 deletions(-)

-- 
2.7.4

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

✓ Fi.CI.BAT: success for Adding driver-private objects to atomic state

[Patchwork]

== Series Details ==

Series: Adding driver-private objects to atomic state
URL   : https://patchwork.freedesktop.org/series/23308/
State : success

== Summary ==

Series 23308v1 Adding driver-private objects to atomic state
https://patchwork.freedesktop.org/api/1.0/series/23308/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                fail       -> PASS       (fi-skl-6700k) fdo#100367
Test pm_rpm:
        Subgroup basic-rte:
                incomplete -> PASS       (fi-bsw-n3050) fdo#100718

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100367 https://bugs.freedesktop.org/show_bug.cgi?id=100367
fdo#100718 https://bugs.freedesktop.org/show_bug.cgi?id=100718

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:437s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:427s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:588s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:511s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time:542s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:481s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:487s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:414s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:406s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:423s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:492s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:486s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:454s
fi-kbl-7560u     total:278  pass:267  dwarn:1   dfail:0   fail:0   skip:10  time:568s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:448s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:568s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:456s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:499s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:428s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:541s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time:405s

9bb844000d9412c2758b861ce7aee1e9546fccb9 drm-tip: 2017y-04m-20d-14h-46m-59s UTC integration manifest
3347ee1 drm/dp: Track MST link bandwidth
8fef895b4 drm/dp: Add DP MST helpers to atomically find and release vcpi slots
403e426 drm/dp: Introduce MST topology state to track available link bandwidth
4eb3832 drm: Add driver-private objects to atomic state

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4527/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RESEND FOR CI PATCH v8 0/4] Adding driver-private objects to atomic state

[Dhinakaran Pandiyan]

Resending for CI.

Pandiyan, Dhinakaran (4):
  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/dp: Track MST link bandwidth

 drivers/gpu/drm/drm_atomic.c          |  65 +++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c   |   5 ++
 drivers/gpu/drm/drm_dp_mst_topology.c | 150 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp_mst.c   |  48 +++++++++--
 include/drm/drm_atomic.h              |  95 +++++++++++++++++++++
 include/drm/drm_dp_mst_helper.h       |  26 ++++++
 6 files changed, 383 insertions(+), 6 deletions(-)

-- 
2.7.4

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

✓ Fi.CI.BAT: success for Adding driver-private objects to atomic state

[Patchwork]

== Series Details ==

Series: Adding driver-private objects to atomic state
URL   : https://patchwork.freedesktop.org/series/23775/
State : success

== Summary ==

Series 23775v1 Adding driver-private objects to atomic state
https://patchwork.freedesktop.org/api/1.0/series/23775/revisions/1/mbox/

Test gvt_basic:
        Subgroup invalid-placeholder-test:
                skip       -> INCOMPLETE (fi-hsw-4770r) fdo#100256

fdo#100256 https://bugs.freedesktop.org/show_bug.cgi?id=100256

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:431s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:434s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:566s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:507s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time:544s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:485s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:486s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:410s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:15 
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:414s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:492s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:467s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:456s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:671s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:457s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:577s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:461s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:488s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:529s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:409s
fi-skl-gvtdvm failed to collect. IGT log at Patchwork_4588/fi-skl-gvtdvm/igt.log

1d490e4b6d5324cfbf8dc800cf4a99471252802c drm-tip: 2017y-04m-28d-14h-14m-47s UTC integration manifest
0e053d7 drm/dp: Track MST link bandwidth
e47590e drm/dp: Add DP MST helpers to atomically find and release vcpi slots
57ae56a drm/dp: Introduce MST topology state to track available link bandwidth
969f6f6 drm: Add driver-private objects to atomic state

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4588/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx