drm: link status property and DP link training failure handling
Subsystems (3)
cover drm: link status property and DP link training failure handling
The two remaining patches from [1], rebased.
BR,
Jani.
[1] http://mid.mail-archive.com/[email protected]
Manasi Navare (2):
drm: Add a new connector atomic property for link status drm/i915: Implement Link Rate fallback on Link training failure
drivers/gpu/drm/drm_atomic.c | 16 +++++++++ drivers/gpu/drm/drm_atomic_helper.c | 15 ++++++++ drivers/gpu/drm/drm_connector.c | 52 +++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_dp.c | 27 ++++++++++++++ drivers/gpu/drm/i915/intel_dp_link_training.c | 22 ++++++++++-- drivers/gpu/drm/i915/intel_drv.h | 3 ++ include/drm/drm_connector.h | 19 ++++++++++ include/drm/drm_mode_config.h | 5 +++ include/uapi/drm/drm_mode.h | 4 +++ 9 files changed, 161 insertions(+), 2 deletions(-)
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 1/2 drm: Add a new connector atomic property for link status
From: Manasi Navare <[email protected]>
At the time userspace does setcrtc, we've already promised the mode would work. The promise is based on the theoretical capabilities of the link, but it's possible we can't reach this in practice. The DP spec describes how the link should be reduced, but we can't reduce the link below the requirements of the mode. Black screen follows.
One idea would be to have setcrtc return a failure. However, it already should not fail as the atomic checks have passed. It would also conflict with the idea of making setcrtc asynchronous in the future, returning before the actual mode setting and link training.
Another idea is to train the link "upfront" at hotplug time, before pruning the mode list, so that we can do the pruning based on practical not theoretical capabilities. However, the changes for link training are pretty drastic, all for the sake of error handling and DP compliance, when the most common happy day scenario is the current approach of link training at mode setting time, using the optimal parameters for the mode. It is also not certain all hardware could do this without the pipe on; not even all our hardware can do this. Some of this can be solved, but not trivially.
Both of the above ideas also fail to address link degradation *during* operation.
The solution is to add a new "link-status" connector property in order to address link training failure in a way that:
a) changes the current happy day scenario as little as possible, to avoid regressions, b) can be implemented the same way by all drm drivers, c) is still opt-in for the drivers and userspace, and opting out doesn't regress the user experience, d) doesn't prevent drivers from implementing better or alternate approaches, possibly without userspace involvement. And, of course, handles all the issues presented. In the usual happy day scenario, this is always "good". If something fails during or after a mode set, the kernel driver can set the link status to "bad" and issue a hotplug uevent for userspace to have it re-check the valid modes through GET_CONNECTOR IOCTL, and try modeset again. If the theoretical capabilities of the link can't be reached, the mode list is trimmed based on that.
v7 by Jani:
* Rebase, simplify set property while at it, checkpatch fix
v6:
* Fix a typo in kernel doc (Sean Paul)
v5:
* Clarify doc for silent rejection of atomic properties by driver (Daniel Vetter) v4:
* Add comments in kernel-doc format (Daniel Vetter)
* Update the kernel-doc for link-status (Sean Paul)
v3:
* Fixed a build error (Jani Saarinen)
v2:
* Removed connector->link_status (Daniel Vetter)
* Set connector->state->link_status in drm_mode_connector_set_link_status_property (Daniel Vetter)
* Set the connector_changed flag to true if connector->state->link_status changed.
* Reset link_status to GOOD in update_output_state (Daniel Vetter)
* Never allow userspace to set link status from Good To Bad (Daniel Vetter)
--- drivers/gpu/drm/drm_atomic.c | 16 ++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 15 +++++++++++ drivers/gpu/drm/drm_connector.c | 52 +++++++++++++++++++++++++++++++++++++ include/drm/drm_connector.h | 19 ++++++++++++++ include/drm/drm_mode_config.h | 5 ++++ include/uapi/drm/drm_mode.h | 4 +++ 6 files changed, 111 insertions(+)
drivers/gpu/drm/drm_atomic.c +16 -0
@@ -1111,6 +1111,20 @@ int drm_atomic_connector_set_property(struct drm_connector *connector, state->tv.saturation = val; }else if (property == config->tv_hue_property) { state->tv.hue = val; + }else if (property == config->link_status_property) { +/* Never downgrade from GOOD to BAD on userspace's request here, +* only hw issues can do that. +* +* For an atomic property the userspace doesn't need to be able +* to understand all the properties, but needs to be able to +* restore the state it wants on VT switch. So if the userspace +* tries to change the link_status from GOOD to BAD, driver +* silently rejects it and returns a 0. This prevents userspace +* from accidently breaking the display when it restores the +* state. +*/ +if (state->link_status !=DRM_LINK_STATUS_GOOD ) + state->link_status = val; }else if (connector->funcs->atomic_set_property) {return connector->funcs->atomic_set_property (connector, state, property, val);
@@ -1185,6 +1199,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, *val = state->tv.saturation; }else if (property == config->tv_hue_property) { *val = state->tv.hue; + }else if (property == config->link_status_property) { + *val = state->link_status; }else if (connector->funcs->atomic_get_property) {return connector->funcs->atomic_get_property (connector, state, property, val);
drivers/gpu/drm/drm_atomic_helper.c +15 -0
@@ -519,6 +519,13 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, connector_state);if (ret)return ret; +if (connector->state->crtc) { + crtc_state =drm_atomic_get_existing_crtc_state (state, + connector->state->crtc); +if (connector->state->link_status != + connector_state->link_status) + crtc_state->connectors_changed = true; + } }/*
@@ -2255,6 +2262,8 @@ static int update_output_state(struct drm_atomic_state *state, NULL);if (ret)return ret; +/* Make sure legacy setCrtc always re-trains */ + conn_state->link_status =DRM_LINK_STATUS_GOOD ; } }
@@ -2298,6 +2307,12 @@ static int update_output_state(struct drm_atomic_state *state,* * Provides a default crtc set_config handler using the atomic driver interface. * +* NOTE: For backwards compatibility with old userspace this automatically +* resets the "link-status" property to GOOD, to force any link +* re-training. The SETCRTC ioctl does not define whether an update does +* need a full modeset or just a plane update, hence we're allowed to do +* that. See also drm_mode_connector_set_link_status_property(). +* * Returns: * Returns 0 on success, negative errno numbers on failure. */
drivers/gpu/drm/drm_connector.c +52 -0
82 lines not on this page · raw message
include/drm/drm_connector.h +19 -0
@@ -90,6 +90,17 @@ enum subpixel_order { };/** +* enum drm_link_status - connector's link_status property value +* +* This enum is used as the connector's link status property value. +* It is set to the values defined in uapi. +*/ +enum drm_link_status { +DRM_LINK_STATUS_GOOD =DRM_MODE_LINK_STATUS_GOOD , +DRM_LINK_STATUS_BAD =DRM_MODE_LINK_STATUS_BAD , +}; + +/** * struct drm_display_info - runtime data about the connected sink * * Describes a given display (e.g. CRT or flat panel) and its limitations. For
@@ -243,6 +254,12 @@ struct drm_connector_state {struct drm_encoder *best_encoder; +/** +* @link_status: Connector link_status to keep track of whether link is +* GOOD or BAD to notify userspace if retraining is necessary. +*/ +enum drm_link_status link_status; +struct drm_atomic_state *state;struct drm_tv_connector_state tv;
@@ -808,6 +825,8 @@ int drm_mode_connector_set_path_property(struct drm_connector *connector,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); +void drm_mode_connector_set_link_status_property (struct drm_connector *connector, +uint64_t link_status);/** * struct drm_tile_group - Tile group metadata
include/drm/drm_mode_config.h +5 -0
@@ -431,6 +431,11 @@ struct drm_mode_config {*/ struct drm_property *tile_property;/** +* @link_status_property: Default connector property for link status +* of a connector +*/ +struct drm_property *link_status_property; +/** * @plane_type_property: Default plane property to differentiate * CURSOR, PRIMARY and OVERLAY legacy uses of planes. */
include/uapi/drm/drm_mode.h +4 -0
@@ -123,6 +123,10 @@ extern "C" {#define DRM_MODE_DIRTY_ON 1#define DRM_MODE_DIRTY_ANNOTATE 2 +/* Link Status options */ +#define DRM_MODE_LINK_STATUS_GOOD 0 +#define DRM_MODE_LINK_STATUS_BAD 1 +struct drm_mode_modeinfo {__u32 clock;__u16 hdisplay;
Signature · 6 lines
2.1.4 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter
AppliedRe: [PATCH 1/2] drm: Add a new connector atomic property for link status
Quoted 60 lines · Jani Nikula
From: Manasi Navare <[email protected]>
At the time userspace does setcrtc, we've already promised the mode would work. The promise is based on the theoretical capabilities of the link, but it's possible we can't reach this in practice. The DP spec describes how the link should be reduced, but we can't reduce the link below the requirements of the mode. Black screen follows.
One idea would be to have setcrtc return a failure. However, it already should not fail as the atomic checks have passed. It would also conflict with the idea of making setcrtc asynchronous in the future, returning before the actual mode setting and link training.
Another idea is to train the link "upfront" at hotplug time, before pruning the mode list, so that we can do the pruning based on practical not theoretical capabilities. However, the changes for link training are pretty drastic, all for the sake of error handling and DP compliance, when the most common happy day scenario is the current approach of link training at mode setting time, using the optimal parameters for the mode. It is also not certain all hardware could do this without the pipe on; not even all our hardware can do this. Some of this can be solved, but not trivially.
Both of the above ideas also fail to address link degradation *during* operation.
The solution is to add a new "link-status" connector property in order to address link training failure in a way that:
a) changes the current happy day scenario as little as possible, to avoid regressions, b) can be implemented the same way by all drm drivers, c) is still opt-in for the drivers and userspace, and opting out doesn't regress the user experience, d) doesn't prevent drivers from implementing better or alternate approaches, possibly without userspace involvement. And, of course, handles all the issues presented. In the usual happy day scenario, this is always "good". If something fails during or after a mode set, the kernel driver can set the link status to "bad" and issue a hotplug uevent for userspace to have it re-check the valid modes throughGET_CONNECTORIOCTL, and try modeset again. If the theoretical capabilities of the link can't be reached, the mode list is trimmed based on that.v7 by Jani:
* Rebase, simplify set property while at it, checkpatch fix
v6:
* Fix a typo in kernel doc (Sean Paul)
v5:
* Clarify doc for silent rejection of atomic properties by driver (Daniel Vetter) v4:
* Add comments in kernel-doc format (Daniel Vetter)
* Update the kernel-doc for link-status (Sean Paul)
v3:
* Fixed a build error (Jani Saarinen)
v2:
* Removedconnector->link_status(Daniel Vetter)
* Setconnector->state->link_statusindrm_mode_connector_set_link_status_property(Daniel Vetter)
* Set theconnector_changedflag to true ifconnector->state->link_statuschanged.
* Resetlink_statusto GOOD inupdate_output_state(Daniel Vetter)
* Never allow userspace to set link status from Good To Bad (Daniel Vetter)
Quoted 12 lines
Reviewed-by Sean Paul <[email protected]>Reviewed-by Daniel Vetter <[email protected]>Reviewed-by Jani Nikula <[email protected]>Acked-by Tony Cheng <[email protected]>Acked-by Harry Wentland <[email protected]>Cc Jani Nikula <[email protected]>Cc Daniel Vetter <[email protected]>Cc Ville Syrjala <[email protected]>Cc Chris Wilson <[email protected]>Cc Sean Paul <[email protected]>Signed-off-by Manasi Navare <[email protected]>Signed-off-by Jani Nikula <[email protected]>
Applied to drm-misc-next. Should we apply the single i915 patch to drm-misc too, or wait for backmerges? That would only be after -rc1 and after Dave pull in the currently pending stuff ...
Thanks, Daniel
Quoted 256 lines
--- drivers/gpu/drm/drm_atomic.c | 16 ++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 15 +++++++++++ drivers/gpu/drm/drm_connector.c | 52 +++++++++++++++++++++++++++++++++++++ include/drm/drm_connector.h | 19 ++++++++++++++ include/drm/drm_mode_config.h | 5 ++++ include/uapi/drm/drm_mode.h | 4 +++ 6 files changed, 111 insertions(+)drivers/gpu/drm/drm_atomic.c +16
-0@@ -1111,6 +1111,20 @@ int drm_atomic_connector_set_property(struct drm_connector *connector, state->tv.saturation = val; }else if (property == config->tv_hue_property) { state->tv.hue = val; + }else if (property == config->link_status_property) { +/* Never downgrade from GOOD to BAD on userspace's request here, +* only hw issues can do that. +* +* For an atomic property the userspace doesn't need to be able +* to understand all the properties, but needs to be able to +* restore the state it wants on VT switch. So if the userspace +* tries to change the link_status from GOOD to BAD, driver +* silently rejects it and returns a 0. This prevents userspace +* from accidently breaking the display when it restores the +* state. +*/ +if (state->link_status !=DRM_LINK_STATUS_GOOD ) + state->link_status = val; }else if (connector->funcs->atomic_set_property) {return connector->funcs->atomic_set_property (connector, state, property, val);@@ -1185,6 +1199,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, *val = state->tv.saturation; }else if (property == config->tv_hue_property) { *val = state->tv.hue; + }else if (property == config->link_status_property) { + *val = state->link_status; }else if (connector->funcs->atomic_get_property) {return connector->funcs->atomic_get_property (connector, state, property, val);drivers/gpu/drm/drm_atomic_helper.c +15
-0@@ -519,6 +519,13 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, connector_state);if (ret)return ret; +if (connector->state->crtc) { + crtc_state =drm_atomic_get_existing_crtc_state (state, + connector->state->crtc); +if (connector->state->link_status != + connector_state->link_status) + crtc_state->connectors_changed = true; + } }/* @@ -2255,6 +2262,8 @@ static int update_output_state(struct drm_atomic_state *state, NULL);if (ret)return ret; +/* Make sure legacy setCrtc always re-trains */ + conn_state->link_status =DRM_LINK_STATUS_GOOD ; } }@@ -2298,6 +2307,12 @@ static int update_output_state(struct drm_atomic_state *state,* * Provides a default crtc set_config handler using the atomic driver interface. * +* NOTE: For backwards compatibility with old userspace this automatically +* resets the "link-status" property to GOOD, to force any link +* re-training. The SETCRTC ioctl does not define whether an update does +* need a full modeset or just a plane update, hence we're allowed to do +* that. See also drm_mode_connector_set_link_status_property(). +* * Returns: * Returns 0 on success, negative errno numbers on failure. */ drivers/gpu/drm/drm_connector.c +52
-0@@ -243,6 +243,10 @@ int drm_connector_init(struct drm_device *dev,drm_object_attach_property (&connector->base, config->dpms_property,0 ); +drm_object_attach_property (&connector->base, + config->link_status_property, +0 ); +if (drm_core_check_feature (dev,DRIVER_ATOMIC )) {drm_object_attach_property (&connector->base, config->prop_crtc_id,0 ); }@@ -506,6 +510,12 @@ static const struct drm_prop_enum_list drm_dpms_enum_list[] = { }; DRM_ENUM_NAME_FN(drm_get_dpms_name, drm_dpms_enum_list) +static const struct drm_prop_enum_list drm_link_status_enum_list[] = { + {DRM_MODE_LINK_STATUS_GOOD ,"Good" }, + {DRM_MODE_LINK_STATUS_BAD ,"Bad" }, +}; +DRM_ENUM_NAME_FN (drm_get_link_status_name, drm_link_status_enum_list) +/** * drm_display_info_set_bus_formats - set the supported bus formats * @info: display info to store bus formats in @@ -625,6 +635,11 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,* tiling and virtualize both &drm_crtc and &drm_plane if needed. Drivers * should update this value using drm_mode_connector_set_tile_property(). * Userspace cannot change this property. +* link-status: +* Connector link-status property to indicate the status of link. The default +* value of link-status is "GOOD". If something fails during or after modeset, +* the kernel driver may set this to "BAD" and issue a hotplug uevent. Drivers +* should update this value using drm_mode_connector_set_link_status_property(). * * Connectors also have one standardized atomic property: * @@ -666,6 +681,13 @@ int drm_connector_create_standard_properties(struct drm_device *dev)return -ENOMEM ; dev->mode_config.tile_property = prop; + prop =drm_property_create_enum (dev,0 ,"link-status" , + drm_link_status_enum_list, +ARRAY_SIZE (drm_link_status_enum_list)); +if (!prop) +return -ENOMEM ; + dev->mode_config.link_status_property = prop; +return 0 ; }@@ -995,6 +1017,36 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector, }EXPORT_SYMBOL (drm_mode_connector_update_edid_property); +/** +* drm_mode_connector_set_link_status_property - Set link status property of a connector +* @connector: drm connector +* @link_status: new value of link status property (0: Good, 1: Bad) +* +* In usual working scenario, this link status property will always be set to +* "GOOD". If something fails during or after a mode set, the kernel driver +* may set this link status property to "BAD". The caller then needs to send a +* hotplug uevent for userspace to re-check the valid modes through +* GET_CONNECTOR_IOCTL and retry modeset. +* +* Note: Drivers cannot rely on userspace to support this property and +* issue a modeset. As such, they may choose to handle issues (like +* re-training a link) without userspace's intervention. +* +* The reason for adding this property is to handle link training failures, but +* it is not limited to DP or link training. For example, if we implement +* asynchronous setcrtc, this property can be used to report any failures in that. +*/ +void drm_mode_connector_set_link_status_property (struct drm_connector *connector, +uint64_t link_status) +{ +struct drm_device *dev = connector->dev; + +drm_modeset_lock (&dev->mode_config.connection_mutex,NULL ); + connector->state->link_status = link_status; +drm_modeset_unlock (&dev->mode_config.connection_mutex); +} +EXPORT_SYMBOL (drm_mode_connector_set_link_status_property); +int drm_mode_connector_set_obj_prop (struct drm_mode_object *obj,struct drm_property *property,uint64_t value)include/drm/drm_connector.h +19
-0@@ -90,6 +90,17 @@ enum subpixel_order { };/** +* enum drm_link_status - connector's link_status property value +* +* This enum is used as the connector's link status property value. +* It is set to the values defined in uapi. +*/ +enum drm_link_status { +DRM_LINK_STATUS_GOOD =DRM_MODE_LINK_STATUS_GOOD , +DRM_LINK_STATUS_BAD =DRM_MODE_LINK_STATUS_BAD , +}; + +/** * struct drm_display_info - runtime data about the connected sink * * Describes a given display (e.g. CRT or flat panel) and its limitations. For @@ -243,6 +254,12 @@ struct drm_connector_state {struct drm_encoder *best_encoder; +/** +* @link_status: Connector link_status to keep track of whether link is +* GOOD or BAD to notify userspace if retraining is necessary. +*/ +enum drm_link_status link_status; +struct drm_atomic_state *state;struct drm_tv_connector_state tv;@@ -808,6 +825,8 @@ int drm_mode_connector_set_path_property(struct drm_connector *connector,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); +void drm_mode_connector_set_link_status_property (struct drm_connector *connector, +uint64_t link_status);/** * struct drm_tile_group - Tile group metadata include/drm/drm_mode_config.h +5
-0@@ -431,6 +431,11 @@ struct drm_mode_config {*/ struct drm_property *tile_property;/** +* @link_status_property: Default connector property for link status +* of a connector +*/ +struct drm_property *link_status_property; +/** * @plane_type_property: Default plane property to differentiate * CURSOR, PRIMARY and OVERLAY legacy uses of planes. */ include/uapi/drm/drm_mode.h +4
-0@@ -123,6 +123,10 @@ extern "C" {#define DRM_MODE_DIRTY_ON 1#define DRM_MODE_DIRTY_ANNOTATE 2 +/* Link Status options */ +#define DRM_MODE_LINK_STATUS_GOOD 0 +#define DRM_MODE_LINK_STATUS_BAD 1 +struct drm_mode_modeinfo {__u32 clock;__u16 hdisplay;Signature · 6 lines
2.1.4 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 1/2] drm: Add a new connector atomic property for link status
In reply to Jani Nikulainclude/drm/drm_connector.h enum subpixel_order { · line 100
Quoted 9 lines · Jani Nikula
/**
+ * enumdrm_link_status- connector'slink_statusproperty value
+ *
+ * This enum is used as the connector's link status property value.
+ * It is set to the values defined in uapi.
+ */include/drm/drm_connector.h +3
-0+enum drm_link_status { +DRM_LINK_STATUS_GOOD =DRM_MODE_LINK_STATUS_GOOD , +DRM_LINK_STATUS_BAD =DRM_MODE_LINK_STATUS_BAD ,
We now have a new warning here when building docs. Please fix in a follow-up patch.
-Daniel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 2/2 drm/i915: Implement Link Rate fallback on Link training failure
In reply to Jani NikulaFrom: Manasi Navare <[email protected]>
If link training at a link rate optimal for a particular mode fails during modeset's atomic commit phase, then we let the modeset complete and then retry. We save the link rate value at which link training failed, update the link status property to "BAD" and use a lower link rate to prune the modes. It will redo the modeset on the current mode at lower link rate or if the current mode gets pruned due to lower link constraints then, it will send a hotplug uevent for userspace to handle it.
This is also required to pass DP CTS tests 4.3.1.3, 4.3.1.4, 4.3.1.6.
v9:
* Use the trimmed max values of link rate/lane count based on link train fallback (Daniel Vetter)
v8:
* Set link_status to BAD first and then call mode_valid (Jani Nikula) v7:
Remove the redundant variable in previous patch itself
v6:
* Obtain link rate index from fallback_link_rate using
the helper intel_dp_link_rate_index (Jani Nikula)
* Include fallback within intel_dp_start_link_train (Jani Nikula) v5:
* Move set link status to drm core (Daniel Vetter, Jani Nikula) v4:
* Add fallback support for non DDI platforms too
* Set connector->link status inside set_link_status function (Jani Nikula)
v3:
* Set link status property to BAd unconditionally (Jani Nikula)
* Dont use two separate variables link_train_failed and link_status to indicate same thing (Jani Nikula)
v2:
* Squashed a few patches (Jani Nikula)
--- drivers/gpu/drm/i915/intel_dp.c | 27 +++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_dp_link_training.c | 22 ++++++++++++++++++++-- drivers/gpu/drm/i915/intel_drv.h | 3 +++ 3 files changed, 50 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +27 -0
@@ -5671,6 +5671,29 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,return false; } +static void intel_dp_modeset_retry_work_fn (struct work_struct *work) +{ +struct intel_connector *intel_connector; +struct drm_connector *connector; + + intel_connector =container_of (work,typeof (*intel_connector), + modeset_retry_work); + connector = &intel_connector->base; +DRM_DEBUG_KMS ("[CONNECTOR:%d:%s]\n" , connector->base.id, + connector->name); + +/* Grab the locks before changing connector property*/ +mutex_lock (&connector->dev->mode_config.mutex); +/* Set connector link status to BAD and send a Uevent to notify +* userspace to do a modeset. +*/ +drm_mode_connector_set_link_status_property (connector, +DRM_MODE_LINK_STATUS_BAD ); +mutex_unlock (&connector->dev->mode_config.mutex); +/* Send Hotplug uevent so userspace can reprobe */ +drm_kms_helper_hotplug_event (connector->dev); +} +bool intel_dp_init_connector (struct intel_digital_port *intel_dig_port,struct intel_connector *intel_connector)
@@ -5683,6 +5706,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,enum port port = intel_dig_port->port;int type; +/* Initialize the work for modeset in case of link train failure */ +INIT_WORK (&intel_connector->modeset_retry_work, + intel_dp_modeset_retry_work_fn); +if (WARN (intel_dig_port->max_lanes <1 ,"Not enough lanes (%d) for DP on port %c\n" , intel_dig_port->max_lanes,port_name (port)))
drivers/gpu/drm/i915/intel_dp_link_training.c +20 -2
@@ -313,6 +313,24 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp)void intel_dp_start_link_train (struct intel_dp *intel_dp) { -intel_dp_link_training_clock_recovery (intel_dp); -intel_dp_link_training_channel_equalization (intel_dp); +struct intel_connector *intel_connector = intel_dp->attached_connector; + +if (!intel_dp_link_training_clock_recovery (intel_dp)) + goto failure_handling; +if (!intel_dp_link_training_channel_equalization (intel_dp)) + goto failure_handling; + +DRM_DEBUG_KMS ("Link Training Passed at Link Rate = %d, Lane count = %d" , + intel_dp->link_rate, intel_dp->lane_count); +return ; + + failure_handling: +DRM_DEBUG_KMS ("Link Training failed at link rate = %d, lane count = %d" , + intel_dp->link_rate, intel_dp->lane_count); +if (!intel_dp_get_link_train_fallback_values (intel_dp, + intel_dp->link_rate, + intel_dp->lane_count)) +/* Schedule a Hotplug Uevent to userspace to start modeset */ +schedule_work (&intel_connector->modeset_retry_work); +return ; }
drivers/gpu/drm/i915/intel_drv.h +3 -0
@@ -315,6 +315,9 @@ struct intel_connector {void *port;/* store this opaque as its illegal to dereference it */ struct intel_dp *mst_port; + +/* Work struct to schedule a uevent on link train failure */ +struct work_struct modeset_retry_work; };struct dpll {
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter
AppliedRe: [PATCH 2/2] drm/i915: Implement Link Rate fallback on Link training failure
Quoted 38 lines · Jani Nikula
From: Manasi Navare <[email protected]>
If link training at a link rate optimal for a particular mode fails during modeset's atomic commit phase, then we let the modeset complete and then retry. We save the link rate value at which link training failed, update the link status property to "BAD" and use a lower link rate to prune the modes. It will redo the modeset on the current mode at lower link rate or if the current mode gets pruned due to lower link constraints then, it will send a hotplug uevent for userspace to handle it.
This is also required to pass DP CTS tests 4.3.1.3, 4.3.1.4, 4.3.1.6.
v9:
* Use the trimmed max values of link rate/lane count based on link train fallback (Daniel Vetter)
v8:
* Setlink_statusto BAD first and then callmode_valid(Jani Nikula) v7:
Remove the redundant variable in previous patch itself
v6:
* Obtain link rate index fromfallback_link_rateusing
the helperintel_dp_link_rate_index(Jani Nikula)
* Include fallback withinintel_dp_start_link_train(Jani Nikula) v5:
* Move set link status to drm core (Daniel Vetter, Jani Nikula) v4:
* Add fallback support for non DDI platforms too
* Setconnector->linkstatus insideset_link_statusfunction (Jani Nikula)
v3:
* Set link status property to BAd unconditionally (Jani Nikula)
* Dont use two separate variableslink_train_failedandlink_statusto indicate same thing (Jani Nikula)
v2:
* Squashed a few patches (Jani Nikula)
Quoted 8 lines
Acked-by Tony Cheng <[email protected]>Acked-by Harry Wentland <[email protected]>Cc Jani Nikula <[email protected]>Cc Daniel Vetter <[email protected]>Cc Ville Syrjala <[email protected]>Signed-off-by Manasi Navare <[email protected]>Reviewed-by Jani Nikula <[email protected]>Signed-off-by Jani Nikula <[email protected]>
Pushed to drm-misc-next after a quick irc chat with Manasi.
Thanks, Daniel
Quoted 103 lines
--- drivers/gpu/drm/i915/intel_dp.c | 27 +++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_dp_link_training.c | 22 ++++++++++++++++++++-- drivers/gpu/drm/i915/intel_drv.h | 3 +++ 3 files changed, 50 insertions(+), 2 deletions(-)drivers/gpu/drm/i915/intel_dp.c +27
-0@@ -5671,6 +5671,29 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,return false; } +static void intel_dp_modeset_retry_work_fn (struct work_struct *work) +{ +struct intel_connector *intel_connector; +struct drm_connector *connector; + + intel_connector =container_of (work,typeof (*intel_connector), + modeset_retry_work); + connector = &intel_connector->base; +DRM_DEBUG_KMS ("[CONNECTOR:%d:%s]\n" , connector->base.id, + connector->name); + +/* Grab the locks before changing connector property*/ +mutex_lock (&connector->dev->mode_config.mutex); +/* Set connector link status to BAD and send a Uevent to notify +* userspace to do a modeset. +*/ +drm_mode_connector_set_link_status_property (connector, +DRM_MODE_LINK_STATUS_BAD ); +mutex_unlock (&connector->dev->mode_config.mutex); +/* Send Hotplug uevent so userspace can reprobe */ +drm_kms_helper_hotplug_event (connector->dev); +} +bool intel_dp_init_connector (struct intel_digital_port *intel_dig_port,struct intel_connector *intel_connector)@@ -5683,6 +5706,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,enum port port = intel_dig_port->port;int type; +/* Initialize the work for modeset in case of link train failure */ +INIT_WORK (&intel_connector->modeset_retry_work, + intel_dp_modeset_retry_work_fn); +if (WARN (intel_dig_port->max_lanes <1 ,"Not enough lanes (%d) for DP on port %c\n" , intel_dig_port->max_lanes,port_name (port)))drivers/gpu/drm/i915/intel_dp_link_training.c +20
-2@@ -313,6 +313,24 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp)void intel_dp_start_link_train (struct intel_dp *intel_dp) { -intel_dp_link_training_clock_recovery (intel_dp); -intel_dp_link_training_channel_equalization (intel_dp); +struct intel_connector *intel_connector = intel_dp->attached_connector; + +if (!intel_dp_link_training_clock_recovery (intel_dp)) + goto failure_handling; +if (!intel_dp_link_training_channel_equalization (intel_dp)) + goto failure_handling; + +DRM_DEBUG_KMS ("Link Training Passed at Link Rate = %d, Lane count = %d" , + intel_dp->link_rate, intel_dp->lane_count); +return ; + + failure_handling: +DRM_DEBUG_KMS ("Link Training failed at link rate = %d, lane count = %d" , + intel_dp->link_rate, intel_dp->lane_count); +if (!intel_dp_get_link_train_fallback_values (intel_dp, + intel_dp->link_rate, + intel_dp->lane_count)) +/* Schedule a Hotplug Uevent to userspace to start modeset */ +schedule_work (&intel_connector->modeset_retry_work); +return ; }drivers/gpu/drm/i915/intel_drv.h +3
-0@@ -315,6 +315,9 @@ struct intel_connector {void *port;/* store this opaque as its illegal to dereference it */ struct intel_dp *mst_port; + +/* Work struct to schedule a uevent on link train failure */ +struct work_struct modeset_retry_work; };struct dpll {Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Patchwork
CI✗ Fi.CI.BAT: failure for drm: link status property and DP link training failure handling
In reply to Jani Nikula== Series Details ==
== Series Details ==
Series: drm: link status property and DP link training failure handling
URL : https://patchwork.freedesktop.org/series/16912/
State : failure
== Summary ==
Series 16912v1 drm: link status property and DP link training failure handling
https://patchwork.freedesktop.org/api/1.0/series/16912/revisions/1/mbox/
Test drv_module_reload:
Subgroup basic-reload:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup basic-reload-final:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup basic-reload-inject:
dmesg-warn -> INCOMPLETE (fi-skl-6700k)
pass -> DMESG-WARN (fi-kbl-7500u)
pass -> DMESG-WARN (fi-ilk-650)
Test gem_busy:
Subgroup basic-hang-default:
fail -> PASS (fi-hsw-4770r)
Test gem_exec_suspend:
Subgroup basic-s3:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup basic-s4-devices:
pass -> DMESG-WARN (fi-ilk-650)
Test gem_flink_basic:
Subgroup bad-open:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup basic:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup double-flink:
pass -> DMESG-WARN (fi-ilk-650)
Test kms_addfb_basic:
Subgroup addfb25-bad-modifier:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup addfb25-framebuffer-vs-set-tiling:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup addfb25-modifier-no-flag:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup addfb25-x-tiled:
pass -> DMESG-WARN (fi-ilk-650)
Test kms_busy:
Subgroup basic-flip-default-a:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup basic-flip-default-b:
pass -> DMESG-WARN (fi-ilk-650)
Test kms_cursor_legacy:
Subgroup basic-busy-flip-before-cursor-legacy:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup basic-busy-flip-before-cursor-varying-size:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup basic-flip-after-cursor-legacy:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup basic-flip-after-cursor-varying-size:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup basic-flip-before-cursor-legacy:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup basic-flip-before-cursor-varying-size:
pass -> DMESG-WARN (fi-ilk-650)
Test kms_flip:
Subgroup basic-flip-vs-dpms:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup basic-flip-vs-modeset:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup basic-flip-vs-wf_vblank:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup basic-plain-flip:
pass -> DMESG-WARN (fi-ilk-650)
Test kms_pipe_crc_basic:
Subgroup bad-nb-words-1:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup bad-nb-words-3:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup bad-pipe:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup bad-source:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup hang-read-crc-pipe-a:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup hang-read-crc-pipe-b:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup nonblocking-crc-pipe-a:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup nonblocking-crc-pipe-a-frame-sequence:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup nonblocking-crc-pipe-b:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup nonblocking-crc-pipe-b-frame-sequence:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup read-crc-pipe-a:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup read-crc-pipe-a-frame-sequence:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup read-crc-pipe-b:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup read-crc-pipe-b-frame-sequence:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup suspend-read-crc-pipe-a:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup suspend-read-crc-pipe-b:
pass -> DMESG-WARN (fi-ilk-650)
Test kms_setmode:
Subgroup basic-clone-single-crtc:
pass -> DMESG-WARN (fi-ilk-650)WARNING: Long output truncated
e5dd5ad8937cd82ea07bde765bde5b4c09d81dcb drm-tip: 2016y-12m-16d-09h-25m-11s UTC integration manifest db2460d drm/i915: Implement Link Rate fallback on Link training failure
d835410 drm: Add a new connector atomic property for link status
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3307/
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 7 lines · Jani Nikula
The two remaining patches from [1], rebased.
BR,
Jani.
Just for the record, I think the only thing missing here is the Xorg review on the -modesetting patch. As soon as we have that I can vacuum this up (probably best through drm-misc, but not sure). -Daniel
Quoted 24 lines
Manasi Navare (2):
drm: Add a new connector atomic property for link status drm/i915: Implement Link Rate fallback on Link training failuredrivers/gpu/drm/drm_atomic.c | 16 +++++++++ drivers/gpu/drm/drm_atomic_helper.c | 15 ++++++++ drivers/gpu/drm/drm_connector.c | 52 +++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_dp.c | 27 ++++++++++++++ drivers/gpu/drm/i915/intel_dp_link_training.c | 22 ++++++++++-- drivers/gpu/drm/i915/intel_drv.h | 3 ++ include/drm/drm_connector.h | 19 ++++++++++ include/drm/drm_mode_config.h | 5 +++ include/uapi/drm/drm_mode.h | 4 +++ 9 files changed, 161 insertions(+), 2 deletions(-)Signature · 6 lines
2.1.4 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 7 lines · Daniel Vetter
Quoted 6 lines · Jani Nikula
The two remaining patches from [1], rebased.
BR,
Jani.
Quoted 5 lines
Quoted 1 line
Just for the record, I think the only thing missing here is the Xorg review on the -modesetting patch. As soon as we have that I can vacuum this up (probably best through drm-misc, but not sure).
Yeah I rebased this (and provided a debug hack privately) so Martin can test the modesetting changes.
BR,
Jani.
Quoted 26 lines
-Daniel
Quoted 24 lines
Manasi Navare (2):
drm: Add a new connector atomic property for link status drm/i915: Implement Link Rate fallback on Link training failuredrivers/gpu/drm/drm_atomic.c | 16 +++++++++ drivers/gpu/drm/drm_atomic_helper.c | 15 ++++++++ drivers/gpu/drm/drm_connector.c | 52 +++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_dp.c | 27 ++++++++++++++ drivers/gpu/drm/i915/intel_dp_link_training.c | 22 ++++++++++-- drivers/gpu/drm/i915/intel_drv.h | 3 ++ include/drm/drm_connector.h | 19 ++++++++++ include/drm/drm_mode_config.h | 5 +++ include/uapi/drm/drm_mode.h | 4 +++ 9 files changed, 161 insertions(+), 2 deletions(-)Signature · 6 lines
2.1.4 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 14 lines · Jani Nikula
Quoted 7 lines · Daniel Vetter
Quoted 6 lines · Jani Nikula
The two remaining patches from [1], rebased.
BR,
Jani.Quoted 5 lines
Quoted 1 line
Just for the record, I think the only thing missing here is the Xorg review on the -modesetting patch. As soon as we have that I can vacuum this up (probably best through drm-misc, but not sure).
Quoted 7 lines
Yeah I rebased this (and provided a debug hack privately) so Martin can test the modesetting changes.
BR,
Jani.
I tested the -modesetting patch, which Martin had provided to Manasi, with a compliance testing device (DPR-120) that can simulate link training failure. The link rate correctly lowered after the link_status property was set to BAD by the kernel and the userspace responded with a modeset.
One thing that was not straight forward to figure out was I had to boot with i915.nuclear_pageflip=1. Is it documented somewhere that the property needs DRIVER_ATOMIC to be set, or is it implicit?
The other thing I had trouble with -modesetting was, there was no modeset following a long pulse from the sink at the begging of the test. I had to force a modeset by changing the resolution so that the link training path is executed. However, the link training failure induced a modeset without any intervention.
-DK
Quoted 19 lines
Quoted 19 lines
-Daniel
Quoted 17 lines
Manasi Navare (2):
drm: Add a new connector atomic property for link status drm/i915: Implement Link Rate fallback on Link training failuredrivers/gpu/drm/drm_atomic.c | 16 +++++++++ drivers/gpu/drm/drm_atomic_helper.c | 15 ++++++++ drivers/gpu/drm/drm_connector.c | 52 +++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_dp.c | 27 ++++++++++++++ drivers/gpu/drm/i915/intel_dp_link_training.c | 22 ++++++++++-- drivers/gpu/drm/i915/intel_drv.h | 3 ++ include/drm/drm_connector.h | 19 ++++++++++ include/drm/drm_mode_config.h | 5 +++ include/uapi/drm/drm_mode.h | 4 +++ 9 files changed, 161 insertions(+), 2 deletions(-)
Quoted 8 lines
Quoted 7 lines
Quoted 7 lines
Signature · 6 lines
2.1.4 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 29 lines · Dhinakaran
Quoted 14 lines · Jani Nikula
Quoted 7 lines · Daniel Vetter
Quoted 6 lines · Jani Nikula
The two remaining patches from [1], rebased.
BR,
Jani.Quoted 5 lines
Quoted 1 line
Just for the record, I think the only thing missing here is the Xorg review on the -modesetting patch. As soon as we have that I can vacuum this up (probably best through drm-misc, but not sure).
Quoted 7 lines
Yeah I rebased this (and provided a debug hack privately) so Martin can test the modesetting changes.
BR,
Jani.I tested the -modesetting patch, which Martin had provided to Manasi, with a compliance testing device (DPR-120) that can simulate link training failure. The link rate correctly lowered after the
link_statusproperty was set to BAD by the kernel and the userspace responded with a modeset.
Quoted 3 lines
One thing that was not straight forward to figure out was I had to boot with i915.nuclear_pageflip=1. Is it documented somewhere that the property needs
DRIVER_ATOMICto be set, or is it implicit?
It should work without DRIVER_ATOMIC. At least the property should be exposed ... If this does only work with DRIVER_ATOMIC set then we have a bug somewhere. Can you pls try to figure out why it doesn't work?
Quoted 5 lines
The other thing I had trouble with -modesetting was, there was no modeset following a long pulse from the sink at the begging of the test. I had to force a modeset by changing the resolution so that the link training path is executed. However, the link training failure induced a modeset without any intervention.
Sounds roughly like how it's supposed to work. For real mode configuration changes the desktop environment is supposed to set the mode it wants, by listening to the xrandr hotplug event. That's not the same as the udev hotplug event. You can listen for the xrandr hotplug event using
$ xev -event randr
Cheers, Daniel
Quoted 32 lines
-DK
Quoted 27 lines
Quoted 26 lines
-Daniel
Quoted 24 lines
Manasi Navare (2):
drm: Add a new connector atomic property for link status drm/i915: Implement Link Rate fallback on Link training failuredrivers/gpu/drm/drm_atomic.c | 16 +++++++++ drivers/gpu/drm/drm_atomic_helper.c | 15 ++++++++ drivers/gpu/drm/drm_connector.c | 52 +++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_dp.c | 27 ++++++++++++++ drivers/gpu/drm/i915/intel_dp_link_training.c | 22 ++++++++++-- drivers/gpu/drm/i915/intel_drv.h | 3 ++ include/drm/drm_connector.h | 19 ++++++++++ include/drm/drm_mode_config.h | 5 +++ include/uapi/drm/drm_mode.h | 4 +++ 9 files changed, 161 insertions(+), 2 deletions(-)Signature · 6 lines
2.1.4 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 30 lines · Daniel Vetter
Quoted 29 lines · Dhinakaran
Quoted 14 lines · Jani Nikula
Quoted 7 lines · Daniel Vetter
Quoted 6 lines · Jani Nikula
The two remaining patches from [1], rebased.
BR,
Jani.Quoted 5 lines
Quoted 1 line
Just for the record, I think the only thing missing here is the Xorg review on the -modesetting patch. As soon as we have that I can vacuum this up (probably best through drm-misc, but not sure).
Quoted 7 lines
Yeah I rebased this (and provided a debug hack privately) so Martin can test the modesetting changes.
BR,
Jani.I tested the -modesetting patch, which Martin had provided to Manasi, with a compliance testing device (DPR-120) that can simulate link training failure. The link rate correctly lowered after the
link_statusproperty was set to BAD by the kernel and the userspace responded with a modeset.
Quoted 8 lines
Quoted 3 lines
One thing that was not straight forward to figure out was I had to boot with i915.nuclear_pageflip=1. Is it documented somewhere that the property needs
DRIVER_ATOMICto be set, or is it implicit?It should work without
DRIVER_ATOMIC. At least the property should be exposed ... If this does only work withDRIVER_ATOMICset then we have a bug somewhere. Can you pls try to figure out why it doesn't work?
The property is exposed even without DRIVER_ATOMIC set, but the value is always GOOD (0).
We set connector->state->link_status to BAD when link training fails but the getconnector() ioctl ends up reading obj->properties->values[i] if DRIVER_ATOMIC is NOT set. But with DRIVER_ATOMIC set, getconnector() calls into drm_atomic_connector_get_property() and retrieves the value stored in connector->state->link_status.
Quoted 6 lines
Quoted 5 lines
The other thing I had trouble with -modesetting was, there was no modeset following a long pulse from the sink at the begging of the test. I had to force a modeset by changing the resolution so that the link training path is executed. However, the link training failure induced a modeset without any intervention.
Quoted 7 lines
Sounds roughly like how it's supposed to work. For real mode configuration changes the desktop environment is supposed to set the mode it wants, by listening to the xrandr hotplug event. That's not the same as the udev hotplug event. You can listen for the xrandr hotplug event using
$ xev -event randr
Got it, -modesetting does indeed send out the hotplug events upon hotplug.
-DK
Quoted 35 lines
Cheers, Daniel
Quoted 32 lines
-DK
Quoted 27 lines
Quoted 26 lines
-Daniel
Quoted 24 lines
Manasi Navare (2):
drm: Add a new connector atomic property for link status drm/i915: Implement Link Rate fallback on Link training failuredrivers/gpu/drm/drm_atomic.c | 16 +++++++++ drivers/gpu/drm/drm_atomic_helper.c | 15 ++++++++ drivers/gpu/drm/drm_connector.c | 52 +++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_dp.c | 27 ++++++++++++++ drivers/gpu/drm/i915/intel_dp_link_training.c | 22 ++++++++++-- drivers/gpu/drm/i915/intel_drv.h | 3 ++ include/drm/drm_connector.h | 19 ++++++++++ include/drm/drm_mode_config.h | 5 +++ include/uapi/drm/drm_mode.h | 4 +++ 9 files changed, 161 insertions(+), 2 deletions(-)Signature · 6 lines
2.1.4 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 40 lines · Dhinakaran
Quoted 30 lines · Daniel Vetter
Quoted 29 lines · Dhinakaran
Quoted 14 lines · Jani Nikula
Quoted 7 lines · Daniel Vetter
Quoted 6 lines · Jani Nikula
The two remaining patches from [1], rebased.
BR,
Jani.Quoted 5 lines
Quoted 1 line
Just for the record, I think the only thing missing here is the Xorg review on the -modesetting patch. As soon as we have that I can vacuum this up (probably best through drm-misc, but not sure).
Quoted 7 lines
Yeah I rebased this (and provided a debug hack privately) so Martin can test the modesetting changes.
BR,
Jani.I tested the -modesetting patch, which Martin had provided to Manasi, with a compliance testing device (DPR-120) that can simulate link training failure. The link rate correctly lowered after the
link_statusproperty was set to BAD by the kernel and the userspace responded with a modeset.Quoted 8 lines
Quoted 3 lines
One thing that was not straight forward to figure out was I had to boot with i915.nuclear_pageflip=1. Is it documented somewhere that the property needs
DRIVER_ATOMICto be set, or is it implicit?It should work without
DRIVER_ATOMIC. At least the property should be exposed ... If this does only work withDRIVER_ATOMICset then we have a bug somewhere. Can you pls try to figure out why it doesn't work?
Quoted 7 lines
The property is exposed even without
DRIVER_ATOMICset, but the value is always GOOD (0).
We setconnector->state->link_statusto BAD when link training fails but thegetconnector()ioctl ends up reading obj->properties->values[i] ifDRIVER_ATOMICis NOT set. But withDRIVER_ATOMICset,getconnector()calls intodrm_atomic_connector_get_property()and retrieves the value stored inconnector->state->link_status.
That sounds like a bug in the getconnector code. This needs the same treatment as other places, see e.g.
commit d3a46183db97536a53df5de6b556bd61197842b2
Author: Daniel Vetter <[email protected]> Date: Wed Jun 8 14:19:15 2016 +0200
drm: Replace fb_helper->atomic with mode_config->atomic_commit
I think it'd be good to extract this check into a drm_drv_uses_atomic_modeset to better self-document the code, roll it out to all existing places that check for atomic_commit and then also roll it out to the getproperty functions (for connectors, planes and crtcs).
Quoted 11 lines
Quoted 11 lines
Quoted 5 lines
The other thing I had trouble with -modesetting was, there was no modeset following a long pulse from the sink at the begging of the test. I had to force a modeset by changing the resolution so that the link training path is executed. However, the link training failure induced a modeset without any intervention.
Sounds roughly like how it's supposed to work. For real mode configuration changes the desktop environment is supposed to set the mode it wants, by listening to the xrandr hotplug event. That's not the same as the udev hotplug event. You can listen for the xrandr hotplug event using
Quoted 5 lines
Quoted 2 lines
$ xev -event randrGot it, -modesetting does indeed send out the hotplug events upon hotplug.
Excellent, so at least that's all working well.
-Daniel
Quoted 40 lines
-DK
Quoted 35 lines
Cheers, Daniel
Quoted 32 lines
-DK
Quoted 27 lines
Quoted 26 lines
-Daniel
Quoted 24 lines
Manasi Navare (2):
drm: Add a new connector atomic property for link status drm/i915: Implement Link Rate fallback on Link training failuredrivers/gpu/drm/drm_atomic.c | 16 +++++++++ drivers/gpu/drm/drm_atomic_helper.c | 15 ++++++++ drivers/gpu/drm/drm_connector.c | 52 +++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_dp.c | 27 ++++++++++++++ drivers/gpu/drm/i915/intel_dp_link_training.c | 22 ++++++++++-- drivers/gpu/drm/i915/intel_drv.h | 3 ++ include/drm/drm_connector.h | 19 ++++++++++ include/drm/drm_mode_config.h | 5 +++ include/uapi/drm/drm_mode.h | 4 +++ 9 files changed, 161 insertions(+), 2 deletions(-)Signature · 6 lines
2.1.4 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 56 lines · Daniel Vetter
Quoted 40 lines · Dhinakaran
Quoted 30 lines · Daniel Vetter
Quoted 29 lines · Dhinakaran
Quoted 14 lines · Jani Nikula
Quoted 7 lines · Daniel Vetter
Quoted 6 lines · Jani Nikula
The two remaining patches from [1], rebased.
BR,
Jani.Quoted 5 lines
Quoted 1 line
Just for the record, I think the only thing missing here is the Xorg review on the -modesetting patch. As soon as we have that I can vacuum this up (probably best through drm-misc, but not sure).
Quoted 7 lines
Yeah I rebased this (and provided a debug hack privately) so Martin can test the modesetting changes.
BR,
Jani.I tested the -modesetting patch, which Martin had provided to Manasi, with a compliance testing device (DPR-120) that can simulate link training failure. The link rate correctly lowered after the
link_statusproperty was set to BAD by the kernel and the userspace responded with a modeset.Quoted 8 lines
Quoted 3 lines
One thing that was not straight forward to figure out was I had to boot with i915.nuclear_pageflip=1. Is it documented somewhere that the property needs
DRIVER_ATOMICto be set, or is it implicit?It should work without
DRIVER_ATOMIC. At least the property should be exposed ... If this does only work withDRIVER_ATOMICset then we have a bug somewhere. Can you pls try to figure out why it doesn't work?Quoted 7 lines
The property is exposed even without
DRIVER_ATOMICset, but the value is always GOOD (0).
We setconnector->state->link_statusto BAD when link training fails but thegetconnector()ioctl ends up reading obj->properties->values[i] ifDRIVER_ATOMICis NOT set. But withDRIVER_ATOMICset,getconnector()calls intodrm_atomic_connector_get_property()and retrieves the value stored inconnector->state->link_status.That sounds like a bug in the getconnector code. This needs the same treatment as other places, see e.g.
commit
d3a46183db97536a53df5de6b556bd61197842b2Author: Daniel Vetter <[email protected]> Date: Wed Jun 8 14:19:15 2016 +0200
Quoted 12 lines
drm: Replace fb_helper->atomic with mode_config->atomic_commitI think it'd be good to extract this check into a
drm_drv_uses_atomic_modesetto better self-document the code, roll it out to all existing places that check foratomic_commitand then also roll it out to the getproperty functions (for connectors, planes and crtcs).Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
The other thing I had trouble with -modesetting was, there was no modeset following a long pulse from the sink at the begging of the test. I had to force a modeset by changing the resolution so that the link training path is executed. However, the link training failure induced a modeset without any intervention.
This seems little strange because in case of SNA driver, it does respond with a full reprobe and a modeset following a long pulse from the sink at the beginning of the test and no forcing through xrandr was required.
I think the modesetting driver should also behave the same and should not need any forcing of the modeset, but should be able to respond to the hotplug/long pulse at the beginning of each test which is trying to simulate the connect/disconnect of the DP cable.
My guess is that the modesetting driver was probably not built with hotplug support which would cause it to not respond to the long pulse at the beginning of the test.
Chris/Martin/Daniel , any thoughts?
Manasi
Quoted 47 lines
Quoted 11 lines
Quoted 8 lines
Sounds roughly like how it's supposed to work. For real mode configuration changes the desktop environment is supposed to set the mode it wants, by listening to the xrandr hotplug event. That's not the same as the udev hotplug event. You can listen for the xrandr hotplug event using
$ xev -event randrGot it, -modesetting does indeed send out the hotplug events upon hotplug.
Excellent, so at least that's all working well.
-DanielQuoted 32 lines
-DK
Quoted 28 lines
Cheers, Daniel
Quoted 26 lines
-DK
Quoted 22 lines
Quoted 22 lines
-Daniel
Quoted 20 lines
Manasi Navare (2):
drm: Add a new connector atomic property for link status drm/i915: Implement Link Rate fallback on Link training failuredrivers/gpu/drm/drm_atomic.c | 16 +++++++++ drivers/gpu/drm/drm_atomic_helper.c | 15 ++++++++ drivers/gpu/drm/drm_connector.c | 52 +++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_dp.c | 27 ++++++++++++++ drivers/gpu/drm/i915/intel_dp_link_training.c | 22 ++++++++++-- drivers/gpu/drm/i915/intel_drv.h | 3 ++ include/drm/drm_connector.h | 19 ++++++++++ include/drm/drm_mode_config.h | 5 +++ include/uapi/drm/drm_mode.h | 4 +++ 9 files changed, 161 insertions(+), 2 deletions(-)Signature · 1 line
2.1.4
Quoted 17 lines
Quoted 8 lines
Quoted 7 lines
Quoted 6 lines
Quoted 5 lines
Quoted 4 lines
Quoted 4 lines
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-develSignature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 7 lines · Daniel Vetter
Quoted 6 lines · Jani Nikula
The two remaining patches from [1], rebased.
BR,
Jani.
Quoted 5 lines
Quoted 1 line
Just for the record, I think the only thing missing here is the Xorg review on the -modesetting patch. As soon as we have that I can vacuum this up (probably best through drm-misc, but not sure).
Hey Daniel,
I tested again on Monday -modesetting with the patch from Jani to inject faults and did manage to get both the link-status BAD and a lower resolution got select dynamically when running KDE. For the latter, I however needed the following patch: https://patchwork.kernel.org/patch/9491869/
Now, that being said, Jani's patch just prevents a new modeset to work, it does not tear down the current mode. This may be the reason why I do not manage to get a black screen after > 3 failures (and already a 1024x768 resolution).
I however need to do more testing when running without a DE (straight X + twm and xterm). Indeed, when I hotplug my DP screen, it gets to the native resolution automatically without me asking for it using xrandr. Also, the mode that is set does not seem to go through intel_dp_start_link_train (what the heck?), so I do not get any failure and I cannot induce one :s
Because of this, I am a little uneasy and cannot say for sure that my patch for -modesetting is correct. I am hoping that Manasi will get a good testing rig to validate everything from end-to-end. At the very least, -modesetting seems to do a reasonable job.
Sorry for keeping you out of the loop, but I was hoping to have a more clear-cut GOOD/BAD, but I seem to be mostly at a road block. Manasi told me it would help if I described it here, so here I am. I will continue assisting Manasi in her validation work!
Martin
PS: My kernel tree is here: https://cgit.freedesktop.org/~mperes/linux
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 27 lines · Martin Peres
Quoted 7 lines · Daniel Vetter
Quoted 6 lines · Jani Nikula
The two remaining patches from [1], rebased.
BR,
Jani.Quoted 5 lines
Quoted 1 line
Just for the record, I think the only thing missing here is the Xorg review on the -modesetting patch. As soon as we have that I can vacuum this up (probably best through drm-misc, but not sure).
Hey Daniel,
I tested again on Monday -modesetting with the patch from Jani to inject faults and did manage to get both the link-status BAD and a lower resolution got select dynamically when running KDE. For the latter, I however needed the following patch:
https://patchwork.kernel.org/patch/9491869/Now, that being said, Jani's patch just prevents a new modeset to work, it does not tear down the current mode. This may be the reason why I do not manage to get a black screen after > 3 failures (and already a 1024x768 resolution).
Quoted 6 lines
I however need to do more testing when running without a DE (straight X
+ twm and xterm). Indeed, when I hotplug my DP screen, it gets to the native resolution automatically without me asking for it using xrandr. Also, the mode that is set does not seem to go throughintel_dp_start_link_train(what the heck?), so I do not get any failure and I cannot induce one :s
Huh, does your X + twm actually respond to hotplugs?
BR,
Jani.
Quoted 18 lines
Because of this, I am a little uneasy and cannot say for sure that my patch for -modesetting is correct. I am hoping that Manasi will get a good testing rig to validate everything from end-to-end. At the very least, -modesetting seems to do a reasonable job.
Sorry for keeping you out of the loop, but I was hoping to have a more clear-cut GOOD/BAD, but I seem to be mostly at a road block. Manasi told me it would help if I described it here, so here I am. I will continue assisting Manasi in her validation work!
Martin
PS: My kernel tree is here: https://cgit.freedesktop.org/~mperes/linux
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 28 lines · Jani Nikula
Quoted 27 lines · Martin Peres
Quoted 7 lines · Daniel Vetter
Quoted 6 lines · Jani Nikula
The two remaining patches from [1], rebased.
BR,
Jani.Quoted 5 lines
Quoted 1 line
Just for the record, I think the only thing missing here is the Xorg review on the -modesetting patch. As soon as we have that I can vacuum this up (probably best through drm-misc, but not sure).
Hey Daniel,
I tested again on Monday -modesetting with the patch from Jani to inject faults and did manage to get both the link-status BAD and a lower resolution got select dynamically when running KDE. For the latter, I however needed the following patch:
https://patchwork.kernel.org/patch/9491869/Now, that being said, Jani's patch just prevents a new modeset to work, it does not tear down the current mode. This may be the reason why I do not manage to get a black screen after > 3 failures (and already a 1024x768 resolution).
Quoted 9 lines
Quoted 6 lines
I however need to do more testing when running without a DE (straight X
+ twm and xterm). Indeed, when I hotplug my DP screen, it gets to the native resolution automatically without me asking for it using xrandr. Also, the mode that is set does not seem to go throughintel_dp_start_link_train(what the heck?), so I do not get any failure and I cannot induce one :sHuh, does your X + twm actually respond to hotplugs?
No, that was the point of the test :) I just wanted to see the screen turn black but it did not because even if the link training fails, i915 keeps on going and pushes the pixels out constantly. My screen was good-enough to pick it up and display it without complaining ... which is really surprising for DP, isn't it?
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 36 lines · Martin Peres
Quoted 35 lines · Jani Nikula
Quoted 33 lines · Martin Peres
Quoted 7 lines · Daniel Vetter
Quoted 6 lines · Jani Nikula
The two remaining patches from [1], rebased.
BR,
Jani.Quoted 5 lines
Quoted 1 line
Just for the record, I think the only thing missing here is the Xorg review on the -modesetting patch. As soon as we have that I can vacuum this up (probably best through drm-misc, but not sure).
Hey Daniel,
I tested again on Monday -modesetting with the patch from Jani to inject faults and did manage to get both the link-status BAD and a lower resolution got select dynamically when running KDE. For the latter, I however needed the following patch:
https://patchwork.kernel.org/patch/9491869/Now, that being said, Jani's patch just prevents a new modeset to work, it does not tear down the current mode. This may be the reason why I do not manage to get a black screen after > 3 failures (and already a 1024x768 resolution).
I however need to do more testing when running without a DE (straight X
+ twm and xterm). Indeed, when I hotplug my DP screen, it gets to the native resolution automatically without me asking for it using xrandr. Also, the mode that is set does not seem to go throughintel_dp_start_link_train(what the heck?), so I do not get any failure and I cannot induce one :s
Quoted 8 lines
Quoted 2 lines
Huh, does your X + twm actually respond to hotplugs?
No, that was the point of the test :) I just wanted to see the screen turn black but it did not because even if the link training fails, i915 keeps on going and pushes the pixels out constantly. My screen was good-enough to pick it up and display it without complaining ... which is really surprising for DP, isn't it?
And this, my friend, is why we plunge on in spite of errors, instead of failing fast and bailing out. If you were a normal user, you'd *appreciate* not having a black screen! ;)
BR,
Jani.
Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 46 lines · Jani Nikula
Quoted 36 lines · Martin Peres
Quoted 35 lines · Jani Nikula
Quoted 33 lines · Martin Peres
Quoted 7 lines · Daniel Vetter
Quoted 6 lines · Jani Nikula
The two remaining patches from [1], rebased.
BR,
Jani.Quoted 5 lines
Quoted 1 line
Just for the record, I think the only thing missing here is the Xorg review on the -modesetting patch. As soon as we have that I can vacuum this up (probably best through drm-misc, but not sure).
Hey Daniel,
I tested again on Monday -modesetting with the patch from Jani to inject faults and did manage to get both the link-status BAD and a lower resolution got select dynamically when running KDE. For the latter, I however needed the following patch:
https://patchwork.kernel.org/patch/9491869/Now, that being said, Jani's patch just prevents a new modeset to work, it does not tear down the current mode. This may be the reason why I do not manage to get a black screen after > 3 failures (and already a 1024x768 resolution).
I however need to do more testing when running without a DE (straight X
+ twm and xterm). Indeed, when I hotplug my DP screen, it gets to the native resolution automatically without me asking for it using xrandr. Also, the mode that is set does not seem to go throughintel_dp_start_link_train(what the heck?), so I do not get any failure and I cannot induce one :sQuoted 8 lines
Quoted 2 lines
Huh, does your X + twm actually respond to hotplugs?
No, that was the point of the test :) I just wanted to see the screen turn black but it did not because even if the link training fails, i915 keeps on going and pushes the pixels out constantly. My screen was good-enough to pick it up and display it without complaining ... which is really surprising for DP, isn't it?
Quoted 3 lines
And this, my friend, is why we plunge on in spite of errors, instead of failing fast and bailing out. If you were a normal user, you'd *appreciate* not having a black screen! ;)
I definitely would *love* this approach. Just was confusing ;) Thanks a lot for yesterday's discussion!
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 27 lines · Martin Peres
Quoted 7 lines · Daniel Vetter
Quoted 6 lines · Jani Nikula
The two remaining patches from [1], rebased.
BR,
Jani.Quoted 5 lines
Quoted 1 line
Just for the record, I think the only thing missing here is the Xorg review on the -modesetting patch. As soon as we have that I can vacuum this up (probably best through drm-misc, but not sure).
Hey Daniel,
I tested again on Monday -modesetting with the patch from Jani to inject faults and did manage to get both the link-status BAD and a lower resolution got select dynamically when running KDE. For the latter, I however needed the following patch:
https://patchwork.kernel.org/patch/9491869/Now, that being said, Jani's patch just prevents a new modeset to work, it does not tear down the current mode. This may be the reason why I do not manage to get a black screen after > 3 failures (and already a 1024x768 resolution).
Quoted 6 lines
I however need to do more testing when running without a DE (straight X
+ twm and xterm). Indeed, when I hotplug my DP screen, it gets to the native resolution automatically without me asking for it using xrandr. Also, the mode that is set does not seem to go throughintel_dp_start_link_train(what the heck?), so I do not get any failure and I cannot induce one :s
Presumably you're already pushing pixels out at the same resolution and the monitor just syncs up to it. I think depending on the monitor that might happen w/ or w/o link retraining.
Signature · 6 lines
Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 35 lines · Ville Syrjälä
Quoted 27 lines · Martin Peres
Quoted 7 lines · Daniel Vetter
Quoted 6 lines · Jani Nikula
The two remaining patches from [1], rebased.
BR,
Jani.Quoted 5 lines
Quoted 1 line
Just for the record, I think the only thing missing here is the Xorg review on the -modesetting patch. As soon as we have that I can vacuum this up (probably best through drm-misc, but not sure).
Hey Daniel,
I tested again on Monday -modesetting with the patch from Jani to inject faults and did manage to get both the link-status BAD and a lower resolution got select dynamically when running KDE. For the latter, I however needed the following patch:
https://patchwork.kernel.org/patch/9491869/Now, that being said, Jani's patch just prevents a new modeset to work, it does not tear down the current mode. This may be the reason why I do not manage to get a black screen after > 3 failures (and already a 1024x768 resolution).
Quoted 6 lines
I however need to do more testing when running without a DE (straight X
+ twm and xterm). Indeed, when I hotplug my DP screen, it gets to the native resolution automatically without me asking for it using xrandr. Also, the mode that is set does not seem to go throughintel_dp_start_link_train(what the heck?), so I do not get any failure and I cannot induce one :s
Quoted 3 lines
Presumably you're already pushing pixels out at the same resolution and the monitor just syncs up to it. I think depending on the monitor that might happen w/ or w/o link retraining.
Thanks a lot Ville, this is exactly what was going on!
I worked yesterday and today on a tool that would respond to randr events and perform a modeset to the highest mode available. As far as I can tell, it works exactly as expected!
I do not use the link-status much in -modesetting, aside from doing a fast-retraining in case of a BAD state, but I definitely proved that this kernel tree[0], this xserver patch[1] and this tool[2] are enough to handle gracefully link degradations and prune modes as they become unstable.
Please review the general idea and I will send the cleaned -modesetting patch as soon as we all agree. I will let Manasi do more testing with it and report back to us.
Thanks everyone for your help!
Martin
[0] https://cgit.freedesktop.org/~mperes/linux/log/?h=dp-compliance
[1] https://cgit.freedesktop.org/~mperes/xserver/commit/?h=dpcompliance&id=ce03d856c1121ca475cc88b9c64c2d2b95fa20bc
[2] https://cgit.freedesktop.org/~mperes/auto-randr
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 52 lines · Martin Peres
Quoted 35 lines · Ville Syrjälä
Quoted 27 lines · Martin Peres
Quoted 7 lines · Daniel Vetter
Quoted 6 lines · Jani Nikula
The two remaining patches from [1], rebased.
BR,
Jani.Quoted 5 lines
Quoted 1 line
Just for the record, I think the only thing missing here is the Xorg review on the -modesetting patch. As soon as we have that I can vacuum this up (probably best through drm-misc, but not sure).
Hey Daniel,
I tested again on Monday -modesetting with the patch from Jani to inject faults and did manage to get both the link-status BAD and a lower resolution got select dynamically when running KDE. For the latter, I however needed the following patch:
https://patchwork.kernel.org/patch/9491869/Now, that being said, Jani's patch just prevents a new modeset to work, it does not tear down the current mode. This may be the reason why I do not manage to get a black screen after > 3 failures (and already a 1024x768 resolution).
Quoted 6 lines
I however need to do more testing when running without a DE (straight X
+ twm and xterm). Indeed, when I hotplug my DP screen, it gets to the native resolution automatically without me asking for it using xrandr. Also, the mode that is set does not seem to go throughintel_dp_start_link_train(what the heck?), so I do not get any failure and I cannot induce one :sQuoted 3 lines
Presumably you're already pushing pixels out at the same resolution and the monitor just syncs up to it. I think depending on the monitor that might happen w/ or w/o link retraining.
Thanks a lot Ville, this is exactly what was going on!
I worked yesterday and today on a tool that would respond to randr events and perform a modeset to the highest mode available. As far as I can tell, it works exactly as expected!
I do not use the link-status much in -modesetting, aside from doing a fast-retraining in case of a BAD state, but I definitely proved that this kernel tree[0], this xserver patch[1] and this tool[2] are enough to handle gracefully link degradations and prune modes as they become unstable.
Quoted 7 lines
Please review the general idea and I will send the cleaned -modesetting patch as soon as we all agree. I will let Manasi do more testing with it and report back to us.
Thanks everyone for your help!
Martin
Thanks for the modesetting patch and the tool that listens to the hotplug events. I testing this whole setup with Xserver and KDE desktop environment with this tool running and verified that the DPR 120 link rate degradation test passes. From the dmesg logs, I verified that the link training fails first time, it sends the hotplug sets the Link status to BAD, Xserver handles this BAD link status by redoing a modeset and retrain the link at lower link rate. In this it kept the resolution same with lower (18) bpp.
So the link status property is working as expected with modesetting driver. The tool was only required to respond with a modeset to the first hotplug event sent at the beginning of the test.
This validates the pending two patches for link status:
https://patchwork.freedesktop.org/series/16912/
Regards
Manasi
Quoted 8 lines
[0] https://cgit.freedesktop.org/~mperes/linux/log/?h=dp-compliance
[1] https://cgit.freedesktop.org/~mperes/xserver/commit/?h=dpcompliance&id=ce03d856c1121ca475cc88b9c64c2d2b95fa20bc
[2] https://cgit.freedesktop.org/~mperes/auto-randr
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel