[Intel-gfx] [PATCH v3 03/19] drm/i915: Give DDI encoders even better names

[Ville Syrjälä]

On Tue, Nov 17, 2020 at 04:33:24PM +0200, Jani Nikula wrote:
> On Wed, 28 Oct 2020, Ville Syrjala <ville.syrjala at linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > Let's pimp the DDI encoder->name to reflect what the spec calls them.
> > Ie. on pre-tgl DDI A-F, on tgl+ DDI A-C or DDI TC1-6.
> >
> > Also since each encoder is really a combination of the DDI and the PHY
> > we include the PHY name as well.
> >
> > ICL is a bit special since it already has the two different types
> > of DDIs (combo or TC) but it still calls them just DDI A-F regarless
> > of the type. For that let's add an extra "(TC)" note to remind
> > is which type of DDI it really is.
> >
> > The code is darn ugly, but not sure there's much we can do about it.
> >
> > Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 27 ++++++++++++++++++++++--
> >  1 file changed, 25 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 24245157dcb9..19b16517a502 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -5174,8 +5174,31 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
> >  
> >  	encoder = &dig_port->base;
> >  
> > -	drm_encoder_init(&dev_priv->drm, &encoder->base, &intel_ddi_funcs,
> > -			 DRM_MODE_ENCODER_TMDS, "DDI %c", port_name(port));
> > +	if (INTEL_GEN(dev_priv) >= 12) {
> > +		enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
> > +
> > +		drm_encoder_init(&dev_priv->drm, &encoder->base, &intel_ddi_funcs,
> > +				 DRM_MODE_ENCODER_TMDS,
> > +				 "DDI %s%c/PHY %s%c",
> > +				 port >= PORT_TC1 ? "TC" : "",
> > +				 port >= PORT_TC1 ? port_name(port) : port - PORT_TC1 + '1',
> > +				 tc_port != TC_PORT_NONE ? "TC" : "",
> > +				 tc_port != TC_PORT_NONE ? phy_name(phy) : tc_port - TC_PORT_1 + '1');
> 
> Frankly, this is a really ugly way to define encoder names, and it's
> hard to decipher what's actually going on. Even after I see logs with
> obviously bogus names such as:
> 
> [ENCODER:235:DDI ./PHY 0]
> 
> I find it tedious to decipher what exactly is wrong here.
> 
> I guess the 2nd port >= PORT_TC1 check should be reversed, but it
> doesn't exactly give me confidence about the rest.

Doh. Yeah, that is definitely the case. The second tc_port check
seems equally crap. Maybe I just don't know how to use ?: anymore :/

I guess a few extra macros/functions could clean it up a bit. The
other option would be just the fully declarative approach that was
discussed before. But there's an annoying amount of runtime
detection going on with port init so not sure how much we can
declare up front.

> 
> BR,
> Jani.
> 
> 
> > +	} else if (INTEL_GEN(dev_priv) >= 11) {
> > +		enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
> > +
> > +		drm_encoder_init(&dev_priv->drm, &encoder->base, &intel_ddi_funcs,
> > +				 DRM_MODE_ENCODER_TMDS,
> > +				 "DDI %c%s/PHY %s%c",
> > +				 port_name(port),
> > +				 port >= PORT_C ? " (TC)" : "",
> > +				 tc_port != TC_PORT_NONE ? "TC" : "",
> > +				 tc_port != TC_PORT_NONE ? phy_name(phy) : tc_port - TC_PORT_1 + '1');
> > +	} else {
> > +		drm_encoder_init(&dev_priv->drm, &encoder->base, &intel_ddi_funcs,
> > +				 DRM_MODE_ENCODER_TMDS,
> > +				 "DDI %c/PHY %c", port_name(port),  phy_name(phy));
> > +	}
> >  
> >  	mutex_init(&dig_port->hdcp_mutex);
> >  	dig_port->num_hdcp_streams = 0;
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Ville Syrjälä
Intel