Re: [PATCH xserver 2/4] dri2: Split resource tracking for DRI2Drawable and references to them

[Adam Jackson]

On Wed, 2016-02-03 at 09:54 +, Chris Wilson wrote:

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

Newp. If AddResource fails it'll call DRI2DrawableGone for you, which
means in the absolute best case you'd double-free. Since you haven't
initialized any of pPriv at this point hell knows precisely what will
explode, but it'll be something.

>  ref = malloc(sizeof *ref);
>  if (ref == NULL)
>  return BadAlloc;
>  
> -if (!AddResource(dri2_id, dri2DrawableRes, pPriv)) {
> +if (!AddResource(pid, dri2ReferenceRes, ref)) {
>  free(ref);
>  return BadAlloc;
>  }

Same deal here, although here you're forgiven for the double-free since
it was already there.

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

[PATCH xserver 2/4] dri2: Split resource tracking for DRI2Drawable and references to them

[Chris Wilson]

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,
-  >dri2_id);
+private->dri2_id = FakeClientID(client->index);
+ret = DRI2CreateDrawable(client, pDraw, private->dri2_id,
+ __glXDRIinvalidateBuffers, private);
 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 ()
-
-#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(>devPrivates, );
+}
+
+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, >reference_list, link) {
-if (ref->id == id)
-return ref;
-}
-
-return NULL;
-}
+pPriv = DRI2GetDrawable(pDraw);
+if (pPriv == NULL)
+pPriv = DRI2AllocateDrawable(pDraw);
+if (pPriv == NULL)
+return BadAlloc;
 
-static int
-DRI2AddDrawableRef(DRI2DrawablePtr pPriv, XID id, XID dri2_id,
-   DRI2InvalidateProcPtr invalidate, void *priv)
-{
-DRI2DrawableRefPtr ref;
+pPriv->prime_id = dri2ClientPrivate(client)->prime_id;
 
 ref = malloc(sizeof *ref);
 if (ref == NULL)
 return BadAlloc;
 
-if (!AddResource(dri2_id, dri2DrawableRes, pPriv)) {
+if (!AddResource(pid, dri2ReferenceRes, ref)) {
 free(ref);
 return BadAlloc;
 }
-if (!DRI2LookupDrawableRef(pPriv, id))
-if (!AddResource(id, dri2DrawableRes, pPriv)) {
-FreeResourceByType(dri2_id, dri2DrawableRes, TRUE);
-free(ref);
-return BadAlloc;
-}
 
-ref->id = id;
-ref->dri2_id = dri2_id;
+ref->pid = pid;
 ref->invalidate = invalidate;
 ref->priv = priv;
 xorg_list_add(>link, >reference_list);
@@ -320,71 +313,32 @@ DRI2AddDrawableRef(DRI2DrawablePtr 

Re: [PATCH xserver 2/4] dri2: Split resource tracking for DRI2Drawable and references to them

[Ville Syrjälä]

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,
> -  >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 ()
> -
> -#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(>devPrivates, );
> +}
> +
> +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, >reference_list, link) {
> -if (ref->id == id)
> -return ref;
> -}
> -
> -return NULL;
> -}
> +pPriv = DRI2GetDrawable(pDraw);
> +if (pPriv == NULL)
> +pPriv = DRI2AllocateDrawable(pDraw);
> +if (pPriv == NULL)
> +return BadAlloc;
>  
> -static int
> -DRI2AddDrawableRef(DRI2DrawablePtr pPriv, XID id, XID dri2_id,
> -   DRI2InvalidateProcPtr