On Wed, 29 Mar 2017, Bob Paauwe <bob.j.paauwe@xxxxxxxxx> wrote: > On Wed, 29 Mar 2017 13:32:58 +0300 > Jani Nikula <jani.nikula@xxxxxxxxx> 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@xxxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
