On Fri, Jun 07, 2019 at 11:21:51PM +0300, Souza, Jose wrote:
> On Tue, 2019-06-04 at 17:58 +0300, Imre Deak wrote:
> > Unify the TypeC port notation in log messages, so that it matches the
> > spec. For instance the first ICL TypeC port will read as 'Port
> > C/TC#1'.
> >
> > Cc: José Roberto de Souza <jose.souza at intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_tc.c | 41 +++++++++++++++++++++++++++--
> > ----
> > 1 file changed, 34 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_tc.c
> > b/drivers/gpu/drm/i915/intel_tc.c
> > index a3057c44bec6..07488235b67a 100644
> > --- a/drivers/gpu/drm/i915/intel_tc.c
> > +++ b/drivers/gpu/drm/i915/intel_tc.c
> > @@ -6,6 +6,29 @@
> > #include "i915_drv.h"
> > #include "intel_tc.h"
> >
> > +static enum port intel_tc_port_to_port(struct drm_i915_private
> > *dev_priv,
> > + enum tc_port tc_port)
> > +{
> > + return tc_port + PORT_C;
> > +}
> > +
> > +static const char *tc_port_name(struct drm_i915_private *dev_priv,
> > + enum tc_port tc_port)
> > +{
> > + static char port_names[I915_MAX_TC_PORTS][8];
> > +
> > + if (WARN_ON(INTEL_GEN(dev_priv) < 11 ||
> > + (unsigned int)tc_port >= I915_MAX_TC_PORTS))
> > + tc_port = PORT_TC1;
>
> Why no WARN_ON on the tc_port >= I915_MAX_TC_PORTS?
Hm, do you mean a seaparate WARN_ON()?
>
> > +
> > + snprintf(&port_names[tc_port][0], sizeof(port_names[tc_port]),
> > + "%c/TC#%d",
> > + port_name(intel_tc_port_to_port(dev_priv, tc_port)),
> > + tc_port + 1);
>
> Maybe do it only once for each port?
>
> if (port_names[tc_port][0])
> return port_names[tc_port];
I thought why not keep it as simple as possible (not really performance
critical), but your version makes it clearer to the reader what the
logic is (static array), so can change it.
>
> snprintf(&port_names[tc_port], sizeof....
>
> Other the above:
>
> Reviewed-by: José Roberto de Souza <jose.souza at intel.com>
>
> > +
> > + return port_names[tc_port];
> > +}
> > +
> > static const char *tc_port_mode_name(enum tc_port_mode mode)
> > {
> > static const char * const names[] = {
> > @@ -85,7 +108,8 @@ static bool icl_tc_phy_connect(struct
> > intel_digital_port *dig_port)
> >
> > val = I915_READ(PORT_TX_DFLEXDPPMS);
> > if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) {
> > - DRM_DEBUG_KMS("DP PHY for TC port %d not ready\n",
> > tc_port);
> > + DRM_DEBUG_KMS("Port %s: PHY not ready\n",
> > + tc_port_name(dev_priv, tc_port));
> > WARN_ON(dig_port->tc_legacy_port);
> > return false;
> > }
> > @@ -106,7 +130,8 @@ static bool icl_tc_phy_connect(struct
> > intel_digital_port *dig_port)
> > */
> > if (dig_port->tc_mode == TC_PORT_DP_ALT &&
> > !(I915_READ(PORT_TX_DFLEXDPSP) &
> > TC_LIVE_STATE_TC(tc_port))) {
> > - DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n",
> > tc_port);
> > + DRM_DEBUG_KMS("Port %s: PHY sudden disconnect\n",
> > + tc_port_name(dev_priv, tc_port));
> > icl_tc_phy_disconnect(dig_port);
> > return false;
> > }
> > @@ -136,8 +161,8 @@ void icl_tc_phy_disconnect(struct
> > intel_digital_port *dig_port)
> > I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > }
> >
> > - DRM_DEBUG_KMS("Port %c TC type %s disconnected\n",
> > - port_name(dig_port->base.port),
> > + DRM_DEBUG_KMS("Port %s: mode %s disconnected\n",
> > + tc_port_name(dev_priv, tc_port),
> > tc_port_mode_name(dig_port->tc_mode));
> >
> > dig_port->tc_mode = TC_PORT_TBT_ALT;
> > @@ -162,7 +187,9 @@ static void icl_update_tc_port_type(struct
> > drm_i915_private *dev_priv,
> > return;
> >
> > if (old_mode != intel_dig_port->tc_mode)
> > - DRM_DEBUG_KMS("Port %c has TC type %s\n",
> > port_name(port),
> > + DRM_DEBUG_KMS("Port %s: port has mode %s\n",
> > + tc_port_name(dev_priv,
> > + intel_port_to_tc(dev_priv,
> > port)),
> > tc_port_mode_name(intel_dig_port-
> > >tc_mode));
> > }
> >
> > @@ -191,8 +218,8 @@ bool intel_tc_port_connected(struct
> > intel_digital_port *dig_port)
> > */
> > if (!dig_port->tc_legacy_port &&
> > I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) {
> > - DRM_ERROR("VBT incorrectly claims port %c is not TypeC
> > legacy\n",
> > - port_name(port));
> > + DRM_ERROR("Port %s: VBT incorrectly claims port is not
> > TypeC legacy\n",
> > + tc_port_name(dev_priv, tc_port));
> > dig_port->tc_legacy_port = true;
> > }
> > is_legacy = dig_port->tc_legacy_port;