[Intel-gfx] [PATCH v4 1/5] drm/i915/gen11: Start distinguishing 'phy' from 'port'

[Lucas De Marchi]

On Thu, Jul 04, 2019 at 12:18:11PM +0300, Ville Syrjälä wrote:
>On Wed, Jul 03, 2019 at 04:37:32PM -0700, Matt Roper wrote:
>> Our past DDI-based Intel platforms have had a fixed DDI<->PHY mapping.
>> Because of this, both the bspec documentation and our i915 code has used
>> the term "port" when talking about either DDI's or PHY's; it was always
>> easy to tell what terms like "Port A" were referring to from the
>> context.
>>
>> Unfortunately this is starting to break down now that EHL allows PHY-A
>> to be driven by either DDI-A or DDI-D.  Is a setup with DDI-D driving
>> PHY-A considered "Port A" or "Port D?"  The answer depends on which
>> register we're working with, and even the bspec doesn't do a great job
>> of clarifying this.
>>
>> Let's try to be more explicit about whether we're talking about the DDI
>> or the PHY on gen11+ by using 'port' to refer to the DDI and creating a
>> new 'enum phy' namespace to refer to the PHY in use.
>>
>> This patch just adds the new PHY namespace, new phy-based versions of
>> intel_port_is_*(), and a helper to convert a port to a PHY.
>> Transitioning various areas of the code over to using the PHY namespace
>> will be done in subsequent patches to make review easier.  We'll remove
>> the intel_port_is_*() functions at the end of the series when we
>> transition all callers over to using the PHY-based versions.
>>
>> v2:
>>  - Convert a few more 'port' uses to 'phy.' (Sparse)
>>
>> v3:
>>  - Switch DDI_CLK_SEL() back to 'port.' (Jose)
>>  - Add a code comment clarifying why DPCLKA_CFGCR0_ICL needs to use PHY
>>    for its bit definitions, even though the register description is
>>    given in terms of DDI.
>>  - To avoid confusion, switch CNL's DPCLKA_CFGCR0 defines back to using
>>    port and create separate ICL+ definitions that work in terms of PHY.
>>
>> v4:
>>  - Rebase and resolve conflicts with Imre's TC series.
>>  - This patch now just adds the namespace and a few convenience
>>    functions; the important changes are now split out into separate
>>    patches to make review easier.
>>
>> Suggested-by: Ville Syrjala <ville.syrjala at linux.intel.com>
>> Cc: José Roberto de Souza <jose.souza at intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> Cc: Imre Deak <imre.deak at intel.com>
>> Cc: Jani Nikula <jani.nikula at intel.com>
>> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_display.c | 32 +++++++++++++++++++-
>>  drivers/gpu/drm/i915/display/intel_display.h | 16 ++++++++++
>>  drivers/gpu/drm/i915/intel_drv.h             |  2 ++
>>  3 files changed, 49 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index 919f5ac844c8..4a85abef93e7 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -6663,6 +6663,20 @@ bool intel_port_is_combophy(struct drm_i915_private *dev_priv, enum port port)
>>  	return false;
>>  }
>>
>> +bool intel_phy_is_combo(struct drm_i915_private *dev_priv, enum phy phy)
>> +{
>> +	if (phy == PHY_NONE)
>> +		return false;
>> +
>> +	if (IS_ELKHARTLAKE(dev_priv))
>> +		return phy <= PHY_C;
>> +
>> +	if (INTEL_GEN(dev_priv) >= 11)
>> +		return phy <= PHY_B;
>> +
>> +	return false;
>> +}
>> +
>>  bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port)
>>  {
>>  	if (INTEL_GEN(dev_priv) >= 11 && !IS_ELKHARTLAKE(dev_priv))
>> @@ -6671,9 +6685,25 @@ bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port)
>>  	return false;
>>  }
>>
>> +bool intel_phy_is_tc(struct drm_i915_private *dev_priv, enum phy phy)
>> +{
>> +	if (INTEL_GEN(dev_priv) >= 11 && !IS_ELKHARTLAKE(dev_priv))
>> +		return phy >= PHY_C && phy <= PHY_F;
>> +
>> +	return false;
>> +}
>> +
>> +enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port)
>> +{
>> +	if (IS_ELKHARTLAKE(i915) && port == PORT_D)
>> +		return PHY_A;
>> +
>> +	return (enum phy)port;
>> +}
>> +
>>  enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv, enum port port)
>>  {
>> -	if (!intel_port_is_tc(dev_priv, port))
>> +	if (!intel_phy_is_tc(dev_priv, intel_port_to_phy(dev_priv, port)))
>>  		return PORT_TC_NONE;
>>
>>  	return port - PORT_C;
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
>> index d296556ed82e..d53285fb883f 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display.h
>> @@ -228,6 +228,21 @@ struct intel_link_m_n {
>>  	u32 link_n;
>>  };
>>
>> +enum phy {
>> +	PHY_NONE = -1,
>> +
>> +	PHY_A = 0,
>> +	PHY_B,
>> +	PHY_C,
>> +	PHY_D,
>> +	PHY_E,
>> +	PHY_F,
>> +
>> +	I915_MAX_PHYS
>> +};
>
>I was pondering if we could eventually do something like:
>
>enum phy {
>	PHY_COMBO_A = 0,
>	PHY_COMBO_B,
>	...
>
>	PHY_TC_1,
>	PHY_TC_2,
>	...
>};
>
>and probably also add encoder->phy so we can contain
>that port<->phy mapping logic in the encoder init.
>I think that should work more or less fine since I
>don't think PHY_TC_1 needs to have any specific value.

that's not true. All TC registers are based off the TC phy number.
Hence all the conversion we do port_to_tc()... I'd like to remove that
in future and just stuff the phy index in intel_digital_port, as we
already do for other tc_phy_* fields (we could add a union there so each
phy adds its own fields).

And I'd rather not do the single phy namespace - it doesn't
play well with TGL and the combo/tc.

Lucas De Marchi

>
>Unfortunaltey I don't have a great idea how to do the
>same for the DDIs since there the number of combo DDIs
>changes but we still need the PORT_TC1 (assuming we had
>one) to be DDI_<last combo DDI> + 1. One random silly
>idea was to decouple the enum port from the register
>definitions by having just some kind of
>encoder->port_index for those. But that doesn't feel
>entirely great either.
>
>Anyways, something to think about in the future perhaps.
>
>Patch is
>Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
>> +
>> +#define phy_name(a) ((a) + 'A')
>> +
>>  #define for_each_pipe(__dev_priv, __p) \
>>  	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++)
>>
>> @@ -356,5 +371,6 @@ void lpt_disable_clkout_dp(struct drm_i915_private *dev_priv);
>>  u32 intel_plane_fb_max_stride(struct drm_i915_private *dev_priv,
>>  			      u32 pixel_format, u64 modifier);
>>  bool intel_plane_can_remap(const struct intel_plane_state *plane_state);
>> +enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port);
>>
>>  #endif
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 24c63ed45c6f..815c26c0b98c 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1493,7 +1493,9 @@ void intel_encoder_destroy(struct drm_encoder *encoder);
>>  struct drm_display_mode *
>>  intel_encoder_current_mode(struct intel_encoder *encoder);
>>  bool intel_port_is_combophy(struct drm_i915_private *dev_priv, enum port port);
>> +bool intel_phy_is_combo(struct drm_i915_private *dev_priv, enum phy phy);
>>  bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port);
>> +bool intel_phy_is_tc(struct drm_i915_private *dev_priv, enum phy phy);
>>  enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv,
>>  			      enum port port);
>>  int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
>> --
>> 2.17.2
>
>-- 
>Ville Syrjälä
>Intel