Re: [PATCH xserver 00/10] Implement RandR 1.6 (leases and non-desktop)

2018-01-23 Thread Keith Packard 
Adam Jackson  writes:

> 4 and 6 are:
>
> Reviewed-by: Adam Jackson 
>
> 3 and 8 would have my r-b too once the review feedback is addressed. 9
> and 10 I think we can expect new versions of anyway, it sounded on IRC
> like you'd found some bugs.

I've marked the reviewed bugs and merged fixes for the bugs found by
your suggested 'xlease' hack:

   v5: Terminate all leases on server reset.

Leases hang around after the associated client exits so that
the client doesn't need to occupy an X server client slot and
consume a file descriptor once it has gotten the output
resources necessary.

Any leases still hanging around when the X server resets or
shuts down need to be cleaned up by calling the kernel to
terminate the lease and freeing any DIX structures.

Note that we cannot simply use the existing
drmmode_terminate_lease function on each lease as that wants
to also reset the video mode, and during server shut down that

   modesetting: Validate leases on VT enter

The kernel doesn't allow any master ioctls to run when another
VT is active, including simple things like listing the active
leases. To deal with that, we check the list of leases
whenever the X server VT is activated.

   xfree86: hide disabled cursors when resetting after lease termination

The lessee may well have played with cursors and left one
active on our screen. Just tell the kernel to turn it off.

> For the actual 1.6 enablement I've been using this for a merged
> protocol repo:
>
> https://cgit.freedesktop.org/~ajax/xorgproto

That's awesome. It sounds like we should do a release of that (pre RandR
1.6) right away, and then a release of that with RandR 1.6 merged. Shall
I just go do that? We're not breaking anything that distros are using as
the old packages will hang around.

-- 
-keith


signature.asc
Description: PGP signature
___
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] xfree86: Remove broken RANDR disabling logic (v2)

2018-01-23 Thread Adam Jackson 
On Wed, 2018-01-24 at 08:12 +1100, Keith Packard wrote:
> Adam Jackson  writes:
> 
> > Yeah fair, this is a bit idiomatic. What this is saying is, if the
> > driver hasn't already initialized randr support for this screen -
> > meaning, if there's not already a randr screen private - then fill in
> > the randr hooks for the screen with the 1.1 compat code. 1.2+ drivers
> > will have called xf86CrtcScreenInit() in their ScreenInit hook before
> > this point.
> 
> Thanks for the extra comment. Do we have any drivers which don't support 1.2+?

Basically all the UMS drivers, yes. Most common among them being vesa,
fbdev, and wsfb, which tend to be what you fall down to if you don't
have KMS. The fallback support is written in terms of the ->SwitchMode
stuff in xfree86 that you needed anyway for initial setup and
xf86vidmode, but that means it's single-head only; for most of the
remaining UMS drivers multihead and hotplug were never really important
so that was emulation enough.

- 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 08/10] xf86-video-modesetting: Create CONNECTOR_ID properties for outputs

2018-01-23 Thread Keith Packard 
Adam Jackson  writes:

> I think I do like an enum better, even if only for the xrandr output
> formatting:
>
>   CONNECTOR_ID: 58 
>   range: (0, 2147483647)
>   non-desktop: 0 
>   supported: 0, 1
>
> If CONNECTOR_ID were an enum that'd just be "supported" with only one
> value, making it clear ahead of time that there's no point trying to
> change it.

Yeah, that does look better. Thanks!

CONNECTOR_ID: 59 
supported: 59

I've pushed an updated version of this patch to my drm-lease branch.

-- 
-keith


signature.asc
Description: PGP signature
___
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] xfree86: Remove broken RANDR disabling logic (v2)

2018-01-23 Thread Keith Packard 
Adam Jackson  writes:

> Yeah fair, this is a bit idiomatic. What this is saying is, if the
> driver hasn't already initialized randr support for this screen -
> meaning, if there's not already a randr screen private - then fill in
> the randr hooks for the screen with the 1.1 compat code. 1.2+ drivers
> will have called xf86CrtcScreenInit() in their ScreenInit hook before
> this point.

Thanks for the extra comment. Do we have any drivers which don't support 1.2+?

-- 
-keith


signature.asc
Description: PGP signature
___
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] xfree86: Remove broken RANDR disabling logic (v3)

2018-01-23 Thread Keith Packard 
Adam Jackson  writes:

> The only way to get at xf86Info.disableRandR from configuration is
> Option "RANDR" "foo" in ServerFlags, which probably nobody is using
> seeing as it's not documented. The other way it could be set is if a
> screen supports RANDR 1.2, in which case we set it to avoid trying to
> use the RANDR 1.1 compat code. If the second screen is not 1.2-aware
> then this would mean we don't do RANDR setup on the second screen at
> all, which would almost certainly crash the first time you try to do
> RANDR operations on the second screen.
>
> Fix that all by deletion, and just check whether the screen already has
> RANDR initialized before installing the stub support. If you want to
> disable RANDR, use the Extensions section of xorg.conf instead.
>
> v2: Also remove a now entirely pointless log message, telling you to
> ignore a line we will no longer print.
>
> v3: Explain the fallback path in InitOutput. (Keith Packard)
>
> Signed-off-by: Adam Jackson 

Looks good to me.

Reviewed-by: Keith Packard 

-- 
-keith


signature.asc
Description: PGP signature
___
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] xfree86: Remove broken RANDR disabling logic (v2)

2018-01-23 Thread Adam Jackson 
On Tue, 2018-01-23 at 08:42 +1100, Keith Packard wrote:
> Adam Jackson  writes:
> 
> > -if (!xf86Info.disableRandR)
> > +if (!rrGetScrPriv(xf86Screens[i]->pScreen))
> >  xf86RandRInit(screenInfo.screens[scr_index]);
> 
> A comment here would help me understand this change; I assume we won't
> generally hit this path, right?

Yeah fair, this is a bit idiomatic. What this is saying is, if the
driver hasn't already initialized randr support for this screen -
meaning, if there's not already a randr screen private - then fill in
the randr hooks for the screen with the 1.1 compat code. 1.2+ drivers
will have called xf86CrtcScreenInit() in their ScreenInit hook before
this point.

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

2018-01-23 Thread Adam Jackson 
On Tue, 2018-01-23 at 10:15 +, Daniel Stone wrote:

> Ooh. serialNumber == 1 means it's the root pixmap, which will actually
> be uselessly empty. It would be interesting to see how we've ended up
> here: it would have to be a top-level window which a) was manually
> redirected by the WM when it was created, b) had damage posted on it,
> and c) was unredirected (in that order). I can't think of how that
> would happen;

Map / draw / unmap without hitting BlockHandler? I think
xwl_unrealize_window() might be broken for that case:

/* ... */
wl_surface_destroy(xwl_window->surface);
if (RegionNotEmpty(DamageRegion(xwl_window->damage)))
xorg_list_del(&xwl_window->link_damage);
DamageUnregister(xwl_window->damage);
DamageDestroy(xwl_window->damage);
/* ... */

If (for whatever reason) the damage region wasn't empty, we'd never
unlink this window from the dirty list. Should probably just unlink it
unconditionally. If this is indeed what's happening, then the window
being updated in xwl_window_post_damage() would have ->mapped = 0, and
would be not the root window itself.

- 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] xfree86: Remove broken RANDR disabling logic (v3)

2018-01-23 Thread Adam Jackson 
The only way to get at xf86Info.disableRandR from configuration is
Option "RANDR" "foo" in ServerFlags, which probably nobody is using
seeing as it's not documented. The other way it could be set is if a
screen supports RANDR 1.2, in which case we set it to avoid trying to
use the RANDR 1.1 compat code. If the second screen is not 1.2-aware
then this would mean we don't do RANDR setup on the second screen at
all, which would almost certainly crash the first time you try to do
RANDR operations on the second screen.

Fix that all by deletion, and just check whether the screen already has
RANDR initialized before installing the stub support. If you want to
disable RANDR, use the Extensions section of xorg.conf instead.

v2: Also remove a now entirely pointless log message, telling you to
ignore a line we will no longer print.

v3: Explain the fallback path in InitOutput. (Keith Packard)

Signed-off-by: Adam Jackson 
---
 hw/xfree86/common/xf86.h|  2 --
 hw/xfree86/common/xf86Config.c  | 12 
 hw/xfree86/common/xf86Globals.c |  2 --
 hw/xfree86/common/xf86Helper.c  |  7 ---
 hw/xfree86/common/xf86Init.c|  9 ++---
 hw/xfree86/common/xf86Mode.c|  6 +-
 hw/xfree86/common/xf86Privstr.h |  2 --
 hw/xfree86/modes/xf86Crtc.c |  3 ---
 8 files changed, 7 insertions(+), 36 deletions(-)

diff --git a/hw/xfree86/common/xf86.h b/hw/xfree86/common/xf86.h
index 1c2546894..5743f0cd4 100644
--- a/hw/xfree86/common/xf86.h
+++ b/hw/xfree86/common/xf86.h
@@ -305,8 +305,6 @@ extern _X_EXPORT Bool
 xf86GetModInDevEnabled(void);
 extern _X_EXPORT Bool
 xf86GetAllowMouseOpenFail(void);
-extern _X_EXPORT void
-xf86DisableRandR(void);
 extern _X_EXPORT CARD32
 xorgGetVersion(void);
 extern _X_EXPORT CARD32
diff --git a/hw/xfree86/common/xf86Config.c b/hw/xfree86/common/xf86Config.c
index 053cac168..2f72c2f76 100644
--- a/hw/xfree86/common/xf86Config.c
+++ b/hw/xfree86/common/xf86Config.c
@@ -634,7 +634,6 @@ typedef enum {
 FLAG_XINERAMA,
 FLAG_LOG,
 FLAG_RENDER_COLORMAP_MODE,
-FLAG_RANDR,
 FLAG_IGNORE_ABI,
 FLAG_ALLOW_EMPTY_INPUT,
 FLAG_USE_DEFAULT_FONT_PATH,
@@ -683,8 +682,6 @@ static OptionInfoRec FlagOptions[] = {
  {0}, FALSE},
 {FLAG_RENDER_COLORMAP_MODE, "RenderColormapMode", OPTV_STRING,
  {0}, FALSE},
-{FLAG_RANDR, "RandR", OPTV_BOOLEAN,
- {0}, FALSE},
 {FLAG_IGNORE_ABI, "IgnoreABI", OPTV_BOOLEAN,
  {0}, FALSE},
 {FLAG_USE_DEFAULT_FONT_PATH, "UseDefaultFontPath", OPTV_BOOLEAN,
@@ -827,15 +824,6 @@ configServerFlags(XF86ConfFlagsPtr flagsconf, 
XF86OptionPtr layoutopts)
 }
 }
 
-#ifdef RANDR
-xf86Info.disableRandR = FALSE;
-xf86Info.randRFrom = X_DEFAULT;
-if (xf86GetOptValBool(FlagOptions, FLAG_RANDR, &value)) {
-xf86Info.disableRandR = !value;
-xf86Info.randRFrom = X_CONFIG;
-}
-#endif
-
 #ifdef GLXEXT
 xf86Info.glxVisuals = XF86_GlxVisualsTypical;
 xf86Info.glxVisualsFrom = X_DEFAULT;
diff --git a/hw/xfree86/common/xf86Globals.c b/hw/xfree86/common/xf86Globals.c
index 85efe3fc1..e890f05c2 100644
--- a/hw/xfree86/common/xf86Globals.c
+++ b/hw/xfree86/common/xf86Globals.c
@@ -117,8 +117,6 @@ xf86InfoRec xf86Info = {
 .miscModInDevEnabled = TRUE,
 .miscModInDevAllowNonLocal = FALSE,
 .pmFlag = TRUE,
-.disableRandR = FALSE,
-.randRFrom = X_DEFAULT,
 #if defined(CONFIG_HAL) || defined(CONFIG_UDEV) || defined(CONFIG_WSCONS)
 .forceInputDevices = FALSE,
 .autoAddDevices = TRUE,
diff --git a/hw/xfree86/common/xf86Helper.c b/hw/xfree86/common/xf86Helper.c
index 393a7aa88..95a90ad88 100644
--- a/hw/xfree86/common/xf86Helper.c
+++ b/hw/xfree86/common/xf86Helper.c
@@ -1458,13 +1458,6 @@ xf86GetAllowMouseOpenFail(void)
 return xf86Info.allowMouseOpenFail;
 }
 
-void
-xf86DisableRandR(void)
-{
-xf86Info.disableRandR = TRUE;
-xf86Info.randRFrom = X_PROBED;
-}
-
 CARD32
 xf86GetModuleVersion(void *module)
 {
diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c
index 57b38d07e..66dcd52fa 100644
--- a/hw/xfree86/common/xf86Init.c
+++ b/hw/xfree86/common/xf86Init.c
@@ -77,6 +77,7 @@
 #include "xf86Xinput.h"
 #include "xf86InPriv.h"
 #include "picturestr.h"
+#include "randrstr.h"
 
 #include "xf86Bus.h"
 #ifdef XSERVER_LIBPCIACCESS
@@ -810,10 +811,12 @@ InitOutput(ScreenInfo * pScreenInfo, int argc, char 
**argv)
 SubPixelUnknown);
 }
 #ifdef RANDR
-if (!xf86Info.disableRandR)
+/*
+ * If the driver hasn't set up its own RANDR support, install the
+ * fallback support.
+ */
+if (!rrGetScrPriv(xf86Screens[i]->pScreen))
 xf86RandRInit(screenInfo.screens[scr_index]);
-xf86Msg(xf86Info.randRFrom, "RandR %s\n",
-xf86Info.disableRandR ? "disabled" : "enabled");
 #endif
 }
 
diff --git a/hw/xfree86/common/xf86Mode.c b/hw/xfree86/common/xf86Mode.c
index ceba87f78..40d09a9f4 100644
--- a/hw/xfree86/common/xf86Mode

Re: Gradients are broken with glamor when RepeatReflect is set

2018-01-23 Thread Michel Dänzer 
On 2018-01-23 05:36 PM, Jeffrey Smith wrote:
> On Tue, Jan 23, 2018 at 10:27 AM, Michel Dänzer  wrote:
>> On 2018-01-23 04:26 PM, Chris Wilson wrote:
>>> Quoting Jeffrey Smith (2018-01-23 15:15:10)
 On Mon, Jan 22, 2018 at 3:01 PM, Chris Wilson  
 wrote:
> Quoting Adam Jackson (2018-01-22 20:09:52)
>> On Sat, 2017-12-23 at 19:26 +0100, Clemens Eisserer wrote:
>>> Hi there,
>>>
>>> Glamor's gradient acceleration code is broken in case RepeatReflect is
>>> used, please see: https://bugs.freedesktop.org/show_bug.cgi?id=98508
>>> I've filed the bug report over a year ago, but except for a
>>> confirmation from Michel Dänzer nothing happend.
>>>
>>> Unfourntunatly I lack the expertise to fix it myself - however instead
>>> of leaving it broken forever, could we fall back to software for
>>> RepeatReflect.
>>> I guess slow is better than completly broken?
>>
>> Just want to note that this isn't forgotten. I got as far as testing
>> the reproducer with Xephyr and verifying glamor was wrong and fb was
>> right, but don't yet get what the RepeatReflect math is getting wrong.
>> I'll definitely have a fix for 1.20 one way or another, but that may
>> just be forcing a fallback.
>>
>> If anyone wanted to investigate this, I think this is the guilty
>> conditional:
>>
>> https://cgit.freedesktop.org/xorg/xserver/tree/glamor/glamor_gradient.c#n296
>>
>> It's broken with llvmpipe/softpipe as well. Does it render correctly
>> with glamor on i965? If so, maybe it's a Gallium non-driver issue.
> 
> I haven't had a chance to do any testing with the testcase code.
> Is it possible to get images of the actual vs expected output?
> Perhaps attached to the bug report.

I attached images of the good and bad output to the bug report.


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

Re: Gradients are broken with glamor when RepeatReflect is set

2018-01-23 Thread Jeffrey Smith 
On Tue, Jan 23, 2018 at 10:27 AM, Michel Dänzer  wrote:
> On 2018-01-23 04:26 PM, Chris Wilson wrote:
>> Quoting Jeffrey Smith (2018-01-23 15:15:10)
>>> On Mon, Jan 22, 2018 at 3:01 PM, Chris Wilson  
>>> wrote:
 Quoting Adam Jackson (2018-01-22 20:09:52)
> On Sat, 2017-12-23 at 19:26 +0100, Clemens Eisserer wrote:
>> Hi there,
>>
>> Glamor's gradient acceleration code is broken in case RepeatReflect is
>> used, please see: https://bugs.freedesktop.org/show_bug.cgi?id=98508
>> I've filed the bug report over a year ago, but except for a
>> confirmation from Michel Dänzer nothing happend.
>>
>> Unfourntunatly I lack the expertise to fix it myself - however instead
>> of leaving it broken forever, could we fall back to software for
>> RepeatReflect.
>> I guess slow is better than completly broken?
>
> Just want to note that this isn't forgotten. I got as far as testing
> the reproducer with Xephyr and verifying glamor was wrong and fb was
> right, but don't yet get what the RepeatReflect math is getting wrong.
> I'll definitely have a fix for 1.20 one way or another, but that may
> just be forcing a fallback.
>
> If anyone wanted to investigate this, I think this is the guilty
> conditional:
>
> https://cgit.freedesktop.org/xorg/xserver/tree/glamor/glamor_gradient.c#n296
>
> It's broken with llvmpipe/softpipe as well. Does it render correctly
> with glamor on i965? If so, maybe it's a Gallium non-driver issue.
>
>
> --
> Earthling Michel Dänzer   |   http://www.amd.com
> Libre software enthusiast | Mesa and X developer

I haven't had a chance to do any testing with the testcase code.
Is it possible to get images of the actual vs expected output?
Perhaps attached to the bug report.

Just looking at the testcase code, it appears that a linear gradient
is being used.
But Adam referenced the radial gradient shader template.

-- Jeff
___
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: Gradients are broken with glamor when RepeatReflect is set

2018-01-23 Thread Michel Dänzer 
On 2018-01-23 04:26 PM, Chris Wilson wrote:
> Quoting Jeffrey Smith (2018-01-23 15:15:10)
>> On Mon, Jan 22, 2018 at 3:01 PM, Chris Wilson  
>> wrote:
>>> Quoting Adam Jackson (2018-01-22 20:09:52)
 On Sat, 2017-12-23 at 19:26 +0100, Clemens Eisserer wrote:
> Hi there,
>
> Glamor's gradient acceleration code is broken in case RepeatReflect is
> used, please see: https://bugs.freedesktop.org/show_bug.cgi?id=98508
> I've filed the bug report over a year ago, but except for a
> confirmation from Michel Dänzer nothing happend.
>
> Unfourntunatly I lack the expertise to fix it myself - however instead
> of leaving it broken forever, could we fall back to software for
> RepeatReflect.
> I guess slow is better than completly broken?

 Just want to note that this isn't forgotten. I got as far as testing
 the reproducer with Xephyr and verifying glamor was wrong and fb was
 right, but don't yet get what the RepeatReflect math is getting wrong.
 I'll definitely have a fix for 1.20 one way or another, but that may
 just be forcing a fallback.

 If anyone wanted to investigate this, I think this is the guilty
 conditional:

 https://cgit.freedesktop.org/xorg/xserver/tree/glamor/glamor_gradient.c#n296
>>>
>>> -t = abs(fract(t * 0.5 + 0.5) * 2.0 - 1.0);
>>> +t = abs(fract(abs(t) * 0.5 + 0.5) * 2.0 - 1.0);
>>
>> Chris, where did this definition for fract come from?
> 
> Naivety.
> 
>> For negative numbers, it does not match the OpenGL definition for
>> fract, which would be
>> #define fract(t) ((t) - floor(t))
>> With fract defined thus, the original transformation of t appears to work 
>> fine.
> 
> nir also uses the same definition:
> operation("fract", 1, source_types=real_types, c_expression={'f':
> "{src0} - floorf({src0})", 'd': "{src0} - floor({src0})"}),
> 
> So maybe it's purely an amdgpu compiler issue? Michel?

It's broken with llvmpipe/softpipe as well. Does it render correctly
with glamor on i965? If so, maybe it's a Gallium non-driver issue.


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

Re: [PATCH xserver 08/10] xf86-video-modesetting: Create CONNECTOR_ID properties for outputs

2018-01-23 Thread Adam Jackson 
On Tue, 2018-01-23 at 11:32 +1100, Keith Packard wrote:
> Adam Jackson  writes:
> 
> > > +if (name != BAD_RESOURCE) {
> > > +err = RRConfigureOutputProperty(output->randr_output, name,
> > > +FALSE, TRUE, TRUE,
> > > +2, prop_range);
> > 
> > There's no good reason to make this a range, is there?
> 
> Well, it's a fixed integer for each output, which can either be
> considered an enumeration which can have only one value or a value in a
> range. I picked 'range' because it didn't seem like this could be
> considered an enumeration to me.
> 
> If you like the other option better, that'd be fine with me :-)

I think I do like an enum better, even if only for the xrandr output
formatting:

CONNECTOR_ID: 58 
range: (0, 2147483647)
non-desktop: 0 
supported: 0, 1

If CONNECTOR_ID were an enum that'd just be "supported" with only one
value, making it clear ahead of time that there's no point trying to
change it.

- 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: Gradients are broken with glamor when RepeatReflect is set

2018-01-23 Thread Chris Wilson 
Quoting Chris Wilson (2018-01-23 15:26:50)
> Quoting Jeffrey Smith (2018-01-23 15:15:10)
> > On Mon, Jan 22, 2018 at 3:01 PM, Chris Wilson  
> > wrote:
> > > Quoting Adam Jackson (2018-01-22 20:09:52)
> > >> On Sat, 2017-12-23 at 19:26 +0100, Clemens Eisserer wrote:
> > >> > Hi there,
> > >> >
> > >> > Glamor's gradient acceleration code is broken in case RepeatReflect is
> > >> > used, please see: https://bugs.freedesktop.org/show_bug.cgi?id=98508
> > >> > I've filed the bug report over a year ago, but except for a
> > >> > confirmation from Michel Dänzer nothing happend.
> > >> >
> > >> > Unfourntunatly I lack the expertise to fix it myself - however instead
> > >> > of leaving it broken forever, could we fall back to software for
> > >> > RepeatReflect.
> > >> > I guess slow is better than completly broken?
> > >>
> > >> Just want to note that this isn't forgotten. I got as far as testing
> > >> the reproducer with Xephyr and verifying glamor was wrong and fb was
> > >> right, but don't yet get what the RepeatReflect math is getting wrong.
> > >> I'll definitely have a fix for 1.20 one way or another, but that may
> > >> just be forcing a fallback.
> > >>
> > >> If anyone wanted to investigate this, I think this is the guilty
> > >> conditional:
> > >>
> > >> https://cgit.freedesktop.org/xorg/xserver/tree/glamor/glamor_gradient.c#n296
> > >
> > > -t = abs(fract(t * 0.5 + 0.5) * 2.0 - 1.0);
> > > +t = abs(fract(abs(t) * 0.5 + 0.5) * 2.0 - 1.0);
> > 
> > Chris, where did this definition for fract come from?
> 
> Naivety.
> 
> > For negative numbers, it does not match the OpenGL definition for
> > fract, which would be
> > #define fract(t) ((t) - floor(t))
> > With fract defined thus, the original transformation of t appears to work 
> > fine.
> 
> nir also uses the same definition:
> operation("fract", 1, source_types=real_types, c_expression={'f':
> "{src0} - floorf({src0})", 'd': "{src0} - floor({src0})"}),
> 
> So maybe it's purely an amdgpu compiler issue? Michel?

static LLVMValueRef emit_ffract(struct ac_llvm_context *ctx,
LLVMValueRef src0)
{
const char *intr = "llvm.floor.f32";
LLVMValueRef fsrc0 = ac_to_float(ctx, src0);
LLVMValueRef params[] = {
fsrc0,
};
LLVMValueRef floor = ac_build_intrinsic(ctx, intr,
ctx->f32, params, 1,
AC_FUNC_ATTR_READNONE);
return LLVMBuildFSub(ctx->builder, fsrc0, floor, "");
}

which looks like it should be correct?
-Chris
___
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: Gradients are broken with glamor when RepeatReflect is set

2018-01-23 Thread Chris Wilson 
Quoting Jeffrey Smith (2018-01-23 15:15:10)
> On Mon, Jan 22, 2018 at 3:01 PM, Chris Wilson  
> wrote:
> > Quoting Adam Jackson (2018-01-22 20:09:52)
> >> On Sat, 2017-12-23 at 19:26 +0100, Clemens Eisserer wrote:
> >> > Hi there,
> >> >
> >> > Glamor's gradient acceleration code is broken in case RepeatReflect is
> >> > used, please see: https://bugs.freedesktop.org/show_bug.cgi?id=98508
> >> > I've filed the bug report over a year ago, but except for a
> >> > confirmation from Michel Dänzer nothing happend.
> >> >
> >> > Unfourntunatly I lack the expertise to fix it myself - however instead
> >> > of leaving it broken forever, could we fall back to software for
> >> > RepeatReflect.
> >> > I guess slow is better than completly broken?
> >>
> >> Just want to note that this isn't forgotten. I got as far as testing
> >> the reproducer with Xephyr and verifying glamor was wrong and fb was
> >> right, but don't yet get what the RepeatReflect math is getting wrong.
> >> I'll definitely have a fix for 1.20 one way or another, but that may
> >> just be forcing a fallback.
> >>
> >> If anyone wanted to investigate this, I think this is the guilty
> >> conditional:
> >>
> >> https://cgit.freedesktop.org/xorg/xserver/tree/glamor/glamor_gradient.c#n296
> >
> > -t = abs(fract(t * 0.5 + 0.5) * 2.0 - 1.0);
> > +t = abs(fract(abs(t) * 0.5 + 0.5) * 2.0 - 1.0);
> 
> Chris, where did this definition for fract come from?

Naivety.

> For negative numbers, it does not match the OpenGL definition for
> fract, which would be
> #define fract(t) ((t) - floor(t))
> With fract defined thus, the original transformation of t appears to work 
> fine.

nir also uses the same definition:
operation("fract", 1, source_types=real_types, c_expression={'f':
"{src0} - floorf({src0})", 'd': "{src0} - floor({src0})"}),

So maybe it's purely an amdgpu compiler issue? Michel?
-Chris
___
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: Gradients are broken with glamor when RepeatReflect is set

2018-01-23 Thread Jeffrey Smith 
On Mon, Jan 22, 2018 at 3:01 PM, Chris Wilson  wrote:
> Quoting Adam Jackson (2018-01-22 20:09:52)
>> On Sat, 2017-12-23 at 19:26 +0100, Clemens Eisserer wrote:
>> > Hi there,
>> >
>> > Glamor's gradient acceleration code is broken in case RepeatReflect is
>> > used, please see: https://bugs.freedesktop.org/show_bug.cgi?id=98508
>> > I've filed the bug report over a year ago, but except for a
>> > confirmation from Michel Dänzer nothing happend.
>> >
>> > Unfourntunatly I lack the expertise to fix it myself - however instead
>> > of leaving it broken forever, could we fall back to software for
>> > RepeatReflect.
>> > I guess slow is better than completly broken?
>>
>> Just want to note that this isn't forgotten. I got as far as testing
>> the reproducer with Xephyr and verifying glamor was wrong and fb was
>> right, but don't yet get what the RepeatReflect math is getting wrong.
>> I'll definitely have a fix for 1.20 one way or another, but that may
>> just be forcing a fallback.
>>
>> If anyone wanted to investigate this, I think this is the guilty
>> conditional:
>>
>> https://cgit.freedesktop.org/xorg/xserver/tree/glamor/glamor_gradient.c#n296
>
> -t = abs(fract(t * 0.5 + 0.5) * 2.0 - 1.0);
> +t = abs(fract(abs(t) * 0.5 + 0.5) * 2.0 - 1.0);

Chris, where did this definition for fract come from?

For negative numbers, it does not match the OpenGL definition for
fract, which would be
#define fract(t) ((t) - floor(t))
With fract defined thus, the original transformation of t appears to work fine.

>
> #include 
> #include 
>
> #define abs(t) fabs(t)
> #define fract(t) fmod((t), 1.0)
>
> static float repeat(float t)
> {
> return abs(fract(abs(t) * 0.5 + 0.5) * 2.0 - 1.0);
> }
>
> int main(void)
> {
> float t;
>
> for (t = -3; t <= 3; t += .5)
> printf("%5.1f  ", t);
> printf("\n");
> for (t = -3; t <= 3; t += .5)
> printf("%5.1f  ", repeat(t));
> printf("\n");
> }
>
>  -3.0   -2.5   -2.0   -1.5   -1.0   -0.50.00.51.01.52.0   
>  2.53.0
>   1.00.50.00.51.00.50.00.51.00.50.0   
>  0.51.0
> -Chris
> ___
> 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
___
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: avoid a crash with empty window pixmaps

2018-01-23 Thread Olivier Fourdan 
Hi Daniel,

On Tue, Jan 23, 2018 at 11:15 AM, Daniel Stone  wrote:

> Ooh. serialNumber == 1 means it's the root pixmap, which will actually
> be uselessly empty. It would be interesting to see how we've ended up
> here: it would have to be a top-level window which a) was manually
> redirected by the WM when it was created, b) had damage posted on it,
> and c) was unredirected (in that order). I can't think of how that
> would happen; maybe you could place logs for the triggers (e.g.
> removing the last manual redirect on a window) somewhere?
>

Unfortunately I don't know how to reproduce that one, all I have is a
couple of core files...

But yes, that window is a toplevel window (hexchat):

Walking down the properties list until we get to the WM_CLASS, it gives
“hexchat”.

(gdb) x /s
xwl_window->window->optional->userProps->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->data
0x215bcf0:"hexchat"

with

(gdb) p
nodeTable[xwl_window->window->optional->userProps->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->next->propertyName]->string
$1 = 0x5b2c2c "WM_CLASS"

But the theory of the toplevel being created → redirected → damaged →
unredirected is plausible because I suspect this happens concurrently with
a crash in the Wayland compositor (gnome-shell) at startup while the apps
are restored via x-session management (from what I understood from the
issue).

(which, if my understanding is correct, means that the user session is
doomed anyway, so this crash in Xwayland might not be the culprit, but
still, I'd rather have Xwayland end with a “failed to read Wayland events”
rather than a segfault)

It would be good to see what the WindowRec under xwl_window looks
> like; maybe that could offer us a clue.
>

The xwl_window->window gives:

(gdb) p *xwl_window->window $2 = {drawable = {type = 0 '\000',
   class = 1 '\001',
   depth = 24 '\030',
   bitsPerPixel = 32 ' ',
   id = 27262979,
   x = 3407,
   y = 574,
   width = 665,
   height = 400,
   pScreen = 0x161d200,
   serialNumber = 11075},
   devPrivates = 0x215b998,
   parent = 0x1e5bff0,
   nextSib = 0x230c490,
   prevSib = 0x0,
   firstChild = 0x215bd60,
   lastChild = 0x215bd60,
   clipList = {extents = {x1 = 0, y1 = 0, x2 = 0, y2 = 0},
   data = 0x8137a0 },
   borderClip = {extents = {x1 = 0, y1 = 0, x2 = 0, y2 = 0},
 data = 0x8137a0 },
   valdata = 0x0,
   winSize = {extents = {x1 = 3407, y1 = 574, x2 = 4072, y2 = 974},
  data = 0x0},
   borderSize = {extents = {x1 = 3407, y1 = 574, x2 = 4072, y2 = 974},
 data = 0x0},
   origin = {x = 3407, y = 574},
   borderWidth = 0,
   deliverableEvents = 32895,
   eventMask = 6520959,
   background = {pixmap = 0xe8e8e7, pixel = 15263975},
   border = {pixmap = 0x0, pixel = 0},
   optional = 0x215bb10,
   backgroundState = 2,
   borderIsPixel = 1,
   cursorIsNone = 1,
   backingStore = 0,
   backStorage = 0,
   saveUnder = 0,
   bitGravity = 1,
   winGravity = 1,
   overrideRedirect = 0,
   visibility = 2, (VisibilityFullyObscured)
   mapped = 1,
   realized = 1,
   viewable = 1,
   dontPropagate = 0,
   forcedBS = 0,
   redirectDraw = 0,
   forcedBG = 0,
   unhittable = 0,
   damagedDescendants = 0,
   inhibitBGPaint = 0}

I didn't spot anything suspicious about it.

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 03/10] xfree86/modes: Check for non-desktop monitors during PreInit

2018-01-23 Thread Keith Packard 
"Keith Packard"  writes:

>> Can we also bump XF86_CRTC_VERSION, so out-of-tree drivers can know when
>> this field is available?
>
> Yes, will do.

I've pushed a new version of my drm-lease branch that has just this
change; it's not very complicated:

@@ -247,7 +247,7 @@ typedef struct _xf86CrtcFuncs {
 
 } xf86CrtcFuncsRec, *xf86CrtcFuncsPtr;
 
-#define XF86_CRTC_VERSION 7
+#define XF86_CRTC_VERSION 8
 
 struct _xf86Crtc {
 /**


-- 
-keith


signature.asc
Description: PGP signature
___
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: avoid a crash with empty window pixmaps

2018-01-23 Thread Daniel Stone 
Hi,

On 23 January 2018 at 09:42, Olivier Fourdan  wrote:
> On 22 January 2018 at 19:57, Adam Jackson  wrote:
>> That can't really be the problem. X drawables are never 0x0.
>
> Yeap, I don't know how we end with a pximap of size 0×0:
>
> [...]
> (gdb) f 7
> #7  xwl_glamor_pixmap_get_wl_buffer (pixmap=pixmap@entry=0x1e5b6f0) at
> xwayland-glamor.c:162
> 162 if (xwl_pixmap->buffer)
>
> (gdb) p *pixmap
> $1 = {drawable = {type = 1 '\001', class = 0 '\000', depth = 24 '\030',
> bitsPerPixel = 32 ' ', id = 0, x = 0, y = 0, width = 0, height = 0,
> pScreen = 0x161d200, serialNumber = 1}, devPrivates = 0x1e5b738, refcnt
> = 1, devKind = 0, devPrivate = {ptr = 0x1e5b7c0, val = 31832000,
> uval = 31832000, fptr = 0x1e5b7c0}, screen_x = 0, screen_y = 0,
> usage_hint = 0, master_pixmap = 0x0}
>
> How we end up here is unclear though, xwl_pixmap is “optimized out” but
> considering it's a segfault I assume it's NULL.
>
> If we also assume the pixmap was of size 0×0 when xwl_glamor_create_pixmap()
> was called, then we wouldn't be calling xwl_glamor_create_pixmap_for_bo()
> which would not call xwl_pixmap_set_private():

Ooh. serialNumber == 1 means it's the root pixmap, which will actually
be uselessly empty. It would be interesting to see how we've ended up
here: it would have to be a top-level window which a) was manually
redirected by the WM when it was created, b) had damage posted on it,
and c) was unredirected (in that order). I can't think of how that
would happen; maybe you could place logs for the triggers (e.g.
removing the last manual redirect on a window) somewhere?

It would be good to see what the WindowRec under xwl_window looks
like; maybe that could offer us a clue.

Cheers,
Daniel
___
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: avoid a crash with empty window pixmaps

2018-01-23 Thread Olivier Fourdan 
Hey Adam,

On 22 January 2018 at 19:57, Adam Jackson  wrote:

> On Thu, 2018-01-18 at 11:41 +0100, Olivier Fourdan wrote:
> > This is a rare occurrence of a crash in Xwayland for which I don't have
> > the reproducing steps, just a core file.
> >
> > The backtrace looks as follow:
> >
> >  [...]
> >
> > The crash is caused by dereferencing “xwl_pixmap->buffer” in
> > xwl_glamor_pixmap_get_wl_buffer() because “xwl_pixmap” is NULL.
> >
> > Reason for this is because the corresponding pixmap has a size of 0×0
> > and no xwl_pixmap is created for pixmaps of size 0×0.
>
> That can't really be the problem. X drawables are never 0x0.
>

Yeap, I don't know how we end with a pximap of size 0×0:

(gdb) bt
#0  0x7f00239a31a7 in __GI_raise (sig=sig@entry=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x7f00239a4898 in __GI_abort () at abort.c:90
#2  0x0058f1da in OsAbort () at utils.c:1361
#3  0x00594ce3 in AbortServer () at log.c:877
#4  0x00595b2d in FatalError (f=f@entry=0x5b7490 "Caught signal %d
(%s). Server aborting\n") at log.c:1015
#5  0x0058c43c in OsSigHandler (signo=11, sip=,
unused=) at osinit.c:154
#6  
#7  xwl_glamor_pixmap_get_wl_buffer (pixmap=pixmap@entry=0x1e5b6f0) at
xwayland-glamor.c:162
#8  0x00424da5 in xwl_screen_post_damage (xwl_screen=0x161d750) at
xwayland.c:514
#9  block_handler (data=0x161d750, timeout=) at
xwayland.c:665
#10 0x00557e46 in BlockHandler (pTimeout=pTimeout@entry=0x7ffeb514bf54)
at dixutils.c:388
#11 0x00585ed9 in WaitForSomething (are_ready=0) at WaitFor.c:219
#12 0x005531e1 in Dispatch () at dispatch.c:422
#13 0x0055744a in dix_main (argc=11, argv=0x7ffeb514c138,
envp=) at main.c:287
#14 0x7f002398f377 in __libc_start_main (main=0x4240d0 , argc=11,
ubp_av=0x7ffeb514c138, init=, fini=,
rtld_fini=, stack_end=0x7ffeb514c128) at
../csu/libc-start.c:274
#15 0x004240fe in _start ()

(gdb) f 7
#7  xwl_glamor_pixmap_get_wl_buffer (pixmap=pixmap@entry=0x1e5b6f0) at
xwayland-glamor.c:162
162 if (xwl_pixmap->buffer)

(gdb) p *pixmap
$1 = {drawable = {type = 1 '\001', class = 0 '\000', depth = 24 '\030',
bitsPerPixel = 32 ' ', id = 0, x = 0, y = 0, width = 0, height = 0,
pScreen = 0x161d200, serialNumber = 1}, devPrivates = 0x1e5b738, refcnt
= 1, devKind = 0, devPrivate = {ptr = 0x1e5b7c0, val = 31832000,
uval = 31832000, fptr = 0x1e5b7c0}, screen_x = 0, screen_y = 0,
usage_hint = 0, master_pixmap = 0x0}

How we end up here is unclear though, xwl_pixmap is “optimized out” but
considering it's a segfault I assume it's NULL.

If we also assume the pixmap was of size 0×0 when
xwl_glamor_create_pixmap() was called, then we wouldn't be calling
xwl_glamor_create_pixmap_for_bo() which would not call
xwl_pixmap_set_private():


https://cgit.freedesktop.org/xorg/xserver/tree/hw/xwayland/xwayland-glamor.c#n183

Instead we would call glamor_create_pixmap() which invokes fbCreatePixmap()
for pixmaps of size 0×0:

  https://cgit.freedesktop.org/xorg/xserver/tree/glamor/glamor.c#n180

And the values found in the core file match what is set for by
fbCreatePixmap():

 https://cgit.freedesktop.org/xorg/xserver/tree/fb/fbpixmap.c#n31

So that matches.

Also that doesn't look like a use after free (As Daniel said), because the
pixmap has a refcnt =1 and also because on unrealize, we delete the list,
destroy the damage and remove the callback (where the crash occurs):

  https://cgit.freedesktop.org/xorg/xserver/tree/hw/xwayland/xwayland.c#n577

The pixmap in question comes from GetWindowPixmap():

  https://cgit.freedesktop.org/xorg/xserver/tree/hw/xwayland/xwayland.c#n637

In our case, _fbGetWindowPixmap():

(gdb) p xwl_screen->screen->GetWindowPixmap
$2 = (GetWindowPixmapProcPtr) 0x4520d0 <_fbGetWindowPixmap>

Considering that the corresponding SetWindowPixmap
is damageSetWindowPixmap... I have no idea how we can end up with a pixmap
of size 0×0...

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: [RFC xserver 1/1] xwayland: reduce over-damage

2018-01-23 Thread Pekka Paalanen 
On Mon, 22 Jan 2018 13:14:10 -0500
Adam Jackson  wrote:

> On Mon, 2018-01-22 at 14:51 +, Daniel Stone wrote:
> >  Hi Pekka,
> > 
> > On 20 December 2017 at 11:18, Pekka Paalanen  wrote:  
> > > If an X11 app draws a little here, some there, and a tiny bit in the
> > > opposite corner, using RegionExtents for the damage to be sent to the
> > > Wayland compositor will cause massive over-damaging.
> > > 
> > > However, we cannot blindly send an arbitrary number of damage
> > > rectangles, because there is a risk of overflowing the Wayland
> > > connection. If that happens, it triggers an abort in libwayland-client.
> > > 
> > > Try to be more accurate with the damage by sending up to 500 rectangles
> > > per window, and fall back to extents otherwise. The number is completely
> > > arbitrary.  
> > 
> > I might have said this on IRC, but 500 sails close enough to our
> > request limit for comfort. I can't find the mail where I did the
> > maths, but ISTR for damage requests, it's around the 6000-7000 mark.
> > Pulling 256 as an equally arbitrary number out of the air (arguably
> > even that number of TexSubImage requests represents a DoS), this is:
> > Reviewed-by: Daniel Stone   
> 
> Changed the magic number to 256 and merged, thanks:
> 
> remote: E: failed to find patch for rev 
> f72587ecc7e1dedfb20a999a0e600b83c06a1b29.
> remote: I: 0 patch(es) updated to state Accepted.
> To ssh://git.freedesktop.org/git/xorg/xserver
>a5e9bcad7..f72587ecc  master -> master

Thank you both!

Daniel, a damage request is 8 + 4 * 4 = 24 B. 500 requests is 12000 B,
256 requests is 6144 B. The libwayland internal send buffer is 4096 B,
so for both values we are relying on the kernel socket buffer already.
Furthermore, the limit is per window and number of windows is
effectively unlimited. Xwayland loops over windows without a
possibility to poll for writable.


On Mon, 22 Jan 2018 14:56:12 +0100
Olivier Fourdan  wrote:

> Hi Pekka,
> 
> On 22 January 2018 at 14:34, Pekka Paalanen 
> wrote:

> > should I invest the effort on a better algorithm than in this patch or
> > would this be acceptable for landing?
> >  
> 
> Not sure which better algorithm you might have in mind here, but the
> approach taken in your patch seems simple enough, do we have a rough idea
> of the average number of rectangles we usually deal with here?

The two other approaches in the cover letter:
https://lists.x.org/archives/xorg-devel/2017-December/055474.html
Of course, the optimization approach could use various different
methods. There is also an implementation detail: pixman_region*_t uses
a banded format which increases the number of rectangles compared to a
free format, both using non-overlapping rectangles. The Wayland
protocol does not require the banded format.

I have no statistics about the rectangles on any general use case.

In summary, the Wayland connection overflowing issue would need a
specific solution. Sending optimized damage rectangles may help making
the overflow less likely, but it will never eliminate the possibility.
This patch is about a performance improvement, which is mostly
orthogonal to avoiding the overflow.


Thanks,
pq


pgptHBZPef5Rq.pgp
Description: OpenPGP digital signature
___
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