Re: [PATCH v1 2/7] drm: add hook to print encoder status

[Philippe CORNU <philippe.cornu@st.com>]

Hi Benjamin,

On 06/05/2018 03:54 PM, Benjamin Gaignard wrote:
> Even if encoders don't have state it could be useful to get information
> from them when dumping of the other elements state.
> Add an optional hook in drm_encoder_funcs structure and call it after crtc
> print state.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> ---
>   drivers/gpu/drm/drm_atomic.c | 15 +++++++++++++++
>   include/drm/drm_encoder.h    | 12 ++++++++++++
>   2 files changed, 27 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index cd1d677617c8..6a9f5be01172 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -28,6 +28,7 @@
>   
>   #include <drm/drmP.h>
>   #include <drm/drm_atomic.h>
> +#include <drm/drm_encoder.h>
>   #include <drm/drm_mode.h>
>   #include <drm/drm_print.h>
>   #include <linux/sync_file.h>
> @@ -1799,6 +1800,15 @@ int drm_atomic_nonblocking_commit(struct drm_atomic_state *state)
>   }
>   EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
>   
> +static void drm_atomic_encoder_print(struct drm_printer *p,
> +				     struct drm_encoder *encoder)
> +{
> +	drm_printf(p, "encoder[%u]: %s\n", encoder->base.id, encoder->name);
> +
> +	if (encoder->funcs->atomic_print)
> +		encoder->funcs->atomic_print(p, encoder);
> +}
> +
>   static void drm_atomic_print_state(const struct drm_atomic_state *state)
>   {
>   	struct drm_printer p = drm_info_printer(state->dev->dev);
> @@ -1828,6 +1838,7 @@ static void __drm_state_dump(struct drm_device *dev, struct drm_printer *p,
>   	struct drm_mode_config *config = &dev->mode_config;
>   	struct drm_plane *plane;
>   	struct drm_crtc *crtc;
> +	struct drm_encoder *encoder;
>   	struct drm_connector *connector;
>   	struct drm_connector_list_iter conn_iter;
>   
> @@ -1850,6 +1861,10 @@ static void __drm_state_dump(struct drm_device *dev, struct drm_printer *p,
>   			drm_modeset_unlock(&crtc->mutex);
>   	}
>   
> +	drm_for_each_encoder(encoder, dev) {
> +		drm_atomic_encoder_print(p, encoder);
> +	}
> +
>   	drm_connector_list_iter_begin(dev, &conn_iter);
>   	if (take_locks)
>   		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> index fb299696c7c4..b847dad817b0 100644
> --- a/include/drm/drm_encoder.h
> +++ b/include/drm/drm_encoder.h
> @@ -80,6 +80,18 @@ struct drm_encoder_funcs {
>   	 * before data structures are torndown.
>   	 */
>   	void (*early_unregister)(struct drm_encoder *encoder);
> +
> +	/**
> +	 * @atomic_print
> +	 *
> +	 * If driver could implement this optional hook for printing
> +	 * additional driver specific information.
> +	 *
> +	 * Do not call this directly, use drm_atomic_encoder_print()
> +	 * instead.
> +	 */
> +	void (*atomic_print)(struct drm_printer *p,
> +			     struct drm_encoder *encoder);

sorry for the late reply.
drm_connector.h, drm_crtc.h & drm_plane.h use "atomic_print_state" 
instead of "atomic_print". You may consider renaming it... or not : )

With or without that,
Reviewed-by: Philippe Cornu <philippe.cornu@st.com>
Many thanks
Philippe :-)

>   };
>   
>   /**
>