[PATCH] dri2: Pass a ScreenPtr through to the driver's AuthMagic

[Kristian Høgsberg]

On Fri, Jun 15, 2012 at 07:01:34PM +1000, Christopher James Halse Rogers wrote:
> xwayland drivers need access to their screen private data to authenticate.
> Now that drivers no longer have direct access to the global screen arrays,
> this needs to be passed in as function context. The way it was working
> was ugly, anyway.

Yes, sure, but it also didn't need to extend the DRI2 API.  Now that
Dave's gone and locked up the screen arrays, we have a good excuse to
go clean that up.

> Signed-off-by: Christopher James Halse Rogers <christopher.halse.rogers at canonical.com>
> ---
> 
> This came up when I got around to fixing the nouveau xwayland patch review
> comments; with things no longer meant to access the global xf86Screens array
> the authmagic callback needed some other way to access driver private data.
> 
> Nouveau patch using this follows; I'll send it upstream once this gets
> applied somewhere.
> 
> CCd to xorg-devel mainly for sanity review, but could be applied; as Airlied
> has pointed out, there's no particular reason for the xwayland branch to
> diverge on core infrastructure.

Yeah, this is good to go upstream.  Just add a new function pointer to
the struct and a new typedef though, no need to play games with
casting the pointers:

  typedef int (*DRI2AuthMagicWithScreenProcPtr) (ScreenPtr pScreen, int fd, uint32_t magic);

or something.  Aside from that and the small nit-pick below, it looks
good.

Kristian

> 
>  hw/xfree86/dri2/dri2.c |   40 ++++++++++++++++++++++++++++++++++------
>  hw/xfree86/dri2/dri2.h |    2 +-
>  2 files changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> index babf32f..0e79875 100644
> --- a/hw/xfree86/dri2/dri2.c
> +++ b/hw/xfree86/dri2/dri2.c
> @@ -89,6 +89,8 @@ typedef struct _DRI2Drawable {
>      Bool needInvalidate;
>  } DRI2DrawableRec, *DRI2DrawablePtr;
>  
> +typedef int (*DRI2LegacyAuthMagicProcPtr) (int fd, uint32_t magic);
> +
>  typedef struct _DRI2Screen {
>      ScreenPtr screen;
>      int refcnt;
> @@ -105,6 +107,7 @@ typedef struct _DRI2Screen {
>      DRI2GetMSCProcPtr GetMSC;
>      DRI2ScheduleWaitMSCProcPtr ScheduleWaitMSC;
>      DRI2AuthMagicProcPtr AuthMagic;
> +    DRI2LegacyAuthMagicProcPtr LegacyAuthMagic;
>      DRI2ReuseBufferNotifyProcPtr ReuseBufferNotify;
>      DRI2SwapLimitValidateProcPtr SwapLimitValidate;
>      DRI2GetParamProcPtr GetParam;
> @@ -1110,12 +1113,22 @@ DRI2Connect(ScreenPtr pScreen, unsigned int driverType, int *fd,
>      return TRUE;
>  }
>  
> +static Bool
> +DRI2AuthMagic (ScreenPtr pScreen, int fd, uint32_t magic)
> +{
> +    DRI2ScreenPtr ds = DRI2GetScreen(pScreen);
> +    if (ds == NULL || (*ds->LegacyAuthMagic) (ds->fd, magic))
> +        return FALSE;

Don't need to check ds == NULL here, we're only called when ds is non-NULL.

> +    return TRUE;
> +}
> +
>  Bool
>  DRI2Authenticate(ScreenPtr pScreen, uint32_t magic)
>  {
>      DRI2ScreenPtr ds = DRI2GetScreen(pScreen);
>  
> -    if (ds == NULL || (*ds->AuthMagic) (ds->fd, magic))
> +    if (ds == NULL || (*ds->AuthMagic) (pScreen, ds->fd, magic))
>          return FALSE;
>  
>      return TRUE;
> @@ -1202,9 +1215,17 @@ DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info)
>          cur_minor = 1;
>      }
>  
> -    if (info->version >= 5) {
> +    if (info->version >= 7) {
>          ds->AuthMagic = info->AuthMagic;
>      }
> +    else if (info->version >= 5) {
> +        /*
> +         * This cast is safe; if the driver has provided a V5 or V6 InfoRec
> +         * then AuthMagic is of type DRI2LegacyAuthMagicProcPtr, and the C
> +         * standard guarantees that we can typecast it back and call it.
> +         */
> +        ds->LegacyAuthMagic = (DRI2LegacyAuthMagicProcPtr)info->AuthMagic;
> +    }
>  
>      if (info->version >= 6) {
>          ds->ReuseBufferNotify = info->ReuseBufferNotify;
> @@ -1218,14 +1239,21 @@ DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info)
>  
>      /*
>       * if the driver doesn't provide an AuthMagic function or the info struct
> -     * version is too low, it relies on the old method (using libdrm) or fail
> +     * version is too low, call through LegacyAuthMagic
>       */
> -    if (!ds->AuthMagic)
> +    if (!ds->AuthMagic) {
> +        ds->AuthMagic = DRI2AuthMagic;
> +        /*
> +         * If the driver doesn't provide an AuthMagic function
> +         * it relies on the old method (using libdrm) or fails
> +         */
> +        if (!ds->LegacyAuthMagic)
>  #ifdef WITH_LIBDRM
> -        ds->AuthMagic = drmAuthMagic;
> +            ds->LegacyAuthMagic = drmAuthMagic;
>  #else
> -        goto err_out;
> +            goto err_out;
>  #endif
> +    }
>  
>      /* Initialize minor if needed and set to minimum provied by DDX */
>      if (!dri2_minor || dri2_minor > cur_minor)
> diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h
> index f849be6..90886a9 100644
> --- a/hw/xfree86/dri2/dri2.h
> +++ b/hw/xfree86/dri2/dri2.h
> @@ -63,7 +63,7 @@ typedef void (*DRI2CopyRegionProcPtr) (DrawablePtr pDraw,
>                                         DRI2BufferPtr pDestBuffer,
>                                         DRI2BufferPtr pSrcBuffer);
>  typedef void (*DRI2WaitProcPtr) (WindowPtr pWin, unsigned int sequence);
> -typedef int (*DRI2AuthMagicProcPtr) (int fd, uint32_t magic);
> +typedef int (*DRI2AuthMagicProcPtr) (ScreenPtr pScreen, int fd, uint32_t magic);
>  
>  /**
>   * Schedule a buffer swap
> -- 
> 1.7.10.4
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel