[PATCH 2/5] drm: port the drm backend to the new init api
Pekka Paalanen
ppaalanen at gmail.com
Wed Apr 6 08:37:57 UTC 2016
On Wed, 9 Mar 2016 16:49:29 -0800
Bryce Harrington <bryce at osg.samsung.com> wrote:
> From: Giulio Camuffo <giuliocamuffo at gmail.com>
>
> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> Reviewed-by: Quentin Glidic <sardemff7+git at sardemff7.net>
> Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> ---
> v4: Update to current trunk
> - Add missing param doc for mode in drm_output_choose_initial_mode
> - Rebase to account for code changes by 91880f1e to make vt
> switching configurable.
>
> Makefile.am | 3 +
> src/compositor-drm.c | 196 ++++++++++++++++++---------------------------------
> src/compositor.h | 2 -
> src/main.c | 94 +++++++++++++++++++++++-
> 4 files changed, 165 insertions(+), 130 deletions(-)
Hi Giulio and Bryce,
I'm sorry it has taken so long for me to come back to this.
> diff --git a/Makefile.am b/Makefile.am
> index fe08d94..9330f0e 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -72,6 +72,7 @@ weston_SOURCES = \
> src/log.c \
> src/compositor.c \
> src/compositor.h \
> + src/compositor-drm.h \
> src/input.c \
> src/data-device.c \
> src/screenshooter.c \
> @@ -207,6 +208,7 @@ westonincludedir = $(includedir)/weston
> westoninclude_HEADERS = \
> src/version.h \
> src/compositor.h \
> + src/compositor-drm.h \
> src/timeline-object.h \
> shared/matrix.h \
> shared/config-parser.h \
> @@ -276,6 +278,7 @@ drm_backend_la_CFLAGS = \
> $(AM_CFLAGS)
> drm_backend_la_SOURCES = \
> src/compositor-drm.c \
> + src/compositor-drm.h \
> $(INPUT_BACKEND_SOURCES) \
> shared/helpers.h \
> shared/timespec-util.h \
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index e01f6b9..111c882 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c
> @@ -2185,30 +2162,45 @@ get_gbm_format_from_section(struct weston_config_section *section,
> * Find the most suitable mode to use for initial setup (or reconfiguration on
> * hotplug etc) for a DRM output.
> *
> + * @param backend The DRM backend object
> * @param output DRM output to choose mode for
> - * @param kind Strategy and preference to use when choosing mode
> - * @param width Desired width for this output
> - * @param height Desired height for this output
> + * @param mode Controls how to select the mode
> + * @param config Desired configuration for the output
> * @param current_mode Mode currently being displayed on this output
> - * @param modeline Manually-entered mode (may be NULL)
> * @returns A mode from the output's mode list, or NULL if none available
> */
> static struct drm_mode *
> -drm_output_choose_initial_mode(struct drm_output *output,
> - enum output_config kind,
> - int width, int height,
> - const drmModeModeInfo *current_mode,
> - const drmModeModeInfo *modeline)
> +drm_output_choose_initial_mode(struct drm_backend *backend,
> + struct drm_output *output,
> + enum weston_drm_backend_output_mode mode,
> + struct weston_drm_backend_output_config *config,
> + const drmModeModeInfo *current_mode)
> {
> struct drm_mode *preferred = NULL;
> struct drm_mode *current = NULL;
> struct drm_mode *configured = NULL;
> struct drm_mode *best = NULL;
> struct drm_mode *drm_mode;
> + drmModeModeInfo modeline;
> + int32_t width, height;
> +
> + if (mode == WESTON_DRM_BACKEND_OUTPUT_PREFERRED && config->modeline) {
> + if (sscanf(config->modeline, "%dx%d", &width, &height) != 2) {
> + width = -1;
> +
> + if (parse_modeline(config->modeline, &modeline) == 0) {
> + configured = drm_output_add_mode(output, &modeline);
> + if (!configured)
> + return NULL;
> + } else {
> + weston_log("Invalid modeline \"%s\" for output %s\n",
> + config->modeline, output->base.name);
> + }
> + }
> + }
Hmm, this logic above looks a little odd to me. If mode=PREFERRED means
the user wants the preferred mode, why would it look at modeline at all?
Is this changing the meaning of modeline to override the PREFERRED
setting?
Maybe this is an API refactorization artifact, since originally we got
a single config file key "mode" which determined several arguments used
here, so if the caller (main) keeps working the same way as before, it
is impossible to have both mode=PREFERRED and modeline set at the same
time.
Am I missing something?
If this to add a new feature of being able to add custom modes without
actually setting them? If so, I would split that into a separate patch,
since it's a new feature and not about just porting to the new API. And
in that case one might also want to set multiple new custom modes, so
this isn't even a sufficient API to my understanding.
I would think that if mode=PREFERRED, we get the EDID mode tagged as
preferred, period. If mode=MODELINE (I forget what the alternatives
actually were), then we inspect the modeline argument and set that.
>
> wl_list_for_each_reverse(drm_mode, &output->base.mode_list, base.link) {
> - if (kind == OUTPUT_CONFIG_MODE &&
> - width == drm_mode->base.width &&
> + if (width == drm_mode->base.width &&
> height == drm_mode->base.height)
Width and height used possibly uninitialized.
> configured = drm_mode;
>
> WL_EXPORT int
> backend_init(struct weston_compositor *compositor, int *argc, char *argv[],
> - struct weston_config *config,
> - struct weston_backend_config *config_base)
> + struct weston_config *wc,
> + struct weston_backend_config *config_base)
> {
> struct drm_backend *b;
> - struct drm_parameters param = { 0, };
> -
> - const struct weston_option drm_options[] = {
> - { WESTON_OPTION_INTEGER, "connector", 0, ¶m.connector },
> - { WESTON_OPTION_STRING, "seat", 0, ¶m.seat_id },
> - { WESTON_OPTION_INTEGER, "tty", 0, ¶m.tty },
> - { WESTON_OPTION_BOOLEAN, "current-mode", 0, &option_current_mode },
> - { WESTON_OPTION_BOOLEAN, "use-pixman", 0, ¶m.use_pixman },
> - };
> -
> - param.seat_id = default_seat;
> -
> - parse_options(drm_options, ARRAY_LENGTH(drm_options), argc, argv);
> + struct weston_drm_backend_config *config =
> + (struct weston_drm_backend_config *)config_base;
The patch to add struct versioning should affect the code here, so I'd
rather see the drm-specific parts of that patch squashed into this one.
>
> - b = drm_backend_create(compositor, ¶m, argc, argv, config);
> + b = drm_backend_create(compositor, config);
> if (b == NULL)
> return -1;
> return 0;
> diff --git a/src/compositor.h b/src/compositor.h
> index 794a1b0..30462cf 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -673,8 +673,6 @@ enum weston_capability {
> */
> struct weston_backend_output_config {
> uint32_t transform;
> - uint32_t width;
> - uint32_t height;
> uint32_t scale;
> };
>
> diff --git a/src/main.c b/src/main.c
> index 43de354..66c054e 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -47,6 +47,8 @@
> #include "git-version.h"
> #include "version.h"
>
> +#include "compositor-drm.h"
> +
> static struct wl_list child_process_list;
> static struct weston_compositor *segv_compositor;
>
> @@ -670,13 +672,101 @@ load_backend_new(struct weston_compositor *compositor, const char *backend,
> return backend_init(compositor, NULL, NULL, NULL, config_base);
> }
>
> +struct drm_config {
> + struct weston_drm_backend_config base;
> + bool use_current_mode;
> +};
> +
> +static enum weston_drm_backend_output_mode
> +drm_configure_output(struct weston_compositor *c,
> + struct weston_drm_backend_config *backend_config,
> + const char *name,
> + struct weston_drm_backend_output_config *config)
> +{
> + struct drm_config *drm_config = (struct drm_config *)backend_config;
I think this cast will break when introducing the struct versioning.
struct drm_config is a main.c thing, but the backend will not be
storing the main-passed pointer as is. Instead the backend will copy
the set part of the passed in struct weston_drm_backend_config
into it's own version of the struct and fill unset fields with
defaults. This happens when main.c has been complied with an older
definition of struct weston_drm_backend_config than what the backend
itself uses.
Callbacks like this will need to carry an explicit void *user_data,
unless you rely on the user_data of weston_compositor.
> + struct weston_config *wc = weston_compositor_get_user_data(c);
> + struct weston_config_section *section;
> + char *s;
> + int scale;
> + enum weston_drm_backend_output_mode mode =
> + WESTON_DRM_BACKEND_OUTPUT_PREFERRED;
> +
> + section = weston_config_get_section(wc, "output", "name", name);
> + weston_config_section_get_string(section, "mode", &s, "preferred");
> + if (strcmp(s, "off") == 0) {
> + free(s);
> + return WESTON_DRM_BACKEND_OUTPUT_OFF;
> + }
> +
> + if (drm_config->use_current_mode || strcmp(s, "current") == 0) {
> + mode = WESTON_DRM_BACKEND_OUTPUT_CURRENT;
> + } else if (strcmp(s, "preferred") != 0) {
> + config->modeline = s;
> + s = NULL;
> + }
> + free(s);
> +
> + weston_config_section_get_int(section, "scale", &scale, 1);
> + config->base.scale = scale >= 1 ? scale : 1;
> + weston_config_section_get_string(section, "transform", &s, "normal");
> + if (weston_parse_transform(s, &config->base.transform) < 0)
> + weston_log("Invalid transform \"%s\" for output %s\n",
> + s, name);
> + free(s);
> +
> + weston_config_section_get_string(section,
> + "gbm-format", &config->format, NULL);
> + weston_config_section_get_string(section, "seat", &config->seat, "");
> + return mode;
> +}
> +
> +static int
> +load_drm_backend(struct weston_compositor *c, const char *backend,
> + int *argc, char **argv, struct weston_config *wc)
> +{
> + struct drm_config *config;
> + struct weston_config_section *section;
> + int ret = 0;
> +
> + config = zalloc(sizeof *config);
> + if (!config)
> + return -1;
This function will be affected by the struct versioning too.
> + const struct weston_option options[] = {
> + { WESTON_OPTION_INTEGER, "connector", 0, &config->base.connector },
> + { WESTON_OPTION_STRING, "seat", 0, &config->base.seat_id },
> + { WESTON_OPTION_INTEGER, "tty", 0, &config->base.tty },
> + { WESTON_OPTION_BOOLEAN, "current-mode", 0,
> + &config->use_current_mode },
> + { WESTON_OPTION_BOOLEAN, "use-pixman", 0, &config->base.use_pixman },
> + };
Mixed declarations and code.
> +
> + parse_options(options, ARRAY_LENGTH(options), argc, argv);
> +
> + section = weston_config_get_section(wc, "core", NULL, NULL);
> + weston_config_section_get_string(section,
> + "gbm-format", &config->base.format,
> + NULL);
> +
> + config->base.configure_output = drm_configure_output;
> +
> + if (load_backend_new(c, backend, &config->base.base) < 0) {
> + ret = -1;
> + free(config->base.format);
> + free(config->base.seat_id);
> + free(config);
> + }
> +
> + return ret;
> +}
> +
> static int
> load_backend(struct weston_compositor *compositor, const char *backend,
> int *argc, char **argv, struct weston_config *config)
> {
> -#if 0
> if (strstr(backend, "drm-backend.so"))
> return load_drm_backend(compositor, backend, argc, argv, config);
> +#if 0
> else if (strstr(backend, "wayland-backend.so"))
> return load_wayland_backend(compositor, backend, argc, argv, config);
> else if (strstr(backend, "x11-backend.so"))
> @@ -785,7 +875,7 @@ int main(int argc, char *argv[])
> backend = weston_choose_default_backend();
> }
>
> - ec = weston_compositor_create(display, NULL);
> + ec = weston_compositor_create(display, config);
> if (ec == NULL) {
> weston_log("fatal: failed to create compositor\n");
> goto out;
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160406/2dca71da/attachment-0001.sig>
More information about the wayland-devel
mailing list