| Message ID | 1480984058-552-2-git-send-email-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 05, 2016 at 04:27:35PM -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> Didn't Sean r-b this already? > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> Looks all nice and tidy, great work. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Can be merged as soon as we have an ack from Martin that the -modesetting side is ready to go too. -Daniel > --- > drivers/gpu/drm/drm_atomic.c | 10 +++++++ > drivers/gpu/drm/drm_atomic_helper.c | 15 +++++++++++ > drivers/gpu/drm/drm_connector.c | 53 +++++++++++++++++++++++++++++++++++++ > include/drm/drm_connector.h | 19 +++++++++++++ > include/drm/drm_mode_config.h | 5 ++++ > include/uapi/drm/drm_mode.h | 4 +++ > 6 files changed, 106 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 19d7bcb..70a5152 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..3d6c1ce 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,8 @@ static int update_output_state(struct drm_atomic_state *state, > NULL); > if (ret) > return ret; > + /* Make sure legacy setCrtc always re-trains */ > + conn_state->link_status = DRM_LINK_STATUS_GOOD; > } > } > > @@ -2301,6 +2310,12 @@ static int update_output_state(struct drm_atomic_state *state, > * > * Provides a default crtc set_config handler using the atomic driver interface. > * > + * NOTE: For backwards compatibility with old userspace this automatically > + * resets the "link-status" property to GOOD, to force any link > + * re-training. The SETCRTC ioctl does not define whether an update does > + * need a full modeset or just a plane update, hence we're allowed to do > + * that. See also drm_mode_connector_set_link_status_property(). > + * > * Returns: > * Returns 0 on success, negative errno numbers on failure. > */ > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 5a45262..eee038a 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,6 +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: > * > @@ -666,6 +681,13 @@ int drm_connector_create_standard_properties(struct drm_device *dev) > return -ENOMEM; > dev->mode_config.tile_property = prop; > > + prop = drm_property_create_enum(dev, 0, "link-status", > + drm_link_status_enum_list, > + ARRAY_SIZE(drm_link_status_enum_list)); > + if (!prop) > + return -ENOMEM; > + dev->mode_config.link_status_property = prop; > + > return 0; > } > > @@ -995,6 +1017,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 1218a0c..64e07c5 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; > }; > > @@ -776,6 +793,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 Tue, Dec 06, 2016 at 08:23:05AM +0100, Daniel Vetter wrote: > On Mon, Dec 05, 2016 at 04:27:35PM -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> > > Didn't Sean r-b this already? > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > Looks all nice and tidy, great work. > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Can be merged as soon as we have an ack from Martin that the -modesetting > side is ready to go too. > -Daniel > Thanks Daniel. And yes I missed Sean Paul's r-b. I will add yours and his r-b and resubmit the patch. Manasi > > --- > > drivers/gpu/drm/drm_atomic.c | 10 +++++++ > > drivers/gpu/drm/drm_atomic_helper.c | 15 +++++++++++ > > drivers/gpu/drm/drm_connector.c | 53 +++++++++++++++++++++++++++++++++++++ > > include/drm/drm_connector.h | 19 +++++++++++++ > > include/drm/drm_mode_config.h | 5 ++++ > > include/uapi/drm/drm_mode.h | 4 +++ > > 6 files changed, 106 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 19d7bcb..70a5152 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..3d6c1ce 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,8 @@ static int update_output_state(struct drm_atomic_state *state, > > NULL); > > if (ret) > > return ret; > > + /* Make sure legacy setCrtc always re-trains */ > > + conn_state->link_status = DRM_LINK_STATUS_GOOD; > > } > > } > > > > @@ -2301,6 +2310,12 @@ static int update_output_state(struct drm_atomic_state *state, > > * > > * Provides a default crtc set_config handler using the atomic driver interface. > > * > > + * NOTE: For backwards compatibility with old userspace this automatically > > + * resets the "link-status" property to GOOD, to force any link > > + * re-training. The SETCRTC ioctl does not define whether an update does > > + * need a full modeset or just a plane update, hence we're allowed to do > > + * that. See also drm_mode_connector_set_link_status_property(). > > + * > > * Returns: > > * Returns 0 on success, negative errno numbers on failure. > > */ > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > > index 5a45262..eee038a 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,6 +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: > > * > > @@ -666,6 +681,13 @@ int drm_connector_create_standard_properties(struct drm_device *dev) > > return -ENOMEM; > > dev->mode_config.tile_property = prop; > > > > + prop = drm_property_create_enum(dev, 0, "link-status", > > + drm_link_status_enum_list, > > + ARRAY_SIZE(drm_link_status_enum_list)); > > + if (!prop) > > + return -ENOMEM; > > + dev->mode_config.link_status_property = prop; > > + > > return 0; > > } > > > > @@ -995,6 +1017,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 1218a0c..64e07c5 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; > > }; > > > > @@ -776,6 +793,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
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 19d7bcb..70a5152 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..3d6c1ce 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,8 @@ static int update_output_state(struct drm_atomic_state *state, NULL); if (ret) return ret; + /* Make sure legacy setCrtc always re-trains */ + conn_state->link_status = DRM_LINK_STATUS_GOOD; } } @@ -2301,6 +2310,12 @@ static int update_output_state(struct drm_atomic_state *state, * * Provides a default crtc set_config handler using the atomic driver interface. * + * NOTE: For backwards compatibility with old userspace this automatically + * resets the "link-status" property to GOOD, to force any link + * re-training. The SETCRTC ioctl does not define whether an update does + * need a full modeset or just a plane update, hence we're allowed to do + * that. See also drm_mode_connector_set_link_status_property(). + * * Returns: * Returns 0 on success, negative errno numbers on failure. */ diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 5a45262..eee038a 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,6 +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: * @@ -666,6 +681,13 @@ int drm_connector_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.tile_property = prop; + prop = drm_property_create_enum(dev, 0, "link-status", + drm_link_status_enum_list, + ARRAY_SIZE(drm_link_status_enum_list)); + if (!prop) + return -ENOMEM; + dev->mode_config.link_status_property = prop; + return 0; } @@ -995,6 +1017,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 1218a0c..64e07c5 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; }; @@ -776,6 +793,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;