On Tue, Nov 29, 2016 at 02:43:55PM -0500, Sean Paul wrote:
> On Tue, Nov 29, 2016 at 9:45 AM, Sean Paul <seanpaul@xxxxxxxxxxxx> wrote:
> > On Mon, Nov 28, 2016 at 7:59 PM, Manasi Navare
> > <manasi.d.navare@xxxxxxxxx> wrote:
> >> On Wed, Nov 23, 2016 at 09:09:28AM +0100, Daniel Vetter 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;
> >>> 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().
>
> While I'm nitpicking documentation, I think this one needs work, too.
> I'd prefer you just describe what the property conveys here instead of
> getting into driver implementation specifics. So something like:
>
> * link-status
> * Connector link-status property to indicate the status of link. The
> * default value is "GOOD". If something fails during, or after modeset,
> * the driver may set this to "BAD" and issue a hotplug uevent. Drivers
> * should update this value using
> * drm_mode_connector_set_link_status_property()
>
> Sean
>
Thanks a lot for your feedback.
I will incorporate these changes here and the ones you pointed out
in the previous email and send a new revision.
Regards
Manasi
> >>> > > * 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?
> >>> >
> >>
> >> Sean, do you have any recommendations on the text that can go in the kernel doc for
> >> this property to clarify that it depends on the kernel implementation whether we want
> >> to use this link-status property to do the retraining or not. Leaving it BAD and not doing
> >> anything will be fine too.
> >>
> >
> > I'd prefer something like:
> >
> > * In usual working scenario, this link status property will always be set to
> > * "GOOD". If something fails during or after a mode set, the driver may
> > * set this 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 can not 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 difference being that we don't need to mention modes since that
> > will be handled through the normal avenue and we don't necessarily
> > want to remove any modes from the list.
> >
> > Sean
> >
> >
> >> Manasi
> >>> > > + * 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?
> >>>
> >>> 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
