| Message ID | 0182487051aa9f1594820e35a4853de2f8747b4e.1481883920.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Dec 16, 2016 at 12:29:06PM +0200, Jani Nikula wrote: > From: Manasi Navare <manasi.d.navare@intel.com> > > 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) > > Reviewed-by: Sean Paul <seanpaul@chromium.org> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Reviewed-by: Jani Nikula <jani.nikula@intel.com> > Acked-by: Tony Cheng <tony.cheng@amd.com> > Acked-by: Harry Wentland <harry.wentland@amd.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Sean Paul <seanpaul@chromium.org> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> 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 > --- > 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(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index ff38592134f5..91fd8a9a7526 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -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); > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 23767df72615..5e7af639a07a 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -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. > */ > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 5a4526289392..ed7da66ae1ac 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -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) > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index a9b95246e26e..1541dbc957c0 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -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 > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index bf9991b20611..86faee45d011 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -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. > */ > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index ce7efe2e8a5e..8c67fc03d53d 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -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; > -- > 2.1.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Dec 16, 2016 at 12:29:06PM +0200, Jani Nikula wrote: > /** > + * 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, We now have a new warning here when building docs. Please fix in a follow-up patch. -Daniel
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index ff38592134f5..91fd8a9a7526 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -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); diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 23767df72615..5e7af639a07a 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -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. */ diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 5a4526289392..ed7da66ae1ac 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -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) diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index a9b95246e26e..1541dbc957c0 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -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 diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index bf9991b20611..86faee45d011 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -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. */ diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index ce7efe2e8a5e..8c67fc03d53d 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -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;