Re: [PATCHv2] Remove static MAXSCREENS limit from Xext/shm.c.
Excerpts from Jamey Sharp's message of Fri Oct 02 14:10:22 -0700 2009: > I'll fix these and the lack of a CloseScreen cleanup hook and send > another patch. Thanks! I haven't seen another version of this patch float by; did I miss it? -- keith.pack...@intel.com signature.asc Description: PGP signature ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCHv2] Remove static MAXSCREENS limit from Xext/shm.c.
Thanks for review, Eric! On Thu, Oct 01, 2009 at 01:53:15PM -0700, Eric Anholt wrote: > On Thu, 2009-10-01 at 10:47 -0700, Jamey Sharp wrote: > > +drawables = xcalloc(screenInfo.numScreens, sizeof(DrawablePtr)); > > Seems like this drawable pointer should be part of the screen private. That would be nice, but ProcPanoramiXShmGetImage seems to need an actual array. I didn't dig deeper to determine whether that array could be eliminated entirely, but I think a change like that would be more invasive than I'm quite prepared for. > > void > > ShmRegisterFuncs(ScreenPtr pScreen, ShmFuncsPtr funcs) > > { > > -shmFuncs[pScreen->myNum] = funcs; > > +ShmInitScreenPriv(pScreen)->shmFuncs = funcs; > > } > > I think this one might be a GetScreenPriv instead? I'm guessing that > ShmExtensionInit happens (set up protocol stuff), then later on DDXes > call ShmRegisterFuncs on their screen. That was my first guess too, but it seems to be the other way around. This was the crash at server startup in my first version of this patch. > > + ShmScrPrivateRec *priv; > > pScreen = screenInfo.screens[j]; > > > > - pMap = (*shmFuncs[j]->CreatePixmap)(pScreen, > > + priv = ShmGetScreenPriv(pScreen); > > Stylistically, I like private fetching to be part of the declaration. > Less chance for early dereferencing. Also, as many layers have screen > privates, pixmap privates, gc privates, etc., it can be nice to name the > variables for the privates screen_priv, pixmap_priv, etc. I'll fix these and the lack of a CloseScreen cleanup hook and send another patch. Thanks! Jamey signature.asc Description: Digital signature ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCHv2] Remove static MAXSCREENS limit from Xext/shm.c.
On Thu, 2009-10-01 at 10:47 -0700, Jamey Sharp wrote: > --- > I didn't test my previous patch right. Sorry. This version doesn't seem > to crash the server at startup. :-) > > Review would still be greatly appreciated. Thanks for jumping in! Comments inline. > Xext/shm.c | 67 +++ > 1 files changed, 49 insertions(+), 18 deletions(-) > > diff --git a/Xext/shm.c b/Xext/shm.c > index a6f804c..b4167ac 100644 > --- a/Xext/shm.c > +++ b/Xext/shm.c > @@ -99,6 +99,11 @@ typedef struct _ShmDesc { > unsigned long size; > } ShmDescRec, *ShmDescPtr; > > +typedef struct _ShmScrPrivateRec { > +ShmFuncsPtr shmFuncs; > +DestroyPixmapProcPtr destroyPixmap; > +} ShmScrPrivateRec; > + > static PixmapPtr fbShmCreatePixmap(XSHM_CREATE_PIXMAP_ARGS); > static int ShmDetachSegment( > pointer /* value */, > @@ -135,13 +140,16 @@ int BadShmSegCode; > RESTYPE ShmSegType; > static ShmDescPtr Shmsegs; > static Bool sharedPixmaps; > -static ShmFuncsPtr shmFuncs[MAXSCREENS]; > -static DestroyPixmapProcPtr destroyPixmap[MAXSCREENS]; > +static DrawablePtr *drawables; > +static int shmScrPrivateKeyIndex; > +static DevPrivateKey shmScrPrivateKey = &shmScrPrivateKeyIndex; > static int shmPixmapPrivateIndex; > static DevPrivateKey shmPixmapPrivate = &shmPixmapPrivateIndex; > static ShmFuncs miFuncs = {NULL, NULL}; > static ShmFuncs fbFuncs = {fbShmCreatePixmap, NULL}; > > +#define ShmGetScreenPriv(s) ((ShmScrPrivateRec > *)dixLookupPrivate(&(s)->devPrivates, shmScrPrivateKey)) > + > #define VERIFY_SHMSEG(shmseg,shmdesc,client) \ > { \ > int rc; \ > @@ -212,6 +220,18 @@ static Bool CheckForShmSyscall(void) > > #endif > > +static ShmScrPrivateRec * > +ShmInitScreenPriv(ScreenPtr pScreen) > +{ > +ShmScrPrivateRec *priv = ShmGetScreenPriv(pScreen); > +if (!priv) > +{ > + priv = xcalloc (1, sizeof (ShmScrPrivateRec)); > + dixSetPrivate(&pScreen->devPrivates, shmScrPrivateKey, priv); > +} > +return priv; > +} > + > void > ShmExtensionInit(INITARGS) > { > @@ -226,20 +246,29 @@ ShmExtensionInit(INITARGS) > } > #endif > > +drawables = xcalloc(screenInfo.numScreens, sizeof(DrawablePtr)); > +if (!drawables) > +{ > + ErrorF("MIT-SHM extension disabled: no memory for per-screen > drawables\n"); > + return; > +} Seems like this drawable pointer should be part of the screen private. > + > sharedPixmaps = xFalse; > { >sharedPixmaps = xTrue; >for (i = 0; i < screenInfo.numScreens; i++) >{ > - if (!shmFuncs[i]) > - shmFuncs[i] = &miFuncs; > - if (!shmFuncs[i]->CreatePixmap) > + ShmScrPrivateRec *priv = ShmInitScreenPriv(screenInfo.screens[i]); > + if (!priv->shmFuncs) > + priv->shmFuncs = &miFuncs; > + if (!priv->shmFuncs->CreatePixmap) > sharedPixmaps = xFalse; >} >if (sharedPixmaps) > for (i = 0; i < screenInfo.numScreens; i++) > { > - destroyPixmap[i] = screenInfo.screens[i]->DestroyPixmap; > + ShmScrPrivateRec *priv = ShmGetScreenPriv(screenInfo.screens[i]); > + priv->destroyPixmap = screenInfo.screens[i]->DestroyPixmap; > screenInfo.screens[i]->DestroyPixmap = ShmDestroyPixmap; > } > } > @@ -261,23 +290,21 @@ static void > ShmResetProc(ExtensionEntry *extEntry) > { > int i; > - > -for (i = 0; i < MAXSCREENS; i++) > -{ > - shmFuncs[i] = NULL; > -} > +for (i = 0; i < screenInfo.numScreens; i++) > + ShmRegisterFuncs(screenInfo.screens[i], NULL); > } > > void > ShmRegisterFuncs(ScreenPtr pScreen, ShmFuncsPtr funcs) > { > -shmFuncs[pScreen->myNum] = funcs; > +ShmInitScreenPriv(pScreen)->shmFuncs = funcs; > } I think this one might be a GetScreenPriv instead? I'm guessing that ShmExtensionInit happens (set up protocol stuff), then later on DDXes call ShmRegisterFuncs on their screen. > > static Bool > ShmDestroyPixmap (PixmapPtr pPixmap) > { > ScreenPtrpScreen = pPixmap->drawable.pScreen; > +ShmScrPrivateRec *priv; > Bool ret; > if (pPixmap->refcnt == 1) > { > @@ -288,9 +315,10 @@ ShmDestroyPixmap (PixmapPtr pPixmap) > ShmDetachSegment ((pointer) shmdesc, pPixmap->drawable.id); > } > > -pScreen->DestroyPixmap = destroyPixmap[pScreen->myNum]; > +priv = ShmGetScreenPriv(pScreen); > +pScreen->DestroyPixmap = priv->destroyPixmap; > ret = (*pScreen->DestroyPixmap) (pPixmap); > -destroyPixmap[pScreen->myNum] = pScreen->DestroyPixmap; > +priv->destroyPixmap = pScreen->DestroyPixmap; > pScreen->DestroyPixmap = ShmDestroyPixmap; > return ret; > } > @@ -298,7 +326,7 @@ ShmDestroyPixmap (PixmapPtr pPixmap) > void > ShmRegisterFbFuncs(ScreenPtr pScreen) > { > -shmFuncs[pScreen->myNum] = &fbFuncs; > +ShmRegisterFuncs(pScreen, &fbFuncs); > } > > static int > @@ -578,7 +606,6 @@
[PATCHv2] Remove static MAXSCREENS limit from Xext/shm.c.
--- I didn't test my previous patch right. Sorry. This version doesn't seem to crash the server at startup. :-) Review would still be greatly appreciated. Xext/shm.c | 67 +++ 1 files changed, 49 insertions(+), 18 deletions(-) diff --git a/Xext/shm.c b/Xext/shm.c index a6f804c..b4167ac 100644 --- a/Xext/shm.c +++ b/Xext/shm.c @@ -99,6 +99,11 @@ typedef struct _ShmDesc { unsigned long size; } ShmDescRec, *ShmDescPtr; +typedef struct _ShmScrPrivateRec { +ShmFuncsPtr shmFuncs; +DestroyPixmapProcPtr destroyPixmap; +} ShmScrPrivateRec; + static PixmapPtr fbShmCreatePixmap(XSHM_CREATE_PIXMAP_ARGS); static int ShmDetachSegment( pointer/* value */, @@ -135,13 +140,16 @@ int BadShmSegCode; RESTYPE ShmSegType; static ShmDescPtr Shmsegs; static Bool sharedPixmaps; -static ShmFuncsPtr shmFuncs[MAXSCREENS]; -static DestroyPixmapProcPtr destroyPixmap[MAXSCREENS]; +static DrawablePtr *drawables; +static int shmScrPrivateKeyIndex; +static DevPrivateKey shmScrPrivateKey = &shmScrPrivateKeyIndex; static int shmPixmapPrivateIndex; static DevPrivateKey shmPixmapPrivate = &shmPixmapPrivateIndex; static ShmFuncs miFuncs = {NULL, NULL}; static ShmFuncs fbFuncs = {fbShmCreatePixmap, NULL}; +#define ShmGetScreenPriv(s) ((ShmScrPrivateRec *)dixLookupPrivate(&(s)->devPrivates, shmScrPrivateKey)) + #define VERIFY_SHMSEG(shmseg,shmdesc,client) \ { \ int rc; \ @@ -212,6 +220,18 @@ static Bool CheckForShmSyscall(void) #endif +static ShmScrPrivateRec * +ShmInitScreenPriv(ScreenPtr pScreen) +{ +ShmScrPrivateRec *priv = ShmGetScreenPriv(pScreen); +if (!priv) +{ + priv = xcalloc (1, sizeof (ShmScrPrivateRec)); + dixSetPrivate(&pScreen->devPrivates, shmScrPrivateKey, priv); +} +return priv; +} + void ShmExtensionInit(INITARGS) { @@ -226,20 +246,29 @@ ShmExtensionInit(INITARGS) } #endif +drawables = xcalloc(screenInfo.numScreens, sizeof(DrawablePtr)); +if (!drawables) +{ + ErrorF("MIT-SHM extension disabled: no memory for per-screen drawables\n"); + return; +} + sharedPixmaps = xFalse; { sharedPixmaps = xTrue; for (i = 0; i < screenInfo.numScreens; i++) { - if (!shmFuncs[i]) - shmFuncs[i] = &miFuncs; - if (!shmFuncs[i]->CreatePixmap) + ShmScrPrivateRec *priv = ShmInitScreenPriv(screenInfo.screens[i]); + if (!priv->shmFuncs) + priv->shmFuncs = &miFuncs; + if (!priv->shmFuncs->CreatePixmap) sharedPixmaps = xFalse; } if (sharedPixmaps) for (i = 0; i < screenInfo.numScreens; i++) { - destroyPixmap[i] = screenInfo.screens[i]->DestroyPixmap; + ShmScrPrivateRec *priv = ShmGetScreenPriv(screenInfo.screens[i]); + priv->destroyPixmap = screenInfo.screens[i]->DestroyPixmap; screenInfo.screens[i]->DestroyPixmap = ShmDestroyPixmap; } } @@ -261,23 +290,21 @@ static void ShmResetProc(ExtensionEntry *extEntry) { int i; - -for (i = 0; i < MAXSCREENS; i++) -{ - shmFuncs[i] = NULL; -} +for (i = 0; i < screenInfo.numScreens; i++) + ShmRegisterFuncs(screenInfo.screens[i], NULL); } void ShmRegisterFuncs(ScreenPtr pScreen, ShmFuncsPtr funcs) { -shmFuncs[pScreen->myNum] = funcs; +ShmInitScreenPriv(pScreen)->shmFuncs = funcs; } static Bool ShmDestroyPixmap (PixmapPtr pPixmap) { ScreenPtr pScreen = pPixmap->drawable.pScreen; +ShmScrPrivateRec *priv; Bool ret; if (pPixmap->refcnt == 1) { @@ -288,9 +315,10 @@ ShmDestroyPixmap (PixmapPtr pPixmap) ShmDetachSegment ((pointer) shmdesc, pPixmap->drawable.id); } -pScreen->DestroyPixmap = destroyPixmap[pScreen->myNum]; +priv = ShmGetScreenPriv(pScreen); +pScreen->DestroyPixmap = priv->destroyPixmap; ret = (*pScreen->DestroyPixmap) (pPixmap); -destroyPixmap[pScreen->myNum] = pScreen->DestroyPixmap; +priv->destroyPixmap = pScreen->DestroyPixmap; pScreen->DestroyPixmap = ShmDestroyPixmap; return ret; } @@ -298,7 +326,7 @@ ShmDestroyPixmap (PixmapPtr pPixmap) void ShmRegisterFbFuncs(ScreenPtr pScreen) { -shmFuncs[pScreen->myNum] = &fbFuncs; +ShmRegisterFuncs(pScreen, &fbFuncs); } static int @@ -578,7 +606,6 @@ static int ProcPanoramiXShmGetImage(ClientPtr client) { PanoramiXRes *draw; -DrawablePtrdrawables[MAXSCREENS]; DrawablePtrpDraw; xShmGetImageReply xgi; ShmDescPtr shmdesc; @@ -767,9 +794,11 @@ CreatePmap: result = (client->noClientException); FOR_NSCREENS(j) { + ShmScrPrivateRec *priv; pScreen = screenInfo.screens[j]; - pMap = (*shmFuncs[j]->CreatePixmap)(pScreen, + priv = ShmGetScreenPriv(pScreen); + pMap = (*priv->shmFuncs->CreatePixmap)(pScreen, stuff->width, st