Re: [Intel-gfx] [PATCH v2 6/7] drm/i915: Init DDI outputs based on port_mask on skl+

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org,
	Lucas De Marchi <lucas.demarchi@intel.com>
Subject: Re: [Intel-gfx] [PATCH v2 6/7] drm/i915: Init DDI outputs based on port_mask on skl+
Date: Thu, 8 Jun 2023 22:48:38 +0300	[thread overview]
Message-ID: <ZIIwlpYuGQAYQNYK@intel.com> (raw)
In-Reply-To: <87edmudiz5.fsf@intel.com>

On Fri, Jun 02, 2023 at 05:41:50PM +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>
> >
> > Instead of listing every platform's possible DDI outputs
> > in intel_setup_outputs() just loop over the new port_mask
> > to achieve the same thing.
> 
> For posterity, I think I rejected a patch from Lucas generalizing the
> initialization in the past. I think that used the VBT child device list
> directly, and I wanted to preserve a clear way to check what the
> supported ports for a platform were. I think having the ports in runtime
> info now covers that concern. And with this, I'm open to using the child
> device list as it can now be cross-checked against the runtime info.
> 
> >
> > HSW/BDW were left as is since they still look at the straps
> > as well.
> >
> > DSI is still a mess. For now just check for the relevant
> > platforms explicitly.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 80 ++++----------------
> >  1 file changed, 13 insertions(+), 67 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index d3fc498c82c1..12f2e3897595 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -7387,73 +7387,19 @@ void intel_setup_outputs(struct drm_i915_private *dev_priv)
> >  	if (!HAS_DISPLAY(dev_priv))
> >  		return;
> >  
> > -	if (IS_METEORLAKE(dev_priv)) {
> > -		intel_ddi_init(dev_priv, PORT_A);
> > -		intel_ddi_init(dev_priv, PORT_B);
> > -		intel_ddi_init(dev_priv, PORT_TC1);
> > -		intel_ddi_init(dev_priv, PORT_TC2);
> > -		intel_ddi_init(dev_priv, PORT_TC3);
> > -		intel_ddi_init(dev_priv, PORT_TC4);
> > -	} else if (IS_DG2(dev_priv)) {
> > -		intel_ddi_init(dev_priv, PORT_A);
> > -		intel_ddi_init(dev_priv, PORT_B);
> > -		intel_ddi_init(dev_priv, PORT_C);
> > -		intel_ddi_init(dev_priv, PORT_D_XELPD);
> > -		intel_ddi_init(dev_priv, PORT_TC1);
> > -	} else if (IS_ALDERLAKE_P(dev_priv)) {
> > -		intel_ddi_init(dev_priv, PORT_A);
> > -		intel_ddi_init(dev_priv, PORT_B);
> > -		intel_ddi_init(dev_priv, PORT_TC1);
> > -		intel_ddi_init(dev_priv, PORT_TC2);
> > -		intel_ddi_init(dev_priv, PORT_TC3);
> > -		intel_ddi_init(dev_priv, PORT_TC4);
> > -		icl_dsi_init(dev_priv);
> > -	} else if (IS_ALDERLAKE_S(dev_priv)) {
> > -		intel_ddi_init(dev_priv, PORT_A);
> > -		intel_ddi_init(dev_priv, PORT_TC1);
> > -		intel_ddi_init(dev_priv, PORT_TC2);
> > -		intel_ddi_init(dev_priv, PORT_TC3);
> > -		intel_ddi_init(dev_priv, PORT_TC4);
> > -	} else if (IS_DG1(dev_priv) || IS_ROCKETLAKE(dev_priv)) {
> > -		intel_ddi_init(dev_priv, PORT_A);
> > -		intel_ddi_init(dev_priv, PORT_B);
> > -		intel_ddi_init(dev_priv, PORT_TC1);
> > -		intel_ddi_init(dev_priv, PORT_TC2);
> > -	} else if (DISPLAY_VER(dev_priv) >= 12) {
> > -		intel_ddi_init(dev_priv, PORT_A);
> > -		intel_ddi_init(dev_priv, PORT_B);
> > -		intel_ddi_init(dev_priv, PORT_TC1);
> > -		intel_ddi_init(dev_priv, PORT_TC2);
> > -		intel_ddi_init(dev_priv, PORT_TC3);
> > -		intel_ddi_init(dev_priv, PORT_TC4);
> > -		intel_ddi_init(dev_priv, PORT_TC5);
> > -		intel_ddi_init(dev_priv, PORT_TC6);
> > -		icl_dsi_init(dev_priv);
> > -	} else if (IS_JSL_EHL(dev_priv)) {
> > -		intel_ddi_init(dev_priv, PORT_A);
> > -		intel_ddi_init(dev_priv, PORT_B);
> > -		intel_ddi_init(dev_priv, PORT_C);
> > -		intel_ddi_init(dev_priv, PORT_D);
> > -		icl_dsi_init(dev_priv);
> > -	} else if (DISPLAY_VER(dev_priv) == 11) {
> > -		intel_ddi_init(dev_priv, PORT_A);
> > -		intel_ddi_init(dev_priv, PORT_B);
> > -		intel_ddi_init(dev_priv, PORT_C);
> > -		intel_ddi_init(dev_priv, PORT_D);
> > -		intel_ddi_init(dev_priv, PORT_E);
> > -		intel_ddi_init(dev_priv, PORT_F);
> > -		icl_dsi_init(dev_priv);
> > -	} else if (IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) {
> > -		intel_ddi_init(dev_priv, PORT_A);
> > -		intel_ddi_init(dev_priv, PORT_B);
> > -		intel_ddi_init(dev_priv, PORT_C);
> > -		vlv_dsi_init(dev_priv);
> > -	} else if (DISPLAY_VER(dev_priv) >= 9) {
> > -		intel_ddi_init(dev_priv, PORT_A);
> > -		intel_ddi_init(dev_priv, PORT_B);
> > -		intel_ddi_init(dev_priv, PORT_C);
> > -		intel_ddi_init(dev_priv, PORT_D);
> > -		intel_ddi_init(dev_priv, PORT_E);
> > +	if (DISPLAY_VER(dev_priv) >= 9) {
> > +		enum port port;
> > +
> > +		for_each_port_masked(port, DISPLAY_RUNTIME_INFO(dev_priv)->port_mask)
> > +			intel_ddi_init(dev_priv, port);
> > +
> > +		/* FIXME do something about DSI */
> > +		if (IS_ALDERLAKE_P(dev_priv) || IS_TIGERLAKE(dev_priv) ||
> > +		    DISPLAY_VER(dev_priv) == 11)
> > +			icl_dsi_init(dev_priv);
> 
> This reflects current code, but apparently commit e341c618acde
> ("drm/i915/adl_s: Initialize display for ADL-S") stopped initializing
> DSI for ADL-S. It does support DSI.

Not according to bspec. The diagram does still show the
DSI transcoders being present but the PHY is missing.

> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> 
> > +
> > +		if (IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv))
> > +			vlv_dsi_init(dev_priv);
> >  	} else if (HAS_DDI(dev_priv)) {
> >  		u32 found;
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Ville Syrjälä
Intel