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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Reply via email to