On Thu, Dec 1, 2016 at 1:58 PM, Manasi Navare <manasi.d.nav...@intel.com> 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 <seanp...@chromium.org>

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.ch...@amd.com>
>> Acked-by: Harry Wentland <harry.wentl...@amd.com>
>> Cc: Jani Nikula <jani.nik...@linux.intel.com>
>> Cc: Daniel Vetter <daniel.vet...@intel.com>
>> Cc: Ville Syrjala <ville.syrj...@linux.intel.com>
>> Cc: Chris Wilson <ch...@chris-wilson.co.uk>
>> Cc: Sean Paul <seanp...@chromium.org>
>> Signed-off-by: Manasi Navare <manasi.d.nav...@intel.com>
>> ---
>>  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to