[PATCH xserver] modesetting: setup colormap

2017-10-23 Thread Rinat Ibragimov 
Signed-off-by: Rinat Ibragimov 
Reviewed-by: Michel Dänzer 
---
 hw/xfree86/drivers/modesetting/driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/xfree86/drivers/modesetting/driver.c 
b/hw/xfree86/drivers/modesetting/driver.c
index 4d5c4e339..45a061efc 100644
--- a/hw/xfree86/drivers/modesetting/driver.c
+++ b/hw/xfree86/drivers/modesetting/driver.c
@@ -1669,7 +1669,7 @@ ScreenInit(ScreenPtr pScreen, int argc, char **argv)
 if (!xf86CrtcScreenInit(pScreen))
 return FALSE;
 
-if (!miCreateDefColormap(pScreen))
+if (!drmmode_setup_colormap(pScreen, pScrn))
 return FALSE;
 
 xf86DPMSInit(pScreen, xf86DPMSSet, 0);
-- 
2.14.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: Determine the X Display edge in a multi X screen configuration

2017-10-23 Thread Adam Jackson 
On Sat, 2017-10-21 at 13:08 -0700, Jim Westfall wrote:

> I'm wanting to determine when the mouse pointer is near the edge of the X 
> Display.  
> 
> In the case when there is only one X screen, the edge of the X screen 
> will be the edge of the X Display.  However when multiple separate X 
> screens (zaphod) make up the X Display using this method is not 
> possible.  For each X screen, there will be at least one edge that is 
> adjacent to another X screen.  As such those edges are not the edge of the 
> X display.
> 
> Originally I figured I would just be able to query the X server for 
> details about the arrangement of the X screens and from that figure out 
> the adjacent X screen edges.  However I've been unable to find any function 
> that provides this information.  Does such a function exist? and/or is 
> there some better way to determine if a given X screen edge is the edge of 
> the X display?

Such a function does not exist. As far as the protocol is concerned,
screens do not have any relative geometry.

- 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] present: Only send PresentCompleteNotify events to the presenting client

2017-10-23 Thread Adam Jackson 
On Mon, 2017-10-23 at 16:26 +0200, Michel Dänzer wrote:
> On 20/10/17 09:58 PM, Keith Packard wrote:
> > Michel Dänzer  writes:
> > 
> > > From: Michel Dänzer 
> > > 
> > > We were sending the events to all clients listening for them on the
> > > window. But clients can get confused by events from another client, and
> > > I can't imagine any case where reciving events from other clients would
> > > be required.
> > 
> > While I agree that it's unlikely to be useful to send the event to all
> > listening clients, there is a 'PRESENTEVENTID' in the CompleteNotify
> > which clients "should" be using to check to see if the event being
> > delivered is the one associated with their action.
> > 
> > Did we mess up in some client library and not do this? If so, we might
> > want to fix that library?
> 
> Looking at the Mesa code, it uses xcb_generate_id to generate the event
> ID. AFAICT the XCB code is properly checking and only queuing special
> events with a matching event ID, so the only possible explanation seems
> that xcb_generate_id ends up generating the same ID for multiple
> clients. Is that plausible?

It shouldn't be. They're XIDs, they're partitioned by client. If xcb
ever generated an XID outside the client's range the server would throw
BadIDChoice in present_select_input().

- 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] present: Only send PresentCompleteNotify events to the presenting client

2017-10-23 Thread Keith Packard 
Michel Dänzer  writes:

> Looking at the Mesa code, it uses xcb_generate_id to generate the event
> ID. AFAICT the XCB code is properly checking and only queuing special
> events with a matching event ID, so the only possible explanation seems
> that xcb_generate_id ends up generating the same ID for multiple
> clients. Is that plausible?

Not unless the first one was closed before the second opened. XIDs are
globally unique in X.

-- 
-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] present: Only send PresentCompleteNotify events to the presenting client

2017-10-23 Thread Michel Dänzer 
On 20/10/17 09:58 PM, Keith Packard wrote:
> Michel Dänzer  writes:
> 
>> From: Michel Dänzer 
>>
>> We were sending the events to all clients listening for them on the
>> window. But clients can get confused by events from another client, and
>> I can't imagine any case where reciving events from other clients would
>> be required.
> 
> While I agree that it's unlikely to be useful to send the event to all
> listening clients, there is a 'PRESENTEVENTID' in the CompleteNotify
> which clients "should" be using to check to see if the event being
> delivered is the one associated with their action.
> 
> Did we mess up in some client library and not do this? If so, we might
> want to fix that library?

Looking at the Mesa code, it uses xcb_generate_id to generate the event
ID. AFAICT the XCB code is properly checking and only queuing special
events with a matching event ID, so the only possible explanation seems
that xcb_generate_id ends up generating the same ID for multiple
clients. Is that plausible?


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer



signature.asc
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

Re: [PATCH xserver] present: Only send PresentCompleteNotify events to the presenting client

2017-10-23 Thread Michel Dänzer 
On 20/10/17 11:45 PM, Keith Packard wrote:
> "Keith Packard"  writes:
> 
>> I'm *not* saying this isn't a good patch in practice, I just want to
>> understand whether the system was designed wrong, or if we've
>> implemented it wrong.
> 
> I spent some time this afternoon on IRC chatting with Pierre-Loup. Valve
> has an application (vrcompositor in 'extended' mode) which is using
> multiple display connections and selecting for Present events on all of
> them. I thought for a while that this was doing precisely what we didn't
> think reasonable -- expecting PresentCompleteNotify events generated
> from a PresentPixmap request sent by client 'A' to be delivered via an
> event selection by client 'B'. Nope, they're using the Vulkan
> presentation API (which eventually sends PresentPixmap) on connection
> 'A', and PresentNotifyMSC on connection 'B'. The only events connection
> 'B' is interested in are those generated by the PresentNotifyMSC, not
> those generated by the PresentPixmap sent on connection 'A'.
> 
> Thinking about this some more, I believe we can look at CopyArea and
> GraphicsExposure events as a good analog -- in that case, the
> GraphicsExposure events are *only* delivered to the client who sent the
> CopyArea request. I think the Present extension is badly designed in
> this area; it shouldn't have bothered to place event selection on the
> window, and should have instead made it part of the generating request,
> just like CopyArea does (well, it's part of the GC state, which is
> effectively part of the request, not part of the drawable state).

Note that this patch only changes the behaviour for
PresentCompleteKindNotifyMsc events specifically. In contrast to those,
with PresentCompleteKindPixmap I can at least imagine a use for seeing
events from other clients, e.g. so multiple clients can consistently
count the number of presentations performed to a window.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer



signature.asc
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

Re: [PATCH] modesetting: fail PreInit() if the device has zero connectors

2017-10-23 Thread Hans de Goede 

Hi,

On 20-10-17 19:08, tobias.jako...@uni-bielefeld.de wrote:

On laptop systems with a dedicated (powerful) GPU A, you usually
have all connectors routed to another (less-powerful) GPU B.

With my setup (GPU A = Nvidia, GPU B = Intel) I keep GPU A switched
off by not loading the nouveau kernel driver during boot.

Loading nouveau while X is running then crashes the server:
[   540.775] (EE) 0: /usr/bin/X (xorg_backtrace+0x41) [0x57fa31]
[   540.775] (EE) 1: /usr/bin/X (0x40+0x183429) [0x583429]
[   540.775] (EE) 2: /lib64/libpthread.so.0 (0x7ff02d508000+0x10ec0) 
[0x7ff02d518ec0]
[   540.775] (EE) 3: /lib64/libc.so.6 (gsignal+0x38) [0x7ff02d1a2178]
[   540.775] (EE) 4: /lib64/libc.so.6 (abort+0x16a) [0x7ff02d1a35fa]
[   540.775] (EE) 5: /lib64/libc.so.6 (0x7ff02d16f000+0x2c0b7) [0x7ff02d19b0b7]
[   540.775] (EE) 6: /lib64/libc.so.6 (0x7ff02d16f000+0x2c162) [0x7ff02d19b162]
[   540.775] (EE) 7: /usr/bin/X (dixRegisterPrivateKey+0x247) [0x452197]
[   540.775] (EE) 8: /usr/lib64/xorg/modules/libglamoregl.so 
(glamor_init+0x160) [0x7ff00ee564d0]
[   540.775] (EE) 9: /usr/lib64/xorg/modules/drivers/modesetting_drv.so 
(0x7ff01c19a000+0x83e1) [0x7ff01c1a23e1]
[   540.775] (EE) 10: /usr/bin/X (AddGPUScreen+0xf3) [0x4348b3]
[   540.775] (EE) 11: /usr/bin/X (0x40+0x90271) [0x490271]
[   540.775] (EE) 12: /usr/bin/X (0x40+0x9547b) [0x49547b]
[   540.775] (EE) 13: /usr/bin/X (0x40+0x905d7) [0x4905d7]
[   540.775] (EE) 14: /usr/bin/X (xf86VTEnter+0x1bb) [0x47399b]
[   540.775] (EE) 15: /usr/bin/X (WakeupHandler+0xda) [0x438e3a]
[   540.775] (EE) 16: /usr/bin/X (WaitForSomething+0x1ce) [0x57d6fe]
[   540.775] (EE) 17: /usr/bin/X (0x40+0x34221) [0x434221]
[   540.775] (EE) 18: /usr/bin/X (0x40+0x382f8) [0x4382f8]
[   540.775] (EE) 19: /lib64/libc.so.6 (__libc_start_main+0xf0) [0x7ff02d18f670]
[   540.776] (EE) 20: /usr/bin/X (_start+0x29) [0x4235b9]

In particular note that GLAMOR is initialized for GPU A, which
makes no sense since it has no connectors.

Fix this by bailing out early in the modesetting DDX when a setup
with zero connectors is detected.

Signed-off-by: Tobias Jakobi 


Sorry, but NACK. The modesetting driver actually had such a check before
and I removed it because not having a driver breaks rendering on
the dedicated GPU with "DRI_PRIME=1" for dri2 clients.

I guess that there is some sort of assumption in the code for dealing
with glamor failure that it only happens on coldplug, if you really
want to be able to modprobe nouveau later you should figure out what
is exactly going wrong and fix that.

Note BTW that nouveau should runtime suspend the GPU, even with Xorg
running and there really is no need to not load nouveau.

Regards,

Hans
___
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] modesetting: call xf86HandleColormaps

2017-10-23 Thread Michel Dänzer 
On 23/10/17 12:07 AM, Rinat Ibragimov wrote:
> Long ago, it was possible to apply color profile with modesetting driver.
> But it's not working now. (Stopped approximately after 1.19.0 release).
> Bisecting points to the patch b4e46c04 (xfree86: Hook up colormaps and
> RandR 1.2 gamma code v6). Applying color profile still works with intel
> driver.
> 
> As far as I understand, it was working because there were fall-backs which
> were removed by the patch b4e46c04. And now, since modesetting does not
> provide LoadPalette method, fails. Fortunately, there is
> drmmode_setup_colormap(), which in turn calls xf86HandleColormaps().
> 
> Patch below works on my machine. "xcalib -i -a" inverts colors as before.
> Applying .icc file also changes colors as expected. But this colormap
> handling surely has pitfalls.

What would those pitfalls be?

Before the change above, X11 colormaps weren't applied at all with a
RandR 1.2 capable driver, effectively always using a no-op mapping
instead (and completely breaking depth 8 pseudocolour). Also, the gamma
values managed via RandR 1.2 and other methods weren't integrated with
each other, changing either clobbered the hardware gamma values from the
other one.

Now, all of those things are fixed, the colormap and various gamma
values are correctly combined with each other.


> So I'm not sure. Is this a correct way to fix it?

Yes, it is.

Reviewed-by: Michel Dänzer 


-- 
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 2/2] modesetting: Use helper to fetch drmModeProperty(Blob)s

2017-10-23 Thread Daniel Martin 
Replace the various loops to lookup drmModeProperty(Blob)s by
introducing helper functions.

Signed-off-by: Daniel Martin 
---
 hw/xfree86/drivers/modesetting/drmmode_display.c | 163 +++
 1 file changed, 77 insertions(+), 86 deletions(-)

diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c 
b/hw/xfree86/drivers/modesetting/drmmode_display.c
index 2375fa07d..3897cbcf2 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.c
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
@@ -1187,13 +1187,57 @@ drmmode_output_mode_valid(xf86OutputPtr output, 
DisplayModePtr pModes)
 return MODE_OK;
 }
 
+static int
+koutput_get_prop_idx(int fd, drmModeConnectorPtr koutput,
+int type, const char *name)
+{
+int idx = -1;
+
+for (int i = 0; i < koutput->count_props; i++) {
+drmModePropertyPtr prop = drmModeGetProperty(fd, koutput->props[i]);
+
+if (!prop)
+continue;
+
+if (drm_property_type_is(prop, type) && !strcmp(prop->name, name))
+idx = i;
+
+drmModeFreeProperty(prop);
+
+if (idx > -1)
+break;
+}
+
+return idx;
+}
+
+static int
+koutput_get_prop_id(int fd, drmModeConnectorPtr koutput,
+int type, const char *name)
+{
+int idx = koutput_get_prop_idx(fd, koutput, type, name);
+
+return (idx > -1) ? koutput->props[idx] : -1;
+}
+
+static drmModePropertyBlobPtr
+koutput_get_prop_blob(int fd, drmModeConnectorPtr koutput, const char *name)
+{
+drmModePropertyBlobPtr blob = NULL;
+int idx = koutput_get_prop_idx(fd, koutput, DRM_MODE_PROP_BLOB, name);
+
+if (idx > -1)
+blob = drmModeGetPropertyBlob(fd, koutput->prop_values[idx]);
+
+return blob;
+}
+
 static void
 drmmode_output_attach_tile(xf86OutputPtr output)
 {
 drmmode_output_private_ptr drmmode_output = output->driver_private;
 drmModeConnectorPtr koutput = drmmode_output->mode_output;
 drmmode_ptr drmmode = drmmode_output->drmmode;
-int i;
 struct xf86CrtcTileInfo tile_info, *set = NULL;
 
 if (!koutput) {
@@ -1201,25 +1245,12 @@ drmmode_output_attach_tile(xf86OutputPtr output)
 return;
 }
 
+drmModeFreePropertyBlob(drmmode_output->tile_blob);
+
 /* look for a TILE property */
-for (i = 0; i < koutput->count_props; i++) {
-drmModePropertyPtr props;
-props = drmModeGetProperty(drmmode->fd, koutput->props[i]);
-if (!props)
-continue;
+drmmode_output->tile_blob =
+koutput_get_prop_blob(drmmode->fd, koutput, "TILE");
 
-if (!(props->flags & DRM_MODE_PROP_BLOB)) {
-drmModeFreeProperty(props);
-continue;
-}
-
-if (!strcmp(props->name, "TILE")) {
-drmModeFreePropertyBlob(drmmode_output->tile_blob);
-drmmode_output->tile_blob =
-drmModeGetPropertyBlob(drmmode->fd, koutput->prop_values[i]);
-}
-drmModeFreeProperty(props);
-}
 if (drmmode_output->tile_blob) {
 if (xf86OutputParseKMSTile(drmmode_output->tile_blob->data, 
drmmode_output->tile_blob->length, _info) == TRUE)
 set = _info;
@@ -1233,26 +1264,15 @@ has_panel_fitter(xf86OutputPtr output)
 drmmode_output_private_ptr drmmode_output = output->driver_private;
 drmModeConnectorPtr koutput = drmmode_output->mode_output;
 drmmode_ptr drmmode = drmmode_output->drmmode;
-int i;
+int idx;
 
 /* Presume that if the output supports scaling, then we have a
  * panel fitter capable of adjust any mode to suit.
  */
-for (i = 0; i < koutput->count_props; i++) {
-drmModePropertyPtr props;
-Bool found = FALSE;
+idx = koutput_get_prop_idx(drmmode->fd, koutput,
+DRM_MODE_PROP_ENUM, "scaling mode");
 
-props = drmModeGetProperty(drmmode->fd, koutput->props[i]);
-if (props) {
-found = strcmp(props->name, "scaling mode") == 0;
-drmModeFreeProperty(props);
-}
-
-if (found)
-return TRUE;
-}
-
-return FALSE;
+return (idx > -1) ? TRUE : FALSE;
 }
 
 static DisplayModePtr
@@ -1306,26 +1326,16 @@ drmmode_output_get_modes(xf86OutputPtr output)
 drmmode_ptr drmmode = drmmode_output->drmmode;
 int i;
 DisplayModePtr Modes = NULL, Mode;
-drmModePropertyPtr props;
 xf86MonPtr mon = NULL;
 
 if (!koutput)
 return NULL;
 
+drmModeFreePropertyBlob(drmmode_output->edid_blob);
+
 /* look for an EDID property */
-for (i = 0; i < koutput->count_props; i++) {
-props = drmModeGetProperty(drmmode->fd, koutput->props[i]);
-if (props && (props->flags & DRM_MODE_PROP_BLOB)) {
-if (!strcmp(props->name, "EDID")) {
-if (drmmode_output->edid_blob)
-drmModeFreePropertyBlob(drmmode_output->edid_blob);
-drmmode_output->edid_blob =
-

[PATCH xserver 1/2] modesetting: Fix leak of tile_blob in drmmode_output_destroy

2017-10-23 Thread Daniel Martin 
And drmModeFreePropertyBlob() can handle NULL pointers, no need to check
edid_blob.

Signed-off-by: Daniel Martin 
---
 hw/xfree86/drivers/modesetting/drmmode_display.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c 
b/hw/xfree86/drivers/modesetting/drmmode_display.c
index 4906b0539..2375fa07d 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.c
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
@@ -1355,8 +1355,9 @@ drmmode_output_destroy(xf86OutputPtr output)
 drmmode_output_private_ptr drmmode_output = output->driver_private;
 int i;
 
-if (drmmode_output->edid_blob)
-drmModeFreePropertyBlob(drmmode_output->edid_blob);
+drmModeFreePropertyBlob(drmmode_output->edid_blob);
+drmModeFreePropertyBlob(drmmode_output->tile_blob);
+
 for (i = 0; i < drmmode_output->num_props; i++) {
 drmModeFreeProperty(drmmode_output->props[i].mode_prop);
 free(drmmode_output->props[i].atoms);
-- 
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

Determine the X Display edge in a multi X screen configuration

2017-10-23 Thread Jim Westfall 
Hi

I'm wanting to determine when the mouse pointer is near the edge of the X 
Display.  

In the case when there is only one X screen, the edge of the X screen 
will be the edge of the X Display.  However when multiple separate X 
screens (zaphod) make up the X Display using this method is not 
possible.  For each X screen, there will be at least one edge that is 
adjacent to another X screen.  As such those edges are not the edge of the 
X display.

Originally I figured I would just be able to query the X server for 
details about the arrangement of the X screens and from that figure out 
the adjacent X screen edges.  However I've been unable to find any function 
that provides this information.  Does such a function exist? and/or is 
there some better way to determine if a given X screen edge is the edge of 
the X display?

thanks
Jim


___
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