On Thu, 30 Mar 2017 09:22:19 +0300 Jani Nikula <jani.nikula@xxxxxxxxx> wrote: > 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; > 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@xxxxxxxxx> 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@xxxxxxxxx IOTG / PED Software Organization Intel Corp. Folsom, CA (916) 356-6193 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
