| Message ID | 1480491033-1923-1-git-send-email-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Sean, could you please review this patch, I have tried to address all the comments from you. Regards Manasi 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@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> > --- > 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; > + } > } > > /* > @@ -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; > } > } > > 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. > - * > + * 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: > * > * 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, > + 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 >
On Thu, Dec 1, 2016 at 1:58 PM, Manasi Navare <manasi.d.navare@intel.com> wrote: > Sean, could you please review this patch, I have tried to address > all the comments from you. > Comments look good to me. Reviewed-by: Sean Paul <seanpaul@chromium.org> Sean > Regards > Manasi > > 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@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> >> --- >> 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; >> + } >> } >> >> /* >> @@ -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; >> } >> } >> >> 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. >> - * >> + * 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: >> * >> * 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, >> + 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 >>
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@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> > --- > 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Dec 02, 2016 at 05:26:35PM +0100, Daniel Vetter wrote: > 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@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> > > --- > > 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. > > > } Yes I have looked at your previous review and this is inside drm_atomic_helper_check_modeset() function just inside the connectors loop. Its not in the handle_conflicting_encoders function. Its weird that in the diff it is showing up here. If you pull the patch you will see that it is written as follows: for_each_connector_in_state(state, connector, connector_state, i) { /* * This only sets crtc->connectors_changed for routing changes, * drivers must set crtc->connectors_changed themselves when * connector properties need to be updated. */ ret = update_connector_routing(state, connector, 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; } } Is this correct? Manasi > > > > /* > > @@ -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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Dec 02, 2016 at 09:48:59AM -0800, Manasi Navare wrote: > On Fri, Dec 02, 2016 at 05:26:35PM +0100, Daniel Vetter wrote: > > 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@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> > > > --- > > > 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. > > > > > } > > Yes I have looked at your previous review and this is inside drm_atomic_helper_check_modeset() > function just inside the connectors loop. Its not in the handle_conflicting_encoders function. > Its weird that in the diff it is showing up here. > If you pull the patch you will see that it is written as follows: > > for_each_connector_in_state(state, connector, connector_state, i) { > /* > * This only sets crtc->connectors_changed for routing changes, > * drivers must set crtc->connectors_changed themselves when > * connector properties need to be updated. > */ > ret = update_connector_routing(state, connector, > 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; > } > } > > Is this correct? Yup, double checked myself, no idea what git diff is doing here thinking it's in a different function ... -Daniel
On Fri, Dec 02, 2016 at 05:26:35PM +0100, Daniel Vetter wrote: > 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@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> > > --- > > 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 > I decided to name it as drm_mode_connector_set_link_status_property() looking at names of other set_property functions like drm_mode_connector_update_edid_property() or drm_mode_connector_set_path_property(). Thats why I think it makes sense to append it with property just to be consistent. Manasi > > + 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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Dec 05, 2016 at 12:33:28PM -0800, Manasi Navare wrote: > On Fri, Dec 02, 2016 at 05:26:35PM +0100, Daniel Vetter wrote: > > 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 > > > > I decided to name it as drm_mode_connector_set_link_status_property() > looking at names of other set_property functions like > drm_mode_connector_update_edid_property() or drm_mode_connector_set_path_property(). > Thats why I think it makes sense to append it with property just to be consistent. Yeah it's fine either way, maybe we could just drop _property from all of them. Really doesn't add much, but these long function names make reasonable code layout a pain. Anyway, nothing that must be done here, agreed. -Daniel
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; + } } /* @@ -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; } } 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. - * + * 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: * * 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, + 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;