Re: [PATCH RFC] drm: Add a new connector atomic property for link status

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Nov 22, 2016 at 8:30 PM, Manasi Navare
<manasi.d.navare@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux