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

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

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.

-- 
Regards,

Laurent Pinchart

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