diff mbox

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

Message ID 20180605135407.20214-3-benjamin.gaignard@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Benjamin Gaignard June 5, 2018, 1:54 p.m. UTC
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(+)

Comments

Philippe CORNU June 18, 2018, 3:58 p.m. UTC | #1
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 :-)

>   };

>   

>   /**

>
Daniel Vetter July 3, 2018, 9:28 a.m. UTC | #2
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
diff mbox

Patch

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);
 };
 
 /**