Re: [PATCH xf86-video-intel 5/6] sna/video/textured: Add XV_COLOR_RANGE port attribute
On Wed, Jul 04, 2018 at 09:59:18PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Add a new Xv port attribute XV_COLOR_RANGE to select the incoming YUV
> quantization range. 0 means limited range (Y: 16-235, Cb/Cr: 16-240),
> 1 means full range (0-255).
>
> Cc: xorg-devel@lists.x.org
> Signed-off-by: Ville Syrjälä
> ---
> src/sna/sna_video_textured.c | 12 +++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/src/sna/sna_video_textured.c b/src/sna/sna_video_textured.c
> index a784fe2ec0f4..a33cbc4506cb 100644
> --- a/src/sna/sna_video_textured.c
> +++ b/src/sna/sna_video_textured.c
> @@ -36,7 +36,7 @@
>
> #define MAKE_ATOM(a) MakeAtom(a, sizeof(a) - 1, true)
>
> -static Atom xvBrightness, xvContrast, xvSyncToVblank, xvColorspace;
> +static Atom xvBrightness, xvContrast, xvSyncToVblank, xvColorspace,
> xvColorRange;
>
> static XvFormatRec Formats[] = {
> {15}, {16}, {24}
> @@ -45,6 +45,7 @@ static XvFormatRec Formats[] = {
> static const XvAttributeRec Attributes[] = {
> {XvSettable | XvGettable, -1, 1, (char *)"XV_SYNC_TO_VBLANK"},
> {XvSettable | XvGettable, 0, 1, (char *)"XV_COLORSPACE"}, /* BT.601,
> BT.709 */
> + {XvSettable | XvGettable, 0, 1, (char *)"XV_COLOR_RANGE"}, /* limited,
> full */
Oh, and I forgot to mention in the commit message that I tried to look
for a pre-existing attribute for this stuff in a bunch other drivers
but couldn't find anything. If I missed something please let me know.
> //{XvSettable | XvGettable, -128, 127, (char *)"XV_BRIGHTNESS"},
> //{XvSettable | XvGettable, 0, 255, (char *)"XV_CONTRAST"},
> };
> @@ -108,6 +109,11 @@ sna_video_textured_set_attribute(ddSetPortAttribute_ARGS)
> return BadValue;
>
> video->colorspace = value;
> + } else if (attribute == xvColorRange) {
> + if (value < 0 || value > 1)
> + return BadValue;
> +
> + video->color_range = value;
> } else
> return BadMatch;
>
> @@ -127,6 +133,8 @@ sna_video_textured_get_attribute(ddGetPortAttribute_ARGS)
> *value = video->SyncToVblank;
> else if (attribute == xvColorspace)
> *value = video->colorspace;
> + else if (attribute == xvColorRange)
> + *value = video->color_range;
> else
> return BadMatch;
>
> @@ -435,6 +443,7 @@ void sna_video_textured_setup(struct sna *sna, ScreenPtr
> screen)
> v->textured = true;
> v->alignment = 4;
> v->colorspace = 1; /* BT.709 */
> + v->color_range = 0; /* limited */
> v->SyncToVblank = (sna->flags & SNA_NO_WAIT) == 0;
>
> RegionNull(&v->clip);
> @@ -456,6 +465,7 @@ void sna_video_textured_setup(struct sna *sna, ScreenPtr
> screen)
> xvBrightness = MAKE_ATOM("XV_BRIGHTNESS");
> xvContrast = MAKE_ATOM("XV_CONTRAST");
> xvColorspace = MAKE_ATOM("XV_COLORSPACE");
> + xvColorRange = MAKE_ATOM("XV_COLOR_RANGE");
> xvSyncToVblank = MAKE_ATOM("XV_SYNC_TO_VBLANK");
>
> DBG(("%s: '%s' initialized %d ports\n", __FUNCTION__, adaptor->name,
> adaptor->nPorts));
> --
> 2.16.4
--
Ville Syrjälä
Intel
___
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 5/7] xfree86/modes: Adapt xf86Randr12CrtcComputeGamma() for depth 30.
On Tue, Feb 20, 2018 at 05:06:32AM +0100, Mario Kleiner wrote:
> At screen depths > 24 bit, the color palettes passed into
> xf86Randr12CrtcComputeGamma() can have a larger number of slots
> than the crtc's hardware lut. E.g., at depth 30, 1024 palette
> slots vs. 256 hw lut slots. This palette size > crtc gamma size
> case is not handled yet and leads to silent failure, so gamma
> table updates do not happen.
>
> Add a new subsampling path for this case, which only takes
> every n'th slot from the input palette, e.g., at depth 30,
> every 4th slot for a 1024 slot palette vs. 256 slot hw lut.
>
> This makes lut updates work again, as tested with the xgamma
> utility (uses XF86VidMode extension) and some RandR based
> gamma ramp animation.
>
> Signed-off-by: Mario Kleiner
> ---
> hw/xfree86/modes/xf86RandR12.c | 99
> ++
> 1 file changed, 72 insertions(+), 27 deletions(-)
>
> diff --git a/hw/xfree86/modes/xf86RandR12.c b/hw/xfree86/modes/xf86RandR12.c
> index fe8770d..8947622 100644
> --- a/hw/xfree86/modes/xf86RandR12.c
> +++ b/hw/xfree86/modes/xf86RandR12.c
> @@ -1258,40 +1258,85 @@ xf86RandR12CrtcComputeGamma(xf86CrtcPtr crtc, LOCO
> *palette,
>
> for (shift = 0; (gamma_size << shift) < (1 << 16); shift++);
>
> -gamma_slots = crtc->gamma_size / palette_red_size;
> -for (i = 0; i < palette_red_size; i++) {
> -value = palette[i].red;
> -if (gamma_red)
> -value = gamma_red[value];
> -else
> -value <<= shift;
> +if (crtc->gamma_size >= palette_red_size) {
> +/* Upsampling of smaller palette to larger hw lut size */
> +gamma_slots = crtc->gamma_size / palette_red_size;
> +for (i = 0; i < palette_red_size; i++) {
> +value = palette[i].red;
> +if (gamma_red)
> +value = gamma_red[value];
> +else
> +value <<= shift;
> +
> +for (j = 0; j < gamma_slots; j++)
> +crtc->gamma_red[i * gamma_slots + j] = value;
> +}
> +} else {
> +/* Downsampling of larger palette to smaller hw lut size */
> +gamma_slots = palette_red_size / crtc->gamma_size;
> +for (i = 0; i < crtc->gamma_size; i++) {
> +value = palette[i * gamma_slots].red;
That's not going to reach the max index of the palette, and it'll not
work correctly when the sizes aren't a nice integer multiple of each
other. Eg. intel hw has interpolated gamma modes which have 2^n+1
entries in the LUT.
So I'm thinking we want this instead:
value = palette[i * (palette_size - 1) / (gamma_size - 1)];
--
Ville Syrjälä
Intel OTC
___
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/4] dri2: Split resource tracking for DRI2Drawable and references to them
On Wed, Feb 03, 2016 at 09:54:44AM +, Chris Wilson wrote:
> In order to ease resource tracking, disentangle DRI2Drawable XIDs from
> their references. This will be used in later patches to first limit the
> object leak from unnamed references created on behalf of Clients and
> then never destroy, and then to allow Clients to explicit manage their
> references to DRI2Drawables.
>
> Signed-off-by: Chris Wilson
> Cc: Ville Syrjälä
> ---
> glx/glxdri2.c | 10 ++--
> hw/xfree86/dri2/dri2.c| 136
> ++
> hw/xfree86/dri2/dri2.h| 11 ++--
> hw/xfree86/dri2/dri2ext.c | 6 +-
> 4 files changed, 66 insertions(+), 97 deletions(-)
>
> diff --git a/glx/glxdri2.c b/glx/glxdri2.c
> index 89ad808..d7f1436 100644
> --- a/glx/glxdri2.c
> +++ b/glx/glxdri2.c
> @@ -105,7 +105,7 @@ __glXDRIdrawableDestroy(__GLXdrawable * drawable)
> __GLXDRIdrawable *private = (__GLXDRIdrawable *) drawable;
> const __DRIcoreExtension *core = private->screen->core;
>
> -FreeResource(private->dri2_id, FALSE);
> +DRI2DestroyDrawable(NULL, private->base.pDraw, private->dri2_id);
>
> (*core->destroyDrawable) (private->driDrawable);
>
> @@ -602,7 +602,7 @@ __glXDRIscreenCreateContext(__GLXscreen * baseScreen,
> }
>
> static void
> -__glXDRIinvalidateBuffers(DrawablePtr pDraw, void *priv, XID id)
> +__glXDRIinvalidateBuffers(DrawablePtr pDraw, void *priv)
> {
> __GLXDRIdrawable *private = priv;
> __GLXDRIscreen *screen = private->screen;
> @@ -641,9 +641,9 @@ __glXDRIscreenCreateDrawable(ClientPtr client,
> private->base.waitGL = __glXDRIdrawableWaitGL;
> private->base.waitX = __glXDRIdrawableWaitX;
>
> -ret = DRI2CreateDrawable2(client, pDraw, drawId,
> - __glXDRIinvalidateBuffers, private,
> - &private->dri2_id);
> +private->dri2_id = FakeClientID(client->index);
> +ret = DRI2CreateDrawable(client, pDraw, private->dri2_id,
> + __glXDRIinvalidateBuffers, private);
I was a but confused by the change to not passing the drawable id
separately, but after another look it seems it should end up being
exactly the same as before. The most special case being the glx pbuffer,
but that seems to have the pixmap drawable id set to the glxdrawable id,
and that is what was passed in also. And everywone else seemed to pass
the relevant X drawable id.
Otherwise adding the references resource looks sane enough to me, so
Reviewed-by: Ville Syrjälä
> if (cx != lastGLContext) {
> lastGLContext = cx;
> cx->makeCurrent(cx);
> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> index bbff11c..4dc40f8 100644
> --- a/hw/xfree86/dri2/dri2.c
> +++ b/hw/xfree86/dri2/dri2.c
> @@ -68,16 +68,16 @@ static DevPrivateKeyRec dri2PixmapPrivateKeyRec;
>
> static DevPrivateKeyRec dri2ClientPrivateKeyRec;
>
> -#define dri2ClientPrivateKey (&dri2ClientPrivateKeyRec)
> -
> -#define dri2ClientPrivate(_pClient)
> (dixLookupPrivate(&(_pClient)->devPrivates, \
> - dri2ClientPrivateKey))
> -
> typedef struct _DRI2Client {
> int prime_id;
> } DRI2ClientRec, *DRI2ClientPtr;
>
> -static RESTYPE dri2DrawableRes;
> +static DRI2ClientPtr dri2ClientPrivate(ClientPtr pClient)
> +{
> +return dixLookupPrivate(&pClient->devPrivates, &dri2ClientPrivateKeyRec);
> +}
> +
> +static RESTYPE dri2DrawableRes, dri2ReferenceRes;
>
> typedef struct _DRI2Screen *DRI2ScreenPtr;
>
> @@ -203,6 +203,11 @@ DRI2AllocateDrawable(DrawablePtr pDraw)
> if (pPriv == NULL)
> return NULL;
>
> +if (!AddResource(pDraw->id, dri2DrawableRes, pPriv)) {
> +free(pPriv);
> +return NULL;
> +}
> +
> pPriv->dri2_screen = ds;
> pPriv->drawable = pDraw;
> pPriv->width = pDraw->width;
> @@ -269,49 +274,37 @@ DRI2SwapLimit(DrawablePtr pDraw, int swap_limit)
> }
>
> typedef struct DRI2DrawableRefRec {
> -XID id;
> -XID dri2_id;
> +XID pid;
> DRI2InvalidateProcPtr invalidate;
> void *priv;
> struct xorg_list link;
> } DRI2DrawableRefRec, *DRI2DrawableRefPtr;
>
> -static DRI2DrawableRefPtr
> -DRI2LookupDrawableRef(DRI2DrawablePtr pPriv, XID id)
> +int
> +DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID pid,
> + DRI2InvalidateProcPtr invalidate, void *priv)
> {
> +DRI2DrawablePtr pPriv;
> DRI2DrawableRefPtr ref;
>
> -xorg_list_for_each_entry(ref, &pPriv-&g
Re: [PATCH xserver 3/4] dri2: Only create one unnamed reference per Drawable per Client
On Wed, Feb 03, 2016 at 09:54:45AM +, Chris Wilson wrote:
> This workarounds issues in mesa and Mali that likes to create a new
> DRI2Drawble per context and per frame, respectively. The idea is that we
> only create one unnamed reference per Client on each Drawable that is
> never destroyed - and we make the assumption that the only caller is
> the DRI2 dispatcher and thus we don't need to check that the invalidate
> handler is the same. Every DRI2Drawable reference sends an invalidate
> event, and so not only do we have a slow memory leak, we also suffer a
> performance loss as the Clients create more and more references to the
> same Drawable - unless we limit them to a single reference each.
>
> The issue was first reported 5^H 6 years ago by Pauli Nieminen and I have
> incorporated his patches here. His improvement was to add a reference
> count to the per-Client unnamed reference and by doing so could support
> DRI2DestroyDrawable again. However, it remains the case that as the
> lifecycles between the GLXDrawable (DRI2Drawable) and the parent
> Drawable are decoupled, mesa cannot call DRI2DestroyDrawable on
> Drawables it did not create (or else risk generating BadDrawable runtime
> errors), see mesa commit 4ebf07a426771b62123e5fcb5a8be0de24037af1
> Author: Kristian Høgsberg
> Date: Mon Sep 13 08:39:42 2010 -0400
>
> glx: Don't destroy DRI2 drawables for legacy glx drawables
>
> For GLX 1.3 drawables, we can destroy the DRI2 drawable when the GLX
> drawable is destroyed. However, for legacy drawables, there os no
> good way of knowing when the application is done with it, so we just
> let the DRI2 drawable linger on the server. The server will destroy
> the DRI2 drawable when it destroys the X drawable or the client exits
> anyway.
>
> https://bugs.freedesktop.org/show_bug.cgi?id=30109
>
> and as such it rules out both using named references by the Clients and
> the reference ever being reaped.
Hmm. IIRC Pauli's solution avoided the BadDrawable by replacing
validDrawable() with something dri2 specific that looks things up in
the dri2 resource instead. And thus you could do the dri2DestroyDrawable
after the X drawable is gone without risking errors. So the dri2
drawable was more or less separate from the X drawable, except you
address both using the same X drawable id since the dri2 drawable
doesn't have its own id.
I suppose the main benefit of that is that the client stops getting
invalidate events for drawables it's no longer interested in. The
downside is that the dri2 drawable could then linger around after the
X drawable is gone if the client didn't explicitly destroy it.
What I can't recall is how it would have dealt with the client reusing
the drawable id after destroying the X drawable if the dri2 drawable
was still around. Maybe it didn't. Shame the dri2 drawable doesn't have
its own id.
>
> Note Present incorporates its own version of this type of reference leak.
>
> v2: Incorporate refcounting of the unnamed reference by Pauli Nieminen.
>
> Cc: Daniel Drake
> Link: https://freedesktop.org/patch/19695/
> Link: http://lists.x.org/archives/xorg-devel/2010-November/014783.html
> Link: http://lists.x.org/archives/xorg-devel/2010-November/014782.html
> Signed-off-by: Chris Wilson
> Cc: Ville Syrjälä
> ---
> hw/xfree86/dri2/dri2.c| 57
> +++
> hw/xfree86/dri2/dri2ext.c | 4 ++--
> 2 files changed, 55 insertions(+), 6 deletions(-)
>
> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> index 4dc40f8..2f05c64 100644
> --- a/hw/xfree86/dri2/dri2.c
> +++ b/hw/xfree86/dri2/dri2.c
> @@ -275,11 +275,24 @@ DRI2SwapLimit(DrawablePtr pDraw, int swap_limit)
>
> typedef struct DRI2DrawableRefRec {
> XID pid;
> +unsigned int refcnt;
> DRI2InvalidateProcPtr invalidate;
> void *priv;
> struct xorg_list link;
> } DRI2DrawableRefRec, *DRI2DrawableRefPtr;
>
> +static DRI2DrawableRefPtr
> +DRI2FindClientDrawableRef(ClientPtr client, DRI2DrawablePtr pPriv)
> +{
> +DRI2DrawableRefPtr ref;
> +
> +xorg_list_for_each_entry(ref, &pPriv->reference_list, link)
> +if (ref->refcnt && CLIENT_ID(ref->pid) == client->index)
So the refcnt check here prevents this from matching with the dri2
drawables created by glx, as those will have refcnt==0.
Looks sane enough to me. Though it's been years since I last looked at
this stuff, so not sure I even remember all the relevant details.
But anyways,
Reviewed-by: Ville Syrjälä
> +return ref;
> +
> +return NULL;
> +}
> +
> int
> DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID pid
Re: [PATCH xserver 4/4] dri2: Unblock Clients on Drawable release
On Wed, Feb 03, 2016 at 09:54:46AM +, Chris Wilson wrote: > If the Window is destroyed by another client, such as the window > manager, the original client may be blocked by DRI2 awaiting a vblank > event. When this happens, DRI2DrawableGone forgets to unblock that > client and so the wait never completes. > > Note Present/xshmfence is also suspectible to this race. > > Testcase: dri2-race/manager > Signed-off-by: Chris Wilson > Cc: Ville Syrjälä Reviewed-by: Ville Syrjälä > --- > hw/xfree86/dri2/dri2.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c > index 2f05c64..80a601e 100644 > --- a/hw/xfree86/dri2/dri2.c > +++ b/hw/xfree86/dri2/dri2.c > @@ -416,6 +416,9 @@ DRI2DrawableGone(void *p, XID id) > (*pDraw->pScreen->DestroyPixmap)(pPriv->redirectpixmap); > } > > +if (pPriv->blockedClient) > +AttendClient(pPriv->blockedClient); > + > free(pPriv); > > return Success; > -- > 2.7.0 -- Ville Syrjälä Intel OTC ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver 1/4] dri2: Only invalidate the immediate Window upon SetWindowPixmap
On Wed, Feb 03, 2016 at 09:54:43AM +, Chris Wilson wrote:
> All callers of SetWindowPixmap will themselves be traversing the Window
> heirachy updating the backing Pixmap of each child and so we can forgo
> doing the identical traversal inside the DRI2SetWindowPixmap handler.
>
> Reported-by: Loïc Yhuel
> Link: http://lists.x.org/archives/xorg-devel/2015-February/045638.html
> Signed-off-by: Chris Wilson
> Cc: Ville Syrjälä
Seems sane.
Reviewed-by: Ville Syrjälä
> ---
> hw/xfree86/dri2/dri2.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> index 60ea6dd..bbff11c 100644
> --- a/hw/xfree86/dri2/dri2.c
> +++ b/hw/xfree86/dri2/dri2.c
> @@ -1385,8 +1385,7 @@ DRI2ConfigNotify(WindowPtr pWin, int x, int y, int w,
> int h, int bw,
> static void
> DRI2SetWindowPixmap(WindowPtr pWin, PixmapPtr pPix)
> {
> -DrawablePtr pDraw = (DrawablePtr) pWin;
> -ScreenPtr pScreen = pDraw->pScreen;
> +ScreenPtr pScreen = pWin->drawable.pScreen;
> DRI2ScreenPtr ds = DRI2GetScreen(pScreen);
>
> pScreen->SetWindowPixmap = ds->SetWindowPixmap;
> @@ -1394,7 +1393,7 @@ DRI2SetWindowPixmap(WindowPtr pWin, PixmapPtr pPix)
> ds->SetWindowPixmap = pScreen->SetWindowPixmap;
> pScreen->SetWindowPixmap = DRI2SetWindowPixmap;
>
> -DRI2InvalidateDrawableAll(pDraw);
> +DRI2InvalidateDrawable(&pWin->drawable);
> }
>
> #define MAX_PRIME DRI2DriverPrimeMask
> --
> 2.7.0
--
Ville Syrjälä
Intel OTC
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/2] Xv: Only stop the adaptors when the Pixmap is finally destroyed
On Sun, Apr 05, 2015 at 10:32:03AM +0100, Chris Wilson wrote:
> Pixmaps are reference counted and DestroyPixmap is called for the
> removal of every reference. However, we only want to stop the adaptors
> writing into the Pixmap just before the Pixmap is finally destroyed,
> similar to how Windows are handled.
>
> Signed-off-by: Chris Wilson
> Cc: Ville Syrjälä
Hmm. found this old nugget in my inbox. Looks sane enough, and
eliminates duplicated code.
Reviewed-by: Ville Syrjälä
BTW as sna doesn't use xf86xv it doesn't appear to check the drawable
type at all. Does that mean it allows even overlay/sprite adaptors to
be used with pixmaps?
> ---
> Xext/xvmain.c | 82
> +--
> 1 file changed, 24 insertions(+), 58 deletions(-)
>
> diff --git a/Xext/xvmain.c b/Xext/xvmain.c
> index 0abf190..ad1b39d 100644
> --- a/Xext/xvmain.c
> +++ b/Xext/xvmain.c
> @@ -327,36 +327,24 @@ XvGetRTPort(void)
> return XvRTPort;
> }
>
> -static Bool
> -XvDestroyPixmap(PixmapPtr pPix)
> +static void
> +XvStopAdaptors(DrawablePtr pDrawable)
> {
> -Bool status;
> -ScreenPtr pScreen;
> -XvScreenPtr pxvs;
> -XvAdaptorPtr pa;
> -int na;
> -XvPortPtr pp;
> -int np;
> -
> -pScreen = pPix->drawable.pScreen;
> -
> -SCREEN_PROLOGUE(pScreen, DestroyPixmap);
> -
> -pxvs = (XvScreenPtr) dixLookupPrivate(&pScreen->devPrivates,
> XvScreenKey);
> +ScreenPtr pScreen = pDrawable->pScreen;
> +XvScreenPtr pxvs = dixLookupPrivate(&pScreen->devPrivates, XvScreenKey);
> +XvAdaptorPtr pa = pxvs->pAdaptors;
> +int na = pxvs->nAdaptors;
>
> /* CHECK TO SEE IF THIS PORT IS IN USE */
> -
> -pa = pxvs->pAdaptors;
> -na = pxvs->nAdaptors;
> while (na--) {
> -np = pa->nPorts;
> -pp = pa->pPorts;
> +XvPortPtr pp = pa->pPorts;
> +int np = pa->nPorts;
>
> while (np--) {
> -if (pp->pDraw == (DrawablePtr) pPix) {
> -XvdiSendVideoNotify(pp, pp->pDraw, XvPreempted);
> +if (pp->pDraw == pDrawable) {
> +XvdiSendVideoNotify(pp, pDrawable, XvPreempted);
>
> -(void) (*pp->pAdaptor->ddStopVideo) (pp, pp->pDraw);
> +(void) (*pp->pAdaptor->ddStopVideo) (pp, pDrawable);
>
> pp->pDraw = NULL;
> pp->client = NULL;
> @@ -366,9 +354,19 @@ XvDestroyPixmap(PixmapPtr pPix)
> }
> pa++;
> }
> +}
>
> -status = (*pScreen->DestroyPixmap) (pPix);
> +static Bool
> +XvDestroyPixmap(PixmapPtr pPix)
> +{
> +ScreenPtr pScreen = pPix->drawable.pScreen;
> +Bool status;
> +
> +if (pPix->refcnt == 1)
> +XvStopAdaptors(&pPix->drawable);
>
> +SCREEN_PROLOGUE(pScreen, DestroyPixmap);
> +status = (*pScreen->DestroyPixmap) (pPix);
> SCREEN_EPILOGUE(pScreen, DestroyPixmap, XvDestroyPixmap);
>
> return status;
> @@ -378,45 +376,13 @@ XvDestroyPixmap(PixmapPtr pPix)
> static Bool
> XvDestroyWindow(WindowPtr pWin)
> {
> +ScreenPtr pScreen = pWin->drawable.pScreen;
> Bool status;
> -ScreenPtr pScreen;
> -XvScreenPtr pxvs;
> -XvAdaptorPtr pa;
> -int na;
> -XvPortPtr pp;
> -int np;
>
> -pScreen = pWin->drawable.pScreen;
> +XvStopAdaptors(&pWin->drawable);
>
> SCREEN_PROLOGUE(pScreen, DestroyWindow);
> -
> -pxvs = (XvScreenPtr) dixLookupPrivate(&pScreen->devPrivates,
> XvScreenKey);
> -
> -/* CHECK TO SEE IF THIS PORT IS IN USE */
> -
> -pa = pxvs->pAdaptors;
> -na = pxvs->nAdaptors;
> -while (na--) {
> -np = pa->nPorts;
> -pp = pa->pPorts;
> -
> -while (np--) {
> -if (pp->pDraw == (DrawablePtr) pWin) {
> -XvdiSendVideoNotify(pp, pp->pDraw, XvPreempted);
> -
> -(void) (*pp->pAdaptor->ddStopVideo) (pp, pp->pDraw);
> -
> -pp->pDraw = NULL;
> -pp->client = NULL;
> -pp->time = currentTime;
> -}
> -pp++;
> -}
> -pa++;
> -}
> -
> status = (*pScreen->DestroyWindow) (pWin);
> -
> SCREEN_EPILOGUE(pScreen, DestroyWindow, XvDestroyWindow);
>
> return status;
> --
> 2.1.4
--
Ville Syrjälä
Intel OTC
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: Limit DRI2Drawable reference leak
On Sat, Feb 21, 2015 at 10:52:49PM +, Chris Wilson wrote: > On Sun, Feb 22, 2015 at 12:13:38AM +0200, Ville Syrjälä wrote: > > On Sat, Feb 21, 2015 at 09:31:07PM +, Chris Wilson wrote: > > > With the current protocol and implementations, we have to often call > > > DRI2CreateDrawable but can never call DRI2DestroyDrawable. This ends up > > > with us leaking references to DRI2Drawables based on the assumption that > > > the references have identical lifetimes to the Drawable going astray. > > > This was spotted by Daniel Drake as the mali driver would create a new > > > reference to the DRI2Drawable on every GetBuffers, but it can also be > > > observed in mesa when running synthetic benchmarks (creating lots of > > > contexts/glxdrawables for each window) and to a lesser extent with > > > normal composited operation. > > > > > > The first two patches implement the capping of the unnamed internal > > > reference used by DRI2CreateDrawable to just one per Client. > > > > IIRC we had many issues around the dri2 reference stuff during the > > Maemo days. Pauli fixed tons of problems in the dri2 code but some > > of the patches never made it in. > > > > These seem somewhat relevant: > > http://lists.x.org/archives/xorg-devel/2010-November/014783.html > > http://lists.x.org/archives/xorg-devel/2010-November/014782.html > > Indeed. Same problem, similar solution. I was a bit dubious as to > whether we could hook up DRI2DestroyDrawable after all this time (for > example mesa ignores it except for GLXPixmaps) and feared there was some > corner case that was going to explode. Yeah dunno. This code did ship in the N9/N950 and our client side did issue these protocol requests appropriately. But then again, Pauli did a boatload of additional work on the dri2 code after these that didn't make it into upstream either. As I recall one of the issues was clients getting BadDrawables from DRI2DestroyDrawable if the X drawable already went away. And the solution was to decouple the lifetimes of the two. I've probably forgotten most of what we did back then, but interested parties may go dig through https://www.gitorious.org/meego-w40/xserver-xorg if they wish. -- Ville Syrjälä syrj...@sci.fi http://www.sci.fi/~syrjala/ ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: Limit DRI2Drawable reference leak
On Sat, Feb 21, 2015 at 09:31:07PM +, Chris Wilson wrote: > With the current protocol and implementations, we have to often call > DRI2CreateDrawable but can never call DRI2DestroyDrawable. This ends up > with us leaking references to DRI2Drawables based on the assumption that > the references have identical lifetimes to the Drawable going astray. > This was spotted by Daniel Drake as the mali driver would create a new > reference to the DRI2Drawable on every GetBuffers, but it can also be > observed in mesa when running synthetic benchmarks (creating lots of > contexts/glxdrawables for each window) and to a lesser extent with > normal composited operation. > > The first two patches implement the capping of the unnamed internal > reference used by DRI2CreateDrawable to just one per Client. IIRC we had many issues around the dri2 reference stuff during the Maemo days. Pauli fixed tons of problems in the dri2 code but some of the patches never made it in. These seem somewhat relevant: http://lists.x.org/archives/xorg-devel/2010-November/014783.html http://lists.x.org/archives/xorg-devel/2010-November/014782.html -- Ville Syrjälä syrj...@sci.fi http://www.sci.fi/~syrjala/ ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [Intel-gfx] [PATCH 09/12] Do more checks for proposed flip pixmaps
On Thu, Jul 31, 2014 at 08:20:20AM -0700, Keith Packard wrote: > Ville Syrjälä writes: > > > Now that we have mmio flips in the kernel we can start to relax that > > restriction. That still needs a bit more work in the mmio flip code > > but I believe some people working on just that. > > I couldn't find any tiling restrictions in the current (ring-based) flip > code; did I just miss them? No, changing tiling is supposed to work via cs flips. Except it doesn't actually work on VLV for some reason. We now fall back to mmio flip on VLV for that, so given a recent enough kernel it should just work (tm) on all platforms. I was referring to the relaxing the restrictions on stride changes. > > > We could even change the pixel format, except a check was added to > > drm_mode_page_flip_ioctl() to prevent that, so I guess it was > > deemed that the API isn't meant to allow that. > > Yeah, not sure I care about this; 32bpp is pretty much the only format I > want. You just need to be careful with the X vs. A because the kernel considers those distinct pixel formats. I now regret adding that distinciton to the pixel formats, but sadly I don't own a time machine so I can't undo it. -- Ville Syrjälä Intel OTC ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [Intel-gfx] [PATCH 09/12] Do more checks for proposed flip pixmaps
On Wed, Jul 30, 2014 at 11:01:48PM -0700, Keith Packard wrote: > Eric Anholt writes: > > > Keith Packard writes: > > > >> Make sure the pitch and tiling are correct. > >> Make sure there's a BO we can get at. > > > > I thought we couldn't change these parameters, but now I can't find what > > prevents them from changing. Can you cite sources? > > Looks like we *can* change tiling format. That actually makes me kinda > happy as that explains why we were able to allocate a linear frame > buffer for the X front buffer (due to a bug) and page flip to DRI3 > buffers which are always tiled. > > However, we can't change the pitch. From the kernel driver: > > /* >* TILEOFF/LINOFF registers can't be changed via MI display flips. >* Note that pitch changes could also affect these register. >*/ > if (INTEL_INFO(dev)->gen > 3 && > (fb->offsets[0] != crtc->primary->fb->offsets[0] || >fb->pitches[0] != crtc->primary->fb->pitches[0])) > return -EINVAL; > > I'll remove the tiling check. Now that we have mmio flips in the kernel we can start to relax that restriction. That still needs a bit more work in the mmio flip code but I believe some people working on just that. We could even change the pixel format, except a check was added to drm_mode_page_flip_ioctl() to prevent that, so I guess it was deemed that the API isn't meant to allow that. -- Ville Syrjälä Intel OTC ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/2] xrandr: Use more decimal places when printing various rates
On Thu, Feb 20, 2014 at 08:51:57AM -0800, Aaron Plattner wrote:
> On 02/20/2014 12:47 AM, Ville Syrjälä wrote:
> > On Wed, Feb 19, 2014 at 04:22:19PM -0800, Aaron Plattner wrote:
> >> On 05/31/2013 07:01 AM, ville.syrj...@linux.intel.com wrote:
> >>> From: Ville Syrjälä
> >>>
> >>> Using just one decimal place for dotclock and refresh rates loses quite
> >>> a bit of information. When dealing with 60Hz vs. 59.94Hz refresh rate
> >>> modes for example, it's useful to see at least two decimal places. For
> >>> the dotclock in similar cases, three decimal places seems quite a bit
> >>> better than just one.
> >>>
> >>> Signed-off-by: Ville Syrjälä
> >>> ---
> >>>xrandr.c | 18 +-
> >>>1 file changed, 9 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/xrandr.c b/xrandr.c
> >>> index 94e5c2e..9467c29 100644
> >>> --- a/xrandr.c
> >>> +++ b/xrandr.c
> >>> @@ -1564,7 +1564,7 @@ crtc_apply (crtc_t *crtc)
> >>> rr_outputs[o] = crtc->outputs[o]->output.xid;
> >>>mode = crtc->mode_info->id;
> >>>if (verbose) {
> >>> - printf ("crtc %d: %12s %6.1f +%d+%d", crtc->crtc.index,
> >>> + printf ("crtc %d: %12s %6.2f +%d+%d", crtc->crtc.index,
> >>> crtc->mode_info->name, mode_refresh (crtc->mode_info),
> >>> crtc->x, crtc->y);
> >>> for (o = 0; o < crtc->noutput; o++)
> >>> @@ -3589,7 +3589,7 @@ main (int argc, char **argv)
> >>> XRRModeInfo *mode = find_mode_by_xid
> >>> (output_info->modes[j]);
> >>> int f;
> >>>
> >>> - printf (" %s (0x%x) %6.1fMHz",
> >>> + printf (" %s (0x%x) %6.3fMHz",
> >>> mode->name, (int)mode->id,
> >>> (double)mode->dotClock / 100.0);
> >>> for (f = 0; mode_flags[f].flag; f++)
> >>> @@ -3600,10 +3600,10 @@ main (int argc, char **argv)
> >>> if (j < output_info->npreferred)
> >>> printf (" +preferred");
> >>> printf ("\n");
> >>> - printf ("h: width %4d start %4d end %4d total %4d
> >>> skew %4d clock %6.1fKHz\n",
> >>> + printf ("h: width %4d start %4d end %4d total %4d
> >>> skew %4d clock %6.2fKHz\n",
> >>> mode->width, mode->hSyncStart,
> >>> mode->hSyncEnd,
> >>> mode->hTotal, mode->hSkew, mode_hsync
> >>> (mode) / 1000);
> >>> - printf ("v: height %4d start %4d end %4d total %4d
> >>> clock %6.1fHz\n",
> >>> + printf ("v: height %4d start %4d end %4d total %4d
> >>> clock %6.2fHz\n",
> >>> mode->height, mode->vSyncStart,
> >>> mode->vSyncEnd, mode->vTotal,
> >>> mode_refresh (mode));
> >>> mode->modeFlags |= ModeShown;
> >>> @@ -3630,7 +3630,7 @@ main (int argc, char **argv)
> >>> if (strcmp (jmode->name, kmode->name) != 0)
> >>> continue;
> >>> mode_shown[k] = True;
> >>> kmode->modeFlags |= ModeShown;
> >>> - printf (" %6.1f", mode_refresh (kmode));
> >>> + printf (" %6.2f", mode_refresh (kmode));
> >>> if (kmode == output->mode_info)
> >>> printf ("*");
> >>> else
> >>> @@ -3651,13 +3651,13 @@ main (int argc, char **argv)
> >>>
> >>> if (!(mode->modeFlags & ModeShown))
> >>> {
> >>> - printf (" %s (0x%x) %6.1fMHz\n",
> >>> + printf (" %s (0x%x) %6.3fMHz\n",
> >>> mode->name, (int)mode->
Re: [PATCH 1/2] xrandr: Use more decimal places when printing various rates
On Wed, Feb 19, 2014 at 04:22:19PM -0800, Aaron Plattner wrote:
> On 05/31/2013 07:01 AM, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä
> >
> > Using just one decimal place for dotclock and refresh rates loses quite
> > a bit of information. When dealing with 60Hz vs. 59.94Hz refresh rate
> > modes for example, it's useful to see at least two decimal places. For
> > the dotclock in similar cases, three decimal places seems quite a bit
> > better than just one.
> >
> > Signed-off-by: Ville Syrjälä
> > ---
> > xrandr.c | 18 +-
> > 1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/xrandr.c b/xrandr.c
> > index 94e5c2e..9467c29 100644
> > --- a/xrandr.c
> > +++ b/xrandr.c
> > @@ -1564,7 +1564,7 @@ crtc_apply (crtc_t *crtc)
> > rr_outputs[o] = crtc->outputs[o]->output.xid;
> > mode = crtc->mode_info->id;
> > if (verbose) {
> > - printf ("crtc %d: %12s %6.1f +%d+%d", crtc->crtc.index,
> > + printf ("crtc %d: %12s %6.2f +%d+%d", crtc->crtc.index,
> > crtc->mode_info->name, mode_refresh (crtc->mode_info),
> > crtc->x, crtc->y);
> > for (o = 0; o < crtc->noutput; o++)
> > @@ -3589,7 +3589,7 @@ main (int argc, char **argv)
> > XRRModeInfo *mode = find_mode_by_xid
> > (output_info->modes[j]);
> > int f;
> >
> > - printf (" %s (0x%x) %6.1fMHz",
> > + printf (" %s (0x%x) %6.3fMHz",
> > mode->name, (int)mode->id,
> > (double)mode->dotClock / 100.0);
> > for (f = 0; mode_flags[f].flag; f++)
> > @@ -3600,10 +3600,10 @@ main (int argc, char **argv)
> > if (j < output_info->npreferred)
> > printf (" +preferred");
> > printf ("\n");
> > - printf ("h: width %4d start %4d end %4d total %4d
> > skew %4d clock %6.1fKHz\n",
> > + printf ("h: width %4d start %4d end %4d total %4d
> > skew %4d clock %6.2fKHz\n",
> > mode->width, mode->hSyncStart, mode->hSyncEnd,
> > mode->hTotal, mode->hSkew, mode_hsync (mode) /
> > 1000);
> > - printf ("v: height %4d start %4d end %4d total %4d
> > clock %6.1fHz\n",
> > + printf ("v: height %4d start %4d end %4d total %4d
> > clock %6.2fHz\n",
> > mode->height, mode->vSyncStart, mode->vSyncEnd,
> > mode->vTotal,
> > mode_refresh (mode));
> > mode->modeFlags |= ModeShown;
> > @@ -3630,7 +3630,7 @@ main (int argc, char **argv)
> > if (strcmp (jmode->name, kmode->name) != 0) continue;
> > mode_shown[k] = True;
> > kmode->modeFlags |= ModeShown;
> > - printf (" %6.1f", mode_refresh (kmode));
> > + printf (" %6.2f", mode_refresh (kmode));
> > if (kmode == output->mode_info)
> > printf ("*");
> > else
> > @@ -3651,13 +3651,13 @@ main (int argc, char **argv)
> >
> > if (!(mode->modeFlags & ModeShown))
> > {
> > - printf (" %s (0x%x) %6.1fMHz\n",
> > + printf (" %s (0x%x) %6.3fMHz\n",
> > mode->name, (int)mode->id,
> > (double)mode->dotClock / 100.0);
> > - printf ("h: width %4d start %4d end %4d total %4d skew
> > %4d clock %6.1fKHz\n",
> > + printf ("h: width %4d start %4d end %4d total %4d skew
> > %4d clock %6.2fKHz\n",
> > mode->width, mode->hSyncStart, mode->hSyncEnd,
> > mode->hTotal, mode->hSkew, mode_hsync (mode) / 1000);
> > - printf ("v: height %4d start %4d end %4d total %4d
> > clock %6.1fHz\n",
> > + printf ("v: height %4d start %4d end %4d total %4d
> > clock %6.2fHz\n",
> > mode->height, mode->vSyncStart, mode->vSyncEnd,
> > mode->vTotal,
&
Re: [PATCH] xrandr:an additional decimal for frequencies
width, mode->hSyncStart, mode->hSyncEnd,
> mode->hTotal, mode->hSkew, mode_hsync (mode) / 1000);
> - printf ("v: height %4d start %4d end %4d total %4d
> clock %6.1fHz\n",
> + printf ("v: height %4d start %4d end %4d total %4d
> clock %7.2fHz\n",
> mode->height, mode->vSyncStart, mode->vSyncEnd,
> mode->vTotal,
> mode_refresh (mode));
> }
> @@ -3799,7 +3799,7 @@ main (int argc, char **argv)
> if (rate == rates[i])
> break;
> if (i == nrate) {
> - fprintf (stderr, "Rate %.1f Hz not available for this size\n",
> rate);
> + fprintf (stderr, "Rate %.2f Hz not available for this size\n",
> rate);
> exit (1);
> }
> }
> --
> 1.9.0.rc3
>
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
--
Ville Syrjälä
syrj...@sci.fi
http://www.sci.fi/~syrjala/
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: Fixing the kernels backlight API
On Thu, Feb 13, 2014 at 06:14:04AM +1000, Dave Airlie wrote: > > > > The biggest remaining stumbling block is the backlight API, because opening > > the > > sysfs files requires root rights. I'll very likely write a little helper > > for this > > for now, but in the long run it would be good to have a better solution. > > > > While discussion this in the graphics devroom at Fosdem, the general > > consensus > > seemed to be that the current backlight API is in need of an overhaul > > anyways. > > > > There are several issues with the current API: > > -there is no reliable way to determine the relation between a backlight > > control in sysfs and the display it controls the backlight off > > -on many laptops we end up with multiple providers of backlight control > > all battling over control of the same backlight controller through various > > firmware interfaces > > -and there is no way to do acl management on it because of sysfs usage > > > > At Fosdem it was suggested to "simply" make the backlight a property of > > the connector in drm/kms and let users control it that way. From an acl pov > > this makes a ton of sense, if a user controls the other display-panel > > settings, > > then he should be able to control the backlight too. > > > > This also nicely solves the issue of userspace having to figure out which > > backlight > > control to use for a certain output. > > > > Last this makes it the kernels responsibility to figure out which firmware > > interface > > (if any) to use and tie that to the connector, rather then just exporting > > all of > > them, including conflicting ones, and just hoping that userspace will > > figure things out. > > > > Note that wrt the last point, the kernel is the one which should have all > > the hardware > > knowledge to do this properly, after all hardware abstraction is one of the > > tasks of > > the kernel. > > > > I realize moving this more into the kernel, and tying things into drm is in > > no means > > easy, but it is about time we clean up this mess. > > > > Note that although I'm kicking of this discussion, my focus within the > > graphics team is > > mostly on input devices, so I'm hoping that someone else will pick things > > up once we've > > a better idea of how we would like to solve this. > > > > I hate to respond with yeah no, but yeah no, > > http://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&show_html=true&highlight_names=&date=2014-02-04 > http://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&show_html=true&highlight_names=&date=2014-02-05 > > read down that until you see me and tagr talking, read it a few times, > and the follow on chat with dvdhrm. > > The biggest problem with leaving the kernel to pick the correct one, > is the kernel simply never knows which is the > correct one, That could be solved by still allowing userspace to change the connection between the property and the actual backlight device. With the prop approach + atomic modeset you could also do some slightly fancier things like changing the backlight at the same time as doing a modeset or just adjusting some other display related properties. > and you also have to deal with a load of problems like > runtime module deps between very misc modules > or using some kind of notifier mechanism that will turn into a locking > nightmare. This would still be an issue though. I like the idea of the property, but I'm not volunteering to do any of the work. > > I don't mean to dissuade you from trying to "fix" this, but actually > getting a solution upstream is going to require a lot of messing > around. > > Talk to Matthew Garrett, if you haven't talked to him, then you > haven't talked to anyone who understands backlights. > > Dave. > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel OTC ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] xf86VGAarbiter,vgaHW: Only wrap co-operating VGA drivers
On Thu, Sep 12, 2013 at 12:37:44PM +0100, Chris Wilson wrote: > Presently, we wrap every single operation on every driver if the kernel > reports that there is more than one VGA capable device in the system. > This is irrespective of whether VGA is being used by any driver, and > causes a significant performance impact (4-5x) for CPU bound operations. > > The approach taken in this patch is to first only enable VGA arbitration > for drivers that require VGA resources. This is detected by moving the > initialisation from the common xf86 code to the vgaHW module. This is > strictly an ABI break as any driver that directly uses VGA (i.e. without > using the vgaHW module) will need to make its own declaration of intent. > Secondly, we then only wrap the operations with vgaarb get/put for the > drivers that require VGA access. If we only have a single driver > requring VGA access, we just wrap Enter/LeaveVT and lock the VGA > arbiter for the entire duration that the Xserver is active. It's not just VGA resource users that are affected. Let's consider a case where you have graphics cards A and B. A is pure VGA and needs VGA resources, B can do VGA but has a native driver, but it has no special way to disable VGA resource decoding other than flipping the PCI command register IO/mem space bits. Now if you tell the arbiter to permanently assign VGA resources to card A, driver for card B can no longer function. So all drivers that don't have a permanent VGA opt out mechanism would need to be modified to register w/ the arbiter. But maybe that's OK? I was actually slightly considering moving the VGA memory decode disable through the VGA misc register we added to i915 into the vga arbiter itself. It's a stadard VGA register, so everyone should be able to use it to opt out of VGA memory decoding and hence we'd never need to disable the entire memory space decoding. That would only leave IO space, but most hardware has a way to access register via MMIO, so IO space decode can often be permanently disabled without affecting functionality. -- Ville Syrjälä syrj...@sci.fi http://www.sci.fi/~syrjala/ ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/3] composite: Automatically enable backing store support on the screen
On Thu, Sep 12, 2013 at 12:04:14PM -0400, Adam Jackson wrote:
> ... unless you explicitly disabled it with -bs on the command line, or
> with the corresponding thing in xorg.conf.
>
> Signed-off-by: Adam Jackson
> ---
> composite/compinit.c | 9 +
> hw/xfree86/common/xf86Helper.c | 5 +
> 2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/composite/compinit.c b/composite/compinit.c
> index bc1130e..2001c8a 100644
> --- a/composite/compinit.c
> +++ b/composite/compinit.c
> @@ -115,13 +115,11 @@ compChangeWindowAttributes(WindowPtr pWin, unsigned
> long mask)
> pScreen->ChangeWindowAttributes = cs->ChangeWindowAttributes;
> ret = pScreen->ChangeWindowAttributes(pWin, mask);
>
> -if (ret && (mask & CWBackingStore) &&
> -pScreen->backingStoreSupport != NotUseful) {
So even if the server was started w/ -bs, you still allow the client to
enable backing store?
> +if (ret && (mask & CWBackingStore)) {
> if (pWin->backingStore != NotUseful) {
> compRedirectWindow(serverClient, pWin,
> CompositeRedirectAutomatic);
> pWin->backStorage = (pointer) (intptr_t) 1;
> -}
> -else {
> +} else {
> compUnredirectWindow(serverClient, pWin,
> CompositeRedirectAutomatic);
> pWin->backStorage = NULL;
> @@ -355,6 +353,9 @@ compScreenInit(ScreenPtr pScreen)
> return FALSE;
> }
>
> +if (!disableBackingStore)
> + pScreen->backingStoreSupport = WhenMapped;
> +
> cs->PositionWindow = pScreen->PositionWindow;
> pScreen->PositionWindow = compPositionWindow;
>
> diff --git a/hw/xfree86/common/xf86Helper.c b/hw/xfree86/common/xf86Helper.c
> index 4f1f3d4..70885e3 100644
> --- a/hw/xfree86/common/xf86Helper.c
> +++ b/hw/xfree86/common/xf86Helper.c
> @@ -1638,6 +1638,11 @@ xf86SetBackingStore(ScreenPtr pScreen)
> else {
> if (xf86GetOptValBool(options, OPTION_BACKING_STORE, &useBS))
> from = X_CONFIG;
> +#ifdef COMPOSITE
> + if (from != X_CONFIG)
> + useBS = xf86ReturnOptValBool(options, OPTION_BACKING_STORE,
> + !noCompositeExtension);
> +#endif
> }
> free(options);
> pScreen->backingStoreSupport = useBS ? WhenMapped : NotUseful;
> --
> 1.8.3.1
>
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
--
Ville Syrjälä
syrj...@sci.fi
http://www.sci.fi/~syrjala/
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 3/3] composite: Don't double-redirect if someone asks for backing store twice
On Thu, Sep 12, 2013 at 12:04:15PM -0400, Adam Jackson wrote:
> Signed-off-by: Adam Jackson
> ---
> composite/compinit.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/composite/compinit.c b/composite/compinit.c
> index 2001c8a..7f8d7cb 100644
> --- a/composite/compinit.c
> +++ b/composite/compinit.c
> @@ -116,10 +116,10 @@ compChangeWindowAttributes(WindowPtr pWin, unsigned
> long mask)
> ret = pScreen->ChangeWindowAttributes(pWin, mask);
>
> if (ret && (mask & CWBackingStore)) {
> -if (pWin->backingStore != NotUseful) {
> +if (pWin->backingStore != NotUseful && !pWin->backStorage) {
> compRedirectWindow(serverClient, pWin,
> CompositeRedirectAutomatic);
> pWin->backStorage = (pointer) (intptr_t) 1;
> -} else {
> +} else if (pWin->backStorage) {
Shouldn't the check be
if (pWin->backingStore == NotUseful && pWin->backStorage) ?
> compUnredirectWindow(serverClient, pWin,
> CompositeRedirectAutomatic);
> pWin->backStorage = NULL;
> --
> 1.8.3.1
>
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
--
Ville Syrjälä
syrj...@sci.fi
http://www.sci.fi/~syrjala/
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/3] bs: Set the screen's bs support level to WhenMapped not Always
On Thu, Sep 12, 2013 at 12:04:13PM -0400, Adam Jackson wrote: > Since we're using RedirectAutomatic to do this, we don't actually > preserve contents when unmapped. > > Signed-off-by: Adam Jackson > --- > dix/window.c | 2 +- > hw/xfree86/common/xf86Helper.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/dix/window.c b/dix/window.c > index 9fa51c2..1bd7d8b 100644 > --- a/dix/window.c > +++ b/dix/window.c > @@ -530,7 +530,7 @@ CreateRootWindow(ScreenPtr pScreen) > if (disableBackingStore) > pScreen->backingStoreSupport = NotUseful; > if (enableBackingStore) > -pScreen->backingStoreSupport = Always; > +pScreen->backingStoreSupport = WhenMapped; So if you start the server w/ +bs it will report backing store as supported even if the composite extension is never initialized? > > pScreen->saveUnderSupport = NotUseful; > > diff --git a/hw/xfree86/common/xf86Helper.c b/hw/xfree86/common/xf86Helper.c > index 721159d..4f1f3d4 100644 > --- a/hw/xfree86/common/xf86Helper.c > +++ b/hw/xfree86/common/xf86Helper.c > @@ -1640,7 +1640,7 @@ xf86SetBackingStore(ScreenPtr pScreen) > from = X_CONFIG; > } > free(options); > -pScreen->backingStoreSupport = useBS ? Always : NotUseful; > +pScreen->backingStoreSupport = useBS ? WhenMapped : NotUseful; > if (serverGeneration == 1) > xf86DrvMsg(pScreen->myNum, from, "Backing store %s\n", > useBS ? "enabled" : "disabled"); > -- > 1.8.3.1 > > _______ > xorg-devel@lists.x.org: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: http://lists.x.org/mailman/listinfo/xorg-devel -- Ville Syrjälä syrj...@sci.fi http://www.sci.fi/~syrjala/ ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: An attempt at fixing a bug involving DGA and mouse buttons
On Sat, Aug 25, 2012 at 04:47:42PM -0500, Steven Elliott wrote:
> On Fri, 2012-08-24 at 12:29 +1000, Peter Hutterer wrote:
> > we can't drop this block, it is central to button handling with multiple
> > devices. right now, if you hold a button on the mouse and the touchpad, the
> > logical button will only be released once _both_ buttons are up. this is
> > behaviour we had since 1.7 (at least), I don't really want to change it.
> >
> > A better fix is what Ville suggested, that sounds like the right thing to
> > do.
>
> The attached patch does that. It no longer removes the block of code in
> question. It moves only the
> if (!IsMaster(keybd))
>return;
> from inside DGAHandleEvent() to inside DGAProcessKeyboardEvent() and
> DGAProcessPointerEvent() after the call to UpdateDeviceState() in those
> functions like Ville suggested.
>
> I've attached an updated patch which fixes the problem for me. This
> time I generated the patch with the latest from git with
> git-format-patch in the interest of following the patch submission
> guidelines a bit more than I did last time.
Hmm. What happened to this patch? Looks like it never got applied to
master.
>
> --
>
> | Steven Elliott | http://selliott.org | sellio...@austin.rr.com |
>
> From a31fb95821d0308dd6882eb67429d39e851e29b4 Mon Sep 17 00:00:00 2001
> From: Steven Elliott
> Date: Sat, 25 Aug 2012 15:44:33 -0500
> Subject: [PATCH] Fixes a problem where mouse buttons get stuck
> down when DGA is engaged (has a handler).
>
> ---
> hw/xfree86/common/xf86DGA.c | 10 +++---
> 1 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/hw/xfree86/common/xf86DGA.c b/hw/xfree86/common/xf86DGA.c
> index a441dee..1af8d2e 100644
> --- a/hw/xfree86/common/xf86DGA.c
> +++ b/hw/xfree86/common/xf86DGA.c
> @@ -1033,6 +1033,9 @@ DGAProcessKeyboardEvent(ScreenPtr pScreen, DGAEvent *
> event, DeviceIntPtr keybd)
>
> UpdateDeviceState(keybd, &ev);
>
> +if (!IsMaster(keybd))
> + return;
> +
> /*
> * Deliver the DGA event
> */
> @@ -1074,6 +1077,7 @@ DGAProcessPointerEvent(ScreenPtr pScreen, DGAEvent *
> event, DeviceIntPtr mouse)
> DeviceEvent ev = {
> .header = ET_Internal,
> .length = sizeof(ev),
> +.detail.key = event->detail,
> .type = event->subtype,
> .corestate = butc ? butc->state : 0
> };
> @@ -1083,6 +1087,9 @@ DGAProcessPointerEvent(ScreenPtr pScreen, DGAEvent *
> event, DeviceIntPtr mouse)
>
> UpdateDeviceState(mouse, &ev);
>
> +if (!IsMaster(mouse))
> + return;
> +
> /*
> * Deliver the DGA event
> */
> @@ -1190,9 +1197,6 @@ DGAHandleEvent(int screen_num, InternalEvent *ev,
> DeviceIntPtr device)
> if (!pScreenPriv)
> return;
>
> -if (!IsMaster(device))
> -return;
> -
> switch (event->subtype) {
> case KeyPress:
> case KeyRelease:
> --
> 1.7.7.6
>
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
--
Ville Syrjälä
syrj...@sci.fi
http://www.sci.fi/~syrjala/
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 5/5] dix: Remove MapUnmapEventsEnabled and friends
On Thu, Sep 20, 2012 at 11:16:26AM -0400, Adam Jackson wrote: > This hack was added to suppress events generated by Composite's internal > unmap/map cycle on redirection state change. Since that cycle was > removed in 193ecc8b4, these can go. I sent an identical patch a long time ago, but I suppose it fell through the cracks. But feel free to go with your version ;) Reviewed-by: Ville Syrjälä -- Ville Syrjälä syrj...@sci.fi http://www.sci.fi/~syrjala/ ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: An attempt at fixing a bug involving DGA and mouse buttons
On Sat, Aug 18, 2012 at 08:46:01PM -0500, Steven Elliott wrote:
> I'd like to describe a bug I've encountered involving mouse events and
> DGA as well as my attempt at fixing it. Hopefully either my patch is
> acceptable, or people will have ideas about how it can be improved.
> I'll mention some specifics about my setup at the end.
>
> Recently I debugged a problem where an application I used seemed to
> break the X Server so that it was no longer possible to cause windows to
> gain focus by clicking on them. The application is MCEdit. It's a
> pygame based python program that edits Minecraft maps.
>
> In MCEdit clicking the right mouse button engages a freelook mode so
> that moving the mouse changes the user's perspective instead of moving
> the mouse pointer. Engaging freelook mode in MCEdit causes the problem
> which persists after MCEdit has been shutdown.
>
> Debugging has revealed that the reason I'm no longer able cause windows
> to gain focus by clicking on them once freelook mode has been used in
> MCEdit is that device->button->buttonsDown ("buttonsDown" only occurs in
> one structure) is not decremented when the mouse button is released
> which causes CheckDeviceGrabs() to return early instead of sending the
> mouse button event to the window being clicked on.
>
> Further debugging revealed that the reason buttonsDown is not
> decremented when the mouse is released has to do with the fact that DGA
> mode is active during freelook mode. When DGA mode is active (when it
> has registered a hook that is called for events so that DGAHandleEvent()
> is called for keyboard and mouse events instead of
> ProcessKeyboardEvent() and ProcessPointerEvent()) the following seems to
> be true:
> 1) DGAProcessPointerEvent() calls UpdateDeviceState() with mouse
> button events where ev.detail.key is not set.
> 2) DGAHandleEvent() refuses to process events that are not master
> events:
> if (!IsMaster(device))
> return;
> which causes UpdateDeviceState() to ignore the master event as it
> thinks the child event has not been processed.
I added this early return because otherwise I would get multiple DGA
events for each button/key press/release.
Unfortunately I don't know the input code very well, so I can't say
whether your patch is he right approach or not. But I wonder if it
would be enough to move the master check to happen inside
DGAProcessKeyboardEvent() and DGAProcessPointerEvent(), after
they've called UpdateDeviceState().
--
Ville Syrjälä
syrj...@sci.fi
http://www.sci.fi/~syrjala/
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] dix: Remove DisableMapUnmapEvents code
On Sun, Dec 18, 2011 at 06:26:04PM +0200, Ville Syrjälä wrote: > This is now dead code. The only user was eliminated in > commit 193ecc8b453b22b3e60248b9354c768dbd405598. > > Signed-off-by: Ville Syrjälä Ping? -- Ville Syrjälä syrj...@sci.fi http://www.sci.fi/~syrjala/ ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 1/4] dri2: Initialize needInvalidate member of DRI2Drawable.
From: Rami Ylimäki
If the client is not behaving correctly and swaps buffers before
getting them, Valgrind will complain about uninitialized memory being
used in DRI2InvalidateDrawable.
Signed-off-by: Rami Ylimäki
Reviewed-by: Ville Syrjälä
---
hw/xfree86/dri2/dri2.c |1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index 0d613be..d2035a4 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -181,6 +181,7 @@ DRI2AllocateDrawable(DrawablePtr pDraw)
pPriv->last_swap_ust = 0;
list_init(&pPriv->reference_list);
pPriv->serialNumber = DRI2DrawableSerial(pDraw);
+pPriv->needInvalidate = FALSE;
if (pDraw->type == DRAWABLE_WINDOW) {
pWin = (WindowPtr) pDraw;
--
1.7.3.4
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 4/4] dri2: Invalidate window pixmaps
While a redirected window is flipped, its pixmap may still be used as
and EGL image and should also get invalidated. When sending invalidate
events for a window, also send the events for its pixmap.
Signed-off-by: Ville Syrjälä
---
glx/glxdri2.c |2 +-
hw/xfree86/dri2/dri2.c|3 ++-
hw/xfree86/dri2/dri2.h|3 ++-
hw/xfree86/dri2/dri2ext.c |4 ++--
4 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/glx/glxdri2.c b/glx/glxdri2.c
index 8187a3e..5e524db 100644
--- a/glx/glxdri2.c
+++ b/glx/glxdri2.c
@@ -423,7 +423,7 @@ __glXDRIscreenCreateContext(__GLXscreen *baseScreen,
}
static void
-__glXDRIinvalidateBuffers(DrawablePtr pDraw, void *priv)
+__glXDRIinvalidateBuffers(DrawablePtr pDraw, void *priv, XID id)
{
#if __DRI2_FLUSH_VERSION >= 3
__GLXDRIdrawable *private = priv;
diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index 86e98a5..d6441a2 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -587,7 +587,7 @@ DRI2InvalidateDrawable(DrawablePtr pDraw)
pPriv->needInvalidate = FALSE;
list_for_each_entry(ref, &pPriv->reference_list, link)
- ref->invalidate(pDraw, ref->priv);
+ ref->invalidate(pDraw, ref->priv, ref->id);
}
/*
@@ -958,6 +958,7 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, CARD64
target_msc,
* windows using the same pixmap
*/
TraverseTree(pWin, DRI2InvalidateWalk, pPixmap);
+ DRI2InvalidateDrawable(&pPixmap->drawable);
} else
DRI2InvalidateDrawable(pDraw);
diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h
index 9c93209..a67e35f 100644
--- a/hw/xfree86/dri2/dri2.h
+++ b/hw/xfree86/dri2/dri2.h
@@ -166,7 +166,8 @@ typedef int (*DRI2ScheduleWaitMSCProcPtr)(ClientPtr
client,
CARD64 remainder);
typedef void (*DRI2InvalidateProcPtr)(DrawablePtr pDraw,
-void *data);
+void *data,
+XID id);
/**
* DRI2 calls this hook when ever swap_limit is going to be changed. Default
diff --git a/hw/xfree86/dri2/dri2ext.c b/hw/xfree86/dri2/dri2ext.c
index e612cf0..73ef7f2 100644
--- a/hw/xfree86/dri2/dri2ext.c
+++ b/hw/xfree86/dri2/dri2ext.c
@@ -156,13 +156,13 @@ ProcDRI2Authenticate(ClientPtr client)
}
static void
-DRI2InvalidateBuffersEvent(DrawablePtr pDraw, void *priv)
+DRI2InvalidateBuffersEvent(DrawablePtr pDraw, void *priv, XID id)
{
xDRI2InvalidateBuffers event;
ClientPtr client = priv;
event.type = DRI2EventBase + DRI2_InvalidateBuffers;
-event.drawable = pDraw->id;
+event.drawable = id;
WriteEventsToClient(client, 1, (xEvent *)&event);
}
--
1.7.3.4
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 2/4] dri2: Always re-generate front buffer information when asked for it.
From: Michel Dänzer
Otherwise we might keep stale cached information, e.g. after the driver
performed page flipping.
This is part of the fix for
https://bugs.freedesktop.org/show_bug.cgi?id=35452 .
Signed-off-by: Michel Dänzer
Reviewed-by: Ville Syrjälä
Reviewed-by: Mario Kleiner
---
hw/xfree86/dri2/dri2.c |1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index d2035a4..eb84aa5 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -377,6 +377,7 @@ allocate_or_reuse_buffer(DrawablePtr pDraw, DRI2ScreenPtr
ds,
int old_buf = find_attachment(pPriv, attachment);
if ((old_buf < 0)
+ || attachment == DRI2BufferFrontLeft
|| !dimensions_match
|| (pPriv->buffers[old_buf]->format != format)) {
*buffer = (*ds->CreateBuffer)(pDraw, attachment, format);
--
1.7.3.4
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
DRI2 invalidate patches
A bunch of DRI2 invalidate event related patches that fell through the cracks. ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 3/4] dri2: Invalidate DRI2 buffers for all windows with the same pixmap on swap
From: Keith Packard
Without this, when a compositing manager unredirects a fullscreen window which
uses DRI2 and page flipping, the DRI2 buffer information for the compositing
manager's output window (typically the Composite Overlay Window or root window)
may become stale, resulting in all kinds of hilarity.
Fixes https://bugs.freedesktop.org/show_bug.cgi?id=35452 .
[Original patch by Michel Dänzer ]
[Tree walk optimized version by Keith Packard ]
Signed-off-by: Ville Syrjälä
---
hw/xfree86/dri2/dri2.c | 31 ++-
1 files changed, 30 insertions(+), 1 deletions(-)
diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index eb84aa5..86e98a5 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -830,6 +830,19 @@ DRI2WaitSwap(ClientPtr client, DrawablePtr pDrawable)
return FALSE;
}
+/*
+ * A TraverseTree callback to invalidate all windows using the same
+ * pixmap
+ */
+static int
+DRI2InvalidateWalk(WindowPtr pWin, pointer data)
+{
+if (pWin->drawable.pScreen->GetWindowPixmap(pWin) != data)
+ return WT_DONTWALKCHILDREN;
+DRI2InvalidateDrawable(&pWin->drawable);
+return WT_WALKCHILDREN;
+}
+
int
DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, CARD64 target_msc,
CARD64 divisor, CARD64 remainder, CARD64 *swap_target,
@@ -930,7 +943,23 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw,
CARD64 target_msc,
*/
*swap_target = pPriv->swap_count + pPriv->swapsPending;
-DRI2InvalidateDrawable(pDraw);
+if (pDraw->type == DRAWABLE_WINDOW) {
+ WindowPtr pWin = (WindowPtr) pDraw;
+ PixmapPtr pPixmap = pScreen->GetWindowPixmap(pWin);
+
+ /*
+* Find the top-most window using this pixmap
+*/
+ while (pWin->parent && pScreen->GetWindowPixmap(pWin->parent) ==
pPixmap)
+ pWin = pWin->parent;
+
+ /*
+* Walk the sub-tree to invalidate all of the
+* windows using the same pixmap
+*/
+ TraverseTree(pWin, DRI2InvalidateWalk, pPixmap);
+} else
+ DRI2InvalidateDrawable(pDraw);
return Success;
}
--
1.7.3.4
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH] dix: Remove DisableMapUnmapEvents code
This is now dead code. The only user was eliminated in
commit 193ecc8b453b22b3e60248b9354c768dbd405598.
Signed-off-by: Ville Syrjälä
---
dix/window.c | 33 +++--
include/window.h |5 -
2 files changed, 3 insertions(+), 35 deletions(-)
diff --git a/dix/window.c b/dix/window.c
index 1953f02..ddde68a 100644
--- a/dix/window.c
+++ b/dix/window.c
@@ -2673,30 +2673,6 @@ RealizeTree(WindowPtr pWin)
}
}
-static WindowPtr windowDisableMapUnmapEvents;
-
-void
-DisableMapUnmapEvents(WindowPtr pWin)
-{
-assert (windowDisableMapUnmapEvents == NULL);
-
-windowDisableMapUnmapEvents = pWin;
-}
-
-void
-EnableMapUnmapEvents(WindowPtr pWin)
-{
-assert (windowDisableMapUnmapEvents != NULL);
-
-windowDisableMapUnmapEvents = NULL;
-}
-
-static Bool
-MapUnmapEventsEnabled(WindowPtr pWin)
-{
-return pWin != windowDisableMapUnmapEvents;
-}
-
/*
* MapWindow
*If some other client has selected SubStructureReDirect on the parent
@@ -2742,7 +2718,7 @@ MapWindow(WindowPtr pWin, ClientPtr client)
}
pWin->mapped = TRUE;
- if (SubStrSend(pWin, pParent) && MapUnmapEventsEnabled(pWin))
+ if (SubStrSend(pWin, pParent))
{
memset(&event, 0, sizeof(xEvent));
event.u.u.type = MapNotify;
@@ -2903,8 +2879,7 @@ UnrealizeTree(
}
#endif
(* Unrealize)(pChild);
- if (MapUnmapEventsEnabled(pWin))
- DeleteWindowFromAnyEvents(pChild, FALSE);
+ DeleteWindowFromAnyEvents(pChild, FALSE);
if (pChild->viewable)
{
pChild->viewable = FALSE;
@@ -2944,7 +2919,7 @@ UnmapWindow(WindowPtr pWin, Bool fromConfigure)
if ((!pWin->mapped) || (!(pParent = pWin->parent)))
return Success;
-if (SubStrSend(pWin, pParent) && MapUnmapEventsEnabled(pWin))
+if (SubStrSend(pWin, pParent))
{
memset(&event, 0, sizeof(xEvent));
event.u.u.type = UnmapNotify;
@@ -3147,8 +3122,6 @@ SendVisibilityNotify(WindowPtr pWin)
xEvent event;
unsigned int visibility = pWin->visibility;
-if (!MapUnmapEventsEnabled(pWin))
-return;
#ifdef PANORAMIX
/* This is not quite correct yet, but it's close */
if(!noPanoramiXExtension) {
diff --git a/include/window.h b/include/window.h
index e13598b..4f09d7c 100644
--- a/include/window.h
+++ b/include/window.h
@@ -261,11 +261,6 @@ extern _X_EXPORT RegionPtr CreateBoundingShape(
extern _X_EXPORT RegionPtr CreateClipShape(
WindowPtr /* pWin */ );
-extern _X_EXPORT void DisableMapUnmapEvents(
-WindowPtr /* pWin */ );
-extern _X_EXPORT void EnableMapUnmapEvents(
-WindowPtr /* pWin */ );
-
extern _X_EXPORT void SetRootClip(ScreenPtr pScreen, Bool enable);
extern _X_EXPORT void PrintWindowTree(void);
--
1.7.3.4
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] composite: Update borderClip in compAllocPixmap()
On Wed, Oct 19, 2011 at 05:26:09PM -0700, Keith Packard wrote: > On Sun, 9 Oct 2011 01:11:04 +0300, Ville Syrjala wrote: > > > Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=22566 > > Merged. >e4787ec..a5266dc master -> master Cheers. Jeremy, I assume you want to cherry-pick this for 1.11... -- Ville Syrjälä syrj...@sci.fi http://www.sci.fi/~syrjala/ ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PULL xserver] Composite borderClip fix
The following changes since commit c8413362049cee8c30e0a9d67f78f9ebefe8e71f: Merge remote-tracking branch 'herrb/master' (2011-10-18 07:45:24 -0700) are available in the git repository at: git://gitorious.org/vsyrjala/xserver.git composite_borderclip Ville Syrjala (1): composite: Update borderClip in compAllocPixmap() composite/compalloc.c | 11 +++ 1 files changed, 7 insertions(+), 4 deletions(-) ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] Revert "composite: Don't backfill non-MapWindow allocations"
On Thu, Jul 28, 2011 at 01:00:54PM -0700, Pierre-Loup A. Griffais wrote:
> Oops, initially got an outdated address for Ville. Properly CCing now; the
> idea
> is that 193ecc8b453b22b3e6 breaks backfilling on resize if db8840600e8e213562
> is
> also applied.
The problem only affects bg=None windows, right?
Might be nice to get 6dd775f57d2f94f0ddaee324aeec33b9b66ed5bc back to
avoid the copy for bg!=None cases. But I suppose no-one has had the
time/incentive to look at the issue in more detail.
>
> Thanks,
> - Pierre-Loup
>
> On 07/28/2011 12:58 PM, Pierre-Loup A. Griffais wrote:
> > This reverts commit db8840600e8e21356241eb87395031388d9b54d2.
> >
> > Conflicts:
> >
> > composite/compalloc.c
> > ---
> > composite/compalloc.c | 10 +++---
> > 1 files changed, 3 insertions(+), 7 deletions(-)
> >
> > diff --git a/composite/compalloc.c b/composite/compalloc.c
> > index 5c27631..841b2dc 100644
> > --- a/composite/compalloc.c
> > +++ b/composite/compalloc.c
> > @@ -557,7 +557,7 @@ compUnredirectOneSubwindow (WindowPtr pParent,
> > WindowPtr pWin)
> > }
> >
> > static PixmapPtr
> > -compNewPixmap (WindowPtr pWin, int x, int y, int w, int h, Bool map)
> > +compNewPixmap (WindowPtr pWin, int x, int y, int w, int h)
> > {
> > ScreenPtr pScreen = pWin->drawable.pScreen;
> > WindowPtr pParent = pWin->parent;
> > @@ -572,10 +572,6 @@ compNewPixmap (WindowPtr pWin, int x, int y, int w,
> > int h, Bool map)
> > pPixmap->screen_x = x;
> > pPixmap->screen_y = y;
> >
> > -/* resize allocations will update later in compCopyWindow, not here */
> > -if (!map)
> > - return pPixmap;
> > -
> > if (pParent->drawable.depth == pWin->drawable.depth)
> > {
> > GCPtr pGC = GetScratchGC (pWin->drawable.depth, pScreen);
> > @@ -641,7 +637,7 @@ compAllocPixmap (WindowPtr pWin)
> > int y = pWin->drawable.y - bw;
> > int w = pWin->drawable.width + (bw<< 1);
> > int h = pWin->drawable.height + (bw<< 1);
> > -PixmapPtr pPixmap = compNewPixmap (pWin, x, y, w, h, TRUE);
> > +PixmapPtr pPixmap = compNewPixmap (pWin, x, y, w, h);
> > CompWindowPtr cw = GetCompWindow (pWin);
> >
> > if (!pPixmap)
> > @@ -713,7 +709,7 @@ compReallocPixmap (WindowPtr pWin, int draw_x, int
> > draw_y,
> > pix_h = h + (bw<< 1);
> > if (pix_w != pOld->drawable.width || pix_h != pOld->drawable.height)
> > {
> > - pNew = compNewPixmap (pWin, pix_x, pix_y, pix_w, pix_h, FALSE);
> > + pNew = compNewPixmap (pWin, pix_x, pix_y, pix_w, pix_h);
> > if (!pNew)
> > return FALSE;
> > cw->pOldPixmap = pOld;
--
Ville Syrjälä
syrj...@sci.fi
http://www.sci.fi/~syrjala/
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] composite: Inhibit window background paint with manual subwindow redirection
On Sat, Jul 23, 2011 at 04:00:11PM +0300, Ville Syrjälä wrote: > On Fri, Jul 22, 2011 at 02:49:54PM -0400, Owen Taylor wrote: > > On Thu, 2011-07-21 at 00:24 +0300, Ville Syrjälä wrote: > > > > I did actually have to see it live to figure out what was going on ... > > > > but once I saw it, and thought about it a bit, I was able to avoid > > > > digging out the debugger. > > > > > > > > What's going on is that for a window with a non-None background, the > > > > copying between the parent pixmap and the composite pixmap that we do > > > > isn't sufficient - all that copying is doing is making sure that things > > > > _look_ right initially - it doesn't actually suppress expose event > > > > generation and the background painting that happens during expose event > > > > generation. > > > > > > The pointless expose events were supposed to be eliminated by another > > > patch set [1]. At least my test app [2] manages to redirect/unredirect > > > non-None windows w/o generating unnecessary expose events. > > > > Hmm, OK, I had those patches applied, but figured that the expose events > > were being generated by some other path than Map/Unmap. It turns out > > that they were being generated because my test Mutter code wasn't > > ordering things correctly. > > > > With that fixed, I can't reproduce any flickering with Evince. It > > redirects and unredirects seamlessly when I pop up a menu. (This is with > > GTK+-3, but there shouldn't be any difference from GTK+-2 in this > > regard.) > > > > So, I'm not sure what was happening in your testing but I'm pretty happy > > with how things are working for me! > > I suppose it's possible compiz does things in the wrong order as well. I > never bothered to investigate the issue properly since compiz and GTK+ > weren't my real target with these patches. I had a quick look at compiz and indeed the problem was a simple ordering issue. I sent patches [1] to fix it. [1] http://lists.compiz.org/pipermail/dev/2011-July/001465.html -- Ville Syrjälä syrj...@sci.fi http://www.sci.fi/~syrjala/ ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] composite: Inhibit window background paint with manual subwindow redirection
On Fri, Jul 22, 2011 at 02:49:54PM -0400, Owen Taylor wrote: > On Thu, 2011-07-21 at 00:24 +0300, Ville Syrjälä wrote: > > > I did actually have to see it live to figure out what was going on ... > > > but once I saw it, and thought about it a bit, I was able to avoid > > > digging out the debugger. > > > > > > What's going on is that for a window with a non-None background, the > > > copying between the parent pixmap and the composite pixmap that we do > > > isn't sufficient - all that copying is doing is making sure that things > > > _look_ right initially - it doesn't actually suppress expose event > > > generation and the background painting that happens during expose event > > > generation. > > > > The pointless expose events were supposed to be eliminated by another > > patch set [1]. At least my test app [2] manages to redirect/unredirect > > non-None windows w/o generating unnecessary expose events. > > Hmm, OK, I had those patches applied, but figured that the expose events > were being generated by some other path than Map/Unmap. It turns out > that they were being generated because my test Mutter code wasn't > ordering things correctly. > > With that fixed, I can't reproduce any flickering with Evince. It > redirects and unredirects seamlessly when I pop up a menu. (This is with > GTK+-3, but there shouldn't be any difference from GTK+-2 in this > regard.) > > So, I'm not sure what was happening in your testing but I'm pretty happy > with how things are working for me! I suppose it's possible compiz does things in the wrong order as well. I never bothered to investigate the issue properly since compiz and GTK+ weren't my real target with these patches. -- Ville Syrjälä syrj...@sci.fi http://www.sci.fi/~syrjala/ ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] composite: Inhibit window background paint with manual subwindow redirection
On Wed, Jul 20, 2011 at 05:06:24PM -0400, Owen Taylor wrote: > On Wed, 2011-07-20 at 10:44 -0400, Owen Taylor wrote: > > On Wed, 2011-07-20 at 02:54 +0300, Ville Syrjälä wrote: > > > > > BTW I noticed afterwards that this doesn't help all apps. Evince, for > > > example, still blinks whenever I bring up the popup menu while in > > > fullscreen mode, whereas Firefox doesn't blink. I did some quick > > > protocol tracing and I saw some window background mode changes from > > > the client. A quick look at gdk-x11 code revealed that it does some > > > tricks with the window background. I never got around to testing if > > > removing that code would make Evince blink-free (tm). > > > > Interesting... there are certainly tricks being played there, but it's > > hard to see why they would cause blinking. > [...] > > The only thing I can think of is that evince might be changing it's size > > when popping up the menu, but that seems pretty strange. Hard to guess > > at without a live situation to debug. > > I did actually have to see it live to figure out what was going on ... > but once I saw it, and thought about it a bit, I was able to avoid > digging out the debugger. > > What's going on is that for a window with a non-None background, the > copying between the parent pixmap and the composite pixmap that we do > isn't sufficient - all that copying is doing is making sure that things > _look_ right initially - it doesn't actually suppress expose event > generation and the background painting that happens during expose event > generation. The pointless expose events were supposed to be eliminated by another patch set [1]. At least my test app [2] manages to redirect/unredirect non-None windows w/o generating unnecessary expose events. [1] http://lists.x.org/archives/xorg-devel/2011-April/021450.html [2] http://gitorious.org/vsyrjala/xwindow_check -- Ville Syrjälä syrj...@sci.fi http://www.sci.fi/~syrjala/ ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] composite: Inhibit window background paint with manual subwindow redirection
On Tue, Jul 19, 2011 at 06:01:21PM -0400, Owen Taylor wrote: > On Fri, 2011-05-06 at 18:19 +0300, ville.syrj...@nokia.com wrote: > > From: Ville Syrjälä > > > > The composite extension spec says that window background painting > > should be inhibited when the subwindow redirection mode is set to > > manual. > > > > This eliminates the ugly flashing effect when compiz unredirects a > > fullscreen window. > > Being able to do no-flash unredirection is something I'd definitely like > to be able to do for Mutter! BTW I noticed afterwards that this doesn't help all apps. Evince, for example, still blinks whenever I bring up the popup menu while in fullscreen mode, whereas Firefox doesn't blink. I did some quick protocol tracing and I saw some window background mode changes from the client. A quick look at gdk-x11 code revealed that it does some tricks with the window background. I never got around to testing if removing that code would make Evince blink-free (tm). > Hmm, OK, so what you are doing is: > > - Unmap or reshape COW, leaving root window contents, which you >don't want to be repainted. (And you can't get this by changing >the background to None because: "Changing the background of a root >window to None or ParentRelative restores the default background >pixmap.") > - Unredirect fullscreen window > > it took me a moment to see why this isn't backwards ordering: if you > unredirected the fullscreen window first it wouldn't have preserved > contents, so the X server would have to expose it from scratch. Yeah. One workaround would be to keep one bg=None window always just above the root window. > This patch does look like it makes the behavior agree with the composite > protocol extension; it's not clear at this point whether there was > always a mismatch or whether it was changed at some point: > > History in the compositeproto repo shows: > > commit 35a9c80252b35720c1afc5dc53153228e2084b10 > Author: Keith Packard > Date: Sun Nov 9 07:07:21 2003 + > > Note that Manual Subwindows mode disables background painting. > > but the X server code from that date wasn't easily discoverable for me. Yeah tracking down the old code is difficult. I _think_ I did track down the original CVS commits for some composite code. Ah now I remember. It was related to the initial damage in ProcCreateDamage(), but the original commit message didn't provide much rationale for the change. Same story with some Xv code I was trying to figure out :( > Patch looks correct - it makes windows with manual subwindow > redirection go through the same codepath as background none windows. > > It seems pretty safe - very little application painting code *depends* > on clearing to the background color, and breakage from this change would > require that plus an application that was doing something pretty unusual > with the composite extension. > > Would be definitely good to get Keith to check this as well if possible. > > - Owen > > Reviewed-by: Owen Taylor Thanks. -- Ville Syrjälä syrj...@sci.fi http://www.sci.fi/~syrjala/ ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xf86-video-ati 2/2] kms: Move flip_count and co. to a per swap structure
On Thu, Jun 09, 2011 at 08:08:24PM -0700, Stéphane Marchesin wrote:
> On Wed, May 4, 2011 at 13:51, Ville Syrjala wrote:
> > If multiple drawables are doing page flipping, the global drmmode
> > structure can't be used to keep per swap information. For example
> > flip_count can increase prematurely due to another swap request,
> > and then the previous swap request never gets completed, leading to a
> > stuck client. Move the relevant pieces of data to a strucuture that
> > gets allocated once per swap request and shared by all involved CRTCs.
> >
> > Signed-off-by: Ville Syrjala
> > ---
> > src/drmmode_display.c | 45 ++---
> > src/drmmode_display.h | 10 +++---
> > 2 files changed, 33 insertions(+), 22 deletions(-)
> >
> > diff --git a/src/drmmode_display.c b/src/drmmode_display.c
> > index 7873d57..7dd5d86 100644
> > --- a/src/drmmode_display.c
> > +++ b/src/drmmode_display.c
> > @@ -1331,31 +1331,34 @@ drmmode_flip_handler(int fd, unsigned int frame,
> > unsigned int tv_sec,
> > unsigned int tv_usec, void *event_data)
> > {
> > drmmode_flipevtcarrier_ptr flipcarrier = event_data;
> > - drmmode_ptr drmmode = flipcarrier->drmmode;
> > + drmmode_flipdata_ptr flipdata = flipcarrier->flipdata;
> > + drmmode_ptr drmmode = flipdata->drmmode;
> >
> > /* Is this the event whose info shall be delivered to higher level?
> > */
> > if (flipcarrier->dispatch_me) {
> > /* Yes: Cache msc, ust for later delivery. */
> > - drmmode->fe_frame = frame;
> > - drmmode->fe_tv_sec = tv_sec;
> > - drmmode->fe_tv_usec = tv_usec;
> > + flipdata->fe_frame = frame;
> > + flipdata->fe_tv_sec = tv_sec;
> > + flipdata->fe_tv_usec = tv_usec;
> > }
> > free(flipcarrier);
> >
> > /* Last crtc completed flip? */
> > - drmmode->flip_count--;
> > - if (drmmode->flip_count > 0)
> > + flipdata->flip_count--;
> > + if (flipdata->flip_count > 0)
> > return;
> >
> > /* Release framebuffer */
> > - drmModeRmFB(drmmode->fd, drmmode->old_fb_id);
> > + drmModeRmFB(drmmode->fd, flipdata->old_fb_id);
> >
> > - if (drmmode->event_data == NULL)
> > + if (flipdata->event_data == NULL)
> > return;
> >
> > /* Deliver cached msc, ust from reference crtc to flip event handler
> > */
> > - radeon_dri2_flip_event_handler(drmmode->fe_frame,
> > drmmode->fe_tv_sec,
> > - drmmode->fe_tv_usec,
> > drmmode->event_data);
> > + radeon_dri2_flip_event_handler(flipdata->fe_frame,
> > flipdata->fe_tv_sec,
> > + flipdata->fe_tv_usec,
> > flipdata->event_data);
> > +
> > + free(flipdata);
> > }
> >
> >
> > @@ -1399,12 +1402,10 @@ Bool drmmode_pre_init(ScrnInfoPtr pScrn,
> > drmmode_ptr drmmode, int cpp)
> >
> > xf86InitialConfiguration(pScrn, TRUE);
> >
> > - drmmode->flip_count = 0;
> > drmmode->event_context.version = DRM_EVENT_CONTEXT_VERSION;
> > drmmode->event_context.vblank_handler = drmmode_vblank_handler;
> > drmmode->event_context.page_flip_handler = drmmode_flip_handler;
> > if (!pRADEONEnt->fd_wakeup_registered &&
> > info->dri->pKernelDRMVersion->version_minor >= 4) {
> > - drmmode->flip_count = 0;
> > AddGeneralSocket(drmmode->fd);
> > RegisterBlockAndWakeupHandlers((BlockHandlerProcPtr)NoopDDA,
> > drm_wakeup_handler, drmmode);
> > @@ -1654,6 +1655,7 @@ Bool radeon_do_pageflip(ScrnInfoPtr scrn, struct
> > radeon_bo *new_front, void *dat
> > int i, old_fb_id;
> > uint32_t tiling_flags = 0;
> > int height;
> > + drmmode_flipdata_ptr flipdata;
> > drmmode_flipevtcarrier_ptr flipcarrier;
> >
> > if (info->allowColorTiling) {
> > @@ -1676,6 +1678,12 @@ Bool radeon_do_pageflip(ScrnInfoPtr scrn, struct
> > radeon_bo *new_front, void *dat
> > new_front->handle, &drmmode->fb_id))
> > goto error_out;
> >
> > + flipdata = calloc(1, sizeof(drmmode_flipdata_rec));
>
> I think you leak that flipdata in the error_undo case.
Quite possible. The code already seemed to leak some stuff on
errors, and it didn't really seem to have proper roll back code
anyway in case of partial success, so I didn't pay much attention
to such details.
--
Ville Syrjälä
syrj...@sci.fi
http://www.sci.fi/~syrjala/
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xf86-video-ati 1/2] dri2: Update front buffer pixmap and name before exchanging buffers
On Thu, May 05, 2011 at 05:30:23PM +0300, Ville Syrjälä wrote:
> On Thu, May 05, 2011 at 12:28:20PM +0200, Michel Dänzer wrote:
> > On Mit, 2011-05-04 at 23:51 +0300, Ville Syrjala wrote:
> > > Buffer exchange assumes that the front buffer pixmap and name
> > > information is accurate. That may not be the case eg. if the window
> > > has been (un)redirected since the buffer was created.
> > >
> > > Signed-off-by: Ville Syrjala
> > > ---
> > > src/radeon_dri2.c | 45 -
> > > 1 files changed, 40 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
> > > index e618cc5..5d1a382 100644
> > > --- a/src/radeon_dri2.c
> > > +++ b/src/radeon_dri2.c
> > > @@ -626,12 +626,42 @@ radeon_dri2_schedule_flip(ScrnInfoPtr scrn,
> > > ClientPtr client,
> > > }
> > >
> > > static Bool
> > > -can_exchange(ScrnInfoPtr pScrn,
> > > +update_front(DrawablePtr draw, DRI2BufferPtr front)
> > > +{
> > > +int r;
> > > +PixmapPtr pixmap;
> > > +struct dri2_buffer_priv *priv = front->driverPrivate;
> > > +struct radeon_exa_pixmap_priv *driver_priv;
> > > +
> > > +if (draw->type == DRAWABLE_PIXMAP)
> > > + pixmap = (PixmapPtr)draw;
> > > +else
> > > + pixmap = (*draw->pScreen->GetWindowPixmap)((WindowPtr)draw);
> > > +
> > > +pixmap->refcnt++;
> > > +
> > > +exaMoveInPixmap(pixmap);
> > > +driver_priv = exaGetPixmapDriverPrivate(pixmap);
> > > +r = radeon_gem_get_kernel_name(driver_priv->bo, &front->name);
> > > +if (r) {
> > > + (*draw->pScreen->DestroyPixmap)(pixmap);
> > > + return FALSE;
> > > +}
> > > +(*draw->pScreen->DestroyPixmap)(priv->pixmap);
> > > +front->pitch = pixmap->devKind;
> > > +front->cpp = pixmap->drawable.bitsPerPixel / 8;
> > > +priv->pixmap = pixmap;
> > > +
> > > +return TRUE;
> > > +}
> >
> > Maybe at least some of this could be short-circuited if it's still the
> > same pixmap.
>
> Yeah, I had the same thought. There's one catch though; Even if it's the
> same pixmap, the bo may have been exchanged, and so the buffer name may
> need to be updated anyway.
Haven't had time to look at this at all. If no one else wants to take
this on I'd suggest merging these patches ASAP to make page flipping +
compiz fullscreen unredirect feature actually work.
--
Ville Syrjälä
syrj...@sci.fi
http://www.sci.fi/~syrjala/
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: X server merge window extended until 5/31?
On Wed, May 25, 2011 at 10:15:36AM -0600, Keith Packard wrote: > > I'm going to be off the network from Thursday afternoon until Monday > evening, which will make it a bit hard to merge any pending patches to > the server. > > I figured it would be nicer to just push the end of the merge window > until after I return instead of pulling it in by a day and a half. > > I think there are only a couple of active patches pending -- the fb > memcpy fix and XRes 1.2. And, the fb memcpy fix is really a bug fix, not > new functionality and so doesn't come under the merge window dictates. > > Let me know if I've missed anything that should be merged; I didn't see any comments about this patch of mine: Subject: [PATCH xserver] composite: Inhibit window background paint with manual subwindow redirection Message-Id: <1304695174-27663-1-git-send-email-ville.syrj...@nokia.com> Also the DRI2 patches you and Michel bounced around seem to have dropped through the cracks. -- Ville Syrjälä syrj...@sci.fi http://www.sci.fi/~syrjala/ ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] composite: Inhibit window background paint with manual subwindow redirection
On Fri, May 06, 2011 at 06:19:34PM +0300, ville.syrj...@nokia.com wrote: > From: Ville Syrjälä > > The composite extension spec says that window background painting > should be inhibited when the subwindow redirection mode is set to > manual. > > This eliminates the ugly flashing effect when compiz unredirects a > fullscreen window. Ping. Any comments on this? -- Ville Syrjälä syrj...@sci.fi http://www.sci.fi/~syrjala/ ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: More DRI2 invalidate stuff
On Mon, May 09, 2011 at 01:42:03PM -0700, ext Eric Anholt wrote: > On Fri, 6 May 2011 18:18:14 +0300, ville.syrj...@nokia.com wrote: > > The proposed DRI@ invalidate patch [1] causes a lot more invalidate events > > to be sent out. That will cause processes to wake up needlessly. Eg. if > > an unredirected fullscreen app is flipping, each flip will also send an > > invalidate event to the compositor, since the composite overlay and client > > window share the same pixmap. Now, assuming that the compositor hasn't > > done a GetBuffers yet, there's no need to send an invalidate event to it. > > > > I took the easy approach and stuck the boolean flag into the > > DRI2DrawableRec. > > The optimal solution would have been to put it into DRI2DrawableRefRec and > > keep track of the invalidate state per reference, but that would require the > > client's drawable ID and client ID so that the correct ref could be found in > > GetBuffers. Too much hassle for my taste, and I think this simple approach > > should provide equal benefit in most cases. > > Have you measured this to reduce invalidates? Yes, of course. Someone else at Nokia noticed the problem too, since soon after integrating the patch from [1] to our internal X server tree, I got a blocker bug saying that some unrelated processes get woken up while scrolling some list in one app. Good thing I had realized the problem already, so I had the fix ready. > I don't see how something > that is producing flip requests that produce invalidates will do so > without having called getbuffers in between. You're barking up the wrong drawable, so to speak. Of course the drawable being flipped needs its GetBuffers and invalidate events. It's all the other drawables which have a DRI2 drawable and share the same pixmap, that get hammered with the unwanted invalidate events. -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: xf86-video-ati page flipping fixes
On Thu, May 05, 2011 at 06:33:11PM +0200, ext Mario Kleiner wrote: > > On May 5, 2011, at 6:06 PM, Ville Syrjälä wrote: > > > On Thu, May 05, 2011 at 05:09:56PM +0200, Mario Kleiner wrote: > >> On May 5, 2011, at 4:23 PM, Ville Syrjälä wrote: > >> > >>> On Thu, May 05, 2011 at 11:46:45AM +0200, Michel Dänzer wrote: > >>>> On Mit, 2011-05-04 at 23:51 +0300, Ville Syrjala wrote: > >>>>> I came to the conclusion that the xserver DRI2 invalidate patches > >>>>> that > >>>>> have been discussed aren't really fixing the problem. I suppose > >>>>> they may > >>>>> make the problem slightly less likely to happen, but at least for > >>>>> me that > >>>>> likelyhood is still very high. The whole mess looks like a simple > >>>>> driver > >>>>> bug to me. > >>>> > >>>> I think the xserver patches are still necessary, otherwise how are > >>>> the > >>>> cached DRI2 pPriv->buffers updated for other windows sharing the > >>>> same > >>>> pixmap? > >>> > >>> As the real front buffers are not handed out to clients, there isn't > >>> that much reason to update them. Making sure the fake front buffer > >>> contain a more recent snapshot of the real front would be one reason > >>> though. > >>> > >> > >> Could it be that your patch fixes a closely related, but different > >> bug? During OpenGL rendering, Mesa/Gallium caches the backbuffer > >> assignment and only queries the current assignment from the x-server > >> if its drawable has been invalidated, which is what the x-server > >> patches do. > >> > >> I'd also think you will need both patch sets. > > > > Back buffers are private so a swap for one drawable doesn't need to > > invalidate them for other drawables. > > > > Are you sure this is also the case even when kms page flipping is > used for fullscreen drawables? Yes, and if not, it's a driver bug. > The workaround i implemented in my toolkit for the page flipping > problem when using fullscreen drawables on current x servers was to > make sure that it always ends a session with an even number of > completed page-flipping swaps, by checking 'sbc' and issuing an extra > swap before closing its window if the sbc wasn't an even number. This > makes sure that the same buffer objects are assigned as front- and > backbuffer at the time the window closes as when the application > started and opened its fullscreen window. This fixed any desktop > corruption i'd usually get when running compiz with non-redirected > fullscreen windows after quitting my application. > > I could be totally wrong, but at least the symptoms and the working > workaround suggested fullscreen drawables do share their back buffers > when page flipping is on. The bug allows the BOs to get mixed up pretty much any which way between DRI2 buffers. Eg. if two drawables got the same BO as the front buffer in their GetBuffers requests, and then both would do a page flip, both would end up with the same BO as their new back buffer. -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: xf86-video-ati page flipping fixes
On Thu, May 05, 2011 at 09:54:48AM -0700, Jesse Barnes wrote: > On Thu, 5 May 2011 18:33:11 +0200 > Mario Kleiner wrote: > > > Are you sure this is also the case even when kms page flipping is > > used for fullscreen drawables? > > > > The workaround i implemented in my toolkit for the page flipping > > problem when using fullscreen drawables on current x servers was to > > make sure that it always ends a session with an even number of > > completed page-flipping swaps, by checking 'sbc' and issuing an extra > > swap before closing its window if the sbc wasn't an even number. This > > makes sure that the same buffer objects are assigned as front- and > > backbuffer at the time the window closes as when the application > > started and opened its fullscreen window. This fixed any desktop > > corruption i'd usually get when running compiz with non-redirected > > fullscreen windows after quitting my application. > > > > I could be totally wrong, but at least the symptoms and the working > > workaround suggested fullscreen drawables do share their back buffers > > when page flipping is on. > > Is this workaround needed on the Intel driver too? Or is this just a > driver bo name tracking bug? I don't have Intel hw for testing, but as the code looks to be simply copy-pasted from one driver to the other, I'd say it needs the same fixes. -- Ville Syrjälä syrj...@sci.fi http://www.sci.fi/~syrjala/ ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: xf86-video-ati page flipping fixes
On Thu, May 05, 2011 at 05:09:56PM +0200, Mario Kleiner wrote: > On May 5, 2011, at 4:23 PM, Ville Syrjälä wrote: > > > On Thu, May 05, 2011 at 11:46:45AM +0200, Michel Dänzer wrote: > >> On Mit, 2011-05-04 at 23:51 +0300, Ville Syrjala wrote: > >>> I came to the conclusion that the xserver DRI2 invalidate patches > >>> that > >>> have been discussed aren't really fixing the problem. I suppose > >>> they may > >>> make the problem slightly less likely to happen, but at least for > >>> me that > >>> likelyhood is still very high. The whole mess looks like a simple > >>> driver > >>> bug to me. > >> > >> I think the xserver patches are still necessary, otherwise how are > >> the > >> cached DRI2 pPriv->buffers updated for other windows sharing the same > >> pixmap? > > > > As the real front buffers are not handed out to clients, there isn't > > that much reason to update them. Making sure the fake front buffer > > contain a more recent snapshot of the real front would be one reason > > though. > > > > Could it be that your patch fixes a closely related, but different > bug? During OpenGL rendering, Mesa/Gallium caches the backbuffer > assignment and only queries the current assignment from the x-server > if its drawable has been invalidated, which is what the x-server > patches do. > > I'd also think you will need both patch sets. Back buffers are private so a swap for one drawable doesn't need to invalidate them for other drawables. -- Ville Syrjälä syrj...@sci.fi http://www.sci.fi/~syrjala/ ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xf86-video-ati 1/2] dri2: Update front buffer pixmap and name before exchanging buffers
On Thu, May 05, 2011 at 12:28:20PM +0200, Michel Dänzer wrote:
> On Mit, 2011-05-04 at 23:51 +0300, Ville Syrjala wrote:
> > Buffer exchange assumes that the front buffer pixmap and name
> > information is accurate. That may not be the case eg. if the window
> > has been (un)redirected since the buffer was created.
> >
> > Signed-off-by: Ville Syrjala
> > ---
> > src/radeon_dri2.c | 45 -
> > 1 files changed, 40 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
> > index e618cc5..5d1a382 100644
> > --- a/src/radeon_dri2.c
> > +++ b/src/radeon_dri2.c
> > @@ -626,12 +626,42 @@ radeon_dri2_schedule_flip(ScrnInfoPtr scrn, ClientPtr
> > client,
> > }
> >
> > static Bool
> > -can_exchange(ScrnInfoPtr pScrn,
> > +update_front(DrawablePtr draw, DRI2BufferPtr front)
> > +{
> > +int r;
> > +PixmapPtr pixmap;
> > +struct dri2_buffer_priv *priv = front->driverPrivate;
> > +struct radeon_exa_pixmap_priv *driver_priv;
> > +
> > +if (draw->type == DRAWABLE_PIXMAP)
> > + pixmap = (PixmapPtr)draw;
> > +else
> > + pixmap = (*draw->pScreen->GetWindowPixmap)((WindowPtr)draw);
> > +
> > +pixmap->refcnt++;
> > +
> > +exaMoveInPixmap(pixmap);
> > +driver_priv = exaGetPixmapDriverPrivate(pixmap);
> > +r = radeon_gem_get_kernel_name(driver_priv->bo, &front->name);
> > +if (r) {
> > + (*draw->pScreen->DestroyPixmap)(pixmap);
> > + return FALSE;
> > +}
> > +(*draw->pScreen->DestroyPixmap)(priv->pixmap);
> > +front->pitch = pixmap->devKind;
> > +front->cpp = pixmap->drawable.bitsPerPixel / 8;
> > +priv->pixmap = pixmap;
> > +
> > +return TRUE;
> > +}
>
> Maybe at least some of this could be short-circuited if it's still the
> same pixmap.
Yeah, I had the same thought. There's one catch though; Even if it's the
same pixmap, the bo may have been exchanged, and so the buffer name may
need to be updated anyway.
--
Ville Syrjälä
syrj...@sci.fi
http://www.sci.fi/~syrjala/
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: xf86-video-ati page flipping fixes
On Thu, May 05, 2011 at 11:46:45AM +0200, Michel Dänzer wrote: > On Mit, 2011-05-04 at 23:51 +0300, Ville Syrjala wrote: > > I came to the conclusion that the xserver DRI2 invalidate patches that > > have been discussed aren't really fixing the problem. I suppose they may > > make the problem slightly less likely to happen, but at least for me that > > likelyhood is still very high. The whole mess looks like a simple driver > > bug to me. > > I think the xserver patches are still necessary, otherwise how are the > cached DRI2 pPriv->buffers updated for other windows sharing the same > pixmap? As the real front buffers are not handed out to clients, there isn't that much reason to update them. Making sure the fake front buffer contain a more recent snapshot of the real front would be one reason though. -- Ville Syrjälä syrj...@sci.fi http://www.sci.fi/~syrjala/ ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PULL xserver] Composite ClipNotify fix and expose event elimination
The following changes since commit 5cb31cd0cbf83fff5f17a475e7b0e45246b19bf3: Merge remote-tracking branch 'jturney/remove-opengl-spec-download' (2011-04-29 09:59:49 -0700) are available in the git repository at: git://gitorious.org/vsyrjala/xserver.git composite_validatetree_2 Ville Syrjälä (6): composite: Call ValidateGC after ChangeGC composite: Initialize borderClip with current values composite: Get rid of the internal UnmapWindow+MapWindow cycle composite: Copy the window contents back from the pixmap composite: Fix pWin->redirectDraw when changing between manual and automatic redirection composite: Recompute clipping when changing between manual and automatic redirection composite/compalloc.c | 147 +++- composite/compint.h|5 +- composite/compwindow.c | 19 +- 3 files changed, 116 insertions(+), 55 deletions(-) -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver 2/2] dri2: Invalidate DRI2 buffers for all windows with the same pixmap on swap.
On Thu, Apr 07, 2011 at 08:50:56AM -0700, Keith Packard wrote:
> On Thu, 07 Apr 2011 12:26:56 +0200, Michel Dänzer wrote:
>
> > Anyway, you know where you can find testers for your patch.
>
> Are you willing to test and review it? I don't know of anyone else
> interested and capable.
>
> --
> keith.pack...@intel.com
I did some testing of these with compiz and they don't actually fix the
problems with unredirecting fullscreen windows on my r300.
Some further thought about the issue leads me to the conclusion that the
only thing these patches really fix, is getting a more fresh snapshot of
the front buffer into the fake front buffer for those drawables that
didn't initiate the swap.
The real problem is that the front buffer pixmap and name might have
changed between GetBuffers and SwapBuffers. I cooked up a couple of
quick and dirty patches for radeon to fix the problem for me. I'll see
about cleaning them up later, but here they are just for reference.
>From fe05109730b1c40c16bf579fe063ed77e18f998c Mon Sep 17 00:00:00 2001
From: Ville Syrjala
Date: Tue, 3 May 2011 00:55:29 +0300
Subject: [PATCH 1/2] foo
---
src/radeon_dri2.c | 14 ++
1 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
index e618cc5..ddb2721 100644
--- a/src/radeon_dri2.c
+++ b/src/radeon_dri2.c
@@ -719,18 +719,18 @@ void radeon_dri2_frame_event_handler(unsigned int frame,
unsigned int tv_sec,
switch (event->type) {
case DRI2_FLIP:
- if (info->allowPageFlip &&
+ if (/*info->allowPageFlip &&
DRI2CanFlip(drawable) &&
- can_exchange(scrn, event->front, event->back) &&
+ can_exchange(scrn, event->front, event->back) &&*/
radeon_dri2_schedule_flip(scrn,
event->client,
drawable,
- event->front,
event->back,
+ event->front,
event->event_complete,
event->event_data,
event->frame)) {
- radeon_dri2_exchange_buffers(drawable, event->front, event->back);
+ /*radeon_dri2_exchange_buffers(drawable, event->front,
event->back);*/
break;
}
/* else fall through to exchange/blit */
@@ -1147,6 +1147,9 @@ static int radeon_dri2_schedule_swap(ClientPtr client,
DrawablePtr draw,
*target_msc = vbl.reply.sequence + flip;
swap_info->frame = *target_msc;
+ if (swap_type == DRI2_FLIP)
+radeon_dri2_exchange_buffers(draw, front, back);
+
return TRUE;
}
@@ -1193,6 +1196,9 @@ static int radeon_dri2_schedule_swap(ClientPtr client,
DrawablePtr draw,
*target_msc = vbl.reply.sequence + flip;
swap_info->frame = *target_msc;
+if (swap_type == DRI2_FLIP)
+ radeon_dri2_exchange_buffers(draw, front, back);
+
return TRUE;
blit_fallback:
--
1.7.3.4
>From 422c75e2254b17f894a00f105a2c18f0a46d0961 Mon Sep 17 00:00:00 2001
From: Ville Syrjala
Date: Tue, 3 May 2011 00:55:51 +0300
Subject: [PATCH 2/2] fix page flipping
---
src/radeon_dri2.c | 24
1 files changed, 24 insertions(+), 0 deletions(-)
diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
index ddb2721..93b3647 100644
--- a/src/radeon_dri2.c
+++ b/src/radeon_dri2.c
@@ -1051,6 +1051,30 @@ static int radeon_dri2_schedule_swap(ClientPtr client,
DrawablePtr draw,
divisor &= 0x;
remainder &= 0x;
+{
+ int r;
+ PixmapPtr pixmap;
+ struct dri2_buffer_priv *priv = front->driverPrivate;
+ struct radeon_exa_pixmap_priv *driver_priv;
+
+ if (draw->type == DRAWABLE_PIXMAP) {
+ pixmap = (PixmapPtr)draw;
+ } else {
+ pixmap = (*draw->pScreen->GetWindowPixmap)((WindowPtr)draw);
+ }
+ pixmap->refcnt++;
+
+ exaMoveInPixmap(pixmap);
+ driver_priv = exaGetPixmapDriverPrivate(pixmap);
+ r = radeon_gem_get_kernel_name(driver_priv->bo, &front->name);
+ if (r)
+ return r;
+ (*draw->pScreen->DestroyPixmap)(priv->pixmap);
+ front->pitch = pixmap->devKind;
+ front->cpp = pixmap->drawable.bitsPerPixel / 8;
+ priv->pixmap = pixmap;
+}
+
/* radeon_dri2_frame_event_handler will get called some unknown time in the
* future with these buffers. Take a reference to ensure that they won't
* get destroyed before then.
--
1.7.3.4
--
Ville Syrjälä
syrj...@sci.fi
http://www.sci.fi/~syrjala/
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v3 1/6] composite: Call ValidateGC after ChangeGC
On Fri, Apr 15, 2011 at 12:17:21PM -0700, ext Jamey Sharp wrote: > I don't see any on-list discussion about this question: Why does this > patch also switch from using serverClient to using NullClient? ChangeGC only uses the client pointer to set client.errorValue if the changed values are invalid. Pointless in this case, so NullClient is more appropriate. I think this is the only ChangeGC call that actually passes in serverClient, all the others pass NullClient, or an actual client where appropriate. > Assuming that was intentional, I'd suggest a comment in the commit > message. Sure, I'll note in in the commit message. -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PULL xserver] DRI2 fixes
The following changes since commit b2997431fd426ab318bc5dfd2cd43956d733ebec: Send events that were missing from RRSelectInput (2011-04-13 19:04:32 -0700) are available in the git repository at: git://gitorious.org/vsyrjala/xserver.git dri2_fixes Ville Syrjälä (2): dri2: Handle calloc() failure dri2: Pass out_count by value to update_dri2_drawable_buffers() hw/xfree86/dri2/dri2.c | 22 +- 1 files changed, 13 insertions(+), 9 deletions(-) -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] dri2: Invalidate DRI2 buffers for all windows with the same pixmap on swap.
On Fri, Mar 25, 2011 at 03:32:06PM +0100, ext Michel Dänzer wrote: > On Fre, 2011-03-25 at 14:47 +0200, Ville Syrjälä wrote: > > On Fri, Mar 25, 2011 at 12:35:37PM +0100, ext Michel Dänzer wrote: > > > From: Michel Dänzer > > > > > > Without this, when a compositing manager unredirects a fullscreen window > > > which > > > uses DRI2 and page flipping, the DRI2 buffer information for the > > > compositing > > > manager's output window (typically the Composite Overlay Window or root > > > window) > > > may become stale, resulting in all kinds of hilarity. > > > > Make sense to me. > > Thanks, does that mean I have your Reviewed-by: ? Reviewed-by: Ville Syrjälä I think you could split the two changes into separate patches though. I was also thinking that there should be a way to avoid walking the whole window tree in some cases. Eg. you would first walk towards the root until you encounter a different window pixmap, and then start walking the tree from there, and stop walking the children whenever you encounter a different window pixmap. > > BTW I'm toying around with offscreen flipping, and for that I'm using > > the following patch to also invalidate the window's pixmap [...] > > Makes sense to me as well. Oh and there's another change that's necessary; DRI2InvalidateBuffersEvent() must send the event with the client's pixmap ID (ref->id) instead of pDraw->id. But that change is already part of the ref count patch set. > > Subject: [PATCH] dri2: Invalidate window pixmaps > > > > While a redirected window is flipped, it's pixmap may still be used as > > and EGL image and should also get invalidated. When sending invalidate > > events for a window, also send the events for it's pixmap. > > it's -> its > > in both cases. Right. > > diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c > > index 03011ff..5faf042 100644 > > --- a/hw/xfree86/dri2/dri2.c > > +++ b/hw/xfree86/dri2/dri2.c > > @@ -1248,6 +1261,7 @@ DRI2ConfigNotify(WindowPtr pWin, int x, int y, int w, > > int h, int bw, > > return Success; > > > > DRI2InvalidateDrawable(DRI2GetDrawable(pDraw)); > > +DRI2InvalidateWindowPixmap(pWin); > > Could just do something like > > DRI2InvalidateDrawable(&pScreen->GetWindowPixmap(pWin)->drawable); > > either in DRI2InvalidateWindowPixmap() or in its callers directly. The ref count patch set changes DRI2InvalidateDrawable() to take a DRI2DrawablePtr instead of DrawablePtr. > > I do wonder if we could always attach the ref list to the window pixmap > > instead of the window itself. That would avoid the need to walk the > > whole window tree when a flip occurs. But I didn't look into this any > > further, so I'm not sure how we would handle cases where the window > > pixmap changes. > > I think the problems start earlier than that. E.g. unredirected windows > all share the screen pixmap, but the DRI2 buffers can't be shared > between windows in general. I was thinking that the buffers would still belong to the DRI2 drawable, but the ref list would be attached to the pixmap. DRI2InvalidateDrawable would then just get the ref list via the window pixmap. > > > Also re-generate the real front buffer information every time the client > > > asks > > > for it, or we might keep around stale cached information. > > > > For offscreen flipping I solved this by always updating the buffer > > information in the ReuseBufferNotify() hook. > > That hasn't landed in master yet, has it? Hmm. I thought it had, but apparently not. > > BTW I don't really like the current dri2 design. The old interface with > > one CreateBuffers call instead of multiple CreateBuffer calls seemed > > better. It made it easier to do some things in the driver code. For > > example the depth_pixmap handling in the ati driver looks broken with > > the new dri2 interface. > > Quite possibly. I think the DRI2 driver interface generally puts too > much logic in the drivers. So e.g. the drivers do flipping in a hackish > way internally, which is why this patch needs to always re-generate the > front buffer information. Yeah. Most things should be in the shared code, however my feeling is that a structure where the shared code would be more of a utility library would allow more flexibility in the driver. Oh well, no time to really think about such design issue at the moment. -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] dri2: Invalidate DRI2 buffers for all windows with the same pixmap on swap.
On Fri, Mar 25, 2011 at 06:10:42AM -0700, ext Jakob Bornecrantz wrote: > On Mar 25, 2011, at 13:47, Ville Syrjälä wrote: > > On Fri, Mar 25, 2011 at 12:35:37PM +0100, ext Michel Dänzer wrote: > >> From: Michel Dänzer > >> > >> Without this, when a compositing manager unredirects a fullscreen window > >> which > >> uses DRI2 and page flipping, the DRI2 buffer information for the > >> compositing > >> manager's output window (typically the Composite Overlay Window or root > >> window) > >> may become stale, resulting in all kinds of hilarity. > > > > Make sense to me. > > > > BTW I'm toying around with offscreen flipping, and for that I'm using > > the following patch to also invalidate the window's pixmap (the patch > > is against our internal codebase which has the DRI2 ref counting > > patches from Pauli and Christopher). > > Does the flipped buffers magically appear in any rendering api sibling > obejcts (such as GL texture obejcts) or does the client application need to > do anything? Initially I hooked it up so that it happens automagically, but I was told by our GL expert that I could just update the objects when they're (re)bound. This approach is based on the rules listed in appendix C of GLES 2.0.25 spec. I haven't actually tried that change yet. -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] dri2: Invalidate DRI2 buffers for all windows with the same pixmap on swap.
On Fri, Mar 25, 2011 at 12:35:37PM +0100, ext Michel Dänzer wrote:
> From: Michel Dänzer
>
> Without this, when a compositing manager unredirects a fullscreen window which
> uses DRI2 and page flipping, the DRI2 buffer information for the compositing
> manager's output window (typically the Composite Overlay Window or root
> window)
> may become stale, resulting in all kinds of hilarity.
Make sense to me.
BTW I'm toying around with offscreen flipping, and for that I'm using
the following patch to also invalidate the window's pixmap (the patch
is against our internal codebase which has the DRI2 ref counting
patches from Pauli and Christopher).
>From 697edc6a648be4ba6de4394ea21d968caf75d4cf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?=
Date: Mon, 7 Feb 2011 20:52:57 +0200
Subject: [PATCH] dri2: Invalidate window pixmaps
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
While a redirected window is flipped, it's pixmap may still be used as
and EGL image and should also get invalidated. When sending invalidate
events for a window, also send the events for it's pixmap.
Signed-off-by: Ville Syrjälä
---
hw/xfree86/dri2/dri2.c | 14 ++
1 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index 03011ff..5faf042 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -673,6 +673,17 @@ DRI2InvalidateDrawable(DRI2DrawablePtr pPriv)
ref->invalidate(pPriv, ref->priv, ref->id);
}
+static void
+DRI2InvalidateWindowPixmap(WindowPtr pWin)
+{
+ScreenPtr pScreen = pWin->drawable.pScreen;
+PixmapPtr pPixmap = pScreen->GetWindowPixmap(pWin);
+DRI2DrawablePtr pPriv = DRI2GetDrawable(&pPixmap->drawable);
+
+if (pPriv)
+ DRI2InvalidateDrawable(pPriv);
+}
+
/*
* In the direct rendered case, we throttle the clients that have more
* than their share of outstanding swaps (and thus busy buffers) when a
@@ -1071,6 +1082,8 @@ DRI2SwapBuffers(ClientPtr client, DRI2DrawablePtr pPriv,
CARD64 target_msc,
*swap_target = pPriv->swap_count + pPriv->swapsPending;
DRI2InvalidateDrawable(pPriv);
+if (pDraw->type == DRAWABLE_WINDOW)
+ DRI2InvalidateWindowPixmap((WindowPtr)pDraw);
DRI2CopyFrontToFakeFront(pDraw, pPriv);
return Success;
@@ -1248,6 +1261,7 @@ DRI2ConfigNotify(WindowPtr pWin, int x, int y, int w, int
h, int bw,
return Success;
DRI2InvalidateDrawable(DRI2GetDrawable(pDraw));
+DRI2InvalidateWindowPixmap(pWin);
return Success;
}
--
1.7.3.4
I do wonder if we could always attach the ref list to the window pixmap
instead of the window itself. That would avoid the need to walk the
whole window tree when a flip occurs. But I didn't look into this any
further, so I'm not sure how we would handle cases where the window
pixmap changes.
> Also re-generate the real front buffer information every time the client asks
> for it, or we might keep around stale cached information.
For offscreen flipping I solved this by always updating the buffer
information in the ReuseBufferNotify() hook. But I suppose your solution
fits better into the current dri2 design.
BTW I don't really like the current dri2 design. The old interface with
one CreateBuffers call instead of multiple CreateBuffer calls seemed
better. It made it easier to do some things in the driver code. For
example the depth_pixmap handling in the ati driver looks broken with
the new dri2 interface. It would also allow the driver to make a copy
from the old buffer to the new buffer in case the client had already
started rendering. OTOH I suppose the client could just avoid
re-getting the buffers after it's started rendering. And I suppose
the copy could also be done by adding a new hook to the current dri2
interface.
> Fixes https://bugs.freedesktop.org/show_bug.cgi?id=35452 .
>
> Signed-off-by: Michel Dänzer
> ---
> hw/xfree86/dri2/dri2.c | 19 ++-
> 1 files changed, 18 insertions(+), 1 deletions(-)
>
> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> index d8851f2..c229959 100644
> --- a/hw/xfree86/dri2/dri2.c
> +++ b/hw/xfree86/dri2/dri2.c
> @@ -339,6 +339,7 @@ allocate_or_reuse_buffer(DrawablePtr pDraw, DRI2ScreenPtr
> ds,
> int old_buf = find_attachment(pPriv, attachment);
>
> if ((old_buf < 0)
> + || attachment == DRI2BufferFrontLeft
> || !dimensions_match
> || (pPriv->buffers[old_buf]->format != format)) {
> *buffer = (*ds->CreateBuffer)(pDraw, attachment, format);
> @@ -814,6 +815,15 @@ DRI2WaitSwap(ClientPtr client, DrawablePtr pDrawable)
> return FALSE;
> }
>
> +static int
> +DRI2SwapWalk(WindowPtr pWin, pointer data)
> +{
> +if (pWin->d
Re: ProcDamageCreate emits a damage event - why?
On Wed, Mar 23, 2011 at 11:53:05AM -0400, ext Adam Jackson wrote: > On 3/23/11 7:24 AM, Erkki Seppala wrote: > > > So what kind of guessing are we talking about here? What is the downside > > of removing this initial damage event? The downside with the current > > code is that it can lead to some excess work when no damage has > > occurred. (I wonder if the behavior can changed, though, if some > > applications already depend on it.) > > I suspect that the guess, from the client's perspective, is about the > actual window geometry at the time the damage was created. > > I can easily imagine clients depending on this behaviour at this point, > since it means you don't need to manually do a "first run" of your > damage handler against the initial window state; you can just let your > event loop pick it up naturally. If you're a damage-based vnc screen > scraper, that's a feature. > > If you have an example where this does cause excess work, it'd be pretty > easy to extend the protocol so the damage level argument includes a bit > for (not) adding the initial clip. There is at least one issue with the current implementation; It sends the event to all clients who are interested in the same window. So if you fire up your vnc screen scraper, your composite manager thinks it has to redraw the window. I think that at least should be fixed. -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: What's up with Xv cliprects?
On Wed, Mar 23, 2011 at 03:36:16PM +0100, ext Thomas Hellstrom wrote: > Hi! > > I just tried Xserver 1.10 on Ubuntu 11.04 and noticed that regardless of > whether I put another window on top of a non-redirected Xv window, > XvPutImage will only be sent a single cliprect covering the whole window. > > Needless to say this is bad news for any other window that happens to be > on top of the video window when colorkey autopainting kicks in..., > > Before I dig further into this, does anybody know of any changes > affecting this? There's been quite a bit of churn in xf86xv, but nothing should have changed as far as a regular PutImage is concerned. The clip region is still computed the same way as before. So it seems to me that pGC->pCompositeClip is out of date somehow. Perhaps ValidateGC()->miComputeCompositeClip() didn't happen for some reason. That might point to the drawable serial being out of date. The other option would be that the drawable's clipList is bad. Anyway, the color key filling uses a scratch GC which will be validated separately, so assuming the clipList for the window is correct the color key should still end up in the correct areas. Well, assuming the drawable serial and scratch GC serial aren't identical. -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 4/4] dix: Make InputOnly window checking more consistent
On Fri, Mar 04, 2011 at 04:05:21PM -0500, ext Adam Jackson wrote: > Remove the UNDRAWABLE_WINDOW macro (which was just needlessly cute) and > always check ->class to decide if a window is InputOnly or InputOutput. > ->type is now always either window or pixmap. I wonder how many places assume that 'type != DRAWABLE_WINDOW && type != DRAWABLE_PIXMAP' type of checks would catch InputOnly windows? At least dri2 relies on that in DRI2CreateDrawable(). Well, actually DRI2CreateDrawable() leaks memory if it encounters UNDRAWABLE_WINDOW, but that's just a bug that should be fixed anyway. I fear you pretty much have to review every piece of code that checks the drawable type. -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: RandR 1.4 restart
On Mon, Feb 28, 2011 at 05:29:56PM -0800, ext Keith Packard wrote: > > Ok, let's try to get a bit of protocol review, then look at the library > API, then the server internal APIs and finally the server > implementation. Most of the code already exists, so this shouldn't take > a terribly long time. > > I've cleaned up the protocol a bit, reducing the changes so that we've > got only one 'set' and one 'get' request, each of which do 'everything' > except deal with output properties. The separate requests to set and > query the sprite transforms are gone. > > For the library changes, I'd like to know is whether I should emulate > the new SetCrtcConfigs interface using old requests on old servers. That > would involve duplicating a bit of structure from the insides of the X > server, but would mean that applications could switch to the new > interface and not worry about preserving compatibility with the old. > > Here's a diff from 1.3.2 of just the protocol specification. Please see > if this looks reasonable from a protocol perspective. So what about also allowing windows to use the scanout pixmaps as their backing pixmap? I'm mainly interested in something that would allow unredirected 32bpp windows on a 16bpp screen. -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PULL xserver] Composite ClipNotify fix and expose event elimination
On Thu, Feb 24, 2011 at 07:17:28PM -0800, ext Keith Packard wrote: > On Thu, 24 Feb 2011 17:38:53 +0200, Ville Syrjälä > wrote: > > > Ville Syrjälä (4): > > composite: Call ValidateGC after ChangeGC > > composite: Initialize borderClip with current values > > I'm good with merging these two obvious fixes. > > > composite: Get rid of the internal UnmapWindow+MapWindow cycle > > composite: Copy the window contents back from the pixmap > > These look too late for 1.10. Yeah, I figured you might say that. I suppose I should've sent the pull req earlier, but since the review discussions seemed to just peter out, I went on to do other things. The patches aren't going to disappear though, so waiting after 1.10 isn't a problem for me. -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PULL xserver] Composite ClipNotify fix and expose event elimination
There were no additional review comments for this patchset, so I think it's ready. We've been running it internally for some time now on prototype devices and I've been running it on my work laptop as well. The following changes since commit 93a73993708b1345c86ec3ec06b02ed236595673: test: write some event → XI1 conversion tests. (2011-02-22 08:08:55 +1000) are available in the git repository at: git://gitorious.org/vsyrjala/xserver.git composite_validatetree Ville Syrjälä (4): composite: Call ValidateGC after ChangeGC composite: Initialize borderClip with current values composite: Get rid of the internal UnmapWindow+MapWindow cycle composite: Copy the window contents back from the pixmap composite/compalloc.c | 135 +++ composite/compint.h|5 ++- composite/compwindow.c | 16 -- 3 files changed, 105 insertions(+), 51 deletions(-) -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v2 08/10] dri2: Send events only to known clients
On Tue, Feb 15, 2011 at 09:18:36AM +0200, Pauli Nieminen wrote:
> On 14/02/11 21:06 +0200, Ville Syrjälä wrote:
> > On Tue, Feb 08, 2011 at 11:42:54PM +0200, ext Pauli wrote:
> > > From: Pauli Nieminen
> > >
> > > If client disconnects and new client gets same id DRI2 events may end to
> > > wrong client. DRI2 reference list can be checked to see if the client
> > > still owns the DRI2Drawable.
> > >
> > > Signed-off-by: Pauli Nieminen
> > > ---
> > > hw/xfree86/dri2/dri2.c | 73
> > > +++
> > > 1 files changed, 54 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> > > index 9133bb3..c77ac9b 100644
> > > --- a/hw/xfree86/dri2/dri2.c
> > > +++ b/hw/xfree86/dri2/dri2.c
> > > @@ -111,12 +111,16 @@ typedef struct _DRI2Screen {
> > > ConfigNotifyProcPtr ConfigNotify;
> > > } DRI2ScreenRec;
> > >
> > > +typedef struct _DRI2ClientEventRec {
> > > +ClientPtrclient;
> > > +struct list link;
> > > +} DRI2ClientEventRec, *DRI2ClientEventPtr;
> > > +
> > > typedef struct _DRI2SwapCompleteDataRec {
> > > DRI2BufferPtrsrc;
> > > DRI2BufferPtrdest;
> > > -ClientPtrclient;
> > > void * data;
> > > -struct list link;
> > > +DRI2ClientEventRec event;
> > > } DRI2SwapCompleteDataRec, *DRI2SwapCompleteDataPtr;
> > >
> > > static struct list *
> > > @@ -266,7 +270,8 @@ DRI2LookupClientDrawableRef(DRI2DrawablePtr pPriv,
> > > ClientPtr client, XID id)
> > > DRI2DrawableRefPtr ref;
> > >
> > > list_for_each_entry(ref, &pPriv->reference_list, link) {
> > > - if (CLIENT_ID(ref->dri2_id) == client->index && ref->id == id)
> > > + if (CLIENT_ID(ref->dri2_id) == client->index &&
> > > + (id == 0 || ref->id == id))
> > > return ref;
> > > }
> > > return NULL;
> > > @@ -753,28 +758,41 @@ DRI2CanExchange(DrawablePtr pDraw)
> > > return FALSE;
> > > }
> > >
> > > +static void
> > > +cleanup_client_event(DRI2ClientEventPtr event)
> > > +{
> > > +if (event->client)
> > > + list_del(&event->link);
> > > +}
> > > +
> > > void
> > > DRI2WaitMSCComplete2(DRI2DrawablePtr pPriv, int frame,
> > >unsigned int tv_sec, unsigned int tv_usec,
> > >void *data)
> > > {
> > > -ClientPtr client = data;
> > > +ClientPtr blockedClient = pPriv->blockedClient;
> > > +DRI2ClientEventPtr pEvent = data;
> > > +ClientPtr client = pEvent->client;
> > >
> > > +pPriv->blockedClient = NULL;
> > > +pPriv->blockedOnMsc = FALSE;
> > > pPriv->refcnt--;
> > >
> > > -if (pPriv->refcnt <= 0) {
> > > - DRI2DrawableGone(pPriv, 0);
> > > - return;
> > > -}
> > > +if (client == NULL)
> > > + goto out;
> > >
> > > ProcDRI2WaitMSCReply(client, ((CARD64)tv_sec * 100) + tv_usec,
> > >frame, pPriv->swap_count);
> > >
> > > -if (pPriv->blockedClient)
> > > - AttendClient(pPriv->blockedClient);
> > > +if (blockedClient)
> > > + AttendClient(blockedClient);
> > >
> > > -pPriv->blockedClient = NULL;
> > > -pPriv->blockedOnMsc = FALSE;
> > > +out:
> > > +cleanup_client_event(pEvent);
> > > +free(pEvent);
> > > +
> > > +if (pPriv->refcnt <= 0)
> > > + DRI2DrawableGone(pPriv, 0);
> > > }
> > >
> > > static void
> > > @@ -816,7 +834,7 @@ DRI2WakeClient(ClientPtr client, DRI2DrawablePtr
> > > pPriv, int frame,
> > > static void
> > > free_swap_complete_data (DRI2DrawablePtr pPriv, DRI2SwapCompleteDataPtr
> > > pSwapData)
> > > {
> > > -list_del(&pSwapData->link);
> > > +cleanup_client_event(&pSwapData->event);
> > > buffer_unref(pPriv, pSwapData->src);
> > > buffer_unref(pPriv, pSwapData->dest);
> > > free(pSwapData);
> > > @@ -828,9
Re: [PATCH v2 03/10] dri2: Change driver interface to support DRI2Drawable
On Thu, Feb 10, 2011 at 06:22:24PM +0200, Pauli Nieminen wrote:
> On 10/02/11 17:40 +0200, Ville Syrjälä wrote:
> > On Tue, Feb 08, 2011 at 11:42:49PM +0200, ext Pauli wrote:
> > > From: Pauli Nieminen
> > >
> > > void
> > > -DRI2WaitMSCComplete(ClientPtr client, DrawablePtr pDraw, int frame,
> > > - unsigned int tv_sec, unsigned int tv_usec)
> > > +DRI2WaitMSCComplete2(DRI2DrawablePtr pPriv, int frame,
> > > +unsigned int tv_sec, unsigned int tv_usec,
> > > +void *data)
> > > {
> > > -DRI2DrawablePtr pPriv;
> > > -
> > > -pPriv = DRI2GetDrawable(pDraw);
> > > -if (pPriv == NULL)
> > > - return;
> > > +ClientPtr client = data;
> >
> > Why void* instead of ClientPtr?
> >
>
> Because driver shouldn't care what is in the data pointer.
>
> Because it will contain special structure to track if client has disconnected
> later on. Only reason to pass ClienPtr there for now is to make this patch
> work without client state tracking.
OK. Since you're already shoveling stuff into these opaque structures,
you could also hide the DRI2SwapEventPtr pointer from the driver by
moving it into _DRI2SwapCompleteDataRec. You already moved the
accompanying data pointer there. Then again, that would also involve
a new version of the ScheduleSwap function, so perhaps it's not worth
it.
I also find the lack of symmetry between the remaining v1 functions
and the new v2 functions a little bit distasteful, but on the other
hand it helps in minimizing the driver changes.
--
Ville Syrjälä
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v2 08/10] dri2: Send events only to known clients
RegionRec region;
> @@ -934,9 +958,9 @@ DRI2SwapBuffers(ClientPtr client, DRI2DrawablePtr pPriv,
> CARD64 target_msc,
> buffer_ref(pDestBuffer);
> pSwapData->src = pSrcBuffer;
> pSwapData->dest = pDestBuffer;
> -pSwapData->client = client;
> pSwapData->data = data;
> -list_add (&pSwapData->link, clientEvents);
> +pSwapData->event.client = client;
> +list_add (&pSwapData->event.link, clientEvents);
>
> /* Old DDX or no swap interval, just blit */
> if (!ds->ScheduleSwap || !pPriv->swap_interval) {
> @@ -1062,13 +1086,22 @@ int
> DRI2WaitMSC(ClientPtr client, DRI2DrawablePtr pPriv, CARD64 target_msc,
> CARD64 divisor, CARD64 remainder)
> {
> -DRI2ScreenPtr ds = pPriv->dri2_screen;
> -DrawablePtr pDraw = pPriv->drawable;
> +DRI2ScreenPtr ds = pPriv->dri2_screen;
> +DrawablePtrpDraw = pPriv->drawable;
> +struct list * clientEvents = DRI2GetClientEventList(client);
> +DRI2ClientEventPtr pEvent;
> Bool ret;
>
> if (pDraw == NULL)
> return BadDrawable;
>
> +pEvent = malloc(sizeof *pEvent);
> +if (!pEvent)
> + return BadAlloc;
> +
> +pEvent->client = client;
> +list_add(&pEvent->link, clientEvents);
> +
> pPriv->refcnt++;
>
> /* Old DDX just completes immediately */
> @@ -1081,6 +1114,8 @@ DRI2WaitMSC(ClientPtr client, DRI2DrawablePtr pPriv,
> CARD64 target_msc,
> ret = (*ds->ScheduleWaitMSC)(client, pDraw, target_msc, divisor,
> remainder, client);
> if (!ret) {
> pPriv->refcnt--;
> + cleanup_client_event(pEvent);
> + free(pEvent);
> return BadDrawable;
> }
>
> @@ -1316,7 +1351,7 @@ DRI2ClientCallback(CallbackListPtr
> *ClientStateCallback, pointer closure, pointe
> NewClientInfoRec *clientinfo = calldata;
> ClientPtr pClient = clientinfo->client;
> struct list *clientEvents = DRI2GetClientEventList(pClient);
> -DRI2SwapCompleteDataPtr ref, next;
> +DRI2ClientEventPtr ref, next;
>
> switch (pClient->clientState) {
> case ClientStateInitial:
> --
> 1.7.0.4
>
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
--
Ville Syrjälä
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v2 09/10] dri2: copy front to fake front in SwapBuffers
On Tue, Feb 08, 2011 at 11:42:55PM +0200, ext Pauli wrote:
> From: Pauli Nieminen
>
> DRI2SwapComplete is too late for front to fake front copy if swap is
> completed asynchronously. Client could be able to use fake front already
> before swap completes.
>
> Moving front to fake front solves the problem but requires that driver
> is capable to copy from new front to fake front immediately after
> ScheduleSwap hook returns.
>
> v2:
> * In blit path copy front to fake before completing the swap
>
> Signed-off-by: Pauli Nieminen
> ---
> hw/xfree86/dri2/dri2.c | 33 +++--
> 1 files changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> index c77ac9b..bfd72ad 100644
> --- a/hw/xfree86/dri2/dri2.c
> +++ b/hw/xfree86/dri2/dri2.c
> @@ -847,7 +847,6 @@ DRI2SwapComplete2(DRI2DrawablePtr pPriv, int frame,
> {
> DRI2SwapCompleteDataPtr pSwapData = swap_data;
> ClientPtr client = pSwapData->event.client;
> -DrawablePtr pDraw = pPriv->drawable;
> CARD64 ust = 0;
> DRI2DrawableRefPtr ref;
>
> @@ -865,19 +864,6 @@ DRI2SwapComplete2(DRI2DrawablePtr pPriv, int frame,
> if (ref == NULL)
> goto out;
>
> -if (pDraw) {
> - BoxRec box;
> - RegionRec region;
> -
> - box.x1 = 0;
> - box.y1 = 0;
> - box.x2 = pDraw->width;
> - box.y2 = pDraw->height;
> - RegionInit(®ion, &box, 0);
> - DRI2CopyRegion(pPriv, ®ion, DRI2BufferFakeFrontLeft,
> -DRI2BufferFrontLeft);
> -}
> -
> if (swap_complete)
> swap_complete(client, pSwapData->data, type, ust, frame,
> pPriv->swap_count);
> @@ -911,6 +897,23 @@ DRI2WaitSwap(ClientPtr client, DRI2DrawablePtr pPriv)
> return FALSE;
> }
>
> +static void
> +DRI2CopyFrontToFakeFront(DrawablePtr pDraw, DRI2DrawablePtr pPriv)
> +{
> +if (pDraw) {
> + BoxRec box;
> + RegionRec region;
> +
> + box.x1 = 0;
> + box.y1 = 0;
> + box.x2 = pDraw->width;
> + box.y2 = pDraw->height;
> + RegionInit(®ion, &box, 0);
> + DRI2CopyRegion(pPriv, ®ion, DRI2BufferFakeFrontLeft,
> +DRI2BufferFrontLeft);
> +}
> +}
> +
> int
> DRI2SwapBuffers(ClientPtr client, DRI2DrawablePtr pPriv, CARD64 target_msc,
> CARD64 divisor, CARD64 remainder, CARD64 *swap_target,
> @@ -977,6 +980,7 @@ DRI2SwapBuffers(ClientPtr client, DRI2DrawablePtr pPriv,
> CARD64 target_msc,
> pPriv->refcnt++;
>
> (*ds->CopyRegion)(pDraw, ®ion, pDestBuffer, pSrcBuffer);
> + DRI2CopyFrontToFakeFront(pDraw, pPriv);
> DRI2SwapComplete2(pPriv, target_msc, 0, 0, DRI2_BLIT_COMPLETE,
> func, pSwapData);
> return Success;
> @@ -1037,6 +1041,7 @@ DRI2SwapBuffers(ClientPtr client, DRI2DrawablePtr
> pPriv, CARD64 target_msc,
>
> DRI2InvalidateDrawable(pPriv);
>
> +DRI2CopyFrontToFakeFront(pDraw, pPriv);
Looks to me like the ati and intel drivers won't like this change since
they don't exchange the buffer names until the vblank event is received.
I'd say those drivers just need fixing.
> return Success;
> }
>
> --
> 1.7.0.4
>
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
--
Ville Syrjälä
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v2 05/10] dri2: Keep DRI2Drawable resource allocated until creator frees it
E;
> -}
> +if (rc == BadValue)
> + rc = DRI2LookupDrawableComplex(client, id, &pTmp);
> *status = rc;
> if (rc != Success)
> return FALSE;
> @@ -239,11 +237,16 @@ send_buffers_reply(ClientPtr client, DRI2DrawablePtr
> pPriv,
> int skip = 0;
> int i;
> DrawablePtr pDrawable = DRI2DrawableGetDrawable(pPriv);
> +unsigned char type = DRAWABLE_WINDOW;
>
> if (buffers == NULL)
> return BadAlloc;
>
> -if (pDrawable->type == DRAWABLE_WINDOW) {
> +/* If drawable is NULL that means drawable was a window */
> +if (pDrawable)
> + type = pDrawable->type;
Maybe it would be cleaner to add 'type' also to DRI2Drawable? That would
make the structure bigger though. Or maybe add a small helper eg.
'int DRI2DrawableType(DRI2DrawablePtr pPriv)' that would hide this subtle
detail about 'NULL drawable == window' from the caller.
> +
> +if (type == DRAWABLE_WINDOW) {
> for (i = 0; i < count; i++) {
> /* Do not send the real front buffer of a window to the client.
> */
> @@ -267,7 +270,7 @@ send_buffers_reply(ClientPtr client, DRI2DrawablePtr
> pPriv,
>
> /* Do not send the real front buffer of a window to the client.
>*/
> - if (pDrawable->type == DRAWABLE_WINDOW
> + if (type == DRAWABLE_WINDOW
> && (buffers[i]->attachment == DRI2BufferFrontLeft)) {
> continue;
> }
> --
> 1.7.0.4
>
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
--
Ville Syrjälä
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v2 03/10] dri2: Change driver interface to support DRI2Drawable
On Tue, Feb 08, 2011 at 11:42:49PM +0200, ext Pauli wrote:
> From: Pauli Nieminen
>
> To let DRI2Drawable exists longer than Drawable driver has to use
> DRI2DrawablePtr to complete swaps and MSC waits. This allows DRI2 to
> clean up after all operations complete without accessing the freed
> DrawablePtr.
>
> v2:
> * Refactor interface to allow tracking when client is gone
>
> Signed-off-by: Pauli Nieminen
> ---
> hw/xfree86/dri2/dri2.c | 118 +--
> hw/xfree86/dri2/dri2.h | 41 +++-
> 2 files changed, 111 insertions(+), 48 deletions(-)
>
> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> index 91ae1a0..7e3e0fd 100644
> --- a/hw/xfree86/dri2/dri2.c
> +++ b/hw/xfree86/dri2/dri2.c
> @@ -95,11 +95,11 @@ typedef struct _DRI2Screen {
> unsigned intlastSequence;
>
> DRI2CreateBufferProcPtr CreateBuffer;
> -DRI2DestroyBufferProcPtrDestroyBuffer;
> +DRI2DestroyBuffer2ProcPtr DestroyBuffer;
> DRI2CopyRegionProcPtr CopyRegion;
> DRI2ScheduleSwapProcPtr ScheduleSwap;
> DRI2GetMSCProcPtr GetMSC;
> -DRI2ScheduleWaitMSCProcPtr ScheduleWaitMSC;
> +DRI2ScheduleWaitMSC2ProcPtr ScheduleWaitMSC;
> DRI2AuthMagicProcPtrAuthMagic;
>
> HandleExposuresProcPtr HandleExposures;
> @@ -107,6 +107,11 @@ typedef struct _DRI2Screen {
> ConfigNotifyProcPtr ConfigNotify;
> } DRI2ScreenRec;
>
> +typedef struct _DRI2SwapCompleteDataRec {
> +ClientPtr client;
> +void *data;
> +} DRI2SwapCompleteDataRec, *DRI2SwapCompleteDataPtr;
> +
> static DRI2ScreenPtr
> DRI2GetScreen(ScreenPtr pScreen)
> {
> @@ -137,6 +142,12 @@ DRI2DrawableGetDrawable(DRI2DrawablePtr pPriv)
> return pPriv->drawable;
> }
>
> +ScreenPtr
> +DRI2DrawableGetScreen(DRI2DrawablePtr pPriv)
> +{
> +return pPriv->dri2_screen->screen;
> +}
> +
> static unsigned long
> DRI2DrawableSerial(DrawablePtr pDraw)
> {
> @@ -323,6 +334,7 @@ static int DRI2DrawableGone(pointer p, XID id)
> return Success;
>
> pDraw = pPriv->drawable;
> +
> if (pDraw->type == DRAWABLE_WINDOW) {
> pWin = (WindowPtr) pDraw;
> dixSetPrivate(&pWin->devPrivates, dri2WindowPrivateKey, NULL);
> @@ -333,7 +345,7 @@ static int DRI2DrawableGone(pointer p, XID id)
>
> if (pPriv->buffers != NULL) {
> for (i = 0; i < pPriv->bufferCount; i++)
> - (*ds->DestroyBuffer)(pDraw, pPriv->buffers[i]);
> + (*ds->DestroyBuffer)(pPriv, pPriv->buffers[i]);
>
> free(pPriv->buffers);
> }
> @@ -394,7 +406,7 @@ update_dri2_drawable_buffers(DRI2DrawablePtr pPriv,
> DrawablePtr pDraw,
> if (pPriv->buffers != NULL) {
> for (i = 0; i < pPriv->bufferCount; i++) {
> if (pPriv->buffers[i] != NULL) {
> - (*ds->DestroyBuffer)(pDraw, pPriv->buffers[i]);
> + (*ds->DestroyBuffer)(pPriv, pPriv->buffers[i]);
> }
> }
>
> @@ -531,7 +543,7 @@ err_out:
>
> for (i = 0; i < count; i++) {
> if (buffers[i] != NULL)
> - (*ds->DestroyBuffer)(pDraw, buffers[i]);
> + (*ds->DestroyBuffer)(pPriv, buffers[i]);
> }
>
> free(buffers);
> @@ -684,14 +696,11 @@ DRI2CanExchange(DrawablePtr pDraw)
> }
>
> void
> -DRI2WaitMSCComplete(ClientPtr client, DrawablePtr pDraw, int frame,
> - unsigned int tv_sec, unsigned int tv_usec)
> +DRI2WaitMSCComplete2(DRI2DrawablePtr pPriv, int frame,
> +unsigned int tv_sec, unsigned int tv_usec,
> +void *data)
> {
> -DRI2DrawablePtr pPriv;
> -
> -pPriv = DRI2GetDrawable(pDraw);
> -if (pPriv == NULL)
> - return;
> +ClientPtr client = data;
Why void* instead of ClientPtr?
--
Ville Syrjälä
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v4 0/6] xf86/xv: Fixing XV auto key filling to track exposes
On Tue, Jan 25, 2011 at 08:08:49PM +0200, Pauli wrote: > From: Pauli Nieminen > > Forth version of the patches changes interface so that driver side requires > less code (based on Ville's review). > > I'm updating my example xf86-video-ati patch also. If this patch gets reviewed > I will create more patches to convert other drivers too. > This set looks good to me. Reviewed-by: Ville Syrjälä > Pauli Nieminen (6): > xf86/xv: Remove copy paste code. > xf86/xv: Remove unused GC pointers > xf86/xv: Remove unused variable from XvPortRecPrivate > xf86/xv: Fill color key on expose > xf86/xv: Use PostValidateTree to do reput > xf86/xv: Only register PostValidateTree hook when there is work to do > > hw/xfree86/common/xf86xv.c | 210 > > hw/xfree86/common/xf86xv.h |3 + > hw/xfree86/common/xf86xvpriv.h | 6 +- > 3 files changed, 131 insertions(+), 88 deletions(-) > -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v3 4/6] xf86/xv: Fill color key on expose
On Thu, Jan 20, 2011 at 12:46:44AM +0200, ext Pauli wrote:
> From: Pauli Nieminen
>
> If window gets exposed but clipboxes doesn't change drivers would avoid
> color key fill. This makes XResizeWindo&co to lose colorkey if
> background is painted.
>
> To help drivers to avoid filling colorkey for each put server can
> provide helper function if there is exposed areas. Server can subtract
> exposed areas from filled region.
>
> As a side effect we can avoid useless color key fills if window only
> moves in screen without background fills.
>
> v3:
> * Change tracking to filled area to account for client initiated clip
> changes
> * Make overlaid XvPutImage behavior like textured XvPutImage or PutImage
> * Make region dynamically allocated only when required.
Since this thing needs driver side changes anyway, I think we should aim
for simplicity in the driver side. So perhaps something like this:
void
xf86XVFillKeyHelperPort (DrawablePtr pDraw, pointer data, CARD32 key,
RegionPtr clipboxes, Bool fillEverything)
{
XF86XVWindowPtr WinPriv = GET_XF86XV_WINDOW((WindowPtr)pDraw);
XvPortRecPrivatePtr pPriv;
RegionRec reg;
RegionPtr fillboxes;
while (WinPriv) {
pPriv = WinPriv->PortRec;
if (pPriv->DevPriv.ptr == data)
break;
WinPriv = WinPriv->next;
}
if (!WinPriv)
return;
if (!pPriv->ckeyFilled)
pPriv->ckeyFilled = RegionCreate(NULL, 0);
if (!fillEverything) {
RegionNull(®);
RegionSubtract(®, clipboxes, pPriv->ckeyFilled);
fillboxes = ®
} else
fillboxes = clipboxes;
if (RegionNotEmpty(fillboxes))
xf86XVFillKeyHelperDrawable (pDraw, key, fillboxes);
RegionCopy(pPriv->ckeyFilled, clipboxes);
if (fillboxes == ®)
RegionUninit(®);
}
--
Ville Syrjälä
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: Fix DGA events
On Mon, Jan 24, 2011 at 10:32:29AM +1000, Peter Hutterer wrote: > On Mon, Jan 24, 2011 at 01:06:38AM +0200, Ville Syrjala wrote: > > This series tries to fix DGA input support. > > > > Note that even after these patches, DGA2 seems to be completely > > useless. XDGASelectInput is refused unless XDGASetMode is done first. > > However XDGASetMode disables all X rendering, and since the other > > DGA2 rendering paths (XDGAOpenFrameBuffer, pixmap, XDGAFillRect & co.) > > were disabled, there is absolutely no way to put any content on the > > screen while in DGA2 mode. > > merged the series, thanks. > > fixed up the copy/paste error in 06/11, don't worry about sending another > patch for that. Thanks. Apprently I didn't look at the events too closely on the client side, and my test app's visual feedback only worked with DGA1 due to the DGA2 rendering problems I mentioned. -- Ville Syrjälä syrj...@sci.fi http://www.sci.fi/~syrjala/ ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v3 4/6] xf86/xv: Fill color key on expose
boxes);
> - int i, nbox = RegionNumRects(clipboxes);
> + BoxPtr pbox = RegionRects(fillboxes);
> + int i, nbox = RegionNumRects(fillboxes);
> xRectangle *rects;
> GCPtr gc;
>
> @@ -1894,9 +1948,9 @@ xf86XVFillKeyHelperDrawable (DrawablePtr pDraw, CARD32
> key, RegionPtr clipboxes)
> }
>
> void
> -xf86XVFillKeyHelper (ScreenPtr pScreen, CARD32 key, RegionPtr clipboxes)
> +xf86XVFillKeyHelper (ScreenPtr pScreen, CARD32 key, RegionPtr fillboxes)
> {
> -xf86XVFillKeyHelperDrawable (&pScreen->root->drawable, key, clipboxes);
> +xf86XVFillKeyHelperDrawable (&pScreen->root->drawable, key, fillboxes);
> }
>
> /* xf86XVClipVideoHelper -
> diff --git a/hw/xfree86/common/xf86xv.h b/hw/xfree86/common/xf86xv.h
> index 47061fe..3243ccc 100644
> --- a/hw/xfree86/common/xf86xv.h
> +++ b/hw/xfree86/common/xf86xv.h
> @@ -239,10 +239,13 @@ extern _X_EXPORT XF86VideoAdaptorPtr
> xf86XVAllocateVideoAdaptorRec(ScrnInfoPtr p
> extern _X_EXPORT void xf86XVFreeVideoAdaptorRec(XF86VideoAdaptorPtr ptr);
>
> extern _X_EXPORT void
> -xf86XVFillKeyHelper (ScreenPtr pScreen, CARD32 key, RegionPtr clipboxes);
> +xf86XVFillKeyHelper (ScreenPtr pScreen, CARD32 key, RegionPtr fillboxes);
>
> extern _X_EXPORT void
> -xf86XVFillKeyHelperDrawable (DrawablePtr pDraw, CARD32 key, RegionPtr
> clipboxes);
> +xf86XVFillKeyHelperDrawable (DrawablePtr pDraw, CARD32 key, RegionPtr
> fillboxes);
> +
> +extern _X_EXPORT Bool
> +xf86XVGetPortFillArea (DrawablePtr pDraw, pointer data, RegionPtr clipboxes,
> RegionPtr fillboxes);
>
> extern _X_EXPORT Bool
> xf86XVClipVideoHelper(
> diff --git a/hw/xfree86/common/xf86xvpriv.h b/hw/xfree86/common/xf86xvpriv.h
> index 4572218..3f1106d 100644
> --- a/hw/xfree86/common/xf86xvpriv.h
> +++ b/hw/xfree86/common/xf86xvpriv.h
> @@ -68,6 +68,7 @@ typedef struct {
> unsigned char type;
> unsigned int subWindowMode;
> RegionPtr clientClip;
> + RegionPtr ckeyFilled;
> RegionPtr pCompositeClip;
> Bool FreeCompositeClip;
> XvAdaptorRecPrivatePtr AdaptorRec;
> --
> 1.7.0.4
>
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
--
Ville Syrjälä
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v2 4/6] xf86/xv: Fill color key on expose
es)
> +xf86XVFillKeyHelper (ScreenPtr pScreen, CARD32 key, RegionPtr fillboxes)
> {
> -xf86XVFillKeyHelperDrawable (&pScreen->root->drawable, key, clipboxes);
> +xf86XVFillKeyHelperDrawable (&pScreen->root->drawable, key, fillboxes);
> }
>
> /* xf86XVClipVideoHelper -
> diff --git a/hw/xfree86/common/xf86xv.h b/hw/xfree86/common/xf86xv.h
> index 47061fe..3243ccc 100644
> --- a/hw/xfree86/common/xf86xv.h
> +++ b/hw/xfree86/common/xf86xv.h
> @@ -239,10 +239,13 @@ extern _X_EXPORT XF86VideoAdaptorPtr
> xf86XVAllocateVideoAdaptorRec(ScrnInfoPtr p
> extern _X_EXPORT void xf86XVFreeVideoAdaptorRec(XF86VideoAdaptorPtr ptr);
>
> extern _X_EXPORT void
> -xf86XVFillKeyHelper (ScreenPtr pScreen, CARD32 key, RegionPtr clipboxes);
> +xf86XVFillKeyHelper (ScreenPtr pScreen, CARD32 key, RegionPtr fillboxes);
>
> extern _X_EXPORT void
> -xf86XVFillKeyHelperDrawable (DrawablePtr pDraw, CARD32 key, RegionPtr
> clipboxes);
> +xf86XVFillKeyHelperDrawable (DrawablePtr pDraw, CARD32 key, RegionPtr
> fillboxes);
> +
> +extern _X_EXPORT Bool
> +xf86XVGetPortFillArea (DrawablePtr pDraw, pointer data, RegionPtr clipboxes,
> RegionPtr fillboxes);
>
> extern _X_EXPORT Bool
> xf86XVClipVideoHelper(
> diff --git a/hw/xfree86/common/xf86xvpriv.h b/hw/xfree86/common/xf86xvpriv.h
> index 4572218..563b55a 100644
> --- a/hw/xfree86/common/xf86xvpriv.h
> +++ b/hw/xfree86/common/xf86xvpriv.h
> @@ -72,6 +72,7 @@ typedef struct {
> Bool FreeCompositeClip;
> XvAdaptorRecPrivatePtr AdaptorRec;
> XvStatus isOn;
> + RegionRec exposedAreas;
> int vid_x, vid_y, vid_w, vid_h;
> int drw_x, drw_y, drw_w, drw_h;
> DevUnion DevPriv;
> --
> 1.7.0.4
>
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
--
Ville Syrjälä
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v2 4/6] xf86/xv: Fill color key on expose
n->root->drawable, key, fillboxes);
> }
>
> /* xf86XVClipVideoHelper -
> diff --git a/hw/xfree86/common/xf86xv.h b/hw/xfree86/common/xf86xv.h
> index 47061fe..3243ccc 100644
> --- a/hw/xfree86/common/xf86xv.h
> +++ b/hw/xfree86/common/xf86xv.h
> @@ -239,10 +239,13 @@ extern _X_EXPORT XF86VideoAdaptorPtr
> xf86XVAllocateVideoAdaptorRec(ScrnInfoPtr p
> extern _X_EXPORT void xf86XVFreeVideoAdaptorRec(XF86VideoAdaptorPtr ptr);
>
> extern _X_EXPORT void
> -xf86XVFillKeyHelper (ScreenPtr pScreen, CARD32 key, RegionPtr clipboxes);
> +xf86XVFillKeyHelper (ScreenPtr pScreen, CARD32 key, RegionPtr fillboxes);
>
> extern _X_EXPORT void
> -xf86XVFillKeyHelperDrawable (DrawablePtr pDraw, CARD32 key, RegionPtr
> clipboxes);
> +xf86XVFillKeyHelperDrawable (DrawablePtr pDraw, CARD32 key, RegionPtr
> fillboxes);
> +
> +extern _X_EXPORT Bool
> +xf86XVGetPortFillArea (DrawablePtr pDraw, pointer data, RegionPtr clipboxes,
> RegionPtr fillboxes);
>
> extern _X_EXPORT Bool
> xf86XVClipVideoHelper(
> diff --git a/hw/xfree86/common/xf86xvpriv.h b/hw/xfree86/common/xf86xvpriv.h
> index 4572218..563b55a 100644
> --- a/hw/xfree86/common/xf86xvpriv.h
> +++ b/hw/xfree86/common/xf86xvpriv.h
> @@ -72,6 +72,7 @@ typedef struct {
> Bool FreeCompositeClip;
> XvAdaptorRecPrivatePtr AdaptorRec;
> XvStatus isOn;
> + RegionRec exposedAreas;
> int vid_x, vid_y, vid_w, vid_h;
> int drw_x, drw_y, drw_w, drw_h;
> DevUnion DevPriv;
> --
> 1.7.0.4
>
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
--
Ville Syrjälä
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PULL xserver] Trigger automatic redirection updates on GetImage and SourceValidate (v5)
The following changes since commit 0dede200c9ac7adbe8b8c16efacc3edc1f183cd9: Merge remote branch 'vsyrjala/misc_fixes' (2011-01-05 08:51:46 -0800) are available in the git repository at: git://gitorious.org/vsyrjala/xserver.git composite_getimage_sourcevalidate_v5 Ville Syrjälä (6): Add subWindowMode parameter to SourceValidate Call SourceValidate even if src == dst Revert "composite: Convert compWindowUpdate to use TraverseTree" composite: Add GetImage wrapper composite: Add SourceValidate wrapper composite: Support updating an arbitrary subtree composite/compalloc.c | 11 +++- composite/compinit.c | 46 composite/compint.h|4 ++- composite/compwindow.c | 34 --- dix/window.c |4 +++ doc/xml/Xserver-spec.xml |7 +++-- exa/exa_unaccel.c |5 ++- hw/xfree86/common/xf86VGAarbiter.c |5 ++- hw/xfree86/common/xf86VGAarbiterPriv.h |2 +- hw/xfree86/xaa/xaaBitBlt.c |6 ++-- include/scrnintstr.h |3 +- include/windowstr.h|3 ++ mi/micopy.c|6 ++-- mi/misprite.c |7 +++-- miext/damage/damage.c | 20 -- miext/rootless/rootlessScreen.c|5 ++- render/mipict.c|3 +- 17 files changed, 117 insertions(+), 54 deletions(-) -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v4 1/7] Add subWindowMode parameter to SourceValidate
On Wed, Jan 05, 2011 at 09:21:13AM -0800, ext Keith Packard wrote: > On Wed, 5 Jan 2011 15:51:31 +0200, ville.syrj...@nokia.com wrote: > > > mi/misprite.c |7 --- > > Note that this patch conflicts with a cleanup done recently by Pauli Right. I can resolve the conflicts. -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v4 6/7] composite: Use DamageSetReportAfterOp
On Wed, Jan 05, 2011 at 08:34:12AM -0800, ext Keith Packard wrote: > On Wed, 5 Jan 2011 15:51:36 +0200, ville.syrj...@nokia.com wrote: > > From: Ville Syrjälä > > > > Use the "report after" damage mode for marking the automatically > > redirected as damaged. Avoids a premature window tree update triggered > > by compSourceValidate eg. during a CopyArea request. If the window is > > marked as damaged before the CopyArea has actaully happened, the window > > tree update will copy stale data to the parent. > > I didn't comment on this one before because I didn't understand what it > was doing. I think I've got it now -- the window damage gets registered > in the Damage CopyArea wrapper, then SourceValidate is called which > copies the window contents to its parent, then the data are copied. At > this point, the window has no damage recorded and yet there are changed > pixels. Exactly. > Given that the hierarchical composite damage fixes this problem in a > cleaner fashion, should we just use that patch and not this one? > > Yeah, I know I said it was "just an optimization", but I think you've > discovered that it isn't just an optimization... This patch doesn't actually interfere with the damagedDescendants patch, so we can either take both or just the damagedDescendants patch. Marking the window as damaged only after the copy has been performed seems a bit cleaner to me. But I don't have any strong feelings either way, so you get to choose ;) -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PULL xserver] Misc. DRI2 and RandR fixes
On Wed, Jan 05, 2011 at 04:19:28PM +, ext Daniel Stone wrote: > On Wed, Jan 05, 2011 at 06:04:05PM +0200, Ville Syrjälä wrote: > > On Thu, Dec 30, 2010 at 03:49:13PM +0200, Ville Syrjälä wrote: > > > The following changes since commit > > > efcb63d0ce43f96d0ac02b6f4a480dfd2374fc84: > > > > > > Render: Fix 'comparing between distinct pointer types' warning > > > (2010-12-27 09:44:07 -0800) > > > > > > are available in the git repository at: > > > git://gitorious.org/vsyrjala/xserver.git misc_fixes > > > > > > Ville Syrjälä (2): > > > dri2: Don't page flip when the window size doesn't match the pixmap > > > size > > > xfree86/modes: Take rotation into account when checking mode size > > > > > > hw/xfree86/dri2/dri2.c | 11 +++++++ > > > hw/xfree86/modes/xf86Modes.c | 33 + > > > 2 files changed, 36 insertions(+), 8 deletions(-) > > > -- > > > Ville Syrjälä > > > > Ping? > > Keith's usual excuse is that he doesn't see it if it's not Cc'ed to him, > so let's see how that goes. ;) He was in Cc. -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PULL xserver] Misc. DRI2 and RandR fixes
On Thu, Dec 30, 2010 at 03:49:13PM +0200, Ville Syrjälä wrote: > The following changes since commit efcb63d0ce43f96d0ac02b6f4a480dfd2374fc84: > > Render: Fix 'comparing between distinct pointer types' warning (2010-12-27 > 09:44:07 -0800) > > are available in the git repository at: > git://gitorious.org/vsyrjala/xserver.git misc_fixes > > Ville Syrjälä (2): > dri2: Don't page flip when the window size doesn't match the pixmap size > xfree86/modes: Take rotation into account when checking mode size > > hw/xfree86/dri2/dri2.c | 11 +++ > hw/xfree86/modes/xf86Modes.c | 33 + > 2 files changed, 36 insertions(+), 8 deletions(-) > -- > Ville Syrjälä Ping? -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v3 5/7] composite: Add SourceValidate wrapper
On Tue, Jan 04, 2011 at 05:55:51PM +0200, ville.syrj...@nokia.com wrote: > From: Ville Syrjälä > > When SourceValidate is performed on a window with IncludeInferiors > sub-window mode, force an immediate update of all the automatically > redirected windows, so that the current window contents will be up > to date. BTW I just realized that this patch won't work properly without the damagedDescendants patch. The problem is that compReportDamage marks the window as damaged before SourceValidate is called during ProcCopyArea. compScreenUpdate would then update the whole tree, before the CopyArea actually happens. It looks like it can be fixed easily with DamageSetReportAfterOp. Assuming we want to make the code work correctly without the damagedDescendants patch. AFAICS changing to "report after" damage shouldn't cause any problems even with the damagedDescendants patch, although it doesn't really need it, since it would only update the subtree that isn't damaged by ProcCopyArea. -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v3 7/7] composite: Provide a CopyArea based compWindowUpdateAutomatic
On Tue, Jan 04, 2011 at 08:16:18AM -0800, ext Keith Packard wrote: > On Tue, 4 Jan 2011 17:55:53 +0200, ville.syrj...@nokia.com wrote: > > From: Ville Syrjälä > > > > Use CopyArea in compWindowUpdateAutomatic if the window and parent > > depths match. It appears EXA Composite op can already switch to > > a straight copy when the conditions are right, but non-EXA drivers > > would fall back to software rendering. > > XAA also handles this case, so I don't think this is necessary. Ah, so it does. I just assumed it didn't, based on the fact that compNewPixmap already had the CopyArea path in addition to the CompositePicture path. Is there another reason why compNewPixmap should be treated differently, or could we just eliminate the CopyArea path there? -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v2 5/6] composite: Support updating an arbitrary subtree
On Mon, Jan 03, 2011 at 09:21:40AM -0800, Keith Packard wrote: > On Mon, 3 Jan 2011 15:54:54 +0200, Ville Syrjälä > wrote: > > > How about this? Stills leaves the compChildrenUpdate name there > > though. > > Yeah, the structure looks good. I still don't like the names, as > compWindowUpdate paints a window into its parent (updating the parent) > and compChildrenUpdate updates the window (along with the > children). Naming is hard. Indeed. compPaintWindowToParent + compPaintChildrenToWindow? That would still leave compWindowUpdateAutomatic somewhat misnamed. compPaintWindowToParentAutomatic would be quite a mouthful :) -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v2 5/6] composite: Support updating an arbitrary subtree
On Fri, Dec 31, 2010 at 02:00:43PM -0800, ext Keith Packard wrote:
> On Fri, 31 Dec 2010 16:49:38 +0200, ville.syrj...@nokia.com wrote:
>
> > WindowRec has a new member 'damagedDescendants' that is used to keep
> > track of which subtrees need updating. When a window is damaged,
> > 'damagedDescendants' will be set for all the ancestors, and when a
> > subtree is updated, the tree walk can be stopped early if no damaged
> > descendants are present.
>
> I think this raises an interesting general topic - how much of the
> current extension private stuff should get folded into the various DIX
> structures?? We've got a bunch of non-optional extensions these days and
> it might be a nice cleanup job to take their privates and move them.
>
> > CompScreenRec no longer needs the 'damaged' member since the root
> > window's 'damagedDescendants' provides the same information.
>
> > Signed-off-by: Ville Syrjälä
>
> This seems like a reasonable optimization to me, reducing the cost of
> having any CompositeRedirectAutomatic windows on the screen.
>
> I'm not entirely happy with the name compChildrenUpdate, but given that
> compWindowUpdate paints a window into its parent, I don't have a great
> suggestion for an alternative name. One option might be to move the
> painting code inside the loop:
>
> compPaintWindowToParent(pWin)
> {
> if (pWin->damaged)
> ...
> pWin->damaged = FALSE;
> }
>
> compChildrenUpdate(pWin)
> {
> if (!pWin->damagedDescendents)
> return;
> for (pChild = pWin->lastChild ...)
> compChildrenUpdate(pChild);
> compPaintWindowToParent(pChild);
> }
> pWin->damagedDescendents = FALSE;
> }
>
> Or some such. Having two functions looping over the child list seems
> ugly...
How about this? Stills leaves the compChildrenUpdate name there though.
void
compChildrenUpdate (WindowPtr pWin)
{
WindowPtr pChild;
if (!pWin->damagedDescendants)
return;
for (pChild = pWin->lastChild; pChild; pChild = pChild->prevSib)
compWindowUpdate (pChild);
pWin->damagedDescendants = FALSE;
}
void
compWindowUpdate (WindowPtr pWin)
{
compChildrenUpdate (pWin);
if (pWin->redirectDraw != RedirectDrawNone)
{
CompWindowPtr cw = GetCompWindow(pWin);
if (cw->damaged)
{
compWindowUpdateAutomatic (pWin);
cw->damaged = FALSE;
}
}
}
--
Ville Syrjälä
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v2 4/6] composite: Add SourceValidate wrapper
On Fri, Dec 31, 2010 at 01:42:16PM -0800, ext Keith Packard wrote: > On Fri, 31 Dec 2010 16:49:37 +0200, ville.syrj...@nokia.com wrote: > > From: Ville Syrjälä > > > > When SourceValidate is performed on a window with IncludeInferiors > > sub-window mode, force an immediate update of all the automatically > > redirected windows, so that the current window contents will be up > > to date. > > Reviewed-by: Keith Packard > > > +pScreen->SourceValidate = cs->SourceValidate; > > +if (pDrawable->type == DRAWABLE_WINDOW && subWindowMode == > > IncludeInferiors) > > + compScreenUpdate (pScreen); > > +if (pScreen->SourceValidate) > > + (*pScreen->SourceValidate) (pDrawable, x, y, width, height, > > + subWindowMode); > > +cs->SourceValidate = pScreen->SourceValidate; > > +pScreen->SourceValidate = compSourceValidate; > > This looks a bit tricky -- unwrapping SourceValidate before calling > compScreenUpdate seems like the right order, but I wouldn't mind a nice > comment here as it's not the 'usual' order of operations for wrapping > functions. To be honest, I didn't even think about it that much. I origianlly just duplicated the pattern from compBlockHandler(). I also used the same order of operations in the GetImage wrapper. But yeah, for SourceValidate unwrapping before updating seems like the correct order. For GetImage I suppose the order shouldn't really matter. I can add a comment to SourceValidate. But should I change the order for GetImage to do the update before unwrapping? If so, compBlockHandler could also be changed in similar fashion. -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v2 5/6] composite: Support updating an arbitrary subtree
On Fri, Dec 31, 2010 at 02:00:43PM -0800, ext Keith Packard wrote:
> On Fri, 31 Dec 2010 16:49:38 +0200, ville.syrj...@nokia.com wrote:
>
> > WindowRec has a new member 'damagedDescendants' that is used to keep
> > track of which subtrees need updating. When a window is damaged,
> > 'damagedDescendants' will be set for all the ancestors, and when a
> > subtree is updated, the tree walk can be stopped early if no damaged
> > descendants are present.
>
> I think this raises an interesting general topic - how much of the
> current extension private stuff should get folded into the various DIX
> structures?? We've got a bunch of non-optional extensions these days and
> it might be a nice cleanup job to take their privates and move them.
>
> > CompScreenRec no longer needs the 'damaged' member since the root
> > window's 'damagedDescendants' provides the same information.
>
> > Signed-off-by: Ville Syrjälä
>
> This seems like a reasonable optimization to me, reducing the cost of
> having any CompositeRedirectAutomatic windows on the screen.
>
> I'm not entirely happy with the name compChildrenUpdate, but given that
> compWindowUpdate paints a window into its parent, I don't have a great
> suggestion for an alternative name. One option might be to move the
> painting code inside the loop:
>
> compPaintWindowToParent(pWin)
> {
> if (pWin->damaged)
> ...
> pWin->damaged = FALSE;
> }
>
> compChildrenUpdate(pWin)
> {
> if (!pWin->damagedDescendents)
> return;
> for (pChild = pWin->lastChild ...)
> compChildrenUpdate(pChild);
> compPaintWindowToParent(pChild);
> }
> pWin->damagedDescendents = FALSE;
> }
>
> Or some such. Having two functions looping over the child list seems
> ugly...
OK. I'll see about making it better.
--
Ville Syrjälä
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: Composite ClipNotify fix and expose event elimination (v2)
On Mon, Dec 27, 2010 at 03:24:09PM +0200, ville.syrj...@nokia.com wrote: > Patch 4/4 was modified to accomodate the review comments. The rest > remain unchanged. Were there still open issues with this set? -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/4] composite: Support updating an arbitrary subtree
On Thu, Dec 30, 2010 at 10:34:22AM -0800, ext Keith Packard wrote: > On Thu, 30 Dec 2010 14:56:19 +0200, Ville Syrjälä > wrote: > > On Wed, Dec 29, 2010 at 01:05:02PM -0800, ext Keith Packard wrote: > > > On Wed, 29 Dec 2010 15:04:29 +0200, ville.syrj...@nokia.com wrote: > > > > From: Ville Syrjälä > > > > > > > > Rename compScreenUpdate to compChildrenUpdate, and pass a window as > > > > the parameter. This allows an arbitrary subtree to be updated, instead > > > > of having to update all the windows. This will be used to make sure > > > > all the children have been updated when the parent window contents need > > > > to be accessed in IncludeInferios sub-window mode. > > > > > > This change isn't right -- compWindowUpdate is already recursive, so > > > there's no need to walk across the children. > > > > I wanted to avoid the copy from pWin to it's parent, > > Probably cleaner to fix compWindowUpdate to just not do that then, which > would be easily done by passing pWin as the 'data' parameter to > TraverseTree and simply skipping that window in compWindowUpdateVisit. TraverseTree is the wrong thing to use here. See my v2 patchset for details. > > Right. I was thinking about making cs->damaged into a counter, but that > > would still cause needless tree walks if the damaged windows are in a > > different subtree. So yeah, marking the ancestors would be the best > > choice. I'll take a look at doing that. > > I'd like to have the simpler fix now and the more complicated fix > later; that way we can separate the bug fix from the optimization. Done in v2. -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PULL xserver] Misc. DRI2 and RandR fixes
The following changes since commit efcb63d0ce43f96d0ac02b6f4a480dfd2374fc84: Render: Fix 'comparing between distinct pointer types' warning (2010-12-27 09:44:07 -0800) are available in the git repository at: git://gitorious.org/vsyrjala/xserver.git misc_fixes Ville Syrjälä (2): dri2: Don't page flip when the window size doesn't match the pixmap size xfree86/modes: Take rotation into account when checking mode size hw/xfree86/dri2/dri2.c | 11 +++ hw/xfree86/modes/xf86Modes.c | 33 + 2 files changed, 36 insertions(+), 8 deletions(-) -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/4] composite: Support updating an arbitrary subtree
On Wed, Dec 29, 2010 at 01:05:02PM -0800, ext Keith Packard wrote: > On Wed, 29 Dec 2010 15:04:29 +0200, ville.syrj...@nokia.com wrote: > > From: Ville Syrjälä > > > > Rename compScreenUpdate to compChildrenUpdate, and pass a window as > > the parameter. This allows an arbitrary subtree to be updated, instead > > of having to update all the windows. This will be used to make sure > > all the children have been updated when the parent window contents need > > to be accessed in IncludeInferios sub-window mode. > > This change isn't right -- compWindowUpdate is already recursive, so > there's no need to walk across the children. I wanted to avoid the copy from pWin to it's parent, > I'm also (vaguely) concerned about performance here -- because there's > no per-window hierarchy damage information, you'll be walking the window > sub-tree on every single operation. > > It seems like a simple change would be to just clean the whole tree on > any IncludeInferiors operation so that at least a sequence of those > would only walk the tree once. That would change the code to just call > compScreenUpdate whenever CompositeUpdateWindow was called. > > A more complicated change would require marking the ancestor chain with > damage on each rendering operation, stopping when you hit a window that > is already marked, and cleaning those bits when updating windows. > However, given that all of this only happens when you have automatic > redirection going on, it's hard to get that worked up about it. Right. I was thinking about making cs->damaged into a counter, but that would still cause needless tree walks if the damaged windows are in a different subtree. So yeah, marking the ancestors would be the best choice. I'll take a look at doing that. -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: Trigger automatic redirection updates on GetImage and SourceValidate
On Wed, Dec 29, 2010 at 03:04:27PM +0200, ville.syrj...@nokia.com wrote: > This is the second attempt at fixing IncludeInferiors access with > automatically redirected children. > > Did as Keith suggested and wrapped GetImage and SourceValidate. So far > I only verified that GetImage works. CopyArea is yet to be tested. Just added a quick CopyArea test to my test app. Without the SourceValidate patch stale pixels are copied, and with the patch I get the correct pixels. So seems to work as expected. -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH modular 14/14] build.sh: verify PREFIX is a directory and is writable
On Wed, Dec 29, 2010 at 11:30:03AM -0500, Gaetan Nadon wrote:
> This will catch the case where user forgets to set PREFIX
> and does not have write permission in the /usr/local default location
>
> Signed-off-by: Gaetan Nadon
> ---
> build.sh | 23 ++-
> 1 files changed, 22 insertions(+), 1 deletions(-)
>
> diff --git a/build.sh b/build.sh
> index 1a29758..afcb158 100755
> --- a/build.sh
> +++ b/build.sh
> @@ -68,6 +68,10 @@ setup_buildenv() {
> check_full_path $LIBDIR LIBDIR
> check_full_path $LOCALSTATEDIR LOCALSTATEDIR
>
> +# This will catch the case where user forgets to set PREFIX
> +# and does not have write permission in the /usr/local default location
> +check_writable_dir $PREFIX PREFIX
Seems like it should check ${DESTDIR}${PREFIX} instead.
--
Ville Syrjälä
syrj...@sci.fi
http://www.sci.fi/~syrjala/
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/4] composite: Support updating an arbitrary subtree
On Wed, Dec 29, 2010 at 01:21:15PM +, ext Daniel Stone wrote:
> Hi,
>
> On Wed, Dec 29, 2010 at 03:04:29PM +0200, ville.syrj...@nokia.com wrote:
> > static void
> > -compScreenUpdate (ScreenPtr pScreen)
> > +compChildrenUpdate (WindowPtr pWin)
> > {
> > -CompScreenPtr cs = GetCompScreen (pScreen);
> > +ScreenPtr pScreen = pWin->drawable.pScreen;
> > +CompScreenPtr cs = GetCompScreen (pScreen);
> >
> > compCheckTree (pScreen);
> > -if (cs->damaged)
> > -{
> > - compWindowUpdate (pScreen->root);
> > - cs->damaged = FALSE;
> > +if (cs->damaged) {
> > + WindowPtr pChild;
> > +
> > + for (pChild = pWin->lastChild; pChild; pChild = pChild->prevSib)
> > + compWindowUpdate (pChild);
> > +
> > + if (pWin == pScreen->root)
> > + cs->damaged = FALSE;
>
> compWindowUpdate walks all the children itself, so you should be able to
> replace the for loop with a simple compWindowUpdate(pWin).
compWindowUpdate would cause pWin contents to be copied to it's parent.
That'd be unnecessary work at this point.
--
Ville Syrjälä
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [RFC][PATCH] dix/composite: Update windows with automatically redirected children on demand
On Mon, Dec 27, 2010 at 11:11:58AM -0800, ext Keith Packard wrote: > On Mon, 27 Dec 2010 20:45:39 +0200, Ville Syrjälä > wrote: > > > Could be overwritten fully, partially, or not at all, depending on > > the operation and GC clipping, I suppose? If any part of it gets > > overwritten it still seems better to copy from the child first, > > otherwise the last rendering results can get overwritten by the > > composite block handler. > > As I said, they'll get overwritten eventually, so it's only a matter of > 'when', not 'if'. Given that we're not going to make this 'correct', > we should probably just figure out what we actually want. Right. GetImage at least seems useful for testing purposes. Not sure if other use cases are even worth considering. I wonder if anyone actually uses automatic redirection? Well, I suppose implicit redirection can be useful when a composite manager is not used. -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [RFC][PATCH] dix/composite: Update windows with automatically redirected children on demand
On Mon, Dec 27, 2010 at 08:45:39PM +0200, Ville Syrjälä wrote: > On Mon, Dec 27, 2010 at 10:25:17AM -0800, ext Keith Packard wrote: > > On Mon, 27 Dec 2010 15:27:10 +0200, ville.syrj...@nokia.com wrote: > > > Instead of trying to identify all parts of the code that have that > > > requirement, add a simple hook into NotClippedByChildren(), which can > > > perform the update process. Hopefully that should cover most of the > > > cases, if not all of them. > > > > It would be nice if this could work, but it's almost entirely > > inaccurate: > > > > 1) GetImage doesn't use NCBC > > How odd. My test app uses GetImage and it got fixed by this. I'll have > to check what's going on there. Breakpoint 2, NotClippedByChildren (pWin=0x3dded0) at ../../dix/window.c:3026 3026in ../../dix/window.c #0 NotClippedByChildren (pWin=0x3dded0) at ../../dix/window.c:3026 #1 0x00078d40 in DoGetImage (client=0x3eac88, format=2, drawable=4194305, x=0, y=0, width=16, height=16, planemask=4294967295, im_return=0x0) at ../../dix/dispatch.c:2149 #2 0x0007905c in ProcGetImage (client=0x3eac88) at ../../dix/dispatch.c:2253 #3 0x00073510 in Dispatch () at ../../dix/dispatch.c:432 #4 0x000207ac in main (argc=16, argv=0xaec4cdd4, envp=0xaec4ce18) at ../../dix/main.c:291 So it apparently does use it. > > 2) CopyArea doesn't use NCBC when copying from the Root > > 3) CopyArea doesn't use NCBC when copying from self > > 4) ValidateGC/ValidatePicture use NCBC only when the clip may have changed Hmm. It seems these actually manage to dodge my NCBC approach :( -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v2 4/4] composite: Copy the window contents back from the pixmap
On Mon, Dec 27, 2010 at 10:45:35AM -0800, ext Keith Packard wrote: > On Mon, 27 Dec 2010 20:30:06 +0200, Ville Syrjälä > wrote: > > > We're going through this codepath when getting rid of an implicitly > > redirected window. That is at least one case where the depths won't > > match. > > I don't understand -- the only pixmap ever passed to compRestoreWindow > was just recently returned from GetWindowPixmap. Are you saying that > there are times when that pixmap doesn't match the depth of the window? It's comparing the depth of the window, and it's parent. The window's pixmap will be switched to the parent's pixmap before compRestoreWindow() will be called. Since the window's and it's parent's depths might not match, the window's and it's current pixmap's depth may not necessarily match either. But just for a very brief period during window destruction, I think. -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [RFC][PATCH] dix/composite: Update windows with automatically redirected children on demand
On Mon, Dec 27, 2010 at 10:25:17AM -0800, ext Keith Packard wrote: > On Mon, 27 Dec 2010 15:27:10 +0200, ville.syrj...@nokia.com wrote: > > > So every time that the parent window's contents are expected to be > > up to date, the contents of the children should be copied immediately > > to the parent. > > When using the parent as a source operand, this looks like a useful > change. However, when using it as a rendering target, any drawing will > be immediately overwritten with the next drawing by the application > unless the parent contents were copied back to the child. Could be overwritten fully, partially, or not at all, depending on the operation and GC clipping, I suppose? If any part of it gets overwritten it still seems better to copy from the child first, otherwise the last rendering results can get overwritten by the composite block handler. BTW isn't the current backing store implementation simply broken if someone renders with IncludeInferiors to the parent? There's no copy from the parent to the child's backing store. > So, I'd say we should look only at cases where we use the window > contents as a rendering source and ignore any rendering target > operations. > > > Instead of trying to identify all parts of the code that have that > > requirement, add a simple hook into NotClippedByChildren(), which can > > perform the update process. Hopefully that should cover most of the > > cases, if not all of them. > > It would be nice if this could work, but it's almost entirely > inaccurate: > > 1) GetImage doesn't use NCBC How odd. My test app uses GetImage and it got fixed by this. I'll have to check what's going on there. > 2) CopyArea doesn't use NCBC when copying from the Root > 3) CopyArea doesn't use NCBC when copying from self > 4) ValidateGC/ValidatePicture use NCBC only when the clip may have changed > > This seems like a job for the existing SourceValidate hook, which gets > called for CopyArea when src != dst (and also when src == dst when > damage is running), and also is called for Composite (all of the time). > > (There aren't any out-of-tree callers to SourceValidate today, so we can > actually just fix the existing bug where damage calls SourceValidate > sometimes to patch-up the missing calls from CopyArea.) > > I'd say a combination of SourceValidate and GetImage wrappers would do > the trick nicely here. > > -- > keith.pack...@intel.com -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v2 4/4] composite: Copy the window contents back from the pixmap
On Mon, Dec 27, 2010 at 10:00:30AM -0800, ext Keith Packard wrote:
> On Mon, 27 Dec 2010 14:40:14 +, Daniel Stone wrote:
> > Hi,
> >
> > On Mon, Dec 27, 2010 at 03:24:13PM +0200, ville.syrj...@nokia.com wrote:
> > > +void
> > > +compRestoreWindow (WindowPtr pWin, PixmapPtr pPixmap)
> > > +{
> > > +ScreenPtr pScreen = pWin->drawable.pScreen;
> > > +WindowPtr pParent = pWin->parent;
> > > +
> > > +if (pParent->drawable.depth == pWin->drawable.depth) {
> > > + GCPtr pGC = GetScratchGC (pWin->drawable.depth, pScreen);
> > > + int bw = (int) pWin->borderWidth;
> > > + int x = bw;
> > > + int y = bw;
> > > + int w = pWin->drawable.width;
> > > + int h = pWin->drawable.height;
> > > +
> > > + if (pGC) {
> > > + ChangeGCVal val;
> > > + val.val = IncludeInferiors;
> > > + ChangeGC (NullClient, pGC, GCSubwindowMode, &val);
> > > + ValidateGC(&pWin->drawable, pGC);
> > > + (*pGC->ops->CopyArea) (&pPixmap->drawable,
> > > +&pWin->drawable,
> > > +pGC,
> > > +x, y, w, h, 0, 0);
> > > + FreeScratchGC (pGC);
> > > + }
> > > +}
> > > +}
> >
> > It might be nice to get the Render fallback path for non-matching depths
> > here; aside from that, for the entire series:
>
> What's confusing here is that there is a test for matching depth, but
> no 'else' branch to cover the other case. I don't think this test is
> needed as the pixmap was the previous window pixmap, and depths for that
> must match the window depth. If that wasn't the case, then all
> drawing to windows would fail, as Pictures and GCs are validated against
> the depth of the window, not the pixmap.
>
> Seems like simply removing the test would be fine; it's not like you're
> requiring some new invariant in the code here.
We're going through this codepath when getting rid of an implicitly
redirected window. That is at least one case where the depths won't
match.
Breakpoint 1, compRestoreWindow (pWin=0x3dded0, pPixmap=0x3ddde8) at
../../composite/compalloc.c:212
212 in ../../composite/compalloc.c
(gdb) bt
#0 compRestoreWindow (pWin=0x3dded0, pPixmap=0x3ddde8) at
../../composite/compalloc.c:212
#1 0x002160a0 in compCheckRedirect (pWin=0x3dded0) at
../../composite/compwindow.c:167
#2 0x00216520 in compUnrealizeWindow (pWin=0x3dded0) at
../../composite/compwindow.c:265
#3 0x00027e00 in UnrealizeTree (pWin=0x3dded0, fromConfigure=0) at
../../dix/window.c:2783
#4 0x00028128 in UnmapWindow (pWin=0x3dded0, fromConfigure=0) at
../../dix/window.c:2841
#5 0x00022f88 in DeleteWindow (value=0x3dded0, wid=4194306) at
../../dix/window.c:905
#6 0x00030030 in FreeClientResources (client=0x3eac88) at
../../dix/resource.c:859
#7 0x0007c700 in CloseDownClient (client=0x3eac88) at ../../dix/dispatch.c:3490
#8 0x00073494 in Dispatch () at ../../dix/dispatch.c:417
#9 0x000207ac in main (argc=16, argv=0xaec4cdd4, envp=0xaec4ce18) at
../../dix/main.c:291
(gdb) p pWin->drawable.depth
$5 = 32 ' '
(gdb) p pWin->parent->drawable.depth
$6 = 16 '\020'
(gdb)
--
Ville Syrjälä
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v2 4/4] composite: Copy the window contents back from the pixmap
On Mon, Dec 27, 2010 at 02:40:14PM +, ext Daniel Stone wrote:
> Hi,
>
> On Mon, Dec 27, 2010 at 03:24:13PM +0200, ville.syrj...@nokia.com wrote:
> > +void
> > +compRestoreWindow (WindowPtr pWin, PixmapPtr pPixmap)
> > +{
> > +ScreenPtr pScreen = pWin->drawable.pScreen;
> > +WindowPtr pParent = pWin->parent;
> > +
> > +if (pParent->drawable.depth == pWin->drawable.depth) {
> > + GCPtr pGC = GetScratchGC (pWin->drawable.depth, pScreen);
> > + int bw = (int) pWin->borderWidth;
> > + int x = bw;
> > + int y = bw;
> > + int w = pWin->drawable.width;
> > + int h = pWin->drawable.height;
> > +
> > + if (pGC) {
> > + ChangeGCVal val;
> > + val.val = IncludeInferiors;
> > + ChangeGC (NullClient, pGC, GCSubwindowMode, &val);
> > + ValidateGC(&pWin->drawable, pGC);
> > + (*pGC->ops->CopyArea) (&pPixmap->drawable,
> > + &pWin->drawable,
> > + pGC,
> > + x, y, w, h, 0, 0);
> > + FreeScratchGC (pGC);
> > + }
> > +}
> > +}
>
> It might be nice to get the Render fallback path for non-matching depths
> here;
AFAICS that shouldn't be needed as unredirecting a non-matching depth
window would always result in an implicit redirection.
--
Ville Syrjälä
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: Composite ClipNotify fix and expose event elimination
On Mon, Dec 20, 2010 at 10:34:34AM -0800, ext Keith Packard wrote: > On Mon, 20 Dec 2010 20:30:34 +0200, Ville Syrjälä > wrote: > > > Yeah. I'm not 100% sold on that myself yet. miValidateTree & co. don't > > seem to sink in without some conscious effort. > > As you might imagine, clipping bugs are really nasty to fix. > > > I've attached an ugly test app I was using. Just ignore/rip out the xv > > stuff etc. You can run it, for example, like so: > > 'xvredirect -F -d -s -b 10` > > Thanks. I'd love to see this cleaned up and automated so that we can > verify that it works. I'd suggest having it run against a server that > isn't running a window manager or compositing manager so that it doesn't > fight those. > > You could also have it run entirely within another window to avoid > interactions with the window manager, but that wouldn't test the root > case (which is often special). I cooked up some kind of small tool that could do some automated tests. http://gitorious.org/vsyrjala/xwindow_check Basically you just tell it what window manipulation operation you want to do, and what areas you expect to be exposed. It then compares the expected areas with the actual expose events. And you can also tell it to read the window content with XGetImage and compare the results with the expected value. Someone more clever could have probably written it in two or three lines of python, or whatever the language of the day is, but I did in C. I hijacked a copy of the the region code from the X server to help me sort out the exposed areas. The example script I included passes without complaints with a server that my patches applied. An unpatched server naturally generates quite a few unnecessary exposes. -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 4/4] composite: Copy the window contents back from the pixmap
On Mon, Dec 20, 2010 at 10:16:22AM -0800, ext Keith Packard wrote:
> On Mon, 20 Dec 2010 18:05:42 +0200, ville.syrj...@nokia.com wrote:
>
> > -void
> > +PixmapPtr
> > compFreePixmap (WindowPtr pWin)
> > {
> > ScreenPtr pScreen = pWin->drawable.pScreen;
> > @@ -612,7 +647,8 @@ compFreePixmap (WindowPtr pWin)
> > pParentPixmap = (*pScreen->GetWindowPixmap) (pWin->parent);
> > pWin->redirectDraw = RedirectDrawNone;
> > compSetPixmap (pWin, pParentPixmap);
> > -(*pScreen->DestroyPixmap) (pRedirectPixmap);
> > +
> > +return pRedirectPixmap;
> > }
>
> This doesn't free the pixmap anymore, so it needs to be renamed, perhaps
> to compSetParentPixmap? Also, it shouldn't be returning the pixmap,
> instead it should remain a void function and the caller should be
> responsible for fetching the pixmap beforehand and freeing afterwards.
OK. I'll adjust the patch accordingly.
> Alternatively, (and I think this is uglier), compCheckRedirect could
> bump the pixmap ref count before calling compFreePixmap and then destroy
> the pixmap after copying the bits.
>
>
> --
> keith.pack...@intel.com
--
Ville Syrjälä
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: Composite ClipNotify fix and expose event elimination
On Mon, Dec 20, 2010 at 10:05:42AM -0800, ext Keith Packard wrote:
> On Mon, 20 Dec 2010 18:05:38 +0200, ville.syrj...@nokia.com wrote:
> > Rather than continue my attempts to hack around the issue of incorrect
> > ClipNotifys during window redirection changes, I decided to tackle the
> > issue in more proper manner.
> >
> > This series will remove the internal MapWindow+UnmapWindow cycle and
> > replace it with a single ValidateTree+HandleExposures pass through
> > the affected windows.
>
> Thanks! As you might imagine, the whole unmap/map adventure was a short
> cut to ensure that all of the regions ended up recomputed correctly:
>
> * compRedirectWindow
>+ Redirected window has anything formerly obscured now exposed
>+ Underlying windows are exposed where the redirected window was
> covering
>
> * compFreeClientWindow
>+ Un-Redirected window gets painted from backing pixmap
>+ Underlying window clip lists get updated
>
> Do you have any small test cases that verify that these are working for
> both manual and automatic redirect in each direction? I'm concerned that
> the validation code won't 'just work' in all cases...
Yeah. I'm not 100% sold on that myself yet. miValidateTree & co. don't
seem to sink in without some conscious effort.
I've attached an ugly test app I was using. Just ignore/rip out the xv
stuff etc. You can run it, for example, like so:
'xvredirect -F -d -s -b 10`
You can input 'm'/'a'/'u' for manual/automatic/unredirected redirection
for the middle sibling. 'M'/'A'/'U' to do the same for the top level
window. It will print the expose events it receives.
--
Ville Syrjälä
/*
*/
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#define WIDTH 400
#define HEIGHT 400
#define MAX(a, b) ((a) > (b) ? (a) : (b))
static void set_nodecorations_motif(Display *dpy, Window win)
{
struct {
unsigned long flags;
unsigned long functions;
unsigned long decorations;
longinputMode;
unsigned long status;
} hints = {
.flags = 2,
.decorations = 0,
};
Atom XA_MOTIF_WM_HINTS = XInternAtom(dpy, "_MOTIF_WM_HINTS", True);
if (XA_MOTIF_WM_HINTS == None)
return;
XChangeProperty(dpy, win, XA_MOTIF_WM_HINTS, XA_MOTIF_WM_HINTS, 32,
PropModeReplace, (unsigned char *) &hints, 5);
}
static void set_fs_ewmh(Display *dpy, Window win)
{
Atom XA_NET_WM_STATE = XInternAtom(dpy, "_NET_WM_STATE", True);
Atom XA_NET_WM_STATE_FULLSCREEN = XInternAtom(dpy, "_NET_WM_STATE_FULLSCREEN", True);
if (XA_NET_WM_STATE == None || XA_NET_WM_STATE_FULLSCREEN == None)
return;
XChangeProperty(dpy, win, XA_NET_WM_STATE, XA_ATOM, 32, PropModeReplace,
(unsigned char *) &XA_NET_WM_STATE_FULLSCREEN, 1);
}
static void
draw_rect(unsigned short rgb[HEIGHT][WIDTH],
int x0, int y0, int sz, unsigned short color)
{
int x, y = y0;
for (x = x0; x < x0+sz; x++) {
rgb[y][x] = color;
}
for (y++; y < y0+sz-1; y++) {
rgb[y][x0 ] = color;
rgb[y][x0+sz-1] = color;
}
for (x = x0; x < x0+sz; x++) {
rgb[y][x] = color;
}
}
static void
draw_circle (unsigned short rgb[HEIGHT][WIDTH],
int x0, int y0, int sz, unsigned short color)
{
int x, y;
for (x = 0; x < sz; x++) {
y = sqrt(sz * sz - x * x) + 0.5;
rgb[y0 + sz - y][x0 + sz + x] = color;
rgb[y0 + sz - y][x0 + sz - x] = color;
rgb[y0 + sz + y][x0 + sz + x] = color;
rgb[y0 + sz + y][x0 + sz - x] = color;
}
for (y = 0; y < sz; y++) {
x = sqrt(sz * sz - y * y) + 0.5;
rgb[y0 + sz - y][x0 + sz + x] = color;
rgb[y0 + sz - y][x0 + sz - x] = color;
rgb[y0 + sz + y][x0 + sz + x] = color;
rgb[y0 + sz + y][x0 + sz - x] = color;
}
}
static void fatal(const char *name)
{
printf("Usage: %s "
"[-c|--child]"
"[-d|--deepchild]"
"[-s|--siblings]"
"[-C|--clip ]"
"[-F|--fullscreen]"
"[-r|--reverse]"
"[-g|--geometry ]"
"\n", name);
exit(1);
}
static void set_wm_props(Display *dpy, Window win, const char *name,
int argc, char *argv[], bool fullscreen)
{
XSizeHints sizeHints;
XWMHints wmHints;
XTextProperty wm_name, icon_name;
sizeHints.flags = 0;
wmHints.flags = InputHint;
wmHints.input = True;
wm_name.value = (unsigned char *) name;
wm_name.encoding = XA_STRING;
wm_name.format = 8;
wm_name.nitems = strlen (wm_name.value) + 1;
icon_name = wm_name;
XSetWMProperties(dpy, win,
&wm_name, &icon_name,
argv, argc,
&sizeHints, &wmHints, 0);
if (fullscreen) {
set_nodecorations_motif(dpy, win);
set_fs_ewmh(dpy, win);
}
}
static void print_redir(const
Re: [PATCH 3/4] composite: Get rid of the internal UnmapWindow+MapWindow cycle
On Mon, Dec 20, 2010 at 01:04:07PM -0500, ext Adam Jackson wrote: > On Mon, 2010-12-20 at 18:05 +0200, ville.syrj...@nokia.com wrote: > > From: Ville Syrjälä > > > > Eliminate the internal MapWindow+UnmapWindow cycle around window > > redirection changes. Instead do the work in a single pass by marking > > the afected windows and calling ValidateTree and HandleExposures > > directly. This gets rid of unnecessary expose events, and invalid > > ClipNotify calls during rediredction changes. Now ClipNotify will only > > get called with the final clip values, and expose events are only sent > > to areas that actually got exposed. > > This can't be right, I don't think. You've eliminated the Map/Unmap > cycle - entirely laudable, thank you for taking it on - but in doing so > you're not replacing it with a call path that ever hits ->RealizeWindow, > so the backing pixmaps aren't ever going to be created. > > Am I missing something? compCheckRedirect() -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] xfree86/modes: Take rotation into account when checking mode size
On Thu, Dec 16, 2010 at 01:50:25PM +0200, ext ville.syrj...@nokia.com wrote:
> From: Ville Syrjälä
>
> Assume that a mode can be used in either landscape or portrait
> orientation. I suppose the correct thing to do would be to collect all
> the supported rotations from the CRTCs that can be used with a specific
> output, but that information doesn't seem to be readily available when
> these checks are done. So just assume that either orientation is fine.
>
> Signed-off-by: Ville Syrjälä
> ---
> hw/xfree86/modes/xf86Modes.c |9 ++---
> 1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/xfree86/modes/xf86Modes.c b/hw/xfree86/modes/xf86Modes.c
> index 05f4319..26db2c2 100644
> --- a/hw/xfree86/modes/xf86Modes.c
> +++ b/hw/xfree86/modes/xf86Modes.c
> @@ -365,13 +365,16 @@ xf86ValidateModesSize(ScrnInfoPtr pScrn, DisplayModePtr
> modeList,
> DisplayModePtr mode;
>
> for (mode = modeList; mode != NULL; mode = mode->next) {
> - if (maxPitch > 0 && mode->HDisplay > maxPitch)
> + if (maxPitch > 0 && xf86ModeWidth(mode, RR_Rotate_0) > maxPitch &&
> + xf86ModeWidth(mode, RR_Rotate_90) > maxPitch)
> mode->status = MODE_BAD_WIDTH;
>
> - if (maxX > 0 && mode->HDisplay > maxX)
> + if (maxX > 0 && xf86ModeWidth(mode, RR_Rotate_0) > maxX &&
> + xf86ModeWidth(mode, RR_Rotate_90) > maxX)
> mode->status = MODE_VIRTUAL_X;
>
> - if (maxY > 0 && mode->VDisplay > maxY)
> + if (maxY > 0 && xf86ModeHeight(mode, RR_Rotate_0) > maxY &&
> + xf86ModeHeight(mode, RR_Rotate_90) > maxY)
> mode->status = MODE_VIRTUAL_Y;
And of course that logic is totally broken. My brain wasn't in gear
when I wrote this. I'll cook up a patch that actually works.
>
> if (mode->next == modeList)
> --
> 1.7.2.2
>
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
--
Ville Syrjälä
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
