On 2020-04-15 at 13:01:21 +0300, Jani Nikula wrote: > On Tue, 07 Apr 2020, Jeevan B <jeevan.b@xxxxxxxxx> wrote: > > From: Oleg Vasilev <oleg.vasilev@xxxxxxxxx> > > > > Currently, downstream port type is only reported in debugfs. This > > information should be considered important since it reflects the actual > > physical connector type. Some userspace (e.g. window compositors) > > may want to show this info to a user. > > > > The 'subconnector' property is already utilized for DVI-I and TV-out for > > reporting connector subtype. > > > > The initial motivation for this feature came from i2c test [1]. > > It is supposed to be skipped on VGA connectors, but it cannot > > detect VGA over DP and fails instead. > > > > v2: > > - Ville: utilized drm_dp_is_branch() > > - Ville: implement DP 1.0 downstream type info > > - Replaced create_dp_properties with add_dp_subconnector_property > > - Added dp_set_subconnector_property helper > > > > v4: > > - Ville: add DP1.0 best assumption about subconnector > > - Ville: assume DVI is DVI-D > > - Ville: reuse Writeback enum value for Virtual subconnector > > - Renamed #defines: HDMI -> HDMIA, DP -> DisplayPort > > > > v5: rebase > > > > [1]: https://bugs.freedesktop.org/show_bug.cgi?id=104097 > > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > Signed-off-by: Jeevan B <jeevan.b@xxxxxxxxx> > > Signed-off-by: Oleg Vasilev <oleg.vasilev@xxxxxxxxx> > > Reviewed-by: Emil Velikov <emil.velikov@xxxxxxxxxxxxx> > > Link: https://patchwork.freedesktop.org/patch/msgid/20190829114854.1539-3-oleg.vasilev@xxxxxxxxx > > High level looks fine to me, please see some nitpicks inline. > > BR, > Jani. > > > > --- > > drivers/gpu/drm/drm_connector.c | 49 ++++++++++++++++++++++++-- > > drivers/gpu/drm/drm_dp_helper.c | 77 +++++++++++++++++++++++++++++++++++++++++ > > include/drm/drm_connector.h | 3 ++ > > include/drm/drm_dp_helper.h | 8 +++++ > > include/drm/drm_mode_config.h | 6 ++++ > > include/uapi/drm/drm_mode.h | 21 ++++++----- > > 6 files changed, 154 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > > index b1099e1..b6972d1 100644 > > --- a/drivers/gpu/drm/drm_connector.c > > +++ b/drivers/gpu/drm/drm_connector.c > > @@ -844,7 +844,7 @@ static const struct drm_prop_enum_list drm_dvi_i_select_enum_list[] = { > > DRM_ENUM_NAME_FN(drm_get_dvi_i_select_name, drm_dvi_i_select_enum_list) > > > > static const struct drm_prop_enum_list drm_dvi_i_subconnector_enum_list[] = { > > - { DRM_MODE_SUBCONNECTOR_Unknown, "Unknown" }, /* DVI-I and TV-out */ > > + { DRM_MODE_SUBCONNECTOR_Unknown, "Unknown" }, /* DVI-I, TV-out and DP */ > > { DRM_MODE_SUBCONNECTOR_DVID, "DVI-D" }, /* DVI-I */ > > { DRM_MODE_SUBCONNECTOR_DVIA, "DVI-A" }, /* DVI-I */ > > }; > > @@ -861,7 +861,7 @@ static const struct drm_prop_enum_list drm_tv_select_enum_list[] = { > > DRM_ENUM_NAME_FN(drm_get_tv_select_name, drm_tv_select_enum_list) > > > > static const struct drm_prop_enum_list drm_tv_subconnector_enum_list[] = { > > - { DRM_MODE_SUBCONNECTOR_Unknown, "Unknown" }, /* DVI-I and TV-out */ > > + { DRM_MODE_SUBCONNECTOR_Unknown, "Unknown" }, /* DVI-I, TV-out and DP */ > > { DRM_MODE_SUBCONNECTOR_Composite, "Composite" }, /* TV-out */ > > { DRM_MODE_SUBCONNECTOR_SVIDEO, "SVIDEO" }, /* TV-out */ > > { DRM_MODE_SUBCONNECTOR_Component, "Component" }, /* TV-out */ > > @@ -870,6 +870,19 @@ static const struct drm_prop_enum_list drm_tv_subconnector_enum_list[] = { > > DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name, > > drm_tv_subconnector_enum_list) > > > > +static const struct drm_prop_enum_list drm_dp_subconnector_enum_list[] = { > > + { DRM_MODE_SUBCONNECTOR_Unknown, "Unknown" }, /* DVI-I, TV-out and DP */ > > + { DRM_MODE_SUBCONNECTOR_VGA, "VGA" }, /* DP */ > > + { DRM_MODE_SUBCONNECTOR_DVID, "DVI-D" }, /* DP */ > > + { DRM_MODE_SUBCONNECTOR_HDMIA, "HDMI" }, /* DP */ > > + { DRM_MODE_SUBCONNECTOR_DisplayPort, "DP" }, /* DP */ > > + { DRM_MODE_SUBCONNECTOR_Wireless, "Wireless" }, /* DP */ > > + { DRM_MODE_SUBCONNECTOR_Native, "Native" }, /* DP */ > > +}; > > + > > +DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name, > > + drm_dp_subconnector_enum_list) > > + > > static const struct drm_prop_enum_list hdmi_colorspaces[] = { > > /* For Default case, driver will set the colorspace */ > > { DRM_MODE_COLORIMETRY_DEFAULT, "Default" }, > > @@ -1186,6 +1199,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = { > > * can also expose this property to external outputs, in which case they > > * must support "None", which should be the default (since external screens > > * have a built-in scaler). > > + * > > + * subconnector: > > + * This property is used by DVI-I, TVout and DisplayPort to indicate different > > + * connector subtypes. Enum values more or less match with those from main > > + * connector types. > > + * For DVI-I and TVout there is also a matching property "select subconnector" > > + * allowing to switch between signal types. > > + * DP subconnector corresponds to a downstream port. > > */ > > > > int drm_connector_create_standard_properties(struct drm_device *dev) > > @@ -1275,6 +1296,30 @@ int drm_mode_create_dvi_i_properties(struct drm_device *dev) > > EXPORT_SYMBOL(drm_mode_create_dvi_i_properties); > > > > /** > > + * drm_mode_add_dp_subconnector_property - create subconnector property for DP > > + * @connector: drm_connector to attach property > > + * > > + * Called by a driver when DP connector is created. > > + */ > > +void drm_mode_add_dp_subconnector_property(struct drm_connector *connector) > > +{ > > So far most functions dealing with properties are named: > > - drm_mode_create_* > - drm_connector_attach_* > > Sometimes the latter calling the former, sometimes creating inline. > > drm_mode_add_* that attaches a property on a connector is confusing. > > Perhaps call it drm_connector_attach_dp_subconnector_property(). > Sure, I will change this. > > + struct drm_mode_config *mode_config = &connector->dev->mode_config; > > + > > + if (!mode_config->dp_subconnector_property) > > + mode_config->dp_subconnector_property = > > + drm_property_create_enum(connector->dev, > > + DRM_MODE_PROP_IMMUTABLE, > > + "subconnector", > > + drm_dp_subconnector_enum_list, > > + ARRAY_SIZE(drm_dp_subconnector_enum_list)); > > + > > + drm_object_attach_property(&connector->base, > > + mode_config->dp_subconnector_property, > > + DRM_MODE_SUBCONNECTOR_Unknown); > > +} > > +EXPORT_SYMBOL(drm_mode_add_dp_subconnector_property); > > + > > +/** > > * DOC: HDMI connector properties > > * > > * content type (HDMI specific): > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > > index 8ba4531..5d5b50f 100644 > > --- a/drivers/gpu/drm/drm_dp_helper.c > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > @@ -594,6 +594,83 @@ void drm_dp_downstream_debug(struct seq_file *m, > > } > > EXPORT_SYMBOL(drm_dp_downstream_debug); > > > > +/** > > + * drm_dp_subconnector_type() - get DP branch device type > > + * @dpcd: DisplayPort configuration data > > + * @port_cap: port capabilities > > This does not help at all. What should this point at? > > > + * > > + */ > > +enum drm_mode_subconnector > > +drm_dp_subconnector_type(const u8 dpcd[DP_RECEIVER_CAP_SIZE], > > + const u8 port_cap[4]) > > +{ > > + int type; > > + if (!drm_dp_is_branch(dpcd)) > > + return DRM_MODE_SUBCONNECTOR_Native; > > + /* DP 1.0 approach */ > > + if (dpcd[DP_DPCD_REV] == DP_DPCD_REV_10) { > > + type = dpcd[DP_DOWNSTREAMPORT_PRESENT] & > > + DP_DWN_STRM_PORT_TYPE_MASK; > > + > > + switch (type) { > > + case DP_DWN_STRM_PORT_TYPE_TMDS: > > + /* Can be HDMI or DVI-D, DVI-D is a safer option */ > > + return DRM_MODE_SUBCONNECTOR_DVID; > > + case DP_DWN_STRM_PORT_TYPE_ANALOG: > > + /* Can be VGA or DVI-A, VGA is more popular */ > > + return DRM_MODE_SUBCONNECTOR_VGA; > > + case DP_DWN_STRM_PORT_TYPE_DP: > > + return DRM_MODE_SUBCONNECTOR_DisplayPort; > > + case DP_DWN_STRM_PORT_TYPE_OTHER: > > + default: > > + return DRM_MODE_SUBCONNECTOR_Unknown; > > + } > > + } > > + type = port_cap[0] & DP_DS_PORT_TYPE_MASK; > > + > > + switch (type) { > > + case DP_DS_PORT_TYPE_DP: > > + case DP_DS_PORT_TYPE_DP_DUALMODE: > > + return DRM_MODE_SUBCONNECTOR_DisplayPort; > > + case DP_DS_PORT_TYPE_VGA: > > + return DRM_MODE_SUBCONNECTOR_VGA; > > + case DP_DS_PORT_TYPE_DVI: > > + return DRM_MODE_SUBCONNECTOR_DVID; > > + case DP_DS_PORT_TYPE_HDMI: > > + return DRM_MODE_SUBCONNECTOR_HDMIA; > > + case DP_DS_PORT_TYPE_WIRELESS: > > + return DRM_MODE_SUBCONNECTOR_Wireless; > > + case DP_DS_PORT_TYPE_NON_EDID: > > + default: > > + return DRM_MODE_SUBCONNECTOR_Unknown; > > + } > > +} > > +EXPORT_SYMBOL(drm_dp_subconnector_type); > > + > > +/** > > + * drm_mode_set_dp_subconnector_property - set subconnector for DP connector > > + * @connector: dp connector to attach property > > + * @status: connector status which is about to be set > > + * @dpcd: DisplayPort configuration data > > + * @port_cap: port capabilities > > Unhelpful documentation. Sure port_cap is port capabilities, but what > should this point at? (I can tell looking at the code, but what's the > point of documentation if you have to look at the code?) > I will correct this. Oleg might have added this just like in other functions as below /** * drm_dp_downstream_debug() - debug DP branch devices * @m: pointer for debugfs file * @dpcd: DisplayPort configuration data * @port_cap: port capabilities * @aux: DisplayPort AUX channel * */ > > + * > > + * Called by a driver on every detect event. > > + */ > > +void drm_dp_set_subconnector_property(struct drm_connector *connector, > > + enum drm_connector_status status, > > + const u8 *dpcd, > > + const u8 port_cap[4]) > > Inconsistent parameters. > > > +{ > > + enum drm_mode_subconnector subconnector = DRM_MODE_SUBCONNECTOR_Unknown; > > + > > + if (status == connector_status_connected) > > + subconnector = drm_dp_subconnector_type(dpcd, port_cap); > > + drm_object_property_set_value(&connector->base, > > + connector->dev->mode_config.dp_subconnector_property, > > + subconnector); > > +} > > +EXPORT_SYMBOL(drm_dp_set_subconnector_property); > > + > > /* > > * I2C-over-AUX implementation > > */ > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > > index fd543d1..82797c8 100644 > > --- a/include/drm/drm_connector.h > > +++ b/include/drm/drm_connector.h > > @@ -1556,10 +1556,13 @@ const char *drm_get_dvi_i_subconnector_name(int val); > > const char *drm_get_dvi_i_select_name(int val); > > const char *drm_get_tv_subconnector_name(int val); > > const char *drm_get_tv_select_name(int val); > > +const char *drm_get_dp_subconnector_name(int val); > > const char *drm_get_content_protection_name(int val); > > const char *drm_get_hdcp_content_type_name(int val); > > > > int drm_mode_create_dvi_i_properties(struct drm_device *dev); > > +void drm_mode_add_dp_subconnector_property(struct drm_connector *connector); > > + > > int drm_mode_create_tv_margin_properties(struct drm_device *dev); > > int drm_mode_create_tv_properties(struct drm_device *dev, > > unsigned int num_modes, > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > > index 305533d..6408b31 100644 > > --- a/include/drm/drm_dp_helper.h > > +++ b/include/drm/drm_dp_helper.h > > @@ -26,6 +26,7 @@ > > #include <linux/delay.h> > > #include <linux/i2c.h> > > #include <linux/types.h> > > +#include <drm/drm_connector.h> > > Forward declarations instead of includes if possible, please. > I will change it. > > > > /* > > * Unless otherwise noted, all values are from the DP 1.1a spec. Note that > > @@ -1599,6 +1600,13 @@ int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE], > > int drm_dp_downstream_id(struct drm_dp_aux *aux, char id[6]); > > void drm_dp_downstream_debug(struct seq_file *m, const u8 dpcd[DP_RECEIVER_CAP_SIZE], > > const u8 port_cap[4], struct drm_dp_aux *aux); > > +enum drm_mode_subconnector > > +drm_dp_subconnector_type(const u8 dpcd[DP_RECEIVER_CAP_SIZE], > > + const u8 port_cap[4]); > > +void drm_dp_set_subconnector_property(struct drm_connector *connector, > > + enum drm_connector_status status, > > + const u8 *dpcd, > > + const u8 port_cap[4]); > > Inconsistent parameters. > > > > > void drm_dp_remote_aux_init(struct drm_dp_aux *aux); > > void drm_dp_aux_init(struct drm_dp_aux *aux); > > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > > index 6c3ef49..085fb00 100644 > > --- a/include/drm/drm_mode_config.h > > +++ b/include/drm/drm_mode_config.h > > @@ -681,6 +681,12 @@ struct drm_mode_config { > > struct drm_property *dvi_i_select_subconnector_property; > > > > /** > > + * @dp_subconnector_property: Optional DP property to differentiate > > + * between different DP downstream port types. > > + */ > > + struct drm_property *dp_subconnector_property; > > + > > + /** > > * @tv_subconnector_property: Optional TV property to differentiate > > * between different TV connector types. > > */ > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > > index 735c8cf..3358c6b 100644 > > --- a/include/uapi/drm/drm_mode.h > > +++ b/include/uapi/drm/drm_mode.h > > @@ -332,14 +332,19 @@ struct drm_mode_get_encoder { > > /* This is for connectors with multiple signal types. */ > > /* Try to match DRM_MODE_CONNECTOR_X as closely as possible. */ > > enum drm_mode_subconnector { > > - DRM_MODE_SUBCONNECTOR_Automatic = 0, > > - DRM_MODE_SUBCONNECTOR_Unknown = 0, > > - DRM_MODE_SUBCONNECTOR_DVID = 3, > > - DRM_MODE_SUBCONNECTOR_DVIA = 4, > > - DRM_MODE_SUBCONNECTOR_Composite = 5, > > - DRM_MODE_SUBCONNECTOR_SVIDEO = 6, > > - DRM_MODE_SUBCONNECTOR_Component = 8, > > - DRM_MODE_SUBCONNECTOR_SCART = 9, > > + DRM_MODE_SUBCONNECTOR_Automatic = 0, /* DVI-I, TV */ > > + DRM_MODE_SUBCONNECTOR_Unknown = 0, /* DVI-I, TV, DP */ > > + DRM_MODE_SUBCONNECTOR_VGA = 1, /* DP */ > > + DRM_MODE_SUBCONNECTOR_DVID = 3, /* DVI-I DP */ > > + DRM_MODE_SUBCONNECTOR_DVIA = 4, /* DVI-I */ > > + DRM_MODE_SUBCONNECTOR_Composite = 5, /* TV */ > > + DRM_MODE_SUBCONNECTOR_SVIDEO = 6, /* TV */ > > + DRM_MODE_SUBCONNECTOR_Component = 8, /* TV */ > > + DRM_MODE_SUBCONNECTOR_SCART = 9, /* TV */ > > + DRM_MODE_SUBCONNECTOR_DisplayPort = 10, /* DP */ > > + DRM_MODE_SUBCONNECTOR_HDMIA = 11, /* DP */ > > + DRM_MODE_SUBCONNECTOR_Native = 15, /* DP */ > > + DRM_MODE_SUBCONNECTOR_Wireless = 18, /* DP */ > > }; > > > > #define DRM_MODE_CONNECTOR_Unknown 0 > > -- > Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
