Re: [PATCH app/xdpyinfo v2] Use XRANDR 1.2 extension for reporting dimensions and resolution per output

[Adam Jackson]

On Wed, 2018-04-04 at 21:30 +0200, Giuseppe Bilotta wrote:
> On Wed, Apr 4, 2018 at 5:12 PM, Adam Jackson  wrote:
> > On Tue, 2018-04-03 at 22:15 +0200, Pali Rohár wrote:
> > > Gently reminder of this patch. Can you comment/review it?
> > 
> > Nack. The whole point of (that part of) xdpyinfo is to show you what
> > was sent in the initial connection block. xrandr already exists, and
> > code exists that depends on xdpyinfo's output.
> 
> Would it make sense to add the output from RANDR via an explicit `-ext
> RANDR` query similar to what is already done for XINERAMA,
> XInputExtension, etc?

If it's going to be in xdpyinfo at all - and I kinda don't think it
needs to be, given xrandr already exists, but I don't insist on that -
then yes, it should be behind -ext RANDR.

- 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 app/xdpyinfo v2] Use XRANDR 1.2 extension for reporting dimensions and resolution per output

[Giuseppe Bilotta]

On Wed, Apr 4, 2018 at 5:12 PM, Adam Jackson  wrote:
> On Tue, 2018-04-03 at 22:15 +0200, Pali Rohár wrote:
>> Gently reminder of this patch. Can you comment/review it?
>
> Nack. The whole point of (that part of) xdpyinfo is to show you what
> was sent in the initial connection block. xrandr already exists, and
> code exists that depends on xdpyinfo's output.

Would it make sense to add the output from RANDR via an explicit `-ext
RANDR` query similar to what is already done for XINERAMA,
XInputExtension, etc?

-- 
Giuseppe "Oblomov" Bilotta
___
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 14/15] dri3: s/intersect/window/ to match the names used in caller

[Adam Jackson]

On Mon, 2018-04-02 at 16:41 +0100, Emil Velikov wrote:

> @@ -217,44 +217,44 @@ dri3_get_supported_modifiers(ScreenPtr screen, 
> DrawablePtr drawable,
>  }
>  
>  /* We're allocating slightly more memory than necessary but it reduces
> - * the complexity of finding the intersection set.
> + * the complexity of finding the windowion set.
>   */

Search-and-replace-o for "windowion".

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

Re: [PATCH xserver 05/15] dri3: annotate {s, }proc_dri3_vector as static const

[Adam Jackson]

On Mon, 2018-04-02 at 16:41 +0100, Emil Velikov wrote:

> diff --git a/dri3/dri3_request.c b/dri3/dri3_request.c
> index fc258711b..7ced6747c 100644
> --- a/dri3/dri3_request.c
> +++ b/dri3/dri3_request.c
> @@ -32,6 +32,9 @@
>  #include 
>  #include 
>  
> +/* Should be provided by the guts of X */
> +typedef int (* DispatchProc) (ClientPtr client);
> +

I mean, maybe. But if you're doing that go ahead and do it in a header
and fix up the other dispatch vectors, otherwise this is just being
different for no reason.

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

Re: [PATCH xserver 2/3] meson: Distribute more SDK headers

[Adam Jackson]

On Tue, 2018-04-03 at 12:16 +0200, Thierry Reding wrote:

> Yeah, looking more closely at the automake file it has XORG guards
> around all of the SDK headers. There's also some inconsistencies there
> with regards to which files are included depending on other extensions
> being enabled. For example, xvdix.h and xvmcext.h are unconditionally
> installed, even if Xv is not enabled. That doesn't make much sense to
> me, but maybe it is something that we should keep around for the Meson
> build? Or do we want to "clean" this up while at it and only distribute
> the SDK headers that really make sense?

The SDK shouldn't install anything conditionally if it's being
installed at all, it's a description of every interface the server
might have (as well as xorg-server.h to say exactly which sets you can
expect on the runtime you're building against). Granted it _does_
install many things conditionally, but I think that's an error.

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

Re: [PATCH xserver 15/15] dri3: correctly handle failed get_supported_modifiers requests

[Adam Jackson]

On Mon, 2018-04-02 at 16:41 +0100, Emil Velikov wrote:
> From: Emil Velikov 
> 
> Currently depending on the code path hit, the helper will set some of
> the output values and not others.
> 
> It could also leak memory ;-)
> 
> At the same time the caller was:
>  - working around the broken behaviour - by initialising the variables
>  - outright ignoring if the helper fails
> 
> Fix all that by consistently setting the output variables and returning
> a protocol error if we fail to get the data required.
> 
> Bonus points: current code does not attribute if we cannot get the
> modifiers info for certain format. A smaller format array is available,
> yet the original length is stored.
> 
> Fixes: cef12efc15c ("glamor: Implement GetSupportedModifiers")
> Cc: Louis-Francis Ratté-Boulianne 
> Cc: Daniel Stone 
> Signed-off-by: Emil Velikov 
> ---
> We could still return "success" to the user - I've opted for this
> solution since we already bail on dixLookupWindow failure.

This, combined with 10/15, means you will now throw BadImplementation
when we used to succeed and simply report no modifiers. I can't think
of a good reason to make that an error, it just makes the client's life
harder if it has to distinguish error from reply.

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

Re: [PATCH xserver] modesetting: Fix page flipping harder under DRI 3.2.

[Adam Jackson]

On Wed, 2018-04-04 at 17:20 +0100, Daniel Stone wrote:
> On 4 April 2018 at 02:49, Mario Kleiner  wrote:
> > Non-atomic kms drivers like radeon-kms (or nouveau-kms with
> > default setting of "atomic ioctl disabled") don't export
> > any formats, so num_formats == 0.
> > 
> > Some atomic drivers (nouveau-kms with boot param nouveau.atomic=1,
> > or intel-kms on, e.g., Linux 4.13) expose num_formats == 0, or
> > don't expose any modifiers, so num_modifiers == 0.
> > 
> > Let the drmmode_is_format_supported() check pass in these cases
> > to allow page flipping, as it works just fine.
> > 
> > Tested on NV-96 for nouveau, HD-5770 for radeon, Intel Ivybridge
> > with Linux 4.13 and drm-next to fix page flipping.
> 
> Reviewed-by: Daniel Stone 

Merged, thanks:

remote: I: patch #214734 updated using rev 
ce2a4313dd31084f7766af59b8477cabe029bf44.
remote: I: 1 patch(es) updated to state Accepted.
To ssh://git.freedesktop.org/git/xorg/xserver
   44e7098367..ce2a4313dd  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 xserver 1/2] modesetting: Use atomic modesetting to set DPMS mode

[Adam Jackson]

On Wed, 2018-04-04 at 17:21 +0100, Daniel Stone wrote:
> Hi Louis-Francis,
> 
> On 4 April 2018 at 05:01, Louis-Francis Ratté-Boulianne
>  wrote:
> > CRTCs and outputs needs to be enabled/disabled when the current
> > DPMS mode is changed. We also try to do it in an atomic commit
> > when possible.
> 
> It would be really nice to separate the code motion from the changes,
> into more granular commits than this. I found this a bit hard to
> review without referring back to both old and new versions constantly
> as I went. Regardless, this series is:
> Tested-by: Daniel Stone 

Merged, thanks:

remote: E: failed to find patch for rev 
bc4d278132956ec3c43695f1bd34083ef5fe7f22.
remote: I: patch #214753 updated using rev 
44e7098367b87c79470d6760753e42014be7ca01.
remote: I: 1 patch(es) updated to state Accepted.
To ssh://git.freedesktop.org/git/xorg/xserver
   23c67987a3..44e7098367  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 xserver 0/7] Fix modifier server / non-mod client

[Adam Jackson]

On Wed, 2018-04-04 at 16:16 +0100, Daniel Stone wrote:
> Hi,
> This series fixes running a modifier-aware server with a
> non-modifier-aware DRI3 compositing manager. We'd thought of and dealt
> with the case where we had legacy/non-modifier-aware clients, but
> slipped up with non-modifier-aware compositing managers.
> 
> Glamor would allocate pixmaps with the full set of modifiers available
> to it from GBM/EGL, then try to export them to a non-modifier-aware
> client. This may fail outright (e.g. when the modifier adds a plane), or
> result in garbage (with an exotic modifier selection the client can't
> infer via magic back channels).
> 
> When we get a request to export a buffer to a non-modifier-aware client
> in Glamor, forcibly reallocate it with no explicit modifiers, so we get
> something that can be exported to legacy clients.

Took me a second to figure out how 5/7 was getting the reallocation to
happen, but this all makes sense. It also fixes Xorg on my laptop, so
thank you very much! Merged:

remote: E: failed to find patch for rev 
75bba3aedcb04132a4fe2806680cd38a8f2bd206.
remote: I: patch #214874 updated using rev 
0e9504e10c4363e24a83f1a82e6a4b9f5fd8f846.
remote: I: patch #214875 updated using rev 
1b9fa3b64ca420eb54b5e5f28074c326e1fbe825.
remote: I: patch #214873 updated using rev 
aab5c46ccbe769830cae383330fd62c074a0d2f7.
remote: I: patch #214879 updated using rev 
86b2d8740a330deafe8a9bbf0402705a43efbb42.
remote: I: patch #214877 updated using rev 
9c407f0a1b40128fc65b19b6a499f1d4dae6f702.
remote: I: patch #214878 updated using rev 
23c67987a337beb91292f8e318d566941453baa3.
remote: I: 6 patch(es) updated to state Accepted.
To ssh://git.freedesktop.org/git/xorg/xserver
   574069c291..23c67987a3  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 xserver] modesetting: Fix page flipping under DRI 3.2.

[Daniel Stone]

Hi Mario,

On 4 April 2018 at 17:40, Mario Kleiner  wrote:
> On Wed, Apr 4, 2018 at 6:19 PM, Daniel Stone  wrote:
>> Ugh. I've applied your pageflip patch, lfrb's two-patch atomic fix
>> series and my fix-old-clients series, which for me fixes KDE running
>> with both old and new Mesa. That's just running a Plasma 5 desktop
>> with 'startkde', either single or dual head. Do things work for you if
>> you have all those applied?
>
> I have all applied, but this new series "Fix modifier server / non-mod client"
> of you: https://patchwork.freedesktop.org/series/41142/
>
> If that's the right one, i'll test it later today.

Correct. If you could please try that series, that would be great. I
did see KDE failing before that series, but it seems much happier now.

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

Re: [PATCH xserver] modesetting: Fix page flipping under DRI 3.2.

[Mario Kleiner]

On Wed, Apr 4, 2018 at 6:19 PM, Daniel Stone  wrote:
> Hi Mario,
>
> On 4 April 2018 at 06:22, Mario Kleiner  wrote:
>> Ok, so it's probably a mesa bug in the egl dri3 backend caused by the
>> new DRI3.1 multibuffers support.
>>
>> If on current mesa master, in egl_dri2.c:dri2_setup_extensions(), i
>> force dri2_dpy->multibuffers_available = false; to disable
>> multibuffers, then EGL based compositing under DRI3 works fine again.
>> Otherwise i get failure on all gpu's and drivers, as tested with
>> ati-ddx,nouveau-ddx,modesetting-ddx under glamor or exa.
>>
>> This tested on current xserver master and Linux drm-next.
>
> Ugh. I've applied your pageflip patch, lfrb's two-patch atomic fix
> series and my fix-old-clients series, which for me fixes KDE running
> with both old and new Mesa. That's just running a Plasma 5 desktop
> with 'startkde', either single or dual head. Do things work for you if
> you have all those applied?
>
> Cheers,
> Daniel

I have all applied, but this new series "Fix modifier server / non-mod client"
of you: https://patchwork.freedesktop.org/series/41142/

If that's the right one, i'll test it later today.

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

Re: [PATCH xserver 1/2] modesetting: Use atomic modesetting to set DPMS mode

[Daniel Stone]

Hi Louis-Francis,

On 4 April 2018 at 05:01, Louis-Francis Ratté-Boulianne
 wrote:
> CRTCs and outputs needs to be enabled/disabled when the current
> DPMS mode is changed. We also try to do it in an atomic commit
> when possible.

It would be really nice to separate the code motion from the changes,
into more granular commits than this. I found this a bit hard to
review without referring back to both old and new versions constantly
as I went. Regardless, this series is:
Tested-by: Daniel Stone 

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

Re: [PATCH xserver] modesetting: Fix page flipping harder under DRI 3.2.

[Daniel Stone]

On 4 April 2018 at 02:49, Mario Kleiner  wrote:
> Non-atomic kms drivers like radeon-kms (or nouveau-kms with
> default setting of "atomic ioctl disabled") don't export
> any formats, so num_formats == 0.
>
> Some atomic drivers (nouveau-kms with boot param nouveau.atomic=1,
> or intel-kms on, e.g., Linux 4.13) expose num_formats == 0, or
> don't expose any modifiers, so num_modifiers == 0.
>
> Let the drmmode_is_format_supported() check pass in these cases
> to allow page flipping, as it works just fine.
>
> Tested on NV-96 for nouveau, HD-5770 for radeon, Intel Ivybridge
> with Linux 4.13 and drm-next to fix page flipping.

Reviewed-by: Daniel Stone 

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

Re: [PATCH xserver] modesetting: Fix page flipping under DRI 3.2.

[Daniel Stone]

Hi Mario,

On 4 April 2018 at 06:22, Mario Kleiner  wrote:
> Ok, so it's probably a mesa bug in the egl dri3 backend caused by the
> new DRI3.1 multibuffers support.
>
> If on current mesa master, in egl_dri2.c:dri2_setup_extensions(), i
> force dri2_dpy->multibuffers_available = false; to disable
> multibuffers, then EGL based compositing under DRI3 works fine again.
> Otherwise i get failure on all gpu's and drivers, as tested with
> ati-ddx,nouveau-ddx,modesetting-ddx under glamor or exa.
>
> This tested on current xserver master and Linux drm-next.

Ugh. I've applied your pageflip patch, lfrb's two-patch atomic fix
series and my fix-old-clients series, which for me fixes KDE running
with both old and new Mesa. That's just running a Plasma 5 desktop
with 'startkde', either single or dual head. Do things work for you if
you have all those applied?

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

Re: Bug 105851 Xserver 1.20 RC2+ issues with Kwin + Present 1.2

[Daniel Stone]

Hi Mike,

On 4 April 2018 at 09:12, Mike Lothian  wrote:
> Kwin doesn't seem to start with the following commit:
>
> commit abb9b58d1af9a0286162e52ef9db390d0c950fc1
> Author: Thierry Reding 
> Date:   Fri Mar 16 14:24:21 2018 +0100
>
> present: Advertise protocol version 1.2
>
> [...]
>
> Though I think the issue may lie in commit:
>
> commit 6a5d51e0823b43280e3646b7a0c919a3b76146ea
> Author: Emil Velikov 
> Date:   Mon Mar 19 16:04:43 2018 +
>
> present: cap the version returned to the client
>
> [...]
>
> I realise that Kwin will probably need to be modified to work with
> Present 1.2, but I don't think we should be seeing breakage unti that
> happens

I think that's just exposing breakage somewhere else in the chain.
With all these series applied, I've been able to get KDE to start with
'startkde' (running Plasma), both with new and old Mesa:
https://patchwork.freedesktop.org/series/41104/
https://patchwork.freedesktop.org/series/41106/
https://patchwork.freedesktop.org/series/41142/

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

Re: Bug 105851 Xserver 1.20 RC2+ issues with Kwin + Present 1.2

[Mike Lothian]

Hi Emil

I'm running the latest everything from git and have that commit

Regards

Mike

On Wed, 4 Apr 2018 at 12:50 Emil Velikov  wrote:

> Hi Mike,
>
> On 4 April 2018 at 09:12, Mike Lothian  wrote:
> > Hi
> >
> > Kwin doesn't seem to start with the following commit:
> >
> > commit abb9b58d1af9a0286162e52ef9db390d0c950fc1
> > Author: Thierry Reding 
> > Date:   Fri Mar 16 14:24:21 2018 +0100
> >
> > present: Advertise protocol version 1.2
> >
> > Everything is implemented to support protocol version 1.2. Make it
> > official.
> >
> > Reviewed-by: Daniel Stone 
> > Signed-off-by: Thierry Reding 
> > Reviewed-by: Emil Velikov 
> >
> > Though I think the issue may lie in commit:
> >
> > commit 6a5d51e0823b43280e3646b7a0c919a3b76146ea
> > Author: Emil Velikov 
> > Date:   Mon Mar 19 16:04:43 2018 +
> >
> > present: cap the version returned to the client
> >
> > As per the protocol, the server should not return version greater
> than
> > the one supported by the client.
> >
> > Add a spec quote and tweak the numbers accordingly.
> >
> > I realise that Kwin will probably need to be modified to work with
> > Present 1.2, but I don't think we should be seeing breakage unti that
> > happens
> >
> AFAICT Kwin does not directly use the present extension, does it?
>
> Can you ensure that you have mesa commit
> 696762eef57e83b4027acbdf0a6e74d1f75083b0.
> It addresses some hunky stuff with the version handling.
>
> -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

[PATCH xserver 5/7] glamor: Reallocate pixmap storage without modifiers if necessary

[Daniel Stone]

If we need a pixmap's storage to be exported to a context in which we
aren't aware of modifiers, reallocate the buffer again without
modifiers.

This makes it possible to run a compositing manager on an old GLX/EGL
stack on top of an X server which allocates internal buffer storage
using exotic modifiers from modifier-aware GBM/EGL/KMS.

Signed-off-by: Daniel Stone 
Reported-by: Adam Jackson 
---
 glamor/glamor_egl.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/glamor/glamor_egl.c b/glamor/glamor_egl.c
index dd6a9a2df..a1e0bc3c9 100644
--- a/glamor/glamor_egl.c
+++ b/glamor/glamor_egl.c
@@ -250,7 +250,7 @@ glamor_get_name_from_bo(int gbm_fd, struct gbm_bo *bo, int 
*name)
 }
 
 static Bool
-glamor_make_pixmap_exportable(PixmapPtr pixmap)
+glamor_make_pixmap_exportable(PixmapPtr pixmap, Bool modifiers_ok)
 {
 ScreenPtr screen = pixmap->drawable.pScreen;
 ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
@@ -266,7 +266,8 @@ glamor_make_pixmap_exportable(PixmapPtr pixmap)
 PixmapPtr exported;
 GCPtr scratch_gc;
 
-if (pixmap_priv->image)
+if (pixmap_priv->image &&
+(modifiers_ok || !pixmap_priv->used_modifiers))
 return TRUE;
 
 if (pixmap->drawable.bitsPerPixel != 32) {
@@ -282,7 +283,7 @@ glamor_make_pixmap_exportable(PixmapPtr pixmap)
 format = GBM_FORMAT_ARGB;
 
 #ifdef GBM_BO_WITH_MODIFIERS
-if (glamor_egl->dmabuf_capable) {
+if (modifiers_ok && glamor_egl->dmabuf_capable) {
 uint32_t num_modifiers;
 uint64_t *modifiers = NULL;
 
@@ -370,7 +371,7 @@ glamor_egl_fds_from_pixmap(ScreenPtr screen, PixmapPtr 
pixmap, int *fds,
 int i;
 #endif
 
-if (!glamor_make_pixmap_exportable(pixmap))
+if (!glamor_make_pixmap_exportable(pixmap, TRUE))
 return 0;
 
 bo = glamor_gbm_bo_from_pixmap(screen, pixmap);
@@ -411,7 +412,7 @@ glamor_egl_fd_name_from_pixmap(ScreenPtr screen,
 
 glamor_egl = glamor_egl_get_screen_private(xf86ScreenToScrn(screen));
 
-if (!glamor_make_pixmap_exportable(pixmap))
+if (!glamor_make_pixmap_exportable(pixmap, FALSE))
 goto failure;
 
 bo = glamor_gbm_bo_from_pixmap(screen, pixmap);
-- 
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 6/7] glamor: Fall back to non-modifier allocations

[Daniel Stone]

If we try to allocate with particular modifiers but it fails, try to
fall back to non-modifier allocations.

Signed-off-by: Daniel Stone 
Reported-by: Adam Jackson 
---
 glamor/glamor_egl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/glamor/glamor_egl.c b/glamor/glamor_egl.c
index a1e0bc3c9..2dffbb8dc 100644
--- a/glamor/glamor_egl.c
+++ b/glamor/glamor_egl.c
@@ -261,7 +261,7 @@ glamor_make_pixmap_exportable(PixmapPtr pixmap, Bool 
modifiers_ok)
 unsigned width = pixmap->drawable.width;
 unsigned height = pixmap->drawable.height;
 uint32_t format;
-struct gbm_bo *bo;
+struct gbm_bo *bo = NULL;
 Bool used_modifiers = FALSE;
 PixmapPtr exported;
 GCPtr scratch_gc;
@@ -295,8 +295,9 @@ glamor_make_pixmap_exportable(PixmapPtr pixmap, Bool 
modifiers_ok)
 used_modifiers = TRUE;
 free(modifiers);
 }
-else
 #endif
+
+if (!bo)
 {
 bo = gbm_bo_create(glamor_egl->gbm, width, height, format,
 #ifdef GLAMOR_HAS_GBM_LINEAR
-- 
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 7/7] glamor: Add fd_from_pixmap hook

[Daniel Stone]

Add a fd_from_pixmap (singular) hook to go with fds_from_pixmap, which
will ensure that the pixmap is allocated without modifiers and is thus
exportable to non-modifier-aware clients.

This makes it possible to run a compositing manager on an old GLX/EGL
stack on top of an X server which allocates internal buffer storage
using exotic modifiers from modifier-aware GBM/EGL/KMS.

Signed-off-by: Daniel Stone 
Reported-by: Adam Jackson 
---
 glamor/glamor_egl.c | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/glamor/glamor_egl.c b/glamor/glamor_egl.c
index 2dffbb8dc..5a47dbd91 100644
--- a/glamor/glamor_egl.c
+++ b/glamor/glamor_egl.c
@@ -402,6 +402,32 @@ glamor_egl_fds_from_pixmap(ScreenPtr screen, PixmapPtr 
pixmap, int *fds,
 #endif
 }
 
+static int
+glamor_egl_fd_from_pixmap(ScreenPtr screen, PixmapPtr pixmap,
+  CARD16 *stride, CARD32 *size)
+{
+#ifdef GLAMOR_HAS_GBM
+struct gbm_bo *bo;
+int fd;
+
+if (!glamor_make_pixmap_exportable(pixmap, FALSE))
+return -1;
+
+bo = glamor_gbm_bo_from_pixmap(screen, pixmap);
+if (!bo)
+return -1;
+
+fd = gbm_bo_get_fd(bo);
+*stride = gbm_bo_get_stride(bo);
+*size = *stride * gbm_bo_get_height(bo);
+gbm_bo_destroy(bo);
+
+return fd;
+#else
+return -1;
+#endif
+}
+
 int
 glamor_egl_fd_name_from_pixmap(ScreenPtr screen,
PixmapPtr pixmap,
@@ -775,6 +801,7 @@ static dri3_screen_info_rec glamor_dri3_info = {
 .version = 2,
 .open_client = glamor_dri3_open_client,
 .pixmap_from_fds = glamor_pixmap_from_fds,
+.fd_from_pixmap = glamor_egl_fd_from_pixmap,
 .fds_from_pixmap = glamor_egl_fds_from_pixmap,
 .get_formats = glamor_get_formats,
 .get_modifiers = glamor_get_modifiers,
-- 
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 4/7] glamor: Push make_exportable into callers

[Daniel Stone]

Rather than calling make_exportable from the get_bo entrypoint, make
sure that someone has already explicitly requested the pixmap be
exportable.

This is technically an ABI break in that it changes observable
behaviour, but no driver other than modesetting has ever used get_bo.

Signed-off-by: Daniel Stone 
Reported-by: Adam Jackson 
---
 glamor/glamor_egl.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/glamor/glamor_egl.c b/glamor/glamor_egl.c
index 0e771b6d2..dd6a9a2df 100644
--- a/glamor/glamor_egl.c
+++ b/glamor/glamor_egl.c
@@ -351,7 +351,7 @@ glamor_gbm_bo_from_pixmap(ScreenPtr screen, PixmapPtr 
pixmap)
 struct glamor_pixmap_private *pixmap_priv =
 glamor_get_pixmap_private(pixmap);
 
-if (!glamor_make_pixmap_exportable(pixmap))
+if (!pixmap_priv->image)
 return NULL;
 
 return gbm_bo_import(glamor_egl->gbm, GBM_BO_IMPORT_EGL_IMAGE,
@@ -411,6 +411,9 @@ glamor_egl_fd_name_from_pixmap(ScreenPtr screen,
 
 glamor_egl = glamor_egl_get_screen_private(xf86ScreenToScrn(screen));
 
+if (!glamor_make_pixmap_exportable(pixmap))
+goto failure;
+
 bo = glamor_gbm_bo_from_pixmap(screen, pixmap);
 if (!bo)
 goto failure;
-- 
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 3/7] glamor: Track if BO allocation used modifiers

[Daniel Stone]

Keep track of whether or not we fed modifiers into GBM when we allocated
a BO. We'll use this later inside Glamor, to reallocate buffer storage
if we allocate buffer storage using modifiers, and a non-modifier-aware
client requests an export of that pixmap.

This makes it possible to run a compositing manager on an old GLX/EGL
stack on top of an X server which allocates internal buffer storage
using exotic modifiers from modifier-aware GBM/EGL/KMS.

Signed-off-by: Daniel Stone 
Reported-by: Adam Jackson 
---
 glamor/glamor.h   |  3 +-
 glamor/glamor_egl.c   | 29 +--
 glamor/glamor_priv.h  |  1 +
 .../drivers/modesetting/drmmode_display.c |  3 +-
 4 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/glamor/glamor.h b/glamor/glamor.h
index 7b5676226..b0b23d3a3 100644
--- a/glamor/glamor.h
+++ b/glamor/glamor.h
@@ -391,7 +391,8 @@ extern _X_EXPORT Bool 
glamor_egl_create_textured_pixmap(PixmapPtr pixmap,
  */
 extern _X_EXPORT Bool
  glamor_egl_create_textured_pixmap_from_gbm_bo(PixmapPtr pixmap,
-   struct gbm_bo *bo);
+   struct gbm_bo *bo,
+   Bool used_modifiers);
 
 #endif
 
diff --git a/glamor/glamor_egl.c b/glamor/glamor_egl.c
index f82fa519b..0e771b6d2 100644
--- a/glamor/glamor_egl.c
+++ b/glamor/glamor_egl.c
@@ -150,7 +150,8 @@ glamor_egl_create_textured_screen(ScreenPtr screen, int 
handle, int stride)
 }
 
 static void
-glamor_egl_set_pixmap_image(PixmapPtr pixmap, EGLImageKHR image)
+glamor_egl_set_pixmap_image(PixmapPtr pixmap, EGLImageKHR image,
+Bool used_modifiers)
 {
 struct glamor_pixmap_private *pixmap_priv =
 glamor_get_pixmap_private(pixmap);
@@ -165,6 +166,7 @@ glamor_egl_set_pixmap_image(PixmapPtr pixmap, EGLImageKHR 
image)
 eglDestroyImageKHR(glamor_egl->display, old);
 }
 pixmap_priv->image = image;
+pixmap_priv->used_modifiers = used_modifiers;
 }
 
 Bool
@@ -204,7 +206,8 @@ glamor_egl_create_textured_pixmap(PixmapPtr pixmap, int 
handle, int stride)
 
 Bool
 glamor_egl_create_textured_pixmap_from_gbm_bo(PixmapPtr pixmap,
-  struct gbm_bo *bo)
+  struct gbm_bo *bo,
+  Bool used_modifiers)
 {
 ScreenPtr screen = pixmap->drawable.pScreen;
 ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
@@ -229,7 +232,7 @@ glamor_egl_create_textured_pixmap_from_gbm_bo(PixmapPtr 
pixmap,
 glamor_create_texture_from_image(screen, image, );
 glamor_set_pixmap_type(pixmap, GLAMOR_TEXTURE_DRM);
 glamor_set_pixmap_texture(pixmap, texture);
-glamor_egl_set_pixmap_image(pixmap, image);
+glamor_egl_set_pixmap_image(pixmap, image, used_modifiers);
 ret = TRUE;
 
  done:
@@ -259,6 +262,7 @@ glamor_make_pixmap_exportable(PixmapPtr pixmap)
 unsigned height = pixmap->drawable.height;
 uint32_t format;
 struct gbm_bo *bo;
+Bool used_modifiers = FALSE;
 PixmapPtr exported;
 GCPtr scratch_gc;
 
@@ -286,6 +290,8 @@ glamor_make_pixmap_exportable(PixmapPtr pixmap)
 
 bo = gbm_bo_create_with_modifiers(glamor_egl->gbm, width, height,
   format, modifiers, num_modifiers);
+if (bo)
+used_modifiers = TRUE;
 free(modifiers);
 }
 else
@@ -309,7 +315,8 @@ glamor_make_pixmap_exportable(PixmapPtr pixmap)
 exported = screen->CreatePixmap(screen, 0, 0, pixmap->drawable.depth, 0);
 screen->ModifyPixmapHeader(exported, width, height, 0, 0,
gbm_bo_get_stride(bo), NULL);
-if (!glamor_egl_create_textured_pixmap_from_gbm_bo(exported, bo)) {
+if (!glamor_egl_create_textured_pixmap_from_gbm_bo(exported, bo,
+   used_modifiers)) {
 xf86DrvMsg(scrn->scrnIndex, X_ERROR,
"Failed to make %dx%dx%dbpp pixmap from GBM bo\n",
width, height, pixmap->drawable.bitsPerPixel);
@@ -452,7 +459,7 @@ glamor_back_pixmap_from_fd(PixmapPtr pixmap,
 
 screen->ModifyPixmapHeader(pixmap, width, height, 0, 0, stride, NULL);
 
-ret = glamor_egl_create_textured_pixmap_from_gbm_bo(pixmap, bo);
+ret = glamor_egl_create_textured_pixmap_from_gbm_bo(pixmap, bo, FALSE);
 gbm_bo_destroy(bo);
 return ret;
 }
@@ -509,7 +516,7 @@ glamor_pixmap_from_fds(ScreenPtr screen,
 bo = gbm_bo_import(glamor_egl->gbm, GBM_BO_IMPORT_FD_MODIFIER, 
_data, 0);
 if (bo) {
 screen->ModifyPixmapHeader(pixmap, width, height, 0, 0, 
strides[0], NULL);
-ret = glamor_egl_create_textured_pixmap_from_gbm_bo(pixmap, bo);
+ret = glamor_egl_create_textured_pixmap_from_gbm_bo(pixmap, bo, 

[PATCH xserver 2/7] drmmode: Track if BO allocation used modifiers

[Daniel Stone]

Keep track of whether or not we fed modifiers into GBM when we allocated
a BO. We'll use this later inside Glamor, to reallocate buffer storage
if we allocate buffer storage using modifiers, and a non-modifier-aware
client requests an export of that pixmap.

This makes it possible to run a compositing manager on an old GLX/EGL
stack on top of an X server which allocates internal buffer storage
using exotic modifiers from modifier-aware GBM/EGL/KMS.

Signed-off-by: Daniel Stone 
Reported-by: Adam Jackson 
---
 hw/xfree86/drivers/modesetting/drmmode_display.c | 5 -
 hw/xfree86/drivers/modesetting/drmmode_display.h | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c 
b/hw/xfree86/drivers/modesetting/drmmode_display.c
index fc1aef2b6..aeb44263b 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.c
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
@@ -1022,13 +1022,16 @@ drmmode_create_bo(drmmode_ptr drmmode, drmmode_bo *bo,
format, modifiers,
num_modifiers);
 free(modifiers);
-if (bo->gbm)
+if (bo->gbm) {
+bo->used_modifiers = TRUE;
 return TRUE;
+}
 }
 #endif
 
 bo->gbm = gbm_bo_create(drmmode->gbm, width, height, format,
 GBM_BO_USE_RENDERING | GBM_BO_USE_SCANOUT);
+bo->used_modifiers = FALSE;
 return bo->gbm != NULL;
 }
 #endif
diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h 
b/hw/xfree86/drivers/modesetting/drmmode_display.h
index 4e1e013ff..74954b853 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.h
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.h
@@ -75,6 +75,7 @@ typedef struct {
 uint32_t height;
 struct dumb_bo *dumb;
 #ifdef GLAMOR_HAS_GBM
+Bool used_modifiers;
 struct gbm_bo *gbm;
 #endif
 } drmmode_bo;
-- 
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 0/7] Fix modifier server / non-mod client

[Daniel Stone]

Hi,
This series fixes running a modifier-aware server with a
non-modifier-aware DRI3 compositing manager. We'd thought of and dealt
with the case where we had legacy/non-modifier-aware clients, but
slipped up with non-modifier-aware compositing managers.

Glamor would allocate pixmaps with the full set of modifiers available
to it from GBM/EGL, then try to export them to a non-modifier-aware
client. This may fail outright (e.g. when the modifier adds a plane), or
result in garbage (with an exotic modifier selection the client can't
infer via magic back channels).

When we get a request to export a buffer to a non-modifier-aware client
in Glamor, forcibly reallocate it with no explicit modifiers, so we get
something that can be exported to legacy clients.

Cheers,
Daniel


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

[PATCH xserver 1/7] dri3: Use single-FD screen call for single-FD request

[Daniel Stone]

When importing client buffers into Pixmaps, we can use the fds_to_pixmap
hook for both single-FD and multi-FD client requests without any harm.

For the other direction of exporting Pixmap buffers to client FDs,
create a new helper which calls the old pixmap_to_fd hook if available.
This allows the implementation to ensure that the Pixmap storage is
accessible to clients not aware of multiple planes or modifiers, e.g. by
reallocating and copying.

This makes it possible to run a compositing manager on an old GLX/EGL
stack on top of an X server which allocates internal buffer storage
using exotic modifiers from modifier-aware GBM/EGL/KMS.

Signed-off-by: Daniel Stone 
Reported-by: Adam Jackson 
---
 dri3/dri3_priv.h|  3 +++
 dri3/dri3_request.c | 16 +---
 dri3/dri3_screen.c  | 40 
 3 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/dri3/dri3_priv.h b/dri3/dri3_priv.h
index 8447680ba..d86f06be0 100644
--- a/dri3/dri3_priv.h
+++ b/dri3/dri3_priv.h
@@ -83,6 +83,9 @@ dri3_pixmap_from_fds(PixmapPtr *ppixmap, ScreenPtr screen, 
CARD8 num_fds, int *f
  CARD16 width, CARD16 height, CARD32 *strides, CARD32 
*offsets,
  CARD8 depth, CARD8 bpp, CARD64 modifier);
 
+int
+dri3_fd_from_pixmap(PixmapPtr pixmap, CARD16 *stride, CARD32 *size);
+
 int
 dri3_fds_from_pixmap(PixmapPtr pixmap, int *fds,
  CARD32 *strides, CARD32 *offsets,
diff --git a/dri3/dri3_request.c b/dri3/dri3_request.c
index fc258711b..2d3deb282 100644
--- a/dri3/dri3_request.c
+++ b/dri3/dri3_request.c
@@ -212,10 +212,7 @@ proc_dri3_buffer_from_pixmap(ClientPtr client)
 .length = 0,
 };
 int rc;
-int num_fds;
-int fds[4];
-uint32_t strides[4], offsets[4];
-uint64_t modifier;
+int fd;
 PixmapPtr pixmap;
 
 REQUEST_SIZE_MATCH(xDRI3BufferFromPixmapReq);
@@ -231,13 +228,10 @@ proc_dri3_buffer_from_pixmap(ClientPtr client)
 rep.depth = pixmap->drawable.depth;
 rep.bpp = pixmap->drawable.bitsPerPixel;
 
-num_fds = dri3_fds_from_pixmap(pixmap, fds, strides, offsets, );
-if (num_fds != 1)
+fd = dri3_fd_from_pixmap(pixmap, , );
+if (fd == -1)
 return BadPixmap;
 
-rep.stride = (CARD16) strides[0];
-rep.size = rep.stride * rep.height;
-
 if (client->swapped) {
 swaps();
 swapl();
@@ -246,8 +240,8 @@ proc_dri3_buffer_from_pixmap(ClientPtr client)
 swaps();
 swaps();
 }
-if (WriteFdToClient(client, fds[0], TRUE) < 0) {
-close(fds[0]);
+if (WriteFdToClient(client, fd, TRUE) < 0) {
+close(fd);
 return BadAlloc;
 }
 
diff --git a/dri3/dri3_screen.c b/dri3/dri3_screen.c
index df40f8281..41595f412 100644
--- a/dri3/dri3_screen.c
+++ b/dri3/dri3_screen.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 
 static inline Bool has_open(dri3_screen_info_ptr info) {
 if (info == NULL)
@@ -124,6 +125,45 @@ dri3_fds_from_pixmap(PixmapPtr pixmap, int *fds,
 }
 }
 
+int
+dri3_fd_from_pixmap(PixmapPtr pixmap, CARD16 *stride, CARD32 *size)
+{
+ScreenPtr   screen = pixmap->drawable.pScreen;
+dri3_screen_priv_ptrds = dri3_screen_priv(screen);
+dri3_screen_info_ptrinfo = ds->info;
+CARD32  strides[4];
+CARD32  offsets[4];
+CARD64  modifier;
+int fds[4];
+int num_fds;
+
+if (!info)
+return -1;
+
+/* Preferentially use the old interface, allowing the implementation to
+ * ensure the buffer is in a single-plane format which doesn't need
+ * modifiers. */
+if (info->fd_from_pixmap != NULL)
+return (*info->fd_from_pixmap)(screen, pixmap, stride, size);
+
+if (info->version < 2 || info->fds_from_pixmap == NULL)
+return -1;
+
+/* If using the new interface, make sure that it's a single plane starting
+ * at 0 within the BO. We don't check the modifier, as the client may
+ * have an auxiliary mechanism for determining the modifier itself. */
+num_fds = info->fds_from_pixmap(screen, pixmap, fds, strides, offsets,
+);
+if (num_fds != 1 || offsets[0] != 0) {
+int i;
+for (i = 0; i < num_fds; i++)
+close(fds[i]);
+return -1;
+}
+
+return fds[0];
+}
+
 static int
 cache_formats_and_modifiers(ScreenPtr screen)
 {
-- 
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: [PATCH app/xdpyinfo v2] Use XRANDR 1.2 extension for reporting dimensions and resolution per output

[Adam Jackson]

On Tue, 2018-04-03 at 22:15 +0200, Pali Rohár wrote:
> Gently reminder of this patch. Can you comment/review it?

Nack. The whole point of (that part of) xdpyinfo is to show you what
was sent in the initial connection block. xrandr already exists, and
code exists that depends on xdpyinfo's output.

- 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 app/xdpyinfo v2] Use XRANDR 1.2 extension for reporting dimensions and resolution per output

[Pali Rohár]

On Wednesday 04 April 2018 13:59:15 Giuseppe Bilotta wrote:
> Hello,
> 
> I took the liberty of moving the last paragraph of your email to the
> top because the reply I'm giving to that part covers both.
> 
> On Wed, Apr 4, 2018 at 11:26 AM, Pali Rohár  wrote:
> >> >> -printf ("  dimensions:%dx%d pixels (%dx%d millimeters)\n",
> >> >> - XDisplayWidth (dpy, scr),  XDisplayHeight (dpy, scr),
> >> >> - XDisplayWidthMM(dpy, scr), XDisplayHeightMM (dpy, scr));
> >> >> -printf ("  resolution:%dx%d dots per inch\n",
> >> >> - (int) (xres + 0.5), (int) (yres + 0.5));
> >>
> >> I would unconditionally show the core output, regardless of RANDR
> >> state. Even if it's fictitious when RANDR is enabled,
> >> it can still be useful to spot inconsistencies. It also ensures that
> >> the xdpyinfo output is "less" broken by this patch (search for
> >> xdpyinfo grep resolution to get an idea of how used this is as a debug 
> >> tool).
> >
> > XRANDR code below provides that 'grep resolution' support correctly and
> > in the same format. It is there for all those scripts which expects
> > resolution and dimensions to be correct.
> 
> But that's a different piece of information. You mention:
> 
> > This patch is just to fix long standing bug, that scripts which parse
> > xdpyinfo output and users who looking at it too just get wrong
> > information about dimensions and DPI.
> 
> For users, having both pieces of information would be better than
> having just the
> RANDR one, especially for debugging, as that's exactly what the server
> is providing
> via its core protocol, and what legacy (non-RANDR-aware) clients will
> get and use.

Agree.

> For scripts, the situation is a bit hairier, but I wonder how such a
> script would work on a
> server with multiple active outputs, where you patch introduces
> multiple “resolution”
> lines anyway.

Multiple resolution lines are there already. Just enable more Xscreens.
Such setups were common in past for dual head or for Xinerama
configurations. Old script needs to be aware of it, otherwise there were
broken in past too.

> If you think it's important for the primary RANDR output information
> to be first,
> maybe you can move the core information to _after_ the RANDR information,
> but I still think it's important enough that it should stick around.
> 
> (And of course it would be better to fix the scripts themselves, but I
> assume not all of them are
> open source.)

I have no idea, but I was told that more people do (or did?)
'xdpyinfo | grep resolution' optionally with '| head -1' to get current
display DPI.

But I have no problem to modify output of xdpyinfo to provide:

1. Information from XRANDR1.2+
   for all outputs, primary first
2. Information from core

> >> >> +
> >> >> +#ifdef XRANDR
> >> >> +if (XRRQueryExtension (dpy, _base, _base) &&
> >> >> +XRRQueryVersion (dpy, , ) &&
> >> >> +(major > 1 || (major == 1 && minor >= 2)) &&
> >> >> +(res = XRRGetScreenResources (dpy, RootWindow (dpy, scr
> >> >> +{
> >>
> >> I'm pondering whether it would be a good idea to print the RANDR
> >> version here, something like:
> >> printf("  RANDR (%d.%d) outputs:\n", major, minor);
> >> and then nesting the per-output data even more.
> >
> > I as a user, would expect to find XRANDR version in "xrandr" utility
> > output, not in xdpyinfo output.
> >
> > But if you really think that it would make sense to have it also in
> > xdpyinfo, it can be included somewhere...
> 
> I'm not sure if it'd be useful or not, really. We could do without.
> (I do show it in my xdpi , but as I said,
> I'm an information junkie).
> 
> (BTW, I don't think xrandr actually prints the version).

Seems, yes xrandr output does not contain it.

> >> This doesn't work correctly when the display is rotated, since the
> >> width/height in pixel will follow the orientation,
> >> but the physical sizes won't. You should probably take that into
> >> account, and possibly show the output orientation (e.g. in the
> >> dimensions line, or in the name line if you do add the "RANDR
> >>  outputs" header).
> >
> > Hm... I have not thinking about it. If this is a real problem I can look
> > at it and try to fix it.
> 
> Here's how it looks for a rotated output:
> 
>   output: eDP-1
> dimensions:1800x3200 pixels (346x194 millimeters)
> resolution:132x419 dots per inch
> 
> whereas non-rotated:
> 
>   output: eDP-1
> dimensions:3200x1800 pixels (346x194 millimeters)
> resolution:235x236 dots per inch

Ok, this should be fixed!

Question is: Should output from xdpyinfo respect rotation or provides
normalized output (as if no rotation was activated)?

> Example from my aforementioned xdpi, rotated:
> 
> eDP-1: 1800x3200 pixels, (R, connected) 194x346 mm: 236x235
> dpi, 92x92 dpcm, dot pitch 0.11mm
> 
> right side up:
> 
> eDP-1: 3200x1800 pixels, (U, 

Re: [PATCH app/xdpyinfo v2] Use XRANDR 1.2 extension for reporting dimensions and resolution per output

[Giuseppe Bilotta]

Hello,

I took the liberty of moving the last paragraph of your email to the
top because the reply I'm giving to that part covers both.

On Wed, Apr 4, 2018 at 11:26 AM, Pali Rohár  wrote:
>> >> -printf ("  dimensions:%dx%d pixels (%dx%d millimeters)\n",
>> >> - XDisplayWidth (dpy, scr),  XDisplayHeight (dpy, scr),
>> >> - XDisplayWidthMM(dpy, scr), XDisplayHeightMM (dpy, scr));
>> >> -printf ("  resolution:%dx%d dots per inch\n",
>> >> - (int) (xres + 0.5), (int) (yres + 0.5));
>>
>> I would unconditionally show the core output, regardless of RANDR
>> state. Even if it's fictitious when RANDR is enabled,
>> it can still be useful to spot inconsistencies. It also ensures that
>> the xdpyinfo output is "less" broken by this patch (search for
>> xdpyinfo grep resolution to get an idea of how used this is as a debug tool).
>
> XRANDR code below provides that 'grep resolution' support correctly and
> in the same format. It is there for all those scripts which expects
> resolution and dimensions to be correct.

But that's a different piece of information. You mention:

> This patch is just to fix long standing bug, that scripts which parse
> xdpyinfo output and users who looking at it too just get wrong
> information about dimensions and DPI.

For users, having both pieces of information would be better than
having just the
RANDR one, especially for debugging, as that's exactly what the server
is providing
via its core protocol, and what legacy (non-RANDR-aware) clients will
get and use.
For scripts, the situation is a bit hairier, but I wonder how such a
script would work on a
server with multiple active outputs, where you patch introduces
multiple “resolution”
lines anyway.
If you think it's important for the primary RANDR output information
to be first,
maybe you can move the core information to _after_ the RANDR information,
but I still think it's important enough that it should stick around.

(And of course it would be better to fix the scripts themselves, but I
assume not all of them are
open source.)

>> >> +
>> >> +#ifdef XRANDR
>> >> +if (XRRQueryExtension (dpy, _base, _base) &&
>> >> +XRRQueryVersion (dpy, , ) &&
>> >> +(major > 1 || (major == 1 && minor >= 2)) &&
>> >> +(res = XRRGetScreenResources (dpy, RootWindow (dpy, scr
>> >> +{
>>
>> I'm pondering whether it would be a good idea to print the RANDR
>> version here, something like:
>> printf("  RANDR (%d.%d) outputs:\n", major, minor);
>> and then nesting the per-output data even more.
>
> I as a user, would expect to find XRANDR version in "xrandr" utility
> output, not in xdpyinfo output.
>
> But if you really think that it would make sense to have it also in
> xdpyinfo, it can be included somewhere...

I'm not sure if it'd be useful or not, really. We could do without.
(I do show it in my xdpi , but as I said,
I'm an information junkie).

(BTW, I don't think xrandr actually prints the version).

>> This doesn't work correctly when the display is rotated, since the
>> width/height in pixel will follow the orientation,
>> but the physical sizes won't. You should probably take that into
>> account, and possibly show the output orientation (e.g. in the
>> dimensions line, or in the name line if you do add the "RANDR
>>  outputs" header).
>
> Hm... I have not thinking about it. If this is a real problem I can look
> at it and try to fix it.

Here's how it looks for a rotated output:

  output: eDP-1
dimensions:1800x3200 pixels (346x194 millimeters)
resolution:132x419 dots per inch

whereas non-rotated:

  output: eDP-1
dimensions:3200x1800 pixels (346x194 millimeters)
resolution:235x236 dots per inch

Example from my aforementioned xdpi, rotated:

eDP-1: 1800x3200 pixels, (R, connected) 194x346 mm: 236x235
dpi, 92x92 dpcm, dot pitch 0.11mm

right side up:

eDP-1: 3200x1800 pixels, (U, connected) 346x194 mm: 235x236
dpi, 92x92 dpcm, dot pitch 0.11mm

The resolution should be rotational invariant (the monitor dot pitch
does not change if I put it on its side),
but the RANDR protocol doesn't take care of swapping the physical
dimensions, so you have to do it yourself.

>> Also (please don't hate me), for RANDR versions 1.5 or higher we
>> should include RANDR monitors too (possibly in place of individual
>> outputs, I'm not sure about this actually, it depends on how detailed
>> we want to be; I would go with both, but then again I'm an information
>> junkie).
>
> I know that I RANDR 1.5+ supports "monitors" which is needed for display
> port outputs, but unfortunately I do not have such configuration for
> testing. And I do not like idea to write and send code without real
> testing.

If no monitors are manually created, the X server will automatically generate
one per output, so you should be able to test the code even without an
actual MST monitor.

-- 
Giuseppe 

Re: Bug 105851 Xserver 1.20 RC2+ issues with Kwin + Present 1.2

[Emil Velikov]

Hi Mike,

On 4 April 2018 at 09:12, Mike Lothian  wrote:
> Hi
>
> Kwin doesn't seem to start with the following commit:
>
> commit abb9b58d1af9a0286162e52ef9db390d0c950fc1
> Author: Thierry Reding 
> Date:   Fri Mar 16 14:24:21 2018 +0100
>
> present: Advertise protocol version 1.2
>
> Everything is implemented to support protocol version 1.2. Make it
> official.
>
> Reviewed-by: Daniel Stone 
> Signed-off-by: Thierry Reding 
> Reviewed-by: Emil Velikov 
>
> Though I think the issue may lie in commit:
>
> commit 6a5d51e0823b43280e3646b7a0c919a3b76146ea
> Author: Emil Velikov 
> Date:   Mon Mar 19 16:04:43 2018 +
>
> present: cap the version returned to the client
>
> As per the protocol, the server should not return version greater than
> the one supported by the client.
>
> Add a spec quote and tweak the numbers accordingly.
>
> I realise that Kwin will probably need to be modified to work with
> Present 1.2, but I don't think we should be seeing breakage unti that
> happens
>
AFAICT Kwin does not directly use the present extension, does it?

Can you ensure that you have mesa commit
696762eef57e83b4027acbdf0a6e74d1f75083b0.
It addresses some hunky stuff with the version handling.

-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 01/15] dri3: annotate the dri3_screen_info data as const

[Emil Velikov]

On 2 April 2018 at 20:34, Adam Jackson  wrote:
> On Mon, 2018-04-02 at 16:41 +0100, Emil Velikov wrote:
>
>> Why do we have the explicit _rec and _ptr typecasts to begin with?
>
> Convention, mostly. The typedef for the struct is because 'struct' is a
> dumb word to need to say all the time. The typedef for the pointer is
> vaguely distasteful and I've been trying to stop.
>
s/dumb/annoying/

Using _t suffix over _rec seems more indicative IMHO.
Regardless, most of the series is a must have for 1.20 - if you want
to give it a skim that'll be appreciated.

Thanks
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 app/xdpyinfo v2] Use XRANDR 1.2 extension for reporting dimensions and resolution per output

[Pali Rohár]

Hi!

On Wednesday 04 April 2018 09:47:59 Giuseppe Bilotta wrote:
> Hello Pali,
> 
> I like the idea of this patch, wasn't aware of its existence, thanks
> for pinging me about it. A few comments below:
> 
> On Tue, Apr 3, 2018 at 10:15 PM, Pali Rohár  wrote:
> >> @@ -455,27 +462,75 @@ print_screen_info(Display *dpy, int scr)
> >>  double xres, yres;
> >>  int ndepths = 0, *depths = NULL;
> >>  unsigned int width, height;
> >> -
> >> -/*
> >> - * there are 2.54 centimeters to an inch; so there are 25.4 
> >> millimeters.
> >> - *
> >> - * dpi = N pixels / (M millimeters / (25.4 millimeters / 1 inch))
> >> - * = N pixels / (M inch / 25.4)
> >> - * = N * 25.4 pixels / M inch
> >> - */
> 
> You may want to keep this comment here, rather than in the else block below,
> since the formula applies to any conversion, regardless if it's core
> or per-output DPI.

Ok, no problem.

> >> -xres = double) DisplayWidth(dpy,scr)) * 25.4) /
> >> - ((double) DisplayWidthMM(dpy,scr)));
> >> -yres = double) DisplayHeight(dpy,scr)) * 25.4) /
> >> - ((double) DisplayHeightMM(dpy,scr)));
> >> +#ifdef XRANDR
> >> +int event_base, error_base;
> >> +int major, minor;
> >> +XRRScreenResources *res = NULL;
> >> +XRROutputInfo *output;
> >> +XRRCrtcInfo *crtc;
> >> +#endif
> >>
> >>  printf ("\n");
> >>  printf ("screen #%d:\n", scr);
> >> -printf ("  dimensions:%dx%d pixels (%dx%d millimeters)\n",
> >> - XDisplayWidth (dpy, scr),  XDisplayHeight (dpy, scr),
> >> - XDisplayWidthMM(dpy, scr), XDisplayHeightMM (dpy, scr));
> >> -printf ("  resolution:%dx%d dots per inch\n",
> >> - (int) (xres + 0.5), (int) (yres + 0.5));
> 
> I would unconditionally show the core output, regardless of RANDR
> state. Even if it's fictitious when RANDR is enabled,
> it can still be useful to spot inconsistencies. It also ensures that
> the xdpyinfo output is "less" broken by this patch (search for
> xdpyinfo grep resolution to get an idea of how used this is as a debug tool).

XRANDR code below provides that 'grep resolution' support correctly and
in the same format. It is there for all those scripts which expects
resolution and dimensions to be correct.

> >> +
> >> +#ifdef XRANDR
> >> +if (XRRQueryExtension (dpy, _base, _base) &&
> >> +XRRQueryVersion (dpy, , ) &&
> >> +(major > 1 || (major == 1 && minor >= 2)) &&
> >> +(res = XRRGetScreenResources (dpy, RootWindow (dpy, scr
> >> +{
> 
> I'm pondering whether it would be a good idea to print the RANDR
> version here, something like:
> printf("  RANDR (%d.%d) outputs:\n", major, minor);
> and then nesting the per-output data even more.

I as a user, would expect to find XRANDR version in "xrandr" utility
output, not in xdpyinfo output.

But if you really think that it would make sense to have it also in
xdpyinfo, it can be included somewhere...

> >> +for (i = 0; i < res->noutput; ++i) {
> >> +output = XRRGetOutputInfo (dpy, res, res->outputs[i]);
> >> +if (!output || !output->crtc || output->connection != 
> >> RR_Connected)
> >> +continue;
> >> +
> >> +crtc = XRRGetCrtcInfo (dpy, res, output->crtc);
> >> +if (!crtc) {
> >> +XRRFreeOutputInfo (output);
> >> +continue;
> >> +}
> >> +
> >> +printf ("  output: %s\n", output->name);
> >> +printf ("dimensions:%ux%u pixels (%lux%lu 
> >> millimeters)\n",
> >> +crtc->width, crtc->height, output->mm_width, 
> >> output->mm_height);
> >> +
> >> +if (output->mm_width && output->mm_height) {
> >> +xres = double) crtc->width) * 25.4) / ((double) 
> >> output->mm_width));
> >> +yres = double) crtc->height) * 25.4) / ((double) 
> >> output->mm_height));
> >> +} else {
> >> +xres = 0;
> >> +yres = 0;
> >> +}
> >> +printf ("resolution:%dx%d dots per inch\n",
> >> +(int) (xres + 0.5), (int) (yres + 0.5));
> 
> This doesn't work correctly when the display is rotated, since the
> width/height in pixel will follow the orientation,
> but the physical sizes won't. You should probably take that into
> account, and possibly show the output orientation (e.g. in the
> dimensions line, or in the name line if you do add the "RANDR
>  outputs" header).

Hm... I have not thinking about it. If this is a real problem I can look
at it and try to fix it.

> Also (please don't hate me), for RANDR versions 1.5 or higher we
> should include RANDR monitors too (possibly in place of individual
> outputs, I'm not sure about this actually, it depends on how detailed
> we want to be; I would go with both, but then again I'm an information
> junkie).

I know that I RANDR 

Bug 105851 Xserver 1.20 RC2+ issues with Kwin + Present 1.2

[Mike Lothian]

Hi

Kwin doesn't seem to start with the following commit:

commit abb9b58d1af9a0286162e52ef9db390d0c950fc1
Author: Thierry Reding 
Date:   Fri Mar 16 14:24:21 2018 +0100

present: Advertise protocol version 1.2

Everything is implemented to support protocol version 1.2. Make it
official.

Reviewed-by: Daniel Stone 
Signed-off-by: Thierry Reding 
Reviewed-by: Emil Velikov 

Though I think the issue may lie in commit:

commit 6a5d51e0823b43280e3646b7a0c919a3b76146ea
Author: Emil Velikov 
Date:   Mon Mar 19 16:04:43 2018 +

present: cap the version returned to the client

As per the protocol, the server should not return version greater than
the one supported by the client.

Add a spec quote and tweak the numbers accordingly.

I realise that Kwin will probably need to be modified to work with
Present 1.2, but I don't think we should be seeing breakage unti that
happens

Regards

Mike
___
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 app/xdpyinfo v2] Use XRANDR 1.2 extension for reporting dimensions and resolution per output

[Giuseppe Bilotta]

Hello Pali,

I like the idea of this patch, wasn't aware of its existence, thanks
for pinging me about it. A few comments below:

On Tue, Apr 3, 2018 at 10:15 PM, Pali Rohár  wrote:
>> @@ -455,27 +462,75 @@ print_screen_info(Display *dpy, int scr)
>>  double xres, yres;
>>  int ndepths = 0, *depths = NULL;
>>  unsigned int width, height;
>> -
>> -/*
>> - * there are 2.54 centimeters to an inch; so there are 25.4 millimeters.
>> - *
>> - * dpi = N pixels / (M millimeters / (25.4 millimeters / 1 inch))
>> - * = N pixels / (M inch / 25.4)
>> - * = N * 25.4 pixels / M inch
>> - */

You may want to keep this comment here, rather than in the else block below,
since the formula applies to any conversion, regardless if it's core
or per-output DPI.

>> -xres = double) DisplayWidth(dpy,scr)) * 25.4) /
>> - ((double) DisplayWidthMM(dpy,scr)));
>> -yres = double) DisplayHeight(dpy,scr)) * 25.4) /
>> - ((double) DisplayHeightMM(dpy,scr)));
>> +#ifdef XRANDR
>> +int event_base, error_base;
>> +int major, minor;
>> +XRRScreenResources *res = NULL;
>> +XRROutputInfo *output;
>> +XRRCrtcInfo *crtc;
>> +#endif
>>
>>  printf ("\n");
>>  printf ("screen #%d:\n", scr);
>> -printf ("  dimensions:%dx%d pixels (%dx%d millimeters)\n",
>> - XDisplayWidth (dpy, scr),  XDisplayHeight (dpy, scr),
>> - XDisplayWidthMM(dpy, scr), XDisplayHeightMM (dpy, scr));
>> -printf ("  resolution:%dx%d dots per inch\n",
>> - (int) (xres + 0.5), (int) (yres + 0.5));

I would unconditionally show the core output, regardless of RANDR
state. Even if it's fictitious when RANDR is enabled,
it can still be useful to spot inconsistencies. It also ensures that
the xdpyinfo output is "less" broken by this patch (search for
xdpyinfo grep resolution to get an idea of how used this is as a debug tool).

>> +
>> +#ifdef XRANDR
>> +if (XRRQueryExtension (dpy, _base, _base) &&
>> +XRRQueryVersion (dpy, , ) &&
>> +(major > 1 || (major == 1 && minor >= 2)) &&
>> +(res = XRRGetScreenResources (dpy, RootWindow (dpy, scr
>> +{

I'm pondering whether it would be a good idea to print the RANDR
version here, something like:
printf("  RANDR (%d.%d) outputs:\n", major, minor);
and then nesting the per-output data even more.

>> +for (i = 0; i < res->noutput; ++i) {
>> +output = XRRGetOutputInfo (dpy, res, res->outputs[i]);
>> +if (!output || !output->crtc || output->connection != 
>> RR_Connected)
>> +continue;
>> +
>> +crtc = XRRGetCrtcInfo (dpy, res, output->crtc);
>> +if (!crtc) {
>> +XRRFreeOutputInfo (output);
>> +continue;
>> +}
>> +
>> +printf ("  output: %s\n", output->name);
>> +printf ("dimensions:%ux%u pixels (%lux%lu 
>> millimeters)\n",
>> +crtc->width, crtc->height, output->mm_width, 
>> output->mm_height);
>> +
>> +if (output->mm_width && output->mm_height) {
>> +xres = double) crtc->width) * 25.4) / ((double) 
>> output->mm_width));
>> +yres = double) crtc->height) * 25.4) / ((double) 
>> output->mm_height));
>> +} else {
>> +xres = 0;
>> +yres = 0;
>> +}
>> +printf ("resolution:%dx%d dots per inch\n",
>> +(int) (xres + 0.5), (int) (yres + 0.5));

This doesn't work correctly when the display is rotated, since the
width/height in pixel will follow the orientation,
but the physical sizes won't. You should probably take that into
account, and possibly show the output orientation (e.g. in the
dimensions line, or in the name line if you do add the "RANDR
 outputs" header).

Also (please don't hate me), for RANDR versions 1.5 or higher we
should include RANDR monitors too (possibly in place of individual
outputs, I'm not sure about this actually, it depends on how detailed
we want to be; I would go with both, but then again I'm an information
junkie).

Cheers,

-- 
Giuseppe "Oblomov" Bilotta
___
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