Re: [PATCH 03/28] drm: Reorganize helper vtables and their docs

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Dec 07, 2015 at 12:00:09PM +0100, Thierry Reding wrote:
> On Fri, Dec 04, 2015 at 09:45:44AM +0100, Daniel Vetter wrote:
> [...]
> > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> > index 10d0989db273..077e48d3cac2 100644
> > --- a/drivers/gpu/drm/drm_crtc_helper.c
> > +++ b/drivers/gpu/drm/drm_crtc_helper.c
> > @@ -62,6 +62,11 @@
> >   * converting to the plane helpers). New drivers must not use these functions
> >   * but need to implement the atomic interface instead, potentially using the
> >   * atomic helpers for that.
> > + *
> > + * The these legacy modeset helpers use the same function table structures as
> 
> s/The these/The/
> 
> > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > new file mode 100644
> > index 000000000000..35c5a1c4e7ba
> > --- /dev/null
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -0,0 +1,252 @@
> > +/*
> > + * Copyright © 2015 Intel Corporation
> > + *   Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Perhaps inherit the copyright statements from the includes that you
> extracted these from?

Done for the above two - all the stuff below is just moved and would
conflict massively with later patches. So left that as per our irc
discussion.
-Daniel

> 
> > +/**
> > + * DOC: overview
> > + *
> > + * The DRM mode setting helper functions are common code for drivers to use if
> > + * they wish.  Drivers are not forced to use this code in their
> > + * implementations but it would be useful if the code they do use at least
> > + * provides a consistent interface and operation to userspace. Therefore it is
> > + * highly recommended to use the provided helpers as much as possible.
> > + *
> > + * Because there is only one pointer per modeset object to hold a vfunc table
> > + * for helper libraries they are by necessity shared among the different
> > + * helpers.
> > + *
> > + * To make this clear all the helper vtables are pulled together in this location here.
> 
> Perhaps drop the "here" at the end of that sentence. Also maybe wrap the
> last line because it stands out as much longer than the above.
> 
> > + */
> > +
> > +enum mode_set_atomic;
> > +
> > +/**
> > + * struct drm_crtc_helper_funcs - helper operations for CRTCs
> > + * @dpms: set power state
> > + * @prepare: prepare the CRTC, called before @mode_set
> > + * @commit: commit changes to CRTC, called after @mode_set
> > + * @mode_fixup: try to fixup proposed mode for this CRTC
> > + * @mode_set: set this mode
> > + * @mode_set_nofb: set mode only (no scanout buffer attached)
> > + * @mode_set_base: update the scanout buffer
> > + * @mode_set_base_atomic: non-blocking mode set (used for kgdb support)
> > + * @load_lut: load color palette
> > + * @disable: disable CRTC when no longer in use
> > + * @enable: enable CRTC
> > + * @atomic_check: check for validity of an atomic state
> > + * @atomic_begin: begin atomic update
> > + * @atomic_flush: flush atomic update
> > + *
> > + * The helper operations are called by the mid-layer CRTC helper.
> > + *
> > + * Note that with atomic helpers @dpms, @prepare and @commit hooks are
> > + * deprecated. Used @enable and @disable instead exclusively.
> 
> I /think/ it would be more correct to say: "Use @enable and @disable
> exclusively instead."
> 
> > + *
> > + * With legacy crtc helpers there's a big semantic difference between @disable
> 
> s/crtc/CRTC/, there's a couple more places where the casing is
> inconsistent, I'll refrain from pointing them out explicitly since your
> editor will be much better at finding them.
> 
> > +/**
> > + * struct drm_encoder_helper_funcs - helper operations for encoders
> > + * @dpms: set power state
> > + * @save: save connector state
> > + * @restore: restore connector state
> > + * @mode_fixup: try to fixup proposed mode for this connector
> > + * @prepare: part of the disable sequence, called before the CRTC modeset
> > + * @commit: called after the CRTC modeset
> > + * @mode_set: set this mode, optional for atomic helpers
> > + * @get_crtc: return CRTC that the encoder is currently attached to
> > + * @detect: connection status detection
> > + * @disable: disable encoder when not in use (overrides DPMS off)
> > + * @enable: enable encoder
> > + * @atomic_check: check for validity of an atomic update
> > + *
> > + * The helper operations are called by the mid-layer CRTC helper.
> > + *
> > + * Note that with atomic helpers @dpms, @prepare and @commit hooks are
> > + * deprecated. Used @enable and @disable instead exclusively.
> 
> Same comment as for the CRTC helper functions.
> 
> Thierry



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx