Removing direct graphics access from DGA
Peter Hutterer
peter.hutterer at who-t.net
Mon Sep 21 05:04:50 PDT 2009
On Fri, Sep 18, 2009 at 10:28:15PM -0700, Keith Packard wrote:
> On Thu, 2009-09-17 at 10:24 +1000, Dave Airlie wrote:
>
> > As long as someone tests quake 1 still works ;-)
>
> Ok, the DGA situation is actually worse than I thought. Any driver
> supporting framebuffer resizing that uses the xf86DiDGAInit function
> will expose the system to potentially catastrophic failures.
>
> The xf86DiDGAInit function takes a static address (in the server's
> address space) for the base of the scanout buffer. This value is given
> to clients to mmap from /dev/mem themselves.
>
> It's important to note here that we're returning a random internal X
> server address space value to applications and encouraging them to mmap
> that part of CPU physical address space and write pixels there. Clearly,
> this code hasn't ever been used by an application as it could never have
> worked.
>
> The other operation that DGA supports is to offer a few rendering
> operations on the framebuffer as well as the ability to create a pixmap
> representing the framebuffer for use by regular X drawing functions. I'm
> betting the only client using this is the DGA2 example application
> 'skull_dga' -- it's not exactly a compelling use case (a magic mechanism
> to create a drawable representing the screen just doesn't seem
> interesting to me). I know that when I googled around, I couldn't find
> anything other than the DGA man page referencing these functions.
>
> I'm sure this could all be fixed with sufficient energy and time, but I
> suggest that it would be better to just not do this anymore and leave
> DGA as a way for old applications to switch video modes and get direct
> mouse input. For these two operations, it seems to work just fine
> (although I did manage to uncover a bug in the intel driver for DGA mode
> switching).
>
> The trick, however, is how to make these 'safe' operations available
> while not exposing the 'bad' parts. The xf86DiDGA code doesn't allow
> this -- you either get all of DGA, including the bad parts, or you get
> none of it. So, a cautious driver must never initialize DGA for fear
> that it will be run against an old X server.
>
> The solution that I came up with is to have xf86CrtcInit call
> xf86DiDGAInit and remove the call to xf86DiDGAInit from the video
> driver. Thus, the driver running against an old server gets no DGA
> (ether bad or safe parts) and when run against a new server gets just
> the safe parts.
>
> I've tested this with gl-117 and warsow, both of which appear to use DGA
> input. Additional testing is encouraged, of course.
>
> I don't think this patch is 'optional' for either X server 1.7 or 1.6.4;
> it removes the potential for some fairly serious system-wide
> consequences while appearing to keep existing applications running.
>
> -keith
>
>
> From 8fd84838477db6195f60aa00a029dd8988b69cbc Mon Sep 17 00:00:00 2001
> From: Keith Packard <keithp at keithp.com>
> Date: Fri, 18 Sep 2009 21:12:17 -0700
> Subject: [PATCH] xfree86/modes: Remove all framebuffer support from DGA
>
> This removes all rendering and mapping code from xf86DiDGA, leaving
> just mode setting and raw input device access. The mapping code didn't
> have the offset within /dev/mem for the frame buffer and the pixmap
> support assumed that the framebuffer was never reallocated.
>
> Signed-off-by: Keith Packard <keithp at keithp.com>
> ---
> hw/xfree86/modes/xf86Crtc.c | 7 ++
> hw/xfree86/modes/xf86DiDGA.c | 119 +++++++---------------------------------
> hw/xfree86/modes/xf86RandR12.c | 8 +--
> 3 files changed, 29 insertions(+), 105 deletions(-)
>
> diff --git a/hw/xfree86/modes/xf86Crtc.c b/hw/xfree86/modes/xf86Crtc.c
> index c6dfd8c..c1e31e0 100644
> --- a/hw/xfree86/modes/xf86Crtc.c
> +++ b/hw/xfree86/modes/xf86Crtc.c
> @@ -805,6 +805,9 @@ xf86CrtcScreenInit (ScreenPtr screen)
> config->CloseScreen = screen->CloseScreen;
> screen->CloseScreen = xf86CrtcCloseScreen;
>
> +#ifdef XFreeXDGA
> + xf86DiDGAInit(screen, 0);
> +#endif
> #ifdef RANDR_13_INTERFACE
> return RANDR_INTERFACE_VERSION;
> #else
> @@ -1923,6 +1926,10 @@ xf86SetScrnInfoModes (ScrnInfoPtr scrn)
> }
> }
> scrn->currentMode = scrn->modes;
> +#ifdef XFreeXDGA
> + if (scrn->pScreen)
> + xf86DiDGAReInit(scrn->pScreen);
> +#endif
> }
>
> static void
> diff --git a/hw/xfree86/modes/xf86DiDGA.c b/hw/xfree86/modes/xf86DiDGA.c
> index 0964cef..0f7b834 100644
> --- a/hw/xfree86/modes/xf86DiDGA.c
> +++ b/hw/xfree86/modes/xf86DiDGA.c
> @@ -72,8 +72,7 @@ xf86_dga_get_modes (ScreenPtr pScreen)
> mode = modes + num++;
>
> mode->mode = display_mode;
> - mode->flags = DGA_CONCURRENT_ACCESS | DGA_PIXMAP_AVAILABLE;
> - mode->flags |= DGA_FILL_RECT | DGA_BLIT_RECT;
> + mode->flags = DGA_CONCURRENT_ACCESS;
> if (display_mode->Flags & V_DBLSCAN)
> mode->flags |= DGA_DOUBLESCAN;
> if (display_mode->Flags & V_INTERLACE)
> @@ -91,14 +90,14 @@ xf86_dga_get_modes (ScreenPtr pScreen)
> mode->yViewportStep = 1;
> mode->viewportFlags = DGA_FLIP_RETRACE;
> mode->offset = 0;
> - mode->address = (unsigned char *) xf86_config->dga_address;
> - mode->bytesPerScanline = xf86_config->dga_stride;
> - mode->imageWidth = xf86_config->dga_width;
> - mode->imageHeight = xf86_config->dga_height;
> + mode->address = 0;
> + mode->imageWidth = mode->viewportWidth;
> + mode->imageHeight = mode->viewportHeight;
> + mode->bytesPerScanline = (mode->imageWidth * scrn->bitsPerPixel) >> 3;
> mode->pixmapWidth = mode->imageWidth;
> mode->pixmapHeight = mode->imageHeight;
> - mode->maxViewportX = mode->imageWidth - mode->viewportWidth;
> - mode->maxViewportY = mode->imageHeight - mode->viewportHeight;
> + mode->maxViewportX = 0;
> + mode->maxViewportY = 0;
>
> display_mode = display_mode->next;
> if (display_mode == scrn->modes)
> @@ -149,93 +148,11 @@ xf86_dga_set_viewport(ScrnInfoPtr scrn, int x, int y, int flags)
> }
>
> static Bool
> -xf86_dga_get_drawable_and_gc (ScrnInfoPtr scrn, DrawablePtr *ppDrawable, GCPtr *ppGC)
> -{
> - ScreenPtr pScreen = scrn->pScreen;
> - xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
> - PixmapPtr pPixmap;
> - GCPtr pGC;
> -
> - pPixmap = GetScratchPixmapHeader (pScreen, xf86_config->dga_width, xf86_config->dga_height,
> - scrn->depth, scrn->bitsPerPixel, xf86_config->dga_stride,
> - (char *) scrn->memPhysBase + scrn->fbOffset);
> - if (!pPixmap)
> - return FALSE;
> - pGC = GetScratchGC (scrn->depth, pScreen);
> - if (!pGC)
> - {
> - FreeScratchPixmapHeader (pPixmap);
> - return FALSE;
> - }
> - *ppDrawable = &pPixmap->drawable;
> - *ppGC = pGC;
> - return TRUE;
> -}
> -
> -static void
> -xf86_dga_release_drawable_and_gc (ScrnInfoPtr scrn, DrawablePtr pDrawable, GCPtr pGC)
> -{
> - FreeScratchGC (pGC);
> - FreeScratchPixmapHeader ((PixmapPtr) pDrawable);
> -}
> -
> -static void
> -xf86_dga_fill_rect(ScrnInfoPtr scrn, int x, int y, int w, int h, unsigned long color)
> -{
> - GCPtr pGC;
> - DrawablePtr pDrawable;
> - XID vals[1];
> - xRectangle r;
> -
> - if (!xf86_dga_get_drawable_and_gc (scrn, &pDrawable, &pGC))
> - return;
> - vals[0] = color;
> - ChangeGC (pGC, GCForeground, vals);
> - ValidateGC (pDrawable, pGC);
> - r.x = x;
> - r.y = y;
> - r.width = w;
> - r.height = h;
> - pGC->ops->PolyFillRect (pDrawable, pGC, 1, &r);
> - xf86_dga_release_drawable_and_gc (scrn, pDrawable, pGC);
> -}
> -
> -static void
> -xf86_dga_sync(ScrnInfoPtr scrn)
> -{
> - ScreenPtr pScreen = scrn->pScreen;
> - WindowPtr pRoot = WindowTable [pScreen->myNum];
> - char buffer[4];
> -
> - pScreen->GetImage (&pRoot->drawable, 0, 0, 1, 1, ZPixmap, ~0L, buffer);
> -}
> -
> -static void
> -xf86_dga_blit_rect(ScrnInfoPtr scrn, int srcx, int srcy, int w, int h, int dstx, int dsty)
> -{
> - DrawablePtr pDrawable;
> - GCPtr pGC;
> -
> - if (!xf86_dga_get_drawable_and_gc (scrn, &pDrawable, &pGC))
> - return;
> - ValidateGC (pDrawable, pGC);
> - pGC->ops->CopyArea (pDrawable, pDrawable, pGC, srcx, srcy, w, h, dstx, dsty);
> - xf86_dga_release_drawable_and_gc (scrn, pDrawable, pGC);
> -}
> -
> -static Bool
> xf86_dga_open_framebuffer(ScrnInfoPtr scrn,
> char **name,
> unsigned char **mem, int *size, int *offset, int *flags)
> {
> - xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
> -
> - *size = xf86_config->dga_stride * xf86_config->dga_height;
> - *mem = (unsigned char *) (xf86_config->dga_address);
> - *offset = 0;
> - *flags = DGA_NEED_ROOT;
> -
> - return TRUE;
> + return FALSE;
> }
>
> static void
> @@ -249,9 +166,9 @@ static DGAFunctionRec xf86_dga_funcs = {
> xf86_dga_set_mode,
> xf86_dga_set_viewport,
> xf86_dga_get_viewport,
> - xf86_dga_sync,
> - xf86_dga_fill_rect,
> - xf86_dga_blit_rect,
> + NULL,
> + NULL,
> + NULL,
> NULL
> };
>
> @@ -261,6 +178,9 @@ xf86DiDGAReInit (ScreenPtr pScreen)
> ScrnInfoPtr scrn = xf86Screens[pScreen->myNum];
> xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
>
> + if (!DGAAvailable(pScreen->myNum))
> + return TRUE;
> +
> if (!xf86_dga_get_modes (pScreen))
> return FALSE;
>
> @@ -273,11 +193,14 @@ xf86DiDGAInit (ScreenPtr pScreen, unsigned long dga_address)
> ScrnInfoPtr scrn = xf86Screens[pScreen->myNum];
> xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
>
> + if (DGAAvailable(pScreen->myNum))
> + return TRUE;
> +
> xf86_config->dga_flags = 0;
> - xf86_config->dga_address = dga_address;
> - xf86_config->dga_width = scrn->virtualX;
> - xf86_config->dga_height = scrn->virtualY;
> - xf86_config->dga_stride = scrn->displayWidth * scrn->bitsPerPixel >> 3;
> + xf86_config->dga_address = 0;
> + xf86_config->dga_width = 0;
> + xf86_config->dga_height = 0;
> + xf86_config->dga_stride = 0;
>
> if (!xf86_dga_get_modes (pScreen))
> return FALSE;
> diff --git a/hw/xfree86/modes/xf86RandR12.c b/hw/xfree86/modes/xf86RandR12.c
> index c2465bc..6ea9d26 100644
> --- a/hw/xfree86/modes/xf86RandR12.c
> +++ b/hw/xfree86/modes/xf86RandR12.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright ? 2002 Keith Packard, member of The XFree86 Project, Inc.
> + * Copyright © 2002 Keith Packard, member of The XFree86 Project, Inc.
> *
> * Permission to use, copy, modify, distribute, and sell this software and its
> * documentation for any purpose is hereby granted without fee, provided that
> @@ -467,9 +467,6 @@ xf86RandR12GetInfo (ScreenPtr pScreen, Rotation *rotations)
> {
> xf86ProbeOutputModes (scrp, 0, 0);
> xf86SetScrnInfoModes (scrp);
> -#ifdef XFreeXDGA
> - xf86DiDGAReInit (pScreen);
> -#endif
> }
>
> for (mode = scrp->modes; ; mode = mode->next)
> @@ -1528,9 +1525,6 @@ xf86RandR12GetInfo12 (ScreenPtr pScreen, Rotation *rotations)
> return TRUE;
> xf86ProbeOutputModes (pScrn, 0, 0);
> xf86SetScrnInfoModes (pScrn);
> -#ifdef XFreeXDGA
> - xf86DiDGAReInit (pScreen);
> -#endif
> return xf86RandR12SetInfo12 (pScreen);
> }
>
> --
> 1.6.3.3
Thanks, pushed.
Cheers,
Peter
More information about the xorg-devel
mailing list