On Mon, Jul 08, 2019 at 07:02:49AM -0700, Lucas De Marchi wrote:
> 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@xxxxxxxxxxxxxxx>
> >> >> >> Cc: José Roberto de Souza <jose.souza@xxxxxxxxx>
> >> >> >> Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> >> >> >> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >> >> >> Cc: Imre Deak <imre.deak@xxxxxxxxx>
> >> >> >> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> >> >> >> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> >> >> >> ---
> >> >> >> 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.
The port->phy mapping doesn't matter. The enum is just about the phy.
And reg macros would be just something like:
#define _TC_PHY_IDX(phy) ((phy)-PHY_TC1)
#define BLAH(phy) _MMIO(..._TC_PHY_IDX(phy)...)
> 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@xxxxxxxxxxxxxxx>
> >> >> >
> >> >> >> +
> >> >> >> +#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
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
