2013/8/29 Paulo Zanoni <przan...@gmail.com>:
> Hi
>
> 2013/8/28 Jani Nikula <jani.nik...@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.nik...@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?

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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to