[PATCH 00/11] drm/i915: VBT/opregion refactor to support >6 KB VBT

[PATCH 00/11] drm/i915: VBT/opregion refactor to support >6 KB VBT

[Jani Nikula]

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/1448923632-16760-1-git-send-email-m.deepak@intel.com


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

-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/11] drm/i915: Add Intel opregion mailbox 5 structure

[Jani Nikula]

From: Deepak M <m.deepak@intel.com>

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 <jani.nikula@intel.com>
Signed-off-by: Deepak M <m.deepak@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_opregion.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 64efedfad879..cd97b9a5df57 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -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:
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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 <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c     | 24 ++----------------------
 drivers/gpu/drm/i915/intel_opregion.c | 25 +++++++++++++++++++++++--
 2 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 070470fe9a91..401e1141f55f 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -24,7 +24,7 @@
  *    Eric Anholt <eric@anholt.net>
  *
  */
-#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");
 
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index cd97b9a5df57..5b9fc790d300 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -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:
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

This will simplify further work.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c | 41 +++++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 401e1141f55f..2fc2a994f395 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -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);
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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 <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 2fc2a994f395..45a90d4a4a81 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -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);
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/11] drm/i915/bios: rename intel_parse_bios to intel_bios_init

[Jani Nikula]

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

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 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(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 84e2b202ecb5..9cb71b6aed5d 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -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");
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 912408539822..a2bd4c6a86a1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -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);
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 45a90d4a4a81..f2b79b7d12b8 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -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;
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 7ec8c9aefb84..689eb42f863c 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -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*.
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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 <jani.nikula@intel.com>
---
 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(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a2bd4c6a86a1..ef15f1710b50 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -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
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index f2b79b7d12b8..007b2b759600 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -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);
 
-	/* 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);
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 5b9fc790d300..9f992e7c6185 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -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;
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/11] drm/i915/opregion: make VBT size limit more strict

[Jani Nikula]

The VBT in OpRegion should fit in mailbox #4.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_opregion.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 9f992e7c6185..3c76a8684ff2 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -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;
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/11] drm/i915/opregion: make VBT pointer a const

[Jani Nikula]

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

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       | 2 +-
 drivers/gpu/drm/i915/intel_opregion.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ef15f1710b50..10e47167c594 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -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;
 };
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 3c76a8684ff2..f9403b1c8fdd 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -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))
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

Hasn't been necessary since

commit 115719fceaa733d646e39cdce83cc32ddb891a49
Author: Williams, Dan J <dan.j.williams@intel.com>
Date:   Mon Oct 12 21:12:57 2015 +0000

    i915: switch from acpi_os_ioremap to memremap

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 24318b79bcfc..a9e1f18c36d1 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -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;
 }
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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 <jani.nikula@intel.com>
---
 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(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index a9e1f18c36d1..aef1393e707f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -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},
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 10e47167c594..ca8c2a64bc6d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -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;
 };
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index f9403b1c8fdd..e89ee2383fe1 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -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;
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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 <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/intel_opregion.c | 25 +++++++++++++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ca8c2a64bc6d..8cfac7398568 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -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;
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index e89ee2383fe1..a139889dd45b 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -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;
+			}
 		}
 	}
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Mon, Dec 14, 2015 at 12:50:54PM +0200, Jani Nikula wrote:
> 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 <jani.nikula@intel.com>
> ---
>  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(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index a9e1f18c36d1..aef1393e707f 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -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);

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

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

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

[Jani Nikula]

On Mon, 14 Dec 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, Dec 14, 2015 at 12:50:54PM +0200, Jani Nikula wrote:
>> 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 <jani.nikula@intel.com>
>> ---
>>  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(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index a9e1f18c36d1..aef1393e707f 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -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);
>
> 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.



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

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

[Chris Wilson]

On Mon, Dec 14, 2015 at 01:06:51PM +0200, Jani Nikula wrote:
> On Mon, 14 Dec 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Mon, Dec 14, 2015 at 12:50:54PM +0200, Jani Nikula wrote:
> >> 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 <jani.nikula@intel.com>
> >> ---
> >>  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(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >> index a9e1f18c36d1..aef1393e707f 100644
> >> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >> @@ -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);
> >
> > 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?

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

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

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

[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 <chris@chris-wilson.co.uk>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 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(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index a9e1f18c36d1..0fc38bb7276c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -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},
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 10e47167c594..ca8c2a64bc6d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -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;
 };
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index f9403b1c8fdd..e89ee2383fe1 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -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;
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Mon, Dec 14, 2015 at 12:50:50PM +0200, Jani Nikula wrote:
> 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 <jani.nikula@intel.com>
> ---
>  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(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a2bd4c6a86a1..ef15f1710b50 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -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
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index f2b79b7d12b8..007b2b759600 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -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);
>  
> -	/* 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.

> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 5b9fc790d300..9f992e7c6185 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -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;
>  
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Ville Syrjälä]

On Mon, Dec 14, 2015 at 12:50:53PM +0200, Jani Nikula wrote:
> Hasn't been necessary since
> 
> commit 115719fceaa733d646e39cdce83cc32ddb891a49
> Author: Williams, Dan J <dan.j.williams@intel.com>
> Date:   Mon Oct 12 21:12:57 2015 +0000
> 
>     i915: switch from acpi_os_ioremap to memremap
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 24318b79bcfc..a9e1f18c36d1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -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;

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

>  
> -	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;
>  }
>  
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Ville Syrjälä]

On Mon, Dec 14, 2015 at 12:50:55PM +0200, Jani Nikula wrote:
> 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 <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |  1 +
>  drivers/gpu/drm/i915/intel_opregion.c | 25 +++++++++++++++++++++++--
>  2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ca8c2a64bc6d..8cfac7398568 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -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;
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index e89ee2383fe1..a139889dd45b 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -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) {

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.

> +			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;
> +			}
>  		}
>  	}
>  
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Ville Syrjälä]

On Mon, Dec 14, 2015 at 04:19:50PM +0200, Ville Syrjälä wrote:
> On Mon, Dec 14, 2015 at 12:50:55PM +0200, Jani Nikula wrote:
> > 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 <jani.nikula@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h       |  1 +
> >  drivers/gpu/drm/i915/intel_opregion.c | 25 +++++++++++++++++++++++--
> >  2 files changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index ca8c2a64bc6d..8cfac7398568 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -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;
> > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> > index e89ee2383fe1..a139889dd45b 100644
> > --- a/drivers/gpu/drm/i915/intel_opregion.c
> > +++ b/drivers/gpu/drm/i915/intel_opregion.c
> > @@ -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) {
> 
> 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ä <ville.syrjala@linux.intel.com>

> 
> 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.
> 
> > +			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;
> > +			}
> >  		}
> >  	}
> >  
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Jani Nikula]

On Mon, 14 Dec 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Mon, Dec 14, 2015 at 12:50:50PM +0200, Jani Nikula wrote:
>> 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 <jani.nikula@intel.com>
>> ---
>>  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(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index a2bd4c6a86a1..ef15f1710b50 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -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
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index f2b79b7d12b8..007b2b759600 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -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);
>>  
>> -	/* 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.



>
>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> index 5b9fc790d300..9f992e7c6185 100644
>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> @@ -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;
>>  
>> -- 
>> 2.1.4
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Jani Nikula]

On Mon, 14 Dec 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Mon, Dec 14, 2015 at 12:50:53PM +0200, Jani Nikula wrote:
>> Hasn't been necessary since
>> 
>> commit 115719fceaa733d646e39cdce83cc32ddb891a49
>> Author: Williams, Dan J <dan.j.williams@intel.com>
>> Date:   Mon Oct 12 21:12:57 2015 +0000
>> 
>>     i915: switch from acpi_os_ioremap to memremap
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c | 11 ++---------
>>  1 file changed, 2 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 24318b79bcfc..a9e1f18c36d1 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -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;
>
> 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.

>
>>  
>> -	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;
>>  }
>>  
>> -- 
>> 2.1.4
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Jani Nikula]

On Mon, 14 Dec 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Mon, Dec 14, 2015 at 12:50:55PM +0200, Jani Nikula wrote:
>> 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 <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h       |  1 +
>>  drivers/gpu/drm/i915/intel_opregion.c | 25 +++++++++++++++++++++++--
>>  2 files changed, 24 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index ca8c2a64bc6d..8cfac7398568 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -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;
>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> index e89ee2383fe1..a139889dd45b 100644
>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> @@ -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) {
>
> 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.

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

commit c85f6c91ec4218487a29a7d1f918f7f6f0424c75
Author: Deepak M <m.deepak@intel.com>
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.


>
>> +			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;
>> +			}
>>  		}
>>  	}
>>  
>> -- 
>> 2.1.4
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Ville Syrjälä]

On Mon, Dec 14, 2015 at 04:34:50PM +0200, Jani Nikula wrote:
> On Mon, 14 Dec 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Mon, Dec 14, 2015 at 12:50:50PM +0200, Jani Nikula wrote:
> >> 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 <jani.nikula@intel.com>
> >> ---
> >>  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(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index a2bd4c6a86a1..ef15f1710b50 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -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
> >> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> >> index f2b79b7d12b8..007b2b759600 100644
> >> --- a/drivers/gpu/drm/i915/intel_bios.c
> >> +++ b/drivers/gpu/drm/i915/intel_bios.c
> >> @@ -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);
> >>  
> >> -	/* 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.

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?

> 
> BR,
> Jani.
> 
> 
> 
> >
> >> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> >> index 5b9fc790d300..9f992e7c6185 100644
> >> --- a/drivers/gpu/drm/i915/intel_opregion.c
> >> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> >> @@ -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;
> >>  
> >> -- 
> >> 2.1.4
> >> 
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

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

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

[Ville Syrjälä]

On Mon, Dec 14, 2015 at 04:38:58PM +0200, Jani Nikula wrote:
> On Mon, 14 Dec 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Mon, Dec 14, 2015 at 12:50:53PM +0200, Jani Nikula wrote:
> >> Hasn't been necessary since
> >> 
> >> commit 115719fceaa733d646e39cdce83cc32ddb891a49
> >> Author: Williams, Dan J <dan.j.williams@intel.com>
> >> Date:   Mon Oct 12 21:12:57 2015 +0000
> >> 
> >>     i915: switch from acpi_os_ioremap to memremap
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_debugfs.c | 11 ++---------
> >>  1 file changed, 2 insertions(+), 9 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >> index 24318b79bcfc..a9e1f18c36d1 100644
> >> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >> @@ -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;
> >
> > 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.

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.

> 
> BR,
> Jani.
> 
> >
> >>  
> >> -	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;
> >>  }
> >>  
> >> -- 
> >> 2.1.4
> >> 
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

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

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

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

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 <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 2fc2a994f395..1c86bc2b628f 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -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);
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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 <jani.nikula@intel.com>
---
 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(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a2bd4c6a86a1..ef15f1710b50 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -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
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 04fb9ded1805..dc3a0fb1946c 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -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,12 +1308,7 @@ 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("Found valid VBT in ACPI OpRegion\n");
-	} else {
+	if (!vbt) {
 		size_t size;
 
 		bios = pci_map_rom(pdev, &size);
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 5b9fc790d300..859c9acbee73 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -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;
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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 <chris@chris-wilson.co.uk>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 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(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index a9e1f18c36d1..0fc38bb7276c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -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},
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 10e47167c594..ca8c2a64bc6d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -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;
 };
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index e1795de440db..53164a05ff7a 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -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;
 		}
 	}
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

v2: Update debug logging (Ville)

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/intel_opregion.c | 27 ++++++++++++++++++++++++---
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ca8c2a64bc6d..8cfac7398568 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -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;
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 53164a05ff7a..4a8db5522432 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -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;
+			}
 		}
 	}
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Mon, Dec 14, 2015 at 04:34:34PM +0200, Ville Syrjälä wrote:
> On Mon, Dec 14, 2015 at 04:19:50PM +0200, Ville Syrjälä wrote:
> > On Mon, Dec 14, 2015 at 12:50:55PM +0200, Jani Nikula wrote:
> > > 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 <jani.nikula@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h       |  1 +
> > >  drivers/gpu/drm/i915/intel_opregion.c | 25 +++++++++++++++++++++++--
> > >  2 files changed, 24 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index ca8c2a64bc6d..8cfac7398568 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -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;
> > > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> > > index e89ee2383fe1..a139889dd45b 100644
> > > --- a/drivers/gpu/drm/i915/intel_opregion.c
> > > +++ b/drivers/gpu/drm/i915/intel_opregion.c
> > > @@ -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) {
> > 
> > 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ä <ville.syrjala@linux.intel.com>

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

> 
> > 
> > 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.
> > 
> > > +			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;
> > > +			}
> > >  		}
> > >  	}
> > >  
> > > -- 
> > > 2.1.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Jani Nikula]

On Tue, 15 Dec 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Mon, Dec 14, 2015 at 04:34:34PM +0200, Ville Syrjälä wrote:
>> On Mon, Dec 14, 2015 at 04:19:50PM +0200, Ville Syrjälä wrote:
>> > 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.
>> 
>> 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ä <ville.syrjala@linux.intel.com>
>
> 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.


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

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

[Mika Kahola]

On Mon, 2015-12-14 at 12:50 +0200, Jani Nikula wrote:
> 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 <mika.kahola@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |  1 +
>  drivers/gpu/drm/i915/intel_opregion.c | 25 +++++++++++++++++++++++--
>  2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ca8c2a64bc6d..8cfac7398568 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -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;
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index e89ee2383fe1..a139889dd45b 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -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;
> +			}
>  		}
>  	}
>  


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Thu, 17 Dec 2015, Mika Kahola <mika.kahola@intel.com> wrote:
> On Mon, 2015-12-14 at 12:50 +0200, Jani Nikula wrote:
>> 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 <mika.kahola@intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

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

BR,
Jani.


>> ---
>>  drivers/gpu/drm/i915/i915_drv.h       |  1 +
>>  drivers/gpu/drm/i915/intel_opregion.c | 25 +++++++++++++++++++++++--
>>  2 files changed, 24 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index ca8c2a64bc6d..8cfac7398568 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -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;
>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> index e89ee2383fe1..a139889dd45b 100644
>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> @@ -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;
>> +			}
>>  		}
>>  	}
>>  
>
>

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