Re: Additional pixel formats for wl_shm
On Fri, Apr 26, 2013 at 04:08:01PM +0300, Pekka Paalanen wrote: > Hi Kristian, > > I'm working on Raspberry Pi, where the VideoCore can deal directly with > a large number of pixel formats. However, wl_shm only exposes ARGB and > XRGB 32-bit per pixel formats, which on such tiny devices are very > wasteful. > > Would it be acceptable to add more formats to the wl_shm formats enum? > I have happily forgot all the previous discussions about the topic, and > a quick search into the mailing list archives didn't turn up much. > > We are especially interested in 16-bit per pixel formats to save memory > and bandwidth, and if I am reading the GLESv2 specification right, > RGB565, RGBA and RGBA5551 should be directly supported, so at least > those could be uploaded into textures without manual conversion. Of > course, the new rpi-renderer I am working on does not use GL for > compositing anymore, but it should be able to use those formats > directly, too. > > I have also another proposition: an extension to let wl_shm support > custom pixel formats outside of the core protocol listed formats. > > It would have a new global interface, which when bound, would advertise > additional pixel formats as a (string, uint) tuple, where the string > describes the pixel format, and the uint is the corresponding format > name to pass into wl_shm_pool.create_buffer as the format argument. > wl_shm would not advertise these format names itself. We would need to > define a WL_SHM_FORMAT_CUSTOM_BASE to reserve some name space for the > custom formats. > > Or something along those lines. > > Of course, this is just working around the pixel format enum wl_shm has > anyway, so if you would be willing to take more exotic pixel formats > into the wl_shm format enum, this extension would not be needed. > > Does any of this sound acceptable for upstream Wayland, or do you think > we'd better just write a new wl_buffer factory with everything we need? There's no reason not to just copy over the drm formats (which wl_drm already uses) to the wl_shm enum. I don't think we need an elaborate scheme for work-in-progress formats, lets just add the full list and then just advertise the ones the compositor supports. The main reason for not doing that originally was that I didn't want to overwhelm clients with a huge list of formats that they would have to pick and choose from. But as long as ARGB32 is always available, I don't see a problem in advertising extra formats. > Btw. Google found this: > http://people.freedesktop.org/~krh/fourcc.patch > Why was that or similar never used? :-) I ended up deciding to keep details about pixel formats private. It's still true that having a wayland list of pixel formats to choose from would be convenient, but in the end I didn't want to imply that wl_buffer is always about pixels. Kristian ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [DRM] Patch for Compositor Drm
On Mon, Apr 22, 2013 at 05:12:58PM +0100, Christopher Michael wrote: > From acb79e4a5921525b35e07e48f7f903e42a08fb7c Mon Sep 17 00:00:00 2001 > From: Chris Michael > Date: Mon, 22 Apr 2013 15:22:48 +0100 > Subject: [PATCH] Fix not checking return value of drmIoctl function call to > map dumb buffer. > > in drm_fb_create_dumb, the return value of the drmIoctl function call > to map the dumb buffer was never checked, thus the following "if > (ret)" check was invalid as it was checking the previous return value > from the above drmModeAddFB call. > > Signed-off-by: Chris Michael Yup, that looks good, thanks. Kristian > --- > src/compositor-drm.c |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/compositor-drm.c b/src/compositor-drm.c > index da1ba79..13b9d79 100644 > --- a/src/compositor-drm.c > +++ b/src/compositor-drm.c > @@ -255,7 +255,7 @@ drm_fb_create_dumb(struct drm_compositor *ec, > unsigned width, unsigned height) > > memset(&map_arg, 0, sizeof(map_arg)); > map_arg.handle = fb->handle; > - drmIoctl(fb->fd, DRM_IOCTL_MODE_MAP_DUMB, &map_arg); > + ret = drmIoctl(fb->fd, DRM_IOCTL_MODE_MAP_DUMB, &map_arg); > > if (ret) > goto err_add_fb; > -- > 1.7.9.5 > > ___ > wayland-devel mailing list > wayland-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 1/4] Add a destroy_signal on weston_output
On Wed, May 01, 2013 at 09:52:10PM +0100, Richard Hughes wrote:
> ---
> src/compositor.c | 3 +++
> src/compositor.h | 1 +
> 2 files changed, 4 insertions(+)
This and the other three patches look good, committed.
Kristian
> diff --git a/src/compositor.c b/src/compositor.c
> index 693df2c..0214eed 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
> @@ -2906,6 +2906,8 @@ weston_output_destroy(struct weston_output *output)
> {
> struct weston_compositor *c = output->compositor;
>
> + wl_signal_emit(&output->destroy_signal, output);
> +
> pixman_region32_fini(&output->region);
> pixman_region32_fini(&output->previous_damage);
> output->compositor->output_id_pool &= ~(1 << output->id);
> @@ -3064,6 +3066,7 @@ weston_output_init(struct weston_output *output, struct
> weston_compositor *c,
> weston_output_damage(output);
>
> wl_signal_init(&output->frame_signal);
> + wl_signal_init(&output->destroy_signal);
> wl_list_init(&output->animation_list);
> wl_list_init(&output->resource_list);
>
> diff --git a/src/compositor.h b/src/compositor.h
> index eb8ad82..3b08f29 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -175,6 +175,7 @@ struct weston_output {
> struct weston_output_zoom zoom;
> int dirty;
> struct wl_signal frame_signal;
> + struct wl_signal destroy_signal;
> uint32_t frame_time;
> int disable_planes;
>
> --
> 1.8.2.1
>
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Clarifying scope and goals for weston
On Wed, May 1, 2013 at 2:01 PM, Kristian Høgsberg wrote: > On Fri, Apr 19, 2013 at 11:03:16PM -0700, Kenneth Graunke wrote: >> On 04/18/2013 12:04 AM, Sam Spilsbury wrote: >> >On Wed, Apr 17, 2013 at 1:34 PM, Kristian Høgsberg >> >wrote: [snip] > Toolkit type scenegraphs > (QML, Clutter) has a lot of flexibility that tend to get in the way > when trying to reason about occlusion and coverage. Bandwidth saving > optimizations such as clipping obscured damage or splitting the > surfaces into opaque and transparent primitives just seem out of reach > when you have to deal with general 3D objects and punch through > several layers of abstractions. > This is true. I don't really think that providing rendering libraries would make a whole lot of sense for existing scene graphs. But it would make sense for other window managers which don't currently provide any kind of compositing story at all. Perhaps this is worth doing some prototyping around to see what does and does not make sense to split out? I'll put my hand up to volunteer, but my time is pretty limited these days so I'm not so sure how much I will be able to put into it. > > Kristian -- Sam Spilsbury ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
On Mon, Apr 22, 2013 at 01:30:28PM +0300, Pekka Paalanen wrote: > On Fri, 5 Apr 2013 14:23:50 -0500 > Thomas Daede wrote: > > > I am not sure that doing the color conversion in the compositor is the > > correct place. Some of it has to be there to support vcgt, but for > > more general conversions, it gets complicated quickly. > > > > Color correction is most important for artists doing work in something > > like the GIMP. Programs like this (as of GIMP 3.0, at least) generally > > work in higher bit depths - 16 bit, half float, or sometimes 32 bit > > float. It's much better to do conversion in the higher bit depth, then > > dither to the final display depth. Doing this with the compositor > > involves supporting all of these texture formats for subsurfaces - > > also, the toolkit has to support subsurfaces, because generally the UI > > will be some lower bit depth, or not need correction. > > > > Converting bit depths in the compositor would also require an extra > > buffer allocation and copy in the compositor. > > > > RGB images are also not the only thing that needs to be color > > corrected - for example, 10bit YUV video streams might want to be > > color corrected too. If we were to go as far as converting bit depths > > in the compositor, it wouldn't be much to add this. > > > > I think that providing color space regions to the client, relative to > > the client's buffer, including vcgt settings, would shove a lot of > > complexity away to clients that are already used to dealing with it. > > I'm not sure we can do that. The regions can be arbitrarily complex and > non-linear shapes. It's not too different from how we handle opaque regions. We split up the surfaces into opaque and transparent parts and render them using different shaders already. Kristian ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Clarifying scope and goals for weston
On Fri, Apr 19, 2013 at 11:03:16PM -0700, Kenneth Graunke wrote: > On 04/18/2013 12:04 AM, Sam Spilsbury wrote: > >On Wed, Apr 17, 2013 at 1:34 PM, Kristian Høgsberg > >wrote: > [snip] > >2. Re-use of weston functionality > > > >Weston has a lot of really great functionality that would be useful to > >lots of different desktops. I'm thinking about some of the core-stuff > >like geometry / texture clipping, opaque region calculation, video > >capture, damage handling, gbm etc. > > > >This stuff is really hard to get right, and its a big ask for people > >to deliver new shells on top of the wayland protocol when they have to > >get all the groundwork right first. Also, the main goals of renderers > >and display managers don't really differ at that much, you just want > >to get it a) right and b) fast. To that end, encouraging code sharing > >makes a lot of sense. > [snip] > > I couldn't agree more. A lot of work has gone into Weston to handle > these things correctly and efficiently. I don't see any benefit to > making each desktop environment reinvent this code in their > compositor. > > Today on X, we have inconsistent performance between KDE/GNOME > Shell/Unity/XFWM and so on. That seems like a tangible downside to > reinventing the wheel. > > I can't comment about the feasibility of weston-as-a-library or > DEs-as-shell-plugins, but finding a solution that takes this burden > off of DE authors and gets it done right/fast for everybody seems > like a fantastic feature in a new window system. I agree too, but the problem is that it's hard to reuse weston as without also reusing the scene graph and renderer. The mess that is all the input helpers in libwayland-server is from trying to split out input code without the scene graph. It's really hard to extract any meaningful, reusable logic without being able to transform events and do hit-testing. On the output side, the weston scene graph is really simple - it's really just a stack of 2D surfaces - to facilitate easy analysis when trying to map KMS pageflips and overlays. Toolkit type scenegraphs (QML, Clutter) has a lot of flexibility that tend to get in the way when trying to reason about occlusion and coverage. Bandwidth saving optimizations such as clipping obscured damage or splitting the surfaces into opaque and transparent primitives just seem out of reach when you have to deal with general 3D objects and punch through several layers of abstractions. However, most existing desktop compositors are integrated with and invested in their current scene graphs. GNOME Shell isn't going to ever drop clutter. I could imagine a split where libweston is a lower level scenegraph and handles top level windows from wayland clients and GNOME Shell just uses clutter for UI elements, which in turn are just wayland clients. That way we can share the input stack between weston and GNOME Shell as well as other weston plugins such as the RDP backend, remote rendering, fbdev and android backends. There are weston backends out there that off-load all rendering to custom, fixed-function 2D blender hw, or even all scanout-time compositing (each surface is a sprite) backends. I wish there was a way to use that with GNOME Shell and other real desktops. Kristian ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH 4/4] Include config.h in compositor-drm.c
--- src/compositor-drm.c | 4 1 file changed, 4 insertions(+) diff --git a/src/compositor-drm.c b/src/compositor-drm.c index 857aeed..db4db33 100644 --- a/src/compositor-drm.c +++ b/src/compositor-drm.c @@ -21,6 +21,10 @@ * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ +#ifdef HAVE_CONFIG_H +#include +#endif + #define _GNU_SOURCE #include -- 1.8.2.1 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH 3/4] Add a set_gamma vfunc on weston_output
---
src/compositor-drm.c | 24
src/compositor.h | 7 +++
2 files changed, 31 insertions(+)
diff --git a/src/compositor-drm.c b/src/compositor-drm.c
index c8016cd..857aeed 100644
--- a/src/compositor-drm.c
+++ b/src/compositor-drm.c
@@ -545,6 +545,27 @@ drm_output_render(struct drm_output *output,
pixman_region32_t *damage)
}
static void
+drm_output_set_gamma(struct weston_output *output_base,
+uint16_t size, uint16_t *r, uint16_t *g, uint16_t *b)
+{
+ int rc;
+ struct drm_output *output = (struct drm_output *) output_base;
+ struct drm_compositor *compositor = (struct drm_compositor *)
output->base.compositor;
+
+ /* check */
+ if (output_base->gamma_size != size)
+ return;
+ if (!output->original_crtc)
+ return;
+
+ rc = drmModeCrtcSetGamma(compositor->drm.fd,
+output->crtc_id,
+size, r, g, b);
+ if (rc)
+ weston_log("set gamma failed: %m\n");
+}
+
+static void
drm_output_repaint(struct weston_output *output_base,
pixman_region32_t *damage)
{
@@ -1799,6 +1820,9 @@ create_output_for_connector(struct drm_compositor *ec,
output->base.set_dpms = drm_set_dpms;
output->base.switch_mode = drm_output_switch_mode;
+ output->base.gamma_size = output->original_crtc->gamma_size;
+ output->base.set_gamma = drm_output_set_gamma;
+
weston_plane_init(&output->cursor_plane, 0, 0);
weston_plane_init(&output->fb_plane, 0, 0);
diff --git a/src/compositor.h b/src/compositor.h
index ce63fc2..63d1127 100644
--- a/src/compositor.h
+++ b/src/compositor.h
@@ -198,6 +198,13 @@ struct weston_output {
uint32_t backlight_current;
void (*set_backlight)(struct weston_output *output, uint32_t value);
void (*set_dpms)(struct weston_output *output, enum dpms_enum level);
+
+ uint16_t gamma_size;
+ void (*set_gamma)(struct weston_output *output,
+ uint16_t size,
+ uint16_t *r,
+ uint16_t *g,
+ uint16_t *b);
};
struct weston_xkb_info {
--
1.8.2.1
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH 2/4] Add a output_created_signal on weston_compositor
---
src/compositor.c | 2 ++
src/compositor.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/src/compositor.c b/src/compositor.c
index 0214eed..c1acd50 100644
--- a/src/compositor.c
+++ b/src/compositor.c
@@ -3076,6 +3076,7 @@ weston_output_init(struct weston_output *output, struct
weston_compositor *c,
output->global =
wl_display_add_global(c->wl_display, &wl_output_interface,
output, bind_output);
+ wl_signal_emit(&c->output_created_signal, output);
}
static void
@@ -3154,6 +3155,7 @@ weston_compositor_init(struct weston_compositor *ec,
wl_signal_init(&ec->show_input_panel_signal);
wl_signal_init(&ec->hide_input_panel_signal);
wl_signal_init(&ec->seat_created_signal);
+ wl_signal_init(&ec->output_created_signal);
ec->launcher_sock = weston_environment_get_fd("WESTON_LAUNCHER_SOCK");
ec->output_id_pool = 0;
diff --git a/src/compositor.h b/src/compositor.h
index 3b08f29..ce63fc2 100644
--- a/src/compositor.h
+++ b/src/compositor.h
@@ -313,6 +313,7 @@ struct weston_compositor {
struct wl_signal hide_input_panel_signal;
struct wl_signal seat_created_signal;
+ struct wl_signal output_created_signal;
struct wl_event_loop *input_loop;
struct wl_event_source *input_loop_source;
--
1.8.2.1
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH 1/4] Add a destroy_signal on weston_output
---
src/compositor.c | 3 +++
src/compositor.h | 1 +
2 files changed, 4 insertions(+)
diff --git a/src/compositor.c b/src/compositor.c
index 693df2c..0214eed 100644
--- a/src/compositor.c
+++ b/src/compositor.c
@@ -2906,6 +2906,8 @@ weston_output_destroy(struct weston_output *output)
{
struct weston_compositor *c = output->compositor;
+ wl_signal_emit(&output->destroy_signal, output);
+
pixman_region32_fini(&output->region);
pixman_region32_fini(&output->previous_damage);
output->compositor->output_id_pool &= ~(1 << output->id);
@@ -3064,6 +3066,7 @@ weston_output_init(struct weston_output *output, struct
weston_compositor *c,
weston_output_damage(output);
wl_signal_init(&output->frame_signal);
+ wl_signal_init(&output->destroy_signal);
wl_list_init(&output->animation_list);
wl_list_init(&output->resource_list);
diff --git a/src/compositor.h b/src/compositor.h
index eb8ad82..3b08f29 100644
--- a/src/compositor.h
+++ b/src/compositor.h
@@ -175,6 +175,7 @@ struct weston_output {
struct weston_output_zoom zoom;
int dirty;
struct wl_signal frame_signal;
+ struct wl_signal destroy_signal;
uint32_t frame_time;
int disable_planes;
--
1.8.2.1
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] Override modules list and don't always load desktop-shell.so
On Fri, Apr 19, 2013 at 04:11:01PM -0700, Bill Spitzak wrote: > >2013/4/16 Kristian Høgsberg : > > >>I know the current behavior is a bit problematic for some use cases. > >>However it works well for the case where you load a plugin in addition > >>to the shell, like xwayland. I'm not sure what the best way to make > >>both cases work is, except to make the shell plugin special. We could > >>go back to use a shell config key and command line option and make > >>modules only about additional modules. > > Would it be possible to load all the named modules, then find out if > any of them are shells, and load the default shell if none are? Ie > if the command line does not name a shell module you will get the > default one. Yeah, I think we could do something like that. Instead of looking up the module_init function we can dlsym for an instance of a weston_module struct, which can let us do ABI version checks and make sure we only load one of each type of module (shell, cms, etc). Kristian ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 3/3] Add a colord implementation of a CMS plugin for weston
On Wed, Apr 24, 2013 at 02:58:04PM +0100, Richard Hughes wrote:
> This allows users to change the assigned display profile in GNOME (using
> gnome-control-center) or KDE (using colord-kde) and also allows the profiling
> tools to correctly inhibit the calibration state whilst measuring the native
> screen response.
> ---
> configure.ac| 10 ++
> src/Makefile.am | 9 ++
> src/colord.c| 440
>
> 3 files changed, 459 insertions(+)
> create mode 100644 src/colord.c
I think this is fine, but I'm not sure about the threading conventions
for the colord objects. Currently the CdDevices are added and removed
in the weston thread for hotplug and in the glib thread for cold plug.
colord_update_output_from_device() is called both when adding outputs
(weston thread) and when the "changed" signal fires (in the glib
thread). In both cases it goes through the pipe and
weston_cms_set_color_profile() is always called from the weston
thread, which is good because that ends up calling the output vfunc,
which has to run in the weston thread.
Maybe a simpler approach would be to only handle the "changed" signal
in the glib thread and do everything else in the weston thread? That
is, cold-plug everything before starting the thread and have the
"changed" handler only write to the pipe?
Kristian
> diff --git a/configure.ac b/configure.ac
> index 1552d73..20011d2 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -277,6 +277,16 @@ AC_ARG_ENABLE(tablet-shell,
> AM_CONDITIONAL(ENABLE_TABLET_SHELL,
> test "x$enable_tablet_shell" = "xyes")
>
> +# CMS modules
> +AC_ARG_ENABLE(colord,
> + AS_HELP_STRING([--disable-colord],
> + [do not build colord CMS support]),,
> + enable_colord=yes)
> +AM_CONDITIONAL(ENABLE_COLORD,
> +test "x$enable_colord" = "xyes")
> +if test x$enable_colord = xyes; then
> + PKG_CHECK_MODULES(COLORD, colord >= 0.1.8)
> +fi
>
> AC_ARG_ENABLE(wcap-tools, [ --disable-wcap-tools],, enable_wcap_tools=yes)
> AM_CONDITIONAL(BUILD_WCAP_TOOLS, test x$enable_wcap_tools = xyes)
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 7489086..513b446 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -96,6 +96,7 @@ moduledir = $(libdir)/weston
> module_LTLIBRARIES = \
> $(desktop_shell)\
> $(tablet_shell) \
> + $(cms_colord) \
> $(x11_backend) \
> $(drm_backend) \
> $(wayland_backend) \
> @@ -253,6 +254,14 @@ tablet_shell_la_SOURCES =\
> tablet-shell-server-protocol.h
> endif
>
> +if ENABLE_COLORD
> +cms_colord = colord.la
> +colord_la_LDFLAGS = -module -avoid-version
> +colord_la_LIBADD = $(COMPOSITOR_LIBS) $(COLORD_LIBS)
> +colord_la_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS) $(COLORD_CFLAGS)
> +colord_la_SOURCES = colord.c
> +endif
> +
> BUILT_SOURCES = \
> screenshooter-server-protocol.h \
> screenshooter-protocol.c\
> diff --git a/src/colord.c b/src/colord.c
> new file mode 100644
> index 000..1a7fce0
> --- /dev/null
> +++ b/src/colord.c
> @@ -0,0 +1,440 @@
> +/*
> + * Copyright © 2013 Richard Hughes
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and
> + * its documentation for any purpose is hereby granted without fee, provided
> + * that the above copyright notice appear in all copies and that both that
> + * copyright notice and this permission notice appear in supporting
> + * documentation, and that the name of the copyright holders not be used in
> + * advertising or publicity pertaining to distribution of the software
> + * without specific, written prior permission. The copyright holders make
> + * no representations about the suitability of this software for any
> + * purpose. It is provided "as is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER
> + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF
> + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include
> +#endif
> +
> +#define _GNU_SOURCE
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "compositor.h"
> +#include "cms.h"
> +
> +struct colord_color_manager {
> + struct weston_color_manager base;
> + CdClient*client;
> + GHashTable
Re: [PATCH 2/3] Add initial color management framework code
On Wed, Apr 24, 2013 at 02:58:03PM +0100, Richard Hughes wrote:
> ICC profiles can now be specified in weston.ini for each output, or a CMS
> implementation can optionally loaded from a pluggable module.
Hey Richard,
This is cool stuff and I'm happy to see weston gain support for color
management. A few comments below.
Kristian
> ---
> configure.ac | 7 +++
> src/Makefile.am | 6 ++-
> src/cms.c| 106 +
> src/cms.h| 84 +
> src/compositor-drm.c | 147
> ++-
> src/compositor.c | 7 +++
> src/compositor.h | 21
> weston.ini | 2 +
> 8 files changed, 376 insertions(+), 4 deletions(-)
> create mode 100644 src/cms.c
> create mode 100644 src/cms.h
>
> diff --git a/configure.ac b/configure.ac
> index d5fea9d..1552d73 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -315,6 +315,13 @@ AC_MSG_NOTICE([Weston's native backend:
> $WESTON_NATIVE_BACKEND])
> AC_DEFINE_UNQUOTED([WESTON_NATIVE_BACKEND], ["$WESTON_NATIVE_BACKEND"],
> [The default backend to load, if not wayland nor x11.])
>
> +PKG_CHECK_MODULES(LCMS, lcms2,
> + [have_lcms=yes], [have_lcms=no])
> +if test "x$have_lcms" = xyes; then
> + AC_DEFINE(HAVE_LCMS, 1, [Have lcms support])
> +fi
> +AM_CONDITIONAL(HAVE_LCMS, [test "x$have_lcms" = xyes])
> +
> WAYLAND_SCANNER_RULES(['$(top_srcdir)/protocol'])
>
> AC_CONFIG_FILES([Makefile
> diff --git a/src/Makefile.am b/src/Makefile.am
> index d33ebc5..7489086 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -9,8 +9,8 @@ AM_CPPFLAGS = \
> -DIN_WESTON
>
> weston_LDFLAGS = -export-dynamic
> -weston_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS) $(LIBUNWIND_CFLAGS)
> -weston_LDADD = $(COMPOSITOR_LIBS) $(LIBUNWIND_LIBS) \
> +weston_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS) $(LIBUNWIND_CFLAGS)
> $(LCMS_CFLAGS)
> +weston_LDADD = $(COMPOSITOR_LIBS) $(LIBUNWIND_LIBS) $(LCMS_LIBS) \
> $(DLOPEN_LIBS) -lm ../shared/libshared.la
>
> weston_SOURCES = \
> @@ -20,6 +20,8 @@ weston_SOURCES =\
> compositor.h\
> filter.c\
> filter.h\
> + cms.c \
> + cms.h \
> screenshooter.c \
> screenshooter-protocol.c\
> screenshooter-server-protocol.h \
> diff --git a/src/cms.c b/src/cms.c
> new file mode 100644
> index 000..1acc680
> --- /dev/null
> +++ b/src/cms.c
> @@ -0,0 +1,106 @@
> +/*
> + * Copyright © 2013 Richard Hughes
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and
> + * its documentation for any purpose is hereby granted without fee, provided
> + * that the above copyright notice appear in all copies and that both that
> + * copyright notice and this permission notice appear in supporting
> + * documentation, and that the name of the copyright holders not be used in
> + * advertising or publicity pertaining to distribution of the software
> + * without specific, written prior permission. The copyright holders make
> + * no representations about the suitability of this software for any
> + * purpose. It is provided "as is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER
> + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF
> + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include
> +#endif
> +
> +#include
> +#include
> +#include
> +
> +#ifdef HAVE_LCMS
> +#include
> +#endif
> +
> +#include "compositor.h"
> +#include "cms.h"
> +
> +WL_EXPORT void
> +weston_cms_set_color_profile(struct weston_output *o,
> + struct weston_color_profile *p)
> +{
> + if (o->color_profile == p)
> + return;
> + if (o->color_profile)
> + weston_cms_destroy_profile(o->color_profile);
> + o->color_profile = p;
> + if (o->updated_color_profile)
> + o->updated_color_profile(o);
> +}
> +
> +WL_EXPORT int
> +weston_cms_output_added(struct weston_output *o,
> + enum weston_color_manager_flags flags)
> +{
> + struct weston_compositor *ec = o->compositor;
> + if (!ec->cms)
> + return 0;
> + if (!ec->cms->output_added)
> + return 0;
> + return ec->cms->output_ad
Re: [PATCH] client: Add wl_display_prepare_read() API to thread model assumptions
Kristian Høgsberg wrote:
+ * A correct sequence would be:
+ *
+ * while (wl_display_prepare_read(display) != 0)
+ * wl_display_dispatch_pending(display);
+ * wl_display_flush(display);
+ * poll(fds, nfds, -1);
+ * wl_display_read_events(display);
+ * wl_display_dispatch_pending(display);
When writing a client I need to do "idle" effects (what Qt calls
"aboutToBlock"), in particular redrawing all damaged surfaces. This may
trigger more events so it has to be in the prepare loop. I need to
remove the second dispatch, because I need to make sure draw is called
if any events are handled but I don't want to call it an extra time if
prepare indicates more events. This also I think removes a redundant
test for pending events that is repeated by the prepare_read():
mainloop() {
for (;;) {
while (wl_display_prepare_read(display) != 0) {
wl_display_dispatch_pending(display);
draw_damaged_surfaces();
}
wl_display_flush(display);
poll(fds, nfds, -1);
wl_display_read_events(display);
}
}
Now I'm wondering if dispatch_pending is *ever* called without the
prepare_read(). Perhaps these can be replaced with a single function
that is the equivalent of:
wl_foo(display) {
if (wl_display_prepare_read(display)) {
wl_display_dispatch_pending(display);
return true;
} else {
return false;
}
}
And the main loop looks like this:
mainloop() {
for (;;) {
while (wl_foo(display))
draw_damaged_surfaces();
wl_display_flush(display);
poll(fds, nfds, -1);
wl_display_read_events(display);
}
}
If clients have to register the idle callback then this function could
do the loop as well, which may be more efficient and allows error
indicators to be returned. I think it could even do a blocking
read_events() and the display_flush(). Then the main loop looks like this:
mainloop() {
wl_add_idle_callback(draw_damaged_surfaces);
for (;;) {
poll(fds, nfds, -1);
if (wl_foo()) abort();
}
}
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 1/3] Extract and parse the EDID when outputs are added
On Wed, Apr 24, 2013 at 02:58:02PM +0100, Richard Hughes wrote:
> At the moment we're only extracting interesting strings. We have to be quite
> careful parsing the EDID data, as vendors like to do insane things.
>
> The original EDID parsing code was written by me for gnome-color-manager.
> ---
> src/compositor-drm.c | 149
> +++
> src/compositor.h | 2 +-
> 2 files changed, 150 insertions(+), 1 deletion(-)
Looks good, thanks.
Kristian
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index da1ba79..c8016cd 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c
> @@ -25,6 +25,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -131,6 +132,13 @@ struct drm_fb {
> void *map;
> };
>
> +struct drm_edid {
> + char eisa_id[13];
> + char monitor_name[13];
> + char pnp_id[5];
> + char serial_number[13];
> +};
> +
> struct drm_output {
> struct weston_output base;
>
> @@ -139,6 +147,7 @@ struct drm_output {
> int pipe;
> uint32_t connector_id;
> drmModeCrtcPtr original_crtc;
> + struct drm_edid edid;
>
> int vblank_pending;
> int page_flip_pending;
> @@ -1486,6 +1495,143 @@ drm_output_fini_pixman(struct drm_output *output)
> }
> }
>
> +static void
> +edid_parse_string(const uint8_t *data, char text[])
> +{
> + int i;
> + int replaced = 0;
> +
> + /* this is always 12 bytes, but we can't guarantee it's null
> + * terminated or not junk. */
> + strncpy(text, (const char *) data, 12);
> +
> + /* remove insane chars */
> + for (i = 0; text[i] != '\0'; i++) {
> + if (text[i] == '\n' ||
> + text[i] == '\r') {
> + text[i] = '\0';
> + break;
> + }
> + }
> +
> + /* ensure string is printable */
> + for (i = 0; text[i] != '\0'; i++) {
> + if (!isprint(text[i])) {
> + text[i] = '-';
> + replaced++;
> + }
> + }
> +
> + /* if the string is random junk, ignore the string */
> + if (replaced > 4)
> + text[0] = '\0';
> +}
> +
> +#define EDID_DESCRIPTOR_ALPHANUMERIC_DATA_STRING 0xfe
> +#define EDID_DESCRIPTOR_DISPLAY_PRODUCT_NAME 0xfc
> +#define EDID_DESCRIPTOR_DISPLAY_PRODUCT_SERIAL_NUMBER0xff
> +#define EDID_OFFSET_DATA_BLOCKS 0x36
> +#define EDID_OFFSET_LAST_BLOCK 0x6c
> +#define EDID_OFFSET_PNPID0x08
> +#define EDID_OFFSET_SERIAL 0x0c
> +
> +static int
> +edid_parse(struct drm_edid *edid, const uint8_t *data, size_t length)
> +{
> + int i;
> + uint32_t serial_number;
> +
> + /* check header */
> + if (length < 128)
> + return -1;
> + if (data[0] != 0x00 || data[1] != 0xff)
> + return -1;
> +
> + /* decode the PNP ID from three 5 bit words packed into 2 bytes
> + * /--08--\/--09--\
> + * 7654321076543210
> + * |\---/\---/\---/
> + * R C1 C2 C3 */
> + edid->pnp_id[0] = 'A' + ((data[EDID_OFFSET_PNPID + 0] & 0x7c) / 4) - 1;
> + edid->pnp_id[1] = 'A' + ((data[EDID_OFFSET_PNPID + 0] & 0x3) * 8) +
> ((data[EDID_OFFSET_PNPID + 1] & 0xe0) / 32) - 1;
> + edid->pnp_id[2] = 'A' + (data[EDID_OFFSET_PNPID + 1] & 0x1f) - 1;
> + edid->pnp_id[3] = '\0';
> +
> + /* maybe there isn't a ASCII serial number descriptor, so use this
> instead */
> + serial_number = (uint32_t) data[EDID_OFFSET_SERIAL + 0];
> + serial_number += (uint32_t) data[EDID_OFFSET_SERIAL + 1] * 0x100;
> + serial_number += (uint32_t) data[EDID_OFFSET_SERIAL + 2] * 0x1;
> + serial_number += (uint32_t) data[EDID_OFFSET_SERIAL + 3] * 0x100;
> + if (serial_number > 0)
> + sprintf(edid->serial_number, "%lu", (unsigned long)
> serial_number);
> +
> + /* parse EDID data */
> + for (i = EDID_OFFSET_DATA_BLOCKS;
> + i <= EDID_OFFSET_LAST_BLOCK;
> + i += 18) {
> + /* ignore pixel clock data */
> + if (data[i] != 0)
> + continue;
> + if (data[i+2] != 0)
> + continue;
> +
> + /* any useful blocks? */
> + if (data[i+3] == EDID_DESCRIPTOR_DISPLAY_PRODUCT_NAME) {
> + edid_parse_string(&data[i+5],
> + edid->monitor_name);
> + } else if (data[i+3] ==
> EDID_DESCRIPTOR_DISPLAY_PRODUCT_SERIAL_NUMBER) {
> + edid_parse_string(&data[i+5],
> + edid->serial_number);
> + } else if (data[i+3] ==
> EDID_DESCRIPTOR_ALPHANUMERIC_DATA_STRING) {
> + edid_parse_string(&data[i+5],
> + edid->eisa_id);
> + }
> +
Re: [PATCH] weston.ini: document background-type
On Mon, Apr 22, 2013 at 11:00:07AM +0200, Emilio Pozuelo Monfort wrote: > --- > man/weston.ini.man |4 > weston.ini |1 + > 2 files changed, 5 insertions(+) Thanks, committed. Kristian > diff --git a/man/weston.ini.man b/man/weston.ini.man > index 8dde82c..2287730 100644 > --- a/man/weston.ini.man > +++ b/man/weston.ini.man > @@ -109,6 +109,10 @@ The entries that can appear in this section are: > .BI "background-image=" file > sets the path for the background image file (string). > .TP 7 > +.BI "background-type=" tile > +determines how the background image is drawn (string). Can be scale or > +tile (default). > +.TP 7 > .BI "background-color=" 0xAARRGGBB > sets the color of the background (unsigned integer). The hexadecimal > digit pairs are in order alpha, red, green, and blue. > diff --git a/weston.ini b/weston.ini > index c6cff76..cd34044 100644 > --- a/weston.ini > +++ b/weston.ini > @@ -4,6 +4,7 @@ > [shell] > background-image=/usr/share/backgrounds/gnome/Aqua.jpg > background-color=0xff002244 > +background-type=tile > panel-color=0x90ff > locking=true > animation=zoom > -- > 1.7.10.4 > > ___ > wayland-devel mailing list > wayland-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] X server uses xwayland.conf instead of xorg.conf
On Sun, Apr 21, 2013 at 10:24:58PM -0700, spit...@gmail.com wrote:
> From: spitzak
>
> This allows X applications to be run on wayland without having to
> delete conf files needed to run the legacy X server on the same machine.
>
> This is the only method I have found that will allow the built-in
> default conf file to be used while an /etc/X11/xorg.conf.d directory
> exists. There is no way to achieve this with switches to the server.
>
> A file is needed to use the wlshm driver, this must now be named
> $INSTALL/etc/X11/xwayland.conf. The instructions for running the
> x server under wayland need to be fixed for this.
This isn't the right way to make X load the wlshm driver. The native
drivers all should fail to load if they don't support xwayland. If
that happens we need to change the fallback in the automatic driver
picking to pick wlshm instead of fbdev or vesa or whatever it's
picking now.
Kristian
> ---
> hw/xfree86/common/xf86Config.c |7 +--
> hw/xfree86/common/xf86Init.c |4
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/hw/xfree86/common/xf86Config.c b/hw/xfree86/common/xf86Config.c
> index 3934ff0..a4f7f97 100644
> --- a/hw/xfree86/common/xf86Config.c
> +++ b/hw/xfree86/common/xf86Config.c
> @@ -2386,8 +2386,11 @@ xf86HandleConfigFile(Bool autoconfig)
> dirfrom = X_CMDLINE;
>
> xf86initConfigFiles();
> -sysdirname = xf86openConfigDirFiles(SYS_CONFIGDIRPATH, NULL,
> -PROJECTROOT);
> +if (xf86ConfigDir)
> + sysdirname = 0;
> +else
> + sysdirname = xf86openConfigDirFiles(SYS_CONFIGDIRPATH, NULL,
> + PROJECTROOT);
> dirname = xf86openConfigDirFiles(dirsearch, xf86ConfigDir,
> PROJECTROOT);
> filename = xf86openConfigFile(filesearch, xf86ConfigFile,
> PROJECTROOT);
> if (filename) {
> diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c
> index a062929..b2a668c 100644
> --- a/hw/xfree86/common/xf86Init.c
> +++ b/hw/xfree86/common/xf86Init.c
> @@ -1412,6 +1412,10 @@ ddxProcessArgument(int argc, char **argv, int i)
>if (!strcmp(argv[i], "-wayland"))
>{
> xorgWayland = TRUE;
> +if (!xf86ConfigFile)
> + xf86ConfigFile = "xwayland.conf";
> +if (!xf86ConfigDir)
> + xf86ConfigDir = "xwayland.conf.d";
> return 1;
>}
>
> --
> 1.7.9.5
>
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] Fixed simple-egl tear-down order to prevent a crash on exit time.
On Fri, Apr 19, 2013 at 05:49:12PM +, Yeh, Sinclair wrote:
> wl_egl_window_destory() distroys the window handle that
> dri2_destroy_surface() later uses when eglTerminate() is called.
>
> Reordering the tear down order prevents such case from occuring.
That's a good fix, thanks. Committed.
Kristian
> ---
> clients/simple-egl.c | 11 ++-
> 1 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/clients/simple-egl.c b/clients/simple-egl.c index
> 26ebe5c..f4468b7 100644
> --- a/clients/simple-egl.c
> +++ b/clients/simple-egl.c
> @@ -146,11 +146,6 @@ init_egl(struct display *display, int opaque) static
> void fini_egl(struct display *display) {
> - /* Required, otherwise segfault in egl_dri2.c: dri2_make_current()
> - * on eglReleaseThread(). */
> - eglMakeCurrent(display->egl.dpy, EGL_NO_SURFACE, EGL_NO_SURFACE,
> -EGL_NO_CONTEXT);
> -
> eglTerminate(display->egl.dpy);
> eglReleaseThread();
> }
> @@ -330,6 +325,12 @@ create_surface(struct window *window) static void
> destroy_surface(struct window *window) {
> + /* Required, otherwise segfault in egl_dri2.c: dri2_make_current()
> + * on eglReleaseThread(). */
> + eglMakeCurrent(window->display->egl.dpy, EGL_NO_SURFACE, EGL_NO_SURFACE,
> +EGL_NO_CONTEXT);
> +
> + eglDestroySurface(window->display->egl.dpy, window->egl_surface);
> wl_egl_window_destroy(window->native);
>
> wl_shell_surface_destroy(window->shell_surface);
> --
> 1.7.7.6
>
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] client: Add wl_display_prepare_read() API to thread model assumptions
On 01.05.2013 17:52, Kristian Høgsberg wrote:
> On Tue, Apr 30, 2013 at 11:45:15PM +0200, Uli Schlachter wrote:
>> On 30.04.2013 22:06, Kristian Høgsberg wrote:
>> [...]
>>> diff --git a/src/connection.c b/src/connection.c
>>> index dca134b..b26402f 100644
>>> --- a/src/connection.c
>>> +++ b/src/connection.c
>>> @@ -324,7 +324,7 @@ wl_connection_read(struct wl_connection *connection)
>>> msg.msg_flags = 0;
>>>
>>> do {
>>> - len = wl_os_recvmsg_cloexec(connection->fd, &msg, 0);
>>> + len = wl_os_recvmsg_cloexec(connection->fd, &msg, MSG_DONTWAIT);
>>> } while (len < 0 && errno == EINTR);
>>
>> Uhm, what? Why?
>>
>> If I understand this correctly, this will make wl_connection_read()
>> fail with EAGAIN or EWOULDBLOCK if nothing can be read. Thus I
>> looked at a random caller of this and the first one that grep found
>> was dispatch_queue() in wayland-client.c. This one will call
>> display_fatal_error() if reading fails.
>>
>> Looking at this patch, this behavior isn't changed by this patch.
>>
>> So: Uhm, what? Why?
>
> Yea, this is a little out-of-the blue. The reason is that
> wl_display_read_events() can't block in wl_connetion_read(). One of
> the problems this approach also solves is that we no longer blocks
> with the display lock held. Before, if we end up blocking in
> wl_display_dispatch(), we do so with the lock held, meaning other
> threads can't send requests. Without this change, a thread could call
> wl_display_prepare_read() and then immediately call
> wl_display_read_events(), which would then block in recvmsg(), with
> the lock held.
>
> wl_connection_read() is not a public entry point and read_events() is
> the only caller. The only place this change might be visible is in
> wl_display_dispatch(), which now instead drops the lock and blocks in
> poll().
Well, according to grep, wl_connection_read() is also used by some tests and
src/wayland-server.c. I didn't look at the tests and wayland-server.c looks like
it should never manage to call wl_connection_read().
Hm... Let's just hope the best.
>>> diff --git a/src/wayland-client.c b/src/wayland-client.c
>>> index 7847370..310d054 100644
>>> --- a/src/wayland-client.c
>>> +++ b/src/wayland-client.c
>>> @@ -21,6 +21,8 @@
>>> * OF THIS SOFTWARE.
>>> */
>>>
>>> +#define _GNU_SOURCE
>>> +
>>> #include
>>> #include
>>> #include
>>> @@ -83,6 +85,9 @@ struct wl_display {
>>> struct wl_event_queue queue;
>>> struct wl_list event_queue_list;
>>> pthread_mutex_t mutex;
>>> +
>>> + int reader_count;
>>> + pthread_cond_t reader_cond;
>>> };
>>>
>>> /** \endcond */
>>> @@ -522,6 +527,8 @@ wl_display_connect_to_fd(int fd)
>>> wl_event_queue_init(&display->queue, display);
>>> wl_list_init(&display->event_queue_list);
>>> pthread_mutex_init(&display->mutex, NULL);
>>> + pthread_cond_init(&display->reader_cond, NULL);
>>> + display->reader_count = 0;
>>>
>>> wl_map_insert_new(&display->objects, WL_MAP_CLIENT_SIDE, NULL);
>>>
>>> @@ -537,14 +544,19 @@ wl_display_connect_to_fd(int fd)
>>> display->proxy.refcount = 1;
>>>
>>> display->connection = wl_connection_create(display->fd);
>>> - if (display->connection == NULL) {
>>> - wl_map_release(&display->objects);
>>> - close(display->fd);
>>> - free(display);
>>> - return NULL;
>>> - }
>>> + if (display->connection == NULL)
>>> + goto err_connection;
>>>
>>> return display;
>>> +
>>> + err_connection:
>>> + pthread_mutex_destroy(&display->mutex);
>>> + pthread_cond_destroy(&display->reader_cond);
>>> + wl_map_release(&display->objects);
>>> + close(display->fd);
>>> + free(display);
>>> +
>>> + return NULL;
>>> }
>>>
>>> /** Connect to a Wayland display
>>> @@ -599,6 +611,7 @@ wl_display_disconnect(struct wl_display *display)
>>> wl_map_release(&display->objects);
>>> wl_event_queue_release(&display->queue);
>>> pthread_mutex_destroy(&display->mutex);
>>> + pthread_cond_destroy(&display->reader_cond);
>>> if (display->fd > 0)
>>> close(display->fd);
>>>
>>> @@ -851,65 +864,202 @@ dispatch_event(struct wl_display *display, struct
>>> wl_event_queue *queue)
>>> }
>>>
>>> static int
>>> -dispatch_queue(struct wl_display *display,
>>> - struct wl_event_queue *queue, int block)
>>> +read_events(struct wl_display *display)
>>> {
>>> - int len, size, count, ret;
>>> + int len, size;
>>>
>>> - pthread_mutex_lock(&display->mutex);
>>> -
>>> - if (display->last_error)
>>> - goto err_unlock;
>>> -
>>> - ret = wl_connection_flush(display->connection);
>>> - if (ret < 0 && errno != EAGAIN) {
>>> - display_fatal_error(display, errno);
>>> - goto err_unlock;
>>> - }
>>> -
>>> - if (block && wl_list_empty(&queue->event_list) &&
>>> - pthread_equal(display->display_thread, pthread_self())) {
>>> + display->reader_count--;
>>> + if (displa
Re: [PATCH] client: Add wl_display_prepare_read() API to thread model assumptions
On Tue, Apr 30, 2013 at 11:45:15PM +0200, Uli Schlachter wrote:
> Evening,
>
> On 30.04.2013 22:06, Kristian Høgsberg wrote:
> [...]
> > diff --git a/src/connection.c b/src/connection.c
> > index dca134b..b26402f 100644
> > --- a/src/connection.c
> > +++ b/src/connection.c
> > @@ -324,7 +324,7 @@ wl_connection_read(struct wl_connection *connection)
> > msg.msg_flags = 0;
> >
> > do {
> > - len = wl_os_recvmsg_cloexec(connection->fd, &msg, 0);
> > + len = wl_os_recvmsg_cloexec(connection->fd, &msg, MSG_DONTWAIT);
> > } while (len < 0 && errno == EINTR);
>
> Uhm, what? Why?
>
> If I understand this correctly, this will make wl_connection_read()
> fail with EAGAIN or EWOULDBLOCK if nothing can be read. Thus I
> looked at a random caller of this and the first one that grep found
> was dispatch_queue() in wayland-client.c. This one will call
> display_fatal_error() if reading fails.
>
> Looking at this patch, this behavior isn't changed by this patch.
>
> So: Uhm, what? Why?
Yea, this is a little out-of-the blue. The reason is that
wl_display_read_events() can't block in wl_connetion_read(). One of
the problems this approach also solves is that we no longer blocks
with the display lock held. Before, if we end up blocking in
wl_display_dispatch(), we do so with the lock held, meaning other
threads can't send requests. Without this change, a thread could call
wl_display_prepare_read() and then immediately call
wl_display_read_events(), which would then block in recvmsg(), with
the lock held.
wl_connection_read() is not a public entry point and read_events() is
the only caller. The only place this change might be visible is in
wl_display_dispatch(), which now instead drops the lock and blocks in
poll().
> > diff --git a/src/wayland-client.c b/src/wayland-client.c
> > index 7847370..310d054 100644
> > --- a/src/wayland-client.c
> > +++ b/src/wayland-client.c
> > @@ -21,6 +21,8 @@
> > * OF THIS SOFTWARE.
> > */
> >
> > +#define _GNU_SOURCE
> > +
> > #include
> > #include
> > #include
> > @@ -83,6 +85,9 @@ struct wl_display {
> > struct wl_event_queue queue;
> > struct wl_list event_queue_list;
> > pthread_mutex_t mutex;
> > +
> > + int reader_count;
> > + pthread_cond_t reader_cond;
> > };
> >
> > /** \endcond */
> > @@ -522,6 +527,8 @@ wl_display_connect_to_fd(int fd)
> > wl_event_queue_init(&display->queue, display);
> > wl_list_init(&display->event_queue_list);
> > pthread_mutex_init(&display->mutex, NULL);
> > + pthread_cond_init(&display->reader_cond, NULL);
> > + display->reader_count = 0;
> >
> > wl_map_insert_new(&display->objects, WL_MAP_CLIENT_SIDE, NULL);
> >
> > @@ -537,14 +544,19 @@ wl_display_connect_to_fd(int fd)
> > display->proxy.refcount = 1;
> >
> > display->connection = wl_connection_create(display->fd);
> > - if (display->connection == NULL) {
> > - wl_map_release(&display->objects);
> > - close(display->fd);
> > - free(display);
> > - return NULL;
> > - }
> > + if (display->connection == NULL)
> > + goto err_connection;
> >
> > return display;
> > +
> > + err_connection:
> > + pthread_mutex_destroy(&display->mutex);
> > + pthread_cond_destroy(&display->reader_cond);
> > + wl_map_release(&display->objects);
> > + close(display->fd);
> > + free(display);
> > +
> > + return NULL;
> > }
> >
> > /** Connect to a Wayland display
> > @@ -599,6 +611,7 @@ wl_display_disconnect(struct wl_display *display)
> > wl_map_release(&display->objects);
> > wl_event_queue_release(&display->queue);
> > pthread_mutex_destroy(&display->mutex);
> > + pthread_cond_destroy(&display->reader_cond);
> > if (display->fd > 0)
> > close(display->fd);
> >
> > @@ -851,65 +864,202 @@ dispatch_event(struct wl_display *display, struct
> > wl_event_queue *queue)
> > }
> >
> > static int
> > -dispatch_queue(struct wl_display *display,
> > - struct wl_event_queue *queue, int block)
> > +read_events(struct wl_display *display)
> > {
> > - int len, size, count, ret;
> > + int len, size;
> >
> > - pthread_mutex_lock(&display->mutex);
> > -
> > - if (display->last_error)
> > - goto err_unlock;
> > -
> > - ret = wl_connection_flush(display->connection);
> > - if (ret < 0 && errno != EAGAIN) {
> > - display_fatal_error(display, errno);
> > - goto err_unlock;
> > - }
> > -
> > - if (block && wl_list_empty(&queue->event_list) &&
> > - pthread_equal(display->display_thread, pthread_self())) {
> > + display->reader_count--;
> > + if (display->reader_count == 0) {
>
> Code like this always makes me think "what happens when the reader_count
> becomes
> negative?". Without having looked much at the rest of this patch yet, I would
> suggest making reader_count an unsigned int and adding an
> assert(display->reader_count > 0); before it is decremented.
Ye
