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 <kei...@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 <kei...@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 _______________________________________________ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel