Re: [Intel-gfx] [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 Wed, Nov 23, 2016 at 10:43 AM, Ville Syrjälä
<ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> 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.
>

I'm not sure I buy the "It's not difficult to do" argument. We've
squashed a ton of asynchronous training bugs over the past few years
in various drivers. It's just one more thing that needs to be
synchronized with hotplug/modeset/suspend/disable, and Synchronization
Is Hard for a lot of people.

So while it may be faster, I'd happily take the performance hit to unify paths.

Sean


>>
>> 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
_______________________________________________
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