[PATCH RESEND 1/4] drm/i915/opregion: bail out early for systems with no opregion VBT

[PATCH RESEND 1/4] drm/i915/opregion: bail out early for systems with no opregion VBT

[Jani Nikula]

Reduce indent. No functional changes.

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

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 441c01466384..76a39ee6d005 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -920,6 +920,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
 	char buf[sizeof(OPREGION_SIGNATURE)];
 	int err = 0;
 	void *base;
+	const void *vbt = NULL;
+	u32 vbt_size = 0;
 
 	BUILD_BUG_ON(sizeof(struct opregion_header) != 0x100);
 	BUILD_BUG_ON(sizeof(struct opregion_acpi) != 0x100);
@@ -972,45 +974,43 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
 	if (mboxes & MBOX_ASLE_EXT)
 		DRM_DEBUG_DRIVER("ASLE extension supported\n");
 
-	if (!dmi_check_system(intel_no_opregion_vbt)) {
-		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 (dmi_check_system(intel_no_opregion_vbt))
+		goto out;
+
+	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 (RVDA)\n");
+		opregion->vbt = vbt;
+		opregion->vbt_size = vbt_size;
+	} else {
+		vbt = base + OPREGION_VBT_OFFSET;
+		/*
+		 * The VBT specification says that if the ASLE ext mailbox is
+		 * not used its area is reserved, but on some CHT boards the VBT
+		 * extends into the ASLE ext area. Allow this even though it is
+		 * against the spec, so we do not end up rejecting the VBT on
+		 * those boards (and end up not finding the LCD panel because of
+		 * this).
+		 */
+		vbt_size = (mboxes & MBOX_ASLE_EXT) ?
+			OPREGION_ASLE_EXT_OFFSET : OPREGION_SIZE;
+		vbt_size -= OPREGION_VBT_OFFSET;
 		if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
-			DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion (RVDA)\n");
+			DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion (Mailbox #4)\n");
 			opregion->vbt = vbt;
 			opregion->vbt_size = vbt_size;
-		} else {
-			vbt = base + OPREGION_VBT_OFFSET;
-			/*
-			 * The VBT specification says that if the ASLE ext
-			 * mailbox is not used its area is reserved, but
-			 * on some CHT boards the VBT extends into the
-			 * ASLE ext area. Allow this even though it is
-			 * against the spec, so we do not end up rejecting
-			 * the VBT on those boards (and end up not finding the
-			 * LCD panel because of this).
-			 */
-			vbt_size = (mboxes & MBOX_ASLE_EXT) ?
-				OPREGION_ASLE_EXT_OFFSET : OPREGION_SIZE;
-			vbt_size -= 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;
-			}
 		}
 	}
 
+out:
 	return 0;
 
 err_out:
-- 
2.1.4

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

[PATCH RESEND 2/4] drm/i915/opregion: try to validate RVDA VBT only if it's there

[Jani Nikula]

Seems more sensible this way, and reduces indent for the more common
case.

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

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 76a39ee6d005..2b64cb6691eb 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -920,8 +920,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
 	char buf[sizeof(OPREGION_SIGNATURE)];
 	int err = 0;
 	void *base;
-	const void *vbt = NULL;
-	u32 vbt_size = 0;
+	const void *vbt;
+	u32 vbt_size;
 
 	BUILD_BUG_ON(sizeof(struct opregion_header) != 0x100);
 	BUILD_BUG_ON(sizeof(struct opregion_acpi) != 0x100);
@@ -984,30 +984,29 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
 					  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 (RVDA)\n");
+			opregion->vbt = vbt;
+			opregion->vbt_size = vbt_size;
+			goto out;
+		}
 	}
 
+	vbt = base + OPREGION_VBT_OFFSET;
+	/*
+	 * The VBT specification says that if the ASLE ext mailbox is not used
+	 * its area is reserved, but on some CHT boards the VBT extends into the
+	 * ASLE ext area. Allow this even though it is against the spec, so we
+	 * do not end up rejecting the VBT on those boards (and end up not
+	 * finding the LCD panel because of this).
+	 */
+	vbt_size = (mboxes & MBOX_ASLE_EXT) ?
+		OPREGION_ASLE_EXT_OFFSET : OPREGION_SIZE;
+	vbt_size -= OPREGION_VBT_OFFSET;
 	if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
-		DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion (RVDA)\n");
+		DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion (Mailbox #4)\n");
 		opregion->vbt = vbt;
 		opregion->vbt_size = vbt_size;
-	} else {
-		vbt = base + OPREGION_VBT_OFFSET;
-		/*
-		 * The VBT specification says that if the ASLE ext mailbox is
-		 * not used its area is reserved, but on some CHT boards the VBT
-		 * extends into the ASLE ext area. Allow this even though it is
-		 * against the spec, so we do not end up rejecting the VBT on
-		 * those boards (and end up not finding the LCD panel because of
-		 * this).
-		 */
-		vbt_size = (mboxes & MBOX_ASLE_EXT) ?
-			OPREGION_ASLE_EXT_OFFSET : OPREGION_SIZE;
-		vbt_size -= 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;
-		}
 	}
 
 out:
-- 
2.1.4

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

[PATCH RESEND 3/4] drm/i915/opregion: debug log about invalid ACPI OpRegion VBT

[Jani Nikula]

Leave more breadcrumbs for debuggers.

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

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 2b64cb6691eb..d44465190dc1 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -989,6 +989,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
 			opregion->vbt = vbt;
 			opregion->vbt_size = vbt_size;
 			goto out;
+		} else {
+			DRM_DEBUG_KMS("Invalid VBT in ACPI OpRegion (RVDA)\n");
 		}
 	}
 
@@ -1007,6 +1009,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
 		DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion (Mailbox #4)\n");
 		opregion->vbt = vbt;
 		opregion->vbt_size = vbt_size;
+	} else {
+		DRM_DEBUG_KMS("Invalid VBT in ACPI OpRegion (Mailbox #4)\n");
 	}
 
 out:
-- 
2.1.4

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

[PATCH RESEND 4/4] drm/i915/opregion: let user specify override VBT via firmware load

[Jani Nikula]

Sometimes it would be most enlightening to debug systems by replacing
the VBT to be used. For example, in the referenced bug the BIOS provides
different VBT depending on the boot mode (UEFI vs. legacy). It would be
interesting to try the failing boot mode with the VBT from the working
boot, and see if that makes a difference.

Add a module parameter to load the VBT using the firmware loader, not
unlike the EDID firmware mechanism.

As a starting point for experimenting, one can pick up the BIOS provided
VBT from /sys/kernel/debug/dri/0/i915_opregion/i915_vbt.

References: https://bugs.freedesktop.org/show_bug.cgi?id=97822#c83
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_params.c    |  4 ++++
 drivers/gpu/drm/i915/i915_params.h    |  1 +
 drivers/gpu/drm/i915/intel_opregion.c | 40 +++++++++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index b6a7e363d076..6d5d334f50b1 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -115,6 +115,10 @@ MODULE_PARM_DESC(vbt_sdvo_panel_type,
 	"Override/Ignore selection of SDVO panel mode in the VBT "
 	"(-2=ignore, -1=auto [default], index in VBT BIOS table)");
 
+module_param_named_unsafe(vbt_firmware, i915.vbt_firmware, charp, 0400);
+MODULE_PARM_DESC(vbt_firmware,
+		 "Load VBT from specified file under /lib/firmware");
+
 module_param_named_unsafe(reset, i915.reset, bool, 0600);
 MODULE_PARM_DESC(reset, "Attempt GPU resets (default: true)");
 
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 34148cc8637c..0aeb106e06af 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -28,6 +28,7 @@
 #include <linux/cache.h> /* for __read_mostly */
 
 #define I915_PARAMS_FOR_EACH(func) \
+	func(char *, vbt_firmware); \
 	func(int, modeset); \
 	func(int, panel_ignore_lid); \
 	func(int, semaphores); \
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index d44465190dc1..7cbd801516ab 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -27,6 +27,7 @@
 
 #include <linux/acpi.h>
 #include <linux/dmi.h>
+#include <linux/firmware.h>
 #include <acpi/video.h>
 
 #include <drm/drmP.h>
@@ -912,6 +913,42 @@ static const struct dmi_system_id intel_no_opregion_vbt[] = {
 	{ }
 };
 
+static int intel_load_vbt_firmware(struct drm_i915_private *dev_priv)
+{
+	struct intel_opregion *opregion = &dev_priv->opregion;
+	const struct firmware *fw = NULL;
+	const char *name = i915.vbt_firmware;
+	int ret;
+
+	if (!name || !*name)
+		return -ENOENT;
+
+	ret = request_firmware(&fw, name, &dev_priv->drm.pdev->dev);
+	if (ret) {
+		DRM_ERROR("Requesting VBT firmware \"%s\" failed (%d)\n",
+			  name, ret);
+		return ret;
+	}
+
+	if (intel_bios_is_valid_vbt(fw->data, fw->size)) {
+		opregion->vbt = kmemdup(fw->data, fw->size, GFP_KERNEL);
+		if (opregion->vbt) {
+			DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name);
+			opregion->vbt_size = fw->size;
+			ret = 0;
+		} else {
+			ret = -ENOMEM;
+		}
+	} else {
+		DRM_DEBUG_KMS("Invalid VBT firmware \"%s\"\n", name);
+		ret = -EINVAL;
+	}
+
+	release_firmware(fw);
+
+	return ret;
+}
+
 int intel_opregion_setup(struct drm_i915_private *dev_priv)
 {
 	struct intel_opregion *opregion = &dev_priv->opregion;
@@ -974,6 +1011,9 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
 	if (mboxes & MBOX_ASLE_EXT)
 		DRM_DEBUG_DRIVER("ASLE extension supported\n");
 
+	if (!intel_load_vbt_firmware(dev_priv))
+		goto out;
+
 	if (dmi_check_system(intel_no_opregion_vbt))
 		goto out;
 
-- 
2.1.4

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

✓ Fi.CI.BAT: success for series starting with [RESEND,1/4] drm/i915/opregion: bail out early for systems with no opregion VBT

[Patchwork]

== Series Details ==

Series: series starting with [RESEND,1/4] drm/i915/opregion: bail out early for systems with no opregion VBT
URL   : https://patchwork.freedesktop.org/series/22075/
State : success

== Summary ==

Series 22075v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/22075/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                dmesg-warn -> PASS       (fi-snb-2600)
                dmesg-warn -> PASS       (fi-kbl-7560u) fdo#100428

fdo#100428 https://bugs.freedesktop.org/show_bug.cgi?id=100428

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 459s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 452s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 598s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 536s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 579s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 504s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 498s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 434s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 436s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 440s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 512s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 502s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 477s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 594s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 486s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 604s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 488s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 516s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time: 470s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 549s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 422s

1610e07cbf696204259f584fcc74fde1c16a4d29 drm-tip: 2017y-03m-29d-10h-12m-03s UTC integration manifest
748b27b drm/i915/opregion: let user specify override VBT via firmware load
4399a24 drm/i915/opregion: debug log about invalid ACPI OpRegion VBT
22aa1b0 drm/i915/opregion: try to validate RVDA VBT only if it's there
aae9dac drm/i915/opregion: bail out early for systems with no opregion VBT

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4338/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RESEND 1/4] drm/i915/opregion: bail out early for systems with no opregion VBT

[Bob Paauwe]

On Wed, 29 Mar 2017 13:32:55 +0300
Jani Nikula <jani.nikula@intel.com> wrote:

> Reduce indent. No functional changes.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_opregion.c | 64 +++++++++++++++++------------------
>  1 file changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 441c01466384..76a39ee6d005 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -920,6 +920,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>  	char buf[sizeof(OPREGION_SIGNATURE)];
>  	int err = 0;
>  	void *base;
> +	const void *vbt = NULL;
> +	u32 vbt_size = 0;
>  
>  	BUILD_BUG_ON(sizeof(struct opregion_header) != 0x100);
>  	BUILD_BUG_ON(sizeof(struct opregion_acpi) != 0x100);
> @@ -972,45 +974,43 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>  	if (mboxes & MBOX_ASLE_EXT)
>  		DRM_DEBUG_DRIVER("ASLE extension supported\n");
>  
> -	if (!dmi_check_system(intel_no_opregion_vbt)) {
> -		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 (dmi_check_system(intel_no_opregion_vbt))
> +		goto out;
> +
> +	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 (RVDA)\n");
> +		opregion->vbt = vbt;
> +		opregion->vbt_size = vbt_size;
> +	} else {
> +		vbt = base + OPREGION_VBT_OFFSET;
> +		/*
> +		 * The VBT specification says that if the ASLE ext mailbox is
> +		 * not used its area is reserved, but on some CHT boards the VBT
> +		 * extends into the ASLE ext area. Allow this even though it is
> +		 * against the spec, so we do not end up rejecting the VBT on
> +		 * those boards (and end up not finding the LCD panel because of
> +		 * this).
> +		 */
> +		vbt_size = (mboxes & MBOX_ASLE_EXT) ?
> +			OPREGION_ASLE_EXT_OFFSET : OPREGION_SIZE;
> +		vbt_size -= OPREGION_VBT_OFFSET;
>  		if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
> -			DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion (RVDA)\n");
> +			DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion (Mailbox #4)\n");
>  			opregion->vbt = vbt;
>  			opregion->vbt_size = vbt_size;
> -		} else {
> -			vbt = base + OPREGION_VBT_OFFSET;
> -			/*
> -			 * The VBT specification says that if the ASLE ext
> -			 * mailbox is not used its area is reserved, but
> -			 * on some CHT boards the VBT extends into the
> -			 * ASLE ext area. Allow this even though it is
> -			 * against the spec, so we do not end up rejecting
> -			 * the VBT on those boards (and end up not finding the
> -			 * LCD panel because of this).
> -			 */
> -			vbt_size = (mboxes & MBOX_ASLE_EXT) ?
> -				OPREGION_ASLE_EXT_OFFSET : OPREGION_SIZE;
> -			vbt_size -= 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;
> -			}
>  		}
>  	}
>  
> +out:
>  	return 0;
>  
>  err_out:



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH RESEND 2/4] drm/i915/opregion: try to validate RVDA VBT only if it's there

[Bob Paauwe]

On Wed, 29 Mar 2017 13:32:56 +0300
Jani Nikula <jani.nikula@intel.com> wrote:

> Seems more sensible this way, and reduces indent for the more common
> case.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_opregion.c | 41 +++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 76a39ee6d005..2b64cb6691eb 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -920,8 +920,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>  	char buf[sizeof(OPREGION_SIGNATURE)];
>  	int err = 0;
>  	void *base;
> -	const void *vbt = NULL;
> -	u32 vbt_size = 0;
> +	const void *vbt;
> +	u32 vbt_size;
>  
>  	BUILD_BUG_ON(sizeof(struct opregion_header) != 0x100);
>  	BUILD_BUG_ON(sizeof(struct opregion_acpi) != 0x100);
> @@ -984,30 +984,29 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>  					  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 (RVDA)\n");
> +			opregion->vbt = vbt;
> +			opregion->vbt_size = vbt_size;
> +			goto out;
> +		}
>  	}
>  
> +	vbt = base + OPREGION_VBT_OFFSET;
> +	/*
> +	 * The VBT specification says that if the ASLE ext mailbox is not used
> +	 * its area is reserved, but on some CHT boards the VBT extends into the
> +	 * ASLE ext area. Allow this even though it is against the spec, so we
> +	 * do not end up rejecting the VBT on those boards (and end up not
> +	 * finding the LCD panel because of this).
> +	 */
> +	vbt_size = (mboxes & MBOX_ASLE_EXT) ?
> +		OPREGION_ASLE_EXT_OFFSET : OPREGION_SIZE;
> +	vbt_size -= OPREGION_VBT_OFFSET;
>  	if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
> -		DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion (RVDA)\n");
> +		DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion (Mailbox #4)\n");
>  		opregion->vbt = vbt;
>  		opregion->vbt_size = vbt_size;
> -	} else {
> -		vbt = base + OPREGION_VBT_OFFSET;
> -		/*
> -		 * The VBT specification says that if the ASLE ext mailbox is
> -		 * not used its area is reserved, but on some CHT boards the VBT
> -		 * extends into the ASLE ext area. Allow this even though it is
> -		 * against the spec, so we do not end up rejecting the VBT on
> -		 * those boards (and end up not finding the LCD panel because of
> -		 * this).
> -		 */
> -		vbt_size = (mboxes & MBOX_ASLE_EXT) ?
> -			OPREGION_ASLE_EXT_OFFSET : OPREGION_SIZE;
> -		vbt_size -= 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;
> -		}
>  	}
>  
>  out:



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH RESEND 3/4] drm/i915/opregion: debug log about invalid ACPI OpRegion VBT

[Bob Paauwe]

On Wed, 29 Mar 2017 13:32:57 +0300
Jani Nikula <jani.nikula@intel.com> wrote:

> Leave more breadcrumbs for debuggers.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_opregion.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 2b64cb6691eb..d44465190dc1 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -989,6 +989,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>  			opregion->vbt = vbt;
>  			opregion->vbt_size = vbt_size;
>  			goto out;
> +		} else {
> +			DRM_DEBUG_KMS("Invalid VBT in ACPI OpRegion (RVDA)\n");
>  		}
>  	}
>  
> @@ -1007,6 +1009,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>  		DRM_DEBUG_KMS("Found valid VBT in ACPI OpRegion (Mailbox #4)\n");
>  		opregion->vbt = vbt;
>  		opregion->vbt_size = vbt_size;
> +	} else {
> +		DRM_DEBUG_KMS("Invalid VBT in ACPI OpRegion (Mailbox #4)\n");
>  	}
>  
>  out:



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH RESEND 4/4] drm/i915/opregion: let user specify override VBT via firmware load

[Bob Paauwe]

On Wed, 29 Mar 2017 13:32:58 +0300
Jani Nikula <jani.nikula@intel.com> wrote:

> Sometimes it would be most enlightening to debug systems by replacing
> the VBT to be used. For example, in the referenced bug the BIOS provides
> different VBT depending on the boot mode (UEFI vs. legacy). It would be
> interesting to try the failing boot mode with the VBT from the working
> boot, and see if that makes a difference.
> 
> Add a module parameter to load the VBT using the firmware loader, not
> unlike the EDID firmware mechanism.
> 
> As a starting point for experimenting, one can pick up the BIOS provided
> VBT from /sys/kernel/debug/dri/0/i915_opregion/i915_vbt.

Jani,

I really like this idea and believe that it will be useful.  Thanks for
doing this!

> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=97822#c83
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_params.c    |  4 ++++
>  drivers/gpu/drm/i915/i915_params.h    |  1 +
>  drivers/gpu/drm/i915/intel_opregion.c | 40 +++++++++++++++++++++++++++++++++++
>  3 files changed, 45 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index b6a7e363d076..6d5d334f50b1 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -115,6 +115,10 @@ MODULE_PARM_DESC(vbt_sdvo_panel_type,
>  	"Override/Ignore selection of SDVO panel mode in the VBT "
>  	"(-2=ignore, -1=auto [default], index in VBT BIOS table)");
>  
> +module_param_named_unsafe(vbt_firmware, i915.vbt_firmware, charp, 0400);
> +MODULE_PARM_DESC(vbt_firmware,
> +		 "Load VBT from specified file under /lib/firmware");
> +
>  module_param_named_unsafe(reset, i915.reset, bool, 0600);
>  MODULE_PARM_DESC(reset, "Attempt GPU resets (default: true)");
>  
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 34148cc8637c..0aeb106e06af 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -28,6 +28,7 @@
>  #include <linux/cache.h> /* for __read_mostly */
>  
>  #define I915_PARAMS_FOR_EACH(func) \
> +	func(char *, vbt_firmware); \
>  	func(int, modeset); \
>  	func(int, panel_ignore_lid); \
>  	func(int, semaphores); \
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index d44465190dc1..7cbd801516ab 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -27,6 +27,7 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/dmi.h>
> +#include <linux/firmware.h>
>  #include <acpi/video.h>
>  
>  #include <drm/drmP.h>
> @@ -912,6 +913,42 @@ static const struct dmi_system_id intel_no_opregion_vbt[] = {
>  	{ }
>  };
>  
> +static int intel_load_vbt_firmware(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_opregion *opregion = &dev_priv->opregion;
> +	const struct firmware *fw = NULL;
> +	const char *name = i915.vbt_firmware;
> +	int ret;
> +
> +	if (!name || !*name)
> +		return -ENOENT;
> +
> +	ret = request_firmware(&fw, name, &dev_priv->drm.pdev->dev);
> +	if (ret) {
> +		DRM_ERROR("Requesting VBT firmware \"%s\" failed (%d)\n",
> +			  name, ret);
> +		return ret;
> +	}
> +
> +	if (intel_bios_is_valid_vbt(fw->data, fw->size)) {
> +		opregion->vbt = kmemdup(fw->data, fw->size, GFP_KERNEL);
> +		if (opregion->vbt) {
> +			DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name);
> +			opregion->vbt_size = fw->size;
> +			ret = 0;
> +		} else {
> +			ret = -ENOMEM;

Since the errors propagated, it might be a good idea to add a debug
message here too.

> +		}
> +	} else {
> +		DRM_DEBUG_KMS("Invalid VBT firmware \"%s\"\n", name);
> +		ret = -EINVAL;
> +	}
> +
> +	release_firmware(fw);
> +
> +	return ret;
> +}
> +
>  int intel_opregion_setup(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_opregion *opregion = &dev_priv->opregion;
> @@ -974,6 +1011,9 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>  	if (mboxes & MBOX_ASLE_EXT)
>  		DRM_DEBUG_DRIVER("ASLE extension supported\n");
>  
> +	if (!intel_load_vbt_firmware(dev_priv))
> +		goto out;

I find the condition a bit confusing. It reads to me as "if firmware
not loaded, goto out" which is backwards from what it's really doing.
Since you're ignoring the error return value anyway, making 
intel_load_vbt_firmware() a boolean it would make it read better.

However, if you have some future plan to propagate errors, then keep
I'm fine with it all as is.

> +
>  	if (dmi_check_system(intel_no_opregion_vbt))
>  		goto out;
>  



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH RESEND 4/4] drm/i915/opregion: let user specify override VBT via firmware load

[Jani Nikula]

On Wed, 29 Mar 2017, Bob Paauwe <bob.j.paauwe@intel.com> wrote:
> On Wed, 29 Mar 2017 13:32:58 +0300
> Jani Nikula <jani.nikula@intel.com> wrote:
>
>> Sometimes it would be most enlightening to debug systems by replacing
>> the VBT to be used. For example, in the referenced bug the BIOS provides
>> different VBT depending on the boot mode (UEFI vs. legacy). It would be
>> interesting to try the failing boot mode with the VBT from the working
>> boot, and see if that makes a difference.
>> 
>> Add a module parameter to load the VBT using the firmware loader, not
>> unlike the EDID firmware mechanism.
>> 
>> As a starting point for experimenting, one can pick up the BIOS provided
>> VBT from /sys/kernel/debug/dri/0/i915_opregion/i915_vbt.
>
> Jani,
>
> I really like this idea and believe that it will be useful.  Thanks for
> doing this!

Thanks for the review! I pushed patches 1-3 to drm-intel-next-queued.

>> 
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=97822#c83
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_params.c    |  4 ++++
>>  drivers/gpu/drm/i915/i915_params.h    |  1 +
>>  drivers/gpu/drm/i915/intel_opregion.c | 40 +++++++++++++++++++++++++++++++++++
>>  3 files changed, 45 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>> index b6a7e363d076..6d5d334f50b1 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -115,6 +115,10 @@ MODULE_PARM_DESC(vbt_sdvo_panel_type,
>>  	"Override/Ignore selection of SDVO panel mode in the VBT "
>>  	"(-2=ignore, -1=auto [default], index in VBT BIOS table)");
>>  
>> +module_param_named_unsafe(vbt_firmware, i915.vbt_firmware, charp, 0400);
>> +MODULE_PARM_DESC(vbt_firmware,
>> +		 "Load VBT from specified file under /lib/firmware");
>> +
>>  module_param_named_unsafe(reset, i915.reset, bool, 0600);
>>  MODULE_PARM_DESC(reset, "Attempt GPU resets (default: true)");
>>  
>> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
>> index 34148cc8637c..0aeb106e06af 100644
>> --- a/drivers/gpu/drm/i915/i915_params.h
>> +++ b/drivers/gpu/drm/i915/i915_params.h
>> @@ -28,6 +28,7 @@
>>  #include <linux/cache.h> /* for __read_mostly */
>>  
>>  #define I915_PARAMS_FOR_EACH(func) \
>> +	func(char *, vbt_firmware); \
>>  	func(int, modeset); \
>>  	func(int, panel_ignore_lid); \
>>  	func(int, semaphores); \
>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> index d44465190dc1..7cbd801516ab 100644
>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> @@ -27,6 +27,7 @@
>>  
>>  #include <linux/acpi.h>
>>  #include <linux/dmi.h>
>> +#include <linux/firmware.h>
>>  #include <acpi/video.h>
>>  
>>  #include <drm/drmP.h>
>> @@ -912,6 +913,42 @@ static const struct dmi_system_id intel_no_opregion_vbt[] = {
>>  	{ }
>>  };
>>  
>> +static int intel_load_vbt_firmware(struct drm_i915_private *dev_priv)
>> +{
>> +	struct intel_opregion *opregion = &dev_priv->opregion;
>> +	const struct firmware *fw = NULL;
>> +	const char *name = i915.vbt_firmware;
>> +	int ret;
>> +
>> +	if (!name || !*name)
>> +		return -ENOENT;
>> +
>> +	ret = request_firmware(&fw, name, &dev_priv->drm.pdev->dev);
>> +	if (ret) {
>> +		DRM_ERROR("Requesting VBT firmware \"%s\" failed (%d)\n",
>> +			  name, ret);
>> +		return ret;
>> +	}
>> +
>> +	if (intel_bios_is_valid_vbt(fw->data, fw->size)) {
>> +		opregion->vbt = kmemdup(fw->data, fw->size, GFP_KERNEL);
>> +		if (opregion->vbt) {
>> +			DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name);
>> +			opregion->vbt_size = fw->size;
>> +			ret = 0;
>> +		} else {
>> +			ret = -ENOMEM;
>
> Since the errors propagated, it might be a good idea to add a debug
> message here too.

Will add.

>
>> +		}
>> +	} else {
>> +		DRM_DEBUG_KMS("Invalid VBT firmware \"%s\"\n", name);
>> +		ret = -EINVAL;
>> +	}
>> +
>> +	release_firmware(fw);
>> +
>> +	return ret;
>> +}
>> +
>>  int intel_opregion_setup(struct drm_i915_private *dev_priv)
>>  {
>>  	struct intel_opregion *opregion = &dev_priv->opregion;
>> @@ -974,6 +1011,9 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>>  	if (mboxes & MBOX_ASLE_EXT)
>>  		DRM_DEBUG_DRIVER("ASLE extension supported\n");
>>  
>> +	if (!intel_load_vbt_firmware(dev_priv))
>> +		goto out;
>
> I find the condition a bit confusing. It reads to me as "if firmware
> not loaded, goto out" which is backwards from what it's really doing.
> Since you're ignoring the error return value anyway, making 
> intel_load_vbt_firmware() a boolean it would make it read better.
>
> However, if you have some future plan to propagate errors, then keep
> I'm fine with it all as is.

Would this be clearer?

	if (intel_load_vbt_firmware(dev_priv) == 0)
		goto out;

BR,
Jani.

>
>> +
>>  	if (dmi_check_system(intel_no_opregion_vbt))
>>  		goto out;
>>  

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

Re: [PATCH RESEND 4/4] drm/i915/opregion: let user specify override VBT via firmware load

[Bob Paauwe]

On Thu, 30 Mar 2017 09:22:19 +0300
Jani Nikula <jani.nikula@intel.com> wrote:

> On Wed, 29 Mar 2017, Bob Paauwe <bob.j.paauwe@intel.com> wrote:
> > On Wed, 29 Mar 2017 13:32:58 +0300
> > Jani Nikula <jani.nikula@intel.com> wrote:
> >  
> >> Sometimes it would be most enlightening to debug systems by replacing
> >> the VBT to be used. For example, in the referenced bug the BIOS provides
> >> different VBT depending on the boot mode (UEFI vs. legacy). It would be
> >> interesting to try the failing boot mode with the VBT from the working
> >> boot, and see if that makes a difference.
> >> 
> >> Add a module parameter to load the VBT using the firmware loader, not
> >> unlike the EDID firmware mechanism.
> >> 
> >> As a starting point for experimenting, one can pick up the BIOS provided
> >> VBT from /sys/kernel/debug/dri/0/i915_opregion/i915_vbt.  
> >
> > Jani,
> >
> > I really like this idea and believe that it will be useful.  Thanks for
> > doing this!  
> 
> Thanks for the review! I pushed patches 1-3 to drm-intel-next-queued.
> 
> >> 
> >> References: https://bugs.freedesktop.org/show_bug.cgi?id=97822#c83
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_params.c    |  4 ++++
> >>  drivers/gpu/drm/i915/i915_params.h    |  1 +
> >>  drivers/gpu/drm/i915/intel_opregion.c | 40 +++++++++++++++++++++++++++++++++++
> >>  3 files changed, 45 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> >> index b6a7e363d076..6d5d334f50b1 100644
> >> --- a/drivers/gpu/drm/i915/i915_params.c
> >> +++ b/drivers/gpu/drm/i915/i915_params.c
> >> @@ -115,6 +115,10 @@ MODULE_PARM_DESC(vbt_sdvo_panel_type,
> >>  	"Override/Ignore selection of SDVO panel mode in the VBT "
> >>  	"(-2=ignore, -1=auto [default], index in VBT BIOS table)");
> >>  
> >> +module_param_named_unsafe(vbt_firmware, i915.vbt_firmware, charp, 0400);
> >> +MODULE_PARM_DESC(vbt_firmware,
> >> +		 "Load VBT from specified file under /lib/firmware");
> >> +
> >>  module_param_named_unsafe(reset, i915.reset, bool, 0600);
> >>  MODULE_PARM_DESC(reset, "Attempt GPU resets (default: true)");
> >>  
> >> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> >> index 34148cc8637c..0aeb106e06af 100644
> >> --- a/drivers/gpu/drm/i915/i915_params.h
> >> +++ b/drivers/gpu/drm/i915/i915_params.h
> >> @@ -28,6 +28,7 @@
> >>  #include <linux/cache.h> /* for __read_mostly */
> >>  
> >>  #define I915_PARAMS_FOR_EACH(func) \
> >> +	func(char *, vbt_firmware); \
> >>  	func(int, modeset); \
> >>  	func(int, panel_ignore_lid); \
> >>  	func(int, semaphores); \
> >> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> >> index d44465190dc1..7cbd801516ab 100644
> >> --- a/drivers/gpu/drm/i915/intel_opregion.c
> >> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> >> @@ -27,6 +27,7 @@
> >>  
> >>  #include <linux/acpi.h>
> >>  #include <linux/dmi.h>
> >> +#include <linux/firmware.h>
> >>  #include <acpi/video.h>
> >>  
> >>  #include <drm/drmP.h>
> >> @@ -912,6 +913,42 @@ static const struct dmi_system_id intel_no_opregion_vbt[] = {
> >>  	{ }
> >>  };
> >>  
> >> +static int intel_load_vbt_firmware(struct drm_i915_private *dev_priv)
> >> +{
> >> +	struct intel_opregion *opregion = &dev_priv->opregion;
> >> +	const struct firmware *fw = NULL;
> >> +	const char *name = i915.vbt_firmware;
> >> +	int ret;
> >> +
> >> +	if (!name || !*name)
> >> +		return -ENOENT;
> >> +
> >> +	ret = request_firmware(&fw, name, &dev_priv->drm.pdev->dev);
> >> +	if (ret) {
> >> +		DRM_ERROR("Requesting VBT firmware \"%s\" failed (%d)\n",
> >> +			  name, ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	if (intel_bios_is_valid_vbt(fw->data, fw->size)) {
> >> +		opregion->vbt = kmemdup(fw->data, fw->size, GFP_KERNEL);
> >> +		if (opregion->vbt) {
> >> +			DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name);
> >> +			opregion->vbt_size = fw->size;
> >> +			ret = 0;
> >> +		} else {
> >> +			ret = -ENOMEM;  
> >
> > Since the errors propagated, it might be a good idea to add a debug
> > message here too.  
> 
> Will add.
> 
> >  
> >> +		}
> >> +	} else {
> >> +		DRM_DEBUG_KMS("Invalid VBT firmware \"%s\"\n", name);
> >> +		ret = -EINVAL;
> >> +	}
> >> +
> >> +	release_firmware(fw);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >>  int intel_opregion_setup(struct drm_i915_private *dev_priv)
> >>  {
> >>  	struct intel_opregion *opregion = &dev_priv->opregion;
> >> @@ -974,6 +1011,9 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
> >>  	if (mboxes & MBOX_ASLE_EXT)
> >>  		DRM_DEBUG_DRIVER("ASLE extension supported\n");
> >>  
> >> +	if (!intel_load_vbt_firmware(dev_priv))
> >> +		goto out;  
> >
> > I find the condition a bit confusing. It reads to me as "if firmware
> > not loaded, goto out" which is backwards from what it's really doing.
> > Since you're ignoring the error return value anyway, making 
> > intel_load_vbt_firmware() a boolean it would make it read better.
> >
> > However, if you have some future plan to propagate errors, then keep
> > I'm fine with it all as is.  
> 
> Would this be clearer?
> 
> 	if (intel_load_vbt_firmware(dev_priv) == 0)
> 		goto out;
> 

Yes, I think so.  zero as a successful return value is common so I
think this better says that if we successfully load the firmware, we're
done.

With this an the above change

Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

I know this capability would have been helpful to me in the past
where I instead was modifying the vbt parsing code to inject the
changes I needed.


> BR,
> Jani.
> 
> >  
> >> +
> >>  	if (dmi_check_system(intel_no_opregion_vbt))
> >>  		goto out;
> >>    
> 



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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