Re: [Intel-gfx] [PATCH v3 4/8] drm: Add driver-private objects to atomic state
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(&state->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 > b/drivers/gpu/drm/drm_atomic_helper.c > index 3a4383f..8795088 100644
Re: [Intel-gfx] [PATCH v3 4/8] drm: Add driver-private objects to atomic state
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(&dev->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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 4/8] drm: Add driver-private objects to atomic state
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(&dev->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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 4/8] drm: Add driver-private objects to atomic state
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 > > >>> > >>> > >>> The caller is expected to take ca
Re: [Intel-gfx] [PATCH v3 4/8] drm: Add driver-private objects to atomic state
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(&dev->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 &drm_modeset_lock responsible for protecting the private object state" Thanks, Archit -DK -- Qualcomm In
Re: [Intel-gfx] [PATCH v3 4/8] drm: Add driver-private objects to atomic state
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 com
Re: [Intel-gfx] [PATCH v3 4/8] drm: Add driver-private objects to atomic state
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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 4/8] drm: Add driver-private objects to atomic state
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(&state->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 = func
Re: [Intel-gfx] [PATCH v3 4/8] drm: Add driver-private objects to atomic state
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 + + 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(&state->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 b/drivers/gpu/drm/drm_atomic_helper.c index 3a4383f..879508
Re: [Intel-gfx] [PATCH v3 4/8] drm: Add driver-private objects to atomic state
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: &drm_crtc_state for the CRTC > > * > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 4/8] drm: Add driver-private objects to atomic state
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: &drm_crtc_state for the CRTC > * -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3 4/8] drm: Add driver-private objects to atomic state
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(&state->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 b/drivers/gpu/drm/drm_atomic_helper.c index 3a4383f..8795088 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1983,6 +1983,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, struct drm_plane *plane; struct drm_plane_state *plane_state; struct drm_crtc_commit *commit; + void *obj, *obj_state; + const struct drm_private_state_funcs *funcs; if (stall) { for_each_crtc_in_s