Re: [PATCH xserver v2] damage: Validate source pictures bound to windows before unwrapping

[Michel Dänzer]

On 09/02/17 03:06 AM, Adam Jackson wrote:
> On Tue, 2017-02-07 at 11:57 -0500, Alex Deucher wrote:
>> On Tue, Feb 7, 2017 at 3:38 AM, Michel Dänzer 
>> wrote:
>>> From: Michel Dänzer 
>>>
>>> The lower layers also do this, but no damage may be reported there,
>>> since we unwrap before calling down.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99220
>>> Signed-off-by: Michel Dänzer 
>>
>> Makes sense to me.
>> Reviewed-by: Alex Deucher 
> 
> Mmm. I might like the original patch better, I think I'd rather have
> composite be aware of how damage works than damage be aware of every
> possible way a caller might abuse it. So I reserve the right to flip
> this back the other way at some point in the future, if we ever get
> around to dix-level damage.

I'm not sure which caller you're referring to or how it's abusing
damage, but okay.


> But this does work, and I do give preference to patches that other
> people review instead of things I have to review as the last resort,
> so, merged:

Thanks!


-- 
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

[PATCH xserver] dmx: fix linking again by brute-forcing

[Mihail Konev]

On Ubuntu 17.04 daily, the following error is seen,
(disappears when librender.a is put before libos.a on the resulting
collect2 linker command line (seen by CFLAGS='-v')):

  ../../render/.libs/librender.a(glyph.o): In function `HashGlyph':
  /mnt/xserver/render/glyph.c:168: undefined reference to `x_sha1_init'
  /mnt/xserver/render/glyph.c:174: undefined reference to `x_sha1_update'
  /mnt/xserver/render/glyph.c:177: undefined reference to `x_sha1_update'
  /mnt/xserver/render/glyph.c:180: undefined reference to `x_sha1_final'
  collect2: error: ld returned 1 exit status
  Makefile:763: recipe for target 'Xdmx' failed

Align XDMX_LIBS to be "... RANDR RENDER ... PRESENT ...",
as in all other DDX-es; but this wouldn't help:

  ../../present/.libs/libpresent.a(present.o): In function 
`present_check_output_slaves_active':
  /mnt/xserver/present/present.c:126: undefined reference to 
`RRHasScanoutPixmap'
  collect2: error: ld returned 1 exit status

Then move PRESENT: "... PRESENT RANDR RENDER ...":

  ../../dix/.libs/libdix.a(pixmap.o): In function `PixmapStartDirtyTracking':
  /mnt/xserver/dix/pixmap.c:204: undefined reference to `RRTransformCompute'
  collect2: error: ld returned 1 exit status

Then align them as "... PRESENT ...LIBDIX... RANDR RENDER ...",
which somehow works.

Then moving PRESENT as "...LIBDIX... PRESENT RANDR RENDER ...",
or surrounding the original RANDR with RENDER or also PRESENT as
"...LIBDIX... PRESENT RANDR RENDER ..." would give the original
HashGlyph error.

Regressed-in: 3ef16dfb ("dmx: fix linking")
Suggested-by: Michel Dänzer 
Reported-by: Byeong-ryeol Kim 
Signed-off-by: Mihail Konev 
---
I was probably wrong in the previous message, as why "linking into"
would be different from listing a library multiple times.

On the other hand, looking at the above, it seems that saying
"randr depends on render" is less a guessing-game than finding
out where to put PRESENT. (Since the "nightmare").

That is, this patch, while smaller and more conservative,
might as well be risky, and is at least less explicit.
(I.e. wouldn't it be harder to figure out whether to reorder the libs,
or to list some the second time, then just to add a LIBADD,
should it break on another toolchain?)
(Also maybe it was not Ubuntu, but gcc 6.3.0).

 configure.ac   | 2 +-
 hw/dmx/Makefile.am | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/configure.ac b/configure.ac
index 4dcf8b5c27a0..ddd0a61c9d97 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2330,7 +2330,7 @@ if test "x$DMX" = xyes; then
fi
DMX_INCLUDES="$XEXT_INC $RENDER_INC $RECORD_INC"
XDMX_CFLAGS="$DMXMODULES_CFLAGS"
-   XDMX_LIBS="$FB_LIB $MI_LIB $XEXT_LIB $RENDER_LIB $RECORD_LIB $XI_LIB 
$XKB_LIB $XKB_STUB_LIB $DRI3_LIB $PRESENT_LIB $MIEXT_SYNC_LIB $MIEXT_SHADOW_LIB 
$MIEXT_DAMAGE_LIB $COMPOSITE_LIB $DAMAGE_LIB $MAIN_LIB $DIX_LIB $RANDR_LIB 
$CONFIG_LIB $OS_LIB $FIXES_LIB"
+   XDMX_LIBS="$FB_LIB $MI_LIB $XEXT_LIB $PRESENT_LIB $RECORD_LIB $XI_LIB 
$XKB_LIB $XKB_STUB_LIB $DRI3_LIB $MIEXT_SYNC_LIB $MIEXT_SHADOW_LIB 
$MIEXT_DAMAGE_LIB $COMPOSITE_LIB $DAMAGE_LIB $MAIN_LIB $DIX_LIB $CONFIG_LIB 
$RANDR_LIB $RENDER_LIB $OS_LIB $FIXES_LIB"
XDMX_SYS_LIBS="$DMXMODULES_LIBS"
AC_SUBST([XDMX_CFLAGS])
AC_SUBST([XDMX_LIBS])
diff --git a/hw/dmx/Makefile.am b/hw/dmx/Makefile.am
index 38d6ac409e76..eef84cb66a76 100644
--- a/hw/dmx/Makefile.am
+++ b/hw/dmx/Makefile.am
@@ -80,8 +80,7 @@ XDMX_LIBS = \
 
 Xdmx_LDFLAGS = $(LD_EXPORT_SYMBOLS_FLAG)
 Xdmx_DEPENDENCIES= $(XDMX_LIBS)
-Xdmx_LDADD = $(XDMX_LIBS) $(XDMX_SYS_LIBS) $(XSERVER_SYS_LIBS) \
- $(top_builddir)/render/librender.la
+Xdmx_LDADD = $(XDMX_LIBS) $(XDMX_SYS_LIBS) $(XSERVER_SYS_LIBS)
 
 relink:
$(AM_V_at)rm -f Xdmx$(EXEEXT) && $(MAKE) Xdmx$(EXEEXT)
-- 
2.9.2

___
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] randr,render: link some necessary symbols into

[Mihail Konev]

On Thu, Feb 09, 2017 at 10:48:14AM +0500, Mihail Konev wrote:
> this patch looks to be making more sense than per-OS adjustments

(In the sense it does record "A depends on B", whereas reordering does
not, while being a "per-OS-adjustment" too).
___
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] randr,render: link some necessary symbols into

[Mihail Konev]

Fixes dmx build, even on Ubuntu 17.04 alpha, where just adding
librender at the end of Xdmx doesn't work (librender HashGlyph then
cannot find x_sha1_init from libos).

This links some libos.a functions into render, and some librender.a
ones into randr.
Adjust the subdirectory order to allow for this.

"Linking into" seems to be the only reliable way to have two
subdirectory libraries reference symbols from each other - before,
it seems, Xserver just happened to have library order that allowed
linkers to find all the symbols in such cases; this broke
in e50da50118408a195d4d2e1b39817fe7c4447c56.

Somehow this does not result in multiple definition errors.

Fixes: 3ef16dfb ("dmx: fix linking")
Reported-by: Byeong-ryeol Kim 
Signed-off-by: Mihail Konev 
---

This requires 3ef16dfb to be reverted first.

Seems to be a sufficient "emulation" of libddx_Xdmx.la,
the to-be only library collecting all the _LIB-s to link into Xdmx,
whose purpose would be to avoid the subdirectory libs dependency
resolution nightmare (i.e. configure.ac/Makefile.am reorderings).
(I brute-forced it as well, but this patch looks to be making more
sense than per-OS adjustments).

 Makefile.am| 2 +-
 randr/Makefile.am  | 3 +++
 render/Makefile.am | 3 +++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Makefile.am b/Makefile.am
index f0fa2d839f7e..270c56103dff 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -45,8 +45,8 @@ SUBDIRS = \
Xext \
miext \
os \
+   render \
randr \
-   render  \
Xi \
xkb \
$(PSEUDORAMIX_DIR) \
diff --git a/randr/Makefile.am b/randr/Makefile.am
index 90dc9ec9aac4..94db11dc12cd 100644
--- a/randr/Makefile.am
+++ b/randr/Makefile.am
@@ -26,6 +26,9 @@ librandr_la_SOURCES = \
rrtransform.h   \
rrtransform.c
 
+librandr_la_DEPENDENCIES = $(top_builddir)/render/librender.la
+librandr_la_LIBADD = $(top_builddir)/render/librender.la
+
 if XINERAMA
 librandr_la_SOURCES += ${XINERAMA_SRCS}
 endif
diff --git a/render/Makefile.am b/render/Makefile.am
index d02028b3b749..f2b0944d51f1 100644
--- a/render/Makefile.am
+++ b/render/Makefile.am
@@ -15,6 +15,9 @@ librender_la_SOURCES =\
picture.c   \
render.c
 
+librender_la_DEPENDENCIES = $(OS_LIB)
+librender_la_LIBADD = $(OS_LIB)
+
 if XORG
 sdk_HEADERS = picture.h mipict.h glyphstr.h picturestr.h
 endif
-- 
2.9.2

___
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] xwayland: replace hardcoded function name with __func__ in error msg

[Peter Hutterer]

On Wed, Feb 08, 2017 at 02:46:16AM -0500, Olivier Fourdan wrote:
> Hey Peter,
> 
> > > LGTM - Yo get rid of two \n along the way, but I think there were not
> > > needed in the first place so:
> > 
> > oops. no, they're neeed so I added them back (and also to the instance where
> > it was missing). thanks
> 
> Are they really needed? I looked at ErrorF() in the source tree and there
> are plenty of cases where there is no \n at the end, so I looked at
> LogVMessageVerb() where ErrorF() should end up, and it seemed to me it
> would ad it if missing:
> 
> https://cgit.freedesktop.org/xorg/xserver/tree/os/log.c#n702

ah, true. Convenient and useful to know, thanks :)

Cheers,
   Peter
___
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 v5 xserver 7/6] tests: fix --disable-xorg build

[Mihail Konev]

On Mon, Jan 16, 2017 at 02:47:26PM -0500, Adam Jackson wrote:
> On Sat, 2017-01-14 at 15:19 +0500, Mihail Konev wrote:
> > Commit ead5064581665ff40c177dd1b447949f1420e209 missed that xi1/
> > and xi2/ were conditioned on XORG, and made xfree86-only tests to be
> > built unconditionally.
> > Ifdef the tests and split tests_SOURCES.
> > 
> > Commit 704a867f8fb7652a8b7d5569bbe44e188457db4e missed that when
> > XORG is false, libxservertest.la isn't linked into anything.
> > However, before putting them into tests_LDADD, its static libraries
> > likely need to be reordered for linking not to fail.
> > Remove the former libxservertest.la for !XORG, as its build was only
> > triggered by 'make check'.
> > XSERVER_LIBS were depending on it; remove them too.
> > 
> > Commit 23f6dbc96e47be6cbeed78cc9ef303987c3e29a9 missed that -wrap
> > arguments to 'ld' could only be present when HAVE_LD_WRAP is true.
> 
> While this does make --disabled-xorg fail differently, it does not work
> for me:
> 
> tests-tests.o: In function `main':
> /home/ajax/git/xserver/test/tests.c:12: undefined reference to `fixes_test'
> /home/ajax/git/xserver/test/tests.c:14: undefined reference to 
> `hashtabletest_test'
> /home/ajax/git/xserver/test/tests.c:16: undefined reference to `input_test'
> /home/ajax/git/xserver/test/tests.c:17: undefined reference to `misc_test'
> /home/ajax/git/xserver/test/tests.c:18: undefined reference to 
> `signal_logging_test'
> /home/ajax/git/xserver/test/tests.c:19: undefined reference to `touch_test'
> /home/ajax/git/xserver/test/tests.c:20: undefined reference to `xfree86_test'
> /home/ajax/git/xserver/test/tests.c:21: undefined reference to `xkb_test'
> /home/ajax/git/xserver/test/tests.c:22: undefined reference to `xtest_test'
> /home/ajax/git/xserver/test/tests.c:26: undefined reference to 
> `protocol_xchangedevicecontrol_test'
> /home/ajax/git/xserver/test/tests.c:28: undefined reference to 
> `protocol_xiqueryversion_test'
> /home/ajax/git/xserver/test/tests.c:29: undefined reference to 
> `protocol_xiquerydevice_test'
> /home/ajax/git/xserver/test/tests.c:30: undefined reference to 
> `protocol_xiselectevents_test'
> /home/ajax/git/xserver/test/tests.c:31: undefined reference to 
> `protocol_xigetselectedevents_test'
> /home/ajax/git/xserver/test/tests.c:32: undefined reference to 
> `protocol_xisetclientpointer_test'
> /home/ajax/git/xserver/test/tests.c:33: undefined reference to 
> `protocol_xigetclientpointer_test'
> /home/ajax/git/xserver/test/tests.c:34: undefined reference to 
> `protocol_xipassivegrabdevice_test'
> /home/ajax/git/xserver/test/tests.c:35: undefined reference to 
> `protocol_xiquerypointer_test'
> /home/ajax/git/xserver/test/tests.c:36: undefined reference to 
> `protocol_xiwarppointer_test'
> /home/ajax/git/xserver/test/tests.c:37: undefined reference to 
> `protocol_eventconvert_test'
> /home/ajax/git/xserver/test/tests.c:38: undefined reference to `xi2_test'
> collect2: error: ld returned 1 exit status
> 
> - ajax

This cannot happen on a clean build, at least for fixes_test():
(I should have sent this earlier, but grepping wouldn't come to mind).

  [xserver]$ git status
  HEAD detached at 730fd8c05f56
  nothing to commit, working tree clean
  [xserver]$

  [xserver]$ git show | head -n1
  commit 730fd8c05f56da21894691bbd2e7ff37f67b45f4
  [xserver]$

The only invocation of fixes_test is guarded by XORG_TESTS #define ..

  [xserver]$ grep '\bfixes_test\b' -r -C1
  Binary file test/tests-fixes.o matches
  --
  test/tests.c-#if XORG_TESTS
  test/tests.c:run_test(fixes_test);
  test/tests.c-#ifdef RES_TESTS
  --
  test/fixes.c-int
  test/fixes.c:fixes_test(void)
  test/fixes.c-{
  --
  test/tests.h-
  test/tests.h:int fixes_test(void);
  test/tests.h-int hashtabletest_test(void);
  [xserver]$

And the only XORG_TESTS definition is guarded by XORG automake define ..

  [xserver]$ grep 'XORG_TESTS\b' -r -C1
  test/Makefile.am-if XORG
  test/Makefile.am:AM_CPPFLAGS += -DXORG_TESTS
  test/Makefile.am-# Tests that require at least some DDX functions in order to 
fully link
  --
  test/tests.c-
  test/tests.c:#if XORG_TESTS
  test/tests.c-run_test(fixes_test);
  --
  test/Makefile.in-host_triplet = @host@
  test/Makefile.in:@ENABLE_UNIT_TESTS_TRUE@@XORG_TRUE@am__append_1 = 
-DXORG_TESTS
  test/Makefile.in-@ENABLE_UNIT_TESTS_TRUE@@RES_TRUE@@XORG_TRUE@am__append_2 = 
-DRES_TESTS
  --
  test/Makefile-host_triplet = x86_64-pc-linux-gnu
  test/Makefile:#am__append_1 = -DXORG_TESTS
  test/Makefile-#am__append_2 = -DRES_TESTS
  [xserver]$

Which is guarded by the XORG shell variable ..

  [xserver]$ grep '\[XORG\]' configure.ac
  AM_CONDITIONAL([XORG], [test "x$XORG" = xyes])
  [xserver]$

Which is 'yes' unless it is --disable-xorg, Windows, or Mac.

  [xserver]$ grep '\bXORG\b.*=' configure.ac -C0
  AC_ARG_ENABLE(xorg,   AS_HELP_STRING([--enable-xorg], [Build Xorg 
server (default: auto)]), [XORG=$enableval], [XORG=auto])
  --
  if test "x$XORG" = xauto; then
  

Re: [PATCH xserver 2/2 v4] xwayland: Apply output rotation for screen size

[Adam Jackson]

On Wed, 2017-02-08 at 13:20 -0500, Olivier Fourdan wrote:

> Yeah... It's all confusing! Sorry!
> 
> Those are the two patches:
> 
> https://patchwork.freedesktop.org/patch/137446/
> https://patchwork.freedesktop.org/patch/137635/

Merged:

remote: I: patch #137446 updated using rev 
afeace27d3818274b75d59375771dc964d2f56bb.
remote: I: patch #137635 updated using rev 
058809c43ec578a407cf40d4c3e54a42503e3562.
remote: I: 2 patch(es) updated to state Accepted.
To ssh://git.freedesktop.org/git/xorg/xserver
   38696ea..058809c  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 xserver 2/2 v4] xwayland: Apply output rotation for screen size

[Olivier Fourdan]

Hey Adam,

- Original Message -
> On Wed, 2017-02-08 at 09:23 +0100, Olivier Fourdan wrote:
> > Previously, we would swap the width/height of the Xwayland output based
> > on the output rotation, so that the overall screen size would match the
> > actual rotation of each output.
> 
> This series makes sense, but I'm a little lost on which versions of
> each patch are intended at this point. I assume it's this and v3 of
> 1/2?
> 
> (In general for short serieses I'd prefer a complete resend than trying
> to sort out revisions within the thread.)

Yeah... It's all confusing! Sorry!

Those are the two patches:

https://patchwork.freedesktop.org/patch/137446/
https://patchwork.freedesktop.org/patch/137635/

Cheers,
Olivier
___
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] damage: Validate source pictures bound to windows before unwrapping

[Alex Deucher]

On Wed, Feb 8, 2017 at 1:06 PM, Adam Jackson  wrote:
> On Tue, 2017-02-07 at 11:57 -0500, Alex Deucher wrote:
>> On Tue, Feb 7, 2017 at 3:38 AM, Michel Dänzer 
>> wrote:
>> > From: Michel Dänzer 
>> >
>> > The lower layers also do this, but no damage may be reported there,
>> > since we unwrap before calling down.
>> >
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99220
>> > Signed-off-by: Michel Dänzer 
>>
>> Makes sense to me.
>> Reviewed-by: Alex Deucher 
>
> Mmm. I might like the original patch better, I think I'd rather have
> composite be aware of how damage works than damage be aware of every
> possible way a caller might abuse it. So I reserve the right to flip
> this back the other way at some point in the future, if we ever get
> around to dix-level damage.

No objections from me.  I don't have a strong preference.

Alex

>
> But this does work, and I do give preference to patches that other
> people review instead of things I have to review as the last resort,
> so, merged:
>
> remote: I: patch #137290 updated using rev 
> 38696ea56854e055c31bd2730adfc7c39aa115b0.
> remote: I: 1 patch(es) updated to state Accepted.
> To ssh://git.freedesktop.org/git/xorg/xserver
>1c78bec..38696ea  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 xserver 2/2 v4] xwayland: Apply output rotation for screen size

[Adam Jackson]

On Wed, 2017-02-08 at 09:23 +0100, Olivier Fourdan wrote:
> Previously, we would swap the width/height of the Xwayland output based
> on the output rotation, so that the overall screen size would match the
> actual rotation of each output.

This series makes sense, but I'm a little lost on which versions of
each patch are intended at this point. I assume it's this and v3 of
1/2?

(In general for short serieses I'd prefer a complete resend than trying
to sort out revisions within the thread.)

- 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 xserver v2] damage: Validate source pictures bound to windows before unwrapping

[Adam Jackson]

On Tue, 2017-02-07 at 11:57 -0500, Alex Deucher wrote:
> On Tue, Feb 7, 2017 at 3:38 AM, Michel Dänzer 
> wrote:
> > From: Michel Dänzer 
> > 
> > The lower layers also do this, but no damage may be reported there,
> > since we unwrap before calling down.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99220
> > Signed-off-by: Michel Dänzer 
> 
> Makes sense to me.
> Reviewed-by: Alex Deucher 

Mmm. I might like the original patch better, I think I'd rather have
composite be aware of how damage works than damage be aware of every
possible way a caller might abuse it. So I reserve the right to flip
this back the other way at some point in the future, if we ever get
around to dix-level damage.

But this does work, and I do give preference to patches that other
people review instead of things I have to review as the last resort,
so, merged:

remote: I: patch #137290 updated using rev 
38696ea56854e055c31bd2730adfc7c39aa115b0.
remote: I: 1 patch(es) updated to state Accepted.
To ssh://git.freedesktop.org/git/xorg/xserver
   1c78bec..38696ea  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 xserver] dmx: fix linking

[Adam Jackson]

On Sat, 2017-02-04 at 00:03 +0500, Mihail Konev wrote:

> diff --git a/hw/dmx/Makefile.am b/hw/dmx/Makefile.am
> index eef84cb66a76..38d6ac409e76 100644
> --- a/hw/dmx/Makefile.am
> +++ b/hw/dmx/Makefile.am
> @@ -80,7 +80,8 @@ XDMX_LIBS = \
>  
>  Xdmx_LDFLAGS = $(LD_EXPORT_SYMBOLS_FLAG)
>  Xdmx_DEPENDENCIES= $(XDMX_LIBS)
> -Xdmx_LDADD = $(XDMX_LIBS) $(XDMX_SYS_LIBS) $(XSERVER_SYS_LIBS)
> +Xdmx_LDADD = $(XDMX_LIBS) $(XDMX_SYS_LIBS) $(XSERVER_SYS_LIBS) \
> + $(top_builddir)/render/librender.la
>  
>  relink:

Merged this, as it fixes things for me here. I think there's a better
solution to be had, since basically all DDXes at this point have the
same set of libraries, so all the BLAHSERVER_LIBS stuff in configure.ac
should stop varying per-ddx at some point. But we can get there
whenever.

- 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 xwayland] xwayland: Add hack for FWXGA resolution #99574

[Adam Jackson]

On Fri, 2017-02-03 at 02:41 -0500, Olivier Fourdan wrote:
> > For some applications (like fullscreen games) it matters for XRandr
> > resolution to be correctly set and equal to root window resolution.
> > 
> > In XServer there is already hack for this, adapted it for XWayland.
> > 
> > Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=99574
> > 
> > Signed-off-by: Svitozar Cherepii 
> Acked-by: Olivier Fourdan 

remote: I: patch #136706 updated using rev 
1c78bec9ca3cd1975a38bf5ebdba7dea65b309ab.
remote: I: 1 patch(es) updated to state Accepted.
To ssh://git.freedesktop.org/git/xorg/xserver
   542d9f6..1c78bec  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 xserver 0/2] present: Allow flipping with PRIME slave outputs

[Adam Jackson]

On Thu, 2017-02-02 at 15:56 -0500, Alex Deucher wrote:

> For the series:
> Reviewed-by: Alex Deucher 

remote: I: patch #136239 updated using rev 
b5b292896f647c85f03f53b20b2f03c0e94de428.
remote: I: patch #136238 updated using rev 
542d9f6807ac06b70f564ccab10af69fa21a1221.
remote: I: 2 patch(es) updated to state Accepted.
To ssh://git.freedesktop.org/git/xorg/xserver
   eb04b20..542d9f6  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 xserver] glamor: Paint first and last pixel of lines

[Adam Jackson]

On Wed, 2017-02-08 at 14:44 +0100, Max Staudt wrote:
> On 02/07/2017 09:06 PM, Keith Packard wrote:
> > > > Max Staudt  writes:
> > 
> > > OpenGL implementations are allowed to be imprecise in drawing line caps.
> > > This patch expands on the original workaround in dc9fa908.
> > 
> > Yeah, finding a way to work around GL differences seems like a good
> > idea. In this case, however, I think you're fixing some drivers and
> > breaking others -- when drawing with non-idempotent raster
> > ops.
> > 
> > Idempotent raster ops are those for which multiple draws generate the
> > same result, like GXcopy; hence non-idempotent operations are those
> > which do not, such as GXxor.
> 
> What does this mean in practice - is the manual pixel painting the
> problem you're seeing here, because it may draw over the previously
> drawn lines?
> 
> How does a GXor line draw operation currently work in GLAMOR? Maybe it
> would be easier to limit acceleration to GXcopy (which I suppose is by
> far the most useful case) and do that case as correct as possible?

On desktop GL, non-GXcopy rops work by setting glLogicOp appropriately.
 So on drivers that _do_ draw lines like the server's zero-width
implementation, the extra points your patch adds for the caps would be
drawn twice, and with GXxor that would mean they're painted and then
immediately reset.

I think, practically speaking, that glamor needs to match fb even along
the corner cases where the X spec allows driver-dependent behavior. The
simplest way to do this for this case would be to fall down to the mi
line code for the case of non-trivial cap style and rop that reads the
destination; if done correctly this will still be "accelerated" in that
we'll still hit glamor's SetSpans path.

A better, but more complicated, solution would be to use the fragment
shader to draw the line, where the GL primitive is just some quad that
happens to extend a bit past the boundaries of the X primitive so we
definitely hit every pixel we need to.

- 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: [xserver PULL master] modesetting HW cursor patches

[Adam Jackson]

On Thu, 2017-02-02 at 11:43 +0100, Michael Thayer wrote:
> Respectful but rather frustrated ping.  Is it still worth my trying to 
> get these changes in or should I just give up?

Sorry for the delay. The cursor code is that perfect horrifying
intersection of input and output that means I hate reviewing it. (To be
fair, apparently so does everybody else...)

Fixed up for ickle's input_lock change and merged:

remote: E: failed to find patch for rev 
c02f6a687c3d6bd0727322b055ee788f8fefa005.
remote: I: patch #113066 updated using rev 
ecd0a62323f26b333c49bddd7237dd5118482a35.
remote: I: patch #113067 updated using rev 
eb04b20160d706d4e0e67122d0adb1e723c0da92.
remote: I: 2 patch(es) updated to state Accepted.
To ssh://git.freedesktop.org/git/xorg/xserver
   3ef16df..eb04b20  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: [RFC PATCH xserver] modesetting: re-set the crtc's mode when link-status goes BAD

[Martin Peres]

On 06/02/17 17:50, Martin Peres wrote:

On 03/02/17 10:04, Daniel Vetter wrote:

On Fri, Feb 03, 2017 at 01:30:14AM +0200, Martin Peres wrote:

On 01/02/17 22:05, Manasi Navare wrote:

On Wed, Feb 01, 2017 at 11:58:16AM -0800, Eric Anholt wrote:

Jani Nikula  writes:


On Tue, 31 Jan 2017, Eric Anholt  wrote:

Martin Peres  writes:


Despite all the careful planing of the kernel, a link may become
insufficient to handle the currently-set mode. At this point, the
kernel should mark this particular configuration as being broken
and potentially prune the mode before setting the offending
connector's
link-status to BAD and send the userspace a hotplug event. This may
happen right after a modeset or later on.

When available, we should use the link-status information to reset
the wanted mode.

Signed-off-by: Martin Peres 

If I understand this right, there are two failure modes being
handled:

1) A mode that won't actually work because the link isn't good
enough.

2) A mode that should work, but link parameters were too
optimistic and
if we just ask the kernel to set the mode again it'll use more
conservative parameters that work.

This patch seems good for 2).  For 1), the drmmode_set_mode_major is
going to set our old mode back.  Won't the modeset then fail to link
train again, and bring us back into this loop?  The only escape
that I
see would be some other userspace responding to the advertised
mode list
changing, and then asking X to modeset to something new.

To avoid that failure busy loop, should we re-fetching modes at this
point, and only re-setting if our mode still exists?

Disclaimer: I don't know anything about the internals of the
modesetting
driver.

Perhaps we can identify the two cases now, but I'd put this more
generally: if the link status has gone bad, it's an indicator to
userspace that the circumstances may have changed, and userspace
should
query the kernel for the list of available modes again. It should no
longer trust information obtained prior to getting the bad link
status,
including the current mode.

But specifically, I think you're right, and AFAICT asking for the
list
of modes again is the only way for the userspace to distinguish
between
the two cases. I don't think there's a shortcut for deciding the
current
mode is still valid.

To avoid the busy-loop problem, I think I'd like this patch to
re-query
the kernel to ask about the current mode list, and only try to re-set
the mode if our mode is still there.

If the mode isn't there, then it's up to the DE to take action in
response to the notification of new modes.  If you don't have a DE to
take appropriate action, you're kind of out of luck.

As far as the ABI goes, this seems fine to me.  The only concern I had
about ABI was having to walk all the connectors on every uevent to see
if any had gone bad -- couldn't we have a flag of some sort about what
the uevent indicates?  But uevents should be super rare, so I'd say
the
kernel could go ahead with the current plan.

Yes I agree. The kernel sets the link status BAD as soona s link
training fails
but does not prune the modes until a new modelist is requested by
the userspace.
So this patch should re query the mode list as soon as it sees the
link status
BAD in order for the kernel to validate the modes again based on new
link
paarmeters and send a new mode list back.

Seems like a bad behaviour from the kernel, isn't it? It should return
immediatly
if the mode is gonna be pruned :s

The mode list pruning isn't relevant for modeesets, the kernel doesn't
validate requested modes against that. The mode list is purely for
userspace's information. Which means updating it only when userspace asks
for it is fine.


Hmm, ok. Fair enough!


I also thought some more about the loop when we're stuck on BAD, and I
think it shouldn't happen: When the link goes BAD we update the link
parameter values to the new limits, and the kernel will reject any mode
that won't fit anymore. Of course you might be unlucky, and the new link
limits are also not working, but eventually you're stuck at the "you're
link is broken, sry" stage, where the kernel rejects everything :-)

So I think the busy-loop has strictly a limited amount of time until it
runs out of steam.


OK, I have given it more thoughts and discussed with Ville and Chris IRL or
on IRC.

My current proposal is based on the idea that the kernel should reject a
mode
it knows it cannot set. This is already largely tested in real life: Try
setting 4K@60Hz on an HDMI 1.x monitor, it should immediately fail on
prepare(). For this proposal to work, we would need to put in the ABI
that a
driver that sets the link-status to BAD should also make sure that whatever
the userspace does, no infinite loop should be created (by changing the
maximum link characteristics before setting the link-status property).

Re-probing the list of modes and 

Re: [PATCH xserver] glamor: Paint first and last pixel of lines

[Max Staudt]

On 02/07/2017 09:06 PM, Keith Packard wrote:
> Max Staudt  writes:
> 
>> OpenGL implementations are allowed to be imprecise in drawing line caps.
>> This patch expands on the original workaround in dc9fa908.
> 
> Yeah, finding a way to work around GL differences seems like a good
> idea. In this case, however, I think you're fixing some drivers and
> breaking others -- when drawing with non-idempotent raster
> ops.
> 
> Idempotent raster ops are those for which multiple draws generate the
> same result, like GXcopy; hence non-idempotent operations are those
> which do not, such as GXxor.

What does this mean in practice - is the manual pixel painting the
problem you're seeing here, because it may draw over the previously
drawn lines?

How does a GXor line draw operation currently work in GLAMOR? Maybe it
would be easier to limit acceleration to GXcopy (which I suppose is by
far the most useful case) and do that case as correct as possible?


> I'm not sure what we should do here; there's no requirement in the
> protocol that we do anything at all as the server is allowed to draw
> pretty much whatever it wants for zero-width lines.

The X11 client developer who filed the openSUSE bug

  https://bugzilla.opensuse.org/show_bug.cgi?id=1021803

is confused because the software fallback and the GLAMOR version produce
different results. IMHO, since the Xorg server's software fallback
tends to be regarded as the reference X11 implementation, we should try
to emulate that as closely as possible.


I have to agree that this is a funny situation. Neither X11 nor GL are
rigidly specified, yet we're connecting them :)


> If you've found specific problems with Mesa drivers, I'd suggest we
> actually go fix those instead of working around them in the X
> server. The GL spec seems pretty clear in what it wants, and I think
> that aligns with what the X server currently expects.

I'm not sure, do you think this is a problem in the OpenGL drivers used?
It seems within the GL spec, so I'd say Mesa is not the right thing to
fix.

Given that there are plenty of binary drivers, and that we may end up
doing GLAMOR in an Xwayland session, I think we should aim for GLAMOR
being as precise as the fallback. Or disable the acceleration (we could
make it a configuration option). Most modern clients just display images
anyway, and the few remaining ones probably expect rather precise output
from Xorg as a rasterizer.

Consistency is important, as anything else can severely confuse users
and debugging developers alike - I think I've once seen that last line
pixel (which you use to draw the cap) being drawn on some backend. Now
that's a weird graphics glitch! And we can avoid it by setting the cap
pixel manually. Though as you say, only in case of GXcopy.


Max

___
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/2 v4] xwayland: Apply output rotation for screen size

[Olivier Fourdan]

Previously, we would swap the width/height of the Xwayland output based
on the output rotation, so that the overall screen size would match the
actual rotation of each output.

Problem is the RandR's ConstrainCursorHarder() handler will also apply
the output rotation, meaning that when the output is rotated, the
pointer will be constrained within the wrong dimension.

Moreover, XRandR assumes the original output width/height are unchanged
when the output is rotated, so by changing the Xwayland output width and
height based on rotation, Xwayland causes XRandr to report the wrong
output sizes (an output of size 1024x768 rotated left or right should
remain 1024x768, not 768x1024).

So to avoid this issue and keep things consistent between Wayland and
Xwayland outputs, leave the actual width/height unchanged but apply the
rotation when computing the screen size. This fixes both the output size
being wrong in "xrandr -q" and the pointer being constrained in the
wrong dimension with rotated with weston.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99663
Signed-off-by: Olivier Fourdan 
---
 v3: Split the patch in two as there two issues. The second issue being
 the pointer events not being reported when a rotation is applied
 this is because of the RR ConstrainCursorHarder() handler that we
 should not need in Xwayland.
 v4: Do not disable the ConstrainCursorHarder() handler set by
 RRScreenInit(), simply leave the oupout size unchanged and apply 
 the rotation only when needed to compute the overall screen size...

 hw/xwayland/xwayland-output.c | 28 
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
index bdf270a..7d6c7b0 100644
--- a/hw/xwayland/xwayland-output.c
+++ b/hw/xwayland/xwayland-output.c
@@ -108,14 +108,8 @@ output_handle_mode(void *data, struct wl_output 
*wl_output, uint32_t flags,
 if (!(flags & WL_OUTPUT_MODE_CURRENT))
 return;
 
-if (xwl_output->rotation & (RR_Rotate_0 | RR_Rotate_180)) {
-xwl_output->width = width;
-xwl_output->height = height;
-} else {
-xwl_output->width = height;
-xwl_output->height = width;
-}
-
+xwl_output->width = width;
+xwl_output->height = height;
 xwl_output->refresh = refresh;
 }
 
@@ -123,11 +117,21 @@ static inline void
 output_get_new_size(struct xwl_output *xwl_output,
 int *height, int *width)
 {
-if (*width < xwl_output->x + xwl_output->width)
-*width = xwl_output->x + xwl_output->width;
+int output_width, output_height;
+
+if (xwl_output->rotation & (RR_Rotate_0 | RR_Rotate_180)) {
+output_width = xwl_output->width;
+output_height = xwl_output->height;
+} else {
+output_width = xwl_output->height;
+output_height = xwl_output->width;
+}
+
+if (*width < xwl_output->x + output_width)
+*width = xwl_output->x + output_width;
 
-if (*height < xwl_output->y + xwl_output->height)
-*height = xwl_output->y + xwl_output->height;
+if (*height < xwl_output->y + output_height)
+*height = xwl_output->y + output_height;
 }
 
 /* Approximate some kind of mmpd (m.m. per dot) of the screen given the outputs
-- 
2.9.3

___
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