On Mon, 2017-03-27 at 10:35 +0200, Maarten Lankhorst wrote:
> 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.pandi...@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.wentl...@amd.com>
> >>>> Suggested-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> >>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@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. :)
>
kfree'ing the state->private_objs array in
drm_atomic_state_default_clear() does not seem consistent with what we
do for crtcs, planes and connectors. default_release() seems to be the
function that frees memory for state arrays. We could just go with v4,
where state->num_private_objs is not reset. As 'n' is small, the NULL
checks shouldn't be costly. I can look at optimizing realloc's once we
have more consumers for private_objs?
-DK
> _______________________________________________
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
