Re: RFC - GLX Extension to control GLXVND dispatching for PRIME GPU offloading

2019-04-24 Thread Kyle Brenneman

On 4/23/19 4:28 PM, Aaron Plattner wrote:

On 4/17/19 8:51 AM, Kyle Brenneman wrote:
For GPU offloading in libglvnd, where individual clients can run with 
an alternate GPU and client-side vendor library, we'd need some way 
for that alternate vendor library to communicate with its server-side 
counterpart. Normally, the server's GLXVND layer would dispatch any 
GLX requests to whichever driver is running an X screen. This is a 
GLX extension that allows a client to tell the server to send GLX 
requests to a different driver instead.


The basic idea is that the server keeps a separate (screen -> 
GLXServerVendor) mapping for each client. The current global mapping 
is used as the default for each new client, but the client can send a 
request to change its own mapping. That way, if the client uses a 
different vendor library, then the client-side vendor can arrange for 
any GLX requests to go to the matching server-side driver.


The extension uses Atoms as an ID to identify each GLXServerVendor, 
using a string provided by the driver. That way, the client-side 
driver can know which Atom it needs to use without having to define 
an extra query. The client can send a request with a screen number 
and a vendor ID to tell the server to dispatch any GLX requests for 
that screen to the specified vendor. A client can also send None as a 
vendor ID to revert to whatever GLXServerVendor would handle that 
screen by default.


I also added a GLXVendorPrivate/GLXVendorPrivateWithReply-style 
request, which sends a request to a specific vendor based on a vendor 
ID, without having to worry about which vendor is assigned to a 
screen at the moment. Strictly speaking, a vendor library could get 
the same result by adding a regular GLXVendorPrivate request, and 
providing a dispatch function that always routes the request to 
itself, but that seems like it's more of an implementation detail of 
GLXVND.


Also, this extension doesn't define any errors or queries to check 
whether a GLXServerVendor can support a given screen. These requests 
would be sent by a client-side vendor library (not by libglvnd or an 
application), so each driver would be responsible for figuring out on 
its own which screens it can support.


Anyway, I've got a draft of the extension spec here, and I've written 
up a series of patches for the X server to implement it here:
https://gitlab.freedesktop.org/kbrenneman/xserver/tree/GLX_EXT_server_vendor_select 



Hi Kyle,

Have you gotten any feedback on the commits there? It might help to 
create an xorg/xserver merge request for them. You can use the "WIP:" 
prefix on the MR title if you just want to request feedback without 
actually getting it merged.
Not a bad idea. I've posted a merge request with the patches here, and 
attached the extension spec:

https://gitlab.freedesktop.org/xorg/xserver/merge_requests/179

-Kyle


Comments and questions welcome.

-Kyle Brenneman


Name

 EXT_server_vendor_select

Name Strings

 GLX_EXT_server_vendor_select

Contact

 Kyle Brenneman, NVIDIA, kbrenneman at nvidia.com

Contributors

 Kyle Brenneman

Status

 XXX - Not complete yet!!!

Version

 Last Modified Date: April 11, 2019
 Revision: 1

Number

 OpenGL Extension #???

Dependencies

 GLX version 1.2 is required.

 This specification is written against the wording of the GLX 1.3
 Protocol Encoding Specification.

Overview

 In multi-GPU systems, a client may decide at runtime which device
 and driver to use for GLX, for example to choose between a
 high-performance and low-power device.

 This extension defines a set of requests that allow a client to
 specify which server-side driver should handle GLX requests from 
the

 sending client for a particular screen.

IP Status

 No known IP claims.

New Procedures and Functions

 None

New Tokens

 None

Additions to the GLX Specification

 None. These requests are intended to be used by a client-side GLX
 implementation, not by an application. Therefore, this extension
 does not define any new functions or changes to the GLX
 specification.

GLX Protocol

 Get a List of Server-Side Drivers

 Name: glXQueryServerVendorIDsEXT

 Description:
 This request fetches a list of available server-side
 drivers, and the current vendor ID selected for each 
screen.

 Each driver is identified by an Atom, with a string chosen
 by the driver.

 The reply contains a list of the currently selected vendors
 first, with one Atom for each screen. This will be the
 vendor selected with the glXSelectScreenServerVendorIDEXT
 request, or the default vendor if the client has not sent a
 glXSelectScreenServerVendorIDEXT request for a screen.

 If a screen is using the default vendor, and the vendor 
does

 not have a vendor ID, then 

Re: [PATCH v2] Fix checking for valid argument of --dpi

2019-04-24 Thread Pali Rohár
On Sunday 30 December 2018 14:11:13 Pali Rohár wrote:
> On Sunday 30 December 2018 13:51:10 Walter Harms wrote:
> > 
> > 
> > > Pali Rohár  hat am 29. Dezember 2018 um 18:45
> > > geschrieben:
> > > 
> > > 
> > > On Monday 26 November 2018 22:17:24 Pali Rohár wrote:
> > > > Function strtod() sets strtod_error to the pointer of the first invalid
> > > > character and therefore it does not have to be first character from 
> > > > input.
> > > > When input is valid then it points to nul byte. Conversion error is
> > > > indicated by setted errno. Zero-length argument, non-positive DPI or DPI
> > > > with trailing or leading whitespaces is invalid too.
> > > > 
> > > > Update also error message about invalid argument.
> > > > 
> > > > Signed-off-by: Pali Rohár 
> > > > 
> > > > --
> > > > 
> > > > Changes since v1:
> > > > * Get same error message for `xrandr --dpi ''` and `xrandr --dpi ' '`
> > > > * Make the check for dpi <= 0
> > > > * Do not accept leading whitespaces (trailing were already disallowed)
> > > > ---
> > > >  xrandr.c | 6 --
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/xrandr.c b/xrandr.c
> > > > index ce3cd91..4baa075 100644
> > > > --- a/xrandr.c
> > > > +++ b/xrandr.c
> > > > @@ -40,6 +40,7 @@
> > > >  #include 
> > > >  #include 
> > > >  #include 
> > > > +#include 
> > > >  
> > > >  #ifdef HAVE_CONFIG_H
> > > >  #include "config.h"
> > > > @@ -3118,8 +3119,9 @@ main (int argc, char **argv)
> > > > if (!strcmp ("--dpi", argv[i])) {
> > > > char *strtod_error;
> > > > if (++i >= argc) argerr ("%s requires an argument\n", 
> > > > argv[i-1]);
> > > > +   errno = 0;
> > > > dpi = strtod(argv[i], &strtod_error);
> > > > -   if (argv[i] == strtod_error)
> > > > +   if (!argv[i][0] || isspace(argv[i][0]) || *strtod_error || 
> > > > errno ||
> > > > dpi <= 0)
> > > > {
> > > > dpi = 0.0;
> > > > dpi_output_name = argv[i];
> > 
> > 
> > Why spending so much effort ? 
> 
> Which "much effort"?
> 
> This is to ensure that incorrect param or garbage supplied by user is
> not interpreted as number.
> 
> > Using atoi(argv[i]) and checking for an invalid value seems fine.
> 
> atoi does not signal when garbage is on input. And dpi can be
> fractional, so atoi cannot be used for it.
> 
> > Special cases like NAN do not matter here.
> > 
> > BTW: leading whitespaces are ignored with strtod() (so far i know)
> 
> But strtod does not signal that they were ignored. Moreover trailing are
> not accepted. So I chose implementation which is consistent for trailing
> and leading whitespaces.

Any other review comments for this patch?

I would like to know if this patch could be applied or if there are some
other problems which needs to be resolved.

> > just my 2 cents
> > re,
> >  wh
> > 
> > 
> > 
> > > > @@ -3569,7 +3571,7 @@ main (int argc, char **argv)
> > > > XRROutputInfo   *output_info;
> > > > XRRModeInfo *mode_info;
> > > > if (!dpi_output)
> > > > -   fatal ("Cannot find output %s\n", dpi_output_name);
> > > > +   fatal ("%s is not valid DPI nor valid output\n", 
> > > > dpi_output_name);
> > > > output_info = dpi_output->output_info;
> > > > mode_info = dpi_output->mode_info;
> > > > if (output_info && mode_info && output_info->mm_height)
> > > 
> > > Hello, can you review this v2 patch?

-- 
Pali Rohár
pali.ro...@gmail.com
___
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