[Intel-gfx] [PATCH RESEND 4/4] drm/i915/opregion: let user specify override VBT via firmware load
Bob Paauwe
bob.j.paauwe at intel.com
Thu Mar 30 17:31:13 UTC 2017
On Thu, 30 Mar 2017 09:22:19 +0300
Jani Nikula <jani.nikula at intel.com> wrote:
> On Wed, 29 Mar 2017, Bob Paauwe <bob.j.paauwe at intel.com> wrote:
> > 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!
>
> 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 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.
>
> 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 at 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 at intel.com
IOTG / PED Software Organization
Intel Corp. Folsom, CA
(916) 356-6193
More information about the Intel-gfx
mailing list