| Message ID | 1479864621-8105-1-git-send-email-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 22, 2016 at 8:30 PM, Manasi Navare <manasi.d.navare@intel.com> 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. > Thanks for sending this. We ran into some re-training awkwardness recently, and I was hoping for such a patch. > 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> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/drm_atomic.c | 5 ++++ > drivers/gpu/drm/drm_connector.c | 65 +++++++++++++++++++++++++++++++++++++++-- > include/drm/drm_connector.h | 12 +++++++- > include/drm/drm_mode_config.h | 5 ++++ > include/uapi/drm/drm_mode.h | 4 +++ > 5 files changed, 88 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 89737e4..644d19f 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -1087,6 +1087,9 @@ 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) { > + 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 +1138,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_connector.c b/drivers/gpu/drm/drm_connector.c > index 5a45262..4576c9f 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 > @@ -616,7 +626,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, > * path property the MST manager created. Userspace cannot change this > * property. > * TILE: > - * Connector tile group property to indicate how a set of DRM connector > + * Connector tile group property to indicate how a set of DRM connector keyboard slip here > * compose together into one logical screen. This is used by both high-res > * external screens (often only using a single cable, but exposing multiple > * DP MST sinks), or high-res integrated panels (like dual-link DSI) which > @@ -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,43 @@ 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 Does the mode list really *need* to be pruned? In the case of needing to re-train a DP link, it seems like we should be able to set this BAD and keep the modelist as-is (since the sink hasn't changed) > + * 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. extra space > + * > + * 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. Hmm, I suppose this answers my last question. It's too bad we can't rely on this, since we'll just end up with each driver rolling their own re-training logic on top of possibly supporting this (but probably not, since it's really just overhead). Perhaps this is an overly pessimistic view? > + * 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; > + > + /* Make sure the mutex is grabbed */ > + lockdep_assert_held(&dev->mode_config.mutex); > + connector->link_status = link_status; > + drm_object_property_set_value(&connector->base, > + dev->mode_config.link_status_property, > + link_status); > +} > +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..3d513ab 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -213,6 +213,9 @@ struct drm_connector_state { > > struct drm_encoder *best_encoder; > > + /* Connector Link status */ > + unsigned int link_status; > + > struct drm_atomic_state *state; > }; > > @@ -695,6 +698,12 @@ 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; > + > + /* Connector Link status > + * 0: If the link is Good > + * 1: If the link is Bad > + */ > + int link_status; > }; > > #define obj_to_connector(x) container_of(x, struct drm_connector, base) > @@ -767,12 +776,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); > - lost a line here > 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 Tue, Nov 22, 2016 at 09:27:35PM -0500, Sean Paul wrote: > On Tue, Nov 22, 2016 at 8:30 PM, Manasi Navare > <manasi.d.navare@intel.com> 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. > > > > Thanks for sending this. We ran into some re-training awkwardness > recently, and I > was hoping for such a patch. > > > 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> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/drm_atomic.c | 5 ++++ > > drivers/gpu/drm/drm_connector.c | 65 +++++++++++++++++++++++++++++++++++++++-- > > include/drm/drm_connector.h | 12 +++++++- > > include/drm/drm_mode_config.h | 5 ++++ > > include/uapi/drm/drm_mode.h | 4 +++ > > 5 files changed, 88 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 89737e4..644d19f 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -1087,6 +1087,9 @@ 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) { > > + state->link_status = val; I think we should have a check here to make sure userspace never tries to set this from "GOOD" to "BAD". "BAD" to "BAD" we need to allow, since with atomic userspace is supposed to be able to just restore everything to what it was. I don't think trying to set it to "BAD" should cause an error though, just silently keep the link status at "GOOG". So: /* Never downgrade from GOOD to BAD on userspace's request here, * only hw issues can do that. */ if (state->link_status == GOOD) return 0; state->link_status = val; return 0; > > + return 0; > > } else if (connector->funcs->atomic_set_property) { > > return connector->funcs->atomic_set_property(connector, > > state, property, val); > > @@ -1135,6 +1138,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_connector.c b/drivers/gpu/drm/drm_connector.c > > index 5a45262..4576c9f 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 > > @@ -616,7 +626,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, > > * path property the MST manager created. Userspace cannot change this > > * property. > > * TILE: > > - * Connector tile group property to indicate how a set of DRM connector > > + * Connector tile group property to indicate how a set of DRM connector > > keyboard slip here > > > * compose together into one logical screen. This is used by both high-res > > * external screens (often only using a single cable, but exposing multiple > > * DP MST sinks), or high-res integrated panels (like dual-link DSI) which > > @@ -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,43 @@ 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 > > Does the mode list really *need* to be pruned? In the case of needing > to re-train a DP link, > it seems like we should be able to set this BAD and keep the modelist > as-is (since the sink > hasn't changed) Well, if you prune it with the same constraints as before, no additional modes will be removed and the same list is there. Pruning here refers to the ->mode_valid step as done in e.g. drm_helper_probe_single_connector_modes. We might want to be a bit more explicit here perhaps? Have some good proposal text to insert? > > > + * 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. > > extra space > > > + * > > + * 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. > > Hmm, I suppose this answers my last question. It's too bad we can't > rely on this, since we'll > just end up with each driver rolling their own re-training logic on > top of possibly supporting > this (but probably not, since it's really just overhead). Perhaps this > is an overly pessimistic > view? You can forgo any re-training of course, that should work too. It's just that DP spec and everyone else seem to recommend to at least do some minimal effort (or maybe even upfront link training) here. But totally not doing anything if the link is bad should be ok too (and probably really simple to implement for drivers, maybe even with a dp_hpd_handler() helper function which checks link status and set link-status to bad&fires off the uevent if needed. Again, do you have some sample prose to add here to clarify this? Thanks for taking a look at this. Cheers, Daniel > > + * 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; > > + > > + /* Make sure the mutex is grabbed */ > > + lockdep_assert_held(&dev->mode_config.mutex); > > + connector->link_status = link_status; > > + drm_object_property_set_value(&connector->base, > > + dev->mode_config.link_status_property, > > + link_status); > > +} > > +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..3d513ab 100644 > > --- a/include/drm/drm_connector.h > > +++ b/include/drm/drm_connector.h > > @@ -213,6 +213,9 @@ struct drm_connector_state { > > > > struct drm_encoder *best_encoder; > > > > + /* Connector Link status */ > > + unsigned int link_status; > > + > > struct drm_atomic_state *state; > > }; > > > > @@ -695,6 +698,12 @@ 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; > > + > > + /* Connector Link status > > + * 0: If the link is Good > > + * 1: If the link is Bad > > + */ > > + int link_status; > > }; > > > > #define obj_to_connector(x) container_of(x, struct drm_connector, base) > > @@ -767,12 +776,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); > > - > > lost a line here > > > 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 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Nov 23, 2016 at 3:09 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Nov 22, 2016 at 09:27:35PM -0500, Sean Paul wrote: >> On Tue, Nov 22, 2016 at 8:30 PM, Manasi Navare >> <manasi.d.navare@intel.com> 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. >> > >> >> Thanks for sending this. We ran into some re-training awkwardness >> recently, and I >> was hoping for such a patch. >> >> > 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> >> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> >> > --- >> > drivers/gpu/drm/drm_atomic.c | 5 ++++ >> > drivers/gpu/drm/drm_connector.c | 65 +++++++++++++++++++++++++++++++++++++++-- >> > include/drm/drm_connector.h | 12 +++++++- >> > include/drm/drm_mode_config.h | 5 ++++ >> > include/uapi/drm/drm_mode.h | 4 +++ >> > 5 files changed, 88 insertions(+), 3 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >> > index 89737e4..644d19f 100644 >> > --- a/drivers/gpu/drm/drm_atomic.c >> > +++ b/drivers/gpu/drm/drm_atomic.c >> > @@ -1087,6 +1087,9 @@ 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) { >> > + state->link_status = val; > > I think we should have a check here to make sure userspace never tries to > set this from "GOOD" to "BAD". "BAD" to "BAD" we need to allow, since with > atomic userspace is supposed to be able to just restore everything to what > it was. > > I don't think trying to set it to "BAD" should cause an error though, just > silently keep the link status at "GOOG". So: > > /* Never downgrade from GOOD to BAD on userspace's request here, > * only hw issues can do that. */ > if (state->link_status == GOOD) > return 0; Can we return an error if the transition is invalid? > state->link_status = val; > return 0; > >> > + return 0; >> > } else if (connector->funcs->atomic_set_property) { >> > return connector->funcs->atomic_set_property(connector, >> > state, property, val); >> > @@ -1135,6 +1138,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_connector.c b/drivers/gpu/drm/drm_connector.c >> > index 5a45262..4576c9f 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 >> > @@ -616,7 +626,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, >> > * path property the MST manager created. Userspace cannot change this >> > * property. >> > * TILE: >> > - * Connector tile group property to indicate how a set of DRM connector >> > + * Connector tile group property to indicate how a set of DRM connector >> >> keyboard slip here >> >> > * compose together into one logical screen. This is used by both high-res >> > * external screens (often only using a single cable, but exposing multiple >> > * DP MST sinks), or high-res integrated panels (like dual-link DSI) which >> > @@ -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,43 @@ 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 >> >> Does the mode list really *need* to be pruned? In the case of needing >> to re-train a DP link, >> it seems like we should be able to set this BAD and keep the modelist >> as-is (since the sink >> hasn't changed) > > Well, if you prune it with the same constraints as before, no additional > modes will be removed and the same list is there. Pruning here refers to > the ->mode_valid step as done in e.g. drm_helper_probe_single_connector_modes. > > We might want to be a bit more explicit here perhaps? Have some good > proposal text to insert? Ah, hmm, I hadn't equated this prune to the normal drm_connector prune_invalid. I suppose the "based on new information" is what did it. It seems like the mode_valid pruning will happen automagically when userspace realizes the connection is BAD and asks for a new mode list. So is it worth even mentioning pruning the mode list? >> >> > + * 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. >> >> extra space >> >> > + * >> > + * 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. >> >> Hmm, I suppose this answers my last question. It's too bad we can't >> rely on this, since we'll >> just end up with each driver rolling their own re-training logic on >> top of possibly supporting >> this (but probably not, since it's really just overhead). Perhaps this >> is an overly pessimistic >> view? > > You can forgo any re-training of course, that should work too. It's just > that DP spec and everyone else seem to recommend to at least do some > minimal effort (or maybe even upfront link training) here. But totally not > doing anything if the link is bad should be ok too (and probably really > simple to implement for drivers, maybe even with a dp_hpd_handler() helper > function which checks link status and set link-status to bad&fires off the > uevent if needed. > > Again, do you have some sample prose to add here to clarify this? > I don't think it's merely a matter of clarifying the comment, tbh. Without relying on userspace to fix us, there's not much point in exposing the property in the first place. As a driver author, I need to decide whether I want re-training to work properly. If I do, I need to handroll it in my driver since I can't rely on userspace, at which point, implementing the property isn't meaningful. If I don't care about it being bulletproof, I can expose the property and let userspace fix me if they know how. So I'd prefer to have the helper be a hybrid between this patch and the previous one that did a full modeset under the covers (perhaps with a client cap or similar to let us know which path to trust). That way I can call the helper from my driver and know that I'm going to be re-trained either way. Sean > Thanks for taking a look at this. > > Cheers, Daniel > >> > + * 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; >> > + >> > + /* Make sure the mutex is grabbed */ >> > + lockdep_assert_held(&dev->mode_config.mutex); >> > + connector->link_status = link_status; >> > + drm_object_property_set_value(&connector->base, >> > + dev->mode_config.link_status_property, >> > + link_status); >> > +} >> > +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..3d513ab 100644 >> > --- a/include/drm/drm_connector.h >> > +++ b/include/drm/drm_connector.h >> > @@ -213,6 +213,9 @@ struct drm_connector_state { >> > >> > struct drm_encoder *best_encoder; >> > >> > + /* Connector Link status */ >> > + unsigned int link_status; >> > + >> > struct drm_atomic_state *state; >> > }; >> > >> > @@ -695,6 +698,12 @@ 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; >> > + >> > + /* Connector Link status >> > + * 0: If the link is Good >> > + * 1: If the link is Bad >> > + */ >> > + int link_status; >> > }; >> > >> > #define obj_to_connector(x) container_of(x, struct drm_connector, base) >> > @@ -767,12 +776,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); >> > - >> >> lost a line here >> >> > 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 >> _______________________________________________ >> 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 Wed, 23 Nov 2016, Sean Paul <seanpaul@chromium.org> wrote: > On Wed, Nov 23, 2016 at 3:09 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Tue, Nov 22, 2016 at 09:27:35PM -0500, Sean Paul wrote: >>> On Tue, Nov 22, 2016 at 8:30 PM, Manasi Navare >>> <manasi.d.navare@intel.com> 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. >>> > >>> >>> Thanks for sending this. We ran into some re-training awkwardness >>> recently, and I >>> was hoping for such a patch. >>> >>> > 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> >>> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> >>> > --- >>> > drivers/gpu/drm/drm_atomic.c | 5 ++++ >>> > drivers/gpu/drm/drm_connector.c | 65 +++++++++++++++++++++++++++++++++++++++-- >>> > include/drm/drm_connector.h | 12 +++++++- >>> > include/drm/drm_mode_config.h | 5 ++++ >>> > include/uapi/drm/drm_mode.h | 4 +++ >>> > 5 files changed, 88 insertions(+), 3 deletions(-) >>> > >>> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >>> > index 89737e4..644d19f 100644 >>> > --- a/drivers/gpu/drm/drm_atomic.c >>> > +++ b/drivers/gpu/drm/drm_atomic.c >>> > @@ -1087,6 +1087,9 @@ 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) { >>> > + state->link_status = val; >> >> I think we should have a check here to make sure userspace never tries to >> set this from "GOOD" to "BAD". "BAD" to "BAD" we need to allow, since with >> atomic userspace is supposed to be able to just restore everything to what >> it was. >> >> I don't think trying to set it to "BAD" should cause an error though, just >> silently keep the link status at "GOOG". So: >> >> /* Never downgrade from GOOD to BAD on userspace's request here, >> * only hw issues can do that. */ >> if (state->link_status == GOOD) >> return 0; > > Can we return an error if the transition is invalid? > >> state->link_status = val; >> return 0; >> >>> > + return 0; >>> > } else if (connector->funcs->atomic_set_property) { >>> > return connector->funcs->atomic_set_property(connector, >>> > state, property, val); >>> > @@ -1135,6 +1138,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_connector.c b/drivers/gpu/drm/drm_connector.c >>> > index 5a45262..4576c9f 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 >>> > @@ -616,7 +626,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, >>> > * path property the MST manager created. Userspace cannot change this >>> > * property. >>> > * TILE: >>> > - * Connector tile group property to indicate how a set of DRM connector >>> > + * Connector tile group property to indicate how a set of DRM connector >>> >>> keyboard slip here >>> >>> > * compose together into one logical screen. This is used by both high-res >>> > * external screens (often only using a single cable, but exposing multiple >>> > * DP MST sinks), or high-res integrated panels (like dual-link DSI) which >>> > @@ -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,43 @@ 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 >>> >>> Does the mode list really *need* to be pruned? In the case of needing >>> to re-train a DP link, >>> it seems like we should be able to set this BAD and keep the modelist >>> as-is (since the sink >>> hasn't changed) >> >> Well, if you prune it with the same constraints as before, no additional >> modes will be removed and the same list is there. Pruning here refers to >> the ->mode_valid step as done in e.g. drm_helper_probe_single_connector_modes. >> >> We might want to be a bit more explicit here perhaps? Have some good >> proposal text to insert? > > Ah, hmm, I hadn't equated this prune to the normal drm_connector > prune_invalid. I suppose the "based on new information" is what did > it. It seems like the mode_valid pruning will happen automagically > when userspace realizes the connection is BAD and asks for a new mode > list. So is it worth even mentioning pruning the mode list? > >>> >>> > + * 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. >>> >>> extra space >>> >>> > + * >>> > + * 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. >>> >>> Hmm, I suppose this answers my last question. It's too bad we can't >>> rely on this, since we'll >>> just end up with each driver rolling their own re-training logic on >>> top of possibly supporting >>> this (but probably not, since it's really just overhead). Perhaps this >>> is an overly pessimistic >>> view? >> >> You can forgo any re-training of course, that should work too. It's just >> that DP spec and everyone else seem to recommend to at least do some >> minimal effort (or maybe even upfront link training) here. But totally not >> doing anything if the link is bad should be ok too (and probably really >> simple to implement for drivers, maybe even with a dp_hpd_handler() helper >> function which checks link status and set link-status to bad&fires off the >> uevent if needed. >> >> Again, do you have some sample prose to add here to clarify this? >> > > I don't think it's merely a matter of clarifying the comment, tbh. > Without relying on userspace to fix us, there's not much point in > exposing the property in the first place. > > As a driver author, I need to decide whether I want re-training to > work properly. If I do, I need to handroll it in my driver since I > can't rely on userspace, at which point, implementing the property > isn't meaningful. If I don't care about it being bulletproof, I can > expose the property and let userspace fix me if they know how. > > So I'd prefer to have the helper be a hybrid between this patch and > the previous one that did a full modeset under the covers (perhaps > with a client cap or similar to let us know which path to trust). That > way I can call the helper from my driver and know that I'm going to be > re-trained either way. If your driver is able to handle everything in kernel, then do so by all means. If it can't, defer to userspace, which may or may not fix stuff, depending on whether it looks at the property. If the userspace doesn't do anything, things will be just as good or bad as they are currently, before this property. This allows the drivers to choose how much they can/want to do in kernel, and what they defer to userspace. BR, Jani. > > Sean > > >> Thanks for taking a look at this. >> >> Cheers, Daniel >> >>> > + * 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; >>> > + >>> > + /* Make sure the mutex is grabbed */ >>> > + lockdep_assert_held(&dev->mode_config.mutex); >>> > + connector->link_status = link_status; >>> > + drm_object_property_set_value(&connector->base, >>> > + dev->mode_config.link_status_property, >>> > + link_status); >>> > +} >>> > +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..3d513ab 100644 >>> > --- a/include/drm/drm_connector.h >>> > +++ b/include/drm/drm_connector.h >>> > @@ -213,6 +213,9 @@ struct drm_connector_state { >>> > >>> > struct drm_encoder *best_encoder; >>> > >>> > + /* Connector Link status */ >>> > + unsigned int link_status; >>> > + >>> > struct drm_atomic_state *state; >>> > }; >>> > >>> > @@ -695,6 +698,12 @@ 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; >>> > + >>> > + /* Connector Link status >>> > + * 0: If the link is Good >>> > + * 1: If the link is Bad >>> > + */ >>> > + int link_status; >>> > }; >>> > >>> > #define obj_to_connector(x) container_of(x, struct drm_connector, base) >>> > @@ -767,12 +776,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); >>> > - >>> >>> lost a line here >>> >>> > 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 >>> _______________________________________________ >>> 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 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Nov 23, 2016 at 10:15 AM, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Wed, 23 Nov 2016, Sean Paul <seanpaul@chromium.org> wrote: >> On Wed, Nov 23, 2016 at 3:09 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >>> On Tue, Nov 22, 2016 at 09:27:35PM -0500, Sean Paul wrote: >>>> On Tue, Nov 22, 2016 at 8:30 PM, Manasi Navare >>>> <manasi.d.navare@intel.com> 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. >>>> > >>>> >>>> Thanks for sending this. We ran into some re-training awkwardness >>>> recently, and I >>>> was hoping for such a patch. >>>> >>>> > 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> >>>> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> >>>> > --- >>>> > drivers/gpu/drm/drm_atomic.c | 5 ++++ >>>> > drivers/gpu/drm/drm_connector.c | 65 +++++++++++++++++++++++++++++++++++++++-- >>>> > include/drm/drm_connector.h | 12 +++++++- >>>> > include/drm/drm_mode_config.h | 5 ++++ >>>> > include/uapi/drm/drm_mode.h | 4 +++ >>>> > 5 files changed, 88 insertions(+), 3 deletions(-) >>>> > >>>> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >>>> > index 89737e4..644d19f 100644 >>>> > --- a/drivers/gpu/drm/drm_atomic.c >>>> > +++ b/drivers/gpu/drm/drm_atomic.c >>>> > @@ -1087,6 +1087,9 @@ 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) { >>>> > + state->link_status = val; >>> >>> I think we should have a check here to make sure userspace never tries to >>> set this from "GOOD" to "BAD". "BAD" to "BAD" we need to allow, since with >>> atomic userspace is supposed to be able to just restore everything to what >>> it was. >>> >>> I don't think trying to set it to "BAD" should cause an error though, just >>> silently keep the link status at "GOOG". So: >>> >>> /* Never downgrade from GOOD to BAD on userspace's request here, >>> * only hw issues can do that. */ >>> if (state->link_status == GOOD) >>> return 0; >> >> Can we return an error if the transition is invalid? >> >>> state->link_status = val; >>> return 0; >>> >>>> > + return 0; >>>> > } else if (connector->funcs->atomic_set_property) { >>>> > return connector->funcs->atomic_set_property(connector, >>>> > state, property, val); >>>> > @@ -1135,6 +1138,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_connector.c b/drivers/gpu/drm/drm_connector.c >>>> > index 5a45262..4576c9f 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 >>>> > @@ -616,7 +626,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, >>>> > * path property the MST manager created. Userspace cannot change this >>>> > * property. >>>> > * TILE: >>>> > - * Connector tile group property to indicate how a set of DRM connector >>>> > + * Connector tile group property to indicate how a set of DRM connector >>>> >>>> keyboard slip here >>>> >>>> > * compose together into one logical screen. This is used by both high-res >>>> > * external screens (often only using a single cable, but exposing multiple >>>> > * DP MST sinks), or high-res integrated panels (like dual-link DSI) which >>>> > @@ -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,43 @@ 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 >>>> >>>> Does the mode list really *need* to be pruned? In the case of needing >>>> to re-train a DP link, >>>> it seems like we should be able to set this BAD and keep the modelist >>>> as-is (since the sink >>>> hasn't changed) >>> >>> Well, if you prune it with the same constraints as before, no additional >>> modes will be removed and the same list is there. Pruning here refers to >>> the ->mode_valid step as done in e.g. drm_helper_probe_single_connector_modes. >>> >>> We might want to be a bit more explicit here perhaps? Have some good >>> proposal text to insert? >> >> Ah, hmm, I hadn't equated this prune to the normal drm_connector >> prune_invalid. I suppose the "based on new information" is what did >> it. It seems like the mode_valid pruning will happen automagically >> when userspace realizes the connection is BAD and asks for a new mode >> list. So is it worth even mentioning pruning the mode list? >> >>>> >>>> > + * 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. >>>> >>>> extra space >>>> >>>> > + * >>>> > + * 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. >>>> >>>> Hmm, I suppose this answers my last question. It's too bad we can't >>>> rely on this, since we'll >>>> just end up with each driver rolling their own re-training logic on >>>> top of possibly supporting >>>> this (but probably not, since it's really just overhead). Perhaps this >>>> is an overly pessimistic >>>> view? >>> >>> You can forgo any re-training of course, that should work too. It's just >>> that DP spec and everyone else seem to recommend to at least do some >>> minimal effort (or maybe even upfront link training) here. But totally not >>> doing anything if the link is bad should be ok too (and probably really >>> simple to implement for drivers, maybe even with a dp_hpd_handler() helper >>> function which checks link status and set link-status to bad&fires off the >>> uevent if needed. >>> >>> Again, do you have some sample prose to add here to clarify this? >>> >> >> I don't think it's merely a matter of clarifying the comment, tbh. >> Without relying on userspace to fix us, there's not much point in >> exposing the property in the first place. >> >> As a driver author, I need to decide whether I want re-training to >> work properly. If I do, I need to handroll it in my driver since I >> can't rely on userspace, at which point, implementing the property >> isn't meaningful. If I don't care about it being bulletproof, I can >> expose the property and let userspace fix me if they know how. >> >> So I'd prefer to have the helper be a hybrid between this patch and >> the previous one that did a full modeset under the covers (perhaps >> with a client cap or similar to let us know which path to trust). That >> way I can call the helper from my driver and know that I'm going to be >> re-trained either way. > > If your driver is able to handle everything in kernel, then do so by all > means. If it can't, defer to userspace, which may or may not fix stuff, > depending on whether it looks at the property. If the userspace doesn't > do anything, things will be just as good or bad as they are currently, > before this property. > > This allows the drivers to choose how much they can/want to do in > kernel, and what they defer to userspace. > Just because I can, doesn't mean I want to :) Take the CDN DP driver in rockchip for example (posted yesterday). There's a worker in the driver that is responsible for handling hpd/extcon events from the USB-C port. Ideally, this worker should just be a thin shell around a kms uevent that lets userspace know there's been a change. Unfortunately since we need to make re-training work (/me grumbles about productization) seamlessly, we need to add a bunch of goo into the worker to handle re-training. Since we need to handle re-training there and in modeset, we need to properly synchronize things. So we end up with a bunch of code that doesn't *really* need to be there. So is the correct path forward to add GOOD/BAD connection handling to Chrome/drm_hwc and rip re-training out of the various kernel drivers? Sean > BR, > Jani. > > > >> >> Sean >> >> >>> Thanks for taking a look at this. >>> >>> Cheers, Daniel >>> >>>> > + * 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; >>>> > + >>>> > + /* Make sure the mutex is grabbed */ >>>> > + lockdep_assert_held(&dev->mode_config.mutex); >>>> > + connector->link_status = link_status; >>>> > + drm_object_property_set_value(&connector->base, >>>> > + dev->mode_config.link_status_property, >>>> > + link_status); >>>> > +} >>>> > +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..3d513ab 100644 >>>> > --- a/include/drm/drm_connector.h >>>> > +++ b/include/drm/drm_connector.h >>>> > @@ -213,6 +213,9 @@ struct drm_connector_state { >>>> > >>>> > struct drm_encoder *best_encoder; >>>> > >>>> > + /* Connector Link status */ >>>> > + unsigned int link_status; >>>> > + >>>> > struct drm_atomic_state *state; >>>> > }; >>>> > >>>> > @@ -695,6 +698,12 @@ 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; >>>> > + >>>> > + /* Connector Link status >>>> > + * 0: If the link is Good >>>> > + * 1: If the link is Bad >>>> > + */ >>>> > + int link_status; >>>> > }; >>>> > >>>> > #define obj_to_connector(x) container_of(x, struct drm_connector, base) >>>> > @@ -767,12 +776,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); >>>> > - >>>> >>>> lost a line here >>>> >>>> > 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 >>>> _______________________________________________ >>>> 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 >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Jani Nikula, Intel Open Source Technology Center
On Wed, Nov 23, 2016 at 10:32:44AM -0500, Sean Paul wrote: > On Wed, Nov 23, 2016 at 10:15 AM, Jani Nikula > <jani.nikula@linux.intel.com> wrote: > > On Wed, 23 Nov 2016, Sean Paul <seanpaul@chromium.org> wrote: > >> On Wed, Nov 23, 2016 at 3:09 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > >>> On Tue, Nov 22, 2016 at 09:27:35PM -0500, Sean Paul wrote: > >>>> On Tue, Nov 22, 2016 at 8:30 PM, Manasi Navare > >>>> <manasi.d.navare@intel.com> 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. > >>>> > > >>>> > >>>> Thanks for sending this. We ran into some re-training awkwardness > >>>> recently, and I > >>>> was hoping for such a patch. > >>>> > >>>> > 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> > >>>> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > >>>> > --- > >>>> > drivers/gpu/drm/drm_atomic.c | 5 ++++ > >>>> > drivers/gpu/drm/drm_connector.c | 65 +++++++++++++++++++++++++++++++++++++++-- > >>>> > include/drm/drm_connector.h | 12 +++++++- > >>>> > include/drm/drm_mode_config.h | 5 ++++ > >>>> > include/uapi/drm/drm_mode.h | 4 +++ > >>>> > 5 files changed, 88 insertions(+), 3 deletions(-) > >>>> > > >>>> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > >>>> > index 89737e4..644d19f 100644 > >>>> > --- a/drivers/gpu/drm/drm_atomic.c > >>>> > +++ b/drivers/gpu/drm/drm_atomic.c > >>>> > @@ -1087,6 +1087,9 @@ 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) { > >>>> > + state->link_status = val; > >>> > >>> I think we should have a check here to make sure userspace never tries to > >>> set this from "GOOD" to "BAD". "BAD" to "BAD" we need to allow, since with > >>> atomic userspace is supposed to be able to just restore everything to what > >>> it was. > >>> > >>> I don't think trying to set it to "BAD" should cause an error though, just > >>> silently keep the link status at "GOOG". So: > >>> > >>> /* Never downgrade from GOOD to BAD on userspace's request here, > >>> * only hw issues can do that. */ > >>> if (state->link_status == GOOD) > >>> return 0; > >> > >> Can we return an error if the transition is invalid? > >> > >>> state->link_status = val; > >>> return 0; > >>> > >>>> > + return 0; > >>>> > } else if (connector->funcs->atomic_set_property) { > >>>> > return connector->funcs->atomic_set_property(connector, > >>>> > state, property, val); > >>>> > @@ -1135,6 +1138,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_connector.c b/drivers/gpu/drm/drm_connector.c > >>>> > index 5a45262..4576c9f 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 > >>>> > @@ -616,7 +626,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, > >>>> > * path property the MST manager created. Userspace cannot change this > >>>> > * property. > >>>> > * TILE: > >>>> > - * Connector tile group property to indicate how a set of DRM connector > >>>> > + * Connector tile group property to indicate how a set of DRM connector > >>>> > >>>> keyboard slip here > >>>> > >>>> > * compose together into one logical screen. This is used by both high-res > >>>> > * external screens (often only using a single cable, but exposing multiple > >>>> > * DP MST sinks), or high-res integrated panels (like dual-link DSI) which > >>>> > @@ -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,43 @@ 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 > >>>> > >>>> Does the mode list really *need* to be pruned? In the case of needing > >>>> to re-train a DP link, > >>>> it seems like we should be able to set this BAD and keep the modelist > >>>> as-is (since the sink > >>>> hasn't changed) > >>> > >>> Well, if you prune it with the same constraints as before, no additional > >>> modes will be removed and the same list is there. Pruning here refers to > >>> the ->mode_valid step as done in e.g. drm_helper_probe_single_connector_modes. > >>> > >>> We might want to be a bit more explicit here perhaps? Have some good > >>> proposal text to insert? > >> > >> Ah, hmm, I hadn't equated this prune to the normal drm_connector > >> prune_invalid. I suppose the "based on new information" is what did > >> it. It seems like the mode_valid pruning will happen automagically > >> when userspace realizes the connection is BAD and asks for a new mode > >> list. So is it worth even mentioning pruning the mode list? > >> > >>>> > >>>> > + * 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. > >>>> > >>>> extra space > >>>> > >>>> > + * > >>>> > + * 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. > >>>> > >>>> Hmm, I suppose this answers my last question. It's too bad we can't > >>>> rely on this, since we'll > >>>> just end up with each driver rolling their own re-training logic on > >>>> top of possibly supporting > >>>> this (but probably not, since it's really just overhead). Perhaps this > >>>> is an overly pessimistic > >>>> view? > >>> > >>> You can forgo any re-training of course, that should work too. It's just > >>> that DP spec and everyone else seem to recommend to at least do some > >>> minimal effort (or maybe even upfront link training) here. But totally not > >>> doing anything if the link is bad should be ok too (and probably really > >>> simple to implement for drivers, maybe even with a dp_hpd_handler() helper > >>> function which checks link status and set link-status to bad&fires off the > >>> uevent if needed. > >>> > >>> Again, do you have some sample prose to add here to clarify this? > >>> > >> > >> I don't think it's merely a matter of clarifying the comment, tbh. > >> Without relying on userspace to fix us, there's not much point in > >> exposing the property in the first place. > >> > >> As a driver author, I need to decide whether I want re-training to > >> work properly. If I do, I need to handroll it in my driver since I > >> can't rely on userspace, at which point, implementing the property > >> isn't meaningful. If I don't care about it being bulletproof, I can > >> expose the property and let userspace fix me if they know how. > >> > >> So I'd prefer to have the helper be a hybrid between this patch and > >> the previous one that did a full modeset under the covers (perhaps > >> with a client cap or similar to let us know which path to trust). That > >> way I can call the helper from my driver and know that I'm going to be > >> re-trained either way. > > > > If your driver is able to handle everything in kernel, then do so by all > > means. If it can't, defer to userspace, which may or may not fix stuff, > > depending on whether it looks at the property. If the userspace doesn't > > do anything, things will be just as good or bad as they are currently, > > before this property. > > > > This allows the drivers to choose how much they can/want to do in > > kernel, and what they defer to userspace. > > > > Just because I can, doesn't mean I want to :) > > Take the CDN DP driver in rockchip for example (posted yesterday). > There's a worker in the driver that is responsible for handling > hpd/extcon events from the USB-C port. Ideally, this worker should > just be a thin shell around a kms uevent that lets userspace know > there's been a change. Unfortunately since we need to make re-training > work (/me grumbles about productization) seamlessly, we need to add a > bunch of goo into the worker to handle re-training. Since we need to > handle re-training there and in modeset, we need to properly > synchronize things. So we end up with a bunch of code that doesn't > *really* need to be there. > > So is the correct path forward to add GOOD/BAD connection handling to > Chrome/drm_hwc and rip re-training out of the various kernel drivers? In i915 I think we might still want to do the "try to retrain with the current link parameters" thing automagically. It's not difficult to do after all, and should be faster than doing a full modeset. > > Sean > > > > BR, > > Jani. > > > > > > > >> > >> Sean > >> > >> > >>> Thanks for taking a look at this. > >>> > >>> Cheers, Daniel > >>> > >>>> > + * 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; > >>>> > + > >>>> > + /* Make sure the mutex is grabbed */ > >>>> > + lockdep_assert_held(&dev->mode_config.mutex); > >>>> > + connector->link_status = link_status; > >>>> > + drm_object_property_set_value(&connector->base, > >>>> > + dev->mode_config.link_status_property, > >>>> > + link_status); > >>>> > +} > >>>> > +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..3d513ab 100644 > >>>> > --- a/include/drm/drm_connector.h > >>>> > +++ b/include/drm/drm_connector.h > >>>> > @@ -213,6 +213,9 @@ struct drm_connector_state { > >>>> > > >>>> > struct drm_encoder *best_encoder; > >>>> > > >>>> > + /* Connector Link status */ > >>>> > + unsigned int link_status; > >>>> > + > >>>> > struct drm_atomic_state *state; > >>>> > }; > >>>> > > >>>> > @@ -695,6 +698,12 @@ 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; > >>>> > + > >>>> > + /* Connector Link status > >>>> > + * 0: If the link is Good > >>>> > + * 1: If the link is Bad > >>>> > + */ > >>>> > + int link_status; > >>>> > }; > >>>> > > >>>> > #define obj_to_connector(x) container_of(x, struct drm_connector, base) > >>>> > @@ -767,12 +776,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); > >>>> > - > >>>> > >>>> lost a line here > >>>> > >>>> > 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 > >>>> _______________________________________________ > >>>> 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 > >> _______________________________________________ > >> dri-devel mailing list > >> dri-devel@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 23 Nov 2016, Sean Paul <seanpaul@chromium.org> wrote: > On Wed, Nov 23, 2016 at 10:15 AM, Jani Nikula > <jani.nikula@linux.intel.com> wrote: >> On Wed, 23 Nov 2016, Sean Paul <seanpaul@chromium.org> wrote: >>> On Wed, Nov 23, 2016 at 3:09 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >>>> On Tue, Nov 22, 2016 at 09:27:35PM -0500, Sean Paul wrote: >>>>> On Tue, Nov 22, 2016 at 8:30 PM, Manasi Navare >>>>> <manasi.d.navare@intel.com> 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. >>>>> > >>>>> >>>>> Thanks for sending this. We ran into some re-training awkwardness >>>>> recently, and I >>>>> was hoping for such a patch. >>>>> >>>>> > 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> >>>>> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> >>>>> > --- >>>>> > drivers/gpu/drm/drm_atomic.c | 5 ++++ >>>>> > drivers/gpu/drm/drm_connector.c | 65 +++++++++++++++++++++++++++++++++++++++-- >>>>> > include/drm/drm_connector.h | 12 +++++++- >>>>> > include/drm/drm_mode_config.h | 5 ++++ >>>>> > include/uapi/drm/drm_mode.h | 4 +++ >>>>> > 5 files changed, 88 insertions(+), 3 deletions(-) >>>>> > >>>>> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >>>>> > index 89737e4..644d19f 100644 >>>>> > --- a/drivers/gpu/drm/drm_atomic.c >>>>> > +++ b/drivers/gpu/drm/drm_atomic.c >>>>> > @@ -1087,6 +1087,9 @@ 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) { >>>>> > + state->link_status = val; >>>> >>>> I think we should have a check here to make sure userspace never tries to >>>> set this from "GOOD" to "BAD". "BAD" to "BAD" we need to allow, since with >>>> atomic userspace is supposed to be able to just restore everything to what >>>> it was. >>>> >>>> I don't think trying to set it to "BAD" should cause an error though, just >>>> silently keep the link status at "GOOG". So: >>>> >>>> /* Never downgrade from GOOD to BAD on userspace's request here, >>>> * only hw issues can do that. */ >>>> if (state->link_status == GOOD) >>>> return 0; >>> >>> Can we return an error if the transition is invalid? >>> >>>> state->link_status = val; >>>> return 0; >>>> >>>>> > + return 0; >>>>> > } else if (connector->funcs->atomic_set_property) { >>>>> > return connector->funcs->atomic_set_property(connector, >>>>> > state, property, val); >>>>> > @@ -1135,6 +1138,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_connector.c b/drivers/gpu/drm/drm_connector.c >>>>> > index 5a45262..4576c9f 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 >>>>> > @@ -616,7 +626,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, >>>>> > * path property the MST manager created. Userspace cannot change this >>>>> > * property. >>>>> > * TILE: >>>>> > - * Connector tile group property to indicate how a set of DRM connector >>>>> > + * Connector tile group property to indicate how a set of DRM connector >>>>> >>>>> keyboard slip here >>>>> >>>>> > * compose together into one logical screen. This is used by both high-res >>>>> > * external screens (often only using a single cable, but exposing multiple >>>>> > * DP MST sinks), or high-res integrated panels (like dual-link DSI) which >>>>> > @@ -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,43 @@ 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 >>>>> >>>>> Does the mode list really *need* to be pruned? In the case of needing >>>>> to re-train a DP link, >>>>> it seems like we should be able to set this BAD and keep the modelist >>>>> as-is (since the sink >>>>> hasn't changed) >>>> >>>> Well, if you prune it with the same constraints as before, no additional >>>> modes will be removed and the same list is there. Pruning here refers to >>>> the ->mode_valid step as done in e.g. drm_helper_probe_single_connector_modes. >>>> >>>> We might want to be a bit more explicit here perhaps? Have some good >>>> proposal text to insert? >>> >>> Ah, hmm, I hadn't equated this prune to the normal drm_connector >>> prune_invalid. I suppose the "based on new information" is what did >>> it. It seems like the mode_valid pruning will happen automagically >>> when userspace realizes the connection is BAD and asks for a new mode >>> list. So is it worth even mentioning pruning the mode list? >>> >>>>> >>>>> > + * 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. >>>>> >>>>> extra space >>>>> >>>>> > + * >>>>> > + * 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. >>>>> >>>>> Hmm, I suppose this answers my last question. It's too bad we can't >>>>> rely on this, since we'll >>>>> just end up with each driver rolling their own re-training logic on >>>>> top of possibly supporting >>>>> this (but probably not, since it's really just overhead). Perhaps this >>>>> is an overly pessimistic >>>>> view? >>>> >>>> You can forgo any re-training of course, that should work too. It's just >>>> that DP spec and everyone else seem to recommend to at least do some >>>> minimal effort (or maybe even upfront link training) here. But totally not >>>> doing anything if the link is bad should be ok too (and probably really >>>> simple to implement for drivers, maybe even with a dp_hpd_handler() helper >>>> function which checks link status and set link-status to bad&fires off the >>>> uevent if needed. >>>> >>>> Again, do you have some sample prose to add here to clarify this? >>>> >>> >>> I don't think it's merely a matter of clarifying the comment, tbh. >>> Without relying on userspace to fix us, there's not much point in >>> exposing the property in the first place. >>> >>> As a driver author, I need to decide whether I want re-training to >>> work properly. If I do, I need to handroll it in my driver since I >>> can't rely on userspace, at which point, implementing the property >>> isn't meaningful. If I don't care about it being bulletproof, I can >>> expose the property and let userspace fix me if they know how. >>> >>> So I'd prefer to have the helper be a hybrid between this patch and >>> the previous one that did a full modeset under the covers (perhaps >>> with a client cap or similar to let us know which path to trust). That >>> way I can call the helper from my driver and know that I'm going to be >>> re-trained either way. >> >> If your driver is able to handle everything in kernel, then do so by all >> means. If it can't, defer to userspace, which may or may not fix stuff, >> depending on whether it looks at the property. If the userspace doesn't >> do anything, things will be just as good or bad as they are currently, >> before this property. >> >> This allows the drivers to choose how much they can/want to do in >> kernel, and what they defer to userspace. >> > > Just because I can, doesn't mean I want to :) And just because I want to, doesn't mean I can! ;) > Take the CDN DP driver in rockchip for example (posted yesterday). > There's a worker in the driver that is responsible for handling > hpd/extcon events from the USB-C port. Ideally, this worker should > just be a thin shell around a kms uevent that lets userspace know > there's been a change. Unfortunately since we need to make re-training > work (/me grumbles about productization) seamlessly, we need to add a > bunch of goo into the worker to handle re-training. Since we need to > handle re-training there and in modeset, we need to properly > synchronize things. So we end up with a bunch of code that doesn't > *really* need to be there. > > So is the correct path forward to add GOOD/BAD connection handling to > Chrome/drm_hwc I think so. > and rip re-training out of the various kernel drivers? IMO if it works, don't change it, at least not in a rush. It also depends on the hardware and the driver; the amount of code required may be vastly different for various drivers. Personally, I think in the long run letting userspace help here leads to a simpler design and more efficient happy day scenario handling, and userspace knows what's going on. BR, Jani. > > Sean > > >> BR, >> Jani. >> >> >> >>> >>> Sean >>> >>> >>>> Thanks for taking a look at this. >>>> >>>> Cheers, Daniel >>>> >>>>> > + * 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; >>>>> > + >>>>> > + /* Make sure the mutex is grabbed */ >>>>> > + lockdep_assert_held(&dev->mode_config.mutex); >>>>> > + connector->link_status = link_status; >>>>> > + drm_object_property_set_value(&connector->base, >>>>> > + dev->mode_config.link_status_property, >>>>> > + link_status); >>>>> > +} >>>>> > +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..3d513ab 100644 >>>>> > --- a/include/drm/drm_connector.h >>>>> > +++ b/include/drm/drm_connector.h >>>>> > @@ -213,6 +213,9 @@ struct drm_connector_state { >>>>> > >>>>> > struct drm_encoder *best_encoder; >>>>> > >>>>> > + /* Connector Link status */ >>>>> > + unsigned int link_status; >>>>> > + >>>>> > struct drm_atomic_state *state; >>>>> > }; >>>>> > >>>>> > @@ -695,6 +698,12 @@ 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; >>>>> > + >>>>> > + /* Connector Link status >>>>> > + * 0: If the link is Good >>>>> > + * 1: If the link is Bad >>>>> > + */ >>>>> > + int link_status; >>>>> > }; >>>>> > >>>>> > #define obj_to_connector(x) container_of(x, struct drm_connector, base) >>>>> > @@ -767,12 +776,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); >>>>> > - >>>>> >>>>> lost a line here >>>>> >>>>> > 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 >>>>> _______________________________________________ >>>>> 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 >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> >> -- >> Jani Nikula, Intel Open Source Technology Center
On Wed, Nov 23, 2016 at 10:43 AM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Wed, Nov 23, 2016 at 10:32:44AM -0500, Sean Paul wrote: >> On Wed, Nov 23, 2016 at 10:15 AM, Jani Nikula >> <jani.nikula@linux.intel.com> wrote: >> > On Wed, 23 Nov 2016, Sean Paul <seanpaul@chromium.org> wrote: >> >> On Wed, Nov 23, 2016 at 3:09 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >> >>> On Tue, Nov 22, 2016 at 09:27:35PM -0500, Sean Paul wrote: >> >>>> On Tue, Nov 22, 2016 at 8:30 PM, Manasi Navare >> >>>> <manasi.d.navare@intel.com> 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. >> >>>> > >> >>>> >> >>>> Thanks for sending this. We ran into some re-training awkwardness >> >>>> recently, and I >> >>>> was hoping for such a patch. >> >>>> >> >>>> > 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> >> >>>> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> >> >>>> > --- >> >>>> > drivers/gpu/drm/drm_atomic.c | 5 ++++ >> >>>> > drivers/gpu/drm/drm_connector.c | 65 +++++++++++++++++++++++++++++++++++++++-- >> >>>> > include/drm/drm_connector.h | 12 +++++++- >> >>>> > include/drm/drm_mode_config.h | 5 ++++ >> >>>> > include/uapi/drm/drm_mode.h | 4 +++ >> >>>> > 5 files changed, 88 insertions(+), 3 deletions(-) >> >>>> > >> >>>> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >> >>>> > index 89737e4..644d19f 100644 >> >>>> > --- a/drivers/gpu/drm/drm_atomic.c >> >>>> > +++ b/drivers/gpu/drm/drm_atomic.c >> >>>> > @@ -1087,6 +1087,9 @@ 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) { >> >>>> > + state->link_status = val; >> >>> >> >>> I think we should have a check here to make sure userspace never tries to >> >>> set this from "GOOD" to "BAD". "BAD" to "BAD" we need to allow, since with >> >>> atomic userspace is supposed to be able to just restore everything to what >> >>> it was. >> >>> >> >>> I don't think trying to set it to "BAD" should cause an error though, just >> >>> silently keep the link status at "GOOG". So: >> >>> >> >>> /* Never downgrade from GOOD to BAD on userspace's request here, >> >>> * only hw issues can do that. */ >> >>> if (state->link_status == GOOD) >> >>> return 0; >> >> >> >> Can we return an error if the transition is invalid? >> >> >> >>> state->link_status = val; >> >>> return 0; >> >>> >> >>>> > + return 0; >> >>>> > } else if (connector->funcs->atomic_set_property) { >> >>>> > return connector->funcs->atomic_set_property(connector, >> >>>> > state, property, val); >> >>>> > @@ -1135,6 +1138,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_connector.c b/drivers/gpu/drm/drm_connector.c >> >>>> > index 5a45262..4576c9f 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 >> >>>> > @@ -616,7 +626,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, >> >>>> > * path property the MST manager created. Userspace cannot change this >> >>>> > * property. >> >>>> > * TILE: >> >>>> > - * Connector tile group property to indicate how a set of DRM connector >> >>>> > + * Connector tile group property to indicate how a set of DRM connector >> >>>> >> >>>> keyboard slip here >> >>>> >> >>>> > * compose together into one logical screen. This is used by both high-res >> >>>> > * external screens (often only using a single cable, but exposing multiple >> >>>> > * DP MST sinks), or high-res integrated panels (like dual-link DSI) which >> >>>> > @@ -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,43 @@ 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 >> >>>> >> >>>> Does the mode list really *need* to be pruned? In the case of needing >> >>>> to re-train a DP link, >> >>>> it seems like we should be able to set this BAD and keep the modelist >> >>>> as-is (since the sink >> >>>> hasn't changed) >> >>> >> >>> Well, if you prune it with the same constraints as before, no additional >> >>> modes will be removed and the same list is there. Pruning here refers to >> >>> the ->mode_valid step as done in e.g. drm_helper_probe_single_connector_modes. >> >>> >> >>> We might want to be a bit more explicit here perhaps? Have some good >> >>> proposal text to insert? >> >> >> >> Ah, hmm, I hadn't equated this prune to the normal drm_connector >> >> prune_invalid. I suppose the "based on new information" is what did >> >> it. It seems like the mode_valid pruning will happen automagically >> >> when userspace realizes the connection is BAD and asks for a new mode >> >> list. So is it worth even mentioning pruning the mode list? >> >> >> >>>> >> >>>> > + * 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. >> >>>> >> >>>> extra space >> >>>> >> >>>> > + * >> >>>> > + * 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. >> >>>> >> >>>> Hmm, I suppose this answers my last question. It's too bad we can't >> >>>> rely on this, since we'll >> >>>> just end up with each driver rolling their own re-training logic on >> >>>> top of possibly supporting >> >>>> this (but probably not, since it's really just overhead). Perhaps this >> >>>> is an overly pessimistic >> >>>> view? >> >>> >> >>> You can forgo any re-training of course, that should work too. It's just >> >>> that DP spec and everyone else seem to recommend to at least do some >> >>> minimal effort (or maybe even upfront link training) here. But totally not >> >>> doing anything if the link is bad should be ok too (and probably really >> >>> simple to implement for drivers, maybe even with a dp_hpd_handler() helper >> >>> function which checks link status and set link-status to bad&fires off the >> >>> uevent if needed. >> >>> >> >>> Again, do you have some sample prose to add here to clarify this? >> >>> >> >> >> >> I don't think it's merely a matter of clarifying the comment, tbh. >> >> Without relying on userspace to fix us, there's not much point in >> >> exposing the property in the first place. >> >> >> >> As a driver author, I need to decide whether I want re-training to >> >> work properly. If I do, I need to handroll it in my driver since I >> >> can't rely on userspace, at which point, implementing the property >> >> isn't meaningful. If I don't care about it being bulletproof, I can >> >> expose the property and let userspace fix me if they know how. >> >> >> >> So I'd prefer to have the helper be a hybrid between this patch and >> >> the previous one that did a full modeset under the covers (perhaps >> >> with a client cap or similar to let us know which path to trust). That >> >> way I can call the helper from my driver and know that I'm going to be >> >> re-trained either way. >> > >> > If your driver is able to handle everything in kernel, then do so by all >> > means. If it can't, defer to userspace, which may or may not fix stuff, >> > depending on whether it looks at the property. If the userspace doesn't >> > do anything, things will be just as good or bad as they are currently, >> > before this property. >> > >> > This allows the drivers to choose how much they can/want to do in >> > kernel, and what they defer to userspace. >> > >> >> Just because I can, doesn't mean I want to :) >> >> Take the CDN DP driver in rockchip for example (posted yesterday). >> There's a worker in the driver that is responsible for handling >> hpd/extcon events from the USB-C port. Ideally, this worker should >> just be a thin shell around a kms uevent that lets userspace know >> there's been a change. Unfortunately since we need to make re-training >> work (/me grumbles about productization) seamlessly, we need to add a >> bunch of goo into the worker to handle re-training. Since we need to >> handle re-training there and in modeset, we need to properly >> synchronize things. So we end up with a bunch of code that doesn't >> *really* need to be there. >> >> So is the correct path forward to add GOOD/BAD connection handling to >> Chrome/drm_hwc and rip re-training out of the various kernel drivers? > > In i915 I think we might still want to do the "try to retrain with > the current link parameters" thing automagically. It's not difficult > to do after all, and should be faster than doing a full modeset. > I'm not sure I buy the "It's not difficult to do" argument. We've squashed a ton of asynchronous training bugs over the past few years in various drivers. It's just one more thing that needs to be synchronized with hotplug/modeset/suspend/disable, and Synchronization Is Hard for a lot of people. So while it may be faster, I'd happily take the performance hit to unify paths. Sean >> >> Sean >> >> >> > BR, >> > Jani. >> > >> > >> > >> >> >> >> Sean >> >> >> >> >> >>> Thanks for taking a look at this. >> >>> >> >>> Cheers, Daniel >> >>> >> >>>> > + * 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; >> >>>> > + >> >>>> > + /* Make sure the mutex is grabbed */ >> >>>> > + lockdep_assert_held(&dev->mode_config.mutex); >> >>>> > + connector->link_status = link_status; >> >>>> > + drm_object_property_set_value(&connector->base, >> >>>> > + dev->mode_config.link_status_property, >> >>>> > + link_status); >> >>>> > +} >> >>>> > +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..3d513ab 100644 >> >>>> > --- a/include/drm/drm_connector.h >> >>>> > +++ b/include/drm/drm_connector.h >> >>>> > @@ -213,6 +213,9 @@ struct drm_connector_state { >> >>>> > >> >>>> > struct drm_encoder *best_encoder; >> >>>> > >> >>>> > + /* Connector Link status */ >> >>>> > + unsigned int link_status; >> >>>> > + >> >>>> > struct drm_atomic_state *state; >> >>>> > }; >> >>>> > >> >>>> > @@ -695,6 +698,12 @@ 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; >> >>>> > + >> >>>> > + /* Connector Link status >> >>>> > + * 0: If the link is Good >> >>>> > + * 1: If the link is Bad >> >>>> > + */ >> >>>> > + int link_status; >> >>>> > }; >> >>>> > >> >>>> > #define obj_to_connector(x) container_of(x, struct drm_connector, base) >> >>>> > @@ -767,12 +776,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); >> >>>> > - >> >>>> >> >>>> lost a line here >> >>>> >> >>>> > 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 >> >>>> _______________________________________________ >> >>>> 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 >> >> _______________________________________________ >> >> dri-devel mailing list >> >> dri-devel@lists.freedesktop.org >> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> > >> > -- >> > Jani Nikula, Intel Open Source Technology Center >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC
On Wed, Nov 23, 2016 at 10:52 AM, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Wed, 23 Nov 2016, Sean Paul <seanpaul@chromium.org> wrote: >> On Wed, Nov 23, 2016 at 10:15 AM, Jani Nikula >> <jani.nikula@linux.intel.com> wrote: >>> On Wed, 23 Nov 2016, Sean Paul <seanpaul@chromium.org> wrote: >>>> On Wed, Nov 23, 2016 at 3:09 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >>>>> On Tue, Nov 22, 2016 at 09:27:35PM -0500, Sean Paul wrote: >>>>>> On Tue, Nov 22, 2016 at 8:30 PM, Manasi Navare >>>>>> <manasi.d.navare@intel.com> 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. >>>>>> > >>>>>> >>>>>> Thanks for sending this. We ran into some re-training awkwardness >>>>>> recently, and I >>>>>> was hoping for such a patch. >>>>>> >>>>>> > 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> >>>>>> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> >>>>>> > --- >>>>>> > drivers/gpu/drm/drm_atomic.c | 5 ++++ >>>>>> > drivers/gpu/drm/drm_connector.c | 65 +++++++++++++++++++++++++++++++++++++++-- >>>>>> > include/drm/drm_connector.h | 12 +++++++- >>>>>> > include/drm/drm_mode_config.h | 5 ++++ >>>>>> > include/uapi/drm/drm_mode.h | 4 +++ >>>>>> > 5 files changed, 88 insertions(+), 3 deletions(-) >>>>>> > >>>>>> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >>>>>> > index 89737e4..644d19f 100644 >>>>>> > --- a/drivers/gpu/drm/drm_atomic.c >>>>>> > +++ b/drivers/gpu/drm/drm_atomic.c >>>>>> > @@ -1087,6 +1087,9 @@ 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) { >>>>>> > + state->link_status = val; >>>>> >>>>> I think we should have a check here to make sure userspace never tries to >>>>> set this from "GOOD" to "BAD". "BAD" to "BAD" we need to allow, since with >>>>> atomic userspace is supposed to be able to just restore everything to what >>>>> it was. >>>>> >>>>> I don't think trying to set it to "BAD" should cause an error though, just >>>>> silently keep the link status at "GOOG". So: >>>>> >>>>> /* Never downgrade from GOOD to BAD on userspace's request here, >>>>> * only hw issues can do that. */ >>>>> if (state->link_status == GOOD) >>>>> return 0; >>>> >>>> Can we return an error if the transition is invalid? >>>> >>>>> state->link_status = val; >>>>> return 0; >>>>> >>>>>> > + return 0; >>>>>> > } else if (connector->funcs->atomic_set_property) { >>>>>> > return connector->funcs->atomic_set_property(connector, >>>>>> > state, property, val); >>>>>> > @@ -1135,6 +1138,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_connector.c b/drivers/gpu/drm/drm_connector.c >>>>>> > index 5a45262..4576c9f 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 >>>>>> > @@ -616,7 +626,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, >>>>>> > * path property the MST manager created. Userspace cannot change this >>>>>> > * property. >>>>>> > * TILE: >>>>>> > - * Connector tile group property to indicate how a set of DRM connector >>>>>> > + * Connector tile group property to indicate how a set of DRM connector >>>>>> >>>>>> keyboard slip here >>>>>> >>>>>> > * compose together into one logical screen. This is used by both high-res >>>>>> > * external screens (often only using a single cable, but exposing multiple >>>>>> > * DP MST sinks), or high-res integrated panels (like dual-link DSI) which >>>>>> > @@ -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,43 @@ 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 >>>>>> >>>>>> Does the mode list really *need* to be pruned? In the case of needing >>>>>> to re-train a DP link, >>>>>> it seems like we should be able to set this BAD and keep the modelist >>>>>> as-is (since the sink >>>>>> hasn't changed) >>>>> >>>>> Well, if you prune it with the same constraints as before, no additional >>>>> modes will be removed and the same list is there. Pruning here refers to >>>>> the ->mode_valid step as done in e.g. drm_helper_probe_single_connector_modes. >>>>> >>>>> We might want to be a bit more explicit here perhaps? Have some good >>>>> proposal text to insert? >>>> >>>> Ah, hmm, I hadn't equated this prune to the normal drm_connector >>>> prune_invalid. I suppose the "based on new information" is what did >>>> it. It seems like the mode_valid pruning will happen automagically >>>> when userspace realizes the connection is BAD and asks for a new mode >>>> list. So is it worth even mentioning pruning the mode list? >>>> >>>>>> >>>>>> > + * 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. >>>>>> >>>>>> extra space >>>>>> >>>>>> > + * >>>>>> > + * 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. >>>>>> >>>>>> Hmm, I suppose this answers my last question. It's too bad we can't >>>>>> rely on this, since we'll >>>>>> just end up with each driver rolling their own re-training logic on >>>>>> top of possibly supporting >>>>>> this (but probably not, since it's really just overhead). Perhaps this >>>>>> is an overly pessimistic >>>>>> view? >>>>> >>>>> You can forgo any re-training of course, that should work too. It's just >>>>> that DP spec and everyone else seem to recommend to at least do some >>>>> minimal effort (or maybe even upfront link training) here. But totally not >>>>> doing anything if the link is bad should be ok too (and probably really >>>>> simple to implement for drivers, maybe even with a dp_hpd_handler() helper >>>>> function which checks link status and set link-status to bad&fires off the >>>>> uevent if needed. >>>>> >>>>> Again, do you have some sample prose to add here to clarify this? >>>>> >>>> >>>> I don't think it's merely a matter of clarifying the comment, tbh. >>>> Without relying on userspace to fix us, there's not much point in >>>> exposing the property in the first place. >>>> >>>> As a driver author, I need to decide whether I want re-training to >>>> work properly. If I do, I need to handroll it in my driver since I >>>> can't rely on userspace, at which point, implementing the property >>>> isn't meaningful. If I don't care about it being bulletproof, I can >>>> expose the property and let userspace fix me if they know how. >>>> >>>> So I'd prefer to have the helper be a hybrid between this patch and >>>> the previous one that did a full modeset under the covers (perhaps >>>> with a client cap or similar to let us know which path to trust). That >>>> way I can call the helper from my driver and know that I'm going to be >>>> re-trained either way. >>> >>> If your driver is able to handle everything in kernel, then do so by all >>> means. If it can't, defer to userspace, which may or may not fix stuff, >>> depending on whether it looks at the property. If the userspace doesn't >>> do anything, things will be just as good or bad as they are currently, >>> before this property. >>> >>> This allows the drivers to choose how much they can/want to do in >>> kernel, and what they defer to userspace. >>> >> >> Just because I can, doesn't mean I want to :) > > And just because I want to, doesn't mean I can! ;) > Truth. Many people have tried and failed :-D >> Take the CDN DP driver in rockchip for example (posted yesterday). >> There's a worker in the driver that is responsible for handling >> hpd/extcon events from the USB-C port. Ideally, this worker should >> just be a thin shell around a kms uevent that lets userspace know >> there's been a change. Unfortunately since we need to make re-training >> work (/me grumbles about productization) seamlessly, we need to add a >> bunch of goo into the worker to handle re-training. Since we need to >> handle re-training there and in modeset, we need to properly >> synchronize things. So we end up with a bunch of code that doesn't >> *really* need to be there. >> >> So is the correct path forward to add GOOD/BAD connection handling to >> Chrome/drm_hwc > > I think so. > >> and rip re-training out of the various kernel drivers? > > IMO if it works, don't change it, at least not in a rush. It also > depends on the hardware and the driver; the amount of code required may > be vastly different for various drivers. > > Personally, I think in the long run letting userspace help here leads to > a simpler design and more efficient happy day scenario handling, and > userspace knows what's going on. > Agreed on this. I wish there were an easier way for drivers to do the right thing, but I suppose we can struggle on until userspace supports this :/ Sean > BR, > Jani. > >> >> Sean >> >> >>> BR, >>> Jani. >>> >>> >>> >>>> >>>> Sean >>>> >>>> >>>>> Thanks for taking a look at this. >>>>> >>>>> Cheers, Daniel >>>>> >>>>>> > + * 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; >>>>>> > + >>>>>> > + /* Make sure the mutex is grabbed */ >>>>>> > + lockdep_assert_held(&dev->mode_config.mutex); >>>>>> > + connector->link_status = link_status; >>>>>> > + drm_object_property_set_value(&connector->base, >>>>>> > + dev->mode_config.link_status_property, >>>>>> > + link_status); >>>>>> > +} >>>>>> > +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..3d513ab 100644 >>>>>> > --- a/include/drm/drm_connector.h >>>>>> > +++ b/include/drm/drm_connector.h >>>>>> > @@ -213,6 +213,9 @@ struct drm_connector_state { >>>>>> > >>>>>> > struct drm_encoder *best_encoder; >>>>>> > >>>>>> > + /* Connector Link status */ >>>>>> > + unsigned int link_status; >>>>>> > + >>>>>> > struct drm_atomic_state *state; >>>>>> > }; >>>>>> > >>>>>> > @@ -695,6 +698,12 @@ 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; >>>>>> > + >>>>>> > + /* Connector Link status >>>>>> > + * 0: If the link is Good >>>>>> > + * 1: If the link is Bad >>>>>> > + */ >>>>>> > + int link_status; >>>>>> > }; >>>>>> > >>>>>> > #define obj_to_connector(x) container_of(x, struct drm_connector, base) >>>>>> > @@ -767,12 +776,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); >>>>>> > - >>>>>> >>>>>> lost a line here >>>>>> >>>>>> > 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 >>>>>> _______________________________________________ >>>>>> 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 >>>> _______________________________________________ >>>> dri-devel mailing list >>>> dri-devel@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>> >>> -- >>> Jani Nikula, Intel Open Source Technology Center > > -- > Jani Nikula, Intel Open Source Technology Center
On Wed, Nov 23, 2016 at 4:52 PM, Jani Nikula <jani.nikula@linux.intel.com> wrote: >> Take the CDN DP driver in rockchip for example (posted yesterday). >> There's a worker in the driver that is responsible for handling >> hpd/extcon events from the USB-C port. Ideally, this worker should >> just be a thin shell around a kms uevent that lets userspace know >> there's been a change. Unfortunately since we need to make re-training >> work (/me grumbles about productization) seamlessly, we need to add a >> bunch of goo into the worker to handle re-training. Since we need to >> handle re-training there and in modeset, we need to properly >> synchronize things. So we end up with a bunch of code that doesn't >> *really* need to be there. >> >> So is the correct path forward to add GOOD/BAD connection handling to >> Chrome/drm_hwc > > I think so. > >> and rip re-training out of the various kernel drivers? > > IMO if it works, don't change it, at least not in a rush. It also > depends on the hardware and the driver; the amount of code required may > be vastly different for various drivers. > > Personally, I think in the long run letting userspace help here leads to > a simpler design and more efficient happy day scenario handling, and > userspace knows what's going on. We can't rip out re-training from existing drivers since it would break existing userspace in certain cases (like cable re-plug into same screen). But we could try to not implement so much for new drivers. -Daniel
On Wed, Nov 23, 2016 at 09:09:28AM +0100, Daniel Vetter wrote: > On Tue, Nov 22, 2016 at 09:27:35PM -0500, Sean Paul wrote: > > On Tue, Nov 22, 2016 at 8:30 PM, Manasi Navare > > <manasi.d.navare@intel.com> 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. > > > > > > > Thanks for sending this. We ran into some re-training awkwardness > > recently, and I > > was hoping for such a patch. > > > > > 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> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > --- > > > drivers/gpu/drm/drm_atomic.c | 5 ++++ > > > drivers/gpu/drm/drm_connector.c | 65 +++++++++++++++++++++++++++++++++++++++-- > > > include/drm/drm_connector.h | 12 +++++++- > > > include/drm/drm_mode_config.h | 5 ++++ > > > include/uapi/drm/drm_mode.h | 4 +++ > > > 5 files changed, 88 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > > index 89737e4..644d19f 100644 > > > --- a/drivers/gpu/drm/drm_atomic.c > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > @@ -1087,6 +1087,9 @@ 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) { > > > + state->link_status = val; > > I think we should have a check here to make sure userspace never tries to > set this from "GOOD" to "BAD". "BAD" to "BAD" we need to allow, since with > atomic userspace is supposed to be able to just restore everything to what > it was. > > I don't think trying to set it to "BAD" should cause an error though, just > silently keep the link status at "GOOG". So: > > /* Never downgrade from GOOD to BAD on userspace's request here, > * only hw issues can do that. */ > if (state->link_status == GOOD) > return 0; > state->link_status = val; > return 0; > > > > + return 0; > > > } else if (connector->funcs->atomic_set_property) { > > > return connector->funcs->atomic_set_property(connector, > > > state, property, val); > > > @@ -1135,6 +1138,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_connector.c b/drivers/gpu/drm/drm_connector.c > > > index 5a45262..4576c9f 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 > > > @@ -616,7 +626,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, > > > * path property the MST manager created. Userspace cannot change this > > > * property. > > > * TILE: > > > - * Connector tile group property to indicate how a set of DRM connector > > > + * Connector tile group property to indicate how a set of DRM connector > > > > keyboard slip here > > > > > * compose together into one logical screen. This is used by both high-res > > > * external screens (often only using a single cable, but exposing multiple > > > * DP MST sinks), or high-res integrated panels (like dual-link DSI) which > > > @@ -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,43 @@ 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 > > > > Does the mode list really *need* to be pruned? In the case of needing > > to re-train a DP link, > > it seems like we should be able to set this BAD and keep the modelist > > as-is (since the sink > > hasn't changed) > > Well, if you prune it with the same constraints as before, no additional > modes will be removed and the same list is there. Pruning here refers to > the ->mode_valid step as done in e.g. drm_helper_probe_single_connector_modes. > > We might want to be a bit more explicit here perhaps? Have some good > proposal text to insert? > > Sean, do you have any recommendations on the text that can go in the kernel doc for this property to clarify that it depends on the kernel implementation whether we want to use this link-status property to do the retraining or not. Leaving it BAD and not doing anything will be fine too. Manasi > > > + * 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. > > > > extra space > > > > > + * > > > + * 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. > > > > Hmm, I suppose this answers my last question. It's too bad we can't > > rely on this, since we'll > > just end up with each driver rolling their own re-training logic on > > top of possibly supporting > > this (but probably not, since it's really just overhead). Perhaps this > > is an overly pessimistic > > view? > > You can forgo any re-training of course, that should work too. It's just > that DP spec and everyone else seem to recommend to at least do some > minimal effort (or maybe even upfront link training) here. But totally not > doing anything if the link is bad should be ok too (and probably really > simple to implement for drivers, maybe even with a dp_hpd_handler() helper > function which checks link status and set link-status to bad&fires off the > uevent if needed. > > Again, do you have some sample prose to add here to clarify this? > > Thanks for taking a look at this. > > Cheers, Daniel > > > > + * 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; > > > + > > > + /* Make sure the mutex is grabbed */ > > > + lockdep_assert_held(&dev->mode_config.mutex); > > > + connector->link_status = link_status; > > > + drm_object_property_set_value(&connector->base, > > > + dev->mode_config.link_status_property, > > > + link_status); > > > +} > > > +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..3d513ab 100644 > > > --- a/include/drm/drm_connector.h > > > +++ b/include/drm/drm_connector.h > > > @@ -213,6 +213,9 @@ struct drm_connector_state { > > > > > > struct drm_encoder *best_encoder; > > > > > > + /* Connector Link status */ > > > + unsigned int link_status; > > > + > > > struct drm_atomic_state *state; > > > }; > > > > > > @@ -695,6 +698,12 @@ 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; > > > + > > > + /* Connector Link status > > > + * 0: If the link is Good > > > + * 1: If the link is Bad > > > + */ > > > + int link_status; > > > }; > > > > > > #define obj_to_connector(x) container_of(x, struct drm_connector, base) > > > @@ -767,12 +776,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); > > > - > > > > lost a line here > > > > > 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 > > _______________________________________________ > > 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 7:59 PM, Manasi Navare <manasi.d.navare@intel.com> wrote: > On Wed, Nov 23, 2016 at 09:09:28AM +0100, Daniel Vetter wrote: >> On Tue, Nov 22, 2016 at 09:27:35PM -0500, Sean Paul wrote: >> > On Tue, Nov 22, 2016 at 8:30 PM, Manasi Navare >> > <manasi.d.navare@intel.com> 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. >> > > >> > >> > Thanks for sending this. We ran into some re-training awkwardness >> > recently, and I >> > was hoping for such a patch. >> > >> > > 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> >> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> >> > > --- >> > > drivers/gpu/drm/drm_atomic.c | 5 ++++ >> > > drivers/gpu/drm/drm_connector.c | 65 +++++++++++++++++++++++++++++++++++++++-- >> > > include/drm/drm_connector.h | 12 +++++++- >> > > include/drm/drm_mode_config.h | 5 ++++ >> > > include/uapi/drm/drm_mode.h | 4 +++ >> > > 5 files changed, 88 insertions(+), 3 deletions(-) >> > > >> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >> > > index 89737e4..644d19f 100644 >> > > --- a/drivers/gpu/drm/drm_atomic.c >> > > +++ b/drivers/gpu/drm/drm_atomic.c >> > > @@ -1087,6 +1087,9 @@ 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) { >> > > + state->link_status = val; >> >> I think we should have a check here to make sure userspace never tries to >> set this from "GOOD" to "BAD". "BAD" to "BAD" we need to allow, since with >> atomic userspace is supposed to be able to just restore everything to what >> it was. >> >> I don't think trying to set it to "BAD" should cause an error though, just >> silently keep the link status at "GOOG". So: >> >> /* Never downgrade from GOOD to BAD on userspace's request here, >> * only hw issues can do that. */ >> if (state->link_status == GOOD) >> return 0; >> state->link_status = val; >> return 0; >> >> > > + return 0; >> > > } else if (connector->funcs->atomic_set_property) { >> > > return connector->funcs->atomic_set_property(connector, >> > > state, property, val); >> > > @@ -1135,6 +1138,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_connector.c b/drivers/gpu/drm/drm_connector.c >> > > index 5a45262..4576c9f 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 >> > > @@ -616,7 +626,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, >> > > * path property the MST manager created. Userspace cannot change this >> > > * property. >> > > * TILE: >> > > - * Connector tile group property to indicate how a set of DRM connector >> > > + * Connector tile group property to indicate how a set of DRM connector >> > >> > keyboard slip here >> > >> > > * compose together into one logical screen. This is used by both high-res >> > > * external screens (often only using a single cable, but exposing multiple >> > > * DP MST sinks), or high-res integrated panels (like dual-link DSI) which >> > > @@ -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,43 @@ 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 >> > >> > Does the mode list really *need* to be pruned? In the case of needing >> > to re-train a DP link, >> > it seems like we should be able to set this BAD and keep the modelist >> > as-is (since the sink >> > hasn't changed) >> >> Well, if you prune it with the same constraints as before, no additional >> modes will be removed and the same list is there. Pruning here refers to >> the ->mode_valid step as done in e.g. drm_helper_probe_single_connector_modes. >> >> We might want to be a bit more explicit here perhaps? Have some good >> proposal text to insert? >> > > > Sean, do you have any recommendations on the text that can go in the kernel doc for > this property to clarify that it depends on the kernel implementation whether we want > to use this link-status property to do the retraining or not. Leaving it BAD and not doing > anything will be fine too. > I'd prefer something like: * In usual working scenario, this link status property will always be set to * "GOOD". If something fails during or after a mode set, the driver may * set this 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 can not 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 difference being that we don't need to mention modes since that will be handled through the normal avenue and we don't necessarily want to remove any modes from the list. Sean > Manasi >> > > + * 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. >> > >> > extra space >> > >> > > + * >> > > + * 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. >> > >> > Hmm, I suppose this answers my last question. It's too bad we can't >> > rely on this, since we'll >> > just end up with each driver rolling their own re-training logic on >> > top of possibly supporting >> > this (but probably not, since it's really just overhead). Perhaps this >> > is an overly pessimistic >> > view? >> >> You can forgo any re-training of course, that should work too. It's just >> that DP spec and everyone else seem to recommend to at least do some >> minimal effort (or maybe even upfront link training) here. But totally not >> doing anything if the link is bad should be ok too (and probably really >> simple to implement for drivers, maybe even with a dp_hpd_handler() helper >> function which checks link status and set link-status to bad&fires off the >> uevent if needed. >> >> Again, do you have some sample prose to add here to clarify this? >> >> Thanks for taking a look at this. >> >> Cheers, Daniel >> >> > > + * 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; >> > > + >> > > + /* Make sure the mutex is grabbed */ >> > > + lockdep_assert_held(&dev->mode_config.mutex); >> > > + connector->link_status = link_status; >> > > + drm_object_property_set_value(&connector->base, >> > > + dev->mode_config.link_status_property, >> > > + link_status); >> > > +} >> > > +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..3d513ab 100644 >> > > --- a/include/drm/drm_connector.h >> > > +++ b/include/drm/drm_connector.h >> > > @@ -213,6 +213,9 @@ struct drm_connector_state { >> > > >> > > struct drm_encoder *best_encoder; >> > > >> > > + /* Connector Link status */ >> > > + unsigned int link_status; >> > > + >> > > struct drm_atomic_state *state; >> > > }; >> > > >> > > @@ -695,6 +698,12 @@ 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; >> > > + >> > > + /* Connector Link status >> > > + * 0: If the link is Good >> > > + * 1: If the link is Bad >> > > + */ >> > > + int link_status; >> > > }; >> > > >> > > #define obj_to_connector(x) container_of(x, struct drm_connector, base) >> > > @@ -767,12 +776,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); >> > > - >> > >> > lost a line here >> > >> > > 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 >> > _______________________________________________ >> > 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 Tue, Nov 29, 2016 at 9:45 AM, Sean Paul <seanpaul@chromium.org> wrote: > On Mon, Nov 28, 2016 at 7:59 PM, Manasi Navare > <manasi.d.navare@intel.com> wrote: >> On Wed, Nov 23, 2016 at 09:09:28AM +0100, Daniel Vetter wrote: >>> On Tue, Nov 22, 2016 at 09:27:35PM -0500, Sean Paul wrote: >>> > On Tue, Nov 22, 2016 at 8:30 PM, Manasi Navare >>> > <manasi.d.navare@intel.com> 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. >>> > > >>> > >>> > Thanks for sending this. We ran into some re-training awkwardness >>> > recently, and I >>> > was hoping for such a patch. >>> > >>> > > 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> >>> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> >>> > > --- >>> > > drivers/gpu/drm/drm_atomic.c | 5 ++++ >>> > > drivers/gpu/drm/drm_connector.c | 65 +++++++++++++++++++++++++++++++++++++++-- >>> > > include/drm/drm_connector.h | 12 +++++++- >>> > > include/drm/drm_mode_config.h | 5 ++++ >>> > > include/uapi/drm/drm_mode.h | 4 +++ >>> > > 5 files changed, 88 insertions(+), 3 deletions(-) >>> > > >>> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >>> > > index 89737e4..644d19f 100644 >>> > > --- a/drivers/gpu/drm/drm_atomic.c >>> > > +++ b/drivers/gpu/drm/drm_atomic.c >>> > > @@ -1087,6 +1087,9 @@ 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) { >>> > > + state->link_status = val; >>> >>> I think we should have a check here to make sure userspace never tries to >>> set this from "GOOD" to "BAD". "BAD" to "BAD" we need to allow, since with >>> atomic userspace is supposed to be able to just restore everything to what >>> it was. >>> >>> I don't think trying to set it to "BAD" should cause an error though, just >>> silently keep the link status at "GOOG". So: >>> >>> /* Never downgrade from GOOD to BAD on userspace's request here, >>> * only hw issues can do that. */ >>> if (state->link_status == GOOD) >>> return 0; >>> state->link_status = val; >>> return 0; >>> >>> > > + return 0; >>> > > } else if (connector->funcs->atomic_set_property) { >>> > > return connector->funcs->atomic_set_property(connector, >>> > > state, property, val); >>> > > @@ -1135,6 +1138,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_connector.c b/drivers/gpu/drm/drm_connector.c >>> > > index 5a45262..4576c9f 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 >>> > > @@ -616,7 +626,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, >>> > > * path property the MST manager created. Userspace cannot change this >>> > > * property. >>> > > * TILE: >>> > > - * Connector tile group property to indicate how a set of DRM connector >>> > > + * Connector tile group property to indicate how a set of DRM connector >>> > >>> > keyboard slip here >>> > >>> > > * compose together into one logical screen. This is used by both high-res >>> > > * external screens (often only using a single cable, but exposing multiple >>> > > * DP MST sinks), or high-res integrated panels (like dual-link DSI) which >>> > > @@ -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(). While I'm nitpicking documentation, I think this one needs work, too. I'd prefer you just describe what the property conveys here instead of getting into driver implementation specifics. So something like: * link-status * Connector link-status property to indicate the status of link. The * default value is "GOOD". If something fails during, or after modeset, * the driver may set this to "BAD" and issue a hotplug uevent. Drivers * should update this value using * drm_mode_connector_set_link_status_property() Sean >>> > > * 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,43 @@ 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 >>> > >>> > Does the mode list really *need* to be pruned? In the case of needing >>> > to re-train a DP link, >>> > it seems like we should be able to set this BAD and keep the modelist >>> > as-is (since the sink >>> > hasn't changed) >>> >>> Well, if you prune it with the same constraints as before, no additional >>> modes will be removed and the same list is there. Pruning here refers to >>> the ->mode_valid step as done in e.g. drm_helper_probe_single_connector_modes. >>> >>> We might want to be a bit more explicit here perhaps? Have some good >>> proposal text to insert? >>> > >> >> Sean, do you have any recommendations on the text that can go in the kernel doc for >> this property to clarify that it depends on the kernel implementation whether we want >> to use this link-status property to do the retraining or not. Leaving it BAD and not doing >> anything will be fine too. >> > > I'd prefer something like: > > * In usual working scenario, this link status property will always be set to > * "GOOD". If something fails during or after a mode set, the driver may > * set this 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 can not 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 difference being that we don't need to mention modes since that > will be handled through the normal avenue and we don't necessarily > want to remove any modes from the list. > > Sean > > >> Manasi >>> > > + * 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. >>> > >>> > extra space >>> > >>> > > + * >>> > > + * 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. >>> > >>> > Hmm, I suppose this answers my last question. It's too bad we can't >>> > rely on this, since we'll >>> > just end up with each driver rolling their own re-training logic on >>> > top of possibly supporting >>> > this (but probably not, since it's really just overhead). Perhaps this >>> > is an overly pessimistic >>> > view? >>> >>> You can forgo any re-training of course, that should work too. It's just >>> that DP spec and everyone else seem to recommend to at least do some >>> minimal effort (or maybe even upfront link training) here. But totally not >>> doing anything if the link is bad should be ok too (and probably really >>> simple to implement for drivers, maybe even with a dp_hpd_handler() helper >>> function which checks link status and set link-status to bad&fires off the >>> uevent if needed. >>> >>> Again, do you have some sample prose to add here to clarify this? >>> >>> Thanks for taking a look at this. >>> >>> Cheers, Daniel >>> >>> > > + * 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; >>> > > + >>> > > + /* Make sure the mutex is grabbed */ >>> > > + lockdep_assert_held(&dev->mode_config.mutex); >>> > > + connector->link_status = link_status; >>> > > + drm_object_property_set_value(&connector->base, >>> > > + dev->mode_config.link_status_property, >>> > > + link_status); >>> > > +} >>> > > +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..3d513ab 100644 >>> > > --- a/include/drm/drm_connector.h >>> > > +++ b/include/drm/drm_connector.h >>> > > @@ -213,6 +213,9 @@ struct drm_connector_state { >>> > > >>> > > struct drm_encoder *best_encoder; >>> > > >>> > > + /* Connector Link status */ >>> > > + unsigned int link_status; >>> > > + >>> > > struct drm_atomic_state *state; >>> > > }; >>> > > >>> > > @@ -695,6 +698,12 @@ 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; >>> > > + >>> > > + /* Connector Link status >>> > > + * 0: If the link is Good >>> > > + * 1: If the link is Bad >>> > > + */ >>> > > + int link_status; >>> > > }; >>> > > >>> > > #define obj_to_connector(x) container_of(x, struct drm_connector, base) >>> > > @@ -767,12 +776,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); >>> > > - >>> > >>> > lost a line here >>> > >>> > > 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 >>> > _______________________________________________ >>> > 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 Tue, Nov 29, 2016 at 02:43:55PM -0500, Sean Paul wrote: > On Tue, Nov 29, 2016 at 9:45 AM, Sean Paul <seanpaul@chromium.org> wrote: > > On Mon, Nov 28, 2016 at 7:59 PM, Manasi Navare > > <manasi.d.navare@intel.com> wrote: > >> On Wed, Nov 23, 2016 at 09:09:28AM +0100, Daniel Vetter wrote: > >>> On Tue, Nov 22, 2016 at 09:27:35PM -0500, Sean Paul wrote: > >>> > On Tue, Nov 22, 2016 at 8:30 PM, Manasi Navare > >>> > <manasi.d.navare@intel.com> 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. > >>> > > > >>> > > >>> > Thanks for sending this. We ran into some re-training awkwardness > >>> > recently, and I > >>> > was hoping for such a patch. > >>> > > >>> > > 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> > >>> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > >>> > > --- > >>> > > drivers/gpu/drm/drm_atomic.c | 5 ++++ > >>> > > drivers/gpu/drm/drm_connector.c | 65 +++++++++++++++++++++++++++++++++++++++-- > >>> > > include/drm/drm_connector.h | 12 +++++++- > >>> > > include/drm/drm_mode_config.h | 5 ++++ > >>> > > include/uapi/drm/drm_mode.h | 4 +++ > >>> > > 5 files changed, 88 insertions(+), 3 deletions(-) > >>> > > > >>> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > >>> > > index 89737e4..644d19f 100644 > >>> > > --- a/drivers/gpu/drm/drm_atomic.c > >>> > > +++ b/drivers/gpu/drm/drm_atomic.c > >>> > > @@ -1087,6 +1087,9 @@ 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) { > >>> > > + state->link_status = val; > >>> > >>> I think we should have a check here to make sure userspace never tries to > >>> set this from "GOOD" to "BAD". "BAD" to "BAD" we need to allow, since with > >>> atomic userspace is supposed to be able to just restore everything to what > >>> it was. > >>> > >>> I don't think trying to set it to "BAD" should cause an error though, just > >>> silently keep the link status at "GOOG". So: > >>> > >>> /* Never downgrade from GOOD to BAD on userspace's request here, > >>> * only hw issues can do that. */ > >>> if (state->link_status == GOOD) > >>> return 0; > >>> state->link_status = val; > >>> return 0; > >>> > >>> > > + return 0; > >>> > > } else if (connector->funcs->atomic_set_property) { > >>> > > return connector->funcs->atomic_set_property(connector, > >>> > > state, property, val); > >>> > > @@ -1135,6 +1138,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_connector.c b/drivers/gpu/drm/drm_connector.c > >>> > > index 5a45262..4576c9f 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 > >>> > > @@ -616,7 +626,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, > >>> > > * path property the MST manager created. Userspace cannot change this > >>> > > * property. > >>> > > * TILE: > >>> > > - * Connector tile group property to indicate how a set of DRM connector > >>> > > + * Connector tile group property to indicate how a set of DRM connector > >>> > > >>> > keyboard slip here > >>> > > >>> > > * compose together into one logical screen. This is used by both high-res > >>> > > * external screens (often only using a single cable, but exposing multiple > >>> > > * DP MST sinks), or high-res integrated panels (like dual-link DSI) which > >>> > > @@ -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(). > > While I'm nitpicking documentation, I think this one needs work, too. > I'd prefer you just describe what the property conveys here instead of > getting into driver implementation specifics. So something like: > > * link-status > * Connector link-status property to indicate the status of link. The > * default value is "GOOD". If something fails during, or after modeset, > * the driver may set this to "BAD" and issue a hotplug uevent. Drivers > * should update this value using > * drm_mode_connector_set_link_status_property() > > Sean > Thanks a lot for your feedback. I will incorporate these changes here and the ones you pointed out in the previous email and send a new revision. Regards Manasi > >>> > > * 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,43 @@ 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 > >>> > > >>> > Does the mode list really *need* to be pruned? In the case of needing > >>> > to re-train a DP link, > >>> > it seems like we should be able to set this BAD and keep the modelist > >>> > as-is (since the sink > >>> > hasn't changed) > >>> > >>> Well, if you prune it with the same constraints as before, no additional > >>> modes will be removed and the same list is there. Pruning here refers to > >>> the ->mode_valid step as done in e.g. drm_helper_probe_single_connector_modes. > >>> > >>> We might want to be a bit more explicit here perhaps? Have some good > >>> proposal text to insert? > >>> > > >> > >> Sean, do you have any recommendations on the text that can go in the kernel doc for > >> this property to clarify that it depends on the kernel implementation whether we want > >> to use this link-status property to do the retraining or not. Leaving it BAD and not doing > >> anything will be fine too. > >> > > > > I'd prefer something like: > > > > * In usual working scenario, this link status property will always be set to > > * "GOOD". If something fails during or after a mode set, the driver may > > * set this 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 can not 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 difference being that we don't need to mention modes since that > > will be handled through the normal avenue and we don't necessarily > > want to remove any modes from the list. > > > > Sean > > > > > >> Manasi > >>> > > + * 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. > >>> > > >>> > extra space > >>> > > >>> > > + * > >>> > > + * 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. > >>> > > >>> > Hmm, I suppose this answers my last question. It's too bad we can't > >>> > rely on this, since we'll > >>> > just end up with each driver rolling their own re-training logic on > >>> > top of possibly supporting > >>> > this (but probably not, since it's really just overhead). Perhaps this > >>> > is an overly pessimistic > >>> > view? > >>> > >>> You can forgo any re-training of course, that should work too. It's just > >>> that DP spec and everyone else seem to recommend to at least do some > >>> minimal effort (or maybe even upfront link training) here. But totally not > >>> doing anything if the link is bad should be ok too (and probably really > >>> simple to implement for drivers, maybe even with a dp_hpd_handler() helper > >>> function which checks link status and set link-status to bad&fires off the > >>> uevent if needed. > >>> > >>> Again, do you have some sample prose to add here to clarify this? > >>> > >>> Thanks for taking a look at this. > >>> > >>> Cheers, Daniel > >>> > >>> > > + * 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; > >>> > > + > >>> > > + /* Make sure the mutex is grabbed */ > >>> > > + lockdep_assert_held(&dev->mode_config.mutex); > >>> > > + connector->link_status = link_status; > >>> > > + drm_object_property_set_value(&connector->base, > >>> > > + dev->mode_config.link_status_property, > >>> > > + link_status); > >>> > > +} > >>> > > +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..3d513ab 100644 > >>> > > --- a/include/drm/drm_connector.h > >>> > > +++ b/include/drm/drm_connector.h > >>> > > @@ -213,6 +213,9 @@ struct drm_connector_state { > >>> > > > >>> > > struct drm_encoder *best_encoder; > >>> > > > >>> > > + /* Connector Link status */ > >>> > > + unsigned int link_status; > >>> > > + > >>> > > struct drm_atomic_state *state; > >>> > > }; > >>> > > > >>> > > @@ -695,6 +698,12 @@ 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; > >>> > > + > >>> > > + /* Connector Link status > >>> > > + * 0: If the link is Good > >>> > > + * 1: If the link is Bad > >>> > > + */ > >>> > > + int link_status; > >>> > > }; > >>> > > > >>> > > #define obj_to_connector(x) container_of(x, struct drm_connector, base) > >>> > > @@ -767,12 +776,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); > >>> > > - > >>> > > >>> > lost a line here > >>> > > >>> > > 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 > >>> > _______________________________________________ > >>> > 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
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 89737e4..644d19f 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1087,6 +1087,9 @@ 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) { + 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 +1138,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_connector.c b/drivers/gpu/drm/drm_connector.c index 5a45262..4576c9f 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 @@ -616,7 +626,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, * path property the MST manager created. Userspace cannot change this * property. * TILE: - * Connector tile group property to indicate how a set of DRM connector + * Connector tile group property to indicate how a set of DRM connector * compose together into one logical screen. This is used by both high-res * external screens (often only using a single cable, but exposing multiple * DP MST sinks), or high-res integrated panels (like dual-link DSI) which @@ -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,43 @@ 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; + + /* Make sure the mutex is grabbed */ + lockdep_assert_held(&dev->mode_config.mutex); + connector->link_status = link_status; + drm_object_property_set_value(&connector->base, + dev->mode_config.link_status_property, + link_status); +} +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..3d513ab 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -213,6 +213,9 @@ struct drm_connector_state { struct drm_encoder *best_encoder; + /* Connector Link status */ + unsigned int link_status; + struct drm_atomic_state *state; }; @@ -695,6 +698,12 @@ 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; + + /* Connector Link status + * 0: If the link is Good + * 1: If the link is Bad + */ + int link_status; }; #define obj_to_connector(x) container_of(x, struct drm_connector, base) @@ -767,12 +776,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. 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> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> --- drivers/gpu/drm/drm_atomic.c | 5 ++++ drivers/gpu/drm/drm_connector.c | 65 +++++++++++++++++++++++++++++++++++++++-- include/drm/drm_connector.h | 12 +++++++- include/drm/drm_mode_config.h | 5 ++++ include/uapi/drm/drm_mode.h | 4 +++ 5 files changed, 88 insertions(+), 3 deletions(-)