Re: [PATCH 3/5] scanner: introduce --object-type option

[Jonas Ådahl]

On Wed, Jan 24, 2018 at 07:15:09PM +, Emil Velikov wrote:
> On 24 January 2018 at 18:20, Derek Foreman  wrote:
> > On 2018-01-22 09:30 AM, Emil Velikov wrote:
> >>
> >> On 22 August 2017 at 14:02, Emil Velikov  wrote:
> >>>
> >>> On 18 August 2017 at 13:05, Pekka Paalanen  wrote:
> >>>
> >>
> >> The exported configuration would then be:
> >> LOCAL_INTERFACE_DECL=extern
> >> EXTERN_INTERFACE_DECL=extern
> >> LOCAL_INTERFACE_DEF=WL_EXPORT
> >>
> >> That would be far too flexible and no-one would use it right, right?
> >>
> > I did not introduce separate tokens, since those are (and should be)
> > used _only_ in the .c file.
> > Personally then do not seem necessary, If you prefer we can add them
> > though.
> 
> 
>  Ah, no, that was just a wild idea of something completely different. I
>  meant that the user project would be setting those macros before using
>  scanner-generated files, and if unset, the scanner-emitted code would
>  default to the legacy behaviour. That way there would be no visibility
>  modes in scanner itself. If it's not obviously better, then nevermind.
>  It certainly has a lot more room to go wrong than your proposal.
> 
> 
> >>> I see.
> >>>
> >>> Personally I'd lean towards with my approach for now since it is
> >>> simpler, despite that it provides less flexibility.
> >>> As you pointed out the proposal is a bit more fragile, so might be
> >>> better to avoid until there's a real need for it.
> >>>
> >>>
>  ...
> 
> >> The patch looks pretty much correct to me, if we choose to go this
> >> way.
> >>
> > Glad to hear.
> >
> > I'll let me know once you guys are settled in on the approach, and
> > I'll respin the series with all the comments addressed.
> 
> 
>  Cool, let's see if we can get the name conflict issue solved, and then
>  I'll try to remember to ping you.
> 
> >>> Ack, I'll keep an eye open, just in case.
> >>>
> >> Considering the status of the the name conflict series, should I
> >> re-spin this lot?
> >> I'm more than happy to tweak things - say rename the toggle, etc.
> >
> >
> > I see there were two series proposed to control symbol visibility, yours and
> > Jonas'?
> >
> > Assuming that once we drop the symbol collision issue they both solve the
> > same problems, it would be good if we could focus on one going forward.
> >
> > Is this the chosen one?
> >
> Right, the cover letter [1] covers some of the high-lights/differences.
> As a TL;DR: using static/shared is more common and gives us more
> flexibility for the future.
> 
> So far Pekka is the only person who commented on the series/approach
> and seemed happy.
> I was expecting others to weight in - say Jonas ;-) I'll respin the
> series tomorrow.
> 
> In hindsight --object-type={shared,static} is too much of a mouthful,
> I'll opt for --{static,shared} for v2.

I have no opinion of what variant to land (I'm slightly too lazy to
search for my own, and this is high up the inbox thanks to the replies,
so lets focus on this one).

My only nit is using the term "object-type". I think refering to it as
"visibility" ("symbol visibility" if wanting to be extra verbose) where
one can say 'export', 'static' or 'private' is more accurate.

"objects" are a Wayland protocol thing and that is not what we are
poking at here.


Jonas

> 
> -Emil
> 
> [1] https://patchwork.freedesktop.org/series/27939/
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston] xwm: Fix icon surface ownership

[Emmanuel Gil Peyrot]

The cairo surface used for the icon must be completely given to the
frame as soon as said frame has been created.  To prevent both the
window and the frame from sharing ownership of the icon, we set
window->icon_surface back to NULL right after creating or changing the
frame, only keeping it there when no frame has been created yet.

Fixes 
https://lists.freedesktop.org/archives/wayland-devel/2018-January/036655.html
Reported-by: Derek Foreman 
Tested-by: Derek Foreman 
Signed-off-by: Emmanuel Gil Peyrot 
---
 shared/cairo-util.h   |  4 
 shared/frame.c| 14 ++
 xwayland/window-manager.c | 16 ++--
 3 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/shared/cairo-util.h b/shared/cairo-util.h
index bab48083..6fd11f6b 100644
--- a/shared/cairo-util.h
+++ b/shared/cairo-util.h
@@ -135,6 +135,10 @@ frame_destroy(struct frame *frame);
 int
 frame_set_title(struct frame *frame, const char *title);
 
+/* May set FRAME_STATUS_REPAINT */
+void
+frame_set_icon(struct frame *frame, cairo_surface_t *icon);
+
 /* May set FRAME_STATUS_REPAINT */
 void
 frame_set_flag(struct frame *frame, enum frame_flag flag);
diff --git a/shared/frame.c b/shared/frame.c
index dc7ff85c..3a7d9199 100644
--- a/shared/frame.c
+++ b/shared/frame.c
@@ -423,6 +423,20 @@ frame_set_title(struct frame *frame, const char *title)
return 0;
 }
 
+void
+frame_set_icon(struct frame *frame, cairo_surface_t *icon)
+{
+   struct frame_button *button;
+   wl_list_for_each(button, >buttons, link) {
+   if (button->status_effect != FRAME_STATUS_MENU)
+   continue;
+   if (button->icon)
+   cairo_surface_destroy(button->icon);
+   button->icon = icon;
+   frame->status |= FRAME_STATUS_REPAINT;
+   }
+}
+
 void
 frame_set_flag(struct frame *frame, enum frame_flag flag)
 {
diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
index ac44a29a..c307e199 100644
--- a/xwayland/window-manager.c
+++ b/xwayland/window-manager.c
@@ -139,7 +139,7 @@ struct weston_wm_window {
struct frame *frame;
cairo_surface_t *cairo_surface;
int icon;
-   cairo_surface_t *icon_surface;
+   cairo_surface_t *icon_surface; /* A temporary slot, to be passed to 
frame on creation */
uint32_t surface_id;
struct weston_surface *surface;
struct weston_desktop_xwayland_surface *shsurf;
@@ -994,6 +994,7 @@ weston_wm_window_create_frame(struct weston_wm_window 
*window)
 window->width, window->height,
 buttons, window->name,
 window->icon_surface);
+   window->icon_surface = NULL;
frame_resize_inside(window->frame, window->width, window->height);
 
weston_wm_window_get_frame_size(window, , );
@@ -1392,10 +1393,10 @@ weston_wm_handle_icon(struct weston_wm *wm, struct 
weston_wm_window *window)
return;
}
 
-   if (window->icon_surface)
-   cairo_surface_destroy(window->icon_surface);
-
-   window->icon_surface = new_surface;
+   if (window->frame)
+   frame_set_icon(window->frame, new_surface);
+   else /* We don’t have a frame yet */
+   window->icon_surface = new_surface;
 }
 
 static void
@@ -1422,7 +1423,10 @@ weston_wm_handle_property_notify(struct weston_wm *wm, 
xcb_generic_event_t *even
if (property_notify->state != XCB_PROPERTY_DELETE) {
weston_wm_handle_icon(wm, window);
} else {
-   cairo_surface_destroy(window->icon_surface);
+   if (window->frame)
+   frame_set_icon(window->frame, NULL);
+   if (window->icon_surface)
+   cairo_surface_destroy(window->icon_surface);
window->icon_surface = NULL;
}
weston_wm_window_schedule_repaint(window);
-- 
2.16.1

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

Re: [PATCH weston] RFC: libweston/compositor-drm: Add support for drm-lease.

[Daniel Stone]

Hi Marius,
The protocol changes I suggested would require a fair bit of work
here, but I've enclosed a few comments on the implementation.

Also, do you have a client you're using for this somewhere, that we
could use to test?

On 24 January 2018 at 19:11, Marius Vlad  wrote:
> @@ -1208,6 +1209,15 @@ drm_backend_output_configure(struct wl_listener 
> *listener, void *data)
>
> api->set_seat(output, seat);
> free(seat);
> +#ifdef HAVE_DRM_LEASE
> +   char *lease;
> +   weston_config_section_get_string(section, "lease", , "off");
> +   if (!strncmp(lease, "on", 2)) {
> +   output->lease = true;
> +   weston_log("Enabling lease on output %s\n", output->name);
> +   }
> +   free(lease);
> +#endif

Hm, doing this in generic code seems a bit odd.

> +#ifdef HAVE_DRM_LEASE
> +/* hold current leases given */
> +static struct wl_list leases;

This should be under drm_backend.

> +struct drm_display {
> +   uint32_t connector_id;
> +   uint32_t crtc_id;
> +   uint32_t plane_id;
> +};

This seems to get stored but not used after that?

> +struct drm_lease {
> +   int leased_fd;
> +   uint32_t leased_id;
> +   struct wl_list list;
> +};

The convention for embedded struct wl_list which is an element of a
list, rather than a list head, is to call it 'link'.

> +struct drm_lease_data {
> +   struct drm_backend *drm_backend;
> +   struct udev_device *drm_device;
> +};

This is a bit odd as we only have one udev device in the backend. Like
the lease list though, any data for leases should just live directly
in the drm_backend.

> +#ifdef HAVE_DRM_LEASE
> +static void
> +drm_lease_destroy(struct drm_backend *b)
> +{
> +   struct drm_lease *lease, *lease_iter;
> +
> +   wl_list_for_each_safe(lease, lease_iter, , list) {
> +   if (drmModeRevokeLease(b->drm.fd, lease->leased_id) < 0) {
> +   weston_log("Failed to revoke lease id %u\n",
> +  lease->leased_id);
> +   continue;
> +   }
> +
> +   weston_log("Lease id %u revoked\n", lease->leased_id);
> +   wl_list_remove(>list);
> +   free(lease);
> +   }
> +}
> +#endif

If individual leases were a separate object, you could have
drmModeRevokeLease inside the resource destruction handler; this would
then get called when either the client or the compositor was
destroyed.

> +static void
> +drm_lease_create(struct wl_client *client, struct wl_resource *resource)
> +{
> +   struct drm_lease_data *user_data = 
> wl_resource_get_user_data(resource);
> +   struct weston_compositor *compositor = 
> user_data->drm_backend->compositor;
> +   int drm_fd = user_data->drm_backend->drm.fd;
> +
> +   struct drm_output *output = NULL;
> +   struct drm_output *choosen_output = NULL;
> +
> +   uint32_t objects[3];
> +   uint32_t nobjects;
> +
> +   struct drm_lease *lease;
> +   struct drm_display display = {};
> +
> +   wl_list_for_each(output, >output_list, base.link) {
> +   struct weston_output *wet_output = >base;
> +   if (wet_output->lease) {
> +   display.crtc_id = output->crtc_id;
> +   display.connector_id = output->connector_id;
> +   display.plane_id = output->scanout_plane->plane_id;
> +
> +   choosen_output = output;
> +   break;
> +   }
> +   }
> +
> +   if (!choosen_output) {
> +   weston_log("No valid output found to lease!\n");
> +   zwp_drm_lease_v1_send_failed(resource);
> +   return;
> +   }
> +
> +   drm_lease_save_objects(, objects, );
> +   lease = zalloc(sizeof(*lease));
> +
> +   /* create lease */
> +   lease->leased_fd = drmModeCreateLease(drm_fd, objects, nobjects, 0,
> + >leased_id);
> +   if (lease->leased_fd < 0) {
> +   weston_log("Failed to create the lease!");
> +   free(lease);
> +   zwp_drm_lease_v1_send_failed(resource);
> +   return;
> +   }
> +
> +   weston_log("Lease leased_id = %u created, on output %s\n",
> +  lease->leased_id, choosen_output->base.name);
> +
> +   wl_list_insert(, >list);
> +   drm_output_destroy(_output->base);
> +
> +   zwp_drm_lease_v1_send_created(resource, lease->leased_fd,
> + lease->leased_id);
> +}

Rather than destroying the output, I'd be much more comfortable
marking it as disabled or so. There is also a race here: in the DRM
backend, output destruction can be deferred, when a pageflip has not
yet completed.

> +static void
> +drm_lease_revoke(struct wl_client *client, struct wl_resource *resource, 
> uint32_t id)
> +{
> +   struct drm_lease *lease, 

Re: [PATCH wayland-protocols] RFC: unstable: DRM lease support

[Daniel Stone]

Hi Marius,
Thanks a lot for taking this on! It would be great to get this merged.

On 24 January 2018 at 19:09, Marius Vlad  wrote:
> +  
> +
> +  This interface makes use of DRM lease written by Keith Packard.
> +  It requires libdrm at least 2.4.89 and a recent (4.15) kernel.

A serious nitpick, but we can leave the versions out of this,
especially as people will backport support to older versions.

> +  Events:
> +
> +  - created -- event sent when the lease has been created succesfully
> +  - revoked -- event sent when the lease has been revoked succesfully
> +  - failed -- event sent when create/revoke requests failed.
> +
> +  Requests:
> +
> +  - create -- asks the server to create a lease. The server decides which
> +  ouput to lease to the client. In case of success, the client receives
> +  an event with a DRM capable-fd. The client can then issue libdrm
> +  ioctls (i.e., modesetting).  The client also receives a lessee_id which
> +  has be used in revoke request. In case of failure, a failed event will
> +  be sent.
> +  - revoke -- asks the server to revoke a previously leased fd, using the
> +  lessee_id.
> +  A revoke event will be sent in case of success or failed event 
> otherwise.

Also, the events/requests are already described in the actual
protocol. A couple of paragraphs about what DRM leases actually are,
and why you would/wouldn't want to use them, would be more useful I
think.

> +
> +  
> +   This asks for the creation of a lease.
> +  
> +
> +
> +
> +
> +   This event indicates that the lease has been created. It provides the
> +   leased fd which the client can use to perform modesetting and a lessee
> +   id to revoke the lease when it has finished with it.
> +
> +
> +
> +
> +
> +
> +
> +   This event indicates that the lease could not be created/revoked.
> +
> +
> +
> +
> +  
> +   This asks to revoke a lease using the lessee id previously given in 
> event
> +   created.
> +  
> +  
> +
> +
> +
> +
> +   This event indicates that the leased has been revoked.
> +
> +

This interface seems a little idiosyncratic. Essentially, the client
asks for creation of one lease (any lease), and the server returns it
a lease with an ID. After that, the client destroys all the leases
through the same interface. There are a couple of things I would
suggest to improve this protocol, and make it more like other Wayland
protocols:

Most Wayland protocols carry lots of small objects, since creating
them is lightweight and straightforward. I think this protocol could
be improved by doing something similar to the dmabuf protocol, which
carries three objects: one global which is pretty much just for
extension advertisement, one temporary object used in the creation of
a buffer, and then the buffer object itself. Applied to leases, this
would be a zwp_kms_lease_manager_v1 global which only had two
requests: one destroy, and the other to create a wp_kms_lease_request
object. zwp_kms_lease_request_v1 would be analagous to
zwp_linux_dmabuf_params_v1: it would have requests to lease particular
parts of the device (e.g. HDMI-2 output as well as two planes), and
successful/failed events.

The successful event would carry a zwp_kms_lease_v1 object, which
would represent the actual lease itself, only having a 'destroy'
method. Essentially, this object would do nothing but represent the
lifetime of a lease. This is more idiomatic: as a rule of thumb, any
time a request/event takes an 'id' parameter which you have to look up
in a list, this means you should probably be using an object instead.

Other than that, I think it looks really good, and I'd be really happy
to merge it in.

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

Re: [PATCH weston v4] xwm: Add icon support to the frame

[Derek Foreman]

On 2017-12-01 02:47 PM, Quentin Glidic wrote:

On 12/1/17 7:20 PM, Emmanuel Gil Peyrot wrote:

This fetches the _NET_WM_ICON property of the X11 window, and use the
first image found as the frame icon.

This has been tested with various X11 programs, and improves usability
and user-friendliness a bit.

Changes since v1:
- Changed frame_button_create() to use
   frame_button_create_from_surface() internally.
- Removed a check that should never have been commited.

Changes since v2:
- Request UINT32_MAX items instead of 2048, to avoid cutting valid
   icons.
- Strengthen checks against malformed input.
- Handle XCB_PROPERTY_DELETE to remove the icon.
- Schedule a repaint if the icon changed.

Changes since v3:
- Keep the previous Cairo surface until the new one has been
   successfully loaded.
- Use uint32_t for cardinals.  Unsigned is the same type except on
   16-bit machines, but uint32_t is clearer.
- Declare length as uint32_t too, like in xcb_get_property_reply_t.

Signed-off-by: Emmanuel Gil Peyrot 
Reviewed-by: Quentin Glidic 


Just re-checked to be sure, found a tiny thing I overlooked (see below), 
but the Rb still stands anyway.




---
  clients/window.c   |  4 +--
  libweston/compositor-wayland.c |  2 +-
  shared/cairo-util.h    |  2 +-
  shared/frame.c | 54 ++-
  xwayland/window-manager.c  | 65 
--

  5 files changed, 107 insertions(+), 20 deletions(-)

diff --git a/clients/window.c b/clients/window.c
index 95796d46..15a86e15 100644
--- a/clients/window.c
+++ b/clients/window.c
@@ -2546,7 +2546,7 @@ window_frame_create(struct window *window, void 
*data)

  frame = xzalloc(sizeof *frame);
  frame->frame = frame_create(window->display->theme, 0, 0,
-    buttons, window->title);
+    buttons, window->title, NULL);
  frame->widget = window_add_widget(window, frame);
  frame->child = widget_add_widget(frame->widget, data);
@@ -5449,7 +5449,7 @@ create_menu(struct display *display,
  menu->user_data = user_data;
  menu->widget = window_add_widget(menu->window, menu);
  menu->frame = frame_create(window->display->theme, 0, 0,
-   FRAME_BUTTON_NONE, NULL);
+   FRAME_BUTTON_NONE, NULL, NULL);
  fail_on_null(menu->frame, 0, __FILE__, __LINE__);
  menu->entries = entries;
  menu->count = count;
diff --git a/libweston/compositor-wayland.c 
b/libweston/compositor-wayland.c

index 3bdfb03e..c5290d85 100644
--- a/libweston/compositor-wayland.c
+++ b/libweston/compositor-wayland.c
@@ -869,7 +869,7 @@ wayland_output_set_windowed(struct wayland_output 
*output)

  return -1;
  }
  output->frame = frame_create(b->theme, 100, 100,
- FRAME_BUTTON_CLOSE, output->title);
+ FRAME_BUTTON_CLOSE, output->title, 
NULL);

  if (!output->frame)
  return -1;
diff --git a/shared/cairo-util.h b/shared/cairo-util.h
index 84cf005e..7893ca24 100644
--- a/shared/cairo-util.h
+++ b/shared/cairo-util.h
@@ -126,7 +126,7 @@ enum {
  struct frame *
  frame_create(struct theme *t, int32_t width, int32_t height, 
uint32_t buttons,

- const char *title);
+ const char *title, cairo_surface_t *icon);
  void
  frame_destroy(struct frame *frame);
diff --git a/shared/frame.c b/shared/frame.c
index eb0cd77a..32779856 100644
--- a/shared/frame.c
+++ b/shared/frame.c
@@ -106,9 +106,9 @@ struct frame {
  };
  static struct frame_button *
-frame_button_create(struct frame *frame, const char *icon,
-    enum frame_status status_effect,
-    enum frame_button_flags flags)
+frame_button_create_from_surface(struct frame *frame, cairo_surface_t 
*icon,

+ enum frame_status status_effect,
+ enum frame_button_flags flags)
  {
  struct frame_button *button;
@@ -116,12 +116,7 @@ frame_button_create(struct frame *frame, const 
char *icon,

  if (!button)
  return NULL;
-    button->icon = cairo_image_surface_create_from_png(icon);
-    if (!button->icon) {
-    free(button);
-    return NULL;
-    }
-
+    button->icon = icon;


This line makes a copy of the icon cairo surface that isn't updated when 
(see below)



  button->frame = frame;
  button->flags = flags;
  button->status_effect = status_effect;
@@ -131,6 +126,30 @@ frame_button_create(struct frame *frame, const 
char *icon,

  return button;
  }
+static struct frame_button *
+frame_button_create(struct frame *frame, const char *icon_name,
+    enum frame_status status_effect,
+    enum frame_button_flags flags)
+{
+    struct frame_button *button;
+    cairo_surface_t *icon;
+
+    icon = cairo_image_surface_create_from_png(icon_name);
+    if 

Re: [PATCH 3/5] scanner: introduce --object-type option

[Emil Velikov]

On 24 January 2018 at 18:20, Derek Foreman  wrote:
> On 2018-01-22 09:30 AM, Emil Velikov wrote:
>>
>> On 22 August 2017 at 14:02, Emil Velikov  wrote:
>>>
>>> On 18 August 2017 at 13:05, Pekka Paalanen  wrote:
>>>
>>
>> The exported configuration would then be:
>> LOCAL_INTERFACE_DECL=extern
>> EXTERN_INTERFACE_DECL=extern
>> LOCAL_INTERFACE_DEF=WL_EXPORT
>>
>> That would be far too flexible and no-one would use it right, right?
>>
> I did not introduce separate tokens, since those are (and should be)
> used _only_ in the .c file.
> Personally then do not seem necessary, If you prefer we can add them
> though.


 Ah, no, that was just a wild idea of something completely different. I
 meant that the user project would be setting those macros before using
 scanner-generated files, and if unset, the scanner-emitted code would
 default to the legacy behaviour. That way there would be no visibility
 modes in scanner itself. If it's not obviously better, then nevermind.
 It certainly has a lot more room to go wrong than your proposal.


>>> I see.
>>>
>>> Personally I'd lean towards with my approach for now since it is
>>> simpler, despite that it provides less flexibility.
>>> As you pointed out the proposal is a bit more fragile, so might be
>>> better to avoid until there's a real need for it.
>>>
>>>
 ...

>> The patch looks pretty much correct to me, if we choose to go this
>> way.
>>
> Glad to hear.
>
> I'll let me know once you guys are settled in on the approach, and
> I'll respin the series with all the comments addressed.


 Cool, let's see if we can get the name conflict issue solved, and then
 I'll try to remember to ping you.

>>> Ack, I'll keep an eye open, just in case.
>>>
>> Considering the status of the the name conflict series, should I
>> re-spin this lot?
>> I'm more than happy to tweak things - say rename the toggle, etc.
>
>
> I see there were two series proposed to control symbol visibility, yours and
> Jonas'?
>
> Assuming that once we drop the symbol collision issue they both solve the
> same problems, it would be good if we could focus on one going forward.
>
> Is this the chosen one?
>
Right, the cover letter [1] covers some of the high-lights/differences.
As a TL;DR: using static/shared is more common and gives us more
flexibility for the future.

So far Pekka is the only person who commented on the series/approach
and seemed happy.
I was expecting others to weight in - say Jonas ;-) I'll respin the
series tomorrow.

In hindsight --object-type={shared,static} is too much of a mouthful,
I'll opt for --{static,shared} for v2.

-Emil

[1] https://patchwork.freedesktop.org/series/27939/
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston] RFC: libweston/compositor-drm: Add support for drm-lease.

[Marius Vlad]

DRM leases is a method developed by Keith Packard to allow other application
manage the output of a display/VR, while a DRM master is already owning the
outputs resources. A more thorough explanation and terminology can be found at
[1].

This patch adds support for DRM lease. libdrm is lease-aware in 2.4.89,
while the kernel support has landed in Linus's master tree (4.15).

The easiest way to choose which output to lease is to have a 'lease' entry for
the output in the config file. My assumption is based on the fact that one would
not want to destroy/close the application running on a particular output. Upon
leasing an output to a client that output will no longer be managed by weston.
When the application is done with the lease it will send a revoke request,
moment in which weston will reclaim the output.

For instance the following configuration file can lease HDMI-A-1 output to other
application.

[output]
name=HDMI-A-1
lease=on

The patch relies on previous drm-lease-unstable-v1 extension protocol
posted previously [4].

[1] https://keithp.com/blogs/DRM-lease/
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v4.15-rc9=62884cd386b876638720ef88374b31a84ca7ee5f
[3] 
https://cgit.freedesktop.org/mesa/drm/commit/?id=c4171535389d72e9135c9615cecd07b346fd6d7e
[4] 
https://lists.freedesktop.org/archives/wayland-devel/2018-January/036652.html

Signed-off-by: Marius Vlad 
---
 Makefile.am|   2 +
 compositor/main.c  |  10 +++
 configure.ac   |   4 +-
 libweston/compositor-drm.c | 204 -
 libweston/compositor.c |   1 +
 libweston/compositor.h |   3 +
 man/weston.ini.man |   8 ++
 7 files changed, 230 insertions(+), 2 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index e224d60..9a5fcb9 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -178,6 +178,8 @@ nodist_libweston_@LIBWESTON_MAJOR@_la_SOURCES = 
\
protocol/viewporter-server-protocol.h   \
protocol/linux-dmabuf-unstable-v1-protocol.c\
protocol/linux-dmabuf-unstable-v1-server-protocol.h \
+   protocol/drm-lease-unstable-v1-protocol.c   \
+   protocol/drm-lease-unstable-v1-server-protocol.h\
protocol/relative-pointer-unstable-v1-protocol.c\
protocol/relative-pointer-unstable-v1-server-protocol.h \
protocol/pointer-constraints-unstable-v1-protocol.c \
diff --git a/compositor/main.c b/compositor/main.c
index 7feb4cb..bf1712c 100644
--- a/compositor/main.c
+++ b/compositor/main.c
@@ -1186,6 +1186,7 @@ drm_backend_output_configure(struct wl_listener 
*listener, void *data)
modeline = s;
s = NULL;
}
+
free(s);
 
if (api->set_mode(output, mode, modeline) < 0) {
@@ -1208,6 +1209,15 @@ drm_backend_output_configure(struct wl_listener 
*listener, void *data)
 
api->set_seat(output, seat);
free(seat);
+#ifdef HAVE_DRM_LEASE
+   char *lease;
+   weston_config_section_get_string(section, "lease", , "off");
+   if (!strncmp(lease, "on", 2)) {
+   output->lease = true;
+   weston_log("Enabling lease on output %s\n", output->name);
+   }
+   free(lease);
+#endif
 
weston_output_enable(output);
 }
diff --git a/configure.ac b/configure.ac
index 6f295dc..444666b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -209,9 +209,11 @@ if test x$enable_drm_compositor = xyes; then
   PKG_CHECK_MODULES(DRM_COMPOSITOR_GBM, [gbm >= 10.2],
[AC_DEFINE([HAVE_GBM_FD_IMPORT], 1, [gbm supports dmabuf 
import])],
[AC_MSG_WARN([gbm does not support dmabuf import, will omit 
that capability])])
+  PKG_CHECK_MODULES(DRM_LEASE, [libdrm >= 2.4.89],
+   [AC_DEFINE([HAVE_DRM_LEASE], 1, [libdrm support lease 
capability])],
+   [AC_MSG_WARN([libdrm doesn't have leases support, will omit 
that capability])])
 fi
 
-
 PKG_CHECK_MODULES(LIBINPUT_BACKEND, [libinput >= 0.8.0])
 PKG_CHECK_MODULES(COMPOSITOR, [$COMPOSITOR_MODULES])
 
diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index fe59bf5..98c2e6b 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -62,6 +62,7 @@
 #include "presentation-time-server-protocol.h"
 #include "linux-dmabuf.h"
 #include "linux-dmabuf-unstable-v1-server-protocol.h"
+#include "drm-lease-unstable-v1-server-protocol.h"
 
 #ifndef DRM_CAP_TIMESTAMP_MONOTONIC
 #define DRM_CAP_TIMESTAMP_MONOTONIC 0x6
@@ -386,6 +387,30 @@ static struct gl_renderer_interface *gl_renderer;
 
 static const char default_seat[] = "seat0";
 
+#ifdef HAVE_DRM_LEASE
+/* hold current leases given */
+static struct wl_list leases;
+
+struct drm_display {
+   uint32_t connector_id;
+   uint32_t crtc_id;
+   uint32_t plane_id;
+};
+
+struct drm_lease {
+   

[PATCH wayland-protocols] RFC: unstable: DRM lease support

[Marius Vlad]

Simple protocol extension for DRM leases, based on the work done 
by Keith Packard in libdrm [1] and in the Linux kernel [2].

There are two requests (create/revoke) and three events
(created/revoked/failed). The server is responsible for choosing which output to
lease. Another patch will follow that makes use of this procotol extension.

[1] 
https://cgit.freedesktop.org/mesa/drm/commit/?id=c4171535389d72e9135c9615cecd07b346fd6d7e
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v4.15-rc9=62884cd386b876638720ef88374b31a84ca7ee5f

Signed-off-by: Marius Vlad 
---
 Makefile.am  |  1 +
 unstable/drm-lease/README|  4 ++
 unstable/drm-lease/drm-lease-unstable-v1.xml | 98 
 3 files changed, 103 insertions(+)
 create mode 100644 unstable/drm-lease/README
 create mode 100644 unstable/drm-lease/drm-lease-unstable-v1.xml

diff --git a/Makefile.am b/Makefile.am
index 4b9a901..4f6a874 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2,6 +2,7 @@ unstable_protocols =
\
unstable/pointer-gestures/pointer-gestures-unstable-v1.xml  
\
unstable/fullscreen-shell/fullscreen-shell-unstable-v1.xml  
\
unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml  
\
+   unstable/drm-lease/drm-lease-unstable-v1.xml
\
unstable/text-input/text-input-unstable-v1.xml  
\
unstable/input-method/input-method-unstable-v1.xml  
\
unstable/xdg-shell/xdg-shell-unstable-v5.xml
\
diff --git a/unstable/drm-lease/README b/unstable/drm-lease/README
new file mode 100644
index 000..a25600c
--- /dev/null
+++ b/unstable/drm-lease/README
@@ -0,0 +1,4 @@
+Linux DRM lease
+
+Maintainers:
+Marius Vlad 
diff --git a/unstable/drm-lease/drm-lease-unstable-v1.xml 
b/unstable/drm-lease/drm-lease-unstable-v1.xml
new file mode 100644
index 000..ec67456
--- /dev/null
+++ b/unstable/drm-lease/drm-lease-unstable-v1.xml
@@ -0,0 +1,98 @@
+
+
+
+  
+Copyright 2018 NXP
+
+Permission is hereby granted, free of charge, to any person obtaining a
+copy of this software and associated documentation files (the "Software"),
+to deal in the Software without restriction, including without limitation
+the rights to use, copy, modify, merge, publish, distribute, sublicense,
+and/or sell copies of the Software, and to permit persons to whom the
+Software is furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice (including the next
+paragraph) shall be included in all copies or substantial portions of the
+Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+DEALINGS IN THE SOFTWARE.
+  
+
+  
+
+  This interface makes use of DRM lease written by Keith Packard.
+  It requires libdrm at least 2.4.89 and a recent (4.15) kernel.
+
+  Events:
+
+  - created -- event sent when the lease has been created succesfully
+  - revoked -- event sent when the lease has been revoked succesfully
+  - failed -- event sent when create/revoke requests failed.
+
+  Requests:
+
+  - create -- asks the server to create a lease. The server decides which
+  ouput to lease to the client. In case of success, the client receives
+  an event with a DRM capable-fd. The client can then issue libdrm
+  ioctls (i.e., modesetting).  The client also receives a lessee_id which
+  has be used in revoke request. In case of failure, a failed event will
+  be sent.
+  - revoke -- asks the server to revoke a previously leased fd, using the
+  lessee_id.
+  A revoke event will be sent in case of success or failed event otherwise.
+
+  Warning! The protocol described in this file is experimental and
+  backward incompatible changes may be made. Backward compatible changes
+  may be added together with the corresponding interface version bump.
+  Backward incompatible changes are done by bumping the version number in
+  the protocol and interface names and resetting the interface version.
+  Once the protocol is to be declared stable, the 'z' prefix and the
+  version number in the protocol and interface names are removed and the
+  interface version number is reset.
+
+
+
+  

Re: [PATCH xserver] xwayland: Support for BTN_STYLUS3 kernel events

[Jason Gerecke]

On Thu, Nov 9, 2017 at 2:19 PM, Jason Gerecke  wrote:
> On Tue, Nov 7, 2017 at 2:37 PM, Peter Hutterer  
> wrote:
>> On Tue, Nov 07, 2017 at 11:09:44AM -0800, Jason Gerecke wrote:
>>> BTN_STYLUS3 has been introduced by the Linux 4.15 kernel to report the
>>> status of the third button present on Wacom's new "Pro Pen 3D" stylus.
>>> Treat this button like xf86-input-wacom and send a button 8 event
>>> ("navigate back") when received from Wayland.
>>>
>>> Signed-off-by: Jason Gerecke 
>>> ---
>>> Note: this patch is pending the formal acceptance of BTN_STYLU3 into
>>> Linux 4.15. I'll post an update once Jiri merges the patch.
>>
>> Reviewed-by: Peter Hutterer , I'll merge this
>> after your reminder ;)
>>
>> Cheers,
>>Peter
>>
>>
>
> Jiri's accepted the patch into his 'for-4.15/wacom' branch :)
>
> Jason
> ---
> Now instead of four in the eights place /
> you’ve got three, ‘Cause you added one  /
> (That is to say, eight) to the two, /
> But you can’t take seven from three,/
> So you look at the sixty-fours
>

Just noticed that this doesn't appear to have been merged, so sending
another poke :)

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one  /
(That is to say, eight) to the two, /
But you can’t take seven from three,/
So you look at the sixty-fours

>>>  hw/xwayland/xwayland-input.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
>>> index 68e365051..4325b4c6e 100644
>>> --- a/hw/xwayland/xwayland-input.c
>>> +++ b/hw/xwayland/xwayland-input.c
>>> @@ -1698,6 +1698,7 @@ tablet_tool_button_state(void *data, struct 
>>> zwp_tablet_tool_v2 *tool,
>>>
>>>  case 0x113: /* BTN_SIDE*/
>>>  case 0x116: /* BTN_BACK*/
>>> +case 0x149: /* BTN_STYLUS3 */
>>>  xbtn = 8;
>>>  break;
>>>
>>> --
>>> 2.15.0
>>>
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 3/5] scanner: introduce --object-type option

[Derek Foreman]

On 2018-01-22 09:30 AM, Emil Velikov wrote:

On 22 August 2017 at 14:02, Emil Velikov  wrote:

On 18 August 2017 at 13:05, Pekka Paalanen  wrote:



The exported configuration would then be:
LOCAL_INTERFACE_DECL=extern
EXTERN_INTERFACE_DECL=extern
LOCAL_INTERFACE_DEF=WL_EXPORT

That would be far too flexible and no-one would use it right, right?


I did not introduce separate tokens, since those are (and should be)
used _only_ in the .c file.
Personally then do not seem necessary, If you prefer we can add them though.


Ah, no, that was just a wild idea of something completely different. I
meant that the user project would be setting those macros before using
scanner-generated files, and if unset, the scanner-emitted code would
default to the legacy behaviour. That way there would be no visibility
modes in scanner itself. If it's not obviously better, then nevermind.
It certainly has a lot more room to go wrong than your proposal.



I see.

Personally I'd lean towards with my approach for now since it is
simpler, despite that it provides less flexibility.
As you pointed out the proposal is a bit more fragile, so might be
better to avoid until there's a real need for it.



...


The patch looks pretty much correct to me, if we choose to go this way.


Glad to hear.

I'll let me know once you guys are settled in on the approach, and
I'll respin the series with all the comments addressed.


Cool, let's see if we can get the name conflict issue solved, and then
I'll try to remember to ping you.


Ack, I'll keep an eye open, just in case.


Considering the status of the the name conflict series, should I
re-spin this lot?
I'm more than happy to tweak things - say rename the toggle, etc.


I see there were two series proposed to control symbol visibility, yours 
and Jonas'?


Assuming that once we drop the symbol collision issue they both solve 
the same problems, it would be good if we could focus on one going forward.


Is this the chosen one?

Thanks,
Derek


Thanks
Emil
___
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 xserver] xwayland: avoid a crash with empty window pixmaps

[Daniel Stone]

Hi,

On 24 January 2018 at 16:17, Adam Jackson  wrote:
> On Wed, 2018-01-24 at 11:36 +0100, Olivier Fourdan wrote:
>> So basically, just remove the  “if
>> (RegionNotEmpty(DamageRegion(xwl_window->damage)))” would suffice?
>
> Worth a try anyway. I'm still just guessing at the root cause.

If it is the root cause, it certainly checks out:
  - window was realized, redirected, damaged
  - manual redirect removed (e.g. WM crashes or stops wanting to
present the window), pixmap destroyed (including damage)
  - possible: window is unrealized, but the DamageNotEmpty() check
doesn't pass due to the region being uninit'ed (or, if the window
lives, it's not unrealized)
  - block handler called
  - oops

Deleting the check in unrealize definitely makes sense, but perhaps to
guard against this even happening in the first place, xwl could just
place a manual redirect of its own when the window is realized and
removed when unrealized?

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

[PATCH xserver] xwayland: remove dirty window unconditionally on unrealize

[Olivier Fourdan]

This is a rare occurrence of a crash in Xwayland for which I don't have
the reproducing steps, just a core file.

The backtrace looks as follow:

  #0  raise () from /usr/lib64/libc.so.6
  #1  abort () from /usr/lib64/libc.so.6
  #2  OsAbort () at utils.c:1361
  #3  AbortServer () at log.c:877
  #4  FatalError () at log.c:1015
  #5  OsSigHandler () at osinit.c:154
  #6  
  #7  xwl_glamor_pixmap_get_wl_buffer () at xwayland-glamor.c:162
  #8  xwl_screen_post_damage () at xwayland.c:514
  #9  block_handler () at xwayland.c:665
  #10 BlockHandler () at dixutils.c:388
  #11 WaitForSomething () at WaitFor.c:219
  #12 Dispatch () at dispatch.c:422
  #13 dix_main () at main.c:287

The crash is caused by dereferencing “xwl_pixmap->buffer” in
xwl_glamor_pixmap_get_wl_buffer() because “xwl_pixmap” is NULL.

Reason for this is because the corresponding pixmap is from the root
window and xwayland is rootless by default.

This can happen if the window was mapped, redirected, damaged and
unredirected immediately, before the damage is processed by Xwayland.

Make sure to remove the dirty window from the damage list on unrealize
to prevent this from happening.

Credit goes to Adam Jackson  and Daniel Stone
 for finding the root cause the issue.

Signed-off-by: Olivier Fourdan 
---
 hw/xwayland/xwayland.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index ab7cee545..88d31f80b 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -587,8 +587,7 @@ xwl_unrealize_window(WindowPtr window)
 }
 
 wl_surface_destroy(xwl_window->surface);
-if (RegionNotEmpty(DamageRegion(xwl_window->damage)))
-xorg_list_del(_window->link_damage);
+xorg_list_del(_window->link_damage);
 DamageUnregister(xwl_window->damage);
 DamageDestroy(xwl_window->damage);
 if (xwl_window->frame_callback)
-- 
2.14.3

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

Re: [PATCH v3 xserver 1/2] xwayland: Add optional xdg-output support

[Adam Jackson]

On Wed, 2018-01-24 at 11:31 +0100, Olivier Fourdan wrote:

> Small bump, would need to land this patch for 1.20 so we can continue
> with fixing mutter once this is available in an Xwayland release :)

Thanks for the poke. This didn't quite build with meson, needed to
generate the new client header there too. Fixed that and some comment
typos, and merged:

remote: E: failed to find patch for rev 
da8de2a7f6ab52ef52039b0dc9978260232a34a6.
remote: I: 0 patch(es) updated to state Accepted.
To ssh://git.freedesktop.org/git/xorg/xserver
   75408f53d4..da8de2a7f6  master -> master

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

Re: [PATCH xserver] xwayland: avoid a crash with empty window pixmaps

[Olivier Fourdan]

On Wed, Jan 24, 2018 at 5:17 PM, Adam Jackson  wrote:

> On Wed, 2018-01-24 at 11:36 +0100, Olivier Fourdan wrote:
>
> > So basically, just remove the  “if
> > (RegionNotEmpty(DamageRegion(xwl_window->damage)))” would suffice?
>
> Worth a try anyway. I'm still just guessing at the root cause.
>

Right, problem is I have no idea how to reproduce the issue so I cannot
tell if that would fix it.

But I don't think removing it unconditionally would cause any trouble (and
seems like the right thing to do), so... patch to follow.

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

Re: [PATCH xserver] xwayland: avoid a crash with empty window pixmaps

[Adam Jackson]

On Wed, 2018-01-24 at 11:36 +0100, Olivier Fourdan wrote:

> So basically, just remove the  “if
> (RegionNotEmpty(DamageRegion(xwl_window->damage)))” would suffice?

Worth a try anyway. I'm still just guessing at the root cause.

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

[PATCH wayland] doc: Document behavior of non-nullable object arguments in clients

[Philipp Kerling]

---
 src/wayland-util.h | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/wayland-util.h b/src/wayland-util.h
index caeac82..b6cbe0e 100644
--- a/src/wayland-util.h
+++ b/src/wayland-util.h
@@ -78,12 +78,23 @@ extern "C" {
  * wl_message is to a protocol message like a class is to an object.
  *
  * The `name` of a wl_message is the name of the corresponding protocol 
message.
+ *
  * The `signature` is an ordered list of symbols representing the data types
  * of message arguments and, optionally, a protocol version and indicators for
  * nullability. A leading integer in the `signature` indicates the _since_
  * version of the protocol message. A `?` preceding a data type symbol 
indicates
- * that the following argument type is nullable. When no arguments accompany a
- * message, `signature` is an empty string.
+ * that the following argument type is nullable. While it is a protocol 
violation
+ * to send messages with non-nullable arguments set to `NULL`, event handlers 
in
+ * clients might still get called with non-nullable object arguments set to
+ * `NULL`. This can happen when the client destroyed the object being used as
+ * argument on its side and an event referencing that object was sent before 
the
+ * server knew about its destruction. As this race cannot be prevented, clients
+ * should - as a general rule - program their event handlers such that they can
+ * handle object arguments declared non-nullable being `NULL` gracefully.
+ *
+ * When no arguments accompany a message, `signature` is an empty string.
+ *
+ * Symbols:
  *
  * * `i`: int
  * * `u`: uint
-- 
2.15.1

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

Re: [PATCH v14 23/41] compositor-drm: Use plane_coords_for_view for cursor

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:40 +
Daniel Stone  wrote:

> Use the new helper to populate the cursor state as well, with some
> special-case handling to account for how we always upload a full-size
> BO.
> 
> Signed-off-by: Daniel Stone 
> Reported-by: Derek Foreman 
> ---
>  libweston/compositor-drm.c | 48 
> +++---
>  1 file changed, 24 insertions(+), 24 deletions(-)
> 

Hi,

I like the idea, but there are some details.

> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index d97efd761..dbe53513b 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -2827,10 +2827,8 @@ drm_output_prepare_cursor_view(struct drm_output_state 
> *output_state,
>   struct drm_backend *b = to_drm_backend(output->base.compositor);
>   struct drm_plane *plane = output->cursor_plane;
>   struct drm_plane_state *plane_state;
> - struct weston_buffer_viewport *viewport = >surface->buffer_viewport;
>   struct wl_shm_buffer *shmbuf;
>   bool needs_update = false;
> - float x, y;
>  
>   if (!plane)
>   return NULL;
> @@ -2860,16 +2858,6 @@ drm_output_prepare_cursor_view(struct drm_output_state 
> *output_state,
>   if (wl_shm_buffer_get_format(shmbuf) != WL_SHM_FORMAT_ARGB)
>   return NULL;
>  
> - if (output->base.transform != WL_OUTPUT_TRANSFORM_NORMAL)
> - return NULL;
> - if (ev->transform.enabled &&
> - (ev->transform.matrix.type > WESTON_MATRIX_TRANSFORM_TRANSLATE))
> - return NULL;
> - if (viewport->buffer.scale != output->base.current_scale)
> - return NULL;
> - if (ev->geometry.scissor_enabled)
> - return NULL;
> -
>   if (ev->surface->width > b->cursor_width ||
>   ev->surface->height > b->cursor_height)
>   return NULL;

This check left in is comparing surface units with buffer units, so it
has the same problem as cursor_bo_update() below. I also think it is
somewhat redundant with the src_w/src_h checks below.

> @@ -2880,6 +2868,26 @@ drm_output_prepare_cursor_view(struct drm_output_state 
> *output_state,
>   if (plane_state && plane_state->fb)
>   return NULL;
>  
> + /* We can't scale with the legacy API, and we don't try to account for
> +  * simple cropping/translation in cursor_bo_update. */
> + plane_state->output = output;
> + drm_plane_state_coords_for_view(plane_state, ev);
> + if (plane_state->src_x != 0 || plane_state->src_y != 0 ||
> + plane_state->src_w > (unsigned) b->cursor_width << 16 ||
> + plane_state->src_h > (unsigned) b->cursor_height << 16 ||
> + plane_state->src_w != plane_state->dest_w << 16 ||
> + plane_state->src_h != plane_state->dest_h << 16)
> + goto err;
> +

cursor_bo_update() has this:

for (i = 0; i < ev->surface->height; i++)
memcpy(buf + i * b->cursor_width,
   s + i * stride,
   ev->surface->width * 4);

Using surface->width and surface->height means that it cannot work on
buffer scale > 1. I think we didn't catch that before or after this
patch.

Should drm_output_prepare_cursor_view() not check that src size matches
the size cursor_bo_update() is going to copy?


> + /* The cursor API is somewhat special: in cursor_bo_update(), we upload
> +  * a buffer which is always cursor_width x cursor_height, even if the
> +  * surface we want to promote is actually smaller than this. Manually
> +  * mangle the plane state to deal with this. */
> + plane_state->src_w = b->cursor_width << 16;
> + plane_state->src_h = b->cursor_height << 16;
> + plane_state->dest_w = b->cursor_width;
> + plane_state->dest_h = b->cursor_height;
> +
>   /* Since we're setting plane state up front, we need to work out
>* whether or not we need to upload a new cursor. We can't use the
>* plane damage, since the planes haven't actually been calculated
> @@ -2896,26 +2904,18 @@ drm_output_prepare_cursor_view(struct 
> drm_output_state *output_state,
>   }
>  
>   output->cursor_view = ev;
> - weston_view_to_global_float(ev, 0, 0, , );
> - plane->base.x = x;
> - plane->base.y = y;
>  
>   plane_state->fb =
>   drm_fb_ref(output->gbm_cursor_fb[output->current_cursor]);
> - plane_state->output = output;
> - plane_state->src_x = 0;
> - plane_state->src_y = 0;
> - plane_state->src_w = b->cursor_width << 16;
> - plane_state->src_h = b->cursor_height << 16;
> - plane_state->dest_x = (x - output->base.x) * output->base.current_scale;
> - plane_state->dest_y = (y - output->base.y) * output->base.current_scale;
> - plane_state->dest_w = b->cursor_width;
> - plane_state->dest_h = b->cursor_height;
>  
>   if (needs_update)
>   

Re: [PATCH v14 22/41] compositor-drm: Use plane_state_coords_for_view for scanout

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:39 +
Daniel Stone  wrote:

> Use the shiny new helper we have for working through scanout as well.
> 
> Signed-off-by: Daniel Stone 
> ---
>  libweston/compositor-drm.c | 81 
> ++
>  1 file changed, 25 insertions(+), 56 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 10bc53ba0..d97efd761 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -1575,13 +1575,6 @@ drm_output_assign_state(struct drm_output_state *state,
>   }
>  }
>  
> -static int
> -drm_view_transform_supported(struct weston_view *ev)
> -{
> - return !ev->transform.enabled ||
> - (ev->transform.matrix.type < WESTON_MATRIX_TRANSFORM_ROTATE);
> -}
> -
>  static bool
>  drm_view_is_opaque(struct weston_view *ev)
>  {
> @@ -1613,7 +1606,6 @@ drm_output_prepare_scanout_view(struct drm_output_state 
> *output_state,
>   struct drm_plane *scanout_plane = output->scanout_plane;
>   struct drm_plane_state *state;
>   struct weston_buffer *buffer = ev->surface->buffer_ref.buffer;
> - struct weston_buffer_viewport *viewport = >surface->buffer_viewport;
>   struct gbm_bo *bo;
>  
>   /* Don't import buffers which span multiple outputs. */
> @@ -1629,28 +1621,6 @@ drm_output_prepare_scanout_view(struct 
> drm_output_state *output_state,
>   if (wl_shm_buffer_get(buffer->resource))
>   return NULL;
>  
> - /* Make sure our view is exactly compatible with the output. */
> - if (ev->geometry.x != output->base.x ||
> - ev->geometry.y != output->base.y)
> - return NULL;
> - if (buffer->width != output->base.current_mode->width ||
> - buffer->height != output->base.current_mode->height)
> - return NULL;
> -
> - if (ev->transform.enabled)
> - return NULL;
> - if (ev->geometry.scissor_enabled)
> - return NULL;
> - if (viewport->buffer.transform != output->base.transform)
> - return NULL;
> - if (viewport->buffer.scale != output->base.current_scale)
> - return NULL;
> - if (!drm_view_transform_supported(ev))
> - return NULL;
> -
> - if (ev->alpha != 1.0f)
> - return NULL;
> -
>   state = drm_output_state_get_plane(output_state, scanout_plane);
>   if (state->fb) {
>   /* If there is already a framebuffer on the scanout plane,
> @@ -1660,44 +1630,50 @@ drm_output_prepare_scanout_view(struct 
> drm_output_state *output_state,
>   return NULL;
>   }
>  
> + state->output = output;
> + drm_plane_state_coords_for_view(state, ev);
> +
> + /* The legacy API does not let us perform cropping or scaling. */

Hi Daniel,

should this not be checking the src coordinates against the buffer
dimensions as well? If the buffer is bigger than the mode but clipped
to the mode size, it might pass all the checks here.

Sources of clipping: layer mask, view mask (scissor).

> + if (state->src_x != 0 || state->src_y != 0 ||
> + state->src_w != state->dest_w << 16 ||
> + state->src_h != state->dest_h << 16 ||
> + state->dest_x != 0 || state->dest_y != 0 ||
> + state->dest_w != (unsigned) output->base.current_mode->width ||
> + state->dest_h != (unsigned) output->base.current_mode->height)
> + goto err;
> +
> + if (ev->alpha != 1.0f)
> + goto err;
> +
>   bo = gbm_bo_import(b->gbm, GBM_BO_IMPORT_WL_BUFFER,
>  buffer->resource, GBM_BO_USE_SCANOUT);
>  
>   /* Unable to use the buffer for scanout */
>   if (!bo)
> - return NULL;
> + goto err;
>  
>   state->fb = drm_fb_get_from_bo(bo, b, drm_view_is_opaque(ev),
>  BUFFER_CLIENT);
>   if (!state->fb) {
> - drm_plane_state_put_back(state);
> + /* We need to explicitly destroy the BO. */
>   gbm_bo_destroy(bo);
> - return NULL;
> + goto err;
>   }
>  
>   /* Can't change formats with just a pageflip */
>   if (state->fb->format->format != output->gbm_format) {
>   /* No need to destroy the GBM BO here, as it's now owned
>* by the FB. */
> - drm_plane_state_put_back(state);
> - return NULL;
> + goto err;
>   }
>  
>   drm_fb_set_buffer(state->fb, buffer);
>  
> - state->output = output;
> -
> - state->src_x = 0;
> - state->src_y = 0;
> - state->src_w = state->fb->width << 16;
> - state->src_h = state->fb->height << 16;
> -
> - state->dest_x = 0;
> - state->dest_y = 0;
> - state->dest_w = output->base.current_mode->width;
> - state->dest_h = output->base.current_mode->height;
> -
>   return _plane->base;
> +
> +err:
> + 

Re: [PATCH xserver] xwayland: avoid a crash with empty window pixmaps

[Olivier Fourdan]

Hi Adam,

On Tue, Jan 23, 2018 at 6:41 PM, Adam Jackson  wrote:

>
> Map / draw / unmap without hitting BlockHandler? I think
> xwl_unrealize_window() might be broken for that case:
>
> /* ... */
> wl_surface_destroy(xwl_window->surface);
> if (RegionNotEmpty(DamageRegion(xwl_window->damage)))
> xorg_list_del(_window->link_damage);
> DamageUnregister(xwl_window->damage);
> DamageDestroy(xwl_window->damage);
> /* ... */
>
> If (for whatever reason) the damage region wasn't empty, we'd never
>

You mean “was empty” here, right, or do I misunderstand?

(we unlink if RegionNotEmpty() so we don't unlink if the region is empty)


> unlink this window from the dirty list. Should probably just unlink it
> unconditionally. If this is indeed what's happening, then the window
> being updated in xwl_window_post_damage() would have ->mapped = 0, and
> would be not the root window itself.
>

So basically, just remove the  “if
(RegionNotEmpty(DamageRegion(xwl_window->damage)))”
would suffice?

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

Re: [PATCH v3 xserver 1/2] xwayland: Add optional xdg-output support

[Olivier Fourdan]

Hi,

On Mon, Dec 18, 2017 at 8:52 AM, Olivier Fourdan 
wrote:

> Hi,
>
> xdg-support has now been added to mutter (in git master), can we consider
> this patch (only this one for now) which adds xdg-output support to
> Xwayland?
>
> Because of the difference between compositors, we cannot all patches at
> once, that would break either weston or mutter/gnome-shell.
>
> So we need to proceed as follow:
>
>  1. Land xdg-output support in mutter ← we're here
>  https://bugzilla.gnome.org/attachment.cgi?id=365582
>  2. Land xdg-output support in wayland
>  https://patchwork.freedesktop.org/patch/175575/
>
> Then, once we have a release of both Xwayland and mutter supporting
> xdg-output:
>
>  3. Land the patc hto send the correct output size and position in mutter
>  https://bugzilla.gnome.org/attachment.cgi?id=365575
>  4. Land the second patch of this series, to apply output scale to monitor
> resolution
>  https://patchwork.freedesktop.org/patch/175578/
>
> This way, in both weston and mutter/gnome-shell, a mode X×Y at scale 2
> will still show up as X×Y in xrandr.
>
> Cheers,
> Olivier
>


Small bump, would need to land this patch for 1.20 so we can continue with
fixing mutter once this is available in an Xwayland release :)

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

Re: [PATCH v14 21/41] [XXX] compositor-drm: Only disallow scaling for overlay planes

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:38 +
Daniel Stone  wrote:

> Now we have a more comprehensive overview of the transform we're going
> to apply, use this to explicitly disallow scaling and rotation.
> 
> XXX: This does not actually disallow rotation for square surfaces.
>  We would have to build up a full buffer->view->output transform
>  matrix, and then analyse that.

Hi,

I think the most important bit is missing in the commit message: why?
What's wrong with the old checks?

Is it only the wp_viewport state or something more?

Maybe we could use this patch and add a helper that explicitly checks
all the little bits of transformations to ensure there is no rotation?
It would not have to allow all cases, e.g. if the transformation matrix
inverts the buffer/output transform rotation we could just ignore and
reject that case. The helper would essentially be:

bool
supported()
{
/* XXX: This is safe, but still rejects some valid configurations */
if (viewport->buffer.transform != output->base.transform)
return false;
if (!drm_view_transform_supported(ev))
return false;
return true;
}

wp_viewport cannot produce a rotation, so there is no need to check it.

That might allow to keep this and the following patches that unify the
transformation handling between primary/overlay/cursor planes.

It might even pay off to integrate this check into
drm_plane_state_coords_for_view(), adding a success/reject return
value. That function does assume that there is no end-to-end rotation
at all. It won't work for arbitrarily rotated views, and for 90-degree
step rotations it may produce negative width and/or height to imply a
flip. I think it should explicitly fail rather than produce garbage
when the preconditions are violated.


Thanks,
pq

> Signed-off-by: Daniel Stone 
> ---
>  libweston/compositor-drm.c | 27 ---
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 001eb5278..10bc53ba0 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -2684,7 +2684,6 @@ drm_output_prepare_overlay_view(struct drm_output_state 
> *output_state,
>  {
>   struct drm_output *output = output_state->output;
>   struct weston_compositor *ec = output->base.compositor;
> - struct weston_buffer_viewport *viewport = >surface->buffer_viewport;
>   struct drm_backend *b = to_drm_backend(ec);
>   struct wl_resource *buffer_resource;
>   struct drm_plane *p;
> @@ -2710,13 +2709,6 @@ drm_output_prepare_overlay_view(struct 
> drm_output_state *output_state,
>   if (wl_shm_buffer_get(buffer_resource))
>   return NULL;
>  
> - if (viewport->buffer.transform != output->base.transform)
> - return NULL;
> - if (viewport->buffer.scale != output->base.current_scale)
> - return NULL;
> - if (!drm_view_transform_supported(ev))
> - return NULL;
> -
>   if (ev->alpha != 1.0f)
>   return NULL;
>  
> @@ -2740,6 +2732,12 @@ drm_output_prepare_overlay_view(struct 
> drm_output_state *output_state,
>   if (!state)
>   return NULL;
>  
> + state->output = output;
> + drm_plane_state_coords_for_view(state, ev);
> + if (state->src_w != state->dest_w << 16 ||
> + state->src_h != state->dest_h << 16)
> + goto err;
> +
>   if ((dmabuf = linux_dmabuf_buffer_get(buffer_resource))) {
>  #ifdef HAVE_GBM_FD_IMPORT
>   /* XXX: TODO:
> @@ -2769,7 +2767,7 @@ drm_output_prepare_overlay_view(struct drm_output_state 
> *output_state,
>   if (dmabuf->attributes.n_planes != 1 ||
>  dmabuf->attributes.offset[0] != 0 ||
>   dmabuf->attributes.flags)
> - return NULL;
> + goto err;
>  
>   bo = gbm_bo_import(b->gbm, GBM_BO_IMPORT_FD, _dmabuf,
>  GBM_BO_USE_SCANOUT);
> @@ -2785,8 +2783,12 @@ drm_output_prepare_overlay_view(struct 
> drm_output_state *output_state,
>  
>   state->fb = drm_fb_get_from_bo(bo, b, drm_view_is_opaque(ev),
>  BUFFER_CLIENT);
> - if (!state->fb)
> + if (!state->fb) {
> + /* Destroy the BO as we've allocated it, but it won't yet
> +  * be deallocated by the state. */
> + gbm_bo_destroy(bo);
>   goto err;
> + }
>  
>   /* Check whether the format is supported */
>   for (i = 0; i < p->count_formats; i++)
> @@ -2797,15 +2799,10 @@ drm_output_prepare_overlay_view(struct 
> drm_output_state *output_state,
>  
>   drm_fb_set_buffer(state->fb, ev->surface->buffer_ref.buffer);
>  
> - state->output = output;
> - drm_plane_state_coords_for_view(state, ev);
> -
>   return >base;
>  
>  err:
>   

Re: libinput touchscreen maxTouchPoints

[Johannes Pointner]

On Wed, Jan 24, 2018 at 10:18 AM, Peter Hutterer
 wrote:
> On Wed, Jan 24, 2018 at 10:04:52AM +0100, Johannes Pointner wrote:
>> Hello,
>>
>> I have noticed that since we are using xf86-input-libinput all our
>> touchscreens (resistive and pcap) reporting a value of 15 for
>> maxTouchPoints.
>> After further looking into this I saw that xf86-input-libinput sets
>> this value fix to 15. I also checked if it is possible to get this
>> info from libinput but I wasn't able to find a possibility.
>>
>> Is this intentionally?
>
> mostly, it's largely a matter of not exporting information that doesn't need
> to be exported. at least with touchpads libinput does a few tricks and
> changes the number of touches available (not yet at runtime, but who knows)
> - once we start exporting this we're a lot more limited in what we can do
> internally to work around broken devices.
>
> Main question is though: what do you need this value for?
We use this value which is available through the browser
(https://developer.mozilla.org/en-US/docs/Web/API/Navigator/maxTouchPoints)
to determine if it is a multitouch device or not.
The same goes for Qt
(http://doc.qt.io/qt-5/qtouchdevice.html#maximumTouchPoints) which has
a libinput backend.
>
> Cheers,
>Peter
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: libinput touchscreen maxTouchPoints

[Peter Hutterer]

On Wed, Jan 24, 2018 at 10:04:52AM +0100, Johannes Pointner wrote:
> Hello,
> 
> I have noticed that since we are using xf86-input-libinput all our
> touchscreens (resistive and pcap) reporting a value of 15 for
> maxTouchPoints.
> After further looking into this I saw that xf86-input-libinput sets
> this value fix to 15. I also checked if it is possible to get this
> info from libinput but I wasn't able to find a possibility.
> 
> Is this intentionally?

mostly, it's largely a matter of not exporting information that doesn't need
to be exported. at least with touchpads libinput does a few tricks and
changes the number of touches available (not yet at runtime, but who knows)
- once we start exporting this we're a lot more limited in what we can do
internally to work around broken devices.

Main question is though: what do you need this value for?

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

libinput touchscreen maxTouchPoints

[Johannes Pointner]

Hello,

I have noticed that since we are using xf86-input-libinput all our
touchscreens (resistive and pcap) reporting a value of 15 for
maxTouchPoints.
After further looking into this I saw that xf86-input-libinput sets
this value fix to 15. I also checked if it is possible to get this
info from libinput but I wasn't able to find a possibility.

Is this intentionally?

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

Re: [PATCH v14 20/41] compositor-drm: Fully account for buffer transformation

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:37 +
Daniel Stone  wrote:

> In our new and improved helper to determine the src/dest values for a
> buffer on a given plane, make sure we account for all buffer
> transformations, including viewport clipping.
> 
> Rather than badly open-coding it ourselves, just use the helper which
> does exactly this.
> 
> Signed-off-by: Daniel Stone 
> Reported-by: Tiago Gomes 
> ---
>  libweston/compositor-drm.c | 55 
> ++
>  1 file changed, 12 insertions(+), 43 deletions(-)
> 

Hi,

the idea here is good, it looks like wp_viewport settings were
completely missed in drm_output_prepare_overlay_view() so they would
get through. This will handle them.

I believe ignoring scissor is ok in drm_output_prepare_overlay_view(),
because the boundingbox already includes its effect, assuming the
boundingbox was exact for the cases we pass through.

> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 71bf94edd..001eb5278 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -1188,10 +1188,9 @@ drm_plane_state_coords_for_view(struct drm_plane_state 
> *state,
>   struct weston_view *ev)
>  {
>   struct drm_output *output = state->output;
> - struct weston_buffer_viewport *viewport = >surface->buffer_viewport;
>   pixman_region32_t dest_rect, src_rect;
>   pixman_box32_t *box, tbox;
> - wl_fixed_t sx1, sy1, sx2, sy2;
> + float sxf1, syf1, sxf2, syf2;
>  
>   /* Update the base weston_plane co-ordinates. */
>   box = pixman_region32_extents(>transform.boundingbox);
> @@ -1217,52 +1216,22 @@ drm_plane_state_coords_for_view(struct 
> drm_plane_state *state,
>   state->dest_h = tbox.y2 - tbox.y1;
>   pixman_region32_fini(_rect);
>  
> - /* Now calculate the source rectangle, by finding the points in the
> -  * view and working backwards to source co-ordinates. */
> + /* Now calculate the source rectangle, by finding the extents of the
> +  * view, and working backwards to source co-ordinates. */
>   pixman_region32_init(_rect);
>   pixman_region32_intersect(_rect, >transform.boundingbox,
> >base.region);
>   box = pixman_region32_extents(_rect);
> -
> - /* Accounting for any transformations made to this particular surface
> -  * view, find the source rectangle to use. */
> - weston_view_from_global_fixed(ev,
> -   wl_fixed_from_int(box->x1),
> -   wl_fixed_from_int(box->y1),
> -   , );
> - weston_view_from_global_fixed(ev,
> -   wl_fixed_from_int(box->x2),
> -   wl_fixed_from_int(box->y2),
> -   , );
> -
> - /* XXX: How is this possible ... ? */
> - if (sx1 < 0)
> - sx1 = 0;
> - if (sy1 < 0)
> - sy1 = 0;
> - if (sx2 > wl_fixed_from_int(ev->surface->width))
> - sx2 = wl_fixed_from_int(ev->surface->width);
> - if (sy2 > wl_fixed_from_int(ev->surface->height))
> - sy2 = wl_fixed_from_int(ev->surface->height);

(I think these checks should have actually been after the
weston_transformed_rect() call.)

> -
> - tbox.x1 = sx1;
> - tbox.y1 = sy1;
> - tbox.x2 = sx2;
> - tbox.y2 = sy2;
> -
> - /* Apply viewport transforms in reverse, to get the source co-ordinates
> -  * in buffer space. */
> - tbox = weston_transformed_rect(wl_fixed_from_int(ev->surface->width),
> -wl_fixed_from_int(ev->surface->height),
> -viewport->buffer.transform,
> -viewport->buffer.scale,
> -tbox);
> -
> - state->src_x = tbox.x1 << 8;
> - state->src_y = tbox.y1 << 8;
> - state->src_w = (tbox.x2 - tbox.x1) << 8;
> - state->src_h = (tbox.y2 - tbox.y1) << 8;
> + weston_view_from_global_float(ev, box->x1, box->y1, , );
> + weston_surface_to_buffer_float(ev->surface, sxf1, syf1, , );
> + weston_view_from_global_float(ev, box->x2, box->y2, , );
> + weston_surface_to_buffer_float(ev->surface, sxf2, syf2, , );
>   pixman_region32_fini(_rect);
> +
> + state->src_x = wl_fixed_from_double(sxf1) << 8;
> + state->src_y = wl_fixed_from_double(syf1) << 8;
> + state->src_w = wl_fixed_from_double(sxf2 - sxf1) << 8;
> + state->src_h = wl_fixed_from_double(syf2 - syf1) << 8;

I would assume that KMS would not be happy about negative or zero width
or height, or negative x or y, or x+w or y+h going over the buffer
dimensions. Should we check those?

This is based on the boundingbox again being the smallest possible but
still in global integer coordinates fully containing the