[Intel-gfx] [PATCH 3/6] drm/atomic: Add ->atomic_check() hook for private objects

[Daniel Vetter]

On Tue, Oct 23, 2018 at 07:12:48PM -0400, Lyude Paul wrote:
> Currently; private objects are mostly used just for driver-specific
> atomic state, but not entirely. MST also uses private objects for
> holding it's atomic state, but in order to make our MST helpers safer
> for atomic we need to be able to check that state after the driver has
> performed it's own checks on the atomic state. So, add an optional
> ->atomic_check() callback into drm_private_state_funcs that gets called
> after the driver's atomic checks.
> 
> Signed-off-by: Lyude Paul <lyude at redhat.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>

I think the overall aim here of putting more of the validation into the dp
mst helper makes tons of sense.

I'm not sure whether adding an atomic_check callback to private state
objects makes sense. These can be used for anything, and drivers do use
them for anything really. So it's entirely impossible to have just 1 place
where you call the ->atomic_check for all private objects. This doesn't
work for all the other more standardized objects either, it's guaranteed
to fail for something that's 100% generic, no standard use case.

Also, you're putting helper-level logic (the object based ->atomic_check
callback) into the core, that's midlayer mixing.

Instead I think a better design is to just write something for dp mst.
With the private state macros it's easy to iterate just over all the mst
states (filter for the mst vtable, you could wrap that into a helper macro
with for_each_if even). And then expose one function from mst helpers to
drivers that they're supposed to call from their atomic_check, at
exactly the right spot. I think the overlap of "drivers that want mst" and
"drivers simply enough they don't need their own atomic_check" is zero.

This won't impose any trapdoors on people trying to use private objects to
e.g. manage plane scaler or things like that.

And we could still hook it up by default and e.g. call the
drm_dp_mst_topology_atomic_check() from drm_atomic_helper_check(), maybe
right after calling drm_atomic_helper_check_modeset(). But not from the
same, to give drivers a bit more flexibility in how they handle this
(similar to how the zpos thing works).
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c | 14 ++++++++++++++
>  include/drm/drm_atomic.h     | 16 ++++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 3dbfbddae7e6..2db9f219732b 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -966,6 +966,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_connector *conn;
>  	struct drm_connector_state *conn_state;
> +	struct drm_private_obj *priv_obj;
> +	struct drm_private_state *priv_state;
>  	int i, ret = 0;
>  
>  	DRM_DEBUG_ATOMIC("checking %p\n", state);
> @@ -1007,6 +1009,18 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  		}
>  	}
>  
> +	for_each_new_private_obj_in_state(state, priv_obj, priv_state, i) {
> +		if (!priv_obj->funcs->atomic_check)
> +			continue;
> +
> +		ret = priv_obj->funcs->atomic_check(priv_obj, priv_state);
> +		if (ret) {
> +			DRM_DEBUG_ATOMIC("[PRIVATE:%p] atomic check on state %p failed\n",
> +					 priv_obj, priv_state);
> +			return ret;
> +		}
> +	}
> +
>  	if (!state->allow_modeset) {
>  		for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>  			if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index f9b35834c45d..3e504eeb1122 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -216,6 +216,22 @@ struct drm_private_state_funcs {
>  	 */
>  	void (*atomic_destroy_state)(struct drm_private_obj *obj,
>  				     struct drm_private_state *state);
> +
> +	/**
> +	 * @atomic_check:
> +	 *
> +	 * Perform a check of the current state of the private object to
> +	 * ensure that it's valid. This is an optional callback. If
> +	 * implemented, it will be called after atomic checks have been
> +	 * performed on all of the planes, CRTCs, connectors, and the new
> +	 * &drm_mode_config in the atomic state.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * 0 on success, negative error code on failure.
> +	 */
> +	int (*atomic_check)(struct drm_private_obj *obj,
> +			    struct drm_private_state *state);
>  };
>  
>  /**
> -- 
> 2.17.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch