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

[Jani Nikula <jani.nikula@intel.com>]

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