Re: [PATCH 07/28] drm: Update drm_plane_funcs kerneldoc

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

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

On Fri, Dec 04, 2015 at 09:45:48AM +0100, Daniel Vetter wrote:
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
[...]
> +	/**
> +	 * @destroy:
> +	 *
> +	 * Clean up CRTC resources. This is only called at driver unload time

Perhaps drop "only" because there are more than a single potential
usage?

> +	/**
> +	 * @set_property:
> +	 *
> +	 * This is the legacy entry point to update a property attach to the

"attached to"

> -	/* atomic update handling */
> +	/**
> +	 * @atomic_duplicate_state:
> +	 *
> +	 * Duplicate the current atomic state for this CRTC and return it.
> +	 * The core and helpers gurantee that any atomic state duplicated with
> +	 * this hook and still owned by the caller (i.e. not transferred to the
> +	 * driver by calling ->atomic_commit() from struct
> +	 * &drm_mode_config_funcs) will be cleaned up by calling the
> +	 * @atomic_destroy_state hook in this structure.
> +	 *
> +	 * Atomic drivers which don't subclass struct &drm_crtc should use
> +	 * drm_atomic_helper_crtc_duplicate_state(). Drivers that subclass the
> +	 * state structure to extend it with driver-private state should use
> +	 * __drm_atomic_helper_crtc_duplicate_state() to make sure shared state is
> +	 * duplicated in a consisten fashion across drivers.

"consistent"

> +	/**
> +	 * @atomic_set_property:
> +	 *
> +	 * Decode a driver-private property value and store the decoded value
> +	 * into the passed-in state structure. Since the atomic core decodes all
> +	 * standardized properties (even for extensions beyond the core set of
> +	 * properties which might not be implemented by all drivers) this
> +	 * requires drivers to subclass the state structure.
> +	 *
> +	 * Such driver-private properties should really only implemented for

"be implemented"

> +	 * This function is called in the state assembly phase of atomic
> +	 * modesets, which can be aborted for any reason (including on
> +	 * userspace's request to just check whether a configuration would be
> +	 * possible). Drivers MUST NOT touch any persistent state (hardware or
> +	 * software) or data structures except the passed in adjusted_mode

Copy-paste error? Should be "... the passed in @state parameter."

> +	 *
> +	 * Also since userspace controls in which order properties are set this
> +	 * function must not do any input validation (since the state update is
> +	 * incompletely and hence likely inconsistent). Instead any such input

"incomplete"

> +	 * validation must be done in the various atomic_check callbacks.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * 0 if the property has been found, -EINVAL if the property isn't
> +	 * implemented by the driver (which shouldn't ever happen, the core only

s/shouldn't ever/should never/?

> +	 * asks for properties attached to this CRTC). No other

Seems like you could put more text on the above line.

> +	 * validation is allowed by the driver. The core checks that the
> +	 * property value is within the range (integer, valid enum value, ...)
> +	 * the driver set when registering the property already.

I'd put the "already" after "The core", otherwise it reads weird in my
opinion.

> +	 */
>  	int (*atomic_set_property)(struct drm_crtc *crtc,
>  				   struct drm_crtc_state *state,
>  				   struct drm_property *property,
>  				   uint64_t val);
> +	/**
> +	 * @atomic_get_property:
> +	 *
> +	 * Reads out the decoded driver-private property. This is used to
> +	 * implement the GETCRTC ioctl.
> +	 *
> +	 * Do not call this function directly, use
> +	 * drm_atomic_crtc_get_property() instead.
> +	 *
> +	 * This callback is optional if the driver does not support any
> +	 * driver-private atomic properties.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * 0 on success, -EINVAL if ther property isn't implemented by the

s/ther/the/

> +	 * driver (which shouldn't ever happen, the core only asks for

s/shouldn't ever/should never/?

> +	 * properties attached on this CRTC).

"attached to"

> @@ -539,19 +662,142 @@ struct drm_connector_funcs {
>  	enum drm_connector_status (*detect)(struct drm_connector *connector,
>  					    bool force);
>  	int (*fill_modes)(struct drm_connector *connector, uint32_t max_width, uint32_t max_height);
> +
> +	/**
> +	 * @set_property:
> +	 *
> +	 * This is the legacy entry point to update a property attach to the

"attached to"

> +	/**
> +	 * @destroy:
> +	 *
> +	 * Clean up connector resources. This is only called at driver unload time
> +	 * through drm_mode_config_cleanup(), but can also be called at runtime
> +	 * when a connector is being hot-unplugged.

Again, perhaps drop the "only" because it's inconsistent when followed
by "but can also".

Most of the comments on the CRTC helpers do apply to the connector
helpers as well, so I haven't repeated them.

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