All of lore.kernel.org
 help / color / mirror / Atom feed
* [Intel-gfx] [PATCH] drm/i915/pps: added get_pps_idx() hook as part of pps_get_register() cleanup
@ 2022-08-03  5:29 Animesh Manna
  2022-08-03  6:32 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork
  2022-08-03  8:13 ` [Intel-gfx] [PATCH] " Jani Nikula
  0 siblings, 2 replies; 7+ messages in thread
From: Animesh Manna @ 2022-08-03  5:29 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula

To support dual LFP two instances of pps added from display gen12 onwards.
Few older platform like VLV also has dual pps support but handling is
different. So added separate hook get_pps_idx() to formulate which pps
instance to used for a soecific LFP on a specific platform.

Simplified pps_get_register() which use get_pps_idx() hook to derive the
pps instance and get_pps_idx() will be initialized at pps_init().

Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bios.c     |  5 ++++
 drivers/gpu/drm/i915/display/intel_bios.h     |  1 +
 .../drm/i915/display/intel_display_types.h    |  2 ++
 drivers/gpu/drm/i915/display/intel_pps.c      | 25 ++++++++++++++-----
 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 51dde5bfd956..42315615a728 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -611,6 +611,11 @@ static int opregion_get_panel_type(struct drm_i915_private *i915,
 	return intel_opregion_get_panel_type(i915);
 }
 
+bool intel_bios_is_lfp2(struct intel_encoder *encoder)
+{
+	return encoder->devdata && encoder->devdata->child.handle == DEVICE_HANDLE_LFP2;
+}
+
 static int vbt_get_panel_type(struct drm_i915_private *i915,
 			      const struct intel_bios_encoder_data *devdata,
 			      const struct edid *edid)
diff --git a/drivers/gpu/drm/i915/display/intel_bios.h b/drivers/gpu/drm/i915/display/intel_bios.h
index e47582b0de0a..aea72a87ea2c 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.h
+++ b/drivers/gpu/drm/i915/display/intel_bios.h
@@ -251,6 +251,7 @@ bool intel_bios_is_lspcon_present(const struct drm_i915_private *i915,
 				  enum port port);
 bool intel_bios_is_lane_reversal_needed(const struct drm_i915_private *i915,
 					enum port port);
+bool intel_bios_is_lfp2(struct intel_encoder *encoder);
 enum aux_ch intel_bios_port_aux_ch(struct drm_i915_private *dev_priv, enum port port);
 bool intel_bios_get_dsc_params(struct intel_encoder *encoder,
 			       struct intel_crtc_state *crtc_state,
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 0da9b208d56e..95f71a572b07 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1723,6 +1723,8 @@ struct intel_dp {
 
 	/* When we last wrote the OUI for eDP */
 	unsigned long last_oui_write;
+
+	int (*get_pps_idx)(struct intel_dp *intel_dp);
 };
 
 enum lspcon_vendor {
diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
index 1b21a341962f..c9cdb302d318 100644
--- a/drivers/gpu/drm/i915/display/intel_pps.c
+++ b/drivers/gpu/drm/i915/display/intel_pps.c
@@ -231,6 +231,17 @@ bxt_power_sequencer_idx(struct intel_dp *intel_dp)
 	return backlight_controller;
 }
 
+static int
+gen12_power_sequencer_idx(struct intel_dp *intel_dp)
+{
+	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
+
+	if (intel_bios_is_lfp2(encoder))
+		return 1;
+
+	return 0;
+}
+
 typedef bool (*vlv_pipe_check)(struct drm_i915_private *dev_priv,
 			       enum pipe pipe);
 
@@ -361,15 +372,10 @@ static void intel_pps_get_registers(struct intel_dp *intel_dp,
 				    struct pps_registers *regs)
 {
 	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
-	int pps_idx = 0;
+	int pps_idx = intel_dp->get_pps_idx(intel_dp);
 
 	memset(regs, 0, sizeof(*regs));
 
-	if (IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv))
-		pps_idx = bxt_power_sequencer_idx(intel_dp);
-	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
-		pps_idx = vlv_power_sequencer_pipe(intel_dp);
-
 	regs->pp_ctrl = PP_CONTROL(pps_idx);
 	regs->pp_stat = PP_STATUS(pps_idx);
 	regs->pp_on = PP_ON_DELAYS(pps_idx);
@@ -1431,6 +1437,13 @@ 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))
+		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;
+	else if (DISPLAY_VER(i915) >= 12)
+		intel_dp->get_pps_idx = gen12_power_sequencer_idx;
+
 	pps_init_timestamps(intel_dp);
 
 	with_intel_pps_lock(intel_dp, wakeref) {
-- 
2.29.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/pps: added get_pps_idx() hook as part of pps_get_register() cleanup
  2022-08-03  5:29 [Intel-gfx] [PATCH] drm/i915/pps: added get_pps_idx() hook as part of pps_get_register() cleanup Animesh Manna
@ 2022-08-03  6:32 ` Patchwork
  2022-08-03  8:13 ` [Intel-gfx] [PATCH] " Jani Nikula
  1 sibling, 0 replies; 7+ messages in thread
From: Patchwork @ 2022-08-03  6:32 UTC (permalink / raw)
  To: Animesh Manna; +Cc: intel-gfx

[-- Attachment #1: Type: text/plain, Size: 8405 bytes --]

== Series Details ==

Series: drm/i915/pps: added get_pps_idx() hook as part of pps_get_register() cleanup
URL   : https://patchwork.freedesktop.org/series/106922/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_11964 -> Patchwork_106922v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_106922v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_106922v1, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/index.html

Participating hosts (45 -> 43)
------------------------------

  Additional (1): fi-kbl-soraka 
  Missing    (3): fi-ctg-p8600 fi-bdw-samus fi-hsw-4200u 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_106922v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@load:
    - fi-kbl-7567u:       [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11964/fi-kbl-7567u/igt@i915_module_load@load.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/fi-kbl-7567u/igt@i915_module_load@load.html
    - fi-kbl-8809g:       [PASS][3] -> [INCOMPLETE][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11964/fi-kbl-8809g/igt@i915_module_load@load.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/fi-kbl-8809g/igt@i915_module_load@load.html
    - fi-kbl-x1275:       [PASS][5] -> [INCOMPLETE][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11964/fi-kbl-x1275/igt@i915_module_load@load.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/fi-kbl-x1275/igt@i915_module_load@load.html
    - fi-skl-6600u:       [PASS][7] -> [INCOMPLETE][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11964/fi-skl-6600u/igt@i915_module_load@load.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/fi-skl-6600u/igt@i915_module_load@load.html
    - fi-kbl-soraka:      NOTRUN -> [INCOMPLETE][9]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/fi-kbl-soraka/igt@i915_module_load@load.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_module_load@load:
    - {bat-jsl-1}:        [PASS][10] -> [INCOMPLETE][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11964/bat-jsl-1/igt@i915_module_load@load.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/bat-jsl-1/igt@i915_module_load@load.html
    - {fi-jsl-1}:         [PASS][12] -> [INCOMPLETE][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11964/fi-jsl-1/igt@i915_module_load@load.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/fi-jsl-1/igt@i915_module_load@load.html
    - {fi-ehl-2}:         [PASS][14] -> [INCOMPLETE][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11964/fi-ehl-2/igt@i915_module_load@load.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/fi-ehl-2/igt@i915_module_load@load.html

  
Known issues
------------

  Here are the changes found in Patchwork_106922v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@hangcheck:
    - bat-dg1-6:          [PASS][16] -> [DMESG-FAIL][17] ([i915#4494] / [i915#4957])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11964/bat-dg1-6/igt@i915_selftest@live@hangcheck.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/bat-dg1-6/igt@i915_selftest@live@hangcheck.html

  * igt@i915_selftest@live@hugepages:
    - bat-adlp-4:         [PASS][18] -> [DMESG-WARN][19] ([i915#1888])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11964/bat-adlp-4/igt@i915_selftest@live@hugepages.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/bat-adlp-4/igt@i915_selftest@live@hugepages.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size:
    - fi-bsw-kefka:       [PASS][20] -> [FAIL][21] ([i915#6298])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11964/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size.html

  * igt@runner@aborted:
    - fi-kbl-7567u:       NOTRUN -> [FAIL][22] ([i915#4312] / [i915#6219])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/fi-kbl-7567u/igt@runner@aborted.html
    - fi-kbl-x1275:       NOTRUN -> [FAIL][23] ([i915#4312] / [i915#6219])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/fi-kbl-x1275/igt@runner@aborted.html
    - fi-kbl-8809g:       NOTRUN -> [FAIL][24] ([i915#4312] / [i915#6219])
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/fi-kbl-8809g/igt@runner@aborted.html
    - fi-skl-6600u:       NOTRUN -> [FAIL][25] ([i915#4312])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/fi-skl-6600u/igt@runner@aborted.html
    - fi-kbl-soraka:      NOTRUN -> [FAIL][26] ([i915#4312] / [i915#6219])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/fi-kbl-soraka/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@fbdev@read:
    - {bat-rpls-2}:       [SKIP][27] ([i915#2582]) -> [PASS][28] +4 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11964/bat-rpls-2/igt@fbdev@read.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/bat-rpls-2/igt@fbdev@read.html

  * igt@gem_exec_suspend@basic-s3@smem:
    - {bat-adlm-1}:       [DMESG-WARN][29] ([i915#2867]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11964/bat-adlm-1/igt@gem_exec_suspend@basic-s3@smem.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/bat-adlm-1/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@gem_ringfill@basic-all:
    - {bat-dg2-8}:        [FAIL][31] ([i915#5886]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11964/bat-dg2-8/igt@gem_ringfill@basic-all.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/bat-dg2-8/igt@gem_ringfill@basic-all.html

  * igt@i915_selftest@live@hugepages:
    - {bat-adlm-1}:       [DMESG-WARN][33] -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11964/bat-adlm-1/igt@i915_selftest@live@hugepages.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/bat-adlm-1/igt@i915_selftest@live@hugepages.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#1888]: https://gitlab.freedesktop.org/drm/intel/issues/1888
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#2867]: https://gitlab.freedesktop.org/drm/intel/issues/2867
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4494]: https://gitlab.freedesktop.org/drm/intel/issues/4494
  [i915#4957]: https://gitlab.freedesktop.org/drm/intel/issues/4957
  [i915#5270]: https://gitlab.freedesktop.org/drm/intel/issues/5270
  [i915#5886]: https://gitlab.freedesktop.org/drm/intel/issues/5886
  [i915#6219]: https://gitlab.freedesktop.org/drm/intel/issues/6219
  [i915#6298]: https://gitlab.freedesktop.org/drm/intel/issues/6298


Build changes
-------------

  * Linux: CI_DRM_11964 -> Patchwork_106922v1

  CI-20190529: 20190529
  CI_DRM_11964: 856e330c2d6aa43e94bfca2d1b09369575f8f498 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6610: c93f93dfe91a77e6a884f826d61f927106988b5f @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_106922v1: 856e330c2d6aa43e94bfca2d1b09369575f8f498 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

e30840c8bccb drm/i915/pps: added get_pps_idx() hook as part of pps_get_register() cleanup

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106922v1/index.html

[-- Attachment #2: Type: text/html, Size: 9857 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/pps: added get_pps_idx() hook as part of pps_get_register() cleanup
  2022-08-03  5:29 [Intel-gfx] [PATCH] drm/i915/pps: added get_pps_idx() hook as part of pps_get_register() cleanup Animesh Manna
  2022-08-03  6:32 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork
@ 2022-08-03  8:13 ` Jani Nikula
  2022-08-05  8:11   ` Manna, Animesh
                     ` (2 more replies)
  1 sibling, 3 replies; 7+ messages in thread
From: Jani Nikula @ 2022-08-03  8:13 UTC (permalink / raw)
  To: Animesh Manna, intel-gfx

On Wed, 03 Aug 2022, Animesh Manna <animesh.manna@intel.com> wrote:
> To support dual LFP two instances of pps added from display gen12 onwards.
> Few older platform like VLV also has dual pps support but handling is
> different. So added separate hook get_pps_idx() to formulate which pps
> instance to used for a soecific LFP on a specific platform.
>
> Simplified pps_get_register() which use get_pps_idx() hook to derive the
> pps instance and get_pps_idx() will be initialized at pps_init().
>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bios.c     |  5 ++++
>  drivers/gpu/drm/i915/display/intel_bios.h     |  1 +
>  .../drm/i915/display/intel_display_types.h    |  2 ++
>  drivers/gpu/drm/i915/display/intel_pps.c      | 25 ++++++++++++++-----
>  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 51dde5bfd956..42315615a728 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -611,6 +611,11 @@ static int opregion_get_panel_type(struct drm_i915_private *i915,
>  	return intel_opregion_get_panel_type(i915);
>  }
>  
> +bool intel_bios_is_lfp2(struct intel_encoder *encoder)
> +{
> +	return encoder->devdata && encoder->devdata->child.handle == DEVICE_HANDLE_LFP2;
> +}

AFAICS the encoder never really needs to know whether it's "lfp1" or
"lfp2". It needs to know the pps controller number.

> +
>  static int vbt_get_panel_type(struct drm_i915_private *i915,
>  			      const struct intel_bios_encoder_data *devdata,
>  			      const struct edid *edid)
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.h b/drivers/gpu/drm/i915/display/intel_bios.h
> index e47582b0de0a..aea72a87ea2c 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.h
> +++ b/drivers/gpu/drm/i915/display/intel_bios.h
> @@ -251,6 +251,7 @@ bool intel_bios_is_lspcon_present(const struct drm_i915_private *i915,
>  				  enum port port);
>  bool intel_bios_is_lane_reversal_needed(const struct drm_i915_private *i915,
>  					enum port port);
> +bool intel_bios_is_lfp2(struct intel_encoder *encoder);
>  enum aux_ch intel_bios_port_aux_ch(struct drm_i915_private *dev_priv, enum port port);
>  bool intel_bios_get_dsc_params(struct intel_encoder *encoder,
>  			       struct intel_crtc_state *crtc_state,
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 0da9b208d56e..95f71a572b07 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1723,6 +1723,8 @@ struct intel_dp {
>  
>  	/* When we last wrote the OUI for eDP */
>  	unsigned long last_oui_write;
> +
> +	int (*get_pps_idx)(struct intel_dp *intel_dp);

Making this a function pointer should be a separate step. Please don't
try to do too many things at once. Rule of thumb, one change per patch.

Also, this should be placed near the other function pointer members in
struct intel_dp.

>  };
>  
>  enum lspcon_vendor {
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
> index 1b21a341962f..c9cdb302d318 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -231,6 +231,17 @@ bxt_power_sequencer_idx(struct intel_dp *intel_dp)
>  	return backlight_controller;
>  }
>  
> +static int
> +gen12_power_sequencer_idx(struct intel_dp *intel_dp)
> +{
> +	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> +
> +	if (intel_bios_is_lfp2(encoder))
> +		return 1;

This is actually not how this works. The bxt_power_sequencer_idx()
matches bspec 20149: "PWM and PPS are assumed to be aligned to be from
same block and not mismatch" i.e. the backlight controller id and the
pps id are the same. There are no requirements to map lfp# and pps
controller like this, even if it might be true in the common case.

The problem is we need the information *before* we call
intel_bios_init_panel().

It's a catch-22. We need the pps id to read the panel EDID, and we need
the panel EDID to choose the correct panel type in VBT, which we need to
get the pps id from the VBT.

This is highlighted in [1]. The 2nd eDP (which is not even physically
present, only in the VBT, *sigh*) screws up the PPS for the 1st during
init.

I think Ville had some ideas here.


BR,
Jani.


[1] https://gitlab.freedesktop.org/drm/intel/-/issues/5531


> +
> +	return 0;
> +}
> +
>  typedef bool (*vlv_pipe_check)(struct drm_i915_private *dev_priv,
>  			       enum pipe pipe);
>  
> @@ -361,15 +372,10 @@ static void intel_pps_get_registers(struct intel_dp *intel_dp,
>  				    struct pps_registers *regs)
>  {
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> -	int pps_idx = 0;
> +	int pps_idx = intel_dp->get_pps_idx(intel_dp);
>  
>  	memset(regs, 0, sizeof(*regs));
>  
> -	if (IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv))
> -		pps_idx = bxt_power_sequencer_idx(intel_dp);
> -	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -		pps_idx = vlv_power_sequencer_pipe(intel_dp);
> -
>  	regs->pp_ctrl = PP_CONTROL(pps_idx);
>  	regs->pp_stat = PP_STATUS(pps_idx);
>  	regs->pp_on = PP_ON_DELAYS(pps_idx);
> @@ -1431,6 +1437,13 @@ 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))
> +		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;
> +	else if (DISPLAY_VER(i915) >= 12)
> +		intel_dp->get_pps_idx = gen12_power_sequencer_idx;
> +
>  	pps_init_timestamps(intel_dp);
>  
>  	with_intel_pps_lock(intel_dp, wakeref) {

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/pps: added get_pps_idx() hook as part of pps_get_register() cleanup
  2022-08-03  8:13 ` [Intel-gfx] [PATCH] " Jani Nikula
@ 2022-08-05  8:11   ` Manna, Animesh
  2022-08-19  9:46   ` Manna, Animesh
  2022-08-23 13:27   ` Ville Syrjälä
  2 siblings, 0 replies; 7+ messages in thread
From: Manna, Animesh @ 2022-08-05  8:11 UTC (permalink / raw)
  To: Nikula, Jani, intel-gfx



> -----Original Message-----
> From: Nikula, Jani <jani.nikula@intel.com>
> Sent: Wednesday, August 3, 2022 1:44 PM
> To: Manna, Animesh <animesh.manna@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: ville.syrjala@linux.intel.com; Shankar, Uma <uma.shankar@intel.com>;
> Manna, Animesh <animesh.manna@intel.com>
> Subject: Re: [PATCH] drm/i915/pps: added get_pps_idx() hook as part of
> pps_get_register() cleanup
> 
> On Wed, 03 Aug 2022, Animesh Manna <animesh.manna@intel.com> wrote:
> > To support dual LFP two instances of pps added from display gen12 onwards.
> > Few older platform like VLV also has dual pps support but handling is
> > different. So added separate hook get_pps_idx() to formulate which pps
> > instance to used for a soecific LFP on a specific platform.
> >
> > Simplified pps_get_register() which use get_pps_idx() hook to derive
> > the pps instance and get_pps_idx() will be initialized at pps_init().
> >
> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bios.c     |  5 ++++
> >  drivers/gpu/drm/i915/display/intel_bios.h     |  1 +
> >  .../drm/i915/display/intel_display_types.h    |  2 ++
> >  drivers/gpu/drm/i915/display/intel_pps.c      | 25 ++++++++++++++-----
> >  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 51dde5bfd956..42315615a728 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > @@ -611,6 +611,11 @@ static int opregion_get_panel_type(struct
> drm_i915_private *i915,
> >  	return intel_opregion_get_panel_type(i915);
> >  }
> >
> > +bool intel_bios_is_lfp2(struct intel_encoder *encoder) {
> > +	return encoder->devdata && encoder->devdata->child.handle ==
> > +DEVICE_HANDLE_LFP2; }
> 
> AFAICS the encoder never really needs to know whether it's "lfp1" or "lfp2". It
> needs to know the pps controller number.
> 
> > +
> >  static int vbt_get_panel_type(struct drm_i915_private *i915,
> >  			      const struct intel_bios_encoder_data *devdata,
> >  			      const struct edid *edid)
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.h
> > b/drivers/gpu/drm/i915/display/intel_bios.h
> > index e47582b0de0a..aea72a87ea2c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.h
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.h
> > @@ -251,6 +251,7 @@ bool intel_bios_is_lspcon_present(const struct
> drm_i915_private *i915,
> >  				  enum port port);
> >  bool intel_bios_is_lane_reversal_needed(const struct drm_i915_private *i915,
> >  					enum port port);
> > +bool intel_bios_is_lfp2(struct intel_encoder *encoder);
> >  enum aux_ch intel_bios_port_aux_ch(struct drm_i915_private *dev_priv,
> > enum port port);  bool intel_bios_get_dsc_params(struct intel_encoder
> *encoder,
> >  			       struct intel_crtc_state *crtc_state, diff --git
> > a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 0da9b208d56e..95f71a572b07 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1723,6 +1723,8 @@ struct intel_dp {
> >
> >  	/* When we last wrote the OUI for eDP */
> >  	unsigned long last_oui_write;
> > +
> > +	int (*get_pps_idx)(struct intel_dp *intel_dp);
> 
> Making this a function pointer should be a separate step. Please don't try to do
> too many things at once. Rule of thumb, one change per patch.
> 
> Also, this should be placed near the other function pointer members in struct
> intel_dp.
> 
> >  };
> >
> >  enum lspcon_vendor {
> > diff --git a/drivers/gpu/drm/i915/display/intel_pps.c
> > b/drivers/gpu/drm/i915/display/intel_pps.c
> > index 1b21a341962f..c9cdb302d318 100644
> > --- a/drivers/gpu/drm/i915/display/intel_pps.c
> > +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> > @@ -231,6 +231,17 @@ bxt_power_sequencer_idx(struct intel_dp *intel_dp)
> >  	return backlight_controller;
> >  }
> >
> > +static int
> > +gen12_power_sequencer_idx(struct intel_dp *intel_dp) {
> > +	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > +
> > +	if (intel_bios_is_lfp2(encoder))
> > +		return 1;
> 
> This is actually not how this works. The bxt_power_sequencer_idx() matches
> bspec 20149: "PWM and PPS are assumed to be aligned to be from same block
> and not mismatch" i.e. the backlight controller id and the pps id are the same.
> There are no requirements to map lfp# and pps controller like this, even if it
> might be true in the common case.
> 
> The problem is we need the information *before* we call
> intel_bios_init_panel().
> 
> It's a catch-22. We need the pps id to read the panel EDID, and we need the
> panel EDID to choose the correct panel type in VBT, which we need to get the
> pps id from the VBT.
> 
> This is highlighted in [1]. The 2nd eDP (which is not even physically present, only
> in the VBT, *sigh*) screws up the PPS for the 1st during init.
> 
> I think Ville had some ideas here.

I will check with Ville, currently not very clear about the solution. Try to get some info how it is handled in windows 

Regards,
Animesh
> 
> 
> BR,
> Jani.
> 
> 
> [1] https://gitlab.freedesktop.org/drm/intel/-/issues/5531
> 
> 
> > +
> > +	return 0;
> > +}
> > +
> >  typedef bool (*vlv_pipe_check)(struct drm_i915_private *dev_priv,
> >  			       enum pipe pipe);
> >
> > @@ -361,15 +372,10 @@ static void intel_pps_get_registers(struct intel_dp
> *intel_dp,
> >  				    struct pps_registers *regs)
> >  {
> >  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > -	int pps_idx = 0;
> > +	int pps_idx = intel_dp->get_pps_idx(intel_dp);
> >
> >  	memset(regs, 0, sizeof(*regs));
> >
> > -	if (IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv))
> > -		pps_idx = bxt_power_sequencer_idx(intel_dp);
> > -	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > -		pps_idx = vlv_power_sequencer_pipe(intel_dp);
> > -
> >  	regs->pp_ctrl = PP_CONTROL(pps_idx);
> >  	regs->pp_stat = PP_STATUS(pps_idx);
> >  	regs->pp_on = PP_ON_DELAYS(pps_idx); @@ -1431,6 +1437,13 @@
> 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))
> > +		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;
> > +	else if (DISPLAY_VER(i915) >= 12)
> > +		intel_dp->get_pps_idx = gen12_power_sequencer_idx;
> > +
> >  	pps_init_timestamps(intel_dp);
> >
> >  	with_intel_pps_lock(intel_dp, wakeref) {
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/pps: added get_pps_idx() hook as part of pps_get_register() cleanup
  2022-08-03  8:13 ` [Intel-gfx] [PATCH] " Jani Nikula
  2022-08-05  8:11   ` Manna, Animesh
@ 2022-08-19  9:46   ` Manna, Animesh
  2022-08-23 13:27   ` Ville Syrjälä
  2 siblings, 0 replies; 7+ messages in thread
From: Manna, Animesh @ 2022-08-19  9:46 UTC (permalink / raw)
  To: ville.syrjala; +Cc: Nikula, Jani, intel-gfx



> -----Original Message-----
> From: Nikula, Jani <jani.nikula@intel.com>
> Sent: Wednesday, August 3, 2022 1:44 PM
> To: Manna, Animesh <animesh.manna@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: ville.syrjala@linux.intel.com; Shankar, Uma <uma.shankar@intel.com>;
> Manna, Animesh <animesh.manna@intel.com>
> Subject: Re: [PATCH] drm/i915/pps: added get_pps_idx() hook as part of
> pps_get_register() cleanup
> 
> On Wed, 03 Aug 2022, Animesh Manna <animesh.manna@intel.com> wrote:
> > To support dual LFP two instances of pps added from display gen12 onwards.
> > Few older platform like VLV also has dual pps support but handling is
> > different. So added separate hook get_pps_idx() to formulate which pps
> > instance to used for a soecific LFP on a specific platform.
> >
> > Simplified pps_get_register() which use get_pps_idx() hook to derive
> > the pps instance and get_pps_idx() will be initialized at pps_init().
> >
> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bios.c     |  5 ++++
> >  drivers/gpu/drm/i915/display/intel_bios.h     |  1 +
> >  .../drm/i915/display/intel_display_types.h    |  2 ++
> >  drivers/gpu/drm/i915/display/intel_pps.c      | 25 ++++++++++++++-----
> >  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 51dde5bfd956..42315615a728 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > @@ -611,6 +611,11 @@ static int opregion_get_panel_type(struct
> drm_i915_private *i915,
> >  	return intel_opregion_get_panel_type(i915);
> >  }
> >
> > +bool intel_bios_is_lfp2(struct intel_encoder *encoder) {
> > +	return encoder->devdata && encoder->devdata->child.handle ==
> > +DEVICE_HANDLE_LFP2; }
> 
> AFAICS the encoder never really needs to know whether it's "lfp1" or "lfp2". It
> needs to know the pps controller number.
> 
> > +
> >  static int vbt_get_panel_type(struct drm_i915_private *i915,
> >  			      const struct intel_bios_encoder_data *devdata,
> >  			      const struct edid *edid)
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.h
> > b/drivers/gpu/drm/i915/display/intel_bios.h
> > index e47582b0de0a..aea72a87ea2c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.h
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.h
> > @@ -251,6 +251,7 @@ bool intel_bios_is_lspcon_present(const struct
> drm_i915_private *i915,
> >  				  enum port port);
> >  bool intel_bios_is_lane_reversal_needed(const struct drm_i915_private *i915,
> >  					enum port port);
> > +bool intel_bios_is_lfp2(struct intel_encoder *encoder);
> >  enum aux_ch intel_bios_port_aux_ch(struct drm_i915_private *dev_priv,
> > enum port port);  bool intel_bios_get_dsc_params(struct intel_encoder
> *encoder,
> >  			       struct intel_crtc_state *crtc_state, diff --git
> > a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 0da9b208d56e..95f71a572b07 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1723,6 +1723,8 @@ struct intel_dp {
> >
> >  	/* When we last wrote the OUI for eDP */
> >  	unsigned long last_oui_write;
> > +
> > +	int (*get_pps_idx)(struct intel_dp *intel_dp);
> 
> Making this a function pointer should be a separate step. Please don't try to do
> too many things at once. Rule of thumb, one change per patch.
> 
> Also, this should be placed near the other function pointer members in struct
> intel_dp.
> 
> >  };
> >
> >  enum lspcon_vendor {
> > diff --git a/drivers/gpu/drm/i915/display/intel_pps.c
> > b/drivers/gpu/drm/i915/display/intel_pps.c
> > index 1b21a341962f..c9cdb302d318 100644
> > --- a/drivers/gpu/drm/i915/display/intel_pps.c
> > +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> > @@ -231,6 +231,17 @@ bxt_power_sequencer_idx(struct intel_dp *intel_dp)
> >  	return backlight_controller;
> >  }
> >
> > +static int
> > +gen12_power_sequencer_idx(struct intel_dp *intel_dp) {
> > +	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > +
> > +	if (intel_bios_is_lfp2(encoder))
> > +		return 1;
> 
> This is actually not how this works. The bxt_power_sequencer_idx() matches
> bspec 20149: "PWM and PPS are assumed to be aligned to be from same block
> and not mismatch" i.e. the backlight controller id and the pps id are the same.
> There are no requirements to map lfp# and pps controller like this, even if it
> might be true in the common case.
> 
> The problem is we need the information *before* we call
> intel_bios_init_panel().
> 
> It's a catch-22. We need the pps id to read the panel EDID, and we need the
> panel EDID to choose the correct panel type in VBT, which we need to get the
> pps id from the VBT.
> 
> This is highlighted in [1]. The 2nd eDP (which is not even physically present, only
> in the VBT, *sigh*) screws up the PPS for the 1st during init.
> 
> I think Ville had some ideas here.

Hi Ville,

Can you please share your thoughts on the above? 

Regards,
Animesh

> 
> 
> BR,
> Jani.
> 
> 
> [1] https://gitlab.freedesktop.org/drm/intel/-/issues/5531
> 
> 
> > +
> > +	return 0;
> > +}
> > +
> >  typedef bool (*vlv_pipe_check)(struct drm_i915_private *dev_priv,
> >  			       enum pipe pipe);
> >
> > @@ -361,15 +372,10 @@ static void intel_pps_get_registers(struct intel_dp
> *intel_dp,
> >  				    struct pps_registers *regs)
> >  {
> >  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > -	int pps_idx = 0;
> > +	int pps_idx = intel_dp->get_pps_idx(intel_dp);
> >
> >  	memset(regs, 0, sizeof(*regs));
> >
> > -	if (IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv))
> > -		pps_idx = bxt_power_sequencer_idx(intel_dp);
> > -	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > -		pps_idx = vlv_power_sequencer_pipe(intel_dp);
> > -
> >  	regs->pp_ctrl = PP_CONTROL(pps_idx);
> >  	regs->pp_stat = PP_STATUS(pps_idx);
> >  	regs->pp_on = PP_ON_DELAYS(pps_idx); @@ -1431,6 +1437,13 @@
> 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))
> > +		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;
> > +	else if (DISPLAY_VER(i915) >= 12)
> > +		intel_dp->get_pps_idx = gen12_power_sequencer_idx;
> > +
> >  	pps_init_timestamps(intel_dp);
> >
> >  	with_intel_pps_lock(intel_dp, wakeref) {
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/pps: added get_pps_idx() hook as part of pps_get_register() cleanup
  2022-08-03  8:13 ` [Intel-gfx] [PATCH] " Jani Nikula
  2022-08-05  8:11   ` Manna, Animesh
  2022-08-19  9:46   ` Manna, Animesh
@ 2022-08-23 13:27   ` Ville Syrjälä
  2022-08-29  6:50     ` Manna, Animesh
  2 siblings, 1 reply; 7+ messages in thread
From: Ville Syrjälä @ 2022-08-23 13:27 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

On Wed, Aug 03, 2022 at 11:13:38AM +0300, Jani Nikula wrote:
> On Wed, 03 Aug 2022, Animesh Manna <animesh.manna@intel.com> wrote:
> > To support dual LFP two instances of pps added from display gen12 onwards.
> > Few older platform like VLV also has dual pps support but handling is
> > different. So added separate hook get_pps_idx() to formulate which pps
> > instance to used for a soecific LFP on a specific platform.
> >
> > Simplified pps_get_register() which use get_pps_idx() hook to derive the
> > pps instance and get_pps_idx() will be initialized at pps_init().
> >
> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bios.c     |  5 ++++
> >  drivers/gpu/drm/i915/display/intel_bios.h     |  1 +
> >  .../drm/i915/display/intel_display_types.h    |  2 ++
> >  drivers/gpu/drm/i915/display/intel_pps.c      | 25 ++++++++++++++-----
> >  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 51dde5bfd956..42315615a728 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > @@ -611,6 +611,11 @@ static int opregion_get_panel_type(struct drm_i915_private *i915,
> >  	return intel_opregion_get_panel_type(i915);
> >  }
> >  
> > +bool intel_bios_is_lfp2(struct intel_encoder *encoder)
> > +{
> > +	return encoder->devdata && encoder->devdata->child.handle == DEVICE_HANDLE_LFP2;
> > +}
> 
> AFAICS the encoder never really needs to know whether it's "lfp1" or
> "lfp2". It needs to know the pps controller number.
> 
> > +
> >  static int vbt_get_panel_type(struct drm_i915_private *i915,
> >  			      const struct intel_bios_encoder_data *devdata,
> >  			      const struct edid *edid)
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.h b/drivers/gpu/drm/i915/display/intel_bios.h
> > index e47582b0de0a..aea72a87ea2c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.h
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.h
> > @@ -251,6 +251,7 @@ bool intel_bios_is_lspcon_present(const struct drm_i915_private *i915,
> >  				  enum port port);
> >  bool intel_bios_is_lane_reversal_needed(const struct drm_i915_private *i915,
> >  					enum port port);
> > +bool intel_bios_is_lfp2(struct intel_encoder *encoder);
> >  enum aux_ch intel_bios_port_aux_ch(struct drm_i915_private *dev_priv, enum port port);
> >  bool intel_bios_get_dsc_params(struct intel_encoder *encoder,
> >  			       struct intel_crtc_state *crtc_state,
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 0da9b208d56e..95f71a572b07 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1723,6 +1723,8 @@ struct intel_dp {
> >  
> >  	/* When we last wrote the OUI for eDP */
> >  	unsigned long last_oui_write;
> > +
> > +	int (*get_pps_idx)(struct intel_dp *intel_dp);
> 
> Making this a function pointer should be a separate step. Please don't
> try to do too many things at once. Rule of thumb, one change per patch.
> 
> Also, this should be placed near the other function pointer members in
> struct intel_dp.
> 
> >  };
> >  
> >  enum lspcon_vendor {
> > diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
> > index 1b21a341962f..c9cdb302d318 100644
> > --- a/drivers/gpu/drm/i915/display/intel_pps.c
> > +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> > @@ -231,6 +231,17 @@ bxt_power_sequencer_idx(struct intel_dp *intel_dp)
> >  	return backlight_controller;
> >  }
> >  
> > +static int
> > +gen12_power_sequencer_idx(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > +
> > +	if (intel_bios_is_lfp2(encoder))
> > +		return 1;
> 
> This is actually not how this works. The bxt_power_sequencer_idx()
> matches bspec 20149: "PWM and PPS are assumed to be aligned to be from
> same block and not mismatch" i.e. the backlight controller id and the
> pps id are the same. There are no requirements to map lfp# and pps
> controller like this, even if it might be true in the common case.
> 
> The problem is we need the information *before* we call
> intel_bios_init_panel().
> 
> It's a catch-22. We need the pps id to read the panel EDID, and we need
> the panel EDID to choose the correct panel type in VBT, which we need to
> get the pps id from the VBT.
> 
> This is highlighted in [1]. The 2nd eDP (which is not even physically
> present, only in the VBT, *sigh*) screws up the PPS for the 1st during
> init.
> 
> I think Ville had some ideas here.

What a mess.

I guess for the panel_type!=255 cases we could just
initialize the panel specific stuff earlier.

The hardest case to solve would be dual panel with
panel_type==255. For that not sure we can much more than to
read out the state of each PPS from the hardware and try to
guess if one of the enabled ones corresponds to our current
panel, and then try to do the EDID read(s).

Or maybe we could just take a shortcut and reject dual
panel + panel_type=255 combos entirely. Or did we already
run into such cases?

-- 
Ville Syrjälä
Intel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/pps: added get_pps_idx() hook as part of pps_get_register() cleanup
  2022-08-23 13:27   ` Ville Syrjälä
@ 2022-08-29  6:50     ` Manna, Animesh
  0 siblings, 0 replies; 7+ messages in thread
From: Manna, Animesh @ 2022-08-29  6:50 UTC (permalink / raw)
  To: Ville Syrjälä, Nikula, Jani; +Cc: intel-gfx



> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Tuesday, August 23, 2022 6:58 PM
> To: Nikula, Jani <jani.nikula@intel.com>
> Cc: Manna, Animesh <animesh.manna@intel.com>; intel-
> gfx@lists.freedesktop.org; Shankar, Uma <uma.shankar@intel.com>
> Subject: Re: [PATCH] drm/i915/pps: added get_pps_idx() hook as part of
> pps_get_register() cleanup
> 
> On Wed, Aug 03, 2022 at 11:13:38AM +0300, Jani Nikula wrote:
> > On Wed, 03 Aug 2022, Animesh Manna <animesh.manna@intel.com> wrote:
> > > To support dual LFP two instances of pps added from display gen12 onwards.
> > > Few older platform like VLV also has dual pps support but handling
> > > is different. So added separate hook get_pps_idx() to formulate
> > > which pps instance to used for a soecific LFP on a specific platform.
> > >
> > > Simplified pps_get_register() which use get_pps_idx() hook to derive
> > > the pps instance and get_pps_idx() will be initialized at pps_init().
> > >
> > > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_bios.c     |  5 ++++
> > >  drivers/gpu/drm/i915/display/intel_bios.h     |  1 +
> > >  .../drm/i915/display/intel_display_types.h    |  2 ++
> > >  drivers/gpu/drm/i915/display/intel_pps.c      | 25 ++++++++++++++-----
> > >  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 51dde5bfd956..42315615a728 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > > @@ -611,6 +611,11 @@ static int opregion_get_panel_type(struct
> drm_i915_private *i915,
> > >  	return intel_opregion_get_panel_type(i915);
> > >  }
> > >
> > > +bool intel_bios_is_lfp2(struct intel_encoder *encoder) {
> > > +	return encoder->devdata && encoder->devdata->child.handle ==
> > > +DEVICE_HANDLE_LFP2; }
> >
> > AFAICS the encoder never really needs to know whether it's "lfp1" or
> > "lfp2". It needs to know the pps controller number.
> >
> > > +
> > >  static int vbt_get_panel_type(struct drm_i915_private *i915,
> > >  			      const struct intel_bios_encoder_data *devdata,
> > >  			      const struct edid *edid)
> > > diff --git a/drivers/gpu/drm/i915/display/intel_bios.h
> > > b/drivers/gpu/drm/i915/display/intel_bios.h
> > > index e47582b0de0a..aea72a87ea2c 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_bios.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_bios.h
> > > @@ -251,6 +251,7 @@ bool intel_bios_is_lspcon_present(const struct
> drm_i915_private *i915,
> > >  				  enum port port);
> > >  bool intel_bios_is_lane_reversal_needed(const struct drm_i915_private
> *i915,
> > >  					enum port port);
> > > +bool intel_bios_is_lfp2(struct intel_encoder *encoder);
> > >  enum aux_ch intel_bios_port_aux_ch(struct drm_i915_private
> > > *dev_priv, enum port port);  bool intel_bios_get_dsc_params(struct
> intel_encoder *encoder,
> > >  			       struct intel_crtc_state *crtc_state, diff --git
> > > a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 0da9b208d56e..95f71a572b07 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -1723,6 +1723,8 @@ struct intel_dp {
> > >
> > >  	/* When we last wrote the OUI for eDP */
> > >  	unsigned long last_oui_write;
> > > +
> > > +	int (*get_pps_idx)(struct intel_dp *intel_dp);
> >
> > Making this a function pointer should be a separate step. Please don't
> > try to do too many things at once. Rule of thumb, one change per patch.
> >
> > Also, this should be placed near the other function pointer members in
> > struct intel_dp.
> >
> > >  };
> > >
> > >  enum lspcon_vendor {
> > > diff --git a/drivers/gpu/drm/i915/display/intel_pps.c
> > > b/drivers/gpu/drm/i915/display/intel_pps.c
> > > index 1b21a341962f..c9cdb302d318 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_pps.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> > > @@ -231,6 +231,17 @@ bxt_power_sequencer_idx(struct intel_dp
> *intel_dp)
> > >  	return backlight_controller;
> > >  }
> > >
> > > +static int
> > > +gen12_power_sequencer_idx(struct intel_dp *intel_dp) {
> > > +	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > > +
> > > +	if (intel_bios_is_lfp2(encoder))
> > > +		return 1;
> >
> > This is actually not how this works. The bxt_power_sequencer_idx()
> > matches bspec 20149: "PWM and PPS are assumed to be aligned to be from
> > same block and not mismatch" i.e. the backlight controller id and the
> > pps id are the same. There are no requirements to map lfp# and pps
> > controller like this, even if it might be true in the common case.
> >
> > The problem is we need the information *before* we call
> > intel_bios_init_panel().
> >
> > It's a catch-22. We need the pps id to read the panel EDID, and we
> > need the panel EDID to choose the correct panel type in VBT, which we
> > need to get the pps id from the VBT.
> >
> > This is highlighted in [1]. The 2nd eDP (which is not even physically
> > present, only in the VBT, *sigh*) screws up the PPS for the 1st during
> > init.
> >
> > I think Ville had some ideas here.
> 
> What a mess.
> 
> I guess for the panel_type!=255 cases we could just initialize the panel specific
> stuff earlier.
> 
> The hardest case to solve would be dual panel with panel_type==255. For that
> not sure we can much more than to read out the state of each PPS from the
> hardware and try to guess if one of the enabled ones corresponds to our current
> panel, and then try to do the EDID read(s).
> 
> Or maybe we could just take a shortcut and reject dual panel + panel_type=255
> combos entirely. Or did we already run into such cases?

Hi Jani/Ville,

For enabling dual EDP scenario I can see vbt_get_panel_type() is getting called for panel type calculation and getting panel type between 0 to 0xf.
Not sure in dual edp enable scenario will there be PANEL_TYPE_PNPID type panel.

Regards,
Animesh
> 
> --
> Ville Syrjälä
> Intel

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-08-29  6:50 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-03  5:29 [Intel-gfx] [PATCH] drm/i915/pps: added get_pps_idx() hook as part of pps_get_register() cleanup Animesh Manna
2022-08-03  6:32 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork
2022-08-03  8:13 ` [Intel-gfx] [PATCH] " Jani Nikula
2022-08-05  8:11   ` Manna, Animesh
2022-08-19  9:46   ` Manna, Animesh
2022-08-23 13:27   ` Ville Syrjälä
2022-08-29  6:50     ` Manna, Animesh

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.