| Message ID | 1479972501-18334-1-git-send-email-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 23, 2016 at 11:28:21PM -0800, Manasi Navare wrote: > This is RFC patch for adding a connector link-status property > and making it atomic by adding it to the drm_connector_state. > This is to make sure its wired properly in drm_atomic_connector_set_property > and drm_atomic_connector_get_property functions. > > 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) > > 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> You lost all the acked-by from AMD about the link-status property. We need those. > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> Yeah I think this should work, but obviously testing has the final say. Some nitpicks below, then it's r-b: me. But I think we also need to polish the kernel-doc a bit more to address Sean Paul's questions. -Daniel > --- > drivers/gpu/drm/drm_atomic.c | 10 ++++++ > drivers/gpu/drm/drm_atomic_helper.c | 8 +++++ > drivers/gpu/drm/drm_connector.c | 61 ++++++++++++++++++++++++++++++++++++- > include/drm/drm_connector.h | 18 ++++++++++- > include/drm/drm_mode_config.h | 5 +++ > include/uapi/drm/drm_mode.h | 4 +++ > 6 files changed, 104 insertions(+), 2 deletions(-) > > 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; > + } Why hide this here in handle_conflicting_encoders? This is a bit confusing. I'd move it into the connector loop in drm_atomic_helper_check_modeset(). > } > > /* > @@ -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..cd53c39 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,14 @@ 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 during > + * the modeset. The default value of link-status is "GOOD". If something > + * fails during modeset, the kernel driver can set this to "BAD", prune > + * the mode list based on new link parameters and send a hotplug uevent > + * to notify userspace to re-check the valid modes through GET_CONNECTOR > + * IOCTL and redo a modeset. Drivers should update this value using > + * drm_mode_connector_set_link_status_property(). Please leave the empty line here after the definition list, for better layout. Also I thought there were some comments from Sean Paul that the kernel-doc was confusing, have you worked together with him to address this? > * Connectors also have one standardized atomic property: > * > * CRTC_ID: > @@ -666,6 +683,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, DRM_MODE_PROP_ATOMIC, "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 +1019,41 @@ 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 should > + * set this link status property to "BAD" and prune the mode list based on new > + * information. 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 that a lot of existing userspace do not handle this property. > + * Drivers can therefore not rely on userspace to fix up everything and > + * should try to handle issues (like just re-training a link) without > + * userspace's intervention. This should only be used when the current mode > + * fails and userspace must select a different display mode. > + * The DRM driver can chose to not modify property and keep link status > + * as "GOOD" always to keep the user experience same as it currently is. > + * > + * 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..ba22ed1 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,9 @@ struct drm_connector_state { > > struct drm_encoder *best_encoder; > > + /* Connector Link status */ > + enum drm_link_status link_status; make htmldocs should complain about the missing kernel-doc here. Just convert your inline comment into a proper kernel-doc: /** * @link_status: Connector link status */ Also in general a comment that literally says the same as the code isn't useful. Comments should explain why the code does something, not what exactly it does - that should be self-evident. > + > struct drm_atomic_state *state; > }; > > @@ -695,6 +709,7 @@ struct drm_connector { > uint8_t num_h_tile, num_v_tile; > uint8_t tile_h_loc, tile_v_loc; > uint16_t tile_h_size, tile_v_size; > + Spurious whitespace change, pls check for these before submitting. I think this one here is new. > }; > > #define obj_to_connector(x) container_of(x, struct drm_connector, base) > @@ -767,12 +782,13 @@ int drm_mode_create_tv_properties(struct drm_device *dev, > int drm_mode_create_scaling_mode_property(struct drm_device *dev); > int drm_mode_create_aspect_ratio_property(struct drm_device *dev); > int drm_mode_create_suggested_offset_properties(struct drm_device *dev); > - Same. > int drm_mode_connector_set_path_property(struct drm_connector *connector, > const char *path); > 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 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Nov 24, 2016 at 08:51:35AM +0100, Daniel Vetter wrote: > On Wed, Nov 23, 2016 at 11:28:21PM -0800, Manasi Navare wrote: > > This is RFC patch for adding a connector link-status property > > and making it atomic by adding it to the drm_connector_state. > > This is to make sure its wired properly in drm_atomic_connector_set_property > > and drm_atomic_connector_get_property functions. > > > > 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) > > > > 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> > > You lost all the acked-by from AMD about the link-status property. We need > those. > I removed the ACKS sinec this was just a RFC patch, I will add them back when I submit the actual patch in the patch series. > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > Yeah I think this should work, but obviously testing has the final say. > Some nitpicks below, then it's r-b: me. But I think we also need to polish > the kernel-doc a bit more to address Sean Paul's questions. > -Daniel > Yes, I still need to change the kernel doc as per Sean's comments but wanted ACK on this new atomic design for the property so that I could update the kernel doc. > > --- > > drivers/gpu/drm/drm_atomic.c | 10 ++++++ > > drivers/gpu/drm/drm_atomic_helper.c | 8 +++++ > > drivers/gpu/drm/drm_connector.c | 61 ++++++++++++++++++++++++++++++++++++- > > include/drm/drm_connector.h | 18 ++++++++++- > > include/drm/drm_mode_config.h | 5 +++ > > include/uapi/drm/drm_mode.h | 4 +++ > > 6 files changed, 104 insertions(+), 2 deletions(-) > > > > 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; > > + } > > Why hide this here in handle_conflicting_encoders? This is a bit > confusing. I'd move it into the connector loop in > drm_atomic_helper_check_modeset(). > > } > > If you pull this patch, you will see that this is added in the connector loop inside drm_atomic_helper_check_modeset(), donno why the diff shows as if its inside handle_conflicting_encoders. 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; > > } > > } > > > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > > index 5a45262..cd53c39 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,14 @@ 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 during > > + * the modeset. The default value of link-status is "GOOD". If something > > + * fails during modeset, the kernel driver can set this to "BAD", prune > > + * the mode list based on new link parameters and send a hotplug uevent > > + * to notify userspace to re-check the valid modes through GET_CONNECTOR > > + * IOCTL and redo a modeset. Drivers should update this value using > > + * drm_mode_connector_set_link_status_property(). > > Please leave the empty line here after the definition list, for better > layout. Also I thought there were some comments from Sean Paul that the > kernel-doc was confusing, have you worked together with him to address > this? > Now that link-status is an atomic property, shouldnt it be added after CRTC_ID in the standardized atomic prop section? > > * Connectors also have one standardized atomic property: > > * > > * CRTC_ID: > > @@ -666,6 +683,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, DRM_MODE_PROP_ATOMIC, "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 +1019,41 @@ 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 should > > + * set this link status property to "BAD" and prune the mode list based on new > > + * information. 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 that a lot of existing userspace do not handle this property. > > + * Drivers can therefore not rely on userspace to fix up everything and > > + * should try to handle issues (like just re-training a link) without > > + * userspace's intervention. This should only be used when the current mode > > + * fails and userspace must select a different display mode. > > + * The DRM driver can chose to not modify property and keep link status > > + * as "GOOD" always to keep the user experience same as it currently is. > > + * > > + * 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..ba22ed1 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,9 @@ struct drm_connector_state { > > > > struct drm_encoder *best_encoder; > > > > + /* Connector Link status */ > > + enum drm_link_status link_status; > > make htmldocs should complain about the missing kernel-doc here. Just > convert your inline comment into a proper kernel-doc: > > /** > * @link_status: Connector link status > */ > > Also in general a comment that literally says the same as the code isn't > useful. Comments should explain why the code does something, not what > exactly it does - that should be self-evident. > > + > > struct drm_atomic_state *state; > > }; > > > > @@ -695,6 +709,7 @@ struct drm_connector { > > uint8_t num_h_tile, num_v_tile; > > uint8_t tile_h_loc, tile_v_loc; > > uint16_t tile_h_size, tile_v_size; > > + > > Spurious whitespace change, pls check for these before submitting. I think > this one here is new. > > > }; Yes removed that. > > > > #define obj_to_connector(x) container_of(x, struct drm_connector, base) > > @@ -767,12 +782,13 @@ int drm_mode_create_tv_properties(struct drm_device *dev, > > int drm_mode_create_scaling_mode_property(struct drm_device *dev); > > int drm_mode_create_aspect_ratio_property(struct drm_device *dev); > > int drm_mode_create_suggested_offset_properties(struct drm_device *dev); > > - > > Same. > > > int drm_mode_connector_set_path_property(struct drm_connector *connector, > > const char *path); > > 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 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Thu, Nov 24, 2016 at 08:51:35AM +0100, Daniel Vetter wrote: > On Wed, Nov 23, 2016 at 11:28:21PM -0800, Manasi Navare wrote: > > This is RFC patch for adding a connector link-status property > > and making it atomic by adding it to the drm_connector_state. > > This is to make sure its wired properly in drm_atomic_connector_set_property > > and drm_atomic_connector_get_property functions. > > > > 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) > > > > 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> > > You lost all the acked-by from AMD about the link-status property. We need > those. > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > Yeah I think this should work, but obviously testing has the final say. > Some nitpicks below, then it's r-b: me. But I think we also need to polish > the kernel-doc a bit more to address Sean Paul's questions. > -Daniel > I tested it with SNA driver with Chris's changes to read the link-status property, but It is not able to detect the link-status property being set to BAD and hence it does not trigger a new modeset. Do we need to make any changes to the SNA driver now that this is made a ATOMIC property so that GETCONNECTOR IOCTL can still read the correct value of this property throught drm_atomic_get_property() interface? Chris, Daniel, any thoughts? Manasi > > --- > > drivers/gpu/drm/drm_atomic.c | 10 ++++++ > > drivers/gpu/drm/drm_atomic_helper.c | 8 +++++ > > drivers/gpu/drm/drm_connector.c | 61 ++++++++++++++++++++++++++++++++++++- > > include/drm/drm_connector.h | 18 ++++++++++- > > include/drm/drm_mode_config.h | 5 +++ > > include/uapi/drm/drm_mode.h | 4 +++ > > 6 files changed, 104 insertions(+), 2 deletions(-) > > > > 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; > > + } > > Why hide this here in handle_conflicting_encoders? This is a bit > confusing. I'd move it into the connector loop in > drm_atomic_helper_check_modeset(). > > } > > > > /* > > @@ -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..cd53c39 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,14 @@ 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 during > > + * the modeset. The default value of link-status is "GOOD". If something > > + * fails during modeset, the kernel driver can set this to "BAD", prune > > + * the mode list based on new link parameters and send a hotplug uevent > > + * to notify userspace to re-check the valid modes through GET_CONNECTOR > > + * IOCTL and redo a modeset. Drivers should update this value using > > + * drm_mode_connector_set_link_status_property(). > > Please leave the empty line here after the definition list, for better > layout. Also I thought there were some comments from Sean Paul that the > kernel-doc was confusing, have you worked together with him to address > this? > > > * Connectors also have one standardized atomic property: > > * > > * CRTC_ID: > > @@ -666,6 +683,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, DRM_MODE_PROP_ATOMIC, "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 +1019,41 @@ 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 should > > + * set this link status property to "BAD" and prune the mode list based on new > > + * information. 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 that a lot of existing userspace do not handle this property. > > + * Drivers can therefore not rely on userspace to fix up everything and > > + * should try to handle issues (like just re-training a link) without > > + * userspace's intervention. This should only be used when the current mode > > + * fails and userspace must select a different display mode. > > + * The DRM driver can chose to not modify property and keep link status > > + * as "GOOD" always to keep the user experience same as it currently is. > > + * > > + * 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..ba22ed1 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,9 @@ struct drm_connector_state { > > > > struct drm_encoder *best_encoder; > > > > + /* Connector Link status */ > > + enum drm_link_status link_status; > > make htmldocs should complain about the missing kernel-doc here. Just > convert your inline comment into a proper kernel-doc: > > /** > * @link_status: Connector link status > */ > > Also in general a comment that literally says the same as the code isn't > useful. Comments should explain why the code does something, not what > exactly it does - that should be self-evident. > > + > > struct drm_atomic_state *state; > > }; > > > > @@ -695,6 +709,7 @@ struct drm_connector { > > uint8_t num_h_tile, num_v_tile; > > uint8_t tile_h_loc, tile_v_loc; > > uint16_t tile_h_size, tile_v_size; > > + > > Spurious whitespace change, pls check for these before submitting. I think > this one here is new. > > > }; > > > > #define obj_to_connector(x) container_of(x, struct drm_connector, base) > > @@ -767,12 +782,13 @@ int drm_mode_create_tv_properties(struct drm_device *dev, > > int drm_mode_create_scaling_mode_property(struct drm_device *dev); > > int drm_mode_create_aspect_ratio_property(struct drm_device *dev); > > int drm_mode_create_suggested_offset_properties(struct drm_device *dev); > > - > > Same. > > > int drm_mode_connector_set_path_property(struct drm_connector *connector, > > const char *path); > > 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 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Mon, Nov 28, 2016 at 05:07:52PM -0800, Manasi Navare wrote: > On Thu, Nov 24, 2016 at 08:51:35AM +0100, Daniel Vetter wrote: > > On Wed, Nov 23, 2016 at 11:28:21PM -0800, Manasi Navare wrote: > > > This is RFC patch for adding a connector link-status property > > > and making it atomic by adding it to the drm_connector_state. > > > This is to make sure its wired properly in drm_atomic_connector_set_property > > > and drm_atomic_connector_get_property functions. > > > > > > 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) > > > > > > 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> > > > > You lost all the acked-by from AMD about the link-status property. We need > > those. > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > > Yeah I think this should work, but obviously testing has the final say. > > Some nitpicks below, then it's r-b: me. But I think we also need to polish > > the kernel-doc a bit more to address Sean Paul's questions. > > -Daniel > > > > I tested it with SNA driver with Chris's changes to read the link-status > property, but It is not able to detect the link-status property being set to BAD > and hence it does not trigger a new modeset. > Do we need to make any changes to the SNA driver now that this is made a ATOMIC > property so that GETCONNECTOR IOCTL can still read the correct value > of this property throught drm_atomic_get_property() interface? > > Chris, Daniel, any thoughts? We agreed that it must _not_ be a PROPERTY_ATOMIC property, so that old userspace can see it. > > > @@ -666,6 +683,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, DRM_MODE_PROP_ATOMIC, "link-status", I.e. remote DRM_MODE_PROP_ATOMIC here. I thought we've discussed this a lot already ...? Cheers, Daniel
On Tue, Nov 29, 2016 at 09:59:14AM +0100, Daniel Vetter wrote: > On Mon, Nov 28, 2016 at 05:07:52PM -0800, Manasi Navare wrote: > > On Thu, Nov 24, 2016 at 08:51:35AM +0100, Daniel Vetter wrote: > > > On Wed, Nov 23, 2016 at 11:28:21PM -0800, Manasi Navare wrote: > > > > This is RFC patch for adding a connector link-status property > > > > and making it atomic by adding it to the drm_connector_state. > > > > This is to make sure its wired properly in drm_atomic_connector_set_property > > > > and drm_atomic_connector_get_property functions. > > > > > > > > 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) > > > > > > > > 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> > > > > > > You lost all the acked-by from AMD about the link-status property. We need > > > those. > > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > > > > Yeah I think this should work, but obviously testing has the final say. > > > Some nitpicks below, then it's r-b: me. But I think we also need to polish > > > the kernel-doc a bit more to address Sean Paul's questions. > > > -Daniel > > > > > > > I tested it with SNA driver with Chris's changes to read the link-status > > property, but It is not able to detect the link-status property being set to BAD > > and hence it does not trigger a new modeset. > > Do we need to make any changes to the SNA driver now that this is made a ATOMIC > > property so that GETCONNECTOR IOCTL can still read the correct value > > of this property throught drm_atomic_get_property() interface? > > > > Chris, Daniel, any thoughts? > > We agreed that it must _not_ be a PROPERTY_ATOMIC property, so that old > userspace can see it. > > > > > @@ -666,6 +683,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, DRM_MODE_PROP_ATOMIC, "link-status", > > I.e. remote DRM_MODE_PROP_ATOMIC here. I thought we've discussed this a > lot already ...? > > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch Hi Daniel, Yes this is where I was confused in the beginning that whether we need the flag. Ok so this property will not have atomic flag, so drm-mode_object_get_properties should call the drm_object_property_get_value() and that should expose the value to userspace right? Manasi
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..cd53c39 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,14 @@ 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 during + * the modeset. The default value of link-status is "GOOD". If something + * fails during modeset, the kernel driver can set this to "BAD", prune + * the mode list based on new link parameters and send a hotplug uevent + * to notify userspace to re-check the valid modes through GET_CONNECTOR + * IOCTL and redo a modeset. Drivers should update this value using + * drm_mode_connector_set_link_status_property(). * Connectors also have one standardized atomic property: * * CRTC_ID: @@ -666,6 +683,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, DRM_MODE_PROP_ATOMIC, "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 +1019,41 @@ 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 should + * set this link status property to "BAD" and prune the mode list based on new + * information. 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 that a lot of existing userspace do not handle this property. + * Drivers can therefore not rely on userspace to fix up everything and + * should try to handle issues (like just re-training a link) without + * userspace's intervention. This should only be used when the current mode + * fails and userspace must select a different display mode. + * The DRM driver can chose to not modify property and keep link status + * as "GOOD" always to keep the user experience same as it currently is. + * + * 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..ba22ed1 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,9 @@ struct drm_connector_state { struct drm_encoder *best_encoder; + /* Connector Link status */ + enum drm_link_status link_status; + struct drm_atomic_state *state; }; @@ -695,6 +709,7 @@ struct drm_connector { uint8_t num_h_tile, num_v_tile; uint8_t tile_h_loc, tile_v_loc; uint16_t tile_h_size, tile_v_size; + }; #define obj_to_connector(x) container_of(x, struct drm_connector, base) @@ -767,12 +782,13 @@ int drm_mode_create_tv_properties(struct drm_device *dev, int drm_mode_create_scaling_mode_property(struct drm_device *dev); int drm_mode_create_aspect_ratio_property(struct drm_device *dev); int drm_mode_create_suggested_offset_properties(struct drm_device *dev); - int drm_mode_connector_set_path_property(struct drm_connector *connector, const char *path); 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;
This is RFC patch for adding a connector link-status property and making it atomic by adding it to the drm_connector_state. This is to make sure its wired properly in drm_atomic_connector_set_property and drm_atomic_connector_get_property functions. 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) 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 | 61 ++++++++++++++++++++++++++++++++++++- include/drm/drm_connector.h | 18 ++++++++++- include/drm/drm_mode_config.h | 5 +++ include/uapi/drm/drm_mode.h | 4 +++ 6 files changed, 104 insertions(+), 2 deletions(-)