[PATCH] RFC drm: Do not leak connector_status_unknown to userspace

2012-06-18 Thread Adam Jackson 
On Sat, 2012-06-09 at 00:24 +0100, Chris Wilson wrote:
> A detect() probe may return 'unknown' to indicate that it was not able
> to successfully determine the connector status. However, exposing this
> to userspace just results in lots of confusion as it is uncertain
> whether or not that is a valid connection. The usual result is for X to
> create a mode that encompasses the unknown connector causing an abnormal
> setup on the known good outputs.

False!  Please refer to xf86CollectEnabledOutputs().  X first attempts
to set up on only definitely-connected outputs, and will only consider
unknown-connected outputs if no definitely-connected outputs are
available.

That was the intent, anyway.  There might be bugs in that, but those
would be X bugs, not kernel bugs.

- ajax
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL:

Re: [PATCH] RFC drm: Do not leak connector_status_unknown to userspace

2012-06-18 Thread Adam Jackson 
On Sat, 2012-06-09 at 00:24 +0100, Chris Wilson wrote:
 A detect() probe may return 'unknown' to indicate that it was not able
 to successfully determine the connector status. However, exposing this
 to userspace just results in lots of confusion as it is uncertain
 whether or not that is a valid connection. The usual result is for X to
 create a mode that encompasses the unknown connector causing an abnormal
 setup on the known good outputs.

False!  Please refer to xf86CollectEnabledOutputs().  X first attempts
to set up on only definitely-connected outputs, and will only consider
unknown-connected outputs if no definitely-connected outputs are
available.

That was the intent, anyway.  There might be bugs in that, but those
would be X bugs, not kernel bugs.

- ajax


signature.asc
Description: This is a digitally signed message part
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] RFC drm: Do not leak connector_status_unknown to userspace

2012-06-09 Thread Daniel Vetter 
On Sat, Jun 09, 2012 at 12:24:49AM +0100, Chris Wilson wrote:
> A detect() probe may return 'unknown' to indicate that it was not able
> to successfully determine the connector status. However, exposing this
> to userspace just results in lots of confusion as it is uncertain
> whether or not that is a valid connection. The usual result is for X to
> create a mode that encompasses the unknown connector causing an abnormal
> setup on the known good outputs.
> 
> This patch initialises the connector status to disconnected and
> thereafter ignores indeterminate results from detect(), leaving the
> status as the old value. This prevents the status from being set to
> unknown, and so leaked to userspace, and so leaves the connector as
> disconnected until we have a valid connection and also prevents that
> valid connection being dropped due to a failed probe.
> 
> The alternative to this is to never return unknown from the drivers, but
> return connector->status instead so that the value never changes on a
> failed probe (and also to initialise the connector to disconnected in
> the driver).

Hm, this is a bit at odds with how I've re-defined unknown in my hpd
rework. There it essentially means that that the kernel truely has no idea
what the status is because the world changed (i.e. resume or fresh boot)
and detect hasn't been run yet. So imo we should initialize the connector
state to unknown (and reset it to unknown after resume).

I agree with the 'not leaking unknown' to userspace, although I'm unsure
about the implications. I guess we just have to try and see what happens.

Yours, Daniel
> ---
>  drivers/gpu/drm/drm_crtc.c|1 +
>  drivers/gpu/drm/drm_crtc_helper.c |   20 +---
>  2 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 08a7aa7..22f58b9 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -495,6 +495,7 @@ int drm_connector_init(struct drm_device *dev,
>   INIT_LIST_HEAD(>probed_modes);
>   INIT_LIST_HEAD(>modes);
>   connector->edid_blob_ptr = NULL;
> + connector->status = connector_status_disconnected;
>  
>   list_add_tail(>head, >mode_config.connector_list);
>   dev->mode_config.num_connector++;
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c 
> b/drivers/gpu/drm/drm_crtc_helper.c
> index 3252e70..143d6d5 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -108,7 +108,11 @@ int drm_helper_probe_single_connector_modes(struct 
> drm_connector *connector,
>   if (connector->funcs->force)
>   connector->funcs->force(connector);
>   } else {
> - connector->status = connector->funcs->detect(connector, true);
> + enum drm_connector_status status;
> +
> + status = connector->funcs->detect(connector, true);
> + if (status)
> + connector->status = status;
>   drm_kms_helper_poll_enable(dev);
>   }
>  
> @@ -949,13 +953,15 @@ static void output_poll_execute(struct work_struct 
> *work)
>   !(connector->polled & DRM_CONNECTOR_POLL_HPD))
>   continue;
>  
> - connector->status = connector->funcs->detect(connector, false);
> - 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);
> - if (old_status != connector->status)
> + new_status = connector->funcs->detect(connector, false);
> + if (new_status && old_status != new_status) {
> + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d 
> to %d\n",
> +   connector->base.id,
> +   drm_get_connector_name(connector),
> +   old_status, new_status);
> + connector->status = new_status;
>   changed = true;
> + }
>   }
>  
>   mutex_unlock(>mode_config.mutex);
> -- 
> 1.7.10
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH] RFC drm: Do not leak connector_status_unknown to userspace

2012-06-09 Thread Chris Wilson 
A detect() probe may return 'unknown' to indicate that it was not able
to successfully determine the connector status. However, exposing this
to userspace just results in lots of confusion as it is uncertain
whether or not that is a valid connection. The usual result is for X to
create a mode that encompasses the unknown connector causing an abnormal
setup on the known good outputs.

This patch initialises the connector status to disconnected and
thereafter ignores indeterminate results from detect(), leaving the
status as the old value. This prevents the status from being set to
unknown, and so leaked to userspace, and so leaves the connector as
disconnected until we have a valid connection and also prevents that
valid connection being dropped due to a failed probe.

The alternative to this is to never return unknown from the drivers, but
return connector->status instead so that the value never changes on a
failed probe (and also to initialise the connector to disconnected in
the driver).
---
 drivers/gpu/drm/drm_crtc.c|1 +
 drivers/gpu/drm/drm_crtc_helper.c |   20 +---
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 08a7aa7..22f58b9 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -495,6 +495,7 @@ int drm_connector_init(struct drm_device *dev,
INIT_LIST_HEAD(>probed_modes);
INIT_LIST_HEAD(>modes);
connector->edid_blob_ptr = NULL;
+   connector->status = connector_status_disconnected;

list_add_tail(>head, >mode_config.connector_list);
dev->mode_config.num_connector++;
diff --git a/drivers/gpu/drm/drm_crtc_helper.c 
b/drivers/gpu/drm/drm_crtc_helper.c
index 3252e70..143d6d5 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -108,7 +108,11 @@ int drm_helper_probe_single_connector_modes(struct 
drm_connector *connector,
if (connector->funcs->force)
connector->funcs->force(connector);
} else {
-   connector->status = connector->funcs->detect(connector, true);
+   enum drm_connector_status status;
+
+   status = connector->funcs->detect(connector, true);
+   if (status)
+   connector->status = status;
drm_kms_helper_poll_enable(dev);
}

@@ -949,13 +953,15 @@ static void output_poll_execute(struct work_struct *work)
!(connector->polled & DRM_CONNECTOR_POLL_HPD))
continue;

-   connector->status = connector->funcs->detect(connector, false);
-   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);
-   if (old_status != connector->status)
+   new_status = connector->funcs->detect(connector, false);
+   if (new_status && old_status != new_status) {
+   DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d 
to %d\n",
+ connector->base.id,
+ drm_get_connector_name(connector),
+ old_status, new_status);
+   connector->status = new_status;
changed = true;
+   }
}

mutex_unlock(>mode_config.mutex);
-- 
1.7.10

Re: [PATCH] RFC drm: Do not leak connector_status_unknown to userspace

2012-06-09 Thread Daniel Vetter 
On Sat, Jun 09, 2012 at 12:24:49AM +0100, Chris Wilson wrote:
 A detect() probe may return 'unknown' to indicate that it was not able
 to successfully determine the connector status. However, exposing this
 to userspace just results in lots of confusion as it is uncertain
 whether or not that is a valid connection. The usual result is for X to
 create a mode that encompasses the unknown connector causing an abnormal
 setup on the known good outputs.
 
 This patch initialises the connector status to disconnected and
 thereafter ignores indeterminate results from detect(), leaving the
 status as the old value. This prevents the status from being set to
 unknown, and so leaked to userspace, and so leaves the connector as
 disconnected until we have a valid connection and also prevents that
 valid connection being dropped due to a failed probe.
 
 The alternative to this is to never return unknown from the drivers, but
 return connector-status instead so that the value never changes on a
 failed probe (and also to initialise the connector to disconnected in
 the driver).

Hm, this is a bit at odds with how I've re-defined unknown in my hpd
rework. There it essentially means that that the kernel truely has no idea
what the status is because the world changed (i.e. resume or fresh boot)
and detect hasn't been run yet. So imo we should initialize the connector
state to unknown (and reset it to unknown after resume).

I agree with the 'not leaking unknown' to userspace, although I'm unsure
about the implications. I guess we just have to try and see what happens.

Yours, Daniel
 ---
  drivers/gpu/drm/drm_crtc.c|1 +
  drivers/gpu/drm/drm_crtc_helper.c |   20 +---
  2 files changed, 14 insertions(+), 7 deletions(-)
 
 diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
 index 08a7aa7..22f58b9 100644
 --- a/drivers/gpu/drm/drm_crtc.c
 +++ b/drivers/gpu/drm/drm_crtc.c
 @@ -495,6 +495,7 @@ int drm_connector_init(struct drm_device *dev,
   INIT_LIST_HEAD(connector-probed_modes);
   INIT_LIST_HEAD(connector-modes);
   connector-edid_blob_ptr = NULL;
 + connector-status = connector_status_disconnected;
  
   list_add_tail(connector-head, dev-mode_config.connector_list);
   dev-mode_config.num_connector++;
 diff --git a/drivers/gpu/drm/drm_crtc_helper.c 
 b/drivers/gpu/drm/drm_crtc_helper.c
 index 3252e70..143d6d5 100644
 --- a/drivers/gpu/drm/drm_crtc_helper.c
 +++ b/drivers/gpu/drm/drm_crtc_helper.c
 @@ -108,7 +108,11 @@ int drm_helper_probe_single_connector_modes(struct 
 drm_connector *connector,
   if (connector-funcs-force)
   connector-funcs-force(connector);
   } else {
 - connector-status = connector-funcs-detect(connector, true);
 + enum drm_connector_status status;
 +
 + status = connector-funcs-detect(connector, true);
 + if (status)
 + connector-status = status;
   drm_kms_helper_poll_enable(dev);
   }
  
 @@ -949,13 +953,15 @@ static void output_poll_execute(struct work_struct 
 *work)
   !(connector-polled  DRM_CONNECTOR_POLL_HPD))
   continue;
  
 - connector-status = connector-funcs-detect(connector, false);
 - 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);
 - if (old_status != connector-status)
 + new_status = connector-funcs-detect(connector, false);
 + if (new_status  old_status != new_status) {
 + DRM_DEBUG_KMS([CONNECTOR:%d:%s] status updated from %d 
 to %d\n,
 +   connector-base.id,
 +   drm_get_connector_name(connector),
 +   old_status, new_status);
 + connector-status = new_status;
   changed = true;
 + }
   }
  
   mutex_unlock(dev-mode_config.mutex);
 -- 
 1.7.10
 
 ___
 dri-devel mailing list
 dri-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Mail: dan...@ffwll.ch
Mobile: +41 (0)79 365 57 48
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] RFC drm: Do not leak connector_status_unknown to userspace

2012-06-08 Thread Chris Wilson 
A detect() probe may return 'unknown' to indicate that it was not able
to successfully determine the connector status. However, exposing this
to userspace just results in lots of confusion as it is uncertain
whether or not that is a valid connection. The usual result is for X to
create a mode that encompasses the unknown connector causing an abnormal
setup on the known good outputs.

This patch initialises the connector status to disconnected and
thereafter ignores indeterminate results from detect(), leaving the
status as the old value. This prevents the status from being set to
unknown, and so leaked to userspace, and so leaves the connector as
disconnected until we have a valid connection and also prevents that
valid connection being dropped due to a failed probe.

The alternative to this is to never return unknown from the drivers, but
return connector-status instead so that the value never changes on a
failed probe (and also to initialise the connector to disconnected in
the driver).
---
 drivers/gpu/drm/drm_crtc.c|1 +
 drivers/gpu/drm/drm_crtc_helper.c |   20 +---
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 08a7aa7..22f58b9 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -495,6 +495,7 @@ int drm_connector_init(struct drm_device *dev,
INIT_LIST_HEAD(connector-probed_modes);
INIT_LIST_HEAD(connector-modes);
connector-edid_blob_ptr = NULL;
+   connector-status = connector_status_disconnected;
 
list_add_tail(connector-head, dev-mode_config.connector_list);
dev-mode_config.num_connector++;
diff --git a/drivers/gpu/drm/drm_crtc_helper.c 
b/drivers/gpu/drm/drm_crtc_helper.c
index 3252e70..143d6d5 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -108,7 +108,11 @@ int drm_helper_probe_single_connector_modes(struct 
drm_connector *connector,
if (connector-funcs-force)
connector-funcs-force(connector);
} else {
-   connector-status = connector-funcs-detect(connector, true);
+   enum drm_connector_status status;
+
+   status = connector-funcs-detect(connector, true);
+   if (status)
+   connector-status = status;
drm_kms_helper_poll_enable(dev);
}
 
@@ -949,13 +953,15 @@ static void output_poll_execute(struct work_struct *work)
!(connector-polled  DRM_CONNECTOR_POLL_HPD))
continue;
 
-   connector-status = connector-funcs-detect(connector, false);
-   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);
-   if (old_status != connector-status)
+   new_status = connector-funcs-detect(connector, false);
+   if (new_status  old_status != new_status) {
+   DRM_DEBUG_KMS([CONNECTOR:%d:%s] status updated from %d 
to %d\n,
+ connector-base.id,
+ drm_get_connector_name(connector),
+ old_status, new_status);
+   connector-status = new_status;
changed = true;
+   }
}
 
mutex_unlock(dev-mode_config.mutex);
-- 
1.7.10

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel