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.wentl...@amd.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com> > Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com> > Cc: Ville Syrjälä <ville.syrj...@linux.intel.com> > Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com> > Cc: Rob Clark <robdcl...@gmail.com> > Cc: Alex Deucher <alexander.deuc...@amd.com> > Cc: Ben Skeggs <bske...@redhat.com> > Signed-off-by: Daniel Vetter <daniel.vet...@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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel