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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux