[Intel-gfx] [MIPI SEQ PARSING v2 PATCH 03/11] drm/i915: Parsing VBT if size of VBT exceeds 6KB
Deepak, M
m.deepak at intel.com
Mon Sep 21 23:37:12 PDT 2015
>-----Original Message-----
>From: Jani Nikula [mailto:jani.nikula at linux.intel.com]
>Sent: Thursday, September 17, 2015 5:41 PM
>To: Deepak, M; intel-gfx at lists.freedesktop.org
>Cc: Deepak, M
>Subject: Re: [Intel-gfx] [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 at intel.com> 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 at intel.com>
>> ---
>> 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.
>> }
>>
>> 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.
>
>>
>> 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.
>>
>> #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 at intel.com>
>> + * Copyright 2008 Red Hat <mjg at redhat.com>
>> + *
>> + * 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 at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>--
>Jani Nikula, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list