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

[Daniel Vetter <daniel@ffwll.ch>]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx