[21/28] drm: Kerneldoc for drm_mode_config_funcs
diff mbox

Message ID 1449218769-16577-22-git-send-email-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter Dec. 4, 2015, 8:46 a.m. UTC
The meat here is definitely the detailed specs for what atomic_check
and atomic_commit are supposed to do.

And another candidate for a core vfunc that should be in a helper really
(output_poll_changed this time around).

v2: Feedback from Eric on irc:
- spelling fixes.
- spec what async should do
- copy the event related paragraphs from page_flip and adjust
- make it clear that a successful async commit is not allowed to leave
  the pipe dead or disabled.

v3: Use FIXME comments to annotate functions that we should move to
some helpers.

Cc: Eric Anholt <eric@anholt.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 include/drm/drm_crtc.h | 241 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 233 insertions(+), 8 deletions(-)

Comments

Thierry Reding Dec. 7, 2015, 1:14 p.m. UTC | #1
On Fri, Dec 04, 2015 at 09:46:02AM +0100, Daniel Vetter wrote:
[...]
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
[...]
>  struct drm_mode_config_funcs {
> +	/**
> +	 * @fb_create:
> +	 *
> +	 * Create a new framebuffer object. The core does basic checks on the
> +	 * requested metadata, but most of that is left to the driver. See
> +	 * struct &drm_mode_fb_cmd2 for details.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * A new framebuffer with an initial refernce count of 1 or a negative

"reference"

> +	 * error code encoded with ERR_PTR().
> +	 */
>  	struct drm_framebuffer *(*fb_create)(struct drm_device *dev,
>  					     struct drm_file *file_priv,
>  					     const struct drm_mode_fb_cmd2 *mode_cmd);
> +
> +	/**
> +	 * @output_poll_changed:
> +	 *
> +	 * Callback used by helpers to inform the driver of output configuration
> +	 * changes.
> +	 *
> +	 * Drivers implementing fbdev emulation with the helpers can call
> +	 * drm_fb_helper_hotplug_changed from this hook to inform the fbdev
> +	 * helper of output changes.
> +	 *
> +	 * FIXME:
> +	 *
> +	 * Except that there's no vtable for device-level helper callbacks
> +	 * there's no reason this is a core function.
> +	 */
>  	void (*output_poll_changed)(struct drm_device *dev);
>  
> +	/**
> +	 * @atomic_check:
> +	 *
> +	 * This is the only hook to validate an atomic modeset update. This
> +	 * function must reject any modeset and state changes which the hardware
> +	 * or driver doesn't support. This includes but is of course not limited
> +	 * to:
> +	 *
> +	 *  - Checking that the modes, framebuffers, scaling and placement
> +	 *    requirements and so on are within the limits of the hardware.
> +	 *
> +	 *  - Checking that any hidden shared resources are not oversubscribed.
> +	 *    This can be shared PLLs, shared lanes, overall memory bandwidth,
> +	 *    display fifo space (where shared between planes or maybe even
> +	 *    CRTCs).
> +	 *
> +	 *  - Checking that virtualized resources exported to userspace are not
> +	 *    oversubscribed. For various reasons it can make sense to expose
> +	 *    more planes, crtcs or encoders than which are physically there. One
> +	 *    example is dual-pipe operations (which generally should be hidden
> +	 *    from userspace if when lockstepped in hardware, otherwise exposed),

"hidden from userspace when lockstepped in hardware, exposed otherwise"?

> +	 *    where a plane might need 1 hardware plane (if it's just on one
> +	 *    pipe), 2 hardware planes (when it spans both pipes) or maybe even
> +	 *    shared a hardware plane with a 2nd plane (if there's a compatible
> +	 *    plane requested on the area handled by the other pipe).
> +	 *
> +	 *  - Check that any transitional state is possible and that if
> +	 *    requested, the update can indeed be done in the vblank period
> +	 *    without temporarily disabling some functions.
> +	 *
> +	 *  - Check any other constraints the driver or hardware might have.
> +	 *
> +	 *  - This callback also needs to correctly fill out the &drm_crtc_state
> +	 *    in this update to make sure that drm_atomic_crtc_needs_modeset()
> +	 *    reflects the nature of the possible update and returns true if and
> +	 *    only if the update cannot be applied without tearing within one
> +	 *    vblank on that CRTC. The core uses that information to reject
> +	 *    updates which require a full modeset (i.e. blanking the screen, or
> +	 *    at least pausing updates for a substantial amount of time) if
> +	 *    userspace has disallowed that in its request.
> +	 *
> +	 *  - The driver also does not need to repeat basic input validation
> +	 *    like done for the corresponding legacy entry points. The core does
> +	 *    that before calling this hook.
> +	 *
> +	 * See the documentation of @atomic_commit for an exhaustive list of
> +	 * error conditions which are allowed to not be checked in this
> +	 * callback.

I think this is confusing. Perhaps "list of error conditions which don't
have to be checked at the ->atomic_check() stage."?

> +	 *
> +	 * See the documentation for struct &drm_atomic_state for how exactly
> +	 * an atomic modeset update is described.
> +	 *
> +	 * Drivers using the atomic helpers can implement this hook using
> +	 * drm_atomic_helper_check(), or one of the exported sub-functions of
> +	 * it.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * 0 on success or one of the below negative error codes:
> +	 *
> +	 *  - -EINVAL, if any of the above constraints are violated.
> +	 *
> +	 *  - -EDEADLK, when returned from an attempt to acquire an additional
> +	 *    &drm_modeset_lock through drm_modeset_lock().
> +	 *
> +	 *  - -ENOMEM, if allocating additional state sub-structures failed due
> +	 *    to lack of memory.
> +	 *
> +	 *  - -EINTR, -EAGAIN or -ERESTARTSYS, if the ioctl should be restarted.
> +	 *    This can either be due to a pending signal, or because the driver
> +	 *    needs to completely bail out to recover from an exceptional
> +	 *    situation like a gpu hang. From a userspace point all errors are
> +	 *    treated equally.
> +	 */
>  	int (*atomic_check)(struct drm_device *dev,
>  			    struct drm_atomic_state *a);
> +
> +	/**
> +	 * @atomic_commit:
> +	 *
> +	 * This is the only hook to commit an atomic modeset update. The core
> +	 * guarantees that @atomic_check has been called successfully before
> +	 * calling this function, and that nothing has been changed in the
> +	 * interim.
> +	 *
> +	 * See the documentation for struct &drm_atomic_state for how exactly
> +	 * an atomic modeset update is described.
> +	 *
> +	 * Drivers using the atomic helpers can implement this hook using
> +	 * drm_atomic_helper_commit(), or one of the exported sub-functions of
> +	 * it.
> +	 *
> +	 * Asynchronous commits (as indicated with the async parameter) must
> +	 * do any preparatory work which might result in an unsuccessful commit
> +	 * in the context of this callback. The only exception is hardware

"The only exception are hardware errors"

> +	 * errors resulting in -EIO. But even in that case the driver must
> +	 * ensure that the display pipe is at least running, to avoid
> +	 * compositors crashing when pageflips don't work. Anything else,
> +	 * specifically committing the update to the hardware, should be done
> +	 * without blocking the caller. For updates which do not require a
> +	 * modeset this must be guaranteed.
> +	 *
> +	 * The driver must wait for any pending rendering to the new
> +	 * framebuffers to complete before executing the flip. It should also
> +	 * wait for any pending rendering from other drivers if the underlying
> +	 * buffer is a shared dma-buf. Asynchronous commits must not wait for
> +	 * rendering in the context of this callback.
> +	 *
> +	 * An application can request to be notified when the atomic commit has
> +	 * completed. These events are per-CRTC and can be distinguished by the
> +	 * CRTC index supplied in &drm_event to userspace.
> +	 *
> +	 * The drm core will supply a struct &drm_event in the event
> +	 * member of each CRTC's &drm_crtc_state structure. This can be handled by the
> +	 * drm_crtc_send_vblank_event() function, which the driver should call on
> +	 * the provided event upon completion of the atomic commit. Note that if
> +	 * the driver supports vblank signalling and timestamping the vblank
> +	 * counters and timestamps must agree with the ones returned from page
> +	 * flip events. With the current vblank helper infrastructure this can
> +	 * be achieved by holding a vblank reference while the page flip is
> +	 * pending, acquired through drm_crtc_vblank_get() and released with
> +	 * drm_crtc_vblank_put(). Drivers are free to implement their own vblank
> +	 * counter and timestamp tracking though, e.g. if they have accurate
> +	 * timestamp registers in hardware.
> +	 *
> +	 * NOTE:
> +	 *
> +	 * Drivers are not allowed to shut down any display pipe successfully
> +	 * enabled through an atomic commit on their own. Doing so can result in
> +	 * compositors crashing if a page flip is suddenly reject because the

"rejected"

> +	 * pipe is off.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * 0 on success or one of the below negative error codes:
> +	 *
> +	 *  - -EBUSY, if an asynchronous updated is requested and there is

"update"

> +	 *    currently an earlier updated still pending. Drivers are allowed to

"update", also I'd drop the "currently" and "still" because they are
somewhat redundant: "... and an earlier update is pending."

> +	 *    support a queue of outstanding updates, but currently no driver
> +	 *    supports that. Note that drivers must wait for preceding updates
> +	 *    to complete if a synchronous update is requested, they are not
> +	 *    allowed to fail the commit in that case.
> +	 *
> +	 *  - -ENOMEM, if the driver failed to allocate memory. Specifically
> +	 *    this can happen when trying to pin framebuffers, which must only
> +	 *    be done when committing the state.
> +	 *
> +	 *  - -ENOSPC, as a refinement of the more generic -ENOMEM to indicate
> +	 *    that the driver has run out of vram, iommu space or similar gpu
> +	 *    address space needed for framebuffer.
> +	 *
> +	 *  - -EIO, if the hardware completely died.
> +	 *
> +	 *  - -EINTR, -EAGAIN or -ERESTARTSYS, if the ioctl should be restarted.

s/ioctl/IOCTL/

> +	 *    This can either be due to a pending signal, or because the driver
> +	 *    needs to completely bail out to recover from an exceptional
> +	 *    situation like a gpu hang. From a userspace point of view all errors are

s/gpu/GPU/

> +	 *    treated equally.
> +	 *
> +	 * This list is exhaustive. Specifically this hook is not allowed to
> +	 * return -EINVAL (any invalid requests should be caught in
> +	 * @atomic_check) or -EDEADLK (this function must not acquire
> +	 * additional modeset locks). The core will also reject any async
> +	 * atomic flips with -EINVAL already (for matching semantics in this
> +	 * case with legacy page flips).

Can you elaborate on this last assertion? Why does the core reject async
atomic flips? I don't see where it does that.

> +	 */
>  	int (*atomic_commit)(struct drm_device *dev,
>  			     struct drm_atomic_state *a,

Why is the state variable called "a" here? Why not "state"? Same for
->atomic_check() above.

> +	/**
> +	 * @atomic_state_alloc:
> +	 *
> +	 * This optional hook can be used by drivers who want to subclass struct

"... drivers that want ..."

> +	 * &drm_atomic_state to be able to track their own driver-private global
> +	 * state easily. If this hook is implemented, drivers must also
> +	 * implement @atomic_state_clear and @atomic_state_free.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * A new &drm_atomic_state on success or NULL on failure.
> +	 */
>  	struct drm_atomic_state *(*atomic_state_alloc)(struct drm_device *dev);
> +
> +	/**
> +	 * @atomic_state_clear:
> +	 *
> +	 * This hook must clear any driver private state duplicated into the
> +	 * passed-in &drm_atomic_state. This hook is called when the caller
> +	 * encountered a &drm_modeset_lock deadlock and needs to drop all
> +	 * already acquired locks as part of the deadlock avoidance dance
> +	 * implemented in drm_modeset_lock_backoff().
> +	 *
> +	 * Any duplicated state must be invalidated since a concurrent atomic
> +	 * update might change it, and the drm atomic interfaces always apply
> +	 * updates as relative changes to the current state.
> +	 *
> +	 * Drivers who implement this must call drm_atomic_state_default_clear()

"Drivers that implement ..."

> +	 * to clear common state.
> +	 */
>  	void (*atomic_state_clear)(struct drm_atomic_state *state);
> +
> +	/**
> +	 * @atomic_state_free:
> +	 *
> +	 * This hook needs driver private resources and the &drm_atomic_state

Did you mean "This hook frees ..."?

> +	 * itself. Note that the core first calls drm_atomic_state_clear to

Parentheses after drm_atomic_state_clear?

Thierry
Daniel Vetter Dec. 7, 2015, 1:34 p.m. UTC | #2
On Mon, Dec 07, 2015 at 02:14:48PM +0100, Thierry Reding wrote:
> On Fri, Dec 04, 2015 at 09:46:02AM +0100, Daniel Vetter wrote:
> > +	 * This list is exhaustive. Specifically this hook is not allowed to
> > +	 * return -EINVAL (any invalid requests should be caught in
> > +	 * @atomic_check) or -EDEADLK (this function must not acquire
> > +	 * additional modeset locks). The core will also reject any async
> > +	 * atomic flips with -EINVAL already (for matching semantics in this
> > +	 * case with legacy page flips).
> 
> Can you elaborate on this last assertion? Why does the core reject async
> atomic flips? I don't see where it does that.

Was an ongoing discussion with Daniels Stone about what exactly we should
do here. Separate patch, not yet resolved so I dropped that sentence for
now - currently core code does indeed not do that.

All other suggestions applied, thanks.
-Daniel

Patch
diff mbox

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 551484fb55e5..37cff64a26ef 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1715,31 +1715,256 @@  struct drm_mode_set {
 
 /**
  * struct drm_mode_config_funcs - basic driver provided mode setting functions
- * @fb_create: create a new framebuffer object
- * @output_poll_changed: function to handle output configuration changes
- * @atomic_check: check whether a given atomic state update is possible
- * @atomic_commit: commit an atomic state update previously verified with
- * 	atomic_check()
- * @atomic_state_alloc: allocate a new atomic state
- * @atomic_state_clear: clear the atomic state
- * @atomic_state_free: free the atomic state
  *
  * Some global (i.e. not per-CRTC, connector, etc) mode setting functions that
  * involve drivers.
  */
 struct drm_mode_config_funcs {
+	/**
+	 * @fb_create:
+	 *
+	 * Create a new framebuffer object. The core does basic checks on the
+	 * requested metadata, but most of that is left to the driver. See
+	 * struct &drm_mode_fb_cmd2 for details.
+	 *
+	 * RETURNS:
+	 *
+	 * A new framebuffer with an initial refernce count of 1 or a negative
+	 * error code encoded with ERR_PTR().
+	 */
 	struct drm_framebuffer *(*fb_create)(struct drm_device *dev,
 					     struct drm_file *file_priv,
 					     const struct drm_mode_fb_cmd2 *mode_cmd);
+
+	/**
+	 * @output_poll_changed:
+	 *
+	 * Callback used by helpers to inform the driver of output configuration
+	 * changes.
+	 *
+	 * Drivers implementing fbdev emulation with the helpers can call
+	 * drm_fb_helper_hotplug_changed from this hook to inform the fbdev
+	 * helper of output changes.
+	 *
+	 * FIXME:
+	 *
+	 * Except that there's no vtable for device-level helper callbacks
+	 * there's no reason this is a core function.
+	 */
 	void (*output_poll_changed)(struct drm_device *dev);
 
+	/**
+	 * @atomic_check:
+	 *
+	 * This is the only hook to validate an atomic modeset update. This
+	 * function must reject any modeset and state changes which the hardware
+	 * or driver doesn't support. This includes but is of course not limited
+	 * to:
+	 *
+	 *  - Checking that the modes, framebuffers, scaling and placement
+	 *    requirements and so on are within the limits of the hardware.
+	 *
+	 *  - Checking that any hidden shared resources are not oversubscribed.
+	 *    This can be shared PLLs, shared lanes, overall memory bandwidth,
+	 *    display fifo space (where shared between planes or maybe even
+	 *    CRTCs).
+	 *
+	 *  - Checking that virtualized resources exported to userspace are not
+	 *    oversubscribed. For various reasons it can make sense to expose
+	 *    more planes, crtcs or encoders than which are physically there. One
+	 *    example is dual-pipe operations (which generally should be hidden
+	 *    from userspace if when lockstepped in hardware, otherwise exposed),
+	 *    where a plane might need 1 hardware plane (if it's just on one
+	 *    pipe), 2 hardware planes (when it spans both pipes) or maybe even
+	 *    shared a hardware plane with a 2nd plane (if there's a compatible
+	 *    plane requested on the area handled by the other pipe).
+	 *
+	 *  - Check that any transitional state is possible and that if
+	 *    requested, the update can indeed be done in the vblank period
+	 *    without temporarily disabling some functions.
+	 *
+	 *  - Check any other constraints the driver or hardware might have.
+	 *
+	 *  - This callback also needs to correctly fill out the &drm_crtc_state
+	 *    in this update to make sure that drm_atomic_crtc_needs_modeset()
+	 *    reflects the nature of the possible update and returns true if and
+	 *    only if the update cannot be applied without tearing within one
+	 *    vblank on that CRTC. The core uses that information to reject
+	 *    updates which require a full modeset (i.e. blanking the screen, or
+	 *    at least pausing updates for a substantial amount of time) if
+	 *    userspace has disallowed that in its request.
+	 *
+	 *  - The driver also does not need to repeat basic input validation
+	 *    like done for the corresponding legacy entry points. The core does
+	 *    that before calling this hook.
+	 *
+	 * See the documentation of @atomic_commit for an exhaustive list of
+	 * error conditions which are allowed to not be checked in this
+	 * callback.
+	 *
+	 * See the documentation for struct &drm_atomic_state for how exactly
+	 * an atomic modeset update is described.
+	 *
+	 * Drivers using the atomic helpers can implement this hook using
+	 * drm_atomic_helper_check(), or one of the exported sub-functions of
+	 * it.
+	 *
+	 * RETURNS:
+	 *
+	 * 0 on success or one of the below negative error codes:
+	 *
+	 *  - -EINVAL, if any of the above constraints are violated.
+	 *
+	 *  - -EDEADLK, when returned from an attempt to acquire an additional
+	 *    &drm_modeset_lock through drm_modeset_lock().
+	 *
+	 *  - -ENOMEM, if allocating additional state sub-structures failed due
+	 *    to lack of memory.
+	 *
+	 *  - -EINTR, -EAGAIN or -ERESTARTSYS, if the ioctl should be restarted.
+	 *    This can either be due to a pending signal, or because the driver
+	 *    needs to completely bail out to recover from an exceptional
+	 *    situation like a gpu hang. From a userspace point all errors are
+	 *    treated equally.
+	 */
 	int (*atomic_check)(struct drm_device *dev,
 			    struct drm_atomic_state *a);
+
+	/**
+	 * @atomic_commit:
+	 *
+	 * This is the only hook to commit an atomic modeset update. The core
+	 * guarantees that @atomic_check has been called successfully before
+	 * calling this function, and that nothing has been changed in the
+	 * interim.
+	 *
+	 * See the documentation for struct &drm_atomic_state for how exactly
+	 * an atomic modeset update is described.
+	 *
+	 * Drivers using the atomic helpers can implement this hook using
+	 * drm_atomic_helper_commit(), or one of the exported sub-functions of
+	 * it.
+	 *
+	 * Asynchronous commits (as indicated with the async parameter) must
+	 * do any preparatory work which might result in an unsuccessful commit
+	 * in the context of this callback. The only exception is hardware
+	 * errors resulting in -EIO. But even in that case the driver must
+	 * ensure that the display pipe is at least running, to avoid
+	 * compositors crashing when pageflips don't work. Anything else,
+	 * specifically committing the update to the hardware, should be done
+	 * without blocking the caller. For updates which do not require a
+	 * modeset this must be guaranteed.
+	 *
+	 * The driver must wait for any pending rendering to the new
+	 * framebuffers to complete before executing the flip. It should also
+	 * wait for any pending rendering from other drivers if the underlying
+	 * buffer is a shared dma-buf. Asynchronous commits must not wait for
+	 * rendering in the context of this callback.
+	 *
+	 * An application can request to be notified when the atomic commit has
+	 * completed. These events are per-CRTC and can be distinguished by the
+	 * CRTC index supplied in &drm_event to userspace.
+	 *
+	 * The drm core will supply a struct &drm_event in the event
+	 * member of each CRTC's &drm_crtc_state structure. This can be handled by the
+	 * drm_crtc_send_vblank_event() function, which the driver should call on
+	 * the provided event upon completion of the atomic commit. Note that if
+	 * the driver supports vblank signalling and timestamping the vblank
+	 * counters and timestamps must agree with the ones returned from page
+	 * flip events. With the current vblank helper infrastructure this can
+	 * be achieved by holding a vblank reference while the page flip is
+	 * pending, acquired through drm_crtc_vblank_get() and released with
+	 * drm_crtc_vblank_put(). Drivers are free to implement their own vblank
+	 * counter and timestamp tracking though, e.g. if they have accurate
+	 * timestamp registers in hardware.
+	 *
+	 * NOTE:
+	 *
+	 * Drivers are not allowed to shut down any display pipe successfully
+	 * enabled through an atomic commit on their own. Doing so can result in
+	 * compositors crashing if a page flip is suddenly reject because the
+	 * pipe is off.
+	 *
+	 * RETURNS:
+	 *
+	 * 0 on success or one of the below negative error codes:
+	 *
+	 *  - -EBUSY, if an asynchronous updated is requested and there is
+	 *    currently an earlier updated still pending. Drivers are allowed to
+	 *    support a queue of outstanding updates, but currently no driver
+	 *    supports that. Note that drivers must wait for preceding updates
+	 *    to complete if a synchronous update is requested, they are not
+	 *    allowed to fail the commit in that case.
+	 *
+	 *  - -ENOMEM, if the driver failed to allocate memory. Specifically
+	 *    this can happen when trying to pin framebuffers, which must only
+	 *    be done when committing the state.
+	 *
+	 *  - -ENOSPC, as a refinement of the more generic -ENOMEM to indicate
+	 *    that the driver has run out of vram, iommu space or similar gpu
+	 *    address space needed for framebuffer.
+	 *
+	 *  - -EIO, if the hardware completely died.
+	 *
+	 *  - -EINTR, -EAGAIN or -ERESTARTSYS, if the ioctl should be restarted.
+	 *    This can either be due to a pending signal, or because the driver
+	 *    needs to completely bail out to recover from an exceptional
+	 *    situation like a gpu hang. From a userspace point of view all errors are
+	 *    treated equally.
+	 *
+	 * This list is exhaustive. Specifically this hook is not allowed to
+	 * return -EINVAL (any invalid requests should be caught in
+	 * @atomic_check) or -EDEADLK (this function must not acquire
+	 * additional modeset locks). The core will also reject any async
+	 * atomic flips with -EINVAL already (for matching semantics in this
+	 * case with legacy page flips).
+	 */
 	int (*atomic_commit)(struct drm_device *dev,
 			     struct drm_atomic_state *a,
 			     bool async);
+
+	/**
+	 * @atomic_state_alloc:
+	 *
+	 * This optional hook can be used by drivers who want to subclass struct
+	 * &drm_atomic_state to be able to track their own driver-private global
+	 * state easily. If this hook is implemented, drivers must also
+	 * implement @atomic_state_clear and @atomic_state_free.
+	 *
+	 * RETURNS:
+	 *
+	 * A new &drm_atomic_state on success or NULL on failure.
+	 */
 	struct drm_atomic_state *(*atomic_state_alloc)(struct drm_device *dev);
+
+	/**
+	 * @atomic_state_clear:
+	 *
+	 * This hook must clear any driver private state duplicated into the
+	 * passed-in &drm_atomic_state. This hook is called when the caller
+	 * encountered a &drm_modeset_lock deadlock and needs to drop all
+	 * already acquired locks as part of the deadlock avoidance dance
+	 * implemented in drm_modeset_lock_backoff().
+	 *
+	 * Any duplicated state must be invalidated since a concurrent atomic
+	 * update might change it, and the drm atomic interfaces always apply
+	 * updates as relative changes to the current state.
+	 *
+	 * Drivers who implement this must call drm_atomic_state_default_clear()
+	 * to clear common state.
+	 */
 	void (*atomic_state_clear)(struct drm_atomic_state *state);
+
+	/**
+	 * @atomic_state_free:
+	 *
+	 * This hook needs driver private resources and the &drm_atomic_state
+	 * itself. Note that the core first calls drm_atomic_state_clear to
+	 * avoid code duplicate between the clear and free hooks.
+	 *
+	 * Drivers who implement this must call drm_atomic_state_default_free()
+	 * to release common resources.
+	 */
 	void (*atomic_state_free)(struct drm_atomic_state *state);
 };