[PATCH v2 2/2] drm: remove drm_device_is_unplugged checks in common drm code.

2016-02-11 Thread David Herrmann 
Hi

On Thu, Feb 11, 2016 at 12:18 AM, 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 
> Reviewed-by: Stéphane Marchesin 

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

[PATCH v2 2/2] drm: remove drm_device_is_unplugged checks in common drm code.

2016-02-11 Thread Haixia Shi 
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 
wrote:

> Hi
>
> On Thu, Feb 11, 2016 at 12:18 AM, 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 
> > Reviewed-by: Stéphane Marchesin 
>
> 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
>
-- next part --
An HTML attachment was scrubbed...
URL:

[PATCH v2 2/2] drm: remove drm_device_is_unplugged checks in common drm code.

2016-02-11 Thread Daniel Vetter 
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 
> Reviewed-by: Stéphane Marchesin 

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(_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(_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(>struct_mutex);
>   ret = drm_mmap_locked(filp, vma);
>   mutex_unlock(>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);
> +

[PATCH v2 2/2] drm: remove drm_device_is_unplugged checks in common drm code.

2016-02-10 Thread Haixia Shi 
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 
Reviewed-by: Stéphane Marchesin 
---
 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(_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(_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(>struct_mutex);
ret = drm_mmap_locked(filp, vma);
mutex_unlock(>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

[PATCH v2 2/2] drm: remove drm_device_is_unplugged checks in common drm code.

2016-02-10 Thread Haixia Shi 
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 
Reviewed-by: Stéphane Marchesin 
---
 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(_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(_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(>struct_mutex);
ret = drm_mmap_locked(filp, vma);
mutex_unlock(>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