[RFC] [PATCH weston] Allow output configuration from config file.

Kristian Høgsberg hoegsberg at gmail.com
Mon Jul 30 17:44:19 PDT 2012


On Mon, Jul 30, 2012 at 05:55:12PM -0600, Scott Moreau wrote:
> Parse the config file for [output] sections and check for 'name'
> and 'mode' keys. The key strings are compared to what is reported
> by weston log. The 'mode' key string can be one of the following:
> 
> 1) WIDTHxHEIGHT - One that is reported by weston log, e.g. 1280x1024
> 2) off - Disables the output
> 3) native - Uses the native mode of the output
> 4) current - Uses the mode currently driving the crtc

This looks good, I'm tempted to just apply it as is, but I just have a
couple of nitpicks below.  We should stick to "preferred" and not call
it "native", both KMS and xrandr uses "preferred".

> ---
>  src/compositor-drm.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  weston.ini           | 11 +++---
>  2 files changed, 103 insertions(+), 6 deletions(-)
> 
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index cd0395e..da75937 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c
> @@ -44,6 +44,16 @@
>  #include "log.h"
>  
>  static int option_current_mode = 0;
> +static char *output_name;
> +static char *output_mode;
> +static struct wl_list configured_output_list;
> +
> +struct drm_configured_output {
> +	char *name;
> +	char *mode;
> +	int32_t width, height;
> +	struct wl_list link;
> +};
>
>  
>  enum {
>  	WESTON_PLANE_DRM_CURSOR =	0x100,
> @@ -1296,7 +1306,8 @@ create_output_for_connector(struct drm_compositor *ec,
>  {
>  	struct drm_output *output;
>  	struct drm_mode *drm_mode, *next;
> -	struct weston_mode *m, *preferred, *current;
> +	struct weston_mode *m, *preferred, *current, *configured;
> +	struct drm_configured_output *o = NULL, *temp;
>  	drmModeEncoder *encoder;
>  	drmModeModeInfo crtc_mode;
>  	drmModeCrtc *crtc;
> @@ -1356,13 +1367,40 @@ create_output_for_connector(struct drm_compositor *ec,
>  
>  	preferred = NULL;
>  	current = NULL;
> +	configured = NULL;
> +
> +	wl_list_for_each(temp, &configured_output_list, link) {
> +		if (strcmp(temp->name, output->name) == 0) {
> +			 weston_log("Matched output from config file: %s\n", temp->name);
> +			 o = temp;
> +			 break;
> +		}
> +	}
> +
> +	if (o && strcmp("off", o->mode) == 0) {
> +		weston_log("%s mode \"%s\" in config\n", o->name, o->mode);

I think just the weston_log() above when we find o is enough, just log
both o->mode and o->name there,

> +		weston_log("Disabling output %s\n", o->name);
> +
> +		drmModeSetCrtc(ec->drm.fd, output->crtc_id,
> +							0, 0, 0, 0, 0, NULL);
> +		goto err_free;
> +	}
> +
>  	wl_list_for_each(drm_mode, &output->base.mode_list, base.link) {
> +		if (o && o->width == drm_mode->base.width &&
> +			o->height == drm_mode->base.height) {
> +			weston_log("Using mode %s for output %s\n", o->mode, o->name);

and then this one shouldn't be necessary either

> +			configured = &drm_mode->base;
> +		}
>  		if (!memcmp(&crtc_mode, &drm_mode->mode_info, sizeof crtc_mode))
>  			current = &drm_mode->base;
>  		if (drm_mode->base.flags & WL_OUTPUT_MODE_PREFERRED)
>  			preferred = &drm_mode->base;
>  	}
>  
> +	if (o && configured == NULL)
> +		weston_log("Using %s mode for output %s\n", o->mode, o->name);
> +

nor this one.

>  	if (current == NULL && crtc_mode.clock != 0) {
>  		ret = drm_output_add_mode(output, &crtc_mode);
>  		if (ret)
> @@ -1371,8 +1409,16 @@ create_output_for_connector(struct drm_compositor *ec,
>  				       struct weston_mode, link);
>  	}
>  
> +	if (configured == NULL && (o && strcmp("current", o->mode) == 0))
> +		configured = current;

No need to check configure here, if it's not NULL, it's because we
matched a mode above.  Instead of this and the "native" check below,
I'd just set configured to current if o->config is
OUTPUT_CONFIG_CURRENT (see below) else if OUTPUT_CONFIG_PREFERRED, set
it to preferred...

> +	if (preferred == NULL)
> +		preferred = current;

(I got rid of these two lines in previous commit, they're not necessary now)

>  	if (option_current_mode && current)
>  		output->base.current = current;
> +	else if (configured && (o && strcmp("native", o->mode) != 0))
> +		output->base.current = configured;

and then here we can just say

  else if (configured)
    output->base.current = configured;
  ...

>  	else if (preferred)
>  		output->base.current = preferred;
>  	else if (current)
> @@ -1712,9 +1758,12 @@ drm_destroy(struct weston_compositor *ec)
>  {
>  	struct drm_compositor *d = (struct drm_compositor *) ec;
>  	struct weston_seat *seat, *next;
> +	struct drm_configured_output *o, *n;
>  
>  	wl_list_for_each_safe(seat, next, &ec->seat_list, link)
>  		evdev_input_destroy(seat);
> +	wl_list_for_each_safe(o, n, &configured_output_list, link)
> +		free(o);
>  
>  	wl_event_source_remove(d->udev_drm_source);
>  	wl_event_source_remove(d->drm_source);
> @@ -1982,6 +2031,38 @@ err_base:
>  	return NULL;
>  }
>  
> +static void
> +output_section_done(void *data)
> +{
> +	struct drm_configured_output *output;
> +	int valid_mode_key = 0;
> +
> +	output = malloc(sizeof *output);
> +
> +	if (!output)
> +		return;
> +
> +	if (sscanf(output_mode, "%dx%d", &output->width, &output->height) == 2)
> +		valid_mode_key = 1;
> +	else
> +		output->width = output->height = 0;
> +
> +	if (strcmp(output_mode, "off") == 0 ||
> +		strcmp(output_mode, "native") == 0 ||
> +		strcmp(output_mode, "current") == 0) {
> +		valid_mode_key = 1;
> +	}
> +
> +	if (valid_mode_key) {
> +		output->name = output_name;
> +		output->mode = output_mode;
> +
> +		wl_list_insert(&configured_output_list, &output->link);
> +	} else
> +		weston_log("Ignoring malformed mode \"%s\" for output %s\n",
> +						output_mode, output_name);

Can we introduce an output_config enum with the values
OUTPUT_CONFIG_CURRENT, OUTPUT_CONFIG_PREFERRED, OUTPUT_CONFIG_OFF and
OUTPUT_CONFIG_MODE and do the strcmp when we parse the config section
and just store the mode in drm_configured_output?

 output->config = OUTPUT_CONFIG_INVALID;
 if (sscanf(output_mode, "%dx%d", &output->width, &output->height) == 2)
   output->config = OUTPUT_CONFIG_MODE;
 else if (strcmp(output_mode, "off") == 0)
   output->config = OUTPUT_CONFIG_OFF;
 etc

and then

  if (output->config != OUTPUT_CONFIG_INVALID)
    wl_list_insert(&configured_output_list, &output->link);
  else {
    free(output);
    weston_log();
  }

but keep output->mode so we can use it in the log.

> +}
> +
>  WL_EXPORT struct weston_compositor *
>  backend_init(struct wl_display *display, int argc, char *argv[],
>  	     const char *config_file)
> @@ -1998,6 +2079,21 @@ backend_init(struct wl_display *display, int argc, char *argv[],
>  
>  	parse_options(drm_options, ARRAY_LENGTH(drm_options), argc, argv);
>  
> +	wl_list_init(&configured_output_list);
> +
> +	const struct config_key drm_config_keys[] = {
> +		{ "name", CONFIG_KEY_STRING, &output_name },
> +		{ "mode", CONFIG_KEY_STRING, &output_mode },
> +	};
> +
> +	const struct config_section config_section[] = {
> +		{ "output", drm_config_keys,
> +		ARRAY_LENGTH(drm_config_keys), output_section_done },
> +	};
> +
> +	parse_config_file(config_file, config_section,
> +				ARRAY_LENGTH(config_section), NULL);
> +
>  	return drm_compositor_create(display, connector, seat, tty, argc, argv,
>  				     config_file);
>  }
> diff --git a/weston.ini b/weston.ini
> index f736c8a..375f4ee 100644
> --- a/weston.ini
> +++ b/weston.ini
> @@ -34,9 +34,10 @@ path=./clients/flower
>  path=/usr/libexec/weston-screensaver
>  duration=600
>  
> -[output]
> -name=LVDS1
> -mode=off
> +#[output]
> +#name=LVDS1
> +#mode=off
>  
> -mode=1366x768
> -modeline=36.25 912 936 1024 1136  512 515 525 533 -hsync +vsync
> +#[output]
> +#name=VGA1
> +#mode=1280x1024
> -- 
> 1.7.11.2
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel


More information about the wayland-devel mailing list