Re: [RFC 1/3] drm/msm/mdp5: Add global state as a private atomic object

[Rob Clark]

On Thu, Dec 21, 2017 at 4:56 AM, Daniel Vetter  wrote:
> On Thu, Dec 21, 2017 at 11:44:23AM +0530, Archit Taneja wrote:
>> 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 
>> ---
>>  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(+)
>>
>> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c 
>> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>> index f7c0698fec40..dfc4d81124d5 100644
>> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>> @@ -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.
>> + */
>> +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.
>

This is used for read-only access to current state, a couple places in
atomic update post-swap (where
drm_atomic_get_existing_private_obj_state() would work, but also for
things like debugfs, where it wouldn't)..

Probably should const'ify the return value to make it clear that this
is read-only.

>
>> +}
>> +
>> +/*
>> + * 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(_kms->glob_state_lock, s->acquire_ctx);
>> + if (ret)
>> + return ERR_PTR(ret);
>> +
>> + priv_state = drm_atomic_get_private_obj_state(s, _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, >base);
>> +
>> + return >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;
>> +
>> + drm_modeset_lock_init(_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 like the idea of adding a modeset lock to private objs.. I've got a
WIP patch to do this.

BR,
-R

> Per-object locking might be a bit overkill (it's definitely overkill for
> mst), but it's 

Re: [RFC 1/3] drm/msm/mdp5: Add global state as a private atomic object

[Rob Clark]

On Wed, Jan 3, 2018 at 6:32 AM, Archit Taneja  wrote:
>
>
> On 12/21/2017 03:26 PM, Daniel Vetter wrote:
>>
>> On Thu, Dec 21, 2017 at 11:44:23AM +0530, Archit Taneja wrote:
>>>
>>> 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 
>>> ---
>>>   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(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>>> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>>> index f7c0698fec40..dfc4d81124d5 100644
>>> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>>> @@ -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.
>>> + */
>>> +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.
>
>
> 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.
>
>
>>
>>
>>> +}
>>> +
>>> +/*
>>> + * 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(_kms->glob_state_lock,
>>> s->acquire_ctx);
>>> +   if (ret)
>>> +   return ERR_PTR(ret);
>>> +
>>> +   priv_state = drm_atomic_get_private_obj_state(s,
>>> _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,
>>> >base);
>>> +
>>> +   return >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;
>>> +
>>> +   drm_modeset_lock_init(_kms->glob_state_lock);
>>
>>
>> I thought a bit the 

Re: [RFC 1/3] drm/msm/mdp5: Add global state as a private atomic object

[Archit Taneja]

On 12/21/2017 03:26 PM, Daniel Vetter wrote:

On Thu, Dec 21, 2017 at 11:44:23AM +0530, Archit Taneja wrote:

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

diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c 
b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
index f7c0698fec40..dfc4d81124d5 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
@@ -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.
+ */
+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.


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.





+}
+
+/*
+ * 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(_kms->glob_state_lock, s->acquire_ctx);
+   if (ret)
+   return ERR_PTR(ret);
+
+   priv_state = drm_atomic_get_private_obj_state(s, _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, >base);
+
+   return >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;
+
+   drm_modeset_lock_init(_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 

Re: [RFC 1/3] drm/msm/mdp5: Add global state as a private atomic object

[Daniel Vetter]

On Thu, Dec 21, 2017 at 11:44:23AM +0530, Archit Taneja wrote:
> 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 
> ---
>  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(+)
> 
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c 
> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> index f7c0698fec40..dfc4d81124d5 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> @@ -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.
> + */
> +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.


> +}
> +
> +/*
> + * 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(_kms->glob_state_lock, s->acquire_ctx);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + priv_state = drm_atomic_get_private_obj_state(s, _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, >base);
> +
> + return >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;
> +
> + drm_modeset_lock_init(_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

> +
> + state->mdp5_kms = mdp5_kms;
> +
> + drm_atomic_private_obj_init(_kms->glob_base,
> + >base,
> + _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 

[RFC 1/3] drm/msm/mdp5: Add global state as a private atomic object

[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 
---
 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(+)

diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c 
b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
index f7c0698fec40..dfc4d81124d5 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
@@ -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.
+ */
+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 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(_kms->glob_state_lock, s->acquire_ctx);
+   if (ret)
+   return ERR_PTR(ret);
+
+   priv_state = drm_atomic_get_private_obj_state(s, _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, >base);
+
+   return >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;
+
+   drm_modeset_lock_init(_kms->glob_state_lock);
+
+   state->mdp5_kms = mdp5_kms;
+
+   drm_atomic_private_obj_init(_kms->glob_base,
+   >base,
+   _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(>dev);
 
+   drm_atomic_private_obj_fini(_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);
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h 
b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
index 9b3fe01089d1..522ddb835593 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
@@ -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