On Thu, Dec 1, 2016 at 1:58 PM, Manasi Navare <manasi.d.navare@xxxxxxxxx> wrote:
> Sean, could you please review this patch, I have tried to address
> all the comments from you.
>
Comments look good to me.
Reviewed-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
Sean
> Regards
> Manasi
>
> On Tue, Nov 29, 2016 at 11:30:31PM -0800, Manasi Navare wrote:
>> At the time userspace does setcrtc, we've already promised the mode
>> would work. The promise is based on the theoretical capabilities of
>> the link, but it's possible we can't reach this in practice. The DP
>> spec describes how the link should be reduced, but we can't reduce
>> the link below the requirements of the mode. Black screen follows.
>>
>> One idea would be to have setcrtc return a failure. However, it
>> already should not fail as the atomic checks have passed. It would
>> also conflict with the idea of making setcrtc asynchronous in the
>> future, returning before the actual mode setting and link training.
>>
>> Another idea is to train the link "upfront" at hotplug time, before
>> pruning the mode list, so that we can do the pruning based on
>> practical not theoretical capabilities. However, the changes for link
>> training are pretty drastic, all for the sake of error handling and
>> DP compliance, when the most common happy day scenario is the current
>> approach of link training at mode setting time, using the optimal
>> parameters for the mode. It is also not certain all hardware could do
>> this without the pipe on; not even all our hardware can do this. Some
>> of this can be solved, but not trivially.
>>
>> Both of the above ideas also fail to address link degradation *during*
>> operation.
>>
>> The solution is to add a new "link-status" connector property in order
>> to address link training failure in a way that:
>> a) changes the current happy day scenario as little as possible, to
>> avoid regressions, b) can be implemented the same way by all drm
>> drivers, c) is still opt-in for the drivers and userspace, and opting
>> out doesn't regress the user experience, d) doesn't prevent drivers
>> from implementing better or alternate approaches, possibly without
>> userspace involvement. And, of course, handles all the issues presented.
>> In the usual happy day scenario, this is always "good". If something
>> fails during or after a mode set, the kernel driver can set the link
>> status to "bad" and issue a hotplug uevent for userspace to have it
>> re-check the valid modes through GET_CONNECTOR IOCTL, and try modeset
>> again. If the theoretical capabilities of the link can't be reached,
>> the mode list is trimmed based on that.
>>
>> v4:
>> * Add comments in kernel-doc format (Daniel Vetter)
>> * Update the kernel-doc for link-status (Sean Paul)
>> v3:
>> * Fixed a build error (Jani Saarinen)
>> v2:
>> * Removed connector->link_status (Daniel Vetter)
>> * Set connector->state->link_status in drm_mode_connector_set_link_status_property
>> (Daniel Vetter)
>> * Set the connector_changed flag to true if connector->state->link_status changed.
>> * Reset link_status to GOOD in update_output_state (Daniel Vetter)
>> * Never allow userspace to set link status from Good To Bad (Daniel Vetter)
>>
>> Acked-by: Tony Cheng <tony.cheng@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;
>> + }
>> }
>>
>> /*
>> @@ -2258,6 +2265,7 @@ static int update_output_state(struct drm_atomic_state *state,
>> NULL);
>> if (ret)
>> return ret;
>> + conn_state->link_status = DRM_LINK_STATUS_GOOD;
>> }
>> }
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index 5a45262..8902367 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -243,6 +243,10 @@ int drm_connector_init(struct drm_device *dev,
>> drm_object_attach_property(&connector->base,
>> config->dpms_property, 0);
>>
>> + drm_object_attach_property(&connector->base,
>> + config->link_status_property,
>> + 0);
>> +
>> if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>> drm_object_attach_property(&connector->base, config->prop_crtc_id, 0);
>> }
>> @@ -506,6 +510,12 @@ const char *drm_get_subpixel_order_name(enum subpixel_order order)
>> };
>> DRM_ENUM_NAME_FN(drm_get_dpms_name, drm_dpms_enum_list)
>>
>> +static const struct drm_prop_enum_list drm_link_status_enum_list[] = {
>> + { DRM_MODE_LINK_STATUS_GOOD, "Good" },
>> + { DRM_MODE_LINK_STATUS_BAD, "Bad" },
>> +};
>> +DRM_ENUM_NAME_FN(drm_get_link_status_name, drm_link_status_enum_list)
>> +
>> /**
>> * drm_display_info_set_bus_formats - set the supported bus formats
>> * @info: display info to store bus formats in
>> @@ -625,7 +635,11 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
>> * tiling and virtualize both &drm_crtc and &drm_plane if needed. Drivers
>> * should update this value using drm_mode_connector_set_tile_property().
>> * Userspace cannot change this property.
>> - *
>> + * link-status:
>> + * Connector link-status property to indicate the status of link. The default
>> + * value of link-status is "GOOD". If something fails during or after modeset,
>> + * the kernel driver may set this to "BAD" and issue a hotplug uevent. Drivers
>> + * should update this value using drm_mode_connector_set_link_status_property().
>> * Connectors also have one standardized atomic property:
>> *
>> * CRTC_ID:
>> @@ -666,6 +680,13 @@ int drm_connector_create_standard_properties(struct drm_device *dev)
>> return -ENOMEM;
>> dev->mode_config.tile_property = prop;
>>
>> + prop = drm_property_create_enum(dev, 0, "link-status",
>> + drm_link_status_enum_list,
>> + ARRAY_SIZE(drm_link_status_enum_list));
>> + if (!prop)
>> + return -ENOMEM;
>> + dev->mode_config.link_status_property = prop;
>> +
>> return 0;
>> }
>>
>> @@ -995,6 +1016,37 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>> }
>> EXPORT_SYMBOL(drm_mode_connector_update_edid_property);
>>
>> +/**
>> + * drm_mode_connector_set_link_status_property - Set link status property of a connector
>> + * @connector: drm connector
>> + * @link_status: new value of link status property (0: Good, 1: Bad)
>> + *
>> + * In usual working scenario, this link status property will always be set to
>> + * "GOOD". If something fails during or after a mode set, the kernel driver
>> + * may set this link status property to "BAD". The caller then needs to send a
>> + * hotplug uevent for userspace to re-check the valid modes through
>> + * GET_CONNECTOR_IOCTL and retry modeset.
>> + *
>> + * Note: Drivers cannot rely on userspace to support this property and
>> + * issue a modeset. As such, they may choose to handle issues (like
>> + * re-training a link) without userspace's intervention.
>> + *
>> + * The reason for adding this property is to handle link training failures, but
>> + * it is not limited to DP or link training. For example, if we implement
>> + * asynchronous setcrtc, this property can be used to report any failures in that.
>> + */
>> +void drm_mode_connector_set_link_status_property(struct drm_connector *connector,
>> + uint64_t link_status)
>> +{
>> + struct drm_device *dev = connector->dev;
>> +
>> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> + connector->state->link_status = link_status;
>> + drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> +
>> +}
>> +EXPORT_SYMBOL(drm_mode_connector_set_link_status_property);
>> +
>> int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
>> struct drm_property *property,
>> uint64_t value)
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 34f9741..53c5f70 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -90,6 +90,17 @@ enum subpixel_order {
>> };
>>
>> /**
>> + * enum drm_link_status - connector's link_status property value
>> + *
>> + * This enum is used as the connector's link status property value.
>> + * It is set to the values defined in uapi.
>> + */
>> +enum drm_link_status {
>> + DRM_LINK_STATUS_GOOD = DRM_MODE_LINK_STATUS_GOOD,
>> + DRM_LINK_STATUS_BAD = DRM_MODE_LINK_STATUS_BAD,
>> +};
>> +
>> +/**
>> * struct drm_display_info - runtime data about the connected sink
>> *
>> * Describes a given display (e.g. CRT or flat panel) and its limitations. For
>> @@ -213,6 +224,12 @@ struct drm_connector_state {
>>
>> struct drm_encoder *best_encoder;
>>
>> + /**
>> + * @link_status: Connector link_status to keep track of whether link is
>> + * GOOD or BAD to notify userspace if retraining is necessary.
>> + */
>> + enum drm_link_status link_status;
>> +
>> struct drm_atomic_state *state;
>> };
>>
>> @@ -773,6 +790,8 @@ int drm_mode_connector_set_path_property(struct drm_connector *connector,
>> int drm_mode_connector_set_tile_property(struct drm_connector *connector);
>> int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>> const struct edid *edid);
>> +void drm_mode_connector_set_link_status_property(struct drm_connector *connector,
>> + uint64_t link_status);
>>
>> /**
>> * struct drm_tile_group - Tile group metadata
>> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
>> index bf9991b..86faee4 100644
>> --- a/include/drm/drm_mode_config.h
>> +++ b/include/drm/drm_mode_config.h
>> @@ -431,6 +431,11 @@ struct drm_mode_config {
>> */
>> struct drm_property *tile_property;
>> /**
>> + * @link_status_property: Default connector property for link status
>> + * of a connector
>> + */
>> + struct drm_property *link_status_property;
>> + /**
>> * @plane_type_property: Default plane property to differentiate
>> * CURSOR, PRIMARY and OVERLAY legacy uses of planes.
>> */
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index 728790b..309c478 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -123,6 +123,10 @@
>> #define DRM_MODE_DIRTY_ON 1
>> #define DRM_MODE_DIRTY_ANNOTATE 2
>>
>> +/* Link Status options */
>> +#define DRM_MODE_LINK_STATUS_GOOD 0
>> +#define DRM_MODE_LINK_STATUS_BAD 1
>> +
>> struct drm_mode_modeinfo {
>> __u32 clock;
>> __u16 hdisplay;
>> --
>> 1.9.1
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
