drm/msm: Avoid subclassing of drm_atomic_state

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 <[email protected]>

---
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 86 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h | 27 +++++++++++
 2 files changed, 113 insertions(+)

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +1 -0

index f7c0698fec40..dfc4d81124d5 100644
@@ -106,6 +106,86 @@ 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 */

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +50 -0

+}
+
+/*
+ * 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;
+	int ret;
+
+	ret = drm_modeset_lock(&mdp5_kms->glob_state_lock, s->acquire_ctx);
+	if (ret)
+		return ERR_PTR(ret);
+
+	priv_state = drm_atomic_get_private_obj_state(s, &mdp5_kms->glob_base);
+	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,
+};

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +15 -0

+
+	state->mdp5_kms = mdp5_kms;
+
+	drm_atomic_private_obj_init(&mdp5_kms->glob_base,
+				    &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));

@@ -727,6 +807,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_base);
+
 	kfree(mdp5_kms->state);
 }
 

@@ -887,6 +969,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);

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h +27 -0

index 9b3fe01089d1..522ddb835593 100644
@@ -55,6 +55,13 @@ 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_base;
+	struct drm_modeset_lock glob_state_lock;
+
 	struct mdp5_smp *smp;
 	struct mdp5_ctl_manager *ctlm;
 

@@ -95,6 +102,26 @@ 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.
  */

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Quoted 29 lines · Archit Taneja

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 <[email protected]>

---
  drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 86 +++++++++++++++++++++++++++++++++
  drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h | 27 +++++++++++
  2 files changed, 113 insertions(+)

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +1 -0

index f7c0698fec40..dfc4d81124d5 100644
@@ -106,6 +106,86 @@ 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 */

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c static void mdp5_swap_state(struct msm_kms *kms, struct drm_atomic_state *state) · line 119

Quoted 10 lines

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +10 -0

+
+/*
+ * 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_base.state);

This doesn't match the existing state stuff for everything else. Here you look at the global state, but not at the one hanging off drm_atomic_state.

Maybe we should add a drm_atomic_get_existing_private_obj_state function to make this easier?

I'm also not 100% sure on what semantics you want precisely.

Quoted 59 lines

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +57 -0

+}
+
+/*
+ * 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;
+	int ret;
+
+	ret = drm_modeset_lock(&mdp5_kms->glob_state_lock, s->acquire_ctx);
+	if (ret)
+		return ERR_PTR(ret);
+
+	priv_state = drm_atomic_get_private_obj_state(s, &mdp5_kms->glob_base);
+	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)

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c static void mdp5_swap_state(struct msm_kms *kms, struct drm_atomic_state *state) · line 179

Quoted 3 lines

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +3 -0

+		return -ENOMEM;
+
+	drm_modeset_lock_init(&mdp5_kms->glob_state_lock);

I thought a bit the last few days about how to integrate modeset locking into driver private state objects. I think the simplest solution would be if we just add a drm_modeset_lock to drm_private_obj, and push the locking (both here and in the mst helper) into the core private obj code.

Per-object locking might be a bit overkill (it's definitely overkill for mst), but it's also easier to avoid special cases.

That would reduce the boilerplate here a bit more, essentially converting the wrappers into type casting functions.
-Daniel

Quoted 81 lines

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +15 -0

+
+	state->mdp5_kms = mdp5_kms;
+
+	drm_atomic_private_obj_init(&mdp5_kms->glob_base,
+				    &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));

@@ -727,6 +807,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_base);
+
  	kfree(mdp5_kms->state);
  }
 

@@ -887,6 +969,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);

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h +27 -0

index 9b3fe01089d1..522ddb835593 100644
@@ -55,6 +55,13 @@ 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_base;
+	struct drm_modeset_lock glob_state_lock;
+
  	struct mdp5_smp *smp;
  	struct mdp5_ctl_manager *ctlm;
 

@@ -95,6 +102,26 @@ 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.
   */

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Quoted 51 lines · Daniel Vetter

Quoted 40 lines · Archit Taneja

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 <[email protected]>

---
  drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 86
+++++++++++++++++++++++++++++++++
  drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h | 27 +++++++++++
  2 files changed, 113 insertions(+)

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +3 -0

index f7c0698fec40..dfc4d81124d5 100644
@@ -106,6 +106,86 @@ 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.
+ */

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c static void mdp5_swap_state(struct msm_kms *kms, struct drm_atomic_state *state) · line 119

Quoted 4 lines

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +4 -0

+struct mdp5_global_state *
+mdp5_get_existing_global_state(struct mdp5_kms *mdp5_kms)
+{
+       return to_mdp5_global_state(mdp5_kms->glob_base.state);

This doesn't match the existing state stuff for everything else. Here you look at the global state, but not at the one hanging off drm_atomic_state.

Quoted 4 lines

Maybe we should add a drm_atomic_get_existing_private_obj_state function to make this easier?

I'm also not 100% sure on what semantics you want precisely.

I just wanted a func that returns me the private obj's state that is currently
"in use". I.e, the old state pre-swap, and the new state post-swap. If I use drm_atomic_get_private_obj_state() after the swap has occurred, I get the old
state back. In other words, there aren't funcs for private objs like drm_atomic_get_new_crtc_state() and drm_atomic_get_old_crtc_state() that we can use. I'll go through the code again carefully to see if my understanding isn't screwed up.

Quoted 65 lines

Quoted 65 lines

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +7 -0

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

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +7 -0

+{
+       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;
+       int ret;
+
+       ret = drm_modeset_lock(&mdp5_kms->glob_state_lock,

s->acquire_ctx);

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +4 -0

+       if (ret)
+               return ERR_PTR(ret);
+
+       priv_state = drm_atomic_get_private_obj_state(s,

&mdp5_kms->glob_base);

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +16 -0

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

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +8 -0

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

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +16 -0

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

Quoted 8 lines

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c

Quoted 2 lines

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +2 -0

+
+       drm_modeset_lock_init(&mdp5_kms->glob_state_lock);

I thought a bit the last few days about how to integrate modeset locking into driver private state objects. I think the simplest solution would be if we just add a drm_modeset_lock to drm_private_obj, and push the locking (both here and in the mst helper) into the core private obj code.

I'm also a bit unclear on how many private objs one should create. In this patchset,
I just create one private obj instance, and stuff all of our shared resources into
it (see mdp5_global_state below). I didn't see the point in creating one priv object
per shared resource, since a single lock for all of them seemed okay here.

Thanks,
Archit

Quoted 82 lines

Per-object locking might be a bit overkill (it's definitely overkill for mst), but it's also easier to avoid special cases.

That would reduce the boilerplate here a bit more, essentially converting the wrappers into type casting functions.
-Daniel

Quoted 74 lines

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +9 -0

+
+       state->mdp5_kms = mdp5_kms;
+
+       drm_atomic_private_obj_init(&mdp5_kms->glob_base,
+                                   &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));

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +1 -0

@@ -727,6 +807,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_base);
+
        kfree(mdp5_kms->state);
  }
  @@ -887,6 +969,10 @@ static int mdp5_init(struct platform_device *pdev,

struct drm_device *dev)

                goto fail;
        }

+ ret = mdp5_global_obj_init(mdp5_kms);

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +3 -0

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

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h +6 -0

index 9b3fe01089d1..522ddb835593 100644
@@ -55,6 +55,13 @@ 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_base;
+       struct drm_modeset_lock glob_state_lock;
+
        struct mdp5_smp *smp;
        struct mdp5_ctl_manager *ctlm;
  @@ -95,6 +102,26 @@ 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)

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h +16 -0

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

Quoted 9 lines

Quoted 8 lines

/* Atomic plane state. Subclasses the base drm_plane_state in order to
* track assigned hwpipe and hw specific state.
*/
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Quoted 29 lines · Archit Taneja

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 <[email protected]>

---
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 86 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h | 27 +++++++++++
 2 files changed, 113 insertions(+)

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +1 -0

index f7c0698fec40..dfc4d81124d5 100644
@@ -106,6 +106,86 @@ 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 */

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c static void mdp5_swap_state(struct msm_kms *kms, struct drm_atomic_state *state) · line 119

Quoted 10 lines

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +10 -0

+
+/*
+ * 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_base.state);

This doesn't match the existing state stuff for everything else. Here you look at the global state, but not at the one hanging off drm_atomic_state.

Maybe we should add a drm_atomic_get_existing_private_obj_state function to make this easier?

I'm also not 100% sure on what semantics you want precisely.

Quoted 58 lines

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +57 -0

+}
+
+/*
+ * 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;
+     int ret;
+
+     ret = drm_modeset_lock(&mdp5_kms->glob_state_lock, s->acquire_ctx);
+     if (ret)
+             return ERR_PTR(ret);
+
+     priv_state = drm_atomic_get_private_obj_state(s, &mdp5_kms->glob_base);
+     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)

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c static void mdp5_swap_state(struct msm_kms *kms, struct drm_atomic_state *state) · line 179

Quoted 3 lines

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +3 -0

+             return -ENOMEM;
+
+     drm_modeset_lock_init(&mdp5_kms->glob_state_lock);

I thought a bit the last few days about how to integrate modeset locking into driver private state objects. I think the simplest solution would be if we just add a drm_modeset_lock to drm_private_obj, and push the locking (both here and in the mst helper) into the core private obj code.

Per-object locking might be a bit overkill (it's definitely overkill for mst), but it's also easier to avoid special cases.

That would reduce the boilerplate here a bit more, essentially converting the wrappers into type casting functions.
-Daniel

Quoted 78 lines

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +15 -0

+
+     state->mdp5_kms = mdp5_kms;
+
+     drm_atomic_private_obj_init(&mdp5_kms->glob_base,
+                                 &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));

@@ -727,6 +807,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_base);
+
      kfree(mdp5_kms->state);
 }
 

@@ -887,6 +969,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);

drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h +27 -1

index 9b3fe01089d1..522ddb835593 100644
@@ -55,6 +55,13 @@ 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_base;
+     struct drm_modeset_lock glob_state_lock;
+
      struct mdp5_smp *smp;
      struct mdp5_ctl_manager *ctlm;
 

@@ -95,6 +102,26 @@ 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.
  */
--

Quoted 3 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation

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