On Tue, 22 Sep 2015, "Deepak, M" <m.deepak@xxxxxxxxx> wrote: >>-----Original Message----- >>From: Jani Nikula [mailto:jani.nikula@xxxxxxxxxxxxxxx] >>Sent: Thursday, September 17, 2015 5:41 PM >>To: Deepak, M; intel-gfx@xxxxxxxxxxxxxxxxxxxxx >>Cc: Deepak, M >>Subject: Re: [MIPI SEQ PARSING v2 PATCH 03/11] drm/i915: Parsing >>VBT if size of VBT exceeds 6KB >> >>On Thu, 10 Sep 2015, Deepak M <m.deepak@xxxxxxxxx> wrote: >>> Currently the iomap for VBT works only if the size of the VBT is less >>> than 6KB, but if the size of the VBT exceeds 6KB than the physical >>> address and the size of the VBT to be iomapped is specified in the >>> mailbox3 and is iomapped accordingly. >>> >>> v2: - Moving the validate_vbt to opregion file (Jani) >>> - Fix the i915_opregion() in debugfs (Jani) >>> >>> Signed-off-by: Deepak M <m.deepak@xxxxxxxxx> >>> --- >>> drivers/gpu/drm/i915/i915_debugfs.c | 24 ++- >>> drivers/gpu/drm/i915/i915_drv.h | 4 + >>> drivers/gpu/drm/i915/intel_bios.c | 49 +----- >>> drivers/gpu/drm/i915/intel_opregion.c | 279 >>> +++++++++------------------------ >>> drivers/gpu/drm/i915/intel_opregion.h | 230 >>> +++++++++++++++++++++++++++ >>> 5 files changed, 329 insertions(+), 257 deletions(-) create mode >>> 100644 drivers/gpu/drm/i915/intel_opregion.h >>> >>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c >>> b/drivers/gpu/drm/i915/i915_debugfs.c >>> index 41629fa..5534aa2 100644 >>> --- a/drivers/gpu/drm/i915/i915_debugfs.c >>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >>> @@ -26,6 +26,8 @@ >>> * >>> */ >>> >>> +#include <linux/acpi.h> >>> +#include <acpi/video.h> >>> #include <linux/seq_file.h> >>> #include <linux/circ_buf.h> >>> #include <linux/ctype.h> >>> @@ -39,6 +41,7 @@ >>> #include "intel_ringbuffer.h" >>> #include <drm/i915_drm.h> >>> #include "i915_drv.h" >>> +#include "intel_opregion.h" >>> >>> enum { >>> ACTIVE_LIST, >>> @@ -1832,7 +1835,7 @@ static int i915_opregion(struct seq_file *m, void >>*unused) >>> struct drm_device *dev = node->minor->dev; >>> struct drm_i915_private *dev_priv = dev->dev_private; >>> struct intel_opregion *opregion = &dev_priv->opregion; >>> - void *data = kmalloc(OPREGION_SIZE, GFP_KERNEL); >>> + void *data = kmalloc(OPREGION_VBT_OFFSET, GFP_KERNEL); >>> int ret; >>> >>> if (data == NULL) >>> @@ -1843,12 +1846,25 @@ static int i915_opregion(struct seq_file *m, void >>*unused) >>> goto out; >>> >>> if (opregion->header) { >>> - memcpy_fromio(data, opregion->header, OPREGION_SIZE); >>> - seq_write(m, data, OPREGION_SIZE); >>> + memcpy_fromio(data, opregion->header, >>OPREGION_VBT_OFFSET); >>> + seq_write(m, data, OPREGION_VBT_OFFSET); >>> + kfree(data); >>> + if (opregion->asle->rvda) { >>> + data = kmalloc(opregion->asle->rvds, GFP_KERNEL); >>> + memcpy_fromio(data, >>> + (const void __iomem *) opregion->asle->rvda, >>> + opregion->asle->rvds); >>> + seq_write(m, data, opregion->asle->rvds); >>> + } else { >>> + data = kmalloc(OPREGION_SIZE - >>OPREGION_VBT_OFFSET, >>> + GFP_KERNEL); >>> + memcpy_fromio(data, opregion->vbt, >>> + OPREGION_SIZE - >>OPREGION_VBT_OFFSET); >>> + seq_write(m, data, OPREGION_SIZE - >>OPREGION_VBT_OFFSET); >>> + } >> >>If rvda != 0, this debugfs file no longer represents the opregion contents. >>Mailboxes #4 and #5 are dropped from the output. BTW, what is mailbox #4 >>expected to contain when rvda != 0? (I still don't have access to the latest >>opregion spec version, so can't check what it actually says.) >> >>I am beginning to think we should leave "i915_opregion" debugfs file intact, >>and add a new "i915_vbt" file that contains either mailbox #4 or the data in >>rvda. This might be a cleaner approach. >> >>See my comments below, and you'll see how this would be feasible. >> > [Deepak M] I was thinking of splitting this function into 5 for dumping each mailbox. Which > I felt will be cleaner. Please have i915_opregion and i915_vbt as I explained. >>> } >>> >>> mutex_unlock(&dev->struct_mutex); >>> - >>> out: >>> kfree(data); >>> return 0; >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h >>> b/drivers/gpu/drm/i915/i915_drv.h index 1287007..507d57a 100644 >>> --- a/drivers/gpu/drm/i915/i915_drv.h >>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>> @@ -1780,6 +1780,7 @@ struct drm_i915_private { >>> struct i915_hotplug hotplug; >>> struct i915_fbc fbc; >>> struct i915_drrs drrs; >>> + const struct bdb_header *bdb_start; >> >>What is wrong with using dev_priv->opregion.vbt for this? >> >>> struct intel_opregion opregion; >>> struct intel_vbt_data vbt; >>> >>> @@ -3306,6 +3307,9 @@ intel_opregion_notify_adapter(struct drm_device >>> *dev, pci_power_t state) } #endif >>> >>> +const struct bdb_header *validate_vbt(const void __iomem *_vbt, >>> + size_t size, const char *source); >>> + >>> /* intel_acpi.c */ >>> #ifdef CONFIG_ACPI >>> extern void intel_register_dsm_handler(void); diff --git >>> a/drivers/gpu/drm/i915/intel_bios.c >>> b/drivers/gpu/drm/i915/intel_bios.c >>> index 0bf0942..1932a86 100644 >>> --- a/drivers/gpu/drm/i915/intel_bios.c >>> +++ b/drivers/gpu/drm/i915/intel_bios.c >>> @@ -1227,49 +1227,6 @@ static const struct dmi_system_id >>intel_no_opregion_vbt[] = { >>> { } >>> }; >>> >>> -static const struct bdb_header *validate_vbt(const void __iomem *_base, >>> - size_t size, >>> - const void __iomem *_vbt, >>> - const char *source) >>> -{ >>> - /* >>> - * This is the one place where we explicitly discard the address space >>> - * (__iomem) of the BIOS/VBT. (And this will cause a sparse >>complaint.) >>> - * From now on everything is based on 'base', and treated as regular >>> - * memory. >>> - */ >>> - const void *base = (const void *) _base; >>> - size_t offset = _vbt - _base; >>> - const struct vbt_header *vbt = base + offset; >>> - const struct bdb_header *bdb; >>> - >>> - if (offset + sizeof(struct vbt_header) > size) { >>> - DRM_DEBUG_DRIVER("VBT header incomplete\n"); >>> - return NULL; >>> - } >>> - >>> - if (memcmp(vbt->signature, "$VBT", 4)) { >>> - DRM_DEBUG_DRIVER("VBT invalid signature\n"); >>> - return NULL; >>> - } >>> - >>> - offset += vbt->bdb_offset; >>> - if (offset + sizeof(struct bdb_header) > size) { >>> - DRM_DEBUG_DRIVER("BDB header incomplete\n"); >>> - return NULL; >>> - } >>> - >>> - bdb = base + offset; >>> - if (offset + bdb->bdb_size > size) { >>> - DRM_DEBUG_DRIVER("BDB incomplete\n"); >>> - return NULL; >>> - } >>> - >>> - DRM_DEBUG_KMS("Using VBT from %s: %20s\n", >>> - source, vbt->signature); >>> - return bdb; >>> -} >>> - >> >>Moving of this function should be a separate prep patch. >> >>> static const struct bdb_header *find_vbt(void __iomem *bios, size_t >>> size) { >>> const struct bdb_header *bdb = NULL; @@ -1278,7 +1235,7 @@ static >>> const struct bdb_header *find_vbt(void __iomem *bios, size_t size) >>> /* Scour memory looking for the VBT signature. */ >>> for (i = 0; i + 4 < size; i++) { >>> if (ioread32(bios + i) == *((const u32 *) "$VBT")) { >>> - bdb = validate_vbt(bios, size, bios + i, "PCI ROM"); >>> + bdb = validate_vbt(bios + i, size - i, "PCI ROM"); >>> break; >>> } >>> } >>> @@ -1308,10 +1265,8 @@ intel_parse_bios(struct drm_device *dev) >>> >>> init_vbt_defaults(dev_priv); >>> >>> - /* XXX Should this validation be moved to intel_opregion.c? */ >>> if (!dmi_check_system(intel_no_opregion_vbt) && dev_priv- >>>opregion.vbt) >>> - bdb = validate_vbt(dev_priv->opregion.header, >>OPREGION_SIZE, >>> - dev_priv->opregion.vbt, "OpRegion"); >>> + bdb = dev_priv->bdb_start; >> >>This should be dev_priv->opregion.vbt. > [Deepak M] validate_vbt function always returns the bdb header and our parsing starts from the bdb_header and not form the dev_priv->opregion.vbt where the VBT header starts. So I have added bdb_start in the dev_priv structure. >> Please don't add new fields when you can reuse existing ones. >>> >>> if (bdb == NULL) { >>> size_t size; >>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c >>> b/drivers/gpu/drm/i915/intel_opregion.c >>> index f46231f..3a43db9 100644 >>> --- a/drivers/gpu/drm/i915/intel_opregion.c >>> +++ b/drivers/gpu/drm/i915/intel_opregion.c >>> @@ -32,210 +32,7 @@ >>> #include <drm/i915_drm.h> >>> #include "i915_drv.h" >>> #include "intel_drv.h" >>> - >>> -#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 >>> -#define ACPI_CLID 0x01ac /* current lid state indicator */ >>> -#define ACPI_CDCK 0x01b0 /* current docking state indicator */ >>> -#define OPREGION_SWSCI_OFFSET 0x200 >>> -#define OPREGION_ASLE_OFFSET 0x300 >>> -#define OPREGION_VBT_OFFSET 0x400 >>> - >>> -#define OPREGION_SIGNATURE "IntelGraphicsMem" >>> -#define MBOX_ACPI (1<<0) >>> -#define MBOX_SWSCI (1<<1) >>> -#define MBOX_ASLE (1<<2) >>> -#define MBOX_ASLE_EXT (1<<4) >>> - >>> -struct opregion_header { >>> - u8 signature[16]; >>> - u32 size; >>> - u32 opregion_ver; >>> - u8 bios_ver[32]; >>> - u8 vbios_ver[16]; >>> - u8 driver_ver[16]; >>> - u32 mboxes; >>> - u32 driver_model; >>> - u32 pcon; >>> - u8 dver[32]; >>> - u8 rsvd[124]; >>> -} __packed; >>> - >>> -/* OpRegion mailbox #1: public ACPI methods */ -struct opregion_acpi >>> { >>> - u32 drdy; /* driver readiness */ >>> - u32 csts; /* notification status */ >>> - u32 cevt; /* current event */ >>> - u8 rsvd1[20]; >>> - u32 didl[8]; /* supported display devices ID list */ >>> - u32 cpdl[8]; /* currently presented display list */ >>> - u32 cadl[8]; /* currently active display list */ >>> - u32 nadl[8]; /* next active devices list */ >>> - u32 aslp; /* ASL sleep time-out */ >>> - u32 tidx; /* toggle table index */ >>> - u32 chpd; /* current hotplug enable indicator */ >>> - u32 clid; /* current lid state*/ >>> - u32 cdck; /* current docking state */ >>> - u32 sxsw; /* Sx state resume */ >>> - u32 evts; /* ASL supported events */ >>> - u32 cnot; /* current OS notification */ >>> - u32 nrdy; /* driver status */ >>> - u32 did2[7]; /* extended supported display devices ID list */ >>> - u32 cpd2[7]; /* extended attached display devices list */ >>> - u8 rsvd2[4]; >>> -} __packed; >>> - >>> -/* OpRegion mailbox #2: SWSCI */ >>> -struct opregion_swsci { >>> - u32 scic; /* SWSCI command|status|data */ >>> - u32 parm; /* command parameters */ >>> - u32 dslp; /* driver sleep time-out */ >>> - u8 rsvd[244]; >>> -} __packed; >>> - >>> -/* OpRegion mailbox #3: ASLE */ >>> -struct opregion_asle { >>> - u32 ardy; /* driver readiness */ >>> - u32 aslc; /* ASLE interrupt command */ >>> - u32 tche; /* technology enabled indicator */ >>> - u32 alsi; /* current ALS illuminance reading */ >>> - u32 bclp; /* backlight brightness to set */ >>> - u32 pfit; /* panel fitting state */ >>> - u32 cblv; /* current brightness level */ >>> - u16 bclm[20]; /* backlight level duty cycle mapping table */ >>> - u32 cpfm; /* current panel fitting mode */ >>> - u32 epfm; /* enabled panel fitting modes */ >>> - u8 plut[74]; /* panel LUT and identifier */ >>> - u32 pfmb; /* PWM freq and min brightness */ >>> - u32 cddv; /* color correction default values */ >>> - u32 pcft; /* power conservation features */ >>> - u32 srot; /* supported rotation angles */ >>> - u32 iuer; /* IUER events */ >>> - u64 fdss; >>> - u32 fdsp; >>> - u32 stat; >>> - u64 rvda; /* Physical address of raw vbt data */ >>> - u32 rvds; /* Size of raw vbt data */ >>> - u8 rsvd[58]; >>> -} __packed; >>> - >>> -/* Driver readiness indicator */ >>> -#define ASLE_ARDY_READY (1 << 0) >>> -#define ASLE_ARDY_NOT_READY (0 << 0) >>> - >>> -/* ASLE Interrupt Command (ASLC) bits */ >>> -#define ASLC_SET_ALS_ILLUM (1 << 0) >>> -#define ASLC_SET_BACKLIGHT (1 << 1) >>> -#define ASLC_SET_PFIT (1 << 2) >>> -#define ASLC_SET_PWM_FREQ (1 << 3) >>> -#define ASLC_SUPPORTED_ROTATION_ANGLES (1 << 4) >>> -#define ASLC_BUTTON_ARRAY (1 << 5) >>> -#define ASLC_CONVERTIBLE_INDICATOR (1 << 6) >>> -#define ASLC_DOCKING_INDICATOR (1 << 7) >>> -#define ASLC_ISCT_STATE_CHANGE (1 << 8) >>> -#define ASLC_REQ_MSK 0x1ff >>> -/* response bits */ >>> -#define ASLC_ALS_ILLUM_FAILED (1 << 10) >>> -#define ASLC_BACKLIGHT_FAILED (1 << 12) >>> -#define ASLC_PFIT_FAILED (1 << 14) >>> -#define ASLC_PWM_FREQ_FAILED (1 << 16) >>> -#define ASLC_ROTATION_ANGLES_FAILED (1 << 18) >>> -#define ASLC_BUTTON_ARRAY_FAILED (1 << 20) >>> -#define ASLC_CONVERTIBLE_FAILED (1 << 22) >>> -#define ASLC_DOCKING_FAILED (1 << 24) >>> -#define ASLC_ISCT_STATE_FAILED (1 << 26) >>> - >>> -/* Technology enabled indicator */ >>> -#define ASLE_TCHE_ALS_EN (1 << 0) >>> -#define ASLE_TCHE_BLC_EN (1 << 1) >>> -#define ASLE_TCHE_PFIT_EN (1 << 2) >>> -#define ASLE_TCHE_PFMB_EN (1 << 3) >>> - >>> -/* ASLE backlight brightness to set */ >>> -#define ASLE_BCLP_VALID (1<<31) >>> -#define ASLE_BCLP_MSK (~(1<<31)) >>> - >>> -/* ASLE panel fitting request */ >>> -#define ASLE_PFIT_VALID (1<<31) >>> -#define ASLE_PFIT_CENTER (1<<0) >>> -#define ASLE_PFIT_STRETCH_TEXT (1<<1) -#define >>ASLE_PFIT_STRETCH_GFX >>> (1<<2) >>> - >>> -/* PWM frequency and minimum brightness */ -#define >>> ASLE_PFMB_BRIGHTNESS_MASK (0xff) -#define >>ASLE_PFMB_BRIGHTNESS_VALID >>> (1<<8) -#define ASLE_PFMB_PWM_MASK (0x7ffffe00) -#define >>> ASLE_PFMB_PWM_VALID (1<<31) >>> - >>> -#define ASLE_CBLV_VALID (1<<31) >>> - >>> -/* IUER */ >>> -#define ASLE_IUER_DOCKING (1 << 7) >>> -#define ASLE_IUER_CONVERTIBLE (1 << 6) >>> -#define ASLE_IUER_ROTATION_LOCK_BTN (1 << 4) >>> -#define ASLE_IUER_VOLUME_DOWN_BTN (1 << 3) >>> -#define ASLE_IUER_VOLUME_UP_BTN (1 << 2) >>> -#define ASLE_IUER_WINDOWS_BTN (1 << 1) >>> -#define ASLE_IUER_POWER_BTN (1 << 0) >>> - >>> -/* 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 (0xff << 8) >>> -#define SWSCI_SCIC_EXIT_PARAMETER_SHIFT 8 >>> -#define SWSCI_SCIC_EXIT_PARAMETER_MASK (0xff << 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) >>> -#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_PANEL_DETAILS >> SWSCI_FUNCTION_CODE(SWSCI_SBCB, 10) >>> -#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) >>> -#define ACPI_DIGITAL_OUTPUT (3<<8) >>> -#define ACPI_LVDS_OUTPUT (4<<8) >>> - >>> -#define MAX_DSLP 1500 >>> +#include "intel_opregion.h" >> >>As said, I don't see the need to move these defines to a separate header. This >>is definitely stuff that we want to keep hidden in one place, and nobody >>outside of intel_opregion.c should use these. >> > [Deepak M] Need some of these definitions in the debugfs.c file, because of this > had created a new file for macros. Please rework your debugfs handling to not need these. If needed, add an interface to intel_opregion.c to do what you want. > >>> >>> #ifdef CONFIG_ACPI >>> static int swsci(struct drm_device *dev, u32 function, u32 parm, u32 >>> *parm_out) @@ -892,13 +689,55 @@ static void swsci_setup(struct >>> drm_device *dev) static inline void swsci_setup(struct drm_device >>> *dev) {} #endif /* CONFIG_ACPI */ >>> >>> +const struct bdb_header *validate_vbt(const void __iomem *_vbt, >>> + size_t size, >>> + const char *source) >>> +{ >>> + /* >>> + * This is the one place where we explicitly discard the address space >>> + * (__iomem) of the BIOS/VBT. (And this will cause a sparse >>complaint.) >>> + */ >>> + const struct vbt_header *vbt = (const struct vbt_header *)_vbt; >>> + const struct bdb_header *bdb; >>> + size_t offset; >>> + >>> + if (sizeof(struct vbt_header) > size) { >>> + DRM_DEBUG_DRIVER("VBT header incomplete\n"); >>> + return NULL; >>> + } >>> + >>> + if (memcmp(vbt->signature, "$VBT", 4)) { >>> + DRM_DEBUG_DRIVER("VBT invalid signature\n"); >>> + return NULL; >>> + } >>> + >>> + offset = vbt->bdb_offset; >>> + if (offset + sizeof(struct bdb_header) > size) { >>> + DRM_DEBUG_DRIVER("BDB header incomplete\n"); >>> + return NULL; >>> + } >>> + >>> + bdb = (const void *)_vbt + offset; >>> + if (offset + bdb->bdb_size > size) { >>> + DRM_DEBUG_DRIVER("BDB incomplete\n"); >>> + return NULL; >>> + } >>> + >>> + DRM_DEBUG_KMS("Using VBT from %s: %20s\n", >>> + source, vbt->signature); >>> + return bdb; >>> +} >>> + >>> int intel_opregion_setup(struct drm_device *dev) { >>> struct drm_i915_private *dev_priv = dev->dev_private; >>> struct intel_opregion *opregion = &dev_priv->opregion; >>> void __iomem *base; >>> + void __iomem *vbt_base; >>> u32 asls, mboxes; >>> char buf[sizeof(OPREGION_SIGNATURE)]; >>> + const struct bdb_header *bdb = NULL; >>> + size_t size; >>> int err = 0; >>> >>> BUILD_BUG_ON(sizeof(struct opregion_header) != 0x100); @@ - >>917,7 >>> +756,7 @@ int intel_opregion_setup(struct drm_device *dev) >>> INIT_WORK(&opregion->asle_work, asle_work); #endif >>> >>> - base = acpi_os_ioremap(asls, OPREGION_SIZE); >>> + base = acpi_os_ioremap(asls, OPREGION_VBT_OFFSET); >> >>Now you leave out mailbox #5. I don't think there's any reason *not* to >>ioremap all of opregion here. >> >>> if (!base) >>> return -ENOMEM; >>> >>> @@ -929,7 +768,33 @@ int intel_opregion_setup(struct drm_device *dev) >>> goto err_out; >>> } >>> opregion->header = base; >>> - opregion->vbt = base + OPREGION_VBT_OFFSET; >>> + /* Assigning the alse to the mailbox 3 of the opregion */ >>> + opregion->asle = base + OPREGION_ASLE_OFFSET; >> >>That's assigned towards the end of the function *if* the mbox is supported. >> >>> + >>> + /* >>> + * Non-zero value in rvda field is an indication to driver that a >>> + * valid Raw VBT is stored in that address and driver should not refer >>> + * to mailbox4 for getting VBT. >>> + */ >>> + if (opregion->header->opregion_ver >= 2 && opregion->asle->rvda) { >>> + size = opregion->asle->rvds; >>> + vbt_base = acpi_os_ioremap(opregion->asle->rvda, >>> + size); >>> + } else { >>> + size = OPREGION_SIZE - OPREGION_VBT_OFFSET; >>> + vbt_base = acpi_os_ioremap(asls + >>OPREGION_VBT_OFFSET, >>> + size); >>> + } >>> + >>> + bdb = validate_vbt(vbt_base, size, "OpRegion"); >>> + >>> + if (bdb == NULL) { >>> + err = -EINVAL; >>> + goto err_vbt; >>> + } >>> + >>> + dev_priv->bdb_start = bdb; >> >>Again, I don't see why you should store this here. Nor I see the need to >>actually validate vbt here. You can just as well do it in intel_bios like before, as >>long as you assign opregion->vbt here appropriately. >> >>You'll also need to iounmap vbt_base in intel_opregion_fini. That may need >>some additional checks if you unconditionally ioremap all of opregion and >>conditionally ioremap rvda, and point opregion->vbt to it. >> > [Deepak M] Okay will handle this. >>> + opregion->vbt = vbt_base; >>> >>> opregion->lid_state = base + ACPI_CLID; >>> >>> @@ -953,6 +818,8 @@ int intel_opregion_setup(struct drm_device *dev) >>> >>> return 0; >>> >>> +err_vbt: >>> + iounmap(vbt_base); >>> err_out: >>> iounmap(base); >>> return err; >>> diff --git a/drivers/gpu/drm/i915/intel_opregion.h >>> b/drivers/gpu/drm/i915/intel_opregion.h >>> new file mode 100644 >>> index 0000000..bcb45ec >>> --- /dev/null >>> +++ b/drivers/gpu/drm/i915/intel_opregion.h >>> @@ -0,0 +1,230 @@ >>> +/* >>> + * Copyright 2008 Intel Corporation <hong.liu@xxxxxxxxx> >>> + * Copyright 2008 Red Hat <mjg@xxxxxxxxxx> >>> + * >>> + * Permission is hereby granted, free of charge, to any person >>> +obtaining >>> + * a copy of this software and associated documentation files (the >>> + * "Software"), to deal in the Software without restriction, >>> +including >>> + * without limitation the rights to use, copy, modify, merge, >>> +publish, >>> + * distribute, sub license, and/or sell copies of the Software, and >>> +to >>> + * permit persons to whom the Software is furnished to do so, subject >>> +to >>> + * the following conditions: >>> + * >>> + * The above copyright notice and this permission notice (including >>> +the >>> + * next paragraph) shall be included in all copies or substantial >>> + * portions of the Software. >>> + * >>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY >>KIND, >>> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE >>WARRANTIES OF >>> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND >>> + * NON-INFRINGEMENT. IN NO EVENT SHALL INTEL AND/OR ITS SUPPLIERS >>BE >>> + * LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN >>> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF >>OR IN >>> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN >>THE >>> + * SOFTWARE. >>> + * >>> + */ >>> + >>> +#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 >>> +#define ACPI_CLID 0x01ac /* current lid state indicator */ >>> +#define ACPI_CDCK 0x01b0 /* current docking state indicator */ >>> +#define OPREGION_SWSCI_OFFSET 0x200 >>> +#define OPREGION_ASLE_OFFSET 0x300 >>> +#define OPREGION_VBT_OFFSET 0x400 >>> + >>> +#define OPREGION_SIGNATURE "IntelGraphicsMem" >>> +#define MBOX_ACPI (1<<0) >>> +#define MBOX_SWSCI (1<<1) >>> +#define MBOX_ASLE (1<<2) >>> +#define MBOX_ASLE_EXT (1<<4) >>> + >>> +struct opregion_header { >>> + u8 signature[16]; >>> + u32 size; >>> + u32 opregion_ver; >>> + u8 bios_ver[32]; >>> + u8 vbios_ver[16]; >>> + u8 driver_ver[16]; >>> + u32 mboxes; >>> + u32 driver_model; >>> + u32 pcon; >>> + u8 dver[32]; >>> + u8 rsvd[124]; >>> +} __packed; >>> + >>> +/* OpRegion mailbox #1: public ACPI methods */ struct opregion_acpi { >>> + u32 drdy; /* driver readiness */ >>> + u32 csts; /* notification status */ >>> + u32 cevt; /* current event */ >>> + u8 rsvd1[20]; >>> + u32 didl[8]; /* supported display devices ID list */ >>> + u32 cpdl[8]; /* currently presented display list */ >>> + u32 cadl[8]; /* currently active display list */ >>> + u32 nadl[8]; /* next active devices list */ >>> + u32 aslp; /* ASL sleep time-out */ >>> + u32 tidx; /* toggle table index */ >>> + u32 chpd; /* current hotplug enable indicator */ >>> + u32 clid; /* current lid state*/ >>> + u32 cdck; /* current docking state */ >>> + u32 sxsw; /* Sx state resume */ >>> + u32 evts; /* ASL supported events */ >>> + u32 cnot; /* current OS notification */ >>> + u32 nrdy; /* driver status */ >>> + u32 did2[7]; /* extended supported display devices ID list */ >>> + u32 cpd2[7]; /* extended attached display devices list */ >>> + u8 rsvd2[4]; >>> +} __packed; >>> + >>> +/* OpRegion mailbox #2: SWSCI */ >>> +struct opregion_swsci { >>> + u32 scic; /* SWSCI command|status|data */ >>> + u32 parm; /* command parameters */ >>> + u32 dslp; /* driver sleep time-out */ >>> + u8 rsvd[244]; >>> +} __packed; >>> + >>> +/* OpRegion mailbox #3: ASLE */ >>> +struct opregion_asle { >>> + u32 ardy; /* driver readiness */ >>> + u32 aslc; /* ASLE interrupt command */ >>> + u32 tche; /* technology enabled indicator */ >>> + u32 alsi; /* current ALS illuminance reading */ >>> + u32 bclp; /* backlight brightness to set */ >>> + u32 pfit; /* panel fitting state */ >>> + u32 cblv; /* current brightness level */ >>> + u16 bclm[20]; /* backlight level duty cycle mapping table */ >>> + u32 cpfm; /* current panel fitting mode */ >>> + u32 epfm; /* enabled panel fitting modes */ >>> + u8 plut[74]; /* panel LUT and identifier */ >>> + u32 pfmb; /* PWM freq and min brightness */ >>> + u32 cddv; /* color correction default values */ >>> + u32 pcft; /* power conservation features */ >>> + u32 srot; /* supported rotation angles */ >>> + u32 iuer; /* IUER events */ >>> + u64 fdss; >>> + u32 fdsp; >>> + u32 stat; >>> + u64 rvda; /* Physical address of raw vbt data */ >>> + u32 rvds; /* Size of raw vbt data */ >>> + u8 rsvd[58]; >>> +} __packed; >>> + >>> +/* Driver readiness indicator */ >>> +#define ASLE_ARDY_READY (1 << 0) >>> +#define ASLE_ARDY_NOT_READY (0 << 0) >>> + >>> +/* ASLE Interrupt Command (ASLC) bits */ >>> +#define ASLC_SET_ALS_ILLUM (1 << 0) >>> +#define ASLC_SET_BACKLIGHT (1 << 1) >>> +#define ASLC_SET_PFIT (1 << 2) >>> +#define ASLC_SET_PWM_FREQ (1 << 3) >>> +#define ASLC_SUPPORTED_ROTATION_ANGLES (1 << 4) >>> +#define ASLC_BUTTON_ARRAY (1 << 5) >>> +#define ASLC_CONVERTIBLE_INDICATOR (1 << 6) >>> +#define ASLC_DOCKING_INDICATOR (1 << 7) >>> +#define ASLC_ISCT_STATE_CHANGE (1 << 8) >>> +#define ASLC_REQ_MSK 0x1ff >>> +/* response bits */ >>> +#define ASLC_ALS_ILLUM_FAILED (1 << 10) >>> +#define ASLC_BACKLIGHT_FAILED (1 << 12) >>> +#define ASLC_PFIT_FAILED (1 << 14) >>> +#define ASLC_PWM_FREQ_FAILED (1 << 16) >>> +#define ASLC_ROTATION_ANGLES_FAILED (1 << 18) >>> +#define ASLC_BUTTON_ARRAY_FAILED (1 << 20) >>> +#define ASLC_CONVERTIBLE_FAILED (1 << 22) >>> +#define ASLC_DOCKING_FAILED (1 << 24) >>> +#define ASLC_ISCT_STATE_FAILED (1 << 26) >>> + >>> +/* Technology enabled indicator */ >>> +#define ASLE_TCHE_ALS_EN (1 << 0) >>> +#define ASLE_TCHE_BLC_EN (1 << 1) >>> +#define ASLE_TCHE_PFIT_EN (1 << 2) >>> +#define ASLE_TCHE_PFMB_EN (1 << 3) >>> + >>> +/* ASLE backlight brightness to set */ >>> +#define ASLE_BCLP_VALID (1<<31) >>> +#define ASLE_BCLP_MSK (~(1<<31)) >>> + >>> +/* ASLE panel fitting request */ >>> +#define ASLE_PFIT_VALID (1<<31) >>> +#define ASLE_PFIT_CENTER (1<<0) >>> +#define ASLE_PFIT_STRETCH_TEXT (1<<1) #define >>ASLE_PFIT_STRETCH_GFX >>> +(1<<2) >>> + >>> +/* PWM frequency and minimum brightness */ #define >>> +ASLE_PFMB_BRIGHTNESS_MASK (0xff) #define >>ASLE_PFMB_BRIGHTNESS_VALID >>> +(1<<8) #define ASLE_PFMB_PWM_MASK (0x7ffffe00) #define >>> +ASLE_PFMB_PWM_VALID (1<<31) >>> + >>> +#define ASLE_CBLV_VALID (1<<31) >>> + >>> +/* IUER */ >>> +#define ASLE_IUER_DOCKING (1 << 7) >>> +#define ASLE_IUER_CONVERTIBLE (1 << 6) >>> +#define ASLE_IUER_ROTATION_LOCK_BTN (1 << 4) >>> +#define ASLE_IUER_VOLUME_DOWN_BTN (1 << 3) >>> +#define ASLE_IUER_VOLUME_UP_BTN (1 << 2) >>> +#define ASLE_IUER_WINDOWS_BTN (1 << 1) >>> +#define ASLE_IUER_POWER_BTN (1 << 0) >>> + >>> +/* 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 (0xff << 8) >>> +#define SWSCI_SCIC_EXIT_PARAMETER_SHIFT 8 >>> +#define SWSCI_SCIC_EXIT_PARAMETER_MASK (0xff << 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) >>> +#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_PANEL_DETAILS >> SWSCI_FUNCTION_CODE(SWSCI_SBCB, 10) >>> +#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) >>> +#define ACPI_DIGITAL_OUTPUT (3<<8) >>> +#define ACPI_LVDS_OUTPUT (4<<8) >>> + >>> +#define MAX_DSLP 1500 >> >>The bottom line here is that I think you could get all of this done with >>*much* smaller changes. If we get a regression report pointing at this patch, >>we'll have a lot of debugging to do to figure out what failed. >> > [Deepak M] Okay will try to break this patch into smaller one`s. >>BR, >>Jani. >> >> >> >>> -- >>> 1.7.9.5 >>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >>-- >>Jani Nikula, Intel Open Source Technology Center -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
