On Fri, Oct 21, 2016 at 02:42:12PM +0100, Chris Wilson wrote:
> After hotplug notification, userspace has to reprobe all connectors to
> detect any changes. This can be expensive (some outputs may require time
> consuming load detection, some outputs may simply be slow to respond)
> and not all outputs need to be checked everytime. The kernel is usually
> in a very good position to know which outputs need to be reprobed (since
> it has just responded to the hardware notification) and can convey this
> information to userspace by tracking the timestamp of the last connector
> change onto the GETCONNECTOR query.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
> Adding an epoch/timestamp field would look something like this. There are
> probably a few more places where the hardware indicates a real change
> (more than just status).
>
> That update_timestamp() should probably be
> atomic64_set(×tamp, ktime_get_ns());
Hm, I thought we'd just add a new epoch property, attach it to every
connector, and userspace would inquire about it using
DRM_IOCTL_MODE_OBJ_GETPROPERTIES.
The only nitpick/polish would be to audit
drm_mode_obj_get_properties_ioctl and figure out how much locking it
really needs (probably almost nothing at all).
> ---
> drivers/gpu/drm/drm_connector.c | 4 +++-
> drivers/gpu/drm/drm_crtc.c | 4 +++-
> drivers/gpu/drm/drm_probe_helper.c | 6 +++++-
> drivers/gpu/drm/gma500/mdfld_dsi_dpi.c | 1 +
> drivers/gpu/drm/gma500/mdfld_dsi_output.c | 1 +
> drivers/gpu/drm/i915/intel_dp.c | 4 ++++
> drivers/gpu/drm/i915/intel_dp_mst.c | 2 ++
> drivers/gpu/drm/i915/intel_hotplug.c | 5 ++++-
> drivers/gpu/drm/i915/intel_lvds.c | 1 +
> drivers/gpu/drm/nouveau/nouveau_connector.c | 1 +
> drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 1 +
> drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 1 +
> drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 1 +
> include/drm/drm_connector.h | 8 ++++++++
> include/uapi/drm/drm.h | 2 +-
> include/uapi/drm/drm_mode.h | 26 ++++++++++++++++++++++++++
> 16 files changed, 63 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 2db7fb510b6c..cd3d85e94b91 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -227,6 +227,7 @@ int drm_connector_init(struct drm_device *dev,
> INIT_LIST_HEAD(&connector->modes);
> connector->edid_blob_ptr = NULL;
> connector->status = connector_status_unknown;
> + drm_connector_update_timestamp(connector);
>
> drm_connector_get_cmdline_mode(connector);
>
> @@ -1010,7 +1011,7 @@ static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
> int drm_mode_getconnector(struct drm_device *dev, void *data,
> struct drm_file *file_priv)
> {
> - struct drm_mode_get_connector *out_resp = data;
> + struct drm_mode_get_connector_v2 *out_resp = data;
> struct drm_connector *connector;
> struct drm_encoder *encoder;
> struct drm_display_mode *mode;
> @@ -1058,6 +1059,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> out_resp->mm_height = connector->display_info.height_mm;
> out_resp->subpixel = connector->display_info.subpixel_order;
> out_resp->connection = connector->status;
> + out_resp->timestamp = ktime_to_ns(connector->timestamp);
>
> drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> encoder = drm_connector_get_encoder(connector);
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 788d6cdc49a0..8668e95bb5b1 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -949,9 +949,11 @@ void drm_mode_config_reset(struct drm_device *dev)
> encoder->funcs->reset(encoder);
>
> mutex_lock(&dev->mode_config.mutex);
> - drm_for_each_connector(connector, dev)
> + drm_for_each_connector(connector, dev) {
> + drm_connector_update_timestamp(connector);
> if (connector->funcs->reset)
> connector->funcs->reset(connector);
> + }
> mutex_unlock(&dev->mode_config.mutex);
> }
> EXPORT_SYMBOL(drm_mode_config_reset);
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 8790ee35a7cd..4b6882edc3fb 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -249,6 +249,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> * check here, and if anything changed start the hotplug code.
> */
> if (old_status != connector->status) {
> + drm_connector_update_timestamp(connector);
> DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n",
> connector->base.id,
> connector->name,
> @@ -438,6 +439,7 @@ static void output_poll_execute(struct work_struct *work)
> connector->name,
> old, new);
>
> + drm_connector_update_timestamp(connector);
> changed = true;
> }
> }
> @@ -573,8 +575,10 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
> connector->name,
> drm_get_connector_status_name(old_status),
> drm_get_connector_status_name(connector->status));
> - if (old_status != connector->status)
> + if (old_status != connector->status) {
> + drm_connector_update_timestamp(connector);
> changed = true;
> + }
> }
>
> mutex_unlock(&dev->mode_config.mutex);
I think with the above updates in probe helpers and _init you don't really
need any of the below in either driver init or probe code. Anything in
driver load code isn't needed anyway since with proper load ordering
userspace can't see the connector before it's finalized.
One additional one which we probably want is in the edid update function,
to catch any changes in edid. Plus maybe the same thing in the eld
updater (although that's just derived, but you never know ...).
Of course we'll still need additional calls (in short pulse handlers or
wherever), but that should reduce the need for them a lot.
> diff --git a/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c b/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c
> index a05c020602bd..8980d7a08c5d 100644
> --- a/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c
> +++ b/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c
> @@ -971,6 +971,7 @@ struct mdfld_dsi_encoder *mdfld_dsi_dpi_init(struct drm_device *dev,
> DRM_INFO("pipe %d power mode 0x%x\n", pipe, data);
> dsi_connector->status = connector_status_connected;
> }
> + drm_connector_update_timestamp(connector);
> }
>
> dpi_output = kzalloc(sizeof(struct mdfld_dsi_dpi_output), GFP_KERNEL);
> diff --git a/drivers/gpu/drm/gma500/mdfld_dsi_output.c b/drivers/gpu/drm/gma500/mdfld_dsi_output.c
> index acb3848ef1c9..f288d9ac9b12 100644
> --- a/drivers/gpu/drm/gma500/mdfld_dsi_output.c
> +++ b/drivers/gpu/drm/gma500/mdfld_dsi_output.c
> @@ -238,6 +238,7 @@ mdfld_dsi_connector_detect(struct drm_connector *connector, bool force)
> = mdfld_dsi_connector(connector);
>
> dsi_connector->status = connector_status_connected;
> + drm_connector_update_timestamp(connector);
>
> return dsi_connector->status;
> }
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 59cd185689c0..56fcc738edc1 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3980,6 +3980,10 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
> DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
> intel_dp->is_mst = false;
> drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
> +
> + if (intel_dp->attached_connector)
> + drm_connector_update_timestamp(&intel_dp->attached_connector->base);
> +
> /* send a hotplug event */
> drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev,
> &intel_dp->attached_connector->base);
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 2bd48a987934..d66e576eff83 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -493,6 +493,8 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
> struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> struct drm_device *dev = intel_dig_port->base.base.dev;
>
> + if (intel_dp->attached_connector)
> + drm_connector_update_timestamp(&intel_dp->attached_connector->base);
> drm_kms_helper_hotplug_event(dev, &intel_dp->attached_connector->base);
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> index da0649aff734..f3c2747be183 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -238,6 +238,7 @@ static bool intel_hpd_irq_event(struct drm_device *dev,
> if (old_status == connector->status)
> return false;
>
> + drm_connector_update_timestamp(connector);
> DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n",
> connector->base.id,
> connector->name,
> @@ -333,8 +334,10 @@ static void i915_hotplug_work_func(struct work_struct *work)
> connector->name, intel_encoder->hpd_pin);
> if (intel_encoder->hot_plug)
> intel_encoder->hot_plug(intel_encoder);
> - if (intel_hpd_irq_event(dev, connector))
> + if (intel_hpd_irq_event(dev, connector)) {
> + drm_connector_update_timestamp(connector);
> changed = true;
> + }
> }
> }
> mutex_unlock(&mode_config->mutex);
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 199b90c7907a..27f7044300ff 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -545,6 +545,7 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
> * the LID nofication event.
> */
> connector->status = connector->funcs->detect(connector, false);
> + drm_connector_update_timestamp(connector);
>
> /* Don't force modeset on machines where it causes a GPU lockup */
> if (dmi_check_system(intel_no_modeset_on_lid))
> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index c1084088f9e4..47ee48489cf3 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> @@ -455,6 +455,7 @@ nouveau_connector_force(struct drm_connector *connector)
> NV_ERROR(drm, "can't find encoder to force %s on!\n",
> connector->name);
> connector->status = connector_status_disconnected;
> + drm_connector_update_timestamp(connector);
> return;
> }
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> index 61a3cce08dfe..4500723cc856 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> @@ -363,6 +363,7 @@ static int vmw_ldu_init(struct vmw_private *dev_priv, unsigned unit)
> drm_connector_init(dev, connector, &vmw_legacy_connector_funcs,
> DRM_MODE_CONNECTOR_VIRTUAL);
> connector->status = vmw_du_connector_detect(connector, true);
> + drm_connector_update_timestamp(connector);
>
> drm_encoder_init(dev, encoder, &vmw_legacy_encoder_funcs,
> DRM_MODE_ENCODER_VIRTUAL, NULL);
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> index 573c7407c32c..acd79e0edf5a 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> @@ -524,6 +524,7 @@ static int vmw_sou_init(struct vmw_private *dev_priv, unsigned unit)
> drm_connector_init(dev, connector, &vmw_sou_connector_funcs,
> DRM_MODE_CONNECTOR_VIRTUAL);
> connector->status = vmw_du_connector_detect(connector, true);
> + drm_connector_update_timestamp(connector);
>
> drm_encoder_init(dev, encoder, &vmw_screen_object_encoder_funcs,
> DRM_MODE_ENCODER_VIRTUAL, NULL);
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> index 01c4d574fa78..3c2b71e605d6 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> @@ -1117,6 +1117,7 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit)
> drm_connector_init(dev, connector, &vmw_stdu_connector_funcs,
> DRM_MODE_CONNECTOR_VIRTUAL);
> connector->status = vmw_du_connector_detect(connector, false);
> + drm_connector_update_timestamp(connector);
>
> drm_encoder_init(dev, encoder, &vmw_stdu_encoder_funcs,
> DRM_MODE_ENCODER_VIRTUAL, NULL);
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index ac9d7d8e0e43..698d86a6af83 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -25,6 +25,7 @@
>
> #include <linux/list.h>
> #include <linux/ctype.h>
> +#include <linux/ktime.h>
> #include <drm/drm_mode_object.h>
>
> #include <uapi/drm/drm_mode.h>
> @@ -576,6 +577,7 @@ struct drm_connector {
> struct list_head modes; /* list of modes on this connector */
>
> enum drm_connector_status status;
> + ktime_t timestamp;
>
> /* these are modes added by probing with DDC or the BIOS */
> struct list_head probed_modes;
> @@ -761,6 +763,12 @@ int drm_mode_connector_set_tile_property(struct drm_connector *connector);
> int drm_mode_connector_update_edid_property(struct drm_connector *connector,
> const struct edid *edid);
>
> +static inline void
> +drm_connector_update_timestamp(struct drm_connector *connector)
Needs some kerneldoc. Otherwise I like.
> +{
> + connector->timestamp = ktime_get();
Should we make sure that the new value is different from the current one,
and if not, just add 1? We want to guarantee that it's increasing on every
change.
Cheers, Daniel
> +}
> +
> /**
> * drm_for_each_connector - iterate over all connectors
> * @connector: the loop cursor
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index b2c52843bc70..5c5941aedd8c 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -787,7 +787,7 @@ extern "C" {
> #define DRM_IOCTL_MODE_GETGAMMA DRM_IOWR(0xA4, struct drm_mode_crtc_lut)
> #define DRM_IOCTL_MODE_SETGAMMA DRM_IOWR(0xA5, struct drm_mode_crtc_lut)
> #define DRM_IOCTL_MODE_GETENCODER DRM_IOWR(0xA6, struct drm_mode_get_encoder)
> -#define DRM_IOCTL_MODE_GETCONNECTOR DRM_IOWR(0xA7, struct drm_mode_get_connector)
> +#define DRM_IOCTL_MODE_GETCONNECTOR DRM_IOWR(0xA7, struct drm_mode_get_connector_v2)
> #define DRM_IOCTL_MODE_ATTACHMODE DRM_IOWR(0xA8, struct drm_mode_mode_cmd) /* deprecated (never worked) */
> #define DRM_IOCTL_MODE_DETACHMODE DRM_IOWR(0xA9, struct drm_mode_mode_cmd) /* deprecated (never worked) */
>
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 084b50a02dc5..d3ecb546918c 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -286,6 +286,32 @@ struct drm_mode_get_connector {
> __u32 pad;
> };
>
> +struct drm_mode_get_connector_v2 {
> +
> + __u64 encoders_ptr;
> + __u64 modes_ptr;
> + __u64 props_ptr;
> + __u64 prop_values_ptr;
> +
> + __u32 count_modes;
> + __u32 count_props;
> + __u32 count_encoders;
> +
> + __u32 encoder_id; /**< Current Encoder */
> + __u32 connector_id; /**< Id */
> + __u32 connector_type;
> + __u32 connector_type_id;
> +
> + __u32 connection;
> + __u32 mm_width; /**< width in millimeters */
> + __u32 mm_height; /**< height in millimeters */
> + __u32 subpixel;
> +
> + __u32 pad; /* align to u64 */
> +
> + __u64 timestamp;
> +};
> +
> #define DRM_MODE_PROP_PENDING (1<<0)
> #define DRM_MODE_PROP_RANGE (1<<1)
> #define DRM_MODE_PROP_IMMUTABLE (1<<2)
> --
> 2.9.3
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
