Subject: Re: drm drivers for NetBSD
To: Yorick Hardy <yhardy@uj.ac.za>
From: Yorick Hardy <yhardy@uj.ac.za>
List: tech-x11
Date: 03/22/2007 09:39:26
This is a multi-part message in MIME format.
--------------000304030403000106050001
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

This time with the patch ;-)

Yorick Hardy wrote:
> Matthias,
>
> Is it possible to integrate the following patch?
>
> Because drm_close is only called once we end
> up with a lot of references to processes that
> may no longer exist.
>
> This is not a fix, but nearly eliminates the problem.
>
> I think I should let Eric Anholt know what we are doing.
> I suspect it could be useful to keep FreeBSD drm
> and NetBSD drm relatively consistent.
>
> Matthias Drochner wrote:
>> macallan@netbsd.org said:
>>> Please commit
>>
>> OK -- there are some minor issues which need reviev and/or cleanup
>> which I wantet to do first, but this will not cause big changes and
>> can be done later.
>> I'll start committing in a minute.
>>
>> best regards
>> Matthias
>>
>>
>
>


-- 
Kind regards,

Yorick Hardy


--------------000304030403000106050001
Content-Type: text/plain;
 name="drm.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="drm.patch"

diff -ur drmP.h.orig drmP.h
--- drmP.h.orig	2007-03-17 23:18:51.000000000 +0200
+++ drmP.h	2007-03-21 18:35:41.000000000 +0200
@@ -906,6 +906,8 @@
 dev_type_mmap(drm_mmap);
 #endif
 
+int drm_close_pid(drm_device_t *dev, drm_file_t *priv, pid_t pid);
+
 /* File operations helpers (drm_fops.c) */
 #ifdef __FreeBSD__
 extern int		drm_open_helper(struct cdev *kdev, int flags, int fmt, 
diff -ur drm_drv.c.orig drm_drv.c
--- drm_drv.c.orig	2007-03-17 23:18:51.000000000 +0200
+++ drm_drv.c	2007-03-21 18:35:41.000000000 +0200
@@ -768,27 +768,10 @@
 	return retcode;
 }
 
-int drm_close(DRM_CDEV kdev, int flags, int fmt, DRM_STRUCTCDEVPROC *p)
+int drm_close_pid(drm_device_t *dev, drm_file_t *priv, pid_t pid)
 {
-	drm_file_t *priv;
-	DRM_DEVICE;
 	int retcode = 0;
-	DRMFILE filp = (void *)(uintptr_t)(DRM_CURRENTPID);
-	
-	DRM_DEBUG( "open_count = %d\n", dev->open_count );
-
-	DRM_LOCK();
-
-#ifdef __FreeBSD__
-	priv = drm_find_file_by_proc(dev, p);
-#elif defined(__NetBSD__)
-	priv = drm_find_file_by_proc(dev, p->l_proc);
-#endif
-	if (!priv) {
-		DRM_UNLOCK();
-		DRM_ERROR("can't find authenticator\n");
-		return EINVAL;
-	}
+	DRMFILE filp = (void *)(uintptr_t)(pid);
 
 	if (dev->driver.preclose != NULL)
 		dev->driver.preclose(dev, filp);
@@ -857,19 +840,6 @@
 	if (dev->driver.use_dma && !dev->driver.reclaim_buffers_locked)
 		drm_reclaim_buffers(dev, filp);
 
-#if defined (__FreeBSD__) && (__FreeBSD_version >= 500000)
-	funsetown(&dev->buf_sigio);
-#elif defined(__FreeBSD__)
-	funsetown(dev->buf_sigio);
-#elif defined(__NetBSD__) || defined(__OpenBSD__)
-	dev->buf_pgid = 0;
-#endif /* __NetBSD__  || __OpenBSD__ */
-
-#ifdef __NetBSD__
-	/* On NetBSD, close will only be called once */
-	DRM_DEBUG("setting priv->refs %d to 1\n", (int)priv->refs);
-	priv->refs = 1;
-#endif
 	if (--priv->refs == 0) {
 		if (dev->driver.postclose != NULL)
 			dev->driver.postclose(dev, priv);
@@ -877,19 +847,52 @@
 		free(priv, M_DRM);
 	}
 
-	/* ========================================================
-	 * End inline drm_release
-	 */
+	return retcode;
+}
+
+int drm_close(DRM_CDEV kdev, int flags, int fmt, DRM_STRUCTCDEVPROC *p)
+{
+	drm_file_t *priv;
+	DRM_DEVICE;
+	int retcode = 0;
+	
+	DRM_DEBUG( "open_count = %d\n", dev->open_count );
+
+	DRM_LOCK();
 
-	atomic_inc( &dev->counts[_DRM_STAT_CLOSES] );
 #ifdef __FreeBSD__
-	device_unbusy(dev->device);
+	priv = drm_find_file_by_proc(dev, p);
+#elif defined(__NetBSD__)
+	priv = drm_find_file_by_proc(dev, p->l_proc);
 #endif
+	if (!priv) {
+		DRM_UNLOCK();
+		DRM_ERROR("can't find authenticator\n");
+		return EINVAL;
+	}
+
 #ifdef __NetBSD__
 	/* On NetBSD, close will only be called once */
+	DRM_DEBUG("setting priv->refs %d to 1\n", (int)priv->refs);
+	priv->refs = 1;
 	DRM_DEBUG("setting open_count %d to 1\n", (int)dev->open_count);
 	dev->open_count = 1;
 #endif
+
+	retcode = drm_close_pid(dev, priv, DRM_CURRENTPID);
+
+#if defined (__FreeBSD__) && (__FreeBSD_version >= 500000)
+	funsetown(&dev->buf_sigio);
+#elif defined(__FreeBSD__)
+	funsetown(dev->buf_sigio);
+#elif defined(__NetBSD__) || defined(__OpenBSD__)
+	dev->buf_pgid = 0;
+#endif /* __NetBSD__  || __OpenBSD__ */
+
+	atomic_inc( &dev->counts[_DRM_STAT_CLOSES] );
+#ifdef __FreeBSD__
+	device_unbusy(dev->device);
+#endif
 	if (--dev->open_count == 0) {
 		retcode = drm_lastclose(dev);
 	}
diff -ur drm_fops.c.orig drm_fops.c
--- drm_fops.c.orig	2007-03-17 23:18:51.000000000 +0200
+++ drm_fops.c	2007-03-21 18:35:41.000000000 +0200
@@ -41,6 +41,7 @@
 
 drm_file_t *drm_find_file_by_proc(drm_device_t *dev, DRM_STRUCTPROC *p)
 {
+	int restart = 1;
 #if __FreeBSD_version >= 500021
 	uid_t uid = p->td_ucred->cr_svuid;
 	pid_t pid = p->td_proc->p_pid;
@@ -55,9 +56,25 @@
 
 	DRM_SPINLOCK_ASSERT(&dev->dev_lock);
 
-	TAILQ_FOREACH(priv, &dev->files, link)
-		if (priv->pid == pid && priv->uid == uid)
-			return priv;
+	while(restart) {
+		restart = 0;
+		TAILQ_FOREACH(priv, &dev->files, link) {
+#ifdef __NetBSD__
+	/* if the process disappeared, free the resources 
+	 * NetBSD only calls drm_close once, so this frees
+	 * resources earlier.
+	 */
+			if (pfind(priv->pid) == NULL) {
+				drm_close_pid(dev, priv, priv->pid);
+				restart = 1;
+				break;
+			}
+			else
+#endif
+			if (priv->pid == pid && priv->uid == uid)
+				return priv;
+		}
+	}
 	return NULL;
 }
 

--------------000304030403000106050001--