drm/i915: VBT/opregion refactor to support >6 KB VBT

Make the validation function a boolean operating on a buffer of given size, removing the extra pointer dances.

Move the OpRegion based VBT validation to intel_opregion_setup(), only initializing opregion->vbt if it's valid.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/intel_bios.c     | 63 ++++++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_opregion.c |  9 +++--
 3 files changed, 40 insertions(+), 33 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +1 -0

index a2bd4c6a86a1..ef15f1710b50 100644
@@ -3354,6 +3354,7 @@ extern void intel_i2c_reset(struct drm_device *dev);
 
 /* intel_bios.c */
 int intel_bios_init(struct drm_device *dev);
+bool intel_bios_is_valid_vbt(const void *buf, size_t size);
 
 /* intel_opregion.c */
 #ifdef CONFIG_ACPI

drivers/gpu/drm/i915/intel_bios.c +32 -28

index f2b79b7d12b8..007b2b759600 100644
@@ -1221,37 +1221,40 @@ static const struct bdb_header *get_bdb_header(const struct vbt_header *vbt)
 	return _vbt + vbt->bdb_offset;
 }
 
-static const struct vbt_header *validate_vbt(const void *base,
-					     size_t size,
-					     const void *_vbt)
+/**
+ * intel_bios_is_valid_vbt - does the given buffer contain a valid VBT
+ * @buf:	pointer to a buffer to validate
+ * @size:	size of the buffer
+ *
+ * Returns true on valid VBT.
+ */
+bool intel_bios_is_valid_vbt(const void *buf, size_t size)
 {
-	size_t offset = _vbt - base;
-	const struct vbt_header *vbt = _vbt;
+	const struct vbt_header *vbt = buf;
 	const struct bdb_header *bdb;
 
 	if (!vbt)
-		return NULL;
+		return false;
 
-	if (offset + sizeof(struct vbt_header) > size) {
+	if (sizeof(struct vbt_header) > size) {
 		DRM_DEBUG_DRIVER("VBT header incomplete\n");
-		return NULL;
+		return false;
 	}
 
 	if (memcmp(vbt->signature, "$VBT", 4)) {
 		DRM_DEBUG_DRIVER("VBT invalid signature\n");
-		return NULL;
+		return false;
 	}
 
-	offset += vbt->bdb_offset;
-	if (offset + sizeof(struct bdb_header) > size) {
+	if (vbt->bdb_offset + sizeof(struct bdb_header) > size) {
 		DRM_DEBUG_DRIVER("BDB header incomplete\n");
-		return NULL;
+		return false;
 	}
 
 	bdb = get_bdb_header(vbt);
-	if (offset + bdb->bdb_size > size) {
+	if (vbt->bdb_offset + bdb->bdb_size > size) {
 		DRM_DEBUG_DRIVER("BDB incomplete\n");
-		return NULL;
+		return false;
 	}
 
 	return vbt;

@@ -1259,26 +1262,27 @@ static const struct vbt_header *validate_vbt(const void *base,
 
 static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
 {
-	const struct vbt_header *vbt = NULL;
 	size_t i;
 
 	/* Scour memory looking for the VBT signature. */
 	for (i = 0; i + 4 < size; i++) {
-		if (ioread32(bios + i) == *((const u32 *) "$VBT")) {
-			/*
-			 * This is the one place where we explicitly discard the
-			 * address space (__iomem) of the BIOS/VBT. From now on
-			 * everything is based on 'base', and treated as regular
-			 * memory.
-			 */
-			void *_bios = (void __force *) bios;
+		void *vbt;
 
-			vbt = validate_vbt(_bios, size, _bios + i);
-			break;
-		}
+		if (ioread32(bios + i) != *((const u32 *) "$VBT"))
+			continue;
+
+		/*
+		 * This is the one place where we explicitly discard the address
+		 * space (__iomem) of the BIOS/VBT.
+		 */
+		vbt = (void __force *) bios + i;
+		if (intel_bios_is_valid_vbt(vbt, size - i))
+			return vbt;
+
+		break;
 	}
 
-	return vbt;
+	return NULL;
 }
 
 /**

@@ -1295,7 +1299,7 @@ intel_bios_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct pci_dev *pdev = dev->pdev;
-	const struct vbt_header *vbt;
+	const struct vbt_header *vbt = dev_priv->opregion.vbt;
 	const struct bdb_header *bdb;
 	u8 __iomem *bios = NULL;
 

drivers/gpu/drm/i915/intel_opregion.c +7 -2

index 5b9fc790d300..9f992e7c6185 100644
@@ -986,8 +986,13 @@ int intel_opregion_setup(struct drm_device *dev)
 	if (mboxes & MBOX_ASLE_EXT)
 		DRM_DEBUG_DRIVER("ASLE extension supported\n");
 
-	if (!dmi_check_system(intel_no_opregion_vbt))
-		opregion->vbt = base + OPREGION_VBT_OFFSET;
+	if (!dmi_check_system(intel_no_opregion_vbt)) {
+		void *vbt = base + OPREGION_VBT_OFFSET;
+		u32 vbt_size = OPREGION_SIZE - OPREGION_VBT_OFFSET;
+
+		if (intel_bios_is_valid_vbt(vbt, vbt_size))
+			opregion->vbt = vbt;
+	}
 
 	return 0;
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 138 lines · Jani Nikula

Make the validation function a boolean operating on a buffer of given size, removing the extra pointer dances.

Move the OpRegion based VBT validation to intel_opregion_setup(), only initializing opregion->vbt if it's valid.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/intel_bios.c     | 63 ++++++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_opregion.c |  9 +++--
 3 files changed, 40 insertions(+), 33 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +1 -0

index a2bd4c6a86a1..ef15f1710b50 100644
@@ -3354,6 +3354,7 @@ extern void intel_i2c_reset(struct drm_device *dev);
 
 /* intel_bios.c */
 int intel_bios_init(struct drm_device *dev);
+bool intel_bios_is_valid_vbt(const void *buf, size_t size);
 
 /* intel_opregion.c */
 #ifdef CONFIG_ACPI

drivers/gpu/drm/i915/intel_bios.c +32 -28

index f2b79b7d12b8..007b2b759600 100644
@@ -1221,37 +1221,40 @@ static const struct bdb_header *get_bdb_header(const struct vbt_header *vbt)
 	return _vbt + vbt->bdb_offset;
 }
 
-static const struct vbt_header *validate_vbt(const void *base,
-					     size_t size,
-					     const void *_vbt)
+/**
+ * intel_bios_is_valid_vbt - does the given buffer contain a valid VBT
+ * @buf:	pointer to a buffer to validate
+ * @size:	size of the buffer
+ *
+ * Returns true on valid VBT.
+ */
+bool intel_bios_is_valid_vbt(const void *buf, size_t size)
 {
-	size_t offset = _vbt - base;
-	const struct vbt_header *vbt = _vbt;
+	const struct vbt_header *vbt = buf;
 	const struct bdb_header *bdb;
 
 	if (!vbt)
-		return NULL;
+		return false;
 
-	if (offset + sizeof(struct vbt_header) > size) {
+	if (sizeof(struct vbt_header) > size) {
 		DRM_DEBUG_DRIVER("VBT header incomplete\n");
-		return NULL;
+		return false;
 	}
 
 	if (memcmp(vbt->signature, "$VBT", 4)) {
 		DRM_DEBUG_DRIVER("VBT invalid signature\n");
-		return NULL;
+		return false;
 	}
 
-	offset += vbt->bdb_offset;
-	if (offset + sizeof(struct bdb_header) > size) {
+	if (vbt->bdb_offset + sizeof(struct bdb_header) > size) {
 		DRM_DEBUG_DRIVER("BDB header incomplete\n");
-		return NULL;
+		return false;
 	}
 
 	bdb = get_bdb_header(vbt);
-	if (offset + bdb->bdb_size > size) {
+	if (vbt->bdb_offset + bdb->bdb_size > size) {
 		DRM_DEBUG_DRIVER("BDB incomplete\n");
-		return NULL;
+		return false;
 	}
 
 	return vbt;

@@ -1259,26 +1262,27 @@ static const struct vbt_header *validate_vbt(const void *base,
 
 static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
 {
-	const struct vbt_header *vbt = NULL;
 	size_t i;
 
 	/* Scour memory looking for the VBT signature. */
 	for (i = 0; i + 4 < size; i++) {
-		if (ioread32(bios + i) == *((const u32 *) "$VBT")) {
-			/*
-			 * This is the one place where we explicitly discard the
-			 * address space (__iomem) of the BIOS/VBT. From now on
-			 * everything is based on 'base', and treated as regular
-			 * memory.
-			 */
-			void *_bios = (void __force *) bios;
+		void *vbt;
 
-			vbt = validate_vbt(_bios, size, _bios + i);
-			break;
-		}
+		if (ioread32(bios + i) != *((const u32 *) "$VBT"))
+			continue;
+
+		/*
+		 * This is the one place where we explicitly discard the address
+		 * space (__iomem) of the BIOS/VBT.
+		 */
+		vbt = (void __force *) bios + i;
+		if (intel_bios_is_valid_vbt(vbt, size - i))
+			return vbt;
+
+		break;
 	}
 
-	return vbt;
+	return NULL;
 }
 
 /**

@@ -1295,7 +1299,7 @@ intel_bios_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct pci_dev *pdev = dev->pdev;
-	const struct vbt_header *vbt;
+	const struct vbt_header *vbt = dev_priv->opregion.vbt;
 	const struct bdb_header *bdb;
 	u8 __iomem *bios = NULL;
 

@@ -1304,9 +1308,6 @@ intel_bios_init(struct drm_device *dev)
 
 	init_vbt_defaults(dev_priv);
 

drivers/gpu/drm/i915/intel_bios.c intel_bios_init(struct drm_device *dev) · line 1313

Quoted 6 lines

drivers/gpu/drm/i915/intel_bios.c +0 -3

-	/* XXX Should this validation be moved to intel_opregion.c? */
-	vbt = validate_vbt(dev_priv->opregion.header, OPREGION_SIZE,
-			   dev_priv->opregion.vbt);
 	if (vbt) {
 		DRM_DEBUG_KMS("Using VBT from ACPI OpRegion: %20s\n",
 			      vbt->signature);

Maybe the debug message should be moved too? It looks a bit out of place after the validation gets moved.

Quoted 28 lines

drivers/gpu/drm/i915/intel_opregion.c +7 -2

index 5b9fc790d300..9f992e7c6185 100644
@@ -986,8 +986,13 @@ int intel_opregion_setup(struct drm_device *dev)
 	if (mboxes & MBOX_ASLE_EXT)
 		DRM_DEBUG_DRIVER("ASLE extension supported\n");
 
-	if (!dmi_check_system(intel_no_opregion_vbt))
-		opregion->vbt = base + OPREGION_VBT_OFFSET;
+	if (!dmi_check_system(intel_no_opregion_vbt)) {
+		void *vbt = base + OPREGION_VBT_OFFSET;
+		u32 vbt_size = OPREGION_SIZE - OPREGION_VBT_OFFSET;
+
+		if (intel_bios_is_valid_vbt(vbt, vbt_size))
+			opregion->vbt = vbt;
+	}
 
 	return 0;
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 139 lines · Ville Syrjälä

Quoted 138 lines · Jani Nikula

Make the validation function a boolean operating on a buffer of given size, removing the extra pointer dances.

Move the OpRegion based VBT validation to intel_opregion_setup(), only initializing opregion->vbt if it's valid.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/intel_bios.c     | 63 ++++++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_opregion.c |  9 +++--
 3 files changed, 40 insertions(+), 33 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +1 -0

index a2bd4c6a86a1..ef15f1710b50 100644
@@ -3354,6 +3354,7 @@ extern void intel_i2c_reset(struct drm_device *dev);
 
 /* intel_bios.c */
 int intel_bios_init(struct drm_device *dev);
+bool intel_bios_is_valid_vbt(const void *buf, size_t size);
 
 /* intel_opregion.c */
 #ifdef CONFIG_ACPI

drivers/gpu/drm/i915/intel_bios.c +32 -28

index f2b79b7d12b8..007b2b759600 100644
@@ -1221,37 +1221,40 @@ static const struct bdb_header *get_bdb_header(const struct vbt_header *vbt)
 	return _vbt + vbt->bdb_offset;
 }
 
-static const struct vbt_header *validate_vbt(const void *base,
-					     size_t size,
-					     const void *_vbt)
+/**
+ * intel_bios_is_valid_vbt - does the given buffer contain a valid VBT
+ * @buf:	pointer to a buffer to validate
+ * @size:	size of the buffer
+ *
+ * Returns true on valid VBT.
+ */
+bool intel_bios_is_valid_vbt(const void *buf, size_t size)
 {
-	size_t offset = _vbt - base;
-	const struct vbt_header *vbt = _vbt;
+	const struct vbt_header *vbt = buf;
 	const struct bdb_header *bdb;
 
 	if (!vbt)
-		return NULL;
+		return false;
 
-	if (offset + sizeof(struct vbt_header) > size) {
+	if (sizeof(struct vbt_header) > size) {
 		DRM_DEBUG_DRIVER("VBT header incomplete\n");
-		return NULL;
+		return false;
 	}
 
 	if (memcmp(vbt->signature, "$VBT", 4)) {
 		DRM_DEBUG_DRIVER("VBT invalid signature\n");
-		return NULL;
+		return false;
 	}
 
-	offset += vbt->bdb_offset;
-	if (offset + sizeof(struct bdb_header) > size) {
+	if (vbt->bdb_offset + sizeof(struct bdb_header) > size) {
 		DRM_DEBUG_DRIVER("BDB header incomplete\n");
-		return NULL;
+		return false;
 	}
 
 	bdb = get_bdb_header(vbt);
-	if (offset + bdb->bdb_size > size) {
+	if (vbt->bdb_offset + bdb->bdb_size > size) {
 		DRM_DEBUG_DRIVER("BDB incomplete\n");
-		return NULL;
+		return false;
 	}
 
 	return vbt;

@@ -1259,26 +1262,27 @@ static const struct vbt_header *validate_vbt(const void *base,
 
 static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
 {
-	const struct vbt_header *vbt = NULL;
 	size_t i;
 
 	/* Scour memory looking for the VBT signature. */
 	for (i = 0; i + 4 < size; i++) {
-		if (ioread32(bios + i) == *((const u32 *) "$VBT")) {
-			/*
-			 * This is the one place where we explicitly discard the
-			 * address space (__iomem) of the BIOS/VBT. From now on
-			 * everything is based on 'base', and treated as regular
-			 * memory.
-			 */
-			void *_bios = (void __force *) bios;
+		void *vbt;
 
-			vbt = validate_vbt(_bios, size, _bios + i);
-			break;
-		}
+		if (ioread32(bios + i) != *((const u32 *) "$VBT"))
+			continue;
+
+		/*
+		 * This is the one place where we explicitly discard the address
+		 * space (__iomem) of the BIOS/VBT.
+		 */
+		vbt = (void __force *) bios + i;
+		if (intel_bios_is_valid_vbt(vbt, size - i))
+			return vbt;
+
+		break;
 	}
 
-	return vbt;
+	return NULL;
 }
 
 /**

@@ -1295,7 +1299,7 @@ intel_bios_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct pci_dev *pdev = dev->pdev;
-	const struct vbt_header *vbt;
+	const struct vbt_header *vbt = dev_priv->opregion.vbt;
 	const struct bdb_header *bdb;
 	u8 __iomem *bios = NULL;
 

@@ -1304,9 +1308,6 @@ intel_bios_init(struct drm_device *dev)
 
 	init_vbt_defaults(dev_priv);
 

Quoted 9 lines

drivers/gpu/drm/i915/intel_bios.c intel_bios_init(struct drm_device *dev) · line 1313

Quoted 6 lines

drivers/gpu/drm/i915/intel_bios.c +0 -3

-	/* XXX Should this validation be moved to intel_opregion.c? */
-	vbt = validate_vbt(dev_priv->opregion.header, OPREGION_SIZE,
-			   dev_priv->opregion.vbt);
 	if (vbt) {
 		DRM_DEBUG_KMS("Using VBT from ACPI OpRegion: %20s\n",
 			      vbt->signature);

Maybe the debug message should be moved too? It looks a bit out of place after the validation gets moved.

This is intentional (see patch 4). In a way this is where the decision is made to use the opregion VBT. The opregion code just validates it, but doesn't actually *use* it.

BR,
Jani.

Quoted 28 lines

Quoted 28 lines

drivers/gpu/drm/i915/intel_opregion.c +7 -2

index 5b9fc790d300..9f992e7c6185 100644
@@ -986,8 +986,13 @@ int intel_opregion_setup(struct drm_device *dev)
 	if (mboxes & MBOX_ASLE_EXT)
 		DRM_DEBUG_DRIVER("ASLE extension supported\n");
 
-	if (!dmi_check_system(intel_no_opregion_vbt))
-		opregion->vbt = base + OPREGION_VBT_OFFSET;
+	if (!dmi_check_system(intel_no_opregion_vbt)) {
+		void *vbt = base + OPREGION_VBT_OFFSET;
+		u32 vbt_size = OPREGION_SIZE - OPREGION_VBT_OFFSET;
+
+		if (intel_bios_is_valid_vbt(vbt, vbt_size))
+			opregion->vbt = vbt;
+	}
 
 	return 0;
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center

Hasn't been necessary since

commit 115719fceaa733d646e39cdce83cc32ddb891a49

Author: Williams, Dan J <[email protected]>
Date:   Mon Oct 12 21:12:57 2015 +0000

i915: switch from acpi_os_ioremap to memremap

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_debugfs.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

drivers/gpu/drm/i915/i915_debugfs.c +0 -0

index 24318b79bcfc..a9e1f18c36d1 100644
@@ -1842,25 +1842,18 @@ 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;

drivers/gpu/drm/i915/i915_debugfs.c +2 -5

-	if (opregion->header) {
-		memcpy(data, opregion->header, OPREGION_SIZE);
-		seq_write(m, data, OPREGION_SIZE);
-	}
+	if (opregion->header)
+		seq_write(m, opregion->header, OPREGION_SIZE);
 
 	mutex_unlock(&dev->struct_mutex);
 
 out:
-	kfree(data);
 	return 0;
 }
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 24 lines · Jani Nikula

Hasn't been necessary since

commit 115719fceaa733d646e39cdce83cc32ddb891a49

Author: Williams, Dan J <[email protected]>
Date:   Mon Oct 12 21:12:57 2015 +0000

i915: switch from acpi_os_ioremap to memremap

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_debugfs.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

drivers/gpu/drm/i915/i915_debugfs.c +0 -1

index 24318b79bcfc..a9e1f18c36d1 100644
@@ -1842,25 +1842,18 @@ 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);
 	int ret;
 

drivers/gpu/drm/i915/i915_debugfs.c static int i915_opregion(struct seq_file *m, void *unused) · line 1849

Quoted 6 lines

drivers/gpu/drm/i915/i915_debugfs.c +0 -3

-	if (data == NULL)
-		return -ENOMEM;
-
 	ret = mutex_lock_interruptible(&dev->struct_mutex);
 	if (ret)
 		goto out;

A bit off topic, but I wonder what this locking is supposed to protect?

Quoted 23 lines

drivers/gpu/drm/i915/i915_debugfs.c +2 -5

-	if (opregion->header) {
-		memcpy(data, opregion->header, OPREGION_SIZE);
-		seq_write(m, data, OPREGION_SIZE);
-	}
+	if (opregion->header)
+		seq_write(m, opregion->header, OPREGION_SIZE);
 
 	mutex_unlock(&dev->struct_mutex);
 
 out:
-	kfree(data);
 	return 0;
 }
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 25 lines · Ville Syrjälä

Quoted 24 lines · Jani Nikula

Hasn't been necessary since

commit 115719fceaa733d646e39cdce83cc32ddb891a49

Author: Williams, Dan J <[email protected]>
Date:   Mon Oct 12 21:12:57 2015 +0000

i915: switch from acpi_os_ioremap to memremap

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_debugfs.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

drivers/gpu/drm/i915/i915_debugfs.c +0 -1

index 24318b79bcfc..a9e1f18c36d1 100644
@@ -1842,25 +1842,18 @@ 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);
 	int ret;
 

Quoted 8 lines

drivers/gpu/drm/i915/i915_debugfs.c static int i915_opregion(struct seq_file *m, void *unused) · line 1849

Quoted 6 lines

drivers/gpu/drm/i915/i915_debugfs.c +0 -3

-	if (data == NULL)
-		return -ENOMEM;
-
 	ret = mutex_lock_interruptible(&dev->struct_mutex);
 	if (ret)
 		goto out;

A bit off topic, but I wonder what this locking is supposed to protect?

I thought about it when Chris mentioned locking in the next patch. Some opregion mailboxes do change due to driver <-> bios notifications, even if the VBT doesn't. Not sure if there's any chance this could lead to us dumping an inconsistent opregion, and if so, what difference that could make. So I thought better safe than sorry and just let it be.

BR,
Jani.

Quoted 23 lines

Quoted 23 lines

drivers/gpu/drm/i915/i915_debugfs.c +2 -5

-	if (opregion->header) {
-		memcpy(data, opregion->header, OPREGION_SIZE);
-		seq_write(m, data, OPREGION_SIZE);
-	}
+	if (opregion->header)
+		seq_write(m, opregion->header, OPREGION_SIZE);
 
 	mutex_unlock(&dev->struct_mutex);
 
 out:
-	kfree(data);
 	return 0;
 }
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center

In the future the VBT might not be in mailbox #4 of the ACPI OpRegion, thus unavailable in i915_opregion, so add a separate file for the VBT.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_debugfs.c   | 22 ++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/intel_opregion.c |  4 +++-
 3 files changed, 26 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_debugfs.c +0 -0

index a9e1f18c36d1..aef1393e707f 100644
@@ -1857,6 +1857,27 @@ out:
 	return 0;
 }

Quoted 18 lines · Jani Nikula

In the future the VBT might not be in mailbox #4 of the ACPI OpRegion, thus unavailable in i915_opregion, so add a separate file for the VBT.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_debugfs.c   | 22 ++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/intel_opregion.c |  4 +++-
 3 files changed, 26 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_debugfs.c +0 -0

index a9e1f18c36d1..aef1393e707f 100644
@@ -1857,6 +1857,27 @@ out:
 	return 0;
 }
 

drivers/gpu/drm/i915/i915_debugfs.c out: · line 1868

Quoted 9 lines

drivers/gpu/drm/i915/i915_debugfs.c +9 -0

+static int i915_vbt(struct seq_file *m, void *unused)
+{
+	struct drm_info_node *node = m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_opregion *opregion = &dev_priv->opregion;
+	int ret;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);

The contents of opregion->vbt are not serialised by struct_mutex, right?

Quoted 19 lines · Chris Wilson

Quoted 18 lines · Jani Nikula

In the future the VBT might not be in mailbox #4 of the ACPI OpRegion, thus unavailable in i915_opregion, so add a separate file for the VBT.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_debugfs.c   | 22 ++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/intel_opregion.c |  4 +++-
 3 files changed, 26 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_debugfs.c +0 -0

index a9e1f18c36d1..aef1393e707f 100644
@@ -1857,6 +1857,27 @@ out:
 	return 0;
 }
 

Quoted 11 lines

drivers/gpu/drm/i915/i915_debugfs.c out: · line 1868

Quoted 9 lines

drivers/gpu/drm/i915/i915_debugfs.c +9 -0

+static int i915_vbt(struct seq_file *m, void *unused)
+{
+	struct drm_info_node *node = m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_opregion *opregion = &dev_priv->opregion;
+	int ret;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);

The contents of opregion->vbt are not serialised by struct_mutex, right?

Cargo culting ftw. I figured it was maybe for suspend/resume paths, but that doesn't really make sense does it?

The RVDA and RVDS (raw VBT data address and size) fields of the ASLE mailbox may specify an alternate location for VBT instead of mailbox #4. Use the alternate location if available and valid, falling back to mailbox #4 otherwise.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/intel_opregion.c | 25 +++++++++++++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +1 -0

index ca8c2a64bc6d..8cfac7398568 100644
@@ -457,6 +457,7 @@ struct intel_opregion {
 	u32 swsci_gbda_sub_functions;
 	u32 swsci_sbcb_sub_functions;
 	struct opregion_asle *asle;
+	void *rvda;
 	const void *vbt;
 	u32 vbt_size;
 	u32 *lid_state;

drivers/gpu/drm/i915/intel_opregion.c +4 -0

index e89ee2383fe1..a139889dd45b 100644
@@ -837,6 +837,10 @@ void intel_opregion_fini(struct drm_device *dev)
 
 	/* just clear all opregion memory pointers now */
 	memunmap(opregion->header);
+	if (opregion->rvda) {
+		memunmap(opregion->rvda);
+		opregion->rvda = NULL;
+	}
 	opregion->header = NULL;
 	opregion->acpi = NULL;
 	opregion->swsci = NULL;

drivers/gpu/drm/i915/intel_opregion.c +14 -0

+			opregion->rvda = memremap(opregion->asle->rvda,
+						  opregion->asle->rvds,
+						  MEMREMAP_WB);
+			vbt = opregion->rvda;
+			vbt_size = opregion->asle->rvds;
+		}
 
 		if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
 			opregion->vbt = vbt;
 			opregion->vbt_size = vbt_size;
+			DRM_DEBUG_DRIVER("VBT from RVDA\n");
+		} else {
+			vbt = base + OPREGION_VBT_OFFSET;
+			vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET;
+			if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
+				opregion->vbt = vbt;
+				opregion->vbt_size = vbt_size;
+			}
 		}
 	}
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 41 lines · Jani Nikula

The RVDA and RVDS (raw VBT data address and size) fields of the ASLE mailbox may specify an alternate location for VBT instead of mailbox #4. Use the alternate location if available and valid, falling back to mailbox #4 otherwise.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/intel_opregion.c | 25 +++++++++++++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +1 -0

index ca8c2a64bc6d..8cfac7398568 100644
@@ -457,6 +457,7 @@ struct intel_opregion {
 	u32 swsci_gbda_sub_functions;
 	u32 swsci_sbcb_sub_functions;
 	struct opregion_asle *asle;
+	void *rvda;
 	const void *vbt;
 	u32 vbt_size;
 	u32 *lid_state;

drivers/gpu/drm/i915/intel_opregion.c +4 -0

index e89ee2383fe1..a139889dd45b 100644
@@ -837,6 +837,10 @@ void intel_opregion_fini(struct drm_device *dev)
 
 	/* just clear all opregion memory pointers now */
 	memunmap(opregion->header);
+	if (opregion->rvda) {
+		memunmap(opregion->rvda);
+		opregion->rvda = NULL;
+	}
 	opregion->header = NULL;
 	opregion->acpi = NULL;
 	opregion->swsci = NULL;

@@ -987,12 +991,29 @@ int intel_opregion_setup(struct drm_device *dev)
 	`	DRM_DEBUG_DRIVER("ASLE extension supported\n");
 

drivers/gpu/drm/i915/intel_opregion.c int intel_opregion_setup(struct drm_device *dev) · line 998

Quoted 8 lines

drivers/gpu/drm/i915/intel_opregion.c +5 -2

 	if (!dmi_check_system(intel_no_opregion_vbt)) {
-		const void *vbt = base + OPREGION_VBT_OFFSET;
-		u32 vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET;
+		const void *vbt = NULL;
+		u32 vbt_size = 0;
+
+		if (opregion->header->opregion_ver >= 2 && opregion->asle &&
+		    opregion->asle->rvda && opregion->asle->rvds) {

Either I'm blind or you didn't actually add rvda/rvds to struct opregion_asle.

Also the spec seems confused as usual. Some parts if it refer to this as RVDA others as RVBT. Although RVBT also seems to be what the mbox #4 contents are called in another place, and to add insult to injury that place also has the offset and size all wrong. Sigh.

Anyway, apart from the missing rvda/rvds definititions the rest looks OK.

Quoted 28 lines

drivers/gpu/drm/i915/intel_opregion.c +14 -0

+			opregion->rvda = memremap(opregion->asle->rvda,
+						  opregion->asle->rvds,
+						  MEMREMAP_WB);
+			vbt = opregion->rvda;
+			vbt_size = opregion->asle->rvds;
+		}
 
 		if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
 			opregion->vbt = vbt;
 			opregion->vbt_size = vbt_size;
+			DRM_DEBUG_DRIVER("VBT from RVDA\n");
+		} else {
+			vbt = base + OPREGION_VBT_OFFSET;
+			vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET;
+			if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
+				opregion->vbt = vbt;
+				opregion->vbt_size = vbt_size;
+			}
 		}
 	}
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 42 lines · Ville Syrjälä

Quoted 41 lines · Jani Nikula

The RVDA and RVDS (raw VBT data address and size) fields of the ASLE mailbox may specify an alternate location for VBT instead of mailbox #4. Use the alternate location if available and valid, falling back to mailbox #4 otherwise.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/intel_opregion.c | 25 +++++++++++++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +1 -0

index ca8c2a64bc6d..8cfac7398568 100644
@@ -457,6 +457,7 @@ struct intel_opregion {
 	u32 swsci_gbda_sub_functions;
 	u32 swsci_sbcb_sub_functions;
 	struct opregion_asle *asle;
+	void *rvda;
 	const void *vbt;
 	u32 vbt_size;
 	u32 *lid_state;

drivers/gpu/drm/i915/intel_opregion.c +4 -0

index e89ee2383fe1..a139889dd45b 100644
@@ -837,6 +837,10 @@ void intel_opregion_fini(struct drm_device *dev)
 
 	/* just clear all opregion memory pointers now */
 	memunmap(opregion->header);
+	if (opregion->rvda) {
+		memunmap(opregion->rvda);
+		opregion->rvda = NULL;
+	}
 	opregion->header = NULL;
 	opregion->acpi = NULL;
 	opregion->swsci = NULL;

@@ -987,12 +991,29 @@ int intel_opregion_setup(struct drm_device *dev)
 	`	DRM_DEBUG_DRIVER("ASLE extension supported\n");
 

Quoted 10 lines

drivers/gpu/drm/i915/intel_opregion.c int intel_opregion_setup(struct drm_device *dev) · line 998

Quoted 8 lines

drivers/gpu/drm/i915/intel_opregion.c +5 -2

 	if (!dmi_check_system(intel_no_opregion_vbt)) {
-		const void *vbt = base + OPREGION_VBT_OFFSET;
-		u32 vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET;
+		const void *vbt = NULL;
+		u32 vbt_size = 0;
+
+		if (opregion->header->opregion_ver >= 2 && opregion->asle &&
+		    opregion->asle->rvda && opregion->asle->rvds) {

Either I'm blind or you didn't actually add rvda/rvds to struct opregion_asle.

Oh I se they went in seprately as
c85f6c91ec42 ("drm/i915: add VBT address and size fields to ASLE mailbox struct")

OK, so apart from the few minor bikesheds I listed it all looks good to me. For the series

Reviewed-by Ville Syrjälä <[email protected]>

Quoted 45 lines

Also the spec seems confused as usual. Some parts if it refer to this as RVDA others as RVBT. Although RVBT also seems to be what the mbox #4 contents are called in another place, and to add insult to injury that place also has the offset and size all wrong. Sigh.

Anyway, apart from the missing rvda/rvds definititions the rest looks OK.

Quoted 28 lines

drivers/gpu/drm/i915/intel_opregion.c +14 -0

+			opregion->rvda = memremap(opregion->asle->rvda,
+						  opregion->asle->rvds,
+						  MEMREMAP_WB);
+			vbt = opregion->rvda;
+			vbt_size = opregion->asle->rvds;
+		}
 
 		if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
 			opregion->vbt = vbt;
 			opregion->vbt_size = vbt_size;
+			DRM_DEBUG_DRIVER("VBT from RVDA\n");
+		} else {
+			vbt = base + OPREGION_VBT_OFFSET;
+			vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET;
+			if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
+				opregion->vbt = vbt;
+				opregion->vbt_size = vbt_size;
+			}
 		}
 	}
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 4 lines · Ville Syrjälä

Quoted 2 lines · Ville Syrjälä

On Mon, Dec 14, 2015 at 12:50:55PM +0200, Jani Nikula wrote:
Either I'm blind or you didn't actually add rvda/rvds to struct opregion_asle.

Quoted 6 lines

Oh I se they went in seprately as
c85f6c91ec42 ("drm/i915: add VBT address and size fields to ASLE mailbox struct")

OK, so apart from the few minor bikesheds I listed it all looks good to me. For the series

Reviewed-by Ville Syrjälä <[email protected]>

r-b still holds for the patches with the revised debug prints.

Quoted 39 lines · Jani Nikula

The RVDA and RVDS (raw VBT data address and size) fields of the ASLE mailbox may specify an alternate location for VBT instead of mailbox #4. Use the alternate location if available and valid, falling back to mailbox #4 otherwise.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/intel_opregion.c | 25 +++++++++++++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +1 -0

index ca8c2a64bc6d..8cfac7398568 100644
@@ -457,6 +457,7 @@ struct intel_opregion {
 	u32 swsci_gbda_sub_functions;
 	u32 swsci_sbcb_sub_functions;
 	struct opregion_asle *asle;
+	void *rvda;
 	const void *vbt;
 	u32 vbt_size;
 	u32 *lid_state;

drivers/gpu/drm/i915/intel_opregion.c +4 -0

index e89ee2383fe1..a139889dd45b 100644
@@ -837,6 +837,10 @@ void intel_opregion_fini(struct drm_device *dev)
 
 	/* just clear all opregion memory pointers now */
 	memunmap(opregion->header);
+	if (opregion->rvda) {
+		memunmap(opregion->rvda);
+		opregion->rvda = NULL;
+	}
 	opregion->header = NULL;
 	opregion->acpi = NULL;
 	opregion->swsci = NULL;

drivers/gpu/drm/i915/intel_opregion.c int intel_opregion_setup(struct drm_device *dev) · line 998

Quoted 10 lines

drivers/gpu/drm/i915/intel_opregion.c +5 -2

@@ -987,12 +991,29 @@ int intel_opregion_setup(struct drm_device *dev)
 	`	DRM_DEBUG_DRIVER("ASLE extension supported\n");
 
 	if (!dmi_check_system(intel_no_opregion_vbt)) {
-		const void *vbt = base + OPREGION_VBT_OFFSET;
-		u32 vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET;
+		const void *vbt = NULL;
+		u32 vbt_size = 0;
+
+		if (opregion->header->opregion_ver >= 2 && opregion->asle &&
+		    opregion->asle->rvda && opregion->asle->rvds) {

Either I'm blind or you didn't actually add rvda/rvds to struct opregion_asle.

Also the spec seems confused as usual. Some parts if it refer to this as RVDA others as RVBT. Although RVBT also seems to be what the mbox #4 contents are called in another place, and to add insult to injury that place also has the offset and size all wrong. Sigh.

Anyway, apart from the missing rvda/rvds definititions the rest looks OK.

Quoted 29 lines

drivers/gpu/drm/i915/intel_opregion.c +14 -0

+			opregion->rvda = memremap(opregion->asle->rvda,
+						  opregion->asle->rvds,
+						  MEMREMAP_WB);
+			vbt = opregion->rvda;
+			vbt_size = opregion->asle->rvds;
+		}
 
 		if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
 			opregion->vbt = vbt;
 			opregion->vbt_size = vbt_size;
+			DRM_DEBUG_DRIVER("VBT from RVDA\n");
+		} else {
+			vbt = base + OPREGION_VBT_OFFSET;
+			vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET;
+			if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
+				opregion->vbt = vbt;
+				opregion->vbt_size = vbt_size;
+			}
 		}
 	}
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

The RVDA and RVDS (raw VBT data address and size) fields of the ASLE mailbox may specify an alternate location for VBT instead of mailbox #4. Use the alternate location if available and valid, falling back to mailbox #4 otherwise.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/intel_opregion.c | 25 +++++++++++++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +1 -0

index ca8c2a64bc6d..8cfac7398568 100644
@@ -457,6 +457,7 @@ struct intel_opregion {
 	u32 swsci_gbda_sub_functions;
 	u32 swsci_sbcb_sub_functions;
 	struct opregion_asle *asle;
+	void *rvda;
 	const void *vbt;
 	u32 vbt_size;
 	u32 *lid_state;

drivers/gpu/drm/i915/intel_opregion.c +16 -2

index e89ee2383fe1..a139889dd45b 100644
@@ -837,6 +837,10 @@ void intel_opregion_fini(struct drm_device *dev)
 
 	/* just clear all opregion memory pointers now */
 	memunmap(opregion->header);
+	if (opregion->rvda) {
+		memunmap(opregion->rvda);
+		opregion->rvda = NULL;
+	}
 	opregion->header = NULL;
 	opregion->acpi = NULL;
 	opregion->swsci = NULL;

@@ -987,12 +991,29 @@ int intel_opregion_setup(struct drm_device *dev)
 		DRM_DEBUG_DRIVER("ASLE extension supported\n");
 
 	if (!dmi_check_system(intel_no_opregion_vbt)) {
-		const void *vbt = base + OPREGION_VBT_OFFSET;
-		u32 vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET;
+		const void *vbt = NULL;
+		u32 vbt_size = 0;
+
+		if (opregion->header->opregion_ver >= 2 && opregion->asle &&
+		    opregion->asle->rvda && opregion->asle->rvds) {
+			opregion->rvda = memremap(opregion->asle->rvda,
+						  opregion->asle->rvds,
+						  MEMREMAP_WB);
+			vbt = opregion->rvda;
+			vbt_size = opregion->asle->rvds;
+		}
 
 		if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
 			opregion->vbt = vbt;
 			opregion->vbt_size = vbt_size;
+			DRM_DEBUG_DRIVER("VBT from RVDA\n");

Quoted 5 lines · Jani Nikula

The RVDA and RVDS (raw VBT data address and size) fields of the ASLE mailbox may specify an alternate location for VBT instead of mailbox #4. Use the alternate location if available and valid, falling back to mailbox #4 otherwise.

Tested-by Mika Kahola <[email protected]>

Quoted 1 line

Signed-off-by Jani Nikula <[email protected]>

Quoted 64 lines

---
 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/intel_opregion.c | 25 +++++++++++++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +1 -0

index ca8c2a64bc6d..8cfac7398568 100644
@@ -457,6 +457,7 @@ struct intel_opregion {
 	u32 swsci_gbda_sub_functions;
 	u32 swsci_sbcb_sub_functions;
 	struct opregion_asle *asle;
+	void *rvda;
 	const void *vbt;
 	u32 vbt_size;
 	u32 *lid_state;

drivers/gpu/drm/i915/intel_opregion.c +23 -2

index e89ee2383fe1..a139889dd45b 100644
@@ -837,6 +837,10 @@ void intel_opregion_fini(struct drm_device *dev)
 
 	/* just clear all opregion memory pointers now */
 	memunmap(opregion->header);
+	if (opregion->rvda) {
+		memunmap(opregion->rvda);
+		opregion->rvda = NULL;
+	}
 	opregion->header = NULL;
 	opregion->acpi = NULL;
 	opregion->swsci = NULL;

@@ -987,12 +991,29 @@ int intel_opregion_setup(struct drm_device *dev)
 		DRM_DEBUG_DRIVER("ASLE extension supported\n");
 
 	if (!dmi_check_system(intel_no_opregion_vbt)) {
-		const void *vbt = base + OPREGION_VBT_OFFSET;
-		u32 vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET;
+		const void *vbt = NULL;
+		u32 vbt_size = 0;
+
+		if (opregion->header->opregion_ver >= 2 && opregion->asle &&
+		    opregion->asle->rvda && opregion->asle->rvds) {
+			opregion->rvda = memremap(opregion->asle->rvda,
+						  opregion->asle->rvds,
+						  MEMREMAP_WB);
+			vbt = opregion->rvda;
+			vbt_size = opregion->asle->rvds;
+		}
 
 		if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
 			opregion->vbt = vbt;
 			opregion->vbt_size = vbt_size;
+			DRM_DEBUG_DRIVER("VBT from RVDA\n");
+		} else {
+			vbt = base + OPREGION_VBT_OFFSET;
+			vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET;
+			if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
+				opregion->vbt = vbt;
+				opregion->vbt_size = vbt_size;
+			}
 		}
 	}