This flag is only used for drm/udl.
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org --- drivers/gpu/drm/msm/msm_fbdev.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c index d95af6e..e119c29 100644 --- a/drivers/gpu/drm/msm/msm_fbdev.c +++ b/drivers/gpu/drm/msm/msm_fbdev.c @@ -65,9 +65,6 @@ static int msm_fbdev_mmap(struct fb_info *info, struct vm_area_struct *vma) struct drm_device *dev = helper->dev; int ret = 0;
- if (drm_device_is_unplugged(dev)) - return -ENODEV; - ret = drm_gem_mmap_obj(drm_obj, drm_obj->size, vma); if (ret) { pr_err("%s:drm_gem_mmap_obj fail\n", __func__);
Remove the general drm_device_is_unplugged() checks, and move the unplugged flag handling logic into drm/udl. In general we want to keep driver-specific logic out of common drm code.
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org --- drivers/gpu/drm/drm_drv.c | 6 ------ drivers/gpu/drm/drm_fops.c | 2 -- drivers/gpu/drm/drm_gem.c | 3 --- drivers/gpu/drm/drm_ioctl.c | 3 --- drivers/gpu/drm/drm_vm.c | 3 --- drivers/gpu/drm/udl/udl_connector.c | 2 +- drivers/gpu/drm/udl/udl_drv.c | 1 + drivers/gpu/drm/udl/udl_drv.h | 3 +++ drivers/gpu/drm/udl/udl_fb.c | 2 +- drivers/gpu/drm/udl/udl_main.c | 15 +++++++++++++++ include/drm/drmP.h | 14 -------------- 11 files changed, 21 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 167c8d3..f93ee12 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)
if (!minor) { return ERR_PTR(-ENODEV); - } else if (drm_device_is_unplugged(minor->dev)) { - drm_dev_unref(minor->dev); - return ERR_PTR(-ENODEV); }
return minor; @@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev) drm_minor_unregister(dev, DRM_MINOR_CONTROL);
mutex_lock(&drm_global_mutex); - - drm_device_set_unplugged(dev); - if (dev->open_count == 0) { drm_put_dev(dev); } diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 1ea8790..b4332d4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file *filp)
if (!--dev->open_count) { retcode = drm_lastclose(dev); - if (drm_device_is_unplugged(dev)) - drm_put_dev(dev); } mutex_unlock(&drm_global_mutex);
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2e8c77e..c622e32 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_vma_offset_node *node; int ret;
- if (drm_device_is_unplugged(dev)) - return -ENODEV; - drm_vma_offset_lock_lookup(dev->vma_offset_manager); node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager, vma->vm_pgoff, diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 8ce2a0c..f959074 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
dev = file_priv->minor->dev;
- if (drm_device_is_unplugged(dev)) - return -ENODEV; - is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
if (is_driver_ioctl) { diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index f90bd5f..3a68be4 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_device *dev = priv->minor->dev; int ret;
- if (drm_device_is_unplugged(dev)) - return -ENODEV; - mutex_lock(&dev->struct_mutex); ret = drm_mmap_locked(filp, vma); mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c index 4709b54..b168f2c 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -96,7 +96,7 @@ static int udl_mode_valid(struct drm_connector *connector, static enum drm_connector_status udl_detect(struct drm_connector *connector, bool force) { - if (drm_device_is_unplugged(connector->dev)) + if (udl_device_is_unplugged(connector->dev)) return connector_status_disconnected; return connector_status_connected; } diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index d5728ec..3cbafe7 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -97,6 +97,7 @@ static void udl_usb_disconnect(struct usb_interface *interface) drm_connector_unplug_all(dev); udl_fbdev_unplug(dev); udl_drop_usb(dev); + udl_device_set_unplugged(dev); drm_unplug_dev(dev); }
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index 4a064ef..61e117a 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -65,6 +65,7 @@ struct udl_device { atomic_t bytes_identical; /* saved effort with backbuffer comparison */ atomic_t bytes_sent; /* to usb, after compression including overhead */ atomic_t cpu_kcycles_used; /* transpired during pixel processing */ + atomic_t unplugged; /* device has been unplugged or gone away */ };
struct udl_gem_object { @@ -140,6 +141,8 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y, int width, int height);
int udl_drop_usb(struct drm_device *dev); +void udl_device_set_unplugged(struct drm_device *dev); +int udl_device_is_unplugged(struct drm_device *dev);
#define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */ #define CMD_WRITE_RL8 "\xAF\x61" /**< 8 bit run length command. */ diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 200419d..df1d53e 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -325,7 +325,7 @@ static int udl_fb_open(struct fb_info *info, int user) struct udl_device *udl = dev->dev_private;
/* If the USB device is gone, we don't accept new opens */ - if (drm_device_is_unplugged(udl->ddev)) + if (udl_device_is_unplugged(udl->ddev)) return -ENODEV;
ufbdev->fb_count++; diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index 33dbfb2..f6028a4 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -350,3 +350,18 @@ int udl_driver_unload(struct drm_device *dev) kfree(udl); return 0; } + +void udl_device_set_unplugged(struct drm_device *dev) +{ + struct udl_device *udl = dev->dev_private; + smp_wmb(); + atomic_set(&udl->unplugged, 1); +} + +int udl_device_is_unplugged(struct drm_device *dev) +{ + struct udl_device *udl = dev->dev_private; + int ret = atomic_read(&udl->unplugged); + smp_rmb(); + return ret; +} diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d7162cf..40c6099 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,7 +748,6 @@ struct drm_device { struct drm_minor *control; /**< Control node */ struct drm_minor *primary; /**< Primary node */ struct drm_minor *render; /**< Render node */ - atomic_t unplugged; /**< Flag whether dev is dead */ struct inode *anon_inode; /**< inode for private address-space */ char *unique; /**< unique name of the device */ /*@} */ @@ -879,19 +878,6 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev, return ((dev->driver->driver_features & feature) ? 1 : 0); }
-static inline void drm_device_set_unplugged(struct drm_device *dev) -{ - smp_wmb(); - atomic_set(&dev->unplugged, 1); -} - -static inline int drm_device_is_unplugged(struct drm_device *dev) -{ - int ret = atomic_read(&dev->unplugged); - smp_rmb(); - return ret; -} - static inline bool drm_is_render_client(const struct drm_file *file_priv) { return file_priv->minor->type == DRM_MINOR_RENDER;
Hi
On Fri, Feb 5, 2016 at 10:57 PM, Haixia Shi hshi@chromium.org wrote:
Remove the general drm_device_is_unplugged() checks, and move the unplugged flag handling logic into drm/udl. In general we want to keep driver-specific logic out of common drm code.
I like the idea of moving this hack into udl. However, I don't think this patch is sufficient, see below for comments.
Btw., hotplug might happen on other buses as well. It just happens that no-one tried pci/platform unplugging so far.. We used to have some patches flying around to make it work properly (with enter/leave markers), but no-one picked those up. But I like the idea of moving this unplugged marker into UDL, to make clear if someone else needs it, they better do it properly.
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org
drivers/gpu/drm/drm_drv.c | 6 ------ drivers/gpu/drm/drm_fops.c | 2 -- drivers/gpu/drm/drm_gem.c | 3 --- drivers/gpu/drm/drm_ioctl.c | 3 --- drivers/gpu/drm/drm_vm.c | 3 --- drivers/gpu/drm/udl/udl_connector.c | 2 +- drivers/gpu/drm/udl/udl_drv.c | 1 + drivers/gpu/drm/udl/udl_drv.h | 3 +++ drivers/gpu/drm/udl/udl_fb.c | 2 +- drivers/gpu/drm/udl/udl_main.c | 15 +++++++++++++++ include/drm/drmP.h | 14 -------------- 11 files changed, 21 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 167c8d3..f93ee12 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)
if (!minor) { return ERR_PTR(-ENODEV);
} else if (drm_device_is_unplugged(minor->dev)) {drm_dev_unref(minor->dev);return ERR_PTR(-ENODEV); }
You cannot just drop this without a replacement. You should add a drm_driver.open callback to UDL which checks for this. drm_minor_acquire() is only ever called from drm_stub_open(), and as such only from drm_open().
Alternatively, you can provide fops.open from UDL and wrap drm_open().
return minor;@@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev) drm_minor_unregister(dev, DRM_MINOR_CONTROL);
mutex_lock(&drm_global_mutex);
drm_device_set_unplugged(dev);
You replace this by moving it into the caller of drm_unplug_dev(). Sounds good to me. There has never been a real reason to put it underneath the global mutex, anyway.
if (dev->open_count == 0) { drm_put_dev(dev); }diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 1ea8790..b4332d4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file *filp)
if (!--dev->open_count) { retcode = drm_lastclose(dev);
if (drm_device_is_unplugged(dev))drm_put_dev(dev);
Again, you cannot drop this without replacement! In this case, you really should wrap fops.release() from UDL and call into drm_release() before copying this unplug-logic.
} mutex_unlock(&drm_global_mutex);diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2e8c77e..c622e32 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_vma_offset_node *node; int ret;
if (drm_device_is_unplugged(dev))return -ENODEV;
Rather than just dropping it, you better move it into udl_drm_gem_mmap().
drm_vma_offset_lock_lookup(dev->vma_offset_manager); node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager, vma->vm_pgoff,diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 8ce2a0c..f959074 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
dev = file_priv->minor->dev;
if (drm_device_is_unplugged(dev))return -ENODEV;
You *must* provide a wrapper for fops.ioctl() in udl which does this check.
is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END; if (is_driver_ioctl) {diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index f90bd5f..3a68be4 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_device *dev = priv->minor->dev; int ret;
if (drm_device_is_unplugged(dev))return -ENODEV;
This looks fine to me.
mutex_lock(&dev->struct_mutex); ret = drm_mmap_locked(filp, vma); mutex_unlock(&dev->struct_mutex);diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c index 4709b54..b168f2c 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -96,7 +96,7 @@ static int udl_mode_valid(struct drm_connector *connector, static enum drm_connector_status udl_detect(struct drm_connector *connector, bool force) {
if (drm_device_is_unplugged(connector->dev))
if (udl_device_is_unplugged(connector->dev)) return connector_status_disconnected; return connector_status_connected;} diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index d5728ec..3cbafe7 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -97,6 +97,7 @@ static void udl_usb_disconnect(struct usb_interface *interface) drm_connector_unplug_all(dev); udl_fbdev_unplug(dev); udl_drop_usb(dev);
udl_device_set_unplugged(dev); drm_unplug_dev(dev);
This looks good.
}
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index 4a064ef..61e117a 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -65,6 +65,7 @@ struct udl_device { atomic_t bytes_identical; /* saved effort with backbuffer comparison */ atomic_t bytes_sent; /* to usb, after compression including overhead */ atomic_t cpu_kcycles_used; /* transpired during pixel processing */
atomic_t unplugged; /* device has been unplugged or gone away */};
struct udl_gem_object { @@ -140,6 +141,8 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y, int width, int height);
int udl_drop_usb(struct drm_device *dev); +void udl_device_set_unplugged(struct drm_device *dev); +int udl_device_is_unplugged(struct drm_device *dev);
#define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */ #define CMD_WRITE_RL8 "\xAF\x61" /**< 8 bit run length command. */ diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 200419d..df1d53e 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -325,7 +325,7 @@ static int udl_fb_open(struct fb_info *info, int user) struct udl_device *udl = dev->dev_private;
/* If the USB device is gone, we don't accept new opens */
if (drm_device_is_unplugged(udl->ddev))
if (udl_device_is_unplugged(udl->ddev)) return -ENODEV; ufbdev->fb_count++;diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index 33dbfb2..f6028a4 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -350,3 +350,18 @@ int udl_driver_unload(struct drm_device *dev) kfree(udl); return 0; }
+void udl_device_set_unplugged(struct drm_device *dev) +{
struct udl_device *udl = dev->dev_private;smp_wmb();atomic_set(&udl->unplugged, 1);+}
+int udl_device_is_unplugged(struct drm_device *dev) +{
struct udl_device *udl = dev->dev_private;int ret = atomic_read(&udl->unplugged);smp_rmb();return ret;+}
This looks fine to me.
Please make sure to wrap most of the file_operations callbacks in UDL and protect them with the "unplugged" check. Yes, we all know that this is racy, and always has been. However, it is still better than nothing. So please don't just drop them without any explanation!
Thanks David
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d7162cf..40c6099 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,7 +748,6 @@ struct drm_device { struct drm_minor *control; /**< Control node */ struct drm_minor *primary; /**< Primary node */ struct drm_minor *render; /**< Render node */
atomic_t unplugged; /**< Flag whether dev is dead */ struct inode *anon_inode; /**< inode for private address-space */ char *unique; /**< unique name of the device */ /*@} */@@ -879,19 +878,6 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev, return ((dev->driver->driver_features & feature) ? 1 : 0); }
-static inline void drm_device_set_unplugged(struct drm_device *dev) -{
smp_wmb();atomic_set(&dev->unplugged, 1);-}
-static inline int drm_device_is_unplugged(struct drm_device *dev) -{
int ret = atomic_read(&dev->unplugged);smp_rmb();return ret;-}
static inline bool drm_is_render_client(const struct drm_file *file_priv) { return file_priv->minor->type == DRM_MINOR_RENDER; -- 2.7.0.rc3.207.g0ac5344
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Regarding the following comment:
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 1ea8790..b4332d4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file
*filp)
if (!--dev->open_count) { retcode = drm_lastclose(dev);
if (drm_device_is_unplugged(dev))drm_put_dev(dev);
Again, you cannot drop this without replacement! In this case, you really should wrap fops.release() from UDL and call into drm_release() before copying this unplug-logic.
Does this really work? drm_release() will release minor at the end, regardless of dev->open_count.
If minor is released, can I still safely get dev and call drm_put_dev(dev) afterwards?
On Tue, Feb 9, 2016 at 4:44 AM, David Herrmann dh.herrmann@gmail.com wrote:
Hi
On Fri, Feb 5, 2016 at 10:57 PM, Haixia Shi hshi@chromium.org wrote:
Remove the general drm_device_is_unplugged() checks, and move the
unplugged
flag handling logic into drm/udl. In general we want to keep
driver-specific
logic out of common drm code.
I like the idea of moving this hack into udl. However, I don't think this patch is sufficient, see below for comments.
Btw., hotplug might happen on other buses as well. It just happens that no-one tried pci/platform unplugging so far.. We used to have some patches flying around to make it work properly (with enter/leave markers), but no-one picked those up. But I like the idea of moving this unplugged marker into UDL, to make clear if someone else needs it, they better do it properly.
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org
drivers/gpu/drm/drm_drv.c | 6 ------ drivers/gpu/drm/drm_fops.c | 2 -- drivers/gpu/drm/drm_gem.c | 3 --- drivers/gpu/drm/drm_ioctl.c | 3 --- drivers/gpu/drm/drm_vm.c | 3 --- drivers/gpu/drm/udl/udl_connector.c | 2 +- drivers/gpu/drm/udl/udl_drv.c | 1 + drivers/gpu/drm/udl/udl_drv.h | 3 +++ drivers/gpu/drm/udl/udl_fb.c | 2 +- drivers/gpu/drm/udl/udl_main.c | 15 +++++++++++++++ include/drm/drmP.h | 14 -------------- 11 files changed, 21 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 167c8d3..f93ee12 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int
minor_id)
if (!minor) { return ERR_PTR(-ENODEV);
} else if (drm_device_is_unplugged(minor->dev)) {drm_dev_unref(minor->dev);return ERR_PTR(-ENODEV); }You cannot just drop this without a replacement. You should add a drm_driver.open callback to UDL which checks for this. drm_minor_acquire() is only ever called from drm_stub_open(), and as such only from drm_open().
Alternatively, you can provide fops.open from UDL and wrap drm_open().
return minor;@@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev) drm_minor_unregister(dev, DRM_MINOR_CONTROL);
mutex_lock(&drm_global_mutex);
drm_device_set_unplugged(dev);You replace this by moving it into the caller of drm_unplug_dev(). Sounds good to me. There has never been a real reason to put it underneath the global mutex, anyway.
if (dev->open_count == 0) { drm_put_dev(dev); }diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 1ea8790..b4332d4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file
*filp)
if (!--dev->open_count) { retcode = drm_lastclose(dev);
if (drm_device_is_unplugged(dev))drm_put_dev(dev);Again, you cannot drop this without replacement! In this case, you really should wrap fops.release() from UDL and call into drm_release() before copying this unplug-logic.
} mutex_unlock(&drm_global_mutex);diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2e8c77e..c622e32 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct
vm_area_struct *vma)
struct drm_vma_offset_node *node; int ret;
if (drm_device_is_unplugged(dev))return -ENODEV;Rather than just dropping it, you better move it into udl_drm_gem_mmap().
drm_vma_offset_lock_lookup(dev->vma_offset_manager); node =drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
vma->vm_pgoff,diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 8ce2a0c..f959074 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
dev = file_priv->minor->dev;
if (drm_device_is_unplugged(dev))return -ENODEV;You *must* provide a wrapper for fops.ioctl() in udl which does this check.
is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END; if (is_driver_ioctl) {diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index f90bd5f..3a68be4 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct
vm_area_struct *vma)
struct drm_device *dev = priv->minor->dev; int ret;
if (drm_device_is_unplugged(dev))return -ENODEV;This looks fine to me.
mutex_lock(&dev->struct_mutex); ret = drm_mmap_locked(filp, vma); mutex_unlock(&dev->struct_mutex);diff --git a/drivers/gpu/drm/udl/udl_connector.c
b/drivers/gpu/drm/udl/udl_connector.c
index 4709b54..b168f2c 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -96,7 +96,7 @@ static int udl_mode_valid(struct drm_connector
*connector,
static enum drm_connector_status udl_detect(struct drm_connector *connector, bool force) {
if (drm_device_is_unplugged(connector->dev))
if (udl_device_is_unplugged(connector->dev)) return connector_status_disconnected; return connector_status_connected;} diff --git a/drivers/gpu/drm/udl/udl_drv.c
b/drivers/gpu/drm/udl/udl_drv.c
index d5728ec..3cbafe7 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -97,6 +97,7 @@ static void udl_usb_disconnect(struct usb_interface
*interface)
drm_connector_unplug_all(dev); udl_fbdev_unplug(dev); udl_drop_usb(dev);
udl_device_set_unplugged(dev); drm_unplug_dev(dev);This looks good.
}
diff --git a/drivers/gpu/drm/udl/udl_drv.h
b/drivers/gpu/drm/udl/udl_drv.h
index 4a064ef..61e117a 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -65,6 +65,7 @@ struct udl_device { atomic_t bytes_identical; /* saved effort with backbuffer
comparison */
atomic_t bytes_sent; /* to usb, after compression includingoverhead */
atomic_t cpu_kcycles_used; /* transpired during pixel processing*/
atomic_t unplugged; /* device has been unplugged or gone away */};
struct udl_gem_object { @@ -140,6 +141,8 @@ int udl_handle_damage(struct udl_framebuffer *fb,
int x, int y,
int width, int height);int udl_drop_usb(struct drm_device *dev); +void udl_device_set_unplugged(struct drm_device *dev); +int udl_device_is_unplugged(struct drm_device *dev);
#define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */ #define CMD_WRITE_RL8 "\xAF\x61" /**< 8 bit run length command. */ diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 200419d..df1d53e 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -325,7 +325,7 @@ static int udl_fb_open(struct fb_info *info, int
user)
struct udl_device *udl = dev->dev_private; /* If the USB device is gone, we don't accept new opens */
if (drm_device_is_unplugged(udl->ddev))
if (udl_device_is_unplugged(udl->ddev)) return -ENODEV; ufbdev->fb_count++;diff --git a/drivers/gpu/drm/udl/udl_main.c
b/drivers/gpu/drm/udl/udl_main.c
index 33dbfb2..f6028a4 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -350,3 +350,18 @@ int udl_driver_unload(struct drm_device *dev) kfree(udl); return 0; }
+void udl_device_set_unplugged(struct drm_device *dev) +{
struct udl_device *udl = dev->dev_private;smp_wmb();atomic_set(&udl->unplugged, 1);+}
+int udl_device_is_unplugged(struct drm_device *dev) +{
struct udl_device *udl = dev->dev_private;int ret = atomic_read(&udl->unplugged);smp_rmb();return ret;+}
This looks fine to me.
Please make sure to wrap most of the file_operations callbacks in UDL and protect them with the "unplugged" check. Yes, we all know that this is racy, and always has been. However, it is still better than nothing. So please don't just drop them without any explanation!
Thanks David
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d7162cf..40c6099 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,7 +748,6 @@ struct drm_device { struct drm_minor *control; /**< Control node */ struct drm_minor *primary; /**< Primary node */ struct drm_minor *render; /**< Render node */
atomic_t unplugged; /**< Flag whether dev isdead */
struct inode *anon_inode; /**< inode for privateaddress-space */
char *unique; /**< unique name of thedevice */
/*@} */@@ -879,19 +878,6 @@ static __inline__ int drm_core_check_feature(struct
drm_device *dev,
return ((dev->driver->driver_features & feature) ? 1 : 0);}
-static inline void drm_device_set_unplugged(struct drm_device *dev) -{
smp_wmb();atomic_set(&dev->unplugged, 1);-}
-static inline int drm_device_is_unplugged(struct drm_device *dev) -{
int ret = atomic_read(&dev->unplugged);smp_rmb();return ret;-}
static inline bool drm_is_render_client(const struct drm_file
*file_priv)
{ return file_priv->minor->type == DRM_MINOR_RENDER; -- 2.7.0.rc3.207.g0ac5344
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi
On Tue, Feb 9, 2016 at 9:45 PM, Haixia Shi hshi@chromium.org wrote:
Regarding the following comment:
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 1ea8790..b4332d4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file *filp)
if (!--dev->open_count) { retcode = drm_lastclose(dev);
if (drm_device_is_unplugged(dev))drm_put_dev(dev);Again, you cannot drop this without replacement! In this case, you really should wrap fops.release() from UDL and call into drm_release() before copying this unplug-logic.
Does this really work? drm_release() will release minor at the end, regardless of dev->open_count.
"open_count" is about "drm_device". "drm_minor" is completely unrelated to this.
If minor is released, can I still safely get dev and call drm_put_dev(dev) afterwards?
Of course!
David
When USB cable is disconnected, we mark udl device as unplugged so that udl_detect reports connector status as disconnected, but still keep the drm device alive until user-space closes it.
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org --- drivers/gpu/drm/drm_drv.c | 6 ------ drivers/gpu/drm/drm_fops.c | 2 -- drivers/gpu/drm/drm_gem.c | 3 --- drivers/gpu/drm/drm_ioctl.c | 3 --- drivers/gpu/drm/drm_vm.c | 3 --- drivers/gpu/drm/udl/udl_connector.c | 2 +- drivers/gpu/drm/udl/udl_drv.c | 1 + drivers/gpu/drm/udl/udl_drv.h | 3 +++ drivers/gpu/drm/udl/udl_fb.c | 2 +- drivers/gpu/drm/udl/udl_main.c | 15 +++++++++++++++ include/drm/drmP.h | 14 -------------- 11 files changed, 21 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 167c8d3..f93ee12 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)
if (!minor) { return ERR_PTR(-ENODEV); - } else if (drm_device_is_unplugged(minor->dev)) { - drm_dev_unref(minor->dev); - return ERR_PTR(-ENODEV); }
return minor; @@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev) drm_minor_unregister(dev, DRM_MINOR_CONTROL);
mutex_lock(&drm_global_mutex); - - drm_device_set_unplugged(dev); - if (dev->open_count == 0) { drm_put_dev(dev); } diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 1ea8790..b4332d4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file *filp)
if (!--dev->open_count) { retcode = drm_lastclose(dev); - if (drm_device_is_unplugged(dev)) - drm_put_dev(dev); } mutex_unlock(&drm_global_mutex);
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2e8c77e..c622e32 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_vma_offset_node *node; int ret;
- if (drm_device_is_unplugged(dev)) - return -ENODEV; - drm_vma_offset_lock_lookup(dev->vma_offset_manager); node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager, vma->vm_pgoff, diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 8ce2a0c..f959074 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
dev = file_priv->minor->dev;
- if (drm_device_is_unplugged(dev)) - return -ENODEV; - is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
if (is_driver_ioctl) { diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index f90bd5f..3a68be4 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_device *dev = priv->minor->dev; int ret;
- if (drm_device_is_unplugged(dev)) - return -ENODEV; - mutex_lock(&dev->struct_mutex); ret = drm_mmap_locked(filp, vma); mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c index 4709b54..b168f2c 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -96,7 +96,7 @@ static int udl_mode_valid(struct drm_connector *connector, static enum drm_connector_status udl_detect(struct drm_connector *connector, bool force) { - if (drm_device_is_unplugged(connector->dev)) + if (udl_device_is_unplugged(connector->dev)) return connector_status_disconnected; return connector_status_connected; } diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index d5728ec..3cbafe7 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -97,6 +97,7 @@ static void udl_usb_disconnect(struct usb_interface *interface) drm_connector_unplug_all(dev); udl_fbdev_unplug(dev); udl_drop_usb(dev); + udl_device_set_unplugged(dev); drm_unplug_dev(dev); }
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index 4a064ef..61e117a 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -65,6 +65,7 @@ struct udl_device { atomic_t bytes_identical; /* saved effort with backbuffer comparison */ atomic_t bytes_sent; /* to usb, after compression including overhead */ atomic_t cpu_kcycles_used; /* transpired during pixel processing */ + atomic_t unplugged; /* device has been unplugged or gone away */ };
struct udl_gem_object { @@ -140,6 +141,8 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y, int width, int height);
int udl_drop_usb(struct drm_device *dev); +void udl_device_set_unplugged(struct drm_device *dev); +int udl_device_is_unplugged(struct drm_device *dev);
#define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */ #define CMD_WRITE_RL8 "\xAF\x61" /**< 8 bit run length command. */ diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 200419d..df1d53e 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -325,7 +325,7 @@ static int udl_fb_open(struct fb_info *info, int user) struct udl_device *udl = dev->dev_private;
/* If the USB device is gone, we don't accept new opens */ - if (drm_device_is_unplugged(udl->ddev)) + if (udl_device_is_unplugged(udl->ddev)) return -ENODEV;
ufbdev->fb_count++; diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index 33dbfb2..f6028a4 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -350,3 +350,18 @@ int udl_driver_unload(struct drm_device *dev) kfree(udl); return 0; } + +void udl_device_set_unplugged(struct drm_device *dev) +{ + struct udl_device *udl = dev->dev_private; + smp_wmb(); + atomic_set(&udl->unplugged, 1); +} + +int udl_device_is_unplugged(struct drm_device *dev) +{ + struct udl_device *udl = dev->dev_private; + int ret = atomic_read(&udl->unplugged); + smp_rmb(); + return ret; +} diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d7162cf..40c6099 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,7 +748,6 @@ struct drm_device { struct drm_minor *control; /**< Control node */ struct drm_minor *primary; /**< Primary node */ struct drm_minor *render; /**< Render node */ - atomic_t unplugged; /**< Flag whether dev is dead */ struct inode *anon_inode; /**< inode for private address-space */ char *unique; /**< unique name of the device */ /*@} */ @@ -879,19 +878,6 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev, return ((dev->driver->driver_features & feature) ? 1 : 0); }
-static inline void drm_device_set_unplugged(struct drm_device *dev) -{ - smp_wmb(); - atomic_set(&dev->unplugged, 1); -} - -static inline int drm_device_is_unplugged(struct drm_device *dev) -{ - int ret = atomic_read(&dev->unplugged); - smp_rmb(); - return ret; -} - static inline bool drm_is_render_client(const struct drm_file *file_priv) { return file_priv->minor->type == DRM_MINOR_RENDER;
Hi
On Fri, Feb 5, 2016 at 10:57 PM, Haixia Shi hshi@chromium.org wrote:
This flag is only used for drm/udl.
Furthermore, it is horribly racy. Lets not make use of it outside of udl.
Reviewed-by: David Herrmann dh.herrmann@gmail.com
Thanks David
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org
drivers/gpu/drm/msm/msm_fbdev.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c index d95af6e..e119c29 100644 --- a/drivers/gpu/drm/msm/msm_fbdev.c +++ b/drivers/gpu/drm/msm/msm_fbdev.c @@ -65,9 +65,6 @@ static int msm_fbdev_mmap(struct fb_info *info, struct vm_area_struct *vma) struct drm_device *dev = helper->dev; int ret = 0;
if (drm_device_is_unplugged(dev))return -ENODEV;ret = drm_gem_mmap_obj(drm_obj, drm_obj->size, vma); if (ret) { pr_err("%s:drm_gem_mmap_obj fail\n", __func__);-- 2.7.0.rc3.207.g0ac5344
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Feb 09, 2016 at 11:25:27AM +0100, David Herrmann wrote:
Hi
On Fri, Feb 5, 2016 at 10:57 PM, Haixia Shi hshi@chromium.org wrote:
This flag is only used for drm/udl.
Furthermore, it is horribly racy. Lets not make use of it outside of udl.
Reviewed-by: David Herrmann dh.herrmann@gmail.com
Applied to drm-misc (assuming that 2/2 will go in there too). -Daniel
Thanks David
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org
drivers/gpu/drm/msm/msm_fbdev.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c index d95af6e..e119c29 100644 --- a/drivers/gpu/drm/msm/msm_fbdev.c +++ b/drivers/gpu/drm/msm/msm_fbdev.c @@ -65,9 +65,6 @@ static int msm_fbdev_mmap(struct fb_info *info, struct vm_area_struct *vma) struct drm_device *dev = helper->dev; int ret = 0;
if (drm_device_is_unplugged(dev))return -ENODEV;ret = drm_gem_mmap_obj(drm_obj, drm_obj->size, vma); if (ret) { pr_err("%s:drm_gem_mmap_obj fail\n", __func__);-- 2.7.0.rc3.207.g0ac5344
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Remove the general drm_device_is_unplugged() checks, and move the unplugged flag handling logic into drm/udl. In general we want to keep driver-specific logic out of common drm code.
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org --- drivers/gpu/drm/drm_drv.c | 6 ---- drivers/gpu/drm/drm_fops.c | 2 -- drivers/gpu/drm/drm_gem.c | 3 -- drivers/gpu/drm/drm_ioctl.c | 3 -- drivers/gpu/drm/drm_vm.c | 3 -- drivers/gpu/drm/udl/udl_connector.c | 2 +- drivers/gpu/drm/udl/udl_drv.c | 7 +++-- drivers/gpu/drm/udl/udl_drv.h | 6 ++++ drivers/gpu/drm/udl/udl_fb.c | 2 +- drivers/gpu/drm/udl/udl_gem.c | 5 +++ drivers/gpu/drm/udl/udl_main.c | 61 +++++++++++++++++++++++++++++++++++++ include/drm/drmP.h | 14 --------- 12 files changed, 78 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 167c8d3..f93ee12 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)
if (!minor) { return ERR_PTR(-ENODEV); - } else if (drm_device_is_unplugged(minor->dev)) { - drm_dev_unref(minor->dev); - return ERR_PTR(-ENODEV); }
return minor; @@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev) drm_minor_unregister(dev, DRM_MINOR_CONTROL);
mutex_lock(&drm_global_mutex); - - drm_device_set_unplugged(dev); - if (dev->open_count == 0) { drm_put_dev(dev); } diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 1ea8790..b4332d4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file *filp)
if (!--dev->open_count) { retcode = drm_lastclose(dev); - if (drm_device_is_unplugged(dev)) - drm_put_dev(dev); } mutex_unlock(&drm_global_mutex);
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2e8c77e..c622e32 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_vma_offset_node *node; int ret;
- if (drm_device_is_unplugged(dev)) - return -ENODEV; - drm_vma_offset_lock_lookup(dev->vma_offset_manager); node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager, vma->vm_pgoff, diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 8ce2a0c..f959074 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
dev = file_priv->minor->dev;
- if (drm_device_is_unplugged(dev)) - return -ENODEV; - is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
if (is_driver_ioctl) { diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index f90bd5f..3a68be4 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_device *dev = priv->minor->dev; int ret;
- if (drm_device_is_unplugged(dev)) - return -ENODEV; - mutex_lock(&dev->struct_mutex); ret = drm_mmap_locked(filp, vma); mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c index 4709b54..b168f2c 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -96,7 +96,7 @@ static int udl_mode_valid(struct drm_connector *connector, static enum drm_connector_status udl_detect(struct drm_connector *connector, bool force) { - if (drm_device_is_unplugged(connector->dev)) + if (udl_device_is_unplugged(connector->dev)) return connector_status_disconnected; return connector_status_connected; } diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index d5728ec..f5c2a97 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -24,12 +24,12 @@ static const struct vm_operations_struct udl_gem_vm_ops = {
static const struct file_operations udl_driver_fops = { .owner = THIS_MODULE, - .open = drm_open, + .open = udl_drm_open, .mmap = udl_drm_gem_mmap, .poll = drm_poll, .read = drm_read, - .unlocked_ioctl = drm_ioctl, - .release = drm_release, + .unlocked_ioctl = udl_drm_ioctl, + .release = udl_drm_release, #ifdef CONFIG_COMPAT .compat_ioctl = drm_compat_ioctl, #endif @@ -97,6 +97,7 @@ static void udl_usb_disconnect(struct usb_interface *interface) drm_connector_unplug_all(dev); udl_fbdev_unplug(dev); udl_drop_usb(dev); + udl_device_set_unplugged(dev); drm_unplug_dev(dev); }
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index 4a064ef..75aa91c 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -65,6 +65,7 @@ struct udl_device { atomic_t bytes_identical; /* saved effort with backbuffer comparison */ atomic_t bytes_sent; /* to usb, after compression including overhead */ atomic_t cpu_kcycles_used; /* transpired during pixel processing */ + atomic_t unplugged; /* device has been unplugged or gone away */ };
struct udl_gem_object { @@ -133,6 +134,9 @@ int udl_gem_get_pages(struct udl_gem_object *obj); void udl_gem_put_pages(struct udl_gem_object *obj); int udl_gem_vmap(struct udl_gem_object *obj); void udl_gem_vunmap(struct udl_gem_object *obj); +int udl_drm_open(struct inode *inode, struct file *filp); +int udl_drm_release(struct inode *inode, struct file *filp); +int udl_drm_ioctl(struct file *filp, unsigned int cmd, unsigned long arg); int udl_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma); int udl_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf);
@@ -140,6 +144,8 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y, int width, int height);
int udl_drop_usb(struct drm_device *dev); +void udl_device_set_unplugged(struct drm_device *dev); +int udl_device_is_unplugged(struct drm_device *dev);
#define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */ #define CMD_WRITE_RL8 "\xAF\x61" /**< 8 bit run length command. */ diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 200419d..df1d53e 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -325,7 +325,7 @@ static int udl_fb_open(struct fb_info *info, int user) struct udl_device *udl = dev->dev_private;
/* If the USB device is gone, we don't accept new opens */ - if (drm_device_is_unplugged(udl->ddev)) + if (udl_device_is_unplugged(udl->ddev)) return -ENODEV;
ufbdev->fb_count++; diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c index 2a0a784..33c302f 100644 --- a/drivers/gpu/drm/udl/udl_gem.c +++ b/drivers/gpu/drm/udl/udl_gem.c @@ -86,8 +86,13 @@ int udl_dumb_create(struct drm_file *file,
int udl_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) { + struct drm_file *priv = filp->private_data; + struct drm_device *dev = priv->minor->dev; int ret;
+ if (udl_device_is_unplugged(dev)) + return -ENODEV; + ret = drm_gem_mmap(filp, vma); if (ret) return ret; diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index 33dbfb2..2ba3ee3 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -350,3 +350,64 @@ int udl_driver_unload(struct drm_device *dev) kfree(udl); return 0; } + +int udl_drm_open(struct inode *inode, struct file *filp) +{ + struct drm_file *file_priv; + struct drm_minor *minor; + + int ret = drm_open(inode, filp); + if (ret) + return ret; + + file_priv = filp->private_data; + minor = file_priv->minor; + if (udl_device_is_unplugged(minor->dev)) { + drm_dev_unref(minor->dev); + return -ENODEV; + } + + return 0; +} + +int udl_drm_release(struct inode *inode, struct file *filp) +{ + struct drm_file *file_priv = filp->private_data; + struct drm_device *dev = file_priv->minor->dev; + + int ret = drm_release(inode, filp); + if (!dev->open_count && udl_device_is_unplugged(dev)) + drm_put_dev(dev); + + return ret; +} + +int udl_drm_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) +{ + struct drm_file *file_priv = filp->private_data; + struct drm_device *dev = file_priv->minor->dev; + unsigned int nr = DRM_IOCTL_NR(cmd); + + if (udl_device_is_unplugged(dev) && + nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_SETCRTC) && + nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_RMFB) && + nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_DESTROY_DUMB)) + return -ENODEV; + + return drm_ioctl(filp, cmd, arg); +} + +void udl_device_set_unplugged(struct drm_device *dev) +{ + struct udl_device *udl = dev->dev_private; + smp_wmb(); + atomic_set(&udl->unplugged, 1); +} + +int udl_device_is_unplugged(struct drm_device *dev) +{ + struct udl_device *udl = dev->dev_private; + int ret = atomic_read(&udl->unplugged); + smp_rmb(); + return ret; +} diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d7162cf..40c6099 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,7 +748,6 @@ struct drm_device { struct drm_minor *control; /**< Control node */ struct drm_minor *primary; /**< Primary node */ struct drm_minor *render; /**< Render node */ - atomic_t unplugged; /**< Flag whether dev is dead */ struct inode *anon_inode; /**< inode for private address-space */ char *unique; /**< unique name of the device */ /*@} */ @@ -879,19 +878,6 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev, return ((dev->driver->driver_features & feature) ? 1 : 0); }
-static inline void drm_device_set_unplugged(struct drm_device *dev) -{ - smp_wmb(); - atomic_set(&dev->unplugged, 1); -} - -static inline int drm_device_is_unplugged(struct drm_device *dev) -{ - int ret = atomic_read(&dev->unplugged); - smp_rmb(); - return ret; -} - static inline bool drm_is_render_client(const struct drm_file *file_priv) { return file_priv->minor->type == DRM_MINOR_RENDER;
On Tue, Feb 09, 2016 at 01:05:43PM -0800, Haixia Shi wrote:
Remove the general drm_device_is_unplugged() checks, and move the unplugged flag handling logic into drm/udl. In general we want to keep driver-specific logic out of common drm code.
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org
Looks like this is v2 to address David's feedback, but doesn't mention that anywhere. Please resend with: - v2 added, plus a small in-patch changelog of what you've changed in the commit message (see other patches on dri-devel). - add David to the Cc: section of the patch to make sure he doesn't miss it.
Thanks, Daniel
drivers/gpu/drm/drm_drv.c | 6 ---- drivers/gpu/drm/drm_fops.c | 2 -- drivers/gpu/drm/drm_gem.c | 3 -- drivers/gpu/drm/drm_ioctl.c | 3 -- drivers/gpu/drm/drm_vm.c | 3 -- drivers/gpu/drm/udl/udl_connector.c | 2 +- drivers/gpu/drm/udl/udl_drv.c | 7 +++-- drivers/gpu/drm/udl/udl_drv.h | 6 ++++ drivers/gpu/drm/udl/udl_fb.c | 2 +- drivers/gpu/drm/udl/udl_gem.c | 5 +++ drivers/gpu/drm/udl/udl_main.c | 61 +++++++++++++++++++++++++++++++++++++ include/drm/drmP.h | 14 --------- 12 files changed, 78 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 167c8d3..f93ee12 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)
if (!minor) { return ERR_PTR(-ENODEV);
} else if (drm_device_is_unplugged(minor->dev)) {
drm_dev_unref(minor->dev);return ERR_PTR(-ENODEV);}
return minor;
@@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev) drm_minor_unregister(dev, DRM_MINOR_CONTROL);
mutex_lock(&drm_global_mutex);
- drm_device_set_unplugged(dev);
- if (dev->open_count == 0) { drm_put_dev(dev); }
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 1ea8790..b4332d4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file *filp)
if (!--dev->open_count) { retcode = drm_lastclose(dev);
if (drm_device_is_unplugged(dev))drm_put_dev(dev);diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2e8c77e..c622e32 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_vma_offset_node *node; int ret;
- if (drm_device_is_unplugged(dev))
return -ENODEV;- drm_vma_offset_lock_lookup(dev->vma_offset_manager); node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager, vma->vm_pgoff,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 8ce2a0c..f959074 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
dev = file_priv->minor->dev;
if (drm_device_is_unplugged(dev))
return -ENODEV;is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
if (is_driver_ioctl) {
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index f90bd5f..3a68be4 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_device *dev = priv->minor->dev; int ret;
- if (drm_device_is_unplugged(dev))
return -ENODEV;- mutex_lock(&dev->struct_mutex); ret = drm_mmap_locked(filp, vma); mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c index 4709b54..b168f2c 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -96,7 +96,7 @@ static int udl_mode_valid(struct drm_connector *connector, static enum drm_connector_status udl_detect(struct drm_connector *connector, bool force) {
- if (drm_device_is_unplugged(connector->dev))
- if (udl_device_is_unplugged(connector->dev)) return connector_status_disconnected; return connector_status_connected;
} diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index d5728ec..f5c2a97 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -24,12 +24,12 @@ static const struct vm_operations_struct udl_gem_vm_ops = {
static const struct file_operations udl_driver_fops = { .owner = THIS_MODULE,
- .open = drm_open,
- .open = udl_drm_open, .mmap = udl_drm_gem_mmap, .poll = drm_poll, .read = drm_read,
- .unlocked_ioctl = drm_ioctl,
- .release = drm_release,
- .unlocked_ioctl = udl_drm_ioctl,
- .release = udl_drm_release,
#ifdef CONFIG_COMPAT .compat_ioctl = drm_compat_ioctl, #endif @@ -97,6 +97,7 @@ static void udl_usb_disconnect(struct usb_interface *interface) drm_connector_unplug_all(dev); udl_fbdev_unplug(dev); udl_drop_usb(dev);
- udl_device_set_unplugged(dev); drm_unplug_dev(dev);
}
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index 4a064ef..75aa91c 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -65,6 +65,7 @@ struct udl_device { atomic_t bytes_identical; /* saved effort with backbuffer comparison */ atomic_t bytes_sent; /* to usb, after compression including overhead */ atomic_t cpu_kcycles_used; /* transpired during pixel processing */
- atomic_t unplugged; /* device has been unplugged or gone away */
};
struct udl_gem_object { @@ -133,6 +134,9 @@ int udl_gem_get_pages(struct udl_gem_object *obj); void udl_gem_put_pages(struct udl_gem_object *obj); int udl_gem_vmap(struct udl_gem_object *obj); void udl_gem_vunmap(struct udl_gem_object *obj); +int udl_drm_open(struct inode *inode, struct file *filp); +int udl_drm_release(struct inode *inode, struct file *filp); +int udl_drm_ioctl(struct file *filp, unsigned int cmd, unsigned long arg); int udl_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma); int udl_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf);
@@ -140,6 +144,8 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y, int width, int height);
int udl_drop_usb(struct drm_device *dev); +void udl_device_set_unplugged(struct drm_device *dev); +int udl_device_is_unplugged(struct drm_device *dev);
#define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */ #define CMD_WRITE_RL8 "\xAF\x61" /**< 8 bit run length command. */ diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 200419d..df1d53e 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -325,7 +325,7 @@ static int udl_fb_open(struct fb_info *info, int user) struct udl_device *udl = dev->dev_private;
/* If the USB device is gone, we don't accept new opens */
- if (drm_device_is_unplugged(udl->ddev))
if (udl_device_is_unplugged(udl->ddev)) return -ENODEV;
ufbdev->fb_count++;
diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c index 2a0a784..33c302f 100644 --- a/drivers/gpu/drm/udl/udl_gem.c +++ b/drivers/gpu/drm/udl/udl_gem.c @@ -86,8 +86,13 @@ int udl_dumb_create(struct drm_file *file,
int udl_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) {
struct drm_file *priv = filp->private_data;
struct drm_device *dev = priv->minor->dev; int ret;
if (udl_device_is_unplugged(dev))
return -ENODEV;ret = drm_gem_mmap(filp, vma); if (ret) return ret;
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index 33dbfb2..2ba3ee3 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -350,3 +350,64 @@ int udl_driver_unload(struct drm_device *dev) kfree(udl); return 0; }
+int udl_drm_open(struct inode *inode, struct file *filp) +{
- struct drm_file *file_priv;
- struct drm_minor *minor;
- int ret = drm_open(inode, filp);
- if (ret)
return ret;- file_priv = filp->private_data;
- minor = file_priv->minor;
- if (udl_device_is_unplugged(minor->dev)) {
drm_dev_unref(minor->dev);return -ENODEV;- }
- return 0;
+}
+int udl_drm_release(struct inode *inode, struct file *filp) +{
- struct drm_file *file_priv = filp->private_data;
- struct drm_device *dev = file_priv->minor->dev;
- int ret = drm_release(inode, filp);
- if (!dev->open_count && udl_device_is_unplugged(dev))
drm_put_dev(dev);- return ret;
+}
+int udl_drm_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) +{
- struct drm_file *file_priv = filp->private_data;
- struct drm_device *dev = file_priv->minor->dev;
- unsigned int nr = DRM_IOCTL_NR(cmd);
- if (udl_device_is_unplugged(dev) &&
nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_SETCRTC) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_RMFB) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_DESTROY_DUMB))return -ENODEV;- return drm_ioctl(filp, cmd, arg);
+}
+void udl_device_set_unplugged(struct drm_device *dev) +{
- struct udl_device *udl = dev->dev_private;
- smp_wmb();
- atomic_set(&udl->unplugged, 1);
+}
+int udl_device_is_unplugged(struct drm_device *dev) +{
- struct udl_device *udl = dev->dev_private;
- int ret = atomic_read(&udl->unplugged);
- smp_rmb();
- return ret;
+} diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d7162cf..40c6099 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,7 +748,6 @@ struct drm_device { struct drm_minor *control; /**< Control node */ struct drm_minor *primary; /**< Primary node */ struct drm_minor *render; /**< Render node */
- atomic_t unplugged; /**< Flag whether dev is dead */ struct inode *anon_inode; /**< inode for private address-space */ char *unique; /**< unique name of the device */ /*@} */
@@ -879,19 +878,6 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev, return ((dev->driver->driver_features & feature) ? 1 : 0); }
-static inline void drm_device_set_unplugged(struct drm_device *dev) -{
- smp_wmb();
- atomic_set(&dev->unplugged, 1);
-}
-static inline int drm_device_is_unplugged(struct drm_device *dev) -{
- int ret = atomic_read(&dev->unplugged);
- smp_rmb();
- return ret;
-}
static inline bool drm_is_render_client(const struct drm_file *file_priv) { return file_priv->minor->type == DRM_MINOR_RENDER; -- 2.7.0.rc3.207.g0ac5344
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi
On Tue, Feb 9, 2016 at 10:05 PM, Haixia Shi hshi@chromium.org wrote:
Remove the general drm_device_is_unplugged() checks, and move the unplugged flag handling logic into drm/udl. In general we want to keep driver-specific logic out of common drm code.
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org
drivers/gpu/drm/drm_drv.c | 6 ---- drivers/gpu/drm/drm_fops.c | 2 -- drivers/gpu/drm/drm_gem.c | 3 -- drivers/gpu/drm/drm_ioctl.c | 3 -- drivers/gpu/drm/drm_vm.c | 3 -- drivers/gpu/drm/udl/udl_connector.c | 2 +- drivers/gpu/drm/udl/udl_drv.c | 7 +++-- drivers/gpu/drm/udl/udl_drv.h | 6 ++++ drivers/gpu/drm/udl/udl_fb.c | 2 +- drivers/gpu/drm/udl/udl_gem.c | 5 +++ drivers/gpu/drm/udl/udl_main.c | 61 +++++++++++++++++++++++++++++++++++++ include/drm/drmP.h | 14 --------- 12 files changed, 78 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 167c8d3..f93ee12 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)
if (!minor) { return ERR_PTR(-ENODEV);
} else if (drm_device_is_unplugged(minor->dev)) {drm_dev_unref(minor->dev);return ERR_PTR(-ENODEV); } return minor;@@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev) drm_minor_unregister(dev, DRM_MINOR_CONTROL);
mutex_lock(&drm_global_mutex);
drm_device_set_unplugged(dev);if (dev->open_count == 0) { drm_put_dev(dev); }diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 1ea8790..b4332d4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file *filp)
if (!--dev->open_count) { retcode = drm_lastclose(dev);
if (drm_device_is_unplugged(dev))drm_put_dev(dev); } mutex_unlock(&drm_global_mutex);diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2e8c77e..c622e32 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_vma_offset_node *node; int ret;
if (drm_device_is_unplugged(dev))return -ENODEV;drm_vma_offset_lock_lookup(dev->vma_offset_manager); node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager, vma->vm_pgoff,diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 8ce2a0c..f959074 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
dev = file_priv->minor->dev;
if (drm_device_is_unplugged(dev))return -ENODEV;is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END; if (is_driver_ioctl) {diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index f90bd5f..3a68be4 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_device *dev = priv->minor->dev; int ret;
if (drm_device_is_unplugged(dev))return -ENODEV;mutex_lock(&dev->struct_mutex); ret = drm_mmap_locked(filp, vma); mutex_unlock(&dev->struct_mutex);diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c index 4709b54..b168f2c 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -96,7 +96,7 @@ static int udl_mode_valid(struct drm_connector *connector, static enum drm_connector_status udl_detect(struct drm_connector *connector, bool force) {
if (drm_device_is_unplugged(connector->dev))
if (udl_device_is_unplugged(connector->dev)) return connector_status_disconnected; return connector_status_connected;} diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index d5728ec..f5c2a97 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -24,12 +24,12 @@ static const struct vm_operations_struct udl_gem_vm_ops = {
static const struct file_operations udl_driver_fops = { .owner = THIS_MODULE,
.open = drm_open,
.open = udl_drm_open, .mmap = udl_drm_gem_mmap, .poll = drm_poll, .read = drm_read,
.unlocked_ioctl = drm_ioctl,.release = drm_release,
.unlocked_ioctl = udl_drm_ioctl,.release = udl_drm_release,#ifdef CONFIG_COMPAT .compat_ioctl = drm_compat_ioctl, #endif @@ -97,6 +97,7 @@ static void udl_usb_disconnect(struct usb_interface *interface) drm_connector_unplug_all(dev); udl_fbdev_unplug(dev); udl_drop_usb(dev);
udl_device_set_unplugged(dev); drm_unplug_dev(dev);}
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index 4a064ef..75aa91c 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -65,6 +65,7 @@ struct udl_device { atomic_t bytes_identical; /* saved effort with backbuffer comparison */ atomic_t bytes_sent; /* to usb, after compression including overhead */ atomic_t cpu_kcycles_used; /* transpired during pixel processing */
atomic_t unplugged; /* device has been unplugged or gone away */};
struct udl_gem_object { @@ -133,6 +134,9 @@ int udl_gem_get_pages(struct udl_gem_object *obj); void udl_gem_put_pages(struct udl_gem_object *obj); int udl_gem_vmap(struct udl_gem_object *obj); void udl_gem_vunmap(struct udl_gem_object *obj); +int udl_drm_open(struct inode *inode, struct file *filp); +int udl_drm_release(struct inode *inode, struct file *filp); +int udl_drm_ioctl(struct file *filp, unsigned int cmd, unsigned long arg); int udl_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma); int udl_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf);
@@ -140,6 +144,8 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y, int width, int height);
int udl_drop_usb(struct drm_device *dev); +void udl_device_set_unplugged(struct drm_device *dev); +int udl_device_is_unplugged(struct drm_device *dev);
#define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */ #define CMD_WRITE_RL8 "\xAF\x61" /**< 8 bit run length command. */ diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 200419d..df1d53e 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -325,7 +325,7 @@ static int udl_fb_open(struct fb_info *info, int user) struct udl_device *udl = dev->dev_private;
/* If the USB device is gone, we don't accept new opens */
if (drm_device_is_unplugged(udl->ddev))
if (udl_device_is_unplugged(udl->ddev)) return -ENODEV; ufbdev->fb_count++;diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c index 2a0a784..33c302f 100644 --- a/drivers/gpu/drm/udl/udl_gem.c +++ b/drivers/gpu/drm/udl/udl_gem.c @@ -86,8 +86,13 @@ int udl_dumb_create(struct drm_file *file,
int udl_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) {
struct drm_file *priv = filp->private_data;struct drm_device *dev = priv->minor->dev; int ret;if (udl_device_is_unplugged(dev))return -ENODEV;ret = drm_gem_mmap(filp, vma); if (ret) return ret;diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index 33dbfb2..2ba3ee3 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -350,3 +350,64 @@ int udl_driver_unload(struct drm_device *dev) kfree(udl); return 0; }
+int udl_drm_open(struct inode *inode, struct file *filp) +{
struct drm_file *file_priv;struct drm_minor *minor;int ret = drm_open(inode, filp);if (ret)return ret;file_priv = filp->private_data;minor = file_priv->minor;if (udl_device_is_unplugged(minor->dev)) {drm_dev_unref(minor->dev);return -ENODEV;}
This doesn't make sense to me. You're called here with global-mutex held, so you can just check udl_device_is_unplugged() before calling into drm_open().
return 0;+}
+int udl_drm_release(struct inode *inode, struct file *filp) +{
struct drm_file *file_priv = filp->private_data;struct drm_device *dev = file_priv->minor->dev;int ret = drm_release(inode, filp);if (!dev->open_count && udl_device_is_unplugged(dev))drm_put_dev(dev);
This should rather be:
drm_release(inode, filp); mutex_lock(&drm_global_mutex); if (!dev->open_count && udl_device_is_unplugged(dev)) drm_put_dev(dev); mutex_unlock(&drm_global_mutex);
return 0;
There is no reason to look at the return code of drm_release(), ever.
return ret;+}
+int udl_drm_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) +{
struct drm_file *file_priv = filp->private_data;struct drm_device *dev = file_priv->minor->dev;unsigned int nr = DRM_IOCTL_NR(cmd);if (udl_device_is_unplugged(dev) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_SETCRTC) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_RMFB) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_DESTROY_DUMB))return -ENODEV;
Why?
Just do:
if (udl_device_is_unplugged(dev)) return -ENODEV;
Why this complex logic here?
Thanks David
return drm_ioctl(filp, cmd, arg);+}
+void udl_device_set_unplugged(struct drm_device *dev) +{
struct udl_device *udl = dev->dev_private;smp_wmb();atomic_set(&udl->unplugged, 1);+}
+int udl_device_is_unplugged(struct drm_device *dev) +{
struct udl_device *udl = dev->dev_private;int ret = atomic_read(&udl->unplugged);smp_rmb();return ret;+} diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d7162cf..40c6099 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,7 +748,6 @@ struct drm_device { struct drm_minor *control; /**< Control node */ struct drm_minor *primary; /**< Primary node */ struct drm_minor *render; /**< Render node */
atomic_t unplugged; /**< Flag whether dev is dead */ struct inode *anon_inode; /**< inode for private address-space */ char *unique; /**< unique name of the device */ /*@} */@@ -879,19 +878,6 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev, return ((dev->driver->driver_features & feature) ? 1 : 0); }
-static inline void drm_device_set_unplugged(struct drm_device *dev) -{
smp_wmb();atomic_set(&dev->unplugged, 1);-}
-static inline int drm_device_is_unplugged(struct drm_device *dev) -{
int ret = atomic_read(&dev->unplugged);smp_rmb();return ret;-}
static inline bool drm_is_render_client(const struct drm_file *file_priv) { return file_priv->minor->type == DRM_MINOR_RENDER; -- 2.7.0.rc3.207.g0ac5344
if (udl_device_is_unplugged(dev) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_SETCRTC) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_RMFB) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_DESTROY_DUMB))return -ENODEV;Why?
Just do:
if (udl_device_is_unplugged(dev)) return -ENODEV;Why this complex logic here?
Because there are legitimate ioctl calls after UDL is unplugged. See crbug.com/583508 and crbug.com/583758 for some background.
The userspace (Chrome in this case) has allocated FBs and Dumb buffers on the drm device and needs to be given a chance to properly deallocate them (via RMFB and DESTROY_DUMB). In addition, it needs to call SETCRTC with fb_id = 0 to properly release the last refcount on the primary fb.
I initially proposed adding an "UNPLUG_DISALLOW" flag to ioctls so that we can whitelist them on a case-by-case basis but that proposal got shot down as being unnecessary, but you can see my original patch at https://chromium-review.googlesource.com/#/c/326160/
On Wed, Feb 10, 2016 at 4:24 AM, David Herrmann dh.herrmann@gmail.com wrote:
Hi
On Tue, Feb 9, 2016 at 10:05 PM, Haixia Shi hshi@chromium.org wrote:
Remove the general drm_device_is_unplugged() checks, and move the
unplugged
flag handling logic into drm/udl. In general we want to keep
driver-specific
logic out of common drm code.
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org
drivers/gpu/drm/drm_drv.c | 6 ---- drivers/gpu/drm/drm_fops.c | 2 -- drivers/gpu/drm/drm_gem.c | 3 -- drivers/gpu/drm/drm_ioctl.c | 3 -- drivers/gpu/drm/drm_vm.c | 3 -- drivers/gpu/drm/udl/udl_connector.c | 2 +- drivers/gpu/drm/udl/udl_drv.c | 7 +++-- drivers/gpu/drm/udl/udl_drv.h | 6 ++++ drivers/gpu/drm/udl/udl_fb.c | 2 +- drivers/gpu/drm/udl/udl_gem.c | 5 +++ drivers/gpu/drm/udl/udl_main.c | 61
+++++++++++++++++++++++++++++++++++++
include/drm/drmP.h | 14 --------- 12 files changed, 78 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 167c8d3..f93ee12 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int
minor_id)
if (!minor) { return ERR_PTR(-ENODEV);
} else if (drm_device_is_unplugged(minor->dev)) {drm_dev_unref(minor->dev);return ERR_PTR(-ENODEV); } return minor;@@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev) drm_minor_unregister(dev, DRM_MINOR_CONTROL);
mutex_lock(&drm_global_mutex);
drm_device_set_unplugged(dev);if (dev->open_count == 0) { drm_put_dev(dev); }diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 1ea8790..b4332d4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file
*filp)
if (!--dev->open_count) { retcode = drm_lastclose(dev);
if (drm_device_is_unplugged(dev))drm_put_dev(dev); } mutex_unlock(&drm_global_mutex);diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2e8c77e..c622e32 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct
vm_area_struct *vma)
struct drm_vma_offset_node *node; int ret;
if (drm_device_is_unplugged(dev))return -ENODEV;drm_vma_offset_lock_lookup(dev->vma_offset_manager); node =drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
vma->vm_pgoff,diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 8ce2a0c..f959074 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
dev = file_priv->minor->dev;
if (drm_device_is_unplugged(dev))return -ENODEV;is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END; if (is_driver_ioctl) {diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index f90bd5f..3a68be4 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct
vm_area_struct *vma)
struct drm_device *dev = priv->minor->dev; int ret;
if (drm_device_is_unplugged(dev))return -ENODEV;mutex_lock(&dev->struct_mutex); ret = drm_mmap_locked(filp, vma); mutex_unlock(&dev->struct_mutex);diff --git a/drivers/gpu/drm/udl/udl_connector.c
b/drivers/gpu/drm/udl/udl_connector.c
index 4709b54..b168f2c 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -96,7 +96,7 @@ static int udl_mode_valid(struct drm_connector
*connector,
static enum drm_connector_status udl_detect(struct drm_connector *connector, bool force) {
if (drm_device_is_unplugged(connector->dev))
if (udl_device_is_unplugged(connector->dev)) return connector_status_disconnected; return connector_status_connected;} diff --git a/drivers/gpu/drm/udl/udl_drv.c
b/drivers/gpu/drm/udl/udl_drv.c
index d5728ec..f5c2a97 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -24,12 +24,12 @@ static const struct vm_operations_struct
udl_gem_vm_ops = {
static const struct file_operations udl_driver_fops = { .owner = THIS_MODULE,
.open = drm_open,
.open = udl_drm_open, .mmap = udl_drm_gem_mmap, .poll = drm_poll, .read = drm_read,
.unlocked_ioctl = drm_ioctl,.release = drm_release,
.unlocked_ioctl = udl_drm_ioctl,.release = udl_drm_release,#ifdef CONFIG_COMPAT .compat_ioctl = drm_compat_ioctl, #endif @@ -97,6 +97,7 @@ static void udl_usb_disconnect(struct usb_interface
*interface)
drm_connector_unplug_all(dev); udl_fbdev_unplug(dev); udl_drop_usb(dev);
udl_device_set_unplugged(dev); drm_unplug_dev(dev);}
diff --git a/drivers/gpu/drm/udl/udl_drv.h
b/drivers/gpu/drm/udl/udl_drv.h
index 4a064ef..75aa91c 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -65,6 +65,7 @@ struct udl_device { atomic_t bytes_identical; /* saved effort with backbuffer
comparison */
atomic_t bytes_sent; /* to usb, after compression includingoverhead */
atomic_t cpu_kcycles_used; /* transpired during pixel processing*/
atomic_t unplugged; /* device has been unplugged or gone away */};
struct udl_gem_object { @@ -133,6 +134,9 @@ int udl_gem_get_pages(struct udl_gem_object *obj); void udl_gem_put_pages(struct udl_gem_object *obj); int udl_gem_vmap(struct udl_gem_object *obj); void udl_gem_vunmap(struct udl_gem_object *obj); +int udl_drm_open(struct inode *inode, struct file *filp); +int udl_drm_release(struct inode *inode, struct file *filp); +int udl_drm_ioctl(struct file *filp, unsigned int cmd, unsigned long
arg);
int udl_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma); int udl_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf);
@@ -140,6 +144,8 @@ int udl_handle_damage(struct udl_framebuffer *fb,
int x, int y,
int width, int height);int udl_drop_usb(struct drm_device *dev); +void udl_device_set_unplugged(struct drm_device *dev); +int udl_device_is_unplugged(struct drm_device *dev);
#define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */ #define CMD_WRITE_RL8 "\xAF\x61" /**< 8 bit run length command. */ diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 200419d..df1d53e 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -325,7 +325,7 @@ static int udl_fb_open(struct fb_info *info, int
user)
struct udl_device *udl = dev->dev_private; /* If the USB device is gone, we don't accept new opens */
if (drm_device_is_unplugged(udl->ddev))
if (udl_device_is_unplugged(udl->ddev)) return -ENODEV; ufbdev->fb_count++;diff --git a/drivers/gpu/drm/udl/udl_gem.c
b/drivers/gpu/drm/udl/udl_gem.c
index 2a0a784..33c302f 100644 --- a/drivers/gpu/drm/udl/udl_gem.c +++ b/drivers/gpu/drm/udl/udl_gem.c @@ -86,8 +86,13 @@ int udl_dumb_create(struct drm_file *file,
int udl_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) {
struct drm_file *priv = filp->private_data;struct drm_device *dev = priv->minor->dev; int ret;if (udl_device_is_unplugged(dev))return -ENODEV;ret = drm_gem_mmap(filp, vma); if (ret) return ret;diff --git a/drivers/gpu/drm/udl/udl_main.c
b/drivers/gpu/drm/udl/udl_main.c
index 33dbfb2..2ba3ee3 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -350,3 +350,64 @@ int udl_driver_unload(struct drm_device *dev) kfree(udl); return 0; }
+int udl_drm_open(struct inode *inode, struct file *filp) +{
struct drm_file *file_priv;struct drm_minor *minor;int ret = drm_open(inode, filp);if (ret)return ret;file_priv = filp->private_data;minor = file_priv->minor;if (udl_device_is_unplugged(minor->dev)) {drm_dev_unref(minor->dev);return -ENODEV;}This doesn't make sense to me. You're called here with global-mutex held, so you can just check udl_device_is_unplugged() before calling into drm_open().
return 0;+}
+int udl_drm_release(struct inode *inode, struct file *filp) +{
struct drm_file *file_priv = filp->private_data;struct drm_device *dev = file_priv->minor->dev;int ret = drm_release(inode, filp);if (!dev->open_count && udl_device_is_unplugged(dev))drm_put_dev(dev);This should rather be:
drm_release(inode, filp); mutex_lock(&drm_global_mutex); if (!dev->open_count && udl_device_is_unplugged(dev)) drm_put_dev(dev); mutex_unlock(&drm_global_mutex); return 0;There is no reason to look at the return code of drm_release(), ever.
return ret;+}
+int udl_drm_ioctl(struct file *filp, unsigned int cmd, unsigned long
arg)
+{
struct drm_file *file_priv = filp->private_data;struct drm_device *dev = file_priv->minor->dev;unsigned int nr = DRM_IOCTL_NR(cmd);if (udl_device_is_unplugged(dev) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_SETCRTC) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_RMFB) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_DESTROY_DUMB))return -ENODEV;Why?
Just do:
if (udl_device_is_unplugged(dev)) return -ENODEV;Why this complex logic here?
Thanks David
return drm_ioctl(filp, cmd, arg);+}
+void udl_device_set_unplugged(struct drm_device *dev) +{
struct udl_device *udl = dev->dev_private;smp_wmb();atomic_set(&udl->unplugged, 1);+}
+int udl_device_is_unplugged(struct drm_device *dev) +{
struct udl_device *udl = dev->dev_private;int ret = atomic_read(&udl->unplugged);smp_rmb();return ret;+} diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d7162cf..40c6099 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,7 +748,6 @@ struct drm_device { struct drm_minor *control; /**< Control node */ struct drm_minor *primary; /**< Primary node */ struct drm_minor *render; /**< Render node */
atomic_t unplugged; /**< Flag whether dev isdead */
struct inode *anon_inode; /**< inode for privateaddress-space */
char *unique; /**< unique name of thedevice */
/*@} */@@ -879,19 +878,6 @@ static __inline__ int drm_core_check_feature(struct
drm_device *dev,
return ((dev->driver->driver_features & feature) ? 1 : 0);}
-static inline void drm_device_set_unplugged(struct drm_device *dev) -{
smp_wmb();atomic_set(&dev->unplugged, 1);-}
-static inline int drm_device_is_unplugged(struct drm_device *dev) -{
int ret = atomic_read(&dev->unplugged);smp_rmb();return ret;-}
static inline bool drm_is_render_client(const struct drm_file
*file_priv)
{ return file_priv->minor->type == DRM_MINOR_RENDER; -- 2.7.0.rc3.207.g0ac5344
Hi
On Wed, Feb 10, 2016 at 9:39 PM, Haixia Shi hshi@chromium.org wrote:
if (udl_device_is_unplugged(dev) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_SETCRTC) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_RMFB) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_DESTROY_DUMB))return -ENODEV;Why?
Just do:
if (udl_device_is_unplugged(dev)) return -ENODEV;Why this complex logic here?
Because there are legitimate ioctl calls after UDL is unplugged. See crbug.com/583508 and crbug.com/583758 for some background.
The userspace (Chrome in this case) has allocated FBs and Dumb buffers on the drm device and needs to be given a chance to properly deallocate them (via RMFB and DESTROY_DUMB). In addition, it needs to call SETCRTC with fb_id = 0 to properly release the last refcount on the primary fb.
I initially proposed adding an "UNPLUG_DISALLOW" flag to ioctls so that we can whitelist them on a case-by-case basis but that proposal got shot down as being unnecessary, but you can see my original patch at https://chromium-review.googlesource.com/#/c/326160/
If a device is unplugged, you should consider all your resources to be destroyed. There is no reason to release them manually. User-space *must* be able to deal with asynchronous unplugs.
If UDL does not release your resources, and you actually hit bugs due to this, then fix UDL to do this. But extending the already broken UNPLUG-hacks we have sounds wrong to me.
Anyway, this change is unrelated to your patch, so please drop it. Feel free to send a separate patch, if you want to continue the discussion.
Thanks David
On Wed, Feb 10, 2016 at 1:38 PM, David Herrmann dh.herrmann@gmail.com wrote:
Hi
On Wed, Feb 10, 2016 at 9:39 PM, Haixia Shi hshi@chromium.org wrote:
if (udl_device_is_unplugged(dev) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_SETCRTC) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_RMFB) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_DESTROY_DUMB))return -ENODEV;Why?
Just do:
if (udl_device_is_unplugged(dev)) return -ENODEV;Why this complex logic here?
Because there are legitimate ioctl calls after UDL is unplugged. See crbug.com/583508 and crbug.com/583758 for some background.
The userspace (Chrome in this case) has allocated FBs and Dumb buffers on the drm device and needs to be given a chance to properly deallocate them (via RMFB and DESTROY_DUMB). In addition, it needs to call SETCRTC with fb_id = 0 to properly release the last refcount on the primary fb.
I initially proposed adding an "UNPLUG_DISALLOW" flag to ioctls so that we can whitelist them on a case-by-case basis but that proposal got shot down as being unnecessary, but you can see my original patch at https://chromium-review.googlesource.com/#/c/326160/
If a device is unplugged, you should consider all your resources to be destroyed. There is no reason to release them manually. User-space *must* be able to deal with asynchronous unplugs.
So the problem if you do that is that things like a buffer's memory pages can disappear from under you. How would you deal with that case? User space certainly can't have a segfault handler catch that just in case :)
Stéphane
If UDL does not release your resources, and you actually hit bugs due to this, then fix UDL to do this. But extending the already broken UNPLUG-hacks we have sounds wrong to me.
Anyway, this change is unrelated to your patch, so please drop it. Feel free to send a separate patch, if you want to continue the discussion.
Thanks David _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi
On Wed, Feb 10, 2016 at 10:46 PM, Stéphane Marchesin stephane.marchesin@gmail.com wrote:
On Wed, Feb 10, 2016 at 1:38 PM, David Herrmann dh.herrmann@gmail.com wrote:
Hi
On Wed, Feb 10, 2016 at 9:39 PM, Haixia Shi hshi@chromium.org wrote:
if (udl_device_is_unplugged(dev) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_SETCRTC) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_RMFB) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_DESTROY_DUMB))return -ENODEV;Why?
Just do:
if (udl_device_is_unplugged(dev)) return -ENODEV;Why this complex logic here?
Because there are legitimate ioctl calls after UDL is unplugged. See crbug.com/583508 and crbug.com/583758 for some background.
The userspace (Chrome in this case) has allocated FBs and Dumb buffers on the drm device and needs to be given a chance to properly deallocate them (via RMFB and DESTROY_DUMB). In addition, it needs to call SETCRTC with fb_id = 0 to properly release the last refcount on the primary fb.
I initially proposed adding an "UNPLUG_DISALLOW" flag to ioctls so that we can whitelist them on a case-by-case basis but that proposal got shot down as being unnecessary, but you can see my original patch at https://chromium-review.googlesource.com/#/c/326160/
If a device is unplugged, you should consider all your resources to be destroyed. There is no reason to release them manually. User-space *must* be able to deal with asynchronous unplugs.
So the problem if you do that is that things like a buffer's memory pages can disappear from under you. How would you deal with that case? User space certainly can't have a segfault handler catch that just in case :)
If you rip out hardware resources, then you better be able to deal with it. Sure, UDL is an exception as it doesn't have memory resources on the chip. But it sounds fishy to me, if you base your API on it. On a lot of other devices, the memory will simply not be there. So you cannot keep it around.
There are many ways to invalidate memory mappings. You either unmap the entire range (and user-space must deal with SIGBUS, which is completely feasible and a lot of code already does it), or you replace all with a zero page, or you duplicate all pages, ... IMO, user-space has to start dealing with hardware unplug properly and stop pretending it cannot happen.
If you mmap() your filesystem, and you rip out your block device, then you also will get SIGBUS if you access pages that are not in pagecache. Why are graphics buffers different?
Thanks David
On Wed, Feb 10, 2016 at 1:54 PM, David Herrmann dh.herrmann@gmail.com wrote:
Hi
On Wed, Feb 10, 2016 at 10:46 PM, Stéphane Marchesin stephane.marchesin@gmail.com wrote:
On Wed, Feb 10, 2016 at 1:38 PM, David Herrmann dh.herrmann@gmail.com wrote:
Hi
On Wed, Feb 10, 2016 at 9:39 PM, Haixia Shi hshi@chromium.org wrote:
if (udl_device_is_unplugged(dev) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_SETCRTC) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_RMFB) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_DESTROY_DUMB))return -ENODEV;Why?
Just do:
if (udl_device_is_unplugged(dev)) return -ENODEV;Why this complex logic here?
Because there are legitimate ioctl calls after UDL is unplugged. See crbug.com/583508 and crbug.com/583758 for some background.
The userspace (Chrome in this case) has allocated FBs and Dumb buffers on the drm device and needs to be given a chance to properly deallocate them (via RMFB and DESTROY_DUMB). In addition, it needs to call SETCRTC with fb_id = 0 to properly release the last refcount on the primary fb.
I initially proposed adding an "UNPLUG_DISALLOW" flag to ioctls so that we can whitelist them on a case-by-case basis but that proposal got shot down as being unnecessary, but you can see my original patch at https://chromium-review.googlesource.com/#/c/326160/
If a device is unplugged, you should consider all your resources to be destroyed. There is no reason to release them manually. User-space *must* be able to deal with asynchronous unplugs.
So the problem if you do that is that things like a buffer's memory pages can disappear from under you. How would you deal with that case? User space certainly can't have a segfault handler catch that just in case :)
If you rip out hardware resources, then you better be able to deal with it. Sure, UDL is an exception as it doesn't have memory resources on the chip. But it sounds fishy to me, if you base your API on it. On a lot of other devices, the memory will simply not be there. So you cannot keep it around.
The thing is, you are not unplugging a device here; you are unplugging a USB monitor. As a proof that this is just a monitor, I can plug another USB monitor with the same driver and pick up where I left off. I guess I am saying that the concept of unplugging a device is not applicable here (or to any driver that I know, for that matter).
Other drivers already handle all this by, for example, failing page flips if the monitor is gone. We basically want to do the same for UDL; I don't think we need to invent a new level of unplug here.
There are many ways to invalidate memory mappings. You either unmap the entire range (and user-space must deal with SIGBUS, which is completely feasible and a lot of code already does it), or you replace all with a zero page, or you duplicate all pages, ... IMO, user-space has to start dealing with hardware unplug properly and stop pretending it cannot happen.
What you are suggesting is much more complicated than you claim, for example if you destroy the dmabuf which is shared with another driver, what happens? User space definitely can't deal with that.
I think we should wait until we have unpluggable display hardware before inventing really complex support for it.
Stéphane
If you mmap() your filesystem, and you rip out your block device, then you also will get SIGBUS if you access pages that are not in pagecache. Why are graphics buffers different?
Thanks David
Hi
On Wed, Feb 10, 2016 at 11:02 PM, Stéphane Marchesin stephane.marchesin@gmail.com wrote:
On Wed, Feb 10, 2016 at 1:54 PM, David Herrmann dh.herrmann@gmail.com wrote:
If a device is unplugged, you should consider all your resources to be destroyed. There is no reason to release them manually. User-space *must* be able to deal with asynchronous unplugs.
So the problem if you do that is that things like a buffer's memory pages can disappear from under you. How would you deal with that case? User space certainly can't have a segfault handler catch that just in case :)
If you rip out hardware resources, then you better be able to deal with it. Sure, UDL is an exception as it doesn't have memory resources on the chip. But it sounds fishy to me, if you base your API on it. On a lot of other devices, the memory will simply not be there. So you cannot keep it around.
The thing is, you are not unplugging a device here; you are unplugging a USB monitor. As a proof that this is just a monitor, I can plug another USB monitor with the same driver and pick up where I left off. I guess I am saying that the concept of unplugging a device is not applicable here (or to any driver that I know, for that matter).
Other drivers already handle all this by, for example, failing page flips if the monitor is gone. We basically want to do the same for UDL; I don't think we need to invent a new level of unplug here.
No, you rip out a display controller. You don't just unplug a monitor, you remove the whole hardware. If UDL were treated on the same level as connector-hotplugging, then you should implement it that way. But currently it is not.
Btw., even if you do this, you still have to deal with devices going away, and new devices being plugged. DRM does not support CRTC/connector hotplugging, yet. Hence, making UDL a singleton driver will not work either.
There are many ways to invalidate memory mappings. You either unmap the entire range (and user-space must deal with SIGBUS, which is completely feasible and a lot of code already does it), or you replace all with a zero page, or you duplicate all pages, ... IMO, user-space has to start dealing with hardware unplug properly and stop pretending it cannot happen.
What you are suggesting is much more complicated than you claim, for example if you destroy the dmabuf which is shared with another driver, what happens? User space definitely can't deal with that.
I think we should wait until we have unpluggable display hardware before inventing really complex support for it.
PCI Hotplugging. Has been around since a decade. Intel just got lucky that they cannot be ripped out. The other drivers don't care (and break horribly if you do).
Feel free to come up with a better way to deal with UDL hotplugging. However, I for myself recommend doing it properly, on the bus level. Like all other subsystems do.
Thanks David
On Wed, Feb 10, 2016 at 02:02:46PM -0800, Stéphane Marchesin wrote:
On Wed, Feb 10, 2016 at 1:54 PM, David Herrmann dh.herrmann@gmail.com wrote:
Hi
On Wed, Feb 10, 2016 at 10:46 PM, Stéphane Marchesin stephane.marchesin@gmail.com wrote:
On Wed, Feb 10, 2016 at 1:38 PM, David Herrmann dh.herrmann@gmail.com wrote:
Hi
On Wed, Feb 10, 2016 at 9:39 PM, Haixia Shi hshi@chromium.org wrote:
if (udl_device_is_unplugged(dev) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_SETCRTC) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_RMFB) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_DESTROY_DUMB))return -ENODEV;Why?
Just do:
if (udl_device_is_unplugged(dev)) return -ENODEV;Why this complex logic here?
Because there are legitimate ioctl calls after UDL is unplugged. See crbug.com/583508 and crbug.com/583758 for some background.
The userspace (Chrome in this case) has allocated FBs and Dumb buffers on the drm device and needs to be given a chance to properly deallocate them (via RMFB and DESTROY_DUMB). In addition, it needs to call SETCRTC with fb_id = 0 to properly release the last refcount on the primary fb.
I initially proposed adding an "UNPLUG_DISALLOW" flag to ioctls so that we can whitelist them on a case-by-case basis but that proposal got shot down as being unnecessary, but you can see my original patch at https://chromium-review.googlesource.com/#/c/326160/
If a device is unplugged, you should consider all your resources to be destroyed. There is no reason to release them manually. User-space *must* be able to deal with asynchronous unplugs.
So the problem if you do that is that things like a buffer's memory pages can disappear from under you. How would you deal with that case? User space certainly can't have a segfault handler catch that just in case :)
Yeah, dma-buf and fence lifetime is entirely unsolved. I agree that they /should/ keep being alive. Of course the actual data in it might be toast, but that's not any different from a gpu hang. At least i915 only gives you asynchronous signalling for "bad stuff happened" in that case, buffer access to corrupted data continues to work. And must do so, because X/compositors/clients would just die if we don't do that, and that's Not Good(tm).
If you rip out hardware resources, then you better be able to deal with it. Sure, UDL is an exception as it doesn't have memory resources on the chip. But it sounds fishy to me, if you base your API on it. On a lot of other devices, the memory will simply not be there. So you cannot keep it around.
The thing is, you are not unplugging a device here; you are unplugging a USB monitor. As a proof that this is just a monitor, I can plug another USB monitor with the same driver and pick up where I left off. I guess I am saying that the concept of unplugging a device is not applicable here (or to any driver that I know, for that matter).
Other drivers already handle all this by, for example, failing page flips if the monitor is gone. We basically want to do the same for UDL; I don't think we need to invent a new level of unplug here.
Just an aside: Imo failing pageflips is really bad behaviour of some kernel drivers (yes mst does it by force-disabling sinks). Imo userspace should ask for things to get disabled explicitly, much less potential for races. For mst I think the right solution is to send out the uevent, stope enumerating the port, but keep it internally alive until it's all gone.
Something similar could make sense for uld.
There are many ways to invalidate memory mappings. You either unmap the entire range (and user-space must deal with SIGBUS, which is completely feasible and a lot of code already does it), or you replace all with a zero page, or you duplicate all pages, ... IMO, user-space has to start dealing with hardware unplug properly and stop pretending it cannot happen.
What you are suggesting is much more complicated than you claim, for example if you destroy the dmabuf which is shared with another driver, what happens? User space definitely can't deal with that.
I think we should wait until we have unpluggable display hardware before inventing really complex support for it.
I agree that for now we probably should just have hacks for udl (and yeah fixing up mst to no longer just go poof is on my todo list somewhere), and leave the larger issue of drivers disappearing unfixed. Atm module unload is the only real user for that (except udl ofc), and that's a developer feature.
Fixing all the dma-buf/fence/drm device lifetime issues properly is super hard I think. And tons of work. -Daniel
This should rather be:
drm_release(inode, filp); mutex_lock(&drm_global_mutex); if (!dev->open_count && udl_device_is_unplugged(dev)) drm_put_dev(dev); mutex_unlock(&drm_global_mutex); return 0;There is no reason to look at the return code of drm_release(), ever.
But drm_release() does return a retcode. It would still make sense to return that as-is in case any existing code relies on it.
On Wed, Feb 10, 2016 at 4:24 AM, David Herrmann dh.herrmann@gmail.com wrote:
Hi
On Tue, Feb 9, 2016 at 10:05 PM, Haixia Shi hshi@chromium.org wrote:
Remove the general drm_device_is_unplugged() checks, and move the
unplugged
flag handling logic into drm/udl. In general we want to keep
driver-specific
logic out of common drm code.
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org
drivers/gpu/drm/drm_drv.c | 6 ---- drivers/gpu/drm/drm_fops.c | 2 -- drivers/gpu/drm/drm_gem.c | 3 -- drivers/gpu/drm/drm_ioctl.c | 3 -- drivers/gpu/drm/drm_vm.c | 3 -- drivers/gpu/drm/udl/udl_connector.c | 2 +- drivers/gpu/drm/udl/udl_drv.c | 7 +++-- drivers/gpu/drm/udl/udl_drv.h | 6 ++++ drivers/gpu/drm/udl/udl_fb.c | 2 +- drivers/gpu/drm/udl/udl_gem.c | 5 +++ drivers/gpu/drm/udl/udl_main.c | 61
+++++++++++++++++++++++++++++++++++++
include/drm/drmP.h | 14 --------- 12 files changed, 78 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 167c8d3..f93ee12 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int
minor_id)
if (!minor) { return ERR_PTR(-ENODEV);
} else if (drm_device_is_unplugged(minor->dev)) {drm_dev_unref(minor->dev);return ERR_PTR(-ENODEV); } return minor;@@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev) drm_minor_unregister(dev, DRM_MINOR_CONTROL);
mutex_lock(&drm_global_mutex);
drm_device_set_unplugged(dev);if (dev->open_count == 0) { drm_put_dev(dev); }diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 1ea8790..b4332d4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file
*filp)
if (!--dev->open_count) { retcode = drm_lastclose(dev);
if (drm_device_is_unplugged(dev))drm_put_dev(dev); } mutex_unlock(&drm_global_mutex);diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2e8c77e..c622e32 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct
vm_area_struct *vma)
struct drm_vma_offset_node *node; int ret;
if (drm_device_is_unplugged(dev))return -ENODEV;drm_vma_offset_lock_lookup(dev->vma_offset_manager); node =drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
vma->vm_pgoff,diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 8ce2a0c..f959074 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
dev = file_priv->minor->dev;
if (drm_device_is_unplugged(dev))return -ENODEV;is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END; if (is_driver_ioctl) {diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index f90bd5f..3a68be4 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct
vm_area_struct *vma)
struct drm_device *dev = priv->minor->dev; int ret;
if (drm_device_is_unplugged(dev))return -ENODEV;mutex_lock(&dev->struct_mutex); ret = drm_mmap_locked(filp, vma); mutex_unlock(&dev->struct_mutex);diff --git a/drivers/gpu/drm/udl/udl_connector.c
b/drivers/gpu/drm/udl/udl_connector.c
index 4709b54..b168f2c 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -96,7 +96,7 @@ static int udl_mode_valid(struct drm_connector
*connector,
static enum drm_connector_status udl_detect(struct drm_connector *connector, bool force) {
if (drm_device_is_unplugged(connector->dev))
if (udl_device_is_unplugged(connector->dev)) return connector_status_disconnected; return connector_status_connected;} diff --git a/drivers/gpu/drm/udl/udl_drv.c
b/drivers/gpu/drm/udl/udl_drv.c
index d5728ec..f5c2a97 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -24,12 +24,12 @@ static const struct vm_operations_struct
udl_gem_vm_ops = {
static const struct file_operations udl_driver_fops = { .owner = THIS_MODULE,
.open = drm_open,
.open = udl_drm_open, .mmap = udl_drm_gem_mmap, .poll = drm_poll, .read = drm_read,
.unlocked_ioctl = drm_ioctl,.release = drm_release,
.unlocked_ioctl = udl_drm_ioctl,.release = udl_drm_release,#ifdef CONFIG_COMPAT .compat_ioctl = drm_compat_ioctl, #endif @@ -97,6 +97,7 @@ static void udl_usb_disconnect(struct usb_interface
*interface)
drm_connector_unplug_all(dev); udl_fbdev_unplug(dev); udl_drop_usb(dev);
udl_device_set_unplugged(dev); drm_unplug_dev(dev);}
diff --git a/drivers/gpu/drm/udl/udl_drv.h
b/drivers/gpu/drm/udl/udl_drv.h
index 4a064ef..75aa91c 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -65,6 +65,7 @@ struct udl_device { atomic_t bytes_identical; /* saved effort with backbuffer
comparison */
atomic_t bytes_sent; /* to usb, after compression includingoverhead */
atomic_t cpu_kcycles_used; /* transpired during pixel processing*/
atomic_t unplugged; /* device has been unplugged or gone away */};
struct udl_gem_object { @@ -133,6 +134,9 @@ int udl_gem_get_pages(struct udl_gem_object *obj); void udl_gem_put_pages(struct udl_gem_object *obj); int udl_gem_vmap(struct udl_gem_object *obj); void udl_gem_vunmap(struct udl_gem_object *obj); +int udl_drm_open(struct inode *inode, struct file *filp); +int udl_drm_release(struct inode *inode, struct file *filp); +int udl_drm_ioctl(struct file *filp, unsigned int cmd, unsigned long
arg);
int udl_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma); int udl_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf);
@@ -140,6 +144,8 @@ int udl_handle_damage(struct udl_framebuffer *fb,
int x, int y,
int width, int height);int udl_drop_usb(struct drm_device *dev); +void udl_device_set_unplugged(struct drm_device *dev); +int udl_device_is_unplugged(struct drm_device *dev);
#define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */ #define CMD_WRITE_RL8 "\xAF\x61" /**< 8 bit run length command. */ diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 200419d..df1d53e 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -325,7 +325,7 @@ static int udl_fb_open(struct fb_info *info, int
user)
struct udl_device *udl = dev->dev_private; /* If the USB device is gone, we don't accept new opens */
if (drm_device_is_unplugged(udl->ddev))
if (udl_device_is_unplugged(udl->ddev)) return -ENODEV; ufbdev->fb_count++;diff --git a/drivers/gpu/drm/udl/udl_gem.c
b/drivers/gpu/drm/udl/udl_gem.c
index 2a0a784..33c302f 100644 --- a/drivers/gpu/drm/udl/udl_gem.c +++ b/drivers/gpu/drm/udl/udl_gem.c @@ -86,8 +86,13 @@ int udl_dumb_create(struct drm_file *file,
int udl_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) {
struct drm_file *priv = filp->private_data;struct drm_device *dev = priv->minor->dev; int ret;if (udl_device_is_unplugged(dev))return -ENODEV;ret = drm_gem_mmap(filp, vma); if (ret) return ret;diff --git a/drivers/gpu/drm/udl/udl_main.c
b/drivers/gpu/drm/udl/udl_main.c
index 33dbfb2..2ba3ee3 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -350,3 +350,64 @@ int udl_driver_unload(struct drm_device *dev) kfree(udl); return 0; }
+int udl_drm_open(struct inode *inode, struct file *filp) +{
struct drm_file *file_priv;struct drm_minor *minor;int ret = drm_open(inode, filp);if (ret)return ret;file_priv = filp->private_data;minor = file_priv->minor;if (udl_device_is_unplugged(minor->dev)) {drm_dev_unref(minor->dev);return -ENODEV;}This doesn't make sense to me. You're called here with global-mutex held, so you can just check udl_device_is_unplugged() before calling into drm_open().
return 0;+}
+int udl_drm_release(struct inode *inode, struct file *filp) +{
struct drm_file *file_priv = filp->private_data;struct drm_device *dev = file_priv->minor->dev;int ret = drm_release(inode, filp);if (!dev->open_count && udl_device_is_unplugged(dev))drm_put_dev(dev);This should rather be:
drm_release(inode, filp); mutex_lock(&drm_global_mutex); if (!dev->open_count && udl_device_is_unplugged(dev)) drm_put_dev(dev); mutex_unlock(&drm_global_mutex); return 0;There is no reason to look at the return code of drm_release(), ever.
return ret;+}
+int udl_drm_ioctl(struct file *filp, unsigned int cmd, unsigned long
arg)
+{
struct drm_file *file_priv = filp->private_data;struct drm_device *dev = file_priv->minor->dev;unsigned int nr = DRM_IOCTL_NR(cmd);if (udl_device_is_unplugged(dev) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_SETCRTC) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_RMFB) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_DESTROY_DUMB))return -ENODEV;Why?
Just do:
if (udl_device_is_unplugged(dev)) return -ENODEV;Why this complex logic here?
Thanks David
return drm_ioctl(filp, cmd, arg);+}
+void udl_device_set_unplugged(struct drm_device *dev) +{
struct udl_device *udl = dev->dev_private;smp_wmb();atomic_set(&udl->unplugged, 1);+}
+int udl_device_is_unplugged(struct drm_device *dev) +{
struct udl_device *udl = dev->dev_private;int ret = atomic_read(&udl->unplugged);smp_rmb();return ret;+} diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d7162cf..40c6099 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,7 +748,6 @@ struct drm_device { struct drm_minor *control; /**< Control node */ struct drm_minor *primary; /**< Primary node */ struct drm_minor *render; /**< Render node */
atomic_t unplugged; /**< Flag whether dev isdead */
struct inode *anon_inode; /**< inode for privateaddress-space */
char *unique; /**< unique name of thedevice */
/*@} */@@ -879,19 +878,6 @@ static __inline__ int drm_core_check_feature(struct
drm_device *dev,
return ((dev->driver->driver_features & feature) ? 1 : 0);}
-static inline void drm_device_set_unplugged(struct drm_device *dev) -{
smp_wmb();atomic_set(&dev->unplugged, 1);-}
-static inline int drm_device_is_unplugged(struct drm_device *dev) -{
int ret = atomic_read(&dev->unplugged);smp_rmb();return ret;-}
static inline bool drm_is_render_client(const struct drm_file
*file_priv)
{ return file_priv->minor->type == DRM_MINOR_RENDER; -- 2.7.0.rc3.207.g0ac5344
Also note that it is not currently feasible for UDL code to access drm_global_mutex.
Please also refer to the similar comments ("FIXME: open_count is protected by drm_global_mutex but that would lead to locking inversion with the driver load path.") in nouveau, i915 and amdgpu.
On Wed, Feb 10, 2016 at 12:51 PM, Haixia Shi hshi@chromium.org wrote:
This should rather be:
drm_release(inode, filp); mutex_lock(&drm_global_mutex); if (!dev->open_count && udl_device_is_unplugged(dev)) drm_put_dev(dev); mutex_unlock(&drm_global_mutex); return 0;There is no reason to look at the return code of drm_release(), ever.
But drm_release() does return a retcode. It would still make sense to return that as-is in case any existing code relies on it.
On Wed, Feb 10, 2016 at 4:24 AM, David Herrmann dh.herrmann@gmail.com wrote:
Hi
On Tue, Feb 9, 2016 at 10:05 PM, Haixia Shi hshi@chromium.org wrote:
Remove the general drm_device_is_unplugged() checks, and move the
unplugged
flag handling logic into drm/udl. In general we want to keep
driver-specific
logic out of common drm code.
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org
drivers/gpu/drm/drm_drv.c | 6 ---- drivers/gpu/drm/drm_fops.c | 2 -- drivers/gpu/drm/drm_gem.c | 3 -- drivers/gpu/drm/drm_ioctl.c | 3 -- drivers/gpu/drm/drm_vm.c | 3 -- drivers/gpu/drm/udl/udl_connector.c | 2 +- drivers/gpu/drm/udl/udl_drv.c | 7 +++-- drivers/gpu/drm/udl/udl_drv.h | 6 ++++ drivers/gpu/drm/udl/udl_fb.c | 2 +- drivers/gpu/drm/udl/udl_gem.c | 5 +++ drivers/gpu/drm/udl/udl_main.c | 61
+++++++++++++++++++++++++++++++++++++
include/drm/drmP.h | 14 --------- 12 files changed, 78 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 167c8d3..f93ee12 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int
minor_id)
if (!minor) { return ERR_PTR(-ENODEV);
} else if (drm_device_is_unplugged(minor->dev)) {drm_dev_unref(minor->dev);return ERR_PTR(-ENODEV); } return minor;@@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev) drm_minor_unregister(dev, DRM_MINOR_CONTROL);
mutex_lock(&drm_global_mutex);
drm_device_set_unplugged(dev);if (dev->open_count == 0) { drm_put_dev(dev); }diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 1ea8790..b4332d4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file
*filp)
if (!--dev->open_count) { retcode = drm_lastclose(dev);
if (drm_device_is_unplugged(dev))drm_put_dev(dev); } mutex_unlock(&drm_global_mutex);diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2e8c77e..c622e32 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct
vm_area_struct *vma)
struct drm_vma_offset_node *node; int ret;
if (drm_device_is_unplugged(dev))return -ENODEV;drm_vma_offset_lock_lookup(dev->vma_offset_manager); node =drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
vma->vm_pgoff,diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 8ce2a0c..f959074 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
dev = file_priv->minor->dev;
if (drm_device_is_unplugged(dev))return -ENODEV;is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr <DRM_COMMAND_END;
if (is_driver_ioctl) {diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index f90bd5f..3a68be4 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct
vm_area_struct *vma)
struct drm_device *dev = priv->minor->dev; int ret;
if (drm_device_is_unplugged(dev))return -ENODEV;mutex_lock(&dev->struct_mutex); ret = drm_mmap_locked(filp, vma); mutex_unlock(&dev->struct_mutex);diff --git a/drivers/gpu/drm/udl/udl_connector.c
b/drivers/gpu/drm/udl/udl_connector.c
index 4709b54..b168f2c 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -96,7 +96,7 @@ static int udl_mode_valid(struct drm_connector
*connector,
static enum drm_connector_status udl_detect(struct drm_connector *connector, bool force) {
if (drm_device_is_unplugged(connector->dev))
if (udl_device_is_unplugged(connector->dev)) return connector_status_disconnected; return connector_status_connected;} diff --git a/drivers/gpu/drm/udl/udl_drv.c
b/drivers/gpu/drm/udl/udl_drv.c
index d5728ec..f5c2a97 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -24,12 +24,12 @@ static const struct vm_operations_struct
udl_gem_vm_ops = {
static const struct file_operations udl_driver_fops = { .owner = THIS_MODULE,
.open = drm_open,
.open = udl_drm_open, .mmap = udl_drm_gem_mmap, .poll = drm_poll, .read = drm_read,
.unlocked_ioctl = drm_ioctl,.release = drm_release,
.unlocked_ioctl = udl_drm_ioctl,.release = udl_drm_release,#ifdef CONFIG_COMPAT .compat_ioctl = drm_compat_ioctl, #endif @@ -97,6 +97,7 @@ static void udl_usb_disconnect(struct usb_interface
*interface)
drm_connector_unplug_all(dev); udl_fbdev_unplug(dev); udl_drop_usb(dev);
udl_device_set_unplugged(dev); drm_unplug_dev(dev);}
diff --git a/drivers/gpu/drm/udl/udl_drv.h
b/drivers/gpu/drm/udl/udl_drv.h
index 4a064ef..75aa91c 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -65,6 +65,7 @@ struct udl_device { atomic_t bytes_identical; /* saved effort with backbuffer
comparison */
atomic_t bytes_sent; /* to usb, after compression includingoverhead */
atomic_t cpu_kcycles_used; /* transpired during pixelprocessing */
atomic_t unplugged; /* device has been unplugged or gone away */};
struct udl_gem_object { @@ -133,6 +134,9 @@ int udl_gem_get_pages(struct udl_gem_object *obj); void udl_gem_put_pages(struct udl_gem_object *obj); int udl_gem_vmap(struct udl_gem_object *obj); void udl_gem_vunmap(struct udl_gem_object *obj); +int udl_drm_open(struct inode *inode, struct file *filp); +int udl_drm_release(struct inode *inode, struct file *filp); +int udl_drm_ioctl(struct file *filp, unsigned int cmd, unsigned long
arg);
int udl_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma); int udl_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf);
@@ -140,6 +144,8 @@ int udl_handle_damage(struct udl_framebuffer *fb,
int x, int y,
int width, int height);int udl_drop_usb(struct drm_device *dev); +void udl_device_set_unplugged(struct drm_device *dev); +int udl_device_is_unplugged(struct drm_device *dev);
#define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */ #define CMD_WRITE_RL8 "\xAF\x61" /**< 8 bit run length command. */ diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 200419d..df1d53e 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -325,7 +325,7 @@ static int udl_fb_open(struct fb_info *info, int
user)
struct udl_device *udl = dev->dev_private; /* If the USB device is gone, we don't accept new opens */
if (drm_device_is_unplugged(udl->ddev))
if (udl_device_is_unplugged(udl->ddev)) return -ENODEV; ufbdev->fb_count++;diff --git a/drivers/gpu/drm/udl/udl_gem.c
b/drivers/gpu/drm/udl/udl_gem.c
index 2a0a784..33c302f 100644 --- a/drivers/gpu/drm/udl/udl_gem.c +++ b/drivers/gpu/drm/udl/udl_gem.c @@ -86,8 +86,13 @@ int udl_dumb_create(struct drm_file *file,
int udl_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) {
struct drm_file *priv = filp->private_data;struct drm_device *dev = priv->minor->dev; int ret;if (udl_device_is_unplugged(dev))return -ENODEV;ret = drm_gem_mmap(filp, vma); if (ret) return ret;diff --git a/drivers/gpu/drm/udl/udl_main.c
b/drivers/gpu/drm/udl/udl_main.c
index 33dbfb2..2ba3ee3 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -350,3 +350,64 @@ int udl_driver_unload(struct drm_device *dev) kfree(udl); return 0; }
+int udl_drm_open(struct inode *inode, struct file *filp) +{
struct drm_file *file_priv;struct drm_minor *minor;int ret = drm_open(inode, filp);if (ret)return ret;file_priv = filp->private_data;minor = file_priv->minor;if (udl_device_is_unplugged(minor->dev)) {drm_dev_unref(minor->dev);return -ENODEV;}This doesn't make sense to me. You're called here with global-mutex held, so you can just check udl_device_is_unplugged() before calling into drm_open().
return 0;+}
+int udl_drm_release(struct inode *inode, struct file *filp) +{
struct drm_file *file_priv = filp->private_data;struct drm_device *dev = file_priv->minor->dev;int ret = drm_release(inode, filp);if (!dev->open_count && udl_device_is_unplugged(dev))drm_put_dev(dev);This should rather be:
drm_release(inode, filp); mutex_lock(&drm_global_mutex); if (!dev->open_count && udl_device_is_unplugged(dev)) drm_put_dev(dev); mutex_unlock(&drm_global_mutex); return 0;There is no reason to look at the return code of drm_release(), ever.
return ret;+}
+int udl_drm_ioctl(struct file *filp, unsigned int cmd, unsigned long
arg)
+{
struct drm_file *file_priv = filp->private_data;struct drm_device *dev = file_priv->minor->dev;unsigned int nr = DRM_IOCTL_NR(cmd);if (udl_device_is_unplugged(dev) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_SETCRTC) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_RMFB) &&nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_DESTROY_DUMB))return -ENODEV;Why?
Just do:
if (udl_device_is_unplugged(dev)) return -ENODEV;Why this complex logic here?
Thanks David
return drm_ioctl(filp, cmd, arg);+}
+void udl_device_set_unplugged(struct drm_device *dev) +{
struct udl_device *udl = dev->dev_private;smp_wmb();atomic_set(&udl->unplugged, 1);+}
+int udl_device_is_unplugged(struct drm_device *dev) +{
struct udl_device *udl = dev->dev_private;int ret = atomic_read(&udl->unplugged);smp_rmb();return ret;+} diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d7162cf..40c6099 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,7 +748,6 @@ struct drm_device { struct drm_minor *control; /**< Control node */ struct drm_minor *primary; /**< Primary node */ struct drm_minor *render; /**< Render node */
atomic_t unplugged; /**< Flag whether devis dead */
struct inode *anon_inode; /**< inode for privateaddress-space */
char *unique; /**< unique name of thedevice */
/*@} */@@ -879,19 +878,6 @@ static __inline__ int
drm_core_check_feature(struct drm_device *dev,
return ((dev->driver->driver_features & feature) ? 1 : 0);}
-static inline void drm_device_set_unplugged(struct drm_device *dev) -{
smp_wmb();atomic_set(&dev->unplugged, 1);-}
-static inline int drm_device_is_unplugged(struct drm_device *dev) -{
int ret = atomic_read(&dev->unplugged);smp_rmb();return ret;-}
static inline bool drm_is_render_client(const struct drm_file
*file_priv)
{ return file_priv->minor->type == DRM_MINOR_RENDER; -- 2.7.0.rc3.207.g0ac5344
Hi
On Wed, Feb 10, 2016 at 10:00 PM, Haixia Shi hshi@chromium.org wrote:
Also note that it is not currently feasible for UDL code to access drm_global_mutex.
Then change this.
Please also refer to the similar comments ("FIXME: open_count is protected by drm_global_mutex but that would lead to locking inversion with the driver load path.") in nouveau, i915 and amdgpu.
This is unrelated to your change. I don't see any locking inversion in your patch.
Thanks David
Hi
On Wed, Feb 10, 2016 at 9:51 PM, Haixia Shi hshi@chromium.org wrote:
This should rather be:
drm_release(inode, filp); mutex_lock(&drm_global_mutex); if (!dev->open_count && udl_device_is_unplugged(dev)) drm_put_dev(dev); mutex_unlock(&drm_global_mutex); return 0;There is no reason to look at the return code of drm_release(), ever.
But drm_release() does return a retcode. It would still make sense to return that as-is in case any existing code relies on it.
Nobody should ever return error codes from fops.release(). It is completely bogus. You rather confuse generic user-space that calls close(), than getting any benefit out of it.
But TBH, I don't care. Feel free to forward the return value. But still, please change the order of the calls as I did.
Thanks David
David
I am having trouble getting the reference to "drm_global_mutex" to link correctly in drm/udl. The error is
ERROR: "drm_global_mutex" [drivers/gpu/drm/udl/udl.ko] undefined! make[1]: *** [__modpost] Error 1 make: *** [modules] Error 2
This seems to be only accessed in the common drm code. Do you have a suggestion how to get around it?
On Wed, Feb 10, 2016 at 1:35 PM, David Herrmann dh.herrmann@gmail.com wrote:
Hi
On Wed, Feb 10, 2016 at 9:51 PM, Haixia Shi hshi@chromium.org wrote:
This should rather be:
drm_release(inode, filp); mutex_lock(&drm_global_mutex); if (!dev->open_count && udl_device_is_unplugged(dev)) drm_put_dev(dev); mutex_unlock(&drm_global_mutex); return 0;There is no reason to look at the return code of drm_release(), ever.
But drm_release() does return a retcode. It would still make sense to
return
that as-is in case any existing code relies on it.
Nobody should ever return error codes from fops.release(). It is completely bogus. You rather confuse generic user-space that calls close(), than getting any benefit out of it.
But TBH, I don't care. Feel free to forward the return value. But still, please change the order of the calls as I did.
Thanks David
Hi
On Wed, Feb 10, 2016 at 10:38 PM, Haixia Shi hshi@chromium.org wrote:
David
I am having trouble getting the reference to "drm_global_mutex" to link correctly in drm/udl. The error is
ERROR: "drm_global_mutex" [drivers/gpu/drm/udl/udl.ko] undefined! make[1]: *** [__modpost] Error 1 make: *** [modules] Error 2
This seems to be only accessed in the common drm code. Do you have a suggestion how to get around it?
You need to add: EXPORT_SYMBOL(drm_global_mutex);
Thanks David
When USB cable is disconnected, we mark udl device as unplugged so that udl_detect reports connector status as disconnected, but still keep the drm device alive until user-space closes it.
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org --- drivers/gpu/drm/drm_drv.c | 6 ------ drivers/gpu/drm/drm_fops.c | 2 -- drivers/gpu/drm/drm_gem.c | 3 --- drivers/gpu/drm/drm_ioctl.c | 3 --- drivers/gpu/drm/drm_vm.c | 3 --- drivers/gpu/drm/udl/udl_connector.c | 2 +- drivers/gpu/drm/udl/udl_drv.c | 1 + drivers/gpu/drm/udl/udl_drv.h | 3 +++ drivers/gpu/drm/udl/udl_fb.c | 2 +- drivers/gpu/drm/udl/udl_main.c | 15 +++++++++++++++ include/drm/drmP.h | 14 -------------- 11 files changed, 21 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 167c8d3..f93ee12 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)
if (!minor) { return ERR_PTR(-ENODEV); - } else if (drm_device_is_unplugged(minor->dev)) { - drm_dev_unref(minor->dev); - return ERR_PTR(-ENODEV); }
return minor; @@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev) drm_minor_unregister(dev, DRM_MINOR_CONTROL);
mutex_lock(&drm_global_mutex); - - drm_device_set_unplugged(dev); - if (dev->open_count == 0) { drm_put_dev(dev); } diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 1ea8790..b4332d4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file *filp)
if (!--dev->open_count) { retcode = drm_lastclose(dev); - if (drm_device_is_unplugged(dev)) - drm_put_dev(dev); } mutex_unlock(&drm_global_mutex);
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2e8c77e..c622e32 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_vma_offset_node *node; int ret;
- if (drm_device_is_unplugged(dev)) - return -ENODEV; - drm_vma_offset_lock_lookup(dev->vma_offset_manager); node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager, vma->vm_pgoff, diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 8ce2a0c..f959074 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
dev = file_priv->minor->dev;
- if (drm_device_is_unplugged(dev)) - return -ENODEV; - is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
if (is_driver_ioctl) { diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index f90bd5f..3a68be4 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_device *dev = priv->minor->dev; int ret;
- if (drm_device_is_unplugged(dev)) - return -ENODEV; - mutex_lock(&dev->struct_mutex); ret = drm_mmap_locked(filp, vma); mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c index 4709b54..b168f2c 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -96,7 +96,7 @@ static int udl_mode_valid(struct drm_connector *connector, static enum drm_connector_status udl_detect(struct drm_connector *connector, bool force) { - if (drm_device_is_unplugged(connector->dev)) + if (udl_device_is_unplugged(connector->dev)) return connector_status_disconnected; return connector_status_connected; } diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index d5728ec..3cbafe7 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -97,6 +97,7 @@ static void udl_usb_disconnect(struct usb_interface *interface) drm_connector_unplug_all(dev); udl_fbdev_unplug(dev); udl_drop_usb(dev); + udl_device_set_unplugged(dev); drm_unplug_dev(dev); }
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index 4a064ef..61e117a 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -65,6 +65,7 @@ struct udl_device { atomic_t bytes_identical; /* saved effort with backbuffer comparison */ atomic_t bytes_sent; /* to usb, after compression including overhead */ atomic_t cpu_kcycles_used; /* transpired during pixel processing */ + atomic_t unplugged; /* device has been unplugged or gone away */ };
struct udl_gem_object { @@ -140,6 +141,8 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y, int width, int height);
int udl_drop_usb(struct drm_device *dev); +void udl_device_set_unplugged(struct drm_device *dev); +int udl_device_is_unplugged(struct drm_device *dev);
#define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */ #define CMD_WRITE_RL8 "\xAF\x61" /**< 8 bit run length command. */ diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 200419d..df1d53e 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -325,7 +325,7 @@ static int udl_fb_open(struct fb_info *info, int user) struct udl_device *udl = dev->dev_private;
/* If the USB device is gone, we don't accept new opens */ - if (drm_device_is_unplugged(udl->ddev)) + if (udl_device_is_unplugged(udl->ddev)) return -ENODEV;
ufbdev->fb_count++; diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index 33dbfb2..f6028a4 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -350,3 +350,18 @@ int udl_driver_unload(struct drm_device *dev) kfree(udl); return 0; } + +void udl_device_set_unplugged(struct drm_device *dev) +{ + struct udl_device *udl = dev->dev_private; + smp_wmb(); + atomic_set(&udl->unplugged, 1); +} + +int udl_device_is_unplugged(struct drm_device *dev) +{ + struct udl_device *udl = dev->dev_private; + int ret = atomic_read(&udl->unplugged); + smp_rmb(); + return ret; +} diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d7162cf..40c6099 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,7 +748,6 @@ struct drm_device { struct drm_minor *control; /**< Control node */ struct drm_minor *primary; /**< Primary node */ struct drm_minor *render; /**< Render node */ - atomic_t unplugged; /**< Flag whether dev is dead */ struct inode *anon_inode; /**< inode for private address-space */ char *unique; /**< unique name of the device */ /*@} */ @@ -879,19 +878,6 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev, return ((dev->driver->driver_features & feature) ? 1 : 0); }
-static inline void drm_device_set_unplugged(struct drm_device *dev) -{ - smp_wmb(); - atomic_set(&dev->unplugged, 1); -} - -static inline int drm_device_is_unplugged(struct drm_device *dev) -{ - int ret = atomic_read(&dev->unplugged); - smp_rmb(); - return ret; -} - static inline bool drm_is_render_client(const struct drm_file *file_priv) { return file_priv->minor->type == DRM_MINOR_RENDER;
On Wed, Feb 10, 2016 at 03:18:38PM -0800, Haixia Shi wrote:
When USB cable is disconnected, we mark udl device as unplugged so that udl_detect reports connector status as disconnected, but still keep the drm device alive until user-space closes it.
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org
Please add more details capturing the discussion, i.e. why exactly this was done. Also, has Stephane really re-reviewed this new version already? And please Cc: everyone involved in the previous patch discussion, too.
Thanks, Daniel
drivers/gpu/drm/drm_drv.c | 6 ------ drivers/gpu/drm/drm_fops.c | 2 -- drivers/gpu/drm/drm_gem.c | 3 --- drivers/gpu/drm/drm_ioctl.c | 3 --- drivers/gpu/drm/drm_vm.c | 3 --- drivers/gpu/drm/udl/udl_connector.c | 2 +- drivers/gpu/drm/udl/udl_drv.c | 1 + drivers/gpu/drm/udl/udl_drv.h | 3 +++ drivers/gpu/drm/udl/udl_fb.c | 2 +- drivers/gpu/drm/udl/udl_main.c | 15 +++++++++++++++ include/drm/drmP.h | 14 -------------- 11 files changed, 21 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 167c8d3..f93ee12 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)
if (!minor) { return ERR_PTR(-ENODEV);
} else if (drm_device_is_unplugged(minor->dev)) {
drm_dev_unref(minor->dev);return ERR_PTR(-ENODEV);}
return minor;
@@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev) drm_minor_unregister(dev, DRM_MINOR_CONTROL);
mutex_lock(&drm_global_mutex);
- drm_device_set_unplugged(dev);
- if (dev->open_count == 0) { drm_put_dev(dev); }
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 1ea8790..b4332d4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file *filp)
if (!--dev->open_count) { retcode = drm_lastclose(dev);
if (drm_device_is_unplugged(dev))drm_put_dev(dev);diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2e8c77e..c622e32 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_vma_offset_node *node; int ret;
- if (drm_device_is_unplugged(dev))
return -ENODEV;- drm_vma_offset_lock_lookup(dev->vma_offset_manager); node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager, vma->vm_pgoff,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 8ce2a0c..f959074 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
dev = file_priv->minor->dev;
if (drm_device_is_unplugged(dev))
return -ENODEV;is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
if (is_driver_ioctl) {
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index f90bd5f..3a68be4 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_device *dev = priv->minor->dev; int ret;
- if (drm_device_is_unplugged(dev))
return -ENODEV;- mutex_lock(&dev->struct_mutex); ret = drm_mmap_locked(filp, vma); mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c index 4709b54..b168f2c 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -96,7 +96,7 @@ static int udl_mode_valid(struct drm_connector *connector, static enum drm_connector_status udl_detect(struct drm_connector *connector, bool force) {
- if (drm_device_is_unplugged(connector->dev))
- if (udl_device_is_unplugged(connector->dev)) return connector_status_disconnected; return connector_status_connected;
} diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index d5728ec..3cbafe7 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -97,6 +97,7 @@ static void udl_usb_disconnect(struct usb_interface *interface) drm_connector_unplug_all(dev); udl_fbdev_unplug(dev); udl_drop_usb(dev);
- udl_device_set_unplugged(dev); drm_unplug_dev(dev);
}
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index 4a064ef..61e117a 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -65,6 +65,7 @@ struct udl_device { atomic_t bytes_identical; /* saved effort with backbuffer comparison */ atomic_t bytes_sent; /* to usb, after compression including overhead */ atomic_t cpu_kcycles_used; /* transpired during pixel processing */
- atomic_t unplugged; /* device has been unplugged or gone away */
};
struct udl_gem_object { @@ -140,6 +141,8 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y, int width, int height);
int udl_drop_usb(struct drm_device *dev); +void udl_device_set_unplugged(struct drm_device *dev); +int udl_device_is_unplugged(struct drm_device *dev);
#define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */ #define CMD_WRITE_RL8 "\xAF\x61" /**< 8 bit run length command. */ diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 200419d..df1d53e 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -325,7 +325,7 @@ static int udl_fb_open(struct fb_info *info, int user) struct udl_device *udl = dev->dev_private;
/* If the USB device is gone, we don't accept new opens */
- if (drm_device_is_unplugged(udl->ddev))
if (udl_device_is_unplugged(udl->ddev)) return -ENODEV;
ufbdev->fb_count++;
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index 33dbfb2..f6028a4 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -350,3 +350,18 @@ int udl_driver_unload(struct drm_device *dev) kfree(udl); return 0; }
+void udl_device_set_unplugged(struct drm_device *dev) +{
- struct udl_device *udl = dev->dev_private;
- smp_wmb();
- atomic_set(&udl->unplugged, 1);
+}
+int udl_device_is_unplugged(struct drm_device *dev) +{
- struct udl_device *udl = dev->dev_private;
- int ret = atomic_read(&udl->unplugged);
- smp_rmb();
- return ret;
+} diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d7162cf..40c6099 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,7 +748,6 @@ struct drm_device { struct drm_minor *control; /**< Control node */ struct drm_minor *primary; /**< Primary node */ struct drm_minor *render; /**< Render node */
- atomic_t unplugged; /**< Flag whether dev is dead */ struct inode *anon_inode; /**< inode for private address-space */ char *unique; /**< unique name of the device */ /*@} */
@@ -879,19 +878,6 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev, return ((dev->driver->driver_features & feature) ? 1 : 0); }
-static inline void drm_device_set_unplugged(struct drm_device *dev) -{
- smp_wmb();
- atomic_set(&dev->unplugged, 1);
-}
-static inline int drm_device_is_unplugged(struct drm_device *dev) -{
- int ret = atomic_read(&dev->unplugged);
- smp_rmb();
- return ret;
-}
static inline bool drm_is_render_client(const struct drm_file *file_priv) { return file_priv->minor->type == DRM_MINOR_RENDER; -- 2.7.0.rc3.207.g0ac5344
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi
On Thu, Feb 11, 2016 at 12:18 AM, Haixia Shi hshi@chromium.org wrote:
When USB cable is disconnected, we mark udl device as unplugged so that udl_detect reports connector status as disconnected, but still keep the drm device alive until user-space closes it.
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org
I assume this is based on the discussion I had with Stephane on IRC. I'm fine with going this way and keeping the device fully alive, and just treat the device removal as a connector-unplug. However, you really must document all this in your commit message.
Anyway, if you want to keep the device alive, then please change the code to entirely drop the "unplugged" flag and all related code. Then make sure you somehow reset "udl->udev" to NULL and make sure no code-path uses the usb-device after it was unplugged. I guess there should be appropriate locks already.
This way, you end up with a fully functional UDL device, which just happens to have to connector plugged. But you *really* have to be careful that no-one touches the usb device, as the interface might be re-used by some other device any time (or in case the usb-core provides protection against this, please document it).
Thanks David
Well, in that case it would be even simpler. We don't even need the "unplugged" flag check in udl_detect because all connectors are already unplugged in udl_usb_disconnect (in drm_connector_unplug_all()).
It is not necessary to check the flag in udl_fb_open() either, if the intention is to keep the device alive.
As for code paths that uses udl->udev, I only see the following 3 places:
(1) udl_parse_vendor_description and udl_alloc_urb_list: both are only called at udl_driver_load(), so not a problem (2) udl_usb_probe: won't happen after USB device is unplugged (3) udl_get_edid: only called from udl_get_modes, won't be an issue because connectors are already unplugged
So, it seems that I *really* just need to drop the "unplugged" flag and update the commit message.
On Thu, Feb 11, 2016 at 2:30 AM, David Herrmann dh.herrmann@gmail.com wrote:
Hi
On Thu, Feb 11, 2016 at 12:18 AM, Haixia Shi hshi@chromium.org wrote:
When USB cable is disconnected, we mark udl device as unplugged so that udl_detect reports connector status as disconnected, but still keep the drm device alive until user-space closes it.
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org
I assume this is based on the discussion I had with Stephane on IRC. I'm fine with going this way and keeping the device fully alive, and just treat the device removal as a connector-unplug. However, you really must document all this in your commit message.
Anyway, if you want to keep the device alive, then please change the code to entirely drop the "unplugged" flag and all related code. Then make sure you somehow reset "udl->udev" to NULL and make sure no code-path uses the usb-device after it was unplugged. I guess there should be appropriate locks already.
This way, you end up with a fully functional UDL device, which just happens to have to connector plugged. But you *really* have to be careful that no-one touches the usb device, as the interface might be re-used by some other device any time (or in case the usb-core provides protection against this, please document it).
Thanks David
When a UDL monitor is unplugged, we need to still treat it as a fully functional device which just happens to have its connector unplugged. This allows user-space to properly deallocate fbs and dumb buffers before closing the device.
This drops the "unplugged" flag hack, which puts the device in a non-functional state after USB unplug and rejects most operations on the device such as ioctls with error -ENODEV.
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org Cc: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_drv.c | 6 ------ drivers/gpu/drm/drm_fops.c | 2 -- drivers/gpu/drm/drm_gem.c | 3 --- drivers/gpu/drm/drm_ioctl.c | 3 --- drivers/gpu/drm/drm_vm.c | 3 --- drivers/gpu/drm/udl/udl_connector.c | 2 -- drivers/gpu/drm/udl/udl_fb.c | 6 ------ include/drm/drmP.h | 14 -------------- 8 files changed, 39 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 167c8d3..f93ee12 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)
if (!minor) { return ERR_PTR(-ENODEV); - } else if (drm_device_is_unplugged(minor->dev)) { - drm_dev_unref(minor->dev); - return ERR_PTR(-ENODEV); }
return minor; @@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev) drm_minor_unregister(dev, DRM_MINOR_CONTROL);
mutex_lock(&drm_global_mutex); - - drm_device_set_unplugged(dev); - if (dev->open_count == 0) { drm_put_dev(dev); } diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 1ea8790..b4332d4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file *filp)
if (!--dev->open_count) { retcode = drm_lastclose(dev); - if (drm_device_is_unplugged(dev)) - drm_put_dev(dev); } mutex_unlock(&drm_global_mutex);
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2e8c77e..c622e32 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_vma_offset_node *node; int ret;
- if (drm_device_is_unplugged(dev)) - return -ENODEV; - drm_vma_offset_lock_lookup(dev->vma_offset_manager); node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager, vma->vm_pgoff, diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 8ce2a0c..f959074 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
dev = file_priv->minor->dev;
- if (drm_device_is_unplugged(dev)) - return -ENODEV; - is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
if (is_driver_ioctl) { diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index f90bd5f..3a68be4 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_device *dev = priv->minor->dev; int ret;
- if (drm_device_is_unplugged(dev)) - return -ENODEV; - mutex_lock(&dev->struct_mutex); ret = drm_mmap_locked(filp, vma); mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c index 4709b54..a6d5e21 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -96,8 +96,6 @@ static int udl_mode_valid(struct drm_connector *connector, static enum drm_connector_status udl_detect(struct drm_connector *connector, bool force) { - if (drm_device_is_unplugged(connector->dev)) - return connector_status_disconnected; return connector_status_connected; }
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 200419d..29aca6c 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -321,12 +321,6 @@ static void udl_fb_imageblit(struct fb_info *info, const struct fb_image *image) static int udl_fb_open(struct fb_info *info, int user) { struct udl_fbdev *ufbdev = info->par; - struct drm_device *dev = ufbdev->ufb.base.dev; - struct udl_device *udl = dev->dev_private; - - /* If the USB device is gone, we don't accept new opens */ - if (drm_device_is_unplugged(udl->ddev)) - return -ENODEV;
ufbdev->fb_count++;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d7162cf..40c6099 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,7 +748,6 @@ struct drm_device { struct drm_minor *control; /**< Control node */ struct drm_minor *primary; /**< Primary node */ struct drm_minor *render; /**< Render node */ - atomic_t unplugged; /**< Flag whether dev is dead */ struct inode *anon_inode; /**< inode for private address-space */ char *unique; /**< unique name of the device */ /*@} */ @@ -879,19 +878,6 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev, return ((dev->driver->driver_features & feature) ? 1 : 0); }
-static inline void drm_device_set_unplugged(struct drm_device *dev) -{ - smp_wmb(); - atomic_set(&dev->unplugged, 1); -} - -static inline int drm_device_is_unplugged(struct drm_device *dev) -{ - int ret = atomic_read(&dev->unplugged); - smp_rmb(); - return ret; -} - static inline bool drm_is_render_client(const struct drm_file *file_priv) { return file_priv->minor->type == DRM_MINOR_RENDER;
On Thu, Feb 11, 2016 at 01:57:30PM -0800, Haixia Shi wrote:
When a UDL monitor is unplugged, we need to still treat it as a fully functional device which just happens to have its connector unplugged. This allows user-space to properly deallocate fbs and dumb buffers before closing the device.
This drops the "unplugged" flag hack, which puts the device in a non-functional state after USB unplug and rejects most operations on the device such as ioctls with error -ENODEV.
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org Cc: David Herrmann dh.herrmann@gmail.com
Still missing the in-patch changelog. Please add something like
v2:
.... changes done for v2, and who suggested it
v3:
... changes done for v3.
Thanks, Daniel
drivers/gpu/drm/drm_drv.c | 6 ------ drivers/gpu/drm/drm_fops.c | 2 -- drivers/gpu/drm/drm_gem.c | 3 --- drivers/gpu/drm/drm_ioctl.c | 3 --- drivers/gpu/drm/drm_vm.c | 3 --- drivers/gpu/drm/udl/udl_connector.c | 2 -- drivers/gpu/drm/udl/udl_fb.c | 6 ------ include/drm/drmP.h | 14 -------------- 8 files changed, 39 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 167c8d3..f93ee12 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)
if (!minor) { return ERR_PTR(-ENODEV);
} else if (drm_device_is_unplugged(minor->dev)) {
drm_dev_unref(minor->dev);return ERR_PTR(-ENODEV);}
return minor;
@@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev) drm_minor_unregister(dev, DRM_MINOR_CONTROL);
mutex_lock(&drm_global_mutex);
- drm_device_set_unplugged(dev);
- if (dev->open_count == 0) { drm_put_dev(dev); }
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 1ea8790..b4332d4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file *filp)
if (!--dev->open_count) { retcode = drm_lastclose(dev);
if (drm_device_is_unplugged(dev))drm_put_dev(dev);diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2e8c77e..c622e32 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_vma_offset_node *node; int ret;
- if (drm_device_is_unplugged(dev))
return -ENODEV;- drm_vma_offset_lock_lookup(dev->vma_offset_manager); node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager, vma->vm_pgoff,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 8ce2a0c..f959074 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
dev = file_priv->minor->dev;
if (drm_device_is_unplugged(dev))
return -ENODEV;is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
if (is_driver_ioctl) {
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index f90bd5f..3a68be4 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_device *dev = priv->minor->dev; int ret;
- if (drm_device_is_unplugged(dev))
return -ENODEV;- mutex_lock(&dev->struct_mutex); ret = drm_mmap_locked(filp, vma); mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c index 4709b54..a6d5e21 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -96,8 +96,6 @@ static int udl_mode_valid(struct drm_connector *connector, static enum drm_connector_status udl_detect(struct drm_connector *connector, bool force) {
- if (drm_device_is_unplugged(connector->dev))
return connector_status_disconnected;}
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 200419d..29aca6c 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -321,12 +321,6 @@ static void udl_fb_imageblit(struct fb_info *info, const struct fb_image *image) static int udl_fb_open(struct fb_info *info, int user) { struct udl_fbdev *ufbdev = info->par;
struct drm_device *dev = ufbdev->ufb.base.dev;
struct udl_device *udl = dev->dev_private;
/* If the USB device is gone, we don't accept new opens */
if (drm_device_is_unplugged(udl->ddev))
return -ENODEV;ufbdev->fb_count++;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d7162cf..40c6099 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,7 +748,6 @@ struct drm_device { struct drm_minor *control; /**< Control node */ struct drm_minor *primary; /**< Primary node */ struct drm_minor *render; /**< Render node */
- atomic_t unplugged; /**< Flag whether dev is dead */ struct inode *anon_inode; /**< inode for private address-space */ char *unique; /**< unique name of the device */ /*@} */
@@ -879,19 +878,6 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev, return ((dev->driver->driver_features & feature) ? 1 : 0); }
-static inline void drm_device_set_unplugged(struct drm_device *dev) -{
- smp_wmb();
- atomic_set(&dev->unplugged, 1);
-}
-static inline int drm_device_is_unplugged(struct drm_device *dev) -{
- int ret = atomic_read(&dev->unplugged);
- smp_rmb();
- return ret;
-}
static inline bool drm_is_render_client(const struct drm_file *file_priv) { return file_priv->minor->type == DRM_MINOR_RENDER; -- 2.7.0.rc3.207.g0ac5344
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
v1: Remove the general drm_device_is_unplugged() checks, and move the unplugged flag handling logic into drm/udl. In general we want to keep driver-specific logic out of common drm code.
v2: Based on discussion with Stephane and David, drop most of the unplugged flag handling logic in drm/udl except for udl_detect() and udl_fb_open(). The intention is to treat the device removal as a connector-unplug, and kep the UDL device fully functional.
v3: Based on feedback from David, entirely drop the "unplugged" flag and all related code. There is no need to check for the unplugged flag as the existing udl_usb_disconnect() behavior already ensures the controller is removed, and all code paths that uses the usb-device are not reachable after unplug.
When a UDL monitor is unplugged, we need to still treat it as a fully functional device which just happens to have its connector unplugged. This allows user-space to properly deallocate fbs and dumb buffers before closing the device.
This drops the "unplugged" flag hack, which puts the device in a non-functional state after USB unplug and rejects most operations on the device such as ioctls with error -ENODEV.
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org Cc: David Herrmann dh.herrmann@gmail.com Cc: Daniel Vetter daniel@ffwll.ch --- drivers/gpu/drm/drm_drv.c | 6 ------ drivers/gpu/drm/drm_fops.c | 2 -- drivers/gpu/drm/drm_gem.c | 3 --- drivers/gpu/drm/drm_ioctl.c | 3 --- drivers/gpu/drm/drm_vm.c | 3 --- drivers/gpu/drm/udl/udl_connector.c | 2 -- drivers/gpu/drm/udl/udl_fb.c | 6 ------ include/drm/drmP.h | 14 -------------- 8 files changed, 39 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 167c8d3..f93ee12 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)
if (!minor) { return ERR_PTR(-ENODEV); - } else if (drm_device_is_unplugged(minor->dev)) { - drm_dev_unref(minor->dev); - return ERR_PTR(-ENODEV); }
return minor; @@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev) drm_minor_unregister(dev, DRM_MINOR_CONTROL);
mutex_lock(&drm_global_mutex); - - drm_device_set_unplugged(dev); - if (dev->open_count == 0) { drm_put_dev(dev); } diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 1ea8790..b4332d4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file *filp)
if (!--dev->open_count) { retcode = drm_lastclose(dev); - if (drm_device_is_unplugged(dev)) - drm_put_dev(dev); } mutex_unlock(&drm_global_mutex);
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2e8c77e..c622e32 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_vma_offset_node *node; int ret;
- if (drm_device_is_unplugged(dev)) - return -ENODEV; - drm_vma_offset_lock_lookup(dev->vma_offset_manager); node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager, vma->vm_pgoff, diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 8ce2a0c..f959074 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
dev = file_priv->minor->dev;
- if (drm_device_is_unplugged(dev)) - return -ENODEV; - is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
if (is_driver_ioctl) { diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index f90bd5f..3a68be4 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_device *dev = priv->minor->dev; int ret;
- if (drm_device_is_unplugged(dev)) - return -ENODEV; - mutex_lock(&dev->struct_mutex); ret = drm_mmap_locked(filp, vma); mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c index 4709b54..a6d5e21 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -96,8 +96,6 @@ static int udl_mode_valid(struct drm_connector *connector, static enum drm_connector_status udl_detect(struct drm_connector *connector, bool force) { - if (drm_device_is_unplugged(connector->dev)) - return connector_status_disconnected; return connector_status_connected; }
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 200419d..29aca6c 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -321,12 +321,6 @@ static void udl_fb_imageblit(struct fb_info *info, const struct fb_image *image) static int udl_fb_open(struct fb_info *info, int user) { struct udl_fbdev *ufbdev = info->par; - struct drm_device *dev = ufbdev->ufb.base.dev; - struct udl_device *udl = dev->dev_private; - - /* If the USB device is gone, we don't accept new opens */ - if (drm_device_is_unplugged(udl->ddev)) - return -ENODEV;
ufbdev->fb_count++;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d7162cf..40c6099 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,7 +748,6 @@ struct drm_device { struct drm_minor *control; /**< Control node */ struct drm_minor *primary; /**< Primary node */ struct drm_minor *render; /**< Render node */ - atomic_t unplugged; /**< Flag whether dev is dead */ struct inode *anon_inode; /**< inode for private address-space */ char *unique; /**< unique name of the device */ /*@} */ @@ -879,19 +878,6 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev, return ((dev->driver->driver_features & feature) ? 1 : 0); }
-static inline void drm_device_set_unplugged(struct drm_device *dev) -{ - smp_wmb(); - atomic_set(&dev->unplugged, 1); -} - -static inline int drm_device_is_unplugged(struct drm_device *dev) -{ - int ret = atomic_read(&dev->unplugged); - smp_rmb(); - return ret; -} - static inline bool drm_is_render_client(const struct drm_file *file_priv) { return file_priv->minor->type == DRM_MINOR_RENDER;
David - is the v3 patch acceptable to you? Thanks.
On Fri, Feb 12, 2016 at 11:50 AM, Haixia Shi hshi@chromium.org wrote:
v1: Remove the general drm_device_is_unplugged() checks, and move the unplugged flag handling logic into drm/udl. In general we want to keep driver-specific logic out of common drm code.
v2: Based on discussion with Stephane and David, drop most of the unplugged flag handling logic in drm/udl except for udl_detect() and udl_fb_open(). The intention is to treat the device removal as a connector-unplug, and kep the UDL device fully functional.
v3: Based on feedback from David, entirely drop the "unplugged" flag and all related code. There is no need to check for the unplugged flag as the existing udl_usb_disconnect() behavior already ensures the controller is removed, and all code paths that uses the usb-device are not reachable after unplug.
When a UDL monitor is unplugged, we need to still treat it as a fully functional device which just happens to have its connector unplugged. This allows user-space to properly deallocate fbs and dumb buffers before closing the device.
This drops the "unplugged" flag hack, which puts the device in a non-functional state after USB unplug and rejects most operations on the device such as ioctls with error -ENODEV.
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org Cc: David Herrmann dh.herrmann@gmail.com Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_drv.c | 6 ------ drivers/gpu/drm/drm_fops.c | 2 -- drivers/gpu/drm/drm_gem.c | 3 --- drivers/gpu/drm/drm_ioctl.c | 3 --- drivers/gpu/drm/drm_vm.c | 3 --- drivers/gpu/drm/udl/udl_connector.c | 2 -- drivers/gpu/drm/udl/udl_fb.c | 6 ------ include/drm/drmP.h | 14 -------------- 8 files changed, 39 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 167c8d3..f93ee12 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)
if (!minor) { return ERR_PTR(-ENODEV);
} else if (drm_device_is_unplugged(minor->dev)) {drm_dev_unref(minor->dev);return ERR_PTR(-ENODEV); } return minor;@@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev) drm_minor_unregister(dev, DRM_MINOR_CONTROL);
mutex_lock(&drm_global_mutex);
drm_device_set_unplugged(dev);if (dev->open_count == 0) { drm_put_dev(dev); }diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 1ea8790..b4332d4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file *filp)
if (!--dev->open_count) { retcode = drm_lastclose(dev);
if (drm_device_is_unplugged(dev))drm_put_dev(dev); } mutex_unlock(&drm_global_mutex);diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2e8c77e..c622e32 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_vma_offset_node *node; int ret;
if (drm_device_is_unplugged(dev))return -ENODEV;drm_vma_offset_lock_lookup(dev->vma_offset_manager); node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager, vma->vm_pgoff,diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 8ce2a0c..f959074 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
dev = file_priv->minor->dev;
if (drm_device_is_unplugged(dev))return -ENODEV;is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END; if (is_driver_ioctl) {diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index f90bd5f..3a68be4 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_device *dev = priv->minor->dev; int ret;
if (drm_device_is_unplugged(dev))return -ENODEV;mutex_lock(&dev->struct_mutex); ret = drm_mmap_locked(filp, vma); mutex_unlock(&dev->struct_mutex);diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c index 4709b54..a6d5e21 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -96,8 +96,6 @@ static int udl_mode_valid(struct drm_connector *connector, static enum drm_connector_status udl_detect(struct drm_connector *connector, bool force) {
if (drm_device_is_unplugged(connector->dev))return connector_status_disconnected; return connector_status_connected;}
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 200419d..29aca6c 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -321,12 +321,6 @@ static void udl_fb_imageblit(struct fb_info *info, const struct fb_image *image) static int udl_fb_open(struct fb_info *info, int user) { struct udl_fbdev *ufbdev = info->par;
struct drm_device *dev = ufbdev->ufb.base.dev;struct udl_device *udl = dev->dev_private;/* If the USB device is gone, we don't accept new opens */if (drm_device_is_unplugged(udl->ddev))return -ENODEV; ufbdev->fb_count++;diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d7162cf..40c6099 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,7 +748,6 @@ struct drm_device { struct drm_minor *control; /**< Control node */ struct drm_minor *primary; /**< Primary node */ struct drm_minor *render; /**< Render node */
atomic_t unplugged; /**< Flag whether dev isdead */ struct inode *anon_inode; /**< inode for private address-space */ char *unique; /**< unique name of the device */ /*@} */ @@ -879,19 +878,6 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev, return ((dev->driver->driver_features & feature) ? 1 : 0); }
-static inline void drm_device_set_unplugged(struct drm_device *dev) -{
smp_wmb();atomic_set(&dev->unplugged, 1);-}
-static inline int drm_device_is_unplugged(struct drm_device *dev) -{
int ret = atomic_read(&dev->unplugged);smp_rmb();return ret;-}
static inline bool drm_is_render_client(const struct drm_file *file_priv) { return file_priv->minor->type == DRM_MINOR_RENDER; -- 2.7.0.rc3.207.g0ac5344
Hi
On Fri, Feb 12, 2016 at 8:50 PM, Haixia Shi hshi@chromium.org wrote:
v1: Remove the general drm_device_is_unplugged() checks, and move the unplugged flag handling logic into drm/udl. In general we want to keep driver-specific logic out of common drm code.
v2: Based on discussion with Stephane and David, drop most of the unplugged flag handling logic in drm/udl except for udl_detect() and udl_fb_open(). The intention is to treat the device removal as a connector-unplug, and kep the UDL device fully functional.
v3: Based on feedback from David, entirely drop the "unplugged" flag and all related code. There is no need to check for the unplugged flag as the existing udl_usb_disconnect() behavior already ensures the controller is removed, and all code paths that uses the usb-device are not reachable after unplug.
When a UDL monitor is unplugged, we need to still treat it as a fully functional device which just happens to have its connector unplugged. This allows user-space to properly deallocate fbs and dumb buffers before closing the device.
This drops the "unplugged" flag hack, which puts the device in a non-functional state after USB unplug and rejects most operations on the device such as ioctls with error -ENODEV.
Signed-off-by: Haixia Shi hshi@chromium.org Reviewed-by: Stéphane Marchesin marcheu@chromium.org Cc: David Herrmann dh.herrmann@gmail.com Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_drv.c | 6 ------ drivers/gpu/drm/drm_fops.c | 2 -- drivers/gpu/drm/drm_gem.c | 3 --- drivers/gpu/drm/drm_ioctl.c | 3 --- drivers/gpu/drm/drm_vm.c | 3 --- drivers/gpu/drm/udl/udl_connector.c | 2 -- drivers/gpu/drm/udl/udl_fb.c | 6 ------ include/drm/drmP.h | 14 -------------- 8 files changed, 39 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 167c8d3..f93ee12 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)
if (!minor) { return ERR_PTR(-ENODEV);
} else if (drm_device_is_unplugged(minor->dev)) {drm_dev_unref(minor->dev);return ERR_PTR(-ENODEV); } return minor;@@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev) drm_minor_unregister(dev, DRM_MINOR_CONTROL);
mutex_lock(&drm_global_mutex);
drm_device_set_unplugged(dev);if (dev->open_count == 0) { drm_put_dev(dev); }diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 1ea8790..b4332d4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file *filp)
if (!--dev->open_count) { retcode = drm_lastclose(dev);
if (drm_device_is_unplugged(dev))drm_put_dev(dev);
Who frees the udl device now? What code-path is responsible to call drm_dev_unregister() for udl devices? This hunk looks bogus to me. Please elaborate.
} mutex_unlock(&drm_global_mutex);diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2e8c77e..c622e32 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_vma_offset_node *node; int ret;
if (drm_device_is_unplugged(dev))return -ENODEV;drm_vma_offset_lock_lookup(dev->vma_offset_manager); node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager, vma->vm_pgoff,diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 8ce2a0c..f959074 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
dev = file_priv->minor->dev;
if (drm_device_is_unplugged(dev))return -ENODEV;is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END; if (is_driver_ioctl) {diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index f90bd5f..3a68be4 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_device *dev = priv->minor->dev; int ret;
if (drm_device_is_unplugged(dev))return -ENODEV;mutex_lock(&dev->struct_mutex); ret = drm_mmap_locked(filp, vma); mutex_unlock(&dev->struct_mutex);diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c index 4709b54..a6d5e21 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -96,8 +96,6 @@ static int udl_mode_valid(struct drm_connector *connector, static enum drm_connector_status udl_detect(struct drm_connector *connector, bool force) {
if (drm_device_is_unplugged(connector->dev))return connector_status_disconnected; return connector_status_connected;
Who marks a connector as "disconnect" once the device is removed?
}
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 200419d..29aca6c 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -321,12 +321,6 @@ static void udl_fb_imageblit(struct fb_info *info, const struct fb_image *image) static int udl_fb_open(struct fb_info *info, int user) { struct udl_fbdev *ufbdev = info->par;
struct drm_device *dev = ufbdev->ufb.base.dev;struct udl_device *udl = dev->dev_private;/* If the USB device is gone, we don't accept new opens */if (drm_device_is_unplugged(udl->ddev))return -ENODEV; ufbdev->fb_count++;diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d7162cf..40c6099 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -748,7 +748,6 @@ struct drm_device { struct drm_minor *control; /**< Control node */ struct drm_minor *primary; /**< Primary node */ struct drm_minor *render; /**< Render node */
atomic_t unplugged; /**< Flag whether dev is dead */ struct inode *anon_inode; /**< inode for private address-space */ char *unique; /**< unique name of the device */ /*@} */@@ -879,19 +878,6 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev, return ((dev->driver->driver_features & feature) ? 1 : 0); }
-static inline void drm_device_set_unplugged(struct drm_device *dev) -{
smp_wmb();atomic_set(&dev->unplugged, 1);-}
-static inline int drm_device_is_unplugged(struct drm_device *dev) -{
int ret = atomic_read(&dev->unplugged);smp_rmb();return ret;-}
static inline bool drm_is_render_client(const struct drm_file *file_priv) { return file_priv->minor->type == DRM_MINOR_RENDER;
Did you actually test this patch? How is it supposed to work? Please explain _exactly_ in your commit message what the new semantics are. I highly doubt that this patch does what you want. Also, please run kmemleak on your tests to verify that all memory is actually released.
Generally, I like the approach. However, I think you still need a state that marks a device as dropped. I'd do something like this:
1) In udl_usb_disconnect(), set udl->udev to NULL, locked by mode_config.mutex. This should be the _first_ thing you do there. This serves as barrier against ->get_modes(), which is the only path that uses "udl->udev".
2) In udl_detect(), return 'connector_status_disconnected' if "udl->udev" is NULL.
3) In udl_get_edid() add "if (WARN_ON(!udl->udev)) return NULL;"
4) Hook into f_ops.release() and call into drm_release(). But afterwards you have to call drm_put_dev() if open_count is 0 now.
Thanks David
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
David Herrmann -
Haixia Shi -
Stéphane Marchesin