[PATCH weston 07/14 v3] weston: Port RDP backend to new output handling API
Pekka Paalanen
ppaalanen at gmail.com
Fri Sep 16 13:49:10 UTC 2016
On Thu, 18 Aug 2016 18:42:35 +0200
Armin Krezović <krezovic.armin at gmail.com> wrote:
> This is a complete port of the RDP 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 command line
> 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:
>
> - Rename output_configure() to output_set_size()
> in plugin API and describe it.
> - Manually fetch parsed_options from wet_compositor.
> - Call rdp_output_disable() explicitly from
> rdp_output_destroy().
>
> v3:
>
> - Disallow calling rdp_output_set_size more than once.
> - Manually assign a hardcoded name to an output as that's
> now mandatory.
> - Use weston_compositor_add_pending_output().
> - Bump weston_rdp_backend_config version to 2.
>
> Reviewed-by: Quentin Glidic <sardemff7+git at sardemff7.net>
> Signed-off-by: Armin Krezović <krezovic.armin at gmail.com>
> ---
> compositor/main.c | 52 +++++++++++++++--
> libweston/compositor-rdp.c | 138 +++++++++++++++++++++++++++++++--------------
> libweston/compositor-rdp.h | 26 ++++++++-
> 3 files changed, 167 insertions(+), 49 deletions(-)
>
Hi Armin,
this patch looks essentially good, so:
Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
I did make a couple of comments below that would be nice to address
later.
I am also slightly concerned that this patch might introduce a hard to
lose race during start-up, but I haven't verified it. It's about the
output initialization vs. accepting RDP clients, I see that the
backend->output is being used in a few functions. Not quite sure if it
could get used before it gets initialized, even when main.c enables the
output ASAP. Hmm... but we probably won't service the listening socket
before all init is done, so maybe it's not an issue.
> diff --git a/compositor/main.c b/compositor/main.c
> index 12f5e76..7007901 100644
> --- a/compositor/main.c
> +++ b/compositor/main.c
> diff --git a/libweston/compositor-rdp.c b/libweston/compositor-rdp.c
> index ee81300..b34024a 100644
> --- a/libweston/compositor-rdp.c
> +++ b/libweston/compositor-rdp.c
> @@ -25,6 +25,7 @@
>
> #include "config.h"
>
> +#include <assert.h>
> #include <stdint.h>
> #include <stdlib.h>
> #include <string.h>
> @@ -371,15 +372,6 @@ rdp_output_repaint(struct weston_output *output_base, pixman_region32_t *damage)
> return 0;
> }
>
> -static void
> -rdp_output_destroy(struct weston_output *output_base)
> -{
> - struct rdp_output *output = to_rdp_output(output_base);
> -
> - wl_event_source_remove(output->finish_frame_timer);
> - free(output);
> -}
> -
> static int
> finish_frame_handler(void *data)
> {
> @@ -471,16 +463,15 @@ rdp_switch_mode(struct weston_output *output, struct weston_mode *target_mode)
> }
>
> static int
> -rdp_backend_create_output(struct rdp_backend *b, int width, int height)
> +rdp_output_set_size(struct weston_output *base,
> + int width, int height)
> {
> - struct rdp_output *output;
> - struct wl_event_loop *loop;
> + struct rdp_output *output = to_rdp_output(base);
> struct weston_mode *currentMode;
> struct weston_mode initMode;
>
> - output = zalloc(sizeof *output);
> - if (output == NULL)
> - return -1;
> + /* We can only be called once. */
> + assert(!output->base.current_mode);
>
> wl_list_init(&output->peers);
> wl_list_init(&output->base.mode_list);
> @@ -492,48 +483,100 @@ rdp_backend_create_output(struct rdp_backend *b, int width, int height)
>
> currentMode = ensure_matching_mode(&output->base, &initMode);
> if (!currentMode)
> - goto out_free_output;
> + return -1;
>
> output->base.current_mode = output->base.native_mode = currentMode;
> - weston_output_init(&output->base, b->compositor, 0, 0, width, height,
> - WL_OUTPUT_TRANSFORM_NORMAL, 1);
> -
> output->base.make = "weston";
> output->base.model = "rdp";
> +
> + /* XXX: Calculate proper size. */
> + output->base.mm_width = width;
> + output->base.mm_height = height;
> +
> + output->base.start_repaint_loop = rdp_output_start_repaint_loop;
> + output->base.repaint = rdp_output_repaint;
> + output->base.assign_planes = NULL;
> + output->base.set_backlight = NULL;
> + output->base.set_dpms = NULL;
> + output->base.switch_mode = rdp_switch_mode;
I'm wondering why setting these vfuncs is here in set_size rather than
in rdp_output_enable() or even rdp_backend_create_output(). It doesn't
feel logical here, but works as well so, whatever.
Ideally the setters would just set some fields and not much else, then
enable() would do all the real work. This comment applies to all
backends.
> +
> + return 0;
> +}
> +
> +static int
> +rdp_output_enable(struct weston_output *base)
> +{
> + struct rdp_output *output = to_rdp_output(base);
> + struct rdp_backend *b = to_rdp_backend(base->compositor);
> + struct wl_event_loop *loop;
> +
> output->shadow_surface = pixman_image_create_bits(PIXMAN_x8r8g8b8,
> - width, height,
> - NULL,
> - width * 4);
> + output->base.current_mode->width,
> + output->base.current_mode->height,
> + NULL,
> + output->base.current_mode->width * 4);
> if (output->shadow_surface == NULL) {
> weston_log("Failed to create surface for frame buffer.\n");
> - goto out_output;
> + return -1;
> }
>
> - if (pixman_renderer_output_create(&output->base) < 0)
> - goto out_shadow_surface;
> + if (pixman_renderer_output_create(&output->base) < 0) {
> + pixman_image_unref(output->shadow_surface);
> + return -1;
> + }
>
> loop = wl_display_get_event_loop(b->compositor->wl_display);
> output->finish_frame_timer = wl_event_loop_add_timer(loop, finish_frame_handler, output);
>
> - output->base.start_repaint_loop = rdp_output_start_repaint_loop;
> - output->base.repaint = rdp_output_repaint;
> - output->base.destroy = rdp_output_destroy;
> - output->base.assign_planes = NULL;
> - output->base.set_backlight = NULL;
> - output->base.set_dpms = NULL;
> - output->base.switch_mode = rdp_switch_mode;
> b->output = output;
>
> - weston_compositor_add_output(b->compositor, &output->base);
> return 0;
> +}
> +
> +static int
> +rdp_output_disable(struct weston_output *base)
> +{
> + struct rdp_output *output = to_rdp_output(base);
> + struct rdp_backend *b = to_rdp_backend(base->compositor);
> +
> + if (!output->base.enabled)
> + return 0;
> +
> + wl_event_source_remove(output->finish_frame_timer);
> + b->output = NULL;
>
> -out_shadow_surface:
> - pixman_image_unref(output->shadow_surface);
> -out_output:
> + return 0;
> +}
> +
> +static void
> +rdp_output_destroy(struct weston_output *base)
> +{
> + struct rdp_output *output = to_rdp_output(base);
> +
> + rdp_output_disable(&output->base);
> weston_output_destroy(&output->base);
> -out_free_output:
> +
> free(output);
> - return -1;
> +}
> +
> +static int
> +rdp_backend_create_output(struct weston_compositor *compositor)
> +{
> + struct rdp_output *output;
> +
> + output = zalloc(sizeof *output);
> + if (output == NULL)
> + return -1;
> +
> + output->base.name = strdup("rdp");
> + output->base.destroy = rdp_output_destroy;
> + output->base.disable = rdp_output_disable;
> + output->base.enable = rdp_output_enable;
> +
> + weston_output_init_pending(&output->base, compositor);
> + weston_compositor_add_pending_output(&output->base, compositor);
> +
> + return 0;
> }
>
> static void
> @@ -1216,6 +1259,10 @@ rdp_incoming_peer(freerdp_listener *instance, freerdp_peer *client)
> FREERDP_CB_RETURN(TRUE);
> }
>
> +static const struct weston_rdp_output_api api = {
> + rdp_output_set_size,
> +};
> +
> static struct rdp_backend *
> rdp_backend_create(struct weston_compositor *compositor,
> struct weston_rdp_backend_config *config)
> @@ -1223,7 +1270,7 @@ rdp_backend_create(struct weston_compositor *compositor,
> struct rdp_backend *b;
> char *fd_str;
> char *fd_tail;
> - int fd;
> + int fd, ret;
>
> b = zalloc(sizeof *b);
> if (b == NULL)
> @@ -1251,7 +1298,7 @@ rdp_backend_create(struct weston_compositor *compositor,
> if (pixman_renderer_init(compositor) < 0)
> goto err_compositor;
>
> - if (rdp_backend_create_output(b, config->width, config->height) < 0)
> + if (rdp_backend_create_output(compositor) < 0)
> goto err_compositor;
>
> compositor->capabilities |= WESTON_CAP_ARBITRARY_MODES;
> @@ -1282,6 +1329,15 @@ rdp_backend_create(struct weston_compositor *compositor,
> }
>
> compositor->backend = &b->base;
> +
> + ret = weston_plugin_api_register(compositor, WESTON_RDP_OUTPUT_API_NAME,
> + &api, sizeof(api));
> +
> + if (ret < 0) {
> + weston_log("Failed to register output API.\n");
> + goto err_output;
> + }
When looking at this, I realized something. You create the pending
outputs first, and only afterward you register the output API.
Registering pending outputs would already call output configuration
handlers which would then try to use the output API before it has been
registered. However, main.c adds the pending output handler only after
loading backends, so this doesn't happen. Then main.c needs to call
weston_pending_output_coldplug() to trigger the handler calls.
Any ideas how to untangle this in the future?
Would it perhaps be possible to reorder things so that main.c registers
the handler first, then loads the backend which will already call the
handler for all initial outputs, making coldplug call unnecessary?
That could be looked into after landing this series.
It is possible for a libweston user to trigger that ordering already,
it's just that main.c avoids it.
At this point I'd rather merge this series as is than review another
extensive revision if it, since I'd be looking at the diff anyway.
Thanks,
pq
> +
> return b;
>
> err_listener:
-------------- 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/20160916/0c657fcb/attachment.sig>
More information about the wayland-devel
mailing list