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