[PATCH weston v5 14/14] compositor-drm: Add drm-backend log debug scope

[Pekka Paalanen]

On Fri, 20 Jul 2018 20:03:35 +0100
Daniel Stone <daniels at collabora.com> wrote:

> Add a 'drm-debug' scope which prints verbose information about the DRM
> backend's repaint cycle, including the decision tree on how views are
> assigned (or not) to planes.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  libweston/compositor-drm.c | 233 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 206 insertions(+), 27 deletions(-)
> 

Hi,

lots of output from this one, nice!


> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 653d13e0c..2cadf036c 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -51,6 +51,7 @@
>  
>  #include "compositor.h"
>  #include "compositor-drm.h"
> +#include "weston-debug.h"
>  #include "shared/helpers.h"
>  #include "shared/timespec-util.h"
>  #include "gl-renderer.h"
> @@ -73,6 +74,9 @@
>  #define GBM_BO_USE_CURSOR GBM_BO_USE_CURSOR_64X64
>  #endif
>  
> +#define drm_debug(b, ...) \
> +	weston_debug_scope_printf((b)->debug, __VA_ARGS__)
> +
>  #define MAX_CLONED_CONNECTORS 4
>  
>  /**
> @@ -302,6 +306,8 @@ struct drm_backend {
>  	bool shutting_down;
>  
>  	bool aspect_ratio_supported;
> +
> +	struct weston_debug_scope *debug;
>  };
>  
>  struct drm_mode {
> @@ -2358,6 +2364,9 @@ crtc_add_prop(drmModeAtomicReq *req, struct drm_output *output,
>  
>  	ret = drmModeAtomicAddProperty(req, output->crtc_id, info->prop_id,
>  				       val);
> +	drm_debug(output->backend, "\t\t\t[CRTC:%d] %d (%s) -> %lld (0x%llx)\n",
> +		  output->crtc_id, info->prop_id, info->name,
> +		  (unsigned long long) val, (unsigned long long) val);

This is using %lld to print (unsigned long long), should it not be %llu
instead?

All the property ids are unsigned as well, so %d -> %u?

>  	return (ret <= 0) ? -1 : 0;
>  }
>  
> @@ -2373,6 +2382,9 @@ connector_add_prop(drmModeAtomicReq *req, struct drm_head *head,
>  
>  	ret = drmModeAtomicAddProperty(req, head->connector_id,
>  				       info->prop_id, val);
> +	drm_debug(head->backend, "\t\t\t[CONN:%d] %d (%s) -> %lld (0x%llx)\n",
> +		  head->connector_id, info->prop_id, info->name,
> +		  (unsigned long long) val, (unsigned long long) val);

%d -> %u
%lld -> %llu

>  	return (ret <= 0) ? -1 : 0;
>  }
>  
> @@ -2388,6 +2400,9 @@ plane_add_prop(drmModeAtomicReq *req, struct drm_plane *plane,
>  
>  	ret = drmModeAtomicAddProperty(req, plane->plane_id, info->prop_id,
>  				       val);
> +	drm_debug(plane->backend, "\t\t\t[PLANE:%d] %d (%s) -> %lld (0x%llx)\n",
> +		  plane->plane_id, info->prop_id, info->name,
> +		  (unsigned long long) val, (unsigned long long) val);

%d -> %u
%lld -> %llu

>  	return (ret <= 0) ? -1 : 0;
>  }
>  
> @@ -2406,6 +2421,9 @@ drm_mode_ensure_blob(struct drm_backend *backend, struct drm_mode *mode)
>  	if (ret != 0)
>  		weston_log("failed to create mode property blob: %m\n");
>  
> +	drm_debug(backend, "\t\t\t[atomic] created new mode blob %ld for %s",
> +		  (unsigned long) mode->blob_id, mode->mode_info.name);

%ld -> %u without the cast?

There are more of these kinds below, should I point them out?


> +
>  	return ret;
>  }

...

> @@ -2969,10 +3010,15 @@ drm_repaint_begin(struct weston_compositor *compositor)
>  {
>  	struct drm_backend *b = to_drm_backend(compositor);
>  	struct drm_pending_state *ret;
> +	char *scene_graph = weston_compositor_print_scene_graph(compositor);

Maybe you'd want to make the call to
weston_compositor_print_scene_graph() conditional on the debug scope
being enabled? To not incur the work while not being debugged.

>  
>  	ret = drm_pending_state_alloc(b);
>  	b->repaint_data = ret;
>  
> +	drm_debug(b, "[repaint] Beignning repaint; pending_state %p\n", ret);

Typoed "Beginning".


> +	drm_debug(b, "%s", scene_graph);
> +	free(scene_graph);
> +
>  	return ret;
>  }
>  

...

> @@ -3531,12 +3691,20 @@ drm_assign_planes(struct weston_output *output_base, void *repaint_data)
>  	struct weston_view *ev;
>  	struct weston_plane *primary = &output_base->compositor->primary_plane;
>  
> +	drm_debug(b, "\t[repaint] preparing state for output %s (%d)\n",
> +		  output_base->name, output_base->id);
> +
>  	if (!b->sprites_are_broken) {
>  		state = drm_output_propose_state(output_base, pending_state,
>  						 DRM_OUTPUT_PROPOSE_STATE_PLANES_ONLY);
> -		if (!state)
> +		if (!state) {
> +			drm_debug(b, "\t[repaint] could not build planes-only "
> +				     "state, trying mixed\n");
>  			state = drm_output_propose_state(output_base, pending_state,
>  							 DRM_OUTPUT_PROPOSE_STATE_MIXED);

Should there be a note if mixed mode failed?

> +		}
> +	} else {
> +		drm_debug(b, "\t[state] no overlay plane support\n");
>  	}
>  
>  	if (!state)

Looks good aside from the minor details.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180807/4bdf521b/attachment.sig>