Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Daniel,

Thank you for the patch.

On Thursday, 14 December 2017 22:30:53 EET Daniel Vetter wrote:
> DK put some nice docs into the commit introducing driver private
> state, but in the git history alone it'll be lost.

You might want to spell DK's name fully.

> 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@amd.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@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

s/api/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.
> 
> + * 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.

This paragraph could benefit from an explanation of what the corresponding 
drm_modeset_lock is. The rest of the document is pretty clear.

> + * 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().

I think one of those two was meant to be for_each_new_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.

This I still don't agree with. I think it still makes sense to subclass the 
global state object when you have true global state data. How about starting 
by making it a recommendation instead, moving state data related to driver-
specific objects to the new framework, and keeping global data in the 
drm_atomic_state subclass ?

> + */
> +
>  /**
>   * 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);
>  };

-- 
Regards,

Laurent Pinchart

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx