diff mbox series

[2/3] drm/i915/bigjoiner: Refactor bigjoiner state readout

Message ID 20240108120725.20057-3-stanislav.lisovskiy@intel.com (mailing list archive)
State New, archived
Headers show
Series Bigjoiner refactoring | expand

Commit Message

Stanislav Lisovskiy Jan. 8, 2024, 12:07 p.m. UTC 
Don't call enabled_bigjoiner_pipes twice, lets just move
intel_get_bigjoiner_config earlier, because it is anyway
calling same function.
Also cleanup hsw_enabled_transcoders from irrelevant bigjoiner code.

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 22 ++++++++++----------
 1 file changed, 11 insertions(+), 11 deletions(-)

Comments

Ville Syrjälä Jan. 12, 2024, 4:42 p.m. UTC | #1 
On Mon, Jan 08, 2024 at 02:07:24PM +0200, Stanislav Lisovskiy wrote:
> Don't call enabled_bigjoiner_pipes twice, lets just move
> intel_get_bigjoiner_config earlier, because it is anyway
> calling same function.
> Also cleanup hsw_enabled_transcoders from irrelevant bigjoiner code.

It's not irrelevant. The function is supposed to return the set of
enabled transcoders associated with the pipe. With this change the
function no longer does what it says on the tin.

> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 22 ++++++++++----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 927d124457b61..eec76ec167069 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -3525,7 +3525,6 @@ static u8 hsw_enabled_transcoders(struct intel_crtc *crtc)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	u8 panel_transcoder_mask = hsw_panel_transcoders(dev_priv);
>  	enum transcoder cpu_transcoder;
> -	u8 master_pipes, slave_pipes;
>  	u8 enabled_transcoders = 0;
>  
>  	/*
> @@ -3576,15 +3575,6 @@ static u8 hsw_enabled_transcoders(struct intel_crtc *crtc)
>  	if (transcoder_ddi_func_is_enabled(dev_priv, cpu_transcoder))
>  		enabled_transcoders |= BIT(cpu_transcoder);
>  
> -	/* bigjoiner slave -> consider the master pipe's transcoder as well */
> -	enabled_bigjoiner_pipes(dev_priv, &master_pipes, &slave_pipes);
> -	if (slave_pipes & BIT(crtc->pipe)) {
> -		cpu_transcoder = (enum transcoder)
> -			get_bigjoiner_master_pipe(crtc->pipe, master_pipes, slave_pipes);
> -		if (transcoder_ddi_func_is_enabled(dev_priv, cpu_transcoder))
> -			enabled_transcoders |= BIT(cpu_transcoder);
> -	}
> -
>  	return enabled_transcoders;
>  }
>  
> @@ -3631,6 +3621,15 @@ static bool hsw_get_transcoder_state(struct intel_crtc *crtc,
>  	u32 tmp;
>  
>  	enabled_transcoders = hsw_enabled_transcoders(crtc);
> +
> +	/* bigjoiner slave -> consider the master pipe's transcoder as well */
> +	if (intel_crtc_is_bigjoiner_slave(pipe_config)) {
> +		unsigned long cpu_transcoder = (enum transcoder)
> +			bigjoiner_master_pipe(pipe_config);
> +		if (transcoder_ddi_func_is_enabled(dev_priv, cpu_transcoder))
> +			enabled_transcoders |= BIT(cpu_transcoder);
> +	}
> +
>  	if (!enabled_transcoders)
>  		return false;
>  
> @@ -3735,6 +3734,8 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
>  
>  	pipe_config->shared_dpll = NULL;
>  
> +	intel_bigjoiner_get_config(pipe_config);

So this is what avoids the "call it twice" part, but it also makes the
state potentially inconsistent as in all other cases we leave everything
zeroed if the transcoder is not enabled. So I'm not sure this is
entirely safe or whether we could end up with some weird state
mismatches due to the inconsistency.

Why do you think calling it twice is problematic? It's supposed to be 
idempotent (ignoring the actual register reads/etc.).

> +
>  	active = hsw_get_transcoder_state(crtc, pipe_config, &crtc->hw_readout_power_domains);
>  
>  	if ((IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) &&
> @@ -3746,7 +3747,6 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
>  	if (!active)
>  		goto out;
>  
> -	intel_bigjoiner_get_config(pipe_config);
>  	intel_dsc_get_config(pipe_config);
>  
>  	if (!transcoder_is_dsi(pipe_config->cpu_transcoder) ||
> -- 
> 2.37.3
Stanislav Lisovskiy Jan. 15, 2024, 9:24 a.m. UTC | #2 
On Fri, Jan 12, 2024 at 06:42:15PM +0200, Ville Syrjälä wrote:
> On Mon, Jan 08, 2024 at 02:07:24PM +0200, Stanislav Lisovskiy wrote:
> > Don't call enabled_bigjoiner_pipes twice, lets just move
> > intel_get_bigjoiner_config earlier, because it is anyway
> > calling same function.
> > Also cleanup hsw_enabled_transcoders from irrelevant bigjoiner code.
> 
> It's not irrelevant. The function is supposed to return the set of
> enabled transcoders associated with the pipe. With this change the
> function no longer does what it says on the tin.

Yes, but I guess it is just a matter what we define to be higher in a
logical hierarchy: a pipe or a bigjoiner?
I thought it won't harm hsw_enabled_transcoders won't have any excess
logic and will return only transcoders naturally associated with a
physical pipe, while for higher complexity level constructs like bigjoiner
we would have some logic on top. 
In fact my main motivation was to avoid calling enabled_bigjoiner_pipe as 
it is quite heavy and call intel_crtc_is_bigjoiner_slave here instead. 

enabled_bigjoiner_pipes reads too much information, which 
we don't need in that function(here we just need to know if we are slave or master)

The absolute need for calling enabled_bigjoiner_pipes happens in 
intel_bigjoiner_get_config, which we moved earlier, which seems to be
logical since hsw_get_transcoder_state needs to have bigjoiner info and
now we can use its results to call more lightweight intel_crtc_is_bigjoiner_slave there
because pipe_config->bigjoiner_pipes is now initialized, so we avoid calling enabled_bigjoiner_pipes
second time..

> 
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 22 ++++++++++----------
> >  1 file changed, 11 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 927d124457b61..eec76ec167069 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -3525,7 +3525,6 @@ static u8 hsw_enabled_transcoders(struct intel_crtc *crtc)
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	u8 panel_transcoder_mask = hsw_panel_transcoders(dev_priv);
> >  	enum transcoder cpu_transcoder;
> > -	u8 master_pipes, slave_pipes;
> >  	u8 enabled_transcoders = 0;
> >  
> >  	/*
> > @@ -3576,15 +3575,6 @@ static u8 hsw_enabled_transcoders(struct intel_crtc *crtc)
> >  	if (transcoder_ddi_func_is_enabled(dev_priv, cpu_transcoder))
> >  		enabled_transcoders |= BIT(cpu_transcoder);
> >  
> > -	/* bigjoiner slave -> consider the master pipe's transcoder as well */
> > -	enabled_bigjoiner_pipes(dev_priv, &master_pipes, &slave_pipes);
> > -	if (slave_pipes & BIT(crtc->pipe)) {
> > -		cpu_transcoder = (enum transcoder)
> > -			get_bigjoiner_master_pipe(crtc->pipe, master_pipes, slave_pipes);
> > -		if (transcoder_ddi_func_is_enabled(dev_priv, cpu_transcoder))
> > -			enabled_transcoders |= BIT(cpu_transcoder);
> > -	}
> > -
> >  	return enabled_transcoders;
> >  }
> >  
> > @@ -3631,6 +3621,15 @@ static bool hsw_get_transcoder_state(struct intel_crtc *crtc,
> >  	u32 tmp;
> >  
> >  	enabled_transcoders = hsw_enabled_transcoders(crtc);
> > +
> > +	/* bigjoiner slave -> consider the master pipe's transcoder as well */
> > +	if (intel_crtc_is_bigjoiner_slave(pipe_config)) {
> > +		unsigned long cpu_transcoder = (enum transcoder)
> > +			bigjoiner_master_pipe(pipe_config);
> > +		if (transcoder_ddi_func_is_enabled(dev_priv, cpu_transcoder))
> > +			enabled_transcoders |= BIT(cpu_transcoder);
> > +	}
> > +
> >  	if (!enabled_transcoders)
> >  		return false;
> >  
> > @@ -3735,6 +3734,8 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
> >  
> >  	pipe_config->shared_dpll = NULL;
> >  
> > +	intel_bigjoiner_get_config(pipe_config);
> 
> So this is what avoids the "call it twice" part, but it also makes the
> state potentially inconsistent as in all other cases we leave everything
> zeroed if the transcoder is not enabled. So I'm not sure this is
> entirely safe or whether we could end up with some weird state
> mismatches due to the inconsistency.

Isn't it vice versa? intel_bigjoiner_get_config is now called way earlier,
before hsw_get_transcoder_state is called(previously it was called later),
the only difference is just that we now have pipe_config->bigjoiner_pipes
filled and enabled_bigjoiner_pipes was called there, so we can now
use that info to call intel_crtc_is_bigjoiner_slave in hsw_get_transcoder_state,
as I mentioned above.

Also if none of the transcoders are enabled, we now in fact have more information
filled than before this change(before we had only enabled_bigjoiner_pipes called
in hsw_get_transcoder_state, but now we have also pipe_config->bigjoiner_pipes
initialized), otherwise if none of the transcoders are active everything should
be pretty much the same.

Stan


> 
> Why do you think calling it twice is problematic? It's supposed to be 
> idempotent (ignoring the actual register reads/etc.).
> 
> > +
> >  	active = hsw_get_transcoder_state(crtc, pipe_config, &crtc->hw_readout_power_domains);
> >  
> >  	if ((IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) &&
> > @@ -3746,7 +3747,6 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
> >  	if (!active)
> >  		goto out;
> >  
> > -	intel_bigjoiner_get_config(pipe_config);
> >  	intel_dsc_get_config(pipe_config);
> >  
> >  	if (!transcoder_is_dsi(pipe_config->cpu_transcoder) ||
> > -- 
> > 2.37.3
> 
> -- 
> Ville Syrjälä
> Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 927d124457b61..eec76ec167069 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -3525,7 +3525,6 @@  static u8 hsw_enabled_transcoders(struct intel_crtc *crtc)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	u8 panel_transcoder_mask = hsw_panel_transcoders(dev_priv);
 	enum transcoder cpu_transcoder;
-	u8 master_pipes, slave_pipes;
 	u8 enabled_transcoders = 0;
 
 	/*
@@ -3576,15 +3575,6 @@  static u8 hsw_enabled_transcoders(struct intel_crtc *crtc)
 	if (transcoder_ddi_func_is_enabled(dev_priv, cpu_transcoder))
 		enabled_transcoders |= BIT(cpu_transcoder);
 
-	/* bigjoiner slave -> consider the master pipe's transcoder as well */
-	enabled_bigjoiner_pipes(dev_priv, &master_pipes, &slave_pipes);
-	if (slave_pipes & BIT(crtc->pipe)) {
-		cpu_transcoder = (enum transcoder)
-			get_bigjoiner_master_pipe(crtc->pipe, master_pipes, slave_pipes);
-		if (transcoder_ddi_func_is_enabled(dev_priv, cpu_transcoder))
-			enabled_transcoders |= BIT(cpu_transcoder);
-	}
-
 	return enabled_transcoders;
 }
 
@@ -3631,6 +3621,15 @@  static bool hsw_get_transcoder_state(struct intel_crtc *crtc,
 	u32 tmp;
 
 	enabled_transcoders = hsw_enabled_transcoders(crtc);
+
+	/* bigjoiner slave -> consider the master pipe's transcoder as well */
+	if (intel_crtc_is_bigjoiner_slave(pipe_config)) {
+		unsigned long cpu_transcoder = (enum transcoder)
+			bigjoiner_master_pipe(pipe_config);
+		if (transcoder_ddi_func_is_enabled(dev_priv, cpu_transcoder))
+			enabled_transcoders |= BIT(cpu_transcoder);
+	}
+
 	if (!enabled_transcoders)
 		return false;
 
@@ -3735,6 +3734,8 @@  static bool hsw_get_pipe_config(struct intel_crtc *crtc,
 
 	pipe_config->shared_dpll = NULL;
 
+	intel_bigjoiner_get_config(pipe_config);
+
 	active = hsw_get_transcoder_state(crtc, pipe_config, &crtc->hw_readout_power_domains);
 
 	if ((IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) &&
@@ -3746,7 +3747,6 @@  static bool hsw_get_pipe_config(struct intel_crtc *crtc,
 	if (!active)
 		goto out;
 
-	intel_bigjoiner_get_config(pipe_config);
 	intel_dsc_get_config(pipe_config);
 
 	if (!transcoder_is_dsi(pipe_config->cpu_transcoder) ||