Re: [Intel-gfx] [PATCH] drm/i915: Sanitycheck PCI BARs on probe

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 20 Jan 2022, "Piorkowski, Piotr" <piotr.piorkowski@intel.com> wrote:
> From: Piotr Piórkowski <piotr.piorkowski@intel.com>
>
> For proper operation of i915 we need usable PCI BARs:
>  - GTTMMADDR BAR 0 (1 for GEN2)
>  - GFXMEM BAR 2.
> Lets check before we start the i915 probe that these BARs are set,
> and that they have a size greater than 0.
>
> Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_pci.c         | 33 +++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_pci_config.h |  5 ++++
>  2 files changed, 38 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index 8261b6455747..ad60c69d9dd8 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -29,6 +29,8 @@
>  #include "i915_drv.h"
>  #include "i915_pci.h"
>  

Superfluous blank line.

> +#include "intel_pci_config.h"

Please put the includes together and sort.

> +
>  #define PLATFORM(x) .platform = (x)
>  #define GEN(x) \
>  	.graphics.ver = (x), \
> @@ -1181,6 +1183,34 @@ static bool force_probe(u16 device_id, const char *devices)
>  	return ret;
>  }
>  
> +static bool __pci_resource_valid(struct pci_dev *pdev, int bar)
> +{
> +	if (!pci_resource_flags(pdev, bar))
> +		return false;
> +
> +	if (pci_resource_flags(pdev, bar) & IORESOURCE_UNSET)
> +		return false;
> +
> +	if (!pci_resource_len(pdev, bar))
> +		return false;
> +
> +	return true;
> +}
> +
> +static bool intel_bars_valid(struct pci_dev *pdev, struct intel_device_info *intel_info)
> +{
> +	const int gttmmaddr_bar = intel_info->graphics.ver == 2 ? GEN2_GTTMMADR_BAR : GTTMMADR_BAR;
> +	const int gfxmem_bar = GFXMEM_BAR;

We don't usually bother with const for non-pointer local variables.

> +
> +	if (!__pci_resource_valid(pdev, gttmmaddr_bar))
> +		return false;
> +
> +	if (!__pci_resource_valid(pdev, gfxmem_bar))
> +		return false;
> +
> +	return true;
> +}
> +
>  static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
>  	struct intel_device_info *intel_info =
> @@ -1206,6 +1236,9 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (PCI_FUNC(pdev->devfn))
>  		return -ENODEV;
>  
> +	if (!intel_bars_valid(pdev, intel_info))
> +		return -ENODEV;
> +
>  	/* Detect if we need to wait for other drivers early on */
>  	if (intel_modeset_probe_defer(pdev))
>  		return -EPROBE_DEFER;
> diff --git a/drivers/gpu/drm/i915/intel_pci_config.h b/drivers/gpu/drm/i915/intel_pci_config.h
> index 12cd9d4f23de..c08fd5d48ada 100644
> --- a/drivers/gpu/drm/i915/intel_pci_config.h
> +++ b/drivers/gpu/drm/i915/intel_pci_config.h
> @@ -6,6 +6,11 @@
>  #ifndef __INTEL_PCI_CONFIG_H__
>  #define __INTEL_PCI_CONFIG_H__
>  
> +/* PCI BARs */
> +#define GTTMMADR_BAR				0
> +#define GEN2_GTTMMADR_BAR			1
> +#define GFXMEM_BAR				2

Is anyone outside of intel_pci_config.c going to need these? They could
be there if not.

BR,
Jani.


> +
>  /* BSM in include/drm/i915_drm.h */
>  
>  #define MCHBAR_I915				0x44

-- 
Jani Nikula, Intel Open Source Graphics Center