2013/8/29 Paulo Zanoni <przanoni@xxxxxxxxx>:
> Hi
>
> 2013/8/28 Jani Nikula <jani.nikula@xxxxxxxxx>:
>> 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@xxxxxxxxx>
>> ---
>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
