[Intel-gfx] [PATCH] drm/i915: add plumbing for SWSCI
Paulo Zanoni
przanoni at gmail.com
Thu Aug 29 15:50:04 CEST 2013
Hi
2013/8/28 Jani Nikula <jani.nikula at 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 at 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
More information about the Intel-gfx
mailing list