[PATCH v2] drm/i915: Invoke BXT _DSM to enable MUX on HP Workstation laptops

[PATCH v2] drm/i915: Invoke BXT _DSM to enable MUX on HP Workstation laptops

[Kai-Heng Feng]

On HP Fury G7 Workstations, graphics output is re-routed from Intel GFX
to discrete GFX after S3. This is not desirable, because userspace will
treat connected display as a new one, losing display settings.

The expected behavior is to let discrete GFX drives all external
displays.

The platform in question uses ACPI method \_SB.PCI0.HGME to enable MUX.
The method is inside the BXT _DSM, so add the _DSM and call it
accordingly.

I also tested some MUX-less and iGPU only laptops with the BXT _DSM, no
regression was found.

v2:
 - Forward declare struct pci_dev.

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3113
References: https://lore.kernel.org/intel-gfx/1460040732-31417-4-git-send-email-animesh.manna@intel.com/
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/gpu/drm/i915/display/intel_acpi.c | 17 +++++++++++++++++
 drivers/gpu/drm/i915/display/intel_acpi.h |  3 +++
 drivers/gpu/drm/i915/i915_drv.c           |  5 +++++
 3 files changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c b/drivers/gpu/drm/i915/display/intel_acpi.c
index 833d0c1be4f1..c7b57c22dce3 100644
--- a/drivers/gpu/drm/i915/display/intel_acpi.c
+++ b/drivers/gpu/drm/i915/display/intel_acpi.c
@@ -14,11 +14,16 @@
 
 #define INTEL_DSM_REVISION_ID 1 /* For Calpella anyway... */
 #define INTEL_DSM_FN_PLATFORM_MUX_INFO 1 /* No args */
+#define INTEL_DSM_FN_PLATFORM_BXT_MUX_INFO 0 /* No args */
 
 static const guid_t intel_dsm_guid =
 	GUID_INIT(0x7ed873d3, 0xc2d0, 0x4e4f,
 		  0xa8, 0x54, 0x0f, 0x13, 0x17, 0xb0, 0x1c, 0x2c);
 
+static const guid_t intel_bxt_dsm_guid =
+	GUID_INIT(0x3e5b41c6, 0xeb1d, 0x4260,
+		  0x9d, 0x15, 0xc7, 0x1f, 0xba, 0xda, 0xe4, 0x14);
+
 static char *intel_dsm_port_name(u8 id)
 {
 	switch (id) {
@@ -176,6 +181,18 @@ void intel_unregister_dsm_handler(void)
 {
 }
 
+void intel_bxt_dsm_detect(struct pci_dev *pdev)
+{
+	acpi_handle dhandle;
+
+	dhandle = ACPI_HANDLE(&pdev->dev);
+	if (!dhandle)
+		return;
+
+	acpi_evaluate_dsm(dhandle, &intel_bxt_dsm_guid, INTEL_DSM_REVISION_ID,
+			  INTEL_DSM_FN_PLATFORM_BXT_MUX_INFO, NULL);
+}
+
 /*
  * ACPI Specification, Revision 5.0, Appendix B.3.2 _DOD (Enumerate All Devices
  * Attached to the Display Adapter).
diff --git a/drivers/gpu/drm/i915/display/intel_acpi.h b/drivers/gpu/drm/i915/display/intel_acpi.h
index e8b068661d22..d2d560d63bb3 100644
--- a/drivers/gpu/drm/i915/display/intel_acpi.h
+++ b/drivers/gpu/drm/i915/display/intel_acpi.h
@@ -6,15 +6,18 @@
 #ifndef __INTEL_ACPI_H__
 #define __INTEL_ACPI_H__
 
+struct pci_dev;
 struct drm_i915_private;
 
 #ifdef CONFIG_ACPI
 void intel_register_dsm_handler(void);
 void intel_unregister_dsm_handler(void);
+void intel_bxt_dsm_detect(struct pci_dev *pdev);
 void intel_acpi_device_id_update(struct drm_i915_private *i915);
 #else
 static inline void intel_register_dsm_handler(void) { return; }
 static inline void intel_unregister_dsm_handler(void) { return; }
+static inline void intel_bxt_dsm_detect(struct pci_dev *pdev) { return; }
 static inline
 void intel_acpi_device_id_update(struct drm_i915_private *i915) { return; }
 #endif /* CONFIG_ACPI */
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 785dcf20c77b..57b12068aab4 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -853,6 +853,8 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ret)
 		goto out_cleanup_gem;
 
+	intel_bxt_dsm_detect(pdev);
+
 	i915_driver_register(i915);
 
 	enable_rpm_wakeref_asserts(&i915->runtime_pm);
@@ -1215,6 +1217,7 @@ int i915_suspend_switcheroo(struct drm_i915_private *i915, pm_message_t state)
 static int i915_drm_resume(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
 	int ret;
 
 	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
@@ -1271,6 +1274,8 @@ static int i915_drm_resume(struct drm_device *dev)
 
 	intel_gvt_resume(dev_priv);
 
+	intel_bxt_dsm_detect(pdev);
+
 	enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
 
 	return 0;
-- 
2.30.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/i915: Invoke BXT _DSM to enable MUX on HP Workstation laptops

[Jani Nikula]

On Fri, 23 Apr 2021, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
> On HP Fury G7 Workstations, graphics output is re-routed from Intel GFX
> to discrete GFX after S3. This is not desirable, because userspace will
> treat connected display as a new one, losing display settings.
>
> The expected behavior is to let discrete GFX drives all external
> displays.
>
> The platform in question uses ACPI method \_SB.PCI0.HGME to enable MUX.
> The method is inside the BXT _DSM, so add the _DSM and call it
> accordingly.
>
> I also tested some MUX-less and iGPU only laptops with the BXT _DSM, no
> regression was found.

I don't know whether this change is the right thing to do. I don't know
if it isn't either. Need to look into it.

However, I have some general comments, inline.

>
> v2:
>  - Forward declare struct pci_dev.
>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3113
> References: https://lore.kernel.org/intel-gfx/1460040732-31417-4-git-send-email-animesh.manna@intel.com/
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/gpu/drm/i915/display/intel_acpi.c | 17 +++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_acpi.h |  3 +++
>  drivers/gpu/drm/i915/i915_drv.c           |  5 +++++
>  3 files changed, 25 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c b/drivers/gpu/drm/i915/display/intel_acpi.c
> index 833d0c1be4f1..c7b57c22dce3 100644
> --- a/drivers/gpu/drm/i915/display/intel_acpi.c
> +++ b/drivers/gpu/drm/i915/display/intel_acpi.c
> @@ -14,11 +14,16 @@
>  
>  #define INTEL_DSM_REVISION_ID 1 /* For Calpella anyway... */
>  #define INTEL_DSM_FN_PLATFORM_MUX_INFO 1 /* No args */
> +#define INTEL_DSM_FN_PLATFORM_BXT_MUX_INFO 0 /* No args */
>  
>  static const guid_t intel_dsm_guid =
>  	GUID_INIT(0x7ed873d3, 0xc2d0, 0x4e4f,
>  		  0xa8, 0x54, 0x0f, 0x13, 0x17, 0xb0, 0x1c, 0x2c);
>  
> +static const guid_t intel_bxt_dsm_guid =
> +	GUID_INIT(0x3e5b41c6, 0xeb1d, 0x4260,
> +		  0x9d, 0x15, 0xc7, 0x1f, 0xba, 0xda, 0xe4, 0x14);
> +
>  static char *intel_dsm_port_name(u8 id)
>  {
>  	switch (id) {
> @@ -176,6 +181,18 @@ void intel_unregister_dsm_handler(void)
>  {
>  }
>  
> +void intel_bxt_dsm_detect(struct pci_dev *pdev)

Please leave out bxt from the naming and make the argument struct
drm_i915_private *i915. Mmh, then it conflicts with existing
intel_dsm_detect(), maybe we need a more descriptive name altogether?

> +{
> +	acpi_handle dhandle;
> +
> +	dhandle = ACPI_HANDLE(&pdev->dev);
> +	if (!dhandle)
> +		return;
> +
> +	acpi_evaluate_dsm(dhandle, &intel_bxt_dsm_guid, INTEL_DSM_REVISION_ID,
> +			  INTEL_DSM_FN_PLATFORM_BXT_MUX_INFO, NULL);
> +}
> +
>  /*
>   * ACPI Specification, Revision 5.0, Appendix B.3.2 _DOD (Enumerate All Devices
>   * Attached to the Display Adapter).
> diff --git a/drivers/gpu/drm/i915/display/intel_acpi.h b/drivers/gpu/drm/i915/display/intel_acpi.h
> index e8b068661d22..d2d560d63bb3 100644
> --- a/drivers/gpu/drm/i915/display/intel_acpi.h
> +++ b/drivers/gpu/drm/i915/display/intel_acpi.h
> @@ -6,15 +6,18 @@
>  #ifndef __INTEL_ACPI_H__
>  #define __INTEL_ACPI_H__
>  
> +struct pci_dev;
>  struct drm_i915_private;
>  
>  #ifdef CONFIG_ACPI
>  void intel_register_dsm_handler(void);
>  void intel_unregister_dsm_handler(void);
> +void intel_bxt_dsm_detect(struct pci_dev *pdev);
>  void intel_acpi_device_id_update(struct drm_i915_private *i915);
>  #else
>  static inline void intel_register_dsm_handler(void) { return; }
>  static inline void intel_unregister_dsm_handler(void) { return; }
> +static inline void intel_bxt_dsm_detect(struct pci_dev *pdev) { return; }
>  static inline
>  void intel_acpi_device_id_update(struct drm_i915_private *i915) { return; }
>  #endif /* CONFIG_ACPI */
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 785dcf20c77b..57b12068aab4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -853,6 +853,8 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (ret)
>  		goto out_cleanup_gem;
>  
> +	intel_bxt_dsm_detect(pdev);
> +

The call sites in i915_driver_probe() and i915_drm_resume() seem rather
arbitrary.

Long term, I'd like most or all of the display stuff like this placed in
appropriate intel_modeset_*() functions in display/intel_display.c. I'm
not keen on having new and very specific calls in the higher levels.

At probe, feels like the routing should happen earlier, before output
setup? In intel_modeset_init_nogem()?

>  	i915_driver_register(i915);
>  
>  	enable_rpm_wakeref_asserts(&i915->runtime_pm);
> @@ -1215,6 +1217,7 @@ int i915_suspend_switcheroo(struct drm_i915_private *i915, pm_message_t state)
>  static int i915_drm_resume(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
>  	int ret;
>  
>  	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> @@ -1271,6 +1274,8 @@ static int i915_drm_resume(struct drm_device *dev)
>  
>  	intel_gvt_resume(dev_priv);
>  
> +	intel_bxt_dsm_detect(pdev);
> +

In intel_display_resume() perhaps?

(Yay for confusing naming wrt display and modeset, it's a
work-in-progress.)

BR,
Jani.


>  	enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
>  
>  	return 0;

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/i915: Invoke BXT _DSM to enable MUX on HP Workstation laptops

[Ville Syrjälä]

On Fri, Apr 23, 2021 at 12:46:54PM +0800, Kai-Heng Feng wrote:
> On HP Fury G7 Workstations, graphics output is re-routed from Intel GFX
> to discrete GFX after S3. This is not desirable, because userspace will
> treat connected display as a new one, losing display settings.
> 
> The expected behavior is to let discrete GFX drives all external
> displays.
> 
> The platform in question uses ACPI method \_SB.PCI0.HGME to enable MUX.
> The method is inside the BXT _DSM, so add the _DSM and call it
> accordingly.
> 
> I also tested some MUX-less and iGPU only laptops with the BXT _DSM, no
> regression was found.
> 
> v2:
>  - Forward declare struct pci_dev.
> 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3113
> References: https://lore.kernel.org/intel-gfx/1460040732-31417-4-git-send-email-animesh.manna@intel.com/
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/gpu/drm/i915/display/intel_acpi.c | 17 +++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_acpi.h |  3 +++
>  drivers/gpu/drm/i915/i915_drv.c           |  5 +++++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c b/drivers/gpu/drm/i915/display/intel_acpi.c
> index 833d0c1be4f1..c7b57c22dce3 100644
> --- a/drivers/gpu/drm/i915/display/intel_acpi.c
> +++ b/drivers/gpu/drm/i915/display/intel_acpi.c
> @@ -14,11 +14,16 @@
>  
>  #define INTEL_DSM_REVISION_ID 1 /* For Calpella anyway... */
>  #define INTEL_DSM_FN_PLATFORM_MUX_INFO 1 /* No args */
> +#define INTEL_DSM_FN_PLATFORM_BXT_MUX_INFO 0 /* No args */
>  
>  static const guid_t intel_dsm_guid =
>  	GUID_INIT(0x7ed873d3, 0xc2d0, 0x4e4f,
>  		  0xa8, 0x54, 0x0f, 0x13, 0x17, 0xb0, 0x1c, 0x2c);
>  
> +static const guid_t intel_bxt_dsm_guid =
> +	GUID_INIT(0x3e5b41c6, 0xeb1d, 0x4260,
> +		  0x9d, 0x15, 0xc7, 0x1f, 0xba, 0xda, 0xe4, 0x14);
> +

I think this dsm is just supposed to be more or less an
alternative to the opregion SCI stuff. Why there are two
ways to do the same things I have no idea. The opregion
spec does not tell us such mundane details.

It's also not documented to do anything except list the
supported functions:
"Get BIOS Data Functions Supported “Function #0"
 This function can be called to discover which “_DSM” Functions are
 supported. It may only return success if the return value accurately
 lists supported Functions."

But what you're apparently saying is that calling this changes
the behaviour of the system somehow? That is troubling.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/i915: Invoke BXT _DSM to enable MUX on HP Workstation laptops

[Kai-Heng Feng]

On Fri, Apr 23, 2021 at 3:35 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Fri, 23 Apr 2021, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
> > On HP Fury G7 Workstations, graphics output is re-routed from Intel GFX
> > to discrete GFX after S3. This is not desirable, because userspace will
> > treat connected display as a new one, losing display settings.
> >
> > The expected behavior is to let discrete GFX drives all external
> > displays.
> >
> > The platform in question uses ACPI method \_SB.PCI0.HGME to enable MUX.
> > The method is inside the BXT _DSM, so add the _DSM and call it
> > accordingly.
> >
> > I also tested some MUX-less and iGPU only laptops with the BXT _DSM, no
> > regression was found.
>
> I don't know whether this change is the right thing to do. I don't know
> if it isn't either. Need to look into it.
>
> However, I have some general comments, inline.
>
> >
> > v2:
> >  - Forward declare struct pci_dev.
> >
> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3113
> > References: https://lore.kernel.org/intel-gfx/1460040732-31417-4-git-send-email-animesh.manna@intel.com/
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_acpi.c | 17 +++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_acpi.h |  3 +++
> >  drivers/gpu/drm/i915/i915_drv.c           |  5 +++++
> >  3 files changed, 25 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c b/drivers/gpu/drm/i915/display/intel_acpi.c
> > index 833d0c1be4f1..c7b57c22dce3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_acpi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_acpi.c
> > @@ -14,11 +14,16 @@
> >
> >  #define INTEL_DSM_REVISION_ID 1 /* For Calpella anyway... */
> >  #define INTEL_DSM_FN_PLATFORM_MUX_INFO 1 /* No args */
> > +#define INTEL_DSM_FN_PLATFORM_BXT_MUX_INFO 0 /* No args */
> >
> >  static const guid_t intel_dsm_guid =
> >       GUID_INIT(0x7ed873d3, 0xc2d0, 0x4e4f,
> >                 0xa8, 0x54, 0x0f, 0x13, 0x17, 0xb0, 0x1c, 0x2c);
> >
> > +static const guid_t intel_bxt_dsm_guid =
> > +     GUID_INIT(0x3e5b41c6, 0xeb1d, 0x4260,
> > +               0x9d, 0x15, 0xc7, 0x1f, 0xba, 0xda, 0xe4, 0x14);
> > +
> >  static char *intel_dsm_port_name(u8 id)
> >  {
> >       switch (id) {
> > @@ -176,6 +181,18 @@ void intel_unregister_dsm_handler(void)
> >  {
> >  }
> >
> > +void intel_bxt_dsm_detect(struct pci_dev *pdev)
>
> Please leave out bxt from the naming and make the argument struct
> drm_i915_private *i915. Mmh, then it conflicts with existing
> intel_dsm_detect(), maybe we need a more descriptive name altogether?

If there's no oppose, I'll change it to intel_hp_dsm_detect() in v2.
So far, I've only seen that DSM in HP platform.

>
> > +{
> > +     acpi_handle dhandle;
> > +
> > +     dhandle = ACPI_HANDLE(&pdev->dev);
> > +     if (!dhandle)
> > +             return;
> > +
> > +     acpi_evaluate_dsm(dhandle, &intel_bxt_dsm_guid, INTEL_DSM_REVISION_ID,
> > +                       INTEL_DSM_FN_PLATFORM_BXT_MUX_INFO, NULL);
> > +}
> > +
> >  /*
> >   * ACPI Specification, Revision 5.0, Appendix B.3.2 _DOD (Enumerate All Devices
> >   * Attached to the Display Adapter).
> > diff --git a/drivers/gpu/drm/i915/display/intel_acpi.h b/drivers/gpu/drm/i915/display/intel_acpi.h
> > index e8b068661d22..d2d560d63bb3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_acpi.h
> > +++ b/drivers/gpu/drm/i915/display/intel_acpi.h
> > @@ -6,15 +6,18 @@
> >  #ifndef __INTEL_ACPI_H__
> >  #define __INTEL_ACPI_H__
> >
> > +struct pci_dev;
> >  struct drm_i915_private;
> >
> >  #ifdef CONFIG_ACPI
> >  void intel_register_dsm_handler(void);
> >  void intel_unregister_dsm_handler(void);
> > +void intel_bxt_dsm_detect(struct pci_dev *pdev);
> >  void intel_acpi_device_id_update(struct drm_i915_private *i915);
> >  #else
> >  static inline void intel_register_dsm_handler(void) { return; }
> >  static inline void intel_unregister_dsm_handler(void) { return; }
> > +static inline void intel_bxt_dsm_detect(struct pci_dev *pdev) { return; }
> >  static inline
> >  void intel_acpi_device_id_update(struct drm_i915_private *i915) { return; }
> >  #endif /* CONFIG_ACPI */
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 785dcf20c77b..57b12068aab4 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -853,6 +853,8 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >       if (ret)
> >               goto out_cleanup_gem;
> >
> > +     intel_bxt_dsm_detect(pdev);
> > +
>
> The call sites in i915_driver_probe() and i915_drm_resume() seem rather
> arbitrary.

Yes, because what it really does is flipping a bit in one GPIO, the
EC/hardware will change the MUX based on the GPIO bit.
So it doesn't have any ordering needs to be enforced.

>
> Long term, I'd like most or all of the display stuff like this placed in
> appropriate intel_modeset_*() functions in display/intel_display.c. I'm
> not keen on having new and very specific calls in the higher levels.
>
> At probe, feels like the routing should happen earlier, before output
> setup? In intel_modeset_init_nogem()?

OK, I'll put that in intel_modeset_init_hw() to cover both probe and
resume routines.

Kai-Heng


>
> >       i915_driver_register(i915);
> >
> >       enable_rpm_wakeref_asserts(&i915->runtime_pm);
> > @@ -1215,6 +1217,7 @@ int i915_suspend_switcheroo(struct drm_i915_private *i915, pm_message_t state)
> >  static int i915_drm_resume(struct drm_device *dev)
> >  {
> >       struct drm_i915_private *dev_priv = to_i915(dev);
> > +     struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
> >       int ret;
> >
> >       disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> > @@ -1271,6 +1274,8 @@ static int i915_drm_resume(struct drm_device *dev)
> >
> >       intel_gvt_resume(dev_priv);
> >
> > +     intel_bxt_dsm_detect(pdev);
> > +
>
> In intel_display_resume() perhaps?
>
> (Yay for confusing naming wrt display and modeset, it's a
> work-in-progress.)
>
> BR,
> Jani.
>
>
> >       enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> >
> >       return 0;
>
> --
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/i915: Invoke BXT _DSM to enable MUX on HP Workstation laptops

[Kai-Heng Feng]

On Fri, Apr 23, 2021 at 8:41 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Fri, Apr 23, 2021 at 12:46:54PM +0800, Kai-Heng Feng wrote:
> > On HP Fury G7 Workstations, graphics output is re-routed from Intel GFX
> > to discrete GFX after S3. This is not desirable, because userspace will
> > treat connected display as a new one, losing display settings.
> >
> > The expected behavior is to let discrete GFX drives all external
> > displays.
> >
> > The platform in question uses ACPI method \_SB.PCI0.HGME to enable MUX.
> > The method is inside the BXT _DSM, so add the _DSM and call it
> > accordingly.
> >
> > I also tested some MUX-less and iGPU only laptops with the BXT _DSM, no
> > regression was found.
> >
> > v2:
> >  - Forward declare struct pci_dev.
> >
> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3113
> > References: https://lore.kernel.org/intel-gfx/1460040732-31417-4-git-send-email-animesh.manna@intel.com/
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_acpi.c | 17 +++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_acpi.h |  3 +++
> >  drivers/gpu/drm/i915/i915_drv.c           |  5 +++++
> >  3 files changed, 25 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c b/drivers/gpu/drm/i915/display/intel_acpi.c
> > index 833d0c1be4f1..c7b57c22dce3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_acpi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_acpi.c
> > @@ -14,11 +14,16 @@
> >
> >  #define INTEL_DSM_REVISION_ID 1 /* For Calpella anyway... */
> >  #define INTEL_DSM_FN_PLATFORM_MUX_INFO 1 /* No args */
> > +#define INTEL_DSM_FN_PLATFORM_BXT_MUX_INFO 0 /* No args */
> >
> >  static const guid_t intel_dsm_guid =
> >       GUID_INIT(0x7ed873d3, 0xc2d0, 0x4e4f,
> >                 0xa8, 0x54, 0x0f, 0x13, 0x17, 0xb0, 0x1c, 0x2c);
> >
> > +static const guid_t intel_bxt_dsm_guid =
> > +     GUID_INIT(0x3e5b41c6, 0xeb1d, 0x4260,
> > +               0x9d, 0x15, 0xc7, 0x1f, 0xba, 0xda, 0xe4, 0x14);
> > +
>
> I think this dsm is just supposed to be more or less an
> alternative to the opregion SCI stuff. Why there are two
> ways to do the same things I have no idea. The opregion
> spec does not tell us such mundane details.

Right now I think it's HP specific and from what I can see it doesn't
touch opregion.

>
> It's also not documented to do anything except list the
> supported functions:
> "Get BIOS Data Functions Supported “Function #0"
>  This function can be called to discover which “_DSM” Functions are
>  supported. It may only return success if the return value accurately
>  lists supported Functions."
>
> But what you're apparently saying is that calling this changes
> the behaviour of the system somehow? That is troubling.

It flips a bit in BIOS-reserved Intel GPIO, and EC/hardware will
change the MUX based on the GPIO bit.

We can add a DMI check to match "HP" to minimize the potential
regression factor.

Kai-Heng

>
> --
> Ville Syrjälä
> Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/i915: Invoke BXT _DSM to enable MUX on HP Workstation laptops

[Jani Nikula]

On Mon, 26 Apr 2021, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
> On Fri, Apr 23, 2021 at 3:35 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>
>> On Fri, 23 Apr 2021, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
>> > On HP Fury G7 Workstations, graphics output is re-routed from Intel GFX
>> > to discrete GFX after S3. This is not desirable, because userspace will
>> > treat connected display as a new one, losing display settings.
>> >
>> > The expected behavior is to let discrete GFX drives all external
>> > displays.
>> >
>> > The platform in question uses ACPI method \_SB.PCI0.HGME to enable MUX.
>> > The method is inside the BXT _DSM, so add the _DSM and call it
>> > accordingly.
>> >
>> > I also tested some MUX-less and iGPU only laptops with the BXT _DSM, no
>> > regression was found.
>>
>> I don't know whether this change is the right thing to do. I don't know
>> if it isn't either. Need to look into it.
>>
>> However, I have some general comments, inline.
>>
>> >
>> > v2:
>> >  - Forward declare struct pci_dev.
>> >
>> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3113
>> > References: https://lore.kernel.org/intel-gfx/1460040732-31417-4-git-send-email-animesh.manna@intel.com/
>> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_acpi.c | 17 +++++++++++++++++
>> >  drivers/gpu/drm/i915/display/intel_acpi.h |  3 +++
>> >  drivers/gpu/drm/i915/i915_drv.c           |  5 +++++
>> >  3 files changed, 25 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c b/drivers/gpu/drm/i915/display/intel_acpi.c
>> > index 833d0c1be4f1..c7b57c22dce3 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_acpi.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_acpi.c
>> > @@ -14,11 +14,16 @@
>> >
>> >  #define INTEL_DSM_REVISION_ID 1 /* For Calpella anyway... */
>> >  #define INTEL_DSM_FN_PLATFORM_MUX_INFO 1 /* No args */
>> > +#define INTEL_DSM_FN_PLATFORM_BXT_MUX_INFO 0 /* No args */
>> >
>> >  static const guid_t intel_dsm_guid =
>> >       GUID_INIT(0x7ed873d3, 0xc2d0, 0x4e4f,
>> >                 0xa8, 0x54, 0x0f, 0x13, 0x17, 0xb0, 0x1c, 0x2c);
>> >
>> > +static const guid_t intel_bxt_dsm_guid =
>> > +     GUID_INIT(0x3e5b41c6, 0xeb1d, 0x4260,
>> > +               0x9d, 0x15, 0xc7, 0x1f, 0xba, 0xda, 0xe4, 0x14);
>> > +
>> >  static char *intel_dsm_port_name(u8 id)
>> >  {
>> >       switch (id) {
>> > @@ -176,6 +181,18 @@ void intel_unregister_dsm_handler(void)
>> >  {
>> >  }
>> >
>> > +void intel_bxt_dsm_detect(struct pci_dev *pdev)
>>
>> Please leave out bxt from the naming and make the argument struct
>> drm_i915_private *i915. Mmh, then it conflicts with existing
>> intel_dsm_detect(), maybe we need a more descriptive name altogether?
>
> If there's no oppose, I'll change it to intel_hp_dsm_detect() in v2.
> So far, I've only seen that DSM in HP platform.

I'd rather have generic calls with generic naming from appropriate
places in the driver, and then hide the ugly platform or device specific
details within the calls. I certainly don't want any references to a
specific OEM in function names in functions called from the highest
levels of driver probe/resume/etc.

BR,
Jani.

>
>>
>> > +{
>> > +     acpi_handle dhandle;
>> > +
>> > +     dhandle = ACPI_HANDLE(&pdev->dev);
>> > +     if (!dhandle)
>> > +             return;
>> > +
>> > +     acpi_evaluate_dsm(dhandle, &intel_bxt_dsm_guid, INTEL_DSM_REVISION_ID,
>> > +                       INTEL_DSM_FN_PLATFORM_BXT_MUX_INFO, NULL);
>> > +}
>> > +
>> >  /*
>> >   * ACPI Specification, Revision 5.0, Appendix B.3.2 _DOD (Enumerate All Devices
>> >   * Attached to the Display Adapter).
>> > diff --git a/drivers/gpu/drm/i915/display/intel_acpi.h b/drivers/gpu/drm/i915/display/intel_acpi.h
>> > index e8b068661d22..d2d560d63bb3 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_acpi.h
>> > +++ b/drivers/gpu/drm/i915/display/intel_acpi.h
>> > @@ -6,15 +6,18 @@
>> >  #ifndef __INTEL_ACPI_H__
>> >  #define __INTEL_ACPI_H__
>> >
>> > +struct pci_dev;
>> >  struct drm_i915_private;
>> >
>> >  #ifdef CONFIG_ACPI
>> >  void intel_register_dsm_handler(void);
>> >  void intel_unregister_dsm_handler(void);
>> > +void intel_bxt_dsm_detect(struct pci_dev *pdev);
>> >  void intel_acpi_device_id_update(struct drm_i915_private *i915);
>> >  #else
>> >  static inline void intel_register_dsm_handler(void) { return; }
>> >  static inline void intel_unregister_dsm_handler(void) { return; }
>> > +static inline void intel_bxt_dsm_detect(struct pci_dev *pdev) { return; }
>> >  static inline
>> >  void intel_acpi_device_id_update(struct drm_i915_private *i915) { return; }
>> >  #endif /* CONFIG_ACPI */
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> > index 785dcf20c77b..57b12068aab4 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.c
>> > +++ b/drivers/gpu/drm/i915/i915_drv.c
>> > @@ -853,6 +853,8 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> >       if (ret)
>> >               goto out_cleanup_gem;
>> >
>> > +     intel_bxt_dsm_detect(pdev);
>> > +
>>
>> The call sites in i915_driver_probe() and i915_drm_resume() seem rather
>> arbitrary.
>
> Yes, because what it really does is flipping a bit in one GPIO, the
> EC/hardware will change the MUX based on the GPIO bit.
> So it doesn't have any ordering needs to be enforced.
>
>>
>> Long term, I'd like most or all of the display stuff like this placed in
>> appropriate intel_modeset_*() functions in display/intel_display.c. I'm
>> not keen on having new and very specific calls in the higher levels.
>>
>> At probe, feels like the routing should happen earlier, before output
>> setup? In intel_modeset_init_nogem()?
>
> OK, I'll put that in intel_modeset_init_hw() to cover both probe and
> resume routines.
>
> Kai-Heng
>
>
>>
>> >       i915_driver_register(i915);
>> >
>> >       enable_rpm_wakeref_asserts(&i915->runtime_pm);
>> > @@ -1215,6 +1217,7 @@ int i915_suspend_switcheroo(struct drm_i915_private *i915, pm_message_t state)
>> >  static int i915_drm_resume(struct drm_device *dev)
>> >  {
>> >       struct drm_i915_private *dev_priv = to_i915(dev);
>> > +     struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
>> >       int ret;
>> >
>> >       disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
>> > @@ -1271,6 +1274,8 @@ static int i915_drm_resume(struct drm_device *dev)
>> >
>> >       intel_gvt_resume(dev_priv);
>> >
>> > +     intel_bxt_dsm_detect(pdev);
>> > +
>>
>> In intel_display_resume() perhaps?
>>
>> (Yay for confusing naming wrt display and modeset, it's a
>> work-in-progress.)
>>
>> BR,
>> Jani.
>>
>>
>> >       enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
>> >
>> >       return 0;
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/i915: Invoke BXT _DSM to enable MUX on HP Workstation laptops

[Ville Syrjälä]

On Mon, Apr 26, 2021 at 07:10:06PM +0800, Kai-Heng Feng wrote:
> On Fri, Apr 23, 2021 at 8:41 PM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Fri, Apr 23, 2021 at 12:46:54PM +0800, Kai-Heng Feng wrote:
> > > On HP Fury G7 Workstations, graphics output is re-routed from Intel GFX
> > > to discrete GFX after S3. This is not desirable, because userspace will
> > > treat connected display as a new one, losing display settings.
> > >
> > > The expected behavior is to let discrete GFX drives all external
> > > displays.
> > >
> > > The platform in question uses ACPI method \_SB.PCI0.HGME to enable MUX.
> > > The method is inside the BXT _DSM, so add the _DSM and call it
> > > accordingly.
> > >
> > > I also tested some MUX-less and iGPU only laptops with the BXT _DSM, no
> > > regression was found.
> > >
> > > v2:
> > >  - Forward declare struct pci_dev.
> > >
> > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3113
> > > References: https://lore.kernel.org/intel-gfx/1460040732-31417-4-git-send-email-animesh.manna@intel.com/
> > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_acpi.c | 17 +++++++++++++++++
> > >  drivers/gpu/drm/i915/display/intel_acpi.h |  3 +++
> > >  drivers/gpu/drm/i915/i915_drv.c           |  5 +++++
> > >  3 files changed, 25 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c b/drivers/gpu/drm/i915/display/intel_acpi.c
> > > index 833d0c1be4f1..c7b57c22dce3 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_acpi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_acpi.c
> > > @@ -14,11 +14,16 @@
> > >
> > >  #define INTEL_DSM_REVISION_ID 1 /* For Calpella anyway... */
> > >  #define INTEL_DSM_FN_PLATFORM_MUX_INFO 1 /* No args */
> > > +#define INTEL_DSM_FN_PLATFORM_BXT_MUX_INFO 0 /* No args */
> > >
> > >  static const guid_t intel_dsm_guid =
> > >       GUID_INIT(0x7ed873d3, 0xc2d0, 0x4e4f,
> > >                 0xa8, 0x54, 0x0f, 0x13, 0x17, 0xb0, 0x1c, 0x2c);
> > >
> > > +static const guid_t intel_bxt_dsm_guid =
> > > +     GUID_INIT(0x3e5b41c6, 0xeb1d, 0x4260,
> > > +               0x9d, 0x15, 0xc7, 0x1f, 0xba, 0xda, 0xe4, 0x14);
> > > +
> >
> > I think this dsm is just supposed to be more or less an
> > alternative to the opregion SCI stuff. Why there are two
> > ways to do the same things I have no idea. The opregion
> > spec does not tell us such mundane details.
> 
> Right now I think it's HP specific and from what I can see it doesn't
> touch opregion.

It's part of the opregion spec.

> 
> >
> > It's also not documented to do anything except list the
> > supported functions:
> > "Get BIOS Data Functions Supported “Function #0"
> >  This function can be called to discover which “_DSM” Functions are
> >  supported. It may only return success if the return value accurately
> >  lists supported Functions."
> >
> > But what you're apparently saying is that calling this changes
> > the behaviour of the system somehow? That is troubling.
> 
> It flips a bit in BIOS-reserved Intel GPIO, and EC/hardware will
> change the MUX based on the GPIO bit.
> 
> We can add a DMI check to match "HP" to minimize the potential
> regression factor.

I'm rather thinking that calling it always may be the right thing to do,
assuming Windows does it as well. Maybe more vendors use it to backdoor in
random junk like this :(

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel