Re: Xorg setting low resolution with double HDMI output despite forcing to full HD

[Chris Wilson]

Quoting Felix Miata (2019-02-21 12:05:36)
> Francesco Nwokeka composed on 2019-02-21 10:45 (UTC+0100):
> 
> > I have an Intel NUC with dual HDMI output. Recently I've been 
> > experiencing 
> > a problem with the output resolution of the screens. When booting the 
> > system i 
> > get screen resolution inferior to what I actually try to set via .xinitrc 
> > file:
> 
> > xrandr --output $(xrandr -q | grep "\sconnected\s" | cut -f1 -d" ") --mode 
> > 1920x1080
> 
> Note in the log the string onnected does not appear. AFAICT, only the use of 
> xf86-video-intel DDX
> driver, which last had an official release in 2015, causes that omission.
> 
> > local/xf86-video-intel 1:2.99.917+859+g33ee0c3b-1 (xorg-drivers)
> 
> > Does anyone have any idea to what might be the problem/solution?
> 
> Easier solution is to purge xf86-video-intel, which when server version is 
> 1.17.0 or higher, should
> automagically cause utilization of the newer modesetting DDX driver on X 
> restart or reboot. Other
> option is to reconfigure /etc/X11/xorg.conf.d/50-device.conf to specify 
> driver modesetting. The DDX
> modesetting driver does not have its own package. Instead it's the default, 
> provided by the server
> package.

Ahem. The ddx is doing exactly what it is being told to do.
-Chris
___
xorg@lists.x.org: X.Org support
Archives: http://lists.freedesktop.org/archives/xorg
Info: https://lists.x.org/mailman/listinfo/xorg
Your subscription address: %(user_address)s

Re: X freezes for a second or every now and then when lid closed

[Chris Wilson]

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: One (Intel) GPU multiseat without Xephyr/Xnest, with a Xorg server per output

[Chris Wilson]

Quoting Troll Berserker (2018-09-18 16:28:02)
> Is it possible?

man 4 intel, search for ZaphodHeads
-Chris
___
xorg@lists.x.org: X.Org support
Archives: http://lists.freedesktop.org/archives/xorg
Info: https://lists.x.org/mailman/listinfo/xorg
Your subscription address: %(user_address)s

Re: [PATCH xserver 1/2] xf86-video-modesetting: Add ms_queue_vblank helper

[Chris Wilson]

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, );
> +ret = ms_queue_vblank(crtc, MS_QUEUE_ABSOLUTE, target_msc, 
> _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()

[Chris Wilson]

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 <ch...@chris-wilson.co.uk>
---
 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, , list)
+xorg_list_for_each_entry(existing, , 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()

[Chris Wilson]

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 <ch...@chris-wilson.co.uk>
---
 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(>list, existing->list.prev);
+xorg_list_append(>list, >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

[Chris Wilson]

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 <ch...@chris-wilson.co.uk>
---
 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();
 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

[Chris Wilson]

Quoting Clemens Eisserer (2018-01-23 17:03:04)
> Hi,
> 
> Great to see movement regading this issue.
> 
> > It's broken with llvmpipe/softpipe as well. Does it render correctly
> > with glamor on i965? If so, maybe it's a Gallium non-driver issue.
> 
> Glamor on Intel Gen5 (Arrendale) procudes results consistent with
> radeonsi/llvmpipe.
> So it seems the shader is the culprit.

Radial shader (which we think is correct):
t = abs(fract(t * 0.5 + 0.5) * 2.0 - 1.0);

Linear shader (note unnormalized):
distance = abs(mod(distance + _pt_distance, 2.0 * _pt_distance) - 
_pt_distance);

Which makes the same mistake as I did in using mod() instead of the GL
definition of fract(). Simplest fix will be to normalize distance and
then use the correct transformation from the radial shader.
-Chris
___
xorg@lists.x.org: X.Org support
Archives: http://lists.freedesktop.org/archives/xorg
Info: https://lists.x.org/mailman/listinfo/xorg
Your subscription address: %(user_address)s

Re: Gradients are broken with glamor when RepeatReflect is set

[Chris Wilson]

Quoting Chris Wilson (2018-01-23 15:26:50)
> Quoting Jeffrey Smith (2018-01-23 15:15:10)
> > On Mon, Jan 22, 2018 at 3:01 PM, Chris Wilson <ch...@chris-wilson.co.uk> 
> > 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

[Chris Wilson]

Quoting Jeffrey Smith (2018-01-23 15:15:10)
> On Mon, Jan 22, 2018 at 3:01 PM, Chris Wilson <ch...@chris-wilson.co.uk> 
> 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

[Chris Wilson]

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

[Chris Wilson]

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

[Chris Wilson]

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"

[Chris Wilson]

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, );
 RegionUninit();
 
-DamageRegister(screen->root ? >root->drawable : >drawable,
-   dirty_update->damage);
+DamageRegister(>drawable, dirty_update->damage);
 xorg_list_add(_update->ent, >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 ? >root->drawable : 
>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, );
-}
+pGC = GetScratchGC(dirty->src->drawable.depth, pScreen);
 ValidateGC(>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, >drawable, pGC,
+pGC->ops->CopyArea(>src->drawable, >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,
->root->drawable,
+>src->drawable,
 format,
 CPSubwindowMode,
 _inferiors, serverClient, );
-- 
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

[Chris Wilson]

==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 <ch...@chris-wilson.co.uk>
---
 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(>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

[Chris Wilson]

Fixes a regression from

commit 41da295eb50fa08eaacd0ecde99f43a716fcb41a
Author: Keith Packard <kei...@keithp.com>
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 <ch...@chris-wilson.co.uk>
---
 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, , list) {
-if ((char *) busfault->addr <= (char *) fault && (char *) fault < 
(char *) busfault->addr + busfault->size) {
-break;
-}
+xorg_list_for_each_entry(iter, , 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

[Chris Wilson]

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 <ch...@chris-wilson.co.uk>
---
 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

[Chris Wilson]

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 <ch...@chris-wilson.co.uk>
---
 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, >slave_list, slave_head) {
@@ -151,14 +156,22 @@ xf86CheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, 
xf86CursorInfoPtr infoPtr
 continue;
 
 sPriv = dixLookupPrivate(>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

[Chris Wilson]

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 <ch...@chris-wilson.co.uk>
---
 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(>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(>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()

[Chris Wilson]

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 <ch...@chris-wilson.co.uk>
---
 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

[Chris Wilson]

On Sun, Jun 12, 2016 at 06:23:25PM +0100, Emil Velikov wrote:
> On 12 June 2016 at 17:09, Chris Wilson <ch...@chris-wilson.co.uk> 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 <ago...@nvidia.com> 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

[Chris Wilson]

On Sun, Jun 12, 2016 at 02:35:43PM +0100, Emil Velikov wrote:
> Hi all,
> 
> On 11 June 2016 at 01:21, Alex Goins <ago...@nvidia.com> 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

[Chris Wilson]

On Tue, Apr 26, 2016 at 02:42:52PM +0200, Stefan Dirsch wrote:
> From: Dominique Leuenberger <dims...@opensuse.org>
> 
> 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()

[Chris Wilson]

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 <ch...@chris-wilson.co.uk>
---
 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(>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(>event_queue, _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, >clipList);
-} else
+else
 damage = >clipList;
 
-DamageDamageRegion(>window->drawable, damage);
+DamageDamageRegion(>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_copy_region(>drawable, vblank->pixmap, vblan

[xrandr v2] Select NearestNeighbour filtering for pixel exact scaling

[Chris Wilson]

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 <ch...@chris-wilson.co.uk>
---
 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, '\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 (>filter);
+output->filter.filter = "auto";
+output->changes |= changes_filter;
+}
+}
+
+static void
 copy_transform (transform_t *dest, transform_t *src)
 {
-set_transform (dest, >transform,
-  src->filter, src->params, src->nparams);
+set_transform (dest, >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 (>transform, >transform, sizeof (XTransform)) != 0)
return False;
+return True;
+}
+
+static Bool
+equal_filter (filter_t *a, filter_t *b)
+{
 if (strcmp (a->filter, b->fi

Re: [xrandr] Select NearestNeighbour filtering for pixel exact scaling

[Chris Wilson]

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

[Chris Wilson]

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 <ch...@chris-wilson.co.uk>
---
 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 = >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 (>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

[Chris Wilson]

In commit f175cf45aebcdda53f3ae49c0eaf27da1f194e92
Author: Olivier Fourdan <ofour...@redhat.com>
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 <ch...@chris-wilson.co.uk>
Cc: Olivier Fourdan <ofour...@redhat.com>
Cc: Adam Jackson <a...@redhat.com>
---
 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

Re: [PATCH xserver 1/3] present: Move msc_is_(equal_or_)after to the top of present.c

[Chris Wilson]

On Wed, Feb 24, 2016 at 04:52:57PM +0900, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daen...@amd.com>
> 
> To make them usable from any other function in the file. No functional
> change.
> 
> Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk>
-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

[Chris Wilson]

On Wed, Feb 24, 2016 at 04:52:58PM +0900, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daen...@amd.com>
> 
> 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 <michel.daen...@amd.com>
Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk>
-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

[Chris Wilson]

On Wed, Feb 24, 2016 at 04:52:59PM +0900, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daen...@amd.com>
> 
> 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 <michel.daen...@amd.com>

Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk>
-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

[Chris Wilson]

On Fri, Feb 19, 2016 at 11:39:12AM +0900, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daen...@amd.com>
> 
> 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 <michel.daen...@amd.com>
Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk>
-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

[Chris Wilson]

On Fri, Feb 19, 2016 at 11:39:11AM +0900, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daen...@amd.com>
> 
> The following fix will use the refactored function.
> 
> Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk>
-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

[Chris Wilson]

On Fri, Feb 19, 2016 at 11:39:10AM +0900, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daen...@amd.com>
> 
> 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 <michel.daen...@amd.com>

And the ordering is better: we should do the restoration of the contents
before we restore the window->pixmap linkage.

Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk>
-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

[Chris Wilson]

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 <ch...@chris-wilson.co.uk>
> Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk>

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

[PATCH xserver 2/2] dri2: Allow many blocked clients per-drawable

[Chris Wilson]

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 <ch...@chris-wilson.co.uk>
---
 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, , >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_lim

[PATCH xserver 1/2] dix: Add ClientSignalAll()

[Chris Wilson]

This is a variant of ClientSignal() that signals all clients with an
optional matching sleeping client, function and closure.

Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
---
 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

Re: [PATCH xserver] dri2: Use the work queue to manage client sleeps

[Chris Wilson]

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 <ch...@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk>
-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

[Chris Wilson]

commit e43abdce964f5ed9689cf908af8c305b39a5dd36
Author: Chris Wilson <ch...@chris-wilson.co.uk>
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 <wol...@onsneteindhoven.nl>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94074
Testcase: dri2-race/client
Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
Cc: Adam Jackson <a...@nwnk.net>
---
 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

[Chris Wilson]

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 <ch...@chris-wilson.co.uk>
Cc: Adam Jackson <a...@nwnk.net>
---
 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, )) {
 if (FD_ISSET(connection, ))
@@ -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, )) {
 FD_SET(connection, );
-- 
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

[Chris Wilson]

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 <k...@bitplanet.net>
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 <dr...@endlessm.com>
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 <ch...@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
---
 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, >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(>link, >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 (re

[PATCH xserver 2/4] dri2: Split resource tracking for DRI2Drawable and references to them

[Chris Wilson]

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 <ch...@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
---
 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,
-  >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 ()
-
-#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(>devPrivates, );
+}
+
+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, >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 = pri

[PATCH xserver 4/4] dri2: Unblock Clients on Drawable release

[Chris Wilson]

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 <ch...@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
---
 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

[Chris Wilson]

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 <loic.yh...@gmail.com>
Link: http://lists.x.org/archives/xorg-devel/2015-February/045638.html
Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
---
 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(>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: xrandr only lists mode after a while of waiting

[Chris Wilson]

On Mon, Dec 14, 2015 at 10:02:41AM +, Daniel Menet wrote:
>New to the list, please bear with me and give me hints if I violate any
>rules.
>Its been a while now since I started facing an issue: xrandr only lists
>mode after a while of waiting. I assume this happend after a upgrade of
>some packages without an immediate reboot an occurred first after the
>reboot. I accepted the situation for a while due to the lack of time. More
>details are given at
>
> [1]http://unix.stackexchange.com/questions/240949/xrandr-only-lists-mode-after-a-while-of-waiting

Should be resolved by now. It was just bug that mistakenly marked the first
current-only probe as valid on the initial modesetting.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
xorg@lists.x.org: X.Org support
Archives: http://lists.freedesktop.org/archives/xorg
Info: http://lists.x.org/mailman/listinfo/xorg
Your subscription address: %(user_address)s

Re: xrandr only lists mode after a while of waiting

[Chris Wilson]

On Mon, Dec 14, 2015 at 11:58:39AM +, Daniel Menet wrote:
>Thanks Chris, that gives me some sense of optimism. Still I'm running Arch
>Linux, all up to date - and still facing the issue. Since you mentioned
>modesettigs I assume that this was a kernel related bug (I am not really
>into the topic, correct me if I'm all wrong).

This one is just the ddx. (Not that some people haven't stumbled into
another issue with 4.4-rc...)

> What release shipped the
>fix? Would you mind providing a reference to a related bug report or
>issue? I skimmed the release notes of 4.2.6 and 4.2.7 (Arch has 4.2.5) and
>could not find a hint on a fix related to KMS.

If you add an xf86-video-intel bug on https://bugs.archlinux.org/ and
mention

commit 627ef68a8cd7a51627d5b6a98cb0a5bdb1d9b534
Author: Chris Wilson <ch...@chris-wilson.co.uk>
Date:   Sat Oct 31 09:27:35 2015 +

sna: Don't extend the display mode cache for an unknown output

I am sure it will quickly get patched.  
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
xorg@lists.x.org: X.Org support
Archives: http://lists.freedesktop.org/archives/xorg
Info: http://lists.x.org/mailman/listinfo/xorg
Your subscription address: %(user_address)s

Re: How to refresh the list of outputs detected by xrandr?

[Chris Wilson]

On Thu, Nov 19, 2015 at 02:19:25PM +0100, Łukasz Maśko wrote:
> Hello.
> I have a Dell E7250 laptop. I'm using it with docking stations with external 
> monitors connected. I have one dock at home and another one at work. The 
> monitors connected to them differ, both in models and connectors (and 
> therefore are recognized as different xrandr outputs).
> 
> At home it looks like this:
> $ xrandr
> Screen 0: minimum 8 x 8, current 1920 x 1080, maximum 32767 x 32767
> eDP1 connected primary 1920x1080+0+0 (normal left inverted right x axis y 
> axis) 276mm x 156mm
>1920x1080 60.02*+  48.03  
>1400x1050 59.98  
>1280x1024 60.02  
>1280x960  60.00  
>1024x768  60.00  
>800x600   60.3256.25  
>640x480   59.94  
> DP1 disconnected (normal left inverted right x axis y axis)
> DP1-1 disconnected (normal left inverted right x axis y axis)
> DP1-2 disconnected (normal left inverted right x axis y axis)
> DP1-3 connected (normal left inverted right x axis y axis)
>1440x900  59.89 +
>1280x1024 75.0260.02  
>1280x960  60.00  
>1152x864  75.00  
>1152x720  59.97  
>1024x768  75.0860.00  
>832x624   74.55  
>800x600   75.0060.3256.25  
>848x480   60.00  
>640x480   75.0060.0059.94  
>720x400   70.08  
> DP2 disconnected (normal left inverted right x axis y axis)
> HDMI1 disconnected (normal left inverted right x axis y axis)
> HDMI2 disconnected (normal left inverted right x axis y axis)
> VIRTUAL1 disconnected (normal left inverted right x axis y axis)
> 
> while at work like this:
> $ xrandr
> Screen 0: minimum 8 x 8, current 1920 x 1080, maximum 32767 x 32767
> eDP1 connected primary 1920x1080+0+0 (normal left inverted right x axis y 
> axis) 276mm x 156mm
>1920x1080 60.02*+  48.03  
>1400x1050 59.98  
>1280x1024 60.02  
>1280x960  60.00  
>1024x768  60.00  
>800x600   60.3256.25  
>640x480   59.94  
> DP1 disconnected (normal left inverted right x axis y axis)
> DP1-1 connected (normal left inverted right x axis y axis)
>1920x1080 60.00 +  50.0059.94  
>1600x1200 60.00  
>1680x1050 59.88  
>1280x1024 60.02  
>1440x900  59.90  
>1280x960  60.00  
>1280x800  59.91  
>1280x720  60.0050.0059.94  
>1024x768  60.00  
>800x600   60.3256.25  
>720x576   50.00  
>720x480   60.0059.94  
>640x480   60.0059.94  
> DP1-2 disconnected (normal left inverted right x axis y axis)
> DP1-3 disconnected (normal left inverted right x axis y axis)
> DP2 disconnected (normal left inverted right x axis y axis)
> HDMI1 disconnected (normal left inverted right x axis y axis)
> HDMI2 disconnected (normal left inverted right x axis y axis)
> VIRTUAL1 disconnected (normal left inverted right x axis y axis)
> 
> 
> The problem is, that if I suspend my laptop while it is connected to one of 
> the docking stations, it remembers the layout of available monitors and when 
> I 
> dock it onto the other station and resume, it still shows me the old list of 
> outputs and modes. I need to undock it while running and dock it again to 
> make 
> it notify the change in outputs.
> 
> Is there a software way to force xrandr to refresh its state and notify the 
> change in connected devices?

The kernel is supposed to send a hotplug notification on resume that
should result in the list being updated.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
xorg@lists.x.org: X.Org support
Archives: http://lists.freedesktop.org/archives/xorg
Info: http://lists.x.org/mailman/listinfo/xorg
Your subscription address: %(user_address)s

Re: How to refresh the list of outputs detected by xrandr?

[Chris Wilson]

On Thu, Nov 19, 2015 at 02:39:21PM +0100, Łukasz Maśko wrote:
> Dnia czwartek, 19 listopada 2015 13:27:37 Chris Wilson pisze:
> [...]
> > The kernel is supposed to send a hotplug notification on resume that
> > should result in the list being updated.
> 
> If I get you right, the kernel does not notify the change of a dock and 
> therefore does not notify X about the change?

The usual report is "suspend, dock/undock, resume". Changing the dock
should generate the usual hardware hotplug notifications. If the machine
was suspended at the time, we have to fake the hotplug event on resume.

> Is there a way to generate such notification using currently available 
> software tools, or am I limited to fix in the kernel to make it notify dock 
> change?

Hmm, can't see a way to synthesize uevents. But still calling xrandr
should be enough to do a forced reprobe of outputs. Without a hotplug
event, there is a small 15s cache of the most recent probe.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
xorg@lists.x.org: X.Org support
Archives: http://lists.freedesktop.org/archives/xorg
Info: http://lists.x.org/mailman/listinfo/xorg
Your subscription address: %(user_address)s

Re: [PATCH] present: Fix Async swap logic

[Chris Wilson]

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 <axel.d...@ens.fr>
> ---
> 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

Re: [PATCH] present: Fix Async swap logic

[Chris Wilson]

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: How to setup independent screens with the Intel driver?

[Chris Wilson]

On Fri, Oct 30, 2015 at 04:58:49PM +0100, Boszormenyi Zoltan wrote:
> Hi,
> 
> I am using kernel 4.2.3, Xorg 1.16.4, Mesa 11.0.4 and the current 
> xf86-video-intel GIT tip
> (commit be3748802398a741208715233d36935378ceff58) on an
> OpenEmbedded / Yocto derived distro.
> 
> This machine I have to configure and develop for is a POS machine with this 
> Intel chip:
> 
> # lspci -s 00:02.0
> 00:02.0 VGA compatible controller: Intel Corporation Atom Processor 
> D4xx/D5xx/N4xx/N5xx
> Integrated Graphics Controller (rev 02) (prog-if 00 [VGA controller])
> 
> Without a configuration file, I get clone mode with the two screens.
> But I would like to setup two separate screens with :0.0 and :0.1
> without Xinerama or unified framebuffer, in order to make fullscreen
> X apps cover only one of the screens.
> 
> Below is the xorg.conf I am trying to use with the touchscreen configuration 
> omitted.
> 
> -
> Section "Monitor"
> Identifier "Monitor-LVDS1"
> EndSection
> 
> Section "Monitor"
> Identifier "Monitor-VGA1"
> #Option "RightOf"  "Monitor-LVDS1"
> EndSection
> 
> Section "Device"
> Identifier "Intel0"
> Driver "intel"
> BusID  "PCI:0:2:0"
> Screen 0
> Option  "AccelMethod"  "sna"
> Option "Monitor-LVDS1" "LVDS1"
Option "ZaphodHeads" "LVDS1"

> Option "TearFree" "on"
> EndSection
> 
> Section "Device"
> Identifier "Intel1"
> Driver "intel"
> BusID  "PCI:0:2:0"
> Screen 1
> Option  "AccelMethod"  "sna"
> Option "Monitor-VGA1" "VGA1"
Option "ZaphodHeads" "VGA1"

> Option "TearFree" "on"
> EndSection
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
xorg@lists.x.org: X.Org support
Archives: http://lists.freedesktop.org/archives/xorg
Info: http://lists.x.org/mailman/listinfo/xorg
Your subscription address: %(user_address)s

Re: How to setup independent screens with the Intel driver?

[Chris Wilson]

On Fri, Oct 30, 2015 at 08:00:46PM +0100, Boszormenyi Zoltan wrote:
> 2015-10-30 19:15 keltezéssel, Chris Wilson írta:
> > On Fri, Oct 30, 2015 at 06:17:50PM +0100, Boszormenyi Zoltan wrote:
> >> In my case with this particular POS machine, the intended primary display,
> >> the built-in LVDS with a touchscreen attached is apparently wired to pipe 
> >> 1.
> >>
> >> With this driver behaviour, I can't configure it to be kept as the default 
> >> :0.0 screen
> >> if an external display is plugged in. Can this behaviour be changed?
> > It would need another user parameter. There are several technical
> > limitations that make automatic assignment difficult. So try
> >
> > commit 94d271b239d358f71ae0bcfcc31422a569d73d41
> > Author: Chris Wilson <ch...@chris-wilson.co.uk>
> > Date:   Fri Oct 30 18:07:37 2015 +
> >
> > sna: Allow pipes to be manually assigned to ZaphodHead
> > -Chris
> 
> Thanks for your work, I just tested it. At startup, it seems to work:
> 1. it doesn't complain about invalid pipe
> 2. cursor appears on :0
> 3. application appears on :0
> 
> Then as soon as I touch the touchscreen, the cursor jumps to :1 and
> further cursor movements are on :1 from that point.

I'm baffled. xinput fun? cursors in the driver are allocated per-CRTC
and handled at a screen level. As far as I am aware, the driver only
has to position a cursor at a certain coordinate on the framebuffer (and
so has to translate that into a CRTC location).
 
> Attached is my current configuration, hopefully I got it right.

Looks fine to me.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
xorg@lists.x.org: X.Org support
Archives: http://lists.freedesktop.org/archives/xorg
Info: http://lists.x.org/mailman/listinfo/xorg
Your subscription address: %(user_address)s

Re: How to setup independent screens with the Intel driver?

[Chris Wilson]

On Fri, Oct 30, 2015 at 05:56:35PM +0100, Boszormenyi Zoltan wrote:
> 2015-10-30 17:46 keltezéssel, Boszormenyi Zoltan írta:
> > 2015-10-30 17:36 keltezéssel, Chris Wilson írta:
> >> On Fri, Oct 30, 2015 at 04:58:49PM +0100, Boszormenyi Zoltan wrote:
> >>> Section "Device"
> >>> Identifier "Intel0"
> >>> Driver "intel"
> >>> BusID  "PCI:0:2:0"
> >>> Screen 0
> >>> Option  "AccelMethod"  "sna"
> >>> Option "Monitor-LVDS1" "LVDS1"
> >> Option "ZaphodHeads" "LVDS1"
> >>
> >>> Option "TearFree" "on"
> >>> EndSection
> >>>
> >>> Section "Device"
> >>> Identifier "Intel1"
> >>> Driver "intel"
> >>> BusID  "PCI:0:2:0"
> >>> Screen 1
> >>> Option  "AccelMethod"  "sna"
> >>> Option "Monitor-VGA1" "VGA1"
> >> Option "ZaphodHeads" "VGA1"
> >>
> >>> Option "TearFree" "on"
> >>> EndSection
> >> -Chris
> > I tried adding the ZaphodHeads option to the device sections before.
> > It didn't work. The driver throws an error:
> >
> > [3114226.306] (II) intel(0): Using Kernel Mode Setting driver: i915, 
> > version 1.6.0 20150522
> > [3114226.307] (II) intel(1): Using Kernel Mode Setting driver: i915, 
> > version 1.6.0 20150522
> > [3114226.307] (--) intel(0): Integrated Graphics Chipset: Intel(R) Pineview 
> > G
> > [3114226.308] (--) intel(0): CPU: x86-64, sse2, sse3, ssse3; using a 
> > maximum of 2 threads
> > [3114226.308] (==) intel(0): Depth 24, (--) framebuffer bpp 32
> > [3114226.308] (==) intel(0): RGB weight 888
> > [3114226.308] (==) intel(0): Default visual is TrueColor
> > [3114226.308] (**) intel(0): Option "AccelMethod" "sna"
> > [3114226.308] (**) intel(0): Option "ZaphodHeads" "LVDS1"
> > [3114226.308] (**) intel(0): Option "TearFree" "on"
> > [3114226.309] (EE) intel(0): LVDS1 is an invalid output for screen (pipe) 0
> 
> It seems the device pipe and screen matching doesn't work properly.
> The driver seems to be assuming that its pipe numbers always match
> the same X screen numbers.

Assuming? That's how I documented it.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
xorg@lists.x.org: X.Org support
Archives: http://lists.freedesktop.org/archives/xorg
Info: http://lists.x.org/mailman/listinfo/xorg
Your subscription address: %(user_address)s

Re: How to setup independent screens with the Intel driver?

[Chris Wilson]

On Fri, Oct 30, 2015 at 05:46:51PM +0100, Boszormenyi Zoltan wrote:
> 2015-10-30 17:36 keltezéssel, Chris Wilson írta:
> > On Fri, Oct 30, 2015 at 04:58:49PM +0100, Boszormenyi Zoltan wrote:
> >> Section "Device"
> >> Identifier "Intel0"
> >> Driver "intel"
> >> BusID  "PCI:0:2:0"
> >> Screen 0
> >> Option  "AccelMethod"  "sna"
> >> Option "Monitor-LVDS1" "LVDS1"
> > Option "ZaphodHeads" "LVDS1"
> >
> >> Option "TearFree" "on"
> >> EndSection
> >>
> >> Section "Device"
> >> Identifier "Intel1"
> >> Driver "intel"
> >> BusID  "PCI:0:2:0"
> >> Screen 1
> >> Option  "AccelMethod"  "sna"
> >> Option     "Monitor-VGA1" "VGA1"
> > Option "ZaphodHeads" "VGA1"
> >
> >> Option "TearFree" "on"
> >> EndSection
> > -Chris
> 
> I tried adding the ZaphodHeads option to the device sections before.
> It didn't work. The driver throws an error:

Swap around Screen 0 and Screen 1, i.e. use VGA in Screen 0 and LVDS1 in
Screen 0.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
xorg@lists.x.org: X.Org support
Archives: http://lists.freedesktop.org/archives/xorg
Info: http://lists.x.org/mailman/listinfo/xorg
Your subscription address: %(user_address)s

Re: How to setup independent screens with the Intel driver?

[Chris Wilson]

On Fri, Oct 30, 2015 at 06:17:50PM +0100, Boszormenyi Zoltan wrote:
> 2015-10-30 18:03 keltezéssel, Chris Wilson írta:
> > On Fri, Oct 30, 2015 at 05:56:35PM +0100, Boszormenyi Zoltan wrote:
> >> 2015-10-30 17:46 keltezéssel, Boszormenyi Zoltan írta:
> >>> 2015-10-30 17:36 keltezéssel, Chris Wilson írta:
> >>>> On Fri, Oct 30, 2015 at 04:58:49PM +0100, Boszormenyi Zoltan wrote:
> >>>>> Section "Device"
> >>>>> Identifier "Intel0"
> >>>>> Driver "intel"
> >>>>> BusID  "PCI:0:2:0"
> >>>>> Screen 0
> >>>>> Option  "AccelMethod"  "sna"
> >>>>> Option "Monitor-LVDS1" "LVDS1"
> >>>> Option "ZaphodHeads" "LVDS1"
> >>>>
> >>>>> Option "TearFree" "on"
> >>>>> EndSection
> >>>>>
> >>>>> Section "Device"
> >>>>> Identifier "Intel1"
> >>>>> Driver "intel"
> >>>>> BusID  "PCI:0:2:0"
> >>>>> Screen 1
> >>>>> Option  "AccelMethod"  "sna"
> >>>>> Option "Monitor-VGA1" "VGA1"
> >>>> Option "ZaphodHeads" "VGA1"
> >>>>
> >>>>> Option "TearFree" "on"
> >>>>> EndSection
> >>>> -Chris
> >>> I tried adding the ZaphodHeads option to the device sections before.
> >>> It didn't work. The driver throws an error:
> >>>
> >>> [3114226.306] (II) intel(0): Using Kernel Mode Setting driver: i915, 
> >>> version 1.6.0 20150522
> >>> [3114226.307] (II) intel(1): Using Kernel Mode Setting driver: i915, 
> >>> version 1.6.0 20150522
> >>> [3114226.307] (--) intel(0): Integrated Graphics Chipset: Intel(R) 
> >>> Pineview G
> >>> [3114226.308] (--) intel(0): CPU: x86-64, sse2, sse3, ssse3; using a 
> >>> maximum of 2 threads
> >>> [3114226.308] (==) intel(0): Depth 24, (--) framebuffer bpp 32
> >>> [3114226.308] (==) intel(0): RGB weight 888
> >>> [3114226.308] (==) intel(0): Default visual is TrueColor
> >>> [3114226.308] (**) intel(0): Option "AccelMethod" "sna"
> >>> [3114226.308] (**) intel(0): Option "ZaphodHeads" "LVDS1"
> >>> [3114226.308] (**) intel(0): Option "TearFree" "on"
> >>> [3114226.309] (EE) intel(0): LVDS1 is an invalid output for screen (pipe) > >>> 0
> >> It seems the device pipe and screen matching doesn't work properly.
> >> The driver seems to be assuming that its pipe numbers always match
> >> the same X screen numbers.
> > Assuming? That's how I documented it.
> > -Chris
> 
> "man intel" on Fedora 22 doesn't says anything like that. Where is it 
> documented?

Apparently in my imagination. I could have sworn that I wrote about the
relationship between the Screen number and the CRTC it was assigned and
how you needed to look at the xrandr output to work out possible
arrangements. Probably lost in some bugzilla.
 
> In my case with this particular POS machine, the intended primary display,
> the built-in LVDS with a touchscreen attached is apparently wired to pipe 1.
> 
> With this driver behaviour, I can't configure it to be kept as the default 
> :0.0 screen
> if an external display is plugged in. Can this behaviour be changed?

It would need another user parameter. There are several technical
limitations that make automatic assignment difficult. So try

commit 94d271b239d358f71ae0bcfcc31422a569d73d41
Author: Chris Wilson <ch...@chris-wilson.co.uk>
Date:   Fri Oct 30 18:07:37 2015 +

sna: Allow pipes to be manually assigned to ZaphodHead
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
xorg@lists.x.org: X.Org support
Archives: http://lists.freedesktop.org/archives/xorg
Info: http://lists.x.org/mailman/listinfo/xorg
Your subscription address: %(user_address)s

[PATCH] os: Fix iteration over busfaults

[Chris Wilson]

Fixes a regression from

commit 41da295eb50fa08eaacd0ecde99f43a716fcb41a
Author: Keith Packard <kei...@keithp.com>
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 <ch...@chris-wilson.co.uk>
---
 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, , list) {
-if ((char *) busfault->addr <= (char *) fault && (char *) fault < 
(char *) busfault->addr + busfault->size) {
-break;
-}
+xorg_list_for_each_entry(iter, , 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

[xrandr] Pretty print modeFlags on unattached modes

[Chris Wilson]

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 <ch...@chris-wilson.co.uk>
---
 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

[xrandr] Only use the current information when setting modes

[Chris Wilson]

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 <ch...@chris-wilson.co.uk>
---
 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 (_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 (_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: [xrandr] Only use the current information when setting modes

[Chris Wilson]

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

[Chris Wilson]

On Sun, Sep 13, 2015 at 01:08:11PM +0200, Mark Kettenis wrote:
> > From: Chris Wilson <ch...@chris-wilson.co.uk>
> > 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

Re: [xrandr] Only use the current information when setting modes

[Chris Wilson]

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 <ch...@chris-wilson.co.uk>
> > 
> > On Sun, Sep 13, 2015 at 01:08:11PM +0200, Mark Kettenis wrote:
> > > > From: Chris Wilson <ch...@chris-wilson.co.uk>
> > > > 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: [PATCH] Xephyr: Paint with subimage for non-Glamor non-XSHM case

[Chris Wilson]

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 jcris...@debian.org
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 ian.sc...@arteris.com
 ---
  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 ch...@chris-wilson.co.uk
-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 1/2] Mark disabling an output as a change in its CRTC

[Chris Wilson]

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 nmschu...@gmail.com
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
---
 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

[xrandr 2/2] Mark all CRTC as currently unused for second picking CRTC pass

[Chris Wilson]

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 nmschu...@gmail.com
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
---
 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

Re: [PATCH 6/6] prime: add rotation support for offloaded outputs

[Chris Wilson]

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 1/2] atom: Bump initial table size

[Chris Wilson]

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 ch...@chris-wilson.co.uk
-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

[Chris Wilson]

On Tue, Jun 02, 2015 at 02:08:39PM -0400, Adam Jackson wrote:
 Signed-off-by: Adam Jackson a...@redhat.com
Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk
-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

[Chris Wilson]

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

[Chris Wilson]

On Tue, Jun 02, 2015 at 02:14:59PM -0400, Adam Jackson wrote:
 Signed-off-by: Adam Jackson a...@redhat.com

For this series,
Acked-by: Chris Wilson ch...@chris-wilson.co.uk
-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

[Chris Wilson]

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 mic...@daenzer.net 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

[Chris Wilson]

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

[Chris Wilson]

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 5/5] render: Eliminate temporary mask when drawing glyphs

[Chris Wilson]

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

[Chris Wilson]

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 2/3] present: Query the Window's CRTC every time

[Chris Wilson]

On Tue, May 26, 2015 at 02:30:32PM -0700, Keith Packard wrote:
 Michel Dänzer mic...@daenzer.net 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

[PATCH] glamor/glyphs: Fix rendering regressions

[Chris Wilson]

commit b0d2e010316d710eb4052963de3a1e2dc7ba356e
Author: Keith Packard kei...@keithp.com
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 ch...@chris-wilson.co.uk
Cc: Keith Packard kei...@keithp.com
---
 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,
 (glyph_pix_priv != 0  glyph_pix_priv-type 
!= GLAMOR_MEMORY)))
 {
 if (glyphs_queued

Re: [PATCH] composite: Install SourceValidation hooks only when required

[Chris Wilson]

On Mon, May 18, 2015 at 03:26:29PM +0100, Chris Wilson wrote:
 Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk

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

[Chris Wilson]

Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
---
 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 compClearDamaged(WindowPtr pWin)
+{
+ScreenPtr pScreen = pWin-drawable.pScreen;
+CompScreenPtr cs = GetCompScreen(pScreen);
+cs-anyDamaged--;
+}
+
 void
 compPaintChildrenToWindow(WindowPtr pWin)
 {
@@ -743,6 +750,7 @@ compPaintChildrenToWindow(WindowPtr pWin

Re: combining Render's Composite + SHM's PutImage

[Chris Wilson]

On Thu, Apr 09, 2015 at 11:01:47AM +1000, Nigel Tao wrote:
 I'm possibly Doing It Wrong, but...
 
 I'm playing around with an X client, I have some shared memory full of
 pixels, and I want to paint them on a window, with alpha-blending. I
 also want to know when the paint is complete, so I can re-use that
 shared memory buffer for different pixels.
 
 IIUC, Render gives me the alpha-blending, but not the SHM completion
 event. SHM gives me the opposite.
 
 Is there a good way that I can have my cake and eat it too?
 
 I suppose that I could call SHM's GetImage to a temporary shared
 buffer, do the alpha-blending on the client, and call SHM's PutImage.
 Or, I could call SHM's PutImage to a temporary server-side buffer, and
 call Render's Composite. But neither approach seems ideal, as it
 involves creating and cleaning up a temporary buffer.

You can either read cairo for an example of how to mix SHM and Render,
or 
http://cgit.freedesktop.org/xorg/driver/xf86-video-intel/tree/tools/virtual.c#n1819
The synopsis is that you want to insert your own SHMCompletionEvents
which are then guarranteed to be sent after the rendering by X's very
strict protocol ordering.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
xorg@lists.x.org: X.Org support
Archives: http://lists.freedesktop.org/archives/xorg
Info: http://lists.x.org/mailman/listinfo/xorg
Your subscription address: %(user_address)s

Re: [PATCH] modesetting: add tile property support (v2.1)

[Chris Wilson]

On Wed, Apr 01, 2015 at 10:31:43AM +1000, Dave Airlie wrote:
 From: Dave Airlie airl...@redhat.com
 
 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 airl...@redhat.com
Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk
-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] prime: Don't lose SourceValidate on PixmapSyncDirtyHelper no-ops

[Chris Wilson]

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 ch...@chris-wilson.co.uk
Cc: Dave Airlie airl...@redhat.com
---
 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

[PATCH 1/2] Xv: Only stop the adaptors when the Pixmap is finally destroyed

[Chris Wilson]

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 ch...@chris-wilson.co.uk
Cc: Ville Syrjälä ville.syrj...@linux.intel.com
---
 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

Re: [PATCH 3/3] modesetting: add tile property support (v2)

[Chris Wilson]

On Tue, Mar 31, 2015 at 01:47:26PM +1000, Dave Airlie wrote:
 From: Dave Airlie airl...@redhat.com
 
 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 airl...@redhat.com
 ---

 +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 3/3] modesetting: add tile property support (v2)

[Chris Wilson]

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 airl...@redhat.com
  
  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 airl...@redhat.com
  ---
 
  +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] glx/dri2: Disable AIGLX if indirect GLX is disabled

[Chris Wilson]

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 ch...@chris-wilson.co.uk
  ---
   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

[Chris Wilson]

There is no point in setting up the acceleration for indirect GLX if
indirect GLX is itself disabled.

Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
---
 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

[Chris Wilson]

On Thu, Feb 26, 2015 at 02:58:45PM -0500, Adam Jackson wrote:
 On Wed, 2015-02-25 at 18:01 +, 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

[Chris Wilson]

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 ch...@chris-wilson.co.uk
Cc: Adam Jackson a...@redhat.com
---
 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

Re: Limit DRI2Drawable reference leak

[Chris Wilson]

On Sun, Feb 22, 2015 at 02:09:36AM +0200, Ville Syrjälä wrote:
 On Sat, Feb 21, 2015 at 10:52:49PM +, 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

[PATCH 3/3] dri2: Only create one unnamed reference per Drawable per Client

[Chris Wilson]

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 k...@bitplanet.net
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 dr...@endlessm.com
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 ch...@chris-wilson.co.uk
Cc: Ville Syrjälä syrj...@sci.fi
---
 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)
+FreeResourceByType(pid, dri2ReferenceRes, FALSE);
+
 return

[PATCH 1/3] dri2: Only invalidate the immediate Window upon SetWindowPixmap

[Chris Wilson]

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 ch...@chris-wilson.co.uk
---
 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

[Chris Wilson]

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 ch...@chris-wilson.co.uk
---
 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;
 ref-priv = priv;
 xorg_list_add(ref-link, pPriv-reference_list);
@@ -324,71 +317,32 @@ DRI2AddDrawableRef(DRI2DrawablePtr pPriv, XID id

Re: Limit DRI2Drawable reference leak

[Chris Wilson]

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

[Chris Wilson]

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. 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 k...@bitplanet.net
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 k...@bitplanet.net
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

Re: [PATCH] dri2: work around broken DRI2CreateDrawable callers

[Chris Wilson]

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

[PATCH 3/3] dri2: Support named DRI2Drawable references

[Chris Wilson]

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 ch...@chris-wilson.co.uk
---
 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

[Chris Wilson]

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 ch...@chris-wilson.co.uk
---
 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, DrawablePtr pDraw, XID id)
 {
-DRI2DrawablePtr pPriv;
-DRI2ClientPtr dri2_client = dri2ClientPrivate(client);
-XID

[PATCH 2/3] dri2: Only create one unnamed reference per Drawable per Client

[Chris Wilson]

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 dr...@endlessm.com
Link: https://freedesktop.org/patch/19695/
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
---
 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

Limit DRI2Drawable reference leak

[Chris Wilson]

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

Re: DRI2InvalidateWalk high in perf top profile

[Chris Wilson]

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