[PATCH RFC] drm: Add a new connector atomic property for link status
Jani Nikula
jani.nikula at linux.intel.com
Wed Nov 23 15:52:16 UTC 2016
On Wed, 23 Nov 2016, Sean Paul <seanpaul at chromium.org> wrote:
> On Wed, Nov 23, 2016 at 10:15 AM, Jani Nikula
> <jani.nikula at linux.intel.com> wrote:
>> On Wed, 23 Nov 2016, Sean Paul <seanpaul at chromium.org> wrote:
>>> On Wed, Nov 23, 2016 at 3:09 AM, Daniel Vetter <daniel at ffwll.ch> 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 at intel.com> 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 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;
>>>>
>>>> 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 :)
And just because I want to, doesn't mean I can! ;)
> 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
I think so.
> and rip re-training out of the various kernel drivers?
IMO if it works, don't change it, at least not in a rush. It also
depends on the hardware and the driver; the amount of code required may
be vastly different for various drivers.
Personally, I think in the long run letting userspace help here leads to
a simpler design and more efficient happy day scenario handling, and
userspace knows what's going on.
BR,
Jani.
>
> 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 at lists.freedesktop.org
>>>>> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>
>>>> --
>>>> Daniel Vetter
>>>> Software Engineer, Intel Corporation
>>>> http://blog.ffwll.ch
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center
--
Jani Nikula, Intel Open Source Technology Center
More information about the dri-devel
mailing list