[PATCH weston 04/14 v3] weston: Port DRM backend to new output handling API

Armin Krezović krezovic.armin at gmail.com
Wed Sep 14 09:50:34 UTC 2016


On 13.09.2016 12:49, Pekka Paalanen wrote:
> On Thu, 18 Aug 2016 18:42:32 +0200
> Armin Krezović <krezovic.armin at 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.armin at 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 = &current->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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 837 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160914/821a04b2/attachment-0001.sig>


More information about the wayland-devel mailing list