| Message ID | 1406129207-1302-5-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 23, 2014 at 05:26:39PM +0200, David Herrmann wrote: > The ctxbitmap code is only used by legacy drivers so lets try to keep it > as separated as possible. Furthermore, the locking is non-obvious and > kinda weird with ctxlist_mutex *and* struct_mutex. Keeping all ctxbitmap > access in one file is much easier to review and makes drm_release() more > readable. > > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> I've started to sprinkle _legacy_ over all the functions only used for non-kms drivers, so that the dragon dungeons are clearly marked off. With that this is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>. -Daniel > --- > drivers/gpu/drm/drm_context.c | 30 ++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_fops.c | 20 +------------------- > include/drm/drmP.h | 1 + > 3 files changed, 32 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/drm_context.c b/drivers/gpu/drm/drm_context.c > index a4b017b..c045505 100644 > --- a/drivers/gpu/drm/drm_context.c > +++ b/drivers/gpu/drm/drm_context.c > @@ -111,6 +111,36 @@ void drm_ctxbitmap_cleanup(struct drm_device * dev) > mutex_unlock(&dev->struct_mutex); > } > > +/** > + * drm_ctxbitmap_flush() - Flush all contexts owned by a file > + * @dev: DRM device to operate on > + * @file: Open file to flush contexts for > + * > + * This iterates over all contexts on @dev and drops them if they're owned by > + * @file. Note that after this call returns, new contexts might be added if > + * the file is still alive. > + */ > +void drm_ctxbitmap_flush(struct drm_device *dev, struct drm_file *file) > +{ > + struct drm_ctx_list *pos, *tmp; > + > + mutex_lock(&dev->ctxlist_mutex); > + > + list_for_each_entry_safe(pos, tmp, &dev->ctxlist, head) { > + if (pos->tag == file && > + pos->handle != DRM_KERNEL_CONTEXT) { > + if (dev->driver->context_dtor) > + dev->driver->context_dtor(dev, pos->handle); > + > + drm_ctxbitmap_free(dev, pos->handle); > + list_del(&pos->head); > + kfree(pos); > + } > + } > + > + mutex_unlock(&dev->ctxlist_mutex); > +} > + > /*@}*/ > > /******************************************************************/ > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c > index 8e73519..fb81d1c 100644 > --- a/drivers/gpu/drm/drm_fops.c > +++ b/drivers/gpu/drm/drm_fops.c > @@ -452,25 +452,7 @@ int drm_release(struct inode *inode, struct file *filp) > if (dev->driver->driver_features & DRIVER_GEM) > drm_gem_release(dev, file_priv); > > - mutex_lock(&dev->ctxlist_mutex); > - if (!list_empty(&dev->ctxlist)) { > - struct drm_ctx_list *pos, *n; > - > - list_for_each_entry_safe(pos, n, &dev->ctxlist, head) { > - if (pos->tag == file_priv && > - pos->handle != DRM_KERNEL_CONTEXT) { > - if (dev->driver->context_dtor) > - dev->driver->context_dtor(dev, > - pos->handle); > - > - drm_ctxbitmap_free(dev, pos->handle); > - > - list_del(&pos->head); > - kfree(pos); > - } > - } > - } > - mutex_unlock(&dev->ctxlist_mutex); > + drm_ctxbitmap_flush(dev, file_priv); > > mutex_lock(&dev->master_mutex); > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index d1730c5..d91e09f 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -1242,6 +1242,7 @@ extern int drm_rmctx(struct drm_device *dev, void *data, > extern int drm_ctxbitmap_init(struct drm_device *dev); > extern void drm_ctxbitmap_cleanup(struct drm_device *dev); > extern void drm_ctxbitmap_free(struct drm_device *dev, int ctx_handle); > +extern void drm_ctxbitmap_flush(struct drm_device *dev, struct drm_file *file); > > extern int drm_setsareactx(struct drm_device *dev, void *data, > struct drm_file *file_priv); > -- > 2.0.2 >
Hi On Wed, Jul 23, 2014 at 9:26 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Jul 23, 2014 at 05:26:39PM +0200, David Herrmann wrote: >> The ctxbitmap code is only used by legacy drivers so lets try to keep it >> as separated as possible. Furthermore, the locking is non-obvious and >> kinda weird with ctxlist_mutex *and* struct_mutex. Keeping all ctxbitmap >> access in one file is much easier to review and makes drm_release() more >> readable. >> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > > I've started to sprinkle _legacy_ over all the functions only used for > non-kms drivers, so that the dragon dungeons are clearly marked off. With > that this is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>. Yeah, that sounds good. But I'd prefer if we keep this patch as-is and I will send a follow-up which renames all "drm_ctxbitmap_*" functions to "drm_legacy_ctxbitmap_*". All those are internal to DRM-core so I can rename them all together and add drivers/gpu/drm/drm-legacy.h. Comments? David
On Thu, Jul 24, 2014 at 11:34:45AM +0200, David Herrmann wrote: > Hi > > On Wed, Jul 23, 2014 at 9:26 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Jul 23, 2014 at 05:26:39PM +0200, David Herrmann wrote: > >> The ctxbitmap code is only used by legacy drivers so lets try to keep it > >> as separated as possible. Furthermore, the locking is non-obvious and > >> kinda weird with ctxlist_mutex *and* struct_mutex. Keeping all ctxbitmap > >> access in one file is much easier to review and makes drm_release() more > >> readable. > >> > >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > > > > I've started to sprinkle _legacy_ over all the functions only used for > > non-kms drivers, so that the dragon dungeons are clearly marked off. With > > that this is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>. > > Yeah, that sounds good. But I'd prefer if we keep this patch as-is and > I will send a follow-up which renames all "drm_ctxbitmap_*" functions > to "drm_legacy_ctxbitmap_*". All those are internal to DRM-core so I > can rename them all together and add drivers/gpu/drm/drm-legacy.h. Sounds like a good plan, so r-b on the patch as-is. -Daniel
diff --git a/drivers/gpu/drm/drm_context.c b/drivers/gpu/drm/drm_context.c index a4b017b..c045505 100644 --- a/drivers/gpu/drm/drm_context.c +++ b/drivers/gpu/drm/drm_context.c @@ -111,6 +111,36 @@ void drm_ctxbitmap_cleanup(struct drm_device * dev) mutex_unlock(&dev->struct_mutex); } +/** + * drm_ctxbitmap_flush() - Flush all contexts owned by a file + * @dev: DRM device to operate on + * @file: Open file to flush contexts for + * + * This iterates over all contexts on @dev and drops them if they're owned by + * @file. Note that after this call returns, new contexts might be added if + * the file is still alive. + */ +void drm_ctxbitmap_flush(struct drm_device *dev, struct drm_file *file) +{ + struct drm_ctx_list *pos, *tmp; + + mutex_lock(&dev->ctxlist_mutex); + + list_for_each_entry_safe(pos, tmp, &dev->ctxlist, head) { + if (pos->tag == file && + pos->handle != DRM_KERNEL_CONTEXT) { + if (dev->driver->context_dtor) + dev->driver->context_dtor(dev, pos->handle); + + drm_ctxbitmap_free(dev, pos->handle); + list_del(&pos->head); + kfree(pos); + } + } + + mutex_unlock(&dev->ctxlist_mutex); +} + /*@}*/ /******************************************************************/ diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 8e73519..fb81d1c 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -452,25 +452,7 @@ int drm_release(struct inode *inode, struct file *filp) if (dev->driver->driver_features & DRIVER_GEM) drm_gem_release(dev, file_priv); - mutex_lock(&dev->ctxlist_mutex); - if (!list_empty(&dev->ctxlist)) { - struct drm_ctx_list *pos, *n; - - list_for_each_entry_safe(pos, n, &dev->ctxlist, head) { - if (pos->tag == file_priv && - pos->handle != DRM_KERNEL_CONTEXT) { - if (dev->driver->context_dtor) - dev->driver->context_dtor(dev, - pos->handle); - - drm_ctxbitmap_free(dev, pos->handle); - - list_del(&pos->head); - kfree(pos); - } - } - } - mutex_unlock(&dev->ctxlist_mutex); + drm_ctxbitmap_flush(dev, file_priv); mutex_lock(&dev->master_mutex); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d1730c5..d91e09f 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1242,6 +1242,7 @@ extern int drm_rmctx(struct drm_device *dev, void *data, extern int drm_ctxbitmap_init(struct drm_device *dev); extern void drm_ctxbitmap_cleanup(struct drm_device *dev); extern void drm_ctxbitmap_free(struct drm_device *dev, int ctx_handle); +extern void drm_ctxbitmap_flush(struct drm_device *dev, struct drm_file *file); extern int drm_setsareactx(struct drm_device *dev, void *data, struct drm_file *file_priv);
The ctxbitmap code is only used by legacy drivers so lets try to keep it as separated as possible. Furthermore, the locking is non-obvious and kinda weird with ctxlist_mutex *and* struct_mutex. Keeping all ctxbitmap access in one file is much easier to review and makes drm_release() more readable. Signed-off-by: David Herrmann <dh.herrmann@gmail.com> --- drivers/gpu/drm/drm_context.c | 30 ++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_fops.c | 20 +------------------- include/drm/drmP.h | 1 + 3 files changed, 32 insertions(+), 19 deletions(-)