Re: [PATCH v5 01/11] drm: Add atomic variants of enable/disable to encoder helper funcs

[Sam Ravnborg <sam@ravnborg.org>]

Hi Sean.

Nits below.

>  
> +	/**
> +	 * @atomic_disable:
> +	 *
...
> +	 *
> +	 * This callback is a variant of @disable that provides the atomic state
> +	 * to the driver. It takes priority over @disable during atomic commits.
> +	 *
> +	 * This hook is used only by atomic helpers. Atomic drivers don't need
> +	 * to implement it if there's no need to disable anything at the encoder
> +	 * level. To ensure that runtime PM handling (using either DPMS or the
> +	 * new "ACTIVE" property) works @atomic_disable must be the inverse of
> +	 * @atomic_enable.
> +	 */
> +	void (*atomic_disable)(struct drm_encoder *encoder,
> +			       struct drm_atomic_state *state);



> +
> +	/**
> +	 * @atomic_enable:
> +	 *
...
> +	 *
> +	 * This callback is a variant of @enable that provides the atomic state
> +	 * to the driver. It is called in place of @enable during atomic
> +	 * commits.

The wording in this paragrap is not the same as the similar paragraph
above.

One says "it takes priority over"
Another says "called in place of"

Maybe be a bit more explicit and say that "if atomic_{dis,en}able is
define then {dis,en}able is not called?


> +	 *
> +	 * This hook is used only by atomic helpers, for symmetry with @disable.
I do not get the "for symmetry with @disable."?

> +	 * Atomic drivers don't need to implement it if there's no need to
> +	 * enable anything at the encoder level. To ensure that runtime PM
> +	 * handling (using either DPMS or the new "ACTIVE" property) works
> +	 * @enable must be the inverse of @disable for atomic drivers.
Did you want to say "@atomic_enable must be the inverse of @atomic_disable for atomic drivers."?

> +	 */
> +	void (*atomic_enable)(struct drm_encoder *encoder,
> +			      struct drm_atomic_state *state);
> +
>  	/**
>  	 * @disable:
>  	 *
> @@ -695,6 +740,9 @@ struct drm_encoder_helper_funcs {
>  	 * handling (using either DPMS or the new "ACTIVE" property) works
>  	 * @disable must be the inverse of @enable for atomic drivers.
>  	 *
> +	 * For atomic drivers also consider @atomic_disable and save yourself
> +	 * from having to read the NOTE below!
Maybe, if this is so, say that atomic drivers shall alwyas use the
atomic_* variants?
And then add a TODO entry to make it so for the other atomic drivers?
> +	 *
>  	 * NOTE:
>  	 *
>  	 * With legacy CRTC helpers there's a big semantic difference between


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