> -----Original Message-----
> From: Deak, Imre <imre.deak@xxxxxxxxx>
> Sent: Tuesday, March 21, 2023 4:00 PM
> To: Kahola, Mika <mika.kahola@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 02/14] drm/i915/tc: Fix TC port link ref init for DP
> MST during HW readout
>
> 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).
Ok, this does make sense.
Reviewed-by: Mika Kahola <mika.kahola@xxxxxxxxx>
>
> 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
> >
