diff mbox series

drm/i915: Sanitycheck PCI BARs on probe

Message ID 20220120113049.213361-1-piotr.piorkowski@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Sanitycheck PCI BARs on probe | expand

Commit Message

Piotr Piórkowski Jan. 20, 2022, 11:30 a.m. UTC 
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(+)

Comments

Lucas De Marchi May 19, 2022, 10:29 p.m. UTC | #1 
On Thu, Jan 20, 2022 at 12:30:49PM +0100, Piorkowski, Piotr 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>

This sounds reasonable to me... should catch issues in which the BIOS
didn't assign resources properly, PCI subsystem tried to reassign them
and we ended up left withou a BAR.

+Matt Auld who is working on small BAR recovery... slightly related.
Does this look ok?

Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks
Lucas De Marchi


>---
> 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"
>
>+#include "intel_pci_config.h"
>+
> #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;
>+
>+	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
>+
> /* BSM in include/drm/i915_drm.h */
>
> #define MCHBAR_I915				0x44
>-- 
>2.25.1
>
Jani Nikula May 20, 2022, 7:40 a.m. UTC | #2 
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
Piotr Piórkowski May 20, 2022, 8:07 a.m. UTC | #3 
Jani Nikula <jani.nikula@linux.intel.com> wrote on pią [2022-maj-20 10:40:01 +0300]:
> 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.
> 

ok

> > +
> >  #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.

ok
> 
> > +
> > +	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.
> 
We could use this in all i915. There are lots of places where we use BAR numbers
instead of constants when operating on pci resources.
E.g. all intel_pci_resource calls, or directs calls pci_resource_start
and pci_resource_len.
For now, we use two ( and an exception for gen2) BARs in i915,
but there may be more in the future.
It may be useful to organize this.

Thanks for review!
Piotr

> BR,
> Jani.
> 
> 
> > +
> >  /* BSM in include/drm/i915_drm.h */
> >  
> >  #define MCHBAR_I915				0x44
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

--
diff mbox series

Patch

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"
 
+#include "intel_pci_config.h"
+
 #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;
+
+	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
+
 /* BSM in include/drm/i915_drm.h */
 
 #define MCHBAR_I915				0x44