On Tue, Nov 29, 2016 at 11:30:31PM -0800, Manasi Navare wrote: > 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. > > 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) > > Acked-by: Tony Cheng <tony.cheng@xxxxxxx> > Acked-by: Harry Wentland <harry.wentland@xxxxxxx> > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx> > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > Cc: Sean Paul <seanpaul@xxxxxxxxxxxx> > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx> > --- > drivers/gpu/drm/drm_atomic.c | 10 +++++++ > drivers/gpu/drm/drm_atomic_helper.c | 8 ++++++ > drivers/gpu/drm/drm_connector.c | 54 ++++++++++++++++++++++++++++++++++++- > include/drm/drm_connector.h | 19 +++++++++++++ > include/drm/drm_mode_config.h | 5 ++++ > include/uapi/drm/drm_mode.h | 4 +++ > 6 files changed, 99 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 89737e4..990f013 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -1087,6 +1087,14 @@ int drm_atomic_connector_set_property(struct drm_connector *connector, > * now?) atomic writes to DPMS property: > */ > return -EINVAL; > + } else if (property == config->link_status_property) { > + /* Never downgrade from GOOD to BAD on userspace's request here, > + * only hw issues can do that. > + */ > + if (state->link_status == DRM_LINK_STATUS_GOOD) > + return 0; > + state->link_status = val; > + return 0; > } else if (connector->funcs->atomic_set_property) { > return connector->funcs->atomic_set_property(connector, > state, property, val); > @@ -1135,6 +1143,8 @@ static void drm_atomic_connector_print_state(struct drm_printer *p, > *val = (state->crtc) ? state->crtc->base.id : 0; > } else if (property == config->dpms_property) { > *val = connector->dpms; > + } 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 494680c..962ed66 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -519,6 +519,13 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state, > 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; > + } See my previous review, this looks should imo be moved to a more obvious place than hidden deep down in the callchain. > } > > /* > @@ -2258,6 +2265,7 @@ static int update_output_state(struct drm_atomic_state *state, > NULL); > if (ret) > return ret; > + conn_state->link_status = DRM_LINK_STATUS_GOOD; Iirc in our discussion we said a small comment here would be good, like /* Make sure legacy setCrtc always re-trains. */ Might also be good to update the kernel-doc of drm_atomic_helper_set_config(). Something like * 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(). * > } > } > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 5a45262..8902367 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 @@ const char *drm_get_subpixel_order_name(enum subpixel_order order) > }; > 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,7 +635,11 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, > * 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. > - * One more from last round: Please leave this whitespace intact ... > + * 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(). ... and add an empty line here. > * Connectors also have one standardized atomic property: > * > * CRTC_ID: > @@ -666,6 +680,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 +1016,37 @@ 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, Hm, tiny new bikeshed: I'd drop the _property postfix here. The function name is already really long as-is, and the fact that the link status is exposed as a property is kinda irrelevant for kernel driver writers. But up to you. -Daniel > + 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 34f9741..53c5f70 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 > @@ -213,6 +224,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; > }; > > @@ -773,6 +790,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 bf9991b..86faee4 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 728790b..309c478 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -123,6 +123,10 @@ > #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; > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel