Re: [DPU PATCH v3] drm/msm: Use atomic private_obj instead of subclassing

Jeykumar Sankaran <jsanka@xxxxxxxxxxxxxx> · Thu, 24 May 2018 18:16:41 -0700

On 2018-03-20 04:01, Archit Taneja wrote:
Hi,

On Tuesday 20 March 2018 01:28 AM, Sean Paul wrote:
Instead of subclassing atomic state, store driver private data in
private_obj/state. This allows us to remove the swap_state driver hook
for mdp5 and get closer to using the atomic helpers entirely.

Changes in v2:
  - Use state->state in disp duplicate_state callback (Jeykumar)
Changes in v3:
  - Update comment describing msm_kms_state (Jeykumar)


One difference w.r.t the patchset
"drm/msm/mdp5: Use the new private_obj state" is that this
adds the atomic private_obj for all kms backends, whereas the one
I'd done originally just added it for mdp5, so this patch set
is better in that respect.

The other difference is how we 'get' the private state. In this patch,
the helper drm_atomic_get_private_obj_state() is used every time to
get the private object state.

I'd tried to do the same initially, but I noticed that
drm_atomic_get_private_obj_state() doesn't return the correct
state post swapping of states. So, instead of relying on the helper,
I created a helper called mdp5_get_existing_global_state() that
returns the desired state(the state pointer in 
msm_kms>state->base.state
in this patch) in all funcs that are called post-swap.


You could read about the issue on the the thread: "[RFC 1/3]
drm/msm/mdp5: Add global state as a private atomic object"

The problem was quite apparent with db410c, where SMP blocks are
assigned to planes. If the latest SMP state isn't referred when
configuring SMP registers, we see underruns immediately.

An easy way to reproduce this is to use modset on db410c. I think
it might occur with this patch too. It might be worth trying it
out.

Thanks,
Archit

Hello Archit, Now that I am trying to use the private obj to maintain 
resource pool in DPU, I realize the need for retrieving existing / new 
state API's. Don't you think this support is missing in the DRM core 
fwk, considering how drm_atomic_get_existing_XXXX_state, 
drm_atomic_get_old_XXXX_state, drm_atomic_get_new_XXXX_state API's are 
available for CRTC/PLANE and CONNECTORS but not for private_obj's?

Thanks,
Jeykumar S.
Cc: Jeykumar Sankaran <jsanka@xxxxxxxxxxxxxx>
Reviewed-by: Jeykumar Sankaran <jsanka@xxxxxxxxxxxxxx>
Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
---
  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 37 ++++++--------
  drivers/gpu/drm/msm/msm_atomic.c         | 30 -----------
  drivers/gpu/drm/msm/msm_drv.c            | 65 
++++++++++++++++++++++--
  drivers/gpu/drm/msm/msm_drv.h            |  4 +-
  drivers/gpu/drm/msm/msm_kms.h            | 27 +++++-----
  5 files changed, 94 insertions(+), 69 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c 
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 6d8e3a9a6fc0..f1a248419655 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -74,36 +74,32 @@ 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;
+	struct msm_kms_state *kms_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);
+	kms_state = msm_kms_get_state(s);
+	if (IS_ERR_OR_NULL(kms_state))
+		return (struct mdp5_state *)kms_state; /* casting ERR_PTR */

-	/* 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;
+	return kms_state->state;
+}

-	state->state = new_state;
+static void *mdp5_duplicate_state(void *state)
+{
+	if (!state)
+		return kzalloc(sizeof(struct mdp5_state), GFP_KERNEL);

-	return new_state;
+	return kmemdup(state, sizeof(struct mdp5_state), GFP_KERNEL);
  }

-static void mdp5_swap_state(struct msm_kms *kms, struct 
drm_atomic_state *state)
+static void mdp5_destroy_state(void *state)
  {
-	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
-	swap(to_kms_state(state)->state, mdp5_kms->state);
+	struct mdp5_state *mdp_state = state;
+	kfree(mdp_state);
  }

  static void mdp5_prepare_commit(struct msm_kms *kms, struct 
drm_atomic_state *state)
@@ -229,7 +225,8 @@ 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,
+		.duplicate_state = mdp5_duplicate_state,
+		.destroy_state	 = mdp5_destroy_state,
  		.prepare_commit  = mdp5_prepare_commit,
  		.complete_commit = mdp5_complete_commit,
  		.wait_for_crtc_commit_done = mdp5_wait_for_crtc_commit_done,
@@ -726,8 +723,6 @@ static void mdp5_destroy(struct platform_device 
*pdev)

  	if (mdp5_kms->rpm_enabled)
  		pm_runtime_disable(&pdev->dev);
-
-	kfree(mdp5_kms->state);
  }

  static int construct_pipes(struct mdp5_kms *mdp5_kms, int cnt,
diff --git a/drivers/gpu/drm/msm/msm_atomic.c 
b/drivers/gpu/drm/msm/msm_atomic.c
index 7e54eb65d096..1f53262ea46b 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -169,9 +169,6 @@ int msm_atomic_commit(struct drm_device *dev,
  	 */
  	BUG_ON(drm_atomic_helper_swap_state(state, false) < 0);

-	if (to_kms_state(state)->state)
-		priv->kms->funcs->swap_state(priv->kms, state);
-
  	/*
  	 * Provide the driver a chance to prepare for output fences. This 
is
  	 * done after the point of no return, but before asynchronous 
commits
@@ -210,30 +207,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 5219792e93bc..4c0740dc7854 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -121,9 +121,62 @@ static const struct drm_mode_config_funcs 
mode_config_funcs = {
  	.output_poll_changed = msm_fb_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,
+};
+
+static inline
+struct msm_kms *obj_to_kms(struct drm_private_obj *obj)
+{
+	return container_of(obj, struct msm_kms, base);
+}
+
+static inline
+struct msm_kms_state *state_to_kms_state(struct drm_private_state 
*state)
+{
+	return container_of(state, struct msm_kms_state, base);
+}
+
+struct msm_kms_state *msm_kms_get_state(struct drm_atomic_state *s)
+{
+	struct msm_drm_private *priv = s->dev->dev_private;
+	struct drm_private_obj *obj = &priv->kms->base;
+	struct drm_private_state *state;
+
+	state = drm_atomic_get_private_obj_state(s, obj);
+	if (IS_ERR_OR_NULL(state))
+		return (struct msm_kms_state *)state; /* casting ERR_PTR */
+
+	return state_to_kms_state(state);
+}
+
+static struct drm_private_state *
+msm_kms_duplicate_state(struct drm_private_obj *obj)
+{
+	struct msm_kms *kms = obj_to_kms(obj);
+	struct msm_kms_state *state = state_to_kms_state(obj->state);
+	struct msm_kms_state *new_state;
+
+	if (kms->funcs->duplicate_state)
+		new_state = kms->funcs->duplicate_state(state->state);
+
+	__drm_atomic_helper_private_obj_duplicate_state(obj, 
&new_state->base);
+
+	return &new_state->base;
+}
+
+static void msm_kms_destroy_state(struct drm_private_obj *obj,
+				  struct drm_private_state *state)
+{
+	struct msm_kms *kms = obj_to_kms(obj);
+	struct msm_kms_state *kms_state = state_to_kms_state(obj->state);
+
+	if (kms->funcs->destroy_state)
+		kms->funcs->destroy_state(kms_state->state);
+	kfree(kms_state);
+}
+
+static const struct drm_private_state_funcs kms_state_funcs = {
+	.atomic_duplicate_state = msm_kms_duplicate_state,
+	.atomic_destroy_state = msm_kms_destroy_state,
  };

  #ifdef CONFIG_DRM_MSM_REGISTER_LOGGING
@@ -341,6 +394,8 @@ static int msm_drm_uninit(struct device *dev)
  		}
  	}

+	drm_atomic_private_obj_fini(&kms->base);
+
  	msm_gem_shrinker_cleanup(ddev);

  	drm_kms_helper_poll_fini(ddev);
@@ -770,6 +825,10 @@ static int msm_drm_init(struct device *dev, 
struct drm_driver *drv)
  	}
  #endif

+	drm_atomic_private_obj_init(&kms->base,
+				    &kms->state.base,
+				    &kms_state_funcs);
+
  	/* perform subdriver post initialization */
  	if (kms && kms->funcs && kms->funcs->postinit) {
  		ret = kms->funcs->postinit(kms);
diff --git a/drivers/gpu/drm/msm/msm_drv.h 
b/drivers/gpu/drm/msm/msm_drv.h
index 292496b682e8..8cab333df717 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -598,9 +598,7 @@ struct msm_format {

  int msm_atomic_commit(struct drm_device *dev,
  		struct drm_atomic_state *state, bool nonblock);
-struct drm_atomic_state *msm_atomic_state_alloc(struct drm_device 
*dev);
-void msm_atomic_state_clear(struct drm_atomic_state *state);
-void msm_atomic_state_free(struct drm_atomic_state *state);
+struct msm_kms_state *msm_kms_get_state(struct drm_atomic_state 
*state);

  void msm_gem_unmap_vma(struct msm_gem_address_space *aspace,
  		struct msm_gem_vma *vma, struct sg_table *sgt);
diff --git a/drivers/gpu/drm/msm/msm_kms.h 
b/drivers/gpu/drm/msm/msm_kms.h
index 09ba1e79db50..5c1bcaf65fd9 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -57,6 +57,8 @@ struct msm_kms_funcs {
  	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);
+	void *(*duplicate_state)(void *state);
+	void (*destroy_state)(void *state);
  	/* modeset, bracketing atomic_commit(): */
  	void (*prepare_fence)(struct msm_kms *kms,
  			struct drm_atomic_state *state);
@@ -114,7 +116,20 @@ struct msm_kms_funcs {
  #endif
  };

+/**
+ * Subclass of drm_private_state, to allow kms backend to have driver
+ * private global state.  The kms backend can do whatever it wants
+ * with the ->state ptr.
+ */
+struct msm_kms_state {
+	struct drm_private_state base;
+	void *state;
+};
+
  struct msm_kms {
+	struct drm_private_obj base;
+	struct msm_kms_state state;
+
  	const struct msm_kms_funcs *funcs;

  	/* irq number to be passed on to drm_irq_install */
@@ -124,18 +139,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)
  {


--
Jeykumar S
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html






