On Wed, Nov 23, 2016 at 10:32:44AM -0500, Sean Paul wrote:
> On Wed, Nov 23, 2016 at 10:15 AM, Jani Nikula
> <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> > On Wed, 23 Nov 2016, Sean Paul <seanpaul@xxxxxxxxxxxx> wrote:
> >> On Wed, Nov 23, 2016 at 3:09 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> >>> On Tue, Nov 22, 2016 at 09:27:35PM -0500, Sean Paul wrote:
> >>>> 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;
> >>>
> >>> I think we should have a check here to make sure userspace never tries to
> >>> set this from "GOOD" to "BAD". "BAD" to "BAD" we need to allow, since with
> >>> atomic userspace is supposed to be able to just restore everything to what
> >>> it was.
> >>>
> >>> I don't think trying to set it to "BAD" should cause an error though, just
> >>> silently keep the link status at "GOOG". So:
> >>>
> >>> /* Never downgrade from GOOD to BAD on userspace's request here,
> >>> * only hw issues can do that. */
> >>> if (state->link_status == GOOD)
> >>> return 0;
> >>
> >> Can we return an error if the transition is invalid?
> >>
> >>> state->link_status = val;
> >>> return 0;
> >>>
> >>>> > + 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)
> >>>
> >>> Well, if you prune it with the same constraints as before, no additional
> >>> modes will be removed and the same list is there. Pruning here refers to
> >>> the ->mode_valid step as done in e.g. drm_helper_probe_single_connector_modes.
> >>>
> >>> We might want to be a bit more explicit here perhaps? Have some good
> >>> proposal text to insert?
> >>
> >> Ah, hmm, I hadn't equated this prune to the normal drm_connector
> >> prune_invalid. I suppose the "based on new information" is what did
> >> it. It seems like the mode_valid pruning will happen automagically
> >> when userspace realizes the connection is BAD and asks for a new mode
> >> list. So is it worth even mentioning pruning the mode list?
> >>
> >>>>
> >>>> > + * 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?
> >>>
> >>> You can forgo any re-training of course, that should work too. It's just
> >>> that DP spec and everyone else seem to recommend to at least do some
> >>> minimal effort (or maybe even upfront link training) here. But totally not
> >>> doing anything if the link is bad should be ok too (and probably really
> >>> simple to implement for drivers, maybe even with a dp_hpd_handler() helper
> >>> function which checks link status and set link-status to bad&fires off the
> >>> uevent if needed.
> >>>
> >>> Again, do you have some sample prose to add here to clarify this?
> >>>
> >>
> >> I don't think it's merely a matter of clarifying the comment, tbh.
> >> Without relying on userspace to fix us, there's not much point in
> >> exposing the property in the first place.
> >>
> >> As a driver author, I need to decide whether I want re-training to
> >> work properly. If I do, I need to handroll it in my driver since I
> >> can't rely on userspace, at which point, implementing the property
> >> isn't meaningful. If I don't care about it being bulletproof, I can
> >> expose the property and let userspace fix me if they know how.
> >>
> >> So I'd prefer to have the helper be a hybrid between this patch and
> >> the previous one that did a full modeset under the covers (perhaps
> >> with a client cap or similar to let us know which path to trust). That
> >> way I can call the helper from my driver and know that I'm going to be
> >> re-trained either way.
> >
> > If your driver is able to handle everything in kernel, then do so by all
> > means. If it can't, defer to userspace, which may or may not fix stuff,
> > depending on whether it looks at the property. If the userspace doesn't
> > do anything, things will be just as good or bad as they are currently,
> > before this property.
> >
> > This allows the drivers to choose how much they can/want to do in
> > kernel, and what they defer to userspace.
> >
>
> Just because I can, doesn't mean I want to :)
>
> Take the CDN DP driver in rockchip for example (posted yesterday).
> There's a worker in the driver that is responsible for handling
> hpd/extcon events from the USB-C port. Ideally, this worker should
> just be a thin shell around a kms uevent that lets userspace know
> there's been a change. Unfortunately since we need to make re-training
> work (/me grumbles about productization) seamlessly, we need to add a
> bunch of goo into the worker to handle re-training. Since we need to
> handle re-training there and in modeset, we need to properly
> synchronize things. So we end up with a bunch of code that doesn't
> *really* need to be there.
>
> So is the correct path forward to add GOOD/BAD connection handling to
> Chrome/drm_hwc and rip re-training out of the various kernel drivers?
In i915 I think we might still want to do the "try to retrain with
the current link parameters" thing automagically. It's not difficult
to do after all, and should be faster than doing a full modeset.
>
> Sean
>
>
> > BR,
> > Jani.
> >
> >
> >
> >>
> >> Sean
> >>
> >>
> >>> Thanks for taking a look at this.
> >>>
> >>> Cheers, Daniel
> >>>
> >>>> > + * 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
> >>>
> >>> --
> >>> Daniel Vetter
> >>> Software Engineer, Intel Corporation
> >>> http://blog.ffwll.ch
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
