Re: [PATCH 3/9] dri2: Change driver interface to support DRI2Drawable

Christopher James Halse Rogers Thu, 03 Feb 2011 18:35:16 -0800

On Thu, 2011-02-03 at 19:48 +0200, Pauli wrote:
> From: Pauli Nieminen <[email protected]>
> 
> 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.
> 
> Signed-off-by: Pauli Nieminen <[email protected]>
> ---
>  hw/xfree86/dri2/dri2.c |   69 +++++++++++++++++++++++------------------------
>  hw/xfree86/dri2/dri2.h |   14 +++++++--
>  2 files changed, 45 insertions(+), 38 deletions(-)
> 
> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> index 91ae1a0..f564822 100644
> --- a/hw/xfree86/dri2/dri2.c
> +++ b/hw/xfree86/dri2/dri2.c
> @@ -137,6 +137,12 @@ DRI2DrawableGetDrawable(DRI2DrawablePtr pPriv)
>      return pPriv->drawable;
>  }
>  
> +ScreenPtr
> +DRI2DrawableGetScreen(DRI2DrawablePtr pPriv)
> +{
> +    return pPriv->dri2_screen->screen;
> +}
> +
>  static unsigned long
>  DRI2DrawableSerial(DrawablePtr pDraw)
>  {
> @@ -323,6 +329,14 @@ static int DRI2DrawableGone(pointer p, XID id)
>       return Success;
>  
>      pDraw = pPriv->drawable;
> +
> +    if (pPriv->buffers != NULL) {
> +     for (i = 0; i < pPriv->bufferCount; i++)
> +         (*ds->DestroyBuffer)(pPriv, pPriv->buffers[i]);
> +
> +     free(pPriv->buffers);
> +    }
> +
>      if (pDraw->type == DRAWABLE_WINDOW) {
>       pWin = (WindowPtr) pDraw;
>       dixSetPrivate(&pWin->devPrivates, dri2WindowPrivateKey, NULL);
> @@ -331,13 +345,6 @@ static int DRI2DrawableGone(pointer p, XID id)
>       dixSetPrivate(&pPixmap->devPrivates, dri2PixmapPrivateKey, NULL);
>      }
>  
> -    if (pPriv->buffers != NULL) {
> -     for (i = 0; i < pPriv->bufferCount; i++)
> -         (*ds->DestroyBuffer)(pDraw, pPriv->buffers[i]);
> -
> -     free(pPriv->buffers);
> -    }
> -


I can't see any reason to move the buffer-destruction loop?  If there
isn't, not moving it would make the functional part of this diff more
obvious.

>      free(pPriv);
>  
>      return Success;
> @@ -394,7 +401,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 +538,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 +691,10 @@ DRI2CanExchange(DrawablePtr pDraw)
>  }
>  
>  void
> -DRI2WaitMSCComplete(ClientPtr client, DrawablePtr pDraw, int frame,
> +DRI2WaitMSCComplete(ClientPtr client, DRI2DrawablePtr pPriv, int frame,
>                   unsigned int tv_sec, unsigned int tv_usec)
>  {
> -    DRI2DrawablePtr pPriv;
>  
> -    pPriv = DRI2GetDrawable(pDraw);
> -    if (pPriv == NULL)
> -     return;
>  
>      ProcDRI2WaitMSCReply(client, ((CARD64)tv_sec * 1000000) + tv_usec,
>                        frame, pPriv->swap_count);
> @@ -740,33 +743,29 @@ DRI2WakeClient(ClientPtr client, DRI2DrawablePtr pPriv, 
> int frame,
>  }
>  
>  void
> -DRI2SwapComplete(ClientPtr client, DrawablePtr pDraw, int frame,
> +DRI2SwapComplete(ClientPtr client, DRI2DrawablePtr pPriv, int frame,
>                  unsigned int tv_sec, unsigned int tv_usec, int type,
>                  DRI2SwapEventPtr swap_complete, void *swap_data)
>  {
> -    ScreenPtr            pScreen = pDraw->pScreen;
> -    DRI2DrawablePtr pPriv;
> +    DrawablePtr     pDraw = pPriv->drawable;
>      CARD64          ust = 0;
> -    BoxRec          box;
> -    RegionRec       region;
> -
> -    pPriv = DRI2GetDrawable(pDraw);
> -    if (pPriv == NULL) {
> -        xf86DrvMsg(pScreen->myNum, X_ERROR,
> -                "[DRI2] %s: bad drawable\n", __func__);
> -     return;
> -    }
>  
>      pPriv->swapsPending--;
>      pPriv->swap_count++;
>  
> -    box.x1 = 0;
> -    box.y1 = 0;
> -    box.x2 = pDraw->width;
> -    box.y2 = pDraw->height;
> -    RegionInit(&region, &box, 0);
> -    DRI2CopyRegion(pPriv, &region, DRI2BufferFakeFrontLeft,
> -                DRI2BufferFrontLeft);
> +
> +    if (pDraw) {
> +     BoxRec          box;
> +     RegionRec       region;
> +
> +     box.x1 = 0;
> +     box.y1 = 0;
> +     box.x2 = pDraw->width;
> +     box.y2 = pDraw->height;
> +     RegionInit(&region, &box, 0);
> +     DRI2CopyRegion(pPriv, &region, DRI2BufferFakeFrontLeft,
> +                    DRI2BufferFrontLeft);
> +    }
>  
>      ust = ((CARD64)tv_sec * 1000000) + tv_usec;
>      if (swap_complete)
> @@ -839,7 +838,7 @@ DRI2SwapBuffers(ClientPtr client, DRI2DrawablePtr pPriv, 
> CARD64 target_msc,
>       pPriv->swapsPending++;
>  
>       (*ds->CopyRegion)(pDraw, &region, pDestBuffer, pSrcBuffer);
> -     DRI2SwapComplete(client, pDraw, target_msc, 0, 0, DRI2_BLIT_COMPLETE,
> +     DRI2SwapComplete(client, pPriv, target_msc, 0, 0, DRI2_BLIT_COMPLETE,
>                        func, data);
>       return Success;
>      }
> @@ -954,7 +953,7 @@ DRI2WaitMSC(ClientPtr client, DRI2DrawablePtr pPriv, 
> CARD64 target_msc,
>  
>      /* Old DDX just completes immediately */
>      if (!ds->ScheduleWaitMSC) {
> -     DRI2WaitMSCComplete(client, pDraw, target_msc, 0, 0);
> +     DRI2WaitMSCComplete(client, pPriv, target_msc, 0, 0);
>  
>       return Success;
>      }
> diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h
> index 509d4f6..0182700 100644
> --- a/hw/xfree86/dri2/dri2.h
> +++ b/hw/xfree86/dri2/dri2.h
> @@ -111,7 +111,7 @@ typedef int               
> (*DRI2ScheduleSwapProcPtr)(ClientPtr client,
>  typedef DRI2BufferPtr        (*DRI2CreateBufferProcPtr)(DrawablePtr pDraw,
>                                                  unsigned int attachment,
>                                                  unsigned int format);
> -typedef void         (*DRI2DestroyBufferProcPtr)(DrawablePtr pDraw,
> +typedef void         (*DRI2DestroyBufferProcPtr)(DRI2DrawablePtr pPriv,
>                                                   DRI2BufferPtr buffer);
>  /**
>   * Get current media stamp counter values
> @@ -282,12 +282,12 @@ extern _X_EXPORT Bool DRI2CanExchange(DrawablePtr 
> pDraw);
>  /* Note: use *only* for MSC related waits */
>  extern _X_EXPORT void DRI2BlockClient(ClientPtr client, DrawablePtr pDraw);
>  
> -extern _X_EXPORT void DRI2SwapComplete(ClientPtr client, DrawablePtr pDraw,
> +extern _X_EXPORT void DRI2SwapComplete(ClientPtr client, DRI2DrawablePtr 
> pPriv,
>                                      int frame, unsigned int tv_sec,
>                                      unsigned int tv_usec, int type,
>                                      DRI2SwapEventPtr swap_complete,
>                                      void *swap_data);
> -extern _X_EXPORT void DRI2WaitMSCComplete(ClientPtr client, DrawablePtr 
> pDraw,
> +extern _X_EXPORT void DRI2WaitMSCComplete(ClientPtr client, DRI2DrawablePtr 
> pPriv,
>                                         int frame, unsigned int tv_sec,
>                                         unsigned int tv_usec);
>  /**
> @@ -301,6 +301,14 @@ extern _X_EXPORT void DRI2WaitMSCComplete(ClientPtr 
> client, DrawablePtr pDraw,
>  extern _X_EXPORT DrawablePtr DRI2DrawableGetDrawable(DRI2DrawablePtr pPriv);
>  
>  /**
> + * Provides access to ScreenPtr trough DRI2DrawablePtr
> + *
> + * \param pPriv DRI2 private drawable
> + * \return valid pointer to Screen
> + */
> +extern _X_EXPORT ScreenPtr DRI2DrawableGetScreen(DRI2DrawablePtr pPriv);
> +
> +/**
>   * Provides access to DRI2DrawablePtr trough DrawablePtr
>   *
>   * \param pDraw drawable pointer

You've changed the driver DRI2 interface here in a way that could cause
old drivers to crash the server.  Shouldn't you also bump
DRI2INFOREC_VERSION and refuse to load old drivers in DRI2ScreenInit?

signature.asc
Description: This is a digitally signed message part

_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 3/9] dri2: Change driver interface to support DRI2Drawable

Reply via email to