[PATCH weston 07/14 v3] weston: Port RDP backend to new output handling API
Armin Krezović
krezovic.armin at gmail.com
Sat Sep 17 13:05:06 UTC 2016
On 16.09.2016 15:49, Pekka Paalanen wrote:
> 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,
>
Salut.
> this patch looks essentially good, so:
> Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>
Thanks!
> 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.
>
In my initial testing I didn't notice any issues. Lets not try to fix what
isn't broken yet.
>> 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.
>
We can move them wherever agreed upon, not a big deal.
>> +
>> + 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.
>
The trick is: Backend needs to be fully initialized before the API can
be registered, and outputs need to be created after that (if backends
do that), but (at least) DRM backend can't create an output after
backend initialization, as something that happens during initialization
also requires the initial output creation to be done at that point.
Investigation pending.
For wayland hotplug and this backend, output creation can be moved after
backend initialization.
>
> Thanks,
> pq
>
>> +
>> return b;
>>
>> err_listener:
-------------- 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/20160917/a3c0113d/attachment-0001.sig>
More information about the wayland-devel
mailing list