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

PATCH In review Jani Nikula 4 participants 33 messages Dec 14, 2015 – Dec 17, 2015

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Jani Nikula

author

2015-12-14 10:50 UTC #0 lore raw

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

This series refactors the VBT and OpRegion code to support VBT bigger than 6 KB. It's plenty of small, hopefully easy to review steps.

This series supersedes patches 2-6 of Deepak's series [1]. I took over because I would not have been able to adequately describe in review what I wanted done.

I'll continue with rebasing the rest of the patches on top of this one.

BR,
Jani.

[1] http://mid.gmane.org/[email protected]

Deepak M (1):
drm/i915: Add Intel opregion mailbox 5 structure

Jani Nikula (10):
drm/i915: move "no VBT in opregion" quirk to intel_opregion_setup() drm/i915/bios: have functions return vbt, not bdb, header pointer drm/i915/bios: move debug logging about VBT source to
intel_parse_bios()
drm/i915/bios: rename intel_parse_bios to intel_bios_init drm/i915: refactor VBT validation
drm/i915/opregion: make VBT size limit more strict drm/i915/opregion: make VBT pointer a const
drm/i915: don't use a temp buffer for opregion debugfs file drm/i915/debugfs: add a separate debugfs file for VBT drm/i915/opregion: handle VBT sizes bigger than 6 KB

 drivers/gpu/drm/i915/i915_debugfs.c   |  31 ++++++---
 drivers/gpu/drm/i915/i915_dma.c       |   2 +-
 drivers/gpu/drm/i915/i915_drv.h       |   8 ++-
 drivers/gpu/drm/i915/intel_bios.c     | 117 ++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_bios.h     |   2 -
 drivers/gpu/drm/i915/intel_opregion.c |  66 ++++++++++++++++++-
 6 files changed, 151 insertions(+), 75 deletions(-)

Signature · 6 lines

2.1.4

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

Jani Nikula

author

2015-12-14 10:50 UTC #1 lore raw

PATCH 1/11 drm/i915: Add Intel opregion mailbox 5 structure

From: Deepak M <[email protected]>

Mailbox 5 is BIOS to Driver Notification mailbox is intended to support BIOS to Driver event notification or data storage for BIOS to Driver data synchronization purpose. Mailbox 5 is the extension of mailbox 3.

v4 by Jani:
- don't add asle_ext to dev_priv as it's unused
- use u8 for bddc and rsvd fields in asle ext struct
- add BUILD_BUG_ON the asle ext struct size
- debug logging for asle ext present

Cc Jani Nikula <[email protected]>

Signed-off-by Deepak M <[email protected]>

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

---
 drivers/gpu/drm/i915/intel_opregion.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

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

index 64efedfad879..cd97b9a5df57 100644
@@ -46,6 +46,7 @@
 #define OPREGION_SWSCI_OFFSET  0x200
 #define OPREGION_ASLE_OFFSET   0x300
 #define OPREGION_VBT_OFFSET    0x400
+#define OPREGION_ASLE_EXT_OFFSET	0x1C00
 
 #define OPREGION_SIGNATURE "IntelGraphicsMem"
 #define MBOX_ACPI      (1<<0)

@@ -125,6 +126,13 @@ struct opregion_asle {
 	u8 rsvd[58];
 } __packed;
 
+/* OpRegion mailbox #5: ASLE ext */
+struct opregion_asle_ext {
+	u32 phed;	/* Panel Header */
+	u8 bddc[256];	/* Panel EDID */
+	u8 rsvd[764];
+} __packed;
+
 /* Driver readiness indicator */
 #define ASLE_ARDY_READY		(1 << 0)
 #define ASLE_ARDY_NOT_READY	(0 << 0)

@@ -909,6 +917,7 @@ int intel_opregion_setup(struct drm_device *dev)
 	BUILD_BUG_ON(sizeof(struct opregion_acpi) != 0x100);
 	BUILD_BUG_ON(sizeof(struct opregion_swsci) != 0x100);
 	BUILD_BUG_ON(sizeof(struct opregion_asle) != 0x100);
+	BUILD_BUG_ON(sizeof(struct opregion_asle_ext) != 0x400);
 
 	pci_read_config_dword(dev->pdev, PCI_ASLS, &asls);
 	DRM_DEBUG_DRIVER("graphic opregion physical addr: 0x%x\n", asls);

@@ -948,6 +957,7 @@ int intel_opregion_setup(struct drm_device *dev)
 		opregion->swsci = base + OPREGION_SWSCI_OFFSET;
 		swsci_setup(dev);
 	}
+
 	if (mboxes & MBOX_ASLE) {
 		DRM_DEBUG_DRIVER("ASLE supported\n");
 		opregion->asle = base + OPREGION_ASLE_OFFSET;

@@ -955,6 +965,9 @@ int intel_opregion_setup(struct drm_device *dev)
 		opregion->asle->ardy = ASLE_ARDY_NOT_READY;
 	}
 
+	if (mboxes & MBOX_ASLE_EXT)
+		DRM_DEBUG_DRIVER("ASLE extension supported\n");
+
 	return 0;
 
 err_out:

Signature · 6 lines

2.1.4

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

Jani Nikula

author

2015-12-14 10:51 UTC #2 lore raw

PATCH 2/11 drm/i915: move "no VBT in opregion" quirk to intel_opregion_setup()

In reply to Jani Nikula

Check the quirk in intel_opregion_setup(), and don't initialize opregion->vbt at all if the quirk says it's not present, hiding the quirk from the rest of the driver.

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

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

drivers/gpu/drm/i915/intel_bios.c +2 ~~-22~~

index 070470fe9a91..401e1141f55f 100644
@@ -24,7 +24,7 @@
  *    Eric Anholt <[email protected]>
  *
  */
-#include <linux/dmi.h>
+
 #include <drm/drm_dp_helper.h>
 #include <drm/drmP.h>
 #include <drm/i915_drm.h>

@@ -1214,26 +1214,6 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
 	}
 }
 
-static int intel_no_opregion_vbt_callback(const struct dmi_system_id *id)
-{
-	DRM_DEBUG_KMS("Falling back to manually reading VBT from "
-		      "VBIOS ROM for %s\n",
-		      id->ident);
-	return 1;
-}
-
-static const struct dmi_system_id intel_no_opregion_vbt[] = {
-	{
-		.callback = intel_no_opregion_vbt_callback,
-		.ident = "ThinkCentre A57",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
-			DMI_MATCH(DMI_PRODUCT_NAME, "97027RG"),
-		},
-	},
-	{ }
-};
-
 static const struct bdb_header *validate_vbt(const void *base,
 					     size_t size,
 					     const void *_vbt,

@@ -1317,7 +1297,7 @@ 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)
+	if (dev_priv->opregion.vbt)
 		bdb = validate_vbt(dev_priv->opregion.header, OPREGION_SIZE,
 				   dev_priv->opregion.vbt, "OpRegion");

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

index cd97b9a5df57..5b9fc790d300 100644
@@ -26,6 +26,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/dmi.h>
 #include <acpi/video.h>
 
 #include <drm/drmP.h>

@@ -904,6 +905,25 @@ static void swsci_setup(struct drm_device *dev)
 static inline void swsci_setup(struct drm_device *dev) {}
 #endif  /* CONFIG_ACPI */
 
+static int intel_no_opregion_vbt_callback(const struct dmi_system_id *id)
+{
+	DRM_DEBUG_KMS("Falling back to manually reading VBT from "
+		      "VBIOS ROM for %s\n", id->ident);
+	return 1;
+}
+
+static const struct dmi_system_id intel_no_opregion_vbt[] = {
+	{
+		.callback = intel_no_opregion_vbt_callback,
+		.ident = "ThinkCentre A57",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "97027RG"),
+		},
+	},
+	{ }
+};
+
 int intel_opregion_setup(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;

@@ -942,8 +962,6 @@ int intel_opregion_setup(struct drm_device *dev)
 		goto err_out;
 	}
 	opregion->header = base;
-	opregion->vbt = base + OPREGION_VBT_OFFSET;
-
 	opregion->lid_state = base + ACPI_CLID;
 
 	mboxes = opregion->header->mboxes;

@@ -968,6 +986,9 @@ 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;
+
 	return 0;
 
 err_out:

Signature · 6 lines

2.1.4

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

Jani Nikula

author

2015-12-14 10:51 UTC #3 lore raw

PATCH 3/11 drm/i915/bios: have functions return vbt, not bdb, header pointer

In reply to Jani Nikula

This will simplify further work.

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

---
 drivers/gpu/drm/i915/intel_bios.c | 41 +++++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 15 deletions(-)

drivers/gpu/drm/i915/intel_bios.c +26 ~~-15~~

index 401e1141f55f..2fc2a994f395 100644
@@ -1214,7 +1214,14 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
 	}
 }
 
-static const struct bdb_header *validate_vbt(const void *base,
+static const struct bdb_header *get_bdb_header(const struct vbt_header *vbt)
+{
+	const void *_vbt = vbt;
+
+	return _vbt + vbt->bdb_offset;
+}
+
+static const struct vbt_header *validate_vbt(const void *base,
 					     size_t size,
 					     const void *_vbt,
 					     const char *source)

@@ -1223,6 +1230,9 @@ static const struct bdb_header *validate_vbt(const void *base,
 	const struct vbt_header *vbt = _vbt;
 	const struct bdb_header *bdb;
 
+	if (!vbt)
+		return NULL;
+
 	if (offset + sizeof(struct vbt_header) > size) {
 		DRM_DEBUG_DRIVER("VBT header incomplete\n");
 		return NULL;

@@ -1239,7 +1249,7 @@ static const struct bdb_header *validate_vbt(const void *base,
 		return NULL;
 	}
 
-	bdb = base + offset;
+	bdb = get_bdb_header(vbt);
 	if (offset + bdb->bdb_size > size) {
 		DRM_DEBUG_DRIVER("BDB incomplete\n");
 		return NULL;

@@ -1247,12 +1257,12 @@ static const struct bdb_header *validate_vbt(const void *base,
 
 	DRM_DEBUG_KMS("Using VBT from %s: %20s\n",
 		      source, vbt->signature);
-	return bdb;
+	return vbt;
 }
 
-static const struct bdb_header *find_vbt(void __iomem *bios, size_t size)
+static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
 {
-	const struct bdb_header *bdb = NULL;
+	const struct vbt_header *vbt = NULL;
 	size_t i;
 
 	/* Scour memory looking for the VBT signature. */

@@ -1266,12 +1276,12 @@ static const struct bdb_header *find_vbt(void __iomem *bios, size_t size)
 			 */
 			void *_bios = (void __force *) bios;
 
-			bdb = validate_vbt(_bios, size, _bios + i, "PCI ROM");
+			vbt = validate_vbt(_bios, size, _bios + i, "PCI ROM");
 			break;
 		}
 	}
 
-	return bdb;
+	return vbt;
 }
 
 /**

@@ -1288,7 +1298,8 @@ intel_parse_bios(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct pci_dev *pdev = dev->pdev;
-	const struct bdb_header *bdb = NULL;
+	const struct vbt_header *vbt;
+	const struct bdb_header *bdb;
 	u8 __iomem *bios = NULL;
 
 	if (HAS_PCH_NOP(dev))

@@ -1297,24 +1308,24 @@ intel_parse_bios(struct drm_device *dev)
 	init_vbt_defaults(dev_priv);
 
 	/* XXX Should this validation be moved to intel_opregion.c? */
-	if (dev_priv->opregion.vbt)
-		bdb = validate_vbt(dev_priv->opregion.header, OPREGION_SIZE,
-				   dev_priv->opregion.vbt, "OpRegion");
-
-	if (bdb == NULL) {
+	vbt = validate_vbt(dev_priv->opregion.header, OPREGION_SIZE,
+			   dev_priv->opregion.vbt, "OpRegion");
+	if (!vbt) {
 		size_t size;
 
 		bios = pci_map_rom(pdev, &size);
 		if (!bios)
 			return -1;
 
-		bdb = find_vbt(bios, size);
-		if (!bdb) {
+		vbt = find_vbt(bios, size);
+		if (!vbt) {
 			pci_unmap_rom(pdev, bios);
 			return -1;
 		}
 	}
 
+	bdb = get_bdb_header(vbt);
+
 	/* Grab useful general definitions */
 	parse_general_features(dev_priv, bdb);
 	parse_general_definitions(dev_priv, bdb);

Signature · 6 lines

2.1.4

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

Jani Nikula

author

2015-12-14 10:51 UTC #4 lore raw

PATCH 4/11 drm/i915/bios: move debug logging about VBT source to intel_parse_bios()

In reply to Jani Nikula

The decision about which source will be used for VBT is done in intel_parse_bios(), not in the VBT validation function. Make the VBT validation function strictly about validation, and move the debug logging to where it logically belongs.

This will make even more sense in the future when the validation for ACPI OpRegion based VBT takes place at OpRegion setup time.

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

---
 drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

drivers/gpu/drm/i915/intel_bios.c +10 -7

index 2fc2a994f395..45a90d4a4a81 100644
@@ -1223,8 +1223,7 @@ static const struct bdb_header *get_bdb_header(const struct vbt_header *vbt)
 
 static const struct vbt_header *validate_vbt(const void *base,
 					     size_t size,
-					     const void *_vbt,
-					     const char *source)
+					     const void *_vbt)
 {
 	size_t offset = _vbt - base;
 	const struct vbt_header *vbt = _vbt;

@@ -1255,8 +1254,6 @@ static const struct vbt_header *validate_vbt(const void *base,
 		return NULL;
 	}
 
-	DRM_DEBUG_KMS("Using VBT from %s: %20s\n",
-		      source, vbt->signature);
 	return vbt;
 }

@@ -1276,7 +1273,7 @@ static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
 			 */
 			void *_bios = (void __force *) bios;
 
-			vbt = validate_vbt(_bios, size, _bios + i, "PCI ROM");
+			vbt = validate_vbt(_bios, size, _bios + i);
 			break;
 		}
 	}

@@ -1309,8 +1306,11 @@ intel_parse_bios(struct drm_device *dev)
 
 	/* XXX Should this validation be moved to intel_opregion.c? */
 	vbt = validate_vbt(dev_priv->opregion.header, OPREGION_SIZE,
-			   dev_priv->opregion.vbt, "OpRegion");
-	if (!vbt) {
+			   dev_priv->opregion.vbt);
+	if (vbt) {
+		DRM_DEBUG_KMS("Using VBT from ACPI OpRegion: %20s\n",
+			      vbt->signature);
+	} else {
 		size_t size;
 
 		bios = pci_map_rom(pdev, &size);

@@ -1322,6 +1322,9 @@ intel_parse_bios(struct drm_device *dev)
 			pci_unmap_rom(pdev, bios);
 			return -1;
 		}
+
+		DRM_DEBUG_KMS("Using VBT from PCI ROM: %20s\n",
+			      vbt->signature);
 	}
 
 	bdb = get_bdb_header(vbt);

Signature · 6 lines

2.1.4

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

Jani Nikula

author

2015-12-14 10:51 UTC #6 lore raw

PATCH 5/11 drm/i915/bios: rename intel_parse_bios to intel_bios_init

In reply to Jani Nikula

While at it, move the declaration to where everything else is declared.

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

---
 drivers/gpu/drm/i915/i915_dma.c   | 2 +-
 drivers/gpu/drm/i915/i915_drv.h   | 3 +++
 drivers/gpu/drm/i915/intel_bios.c | 4 ++--
 drivers/gpu/drm/i915/intel_bios.h | 2 --
 4 files changed, 6 insertions(+), 5 deletions(-)

drivers/gpu/drm/i915/i915_dma.c +1 -1

index 84e2b202ecb5..9cb71b6aed5d 100644
@@ -370,7 +370,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
-	ret = intel_parse_bios(dev);
+	ret = intel_bios_init(dev);
 	if (ret)
 		DRM_INFO("failed to find VBIOS tables\n");

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

index 912408539822..a2bd4c6a86a1 100644
@@ -3352,6 +3352,9 @@ static inline bool intel_gmbus_is_forced_bit(struct i2c_adapter *adapter)
 }
 extern void intel_i2c_reset(struct drm_device *dev);
 
+/* intel_bios.c */
+int intel_bios_init(struct drm_device *dev);
+
 /* intel_opregion.c */
 #ifdef CONFIG_ACPI
 extern int intel_opregion_setup(struct drm_device *dev);

drivers/gpu/drm/i915/intel_bios.c +2 -2

index 45a90d4a4a81..f2b79b7d12b8 100644
@@ -1282,7 +1282,7 @@ static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
 }
 
 /**
- * intel_parse_bios - find VBT and initialize settings from the BIOS
+ * intel_bios_init - find VBT and initialize settings from the BIOS
  * @dev: DRM device
  *
  * Loads the Video BIOS and checks that the VBT exists.  Sets scratch registers

@@ -1291,7 +1291,7 @@ static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
  * Returns 0 on success, nonzero on failure.
  */
 int
-intel_parse_bios(struct drm_device *dev)
+intel_bios_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct pci_dev *pdev = dev->pdev;

drivers/gpu/drm/i915/intel_bios.h +0 -2

index 7ec8c9aefb84..689eb42f863c 100644
@@ -588,8 +588,6 @@ struct bdb_psr {
 	struct psr_table psr_table[16];
 } __packed;
 
-int intel_parse_bios(struct drm_device *dev);
-
 /*
  * Driver<->VBIOS interaction occurs through scratch bits in
  * GR18 & SWF*.

Signature · 6 lines

2.1.4

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

Jani Nikula

author

2015-12-14 10:51 UTC #7 lore raw

PATCH 6/11 drm/i915: refactor VBT validation

In reply to 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 ~~-31~~

111 lines not on this page · raw message

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

Jani Nikula

author

2015-12-14 10:51 UTC #12 lore raw

PATCH 7/11 drm/i915/opregion: make VBT size limit more strict

In reply to Jani Nikula

The VBT in OpRegion should fit in mailbox #4.

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

---
 drivers/gpu/drm/i915/intel_opregion.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_opregion.c +1 -1

index 9f992e7c6185..3c76a8684ff2 100644
@@ -988,7 +988,7 @@ int intel_opregion_setup(struct drm_device *dev)
 
 	if (!dmi_check_system(intel_no_opregion_vbt)) {
 		void *vbt = base + OPREGION_VBT_OFFSET;
-		u32 vbt_size = OPREGION_SIZE - OPREGION_VBT_OFFSET;
+		u32 vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET;
 
 		if (intel_bios_is_valid_vbt(vbt, vbt_size))
 			opregion->vbt = vbt;

Signature · 6 lines

2.1.4

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

Jani Nikula

author

2015-12-14 10:51 UTC #13 lore raw

PATCH 8/11 drm/i915/opregion: make VBT pointer a const

In reply to Jani Nikula

Because we can. It's not to be touched so tell the compiler too.

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

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

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

index ef15f1710b50..10e47167c594 100644
@@ -457,7 +457,7 @@ struct intel_opregion {
 	u32 swsci_gbda_sub_functions;
 	u32 swsci_sbcb_sub_functions;
 	struct opregion_asle *asle;
-	void *vbt;
+	const void *vbt;
 	u32 *lid_state;
 	struct work_struct asle_work;
 };

drivers/gpu/drm/i915/intel_opregion.c +1 -1

index 3c76a8684ff2..f9403b1c8fdd 100644
@@ -987,7 +987,7 @@ int intel_opregion_setup(struct drm_device *dev)
 		DRM_DEBUG_DRIVER("ASLE extension supported\n");
 
 	if (!dmi_check_system(intel_no_opregion_vbt)) {
-		void *vbt = base + OPREGION_VBT_OFFSET;
+		const void *vbt = base + OPREGION_VBT_OFFSET;
 		u32 vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET;
 
 		if (intel_bios_is_valid_vbt(vbt, vbt_size))

Signature · 6 lines

2.1.4

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

Jani Nikula

author

2015-12-14 10:51 UTC #14 lore raw

PATCH 9/11 drm/i915: don't use a temp buffer for opregion debugfs file

In reply to 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 +2 -9

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;
 
-	if (data == NULL)
-		return -ENOMEM;
-
 	ret = mutex_lock_interruptible(&dev->struct_mutex);
 	if (ret)
 		goto out;
 
-	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

Jani Nikula

author

2015-12-14 10:51 UTC #18 lore raw

PATCH 10/11 drm/i915/debugfs: add a separate debugfs file for VBT

In reply to 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 +22 -0

index a9e1f18c36d1..aef1393e707f 100644
@@ -1857,6 +1857,27 @@ out:
 	return 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);
+	if (ret)
+		goto out;
+
+	if (opregion->vbt)
+		seq_write(m, opregion->vbt, opregion->vbt_size);
+
+	mutex_unlock(&dev->struct_mutex);
+
+out:
+	return 0;
+}
+
 static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 {
 	struct drm_info_node *node = m->private;

@@ -5325,6 +5346,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_ips_status", i915_ips_status, 0},
 	{"i915_sr_status", i915_sr_status, 0},
 	{"i915_opregion", i915_opregion, 0},
+	{"i915_vbt", i915_vbt, 0},
 	{"i915_gem_framebuffer", i915_gem_framebuffer_info, 0},
 	{"i915_context_status", i915_context_status, 0},
 	{"i915_dump_lrc", i915_dump_lrc, 0},

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

index 10e47167c594..ca8c2a64bc6d 100644
@@ -458,6 +458,7 @@ struct intel_opregion {
 	u32 swsci_sbcb_sub_functions;
 	struct opregion_asle *asle;
 	const void *vbt;
+	u32 vbt_size;
 	u32 *lid_state;
 	struct work_struct asle_work;
 };

drivers/gpu/drm/i915/intel_opregion.c +3 -1

index f9403b1c8fdd..e89ee2383fe1 100644
@@ -990,8 +990,10 @@ int intel_opregion_setup(struct drm_device *dev)
 		const void *vbt = base + OPREGION_VBT_OFFSET;
 		u32 vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET;
 
-		if (intel_bios_is_valid_vbt(vbt, vbt_size))
+		if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
 			opregion->vbt = vbt;
+			opregion->vbt_size = vbt_size;
+		}
 	}
 
 	return 0;

Signature · 6 lines

2.1.4

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

Jani Nikula

author

2015-12-14 10:51 UTC #24 lore raw

PATCH 11/11 drm/i915/opregion: handle VBT sizes bigger than 6 KB

In reply to 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 +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;
+			}
 		}
 	}

Signature · 6 lines

2.1.4

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

Chris Wilson

2015-12-14 10:57 UTC #19 lore raw

Re: [PATCH 10/11] drm/i915/debugfs: add a separate debugfs file for VBT

In reply to Jani Nikula

Quoted 17 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 10 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? -Chris

Signature · 5 lines

Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jani Nikula

author

2015-12-14 11:06 UTC #20 lore raw

Re: [PATCH 10/11] drm/i915/debugfs: add a separate debugfs file for VBT

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?

BR,
Jani.

Signature · 5 lines

Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Chris Wilson

2015-12-14 11:30 UTC #21 lore raw

Re: [PATCH 10/11] drm/i915/debugfs: add a separate debugfs file for VBT

Quoted 32 lines · Jani Nikula

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?

Quoted 2 lines

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

No, the only thing that we control here are the dev_priv->opregion pointers - and they are init only. So I think we are totally safe to drop the struct_mutex.
-Chris

Signature · 5 lines

Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jani Nikula

author

2015-12-14 13:20 UTC #22 lore raw

PATCH v2 10/11 drm/i915/debugfs: add a separate debugfs file for VBT

In reply to 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.

v2: Drop the locking as unneeded (Chris)

Cc Chris Wilson <[email protected]>

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

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

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

index a9e1f18c36d1..0fc38bb7276c 100644
@@ -1857,6 +1857,19 @@ out:
 	return 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;
+
+	if (opregion->vbt)
+		seq_write(m, opregion->vbt, opregion->vbt_size);
+
+	return 0;
+}
+
 static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 {
 	struct drm_info_node *node = m->private;

@@ -5325,6 +5338,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_ips_status", i915_ips_status, 0},
 	{"i915_sr_status", i915_sr_status, 0},
 	{"i915_opregion", i915_opregion, 0},
+	{"i915_vbt", i915_vbt, 0},
 	{"i915_gem_framebuffer", i915_gem_framebuffer_info, 0},
 	{"i915_context_status", i915_context_status, 0},
 	{"i915_dump_lrc", i915_dump_lrc, 0},

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

index 10e47167c594..ca8c2a64bc6d 100644
@@ -458,6 +458,7 @@ struct intel_opregion {
 	u32 swsci_sbcb_sub_functions;
 	struct opregion_asle *asle;
 	const void *vbt;
+	u32 vbt_size;
 	u32 *lid_state;
 	struct work_struct asle_work;
 };

drivers/gpu/drm/i915/intel_opregion.c +3 -1

index f9403b1c8fdd..e89ee2383fe1 100644
@@ -990,8 +990,10 @@ int intel_opregion_setup(struct drm_device *dev)
 		const void *vbt = base + OPREGION_VBT_OFFSET;
 		u32 vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET;
 
-		if (intel_bios_is_valid_vbt(vbt, vbt_size))
+		if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
 			opregion->vbt = vbt;
+			opregion->vbt_size = vbt_size;
+		}
 	}
 
 	return 0;

Signature · 6 lines

2.1.4

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

Ville Syrjälä

2015-12-14 13:41 UTC #8 lore raw

Re: [PATCH 06/11] drm/i915: refactor VBT validation

In reply to Jani Nikula

Quoted 135 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;

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

Quoted 9 lines

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

@@ -1304,9 +1308,6 @@ intel_bios_init(struct drm_device *dev)
 
 	init_vbt_defaults(dev_priv);
 
-	/* 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 27 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 · 6 lines

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

Ville Syrjälä

2015-12-14 13:45 UTC #15 lore raw

Re: [PATCH 09/11] drm/i915: don't use a temp buffer for opregion debugfs file

In reply to Jani Nikula

Quoted 20 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 -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 static int i915_opregion(struct seq_file *m, void *unused) · line 1849

Quoted 10 lines

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

 	struct intel_opregion *opregion = &dev_priv->opregion;
-	void *data = kmalloc(OPREGION_SIZE, GFP_KERNEL);
 	int ret;
 
-	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 22 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 · 6 lines

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

Ville Syrjälä

2015-12-14 14:19 UTC #25 lore raw

Re: [PATCH 11/11] drm/i915/opregion: handle VBT sizes bigger than 6 KB

In reply to Jani Nikula

Quoted 39 lines · Jani Nikula

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 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

Ville Syrjälä

Reviewed-by

2015-12-14 14:34 UTC #26 lore raw

Re: [PATCH 11/11] drm/i915/opregion: handle VBT sizes bigger than 6 KB

Quoted 42 lines · Ville Syrjälä

Quoted 41 lines · Jani Nikula

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 44 lines

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

Jani Nikula

author

2015-12-14 14:34 UTC #9 lore raw

Re: [PATCH 06/11] drm/i915: refactor VBT validation

In reply to Ville Syrjälä

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

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 · 5 lines

Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jani Nikula

author

2015-12-14 14:39 UTC #16 lore raw

Re: [PATCH 09/11] drm/i915: don't use a temp buffer for opregion debugfs file

In reply to Ville Syrjälä

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

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 · 5 lines

Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jani Nikula

author

2015-12-14 14:40 UTC #29 lore raw

Re: [PATCH 11/11] drm/i915/opregion: handle VBT sizes bigger than 6 KB

In reply to Ville Syrjälä

Quoted 53 lines · Ville Syrjälä

Quoted 39 lines · Jani Nikula

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.

Quoted 6 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.

Those are in Deepak's commit I pushed last week:

commit c85f6c91ec4218487a29a7d1f918f7f6f0424c75

Author: Deepak M <[email protected]>
Date:   Tue Dec 1 04:17:05 2015 +0530

drm/i915: add VBT address and size fields to ASLE mailbox struct

I did build this, and even booted it. ;)

BR,
Jani.

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

Signature · 5 lines

Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Ville Syrjälä

2015-12-14 14:43 UTC #10 lore raw

Re: [PATCH 06/11] drm/i915: refactor VBT validation

In reply to Jani Nikula

Quoted 150 lines · Jani Nikula

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.

Quoted 3 lines

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.

But when you add the RVDA thing, the debug print for that is in the opregion code, so with that we get two debug prints?

Quoted 37 lines

BR,
Jani.

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

Signature · 6 lines

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

Ville Syrjälä

2015-12-14 14:45 UTC #17 lore raw

Re: [PATCH 09/11] drm/i915: don't use a temp buffer for opregion debugfs file

In reply to Jani Nikula

Quoted 35 lines · Jani Nikula

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?

Quoted 5 lines

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.

Do we actually take struct_mutex in places where we intentionally poke at opregion mailboxes?

The BIOS notifications could anyway be fully asynchronous no? So no way we could protect against that, except maybe via some ACPI AML lock.

Quoted 30 lines

BR,
Jani.

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

Signature · 6 lines

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

Jani Nikula

author

2015-12-15 11:14 UTC #5 lore raw

PATCH v2 4/11 drm/i915/bios: move debug logging about VBT source to intel_parse_bios()

In reply to Jani Nikula

Also split the logging about where the valid VBT was found and what the signature is. This will make even more sense in the future when the validation for ACPI OpRegion based VBT takes place at OpRegion setup time.

v2: Split logging about VBT signature and BDB version.

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

---
 drivers/gpu/drm/i915/intel_bios.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

drivers/gpu/drm/i915/intel_bios.c +11 -7

index 2fc2a994f395..1c86bc2b628f 100644
@@ -1223,8 +1223,7 @@ static const struct bdb_header *get_bdb_header(const struct vbt_header *vbt)
 
 static const struct vbt_header *validate_vbt(const void *base,
 					     size_t size,
-					     const void *_vbt,
-					     const char *source)
+					     const void *_vbt)
 {
 	size_t offset = _vbt - base;
 	const struct vbt_header *vbt = _vbt;

@@ -1255,8 +1254,6 @@ static const struct vbt_header *validate_vbt(const void *base,
 		return NULL;
 	}
 
-	DRM_DEBUG_KMS("Using VBT from %s: %20s\n",
-		      source, vbt->signature);
 	return vbt;
 }

@@ -1276,7 +1273,7 @@ static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
 			 */
 			void *_bios = (void __force *) bios;
 
-			vbt = validate_vbt(_bios, size, _bios + i, "PCI ROM");
+			vbt = validate_vbt(_bios, size, _bios + i);
 			break;
 		}
 	}

@@ -1309,8 +1306,10 @@ intel_parse_bios(struct drm_device *dev)
 
 	/* XXX Should this validation be moved to intel_opregion.c? */
 	vbt = validate_vbt(dev_priv->opregion.header, OPREGION_SIZE,
-			   dev_priv->opregion.vbt, "OpRegion");
-	if (!vbt) {
+			   dev_priv->opregion.vbt);
+	if (vbt) {
+		DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion\n");
+	} else {
 		size_t size;
 
 		bios = pci_map_rom(pdev, &size);

@@ -1322,10 +1321,15 @@ intel_parse_bios(struct drm_device *dev)
 			pci_unmap_rom(pdev, bios);
 			return -1;
 		}
+
+		DRM_DEBUG_KMS("Found valid VBT in PCI ROM\n");
 	}
 
 	bdb = get_bdb_header(vbt);
 
+	DRM_DEBUG_KMS("VBT signature \"%20s\", BDB version %d\n",
+		      vbt->signature, bdb->version);
+
 	/* Grab useful general definitions */
 	parse_general_features(dev_priv, bdb);
 	parse_general_definitions(dev_priv, bdb);

Signature · 6 lines

2.1.4

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

Jani Nikula

author

2015-12-15 11:16 UTC #11 lore raw

PATCH v2 6/11 drm/i915: refactor VBT validation

In reply to 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.

v2: move logging about valid VBT to opregion setup too (Ville)

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

---
 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/intel_bios.c     | 67 +++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_opregion.c | 11 ++++--
 3 files changed, 43 insertions(+), 36 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 +33 ~~-34~~

115 lines not on this page · raw message

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

index 5b9fc790d300..859c9acbee73 100644
@@ -986,8 +986,15 @@ 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)) {
+			DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion\n");
+			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

Jani Nikula

author

2015-12-15 11:17 UTC #23 lore raw

PATCH v3 10/11 drm/i915/debugfs: add a separate debugfs file for VBT

In reply to 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.

v2: Drop the locking as unneeded (Chris)
v3: Rebase

Cc Chris Wilson <[email protected]>

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

---
 drivers/gpu/drm/i915/i915_debugfs.c   | 14 ++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/intel_opregion.c |  1 +
 3 files changed, 16 insertions(+)

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

index a9e1f18c36d1..0fc38bb7276c 100644
@@ -1857,6 +1857,19 @@ out:
 	return 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;
+
+	if (opregion->vbt)
+		seq_write(m, opregion->vbt, opregion->vbt_size);
+
+	return 0;
+}
+
 static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 {
 	struct drm_info_node *node = m->private;

@@ -5325,6 +5338,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_ips_status", i915_ips_status, 0},
 	{"i915_sr_status", i915_sr_status, 0},
 	{"i915_opregion", i915_opregion, 0},
+	{"i915_vbt", i915_vbt, 0},
 	{"i915_gem_framebuffer", i915_gem_framebuffer_info, 0},
 	{"i915_context_status", i915_context_status, 0},
 	{"i915_dump_lrc", i915_dump_lrc, 0},

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

index 10e47167c594..ca8c2a64bc6d 100644
@@ -458,6 +458,7 @@ struct intel_opregion {
 	u32 swsci_sbcb_sub_functions;
 	struct opregion_asle *asle;
 	const void *vbt;
+	u32 vbt_size;
 	u32 *lid_state;
 	struct work_struct asle_work;
 };

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

index e1795de440db..53164a05ff7a 100644
@@ -993,6 +993,7 @@ int intel_opregion_setup(struct drm_device *dev)
 		if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
 			DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion\n");
 			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

Jani Nikula

author

2015-12-15 11:18 UTC #30 lore raw

PATCH v2 11/11 drm/i915/opregion: handle VBT sizes bigger than 6 KB

In reply to Jani Nikula

v2: Update debug logging (Ville)

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

---
 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/intel_opregion.c | 27 ++++++++++++++++++++++++---
 2 files changed, 25 insertions(+), 3 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 +24 -3

index 53164a05ff7a..4a8db5522432 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,13 +991,30 @@ 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)) {
-			DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion\n");
+			DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion (RVDA)\n");
 			opregion->vbt = vbt;
 			opregion->vbt_size = vbt_size;
+		} else {
+			vbt = base + OPREGION_VBT_OFFSET;
+			vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET;
+			if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
+				DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion (Mailbox #4)\n");
+				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

Ville Syrjälä

2015-12-15 14:03 UTC #27 lore raw

Re: [PATCH 11/11] drm/i915/opregion: handle VBT sizes bigger than 6 KB

In reply to Ville Syrjälä

Quoted 54 lines · Ville Syrjälä

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.

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 53 lines

Quoted 45 lines

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

Signature · 6 lines

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

Jani Nikula

author

2015-12-16 09:38 UTC #28 lore raw

Re: [PATCH 11/11] drm/i915/opregion: handle VBT sizes bigger than 6 KB

Quoted 5 lines · Ville Syrjälä

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 8 lines

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.

Patches 1-10 pushed to drm-intel-next-queued, thanks for the review. I'm still holding off pushing patch 11 until I get a Tested-by from Mika, as I don't have a machine with rvda set.

BR,
Jani.

Signature · 5 lines

Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Mika Kahola

Tested-by on v1 11/11

2015-12-17 07:30 UTC #31 lore raw

Re: [PATCH 11/11] drm/i915/opregion: handle VBT sizes bigger than 6 KB

In reply to Jani Nikula

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 55 lines

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");

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

Quoted 10 lines

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

+		} 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;
+			}
 		}
 	}

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

Jani Nikula

author

2015-12-17 09:46 UTC #32 lore raw

Re: [PATCH 11/11] drm/i915/opregion: handle VBT sizes bigger than 6 KB

Quoted 8 lines · Mika Kahola

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]>

Pushed to drm-intel-next-queued, thanks for the review and testing.

BR,
Jani.

Quoted 66 lines

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;
+			}
 		}
 	}

Signature · 5 lines

Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx