Re: [PATCH RFC v3] drm: Add a new connector atomic property for link status

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Nov 24, 2016 at 08:51:35AM +0100, Daniel Vetter wrote:
> On Wed, Nov 23, 2016 at 11:28: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.
> > 
> > v3:
> > * Fixed a build error (Jani Saarinen)
> > v2:
> > * Removed connector->link_status (Daniel Vetter)
> > * Set connector->state->link_status in drm_mode_connector_set_link_status_property
> > (Daniel Vetter)
> > * Set the connector_changed flag to true if connector->state->link_status changed.
> > * Reset link_status to GOOD in update_output_state (Daniel Vetter)
> > * Never allow userspace to set link status from Good To Bad (Daniel Vetter)
> > 
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Sean Paul <seanpaul@xxxxxxxxxxxx>
> 
> You lost all the acked-by from AMD about the link-status property. We need
> those.
>

I removed the ACKS sinec this was just a RFC patch, I will add them back
when I submit the actual patch in the patch series.

 
> > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> 
> Yeah I think this should work, but obviously testing has the final say.
> Some nitpicks below, then it's r-b: me. But I think we also need to polish
> the kernel-doc a bit more to address Sean Paul's questions.
> -Daniel
>

Yes, I still need to change the kernel doc as per Sean's comments but wanted
ACK on this new atomic design for the property so that I could update
the kernel doc.

 
> > ---
> >  drivers/gpu/drm/drm_atomic.c        | 10 ++++++
> >  drivers/gpu/drm/drm_atomic_helper.c |  8 +++++
> >  drivers/gpu/drm/drm_connector.c     | 61 ++++++++++++++++++++++++++++++++++++-
> >  include/drm/drm_connector.h         | 18 ++++++++++-
> >  include/drm/drm_mode_config.h       |  5 +++
> >  include/uapi/drm/drm_mode.h         |  4 +++
> >  6 files changed, 104 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 89737e4..990f013 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1087,6 +1087,14 @@ 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) {
> > +		/* Never downgrade from GOOD to BAD on userspace's request here,
> > +		 * only hw issues can do that.
> > +		 */
> > +		if (state->link_status == DRM_LINK_STATUS_GOOD)
> > +			return 0;
> > +		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 +1143,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_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 494680c..962ed66 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -519,6 +519,13 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
> >  					       connector_state);
> >  		if (ret)
> >  			return ret;
> > +		if (connector->state->crtc) {
> > +			crtc_state = drm_atomic_get_existing_crtc_state(state,
> > +									connector->state->crtc);
> > +			if (connector->state->link_status !=
> > +			    connector_state->link_status)
> > +				crtc_state->connectors_changed = true;
> > +		}
> 
> Why hide this here in handle_conflicting_encoders? This is a bit
> confusing. I'd move it into the connector loop in
> drm_atomic_helper_check_modeset().
> >  	}
> >  

If you pull this patch, you will see that this is added in  the connector
loop inside drm_atomic_helper_check_modeset(), donno why the diff shows as if
its inside handle_conflicting_encoders.

Manasi

> >  	/*
> > @@ -2258,6 +2265,7 @@ static int update_output_state(struct drm_atomic_state *state,
> >  								NULL);
> >  			if (ret)
> >  				return ret;
> > +			conn_state->link_status = DRM_LINK_STATUS_GOOD;
> >  		}
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 5a45262..cd53c39 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
> > @@ -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().
> 
> Please leave the empty line here after the definition list, for better
> layout. Also I thought there were some comments from Sean Paul that the
> kernel-doc was confusing, have you worked together with him to address
> this?
>

Now that link-status is an atomic property, shouldnt it be added after CRTC_ID in the
standardized atomic prop section?

 
> >   * 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,41 @@ 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;
> > +
> > +	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > +	connector->state->link_status = link_status;
> > +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > +
> > +}
> > +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..ba22ed1 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -90,6 +90,17 @@ enum subpixel_order {
> >  };
> >  
> >  /**
> > + * enum drm_link_status - connector's link_status property value
> > + *
> > + * This enum is used as the connector's link status property value.
> > + * It is set to the values defined in uapi.
> > + */
> > +enum drm_link_status {
> > +	DRM_LINK_STATUS_GOOD = DRM_MODE_LINK_STATUS_GOOD,
> > +	DRM_LINK_STATUS_BAD = DRM_MODE_LINK_STATUS_BAD,
> > +};
> > +
> > +/**
> >   * struct drm_display_info - runtime data about the connected sink
> >   *
> >   * Describes a given display (e.g. CRT or flat panel) and its limitations. For
> > @@ -213,6 +224,9 @@ struct drm_connector_state {
> >  
> >  	struct drm_encoder *best_encoder;
> >  
> > +	/* Connector Link status */
> > +	enum drm_link_status link_status;
> 
> make htmldocs should complain about the missing kernel-doc here. Just
> convert your inline comment into a proper kernel-doc:
> 
> 	/**
> 	 * @link_status: Connector link status
> 	 */
> 
> Also in general a comment that literally says the same as the code isn't
> useful. Comments should explain why the code does something, not what
> exactly it does - that should be self-evident.
> > +
> >  	struct drm_atomic_state *state;
> >  };
> >  
> > @@ -695,6 +709,7 @@ 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;
> > +
> 
> Spurious whitespace change, pls check for these before submitting. I think
> this one here is new.
> 
> >  };

Yes removed that.

> >  
> >  #define obj_to_connector(x) container_of(x, struct drm_connector, base)
> > @@ -767,12 +782,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);
> > -
> 
> Same.
> 
> >  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
> 
> -- 
> 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




[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