Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

[Keith Packard]

Michel Dänzer  writes:

> This does mean though that if one has only up to patch 3 applied (e.g.
> during a bisection), one is exposed to the issues fixed by patch 4. So
> maybe patch 4 should be squashed into patch 3.

That's a very important point -- developing code in small logical steps
is a most excellent plan, but the resulting set of commits in git need
to satisfy different requirements:

 1) Make forward progress; no commit should regress any functionality,
no commit should introduce compiler warnings (or, even worse,
compiler errors). Adam Jackson has reminded me several times to use
'git rebase -x make' to check a long sequence of patches with the
compiler. Sometimes I remember on my own.

 2) Each patch should be a single change, self contained and easy to
understand. If you can't summarize the patch in one line, it's
probably too complicated for anyone to review effectively. If you
ever use the word 'and' in the summary line, that's a good sign that
the patch does more than one thing and should be split apart.

 3) The series should tell a good story. Just like writing a book, the
final product (sequence of patches or storyline) almost certainly
will not be presented in the original development
order. Splitting/merging patch chunks and reordering commits is
almost always necessary in a long patch series.

I think review takes time related to some high order polynomial function
of the patch length; maybe just quadratic, but possibly more. Long
patches that are purely mechanical changes (replacing function names,
cleaning up whitespace) might be an exception to this rule. Remember
that development is a shared activity, and that spending time making the
patches easy to review moves work from reviewer to committer, and that
is almost always a net win in total development time.

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

[Yu, Qiang]

Hi Emil,

Each point of the patch set is not broken. Patches are arranged like
this to show how I do it:
1. create a pageflip.c to host common page flip code
2. copy amdgpu DDX DRI2 page flip code to modesetting dri2.c
3. merge common DRI2 and present page flip code to pageflip.c

Patch 4 is not a fix for broken code, just a commit from amdgpu DDX
to prevent present and DRI2 from flipping at the same time.

Regards,
Qiang

From: Emil Velikov 
Sent: Thursday, August 18, 2016 7:03:12 PM
To: Yu, Qiang
Cc: ML xorg-devel; amd-...@lists.freedesktop.org
Subject: Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

Hi Qiang,

On 17 August 2016 at 11:29, Qiang Yu  wrote:
> Hi guys,
>
> This patch set is for adding DRI2 page flip support to modesetting
> driver. I mainly take reference of amdgpu DDX and reuse present
> page flip code in the modesetting driver.
>
> Regards,
> Qiang
>
> Qiang Yu (6):
>   modesetting: make ms_do_pageflip generic for share with DRI2
>   modesetting: move ms_do_pageflip to pageflip.c
>   modesetting: add DRI2 page flip support
>   modesetting: exclude DRI2 and Present page flip
>   modesetting: merge common page flip code for present and dri2
>   modesetting: remove redundent pixmap destroy
>
The work itself is quite interesting thanks for that !

Is it me or the newly added code (with patch 3) seems intermittently
broken (fixed with patch 4), and then removed with a later patch (5) ?
Should one factor the common pageflip code first, then add the
distinction between dri2 and present flips, then finally add the dri2
implementation ?

Regards,
Emil
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

[Michel Dänzer]

On 19/08/16 11:02 AM, Yu, Qiang wrote:
> 
> Each point of the patch set is not broken. Patches are arranged like
> this to show how I do it:
> 1. create a pageflip.c to host common page flip code
> 2. copy amdgpu DDX DRI2 page flip code to modesetting dri2.c
> 3. merge common DRI2 and present page flip code to pageflip.c
> 
> Patch 4 is not a fix for broken code, just a commit from amdgpu DDX
> to prevent present and DRI2 from flipping at the same time.

This does mean though that if one has only up to patch 3 applied (e.g.
during a bisection), one is exposed to the issues fixed by patch 4. So
maybe patch 4 should be squashed into patch 3.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

[Michel Dänzer]

On 19/08/16 01:56 AM, ⚛ wrote:
> 
> There is some confusion at
> https://www.phoronix.com/forums/forum/phoronix/latest-phoronix-articles/891632-dri2-page-flipping-patches-for-the-generic-modesetting-ddx
> about this patch.

Confusion in the Phoronix forums? No way! ;)


> There was no previous DRI2 page flipping support in modesetting?

That's correct.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

[Yu, Qiang]

Thanks Michel. I'll update patches in V2 according to your comments.

Regards,
Qiang

From: Michel Dänzer 
Sent: Thursday, August 18, 2016 4:18:11 PM
To: Yu, Qiang
Cc: xorg-devel@lists.x.org; amd-...@lists.freedesktop.org
Subject: Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

On 17/08/16 07:29 PM, Qiang Yu wrote:
> Hi guys,
>
> This patch set is for adding DRI2 page flip support to modesetting
> driver. I mainly take reference of amdgpu DDX and reuse present
> page flip code in the modesetting driver.
>
> Regards,
> Qiang
>
> Qiang Yu (6):
>   modesetting: make ms_do_pageflip generic for share with DRI2
>   modesetting: move ms_do_pageflip to pageflip.c
>   modesetting: add DRI2 page flip support
>   modesetting: exclude DRI2 and Present page flip
>   modesetting: merge common page flip code for present and dri2
>   modesetting: remove redundent pixmap destroy

Patches 2, 4 & 5 are

Reviewed-by: Michel Dänzer 


Patch 6 was already submitted by Hans de Goede:

https://patchwork.freedesktop.org/patch/105351/


--
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] wayland: Emulate crossing for native window

[Peter Hutterer]

On Wed, Aug 17, 2016 at 03:53:14AM -0400, Olivier Fourdan wrote:
> Hi all,
> 
> Gentle reminder about this patch... 

sorry about the delay.

   48c5c23..6e5bec2  master -> master

Cheers,
   Peter

> 
> Cheers,
> Olivier
> 
> - Original Message -
> > Emitting a LeaveNotify event every time the pointer leaves an X11 window
> > may confuse focus follow mouse mode in window managers such as
> > mutter/gnome-shell.
> > 
> > Keep the previously found X window and compare against the new one, and
> > if they match then it means the pointer has left an Xwayland window for
> > a native Wayland surface, only in this case fake the crossing to the
> > root window.
> > 
> > Signed-off-by: Olivier Fourdan 
> > ---
> >  hw/xwayland/xwayland-input.c | 15 ++-
> >  hw/xwayland/xwayland.c   |  3 ++-
> >  hw/xwayland/xwayland.h   |  1 +
> >  3 files changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
> > index e295c71..043379e 100644
> > --- a/hw/xwayland/xwayland-input.c
> > +++ b/hw/xwayland/xwayland-input.c
> > @@ -959,7 +959,7 @@ xwl_xy_to_window(ScreenPtr screen, SpritePtr sprite, int
> > x, int y)
> >  }
> >  }
> >  
> > -if (xwl_seat == NULL || !xwl_seat->focus_window) {
> > +if (xwl_seat == NULL) {
> >  sprite->spriteTraceGood = 1;
> >  return sprite->spriteTrace[0];
> >  }
> > @@ -969,6 +969,19 @@ xwl_xy_to_window(ScreenPtr screen, SpritePtr sprite, 
> > int
> > x, int y)
> >  xwl_seat->xwl_screen->XYToWindow = screen->XYToWindow;
> >  screen->XYToWindow = xwl_xy_to_window;
> >  
> > +/* If the pointer has left the Wayland surface but the DIX still
> > + * finds the pointer within the previous X11 window, it means that
> > + * the pointer has crossed to another native Wayland window, in this
> > + * case, pretend we entered the root window so that a LeaveNotify
> > + * event is emitted.
> > + */
> > +if (xwl_seat->focus_window == NULL && xwl_seat->last_xwindow == ret) {
> > +sprite->spriteTraceGood = 1;
> > +return sprite->spriteTrace[0];
> > +}
> > +
> > +xwl_seat->last_xwindow = ret;
> > +
> >  return ret;
> >  }
> >  
> > diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
> > index 8c49b0b..1b29491 100644
> > --- a/hw/xwayland/xwayland.c
> > +++ b/hw/xwayland/xwayland.c
> > @@ -323,7 +323,8 @@ xwl_unrealize_window(WindowPtr window)
> >  xorg_list_for_each_entry(xwl_seat, _screen->seat_list, link) {
> >  if (xwl_seat->focus_window && xwl_seat->focus_window->window ==
> >  window)
> >  xwl_seat->focus_window = NULL;
> > -
> > +if (xwl_seat->last_xwindow == window)
> > +xwl_seat->last_xwindow = NullWindow;
> >  xwl_seat_clear_touch(xwl_seat, window);
> >  }
> >  
> > diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
> > index b8a58e7..aa78b44 100644
> > --- a/hw/xwayland/xwayland.h
> > +++ b/hw/xwayland/xwayland.h
> > @@ -132,6 +132,7 @@ struct xwl_seat {
> >  struct wl_surface *cursor;
> >  struct wl_callback *cursor_frame_cb;
> >  Bool cursor_needs_update;
> > +WindowPtr last_xwindow;
> >  
> >  struct xorg_list touches;
> >  
> > --
> > 2.7.4
> > 
> > ___
> > xorg-devel@lists.x.org: X.Org development
> > Archives: http://lists.x.org/archives/xorg-devel
> > Info: https://lists.x.org/mailman/listinfo/xorg-devel
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
> 
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

[⚛]

Hello

There is some confusion at
https://www.phoronix.com/forums/forum/phoronix/latest-phoronix-articles/891632-dri2-page-flipping-patches-for-the-generic-modesetting-ddx
about this patch.

There was no previous DRI2 page flipping support in modesetting? What
is improved by the patch?

Thanks.
___
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: Michel Dänzer is invited to help maintain master X server branch

[Eric Anholt]

Keith Packard  writes:

> Michel Dänzer  writes:
>
>> Many glamor changes have been pushed by Adam or Keith lately. Is that
>> intentional, i.e. is it okay for others to push glamor changes which are
>> ready, or should we always get in touch with Eric first?
>
> Would saying "it depends" help at all? When there's a major chunk of
> code getting ready to be merged, then coordinating through a single
> person can avoid merge conflicts. In quiet times, then working
> separately will reduce communication overhead.
>
> So, I think the key is to know when things are busy and when things are
> quiet, and to have a sense of how much conflict any particular patch is
> likely to cause with other changes in process. It's probably not a great
> idea to merge a whitespace patch touching every file in the repo just
> before someone merges a pile of changes to a single file.
>
> When in doubt, feel free to just ask around. If several of us agree that
> merging a patch seems fine, then at least it's not entirely your fault?

I'm generally in favor of people pushing reviewed patches to glamor.
The only thing I'll get grumpy about is new code without coverage in XTS
or rendercheck.  X rendering is hard to get right, so make sure that
there's a test covering it.

However, the manual operation of our testsuites is also awful (which
subsets am I supposed to actually run?  How do I know if I succeeded,
when there are expected failures?).  Having spent a while recently
working in a project with actual CI, I'll try not to be too irritated
about others not doing testing on X when I haven't actually made X's
rendering testing usable.


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] xace: Fix XaceCensorImage to actually censor the right part of the image

[Keith Packard]

Aaron Plattner  writes:

> Aargh, stupid borders. I always forget about them. I guess this is why
> we have regression tests.

You just don't recall the whole original fight about borders. Left in
X11 solely for X10 compatibility. Aren't you glad we have them?

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] Fix Xorg -configure not working anymore

[Adam Jackson]

On Thu, 2016-08-18 at 11:17 +0200, Hans de Goede wrote:
> Hi,
> 
> On 17-08-16 16:58, Adam Jackson wrote:
> > 
> > On Wed, 2016-08-17 at 10:01 +0900, Michel Dänzer wrote:
> > 
> > > 
> > > At least longer term, maybe we should just remove Xorg -configure? Does
> > > it serve any useful purpose anymore? I know it causes quite a lot of
> > > trouble for users and those trying to help them...
> > 
> > I really want to say yes, ...
> 
> As interesting as this discussion is, I would still like to see
> us move forward with a fix for now.

Yeah, fair enough.

remote: I: patch #103826 updated using rev 
48c5c23a1b250c7f9d7a1747c76e4669ebf752cf.
remote: I: 1 patch(es) updated to state Accepted.
To ssh://git.freedesktop.org/git/xorg/xserver
   6acd0d0..48c5c23  master -> master

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] xace: Fix XaceCensorImage to actually censor the right part of the image

[Aaron Plattner]

Aargh, stupid borders. I always forget about them. I guess this is why
we have regression tests.

On 08/18/2016 09:11 AM, Adam Jackson wrote:
> On Thu, 2016-08-18 at 11:09 +0900, Michel Dänzer wrote:
> 
>> Unfortunately, this broke two XTS tests:
>>
>> xts5@xlib9@xgetimage@7
>> xts5@xlib9@xgetsubimage@7

Thanks for catching this.

> Low impact, fortunately, but still unpleasant. The test in question is:
> 
> 520|0 7 00020031 1 2|Assertion XGetImage-7.(A)
> 520|0 7 00020031 1 3|When the specified rectangle includes the window border,
> 520|0 7 00020031 1 4|then the contents of the window border are obtained in 
> the
> 520|0 7 00020031 1 5|XImage structure returned by a call to XGetImage.
> 
> I think there are two issues here. One is pVisibleRegion (the region we
> don't censor) is the intersection of borderClip (the exterior
> dimensions of the window including the border, clipped by siblings) and
> winSize (the inside-the-border region of the window). Clipping by
> winSize means we'll censor the window border. I think what's actually
> wanted there is borderClip also clipped by children [1]; we don't have
> a function handy to compute that, but it's straightforward enough.
> 
> The other issue is we censor the image unconditionally if the server
> was built with support for any security extensions, regardless of
> whether the requesting client is trusted (for XC-SECURITY) or in a
> different security context than the window (for XACE).
> 
> Patches forthcoming.

And thanks Adam for fixing it.

> [1] - Well kinda. You want to clip away children whose contents you
> aren't authorized to see, which isn't quite the same.
> 
> - ajax
> 

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xf86-input-libinput] Always delay hotplugging subdevices

[Keith Packard]

Peter Hutterer  writes:

> Avoid creating new devices from within the input thread which was the case for
> tablet tools. It requires a lot more care about locking and has a potential to
> mess up things.
>
> Instead, schedule a WorkProc and buffer all events until we have the device
> created. Once that's done, replay the event sequence so far. If the device
> comes into proximity and out again before we manage to create the new device
> we just ditch the whole sequence and wait for the next proximity in.
>
> Signed-off-by: Peter Hutterer 
> ---
>  src/xf86libinput.c | 259 
> +
>  1 file changed, 202 insertions(+), 57 deletions(-)
>
> diff --git a/src/xf86libinput.c b/src/xf86libinput.c
> index 1ecbc41..9b9ba12 100644
> --- a/src/xf86libinput.c
> +++ b/src/xf86libinput.c
> @@ -102,6 +102,12 @@ struct xf86libinput_device {
>   struct xorg_list unclaimed_tablet_tool_list;
>  };
>  
> +struct xf86libinput_tablet_tool_event_queue {
> + bool need_to_queue;
> + struct libinput_event_tablet_tool *events[128];
> + size_t nevents;

Any particular reason to use an array of fixed size here, instead of a
list which wouldn't have any limit?

> -static void
> +static inline void

Yeah, just drop the 'inline' for the huge functions in this file; the
compiler will probably ignore it anyways...


> +xf86libinput_tool_replay_events(struct xf86libinput_tablet_tool_event_queue 
> *queue)
> +{
> + size_t i;
> +
> + for (i = 0; i < queue->nevents; i++) {
> + struct libinput_event *event;
> + event = 
> libinput_event_tablet_tool_get_base_event(queue->events[i]);
> + xf86libinput_handle_event(event);
> + queue->events[i] = NULL;
> + /* we're not queueing anymore so we expect handle_event to
> +libinput_event_destroy() */

looks like handle_event just returns whether to destroy the event, so
you'd need a call here, or make handle_event handle the event
destruction internally.

The rest of this looks reasonable to me.

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] modesetting: Fix msSharePixmapBacking returning a non-linear bo

[Keith Packard]

Hans de Goede  writes:

> An alternative solution would be to add a flag argument to
> glamor_fd_from_pixmap, so that callers like this one which
> know for sure they want a linear bo can specify that through
> a flag, and other callers can just pass in:

fd_from_pixmap is the API exposed for DRI3 to move pixmaps between
applications and the server. Using this for driver to driver sharing
seems like a stretch to me, so instead of adding an argument to this
function, we should just create a new function which does what we
want. Internally, that could either pass the necessary values through
the rest of the glamor API or it could do the hack which you've already
identified.

Maybe 'glamor_fd_from_back_pixmap'? given where this is used?
Suggestions welcome.

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] xace: Fix XaceCensorImage to actually censor the right part of the image

[Adam Jackson]

On Thu, 2016-08-18 at 11:09 +0900, Michel Dänzer wrote:

> Unfortunately, this broke two XTS tests:
> 
> xts5@xlib9@xgetimage@7
> xts5@xlib9@xgetsubimage@7

Low impact, fortunately, but still unpleasant. The test in question is:

520|0 7 00020031 1 2|Assertion XGetImage-7.(A)
520|0 7 00020031 1 3|When the specified rectangle includes the window border,
520|0 7 00020031 1 4|then the contents of the window border are obtained in the
520|0 7 00020031 1 5|XImage structure returned by a call to XGetImage.

I think there are two issues here. One is pVisibleRegion (the region we
don't censor) is the intersection of borderClip (the exterior
dimensions of the window including the border, clipped by siblings) and
winSize (the inside-the-border region of the window). Clipping by
winSize means we'll censor the window border. I think what's actually
wanted there is borderClip also clipped by children [1]; we don't have
a function handy to compute that, but it's straightforward enough.

The other issue is we censor the image unconditionally if the server
was built with support for any security extensions, regardless of
whether the requesting client is trusted (for XC-SECURITY) or in a
different security context than the window (for XACE).

Patches forthcoming.

[1] - Well kinda. You want to clip away children whose contents you
aren't authorized to see, which isn't quite the same.

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: Michel Dänzer is invited to help maintain master X server branch

[Keith Packard]

Michel Dänzer  writes:

> Many glamor changes have been pushed by Adam or Keith lately. Is that
> intentional, i.e. is it okay for others to push glamor changes which are
> ready, or should we always get in touch with Eric first?

Would saying "it depends" help at all? When there's a major chunk of
code getting ready to be merged, then coordinating through a single
person can avoid merge conflicts. In quiet times, then working
separately will reduce communication overhead.

So, I think the key is to know when things are busy and when things are
quiet, and to have a sense of how much conflict any particular patch is
likely to cause with other changes in process. It's probably not a great
idea to merge a whitespace patch touching every file in the repo just
before someone merges a pile of changes to a single file.

When in doubt, feel free to just ask around. If several of us agree that
merging a patch seems fine, then at least it's not entirely your fault?

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver v3] glamor: Handle bitplane in glamor_copy_fbo_cpu

[Alex Deucher]

On Thu, Aug 18, 2016 at 5:42 AM, Michel Dänzer  wrote:
> From: Michel Dänzer 
>
> This can significantly speed up at least some CopyPlane cases, e.g.
> indirectly for stippled fills.
>
> v2:
> * Make temporary pixmap the same size as the destination pixmap
>   (instead of the destination drawable size), and fix coordinate
>   parameters passed to fbCopyXtoX and glamor_upload_boxes. Fixes
>   incorrect rendering rendering with x11perf -copyplane* and crashes
>   with the xscreensaver phosphor hack.
> v3:
> * Make the change a bit more compact and hopefully more readable by
>   re-using the existing src_* locals in the bitplane case as well.
>
> Reported-by: Keith Raghubar 
> Signed-off-by: Michel Dänzer 

Reviewed-by: Alex Deucher 

> ---
>  glamor/glamor_copy.c | 40 +---
>  1 file changed, 33 insertions(+), 7 deletions(-)
>
> diff --git a/glamor/glamor_copy.c b/glamor/glamor_copy.c
> index 3501a0d..5b5f0e6 100644
> --- a/glamor/glamor_copy.c
> +++ b/glamor/glamor_copy.c
> @@ -220,11 +220,37 @@ glamor_copy_cpu_fbo(DrawablePtr src,
>
>  glamor_get_drawable_deltas(dst, dst_pixmap, _xoff, _yoff);
>
> -fbGetDrawable(src, src_bits, src_stride, src_bpp, src_xoff, src_yoff);
> +if (bitplane) {
> +PixmapPtr src_pix = fbCreatePixmap(screen, 
> dst_pixmap->drawable.width,
> +   dst_pixmap->drawable.height,
> +   dst->depth, 0);
> +
> +if (!src_pix) {
> +glamor_finish_access(src);
> +goto bail;
> +}
>
> -glamor_upload_boxes(dst_pixmap, box, nbox, src_xoff + dx, src_yoff + dy,
> -dst_xoff, dst_yoff,
> -(uint8_t *) src_bits, src_stride * sizeof (FbBits));
> +fbGetDrawable(_pix->drawable, src_bits, src_stride, src_bpp, 
> src_xoff,
> +  src_yoff);
> +
> +if (src->bitsPerPixel > 1)
> +fbCopyNto1(src, _pix->drawable, gc, box, nbox,
> +   dst_xoff + dx, dst_yoff + dy, reverse, upsidedown,
> +   bitplane, closure);
> +else
> +fbCopy1toN(src, _pix->drawable, gc, box, nbox,
> +   dst_xoff + dx, dst_yoff + dy, reverse, upsidedown,
> +   bitplane, closure);
> +
> +glamor_upload_boxes(dst_pixmap, box, nbox, 0, 0, 0, 0,
> +(uint8_t *) src_bits, src_stride * 
> sizeof(FbBits));
> +fbDestroyPixmap(src_pix);
> +} else {
> +fbGetDrawable(src, src_bits, src_stride, src_bpp, src_xoff, 
> src_yoff);
> +glamor_upload_boxes(dst_pixmap, box, nbox, src_xoff + dx, src_yoff + 
> dy,
> +dst_xoff, dst_yoff,
> +(uint8_t *) src_bits, src_stride * sizeof 
> (FbBits));
> +}
>  glamor_finish_access(src);
>
>  return TRUE;
> @@ -616,9 +642,9 @@ glamor_copy_gl(DrawablePtr src,
>  return glamor_copy_fbo_fbo_draw(src, dst, gc, box, nbox, dx, 
> dy,
>  reverse, upsidedown, 
> bitplane, closure);
>  }
> -if (bitplane == 0)
> -return glamor_copy_cpu_fbo(src, dst, gc, box, nbox, dx, dy,
> -   reverse, upsidedown, bitplane, 
> closure);
> +
> +return glamor_copy_cpu_fbo(src, dst, gc, box, nbox, dx, dy,
> +   reverse, upsidedown, bitplane, closure);
>  } else if (GLAMOR_PIXMAP_PRIV_HAS_FBO(src_priv) &&
> dst_priv->type != GLAMOR_DRM_ONLY &&
> bitplane == 0) {
> --
> 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
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

[Emil Velikov]

Hi Qiang,

On 17 August 2016 at 11:29, Qiang Yu  wrote:
> Hi guys,
>
> This patch set is for adding DRI2 page flip support to modesetting
> driver. I mainly take reference of amdgpu DDX and reuse present
> page flip code in the modesetting driver.
>
> Regards,
> Qiang
>
> Qiang Yu (6):
>   modesetting: make ms_do_pageflip generic for share with DRI2
>   modesetting: move ms_do_pageflip to pageflip.c
>   modesetting: add DRI2 page flip support
>   modesetting: exclude DRI2 and Present page flip
>   modesetting: merge common page flip code for present and dri2
>   modesetting: remove redundent pixmap destroy
>
The work itself is quite interesting thanks for that !

Is it me or the newly added code (with patch 3) seems intermittently
broken (fixed with patch 4), and then removed with a later patch (5) ?
Should one factor the common pageflip code first, then add the
distinction between dri2 and present flips, then finally add the dri2
implementation ?

Regards,
Emil
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver v2] glamor: Handle bitplane in glamor_copy_fbo_cpu

[Michel Dänzer]

On 15/08/16 11:18 PM, walter harms wrote:
> Am 15.08.2016 11:43, schrieb Michel Dänzer:
>>
>> diff --git a/glamor/glamor_copy.c b/glamor/glamor_copy.c
>> index 3501a0d..82e040a 100644
>> --- a/glamor/glamor_copy.c
>> +++ b/glamor/glamor_copy.c
>> @@ -222,9 +222,40 @@ glamor_copy_cpu_fbo(DrawablePtr src,
>>  
>>  fbGetDrawable(src, src_bits, src_stride, src_bpp, src_xoff, src_yoff);
>>  
>> -glamor_upload_boxes(dst_pixmap, box, nbox, src_xoff + dx, src_yoff + dy,
>> -dst_xoff, dst_yoff,
>> -(uint8_t *) src_bits, src_stride * sizeof (FbBits));
> 
> 
> my i suggest to invert the question to improve readability ?
> 
>   if (!bitplane) {
>   glamor_upload_boxes(dst_pixmap, box, nbox, src_xoff + dx, 
> src_yoff + dy,
> dst_xoff, dst_yoff,
> (uint8_t *) src_bits, src_stride * sizeof 
> (FbBits));
> glamor_finish_access(src);
>   return TRUE;
> }
> 
> btw: i hope return TRUE is intended here

Yes, it is.


> and then the rest ...

Thanks for your suggestion. I don't agree that it would improve
readability, but it did make me realize other potential to slightly
simplify the patch, see v3.


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

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver v3] glamor: Handle bitplane in glamor_copy_fbo_cpu

[Michel Dänzer]

From: Michel Dänzer 

This can significantly speed up at least some CopyPlane cases, e.g.
indirectly for stippled fills.

v2:
* Make temporary pixmap the same size as the destination pixmap
  (instead of the destination drawable size), and fix coordinate
  parameters passed to fbCopyXtoX and glamor_upload_boxes. Fixes
  incorrect rendering rendering with x11perf -copyplane* and crashes
  with the xscreensaver phosphor hack.
v3:
* Make the change a bit more compact and hopefully more readable by
  re-using the existing src_* locals in the bitplane case as well.

Reported-by: Keith Raghubar 
Signed-off-by: Michel Dänzer 
---
 glamor/glamor_copy.c | 40 +---
 1 file changed, 33 insertions(+), 7 deletions(-)

diff --git a/glamor/glamor_copy.c b/glamor/glamor_copy.c
index 3501a0d..5b5f0e6 100644
--- a/glamor/glamor_copy.c
+++ b/glamor/glamor_copy.c
@@ -220,11 +220,37 @@ glamor_copy_cpu_fbo(DrawablePtr src,
 
 glamor_get_drawable_deltas(dst, dst_pixmap, _xoff, _yoff);
 
-fbGetDrawable(src, src_bits, src_stride, src_bpp, src_xoff, src_yoff);
+if (bitplane) {
+PixmapPtr src_pix = fbCreatePixmap(screen, dst_pixmap->drawable.width,
+   dst_pixmap->drawable.height,
+   dst->depth, 0);
+
+if (!src_pix) {
+glamor_finish_access(src);
+goto bail;
+}
 
-glamor_upload_boxes(dst_pixmap, box, nbox, src_xoff + dx, src_yoff + dy,
-dst_xoff, dst_yoff,
-(uint8_t *) src_bits, src_stride * sizeof (FbBits));
+fbGetDrawable(_pix->drawable, src_bits, src_stride, src_bpp, 
src_xoff,
+  src_yoff);
+
+if (src->bitsPerPixel > 1)
+fbCopyNto1(src, _pix->drawable, gc, box, nbox,
+   dst_xoff + dx, dst_yoff + dy, reverse, upsidedown,
+   bitplane, closure);
+else
+fbCopy1toN(src, _pix->drawable, gc, box, nbox,
+   dst_xoff + dx, dst_yoff + dy, reverse, upsidedown,
+   bitplane, closure);
+
+glamor_upload_boxes(dst_pixmap, box, nbox, 0, 0, 0, 0,
+(uint8_t *) src_bits, src_stride * sizeof(FbBits));
+fbDestroyPixmap(src_pix);
+} else {
+fbGetDrawable(src, src_bits, src_stride, src_bpp, src_xoff, src_yoff);
+glamor_upload_boxes(dst_pixmap, box, nbox, src_xoff + dx, src_yoff + 
dy,
+dst_xoff, dst_yoff,
+(uint8_t *) src_bits, src_stride * sizeof 
(FbBits));
+}
 glamor_finish_access(src);
 
 return TRUE;
@@ -616,9 +642,9 @@ glamor_copy_gl(DrawablePtr src,
 return glamor_copy_fbo_fbo_draw(src, dst, gc, box, nbox, dx, 
dy,
 reverse, upsidedown, bitplane, 
closure);
 }
-if (bitplane == 0)
-return glamor_copy_cpu_fbo(src, dst, gc, box, nbox, dx, dy,
-   reverse, upsidedown, bitplane, closure);
+
+return glamor_copy_cpu_fbo(src, dst, gc, box, nbox, dx, dy,
+   reverse, upsidedown, bitplane, closure);
 } else if (GLAMOR_PIXMAP_PRIV_HAS_FBO(src_priv) &&
dst_priv->type != GLAMOR_DRM_ONLY &&
bitplane == 0) {
-- 
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 xserver] Fix Xorg -configure not working anymore

[Hans de Goede]

Hi,

On 17-08-16 16:58, Adam Jackson wrote:

On Wed, 2016-08-17 at 10:01 +0900, Michel Dänzer wrote:


At least longer term, maybe we should just remove Xorg -configure? Does
it serve any useful purpose anymore? I know it causes quite a lot of
trouble for users and those trying to help them...


I really want to say yes, ...


As interesting as this discussion is, I would still like to see
us move forward with a fix for now. Even if it is just so that
we can cherry-pick it into 1.18.x and then drop the entire
autoconfig thing.

So does anyone have a better solution then my patch ready
(or is anyone willing to invest the time soon-ish) ?

Regards,

Hans
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] modesetting: Fix msSharePixmapBacking returning a non-linear bo

[Hans de Goede]

Hi,

On 17-08-16 16:44, Keith Packard wrote:

Hans de Goede  writes:


Some code paths end up in msSharePixmapBacking with a pixmap
which does not have its usage_hint set to sharable.

This causes glamor_fd_from_pixmap() to create a non-linear bo,
which is wrong for a shared pixmap.

This commits sets the pixmap usage hint to shared before
calling glamor_fd_from_pixmap(), fixing this.

Specifically this fixes mis-rendering when running the mode
setting driver on the master gpu in a dual-gpu setup and
running an opengl app with DRI_PRIME=1.

One could argue that this is a problem of the caller, but the
usage_hint is as the name implies just a hint. I've tried
tracing the DRI_PRIME case from above, but the pixmap
ends up coming from a drawable passed into the server by
a client.

Signed-off-by: Hans de Goede 
---
 hw/xfree86/drivers/modesetting/driver.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/hw/xfree86/drivers/modesetting/driver.c 
b/hw/xfree86/drivers/modesetting/driver.c
index 5ebb394..023d491 100644
--- a/hw/xfree86/drivers/modesetting/driver.c
+++ b/hw/xfree86/drivers/modesetting/driver.c
@@ -1382,7 +1382,12 @@ msSharePixmapBacking(PixmapPtr ppix, ScreenPtr screen, 
void **handle)
 int ret;
 CARD16 stride;
 CARD32 size;
+unsigned orig_usage_hint = ppix->usage_hint;
+
+/* Ensure that glamor_fd_from_pixmap creates a sharable (linear) bo */
+ppix->usage_hint = CREATE_PIXMAP_USAGE_SHARED;
 ret = glamor_fd_from_pixmap(ppix->drawable.pScreen, ppix, , );
+ppix->usage_hint = orig_usage_hint;


That just can't be the right way to fix this...


That is what I thought, see the commit message. Trying to come up
with a better fix did not get me anywhere.

An alternative solution would be to add a flag argument to
glamor_fd_from_pixmap, so that callers like this one which
know for sure they want a linear bo can specify that through
a flag, and other callers can just pass in:

ppix->usage_hint == CREATE_PIXMAP_USAGE_SHARED

For that flag, the reason I did not do that is because
glamor_fd_from_pixmap is part of the xserver ABI AFAICT.

> Not that I have a better idea at this point.

Is that a convoluted way of acking the patch ?   :)

Regards,

Hans


___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

[Michel Dänzer]

On 17/08/16 07:29 PM, Qiang Yu wrote:
> Hi guys,
> 
> This patch set is for adding DRI2 page flip support to modesetting
> driver. I mainly take reference of amdgpu DDX and reuse present
> page flip code in the modesetting driver.
> 
> Regards,
> Qiang
> 
> Qiang Yu (6):
>   modesetting: make ms_do_pageflip generic for share with DRI2
>   modesetting: move ms_do_pageflip to pageflip.c
>   modesetting: add DRI2 page flip support
>   modesetting: exclude DRI2 and Present page flip
>   modesetting: merge common page flip code for present and dri2
>   modesetting: remove redundent pixmap destroy

Patches 2, 4 & 5 are

Reviewed-by: Michel Dänzer 


Patch 6 was already submitted by Hans de Goede:

https://patchwork.freedesktop.org/patch/105351/


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 3/6] modesetting: add DRI2 page flip support

[Michel Dänzer]

On 17/08/16 07:29 PM, Qiang Yu wrote:
> 
> +static void
> +ms_dri2_flip_free(struct ms_crtc_pageflip *flip)
> +{
> +struct ms_flipdata *flipdata = flip->flipdata;
> +
> +free(flip);
> +if (--flipdata->flip_count > 0)
> +return;
> +free(flipdata);
> +}
> +
> +static void
> +ms_dri2_flip_abort(void *data)
> +{
> +struct ms_crtc_pageflip *flip = data;
> +struct ms_flipdata *flipdata = flip->flipdata;
> +
> +if (flipdata->flip_count == 1)
> +free(flipdata->event);

This should be moved to ms_dri2_flip_free, or flipdata->event will be
leaked for successful flips?


> +static Bool
> +can_exchange(ScrnInfoPtr scrn, DrawablePtr draw,
> +  DRI2BufferPtr front, DRI2BufferPtr back)
> +{

[...]

> +if (!update_front(draw, front))
> +return FALSE;

I know you just copied this from -ati/amdgpu, but: I don't think
can_exchange should call update_front, or the front buffer may be
updated even though the flip later fails.


> +/* Post damage on the front buffer so that listeners, such
> + * as DisplayLink know take a copy and shove it over the USB.
> + * also for sw cursors.
> + */

SW cursors cannot work correctly with page flipping. For that reason,
xf86-video-ati/amdgpu disable page flipping while there's an SW cursor.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 1/6] modesetting: make ms_do_pageflip generic for share with DRI2

[Michel Dänzer]

On 17/08/16 07:29 PM, Qiang Yu wrote:
> 
> -DebugPresent(("\t\tms:fq %lld c %d -> %d seq %llu\n",
> -  (long long) flipdata->event->event_id,
> -  flipdata->flip_count, flipdata->flip_count + 1,
> -  (long long) seq));

Might be nice to move this debugging output to ms_present_flip instead
of removing it completely.


With that fixed, this patch is

Reviewed-by: Michel Dänzer 


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: Michel Dänzer is invited to help maintain master X server branch

[Michel Dänzer]

On 16/08/16 01:51 AM, Keith Packard wrote:
> 
> When Adam left for a week's vacation, Michel noted that the master X
> server branch was not keeping up -- a couple of important patches were
> not getting committed in a reasonable amount of time. I haven't had
> enough bandwidth myself for several months and it wasn't obvious until
> Adam's departure. I talked with Adam this morning and he agreed that it
> would be really helpful if Michel would be willing to help maintain the
> master branch.

Thanks for the invitation. :)


> The goal of collaborative maintenance of the master branch is to get
> patches merged in a timely fashion while still having a strong process
> of review and sequencing of major changes. We want master to always be
> usable on most people's machines so that more people are testing the
> code, and having a bit of central process has proven a useful tool for
> that.
> 
> Of course, the problem with having a group of people sharing
> responsibility is that sometimes no-one ends up picking up a piece of
> work. If you've got a patch which is ready to merge and has been left on
> the side of the road, please feel free to poke one of us and cc'
> xorg-devel. People on this list have volunteered to spend their time
> helping manage the process of merging patches, so please be kind to them.
> 
>  * Eric Anholt  - glamor

Many glamor changes have been pushed by Adam or Keith lately. Is that
intentional, i.e. is it okay for others to push glamor changes which are
ready, or should we always get in touch with Eric first?


>  * Alan Coopersmith - security
>  * Peter Hutterer   - input
>  * Adam Jackson - xfree86 backend
>  * Keith Packard- misc
>  * Michel Dänzer- mode setting stuff? glamor? (please let us know!)

My recent work has been mostly in present and glamor. Before that, I was
also working on DRI2, EXA and other miscellaneous stuff, mostly in the
xfree86 backend. I guess those will be the areas I'll feel most
confident in pushing changes for.


> Thanks much to everyone for keeping X running, and especially to Michel
> for being willing to help out with some of the tedium.

Thank me once I've actually proven useful. :)


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

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel