On Fri, Dec 02, 2016 at 05:26:35PM +0100, Daniel Vetter wrote:
> On Tue, Nov 29, 2016 at 11:30:31PM -0800, Manasi Navare wrote:
> > At the time userspace does setcrtc, we've already promised the mode
> > would work. The promise is based on the theoretical capabilities of
> > the link, but it's possible we can't reach this in practice. The DP
> > spec describes how the link should be reduced, but we can't reduce
> > the link below the requirements of the mode. Black screen follows.
> >
> > One idea would be to have setcrtc return a failure. However, it
> > already should not fail as the atomic checks have passed. It would
> > also conflict with the idea of making setcrtc asynchronous in the
> > future, returning before the actual mode setting and link training.
> >
> > Another idea is to train the link "upfront" at hotplug time, before
> > pruning the mode list, so that we can do the pruning based on
> > practical not theoretical capabilities. However, the changes for link
> > training are pretty drastic, all for the sake of error handling and
> > DP compliance, when the most common happy day scenario is the current
> > approach of link training at mode setting time, using the optimal
> > parameters for the mode. It is also not certain all hardware could do
> > this without the pipe on; not even all our hardware can do this. Some
> > of this can be solved, but not trivially.
> >
> > Both of the above ideas also fail to address link degradation *during*
> > operation.
> >
> > The solution is to add a new "link-status" connector property in order
> > to address link training failure in a way that:
> > a) changes the current happy day scenario as little as possible, to
> > avoid regressions, b) can be implemented the same way by all drm
> > drivers, c) is still opt-in for the drivers and userspace, and opting
> > out doesn't regress the user experience, d) doesn't prevent drivers
> > from implementing better or alternate approaches, possibly without
> > userspace involvement. And, of course, handles all the issues presented.
> > In the usual happy day scenario, this is always "good". If something
> > fails during or after a mode set, the kernel driver can set the link
> > status to "bad" and issue a hotplug uevent for userspace to have it
> > re-check the valid modes through GET_CONNECTOR IOCTL, and try modeset
> > again. If the theoretical capabilities of the link can't be reached,
> > the mode list is trimmed based on that.
> >
> > v4:
> > * Add comments in kernel-doc format (Daniel Vetter)
> > * Update the kernel-doc for link-status (Sean Paul)
> > v3:
> > * Fixed a build error (Jani Saarinen)
> > v2:
> > * Removed connector->link_status (Daniel Vetter)
> > * Set connector->state->link_status in drm_mode_connector_set_link_status_property
> > (Daniel Vetter)
> > * Set the connector_changed flag to true if connector->state->link_status changed.
> > * Reset link_status to GOOD in update_output_state (Daniel Vetter)
> > * Never allow userspace to set link status from Good To Bad (Daniel Vetter)
> >
> > Acked-by: Tony Cheng <tony.cheng@xxxxxxx>
> > Acked-by: Harry Wentland <harry.wentland@xxxxxxx>
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Sean Paul <seanpaul@xxxxxxxxxxxx>
> > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_atomic.c | 10 +++++++
> > drivers/gpu/drm/drm_atomic_helper.c | 8 ++++++
> > drivers/gpu/drm/drm_connector.c | 54 ++++++++++++++++++++++++++++++++++++-
> > include/drm/drm_connector.h | 19 +++++++++++++
> > include/drm/drm_mode_config.h | 5 ++++
> > include/uapi/drm/drm_mode.h | 4 +++
> > 6 files changed, 99 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 89737e4..990f013 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1087,6 +1087,14 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
> > * now?) atomic writes to DPMS property:
> > */
> > return -EINVAL;
> > + } else if (property == config->link_status_property) {
> > + /* Never downgrade from GOOD to BAD on userspace's request here,
> > + * only hw issues can do that.
> > + */
> > + if (state->link_status == DRM_LINK_STATUS_GOOD)
> > + return 0;
> > + state->link_status = val;
> > + return 0;
> > } else if (connector->funcs->atomic_set_property) {
> > return connector->funcs->atomic_set_property(connector,
> > state, property, val);
> > @@ -1135,6 +1143,8 @@ static void drm_atomic_connector_print_state(struct drm_printer *p,
> > *val = (state->crtc) ? state->crtc->base.id : 0;
> > } else if (property == config->dpms_property) {
> > *val = connector->dpms;
> > + } else if (property == config->link_status_property) {
> > + *val = state->link_status;
> > } else if (connector->funcs->atomic_get_property) {
> > return connector->funcs->atomic_get_property(connector,
> > state, property, val);
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 494680c..962ed66 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -519,6 +519,13 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
> > connector_state);
> > if (ret)
> > return ret;
> > + if (connector->state->crtc) {
> > + crtc_state = drm_atomic_get_existing_crtc_state(state,
> > + connector->state->crtc);
> > + if (connector->state->link_status !=
> > + connector_state->link_status)
> > + crtc_state->connectors_changed = true;
> > + }
>
> See my previous review, this looks should imo be moved to a more obvious
> place than hidden deep down in the callchain.
>
> > }
Yes I have looked at your previous review and this is inside drm_atomic_helper_check_modeset()
function just inside the connectors loop. Its not in the handle_conflicting_encoders function.
Its weird that in the diff it is showing up here.
If you pull the patch you will see that it is written as follows:
for_each_connector_in_state(state, connector, connector_state, i) {
/*
* This only sets crtc->connectors_changed for routing changes,
* drivers must set crtc->connectors_changed themselves when
* connector properties need to be updated.
*/
ret = update_connector_routing(state, connector,
connector_state);
if (ret)
return ret;
if (connector->state->crtc) {
crtc_state = drm_atomic_get_existing_crtc_state(state,
connector->state->crtc);
if (connector->state->link_status !=
connector_state->link_status)
crtc_state->connectors_changed = true;
}
}
Is this correct?
Manasi
> >
> > /*
> > @@ -2258,6 +2265,7 @@ static int update_output_state(struct drm_atomic_state *state,
> > NULL);
> > if (ret)
> > return ret;
> > + conn_state->link_status = DRM_LINK_STATUS_GOOD;
>
> Iirc in our discussion we said a small comment here would be good, like
>
> /* Make sure legacy setCrtc always re-trains. */
>
> Might also be good to update the kernel-doc of
> drm_atomic_helper_set_config(). Something like
>
> * NOTE: For backwards compatibility with old userspace this automatically
> * resets the "link-status" property to GOOD, to force any link
> * re-training. The SETCRTC ioctl does not define whether an update does
> * need a full modeset or just a plane update, hence we're allowed to do
> * that. See also drm_mode_connector_set_link_status_property().
> *
>
> > }
> > }
> >
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 5a45262..8902367 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -243,6 +243,10 @@ int drm_connector_init(struct drm_device *dev,
> > drm_object_attach_property(&connector->base,
> > config->dpms_property, 0);
> >
> > + drm_object_attach_property(&connector->base,
> > + config->link_status_property,
> > + 0);
> > +
> > if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
> > drm_object_attach_property(&connector->base, config->prop_crtc_id, 0);
> > }
> > @@ -506,6 +510,12 @@ const char *drm_get_subpixel_order_name(enum subpixel_order order)
> > };
> > DRM_ENUM_NAME_FN(drm_get_dpms_name, drm_dpms_enum_list)
> >
> > +static const struct drm_prop_enum_list drm_link_status_enum_list[] = {
> > + { DRM_MODE_LINK_STATUS_GOOD, "Good" },
> > + { DRM_MODE_LINK_STATUS_BAD, "Bad" },
> > +};
> > +DRM_ENUM_NAME_FN(drm_get_link_status_name, drm_link_status_enum_list)
> > +
> > /**
> > * drm_display_info_set_bus_formats - set the supported bus formats
> > * @info: display info to store bus formats in
> > @@ -625,7 +635,11 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
> > * tiling and virtualize both &drm_crtc and &drm_plane if needed. Drivers
> > * should update this value using drm_mode_connector_set_tile_property().
> > * Userspace cannot change this property.
> > - *
>
> One more from last round: Please leave this whitespace intact ...
>
> > + * link-status:
> > + * Connector link-status property to indicate the status of link. The default
> > + * value of link-status is "GOOD". If something fails during or after modeset,
> > + * the kernel driver may set this to "BAD" and issue a hotplug uevent. Drivers
> > + * should update this value using drm_mode_connector_set_link_status_property().
>
> ... and add an empty line here.
>
> > * Connectors also have one standardized atomic property:
> > *
> > * CRTC_ID:
> > @@ -666,6 +680,13 @@ int drm_connector_create_standard_properties(struct drm_device *dev)
> > return -ENOMEM;
> > dev->mode_config.tile_property = prop;
> >
> > + prop = drm_property_create_enum(dev, 0, "link-status",
> > + drm_link_status_enum_list,
> > + ARRAY_SIZE(drm_link_status_enum_list));
> > + if (!prop)
> > + return -ENOMEM;
> > + dev->mode_config.link_status_property = prop;
> > +
> > return 0;
> > }
> >
> > @@ -995,6 +1016,37 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector,
> > }
> > EXPORT_SYMBOL(drm_mode_connector_update_edid_property);
> >
> > +/**
> > + * drm_mode_connector_set_link_status_property - Set link status property of a connector
> > + * @connector: drm connector
> > + * @link_status: new value of link status property (0: Good, 1: Bad)
> > + *
> > + * In usual working scenario, this link status property will always be set to
> > + * "GOOD". If something fails during or after a mode set, the kernel driver
> > + * may set this link status property to "BAD". The caller then needs to send a
> > + * hotplug uevent for userspace to re-check the valid modes through
> > + * GET_CONNECTOR_IOCTL and retry modeset.
> > + *
> > + * Note: Drivers cannot rely on userspace to support this property and
> > + * issue a modeset. As such, they may choose to handle issues (like
> > + * re-training a link) without userspace's intervention.
> > + *
> > + * The reason for adding this property is to handle link training failures, but
> > + * it is not limited to DP or link training. For example, if we implement
> > + * asynchronous setcrtc, this property can be used to report any failures in that.
> > + */
> > +void drm_mode_connector_set_link_status_property(struct drm_connector *connector,
>
> Hm, tiny new bikeshed: I'd drop the _property postfix here. The function
> name is already really long as-is, and the fact that the link status is
> exposed as a property is kinda irrelevant for kernel driver writers. But
> up to you.
> -Daniel
>
> > + uint64_t link_status)
> > +{
> > + struct drm_device *dev = connector->dev;
> > +
> > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > + connector->state->link_status = link_status;
> > + drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > +
> > +}
> > +EXPORT_SYMBOL(drm_mode_connector_set_link_status_property);
> > +
> > int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
> > struct drm_property *property,
> > uint64_t value)
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 34f9741..53c5f70 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -90,6 +90,17 @@ enum subpixel_order {
> > };
> >
> > /**
> > + * enum drm_link_status - connector's link_status property value
> > + *
> > + * This enum is used as the connector's link status property value.
> > + * It is set to the values defined in uapi.
> > + */
> > +enum drm_link_status {
> > + DRM_LINK_STATUS_GOOD = DRM_MODE_LINK_STATUS_GOOD,
> > + DRM_LINK_STATUS_BAD = DRM_MODE_LINK_STATUS_BAD,
> > +};
> > +
> > +/**
> > * struct drm_display_info - runtime data about the connected sink
> > *
> > * Describes a given display (e.g. CRT or flat panel) and its limitations. For
> > @@ -213,6 +224,12 @@ struct drm_connector_state {
> >
> > struct drm_encoder *best_encoder;
> >
> > + /**
> > + * @link_status: Connector link_status to keep track of whether link is
> > + * GOOD or BAD to notify userspace if retraining is necessary.
> > + */
> > + enum drm_link_status link_status;
> > +
> > struct drm_atomic_state *state;
> > };
> >
> > @@ -773,6 +790,8 @@ int drm_mode_connector_set_path_property(struct drm_connector *connector,
> > int drm_mode_connector_set_tile_property(struct drm_connector *connector);
> > int drm_mode_connector_update_edid_property(struct drm_connector *connector,
> > const struct edid *edid);
> > +void drm_mode_connector_set_link_status_property(struct drm_connector *connector,
> > + uint64_t link_status);
> >
> > /**
> > * struct drm_tile_group - Tile group metadata
> > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > index bf9991b..86faee4 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -431,6 +431,11 @@ struct drm_mode_config {
> > */
> > struct drm_property *tile_property;
> > /**
> > + * @link_status_property: Default connector property for link status
> > + * of a connector
> > + */
> > + struct drm_property *link_status_property;
> > + /**
> > * @plane_type_property: Default plane property to differentiate
> > * CURSOR, PRIMARY and OVERLAY legacy uses of planes.
> > */
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index 728790b..309c478 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -123,6 +123,10 @@
> > #define DRM_MODE_DIRTY_ON 1
> > #define DRM_MODE_DIRTY_ANNOTATE 2
> >
> > +/* Link Status options */
> > +#define DRM_MODE_LINK_STATUS_GOOD 0
> > +#define DRM_MODE_LINK_STATUS_BAD 1
> > +
> > struct drm_mode_modeinfo {
> > __u32 clock;
> > __u16 hdisplay;
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
