Re: [PATCHv2] Remove static MAXSCREENS limit from Xext/shm.c.

2009-10-28 Thread Keith Packard 
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.

2009-10-02 Thread Eric Anholt 
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 @@ static int
  ProcPanoramiXShmGetImage(ClientPtr client)
  {
  PanoramiXRes *draw;
 -DrawablePtr  drawables[MAXSCREENS];
  DrawablePtr  pDraw;

Re: [PATCHv2] Remove static MAXSCREENS limit from Xext/shm.c.

2009-10-02 Thread Jamey Sharp 
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

[PATCHv2] Remove static MAXSCREENS limit from Xext/shm.c.

2009-10-01 Thread Jamey Sharp 
---
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, stuff-height, stuff-depth,