Re: wl_list_remove(>link) point to invalid address

[zou lan]

Hi Pekka

Thank you for your comment. They are very helpful.

>that's a good find. If I understand correctly, we do not handle the
>case where a weston_view is effectively unmapped by removing it from a
>weston_layer. As weston_compositor::view_list is never torn down
>properly but simply rebuilt, the unmapped view is left with dangling
>pointers, and those will be hit when finally destroying the view.

Your explanation is exactly correct.

I have verified the optional 1 can't work.

optional 2
>> weston_view_damage_below(shsurf->fullscreen.black_view);
>> +
>>  wl_list_remove(>fullscreen.black_view->link);

> Removing without init might cause issues with a potential double-remove
>on e.g. view destruction.

yes.  I add the initialization for the list.

+ wl_list_init (>fullscreen.black_view->link);

I agree with you weston_view_unmap maybe better for optional 2.  Since I
find crashes  caused by invalid pointer, I only concern how to make the
view.link to point  the right address.

No matter what functions use here, this only handle the black views.
Are there other views have the same problems?  Especially in ivi shell, I
find the crash exist in many  scenarios.

How would weston to resolve the potential risks? Thank you.

Best Regards
Nancy

2018-04-03 20:28 GMT+08:00 Pekka Paalanen :

> On Thu, 29 Mar 2018 11:01:47 +0800
> zou lan  wrote:
>
> > Hi Pekka & all
> >
> > When I run some full screen applications under desktop shell,  then kill
> > some applications randomly, I find
> > crashes maybe happened sometimes.
> >
> > Crash call stack:
> > weston_view_destroy-->weston_view_unmap-->wl_list_reomve(>link).
> > The view is black view.
>
> Hi,
>
> that's a good find. If I understand correctly, we do not handle the
> case where a weston_view is effectively unmapped by removing it from a
> weston_layer. As weston_compositor::view_list is never torn down
> properly but simply rebuilt, the unmapped view is left with dangling
> pointers, and those will be hit when finally destroying the view.
>
> Would that be an accurate explanation?
>
> > Then I debug code, find that shell.c  lower_fullscreen_layer function
> only
> > delete the layer of black view from layer_list, but not handle the
> > 
> > lower_fullscreen_layer()
> > {
> > .
> > /* Hide the black view */
> >
> > weston_layer_entry_remove(>fullscreen.black_view->layer_link);
> >
> > wl_list_init(>fullscreen.black_view->layer_link.link);
>
> This wl_list_init() seems to be redundant, btw.
>
> >
> > weston_view_damage_below(shsurf->fullscreen.black_view);
>
> Could this path simply call weston_view_unmap() to unmap the black_view?
>
> It effectively is unmapping the view, but forgetting to clear the
> 'is_mapped' boolean, which later leads to accessing weston_view::link
> dangling pointers. weston_view_unmap() does a lot more which might be
> necessary as well.
>
> >
> > }
> >
> > The code will be called when start second full screen application.
> >
> > when weston build view list again, the first surface's black view is not
> in
> > the view list, the list  maybe point some invalid address.
> >
> > I think ivi shell will have the same problems too.  I have a temporary
> > patch to resolve this problem. I want to discuss it in the community.
> >
> > option 1
> > --- a/libweston/compositor.c
> > +++ b/libweston/compositor.c
> > @@ -2195,7 +2195,7 @@ view_list_add(struct weston_compositor *compositor,
> > struct weston_subsurface *sub;
> >
> > weston_view_update_transform(view);
> > -
> > +   wl_list_remove();
>
> For better or worse, the weston_view::link is deliberately left with
> garbage pointers when the weston_compositor::view_list is discarded for
> a rebuild.
>
> Wouldn't this addition cause weston_compositor::view_list to become
> corrupted if the link removed here happened to be the first or the last
> in the old list order? Or maybe be get lucky most times, and it would
> need quite special circumstances to trigger the corruption.
>
> I still think this approach is not good.
>
> >
> > option 2:
> > --- a/desktop-shell/shell.c
> > +++ b/desktop-shell/shell.c
> > @@ -3651,6 +3651,7 @@ lower_fullscreen_layer(struct desktop_shell *shell,
> >
> > weston_layer_entry_remove(>fullscreen.black_view->layer_link);
> >
> > wl_list_init(>fullscreen.black_view->layer_link.link);
> >
> > weston_view_damage_below(shsurf->fullscreen.black_view);
> > +
> >  wl_list_remove(>fullscreen.black_view->link);
>
> Removing without init might cause issues with a potential double-remove
> on e.g. view destruction.
>
> >
> > }
> >
> > Do you think these options reasonable? Thank you.
>
> I think using weston_view_unmap() would be the best as I mentioned
> above, unless there is some specific reason why it cannot be used. None
> come to my mind this time.
>
>
> Thanks,
> pq
>
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org

Re: [PATCH weston 00/25] A new touchscreen calibrator

[Peter Hutterer]

On Tue, Apr 03, 2018 at 12:20:08PM +0300, Pekka Paalanen wrote:
> On Mon, 02 Apr 2018 13:11:24 +
> Matt Hoosier  wrote:
> 
> > Do you happen to know of a good readymade program that fakes the presence
> > of a touch input device with uinput? I'd love to test this series, but
> > current Weston is far ahead of what my embedded devices will do; so I'm in
> > the position of mostly relying on the desktop for testing.
> 
> Hi Matt,
> 
> sorry, I have no knowledge of such tool, and the lack of it has
> prevented me from testing things before as well.
> 
> Right now I have two borrowed touchscreens to work with, but I need to
> send them back one day.


$ cat /etc/udev/rules.d/99-touchpads-are-touchscreens.rules
ACTION!="add|change", GOTO="_end"
ENV{ID_INPUT_TOUCHPAD}=="1", ENV{ID_INPUT_TOUCHPAD}="", 
ENV{ID_INPUT_TOUCHSCREEN}="1"
LABEL="_end"

This turns your touchpad into a touchscreen, should be good enough for
testing. Afterwards, reboot or run

sudo udevadm test /sys/class/input/event17
sudo udevadm test /sys/class/input/event17/device

with your event node. Note that you need both the event node and the parent
input device to avoid both flags being set.

Cheers,
   Peter

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

Re: [PATCH weston 00/25] A new touchscreen calibrator

[Matt Hoosier]

On Tue, Apr 3, 2018, 08:10 Matt Hoosier  wrote:

> On Tue, Apr 3, 2018 at 5:35 AM, Philipp Kerling 
> wrote:
>
>> > Do you happen to know of a good readymade program that fakes the
>> > presence of a touch input device with uinput? I'd love to test this
>> > series, but current Weston is far ahead of what my embedded devices
>> > will do; so I'm in the position of mostly relying on the desktop for
>> > testing.
>> I'm not sure whether it fits your use case, but you can give mtemu a
>> spin.
>>
> https://gitlab.com/shul/mtemu
>
>
> Thanks. That looks like a really useful program. As it turns out, I ended
> up being able to find some real touch hardware to use. It was ancient stuff
> that required writing a uinput program to translate the old ABS_X/Y-style
> input into modern mtdev representation, but it eventually worked.
>
> The calibration functionality in Pekka's patch series seems to work for
> me. I didn't test out any of the mechanism to spawn an auxiliary program
> that saves the results to disk, but the hot-installed copy of the
> calibration worked.
>
> I did have a little trouble understanding how I can pick-and-choose from
> the client side which touch device should be associated with which output.
> The new weston-touch-calibrator program just prints out a flat list of
> input devices and head names, but doesn't seem to let you do permutations
> of that.
>
> So I ended up having to use a 'mode=off' directive in weston.ini to
> temporarily turn off the main display of my laptop so that the touchscreen
> got pegged to the correct physical output. I have the feeling that I just
> overlooked some aspect of configuration about this.
>

... which is of course the statically set UDev properties that associate an
input device with the Wayland output. Nevermind about this question.


> -Matt
>
>
>>
>>
>>
>> > On Fri, Mar 23, 2018, 09:38 Matt Hoosier 
>> > wrote:
>> > > On Fri, Mar 23, 2018 at 9:30 AM, Pekka Paalanen > > > m> wrote:
>> > > > On Fri, 23 Mar 2018 08:46:46 -0500
>> > > > Matt Hoosier  wrote:
>> > > >
>> > > >> I am very much in favor of the overall approach on this patch
>> > > series.
>> > > >> I've experienced every single one of the problems described in
>> > > this
>> > > >> summary, and my company currently resorts to maintaining a hacky
>> > > >> out-of-tree calibration tool to paper over these problems.
>> > > >
>> > > > Hi Matt,
>> > > >
>> > > > that is very heartwarming to hear. Is your tool specifically for
>> > > Weston
>> > > > too?
>> > >
>> > > Yes and no. It's not phrased as a patch against the Weston source
>> > > code, but it uses heuristics for determining which output the raw
>> > > /dev/input/* events should be correlated against, and those
>> > > heuristics
>> > > probably would fail if some different compositor happened to be
>> > > running.
>> > >
>> > > >
>> > > > I would be very happy if this proposal fits your needs, and
>> > > certainly
>> > > > interested in hearing where it falls short.
>> > > >
>> > > >
>> > > > Thanks,
>> > > > pq
>> > > >
>> > > >> On Fri, Mar 23, 2018 at 7:00 AM, Pekka Paalanen > > > .com> wrote:
>> > > >> > From: Pekka Paalanen 
>> > > >> >
>> > > >> > Hi all,
>> > > >> >
>> > > >> > the existing touchscreen calibrator in Weston has several
>> > > problems. This
>> > > >> > proposal intends to solve them all by introducing a new
>> > > protocol
>> > > >> > extension for touchscreen calibration and a new calibrator
>> > > tool.
>> > > >> >
>> > > >> > The benefits of the new tool, which the old tool lacks, are:
>> > > >> >
>> > > >> > - You can unambiguously pick a physical touch device to
>> > > calibrate.
>> > > >> >
>> > > >> > - You can be sure your touch events come only from that
>> > > particular
>> > > >> >   device, and that you cannot miss touch events even if the
>> > > current
>> > > >> >   calibration is horribly wrong.
>> > > >> >
>> > > >> > - You can be sure the calibration window (pattern) is shown on
>> > > the right
>> > > >> >   output with the right coordinates.
>> > > >> >
>> > > >> > - You can unambiguously calibrate even multiple touchscreens
>> > > that are
>> > > >> >   all cloned (showing the same image).
>> > > >> >
>> > > >> > - You get a libinput style calibation matrix instead of the
>> > > >> >   WL_CALIBRATION format which depends on output resolution.
>> > > >> >
>> > > >> > - You can load a new calibration into the compositor without
>> > > playing
>> > > >> >   tricks with udev or restarting the compositor.
>> > > >> >
>> > > >> > There is more discussion about the topic at:
>> > > >> > https://phabricator.freedesktop.org/T7868
>> > > >> >
>> > > >> > This patch series depends on the clone mode series:
>> > > >> > https://patchwork.freedesktop.org/series/32898/
>> > > >> >
>> > > >> > There is a full branch available at:
>> > > >> > 

[PATCH] terminal: Fix unintended fallthrough to cursor restore

[Daniel Stone]

ef57a9b788 added support for window operations such as reporting the
title in escape mode. It implemented this by which-window-op case,
inside the existing which-escape-code case. Whilst it would break out of
the former window-op case, it never broke out of the latter escape-code
case. This would lead to window ops (such as reporting title) falling
through to restoring the saved cursor position.

This doesn't seem at all right, and also fixes a warning with GCC 8.

Signed-off-by: Daniel Stone 
---
 clients/terminal.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/clients/terminal.c b/clients/terminal.c
index 16a449540..f792badcd 100644
--- a/clients/terminal.c
+++ b/clients/terminal.c
@@ -1686,6 +1686,7 @@ handle_escape(struct terminal *terminal)
fprintf(stderr, "Unimplemented windowOp %d\n", 
args[0]);
break;
}
+   break;
case 'u':/* Restore cursor location */
terminal->row = terminal->saved_row;
terminal->column = terminal->saved_column;
-- 
2.17.0.rc2

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

Re: [PATCH 3/3] rdp-backend.so: OpenGL hardware acceleration

[Hardening]

Le 06/07/2017 à 12:06, Olivier Blin a écrit :
> From: DRC 
> 
> ---
>  compositor/main.c  |  23 +++-
>  configure.ac   |   4 +-
>  libweston/compositor-rdp.c | 314 
> +++--
>  libweston/compositor-rdp.h |  24 
>  4 files changed, 352 insertions(+), 13 deletions(-)
> 
> diff --git a/compositor/main.c b/compositor/main.c
> index f8a60e97..125cc0f8 100644
> --- a/compositor/main.c
> +++ b/compositor/main.c
> @@ -601,6 +601,7 @@ usage(int error_code)
>   "  --rdp4-key=FILE\tThe file containing the key for RDP4 
> encryption\n"
>   "  --rdp-tls-cert=FILE\tThe file containing the certificate for 
> TLS encryption\n"
>   "  --rdp-tls-key=FILE\tThe file containing the private key for 
> TLS encryption\n"
> + "  --use-pixman\t\tUse the pixman (CPU) renderer\n"
>   "\n");
>  #endif
>  
> @@ -1331,11 +1332,14 @@ static void
>  rdp_backend_output_configure(struct wl_listener *listener, void *data)
>  {
>   struct weston_output *output = data;
> + struct weston_config *wc = wet_get_config(output->compositor);
>   struct wet_compositor *compositor = 
> to_wet_compositor(output->compositor);
>   struct wet_output_config *parsed_options = compositor->parsed_options;
> + struct weston_config_section *section;
>   const struct weston_rdp_output_api *api = 
> weston_rdp_output_get_api(output->compositor);
>   int width = 640;
>   int height = 480;
> + char *gbm_format = NULL;
>  
>   assert(parsed_options);
>  
> @@ -1344,6 +1348,8 @@ rdp_backend_output_configure(struct wl_listener 
> *listener, void *data)
>   return;
>   }
>  
> + section = weston_config_get_section(wc, "output", "name", output->name);
> +
>   if (parsed_options->width)
>   width = parsed_options->width;
>  
> @@ -1353,6 +1359,12 @@ rdp_backend_output_configure(struct wl_listener 
> *listener, void *data)
>   weston_output_set_scale(output, 1);
>   weston_output_set_transform(output, WL_OUTPUT_TRANSFORM_NORMAL);
>  
> + weston_config_section_get_string(section,
> +  "gbm-format", _format, NULL);
> +
> + api->set_gbm_format(output, gbm_format);
> + free(gbm_format);
> +
>   if (api->output_set_size(output, width, height) < 0) {
>   weston_log("Cannot configure output \"%s\" using 
> weston_rdp_output_api.\n",
>  output->name);
> @@ -1375,6 +1387,7 @@ weston_rdp_backend_config_init(struct 
> weston_rdp_backend_config *config)
>   config->server_key = NULL;
>   config->env_socket = 0;
>   config->no_clients_resize = 0;
> + config->use_pixman = false;
>  }
>  
>  static int
> @@ -1382,6 +1395,7 @@ load_rdp_backend(struct weston_compositor *c,
>   int *argc, char *argv[], struct weston_config *wc)
>  {
>   struct weston_rdp_backend_config config  = {{ 0, }};
> + struct weston_config_section *section;
>   int ret = 0;
>  
>   struct wet_output_config *parsed_options = wet_init_parsed_options(c);
> @@ -1399,11 +1413,17 @@ load_rdp_backend(struct weston_compositor *c,
>   { WESTON_OPTION_BOOLEAN, "no-clients-resize", 0, 
> _clients_resize },
>   { WESTON_OPTION_STRING,  "rdp4-key", 0, _key },
>   { WESTON_OPTION_STRING,  "rdp-tls-cert", 0, _cert 
> },
> - { WESTON_OPTION_STRING,  "rdp-tls-key", 0, _key }
> + { WESTON_OPTION_STRING,  "rdp-tls-key", 0, _key },
> + { WESTON_OPTION_BOOLEAN, "use-pixman", 0, _pixman }
>   };
>  
>   parse_options(rdp_options, ARRAY_LENGTH(rdp_options), argc, argv);
>  
> + section = weston_config_get_section(wc, "core", NULL, NULL);
> + weston_config_section_get_string(section,
> +  "gbm-format", _format,
> +  NULL);
> +
>   ret = weston_compositor_load_backend(c, WESTON_BACKEND_RDP,
>);
>  
> @@ -1417,6 +1437,7 @@ out:
>   free(config.rdp_key);
>   free(config.server_cert);
>   free(config.server_key);
> + free(config.gbm_format);
>  
>   return ret;
>  }
> diff --git a/configure.ac b/configure.ac
> index 53faee34..a7b2d517 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -250,9 +250,9 @@ AM_CONDITIONAL([ENABLE_RDP_COMPOSITOR],
> [test x$enable_rdp_compositor = xyes])
>  if test x$enable_rdp_compositor = xyes; then
>AC_DEFINE([BUILD_RDP_COMPOSITOR], [1], [Build the RDP compositor])
> -  PKG_CHECK_MODULES(RDP_COMPOSITOR, [freerdp2 >= 2.0.0],
> +  PKG_CHECK_MODULES(RDP_COMPOSITOR, [freerdp2 >= 2.0.0 libudev >= 136 gbm],
>  [],
> -[PKG_CHECK_MODULES(RDP_COMPOSITOR, [freerdp >= 1.1.0],[])]
> +[PKG_CHECK_MODULES(RDP_COMPOSITOR, [freerdp >= 1.1.0 libudev >= 136 
> gbm],[])]
>)
>  
>SAVED_CPPFLAGS="$CPPFLAGS"
> 

Re: [PATCH 2/3] gl-renderer: read PIXMAN_x8r8g8b8 as GL_BGRA_EXT

[Hardening]

Le 06/07/2017 à 12:06, Olivier Blin a écrit :
> This is needed by the RDP backend, which uses PIXMAN_x8r8g8b8 for its
> shadow buffers.
> ---
>  libweston/gl-renderer.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libweston/gl-renderer.c b/libweston/gl-renderer.c
> index da29b072..b8b120a1 100644
> --- a/libweston/gl-renderer.c
> +++ b/libweston/gl-renderer.c
> @@ -1213,6 +1213,7 @@ gl_renderer_read_pixels(struct weston_output *output,
>  
>   switch (format) {
>   case PIXMAN_a8r8g8b8:
> + case PIXMAN_x8r8g8b8:
>   gl_format = GL_BGRA_EXT;
>   break;
>   case PIXMAN_a8b8g8r8:
> 

Reviewed-by: David Fort 

-- 
David FORT
website: http://www.hardening-consulting.com/

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

Re: [PATCH 3/3] rdp-backend.so: OpenGL hardware acceleration

[Pekka Paalanen]

On Fri, 23 Feb 2018 16:08:00 +0100
Raimundo Sagarzazu  wrote:

> Hi,
>  
> Two patches that improve performance in my case. 
>  
> First, supporting ARGB gbm-format for the output allows a much better 
> performance of the intelReadPixels function of the i965 driver of Mesa, which 
> is my case:
>  
> diff -rup a/libweston/compositor-rdp.c b/libweston/compositor-rdp.c
> --- a/libweston/compositor-rdp.c   2018-02-22 11:35:14.0 +0100
> +++ b/libweston/compositor-rdp.c2018-02-22 11:37:20.312159332 +0100
> @@ -592,6 +592,8 @@ parse_gbm_format(const char *s, uint32_t
>   *gbm_format = default_value;
>else if (strcmp(s, "xrgb") == 0)
>   *gbm_format = GBM_FORMAT_XRGB;
> + else if (strcmp(s, "argb") == 0)
> + *gbm_format = GBM_FORMAT_ARGB;
>else if (strcmp(s, "rgb565") == 0)
>   *gbm_format = GBM_FORMAT_RGB565;
>else if (strcmp(s, "xrgb2101010") == 0)
>  
> Second, reading just the damaged pixels and y-flipping back the image:

Hi,

thanks for the patches, the ideas look good to me, but the patches
themselves are not formatted such that they could be applied. Please see
https://cgit.freedesktop.org/wayland/wayland/tree/doc/Contributing
for some guidance.

I would expect a re-send of these patches. The OpenGL support in
RDP-backend has not been merged yet, so it would be good to point to
the depended-on patches in your cover letter or even offer a branch
from which people could test.


Thanks,
pq


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

Re: [PATCH weston 00/25] A new touchscreen calibrator

[Matt Hoosier]

On Tue, Apr 3, 2018 at 5:35 AM, Philipp Kerling  wrote:

> > Do you happen to know of a good readymade program that fakes the
> > presence of a touch input device with uinput? I'd love to test this
> > series, but current Weston is far ahead of what my embedded devices
> > will do; so I'm in the position of mostly relying on the desktop for
> > testing.
> I'm not sure whether it fits your use case, but you can give mtemu a
> spin.
>
https://gitlab.com/shul/mtemu


Thanks. That looks like a really useful program. As it turns out, I ended
up being able to find some real touch hardware to use. It was ancient stuff
that required writing a uinput program to translate the old ABS_X/Y-style
input into modern mtdev representation, but it eventually worked.

The calibration functionality in Pekka's patch series seems to work for me.
I didn't test out any of the mechanism to spawn an auxiliary program that
saves the results to disk, but the hot-installed copy of the calibration
worked.

I did have a little trouble understanding how I can pick-and-choose from
the client side which touch device should be associated with which output.
The new weston-touch-calibrator program just prints out a flat list of
input devices and head names, but doesn't seem to let you do permutations
of that.

So I ended up having to use a 'mode=off' directive in weston.ini to
temporarily turn off the main display of my laptop so that the touchscreen
got pegged to the correct physical output. I have the feeling that I just
overlooked some aspect of configuration about this.

-Matt


>
>
>
> > On Fri, Mar 23, 2018, 09:38 Matt Hoosier 
> > wrote:
> > > On Fri, Mar 23, 2018 at 9:30 AM, Pekka Paalanen  > > m> wrote:
> > > > On Fri, 23 Mar 2018 08:46:46 -0500
> > > > Matt Hoosier  wrote:
> > > >
> > > >> I am very much in favor of the overall approach on this patch
> > > series.
> > > >> I've experienced every single one of the problems described in
> > > this
> > > >> summary, and my company currently resorts to maintaining a hacky
> > > >> out-of-tree calibration tool to paper over these problems.
> > > >
> > > > Hi Matt,
> > > >
> > > > that is very heartwarming to hear. Is your tool specifically for
> > > Weston
> > > > too?
> > >
> > > Yes and no. It's not phrased as a patch against the Weston source
> > > code, but it uses heuristics for determining which output the raw
> > > /dev/input/* events should be correlated against, and those
> > > heuristics
> > > probably would fail if some different compositor happened to be
> > > running.
> > >
> > > >
> > > > I would be very happy if this proposal fits your needs, and
> > > certainly
> > > > interested in hearing where it falls short.
> > > >
> > > >
> > > > Thanks,
> > > > pq
> > > >
> > > >> On Fri, Mar 23, 2018 at 7:00 AM, Pekka Paalanen  > > .com> wrote:
> > > >> > From: Pekka Paalanen 
> > > >> >
> > > >> > Hi all,
> > > >> >
> > > >> > the existing touchscreen calibrator in Weston has several
> > > problems. This
> > > >> > proposal intends to solve them all by introducing a new
> > > protocol
> > > >> > extension for touchscreen calibration and a new calibrator
> > > tool.
> > > >> >
> > > >> > The benefits of the new tool, which the old tool lacks, are:
> > > >> >
> > > >> > - You can unambiguously pick a physical touch device to
> > > calibrate.
> > > >> >
> > > >> > - You can be sure your touch events come only from that
> > > particular
> > > >> >   device, and that you cannot miss touch events even if the
> > > current
> > > >> >   calibration is horribly wrong.
> > > >> >
> > > >> > - You can be sure the calibration window (pattern) is shown on
> > > the right
> > > >> >   output with the right coordinates.
> > > >> >
> > > >> > - You can unambiguously calibrate even multiple touchscreens
> > > that are
> > > >> >   all cloned (showing the same image).
> > > >> >
> > > >> > - You get a libinput style calibation matrix instead of the
> > > >> >   WL_CALIBRATION format which depends on output resolution.
> > > >> >
> > > >> > - You can load a new calibration into the compositor without
> > > playing
> > > >> >   tricks with udev or restarting the compositor.
> > > >> >
> > > >> > There is more discussion about the topic at:
> > > >> > https://phabricator.freedesktop.org/T7868
> > > >> >
> > > >> > This patch series depends on the clone mode series:
> > > >> > https://patchwork.freedesktop.org/series/32898/
> > > >> >
> > > >> > There is a full branch available at:
> > > >> > https://gitlab.collabora.com/pq/weston/commits/touchcalib-1
> > > >
> >
> > ___
> > 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

Re: wl_list_remove(>link) point to invalid address

[Pekka Paalanen]

On Thu, 29 Mar 2018 11:01:47 +0800
zou lan  wrote:

> Hi Pekka & all
> 
> When I run some full screen applications under desktop shell,  then kill
> some applications randomly, I find
> crashes maybe happened sometimes.
> 
> Crash call stack:
> weston_view_destroy-->weston_view_unmap-->wl_list_reomve(>link).
> The view is black view.

Hi,

that's a good find. If I understand correctly, we do not handle the
case where a weston_view is effectively unmapped by removing it from a
weston_layer. As weston_compositor::view_list is never torn down
properly but simply rebuilt, the unmapped view is left with dangling
pointers, and those will be hit when finally destroying the view.

Would that be an accurate explanation?

> Then I debug code, find that shell.c  lower_fullscreen_layer function only
> delete the layer of black view from layer_list, but not handle the
> 
> lower_fullscreen_layer()
> {
> .
> /* Hide the black view */
> 
> weston_layer_entry_remove(>fullscreen.black_view->layer_link);
> 
> wl_list_init(>fullscreen.black_view->layer_link.link);

This wl_list_init() seems to be redundant, btw.

> 
> weston_view_damage_below(shsurf->fullscreen.black_view);

Could this path simply call weston_view_unmap() to unmap the black_view?

It effectively is unmapping the view, but forgetting to clear the
'is_mapped' boolean, which later leads to accessing weston_view::link
dangling pointers. weston_view_unmap() does a lot more which might be
necessary as well.

> 
> }
> 
> The code will be called when start second full screen application.
> 
> when weston build view list again, the first surface's black view is not in
> the view list, the list  maybe point some invalid address.
> 
> I think ivi shell will have the same problems too.  I have a temporary
> patch to resolve this problem. I want to discuss it in the community.
> 
> option 1
> --- a/libweston/compositor.c
> +++ b/libweston/compositor.c
> @@ -2195,7 +2195,7 @@ view_list_add(struct weston_compositor *compositor,
> struct weston_subsurface *sub;
> 
> weston_view_update_transform(view);
> -
> +   wl_list_remove();

For better or worse, the weston_view::link is deliberately left with
garbage pointers when the weston_compositor::view_list is discarded for
a rebuild.

Wouldn't this addition cause weston_compositor::view_list to become
corrupted if the link removed here happened to be the first or the last
in the old list order? Or maybe be get lucky most times, and it would
need quite special circumstances to trigger the corruption.

I still think this approach is not good.

> 
> option 2:
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
> @@ -3651,6 +3651,7 @@ lower_fullscreen_layer(struct desktop_shell *shell,
> 
> weston_layer_entry_remove(>fullscreen.black_view->layer_link);
> 
> wl_list_init(>fullscreen.black_view->layer_link.link);
> 
> weston_view_damage_below(shsurf->fullscreen.black_view);
> +
>  wl_list_remove(>fullscreen.black_view->link);

Removing without init might cause issues with a potential double-remove
on e.g. view destruction.

> 
> }
> 
> Do you think these options reasonable? Thank you.

I think using weston_view_unmap() would be the best as I mentioned
above, unless there is some specific reason why it cannot be used. None
come to my mind this time.


Thanks,
pq


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

Re: [PATCH] compositor-fbdev: Add support for ABGR in calculate_pixman_format

[Pekka Paalanen]

On Wed, 19 Jul 2017 00:06:38 +0200
Pablo Castellano  wrote:

> In PostmarketOS we have added support for the Asus grouper device
> (Google Nexus 7 2012 tablet), which uses ABGR.
> 
> This mode seems to be less common and previously it was not correctly
> handled by calculate_pixman_format()
> 
> For more information, see:
> https://github.com/postmarketOS/pmbootstrap/wiki/asus-grouper-%28Google-Nexus-7-2012%29
> 
> Signed-off-by: Pablo Castellano 

Hi,

the web page mentions Tegra. Is there really no way to get DRM drivers
on that device?


Thanks,
pq

> ---
>  libweston/compositor-fbdev.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/libweston/compositor-fbdev.c b/libweston/compositor-fbdev.c
> index e80a504..37c8005 100644
> --- a/libweston/compositor-fbdev.c
> +++ b/libweston/compositor-fbdev.c
> @@ -211,8 +211,8 @@ calculate_pixman_format(struct fb_var_screeninfo *vinfo,
>   vinfo->blue.msb_right != 0)
>   return 0;
>  
> - /* Work out the format type from the offsets. We only support RGBA and
> -  * ARGB at the moment. */
> + /* Work out the format type from the offsets. We only support RGBA,
> +  * ARGB and ABGR at the moment. */
>   type = PIXMAN_TYPE_OTHER;
>  
>   if ((vinfo->transp.offset >= vinfo->red.offset ||
> @@ -224,6 +224,10 @@ calculate_pixman_format(struct fb_var_screeninfo *vinfo,
>vinfo->green.offset >= vinfo->blue.offset &&
>vinfo->blue.offset >= vinfo->transp.offset)
>   type = PIXMAN_TYPE_RGBA;
> + else if (vinfo->transp.offset >= vinfo->blue.offset &&
> +  vinfo->blue.offset >= vinfo->green.offset &&
> +  vinfo->green.offset >= vinfo->red.offset)
> + type = PIXMAN_TYPE_ABGR;
>  
>   if (type == PIXMAN_TYPE_OTHER)
>   return 0;



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

Re: [PATCH weston] Add a missing error check to weston_wm_handle_icon

[Pekka Paalanen]

On Sat, 17 Mar 2018 00:09:00 -0400
Dima Ryazanov  wrote:

> This fixes a crash when launching Duke Nukem Forever.
> (Sorry, I wish I had a less ridiculous test case...)
> 
> Signed-off-by: Dima Ryazanov 
> ---
>  xwayland/window-manager.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
> index c307e199..e9c60c1e 100644
> --- a/xwayland/window-manager.c
> +++ b/xwayland/window-manager.c
> @@ -1368,6 +1368,9 @@ weston_wm_handle_icon(struct weston_wm *wm, struct 
> weston_wm_window *window)
> wm->atom.net_wm_icon, XCB_ATOM_ANY, 0,
> UINT32_MAX);
>   reply = xcb_get_property_reply(wm->conn, cookie, NULL);
> + if (!reply)
> + return;
> +
>   length = xcb_get_property_value_length(reply);
>  
>   /* This is in 32-bit words, not in bytes. */

Hi,

this looks like a good fix, but the code was just reverted due to
several other issues. I'm copying to Scott so that if he wants to
re-send the icon feature once we have passed the final release, he could
integrate this fix as well.


Thanks,
pq


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

Re: [PATCH weston 00/25] A new touchscreen calibrator

[Philipp Kerling]

> Do you happen to know of a good readymade program that fakes the
> presence of a touch input device with uinput? I'd love to test this
> series, but current Weston is far ahead of what my embedded devices
> will do; so I'm in the position of mostly relying on the desktop for
> testing. 
I'm not sure whether it fits your use case, but you can give mtemu a
spin.

https://gitlab.com/shul/mtemu


> On Fri, Mar 23, 2018, 09:38 Matt Hoosier 
> wrote:
> > On Fri, Mar 23, 2018 at 9:30 AM, Pekka Paalanen  > m> wrote:
> > > On Fri, 23 Mar 2018 08:46:46 -0500
> > > Matt Hoosier  wrote:
> > >
> > >> I am very much in favor of the overall approach on this patch
> > series.
> > >> I've experienced every single one of the problems described in
> > this
> > >> summary, and my company currently resorts to maintaining a hacky
> > >> out-of-tree calibration tool to paper over these problems.
> > >
> > > Hi Matt,
> > >
> > > that is very heartwarming to hear. Is your tool specifically for
> > Weston
> > > too?
> > 
> > Yes and no. It's not phrased as a patch against the Weston source
> > code, but it uses heuristics for determining which output the raw
> > /dev/input/* events should be correlated against, and those
> > heuristics
> > probably would fail if some different compositor happened to be
> > running.
> > 
> > >
> > > I would be very happy if this proposal fits your needs, and
> > certainly
> > > interested in hearing where it falls short.
> > >
> > >
> > > Thanks,
> > > pq
> > >
> > >> On Fri, Mar 23, 2018 at 7:00 AM, Pekka Paalanen  > .com> wrote:
> > >> > From: Pekka Paalanen 
> > >> >
> > >> > Hi all,
> > >> >
> > >> > the existing touchscreen calibrator in Weston has several
> > problems. This
> > >> > proposal intends to solve them all by introducing a new
> > protocol
> > >> > extension for touchscreen calibration and a new calibrator
> > tool.
> > >> >
> > >> > The benefits of the new tool, which the old tool lacks, are:
> > >> >
> > >> > - You can unambiguously pick a physical touch device to
> > calibrate.
> > >> >
> > >> > - You can be sure your touch events come only from that
> > particular
> > >> >   device, and that you cannot miss touch events even if the
> > current
> > >> >   calibration is horribly wrong.
> > >> >
> > >> > - You can be sure the calibration window (pattern) is shown on
> > the right
> > >> >   output with the right coordinates.
> > >> >
> > >> > - You can unambiguously calibrate even multiple touchscreens
> > that are
> > >> >   all cloned (showing the same image).
> > >> >
> > >> > - You get a libinput style calibation matrix instead of the
> > >> >   WL_CALIBRATION format which depends on output resolution.
> > >> >
> > >> > - You can load a new calibration into the compositor without
> > playing
> > >> >   tricks with udev or restarting the compositor.
> > >> >
> > >> > There is more discussion about the topic at:
> > >> > https://phabricator.freedesktop.org/T7868
> > >> >
> > >> > This patch series depends on the clone mode series:
> > >> > https://patchwork.freedesktop.org/series/32898/
> > >> >
> > >> > There is a full branch available at:
> > >> > https://gitlab.collabora.com/pq/weston/commits/touchcalib-1
> > >
> 
> ___
> 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 00/25] A new touchscreen calibrator

[Pekka Paalanen]

On Mon, 02 Apr 2018 13:11:24 +
Matt Hoosier  wrote:

> Do you happen to know of a good readymade program that fakes the presence
> of a touch input device with uinput? I'd love to test this series, but
> current Weston is far ahead of what my embedded devices will do; so I'm in
> the position of mostly relying on the desktop for testing.

Hi Matt,

sorry, I have no knowledge of such tool, and the lack of it has
prevented me from testing things before as well.

Right now I have two borrowed touchscreens to work with, but I need to
send them back one day.


Thanks,
pq


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