Re: [Intel-gfx] [PATCH 2/3] drm/i915/pps: Enable 2nd pps for dual EDP scenario

[Jani Nikula <jani.nikula@intel.com>]

On Wed, 09 Nov 2022, Animesh Manna <animesh.manna@intel.com> wrote:
> From display gen12 onwards to support dual EDP two instances of pps added.
> Currently backlight controller and pps instance can be mapped together
> for a specific panel. Currently dual PPS support is broken. This patch
> fixes it and enables for display 12+.
>
> v1: Iniital revision.
> v2: Called intel_bios_panel_init w/o PNPID before intel_pps_init. [Jani]
> v3: Set pps_id to -1 for pnpid type of panel which will be used by
> bxt_power_sequencer_idx() to set 2nd pps instance as default for
> 2nd EDP panel. [Jani]
> v4: Early return for PANEL_TYPE_FALLBACK. [Jani]
> v5: Removed additional pps_id variable and reused backlight
> controller. [Jani]
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bios.c |  9 ++++++++-
>  drivers/gpu/drm/i915/display/intel_bios.h |  2 +-
>  drivers/gpu/drm/i915/display/intel_dp.c   | 10 +++++++---
>  drivers/gpu/drm/i915/display/intel_pps.c  | 12 +++++++++++-
>  4 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index c2987f2c2b2e..1c1eea061fbb 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -3183,7 +3183,7 @@ void intel_bios_init(struct drm_i915_private *i915)
>  	kfree(oprom_vbt);
>  }
>  
> -void intel_bios_init_panel(struct drm_i915_private *i915,
> +bool intel_bios_init_panel(struct drm_i915_private *i915,
>  			   struct intel_panel *panel,
>  			   const struct intel_bios_encoder_data *devdata,
>  			   const struct edid *edid)
> @@ -3192,6 +3192,11 @@ void intel_bios_init_panel(struct drm_i915_private *i915,
>  
>  	panel->vbt.panel_type = get_panel_type(i915, devdata, edid);
>  
> +	if (panel->vbt.panel_type == PANEL_TYPE_FALLBACK && !edid) {

As Ville pointed out, "panel->vbt.panel_type == PANEL_TYPE_FALLBACK" is
comparing apples to oranges, and we hit this path only in the rare case
that panel_type == 3, regardless of where the panel type originated.

This is probably the reason patches 1&2 don't fix the original issue
like it should.

I remind you that the issue is two eDP having their own pps software
states but using the same pps hardware due to conflicting pps index
(which is the same as panel->vbt.backlight.controller). This is what we
need to solve here.

We get the wakeref imbalance because of two eDP trampling on the same
pps hardware. Patch 3 does nothing to address that issue, it just tries
to hide the problem.


BR,
Jani.


> +		panel->vbt.backlight.controller = -1;
> +		return true;
> +	}
> +
>  	parse_panel_options(i915, panel);
>  	parse_generic_dtd(i915, panel);
>  	parse_lfp_data(i915, panel);
> @@ -3203,6 +3208,8 @@ void intel_bios_init_panel(struct drm_i915_private *i915,
>  	parse_psr(i915, panel);
>  	parse_mipi_config(i915, panel);
>  	parse_mipi_sequence(i915, panel);
> +
> +	return false;
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.h b/drivers/gpu/drm/i915/display/intel_bios.h
> index e375405a7828..f8ef0274f3ee 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.h
> +++ b/drivers/gpu/drm/i915/display/intel_bios.h
> @@ -232,7 +232,7 @@ struct mipi_pps_data {
>  } __packed;
>  
>  void intel_bios_init(struct drm_i915_private *dev_priv);
> -void intel_bios_init_panel(struct drm_i915_private *dev_priv,
> +bool intel_bios_init_panel(struct drm_i915_private *dev_priv,
>  			   struct intel_panel *panel,
>  			   const struct intel_bios_encoder_data *devdata,
>  			   const struct edid *edid);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 7400d6b4c587..78cf3a77f026 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5235,6 +5235,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
>  	bool has_dpcd;
>  	struct edid *edid;
> +	bool retry;
>  
>  	if (!intel_dp_is_edp(intel_dp))
>  		return true;
> @@ -5254,6 +5255,9 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  		return false;
>  	}
>  
> +	retry = intel_bios_init_panel(dev_priv, &intel_connector->panel,
> +				      encoder->devdata, NULL);
> +
>  	intel_pps_init(intel_dp);
>  
>  	/* Cache DPCD and EDID for edp. */
> @@ -5288,9 +5292,9 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  		edid = ERR_PTR(-ENOENT);
>  	}
>  	intel_connector->edid = edid;
> -
> -	intel_bios_init_panel(dev_priv, &intel_connector->panel,
> -			      encoder->devdata, IS_ERR(edid) ? NULL : edid);
> +	if (retry)
> +		intel_bios_init_panel(dev_priv, &intel_connector->panel,
> +				      encoder->devdata, IS_ERR(edid) ? NULL : edid);
>  
>  	intel_panel_add_edid_fixed_modes(intel_connector, true);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
> index 3949fb449353..5738af154bd2 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -219,6 +219,16 @@ bxt_power_sequencer_idx(struct intel_dp *intel_dp)
>  	/* We should never land here with regular DP ports */
>  	drm_WARN_ON(&dev_priv->drm, !intel_dp_is_edp(intel_dp));
>  
> +	if (backlight_controller == -1) {
> +		/*
> +		 * Use 2nd PPS instance as default for 2nd EDP panel.
> +		 */
> +		if (connector->encoder->port == PORT_A)
> +			return 0;
> +		else
> +			return 1;
> +	}
> +
>  	if (!intel_dp->pps.pps_reset)
>  		return backlight_controller;
>  
> @@ -1430,7 +1440,7 @@ void intel_pps_init(struct intel_dp *intel_dp)
>  	intel_dp->pps.initializing = true;
>  	INIT_DELAYED_WORK(&intel_dp->pps.panel_vdd_work, edp_panel_vdd_work);
>  
> -	if (IS_GEMINILAKE(i915) || IS_BROXTON(i915))
> +	if (IS_GEMINILAKE(i915) || IS_BROXTON(i915) || DISPLAY_VER(i915) >= 12)
>  		intel_dp->get_pps_idx = bxt_power_sequencer_idx;
>  	else if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
>  		intel_dp->get_pps_idx = vlv_power_sequencer_pipe;

-- 
Jani Nikula, Intel Open Source Graphics Center