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

[Daniel Vetter]

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