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

[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 at 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 at amd.com>
> >> Suggested-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> >> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch