Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote on pią [2022-maj-20 10:40:01 +0300]:
> On Thu, 20 Jan 2022, "Piorkowski, Piotr" <piotr.piorkowski@xxxxxxxxx> wrote:
> > From: Piotr Piórkowski <piotr.piorkowski@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> > ---
> > 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
--
