[PATCH 1/4] drm/atomic: integrate modeset lock with private objects

[PATCH 1/4] drm/atomic: integrate modeset lock with private objects

[Rob Clark]

Follow the same pattern of locking as with other state objects.  This
avoids boilerplate in the driver.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/drm_atomic.c | 9 ++++++++-
 include/drm/drm_atomic.h     | 5 +++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index fc8c4da409ff..004e621ab307 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj,
 {
 	memset(obj, 0, sizeof(*obj));
 
+	drm_modeset_lock_init(&obj->lock);
+
 	obj->state = state;
 	obj->funcs = funcs;
 }
@@ -1093,6 +1095,7 @@ void
 drm_atomic_private_obj_fini(struct drm_private_obj *obj)
 {
 	obj->funcs->atomic_destroy_state(obj, obj->state);
+	drm_modeset_lock_fini(&obj->lock);
 }
 EXPORT_SYMBOL(drm_atomic_private_obj_fini);
 
@@ -1113,7 +1116,7 @@ struct drm_private_state *
 drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
 				 struct drm_private_obj *obj)
 {
-	int index, num_objs, i;
+	int index, num_objs, i, ret;
 	size_t size;
 	struct __drm_private_objs_state *arr;
 	struct drm_private_state *obj_state;
@@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
 		if (obj == state->private_objs[i].ptr)
 			return state->private_objs[i].state;
 
+	ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);
+	if (ret)
+		return ERR_PTR(ret);
+
 	num_objs = state->num_private_objs + 1;
 	size = sizeof(*state->private_objs) * num_objs;
 	arr = krealloc(state->private_objs, size, GFP_KERNEL);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 09076a625637..9ae53b73c9d2 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -218,6 +218,11 @@ struct drm_private_state_funcs {
  * &drm_modeset_lock is required to duplicate and update this object's state.
  */
 struct drm_private_obj {
+	/**
+	 * @lock: Modeset lock to protect the state object.
+	 */
+	struct drm_modeset_lock lock;
+
 	/**
 	 * @state: Current atomic state for this driver private object.
 	 */
-- 
2.14.3

[PATCH 2/4] drm/msm/mdp5: Add global state as a private atomic object

[Rob Clark]

From: Archit Taneja <architt@codeaurora.org>

Global shared resources (hwpipes, hwmixers and SMP) for MDP5 are
implemented as a part of atomic state by subclassing drm_atomic_state.

The preferred approach is to use the drm_private_obj infrastructure
available in the atomic core.

mdp5_global_state is introduced as a drm atomic private object. The two
funcs mdp5_get_global_state() and mdp5_get_existing_global_state() are
the two variants that will be used to access mdp5_global_state.

This will replace the existing mdp5_state struct (which subclasses
drm_atomic_state) and the funcs around it. These will be removed later
once we mdp5_global_state is put to use everywhere.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 79 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h | 24 ++++++++++
 2 files changed, 103 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 7ad0560dbc9f..0f314c1f3e8a 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -106,6 +106,79 @@ static void mdp5_swap_state(struct msm_kms *kms, struct drm_atomic_state *state)
 	swap(to_kms_state(state)->state, mdp5_kms->state);
 }
 
+/* Global/shared object state funcs */
+
+/*
+ * This is a helper that returns the private state currently in operation.
+ * Note that this would return the "old_state" if called in the atomic check
+ * path, and the "new_state" after the atomic swap has been done.
+ */
+struct mdp5_global_state *
+mdp5_get_existing_global_state(struct mdp5_kms *mdp5_kms)
+{
+	return to_mdp5_global_state(mdp5_kms->glob_state.state);
+}
+
+/*
+ * This acquires the modeset lock set aside for global state, creates
+ * a new duplicated private object state.
+ */
+struct mdp5_global_state *mdp5_get_global_state(struct drm_atomic_state *s)
+{
+	struct msm_drm_private *priv = s->dev->dev_private;
+	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
+	struct drm_private_state *priv_state;
+
+	priv_state = drm_atomic_get_private_obj_state(s, &mdp5_kms->glob_state);
+	if (IS_ERR(priv_state))
+		return ERR_CAST(priv_state);
+
+	return to_mdp5_global_state(priv_state);
+}
+
+static struct drm_private_state *
+mdp5_global_duplicate_state(struct drm_private_obj *obj)
+{
+	struct mdp5_global_state *state;
+
+	state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return NULL;
+
+	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
+
+	return &state->base;
+}
+
+static void mdp5_global_destroy_state(struct drm_private_obj *obj,
+				      struct drm_private_state *state)
+{
+	struct mdp5_global_state *mdp5_state = to_mdp5_global_state(state);
+
+	kfree(mdp5_state);
+}
+
+static const struct drm_private_state_funcs mdp5_global_state_funcs = {
+	.atomic_duplicate_state = mdp5_global_duplicate_state,
+	.atomic_destroy_state = mdp5_global_destroy_state,
+};
+
+static int mdp5_global_obj_init(struct mdp5_kms *mdp5_kms)
+{
+	struct mdp5_global_state *state;
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;
+
+	state->mdp5_kms = mdp5_kms;
+
+	drm_atomic_private_obj_init(&mdp5_kms->glob_state,
+				    &state->base,
+				    &mdp5_global_state_funcs);
+	return 0;
+}
+
 static void mdp5_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *state)
 {
 	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
@@ -779,6 +852,8 @@ static void mdp5_destroy(struct platform_device *pdev)
 	if (mdp5_kms->rpm_enabled)
 		pm_runtime_disable(&pdev->dev);
 
+	drm_atomic_private_obj_fini(&mdp5_kms->glob_state);
+
 	kfree(mdp5_kms->state);
 }
 
@@ -939,6 +1014,10 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev)
 		goto fail;
 	}
 
+	ret = mdp5_global_obj_init(mdp5_kms);
+	if (ret)
+		goto fail;
+
 	mdp5_kms->mmio = msm_ioremap(pdev, "mdp_phys", "MDP5");
 	if (IS_ERR(mdp5_kms->mmio)) {
 		ret = PTR_ERR(mdp5_kms->mmio);
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
index 312e6a47dff8..1c70aac20a98 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
@@ -57,6 +57,12 @@ struct mdp5_kms {
 	struct mdp5_state *state;
 	struct drm_modeset_lock state_lock;
 
+	/*
+	 * Global private object state, Do not access directly, use
+	 * mdp5_global_get_state()
+	 */
+	struct drm_private_obj glob_state;
+
 	struct mdp5_smp *smp;
 	struct mdp5_ctl_manager *ctlm;
 
@@ -97,6 +103,24 @@ struct mdp5_state {
 struct mdp5_state *__must_check
 mdp5_get_state(struct drm_atomic_state *s);
 
+/* Global private object state for tracking resources that are shared across
+ * multiple kms objects (planes/crtcs/etc).
+ */
+#define to_mdp5_global_state(x) container_of(x, struct mdp5_global_state, base)
+struct mdp5_global_state {
+	struct drm_private_state base;
+
+	struct drm_atomic_state *state;
+	struct mdp5_kms *mdp5_kms;
+
+	struct mdp5_hw_pipe_state hwpipe;
+	struct mdp5_hw_mixer_state hwmixer;
+	struct mdp5_smp_state smp;
+};
+
+struct mdp5_global_state * mdp5_get_existing_global_state(struct mdp5_kms *mdp5_kms);
+struct mdp5_global_state *__must_check mdp5_get_global_state(struct drm_atomic_state *s);
+
 /* Atomic plane state.  Subclasses the base drm_plane_state in order to
  * track assigned hwpipe and hw specific state.
  */
-- 
2.14.3

[PATCH 3/4] drm/msm/mdp5: Use the new private_obj state

[Rob Clark]

From: Archit Taneja <architt@codeaurora.org>

This replaces the usage of the subclassed atomic state (mdp5_state)
with a private_obj state embedded within drm_atomic_state. The latter
method is the preferred approach, since it's simpler to implement
and less prone to errors.

The new API replaces the older and equivalent mdp5_state usage in the
following pattern:
- References to "mdp5_kms->state" (i.e, the old/existing state) is
  replaced with mdp5_get_existing_global_state(). In the atomic_check
  path, this should be called with the glob_state_lock drm_modeset_lock
  alredy taken.
- References to "mdp5_get_state()" are replaced with
  mdp5_get_global_state(). This acquires glob_state_lock and uses
  drm_atomic_get_private_obj_state() to create a new duplicated state.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c   | 10 ++++++++--
 drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c | 12 ++++++------
 drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c  | 20 +++++++++++---------
 drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c   | 17 ++++++++++++-----
 4 files changed, 37 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 0f314c1f3e8a..a7e80ebf0207 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -183,11 +183,14 @@ static void mdp5_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *st
 {
 	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
 	struct device *dev = &mdp5_kms->pdev->dev;
+	struct mdp5_global_state *global_state;
+
+	global_state = mdp5_get_existing_global_state(mdp5_kms);
 
 	pm_runtime_get_sync(dev);
 
 	if (mdp5_kms->smp)
-		mdp5_smp_prepare_commit(mdp5_kms->smp, &mdp5_kms->state->smp);
+		mdp5_smp_prepare_commit(mdp5_kms->smp, &global_state->smp);
 }
 
 static void mdp5_commit(struct msm_kms *kms, struct drm_atomic_state *state)
@@ -209,9 +212,12 @@ static void mdp5_complete_commit(struct msm_kms *kms, struct drm_atomic_state *s
 {
 	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
 	struct device *dev = &mdp5_kms->pdev->dev;
+	struct mdp5_global_state *global_state;
+
+	global_state = mdp5_get_existing_global_state(mdp5_kms);
 
 	if (mdp5_kms->smp)
-		mdp5_smp_complete_commit(mdp5_kms->smp, &mdp5_kms->state->smp);
+		mdp5_smp_complete_commit(mdp5_kms->smp, &global_state->smp);
 
 	pm_runtime_put_sync(dev);
 }
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c
index 8a00991f03c7..113e6b569562 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c
@@ -52,14 +52,14 @@ int mdp5_mixer_assign(struct drm_atomic_state *s, struct drm_crtc *crtc,
 {
 	struct msm_drm_private *priv = s->dev->dev_private;
 	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
-	struct mdp5_state *state = mdp5_get_state(s);
+	struct mdp5_global_state *global_state = mdp5_get_global_state(s);
 	struct mdp5_hw_mixer_state *new_state;
 	int i;
 
-	if (IS_ERR(state))
-		return PTR_ERR(state);
+	if (IS_ERR(global_state))
+		return PTR_ERR(global_state);
 
-	new_state = &state->hwmixer;
+	new_state = &global_state->hwmixer;
 
 	for (i = 0; i < mdp5_kms->num_hwmixers; i++) {
 		struct mdp5_hw_mixer *cur = mdp5_kms->hwmixers[i];
@@ -129,8 +129,8 @@ int mdp5_mixer_assign(struct drm_atomic_state *s, struct drm_crtc *crtc,
 
 void mdp5_mixer_release(struct drm_atomic_state *s, struct mdp5_hw_mixer *mixer)
 {
-	struct mdp5_state *state = mdp5_get_state(s);
-	struct mdp5_hw_mixer_state *new_state = &state->hwmixer;
+	struct mdp5_global_state *global_state = mdp5_get_global_state(s);
+	struct mdp5_hw_mixer_state *new_state = &global_state->hwmixer;
 
 	if (!mixer)
 		return;
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
index ff52c49095f9..1ef26bc63163 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
@@ -24,17 +24,19 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct drm_plane *plane,
 {
 	struct msm_drm_private *priv = s->dev->dev_private;
 	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
-	struct mdp5_state *state;
+	struct mdp5_global_state *new_global_state, *old_global_state;
 	struct mdp5_hw_pipe_state *old_state, *new_state;
 	int i, j;
 
-	state = mdp5_get_state(s);
-	if (IS_ERR(state))
-		return PTR_ERR(state);
+	new_global_state = mdp5_get_global_state(s);
+	if (IS_ERR(new_global_state))
+		return PTR_ERR(new_global_state);
 
-	/* grab old_state after mdp5_get_state(), since now we hold lock: */
-	old_state = &mdp5_kms->state->hwpipe;
-	new_state = &state->hwpipe;
+	/* grab old_state after mdp5_get_global_state(), since now we hold lock: */
+	old_global_state = mdp5_get_existing_global_state(mdp5_kms);
+
+	old_state = &old_global_state->hwpipe;
+	new_state = &new_global_state->hwpipe;
 
 	for (i = 0; i < mdp5_kms->num_hwpipes; i++) {
 		struct mdp5_hw_pipe *cur = mdp5_kms->hwpipes[i];
@@ -107,7 +109,7 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct drm_plane *plane,
 		WARN_ON(r_hwpipe);
 
 		DBG("%s: alloc SMP blocks", (*hwpipe)->name);
-		ret = mdp5_smp_assign(mdp5_kms->smp, &state->smp,
+		ret = mdp5_smp_assign(mdp5_kms->smp, &new_global_state->smp,
 				(*hwpipe)->pipe, blkcfg);
 		if (ret)
 			return -ENOMEM;
@@ -132,7 +134,7 @@ void mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe *hwpipe)
 {
 	struct msm_drm_private *priv = s->dev->dev_private;
 	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
-	struct mdp5_state *state = mdp5_get_state(s);
+	struct mdp5_global_state *state = mdp5_get_global_state(s);
 	struct mdp5_hw_pipe_state *new_state = &state->hwpipe;
 
 	if (!hwpipe)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c
index ae4983d9d0a5..1408eec75440 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c
@@ -340,17 +340,20 @@ void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p)
 	struct mdp5_kms *mdp5_kms = get_kms(smp);
 	struct mdp5_hw_pipe_state *hwpstate;
 	struct mdp5_smp_state *state;
+	struct mdp5_global_state *global_state;
 	int total = 0, i, j;
 
 	drm_printf(p, "name\tinuse\tplane\n");
 	drm_printf(p, "----\t-----\t-----\n");
 
 	if (drm_can_sleep())
-		drm_modeset_lock(&mdp5_kms->state_lock, NULL);
+		drm_modeset_lock(&mdp5_kms->glob_state.lock, NULL);
+
+	global_state = mdp5_get_existing_global_state(mdp5_kms);
 
 	/* grab these *after* we hold the state_lock */
-	hwpstate = &mdp5_kms->state->hwpipe;
-	state = &mdp5_kms->state->smp;
+	hwpstate = &global_state->hwpipe;
+	state = &global_state->smp;
 
 	for (i = 0; i < mdp5_kms->num_hwpipes; i++) {
 		struct mdp5_hw_pipe *hwpipe = mdp5_kms->hwpipes[i];
@@ -374,7 +377,7 @@ void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p)
 			bitmap_weight(state->state, smp->blk_cnt));
 
 	if (drm_can_sleep())
-		drm_modeset_unlock(&mdp5_kms->state_lock);
+		drm_modeset_unlock(&mdp5_kms->glob_state.lock);
 }
 
 void mdp5_smp_destroy(struct mdp5_smp *smp)
@@ -384,7 +387,8 @@ void mdp5_smp_destroy(struct mdp5_smp *smp)
 
 struct mdp5_smp *mdp5_smp_init(struct mdp5_kms *mdp5_kms, const struct mdp5_smp_block *cfg)
 {
-	struct mdp5_smp_state *state = &mdp5_kms->state->smp;
+	struct mdp5_smp_state *state;
+	struct mdp5_global_state *global_state;
 	struct mdp5_smp *smp = NULL;
 	int ret;
 
@@ -398,6 +402,9 @@ struct mdp5_smp *mdp5_smp_init(struct mdp5_kms *mdp5_kms, const struct mdp5_smp_
 	smp->blk_cnt = cfg->mmb_count;
 	smp->blk_size = cfg->mmb_size;
 
+	global_state = mdp5_get_existing_global_state(mdp5_kms);
+	state = &global_state->smp;
+
 	/* statically tied MMBs cannot be re-allocated: */
 	bitmap_copy(state->state, cfg->reserved_state, smp->blk_cnt);
 	memcpy(smp->reserved, cfg->reserved, sizeof(smp->reserved));
-- 
2.14.3

[PATCH 4/4] drm/msm: Don't subclass drm_atomic_state anymore

[Rob Clark]

From: Archit Taneja <architt@codeaurora.org>

With the addition of "private_objs" in drm_atomic_state, we no longer
need to subclass drm_atomic_state to store state of share resources
that don't perfectly fit within planes/crtc/connector state information.
We can now save this state within drm_atomic_state itself using
the private objects.

Remove the infrastructure that allowed subclassing of drm_atomic_state
in the driver.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 46 --------------------------------
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h | 22 ---------------
 drivers/gpu/drm/msm/msm_atomic.c         | 31 ---------------------
 drivers/gpu/drm/msm/msm_drv.c            |  3 ---
 drivers/gpu/drm/msm/msm_kms.h            | 14 ----------
 5 files changed, 116 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index a7e80ebf0207..da9c4a4eb050 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -70,42 +70,6 @@ static int mdp5_hw_init(struct msm_kms *kms)
 	return 0;
 }
 
-struct mdp5_state *mdp5_get_state(struct drm_atomic_state *s)
-{
-	struct msm_drm_private *priv = s->dev->dev_private;
-	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
-	struct msm_kms_state *state = to_kms_state(s);
-	struct mdp5_state *new_state;
-	int ret;
-
-	if (state->state)
-		return state->state;
-
-	ret = drm_modeset_lock(&mdp5_kms->state_lock, s->acquire_ctx);
-	if (ret)
-		return ERR_PTR(ret);
-
-	new_state = kmalloc(sizeof(*mdp5_kms->state), GFP_KERNEL);
-	if (!new_state)
-		return ERR_PTR(-ENOMEM);
-
-	/* Copy state: */
-	new_state->hwpipe = mdp5_kms->state->hwpipe;
-	new_state->hwmixer = mdp5_kms->state->hwmixer;
-	if (mdp5_kms->smp)
-		new_state->smp = mdp5_kms->state->smp;
-
-	state->state = new_state;
-
-	return new_state;
-}
-
-static void mdp5_swap_state(struct msm_kms *kms, struct drm_atomic_state *state)
-{
-	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
-	swap(to_kms_state(state)->state, mdp5_kms->state);
-}
-
 /* Global/shared object state funcs */
 
 /*
@@ -323,7 +287,6 @@ static const struct mdp_kms_funcs kms_funcs = {
 		.irq             = mdp5_irq,
 		.enable_vblank   = mdp5_enable_vblank,
 		.disable_vblank  = mdp5_disable_vblank,
-		.swap_state      = mdp5_swap_state,
 		.prepare_commit  = mdp5_prepare_commit,
 		.commit          = mdp5_commit,
 		.complete_commit = mdp5_complete_commit,
@@ -859,8 +822,6 @@ static void mdp5_destroy(struct platform_device *pdev)
 		pm_runtime_disable(&pdev->dev);
 
 	drm_atomic_private_obj_fini(&mdp5_kms->glob_state);
-
-	kfree(mdp5_kms->state);
 }
 
 static int construct_pipes(struct mdp5_kms *mdp5_kms, int cnt,
@@ -1013,13 +974,6 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev)
 	mdp5_kms->dev = dev;
 	mdp5_kms->pdev = pdev;
 
-	drm_modeset_lock_init(&mdp5_kms->state_lock);
-	mdp5_kms->state = kzalloc(sizeof(*mdp5_kms->state), GFP_KERNEL);
-	if (!mdp5_kms->state) {
-		ret = -ENOMEM;
-		goto fail;
-	}
-
 	ret = mdp5_global_obj_init(mdp5_kms);
 	if (ret)
 		goto fail;
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
index 1c70aac20a98..a75fbd3aeaf7 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
@@ -30,8 +30,6 @@
 #include "mdp5_ctl.h"
 #include "mdp5_smp.h"
 
-struct mdp5_state;
-
 struct mdp5_kms {
 	struct mdp_kms base;
 
@@ -51,12 +49,6 @@ struct mdp5_kms {
 	struct mdp5_cfg_handler *cfg;
 	uint32_t caps;	/* MDP capabilities (MDP_CAP_XXX bits) */
 
-	/**
-	 * Global atomic state.  Do not access directly, use mdp5_get_state()
-	 */
-	struct mdp5_state *state;
-	struct drm_modeset_lock state_lock;
-
 	/*
 	 * Global private object state, Do not access directly, use
 	 * mdp5_global_get_state()
@@ -89,20 +81,6 @@ struct mdp5_kms {
 };
 #define to_mdp5_kms(x) container_of(x, struct mdp5_kms, base)
 
-/* Global atomic state for tracking resources that are shared across
- * multiple kms objects (planes/crtcs/etc).
- *
- * For atomic updates which require modifying global state,
- */
-struct mdp5_state {
-	struct mdp5_hw_pipe_state hwpipe;
-	struct mdp5_hw_mixer_state hwmixer;
-	struct mdp5_smp_state smp;
-};
-
-struct mdp5_state *__must_check
-mdp5_get_state(struct drm_atomic_state *s);
-
 /* Global private object state for tracking resources that are shared across
  * multiple kms objects (planes/crtcs/etc).
  */
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index 9de579e0a1df..761d9bd65302 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -226,11 +226,7 @@ int msm_atomic_commit(struct drm_device *dev,
 	 * This is the point of no return - everything below never fails except
 	 * when the hw goes bonghits. Which means we can commit the new state on
 	 * the software side now.
-	 *
-	 * swap driver private state while still holding state_lock
 	 */
-	if (to_kms_state(state)->state)
-		priv->kms->funcs->swap_state(priv->kms, state);
 
 	/*
 	 * Everything below can be run asynchronously without the need to grab
@@ -264,30 +260,3 @@ int msm_atomic_commit(struct drm_device *dev,
 	drm_atomic_helper_cleanup_planes(dev, state);
 	return ret;
 }
-
-struct drm_atomic_state *msm_atomic_state_alloc(struct drm_device *dev)
-{
-	struct msm_kms_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
-
-	if (!state || drm_atomic_state_init(dev, &state->base) < 0) {
-		kfree(state);
-		return NULL;
-	}
-
-	return &state->base;
-}
-
-void msm_atomic_state_clear(struct drm_atomic_state *s)
-{
-	struct msm_kms_state *state = to_kms_state(s);
-	drm_atomic_state_default_clear(&state->base);
-	kfree(state->state);
-	state->state = NULL;
-}
-
-void msm_atomic_state_free(struct drm_atomic_state *state)
-{
-	kfree(to_kms_state(state)->state);
-	drm_atomic_state_default_release(state);
-	kfree(state);
-}
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 65d3b0bb7c86..a631e94ca731 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -42,9 +42,6 @@ static const struct drm_mode_config_funcs mode_config_funcs = {
 	.output_poll_changed = drm_fb_helper_output_poll_changed,
 	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = msm_atomic_commit,
-	.atomic_state_alloc = msm_atomic_state_alloc,
-	.atomic_state_clear = msm_atomic_state_clear,
-	.atomic_state_free = msm_atomic_state_free,
 };
 
 #ifdef CONFIG_DRM_MSM_REGISTER_LOGGING
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 7585a6b988c7..c776651eed08 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -40,8 +40,6 @@ struct msm_kms_funcs {
 	irqreturn_t (*irq)(struct msm_kms *kms);
 	int (*enable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc);
 	void (*disable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc);
-	/* swap global atomic state: */
-	void (*swap_state)(struct msm_kms *kms, struct drm_atomic_state *state);
 	/* modeset, bracketing atomic_commit(): */
 	void (*prepare_commit)(struct msm_kms *kms, struct drm_atomic_state *state);
 	void (*commit)(struct msm_kms *kms, struct drm_atomic_state *state);
@@ -78,18 +76,6 @@ struct msm_kms {
 	struct msm_gem_address_space *aspace;
 };
 
-/**
- * Subclass of drm_atomic_state, to allow kms backend to have driver
- * private global state.  The kms backend can do whatever it wants
- * with the ->state ptr.  On ->atomic_state_clear() the ->state ptr
- * is kfree'd and set back to NULL.
- */
-struct msm_kms_state {
-	struct drm_atomic_state base;
-	void *state;
-};
-#define to_kms_state(x) container_of(x, struct msm_kms_state, base)
-
 static inline void msm_kms_init(struct msm_kms *kms,
 		const struct msm_kms_funcs *funcs)
 {
-- 
2.14.3

Re: [PATCH 1/4] drm/atomic: integrate modeset lock with private objects

[Ville Syrjälä]

On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote:
> Follow the same pattern of locking as with other state objects.  This
> avoids boilerplate in the driver.

I'm not sure we really want to do this. What if the driver wants a
custom locking scheme for this state?

> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/gpu/drm/drm_atomic.c | 9 ++++++++-
>  include/drm/drm_atomic.h     | 5 +++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index fc8c4da409ff..004e621ab307 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj,
>  {
>  	memset(obj, 0, sizeof(*obj));
>  
> +	drm_modeset_lock_init(&obj->lock);
> +
>  	obj->state = state;
>  	obj->funcs = funcs;
>  }
> @@ -1093,6 +1095,7 @@ void
>  drm_atomic_private_obj_fini(struct drm_private_obj *obj)
>  {
>  	obj->funcs->atomic_destroy_state(obj, obj->state);
> +	drm_modeset_lock_fini(&obj->lock);
>  }
>  EXPORT_SYMBOL(drm_atomic_private_obj_fini);
>  
> @@ -1113,7 +1116,7 @@ struct drm_private_state *
>  drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
>  				 struct drm_private_obj *obj)
>  {
> -	int index, num_objs, i;
> +	int index, num_objs, i, ret;
>  	size_t size;
>  	struct __drm_private_objs_state *arr;
>  	struct drm_private_state *obj_state;
> @@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
>  		if (obj == state->private_objs[i].ptr)
>  			return state->private_objs[i].state;
>  
> +	ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>  	num_objs = state->num_private_objs + 1;
>  	size = sizeof(*state->private_objs) * num_objs;
>  	arr = krealloc(state->private_objs, size, GFP_KERNEL);
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 09076a625637..9ae53b73c9d2 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
>   * &drm_modeset_lock is required to duplicate and update this object's state.
>   */
>  struct drm_private_obj {
> +	/**
> +	 * @lock: Modeset lock to protect the state object.
> +	 */
> +	struct drm_modeset_lock lock;
> +
>  	/**
>  	 * @state: Current atomic state for this driver private object.
>  	 */
> -- 
> 2.14.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/4] drm/atomic: integrate modeset lock with private objects

[Rob Clark]

On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote:
>> Follow the same pattern of locking as with other state objects.  This
>> avoids boilerplate in the driver.
>
> I'm not sure we really want to do this. What if the driver wants a
> custom locking scheme for this state?

That seems like something we want to discourage, ie. all the more
reason for this patch.

There is no reason drivers could not split their global state into
multiple private objs's, each with their own lock, for more fine
grained locking.  That is basically the only valid reason I can think
of for "custom locking".

(And ofc drivers could add there own locks in addition to what is done
by core, but I'd rather look at that on a case by case basis, rather
than it being part of the boilerplate in each driver.)

BR,
-R

>>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> ---
>>  drivers/gpu/drm/drm_atomic.c | 9 ++++++++-
>>  include/drm/drm_atomic.h     | 5 +++++
>>  2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index fc8c4da409ff..004e621ab307 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj,
>>  {
>>       memset(obj, 0, sizeof(*obj));
>>
>> +     drm_modeset_lock_init(&obj->lock);
>> +
>>       obj->state = state;
>>       obj->funcs = funcs;
>>  }
>> @@ -1093,6 +1095,7 @@ void
>>  drm_atomic_private_obj_fini(struct drm_private_obj *obj)
>>  {
>>       obj->funcs->atomic_destroy_state(obj, obj->state);
>> +     drm_modeset_lock_fini(&obj->lock);
>>  }
>>  EXPORT_SYMBOL(drm_atomic_private_obj_fini);
>>
>> @@ -1113,7 +1116,7 @@ struct drm_private_state *
>>  drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
>>                                struct drm_private_obj *obj)
>>  {
>> -     int index, num_objs, i;
>> +     int index, num_objs, i, ret;
>>       size_t size;
>>       struct __drm_private_objs_state *arr;
>>       struct drm_private_state *obj_state;
>> @@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
>>               if (obj == state->private_objs[i].ptr)
>>                       return state->private_objs[i].state;
>>
>> +     ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);
>> +     if (ret)
>> +             return ERR_PTR(ret);
>> +
>>       num_objs = state->num_private_objs + 1;
>>       size = sizeof(*state->private_objs) * num_objs;
>>       arr = krealloc(state->private_objs, size, GFP_KERNEL);
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index 09076a625637..9ae53b73c9d2 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
>>   * &drm_modeset_lock is required to duplicate and update this object's state.
>>   */
>>  struct drm_private_obj {
>> +     /**
>> +      * @lock: Modeset lock to protect the state object.
>> +      */
>> +     struct drm_modeset_lock lock;
>> +
>>       /**
>>        * @state: Current atomic state for this driver private object.
>>        */
>> --
>> 2.14.3
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel OTC

Re: [PATCH 1/4] drm/atomic: integrate modeset lock with private objects

[Ville Syrjälä]

On Wed, Feb 21, 2018 at 09:54:49AM -0500, Rob Clark wrote:
> On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote:
> >> Follow the same pattern of locking as with other state objects.  This
> >> avoids boilerplate in the driver.
> >
> > I'm not sure we really want to do this. What if the driver wants a
> > custom locking scheme for this state?
> 
> That seems like something we want to discourage, ie. all the more
> reason for this patch.
> 
> There is no reason drivers could not split their global state into
> multiple private objs's, each with their own lock, for more fine
> grained locking.  That is basically the only valid reason I can think
> of for "custom locking".

In i915 we have at least one case that would want something close to an
rwlock. Any crtc lock is enough for read, need all of them for write.
Though if we wanted to use private objs for that we might need to
actually make the states refcounted as well, otherwise I can imagine
we might land in some use-after-free issues once again.

Maybe we could duplicate the state into per-crtc and global copies, but
then we have to keep all of those in sync somehow which doesn't sound
particularly pleasant.

> 
> (And ofc drivers could add there own locks in addition to what is done
> by core, but I'd rather look at that on a case by case basis, rather
> than it being part of the boilerplate in each driver.)
> 
> BR,
> -R
> 
> >>
> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
> >> ---
> >>  drivers/gpu/drm/drm_atomic.c | 9 ++++++++-
> >>  include/drm/drm_atomic.h     | 5 +++++
> >>  2 files changed, 13 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> index fc8c4da409ff..004e621ab307 100644
> >> --- a/drivers/gpu/drm/drm_atomic.c
> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >> @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj,
> >>  {
> >>       memset(obj, 0, sizeof(*obj));
> >>
> >> +     drm_modeset_lock_init(&obj->lock);
> >> +
> >>       obj->state = state;
> >>       obj->funcs = funcs;
> >>  }
> >> @@ -1093,6 +1095,7 @@ void
> >>  drm_atomic_private_obj_fini(struct drm_private_obj *obj)
> >>  {
> >>       obj->funcs->atomic_destroy_state(obj, obj->state);
> >> +     drm_modeset_lock_fini(&obj->lock);
> >>  }
> >>  EXPORT_SYMBOL(drm_atomic_private_obj_fini);
> >>
> >> @@ -1113,7 +1116,7 @@ struct drm_private_state *
> >>  drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
> >>                                struct drm_private_obj *obj)
> >>  {
> >> -     int index, num_objs, i;
> >> +     int index, num_objs, i, ret;
> >>       size_t size;
> >>       struct __drm_private_objs_state *arr;
> >>       struct drm_private_state *obj_state;
> >> @@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
> >>               if (obj == state->private_objs[i].ptr)
> >>                       return state->private_objs[i].state;
> >>
> >> +     ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);
> >> +     if (ret)
> >> +             return ERR_PTR(ret);
> >> +
> >>       num_objs = state->num_private_objs + 1;
> >>       size = sizeof(*state->private_objs) * num_objs;
> >>       arr = krealloc(state->private_objs, size, GFP_KERNEL);
> >> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> >> index 09076a625637..9ae53b73c9d2 100644
> >> --- a/include/drm/drm_atomic.h
> >> +++ b/include/drm/drm_atomic.h
> >> @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
> >>   * &drm_modeset_lock is required to duplicate and update this object's state.
> >>   */
> >>  struct drm_private_obj {
> >> +     /**
> >> +      * @lock: Modeset lock to protect the state object.
> >> +      */
> >> +     struct drm_modeset_lock lock;
> >> +
> >>       /**
> >>        * @state: Current atomic state for this driver private object.
> >>        */
> >> --
> >> 2.14.3
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Ville Syrjälä
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/4] drm/atomic: integrate modeset lock with private objects

[Maarten Lankhorst]

Hey,

Op 21-02-18 om 15:37 schreef Rob Clark:
> Follow the same pattern of locking as with other state objects.  This
> avoids boilerplate in the driver.
I'm afraid this will prohibit any uses of this on i915, since it still uses legacy lock_all().

Oh well, afaict nothing in i915 uses private objects, so I don't think it's harmful. :)

Could you cc intel-gfx just in case?
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/gpu/drm/drm_atomic.c | 9 ++++++++-
>  include/drm/drm_atomic.h     | 5 +++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index fc8c4da409ff..004e621ab307 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj,
>  {
>  	memset(obj, 0, sizeof(*obj));
>  
> +	drm_modeset_lock_init(&obj->lock);
> +
>  	obj->state = state;
>  	obj->funcs = funcs;
>  }
> @@ -1093,6 +1095,7 @@ void
>  drm_atomic_private_obj_fini(struct drm_private_obj *obj)
>  {
>  	obj->funcs->atomic_destroy_state(obj, obj->state);
> +	drm_modeset_lock_fini(&obj->lock);
>  }
>  EXPORT_SYMBOL(drm_atomic_private_obj_fini);
>  
> @@ -1113,7 +1116,7 @@ struct drm_private_state *
>  drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
>  				 struct drm_private_obj *obj)
>  {
> -	int index, num_objs, i;
> +	int index, num_objs, i, ret;
>  	size_t size;
>  	struct __drm_private_objs_state *arr;
>  	struct drm_private_state *obj_state;
> @@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
>  		if (obj == state->private_objs[i].ptr)
>  			return state->private_objs[i].state;
>  
> +	ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>  	num_objs = state->num_private_objs + 1;
>  	size = sizeof(*state->private_objs) * num_objs;
>  	arr = krealloc(state->private_objs, size, GFP_KERNEL);
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 09076a625637..9ae53b73c9d2 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
>   * &drm_modeset_lock is required to duplicate and update this object's state.
>   */
>  struct drm_private_obj {
> +	/**
> +	 * @lock: Modeset lock to protect the state object.
> +	 */
> +	struct drm_modeset_lock lock;
> +
>  	/**
>  	 * @state: Current atomic state for this driver private object.
>  	 */

Re: [PATCH 1/4] drm/atomic: integrate modeset lock with private objects

[Rob Clark]

On Wed, Feb 21, 2018 at 10:07 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Wed, Feb 21, 2018 at 09:54:49AM -0500, Rob Clark wrote:
>> On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>> > On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote:
>> >> Follow the same pattern of locking as with other state objects.  This
>> >> avoids boilerplate in the driver.
>> >
>> > I'm not sure we really want to do this. What if the driver wants a
>> > custom locking scheme for this state?
>>
>> That seems like something we want to discourage, ie. all the more
>> reason for this patch.
>>
>> There is no reason drivers could not split their global state into
>> multiple private objs's, each with their own lock, for more fine
>> grained locking.  That is basically the only valid reason I can think
>> of for "custom locking".
>
> In i915 we have at least one case that would want something close to an
> rwlock. Any crtc lock is enough for read, need all of them for write.
> Though if we wanted to use private objs for that we might need to
> actually make the states refcounted as well, otherwise I can imagine
> we might land in some use-after-free issues once again.
>
> Maybe we could duplicate the state into per-crtc and global copies, but
> then we have to keep all of those in sync somehow which doesn't sound
> particularly pleasant.

Or just keep your own driver lock for read, and use that plus the core
modeset lock for write?

BR,
-R

>
>>
>> (And ofc drivers could add there own locks in addition to what is done
>> by core, but I'd rather look at that on a case by case basis, rather
>> than it being part of the boilerplate in each driver.)
>>
>> BR,
>> -R
>>
>> >>
>> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> >> ---
>> >>  drivers/gpu/drm/drm_atomic.c | 9 ++++++++-
>> >>  include/drm/drm_atomic.h     | 5 +++++
>> >>  2 files changed, 13 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> >> index fc8c4da409ff..004e621ab307 100644
>> >> --- a/drivers/gpu/drm/drm_atomic.c
>> >> +++ b/drivers/gpu/drm/drm_atomic.c
>> >> @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj,
>> >>  {
>> >>       memset(obj, 0, sizeof(*obj));
>> >>
>> >> +     drm_modeset_lock_init(&obj->lock);
>> >> +
>> >>       obj->state = state;
>> >>       obj->funcs = funcs;
>> >>  }
>> >> @@ -1093,6 +1095,7 @@ void
>> >>  drm_atomic_private_obj_fini(struct drm_private_obj *obj)
>> >>  {
>> >>       obj->funcs->atomic_destroy_state(obj, obj->state);
>> >> +     drm_modeset_lock_fini(&obj->lock);
>> >>  }
>> >>  EXPORT_SYMBOL(drm_atomic_private_obj_fini);
>> >>
>> >> @@ -1113,7 +1116,7 @@ struct drm_private_state *
>> >>  drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
>> >>                                struct drm_private_obj *obj)
>> >>  {
>> >> -     int index, num_objs, i;
>> >> +     int index, num_objs, i, ret;
>> >>       size_t size;
>> >>       struct __drm_private_objs_state *arr;
>> >>       struct drm_private_state *obj_state;
>> >> @@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
>> >>               if (obj == state->private_objs[i].ptr)
>> >>                       return state->private_objs[i].state;
>> >>
>> >> +     ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);
>> >> +     if (ret)
>> >> +             return ERR_PTR(ret);
>> >> +
>> >>       num_objs = state->num_private_objs + 1;
>> >>       size = sizeof(*state->private_objs) * num_objs;
>> >>       arr = krealloc(state->private_objs, size, GFP_KERNEL);
>> >> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> >> index 09076a625637..9ae53b73c9d2 100644
>> >> --- a/include/drm/drm_atomic.h
>> >> +++ b/include/drm/drm_atomic.h
>> >> @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
>> >>   * &drm_modeset_lock is required to duplicate and update this object's state.
>> >>   */
>> >>  struct drm_private_obj {
>> >> +     /**
>> >> +      * @lock: Modeset lock to protect the state object.
>> >> +      */
>> >> +     struct drm_modeset_lock lock;
>> >> +
>> >>       /**
>> >>        * @state: Current atomic state for this driver private object.
>> >>        */
>> >> --
>> >> 2.14.3
>> >>
>> >> _______________________________________________
>> >> dri-devel mailing list
>> >> dri-devel@lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> >
>> > --
>> > Ville Syrjälä
>> > Intel OTC
>
> --
> Ville Syrjälä
> Intel OTC

Re: [PATCH 1/4] drm/atomic: integrate modeset lock with private objects

[Ville Syrjälä]

On Wed, Feb 21, 2018 at 10:20:03AM -0500, Rob Clark wrote:
> On Wed, Feb 21, 2018 at 10:07 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Wed, Feb 21, 2018 at 09:54:49AM -0500, Rob Clark wrote:
> >> On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä
> >> <ville.syrjala@linux.intel.com> wrote:
> >> > On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote:
> >> >> Follow the same pattern of locking as with other state objects.  This
> >> >> avoids boilerplate in the driver.
> >> >
> >> > I'm not sure we really want to do this. What if the driver wants a
> >> > custom locking scheme for this state?
> >>
> >> That seems like something we want to discourage, ie. all the more
> >> reason for this patch.
> >>
> >> There is no reason drivers could not split their global state into
> >> multiple private objs's, each with their own lock, for more fine
> >> grained locking.  That is basically the only valid reason I can think
> >> of for "custom locking".
> >
> > In i915 we have at least one case that would want something close to an
> > rwlock. Any crtc lock is enough for read, need all of them for write.
> > Though if we wanted to use private objs for that we might need to
> > actually make the states refcounted as well, otherwise I can imagine
> > we might land in some use-after-free issues once again.
> >
> > Maybe we could duplicate the state into per-crtc and global copies, but
> > then we have to keep all of those in sync somehow which doesn't sound
> > particularly pleasant.
> 
> Or just keep your own driver lock for read, and use that plus the core
> modeset lock for write?

If we can't add the private obj to the state we can't really use it.

> 
> BR,
> -R
> 
> >
> >>
> >> (And ofc drivers could add there own locks in addition to what is done
> >> by core, but I'd rather look at that on a case by case basis, rather
> >> than it being part of the boilerplate in each driver.)
> >>
> >> BR,
> >> -R
> >>
> >> >>
> >> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
> >> >> ---
> >> >>  drivers/gpu/drm/drm_atomic.c | 9 ++++++++-
> >> >>  include/drm/drm_atomic.h     | 5 +++++
> >> >>  2 files changed, 13 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> >> index fc8c4da409ff..004e621ab307 100644
> >> >> --- a/drivers/gpu/drm/drm_atomic.c
> >> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >> >> @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj,
> >> >>  {
> >> >>       memset(obj, 0, sizeof(*obj));
> >> >>
> >> >> +     drm_modeset_lock_init(&obj->lock);
> >> >> +
> >> >>       obj->state = state;
> >> >>       obj->funcs = funcs;
> >> >>  }
> >> >> @@ -1093,6 +1095,7 @@ void
> >> >>  drm_atomic_private_obj_fini(struct drm_private_obj *obj)
> >> >>  {
> >> >>       obj->funcs->atomic_destroy_state(obj, obj->state);
> >> >> +     drm_modeset_lock_fini(&obj->lock);
> >> >>  }
> >> >>  EXPORT_SYMBOL(drm_atomic_private_obj_fini);
> >> >>
> >> >> @@ -1113,7 +1116,7 @@ struct drm_private_state *
> >> >>  drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
> >> >>                                struct drm_private_obj *obj)
> >> >>  {
> >> >> -     int index, num_objs, i;
> >> >> +     int index, num_objs, i, ret;
> >> >>       size_t size;
> >> >>       struct __drm_private_objs_state *arr;
> >> >>       struct drm_private_state *obj_state;
> >> >> @@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
> >> >>               if (obj == state->private_objs[i].ptr)
> >> >>                       return state->private_objs[i].state;
> >> >>
> >> >> +     ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);
> >> >> +     if (ret)
> >> >> +             return ERR_PTR(ret);
> >> >> +
> >> >>       num_objs = state->num_private_objs + 1;
> >> >>       size = sizeof(*state->private_objs) * num_objs;
> >> >>       arr = krealloc(state->private_objs, size, GFP_KERNEL);
> >> >> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> >> >> index 09076a625637..9ae53b73c9d2 100644
> >> >> --- a/include/drm/drm_atomic.h
> >> >> +++ b/include/drm/drm_atomic.h
> >> >> @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
> >> >>   * &drm_modeset_lock is required to duplicate and update this object's state.
> >> >>   */
> >> >>  struct drm_private_obj {
> >> >> +     /**
> >> >> +      * @lock: Modeset lock to protect the state object.
> >> >> +      */
> >> >> +     struct drm_modeset_lock lock;
> >> >> +
> >> >>       /**
> >> >>        * @state: Current atomic state for this driver private object.
> >> >>        */
> >> >> --
> >> >> 2.14.3
> >> >>
> >> >> _______________________________________________
> >> >> dri-devel mailing list
> >> >> dri-devel@lists.freedesktop.org
> >> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >> >
> >> > --
> >> > Ville Syrjälä
> >> > Intel OTC
> >
> > --
> > Ville Syrjälä
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/4] drm/atomic: integrate modeset lock with private objects

[Rob Clark]

On Wed, Feb 21, 2018 at 10:27 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Wed, Feb 21, 2018 at 10:20:03AM -0500, Rob Clark wrote:
>> On Wed, Feb 21, 2018 at 10:07 AM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>> > On Wed, Feb 21, 2018 at 09:54:49AM -0500, Rob Clark wrote:
>> >> On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä
>> >> <ville.syrjala@linux.intel.com> wrote:
>> >> > On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote:
>> >> >> Follow the same pattern of locking as with other state objects.  This
>> >> >> avoids boilerplate in the driver.
>> >> >
>> >> > I'm not sure we really want to do this. What if the driver wants a
>> >> > custom locking scheme for this state?
>> >>
>> >> That seems like something we want to discourage, ie. all the more
>> >> reason for this patch.
>> >>
>> >> There is no reason drivers could not split their global state into
>> >> multiple private objs's, each with their own lock, for more fine
>> >> grained locking.  That is basically the only valid reason I can think
>> >> of for "custom locking".
>> >
>> > In i915 we have at least one case that would want something close to an
>> > rwlock. Any crtc lock is enough for read, need all of them for write.
>> > Though if we wanted to use private objs for that we might need to
>> > actually make the states refcounted as well, otherwise I can imagine
>> > we might land in some use-after-free issues once again.
>> >
>> > Maybe we could duplicate the state into per-crtc and global copies, but
>> > then we have to keep all of those in sync somehow which doesn't sound
>> > particularly pleasant.
>>
>> Or just keep your own driver lock for read, and use that plus the core
>> modeset lock for write?
>
> If we can't add the private obj to the state we can't really use it.
>

I'm not sure why that is strictly true (that you need to add it to the
state if for read-only), since you'd be guarding it with your own
driver read-lock you can just priv->foo_state->bar.

Since it is read-only access, there is no roll-back to worry about for
test-only or failed atomic_check()s..

BR,
-R


>>
>> BR,
>> -R
>>
>> >
>> >>
>> >> (And ofc drivers could add there own locks in addition to what is done
>> >> by core, but I'd rather look at that on a case by case basis, rather
>> >> than it being part of the boilerplate in each driver.)
>> >>
>> >> BR,
>> >> -R
>> >>
>> >> >>
>> >> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> >> >> ---
>> >> >>  drivers/gpu/drm/drm_atomic.c | 9 ++++++++-
>> >> >>  include/drm/drm_atomic.h     | 5 +++++
>> >> >>  2 files changed, 13 insertions(+), 1 deletion(-)
>> >> >>
>> >> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> >> >> index fc8c4da409ff..004e621ab307 100644
>> >> >> --- a/drivers/gpu/drm/drm_atomic.c
>> >> >> +++ b/drivers/gpu/drm/drm_atomic.c
>> >> >> @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj,
>> >> >>  {
>> >> >>       memset(obj, 0, sizeof(*obj));
>> >> >>
>> >> >> +     drm_modeset_lock_init(&obj->lock);
>> >> >> +
>> >> >>       obj->state = state;
>> >> >>       obj->funcs = funcs;
>> >> >>  }
>> >> >> @@ -1093,6 +1095,7 @@ void
>> >> >>  drm_atomic_private_obj_fini(struct drm_private_obj *obj)
>> >> >>  {
>> >> >>       obj->funcs->atomic_destroy_state(obj, obj->state);
>> >> >> +     drm_modeset_lock_fini(&obj->lock);
>> >> >>  }
>> >> >>  EXPORT_SYMBOL(drm_atomic_private_obj_fini);
>> >> >>
>> >> >> @@ -1113,7 +1116,7 @@ struct drm_private_state *
>> >> >>  drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
>> >> >>                                struct drm_private_obj *obj)
>> >> >>  {
>> >> >> -     int index, num_objs, i;
>> >> >> +     int index, num_objs, i, ret;
>> >> >>       size_t size;
>> >> >>       struct __drm_private_objs_state *arr;
>> >> >>       struct drm_private_state *obj_state;
>> >> >> @@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
>> >> >>               if (obj == state->private_objs[i].ptr)
>> >> >>                       return state->private_objs[i].state;
>> >> >>
>> >> >> +     ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);
>> >> >> +     if (ret)
>> >> >> +             return ERR_PTR(ret);
>> >> >> +
>> >> >>       num_objs = state->num_private_objs + 1;
>> >> >>       size = sizeof(*state->private_objs) * num_objs;
>> >> >>       arr = krealloc(state->private_objs, size, GFP_KERNEL);
>> >> >> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> >> >> index 09076a625637..9ae53b73c9d2 100644
>> >> >> --- a/include/drm/drm_atomic.h
>> >> >> +++ b/include/drm/drm_atomic.h
>> >> >> @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
>> >> >>   * &drm_modeset_lock is required to duplicate and update this object's state.
>> >> >>   */
>> >> >>  struct drm_private_obj {
>> >> >> +     /**
>> >> >> +      * @lock: Modeset lock to protect the state object.
>> >> >> +      */
>> >> >> +     struct drm_modeset_lock lock;
>> >> >> +
>> >> >>       /**
>> >> >>        * @state: Current atomic state for this driver private object.
>> >> >>        */
>> >> >> --
>> >> >> 2.14.3
>> >> >>
>> >> >> _______________________________________________
>> >> >> dri-devel mailing list
>> >> >> dri-devel@lists.freedesktop.org
>> >> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> >> >
>> >> > --
>> >> > Ville Syrjälä
>> >> > Intel OTC
>> >
>> > --
>> > Ville Syrjälä
>> > Intel OTC
>
> --
> Ville Syrjälä
> Intel OTC

Re: [PATCH 1/4] drm/atomic: integrate modeset lock with private objects

[Ville Syrjälä]

On Wed, Feb 21, 2018 at 10:36:06AM -0500, Rob Clark wrote:
> On Wed, Feb 21, 2018 at 10:27 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Wed, Feb 21, 2018 at 10:20:03AM -0500, Rob Clark wrote:
> >> On Wed, Feb 21, 2018 at 10:07 AM, Ville Syrjälä
> >> <ville.syrjala@linux.intel.com> wrote:
> >> > On Wed, Feb 21, 2018 at 09:54:49AM -0500, Rob Clark wrote:
> >> >> On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä
> >> >> <ville.syrjala@linux.intel.com> wrote:
> >> >> > On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote:
> >> >> >> Follow the same pattern of locking as with other state objects.  This
> >> >> >> avoids boilerplate in the driver.
> >> >> >
> >> >> > I'm not sure we really want to do this. What if the driver wants a
> >> >> > custom locking scheme for this state?
> >> >>
> >> >> That seems like something we want to discourage, ie. all the more
> >> >> reason for this patch.
> >> >>
> >> >> There is no reason drivers could not split their global state into
> >> >> multiple private objs's, each with their own lock, for more fine
> >> >> grained locking.  That is basically the only valid reason I can think
> >> >> of for "custom locking".
> >> >
> >> > In i915 we have at least one case that would want something close to an
> >> > rwlock. Any crtc lock is enough for read, need all of them for write.
> >> > Though if we wanted to use private objs for that we might need to
> >> > actually make the states refcounted as well, otherwise I can imagine
> >> > we might land in some use-after-free issues once again.
> >> >
> >> > Maybe we could duplicate the state into per-crtc and global copies, but
> >> > then we have to keep all of those in sync somehow which doesn't sound
> >> > particularly pleasant.
> >>
> >> Or just keep your own driver lock for read, and use that plus the core
> >> modeset lock for write?
> >
> > If we can't add the private obj to the state we can't really use it.
> >
> 
> I'm not sure why that is strictly true (that you need to add it to the
> state if for read-only), since you'd be guarding it with your own
> driver read-lock you can just priv->foo_state->bar.
> 
> Since it is read-only access, there is no roll-back to worry about for
> test-only or failed atomic_check()s..

That would be super ugly. We want to access the information the same
way whether it has been modified or not.

> 
> BR,
> -R
> 
> 
> >>
> >> BR,
> >> -R
> >>
> >> >
> >> >>
> >> >> (And ofc drivers could add there own locks in addition to what is done
> >> >> by core, but I'd rather look at that on a case by case basis, rather
> >> >> than it being part of the boilerplate in each driver.)
> >> >>
> >> >> BR,
> >> >> -R
> >> >>
> >> >> >>
> >> >> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
> >> >> >> ---
> >> >> >>  drivers/gpu/drm/drm_atomic.c | 9 ++++++++-
> >> >> >>  include/drm/drm_atomic.h     | 5 +++++
> >> >> >>  2 files changed, 13 insertions(+), 1 deletion(-)
> >> >> >>
> >> >> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> >> >> index fc8c4da409ff..004e621ab307 100644
> >> >> >> --- a/drivers/gpu/drm/drm_atomic.c
> >> >> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >> >> >> @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj,
> >> >> >>  {
> >> >> >>       memset(obj, 0, sizeof(*obj));
> >> >> >>
> >> >> >> +     drm_modeset_lock_init(&obj->lock);
> >> >> >> +
> >> >> >>       obj->state = state;
> >> >> >>       obj->funcs = funcs;
> >> >> >>  }
> >> >> >> @@ -1093,6 +1095,7 @@ void
> >> >> >>  drm_atomic_private_obj_fini(struct drm_private_obj *obj)
> >> >> >>  {
> >> >> >>       obj->funcs->atomic_destroy_state(obj, obj->state);
> >> >> >> +     drm_modeset_lock_fini(&obj->lock);
> >> >> >>  }
> >> >> >>  EXPORT_SYMBOL(drm_atomic_private_obj_fini);
> >> >> >>
> >> >> >> @@ -1113,7 +1116,7 @@ struct drm_private_state *
> >> >> >>  drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
> >> >> >>                                struct drm_private_obj *obj)
> >> >> >>  {
> >> >> >> -     int index, num_objs, i;
> >> >> >> +     int index, num_objs, i, ret;
> >> >> >>       size_t size;
> >> >> >>       struct __drm_private_objs_state *arr;
> >> >> >>       struct drm_private_state *obj_state;
> >> >> >> @@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
> >> >> >>               if (obj == state->private_objs[i].ptr)
> >> >> >>                       return state->private_objs[i].state;
> >> >> >>
> >> >> >> +     ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);
> >> >> >> +     if (ret)
> >> >> >> +             return ERR_PTR(ret);
> >> >> >> +
> >> >> >>       num_objs = state->num_private_objs + 1;
> >> >> >>       size = sizeof(*state->private_objs) * num_objs;
> >> >> >>       arr = krealloc(state->private_objs, size, GFP_KERNEL);
> >> >> >> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> >> >> >> index 09076a625637..9ae53b73c9d2 100644
> >> >> >> --- a/include/drm/drm_atomic.h
> >> >> >> +++ b/include/drm/drm_atomic.h
> >> >> >> @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
> >> >> >>   * &drm_modeset_lock is required to duplicate and update this object's state.
> >> >> >>   */
> >> >> >>  struct drm_private_obj {
> >> >> >> +     /**
> >> >> >> +      * @lock: Modeset lock to protect the state object.
> >> >> >> +      */
> >> >> >> +     struct drm_modeset_lock lock;
> >> >> >> +
> >> >> >>       /**
> >> >> >>        * @state: Current atomic state for this driver private object.
> >> >> >>        */
> >> >> >> --
> >> >> >> 2.14.3
> >> >> >>
> >> >> >> _______________________________________________
> >> >> >> dri-devel mailing list
> >> >> >> dri-devel@lists.freedesktop.org
> >> >> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >> >> >
> >> >> > --
> >> >> > Ville Syrjälä
> >> >> > Intel OTC
> >> >
> >> > --
> >> > Ville Syrjälä
> >> > Intel OTC
> >
> > --
> > Ville Syrjälä
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/4] drm/atomic: integrate modeset lock with private objects

[Rob Clark]

On Wed, Feb 21, 2018 at 10:54 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Wed, Feb 21, 2018 at 10:36:06AM -0500, Rob Clark wrote:
>> On Wed, Feb 21, 2018 at 10:27 AM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>> > On Wed, Feb 21, 2018 at 10:20:03AM -0500, Rob Clark wrote:
>> >> On Wed, Feb 21, 2018 at 10:07 AM, Ville Syrjälä
>> >> <ville.syrjala@linux.intel.com> wrote:
>> >> > On Wed, Feb 21, 2018 at 09:54:49AM -0500, Rob Clark wrote:
>> >> >> On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä
>> >> >> <ville.syrjala@linux.intel.com> wrote:
>> >> >> > On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote:
>> >> >> >> Follow the same pattern of locking as with other state objects.  This
>> >> >> >> avoids boilerplate in the driver.
>> >> >> >
>> >> >> > I'm not sure we really want to do this. What if the driver wants a
>> >> >> > custom locking scheme for this state?
>> >> >>
>> >> >> That seems like something we want to discourage, ie. all the more
>> >> >> reason for this patch.
>> >> >>
>> >> >> There is no reason drivers could not split their global state into
>> >> >> multiple private objs's, each with their own lock, for more fine
>> >> >> grained locking.  That is basically the only valid reason I can think
>> >> >> of for "custom locking".
>> >> >
>> >> > In i915 we have at least one case that would want something close to an
>> >> > rwlock. Any crtc lock is enough for read, need all of them for write.
>> >> > Though if we wanted to use private objs for that we might need to
>> >> > actually make the states refcounted as well, otherwise I can imagine
>> >> > we might land in some use-after-free issues once again.
>> >> >
>> >> > Maybe we could duplicate the state into per-crtc and global copies, but
>> >> > then we have to keep all of those in sync somehow which doesn't sound
>> >> > particularly pleasant.
>> >>
>> >> Or just keep your own driver lock for read, and use that plus the core
>> >> modeset lock for write?
>> >
>> > If we can't add the private obj to the state we can't really use it.
>> >
>>
>> I'm not sure why that is strictly true (that you need to add it to the
>> state if for read-only), since you'd be guarding it with your own
>> driver read-lock you can just priv->foo_state->bar.
>>
>> Since it is read-only access, there is no roll-back to worry about for
>> test-only or failed atomic_check()s..
>
> That would be super ugly. We want to access the information the same
> way whether it has been modified or not.

Well, I mean the whole idea of what you want to do seems a bit super-ugly ;-)

I mean, in mdp5 the assigned global resources go in plane/crtc state,
and tracking of what is assigned to which plane/crtc is in global
state, so it fits nicely in the current locking model.  For i915, I'm
not quite sure what is the global state you are concerned about, so it
is a bit hard to talk about the best solution in the abstract.  Maybe
the better option is to teach modeset-lock how to be a rwlock instead?

BR,
-R

>>
>> BR,
>> -R
>>
>>
>> >>
>> >> BR,
>> >> -R
>> >>
>> >> >
>> >> >>
>> >> >> (And ofc drivers could add there own locks in addition to what is done
>> >> >> by core, but I'd rather look at that on a case by case basis, rather
>> >> >> than it being part of the boilerplate in each driver.)
>> >> >>
>> >> >> BR,
>> >> >> -R
>> >> >>
>> >> >> >>
>> >> >> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> >> >> >> ---
>> >> >> >>  drivers/gpu/drm/drm_atomic.c | 9 ++++++++-
>> >> >> >>  include/drm/drm_atomic.h     | 5 +++++
>> >> >> >>  2 files changed, 13 insertions(+), 1 deletion(-)
>> >> >> >>
>> >> >> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> >> >> >> index fc8c4da409ff..004e621ab307 100644
>> >> >> >> --- a/drivers/gpu/drm/drm_atomic.c
>> >> >> >> +++ b/drivers/gpu/drm/drm_atomic.c
>> >> >> >> @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj,
>> >> >> >>  {
>> >> >> >>       memset(obj, 0, sizeof(*obj));
>> >> >> >>
>> >> >> >> +     drm_modeset_lock_init(&obj->lock);
>> >> >> >> +
>> >> >> >>       obj->state = state;
>> >> >> >>       obj->funcs = funcs;
>> >> >> >>  }
>> >> >> >> @@ -1093,6 +1095,7 @@ void
>> >> >> >>  drm_atomic_private_obj_fini(struct drm_private_obj *obj)
>> >> >> >>  {
>> >> >> >>       obj->funcs->atomic_destroy_state(obj, obj->state);
>> >> >> >> +     drm_modeset_lock_fini(&obj->lock);
>> >> >> >>  }
>> >> >> >>  EXPORT_SYMBOL(drm_atomic_private_obj_fini);
>> >> >> >>
>> >> >> >> @@ -1113,7 +1116,7 @@ struct drm_private_state *
>> >> >> >>  drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
>> >> >> >>                                struct drm_private_obj *obj)
>> >> >> >>  {
>> >> >> >> -     int index, num_objs, i;
>> >> >> >> +     int index, num_objs, i, ret;
>> >> >> >>       size_t size;
>> >> >> >>       struct __drm_private_objs_state *arr;
>> >> >> >>       struct drm_private_state *obj_state;
>> >> >> >> @@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
>> >> >> >>               if (obj == state->private_objs[i].ptr)
>> >> >> >>                       return state->private_objs[i].state;
>> >> >> >>
>> >> >> >> +     ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);
>> >> >> >> +     if (ret)
>> >> >> >> +             return ERR_PTR(ret);
>> >> >> >> +
>> >> >> >>       num_objs = state->num_private_objs + 1;
>> >> >> >>       size = sizeof(*state->private_objs) * num_objs;
>> >> >> >>       arr = krealloc(state->private_objs, size, GFP_KERNEL);
>> >> >> >> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> >> >> >> index 09076a625637..9ae53b73c9d2 100644
>> >> >> >> --- a/include/drm/drm_atomic.h
>> >> >> >> +++ b/include/drm/drm_atomic.h
>> >> >> >> @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
>> >> >> >>   * &drm_modeset_lock is required to duplicate and update this object's state.
>> >> >> >>   */
>> >> >> >>  struct drm_private_obj {
>> >> >> >> +     /**
>> >> >> >> +      * @lock: Modeset lock to protect the state object.
>> >> >> >> +      */
>> >> >> >> +     struct drm_modeset_lock lock;
>> >> >> >> +
>> >> >> >>       /**
>> >> >> >>        * @state: Current atomic state for this driver private object.
>> >> >> >>        */
>> >> >> >> --
>> >> >> >> 2.14.3
>> >> >> >>
>> >> >> >> _______________________________________________
>> >> >> >> dri-devel mailing list
>> >> >> >> dri-devel@lists.freedesktop.org
>> >> >> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> >> >> >
>> >> >> > --
>> >> >> > Ville Syrjälä
>> >> >> > Intel OTC
>> >> >
>> >> > --
>> >> > Ville Syrjälä
>> >> > Intel OTC
>> >
>> > --
>> > Ville Syrjälä
>> > Intel OTC
>
> --
> Ville Syrjälä
> Intel OTC

Re: [PATCH 1/4] drm/atomic: integrate modeset lock with private objects

[Ville Syrjälä]

On Wed, Feb 21, 2018 at 11:17:21AM -0500, Rob Clark wrote:
> On Wed, Feb 21, 2018 at 10:54 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Wed, Feb 21, 2018 at 10:36:06AM -0500, Rob Clark wrote:
> >> On Wed, Feb 21, 2018 at 10:27 AM, Ville Syrjälä
> >> <ville.syrjala@linux.intel.com> wrote:
> >> > On Wed, Feb 21, 2018 at 10:20:03AM -0500, Rob Clark wrote:
> >> >> On Wed, Feb 21, 2018 at 10:07 AM, Ville Syrjälä
> >> >> <ville.syrjala@linux.intel.com> wrote:
> >> >> > On Wed, Feb 21, 2018 at 09:54:49AM -0500, Rob Clark wrote:
> >> >> >> On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä
> >> >> >> <ville.syrjala@linux.intel.com> wrote:
> >> >> >> > On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote:
> >> >> >> >> Follow the same pattern of locking as with other state objects.  This
> >> >> >> >> avoids boilerplate in the driver.
> >> >> >> >
> >> >> >> > I'm not sure we really want to do this. What if the driver wants a
> >> >> >> > custom locking scheme for this state?
> >> >> >>
> >> >> >> That seems like something we want to discourage, ie. all the more
> >> >> >> reason for this patch.
> >> >> >>
> >> >> >> There is no reason drivers could not split their global state into
> >> >> >> multiple private objs's, each with their own lock, for more fine
> >> >> >> grained locking.  That is basically the only valid reason I can think
> >> >> >> of for "custom locking".
> >> >> >
> >> >> > In i915 we have at least one case that would want something close to an
> >> >> > rwlock. Any crtc lock is enough for read, need all of them for write.
> >> >> > Though if we wanted to use private objs for that we might need to
> >> >> > actually make the states refcounted as well, otherwise I can imagine
> >> >> > we might land in some use-after-free issues once again.
> >> >> >
> >> >> > Maybe we could duplicate the state into per-crtc and global copies, but
> >> >> > then we have to keep all of those in sync somehow which doesn't sound
> >> >> > particularly pleasant.
> >> >>
> >> >> Or just keep your own driver lock for read, and use that plus the core
> >> >> modeset lock for write?
> >> >
> >> > If we can't add the private obj to the state we can't really use it.
> >> >
> >>
> >> I'm not sure why that is strictly true (that you need to add it to the
> >> state if for read-only), since you'd be guarding it with your own
> >> driver read-lock you can just priv->foo_state->bar.
> >>
> >> Since it is read-only access, there is no roll-back to worry about for
> >> test-only or failed atomic_check()s..
> >
> > That would be super ugly. We want to access the information the same
> > way whether it has been modified or not.
> 
> Well, I mean the whole idea of what you want to do seems a bit super-ugly ;-)
> 
> I mean, in mdp5 the assigned global resources go in plane/crtc state,
> and tracking of what is assigned to which plane/crtc is in global
> state, so it fits nicely in the current locking model.  For i915, I'm
> not quite sure what is the global state you are concerned about, so it
> is a bit hard to talk about the best solution in the abstract.  Maybe
> the better option is to teach modeset-lock how to be a rwlock instead?

The thing I'm thinking is the core display clock (cdclk) frequency which
we need to consult whenever computing plane states and whatnot. We don't
want a modeset on one crtc to block a plane update on another crtc
unless we actually have to bump the cdclk (which would generally require
all crtcs to undergo a full modeset). Seems like a generally useful
pattern to me.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/4] drm/atomic: integrate modeset lock with private objects

[Rob Clark]

On Wed, Feb 21, 2018 at 11:36 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Wed, Feb 21, 2018 at 11:17:21AM -0500, Rob Clark wrote:
>> On Wed, Feb 21, 2018 at 10:54 AM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>> > On Wed, Feb 21, 2018 at 10:36:06AM -0500, Rob Clark wrote:
>> >> On Wed, Feb 21, 2018 at 10:27 AM, Ville Syrjälä
>> >> <ville.syrjala@linux.intel.com> wrote:
>> >> > On Wed, Feb 21, 2018 at 10:20:03AM -0500, Rob Clark wrote:
>> >> >> On Wed, Feb 21, 2018 at 10:07 AM, Ville Syrjälä
>> >> >> <ville.syrjala@linux.intel.com> wrote:
>> >> >> > On Wed, Feb 21, 2018 at 09:54:49AM -0500, Rob Clark wrote:
>> >> >> >> On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä
>> >> >> >> <ville.syrjala@linux.intel.com> wrote:
>> >> >> >> > On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote:
>> >> >> >> >> Follow the same pattern of locking as with other state objects.  This
>> >> >> >> >> avoids boilerplate in the driver.
>> >> >> >> >
>> >> >> >> > I'm not sure we really want to do this. What if the driver wants a
>> >> >> >> > custom locking scheme for this state?
>> >> >> >>
>> >> >> >> That seems like something we want to discourage, ie. all the more
>> >> >> >> reason for this patch.
>> >> >> >>
>> >> >> >> There is no reason drivers could not split their global state into
>> >> >> >> multiple private objs's, each with their own lock, for more fine
>> >> >> >> grained locking.  That is basically the only valid reason I can think
>> >> >> >> of for "custom locking".
>> >> >> >
>> >> >> > In i915 we have at least one case that would want something close to an
>> >> >> > rwlock. Any crtc lock is enough for read, need all of them for write.
>> >> >> > Though if we wanted to use private objs for that we might need to
>> >> >> > actually make the states refcounted as well, otherwise I can imagine
>> >> >> > we might land in some use-after-free issues once again.
>> >> >> >
>> >> >> > Maybe we could duplicate the state into per-crtc and global copies, but
>> >> >> > then we have to keep all of those in sync somehow which doesn't sound
>> >> >> > particularly pleasant.
>> >> >>
>> >> >> Or just keep your own driver lock for read, and use that plus the core
>> >> >> modeset lock for write?
>> >> >
>> >> > If we can't add the private obj to the state we can't really use it.
>> >> >
>> >>
>> >> I'm not sure why that is strictly true (that you need to add it to the
>> >> state if for read-only), since you'd be guarding it with your own
>> >> driver read-lock you can just priv->foo_state->bar.
>> >>
>> >> Since it is read-only access, there is no roll-back to worry about for
>> >> test-only or failed atomic_check()s..
>> >
>> > That would be super ugly. We want to access the information the same
>> > way whether it has been modified or not.
>>
>> Well, I mean the whole idea of what you want to do seems a bit super-ugly ;-)
>>
>> I mean, in mdp5 the assigned global resources go in plane/crtc state,
>> and tracking of what is assigned to which plane/crtc is in global
>> state, so it fits nicely in the current locking model.  For i915, I'm
>> not quite sure what is the global state you are concerned about, so it
>> is a bit hard to talk about the best solution in the abstract.  Maybe
>> the better option is to teach modeset-lock how to be a rwlock instead?
>
> The thing I'm thinking is the core display clock (cdclk) frequency which
> we need to consult whenever computing plane states and whatnot. We don't
> want a modeset on one crtc to block a plane update on another crtc
> unless we actually have to bump the cdclk (which would generally require
> all crtcs to undergo a full modeset). Seems like a generally useful
> pattern to me.
>

Hmm.. I suppose either way, you'd have to make modeset-lock have rw
semantics or invent some way to play nicely with acquire_ctx.. once
you have that, I'd have no objection to make private state use rwlock
semantics.  That seems better than having drivers (badly) roll their
own.  And maybe as you say, it would be a useful pattern for other
drivers.

BR,
-R

Re: [PATCH 1/4] drm/atomic: integrate modeset lock with private objects

[Daniel Vetter]

On Wed, Feb 21, 2018 at 04:19:40PM +0100, Maarten Lankhorst wrote:
> Hey,
> 
> Op 21-02-18 om 15:37 schreef Rob Clark:
> > Follow the same pattern of locking as with other state objects.  This
> > avoids boilerplate in the driver.
> I'm afraid this will prohibit any uses of this on i915, since it still uses legacy lock_all().
> 
> Oh well, afaict nothing in i915 uses private objects, so I don't think it's harmful. :)

We do use private objects, as part of dp mst helpers. But I also thought
that the only users left of lock_all are in the debugfs code, where this
really doesn't matter all that much.

> Could you cc intel-gfx just in case?

Yeah, best to double-check with CI.

> > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > ---
> >  drivers/gpu/drm/drm_atomic.c | 9 ++++++++-
> >  include/drm/drm_atomic.h     | 5 +++++
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index fc8c4da409ff..004e621ab307 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj,
> >  {
> >  	memset(obj, 0, sizeof(*obj));
> >  
> > +	drm_modeset_lock_init(&obj->lock);
> > +
> >  	obj->state = state;
> >  	obj->funcs = funcs;
> >  }
> > @@ -1093,6 +1095,7 @@ void
> >  drm_atomic_private_obj_fini(struct drm_private_obj *obj)
> >  {
> >  	obj->funcs->atomic_destroy_state(obj, obj->state);
> > +	drm_modeset_lock_fini(&obj->lock);
> >  }
> >  EXPORT_SYMBOL(drm_atomic_private_obj_fini);
> >  
> > @@ -1113,7 +1116,7 @@ struct drm_private_state *
> >  drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
> >  				 struct drm_private_obj *obj)
> >  {
> > -	int index, num_objs, i;
> > +	int index, num_objs, i, ret;
> >  	size_t size;
> >  	struct __drm_private_objs_state *arr;
> >  	struct drm_private_state *obj_state;
> > @@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
> >  		if (obj == state->private_objs[i].ptr)
> >  			return state->private_objs[i].state;
> >  
> > +	ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> >  	num_objs = state->num_private_objs + 1;
> >  	size = sizeof(*state->private_objs) * num_objs;
> >  	arr = krealloc(state->private_objs, size, GFP_KERNEL);
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index 09076a625637..9ae53b73c9d2 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
> >   * &drm_modeset_lock is required to duplicate and update this object's state.
> >   */
> >  struct drm_private_obj {
> > +	/**
> > +	 * @lock: Modeset lock to protect the state object.
> > +	 */
> > +	struct drm_modeset_lock lock;
> > +
> >  	/**
> >  	 * @state: Current atomic state for this driver private object.
> >  	 */
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/4] drm/atomic: integrate modeset lock with private objects

[Daniel Vetter]

On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote:
> Follow the same pattern of locking as with other state objects.  This
> avoids boilerplate in the driver.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

Please also adjust the kernel doc, and I think we can remove the locking
WARN_ON in drm_atomic_get_mst_topology_state after this patch (plus again
adjust the kerneldoc for that please).

Otherwise I think this makes sense, and ecnourages reasonable semantics
for driver private state objects.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c | 9 ++++++++-
>  include/drm/drm_atomic.h     | 5 +++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index fc8c4da409ff..004e621ab307 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj,
>  {
>  	memset(obj, 0, sizeof(*obj));
>  
> +	drm_modeset_lock_init(&obj->lock);
> +
>  	obj->state = state;
>  	obj->funcs = funcs;
>  }
> @@ -1093,6 +1095,7 @@ void
>  drm_atomic_private_obj_fini(struct drm_private_obj *obj)
>  {
>  	obj->funcs->atomic_destroy_state(obj, obj->state);
> +	drm_modeset_lock_fini(&obj->lock);
>  }
>  EXPORT_SYMBOL(drm_atomic_private_obj_fini);
>  
> @@ -1113,7 +1116,7 @@ struct drm_private_state *
>  drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
>  				 struct drm_private_obj *obj)
>  {
> -	int index, num_objs, i;
> +	int index, num_objs, i, ret;
>  	size_t size;
>  	struct __drm_private_objs_state *arr;
>  	struct drm_private_state *obj_state;
> @@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
>  		if (obj == state->private_objs[i].ptr)
>  			return state->private_objs[i].state;
>  
> +	ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>  	num_objs = state->num_private_objs + 1;
>  	size = sizeof(*state->private_objs) * num_objs;
>  	arr = krealloc(state->private_objs, size, GFP_KERNEL);
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 09076a625637..9ae53b73c9d2 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
>   * &drm_modeset_lock is required to duplicate and update this object's state.
>   */
>  struct drm_private_obj {
> +	/**
> +	 * @lock: Modeset lock to protect the state object.
> +	 */
> +	struct drm_modeset_lock lock;
> +
>  	/**
>  	 * @state: Current atomic state for this driver private object.
>  	 */
> -- 
> 2.14.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/4] drm/atomic: integrate modeset lock with private objects

[Daniel Vetter]

On Wed, Feb 21, 2018 at 06:36:54PM +0200, Ville Syrjälä wrote:
> On Wed, Feb 21, 2018 at 11:17:21AM -0500, Rob Clark wrote:
> > On Wed, Feb 21, 2018 at 10:54 AM, Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > > On Wed, Feb 21, 2018 at 10:36:06AM -0500, Rob Clark wrote:
> > >> On Wed, Feb 21, 2018 at 10:27 AM, Ville Syrjälä
> > >> <ville.syrjala@linux.intel.com> wrote:
> > >> > On Wed, Feb 21, 2018 at 10:20:03AM -0500, Rob Clark wrote:
> > >> >> On Wed, Feb 21, 2018 at 10:07 AM, Ville Syrjälä
> > >> >> <ville.syrjala@linux.intel.com> wrote:
> > >> >> > On Wed, Feb 21, 2018 at 09:54:49AM -0500, Rob Clark wrote:
> > >> >> >> On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä
> > >> >> >> <ville.syrjala@linux.intel.com> wrote:
> > >> >> >> > On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote:
> > >> >> >> >> Follow the same pattern of locking as with other state objects.  This
> > >> >> >> >> avoids boilerplate in the driver.
> > >> >> >> >
> > >> >> >> > I'm not sure we really want to do this. What if the driver wants a
> > >> >> >> > custom locking scheme for this state?
> > >> >> >>
> > >> >> >> That seems like something we want to discourage, ie. all the more
> > >> >> >> reason for this patch.
> > >> >> >>
> > >> >> >> There is no reason drivers could not split their global state into
> > >> >> >> multiple private objs's, each with their own lock, for more fine
> > >> >> >> grained locking.  That is basically the only valid reason I can think
> > >> >> >> of for "custom locking".
> > >> >> >
> > >> >> > In i915 we have at least one case that would want something close to an
> > >> >> > rwlock. Any crtc lock is enough for read, need all of them for write.
> > >> >> > Though if we wanted to use private objs for that we might need to
> > >> >> > actually make the states refcounted as well, otherwise I can imagine
> > >> >> > we might land in some use-after-free issues once again.
> > >> >> >
> > >> >> > Maybe we could duplicate the state into per-crtc and global copies, but
> > >> >> > then we have to keep all of those in sync somehow which doesn't sound
> > >> >> > particularly pleasant.
> > >> >>
> > >> >> Or just keep your own driver lock for read, and use that plus the core
> > >> >> modeset lock for write?
> > >> >
> > >> > If we can't add the private obj to the state we can't really use it.
> > >> >
> > >>
> > >> I'm not sure why that is strictly true (that you need to add it to the
> > >> state if for read-only), since you'd be guarding it with your own
> > >> driver read-lock you can just priv->foo_state->bar.
> > >>
> > >> Since it is read-only access, there is no roll-back to worry about for
> > >> test-only or failed atomic_check()s..
> > >
> > > That would be super ugly. We want to access the information the same
> > > way whether it has been modified or not.
> > 
> > Well, I mean the whole idea of what you want to do seems a bit super-ugly ;-)
> > 
> > I mean, in mdp5 the assigned global resources go in plane/crtc state,
> > and tracking of what is assigned to which plane/crtc is in global
> > state, so it fits nicely in the current locking model.  For i915, I'm
> > not quite sure what is the global state you are concerned about, so it
> > is a bit hard to talk about the best solution in the abstract.  Maybe
> > the better option is to teach modeset-lock how to be a rwlock instead?
> 
> The thing I'm thinking is the core display clock (cdclk) frequency which
> we need to consult whenever computing plane states and whatnot. We don't
> want a modeset on one crtc to block a plane update on another crtc
> unless we actually have to bump the cdclk (which would generally require
> all crtcs to undergo a full modeset). Seems like a generally useful
> pattern to me.

The usual way to fix that is to have read-only copies of the state in the
plane or crtc states. And for writing (or if the requirement changes) you
have to lock all the objects. Essentially what Rob's doing for his
plane/crtc assignment stuff.

What we do in i915 is kinda not what I've been recommending to everyone
else, because it is a rather tricky and complicated way to get things
done. Sure there's a tradeoff between duplicating data and complicated
locking schemes, but I think for the kms case having to explicitly type
code that reflects the depencies in computation (instead of having that
embedded implicitly in the locking scheme) is a feature, not a bug.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/4] drm/atomic: integrate modeset lock with private objects

[Daniel Vetter]

On Tue, Mar 06, 2018 at 08:29:20AM +0100, Daniel Vetter wrote:
> On Wed, Feb 21, 2018 at 04:19:40PM +0100, Maarten Lankhorst wrote:
> > Hey,
> > 
> > Op 21-02-18 om 15:37 schreef Rob Clark:
> > > Follow the same pattern of locking as with other state objects.  This
> > > avoids boilerplate in the driver.
> > I'm afraid this will prohibit any uses of this on i915, since it still uses legacy lock_all().
> > 
> > Oh well, afaict nothing in i915 uses private objects, so I don't think it's harmful. :)
> 
> We do use private objects, as part of dp mst helpers. But I also thought
> that the only users left of lock_all are in the debugfs code, where this
> really doesn't matter all that much.

Correction, we use it in other places than debugfs. But thanks to Ville's
private state obj refactoring we now have drm_atomic_private_obj_init(),
so it's easy to add all the private state objects to a new list in
drm_dev->mode_config.private_states or so, and use that list in
drm_modeset_lock_all_ctx to also take driver private locks.

I think that would actually be useful in other places, just in case.
-Daniel

> 
> > Could you cc intel-gfx just in case?
> 
> Yeah, best to double-check with CI.
> 
> > > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > > ---
> > >  drivers/gpu/drm/drm_atomic.c | 9 ++++++++-
> > >  include/drm/drm_atomic.h     | 5 +++++
> > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index fc8c4da409ff..004e621ab307 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj,
> > >  {
> > >  	memset(obj, 0, sizeof(*obj));
> > >  
> > > +	drm_modeset_lock_init(&obj->lock);
> > > +
> > >  	obj->state = state;
> > >  	obj->funcs = funcs;
> > >  }
> > > @@ -1093,6 +1095,7 @@ void
> > >  drm_atomic_private_obj_fini(struct drm_private_obj *obj)
> > >  {
> > >  	obj->funcs->atomic_destroy_state(obj, obj->state);
> > > +	drm_modeset_lock_fini(&obj->lock);
> > >  }
> > >  EXPORT_SYMBOL(drm_atomic_private_obj_fini);
> > >  
> > > @@ -1113,7 +1116,7 @@ struct drm_private_state *
> > >  drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
> > >  				 struct drm_private_obj *obj)
> > >  {
> > > -	int index, num_objs, i;
> > > +	int index, num_objs, i, ret;
> > >  	size_t size;
> > >  	struct __drm_private_objs_state *arr;
> > >  	struct drm_private_state *obj_state;
> > > @@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
> > >  		if (obj == state->private_objs[i].ptr)
> > >  			return state->private_objs[i].state;
> > >  
> > > +	ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);
> > > +	if (ret)
> > > +		return ERR_PTR(ret);
> > > +
> > >  	num_objs = state->num_private_objs + 1;
> > >  	size = sizeof(*state->private_objs) * num_objs;
> > >  	arr = krealloc(state->private_objs, size, GFP_KERNEL);
> > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > > index 09076a625637..9ae53b73c9d2 100644
> > > --- a/include/drm/drm_atomic.h
> > > +++ b/include/drm/drm_atomic.h
> > > @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
> > >   * &drm_modeset_lock is required to duplicate and update this object's state.
> > >   */
> > >  struct drm_private_obj {
> > > +	/**
> > > +	 * @lock: Modeset lock to protect the state object.
> > > +	 */
> > > +	struct drm_modeset_lock lock;
> > > +
> > >  	/**
> > >  	 * @state: Current atomic state for this driver private object.
> > >  	 */
> > 
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch