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

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jun 05, 2018 at 03:54:02PM +0200, 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);
> +	}

This doesn't make sense to me at all. There's no encoder state, what
exactly do you want to print here?

Note that if you just want to dump hw state from your atomic state
functions (which might be useful sometimes, who knows) you can also dump
encoders by looking at connector_state->best_encoder.

Of have your own sti_dump_state which also adds this loop here.

Ack on the first patch from me.
-Daniel

> +
>  	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);
>  };
>  
>  /**
> -- 
> 2.15.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch