Re: [PATCH 1/3] drm/i915/bios: iterate over child devices to initialize ddi_port_info

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Mar 06, 2019 at 05:34:14PM +0200, Jani Nikula wrote:
> Iterate over child devices instead of ports in parse_ddi_ports() to
> initialize dri_port_info. We'll eventually need to decide some stuff
> based on the child device order, which may be different from the port
> order.
> 
> As a bonus, this allows better abstractions for e.g. dvo port mapping.
> 
> There's a subtle change in the DDC pin and AUX channel sanitization as
> we change the order. Otherwise, this should not change behaviour.

I have a feeling we had some odd cases with the port A vs. port E, but I
think those were probably due to the non-VBT defaults for port A
smashing into the VBT provided values of port E.

So yeah, I think (or at least hope) that this is fine.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |   1 +
>  drivers/gpu/drm/i915/intel_bios.c | 104 +++++++++++++++++-------------
>  2 files changed, 59 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ff039750069d..eeeb0d9cfdcd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -946,6 +946,7 @@ struct ddi_vbt_port_info {
>  #define HDMI_LEVEL_SHIFT_UNKNOWN	0xff
>  	u8 hdmi_level_shift;
>  
> +	u8 present:1;
>  	u8 supports_dvi:1;
>  	u8 supports_hdmi:1;
>  	u8 supports_dp:1;
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index b508d8a735e0..fc27ffe81c14 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1222,10 +1222,11 @@ static void sanitize_ddc_pin(struct drm_i915_private *dev_priv,
>  	if (!info->alternate_ddc_pin)
>  		return;
>  
> -	for_each_port_masked(p, (1 << port) - 1) {
> +	for (p = PORT_A; p < I915_MAX_PORTS; p++) {
>  		struct ddi_vbt_port_info *i = &dev_priv->vbt.ddi_port_info[p];
>  
> -		if (info->alternate_ddc_pin != i->alternate_ddc_pin)
> +		if (p == port || !i->present ||
> +		    info->alternate_ddc_pin != i->alternate_ddc_pin)
>  			continue;
>  
>  		DRM_DEBUG_KMS("port %c trying to use the same DDC pin (0x%x) as port %c, "
> @@ -1239,8 +1240,8 @@ static void sanitize_ddc_pin(struct drm_i915_private *dev_priv,
>  		 * port. Otherwise they share the same ddc bin and
>  		 * system couldn't communicate with them separately.
>  		 *
> -		 * Due to parsing the ports in alphabetical order,
> -		 * a higher port will always clobber a lower one.
> +		 * Due to parsing the ports in child device order,
> +		 * a later device will always clobber an earlier one.
>  		 */
>  		i->supports_dvi = false;
>  		i->supports_hdmi = false;
> @@ -1258,10 +1259,11 @@ static void sanitize_aux_ch(struct drm_i915_private *dev_priv,
>  	if (!info->alternate_aux_channel)
>  		return;
>  
> -	for_each_port_masked(p, (1 << port) - 1) {
> +	for (p = PORT_A; p < I915_MAX_PORTS; p++) {
>  		struct ddi_vbt_port_info *i = &dev_priv->vbt.ddi_port_info[p];
>  
> -		if (info->alternate_aux_channel != i->alternate_aux_channel)
> +		if (p == port || !i->present ||
> +		    info->alternate_aux_channel != i->alternate_aux_channel)
>  			continue;
>  
>  		DRM_DEBUG_KMS("port %c trying to use the same AUX CH (0x%x) as port %c, "
> @@ -1275,8 +1277,8 @@ static void sanitize_aux_ch(struct drm_i915_private *dev_priv,
>  		 * port. Otherwise they share the same aux channel
>  		 * and system couldn't communicate with them separately.
>  		 *
> -		 * Due to parsing the ports in alphabetical order,
> -		 * a higher port will always clobber a lower one.
> +		 * Due to parsing the ports in child device order,
> +		 * a later device will always clobber an earlier one.
>  		 */
>  		i->supports_dp = false;
>  		i->alternate_aux_channel = 0;
> @@ -1324,49 +1326,58 @@ static u8 map_ddc_pin(struct drm_i915_private *dev_priv, u8 vbt_pin)
>  	return 0;
>  }
>  
> -static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
> -			   u8 bdb_version)
> +static enum port dvo_port_to_port(u8 dvo_port)
>  {
> -	struct child_device_config *it, *child = NULL;
> -	struct ddi_vbt_port_info *info = &dev_priv->vbt.ddi_port_info[port];
> -	int i, j;
> -	bool is_dvi, is_hdmi, is_dp, is_edp, is_crt;
> -	/* Each DDI port can have more than one value on the "DVO Port" field,
> +	/*
> +	 * Each DDI port can have more than one value on the "DVO Port" field,
>  	 * so look for all the possible values for each port.
>  	 */
> -	int dvo_ports[][3] = {
> -		{DVO_PORT_HDMIA, DVO_PORT_DPA, -1},
> -		{DVO_PORT_HDMIB, DVO_PORT_DPB, -1},
> -		{DVO_PORT_HDMIC, DVO_PORT_DPC, -1},
> -		{DVO_PORT_HDMID, DVO_PORT_DPD, -1},
> -		{DVO_PORT_CRT, DVO_PORT_HDMIE, DVO_PORT_DPE},
> -		{DVO_PORT_HDMIF, DVO_PORT_DPF, -1},
> +	static const int dvo_ports[][3] = {
> +		[PORT_A] = { DVO_PORT_HDMIA, DVO_PORT_DPA, -1},
> +		[PORT_B] = { DVO_PORT_HDMIB, DVO_PORT_DPB, -1},
> +		[PORT_C] = { DVO_PORT_HDMIC, DVO_PORT_DPC, -1},
> +		[PORT_D] = { DVO_PORT_HDMID, DVO_PORT_DPD, -1},
> +		[PORT_E] = { DVO_PORT_CRT, DVO_PORT_HDMIE, DVO_PORT_DPE},
> +		[PORT_F] = { DVO_PORT_HDMIF, DVO_PORT_DPF, -1},
>  	};
> +	enum port port;
> +	int i;
>  
> -	/*
> -	 * Find the first child device to reference the port, report if more
> -	 * than one found.
> -	 */
> -	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
> -		it = dev_priv->vbt.child_dev + i;
> -
> -		for (j = 0; j < 3; j++) {
> -			if (dvo_ports[port][j] == -1)
> +	for (port = PORT_A; port < ARRAY_SIZE(dvo_ports); port++) {
> +		for (i = 0; i < ARRAY_SIZE(dvo_ports[port]); i++) {
> +			if (dvo_ports[port][i] == -1)
>  				break;
>  
> -			if (it->dvo_port == dvo_ports[port][j]) {
> -				if (child) {
> -					DRM_DEBUG_KMS("More than one child device for port %c in VBT, using the first.\n",
> -						      port_name(port));
> -				} else {
> -					child = it;
> -				}
> -			}
> +			if (dvo_port == dvo_ports[port][i])
> +				return port;
>  		}
>  	}
> -	if (!child)
> +
> +	return PORT_NONE;
> +}
> +
> +static void parse_ddi_port(struct drm_i915_private *dev_priv,
> +			   const struct child_device_config *child,
> +			   u8 bdb_version)
> +{
> +	struct ddi_vbt_port_info *info;
> +	bool is_dvi, is_hdmi, is_dp, is_edp, is_crt;
> +	enum port port;
> +
> +	port = dvo_port_to_port(child->dvo_port);
> +	if (port == PORT_NONE)
>  		return;
>  
> +	info = &dev_priv->vbt.ddi_port_info[port];
> +
> +	if (info->present) {
> +		DRM_DEBUG_KMS("More than one child device for port %c in VBT, using the first.\n",
> +			      port_name(port));
> +		return;
> +	}
> +
> +	info->present = true;
> +
>  	is_dvi = child->device_type & DEVICE_TYPE_TMDS_DVI_SIGNALING;
>  	is_dp = child->device_type & DEVICE_TYPE_DISPLAYPORT_OUTPUT;
>  	is_crt = child->device_type & DEVICE_TYPE_ANALOG_OUTPUT;
> @@ -1498,19 +1509,20 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
>  
>  static void parse_ddi_ports(struct drm_i915_private *dev_priv, u8 bdb_version)
>  {
> -	enum port port;
> +	const struct child_device_config *child;
> +	int i;
>  
>  	if (!HAS_DDI(dev_priv) && !IS_CHERRYVIEW(dev_priv))
>  		return;
>  
> -	if (!dev_priv->vbt.child_dev_num)
> -		return;
> -
>  	if (bdb_version < 155)
>  		return;
>  
> -	for (port = PORT_A; port < I915_MAX_PORTS; port++)
> -		parse_ddi_port(dev_priv, port, bdb_version);
> +	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
> +		child = dev_priv->vbt.child_dev + i;
> +
> +		parse_ddi_port(dev_priv, child, bdb_version);
> +	}
>  }
>  
>  static void
> -- 
> 2.20.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx