Re: [PATCH] drm/i915: add force_probe module parameter to replace alpha_support

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, May 06, 2019 at 04:48:01PM +0300, Jani Nikula wrote:
> The i915.alpha_support module parameter has caused some confusion along
> the way. Add new i915.force_probe parameter to specify PCI IDs of
> devices to probe, when the devices are recognized but not automatically
> probed by the driver. The name is intended to reflect what the parameter
> effectively does, avoiding any overloaded semantics of "alpha" and
> "support".
> 
> The parameter supports "" to disable, "<pci-id>,[<pci-id>,...]" to
> enable force probe for one or more devices, and "*" to enable force
> probe for all known devices.
> 
> Also add new CONFIG_DRM_I915_FORCE_PROBE config option to replace the
> DRM_I915_ALPHA_SUPPORT option. This defaults to "*" if
> DRM_I915_ALPHA_SUPPORT=y.
> 
> Instead of replacing i915.alpha_support immediately, let the two coexist
> for a while, with a deprecation message, for a transition period.
> 
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

First of all I'm sorry for the delay. I could swear that I had
reviewed it already.

> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/Kconfig             | 29 +++++++++-----
>  drivers/gpu/drm/i915/i915_drv.h          |  2 -
>  drivers/gpu/drm/i915/i915_params.c       |  7 +++-
>  drivers/gpu/drm/i915/i915_params.h       |  1 +
>  drivers/gpu/drm/i915/i915_pci.c          | 51 +++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_device_info.h |  2 +-
>  6 files changed, 72 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index f05563..e7b617 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -45,19 +45,28 @@ config DRM_I915
>  config DRM_I915_ALPHA_SUPPORT
>  	bool "Enable alpha quality support for new Intel hardware by default"
>  	depends on DRM_I915
> -	default n
>  	help
> -	  Choose this option if you have new Intel hardware and want to enable
> -	  the alpha quality i915 driver support for the hardware in this kernel
> -	  version. You can also enable the support at runtime using the module
> -	  parameter i915.alpha_support=1; this option changes the default for
> -	  that module parameter.
> +	  This option is deprecated. Use DRM_I915_FORCE_PROBE option instead.
>  
> -	  It is recommended to upgrade to a kernel version with proper support
> -	  as soon as it is available. Generally fixes for platforms with alpha
> -	  support are not backported to older kernels.
> +config DRM_I915_FORCE_PROBE
> +	string "Force probe driver for selected new Intel hardware"
> +	depends on DRM_I915
> +	default "*" if DRM_I915_ALPHA_SUPPORT
> +	help
> +	  This is the default value for the i915.force_probe module
> +	  parameter. Using the module parameter overrides this option.
>  
> -	  If in doubt, say "N".
> +	  Force probe the driver for new Intel graphics devices that are
> +	  recognized but not properly supported by this kernel version. It is
> +	  recommended to upgrade to a kernel version with proper support as soon
> +	  as it is available.
> +
> +	  Use "" to disable force probe. If in doubt, use this.
> +
> +	  Use "<pci-id>[,<pci-id>,...]" to force probe the driver for listed

This is kind of confusing.... "[]" suggest optional, but based on the line above
"" is also allowed.

> +	  devices. For example, "4500" or "4500,4571".

But it is good that we have an example here so I won't worry much and it
is up to you on how to proceed.

> +
> +	  Use "*" to force probe the driver for all known devices.
>  
>  config DRM_I915_CAPTURE_ERROR
>  	bool "Enable capturing GPU state following a hang"
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 64fa35..04415d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2435,8 +2435,6 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
>  #define IS_ICL_WITH_PORT_F(dev_priv) \
>  	IS_SUBPLATFORM(dev_priv, INTEL_ICELAKE, INTEL_SUBPLATFORM_PORTF)
>  
> -#define IS_ALPHA_SUPPORT(intel_info) ((intel_info)->is_alpha_support)

We need to remove the usage of this on i915_pci.c...

But I'm wondering how just kbuild bot got that and not CI?

With this fixed,

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> -
>  #define SKL_REVID_A0		0x0
>  #define SKL_REVID_B0		0x1
>  #define SKL_REVID_C0		0x2
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index b5be0a..5b0776 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -87,9 +87,12 @@ i915_param_named_unsafe(enable_psr, int, 0600,
>  	"(0=disabled, 1=enabled) "
>  	"Default: -1 (use per-chip default)");
>  
> +i915_param_named_unsafe(force_probe, charp, 0400,
> +	"Force probe the driver for specified devices. "
> +	"See CONFIG_DRM_I915_FORCE_PROBE for details.");
> +
>  i915_param_named_unsafe(alpha_support, bool, 0400,
> -	"Enable alpha quality driver support for latest hardware. "
> -	"See also CONFIG_DRM_I915_ALPHA_SUPPORT.");
> +	"Deprecated. See i915.force_probe.");
>  
>  i915_param_named_unsafe(disable_power_well, int, 0400,
>  	"Disable display power wells when possible "
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 3f14e9..a2bacd0 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -64,6 +64,7 @@ struct drm_printer;
>  	param(int, reset, 2) \
>  	param(unsigned int, inject_load_failure, 0) \
>  	param(int, fastboot, -1) \
> +	param(char *, force_probe, CONFIG_DRM_I915_FORCE_PROBE) \
>  	/* leave bools at the end to not create holes */ \
>  	param(bool, alpha_support, IS_ENABLED(CONFIG_DRM_I915_ALPHA_SUPPORT)) \
>  	param(bool, enable_hangcheck, true) \
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index ffa2ee..892f2ac 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -761,7 +761,7 @@ static const struct intel_device_info intel_icelake_11_info = {
>  static const struct intel_device_info intel_elkhartlake_info = {
>  	GEN11_FEATURES,
>  	PLATFORM(INTEL_ELKHARTLAKE),
> -	.is_alpha_support = 1,
> +	.require_force_probe = 1,
>  	.engine_mask = BIT(RCS0) | BIT(BCS0) | BIT(VCS0),
>  	.ppgtt_size = 36,
>  };
> @@ -855,16 +855,57 @@ static void i915_pci_remove(struct pci_dev *pdev)
>  	pci_set_drvdata(pdev, NULL);
>  }
>  
> +/* is device_id present in comma separated list of ids */
> +static bool force_probe(u16 device_id, const char *devices)
> +{
> +	char *s, *p, *tok;
> +	bool ret;
> +
> +	/* FIXME: transitional */
> +	if (i915_modparams.alpha_support) {
> +		DRM_INFO("i915.alpha_support is deprecated, use i915.force_probe=%04x instead\n",
> +			 device_id);
> +		return true;
> +	}
> +
> +	if (!devices || !*devices)
> +		return false;
> +
> +	/* match everything */
> +	if (strcmp(devices, "*") == 0)
> +		return true;
> +
> +	s = kstrdup(devices, GFP_KERNEL);
> +	if (!s)
> +		return false;
> +
> +	for (p = s, ret = false; (tok = strsep(&p, ",")) != NULL; ) {
> +		u16 val;
> +
> +		if (kstrtou16(tok, 16, &val) == 0 && val == device_id) {
> +			ret = true;
> +			break;
> +		}
> +	}
> +
> +	kfree(s);
> +
> +	return ret;
> +}
> +
>  static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
>  	struct intel_device_info *intel_info =
>  		(struct intel_device_info *) ent->driver_data;
>  	int err;
>  
> -	if (IS_ALPHA_SUPPORT(intel_info) && !i915_modparams.alpha_support) {
> -		DRM_INFO("The driver support for your hardware in this kernel version is alpha quality\n"
> -			 "See CONFIG_DRM_I915_ALPHA_SUPPORT or i915.alpha_support module parameter\n"
> -			 "to enable support in this kernel version, or check for kernel updates.\n");
> +	if (intel_info->require_force_probe &&
> +	    !force_probe(pdev->device, i915_modparams.force_probe)) {
> +		DRM_INFO("Your graphics device %04x is not properly supported by the driver in this\n"
> +			 "kernel version. To force driver probe anyway, use i915.force_probe=%04x\n"
> +			 "module parameter or CONFIG_DRM_I915_FORCE_PROBE=%04x configuration option,\n"
> +			 "or (recommended) check for kernel updates.\n",
> +			 pdev->device, pdev->device, pdev->device);
>  		return -ENODEV;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> index 5a2e17..0187d6 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -105,7 +105,7 @@ enum intel_ppgtt_type {
>  #define DEV_INFO_FOR_EACH_FLAG(func) \
>  	func(is_mobile); \
>  	func(is_lp); \
> -	func(is_alpha_support); \
> +	func(require_force_probe); \
>  	/* Keep has_* in alphabetical order */ \
>  	func(has_64bit_reloc); \
>  	func(gpu_reset_clobbers_display); \
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx