On Wed, Nov 23, 2016 at 11:00:59AM +0100, Daniel Vetter wrote:
> On Tue, Nov 22, 2016 at 05:30:21PM -0800, Manasi Navare 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.
> >
> > Cc: Jani Nikula <jani.nikula at linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter at intel.com>
> > Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> > ---
> > 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
> > * 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
> > + * 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.
> > + *
> > + * 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.
> > + * 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);
>
> For atomic the lock we need is dev->mode_config.connection_mutex); Maybe
> we should just grab that here, from a quick check it shouldn't result in a
> deadlock.
>
So after grabing the connection mutex, do we still need the lockdep assert held
for
mode_config.mutex?
This gets called from the work func that holds the mode-config.mutex, do we
need to drop that
mutex then or is it okay to hold mode_config.mutex as well as connection mutex?
> > + connector->link_status = link_status;
>
> You need to set connector->state->link_status here for an atomic property.
>
Yes I need to set the connector state here. I will add that.
> > + drm_object_property_set_value(&connector->base,
> > + dev->mode_config.link_status_property,
> > + link_status);
>
> And this here isn't needed anymore with atomic.
>
So we no longer have the link_status property in mode_config struct?
All we have is the link_status in dmr_connector_state?
Manasi
> > +}
> > +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;
>
> enum drm_link_status {
> DRM_LINK_STATUS_GOOD = DRM_MODE_LINK_STATUS_GOOD;
> DRM_LINK_STATUS_BAD = DRM_MODE_LINK_STATUS_GOOD;
> };
>
> This way you don't need a comment explaing that 0 is good and 1 is bad.
>
> > +
> > 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;
>
> This one needs to go, we don't want to duplicate this information.
>
> > };
This was added so we can easily grab this value within intel_dp functions,
or should those functions directly read the connector->state->link_sttaus?
> >
> > #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);
> > -
> > 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
>
> The change to reset link_status to good for SETCRTC seems to be missing.
> Something like the below (totally untested) should get the job done.
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 494680c9056e..5f47049d611e 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2258,6 +2258,8 @@ static int update_output_state(struct drm_atomic_state
> *state,
> NULL);
> if (ret)
> return ret;
> +
> + conn_state->link_status = DRM_LINK_STATUS_GOOD;
> }
> }
>
> Small exercise for you: Follow the call chain of this function and figure
> out how it works. And let's hope I did it right and it does work ;-)
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
