Re: [PATCH v3 wayland-protocols 4/4] tablet: Add pad support to the tablet protocol

[Peter Hutterer]

On Tue, May 17, 2016 at 01:30:03AM +0200, Carlos Garnacho wrote:
> Hey :),
> 
> On Wed, May 11, 2016 at 4:51 AM, Peter Hutterer
>  wrote:
> > From: Carlos Garnacho 
> >
> > The pad's interface is similar to the tool interface, a client is notified 
> > of
> > the pad after the tablet_added event.
> >
> > The pad has three functionalities: buttons, rings and strips.
> > Buttons are fairly straightforward, rings and strips are separate interfaces
> > with a pointer-axis-like source/value/frame events.
> > The two interfaces are effectively identical but for the actual value they
> > send (degrees vs normalized position).
> >
> > Buttons are sequentially indexed starting with zero, unlike other protocols
> > where a linux/input.h-style semantic event code is used. Since we expect all
> > buttons to have client-specific functionality, an additional event tells the
> > client when a given button index is not available, usually because the
> > compositor assignes some function to it (e.g. mode switching, see below).
> >
> > Specific to the pad device is the set_feedback request which enables a 
> > client
> > to set a user-defined string to display for an OSD on the current mappings.
> > This request is available for buttons, rings and strips.
> >
> > Finally, the pad supports "modes", effectively sets of button/ring/strip
> > configurations.
> >
> > Signed-off-by: Carlos Garnacho 
> > Signed-off-by: Peter Hutterer 
> > ---
> > Changes to v2:
> > - change to v2 of the protocol
> > - various comments and suggestions for improved wording incorporated
> > - ring is in wl_fixed degrees now (was int, in 0.01 of a degree)
> > - button events changed to sequential indices
> > - new buttons_reserved event
> >
> >  unstable/tablet/tablet-unstable-v2.xml | 436 
> > +
> >  1 file changed, 436 insertions(+)
> >

..

> > +
> > +  
> > +   Sent on wp_tablet_pad initialization and/or at a later time to 
> > notify the
> > +   client of reserved buttons.
> > +
> > +   Compositors may consume some buttons for global actions, those
> > +   global actions will not trigger wl_tablet_pad.button events (e.g.
> > +   the button to switch between modes, if any). This event notifies the
> > +   client that some button indices are reserved by the compositor and
> > +   will not generate events visible to the client. Button indices start
> > +   at 0.
> > +
> > +   This event may is sent in the initial burst of events before the
> > +   wp_tablet_pad.done event and/or at any later time when the
> > +   compositor changes the list of reserved buttons. This event is only
> > +   sent when the compositor reserves at least one button.
> > +  
> > +  
> > +
> > +
> > +
> 
> After some hands-on experience, I see this API in libwacom:
> 
> int libwacom_get_ring_num_modes(const WacomDevice *device);
> int libwacom_get_ring2_num_modes(const WacomDevice *device);
> int libwacom_get_strips_num_modes(const WacomDevice *device);
> 
> This makes me think, are there devices with more than one of these
> modes? In that case I guess would be better to move .modes and .mode
> to wp_tablet_pad_ring/strip than exposing the NxM combinations here. I
> guess it also means renouncing to making these modes affect anything
> else than the ring/strip.
> 
> If we move these events there, I wonder if we better add .done events
> there too, or it suffices with wp_tablet_pad.done. I'd expect all
> device characteristics to be announced before wp_tablet_pad.done
> anyway.

the only device that had two rings and modes was the 24HD and both had 4
modes iirc. that device isn't on sale anymore and given the current gen of
wacom tablets I doubt it comes back.

however, the 22hd has two touch strips at the back and two mode switch
buttons. I don't think there's much of a use-case for having different
numbers of modes but having the modes switch independently is certainly
something we should support.

the next step is then the buttons of course: on the 22hd I'd expect only the
right set of buttons to switch mode when the center button is pressed, but
the strip could/should be paird with that right set of buttons.
so we effectively need something like button groups within the pad, and the
strip/ring (and later LEDs) could be part of that button group.

The two options we have here is to either nest them within the tablet_pad
with most of the current pad events/requests move to the button group:
wp_tablet_manager
  wp_tablet_seat
wp_tablet_pad
  wp_tablet_pad_button_group
wp_tablet_pad_ring
wp_tablet_pad_strip


An alternative would be to provide multiple tablet_pads for the same
physical device, one for each virtual button group. That just avoids one
nesting layer at the cost of less clarity of what belongs to what. OTOH,
does that matter? because the only case 

Re: [PATCH v3 wayland-protocols 4/4] tablet: Add pad support to the tablet protocol

[Carlos Garnacho]

Hey :),

On Wed, May 11, 2016 at 4:51 AM, Peter Hutterer
 wrote:
> From: Carlos Garnacho 
>
> The pad's interface is similar to the tool interface, a client is notified of
> the pad after the tablet_added event.
>
> The pad has three functionalities: buttons, rings and strips.
> Buttons are fairly straightforward, rings and strips are separate interfaces
> with a pointer-axis-like source/value/frame events.
> The two interfaces are effectively identical but for the actual value they
> send (degrees vs normalized position).
>
> Buttons are sequentially indexed starting with zero, unlike other protocols
> where a linux/input.h-style semantic event code is used. Since we expect all
> buttons to have client-specific functionality, an additional event tells the
> client when a given button index is not available, usually because the
> compositor assignes some function to it (e.g. mode switching, see below).
>
> Specific to the pad device is the set_feedback request which enables a client
> to set a user-defined string to display for an OSD on the current mappings.
> This request is available for buttons, rings and strips.
>
> Finally, the pad supports "modes", effectively sets of button/ring/strip
> configurations.
>
> Signed-off-by: Carlos Garnacho 
> Signed-off-by: Peter Hutterer 
> ---
> Changes to v2:
> - change to v2 of the protocol
> - various comments and suggestions for improved wording incorporated
> - ring is in wl_fixed degrees now (was int, in 0.01 of a degree)
> - button events changed to sequential indices
> - new buttons_reserved event
>
>  unstable/tablet/tablet-unstable-v2.xml | 436 
> +
>  1 file changed, 436 insertions(+)
>
> diff --git a/unstable/tablet/tablet-unstable-v2.xml 
> b/unstable/tablet/tablet-unstable-v2.xml
> index d3f57ff..388b4d2 100644
> --- a/unstable/tablet/tablet-unstable-v2.xml
> +++ b/unstable/tablet/tablet-unstable-v2.xml
> @@ -172,6 +172,22 @@
>
> summary="the newly added tablet tool"/>
>  
> +
> +
> +  
> +   This event is sent whenever a new pad is known to the system. 
> Typically,
> +   pads are physically attached to tablets and a pad_added event is
> +   sent immediately after the wp_tablet_seat.tablet_added.
> +   However, some standalone pad devices logically attach to tablets at
> +   runtime, and the client must wait for wp_tablet_pad.enter to know
> +   the tablet a pad is attached to.
> +
> +   This event only provides the object id of the pad; and all further
> +   features (buttons, strips, rings) are sent through the wp_tablet_pad
> +   interface.
> +  
> +   summary="the newly added pad"/>
> +
>
>
>
> @@ -636,4 +652,424 @@
>  
>
>
> +  
> +
> +  A circular interaction area.
> +
> +  Events on a ring are logically grouped by the wl_tablet_pad_ring.frame
> +  event.
> +
> +
> +
> +  
> +   Request that the compositor use the provided feedback string
> +   associated with this ring. This request should be issued immediately
> +   after a wp_tablet_pad.enter, or whenever the ring is mapped to a
> +   different action.
> +
> +   Clients are encouraged to provide context-aware descriptions for
> +   the actions associated with the ring; compositors may use this
> +   information to offer visual feedback about the button layout
> +   (eg. on-screen displays).
> +
> +   The provided string 'description' is an UTF-8 encoded string to be
> +   associated with this ring, and is considered user visible; general
> +   internationalization rules apply.
> +  
> +  
> +
> +
> +
> +  
> +   This destroys the client's resource for this ring object.
> +  
> +
> +
> +
> +  
> +   Describes the source types for ring events. This indicates to the
> +   client how a ring event was physically generated; a client may
> +   adjust the user interface accordingly. For example, events
> +   from a "finger" source may trigger kinetic scrolling.
> +  
> +  
> +
> +
> +
> +  
> +   Source information for ring events.
> +
> +   This event does not occur on its own. It is sent before a
> +   wp_tablet_pad_ring.frame event and carries the source information
> +   for all events within that frame.
> +
> +   The source specifies how this event was generated. If the source is
> +   wp_tablet_pad_ring.source.finger, a wp_tablet_pad_ring.stop event
> +   will be sent when the user lifts the finger off the device.
> +
> +   This event is optional. If the source is unknown for an interaction,
> +   no event is sent.
> +  
> +  
> +
> +
> +
> +  
> +   Sent whenever the angle on a ring changes.
> +
> +   The angle is provided in degrees clockwise from the logical
> +   

[PATCH] weston: Don't exit just because tty is in gfx mode already

[Florian Hänel]

From: =?UTF-8?q?Florian=20H=C3=A4nel?= 

If weston crashed (or during development) it can leave the tty
in graphics mode, which would prevent it from ever coming up again.

Another compositor running should be handled by upstart/systemd
and the tty in graphics mode does not prevent us from using it.

A informational log message for debugging purposes should be enough.
---
 src/launcher-util.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/launcher-util.c b/src/launcher-util.c
index e89710b..a98ff74 100644
--- a/src/launcher-util.c
+++ b/src/launcher-util.c
@@ -319,7 +319,6 @@ setup_tty(struct weston_launcher *launcher, int tty)
 if (kd_mode != KD_TEXT) {
 weston_log("%s is already in graphics mode, "
"is another display server running?\n", tty_device);
-goto err_close;
 }

 ioctl(launcher->tty, VT_ACTIVATE, minor(buf.st_rdev));
--
2.7.4

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH] weston: Don't exit just because tty is in gfx mode already

[Florian Hänel]

From: =?UTF-8?q?Florian=20H=C3=A4nel?= 

If weston crashed (or during development) it can leave the tty
in graphics mode, which would prevent it from ever coming up again.

Another compositor running should be handled by upstart/systemd
and the tty in graphics mode does not prevent us from using it.

A informational log message for debugging purposes should be enough.
---
 src/launcher-util.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/launcher-util.c b/src/launcher-util.c
index e89710b..a98ff74 100644
--- a/src/launcher-util.c
+++ b/src/launcher-util.c
@@ -319,7 +319,6 @@ setup_tty(struct weston_launcher *launcher, int tty)
 if (kd_mode != KD_TEXT) {
 weston_log("%s is already in graphics mode, "
"is another display server running?\n", tty_device);
-goto err_close;
 }

 ioctl(launcher->tty, VT_ACTIVATE, minor(buf.st_rdev));
--
2.7.4

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: Introduction and updates from NVIDIA

[Daniel Vetter]

On Mon, May 16, 2016 at 11:12:35AM -0700, James Jones wrote:
> On 05/16/2016 02:36 AM, Daniel Vetter wrote:
> >On Sat, May 14, 2016 at 05:46:51PM +0100, Daniel Stone wrote:
> >>On 12 May 2016 at 00:08, James Jones  wrote:
> >>>The EGLStream encapsulation takes into consideration the new use cases
> >>>EGLImage, GBM, etc. were intended to address, and restores what I believe 
> >>>to
> >>>be the minimal amount of the traditional GL+GLX/EGL/etc. model, while still
> >>>allowing as much of the flexibility of the "a bunch of buffers" mental 
> >>>model
> >>>as possible.  We can re-invent that with GBM API adjustments, a set of
> >>>restrictions on how the buffers it allocates can be used, and another layer
> >>>of metadata being pumped into drivers on top of that, but I suspect we'd
> >>>wind up with something that looks very similar to streams.
> >>
> >>The only allocation GBM does is for buffers produced by the compositor
> >>and used for scanout, so in this regard it's quite straightforward.
> >>Client buffers are a separate topic, and I don't buy that the
> >>non-Streams model precludes things like render compression. In fact,
> >>Ben Widawsky, Dan Vetter, and some others are as we speak working on
> >>support for render compression within both Wayland EGL and GBM itself
> >>(for direct scanout from compressed buffers with an auxiliary plane).
> >>So far, the only external impact has been a very small extension to
> >>the GBM API to allow use of multiple planes and FB modifiers: a far
> >>smaller change than implementing the whole of Streams and all its
> >>future extensions (Switch et al).
> >
> >Just a quick correction: For render compression we also do need some
> >allocation hinting interface, since on intel gpus you can't always scan
> >out render compressed buffers. So exactly what EGLstreams tries to also
> >solve (at least if my understanding is correct). So we need a bit more in
> >gbm than just be able to pass fb modifiers around.
> 
> Yes, this, and it goes beyond just hinting at allocation time for us if you
> intend to reconfigure the output without reallocating the surface (E.g.,
> switch to a different plane, start rotating the output, etc.).
> 
> >I still think it's the better approach though since it's still fairly
> >incremental. And exposing the allocation hints and making them explicitly
> >will avoid the need to teach everything in the world about EGLstreams (vk,
> >v4l, drm, ...). Which as Daniel Stone pointed out, doesn't really work
> >well if you have IP blocks from multiple vendors on your SoC.
> >-Daniel
> 
> Yeah, IP blocks from multiple vendors are hard.  I don't see how they're any
> harder with streams though Vs. the alternate GBM-based proposals that have
> been suggested thus far.  We're not entirely immune to this at NVIDIA.
> Sometimes we want to present to an Intel display engine, for example.  An
> EGL-based solution doesn't necessarily mean a single vendor's EGL driver
> (GLVND is coming, slowly), and even if it does, it only requires explicit
> cooperation if both vendors share some more optimal layout than basic
> pitch-linear with minimal alignment requirements and whatnot, no
> compression, either fully-coherent caches or no caching.
> 
> However, there are two ways to solve this:
> 
> -Always resort to the lowest common denominator when the producer/consumer
> aren't from the same vendor, as mentioned above.
> 
> -Have some sort of coordination, either handled by the application and a
> bunch of capability bits, or handled by a driver<->driver API below the
> level of the application API.
> 
> Neither of these seem specific to either a streams-based or EGL-based
> solution to me.  The important part is to standardize the interfaces exposed
> to applications or drivers to coordinate the right formats.
> 
> As to needing to teach everything about EGLStreams, I think there's a
> misconception that this means every component vendor needs to get on the EGL
> bandwagon and start writing a bunch of no-op eglGetConfig() entry points and
> whatnot.  Even with all our in-house IP, that's not the case at NVIDIA.  Our
> media codecs aren't baked into the same driver module as our OpenGL drivers
> for example, and the drivers and engineers maintaining them know very little
> about eachother.  Our EGL driver allows stream producers/consumers to plug
> into it using some internal-standard interfaces and a relatively minimal
> amount of code, and without even including any Khronos EGL headers.

I think this is the crux here - we need to standardize those hints/minimal
set of shared metadata, in a public, cross-vendor interface. Since
obviously without that it doesn't even work for you in your one-vendor
case.

And as soon as we have that standard, I don't really see the benefit of
EGLstreams any more. At least my understanding is that the entire point of
EGLstreams is to hide these hints and metadata and allow them to be vendor
specific.

The other argument 

Re: [PATCH] weston-launch: Handle invalid command line options

[Yong Bakos]

On May 7, 2016, at 5:57 AM, Otavio Salvador  wrote:
> 
> From: Tom Hochstein 
> 
> Exit the program if an unrecognized command line option is found.
> 
> Signed-off-by; Tom Hochstein 
> Signed-off-by: Otavio Salvador 

Simple enough of a review, and I did test this. But the question is
whether we want weston-launch to ignore invalid options or to quit in the
event of their presence. I'm not experienced enough to judge, so others
will have to chime in. So fwiw,

Reviewed-by: Yong Bakos 
Tested-by: Yong Bakos 

yong


> ---
> 
> src/weston-launch.c | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/src/weston-launch.c b/src/weston-launch.c
> index b8dfb17..9987d8e 100644
> --- a/src/weston-launch.c
> +++ b/src/weston-launch.c
> @@ -703,6 +703,8 @@ main(int argc, char *argv[])
>   case 'h':
>   help("weston-launch");
>   exit(EXIT_FAILURE);
> + default:
> + exit(EXIT_FAILURE);
>   }
>   }
> 
> -- 
> 2.8.2
> 
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: Introduction and updates from NVIDIA

[James Jones]

On 05/16/2016 02:36 AM, Daniel Vetter wrote:

On Sat, May 14, 2016 at 05:46:51PM +0100, Daniel Stone wrote:

On 12 May 2016 at 00:08, James Jones  wrote:

The EGLStream encapsulation takes into consideration the new use cases
EGLImage, GBM, etc. were intended to address, and restores what I believe to
be the minimal amount of the traditional GL+GLX/EGL/etc. model, while still
allowing as much of the flexibility of the "a bunch of buffers" mental model
as possible.  We can re-invent that with GBM API adjustments, a set of
restrictions on how the buffers it allocates can be used, and another layer
of metadata being pumped into drivers on top of that, but I suspect we'd
wind up with something that looks very similar to streams.


The only allocation GBM does is for buffers produced by the compositor
and used for scanout, so in this regard it's quite straightforward.
Client buffers are a separate topic, and I don't buy that the
non-Streams model precludes things like render compression. In fact,
Ben Widawsky, Dan Vetter, and some others are as we speak working on
support for render compression within both Wayland EGL and GBM itself
(for direct scanout from compressed buffers with an auxiliary plane).
So far, the only external impact has been a very small extension to
the GBM API to allow use of multiple planes and FB modifiers: a far
smaller change than implementing the whole of Streams and all its
future extensions (Switch et al).


Just a quick correction: For render compression we also do need some
allocation hinting interface, since on intel gpus you can't always scan
out render compressed buffers. So exactly what EGLstreams tries to also
solve (at least if my understanding is correct). So we need a bit more in
gbm than just be able to pass fb modifiers around.


Yes, this, and it goes beyond just hinting at allocation time for us if 
you intend to reconfigure the output without reallocating the surface 
(E.g., switch to a different plane, start rotating the output, etc.).



I still think it's the better approach though since it's still fairly
incremental. And exposing the allocation hints and making them explicitly
will avoid the need to teach everything in the world about EGLstreams (vk,
v4l, drm, ...). Which as Daniel Stone pointed out, doesn't really work
well if you have IP blocks from multiple vendors on your SoC.
-Daniel


Yeah, IP blocks from multiple vendors are hard.  I don't see how they're 
any harder with streams though Vs. the alternate GBM-based proposals 
that have been suggested thus far.  We're not entirely immune to this at 
NVIDIA.  Sometimes we want to present to an Intel display engine, for 
example.  An EGL-based solution doesn't necessarily mean a single 
vendor's EGL driver (GLVND is coming, slowly), and even if it does, it 
only requires explicit cooperation if both vendors share some more 
optimal layout than basic pitch-linear with minimal alignment 
requirements and whatnot, no compression, either fully-coherent caches 
or no caching.


However, there are two ways to solve this:

-Always resort to the lowest common denominator when the 
producer/consumer aren't from the same vendor, as mentioned above.


-Have some sort of coordination, either handled by the application and a 
bunch of capability bits, or handled by a driver<->driver API below the 
level of the application API.


Neither of these seem specific to either a streams-based or EGL-based 
solution to me.  The important part is to standardize the interfaces 
exposed to applications or drivers to coordinate the right formats.


As to needing to teach everything about EGLStreams, I think there's a 
misconception that this means every component vendor needs to get on the 
EGL bandwagon and start writing a bunch of no-op eglGetConfig() entry 
points and whatnot.  Even with all our in-house IP, that's not the case 
at NVIDIA.  Our media codecs aren't baked into the same driver module as 
our OpenGL drivers for example, and the drivers and engineers 
maintaining them know very little about eachother.  Our EGL driver 
allows stream producers/consumers to plug into it using some 
internal-standard interfaces and a relatively minimal amount of code, 
and without even including any Khronos EGL headers.


The current Khronos EGL API doesn't need to be the only interface 
through which drivers plug in to a libEGL or vendor EGL implementation. 
 The proposal to expose a vendor-agnostic set of hooks to allow writing 
EGL platform implementations without EGL vendor involvement is one 
example of a non-application facing EGL API.  EGLStream producer and 
consumer hooks could be handled with another non-application facing API.


> As Kristian says, I really don't see where the existing non-Streams
> solutions, being GBM on the compositor side and private frame-based
> protocols between compositor and client, leave you unable to reach
> full performance potential. Do you have any concrete usecases that you
> can 

[PATCH wayland] event-loop: Make transitive include explicit

[Yong Bakos]

From: Yong Bakos 

The explicit inclusion of wayland-server.h hides the real dependency, which
is wayland-server-core.h.

Signed-off-by: Yong Bakos 
---
 src/event-loop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/event-loop.c b/src/event-loop.c
index ea27b69..11a9bf2 100644
--- a/src/event-loop.c
+++ b/src/event-loop.c
@@ -37,7 +37,7 @@
 #include 
 #include 
 #include "wayland-private.h"
-#include "wayland-server.h"
+#include "wayland-server-core.h"
 #include "wayland-os.h"
 
 struct wl_event_loop {
-- 
2.7.2

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH v2 libinput] Add configure.ac check for static_assert

[Baruch Siach]

Hi Peter,

On Mon, May 16, 2016 at 01:32:07PM +1000, Peter Hutterer wrote:
> Part of C11, defined via assert.h.
> 
> Signed-off-by: Peter Hutterer 

This fixes the build using a uClibc-ng toolchain that does not define the 
static_assert macro.

Tested-by: Baruch Siach 

Thanks,
baruch

> ---
> Changes to v1: 
> - leave static_assert in place and just check for that in configure
> 
>  configure.ac | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/configure.ac b/configure.ac
> index 8ddc3b6..8c14efe 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -59,6 +59,10 @@ AC_CHECK_DECL(TFD_CLOEXEC,[],
>  AC_CHECK_DECL(CLOCK_MONOTONIC,[],
> [AC_MSG_ERROR("CLOCK_MONOTONIC is needed to compile libinput")],
> [[#include ]])
> +AC_CHECK_DECL(static_assert, [],
> +   [AC_DEFINE(static_assert(...), [/* */], [noop static_assert() 
> replacement]),
> +  AC_MSG_RESULT([no])],
> +   [[#include ]])
>  
>  PKG_PROG_PKG_CONFIG()
>  PKG_CHECK_MODULES(MTDEV, [mtdev >= 1.1.0])
> -- 
> 2.7.4
> 

-- 
 http://baruch.siach.name/blog/  ~. .~   Tk Open Systems
=}ooO--U--Ooo{=
   - bar...@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 2/2] compositor: surface and view output comment fixes

[Armin Krezović]

On 16.05.2016 17:02, Jonas Ådahl wrote:
> On Mon, May 16, 2016 at 04:45:05PM +0200, Armin Krezović wrote:
>> On 10.05.2016 16:10, Pekka Paalanen wrote:
>>> From: Pekka Paalanen 
>>>
>>> weston_surface::output and weston_view::output as used for different
>>> purposes. Only the surface output is used for frame callbacks.
>>>
>>> The uses of the view output are much more vague and hard to describe.
>>>
>>> Also fix a comment mistake in weston_surface_assign_output().
>>>
>>
>> All the changes look fine to me. I have one comment below, as I'm not sure
>> if it's a mistake or not.
>>
>> In case it isn't a mistake (or with the mistake fixed, in case it is one),
>> this patch is also:
>>
>> Reviewed-by: Armin Krezović 
>>
>>> Signed-off-by: Pekka Paalanen 
>>> ---
>>>  src/compositor.c | 8 +++-
>>>  src/compositor.h | 5 +++--
>>>  2 files changed, 6 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/compositor.c b/src/compositor.c
>>> index ee47a82..40d8baf 100644
>>> --- a/src/compositor.c
>>> +++ b/src/compositor.c
>>> @@ -1082,16 +1082,15 @@ weston_surface_update_output_mask(struct 
>>> weston_surface *es, uint32_t mask)
>>> }
>>>  }
>>>  
>>> -
>>>  /** Recalculate which output(s) the surface has views displayed on
>>>   *
>>>   * \param es  The surface to remap to outputs
>>>   *
>>>   * Finds the output that is showing the largest amount of one
>>>   * of the surface's various views.  This output becomes the
>>> - * surface's primary output for vsync and frame event purposes.
>>> + * surface's primary output for vsync and frame callback purposes.
>>>   *
>>> - * Also notes the primary outputs of all of the surface's views
>>> + * Also notes all outputs of all of the surface's views
>>>   * in the output_mask for the surface.
>>>   */
>>>  static void
>>> @@ -1136,8 +1135,7 @@ weston_surface_assign_output(struct weston_surface 
>>> *es)
>>>   *
>>>   * Identifies the set of outputs that the view is visible on,
>>>   * noting them into the output_mask.  The output that the view
>>> - * is most visible on is set as the view's primary output for
>>> - * vsync and frame event purposes.
>>> + * is most visible on is set as the view's primary output.
>>>   *
>>>   * Also does the same for the view's surface.  See
>>>   * weston_surface_assign_output().
>>> diff --git a/src/compositor.h b/src/compositor.h
>>> index 7851000..0801f20 100644
>>> --- a/src/compositor.h
>>> +++ b/src/compositor.h
>>> @@ -949,8 +949,9 @@ struct weston_view {
>>> } transform;
>>>  
>>> /*
>>> -* Which output to vsync this surface to.
>>> -* Used to determine, whether to send or queue frame events.
>>> +* The primary output for this view.
>>> +* Used for picking the output for driving view animations, inheriting
>>
>> Is the correct term "driving animations" or "drawing animations" ?
> 
> It is actually "driving" here. One surface may be drawn on multiple
> outputs, but only one output will "drive" the animation.
> 
> Driving the animation means to respond to frame callbacks, and since
> outputs may be rendered independently, we tie the animation of a surface
> to only one output.
> 
> For example: a client rendering an animation will attach and commit a
> new buffer of a frame of its animation, and ask for a frame callback.
> Then it will wait for the frame callback until it draws, attaches and
> commits the buffer of the next frame of its animation.
> 
> In order to get as good results as possible, meaning content drawn by
> the client should end up on the output as quickly as possible, we
> calculate the output it has the largest overlapping region on, and
> whenever that output was painted, we reply to the frame callback.
> 
> If we for example would wait until all outputs the surface is one is
> drawn on until replying, we would potentially delay the frame callback,
> making the latency between the client drawing and the content being
> displayed on the output, unnecessarily long, since we would effectively
> let the output drawn last, no matter how large portion of the surface is
> drawn on it, always "drive" the animation.
> 
> 
> Jonas
> 

Thanks for the brief explanation!

>>
>>> +* the primary output for related views in shells, etc.
>>>  * Must be NULL, if 'link' is not in weston_compositor::view_list.
>>>  */
>>> struct weston_output *output;
>>>
>>




signature.asc
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/2] compositor: surface and view output comment fixes

[Jonas Ådahl]

On Mon, May 16, 2016 at 04:45:05PM +0200, Armin Krezović wrote:
> On 10.05.2016 16:10, Pekka Paalanen wrote:
> > From: Pekka Paalanen 
> > 
> > weston_surface::output and weston_view::output as used for different
> > purposes. Only the surface output is used for frame callbacks.
> > 
> > The uses of the view output are much more vague and hard to describe.
> > 
> > Also fix a comment mistake in weston_surface_assign_output().
> > 
> 
> All the changes look fine to me. I have one comment below, as I'm not sure
> if it's a mistake or not.
> 
> In case it isn't a mistake (or with the mistake fixed, in case it is one),
> this patch is also:
> 
> Reviewed-by: Armin Krezović 
> 
> > Signed-off-by: Pekka Paalanen 
> > ---
> >  src/compositor.c | 8 +++-
> >  src/compositor.h | 5 +++--
> >  2 files changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/compositor.c b/src/compositor.c
> > index ee47a82..40d8baf 100644
> > --- a/src/compositor.c
> > +++ b/src/compositor.c
> > @@ -1082,16 +1082,15 @@ weston_surface_update_output_mask(struct 
> > weston_surface *es, uint32_t mask)
> > }
> >  }
> >  
> > -
> >  /** Recalculate which output(s) the surface has views displayed on
> >   *
> >   * \param es  The surface to remap to outputs
> >   *
> >   * Finds the output that is showing the largest amount of one
> >   * of the surface's various views.  This output becomes the
> > - * surface's primary output for vsync and frame event purposes.
> > + * surface's primary output for vsync and frame callback purposes.
> >   *
> > - * Also notes the primary outputs of all of the surface's views
> > + * Also notes all outputs of all of the surface's views
> >   * in the output_mask for the surface.
> >   */
> >  static void
> > @@ -1136,8 +1135,7 @@ weston_surface_assign_output(struct weston_surface 
> > *es)
> >   *
> >   * Identifies the set of outputs that the view is visible on,
> >   * noting them into the output_mask.  The output that the view
> > - * is most visible on is set as the view's primary output for
> > - * vsync and frame event purposes.
> > + * is most visible on is set as the view's primary output.
> >   *
> >   * Also does the same for the view's surface.  See
> >   * weston_surface_assign_output().
> > diff --git a/src/compositor.h b/src/compositor.h
> > index 7851000..0801f20 100644
> > --- a/src/compositor.h
> > +++ b/src/compositor.h
> > @@ -949,8 +949,9 @@ struct weston_view {
> > } transform;
> >  
> > /*
> > -* Which output to vsync this surface to.
> > -* Used to determine, whether to send or queue frame events.
> > +* The primary output for this view.
> > +* Used for picking the output for driving view animations, inheriting
> 
> Is the correct term "driving animations" or "drawing animations" ?

It is actually "driving" here. One surface may be drawn on multiple
outputs, but only one output will "drive" the animation.

Driving the animation means to respond to frame callbacks, and since
outputs may be rendered independently, we tie the animation of a surface
to only one output.

For example: a client rendering an animation will attach and commit a
new buffer of a frame of its animation, and ask for a frame callback.
Then it will wait for the frame callback until it draws, attaches and
commits the buffer of the next frame of its animation.

In order to get as good results as possible, meaning content drawn by
the client should end up on the output as quickly as possible, we
calculate the output it has the largest overlapping region on, and
whenever that output was painted, we reply to the frame callback.

If we for example would wait until all outputs the surface is one is
drawn on until replying, we would potentially delay the frame callback,
making the latency between the client drawing and the content being
displayed on the output, unnecessarily long, since we would effectively
let the output drawn last, no matter how large portion of the surface is
drawn on it, always "drive" the animation.


Jonas

> 
> > +* the primary output for related views in shells, etc.
> >  * Must be NULL, if 'link' is not in weston_compositor::view_list.
> >  */
> > struct weston_output *output;
> > 
> 




> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 2/2] compositor: surface and view output comment fixes

[Armin Krezović]

On 10.05.2016 16:10, Pekka Paalanen wrote:
> From: Pekka Paalanen 
> 
> weston_surface::output and weston_view::output as used for different
> purposes. Only the surface output is used for frame callbacks.
> 
> The uses of the view output are much more vague and hard to describe.
> 
> Also fix a comment mistake in weston_surface_assign_output().
> 

All the changes look fine to me. I have one comment below, as I'm not sure
if it's a mistake or not.

In case it isn't a mistake (or with the mistake fixed, in case it is one),
this patch is also:

Reviewed-by: Armin Krezović 

> Signed-off-by: Pekka Paalanen 
> ---
>  src/compositor.c | 8 +++-
>  src/compositor.h | 5 +++--
>  2 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/src/compositor.c b/src/compositor.c
> index ee47a82..40d8baf 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
> @@ -1082,16 +1082,15 @@ weston_surface_update_output_mask(struct 
> weston_surface *es, uint32_t mask)
>   }
>  }
>  
> -
>  /** Recalculate which output(s) the surface has views displayed on
>   *
>   * \param es  The surface to remap to outputs
>   *
>   * Finds the output that is showing the largest amount of one
>   * of the surface's various views.  This output becomes the
> - * surface's primary output for vsync and frame event purposes.
> + * surface's primary output for vsync and frame callback purposes.
>   *
> - * Also notes the primary outputs of all of the surface's views
> + * Also notes all outputs of all of the surface's views
>   * in the output_mask for the surface.
>   */
>  static void
> @@ -1136,8 +1135,7 @@ weston_surface_assign_output(struct weston_surface *es)
>   *
>   * Identifies the set of outputs that the view is visible on,
>   * noting them into the output_mask.  The output that the view
> - * is most visible on is set as the view's primary output for
> - * vsync and frame event purposes.
> + * is most visible on is set as the view's primary output.
>   *
>   * Also does the same for the view's surface.  See
>   * weston_surface_assign_output().
> diff --git a/src/compositor.h b/src/compositor.h
> index 7851000..0801f20 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -949,8 +949,9 @@ struct weston_view {
>   } transform;
>  
>   /*
> -  * Which output to vsync this surface to.
> -  * Used to determine, whether to send or queue frame events.
> +  * The primary output for this view.
> +  * Used for picking the output for driving view animations, inheriting

Is the correct term "driving animations" or "drawing animations" ?

> +  * the primary output for related views in shells, etc.
>* Must be NULL, if 'link' is not in weston_compositor::view_list.
>*/
>   struct weston_output *output;
> 



signature.asc
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/2] compositor: fix comments about weston_compositor::surface_list

[Armin Krezović]

On 10.05.2016 16:10, Pekka Paalanen wrote:
> From: Pekka Paalanen 
> 
> a7af70436b7dccfacd736626d6719b3e751fd985 converted the surface list into
> a view list.
> 
> It looks like weston_surface::output's comment about surface list does
> not apply to view list. Still, many places assume weston_surface::output
> is not NULL when processing "visible" surfaces, e.g. those reachable via
> the view list.
> 
> The comment on weston_view::output is updated, but I could not figure
> out the actual relationship between that and being on the view list.
> 
> weston_view::link is documented to be in weston_compositor::view_list,
> and weston_compositor::view_list is documented to contain weston_views.
> 

It is always nice to see someone documenting the code. I've been banging
my head lately to understand the codebase, and the currently present
comments really help there.

With the issue noted below fixed, this patch is:

Reviewed-by: Armin Krezović 

> Signed-off-by: Pekka Paalanen 
> ---
>  src/compositor.h | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/src/compositor.h b/src/compositor.h
> index a95f05d..7851000 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -754,7 +754,7 @@ struct weston_compositor {
>   struct wl_list output_list;
>   struct wl_list seat_list;
>   struct wl_list layer_list;
> - struct wl_list view_list;
> + struct wl_list view_list;   /* struct weston_view::link */
>   struct wl_list plane_list;
>   struct wl_list key_binding_list;
>   struct wl_list modifier_binding_list;
> @@ -890,7 +890,7 @@ struct weston_view {
>   struct wl_list surface_link;
>   struct wl_signal destroy_signal;
>  
> - struct wl_list link;
> + struct wl_list link; /* weston_compositor::view_list */
>   struct weston_layer_entry layer_link; /* part of geometry */
>   struct weston_plane *plane;
>  
> @@ -951,7 +951,7 @@ struct weston_view {
>   /*
>* Which output to vsync this surface to.
>* Used to determine, whether to send or queue frame events.
> -  * Must be NULL, if 'link' is not in weston_compositor::surface_list.
> +  * Must be NULL, if 'link' is not in weston_compositor::view_list.

I couldn't find if anything checks for this, and view->output can only be null
if it got NULL from an output list. So I think the comment is redundant.

>*/
>   struct weston_output *output;
>  
> @@ -1021,7 +1021,6 @@ struct weston_surface {
>   /*
>* Which output to vsync this surface to.
>* Used to determine, whether to send or queue frame events.
> -  * Must be NULL, if 'link' is not in weston_compositor::surface_list.
>*/
>   struct weston_output *output;
>  
> 



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

Re: Feedback on xdg-shell v5

[Bill Spitzak]

On Mon, May 9, 2016 at 8:18 PM, Jonas Ådahl  wrote:

> On Fri, Apr 29, 2016 at 12:37:22PM +0300, Pekka Paalanen wrote:
> > On Fri, 29 Apr 2016 15:31:28 +0800
> > Jonas Ådahl  wrote:
> >
> > > On Thu, Apr 28, 2016 at 02:08:07PM +0300, Pekka Paalanen wrote:
> > > > On Thu, 28 Apr 2016 17:36:58 +0800
> > > > Jonas Ådahl  wrote:
> > > >
> > > > > On Thu, Apr 28, 2016 at 10:36:14AM +0200, Martin Graesslin wrote:
> > > > > > Hi Wayland and Plasma,
> > > > > >
> > > > > > I finished the implementation of xdg-shell in KWayland [1] and
> KWin [2].
> > > > > > Overall it was more straight forward than I would have expected.
> Especially
> > > > > > the changes to KWin were surprisingly minor (with one huge
> exception).
> > > > > >
> > > > > > Now some questions/remarks:
> > > >
> > > > > > * Why is the ping on the shell and not on the surface? I would
> have expected
> > > > > > to ping the surface. At least that's how I would want to do it
> from KWin.
> > > > >
> > > > > Because you don't ping a surface, you ping the client. It's the
> client
> > > > > that may be inresponsive, and if the client is in responsive, it's
> safe
> > > > > to assume all its surfaces are as well.
> > > >
> > > > Hi,
> > > >
> > > > I was going to plain agree and say, that all events to a client come
> > > > through the same connection (wl_display), and it does not make sense
> to
> > > > have a series of ping events on different objects when they could
> just
> > > > be collapsed into one equivalent event.
> > > >
> > > > But then I thought about multiple client-side event queues. If a
> client
> > > > has multiple queues, and windows on different queues, it could be
> > > > possible that only some queues get stuck while others are serviced.
> > > > With per-surface pings, the compositor should then "shade out" only
> the
> > > > windows where the queue is actually stuck.
> > > >
> > > > Would it be worth it?
> > >
> > > I doubt it. What would you do? It's not like you can disconnect half of
> > > a client.
> >
> > Wasn't it more about marking unresponsive surfaces and providing UI
> > aids for dealing with them, e.g. moving the window out of the way?
> >
> > If you hover over the "kill this app" option, the compositor could e.g.
> > color all the surfaces that would go, not just the ones unresponsive.
> >
> > > >
> > > > Or, is there an underlying assumption that a client might not send
> pong
> > > > requests as a simple reaction to ping events directly, but delay or
> > > > delegate it to some mechanism responsible for other updates?
> > > >
> > > > That is, is the ping-pong protocol a keep-alive for the Wayland
> > > > connection, for client's event queues, or something deeper in the
> > > > client?
> > >
> > > My understanding of the ping-pong protocol is to make it possible for
> > > the compositor to know when a client has frozen so that it can get rid
> > > of it (ala "Force Quit").
> >
> > Yeah, that's the final action, but there could be some less drastic
> > options too.
> >
> > > > > >
> > > > > > The biggest problem for me is the request set_window_geometry. I
> think I
> > > > > > mentioned it already before on the wayland list. Basically I
> have no idea how
> > > > > > to implement that in KWin. We have only one geometry for a
> window and that's
> > > > > > mapped to the size of the surface/x11 pixmap. This geometry is
> used all over
> > > > > > the place, for positioning, for resizing and for rendering. I
> cannot add
> > > > > > support for this without either breaking code or having to
> introduce a new
> > > > > > internal API. That's lots of work with high potential for
> breakage :-(
> > > >
> > > > Have you looked at what you need to do to support windows that are
> > > > built from non-overlapping sub-surfaces, like what Jonas describes
> below?
> > > >
> > > > I suspect you might end up having to do that major internal API work
> > > > anyway by the sounds of it. A window may be a collection of surfaces,
> > > > not just one.
> > > >
> > > > How do you do the window geometry with server-side decorations? Why
> is
> > > > using only a part of client surface so different from using a
> > > > combination of client and server-only surfaces?
> > > >
> > > > > >
> > > > > > From the description it seems to be only relevant for shadows.
> Could we make
> > > > > > shadows an optional part in xdg-shell? That the server can
> announce that it
> > > > > > supports shadows in the surface?
> > > > >
> > > > > It's not only about shadow. Let me explain a scenario where a
> window
> > > > > geometry is needed, even when there is a mode where no shadow is
> drawn
> > > > > by the client.
> > > > >
> > > > > Consider we have the following window:
> > > > >
> > > > >
> > > > > +---+
> > > > > |   A window  X |
> > > > > +---+
> > > > > |  

[PATCH] weston-launch: Handle invalid command line options

[Otavio Salvador]

From: Tom Hochstein 

Exit the program if an unrecognized command line option is found.

Signed-off-by; Tom Hochstein 
Signed-off-by: Otavio Salvador 

---

 src/weston-launch.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/weston-launch.c b/src/weston-launch.c
index b8dfb17..9987d8e 100644
--- a/src/weston-launch.c
+++ b/src/weston-launch.c
@@ -703,6 +703,8 @@ main(int argc, char *argv[])
case 'h':
help("weston-launch");
exit(EXIT_FAILURE);
+   default:
+   exit(EXIT_FAILURE);
}
}
 
-- 
2.8.2

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v1 3/3] compositor-x11: remove manual mouse button grab/ungrab

[Bill Spitzak]

Does the x11 compositor have to do this at all? It owns that window so it
gets automatic grabs. This is for one client to eat the events being passed
to another client.


On Fri, May 13, 2016 at 4:34 AM, Benoit Gschwind 
wrote:

> In current compositor-x11, the mouse buttons are grabbed and ungrabbed
> manually that may produce weird cases like starting a grab while the
> buttons are already released, due to asynchronous X11 events dispatching.
>
> The patch avoid the issue by using the better passive button grab, that
> automatically grab buttons as soon as they occur. The passive grab
> include an automatic ungrab when all mouse button are released.
>
> This probably fix some mysterious bugs.
>
> Signed-off-by: Benoit Gschwind 
> ---
>  src/compositor-x11.c | 27 +--
>  1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/src/compositor-x11.c b/src/compositor-x11.c
> index 5b76dba..ae155d1 100644
> --- a/src/compositor-x11.c
> +++ b/src/compositor-x11.c
> @@ -882,6 +882,19 @@ x11_backend_create_output(struct x11_backend *b, int
> x, int y,
>
> x11_output_set_wm_protocols(b, output);
>
> +   /* Automatically grab and ungrab mouse buttons */
> +   xcb_grab_button(b->conn, 0, output->window,
> +   XCB_EVENT_MASK_BUTTON_PRESS |
> +   XCB_EVENT_MASK_BUTTON_RELEASE |
> +   XCB_EVENT_MASK_POINTER_MOTION |
> +   XCB_EVENT_MASK_ENTER_WINDOW |
> +   XCB_EVENT_MASK_LEAVE_WINDOW,
> +   XCB_GRAB_MODE_ASYNC,
> +   XCB_GRAB_MODE_ASYNC,
> +   output->window, XCB_CURSOR_NONE,
> +   XCB_BUTTON_INDEX_ANY,
> +   XCB_MOD_MASK_ANY);
> +
> xcb_map_window(b->conn, output->window);
>
> if (fullscreen)
> @@ -1052,20 +1065,6 @@ x11_backend_deliver_button_event(struct x11_backend
> *b,
> if (!output)
> return;
>
> -   if (is_button_pressed)
> -   xcb_grab_pointer(b->conn, 0, output->window,
> -XCB_EVENT_MASK_BUTTON_PRESS |
> -XCB_EVENT_MASK_BUTTON_RELEASE |
> -XCB_EVENT_MASK_POINTER_MOTION |
> -XCB_EVENT_MASK_ENTER_WINDOW |
> -XCB_EVENT_MASK_LEAVE_WINDOW,
> -XCB_GRAB_MODE_ASYNC,
> -XCB_GRAB_MODE_ASYNC,
> -output->window, XCB_CURSOR_NONE,
> -button_event->time);
> -   else
> -   xcb_ungrab_pointer(b->conn, button_event->time);
> -
> if (!b->has_xkb)
> update_xkb_state_from_core(b, button_event->state);
>
> --
> 2.7.3
>
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland-protocols v2 6/7] xdg-shell: Make xdg_popup non-grabbing by default

[Bill Spitzak]

On Tue, May 10, 2016 at 10:50 PM, Jonas Ådahl  wrote:

> Turn xdg_popup into plain temporary child surfaces without any grabbing
> or mapping order requirements by default.
>
> In order to create grabbing popup chains, a new request 'grab' is
> introduced which enables more or less the same semantics and
> requirements as xdg_popup previously had related to grabbing, stacking
> and mapping order.
>
> This enables using xdg_popup for creating tooltips and other user
> interface elements that does not want to take an explicit grab.
>

YAY

However clients need the ability to "grab" from any surface. Otherwise they
cannot use their own graphics to create popup menus (they can make a dummy
invisible popup window, but that seems like a kludge), or make an interface
that uses multiple popup windows but can be in a state where there are zero
of them displayed. Also necessary if you want to emulate X grabs since they
can be done from any surface. Basically the grab request should be moved to
xdg_surface.

Dismissing a grab should not unmap the popup surfaces (except perhaps if
the client is failing to respond to pings, but in that case I would have a
click anywhere remove the popups whether or not a grab was done). This is a
requirement so the client can synchronize other graphics with the removal
of the popups. It sounds like this is true right now, but the description
of grab contains a lot of text that implies some relationship that should
maybe be deleted.

The idea of sending a copy of every event to every client surface is pretty
ugly. You need at least a 'frame' event so the client can tell which blocks
belong together. Instead the events can go to the surface that did the
grab. The compositor should send events to the client telling the actual
position of any child surface relative to it's parent, this allows a client
to translate an event in any child surface to any other one.
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] eventdemo: use %u for uint32_t printing

[Pekka Paalanen]

On Fri, 13 May 2016 13:50:17 +0200
Benoit Gschwind  wrote:

> Hello Pekka,
> 
> Look sane.
> 
> Reviewed-by: Benoit Gschwind 

And pushed:
   0baffb0..901ac32  master -> master


Thanks,
pq

> On 13/05/2016 13:38, Pekka Paalanen wrote:
> > From: Pekka Paalanen 
> > 
> > I was confused why timestamp was printed negative. This fixes it, and
> > others while at it.
> > 
> > Signed-off-by: Pekka Paalanen 
> > ---
> >  clients/eventdemo.c | 12 ++--
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/clients/eventdemo.c b/clients/eventdemo.c
> > index 64b3d01..f04e39b 100644
> > --- a/clients/eventdemo.c
> > +++ b/clients/eventdemo.c
> > @@ -206,7 +206,7 @@ key_handler(struct window *window, struct input *input, 
> > uint32_t time,
> > if (!log_key)
> > return;
> >  
> > -   printf("key key: %d, unicode: %d, state: %s, modifiers: 0x%x\n",
> > +   printf("key key: %u, unicode: %u, state: %s, modifiers: 0x%x\n",
> >key, unicode,
> >(state == WL_KEYBOARD_KEY_STATE_PRESSED) ? "pressed" :
> >   "released",
> > @@ -235,7 +235,7 @@ button_handler(struct widget *widget, struct input 
> > *input, uint32_t time,
> > e->print_pointer_frame = true;
> >  
> > input_get_position(input, , );
> > -   printf("button time: %d, button: %d, state: %s, x: %d, y: %d\n",
> > +   printf("button time: %u, button: %u, state: %s, x: %d, y: %d\n",
> >time, button,
> >(state == WL_POINTER_BUTTON_STATE_PRESSED) ? "pressed" :
> > "released",
> > @@ -262,7 +262,7 @@ axis_handler(struct widget *widget, struct input 
> > *input, uint32_t time,
> >  
> > e->print_pointer_frame = true;
> >  
> > -   printf("axis time: %d, axis: %s, value: %f\n",
> > +   printf("axis time: %u, axis: %s, value: %f\n",
> >time,
> >axis == WL_POINTER_AXIS_VERTICAL_SCROLL ? "vertical" :
> >  "horizontal",
> > @@ -322,7 +322,7 @@ axis_stop_handler(struct widget *widget, struct input 
> > *input,
> > return;
> >  
> > e->print_pointer_frame = true;
> > -   printf("axis stop time: %d, axis: %s\n",
> > +   printf("axis stop time: %u, axis: %s\n",
> >time,
> >axis == WL_POINTER_AXIS_VERTICAL_SCROLL ? "vertical" :
> >  "horizontal");
> > @@ -338,7 +338,7 @@ axis_discrete_handler(struct widget *widget, struct 
> > input *input,
> > return;
> >  
> > e->print_pointer_frame = true;
> > -   printf("axis discrete axis: %d value: %d\n", axis, discrete);
> > +   printf("axis discrete axis: %u value: %d\n", axis, discrete);
> >  }
> >  
> >  /**
> > @@ -361,7 +361,7 @@ motion_handler(struct widget *widget, struct input 
> > *input, uint32_t time,
> > struct eventdemo *e = data;
> >  
> > if (log_motion) {
> > -   printf("motion time: %d, x: %f, y: %f\n", time, x, y);
> > +   printf("motion time: %u, x: %f, y: %f\n", time, x, y);
> > e->print_pointer_frame = true;
> > }
> >  
> >   



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

Re: [PATCH wayland 5/5] tests: test whether the destroyed object is set to NULL on client destruction

[Pekka Paalanen]

On Fri, 13 May 2016 15:01:22 +0200
Marek Chalupa  wrote:

> when the client is being destroyed, we can use wl_client_get_object()
> to get an object. Test whether it returns NULL when the object
> has been destroyed
> 
> Signed-off-by: Marek Chalupa 
> ---
>  tests/client-test.c | 63 
> +
>  1 file changed, 63 insertions(+)
> 
> diff --git a/tests/client-test.c b/tests/client-test.c
> index cb08e39..f171f19 100644
> --- a/tests/client-test.c
> +++ b/tests/client-test.c
> @@ -126,3 +126,66 @@ TEST(client_post_nomem_on_destruction)
>   wl_display_destroy(display);
>  }
>  
> +static void
> +seat_destroy_get_object(struct wl_listener *l, void *data)
> +{
> + struct wl_resource *resource = data;
> + struct wl_client *client = wl_resource_get_client(resource);
> + /* This seat was created last,
> +  * therefore the seat's id is the upper bound for all ids */
> + uint32_t max_id = wl_resource_get_id(resource);
> + uint32_t i;
> +
> + for (i = WL_SERVER_ID_START; i < max_id; ++i) {
> + /* all these objects are destroyed now, since
> +  * we destroy the objects in id order */
> + assert(wl_client_get_object(client, i) == NULL);

Hi,

because the objects are destroyed, calling wl_client_get_object() on
them is illegal. The ids you pass in are not what you expect them to
be. See my comments to patch 2.

Also, I don't think you can rely on the destruction order, so you don't
actually know *if* they have been destroyed already. Unless, you are
specifically also testing for the destruction order in quite a weak
manner.

The legal way to do this would be to record the object ids in a list,
and install a destroy listener for each wl_resource, because that is
the only way to know if an id is still valid for the thing you think it
is.

Making invalid ids guaranteed to return NULL does not get you very far
without explicitly tracking the wl_resource lifetime anyway. If the id
gets re-used, you will get a valid pointer but to a different object.
Checking the object interface and implementation is not enough either,
because it might be another object of the same type, but still not the
object you were looking for.

Therefore NAK from me for this patch.

Makes me wonder if we should actually deprecate
wl_client_get_object(), and maybe offer a new
wl_client_get_display_resource() for the few cases where one just needs
to send an error without having an appropriate wl_resource and/or error
code. Or even a wl_client_send_display_error().


Thanks,
pq

> + }
> +
> + /* only our object is not still destroyed */
> + assert(wl_client_get_object(client, max_id) != NULL);
> + assert(wl_client_get_object(client, max_id + 1) == NULL);
> +}
> +
> +TEST(client_get_object_on_destruction)
> +{
> + struct wl_display *display;
> + struct wl_client *client;
> + struct wl_resource *resource;
> + int s[2];
> +
> + assert(socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, s) == 0);
> + display = wl_display_create();
> + assert(display);
> + client = wl_client_create(display, s[0]);
> + assert(client);
> +
> + /* create few other objects
> +  * -- it doesn't matter what is the interface */
> + resource = wl_resource_create(client, _seat_interface,
> +   wl_seat_interface.version, 0);
> + assert(resource);
> + resource = wl_resource_create(client, _seat_interface,
> +   wl_seat_interface.version, 0);
> + assert(resource);
> + resource = wl_resource_create(client, _seat_interface,
> +   wl_seat_interface.version, 0);
> + assert(resource);
> +
> +
> + /* for the last one we set destroy listener */
> + resource = wl_resource_create(client, _seat_interface,
> +   wl_seat_interface.version, 0);
> + assert(resource);
> +
> + seat_destroy_listener.notify = seat_destroy_get_object;
> + wl_resource_add_destroy_listener(resource, _destroy_listener);
> +
> + wl_client_destroy(client);
> +
> + close(s[0]);
> + close(s[1]);
> +
> + wl_display_destroy(display);
> +}
> +



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

Re: [PATCH wayland 4/5] client-test: test posting no-mem error on client destruction

[Pekka Paalanen]

On Mon, 16 May 2016 13:52:24 +0300
Pekka Paalanen  wrote:

> On Fri, 13 May 2016 15:01:21 +0200
> Marek Chalupa  wrote:
> 
> > while client is being destroyed, it destroys all its resources.
> > Destroy handlers are called for the resources and if some
> > destroy handler post no memory error, we get crash, because
> > the display resource is NULL (it is destroyed as a first resource)
> > 
> > Signed-off-by: Marek Chalupa 
> > ---
> >  tests/client-test.c | 37 +
> >  1 file changed, 37 insertions(+)
> > 
> > diff --git a/tests/client-test.c b/tests/client-test.c
> > index 2e332f8..cb08e39 100644
> > --- a/tests/client-test.c
> > +++ b/tests/client-test.c
> > @@ -89,3 +89,40 @@ TEST(client_destroy_listener)
> > wl_display_destroy(display);
> >  }
> >  
> > +static struct wl_listener seat_destroy_listener;

Hi,

one more thing:

Why is this a global? Should be just fine as a local variable in the
function scope.

I think you should move it.


Thanks,
pq


> > +
> > +static void
> > +seat_destroy_post_no_memory(struct wl_listener *l, void *data)
> > +{
> > +   struct wl_resource *resource = data;  
> 
> Add an empty line.
> 
> > +   wl_client_post_no_memory(wl_resource_get_client(resource));  
> 
> This tests wl_client_post_no_memory(), could it not test
> wl_resource_post_no_memory() at the same time?
> 
> > +}
> > +
> > +TEST(client_post_nomem_on_destruction)
> > +{
> > +   struct wl_display *display;
> > +   struct wl_client *client;
> > +   struct wl_resource *resource;
> > +   int s[2];
> > +
> > +   assert(socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, s) == 0);
> > +   display = wl_display_create();
> > +   assert(display);
> > +   client = wl_client_create(display, s[0]);
> > +   assert(client);
> > +
> > +   resource = wl_resource_create(client, _seat_interface,
> > + wl_seat_interface.version, 0);
> > +   assert(resource);
> > +
> > +   seat_destroy_listener.notify = seat_destroy_post_no_memory;
> > +   wl_resource_add_destroy_listener(resource, _destroy_listener);
> > +
> > +   wl_client_destroy(client);
> > +
> > +   close(s[0]);
> > +   close(s[1]);
> > +
> > +   wl_display_destroy(display);
> > +}
> > +  
> 
> Anyway
> Reviewed-by: Pekka Paalanen 
> 
> 
> Thanks,
> pq



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

Re: [PATCH wayland 4/5] client-test: test posting no-mem error on client destruction

[Pekka Paalanen]

On Fri, 13 May 2016 15:01:21 +0200
Marek Chalupa  wrote:

> while client is being destroyed, it destroys all its resources.
> Destroy handlers are called for the resources and if some
> destroy handler post no memory error, we get crash, because
> the display resource is NULL (it is destroyed as a first resource)
> 
> Signed-off-by: Marek Chalupa 
> ---
>  tests/client-test.c | 37 +
>  1 file changed, 37 insertions(+)
> 
> diff --git a/tests/client-test.c b/tests/client-test.c
> index 2e332f8..cb08e39 100644
> --- a/tests/client-test.c
> +++ b/tests/client-test.c
> @@ -89,3 +89,40 @@ TEST(client_destroy_listener)
>   wl_display_destroy(display);
>  }
>  
> +static struct wl_listener seat_destroy_listener;
> +
> +static void
> +seat_destroy_post_no_memory(struct wl_listener *l, void *data)
> +{
> + struct wl_resource *resource = data;

Add an empty line.

> + wl_client_post_no_memory(wl_resource_get_client(resource));

This tests wl_client_post_no_memory(), could it not test
wl_resource_post_no_memory() at the same time?

> +}
> +
> +TEST(client_post_nomem_on_destruction)
> +{
> + struct wl_display *display;
> + struct wl_client *client;
> + struct wl_resource *resource;
> + int s[2];
> +
> + assert(socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, s) == 0);
> + display = wl_display_create();
> + assert(display);
> + client = wl_client_create(display, s[0]);
> + assert(client);
> +
> + resource = wl_resource_create(client, _seat_interface,
> +   wl_seat_interface.version, 0);
> + assert(resource);
> +
> + seat_destroy_listener.notify = seat_destroy_post_no_memory;
> + wl_resource_add_destroy_listener(resource, _destroy_listener);
> +
> + wl_client_destroy(client);
> +
> + close(s[0]);
> + close(s[1]);
> +
> + wl_display_destroy(display);
> +}
> +

Anyway
Reviewed-by: Pekka Paalanen 


Thanks,
pq


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

Re: [PATCH 1/6] rdp: allow to compile against FreeRDP 2.0

[Quentin Glidic]

On 01/05/2016 23:32, David Fort wrote:

FreeRDP 2.0 is about to be released, this allows to compile against this 
version.
The detection is adjusted to prefer FreeRDP 2 against version 1.x.

Signed-off-by: David Fort 


Reviewed-by: Quentin Glidic 

If think you can retain Rb from Bryce too, as you only changed the 
Autotools chunk.


Thanks :-)

--

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

Re: Introduction and updates from NVIDIA

[Daniel Vetter]

On Sat, May 14, 2016 at 05:46:51PM +0100, Daniel Stone wrote:
> On 12 May 2016 at 00:08, James Jones  wrote:
> > The EGLStream encapsulation takes into consideration the new use cases
> > EGLImage, GBM, etc. were intended to address, and restores what I believe to
> > be the minimal amount of the traditional GL+GLX/EGL/etc. model, while still
> > allowing as much of the flexibility of the "a bunch of buffers" mental model
> > as possible.  We can re-invent that with GBM API adjustments, a set of
> > restrictions on how the buffers it allocates can be used, and another layer
> > of metadata being pumped into drivers on top of that, but I suspect we'd
> > wind up with something that looks very similar to streams.
> 
> The only allocation GBM does is for buffers produced by the compositor
> and used for scanout, so in this regard it's quite straightforward.
> Client buffers are a separate topic, and I don't buy that the
> non-Streams model precludes things like render compression. In fact,
> Ben Widawsky, Dan Vetter, and some others are as we speak working on
> support for render compression within both Wayland EGL and GBM itself
> (for direct scanout from compressed buffers with an auxiliary plane).
> So far, the only external impact has been a very small extension to
> the GBM API to allow use of multiple planes and FB modifiers: a far
> smaller change than implementing the whole of Streams and all its
> future extensions (Switch et al).

Just a quick correction: For render compression we also do need some
allocation hinting interface, since on intel gpus you can't always scan
out render compressed buffers. So exactly what EGLstreams tries to also
solve (at least if my understanding is correct). So we need a bit more in
gbm than just be able to pass fb modifiers around.

I still think it's the better approach though since it's still fairly
incremental. And exposing the allocation hints and making them explicitly
will avoid the need to teach everything in the world about EGLstreams (vk,
v4l, drm, ...). Which as Daniel Stone pointed out, doesn't really work
well if you have IP blocks from multiple vendors on your SoC.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland 2/5] server: set map entry to NULL after resource is destroyed

[Pekka Paalanen]

On Fri, 13 May 2016 15:01:19 +0200
Marek Chalupa  wrote:

> We did it only for client entries for some reason, so when
> we used wl_client_get_object() for some server object that
> has been destroyed, we got dangling pointer.
> 
> NOTE: this is basically an API change, since it changes
> the return value of wl_client_get_object() in some corner cases.
> However, now we return NULL insted of a pointer to invalid memory,
> which could be OK API break.

Hi Marek,

I'm not sure I see why there is something to fix here.

Let's assume we gave a server-side created wl_resource, and
wl_resource_destroy() is called on it:

wl_resource_destroy():
- destroy_resource():
- - does nothing to the map, really
- server-side created object so get to else-branch of
  (id < WL_SERVER_ID_START)
- wl_map_remove():
- - The entry is completely removed from the map, so wl_map_lookup() on
it will return NULL.
- At this point the id longer exists and can be re-used, so no lookups
  should be made on it.

Then if we look at wl_client_get_object():
- wl_map_lookup()
Ok, so this will just return NULL now, but assuming the id has not been
re-used. If it was re-used, it returns a valid pointer to a "wrong"
object.

Did I miss something?

To me it seems there are two things that make the premise of this patch
invalid:
1. You already get NULL from wl_client_lookup_object().
2. The lookup would be invalid anyway as the id as you knew it does not
   exist anymore.


The other call site for destroy_resource() is wl_client_destroy() which
also means all ids are invalidated.

Now, client entries are NULLed, because they *need* to stay in the map,
because a) the free_list is only for the map->side of the two object
arrays in a wl_map, and b) wl_map_insert_at() will only increase the
array size by one at most. The latter is also limiting what ids clients
can allocate at a time.

Therefore, NAK for this patch because the premise why this patch was
written is false.

If you wanted to apply this only as a clean-up or clarification, I'm
not sure it would be a win. wl_map_remove() would still be called only
for server-allocated ids.


Thanks,
pq

> Signed-off-by: Marek Chalupa 
> ---
>  src/wayland-server.c | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index f745e62..c93a426 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -562,16 +562,20 @@ destroy_resource(void *element, void *data)
>  {
>   struct wl_resource *resource = element;
>   struct wl_client *client = resource->client;
> + uint32_t id = resource->object.id;
>   uint32_t flags;
>  
>   wl_signal_emit(>destroy_signal, resource);
>  
> - flags = wl_map_lookup_flags(>objects, resource->object.id);
> + flags = wl_map_lookup_flags(>objects, id);
>   if (resource->destroy)
>   resource->destroy(resource);
>  
>   if (!(flags & WL_MAP_ENTRY_LEGACY))
>   free(resource);
> +
> + /* replace the object with NULL since it is destroyed */
> + wl_map_insert_at(>objects, 0, id, NULL);
>  }
>  
>  WL_EXPORT void
> @@ -584,11 +588,9 @@ wl_resource_destroy(struct wl_resource *resource)
>   destroy_resource(resource, NULL);
>  
>   if (id < WL_SERVER_ID_START) {
> - if (client->display_resource) {
> + if (client->display_resource)
>   wl_resource_queue_event(client->display_resource,
>   WL_DISPLAY_DELETE_ID, id);
> - }
> - wl_map_insert_at(>objects, 0, id, NULL);
>   } else {
>   wl_map_remove(>objects, id);
>   }



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