[PATCH 0/6] drm/i915: BIOS display/adapter power state notifications

[PATCH 0/6] drm/i915: BIOS display/adapter power state notifications

[Jani Nikula]

Hi all, here are some patches for letting the BIOS do some further power
savings on HSW, now based on intel-drm-nightly with Paulo's PC8+ work.

Patches 1-4 are the glue code to wrap the BIOS SWSCI calls into a few
sensible function calls.

Patches 5-6 are for reference only, and I presume they will have to be
tweaked based on testing. Where exactly to do the calls? What parameters
are to be used in the end? Possibly calls need to be added on the
suspend/resume paths too.

I don't have the setup to do the testing and measurements, but I hope
these will enable others.


BR,
Jani.



Jani Nikula (6):
  drm/i915: expose intel_ddi_get_encoder_port()
  drm/i915: add plumbing for SWSCI
  drm/i915: add opregion function to notify bios of encoder
    enable/disable
  drm/i915: add opregion function to notify bios of adapter power state
  DRAFT: drm/i915: do display power state notification on crtc
    enable/disable
  DRAFT: drm/i915: do adapter power state notification on PC8+
    enable/disable

 drivers/gpu/drm/i915/i915_drv.h       |   12 ++
 drivers/gpu/drm/i915/intel_ddi.c      |    2 +-
 drivers/gpu/drm/i915/intel_display.c  |   12 +-
 drivers/gpu/drm/i915/intel_drv.h      |    1 +
 drivers/gpu/drm/i915/intel_opregion.c |  200 ++++++++++++++++++++++++++++++++-
 5 files changed, 223 insertions(+), 4 deletions(-)

-- 
1.7.9.5

[PATCH 1/6] drm/i915: expose intel_ddi_get_encoder_port()

[Jani Nikula]

In preparation for followup work.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c |    2 +-
 drivers/gpu/drm/i915/intel_drv.h |    1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 63aca49..060ea50 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -58,7 +58,7 @@ static const u32 hsw_ddi_translations_fdi[] = {
 	0x00FFFFFF, 0x00040006		/* HDMI parameters */
 };
 
-static enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder)
+enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder)
 {
 	struct drm_encoder *encoder = &intel_encoder->base;
 	int type = intel_encoder->type;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1760808..3ab1a52 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -707,6 +707,7 @@ extern void intel_write_eld(struct drm_encoder *encoder,
 extern void intel_prepare_ddi(struct drm_device *dev);
 extern void hsw_fdi_link_train(struct drm_crtc *crtc);
 extern void intel_ddi_init(struct drm_device *dev, enum port port);
+extern enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder);
 
 /* For use by IVB LP watermark workaround in intel_sprite.c */
 extern void intel_update_watermarks(struct drm_device *dev);
-- 
1.7.9.5

Re: [PATCH 1/6] drm/i915: expose intel_ddi_get_encoder_port()

[Paulo Zanoni]

2013/8/23 Jani Nikula <jani.nikula@intel.com>:
> In preparation for followup work.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>


> ---
>  drivers/gpu/drm/i915/intel_ddi.c |    2 +-
>  drivers/gpu/drm/i915/intel_drv.h |    1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 63aca49..060ea50 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -58,7 +58,7 @@ static const u32 hsw_ddi_translations_fdi[] = {
>         0x00FFFFFF, 0x00040006          /* HDMI parameters */
>  };
>
> -static enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder)
> +enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder)
>  {
>         struct drm_encoder *encoder = &intel_encoder->base;
>         int type = intel_encoder->type;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1760808..3ab1a52 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -707,6 +707,7 @@ extern void intel_write_eld(struct drm_encoder *encoder,
>  extern void intel_prepare_ddi(struct drm_device *dev);
>  extern void hsw_fdi_link_train(struct drm_crtc *crtc);
>  extern void intel_ddi_init(struct drm_device *dev, enum port port);
> +extern enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder);
>
>  /* For use by IVB LP watermark workaround in intel_sprite.c */
>  extern void intel_update_watermarks(struct drm_device *dev);
> --
> 1.7.9.5
>



-- 
Paulo Zanoni

[PATCH 2/6] drm/i915: add plumbing for SWSCI

[Jani Nikula]

SWSCI is a driver to bios call interface.

This checks for SWSCI availability and bios requested callbacks, and
filters out any calls that shouldn't happen. This way the callers don't
need to do the checks all over the place.

v2: silence some checkpatch nagging

v3: set PCI_SWSCI bit 0 to trigger interrupt (Mengdong Lin)

v4: remove an extra #define (Jesse)

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       |    1 +
 drivers/gpu/drm/i915/intel_opregion.c |  121 ++++++++++++++++++++++++++++++++-
 2 files changed, 121 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 84b95b1..adc2f46 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -225,6 +225,7 @@ struct intel_opregion {
 	struct opregion_header __iomem *header;
 	struct opregion_acpi __iomem *acpi;
 	struct opregion_swsci __iomem *swsci;
+	u32 swsci_requested_callbacks;
 	struct opregion_asle __iomem *asle;
 	void __iomem *vbt;
 	u32 __iomem *lid_state;
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index cfb8fb6..a3558ae 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -151,6 +151,51 @@ struct opregion_asle {
 
 #define ASLE_CBLV_VALID         (1<<31)
 
+/* Software System Control Interrupt (SWSCI) */
+#define SWSCI_SCIC_INDICATOR		(1 << 0)
+#define SWSCI_SCIC_MAIN_FUNCTION_SHIFT	1
+#define SWSCI_SCIC_MAIN_FUNCTION_MASK	(0xf << 1)
+#define SWSCI_SCIC_SUB_FUNCTION_SHIFT	8
+#define SWSCI_SCIC_SUB_FUNCTION_MASK	(0x7f << 8)
+#define SWSCI_SCIC_EXIT_STATUS_SHIFT	5
+#define SWSCI_SCIC_EXIT_STATUS_MASK	(7 << 5)
+#define SWSCI_SCIC_EXIT_STATUS_SUCCESS	1
+
+#define SWSCI_FUNCTION_CODE(main, sub) \
+	((main) << SWSCI_SCIC_MAIN_FUNCTION_SHIFT | \
+	 (sub) << SWSCI_SCIC_SUB_FUNCTION_SHIFT)
+
+/* SWSCI: Get BIOS Data (GBDA) */
+#define SWSCI_GBDA			4
+#define SWSCI_GBDA_SUPPORTED_CALLS	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 0)
+#define SWSCI_GBDA_REQUESTED_CALLBACKS	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 1)
+#define SWSCI_GBDA_BOOT_DISPLAY_PREF	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 4)
+#define SWSCI_GBDA_PANEL_DETAILS	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 5)
+#define SWSCI_GBDA_TV_STANDARD		SWSCI_FUNCTION_CODE(SWSCI_GBDA, 6)
+#define SWSCI_GBDA_INTERNAL_GRAPHICS	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 7)
+#define SWSCI_GBDA_SPREAD_SPECTRUM	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 10)
+
+/* SWSCI: System BIOS Callbacks (SBCB) */
+#define SWSCI_SBCB			6
+#define SWSCI_SBCB_SUPPORTED_CALLBACKS	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 0)
+#define SWSCI_SBCB_INIT_COMPLETION	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 1)
+#define SWSCI_SBCB_PRE_HIRES_SET_MODE	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 3)
+#define SWSCI_SBCB_POST_HIRES_SET_MODE	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 4)
+#define SWSCI_SBCB_DISPLAY_SWITCH	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 5)
+#define SWSCI_SBCB_SET_TV_FORMAT	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 6)
+#define SWSCI_SBCB_ADAPTER_POWER_STATE	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 7)
+#define SWSCI_SBCB_DISPLAY_POWER_STATE	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 8)
+#define SWSCI_SBCB_SET_BOOT_DISPLAY	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 9)
+#define SWSCI_SBCB_SET_INTERNAL_GFX	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 11)
+#define SWSCI_SBCB_POST_HIRES_TO_DOS_FS	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 16)
+#define SWSCI_SBCB_SUSPEND_RESUME	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 17)
+#define SWSCI_SBCB_SET_SPREAD_SPECTRUM	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 18)
+#define SWSCI_SBCB_POST_VBE_PM		SWSCI_FUNCTION_CODE(SWSCI_SBCB, 19)
+#define SWSCI_SBCB_ENABLE_DISABLE_AUDIO	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 21)
+
+#define PCI_SWSCI			0xe8
+#define PCI_SWSCI_IRQ			(1 << 0)
+
 #define ACPI_OTHER_OUTPUT (0<<8)
 #define ACPI_VGA_OUTPUT (1<<8)
 #define ACPI_TV_OUTPUT (2<<8)
@@ -158,6 +203,76 @@ struct opregion_asle {
 #define ACPI_LVDS_OUTPUT (4<<8)
 
 #ifdef CONFIG_ACPI
+static int swsci(struct drm_device *dev, u32 function, u32 parm, u32 *parm_out)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct opregion_swsci __iomem *swsci = dev_priv->opregion.swsci;
+	u32 main_function, sub_function, scic;
+	u16 pci_swsci;
+
+	if (!swsci)
+		return 0;
+
+	main_function = (function & SWSCI_SCIC_MAIN_FUNCTION_MASK) >>
+		SWSCI_SCIC_MAIN_FUNCTION_SHIFT;
+	sub_function = (function & SWSCI_SCIC_SUB_FUNCTION_MASK) >>
+		SWSCI_SCIC_SUB_FUNCTION_SHIFT;
+
+	if (main_function == SWSCI_SBCB && sub_function != 0) {
+		/*
+		 * SBCB sub-function codes correspond to the bits in requested
+		 * callbacks, except for bit 0 which is reserved. The driver
+		 * must not call interfaces that are not specifically requested
+		 * by the bios.
+		 */
+		if ((dev_priv->opregion.swsci_requested_callbacks &
+		     (1 << sub_function)) == 0)
+			return 0;
+	}
+
+	/* XXX: The spec tells us to do this, but we are the only user... */
+	scic = ioread32(&swsci->scic);
+	if (scic & SWSCI_SCIC_INDICATOR) {
+		DRM_DEBUG_DRIVER("SWSCI request already in progress\n");
+		return -EBUSY;
+	}
+
+	scic = function | SWSCI_SCIC_INDICATOR;
+
+	iowrite32(parm, &swsci->parm);
+	iowrite32(scic, &swsci->scic);
+
+	/* Tell bios to check the mail. */
+	pci_read_config_word(dev->pdev, PCI_SWSCI, &pci_swsci);
+	pci_write_config_word(dev->pdev, PCI_SWSCI, pci_swsci | PCI_SWSCI_IRQ);
+
+	/*
+	 * Poll for the result. The spec says nothing about timing out; add an
+	 * arbitrary timeout to not hang if the bios fails.
+	 */
+#define C (((scic = ioread32(&swsci->scic)) & SWSCI_SCIC_INDICATOR) == 0)
+	if (wait_for(C, 500)) {
+		DRM_DEBUG_DRIVER("SWSCI request timed out\n");
+		return -ETIMEDOUT;
+	}
+
+	scic = (scic & SWSCI_SCIC_EXIT_STATUS_MASK) >>
+		SWSCI_SCIC_EXIT_STATUS_SHIFT;
+
+	/* Note: scic == 0 is an error! */
+	if (scic != SWSCI_SCIC_EXIT_STATUS_SUCCESS) {
+		DRM_DEBUG_DRIVER("SWSCI request error %u\n", scic);
+		return -EINVAL; /* XXX */
+	}
+
+	if (parm_out)
+		*parm_out = ioread32(&swsci->parm);
+
+	return 0;
+
+#undef C
+}
+
 static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -488,8 +603,12 @@ int intel_opregion_setup(struct drm_device *dev)
 	}
 
 	if (mboxes & MBOX_SWSCI) {
-		DRM_DEBUG_DRIVER("SWSCI supported\n");
 		opregion->swsci = base + OPREGION_SWSCI_OFFSET;
+
+		swsci(dev, SWSCI_GBDA_REQUESTED_CALLBACKS, 0,
+		      &opregion->swsci_requested_callbacks);
+		DRM_DEBUG_DRIVER("SWSCI supported, requested callbacks %08x\n",
+				 opregion->swsci_requested_callbacks);
 	}
 	if (mboxes & MBOX_ASLE) {
 		DRM_DEBUG_DRIVER("ASLE supported\n");
-- 
1.7.9.5

[PATCH] drm/i915: add plumbing for SWSCI

[Jani Nikula]

SWSCI is a driver to bios call interface.

This checks for SWSCI availability and bios requested callbacks, and
filters out any calls that shouldn't happen. This way the callers don't
need to do the checks all over the place.

v2: silence some checkpatch nagging

v3: set PCI_SWSCI bit 0 to trigger interrupt (Mengdong Lin)

v4: remove an extra #define (Jesse)

v5: spec says s/w is responsible for clearing PCI_SWSCI bit 0 too

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       |    1 +
 drivers/gpu/drm/i915/intel_opregion.c |  133 ++++++++++++++++++++++++++++++++-
 2 files changed, 131 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 84b95b1..adc2f46 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -225,6 +225,7 @@ struct intel_opregion {
 	struct opregion_header __iomem *header;
 	struct opregion_acpi __iomem *acpi;
 	struct opregion_swsci __iomem *swsci;
+	u32 swsci_requested_callbacks;
 	struct opregion_asle __iomem *asle;
 	void __iomem *vbt;
 	u32 __iomem *lid_state;
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index cfb8fb6..233cc7f 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -36,8 +36,11 @@
 #include "i915_drv.h"
 #include "intel_drv.h"
 
-#define PCI_ASLE 0xe4
-#define PCI_ASLS 0xfc
+#define PCI_ASLE		0xe4
+#define PCI_ASLS		0xfc
+#define PCI_SWSCI		0xe8
+#define PCI_SWSCI_SCISEL	(1 << 15)
+#define PCI_SWSCI_GSSCIE	(1 << 0)
 
 #define OPREGION_HEADER_OFFSET 0
 #define OPREGION_ACPI_OFFSET   0x100
@@ -151,6 +154,48 @@ struct opregion_asle {
 
 #define ASLE_CBLV_VALID         (1<<31)
 
+/* Software System Control Interrupt (SWSCI) */
+#define SWSCI_SCIC_INDICATOR		(1 << 0)
+#define SWSCI_SCIC_MAIN_FUNCTION_SHIFT	1
+#define SWSCI_SCIC_MAIN_FUNCTION_MASK	(0xf << 1)
+#define SWSCI_SCIC_SUB_FUNCTION_SHIFT	8
+#define SWSCI_SCIC_SUB_FUNCTION_MASK	(0x7f << 8)
+#define SWSCI_SCIC_EXIT_STATUS_SHIFT	5
+#define SWSCI_SCIC_EXIT_STATUS_MASK	(7 << 5)
+#define SWSCI_SCIC_EXIT_STATUS_SUCCESS	1
+
+#define SWSCI_FUNCTION_CODE(main, sub) \
+	((main) << SWSCI_SCIC_MAIN_FUNCTION_SHIFT | \
+	 (sub) << SWSCI_SCIC_SUB_FUNCTION_SHIFT)
+
+/* SWSCI: Get BIOS Data (GBDA) */
+#define SWSCI_GBDA			4
+#define SWSCI_GBDA_SUPPORTED_CALLS	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 0)
+#define SWSCI_GBDA_REQUESTED_CALLBACKS	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 1)
+#define SWSCI_GBDA_BOOT_DISPLAY_PREF	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 4)
+#define SWSCI_GBDA_PANEL_DETAILS	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 5)
+#define SWSCI_GBDA_TV_STANDARD		SWSCI_FUNCTION_CODE(SWSCI_GBDA, 6)
+#define SWSCI_GBDA_INTERNAL_GRAPHICS	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 7)
+#define SWSCI_GBDA_SPREAD_SPECTRUM	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 10)
+
+/* SWSCI: System BIOS Callbacks (SBCB) */
+#define SWSCI_SBCB			6
+#define SWSCI_SBCB_SUPPORTED_CALLBACKS	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 0)
+#define SWSCI_SBCB_INIT_COMPLETION	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 1)
+#define SWSCI_SBCB_PRE_HIRES_SET_MODE	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 3)
+#define SWSCI_SBCB_POST_HIRES_SET_MODE	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 4)
+#define SWSCI_SBCB_DISPLAY_SWITCH	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 5)
+#define SWSCI_SBCB_SET_TV_FORMAT	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 6)
+#define SWSCI_SBCB_ADAPTER_POWER_STATE	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 7)
+#define SWSCI_SBCB_DISPLAY_POWER_STATE	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 8)
+#define SWSCI_SBCB_SET_BOOT_DISPLAY	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 9)
+#define SWSCI_SBCB_SET_INTERNAL_GFX	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 11)
+#define SWSCI_SBCB_POST_HIRES_TO_DOS_FS	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 16)
+#define SWSCI_SBCB_SUSPEND_RESUME	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 17)
+#define SWSCI_SBCB_SET_SPREAD_SPECTRUM	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 18)
+#define SWSCI_SBCB_POST_VBE_PM		SWSCI_FUNCTION_CODE(SWSCI_SBCB, 19)
+#define SWSCI_SBCB_ENABLE_DISABLE_AUDIO	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 21)
+
 #define ACPI_OTHER_OUTPUT (0<<8)
 #define ACPI_VGA_OUTPUT (1<<8)
 #define ACPI_TV_OUTPUT (2<<8)
@@ -158,6 +203,84 @@ struct opregion_asle {
 #define ACPI_LVDS_OUTPUT (4<<8)
 
 #ifdef CONFIG_ACPI
+static int swsci(struct drm_device *dev, u32 function, u32 parm, u32 *parm_out)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct opregion_swsci __iomem *swsci = dev_priv->opregion.swsci;
+	u32 main_function, sub_function, scic;
+	u16 pci_swsci;
+
+	if (!swsci)
+		return 0;
+
+	main_function = (function & SWSCI_SCIC_MAIN_FUNCTION_MASK) >>
+		SWSCI_SCIC_MAIN_FUNCTION_SHIFT;
+	sub_function = (function & SWSCI_SCIC_SUB_FUNCTION_MASK) >>
+		SWSCI_SCIC_SUB_FUNCTION_SHIFT;
+
+	if (main_function == SWSCI_SBCB && sub_function != 0) {
+		/*
+		 * SBCB sub-function codes correspond to the bits in requested
+		 * callbacks, except for bit 0 which is reserved. The driver
+		 * must not call interfaces that are not specifically requested
+		 * by the bios.
+		 */
+		if ((dev_priv->opregion.swsci_requested_callbacks &
+		     (1 << sub_function)) == 0)
+			return 0;
+	}
+
+	/* XXX: The spec tells us to do this, but we are the only user... */
+	scic = ioread32(&swsci->scic);
+	if (scic & SWSCI_SCIC_INDICATOR) {
+		DRM_DEBUG_DRIVER("SWSCI request already in progress\n");
+		return -EBUSY;
+	}
+
+	scic = function | SWSCI_SCIC_INDICATOR;
+
+	iowrite32(parm, &swsci->parm);
+	iowrite32(scic, &swsci->scic);
+
+	/* Ensure SCI event is selected and event trigger is cleared. */
+	pci_read_config_word(dev->pdev, PCI_SWSCI, &pci_swsci);
+	if (!(pci_swsci & PCI_SWSCI_SCISEL) || (pci_swsci & PCI_SWSCI_GSSCIE)) {
+		pci_swsci |= PCI_SWSCI_SCISEL;
+		pci_swsci &= ~PCI_SWSCI_GSSCIE;
+		pci_write_config_word(dev->pdev, PCI_SWSCI, pci_swsci);
+	}
+
+	/* Use event trigger to tell bios to check the mail. */
+	pci_swsci |= PCI_SWSCI_GSSCIE;
+	pci_write_config_word(dev->pdev, PCI_SWSCI, pci_swsci);
+
+	/*
+	 * Poll for the result. The spec says nothing about timing out; add an
+	 * arbitrary timeout to not hang if the bios fails.
+	 */
+#define C (((scic = ioread32(&swsci->scic)) & SWSCI_SCIC_INDICATOR) == 0)
+	if (wait_for(C, 500)) {
+		DRM_DEBUG_DRIVER("SWSCI request timed out\n");
+		return -ETIMEDOUT;
+	}
+
+	scic = (scic & SWSCI_SCIC_EXIT_STATUS_MASK) >>
+		SWSCI_SCIC_EXIT_STATUS_SHIFT;
+
+	/* Note: scic == 0 is an error! */
+	if (scic != SWSCI_SCIC_EXIT_STATUS_SUCCESS) {
+		DRM_DEBUG_DRIVER("SWSCI request error %u\n", scic);
+		return -EINVAL; /* XXX */
+	}
+
+	if (parm_out)
+		*parm_out = ioread32(&swsci->parm);
+
+	return 0;
+
+#undef C
+}
+
 static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -488,8 +611,12 @@ int intel_opregion_setup(struct drm_device *dev)
 	}
 
 	if (mboxes & MBOX_SWSCI) {
-		DRM_DEBUG_DRIVER("SWSCI supported\n");
 		opregion->swsci = base + OPREGION_SWSCI_OFFSET;
+
+		swsci(dev, SWSCI_GBDA_REQUESTED_CALLBACKS, 0,
+		      &opregion->swsci_requested_callbacks);
+		DRM_DEBUG_DRIVER("SWSCI supported, requested callbacks %08x\n",
+				 opregion->swsci_requested_callbacks);
 	}
 	if (mboxes & MBOX_ASLE) {
 		DRM_DEBUG_DRIVER("ASLE supported\n");
-- 
1.7.10.4

Re: [PATCH] drm/i915: add plumbing for SWSCI

[Paulo Zanoni]

Hi

2013/8/28 Jani Nikula <jani.nikula@intel.com>:
> SWSCI is a driver to bios call interface.
>
> This checks for SWSCI availability and bios requested callbacks, and
> filters out any calls that shouldn't happen. This way the callers don't
> need to do the checks all over the place.
>
> v2: silence some checkpatch nagging
>
> v3: set PCI_SWSCI bit 0 to trigger interrupt (Mengdong Lin)
>
> v4: remove an extra #define (Jesse)
>
> v5: spec says s/w is responsible for clearing PCI_SWSCI bit 0 too
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |    1 +
>  drivers/gpu/drm/i915/intel_opregion.c |  133 ++++++++++++++++++++++++++++++++-
>  2 files changed, 131 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 84b95b1..adc2f46 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -225,6 +225,7 @@ struct intel_opregion {
>         struct opregion_header __iomem *header;
>         struct opregion_acpi __iomem *acpi;
>         struct opregion_swsci __iomem *swsci;
> +       u32 swsci_requested_callbacks;
>         struct opregion_asle __iomem *asle;
>         void __iomem *vbt;
>         u32 __iomem *lid_state;
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index cfb8fb6..233cc7f 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -36,8 +36,11 @@
>  #include "i915_drv.h"
>  #include "intel_drv.h"
>
> -#define PCI_ASLE 0xe4
> -#define PCI_ASLS 0xfc
> +#define PCI_ASLE               0xe4
> +#define PCI_ASLS               0xfc
> +#define PCI_SWSCI              0xe8
> +#define PCI_SWSCI_SCISEL       (1 << 15)
> +#define PCI_SWSCI_GSSCIE       (1 << 0)
>
>  #define OPREGION_HEADER_OFFSET 0
>  #define OPREGION_ACPI_OFFSET   0x100
> @@ -151,6 +154,48 @@ struct opregion_asle {
>
>  #define ASLE_CBLV_VALID         (1<<31)
>
> +/* Software System Control Interrupt (SWSCI) */
> +#define SWSCI_SCIC_INDICATOR           (1 << 0)
> +#define SWSCI_SCIC_MAIN_FUNCTION_SHIFT 1
> +#define SWSCI_SCIC_MAIN_FUNCTION_MASK  (0xf << 1)
> +#define SWSCI_SCIC_SUB_FUNCTION_SHIFT  8
> +#define SWSCI_SCIC_SUB_FUNCTION_MASK   (0x7f << 8)

Shouldn't this be (0xff << 8) since it's bits 15:8 ?


> +#define SWSCI_SCIC_EXIT_STATUS_SHIFT   5
> +#define SWSCI_SCIC_EXIT_STATUS_MASK    (7 << 5)
> +#define SWSCI_SCIC_EXIT_STATUS_SUCCESS 1
> +
> +#define SWSCI_FUNCTION_CODE(main, sub) \
> +       ((main) << SWSCI_SCIC_MAIN_FUNCTION_SHIFT | \
> +        (sub) << SWSCI_SCIC_SUB_FUNCTION_SHIFT)
> +
> +/* SWSCI: Get BIOS Data (GBDA) */
> +#define SWSCI_GBDA                     4
> +#define SWSCI_GBDA_SUPPORTED_CALLS     SWSCI_FUNCTION_CODE(SWSCI_GBDA, 0)
> +#define SWSCI_GBDA_REQUESTED_CALLBACKS SWSCI_FUNCTION_CODE(SWSCI_GBDA, 1)
> +#define SWSCI_GBDA_BOOT_DISPLAY_PREF   SWSCI_FUNCTION_CODE(SWSCI_GBDA, 4)
> +#define SWSCI_GBDA_PANEL_DETAILS       SWSCI_FUNCTION_CODE(SWSCI_GBDA, 5)
> +#define SWSCI_GBDA_TV_STANDARD         SWSCI_FUNCTION_CODE(SWSCI_GBDA, 6)

The newer spec says "reserved" here. But let's leave your definition
until they assign it again to something else :) (same thing for the
other TV bit below)


> +#define SWSCI_GBDA_INTERNAL_GRAPHICS   SWSCI_FUNCTION_CODE(SWSCI_GBDA, 7)
> +#define SWSCI_GBDA_SPREAD_SPECTRUM     SWSCI_FUNCTION_CODE(SWSCI_GBDA, 10)
> +
> +/* SWSCI: System BIOS Callbacks (SBCB) */
> +#define SWSCI_SBCB                     6
> +#define SWSCI_SBCB_SUPPORTED_CALLBACKS SWSCI_FUNCTION_CODE(SWSCI_SBCB, 0)
> +#define SWSCI_SBCB_INIT_COMPLETION     SWSCI_FUNCTION_CODE(SWSCI_SBCB, 1)
> +#define SWSCI_SBCB_PRE_HIRES_SET_MODE  SWSCI_FUNCTION_CODE(SWSCI_SBCB, 3)
> +#define SWSCI_SBCB_POST_HIRES_SET_MODE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 4)
> +#define SWSCI_SBCB_DISPLAY_SWITCH      SWSCI_FUNCTION_CODE(SWSCI_SBCB, 5)
> +#define SWSCI_SBCB_SET_TV_FORMAT       SWSCI_FUNCTION_CODE(SWSCI_SBCB, 6)
> +#define SWSCI_SBCB_ADAPTER_POWER_STATE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 7)
> +#define SWSCI_SBCB_DISPLAY_POWER_STATE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 8)
> +#define SWSCI_SBCB_SET_BOOT_DISPLAY    SWSCI_FUNCTION_CODE(SWSCI_SBCB, 9)
> +#define SWSCI_SBCB_SET_INTERNAL_GFX    SWSCI_FUNCTION_CODE(SWSCI_SBCB, 11)
> +#define SWSCI_SBCB_POST_HIRES_TO_DOS_FS        SWSCI_FUNCTION_CODE(SWSCI_SBCB, 16)
> +#define SWSCI_SBCB_SUSPEND_RESUME      SWSCI_FUNCTION_CODE(SWSCI_SBCB, 17)
> +#define SWSCI_SBCB_SET_SPREAD_SPECTRUM SWSCI_FUNCTION_CODE(SWSCI_SBCB, 18)
> +#define SWSCI_SBCB_POST_VBE_PM         SWSCI_FUNCTION_CODE(SWSCI_SBCB, 19)
> +#define SWSCI_SBCB_ENABLE_DISABLE_AUDIO        SWSCI_FUNCTION_CODE(SWSCI_SBCB, 21)
> +
>  #define ACPI_OTHER_OUTPUT (0<<8)
>  #define ACPI_VGA_OUTPUT (1<<8)
>  #define ACPI_TV_OUTPUT (2<<8)
> @@ -158,6 +203,84 @@ struct opregion_asle {
>  #define ACPI_LVDS_OUTPUT (4<<8)
>
>  #ifdef CONFIG_ACPI
> +static int swsci(struct drm_device *dev, u32 function, u32 parm, u32 *parm_out)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct opregion_swsci __iomem *swsci = dev_priv->opregion.swsci;
> +       u32 main_function, sub_function, scic;
> +       u16 pci_swsci;
> +
> +       if (!swsci)
> +               return 0;

You also use "return 0" for success below. I'd return -ESOMETHING
here. Perhaps also print an error message.


> +
> +       main_function = (function & SWSCI_SCIC_MAIN_FUNCTION_MASK) >>
> +               SWSCI_SCIC_MAIN_FUNCTION_SHIFT;
> +       sub_function = (function & SWSCI_SCIC_SUB_FUNCTION_MASK) >>
> +               SWSCI_SCIC_SUB_FUNCTION_SHIFT;
> +
> +       if (main_function == SWSCI_SBCB && sub_function != 0) {
> +               /*
> +                * SBCB sub-function codes correspond to the bits in requested
> +                * callbacks, except for bit 0 which is reserved. The driver
> +                * must not call interfaces that are not specifically requested
> +                * by the bios.
> +                */
> +               if ((dev_priv->opregion.swsci_requested_callbacks &
> +                    (1 << sub_function)) == 0)

I think you have to do (1 << sub_funcition -1) to make it correct.


> +                       return 0;

I'd return -ESOMETHINGELSE here too, possibly printing an error message.


> +       }
> +
> +       /* XXX: The spec tells us to do this, but we are the only user... */

I'd remove the "XXX: " substring from the comment. The spec suggests
only graphics uses this specific interface, but it doesn't say that
other events happening on the system won't cause this bit to be
flipped. I don't know... the code seems correct, so the "XXX" code may
be misleading.

The sad thing is that there's also no guarantees that someone won't
start using the interface just after you read this bit and before
writing it to 1... We may need dev_priv->swsci_lock depending on how
we use this function (I haven't checked the next patches yet).


> +       scic = ioread32(&swsci->scic);
> +       if (scic & SWSCI_SCIC_INDICATOR) {
> +               DRM_DEBUG_DRIVER("SWSCI request already in progress\n");
> +               return -EBUSY;
> +       }
> +
> +       scic = function | SWSCI_SCIC_INDICATOR;
> +
> +       iowrite32(parm, &swsci->parm);
> +       iowrite32(scic, &swsci->scic);
> +


>From what I understood of the spec, from the line below...

> +       /* Ensure SCI event is selected and event trigger is cleared. */
> +       pci_read_config_word(dev->pdev, PCI_SWSCI, &pci_swsci);
> +       if (!(pci_swsci & PCI_SWSCI_SCISEL) || (pci_swsci & PCI_SWSCI_GSSCIE)) {
> +               pci_swsci |= PCI_SWSCI_SCISEL;
> +               pci_swsci &= ~PCI_SWSCI_GSSCIE;
> +               pci_write_config_word(dev->pdev, PCI_SWSCI, pci_swsci);
> +       }
> +
> +       /* Use event trigger to tell bios to check the mail. */
> +       pci_swsci |= PCI_SWSCI_GSSCIE;
> +       pci_write_config_word(dev->pdev, PCI_SWSCI, pci_swsci);

... to the line above, we don't need this code. My understanding is
that the BIOS will do all these things automagically for us, so if we
write these regs we may even be racing with it, especially since we
already wrote swsci->scic. Do we really need this code?


> +
> +       /*
> +        * Poll for the result. The spec says nothing about timing out; add an
> +        * arbitrary timeout to not hang if the bios fails.
> +        */

The spec does mention a timeout, check the description of DSLP: Driver
Sleep Timeout.


> +#define C (((scic = ioread32(&swsci->scic)) & SWSCI_SCIC_INDICATOR) == 0)
> +       if (wait_for(C, 500)) {
> +               DRM_DEBUG_DRIVER("SWSCI request timed out\n");
> +               return -ETIMEDOUT;
> +       }
> +
> +       scic = (scic & SWSCI_SCIC_EXIT_STATUS_MASK) >>
> +               SWSCI_SCIC_EXIT_STATUS_SHIFT;
> +
> +       /* Note: scic == 0 is an error! */
> +       if (scic != SWSCI_SCIC_EXIT_STATUS_SUCCESS) {
> +               DRM_DEBUG_DRIVER("SWSCI request error %u\n", scic);
> +               return -EINVAL; /* XXX */

Why XXX above? To me, a XXX means there's something on the code that
must be fixed.


> +       }
> +
> +       if (parm_out)
> +               *parm_out = ioread32(&swsci->parm);
> +
> +       return 0;
> +
> +#undef C
> +}
> +
>  static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -488,8 +611,12 @@ int intel_opregion_setup(struct drm_device *dev)
>         }
>
>         if (mboxes & MBOX_SWSCI) {
> -               DRM_DEBUG_DRIVER("SWSCI supported\n");
>                 opregion->swsci = base + OPREGION_SWSCI_OFFSET;
> +
> +               swsci(dev, SWSCI_GBDA_REQUESTED_CALLBACKS, 0,
> +                     &opregion->swsci_requested_callbacks);

No error checking?


> +               DRM_DEBUG_DRIVER("SWSCI supported, requested callbacks %08x\n",
> +                                opregion->swsci_requested_callbacks);
>         }
>         if (mboxes & MBOX_ASLE) {
>                 DRM_DEBUG_DRIVER("ASLE supported\n");
> --
> 1.7.10.4
>



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: add plumbing for SWSCI

[Paulo Zanoni]

2013/8/29 Paulo Zanoni <przanoni@gmail.com>:
> Hi
>
> 2013/8/28 Jani Nikula <jani.nikula@intel.com>:
>> SWSCI is a driver to bios call interface.
>>
>> This checks for SWSCI availability and bios requested callbacks, and
>> filters out any calls that shouldn't happen. This way the callers don't
>> need to do the checks all over the place.
>>
>> v2: silence some checkpatch nagging
>>
>> v3: set PCI_SWSCI bit 0 to trigger interrupt (Mengdong Lin)
>>
>> v4: remove an extra #define (Jesse)
>>
>> v5: spec says s/w is responsible for clearing PCI_SWSCI bit 0 too
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h       |    1 +
>>  drivers/gpu/drm/i915/intel_opregion.c |  133 ++++++++++++++++++++++++++++++++-
>>  2 files changed, 131 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 84b95b1..adc2f46 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -225,6 +225,7 @@ struct intel_opregion {
>>         struct opregion_header __iomem *header;
>>         struct opregion_acpi __iomem *acpi;
>>         struct opregion_swsci __iomem *swsci;
>> +       u32 swsci_requested_callbacks;
>>         struct opregion_asle __iomem *asle;
>>         void __iomem *vbt;
>>         u32 __iomem *lid_state;
>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> index cfb8fb6..233cc7f 100644
>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> @@ -36,8 +36,11 @@
>>  #include "i915_drv.h"
>>  #include "intel_drv.h"
>>
>> -#define PCI_ASLE 0xe4
>> -#define PCI_ASLS 0xfc
>> +#define PCI_ASLE               0xe4
>> +#define PCI_ASLS               0xfc
>> +#define PCI_SWSCI              0xe8
>> +#define PCI_SWSCI_SCISEL       (1 << 15)
>> +#define PCI_SWSCI_GSSCIE       (1 << 0)
>>
>>  #define OPREGION_HEADER_OFFSET 0
>>  #define OPREGION_ACPI_OFFSET   0x100
>> @@ -151,6 +154,48 @@ struct opregion_asle {
>>
>>  #define ASLE_CBLV_VALID         (1<<31)
>>
>> +/* Software System Control Interrupt (SWSCI) */
>> +#define SWSCI_SCIC_INDICATOR           (1 << 0)
>> +#define SWSCI_SCIC_MAIN_FUNCTION_SHIFT 1
>> +#define SWSCI_SCIC_MAIN_FUNCTION_MASK  (0xf << 1)
>> +#define SWSCI_SCIC_SUB_FUNCTION_SHIFT  8
>> +#define SWSCI_SCIC_SUB_FUNCTION_MASK   (0x7f << 8)
>
> Shouldn't this be (0xff << 8) since it's bits 15:8 ?
>
>
>> +#define SWSCI_SCIC_EXIT_STATUS_SHIFT   5
>> +#define SWSCI_SCIC_EXIT_STATUS_MASK    (7 << 5)
>> +#define SWSCI_SCIC_EXIT_STATUS_SUCCESS 1
>> +
>> +#define SWSCI_FUNCTION_CODE(main, sub) \
>> +       ((main) << SWSCI_SCIC_MAIN_FUNCTION_SHIFT | \
>> +        (sub) << SWSCI_SCIC_SUB_FUNCTION_SHIFT)
>> +
>> +/* SWSCI: Get BIOS Data (GBDA) */
>> +#define SWSCI_GBDA                     4
>> +#define SWSCI_GBDA_SUPPORTED_CALLS     SWSCI_FUNCTION_CODE(SWSCI_GBDA, 0)
>> +#define SWSCI_GBDA_REQUESTED_CALLBACKS SWSCI_FUNCTION_CODE(SWSCI_GBDA, 1)
>> +#define SWSCI_GBDA_BOOT_DISPLAY_PREF   SWSCI_FUNCTION_CODE(SWSCI_GBDA, 4)
>> +#define SWSCI_GBDA_PANEL_DETAILS       SWSCI_FUNCTION_CODE(SWSCI_GBDA, 5)
>> +#define SWSCI_GBDA_TV_STANDARD         SWSCI_FUNCTION_CODE(SWSCI_GBDA, 6)
>
> The newer spec says "reserved" here. But let's leave your definition
> until they assign it again to something else :) (same thing for the
> other TV bit below)
>
>
>> +#define SWSCI_GBDA_INTERNAL_GRAPHICS   SWSCI_FUNCTION_CODE(SWSCI_GBDA, 7)
>> +#define SWSCI_GBDA_SPREAD_SPECTRUM     SWSCI_FUNCTION_CODE(SWSCI_GBDA, 10)
>> +
>> +/* SWSCI: System BIOS Callbacks (SBCB) */
>> +#define SWSCI_SBCB                     6
>> +#define SWSCI_SBCB_SUPPORTED_CALLBACKS SWSCI_FUNCTION_CODE(SWSCI_SBCB, 0)
>> +#define SWSCI_SBCB_INIT_COMPLETION     SWSCI_FUNCTION_CODE(SWSCI_SBCB, 1)
>> +#define SWSCI_SBCB_PRE_HIRES_SET_MODE  SWSCI_FUNCTION_CODE(SWSCI_SBCB, 3)
>> +#define SWSCI_SBCB_POST_HIRES_SET_MODE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 4)
>> +#define SWSCI_SBCB_DISPLAY_SWITCH      SWSCI_FUNCTION_CODE(SWSCI_SBCB, 5)
>> +#define SWSCI_SBCB_SET_TV_FORMAT       SWSCI_FUNCTION_CODE(SWSCI_SBCB, 6)
>> +#define SWSCI_SBCB_ADAPTER_POWER_STATE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 7)
>> +#define SWSCI_SBCB_DISPLAY_POWER_STATE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 8)
>> +#define SWSCI_SBCB_SET_BOOT_DISPLAY    SWSCI_FUNCTION_CODE(SWSCI_SBCB, 9)
>> +#define SWSCI_SBCB_SET_INTERNAL_GFX    SWSCI_FUNCTION_CODE(SWSCI_SBCB, 11)
>> +#define SWSCI_SBCB_POST_HIRES_TO_DOS_FS        SWSCI_FUNCTION_CODE(SWSCI_SBCB, 16)
>> +#define SWSCI_SBCB_SUSPEND_RESUME      SWSCI_FUNCTION_CODE(SWSCI_SBCB, 17)
>> +#define SWSCI_SBCB_SET_SPREAD_SPECTRUM SWSCI_FUNCTION_CODE(SWSCI_SBCB, 18)
>> +#define SWSCI_SBCB_POST_VBE_PM         SWSCI_FUNCTION_CODE(SWSCI_SBCB, 19)
>> +#define SWSCI_SBCB_ENABLE_DISABLE_AUDIO        SWSCI_FUNCTION_CODE(SWSCI_SBCB, 21)
>> +
>>  #define ACPI_OTHER_OUTPUT (0<<8)
>>  #define ACPI_VGA_OUTPUT (1<<8)
>>  #define ACPI_TV_OUTPUT (2<<8)
>> @@ -158,6 +203,84 @@ struct opregion_asle {
>>  #define ACPI_LVDS_OUTPUT (4<<8)
>>
>>  #ifdef CONFIG_ACPI
>> +static int swsci(struct drm_device *dev, u32 function, u32 parm, u32 *parm_out)
>> +{
>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>> +       struct opregion_swsci __iomem *swsci = dev_priv->opregion.swsci;
>> +       u32 main_function, sub_function, scic;
>> +       u16 pci_swsci;
>> +
>> +       if (!swsci)
>> +               return 0;
>
> You also use "return 0" for success below. I'd return -ESOMETHING
> here. Perhaps also print an error message.
>
>
>> +
>> +       main_function = (function & SWSCI_SCIC_MAIN_FUNCTION_MASK) >>
>> +               SWSCI_SCIC_MAIN_FUNCTION_SHIFT;
>> +       sub_function = (function & SWSCI_SCIC_SUB_FUNCTION_MASK) >>
>> +               SWSCI_SCIC_SUB_FUNCTION_SHIFT;
>> +
>> +       if (main_function == SWSCI_SBCB && sub_function != 0) {
>> +               /*
>> +                * SBCB sub-function codes correspond to the bits in requested
>> +                * callbacks, except for bit 0 which is reserved. The driver
>> +                * must not call interfaces that are not specifically requested
>> +                * by the bios.
>> +                */
>> +               if ((dev_priv->opregion.swsci_requested_callbacks &
>> +                    (1 << sub_function)) == 0)
>
> I think you have to do (1 << sub_funcition -1) to make it correct.
>
>
>> +                       return 0;
>
> I'd return -ESOMETHINGELSE here too, possibly printing an error message.
>
>
>> +       }
>> +
>> +       /* XXX: The spec tells us to do this, but we are the only user... */
>
> I'd remove the "XXX: " substring from the comment. The spec suggests
> only graphics uses this specific interface, but it doesn't say that
> other events happening on the system won't cause this bit to be
> flipped. I don't know... the code seems correct, so the "XXX" code may
> be misleading.
>
> The sad thing is that there's also no guarantees that someone won't
> start using the interface just after you read this bit and before
> writing it to 1... We may need dev_priv->swsci_lock depending on how
> we use this function (I haven't checked the next patches yet).
>
>
>> +       scic = ioread32(&swsci->scic);
>> +       if (scic & SWSCI_SCIC_INDICATOR) {
>> +               DRM_DEBUG_DRIVER("SWSCI request already in progress\n");
>> +               return -EBUSY;
>> +       }
>> +
>> +       scic = function | SWSCI_SCIC_INDICATOR;
>> +
>> +       iowrite32(parm, &swsci->parm);
>> +       iowrite32(scic, &swsci->scic);
>> +
>
>
> From what I understood of the spec, from the line below...
>
>> +       /* Ensure SCI event is selected and event trigger is cleared. */
>> +       pci_read_config_word(dev->pdev, PCI_SWSCI, &pci_swsci);
>> +       if (!(pci_swsci & PCI_SWSCI_SCISEL) || (pci_swsci & PCI_SWSCI_GSSCIE)) {
>> +               pci_swsci |= PCI_SWSCI_SCISEL;
>> +               pci_swsci &= ~PCI_SWSCI_GSSCIE;
>> +               pci_write_config_word(dev->pdev, PCI_SWSCI, pci_swsci);
>> +       }
>> +
>> +       /* Use event trigger to tell bios to check the mail. */
>> +       pci_swsci |= PCI_SWSCI_GSSCIE;
>> +       pci_write_config_word(dev->pdev, PCI_SWSCI, pci_swsci);
>
> ... to the line above, we don't need this code. My understanding is
> that the BIOS will do all these things automagically for us, so if we
> write these regs we may even be racing with it, especially since we
> already wrote swsci->scic. Do we really need this code?
>
>
>> +
>> +       /*
>> +        * Poll for the result. The spec says nothing about timing out; add an
>> +        * arbitrary timeout to not hang if the bios fails.
>> +        */
>
> The spec does mention a timeout, check the description of DSLP: Driver
> Sleep Timeout.
>
>
>> +#define C (((scic = ioread32(&swsci->scic)) & SWSCI_SCIC_INDICATOR) == 0)
>> +       if (wait_for(C, 500)) {
>> +               DRM_DEBUG_DRIVER("SWSCI request timed out\n");
>> +               return -ETIMEDOUT;
>> +       }
>> +
>> +       scic = (scic & SWSCI_SCIC_EXIT_STATUS_MASK) >>
>> +               SWSCI_SCIC_EXIT_STATUS_SHIFT;
>> +
>> +       /* Note: scic == 0 is an error! */
>> +       if (scic != SWSCI_SCIC_EXIT_STATUS_SUCCESS) {
>> +               DRM_DEBUG_DRIVER("SWSCI request error %u\n", scic);
>> +               return -EINVAL; /* XXX */
>
> Why XXX above? To me, a XXX means there's something on the code that
> must be fixed.
>
>
>> +       }
>> +
>> +       if (parm_out)
>> +               *parm_out = ioread32(&swsci->parm);
>> +
>> +       return 0;
>> +
>> +#undef C
>> +}
>> +
>>  static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>>  {
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -488,8 +611,12 @@ int intel_opregion_setup(struct drm_device *dev)
>>         }
>>
>>         if (mboxes & MBOX_SWSCI) {
>> -               DRM_DEBUG_DRIVER("SWSCI supported\n");
>>                 opregion->swsci = base + OPREGION_SWSCI_OFFSET;
>> +
>> +               swsci(dev, SWSCI_GBDA_REQUESTED_CALLBACKS, 0,
>> +                     &opregion->swsci_requested_callbacks);
>
> No error checking?

Also, the "swsci" declaration is under "#ifdef CONFIG_ACPI", but not this call.


>
>
>> +               DRM_DEBUG_DRIVER("SWSCI supported, requested callbacks %08x\n",
>> +                                opregion->swsci_requested_callbacks);
>>         }
>>         if (mboxes & MBOX_ASLE) {
>>                 DRM_DEBUG_DRIVER("ASLE supported\n");
>> --
>> 1.7.10.4
>>
>
>
>
> --
> Paulo Zanoni



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: add plumbing for SWSCI

[Jani Nikula]

On Thu, 29 Aug 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> Hi
>
> 2013/8/28 Jani Nikula <jani.nikula@intel.com>:
>> SWSCI is a driver to bios call interface.
>>
>> This checks for SWSCI availability and bios requested callbacks, and
>> filters out any calls that shouldn't happen. This way the callers don't
>> need to do the checks all over the place.
>>
>> v2: silence some checkpatch nagging
>>
>> v3: set PCI_SWSCI bit 0 to trigger interrupt (Mengdong Lin)
>>
>> v4: remove an extra #define (Jesse)
>>
>> v5: spec says s/w is responsible for clearing PCI_SWSCI bit 0 too
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h       |    1 +
>>  drivers/gpu/drm/i915/intel_opregion.c |  133 ++++++++++++++++++++++++++++++++-
>>  2 files changed, 131 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 84b95b1..adc2f46 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -225,6 +225,7 @@ struct intel_opregion {
>>         struct opregion_header __iomem *header;
>>         struct opregion_acpi __iomem *acpi;
>>         struct opregion_swsci __iomem *swsci;
>> +       u32 swsci_requested_callbacks;
>>         struct opregion_asle __iomem *asle;
>>         void __iomem *vbt;
>>         u32 __iomem *lid_state;
>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> index cfb8fb6..233cc7f 100644
>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> @@ -36,8 +36,11 @@
>>  #include "i915_drv.h"
>>  #include "intel_drv.h"
>>
>> -#define PCI_ASLE 0xe4
>> -#define PCI_ASLS 0xfc
>> +#define PCI_ASLE               0xe4
>> +#define PCI_ASLS               0xfc
>> +#define PCI_SWSCI              0xe8
>> +#define PCI_SWSCI_SCISEL       (1 << 15)
>> +#define PCI_SWSCI_GSSCIE       (1 << 0)
>>
>>  #define OPREGION_HEADER_OFFSET 0
>>  #define OPREGION_ACPI_OFFSET   0x100
>> @@ -151,6 +154,48 @@ struct opregion_asle {
>>
>>  #define ASLE_CBLV_VALID         (1<<31)
>>
>> +/* Software System Control Interrupt (SWSCI) */
>> +#define SWSCI_SCIC_INDICATOR           (1 << 0)
>> +#define SWSCI_SCIC_MAIN_FUNCTION_SHIFT 1
>> +#define SWSCI_SCIC_MAIN_FUNCTION_MASK  (0xf << 1)
>> +#define SWSCI_SCIC_SUB_FUNCTION_SHIFT  8
>> +#define SWSCI_SCIC_SUB_FUNCTION_MASK   (0x7f << 8)
>
> Shouldn't this be (0xff << 8) since it's bits 15:8 ?

In my spec, section 4.1.1: "Function codes are broken down into two
parts the main function code (SCIC bits [4:1]) and the sub-function code
(SCIC bits [14:8])."

>> +#define SWSCI_SCIC_EXIT_STATUS_SHIFT   5
>> +#define SWSCI_SCIC_EXIT_STATUS_MASK    (7 << 5)
>> +#define SWSCI_SCIC_EXIT_STATUS_SUCCESS 1
>> +
>> +#define SWSCI_FUNCTION_CODE(main, sub) \
>> +       ((main) << SWSCI_SCIC_MAIN_FUNCTION_SHIFT | \
>> +        (sub) << SWSCI_SCIC_SUB_FUNCTION_SHIFT)
>> +
>> +/* SWSCI: Get BIOS Data (GBDA) */
>> +#define SWSCI_GBDA                     4
>> +#define SWSCI_GBDA_SUPPORTED_CALLS     SWSCI_FUNCTION_CODE(SWSCI_GBDA, 0)
>> +#define SWSCI_GBDA_REQUESTED_CALLBACKS SWSCI_FUNCTION_CODE(SWSCI_GBDA, 1)
>> +#define SWSCI_GBDA_BOOT_DISPLAY_PREF   SWSCI_FUNCTION_CODE(SWSCI_GBDA, 4)
>> +#define SWSCI_GBDA_PANEL_DETAILS       SWSCI_FUNCTION_CODE(SWSCI_GBDA, 5)
>> +#define SWSCI_GBDA_TV_STANDARD         SWSCI_FUNCTION_CODE(SWSCI_GBDA, 6)
>
> The newer spec says "reserved" here. But let's leave your definition
> until they assign it again to something else :) (same thing for the
> other TV bit below)

Okay. :)

>> +#define SWSCI_GBDA_INTERNAL_GRAPHICS   SWSCI_FUNCTION_CODE(SWSCI_GBDA, 7)
>> +#define SWSCI_GBDA_SPREAD_SPECTRUM     SWSCI_FUNCTION_CODE(SWSCI_GBDA, 10)
>> +
>> +/* SWSCI: System BIOS Callbacks (SBCB) */
>> +#define SWSCI_SBCB                     6
>> +#define SWSCI_SBCB_SUPPORTED_CALLBACKS SWSCI_FUNCTION_CODE(SWSCI_SBCB, 0)
>> +#define SWSCI_SBCB_INIT_COMPLETION     SWSCI_FUNCTION_CODE(SWSCI_SBCB, 1)
>> +#define SWSCI_SBCB_PRE_HIRES_SET_MODE  SWSCI_FUNCTION_CODE(SWSCI_SBCB, 3)
>> +#define SWSCI_SBCB_POST_HIRES_SET_MODE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 4)
>> +#define SWSCI_SBCB_DISPLAY_SWITCH      SWSCI_FUNCTION_CODE(SWSCI_SBCB, 5)
>> +#define SWSCI_SBCB_SET_TV_FORMAT       SWSCI_FUNCTION_CODE(SWSCI_SBCB, 6)
>> +#define SWSCI_SBCB_ADAPTER_POWER_STATE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 7)
>> +#define SWSCI_SBCB_DISPLAY_POWER_STATE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 8)
>> +#define SWSCI_SBCB_SET_BOOT_DISPLAY    SWSCI_FUNCTION_CODE(SWSCI_SBCB, 9)
>> +#define SWSCI_SBCB_SET_INTERNAL_GFX    SWSCI_FUNCTION_CODE(SWSCI_SBCB, 11)
>> +#define SWSCI_SBCB_POST_HIRES_TO_DOS_FS        SWSCI_FUNCTION_CODE(SWSCI_SBCB, 16)
>> +#define SWSCI_SBCB_SUSPEND_RESUME      SWSCI_FUNCTION_CODE(SWSCI_SBCB, 17)
>> +#define SWSCI_SBCB_SET_SPREAD_SPECTRUM SWSCI_FUNCTION_CODE(SWSCI_SBCB, 18)
>> +#define SWSCI_SBCB_POST_VBE_PM         SWSCI_FUNCTION_CODE(SWSCI_SBCB, 19)
>> +#define SWSCI_SBCB_ENABLE_DISABLE_AUDIO        SWSCI_FUNCTION_CODE(SWSCI_SBCB, 21)
>> +
>>  #define ACPI_OTHER_OUTPUT (0<<8)
>>  #define ACPI_VGA_OUTPUT (1<<8)
>>  #define ACPI_TV_OUTPUT (2<<8)
>> @@ -158,6 +203,84 @@ struct opregion_asle {
>>  #define ACPI_LVDS_OUTPUT (4<<8)
>>
>>  #ifdef CONFIG_ACPI
>> +static int swsci(struct drm_device *dev, u32 function, u32 parm, u32 *parm_out)
>> +{
>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>> +       struct opregion_swsci __iomem *swsci = dev_priv->opregion.swsci;
>> +       u32 main_function, sub_function, scic;
>> +       u16 pci_swsci;
>> +
>> +       if (!swsci)
>> +               return 0;
>
> You also use "return 0" for success below. I'd return -ESOMETHING
> here. Perhaps also print an error message.

My reasoning was, if the platform doesn't support SWSCI (this one here),
or does not request a specific sub-function (the one below), we just
carry on. I can add a -ESOMETHING if you like, but I think an error
message would pollute dmesg too much. Or else we'd have to burden the
call sites with checks if we can call the functions.

>> +
>> +       main_function = (function & SWSCI_SCIC_MAIN_FUNCTION_MASK) >>
>> +               SWSCI_SCIC_MAIN_FUNCTION_SHIFT;
>> +       sub_function = (function & SWSCI_SCIC_SUB_FUNCTION_MASK) >>
>> +               SWSCI_SCIC_SUB_FUNCTION_SHIFT;
>> +
>> +       if (main_function == SWSCI_SBCB && sub_function != 0) {
>> +               /*
>> +                * SBCB sub-function codes correspond to the bits in requested
>> +                * callbacks, except for bit 0 which is reserved. The driver
>> +                * must not call interfaces that are not specifically requested
>> +                * by the bios.
>> +                */
>> +               if ((dev_priv->opregion.swsci_requested_callbacks &
>> +                    (1 << sub_function)) == 0)
>
> I think you have to do (1 << sub_funcition -1) to make it correct.

I think you're off by one, please double check.

>> +                       return 0;
>
> I'd return -ESOMETHINGELSE here too, possibly printing an error
> message.

See above.

>> +       }
>> +
>> +       /* XXX: The spec tells us to do this, but we are the only user... */
>
> I'd remove the "XXX: " substring from the comment. The spec suggests
> only graphics uses this specific interface, but it doesn't say that
> other events happening on the system won't cause this bit to be
> flipped. I don't know... the code seems correct, so the "XXX" code may
> be misleading.

I'll drop it.

> The sad thing is that there's also no guarantees that someone won't
> start using the interface just after you read this bit and before
> writing it to 1... We may need dev_priv->swsci_lock depending on how
> we use this function (I haven't checked the next patches yet).

I think I thought we'd be covered by struct_mutex here, but perhaps that
was before the adapter notification... how's the pc8 call paths?

>> +       scic = ioread32(&swsci->scic);
>> +       if (scic & SWSCI_SCIC_INDICATOR) {
>> +               DRM_DEBUG_DRIVER("SWSCI request already in progress\n");
>> +               return -EBUSY;
>> +       }
>> +
>> +       scic = function | SWSCI_SCIC_INDICATOR;
>> +
>> +       iowrite32(parm, &swsci->parm);
>> +       iowrite32(scic, &swsci->scic);
>> +
>
>
> From what I understood of the spec, from the line below...
>
>> +       /* Ensure SCI event is selected and event trigger is cleared. */
>> +       pci_read_config_word(dev->pdev, PCI_SWSCI, &pci_swsci);
>> +       if (!(pci_swsci & PCI_SWSCI_SCISEL) || (pci_swsci & PCI_SWSCI_GSSCIE)) {
>> +               pci_swsci |= PCI_SWSCI_SCISEL;
>> +               pci_swsci &= ~PCI_SWSCI_GSSCIE;
>> +               pci_write_config_word(dev->pdev, PCI_SWSCI, pci_swsci);
>> +       }
>> +
>> +       /* Use event trigger to tell bios to check the mail. */
>> +       pci_swsci |= PCI_SWSCI_GSSCIE;
>> +       pci_write_config_word(dev->pdev, PCI_SWSCI, pci_swsci);
>
> ... to the line above, we don't need this code. My understanding is
> that the BIOS will do all these things automagically for us, so if we
> write these regs we may even be racing with it, especially since we
> already wrote swsci->scic. Do we really need this code?

We need it. It gives me some consolation that I made the same
interpretation as you did, before Mengdong Lin corrected me... The
opregion spec is really unclear about this, and it's easy to confuse
between the SCIC register and the PCI SWSCI register. Have a look at
SWSCI in the bspec PCI section. Only that triggers the event.

>> +
>> +       /*
>> +        * Poll for the result. The spec says nothing about timing out; add an
>> +        * arbitrary timeout to not hang if the bios fails.
>> +        */
>
> The spec does mention a timeout, check the description of DSLP: Driver
> Sleep Timeout.

Ah yes, thanks. Any idea what the unit for DSLP might be? Not in my
spec...

>> +#define C (((scic = ioread32(&swsci->scic)) & SWSCI_SCIC_INDICATOR) == 0)
>> +       if (wait_for(C, 500)) {
>> +               DRM_DEBUG_DRIVER("SWSCI request timed out\n");
>> +               return -ETIMEDOUT;
>> +       }
>> +
>> +       scic = (scic & SWSCI_SCIC_EXIT_STATUS_MASK) >>
>> +               SWSCI_SCIC_EXIT_STATUS_SHIFT;
>> +
>> +       /* Note: scic == 0 is an error! */
>> +       if (scic != SWSCI_SCIC_EXIT_STATUS_SUCCESS) {
>> +               DRM_DEBUG_DRIVER("SWSCI request error %u\n", scic);
>> +               return -EINVAL; /* XXX */
>
> Why XXX above? To me, a XXX means there's something on the code that
> must be fixed.

s/XXX/check if there's an appropriate error code we could use/ :)

>> +       }
>> +
>> +       if (parm_out)
>> +               *parm_out = ioread32(&swsci->parm);
>> +
>> +       return 0;
>> +
>> +#undef C
>> +}
>> +
>>  static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>>  {
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -488,8 +611,12 @@ int intel_opregion_setup(struct drm_device *dev)
>>         }
>>
>>         if (mboxes & MBOX_SWSCI) {
>> -               DRM_DEBUG_DRIVER("SWSCI supported\n");
>>                 opregion->swsci = base + OPREGION_SWSCI_OFFSET;
>> +
>> +               swsci(dev, SWSCI_GBDA_REQUESTED_CALLBACKS, 0,
>> +                     &opregion->swsci_requested_callbacks);
>
> No error checking?

Will fix.

Thanks for the review,
Jani.

>
>
>> +               DRM_DEBUG_DRIVER("SWSCI supported, requested callbacks %08x\n",
>> +                                opregion->swsci_requested_callbacks);
>>         }
>>         if (mboxes & MBOX_ASLE) {
>>                 DRM_DEBUG_DRIVER("ASLE supported\n");
>> --
>> 1.7.10.4
>>
>
>
>
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: add plumbing for SWSCI

[Paulo Zanoni]

2013/8/29 Jani Nikula <jani.nikula@intel.com>:
> On Thu, 29 Aug 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
>> Hi
>>
>> 2013/8/28 Jani Nikula <jani.nikula@intel.com>:
>>> SWSCI is a driver to bios call interface.
>>>
>>> This checks for SWSCI availability and bios requested callbacks, and
>>> filters out any calls that shouldn't happen. This way the callers don't
>>> need to do the checks all over the place.
>>>
>>> v2: silence some checkpatch nagging
>>>
>>> v3: set PCI_SWSCI bit 0 to trigger interrupt (Mengdong Lin)
>>>
>>> v4: remove an extra #define (Jesse)
>>>
>>> v5: spec says s/w is responsible for clearing PCI_SWSCI bit 0 too
>>>
>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_drv.h       |    1 +
>>>  drivers/gpu/drm/i915/intel_opregion.c |  133 ++++++++++++++++++++++++++++++++-
>>>  2 files changed, 131 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 84b95b1..adc2f46 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -225,6 +225,7 @@ struct intel_opregion {
>>>         struct opregion_header __iomem *header;
>>>         struct opregion_acpi __iomem *acpi;
>>>         struct opregion_swsci __iomem *swsci;
>>> +       u32 swsci_requested_callbacks;
>>>         struct opregion_asle __iomem *asle;
>>>         void __iomem *vbt;
>>>         u32 __iomem *lid_state;
>>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>>> index cfb8fb6..233cc7f 100644
>>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>>> @@ -36,8 +36,11 @@
>>>  #include "i915_drv.h"
>>>  #include "intel_drv.h"
>>>
>>> -#define PCI_ASLE 0xe4
>>> -#define PCI_ASLS 0xfc
>>> +#define PCI_ASLE               0xe4
>>> +#define PCI_ASLS               0xfc
>>> +#define PCI_SWSCI              0xe8
>>> +#define PCI_SWSCI_SCISEL       (1 << 15)
>>> +#define PCI_SWSCI_GSSCIE       (1 << 0)
>>>
>>>  #define OPREGION_HEADER_OFFSET 0
>>>  #define OPREGION_ACPI_OFFSET   0x100
>>> @@ -151,6 +154,48 @@ struct opregion_asle {
>>>
>>>  #define ASLE_CBLV_VALID         (1<<31)
>>>
>>> +/* Software System Control Interrupt (SWSCI) */
>>> +#define SWSCI_SCIC_INDICATOR           (1 << 0)
>>> +#define SWSCI_SCIC_MAIN_FUNCTION_SHIFT 1
>>> +#define SWSCI_SCIC_MAIN_FUNCTION_MASK  (0xf << 1)
>>> +#define SWSCI_SCIC_SUB_FUNCTION_SHIFT  8
>>> +#define SWSCI_SCIC_SUB_FUNCTION_MASK   (0x7f << 8)
>>
>> Shouldn't this be (0xff << 8) since it's bits 15:8 ?
>
> In my spec, section 4.1.1: "Function codes are broken down into two
> parts the main function code (SCIC bits [4:1]) and the sub-function code
> (SCIC bits [14:8])."

In the same spec, section 3.3.1, table 3-35 says it's 15:8 :(
We're both right and wrong...


>
>>> +#define SWSCI_SCIC_EXIT_STATUS_SHIFT   5
>>> +#define SWSCI_SCIC_EXIT_STATUS_MASK    (7 << 5)
>>> +#define SWSCI_SCIC_EXIT_STATUS_SUCCESS 1
>>> +
>>> +#define SWSCI_FUNCTION_CODE(main, sub) \
>>> +       ((main) << SWSCI_SCIC_MAIN_FUNCTION_SHIFT | \
>>> +        (sub) << SWSCI_SCIC_SUB_FUNCTION_SHIFT)
>>> +
>>> +/* SWSCI: Get BIOS Data (GBDA) */
>>> +#define SWSCI_GBDA                     4
>>> +#define SWSCI_GBDA_SUPPORTED_CALLS     SWSCI_FUNCTION_CODE(SWSCI_GBDA, 0)
>>> +#define SWSCI_GBDA_REQUESTED_CALLBACKS SWSCI_FUNCTION_CODE(SWSCI_GBDA, 1)
>>> +#define SWSCI_GBDA_BOOT_DISPLAY_PREF   SWSCI_FUNCTION_CODE(SWSCI_GBDA, 4)
>>> +#define SWSCI_GBDA_PANEL_DETAILS       SWSCI_FUNCTION_CODE(SWSCI_GBDA, 5)
>>> +#define SWSCI_GBDA_TV_STANDARD         SWSCI_FUNCTION_CODE(SWSCI_GBDA, 6)
>>
>> The newer spec says "reserved" here. But let's leave your definition
>> until they assign it again to something else :) (same thing for the
>> other TV bit below)
>
> Okay. :)
>
>>> +#define SWSCI_GBDA_INTERNAL_GRAPHICS   SWSCI_FUNCTION_CODE(SWSCI_GBDA, 7)
>>> +#define SWSCI_GBDA_SPREAD_SPECTRUM     SWSCI_FUNCTION_CODE(SWSCI_GBDA, 10)
>>> +
>>> +/* SWSCI: System BIOS Callbacks (SBCB) */
>>> +#define SWSCI_SBCB                     6
>>> +#define SWSCI_SBCB_SUPPORTED_CALLBACKS SWSCI_FUNCTION_CODE(SWSCI_SBCB, 0)
>>> +#define SWSCI_SBCB_INIT_COMPLETION     SWSCI_FUNCTION_CODE(SWSCI_SBCB, 1)
>>> +#define SWSCI_SBCB_PRE_HIRES_SET_MODE  SWSCI_FUNCTION_CODE(SWSCI_SBCB, 3)
>>> +#define SWSCI_SBCB_POST_HIRES_SET_MODE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 4)
>>> +#define SWSCI_SBCB_DISPLAY_SWITCH      SWSCI_FUNCTION_CODE(SWSCI_SBCB, 5)
>>> +#define SWSCI_SBCB_SET_TV_FORMAT       SWSCI_FUNCTION_CODE(SWSCI_SBCB, 6)
>>> +#define SWSCI_SBCB_ADAPTER_POWER_STATE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 7)
>>> +#define SWSCI_SBCB_DISPLAY_POWER_STATE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 8)
>>> +#define SWSCI_SBCB_SET_BOOT_DISPLAY    SWSCI_FUNCTION_CODE(SWSCI_SBCB, 9)
>>> +#define SWSCI_SBCB_SET_INTERNAL_GFX    SWSCI_FUNCTION_CODE(SWSCI_SBCB, 11)
>>> +#define SWSCI_SBCB_POST_HIRES_TO_DOS_FS        SWSCI_FUNCTION_CODE(SWSCI_SBCB, 16)
>>> +#define SWSCI_SBCB_SUSPEND_RESUME      SWSCI_FUNCTION_CODE(SWSCI_SBCB, 17)
>>> +#define SWSCI_SBCB_SET_SPREAD_SPECTRUM SWSCI_FUNCTION_CODE(SWSCI_SBCB, 18)
>>> +#define SWSCI_SBCB_POST_VBE_PM         SWSCI_FUNCTION_CODE(SWSCI_SBCB, 19)
>>> +#define SWSCI_SBCB_ENABLE_DISABLE_AUDIO        SWSCI_FUNCTION_CODE(SWSCI_SBCB, 21)
>>> +
>>>  #define ACPI_OTHER_OUTPUT (0<<8)
>>>  #define ACPI_VGA_OUTPUT (1<<8)
>>>  #define ACPI_TV_OUTPUT (2<<8)
>>> @@ -158,6 +203,84 @@ struct opregion_asle {
>>>  #define ACPI_LVDS_OUTPUT (4<<8)
>>>
>>>  #ifdef CONFIG_ACPI
>>> +static int swsci(struct drm_device *dev, u32 function, u32 parm, u32 *parm_out)
>>> +{
>>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>>> +       struct opregion_swsci __iomem *swsci = dev_priv->opregion.swsci;
>>> +       u32 main_function, sub_function, scic;
>>> +       u16 pci_swsci;
>>> +
>>> +       if (!swsci)
>>> +               return 0;
>>
>> You also use "return 0" for success below. I'd return -ESOMETHING
>> here. Perhaps also print an error message.
>
> My reasoning was, if the platform doesn't support SWSCI (this one here),
> or does not request a specific sub-function (the one below), we just
> carry on. I can add a -ESOMETHING if you like, but I think an error
> message would pollute dmesg too much. Or else we'd have to burden the
> call sites with checks if we can call the functions.

Makes sense.


>
>>> +
>>> +       main_function = (function & SWSCI_SCIC_MAIN_FUNCTION_MASK) >>
>>> +               SWSCI_SCIC_MAIN_FUNCTION_SHIFT;
>>> +       sub_function = (function & SWSCI_SCIC_SUB_FUNCTION_MASK) >>
>>> +               SWSCI_SCIC_SUB_FUNCTION_SHIFT;
>>> +
>>> +       if (main_function == SWSCI_SBCB && sub_function != 0) {
>>> +               /*
>>> +                * SBCB sub-function codes correspond to the bits in requested
>>> +                * callbacks, except for bit 0 which is reserved. The driver
>>> +                * must not call interfaces that are not specifically requested
>>> +                * by the bios.
>>> +                */
>>> +               if ((dev_priv->opregion.swsci_requested_callbacks &
>>> +                    (1 << sub_function)) == 0)
>>
>> I think you have to do (1 << sub_funcition -1) to make it correct.
>
> I think you're off by one, please double check.

Ok, it seems you were using spec version 0.8 from December 2012,
right? Mine is version 1.0 from June 2013. The sub-function codes are
still the same, but the callbacks reply changed. Now I see some bits
moved even 5 positions (e.g, enable/disable audio) while the ones I
checked only moved 1 position. Trolls!!! Where's my sword?


>
>>> +                       return 0;
>>
>> I'd return -ESOMETHINGELSE here too, possibly printing an error
>> message.
>
> See above.
>
>>> +       }
>>> +
>>> +       /* XXX: The spec tells us to do this, but we are the only user... */
>>
>> I'd remove the "XXX: " substring from the comment. The spec suggests
>> only graphics uses this specific interface, but it doesn't say that
>> other events happening on the system won't cause this bit to be
>> flipped. I don't know... the code seems correct, so the "XXX" code may
>> be misleading.
>
> I'll drop it.
>
>> The sad thing is that there's also no guarantees that someone won't
>> start using the interface just after you read this bit and before
>> writing it to 1... We may need dev_priv->swsci_lock depending on how
>> we use this function (I haven't checked the next patches yet).
>
> I think I thought we'd be covered by struct_mutex here, but perhaps that
> was before the adapter notification... how's the pc8 call paths?

If we only call this while modesetting and entering/leaving PC8, we
should be fine. But we never know what's going to be needed in the
future. For now, we can leave as it is.


>
>>> +       scic = ioread32(&swsci->scic);
>>> +       if (scic & SWSCI_SCIC_INDICATOR) {
>>> +               DRM_DEBUG_DRIVER("SWSCI request already in progress\n");
>>> +               return -EBUSY;
>>> +       }
>>> +
>>> +       scic = function | SWSCI_SCIC_INDICATOR;
>>> +
>>> +       iowrite32(parm, &swsci->parm);
>>> +       iowrite32(scic, &swsci->scic);
>>> +
>>
>>
>> From what I understood of the spec, from the line below...
>>
>>> +       /* Ensure SCI event is selected and event trigger is cleared. */
>>> +       pci_read_config_word(dev->pdev, PCI_SWSCI, &pci_swsci);
>>> +       if (!(pci_swsci & PCI_SWSCI_SCISEL) || (pci_swsci & PCI_SWSCI_GSSCIE)) {
>>> +               pci_swsci |= PCI_SWSCI_SCISEL;
>>> +               pci_swsci &= ~PCI_SWSCI_GSSCIE;
>>> +               pci_write_config_word(dev->pdev, PCI_SWSCI, pci_swsci);
>>> +       }
>>> +
>>> +       /* Use event trigger to tell bios to check the mail. */
>>> +       pci_swsci |= PCI_SWSCI_GSSCIE;
>>> +       pci_write_config_word(dev->pdev, PCI_SWSCI, pci_swsci);
>>
>> ... to the line above, we don't need this code. My understanding is
>> that the BIOS will do all these things automagically for us, so if we
>> write these regs we may even be racing with it, especially since we
>> already wrote swsci->scic. Do we really need this code?
>
> We need it. It gives me some consolation that I made the same
> interpretation as you did, before Mengdong Lin corrected me... The
> opregion spec is really unclear about this, and it's easy to confuse
> between the SCIC register and the PCI SWSCI register. Have a look at
> SWSCI in the bspec PCI section. Only that triggers the event.

I'll take a look and do some experiments.


>
>>> +
>>> +       /*
>>> +        * Poll for the result. The spec says nothing about timing out; add an
>>> +        * arbitrary timeout to not hang if the bios fails.
>>> +        */
>>
>> The spec does mention a timeout, check the description of DSLP: Driver
>> Sleep Timeout.
>
> Ah yes, thanks. Any idea what the unit for DSLP might be? Not in my
> spec...

On the 1.0 spec, the second paragraph of section 3.3.3 is just
"Timeout Unit will be in milliseconds". I couldn't find anything in
the 0.8 spec... Section 3.3.3 also says that if the timeout value is
0, we should consider 2ms.


>
>>> +#define C (((scic = ioread32(&swsci->scic)) & SWSCI_SCIC_INDICATOR) == 0)
>>> +       if (wait_for(C, 500)) {
>>> +               DRM_DEBUG_DRIVER("SWSCI request timed out\n");
>>> +               return -ETIMEDOUT;
>>> +       }
>>> +
>>> +       scic = (scic & SWSCI_SCIC_EXIT_STATUS_MASK) >>
>>> +               SWSCI_SCIC_EXIT_STATUS_SHIFT;
>>> +
>>> +       /* Note: scic == 0 is an error! */
>>> +       if (scic != SWSCI_SCIC_EXIT_STATUS_SUCCESS) {
>>> +               DRM_DEBUG_DRIVER("SWSCI request error %u\n", scic);
>>> +               return -EINVAL; /* XXX */
>>
>> Why XXX above? To me, a XXX means there's something on the code that
>> must be fixed.
>
> s/XXX/check if there's an appropriate error code we could use/ :)

Ok. I'll leave the decision to you :)


>
>>> +       }
>>> +
>>> +       if (parm_out)
>>> +               *parm_out = ioread32(&swsci->parm);
>>> +
>>> +       return 0;
>>> +
>>> +#undef C
>>> +}
>>> +
>>>  static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>>>  {
>>>         struct drm_i915_private *dev_priv = dev->dev_private;
>>> @@ -488,8 +611,12 @@ int intel_opregion_setup(struct drm_device *dev)
>>>         }
>>>
>>>         if (mboxes & MBOX_SWSCI) {
>>> -               DRM_DEBUG_DRIVER("SWSCI supported\n");
>>>                 opregion->swsci = base + OPREGION_SWSCI_OFFSET;
>>> +
>>> +               swsci(dev, SWSCI_GBDA_REQUESTED_CALLBACKS, 0,
>>> +                     &opregion->swsci_requested_callbacks);
>>
>> No error checking?
>
> Will fix.
>
> Thanks for the review,
> Jani.
>
>>
>>
>>> +               DRM_DEBUG_DRIVER("SWSCI supported, requested callbacks %08x\n",
>>> +                                opregion->swsci_requested_callbacks);
>>>         }
>>>         if (mboxes & MBOX_ASLE) {
>>>                 DRM_DEBUG_DRIVER("ASLE supported\n");
>>> --
>>> 1.7.10.4
>>>
>>
>>
>>
>> --
>> Paulo Zanoni
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH 3/6] drm/i915: add opregion function to notify bios of encoder enable/disable

[Jani Nikula]

The bios interface seems messy, and it's hard to tell what the bios
really wants. At first, only add support for DDI based machines (hsw+),
and see how it turns out.

The spec says to notify prior to power down and after power up. It is
unclear whether it makes a difference.

v2:
 - squash notification function and callers patches together (Daniel)
 - move callers to haswell_crtc_{enable,disable} (Daniel)
 - rename notification function (Chris)

v3:
 - separate notification function and callers again, as it's not clear
   whether the display power state notification is the right thing to do
   after all

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       |    8 +++++
 drivers/gpu/drm/i915/intel_opregion.c |   52 +++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index adc2f46..1703029 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2171,15 +2171,23 @@ static inline bool intel_gmbus_is_forced_bit(struct i2c_adapter *adapter)
 extern void intel_i2c_reset(struct drm_device *dev);
 
 /* intel_opregion.c */
+struct intel_encoder;
 extern int intel_opregion_setup(struct drm_device *dev);
 #ifdef CONFIG_ACPI
 extern void intel_opregion_init(struct drm_device *dev);
 extern void intel_opregion_fini(struct drm_device *dev);
 extern void intel_opregion_asle_intr(struct drm_device *dev);
+extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
+					 bool enable);
 #else
 static inline void intel_opregion_init(struct drm_device *dev) { return; }
 static inline void intel_opregion_fini(struct drm_device *dev) { return; }
 static inline void intel_opregion_asle_intr(struct drm_device *dev) { return; }
+static inline int
+intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, bool enable)
+{
+	return 0;
+}
 #endif
 
 /* intel_acpi.c */
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index a3558ae..72a4af6 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -273,6 +273,58 @@ static int swsci(struct drm_device *dev, u32 function, u32 parm, u32 *parm_out)
 #undef C
 }
 
+#define DISPLAY_TYPE_CRT			0
+#define DISPLAY_TYPE_TV				1
+#define DISPLAY_TYPE_EXTERNAL_FLAT_PANEL	2
+#define DISPLAY_TYPE_INTERNAL_FLAT_PANEL	3
+
+int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
+				  bool enable)
+{
+	struct drm_device *dev = intel_encoder->base.dev;
+	u32 parm = 0;
+	u32 type = 0;
+	u32 port;
+
+	/* don't care about old stuff for now */
+	if (!HAS_DDI(dev))
+		return 0;
+
+	port = intel_ddi_get_encoder_port(intel_encoder);
+	if (port == PORT_E) {
+		port = 0;
+	} else {
+		parm |= 1 << port;
+		port++;
+	}
+
+	if (!enable)
+		parm |= 4 << 8;
+
+	switch (intel_encoder->type) {
+	case INTEL_OUTPUT_ANALOG:
+		type = DISPLAY_TYPE_CRT;
+		break;
+	case INTEL_OUTPUT_UNKNOWN:
+	case INTEL_OUTPUT_DISPLAYPORT:
+	case INTEL_OUTPUT_HDMI:
+		type = DISPLAY_TYPE_EXTERNAL_FLAT_PANEL;
+		break;
+	case INTEL_OUTPUT_LVDS:
+	case INTEL_OUTPUT_EDP:
+		type = DISPLAY_TYPE_INTERNAL_FLAT_PANEL;
+		break;
+	default:
+		DRM_DEBUG_DRIVER("unsupported intel_encoder type %d\n",
+				 intel_encoder->type);
+		return -EINVAL;
+	}
+
+	parm |= type << (16 + port * 3);
+
+	return swsci(dev, SWSCI_SBCB_DISPLAY_POWER_STATE, parm, NULL);
+}
+
 static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-- 
1.7.9.5

Re: [PATCH 3/6] drm/i915: add opregion function to notify bios of encoder enable/disable

[Paulo Zanoni]

Hi

2013/8/23 Jani Nikula <jani.nikula@intel.com>:
> The bios interface seems messy, and it's hard to tell what the bios
> really wants. At first, only add support for DDI based machines (hsw+),
> and see how it turns out.
>
> The spec says to notify prior to power down and after power up. It is
> unclear whether it makes a difference.
>
> v2:
>  - squash notification function and callers patches together (Daniel)
>  - move callers to haswell_crtc_{enable,disable} (Daniel)
>  - rename notification function (Chris)
>
> v3:
>  - separate notification function and callers again, as it's not clear
>    whether the display power state notification is the right thing to do
>    after all
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |    8 +++++
>  drivers/gpu/drm/i915/intel_opregion.c |   52 +++++++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index adc2f46..1703029 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2171,15 +2171,23 @@ static inline bool intel_gmbus_is_forced_bit(struct i2c_adapter *adapter)
>  extern void intel_i2c_reset(struct drm_device *dev);
>
>  /* intel_opregion.c */
> +struct intel_encoder;
>  extern int intel_opregion_setup(struct drm_device *dev);
>  #ifdef CONFIG_ACPI
>  extern void intel_opregion_init(struct drm_device *dev);
>  extern void intel_opregion_fini(struct drm_device *dev);
>  extern void intel_opregion_asle_intr(struct drm_device *dev);
> +extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
> +                                        bool enable);
>  #else
>  static inline void intel_opregion_init(struct drm_device *dev) { return; }
>  static inline void intel_opregion_fini(struct drm_device *dev) { return; }
>  static inline void intel_opregion_asle_intr(struct drm_device *dev) { return; }
> +static inline int
> +intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, bool enable)
> +{
> +       return 0;
> +}
>  #endif
>
>  /* intel_acpi.c */
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index a3558ae..72a4af6 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -273,6 +273,58 @@ static int swsci(struct drm_device *dev, u32 function, u32 parm, u32 *parm_out)
>  #undef C
>  }
>
> +#define DISPLAY_TYPE_CRT                       0
> +#define DISPLAY_TYPE_TV                                1
> +#define DISPLAY_TYPE_EXTERNAL_FLAT_PANEL       2
> +#define DISPLAY_TYPE_INTERNAL_FLAT_PANEL       3
> +
> +int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
> +                                 bool enable)
> +{
> +       struct drm_device *dev = intel_encoder->base.dev;
> +       u32 parm = 0;
> +       u32 type = 0;
> +       u32 port;
> +
> +       /* don't care about old stuff for now */
> +       if (!HAS_DDI(dev))
> +               return 0;
> +
> +       port = intel_ddi_get_encoder_port(intel_encoder);
> +       if (port == PORT_E) {
> +               port = 0;
> +       } else {
> +               parm |= 1 << port;
> +               port++;
> +       }
> +
> +       if (!enable)
> +               parm |= 4 << 8;
> +
> +       switch (intel_encoder->type) {
> +       case INTEL_OUTPUT_ANALOG:
> +               type = DISPLAY_TYPE_CRT;
> +               break;
> +       case INTEL_OUTPUT_UNKNOWN:
> +       case INTEL_OUTPUT_DISPLAYPORT:
> +       case INTEL_OUTPUT_HDMI:
> +               type = DISPLAY_TYPE_EXTERNAL_FLAT_PANEL;
> +               break;
> +       case INTEL_OUTPUT_LVDS:

We don't have LVDS on DDI platforms.


> +       case INTEL_OUTPUT_EDP:
> +               type = DISPLAY_TYPE_INTERNAL_FLAT_PANEL;
> +               break;
> +       default:
> +               DRM_DEBUG_DRIVER("unsupported intel_encoder type %d\n",
> +                                intel_encoder->type);
> +               return -EINVAL;
> +       }
> +
> +       parm |= type << (16 + port * 3);
> +
> +       return swsci(dev, SWSCI_SBCB_DISPLAY_POWER_STATE, parm, NULL);

In patch 2, on the initialization code you call the GBDA request
callbacks function to see which ones will work. Shouldn't you also add
code to call the SBCB request callbacks function and see if
DISPLAY_POWER_STATE is really supported? I know that
DISPLAY_POWER_STATE is supposed to be mandatory, but just checking
won't hurt us. We could maybe even add a code to WARN in case one of
the mandatory callbacks is not available :) This suggestion could be
in a separate patch.

With or without any changes: Reviewed-by: Paulo Zanoni
<paulo.r.zanoni@intel.com>


> +}
> +
>  static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> --
> 1.7.9.5
>



-- 
Paulo Zanoni

Re: [PATCH 3/6] drm/i915: add opregion function to notify bios of encoder enable/disable

[Jani Nikula]

On Thu, 29 Aug 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> Hi
>
> 2013/8/23 Jani Nikula <jani.nikula@intel.com>:
>> The bios interface seems messy, and it's hard to tell what the bios
>> really wants. At first, only add support for DDI based machines (hsw+),
>> and see how it turns out.
>>
>> The spec says to notify prior to power down and after power up. It is
>> unclear whether it makes a difference.
>>
>> v2:
>>  - squash notification function and callers patches together (Daniel)
>>  - move callers to haswell_crtc_{enable,disable} (Daniel)
>>  - rename notification function (Chris)
>>
>> v3:
>>  - separate notification function and callers again, as it's not clear
>>    whether the display power state notification is the right thing to do
>>    after all
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h       |    8 +++++
>>  drivers/gpu/drm/i915/intel_opregion.c |   52 +++++++++++++++++++++++++++++++++
>>  2 files changed, 60 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index adc2f46..1703029 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2171,15 +2171,23 @@ static inline bool intel_gmbus_is_forced_bit(struct i2c_adapter *adapter)
>>  extern void intel_i2c_reset(struct drm_device *dev);
>>
>>  /* intel_opregion.c */
>> +struct intel_encoder;
>>  extern int intel_opregion_setup(struct drm_device *dev);
>>  #ifdef CONFIG_ACPI
>>  extern void intel_opregion_init(struct drm_device *dev);
>>  extern void intel_opregion_fini(struct drm_device *dev);
>>  extern void intel_opregion_asle_intr(struct drm_device *dev);
>> +extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>> +                                        bool enable);
>>  #else
>>  static inline void intel_opregion_init(struct drm_device *dev) { return; }
>>  static inline void intel_opregion_fini(struct drm_device *dev) { return; }
>>  static inline void intel_opregion_asle_intr(struct drm_device *dev) { return; }
>> +static inline int
>> +intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, bool enable)
>> +{
>> +       return 0;
>> +}
>>  #endif
>>
>>  /* intel_acpi.c */
>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> index a3558ae..72a4af6 100644
>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> @@ -273,6 +273,58 @@ static int swsci(struct drm_device *dev, u32 function, u32 parm, u32 *parm_out)
>>  #undef C
>>  }
>>
>> +#define DISPLAY_TYPE_CRT                       0
>> +#define DISPLAY_TYPE_TV                                1
>> +#define DISPLAY_TYPE_EXTERNAL_FLAT_PANEL       2
>> +#define DISPLAY_TYPE_INTERNAL_FLAT_PANEL       3
>> +
>> +int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>> +                                 bool enable)
>> +{
>> +       struct drm_device *dev = intel_encoder->base.dev;
>> +       u32 parm = 0;
>> +       u32 type = 0;
>> +       u32 port;
>> +
>> +       /* don't care about old stuff for now */
>> +       if (!HAS_DDI(dev))
>> +               return 0;
>> +
>> +       port = intel_ddi_get_encoder_port(intel_encoder);
>> +       if (port == PORT_E) {
>> +               port = 0;
>> +       } else {
>> +               parm |= 1 << port;
>> +               port++;
>> +       }
>> +
>> +       if (!enable)
>> +               parm |= 4 << 8;
>> +
>> +       switch (intel_encoder->type) {
>> +       case INTEL_OUTPUT_ANALOG:
>> +               type = DISPLAY_TYPE_CRT;
>> +               break;
>> +       case INTEL_OUTPUT_UNKNOWN:
>> +       case INTEL_OUTPUT_DISPLAYPORT:
>> +       case INTEL_OUTPUT_HDMI:
>> +               type = DISPLAY_TYPE_EXTERNAL_FLAT_PANEL;
>> +               break;
>> +       case INTEL_OUTPUT_LVDS:
>
> We don't have LVDS on DDI platforms.

Right.

>> +       case INTEL_OUTPUT_EDP:
>> +               type = DISPLAY_TYPE_INTERNAL_FLAT_PANEL;
>> +               break;
>> +       default:
>> +               DRM_DEBUG_DRIVER("unsupported intel_encoder type %d\n",
>> +                                intel_encoder->type);
>> +               return -EINVAL;
>> +       }
>> +
>> +       parm |= type << (16 + port * 3);
>> +
>> +       return swsci(dev, SWSCI_SBCB_DISPLAY_POWER_STATE, parm, NULL);
>
> In patch 2, on the initialization code you call the GBDA request
> callbacks function to see which ones will work. Shouldn't you also add
> code to call the SBCB request callbacks function and see if
> DISPLAY_POWER_STATE is really supported? I know that
> DISPLAY_POWER_STATE is supposed to be mandatory, but just checking
> won't hurt us. We could maybe even add a code to WARN in case one of
> the mandatory callbacks is not available :) This suggestion could be
> in a separate patch.

As I explained in my reply to your review on patch 2, my idea was to
filter out unsupported calls in swsci(), so we don't have to add checks
to all call sites. I also log the swsci_requested_callbacks value after
GBDA. We could add a bigger warn (as a separate patch) after the GBDA
call if some needed callback is not requested.

BR,
Jani.


>
> With or without any changes: Reviewed-by: Paulo Zanoni
> <paulo.r.zanoni@intel.com>
>
>
>> +}
>> +
>>  static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>>  {
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>> --
>> 1.7.9.5
>>
>
>
>
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/6] drm/i915: add opregion function to notify bios of encoder enable/disable

[Paulo Zanoni]

2013/8/29 Jani Nikula <jani.nikula@intel.com>:
> On Thu, 29 Aug 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
>> Hi
>>
>> 2013/8/23 Jani Nikula <jani.nikula@intel.com>:
>>> The bios interface seems messy, and it's hard to tell what the bios
>>> really wants. At first, only add support for DDI based machines (hsw+),
>>> and see how it turns out.
>>>
>>> The spec says to notify prior to power down and after power up. It is
>>> unclear whether it makes a difference.
>>>
>>> v2:
>>>  - squash notification function and callers patches together (Daniel)
>>>  - move callers to haswell_crtc_{enable,disable} (Daniel)
>>>  - rename notification function (Chris)
>>>
>>> v3:
>>>  - separate notification function and callers again, as it's not clear
>>>    whether the display power state notification is the right thing to do
>>>    after all
>>>
>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_drv.h       |    8 +++++
>>>  drivers/gpu/drm/i915/intel_opregion.c |   52 +++++++++++++++++++++++++++++++++
>>>  2 files changed, 60 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index adc2f46..1703029 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2171,15 +2171,23 @@ static inline bool intel_gmbus_is_forced_bit(struct i2c_adapter *adapter)
>>>  extern void intel_i2c_reset(struct drm_device *dev);
>>>
>>>  /* intel_opregion.c */
>>> +struct intel_encoder;
>>>  extern int intel_opregion_setup(struct drm_device *dev);
>>>  #ifdef CONFIG_ACPI
>>>  extern void intel_opregion_init(struct drm_device *dev);
>>>  extern void intel_opregion_fini(struct drm_device *dev);
>>>  extern void intel_opregion_asle_intr(struct drm_device *dev);
>>> +extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>>> +                                        bool enable);
>>>  #else
>>>  static inline void intel_opregion_init(struct drm_device *dev) { return; }
>>>  static inline void intel_opregion_fini(struct drm_device *dev) { return; }
>>>  static inline void intel_opregion_asle_intr(struct drm_device *dev) { return; }
>>> +static inline int
>>> +intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, bool enable)
>>> +{
>>> +       return 0;
>>> +}
>>>  #endif
>>>
>>>  /* intel_acpi.c */
>>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>>> index a3558ae..72a4af6 100644
>>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>>> @@ -273,6 +273,58 @@ static int swsci(struct drm_device *dev, u32 function, u32 parm, u32 *parm_out)
>>>  #undef C
>>>  }
>>>
>>> +#define DISPLAY_TYPE_CRT                       0
>>> +#define DISPLAY_TYPE_TV                                1
>>> +#define DISPLAY_TYPE_EXTERNAL_FLAT_PANEL       2
>>> +#define DISPLAY_TYPE_INTERNAL_FLAT_PANEL       3
>>> +
>>> +int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>>> +                                 bool enable)
>>> +{
>>> +       struct drm_device *dev = intel_encoder->base.dev;
>>> +       u32 parm = 0;
>>> +       u32 type = 0;
>>> +       u32 port;
>>> +
>>> +       /* don't care about old stuff for now */
>>> +       if (!HAS_DDI(dev))
>>> +               return 0;
>>> +
>>> +       port = intel_ddi_get_encoder_port(intel_encoder);
>>> +       if (port == PORT_E) {
>>> +               port = 0;
>>> +       } else {
>>> +               parm |= 1 << port;
>>> +               port++;
>>> +       }
>>> +
>>> +       if (!enable)
>>> +               parm |= 4 << 8;
>>> +
>>> +       switch (intel_encoder->type) {
>>> +       case INTEL_OUTPUT_ANALOG:
>>> +               type = DISPLAY_TYPE_CRT;
>>> +               break;
>>> +       case INTEL_OUTPUT_UNKNOWN:
>>> +       case INTEL_OUTPUT_DISPLAYPORT:
>>> +       case INTEL_OUTPUT_HDMI:
>>> +               type = DISPLAY_TYPE_EXTERNAL_FLAT_PANEL;
>>> +               break;
>>> +       case INTEL_OUTPUT_LVDS:
>>
>> We don't have LVDS on DDI platforms.
>
> Right.
>
>>> +       case INTEL_OUTPUT_EDP:
>>> +               type = DISPLAY_TYPE_INTERNAL_FLAT_PANEL;
>>> +               break;
>>> +       default:
>>> +               DRM_DEBUG_DRIVER("unsupported intel_encoder type %d\n",
>>> +                                intel_encoder->type);
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       parm |= type << (16 + port * 3);
>>> +
>>> +       return swsci(dev, SWSCI_SBCB_DISPLAY_POWER_STATE, parm, NULL);
>>
>> In patch 2, on the initialization code you call the GBDA request
>> callbacks function to see which ones will work. Shouldn't you also add
>> code to call the SBCB request callbacks function and see if
>> DISPLAY_POWER_STATE is really supported? I know that
>> DISPLAY_POWER_STATE is supposed to be mandatory, but just checking
>> won't hurt us. We could maybe even add a code to WARN in case one of
>> the mandatory callbacks is not available :) This suggestion could be
>> in a separate patch.
>
> As I explained in my reply to your review on patch 2, my idea was to
> filter out unsupported calls in swsci(), so we don't have to add checks
> to all call sites. I also log the swsci_requested_callbacks value after
> GBDA. We could add a bigger warn (as a separate patch) after the GBDA
> call if some needed callback is not requested.

I re-checked the code and now I see why we're both confused :). We
have 2 request_callbacks functions: the GBDA one and the SBCB one. If
you look at patch 2, you'll notice that at intel_opregion_setup we
request the GBDA callbacks. But if you look at the swsci function
you'll see that our requested_callbacks check is for the SBCB main
function. For some reason I was thinking we only did the "check and
store requested callbacks" for GBDA, so I suggested you to implement
the same thing (inside the swsci funcion), but for SBCB: with this, we
wouldn't need check at the call sites. But I see that in the current
code we call the "give me the GBDA callbacks" but never check them,
and we don't call the "give me the SBCB callbacks" but always check
them.

Also, on patch 2 when I was saying that we need the "-1" on the shift
and that some bits moved around, I was reviewing the SBCB code, not
the GBDA, we need to check both...

>
> BR,
> Jani.
>
>
>>
>> With or without any changes: Reviewed-by: Paulo Zanoni
>> <paulo.r.zanoni@intel.com>
>>
>>
>>> +}
>>> +
>>>  static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>>>  {
>>>         struct drm_i915_private *dev_priv = dev->dev_private;
>>> --
>>> 1.7.9.5
>>>
>>
>>
>>
>> --
>> Paulo Zanoni
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 3/6] drm/i915: add opregion function to notify bios of encoder enable/disable

[Paulo Zanoni]

2013/8/29 Paulo Zanoni <przanoni@gmail.com>:
> Hi
>
> 2013/8/23 Jani Nikula <jani.nikula@intel.com>:
>> The bios interface seems messy, and it's hard to tell what the bios
>> really wants. At first, only add support for DDI based machines (hsw+),
>> and see how it turns out.
>>
>> The spec says to notify prior to power down and after power up. It is
>> unclear whether it makes a difference.
>>
>> v2:
>>  - squash notification function and callers patches together (Daniel)
>>  - move callers to haswell_crtc_{enable,disable} (Daniel)
>>  - rename notification function (Chris)
>>
>> v3:
>>  - separate notification function and callers again, as it's not clear
>>    whether the display power state notification is the right thing to do
>>    after all
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h       |    8 +++++
>>  drivers/gpu/drm/i915/intel_opregion.c |   52 +++++++++++++++++++++++++++++++++
>>  2 files changed, 60 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index adc2f46..1703029 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2171,15 +2171,23 @@ static inline bool intel_gmbus_is_forced_bit(struct i2c_adapter *adapter)
>>  extern void intel_i2c_reset(struct drm_device *dev);
>>
>>  /* intel_opregion.c */
>> +struct intel_encoder;
>>  extern int intel_opregion_setup(struct drm_device *dev);
>>  #ifdef CONFIG_ACPI
>>  extern void intel_opregion_init(struct drm_device *dev);
>>  extern void intel_opregion_fini(struct drm_device *dev);
>>  extern void intel_opregion_asle_intr(struct drm_device *dev);
>> +extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>> +                                        bool enable);
>>  #else
>>  static inline void intel_opregion_init(struct drm_device *dev) { return; }
>>  static inline void intel_opregion_fini(struct drm_device *dev) { return; }
>>  static inline void intel_opregion_asle_intr(struct drm_device *dev) { return; }
>> +static inline int
>> +intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, bool enable)
>> +{
>> +       return 0;
>> +}
>>  #endif
>>
>>  /* intel_acpi.c */
>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> index a3558ae..72a4af6 100644
>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> @@ -273,6 +273,58 @@ static int swsci(struct drm_device *dev, u32 function, u32 parm, u32 *parm_out)
>>  #undef C
>>  }
>>
>> +#define DISPLAY_TYPE_CRT                       0
>> +#define DISPLAY_TYPE_TV                                1
>> +#define DISPLAY_TYPE_EXTERNAL_FLAT_PANEL       2
>> +#define DISPLAY_TYPE_INTERNAL_FLAT_PANEL       3
>> +
>> +int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>> +                                 bool enable)
>> +{
>> +       struct drm_device *dev = intel_encoder->base.dev;
>> +       u32 parm = 0;
>> +       u32 type = 0;
>> +       u32 port;
>> +
>> +       /* don't care about old stuff for now */
>> +       if (!HAS_DDI(dev))
>> +               return 0;
>> +
>> +       port = intel_ddi_get_encoder_port(intel_encoder);
>> +       if (port == PORT_E) {
>> +               port = 0;
>> +       } else {
>> +               parm |= 1 << port;
>> +               port++;
>> +       }
>> +
>> +       if (!enable)
>> +               parm |= 4 << 8;
>> +
>> +       switch (intel_encoder->type) {
>> +       case INTEL_OUTPUT_ANALOG:
>> +               type = DISPLAY_TYPE_CRT;
>> +               break;
>> +       case INTEL_OUTPUT_UNKNOWN:
>> +       case INTEL_OUTPUT_DISPLAYPORT:
>> +       case INTEL_OUTPUT_HDMI:
>> +               type = DISPLAY_TYPE_EXTERNAL_FLAT_PANEL;
>> +               break;
>> +       case INTEL_OUTPUT_LVDS:
>
> We don't have LVDS on DDI platforms.
>
>
>> +       case INTEL_OUTPUT_EDP:
>> +               type = DISPLAY_TYPE_INTERNAL_FLAT_PANEL;
>> +               break;
>> +       default:
>> +               DRM_DEBUG_DRIVER("unsupported intel_encoder type %d\n",
>> +                                intel_encoder->type);

This should be a WARN.


>> +               return -EINVAL;
>> +       }
>> +
>> +       parm |= type << (16 + port * 3);
>> +
>> +       return swsci(dev, SWSCI_SBCB_DISPLAY_POWER_STATE, parm, NULL);
>
> In patch 2, on the initialization code you call the GBDA request
> callbacks function to see which ones will work. Shouldn't you also add
> code to call the SBCB request callbacks function and see if
> DISPLAY_POWER_STATE is really supported? I know that
> DISPLAY_POWER_STATE is supposed to be mandatory, but just checking
> won't hurt us. We could maybe even add a code to WARN in case one of
> the mandatory callbacks is not available :) This suggestion could be
> in a separate patch.
>
> With or without any changes: Reviewed-by: Paulo Zanoni
> <paulo.r.zanoni@intel.com>
>
>
>> +}
>> +
>>  static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>>  {
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>> --
>> 1.7.9.5
>>
>
>
>
> --
> Paulo Zanoni



-- 
Paulo Zanoni

[PATCH 4/6] drm/i915: add opregion function to notify bios of adapter power state

[Jani Nikula]

Notifying the bios lets it enter power saving states.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       |    3 +++
 drivers/gpu/drm/i915/intel_opregion.c |   27 +++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1703029..e17a9a0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2179,12 +2179,15 @@ extern void intel_opregion_fini(struct drm_device *dev);
 extern void intel_opregion_asle_intr(struct drm_device *dev);
 extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
 					 bool enable);
+extern int intel_opregion_notify_adapter(struct drm_device *dev,
+					 pci_power_t state);
 #else
 static inline void intel_opregion_init(struct drm_device *dev) { return; }
 static inline void intel_opregion_fini(struct drm_device *dev) { return; }
 static inline void intel_opregion_asle_intr(struct drm_device *dev) { return; }
 static inline int
 intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, bool enable)
+intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state)
 {
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 72a4af6..e47aefc 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -325,6 +325,33 @@ int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
 	return swsci(dev, SWSCI_SBCB_DISPLAY_POWER_STATE, parm, NULL);
 }
 
+static const struct {
+	pci_power_t pci_power_state;
+	u32 parm;
+} power_state_map[] = {
+	{ PCI_D0,	0x00 },
+	{ PCI_D1,	0x01 },
+	{ PCI_D2,	0x02 },
+	{ PCI_D3hot,	0x04 },
+	{ PCI_D3cold,	0x04 },
+};
+
+int intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state)
+{
+	int i;
+
+	if (!HAS_DDI(dev))
+		return 0;
+
+	for (i = 0; i < ARRAY_SIZE(power_state_map); i++) {
+		if (state == power_state_map[i].pci_power_state)
+			return swsci(dev, SWSCI_SBCB_ADAPTER_POWER_STATE,
+				     power_state_map[i].parm, NULL);
+	}
+
+	return -EINVAL;
+}
+
 static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-- 
1.7.9.5

Re: [PATCH 4/6] drm/i915: add opregion function to notify bios of adapter power state

[Paulo Zanoni]

2013/8/23 Jani Nikula <jani.nikula@intel.com>:
> Notifying the bios lets it enter power saving states.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |    3 +++
>  drivers/gpu/drm/i915/intel_opregion.c |   27 +++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1703029..e17a9a0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2179,12 +2179,15 @@ extern void intel_opregion_fini(struct drm_device *dev);
>  extern void intel_opregion_asle_intr(struct drm_device *dev);
>  extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>                                          bool enable);
> +extern int intel_opregion_notify_adapter(struct drm_device *dev,
> +                                        pci_power_t state);
>  #else
>  static inline void intel_opregion_init(struct drm_device *dev) { return; }
>  static inline void intel_opregion_fini(struct drm_device *dev) { return; }
>  static inline void intel_opregion_asle_intr(struct drm_device *dev) { return; }
>  static inline int
>  intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, bool enable)
> +intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state)

I don't think this will compile when you don't have CONFIG_ACPI. The
"static inline int" part is missing, and the new function stole the
implementation of intel_opregion_notify_encoder.

Besides this, the patch looks correct. We could try to merge patches
1-4 before the others.

>  {
>         return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 72a4af6..e47aefc 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -325,6 +325,33 @@ int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>         return swsci(dev, SWSCI_SBCB_DISPLAY_POWER_STATE, parm, NULL);
>  }
>
> +static const struct {
> +       pci_power_t pci_power_state;
> +       u32 parm;
> +} power_state_map[] = {
> +       { PCI_D0,       0x00 },
> +       { PCI_D1,       0x01 },
> +       { PCI_D2,       0x02 },
> +       { PCI_D3hot,    0x04 },
> +       { PCI_D3cold,   0x04 },
> +};
> +
> +int intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state)
> +{
> +       int i;
> +
> +       if (!HAS_DDI(dev))
> +               return 0;
> +
> +       for (i = 0; i < ARRAY_SIZE(power_state_map); i++) {
> +               if (state == power_state_map[i].pci_power_state)
> +                       return swsci(dev, SWSCI_SBCB_ADAPTER_POWER_STATE,
> +                                    power_state_map[i].parm, NULL);
> +       }
> +
> +       return -EINVAL;
> +}
> +
>  static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> --
> 1.7.9.5
>



-- 
Paulo Zanoni

Re: [PATCH 4/6] drm/i915: add opregion function to notify bios of adapter power state

[Jani Nikula]

On Thu, 29 Aug 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2013/8/23 Jani Nikula <jani.nikula@intel.com>:
>> Notifying the bios lets it enter power saving states.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h       |    3 +++
>>  drivers/gpu/drm/i915/intel_opregion.c |   27 +++++++++++++++++++++++++++
>>  2 files changed, 30 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 1703029..e17a9a0 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2179,12 +2179,15 @@ extern void intel_opregion_fini(struct drm_device *dev);
>>  extern void intel_opregion_asle_intr(struct drm_device *dev);
>>  extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>>                                          bool enable);
>> +extern int intel_opregion_notify_adapter(struct drm_device *dev,
>> +                                        pci_power_t state);
>>  #else
>>  static inline void intel_opregion_init(struct drm_device *dev) { return; }
>>  static inline void intel_opregion_fini(struct drm_device *dev) { return; }
>>  static inline void intel_opregion_asle_intr(struct drm_device *dev) { return; }
>>  static inline int
>>  intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, bool enable)
>> +intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state)
>
> I don't think this will compile when you don't have CONFIG_ACPI. The
> "static inline int" part is missing, and the new function stole the
> implementation of intel_opregion_notify_encoder.

*facepalm* that's ugly.

Thanks for the review,
Jani.


>
> Besides this, the patch looks correct. We could try to merge patches
> 1-4 before the others.
>
>>  {
>>         return 0;
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> index 72a4af6..e47aefc 100644
>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> @@ -325,6 +325,33 @@ int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>>         return swsci(dev, SWSCI_SBCB_DISPLAY_POWER_STATE, parm, NULL);
>>  }
>>
>> +static const struct {
>> +       pci_power_t pci_power_state;
>> +       u32 parm;
>> +} power_state_map[] = {
>> +       { PCI_D0,       0x00 },
>> +       { PCI_D1,       0x01 },
>> +       { PCI_D2,       0x02 },
>> +       { PCI_D3hot,    0x04 },
>> +       { PCI_D3cold,   0x04 },
>> +};
>> +
>> +int intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state)
>> +{
>> +       int i;
>> +
>> +       if (!HAS_DDI(dev))
>> +               return 0;
>> +
>> +       for (i = 0; i < ARRAY_SIZE(power_state_map); i++) {
>> +               if (state == power_state_map[i].pci_power_state)
>> +                       return swsci(dev, SWSCI_SBCB_ADAPTER_POWER_STATE,
>> +                                    power_state_map[i].parm, NULL);
>> +       }
>> +
>> +       return -EINVAL;
>> +}
>> +
>>  static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>>  {
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>> --
>> 1.7.9.5
>>
>
>
>
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/6] DRAFT: drm/i915: do display power state notification on crtc enable/disable

[Jani Nikula]

The spec says to notify prior to power down and after power up. It is
unclear whether it makes a difference.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bcb62fe..a6df68e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3404,8 +3404,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	intel_update_fbc(dev);
 	mutex_unlock(&dev->struct_mutex);
 
-	for_each_encoder_on_crtc(dev, crtc, encoder)
+	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		encoder->enable(encoder);
+		intel_opregion_notify_encoder(encoder, true);
+	}
 
 	/*
 	 * There seems to be a race in PCH platform hw (at least on some
@@ -3519,8 +3521,10 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	if (!intel_crtc->active)
 		return;
 
-	for_each_encoder_on_crtc(dev, crtc, encoder)
+	for_each_encoder_on_crtc(dev, crtc, encoder) {
+		intel_opregion_notify_encoder(encoder, false);
 		encoder->disable(encoder);
+	}
 
 	intel_crtc_wait_for_pending_flips(crtc);
 	drm_vblank_off(dev, pipe);
-- 
1.7.9.5

Re: [PATCH 5/6] DRAFT: drm/i915: do display power state notification on crtc enable/disable

[Paulo Zanoni]

2013/8/23 Jani Nikula <jani.nikula@intel.com>:
> The spec says to notify prior to power down and after power up. It is
> unclear whether it makes a difference.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bcb62fe..a6df68e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3404,8 +3404,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>         intel_update_fbc(dev);
>         mutex_unlock(&dev->struct_mutex);
>
> -       for_each_encoder_on_crtc(dev, crtc, encoder)
> +       for_each_encoder_on_crtc(dev, crtc, encoder) {
>                 encoder->enable(encoder);
> +               intel_opregion_notify_encoder(encoder, true);

No error checking. But if you followed my bikesheds on the previous
patches, I believe all the error cases have already been singaled by
error messages, so this could be fine.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> (and you can
remove the DRAFT word form the patch title)


> +       }
>
>         /*
>          * There seems to be a race in PCH platform hw (at least on some
> @@ -3519,8 +3521,10 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>         if (!intel_crtc->active)
>                 return;
>
> -       for_each_encoder_on_crtc(dev, crtc, encoder)
> +       for_each_encoder_on_crtc(dev, crtc, encoder) {
> +               intel_opregion_notify_encoder(encoder, false);
>                 encoder->disable(encoder);
> +       }
>
>         intel_crtc_wait_for_pending_flips(crtc);
>         drm_vblank_off(dev, pipe);
> --
> 1.7.9.5
>



-- 
Paulo Zanoni

[PATCH 6/6] DRAFT: drm/i915: do adapter power state notification on PC8+ enable/disable

[Jani Nikula]

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a6df68e..7ed2248 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6093,6 +6093,8 @@ void hsw_enable_pc8_work(struct work_struct *__work)
 	lpt_disable_clkout_dp(dev);
 	hsw_pc8_disable_interrupts(dev);
 	hsw_disable_lcpll(dev_priv, true, true);
+
+	intel_opregion_notify_adapter(dev, PCI_D1);
 }
 
 static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
@@ -6126,6 +6128,8 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
 	if (!dev_priv->pc8.enabled)
 		return;
 
+	intel_opregion_notify_adapter(dev, PCI_D0);
+
 	DRM_DEBUG_KMS("Disabling package C8+\n");
 
 	hsw_restore_lcpll(dev_priv);
-- 
1.7.9.5

Re: [PATCH 6/6] DRAFT: drm/i915: do adapter power state notification on PC8+ enable/disable

[Paulo Zanoni]

2013/8/23 Jani Nikula <jani.nikula@intel.com>:

/* Please insert explanation on why we need this and what changes if
we do this. */

I applied your patches and booted them. I got into PC8, did the PC8
test suite and nothing changed. I really don't know what to expect
from this series and/or how to check what's improving. Also, see
below:


> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a6df68e..7ed2248 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6093,6 +6093,8 @@ void hsw_enable_pc8_work(struct work_struct *__work)
>         lpt_disable_clkout_dp(dev);
>         hsw_pc8_disable_interrupts(dev);
>         hsw_disable_lcpll(dev_priv, true, true);
> +
> +       intel_opregion_notify_adapter(dev, PCI_D1);

Why D1? Shouldn't this be D3? I think that's what people having been
asking us to implement.

On the doc that explains "adapter power state notification", my
understanding is that it suggests that we should call this _before_ we
go into the lower states and the other chunk should be called _after_
we're at the higher power states. So perhaps we should call
intel_opregion_notify_adapter before hsw_disable_lcpll, and, on the
chunk below, after hsw_restore_lcpll? But this is not 100% clear, I
may be wrong.

By the way, I modified your patch to implement the suggestions above,
and got the same results: no noticeable difference, everything still
works. No news is good news?


>  }
>
>  static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
> @@ -6126,6 +6128,8 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
>         if (!dev_priv->pc8.enabled)
>                 return;
>
> +       intel_opregion_notify_adapter(dev, PCI_D0);
> +
>         DRM_DEBUG_KMS("Disabling package C8+\n");
>
>         hsw_restore_lcpll(dev_priv);
> --
> 1.7.9.5
>



-- 
Paulo Zanoni

Re: [PATCH 6/6] DRAFT: drm/i915: do adapter power state notification on PC8+ enable/disable

[Kristen Carlson Accardi]

On Fri, 23 Aug 2013 13:44:17 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> 2013/8/23 Jani Nikula <jani.nikula@intel.com>:
> 
> /* Please insert explanation on why we need this and what changes if
> we do this. */
> 
> I applied your patches and booted them. I got into PC8, did the PC8
> test suite and nothing changed. I really don't know what to expect
> from this series and/or how to check what's improving. Also, see
> below:
> 

So this is one of these things that will have no visible impact on
i915, but will impact other parts of the system.  So I think the only
way to test it is by throwing it on the SIP board and checking the
power level of the components this impacts (Audio, thermal, KBC/EC,
LPT).  And without the code which does the actual PCI D3 request from
i915, nothing will happen.  Is it possible to get a patch which finds
some very obvious place to put the controller into D3 so we can check
to see if the opregion notifies are doing what they are supposed to?

> 
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index a6df68e..7ed2248 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6093,6 +6093,8 @@ void hsw_enable_pc8_work(struct work_struct *__work)
> >         lpt_disable_clkout_dp(dev);
> >         hsw_pc8_disable_interrupts(dev);
> >         hsw_disable_lcpll(dev_priv, true, true);
> > +
> > +       intel_opregion_notify_adapter(dev, PCI_D1);
> 
> Why D1? Shouldn't this be D3? I think that's what people having been
> asking us to implement.
> 
> On the doc that explains "adapter power state notification", my
> understanding is that it suggests that we should call this _before_ we
> go into the lower states and the other chunk should be called _after_
> we're at the higher power states. So perhaps we should call
> intel_opregion_notify_adapter before hsw_disable_lcpll, and, on the
> chunk below, after hsw_restore_lcpll? But this is not 100% clear, I
> may be wrong.
> 
> By the way, I modified your patch to implement the suggestions above,
> and got the same results: no noticeable difference, everything still
> works. No news is good news?
> 
> 
> >  }
> >
> >  static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
> > @@ -6126,6 +6128,8 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
> >         if (!dev_priv->pc8.enabled)
> >                 return;
> >
> > +       intel_opregion_notify_adapter(dev, PCI_D0);
> > +
> >         DRM_DEBUG_KMS("Disabling package C8+\n");
> >
> >         hsw_restore_lcpll(dev_priv);
> > --
> > 1.7.9.5
> >
> 
> 
> 

Re: [PATCH 6/6] DRAFT: drm/i915: do adapter power state notification on PC8+ enable/disable

[Paulo Zanoni]

2013/8/23 Kristen Carlson Accardi <kristen.c.accardi@intel.com>:
> On Fri, 23 Aug 2013 13:44:17 -0300
> Paulo Zanoni <przanoni@gmail.com> wrote:
>
>> 2013/8/23 Jani Nikula <jani.nikula@intel.com>:
>>
>> /* Please insert explanation on why we need this and what changes if
>> we do this. */
>>
>> I applied your patches and booted them. I got into PC8, did the PC8
>> test suite and nothing changed. I really don't know what to expect
>> from this series and/or how to check what's improving. Also, see
>> below:
>>
>
> So this is one of these things that will have no visible impact on
> i915, but will impact other parts of the system.  So I think the only
> way to test it is by throwing it on the SIP board and checking the
> power level of the components this impacts (Audio, thermal, KBC/EC,
> LPT).  And without the code which does the actual PCI D3 request from
> i915, nothing will happen.

I was told we could try to verify this by reading PCI config register
0xd4, but for me it's always 0x0, which means we're probably not
really getting into D3.

> Is it possible to get a patch which finds
> some very obvious place to put the controller into D3 so we can check
> to see if the opregion notifies are doing what they are supposed to?

http://cgit.freedesktop.org/~pzanoni/linux/?h=d3-wip

My check using the PCI config register 0xd4 suggests we're probably
not really enabling D3 :(

>
>>
>> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c |    4 ++++
>> >  1 file changed, 4 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index a6df68e..7ed2248 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -6093,6 +6093,8 @@ void hsw_enable_pc8_work(struct work_struct *__work)
>> >         lpt_disable_clkout_dp(dev);
>> >         hsw_pc8_disable_interrupts(dev);
>> >         hsw_disable_lcpll(dev_priv, true, true);
>> > +
>> > +       intel_opregion_notify_adapter(dev, PCI_D1);
>>
>> Why D1? Shouldn't this be D3? I think that's what people having been
>> asking us to implement.
>>
>> On the doc that explains "adapter power state notification", my
>> understanding is that it suggests that we should call this _before_ we
>> go into the lower states and the other chunk should be called _after_
>> we're at the higher power states. So perhaps we should call
>> intel_opregion_notify_adapter before hsw_disable_lcpll, and, on the
>> chunk below, after hsw_restore_lcpll? But this is not 100% clear, I
>> may be wrong.
>>
>> By the way, I modified your patch to implement the suggestions above,
>> and got the same results: no noticeable difference, everything still
>> works. No news is good news?
>>
>>
>> >  }
>> >
>> >  static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
>> > @@ -6126,6 +6128,8 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
>> >         if (!dev_priv->pc8.enabled)
>> >                 return;
>> >
>> > +       intel_opregion_notify_adapter(dev, PCI_D0);
>> > +
>> >         DRM_DEBUG_KMS("Disabling package C8+\n");
>> >
>> >         hsw_restore_lcpll(dev_priv);
>> > --
>> > 1.7.9.5
>> >
>>
>>
>>
>



-- 
Paulo Zanoni

Re: [PATCH 6/6] DRAFT: drm/i915: do adapter power state notification on PC8+ enable/disable

[Ville Syrjälä]

On Fri, Aug 23, 2013 at 04:41:57PM -0300, Paulo Zanoni wrote:
> 2013/8/23 Kristen Carlson Accardi <kristen.c.accardi@intel.com>:
> > On Fri, 23 Aug 2013 13:44:17 -0300
> > Paulo Zanoni <przanoni@gmail.com> wrote:
> >
> >> 2013/8/23 Jani Nikula <jani.nikula@intel.com>:
> >>
> >> /* Please insert explanation on why we need this and what changes if
> >> we do this. */
> >>
> >> I applied your patches and booted them. I got into PC8, did the PC8
> >> test suite and nothing changed. I really don't know what to expect
> >> from this series and/or how to check what's improving. Also, see
> >> below:
> >>
> >
> > So this is one of these things that will have no visible impact on
> > i915, but will impact other parts of the system.  So I think the only
> > way to test it is by throwing it on the SIP board and checking the
> > power level of the components this impacts (Audio, thermal, KBC/EC,
> > LPT).  And without the code which does the actual PCI D3 request from
> > i915, nothing will happen.
> 
> I was told we could try to verify this by reading PCI config register
> 0xd4, but for me it's always 0x0, which means we're probably not
> really getting into D3.

You have to write 0x3 into the PCI PM register to make it enter D3.
Exactly like you do when you suspend the whole machine.

Not sure 0xd4 is the correct offset in this case, but you can look
it up from lspci output (remember +4), or in kernel code just use
pci_dev.pm_cap+4. Hmm, it's 0xd4 in my SNB at least. Maybe it's always
the same for the GPU.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 6/6] DRAFT: drm/i915: do adapter power state notification on PC8+ enable/disable

[Paulo Zanoni]

2013/8/23 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Fri, Aug 23, 2013 at 04:41:57PM -0300, Paulo Zanoni wrote:
>> 2013/8/23 Kristen Carlson Accardi <kristen.c.accardi@intel.com>:
>> > On Fri, 23 Aug 2013 13:44:17 -0300
>> > Paulo Zanoni <przanoni@gmail.com> wrote:
>> >
>> >> 2013/8/23 Jani Nikula <jani.nikula@intel.com>:
>> >>
>> >> /* Please insert explanation on why we need this and what changes if
>> >> we do this. */
>> >>
>> >> I applied your patches and booted them. I got into PC8, did the PC8
>> >> test suite and nothing changed. I really don't know what to expect
>> >> from this series and/or how to check what's improving. Also, see
>> >> below:
>> >>
>> >
>> > So this is one of these things that will have no visible impact on
>> > i915, but will impact other parts of the system.  So I think the only
>> > way to test it is by throwing it on the SIP board and checking the
>> > power level of the components this impacts (Audio, thermal, KBC/EC,
>> > LPT).  And without the code which does the actual PCI D3 request from
>> > i915, nothing will happen.
>>
>> I was told we could try to verify this by reading PCI config register
>> 0xd4, but for me it's always 0x0, which means we're probably not
>> really getting into D3.
>
> You have to write 0x3 into the PCI PM register to make it enter D3.
> Exactly like you do when you suspend the whole machine.
>
> Not sure 0xd4 is the correct offset in this case, but you can look
> it up from lspci output (remember +4), or in kernel code just use
> pci_dev.pm_cap+4. Hmm, it's 0xd4 in my SNB at least. Maybe it's always
> the same for the GPU.

Check the description of 0xd4 on BSpec.

I actually wrote the "get into D3" value to it (using setpci), and
then when I got out of PC8 I saw tons and tons of error messages on
dmesg due to the fact that we were failing to write registers. Which
means it probably works, but we may have more work to do.

>
> --
> Ville Syrjälä
> Intel OTC



-- 
Paulo Zanoni

Re: [PATCH 6/6] DRAFT: drm/i915: do adapter power state notification on PC8+ enable/disable

[Ville Syrjälä]

On Fri, Aug 23, 2013 at 05:14:00PM -0300, Paulo Zanoni wrote:
> 2013/8/23 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > On Fri, Aug 23, 2013 at 04:41:57PM -0300, Paulo Zanoni wrote:
> >> 2013/8/23 Kristen Carlson Accardi <kristen.c.accardi@intel.com>:
> >> > On Fri, 23 Aug 2013 13:44:17 -0300
> >> > Paulo Zanoni <przanoni@gmail.com> wrote:
> >> >
> >> >> 2013/8/23 Jani Nikula <jani.nikula@intel.com>:
> >> >>
> >> >> /* Please insert explanation on why we need this and what changes if
> >> >> we do this. */
> >> >>
> >> >> I applied your patches and booted them. I got into PC8, did the PC8
> >> >> test suite and nothing changed. I really don't know what to expect
> >> >> from this series and/or how to check what's improving. Also, see
> >> >> below:
> >> >>
> >> >
> >> > So this is one of these things that will have no visible impact on
> >> > i915, but will impact other parts of the system.  So I think the only
> >> > way to test it is by throwing it on the SIP board and checking the
> >> > power level of the components this impacts (Audio, thermal, KBC/EC,
> >> > LPT).  And without the code which does the actual PCI D3 request from
> >> > i915, nothing will happen.
> >>
> >> I was told we could try to verify this by reading PCI config register
> >> 0xd4, but for me it's always 0x0, which means we're probably not
> >> really getting into D3.
> >
> > You have to write 0x3 into the PCI PM register to make it enter D3.
> > Exactly like you do when you suspend the whole machine.
> >
> > Not sure 0xd4 is the correct offset in this case, but you can look
> > it up from lspci output (remember +4), or in kernel code just use
> > pci_dev.pm_cap+4. Hmm, it's 0xd4 in my SNB at least. Maybe it's always
> > the same for the GPU.
> 
> Check the description of 0xd4 on BSpec.
> 
> I actually wrote the "get into D3" value to it (using setpci), and
> then when I got out of PC8 I saw tons and tons of error messages on
> dmesg due to the fact that we were failing to write registers. Which
> means it probably works, but we may have more work to do.

My quick and dirty idea for runtime PM would be something like this:
- for ioctls just slap rpm_get_sync/put() around drm_ioctl,
  not optimal but it's very easy for getting started
- all sysfs/debugfs stuff would need to be handled individually
- to deal w/ gtt mmaps just call put_fence or something during suspend,
  grab one ref in fault and probably release it from a delayed work,
  or mabe record a timestamp at last fault time and check it in the idle
  callback
- grab one ref per request (or per active ring maybe?)
- grab one ref per active pipe
- maybe some odd delayed work would need another reference

With that, I think everything of importance would hold a reference,
so the runtime pm idle callback shouldn't really need to do much.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 6/6] DRAFT: drm/i915: do adapter power state notification on PC8+ enable/disable

[Daniel Vetter]

On Mon, Aug 26, 2013 at 9:43 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> My quick and dirty idea for runtime PM would be something like this:
> - for ioctls just slap rpm_get_sync/put() around drm_ioctl,
>   not optimal but it's very easy for getting started
> - all sysfs/debugfs stuff would need to be handled individually
> - to deal w/ gtt mmaps just call put_fence or something during suspend,
>   grab one ref in fault and probably release it from a delayed work,
>   or mabe record a timestamp at last fault time and check it in the idle
>   callback
> - grab one ref per request (or per active ring maybe?)
> - grab one ref per active pipe
> - maybe some odd delayed work would need another reference
>
> With that, I think everything of importance would hold a reference,
> so the runtime pm idle callback shouldn't really need to do much.

I think we can do this cheaper:
- Grab a ref everywhere we want to disallow pc8+ currently (by hooking
into the same get/put calls Paulo's patches sprinkled all over). That
should cover 100% of all the runtime stuff - Paulo's pc8+ test is
rather extensive.

That leaves us with the debugfs/sysfs crap doing register access.
Cheap and dirty (and likely to make Chris rage) is to add get/put
calls with a suitable timeout to the register i/o functions. Less
cheap (but I'm unsure whether that's better) would be to sprinkle
get/put calls around all relevant sysfs/debugfs files. Pure s/w stuff
like the error state wouldn't need it though (and we should try hard
not to require it in case runtime pm hangs).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 0/6] drm/i915: BIOS display/adapter power state notifications

[Jani Nikula]

Hi all, a new version of [1] addressing Paulo's review comments. I hope
I didn't rush the changes too much; the new info about requested
vs. supported callbacks is confusing to say the least...

I'm sure Daniel will appreciate any Tested-by's!


BR,
Jani.


[1] http://mid.gmane.org/cover.1377246881.git.jani.nikula@intel.com

Jani Nikula (6):
  drm/i915: expose intel_ddi_get_encoder_port()
  drm/i915: add plumbing for SWSCI
  drm/i915: add opregion function to notify bios of encoder
    enable/disable
  drm/i915: add opregion function to notify bios of adapter power state
  drm/i915: do display power state notification on crtc enable/disable
  DRAFT: drm/i915: do adapter power state notification on PC8+
    enable/disable

 drivers/gpu/drm/i915/i915_drv.h       |   17 ++
 drivers/gpu/drm/i915/intel_ddi.c      |    2 +-
 drivers/gpu/drm/i915/intel_display.c  |   10 +-
 drivers/gpu/drm/i915/intel_drv.h      |    1 +
 drivers/gpu/drm/i915/intel_opregion.c |  276 ++++++++++++++++++++++++++++++++-
 5 files changed, 300 insertions(+), 6 deletions(-)

-- 
1.7.10.4

[PATCH 6/6] DRAFT: drm/i915: do adapter power state notification on PC8+ enable/disable

[Jani Nikula]

v2:
 - go to PCI_D3cold
 - shuffle the call site a bit

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 83d853f..e33fa6d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6126,6 +6126,7 @@ void hsw_enable_pc8_work(struct work_struct *__work)
 
 	lpt_disable_clkout_dp(dev);
 	hsw_pc8_disable_interrupts(dev);
+	intel_opregion_notify_adapter(dev, PCI_D3cold);
 	hsw_disable_lcpll(dev_priv, true, true);
 }
 
@@ -6163,6 +6164,7 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
 	DRM_DEBUG_KMS("Disabling package C8+\n");
 
 	hsw_restore_lcpll(dev_priv);
+	intel_opregion_notify_adapter(dev, PCI_D0);
 	hsw_pc8_restore_interrupts(dev);
 	lpt_init_pch_refclk(dev);
 
-- 
1.7.10.4

Re: [PATCH 6/6] DRAFT: drm/i915: do adapter power state notification on PC8+ enable/disable

[Paulo Zanoni]

2013/8/30 Jani Nikula <jani.nikula@intel.com>:
> v2:
>  - go to PCI_D3cold
>  - shuffle the call site a bit

Ok, so I know I'm the one who requested to shuffle the call site, but
it's because I thought that when we disable LCPLL we actually put the
device in D3. After some experimentation last week, we discovered we
need to write a PCI config register to actually enable D3, so your
call to intel_opregion_notify_adapter should probably be glued to that
write (which we don't have yet). The problem is that if we really put
the device in D3, all the registers go away, so we need to properly
recover from that. I think it makes sense to postpone this patch until
we have the actual code to put our device in D3 and properly restore
from it. So my suggestion is to merge patches 1-5 so we can work on
the D3 feature on top of the work you already completed.

Thanks,
Paulo

>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 83d853f..e33fa6d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6126,6 +6126,7 @@ void hsw_enable_pc8_work(struct work_struct *__work)
>
>         lpt_disable_clkout_dp(dev);
>         hsw_pc8_disable_interrupts(dev);
> +       intel_opregion_notify_adapter(dev, PCI_D3cold);
>         hsw_disable_lcpll(dev_priv, true, true);
>  }
>
> @@ -6163,6 +6164,7 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
>         DRM_DEBUG_KMS("Disabling package C8+\n");
>
>         hsw_restore_lcpll(dev_priv);
> +       intel_opregion_notify_adapter(dev, PCI_D0);
>         hsw_pc8_restore_interrupts(dev);
>         lpt_init_pch_refclk(dev);
>
> --
> 1.7.10.4
>



-- 
Paulo Zanoni