Re: [PATCH 15/15] drm: Update kerneldoc for drm_crtc.[hc]

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jan 25, 2017 at 02:26:09PM +0000, Eric Engestrom wrote:
> On Wednesday, 2017-01-25 07:26:57 +0100, Daniel Vetter wrote:
> > After going through all the trouble of splitting out parts from
> > drm_crtc.[hc] and then properly documenting each I've entirely
> > forgotten to show the same TLC for CRTCs themselves!
> > 
> > Let's make amends asap.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  Documentation/gpu/drm-kms.rst |  6 ++++++
> >  drivers/gpu/drm/drm_crtc.c    | 21 +++++++++++++++++++++
> >  include/drm/drm_crtc.h        | 25 +++++++++++++++++++------
> >  3 files changed, 46 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> > index 35c41cf84a1b..979cee853bb1 100644
> > --- a/Documentation/gpu/drm-kms.rst
> > +++ b/Documentation/gpu/drm-kms.rst
> > @@ -284,6 +284,12 @@ Atomic Mode Setting Function Reference
> >  CRTC Abstraction
> >  ================
> >  
> > +.. kernel-doc:: drivers/gpu/drm/drm_crtc.c
> > +   :doc: overview
> > +
> > +CRTC Functions Reference
> > +--------------------------------
> > +
> >  .. kernel-doc:: include/drm/drm_crtc.h
> >     :internal:
> >  
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 5f28e3a5a3e0..6915f897bd8e 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -47,6 +47,27 @@
> >  #include "drm_internal.h"
> >  
> >  /**
> > + * DOC: overview
> > + *
> > + * A CRTC represents the overall display pipeline. It receives pixel data from
> > + * &drm_plane and blends them together. The &drm_display_mode is also attached
> > + * to the CRTC, specifying display timings. On the output side the data is fed
> > + * to one or more &drm_encoder, which are then each connected to one
> > + * &drm_connector.
> > + *
> > + * To create a CRTC, a KMS drivers allocates and zeroes an instances of
> > + * &struct drm_crtc (possibly as part of a larger structure) and registers it
> > + * with a call to drm_crtc_init_with_planes().
> > + *
> > + * The CRTC is also the entry point for legacy modeset operations, see
> > + * &drm_crtc_funcs.set_config, legacy plane operations, see
> > + * &drm_crtc_funcs.page_flip and &drm_crtc_funcs.cursor_set2, and other legacy
> > + * operations like &drm_crtc_funcs.gamma_set. For atomic drivers all these
> > + * features are controlled through &drm_property and
> > + * &drm_mode_config_funcs.atomic_check and &drm_mode_config_funcs.atomic_check.
> > + */
> > +
> > +/**
> >   * drm_crtc_from_index - find the registered CRTC at an index
> >   * @dev: DRM device
> >   * @idx: index of registered CRTC to find for
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 816edab054e6..d788c624f67a 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -641,7 +641,7 @@ struct drm_crtc {
> >  	 *
> >  	 * This provides a read lock for the overall crtc state (mode, dpms
> >  	 * state, ...) and a write lock for everything which can be update
> > -	 * without a full modeset (fb, cursor data, crtc properties ...). Full
> > +	 * without a full modeset (fb, cursor data, crtc properties ...). A full
> >  	 * modeset also need to grab &drm_mode_config.connection_mutex.
> >  	 */
> >  	struct drm_modeset_lock mutex;
> > @@ -774,10 +774,8 @@ struct drm_crtc {
> >   * @connectors: array of connectors to drive with this CRTC if possible
> >   * @num_connectors: size of @connectors array
> >   *
> > - * Represents a single crtc the connectors that it drives with what mode
> > - * and from which framebuffer it scans out from.
> > - *
> > - * This is used to set modes.
> > + * This represents a modeset configuration for the legacy SETCRTC ioctl and is
> > + * also used internally. Atomic drivers instead use &drm_atomic_state.
> >   */
> >  struct drm_mode_set {
> >  	struct drm_framebuffer *fb;
> > @@ -832,7 +830,15 @@ int drm_crtc_force_disable_all(struct drm_device *dev);
> >  int drm_mode_set_config_internal(struct drm_mode_set *set);
> >  struct drm_crtc *drm_crtc_from_index(struct drm_device *dev, int idx);
> >  
> > -/* Helpers */
> > +/**
> > + * drm_crtc_find - look up a CRTC object from its ID
> > + * @dev: DRM device
> > + * @id: &drm_mode_object ID
> > + *
> > + * This can be used to look up a CRTC from its userspace ID. Only used by
> > + * drivers for legacy IOCTLs and interface, nowadays extensions to the KMS
> > + * userspace interface should be done using &drm_property.
> > + */
> >  static inline struct drm_crtc *drm_crtc_find(struct drm_device *dev,
> >  	uint32_t id)
> >  {
> > @@ -841,6 +847,13 @@ static inline struct drm_crtc *drm_crtc_find(struct drm_device *dev,
> >  	return mo ? obj_to_crtc(mo) : NULL;
> >  }
> >  
> > +/**
> > + * drm_for_each_crtc - iterate over all encoders
> 
> s/encoder/crtc/ :)
> 
> Other than that, this and #3 (btw I know you didn't introduce any of the
> typos in #3, but you can fix them :P) are:
> Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>

Typos all fixed and both of these applied, thanks for your review.
-Daniel

> 
> > + * @crtc: a &struct drm_crtc as the loop cursor
> > + * @dev: the &struct drm_device
> > + *
> > + * Iterate over all CRTCs of @dev.
> > + */
> >  #define drm_for_each_crtc(crtc, dev) \
> >  	list_for_each_entry(crtc, &(dev)->mode_config.crtc_list, head)
> >  
> > -- 
> > 2.11.0
> > 

-- 
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