Re: [Intel-gfx] [PATCH 1/4] drm: Add a new connector atomic property for link status

From: Daniel Vetter <daniel@ffwll.ch>
To: Manasi Navare <manasi.d.navare@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 1/4] drm: Add a new connector atomic property for link status
Date: Tue, 6 Dec 2016 08:23:05 +0100	[thread overview]
Message-ID: <20161206072305.mfldb5h3fwms3ifq@phenom.ffwll.local> (raw)
In-Reply-To: <1480984058-552-2-git-send-email-manasi.d.navare@intel.com>

On Mon, Dec 05, 2016 at 04:27:35PM -0800, Manasi Navare wrote:
> At the time userspace does setcrtc, we've already promised the mode
> would work. The promise is based on the theoretical capabilities of
> the link, but it's possible we can't reach this in practice. The DP
> spec describes how the link should be reduced, but we can't reduce
> the link below the requirements of the mode. Black screen follows.
> 
> One idea would be to have setcrtc return a failure. However, it
> already should not fail as the atomic checks have passed. It would
> also conflict with the idea of making setcrtc asynchronous in the
> future, returning before the actual mode setting and link training.
> 
> Another idea is to train the link "upfront" at hotplug time, before
> pruning the mode list, so that we can do the pruning based on
> practical not theoretical capabilities. However, the changes for link
> training are pretty drastic, all for the sake of error handling and
> DP compliance, when the most common happy day scenario is the current
> approach of link training at mode setting time, using the optimal
> parameters for the mode. It is also not certain all hardware could do
> this without the pipe on; not even all our hardware can do this. Some
> of this can be solved, but not trivially.
> 
> Both of the above ideas also fail to address link degradation *during*
> operation.
> 
> The solution is to add a new "link-status" connector property in order
> to address link training failure in a way that:
> a) changes the current happy day scenario as little as possible, to
> avoid regressions, b) can be implemented the same way by all drm
> drivers, c) is still opt-in for the drivers and userspace, and opting
> out doesn't regress the user experience, d) doesn't prevent drivers
> from implementing better or alternate approaches, possibly without
> userspace involvement. And, of course, handles all the issues presented.
> In the usual happy day scenario, this is always "good". If something
> fails during or after a mode set, the kernel driver can set the link
> status to "bad" and issue a hotplug uevent for userspace to have it
> re-check the valid modes through GET_CONNECTOR IOCTL, and try modeset
> again. If the theoretical capabilities of the link can't be reached,
> the mode list is trimmed based on that.
> 
> v4:
> * Add comments in kernel-doc format (Daniel Vetter)
> * Update the kernel-doc for link-status (Sean Paul)
> v3:
> * Fixed a build error (Jani Saarinen)
> v2:
> * Removed connector->link_status (Daniel Vetter)
> * Set connector->state->link_status in drm_mode_connector_set_link_status_property
> (Daniel Vetter)
> * Set the connector_changed flag to true if connector->state->link_status changed.
> * Reset link_status to GOOD in update_output_state (Daniel Vetter)
> * Never allow userspace to set link status from Good To Bad (Daniel Vetter)
> 
> Acked-by: Tony Cheng <tony.cheng@amd.com>
> Acked-by: Harry Wentland <harry.wentland@amd.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sean Paul <seanpaul@chromium.org>

Didn't Sean r-b this already?

> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>

Looks all nice and tidy, great work.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Can be merged as soon as we have an ack from Martin that the -modesetting
side is ready to go too.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c        | 10 +++++++
>  drivers/gpu/drm/drm_atomic_helper.c | 15 +++++++++++
>  drivers/gpu/drm/drm_connector.c     | 53 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h         | 19 +++++++++++++
>  include/drm/drm_mode_config.h       |  5 ++++
>  include/uapi/drm/drm_mode.h         |  4 +++
>  6 files changed, 106 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 19d7bcb..70a5152 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1087,6 +1087,14 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
>  		 * now?) atomic writes to DPMS property:
>  		 */
>  		return -EINVAL;
> +	} else if (property == config->link_status_property) {
> +		/* Never downgrade from GOOD to BAD on userspace's request here,
> +		 * only hw issues can do that.
> +		 */
> +		if (state->link_status == DRM_LINK_STATUS_GOOD)
> +			return 0;
> +		state->link_status = val;
> +		return 0;
>  	} else if (connector->funcs->atomic_set_property) {
>  		return connector->funcs->atomic_set_property(connector,
>  				state, property, val);
> @@ -1135,6 +1143,8 @@ static void drm_atomic_connector_print_state(struct drm_printer *p,
>  		*val = (state->crtc) ? state->crtc->base.id : 0;
>  	} else if (property == config->dpms_property) {
>  		*val = connector->dpms;
> +	} else if (property == config->link_status_property) {
> +		*val = state->link_status;
>  	} else if (connector->funcs->atomic_get_property) {
>  		return connector->funcs->atomic_get_property(connector,
>  				state, property, val);
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 494680c..3d6c1ce 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -519,6 +519,13 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
>  					       connector_state);
>  		if (ret)
>  			return ret;
> +		if (connector->state->crtc) {
> +			crtc_state = drm_atomic_get_existing_crtc_state(state,
> +									connector->state->crtc);
> +			if (connector->state->link_status !=
> +			    connector_state->link_status)
> +				crtc_state->connectors_changed = true;
> +		}
>  	}
>  
>  	/*
> @@ -2258,6 +2265,8 @@ static int update_output_state(struct drm_atomic_state *state,
>  								NULL);
>  			if (ret)
>  				return ret;
> +			/* Make sure legacy setCrtc always re-trains */
> +			conn_state->link_status = DRM_LINK_STATUS_GOOD;
>  		}
>  	}
>  
> @@ -2301,6 +2310,12 @@ static int update_output_state(struct drm_atomic_state *state,
>   *
>   * Provides a default crtc set_config handler using the atomic driver interface.
>   *
> + * NOTE: For backwards compatibility with old userspace this automatically
> + * resets the "link-status" property to GOOD, to force any link
> + * re-training. The SETCRTC ioctl does not define whether an update does
> + * need a full modeset or just a plane update, hence we're allowed to do
> + * that. See also drm_mode_connector_set_link_status_property().
> + *
>   * Returns:
>   * Returns 0 on success, negative errno numbers on failure.
>   */
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 5a45262..eee038a 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -243,6 +243,10 @@ int drm_connector_init(struct drm_device *dev,
>  	drm_object_attach_property(&connector->base,
>  				      config->dpms_property, 0);
>  
> +	drm_object_attach_property(&connector->base,
> +				   config->link_status_property,
> +				   0);
> +
>  	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>  		drm_object_attach_property(&connector->base, config->prop_crtc_id, 0);
>  	}
> @@ -506,6 +510,12 @@ const char *drm_get_subpixel_order_name(enum subpixel_order order)
>  };
>  DRM_ENUM_NAME_FN(drm_get_dpms_name, drm_dpms_enum_list)
>  
> +static const struct drm_prop_enum_list drm_link_status_enum_list[] = {
> +	{ DRM_MODE_LINK_STATUS_GOOD, "Good" },
> +	{ DRM_MODE_LINK_STATUS_BAD, "Bad" },
> +};
> +DRM_ENUM_NAME_FN(drm_get_link_status_name, drm_link_status_enum_list)
> +
>  /**
>   * drm_display_info_set_bus_formats - set the supported bus formats
>   * @info: display info to store bus formats in
> @@ -625,6 +635,11 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
>   * 	tiling and virtualize both &drm_crtc and &drm_plane if needed. Drivers
>   * 	should update this value using drm_mode_connector_set_tile_property().
>   * 	Userspace cannot change this property.
> + * link-status:
> + *      Connector link-status property to indicate the status of link. The default
> + *      value of link-status is "GOOD". If something fails during or after modeset,
> + *      the kernel driver may set this to "BAD" and issue a hotplug uevent. Drivers
> + *      should update this value using drm_mode_connector_set_link_status_property().
>   *
>   * Connectors also have one standardized atomic property:
>   *
> @@ -666,6 +681,13 @@ int drm_connector_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.tile_property = prop;
>  
> +	prop = drm_property_create_enum(dev, 0, "link-status",
> +					drm_link_status_enum_list,
> +					ARRAY_SIZE(drm_link_status_enum_list));
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.link_status_property = prop;
> +
>  	return 0;
>  }
>  
> @@ -995,6 +1017,37 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_mode_connector_update_edid_property);
>  
> +/**
> + * drm_mode_connector_set_link_status_property - Set link status property of a connector
> + * @connector: drm connector
> + * @link_status: new value of link status property (0: Good, 1: Bad)
> + *
> + * In usual working scenario, this link status property will always be set to
> + * "GOOD". If something fails during or after a mode set, the kernel driver
> + * may set this link status property to "BAD". The caller then needs to send a
> + * hotplug uevent for userspace to re-check the valid modes through
> + * GET_CONNECTOR_IOCTL and retry modeset.
> + *
> + * Note: Drivers cannot rely on userspace to support this property and
> + * issue a modeset. As such, they may choose to handle issues (like
> + * re-training a link) without userspace's intervention.
> + *
> + * The reason for adding this property is to handle link training failures, but
> + * it is not limited to DP or link training. For example, if we implement
> + * asynchronous setcrtc, this property can be used to report any failures in that.
> + */
> +void drm_mode_connector_set_link_status_property(struct drm_connector *connector,
> +						 uint64_t link_status)
> +{
> +	struct drm_device *dev = connector->dev;
> +
> +	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +	connector->state->link_status = link_status;
> +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +
> +}
> +EXPORT_SYMBOL(drm_mode_connector_set_link_status_property);
> +
>  int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
>  				    struct drm_property *property,
>  				    uint64_t value)
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 1218a0c..64e07c5 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -90,6 +90,17 @@ enum subpixel_order {
>  };
>  
>  /**
> + * enum drm_link_status - connector's link_status property value
> + *
> + * This enum is used as the connector's link status property value.
> + * It is set to the values defined in uapi.
> + */
> +enum drm_link_status {
> +	DRM_LINK_STATUS_GOOD = DRM_MODE_LINK_STATUS_GOOD,
> +	DRM_LINK_STATUS_BAD = DRM_MODE_LINK_STATUS_BAD,
> +};
> +
> +/**
>   * struct drm_display_info - runtime data about the connected sink
>   *
>   * Describes a given display (e.g. CRT or flat panel) and its limitations. For
> @@ -213,6 +224,12 @@ struct drm_connector_state {
>  
>  	struct drm_encoder *best_encoder;
>  
> +	/**
> +	 * @link_status: Connector link_status to keep track of whether link is
> +	 * GOOD or BAD to notify userspace if retraining is necessary.
> +	 */
> +	enum drm_link_status link_status;
> +
>  	struct drm_atomic_state *state;
>  };
>  
> @@ -776,6 +793,8 @@ int drm_mode_connector_set_path_property(struct drm_connector *connector,
>  int drm_mode_connector_set_tile_property(struct drm_connector *connector);
>  int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>  					    const struct edid *edid);
> +void drm_mode_connector_set_link_status_property(struct drm_connector *connector,
> +						 uint64_t link_status);
>  
>  /**
>   * struct drm_tile_group - Tile group metadata
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index bf9991b..86faee4 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -431,6 +431,11 @@ struct drm_mode_config {
>  	 */
>  	struct drm_property *tile_property;
>  	/**
> +	 * @link_status_property: Default connector property for link status
> +	 * of a connector
> +	 */
> +	struct drm_property *link_status_property;
> +	/**
>  	 * @plane_type_property: Default plane property to differentiate
>  	 * CURSOR, PRIMARY and OVERLAY legacy uses of planes.
>  	 */
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 728790b..309c478 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -123,6 +123,10 @@
>  #define DRM_MODE_DIRTY_ON       1
>  #define DRM_MODE_DIRTY_ANNOTATE 2
>  
> +/* Link Status options */
> +#define DRM_MODE_LINK_STATUS_GOOD	0
> +#define DRM_MODE_LINK_STATUS_BAD	1
> +
>  struct drm_mode_modeinfo {
>  	__u32 clock;
>  	__u16 hdisplay;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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