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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4317 bytes --]

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?

> +/**
> + * 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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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