Re: [Intel-gfx] [PATCH i-g-t 1/2] drm/i915: Add support for LMEM PCIe resizable bar
On Thu, 16 Jun 2022 at 15:55, Jani Nikula wrote:
>
> On Thu, 16 Jun 2022, priyanka.dandam...@intel.com wrote:
> > From: Akeem G Abodunrin
> >
> > This patch adds support for the local memory PICe resizable bar, so that
> > local memory can be resized to the maximum size supported by the device,
> > and mapped correctly to the PCIe memory bar. It is usual that GPU
> > devices expose only 256MB BARs primarily to be compatible with 32-bit
> > systems. So, those devices cannot claim larger memory BAR windows size due
> > to the system BIOS limitation. With this change, it would be possible to
> > reprogram the windows of the bridge directly above the requesting device
> > on the same BAR type.
> >
> > Signed-off-by: Akeem G Abodunrin
> > Signed-off-by: Michał Winiarski
> > Cc: Stuart Summers
> > Cc: Michael J Ruhl
> > Cc: Prathap Kumar Valsan
> > Signed-off-by: Priyanka Dandamudi
> > Reviewed-by: Matthew Auld
> > ---
> > drivers/gpu/drm/i915/i915_driver.c | 92 ++
> > 1 file changed, 92 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c
> > b/drivers/gpu/drm/i915/i915_driver.c
> > index d26dcca7e654..4bdb471cb2e2 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -303,6 +303,95 @@ static void sanitize_gpu(struct drm_i915_private *i915)
> > __intel_gt_reset(to_gt(i915), ALL_ENGINES);
> > }
> >
> > +static void __release_bars(struct pci_dev *pdev)
>
> What's with the double underscores?
>
> > +{
> > + int resno;
> > +
> > + for (resno = PCI_STD_RESOURCES; resno < PCI_STD_RESOURCE_END;
> > resno++) {
> > + if (pci_resource_len(pdev, resno))
> > + pci_release_resource(pdev, resno);
> > + }
> > +}
> > +
> > +static void
> > +__resize_bar(struct drm_i915_private *i915, int resno, resource_size_t
> > size)
> > +{
> > + struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> > + int bar_size = pci_rebar_bytes_to_size(size);
> > + int ret;
> > +
> > + __release_bars(pdev);
> > +
> > + ret = pci_resize_resource(pdev, resno, bar_size);
> > + if (ret) {
> > + drm_info(>drm, "Failed to resize BAR%d to %dM (%pe)\n",
> > + resno, 1 << bar_size, ERR_PTR(ret));
> > + return;
> > + }
> > +
> > + drm_info(>drm, "BAR%d resized to %dM\n", resno, 1 << bar_size);
> > +}
> > +
> > +/* BAR size starts from 1MB - 2^20 */
> > +#define BAR_SIZE_SHIFT 20
> > +static resource_size_t
> > +__lmem_rebar_size(struct drm_i915_private *i915, int resno)
> > +{
> > + struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> > + u32 rebar = pci_rebar_get_possible_sizes(pdev, resno);
> > + resource_size_t size;
> > +
> > + if (!rebar)
> > + return 0;
> > +
> > + size = 1ULL << (__fls(rebar) + BAR_SIZE_SHIFT);
> > +
> > + if (size <= pci_resource_len(pdev, resno))
> > + return 0;
> > +
> > + return size;
> > +}
> > +
> > +#define LMEM_BAR_NUM 2
> > +static void i915_resize_lmem_bar(struct drm_i915_private *i915)
> > +{
> > + struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> > + struct pci_bus *root = pdev->bus;
> > + struct resource *root_res;
> > + resource_size_t rebar_size = __lmem_rebar_size(i915, LMEM_BAR_NUM);
> > + u32 pci_cmd;
> > + int i;
> > +
> > + if (!rebar_size)
> > + return;
> > +
> > + /* Find out if root bus contains 64bit memory addressing */
> > + while (root->parent)
> > + root = root->parent;
> > +
> > + pci_bus_for_each_resource(root, root_res, i) {
> > + if (root_res && root_res->flags & (IORESOURCE_MEM |
> > + IORESOURCE_MEM_64) && root_res->start
> > > 0x1ull)
> > + break;
> > + }
> > +
> > + /* pci_resize_resource will fail anyways */
> > + if (!root_res) {
> > + drm_info(>drm, "Can't resize LMEM BAR - platform
> > support is missing\n");
> > + return;
> > + }
> > +
> > + /* First disable PCI memory decoding references */
> > + pci_read_config_dword(pdev, PCI_COMMAND, _cmd);
> > + pci_write_config_dword(pdev, PCI_COMMAND,
> > +pci_cmd & ~PCI_COMMAND_MEMORY);
> > +
> > + __resize_bar(i915, LMEM_BAR_NUM, rebar_size);
> > +
> > + pci_assign_unassigned_bus_resources(pdev->bus);
> > + pci_write_config_dword(pdev, PCI_COMMAND, pci_cmd);
> > +}
>
> Doesn't feel like the above code belongs in this file. The file is
> supposed to be very high level. The mchbar stuff is the only low level
> thing here, and that feels out of place too. Maybe this and the mchbar
> stuff belong in a new file.
Not sure about mchbar, but maybe i915_resize_lmem_bar() could be moved
into gt/intel_region_lmem.[ch]? That's at least where the consumer of
lmem-bar lives.
>
> BR,
> Jani.
>
>
> > +
> > /**
> > * i915_driver_early_probe -
Re: [Intel-gfx] [PATCH i-g-t 1/2] drm/i915: Add support for LMEM PCIe resizable bar
Hi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-tip/drm-tip]
url:
https://github.com/intel-lab-lkp/linux/commits/priyanka-dandamudi-intel-com/Add-support-for-LMEM-PCIe-resizable-bar/20220616-201631
base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: i386-randconfig-a013
(https://download.01.org/0day-ci/archive/20220616/202206162313.aymhl5br-...@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project
f0e608de27b3d56846eebf3712ab542979d6)
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
#
https://github.com/intel-lab-lkp/linux/commit/0242b37c1e2e73134035a0847c34367331f16cca
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review
priyanka-dandamudi-intel-com/Add-support-for-LMEM-PCIe-resizable-bar/20220616-201631
git checkout 0242b37c1e2e73134035a0847c34367331f16cca
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1
O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/i915/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/i915/i915_driver.c:374:44: warning: result of comparison of
>> constant 4294967296 with expression of type 'resource_size_t' (aka 'unsigned
>> int') is always false [-Wtautological-constant-out-of-range-compare]
IORESOURCE_MEM_64) &&
root_res->start > 0x1ull)
~~~ ^ ~~
1 warning generated.
vim +374 drivers/gpu/drm/i915/i915_driver.c
354
355 #define LMEM_BAR_NUM 2
356 static void i915_resize_lmem_bar(struct drm_i915_private *i915)
357 {
358 struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
359 struct pci_bus *root = pdev->bus;
360 struct resource *root_res;
361 resource_size_t rebar_size = __lmem_rebar_size(i915,
LMEM_BAR_NUM);
362 u32 pci_cmd;
363 int i;
364
365 if (!rebar_size)
366 return;
367
368 /* Find out if root bus contains 64bit memory addressing */
369 while (root->parent)
370 root = root->parent;
371
372 pci_bus_for_each_resource(root, root_res, i) {
373 if (root_res && root_res->flags & (IORESOURCE_MEM |
> 374 IORESOURCE_MEM_64) &&
> root_res->start > 0x1ull)
375 break;
376 }
377
378 /* pci_resize_resource will fail anyways */
379 if (!root_res) {
380 drm_info(>drm, "Can't resize LMEM BAR - platform
support is missing\n");
381 return;
382 }
383
384 /* First disable PCI memory decoding references */
385 pci_read_config_dword(pdev, PCI_COMMAND, _cmd);
386 pci_write_config_dword(pdev, PCI_COMMAND,
387 pci_cmd & ~PCI_COMMAND_MEMORY);
388
389 __resize_bar(i915, LMEM_BAR_NUM, rebar_size);
390
391 pci_assign_unassigned_bus_resources(pdev->bus);
392 pci_write_config_dword(pdev, PCI_COMMAND, pci_cmd);
393 }
394
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Re: [Intel-gfx] [PATCH i-g-t 1/2] drm/i915: Add support for LMEM PCIe resizable bar
On Thu, 16 Jun 2022, priyanka.dandam...@intel.com wrote:
> From: Akeem G Abodunrin
>
> This patch adds support for the local memory PICe resizable bar, so that
Please use imperative. "Add support ..."
Please don't refer to "this patch".
Please fix your git settings to not prefix with "i-g-t" when sending
i915 changes.
BR,
Jani.
> local memory can be resized to the maximum size supported by the device,
> and mapped correctly to the PCIe memory bar. It is usual that GPU
> devices expose only 256MB BARs primarily to be compatible with 32-bit
> systems. So, those devices cannot claim larger memory BAR windows size due
> to the system BIOS limitation. With this change, it would be possible to
> reprogram the windows of the bridge directly above the requesting device
> on the same BAR type.
>
> Signed-off-by: Akeem G Abodunrin
> Signed-off-by: Michał Winiarski
> Cc: Stuart Summers
> Cc: Michael J Ruhl
> Cc: Prathap Kumar Valsan
> Signed-off-by: Priyanka Dandamudi
> Reviewed-by: Matthew Auld
> ---
> drivers/gpu/drm/i915/i915_driver.c | 92 ++
> 1 file changed, 92 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_driver.c
> b/drivers/gpu/drm/i915/i915_driver.c
> index d26dcca7e654..4bdb471cb2e2 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -303,6 +303,95 @@ static void sanitize_gpu(struct drm_i915_private *i915)
> __intel_gt_reset(to_gt(i915), ALL_ENGINES);
> }
>
> +static void __release_bars(struct pci_dev *pdev)
> +{
> + int resno;
> +
> + for (resno = PCI_STD_RESOURCES; resno < PCI_STD_RESOURCE_END; resno++) {
> + if (pci_resource_len(pdev, resno))
> + pci_release_resource(pdev, resno);
> + }
> +}
> +
> +static void
> +__resize_bar(struct drm_i915_private *i915, int resno, resource_size_t size)
> +{
> + struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> + int bar_size = pci_rebar_bytes_to_size(size);
> + int ret;
> +
> + __release_bars(pdev);
> +
> + ret = pci_resize_resource(pdev, resno, bar_size);
> + if (ret) {
> + drm_info(>drm, "Failed to resize BAR%d to %dM (%pe)\n",
> + resno, 1 << bar_size, ERR_PTR(ret));
> + return;
> + }
> +
> + drm_info(>drm, "BAR%d resized to %dM\n", resno, 1 << bar_size);
> +}
> +
> +/* BAR size starts from 1MB - 2^20 */
> +#define BAR_SIZE_SHIFT 20
> +static resource_size_t
> +__lmem_rebar_size(struct drm_i915_private *i915, int resno)
> +{
> + struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> + u32 rebar = pci_rebar_get_possible_sizes(pdev, resno);
> + resource_size_t size;
> +
> + if (!rebar)
> + return 0;
> +
> + size = 1ULL << (__fls(rebar) + BAR_SIZE_SHIFT);
> +
> + if (size <= pci_resource_len(pdev, resno))
> + return 0;
> +
> + return size;
> +}
> +
> +#define LMEM_BAR_NUM 2
> +static void i915_resize_lmem_bar(struct drm_i915_private *i915)
> +{
> + struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> + struct pci_bus *root = pdev->bus;
> + struct resource *root_res;
> + resource_size_t rebar_size = __lmem_rebar_size(i915, LMEM_BAR_NUM);
> + u32 pci_cmd;
> + int i;
> +
> + if (!rebar_size)
> + return;
> +
> + /* Find out if root bus contains 64bit memory addressing */
> + while (root->parent)
> + root = root->parent;
> +
> + pci_bus_for_each_resource(root, root_res, i) {
> + if (root_res && root_res->flags & (IORESOURCE_MEM |
> + IORESOURCE_MEM_64) && root_res->start >
> 0x1ull)
> + break;
> + }
> +
> + /* pci_resize_resource will fail anyways */
> + if (!root_res) {
> + drm_info(>drm, "Can't resize LMEM BAR - platform support
> is missing\n");
> + return;
> + }
> +
> + /* First disable PCI memory decoding references */
> + pci_read_config_dword(pdev, PCI_COMMAND, _cmd);
> + pci_write_config_dword(pdev, PCI_COMMAND,
> +pci_cmd & ~PCI_COMMAND_MEMORY);
> +
> + __resize_bar(i915, LMEM_BAR_NUM, rebar_size);
> +
> + pci_assign_unassigned_bus_resources(pdev->bus);
> + pci_write_config_dword(pdev, PCI_COMMAND, pci_cmd);
> +}
> +
> /**
> * i915_driver_early_probe - setup state not requiring device access
> * @dev_priv: device private
> @@ -852,6 +941,9 @@ int i915_driver_probe(struct pci_dev *pdev, const struct
> pci_device_id *ent)
>
> disable_rpm_wakeref_asserts(>runtime_pm);
>
> + if (HAS_LMEM(i915))
> + i915_resize_lmem_bar(i915);
> +
> intel_vgpu_detect(i915);
>
> ret = intel_gt_probe_all(i915);
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH i-g-t 1/2] drm/i915: Add support for LMEM PCIe resizable bar
On Thu, 16 Jun 2022, priyanka.dandam...@intel.com wrote:
> From: Akeem G Abodunrin
>
> This patch adds support for the local memory PICe resizable bar, so that
> local memory can be resized to the maximum size supported by the device,
> and mapped correctly to the PCIe memory bar. It is usual that GPU
> devices expose only 256MB BARs primarily to be compatible with 32-bit
> systems. So, those devices cannot claim larger memory BAR windows size due
> to the system BIOS limitation. With this change, it would be possible to
> reprogram the windows of the bridge directly above the requesting device
> on the same BAR type.
>
> Signed-off-by: Akeem G Abodunrin
> Signed-off-by: Michał Winiarski
> Cc: Stuart Summers
> Cc: Michael J Ruhl
> Cc: Prathap Kumar Valsan
> Signed-off-by: Priyanka Dandamudi
> Reviewed-by: Matthew Auld
> ---
> drivers/gpu/drm/i915/i915_driver.c | 92 ++
> 1 file changed, 92 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_driver.c
> b/drivers/gpu/drm/i915/i915_driver.c
> index d26dcca7e654..4bdb471cb2e2 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -303,6 +303,95 @@ static void sanitize_gpu(struct drm_i915_private *i915)
> __intel_gt_reset(to_gt(i915), ALL_ENGINES);
> }
>
> +static void __release_bars(struct pci_dev *pdev)
What's with the double underscores?
> +{
> + int resno;
> +
> + for (resno = PCI_STD_RESOURCES; resno < PCI_STD_RESOURCE_END; resno++) {
> + if (pci_resource_len(pdev, resno))
> + pci_release_resource(pdev, resno);
> + }
> +}
> +
> +static void
> +__resize_bar(struct drm_i915_private *i915, int resno, resource_size_t size)
> +{
> + struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> + int bar_size = pci_rebar_bytes_to_size(size);
> + int ret;
> +
> + __release_bars(pdev);
> +
> + ret = pci_resize_resource(pdev, resno, bar_size);
> + if (ret) {
> + drm_info(>drm, "Failed to resize BAR%d to %dM (%pe)\n",
> + resno, 1 << bar_size, ERR_PTR(ret));
> + return;
> + }
> +
> + drm_info(>drm, "BAR%d resized to %dM\n", resno, 1 << bar_size);
> +}
> +
> +/* BAR size starts from 1MB - 2^20 */
> +#define BAR_SIZE_SHIFT 20
> +static resource_size_t
> +__lmem_rebar_size(struct drm_i915_private *i915, int resno)
> +{
> + struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> + u32 rebar = pci_rebar_get_possible_sizes(pdev, resno);
> + resource_size_t size;
> +
> + if (!rebar)
> + return 0;
> +
> + size = 1ULL << (__fls(rebar) + BAR_SIZE_SHIFT);
> +
> + if (size <= pci_resource_len(pdev, resno))
> + return 0;
> +
> + return size;
> +}
> +
> +#define LMEM_BAR_NUM 2
> +static void i915_resize_lmem_bar(struct drm_i915_private *i915)
> +{
> + struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> + struct pci_bus *root = pdev->bus;
> + struct resource *root_res;
> + resource_size_t rebar_size = __lmem_rebar_size(i915, LMEM_BAR_NUM);
> + u32 pci_cmd;
> + int i;
> +
> + if (!rebar_size)
> + return;
> +
> + /* Find out if root bus contains 64bit memory addressing */
> + while (root->parent)
> + root = root->parent;
> +
> + pci_bus_for_each_resource(root, root_res, i) {
> + if (root_res && root_res->flags & (IORESOURCE_MEM |
> + IORESOURCE_MEM_64) && root_res->start >
> 0x1ull)
> + break;
> + }
> +
> + /* pci_resize_resource will fail anyways */
> + if (!root_res) {
> + drm_info(>drm, "Can't resize LMEM BAR - platform support
> is missing\n");
> + return;
> + }
> +
> + /* First disable PCI memory decoding references */
> + pci_read_config_dword(pdev, PCI_COMMAND, _cmd);
> + pci_write_config_dword(pdev, PCI_COMMAND,
> +pci_cmd & ~PCI_COMMAND_MEMORY);
> +
> + __resize_bar(i915, LMEM_BAR_NUM, rebar_size);
> +
> + pci_assign_unassigned_bus_resources(pdev->bus);
> + pci_write_config_dword(pdev, PCI_COMMAND, pci_cmd);
> +}
Doesn't feel like the above code belongs in this file. The file is
supposed to be very high level. The mchbar stuff is the only low level
thing here, and that feels out of place too. Maybe this and the mchbar
stuff belong in a new file.
BR,
Jani.
> +
> /**
> * i915_driver_early_probe - setup state not requiring device access
> * @dev_priv: device private
> @@ -852,6 +941,9 @@ int i915_driver_probe(struct pci_dev *pdev, const struct
> pci_device_id *ent)
>
> disable_rpm_wakeref_asserts(>runtime_pm);
>
> + if (HAS_LMEM(i915))
> + i915_resize_lmem_bar(i915);
> +
> intel_vgpu_detect(i915);
>
> ret = intel_gt_probe_all(i915);
--
Jani Nikula, Intel Open Source Graphics Center
