[Intel-gfx] [PATCH v3 1/6] drm/i915: Initialize dkl_phy spin lock from display code path

[Intel-gfx] [PATCH v3 1/6] drm/i915: Initialize dkl_phy spin lock from display code path

[José Roberto de Souza]

Start to move the initialization of some lock from
i915_driver_early_probe().
No dkl function is called prior to intel_setup_outputs(), so this is
a good place to initialize it.

This will also fix a warning in Xe kmd:

[  201.894839] xe 0000:00:02.0: [drm] [ENCODER:235:DDI A/PHY A] failed to retrieve link info, disabling eDP
[  202.136336] xe 0000:00:02.0: [drm] *ERROR* Failed to write source OUI
[  202.175346] INFO: trying to register non-static key.
[  202.175347] irq event stamp: 754060
[  202.175359] hardirqs last  enabled at (754059): [<ffffffff8122cf79>] tick_nohz_idle_enter+0x59/0x80
[  202.180294] The code is fine but needs lockdep annotation, or maybe
[  202.183774] hardirqs last disabled at (754060): [<ffffffff811a5539>] do_idle+0x99/0x230
[  202.192734] you didn't initialize this object before use?
[  202.198951] softirqs last  enabled at (753948): [<ffffffff8114abae>] irq_exit_rcu+0xbe/0x130
[  202.206882] turning off the locking correctness validator.
[  202.212236] softirqs last disabled at (753943): [<ffffffff8114abae>] irq_exit_rcu+0xbe/0x130
[  202.220592] CPU: 2 PID: 1415 Comm: modprobe Tainted: G        W          6.3.0-rc4+zeh-xe+ #909
[  202.243002] Hardware name: Intel Corporation Raptor Lake Client Platform/RaptorLake-P LP5 RVP, BIOS RPLPFWI1.R00.3361.A14.2211151548 11/15/2022
[  202.255737] Call Trace:
[  202.258179]  <TASK>
[  202.260275]  dump_stack_lvl+0x58/0xc0
[  202.263922]  register_lock_class+0x756/0x7d0
[  202.268165]  ? find_held_lock+0x2b/0x80
[  202.271975]  __lock_acquire+0x72/0x28b0
[  202.275786]  ? debug_object_free+0xb4/0x160
[  202.279946]  lock_acquire+0xd1/0x2d0
[  202.283503]  ? intel_dkl_phy_read+0x18/0x60 [xe]
[  202.288181]  _raw_spin_lock+0x2a/0x40
[  202.291825]  ? intel_dkl_phy_read+0x18/0x60 [xe]
[  202.296475]  intel_dkl_phy_read+0x18/0x60 [xe]
[  202.300949]  icl_aux_power_well_enable+0x2bd/0x400 [xe]
[  202.306202]  ? intel_display_power_grab_async_put_ref+0x75/0x120 [xe]
[  202.312649]  intel_power_well_enable+0x1c/0x70 [xe]
[  202.317543]  __intel_display_power_get_domain.part.0+0x4d/0x70 [xe]
[  202.323812]  intel_display_power_get+0x43/0x70 [xe]
[  202.328708]  intel_tc_port_init+0x199/0x2a0 [xe]
[  202.333363]  intel_ddi_init+0x6ad/0xb00 [xe]
[  202.337678]  intel_modeset_init_nogem+0x536/0x6d0 [xe]
[  202.342838]  xe_display_init_noaccel+0x19/0x40 [xe]
[  202.347743]  xe_device_probe+0x1f5/0x2a0 [xe]
[  202.352127]  xe_pci_probe+0x28c/0x480 [xe]
[  202.356260]  pci_device_probe+0x9d/0x150
[  202.360164]  really_probe+0x19a/0x400
[  202.363809]  ? __pfx___driver_attach+0x10/0x10
[  202.368226]  __driver_probe_device+0x73/0x170
[  202.372558]  driver_probe_device+0x1a/0x90
[  202.376632]  __driver_attach+0xcd/0x1c0
[  202.380442]  bus_for_each_dev+0x72/0xc0
[  202.384253]  bus_add_driver+0x110/0x210
[  202.388063]  driver_register+0x50/0x100
[  202.391873]  ? __pfx_init_module+0x10/0x10 [xe]
[  202.396431]  do_one_initcall+0x55/0x260
[  202.400245]  ? rcu_is_watching+0xd/0x40
[  202.404058]  ? kmalloc_trace+0xa0/0xb0
[  202.407786]  do_init_module+0x45/0x1e0
[  202.411512]  __do_sys_finit_module+0xac/0x120
[  202.415838]  do_syscall_64+0x37/0x90
[  202.419397]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
[  202.424409] RIP: 0033:0x7fd11291ea3d
[  202.427967] Code: 5b 41 5c c3 66 0f 1f 84 00 00 00 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c3 a3 0f 00 f7 d8 64 89 01 48
[  202.446530] RSP: 002b:00007ffffde11368 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[  202.454031] RAX: ffffffffffffffda RBX: 00005616a617f210 RCX: 00007fd11291ea3d
[  202.461106] RDX: 0000000000000000 RSI: 00005616a617fe60 RDI: 000000000000000e
[  202.468182] RBP: 0000000000040000 R08: 0000000000000000 R09: 0000000000000002
[  202.475250] R10: 000000000000000e R11: 0000000000000246 R12: 00005616a617fe60
[  202.482319] R13: 00005616a617f340 R14: 0000000000000000 R15: 00005616a6180650
[  202.489396]  </TASK>

Cc: intel-gfx@lists.freedesktop.org
Cc: intel-xe@lists.freedesktop.org
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 3 +++
 drivers/gpu/drm/i915/display/intel_dkl_phy.c | 6 ++++++
 drivers/gpu/drm/i915/display/intel_dkl_phy.h | 2 ++
 drivers/gpu/drm/i915/i915_driver.c           | 1 -
 4 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 24c96f388d521..cf3a3b3afdd13 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -83,6 +83,7 @@
 #include "intel_display_debugfs.h"
 #include "intel_display_power.h"
 #include "intel_display_types.h"
+#include "intel_dkl_phy.h"
 #include "intel_dmc.h"
 #include "intel_dp.h"
 #include "intel_dp_link_training.h"
@@ -7883,6 +7884,8 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
 	if (!HAS_DISPLAY(dev_priv))
 		return;
 
+	intel_dkl_phy_init(dev_priv);
+
 	if (IS_DG2(dev_priv)) {
 		intel_ddi_init(dev_priv, PORT_A);
 		intel_ddi_init(dev_priv, PORT_B);
diff --git a/drivers/gpu/drm/i915/display/intel_dkl_phy.c b/drivers/gpu/drm/i915/display/intel_dkl_phy.c
index 57cc3edba0163..5bce7b5b27bc7 100644
--- a/drivers/gpu/drm/i915/display/intel_dkl_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_dkl_phy.c
@@ -104,3 +104,9 @@ intel_dkl_phy_posting_read(struct drm_i915_private *i915, struct intel_dkl_phy_r
 
 	spin_unlock(&i915->display.dkl.phy_lock);
 }
+
+void
+intel_dkl_phy_init(struct drm_i915_private *i915)
+{
+	spin_lock_init(&i915->display.dkl.phy_lock);
+}
diff --git a/drivers/gpu/drm/i915/display/intel_dkl_phy.h b/drivers/gpu/drm/i915/display/intel_dkl_phy.h
index 570ee36f9386f..615429b6392c5 100644
--- a/drivers/gpu/drm/i915/display/intel_dkl_phy.h
+++ b/drivers/gpu/drm/i915/display/intel_dkl_phy.h
@@ -20,5 +20,7 @@ void
 intel_dkl_phy_rmw(struct drm_i915_private *i915, struct intel_dkl_phy_reg reg, u32 clear, u32 set);
 void
 intel_dkl_phy_posting_read(struct drm_i915_private *i915, struct intel_dkl_phy_reg reg);
+void
+intel_dkl_phy_init(struct drm_i915_private *i915);
 
 #endif /* __INTEL_DKL_PHY_H__ */
diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index 066d79c2069c4..6fe1a97cc246d 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -224,7 +224,6 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
 	mutex_init(&dev_priv->display.wm.wm_mutex);
 	mutex_init(&dev_priv->display.pps.mutex);
 	mutex_init(&dev_priv->display.hdcp.comp_mutex);
-	spin_lock_init(&dev_priv->display.dkl.phy_lock);
 
 	i915_memcpy_init_early(dev_priv);
 	intel_runtime_pm_init_early(&dev_priv->runtime_pm);
-- 
2.40.0

[Intel-gfx] [PATCH v3 2/6] drm/i915: Only initialize dlk phy lock in display 12 and newer

[José Roberto de Souza]

This spin lock will not be used in older display versions, so no need
to initialize it.

Cc: intel-gfx@lists.freedesktop.org
Cc: intel-xe@lists.freedesktop.org
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dkl_phy.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dkl_phy.c b/drivers/gpu/drm/i915/display/intel_dkl_phy.c
index 5bce7b5b27bc7..1c5d410b74e5d 100644
--- a/drivers/gpu/drm/i915/display/intel_dkl_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_dkl_phy.c
@@ -108,5 +108,6 @@ intel_dkl_phy_posting_read(struct drm_i915_private *i915, struct intel_dkl_phy_r
 void
 intel_dkl_phy_init(struct drm_i915_private *i915)
 {
-	spin_lock_init(&i915->display.dkl.phy_lock);
+	if (DISPLAY_VER(i915) >= 12)
+		spin_lock_init(&i915->display.dkl.phy_lock);
 }
-- 
2.40.0

Re: [Intel-gfx] [Intel-xe] [PATCH v3 1/6] drm/i915: Initialize dkl_phy spin lock from display code path

[Jani Nikula]

On Mon, 10 Apr 2023, José Roberto de Souza <jose.souza@intel.com> wrote:
> Start to move the initialization of some lock from
> i915_driver_early_probe().

Please send this as standalone patch to i915. It won't get merged to
i915 as part of series only parts of which apply to i915.

BR,
Jani.


> No dkl function is called prior to intel_setup_outputs(), so this is
> a good place to initialize it.
>
> This will also fix a warning in Xe kmd:
>
> [  201.894839] xe 0000:00:02.0: [drm] [ENCODER:235:DDI A/PHY A] failed to retrieve link info, disabling eDP
> [  202.136336] xe 0000:00:02.0: [drm] *ERROR* Failed to write source OUI
> [  202.175346] INFO: trying to register non-static key.
> [  202.175347] irq event stamp: 754060
> [  202.175359] hardirqs last  enabled at (754059): [<ffffffff8122cf79>] tick_nohz_idle_enter+0x59/0x80
> [  202.180294] The code is fine but needs lockdep annotation, or maybe
> [  202.183774] hardirqs last disabled at (754060): [<ffffffff811a5539>] do_idle+0x99/0x230
> [  202.192734] you didn't initialize this object before use?
> [  202.198951] softirqs last  enabled at (753948): [<ffffffff8114abae>] irq_exit_rcu+0xbe/0x130
> [  202.206882] turning off the locking correctness validator.
> [  202.212236] softirqs last disabled at (753943): [<ffffffff8114abae>] irq_exit_rcu+0xbe/0x130
> [  202.220592] CPU: 2 PID: 1415 Comm: modprobe Tainted: G        W          6.3.0-rc4+zeh-xe+ #909
> [  202.243002] Hardware name: Intel Corporation Raptor Lake Client Platform/RaptorLake-P LP5 RVP, BIOS RPLPFWI1.R00.3361.A14.2211151548 11/15/2022
> [  202.255737] Call Trace:
> [  202.258179]  <TASK>
> [  202.260275]  dump_stack_lvl+0x58/0xc0
> [  202.263922]  register_lock_class+0x756/0x7d0
> [  202.268165]  ? find_held_lock+0x2b/0x80
> [  202.271975]  __lock_acquire+0x72/0x28b0
> [  202.275786]  ? debug_object_free+0xb4/0x160
> [  202.279946]  lock_acquire+0xd1/0x2d0
> [  202.283503]  ? intel_dkl_phy_read+0x18/0x60 [xe]
> [  202.288181]  _raw_spin_lock+0x2a/0x40
> [  202.291825]  ? intel_dkl_phy_read+0x18/0x60 [xe]
> [  202.296475]  intel_dkl_phy_read+0x18/0x60 [xe]
> [  202.300949]  icl_aux_power_well_enable+0x2bd/0x400 [xe]
> [  202.306202]  ? intel_display_power_grab_async_put_ref+0x75/0x120 [xe]
> [  202.312649]  intel_power_well_enable+0x1c/0x70 [xe]
> [  202.317543]  __intel_display_power_get_domain.part.0+0x4d/0x70 [xe]
> [  202.323812]  intel_display_power_get+0x43/0x70 [xe]
> [  202.328708]  intel_tc_port_init+0x199/0x2a0 [xe]
> [  202.333363]  intel_ddi_init+0x6ad/0xb00 [xe]
> [  202.337678]  intel_modeset_init_nogem+0x536/0x6d0 [xe]
> [  202.342838]  xe_display_init_noaccel+0x19/0x40 [xe]
> [  202.347743]  xe_device_probe+0x1f5/0x2a0 [xe]
> [  202.352127]  xe_pci_probe+0x28c/0x480 [xe]
> [  202.356260]  pci_device_probe+0x9d/0x150
> [  202.360164]  really_probe+0x19a/0x400
> [  202.363809]  ? __pfx___driver_attach+0x10/0x10
> [  202.368226]  __driver_probe_device+0x73/0x170
> [  202.372558]  driver_probe_device+0x1a/0x90
> [  202.376632]  __driver_attach+0xcd/0x1c0
> [  202.380442]  bus_for_each_dev+0x72/0xc0
> [  202.384253]  bus_add_driver+0x110/0x210
> [  202.388063]  driver_register+0x50/0x100
> [  202.391873]  ? __pfx_init_module+0x10/0x10 [xe]
> [  202.396431]  do_one_initcall+0x55/0x260
> [  202.400245]  ? rcu_is_watching+0xd/0x40
> [  202.404058]  ? kmalloc_trace+0xa0/0xb0
> [  202.407786]  do_init_module+0x45/0x1e0
> [  202.411512]  __do_sys_finit_module+0xac/0x120
> [  202.415838]  do_syscall_64+0x37/0x90
> [  202.419397]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
> [  202.424409] RIP: 0033:0x7fd11291ea3d
> [  202.427967] Code: 5b 41 5c c3 66 0f 1f 84 00 00 00 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c3 a3 0f 00 f7 d8 64 89 01 48
> [  202.446530] RSP: 002b:00007ffffde11368 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
> [  202.454031] RAX: ffffffffffffffda RBX: 00005616a617f210 RCX: 00007fd11291ea3d
> [  202.461106] RDX: 0000000000000000 RSI: 00005616a617fe60 RDI: 000000000000000e
> [  202.468182] RBP: 0000000000040000 R08: 0000000000000000 R09: 0000000000000002
> [  202.475250] R10: 000000000000000e R11: 0000000000000246 R12: 00005616a617fe60
> [  202.482319] R13: 00005616a617f340 R14: 0000000000000000 R15: 00005616a6180650
> [  202.489396]  </TASK>
>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: intel-xe@lists.freedesktop.org
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 3 +++
>  drivers/gpu/drm/i915/display/intel_dkl_phy.c | 6 ++++++
>  drivers/gpu/drm/i915/display/intel_dkl_phy.h | 2 ++
>  drivers/gpu/drm/i915/i915_driver.c           | 1 -
>  4 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 24c96f388d521..cf3a3b3afdd13 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -83,6 +83,7 @@
>  #include "intel_display_debugfs.h"
>  #include "intel_display_power.h"
>  #include "intel_display_types.h"
> +#include "intel_dkl_phy.h"
>  #include "intel_dmc.h"
>  #include "intel_dp.h"
>  #include "intel_dp_link_training.h"
> @@ -7883,6 +7884,8 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
>  	if (!HAS_DISPLAY(dev_priv))
>  		return;
>  
> +	intel_dkl_phy_init(dev_priv);
> +
>  	if (IS_DG2(dev_priv)) {
>  		intel_ddi_init(dev_priv, PORT_A);
>  		intel_ddi_init(dev_priv, PORT_B);
> diff --git a/drivers/gpu/drm/i915/display/intel_dkl_phy.c b/drivers/gpu/drm/i915/display/intel_dkl_phy.c
> index 57cc3edba0163..5bce7b5b27bc7 100644
> --- a/drivers/gpu/drm/i915/display/intel_dkl_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_dkl_phy.c
> @@ -104,3 +104,9 @@ intel_dkl_phy_posting_read(struct drm_i915_private *i915, struct intel_dkl_phy_r
>  
>  	spin_unlock(&i915->display.dkl.phy_lock);
>  }
> +
> +void
> +intel_dkl_phy_init(struct drm_i915_private *i915)
> +{
> +	spin_lock_init(&i915->display.dkl.phy_lock);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dkl_phy.h b/drivers/gpu/drm/i915/display/intel_dkl_phy.h
> index 570ee36f9386f..615429b6392c5 100644
> --- a/drivers/gpu/drm/i915/display/intel_dkl_phy.h
> +++ b/drivers/gpu/drm/i915/display/intel_dkl_phy.h
> @@ -20,5 +20,7 @@ void
>  intel_dkl_phy_rmw(struct drm_i915_private *i915, struct intel_dkl_phy_reg reg, u32 clear, u32 set);
>  void
>  intel_dkl_phy_posting_read(struct drm_i915_private *i915, struct intel_dkl_phy_reg reg);
> +void
> +intel_dkl_phy_init(struct drm_i915_private *i915);
>  
>  #endif /* __INTEL_DKL_PHY_H__ */
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index 066d79c2069c4..6fe1a97cc246d 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -224,7 +224,6 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
>  	mutex_init(&dev_priv->display.wm.wm_mutex);
>  	mutex_init(&dev_priv->display.pps.mutex);
>  	mutex_init(&dev_priv->display.hdcp.comp_mutex);
> -	spin_lock_init(&dev_priv->display.dkl.phy_lock);
>  
>  	i915_memcpy_init_early(dev_priv);
>  	intel_runtime_pm_init_early(&dev_priv->runtime_pm);

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [Intel-xe] [PATCH v3 1/6] drm/i915: Initialize dkl_phy spin lock from display code path

[Jani Nikula]

On Tue, 11 Apr 2023, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Mon, 10 Apr 2023, José Roberto de Souza <jose.souza@intel.com> wrote:
>> Start to move the initialization of some lock from
>> i915_driver_early_probe().
>
> Please send this as standalone patch to i915. It won't get merged to
> i915 as part of series only parts of which apply to i915.

Not to mention drm-xe-next is *not* the path to upstream i915
changes. Each i915 merged there is a potential problem in upstreaming
xe.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH v3 2/6] drm/i915: Only initialize dlk phy lock in display 12 and newer

[Ville Syrjälä]

On Mon, Apr 10, 2023 at 11:32:15AM -0700, José Roberto de Souza wrote:
> This spin lock will not be used in older display versions, so no need
> to initialize it.
> 
> Cc: intel-gfx@lists.freedesktop.org
> Cc: intel-xe@lists.freedesktop.org
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dkl_phy.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dkl_phy.c b/drivers/gpu/drm/i915/display/intel_dkl_phy.c
> index 5bce7b5b27bc7..1c5d410b74e5d 100644
> --- a/drivers/gpu/drm/i915/display/intel_dkl_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_dkl_phy.c
> @@ -108,5 +108,6 @@ intel_dkl_phy_posting_read(struct drm_i915_private *i915, struct intel_dkl_phy_r
>  void
>  intel_dkl_phy_init(struct drm_i915_private *i915)
>  {
> -	spin_lock_init(&i915->display.dkl.phy_lock);
> +	if (DISPLAY_VER(i915) >= 12)

Looks like a pointless check to me. This is so cheap it doesn't
matter, and less LOC is better.

> +		spin_lock_init(&i915->display.dkl.phy_lock);
>  }
> -- 
> 2.40.0

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH v3 1/6] drm/i915: Initialize dkl_phy spin lock from display code path

[Ville Syrjälä]

On Mon, Apr 10, 2023 at 11:32:14AM -0700, José Roberto de Souza wrote:
> Start to move the initialization of some lock from
> i915_driver_early_probe().
> No dkl function is called prior to intel_setup_outputs(), so this is
> a good place to initialize it.

I disagree. We don't want to sprinke these all over the place.

> This will also fix a warning in Xe kmd:
> 
> [  201.894839] xe 0000:00:02.0: [drm] [ENCODER:235:DDI A/PHY A] failed to retrieve link info, disabling eDP
> [  202.136336] xe 0000:00:02.0: [drm] *ERROR* Failed to write source OUI
> [  202.175346] INFO: trying to register non-static key.
> [  202.175347] irq event stamp: 754060
> [  202.175359] hardirqs last  enabled at (754059): [<ffffffff8122cf79>] tick_nohz_idle_enter+0x59/0x80
> [  202.180294] The code is fine but needs lockdep annotation, or maybe
> [  202.183774] hardirqs last disabled at (754060): [<ffffffff811a5539>] do_idle+0x99/0x230
> [  202.192734] you didn't initialize this object before use?
> [  202.198951] softirqs last  enabled at (753948): [<ffffffff8114abae>] irq_exit_rcu+0xbe/0x130
> [  202.206882] turning off the locking correctness validator.
> [  202.212236] softirqs last disabled at (753943): [<ffffffff8114abae>] irq_exit_rcu+0xbe/0x130
> [  202.220592] CPU: 2 PID: 1415 Comm: modprobe Tainted: G        W          6.3.0-rc4+zeh-xe+ #909
> [  202.243002] Hardware name: Intel Corporation Raptor Lake Client Platform/RaptorLake-P LP5 RVP, BIOS RPLPFWI1.R00.3361.A14.2211151548 11/15/2022
> [  202.255737] Call Trace:
> [  202.258179]  <TASK>
> [  202.260275]  dump_stack_lvl+0x58/0xc0
> [  202.263922]  register_lock_class+0x756/0x7d0
> [  202.268165]  ? find_held_lock+0x2b/0x80
> [  202.271975]  __lock_acquire+0x72/0x28b0
> [  202.275786]  ? debug_object_free+0xb4/0x160
> [  202.279946]  lock_acquire+0xd1/0x2d0
> [  202.283503]  ? intel_dkl_phy_read+0x18/0x60 [xe]
> [  202.288181]  _raw_spin_lock+0x2a/0x40
> [  202.291825]  ? intel_dkl_phy_read+0x18/0x60 [xe]
> [  202.296475]  intel_dkl_phy_read+0x18/0x60 [xe]
> [  202.300949]  icl_aux_power_well_enable+0x2bd/0x400 [xe]
> [  202.306202]  ? intel_display_power_grab_async_put_ref+0x75/0x120 [xe]
> [  202.312649]  intel_power_well_enable+0x1c/0x70 [xe]
> [  202.317543]  __intel_display_power_get_domain.part.0+0x4d/0x70 [xe]
> [  202.323812]  intel_display_power_get+0x43/0x70 [xe]
> [  202.328708]  intel_tc_port_init+0x199/0x2a0 [xe]
> [  202.333363]  intel_ddi_init+0x6ad/0xb00 [xe]
> [  202.337678]  intel_modeset_init_nogem+0x536/0x6d0 [xe]
> [  202.342838]  xe_display_init_noaccel+0x19/0x40 [xe]
> [  202.347743]  xe_device_probe+0x1f5/0x2a0 [xe]
> [  202.352127]  xe_pci_probe+0x28c/0x480 [xe]
> [  202.356260]  pci_device_probe+0x9d/0x150
> [  202.360164]  really_probe+0x19a/0x400
> [  202.363809]  ? __pfx___driver_attach+0x10/0x10
> [  202.368226]  __driver_probe_device+0x73/0x170
> [  202.372558]  driver_probe_device+0x1a/0x90
> [  202.376632]  __driver_attach+0xcd/0x1c0
> [  202.380442]  bus_for_each_dev+0x72/0xc0
> [  202.384253]  bus_add_driver+0x110/0x210
> [  202.388063]  driver_register+0x50/0x100
> [  202.391873]  ? __pfx_init_module+0x10/0x10 [xe]
> [  202.396431]  do_one_initcall+0x55/0x260
> [  202.400245]  ? rcu_is_watching+0xd/0x40
> [  202.404058]  ? kmalloc_trace+0xa0/0xb0
> [  202.407786]  do_init_module+0x45/0x1e0
> [  202.411512]  __do_sys_finit_module+0xac/0x120
> [  202.415838]  do_syscall_64+0x37/0x90
> [  202.419397]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
> [  202.424409] RIP: 0033:0x7fd11291ea3d
> [  202.427967] Code: 5b 41 5c c3 66 0f 1f 84 00 00 00 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c3 a3 0f 00 f7 d8 64 89 01 48
> [  202.446530] RSP: 002b:00007ffffde11368 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
> [  202.454031] RAX: ffffffffffffffda RBX: 00005616a617f210 RCX: 00007fd11291ea3d
> [  202.461106] RDX: 0000000000000000 RSI: 00005616a617fe60 RDI: 000000000000000e
> [  202.468182] RBP: 0000000000040000 R08: 0000000000000000 R09: 0000000000000002
> [  202.475250] R10: 000000000000000e R11: 0000000000000246 R12: 00005616a617fe60
> [  202.482319] R13: 00005616a617f340 R14: 0000000000000000 R15: 00005616a6180650
> [  202.489396]  </TASK>
> 
> Cc: intel-gfx@lists.freedesktop.org
> Cc: intel-xe@lists.freedesktop.org
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 3 +++
>  drivers/gpu/drm/i915/display/intel_dkl_phy.c | 6 ++++++
>  drivers/gpu/drm/i915/display/intel_dkl_phy.h | 2 ++
>  drivers/gpu/drm/i915/i915_driver.c           | 1 -
>  4 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 24c96f388d521..cf3a3b3afdd13 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -83,6 +83,7 @@
>  #include "intel_display_debugfs.h"
>  #include "intel_display_power.h"
>  #include "intel_display_types.h"
> +#include "intel_dkl_phy.h"
>  #include "intel_dmc.h"
>  #include "intel_dp.h"
>  #include "intel_dp_link_training.h"
> @@ -7883,6 +7884,8 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
>  	if (!HAS_DISPLAY(dev_priv))
>  		return;
>  
> +	intel_dkl_phy_init(dev_priv);
> +
>  	if (IS_DG2(dev_priv)) {
>  		intel_ddi_init(dev_priv, PORT_A);
>  		intel_ddi_init(dev_priv, PORT_B);
> diff --git a/drivers/gpu/drm/i915/display/intel_dkl_phy.c b/drivers/gpu/drm/i915/display/intel_dkl_phy.c
> index 57cc3edba0163..5bce7b5b27bc7 100644
> --- a/drivers/gpu/drm/i915/display/intel_dkl_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_dkl_phy.c
> @@ -104,3 +104,9 @@ intel_dkl_phy_posting_read(struct drm_i915_private *i915, struct intel_dkl_phy_r
>  
>  	spin_unlock(&i915->display.dkl.phy_lock);
>  }
> +
> +void
> +intel_dkl_phy_init(struct drm_i915_private *i915)
> +{
> +	spin_lock_init(&i915->display.dkl.phy_lock);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dkl_phy.h b/drivers/gpu/drm/i915/display/intel_dkl_phy.h
> index 570ee36f9386f..615429b6392c5 100644
> --- a/drivers/gpu/drm/i915/display/intel_dkl_phy.h
> +++ b/drivers/gpu/drm/i915/display/intel_dkl_phy.h
> @@ -20,5 +20,7 @@ void
>  intel_dkl_phy_rmw(struct drm_i915_private *i915, struct intel_dkl_phy_reg reg, u32 clear, u32 set);
>  void
>  intel_dkl_phy_posting_read(struct drm_i915_private *i915, struct intel_dkl_phy_reg reg);
> +void
> +intel_dkl_phy_init(struct drm_i915_private *i915);
>  
>  #endif /* __INTEL_DKL_PHY_H__ */
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index 066d79c2069c4..6fe1a97cc246d 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -224,7 +224,6 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
>  	mutex_init(&dev_priv->display.wm.wm_mutex);
>  	mutex_init(&dev_priv->display.pps.mutex);
>  	mutex_init(&dev_priv->display.hdcp.comp_mutex);
> -	spin_lock_init(&dev_priv->display.dkl.phy_lock);
>  
>  	i915_memcpy_init_early(dev_priv);
>  	intel_runtime_pm_init_early(&dev_priv->runtime_pm);
> -- 
> 2.40.0

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [Intel-xe] [PATCH v3 1/6] drm/i915: Initialize dkl_phy spin lock from display code path

[Jani Nikula]

On Tue, 11 Apr 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Mon, Apr 10, 2023 at 11:32:14AM -0700, José Roberto de Souza wrote:
>> Start to move the initialization of some lock from
>> i915_driver_early_probe().
>> No dkl function is called prior to intel_setup_outputs(), so this is
>> a good place to initialize it.
>
> I disagree. We don't want to sprinke these all over the place.

I'm thinking if only foo.c uses a lock, foo.c should initialize it, not
someone else.

BR,
Jani.



>
>> This will also fix a warning in Xe kmd:
>> 
>> [  201.894839] xe 0000:00:02.0: [drm] [ENCODER:235:DDI A/PHY A] failed to retrieve link info, disabling eDP
>> [  202.136336] xe 0000:00:02.0: [drm] *ERROR* Failed to write source OUI
>> [  202.175346] INFO: trying to register non-static key.
>> [  202.175347] irq event stamp: 754060
>> [  202.175359] hardirqs last  enabled at (754059): [<ffffffff8122cf79>] tick_nohz_idle_enter+0x59/0x80
>> [  202.180294] The code is fine but needs lockdep annotation, or maybe
>> [  202.183774] hardirqs last disabled at (754060): [<ffffffff811a5539>] do_idle+0x99/0x230
>> [  202.192734] you didn't initialize this object before use?
>> [  202.198951] softirqs last  enabled at (753948): [<ffffffff8114abae>] irq_exit_rcu+0xbe/0x130
>> [  202.206882] turning off the locking correctness validator.
>> [  202.212236] softirqs last disabled at (753943): [<ffffffff8114abae>] irq_exit_rcu+0xbe/0x130
>> [  202.220592] CPU: 2 PID: 1415 Comm: modprobe Tainted: G        W          6.3.0-rc4+zeh-xe+ #909
>> [  202.243002] Hardware name: Intel Corporation Raptor Lake Client Platform/RaptorLake-P LP5 RVP, BIOS RPLPFWI1.R00.3361.A14.2211151548 11/15/2022
>> [  202.255737] Call Trace:
>> [  202.258179]  <TASK>
>> [  202.260275]  dump_stack_lvl+0x58/0xc0
>> [  202.263922]  register_lock_class+0x756/0x7d0
>> [  202.268165]  ? find_held_lock+0x2b/0x80
>> [  202.271975]  __lock_acquire+0x72/0x28b0
>> [  202.275786]  ? debug_object_free+0xb4/0x160
>> [  202.279946]  lock_acquire+0xd1/0x2d0
>> [  202.283503]  ? intel_dkl_phy_read+0x18/0x60 [xe]
>> [  202.288181]  _raw_spin_lock+0x2a/0x40
>> [  202.291825]  ? intel_dkl_phy_read+0x18/0x60 [xe]
>> [  202.296475]  intel_dkl_phy_read+0x18/0x60 [xe]
>> [  202.300949]  icl_aux_power_well_enable+0x2bd/0x400 [xe]
>> [  202.306202]  ? intel_display_power_grab_async_put_ref+0x75/0x120 [xe]
>> [  202.312649]  intel_power_well_enable+0x1c/0x70 [xe]
>> [  202.317543]  __intel_display_power_get_domain.part.0+0x4d/0x70 [xe]
>> [  202.323812]  intel_display_power_get+0x43/0x70 [xe]
>> [  202.328708]  intel_tc_port_init+0x199/0x2a0 [xe]
>> [  202.333363]  intel_ddi_init+0x6ad/0xb00 [xe]
>> [  202.337678]  intel_modeset_init_nogem+0x536/0x6d0 [xe]
>> [  202.342838]  xe_display_init_noaccel+0x19/0x40 [xe]
>> [  202.347743]  xe_device_probe+0x1f5/0x2a0 [xe]
>> [  202.352127]  xe_pci_probe+0x28c/0x480 [xe]
>> [  202.356260]  pci_device_probe+0x9d/0x150
>> [  202.360164]  really_probe+0x19a/0x400
>> [  202.363809]  ? __pfx___driver_attach+0x10/0x10
>> [  202.368226]  __driver_probe_device+0x73/0x170
>> [  202.372558]  driver_probe_device+0x1a/0x90
>> [  202.376632]  __driver_attach+0xcd/0x1c0
>> [  202.380442]  bus_for_each_dev+0x72/0xc0
>> [  202.384253]  bus_add_driver+0x110/0x210
>> [  202.388063]  driver_register+0x50/0x100
>> [  202.391873]  ? __pfx_init_module+0x10/0x10 [xe]
>> [  202.396431]  do_one_initcall+0x55/0x260
>> [  202.400245]  ? rcu_is_watching+0xd/0x40
>> [  202.404058]  ? kmalloc_trace+0xa0/0xb0
>> [  202.407786]  do_init_module+0x45/0x1e0
>> [  202.411512]  __do_sys_finit_module+0xac/0x120
>> [  202.415838]  do_syscall_64+0x37/0x90
>> [  202.419397]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
>> [  202.424409] RIP: 0033:0x7fd11291ea3d
>> [  202.427967] Code: 5b 41 5c c3 66 0f 1f 84 00 00 00 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c3 a3 0f 00 f7 d8 64 89 01 48
>> [  202.446530] RSP: 002b:00007ffffde11368 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
>> [  202.454031] RAX: ffffffffffffffda RBX: 00005616a617f210 RCX: 00007fd11291ea3d
>> [  202.461106] RDX: 0000000000000000 RSI: 00005616a617fe60 RDI: 000000000000000e
>> [  202.468182] RBP: 0000000000040000 R08: 0000000000000000 R09: 0000000000000002
>> [  202.475250] R10: 000000000000000e R11: 0000000000000246 R12: 00005616a617fe60
>> [  202.482319] R13: 00005616a617f340 R14: 0000000000000000 R15: 00005616a6180650
>> [  202.489396]  </TASK>
>> 
>> Cc: intel-gfx@lists.freedesktop.org
>> Cc: intel-xe@lists.freedesktop.org
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_display.c | 3 +++
>>  drivers/gpu/drm/i915/display/intel_dkl_phy.c | 6 ++++++
>>  drivers/gpu/drm/i915/display/intel_dkl_phy.h | 2 ++
>>  drivers/gpu/drm/i915/i915_driver.c           | 1 -
>>  4 files changed, 11 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index 24c96f388d521..cf3a3b3afdd13 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -83,6 +83,7 @@
>>  #include "intel_display_debugfs.h"
>>  #include "intel_display_power.h"
>>  #include "intel_display_types.h"
>> +#include "intel_dkl_phy.h"
>>  #include "intel_dmc.h"
>>  #include "intel_dp.h"
>>  #include "intel_dp_link_training.h"
>> @@ -7883,6 +7884,8 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
>>  	if (!HAS_DISPLAY(dev_priv))
>>  		return;
>>  
>> +	intel_dkl_phy_init(dev_priv);
>> +
>>  	if (IS_DG2(dev_priv)) {
>>  		intel_ddi_init(dev_priv, PORT_A);
>>  		intel_ddi_init(dev_priv, PORT_B);
>> diff --git a/drivers/gpu/drm/i915/display/intel_dkl_phy.c b/drivers/gpu/drm/i915/display/intel_dkl_phy.c
>> index 57cc3edba0163..5bce7b5b27bc7 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dkl_phy.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dkl_phy.c
>> @@ -104,3 +104,9 @@ intel_dkl_phy_posting_read(struct drm_i915_private *i915, struct intel_dkl_phy_r
>>  
>>  	spin_unlock(&i915->display.dkl.phy_lock);
>>  }
>> +
>> +void
>> +intel_dkl_phy_init(struct drm_i915_private *i915)
>> +{
>> +	spin_lock_init(&i915->display.dkl.phy_lock);
>> +}
>> diff --git a/drivers/gpu/drm/i915/display/intel_dkl_phy.h b/drivers/gpu/drm/i915/display/intel_dkl_phy.h
>> index 570ee36f9386f..615429b6392c5 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dkl_phy.h
>> +++ b/drivers/gpu/drm/i915/display/intel_dkl_phy.h
>> @@ -20,5 +20,7 @@ void
>>  intel_dkl_phy_rmw(struct drm_i915_private *i915, struct intel_dkl_phy_reg reg, u32 clear, u32 set);
>>  void
>>  intel_dkl_phy_posting_read(struct drm_i915_private *i915, struct intel_dkl_phy_reg reg);
>> +void
>> +intel_dkl_phy_init(struct drm_i915_private *i915);
>>  
>>  #endif /* __INTEL_DKL_PHY_H__ */
>> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
>> index 066d79c2069c4..6fe1a97cc246d 100644
>> --- a/drivers/gpu/drm/i915/i915_driver.c
>> +++ b/drivers/gpu/drm/i915/i915_driver.c
>> @@ -224,7 +224,6 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
>>  	mutex_init(&dev_priv->display.wm.wm_mutex);
>>  	mutex_init(&dev_priv->display.pps.mutex);
>>  	mutex_init(&dev_priv->display.hdcp.comp_mutex);
>> -	spin_lock_init(&dev_priv->display.dkl.phy_lock);
>>  
>>  	i915_memcpy_init_early(dev_priv);
>>  	intel_runtime_pm_init_early(&dev_priv->runtime_pm);
>> -- 
>> 2.40.0

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [Intel-xe] [PATCH v3 1/6] drm/i915: Initialize dkl_phy spin lock from display code path

[Ville Syrjälä]

On Tue, Apr 11, 2023 at 11:51:33AM +0300, Jani Nikula wrote:
> On Tue, 11 Apr 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Mon, Apr 10, 2023 at 11:32:14AM -0700, José Roberto de Souza wrote:
> >> Start to move the initialization of some lock from
> >> i915_driver_early_probe().
> >> No dkl function is called prior to intel_setup_outputs(), so this is
> >> a good place to initialize it.
> >
> > I disagree. We don't want to sprinke these all over the place.
> 
> I'm thinking if only foo.c uses a lock, foo.c should initialize it, not
> someone else.

Perhaps. But I think there should be some consistent place in the higher
level code where all such things get called instead of dropping each one
individually into some random spot in the overlall display init flow.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [Intel-xe] [PATCH v3 1/6] drm/i915: Initialize dkl_phy spin lock from display code path

[Jani Nikula]

On Tue, 11 Apr 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Apr 11, 2023 at 11:51:33AM +0300, Jani Nikula wrote:
>> On Tue, 11 Apr 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > On Mon, Apr 10, 2023 at 11:32:14AM -0700, José Roberto de Souza wrote:
>> >> Start to move the initialization of some lock from
>> >> i915_driver_early_probe().
>> >> No dkl function is called prior to intel_setup_outputs(), so this is
>> >> a good place to initialize it.
>> >
>> > I disagree. We don't want to sprinke these all over the place.
>> 
>> I'm thinking if only foo.c uses a lock, foo.c should initialize it, not
>> someone else.
>
> Perhaps. But I think there should be some consistent place in the higher
> level code where all such things get called instead of dropping each one
> individually into some random spot in the overlall display init flow.

Agreed.

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [Intel-xe] [PATCH v3 1/6] drm/i915: Initialize dkl_phy spin lock from display code path

[Souza, Jose]

On Tue, 2023-04-11 at 11:33 +0300, Jani Nikula wrote:
> On Tue, 11 Apr 2023, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Mon, 10 Apr 2023, José Roberto de Souza <jose.souza@intel.com> wrote:
> > > Start to move the initialization of some lock from
> > > i915_driver_early_probe().
> > 
> > Please send this as standalone patch to i915. It won't get merged to
> > i915 as part of series only parts of which apply to i915.
> 
> Not to mention drm-xe-next is *not* the path to upstream i915
> changes. Each i915 merged there is a potential problem in upstreaming
> xe.

Already sent it https://patchwork.freedesktop.org/series/116272/

> 
> BR,
> Jani.
> 
> 

Re: [Intel-gfx] [Intel-xe] [PATCH v3 1/6] drm/i915: Initialize dkl_phy spin lock from display code path

[Rodrigo Vivi]

On Tue, Apr 11, 2023 at 12:14:36PM +0300, Jani Nikula wrote:
> On Tue, 11 Apr 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Tue, Apr 11, 2023 at 11:51:33AM +0300, Jani Nikula wrote:
> >> On Tue, 11 Apr 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >> > On Mon, Apr 10, 2023 at 11:32:14AM -0700, José Roberto de Souza wrote:
> >> >> Start to move the initialization of some lock from
> >> >> i915_driver_early_probe().
> >> >> No dkl function is called prior to intel_setup_outputs(), so this is
> >> >> a good place to initialize it.
> >> >
> >> > I disagree. We don't want to sprinke these all over the place.
> >> 
> >> I'm thinking if only foo.c uses a lock, foo.c should initialize it, not
> >> someone else.
> >
> > Perhaps. But I think there should be some consistent place in the higher
> > level code where all such things get called instead of dropping each one
> > individually into some random spot in the overlall display init flow.
> 
> Agreed.

Ops, I just saw this now, right after I cc'ed you in the other thread.

So, probably good to hold this and do the entire refactor together of all
those locks initialization so we find this common consistent place apparently...

> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [Intel-xe] [PATCH v3 1/6] drm/i915: Initialize dkl_phy spin lock from display code path

[Lucas De Marchi]

On Tue, Apr 11, 2023 at 10:51:04AM -0400, Rodrigo Vivi wrote:
>On Tue, Apr 11, 2023 at 12:14:36PM +0300, Jani Nikula wrote:
>> On Tue, 11 Apr 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > On Tue, Apr 11, 2023 at 11:51:33AM +0300, Jani Nikula wrote:
>> >> On Tue, 11 Apr 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> >> > On Mon, Apr 10, 2023 at 11:32:14AM -0700, José Roberto de Souza wrote:
>> >> >> Start to move the initialization of some lock from
>> >> >> i915_driver_early_probe().
>> >> >> No dkl function is called prior to intel_setup_outputs(), so this is
>> >> >> a good place to initialize it.
>> >> >
>> >> > I disagree. We don't want to sprinke these all over the place.
>> >>
>> >> I'm thinking if only foo.c uses a lock, foo.c should initialize it, not
>> >> someone else.
>> >
>> > Perhaps. But I think there should be some consistent place in the higher
>> > level code where all such things get called instead of dropping each one
>> > individually into some random spot in the overlall display init flow.
>>
>> Agreed.
>
>Ops, I just saw this now, right after I cc'ed you in the other thread.
>
>So, probably good to hold this and do the entire refactor together of all
>those locks initialization so we find this common consistent place apparently...

"internal" sw initialization of display-related stuff. It doesn't belong in
i915_driver_early_probe(), it makes harder to follow the sequence if we sprinkle
them around, like here in intel_setup_outputs.

But I don't see why this couldn't be done in a higher level "sw
initialization of display-related stuff".  Should we add an equivalent
of i915_driver_early_probe(), e.g.  intel_display_early_probe()[1],  and
move the display-related things from i915_driver_early_probe()?

In that case, from xe we would call this function rather than
initializing these fields in xe_display_create()

Lucas De Marchi

[1] I don't like the name, but it follows what is already there

>
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [Intel-xe] [PATCH v3 1/6] drm/i915: Initialize dkl_phy spin lock from display code path

[Souza, Jose]

On Tue, 2023-04-11 at 12:59 -0700, Lucas De Marchi wrote:
> On Tue, Apr 11, 2023 at 10:51:04AM -0400, Rodrigo Vivi wrote:
> > On Tue, Apr 11, 2023 at 12:14:36PM +0300, Jani Nikula wrote:
> > > On Tue, 11 Apr 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > > On Tue, Apr 11, 2023 at 11:51:33AM +0300, Jani Nikula wrote:
> > > > > On Tue, 11 Apr 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > > > > On Mon, Apr 10, 2023 at 11:32:14AM -0700, José Roberto de Souza wrote:
> > > > > > > Start to move the initialization of some lock from
> > > > > > > i915_driver_early_probe().
> > > > > > > No dkl function is called prior to intel_setup_outputs(), so this is
> > > > > > > a good place to initialize it.
> > > > > > 
> > > > > > I disagree. We don't want to sprinke these all over the place.
> > > > > 
> > > > > I'm thinking if only foo.c uses a lock, foo.c should initialize it, not
> > > > > someone else.
> > > > 
> > > > Perhaps. But I think there should be some consistent place in the higher
> > > > level code where all such things get called instead of dropping each one
> > > > individually into some random spot in the overlall display init flow.
> > > 
> > > Agreed.
> > 
> > Ops, I just saw this now, right after I cc'ed you in the other thread.
> > 
> > So, probably good to hold this and do the entire refactor together of all
> > those locks initialization so we find this common consistent place apparently...
> 
> "internal" sw initialization of display-related stuff. It doesn't belong in
> i915_driver_early_probe(), it makes harder to follow the sequence if we sprinkle
> them around, like here in intel_setup_outputs.
> 
> But I don't see why this couldn't be done in a higher level "sw
> initialization of display-related stuff".  Should we add an equivalent
> of i915_driver_early_probe(), e.g.  intel_display_early_probe()[1],  and
> move the display-related things from i915_driver_early_probe()?
> 
> In that case, from xe we would call this function rather than
> initializing these fields in xe_display_create()

Sent another version, added intel_display_locks_init() that is called in the beginning of intel_modeset_init_noirq().
https://patchwork.freedesktop.org/series/116326/

If this is accepted we can then move the other display locks from i915_driver_early_probe().

> 
> Lucas De Marchi
> 
> [1] I don't like the name, but it follows what is already there
> 
> > 
> > > 
> > > --
> > > Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [Intel-xe] [PATCH v3 1/6] drm/i915: Initialize dkl_phy spin lock from display code path

[Lucas De Marchi]

On Tue, Apr 11, 2023 at 08:07:12PM +0000, Jose Souza wrote:
>On Tue, 2023-04-11 at 12:59 -0700, Lucas De Marchi wrote:
>> On Tue, Apr 11, 2023 at 10:51:04AM -0400, Rodrigo Vivi wrote:
>> > On Tue, Apr 11, 2023 at 12:14:36PM +0300, Jani Nikula wrote:
>> > > On Tue, 11 Apr 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > > > On Tue, Apr 11, 2023 at 11:51:33AM +0300, Jani Nikula wrote:
>> > > > > On Tue, 11 Apr 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > > > > > On Mon, Apr 10, 2023 at 11:32:14AM -0700, José Roberto de Souza wrote:
>> > > > > > > Start to move the initialization of some lock from
>> > > > > > > i915_driver_early_probe().
>> > > > > > > No dkl function is called prior to intel_setup_outputs(), so this is
>> > > > > > > a good place to initialize it.
>> > > > > >
>> > > > > > I disagree. We don't want to sprinke these all over the place.
>> > > > >
>> > > > > I'm thinking if only foo.c uses a lock, foo.c should initialize it, not
>> > > > > someone else.
>> > > >
>> > > > Perhaps. But I think there should be some consistent place in the higher
>> > > > level code where all such things get called instead of dropping each one
>> > > > individually into some random spot in the overlall display init flow.
>> > >
>> > > Agreed.
>> >
>> > Ops, I just saw this now, right after I cc'ed you in the other thread.
>> >
>> > So, probably good to hold this and do the entire refactor together of all
>> > those locks initialization so we find this common consistent place apparently...
>>
>> "internal" sw initialization of display-related stuff. It doesn't belong in
>> i915_driver_early_probe(), it makes harder to follow the sequence if we sprinkle
>> them around, like here in intel_setup_outputs.
>>
>> But I don't see why this couldn't be done in a higher level "sw
>> initialization of display-related stuff".  Should we add an equivalent
>> of i915_driver_early_probe(), e.g.  intel_display_early_probe()[1],  and
>> move the display-related things from i915_driver_early_probe()?
>>
>> In that case, from xe we would call this function rather than
>> initializing these fields in xe_display_create()
>
>Sent another version, added intel_display_locks_init() that is called in the beginning of intel_modeset_init_noirq().
>https://patchwork.freedesktop.org/series/116326/

modeset? why? That is after we are already probing the hw....
and what does that have to do with modeset?

Lucas De Marchi

>
>If this is accepted we can then move the other display locks from i915_driver_early_probe().
>
>>
>> Lucas De Marchi
>>
>> [1] I don't like the name, but it follows what is already there
>>
>> >
>> > >
>> > > --
>> > > Jani Nikula, Intel Open Source Graphics Center
>

Re: [Intel-gfx] [Intel-xe] [PATCH v3 1/6] drm/i915: Initialize dkl_phy spin lock from display code path

[Souza, Jose]

On Tue, 2023-04-11 at 14:20 -0700, Lucas De Marchi wrote:
> On Tue, Apr 11, 2023 at 08:07:12PM +0000, Jose Souza wrote:
> > On Tue, 2023-04-11 at 12:59 -0700, Lucas De Marchi wrote:
> > > On Tue, Apr 11, 2023 at 10:51:04AM -0400, Rodrigo Vivi wrote:
> > > > On Tue, Apr 11, 2023 at 12:14:36PM +0300, Jani Nikula wrote:
> > > > > On Tue, 11 Apr 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > > > > On Tue, Apr 11, 2023 at 11:51:33AM +0300, Jani Nikula wrote:
> > > > > > > On Tue, 11 Apr 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > > > > > > On Mon, Apr 10, 2023 at 11:32:14AM -0700, José Roberto de Souza wrote:
> > > > > > > > > Start to move the initialization of some lock from
> > > > > > > > > i915_driver_early_probe().
> > > > > > > > > No dkl function is called prior to intel_setup_outputs(), so this is
> > > > > > > > > a good place to initialize it.
> > > > > > > > 
> > > > > > > > I disagree. We don't want to sprinke these all over the place.
> > > > > > > 
> > > > > > > I'm thinking if only foo.c uses a lock, foo.c should initialize it, not
> > > > > > > someone else.
> > > > > > 
> > > > > > Perhaps. But I think there should be some consistent place in the higher
> > > > > > level code where all such things get called instead of dropping each one
> > > > > > individually into some random spot in the overlall display init flow.
> > > > > 
> > > > > Agreed.
> > > > 
> > > > Ops, I just saw this now, right after I cc'ed you in the other thread.
> > > > 
> > > > So, probably good to hold this and do the entire refactor together of all
> > > > those locks initialization so we find this common consistent place apparently...
> > > 
> > > "internal" sw initialization of display-related stuff. It doesn't belong in
> > > i915_driver_early_probe(), it makes harder to follow the sequence if we sprinkle
> > > them around, like here in intel_setup_outputs.
> > > 
> > > But I don't see why this couldn't be done in a higher level "sw
> > > initialization of display-related stuff".  Should we add an equivalent
> > > of i915_driver_early_probe(), e.g.  intel_display_early_probe()[1],  and
> > > move the display-related things from i915_driver_early_probe()?
> > > 
> > > In that case, from xe we would call this function rather than
> > > initializing these fields in xe_display_create()
> > 
> > Sent another version, added intel_display_locks_init() that is called in the beginning of intel_modeset_init_noirq().
> > https://patchwork.freedesktop.org/series/116326/
> 
> modeset? why? That is after we are already probing the hw....

That called during probe, called from i915_driver_probe().

> and what does that have to do with modeset?

The name is misleading but intel_modeset_init_noirq() is the first generic display initialization function called.
There is other display functions called before it but they are very specific(intel_dram_detect(), intel_bw_init_hw() and
intel_device_info_runtime_init()).

> 
> Lucas De Marchi
> 
> > 
> > If this is accepted we can then move the other display locks from i915_driver_early_probe().
> > 
> > > 
> > > Lucas De Marchi
> > > 
> > > [1] I don't like the name, but it follows what is already there
> > > 
> > > > 
> > > > > 
> > > > > --
> > > > > Jani Nikula, Intel Open Source Graphics Center
> > 

Re: [Intel-gfx] [Intel-xe] [PATCH v3 1/6] drm/i915: Initialize dkl_phy spin lock from display code path

[Jani Nikula]

On Wed, 12 Apr 2023, "Souza, Jose" <jose.souza@intel.com> wrote:
> On Tue, 2023-04-11 at 14:20 -0700, Lucas De Marchi wrote:
>> On Tue, Apr 11, 2023 at 08:07:12PM +0000, Jose Souza wrote:
>> > On Tue, 2023-04-11 at 12:59 -0700, Lucas De Marchi wrote:
>> > > On Tue, Apr 11, 2023 at 10:51:04AM -0400, Rodrigo Vivi wrote:
>> > > > On Tue, Apr 11, 2023 at 12:14:36PM +0300, Jani Nikula wrote:
>> > > > > On Tue, 11 Apr 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > > > > > On Tue, Apr 11, 2023 at 11:51:33AM +0300, Jani Nikula wrote:
>> > > > > > > On Tue, 11 Apr 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > > > > > > > On Mon, Apr 10, 2023 at 11:32:14AM -0700, José Roberto de Souza wrote:
>> > > > > > > > > Start to move the initialization of some lock from
>> > > > > > > > > i915_driver_early_probe().
>> > > > > > > > > No dkl function is called prior to intel_setup_outputs(), so this is
>> > > > > > > > > a good place to initialize it.
>> > > > > > > > 
>> > > > > > > > I disagree. We don't want to sprinke these all over the place.
>> > > > > > > 
>> > > > > > > I'm thinking if only foo.c uses a lock, foo.c should initialize it, not
>> > > > > > > someone else.
>> > > > > > 
>> > > > > > Perhaps. But I think there should be some consistent place in the higher
>> > > > > > level code where all such things get called instead of dropping each one
>> > > > > > individually into some random spot in the overlall display init flow.
>> > > > > 
>> > > > > Agreed.
>> > > > 
>> > > > Ops, I just saw this now, right after I cc'ed you in the other thread.
>> > > > 
>> > > > So, probably good to hold this and do the entire refactor together of all
>> > > > those locks initialization so we find this common consistent place apparently...
>> > > 
>> > > "internal" sw initialization of display-related stuff. It doesn't belong in
>> > > i915_driver_early_probe(), it makes harder to follow the sequence if we sprinkle
>> > > them around, like here in intel_setup_outputs.
>> > > 
>> > > But I don't see why this couldn't be done in a higher level "sw
>> > > initialization of display-related stuff".  Should we add an equivalent
>> > > of i915_driver_early_probe(), e.g.  intel_display_early_probe()[1],  and
>> > > move the display-related things from i915_driver_early_probe()?
>> > > 
>> > > In that case, from xe we would call this function rather than
>> > > initializing these fields in xe_display_create()
>> > 
>> > Sent another version, added intel_display_locks_init() that is called in the beginning of intel_modeset_init_noirq().
>> > https://patchwork.freedesktop.org/series/116326/
>> 
>> modeset? why? That is after we are already probing the hw....
>
> That called during probe, called from i915_driver_probe().
>
>> and what does that have to do with modeset?
>
> The name is misleading but intel_modeset_init_noirq() is the first generic display initialization function called.
> There is other display functions called before it but they are very specific(intel_dram_detect(), intel_bw_init_hw() and
> intel_device_info_runtime_init()).

See [1].

BR,
Jani.


[1] https://lore.kernel.org/r/87ile1cjh8.fsf@intel.com

>
>> 
>> Lucas De Marchi
>> 
>> > 
>> > If this is accepted we can then move the other display locks from i915_driver_early_probe().
>> > 
>> > > 
>> > > Lucas De Marchi
>> > > 
>> > > [1] I don't like the name, but it follows what is already there
>> > > 
>> > > > 
>> > > > > 
>> > > > > --
>> > > > > Jani Nikula, Intel Open Source Graphics Center
>> > 
>

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [Intel-xe] [PATCH v3 1/6] drm/i915: Initialize dkl_phy spin lock from display code path

[Jani Nikula]

On Thu, 13 Apr 2023, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 12 Apr 2023, "Souza, Jose" <jose.souza@intel.com> wrote:
>> The name is misleading but intel_modeset_init_noirq() is the first generic display initialization function called.
>> There is other display functions called before it but they are very specific(intel_dram_detect(), intel_bw_init_hw() and
>> intel_device_info_runtime_init()).
>
> See [1].
>
> BR,
> Jani.
>
>
> [1] https://lore.kernel.org/r/87ile1cjh8.fsf@intel.com

All of this is cleaned up in
https://patchwork.freedesktop.org/series/116431/

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center