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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux