[PATCH 2/5] drm: port the drm backend to the new init api
Giulio Camuffo
giuliocamuffo at gmail.com
Thu Apr 7 06:04:46 UTC 2016
2016-04-06 11:37 GMT+03:00 Pekka Paalanen <ppaalanen at gmail.com>:
> 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?
This is the definition of the relevant enum in compositor-drm.h:
enum weston_drm_backend_output_mode {
/** The output is disabled */
WESTON_DRM_BACKEND_OUTPUT_OFF,
/** The output will use the current active mode */
WESTON_DRM_BACKEND_OUTPUT_CURRENT,
/** The output will use the preferred mode. A modeline can be provided
* by setting weston_backend_output_config::modeline in the form of
* "WIDTHxHEIGHT" or in the form of an explicit modeline calculated
* using e.g. the cvt tool. If a valid modeline is supplied it will be
* used, if invalid or NULL the preferred available mode will be used. */
WESTON_DRM_BACKEND_OUTPUT_PREFERRED,
};
So WESTON_DRM_BACKEND_OUTPUT_PREFERRED is not equivalent to
DRM_MODE_TYPE_PREFERRED. Instead of having both a PREFERRED and
MODELINE entries in that enum PREFERRED is used for both, and if the
given modeline is NULL it has the same meaning as
DRM_MODE_TYPE_PREFERRED.
So maybe the name is not the best one.
WESTON_DRM_BACKEND_OUTPUT_PREFERRED_OR_MODELINE would be more
explicit.
>
> 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.
This is how it was in a previous iteration of the patch iirc, but
while it was ok as a weston internal api i think that as a library api
it would be less than optimal. Using MODELINE without providing a
modeline doesn't make sense, and also using PREFERRED while providing
a modeline doesn't. And then we also had MODE, which was different
from MODELINE, and all expected the modeline string to have different
formats. So instead i think a simple enum with specified behavior
depending to the modeline format is simpler.
>
>>
>> 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
More information about the wayland-devel
mailing list