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

[Pandiyan, Dhinakaran]

IRC acked by Harry Wentland


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


-DK
On Wed, 2017-02-08 at 22:38 -0800, Dhinakaran Pandiyan wrote:
> It is necessary to track states for objects other than connector, crtc
> and plane for atomic modesets. But adding objects like DP MST link
> bandwidth to drm_atomic_state would mean that a non-core object will be
> modified by the core helper functions for swapping and clearing
> it's state. So, lets add void * objects and helper functions that operate
> on void * types to keep these objects and states private to the core.
> Drivers can then implement specific functions to swap and clear states.
> The other advantage having just void * for these objects in
> drm_atomic_state is that objects of different types can be managed in the
> same state array.
> 
> v2: Added docs and new iterator to filter private objects (Daniel)
> 
> Suggested-by: Daniel Vetter 
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  drivers/gpu/drm/drm_atomic.c| 68 +++
>  drivers/gpu/drm/drm_atomic_helper.c |  5 ++
>  include/drm/drm_atomic.h| 91 
> +
>  3 files changed, 164 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index a567310..1a9ffe8 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct 
> drm_atomic_state *state)
>   kfree(state->connectors);
>   kfree(state->crtcs);
>   kfree(state->planes);
> + kfree(state->private_objs);
>  }
>  EXPORT_SYMBOL(drm_atomic_state_default_release);
>  
> @@ -184,6 +185,20 @@ void drm_atomic_state_default_clear(struct 
> drm_atomic_state *state)
>   state->planes[i].ptr = NULL;
>   state->planes[i].state = NULL;
>   }
> +
> + for (i = 0; i < state->num_private_objs; i++) {
> + void *private_obj = state->private_objs[i].obj;
> + void *obj_state = state->private_objs[i].obj_state;
> +
> + if (!private_obj)
> + continue;
> +
> + state->private_objs[i].funcs->destroy_state(obj_state);
> + state->private_objs[i].obj = NULL;
> + state->private_objs[i].obj_state = NULL;
> + state->private_objs[i].funcs = NULL;
> + }
> +
>  }
>  EXPORT_SYMBOL(drm_atomic_state_default_clear);
>  
> @@ -974,6 +989,59 @@ static void drm_atomic_plane_print_state(struct 
> drm_printer *p,
>  }
>  
>  /**
> + * drm_atomic_get_private_obj_state - get private object state
> + * @state: global atomic state
> + * @obj: private object to get the state for
> + * @funcs: pointer to the struct of function pointers that identify the 
> object
> + * type
> + *
> + * This function returns the private object state for the given private 
> object,
> + * allocating the state if needed. It does not grab any locks as the caller 
> is
> + * expected to care of any required locking.
> + *
> + * RETURNS:
> + *
> + * Either the allocated state or the error code encoded into a pointer.
> + */
> +void *
> +drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,
> +   const struct drm_private_state_funcs *funcs)
> +{
> + int index, num_objs, i;
> + size_t size;
> + struct __drm_private_objs_state *arr;
> +
> + for (i = 0; i < state->num_private_objs; i++)
> + if (obj == state->private_objs[i].obj &&
> + state->private_objs[i].obj_state)
> + return state->private_objs[i].obj_state;
> +
> + num_objs = state->num_private_objs + 1;
> + size = sizeof(*state->private_objs) * num_objs;
> + arr = krealloc(state->private_objs, size, GFP_KERNEL);
> + if (!arr)
> + return ERR_PTR(-ENOMEM);
> +
> + state->private_objs = arr;
> + index = state->num_private_objs;
> + memset(>private_objs[index], 0, sizeof(*state->private_objs));
> +
> + state->private_objs[index].obj_state = funcs->duplicate_state(state, 
> obj);
> + if (!state->private_objs[index].obj_state)
> + return ERR_PTR(-ENOMEM);
> +
> + state->private_objs[index].obj = obj;
> + state->private_objs[index].funcs = funcs;
> + state->num_private_objs = num_objs;
> +
> + DRM_DEBUG_ATOMIC("Added new private object state %p to %p\n",
> +  state->private_objs[index].obj_state, state);
> +
> + return state->private_objs[index].obj_state;
> +}
> +EXPORT_SYMBOL(drm_atomic_get_private_obj_state);
> +
> +/**
>   * drm_atomic_get_connector_state - get connector state
>   * @state: global atomic state object
>   * @connector: connector to get state object for
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> 

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

[Pandiyan, Dhinakaran]

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

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

-DK

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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

Please don't add this as a comment, but as real code so it is checked at
runtime :-) Personally I wouldn't mention locking rules in kernel-doc,
that part tends to get outdated fast. Better to enforce with
runtime-checks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Pandiyan, Dhinakaran]

On Wed, 2017-02-22 at 09:59 +0530, Archit Taneja wrote:
> 
> On 02/22/2017 05:31 AM, Pandiyan, Dhinakaran wrote:
> > On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote:
> >>
> >> On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote:
> >>> On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
>  Hi,
> 
>  On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:
> > It is necessary to track states for objects other than connector, crtc
> > and plane for atomic modesets. But adding objects like DP MST link
> > bandwidth to drm_atomic_state would mean that a non-core object will be
> > modified by the core helper functions for swapping and clearing
> > it's state. So, lets add void * objects and helper functions that 
> > operate
> > on void * types to keep these objects and states private to the core.
> > Drivers can then implement specific functions to swap and clear states.
> > The other advantage having just void * for these objects in
> > drm_atomic_state is that objects of different types can be managed in 
> > the
> > same state array.
> >
> > v2: Added docs and new iterator to filter private objects (Daniel)
> >
> > Suggested-by: Daniel Vetter 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/drm_atomic.c| 68 +++
> >  drivers/gpu/drm/drm_atomic_helper.c |  5 ++
> >  include/drm/drm_atomic.h| 91 
> > +
> >  3 files changed, 164 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index a567310..1a9ffe8 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct 
> > drm_atomic_state *state)
> > kfree(state->connectors);
> > kfree(state->crtcs);
> > kfree(state->planes);
> > +   kfree(state->private_objs);
> >  }
> >  EXPORT_SYMBOL(drm_atomic_state_default_release);
> >
> > @@ -184,6 +185,20 @@ void drm_atomic_state_default_clear(struct 
> > drm_atomic_state *state)
> > state->planes[i].ptr = NULL;
> > state->planes[i].state = NULL;
> > }
> > +
> > +   for (i = 0; i < state->num_private_objs; i++) {
> > +   void *private_obj = state->private_objs[i].obj;
> > +   void *obj_state = state->private_objs[i].obj_state;
> > +
> > +   if (!private_obj)
> > +   continue;
> > +
> > +   state->private_objs[i].funcs->destroy_state(obj_state);
> > +   state->private_objs[i].obj = NULL;
> > +   state->private_objs[i].obj_state = NULL;
> > +   state->private_objs[i].funcs = NULL;
> > +   }
> > +
> >  }
> >  EXPORT_SYMBOL(drm_atomic_state_default_clear);
> >
> > @@ -974,6 +989,59 @@ static void drm_atomic_plane_print_state(struct 
> > drm_printer *p,
> >  }
> >
> >  /**
> > + * drm_atomic_get_private_obj_state - get private object state
> > + * @state: global atomic state
> > + * @obj: private object to get the state for
> > + * @funcs: pointer to the struct of function pointers that identify 
> > the object
> > + * type
> > + *
> > + * This function returns the private object state for the given 
> > private object,
> > + * allocating the state if needed. It does not grab any locks as the 
> > caller is
> > + * expected to care of any required locking.
> > + *
> > + * RETURNS:
> > + *
> > + * Either the allocated state or the error code encoded into a pointer.
> > + */
> > +void *
> > +drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void 
> > *obj,
> > + const struct drm_private_state_funcs 
> > *funcs)
> > +{
> > +   int index, num_objs, i;
> > +   size_t size;
> > +   struct __drm_private_objs_state *arr;
> > +
> > +   for (i = 0; i < state->num_private_objs; i++)
> > +   if (obj == state->private_objs[i].obj &&
> > +   state->private_objs[i].obj_state)
> > +   return state->private_objs[i].obj_state;
> 
>  Comparing this func to 
>  drm_atomic_get_plane_state/drm_atomic_get_crtc_state, it
>  doesn't seem to call drm_modeset_lock if the obj_state doesn't already 
>  exist. I
>  don't understand the locking stuff toowell, I just noticed this 
>  difference when
>  comparing this approach with what is done in the msm kms driver (where we
>  have subclassed drm_atomic_state to msm_kms_state).
> 
>  Thanks,
>  Archit
> 

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

[Archit Taneja]

On 02/22/2017 05:31 AM, Pandiyan, Dhinakaran wrote:

On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote:


On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote:

On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:

Hi,

On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:

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

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

Suggested-by: Daniel Vetter 
Signed-off-by: Dhinakaran Pandiyan 
---
 drivers/gpu/drm/drm_atomic.c| 68 +++
 drivers/gpu/drm/drm_atomic_helper.c |  5 ++
 include/drm/drm_atomic.h| 91 +
 3 files changed, 164 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index a567310..1a9ffe8 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state 
*state)
kfree(state->connectors);
kfree(state->crtcs);
kfree(state->planes);
+   kfree(state->private_objs);
 }
 EXPORT_SYMBOL(drm_atomic_state_default_release);

@@ -184,6 +185,20 @@ void drm_atomic_state_default_clear(struct 
drm_atomic_state *state)
state->planes[i].ptr = NULL;
state->planes[i].state = NULL;
}
+
+   for (i = 0; i < state->num_private_objs; i++) {
+   void *private_obj = state->private_objs[i].obj;
+   void *obj_state = state->private_objs[i].obj_state;
+
+   if (!private_obj)
+   continue;
+
+   state->private_objs[i].funcs->destroy_state(obj_state);
+   state->private_objs[i].obj = NULL;
+   state->private_objs[i].obj_state = NULL;
+   state->private_objs[i].funcs = NULL;
+   }
+
 }
 EXPORT_SYMBOL(drm_atomic_state_default_clear);

@@ -974,6 +989,59 @@ static void drm_atomic_plane_print_state(struct 
drm_printer *p,
 }

 /**
+ * drm_atomic_get_private_obj_state - get private object state
+ * @state: global atomic state
+ * @obj: private object to get the state for
+ * @funcs: pointer to the struct of function pointers that identify the object
+ * type
+ *
+ * This function returns the private object state for the given private object,
+ * allocating the state if needed. It does not grab any locks as the caller is
+ * expected to care of any required locking.
+ *
+ * RETURNS:
+ *
+ * Either the allocated state or the error code encoded into a pointer.
+ */
+void *
+drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,
+ const struct drm_private_state_funcs *funcs)
+{
+   int index, num_objs, i;
+   size_t size;
+   struct __drm_private_objs_state *arr;
+
+   for (i = 0; i < state->num_private_objs; i++)
+   if (obj == state->private_objs[i].obj &&
+   state->private_objs[i].obj_state)
+   return state->private_objs[i].obj_state;


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

Thanks,
Archit




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


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

Thanks,
Archit



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

WARN_ON(!drm_modeset_is_locked(>mode_config.connection_mutex));


That would be nice to have.

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

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

[Pandiyan, Dhinakaran]

On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote:
> 
> On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote:
> > On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
> >> Hi,
> >>
> >> On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:
> >>> It is necessary to track states for objects other than connector, crtc
> >>> and plane for atomic modesets. But adding objects like DP MST link
> >>> bandwidth to drm_atomic_state would mean that a non-core object will be
> >>> modified by the core helper functions for swapping and clearing
> >>> it's state. So, lets add void * objects and helper functions that operate
> >>> on void * types to keep these objects and states private to the core.
> >>> Drivers can then implement specific functions to swap and clear states.
> >>> The other advantage having just void * for these objects in
> >>> drm_atomic_state is that objects of different types can be managed in the
> >>> same state array.
> >>>
> >>> v2: Added docs and new iterator to filter private objects (Daniel)
> >>>
> >>> Suggested-by: Daniel Vetter 
> >>> Signed-off-by: Dhinakaran Pandiyan 
> >>> ---
> >>>  drivers/gpu/drm/drm_atomic.c| 68 +++
> >>>  drivers/gpu/drm/drm_atomic_helper.c |  5 ++
> >>>  include/drm/drm_atomic.h| 91 
> >>> +
> >>>  3 files changed, 164 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >>> index a567310..1a9ffe8 100644
> >>> --- a/drivers/gpu/drm/drm_atomic.c
> >>> +++ b/drivers/gpu/drm/drm_atomic.c
> >>> @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct 
> >>> drm_atomic_state *state)
> >>>   kfree(state->connectors);
> >>>   kfree(state->crtcs);
> >>>   kfree(state->planes);
> >>> + kfree(state->private_objs);
> >>>  }
> >>>  EXPORT_SYMBOL(drm_atomic_state_default_release);
> >>>
> >>> @@ -184,6 +185,20 @@ void drm_atomic_state_default_clear(struct 
> >>> drm_atomic_state *state)
> >>>   state->planes[i].ptr = NULL;
> >>>   state->planes[i].state = NULL;
> >>>   }
> >>> +
> >>> + for (i = 0; i < state->num_private_objs; i++) {
> >>> + void *private_obj = state->private_objs[i].obj;
> >>> + void *obj_state = state->private_objs[i].obj_state;
> >>> +
> >>> + if (!private_obj)
> >>> + continue;
> >>> +
> >>> + state->private_objs[i].funcs->destroy_state(obj_state);
> >>> + state->private_objs[i].obj = NULL;
> >>> + state->private_objs[i].obj_state = NULL;
> >>> + state->private_objs[i].funcs = NULL;
> >>> + }
> >>> +
> >>>  }
> >>>  EXPORT_SYMBOL(drm_atomic_state_default_clear);
> >>>
> >>> @@ -974,6 +989,59 @@ static void drm_atomic_plane_print_state(struct 
> >>> drm_printer *p,
> >>>  }
> >>>
> >>>  /**
> >>> + * drm_atomic_get_private_obj_state - get private object state
> >>> + * @state: global atomic state
> >>> + * @obj: private object to get the state for
> >>> + * @funcs: pointer to the struct of function pointers that identify the 
> >>> object
> >>> + * type
> >>> + *
> >>> + * This function returns the private object state for the given private 
> >>> object,
> >>> + * allocating the state if needed. It does not grab any locks as the 
> >>> caller is
> >>> + * expected to care of any required locking.
> >>> + *
> >>> + * RETURNS:
> >>> + *
> >>> + * Either the allocated state or the error code encoded into a pointer.
> >>> + */
> >>> +void *
> >>> +drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void 
> >>> *obj,
> >>> +   const struct drm_private_state_funcs *funcs)
> >>> +{
> >>> + int index, num_objs, i;
> >>> + size_t size;
> >>> + struct __drm_private_objs_state *arr;
> >>> +
> >>> + for (i = 0; i < state->num_private_objs; i++)
> >>> + if (obj == state->private_objs[i].obj &&
> >>> + state->private_objs[i].obj_state)
> >>> + return state->private_objs[i].obj_state;
> >>
> >> Comparing this func to 
> >> drm_atomic_get_plane_state/drm_atomic_get_crtc_state, it
> >> doesn't seem to call drm_modeset_lock if the obj_state doesn't already 
> >> exist. I
> >> don't understand the locking stuff toowell, I just noticed this difference 
> >> when
> >> comparing this approach with what is done in the msm kms driver (where we
> >> have subclassed drm_atomic_state to msm_kms_state).
> >>
> >> Thanks,
> >> Archit
> >>
> >
> >
> > The caller is expected to take care of any required locking. The
> > driver-private objects are opaque from core's pov, so the core is not
> > aware of necessary locks for that object type.
> 
> I had a look at the rest of the series, and I couldn't easily understand
> whether the caller code protects the MST related driver private state. Is
> it expected to be protect via the drm_mode_config.connection_mutex lock?
> 
> Thanks,
> Archit
> 

That's right, the connection_mutex takes care of the 

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

[Archit Taneja]

On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote:

On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:

Hi,

On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:

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

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

Suggested-by: Daniel Vetter 
Signed-off-by: Dhinakaran Pandiyan 
---
 drivers/gpu/drm/drm_atomic.c| 68 +++
 drivers/gpu/drm/drm_atomic_helper.c |  5 ++
 include/drm/drm_atomic.h| 91 +
 3 files changed, 164 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index a567310..1a9ffe8 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state 
*state)
kfree(state->connectors);
kfree(state->crtcs);
kfree(state->planes);
+   kfree(state->private_objs);
 }
 EXPORT_SYMBOL(drm_atomic_state_default_release);

@@ -184,6 +185,20 @@ void drm_atomic_state_default_clear(struct 
drm_atomic_state *state)
state->planes[i].ptr = NULL;
state->planes[i].state = NULL;
}
+
+   for (i = 0; i < state->num_private_objs; i++) {
+   void *private_obj = state->private_objs[i].obj;
+   void *obj_state = state->private_objs[i].obj_state;
+
+   if (!private_obj)
+   continue;
+
+   state->private_objs[i].funcs->destroy_state(obj_state);
+   state->private_objs[i].obj = NULL;
+   state->private_objs[i].obj_state = NULL;
+   state->private_objs[i].funcs = NULL;
+   }
+
 }
 EXPORT_SYMBOL(drm_atomic_state_default_clear);

@@ -974,6 +989,59 @@ static void drm_atomic_plane_print_state(struct 
drm_printer *p,
 }

 /**
+ * drm_atomic_get_private_obj_state - get private object state
+ * @state: global atomic state
+ * @obj: private object to get the state for
+ * @funcs: pointer to the struct of function pointers that identify the object
+ * type
+ *
+ * This function returns the private object state for the given private object,
+ * allocating the state if needed. It does not grab any locks as the caller is
+ * expected to care of any required locking.
+ *
+ * RETURNS:
+ *
+ * Either the allocated state or the error code encoded into a pointer.
+ */
+void *
+drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,
+ const struct drm_private_state_funcs *funcs)
+{
+   int index, num_objs, i;
+   size_t size;
+   struct __drm_private_objs_state *arr;
+
+   for (i = 0; i < state->num_private_objs; i++)
+   if (obj == state->private_objs[i].obj &&
+   state->private_objs[i].obj_state)
+   return state->private_objs[i].obj_state;


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

Thanks,
Archit




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


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

Thanks,
Archit

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

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

[Pandiyan, Dhinakaran]

On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
> Hi,
> 
> On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:
> > It is necessary to track states for objects other than connector, crtc
> > and plane for atomic modesets. But adding objects like DP MST link
> > bandwidth to drm_atomic_state would mean that a non-core object will be
> > modified by the core helper functions for swapping and clearing
> > it's state. So, lets add void * objects and helper functions that operate
> > on void * types to keep these objects and states private to the core.
> > Drivers can then implement specific functions to swap and clear states.
> > The other advantage having just void * for these objects in
> > drm_atomic_state is that objects of different types can be managed in the
> > same state array.
> >
> > v2: Added docs and new iterator to filter private objects (Daniel)
> >
> > Suggested-by: Daniel Vetter 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/drm_atomic.c| 68 +++
> >  drivers/gpu/drm/drm_atomic_helper.c |  5 ++
> >  include/drm/drm_atomic.h| 91 
> > +
> >  3 files changed, 164 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index a567310..1a9ffe8 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct 
> > drm_atomic_state *state)
> > kfree(state->connectors);
> > kfree(state->crtcs);
> > kfree(state->planes);
> > +   kfree(state->private_objs);
> >  }
> >  EXPORT_SYMBOL(drm_atomic_state_default_release);
> >
> > @@ -184,6 +185,20 @@ void drm_atomic_state_default_clear(struct 
> > drm_atomic_state *state)
> > state->planes[i].ptr = NULL;
> > state->planes[i].state = NULL;
> > }
> > +
> > +   for (i = 0; i < state->num_private_objs; i++) {
> > +   void *private_obj = state->private_objs[i].obj;
> > +   void *obj_state = state->private_objs[i].obj_state;
> > +
> > +   if (!private_obj)
> > +   continue;
> > +
> > +   state->private_objs[i].funcs->destroy_state(obj_state);
> > +   state->private_objs[i].obj = NULL;
> > +   state->private_objs[i].obj_state = NULL;
> > +   state->private_objs[i].funcs = NULL;
> > +   }
> > +
> >  }
> >  EXPORT_SYMBOL(drm_atomic_state_default_clear);
> >
> > @@ -974,6 +989,59 @@ static void drm_atomic_plane_print_state(struct 
> > drm_printer *p,
> >  }
> >
> >  /**
> > + * drm_atomic_get_private_obj_state - get private object state
> > + * @state: global atomic state
> > + * @obj: private object to get the state for
> > + * @funcs: pointer to the struct of function pointers that identify the 
> > object
> > + * type
> > + *
> > + * This function returns the private object state for the given private 
> > object,
> > + * allocating the state if needed. It does not grab any locks as the 
> > caller is
> > + * expected to care of any required locking.
> > + *
> > + * RETURNS:
> > + *
> > + * Either the allocated state or the error code encoded into a pointer.
> > + */
> > +void *
> > +drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,
> > + const struct drm_private_state_funcs *funcs)
> > +{
> > +   int index, num_objs, i;
> > +   size_t size;
> > +   struct __drm_private_objs_state *arr;
> > +
> > +   for (i = 0; i < state->num_private_objs; i++)
> > +   if (obj == state->private_objs[i].obj &&
> > +   state->private_objs[i].obj_state)
> > +   return state->private_objs[i].obj_state;
> 
> Comparing this func to drm_atomic_get_plane_state/drm_atomic_get_crtc_state, 
> it
> doesn't seem to call drm_modeset_lock if the obj_state doesn't already exist. 
> I
> don't understand the locking stuff toowell, I just noticed this difference 
> when
> comparing this approach with what is done in the msm kms driver (where we
> have subclassed drm_atomic_state to msm_kms_state).
> 
> Thanks,
> Archit
> 


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

-DK 

> > +
> > +   num_objs = state->num_private_objs + 1;
> > +   size = sizeof(*state->private_objs) * num_objs;
> > +   arr = krealloc(state->private_objs, size, GFP_KERNEL);
> > +   if (!arr)
> > +   return ERR_PTR(-ENOMEM);
> > +
> > +   state->private_objs = arr;
> > +   index = state->num_private_objs;
> > +   memset(>private_objs[index], 0, sizeof(*state->private_objs));
> > +
> > +   state->private_objs[index].obj_state = funcs->duplicate_state(state, 
> > obj);
> > +   if (!state->private_objs[index].obj_state)
> > +   return ERR_PTR(-ENOMEM);
> > +
> > +   state->private_objs[index].obj = 

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

[Pandiyan, Dhinakaran]

On Thu, 2017-02-09 at 08:08 +, Chris Wilson wrote:
> On Wed, Feb 08, 2017 at 10:38:07PM -0800, Dhinakaran Pandiyan wrote:
> > +#define for_each_private_obj(__state, obj_funcs, obj, obj_state, __i, 
> > __funcs) \
> > +   for ((__i) = 0; \
> > +(__i) < (__state)->num_private_objs && \
> > +((obj) = (__state)->private_objs[__i].obj, \
> > +(__funcs) = (__state)->private_objs[__i].funcs,\
> > +(obj_state) = (__state)->private_objs[__i].obj_state, 1);  \
> 
> Align to ( and put the trailing 1 on its own line so it stands out.

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

-DK
> 
>(__i) < (__state)->num_private_objs && \
>((obj) = (__state)->private_objs[__i].obj, \
> (__funcs) = (__state)->private_objs[__i].funcs,   \
> (obj_state) = (__state)->private_objs[__i].obj_state, \
> 1);   \
>(__i)++)   \
> > +   for_each_if (__funcs == obj_funcs)
> > +
> > +/**
> >   * drm_atomic_crtc_needs_modeset - compute combined modeset need
> >   * @state: _crtc_state for the CRTC
> >   *
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chris Wilson]

On Wed, Feb 08, 2017 at 10:38:07PM -0800, Dhinakaran Pandiyan wrote:
> +#define for_each_private_obj(__state, obj_funcs, obj, obj_state, __i, 
> __funcs)   \
> + for ((__i) = 0; \
> +  (__i) < (__state)->num_private_objs && \
> +  ((obj) = (__state)->private_objs[__i].obj, \
> +  (__funcs) = (__state)->private_objs[__i].funcs,\
> +  (obj_state) = (__state)->private_objs[__i].obj_state, 1);  \

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

 (__i) < (__state)->num_private_objs && \
 ((obj) = (__state)->private_objs[__i].obj, \
  (__funcs) = (__state)->private_objs[__i].funcs,   \
  (obj_state) = (__state)->private_objs[__i].obj_state, \
  1);   \
 (__i)++)   \
> + for_each_if (__funcs == obj_funcs)
> +
> +/**
>   * drm_atomic_crtc_needs_modeset - compute combined modeset need
>   * @state: _crtc_state for the CRTC
>   *

-- 
Chris Wilson, Intel Open Source Technology Centre
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel