Re: [Intel-gfx] [PATCH v2 7/7] drm/i915: Convert HSW/BDW to use port_mask for DDI probe

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

On Fri, Jun 02, 2023 at 05:51:19PM +0300, Jani Nikula wrote:
> On Wed, 31 May 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Make HSW/BDW use port_mask for output probing as well.
> > To achieve that the strap checks are moved into
> > intel_ddi_init() itself. Or should we move them to the
> > runtime port_mask init instead? Maybe not since the hardware
> > is still there, just not connected to anything.
> >
> > v2: Account for DDI-E in strap detection
> >     Keep to the old CRT->DDI init order
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c     | 29 ++++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_display.c | 23 +++-------------
> >  2 files changed, 33 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 31001b9a29b0..d89a9b85a780 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -4657,6 +4657,29 @@ static void intel_ddi_tc_encoder_shutdown_complete(struct intel_encoder *encoder
> >  #define port_tc_name(port) ((port) - PORT_TC1 + '1')
> >  #define tc_port_name(tc_port) ((tc_port) - TC_PORT_1 + '1')
> >  
> > +static bool port_strap_detected(struct drm_i915_private *i915, enum port port)
> > +{
> > +	/* straps not used on skl+ */
> > +	if (DISPLAY_VER(i915) >= 9)
> > +		return true;
> > +
> > +	switch (port) {
> > +	case PORT_A:
> > +		return intel_de_read(i915, DDI_BUF_CTL(PORT_A)) & DDI_INIT_DISPLAY_DETECTED;
> > +	case PORT_B:
> > +		return intel_de_read(i915, SFUSE_STRAP) & SFUSE_STRAP_DDIB_DETECTED;
> > +	case PORT_C:
> > +		return intel_de_read(i915, SFUSE_STRAP) & SFUSE_STRAP_DDIC_DETECTED;
> > +	case PORT_D:
> > +		return intel_de_read(i915, SFUSE_STRAP) & SFUSE_STRAP_DDID_DETECTED;
> > +	case PORT_E:
> > +		return true; /* no strap for DDI-E */
> > +	default:
> > +		MISSING_CASE(port);
> > +		return false;
> > +	}
> > +}
> > +
> >  void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
> >  {
> >  	struct intel_digital_port *dig_port;
> > @@ -4665,6 +4688,12 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
> >  	bool init_hdmi, init_dp;
> >  	enum phy phy = intel_port_to_phy(dev_priv, port);
> >  
> > +	if (!port_strap_detected(dev_priv, port)) {
> > +		drm_dbg_kms(&dev_priv->drm,
> > +			    "Port %c strap not detected\n", port_name(port));
> > +		return;
> > +	}
> > +
> >  	if (!assert_port_valid(dev_priv, port))
> >  		return;
> 
> I might have put this check first before the straps, so we get the
> invalid port message for bogus ports instead of MISSING_CASE().

That would trip assert_port_valid() when we try to init DDI D on
ULT/ULX machines.

Sadly I also have a one HSW ULT machine here where the VBT
(arguably incorrectly) declares that DDI D is present. So
even when we switch to VBT based init we need to do the strap
check first. I guess one option would be to remove the
MISSING_CASE() and just return true for all DDI E+.

> 
> Either way,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 12f2e3897595..1ae4854b275e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -7387,9 +7387,12 @@ void intel_setup_outputs(struct drm_i915_private *dev_priv)
> >  	if (!HAS_DISPLAY(dev_priv))
> >  		return;
> >  
> > -	if (DISPLAY_VER(dev_priv) >= 9) {
> > +	if (HAS_DDI(dev_priv)) {
> >  		enum port port;
> >  
> > +		if (intel_ddi_crt_present(dev_priv))
> > +			intel_crt_init(dev_priv);
> > +
> >  		for_each_port_masked(port, DISPLAY_RUNTIME_INFO(dev_priv)->port_mask)
> >  			intel_ddi_init(dev_priv, port);
> >  
> > @@ -7400,24 +7403,6 @@ void intel_setup_outputs(struct drm_i915_private *dev_priv)
> >  
> >  		if (IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv))
> >  			vlv_dsi_init(dev_priv);
> > -	} else if (HAS_DDI(dev_priv)) {
> > -		u32 found;
> > -
> > -		if (intel_ddi_crt_present(dev_priv))
> > -			intel_crt_init(dev_priv);
> > -
> > -		/* Haswell uses DDI functions to detect digital outputs. */
> > -		found = intel_de_read(dev_priv, DDI_BUF_CTL(PORT_A)) & DDI_INIT_DISPLAY_DETECTED;
> > -		if (found)
> > -			intel_ddi_init(dev_priv, PORT_A);
> > -
> > -		found = intel_de_read(dev_priv, SFUSE_STRAP);
> > -		if (found & SFUSE_STRAP_DDIB_DETECTED)
> > -			intel_ddi_init(dev_priv, PORT_B);
> > -		if (found & SFUSE_STRAP_DDIC_DETECTED)
> > -			intel_ddi_init(dev_priv, PORT_C);
> > -		if (found & SFUSE_STRAP_DDID_DETECTED)
> > -			intel_ddi_init(dev_priv, PORT_D);
> >  	} else if (HAS_PCH_SPLIT(dev_priv)) {
> >  		int found;
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Ville Syrjälä
Intel