On Thu, 29 Aug 2013, Paulo Zanoni <przanoni@xxxxxxxxx> wrote:
> 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 ?
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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
