Re: X freezes for a second or every now and then when lid closed
Quoting Joel Fernandes (2018-10-27 09:14:07) > On Sat, Oct 27, 2018 at 12:38:56AM -0700, Joel Fernandes wrote: > > Hi! > > My Linux laptop running kernel v4.17 freezes intermittently when the laptop > > lid is closed but external monitors are connected to 2 HDMI ports. I > > provided > > details on the issue, any idea what could be causing it? > > > > I ruled out many different subsystems by trial and error, finally I enabled > > ftrace events on power subsystem and see that the freeze is precisely at the > > same time as drm_mode_getconnector is called on these events and its the > > Xorg > > process. I think since Xorg is busy on this drm_mode_getconnector ioctl, its > > not able to perform its duties. > > > > I get a stacktrace like so: > > Xorg-1285 [001] 801.156606: pm_qos_update_request: > >pm_qos_class=CPU_DMA_LATENCY value=-1 > > Xorg-1285 [001] 801.156607: pm_qos_update_target: > >action=UPDATE_REQ prev_value=0 curr_value=20 > > Xorg-1285 [001] 801.156609: > > => pm_qos_update_target > > => intel_dp_aux_xfer > > => intel_dp_aux_transfer > > => drm_dp_dpcd_access > > => drm_dp_dpcd_read > > => intel_dp_read_dpcd > > => intel_dp_detect > > => drm_helper_probe_single_connector_modes > > => drm_mode_getconnector > > => drm_ioctl_kernel > > => drm_ioctl > > => do_vfs_ioctl > > => ksys_ioctl > > => __x64_sys_ioctl > > => do_syscall_64 > > => entry_SYSCALL_64_after_hwframe > > > > The X version I'm running is X.Org X Server 1.19.6 > > > > I also see these in /var/log/Xorg.0.log every 30 seconds and it seems > > correlated to the time of freezing: > > [ 1141.925] (--) modeset(0): HDMI max TMDS frequency 17KHz > > [ 1142.223] (II) modeset(0): EDID vendor "HWP", prod id 13093 > > [ 1142.223] (II) modeset(0): Using hsync ranges from config file > > [ 1142.223] (II) modeset(0): Using vrefresh ranges from config file > > [ 1142.223] (II) modeset(0): Printing DDC gathered Modelines: > > [ 1142.223] (II) modeset(0): Modeline "1920x1080"x0.0 148.50 1920 2008 > > 2052 2200 1080 1084 1089 1125 +hsync +vsync (67.5 kHz eP) > > [ 1142.223] (II) modeset(0): Modeline "1920x1080"x0.0 148.50 1920 2448 > > 2492 2640 1080 1084 1089 1125 +hsync +vsync (56.2 kHz e) > > > > Let me know if you spot anything weird, and if you have any suggestions? > > Just for documenting it, the issue seems very similar to what is reported in > this comment: > https://forums.linuxmint.com/viewtopic.php?t=253475#p1457587 > > I am indeed using Cinammon as well. With the lid closed, the same 'modeset' > log messages appear and freeze the system every 30 seconds. 30s == hotplug polling kthread; the implication would be that it is generating a hotplug uevent everytime. drm.debug=0xe should record what it going on, so capture a dmesg with the lid closed. Platform and panel HW would be useful to know, dmesg from boot is usually sufficient. -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: [PATCH xserver 1/2] xf86-video-modesetting: Add ms_queue_vblank helper
Quoting Keith Packard (2017-09-29 07:20:46) > This provides an API wrapper around the kernel interface for queueing > a vblank event, simplifying all of the callers. > > Signed-off-by: Keith Packard > --- > diff --git a/hw/xfree86/drivers/modesetting/dri2.c > b/hw/xfree86/drivers/modesetting/dri2.c > index bfaea3b84..b2278e78b 100644 > --- a/hw/xfree86/drivers/modesetting/dri2.c > +++ b/hw/xfree86/drivers/modesetting/dri2.c > @@ -747,13 +744,8 @@ ms_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr > draw, CARD64 target_msc, > > if (current_msc >= target_msc) > target_msc = current_msc; > -vbl.request.type = (DRM_VBLANK_ABSOLUTE | > -DRM_VBLANK_EVENT | > -drmmode_crtc->vblank_pipe); > -vbl.request.sequence = ms_crtc_msc_to_kernel_msc(crtc, target_msc); > -vbl.request.signal = (unsigned long)seq; > > -ret = drmWaitVBlank(ms->fd, &vbl); > +ret = ms_queue_vblank(crtc, MS_QUEUE_ABSOLUTE, target_msc, > &queued_msc, seq); > if (ret) { > static int limit = 5; > if (limit) { Inverted error check. -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
[PATCH xserver 3/3] os/WaitFor: Use the simpler xorg_list_for_each_entry()
As we are not freeing elements while iterating the list of timers, we can forgo using the safe variant, and reduce the number of pointer dances required for the insertion sort. Signed-off-by: Chris Wilson --- os/WaitFor.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/os/WaitFor.c b/os/WaitFor.c index e3b545b93..ae317dc11 100644 --- a/os/WaitFor.c +++ b/os/WaitFor.c @@ -295,7 +295,7 @@ OsTimerPtr TimerSet(OsTimerPtr timer, int flags, CARD32 millis, OsTimerCallback func, void *arg) { -OsTimerPtr existing, tmp; +OsTimerPtr existing; CARD32 now = GetTimeInMillis(); if (!timer) { @@ -328,7 +328,7 @@ TimerSet(OsTimerPtr timer, int flags, CARD32 millis, input_lock(); /* Sort into list */ -xorg_list_for_each_entry_safe(existing, tmp, &timers, list) +xorg_list_for_each_entry(existing, &timers, list) if ((int) (existing->expires - millis) > 0) break; /* This even works at the end of the list -- existing->list will be timers */ -- 2.17.0 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 2/3] os/WaitFor: Use xorg_list_append()
Currently, we use xorg_list_add(new, head->prev) which is functionaly equivalent to xorg_list_append(), but with more pointer chasing, so reduce the strain on the reader and compiler by using the simpler append(). Signed-off-by: Chris Wilson --- os/WaitFor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/os/WaitFor.c b/os/WaitFor.c index 7c7b1d2d4..e3b545b93 100644 --- a/os/WaitFor.c +++ b/os/WaitFor.c @@ -332,7 +332,7 @@ TimerSet(OsTimerPtr timer, int flags, CARD32 millis, if ((int) (existing->expires - millis) > 0) break; /* This even works at the end of the list -- existing->list will be timers */ -xorg_list_add(&timer->list, existing->list.prev); +xorg_list_append(&timer->list, &existing->list); /* Check to see if the timer is ready to run now */ if ((int) (millis - now) <= 0) -- 2.17.0 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 1/3] os/WaitFor: Check timers on every iteration
Currently we only check timer expiry if there are no client fd (or other input) waiting to be serviced. This makes it very easy to starve the timers with long request queues, and so miss critical timestamps. The timer subsystem is just another input waiting to be serviced, so evaluate it on every loop like all the others, at the cost of calling GetTimeInMillis() slightly more frequently. (A more invasive and likely OS specific alternative would be to move the timer wheel to the local equivalent of timerfd, and treat it as an input fd to the event loop exactly equivalent to all the others, and so also serviced on every pass. The trade-off being that the kernel timer wheel is likely more efficiently integrated with epoll, but individual updates to each timer would then require syscalls.) Signed-off-by: Chris Wilson --- os/WaitFor.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/os/WaitFor.c b/os/WaitFor.c index fa6a99b18..7c7b1d2d4 100644 --- a/os/WaitFor.c +++ b/os/WaitFor.c @@ -193,10 +193,9 @@ WaitForSomething(Bool are_ready) are_ready = clients_are_ready(); } +timeout = check_timers(); if (are_ready) timeout = 0; -else -timeout = check_timers(); BlockHandler(&timeout); if (NewOutputPending) -- 2.17.0 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: Gradients are broken with glamor when RepeatReflect is set
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
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
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); #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
Re: [xrandr v2] Select NearestNeighbour filtering for pixel exact scaling
Quoting Matt Turner (2017-09-01 22:17:58) > Was there a reason this did not land? It needs a touch more work: "I was wondering, did you (or anyone else) follow up on this patch? I recently used it with good success, and would be happy if it was upstream. Especially as I am planning on getting a 4K monitor soon, and will likely need to run e.g. games at 1080p resolution. I did have to add a "continue;" at the end of the if (!strcmp ("--filter", argv[i])) { block though, or selecting a filter would not work. As another note, changing the filter only has an effect if the resolution/scaling is also altered." My attempt at doing the latter just ended in segv. After which it just slipped my mind. -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: [Intel-gfx] intel driver 4k MST
On Mon, Mar 27, 2017 at 04:47:44PM +0200, René Rebe wrote: > Hi all, > > connecting a Dell 4k MST display using the xorg intel driver on two different > machines (Surface Pro 3, Dell XPS 13 9360) results in one of the two halts > mirrored on the two MST halfs of the display. Using xrandr --left-of / > --right-of I could not yet find a combination that would correctly show the > whole screen. > > In contrast using the xorg modesetting driver on the same kernel and > xorg-server 4k MST works out of the box. > > Is this a know limitation or the intel xorg driver that is just considered > obsolete and dead or anything? It is not likely a ddx bug... -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ 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] Revert "prime: Sync shared pixmap from root window instead of screen pixmap"
This reverts commit b5b292896f647c85f03f53b20b2f03c0e94de428. This breaks the concept of the screen->pixmap_dirty_list as it no longer tracks the relationship between the PixmapDirtyUpdate src and slave_dst, for the supposed convenience of not tracking present flips. --- dix/pixmap.c | 16 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/dix/pixmap.c b/dix/pixmap.c index b67a2e8a6..7a6402411 100644 --- a/dix/pixmap.c +++ b/dix/pixmap.c @@ -241,8 +241,7 @@ PixmapStartDirtyTracking(PixmapPtr src, RegionUnion(damageregion, damageregion, &dstregion); RegionUninit(&dstregion); -DamageRegister(screen->root ? &screen->root->drawable : &src->drawable, - dirty_update->damage); +DamageRegister(&src->drawable, dirty_update->damage); xorg_list_add(&dirty_update->ent, &screen->pixmap_dirty_list); return TRUE; } @@ -270,7 +269,6 @@ PixmapDirtyCopyArea(PixmapPtr dst, RegionPtr dirty_region) { ScreenPtr pScreen = dirty->src->drawable.pScreen; -DrawablePtr src = pScreen->root ? &pScreen->root->drawable : &dirty->src->drawable; int n; BoxPtr b; GCPtr pGC; @@ -278,13 +276,7 @@ PixmapDirtyCopyArea(PixmapPtr dst, n = RegionNumRects(dirty_region); b = RegionRects(dirty_region); -pGC = GetScratchGC(src->depth, pScreen); -if (pScreen->root) { -ChangeGCVal subWindowMode; - -subWindowMode.val = IncludeInferiors; -ChangeGC(NullClient, pGC, GCSubwindowMode, &subWindowMode); -} +pGC = GetScratchGC(dirty->src->drawable.depth, pScreen); ValidateGC(&dst->drawable, pGC); while (n--) { @@ -295,7 +287,7 @@ PixmapDirtyCopyArea(PixmapPtr dst, w = dst_box.x2 - dst_box.x1; h = dst_box.y2 - dst_box.y1; -pGC->ops->CopyArea(src, &dst->drawable, pGC, +pGC->ops->CopyArea(&dirty->src->drawable, &dst->drawable, pGC, dirty->x + dst_box.x1, dirty->y + dst_box.y1, w, h, dirty->dst_x + dst_box.x1, dirty->dst_y + dst_box.y1); @@ -318,7 +310,7 @@ PixmapDirtyCompositeRotate(PixmapPtr dst_pixmap, int error; src = CreatePicture(None, -&pScreen->root->drawable, +&dirty->src->drawable, format, CPSubwindowMode, &include_inferiors, serverClient, &error); -- 2.11.0 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] inputthread: Initialise inputThreadInfo->changed before use
==8734== Thread 2 InputThread: ==8734== Conditional jump or move depends on uninitialised value(s) ==8734==at 0x2FDB05: InputThreadDoWork (inputthread.c:333) ==8734==by 0x6924423: start_thread (pthread_create.c:333) ==8734==by 0x6C229BE: clone (clone.S:105) Signed-off-by: Chris Wilson --- os/inputthread.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/os/inputthread.c b/os/inputthread.c index 8e7f2edb9..4400fba3f 100644 --- a/os/inputthread.c +++ b/os/inputthread.c @@ -403,6 +403,8 @@ InputThreadPreInit(void) if (!inputThreadInfo) FatalError("input-thread: could not allocate memory"); +inputThreadInfo->changed = FALSE; + inputThreadInfo->thread = 0; xorg_list_init(&inputThreadInfo->devs); inputThreadInfo->fds = ospoll_create(); -- 2.11.0 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] os: Fix iteration over busfaults
Fixes a regression from commit 41da295eb50fa08eaacd0ecde99f43a716fcb41a Author: Keith Packard Date: Sun Nov 3 13:12:40 2013 -0800 Trap SIGBUS to handle truncated shared memory segments that causes the SIGBUS handler to fail to chain up correctly and corrupts nearby memory instead. Signed-off-by: Chris Wilson --- os/busfault.c | 13 +++-- 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/os/busfault.c b/os/busfault.c index d4afa6df3..a2d433a2e 100644 --- a/os/busfault.c +++ b/os/busfault.c @@ -98,15 +98,16 @@ static void busfault_sigaction(int sig, siginfo_t *info, void *param) { void*fault = info->si_addr; -struct busfault *busfault = NULL; +struct busfault *iter, *busfault = NULL; void*new_addr; /* Locate the faulting address in our list of shared segments */ -xorg_list_for_each_entry(busfault, &busfaults, list) { -if ((char *) busfault->addr <= (char *) fault && (char *) fault < (char *) busfault->addr + busfault->size) { -break; -} +xorg_list_for_each_entry(iter, &busfaults, list) { + if ((char *) iter->addr <= (char *) fault && (char *) fault < (char *) iter->addr + iter->size) { + busfault = iter; + break; + } } if (!busfault) goto panic; @@ -132,7 +133,7 @@ panic: if (previous_busfault_sigaction) (*previous_busfault_sigaction)(sig, info, param); else -FatalError("bus error"); +FatalError("bus error\n"); } Bool -- 2.11.0 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 2/3] xfree86: Take input lock for xf86TransparentCursor
The new input lock is missing for the xf86TransparentCursor() entry point. Fixes: 6a5a4e60373c ("Remove SIGIO support for input [v5]") References: https://bugs.freedesktop.org/show_bug.cgi?id=99358 Signed-off-by: Chris Wilson --- hw/xfree86/ramdac/xf86HWCurs.c | 4 1 file changed, 4 insertions(+) diff --git a/hw/xfree86/ramdac/xf86HWCurs.c b/hw/xfree86/ramdac/xf86HWCurs.c index 55d5861c1..26dc7e5af 100644 --- a/hw/xfree86/ramdac/xf86HWCurs.c +++ b/hw/xfree86/ramdac/xf86HWCurs.c @@ -261,6 +261,8 @@ xf86SetTransparentCursor(ScreenPtr pScreen) xf86CursorScreenKey); xf86CursorInfoPtr infoPtr = ScreenPriv->CursorInfoPtr; +input_lock(); + if (!ScreenPriv->transparentData) ScreenPriv->transparentData = (*infoPtr->RealizeCursor) (infoPtr, NullCursor); @@ -273,6 +275,8 @@ xf86SetTransparentCursor(ScreenPtr pScreen) ScreenPriv->transparentData); (*infoPtr->ShowCursor) (infoPtr->pScrn); + +input_unlock(); } static void -- 2.11.0 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 3/3] xfree86: Take input_lock() for xf86ScreenCheckHWCursor
Add the missing input_lock() around the call into the driver's UseHWCursor() callback. Fixes: 6a5a4e60373c ("Remove SIGIO support for input [v5]") References: https://bugs.freedesktop.org/show_bug.cgi?id=99358 Signed-off-by: Chris Wilson --- hw/xfree86/ramdac/xf86HWCurs.c | 27 --- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/hw/xfree86/ramdac/xf86HWCurs.c b/hw/xfree86/ramdac/xf86HWCurs.c index 26dc7e5af..09d59b15d 100644 --- a/hw/xfree86/ramdac/xf86HWCurs.c +++ b/hw/xfree86/ramdac/xf86HWCurs.c @@ -139,9 +139,14 @@ Bool xf86CheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, xf86CursorInfoPtr infoPtr) { ScreenPtr pSlave; +Bool use_hw_cursor = TRUE; -if (!xf86ScreenCheckHWCursor(pScreen, cursor, infoPtr)) -return FALSE; +input_lock(); + +if (!xf86ScreenCheckHWCursor(pScreen, cursor, infoPtr)) { +use_hw_cursor = FALSE; +goto unlock; +} /* ask each driver consuming a pixmap if it can support HW cursor */ xorg_list_for_each_entry(pSlave, &pScreen->slave_list, slave_head) { @@ -151,14 +156,22 @@ xf86CheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, xf86CursorInfoPtr infoPtr continue; sPriv = dixLookupPrivate(&pSlave->devPrivates, xf86CursorScreenKey); -if (!sPriv) /* NULL if Option "SWCursor", possibly other conditions */ -return FALSE; +if (!sPriv) { /* NULL if Option "SWCursor", possibly other conditions */ +use_hw_cursor = FALSE; +break; +} /* FALSE if HWCursor not supported by slave */ -if (!xf86ScreenCheckHWCursor(pSlave, cursor, sPriv->CursorInfoPtr)) -return FALSE; +if (!xf86ScreenCheckHWCursor(pSlave, cursor, sPriv->CursorInfoPtr)) { +use_hw_cursor = FALSE; +break; +} } -return TRUE; + +unlock: +input_unlock(); + +return use_hw_cursor; } static Bool -- 2.11.0 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 1/3] xfree86: Take the input lock for xf86RecolorCursor
xf86RecolorCursor() may be called directly from XRecolorCursor as well as from xf86ScreenSetCursor(). In the latter case, the input lock is already held, but not for the former and so we need to add a wrapper function that acquires the input lock before performing xf86RecolorCursor() Fixes: 6a5a4e60373c ("Remove SIGIO support for input [v5]") References: https://bugs.freedesktop.org/show_bug.cgi?id=99358 Signed-off-by: Chris Wilson --- hw/xfree86/ramdac/xf86HWCurs.c | 24 ++-- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/hw/xfree86/ramdac/xf86HWCurs.c b/hw/xfree86/ramdac/xf86HWCurs.c index 448132095..55d5861c1 100644 --- a/hw/xfree86/ramdac/xf86HWCurs.c +++ b/hw/xfree86/ramdac/xf86HWCurs.c @@ -22,6 +22,9 @@ #include "servermd.h" +static void +xf86RecolorCursor_locked(xf86CursorScreenPtr ScreenPriv, CursorPtr pCurs); + static CARD32 xf86ReverseBitOrder(CARD32 v) { @@ -204,7 +207,7 @@ xf86ScreenSetCursor(ScreenPtr pScreen, CursorPtr pCurs, int x, int y) if (!xf86DriverLoadCursorImage (infoPtr, bits)) return FALSE; -xf86RecolorCursor(pScreen, pCurs, 1); +xf86RecolorCursor_locked (ScreenPriv, pCurs); (*infoPtr->SetCursorPosition) (infoPtr->pScrn, x, y); @@ -312,12 +315,9 @@ xf86MoveCursor(ScreenPtr pScreen, int x, int y) input_unlock(); } -void -xf86RecolorCursor(ScreenPtr pScreen, CursorPtr pCurs, Bool displayed) +static void +xf86RecolorCursor_locked(xf86CursorScreenPtr ScreenPriv, CursorPtr pCurs) { -xf86CursorScreenPtr ScreenPriv = -(xf86CursorScreenPtr) dixLookupPrivate(&pScreen->devPrivates, - xf86CursorScreenKey); xf86CursorInfoPtr infoPtr = ScreenPriv->CursorInfoPtr; /* recoloring isn't applicable to ARGB cursors and drivers @@ -357,6 +357,18 @@ xf86RecolorCursor(ScreenPtr pScreen, CursorPtr pCurs, Bool displayed) } } +void +xf86RecolorCursor(ScreenPtr pScreen, CursorPtr pCurs, Bool displayed) +{ +xf86CursorScreenPtr ScreenPriv = +(xf86CursorScreenPtr) dixLookupPrivate(&pScreen->devPrivates, + xf86CursorScreenKey); + +input_lock(); +xf86RecolorCursor_locked (ScreenPriv, pCurs); +input_unlock(); +} + /* These functions assume that MaxWidth is a multiple of 32 */ static unsigned char * RealizeCursorInterleave0(xf86CursorInfoPtr infoPtr, CursorPtr pCurs) -- 2.11.0 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xfree86: Choose the largest output as primary for xf86TargetFallback()
With all things equal (i.e. same output preferences), use the largest output as the primary. From the primary, we choose compatible modes for the others, and given that we are configuring an extended desktop, we have greater freedom in our selection and may as well base our choice on the largest mode available. Signed-off-by: Chris Wilson --- hw/xfree86/modes/xf86Crtc.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/hw/xfree86/modes/xf86Crtc.c b/hw/xfree86/modes/xf86Crtc.c index 966a168..048c3b2 100644 --- a/hw/xfree86/modes/xf86Crtc.c +++ b/hw/xfree86/modes/xf86Crtc.c @@ -2413,7 +2413,10 @@ xf86TargetFallback(ScrnInfoPtr scrn, xf86CrtcConfigPtr config, default_preferred = (((default_mode->type & M_T_PREFERRED) != 0) + ((default_mode->type & M_T_USERPREF) != 0)); -if (default_preferred > target_preferred || !target_mode) { +if (!target_mode || +default_preferred > target_preferred || +(default_preferred == target_preferred && + biggestMode(default_mode, target_mode))) { target_mode = default_mode; target_preferred = default_preferred; target_rotation = config->output[o]->initial_rotation; -- 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
Re: [PATCH v6 10/11] modesetting: Disable Reverse PRIME for i915
On Sun, Jun 12, 2016 at 06:23:25PM +0100, Emil Velikov wrote: > On 12 June 2016 at 17:09, Chris Wilson wrote: > > On Sun, Jun 12, 2016 at 02:35:43PM +0100, Emil Velikov wrote: > >> Hi all, > >> > >> On 11 June 2016 at 01:21, Alex Goins wrote: > >> > Reverse PRIME seems to be designed with discrete graphics as a sink in > >> > mind, designed to do an extra copy from sysmem to vidmem to prevent a > >> > discrete chip from needing to scan out from sysmem. > >> > > >> > The criteria it used to detect this case is if we are a GPU screen and > >> > Glamor accelerated. It's possible for i915 to fulfill these conditions, > >> > despite the fact that the additional copy doesn't make sense for i915. > >> > > >> > Normally, you could just set AccelMethod = none as an option for the > >> > device > >> > and call it a day. However, when running with modesetting as both the > >> > sink > >> > and the source, Glamor must be enabled. > >> > > >> > Ideally, you would be able to set AccelMethod individually for devices > >> > using the same driver, but there seems to be a bug in X option parsing > >> > that > >> > makes all devices on a driver inherit the options from the first detected > >> > device. Thus, glamor needs to be enabled for all or for none until that > >> > bug > >> > (if it's even a bug) is fixed. > >> > > >> > Nonetheless, it probably doesn't make sense to do the extra copy on i915 > >> > even if Glamor is enabled for the device, so this is more user friendly > >> > by > >> > not requiring users to disable acceleration for i915. > >> > > >> IMHO the proposed patch does not sound like a reasonable long-term > >> solution. Ideally the driver will expose a flag, based on which Xorg > >> will enable/disable reverse prime. That said, as a short-term fix this > >> is fine, barring the issues mentioned below. > > > > The decision as to whether or not the slave can use the passed pixmap as > > its own scanout (or whether it requires a copy) is not part of the > > master's policy. > Hi Chris, it's this precisely what I've said/meant :-) > > To put in other words: > IMHO the master/slave device should expose all their capabilities. > Based on these, Xorg should {en,dis}able reverse prime/etc. Yes. But I would prefer it if the user made the choice, it may require to jump through 20 different hoops, but if it is the only way to achieve the user's configuration, that is what is need to be done. This patch seems to be second guessing what the slave might be doing instead, as you said, exposing a method by which the master/slave can negotiate on what is being passed between them. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ 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 v6 10/11] modesetting: Disable Reverse PRIME for i915
On Sun, Jun 12, 2016 at 02:35:43PM +0100, Emil Velikov wrote: > Hi all, > > On 11 June 2016 at 01:21, Alex Goins wrote: > > Reverse PRIME seems to be designed with discrete graphics as a sink in > > mind, designed to do an extra copy from sysmem to vidmem to prevent a > > discrete chip from needing to scan out from sysmem. > > > > The criteria it used to detect this case is if we are a GPU screen and > > Glamor accelerated. It's possible for i915 to fulfill these conditions, > > despite the fact that the additional copy doesn't make sense for i915. > > > > Normally, you could just set AccelMethod = none as an option for the device > > and call it a day. However, when running with modesetting as both the sink > > and the source, Glamor must be enabled. > > > > Ideally, you would be able to set AccelMethod individually for devices > > using the same driver, but there seems to be a bug in X option parsing that > > makes all devices on a driver inherit the options from the first detected > > device. Thus, glamor needs to be enabled for all or for none until that bug > > (if it's even a bug) is fixed. > > > > Nonetheless, it probably doesn't make sense to do the extra copy on i915 > > even if Glamor is enabled for the device, so this is more user friendly by > > not requiring users to disable acceleration for i915. > > > IMHO the proposed patch does not sound like a reasonable long-term > solution. Ideally the driver will expose a flag, based on which Xorg > will enable/disable reverse prime. That said, as a short-term fix this > is fine, barring the issues mentioned below. The decision as to whether or not the slave can use the passed pixmap as its own scanout (or whether it requires a copy) is not part of the master's policy. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ 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] uxa: fix missing includes for fstat
On Tue, Apr 26, 2016 at 02:42:52PM +0200, Stefan Dirsch wrote: > From: Dominique Leuenberger > > Without these headers, we can run into build errors like: sys/stat.h is already included. Curious. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ 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] present: Be consistent in using window vs vblank->window in present_execute()
Upon entering the function, we copy frequently accessed members of the vblank structure to locals (such as the Window). However, we then fluctuate between using the local window and the vblank->window. Under certain situations, the present_flip callback into the driver may be completed instantaneously and so accessing the vblank structure after a successful call into the driver may cause a use-after-free. This is trivially avoided by using the locals we took earlier. Signed-off-by: Chris Wilson --- present/present.c | 30 +++--- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/present/present.c b/present/present.c index 105e2bf..a90f08d 100644 --- a/present/present.c +++ b/present/present.c @@ -628,6 +628,7 @@ present_execute(present_vblank_ptr vblank, uint64_t ust, uint64_t crtc_msc) { WindowPtr window = vblank->window; ScreenPtr screen = window->drawable.pScreen; +PixmapPtr pixmap = vblank->pixmap; present_screen_priv_ptr screen_priv = present_screen_priv(screen); uint8_t mode; @@ -648,7 +649,7 @@ present_execute(present_vblank_ptr vblank, uint64_t ust, uint64_t crtc_msc) } } -if (vblank->flip && vblank->pixmap && vblank->window) { +if (vblank->flip && pixmap && window) { if (screen_priv->flip_pending || screen_priv->unflip_event_id) { DebugPresent(("\tr %lld %p (pending %p unflip %lld)\n", vblank->event_id, vblank, @@ -662,13 +663,14 @@ present_execute(present_vblank_ptr vblank, uint64_t ust, uint64_t crtc_msc) xorg_list_del(&vblank->window_list); vblank->queued = FALSE; -if (vblank->pixmap && vblank->window) { +if (pixmap && window) { if (vblank->flip) { + RegionPtr damage = vblank->update; DebugPresent(("\tf %lld %p %8lld: %08lx -> %08lx\n", vblank->event_id, vblank, crtc_msc, - vblank->pixmap->drawable.id, vblank->window->drawable.id)); + id, window->drawable.id)); /* Prepare to flip by placing it in the flip queue and * and sticking it into the flip_pending field @@ -678,8 +680,7 @@ present_execute(present_vblank_ptr vblank, uint64_t ust, uint64_t crtc_msc) xorg_list_add(&vblank->event_queue, &present_flip_queue); /* Try to flip */ -if (present_flip(vblank->crtc, vblank->event_id, vblank->target_msc, vblank->pixmap, vblank->sync_flip)) { -RegionPtr damage; +if (present_flip(vblank->crtc, vblank->event_id, vblank->target_msc, pixmap, vblank->sync_flip)) { /* Fix window pixmaps: * 1) Restore previous flip window pixmap @@ -689,18 +690,17 @@ present_execute(present_vblank_ptr vblank, uint64_t ust, uint64_t crtc_msc) present_set_tree_pixmap(screen_priv->flip_window, screen_priv->flip_pixmap, (*screen->GetScreenPixmap)(screen)); -present_set_tree_pixmap(vblank->window, NULL, vblank->pixmap); -present_set_tree_pixmap(screen->root, NULL, vblank->pixmap); +present_set_tree_pixmap(window, NULL, pixmap); +present_set_tree_pixmap(screen->root, NULL, pixmap); /* Report update region as damaged */ -if (vblank->update) { -damage = vblank->update; +if (damage) RegionIntersect(damage, damage, &window->clipList); -} else +else damage = &window->clipList; -DamageDamageRegion(&vblank->window->drawable, damage); +DamageDamageRegion(&window->drawable, damage); return; } @@ -710,7 +710,7 @@ present_execute(present_vblank_ptr vblank, uint64_t ust, uint64_t crtc_msc) screen_priv->flip_pending = NULL; vblank->flip = FALSE; } -DebugPresent(("\tc %p %8lld: %08lx -> %08lx\n", vblank, crtc_msc, vblank->pixmap->drawable.id, vblank->window->drawable.id)); +DebugPresent(("\tc %p %8lld: %08lx -> %08lx\n", vblank, crtc_msc, pixmap->drawable.id, window->drawable.id)); if (screen_priv->flip_pending) { /* Check pending flip @@ -738,7 +738,7 @@ present_execute(present_vblank_ptr vblank, uint64_t ust, uint64_t crtc_msc) return; } -present
[xrandr v2] Select NearestNeighbour filtering for pixel exact scaling
When using pixel-exact scaling from for example running a 3840x2160 monitor at 1920x1080 half-resolution upscaling using a bilinear filter introduces blur where none is expected. v2: Allow the user to specify what filter they want using --filter, but default to automatic selection. References: https://bugs.freedesktop.org/show_bug.cgi?id=94816 Signed-off-by: Chris Wilson --- configure.ac | 2 +- xrandr.c | 275 +-- 2 files changed, 192 insertions(+), 85 deletions(-) diff --git a/configure.ac b/configure.ac index dcf7c10..95cc1f6 100644 --- a/configure.ac +++ b/configure.ac @@ -39,7 +39,7 @@ XORG_DEFAULT_OPTIONS AC_CHECK_LIB(m,floor) # Checks for pkg-config packages -PKG_CHECK_MODULES(XRANDR, xrandr >= 1.5 xrender x11 xproto >= 7.0.17) +PKG_CHECK_MODULES(XRANDR, xrandr >= 1.5 xrender x11 xproto >= 7.0.17 pixman-1) AC_CONFIG_FILES([ Makefile diff --git a/xrandr.c b/xrandr.c index dcfdde0..5c3a326 100644 --- a/xrandr.c +++ b/xrandr.c @@ -40,6 +40,7 @@ #include #include #include +#include #ifdef HAVE_CONFIG_H #include "config.h" @@ -135,6 +136,7 @@ usage(void) " --scale x\n" " --scale-from x\n" " --transform \n" + " --filter auto,bilinear,nearest\n" " --off\n" " --crtc \n" " --panning x[++[/x++[]]]\n" @@ -285,6 +287,7 @@ typedef enum _changes { changes_panning = (1 << 10), changes_gamma = (1 << 11), changes_primary = (1 << 12), +changes_filter = (1 << 13), } changes_t; typedef enum _name_kind { @@ -305,19 +308,24 @@ typedef struct { typedef struct _crtc crtc_t; typedef struct _output output_t; typedef struct _transform transform_t; +typedef struct _filter filter_t; typedef struct _umode umode_t; typedef struct _output_prop output_prop_t; typedef struct _provider provider_t; typedef struct _monitors monitors_t; typedef struct _umonitor umonitor_t; -struct _transform { -XTransform transform; +struct _filter { const char *filter; intnparams; XFixed *params; }; + +struct _transform { +XTransform transform; +}; + struct _crtc { name_t crtc; Bool changing; @@ -331,6 +339,7 @@ struct _crtc { output_t **outputs; intnoutput; transform_tcurrent_transform, pending_transform; +filter_tcurrent_filter, pending_filter; }; struct _output_prop { @@ -370,6 +379,7 @@ struct _output { Bool automatic; intscale_from_w, scale_from_h; transform_ttransform; +filter_tfilter; struct { float red; @@ -707,19 +717,29 @@ init_transform (transform_t *transform) memset (&transform->transform, '\0', sizeof (transform->transform)); for (x = 0; x < 3; x++) transform->transform.matrix[x][x] = XDoubleToFixed (1.0); -transform->filter = ""; -transform->nparams = 0; -transform->params = NULL; +} + +static void +init_filter (filter_t *filter) +{ +filter->filter = ""; +filter->nparams = 0; +filter->params = NULL; } static void set_transform (transform_t *dest, - XTransform *transform, - const char *filter, - XFixed *params, - int nparams) + XTransform *transform) { dest->transform = *transform; +} + +static void +set_filter (filter_t*dest, + const char *filter, + XFixed *params, + int nparams) +{ /* note: this string is leaked */ dest->filter = strdup (filter); dest->nparams = nparams; @@ -728,10 +748,25 @@ set_transform (transform_t *dest, } static void +auto_filter (output_t *output) +{ +if ((output->changes & changes_filter) == 0) { +init_filter (&output->filter); +output->filter.filter = "auto"; +output->changes |= changes_filter; +} +} + +static void copy_transform (transform_t *dest, transform_t *src) { -set_transform (dest, &src->transform, - src->filter, src->params, src->nparams); +set_transform (dest, &src->transform); +} + +static void +copy_filter (filter_t *dest, filter_t *src) +{ +set_filter(dest, src->filter, src->params, src->nparams); } static Bool @@ -739,6 +774,12 @@ equal_transform (transform_t *a, transform_t *b) { if (memcmp (&a->transform, &b->transform, sizeof (XTransform)) != 0) return False; +return True; +} + +static Bool +equal_filter (filter_t *a, filter_t *b) +{ if (strcmp (a->
Re: [xrandr] Select NearestNeighbour filtering for pixel exact scaling
On Mon, Apr 04, 2016 at 05:11:11PM +0100, Chris Wilson wrote: > When using pixel-exact scaling from for example running a 3840x2160 monitor > at 1920x1080 half-resolution upscaling using a bilinear filter > introduces blur where none is expected. Missed --scale, this only changes --scale-from. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ 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
[xrandr] Select NearestNeighbour filtering for pixel exact scaling
When using pixel-exact scaling from for example running a 3840x2160 monitor at 1920x1080 half-resolution upscaling using a bilinear filter introduces blur where none is expected. References: https://bugs.freedesktop.org/show_bug.cgi?id=94816 Signed-off-by: Chris Wilson --- xrandr.c | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/xrandr.c b/xrandr.c index dcfdde0..b6b208c 100644 --- a/xrandr.c +++ b/xrandr.c @@ -1292,6 +1292,7 @@ set_output_info (output_t *output, RROutput xid, XRROutputInfo *output_info) /* for --scale-from, figure out the mode size and compute the transform * for the target framebuffer area */ if (output->scale_from_w > 0 && output->mode_info) { + XTransform *t = &output->transform.transform; double sx = (double)output->scale_from_w / output->mode_info->width; double sy = (double)output->scale_from_h / @@ -1300,10 +1301,10 @@ set_output_info (output_t *output, RROutput xid, XRROutputInfo *output_info) printf("scaling %s by %lfx%lf\n", output->output.string, sx, sy); init_transform (&output->transform); - output->transform.transform.matrix[0][0] = XDoubleToFixed (sx); - output->transform.transform.matrix[1][1] = XDoubleToFixed (sy); - output->transform.transform.matrix[2][2] = XDoubleToFixed (1.0); - if (sx != 1 || sy != 1) + t->matrix[0][0] = XDoubleToFixed (sx); + t->matrix[1][1] = XDoubleToFixed (sy); + t->matrix[2][2] = XDoubleToFixed (1.0); + if ((t->matrix[0][0] | t->matrix[1][1]) & 0x) output->transform.filter = "bilinear"; else output->transform.filter = "nearest"; -- 2.8.0.rc3 ___ 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] Xext/vidmode: Reduce verbosity of GetModeLine debug messages
In commit f175cf45aebcdda53f3ae49c0eaf27da1f194e92 Author: Olivier Fourdan Date: Wed Feb 10 09:34:34 2016 +0100 vidmode: move to a separate library of its own the verbosity of some old debug messages (which print the reply to every GetModeLine client request and others) was increased leading to lots of log spam. Downgrade the logging back to DebugF. References: https://bugs.freedesktop.org/show_bug.cgi?id=94515 Signed-off-by: Chris Wilson Cc: Olivier Fourdan Cc: Adam Jackson --- Xext/vidmode.c | 212 - 1 file changed, 106 insertions(+), 106 deletions(-) diff --git a/Xext/vidmode.c b/Xext/vidmode.c index 7c838f4..0cbbdc3 100644 --- a/Xext/vidmode.c +++ b/Xext/vidmode.c @@ -69,7 +69,7 @@ typedef struct { dixSetPrivate(&(c)->devPrivates, VidModeClientPrivateKey, p) #ifdef DEBUG -#define DEBUG_P(x) LogMessage(X_INFO, x"\n"); +#define DEBUG_P(x) DebugF(x"\n") #else #define DEBUG_P(x) /**/ #endif @@ -267,13 +267,13 @@ ProcVidModeGetModeLine(ClientPtr client) rep.vtotal = VidModeGetModeValue(mode, VIDMODE_V_TOTAL); rep.flags = VidModeGetModeValue(mode, VIDMODE_FLAGS); -LogMessage(X_INFO, "GetModeLine - scrn: %d clock: %ld\n", - stuff->screen, (unsigned long) rep.dotclock); -LogMessage(X_INFO, "GetModeLine - hdsp: %d hbeg: %d hend: %d httl: %d\n", - rep.hdisplay, rep.hsyncstart, rep.hsyncend, rep.htotal); -LogMessage(X_INFO, " vdsp: %d vbeg: %d vend: %d vttl: %d flags: %ld\n", - rep.vdisplay, rep.vsyncstart, rep.vsyncend, - rep.vtotal, (unsigned long) rep.flags); +DebugF("GetModeLine - scrn: %d clock: %ld\n", + stuff->screen, (unsigned long) rep.dotclock); +DebugF("GetModeLine - hdsp: %d hbeg: %d hend: %d httl: %d\n", + rep.hdisplay, rep.hsyncstart, rep.hsyncend, rep.htotal); +DebugF(" vdsp: %d vbeg: %d vend: %d vttl: %d flags: %ld\n", + rep.vdisplay, rep.vsyncstart, rep.vsyncend, + rep.vtotal, (unsigned long) rep.flags); /* * Older servers sometimes had server privates that the VidMode @@ -483,23 +483,23 @@ ProcVidModeAddModeLine(ClientPtr client) stuff->after_vtotal = oldstuff->after_vtotal; stuff->after_flags = oldstuff->after_flags; } -LogMessage(X_INFO, "AddModeLine - scrn: %d clock: %ld\n", - (int) stuff->screen, (unsigned long) stuff->dotclock); -LogMessage(X_INFO, "AddModeLine - hdsp: %d hbeg: %d hend: %d httl: %d\n", - stuff->hdisplay, stuff->hsyncstart, - stuff->hsyncend, stuff->htotal); -LogMessage(X_INFO, " vdsp: %d vbeg: %d vend: %d vttl: %d flags: %ld\n", - stuff->vdisplay, stuff->vsyncstart, stuff->vsyncend, - stuff->vtotal, (unsigned long) stuff->flags); -LogMessage(X_INFO, " after - scrn: %d clock: %ld\n", - (int) stuff->screen, (unsigned long) stuff->after_dotclock); -LogMessage(X_INFO, " hdsp: %d hbeg: %d hend: %d httl: %d\n", - stuff->after_hdisplay, stuff->after_hsyncstart, - stuff->after_hsyncend, stuff->after_htotal); -LogMessage(X_INFO, " vdsp: %d vbeg: %d vend: %d vttl: %d flags: %ld\n", - stuff->after_vdisplay, stuff->after_vsyncstart, - stuff->after_vsyncend, stuff->after_vtotal, - (unsigned long) stuff->after_flags); +DebugF("AddModeLine - scrn: %d clock: %ld\n", + (int) stuff->screen, (unsigned long) stuff->dotclock); +DebugF("AddModeLine - hdsp: %d hbeg: %d hend: %d httl: %d\n", + stuff->hdisplay, stuff->hsyncstart, + stuff->hsyncend, stuff->htotal); +DebugF(" vdsp: %d vbeg: %d vend: %d vttl: %d flags: %ld\n", + stuff->vdisplay, stuff->vsyncstart, stuff->vsyncend, + stuff->vtotal, (unsigned long) stuff->flags); +DebugF(" after - scrn: %d clock: %ld\n", + (int) stuff->screen, (unsigned long) stuff->after_dotclock); +DebugF(" hdsp: %d hbeg: %d hend: %d httl: %d\n", + stuff->after_hdisplay, stuff->after_hsyncstart, + stuff->after_hsyncend, stuff->after_htotal); +DebugF(" vdsp: %d vbeg: %d vend: %d vttl: %d flags: %ld\n", + stuff->after_vdisplay, stuff->after_vsyncstart, + stuff->after_vsyncend, stuff->after_vtotal, + (unsigned long) stuff->after_flags); if (ver < 2) { REQUEST_AT_LEAST_SIZE(xXF86OldVidModeAddModeLineReq); @@ -572,7 +572,7
Re: [PATCH xserver 1/3] present: Move msc_is_(equal_or_)after to the top of present.c
On Wed, Feb 24, 2016 at 04:52:57PM +0900, Michel Dänzer wrote: > From: Michel Dänzer > > To make them usable from any other function in the file. No functional > change. > > Signed-off-by: Michel Dänzer Reviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ 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/3] present: Requeue if flip driver hook fails and target MSC not reached
On Wed, Feb 24, 2016 at 04:52:58PM +0900, Michel Dänzer wrote: > From: Michel Dänzer > > For flipping, we wait for the MSC before the target MSC and then call > the driver flip hook. If the latter fails, we have to wait for the > target MSC before falling back to a copy, or else it's executed too > early. > > Fixes glxgears running at unbounded framerate (not synchronized to the > refresh rate) in fullscreen if the driver flip hook fails. > > Signed-off-by: Michel Dänzer Reviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ 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 3/3] present: Only requeue if target MSC is not reached after an unflip
On Wed, Feb 24, 2016 at 04:52:59PM +0900, Michel Dänzer wrote: > From: Michel Dänzer > > While present_pixmap decrements target_msc by 1 for present_queue_vblank, > it leaves the original vblank->target_msc intact. So incrementing the > latter for requeueing resulted in the requeued presentation being > executed too late. My mistake. Yes, the local target_msc is decremented but after vblank->target_msc is assigned. > Also, no need to requeue if the target MSC is already reached. > > This further reduces stutter when a popup menu appears on top of a > flipping fullscreen window. > > Signed-off-by: Michel Dänzer Reviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ 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 3/3] present: Call present_restore_screen_pixmap from present_set_abort_flip
On Fri, Feb 19, 2016 at 11:39:12AM +0900, Michel Dänzer wrote: > From: Michel Dänzer > > After present_set_abort_flip, the screen pixmap will be used for all > screen drawing, so we need to restore the current flip pixmap contents > to the screen pixmap here as well. > > Improves flashing / stutter e.g. when something like a popup menu appears > on top of a flipping fullscreen window or when switching out of > fullscreen. > > Note that this means present_set_abort_flip now relies on screen->root > being non-NULL, but that's already the case in other present code. It is true that we cannot schedule a flip without screen->root (though present_check_flip would crash rather than reject the call). > Signed-off-by: Michel Dänzer Reviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ 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/3] present: Factor code for restoring screen pixmap out of present_unflip
On Fri, Feb 19, 2016 at 11:39:11AM +0900, Michel Dänzer wrote: > From: Michel Dänzer > > The following fix will use the refactored function. > > Signed-off-by: Michel Dänzer Reviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ 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 1/3] present: Only update screen pixmap from flip pixmap once per unflip
On Fri, Feb 19, 2016 at 11:39:10AM +0900, Michel Dänzer wrote: > From: Michel Dänzer > > present_unflip may be called several times from present_check_flip_window > during the same unflip. We can only copy to the screen pixmap the first > time, otherwise we may scribble over other windows. The flip pixmap > contents don't get updated after the first time anyway. > > Fixes at least the following problems, which were introduced by commit > 806470b9 ("present: Copy unflip contents back to the Screen Pixmap"): > > On xfwm4 without compositing, run glxgears and put its window into > fullscreen mode to start flipping. While in fullscreen, open the xfwm4 > window menu by pressing Alt-Space. The window menu was invisible most > of the time because it was getting scribbled over by a repeated unflip > copy. > > When switching a flipping window out of fullscreen, a repeated unflip > copy could leave artifacts of the flip pixmap on the desktop. > > Signed-off-by: Michel Dänzer And the ordering is better: we should do the restoration of the contents before we restore the window->pixmap linkage. Reviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 1/2] dix: Add ClientSignalAll()
This is a variant of ClientSignal() that signals all clients with an optional matching sleeping client, function and closure. Signed-off-by: Chris Wilson --- dix/dixutils.c | 22 ++ include/dix.h | 8 2 files changed, 30 insertions(+) diff --git a/dix/dixutils.c b/dix/dixutils.c index 205550e..b6b0023 100644 --- a/dix/dixutils.c +++ b/dix/dixutils.c @@ -620,6 +620,28 @@ ClientSignal(ClientPtr client) return FALSE; } +int +ClientSignalAll(ClientPtr client, ClientSleepProcPtr function, void *closure) +{ +SleepQueuePtr q; +int count = 0; + +for (q = sleepQueue; q; q = q->next) { +if (!(client == CLIENT_SIGNAL_ANY || q->client == client)) +continue; + +if (!(function == CLIENT_SIGNAL_ANY || q->function == function)) +continue; + +if (!(closure == CLIENT_SIGNAL_ANY || q->closure == closure)) +continue; + +count += QueueWorkProc(q->function, q->client, q->closure); +} + +return count; +} + void ClientWakeup(ClientPtr client) { diff --git a/include/dix.h b/include/dix.h index 921156b..d49d055 100644 --- a/include/dix.h +++ b/include/dix.h @@ -255,6 +255,14 @@ extern _X_EXPORT Bool ClientSleep(ClientPtr client, extern _X_EXPORT Bool ClientSignal(ClientPtr /*client */ ); #endif /* ___CLIENTSIGNAL_DEFINED___ */ +#ifndef ___CLIENTSIGNALALL_DEFINED___ +#define ___CLIENTSIGNALALL_DEFINED___ +#define CLIENT_SIGNAL_ANY ((void *)-1) +extern _X_EXPORT int ClientSignalAll(ClientPtr /*client*/, + ClientSleepProcPtr /*function*/, + void * /*closure*/); +#endif /* ___CLIENTSIGNALALL_DEFINED___ */ + extern _X_EXPORT void ClientWakeup(ClientPtr /*client */ ); extern _X_EXPORT Bool ClientIsAsleep(ClientPtr /*client */ ); -- 2.7.0 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 2/2] dri2: Allow many blocked clients per-drawable
This patch was motivated by the need to fix the use-after-free in dri2ClientWake, but in doing so removes an arbitrary restriction that limits DRI2 to only blocking the first client on each drawable. In order to fix the use-after-free, we need to avoid touching our privates in the ClientSleep callback and so we want to only use that external list as our means of controlling sleeps and wakeups. We thus have a list of sleeping clients at our disposal and can manage multiple events and sources. Signed-off-by: Chris Wilson --- hw/xfree86/dri2/dri2.c | 130 +++-- 1 file changed, 71 insertions(+), 59 deletions(-) diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c index f9d818c..d55be19 100644 --- a/hw/xfree86/dri2/dri2.c +++ b/hw/xfree86/dri2/dri2.c @@ -90,8 +90,6 @@ typedef struct _DRI2Drawable { DRI2BufferPtr *buffers; int bufferCount; unsigned int swapsPending; -ClientPtr blockedClient; -Bool blockedOnMsc; int swap_interval; CARD64 swap_count; int64_t target_sbc; /* -1 means no SBC wait outstanding */ @@ -99,6 +97,7 @@ typedef struct _DRI2Drawable { CARD64 last_swap_msc; /* msc at completion of most recent swap */ CARD64 last_swap_ust; /* ust at completion of most recent swap */ int swap_limit; /* for N-buffering */ +unsigned blocked[3]; Bool needInvalidate; int prime_id; PixmapPtr prime_slave_pixmap; @@ -139,6 +138,44 @@ typedef struct _DRI2Screen { static void destroy_buffer(DrawablePtr pDraw, DRI2BufferPtr buffer, int prime_id); +enum DRI2WakeType { +WAKE_SBC, +WAKE_MSC, +WAKE_SWAP, +}; + +#define Wake(c, t) (void *)((uintptr_t)(c) | (t)) + +static Bool +dri2WakeClient(ClientPtr client, void *closure) +{ +ClientWakeup(client); +return TRUE; +} + +static Bool +dri2WakeAll(ClientPtr client, DRI2DrawablePtr pPriv, enum DRI2WakeType t) +{ +int count; + +if (!pPriv->blocked[t]) +return FALSE; + +count = ClientSignalAll(client, dri2WakeClient, Wake(pPriv, t)); +pPriv->blocked[t] -= count; +return count; +} + +static Bool +dri2Sleep(ClientPtr client, DRI2DrawablePtr pPriv, enum DRI2WakeType t) +{ +if (ClientSleep(client, dri2WakeClient, Wake(pPriv, t))) { +pPriv->blocked[t]++; +return TRUE; +} +return FALSE; +} + static DRI2ScreenPtr DRI2GetScreen(ScreenPtr pScreen) { @@ -210,8 +247,6 @@ DRI2AllocateDrawable(DrawablePtr pDraw) pPriv->buffers = NULL; pPriv->bufferCount = 0; pPriv->swapsPending = 0; -pPriv->blockedClient = NULL; -pPriv->blockedOnMsc = FALSE; pPriv->swap_count = 0; pPriv->target_sbc = -1; pPriv->swap_interval = 1; @@ -219,6 +254,7 @@ DRI2AllocateDrawable(DrawablePtr pDraw) if (!ds->GetMSC || !(*ds->GetMSC) (pDraw, &ust, &pPriv->last_swap_target)) pPriv->last_swap_target = 0; +memset(pPriv->blocked, 0, sizeof(pPriv->blocked)); pPriv->swap_limit = 1; /* default to double buffering */ pPriv->last_swap_msc = 0; pPriv->last_swap_ust = 0; @@ -258,12 +294,7 @@ DRI2SwapLimit(DrawablePtr pDraw, int swap_limit) if (pPriv->swapsPending >= pPriv->swap_limit) return TRUE; -if (pPriv->target_sbc == -1 && !pPriv->blockedOnMsc) { -if (pPriv->blockedClient) { -ClientSignal(pPriv->blockedClient); -} -} - +dri2WakeAll(CLIENT_SIGNAL_ANY, pPriv, WAKE_SWAP); return TRUE; } @@ -412,8 +443,9 @@ DRI2DrawableGone(void *p, XID id) (*pDraw->pScreen->DestroyPixmap)(pPriv->redirectpixmap); } -if (pPriv->blockedClient) -ClientSignal(pPriv->blockedClient); +dri2WakeAll(CLIENT_SIGNAL_ANY, pPriv, WAKE_SWAP); +dri2WakeAll(CLIENT_SIGNAL_ANY, pPriv, WAKE_MSC); +dri2WakeAll(CLIENT_SIGNAL_ANY, pPriv, WAKE_SBC); free(pPriv); @@ -689,15 +721,6 @@ DRI2InvalidateDrawable(DrawablePtr pDraw) ref->invalidate(pDraw, ref->priv, ref->id); } -static Bool -dri2ClientWake(ClientPtr client, void *closure) -{ -DRI2DrawablePtr pPriv = closure; -ClientWakeup(client); -pPriv->blockedClient = NULL; -return TRUE; -} - /* * In the direct rendered case, we throttle the clients that have more * than their share of outstanding swaps (and thus busy buffers) when a @@ -715,26 +738,17 @@ DRI2ThrottleClient(ClientPtr client, DrawablePtr pDraw) return FALSE; /* Throttle to swap limit */ -if ((pPriv->swapsPending >= pPriv->swap_limit) && !pPriv->blockedClient) { -ResetCurrentRequest(client); -client->sequence--; -ClientSleep(client, dri2ClientWake, pPriv); -pPriv->blockedClient = client; -return TRUE; +if (pPriv->swapsPending >= pPriv->swap_limit) { +if (dri2Sleep(client,
Re: [PATCH xserver] dri2: Use the work queue to manage client sleeps
On Thu, Feb 11, 2016 at 05:41:16PM +, Chris Wilson wrote: > On Wed, Feb 10, 2016 at 11:51:18AM -0500, Adam Jackson wrote: > > @@ -983,7 +990,7 @@ DRI2WakeClient(ClientPtr client, DrawablePtr pDraw, int > > frame, > > { > > ScreenPtr pScreen = pDraw->pScreen; > > DRI2DrawablePtr pPriv; > > - > > +t > > Without this, > Tested-by: Chris Wilson > Reviewed-by: Chris Wilson Drat, valgrind turned up something obnoxious: ==8695== Invalid write of size 8 ==8695==at 0x5A74E9: dri2ClientWake (in /opt/xorg/bin/Xorg) ==8695==by 0x43F811: ProcessWorkQueue (in /opt/xorg/bin/Xorg) ==8695==by 0x5DA640: WaitForSomething (in /opt/xorg/bin/Xorg) ==8695==by 0x4397E0: Dispatch (in /opt/xorg/bin/Xorg) ==8695==by 0x43E8B9: dix_main (in /opt/xorg/bin/Xorg) ==8695==by 0x6CD5EC4: (below main) (libc-start.c:287) ==8695== Address 0xcf4c688 is 56 bytes inside a block of size 144 free'd ==8695==at 0x4C2BE10: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==8695==by 0x5A7AB9: DRI2DrawableGone (in /opt/xorg/bin/Xorg) ==8695==by 0x467EE6: FreeResource (in /opt/xorg/bin/Xorg) ==8695==by 0x4336D6: ProcDestroyWindow (in /opt/xorg/bin/Xorg) ==8695==by 0x439A85: Dispatch (in /opt/xorg/bin/Xorg) ==8695==by 0x43E8B9: dix_main (in /opt/xorg/bin/Xorg) ==8695==by 0x6CD5EC4: (below main) (libc-start.c:287) ==8695== Fix incoming; if we remove limitation to only allow one DRI2 client to block (thus allowing multiple clients to wait on a MSC on the root for instance) we can use the ClientSleep to keep track of all the pending wake ups, with just an introduction of ClientSignalAll. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ 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] dri2: Use the work queue to manage client sleeps
On Wed, Feb 10, 2016 at 11:51:18AM -0500, Adam Jackson wrote: > @@ -983,7 +990,7 @@ DRI2WakeClient(ClientPtr client, DrawablePtr pDraw, int > frame, > { > ScreenPtr pScreen = pDraw->pScreen; > DRI2DrawablePtr pPriv; > - > +t Without this, Tested-by: Chris Wilson Reviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ 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] dix: Disable AttendClient() operation during client teardown
commit e43abdce964f5ed9689cf908af8c305b39a5dd36 Author: Chris Wilson Date: Wed Feb 3 09:54:46 2016 + dri2: Unblock Clients on Drawable release added a call to AttendClient during drawable teardown, which also happens as a result of client teardown. However, at that point the Client state is no longer valid causing a possible explosion from AttendClient(). A simple workaround is to set the Client as ignored when tearing it down, but we then also need to teach AttendClient not to dereference its private pointer during teardown. References: Jos van Wolput Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94074 Testcase: dri2-race/client Signed-off-by: Chris Wilson Cc: Ville Syrjälä Cc: Adam Jackson --- dix/dispatch.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dix/dispatch.c b/dix/dispatch.c index 53032dc..bfb5e1a 100644 --- a/dix/dispatch.c +++ b/dix/dispatch.c @@ -3414,6 +3414,9 @@ CloseDownClient(ClientPtr client) } if (really_close_down) { +/* The client is going away, disable any AttendClient during shutdown */ +client->ignoreCount++; + if (client->clientState == ClientStateRunning && nClients == 0) dispatchException |= dispatchExceptionAtReset; -- 2.7.0 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 1/2] os: Move OsCommPtr dereference after ignoreCount check
Make AttendClient safe to call along the client teardown path (i.e. after CloseDownConnection which is called before the Client's resources are freed) by checking the ClientPtr before the OsCommPtr. References: https://bugs.freedesktop.org/show_bug.cgi?id=94074 Signed-off-by: Chris Wilson Cc: Adam Jackson --- os/connection.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/os/connection.c b/os/connection.c index 4c1ba4b..be3e4d1 100644 --- a/os/connection.c +++ b/os/connection.c @@ -1254,12 +1254,13 @@ void IgnoreClient(ClientPtr client) { OsCommPtr oc = (OsCommPtr) client->osPrivate; -int connection = oc->fd; +int connection; client->ignoreCount++; if (client->ignoreCount > 1) return; +connection = oc->fd; isItTimeToYield = TRUE; if (!GrabInProgress || FD_ISSET(connection, &AllClients)) { if (FD_ISSET(connection, &ClientsWithInput)) @@ -1291,12 +1292,13 @@ void AttendClient(ClientPtr client) { OsCommPtr oc = (OsCommPtr) client->osPrivate; -int connection = oc->fd; +int connection; client->ignoreCount--; if (client->ignoreCount) return; +connection = oc->fd; if (!GrabInProgress || GrabInProgress == client->index || FD_ISSET(connection, &GrabImperviousClients)) { FD_SET(connection, &AllClients); -- 2.7.0 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 3/4] dri2: Only create one unnamed reference per Drawable per Client
This workarounds issues in mesa and Mali that likes to create a new DRI2Drawble per context and per frame, respectively. The idea is that we only create one unnamed reference per Client on each Drawable that is never destroyed - and we make the assumption that the only caller is the DRI2 dispatcher and thus we don't need to check that the invalidate handler is the same. Every DRI2Drawable reference sends an invalidate event, and so not only do we have a slow memory leak, we also suffer a performance loss as the Clients create more and more references to the same Drawable - unless we limit them to a single reference each. The issue was first reported 5^H 6 years ago by Pauli Nieminen and I have incorporated his patches here. His improvement was to add a reference count to the per-Client unnamed reference and by doing so could support DRI2DestroyDrawable again. However, it remains the case that as the lifecycles between the GLXDrawable (DRI2Drawable) and the parent Drawable are decoupled, mesa cannot call DRI2DestroyDrawable on Drawables it did not create (or else risk generating BadDrawable runtime errors), see mesa commit 4ebf07a426771b62123e5fcb5a8be0de24037af1 Author: Kristian Høgsberg Date: Mon Sep 13 08:39:42 2010 -0400 glx: Don't destroy DRI2 drawables for legacy glx drawables For GLX 1.3 drawables, we can destroy the DRI2 drawable when the GLX drawable is destroyed. However, for legacy drawables, there os no good way of knowing when the application is done with it, so we just let the DRI2 drawable linger on the server. The server will destroy the DRI2 drawable when it destroys the X drawable or the client exits anyway. https://bugs.freedesktop.org/show_bug.cgi?id=30109 and as such it rules out both using named references by the Clients and the reference ever being reaped. Note Present incorporates its own version of this type of reference leak. v2: Incorporate refcounting of the unnamed reference by Pauli Nieminen. Cc: Daniel Drake Link: https://freedesktop.org/patch/19695/ Link: http://lists.x.org/archives/xorg-devel/2010-November/014783.html Link: http://lists.x.org/archives/xorg-devel/2010-November/014782.html Signed-off-by: Chris Wilson Cc: Ville Syrjälä --- hw/xfree86/dri2/dri2.c| 57 +++ hw/xfree86/dri2/dri2ext.c | 4 ++-- 2 files changed, 55 insertions(+), 6 deletions(-) diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c index 4dc40f8..2f05c64 100644 --- a/hw/xfree86/dri2/dri2.c +++ b/hw/xfree86/dri2/dri2.c @@ -275,11 +275,24 @@ DRI2SwapLimit(DrawablePtr pDraw, int swap_limit) typedef struct DRI2DrawableRefRec { XID pid; +unsigned int refcnt; DRI2InvalidateProcPtr invalidate; void *priv; struct xorg_list link; } DRI2DrawableRefRec, *DRI2DrawableRefPtr; +static DRI2DrawableRefPtr +DRI2FindClientDrawableRef(ClientPtr client, DRI2DrawablePtr pPriv) +{ +DRI2DrawableRefPtr ref; + +xorg_list_for_each_entry(ref, &pPriv->reference_list, link) +if (ref->refcnt && CLIENT_ID(ref->pid) == client->index) +return ref; + +return NULL; +} + int DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID pid, DRI2InvalidateProcPtr invalidate, void *priv) @@ -295,16 +308,34 @@ DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID pid, pPriv->prime_id = dri2ClientPrivate(client)->prime_id; +if (pid == 0) { +ref = DRI2FindClientDrawableRef(client, pPriv); +if (ref) { +ref->refcnt++; +assert(ref->invalidate == invalidate); +assert(ref->priv == priv); +return Success; +} +} + ref = malloc(sizeof *ref); if (ref == NULL) return BadAlloc; -if (!AddResource(pid, dri2ReferenceRes, ref)) { +if (pid == 0) { +ref->refcnt = 1; +ref->pid = FakeClientID(client->index); +} +else { +ref->refcnt = 0; +ref->pid = pid; +} + +if (!AddResource(ref->pid, dri2ReferenceRes, ref)) { free(ref); return BadAlloc; } -ref->pid = pid; ref->invalidate = invalidate; ref->priv = priv; xorg_list_add(&ref->link, &pPriv->reference_list); @@ -313,9 +344,27 @@ DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID pid, } int -DRI2DestroyDrawable(ClientPtr client, DrawablePtr pDraw, XID id) +DRI2DestroyDrawable(ClientPtr client, DrawablePtr pDraw, XID pid) { -FreeResourceByType(id, dri2ReferenceRes, FALSE); +if (pid == 0) { +DRI2DrawablePtr pPriv; +DRI2DrawableRefPtr ref; + +pPriv = DRI2GetDrawable(pDraw); +if (pPriv == NULL) +return BadDrawable; + +ref = DRI2FindClientDrawableRef(client, pPriv); +if (ref == NULL) +return BadMatch; + +if (--ref->refcnt =
[PATCH xserver 2/4] dri2: Split resource tracking for DRI2Drawable and references to them
In order to ease resource tracking, disentangle DRI2Drawable XIDs from their references. This will be used in later patches to first limit the object leak from unnamed references created on behalf of Clients and then never destroy, and then to allow Clients to explicit manage their references to DRI2Drawables. Signed-off-by: Chris Wilson Cc: Ville Syrjälä --- glx/glxdri2.c | 10 ++-- hw/xfree86/dri2/dri2.c| 136 ++ hw/xfree86/dri2/dri2.h| 11 ++-- hw/xfree86/dri2/dri2ext.c | 6 +- 4 files changed, 66 insertions(+), 97 deletions(-) diff --git a/glx/glxdri2.c b/glx/glxdri2.c index 89ad808..d7f1436 100644 --- a/glx/glxdri2.c +++ b/glx/glxdri2.c @@ -105,7 +105,7 @@ __glXDRIdrawableDestroy(__GLXdrawable * drawable) __GLXDRIdrawable *private = (__GLXDRIdrawable *) drawable; const __DRIcoreExtension *core = private->screen->core; -FreeResource(private->dri2_id, FALSE); +DRI2DestroyDrawable(NULL, private->base.pDraw, private->dri2_id); (*core->destroyDrawable) (private->driDrawable); @@ -602,7 +602,7 @@ __glXDRIscreenCreateContext(__GLXscreen * baseScreen, } static void -__glXDRIinvalidateBuffers(DrawablePtr pDraw, void *priv, XID id) +__glXDRIinvalidateBuffers(DrawablePtr pDraw, void *priv) { __GLXDRIdrawable *private = priv; __GLXDRIscreen *screen = private->screen; @@ -641,9 +641,9 @@ __glXDRIscreenCreateDrawable(ClientPtr client, private->base.waitGL = __glXDRIdrawableWaitGL; private->base.waitX = __glXDRIdrawableWaitX; -ret = DRI2CreateDrawable2(client, pDraw, drawId, - __glXDRIinvalidateBuffers, private, - &private->dri2_id); +private->dri2_id = FakeClientID(client->index); +ret = DRI2CreateDrawable(client, pDraw, private->dri2_id, + __glXDRIinvalidateBuffers, private); if (cx != lastGLContext) { lastGLContext = cx; cx->makeCurrent(cx); diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c index bbff11c..4dc40f8 100644 --- a/hw/xfree86/dri2/dri2.c +++ b/hw/xfree86/dri2/dri2.c @@ -68,16 +68,16 @@ static DevPrivateKeyRec dri2PixmapPrivateKeyRec; static DevPrivateKeyRec dri2ClientPrivateKeyRec; -#define dri2ClientPrivateKey (&dri2ClientPrivateKeyRec) - -#define dri2ClientPrivate(_pClient) (dixLookupPrivate(&(_pClient)->devPrivates, \ - dri2ClientPrivateKey)) - typedef struct _DRI2Client { int prime_id; } DRI2ClientRec, *DRI2ClientPtr; -static RESTYPE dri2DrawableRes; +static DRI2ClientPtr dri2ClientPrivate(ClientPtr pClient) +{ +return dixLookupPrivate(&pClient->devPrivates, &dri2ClientPrivateKeyRec); +} + +static RESTYPE dri2DrawableRes, dri2ReferenceRes; typedef struct _DRI2Screen *DRI2ScreenPtr; @@ -203,6 +203,11 @@ DRI2AllocateDrawable(DrawablePtr pDraw) if (pPriv == NULL) return NULL; +if (!AddResource(pDraw->id, dri2DrawableRes, pPriv)) { +free(pPriv); +return NULL; +} + pPriv->dri2_screen = ds; pPriv->drawable = pDraw; pPriv->width = pDraw->width; @@ -269,49 +274,37 @@ DRI2SwapLimit(DrawablePtr pDraw, int swap_limit) } typedef struct DRI2DrawableRefRec { -XID id; -XID dri2_id; +XID pid; DRI2InvalidateProcPtr invalidate; void *priv; struct xorg_list link; } DRI2DrawableRefRec, *DRI2DrawableRefPtr; -static DRI2DrawableRefPtr -DRI2LookupDrawableRef(DRI2DrawablePtr pPriv, XID id) +int +DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID pid, + DRI2InvalidateProcPtr invalidate, void *priv) { +DRI2DrawablePtr pPriv; DRI2DrawableRefPtr ref; -xorg_list_for_each_entry(ref, &pPriv->reference_list, link) { -if (ref->id == id) -return ref; -} - -return NULL; -} +pPriv = DRI2GetDrawable(pDraw); +if (pPriv == NULL) +pPriv = DRI2AllocateDrawable(pDraw); +if (pPriv == NULL) +return BadAlloc; -static int -DRI2AddDrawableRef(DRI2DrawablePtr pPriv, XID id, XID dri2_id, - DRI2InvalidateProcPtr invalidate, void *priv) -{ -DRI2DrawableRefPtr ref; +pPriv->prime_id = dri2ClientPrivate(client)->prime_id; ref = malloc(sizeof *ref); if (ref == NULL) return BadAlloc; -if (!AddResource(dri2_id, dri2DrawableRes, pPriv)) { +if (!AddResource(pid, dri2ReferenceRes, ref)) { free(ref); return BadAlloc; } -if (!DRI2LookupDrawableRef(pPriv, id)) -if (!AddResource(id, dri2DrawableRes, pPriv)) { -FreeResourceByType(dri2_id, dri2DrawableRes, TRUE); -free(ref); -return BadAlloc; -} -ref->id = id; -ref->dri2_id = dri2_id; +ref->pid = pid; ref->invalida
[PATCH xserver 4/4] dri2: Unblock Clients on Drawable release
If the Window is destroyed by another client, such as the window manager, the original client may be blocked by DRI2 awaiting a vblank event. When this happens, DRI2DrawableGone forgets to unblock that client and so the wait never completes. Note Present/xshmfence is also suspectible to this race. Testcase: dri2-race/manager Signed-off-by: Chris Wilson Cc: Ville Syrjälä --- hw/xfree86/dri2/dri2.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c index 2f05c64..80a601e 100644 --- a/hw/xfree86/dri2/dri2.c +++ b/hw/xfree86/dri2/dri2.c @@ -416,6 +416,9 @@ DRI2DrawableGone(void *p, XID id) (*pDraw->pScreen->DestroyPixmap)(pPriv->redirectpixmap); } +if (pPriv->blockedClient) +AttendClient(pPriv->blockedClient); + free(pPriv); return Success; -- 2.7.0 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 1/4] dri2: Only invalidate the immediate Window upon SetWindowPixmap
All callers of SetWindowPixmap will themselves be traversing the Window heirachy updating the backing Pixmap of each child and so we can forgo doing the identical traversal inside the DRI2SetWindowPixmap handler. Reported-by: Loïc Yhuel Link: http://lists.x.org/archives/xorg-devel/2015-February/045638.html Signed-off-by: Chris Wilson Cc: Ville Syrjälä --- hw/xfree86/dri2/dri2.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c index 60ea6dd..bbff11c 100644 --- a/hw/xfree86/dri2/dri2.c +++ b/hw/xfree86/dri2/dri2.c @@ -1385,8 +1385,7 @@ DRI2ConfigNotify(WindowPtr pWin, int x, int y, int w, int h, int bw, static void DRI2SetWindowPixmap(WindowPtr pWin, PixmapPtr pPix) { -DrawablePtr pDraw = (DrawablePtr) pWin; -ScreenPtr pScreen = pDraw->pScreen; +ScreenPtr pScreen = pWin->drawable.pScreen; DRI2ScreenPtr ds = DRI2GetScreen(pScreen); pScreen->SetWindowPixmap = ds->SetWindowPixmap; @@ -1394,7 +1393,7 @@ DRI2SetWindowPixmap(WindowPtr pWin, PixmapPtr pPix) ds->SetWindowPixmap = pScreen->SetWindowPixmap; pScreen->SetWindowPixmap = DRI2SetWindowPixmap; -DRI2InvalidateDrawableAll(pDraw); +DRI2InvalidateDrawable(&pWin->drawable); } #define MAX_PRIME DRI2DriverPrimeMask -- 2.7.0 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] present: Fix Async swap logic
On Wed, Nov 04, 2015 at 10:48:40AM +0100, Axel Davy wrote: > On 04/11/2015 10:40, Chris Wilson wrote: > >On Tue, Nov 03, 2015 at 09:14:51AM +0100, Axel Davy wrote: > >>+if (pixmap != NULL && > >>+!(options & PresentOptionCopy) && > >>+screen_priv->info) { > >>+if (target_msc > crtc_msc && > >>+present_check_flip (target_crtc, window, pixmap, TRUE, valid, > >>x_off, y_off)) > >>+{ > >>+ vblank->flip = TRUE; > >>+ vblank->sync_flip = TRUE; > >>+ target_msc--; > >>+} else if (target_msc == crtc_msc && > >>+(options & PresentOptionAsync) && > >>+(screen_priv->info->capabilities & PresentCapabilityAsync) && > >>+present_check_flip (target_crtc, window, pixmap, FALSE, valid, > >>x_off, y_off)) > >>+{ > >>+ vblank->flip = TRUE; > >>+} > >> } > >For reference, this is how I fixed the async flip + swap elision: > > > >t a/present/present.c b/present/present.c > >index e9ccfb8..32522af 100644 > >--- a/present/present.c > >+++ b/present/present.c > >@@ -861,19 +861,19 @@ present_pixmap(WindowPtr window, > > vblank->notifies = notifies; > > vblank->num_notifies = num_notifies; > >-if (!(options & PresentOptionAsync)) > >-vblank->sync_flip = TRUE; > >- > >-if (!(options & PresentOptionCopy) && > >-!((options & PresentOptionAsync) && > >- (!screen_priv->info || > >- !(screen_priv->info->capabilities & PresentCapabilityAsync))) && > >-pixmap != NULL && > >-present_check_flip (target_crtc, window, pixmap, vblank->sync_flip, > >valid, x_off, y_off)) > >-{ > >-vblank->flip = TRUE; > >-if (vblank->sync_flip) > >+if (!(options & PresentOptionCopy) && pixmap != NULL) { > >+if (options & PresentOptionAsync && > >+(screen_priv->info && > >+ screen_priv->info->capabilities & PresentCapabilityAsync) && > >+present_check_flip (target_crtc, window, pixmap, FALSE, valid, > >x_off, y_off)) { > >+vblank->flip = TRUE; > >+} > >+else if (present_check_flip (target_crtc, window, pixmap, TRUE, > >valid, x_off, y_off)) { > >+vblank->flip = TRUE; > >+vblank->sync_flip = TRUE; > > target_msc--; > >+} else if (options & PresentOptionAsync) > >+target_msc++; > > } > > if (wait_fence) { > > > > > Hi, > > Could you explain: > . Why you increase target_msc when the Async option is requested ? It's the fallthrough path where the client requested an async swap but we cannot perform one and so must use a synchronous wait for the vblank (in which case we actually wait for the vblank before target_msc, hence the increment here to compenstae). > . Why you check for Async flips first (isn't sync flips better when > possible) ? The choice is between using native async flips or emulating them through sync flips. Native wins. > To me the Async option isn't related particularly to Async flips. > Async flips is just an optimization to handle it without a copy in > the case you would need one. It is described as being "perform the flip as soon as possible (if target < current msc) without regard to synchronisation to vrefresh". That says use tearing native async flips to me. > http://cgit.freedesktop.org/xorg/proto/presentproto/tree/presentproto.txt#n212 > Given the spec, I believe when target_msc > crtc_msc, the behaviour > should be the same with or without the flag, and thus you shouldn't > increase target_msc. Exactly, hence why we need to fudge it in the code to maintain the requested msc (as the code fudges it again later on). -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] present: Fix Async swap logic
On Tue, Nov 03, 2015 at 09:14:51AM +0100, Axel Davy wrote: > According to the spec, PresentOptionAsync should only > trigger a different behaviour when the target msc has been reached. > > In this case if the driver is able to do async swaps, we use > them to avoid a screen copy. > > When the target msc hasn't been reached yet, we want to use sync swaps. > > Signed-off-by: Axel Davy > --- > I'm not sure for indentation, I tried to do the same than previous check > present/present.c | 29 - > 1 file changed, 16 insertions(+), 13 deletions(-) > > diff --git a/present/present.c b/present/present.c > index beb4ff0..3b8679c 100644 > --- a/present/present.c > +++ b/present/present.c > @@ -836,19 +836,22 @@ present_pixmap(WindowPtr window, > vblank->notifies = notifies; > vblank->num_notifies = num_notifies; > > -if (!(options & PresentOptionAsync)) > -vblank->sync_flip = TRUE; > - > -if (!(options & PresentOptionCopy) && > -!((options & PresentOptionAsync) && > - (!screen_priv->info || > - !(screen_priv->info->capabilities & PresentCapabilityAsync))) && > -pixmap != NULL && > -present_check_flip (target_crtc, window, pixmap, vblank->sync_flip, > valid, x_off, y_off)) > -{ > -vblank->flip = TRUE; > -if (vblank->sync_flip) > -target_msc--; > +if (pixmap != NULL && > +!(options & PresentOptionCopy) && > +screen_priv->info) { > +if (target_msc > crtc_msc && > +present_check_flip (target_crtc, window, pixmap, TRUE, valid, > x_off, y_off)) > +{ > + vblank->flip = TRUE; > + vblank->sync_flip = TRUE; > + target_msc--; > +} else if (target_msc == crtc_msc && > +(options & PresentOptionAsync) && > +(screen_priv->info->capabilities & PresentCapabilityAsync) && > +present_check_flip (target_crtc, window, pixmap, FALSE, valid, > x_off, y_off)) > +{ > + vblank->flip = TRUE; > +} > } For reference, this is how I fixed the async flip + swap elision: t a/present/present.c b/present/present.c index e9ccfb8..32522af 100644 --- a/present/present.c +++ b/present/present.c @@ -861,19 +861,19 @@ present_pixmap(WindowPtr window, vblank->notifies = notifies; vblank->num_notifies = num_notifies; -if (!(options & PresentOptionAsync)) -vblank->sync_flip = TRUE; - -if (!(options & PresentOptionCopy) && -!((options & PresentOptionAsync) && - (!screen_priv->info || - !(screen_priv->info->capabilities & PresentCapabilityAsync))) && -pixmap != NULL && -present_check_flip (target_crtc, window, pixmap, vblank->sync_flip, valid, x_off, y_off)) -{ -vblank->flip = TRUE; -if (vblank->sync_flip) +if (!(options & PresentOptionCopy) && pixmap != NULL) { +if (options & PresentOptionAsync && +(screen_priv->info && + screen_priv->info->capabilities & PresentCapabilityAsync) && +present_check_flip (target_crtc, window, pixmap, FALSE, valid, x_off, y_off)) { +vblank->flip = TRUE; +} +else if (present_check_flip (target_crtc, window, pixmap, TRUE, valid, x_off, y_off)) { +vblank->flip = TRUE; +vblank->sync_flip = TRUE; target_msc--; +} else if (options & PresentOptionAsync) +target_msc++; } if (wait_fence) { -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH] os: Fix iteration over busfaults
Fixes a regression from commit 41da295eb50fa08eaacd0ecde99f43a716fcb41a Author: Keith Packard Date: Sun Nov 3 13:12:40 2013 -0800 Trap SIGBUS to handle truncated shared memory segments that causes the SIGBUS handler to fail to chain up correctly and corrupts nearby memory instead. Signed-off-by: Chris Wilson --- os/busfault.c | 13 +++-- 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/os/busfault.c b/os/busfault.c index d4afa6d..a2d433a 100644 --- a/os/busfault.c +++ b/os/busfault.c @@ -98,15 +98,16 @@ static void busfault_sigaction(int sig, siginfo_t *info, void *param) { void*fault = info->si_addr; -struct busfault *busfault = NULL; +struct busfault *iter, *busfault = NULL; void*new_addr; /* Locate the faulting address in our list of shared segments */ -xorg_list_for_each_entry(busfault, &busfaults, list) { -if ((char *) busfault->addr <= (char *) fault && (char *) fault < (char *) busfault->addr + busfault->size) { -break; -} +xorg_list_for_each_entry(iter, &busfaults, list) { + if ((char *) iter->addr <= (char *) fault && (char *) fault < (char *) iter->addr + iter->size) { + busfault = iter; + break; + } } if (!busfault) goto panic; @@ -132,7 +133,7 @@ panic: if (previous_busfault_sigaction) (*previous_busfault_sigaction)(sig, info, param); else -FatalError("bus error"); +FatalError("bus error\n"); } Bool -- 2.1.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xf86-video-intel] uxa: Add missing const to string to stop compiler warning
On Fri, Oct 02, 2015 at 10:49:45AM +1300, Robert Ancell wrote: > --- > src/uxa/intel_hwmc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) To ssh://git.freedesktop.org/git/xorg/driver/xf86-video-intel 096ddef..4e668dd master -> master Thanks, -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[xrandr] Pretty print modeFlags on unattached modes
For modes attached to an output we decode the modeFlags into their human readable strings. For unattached modes, we currently do not print the modeFlags at all - so continue the copy'n'paste for mode printing. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92025 Signed-off-by: Chris Wilson --- xrandr.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/xrandr.c b/xrandr.c index bcaf247..a38417d 100644 --- a/xrandr.c +++ b/xrandr.c @@ -3372,9 +3372,15 @@ main (int argc, char **argv) if (!(mode->modeFlags & ModeShown)) { - printf (" %s (0x%x) %6.1fMHz\n", + int f; + + printf (" %s (0x%x) %6.1fMHz", mode->name, (int)mode->id, (double)mode->dotClock / 100.0); + for (f = 0; mode_flags[f].flag; f++) + if (mode->modeFlags & mode_flags[f].flag) + printf (" %s", mode_flags[f].string); + printf("\n"); printf ("h: width %4d start %4d end %4d total %4d skew %4d clock %6.1fKHz\n", mode->width, mode->hSyncStart, mode->hSyncEnd, mode->hTotal, mode->hSkew, mode_hsync (mode) / 1000); -- 2.5.1 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [xrandr] Only use the current information when setting modes
On Sun, Sep 13, 2015 at 08:01:46PM +0200, Mark Kettenis wrote: > > Date: Sun, 13 Sep 2015 12:14:57 +0100 > > From: Chris Wilson > > > > On Sun, Sep 13, 2015 at 01:08:11PM +0200, Mark Kettenis wrote: > > > > From: Chris Wilson > > > > Date: Sun, 13 Sep 2015 11:40:37 +0100 > > > > > > > > Before we change the state (e.g. adding a mode or applying one to an > > > > output), we query the screen resources for the right identifiers. This > > > > should only use the current information rather than force a reprobe on > > > > all hardware - not only can that reprobe be very slow (e.g. EDID > > > > timeouts on the order of seconds), but it may perturb the setup that the > > > > user is trying to configure. > > > > > > How do you guarantee that that cached information isn't stale? > > > > There is no issue in that, since the user parameters are against the > > output from a previous run. If the configuration is stale then so are > > the xrandr parameters. > > I bet tons of people have a script that runs xrandr to configure a > projector connected to the VGA port on their laptop. Yes there is no > guarantee that the parameters I give to xrandr match the current > configuration. But if I connect the same projector (or even a > different one) to the same VGA port I expect this script to work and > make the display of my laptop appear on the projector. I expect this > to work even when I connected the VGA cable after I started X. If you relied on xrandr without checking for the existence of the output first, you are describing a race condition. (Any use of xrandr is a race condition!) To maintain the behaviour of providing that automagic probe, you can extend get_outputs() to reprobe if the user requests a conflicting operation, like @@ -1800,10 +1800,26 @@ apply (void) * Use current output state to complete the output list */ static void -get_outputs (void) +get_outputs (int use_current) { into; output_t*q; +int disconnected; + +do +{ +if (res) +{ +XRRFreeScreenResources(res); +res = NULL; +} + +get_screen (use_current); + +disconnected = 0; + +for (q = all_outputs; q; q = q->next) +q->found = False; for (o = 0; o < res->noutput; o++) { @@ -1872,7 +1888,16 @@ get_outputs (void) } set_output_info (output, res->outputs[o], output_info); +disconnected += +output_info->connection == RR_Disconnected && +output_info->crtc; } + +for (q = all_outputs; q; q = q->next) +disconnected += !q->found; + +} while (disconnected && ++use_current == 0); + for (q = all_outputs; q; q = q->next) { if (!q->found) > > > Seems you already can get the behaviour you want by specifying > > > --current. Whereas there is no convenient way to force a probe, other > > > than an explicit query? > > > > And there should not be. On KMS platforms regeneration of the RR > > information is driven by hotplug events (either generated by the hw > > itself or by a kernel worker emulating the notifications otherwise). An > > explicit probe by the user should be the means of last resort not the > > default. > > Well, that only works on KMS with udev. And only if the specific > driver you're using has udev support. > > To me it seems that you're trying to solve this at the wrong level. > If a driver (kernel or Xorg) has reliable hotplug support, shouldn't > it be able to decide whether it needs to reread the EDID information > of some monitor or not? Eh? We already have multiple layers of caching. Howver users making explicit reprobe requests have to override the caching just in case the cache/hardware is broken. The issue is that xrandr does the explicit by default (and similarly that very few pieces of software use XRRGetScreenResourcesCurrent but there are a lot of silly (i.e. where applications are only querying monitor layout with no desire to set configurations) uses of XRRGetScreenResources). -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [xrandr] Only use the current information when setting modes
On Sun, Sep 13, 2015 at 12:14:57PM +0100, Chris Wilson wrote: > On Sun, Sep 13, 2015 at 01:08:11PM +0200, Mark Kettenis wrote: > > Seems you already can get the behaviour you want by specifying > > --current. Whereas there is no convenient way to force a probe, other > > than an explicit query? > > And there should not be. On KMS platforms regeneration of the RR > information is driven by hotplug events (either generated by the hw > itself or by a kernel worker emulating the notifications otherwise). An > explicit probe by the user should be the means of last resort not the > default. Also "xrandr" still does the explicit probe as the query is the default operation, and only using the current on queries requires a user option. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [xrandr] Only use the current information when setting modes
On Sun, Sep 13, 2015 at 01:08:11PM +0200, Mark Kettenis wrote: > > From: Chris Wilson > > Date: Sun, 13 Sep 2015 11:40:37 +0100 > > > > Before we change the state (e.g. adding a mode or applying one to an > > output), we query the screen resources for the right identifiers. This > > should only use the current information rather than force a reprobe on > > all hardware - not only can that reprobe be very slow (e.g. EDID > > timeouts on the order of seconds), but it may perturb the setup that the > > user is trying to configure. > > How do you guarantee that that cached information isn't stale? There is no issue in that, since the user parameters are against the output from a previous run. If the configuration is stale then so are the xrandr parameters. > Seems you already can get the behaviour you want by specifying > --current. Whereas there is no convenient way to force a probe, other > than an explicit query? And there should not be. On KMS platforms regeneration of the RR information is driven by hotplug events (either generated by the hw itself or by a kernel worker emulating the notifications otherwise). An explicit probe by the user should be the means of last resort not the default. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[xrandr] Only use the current information when setting modes
Before we change the state (e.g. adding a mode or applying one to an output), we query the screen resources for the right identifiers. This should only use the current information rather than force a reprobe on all hardware - not only can that reprobe be very slow (e.g. EDID timeouts on the order of seconds), but it may perturb the setup that the user is trying to configure. Signed-off-by: Chris Wilson --- xrandr.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/xrandr.c b/xrandr.c index 181c76e..dcfdde0 100644 --- a/xrandr.c +++ b/xrandr.c @@ -3325,7 +3325,7 @@ main (int argc, char **argv) { umode_t *m; -get_screen (current); +get_screen (True); get_crtcs(); get_outputs(); @@ -3374,7 +3374,7 @@ main (int argc, char **argv) { output_t *output; -get_screen (current); +get_screen (True); get_crtcs(); get_outputs(); @@ -3465,7 +3465,7 @@ main (int argc, char **argv) if (!has_1_4) fatal ("--setprovideroutputsource requires RandR 1.4\n"); - get_screen (current); + get_screen (True); get_providers (); provider = find_provider (&provider_name); @@ -3480,7 +3480,7 @@ main (int argc, char **argv) if (!has_1_4) fatal ("--setprovideroffloadsink requires RandR 1.4\n"); - get_screen (current); + get_screen (True); get_providers (); provider = find_provider (&provider_name); @@ -3490,7 +3490,7 @@ main (int argc, char **argv) } if (setit_1_2) { - get_screen (current); + get_screen (True); get_crtcs (); get_outputs (); set_positions (); @@ -3589,7 +3589,7 @@ main (int argc, char **argv) exit(0); } - get_screen(current); + get_screen(True); get_monitors(True); get_crtcs(); get_outputs(); -- 2.5.1 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] Xephyr: Paint with subimage for non-Glamor & non-XSHM case
On Thu, May 21, 2015 at 04:13:12PM -0700, Ian Scott wrote: > This improves the case for when we paint an area without SHM. Improves? Xephyr is very much broken since commit a2b73da78de4e627965213d24a6c33f243a60eb6 Author: Julien Cristau Date: Sun Jun 20 00:05:40 2010 +0100 > xcb_image_subimage() is used to create a subimage for the damaged area, which > is converted to native format if necessary. > > Signed-off-by: Ian Scott > --- > hw/kdrive/ephyr/hostx.c | 10 ++ > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/hw/kdrive/ephyr/hostx.c b/hw/kdrive/ephyr/hostx.c > index dc265d5..3914f73 100644 > --- a/hw/kdrive/ephyr/hostx.c > +++ b/hw/kdrive/ephyr/hostx.c > @@ -1039,11 +1039,13 @@ hostx_paint_rect(KdScreenInfo *screen, >sx, sy, dx, dy, width, height, FALSE); > } > else { > -/* This is slow and could be done better */ This comment is still valid. :( xcb_image_subimage() does a very slow (get_pixel/put_pixel) copy even when it could just create a view in the native case. > -xcb_image_t *img = xcb_image_native (HostX.conn, scrpriv->ximg, 1); > -xcb_image_put(HostX.conn, scrpriv->win, HostX.gc, img, 0, 0, 0); > -if (scrpriv->ximg != img) > +xcb_image_t *subimg = xcb_image_subimage(scrpriv->ximg, sx, sy, > + width, height, 0, 0, 0); > +xcb_image_t *img = xcb_image_native(HostX.conn, subimg, 1); > +xcb_image_put(HostX.conn, scrpriv->win, HostX.gc, img, dx, dy, 0); > +if (subimg != img) > xcb_image_destroy(img); > + xcb_image_destroy(subimg); Nevertheless, Reviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[xrandr 2/2] Mark all CRTC as currently unused for second picking CRTC pass
We perform two passes over the CRTC in order to find the preferred CRTC for each enabled output. In the first pass, we try to preserve the existing output <-> CRTC relationships (to avoid unnecessary flicker). If that pass fails, we try again but with all outputs first disabled. However, the logic to preserve an active CRTC was not disabled along with the outputs - meaning that if one was active but its associated output was disabled by the user, then that CRTC would remain unavailable for other outputs. The result would be that we would try to assign more CRTC than available (i.e. if the user request 3 new HDMI outputs on a system with only 3 CRTC, and wished to switch off an active internal panel, we would report "cannot find CRTC" even though that configuration could be established.) Reported-and-tested-by: Nathan Schulte Signed-off-by: Chris Wilson --- xrandr.c | 13 + 1 file changed, 13 insertions(+) diff --git a/xrandr.c b/xrandr.c index c0feac3..181c76e 100644 --- a/xrandr.c +++ b/xrandr.c @@ -2243,6 +2243,8 @@ static void pick_crtcs (void) { output_t *output; +int saved_crtc_noutput[num_crtcs]; +int n; /* * First try to match up newly enabled outputs with spare crtcs @@ -2274,7 +2276,18 @@ pick_crtcs (void) */ for (output = all_outputs; output; output = output->next) output->current_crtc_info = output->crtc_info; + +/* Mark all CRTC as currently unused */ +for (n = 0; n < num_crtcs; n++) { + saved_crtc_noutput[n] = crtcs[n].crtc_info->noutput; + crtcs[n].crtc_info->noutput = 0; +} + pick_crtcs_score (all_outputs); + +for (n = 0; n < num_crtcs; n++) + crtcs[n].crtc_info->noutput = saved_crtc_noutput[n]; + for (output = all_outputs; output; output = output->next) { if (output->mode_info && !output->crtc_info) -- 2.1.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[xrandr 1/2] Mark disabling an output as a change in its CRTC
When an output is disabled via the cmdline, we can use that information to prevent assigning the current CRTC to the output and free it up for reuse by other outputs in the first pass of picking CRTC. Reported-and-tested-by: Nathan Schulte Signed-off-by: Chris Wilson --- xrandr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xrandr.c b/xrandr.c index fbfd93e..c0feac3 100644 --- a/xrandr.c +++ b/xrandr.c @@ -3029,7 +3029,7 @@ main (int argc, char **argv) if (!config_output) argerr ("%s must be used after --output\n", argv[i]); set_name_xid (&config_output->mode, None); set_name_xid (&config_output->crtc, None); - config_output->changes |= changes_mode; + config_output->changes |= changes_mode | changes_crtc; continue; } if (!strcmp ("--fb", argv[i])) { -- 2.1.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 6/6] prime: add rotation support for offloaded outputs
On Mon, Jun 15, 2015 at 09:25:01AM +1000, Dave Airlie wrote: > One of the lacking features with output offloading was > that screen rotation didn't work at all. > > This patch makes 0/90/180/270 rotation work with USB output > and GPU outputs. > > When it allocates the shared pixmap it allocates it rotated, > and any updates to the shared pixmap are done using a composite > path that does the rotation. The slave GPU then doesn't need > to know about the rotation and just displays the pixmap. This doesn't seem right. The slaved output already has the transform details and currently the ability to apply HW transformation when possible. It also needs to know the transform for applying the HW cursor. I guess the problem you face is that you want the host GPU to accelerate the rotation for a slaved USB device? -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] atom: make FreeAtom static
On Tue, Jun 02, 2015 at 02:08:39PM -0400, Adam Jackson wrote: > Signed-off-by: Adam Jackson Reviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/2] atom: Bump initial table size
On Fri, Jun 05, 2015 at 10:26:15AM -0400, Adam Jackson wrote: > On Tue, 2015-06-02 at 20:54 +0100, Chris Wilson wrote: > > On Tue, Jun 02, 2015 at 02:08:38PM -0400, Adam Jackson wrote: > > > We're always creating ~230 atoms at startup, might as well tune it > > > so we don't hit the realloc path before Dispatch. > > > > Any clue as to how you found out this figure? > > dmt:~% Xvfb -ac -terminate -fp built-ins :1 & > [1] 31760 > dmt:~% DISPLAY=:1 xlsatoms | wc -l > 233 Thank you, Reviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/2] atom: Bump initial table size
On Tue, Jun 02, 2015 at 02:08:38PM -0400, Adam Jackson wrote: > We're always creating ~230 atoms at startup, might as well tune it so we > don't hit the realloc path before Dispatch. Any clue as to how you found out this figure? Maybe add a DebugF for when we realloc the table, and/or print out the number of Atoms created during initialisation. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/8] dix: Unexport various implementation details
On Tue, Jun 02, 2015 at 02:14:59PM -0400, Adam Jackson wrote: > Signed-off-by: Adam Jackson For this series, Acked-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/3] present: Query the Window's CRTC every time
On Thu, May 28, 2015 at 06:27:34PM +0900, Michel Dänzer wrote: > On 28.05.2015 18:07, Chris Wilson wrote: > > On Thu, May 28, 2015 at 05:56:15PM +0900, Michel Dänzer wrote: > >> On 28.05.2015 17:38, Chris Wilson wrote: > >>> On Thu, May 28, 2015 at 04:59:14PM +0900, Michel Dänzer wrote: > >>>> The patch below is an alternative fix for the problem I'm seeing, while > >>>> preserving the window CRTC for MSC waits when possible. Your > >>>> description sounds like it might work for you as well, Chris. Can you > >>>> try it? > >>>> > >>>> > >>>> diff --git a/present/present.c b/present/present.c > >>>> index a634601..dc58f25 100644 > >>>> --- a/present/present.c > >>>> +++ b/present/present.c > >>>> @@ -713,6 +713,7 @@ present_pixmap(WindowPtr window, > >>>> uint64_tust; > >>>> uint64_ttarget_msc; > >>>> uint64_tcrtc_msc; > >>>> +RRCrtcPtr new_crtc; > >>>> int ret; > >>>> present_vblank_ptr vblank, tmp; > >>>> ScreenPtr screen = window->drawable.pScreen; > >>>> @@ -734,7 +735,21 @@ present_pixmap(WindowPtr window, > >>>> target_crtc = present_get_crtc(window); > >>>> } > >>>> > >>>> -present_get_ust_msc(screen, target_crtc, &ust, &crtc_msc); > >>>> +if (present_get_ust_msc(screen, target_crtc, &ust, &crtc_msc) != > >>>> Success) { > >>>> +/* Maybe we need to try a different CRTC? > >>>> + */ > >>>> +new_crtc = present_get_crtc(window); > >>>> + > >>>> +if (new_crtc != target_crtc && > >>>> +present_get_ust_msc(screen, new_crtc, &ust, &crtc_msc) == > >>>> Success) > >>>> +target_crtc = new_crtc; > >>>> +else { > >>>> +/* Fall back to fake MSC handling > >>>> + */ > >>>> +target_crtc = NULL; > >>>> +present_fake_get_ust_msc(screen, &ust, &crtc_msc); > >>>> +} > >>>> +} > >>> > >>> It survived for one more CRTC change, but still feeds passed msc into > >>> the wait_vblank. > >> > >> By how much is it off? Does it cause a hang? > > > > Just a few -1000 msc, which is a pretty long wait (it would be if we > > honoured it at least). > > The kernel should immediately return / send the event in that case. Given that it is supposedly an impossible condition in present_pixmap, it is simple evidence of a bug. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/3] present: Query the Window's CRTC every time
On Thu, May 28, 2015 at 05:56:15PM +0900, Michel Dänzer wrote: > On 28.05.2015 17:38, Chris Wilson wrote: > > On Thu, May 28, 2015 at 04:59:14PM +0900, Michel Dänzer wrote: > >> The patch below is an alternative fix for the problem I'm seeing, while > >> preserving the window CRTC for MSC waits when possible. Your > >> description sounds like it might work for you as well, Chris. Can you > >> try it? > >> > >> > >> diff --git a/present/present.c b/present/present.c > >> index a634601..dc58f25 100644 > >> --- a/present/present.c > >> +++ b/present/present.c > >> @@ -713,6 +713,7 @@ present_pixmap(WindowPtr window, > >> uint64_tust; > >> uint64_ttarget_msc; > >> uint64_tcrtc_msc; > >> +RRCrtcPtr new_crtc; > >> int ret; > >> present_vblank_ptr vblank, tmp; > >> ScreenPtr screen = window->drawable.pScreen; > >> @@ -734,7 +735,21 @@ present_pixmap(WindowPtr window, > >> target_crtc = present_get_crtc(window); > >> } > >> > >> -present_get_ust_msc(screen, target_crtc, &ust, &crtc_msc); > >> +if (present_get_ust_msc(screen, target_crtc, &ust, &crtc_msc) != > >> Success) { > >> +/* Maybe we need to try a different CRTC? > >> + */ > >> +new_crtc = present_get_crtc(window); > >> + > >> +if (new_crtc != target_crtc && > >> +present_get_ust_msc(screen, new_crtc, &ust, &crtc_msc) == > >> Success) > >> +target_crtc = new_crtc; > >> +else { > >> +/* Fall back to fake MSC handling > >> + */ > >> +target_crtc = NULL; > >> +present_fake_get_ust_msc(screen, &ust, &crtc_msc); > >> +} > >> + } > > > > It survived for one more CRTC change, but still feeds passed msc into > > the wait_vblank. > > By how much is it off? Does it cause a hang? Just a few -1000 msc, which is a pretty long wait (it would be if we honoured it at least). > What's your test-case? Mine is the GNOME lock screen, i.e. basically > DPMS off vs vblank waits and flips. xf86-video-intel/test/present-test -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/3] present: Query the Window's CRTC every time
On Thu, May 28, 2015 at 04:59:14PM +0900, Michel Dänzer wrote: > On 27.05.2015 15:51, Chris Wilson wrote: > > On Tue, May 26, 2015 at 02:30:32PM -0700, Keith Packard wrote: > >> Michel Dänzer writes: > >> > >>> The old code also called present_get_crtc() unless pixmap == NULL, so > >>> the problem couldn't affect flips but only MSC waits. > >> > >> The original code was trying to let the client control which CRTC to > >> track for each window. For PresentPixmap requests, there's an explicit > >> CRTC argument, which allows the client to select the right one. For > >> PresentNotifyMSC, there's no such argument. > >> > >> So, the code was trying to respect the client's wishes by using > >> whichever CRTC was last associated with the window -- either implicitly > >> through the last call to present_get_crtc or explicitly from the last > >> crtc passed to PresentPixmap for the same window. > >> > >> Probably the right thing to do is to add an explicit CRTC parameter to > >> PresentNotifyMSC, and then have both requests call present_get_crtc when > >> an explicit CRTC is not provided. > >> > >> That doesn't solve the problem when an explicitly requested CRTC is > >> disabled though, so this fix doesn't address the root cause, only one of > >> the symptoms. > >> > >>> The problem I was hitting was that this code was running for an MSC wait > >>> when the CRTC referenced by window_priv->crtc was already disabled for > >>> DPMS off. This caused hangs at the GNOME lock screen. This patch seems > >>> to fix that problem. > >> > >> Why isn't your queue_vblank function bailing when asked to queue a > >> request for a CRTC which is disabled? If it simply fails, > >> present_execute will get called immediately and the client would > >> continue happily along. > > > > Oh, but it does fail gracefully. The problem is not that but that it > > sends a garbage msc to a valid CRTC. > > The patch below is an alternative fix for the problem I'm seeing, while > preserving the window CRTC for MSC waits when possible. Your > description sounds like it might work for you as well, Chris. Can you > try it? > > > diff --git a/present/present.c b/present/present.c > index a634601..dc58f25 100644 > --- a/present/present.c > +++ b/present/present.c > @@ -713,6 +713,7 @@ present_pixmap(WindowPtr window, > uint64_tust; > uint64_ttarget_msc; > uint64_tcrtc_msc; > +RRCrtcPtr new_crtc; > int ret; > present_vblank_ptr vblank, tmp; > ScreenPtr screen = window->drawable.pScreen; > @@ -734,7 +735,21 @@ present_pixmap(WindowPtr window, > target_crtc = present_get_crtc(window); > } > > -present_get_ust_msc(screen, target_crtc, &ust, &crtc_msc); > +if (present_get_ust_msc(screen, target_crtc, &ust, &crtc_msc) != > Success) { > +/* Maybe we need to try a different CRTC? > + */ > +new_crtc = present_get_crtc(window); > + > +if (new_crtc != target_crtc && > +present_get_ust_msc(screen, new_crtc, &ust, &crtc_msc) == > Success) > +target_crtc = new_crtc; > +else { > +/* Fall back to fake MSC handling > + */ > +target_crtc = NULL; > +present_fake_get_ust_msc(screen, &ust, &crtc_msc); > +} > +} It survived for one more CRTC change, but still feeds passed msc into the wait_vblank. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/3] present: Query the Window's CRTC every time
On Tue, May 26, 2015 at 02:30:32PM -0700, Keith Packard wrote: > Michel Dänzer writes: > > > The old code also called present_get_crtc() unless pixmap == NULL, so > > the problem couldn't affect flips but only MSC waits. > > The original code was trying to let the client control which CRTC to > track for each window. For PresentPixmap requests, there's an explicit > CRTC argument, which allows the client to select the right one. For > PresentNotifyMSC, there's no such argument. > > So, the code was trying to respect the client's wishes by using > whichever CRTC was last associated with the window -- either implicitly > through the last call to present_get_crtc or explicitly from the last > crtc passed to PresentPixmap for the same window. > > Probably the right thing to do is to add an explicit CRTC parameter to > PresentNotifyMSC, and then have both requests call present_get_crtc when > an explicit CRTC is not provided. > > That doesn't solve the problem when an explicitly requested CRTC is > disabled though, so this fix doesn't address the root cause, only one of > the symptoms. > > > The problem I was hitting was that this code was running for an MSC wait > > when the CRTC referenced by window_priv->crtc was already disabled for > > DPMS off. This caused hangs at the GNOME lock screen. This patch seems > > to fix that problem. > > Why isn't your queue_vblank function bailing when asked to queue a > request for a CRTC which is disabled? If it simply fails, > present_execute will get called immediately and the client would > continue happily along. Oh, but it does fail gracefully. The problem is not that but that it sends a garbage msc to a valid CRTC. > > So, I vote for applying this patch (possibly with a better commit log) > > to master ASAP and backporting it to stable branches. > > It looks like this will only solve the problem of how to deal with new > PresentNotifyMSC requests; any PresentPixmap or PresentNotifyMSC > requests which are pending when the CRTC is disabled may end up getting > wedged too? Queued events are flushed when the CRTC is disabled. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 5/5] render: Eliminate temporary mask when drawing glyphs
On Tue, May 26, 2015 at 01:47:55PM -0700, Keith Packard wrote: > The temporary mask was supposed to do a better job when drawing glyphs > sharing pixels than just drawing them one at a time. However, the way > it was defined assumed that the glyphs didn't actually overlap, and > the computational cost for this minor adjustment is quite high. Nak. The mask is expected by users, who *already* discard the mask when they know that the rendering will be identical without it. The existing renderproto was written to render a union of the glyph path, which accounts for overlapping glyphs quite succinctly. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] renderproto: Fix compositeglyph dst position. eliminate use of mask-format
On Tue, May 26, 2015 at 01:55:09PM -0700, Keith Packard wrote: > Change the definition of composite glyphs from using an explicit > dst-x/dst-y location to match current implementation which uses the > dx/dy values from the first glyphitem for the destination location. > > Define the source pattern origin to align with that first glyph > location. > > Eliminate use of the mask-format in composite glyphs to clean up > rendering. Pardon? You are changing the expected rendering from being a union of the glyphs to rendering individual glyphs. That's a big change for the users who expect a union of the glyph path when specifying a mask - plus the effect the mask has for allowing combining a mixture of glyph formats without unwanted fringing. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] composite: Install SourceValidation hooks only when required
On Mon, May 18, 2015 at 03:26:29PM +0100, Chris Wilson wrote: > Signed-off-by: Chris Wilson Eek, used send-email from the wrong branch. This is fun but incomplete... -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH] composite: Install SourceValidation hooks only when required
Signed-off-by: Chris Wilson --- composite/compalloc.c | 3 +++ composite/compinit.c | 49 +++-- composite/compint.h| 4 composite/compwindow.c | 8 4 files changed, 46 insertions(+), 18 deletions(-) diff --git a/composite/compalloc.c b/composite/compalloc.c index 8daded0..c3973e9 100644 --- a/composite/compalloc.c +++ b/composite/compalloc.c @@ -75,6 +75,8 @@ compReportDamage(DamagePtr pDamage, RegionPtr pRegion, void *closure) CompScreenPtr cs = GetCompScreen(pScreen); CompWindowPtr cw = GetCompWindow(pWin); +compMarkDamaged(pScreen); + if (!cs->BlockHandler) { cs->BlockHandler = pScreen->BlockHandler; pScreen->BlockHandler = compBlockHandler; @@ -86,6 +88,7 @@ compReportDamage(DamagePtr pDamage, RegionPtr pRegion, void *closure) while (pWin) { if (pWin->damagedDescendants) break; + cs->anyDamaged++; pWin->damagedDescendants = TRUE; pWin = pWin->parent; } diff --git a/composite/compinit.c b/composite/compinit.c index 3ac075a..988bc58 100644 --- a/composite/compinit.c +++ b/composite/compinit.c @@ -77,9 +77,7 @@ compCloseScreen(ScreenPtr pScreen) pScreen->CopyWindow = cs->CopyWindow; pScreen->PositionWindow = cs->PositionWindow; -pScreen->GetImage = cs->GetImage; -pScreen->GetSpans = cs->GetSpans; -pScreen->SourceValidate = cs->SourceValidate; +assert(!cs->anyDamaged); free(cs); dixSetPrivate(&pScreen->devPrivates, CompScreenPrivateKey, NULL); @@ -147,8 +145,11 @@ compGetImage(DrawablePtr pDrawable, if (pDrawable->type == DRAWABLE_WINDOW) compPaintChildrenToWindow((WindowPtr) pDrawable); (*pScreen->GetImage) (pDrawable, sx, sy, w, h, format, planemask, pdstLine); -cs->GetImage = pScreen->GetImage; -pScreen->GetImage = compGetImage; + +if (cs->anyDamaged) { + cs->GetImage = pScreen->GetImage; + pScreen->GetImage = compGetImage; +} } static void @@ -162,8 +163,11 @@ compGetSpans(DrawablePtr pDrawable, int wMax, DDXPointPtr ppt, int *pwidth, if (pDrawable->type == DRAWABLE_WINDOW) compPaintChildrenToWindow((WindowPtr) pDrawable); (*pScreen->GetSpans) (pDrawable, wMax, ppt, pwidth, nspans, pdstStart); -cs->GetSpans = pScreen->GetSpans; -pScreen->GetSpans = compGetSpans; + +if (cs->anyDamaged) { + cs->GetSpans = pScreen->GetSpans; + pScreen->GetSpans = compGetSpans; +} } static void @@ -180,8 +184,26 @@ compSourceValidate(DrawablePtr pDrawable, if (pScreen->SourceValidate) (*pScreen->SourceValidate) (pDrawable, x, y, width, height, subWindowMode); -cs->SourceValidate = pScreen->SourceValidate; -pScreen->SourceValidate = compSourceValidate; +if (cs->anyDamaged) { + cs->SourceValidate = pScreen->SourceValidate; + pScreen->SourceValidate = compSourceValidate; +} +} + +void compMarkDamaged(ScreenPtr pScreen) +{ +CompScreenPtr cs = GetCompScreen(pScreen); + +if (cs->anyDamaged == 0) { + cs->GetImage = pScreen->GetImage; + pScreen->GetImage = compGetImage; + + cs->GetSpans = pScreen->GetSpans; + pScreen->GetSpans = compGetSpans; + + cs->SourceValidate = pScreen->SourceValidate; + pScreen->SourceValidate = compSourceValidate; +} } /* @@ -445,15 +467,6 @@ compScreenInit(ScreenPtr pScreen) cs->CloseScreen = pScreen->CloseScreen; pScreen->CloseScreen = compCloseScreen; -cs->GetImage = pScreen->GetImage; -pScreen->GetImage = compGetImage; - -cs->GetSpans = pScreen->GetSpans; -pScreen->GetSpans = compGetSpans; - -cs->SourceValidate = pScreen->SourceValidate; -pScreen->SourceValidate = compSourceValidate; - dixSetPrivate(&pScreen->devPrivates, CompScreenPrivateKey, cs); RegisterRealChildHeadProc(CompositeRealChildHead); diff --git a/composite/compint.h b/composite/compint.h index 09241f2..e6b046d 100644 --- a/composite/compint.h +++ b/composite/compint.h @@ -167,6 +167,7 @@ typedef struct _CompScreen { Window overlayWid; CompOverlayClientPtr pOverlayClients; +int anyDamaged; GetImageProcPtr GetImage; GetSpansProcPtr GetSpans; SourceValidateProcPtr SourceValidate; @@ -243,6 +244,9 @@ compReallocPixmap(WindowPtr pWin, int x, int y, Bool compScreenInit(ScreenPtr pScreen); +void + compMarkDamaged(ScreenPtr pScreen); + /* * compoverlay.c */ diff --git a/composite/compwindow.c b/composite/compwindow.c index 6eacbae..81f2a93 100644 --- a/composite/compwindow.c +++ b/composite/compwindow.c @@ -732,6 +732,13 @@ compPaintWindowToParent(WindowPtr pWin) } } +static void compClearDa
[PATCH] glamor/glyphs: Fix rendering regressions
commit b0d2e010316d710eb4052963de3a1e2dc7ba356e Author: Keith Packard Date: Fri Oct 10 09:25:51 2014 +0200 glamor: Replace CompositeGlyphs code [v2] introduced a number of regressions in both ignoring the effect of applying the user requested mask on the glyphs (for example to correctly render overlapping glyphs and to apply quantisation effects, e.g. mixing different filters) along with incorrectly computing the glyph source coordinates. Signed-off-by: Chris Wilson Cc: Keith Packard --- glamor/glamor_composite_glyphs.c | 130 --- 1 file changed, 122 insertions(+), 8 deletions(-) diff --git a/glamor/glamor_composite_glyphs.c b/glamor/glamor_composite_glyphs.c index 39ed854..3a21eed 100644 --- a/glamor/glamor_composite_glyphs.c +++ b/glamor/glamor_composite_glyphs.c @@ -316,6 +316,54 @@ glamor_atlas_for_glyph(glamor_screen_private *glamor_priv, DrawablePtr drawable) return glamor_priv->glyph_atlas_a; } +static void +GlyphExtents(int nlist, GlyphListPtr list, GlyphPtr * glyphs, BoxPtr extents) +{ +int x1, x2, y1, y2; +int n; +GlyphPtr glyph; +int x, y; + +x = 0; +y = 0; +extents->x1 = MAXSHORT; +extents->x2 = MINSHORT; +extents->y1 = MAXSHORT; +extents->y2 = MINSHORT; +while (nlist--) { +x += list->xOff; +y += list->yOff; +n = list->len; +list++; +while (n--) { +glyph = *glyphs++; +x1 = x - glyph->info.x; +if (x1 < MINSHORT) +x1 = MINSHORT; +y1 = y - glyph->info.y; +if (y1 < MINSHORT) +y1 = MINSHORT; +x2 = x1 + glyph->info.width; +if (x2 > MAXSHORT) +x2 = MAXSHORT; +y2 = y1 + glyph->info.height; +if (y2 > MAXSHORT) +y2 = MAXSHORT; +if (x1 < extents->x1) +extents->x1 = x1; +if (x2 > extents->x2) +extents->x2 = x2; +if (y1 < extents->y1) +extents->y1 = y1; +if (y2 > extents->y2) +extents->y2 = y2; +x += glyph->info.xOff; +y += glyph->info.yOff; +} +} +} + +#define NeedsComponent(f) (PICT_FORMAT_A(f) != 0 && PICT_FORMAT_RGB(f) != 0) void glamor_composite_glyphs(CARD8 op, PicturePtr src, @@ -329,6 +377,8 @@ glamor_composite_glyphs(CARD8 op, GLshort *v = NULL; DrawablePtr drawable = dst->pDrawable; ScreenPtr screen = drawable->pScreen; +PicturePtr mask = NULL; +PicturePtr glyph_dst, glyph_src; glamor_screen_private *glamor_priv = glamor_get_screen_private(screen); glamor_program *prog = NULL; PicturePtr glyph_pict = NULL; @@ -341,6 +391,57 @@ glamor_composite_glyphs(CARD8 op, int glyph_max_dim = glamor_priv->glyph_max_dim; int nglyph = 0; int screen_num = screen->myNum; +CARD32 glyph_op; +BoxRec extents; + +if (glyph_format) { + xRenderColor white = { 0x, 0x, 0x, 0x }; + CARD32 component_alpha; + PixmapPtr pixmap; + int error; + + GlyphExtents(nlist, list, glyphs, &extents); + pixmap = screen->CreatePixmap(screen, + extents.x2 - extents.x1, + extents.y2 - extents.y1, + glyph_format->depth, + CREATE_PIXMAP_USAGE_SCRATCH); + if (!pixmap) + return; + + component_alpha = NeedsComponent(glyph_format->format); + mask = CreatePicture(0, &pixmap->drawable, +glyph_format, +CPComponentAlpha, &component_alpha, +serverClient, &error); + screen->DestroyPixmap(pixmap); + if (!mask) + return; + + glyph_src = CreateSolidPicture(0, &white, &error); + + ValidatePicture(glyph_src); + ValidatePicture(mask); + + glamor_composite(PictOpClear, glyph_src, NULL, mask, +0, 0, +0, 0, +0, 0, +extents.x2 - extents.x1, +extents.y2 - extents.y1); + + glyph_dst = mask; + x += -extents.x1; + y += -extents.y1; + glyph_op = PictOpAdd; +} else { + glyph_dst = dst; + glyph_src = src; + glyph_op = op; +} + +x_src += list->xOff; +y_src += list->yOff; for (n = 0; n < nlist; n++) nglyph += list[n].len; @@ -373,12 +474,13 @@ glamor_composite_glyphs(CARD8 op
Re: [PATCH] modesetting: add tile property support (v2.1)
On Wed, Apr 01, 2015 at 10:31:43AM +1000, Dave Airlie wrote: > From: Dave Airlie > > This adds tiling support to the server modesetting driver, > it retrieves the tile info from the kernel and translates > it into the server format and exposes the property. > > v2.1: fix resetting tile property (Chris) > > Signed-off-by: Dave Airlie Reviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 1/2] Xv: Only stop the adaptors when the Pixmap is finally destroyed
Pixmaps are reference counted and DestroyPixmap is called for the removal of every reference. However, we only want to stop the adaptors writing into the Pixmap just before the Pixmap is finally destroyed, similar to how Windows are handled. Signed-off-by: Chris Wilson Cc: Ville Syrjälä --- Xext/xvmain.c | 82 +-- 1 file changed, 24 insertions(+), 58 deletions(-) diff --git a/Xext/xvmain.c b/Xext/xvmain.c index 0abf190..ad1b39d 100644 --- a/Xext/xvmain.c +++ b/Xext/xvmain.c @@ -327,36 +327,24 @@ XvGetRTPort(void) return XvRTPort; } -static Bool -XvDestroyPixmap(PixmapPtr pPix) +static void +XvStopAdaptors(DrawablePtr pDrawable) { -Bool status; -ScreenPtr pScreen; -XvScreenPtr pxvs; -XvAdaptorPtr pa; -int na; -XvPortPtr pp; -int np; - -pScreen = pPix->drawable.pScreen; - -SCREEN_PROLOGUE(pScreen, DestroyPixmap); - -pxvs = (XvScreenPtr) dixLookupPrivate(&pScreen->devPrivates, XvScreenKey); +ScreenPtr pScreen = pDrawable->pScreen; +XvScreenPtr pxvs = dixLookupPrivate(&pScreen->devPrivates, XvScreenKey); +XvAdaptorPtr pa = pxvs->pAdaptors; +int na = pxvs->nAdaptors; /* CHECK TO SEE IF THIS PORT IS IN USE */ - -pa = pxvs->pAdaptors; -na = pxvs->nAdaptors; while (na--) { -np = pa->nPorts; -pp = pa->pPorts; +XvPortPtr pp = pa->pPorts; +int np = pa->nPorts; while (np--) { -if (pp->pDraw == (DrawablePtr) pPix) { -XvdiSendVideoNotify(pp, pp->pDraw, XvPreempted); +if (pp->pDraw == pDrawable) { +XvdiSendVideoNotify(pp, pDrawable, XvPreempted); -(void) (*pp->pAdaptor->ddStopVideo) (pp, pp->pDraw); +(void) (*pp->pAdaptor->ddStopVideo) (pp, pDrawable); pp->pDraw = NULL; pp->client = NULL; @@ -366,9 +354,19 @@ XvDestroyPixmap(PixmapPtr pPix) } pa++; } +} -status = (*pScreen->DestroyPixmap) (pPix); +static Bool +XvDestroyPixmap(PixmapPtr pPix) +{ +ScreenPtr pScreen = pPix->drawable.pScreen; +Bool status; + +if (pPix->refcnt == 1) +XvStopAdaptors(&pPix->drawable); +SCREEN_PROLOGUE(pScreen, DestroyPixmap); +status = (*pScreen->DestroyPixmap) (pPix); SCREEN_EPILOGUE(pScreen, DestroyPixmap, XvDestroyPixmap); return status; @@ -378,45 +376,13 @@ XvDestroyPixmap(PixmapPtr pPix) static Bool XvDestroyWindow(WindowPtr pWin) { +ScreenPtr pScreen = pWin->drawable.pScreen; Bool status; -ScreenPtr pScreen; -XvScreenPtr pxvs; -XvAdaptorPtr pa; -int na; -XvPortPtr pp; -int np; -pScreen = pWin->drawable.pScreen; +XvStopAdaptors(&pWin->drawable); SCREEN_PROLOGUE(pScreen, DestroyWindow); - -pxvs = (XvScreenPtr) dixLookupPrivate(&pScreen->devPrivates, XvScreenKey); - -/* CHECK TO SEE IF THIS PORT IS IN USE */ - -pa = pxvs->pAdaptors; -na = pxvs->nAdaptors; -while (na--) { -np = pa->nPorts; -pp = pa->pPorts; - -while (np--) { -if (pp->pDraw == (DrawablePtr) pWin) { -XvdiSendVideoNotify(pp, pp->pDraw, XvPreempted); - -(void) (*pp->pAdaptor->ddStopVideo) (pp, pp->pDraw); - -pp->pDraw = NULL; -pp->client = NULL; -pp->time = currentTime; -} -pp++; -} -pa++; -} - status = (*pScreen->DestroyWindow) (pWin); - SCREEN_EPILOGUE(pScreen, DestroyWindow, XvDestroyWindow); return status; -- 2.1.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 2/2] prime: Don't lose SourceValidate on PixmapSyncDirtyHelper no-ops
We need to ignore the SourceValidate callback when copying across framebuffer pixels to slaved scanouts so that the swcursor and such is copied across (otherwise the swcursor SourceValidate would restore the pristine frontbuffer hiding the cursor in the slaves). However, we need to remember to put the SourceValidate callback back in place even for an early exit. Signed-off-by: Chris Wilson Cc: Dave Airlie --- dix/pixmap.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/dix/pixmap.c b/dix/pixmap.c index 00e298f..d691127 100644 --- a/dix/pixmap.c +++ b/dix/pixmap.c @@ -234,6 +234,14 @@ Bool PixmapSyncDirtyHelper(PixmapDirtyUpdatePtr dirty, RegionPtr dirty_region) PixmapPtr dst; SourceValidateProcPtr SourceValidate; +RegionTranslate(dirty_region, dirty->x, dirty->y); +RegionIntersect(dirty_region, dirty_region, region); + +if (RegionNil(dirty_region)) { +RegionUninit(dirty_region); +return FALSE; +} + /* * SourceValidate is used by the software cursor code * to pull the cursor off of the screen when reading @@ -243,14 +251,6 @@ Bool PixmapSyncDirtyHelper(PixmapDirtyUpdatePtr dirty, RegionPtr dirty_region) SourceValidate = pScreen->SourceValidate; pScreen->SourceValidate = NULL; -RegionTranslate(dirty_region, dirty->x, dirty->y); -RegionIntersect(dirty_region, dirty_region, region); - -if (RegionNil(dirty_region)) { -RegionUninit(dirty_region); -return FALSE; -} - dst = dirty->slave_dst->master_pixmap; if (!dst) dst = dirty->slave_dst; -- 2.1.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 3/3] modesetting: add tile property support (v2)
On Tue, Mar 31, 2015 at 10:07:53AM +0100, Chris Wilson wrote: > On Tue, Mar 31, 2015 at 01:47:26PM +1000, Dave Airlie wrote: > > From: Dave Airlie > > > > This adds tiling support to the server modesetting driver, > > it retrieves the tile info from the kernel and translates > > it into the server format and exposes the property. > > > > Signed-off-by: Dave Airlie > > --- > > > +if (drmmode_output->tile_blob) { > > +struct xf86CrtcTileInfo tile_info; > > + > > +if (xf86OutputParseKMSTile(drmmode_output->tile_blob->data, > > drmmode_output->tile_blob->length, &tile_info) == TRUE) > > +xf86OutputSetTile(output, &tile_info); > > +} > > How do we clear the previous tile? > > struct xf86CrtcTileInfo tile_info; > > memset(&tile_info, 0, sizeof(tile_info)); > if (drmmode_output->tile_blob) > xf86OutputParseKMSTile(drmmode_output->tile_blob->data, > drmmode_output->tile_blob->length, > &tile_info); > xf86OutputSetTile(output, &tile_info); Or maybe: struct xf86CrtcTileInfo tile_info, *set = NULL; if (drmmode_output->tile_blob && xf86OutputParseKMSTile(drmmode_output->tile_blob->data, drmmode_output->tile_blob->length, &tile_info)) set = &tile_info; xf86OutputSetTile(output, set); -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 3/3] modesetting: add tile property support (v2)
On Tue, Mar 31, 2015 at 01:47:26PM +1000, Dave Airlie wrote: > From: Dave Airlie > > This adds tiling support to the server modesetting driver, > it retrieves the tile info from the kernel and translates > it into the server format and exposes the property. > > Signed-off-by: Dave Airlie > --- > +if (drmmode_output->tile_blob) { > +struct xf86CrtcTileInfo tile_info; > + > +if (xf86OutputParseKMSTile(drmmode_output->tile_blob->data, > drmmode_output->tile_blob->length, &tile_info) == TRUE) > +xf86OutputSetTile(output, &tile_info); > +} How do we clear the previous tile? struct xf86CrtcTileInfo tile_info; memset(&tile_info, 0, sizeof(tile_info)); if (drmmode_output->tile_blob) xf86OutputParseKMSTile(drmmode_output->tile_blob->data, drmmode_output->tile_blob->length, &tile_info); xf86OutputSetTile(output, &tile_info); -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] glx/dri2: Disable AIGLX if indirect GLX is disabled
On Thu, Mar 05, 2015 at 01:18:51PM +0900, Michel Dänzer wrote: > On 04.03.2015 21:16, Chris Wilson wrote: > > There is no point in setting up the acceleration for indirect GLX if > > indirect GLX is itself disabled. > > > > Signed-off-by: Chris Wilson > > --- > > glx/glxdri2.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/glx/glxdri2.c b/glx/glxdri2.c > > index ec86a73..7cb0f28 100644 > > --- a/glx/glxdri2.c > > +++ b/glx/glxdri2.c > > @@ -945,6 +945,9 @@ __glXDRIscreenProbe(ScreenPtr pScreen) > > size_t buffer_size; > > ScrnInfoPtr pScrn = xf86ScreenToScrn(pScreen); > > > > +if (!enableIndirectGLX) > > + return NULL; > > + > > screen = calloc(1, sizeof *screen); > > if (screen == NULL) > > return NULL; > > > > Can the GLX visual information still be probed from the DRI driver with > this patch? No. This patch kills the AIGLX messages and also DRI. Sadly as probing mesa/i965 is the single most CPU expensive step in starting X. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH] glx/dri2: Disable AIGLX if indirect GLX is disabled
There is no point in setting up the acceleration for indirect GLX if indirect GLX is itself disabled. Signed-off-by: Chris Wilson --- glx/glxdri2.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/glx/glxdri2.c b/glx/glxdri2.c index ec86a73..7cb0f28 100644 --- a/glx/glxdri2.c +++ b/glx/glxdri2.c @@ -945,6 +945,9 @@ __glXDRIscreenProbe(ScreenPtr pScreen) size_t buffer_size; ScrnInfoPtr pScrn = xf86ScreenToScrn(pScreen); +if (!enableIndirectGLX) + return NULL; + screen = calloc(1, sizeof *screen); if (screen == NULL) return NULL; -- 2.1.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] damage: Only track extents where possible
On Thu, Feb 26, 2015 at 02:58:45PM -0500, Adam Jackson wrote: > On Wed, 2015-02-25 at 18:01 +0000, Chris Wilson wrote: > > For external Damage, we need only track sufficient information to > > satisfy the DamageReportLevel. That is if the Client only wishes to hear > > that the Damage is now non-empty or if the extents change, we only need > > to track the extents of the Damage and can discard the actual > > rectangles. This speeds up the union operation, speeding up damage > > processing for Client as well - with a noticeable increase in > > performance of gnome-shell (which uses DamageReportBoundingBox) for > > example. > > I like the idea, not quite sold on the realization. > > > Internal users of Damage have access to the DamageRegion irrespective of > > the DamageReportLevel and so we need to keep the full region intact for > > them. > > That's not quite what isInternal means, it's currently used to hide > software sprite rendering from the protocol event stream. xwl and > composite also create their damages with isInternal FALSE. For xwl that > could just be fixed since it's not initializing midc, but composite > really does want that suppression to work. > > So I guess the question is whether automatic windows should take the > bounding box snap too, and I guess whichever method makes x11perf look > better when 'xcompmgr -a' is good enough for me. x11perf -aa10text certainly prefers extents-only tracking here. I don't imagine any other test would regress, but I can check for completeness. > > +static void DamageCombineExtents(DamagePtr pDamage, RegionPtr > > pDamageRegion) > > +{ > > +if (!pDamage->isInternal) { > > +RegionUninit(pDamageRegion); > > +RegionUnion(&pDamage->damage, &pDamage->damage, pDamageRegion); > > +RegionUninit(&pDamage->damage); > > +} else > > +RegionUnion(&pDamage->damage, &pDamage->damage, pDamageRegion); > > +} > > First time I've seen this idiom of relying on RegionUninit to empty the > rect list but leave the bounding box. I don't necessarily have an issue > with it but a comment would be nice. It was a bit cheeky, and it definitely relies on the current code in regionstr.h. Looking at it again, we can't discard the rectangles from pDamageRegion as we may be called before the op and so required to keep pPendingDamage intact. > > @@ -1929,7 +1939,7 @@ DamageReportDamage(DamagePtr pDamage, RegionPtr > > pDamageRegion) > > break; > > case DamageReportBoundingBox: > > tmpBox = *RegionExtents(&pDamage->damage); > > -RegionUnion(&pDamage->damage, &pDamage->damage, pDamageRegion); > > +DamageCombineExtents(pDamage, pDamageRegion); > > if (!BOX_SAME(&tmpBox, RegionExtents(&pDamage->damage))) { > > (*pDamage->damageReport) (pDamage, &pDamage->damage, > >pDamage->closure); > > Presumably this half doesn't even need to inspect isInternal, since the > bounding box is all that's requested in any case. There are no in-tree > BoundingBox consumers so nothing can be relying on getting anything more > detailed in the report hook, and the protocol event snaps to the box > anyway. In tree users: DamageReportNone: ephyr - region is used for host migration exa - region is used for video memory migrations modesetting - region is used to limit shadow updates randr - region is used during composite shadow(mi) - region is exposed to clients DamageReportNonEmpty: composite - region is used exa - region is used glamor - region ignored xwayland - only extents used DamageReportRawRegion: shadow(xf86) - rectangles are exposed to clients The DamageReportNone and DamageReportRawRegion the rectangles are part of the respective API (though we could just report the extents rectangle), but for e.g. drm/udl (modesetting, shadow) at least we do want the raw rectangles and the migration would be very expensive. randr doesn't know if it will be quick or slow, so must err on the side of tracking the rectangles. ephyr should be able to determine if it has quick updates via DRI2/SHM or pushing pixels over the wire. As for DamageReportNonEmpty, exa has to pessimise that pixel data is slow to readback and so should track rectangles. xwayland/glamor already ignore the rectangles. composite however doesn't know whether it will be faster to ignore rectangles (because the copy is hwaccel) or to track rectangles... Which is where we can make a decision based on xvfb + xcompgr -a (and also check with the major ddx). So because of exa, I think we want a DamageSetTrackExtentsOnly() function call so that consumers can opt out of the full region tracking. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH] damage: Only track extents where possible
For external Damage, we need only track sufficient information to satisfy the DamageReportLevel. That is if the Client only wishes to hear that the Damage is now non-empty or if the extents change, we only need to track the extents of the Damage and can discard the actual rectangles. This speeds up the union operation, speeding up damage processing for Client as well - with a noticeable increase in performance of gnome-shell (which uses DamageReportBoundingBox) for example. Internal users of Damage have access to the DamageRegion irrespective of the DamageReportLevel and so we need to keep the full region intact for them. Signed-off-by: Chris Wilson Cc: Adam Jackson --- miext/damage/damage.c | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/miext/damage/damage.c b/miext/damage/damage.c index b99cfb0..450a517 100644 --- a/miext/damage/damage.c +++ b/miext/damage/damage.c @@ -1906,6 +1906,16 @@ DamageGetScreenFuncs(ScreenPtr pScreen) return &pScrPriv->funcs; } +static void DamageCombineExtents(DamagePtr pDamage, RegionPtr pDamageRegion) +{ +if (!pDamage->isInternal) { +RegionUninit(pDamageRegion); +RegionUnion(&pDamage->damage, &pDamage->damage, pDamageRegion); +RegionUninit(&pDamage->damage); +} else +RegionUnion(&pDamage->damage, &pDamage->damage, pDamageRegion); +} + void DamageReportDamage(DamagePtr pDamage, RegionPtr pDamageRegion) { @@ -1929,7 +1939,7 @@ DamageReportDamage(DamagePtr pDamage, RegionPtr pDamageRegion) break; case DamageReportBoundingBox: tmpBox = *RegionExtents(&pDamage->damage); -RegionUnion(&pDamage->damage, &pDamage->damage, pDamageRegion); +DamageCombineExtents(pDamage, pDamageRegion); if (!BOX_SAME(&tmpBox, RegionExtents(&pDamage->damage))) { (*pDamage->damageReport) (pDamage, &pDamage->damage, pDamage->closure); @@ -1937,7 +1947,7 @@ DamageReportDamage(DamagePtr pDamage, RegionPtr pDamageRegion) break; case DamageReportNonEmpty: was_empty = !RegionNotEmpty(&pDamage->damage); -RegionUnion(&pDamage->damage, &pDamage->damage, pDamageRegion); +DamageCombineExtents(pDamage, pDamageRegion); if (was_empty && RegionNotEmpty(&pDamage->damage)) { (*pDamage->damageReport) (pDamage, &pDamage->damage, pDamage->closure); -- 2.1.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 3/3] dri2: Only create one unnamed reference per Drawable per Client
This workarounds issues in mesa and Mali that likes to create a new DRI2Drawble per context and per frame, respectively. The idea is that we only create one unnamed reference per Client on each Drawable that is never destroyed - and we make the assumption that the only caller is the DRI2 dispatcher and thus we don't need to check that the invalidate handler is the same. Every DRI2Drawable reference sends an invalidate event, and so not only do we have a slow memory leak, we also suffer a performance loss as the Clients create more and more references to the same Drawable - unless we limit them to a single reference each. The issue was first reported 5 years ago by Pauli Nieminen and I have incorporated his patches here. His improvement was to add a reference count to the per-Client unnamed reference and by doing so could support DRI2DestroyDrawable again. However, it remains the case that as the lifecycles between the GLXDrawable (DRI2Drawable) and the parent Drawable are decoupled, mesa cannot call DRI2DestroyDrawable on Drawables it did not create (or else risk generating BadDrawable runtime errors), see mesa commit 4ebf07a426771b62123e5fcb5a8be0de24037af1 Author: Kristian Høgsberg Date: Mon Sep 13 08:39:42 2010 -0400 glx: Don't destroy DRI2 drawables for legacy glx drawables For GLX 1.3 drawables, we can destroy the DRI2 drawable when the GLX drawable is destroyed. However, for legacy drawables, there os no good way of knowing when the application is done with it, so we just let the DRI2 drawable linger on the server. The server will destroy the DRI2 drawable when it destroys the X drawable or the client exits anyway. https://bugs.freedesktop.org/show_bug.cgi?id=30109 and as such it rules out both using named references by the Clients and the reference ever being reaped. v2: Incorporate refcounting of the unnamed reference by Pauli Nieminen. Cc: Daniel Drake Link: https://freedesktop.org/patch/19695/ Link: http://lists.x.org/archives/xorg-devel/2010-November/014783.html Link: http://lists.x.org/archives/xorg-devel/2010-November/014782.html Signed-off-by: Chris Wilson Cc: Ville Syrjälä --- hw/xfree86/dri2/dri2.c| 57 +++ hw/xfree86/dri2/dri2ext.c | 4 ++-- 2 files changed, 55 insertions(+), 6 deletions(-) diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c index af286e6..065289d 100644 --- a/hw/xfree86/dri2/dri2.c +++ b/hw/xfree86/dri2/dri2.c @@ -279,11 +279,24 @@ DRI2SwapLimit(DrawablePtr pDraw, int swap_limit) typedef struct DRI2DrawableRefRec { XID pid; +unsigned int refcnt; DRI2InvalidateProcPtr invalidate; void *priv; struct xorg_list link; } DRI2DrawableRefRec, *DRI2DrawableRefPtr; +static DRI2DrawableRefPtr +DRI2FindClientDrawableRef(ClientPtr client, DRI2DrawablePtr pPriv) +{ +DRI2DrawableRefPtr ref; + +xorg_list_for_each_entry(ref, &pPriv->reference_list, link) +if (ref->refcnt && CLIENT_ID(ref->pid) == client->index) +return ref; + +return NULL; +} + int DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID pid, DRI2InvalidateProcPtr invalidate, void *priv) @@ -299,16 +312,34 @@ DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID pid, pPriv->prime_id = dri2ClientPrivate(client)->prime_id; +if (pid == 0) { +ref = DRI2FindClientDrawableRef(client, pPriv); +if (ref) { +ref->refcnt++; +assert(ref->invalidate == invalidate); +assert(ref->priv == priv); +return Success; +} +} + ref = malloc(sizeof *ref); if (ref == NULL) return BadAlloc; -if (!AddResource(pid, dri2ReferenceRes, ref)) { +if (pid == 0) { +ref->refcnt = 1; +ref->pid = FakeClientID(client->index); +} +else { +ref->refcnt = 0; +ref->pid = pid; +} + +if (!AddResource(ref->pid, dri2ReferenceRes, ref)) { free(ref); return BadAlloc; } -ref->pid = pid; ref->invalidate = invalidate; ref->priv = priv; xorg_list_add(&ref->link, &pPriv->reference_list); @@ -317,9 +348,27 @@ DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID pid, } int -DRI2DestroyDrawable(ClientPtr client, DrawablePtr pDraw, XID id) +DRI2DestroyDrawable(ClientPtr client, DrawablePtr pDraw, XID pid) { -FreeResourceByType(id, dri2ReferenceRes, FALSE); +if (pid == 0) { +DRI2DrawablePtr pPriv; +DRI2DrawableRefPtr ref; + +pPriv = DRI2GetDrawable(pDraw); +if (pPriv == NULL) +return BadMatch; + +ref = DRI2FindClientDrawableRef(client, pPriv); +if (ref == NULL) +return BadMatch; + +if (--ref->refcnt == 0) +pid = ref->pid; +} + +if (pid != 0) +FreeResour
[PATCH 1/3] dri2: Only invalidate the immediate Window upon SetWindowPixmap
All callers of SetWindowPixmap will themselves be traversing the Window heirachy updating the backing Pixmap of each child and so we can forgo doing the identical traversal inside the DRI2SetWindowPixmap handler. Reported-by: Signed-off-by: Chris Wilson --- hw/xfree86/dri2/dri2.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c index e37f2de..1f33d16 100644 --- a/hw/xfree86/dri2/dri2.c +++ b/hw/xfree86/dri2/dri2.c @@ -1406,8 +1406,7 @@ DRI2ConfigNotify(WindowPtr pWin, int x, int y, int w, int h, int bw, static void DRI2SetWindowPixmap(WindowPtr pWin, PixmapPtr pPix) { -DrawablePtr pDraw = (DrawablePtr) pWin; -ScreenPtr pScreen = pDraw->pScreen; +ScreenPtr pScreen = pWin->drawable.pScreen; DRI2ScreenPtr ds = DRI2GetScreen(pScreen); pScreen->SetWindowPixmap = ds->SetWindowPixmap; @@ -1415,7 +1414,7 @@ DRI2SetWindowPixmap(WindowPtr pWin, PixmapPtr pPix) ds->SetWindowPixmap = pScreen->SetWindowPixmap; pScreen->SetWindowPixmap = DRI2SetWindowPixmap; -DRI2InvalidateDrawableAll(pDraw); +DRI2InvalidateDrawable(&pWin->drawable); } #define MAX_PRIME DRI2DriverPrimeMask -- 2.1.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 2/3] dri2: Split resource tracking for DRI2Drawable and references to them
In order to ease resource tracking, disentangle DRI2Drawable XIDs from their references. This will be used in later patches to first limit the object leak from unnamed references created on behalf of Clients and then never destroy, and then to allow Clients to explicit manage their references to DRI2Drawables. Signed-off-by: Chris Wilson --- glx/glxdri2.c | 10 ++-- hw/xfree86/dri2/dri2.c| 136 ++ hw/xfree86/dri2/dri2.h| 11 ++-- hw/xfree86/dri2/dri2ext.c | 6 +- 4 files changed, 66 insertions(+), 97 deletions(-) diff --git a/glx/glxdri2.c b/glx/glxdri2.c index 9d37b32..2bfacaa 100644 --- a/glx/glxdri2.c +++ b/glx/glxdri2.c @@ -105,7 +105,7 @@ __glXDRIdrawableDestroy(__GLXdrawable * drawable) __GLXDRIdrawable *private = (__GLXDRIdrawable *) drawable; const __DRIcoreExtension *core = private->screen->core; -FreeResource(private->dri2_id, FALSE); +DRI2DestroyDrawable(NULL, private->base.pDraw, private->dri2_id); (*core->destroyDrawable) (private->driDrawable); @@ -610,7 +610,7 @@ __glXDRIscreenCreateContext(__GLXscreen * baseScreen, } static void -__glXDRIinvalidateBuffers(DrawablePtr pDraw, void *priv, XID id) +__glXDRIinvalidateBuffers(DrawablePtr pDraw, void *priv) { __GLXDRIdrawable *private = priv; __GLXDRIscreen *screen = private->screen; @@ -649,9 +649,9 @@ __glXDRIscreenCreateDrawable(ClientPtr client, private->base.waitGL = __glXDRIdrawableWaitGL; private->base.waitX = __glXDRIdrawableWaitX; -ret = DRI2CreateDrawable2(client, pDraw, drawId, - __glXDRIinvalidateBuffers, private, - &private->dri2_id); +private->dri2_id = FakeClientID(client->index); +ret = DRI2CreateDrawable(client, pDraw, private->dri2_id, + __glXDRIinvalidateBuffers, private); if (cx != lastGLContext) { lastGLContext = cx; cx->makeCurrent(cx); diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c index 1f33d16..af286e6 100644 --- a/hw/xfree86/dri2/dri2.c +++ b/hw/xfree86/dri2/dri2.c @@ -70,16 +70,16 @@ static DevPrivateKeyRec dri2PixmapPrivateKeyRec; static DevPrivateKeyRec dri2ClientPrivateKeyRec; -#define dri2ClientPrivateKey (&dri2ClientPrivateKeyRec) - -#define dri2ClientPrivate(_pClient) (dixLookupPrivate(&(_pClient)->devPrivates, \ - dri2ClientPrivateKey)) - typedef struct _DRI2Client { int prime_id; } DRI2ClientRec, *DRI2ClientPtr; -static RESTYPE dri2DrawableRes; +static DRI2ClientPtr dri2ClientPrivate(ClientPtr pClient) +{ +return dixLookupPrivate(&pClient->devPrivates, &dri2ClientPrivateKeyRec); +} + +static RESTYPE dri2DrawableRes, dri2ReferenceRes; typedef struct _DRI2Screen *DRI2ScreenPtr; @@ -207,6 +207,11 @@ DRI2AllocateDrawable(DrawablePtr pDraw) if (pPriv == NULL) return NULL; +if (!AddResource(pDraw->id, dri2DrawableRes, pPriv)) { +free(pPriv); +return NULL; +} + pPriv->dri2_screen = ds; pPriv->drawable = pDraw; pPriv->width = pDraw->width; @@ -273,49 +278,37 @@ DRI2SwapLimit(DrawablePtr pDraw, int swap_limit) } typedef struct DRI2DrawableRefRec { -XID id; -XID dri2_id; +XID pid; DRI2InvalidateProcPtr invalidate; void *priv; struct xorg_list link; } DRI2DrawableRefRec, *DRI2DrawableRefPtr; -static DRI2DrawableRefPtr -DRI2LookupDrawableRef(DRI2DrawablePtr pPriv, XID id) +int +DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID pid, + DRI2InvalidateProcPtr invalidate, void *priv) { +DRI2DrawablePtr pPriv; DRI2DrawableRefPtr ref; -xorg_list_for_each_entry(ref, &pPriv->reference_list, link) { -if (ref->id == id) -return ref; -} - -return NULL; -} +pPriv = DRI2GetDrawable(pDraw); +if (pPriv == NULL) +pPriv = DRI2AllocateDrawable(pDraw); +if (pPriv == NULL) +return BadAlloc; -static int -DRI2AddDrawableRef(DRI2DrawablePtr pPriv, XID id, XID dri2_id, - DRI2InvalidateProcPtr invalidate, void *priv) -{ -DRI2DrawableRefPtr ref; +pPriv->prime_id = dri2ClientPrivate(client)->prime_id; ref = malloc(sizeof *ref); if (ref == NULL) return BadAlloc; -if (!AddResource(dri2_id, dri2DrawableRes, pPriv)) { +if (!AddResource(pid, dri2ReferenceRes, ref)) { free(ref); return BadAlloc; } -if (!DRI2LookupDrawableRef(pPriv, id)) -if (!AddResource(id, dri2DrawableRes, pPriv)) { -FreeResourceByType(dri2_id, dri2DrawableRes, TRUE); -free(ref); -return BadAlloc; -} -ref->id = id; -ref->dri2_id = dri2_id; +ref->pid = pid; ref->invalidate = invalidate;
Re: Limit DRI2Drawable reference leak
On Sun, Feb 22, 2015 at 09:02:41AM +, Chris Wilson wrote: > Yes, that is what I think the mesa patch is about. I think the solution > would have been that the DRI2Drawable take a refcnt on its parent and > then DRI2DestroyDrawable could be made to work. However, if we did that > today we would end up with a massive Drawable leak - and pushing the > change to mesa would also then expose us to the GLXDrawable vs Drawable > close race (and BadDrawable errors again). This also sinks using named > references - unless only named references also reference their parent. Except Windows don't have reference counts. > I think this is something we cannot fix, but we can at least apply > damage limitation. Doubly so now. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: Limit DRI2Drawable reference leak
On Sun, Feb 22, 2015 at 02:09:36AM +0200, Ville Syrjälä wrote: > On Sat, Feb 21, 2015 at 10:52:49PM +0000, Chris Wilson wrote: > > On Sun, Feb 22, 2015 at 12:13:38AM +0200, Ville Syrjälä wrote: > > > On Sat, Feb 21, 2015 at 09:31:07PM +, Chris Wilson wrote: > > > > With the current protocol and implementations, we have to often call > > > > DRI2CreateDrawable but can never call DRI2DestroyDrawable. This ends up > > > > with us leaking references to DRI2Drawables based on the assumption that > > > > the references have identical lifetimes to the Drawable going astray. > > > > This was spotted by Daniel Drake as the mali driver would create a new > > > > reference to the DRI2Drawable on every GetBuffers, but it can also be > > > > observed in mesa when running synthetic benchmarks (creating lots of > > > > contexts/glxdrawables for each window) and to a lesser extent with > > > > normal composited operation. > > > > > > > > The first two patches implement the capping of the unnamed internal > > > > reference used by DRI2CreateDrawable to just one per Client. > > > > > > IIRC we had many issues around the dri2 reference stuff during the > > > Maemo days. Pauli fixed tons of problems in the dri2 code but some > > > of the patches never made it in. > > > > > > These seem somewhat relevant: > > > http://lists.x.org/archives/xorg-devel/2010-November/014783.html > > > http://lists.x.org/archives/xorg-devel/2010-November/014782.html > > > > Indeed. Same problem, similar solution. I was a bit dubious as to > > whether we could hook up DRI2DestroyDrawable after all this time (for > > example mesa ignores it except for GLXPixmaps) and feared there was some > > corner case that was going to explode. > > Yeah dunno. This code did ship in the N9/N950 and our client side did > issue these protocol requests appropriately. But then again, Pauli did > a boatload of additional work on the dri2 code after these that didn't > make it into upstream either. > > As I recall one of the issues was clients getting BadDrawables from > DRI2DestroyDrawable if the X drawable already went away. And the > solution was to decouple the lifetimes of the two. Yes, that is what I think the mesa patch is about. I think the solution would have been that the DRI2Drawable take a refcnt on its parent and then DRI2DestroyDrawable could be made to work. However, if we did that today we would end up with a massive Drawable leak - and pushing the change to mesa would also then expose us to the GLXDrawable vs Drawable close race (and BadDrawable errors again). This also sinks using named references - unless only named references also reference their parent. I think this is something we cannot fix, but we can at least apply damage limitation. > I've probably forgotten most of what we did back then, but interested > parties may go dig through > https://www.gitorious.org/meego-w40/xserver-xorg if they wish. Ta, -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: Limit DRI2Drawable reference leak
On Sun, Feb 22, 2015 at 12:13:38AM +0200, Ville Syrjälä wrote: > On Sat, Feb 21, 2015 at 09:31:07PM +0000, Chris Wilson wrote: > > With the current protocol and implementations, we have to often call > > DRI2CreateDrawable but can never call DRI2DestroyDrawable. This ends up > > with us leaking references to DRI2Drawables based on the assumption that > > the references have identical lifetimes to the Drawable going astray. > > This was spotted by Daniel Drake as the mali driver would create a new > > reference to the DRI2Drawable on every GetBuffers, but it can also be > > observed in mesa when running synthetic benchmarks (creating lots of > > contexts/glxdrawables for each window) and to a lesser extent with > > normal composited operation. > > > > The first two patches implement the capping of the unnamed internal > > reference used by DRI2CreateDrawable to just one per Client. > > IIRC we had many issues around the dri2 reference stuff during the > Maemo days. Pauli fixed tons of problems in the dri2 code but some > of the patches never made it in. > > These seem somewhat relevant: > http://lists.x.org/archives/xorg-devel/2010-November/014783.html > http://lists.x.org/archives/xorg-devel/2010-November/014782.html Indeed. Same problem, similar solution. I was a bit dubious as to whether we could hook up DRI2DestroyDrawable after all this time (for example mesa ignores it except for GLXPixmaps) and feared there was some corner case that was going to explode. Separating the two resource types cleans up the code slightly and should speed it as well (if many references do persist). Further info, the leak is a result of the conversion in: xserver commit 1da1f33f2dd5b437dd56cd9f5d6782de4ad5a1bc Author: Kristian Høgsberg Date: Fri Apr 16 05:55:34 2010 -0400 DRI2: Track DRI2 drawables as resources, not privates The main motivation here is to have the resource system clean up the DRI2 drawable automatically so glx doesn't have to. Right now, the glx drawable resource must be destroyed before the X drawable, so that calling DRI2DestroyDrawable doesn't crash. By making the DRI2 drawable a resource, GLX doesn't have to worry about that and the resource destruction order becomes irrelevant. But the scary one is mesa commit 4ebf07a426771b62123e5fcb5a8be0de24037af1 Author: Kristian Høgsberg Date: Mon Sep 13 08:39:42 2010 -0400 glx: Don't destroy DRI2 drawables for legacy glx drawables For GLX 1.3 drawables, we can destroy the DRI2 drawable when the GLX drawable is destroyed. However, for legacy drawables, there os no good way of knowing when the application is done with it, so we just let the DRI2 drawable linger on the server. The server will destroy the DRI2 drawable when it destroys the X drawable or the client exits anyway. https://bugs.freedesktop.org/show_bug.cgi?id=30109 Which leads me to believe that pushing the named references out to the Clients requires less thinking... -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Limit DRI2Drawable reference leak
With the current protocol and implementations, we have to often call DRI2CreateDrawable but can never call DRI2DestroyDrawable. This ends up with us leaking references to DRI2Drawables based on the assumption that the references have identical lifetimes to the Drawable going astray. This was spotted by Daniel Drake as the mali driver would create a new reference to the DRI2Drawable on every GetBuffers, but it can also be observed in mesa when running synthetic benchmarks (creating lots of contexts/glxdrawables for each window) and to a lesser extent with normal composited operation. The first two patches implement the capping of the unnamed internal reference used by DRI2CreateDrawable to just one per Client. The third patch uses a pair of new DRI2 requests to allow the Client to explicitly manage the lifetime via use of a named reference to the DRI2Drawable. -Chris ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 3/3] dri2: Support named DRI2Drawable references
Allow the Client to manage its references to the DRI2Drawable by providing named references whose lifetimes can be managed with DRI2CreateDrawable2 and DRI2DestroyDrawable2. This stops the Client from relying on such references being cleaned up along with the Drawable. As each stale reference still causes an invalidate event to be sent, the overhead of sending those unnecessary events quickly mounts. Signed-off-by: Chris Wilson --- hw/xfree86/dri2/dri2ext.c | 37 + 1 file changed, 37 insertions(+) diff --git a/hw/xfree86/dri2/dri2ext.c b/hw/xfree86/dri2/dri2ext.c index 8e70bc5..270280b 100644 --- a/hw/xfree86/dri2/dri2ext.c +++ b/hw/xfree86/dri2/dri2ext.c @@ -194,6 +194,24 @@ ProcDRI2CreateDrawable(ClientPtr client) } static int +ProcDRI2CreateDrawable2(ClientPtr client) +{ +REQUEST(xDRI2CreateDrawable2Req); +DrawablePtr pDrawable; +int status; + +REQUEST_SIZE_MATCH(xDRI2CreateDrawable2Req); +LEGAL_NEW_RESOURCE(stuff->pid, client); + +if (!validDrawable(client, stuff->drawable, DixAddAccess, + &pDrawable, &status)) +return status; + +return DRI2CreateDrawable(client, pDrawable, stuff->pid, + DRI2InvalidateBuffersEvent, client); +} + +static int ProcDRI2DestroyDrawable(ClientPtr client) { REQUEST(xDRI2DestroyDrawableReq); @@ -209,6 +227,21 @@ ProcDRI2DestroyDrawable(ClientPtr client) } static int +ProcDRI2DestroyDrawable2(ClientPtr client) +{ +REQUEST(xDRI2DestroyDrawable2Req); +DrawablePtr pDrawable; +int status; + +REQUEST_SIZE_MATCH(xDRI2DestroyDrawable2Req); +if (!validDrawable(client, stuff->drawable, DixRemoveAccess, + &pDrawable, &status)) +return status; + +return DRI2DestroyDrawable(client, pDrawable, stuff->pid); +} + +static int send_buffers_reply(ClientPtr client, DrawablePtr pDrawable, DRI2BufferPtr * buffers, int count, int width, int height) { @@ -601,8 +634,12 @@ ProcDRI2Dispatch(ClientPtr client) return ProcDRI2Authenticate(client); case X_DRI2CreateDrawable: return ProcDRI2CreateDrawable(client); +case X_DRI2CreateDrawable2: +return ProcDRI2CreateDrawable2(client); case X_DRI2DestroyDrawable: return ProcDRI2DestroyDrawable(client); +case X_DRI2DestroyDrawable2: +return ProcDRI2DestroyDrawable2(client); case X_DRI2GetBuffers: return ProcDRI2GetBuffers(client); case X_DRI2CopyRegion: -- 2.1.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 1/3] dri2: Split resource tracking for DRI2Drawable and references to them
In order to ease resource tracking, disentangle DRI2Drawable XIDs from their references. This will be used in later patches to first limit the object leak from unnamed references created on behalf of Clients and then never destroy, and then to allow Clients to explicit manage their references to DRI2Drawables. Signed-off-by: Chris Wilson --- glx/glxdri2.c | 8 +-- hw/xfree86/dri2/dri2.c| 136 ++ hw/xfree86/dri2/dri2.h| 11 ++-- hw/xfree86/dri2/dri2ext.c | 6 +- 4 files changed, 65 insertions(+), 96 deletions(-) diff --git a/glx/glxdri2.c b/glx/glxdri2.c index 9d37b32..1d5f9db 100644 --- a/glx/glxdri2.c +++ b/glx/glxdri2.c @@ -610,7 +610,7 @@ __glXDRIscreenCreateContext(__GLXscreen * baseScreen, } static void -__glXDRIinvalidateBuffers(DrawablePtr pDraw, void *priv, XID id) +__glXDRIinvalidateBuffers(DrawablePtr pDraw, void *priv) { __GLXDRIdrawable *private = priv; __GLXDRIscreen *screen = private->screen; @@ -649,9 +649,9 @@ __glXDRIscreenCreateDrawable(ClientPtr client, private->base.waitGL = __glXDRIdrawableWaitGL; private->base.waitX = __glXDRIdrawableWaitX; -ret = DRI2CreateDrawable2(client, pDraw, drawId, - __glXDRIinvalidateBuffers, private, - &private->dri2_id); +private->dri2_id = FakeClientID(client->index); +ret = DRI2CreateDrawable(client, pDraw, private->dri2_id, + __glXDRIinvalidateBuffers, private); if (cx != lastGLContext) { lastGLContext = cx; cx->makeCurrent(cx); diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c index 1f33d16..af286e6 100644 --- a/hw/xfree86/dri2/dri2.c +++ b/hw/xfree86/dri2/dri2.c @@ -70,16 +70,16 @@ static DevPrivateKeyRec dri2PixmapPrivateKeyRec; static DevPrivateKeyRec dri2ClientPrivateKeyRec; -#define dri2ClientPrivateKey (&dri2ClientPrivateKeyRec) - -#define dri2ClientPrivate(_pClient) (dixLookupPrivate(&(_pClient)->devPrivates, \ - dri2ClientPrivateKey)) - typedef struct _DRI2Client { int prime_id; } DRI2ClientRec, *DRI2ClientPtr; -static RESTYPE dri2DrawableRes; +static DRI2ClientPtr dri2ClientPrivate(ClientPtr pClient) +{ +return dixLookupPrivate(&pClient->devPrivates, &dri2ClientPrivateKeyRec); +} + +static RESTYPE dri2DrawableRes, dri2ReferenceRes; typedef struct _DRI2Screen *DRI2ScreenPtr; @@ -207,6 +207,11 @@ DRI2AllocateDrawable(DrawablePtr pDraw) if (pPriv == NULL) return NULL; +if (!AddResource(pDraw->id, dri2DrawableRes, pPriv)) { +free(pPriv); +return NULL; +} + pPriv->dri2_screen = ds; pPriv->drawable = pDraw; pPriv->width = pDraw->width; @@ -273,49 +278,37 @@ DRI2SwapLimit(DrawablePtr pDraw, int swap_limit) } typedef struct DRI2DrawableRefRec { -XID id; -XID dri2_id; +XID pid; DRI2InvalidateProcPtr invalidate; void *priv; struct xorg_list link; } DRI2DrawableRefRec, *DRI2DrawableRefPtr; -static DRI2DrawableRefPtr -DRI2LookupDrawableRef(DRI2DrawablePtr pPriv, XID id) +int +DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID pid, + DRI2InvalidateProcPtr invalidate, void *priv) { +DRI2DrawablePtr pPriv; DRI2DrawableRefPtr ref; -xorg_list_for_each_entry(ref, &pPriv->reference_list, link) { -if (ref->id == id) -return ref; -} - -return NULL; -} +pPriv = DRI2GetDrawable(pDraw); +if (pPriv == NULL) +pPriv = DRI2AllocateDrawable(pDraw); +if (pPriv == NULL) +return BadAlloc; -static int -DRI2AddDrawableRef(DRI2DrawablePtr pPriv, XID id, XID dri2_id, - DRI2InvalidateProcPtr invalidate, void *priv) -{ -DRI2DrawableRefPtr ref; +pPriv->prime_id = dri2ClientPrivate(client)->prime_id; ref = malloc(sizeof *ref); if (ref == NULL) return BadAlloc; -if (!AddResource(dri2_id, dri2DrawableRes, pPriv)) { +if (!AddResource(pid, dri2ReferenceRes, ref)) { free(ref); return BadAlloc; } -if (!DRI2LookupDrawableRef(pPriv, id)) -if (!AddResource(id, dri2DrawableRes, pPriv)) { -FreeResourceByType(dri2_id, dri2DrawableRes, TRUE); -free(ref); -return BadAlloc; -} -ref->id = id; -ref->dri2_id = dri2_id; +ref->pid = pid; ref->invalidate = invalidate; ref->priv = priv; xorg_list_add(&ref->link, &pPriv->reference_list); @@ -324,71 +317,32 @@ DRI2AddDrawableRef(DRI2DrawablePtr pPriv, XID id, XID dri2_id, } int -DRI2CreateDrawable2(ClientPtr client, DrawablePtr pDraw, XID id, -DRI2InvalidateProcPtr invalidate, void *priv, -XID *dri2_id_out) +DRI2DestroyDrawable(ClientPtr client, Drawa
[PATCH 2/3] dri2: Only create one unnamed reference per Drawable per Client
This workarounds issues in mesa and Mali that likes to create a new DRI2Drawble per context and per frame, respectively. The idea is that we only create one unnamed reference per Client on each Drawable that is never destroyed - and we make the assumption that the only caller is the DRI2 dispatcher and thus we don't need to check that the invalidate handler is the same. Cc: Daniel Drake Link: https://freedesktop.org/patch/19695/ Signed-off-by: Chris Wilson --- hw/xfree86/dri2/dri2.c| 22 -- hw/xfree86/dri2/dri2ext.c | 2 +- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c index af286e6..bfa551e 100644 --- a/hw/xfree86/dri2/dri2.c +++ b/hw/xfree86/dri2/dri2.c @@ -279,6 +279,7 @@ DRI2SwapLimit(DrawablePtr pDraw, int swap_limit) typedef struct DRI2DrawableRefRec { XID pid; +unsigned int unnamed:1; DRI2InvalidateProcPtr invalidate; void *priv; struct xorg_list link; @@ -299,16 +300,33 @@ DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID pid, pPriv->prime_id = dri2ClientPrivate(client)->prime_id; +if (pid == 0) { +xorg_list_for_each_entry(ref, &pPriv->reference_list, link) +if (ref->unnamed && CLIENT_ID(ref->pid) == client->index) { +assert(ref->invalidate == invalidate); +assert(ref->priv == priv); +return Success; +} +} + ref = malloc(sizeof *ref); if (ref == NULL) return BadAlloc; -if (!AddResource(pid, dri2ReferenceRes, ref)) { +if (pid == 0) { +ref->unnamed = 1; +ref->pid = FakeClientID(client->index); +} +else { +ref->unnamed = 0; +ref->pid = pid; +} + +if (!AddResource(ref->pid, dri2ReferenceRes, ref)) { free(ref); return BadAlloc; } -ref->pid = pid; ref->invalidate = invalidate; ref->priv = priv; xorg_list_add(&ref->link, &pPriv->reference_list); diff --git a/hw/xfree86/dri2/dri2ext.c b/hw/xfree86/dri2/dri2ext.c index 5dae806..8e70bc5 100644 --- a/hw/xfree86/dri2/dri2ext.c +++ b/hw/xfree86/dri2/dri2ext.c @@ -185,7 +185,7 @@ ProcDRI2CreateDrawable(ClientPtr client) &pDrawable, &status)) return status; -status = DRI2CreateDrawable(client, pDrawable, FakeClientID(client->index), +status = DRI2CreateDrawable(client, pDrawable, 0, DRI2InvalidateBuffersEvent, client); if (status != Success) return status; -- 2.1.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] dri2: work around broken DRI2CreateDrawable callers
On Fri, Feb 07, 2014 at 08:43:11AM -0600, Daniel Drake wrote: > The state of the DRI2CreateDrawable request is a little unclear. > dri2proto.txt states that it was dropped in version 1.99.2, but > actually this must still be called exactly once for each drawable > before GetBuffers and friends can be called, because it is the only > way that the internal DRI2DrawableRec structure will be allocated. > > Mali fails to obey this unwritten rule and instead calls CreateDrawable > before each and every GetBuffers request. That causes a new dri2_id and > internal reference to be created every time. When we then come to > invalidate the drawable after GetBuffers+SwapBuffers for frame N, we end > up generating N invalidate events from DRI2InvalidateDrawable. Things > quickly get out of hand. > > Fix this by detecting the fact that we're being called from > the DRI2CreateDrawable request context (i.e. caller does not get to > learn about the assigned dri2_id). In that case, just ensure that > we have allocated the relevant internal DRI2 private data, and return. I recently came across something very similar because it affects mesa also. The problem is that we do want to track a reference per Client, so simply creating a single reference seems fraught with danger. For mesa, I thought using a named reference, i.e. passing the dri2_id from the Client to use for the reference, and then destroying that reference along with mesa's glXDrawable, actually fixes a few bugs in mesa/src/glx/dri2_glx.c Did you find an alternative solution for mali? If not, I think I can generalise this into only allocating a single reference per DRI2 Client per Drawable. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: DRI2InvalidateWalk high in "perf top" profile
On Mon, Feb 16, 2015 at 09:30:43PM +0100, Loïc Yhuel wrote: > Hi, > > Please forgive me if my analysis is wrong for the current code, since > I'm running Fedora 21, so xorg 1.16.3, but it doesn't seem to to have > changed much. > > I was surprised to see DRI2InvalidateWalk high in "perf top" profile, > both with an idle system, and when running some graphics tests, so I > looked at callstacks with gdb : sna_present_vblank_handler calls > present_execute, which calls present_set_tree_pixmap(screen->root, > vblank->pixmap). > For each affected window, DRI2SetWindowPixmap is called, and it calls > DRI2InvalidateDrawableAll, which goes up back to the root and > invalidates all windows which already have the new pixmap. > So if I'm right, the invalidation added by > 18744907d0766b1b57be12df5adafd0f93221006 "dri2: Invalidate DRI2Buffers > upon SetWindowPixmap updates" is done in quadratic time. Indeed, it is overkill to use DRI2InvalidateDrawableAll() as all callers of pScreen->SetWindowPixmap will be traversing the tree themselves, so diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c index e37f2de..1f33d16 100644 --- a/hw/xfree86/dri2/dri2.c +++ b/hw/xfree86/dri2/dri2.c @@ -1406,8 +1406,7 @@ DRI2ConfigNotify(WindowPtr pWin, int x, int y, int w, int h, int bw, static void DRI2SetWindowPixmap(WindowPtr pWin, PixmapPtr pPix) { -DrawablePtr pDraw = (DrawablePtr) pWin; -ScreenPtr pScreen = pDraw->pScreen; +ScreenPtr pScreen = pWin->drawable.pScreen; DRI2ScreenPtr ds = DRI2GetScreen(pScreen); pScreen->SetWindowPixmap = ds->SetWindowPixmap; @@ -1415,7 +1414,7 @@ DRI2SetWindowPixmap(WindowPtr pWin, PixmapPtr pPix) ds->SetWindowPixmap = pScreen->SetWindowPixmap; pScreen->SetWindowPixmap = DRI2SetWindowPixmap; -DRI2InvalidateDrawableAll(pDraw); +DRI2InvalidateDrawable(&pWin->drawable); } would suffice -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH] present: Cancel the copy on unflip when aborting a pending flip
If we handle a vblank notification and perform a copy whilst a flip is still pending, we mark the pending flip for abort. After marking the next flip completion for abortion, we then proceed to copy the requested region into the restored Window drawable. However, when we then process the unflip for the aborted Pixmap we copy the contents of flip pixmap over top of the previously copied region - overwriting it with stale data. A simplish hack that seems to prevent this is to mark the flip_window as NULL during set_abort so that unflip skips the copy back over the updated contents. Signed-off-by: Chris Wilson --- present/present.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/present/present.c b/present/present.c index 9a283d4..e9ccfb8 100644 --- a/present/present.c +++ b/present/present.c @@ -401,10 +401,12 @@ present_set_abort_flip(ScreenPtr screen) * 2D applications drawing to the wrong pixmap. */ -if (screen_priv->flip_window) +if (screen_priv->flip_window) { present_set_tree_pixmap(screen_priv->flip_window, screen_priv->flip_pixmap, pixmap); + screen_priv->flip_window = NULL; +} if (screen->root) present_set_tree_pixmap(screen->root, NULL, pixmap); -- 2.1.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH] present: Fix missed notify MSC computation
Only treat divisor==0 as async to immediately report the actual vblank. If the user species a non-zero divisor, we should compute the missed vblank properly or else we report too early. Signed-off-by: Chris Wilson --- present/present.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/present/present.c b/present/present.c index 1dddcc0..9a283d4 100644 --- a/present/present.c +++ b/present/present.c @@ -961,7 +961,7 @@ present_notify_msc(WindowPtr window, 0, 0, NULL, NULL, NULL, - PresentOptionAsync, + divisor == 0 ? PresentOptionAsync : 0, target_msc, divisor, remainder, NULL, 0); } -- 2.1.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/2] shm: Fix use-after-free in ShmDestroyPixmap
On Sat, Feb 14, 2015 at 08:33:18PM +0900, Michel Dänzer wrote: > On 14.02.2015 19:36, Chris Wilson wrote: > > /*ARGSUSED*/ static int > > ShmDetachSegment(void *value, /* must conform to DeleteType */ > > - XID shmseg) > > + XID unused) > > { > > ShmDescPtr shmdesc = (ShmDescPtr) value; > > ShmDescPtr *prev; > > > > It's a static function, so you can just remove the unused parameter. We want to keep it around so that it matches DeleteType as it is passed to CreateNewResourceType as the cleanup handler for the resource. The comment is telling the truth for once! -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 2/2] shm: Expose SHM segments to backends
For drivers that can map system memory into their video memory, the overhead in utilizing SHM memory directly is in the cost of creating that mapping. SHM segments are typically much longer lived than either the Pixmap or Image created within them, and so exposing those for the driver to map amoritizes the cost of that mapping. A typical example would be the Chromium web browser which creates a SHM transport buffer but then recreates a SHM Pixmap to perform its upload for every frame. Recreating a mapping for the SHM Pixmap on every frame is approximately an extra 25% CPU overhead for a UMA driver using the GPU to do the compositing transfer from SHM transport buffer to the screen. In order to expose the SHM segments, we turn the ShmDesc into a regular object by extending it with dixPrivates and add a destroy hook. Then the ShmDescPtr is added to the SHM CreatePixmap, PutImage, GetImage interfaces and hooks provided for the driver. The driver may not implement any of these, in which case we fallback to the current implementation that wraps the normal PutImage and GetImage interface. Signed-off-by: Chris Wilson --- Xext/shm.c | 224 - Xext/shmint.h | 34 ++-- dix/privates.c | 1 + include/privates.h | 1 + 4 files changed, 184 insertions(+), 76 deletions(-) diff --git a/Xext/shm.c b/Xext/shm.c index 52d9974..9877b0a 100644 --- a/Xext/shm.c +++ b/Xext/shm.c @@ -94,11 +94,14 @@ in this Software without prior written authorization from The Open Group. typedef struct _ShmScrPrivateRec { CloseScreenProcPtr CloseScreen; -ShmFuncsPtr shmFuncs; +ShmFuncs2 shmFuncs; DestroyPixmapProcPtr destroyPixmap; } ShmScrPrivateRec; -static PixmapPtr fbShmCreatePixmap(XSHM_CREATE_PIXMAP_ARGS); +static PixmapPtr +fbShmCreatePixmap(ScreenPtr pScreen, + int width, int height, int depth, + ShmDescPtr shmdesc, int offset); static int ShmDetachSegment(void *value, XID shmseg); static void ShmResetProc(ExtensionEntry *extEntry); static void SShmCompletionEvent(xShmCompletionEvent *from, @@ -118,8 +121,6 @@ static DevPrivateKeyRec shmScrPrivateKeyRec; static DevPrivateKeyRec shmPixmapPrivateKeyRec; #define shmPixmapPrivateKey (&shmPixmapPrivateKeyRec) -static ShmFuncs miFuncs = { NULL, NULL }; -static ShmFuncs fbFuncs = { fbShmCreatePixmap, NULL }; #define ShmGetScreenPriv(s) ((ShmScrPrivateRec *)dixLookupPrivate(&(s)->devPrivates, shmScrPrivateKey)) @@ -236,11 +237,43 @@ ShmResetProc(ExtensionEntry * extEntry) } void +ShmRegisterFuncs2(ScreenPtr pScreen, const ShmFuncs2 *funcs) +{ +if (funcs->version != 2) +return; + +if (!ShmRegisterPrivates()) +return; + +ShmInitScreenPriv(pScreen)->shmFuncs = *funcs; +} + +static PixmapPtr +ShmCreatePixmap1(ScreenPtr pScreen, + int width, int height, int depth, + ShmDescPtr shmdesc, int offset) +{ +ShmScrPrivateRec *screen_priv = ShmGetScreenPriv(pScreen); +return (*screen_priv->shmFuncs._CreatePixmap1) (pScreen, width, height, depth, +shmdesc->addr + offset); +} + +void ShmRegisterFuncs(ScreenPtr pScreen, ShmFuncsPtr funcs) { +ShmFuncs2 funcs2; + if (!ShmRegisterPrivates()) return; -ShmInitScreenPriv(pScreen)->shmFuncs = funcs; + +memset(&funcs2, 0, sizeof(funcs2)); +funcs2.version = SHM_VERSION; +if (funcs && funcs->CreatePixmap) { +funcs2._CreatePixmap1 = funcs->CreatePixmap; +funcs2.CreatePixmap = ShmCreatePixmap1; +} + +ShmRegisterFuncs2(pScreen, &funcs2); } static Bool @@ -268,7 +301,12 @@ ShmDestroyPixmap(PixmapPtr pPixmap) void ShmRegisterFbFuncs(ScreenPtr pScreen) { -ShmRegisterFuncs(pScreen, &fbFuncs); +ShmFuncs2 funcs; + +memset(&funcs, 0, sizeof(funcs)); +funcs.version = SHM_VERSION; +funcs.CreatePixmap = fbShmCreatePixmap; +ShmRegisterFuncs2(pScreen, &funcs); } static int @@ -390,7 +428,8 @@ ProcShmAttach(ClientPtr client) shmdesc->refcnt++; } else { -shmdesc = malloc(sizeof(ShmDescRec)); +shmdesc = (ShmDescPtr) dixAllocateObjectWithPrivates(ShmDescRec, + PRIVATE_SHM); if (!shmdesc) return BadAlloc; #ifdef SHM_FD_PASSING @@ -399,7 +438,7 @@ ProcShmAttach(ClientPtr client) shmdesc->addr = shmat(stuff->shmid, 0, stuff->readOnly ? SHM_RDONLY : 0); if ((shmdesc->addr == ((char *) -1)) || SHMSTAT(stuff->shmid, &buf)) { -free(shmdesc); +dixFreeObjectWithPrivates(shmdesc, PRIVATE_SHM); return BadAccess; } @@ -409,7 +448,7 @@ ProcShmAttach(ClientPtr client) if (shm_access(client, &(SHM_PERM(buf)),
[PATCH 1/2] shm: Fix use-after-free in ShmDestroyPixmap
We pass the pPixmap->drawable.id to the ShmDetachSegment function after the pPixmap is freed. Fortunately, we don't use the value inside ShmDetachSegment and can simply pass zero instead. Signed-off-by: Chris Wilson --- Xext/shm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Xext/shm.c b/Xext/shm.c index db9d474..52d9974 100644 --- a/Xext/shm.c +++ b/Xext/shm.c @@ -260,7 +260,7 @@ ShmDestroyPixmap(PixmapPtr pPixmap) pScreen->DestroyPixmap = ShmDestroyPixmap; if (shmdesc) - ShmDetachSegment(shmdesc, pPixmap->drawable.id); + ShmDetachSegment(shmdesc, 0); return ret; } @@ -427,7 +427,7 @@ ProcShmAttach(ClientPtr client) /*ARGSUSED*/ static int ShmDetachSegment(void *value, /* must conform to DeleteType */ - XID shmseg) + XID unused) { ShmDescPtr shmdesc = (ShmDescPtr) value; ShmDescPtr *prev; -- 2.1.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 2/2] present: Fix presentation of flips out of order
The flip queue currently only holds events submitted to the driver for flipping, awaiting the completion notifier. It is short. We therefore can speed up interrupt processing by keeping the small number of events ready to be flipped on the end of the flip queue. By appending the events to the flip_queue in the order that they become ready, we also resolve one issue causing Present to display frames out of order. Signed-off-by: Chris Wilson --- present/present.c | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/present/present.c b/present/present.c index b132f4d..bce41f4 100644 --- a/present/present.c +++ b/present/present.c @@ -327,10 +327,10 @@ present_re_execute(present_vblank_ptr vblank) static void present_flip_try_ready(ScreenPtr screen) { -present_vblank_ptr vblank, tmp; +present_vblank_ptr vblank; -xorg_list_for_each_entry_safe(vblank, tmp, &present_exec_queue, event_queue) { -if (vblank->flip_ready) { +xorg_list_for_each_entry(vblank, &present_flip_queue, event_queue) { +if (vblank->queued) { present_re_execute(vblank); return; } @@ -623,6 +623,8 @@ present_execute(present_vblank_ptr vblank, uint64_t ust, uint64_t crtc_msc) DebugPresent(("\tr %lld %p (pending %p unflip %lld)\n", vblank->event_id, vblank, screen_priv->flip_pending, screen_priv->unflip_event_id)); +xorg_list_del(&vblank->event_queue); +xorg_list_append(&vblank->event_queue, &present_flip_queue); vblank->flip_ready = TRUE; return; } @@ -938,6 +940,7 @@ present_abort_vblank(ScreenPtr screen, RRCrtcPtr crtc, uint64_t event_id, uint64 xorg_list_for_each_entry(vblank, &present_flip_queue, event_queue) { if (vblank->event_id == event_id) { xorg_list_del(&vblank->event_queue); +vblank->queued = FALSE; return; } } -- 2.1.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 1/2] present: Improve scaling of vblank handler
With large numbers of queued vblank, the list iteration on every interupt dominates processing time. If we reorder the list to be in ascending event order, then not only is also likely to be in order for notification queries (i.e. the notification will be near the start of the list), we can also stop iterating when past the target event_id. Signed-off-by: Chris Wilson --- present/present.c | 24 +++- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/present/present.c b/present/present.c index ba88f79..b132f4d 100644 --- a/present/present.c +++ b/present/present.c @@ -482,19 +482,22 @@ present_flip_notify(present_vblank_ptr vblank, uint64_t ust, uint64_t crtc_msc) void present_event_notify(uint64_t event_id, uint64_t ust, uint64_t msc) { -present_vblank_ptr vblank, tmp; +present_vblank_ptr vblank; int s; if (!event_id) return; DebugPresent(("\te %lld ust %lld msc %lld\n", event_id, ust, msc)); -xorg_list_for_each_entry_safe(vblank, tmp, &present_exec_queue, event_queue) { -if (vblank->event_id == event_id) { +xorg_list_for_each_entry(vblank, &present_exec_queue, event_queue) { +int64_t match = event_id - vblank->event_id; +if (match == 0) { present_execute(vblank, ust, msc); return; } +if (match < 0) +break; } -xorg_list_for_each_entry_safe(vblank, tmp, &present_flip_queue, event_queue) { +xorg_list_for_each_entry(vblank, &present_flip_queue, event_queue) { if (vblank->event_id == event_id) { present_flip_notify(vblank, ust, msc); return; @@ -887,7 +890,7 @@ present_pixmap(WindowPtr window, vblank->pixmap->drawable.id, vblank->window->drawable.id, target_crtc, vblank->flip, vblank->sync_flip, vblank->serial)); -xorg_list_add(&vblank->event_queue, &present_exec_queue); +xorg_list_append(&vblank->event_queue, &present_exec_queue); vblank->queued = TRUE; if ((pixmap && target_msc >= crtc_msc) || (!pixmap && target_msc > crtc_msc)) { ret = present_queue_vblank(screen, target_crtc, vblank->event_id, target_msc); @@ -911,7 +914,7 @@ no_mem: void present_abort_vblank(ScreenPtr screen, RRCrtcPtr crtc, uint64_t event_id, uint64_t msc) { -present_vblank_ptr vblank, tmp; +present_vblank_ptr vblank; if (crtc == NULL) present_fake_abort_vblank(screen, event_id, msc); @@ -922,14 +925,17 @@ present_abort_vblank(ScreenPtr screen, RRCrtcPtr crtc, uint64_t event_id, uint64 (*screen_priv->info->abort_vblank) (crtc, event_id, msc); } -xorg_list_for_each_entry_safe(vblank, tmp, &present_exec_queue, event_queue) { -if (vblank->event_id == event_id) { +xorg_list_for_each_entry(vblank, &present_exec_queue, event_queue) { +int64_t match = event_id - vblank->event_id; +if (match == 0) { xorg_list_del(&vblank->event_queue); vblank->queued = FALSE; return; } +if (match < 0) +break; } -xorg_list_for_each_entry_safe(vblank, tmp, &present_flip_queue, event_queue) { +xorg_list_for_each_entry(vblank, &present_flip_queue, event_queue) { if (vblank->event_id == event_id) { xorg_list_del(&vblank->event_queue); return; -- 2.1.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 3/3] present: Check for an invalid target CRTC before flipping
On Sun, Feb 08, 2015 at 04:28:31PM -0800, Kenneth Graunke wrote: > On Sunday, February 08, 2015 12:00:37 PM Chris Wilson wrote: > > On Sun, Feb 08, 2015 at 02:10:07AM -0800, Kenneth Graunke wrote: > > > On Friday, February 06, 2015 08:25:44 AM Chris Wilson wrote: > > > > Once we have chosen the target CRTC, check for an error when querying > > > > the current MSC so we can report the error before attempting to record > > > > the uninitialised MSC/UST, and subsequently feeding garbage values to > > > > drmWaitVBlank. > > > > > > > > Signed-off-by: Chris Wilson > > > > --- > > > > present/present.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/present/present.c b/present/present.c > > > > index 1ce587e..4a1cf0c 100644 > > > > --- a/present/present.c > > > > +++ b/present/present.c > > > > @@ -727,7 +727,9 @@ present_pixmap(WindowPtr window, > > > > else if (!target_crtc) > > > > target_crtc = present_get_crtc(window); > > > > > > > > -present_get_ust_msc(screen, target_crtc, &ust, &crtc_msc); > > > > +ret = present_get_ust_msc(screen, target_crtc, &ust, &crtc_msc); > > > > +if (ret) > > > > + return ret; > > > > > > > > target_msc = present_window_to_crtc_msc(window, target_crtc, > > > > window_msc, crtc_msc); > > > > > > You've definitely found a bug here. Some drivers' get_ust_msc hooks > > > (UXA, modesetting) return BadMatch and leave crtc_msc uninitialized. > > > At which point we'd happily continue and use rubbish values. > > > > > > I discovered this today as well, when using valgrind to try and debug > > > some crashes in xf86-video-modesetting using my 'pageflip' branch: > > > [term1]# valgrind --vgdb-error=1 X > > > [term2]# gdb > > > [term2](gdb) target remote | vgdb > > > [term2](gdb) c > > > [term3]$ DISPLAY=:0 glxgears -fullscreen & > > > [term3]$ DISPLAY=:0 xset force dpms off > > > > > > > > > valgrind will complain about the uninitialized values in the crtc_msc >= > > > target_msc branch. > > > > > > But...is an early return really the right thing to do? I tested this > > > with xf86-video-modesetting and my 'pageflip' branch, and it caused > > > "glxgears -fullscreen" to die from a BadMatch error after a single DPMS > > > cycle. I tried changing it to 'return Success', and that caused it to > > > stop drawing altogether. I also tried it with UXA+DRI3, and it seemed > > > to break there as well. So it seems like we need to do something more. > > > > The previous patch fixes that bug. > > I was running with all three of your patches in this series. In that case, the only way we end up with an invalid target_crtc is if either the driver tells Present to use a CRTC it cannot use with drmWaitVBlank, or if the user requests an invalid CRTC. If the user requests an unusable CRTC, we definitely want to report that early. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 3/3] present: Check for an invalid target CRTC before flipping
On Sun, Feb 08, 2015 at 02:10:07AM -0800, Kenneth Graunke wrote: > On Friday, February 06, 2015 08:25:44 AM Chris Wilson wrote: > > Once we have chosen the target CRTC, check for an error when querying > > the current MSC so we can report the error before attempting to record > > the uninitialised MSC/UST, and subsequently feeding garbage values to > > drmWaitVBlank. > > > > Signed-off-by: Chris Wilson > > --- > > present/present.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/present/present.c b/present/present.c > > index 1ce587e..4a1cf0c 100644 > > --- a/present/present.c > > +++ b/present/present.c > > @@ -727,7 +727,9 @@ present_pixmap(WindowPtr window, > > else if (!target_crtc) > > target_crtc = present_get_crtc(window); > > > > -present_get_ust_msc(screen, target_crtc, &ust, &crtc_msc); > > +ret = present_get_ust_msc(screen, target_crtc, &ust, &crtc_msc); > > +if (ret) > > + return ret; > > > > target_msc = present_window_to_crtc_msc(window, target_crtc, > > window_msc, crtc_msc); > > You've definitely found a bug here. Some drivers' get_ust_msc hooks > (UXA, modesetting) return BadMatch and leave crtc_msc uninitialized. > At which point we'd happily continue and use rubbish values. > > I discovered this today as well, when using valgrind to try and debug > some crashes in xf86-video-modesetting using my 'pageflip' branch: > [term1]# valgrind --vgdb-error=1 X > [term2]# gdb > [term2](gdb) target remote | vgdb > [term2](gdb) c > [term3]$ DISPLAY=:0 glxgears -fullscreen & > [term3]$ DISPLAY=:0 xset force dpms off > > > valgrind will complain about the uninitialized values in the crtc_msc >= > target_msc branch. > > But...is an early return really the right thing to do? I tested this > with xf86-video-modesetting and my 'pageflip' branch, and it caused > "glxgears -fullscreen" to die from a BadMatch error after a single DPMS > cycle. I tried changing it to 'return Success', and that caused it to > stop drawing altogether. I also tried it with UXA+DRI3, and it seemed > to break there as well. So it seems like we need to do something more. The previous patch fixes that bug. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 2/3] present: When cancelling a pending synchronous flip, requeue it
The vblank event request for a synchronous flip is scheduled for the vblank before the target flip msc (so that the flip itself appears at the right frame). If we cancel that flip and so wish to schedule a copy instead, that copy needs to be postponed by a frame in order for it be performed at the requested time. Signed-off-by: Chris Wilson --- present/present.c | 16 +++- present/present_priv.h | 1 + 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/present/present.c b/present/present.c index ce978f4..a53dc11 100644 --- a/present/present.c +++ b/present/present.c @@ -549,8 +549,13 @@ present_check_flip_window (WindowPtr window) /* Now check any queued vblanks */ xorg_list_for_each_entry(vblank, &window_priv->vblank, window_list) { -if (vblank->queued && vblank->flip && !present_check_flip(vblank->crtc, window, vblank->pixmap, vblank->sync_flip, NULL, 0, 0)) +if (vblank->queued && vblank->flip && !present_check_flip(vblank->crtc, window, vblank->pixmap, vblank->sync_flip, NULL, 0, 0)) { vblank->flip = FALSE; +if (vblank->sync_flip) { +vblank->requeue = TRUE; +vblank->target_msc++; +} +} } } @@ -585,6 +590,15 @@ present_execute(present_vblank_ptr vblank, uint64_t ust, uint64_t crtc_msc) present_screen_priv_ptr screen_priv = present_screen_priv(screen); uint8_t mode; +if (vblank->requeue) { +vblank->requeue = FALSE; +if (Success == present_queue_vblank(screen, +vblank->crtc, +vblank->event_id, +vblank->target_msc)) +return; +} + if (vblank->wait_fence) { if (!present_fence_check_triggered(vblank->wait_fence)) { present_fence_set_callback(vblank->wait_fence, present_wait_fence_triggered, vblank); diff --git a/present/present_priv.h b/present/present_priv.h index 110c925..7db4321 100644 --- a/present/present_priv.h +++ b/present/present_priv.h @@ -70,6 +70,7 @@ struct present_vblank { present_notify_ptr notifies; int num_notifies; Boolqueued; /* on present_exec_queue */ +Boolrequeue;/* on queue, but target_msc has changed */ Boolflip; /* planning on using flip */ Boolflip_ready; /* wants to flip, but waiting for previous flip or unflip */ Boolsync_flip; /* do flip synchronous to vblank */ -- 2.1.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 3/3] present: Do not replace Pixmaps on redirected Window on unflip
When unflipping, we may find that our flip window has been redirected. If we replace the redirected Window with the Screen Pixmap we then have mutliple fullscreen Windows believing that their own the Screen Pixmap - multiple fullscreen Windows that are being flipped by Clients, and so continue to flip causing popping between e.g. the compositor and the game. Signed-off-by: Chris Wilson --- present/present.c | 28 +++- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/present/present.c b/present/present.c index a53dc11..ba88f79 100644 --- a/present/present.c +++ b/present/present.c @@ -374,12 +374,17 @@ present_set_tree_pixmap_visit(WindowPtr window, void *data) } static void -present_set_tree_pixmap(WindowPtr window, PixmapPtr pixmap) +present_set_tree_pixmap(WindowPtr window, +PixmapPtr expected, +PixmapPtr pixmap) { struct pixmap_visit visit; ScreenPtr screen = window->drawable.pScreen; visit.old = (*screen->GetWindowPixmap)(window); +if (expected && visit.old != expected) +return; + visit.new = pixmap; if (visit.old == visit.new) return; @@ -390,6 +395,7 @@ static void present_set_abort_flip(ScreenPtr screen) { present_screen_priv_ptr screen_priv = present_screen_priv(screen); +PixmapPtr pixmap = (*screen->GetScreenPixmap)(screen); /* Switch back to using the screen pixmap now to avoid * 2D applications drawing to the wrong pixmap. @@ -397,10 +403,11 @@ present_set_abort_flip(ScreenPtr screen) if (screen_priv->flip_window) present_set_tree_pixmap(screen_priv->flip_window, - (*screen->GetScreenPixmap)(screen)); +screen_priv->flip_pixmap, +pixmap); if (screen->root) -present_set_tree_pixmap(screen->root, (*screen->GetScreenPixmap)(screen)); +present_set_tree_pixmap(screen->root, NULL, pixmap); screen_priv->flip_pending->abort_flip = TRUE; } @@ -414,10 +421,12 @@ present_unflip(ScreenPtr screen) assert (!screen_priv->unflip_event_id); assert (!screen_priv->flip_pending); -if (screen_priv->flip_window) -present_set_tree_pixmap(screen_priv->flip_window, pixmap); +if (screen_priv->flip_pixmap && screen_priv->flip_window) +present_set_tree_pixmap(screen_priv->flip_window, +screen_priv->flip_pixmap, +pixmap); -present_set_tree_pixmap(screen->root, pixmap); +present_set_tree_pixmap(screen->root, NULL, pixmap); /* Update the screen pixmap with the current flip pixmap contents */ @@ -645,9 +654,10 @@ present_execute(present_vblank_ptr vblank, uint64_t ust, uint64_t crtc_msc) */ if (screen_priv->flip_window && screen_priv->flip_window != window) present_set_tree_pixmap(screen_priv->flip_window, - (*screen->GetScreenPixmap)(screen)); -present_set_tree_pixmap(window, pixmap); -present_set_tree_pixmap(screen->root, pixmap); +screen_priv->flip_pixmap, + (*screen->GetScreenPixmap)(screen)); +present_set_tree_pixmap(window, NULL, pixmap); +present_set_tree_pixmap(screen->root, NULL, pixmap); /* Report update region as damaged */ -- 2.1.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 1/3] present: Requery pending flips with the right sync_flip mode
When verifying whether a pending flip is still valid, we need to pass down the orignal sync_flip mode (e.g. if the driver only supports sync flips, verifying a async flip will falsely fail). Signed-off-by: Chris Wilson --- present/present.c | 5 +++-- present/present_priv.h | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/present/present.c b/present/present.c index 4a1cf0c..ce978f4 100644 --- a/present/present.c +++ b/present/present.c @@ -453,6 +453,7 @@ present_flip_notify(present_vblank_ptr vblank, uint64_t ust, uint64_t crtc_msc) screen_priv->flip_window = vblank->window; screen_priv->flip_serial = vblank->serial; screen_priv->flip_pixmap = vblank->pixmap; +screen_priv->flip_sync = vblank->sync_flip; screen_priv->flip_idle_fence = vblank->idle_fence; vblank->pixmap = NULL; @@ -541,14 +542,14 @@ present_check_flip_window (WindowPtr window) * Check current flip */ if (window == screen_priv->flip_window) { -if (!present_check_flip(screen_priv->flip_crtc, window, screen_priv->flip_pixmap, FALSE, NULL, 0, 0)) +if (!present_check_flip(screen_priv->flip_crtc, window, screen_priv->flip_pixmap, screen_priv->flip_sync, NULL, 0, 0)) present_unflip(screen); } } /* Now check any queued vblanks */ xorg_list_for_each_entry(vblank, &window_priv->vblank, window_list) { -if (vblank->queued && vblank->flip && !present_check_flip(vblank->crtc, window, vblank->pixmap, FALSE, NULL, 0, 0)) +if (vblank->queued && vblank->flip && !present_check_flip(vblank->crtc, window, vblank->pixmap, vblank->sync_flip, NULL, 0, 0)) vblank->flip = FALSE; } } diff --git a/present/present_priv.h b/present/present_priv.h index f5c1652..110c925 100644 --- a/present/present_priv.h +++ b/present/present_priv.h @@ -93,6 +93,7 @@ typedef struct present_screen_priv { uint32_tflip_serial; PixmapPtr flip_pixmap; present_fence_ptr flip_idle_fence; +Boolflip_sync; present_screen_info_ptr info; } present_screen_priv_rec, *present_screen_priv_ptr; -- 2.1.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel