On 13.09.2016 12:49, Pekka Paalanen wrote:
> On Thu, 18 Aug 2016 18:42:32 +0200
> Armin Krezović <krezovic.ar...@gmail.com> wrote:
>
>> This is a complete port of the DRM backend that uses
>> recently added output handling API for output
>> configuration.
>>
>> Output can be configured at runtime by passing the
>> necessary configuration parameters, which can be
>> filled in manually or obtained from the configuration
>> file using previously added functionality. It is
>> required that the scale and transform values are set
>> using the previously added functionality.
>>
>> After everything has been set, output needs to be
>> enabled manually using weston_output_enable().
>>
>> v2:
>>
>> - Added missing drmModeFreeCrtc() to drm_output_enable()
>> cleanup list in case of failure.
>> - Split drm_backend_disable() into drm_backend_deinit()
>> to accomodate for changes in the first patch in the
>> series. Moved restoring original crtc to
>> drm_output_destroy().
>>
>> v3:
>>
>> - Moved origcrtc allocation to drm_output_set_mode().
>> - Swapped connector_get_current_mode() and
>> drm_output_add_mode() calls in drm_output_set_mode()
>> to match current weston.
>> - Moved crtc_allocator and connector_allocator update
>> from drm_output_enable() to create_output_for_connector()
>> to avoid problems when more than one monitor is connected
>> at startup and crtc allocator wasn't updated before
>> create_output_for_connector() was called second time,
>> resulting in one screen being turned off.
>> - Moved crtc_allocator and connector_allocator update from
>> drm_output_deinit() to drm_output_destroy(), as it
>> should not be called on drm_output_disable().
>> - Use weston_compositor_add_pending_output().
>> - Bump weston_drm_backend_config version to 2.
>>
>> Signed-off-by: Armin Krezović <krezovic.ar...@gmail.com>
>> ---
>> compositor/main.c | 104 ++++++-----
>> libweston/compositor-drm.c | 434
>> ++++++++++++++++++++++++++-------------------
>> libweston/compositor-drm.h | 50 +++---
>> 3 files changed, 343 insertions(+), 245 deletions(-)
>
> Hi Armin,
>
> all in all, this patch looks very good. There are a few minor bugs to
> be squashed, and I did list some future development ideas you are free
> to ignore.
>
> Please wait for reviews for the whole series before re-sending.
>
> Details inlined below.
>
>>
>> diff --git a/compositor/main.c b/compositor/main.c
>> index 0cc11a5..38df77f 100644
>> --- a/compositor/main.c
>> +++ b/compositor/main.c
>> @@ -1087,48 +1087,6 @@ wet_configure_windowed_output_from_config(struct
>> weston_output *output,
>> return 0;
>> }
>>
>> -static enum weston_drm_backend_output_mode
>> -drm_configure_output(struct weston_compositor *c,
>> - bool use_current_mode,
>> - const char *name,
>> - struct weston_drm_backend_output_config *config)
>> -{
>> - struct weston_config *wc = wet_get_config(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 (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->gbm_format,
>> NULL);
>> - weston_config_section_get_string(section, "seat", &config->seat, "");
>> - return mode;
>> -}
>> -
>> static void
>> configure_input_device(struct weston_compositor *compositor,
>> struct libinput_device *device)
>> @@ -1153,6 +1111,65 @@ configure_input_device(struct weston_compositor
>> *compositor,
>> }
>> }
>>
>> +static void
>> +drm_backend_output_configure(struct wl_listener *listener, void *data)
>> +{
>> + struct weston_output *output = data;
>> + struct weston_config *wc = wet_get_config(output->compositor);
>> + struct weston_config_section *section;
>> + const struct weston_drm_output_api *api =
>> weston_drm_output_get_api(output->compositor);
>> + enum weston_drm_backend_output_mode mode =
>> + WESTON_DRM_BACKEND_OUTPUT_PREFERRED;
>> +
>> + char *s;
>> + char *modeline = NULL;
>> + char *gbm_format = NULL;
>> + char *seat = NULL;
>> +
>> + if (!api) {
>> + weston_log("Cannot use weston_drm_output_api.\n");
>> + return;
>> + }
>> +
>> + section = weston_config_get_section(wc, "output", "name", output->name);
>> + weston_config_section_get_string(section, "mode", &s, "preferred");
>> +
>> + if (strcmp(s, "off") == 0) {
>> + weston_output_disable(output);
>> + free(s);
>> + return;
>> + } else if (strcmp(s, "current") == 0) {
>> + mode = WESTON_DRM_BACKEND_OUTPUT_CURRENT;
>> + } else if (strcmp(s, "preferred") != 0) {
>> + modeline = s;
>> + s = NULL;
>> + }
>> + free(s);
>> +
>> + if (api->set_mode(output, mode, modeline) < 0) {
>> + weston_log("Cannot configure an output using
>> weston_drm_output_api.\n");
>> + free(modeline);
>> + return;
>> + }
>> + free(modeline);
>> +
>> + wet_output_set_scale(output, section, 1, 0);
>> + wet_output_set_transform(output, section, WL_OUTPUT_TRANSFORM_NORMAL,
>> UINT32_MAX);
>> +
>> + weston_config_section_get_string(section,
>> + "gbm-format", &gbm_format, NULL);
>> +
>> + api->set_gbm_format(output, gbm_format);
>> + free(gbm_format);
>> +
>> + weston_config_section_get_string(section, "seat", &seat, "");
>> +
>> + api->set_seat(output, seat);
>> + free(seat);
>> +
>> + weston_output_enable(output);
>> +}
>> +
>> static int
>> load_drm_backend(struct weston_compositor *c,
>> int *argc, char **argv, struct weston_config *wc)
>> @@ -1178,12 +1195,13 @@ load_drm_backend(struct weston_compositor *c,
>>
>> config.base.struct_version = WESTON_DRM_BACKEND_CONFIG_VERSION;
>> config.base.struct_size = sizeof(struct weston_drm_backend_config);
>> - config.configure_output = drm_configure_output;
>> config.configure_device = configure_input_device;
>>
>> ret = weston_compositor_load_backend(c, WESTON_BACKEND_DRM,
>> &config.base);
>>
>> + wet_set_pending_output_handler(c, drm_backend_output_configure);
>> +
>> free(config.gbm_format);
>> free(config.seat_id);
>>
>> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
>> index 8319d7c..5b23ea1 100644
>> --- a/libweston/compositor-drm.c
>> +++ b/libweston/compositor-drm.c
>> @@ -122,16 +122,6 @@ struct drm_backend {
>> int32_t cursor_width;
>> int32_t cursor_height;
>>
>> - /** Callback used to configure the outputs.
>> - *
>> - * This function will be called by the backend when a new DRM
>> - * output needs to be configured.
>> - */
>> - enum weston_drm_backend_output_mode
>> - (*configure_output)(struct weston_compositor *compositor,
>> - bool use_current_mode,
>> - const char *name,
>> - struct weston_drm_backend_output_config
>> *output_config);
>> bool use_current_mode;
>> };
>>
>> @@ -161,7 +151,8 @@ struct drm_edid {
>> };
>>
>> struct drm_output {
>> - struct weston_output base;
>> + struct weston_output base;
>> + drmModeConnector *connector;
>>
>> uint32_t crtc_id;
>> int pipe;
>> @@ -176,6 +167,7 @@ struct drm_output {
>> int vblank_pending;
>> int page_flip_pending;
>> int destroy_pending;
>> + int disable_pending;
>>
>> struct gbm_surface *gbm_surface;
>> struct gbm_bo *gbm_cursor_bo[2];
>> @@ -681,7 +673,7 @@ drm_output_repaint(struct weston_output *output_base,
>> struct drm_mode *mode;
>> int ret = 0;
>>
>> - if (output->destroy_pending)
>> + if (output->disable_pending || output->destroy_pending)
>> return -1;
>>
>> if (!output->next)
>> @@ -787,7 +779,7 @@ drm_output_start_repaint_loop(struct weston_output
>> *output_base)
>> .request.signal = 0,
>> };
>>
>> - if (output->destroy_pending)
>> + if (output->disable_pending || output->destroy_pending)
>> return;
>>
>> if (!output->current) {
>> @@ -877,7 +869,7 @@ vblank_handler(int fd, unsigned int frame, unsigned int
>> sec, unsigned int usec,
>> }
>>
>> static void
>> -drm_output_destroy(struct weston_output *output_base);
>> +drm_output_destroy(struct weston_output *base);
>>
>> static void
>> page_flip_handler(int fd, unsigned int frame,
>> @@ -904,6 +896,8 @@ page_flip_handler(int fd, unsigned int frame,
>>
>> if (output->destroy_pending)
>> drm_output_destroy(&output->base);
>> + else if (output->disable_pending)
>> + weston_output_disable(&output->base);
>> else if (!output->vblank_pending) {
>> ts.tv_sec = sec;
>> ts.tv_nsec = usec * 1000;
>> @@ -1344,51 +1338,6 @@ drm_assign_planes(struct weston_output *output_base)
>> static void
>> drm_output_fini_pixman(struct drm_output *output);
>>
>> -static void
>> -drm_output_destroy(struct weston_output *output_base)
>> -{
>> - struct drm_output *output = to_drm_output(output_base);
>> - struct drm_backend *b = to_drm_backend(output->base.compositor);
>> - drmModeCrtcPtr origcrtc = output->original_crtc;
>> -
>> - if (output->page_flip_pending) {
>> - output->destroy_pending = 1;
>> - weston_log("destroy output while page flip pending\n");
>> - return;
>> - }
>> -
>> - if (output->backlight)
>> - backlight_destroy(output->backlight);
>> -
>> - drmModeFreeProperty(output->dpms_prop);
>> -
>> - /* Turn off hardware cursor */
>> - drmModeSetCursor(b->drm.fd, output->crtc_id, 0, 0, 0);
>> -
>> - /* Restore original CRTC state */
>> - drmModeSetCrtc(b->drm.fd, origcrtc->crtc_id, origcrtc->buffer_id,
>> - origcrtc->x, origcrtc->y,
>> - &output->connector_id, 1, &origcrtc->mode);
>> - drmModeFreeCrtc(origcrtc);
>> -
>> - b->crtc_allocator &= ~(1 << output->crtc_id);
>> - b->connector_allocator &= ~(1 << output->connector_id);
>> -
>> - if (b->use_pixman) {
>> - drm_output_fini_pixman(output);
>> - } else {
>> - gl_renderer->output_destroy(output_base);
>> - gbm_surface_destroy(output->gbm_surface);
>> - }
>> -
>> - weston_plane_release(&output->fb_plane);
>> - weston_plane_release(&output->cursor_plane);
>> -
>> - weston_output_destroy(&output->base);
>> -
>> - free(output);
>> -}
>> -
>> /**
>> * Find the closest-matching mode for a given target
>> *
>> @@ -2239,7 +2188,7 @@ static struct drm_mode *
>> 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 char *modeline,
>> const drmModeModeInfo *current_mode)
>> {
>> struct drm_mode *preferred = NULL;
>> @@ -2247,21 +2196,21 @@ drm_output_choose_initial_mode(struct drm_backend
>> *backend,
>> struct drm_mode *configured = NULL;
>> struct drm_mode *best = NULL;
>> struct drm_mode *drm_mode;
>> - drmModeModeInfo modeline;
>> + drmModeModeInfo drm_modeline;
>> int32_t width = 0;
>> int32_t height = 0;
>>
>> - if (mode == WESTON_DRM_BACKEND_OUTPUT_PREFERRED && config->modeline) {
>> - if (sscanf(config->modeline, "%dx%d", &width, &height) != 2) {
>> + if (mode == WESTON_DRM_BACKEND_OUTPUT_PREFERRED && modeline) {
>> + if (sscanf(modeline, "%dx%d", &width, &height) != 2) {
>> width = -1;
>>
>> - if (parse_modeline(config->modeline, &modeline) == 0) {
>> - configured = drm_output_add_mode(output,
>> &modeline);
>> + if (parse_modeline(modeline, &drm_modeline) == 0) {
>> + configured = drm_output_add_mode(output,
>> &drm_modeline);
>> if (!configured)
>> return NULL;
>> } else {
>> weston_log("Invalid modeline \"%s\" for output
>> %s\n",
>> - config->modeline, output->base.name);
>> + modeline, output->base.name);
>> }
>> }
>> }
>> @@ -2330,109 +2279,103 @@ connector_get_current_mode(drmModeConnector
>> *connector, int drm_fd,
>> return 0;
>> }
>>
>> -/**
>> - * Create and configure a Weston output structure
>> - *
>> - * Given a DRM connector, create a matching drm_output structure and add it
>> - * to Weston's output list.
>> - *
>> - * @param b Weston backend structure structure
>> - * @param resources DRM resources for this device
>> - * @param connector DRM connector to use for this new output
>> - * @param x Horizontal offset to use into global co-ordinate space
>> - * @param y Vertical offset to use into global co-ordinate space
>> - * @param drm_device udev device pointer
>> - * @returns 0 on success, or -1 on failure
>> - */
>> static int
>> -create_output_for_connector(struct drm_backend *b,
>> - drmModeRes *resources,
>> - drmModeConnector *connector,
>> - int x, int y, struct udev_device *drm_device)
>> +drm_output_set_mode(struct weston_output *base,
>> + enum weston_drm_backend_output_mode mode,
>> + const char *modeline)
>> {
>> - struct drm_output *output;
>> - struct drm_mode *drm_mode, *next, *current;
>> - struct weston_mode *m;
>> + struct drm_output *output = to_drm_output(base);
>> + struct drm_backend *b = to_drm_backend(base->compositor);
>>
>> + struct drm_mode *drm_mode, *next, *current;
>> drmModeModeInfo crtc_mode;
>> int i;
>> - enum weston_drm_backend_output_mode mode;
>> - struct weston_drm_backend_output_config config = {{ 0 }};
>> -
>> - i = find_crtc_for_connector(b, resources, connector);
>> - if (i < 0) {
>> - weston_log("No usable crtc/encoder pair for connector.\n");
>> - return -1;
>> - }
>> -
>> - output = zalloc(sizeof *output);
>> - if (output == NULL)
>> - return -1;
>>
>> - output->base.subpixel = drm_subpixel_to_wayland(connector->subpixel);
>> - output->base.name = make_connector_name(connector);
>> output->base.make = "unknown";
>> output->base.model = "unknown";
>> output->base.serial_number = "unknown";
>> wl_list_init(&output->base.mode_list);
>>
>> - mode = b->configure_output(b->compositor, b->use_current_mode,
>> - output->base.name, &config);
>> - if (parse_gbm_format(config.gbm_format, b->gbm_format,
>> &output->gbm_format) == -1)
>> - output->gbm_format = b->gbm_format;
>> -
>> - setup_output_seat_constraint(b, &output->base,
>> - config.seat ? config.seat : "");
>> - free(config.seat);
>> -
>> - output->crtc_id = resources->crtcs[i];
>> - output->pipe = i;
>> - b->crtc_allocator |= (1 << output->crtc_id);
>> - output->connector_id = connector->connector_id;
>> - b->connector_allocator |= (1 << output->connector_id);
>> -
>> output->original_crtc = drmModeGetCrtc(b->drm.fd, output->crtc_id);
>> - output->dpms_prop = drm_get_prop(b->drm.fd, connector, "DPMS");
>>
>> - if (connector_get_current_mode(connector, b->drm.fd, &crtc_mode) < 0)
>> + if (connector_get_current_mode(output->connector, b->drm.fd,
>> &crtc_mode) < 0)
>> goto err_free;
>>
>> - for (i = 0; i < connector->count_modes; i++) {
>> - drm_mode = drm_output_add_mode(output, &connector->modes[i]);
>> + for (i = 0; i < output->connector->count_modes; i++) {
>> + drm_mode = drm_output_add_mode(output,
>> &output->connector->modes[i]);
>> if (!drm_mode)
>> goto err_free;
>> }
>>
>> - if (mode == WESTON_DRM_BACKEND_OUTPUT_OFF) {
>> - weston_log("Disabling output %s\n", output->base.name);
>> - drmModeSetCrtc(b->drm.fd, output->crtc_id,
>> - 0, 0, 0, 0, 0, NULL);
>> - goto err_free;
>> - }
>> -
>> - current = drm_output_choose_initial_mode(b, output, mode, &config,
>> - &crtc_mode);
>> + current = drm_output_choose_initial_mode(b, output, mode, modeline,
>> &crtc_mode);
>> if (!current)
>> goto err_free;
>> +
>> output->base.current_mode = ¤t->base;
>> output->base.current_mode->flags |= WL_OUTPUT_MODE_CURRENT;
>
> Here is an idea for optional follow-up work, not to be done in this
> patch: construct the mode list already in
> create_output_for_connector(). This way the compositor can inspect the
> possible video modes before it picks a video mode. That would actually
> be a beginning for redesigning how the the video mode setting API for
> DRM works. Obviously, that's for another time.
>
Fine by me, just let us land these series first.
>>
>> - weston_output_init(&output->base, b->compositor, x, y,
>> - connector->mmWidth, connector->mmHeight,
>> - config.base.transform, config.base.scale);
>> + /* Set native_ fields, so weston_output_mode_switch_to_native() works */
>> + output->base.native_mode = output->base.current_mode;
>> + output->base.native_scale = output->base.current_scale;
>> +
>> + output->base.mm_width = output->connector->mmWidth;
>> + output->base.mm_height = output->connector->mmHeight;
>> +
>> + return 0;
>> +
>> +err_free:
>> + drmModeFreeCrtc(output->original_crtc);
>
> original_crtc needs to be reset to NULL, otherwise drm_output_destroy()
> uses it.
>
Oops. Probably a mistake while refactoring the code.
>> +
>> + wl_list_for_each_safe(drm_mode, next, &output->base.mode_list,
>> + base.link) {
>> + wl_list_remove(&drm_mode->base.link);
>> + free(drm_mode);
>> + }
>> +
>> + return -1;
>> +}
>> +
>> +static void
>> +drm_output_set_gbm_format(struct weston_output *base,
>> + const char *gbm_format)
>> +{
>> + struct drm_output *output = to_drm_output(base);
>> + struct drm_backend *b = to_drm_backend(base->compositor);
>> +
>> + if (parse_gbm_format(gbm_format, b->gbm_format, &output->gbm_format) ==
>> -1)
>> + output->gbm_format = b->gbm_format;
>> +}
>> +
>> +static void
>> +drm_output_set_seat(struct weston_output *base,
>> + const char *seat)
>> +{
>> + struct drm_output *output = to_drm_output(base);
>> + struct drm_backend *b = to_drm_backend(base->compositor);
>> +
>> + setup_output_seat_constraint(b, &output->base,
>> + seat ? seat : "");
>> +}
>> +
>> +static int
>> +drm_output_enable(struct weston_output *base)
>> +{
>> + struct drm_output *output = to_drm_output(base);
>> + struct drm_backend *b = to_drm_backend(base->compositor);
>> + struct weston_mode *m;
>> +
>> + output->dpms_prop = drm_get_prop(b->drm.fd, output->connector, "DPMS");
>>
>> if (b->use_pixman) {
>> if (drm_output_init_pixman(output, b) < 0) {
>> weston_log("Failed to init output pixman state\n");
>> - goto err_output;
>> + goto err_free;
>> }
>> } else if (drm_output_init_egl(output, b) < 0) {
>> weston_log("Failed to init output gl state\n");
>> - goto err_output;
>> + goto err_free;
>> }
>>
>> - output->backlight = backlight_init(drm_device,
>> - connector->connector_type);
>> if (output->backlight) {
>
> Any reason you left the backlight_init() call in
> create_output_for_connector()? I think it would be more logical here,
> being called only when enabled. It shouldn't matter in practise though.
>
It would, yes. But backlight_init uses struct udev_device, which is passed
to create_output_for_connector, and not preserved anywhere.
Sure, I could preserve that one too, but I didn't like the idea (it's enough
we have to preserve the connector).
If you still want it to be initialized here, I can preserve struct udev_device
just fine.
>> weston_log("Initialized backlight, device %s\n",
>> output->backlight->path);
>> @@ -2442,15 +2385,8 @@ create_output_for_connector(struct drm_backend *b,
>> weston_log("Failed to initialize backlight\n");
>> }
>>
>> - weston_compositor_add_output(b->compositor, &output->base);
>> -
>> - find_and_parse_output_edid(b, output, connector);
>> - if (connector->connector_type == DRM_MODE_CONNECTOR_LVDS)
>> - output->base.connection_internal = 1;
>> -
>> output->base.start_repaint_loop = drm_output_start_repaint_loop;
>> output->base.repaint = drm_output_repaint;
>> - output->base.destroy = drm_output_destroy;
>> output->base.assign_planes = drm_assign_planes;
>> output->base.set_dpms = drm_set_dpms;
>> output->base.switch_mode = drm_output_switch_mode;
>> @@ -2458,6 +2394,12 @@ create_output_for_connector(struct drm_backend *b,
>> output->base.gamma_size = output->original_crtc->gamma_size;
>> output->base.set_gamma = drm_output_set_gamma;
>>
>> + output->base.subpixel =
>> drm_subpixel_to_wayland(output->connector->subpixel);
>> +
>> + find_and_parse_output_edid(b, output, output->connector);
>> + if (output->connector->connector_type == DRM_MODE_CONNECTOR_LVDS)
>> + output->base.connection_internal = 1;
>> +
>> weston_plane_init(&output->cursor_plane, b->compositor,
>> INT32_MIN, INT32_MIN);
>> weston_plane_init(&output->fb_plane, b->compositor, 0, 0);
>> @@ -2475,31 +2417,155 @@ create_output_for_connector(struct drm_backend *b,
>> ", preferred" : "",
>> m->flags & WL_OUTPUT_MODE_CURRENT ?
>> ", current" : "",
>> - connector->count_modes == 0 ?
>> + output->connector->count_modes == 0 ?
>> ", built-in" : "");
>>
>> - /* Set native_ fields, so weston_output_mode_switch_to_native() works */
>> - output->base.native_mode = output->base.current_mode;
>> - output->base.native_scale = output->base.current_scale;
>> -
>> return 0;
>>
>> -err_output:
>> - weston_output_destroy(&output->base);
>> err_free:
>> - wl_list_for_each_safe(drm_mode, next, &output->base.mode_list,
>> - base.link) {
>> - wl_list_remove(&drm_mode->base.link);
>> - free(drm_mode);
>> + drmModeFreeProperty(output->dpms_prop);
>> +
>> + return -1;
>> +}
>> +
>> +static void
>> +drm_output_deinit(struct weston_output *base)
>> +{
>> + struct drm_output *output = to_drm_output(base);
>> + struct drm_backend *b = to_drm_backend(base->compositor);
>> +
>> + if (b->use_pixman) {
>> + drm_output_fini_pixman(output);
>> + } else {
>> + gl_renderer->output_destroy(&output->base);
>> + gbm_surface_destroy(output->gbm_surface);
>> }
>>
>> - drmModeFreeCrtc(output->original_crtc);
>> + weston_plane_release(&output->fb_plane);
>> + weston_plane_release(&output->cursor_plane);
>> +
>> + drmModeFreeProperty(output->dpms_prop);
>> +
>> + /* Turn off hardware cursor */
>> + drmModeSetCursor(b->drm.fd, output->crtc_id, 0, 0, 0);
>> +}
>> +
>> +static void
>> +drm_output_destroy(struct weston_output *base)
>> +{
>> + struct drm_output *output = to_drm_output(base);
>> + struct drm_backend *b = to_drm_backend(base->compositor);
>> + drmModeCrtcPtr origcrtc = output->original_crtc;
>> +
>> + if (output->page_flip_pending) {
>> + output->destroy_pending = 1;
>> + weston_log("destroy output while page flip pending\n");
>> + return;
>> + }
>> +
>> + if (output->base.enabled)
>> + drm_output_deinit(&output->base);
>> +
>> + if (origcrtc) {
>> + /* Restore original CRTC state */
>> + drmModeSetCrtc(b->drm.fd, origcrtc->crtc_id,
>> origcrtc->buffer_id,
>> + origcrtc->x, origcrtc->y,
>> + &output->connector_id, 1, &origcrtc->mode);
>> + drmModeFreeCrtc(origcrtc);
>> + }
>> +
>> + weston_output_destroy(&output->base);
>> +
>> + drmModeFreeConnector(output->connector);
>> +
>> + if (output->backlight)
>> + backlight_destroy(output->backlight);
>
> I think nothing is releasing the mode list constructed in
> drm_output_set_mode(), but that was the case also before your patches,
> so it's not for this patch to fix. It could be a follow-up.
>
Alright.
>> +
>> b->crtc_allocator &= ~(1 << output->crtc_id);
>> b->connector_allocator &= ~(1 << output->connector_id);
>> +
>> free(output);
>> - free(config.modeline);
>> +}
>>
>> - return -1;
>> +static int
>> +drm_output_disable(struct weston_output *base)
>> +{
>> + struct drm_output *output = to_drm_output(base);
>> + struct drm_backend *b = to_drm_backend(base->compositor);
>> +
>> + if (output->page_flip_pending) {
>> + output->disable_pending = 1;
>> + weston_log("disable output while page flip pending\n");
>
> Disable while page flip pending shouldn't be a problem, because the
> compositor continues to run, and when the page flip event arrives, the
> handler there will finish the disabling. So I think this message is not
> needed.
>
Sure.
> Destroy while page flip pending is a different matter, since it can
> happen during shutdown.
>
>> + return -1;
>> + }
>> +
>> + if (output->base.enabled)
>> + drm_output_deinit(&output->base);
>> +
>> + output->disable_pending = 0;
>> +
>> + weston_log("Disabling output %s\n", output->base.name);
>> + drmModeSetCrtc(b->drm.fd, output->crtc_id,
>> + 0, 0, 0, 0, 0, NULL);
>
> I was a bit surprised by these two statements here, but then I saw that
> the page flip handler calls weston_output_disable() again, so also the
> page flip path will end up here eventually. All good.
>
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * Create and configure a Weston output structure
>> + *
>> + * Given a DRM connector, create a matching drm_output structure and add it
>> + * to Weston's output list.
>> + *
>> + * @param b Weston backend structure structure
>> + * @param resources DRM resources for this device
>> + * @param connector DRM connector to use for this new output
>
> Would be worth documenting that this function takes the ownership of
> 'connector' if it succeeds. It is fairly rare to do that.
>
>
Sure.
>> + * @param drm_device udev device pointer
>> + * @returns 0 on success, or -1 on failure
>> + */
>> +static int
>> +create_output_for_connector(struct drm_backend *b,
>> + drmModeRes *resources,
>> + drmModeConnector *connector,
>> + struct udev_device *drm_device)
>> +{
>> + struct drm_output *output;
>> + int i;
>> +
>> + i = find_crtc_for_connector(b, resources, connector);
>> + if (i < 0) {
>> + weston_log("No usable crtc/encoder pair for connector.\n");
>> + return -1;
>> + }
>> +
>> + output = zalloc(sizeof *output);
>> + if (output == NULL)
>> + return -1;
>> +
>> + output->connector = connector;
>> + output->crtc_id = resources->crtcs[i];
>> + output->pipe = i;
>> + output->connector_id = connector->connector_id;
>> +
>> + output->backlight = backlight_init(drm_device,
>> + connector->connector_type);
>> +
>> + output->base.enable = drm_output_enable;
>> + output->base.destroy = drm_output_destroy;
>> + output->base.disable = drm_output_disable;
>> + output->base.name = make_connector_name(connector);
>> +
>> + output->destroy_pending = 0;
>> + output->disable_pending = 0;
>> + output->original_crtc = NULL;
>> +
>> + b->crtc_allocator |= (1 << output->crtc_id);
>> + b->connector_allocator |= (1 << output->connector_id);
>> +
>> + weston_output_init_pending(&output->base, b->compositor);
>> + weston_compositor_add_pending_output(&output->base, b->compositor);
>> +
>> + return 0;
>> }
>>
>> static void
>> @@ -2578,7 +2644,6 @@ create_outputs(struct drm_backend *b, uint32_t
>> option_connector,
>> drmModeConnector *connector;
>> drmModeRes *resources;
>> int i;
>> - int x = 0, y = 0;
>>
>> resources = drmModeGetResources(b->drm.fd);
>> if (!resources) {
>> @@ -2610,21 +2675,15 @@ create_outputs(struct drm_backend *b, uint32_t
>> option_connector,
>> (option_connector == 0 ||
>> connector->connector_id == option_connector)) {
>> if (create_output_for_connector(b, resources,
>> - connector, x, y,
>> - drm_device) < 0) {
>> + connector, drm_device)
>> < 0) {
>> drmModeFreeConnector(connector);
---^
>> continue;
>> }
>> -
>> - x += container_of(b->compositor->output_list.prev,
>> - struct weston_output,
>> - link)->width;
>> }
>> -
>> - drmModeFreeConnector(connector);
>
> The Free should be moved into an else-branch instead of removed,
> shouldn't it?
>
>
There's already one above (when if fails). This one would run after
create_output_for_connector has been ran (since previously the
connector wasn't preserved).
>> }
>>
>> - if (wl_list_empty(&b->compositor->output_list))
>> + if (wl_list_empty(&b->compositor->output_list) &&
>> + wl_list_empty(&b->compositor->pending_output_list))
>> weston_log("No currently active connector found.\n");
>>
>> drmModeFreeResources(resources);
>> @@ -2638,7 +2697,6 @@ update_outputs(struct drm_backend *b, struct
>> udev_device *drm_device)
>> drmModeConnector *connector;
>> drmModeRes *resources;
>> struct drm_output *output, *next;
>> - int x = 0, y = 0;
>> uint32_t connected = 0, disconnects = 0;
>> int i;
>>
>> @@ -2664,23 +2722,11 @@ update_outputs(struct drm_backend *b, struct
>> udev_device *drm_device)
>> connected |= (1 << connector_id);
>>
>> if (!(b->connector_allocator & (1 << connector_id))) {
>> - struct weston_output *last =
>> - container_of(b->compositor->output_list.prev,
>> - struct weston_output, link);
>> -
>> - /* XXX: not yet needed, we die with 0 outputs */
>> - if (!wl_list_empty(&b->compositor->output_list))
>> - x = last->x + last->width;
>> - else
>> - x = 0;
>> - y = 0;
>> create_output_for_connector(b, resources,
>> - connector, x, y,
>> - drm_device);
>> + connector, drm_device);
>> weston_log("connector %d connected\n", connector_id);
>>
>> }
>> - drmModeFreeConnector(connector);
>
> drmModeFreeConnector() should stay but in a new else-branch, right?
>
>
>> }
>> drmModeFreeResources(resources);
>>
>> @@ -2695,6 +2741,16 @@ update_outputs(struct drm_backend *b, struct
>> udev_device *drm_device)
>> drm_output_destroy(&output->base);
>> }
>> }
>> +
>> + wl_list_for_each_safe(output, next,
>> &b->compositor->pending_output_list,
>> + base.link) {
>> + if (disconnects & (1 << output->connector_id)) {
>> + disconnects &= ~(1 << output->connector_id);
>> + weston_log("connector %d disconnected\n",
>> + output->connector_id);
>> + drm_output_destroy(&output->base);
>> + }
>> + }
>> }
>> }
>>
>> @@ -3078,6 +3134,12 @@ renderer_switch_binding(struct weston_keyboard
>> *keyboard, uint32_t time,
>> switch_to_gl_renderer(b);
>> }
>>
>> +static const struct weston_drm_output_api api = {
>> + drm_output_set_mode,
>> + drm_output_set_gbm_format,
>> + drm_output_set_seat,
>> +};
>> +
>> static struct drm_backend *
>> drm_backend_create(struct weston_compositor *compositor,
>> struct weston_drm_backend_config *config)
>> @@ -3087,6 +3149,7 @@ drm_backend_create(struct weston_compositor
>> *compositor,
>> struct wl_event_loop *loop;
>> const char *path;
>> const char *seat_id = default_seat;
>> + int ret;
>>
>> weston_log("initializing drm backend\n");
>>
>> @@ -3107,7 +3170,6 @@ drm_backend_create(struct weston_compositor
>> *compositor,
>> b->sprites_are_broken = 1;
>> b->compositor = compositor;
>> b->use_pixman = config->use_pixman;
>> - b->configure_output = config->configure_output;
>> b->use_current_mode = config->use_current_mode;
>>
>> if (parse_gbm_format(config->gbm_format, GBM_FORMAT_XRGB8888,
>> &b->gbm_format) < 0)
>> @@ -3230,6 +3292,14 @@ drm_backend_create(struct weston_compositor
>> *compositor,
>>
>> compositor->backend = &b->base;
>>
>> + ret = weston_plugin_api_register(compositor, WESTON_DRM_OUTPUT_API_NAME,
>> + &api, sizeof(api));
>> +
>> + if (ret < 0) {
>> + weston_log("Failed to register output API.\n");
>> + goto err_udev_monitor;
>> + }
>> +
>> return b;
>>
>> err_udev_monitor:
>> diff --git a/libweston/compositor-drm.h b/libweston/compositor-drm.h
>> index 1266031..8f89a2b 100644
>> --- a/libweston/compositor-drm.h
>> +++ b/libweston/compositor-drm.h
>> @@ -29,12 +29,13 @@
>> #define WESTON_COMPOSITOR_DRM_H
>>
>> #include "compositor.h"
>> +#include "plugin-registry.h"
>>
>> #ifdef __cplusplus
>> extern "C" {
>> #endif
>>
>> -#define WESTON_DRM_BACKEND_CONFIG_VERSION 1
>> +#define WESTON_DRM_BACKEND_CONFIG_VERSION 2
>>
>> struct libinput_device;
>>
>> @@ -51,8 +52,17 @@ enum weston_drm_backend_output_mode {
>> WESTON_DRM_BACKEND_OUTPUT_PREFERRED,
>> };
>>
>> -struct weston_drm_backend_output_config {
>> - struct weston_backend_output_config base;
>> +#define WESTON_DRM_OUTPUT_API_NAME "weston_drm_output_api_v1"
>> +
>> +struct weston_drm_output_api {
>> + /** The mode to be used by the output. Refer to the documentation
>> + * of WESTON_DRM_BACKEND_OUTPUT_PREFERRED for details.
>> + *
>> + * Returns 0 on success, -1 on failure.
>> + */
>> + int (*set_mode)(struct weston_output *output,
>> + enum weston_drm_backend_output_mode mode,
>> + const char *modeline);
>>
>> /** The pixel format to be used by the output. Valid values are:
>> * - NULL - The format set at backend creation time will be used;
>> @@ -60,15 +70,26 @@ struct weston_drm_backend_output_config {
>> * - "rgb565"
>> * - "xrgb2101010"
>> */
>> - char *gbm_format;
>> + void (*set_gbm_format)(struct weston_output *output,
>> + const char *gbm_format);
>> +
>> /** The seat to be used by the output. Set to NULL to use the
>> - * default seat. */
>> - char *seat;
>> - /** The modeline to be used by the output. Refer to the documentation
>> - * of WESTON_DRM_BACKEND_OUTPUT_PREFERRED for details. */
>> - char *modeline;
>> + * default seat.
>> + */
>> + void (*set_seat)(struct weston_output *output,
>> + const char *seat);
>> };
>>
>> +static inline const struct weston_drm_output_api *
>> +weston_drm_output_get_api(struct weston_compositor *compositor)
>> +{
>> + const void *api;
>> + api = weston_plugin_api_get(compositor, WESTON_DRM_OUTPUT_API_NAME,
>> + sizeof(struct weston_drm_output_api));
>> +
>> + return (const struct weston_drm_output_api *)api;
>> +}
>> +
>> /** The backend configuration struct.
>> *
>> * weston_drm_backend_config contains the configuration used by a DRM
>> @@ -109,17 +130,6 @@ struct weston_drm_backend_config {
>> */
>> char *gbm_format;
>>
>> - /** Callback used to configure the outputs.
>> - *
>> - * This function will be called by the backend when a new DRM
>> - * output needs to be configured.
>> - */
>> - enum weston_drm_backend_output_mode
>> - (*configure_output)(struct weston_compositor *compositor,
>> - bool use_current_mode,
>> - const char *name,
>> - struct weston_drm_backend_output_config
>> *output_config);
>> -
>> /** Callback used to configure input devices.
>> *
>> * This function will be called by the backend when a new input device
>
> Nice.
>
>
> Thanks,
> pq
>
Thanks for the review.
signature.asc
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
