Re: [PATCH v2] drm/i915/display: UHBR rates for Thunderbolt

[Imre Deak <imre.deak@intel.com>]

On Tue, Dec 24, 2024 at 06:01:48PM -0300, Gustavo Sousa wrote:
> Quoting Imre Deak (2024-12-18 11:56:46-03:00)
> >On Tue, Dec 17, 2024 at 04:34:40PM +0200, Mika Kahola wrote:
> >> tbt-alt mode is missing uhbr rates 10G and 20G. This requires
> >> requires pll clock rates 312.5 MHz and 625 MHz to be added,
> >> respectively. The uhbr rates are supported only form PTL+
> >> platforms.
> >> 
> >> v2: Add drm_WARN_ON() to check if port clock is not supported by
> >>     the platform (Imre)
> >>     Combine forward ungate with mask parameter (Imre)
> >>     Rename XE3LPDP_* to XE3D_* (Imre)
> >
> >I highly disagree with the usage of the XE[23]{D,LPD,LPDP} etc. ciphers
> >in the driver in general, instead of the human readable MTL, LNL, PTL
> >etc., the human readable versions actually serving the intended purpose
> >of reminding a reader what exact platform the code they read is about.
> >
> >I still don't know and haven't managed to figure out from the spec what
> >either XE3LPDP or XE3D is about, looks like this same clock select field
> >width is used by other XE3 platforms as well. So imo let's stick with
> >the XE3 prefix used already elsewhere in the driver (no need to resend
> >the patch just for that). With that:
> 
> One argument for using the XE* prefixes (e.g. XE3LPD for Xe3_LPD, which
> is PTL's display) is that, technically speaking, the deltas are
> associated to the IP rather than the whole platform.
> 
> For the recent display IPs, the same display IP could theoretically be
> used in different platforms. I believe that became true starting with
> Xe_LPD+ (MTL's display). Conversely, it could also be possible that
> different SKUs of a platform X could be using different display IPs.
> And the driver figures out which display it is driving via GMD IDs.
> 
> As such, I personally prefer the more accurate IP-based naming rather
> than platform-based for stuff that is tied to the IP.

Ftr, the xeX-YYZW terminology decoded (IIUC):

xe: Exascale for Everyone
X:  -,2,3,4 (microarchitecture 1,2,3,4)
YY: LP,HP (Low Power, High Power)
Z:  C,D,G,M (IP: Compute, Display, Graphics, Media)
W:  -,P (?)

I still don't know what is the last P in XELPDP for instance. I also
suspect that more letters/numbers will be needed to distinguish between
IPs, if the categories above are not enough for this.

Imo, the first thing one wants to know when reading the code is which
actual platform(s) (i.e. DG2, MeteorLake, LunarLake, PantherLake) the
given code applies to. Using the XE terminology in the code is just an
extra burden in this (first decode xeX-YYZW based on the above, then map
this to all the actual platforms using the IP).

I still think it's better to follow the existing rule to use instead the
first platform where the IP appeared.

> --
> Gustavo Sousa
> 
> >
> >Reviewed-by: Imre Deak <imre.deak@intel.com>
> >
> >> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_cx0_phy.c  | 39 +++++++++++++++++--
> >>  .../gpu/drm/i915/display/intel_cx0_phy_regs.h |  4 ++
> >>  2 files changed, 39 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> index cc734079c3b8..a8e0450c0378 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> @@ -3070,7 +3070,10 @@ int intel_mtl_tbt_calc_port_clock(struct intel_encoder *encoder)
> >>  
> >>          val = intel_de_read(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port));
> >>  
> >> -        clock = REG_FIELD_GET(XELPDP_DDI_CLOCK_SELECT_MASK, val);
> >> +        if (DISPLAY_VER(display) >= 30)
> >> +                clock = REG_FIELD_GET(XE3D_DDI_CLOCK_SELECT_MASK, val);
> >> +        else
> >> +                clock = REG_FIELD_GET(XELPDP_DDI_CLOCK_SELECT_MASK, val);
> >>  
> >>          drm_WARN_ON(display->drm, !(val & XELPDP_FORWARD_CLOCK_UNGATE));
> >>          drm_WARN_ON(display->drm, !(val & XELPDP_TBT_CLOCK_REQUEST));
> >> @@ -3085,13 +3088,18 @@ int intel_mtl_tbt_calc_port_clock(struct intel_encoder *encoder)
> >>                  return 540000;
> >>          case XELPDP_DDI_CLOCK_SELECT_TBT_810:
> >>                  return 810000;
> >> +        case XELPDP_DDI_CLOCK_SELECT_TBT_312_5:
> >> +                return 1000000;
> >> +        case XELPDP_DDI_CLOCK_SELECT_TBT_625:
> >> +                return 2000000;
> >>          default:
> >>                  MISSING_CASE(clock);
> >>                  return 162000;
> >>          }
> >>  }
> >>  
> >> -static int intel_mtl_tbt_clock_select(int clock)
> >> +static int intel_mtl_tbt_clock_select(struct intel_display *display,
> >> +                                      int clock)
> >>  {
> >>          switch (clock) {
> >>          case 162000:
> >> @@ -3102,6 +3110,18 @@ static int intel_mtl_tbt_clock_select(int clock)
> >>                  return XELPDP_DDI_CLOCK_SELECT_TBT_540;
> >>          case 810000:
> >>                  return XELPDP_DDI_CLOCK_SELECT_TBT_810;
> >> +        case 1000000:
> >> +                if (DISPLAY_VER(display) < 30) {
> >> +                        drm_WARN_ON(display->drm, "UHBR10 not supported for the platform\n");
> >> +                        return XELPDP_DDI_CLOCK_SELECT_TBT_162;
> >> +                }
> >> +                return XELPDP_DDI_CLOCK_SELECT_TBT_312_5;
> >> +        case 2000000:
> >> +                if (DISPLAY_VER(display) < 30) {
> >> +                        drm_WARN_ON(display->drm, "UHBR20 not supported for the platform\n");
> >> +                        return XELPDP_DDI_CLOCK_SELECT_TBT_162;
> >> +                }
> >> +                return XELPDP_DDI_CLOCK_SELECT_TBT_625;
> >>          default:
> >>                  MISSING_CASE(clock);
> >>                  return XELPDP_DDI_CLOCK_SELECT_TBT_162;
> >> @@ -3114,15 +3134,26 @@ static void intel_mtl_tbt_pll_enable(struct intel_encoder *encoder,
> >>          struct intel_display *display = to_intel_display(encoder);
> >>          enum phy phy = intel_encoder_to_phy(encoder);
> >>          u32 val = 0;
> >> +        u32 mask;
> >>  
> >>          /*
> >>           * 1. Program PORT_CLOCK_CTL REGISTER to configure
> >>           * clock muxes, gating and SSC
> >>           */
> >> -        val |= XELPDP_DDI_CLOCK_SELECT(intel_mtl_tbt_clock_select(crtc_state->port_clock));
> >> +
> >> +        if (DISPLAY_VER(display) >= 30) {
> >> +                mask = XE3D_DDI_CLOCK_SELECT_MASK;
> >> +                val |= XE3D_DDI_CLOCK_SELECT(intel_mtl_tbt_clock_select(display, crtc_state->port_clock));
> >> +        } else {
> >> +                mask = XELPDP_DDI_CLOCK_SELECT_MASK;
> >> +                val |= XELPDP_DDI_CLOCK_SELECT(intel_mtl_tbt_clock_select(display, crtc_state->port_clock));
> >> +        }
> >> +
> >> +        mask |= XELPDP_FORWARD_CLOCK_UNGATE;
> >>          val |= XELPDP_FORWARD_CLOCK_UNGATE;
> >> +
> >>          intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
> >> -                     XELPDP_DDI_CLOCK_SELECT_MASK | XELPDP_FORWARD_CLOCK_UNGATE, val);
> >> +                     mask, val);
> >>  
> >>          /* 2. Read back PORT_CLOCK_CTL REGISTER */
> >>          val = intel_de_read(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port));
> >> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> >> index c685479c9756..bf95ac234b2b 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> >> @@ -187,7 +187,9 @@
> >>  #define   XELPDP_TBT_CLOCK_REQUEST                        REG_BIT(19)
> >>  #define   XELPDP_TBT_CLOCK_ACK                                REG_BIT(18)
> >>  #define   XELPDP_DDI_CLOCK_SELECT_MASK                        REG_GENMASK(15, 12)
> >> +#define   XE3D_DDI_CLOCK_SELECT_MASK                        REG_GENMASK(16, 12)
> >>  #define   XELPDP_DDI_CLOCK_SELECT(val)                        REG_FIELD_PREP(XELPDP_DDI_CLOCK_SELECT_MASK, val)
> >> +#define   XE3D_DDI_CLOCK_SELECT(val)                        REG_FIELD_PREP(XE3D_DDI_CLOCK_SELECT_MASK, val)
> >>  #define   XELPDP_DDI_CLOCK_SELECT_NONE                        0x0
> >>  #define   XELPDP_DDI_CLOCK_SELECT_MAXPCLK                0x8
> >>  #define   XELPDP_DDI_CLOCK_SELECT_DIV18CLK                0x9
> >> @@ -195,6 +197,8 @@
> >>  #define   XELPDP_DDI_CLOCK_SELECT_TBT_270                0xd
> >>  #define   XELPDP_DDI_CLOCK_SELECT_TBT_540                0xe
> >>  #define   XELPDP_DDI_CLOCK_SELECT_TBT_810                0xf
> >> +#define   XELPDP_DDI_CLOCK_SELECT_TBT_312_5                0x18
> >> +#define   XELPDP_DDI_CLOCK_SELECT_TBT_625                0x19
> >>  #define   XELPDP_FORWARD_CLOCK_UNGATE                        REG_BIT(10)
> >>  #define   XELPDP_LANE1_PHY_CLOCK_SELECT                        REG_BIT(8)
> >>  #define   XELPDP_SSC_ENABLE_PLLA                        REG_BIT(1)
> >> -- 
> >> 2.43.0
> >>