Re: [PATCH xserver 2/9] Remove SIGIO support for input [v3]

2016-05-16 Thread Peter Hutterer 
On Mon, May 16, 2016 at 01:24:21AM -0500, Keith Packard wrote:
> Peter Hutterer  writes:
> 
> Thanks for taking a look at these. This one is less mechanical than I'd
> like, so some of the changes aren't obvious. It's definitely good to
> review them carefully.
> 
> >>  KdNotifyFd(int fd, int ready, void *data)
> >>  {
> >>  int i = (int) (intptr_t) data;
> >> -OsBlockSIGIO();
> >>  (*kdInputFds[i].read)(fd, kdInputFds[i].closure);
> >> -OsReleaseSIGIO();
> >
> > how comes you don't replace these with input_lock/input_unlock()? if there's
> > a specific reason it'd be better to have this in a follow-up patch, I'd
> > prefer this patch to be mostly mechanical.
> 
> Just that we won't need a lock here in the threaded input case; threaded
> input holds the input lock across all input processing. This could go in
> a separate patch, but of course with SIGIO removed, we wouldn't need
> locking anyways.
> 
> If I replaced them with input_lock/input_unlock, I'd just have to remove
> those calls when threaded input got added.

yeah, but again it's imo easier to have this as a plain replacement,
followed by a patch that says "Not needed here" after the sigio replacement.
better documentation for the archives because without any comments it's hard
to tell whether this is a mistake or not.

> 
> >>  static void
> >>  KdAddFd(int fd, int i)
> >>  {
> >> -struct sigaction act;
> >> -sigset_t set;
> >> -
> >> -kdnFds++;
> >> -fcntl(fd, F_SETOWN, getpid());
> >>  KdNonBlockFd(fd);
> >> -AddEnabledDevice(fd);
> >
> >
> > it's not clear why you're dropping this bit.
> 
> (the fcntl should be self-evident)
> 
> The AddEnabledDevice call was not necessary, given the SetNotifyFd call
> just below. Was a harmless bug before threaded input as it just whacked
> the same mask that SetNotifyFd was going to whack. However, with
> threaded input, we don't want the main server select to see the input
> devices, so the call needs to be removed.

right, in that case it'd be better to remove the AddEnabledDevice() call in
a patch before this one and then just do the obvious replacements in this
commit?

> > same here.
> 
> Same reason.
> 
> >> -{FLAG_USE_SIGIO, "UseSIGIO", OPTV_BOOLEAN,
> >> - {0}, FALSE},
> >
> > not 100% here - if you take this out won't current configs with that option
> > now throw an error? should't we be more lenient here?
> 
> Good point. Leaving this in seems harmless enough. One wonders if this
> value has ever been used...
> 
> Do you want me to rearrange the other fixes above to make more sense? Or
> is it good enough to have explanation here?

long-term it's better to have separate patches here, it makes archeology
less mysterious :)

Cheers,
   Peter

___
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 7/7] glx: Conditionalize building indirect GLX support

2016-05-16 Thread Emil Velikov 
Hi Adam,

As some people (me alone?) finds all the ifdefined a bit annoying here
are some ideas of how to resolve it. Other than that, there's a couple
of serious suggestions inline, listing them here to just in case:
- One should AC_ERROR when using --disable-glx --enable-iglx ?
- How do we manage {+,-}iglx command line options, when built without
IGLX ? Warn, error or hide the options ?

On 1 April 2016 at 17:53, Adam Jackson  wrote:
> This nerfs:
>
> - Render and RenderLarge
> - UseXFont
> - CopyContext
> - vendor private GL requests
> - vendor private GLX requests where GLX protocol is only generated for
>   indirect contexts, namely SGI_swap_control, MESA_copy_sub_buffer, and
>   EXT_texture_from_pixmap
>
> What's left is enough GLX to support direct contexts only, and the
> resulting libglx does not need to link against libGL.
>
> Signed-off-by: Adam Jackson 
> ---
>  configure.ac| 12 +--
>  glx/Makefile.am | 39 +++-
>  glx/glxcmds.c   | 50 +-
>  glx/glxcmdsswap.c   | 34 ++
>  glx/glxdri2.c   |  6 
>  glx/glxdriswrast.c  |  7 +++-
>  glx/glxext.c| 96 
> -
>  glx/glxserver.h |  4 +++
>  include/dix-config.h.in |  3 ++
>  9 files changed, 229 insertions(+), 22 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index dff06ef..9ef0f39 100644
> --- a/configure.ac
> +++ b/configure.ac

> +if test "x$IGLX" = xyes; then
> +AC_DEFINE(IGLX, 1, [Build indirect GLX support])
> +fi
> +AM_CONDITIONAL(IGLX, test "x$IGLX" = xyes)
> +
Afaics IGLX depends on GLX, thus we should error here ?

That is unless we have have a volunteer to refactor things to allow
IGLX without GLX.


> @@ -52,11 +72,8 @@ libglx_la_SOURCES = \
> createcontext.c \
> extension_string.c \
> extension_string.h \
> -   indirect_util.c \
> indirect_util.h \
> -   indirect_program.c \
> indirect_table.h \
Worth moving these alongside their indirect brothers/sisters above,
even though some/most of these are included by the dri* code. Ideally
that'll get untangled at some point.


> --- a/glx/glxcmds.c
> +++ b/glx/glxcmds.c
> @@ -289,7 +289,10 @@ DoCreateContext(__GLXclientState * cl, GLXContextID gcId,
>   * it's a massive attack surface for buffer overflow type
>   * errors.
>   */
> -if (!enableIndirectGLX) {
> +#if defined(IGLX)
> +if (!enableIndirectGLX)
> +#endif
I would move the enableIndirectGLX management to inline wrappers in
the header. As is, if xserver is built without IGLX, yet the user
attempts to enable it via the cmd line, they won't get any feedback.
Thus as they try using it there will be a lot of confusion.

I'm thinking about something like

cat some_header.h

static inline void
toggleIndirectGLX(Bool foo)
{
#if defined(IGLX)
   enableIndirectGLX = foo;
#else
   warn/error(xserver was built without support for IGLX)
#endif
}

static inline Bool
isEnabledIndirectGLX(void)
{
#if defined(IGLX)
   return enableIndirectGLX;
#else
   /* warn_once ? */
   return False;
#endif
}

Speaking of which ... enableIndirectGLX should be hidden (kill off the
_X_EXPORT), shouldn't it ?

> +{
>  client->errorValue = isDirect;
>  return BadValue;
>  }
> @@ -863,6 +866,7 @@ __glXDisp_WaitX(__GLXclientState * cl, GLbyte * pc)
>  int
>  __glXDisp_CopyContext(__GLXclientState * cl, GLbyte * pc)
>  {
> +#if defined(IGLX)
>  ClientPtr client = cl->client;
>  xGLXCopyContextReq *req = (xGLXCopyContextReq *) pc;
>  GLXContextID source;
> @@ -937,6 +941,10 @@ __glXDisp_CopyContext(__GLXclientState * cl, GLbyte * pc)
>  return BadValue;
>  }
>  return Success;
> +#else
> +/* we know we never have indirect contexts */
> +return __glXError(GLXBadContext);
> +#endif
>  }
>
This and the remaining ifdef IGLX could be moved with headers
providing handly stubs. Namely

$ git grep __glXDisp_CopyContext -- glx/indirect_dispatch.h

#if defined(IGLX)
// Function implementation/definition is in indirect_something.c file
int
__glXDisp_CopyContext(__GLXclientState * cl, GLbyte * pc);
#else
static inline int
__glXDisp_CopyContext(__GLXclientState * cl, GLbyte * pc)
{
/* we know we never have indirect contexts */
return __glXError(GLXBadContext);
}
#endif


One cold even move the generic (non IGLX specific) function
declarations to glx{,_}dispatch.h or alike.


> +#if !defined(IGLX)
> +
> +#define proc(s) \
> +if (vendorcode == X_GLXvop_ ## s) \
> +return __glXDisp_ ## s;
> +
Move the define where it's used (within the function) and undef when done ?


> @@ -2359,9 +2399,13 @@ __glXDisp_VendorPrivate(__GLXclientState * cl, GLbyte 
> * pc)
>
>  REQUEST_AT_LEAST_SIZE(xGLXVendorPrivateReq);
>
> +#if defined(IGLX)
>  proc = (__GLXdispatchVendorPrivProcPtr)
>

Re: [PATCH xserver 1/3] glamor: Disable logic ops when doing compositing [v3]

2016-05-16 Thread Keith Packard 
Michel Dänzer  writes:

> https://bugs.freedesktop.org/show_bug.cgi?id=63397#c24

Some day I'll think to look at bugzilla when I fix a bug :-)

I've added a comment and copy of the patch to that bug.

-- 
-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 1/3] glamor: Disable logic ops when doing compositing [v3]

2016-05-16 Thread Michel Dänzer 

On 15.05.2016 19:48, Keith Packard wrote:


I'm surprised no-one else has noticed the problems with libreoffice;
random bits of GUI text rendered as black rectangles was pretty annoying
to me.


https://bugs.freedesktop.org/show_bug.cgi?id=63397#c24


--
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 7/9] dix: Reallocate touchpoint buffer at input event time

2016-05-16 Thread Keith Packard 
Peter Hutterer  writes:

> fwiw, that first sentence isn't correct anymore, you can drop it.
> rev-by still stands.

I've edited the comment, along with re-adding the UseSIGIO option to the
parsing code and pushed out an updated thread with all of your kind
Rb/Ab lines added. We have but the two giant patches left to see through
the review process before this series can be merged. I'd love to make
those simpler, but I can't imagine how...

Thanks again for helping get us this far.

-- 
-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 2/9] Remove SIGIO support for input [v3]

2016-05-16 Thread Keith Packard 
Peter Hutterer  writes:

Thanks for taking a look at these. This one is less mechanical than I'd
like, so some of the changes aren't obvious. It's definitely good to
review them carefully.

>>  KdNotifyFd(int fd, int ready, void *data)
>>  {
>>  int i = (int) (intptr_t) data;
>> -OsBlockSIGIO();
>>  (*kdInputFds[i].read)(fd, kdInputFds[i].closure);
>> -OsReleaseSIGIO();
>
> how comes you don't replace these with input_lock/input_unlock()? if there's
> a specific reason it'd be better to have this in a follow-up patch, I'd
> prefer this patch to be mostly mechanical.

Just that we won't need a lock here in the threaded input case; threaded
input holds the input lock across all input processing. This could go in
a separate patch, but of course with SIGIO removed, we wouldn't need
locking anyways.

If I replaced them with input_lock/input_unlock, I'd just have to remove
those calls when threaded input got added.

>>  static void
>>  KdAddFd(int fd, int i)
>>  {
>> -struct sigaction act;
>> -sigset_t set;
>> -
>> -kdnFds++;
>> -fcntl(fd, F_SETOWN, getpid());
>>  KdNonBlockFd(fd);
>> -AddEnabledDevice(fd);
>
>
> it's not clear why you're dropping this bit.

(the fcntl should be self-evident)

The AddEnabledDevice call was not necessary, given the SetNotifyFd call
just below. Was a harmless bug before threaded input as it just whacked
the same mask that SetNotifyFd was going to whack. However, with
threaded input, we don't want the main server select to see the input
devices, so the call needs to be removed.

> same here.

Same reason.

>> -{FLAG_USE_SIGIO, "UseSIGIO", OPTV_BOOLEAN,
>> - {0}, FALSE},
>
> not 100% here - if you take this out won't current configs with that option
> now throw an error? should't we be more lenient here?

Good point. Leaving this in seems harmless enough. One wonders if this
value has ever been used...

Do you want me to rearrange the other fixes above to make more sense? Or
is it good enough to have explanation here?

-- 
-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