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

[Pandiyan, Dhinakaran]

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 at ffwll.ch>
> >>>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at 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
> >
>