| Message ID | 20171214203054.20141-5-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 14, 2017 at 3:30 PM, Daniel Vetter <daniel.vetter@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@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 > + * (&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@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, 2017-12-14 at 21:30 +0100, 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. > > 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). > I have pointed out a couple of typos below, other than lgtm Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > 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 > + * (&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 ^private > + * 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(). ^for_each_new_private_obj_in_state > + * Drivers are recommend to wrap these for each type of driver private state recommended > + * 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 ^updated > + * 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); > };
Hi Alex, On Friday, 15 December 2017 03:57:48 EET Alex Deucher wrote: > On Thu, Dec 14, 2017 at 3:30 PM, 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. > > > > 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(-) [snip] > > 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 [snip] > > +/** > > + * 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). I think Daniel meant "Drivers then need 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." [snip]
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); > };
On Mon, Dec 18, 2017 at 06:09:20PM +0200, Laurent Pinchart wrote: > Hi Alex, > > On Friday, 15 December 2017 03:57:48 EET Alex Deucher wrote: > > On Thu, Dec 14, 2017 at 3:30 PM, 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. > > > > > > 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(-) > > [snip] > > > > 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 > > [snip] > > > > +/** > > > + * 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). > > I think Daniel meant > > "Drivers then need 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." Already applied before I spotted your reply, but yeah Alex' wasn't entirely correct either, so I picked a mix. Thanks for feedback anyway. -Daniel
On Mon, Dec 18, 2017 at 06:13:46PM +0200, Laurent Pinchart wrote: > 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. Hm yeah ... This is also one of those things I'd like to improve in the private state stuff: If we add a filed for the lock (a pointer, not the lock itself) we could simplify this stuff a lot. > > + * 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(). Fixed. > > + * 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 ? Converting all the existing drivers over is somewhere on my todo. I'm also not really clear what you mean with global data compared to driver-specific objects ... And see the evolution of the i915 state stuff, imo it's a bit too easy to get the drm_atomic_state subclassing wrong. -Daniel
Hi Daniel, On Tuesday, 19 December 2017 12:03:55 EET Daniel Vetter wrote: > On Mon, Dec 18, 2017 at 06:13:46PM +0200, Laurent Pinchart wrote: > > 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(-) [snip] > >> 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 [snip] > >> +/** > >> + * 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. > > Hm yeah ... This is also one of those things I'd like to improve in the > private state stuff: If we add a filed for the lock (a pointer, not the > lock itself) we could simplify this stuff a lot. We don't have to fix everything in one go of course, but a small explanation of what drivers are supposed to do would be helpful. > >> + * 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(). > > Fixed. > > >> + * 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 ? > > Converting all the existing drivers over is somewhere on my todo. I'm also > not really clear what you mean with global data compared to > driver-specific objects ... I'll take an example related to the rcar-du driver. The hardware groups CRTCs by two and share resources (such as planes) between CRTCs in a group. This is something I currently implement in a convoluted way, and using private objects to handle groups (I already have a group object in my driver) will likely help to model the group state. On the other hand, if the hardware didn't have groups but shared planes between all CRTCs, the shared resources would be global, and it would make sense to store them in the global state. > And see the evolution of the i915 state stuff, > imo it's a bit too easy to get the drm_atomic_state subclassing wrong.
On Tue, Dec 19, 2017 at 03:33:57PM +0200, Laurent Pinchart wrote: > On Tuesday, 19 December 2017 12:03:55 EET Daniel Vetter wrote: > > On Mon, Dec 18, 2017 at 06:13:46PM +0200, Laurent Pinchart wrote: > > > On Thursday, 14 December 2017 22:30:53 EET Daniel Vetter wrote: > > >> + * 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 ? > > > > Converting all the existing drivers over is somewhere on my todo. I'm also > > not really clear what you mean with global data compared to > > driver-specific objects ... > > I'll take an example related to the rcar-du driver. The hardware groups CRTCs > by two and share resources (such as planes) between CRTCs in a group. This is > something I currently implement in a convoluted way, and using private objects > to handle groups (I already have a group object in my driver) will likely help > to model the group state. > > On the other hand, if the hardware didn't have groups but shared planes > between all CRTCs, the shared resources would be global, and it would make > sense to store them in the global state. Yeah the private stuff should probably get a hole lot better for singleton objects. I still think one global thing overall (and with a state handling pattern that's different from everything else) is not a good idea. Now it would be fairly easy to generate all the silly boilerplate with a macro, but that tends to wreak havoc with cscope and friends, so I'm not sure it's a great idea really. I'll probably have better ideas once the i915 conversion exists ... -Daniel
Hi Daniel, On Tuesday, 19 December 2017 16:00:36 EET Daniel Vetter wrote: > On Tue, Dec 19, 2017 at 03:33:57PM +0200, Laurent Pinchart wrote: > > On Tuesday, 19 December 2017 12:03:55 EET Daniel Vetter wrote: > > > On Mon, Dec 18, 2017 at 06:13:46PM +0200, Laurent Pinchart wrote: > > > > On Thursday, 14 December 2017 22:30:53 EET Daniel Vetter wrote: > > > >> + * 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 ? > > > > > > Converting all the existing drivers over is somewhere on my todo. I'm > > > also > > > not really clear what you mean with global data compared to > > > driver-specific objects ... > > > > I'll take an example related to the rcar-du driver. The hardware groups > > CRTCs by two and share resources (such as planes) between CRTCs in a > > group. This is something I currently implement in a convoluted way, and > > using private objects to handle groups (I already have a group object in > > my driver) will likely help to model the group state. > > > > On the other hand, if the hardware didn't have groups but shared planes > > between all CRTCs, the shared resources would be global, and it would make > > sense to store them in the global state. > > Yeah the private stuff should probably get a hole lot better for singleton > objects. I still think one global thing overall (and with a state handling > pattern that's different from everything else) is not a good idea. > > Now it would be fairly easy to generate all the silly boilerplate with a > macro, but that tends to wreak havoc with cscope and friends, so I'm not > sure it's a great idea really. I'll probably have better ideas once the > i915 conversion exists ... So how about splitting this in two steps then, first deprecating subclassing drm_atomic_state to store private object state, and only in a second step also deprecating subclassing the structure for global state ?
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. + * + * 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); };
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@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(-)