On Tue, Mar 21, 2023 at 02:06:38PM +0200, Kahola, Mika wrote:
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Imre
> > Deak
> > Sent: Thursday, March 16, 2023 3:17 PM
> > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Subject: [PATCH 02/14] drm/i915/tc: Fix TC port link ref init for DP
> > MST during HW readout
> >
> > An enabled TC MST port holds one TC port link reference, regardless of the
> > number of enabled streams on it, but the TC port HW readout takes one
> > reference for each active MST stream.
> >
> > Fix the HW readout, taking only one reference for MST ports.
> >
> > This didn't cause an actual problem, since the encoder HW readout doesn't yet
> > support reading out the MST HW state.
> >
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_tc.c | 20 +++++++++++---------
> > 1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> > b/drivers/gpu/drm/i915/display/intel_tc.c
> > index 050f998284592..0b6fe96ab4759 100644
> > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > @@ -660,11 +660,14 @@ static void intel_tc_port_update_mode(struct
> > intel_digital_port *dig_port,
> > tc_cold_unblock(dig_port, domain, wref); }
> >
> > -static void
> > -intel_tc_port_link_init_refcount(struct intel_digital_port *dig_port,
> > - int refcount)
> > +static void __intel_tc_port_get_link(struct intel_digital_port
> > +*dig_port)
> > {
> > - dig_port->tc_link_refcount = refcount;
> > + dig_port->tc_link_refcount++;
> > +}
> > +
> > +static void __intel_tc_port_put_link(struct intel_digital_port
> > +*dig_port) {
> > + dig_port->tc_link_refcount--;
> > }
>
> When I read this first time, I had an impression that *_put_link() and
> *_get_link() would do something for the mst streams. However, these
> get/put just increases or decreases the link refcount. Should we
> rename these functions to restore the "refcount" to the function name
> as the replaced function had?
A link ref is taken whenever the port's TC mode should stay unchanged.
This may be because the port is enabled in any mode (DP-SST, -MST or HDMI)
or as here not necessarilty enabled, but not fully initialized yet
(which is done only once intel_tc_port_sanitize_mode() is called).
Based on the above get/put_link here means the same thing as later when
enabling/disabling outputs; hence I added the above functions used in
both cases.
> Otherwise, the patch does what is supposed to do here and looks ok.
>
> -Mika-
>
> >
> > /**
> > @@ -690,7 +693,7 @@ void intel_tc_port_init_mode(struct intel_digital_port
> > *dig_port)
> >
> > dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
> > /* Prevent changing dig_port->tc_mode until
> > intel_tc_port_sanitize_mode() is called. */
> > - intel_tc_port_link_init_refcount(dig_port, 1);
> > + __intel_tc_port_get_link(dig_port);
> > dig_port->tc_lock_wakeref = tc_cold_block(dig_port, &dig_port-
> > >tc_lock_power_domain);
> >
> > tc_cold_unblock(dig_port, domain, tc_cold_wref); @@ -726,8 +729,6
> > @@ void intel_tc_port_sanitize_mode(struct intel_digital_port *dig_port)
> > active_links = to_intel_crtc(encoder->base.crtc)->active;
> >
> > drm_WARN_ON(&i915->drm, dig_port->tc_link_refcount != 1);
> > - intel_tc_port_link_init_refcount(dig_port, active_links);
> > -
> > if (active_links) {
> > if (!icl_tc_phy_is_connected(dig_port))
> > drm_dbg_kms(&i915->drm,
> > @@ -746,6 +747,7 @@ void intel_tc_port_sanitize_mode(struct
> > intel_digital_port *dig_port)
> > dig_port->tc_port_name,
> > tc_port_mode_name(dig_port->tc_mode));
> > icl_tc_phy_disconnect(dig_port);
> > + __intel_tc_port_put_link(dig_port);
> >
> > tc_cold_unblock(dig_port, dig_port->tc_lock_power_domain,
> > fetch_and_zero(&dig_port->tc_lock_wakeref));
> > @@ -864,14 +866,14 @@ void intel_tc_port_get_link(struct intel_digital_port
> > *dig_port,
> > int required_lanes)
> > {
> > __intel_tc_port_lock(dig_port, required_lanes);
> > - dig_port->tc_link_refcount++;
> > + __intel_tc_port_get_link(dig_port);
> > intel_tc_port_unlock(dig_port);
> > }
> >
> > void intel_tc_port_put_link(struct intel_digital_port *dig_port) {
> > intel_tc_port_lock(dig_port);
> > - --dig_port->tc_link_refcount;
> > + __intel_tc_port_put_link(dig_port);
> > intel_tc_port_unlock(dig_port);
> >
> > /*
> > --
> > 2.37.1
>
