On Sat, 2020-03-28 at 11:57 +0200, Imre Deak wrote:
> Hi José,
>
> On Tue, Mar 24, 2020 at 01:14:24PM -0700, José Roberto de Souza
> wrote:
> > TC ports can enter in TCCOLD to save power and is required to
> > request
> > to PCODE to exit this state before use or read to TC registers.
> >
> > For TGL there is a new MBOX command to do that with a parameter to
> > ask
> > PCODE to exit and block TCCOLD entry or unblock TCCOLD entry.
> > For GEN11 the sequence is more complex and will be handled in a
> > separated patch.
> >
> > BSpec: 49294
> > Cc: Imre Deak <imre.deak@xxxxxxxxx>
> > Cc: Cooper Chiou <cooper.chiou@xxxxxxxxx>
> > Cc: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
> > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_tc.c | 61
> > ++++++++++++++++++++++++-
> > drivers/gpu/drm/i915/i915_reg.h | 3 ++
> > drivers/gpu/drm/i915/intel_sideband.c | 22 +++++++++
> > drivers/gpu/drm/i915/intel_sideband.h | 4 ++
> > 4 files changed, 88 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> > b/drivers/gpu/drm/i915/display/intel_tc.c
> > index 9b850c11aa78..e4c5de5ce874 100644
> > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > @@ -7,6 +7,7 @@
> > #include "intel_display.h"
> > #include "intel_display_types.h"
> > #include "intel_dp_mst.h"
> > +#include "intel_sideband.h"
> > #include "intel_tc.h"
> >
> > static const char *tc_port_mode_name(enum tc_port_mode mode)
> > @@ -496,6 +497,55 @@ bool intel_tc_port_connected(struct
> > intel_digital_port *dig_port)
> > return is_connected;
> > }
> >
> > +static inline int tgl_tc_cold_request(struct intel_digital_port
> > *dig_port,
> > + bool block)
> > +{
> > + struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> > + u32 low_val, high_val;
> > + u8 tries = 0;
> > + int ret;
> > +
> > + do {
> > + low_val = 0;
> > + high_val = block ? 0 :
> > TGL_PCODE_EXIT_TCCOLD_DATA_H_UNBLOCK_REQ;
> > +
> > + ret = sandybridge_pcode_write_read_timeout(i915,
> > + TGL_PCODE_TC
> > COLD,
> > + &low_val,
> > &high_val,
> > + 150, 1);
>
> The spec says trying 3 times for 200 usec. Is there a problem if we
> just
> reuse sandybridge_pcode_read() here which has a 500 usec timeout?
Yeah I guess we can switch to sandybridge_pcode_read().
>
> > + if (ret == 0) {
> > + if (block &&
> > + low_val &
> > TGL_PCODE_EXIT_TCCOLD_DATA_L_EXIT_FAILED)
> > + ret = -EIO;
> > + else
> > + break;
> > + }
> > +
> > + if (ret != -EAGAIN)
> > + tries++;
> > + } while (tries < 3);
> > +
> > + return ret;
>
> The return value isn't used and I think we can't do much about it, so
> just make the function a void type and warn about a timeout?
The return is usefull to have just one warm message between ICL and
TGL.
>
> > +}
> > +
> > +static int tc_cold_request(struct intel_digital_port *dig_port,
> > bool block)
> > +{
> > + struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> > + int ret;
> > +
> > + if (INTEL_GEN(i915) >= 12)
> > + ret = tgl_tc_cold_request(dig_port, block);
> > + else
> > + /* TODO: implement GEN11 TCCOLD sequences */
> > + ret = 0;
> > +
> > + drm_dbg_kms(&i915->drm, "Port %s: TCCOLD %sblock %s\n",
> > + dig_port->tc_port_name, (block ? "" : "un"),
> > + (ret == 0 ? "succeeded" : "failed"));
> > +
> > + return ret;
> > +}
> > +
> > static void __intel_tc_port_lock(struct intel_digital_port
> > *dig_port,
> > int required_lanes)
> > {
> > @@ -506,9 +556,11 @@ static void __intel_tc_port_lock(struct
> > intel_digital_port *dig_port,
> >
> > mutex_lock(&dig_port->tc_lock);
> >
> > - if (!dig_port->tc_link_refcount &&
> > - intel_tc_port_needs_reset(dig_port))
> > + if (dig_port->tc_link_refcount == 0) {
> > + tc_cold_request(dig_port, true);
>
> I'm not sure if PCODE really refcounts the requests what this would
> need
> (since we submit the same request for all ports). But for other
Oh I complete missed the multiple ports handling.
> reasons,
> like the overhead of the request and the AUX PW ack trickery we need
> on
> ICL, I think we need a tc_cold_off power well/domain. The tc_cold_off
> power ref would be get/put around the FIA access sequence here
> (intel_tc_port_reset_mode()) and would be held whenever we hold an
> AUX
> power ref.
For TGL the tc_cold_off power well would work and would be pretty easy
to implement but for ICL I'm not sure.
For ICL, because of preemptions we need to get the aux power of the TC
port before request PCODE to exit TC cold.
So a single tc_cold_off would
need to depend into all aux's?
Even if we have one tc_cold_off per TC
DDI, if we make it depend into aux we would get aux power enable
timeouts. So we would need to enable aux power inside of the
tc_cold_off enable function and the aux enable call would need to not
check the HW status.
Thoughts?
>
> ICL legacy ports would hold an AUX power ref around the FIA access
> here
> and the AUX power well code would internally do the PCODE request and
> deal with the delayed power well ack (so we don't need to create a
> new
> interface for that).
>
> > + intel_tc_port_needs_reset(dig_port);
> > intel_tc_port_reset_mode(dig_port, required_lanes);
> > + }
> >
> > drm_WARN_ON(&i915->drm, dig_port->tc_lock_wakeref);
> > dig_port->tc_lock_wakeref = wakeref;
> > @@ -524,6 +576,9 @@ void intel_tc_port_unlock(struct
> > intel_digital_port *dig_port)
> > struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> > intel_wakeref_t wakeref = fetch_and_zero(&dig_port-
> > >tc_lock_wakeref);
> >
> > + if (dig_port->tc_link_refcount == 0)
> > + tc_cold_request(dig_port, false);
> > +
> > mutex_unlock(&dig_port->tc_lock);
> >
> > intel_display_power_put_async(i915, POWER_DOMAIN_DISPLAY_CORE,
> > @@ -548,6 +603,8 @@ void intel_tc_port_put_link(struct
> > intel_digital_port *dig_port)
> > {
> > mutex_lock(&dig_port->tc_lock);
> > dig_port->tc_link_refcount--;
> > + if (dig_port->tc_link_refcount == 0)
> > + tc_cold_request(dig_port, false);
> > mutex_unlock(&dig_port->tc_lock);
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 9c53fe918be6..7e341d9945b3 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -9019,6 +9019,9 @@ enum {
> > #define GEN6_PCODE_WRITE_D_COMP 0x11
> > #define HSW_PCODE_DE_WRITE_FREQ_REQ 0x17
> > #define DISPLAY_IPS_CONTROL 0x19
> > +#define TGL_PCODE_TCCOLD 0x26
> > +#define TGL_PCODE_EXIT_TCCOLD_DATA_L_EXIT_FAILED REG_BIT
> > (0)
> > +#define TGL_PCODE_EXIT_TCCOLD_DATA_H_UNBLOCK_REQ REG_BIT
> > (0)
> > /* See also IPS_CTL */
> > #define IPS_PCODE_CONTROL (1 << 30)
> > #define HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL 0x1A
> > diff --git a/drivers/gpu/drm/i915/intel_sideband.c
> > b/drivers/gpu/drm/i915/intel_sideband.c
> > index 1447e7516cb7..20a9d3970930 100644
> > --- a/drivers/gpu/drm/i915/intel_sideband.c
> > +++ b/drivers/gpu/drm/i915/intel_sideband.c
> > @@ -463,6 +463,28 @@ int sandybridge_pcode_write_timeout(struct
> > drm_i915_private *i915,
> > return err;
> > }
> >
> > +int sandybridge_pcode_write_read_timeout(struct drm_i915_private
> > *i915,
> > + u32 mbox, u32 *val, u32 *val1,
> > + int fast_timeout_us,
> > + int slow_timeout_ms)
> > +{
> > + int err;
> > +
> > + mutex_lock(&i915->sb_lock);
> > + err = __sandybridge_pcode_rw(i915, mbox, val, val1,
> > + fast_timeout_us, slow_timeout_ms,
> > + true);
> > + mutex_unlock(&i915->sb_lock);
> > +
> > + if (err) {
> > + drm_dbg(&i915->drm,
> > + "warning: pcode (write of 0x%08x to mbox %x)
> > mailbox access failed for %ps: %d\n",
> > + *val, mbox, __builtin_return_address(0), err);
> > + }
> > +
> > + return err;
> > +}
> > +
> > static bool skl_pcode_try_request(struct drm_i915_private *i915,
> > u32 mbox,
> > u32 request, u32 reply_mask, u32
> > reply,
> > u32 *status)
> > diff --git a/drivers/gpu/drm/i915/intel_sideband.h
> > b/drivers/gpu/drm/i915/intel_sideband.h
> > index 7fb95745a444..1939bebb4e67 100644
> > --- a/drivers/gpu/drm/i915/intel_sideband.h
> > +++ b/drivers/gpu/drm/i915/intel_sideband.h
> > @@ -132,6 +132,10 @@ int sandybridge_pcode_read(struct
> > drm_i915_private *i915, u32 mbox,
> > int sandybridge_pcode_write_timeout(struct drm_i915_private *i915,
> > u32 mbox,
> > u32 val, int fast_timeout_us,
> > int slow_timeout_ms);
> > +int sandybridge_pcode_write_read_timeout(struct drm_i915_private
> > *i915,
> > + u32 mbox, u32 *val, u32 *val1,
> > + int fast_timeout_us,
> > + int slow_timeout_ms);
> > #define sandybridge_pcode_write(i915, mbox, val) \
> > sandybridge_pcode_write_timeout(i915, mbox, val, 500, 0)
> >
> > --
> > 2.26.0
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
