Certainly can't see any immediate problem David.
We can always cut a 4.3.1 if issues are found anyway.
Alan.
On Fri, Feb 14, 2003 at 04:17:16 -0500, David Dawes wrote:
> Egbert and I have been looking into the issues that are preventing a second
> X server to be started for i810/830M platforms when DRI is enabled. Several
> issues were found:
>
> 1. The i810 support doesn't unbind/release the agpgart module when VT
> switching away, and this prevents a second X server from acquiring
> it. Since the i810 driver requires agpgart access even for 2D
> operation, this is prevents a second X server from running. A fix
> for this is to add the unbind/release calls at LeaveVT, and
> acquire/rebind at EnterVT. The attached patch does this. It also
> makes a small change to the unbind code in the DRM kernel modules
> to handle this.
>
> 2. The 830M support does unbind/release the agpgart module, but code
> in DRM(release) called when closing a /dev/dri/* device blocks
> until it can obtain the lock. Since the first server holds the
> lock while switched away, the second one can never get it, so it
> ends up blocking in close(). The second server opens/closes the
> devices to find out whether DRI can be enabled. DRI can't be
> enabled on the second server (which isn't a problem), but this
> blocking behaviour is preventing it from starting up at all. I've
> found that this can be solved by keeping track of whether the opener
> has ever tried to acquire the lock, and not try to acquire it at
> close/release when it had never previously been acquired. The patch
> below implements this.
>
> 3. The drm module AGP support code keeps track of a "handle" for
> allocated AGP regions. It uses the memory address returned from
> the allocation for this purpose. This would normally be unique,
> but for the i810 driver case where "DCACHE" memory is "allocated".
> In the DCACHE case, the allocated memory is freed immediately (since
> DCACHE doesn't come from the system memory pool), and the next
> allocation often ends up getting the same memory address, and thus
> the same "handle". This showed up as a problem when the unbind/rebind
> code was added.
>
> The user-level agpgart interfaces use a "key" value to uniquely
> identify allocated AGP regions. I don't know why the DRM module
> doesn't do the same, since the key is guaranteed to be unique.
> The patch below changes the handle to be the "key" value.
>
> I'm wary of making changes like this so close to the 4.3 release, but
> I would like to see this problem fixed in 4.3. Does anyone see any
> potential problems with the attached patch?
>
> David
> --
> David Dawes
> Release Engineer/Architect The XFree86 Project
> www.XFree86.org/~dawes
> Index: drivers/i810/i810.h
> ===================================================================
> RCS file: /home/x-cvs/xc/programs/Xserver/hw/xfree86/drivers/i810/i810.h,v
> retrieving revision 1.37
> diff -u -r1.37 i810.h
> --- drivers/i810/i810.h 2002/12/10 01:27:04 1.37
> +++ drivers/i810/i810.h 2003/02/14 20:00:33
> @@ -264,6 +264,9 @@
> extern Bool I810DRIFinishScreenInit(ScreenPtr pScreen);
> extern Bool I810InitDma(ScrnInfoPtr pScrn);
> extern Bool I810CleanupDma(ScrnInfoPtr pScrn);
> +extern Bool I810DRILeave(ScrnInfoPtr pScrn);
> +extern Bool I810DRIEnter(ScrnInfoPtr pScrn);
> +
>
> #define I810PTR(p) ((I810Ptr)((p)->driverPrivate))
> #define I810REGPTR(p) (&(I810PTR(p)->ModeReg))
> Index: drivers/i810/i810_dri.c
> ===================================================================
> RCS file: /home/x-cvs/xc/programs/Xserver/hw/xfree86/drivers/i810/i810_dri.c,v
> retrieving revision 1.33
> diff -u -r1.33 i810_dri.c
> --- drivers/i810/i810_dri.c 2002/12/10 01:27:04 1.33
> +++ drivers/i810/i810_dri.c 2003/02/14 20:00:33
> @@ -1218,3 +1218,84 @@
>
> pI810->AccelInfoRec->NeedToSync = TRUE;
> }
> +
> +Bool
> +I810DRILeave(ScrnInfoPtr pScrn)
> +{
> + I810Ptr pI810 = I810PTR(pScrn);
> +
> + if (pI810->directRenderingEnabled) {
> + if (pI810->dcacheHandle != 0)
> + if (drmAgpUnbind(pI810->drmSubFD, pI810->dcacheHandle) != 0) {
> + xf86DrvMsg(pScrn->scrnIndex, X_ERROR,"%s\n",strerror(errno));
> + return FALSE;
> + }
> + if (pI810->backHandle != 0)
> + if (drmAgpUnbind(pI810->drmSubFD, pI810->backHandle) != 0) {
> + xf86DrvMsg(pScrn->scrnIndex, X_ERROR,"%s\n",strerror(errno));
> + return FALSE;
> + }
> + if (pI810->zHandle != 0)
> + if (drmAgpUnbind(pI810->drmSubFD, pI810->zHandle) != 0) {
> + xf86DrvMsg(pScrn->scrnIndex, X_ERROR,"%s\n",strerror(errno));
> + return FALSE;
> + }
> + if (pI810->sysmemHandle != 0)
> + if (drmAgpUnbind(pI810->drmSubFD, pI810->sysmemHandle) != 0) {
> + xf86DrvMsg(pScrn->scrnIndex, X_ERROR,"%s\n",strerror(errno));
> + return FALSE;
> + }
> + if (pI810->xvmcHandle != 0)
> + if (drmAgpUnbind(pI810->drmSubFD, pI810->xvmcHandle) != 0) {
> + xf86DrvMsg(pScrn->scrnIndex, X_ERROR,"%s\n",strerror(errno));
> + return FALSE;
> + }
> + if (pI810->cursorHandle != 0)
> + if (drmAgpUnbind(pI810->drmSubFD, pI810->cursorHandle) != 0) {
> + xf86DrvMsg(pScrn->scrnIndex, X_ERROR,"%s\n",strerror(errno));
> + return FALSE;
> + }
> + if (pI810->agpAcquired == TRUE)
> + drmAgpRelease(pI810->drmSubFD);
> + pI810->agpAcquired = FALSE;
> + }
> + return TRUE;
> +}
> +
> +Bool
> +I810DRIEnter(ScrnInfoPtr pScrn)
> +{
> + I810Ptr pI810 = I810PTR(pScrn);
> +
> + if (pI810->directRenderingEnabled) {
> +
> + if (pI810->agpAcquired == FALSE)
> + drmAgpAcquire(pI810->drmSubFD);
> + pI810->agpAcquired = TRUE;
> + if (pI810->dcacheHandle != 0)
> + if (drmAgpBind(pI810->drmSubFD, pI810->dcacheHandle,
> + pI810->DepthOffset) != 0)
> + return FALSE;
> + if (pI810->backHandle != 0)
> + if (drmAgpBind(pI810->drmSubFD, pI810->backHandle,
> + pI810->BackOffset) != 0)
> + return FALSE;
> + if (pI810->zHandle != 0)
> + if (drmAgpBind(pI810->drmSubFD, pI810->zHandle,
> + pI810->DepthOffset) != 0)
> + return FALSE;
> + if (pI810->sysmemHandle != 0)
> + if (drmAgpBind(pI810->drmSubFD, pI810->sysmemHandle,
> + 0) != 0)
> + return FALSE;
> + if (pI810->xvmcHandle != 0)
> + if (drmAgpBind(pI810->drmSubFD, pI810->xvmcHandle,
> + pI810->MC.Start) != 0)
> + return FALSE;
> + if (pI810->cursorHandle != 0)
> + if (drmAgpBind(pI810->drmSubFD, pI810->cursorHandle,
> + pI810->CursorStart) != 0)
> + return FALSE;
> + }
> + return TRUE;
> +}
> Index: drivers/i810/i810_driver.c
> ===================================================================
> RCS file: /home/x-cvs/xc/programs/Xserver/hw/xfree86/drivers/i810/i810_driver.c,v
> retrieving revision 1.79
> diff -u -r1.79 i810_driver.c
> --- drivers/i810/i810_driver.c 2003/02/14 17:12:42 1.79
> +++ drivers/i810/i810_driver.c 2003/02/14 20:00:21
> @@ -2218,6 +2218,9 @@
> return FALSE;
>
> #ifdef XF86DRI
> + if (!I810DRIEnter(pScrn))
> + return FALSE;
> +
> if (pI810->directRenderingEnabled) {
> if (I810_DEBUG & DEBUG_VERBOSE_DRI)
> ErrorF("calling dri unlock\n");
> @@ -2260,6 +2263,11 @@
>
> if (!I810UnbindGARTMemory(pScrn))
> return;
> +
> +#ifdef XF86DRI
> + if (!I810DRILeave(pScrn))
> + return;
> +#endif
>
> vgaHWLock(hwp);
> }
> Index: os-support/linux/drm/kernel/drmP.h
> ===================================================================
> RCS file:
>/home/x-cvs/xc/programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drmP.h,v
> retrieving revision 1.27
> diff -u -r1.27 drmP.h
> --- os-support/linux/drm/kernel/drmP.h 2003/01/29 23:00:43 1.27
> +++ os-support/linux/drm/kernel/drmP.h 2003/02/14 19:53:44
> @@ -418,6 +418,7 @@
> struct drm_file *prev;
> struct drm_device *dev;
> int remove_auth_on_close;
> + unsigned long lock_count;
> } drm_file_t;
>
>
> @@ -484,7 +485,7 @@
>
> #if __REALLY_HAVE_AGP
> typedef struct drm_agp_mem {
> - unsigned long handle;
> + int key;
> agp_memory *memory;
> unsigned long bound; /* address */
> int pages;
> Index: os-support/linux/drm/kernel/drm_agpsupport.h
> ===================================================================
> RCS file:
>/home/x-cvs/xc/programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drm_agpsupport.h,v
> retrieving revision 1.7
> diff -u -r1.7 drm_agpsupport.h
> --- os-support/linux/drm/kernel/drm_agpsupport.h 2002/12/16 16:19:28 1.7
> +++ os-support/linux/drm/kernel/drm_agpsupport.h 2003/02/14 20:01:18
> @@ -147,7 +147,7 @@
> return -ENOMEM;
> }
>
> - entry->handle = (unsigned long)memory->memory;
> + entry->key = memory->key;
> entry->memory = memory;
> entry->bound = 0;
> entry->pages = pages;
> @@ -156,7 +156,7 @@
> if (dev->agp->memory) dev->agp->memory->prev = entry;
> dev->agp->memory = entry;
>
> - request.handle = entry->handle;
> + request.handle = entry->key;
> request.physical = memory->physical;
>
> if (copy_to_user((drm_agp_buffer_t *)arg, &request, sizeof(request))) {
> @@ -175,7 +175,7 @@
> drm_agp_mem_t *entry;
>
> for (entry = dev->agp->memory; entry; entry = entry->next) {
> - if (entry->handle == handle) return entry;
> + if (entry->key == handle) return entry;
> }
> return NULL;
> }
> @@ -187,6 +187,7 @@
> drm_device_t *dev = priv->dev;
> drm_agp_binding_t request;
> drm_agp_mem_t *entry;
> + int ret;
>
> if (!dev->agp || !dev->agp->acquired) return -EINVAL;
> if (copy_from_user(&request, (drm_agp_binding_t *)arg, sizeof(request)))
> @@ -194,7 +195,10 @@
> if (!(entry = DRM(agp_lookup_entry)(dev, request.handle)))
> return -EINVAL;
> if (!entry->bound) return -EINVAL;
> - return DRM(unbind_agp)(entry->memory);
> + ret = DRM(unbind_agp)(entry->memory);
> + if (ret == 0)
> + entry->bound = 0;
> + return ret;
> }
>
> int DRM(agp_bind)(struct inode *inode, struct file *filp,
> Index: os-support/linux/drm/kernel/drm_drv.h
> ===================================================================
> RCS file:
>/home/x-cvs/xc/programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drm_drv.h,v
> retrieving revision 1.10
> diff -u -r1.10 drm_drv.h
> --- os-support/linux/drm/kernel/drm_drv.h 2002/11/25 14:05:04 1.10
> +++ os-support/linux/drm/kernel/drm_drv.h 2003/02/14 17:32:02
> @@ -768,7 +768,7 @@
> DRM_DEBUG( "pid = %d, device = 0x%lx, open_count = %d\n",
> current->pid, (long)dev->device, dev->open_count );
>
> - if ( dev->lock.hw_lock &&
> + if ( priv->lock_count && dev->lock.hw_lock &&
> _DRM_LOCK_IS_HELD(dev->lock.hw_lock->lock) &&
> dev->lock.pid == current->pid ) {
> DRM_DEBUG( "Process %d dead, freeing lock for context %d\n",
> @@ -786,7 +786,7 @@
> server. */
> }
> #if __HAVE_RELEASE
> - else if ( dev->lock.hw_lock ) {
> + else if ( priv->lock_count && dev->lock.hw_lock ) {
> /* The lock is required to reclaim buffers */
> DECLARE_WAITQUEUE( entry, current );
> add_wait_queue( &dev->lock.lock_queue, &entry );
> @@ -932,6 +932,8 @@
>
> dev->lck_start = start = get_cycles();
> #endif
> +
> + ++priv->lock_count;
>
> if ( copy_from_user( &lock, (drm_lock_t *)arg, sizeof(lock) ) )
> return -EFAULT;
> Index: os-support/linux/drm/kernel/drm_fops.h
> ===================================================================
> RCS file:
>/home/x-cvs/xc/programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drm_fops.h,v
> retrieving revision 1.7
> diff -u -r1.7 drm_fops.h
> --- os-support/linux/drm/kernel/drm_fops.h 2002/11/25 14:05:04 1.7
> +++ os-support/linux/drm/kernel/drm_fops.h 2003/02/14 17:32:26
> @@ -57,6 +57,7 @@
> priv->dev = dev;
> priv->ioctl_count = 0;
> priv->authenticated = capable(CAP_SYS_ADMIN);
> + priv->lock_count = 0;
>
> down(&dev->struct_sem);
> if (!dev->file_last) {
-------------------------------------------------------
This SF.NET email is sponsored by: FREE SSL Guide from Thawte
are you planning your Web Server Security? Click here to get a FREE
Thawte SSL guide and find the answers to all your SSL security issues.
http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0026en
_______________________________________________
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel
