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

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Ville Syrjala <ville.syrjala@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Subject: Re: [Intel-gfx] [PATCH v3 03/19] drm/i915: Give DDI encoders even better names
Date: Tue, 17 Nov 2020 16:33:24 +0200	[thread overview]
Message-ID: <87wnykoxxn.fsf@intel.com> (raw)
In-Reply-To: <20201028213323.5423-4-ville.syrjala@linux.intel.com>

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;

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx