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 daniel.vetter@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
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
The core leaves it to the drivers to choose the private-object types to add. So, I believe the core should do not mandate the use of modeset_locks. MST happens to be one example where connection_mutex is the appropriate lock.
-DK
-DK