[Intel-gfx] [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 at 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 at 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 at intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193