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@xxxxxxxxx>
>>
>> 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@xxxxxxx>
>> Suggested-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
>> ---
>> 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. :)
~Maarten
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
