Re: [PATCH] drm/i915/ehl: Allow combo PHY A to drive a third external display

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

On Fri, Jun 14, 2019 at 11:07:34AM -0700, Matt Roper wrote:
> EHL has a mux on combo PHY A that allows it to be driven either by an
> internal display (using DDI-A or DSI DPHY) or by an external display
> (using DDI-D).  This is a motherboard design decision that can not be
> changed on the fly.  Let's use the VBT child device settings to try to
> figure out our board's configuration and program the mux accordingly.
> 
> Cc: Clint Taylor <Clinton.A.Taylor@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h        |  1 +
>  drivers/gpu/drm/i915/intel_bios.c      | 24 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_bios.h      |  1 +
>  drivers/gpu/drm/i915/intel_combo_phy.c | 13 +++++++++++++
>  4 files changed, 39 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 368ee717580c..0ae0d85304b5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -11121,6 +11121,7 @@ enum skl_power_gate {
>  #define _ICL_PHY_MISC_B		0x64C04
>  #define ICL_PHY_MISC(port)	_MMIO_PORT(port, _ICL_PHY_MISC_A, \
>  						 _ICL_PHY_MISC_B)
> +#define  ICL_PHY_MISC_MUX_DDID			(1 << 28)
>  #define  ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN	(1 << 23)
>  
>  /* Icelake Display Stream Compression Registers */
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index d3d473934ea4..ac861852e843 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1667,6 +1667,9 @@ parse_general_definitions(struct drm_i915_private *dev_priv,
>  		if (!child->device_type)
>  			continue;
>  
> +		DRM_DEBUG_KMS("Found VBT child device with type 0x%x\n",
> +			      child->device_type);
> +
>  		/*
>  		 * Copy as much as we know (sizeof) and is available
>  		 * (child_dev_size) of the child device. Accessing the data must
> @@ -2259,3 +2262,24 @@ enum aux_ch intel_bios_port_aux_ch(struct drm_i915_private *dev_priv,
>  
>  	return aux_ch;
>  }
> +
> +/**
> + * intel_bios_has_internal_display - are any child devices marked 'internal?'
> + * @dev_priv:	i915 device instance
> + *
> + * Returns true if any of the child devices have a device type containing
> + * the %DEVICE_TYPE_INTERNAL_CONNECTOR bit.
> + */
> +bool intel_bios_has_internal_display(struct drm_i915_private *dev_priv)
> +{
> +	int i;
> +
> +	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
> +		struct child_device_config *child = &dev_priv->vbt.child_dev[i];
> +
> +		if (child->device_type & DEVICE_TYPE_INTERNAL_CONNECTOR)
> +			return true;
> +	}
> +
> +	return false;
> +}

This feels super fragile. No hw strap for this?

Is the VBT DVO port referring to DDI or PHY?

If it's referring to DDI then I think we could check
for child device being present on DDI D and no child
device on DDI A/DSI. Not sure which way we should go
if there's a conflict.

But if it's referring to the PHY then we're hosed because
we can't tell which DDI is driving DVO port A aka. PHY A.
I guess in that case we should check for an internal/DSI vs.
external connector on DVO port A?

Not sure how many places we have in the code which assumes
1:1 mapping between DDI:PHY. I think we really need to
introduce a new namespace for the PHY so that we aren't
super confused all the time which thing we're talking about.

> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index 4e42cfaf61a7..df822a1efe55 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -240,5 +240,6 @@ bool intel_bios_is_port_hpd_inverted(const struct drm_i915_private *i915,
>  bool intel_bios_is_lspcon_present(const struct drm_i915_private *i915,
>  				  enum port port);
>  enum aux_ch intel_bios_port_aux_ch(struct drm_i915_private *dev_priv, enum port port);
> +bool intel_bios_has_internal_display(struct drm_i915_private *dev_priv);
>  
>  #endif /* _INTEL_BIOS_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_combo_phy.c b/drivers/gpu/drm/i915/intel_combo_phy.c
> index 841708da5a56..3bf3e41c5296 100644
> --- a/drivers/gpu/drm/i915/intel_combo_phy.c
> +++ b/drivers/gpu/drm/i915/intel_combo_phy.c
> @@ -273,7 +273,20 @@ static void icl_combo_phys_init(struct drm_i915_private *dev_priv)
>  			continue;
>  		}
>  
> +		/*
> +		 * EHL's combo PHY A can be hooked up to either an external
> +		 * display (via DDI-D) or an internal display (via DDI-A or
> +		 * the DSI DPHY).  This is a motherboard design decision that
> +		 * can't be changed on the fly, so initialize the PHY's mux
> +		 * based on whether our VBT indicates the presence of any
> +		 * "internal" child devices.
> +		 */
>  		val = I915_READ(ICL_PHY_MISC(port));
> +		if (!IS_ICELAKE(dev_priv) && port == PORT_A &&
> +		    !intel_bios_has_internal_display(dev_priv))
> +			val |= ICL_PHY_MISC_MUX_DDID;
> +		else
> +			val &= ~ICL_PHY_MISC_MUX_DDID;
>  		val &= ~ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN;
>  		I915_WRITE(ICL_PHY_MISC(port), val);
>  
> -- 
> 2.14.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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