diff mbox series

[v3,03/19] drm/i915: Give DDI encoders even better names

Message ID 20201028213323.5423-4-ville.syrjala@linux.intel.com
State New, archived
Headers show
Series drm/i915: Futher cleanup around hpd pins and port identfiers | expand

Commit Message

Ville Syrjälä Oct. 28, 2020, 9:33 p.m. UTC
From: Ville Syrjälä <ville.syrjala@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@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 27 ++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

Comments

Jani Nikula Nov. 17, 2020, 2:33 p.m. UTC | #1
On Wed, 28 Oct 2020, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@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@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@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.

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;
Ville Syrjälä Nov. 17, 2020, 3:23 p.m. UTC | #2
On Tue, Nov 17, 2020 at 04:33:24PM +0200, Jani Nikula wrote:
> On Wed, 28 Oct 2020, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@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@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@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
diff mbox series

Patch

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');
+	} 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;