[PATCH] Replace malloc with calloc to initialize the buffers[] as NULL in do_get_buffers function
From: Justin Dou
The calling for allocate_or_reuse_buffer may fail due to some reason, e.g. out
of memory.
If the buffers[] were not initialized to be NULL, the following err_out may try
to access an illegal memory, which will cause X crash afterward.
Reviewed-by: Kristian Høgsberg
Signed-off-by: Justin Dou
---
hw/xfree86/dri2/dri2.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index 34f735f..5d31e77 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -403,7 +403,7 @@ do_get_buffers(DrawablePtr pDraw, int *width, int *height,
&& (pDraw->height == pPriv->height)
&& (pPriv->serialNumber == DRI2DrawableSerial(pDraw));
-buffers = malloc((count + 1) * sizeof(buffers[0]));
+buffers = calloc((count + 1), sizeof(buffers[0]));
for (i = 0; i < count; i++) {
const unsigned attachment = *(attachments++);
--
1.7.2.2
___
xorg@lists.freedesktop.org: X.Org support
Archives: http://lists.freedesktop.org/archives/xorg
Info: http://lists.freedesktop.org/mailman/listinfo/xorg
Your subscription address: arch...@mail-archive.com
Re: libxkbcommon build system updates
On Sun, Nov 28, 2010 at 9:46 AM, Jan Engelhardt wrote: > > The following changes since commit f94a64cc08b47cdbfdfea5b5756340246fc391ed: > > Link with -no-undefined (2010-10-26 14:11:38 -0400) Thanks, those all look good, pulled and pushed. Kristian > are available in the git repository at: > git://dev.medozas.de/libxkbcommon master > > Jan Engelhardt (5): > build: use AC_CONFIG_MACRO_DIR as per libtoolize warning > build: use proper quoting in autogen.sh > build: run autoupdate > xkbscan: resolve build warning/rpmlint error > Update .gitignore > > .gitignore | 34 +- > Makefile.am | 2 ++ > autogen.sh | 8 > configure.ac | 8 +--- > m4/.gitignore | 2 ++ > src/xkbcomp/parseutils.h | 4 ++-- > src/xkbcomp/xkbscan.l | 2 +- > 7 files changed, 33 insertions(+), 27 deletions(-) > create mode 100644 m4/.gitignore > ___ xorg@lists.freedesktop.org: X.Org support Archives: http://lists.freedesktop.org/archives/xorg Info: http://lists.freedesktop.org/mailman/listinfo/xorg Your subscription address: arch...@mail-archive.com
Re: [PATCH] os: Don't write the reply that overflows output buffer
On Mon, Aug 2, 2010 at 2:36 PM, Keith Packard wrote: > On Mon, 2 Aug 2010 11:29:55 -0400, Kristian Høgsberg > wrote: >> >> Here's the other approach for fixing damage vs flush client. In writing the >> commit message, it certainly does feel like damage should just be fixed to >> not reply on that behaviour. And writing the patch, I realize that the >> optimization is not just about saving a syscall, but also about avoiding >> having to realloc the output buffer to hold a request that's bigger than >> BUFSIZE (hello, XkbSendMap()). The current code only does that when it >> fails to write the output buffer + extra request and the remaining bytes >> don't fit in a standard sized buffer. > > This doesn't really fix the issue as we're potentially sending a stream > of events from the damage extension. We'd have to buffer all of them > until FlushAllOutput, I think. So from IRC it sounds like the post-op damage reporting code isn't too horrible after all. If you're OK with changing damageext to use the post-op reporting, please add a Reviewed-by and I'll send a pull request. Kristian ___ xorg@lists.freedesktop.org: X.Org support Archives: http://lists.freedesktop.org/archives/xorg Info: http://lists.freedesktop.org/mailman/listinfo/xorg Your subscription address: arch...@mail-archive.com
Re: [PATCH] os: Don't write the reply that overflows output buffer
2010/8/2 Keith Packard : > On Mon, 2 Aug 2010 19:58:47 -0400, Kristian Høgsberg > wrote: > >> And if I don't have to touch low level protocol code, I'd >> rather not. > > Scaredy cat. Oh, I'm mostly just lazy. Cleaning up X code that works isn't my kind of fun. Kristian ___ xorg@lists.freedesktop.org: X.Org support Archives: http://lists.freedesktop.org/archives/xorg Info: http://lists.freedesktop.org/mailman/listinfo/xorg Your subscription address: arch...@mail-archive.com
Re: [PATCH] os: Don't write the reply that overflows output buffer
On Mon, Aug 2, 2010 at 2:36 PM, Keith Packard wrote: > On Mon, 2 Aug 2010 11:29:55 -0400, Kristian Høgsberg > wrote: >> >> Here's the other approach for fixing damage vs flush client. In writing the >> commit message, it certainly does feel like damage should just be fixed to >> not reply on that behaviour. And writing the patch, I realize that the >> optimization is not just about saving a syscall, but also about avoiding >> having to realloc the output buffer to hold a request that's bigger than >> BUFSIZE (hello, XkbSendMap()). The current code only does that when it >> fails to write the output buffer + extra request and the remaining bytes >> don't fit in a standard sized buffer. > > This doesn't really fix the issue as we're potentially sending a stream > of events from the damage extension. We'd have to buffer all of them > until FlushAllOutput, I think. Ah, yes, I see the loop over the damage region boxes, should've checked that. It was pretty fragile to rely on that guarantee anyway; if we want the rendering to happen before the event is sent, we should do the rendering before we send the event. > It is a nice cleanup though; let's look at this post-1.9 separately. Maybe - the old code had the benefit of typically not having to allocate a big buffer for big events. It would just add the big buffer that overflowed the protocol buffer to the writev request and avoid the copy. And if I don't have to touch low level protocol code, I'd rather not. Kristian ___ xorg@lists.freedesktop.org: X.Org support Archives: http://lists.freedesktop.org/archives/xorg Info: http://lists.freedesktop.org/mailman/listinfo/xorg Your subscription address: arch...@mail-archive.com
[PATCH] os: Don't write the reply that overflows output buffer
The io code has a small optimization where it will add the reply buffer
that causes the client output buffer to overflow to the writev that
flushes the buffer. The idea is that since we're doing a syscall anyway
we may as well use writev and write as much as possible to the client.
However, this optimization significantly complicates the FlushClient()
implementation and worse, it means that the server can not reply on
WriteToClient() to buffer a reply until after a request has been
processed and control returns to Dispatch(). This matters for the damage
extension, which writes events to the client buffer before the
driver sees the rendering request, but wants to make sure that once the
output buffer is flushed, all the rendering has been submitted.
Signed-off-by: Kristian Høgsberg
---
Here's the other approach for fixing damage vs flush client. In writing the
commit message, it certainly does feel like damage should just be fixed to
not reply on that behaviour. And writing the patch, I realize that the
optimization is not just about saving a syscall, but also about avoiding
having to realloc the output buffer to hold a request that's bigger than
BUFSIZE (hello, XkbSendMap()). The current code only does that when it
fails to write the output buffer + extra request and the remaining bytes
don't fit in a standard sized buffer.
Kristian
os/connection.c |2 +-
os/io.c | 137 +++---
os/osdep.h |4 +-
3 files changed, 40 insertions(+), 103 deletions(-)
diff --git a/os/connection.c b/os/connection.c
index 77910be..2248edc 100644
--- a/os/connection.c
+++ b/os/connection.c
@@ -1037,7 +1037,7 @@ CloseDownConnection(ClientPtr client)
CallCallbacks(&FlushCallback, NULL);
if (oc->output && oc->output->count)
- FlushClient(client, oc, (char *)NULL, 0);
+ FlushClient(client, oc);
#ifdef XDMCP
XdmcpCloseDisplay(oc->fd);
#endif
diff --git a/os/io.c b/os/io.c
index e2df2e3..86e92c9 100644
--- a/os/io.c
+++ b/os/io.c
@@ -85,7 +85,7 @@ CallbackListPtr ReplyCallback;
CallbackListPtr FlushCallback;
static ConnectionInputPtr AllocateInputBuffer(void);
-static ConnectionOutputPtr AllocateOutputBuffer(void);
+static ConnectionOutputPtr AllocateOutputBuffer(int min_size);
/* check for both EAGAIN and EWOULDBLOCK, because some supposedly POSIX
* systems are broken and return EWOULDBLOCK when they should return EAGAIN
@@ -645,7 +645,7 @@ FlushAllOutput(void)
NewOutputPending = TRUE;
}
else
- (void)FlushClient(client, oc, (char *)NULL, 0);
+ (void)FlushClient(client, oc);
}
}
#else /* WIN32 */
@@ -665,7 +665,7 @@ FlushAllOutput(void)
NewOutputPending = TRUE;
}
else
- (void)FlushClient(client, oc, (char *)NULL, 0);
+ (void)FlushClient(client, oc);
}
XFD_COPYSET(&newOutputPending, &OutputPending);
#endif /* WIN32 */
@@ -700,7 +700,7 @@ WriteToClient (ClientPtr who, int count, const void *__buf)
{
OsCommPtr oc;
ConnectionOutputPtr oco;
-int padBytes;
+int padBytes, ret;
const char *buf = __buf;
#ifdef DEBUG_COMMUNICATION
Bool multicount = FALSE;
@@ -749,14 +749,35 @@ WriteToClient (ClientPtr who, int count, const void
*__buf)
}
#endif
+padBytes = padlength[count & 3];
+
+if (oco && oco->count + count + padBytes > oco->size)
+{
+ FD_CLR(oc->fd, &OutputPending);
+ if(!XFD_ANYSET(&OutputPending)) {
+ CriticalOutputPending = FALSE;
+ NewOutputPending = FALSE;
+ }
+
+ if (FlushCallback)
+ CallCallbacks(&FlushCallback, NULL);
+
+ ret = FlushClient(who, oc);
+ if (ret < 0)
+ return ret;
+
+ oco = NULL;
+}
+
if (!oco)
{
- if ((oco = FreeOutputs))
+ if (count + padBytes < BUFSIZE && (oco = FreeOutputs))
{
FreeOutputs = oco->next;
}
- else if (!(oco = AllocateOutputBuffer()))
+ else if (!(oco = AllocateOutputBuffer(count + padBytes)))
{
+
if (oc->trans_conn) {
_XSERVTransDisconnect(oc->trans_conn);
_XSERVTransClose(oc->trans_conn);
@@ -768,8 +789,6 @@ WriteToClient (ClientPtr who, int count, const void *__buf)
oc->output = oco;
}
-padBytes = padlength[count & 3];
-
if(ReplyCallback)
{
ReplyInfoRec replyinfo;
@@ -812,19 +831,6 @@ WriteToClient (ClientPtr who, int count, const void *__buf)
}
}
#endif
-if (oco->count + count + padBytes > oco->size)
-{
- FD_CLR(oc->fd, &OutputPending);
- if(!XFD_ANYSET(&OutputPending)) {
- CriticalOutputPending = FALSE;
- NewOutputPending = FA
Re: [PATCH 1/2] Always call the flush callback chain when we might send out damage events
2010/8/2 Keith Packard : > On Mon, 2 Aug 2010 08:24:04 -0400, Kristian Høgsberg > wrote: >> On Sun, Aug 1, 2010 at 3:20 PM, Keith Packard wrote: >> > On Sun, 1 Aug 2010 14:39:46 -0400, Kristian Høgsberg >> > wrote: >> >> Before this gets drowned out in janitorial patches... Keith, do you >> >> want a pull request for this and the damageext patch? Did you have >> >> have a look at the damage change? >> > >> > I read through the change and reviewed the potential impacts on the >> > server. I'm a little concerned about having the semantics of the damage >> > extension accidentally change as the code paths for the post-activity >> > damage region processing. >> >> Half a sentence missing here? > > Yeah, missed the 'are completely different than the pre-activity code path' > >> The patch is not changing the >> semantics, just making sure that the semantics that we effectively >> provide and clients expect is what they get (post rendering >> notification). > > Sure, my concern is that within the damage code itself, the code to > perform post-activity notification is quite convoluted and may easily > introduce subtle changes in the events delivered to applications. I just > don't know; I haven't really reviewed that in depth. OK, I see. Well, it was almost working before, except in the case where the client io buffer overflows and we send out an event before the driver has seen the rendering request. Another way to fix that would be to change the odd semantics of WriteToClient() of also writing the reply that caused the io buffer to overflow to just putting that in the next output buffer instead. It looks to me like that's just an silly little optimization - "we're going to do a writev syscall, might as well add the reply that overflowed in a separate iovec." And nothing should rely on that behaviour, there's no way the caller can know whether the reply is going to be placed in the buffer or cause a flush. And removing the __extraBuf argument from FlushClient would make that code a little simpler too. Kristian >> Yup, it is the kind of change that requires a bit of review. > > I'll merge your change and then probably spend some time looking at the > damage code and seeing if I can't clean it up a bit; right now it's > messy with lots of different options about when and how the callbacks > are made. > > Just for fun, I briefly considered having the damage extension use the > pre-activity mode and simply pend all output to the client until the > FlushAllOutput call from the main loop. That would have been a > smaller change in the code paths than your patch, but would expose the X > server to potentially buffering a fairly hefty set of events, which > didn't seem like a great plan -- in case of malloc failure in that code > path, the client is disconnected from the X server... > > -- > keith.pack...@intel.com > ___ xorg@lists.freedesktop.org: X.Org support Archives: http://lists.freedesktop.org/archives/xorg Info: http://lists.freedesktop.org/mailman/listinfo/xorg Your subscription address: arch...@mail-archive.com
Re: [PATCH 1/2] Always call the flush callback chain when we might send out damage events
On Sun, Aug 1, 2010 at 3:20 PM, Keith Packard wrote: > On Sun, 1 Aug 2010 14:39:46 -0400, Kristian Høgsberg > wrote: >> Before this gets drowned out in janitorial patches... Keith, do you >> want a pull request for this and the damageext patch? Did you have >> have a look at the damage change? > > I read through the change and reviewed the potential impacts on the > server. I'm a little concerned about having the semantics of the damage > extension accidentally change as the code paths for the post-activity > damage region processing. Half a sentence missing here? The patch is not changing the semantics, just making sure that the semantics that we effectively provide and clients expect is what they get (post rendering notification). The order of rendering vs damage reporting doesn't determine the order in which the client sees things, the order in which we flush protocol buffers and submit batch buffers does. And we can't possibly provide pre-rendering notification to clients, that can only work as a callback in the server. > And, I'd love to have that code 'fixed' so > that there wasn't additional region processing needed. So, it's not my > favorite change, but anything additional seems like a riskier > proposition for 1.9. > > So, I was planning to merge the damage change along with your other > fixes for 1.9. It'd be lovely to get other people to take another look > at the damage code to see if they can identify potential issues with > your change. Yup, it is the kind of change that requires a bit of review. Kristian ___ xorg@lists.freedesktop.org: X.Org support Archives: http://lists.freedesktop.org/archives/xorg Info: http://lists.freedesktop.org/mailman/listinfo/xorg Your subscription address: arch...@mail-archive.com
Re: [PATCH 1/2] Always call the flush callback chain when we might send out damage events
Before this gets drowned out in janitorial patches... Keith, do you
want a pull request for this and the damageext patch? Did you have
have a look at the damage change?
Kristian
2010/7/29 Kristian Høgsberg :
> We were missing the callback in a couple of places. Drivers may use
> the flush callback to submit batched up rendering before events (for
> example, damage events) are sent out, to ensure that the rendering
> has happend when the client receives the event.
>
> Signed-off-by: Kristian Høgsberg
> ---
> os/connection.c | 3 +++
> os/io.c | 4
> 2 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/os/connection.c b/os/connection.c
> index c143fb6..77910be 100644
> --- a/os/connection.c
> +++ b/os/connection.c
> @@ -1033,6 +1033,9 @@ CloseDownConnection(ClientPtr client)
> {
> OsCommPtr oc = (OsCommPtr)client->osPrivate;
>
> + if (FlushCallback)
> + CallCallbacks(&FlushCallback, NULL);
> +
> if (oc->output && oc->output->count)
> FlushClient(client, oc, (char *)NULL, 0);
> #ifdef XDMCP
> diff --git a/os/io.c b/os/io.c
> index b5f98b7..e2df2e3 100644
> --- a/os/io.c
> +++ b/os/io.c
> @@ -819,6 +819,10 @@ WriteToClient (ClientPtr who, int count, const void
> *__buf)
> CriticalOutputPending = FALSE;
> NewOutputPending = FALSE;
> }
> +
> + if (FlushCallback)
> + CallCallbacks(&FlushCallback, NULL);
> +
> return FlushClient(who, oc, buf, count);
> }
>
> --
> 1.7.2
>
> ___
> xorg@lists.freedesktop.org: X.Org support
> Archives: http://lists.freedesktop.org/archives/xorg
> Info: http://lists.freedesktop.org/mailman/listinfo/xorg
> Your subscription address: k...@bitplanet.net
___
xorg@lists.freedesktop.org: X.Org support
Archives: http://lists.freedesktop.org/archives/xorg
Info: http://lists.freedesktop.org/mailman/listinfo/xorg
Your subscription address: arch...@mail-archive.com
[PATCH 1/2] Always call the flush callback chain when we might send out damage events
We were missing the callback in a couple of places. Drivers may use
the flush callback to submit batched up rendering before events (for
example, damage events) are sent out, to ensure that the rendering
has happend when the client receives the event.
Signed-off-by: Kristian Høgsberg
---
os/connection.c |3 +++
os/io.c |4
2 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/os/connection.c b/os/connection.c
index c143fb6..77910be 100644
--- a/os/connection.c
+++ b/os/connection.c
@@ -1033,6 +1033,9 @@ CloseDownConnection(ClientPtr client)
{
OsCommPtr oc = (OsCommPtr)client->osPrivate;
+if (FlushCallback)
+ CallCallbacks(&FlushCallback, NULL);
+
if (oc->output && oc->output->count)
FlushClient(client, oc, (char *)NULL, 0);
#ifdef XDMCP
diff --git a/os/io.c b/os/io.c
index b5f98b7..e2df2e3 100644
--- a/os/io.c
+++ b/os/io.c
@@ -819,6 +819,10 @@ WriteToClient (ClientPtr who, int count, const void *__buf)
CriticalOutputPending = FALSE;
NewOutputPending = FALSE;
}
+
+ if (FlushCallback)
+ CallCallbacks(&FlushCallback, NULL);
+
return FlushClient(who, oc, buf, count);
}
--
1.7.2
___
xorg@lists.freedesktop.org: X.Org support
Archives: http://lists.freedesktop.org/archives/xorg
Info: http://lists.freedesktop.org/mailman/listinfo/xorg
Your subscription address: arch...@mail-archive.com
[PATCH 2/2] Set DamageSetReportAfterOp to true for the damage extension
Change the damage extension reporter to queue up events after we chain to the wrapped functions. Damage events are typically sent out after the rendering happens anyway, since we submit batch buffers from the flush callback chain and then flush client io buffers. Compositing managers relie on this order, and there is no way we could reliably provide damage events to clients before the rendering happens anyway. By queueing up the damage events before the rendering happens, there's a risk that the client io buffer may overflow and send the damage events to the client before the driver has even seen the rendering request. Reporting damage events after the rendering fixes this corner case and better corresponds with how we expect this to work. Signed-off-by: Kristian Høgsberg --- damageext/damageext.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/damageext/damageext.c b/damageext/damageext.c index f5265dd..b4bb478 100644 --- a/damageext/damageext.c +++ b/damageext/damageext.c @@ -217,6 +217,7 @@ ProcDamageCreate (ClientPtr client) if (!AddResource (stuff->damage, DamageExtType, (pointer) pDamageExt)) return BadAlloc; +DamageSetReportAfterOp (pDamageExt->pDamage, TRUE); DamageRegister (pDamageExt->pDrawable, pDamageExt->pDamage); if (pDrawable->type == DRAWABLE_WINDOW) -- 1.7.2 ___ xorg@lists.freedesktop.org: X.Org support Archives: http://lists.freedesktop.org/archives/xorg Info: http://lists.freedesktop.org/mailman/listinfo/xorg Your subscription address: arch...@mail-archive.com
Re: [PATCH] Call flush callback chain when we flush client buffer due to overflow
2010/7/29 Dave Airlie : >>> Also, there is one more call to FlushClient in CloseDownConnection; any >>> reason we shouldn't be invoking the FlushCallback there too? More from a >>> sense of completeness than a chance that it's going to actually matter. >> >> Yes, that sounds fine. And I just realized that we need to change the >> damageext handler to report damage after, so the batch buffer will >> already have the rendering in it, in case writing the event overflows >> the output buffers. I'll roll these changes into an updated patch and >> resend >> > > Just from memory, we changed the damage reporting order before Yeah, but not for the DamagePtr used by the damage *extension*. The damage module (miext/damage/) has a few internal users, such as sw cursor, shadowfb and others. I'm talking about changing the damage extension reporter (damageext/) to queue up events after we chain to the wrapped functions. So it will only affect clients, but as it is, damage events are typically sent out after the rendering happens anyway (because of the order we normally flush client io buffers and driver batch buffers). Compositing managers relies on this order, and there is no way we could reliably provide damage events to clients before the rendering happens anyway. Kristian ___ xorg@lists.freedesktop.org: X.Org support Archives: http://lists.freedesktop.org/archives/xorg Info: http://lists.freedesktop.org/mailman/listinfo/xorg Your subscription address: arch...@mail-archive.com
Re: [PATCH] Call flush callback chain when we flush client buffer due to overflow
On Thu, Jul 29, 2010 at 7:18 PM, Keith Packard wrote: > On Thu, 29 Jul 2010 18:53:34 -0400, Kristian Høgsberg > wrote: > >> This case was missing and could cause corruption if we flush damage events >> to a client before the driver gets to submit the rendering from the >> callback chain. > > Seems like this is a good candidate for 1.9, yes? Is there a specific > bug that this helps address? It goes with this patch to xf86-video-intel: http://lists.freedesktop.org/archives/intel-gfx/2010-July/007551.html which fixes 28438. > Also, there is one more call to FlushClient in CloseDownConnection; any > reason we shouldn't be invoking the FlushCallback there too? More from a > sense of completeness than a chance that it's going to actually matter. Yes, that sounds fine. And I just realized that we need to change the damageext handler to report damage after, so the batch buffer will already have the rendering in it, in case writing the event overflows the output buffers. I'll roll these changes into an updated patch and resend Kristian ___ xorg@lists.freedesktop.org: X.Org support Archives: http://lists.freedesktop.org/archives/xorg Info: http://lists.freedesktop.org/mailman/listinfo/xorg Your subscription address: arch...@mail-archive.com
[PATCH] Call flush callback chain when we flush client buffer due to overflow
This case was missing and could cause corruption if we flush damage events to a client before the driver gets to submit the rendering from the callback chain. Signed-off-by: Kristian Høgsberg --- os/io.c |4 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/os/io.c b/os/io.c index b5f98b7..e2df2e3 100644 --- a/os/io.c +++ b/os/io.c @@ -819,6 +819,10 @@ WriteToClient (ClientPtr who, int count, const void *__buf) CriticalOutputPending = FALSE; NewOutputPending = FALSE; } + + if (FlushCallback) + CallCallbacks(&FlushCallback, NULL); + return FlushClient(who, oc, buf, count); } -- 1.7.2 ___ xorg@lists.freedesktop.org: X.Org support Archives: http://lists.freedesktop.org/archives/xorg Info: http://lists.freedesktop.org/mailman/listinfo/xorg Your subscription address: arch...@mail-archive.com
[ANNOUNCE] dri2proto 2.3
Here is dri2proto version 2.3. This release adds an event designed by Francisco Jerez to let the server notify clients when the buffers associated with a drawable become invalid. This can happen when the drawable is resized or when DRI2SwapBuffer exchanges front and back buffers. Francisco Jerez (1): Define an event to notify clients about the validity of their buffers. git tag: dri2proto-2.3 http://xorg.freedesktop.org/archive/individual/proto/dri2proto-2.3.tar.bz2 MD5: 3407b494d5e90d584c9af52aa8f9f028 dri2proto-2.3.tar.bz2 SHA1: 6c9e2c822e777db435a43616067f8dd21cc00a53 dri2proto-2.3.tar.bz2 http://xorg.freedesktop.org/archive/individual/proto/dri2proto-2.3.tar.gz MD5: 2087269416782cff56f9b22d1449c192 dri2proto-2.3.tar.gz SHA1: f1fd760867bffd91f130d45cda38dfdfb4c54a86 dri2proto-2.3.tar.gz ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
[ANNOUNCE] dri2proto 2.0
Here's 2.0 release of dri2proto, at long last. The 1.99.3 release has been in use for quite a while and getting a 2.0 release out dropped of the todo list. Well here it is. Just as a reminder, the 1.x releases were never put in use and the 2.0 branch is the first release that is used by a server release. Julien Cristau (1): Distribute the protocol documentation Kristian Høgsberg (1): Bump to 2.0 and release Paulo Cesar Pereira de Andrade (1): Janitor: Correct make distcheck and dont distribute autogen.sh git tag: dri2proto-2.0 http://xorg.freedesktop.org/archive/individual/proto/dri2proto-2.0.tar.bz2 MD5: 26f6515322ec50691584ac74291b4254 dri2proto-2.0.tar.bz2 SHA1: cb6342ea88f57f7d0023abdf9a3ff6000f8e7a0f dri2proto-2.0.tar.bz2 http://xorg.freedesktop.org/archive/individual/proto/dri2proto-2.0.tar.gz MD5: 04020a0749d4ed58f090cad4e1e6e117 dri2proto-2.0.tar.gz SHA1: 4f82e4d24469a1c12d120b4b94009d8459a76eb6 dri2proto-2.0.tar.gz ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [RFC] glx: fix DRI2 memory leak
On Wed, Mar 25, 2009 at 10:21:51AM -0700, Jesse Barnes wrote:
> In trying to track down the memory leak in 20704, I found that the DRI2
> drawable destroy routine doesn't seem to get called when new drawables
> are created and old ones destroyed (as in the glViewport case in the
> bug).
Ok, sorry for taking so long on this. Here's the patch I'd like to push
to fix the problem - let me know if it works alright.
thanks,
Kristian
>From 2f2538d20291ead2187087228a391dade1a0434f Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Kristian=20H=C3=B8gsberg?=
Date: Thu, 9 Apr 2009 13:16:37 -0400
Subject: [PATCH] glx: Fix drawable private leak on destroy
When a drawable goes away, we don't destroy the GLX drawable in full,
since it may be current for a context. This means that when the drawable
is destroyed in full later, the backend doesn't get a chance to
destroy resources associated with the drawable (the DRI2Drawable).
With this patch, we destroy the GLX drawable in full when it goes away
and then track down all contexts that reference it and NULL their
pointers.
---
glx/Makefile.am|1 -
glx/glxcmds.c | 44 --
glx/glxdrawable.h |5 ---
glx/glxdri.c |2 +
glx/glxdri2.c |2 +
glx/glxdriswrast.c |2 +
glx/glxext.c | 42 +
glx/glxext.h |1 +
glx/glxutil.c | 74
glx/glxutil.h |9 +-
10 files changed, 67 insertions(+), 115 deletions(-)
delete mode 100644 glx/glxutil.c
diff --git a/glx/Makefile.am b/glx/Makefile.am
index 2537db8..a23ae0a 100644
--- a/glx/Makefile.am
+++ b/glx/Makefile.am
@@ -80,7 +80,6 @@ libglx_la_SOURCES = \
glxscreens.c \
glxscreens.h \
glxserver.h \
-glxutil.c \
glxutil.h \
render2.c \
render2swap.c \
diff --git a/glx/glxcmds.c b/glx/glxcmds.c
index e21f0f0..86e8dd8 100644
--- a/glx/glxcmds.c
+++ b/glx/glxcmds.c
@@ -193,18 +193,9 @@ validGlxDrawable(ClientPtr client, XID id, int type, int
access_mode,
void
__glXContextDestroy(__GLXcontext *context)
{
-if (!context->isDirect) {
- if (context->drawPriv)
- __glXUnrefDrawable(context->drawPriv);
- if (context->readPriv)
- __glXUnrefDrawable(context->readPriv);
- context->drawPriv = NULL;
- context->readPriv = NULL;
-}
__glXFlushContextCache();
}
-
static void __glXdirectContextDestroy(__GLXcontext *context)
{
__glXContextDestroy(context);
@@ -320,6 +311,8 @@ DoCreateContext(__GLXclientState *cl, GLXContextID gcId,
glxc->isDirect = isDirect;
glxc->renderMode = GL_RENDER;
+__glXAddToContextList(glxc);
+
return Success;
}
@@ -639,10 +632,6 @@ DoMakeCurrent(__GLXclientState *cl,
}
__glXFlushContextCache();
if (!prevglxc->isDirect) {
- if (prevglxc->drawPriv)
- __glXUnrefDrawable(prevglxc->drawPriv);
- if (prevglxc->readPriv)
- __glXUnrefDrawable(prevglxc->readPriv);
prevglxc->drawPriv = NULL;
prevglxc->readPriv = NULL;
}
@@ -662,8 +651,6 @@ DoMakeCurrent(__GLXclientState *cl,
}
glxc->isCurrent = GL_TRUE;
- __glXRefDrawable(glxc->drawPriv);
- __glXRefDrawable(glxc->readPriv);
}
if (prevglxc) {
@@ -1090,6 +1077,33 @@ int __glXDisp_GetFBConfigsSGIX(__GLXclientState *cl,
GLbyte *pc)
return DoGetFBConfigs(cl, req->screen);
}
+GLboolean
+__glXDrawableInit(__GLXdrawable *drawable,
+ __GLXscreen *screen, DrawablePtr pDraw, int type,
+ XID drawId, __GLXconfig *config)
+{
+drawable->pDraw = pDraw;
+drawable->type = type;
+drawable->drawId = drawId;
+drawable->config = config;
+drawable->eventMask = 0;
+
+return GL_TRUE;
+}
+
+void
+__glXDrawableRelease(__GLXdrawable *drawable)
+{
+ScreenPtr pScreen = drawable->pDraw->pScreen;
+
+switch (drawable->type) {
+case GLX_DRAWABLE_PIXMAP:
+case GLX_DRAWABLE_PBUFFER:
+ (*pScreen->DestroyPixmap)((PixmapPtr) drawable->pDraw);
+ break;
+}
+}
+
static int
DoCreateGLXDrawable(ClientPtr client, __GLXscreen *pGlxScreen, __GLXconfig
*config,
DrawablePtr pDraw, XID glxDrawableId, int type)
diff --git a/glx/glxdrawable.h b/glx/glxdrawable.h
index b64ff35..0215b3b 100644
--- a/glx/glxdrawable.h
+++ b/glx/glxdrawable.h
@@ -67,11 +67,6 @@ struct __GLXdrawable {
*/
__GLXconfig *config;
-/*
-** reference count
-*/
-int refCount;
-
GLenum target;
GLenum format;
diff --git a/glx/glxdri.c b/glx/glxdri.c
index cc6d939..eeac7fc 100644
--- a/glx/glxdri.c
+++ b/glx/glxdri.c
@@ -238,6 +238,8 @@ __glXDRIdrawableDestroy(__GLXdrawable *drawable)
__glXleaveServer(GL_FALSE);
}
+__glXDrawableRelease(drawable);
+
xfree(private);
}
diff --git a/glx/glxdri2.c b/glx/glxdri2.c
index c896536
Re: [RFC] glx: fix DRI2 memory leak
2009/4/1 Michel Dänzer :
> On Mon, 2009-03-30 at 13:04 +0800, Shuang He wrote:
>> Shuang He wrote:
>> > Jesse Barnes wrote:
>> > > On Fri, 27 Mar 2009 13:20:25 -0400
>> > > Kristian Høgsberg wrote:
>> > > >
>> > > > The leak is (as you pointed out before) because we NULL the pDraw
>> > > > pointer before calling the backend destructor, which then can't unref
>> > > > the DRI2Drawable, which we then leak. You had the right fix in the
>> > > > originial post, we just need to not touch glxPriv after
>> > > > __glXUnrefDrawable if it got destroyed. I suggest this change to
>> > > > DrawableGone in irc:
>> > > >
>> > > > refcount = glxPriv->refcount;
>> > > > __glXUnrefDrawable(glxPriv);
>> > > > if (refcount > 1) {
>> > > > glxPriv->pDraw = NULL;
>> > > > glxPriv->drawId = 0;
>> > > > }
>> > > >
>> > >
>> > > Yep, that seems to work too... Magnus or Vasily can you guys confirm?
>> > >
>> > Memory leak problem seems resolved, but I still see X crash with:
>> > (gdb) bt
>> > #0 0xb7fd5424 in __kernel_vsyscall ()
>> > #1 0x03155660 in raise () from /lib/libc.so.6
>> > #2 0x03157028 in abort () from /lib/libc.so.6
>> > #3 0x031925bd in __libc_message () from /lib/libc.so.6
>> > #4 0x031987e4 in malloc_printerr () from /lib/libc.so.6
>> > #5 0x0319c441 in _int_realloc () from /lib/libc.so.6
>> > #6 0x0319d176 in realloc () from /lib/libc.so.6
>> > #7 0x08131002 in Xrealloc (ptr=0x6, amount=0) at utils.c:1133
>> > #8 0x0806d10b in dixAllocatePrivate (privates=0x91487c8, key=0xb7e90a3c)
>> > at privates.c:129
>> > #9 0x0806d1cc in dixSetPrivate (privates=0x91487c8, key=0xb7e90a3c,
>> > val=0x0)
>> > at privates.c:193
>> > #10 0xb7e8eca1 in DRI2DestroyDrawable (pDraw=0x91487b0) at dri2.c:218
>> > #11 0xb7eee668 in __glXDRIdrawableDestroy (drawable=0x9205ff0) at
>> > glxdri2.c:108
>> > #12 0xb7ee49bb in __glXUnrefDrawable (glxPriv=0x9205ff0) at glxutil.c:58
>> > #13 0xb7ee3183 in DrawableGone (glxPriv=0x9205ff0, xid=12583326)
>> > at glxext.c:131
>> > #14 0x0806efdc in FreeResource (id=12583326, skipDeleteFuncType=0)
>> > at resource.c:561
>> > #15 0xb7edffa6 in DoDestroyDrawable (cl=,
>> > glxdrawable=12583326, type=1) at glxcmds.c:1225
>> > #16 0xb7ee340a in __glXDispatch (client=0x8d79db8) at glxext.c:523
>> > #17 0x080874cf in Dispatch () at dispatch.c:437
>> > ---Type to continue, or q to quit---
>> > #18 0x0806c69d in main (argc=2, argv=0xbf9d2754, envp=Cannot access memory
>> > at
>> > address 0xbde
>>
>> Just debug a bit, check out this series of calls in DRI2DestroyDrawable when
>> X
>> crashed:
>> in (*ds->DestroyBuffers)(pDraw, pPriv->buffers, pPriv->bufferCount);
>> Xfree: free(0x9eef330)
>> Xfree: free(0x9eeef20)
>> Xfree: free(0x9efdde0)
>> Xfree: free(0x9efce08)
>> Xfree: free(0x9eee8b0)
>> Xrealloc: ptr = 0x9efaf20
>> Xrealloc: amount = 384
>> Xfree: free(0x9efcd18)
>> Xfree: free(0x9ef8468)
>> Xrealloc: ptr = 0x9efa278
>> Xrealloc: amount = 384
>> Xfree: free(0x9efcd18)
>> Xfree: free(0x9ef9808)
>> Xfree: free(0x9eeef38)
>> Xfree: free(0x9efa648)
>> Xfree: free(0x9efd788)
>> in dixSetPrivate(&pPixmap->devPrivates, dri2PixmapPrivateKey, NULL);
>> Xrealloc: ptr = 0x9efce08
>> Xrealloc: amount = 512
>>
>> So dixSetPrivate is trying to realloc memory at 0x9efce08, which is alreay
>> freed in DetroyBuffers. So maybe we should also do this: [...]
>
> The real problem was pointed out by Pierre Willenbrock: If
> glxPriv->pDraw is a pixmap, DrawableGone() destroys it, then later
> DRI2DestroyDrawable dereferences it... The patch below seems to work
> here - at least it hasn't crashed in a couple of hours, not sure about
> the leak yet.
>
> Note that unless I'm missing something, it might still be possible for
> this to occur with windows if the underlying DrawableRec is freed before
> this code is reached.
>
> Also, I don't really understand the logic behind clearing glxPriv->pDraw
> and ->drawId here. In particular, I'm not sure DrawableGone will ever be
> called with glxPriv->refCount > 1. If it won't, this change makes the
> assignments unreachabl
Re: [RFC] glx: fix DRI2 memory leak
On Thu, Mar 26, 2009 at 6:33 PM, Jesse Barnes wrote:
> Ok, I think this is where the leak was:
> __glXUnrefDrawable->DrawableGone->__glXUnrefDrawable. This sequence
> meant the glxPriv would stay around (since it was used), but
> DrawableGone had generally already disposed of the pDrawable before the
> call to Unref, so we never got into DRI2DestroyBuffers, thus the leak.
>
> The loop seems broken to me. It *looks* like DrawableGone should be
> the real free routine, only called when a drawable really is free, so
> I've fixed up the code to conform to that. This means removing the GLX
> priv frees from DRI and DRI2 routines and putting them here and using
> the GLX unref routine everwhere (since it will only free the resource
> when the refcount reaches 0).
>
> Any thoughts? I'd appreciate some more testers too... I'm not quite
> sure about the generic DoDestroyDrawable change, but if the refcount is
> always 1 there anyway it shouldn't make a difference and seems more
> correct.
The __GLXdrawable is a reference counted object, and the glx code
references it in a couple of places: when there's an X resource
pointing to it (a GLXWindow, GLXPixmap or GLXPbuffer) and when it's
the current drawable or readable for a context. The __GLXdrawable is
allocated by the backend in use (dri, dri2 or swrast) and must be
freed by the same backend (don't mix alloc and free across abstraction
barriers). We unref the __GLXdrawable when we either set a different
drawable/readable and when the X resource goes away.
The leak is (as you pointed out before) because we NULL the pDraw
pointer before calling the backend destructor, which then can't unref
the DRI2Drawable, which we then leak. You had the right fix in the
originial post, we just need to not touch glxPriv after
__glXUnrefDrawable if it got destroyed. I suggest this change to
DrawableGone in irc:
refcount = glxPriv->refcount;
__glXUnrefDrawable(glxPriv);
if (refcount > 1) {
glxPriv->pDraw = NULL;
glxPriv->drawId = 0;
}
Did you try that?
cheers,
Kristian
> Thanks,
> --
> Jesse Barnes, Intel Open Source Technology Center
>
>
> diff --git a/glx/glxcmds.c b/glx/glxcmds.c
> index ab2d91b..08b866d 100644
> --- a/glx/glxcmds.c
> +++ b/glx/glxcmds.c
> @@ -1222,7 +1222,7 @@ static int DoDestroyDrawable(__GLXclientState *cl, XID
> glxdrawable, int type)
> }
> }
>
> - FreeResource(glxdrawable, FALSE);
> + __glXUnrefDrawable(pGlxDraw);
>
> return Success;
> }
> diff --git a/glx/glxdri.c b/glx/glxdri.c
> index 223b06e..253d868 100644
> --- a/glx/glxdri.c
> +++ b/glx/glxdri.c
> @@ -237,8 +237,6 @@ __glXDRIdrawableDestroy(__GLXdrawable *drawable)
> serverClient, drawable->pDraw);
> __glXleaveServer(GL_FALSE);
> }
> -
> - xfree(private);
> }
>
> static GLboolean
> diff --git a/glx/glxdri2.c b/glx/glxdri2.c
> index 4e76c71..4fdc7df 100644
> --- a/glx/glxdri2.c
> +++ b/glx/glxdri2.c
> @@ -106,8 +106,6 @@ __glXDRIdrawableDestroy(__GLXdrawable *drawable)
> * aready have taken care of this, so only call if pDraw isn't NULL. */
> if (drawable->pDraw != NULL)
> DRI2DestroyDrawable(drawable->pDraw);
> -
> - xfree(private);
> }
>
> static void
> diff --git a/glx/glxext.c b/glx/glxext.c
> index c882372..0b6c752 100644
> --- a/glx/glxext.c
> +++ b/glx/glxext.c
> @@ -120,6 +120,8 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
> {
> ScreenPtr pScreen = glxPriv->pDraw->pScreen;
>
> + glxPriv->destroy(glxPriv);
> +
> switch (glxPriv->type) {
> case GLX_DRAWABLE_PIXMAP:
> case GLX_DRAWABLE_PBUFFER:
> @@ -129,7 +131,7 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
>
> glxPriv->pDraw = NULL;
> glxPriv->drawId = 0;
> - __glXUnrefDrawable(glxPriv);
> + xfree(glxPriv);
>
> return True;
> }
> diff --git a/glx/glxutil.c b/glx/glxutil.c
> index bc71087..61323f5 100644
> --- a/glx/glxutil.c
> +++ b/glx/glxutil.c
> @@ -52,11 +52,9 @@ void
> __glXUnrefDrawable(__GLXdrawable *glxPriv)
> {
> glxPriv->refCount--;
> - if (glxPriv->refCount == 0) {
> + if (glxPriv->refCount == 0)
> /* remove the drawable from the drawable list */
> FreeResourceByType(glxPriv->drawId, __glXDrawableRes, FALSE);
> - glxPriv->destroy(glxPriv);
> - }
> }
>
> GLboolean
>
___
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg
Re: Google / X.Org Summer of Code 2009: Ideas Wanted
On Fri, Mar 13, 2009 at 2:55 PM, Dan Nicholson wrote: > On Thu, Mar 12, 2009 at 11:51 PM, Donnie Berkholz > wrote: >> >> - merging xkbcomp into the server > > I'm been working on this in my spare time as per Daniel's advice to > create a libxkbcommon shared library. If anyone wants any more details > on what I've been doing there, let me know. Right now I'm getting > familiar with the xkbcomp parser/compiler and figuring out how to > expose it as library. And, of course, finding out how much XKB API > would need to be duplicated to not drag in Xlib datatypes. It's not > that far along, but I have a pretty good idea what I want to do with > it. Cool, I'm interested. I've been thinking of doing this for wayland and X, and one thing I wanted to avoid was linking to libX11. That's only to call XStringToKeysym, right? We should just copy the files over and include the hash table in libxkbcomp, I think. If you have a git repo, can you put it up somewhere? thanks, Kristian ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [ANNOUNCE] xf86-video-intel 2.6.2
On Sat, Feb 28, 2009 at 12:21 AM, Eric Anholt wrote: ... > I think the problem here was the DRI2 tiling fix, which was great for > the 915-class 3D performance regression but bad for 915-class 2D. I've > pushed a fix to master that should help. If it does, I'll try to get a > 2.6.3 out soon. > > commit 5bfd73cd31ba197a62f549cdbad1a1270b571027 > Author: Eric Anholt > Date: Fri Feb 27 19:09:49 2009 -0800 > > Only allocate pixmaps aligned for tiling when requested by DRI2 GetBuffers. > > This saves massive quantities of memory on pre-965 since the DRI2 tiling > enable caused the minimum size of any pixmap to be 1MB. Excellent, thanks Eric, Kristian ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [PATCH] glx: Replace broken GLX visual setup with a fixed "all" mode.
On Sun, Feb 8, 2009 at 7:00 AM, Eric Anholt wrote: > With trying to match depths so that you didn't end up with a depth 24 > fbconfig for the 32-bit composite visual, I broke the alpha bits on the depth > 24 X visual, which angered other applications. But in fixing that, the > pickFBconfigs code for "minimal" also could end up breaking GLX visuals if > the same FBconfig was chosen for more than one X visual. > We have no reason to not expose as many visuals as possible, but the old > "all" mode didn't match any existing X visuals to GLX visuals, so normal > GL apps didn't work at all. > > Instead, replace it with a simple combination of the two modes: Create GLX > visuals by picking unique FBconfigs with as many features as possible for > each X visual in order. Then, for all remaining FBconfigs that are > appropriate for display, add a corresponding X and GLX visual. > > This gets all applications (even ones that aren't smart enough to do > FBconfigs) > get all the options to get the visual configuration they want. The only > potential downside is that the composite ARGB visual is unique and gets a > nearly full-featured GLX visual (except that the root visual might have taken > the tastiest FBconfig), which means that a dumb compositing manager could > waste resources. Write compositing managers using FBconfigs instead, please. That's probably fine. The code in question was, erhm a little un-inspired and I never liked the option. Looks good to me. cheers, Kristian ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: Radeon R6xx/R7xx 3D Register Reference Guide
On Mon, Jan 26, 2009 at 7:41 PM, Alex Deucher wrote: > AMD is pleased to announce the release of a 3D register reference > guide for Radeon 6xx/7xx chips. > > The guide will be available in the usual places: > http://www.x.org/docs/AMD/R6xx_3D_Registers.pdf > and > http://ati.amd.com/developer/open_gpu_documentation.html > > If you have any development related questions please ask at > gpudriverdevsupp...@amd.com. Great news, thanks Alex. cheers, Kristian ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: undefined symbol: intel_wait_flips
On Wed, Jan 21, 2009 at 11:14 AM, Vasily Khoruzhick wrote: > Hi, with mesa from git 3D is not hardware accellerated anymore on gma950, > > glxinfo | grep rend > direct rendering: Yes > OpenGL renderer string: Software Rasterizer > > LIBGL_DEBUG=1 glxgears > libGL error: dlopen /usr/lib/dri/i915_dri.so failed (/usr/lib/dri/i915_dri.so: > undefined symbol: intel_wait_flips) > libGL error: unable to load driver: i915_dri.so Oops, my bad, I must have tested the wrong driver when I committed this... :-/ Try pulling again. thanks, Kristian ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
[ANNOUNCE] dri2proto 1.99.3
Here's the 1.99.3 release of dri2proto, required by xserver master and 1.6. cheers, Kristian Kristian Høgsberg (3): Add protocol documentation, update to DRI2CopyRegion request. Bump version to 1.99.2. Bump to 1.99.3 and back out the value bitmask from the CopyRegion request. git tag: dri2proto-1.99.3 http://xorg.freedesktop.org/archive/individual/proto/dri2proto-1.99.3.tar.bz2 MD5: fffbbb3c26368ac63d408e81c51ec374 dri2proto-1.99.3.tar.bz2 SHA1: 0be97cbc1440df50cd950b4b5f36e5fcb44a4ab7 dri2proto-1.99.3.tar.bz2 http://xorg.freedesktop.org/archive/individual/proto/dri2proto-1.99.3.tar.gz MD5: 968284bc6a5a54541603cd15e306cf19 dri2proto-1.99.3.tar.gz SHA1: c4c6314c0212a196f18874fe2d78091524f29160 dri2proto-1.99.3.tar.gz ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
DRI2 Heads up
Hi, Here's a quick, last-minute note on the state of DRI2 before I take off for 2 weeks of vacation. I just pushed the updated protocol+docs to dri2proto and updated mesa, xserver and the dri2 branch of xf86-video-intel with the corresponding changes. What's implemented here is the lowest level of support that we've discussed: none vsync handling or page-flipping idea we've discussed are there yet. What is in place is the basic DRI2CopyRegion request, and more importantly, the mechanism for adding further args and return values to that request in later DRI2 revisions. I think we can merge the dri2 branch to master in xf86-video-intel now, since it still requires an Option "DRI2" in xorg.conf to enable and the APIs and protocol feels like they're in place at this point. But since I don't expect to be back online and help with merging issues until November, I can understand if we want to hold off on that. Either way, please have a look and hopefully we can sort this out next month at least. cheers, Kristian ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
DRI2 Protocol Spec Draft v2
Hi,
Here's an updated version of the DRi2 specification. I've added some
discussion points with ISSUE: in the spec. The new stuff here is the
XChangeWindowAttributes inspired DRI2CopyRegion, that'll let us better
extend it in the future. I clarified and simplified the auth stuff,
dropping the group concept. Also I'm still not convinced that the
swap pipe stuff can't just be an xorg.conf option, or maybe an randr
property on the display (preferred swap pipe or whatever).
And most of all, I'd like to keep the first version simple,
considering that we have a lot of options for extending this as we go.
Release early etc...
Kristian
The DRI2 Extension
Version 2.0
2008-09-04
Kristian Høgsberg
[EMAIL PROTECTED]
Red Hat, Inc
1. Introduction
The DRI2 extension is designed to associate and access auxillary
rendering buffers with an X drawable.
DRI2 is a essentially a helper extension to support implementation of
direct rendering drivers/libraries/technologies.
The main consumer of this extension will be a direct rendering OpenGL
driver, but the DRI2 extension is not designed to be OpenGL specific.
Direct rendering implementations of OpenVG, Xv, cairo and other
graphics APIs should find the functionality exposed by this extension
helpful and hopefully sufficient.
Relation to XF86DRI
1.1. Acknowledgements
Kevin E. Martin <[EMAIL PROTECTED]>
Keith Packard <[EMAIL PROTECTED]>
Eric Anholt <[EMAIL PROTECTED]>
Keith Whitwell <[EMAIL PROTECTED]>
Jerome Glisse <[EMAIL PROTECTED]>
Ian Romanick <[EMAIL PROTECTED]>
Michel Dänzer <[EMAIL PROTECTED]>
â â â â â â
2. DRI2 Concepts
2.1. Attachment points
Stolen from OpenGL FBOs, I guess.
2.2. Kernel rendering manager
This specification assumes a rendering architechture, where an
underlying kernel rendering manager that can provide 32 bit integer
handles to video memory buffers. These handles can be passed between
processes, which, through a direct rendering driver, submit rendering
to the kernel rendering manager, targeting and/or sourcing from these
buffers. This extension provides a means to communicate about such
buffers as associated with an X drawable.
The details of how the a direct rendering driver use the buffer names
and submit the rendering requests is outside the scope of this
specification. However, Appendix B does discuss implementation of
this specification on the Graphics Execution Manager (GEM).
2.3. Request ordering
No ordering between swap buffers and X rendering. X rendering to src
buffers will block if they have a vblank pending.
2.4 Authentication model
The purpose of the DRM authentication scheme is to grant access to the
kernel rendering manager buffers created by the X server if, and only
if, the client has access to the X server. This is achieved in a
three-step protocol:
1) The client gets a token from the kernel rendering manager
that uniquely identifies it. The token is a 32 bit integer.
2) The client passes the token to the X server in the
DRI2Authenticate request.
3) The X server authorizes the client by passing the token to
the kernel rendering manager.
A kernel rendering manager can choose not to implement any
authentication and just allow access to all buffers.
2.5 Rendering to the X front buffer
OpenGL allows the client to render to the front buffer, either by
using a single-buffered configuration or but explicitly setting the
draw buffer to GL_FRONT_LEFT. Not allowed!
The client must ask for a fake front buffer, render to that and then
use DRI2CopyRegion to copy contents back and forth between the fake
front buffer and the real front buffer. When X and direct rendering
to a front buffer is interleaved, it is the responsibility of the
application to synchronize access using glXWaitGL and glXWaitX. A
DRI2 implementation of direct rendering GLX, should use these enty
points to copy contents back and forth to as necessary to ensure
consistent rendering.
â â â â â â
3. Data Types
The server side region support specified in the Xfixes extension
version 2 is used in the CopyRegion request.
â â â â â â
4. Errors
Errors are sent using core X error reports.
Authenticate
The X server failed to authenticate the client.
â â â â â â
5. Protocol Types
DRI2ATTACHMENT { DRI2BufferFrontLeft
DRI2BufferBackLeft
DRI2BufferFrontRight
DRI2BufferBackRight
DRI2BufferDepth
DRI2BufferStencil
DRI2BufferAccum
DRI2BufferFakeFrontLeft
DRI2BufferFakeFrontRight }
These values de
Re: DRI2 Protocol Spec Draft
On Thu, Sep 11, 2008 at 3:49 PM, Jesse Barnes <[EMAIL PROTECTED]> wrote: > On Thursday, September 11, 2008 8:59 am Kristian Høgsberg wrote: >> On Thu, Sep 11, 2008 at 6:40 AM, Michel Dänzer >> >> <[EMAIL PROTECTED]> wrote: >> > On Wed, 2008-09-10 at 14:09 -0400, Kristian Høgsberg wrote: >> >> On Tue, Sep 9, 2008 at 11:46 AM, Keith Packard <[EMAIL PROTECTED]> wrote: >> >> > On Tue, 2008-09-09 at 13:27 +0200, Michel Dänzer wrote: >> >> >> For DRI2CopyRegion, you're leaving it to the DDX driver to pick the >> >> >> CRTC to synchronize to? I'm not sure that'll work too well with >> >> >> overlapping viewports, where the user may want to choose which CRTC >> >> >> to synchronize to for each application. >> >> > >> >> > Yeah, I don't see a good way to avoid this, and the client can always >> >> > pass in 'Automatic' (0) and let the server pick the 'right' one. >> >> >> >> Do we need this? When will the client have a better idea of which >> >> pipe a window is on than the X server? >> > >> > Whenever a window is visible on multiple CRTCs (in particular, think >> > fullscreen video/presentation on laptop panel and beamer), only the user >> > can know which CRTC should be synchronized to. >> >> I don't know that we need to make it this complicated... there is no >> right choice when your window spans two CRTCs. Either you have fancy >> gen-locked hardware in which case it doesn't matter, or your CRTCs >> scan out at different refresh rates, in which case you can't win. If >> the window is completely within one or the other CRTC, the X server >> can pick the right CRTC. > > In the sense that you'd get tearing on at least one output that's true. > However, in Michel's example, the user would probably want to sync to the > beamer not the laptop display, if they were doing a multimedia presentation. > So in some cases there definitely is a "right choice". It may be sufficient > to sync everything to one output though, rather than do things on a > per-client basis; I'm not sure if one is easier than the other, design-wise. Ok, I guess what I want to say is: until we have a good story on how the app is going to figure out which display it wants to sync to, tell this to GLX, I'd like to hold off on putting it in the protocol. We can extend the CopyRegion request easily, so lets add the pipe attribute when we have a way of getting that info from the app. Kristian. ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: DRI2 Protocol Spec Draft
On Thu, Sep 11, 2008 at 6:40 AM, Michel Dänzer <[EMAIL PROTECTED]> wrote: > On Wed, 2008-09-10 at 14:09 -0400, Kristian Høgsberg wrote: >> On Tue, Sep 9, 2008 at 11:46 AM, Keith Packard <[EMAIL PROTECTED]> wrote: >> > On Tue, 2008-09-09 at 13:27 +0200, Michel Dänzer wrote: >> > >> >> For DRI2CopyRegion, you're leaving it to the DDX driver to pick the CRTC >> >> to synchronize to? I'm not sure that'll work too well with overlapping >> >> viewports, where the user may want to choose which CRTC to synchronize >> >> to for each application. >> > >> > Yeah, I don't see a good way to avoid this, and the client can always >> > pass in 'Automatic' (0) and let the server pick the 'right' one. >> >> Do we need this? When will the client have a better idea of which >> pipe a window is on than the X server? > > Whenever a window is visible on multiple CRTCs (in particular, think > fullscreen video/presentation on laptop panel and beamer), only the user > can know which CRTC should be synchronized to. I don't know that we need to make it this complicated... there is no right choice when your window spans two CRTCs. Either you have fancy gen-locked hardware in which case it doesn't matter, or your CRTCs scan out at different refresh rates, in which case you can't win. If the window is completely within one or the other CRTC, the X server can pick the right CRTC. I'm changing the protocol for CopyRegion to be like XChangeWindowAttributes, taking a value mask and a list of values as determined by the mask. This will let us bump the protocol version and add more arguments to the CopyRegion request in a backwards compatible manner. We can then add the CRTC to sync to as an argument later if we find a case where the client really can make a better decision than the server. > On Wed, 2008-09-10 at 14:16 -0400, Kristian Høgsberg wrote: >> On Tue, Sep 9, 2008 at 1:34 PM, Michel Dänzer >> <[EMAIL PROTECTED]> wrote: >> > >> > If the GLX implementation is to use DRI2CopyRegion for synchronization >> > purposes, then the interface of the latter needs to be at least as >> > expressive as the DRM synchronization interfaces currently used by the >> > former. >> >> Could you give a quick summary of what those are and where you think >> DRI2CopyRegion falls short? > > See e.g. intelScheduleSwap() in > mesa/src/mesa/drivers/dri/intel/intel_buffers.c: > > * For the case of missing the target, chooses to accept tearing or >execute the swap during the next vblank period (based on driconf >vblank_mode). > * Chooses the primary or secondary CRTC. > * Needs the returned effective/expected sequence number to know if >the target was hit or missed and to calculate the next target >sequence number. > > The MSC related APIs as in mesa/src/mesa/drivers/dri/common/vblank.c > also need to be consistent with the DRI2CopyRegion sequence numbers. So > I think DRI2 would need to wrap the DRM_IOCTL_WAIT_VBLANK functionality > as well (or be DRM specific, which would be unfortunate - and could > still have issues like mapping the CRTC IDs between the DRM and DRI2 > interfaces). So we need to be able to specify whether to drop a frame or postpone it when we miss the target. I'm not happy about having to return the effective swap sequence number... is there a way we can move the computation of the next target to the server? What is the heurstics there anyway? Hmm... I'll take a look a the code. Kristian ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: DRI2 Protocol Spec Draft
On Wed, Sep 10, 2008 at 5:10 PM, Keith Packard <[EMAIL PROTECTED]> wrote: > On Wed, 2008-09-10 at 14:09 -0400, Kristian Høgsberg wrote: > >> Everybody can talk to the DRM and create >> a token, but only if you can pass it to the server over DRI2 protocol, >> can you authenticate. > > Oh, so the cookie in the protocol is a client identifier of some kind. > > In any case, 32 bits of unique id isn't exactly high security; my > thought was that we should allow the system to use a longer key to avoid > spoofing. No that's why the existing scheme is better, it doesn't rely on random/cryptographical tokens. It just needs to be a unique handle that lets the server identify the right client to authenticate. If you can pass this token to the X server you're authenticated. What better way to establish that than, erh, passing it through protocol? The key point is that the server does the ioctl that authenticates the client. Kristian ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: DRI2 Protocol Spec Draft
On Tue, Sep 9, 2008 at 1:34 PM, Michel Dänzer <[EMAIL PROTECTED]> wrote: > On Tue, 2008-09-09 at 10:11 -0700, Keith Packard wrote: >> On Tue, 2008-09-09 at 18:41 +0200, Michel Dänzer wrote: >> > Generally, if DRI2CopyRegion seriously wants to be useful for >> > synchronization purposes, it probably needs to provide at least the same >> > functionality as the DRM vblank related ioctls. >> >> Certainly the combination of DRI2CopyRegion and DRM should be able to >> provide sufficient synchronization. The question is what portion of this >> task belongs in the extension and what portion belongs in the DRM kernel >> API. > > If the GLX implementation is to use DRI2CopyRegion for synchronization > purposes, then the interface of the latter needs to be at least as > expressive as the DRM synchronization interfaces currently used by the > former. Could you give a quick summary of what those are and where you think DRI2CopyRegion falls short? Kristian ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: DRI2 Protocol Spec Draft
On Tue, Sep 9, 2008 at 11:46 AM, Keith Packard <[EMAIL PROTECTED]> wrote: > On Tue, 2008-09-09 at 13:27 +0200, Michel Dänzer wrote: > >> I think it'd be good if the authentication stuff could be made/kept >> optional or at least not DRM specific. (I'm not sure GEM or the DRM in >> general is within scope of this spec at all) > > I have to admit that I'm not very excited by the existing authentication > protocol. > > What we want is a way to let applications identify themselves with the > kernel and 'prove' that they have permission to access kernel objects. That's what the current XF86DRI scheme is and it's what I've copied. I've updated the spec to drop the group concept, and instead just require that the server authenticates the client into the group it created. > It seems like having the X server return a 'cookie' that the client > could use with the kernel module might make things simpler: > > ┌─── >DRI2Connect >window: WINDOW >type: STRING > ▶ >driver: STRING >device: STRING -- device file name >auth-data: LISTofCARD8 > └─── > >'auth-data' is an arbitrary array of bytes which the client >passes to the direct rendering system to validate the client's >access of direct rendering objects created by the X server. > > It seems like this offers precisely the right guarantee -- the client > proves to the kernel that it is connected to the X server and thus > should be granted permission to access the X server objects. Under some > tighter access control mechanisms, the 'auth-data' could even be > generated per-client so that the client would only have access to a > sub-set of X server objects. This is basically what we have, except the other way around. You're proposing that the server generates a random cookie, informs the DRM that if anybody presents that cookie, they're authenticated and the sends it to the client. The client then receives the cookie and introduces itself to the DRM. What XF86DRI does and what I'm doing for DRI2 is to have the client get a cookie from the DRM that represents the client and then pass it to the server which then asks the DRM to authenticate it. Everybody can talk to the DRM and create a token, but only if you can pass it to the server over DRI2 protocol, can you authenticate. I'd say the two schemes are pretty much equivalent in complexity and in what options we have for narrowing down access per client as you suggest. Pros and cons of the two schemes as I see it is that your scheme eliminates the DRI2Authenticate request from the protocol, but requires a random cookie to be generated, which is a little icky... how many bits etc? The old scheme is well established and the extra request isn't really a concern - it's async. As for Michels concern, sure authentication can be optional, if your video memory manager (trying hard not to say DRM or GEM here) doesn't enforce access restriction. For the Xorg DRI2 and Linux DRM implementations, I do intend to implement access control so we can run multiple X servers without one being able to read out the contents of the others framebuffer. >> For DRI2CopyRegion, you're leaving it to the DDX driver to pick the CRTC >> to synchronize to? I'm not sure that'll work too well with overlapping >> viewports, where the user may want to choose which CRTC to synchronize >> to for each application. > > Yeah, I don't see a good way to avoid this, and the client can always > pass in 'Automatic' (0) and let the server pick the 'right' one. Do we need this? When will the client have a better idea of which pipe a window is on than the X server? >> This request also still seems to be missing >> return values for the sequence number when the copy is expected to take >> place and tokens for synchronization of direct rendering to the >> source/destination buffer. > > Eliminating the reply avoids a round trip, so I'm in favor of not > providing any if it's not strictly necessary. > > I don't know if the GL api requires us to provide the expected sequence > number back to the application. > > For synchronization, we should expect the kernel module to perform this > automatically -- once the X server has processed this request, the > kernel can pend further rendering to the source buffer until the copy > has finished. That would, of course, require that the application know > that the kernel has received the copy command from the X server -- so > the client would need to get something from X server indicating that it > had finished processing the Copy request. The easiest thing to use would > be a reply, but we'd structure the library so that the client wouldn't > pend on the reply and could block just before touching the back buffer > again. Yes, there needs to be a round trip after calling DRI2CopyRegion, to make sure the server submits the copy commands to the DRM before the client can continue rendering. That round trip is DRI2GetBuffers. So glXSwapBuffers() will
Re: DRI2 Protocol Spec Draft
On Mon, Sep 8, 2008 at 9:38 PM, Keith Packard <[EMAIL PROTECTED]> wrote: > On Mon, 2008-09-08 at 17:58 -0400, Kristian Høgsberg wrote: >> Hi, >> >> Keith talked me intro writing a spec for DRI2, which I grudgingly must >> admit is a good idea. I used the randr spec as a template, except I >> replaced the flowery section dividers with a more fitting gears motif. >> It's still a work in progress, mostly the introductory/background >> sections though. The requests and wire encoding sections are mostly >> complete and is close to what's in the git repos at this point. The >> implementation isn't fully uptodate with the spec yet, but I figured >> it would be wise to circulate the spec before doing more code churn, >> so here it is. > > You probably don't need the sub-pixel ordering stuff :-) Ugh, I thought I had replaced that with a reference to the Xfixes regions instead. > Do you want a request to list the available DRI2 types? The buffer types available? This is for when you want to check if, say, a DRI2_BUFFER_YUV is available, and if not fall back to something else? > I don't think we want the window position in the GetBuffers request, > that makes the buffers 'obviously' independent of position. The only DRI2 buffer where the window position may be of interest is the front buffer, since in that case the buffer contents for the window will be at an offset into the buffer. We can't ever reliably access the contents (read or write) in those cases because of the clip rects of course, and the only time the front buffer is used is for texture_from_pixmap, where the drawable in question is a pixmap. So I guess that's a "no, we don't need the window position". > Oh, and some checking to make sure the remaining 'randr' wording is > cleaned up. > It would also be nice to see some additional description about how this > works with GEM. Yeah. Though, I imagine the GEM interaction text will be "implementation notes/suggestion" type of stuff, we want the DRI2 spec to be DRM and GEM independent. I'll try to spell out the requirements DRI2 has on the underlying system (essentially, the ability to name buffers by a 32 bit integer and share them and the auth mechanism). Kristian ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
DRI2 Protocol Spec Draft
Hi,
Keith talked me intro writing a spec for DRI2, which I grudgingly must
admit is a good idea. I used the randr spec as a template, except I
replaced the flowery section dividers with a more fitting gears motif.
It's still a work in progress, mostly the introductory/background
sections though. The requests and wire encoding sections are mostly
complete and is close to what's in the git repos at this point. The
implementation isn't fully uptodate with the spec yet, but I figured
it would be wise to circulate the spec before doing more code churn,
so here it is.
cheers,
Kristian
The DRI2 Extension
Version 2.0
2008-09-04
Kristian Høgsberg
[EMAIL PROTECTED]
Red Hat, Inc
1. Introduction
DRI2 blurb
1.1 Acknowledgements
kem, keithp, anholt, keithw, glisse, idr
â â â â â â
2. DRI2 Concepts
The DRI2 extension is designed to associate and access auxillary
buffers with an X drawable.
DRI2 is a essentially a helper extension to support implementation of
direct rendering drivers/libraries/technologies.
Relation to XF86DRI
Attachment points
Assumes an underlying kernel video memory manager that can provide 32
bit integer handles to buffers, that can be passed between processes.
No ordering between swap buffers and X rendering. X rendering to src
buffers will block if they have a vblank pending.
Relies on DRM, not memory manager specific, but designed to work with GEM.
Authentication model.
â â â â â â
3. Data Types
The subpixel order is shared with the Render extension, and is documented
there. The only datatype defined is the screen size, defined in the normal
(0 degree) orientation.
â â â â â â
4. Errors
Errors are sent using core X error reports.
Authenticate
The X server failed to authenticate the client.
â â â â â â
5. Protocol Types
DRI2ATTACHMENT { DRI2_BUFFER_FRONT_LEFT
DRI2_BUFFER_BACK_LEFT
DRI2_BUFFER_FRONT_RIGHT
DRI2_BUFFER_BACK_RIGHT
DRI2_BUFFER_DEPTH
DRI2_BUFFER_STENCIL
DRI2_BUFFER_ACCUM
DRI2_BUFFER_FAKE_FRONT_LEFT
DRI2_BUFFER_FAKE_FRONT_RIGHT }
These values describe various attachment points for DRI2
buffers.
DRI2BUFFER { attachment: CARD32
name: CARD32
pitch: CARD32
cpp: CARD32
flags: CARD32 }
â â â â â â
6. Extension Initialization
The name of this extension is "DRI2".
ââââ
DRI2QueryVersion
client-major-version: CARD32
client-minor-version: CARD32
â¶
major-version: CARD32
minor-version: CARD32
ââââ
The client sends the highest supported version to the server
and the server sends the highest version it supports, but no
higher than the requested version. Major versions changes can
introduce incompatibilities in existing functionality, minor
version changes introduce only backward compatible changes.
It is the clients responsibility to ensure that the server
supports a version which is compatible with its expectations.
â â â â â â
7. Extension Requests
ââââ
DRI2Connect
window: WINDOW
type: STRING
â¶
driver: STRING
device: STRING -- device file name
group: CARD32
ââââ
Returns the driver name and device file to use for the
specified driver type for the screen associated with 'window'.
'type' identifies the type of driver to query for.
'driver' is the name of the driver to load. The client is
assumed to know where to look for the drivers and what to do
with it.
'device' is the filename of the drm device file.
'group' is the authentication group the server uses.
If the client is not local, or the request driver type is
unknown or not available, 'driver' and 'device' will be empty
strings, 'group' will be '0'. We are not using an regular X
error here to indicate failure, which will allow the client
fall back to other options more easily.
Implementation notes: For the GEM memory manager, use the
GEM_INVITE ioctl to create a token for requesting access to
the servers authentication group.
ââââ
DRI2Authenticate
window: WINDOW
token: CARD32
ââââ
Errors: Window, Authenticate
Request that the X server a
