[PATCH] drm: kms_helper: don't lose hotplug event

[Daniel Vetter]

Hi Dave,

Will hunting down a patch in my machine farm I've stumbled over this
one here. It fixes a detection race I've noticed while testing the new
hpd stuff a while back. Chris already smashed his r-b onto it.

Can you please include this in drm-next for 3.8?

Thanks, Daniel

On Wed, Oct 24, 2012 at 9:16 PM, Daniel Vetter  
wrote:
> There's a race window (small for hpd, 10s large for polled outputs)
> where userspace could sneak in with an unrelated connnector probe
> ioctl call and eat the hotplug event (since neither the hpd nor the
> poll code see a state change).
>
> To avoid this, check whether the connector state changes in all other
> ->detect calls (in the current helper code that's only probe_single)
> and if that's the case, fire off a hotplug event. Note that we can't
> directly call the hotplug event handler, since that expects that no
> locks are held (due to reentrancy with the fb code to update the kms
> console).
>
> Also, this requires that drivers using the probe_single helper
> function set up the poll work. All current drivers do that already,
> and with the reworked hpd handling there'll be no downside to
> unconditionally setting up the poll work any more.
>
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/drm_crtc_helper.c | 33 -
>  include/drm/drm_crtc.h|  1 +
>  2 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c 
> b/drivers/gpu/drm/drm_crtc_helper.c
> index 9d186d0..b79d7cb 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -62,6 +62,7 @@ static void drm_mode_validate_flag(struct drm_connector 
> *connector,
> return;
>  }
>
> +
>  /**
>   * drm_helper_probe_single_connector_modes - get complete set of display 
> modes
>   * @dev: DRM device
> @@ -93,6 +94,7 @@ int drm_helper_probe_single_connector_modes(struct 
> drm_connector *connector,
> connector->helper_private;
> int count = 0;
> int mode_flags = 0;
> +   enum drm_connector_status old_status;
>
> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
> drm_get_connector_name(connector));
> @@ -108,7 +110,32 @@ int drm_helper_probe_single_connector_modes(struct 
> drm_connector *connector,
> if (connector->funcs->force)
> connector->funcs->force(connector);
> } else {
> +   old_status = connector->status;
> +
> connector->status = connector->funcs->detect(connector, true);
> +
> +   /*
> +* Normally either the driver's hpd code or the poll loop 
> should
> +* pick up any changes and fire the hotplug event. But if
> +* userspace sneaks in a probe, we might miss a change. Hence
> +* check here, and if anything changed start the hotplug code.
> +*/
> +   if (old_status != connector->status) {
> +   DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from 
> %d to %d\n",
> + connector->base.id,
> + drm_get_connector_name(connector),
> + old_status, connector->status);
> +
> +   /*
> +* The hotplug event code might call into the fb
> +* helpers, and so expects that we do not hold any
> +* locks. Fire up the poll struct instead, it will
> +* disable itself again.
> +*/
> +   dev->mode_config.delayed_event = true;
> +   
> schedule_delayed_work(>mode_config.output_poll_work,
> + 0);
> +   }
> }
>
> /* Re-enable polling in case the global poll config changed. */
> @@ -939,7 +966,11 @@ static void output_poll_execute(struct work_struct *work)
> struct drm_device *dev = container_of(delayed_work, struct 
> drm_device, mode_config.output_poll_work);
> struct drm_connector *connector;
> enum drm_connector_status old_status;
> -   bool repoll = false, changed = false;
> +   bool repoll = false, changed;
> +
> +   /* Pick up any changes detected by the probe functions. */
> +   changed = dev->mode_config.delayed_event;
> +   dev->mode_config.delayed_event = false;
>
> if (!drm_kms_helper_poll)
> return;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 89f8f7f..ec207a2 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -793,6 +793,7 @@ struct drm_mode_config {
> /* output poll support */
> bool poll_enabled;
> bool poll_running;
> +   bool delayed_event;
> struct delayed_work output_poll_work;
>
> /* pointers to 

Re: [PATCH] drm: kms_helper: don't lose hotplug event

[Daniel Vetter]

Hi Dave,

Will hunting down a patch in my machine farm I've stumbled over this
one here. It fixes a detection race I've noticed while testing the new
hpd stuff a while back. Chris already smashed his r-b onto it.

Can you please include this in drm-next for 3.8?

Thanks, Daniel

On Wed, Oct 24, 2012 at 9:16 PM, Daniel Vetter daniel.vet...@ffwll.ch wrote:
 There's a race window (small for hpd, 10s large for polled outputs)
 where userspace could sneak in with an unrelated connnector probe
 ioctl call and eat the hotplug event (since neither the hpd nor the
 poll code see a state change).

 To avoid this, check whether the connector state changes in all other
 -detect calls (in the current helper code that's only probe_single)
 and if that's the case, fire off a hotplug event. Note that we can't
 directly call the hotplug event handler, since that expects that no
 locks are held (due to reentrancy with the fb code to update the kms
 console).

 Also, this requires that drivers using the probe_single helper
 function set up the poll work. All current drivers do that already,
 and with the reworked hpd handling there'll be no downside to
 unconditionally setting up the poll work any more.

 Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
 ---
  drivers/gpu/drm/drm_crtc_helper.c | 33 -
  include/drm/drm_crtc.h|  1 +
  2 files changed, 33 insertions(+), 1 deletion(-)

 diff --git a/drivers/gpu/drm/drm_crtc_helper.c 
 b/drivers/gpu/drm/drm_crtc_helper.c
 index 9d186d0..b79d7cb 100644
 --- a/drivers/gpu/drm/drm_crtc_helper.c
 +++ b/drivers/gpu/drm/drm_crtc_helper.c
 @@ -62,6 +62,7 @@ static void drm_mode_validate_flag(struct drm_connector 
 *connector,
 return;
  }

 +
  /**
   * drm_helper_probe_single_connector_modes - get complete set of display 
 modes
   * @dev: DRM device
 @@ -93,6 +94,7 @@ int drm_helper_probe_single_connector_modes(struct 
 drm_connector *connector,
 connector-helper_private;
 int count = 0;
 int mode_flags = 0;
 +   enum drm_connector_status old_status;

 DRM_DEBUG_KMS([CONNECTOR:%d:%s]\n, connector-base.id,
 drm_get_connector_name(connector));
 @@ -108,7 +110,32 @@ int drm_helper_probe_single_connector_modes(struct 
 drm_connector *connector,
 if (connector-funcs-force)
 connector-funcs-force(connector);
 } else {
 +   old_status = connector-status;
 +
 connector-status = connector-funcs-detect(connector, true);
 +
 +   /*
 +* Normally either the driver's hpd code or the poll loop 
 should
 +* pick up any changes and fire the hotplug event. But if
 +* userspace sneaks in a probe, we might miss a change. Hence
 +* check here, and if anything changed start the hotplug code.
 +*/
 +   if (old_status != connector-status) {
 +   DRM_DEBUG_KMS([CONNECTOR:%d:%s] status updated from 
 %d to %d\n,
 + connector-base.id,
 + drm_get_connector_name(connector),
 + old_status, connector-status);
 +
 +   /*
 +* The hotplug event code might call into the fb
 +* helpers, and so expects that we do not hold any
 +* locks. Fire up the poll struct instead, it will
 +* disable itself again.
 +*/
 +   dev-mode_config.delayed_event = true;
 +   
 schedule_delayed_work(dev-mode_config.output_poll_work,
 + 0);
 +   }
 }

 /* Re-enable polling in case the global poll config changed. */
 @@ -939,7 +966,11 @@ static void output_poll_execute(struct work_struct *work)
 struct drm_device *dev = container_of(delayed_work, struct 
 drm_device, mode_config.output_poll_work);
 struct drm_connector *connector;
 enum drm_connector_status old_status;
 -   bool repoll = false, changed = false;
 +   bool repoll = false, changed;
 +
 +   /* Pick up any changes detected by the probe functions. */
 +   changed = dev-mode_config.delayed_event;
 +   dev-mode_config.delayed_event = false;

 if (!drm_kms_helper_poll)
 return;
 diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
 index 89f8f7f..ec207a2 100644
 --- a/include/drm/drm_crtc.h
 +++ b/include/drm/drm_crtc.h
 @@ -793,6 +793,7 @@ struct drm_mode_config {
 /* output poll support */
 bool poll_enabled;
 bool poll_running;
 +   bool delayed_event;
 struct delayed_work output_poll_work;

 /* pointers to standard properties */
 --
 1.7.11.7




-- 
Daniel Vetter
Software Engineer, Intel 

[PATCH] drm: kms_helper: don't lose hotplug event

[Chris Wilson]

On Wed, 24 Oct 2012 21:16:53 +0200, Daniel Vetter  
wrote:
> There's a race window (small for hpd, 10s large for polled outputs)
> where userspace could sneak in with an unrelated connnector probe
> ioctl call and eat the hotplug event (since neither the hpd nor the
> poll code see a state change).
> 
> To avoid this, check whether the connector state changes in all other
> ->detect calls (in the current helper code that's only probe_single)
> and if that's the case, fire off a hotplug event. Note that we can't
> directly call the hotplug event handler, since that expects that no
> locks are held (due to reentrancy with the fb code to update the kms
> console).
> 
> Also, this requires that drivers using the probe_single helper
> function set up the poll work. All current drivers do that already,
> and with the reworked hpd handling there'll be no downside to
> unconditionally setting up the poll work any more.
> 
> Signed-off-by: Daniel Vetter 

Aye, we do want the hotplug notification for the desktop to respond to
if the user happens to steal it by haplessly querying the system with
xrandr. lgtm,

Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm: kms_helper: don't lose hotplug event

[Daniel Vetter]

There's a race window (small for hpd, 10s large for polled outputs)
where userspace could sneak in with an unrelated connnector probe
ioctl call and eat the hotplug event (since neither the hpd nor the
poll code see a state change).

To avoid this, check whether the connector state changes in all other
->detect calls (in the current helper code that's only probe_single)
and if that's the case, fire off a hotplug event. Note that we can't
directly call the hotplug event handler, since that expects that no
locks are held (due to reentrancy with the fb code to update the kms
console).

Also, this requires that drivers using the probe_single helper
function set up the poll work. All current drivers do that already,
and with the reworked hpd handling there'll be no downside to
unconditionally setting up the poll work any more.

Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/drm_crtc_helper.c | 33 -
 include/drm/drm_crtc.h|  1 +
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c 
b/drivers/gpu/drm/drm_crtc_helper.c
index 9d186d0..b79d7cb 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -62,6 +62,7 @@ static void drm_mode_validate_flag(struct drm_connector 
*connector,
return;
 }

+
 /**
  * drm_helper_probe_single_connector_modes - get complete set of display modes
  * @dev: DRM device
@@ -93,6 +94,7 @@ int drm_helper_probe_single_connector_modes(struct 
drm_connector *connector,
connector->helper_private;
int count = 0;
int mode_flags = 0;
+   enum drm_connector_status old_status;

DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
drm_get_connector_name(connector));
@@ -108,7 +110,32 @@ int drm_helper_probe_single_connector_modes(struct 
drm_connector *connector,
if (connector->funcs->force)
connector->funcs->force(connector);
} else {
+   old_status = connector->status;
+
connector->status = connector->funcs->detect(connector, true);
+
+   /*
+* Normally either the driver's hpd code or the poll loop should
+* pick up any changes and fire the hotplug event. But if
+* userspace sneaks in a probe, we might miss a change. Hence
+* check here, and if anything changed start the hotplug code.
+*/
+   if (old_status != connector->status) {
+   DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d 
to %d\n",
+ connector->base.id,
+ drm_get_connector_name(connector),
+ old_status, connector->status);
+
+   /*
+* The hotplug event code might call into the fb
+* helpers, and so expects that we do not hold any
+* locks. Fire up the poll struct instead, it will
+* disable itself again.
+*/
+   dev->mode_config.delayed_event = true;
+   
schedule_delayed_work(>mode_config.output_poll_work,
+ 0);
+   }
}

/* Re-enable polling in case the global poll config changed. */
@@ -939,7 +966,11 @@ static void output_poll_execute(struct work_struct *work)
struct drm_device *dev = container_of(delayed_work, struct drm_device, 
mode_config.output_poll_work);
struct drm_connector *connector;
enum drm_connector_status old_status;
-   bool repoll = false, changed = false;
+   bool repoll = false, changed;
+
+   /* Pick up any changes detected by the probe functions. */
+   changed = dev->mode_config.delayed_event;
+   dev->mode_config.delayed_event = false;

if (!drm_kms_helper_poll)
return;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 89f8f7f..ec207a2 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -793,6 +793,7 @@ struct drm_mode_config {
/* output poll support */
bool poll_enabled;
bool poll_running;
+   bool delayed_event;
struct delayed_work output_poll_work;

/* pointers to standard properties */
-- 
1.7.11.7