Re: [PATCH:libX11] If XGetImage fails to create image, don't dereference it to bounds check

[Alan Coopersmith]

On 03/ 7/18 01:42 PM, Emil Velikov wrote:
> On 7 March 2018 at 20:10, Alan Coopersmith  
> wrote:
>> That should be effectively equivalent, just less change in indentation for
>> the lines in between.
>>
> You're correct - it's functionally identical although it seems cleaner.

While the patch seems cleaner & simpler this way, I think the version in my
original patch results in the final code being cleaner and more straightforward
to understand without having to trace where goto's are jumping to, and that's
more important long-term than the patch itself.

> Regardless of which version you opt for
> Reviewed-by: Emil Velikov 

Thanks,

-alan-
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH:libX11] If XGetImage fails to create image, don't dereference it to bounds check

[Emil Velikov]

On 7 March 2018 at 20:10, Alan Coopersmith  wrote:
> On 03/ 7/18 05:36 AM, Emil Velikov wrote:
>> Hi Alan,
>>
>> On 6 March 2018 at 21:47, Alan Coopersmith  
>> wrote:
>>> Reported by gcc 7.3:
>>>
>>> GetImage.c:110:25: warning: potential null pointer dereference 
>>> [-Wnull-dereference]
>>>   if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 ||
>>> ~^~~~
>>>
>>> Introduced by 8ea762f94f4c942d898fdeb590a1630c83235c17 in Xlib 1.6.4
>>>
>>> Signed-off-by: Alan Coopersmith 
>>> ---
>>>  src/GetImage.c | 16 +---
>>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/GetImage.c b/src/GetImage.c
>>> index ff32d589..44a576a1 100644
>>> --- a/src/GetImage.c
>>> +++ b/src/GetImage.c
>>> @@ -105,14 +105,16 @@ XImage *XGetImage (
>>> planes = 1;
>>> }
>>>
>>> -   if (!image)
>>> +   if (!image) {
>>> Xfree(data);
>>> -   if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 ||
>>> -   INT_MAX / image->height <= image->bytes_per_line ||
>>> -   INT_MAX / planes <= image->height * image->bytes_per_line ||
>>> -   nbytes < planes * image->height * image->bytes_per_line) {
>>> -   XDestroyImage(image);
>>> -   image = NULL;
>>> +   } else {
>>> +if (planes < 1 || image->height < 1 || image->bytes_per_line < 
>>> 1 ||
>>> +INT_MAX / image->height <= image->bytes_per_line ||
>>> +INT_MAX / planes <= image->height * image->bytes_per_line 
>>> ||
>>> +nbytes < planes * image->height * image->bytes_per_line) {
>>> +XDestroyImage(image);
>>> +image = NULL;
>>> +}
>>
>> Instead of reshuffling, one could easily add the missing unlock/sync
>> in the error path.
>> Or even a goto unlock?
>
> Sorry - not sure I'm following - are you suggesting something like this?
>
> --- a/src/GetImage.c
> +++ b/src/GetImage.c
> @@ -104,17 +104,20 @@ XImage *XGetImage (
> _XGetScanlinePad(dpy, (int) rep.depth), 0);
> planes = 1;
> }
>
> -   if (!image)
> +   if (!image) {
> Xfree(data);
> +   goto unlock;
> +   }
> if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 ||
> INT_MAX / image->height <= image->bytes_per_line ||
> INT_MAX / planes <= image->height * image->bytes_per_line ||
> nbytes < planes * image->height * image->bytes_per_line) {
> XDestroyImage(image);
> image = NULL;
> }
> +  unlock:
> UnlockDisplay(dpy);
> SyncHandle();
> return (image);
>  }
>
Yes, precisely.

>
> That should be effectively equivalent, just less change in indentation for
> the lines in between.
>
You're correct - it's functionally identical although it seems cleaner.

Regardless of which version you opt for
Reviewed-by: Emil Velikov 

-Emil
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] dri2: Sync i965_pci_ids.h from Mesa.

[Adam Jackson]

On Wed, 2018-03-07 at 07:46 -0800, Rodrigo Vivi wrote:
> Copied from Mesa with no modifications.
> 
> Gives us Geminilake and Kaby Lake platform names updates and
> sync on Coffee Lake PCI IDs.
> 
> Cc: Timo Aaltonen 
> Signed-off-by: Rodrigo Vivi 

Merged, thanks:

remote: I: patch #208689 updated using rev 
90e0cdd42dfda2accfadffa5c550712696902e14.
remote: I: 1 patch(es) updated to state Accepted.
To ssh://git.freedesktop.org/git/xorg/xserver
   43576b901..90e0cdd42  master -> master

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH:libX11] If XGetImage fails to create image, don't dereference it to bounds check

[Alan Coopersmith]

On 03/ 7/18 05:36 AM, Emil Velikov wrote:
> Hi Alan,
> 
> On 6 March 2018 at 21:47, Alan Coopersmith  
> wrote:
>> Reported by gcc 7.3:
>>
>> GetImage.c:110:25: warning: potential null pointer dereference 
>> [-Wnull-dereference]
>>   if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 ||
>> ~^~~~
>>
>> Introduced by 8ea762f94f4c942d898fdeb590a1630c83235c17 in Xlib 1.6.4
>>
>> Signed-off-by: Alan Coopersmith 
>> ---
>>  src/GetImage.c | 16 +---
>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/GetImage.c b/src/GetImage.c
>> index ff32d589..44a576a1 100644
>> --- a/src/GetImage.c
>> +++ b/src/GetImage.c
>> @@ -105,14 +105,16 @@ XImage *XGetImage (
>> planes = 1;
>> }
>>
>> -   if (!image)
>> +   if (!image) {
>> Xfree(data);
>> -   if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 ||
>> -   INT_MAX / image->height <= image->bytes_per_line ||
>> -   INT_MAX / planes <= image->height * image->bytes_per_line ||
>> -   nbytes < planes * image->height * image->bytes_per_line) {
>> -   XDestroyImage(image);
>> -   image = NULL;
>> +   } else {
>> +if (planes < 1 || image->height < 1 || image->bytes_per_line < 
>> 1 ||
>> +INT_MAX / image->height <= image->bytes_per_line ||
>> +INT_MAX / planes <= image->height * image->bytes_per_line ||
>> +nbytes < planes * image->height * image->bytes_per_line) {
>> +XDestroyImage(image);
>> +image = NULL;
>> +}
> 
> Instead of reshuffling, one could easily add the missing unlock/sync
> in the error path.
> Or even a goto unlock?

Sorry - not sure I'm following - are you suggesting something like this?

--- a/src/GetImage.c
+++ b/src/GetImage.c
@@ -104,17 +104,20 @@ XImage *XGetImage (
_XGetScanlinePad(dpy, (int) rep.depth), 0);
planes = 1;
}

-   if (!image)
+   if (!image) {
Xfree(data);
+   goto unlock;
+   }
if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 ||
INT_MAX / image->height <= image->bytes_per_line ||
INT_MAX / planes <= image->height * image->bytes_per_line ||
nbytes < planes * image->height * image->bytes_per_line) {
XDestroyImage(image);
image = NULL;
}
+  unlock:
UnlockDisplay(dpy);
SyncHandle();
return (image);
 }


That should be effectively equivalent, just less change in indentation for
the lines in between.

-- 
-Alan Coopersmith-   alan.coopersm...@oracle.com
 Oracle Solaris Engineering - https://blogs.oracle.com/alanc
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver 2/3] xwayland: Add xwayland-config.h

[Adam Jackson]

From: Lyude Paul 

Just a small autogenerated header that will soon contain more then just
one macro.

Signed-off-by: Lyude Paul 
---
 configure.ac   |  7 +++
 hw/xwayland/xwayland.c | 10 ++
 hw/xwayland/xwayland.h |  2 +-
 include/meson.build|  7 +++
 include/xwayland-config.h.in   | 10 ++
 include/xwayland-config.h.meson.in |  8 
 6 files changed, 39 insertions(+), 5 deletions(-)
 create mode 100644 include/xwayland-config.h.in
 create mode 100644 include/xwayland-config.h.meson.in

diff --git a/configure.ac b/configure.ac
index f82c0a66a..e8dba70d8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -67,6 +67,8 @@ dnl xkb-config.h covers XKB for the Xorg and Xnest DDXs.
 AC_CONFIG_HEADERS(include/xkb-config.h)
 dnl xwin-config.h covers the XWin DDX.
 AC_CONFIG_HEADERS(include/xwin-config.h)
+dnl xwayland-config.h covers Xwayland.
+AC_CONFIG_HEADERS(include/xwayland-config.h)
 dnl version-config.h covers the version numbers so they can be bumped without
 dnl forcing an entire recompile.x
 AC_CONFIG_HEADERS(include/version-config.h)
@@ -2383,6 +2385,11 @@ if test "x$XWAYLAND" = xyes; then
AC_MSG_ERROR([Xwayland build explicitly requested, but required 
modules not found.])
fi
 
+   if test "x$GLAMOR" = xyes && test "x$GBM" = xyes; then
+   AC_DEFINE(XWL_HAS_GLAMOR, 1,
+ [Build xwayland with glamor support])
+   fi
+
XWAYLAND_LIBS="$FB_LIB $FIXES_LIB $MI_LIB $XEXT_LIB $DBE_LIB 
$RECORD_LIB $GLX_LIBS $RANDR_LIB $RENDER_LIB $DAMAGE_LIB $DRI3_LIB $PRESENT_LIB 
$MIEXT_SYNC_LIB $MIEXT_DAMAGE_LIB $MIEXT_SHADOW_LIB $XI_LIB $XKB_LIB 
$XKB_STUB_LIB $COMPOSITE_LIB $MAIN_LIB $DIX_LIB $OS_LIB"
XWAYLAND_SYS_LIBS="$XWAYLANDMODULES_LIBS $GLX_SYS_LIBS"
AC_SUBST([XWAYLAND_LIBS])
diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 8b8fbc1aa..941127cf0 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -647,7 +647,7 @@ xwl_window_post_damage(struct xwl_window *xwl_window)
 region = DamageRegion(xwl_window->damage);
 pixmap = (*xwl_screen->screen->GetWindowPixmap) (xwl_window->window);
 
-#ifdef GLAMOR_HAS_GBM
+#ifdef XWL_HAS_GLAMOR
 if (xwl_screen->glamor)
 buffer = xwl_glamor_pixmap_get_wl_buffer(pixmap);
 else
@@ -725,7 +725,7 @@ registry_global(void *data, struct wl_registry *registry, 
uint32_t id,
 wl_registry_bind(registry, id, _output_manager_v1_interface, 
1);
 xwl_screen_init_xdg_output(xwl_screen);
 }
-#ifdef GLAMOR_HAS_GBM
+#ifdef XWL_HAS_GLAMOR
 #if 0
 else if (xwl_screen->glamor &&
  strcmp(interface, "wl_drm") == 0 && version >= 2) {
@@ -919,7 +919,7 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
 dixSetPrivate(>devPrivates, _screen_private_key, xwl_screen);
 xwl_screen->screen = pScreen;
 
-#ifdef GLAMOR_HAS_GBM
+#ifdef XWL_HAS_GLAMOR
 xwl_screen->glamor = 1;
 #endif
 
@@ -947,12 +947,14 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
 }
 }
 
+#ifdef XWL_HAS_GLAMOR
 if (xwl_screen->glamor) {
 if (!xwl_glamor_init_gbm(xwl_screen)) {
 ErrorF("xwayland glamor: failed to setup GBM backend, falling back 
to sw accel\n");
 xwl_screen->glamor = 0;
 }
 }
+#endif
 
 /* In rootless mode, we don't have any screen storage, and the only
  * rendering should be to redirected mode. */
@@ -1036,7 +1038,7 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
 if (!xwl_screen_init_cursor(xwl_screen))
 return FALSE;
 
-#ifdef GLAMOR_HAS_GBM
+#ifdef XWL_HAS_GLAMOR
 if (xwl_screen->glamor && !xwl_glamor_init(xwl_screen)) {
 ErrorF("Failed to initialize glamor, falling back to sw\n");
 xwl_screen->glamor = 0;
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index 64c504373..bf29f31e1 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -26,7 +26,7 @@
 #ifndef XWAYLAND_H
 #define XWAYLAND_H
 
-#include 
+#include 
 
 #include 
 #include 
diff --git a/include/meson.build b/include/meson.build
index e6abf22f8..19acdca4f 100644
--- a/include/meson.build
+++ b/include/meson.build
@@ -306,6 +306,13 @@ configure_file(output : 'xwin-config.h',
input : 'xwin-config.h.meson.in',
configuration : xwin_data)
 
+xwayland_data = configuration_data()
+xwayland_data.set('XWL_HAS_GLAMOR', build_glamor and gbm_dep.found())
+
+configure_file(output : 'xwayland-config.h',
+   input : 'xwayland-config.h.meson.in',
+   configuration : xwayland_data)
+
 if build_xorg
 install_data(
 [
diff --git a/include/xwayland-config.h.in b/include/xwayland-config.h.in
new file mode 100644
index 0..333b53f23
--- /dev/null
+++ b/include/xwayland-config.h.in
@@ -0,0 +1,10 @@
+/* xwayland-config.h.in: not at all 

[PATCH xserver 3/3] xwayland: Add glamor egl_backend for EGLStreams

[Adam Jackson]

From: Lyude Paul 

This adds initial support for displaying Xwayland applications through
the use of EGLStreams and nvidia's custom wayland protocol by adding
another egl_backend driver. This also adds some additional egl_backend
hooks that are required to make things work properly.

EGLStreams work a lot differently then the traditional way of handling
buffers with wayland. Unfortunately, there are also a LOT of various
pitfalls baked into it's design that need to be explained.

This has a very large and unfortunate implication: direct rendering is,
for the time being at least, impossible to do through EGLStreams. The
main reason being that the EGLStream spec mandates that we lose the
entire color buffer contents with each eglSwapBuffers(), which goes
against X's requirement of not losing data with pixmaps.  no way to use
an allocated EGLSurface as the storage for glamor rendering like we do
with GBM, we have to rely on blitting each pixmap to it's respective
EGLSurface producer each frame. In order to pull this off, we add two
different additional egl_backend hooks that GBM opts out of
implementing:

- egl_backend.allow_commits for holding off displaying any EGLStream
  backed pixmaps until the point where it's stream is completely
  initialized and ready for use
- egl_backend.post_damage for blitting the content of the EGLStream
  surface producer before Xwayland actually damages and commits the
  wl_surface to the screen.

The other big pitfall here is that using nvidia's wayland-eglstreams
helper library is also not possible for the most part. All of it's API
for creating and destroying streams rely on being able to perform a
roundtrip in order to bring each stream to completion since the wayland
compositor must perform it's job of connecting a consumer to each
EGLstream. Because Xwayland has to potentially handle both responding to
the wayland compositor and it's own X clients, the situation of the
wayland compositor being one of our X clients must be considered. If we
perform a roundtrip with the Wayland compositor, it's possible that the
wayland compositor might currently be connected to us as an X client and
thus hang while both Xwayland and the wayland compositor await responses
from eachother. To avoid this, we work directly with the wayland
protocol and use wl_display_sync() events along with release() events to
set up and destroy EGLStreams asynchronously alongside handling X
clients.

Additionally, since setting up EGLStreams is not an atomic operation we
have to take into consideration the fact that an EGLStream can
potentially be created in response to a window resize, then immediately
deleted due to another pending window resize in the same X client's
pending reqests before Xwayland hits the part of it's event loop where
we read from the wayland compositor. To make this even more painful, we
also have to take into consideration that since EGLStreams are not
atomic that it's possible we could delete wayland resources for an
EGLStream before the compositor even finishes using them and thus run
into errors. So, we use quite a bit of tracking logic to keep EGLStream
objects alive until we know the compositor isn't using them (even if
this means the stream outlives the pixmap it backed).

While the default backend for glamor remains GBM, this patch exists for
users who have had to deal with the reprecussion of their GPU
manufacturers ignoring the advice of upstream and the standardization of
GBM across most major GPU manufacturers. It is not intended to be a
final solution to the GBM debate, but merely a baindaid so our users
don't have to suffer from the consequences of companies avoiding working
upstream. New drivers are strongly encouraged not to use this as a
backend, and use GBM like everyone else. We even spit this out as an
error from Xwayland when using the eglstream backend.

Signed-off-by: Lyude Paul 
---
 configure.ac|  24 +
 hw/xwayland/Makefile.am |  24 +-
 hw/xwayland/meson.build |  19 +-
 hw/xwayland/xwayland-glamor-eglstream.c | 826 
 hw/xwayland/xwayland-glamor-gbm.c   |   6 +-
 hw/xwayland/xwayland-glamor.c   | 111 -
 hw/xwayland/xwayland.c  |  34 ++
 hw/xwayland/xwayland.h  |  39 ++
 include/meson.build |   7 +-
 include/xwayland-config.h.in|   3 +
 include/xwayland-config.h.meson.in  |   3 +
 meson.build |  15 +
 meson_options.txt   |   2 +
 13 files changed, 1100 insertions(+), 13 deletions(-)
 create mode 100644 hw/xwayland/xwayland-glamor-eglstream.c

diff --git a/configure.ac b/configure.ac
index e8dba70d8..1d60429b9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -590,6 +590,7 @@ AC_ARG_ENABLE(xvfb,   
AS_HELP_STRING([--enable-xvfb], [Build Xvfb server
 AC_ARG_ENABLE(xnest, 

[PATCH xserver 0/3] EGLStreams for Xwayland, v2

[Adam Jackson]

This is a not-quite-mechanical rebase against master, mostly to cope
with the new DRI3 code. I'm not entirely sure I got those bits correct,
additional eyes would be appreciated (all in 1/3).

I do wonder, though, why this needs to be about streams at all. Why
can't the surface be from eglCreatePlatformWindowSurface? Everyone
supports EGL_KHR_platform_wayland, right?

- ajax

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver 1/3] xwayland: Decouple GBM from glamor

[Adam Jackson]

From: Lyude Paul 

This takes all of the gbm related code in wayland-glamor.c and moves it
into it's own EGL backend for Xwayland, xwayland-glamor-gbm.c.
Additionally, we add the egl_backend struct into xwl_screen in order to
provide hooks for alternative EGL backends such as nvidia's EGLStreams.

Signed-off-by: Lyude Paul 
---
 hw/xwayland/Makefile.am   |   3 +-
 hw/xwayland/meson.build   |   1 +
 hw/xwayland/xwayland-glamor-gbm.c | 887 ++
 hw/xwayland/xwayland-glamor.c | 751 +---
 hw/xwayland/xwayland.c|  13 +
 hw/xwayland/xwayland.h|  55 ++-
 6 files changed, 965 insertions(+), 745 deletions(-)
 create mode 100644 hw/xwayland/xwayland-glamor-gbm.c

diff --git a/hw/xwayland/Makefile.am b/hw/xwayland/Makefile.am
index f44a7ded3..14c90456a 100644
--- a/hw/xwayland/Makefile.am
+++ b/hw/xwayland/Makefile.am
@@ -34,7 +34,8 @@ Xwayland_built_sources =
 
 if GLAMOR_EGL
 Xwayland_SOURCES +=\
-   xwayland-glamor.c
+   xwayland-glamor.c   \
+   xwayland-glamor-gbm.c
 if XV
 Xwayland_SOURCES +=\
xwayland-glamor-xv.c
diff --git a/hw/xwayland/meson.build b/hw/xwayland/meson.build
index 7e24c5d63..dd696d456 100644
--- a/hw/xwayland/meson.build
+++ b/hw/xwayland/meson.build
@@ -46,6 +46,7 @@ srcs += code.process(dmabuf_xml)
 xwayland_glamor = []
 if gbm_dep.found()
 srcs += 'xwayland-glamor.c'
+srcs += 'xwayland-glamor-gbm.c'
 if build_xv
 srcs += 'xwayland-glamor-xv.c'
 endif
diff --git a/hw/xwayland/xwayland-glamor-gbm.c 
b/hw/xwayland/xwayland-glamor-gbm.c
new file mode 100644
index 0..618c218e9
--- /dev/null
+++ b/hw/xwayland/xwayland-glamor-gbm.c
@@ -0,0 +1,887 @@
+/*
+ * Copyright © 2011-2014 Intel Corporation
+ * Copyright © 2017 Red Hat Inc.
+ *
+ * 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.
+ *
+ * Authors:
+ *Lyude Paul 
+ *
+ */
+
+#include "xwayland.h"
+
+#include 
+#include 
+#include 
+#include 
+
+#define MESA_EGL_NO_X11_HEADERS
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include "drm-client-protocol.h"
+
+struct xwl_gbm_private {
+char *device_name;
+struct gbm_device *gbm;
+struct wl_drm *drm;
+struct zwp_linux_dmabuf_v1 *dmabuf;
+int drm_fd;
+int fd_render_node;
+Bool drm_authenticated;
+uint32_t capabilities;
+int dmabuf_capable;
+};
+
+struct xwl_pixmap {
+struct wl_buffer *buffer;
+EGLImage image;
+unsigned int texture;
+struct gbm_bo *bo;
+};
+
+static DevPrivateKeyRec xwl_gbm_private_key;
+static DevPrivateKeyRec xwl_auth_state_private_key;
+
+static inline struct xwl_gbm_private *
+xwl_gbm_get(struct xwl_screen *xwl_screen)
+{
+return dixLookupPrivate(_screen->screen->devPrivates,
+_gbm_private_key);
+}
+
+static uint32_t
+gbm_format_for_depth(int depth)
+{
+switch (depth) {
+case 16:
+return GBM_FORMAT_RGB565;
+case 24:
+return GBM_FORMAT_XRGB;
+default:
+ErrorF("unexpected depth: %d\n", depth);
+case 32:
+return GBM_FORMAT_ARGB;
+}
+}
+
+static uint32_t
+wl_drm_format_for_depth(int depth)
+{
+switch (depth) {
+case 15:
+return WL_DRM_FORMAT_XRGB1555;
+case 16:
+return WL_DRM_FORMAT_RGB565;
+case 24:
+return WL_DRM_FORMAT_XRGB;
+default:
+ErrorF("unexpected depth: %d\n", depth);
+case 32:
+return WL_DRM_FORMAT_ARGB;
+}
+}
+
+static char
+is_fd_render_node(int fd)
+{
+struct stat render;
+
+if (fstat(fd, ))
+return 0;
+if (!S_ISCHR(render.st_mode))
+return 0;
+if (render.st_rdev & 0x80)
+return 1;
+
+return 0;
+}
+
+static PixmapPtr

[PATCH xserver 2/5] Remove always true GLAMOR_HAS_DRM_* guards

[Emil Velikov]

From: Emil Velikov 

With earlier commit the required version was bumped to 2.4.89, thus the
guards always evaluate to true.

Fixes: e4e3447603b ("Add RandR leases with modesetting driver support
[v6]")
Cc: Keith Packard 
Cc: Daniel Stone 
Cc: Louis-Francis Ratté-Boulianne 
Signed-off-by: Emil Velikov 
---
Daniel, Louis,
Seems like the existing code has subtle bugs ... right?

Namely we have compile-time checks for atomic/modifiers where a runtime
ones must be used. See the XXX fragments in particular.

Note: apart from the usual "old build server, new runtime machine" this
suffers in the opposite direction "new build server + kernel not
supporting X at runtime"
---
 configure.ac | 11 ---
 glamor/glamor_egl.c  |  4 ---
 hw/xfree86/drivers/modesetting/driver.c  |  2 --
 hw/xfree86/drivers/modesetting/drmmode_display.c | 42 ++--
 hw/xfree86/drivers/modesetting/pageflip.c|  2 --
 hw/xfree86/drivers/modesetting/present.c |  5 ++-
 include/dix-config.h.in  |  9 -
 include/meson.build  |  6 
 8 files changed, 12 insertions(+), 69 deletions(-)

diff --git a/configure.ac b/configure.ac
index 14fe34995..e9a393856 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2107,17 +2107,6 @@ if test "x$GLAMOR" = xyes; then
AC_MSG_ERROR([Glamor for Xorg requires $LIBGBM])
fi
fi
-
-   PKG_CHECK_EXISTS(libdrm >= 2.4.62,
-[AC_DEFINE(GLAMOR_HAS_DRM_ATOMIC, 1, [libdrm supports 
atomic API])],
-[])
-   PKG_CHECK_EXISTS(libdrm >= 2.4.74,
-[AC_DEFINE(GLAMOR_HAS_DRM_NAME_FROM_FD_2, 1, [Have 
GLAMOR_HAS_DRM_NAME_FROM_FD_2])],
-[])
-
-   PKG_CHECK_EXISTS(libdrm >= 2.4.83,
-[AC_DEFINE(GLAMOR_HAS_DRM_MODIFIERS, 1, [Have 
GLAMOR_HAS_DRM_MODIFIERS])],
-[])
 fi
 AM_CONDITIONAL([GLAMOR_EGL], [test "x$GBM" = xyes])
 
diff --git a/glamor/glamor_egl.c b/glamor/glamor_egl.c
index 8389d5f29..4eb66cb9f 100644
--- a/glamor/glamor_egl.c
+++ b/glamor/glamor_egl.c
@@ -799,11 +799,7 @@ glamor_egl_screen_init(ScreenPtr screen, struct 
glamor_context *glamor_ctx)
 /* To do DRI3 device FD generation, we need to open a new fd
  * to the same device we were handed in originally.
  */
-#ifdef GLAMOR_HAS_DRM_NAME_FROM_FD_2
 glamor_egl->device_path = drmGetDeviceNameFromFd2(glamor_egl->fd);
-#else
-glamor_egl->device_path = drmGetDeviceNameFromFd(glamor_egl->fd);
-#endif
 
 if (!dri3_screen_init(screen, _dri3_info)) {
 xf86DrvMsg(scrn->scrnIndex, X_ERROR,
diff --git a/hw/xfree86/drivers/modesetting/driver.c 
b/hw/xfree86/drivers/modesetting/driver.c
index f20284bb0..2702a2eb2 100644
--- a/hw/xfree86/drivers/modesetting/driver.c
+++ b/hw/xfree86/drivers/modesetting/driver.c
@@ -1018,11 +1018,9 @@ PreInit(ScrnInfoPtr pScrn, int flags)
 }
 #endif
 
-#ifdef GLAMOR_HAS_DRM_ATOMIC
 ret = drmSetClientCap(ms->fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
 ret |= drmSetClientCap(ms->fd, DRM_CLIENT_CAP_ATOMIC, 1);
 ms->atomic_modeset = (ret == 0);
-#endif
 
 if (drmmode_pre_init(pScrn, >drmmode, pScrn->bitsPerPixel / 8) == 
FALSE) {
 xf86DrvMsg(pScrn->scrnIndex, X_ERROR, "KMS setup failed\n");
diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c 
b/hw/xfree86/drivers/modesetting/drmmode_display.c
index 1027e637a..e0a982ac6 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.c
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
@@ -58,8 +58,6 @@ static PixmapPtr drmmode_create_pixmap_header(ScreenPtr 
pScreen, int width, int
   int depth, int bitsPerPixel, int 
devKind,
   void *pPixData);
 
-#ifdef GLAMOR_HAS_DRM_MODIFIERS
-
 static inline uint32_t *
 formats_ptr(struct drm_format_modifier_blob *blob)
 {
@@ -72,8 +70,6 @@ modifiers_ptr(struct drm_format_modifier_blob *blob)
 return (struct drm_format_modifier *)(((char *)blob) + 
blob->modifiers_offset);
 }
 
-#endif
-
 Bool
 drmmode_is_format_supported(ScrnInfoPtr scrn, uint32_t format, uint64_t 
modifier)
 {
@@ -392,7 +388,6 @@ drmmode_prop_info_free(drmmode_prop_info_ptr info, int 
num_props)
 free(info[i].enum_values);
 }
 
-#ifdef GLAMOR_HAS_DRM_ATOMIC
 static int
 plane_add_prop(drmModeAtomicReq *req, drmmode_crtc_private_ptr drmmode_crtc,
enum drmmode_plane_property prop, uint64_t val)
@@ -480,7 +475,6 @@ drm_mode_destroy(xf86CrtcPtr crtc, drmmode_mode_ptr mode)
 xorg_list_del(>entry);
 free(mode);
 }
-#endif
 
 static void
 drmmode_ConvertToKMode(ScrnInfoPtr scrn,
@@ -502,7 +496,6 @@ 

[PATCH xserver 3/5] modesetting: remove always true defined(DRM_CAP_PRIME) guards

[Emil Velikov]

From: Emil Velikov 

The macro was available in libdrm for ages. Furthermore having a guard
like this is a very bad idea.

Building on an old server will result in a missing run-time functionality.
Since it's UABI one can use a local fallback, old kernels will return
-EINVAL and the fallback path will kick in.

Signed-off-by: Emil Velikov 
---
 hw/xfree86/drivers/modesetting/driver.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/xfree86/drivers/modesetting/driver.c 
b/hw/xfree86/drivers/modesetting/driver.c
index 2702a2eb2..8c5a2635f 100644
--- a/hw/xfree86/drivers/modesetting/driver.c
+++ b/hw/xfree86/drivers/modesetting/driver.c
@@ -227,7 +227,7 @@ check_outputs(int fd, int *count)
 *count = res->count_connectors;
 
 ret = res->count_connectors > 0;
-#if defined(DRM_CAP_PRIME) && defined(GLAMOR_HAS_GBM_LINEAR)
+#if defined(GLAMOR_HAS_GBM_LINEAR)
 if (ret == FALSE) {
 uint64_t value = 0;
 if (drmGetCap(fd, DRM_CAP_PRIME, ) == 0 &&
@@ -1003,7 +1003,6 @@ PreInit(ScrnInfoPtr pScrn, int flags)
 xf86ReturnOptValBool(ms->drmmode.Options, OPTION_PAGEFLIP, TRUE);
 
 pScrn->capabilities = 0;
-#ifdef DRM_CAP_PRIME
 ret = drmGetCap(ms->fd, DRM_CAP_PRIME, );
 if (ret == 0) {
 if (connector_count && (value & DRM_PRIME_CAP_IMPORT)) {
@@ -1016,7 +1015,6 @@ PreInit(ScrnInfoPtr pScrn, int flags)
 pScrn->capabilities |= RR_Capability_SourceOutput | 
RR_Capability_SourceOffload;
 #endif
 }
-#endif
 
 ret = drmSetClientCap(ms->fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
 ret |= drmSetClientCap(ms->fd, DRM_CLIENT_CAP_ATOMIC, 1);
-- 
2.16.0

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver 5/5] modesetting: remove fallback DRM_CAP_* defines

[Emil Velikov]

From: Emil Velikov 

All the macros are available in the libdrm that we depend on.

Signed-off-by: Emil Velikov 
---
 hw/xfree86/drivers/modesetting/driver.c  | 8 
 hw/xfree86/drivers/modesetting/drmmode_display.h | 7 ---
 2 files changed, 15 deletions(-)

diff --git a/hw/xfree86/drivers/modesetting/driver.c 
b/hw/xfree86/drivers/modesetting/driver.c
index 8c5a2635f..2f9e3ac5c 100644
--- a/hw/xfree86/drivers/modesetting/driver.c
+++ b/hw/xfree86/drivers/modesetting/driver.c
@@ -794,14 +794,6 @@ msShouldDoubleShadow(ScrnInfoPtr pScrn, modesettingPtr ms)
 return ret;
 }
 
-#ifndef DRM_CAP_CURSOR_WIDTH
-#define DRM_CAP_CURSOR_WIDTH 0x8
-#endif
-
-#ifndef DRM_CAP_CURSOR_HEIGHT
-#define DRM_CAP_CURSOR_HEIGHT 0x9
-#endif
-
 static Bool
 ms_get_drm_master_fd(ScrnInfoPtr pScrn)
 {
diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h 
b/hw/xfree86/drivers/modesetting/drmmode_display.h
index ee59711cb..d2dc7c225 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.h
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.h
@@ -285,11 +285,4 @@ void drmmode_copy_fb(ScrnInfoPtr pScrn, drmmode_ptr 
drmmode);
 int drmmode_crtc_set_fb(xf86CrtcPtr crtc, DisplayModePtr mode, uint32_t fb_id,
 int x, int y, uint32_t flags, void *data);
 
-#ifndef DRM_CAP_DUMB_PREFERRED_DEPTH
-#define DRM_CAP_DUMB_PREFERRED_DEPTH 3
-#endif
-#ifndef DRM_CAP_DUMB_PREFER_SHADOW
-#define DRM_CAP_DUMB_PREFER_SHADOW 4
-#endif
-
 #endif
-- 
2.16.0

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver 4/5] modesetting: remove always true DRM_IOCTL_CRTC_QUEUE_SEQUENCE guard

[Emil Velikov]

From: Emil Velikov 

We already require libdrm 2.4.89 which provides the definition plus
guarding kernel UABI like that is generally a bad idea.

See previous commit for details why :-)

Cc: Keith Packard 
Signed-off-by: Emil Velikov 
---
 hw/xfree86/drivers/modesetting/vblank.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/hw/xfree86/drivers/modesetting/vblank.c 
b/hw/xfree86/drivers/modesetting/vblank.c
index 1d331ccdb..ae3018b4b 100644
--- a/hw/xfree86/drivers/modesetting/vblank.c
+++ b/hw/xfree86/drivers/modesetting/vblank.c
@@ -182,7 +182,6 @@ ms_get_kernel_ust_msc(xf86CrtcPtr crtc,
 drmVBlank vbl;
 int ret;
 
-#ifdef DRM_IOCTL_CRTC_QUEUE_SEQUENCE
 if (ms->has_queue_sequence || !ms->tried_queue_sequence) {
 uint64_t ns;
 ms->tried_queue_sequence = TRUE;
@@ -196,7 +195,6 @@ ms_get_kernel_ust_msc(xf86CrtcPtr crtc,
 return ret == 0;
 }
 }
-#endif
 /* Get current count */
 vbl.request.type = DRM_VBLANK_RELATIVE | drmmode_crtc->vblank_pipe;
 vbl.request.sequence = 0;
@@ -226,7 +224,6 @@ ms_queue_vblank(xf86CrtcPtr crtc, ms_queue_flag flags,
 
 for (;;) {
 /* Queue an event at the specified sequence */
-#ifdef DRM_IOCTL_CRTC_QUEUE_SEQUENCE
 if (ms->has_queue_sequence || !ms->tried_queue_sequence) {
 uint32_t drm_flags = 0;
 uint64_t kernel;
@@ -255,7 +252,6 @@ ms_queue_vblank(xf86CrtcPtr crtc, ms_queue_flag flags,
 goto check;
 }
 }
-#endif
 vbl.request.type = DRM_VBLANK_EVENT | drmmode_crtc->vblank_pipe;
 if (flags & MS_QUEUE_RELATIVE)
 vbl.request.type |= DRM_VBLANK_RELATIVE;
@@ -273,9 +269,7 @@ ms_queue_vblank(xf86CrtcPtr crtc, ms_queue_flag flags,
 *msc_queued = ms_kernel_msc_to_crtc_msc(crtc, 
vbl.reply.sequence);
 return TRUE;
 }
-#ifdef DRM_IOCTL_CRTC_QUEUE_SEQUENCE
 check:
-#endif
 if (errno != EBUSY) {
 ms_drm_abort_seq(scrn, seq);
 return FALSE;
-- 
2.16.0

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver 1/5] configure: remove libdrm version check

[Emil Velikov]

From: Emil Velikov 

We already require said version.

Signed-off-by: Emil Velikov 
---
 configure.ac | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index f82c0a66a..14fe34995 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1999,8 +1999,7 @@ if test "x$XORG" = xyes; then
fi
 
if test "x$DRM" = xyes; then
-   dnl 2.4.46 is required for cursor hotspot support.
-   PKG_CHECK_EXISTS(libdrm >= 2.4.46, XORG_DRIVER_MODESETTING=yes, 
XORG_DRIVER_MODESETTING=no)
+   XORG_DRIVER_MODESETTING=yes
fi
 
AC_SUBST([XORG_LIBS])
-- 
2.16.0

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver 1/5] configure: remove libdrm version check

[Emil Velikov]

From: Emil Velikov 

We already require said version.

Signed-off-by: Emil Velikov 
---
 configure.ac | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index f82c0a66a..14fe34995 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1999,8 +1999,7 @@ if test "x$XORG" = xyes; then
fi
 
if test "x$DRM" = xyes; then
-   dnl 2.4.46 is required for cursor hotspot support.
-   PKG_CHECK_EXISTS(libdrm >= 2.4.46, XORG_DRIVER_MODESETTING=yes, 
XORG_DRIVER_MODESETTING=no)
+   XORG_DRIVER_MODESETTING=yes
fi
 
AC_SUBST([XORG_LIBS])
-- 
2.16.0

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH] dri2: Sync i965_pci_ids.h from Mesa.

[Rodrigo Vivi]

Copied from Mesa with no modifications.

Gives us Geminilake and Kaby Lake platform names updates and
sync on Coffee Lake PCI IDs.

Cc: Timo Aaltonen 
Signed-off-by: Rodrigo Vivi 
---
 hw/xfree86/dri2/pci_ids/i965_pci_ids.h | 26 +-
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/hw/xfree86/dri2/pci_ids/i965_pci_ids.h 
b/hw/xfree86/dri2/pci_ids/i965_pci_ids.h
index 57e70b7ae..feb9c582b 100644
--- a/hw/xfree86/dri2/pci_ids/i965_pci_ids.h
+++ b/hw/xfree86/dri2/pci_ids/i965_pci_ids.h
@@ -151,7 +151,7 @@ CHIPSET(0x590B, kbl_gt1, "Intel(R) Kabylake GT1")
 CHIPSET(0x590E, kbl_gt1, "Intel(R) Kabylake GT1")
 CHIPSET(0x5913, kbl_gt1_5, "Intel(R) Kabylake GT1.5")
 CHIPSET(0x5915, kbl_gt1_5, "Intel(R) Kabylake GT1.5")
-CHIPSET(0x5917, kbl_gt1_5, "Intel(R) Kabylake GT1.5")
+CHIPSET(0x5917, kbl_gt2, "Intel(R) UHD Graphics 620 (Kabylake GT2)")
 CHIPSET(0x5912, kbl_gt2, "Intel(R) HD Graphics 630 (Kaby Lake GT2)")
 CHIPSET(0x5916, kbl_gt2, "Intel(R) HD Graphics 620 (Kaby Lake GT2)")
 CHIPSET(0x591A, kbl_gt2, "Intel(R) HD Graphics P630 (Kaby Lake GT2)")
@@ -160,22 +160,30 @@ CHIPSET(0x591D, kbl_gt2, "Intel(R) HD Graphics P630 (Kaby 
Lake GT2)")
 CHIPSET(0x591E, kbl_gt2, "Intel(R) HD Graphics 615 (Kaby Lake GT2)")
 CHIPSET(0x5921, kbl_gt2, "Intel(R) Kabylake GT2F")
 CHIPSET(0x5923, kbl_gt3, "Intel(R) Kabylake GT3")
-CHIPSET(0x5926, kbl_gt3, "Intel(R) Iris Plus Graphics 640 (Kaby Lake GT3)")
-CHIPSET(0x5927, kbl_gt3, "Intel(R) Iris Plus Graphics 650 (Kaby Lake GT3)")
+CHIPSET(0x5926, kbl_gt3, "Intel(R) Iris Plus Graphics 640 (Kaby Lake GT3e)")
+CHIPSET(0x5927, kbl_gt3, "Intel(R) Iris Plus Graphics 650 (Kaby Lake GT3e)")
 CHIPSET(0x593B, kbl_gt4, "Intel(R) Kabylake GT4")
-CHIPSET(0x3184, glk, "Intel(R) HD Graphics (Geminilake)")
-CHIPSET(0x3185, glk_2x6, "Intel(R) HD Graphics (Geminilake 2x6)")
+CHIPSET(0x3184, glk, "Intel(R) UHD Graphics 605 (Geminilake)")
+CHIPSET(0x3185, glk_2x6, "Intel(R) UHD Graphics 600 (Geminilake 2x6)")
 CHIPSET(0x3E90, cfl_gt1, "Intel(R) HD Graphics (Coffeelake 2x6 GT1)")
 CHIPSET(0x3E93, cfl_gt1, "Intel(R) HD Graphics (Coffeelake 2x6 GT1)")
-CHIPSET(0x3E91, cfl_gt2, "Intel(R) HD Graphics (Coffeelake 3x8 GT2)")
-CHIPSET(0x3E92, cfl_gt2, "Intel(R) HD Graphics (Coffeelake 3x8 GT2)")
+CHIPSET(0x3E99, cfl_gt1, "Intel(R) HD Graphics (Coffeelake 2x6 GT1)")
+CHIPSET(0x3EA1, cfl_gt1, "Intel(R) HD Graphics (Coffeelake 2x6 GT1)")
+CHIPSET(0x3EA4, cfl_gt1, "Intel(R) HD Graphics (Coffeelake 2x6 GT1)")
+CHIPSET(0x3E91, cfl_gt2, "Intel(R) UHD Graphics 630 (Coffeelake 3x8 GT2)")
+CHIPSET(0x3E92, cfl_gt2, "Intel(R) UHD Graphics 630 (Coffeelake 3x8 GT2)")
 CHIPSET(0x3E96, cfl_gt2, "Intel(R) HD Graphics (Coffeelake 3x8 GT2)")
-CHIPSET(0x3E9B, cfl_gt2, "Intel(R) HD Graphics (Coffeelake 3x8 GT2)")
+CHIPSET(0x3E9A, cfl_gt2, "Intel(R) HD Graphics (Coffeelake 3x8 GT2)")
+CHIPSET(0x3E9B, cfl_gt2, "Intel(R) UHD Graphics 630 (Coffeelake 3x8 GT2)")
 CHIPSET(0x3E94, cfl_gt2, "Intel(R) HD Graphics (Coffeelake 3x8 GT2)")
+CHIPSET(0x3EA0, cfl_gt2, "Intel(R) HD Graphics (Coffeelake 3x8 GT2)")
+CHIPSET(0x3EA3, cfl_gt2, "Intel(R) HD Graphics (Coffeelake 3x8 GT2)")
+CHIPSET(0x3EA9, cfl_gt2, "Intel(R) HD Graphics (Coffeelake 3x8 GT2)")
+CHIPSET(0x3EA2, cfl_gt3, "Intel(R) HD Graphics (Coffeelake 3x8 GT3)")
+CHIPSET(0x3EA5, cfl_gt3, "Intel(R) HD Graphics (Coffeelake 3x8 GT3)")
 CHIPSET(0x3EA6, cfl_gt3, "Intel(R) HD Graphics (Coffeelake 3x8 GT3)")
 CHIPSET(0x3EA7, cfl_gt3, "Intel(R) HD Graphics (Coffeelake 3x8 GT3)")
 CHIPSET(0x3EA8, cfl_gt3, "Intel(R) HD Graphics (Coffeelake 3x8 GT3)")
-CHIPSET(0x3EA5, cfl_gt3, "Intel(R) HD Graphics (Coffeelake 3x8 GT3)")
 CHIPSET(0x5A49, cnl_2x8, "Intel(R) HD Graphics (Cannonlake 2x8 GT0.5)")
 CHIPSET(0x5A4A, cnl_2x8, "Intel(R) HD Graphics (Cannonlake 2x8 GT0.5)")
 CHIPSET(0x5A41, cnl_3x8, "Intel(R) HD Graphics (Cannonlake 3x8 GT1)")
-- 
2.13.6

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH:libX11] If XGetImage fails to create image, don't dereference it to bounds check

[Emil Velikov]

Hi Alan,

On 6 March 2018 at 21:47, Alan Coopersmith  wrote:
> Reported by gcc 7.3:
>
> GetImage.c:110:25: warning: potential null pointer dereference 
> [-Wnull-dereference]
>   if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 ||
> ~^~~~
>
> Introduced by 8ea762f94f4c942d898fdeb590a1630c83235c17 in Xlib 1.6.4
>
> Signed-off-by: Alan Coopersmith 
> ---
>  src/GetImage.c | 16 +---
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/src/GetImage.c b/src/GetImage.c
> index ff32d589..44a576a1 100644
> --- a/src/GetImage.c
> +++ b/src/GetImage.c
> @@ -105,14 +105,16 @@ XImage *XGetImage (
> planes = 1;
> }
>
> -   if (!image)
> +   if (!image) {
> Xfree(data);
> -   if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 ||
> -   INT_MAX / image->height <= image->bytes_per_line ||
> -   INT_MAX / planes <= image->height * image->bytes_per_line ||
> -   nbytes < planes * image->height * image->bytes_per_line) {
> -   XDestroyImage(image);
> -   image = NULL;
> +   } else {
> +if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 
> ||
> +INT_MAX / image->height <= image->bytes_per_line ||
> +INT_MAX / planes <= image->height * image->bytes_per_line ||
> +nbytes < planes * image->height * image->bytes_per_line) {
> +XDestroyImage(image);
> +image = NULL;
> +}

Instead of reshuffling, one could easily add the missing unlock/sync
in the error path.
Or even a goto unlock?

-Emil
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver v2 22/22] xwayland: Guard against very late vblanks

[Michel Dänzer]

On 2018-02-28 07:00 PM, Roman Gilg wrote:
> On Wed, Feb 28, 2018 at 6:43 PM, Michel Dänzer  wrote:
>> I'm unable to reproduce the issue you described with Steam (without this
>> patch series applied). I'd really like to know where the bogus target
>> MSC values you're seeing are coming from. What are the values of the
>> window_msc parameter and window_priv->msc_offset in
>> present_window_to_crtc_msc?
> 
> You don't have to go that far. I did read out on current master high
> target msc values directly for stuff->target_msc in
> proc_present_pixmap. But this happened only on like two or three
> occasions directly at the startup of the Steam client while hundreds
> of other present_pixmap requests before and after had normal
> stuff->target_msc values.
> 
> So just put a line
> 
> ErrorF("XXX %lu\n", stuff->target_msc);
> 
> in proc_present_pixmap and if you look carefully through the debug
> output there should be one or two very high numbers in comparison to
> all the other values.

As discussed on IRC, I've been unable to reproduce the issue you're
seeing yet, despite trying various things. At this point, the most
likely trigger of your symptoms is that in Mesa's
dri3_handle_present_event, the

 if (draw->recv_sbc > draw->send_sbc)
draw->recv_sbc -= 0x1;

is incorrectly hit, e.g. due to processing events from presentations of
another client/context. We'll need more information from you to validate
that hypothesis.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel