[PATCH weston v3] clients: Add XKB compose key support

2016-10-10 Thread Bryce Harrington
This adds single-symbol compose support using libxkbcommon's compose
functionality.  E.g., assuming you have the right alt key defined as
your compose key, typing +i+' will produce í, and +y+= will
produce ¥.  This makes compose key work for weston-editor,
weston-terminal, weston-eventdemo, and any other clients that use
Weston's window.* routines for accepting and managing keyboard input.

Compose sequences are loaded from the system's standard tables.  As
well, libxkbcommon will transparently load custom sequences from the
user's ~/.XCompose file.

Note that due to limitations in toytoolkit's key handler interface, only
compose sequences resulting in single symbols are supported.  While
libxkbcommon supports multi-symbol compose strings, support for passing
text buffers to Weston clients is left as future work.

This largely obviates the need for the weston-simple-im input method
client, which had provided a very limited compose functionality that was
only available in clients implementing the zwp_input_method protocol,
and with no mechanism to load system or user-specified compose keys.

Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=53648
Signed-off-by: Bryce Harrington 
Reviewed-by: Daniel Stone 

Signed-off-by: Bryce Harrington 
---
v3:  Don't die if compose can't be established
 Format a couple lengthy lines to 80 columns
 Also check LC_CTYPE and LANG for locale settings

 clients/window.c | 66 
 1 file changed, 66 insertions(+)

diff --git a/clients/window.c b/clients/window.c
index 216ef96..1c53b5f 100644
--- a/clients/window.c
+++ b/clients/window.c
@@ -63,6 +63,7 @@ typedef void *EGLContext;
 #endif /* no HAVE_CAIRO_EGL */
 
 #include 
+#include 
 #include 
 
 #include 
@@ -372,6 +373,8 @@ struct input {
struct {
struct xkb_keymap *keymap;
struct xkb_state *state;
+   struct xkb_compose_table *compose_table;
+   struct xkb_compose_state *compose_state;
xkb_mod_mask_t control_mask;
xkb_mod_mask_t alt_mask;
xkb_mod_mask_t shift_mask;
@@ -2979,6 +2982,9 @@ keyboard_handle_keymap(void *data, struct wl_keyboard 
*keyboard,
struct input *input = data;
struct xkb_keymap *keymap;
struct xkb_state *state;
+   struct xkb_compose_table *compose_table;
+   struct xkb_compose_state *compose_state;
+   char *locale;
char *map_str;
 
if (!data) {
@@ -2997,6 +3003,7 @@ keyboard_handle_keymap(void *data, struct wl_keyboard 
*keyboard,
return;
}
 
+   /* Set up XKB keymap */
keymap = xkb_keymap_new_from_string(input->display->xkb_context,
map_str,
XKB_KEYMAP_FORMAT_TEXT_V1,
@@ -3009,6 +3016,7 @@ keyboard_handle_keymap(void *data, struct wl_keyboard 
*keyboard,
return;
}
 
+   /* Set up XKB state */
state = xkb_state_new(keymap);
if (!state) {
fprintf(stderr, "failed to create XKB state\n");
@@ -3016,6 +3024,37 @@ keyboard_handle_keymap(void *data, struct wl_keyboard 
*keyboard,
return;
}
 
+   /* Look up the preferred locale, falling back to "C" as default */
+   if (!(locale = getenv("LC_ALL")))
+   if (!(locale = getenv("LC_CTYPE")))
+   if (!(locale = getenv("LANG")))
+   locale = "C";
+
+   /* Set up XKB compose table */
+   compose_table =
+   xkb_compose_table_new_from_locale(input->display->xkb_context,
+ locale,
+ XKB_COMPOSE_COMPILE_NO_FLAGS);
+   if (compose_table) {
+   /* Set up XKB compose state */
+   compose_state = xkb_compose_state_new(compose_table,
+ XKB_COMPOSE_STATE_NO_FLAGS);
+   if (compose_state) {
+   xkb_compose_state_unref(input->xkb.compose_state);
+   xkb_compose_table_unref(input->xkb.compose_table);
+   input->xkb.compose_state = compose_state;
+   input->xkb.compose_table = compose_table;
+   } else {
+   fprintf(stderr, "could not create XKB compose state.  "
+   "Disabiling compose.\n");
+   xkb_compose_table_unref(compose_table);
+   compose_table = NULL;
+   }
+   } else {
+   fprintf(stderr, "could not create XKB compose table for locale 
'%s'.  "
+   "Disabiling compose\n", locale);
+   }
+
xkb_keymap_unref(input->xkb.keymap);
xkb_state_unref(input->xkb.state);

Re: [PATCH weston v2] clients: Add a weston-autorotator client and rotator protocol

2016-10-10 Thread Pekka Paalanen
On Mon,  8 Aug 2016 15:41:04 +0100
Emmanuel Gil Peyrot  wrote:

> This client uses libiio to retrieve accelerometer values from the iio
> subsystem on Linux (and maybe some other kernels), and automatically
> rotate the output whenever orientation changed.
> 
> I tested it with a mma8453 accelerometer, but everything needed should
> be available in the configuration to make it work with any other iio
> device.
> 
> v2:
> - Rename rotater to rotator, which is actual English.
> - Add documentation for every new option, and a default value.
> - Add the bare minimum of those options in the sample weston.ini.
> - Fix for an inaccurate sampling frequency due to integer downcasting.
> - Make the server-side a module instead of a simple function that has to be
>   called by every shell.
> - Remove the mostly-useless smoothing of the captured input.
> - Make the protocol take an accelerometer name, to let the rotator
>   match its name with the outputs using it.
> 
> Signed-off-by: Emmanuel Gil Peyrot 
> ---
>  .gitignore  |   1 +
>  Makefile.am |  28 +++
>  clients/autorotator.c   | 435 
> 
>  compositor/rotator.c| 246 +
>  configure.ac|  15 ++
>  libweston/compositor.c  |   1 +
>  libweston/compositor.h  |   1 +
>  man/weston.ini.man  |  41 +
>  protocol/weston-rotator.xml |  26 +++
>  weston.ini.in   |   4 +
>  10 files changed, 798 insertions(+)
>  create mode 100644 clients/autorotator.c
>  create mode 100644 compositor/rotator.c
>  create mode 100644 protocol/weston-rotator.xml

Hi,

this rebases to current master with just a couple trivial conflicts.

The architecture looks good now, with the justification from last time.
All the comments below are implementation details, the biggest of them
being the need for a proper interface to change the transform of a
weston_output.

There is nothing even packaged in Gentoo with 'iio' in its name, so
people probably won't build-test this stuff often. Where does one get
libiio?

Maybe mention the URL in at least the commit message or even in the
configure.ac error.


> diff --git a/.gitignore b/.gitignore
> index 41a140b..72cea9f 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -51,6 +51,7 @@ protocol/*.[ch]
>  
>  00*.patch
>  
> +weston-autorotator
>  weston-calibrator
>  weston-clickdot
>  weston-cliptest
> diff --git a/Makefile.am b/Makefile.am
> index 32627f5..0e990ab 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -124,6 +124,8 @@ endif
>  nodist_libweston_@LIBWESTON_MAJOR@_la_SOURCES =  
> \
>   protocol/weston-screenshooter-protocol.c\
>   protocol/weston-screenshooter-server-protocol.h \
> + protocol/weston-rotator-protocol.c  \
> + protocol/weston-rotator-server-protocol.h   \

These would belong in nodist_rotator_la_SOURCES.


>   protocol/text-cursor-position-protocol.c\
>   protocol/text-cursor-position-server-protocol.h \
>   protocol/text-input-unstable-v1-protocol.c  \
> @@ -610,6 +612,32 @@ nodist_weston_screenshooter_SOURCES =
> \
>  weston_screenshooter_LDADD = $(CLIENT_LIBS) libshared.la
>  weston_screenshooter_CFLAGS = $(AM_CFLAGS) $(CLIENT_CFLAGS)
>  
> +if BUILD_AUTOROTATOR
> +libexec_PROGRAMS += weston-autorotator
> +
> +weston_autorotator_SOURCES = \
> + clients/autorotator.c
> +nodist_weston_autorotator_SOURCES =  \
> + protocol/weston-rotator-protocol.c  \
> + protocol/weston-rotator-client-protocol.h
> +weston_autorotator_LDADD = $(CLIENT_LIBS) $(LIBIIO_LIBS) libshared.la
> +weston_autorotator_CFLAGS = $(AM_CFLAGS) $(CLIENT_CFLAGS) $(LIBIIO_CFLAGS)
> +
> +module_LTLIBRARIES += rotator.la
> +rotator_la_LDFLAGS = -module -avoid-version
> +rotator_la_LIBADD = $(COMPOSITOR_LIBS) libshared.la
> +rotator_la_CFLAGS = $(AM_CFLAGS) $(COMPOSITOR_CFLAGS)
> +rotator_la_SOURCES = \
> + compositor/rotator.c
> +
> +BUILT_SOURCES += \
> + protocol/weston-rotator-protocol.c  \
> + protocol/weston-rotator-client-protocol.h
> +
> +EXTRA_DIST +=\
> + protocol/weston-rotator.xml
> +endif

Not sure BUILT_SOURCES and EXTRA_DIST should be inside 'if'.
Distcheck does fail (when libiio not available).


> +
>  weston_terminal_SOURCES =\
>   clients/terminal.c  \
>   shared/helpers.h
> diff --git a/clients/autorotator.c b/clients/autorotator.c
> new file mode 100644
> index 000..89697c9
> --- /dev/null
> +++ b/clients/autorotator.c
> @@ -0,0 +1,435 @@
> +/*
> + * Copyright © 2016 Collabora, Ltd.

Is the copyright 

Re: [PATCH weston 03/10] compositor-wayland: Convert fullscreen flag to bool

2016-10-10 Thread Quentin Glidic

On 09/10/2016 17:30, Armin Krezović wrote:

Signed-off-by: Armin Krezović 


You are missing the compositor/main.c change in this one.

Patches 1, 2 and 4 are Rb me, and pushed:
00a03d2..2d321e3  master -> master

Cheers,



---
 libweston/compositor-wayland.c | 2 +-
 libweston/compositor-wayland.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/libweston/compositor-wayland.c b/libweston/compositor-wayland.c
index 32c44d4..a5a360c 100644
--- a/libweston/compositor-wayland.c
+++ b/libweston/compositor-wayland.c
@@ -77,7 +77,7 @@ struct wayland_backend {

bool use_pixman;
bool sprawl_across_outputs;
-   int fullscreen;
+   bool fullscreen;

struct theme *theme;
cairo_device_t *frame_device;
diff --git a/libweston/compositor-wayland.h b/libweston/compositor-wayland.h
index 3ca875a..d5c29f0 100644
--- a/libweston/compositor-wayland.h
+++ b/libweston/compositor-wayland.h
@@ -41,7 +41,7 @@ struct weston_wayland_backend_config {
bool use_pixman;
bool sprawl;
char *display_name;
-   int fullscreen;
+   bool fullscreen;
char *cursor_theme;
int cursor_size;
 };




--

Quentin “Sardem FF7” Glidic
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel


Re: [PATCH weston 4/4] compositor: Implement horizontal output layout configuration

2016-10-10 Thread Pekka Paalanen
On Sun, 9 Oct 2016 19:30:11 +0200
Armin Krezović  wrote:

> On 07.10.2016 12:19, Pekka Paalanen wrote:
> > On Fri, 30 Sep 2016 23:25:30 +0200
> > Armin Krezović  wrote:
> >   
> >> This patch adds horizontal output layout configuration using
> >> the previously added code.
> >>
> >> When an output is added, it looks for outputs that it should
> >> be placed next to, if they are specified. If such output is
> >> found, output layout is updated if needed (outputs on a given
> >> position are moved right if they collide with the new output)
> >> and the output is positioned next to the specified output,
> >> either on its left or its right.
> >>
> >> If a certain position wasn't specified for the current output,
> >> the compositor looks for any existing outputs that have
> >> current output specified on that position, and updates the
> >> positions accordingly, if corresponding output is found.
> >>
> >> Output positions can only be set once, where config file
> >> specified option is always set, even if such output will
> >> never be present. If a certain position wasn't set, the
> >> compositor may update the position if it finds another
> >> output that wants to be placed next to the output that's
> >> being configured. In such case, first existing output that
> >> matches the position is picked up, rest is ignored.
> >>
> >> Default behaviour is still to place outputs that have
> >> no matching configuration top right.
> >>
> >> Signed-off-by: Armin Krezović 
> >> ---
> >>  compositor/main.c | 159 
> >> +++---
> >>  1 file changed, 153 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/compositor/main.c b/compositor/main.c
> >> index e379d8d..c039ae6 100644
> >> --- a/compositor/main.c
> >> +++ b/compositor/main.c
> >> @@ -480,20 +480,167 @@ wet_init_parsed_options(struct weston_compositor 
> >> *ec)
> >>return config;
> >>  }
> >>  
> >> +static struct wet_output *
> >> +wet_output_from_output(struct wet_compositor *compositor, struct 
> >> weston_output *output)
> >> +{
> >> +  struct wet_output *iterator, *next;
> >> +
> >> +  if (wl_list_empty(>output_layout_list))
> >> +  return NULL;
> >> +
> >> +  wl_list_for_each_safe(iterator, next, >output_layout_list, 
> >> link) {
> >> +  if (iterator->output == output)
> >> +  return iterator;
> >> +  }  
> > 
> > Hi,
> > 
> > wet_output uses a destroy listener on the output, so use that:
> > 
> > listener = wl_signal_get(>destroy_signal, handle_output_destroy);
> > wet_output = container_of(...);
> >   
> 
> Thanks for the hint.
> 
> >> +
> >> +  return NULL;
> >> +}
> >> +
> >> +static struct wet_output *
> >> +wet_output_from_name(struct wet_compositor *compositor, const char *name)
> >> +{
> >> +  struct wet_output *iterator, *next;
> >> +
> >> +  if (wl_list_empty(>output_layout_list))
> >> +  return NULL;
> >> +
> >> +  wl_list_for_each_safe(iterator, next, >output_layout_list, 
> >> link) {
> >> +  if (!strcmp(iterator->output->name, name))
> >> +  return iterator;
> >> +  }  
> > 
> > No need to check for empty list, and no need to use _safe. Applies
> > probably to all loop below, too.
> >   
> 
> I have been burned when using non-safe version in the past (getting
> random segfaults with non-safe version and not with safe version),
> so I keep on using it. I'll see if it's still needed.

Hi,

the _safe version is only safe against a single kind of use: removal of
the current element. If you remove any other element, it can explode
still. So you really should know when and which elements you are
removing. This is one of the nastiest traps with wl_list.

> >> +
> >> +  return NULL;
> >> +}
> >> +  
> > 
> > I kind of wonder how much sense it makes to do first just 1-D layout
> > and then later expand to 2-D layout. I fear the code re-use might be
> > non-existant.
> > 
> > If you are struggling with 2-D layout, maybe X.org could provide some
> > example?
> > 
> > I do not have a suggestion for a good layout algorithm or even a method
> > of description. People have been configuring X.org for ages, so
> > something similar might be familiar.
> > 
> > Positioning by absolute coordinates I would like to leave out if
> > possible, or at least you would need to check that:
> > - all outputs create a single connected region (so pointers can travel
> >   everywhere)
> > - no outputs overlap (IIRC this is a limitation of Weston's damage
> >   tracking)
> > 
> > 
> > Thanks,
> > pq
> >   
> 
> I have thought about 2D layout, but I have ran in a problem that I can't
> solve.
> 
> I'll try to illustrate.
> 
> 
> |||-|
> ||| |
> ||| |
>   |_|
> |--|
> |  |
> |  |
> |__|
> 
> Lets say I have an 800x600 output top left, and 1024x768 next to it.
> 

Re: [PATCH weston 3/4] compositor: Add internal output object used for layout configuration

2016-10-10 Thread Pekka Paalanen
On Sun, 9 Oct 2016 19:21:57 +0200
Armin Krezović  wrote:

> On 07.10.2016 12:08, Pekka Paalanen wrote:
> > On Fri, 30 Sep 2016 23:25:29 +0200
> > Armin Krezović  wrote:
> >   
> >> This adds weston specific output object, which contains information
> >> about output's position, relative to other outputs. DRM and Windowed
> >> backends code has been updated to make use of the new code and
> >> parse new configuration options for a given output, if it was
> >> specified.
> >>
> >> New configuration file options for [output] section have been added:
> >>
> >> left-of: Specifies which output should be on the right side of the
> >> current output.
> >>
> >> right-of: Specifies which output should be on the left side of the
> >> current output.
> >>
> >> Signed-off-by: Armin Krezović 
> >> ---
> >>  compositor/main.c | 81 
> >> +++
> >>  1 file changed, 81 insertions(+)
> >>
> >> diff --git a/compositor/main.c b/compositor/main.c
> >> index 503016e..e379d8d 100644
> >> --- a/compositor/main.c
> >> +++ b/compositor/main.c
> >> @@ -78,6 +78,18 @@ struct wet_compositor {
> >>struct weston_config *config;
> >>struct wet_output_config *parsed_options;
> >>struct wl_listener pending_output_listener;
> >> +  struct wl_list output_layout_list;
> >> +};
> >> +
> >> +struct wet_output {
> >> +  struct weston_output *output;
> >> +  struct wl_listener output_destroy_listener;
> >> +  struct wl_list link;
> >> +
> >> +  char *left_output_name;
> >> +  char *right_output_name;  
> > 
> > Hi
> >   
> 
> Salut
> 
> > Rather than linking by name, would it not be easier to link by pointer
> > to struct wet_output?
> >   
> 
> I can try that, too.
> 
> > That means when you encounter a directive like "left-of=F5" and there
> > is no output F5 yet, you would create a new struct wet_output for F5
> > but leave the weston_output pointer NULL, to be filled out if F5
> > actually appears.
> > 
> > That means you would always have a graph in memory instead of doing
> > searches to find if output of this name actually exists. I'm not sure
> > if that's actually simpler in the end.
> >   
> 
> I'd still have to search for a wet_output matching an output when it
> gets attached (implement a way of matching an unclaimed wet_output to
> weston_output), and if it isn't there, I'll have to create it. That
> includes a bit of complexity too. How much however, I can't say
> until I try.
> 
> > Having the complete graph would allow you to "jump over" outputs that
> > do not exist at the moment: e.g. order F1 -> (F2) -> F3.
> >   
> 
> That could lead to waste of memory, as the output could never get attached,
> and I'll have to implement a function free-ing such outputs, as I can't rely
> on output destroyed signal as I currently do. Not to mention that the third
> output needs to be moved when second one gets attached, so it's not really
> a win-win situation, besides having a better picture in the memory.

But without that structure, how will you position output F3 right-of
F2, when F2 has not been connected? Where do you store the information
that F2 is right-of F1, so that you could look at where F1 is and use
that to position F3?

> We could accomplish that using wl_array with this implementation (instead
> of wl_list that's currently used).
> 
> In the end, the implementation looks more complex than this one, and
> we already configure outputs by name, so why not simply keep it?

Sorry?

I don't think any linear list or array is going to cut it, you can only
use a list (or array) as a collection without any meaning to the layout.

Is the 2D layout really that far away still that we really benefit from
the 1D layout already? It's the trade-off between reviewing and
polishing the 1D solution and getting that early to users then
replacing it with the 2D layout vs. going straight for 2D layout.

Or are you perhaps thinking that the 2D solution is just a 1D solution
in x-direction plus a 1D solution in y-direction, like a list a of rows
of outputs? I might not be that optimistic.


Thanks,
pq


pgpUHYWqCZdNR.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel


Re: [PATCH weston 2/4] weston: Move output position setting to compositor

2016-10-10 Thread Pekka Paalanen
On Sun, 9 Oct 2016 19:04:19 +0200
Armin Krezović  wrote:

> On 07.10.2016 11:58, Pekka Paalanen wrote:
> > On Fri, 30 Sep 2016 23:25:28 +0200
> > Armin Krezović  wrote:
> >   
> >> This moves current output positioning code and scale/transform
> >> application to the compositor itself, so the compositor
> >> can configure output layouts any way it wants.
> >>
> >> A helper function for setting x and y coordinates is also
> >> added, and couple of assertions to weston_output_enable()
> >> as well, to make sure everything has been set up.
> >>
> >> Signed-off-by: Armin Krezović 
> >> ---
> >>  compositor/main.c  | 26 ++
> >>  libweston/compositor.c | 40 ++--
> >>  libweston/compositor.h |  3 +++
> >>  3 files changed, 51 insertions(+), 18 deletions(-)  
> > 
> > Hi,
> > 
> > moving the output positioning out from libweston and into the
> > compositor is very good. The setter for x,y is good too.
> > 
> > I'm just not sure we should assume that outputs cannot occupy the
> > negative coordinates. If one hotplugs an output to the left, I'd assume
> > the existing windows would stay put on the right. If we cannot use
> > negative coordinates, everything has to be moved to keep them at the
> > same position from the user point of view.
> > 
> > 
> > Thanks,
> > pq
> >   
> 
> Hi,
> 
> I'd rather go for the second approach - move everything to the output it
> was on, in case an output gets attached on the left of an output that had
> something displayed (weston_output_move() can be easily adapted).

I wonder if that is possible in libweston core rather than delegating
to all shells individually.

See e.g. handle_output_move() in shell.c.

You are also going to have to move all input device coordinates, so
that the pointers stay on the same outputs as they used to, and
touch-points that are down do the same.

It doesn't sound easier to me.

Moving the global coordinate frame origin involves much more than just
moving outputs. If you only move outputs, your relative input devices
may warp away from the output they were on. I'm not sure how absolute
input devices like touch screens behave. So the question is, will the
user expect the warp or not?

There are cases when you cannot avoid moving existing outputs and that
is natural. But there are also cases where you could, if you were able
to use negative coordinates (or place the first output at 1M,1M instead
of 0,0).


Thanks,
pq


pgpXMYomjaGtL.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel


Re: [PATCH weston 1/4] libweston: Export weston_output_transform_scale_init

2016-10-10 Thread Pekka Paalanen
On Sun, 9 Oct 2016 19:01:12 +0200
Armin Krezović  wrote:

> On 07.10.2016 11:52, Pekka Paalanen wrote:
> > On Fri, 30 Sep 2016 23:25:27 +0200
> > Armin Krezović  wrote:
> >   
> >> This is required for implementing output layout setting
> >> which relies on current output width and height, and
> >> those are calculated in this function.
> >>
> >> It also changes the function signature to make use
> >> of already stored scale and transform values in the
> >> weston_output object.
> >>
> >> Signed-off-by: Armin Krezović 
> >> ---
> >>  libweston/compositor.c | 31 ++-
> >>  libweston/compositor.h |  2 ++
> >>  2 files changed, 20 insertions(+), 13 deletions(-)  
> > 
> > Hi,
> > 
> > I think this would need a bit more thought on the API.
> > 
> > The requirement is that one has to set some output parameters (video
> > mode, transform, scale) before the output size is available. Output
> > size then is needed for doing the layout.
> > 
> > IMO it would make more sense to add a new function
> > weston_output_get_size(), which would take into account the currently
> > set (pending) parameters and compute the resulting size.
> > 
> > This is also good in the long term since we would like to make struct
> > weston_output opaque to the libweston user. We are going to need a
> > getter for the size anyway.
> > 
> > 
> > Thanks,
> > pq
> >   
> 
> Hi,
> 
> It makes sense, yes. Do you think we'd need a similar function to get
> the current output coordinates (used for computing new position)?

Hi,

eventually we're going to need getters for everything as struct
weston_output should become opaque. What "everything" is is an open
question, but if you see use for something, then it probably needs a
getter.


Thanks,
pq


pgp0FQAgFkh0P.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel


Re: [PATCH weston v2 3/3] compositor-drm: Support linux_dmabuf output for sprite planes without gbm

2016-10-10 Thread Fabien DESSENNE
Hi,


Please find below a comment.


Fabien


On 09/30/2016 11:28 AM, Tomohito Esaki wrote:
> Multiplanar formats are supported by using drmModeAddFB2 and bypassing
> gbm. If drmModeAddFB2 isn't available, the existing gbm bo import path
> is used and multiplanar formats are unsupported.
>
> Signed-off-by: Tomohito Esaki 
> ---
>   libweston/compositor-drm.c | 53 
> +++---
>   1 file changed, 26 insertions(+), 27 deletions(-)
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index b15fa01..f0e6f7c 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -1008,11 +1008,9 @@ page_flip_handler(int fd, unsigned int frame,
>   
>   static uint32_t
>   drm_output_check_sprite_format(struct drm_sprite *s,
> -struct weston_view *ev, struct gbm_bo *bo)
> +struct weston_view *ev, uint32_t format)
>   {
> - uint32_t i, format;
> -
> - format = gbm_bo_get_format(bo);
> + uint32_t i;
>   
>   if (format == GBM_FORMAT_ARGB) {
>   pixman_region32_t r;
> @@ -1053,15 +1051,12 @@ drm_output_prepare_overlay_view(struct drm_output 
> *output,
>   struct drm_sprite *s;
>   struct linux_dmabuf_buffer *dmabuf;
>   int found = 0;
> - struct gbm_bo *bo;
> + struct gbm_bo *bo = NULL;
>   pixman_region32_t dest_rect, src_rect;
>   pixman_box32_t *box, tbox;
>   uint32_t format;
>   wl_fixed_t sx1, sy1, sx2, sy2;
>   
> - if (b->gbm == NULL)
> - return NULL;
> -
>   if (viewport->buffer.transform != output->base.transform)
>   return NULL;
>   
> @@ -1101,15 +1096,9 @@ drm_output_prepare_overlay_view(struct drm_output 
> *output,
>   if (!found)
>   return NULL;
>   
> - if ((dmabuf = linux_dmabuf_buffer_get(buffer_resource))) {
> + if ((dmabuf = linux_dmabuf_buffer_get(buffer_resource)) &&
> + b->no_addfb2 && b->gbm) {
>   #ifdef HAVE_GBM_FD_IMPORT
> - /* XXX: TODO:
> -  *
> -  * Use AddFB2 directly, do not go via GBM.
> -  * Add support for multiplanar formats.
> -  * Both require refactoring in the DRM-backend to
> -  * support a mix of gbm_bos and drmfbs.
> -  */
>   struct gbm_import_fd_data gbm_dmabuf = {
>   .fd = dmabuf->attributes.fd[0],
>   .width  = dmabuf->attributes.width,
> @@ -1126,22 +1115,32 @@ drm_output_prepare_overlay_view(struct drm_output 
> *output,
>   #else
>   return NULL;
>   #endif
> - } else {
> + } else if (b->gbm) {
>   bo = gbm_bo_import(b->gbm, GBM_BO_IMPORT_WL_BUFFER,
>  buffer_resource, GBM_BO_USE_SCANOUT);
>   }
> - if (!bo)
> - return NULL;
>   
> - format = drm_output_check_sprite_format(s, ev, bo);
> - if (format == 0) {
> - gbm_bo_destroy(bo);
> - return NULL;
> - }
> + if (bo) {
> + format = drm_output_check_sprite_format(
> + s, ev, gbm_bo_get_format(bo));
> + if (format == 0)
Unless I missed something, you shall call gbm_bo_destroy(bo) before 
returning.

> + return NULL;
> +
> + s->next = drm_fb_get_from_bo(bo, b, format);
> + if (!s->next) {
> + gbm_bo_destroy(bo);
> + return NULL;
> + }
> + } else if (dmabuf) {
> + format = drm_output_check_sprite_format(
> + s, ev, dmabuf->attributes.format);
> + if (format == 0)
> + return NULL;
>   
> - s->next = drm_fb_get_from_bo(bo, b, format);
> - if (!s->next) {
> - gbm_bo_destroy(bo);
> + s->next = drm_fb_create_dmabuf(dmabuf, b, format);
> + if (!s->next)
> + return NULL;
> + } else {
>   return NULL;
>   }
>   
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel