[Intel-gfx] [PATCH v4 1/5] drm/i915/gen11: Start distinguishing 'phy' from 'port'
Lucas De Marchi
lucas.demarchi at intel.com
Mon Jul 8 14:02:49 UTC 2019
On Fri, Jul 05, 2019 at 01:33:10PM +0300, Ville Syrjälä wrote:
>On Thu, Jul 04, 2019 at 08:55:51AM -0700, Lucas De Marchi wrote:
>> On Thu, Jul 04, 2019 at 06:09:04PM +0300, Ville Syrjälä wrote:
>> >On Thu, Jul 04, 2019 at 07:54:26AM -0700, Lucas De Marchi wrote:
>> >> 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.
>> >
>> >That's just a trivial (x)-TC1, so I stand by what I said.
>>
>> EHL and TGL have 3 combo phys. ICL has 2. So TC1 would have to be a
>> different value for ICL and the others for this to work.
>
>It would just be an arbitrary number (eg. 8).
One that is propagated to every access to those registers, and we need
to keep this enum updated as well the conversion functions because every
platform has a different port->phy mapping. IMO it should be done during
init like in the pseudo code I posted. And in that case TC1 = 0. Always.
Lucas De Marchi
>
>> I think we should treat the index as just a number we use to compute the
>> right base for the registers in that hw IP.
>>
>> >
>> >> 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.
>> >
>> >I think it would work just fine for that. A single namespace would allow
>> >us to remove all the crazy port->PHY type mapping we have going on
>> >currently. Probably the best alternative would be separate namespaces
>> >for each type with a new enum to identify the type.
>>
>> My proposal is in the lines of that alternative approach. Save the phy
>> type and the phy index in intel_digital_port. And allow each phy to store
>> its fields there. Something along:
>>
>> struct intel_digital_port {
>> ...
>> struct {
>> enum phy_type type;
>> u8 idx;
>> union {
>> struct {
>> struct mutex lock; /* protects the TypeC port mode */
>> intel_wakeref_t lock_wakeref;
>> int link_refcount;
>> char tc_port_name[8];
>> enum tc_port_mode tc_mode;
>> bool legacy_port;
>> u8 fia;
>> } tc;
>> struct {
>> ...
>> } combo;
>> struct {
>> ...
>> } dpio??;
>> };
>> } phy;
>> };
>>
>> >From a quick look I don't think the idx is relevant enough to have its
>> own enum, but I wouldn't mind.
>>
>> then no more port->phy everywhere as we have right now and we only
>> assign idx to the right value during init. The TC rework we had recently
>> makes it nice and I'm starting to do it, but I'd rather merge the TGL
>> patches before. For Modular FIA I'm already stashing the fia index
>> there (since I had to redo the support due to the TC rework),
>> but without the additional struct.
>> See https://patchwork.freedesktop.org/series/63175/
>>
>> Lucas De Marchi
>>
>> >
>> >>
>> >> 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
>> >
>> >--
>> >Ville Syrjälä
>> >Intel
>
>--
>Ville Syrjälä
>Intel
More information about the Intel-gfx
mailing list