[Intel-gfx] [PATCH 4/5] drm/atomic: document how to handle driver private objects
Alex Deucher
alexdeucher at gmail.com
Fri Dec 15 01:57:48 UTC 2017
On Thu, Dec 14, 2017 at 3:30 PM, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> DK put some nice docs into the commit introducing driver private
> state, but in the git history alone it'll be lost.
>
> Also, since Ville remove the void* usage it's a good opportunity to
> give the driver private stuff some tlc on the doc front.
>
> Finally try to explain why the "let's just subclass drm_atomic_state"
> approach wasn't the greatest, and annotate all those functions as
> deprecated in favour of more standardized driver private states. Also
> note where we could/should extend driver private states going forward
> (atm neither locking nor synchronization is handled in core/helpers,
> which isn't really all that great).
>
> Cc: Harry Wentland <harry.wentland at amd.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Cc: Rob Clark <robdclark at gmail.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: Ben Skeggs <bskeggs at redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
> Documentation/gpu/drm-kms.rst | 6 ++++++
> drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++---
> include/drm/drm_atomic.h | 28 +++++++++++++++++++++++++++
> include/drm/drm_mode_config.h | 9 +++++++++
> 4 files changed, 85 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 307284125d7a..420025bd6a9b 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -271,6 +271,12 @@ Taken all together there's two consequences for the atomic design:
> Read on in this chapter, and also in :ref:`drm_atomic_helper` for more detailed
> coverage of specific topics.
>
> +Handling Driver Private State
> +-----------------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_atomic.c
> + :doc: handling driver private state
> +
> Atomic Mode Setting Function Reference
> --------------------------------------
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 37445d50816a..15e1a35c74a8 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -50,7 +50,8 @@ EXPORT_SYMBOL(__drm_crtc_commit_free);
> * @state: atomic state
> *
> * Free all the memory allocated by drm_atomic_state_init.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * &drm_atomic_state and haven't switched to &drm_private_state yet.
> */
> void drm_atomic_state_default_release(struct drm_atomic_state *state)
> {
> @@ -67,7 +68,8 @@ EXPORT_SYMBOL(drm_atomic_state_default_release);
> * @state: atomic state
> *
> * Default implementation for filling in a new atomic state.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * &drm_atomic_state and haven't switched to &drm_private_state yet.
> */
> int
> drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state)
> @@ -132,7 +134,8 @@ EXPORT_SYMBOL(drm_atomic_state_alloc);
> * @state: atomic state
> *
> * Default implementation for clearing atomic state.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * &drm_atomic_state and haven't switched to &drm_private_state yet.
> */
> void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> {
> @@ -946,6 +949,42 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
> plane->funcs->atomic_print_state(p, state);
> }
>
> +/**
> + * DOC: handling driver private state
> + *
> + * Very often the DRM objects exposed to userspace in the atomic modeset api
> + * (&drm_connector, &drm_crtc and &drm_plane) do not map neatly to the
> + * underlying hardware. Especially for any kind of shared resources (e.g. shared
> + * clocks, scaler units, bandwidth and fifo limits shared among a group of
> + * planes or CRTCs, and so on) it makes sense to model these as independent
> + * objects. Drivers then need to similar state tracking and commit ordering for
> + * such private (since not exposed to userpace) objects as the atomic core and
> + * helpers already provide for connectors, planes and CRTCs.
This last sentence doesn't quite parse. I think it should be as follows:
Drivers then need to do similar state tracking and commit ordering for
such private (since not exposed to userpace) objects as the atomic core that
helpers already provide for DRM objects (connectors, planes and CRTCs).
Feel free to adjust as you see fit. With that fixed up:
Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
> + *
> + * To make this easier on drivers the atomic core provides some support to track
> + * driver private state objects using struct &drm_private_obj, with the
> + * associated state struct &drm_private_state.
> + *
> + * Similar to userspace-exposed objects, state structures can be acquired by
> + * calling drm_atomic_get_private_obj_state(). Since this function does not take
> + * care of locking, drivers should wrap it for each type of private state object
> + * they have with the required call to drm_modeset_lock() for the corresponding
> + * &drm_modeset_lock.
> + *
> + * All private state structures contained in a &drm_atomic_state update can be
> + * iterated using for_each_oldnew_private_obj_in_state(),
> + * for_each_old_private_obj_in_state() and for_each_old_private_obj_in_state().
> + * Drivers are recommend to wrap these for each type of driver private state
> + * object they have, filtering on &drm_private_obj.funcs using for_each_if(), at
> + * least if they want to iterate over all objects of a given type.
> + *
> + * An earlier way to handle driver private state was by subclassing struct
> + * &drm_atomic_state. But since that encourages non-standard ways to implement
> + * the check/commit split atomic requires (by using e.g. "check and rollback or
> + * commit instead" of "duplicate state, check, then either commit or release
> + * duplicated state) it is deprecated in favour of using &drm_private_state.
> + */
> +
> /**
> * drm_atomic_private_obj_init - initialize private object
> * @obj: private object
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 5afd6e364fb6..8e4829a25c1b 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -189,12 +189,40 @@ struct drm_private_state_funcs {
> struct drm_private_state *state);
> };
>
> +/**
> + * struct drm_private_obj - base struct for driver private atomic object
> + *
> + * A driver private object is initialized by calling
> + * drm_atomic_private_obj_init() and cleaned up by calling
> + * drm_atomic_private_obj_fini().
> + *
> + * Currently only tracks the state update functions and the opaque driver
> + * private state itself, but in the future might also track which
> + * &drm_modeset_lock is required to duplicate and update this object's state.
> + */
> struct drm_private_obj {
> + /**
> + * @state: Current atomic state for this driver private object.
> + */
> struct drm_private_state *state;
>
> + /**
> + * @funcs:
> + *
> + * Functions to manipulate the state of this driver private object, see
> + * &drm_private_state_funcs.
> + */
> const struct drm_private_state_funcs *funcs;
> };
>
> +/**
> + * struct drm_private_state - base struct for driver private object state
> + * @state: backpointer to global drm_atomic_state
> + *
> + * Currently only contains a backpointer to the overall atomic upated, but in
> + * the future also might hold synchronization information similar to e.g.
> + * &drm_crtc.commit.
> + */
> struct drm_private_state {
> struct drm_atomic_state *state;
> };
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index c6639e8e5007..2cb6f02df64a 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -269,6 +269,9 @@ struct drm_mode_config_funcs {
> * state easily. If this hook is implemented, drivers must also
> * implement @atomic_state_clear and @atomic_state_free.
> *
> + * Subclassing of &drm_atomic_state is deprecated in favour of using
> + * &drm_private_state and &drm_private_obj.
> + *
> * RETURNS:
> *
> * A new &drm_atomic_state on success or NULL on failure.
> @@ -290,6 +293,9 @@ struct drm_mode_config_funcs {
> *
> * Drivers that implement this must call drm_atomic_state_default_clear()
> * to clear common state.
> + *
> + * Subclassing of &drm_atomic_state is deprecated in favour of using
> + * &drm_private_state and &drm_private_obj.
> */
> void (*atomic_state_clear)(struct drm_atomic_state *state);
>
> @@ -302,6 +308,9 @@ struct drm_mode_config_funcs {
> *
> * Drivers that implement this must call
> * drm_atomic_state_default_release() to release common resources.
> + *
> + * Subclassing of &drm_atomic_state is deprecated in favour of using
> + * &drm_private_state and &drm_private_obj.
> */
> void (*atomic_state_free)(struct drm_atomic_state *state);
> };
> --
> 2.15.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the Intel-gfx
mailing list