Re: [Intel-gfx] [PATCH 02/14] drm/i915/tc: Fix TC port link ref init for DP MST during HW readout

From: "Kahola, Mika" <mika.kahola@intel.com>
To: "Deak, Imre" <imre.deak@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH 02/14] drm/i915/tc: Fix TC port link ref init for DP MST during HW readout
Date: Tue, 21 Mar 2023 12:06:38 +0000	[thread overview]
Message-ID: <MW4PR11MB7054FE3F9F2904F694633EEEEF819@MW4PR11MB7054.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20230316131724.359612-3-imre.deak@intel.com>

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Imre
> Deak
> Sent: Thursday, March 16, 2023 3:17 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [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@intel.com>
> ---
>  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?

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