Op 27-03-17 om 10:31 schreef Daniel Vetter:
On Mon, Mar 27, 2017 at 10:28:42AM +0200, Maarten Lankhorst wrote:
Op 27-03-17 om 08:38 schreef Daniel Vetter:
On Wed, Mar 22, 2017 at 03:30:49PM -0700, Dhinakaran Pandiyan wrote:
From: "Pandiyan, Dhinakaran" dhinakaran.pandiyan@intel.com
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.
v4: Avoid redundant NULL checks when private_objs array is empty (Maarten) v3: Macro alignment (Chris) v2: Added docs and new iterator to filter private objects (Daniel)
Acked-by: Harry Wentland harry.wentland@amd.com Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/drm_atomic.c | 69 +++++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 5 ++ include/drm/drm_atomic.h | 93 +++++++++++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 9b892af..e590148 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,21 @@ 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;- }
- state->num_private_objs = 0;
Here we set num_private_objs = 0;
} EXPORT_SYMBOL(drm_atomic_state_default_clear);
@@ -978,6 +994,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);
But here we unconditionally realloc to a presumably smaller size. If you look at drm_atomic_state->num_connector (which also does dynamic array realloc), that one works a bit differently (and hence needs these NULL checks).
I think aligning with how we do things with connectors, for consistency (no other reason really) would be good.
Just noticed this while reading Maarten's review, which seems to go even farther away from how we handle this for connectors. -Daniel
Connectors are handled differently, because there's a fixed number of connectors and each connector is assigned to its slot at state->connectors[drm_connector_index];
For private objects this is not the case, there's no way to put them in a fixed index, so the array is resized and reallocated as needed. If you care about the realloc to a smaller size, add a separate variable max_private_objs and multiply its size by 2 every time it's not big enough. This also changes get_private_obj_state from O(n²) to O(n log(n)).
I don't propose you should though, because N is small enough and the increased complexity isn't worth the decreased readability. So just set num to zero and don't worry about null checks. :)
Hm, in that case shouldn't we also kfree the allocation in default_clear? Makes no sense resetting to 0 and not freeing, when we do an unconditional krealloc afterwards. That's the part that confused me ... I'm not worried about the realloc overahead (and that's easy to fix indeed). -Daniel
We might as well, strictly speaking not needed but probably reduces confusion. :)