Re: [Intel-gfx] [PATCH 01/15] drm/kms-helpers: Use recommened kerneldoc for struct member refs

[Gustavo Padovan <gustavo@padovan.org>]

Hi Daniel,

2017-01-25 Daniel Vetter <daniel.vetter@ffwll.ch>:

> I just learned that &struct_name.member_name works and looks pretty
> even. It doesn't (yet) link to the member directly though, which would
> be really good for big structures or vfunc tables (where the
> per-member kerneldoc tends to be long).
> 
> Also some minor drive-by polish where it makes sense, I read a lot
> of docs ...
> 
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c      |  97 ++++++++++----------
>  drivers/gpu/drm/drm_crtc_helper.c        |  28 +++---
>  drivers/gpu/drm/drm_dp_helper.c          |   2 +-
>  drivers/gpu/drm/drm_fb_helper.c          |  48 +++++-----
>  drivers/gpu/drm/drm_plane_helper.c       |   9 +-
>  drivers/gpu/drm/drm_probe_helper.c       |  14 +--
>  include/drm/drm_atomic_helper.h          |  13 +--
>  include/drm/drm_dp_mst_helper.h          |   7 +-
>  include/drm/drm_flip_work.h              |   2 +-
>  include/drm/drm_modeset_helper_vtables.h | 146 ++++++++++++++++---------------
>  include/drm/drm_simple_kms_helper.h      |  12 +--
>  11 files changed, 197 insertions(+), 181 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 1f0cd7e715c9..5e512dd3a2c4 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -458,22 +458,25 @@ mode_fixup(struct drm_atomic_state *state)
>   * Check the state object to see if the requested state is physically possible.
>   * This does all the crtc and connector related computations for an atomic
>   * update and adds any additional connectors needed for full modesets and calls
> - * down into ->mode_fixup functions of the driver backend.
> - *
> - * crtc_state->mode_changed is set when the input mode is changed.
> - * crtc_state->connectors_changed is set when a connector is added or
> - * removed from the crtc.
> - * crtc_state->active_changed is set when crtc_state->active changes,
> - * which is used for dpms.
> + * down into &drm_crtc_helper_funcs.mode_fixup and
> + * &drm_encoder_helper_funcs.mode_fixup or
> + * &drm_encoder_helper_funcs.atomic_check functions of the driver backend.
> + *
> + * &drm_crtc_state.mode_changed is set when the input mode is changed.
> + * &drm_crtc_state.connectors_changed is set when a connector is added or
> + * removed from the crtc.  &drm_crtc_state.active_changed is set when
> + * &drm_crtc_state.active changes, which is used for DPMS.
>   * See also: drm_atomic_crtc_needs_modeset()
>   *
>   * IMPORTANT:
>   *
> - * Drivers which set ->mode_changed (e.g. in their ->atomic_check hooks if a
> - * plane update can't be done without a full modeset) _must_ call this function
> - * afterwards after that change. It is permitted to call this function multiple
> - * times for the same update, e.g. when the ->atomic_check functions depend upon
> - * the adjusted dotclock for fifo space allocation and watermark computation.
> + * Drivers which set &drm_crtc_state.mode_changed (e.g. in their
> + * &drm_plane_helper_funcs.atomic_check hooks if a plane update can't be done
> + * without a full modeset) _must_ call this function afterwards after that
> + * change. It is permitted to call this function multiple times for the same
> + * update, e.g. when the &drm_crtc_helper_funcs.atomic_check functions depend
> + * upon the adjusted dotclock for fifo space allocation and watermark
> + * computation.
>   *
>   * RETURNS:
>   * Zero for success or -errno
> @@ -584,9 +587,10 @@ EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
>   *
>   * Check the state object to see if the requested state is physically possible.
>   * This does all the plane update related checks using by calling into the
> - * ->atomic_check hooks provided by the driver.
> + * &drm_crtc_helper_funcs.atomic_check and &drm_plane_helper_funcs.atomic_check
> + * hooks provided by the driver.
>   *
> - * It also sets crtc_state->planes_changed to indicate that a crtc has
> + * It also sets &drm_crtc_state.planes_changed to indicate that a crtc has
>   * updated planes.
>   *
>   * RETURNS:
> @@ -648,14 +652,15 @@ EXPORT_SYMBOL(drm_atomic_helper_check_planes);
>   * Check the state object to see if the requested state is physically possible.
>   * Only crtcs and planes have check callbacks, so for any additional (global)
>   * checking that a driver needs it can simply wrap that around this function.
> - * Drivers without such needs can directly use this as their ->atomic_check()
> - * callback.
> + * Drivers without such needs can directly use this as their
> + * &drm_mode_config_funcs.atomic_check callback.
>   *
>   * This just wraps the two parts of the state checking for planes and modeset
>   * state in the default order: First it calls drm_atomic_helper_check_modeset()
>   * and then drm_atomic_helper_check_planes(). The assumption is that the
> - * ->atomic_check functions depend upon an updated adjusted_mode.clock to
> - * e.g. properly compute watermarks.
> + * @drm_plane_helper_funcs.atomic_check and @drm_crtc_helper_funcs.atomic_check
> + * functions depend upon an updated adjusted_mode.clock to e.g. properly compute
> + * watermarks.
>   *
>   * RETURNS:
>   * Zero for success or -errno
> @@ -1125,8 +1130,8 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
>   * drm_atomic_helper_commit_tail - commit atomic update to hardware
>   * @old_state: atomic state object with old state structures
>   *
> - * This is the default implemenation for the ->atomic_commit_tail() hook of the
> - * &drm_mode_config_helper_funcs vtable.
> + * This is the default implemenation for the
> + * &drm_mode_config_helper_funcs.atomic_commit_tail hook.

implementation

>   *
>   * Note that the default ordering of how the various stages are called is to
>   * match the legacy modeset helper library closest. One peculiarity of that is
> @@ -1203,8 +1208,8 @@ static void commit_work(struct work_struct *work)
>   * drm_atomic_helper_setup_commit() and related functions.
>   *
>   * Committing the actual hardware state is done through the
> - * ->atomic_commit_tail() callback of the &drm_mode_config_helper_funcs vtable,
> - * or it's default implementation drm_atomic_helper_commit_tail().
> + * &drm_mode_config_helper_funcs.atomic_commit_tail callback, or it's default
> + * implementation drm_atomic_helper_commit_tail().
>   *
>   * RETURNS:
>   * Zero for success or -errno.
> @@ -1373,14 +1378,15 @@ static void release_crtc_commit(struct completion *completion)
>   *
>   * This function prepares @state to be used by the atomic helper's support for
>   * nonblocking commits. Drivers using the nonblocking commit infrastructure
> - * should always call this function from their ->atomic_commit hook.
> + * should always call this function from their
> + * &drm_mode_config_funcs.atomic_commit hook.
>   *
>   * To be able to use this support drivers need to use a few more helper
>   * functions. drm_atomic_helper_wait_for_dependencies() must be called before
>   * actually committing the hardware state, and for nonblocking commits this call
>   * must be placed in the async worker. See also drm_atomic_helper_swap_state()
>   * and it's stall parameter, for when a driver's commit hooks look at the
> - * ->state pointers of &struct drm_crtc, &drm_plane or &drm_connector directly.
> + * &drm_crtc.state, &drm_plane.state or &drm_connector.state pointer directly.
>   *
>   * Completion of the hardware commit step must be signalled using
>   * drm_atomic_helper_commit_hw_done(). After this step the driver is not allowed
> @@ -1489,8 +1495,7 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
>   * This function waits for all preceeding commits that touch the same CRTC as
>   * @old_state to both be committed to the hardware (as signalled by
>   * drm_atomic_helper_commit_hw_done) and executed by the hardware (as signalled
> - * by calling drm_crtc_vblank_send_event on the event member of
> - * &drm_crtc_state).
> + * by calling drm_crtc_vblank_send_event on the &drm_crtc_state.event).

drm_crtc_vblank_send_event()

>   *
>   * This is part of the atomic helper support for nonblocking commits, see
>   * drm_atomic_helper_setup_commit() for an overview.
> @@ -1627,8 +1632,9 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done);
>   * @state: atomic state object with new state structures
>   *
>   * This function prepares plane state, specifically framebuffers, for the new
> - * configuration. If any failure is encountered this function will call
> - * ->cleanup_fb on any already successfully prepared framebuffer.
> + * configuration, by calling &drm_plane_helper_funcs.prepare_fb. If any failure
> + * is encountered this function will call &drm_plane_helper_funcs.cleanup_fb on
> + * any already successfully prepared framebuffer.
>   *
>   * Returns:
>   * 0 on success, negative error code on failure.
> @@ -1708,10 +1714,10 @@ static bool plane_crtc_active(const struct drm_plane_state *state)
>   *
>   * Drivers may set the NO_DISABLE_AFTER_MODESET flag in @flags if the relevant
>   * display controllers require to disable a CRTC's planes when the CRTC is
> - * disabled. This function would skip the ->atomic_disable call for a plane if
> - * the CRTC of the old plane state needs a modesetting operation. Of course,
> - * the drivers need to disable the planes in their CRTC disable callbacks
> - * since no one else would do that.
> + * disabled. This function would skip the &drm_plane_helper_funcs.atomic_disable
> + * call for a plane if the CRTC of the old plane state needs a modesetting
> + * operation. Of course, the drivers need to disable the planes in their CRTC
> + * disable callbacks since no one else would do that.
>   *
>   * The drm_atomic_helper_commit() default implementation doesn't set the
>   * ACTIVE_ONLY flag to most closely match the behaviour of the legacy helpers.
> @@ -1874,7 +1880,7 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_planes_on_crtc);
>   * planes.
>   *
>   * It is a bug to call this function without having implemented the
> - * ->atomic_disable() plane hook.
> + * &drm_plane_helper_funcs.atomic_disable plane hook.
>   */
>  void
>  drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc_state *old_crtc_state,
> @@ -1961,8 +1967,8 @@ EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes);
>   * contains the old state. Also do any other cleanup required with that state.
>   *
>   * @stall must be set when nonblocking commits for this driver directly access
> - * the ->state pointer of &drm_plane, &drm_crtc or &drm_connector. With the
> - * current atomic helpers this is almost always the case, since the helpers
> + * the &drm_plane.state, &drm_crtc.state or &drm_connector.state pointer. With
> + * the current atomic helpers this is almost always the case, since the helpers
>   * don't pass the right state structures to the callbacks.
>   */
>  void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> @@ -2892,8 +2898,8 @@ EXPORT_SYMBOL(drm_atomic_helper_page_flip_target);
>   *
>   * This is the main helper function provided by the atomic helper framework for
>   * implementing the legacy DPMS connector interface. It computes the new desired
> - * ->active state for the corresponding CRTC (if the connector is enabled) and
> - * updates it.
> + * &drm_crtc_state.active state for the corresponding CRTC (if the connector is
> + * enabled) and updates it.
>   *
>   * Returns:
>   * Returns 0 on success, negative errno numbers on failure.
> @@ -2965,11 +2971,11 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
>  EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
>  
>  /**
> - * drm_atomic_helper_best_encoder - Helper for &drm_connector_helper_funcs
> - *                                  ->best_encoder callback
> + * drm_atomic_helper_best_encoder - Helper for
> + * 	&drm_connector_helper_funcs.best_encoder callback
>   * @connector: Connector control structure
>   *
> - * This is a &drm_connector_helper_funcs ->best_encoder callback helper for
> + * This is a &drm_connector_helper_funcs.best_encoder callback helper for
>   * connectors that support exactly 1 encoder, statically determined at driver
>   * init time.
>   */
> @@ -3003,7 +3009,7 @@ EXPORT_SYMBOL(drm_atomic_helper_best_encoder);
>   */
>  
>  /**
> - * drm_atomic_helper_crtc_reset - default ->reset hook for CRTCs
> + * drm_atomic_helper_crtc_reset - default &drm_crtc_funcs.reset hook for CRTCs
>   * @crtc: drm CRTC
>   *
>   * Resets the atomic state for @crtc by freeing the state pointer (which might
> @@ -3110,7 +3116,7 @@ void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
>  EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
>  
>  /**
> - * drm_atomic_helper_plane_reset - default ->reset hook for planes
> + * drm_atomic_helper_plane_reset - default &drm_plane_funcs.reset hook for planes
>   * @plane: drm plane
>   *
>   * Resets the atomic state for @plane by freeing the state pointer (which might
> @@ -3214,8 +3220,9 @@ EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state);
>   * @conn_state: connector state to assign
>   *
>   * Initializes the newly allocated @conn_state and assigns it to
> - * #connector ->state, usually required when initializing the drivers
> - * or when called from the ->reset hook.
> + * the &drm_conector->state pointer of @connector, usually required when
> + * initializing the drivers or when called from the &drm_connector_funcs.reset
> + * hook.
>   *
>   * This is useful for drivers that subclass the connector state.
>   */
> @@ -3231,7 +3238,7 @@ __drm_atomic_helper_connector_reset(struct drm_connector *connector,
>  EXPORT_SYMBOL(__drm_atomic_helper_connector_reset);
>  
>  /**
> - * drm_atomic_helper_connector_reset - default ->reset hook for connectors
> + * drm_atomic_helper_connector_reset - default &drm_connector_funcs.reset hook for connectors
>   * @connector: drm connector
>   *
>   * Resets the atomic state for @connector by freeing the state pointer (which
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 1e281dd42e4b..8c1e4d93a4f4 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -53,9 +53,9 @@
>   * configuration on resume with drm_helper_resume_force_mode().
>   *
>   * Note that this helper library doesn't track the current power state of CRTCs
> - * and encoders. It can call callbacks like ->dpms() even though the hardware is
> - * already in the desired state. This deficiency has been fixed in the atomic
> - * helpers.
> + * and encoders. It can call callbacks like &drm_encoder_helper_funcs.dpms even
> + * though the hardware is already in the desired state. This deficiency has been
> + * fixed in the atomic helpers.
>   *
>   * The driver callbacks are mostly compatible with the atomic modeset helpers,
>   * except for the handling of the primary plane: Atomic helpers require that the
> @@ -477,12 +477,12 @@ drm_crtc_helper_disable(struct drm_crtc *crtc)
>   * drm_crtc_helper_set_config - set a new config from userspace
>   * @set: mode set configuration
>   *
> - * The drm_crtc_helper_set_config() helper function implements the set_config
> - * callback of &struct drm_crtc_funcs for drivers using the legacy CRTC helpers.
> + * The drm_crtc_helper_set_config() helper function implements the of
> + * &drm_crtc_funcs.set_config callback for drivers using the legacy CRTC
> + * helpers.
>   *
>   * It first tries to locate the best encoder for each connector by calling the
> - * connector ->best_encoder() (&struct drm_connector_helper_funcs) helper
> - * operation.
> + * connector @drm_connector_helper_funcs.best_encoder helper operation.
>   *
>   * After locating the appropriate encoders, the helper function will call the
>   * mode_fixup encoder and CRTC helper operations to adjust the requested mode,
> @@ -493,8 +493,7 @@ drm_crtc_helper_disable(struct drm_crtc *crtc)
>   *
>   * If the adjusted mode is identical to the current mode but changes to the
>   * frame buffer need to be applied, the drm_crtc_helper_set_config() function
> - * will call the CRTC ->mode_set_base() (&struct drm_crtc_helper_funcs) helper
> - * operation.
> + * will call the CRTC &drm_crtc_helper_funcs.mode_set_base helper operation.
>   *
>   * If the adjusted mode differs from the current mode, or if the
>   * ->mode_set_base() helper operation is not provided, the helper function
> @@ -851,14 +850,15 @@ static int drm_helper_choose_crtc_dpms(struct drm_crtc *crtc)
>   * @connector: affected connector
>   * @mode: DPMS mode
>   *
> - * The drm_helper_connector_dpms() helper function implements the ->dpms()
> - * callback of &struct drm_connector_funcs for drivers using the legacy CRTC helpers.
> + * The drm_helper_connector_dpms() helper function implements the
> + * &drm_connector_funcs.dpms() callback for drivers using the legacy CRTC
> + * helpers.

You didn't use () in any of the others.

Otherwise looks good to me:

Rewiewed-by: Gustavo Padovan <gustavo.padovan@collabora.com>

Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel