Re: [PATCH 03/14] drm/i915/tgl: Finish modular FIA support on registers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2019-09-13 at 23:24 -0700, Lucas De Marchi wrote:
> On Fri, Sep 13, 2019 at 3:33 PM José Roberto de Souza
> <jose.souza@xxxxxxxxx> wrote:
> > If platform supports and has modular FIA is enabled, the registers
> > bits also change, example: reading TC3 registers with modular FIA
> > enabled, driver should read from FIA2 but with TC1 bits offsets.
> > 
> > It is described in BSpec 50231 for DFLEXDPSP, other registers don't
> > have the BSpec description but testing in real hardware have proven
> > that it had moved for all other registers too.
> > 
> > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c |  2 +
> >  drivers/gpu/drm/i915/display/intel_tc.c      | 52 ++++++++++++--
> > ------
> >  drivers/gpu/drm/i915/display/intel_tc.h      |  2 +
> >  drivers/gpu/drm/i915/i915_drv.h              |  3 ++
> >  drivers/gpu/drm/i915/i915_reg.h              | 45 ++++++++------
> > ---
> >  5 files changed, 61 insertions(+), 43 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 4e001113e828..cd0a248bfe49 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -15384,6 +15384,8 @@ static void intel_setup_outputs(struct
> > drm_i915_private *dev_priv)
> >         if (!HAS_DISPLAY(dev_priv))
> >                 return;
> > 
> > +       intel_tc_init(dev_priv);
> > +
> >         if (INTEL_GEN(dev_priv) >= 12) {
> >                 /* TODO: initialize TC ports as well */
> >                 intel_ddi_init(dev_priv, PORT_A);
> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> > b/drivers/gpu/drm/i915/display/intel_tc.c
> > index 3a7302e360cc..fc5d0e73cf21 100644
> > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > @@ -23,19 +23,21 @@ static const char *tc_port_mode_name(enum
> > tc_port_mode mode)
> >         return names[mode];
> >  }
> > 
> > -static bool has_modular_fia(struct drm_i915_private *i915)
> > +void intel_tc_init(struct drm_i915_private *i915)
> >  {
> > +       u32 val;
> > +
> >         if (!INTEL_INFO(i915)->display.has_modular_fia)
> > -               return false;
> > +               return;
> > 
> > -       return intel_uncore_read(&i915->uncore,
> > -                                PORT_TX_DFLEXDPSP(FIA1)) &
> > MODULAR_FIA_MASK;
> > +       val = intel_uncore_read(&i915->uncore,
> > PORT_TX_DFLEXDPSP(FIA1));
> > +       i915->has_modular_fia = val & MODULAR_FIA_MASK;
> 
> Not a fan of stuffing tc-private details in i195 struct. Since this
> is
> only executed on initialization maybe it's
> not worth the few register reads we spare. If we go with this
> approach, then I think we should not use
> "has_" prefix so we don't get confused by the device_info fields.
> 
> >  }
> > 
> >  static enum phy_fia tc_port_to_fia(struct drm_i915_private *i915,
> >                                    enum tc_port tc_port)
> >  {
> > -       if (!has_modular_fia(i915))
> > +       if (!i915->has_modular_fia)
> >                 return FIA1;
> > 
> >         /*
> > @@ -51,14 +53,15 @@ u32 intel_tc_port_get_lane_mask(struct
> > intel_digital_port *dig_port)
> >         enum tc_port tc_port = intel_port_to_tc(i915, dig_port-
> > >base.port);
> >         struct intel_uncore *uncore = &i915->uncore;
> >         u32 lane_mask;
> > +       bool mod_fia = i915->has_modular_fia;
> 
> s/mod_fia/modular_fia/ or just don't add the additional var
> 
> >         lane_mask = intel_uncore_read(uncore,
> >                                       PORT_TX_DFLEXDPSP(dig_port-
> > >tc_phy_fia));
> > 
> >         WARN_ON(lane_mask == 0xffffffff);
> > 
> > -       return (lane_mask & DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
> > -              DP_LANE_ASSIGNMENT_SHIFT(tc_port);
> > +       return (lane_mask & DP_LANE_ASSIGNMENT_MASK(mod_fia,
> > tc_port)) >>
> > +              DP_LANE_ASSIGNMENT_SHIFT(mod_fia, tc_port);
> >  }
> > 
> >  u32 intel_tc_port_get_pin_assignment_mask(struct
> > intel_digital_port *dig_port)
> > @@ -66,6 +69,7 @@ u32 intel_tc_port_get_pin_assignment_mask(struct
> > intel_digital_port *dig_port)
> >         struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> >         enum tc_port tc_port = intel_port_to_tc(i915, dig_port-
> > >base.port);
> >         struct intel_uncore *uncore = &i915->uncore;
> > +       bool mod_fia = i915->has_modular_fia;
> >         u32 pin_mask;
> > 
> >         pin_mask = intel_uncore_read(uncore,
> > @@ -73,8 +77,8 @@ u32 intel_tc_port_get_pin_assignment_mask(struct
> > intel_digital_port *dig_port)
> > 
> >         WARN_ON(pin_mask == 0xffffffff);
> > 
> > -       return (pin_mask & DP_PIN_ASSIGNMENT_MASK(tc_port)) >>
> > -              DP_PIN_ASSIGNMENT_SHIFT(tc_port);
> > +       return (pin_mask & DP_PIN_ASSIGNMENT_MASK(mod_fia,
> > tc_port)) >>
> > +              DP_PIN_ASSIGNMENT_SHIFT(mod_fia, tc_port);
> >  }
> > 
> >  int intel_tc_port_fia_max_lane_count(struct intel_digital_port
> > *dig_port)
> > @@ -114,25 +118,28 @@ void intel_tc_port_set_fia_lane_count(struct
> > intel_digital_port *dig_port,
> >         enum tc_port tc_port = intel_port_to_tc(i915, dig_port-
> > >base.port);
> >         bool lane_reversal = dig_port->saved_port_bits &
> > DDI_BUF_PORT_REVERSAL;
> >         struct intel_uncore *uncore = &i915->uncore;
> > +       bool mod_fia = i915->has_modular_fia;
> >         u32 val;
> > 
> >         WARN_ON(lane_reversal && dig_port->tc_mode !=
> > TC_PORT_LEGACY);
> > 
> >         val = intel_uncore_read(uncore,
> >                                 PORT_TX_DFLEXDPMLE1(dig_port-
> > >tc_phy_fia));
> > -       val &= ~DFLEXDPMLE1_DPMLETC_MASK(tc_port);
> > +       val &= ~DFLEXDPMLE1_DPMLETC_MASK(mod_fia, tc_port);
> > 
> >         switch (required_lanes) {
> >         case 1:
> > -               val |= lane_reversal ?
> > DFLEXDPMLE1_DPMLETC_ML3(tc_port) :
> > -                       DFLEXDPMLE1_DPMLETC_ML0(tc_port);
> > +               val |= lane_reversal ?
> > +                       DFLEXDPMLE1_DPMLETC_ML3(mod_fia, tc_port) :
> > +                       DFLEXDPMLE1_DPMLETC_ML0(mod_fia, tc_port);
> >                 break;
> >         case 2:
> > -               val |= lane_reversal ?
> > DFLEXDPMLE1_DPMLETC_ML3_2(tc_port) :
> > -                       DFLEXDPMLE1_DPMLETC_ML1_0(tc_port);
> > +               val |= lane_reversal ?
> > +                       DFLEXDPMLE1_DPMLETC_ML3_2(mod_fia, tc_port)
> > :
> > +                       DFLEXDPMLE1_DPMLETC_ML1_0(mod_fia,
> > tc_port);
> >                 break;
> >         case 4:
> > -               val |= DFLEXDPMLE1_DPMLETC_ML3_0(tc_port);
> > +               val |= DFLEXDPMLE1_DPMLETC_ML3_0(mod_fia, tc_port);
> >                 break;
> >         default:
> >                 MISSING_CASE(required_lanes);
> > @@ -180,9 +187,9 @@ static u32 tc_port_live_status_mask(struct
> > intel_digital_port *dig_port)
> >                 return mask;
> >         }
> > 
> > -       if (val & TC_LIVE_STATE_TBT(tc_port))
> > +       if (val & TC_LIVE_STATE_TBT(i915->has_modular_fia,
> > tc_port))
> >                 mask |= BIT(TC_PORT_TBT_ALT);
> > -       if (val & TC_LIVE_STATE_TC(tc_port))
> > +       if (val & TC_LIVE_STATE_TC(i915->has_modular_fia, tc_port))
> >                 mask |= BIT(TC_PORT_DP_ALT);
> > 
> >         if (intel_uncore_read(uncore, SDEISR) &
> > SDE_TC_HOTPLUG_ICP(tc_port))
> > @@ -200,6 +207,7 @@ static bool icl_tc_phy_status_complete(struct
> > intel_digital_port *dig_port)
> >         struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> >         enum tc_port tc_port = intel_port_to_tc(i915, dig_port-
> > >base.port);
> >         struct intel_uncore *uncore = &i915->uncore;
> > +       bool mod_fia = i915->has_modular_fia;
> >         u32 val;
> > 
> >         val = intel_uncore_read(uncore,
> > @@ -210,7 +218,7 @@ static bool icl_tc_phy_status_complete(struct
> > intel_digital_port *dig_port)
> >                 return false;
> >         }
> > 
> > -       return val & DP_PHY_MODE_STATUS_COMPLETED(tc_port);
> > +       return val & DP_PHY_MODE_STATUS_COMPLETED(mod_fia,
> > tc_port);
> >  }
> > 
> >  static bool icl_tc_phy_set_safe_mode(struct intel_digital_port
> > *dig_port,
> > @@ -219,6 +227,7 @@ static bool icl_tc_phy_set_safe_mode(struct
> > intel_digital_port *dig_port,
> >         struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> >         enum tc_port tc_port = intel_port_to_tc(i915, dig_port-
> > >base.port);
> >         struct intel_uncore *uncore = &i915->uncore;
> > +       bool mod_fia = i915->has_modular_fia;
> >         u32 val;
> > 
> >         val = intel_uncore_read(uncore,
> > @@ -231,9 +240,9 @@ static bool icl_tc_phy_set_safe_mode(struct
> > intel_digital_port *dig_port,
> >                 return false;
> >         }
> > 
> > -       val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > +       val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(mod_fia, tc_port);
> >         if (!enable)
> > -               val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > +               val |= DP_PHY_MODE_STATUS_NOT_SAFE(mod_fia,
> > tc_port);
> > 
> >         intel_uncore_write(uncore,
> >                            PORT_TX_DFLEXDPCSSS(dig_port-
> > >tc_phy_fia), val);
> > @@ -250,6 +259,7 @@ static bool icl_tc_phy_is_in_safe_mode(struct
> > intel_digital_port *dig_port)
> >         struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> >         enum tc_port tc_port = intel_port_to_tc(i915, dig_port-
> > >base.port);
> >         struct intel_uncore *uncore = &i915->uncore;
> > +       bool mod_fia = i915->has_modular_fia;
> >         u32 val;
> > 
> >         val = intel_uncore_read(uncore,
> > @@ -260,7 +270,7 @@ static bool icl_tc_phy_is_in_safe_mode(struct
> > intel_digital_port *dig_port)
> >                 return true;
> >         }
> > 
> > -       return !(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port));
> > +       return !(val & DP_PHY_MODE_STATUS_NOT_SAFE(mod_fia,
> > tc_port));
> >  }
> > 
> >  /*
> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.h
> > b/drivers/gpu/drm/i915/display/intel_tc.h
> > index 463f1b3c836f..2374352d7c31 100644
> > --- a/drivers/gpu/drm/i915/display/intel_tc.h
> > +++ b/drivers/gpu/drm/i915/display/intel_tc.h
> > @@ -10,6 +10,7 @@
> >  #include <linux/types.h>
> > 
> >  struct intel_digital_port;
> > +struct drm_i915_private;
> > 
> >  bool intel_tc_port_connected(struct intel_digital_port *dig_port);
> >  u32 intel_tc_port_get_lane_mask(struct intel_digital_port
> > *dig_port);
> > @@ -27,5 +28,6 @@ void intel_tc_port_put_link(struct
> > intel_digital_port *dig_port);
> >  bool intel_tc_port_ref_held(struct intel_digital_port *dig_port);
> > 
> >  void intel_tc_port_init(struct intel_digital_port *dig_port, bool
> > is_legacy);
> > +void intel_tc_init(struct drm_i915_private *dev_priv);
> > 
> >  #endif /* __INTEL_TC_H__ */
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index bf600888b3f1..a36d1929fde1 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1765,6 +1765,9 @@ struct drm_i915_private {
> >         /* Mutex to protect the above hdcp component related
> > values. */
> >         struct mutex hdcp_comp_mutex;
> > 
> > +       /* Monolithic or modular FIA */
> > +       bool has_modular_fia;
> > +
> >         /*
> >          * NOTE: This is the dri1/ums dungeon, don't add stuff
> > here. Your patch
> >          * will be rejected. Instead look for a better place.
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 16d5548adb84..8aaf53283200 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -2165,14 +2165,15 @@ static inline bool
> > i915_mmio_reg_valid(i915_reg_t reg)
> >  #define _FIA(fia)                      _PICK((fia), FIA1_BASE,
> > FIA2_BASE, FIA3_BASE)
> >  #define _MMIO_FIA(fia, off)            _MMIO(_FIA(fia) + (off))
> > 
> > +#define _TC_MOD_PORT_ID(has_modular_fia,
> > tc_port)      ((has_modular_fia) ? (tc_port) % 2 : (tc_port))
> 
> instead of doing all of this, what about storing a
> dig_port->tc_phy_fia_idx that is set to
> tc_port on monolithic FIA and to tc_port % 2 on modular...? Then it
> would be initialized together with
> dig_port->tc_phy_fia and we could even remove tc_port_to_fia() that
> would not be used anymore since we would
> check that in the caller.

Looks, better going to move to tc_phy_fia_idx.

Thanks

> 
> >  /* ICL PHY DFLEX registers */
> > -#define PORT_TX_DFLEXDPMLE1(fia)       _MMIO_FIA((fia),  0x008C0)
> > -#define   DFLEXDPMLE1_DPMLETC_MASK(tc_port)    (0xf << (4 *
> > (tc_port)))
> 
> in all these macros it would be s/tc_port/idx/
> 
> Lucas De Marchi
> 
> > -#define   DFLEXDPMLE1_DPMLETC_ML0(tc_port)     (1 << (4 *
> > (tc_port)))
> > -#define   DFLEXDPMLE1_DPMLETC_ML1_0(tc_port)   (3 << (4 *
> > (tc_port)))
> > -#define   DFLEXDPMLE1_DPMLETC_ML3(tc_port)     (8 << (4 *
> > (tc_port)))
> > -#define   DFLEXDPMLE1_DPMLETC_ML3_2(tc_port)   (12 << (4 *
> > (tc_port)))
> > -#define   DFLEXDPMLE1_DPMLETC_ML3_0(tc_port)   (15 << (4 *
> > (tc_port)))
> > +#define
> > PORT_TX_DFLEXDPMLE1(fia)                       _MMIO_FIA((fia),  0x
> > 008C0)
> > +#define   DFLEXDPMLE1_DPMLETC_MASK(mod, tc_port)       (0xf << (4
> > * (_TC_MOD_PORT_ID(mod, tc_port))))
> > +#define   DFLEXDPMLE1_DPMLETC_ML0(mod, tc_port)                (1
> > << (4 * (_TC_MOD_PORT_ID(mod, tc_port))))
> > +#define   DFLEXDPMLE1_DPMLETC_ML1_0(mod, tc_port)      (3 << (4 *
> > (_TC_MOD_PORT_ID(mod, tc_port))))
> > +#define   DFLEXDPMLE1_DPMLETC_ML3(mod, tc_port)                (8
> > << (4 * (_TC_MOD_PORT_ID(mod, tc_port))))
> > +#define   DFLEXDPMLE1_DPMLETC_ML3_2(mod, tc_port)      (12 << (4 *
> > (_TC_MOD_PORT_ID(mod, tc_port))))
> > +#define   DFLEXDPMLE1_DPMLETC_ML3_0(mod, tc_port)      (15 << (4 *
> > (_TC_MOD_PORT_ID(mod, tc_port))))
> > 
> >  /* BXT PHY Ref registers */
> >  #define _PORT_REF_DW3_A                        0x16218C
> > @@ -11669,23 +11670,23 @@ enum skl_power_gate {
> >                                                 _ICL_DSC1_RC_BUF_TH
> > RESH_1_UDW_PB, \
> >                                                 _ICL_DSC1_RC_BUF_TH
> > RESH_1_UDW_PC)
> > 
> > -#define PORT_TX_DFLEXDPSP(fia)                 _MMIO_FIA((fia),
> > 0x008A0)
> > -#define   MODULAR_FIA_MASK                     (1 << 4)
> > -#define   TC_LIVE_STATE_TBT(tc_port)           (1 << ((tc_port) *
> > 8 + 6))
> > -#define   TC_LIVE_STATE_TC(tc_port)            (1 << ((tc_port) *
> > 8 + 5))
> > -#define   DP_LANE_ASSIGNMENT_SHIFT(tc_port)    ((tc_port) * 8)
> > -#define   DP_LANE_ASSIGNMENT_MASK(tc_port)     (0xf << ((tc_port)
> > * 8))
> > -#define   DP_LANE_ASSIGNMENT(tc_port, x)       ((x) << ((tc_port)
> > * 8))
> > +#define
> > PORT_TX_DFLEXDPSP(fia)                         _MMIO_FIA((fia),
> > 0x008A0)
> > +#define   MODULAR_FIA_MASK                             (1 << 4)
> > +#define   TC_LIVE_STATE_TBT(mod, tc_port)              (1 <<
> > ((_TC_MOD_PORT_ID(mod, tc_port)) * 8 + 6))
> > +#define   TC_LIVE_STATE_TC(mod, tc_port)               (1 <<
> > ((_TC_MOD_PORT_ID(mod, tc_port)) * 8 + 5))
> > +#define   DP_LANE_ASSIGNMENT_SHIFT(mod,
> > tc_port)       ((_TC_MOD_PORT_ID(mod, tc_port)) * 8)
> > +#define   DP_LANE_ASSIGNMENT_MASK(mod,
> > tc_port)                (0xf << ((_TC_MOD_PORT_ID(mod, tc_port)) *
> > 8))
> > +#define   DP_LANE_ASSIGNMENT(mod, tc_port, x)          ((x) <<
> > ((_TC_MOD_PORT_ID(mod, tc_port)) * 8))
> > 
> > -#define
> > PORT_TX_DFLEXDPPMS(fia)                        _MMIO_FIA((fia),
> > 0x00890)
> > -#define   DP_PHY_MODE_STATUS_COMPLETED(tc_port)                (1
> > << (tc_port))
> > +#define
> > PORT_TX_DFLEXDPPMS(fia)                                _MMIO_FIA((f
> > ia), 0x00890)
> > +#define   DP_PHY_MODE_STATUS_COMPLETED(mod, tc_port)   (1 <<
> > (_TC_MOD_PORT_ID(mod, tc_port)))
> > 
> > -#define PORT_TX_DFLEXDPCSSS(fia)               _MMIO_FIA((fia),
> > 0x00894)
> > -#define   DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)         (1 <<
> > (tc_port))
> > +#define
> > PORT_TX_DFLEXDPCSSS(fia)                       _MMIO_FIA((fia),
> > 0x00894)
> > +#define   DP_PHY_MODE_STATUS_NOT_SAFE(mod, tc_port)    (1 <<
> > (_TC_MOD_PORT_ID(mod, tc_port)))
> > 
> > -#define PORT_TX_DFLEXPA1(fia)                  _MMIO_FIA((fia),
> > 0x00880)
> > -#define   DP_PIN_ASSIGNMENT_SHIFT(tc_port)             ((tc_port)
> > * 4)
> > -#define   DP_PIN_ASSIGNMENT_MASK(tc_port)              (0xf <<
> > ((tc_port) * 4))
> > -#define   DP_PIN_ASSIGNMENT(tc_port, x)        ((x) << ((tc_port)
> > * 4))
> > +#define
> > PORT_TX_DFLEXPA1(fia)                          _MMIO_FIA((fia),
> > 0x00880)
> > +#define   DP_PIN_ASSIGNMENT_SHIFT(mod,
> > tc_port)                ((_TC_MOD_PORT_ID(mod, tc_port)) * 4)
> > +#define   DP_PIN_ASSIGNMENT_MASK(mod, tc_port) (0xf <<
> > ((_TC_MOD_PORT_ID(mod, tc_port)) * 4))
> > +#define   DP_PIN_ASSIGNMENT(mod, tc_port, x)   ((x) <<
> > ((_TC_MOD_PORT_ID(mod, tc_port)) * 4))
> > 
> >  #endif /* _I915_REG_H_ */
> > --
> > 2.23.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux