Re: [Intel-xe] [PATCH 1/2] drm/xe: Keep all resize bar related prints inside xe_resize_vram_bar

[Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>]

On 25.04.2023 11:25, Matthew Auld wrote:
> On Tue, 25 Apr 2023 at 10:28, Balasubramani Vivekanandan
> <balasubramani.vivekanandan@intel.com> wrote:
> >
> > On 21.04.2023 17:31, Matthew Auld wrote:
> > > On Fri, 21 Apr 2023 at 16:39, Balasubramani Vivekanandan
> > > <balasubramani.vivekanandan@intel.com> wrote:
> > > >
> > > > xe_resize_vram_bar() function is already printing the status of bar
> > > > resizing. It has prints covering both success and failure.
> > > > There is no need of additional prints in the caller which were not so
> > > > easily to follow.
> > > >
> > > > Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/xe/xe_mmio.c | 15 +++++----------
> > > >  1 file changed, 5 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
> > > > index 98357c1f109f..8bd39c3e65ae 100644
> > > > --- a/drivers/gpu/drm/xe/xe_mmio.c
> > > > +++ b/drivers/gpu/drm/xe/xe_mmio.c
> > > > @@ -107,6 +107,8 @@ static int xe_resize_vram_bar(struct xe_device *xe, resource_size_t vram_size)
> > > >                         return 0;
> > > >         }
> > > >
> > > > +       drm_info(&xe->drm, "Resizing bar from %llu -> %llu\n", current_size, rebar_size);
> > >
> > > Maybe convert to MiB and print the unit?
> > Will change in the new revision.
> > >
> > > > +
> > > >         while (root->parent)
> > > >                 root = root->parent;
> > > >
> > > > @@ -117,7 +119,7 @@ static int xe_resize_vram_bar(struct xe_device *xe, resource_size_t vram_size)
> > > >         }
> > > >
> > > >         if (!root_res) {
> > > > -               drm_info(&xe->drm, "Can't resize VRAM BAR - platform support is missing\n");
> > > > +               drm_info(&xe->drm, "Can't resize VRAM BAR - platform support is missing. Consider enabling 'Resizable BAR' support in your BIOS\n");

Print regarding 'Resizable BAR' support is added here. I thought this is
the point where it would fail if platform lacks resizable BAR support.

Let me add the same print inside _resize_bar() function as well when it
fails and send a new revision.

> > > >                 return -1;
> > > >         }
> > > >
> > > > @@ -183,7 +185,7 @@ int xe_mmio_probe_vram(struct xe_device *xe)
> > > >         u64 vram_size;
> > > >         u64 original_size;
> > > >         u64 usable_size;
> > > > -       int resize_result, err;
> > > > +       int err;
> > > >
> > > >         if (!IS_DGFX(xe)) {
> > > >                 xe->mem.vram.mapping = 0;
> > > > @@ -212,7 +214,7 @@ int xe_mmio_probe_vram(struct xe_device *xe)
> > > >         if (err)
> > > >                 return err;
> > > >
> > > > -       resize_result = xe_resize_vram_bar(xe, vram_size);
> > > > +       xe_resize_vram_bar(xe, vram_size);
> > > >         xe->mem.vram.io_start = pci_resource_start(pdev, GEN12_LMEM_BAR);
> > > >         xe->mem.vram.io_size = min(usable_size,
> > > >                                    pci_resource_len(pdev, GEN12_LMEM_BAR));
> > > > @@ -221,13 +223,6 @@ int xe_mmio_probe_vram(struct xe_device *xe)
> > > >         if (!xe->mem.vram.size)
> > > >                 return -EIO;
> > > >
> > > > -       if (resize_result > 0)
> > > > -               drm_info(&xe->drm, "Successfully resize VRAM from %lluMiB to %lluMiB\n",
> > > > -                        (u64)original_size >> 20,
> > > > -                        (u64)xe->mem.vram.io_size >> 20);
> > > > -       else if (xe->mem.vram.io_size < usable_size && !xe_force_vram_bar_size)
> > > > -               drm_info(&xe->drm, "Using a reduced BAR size of %lluMiB. Consider enabling 'Resizable BAR' support in your BIOS.\n",
> > > > -                        (u64)xe->mem.vram.size >> 20);
> > >
> > > Should we then update the _resize_bar() fail message to include this
> > > information?
> > This messsage is included in the print when resize fails.
> 
> I mean specifically the "..Consider enabling 'Resizable BAR' support
> in your BIOS". The 'Resizable BAR' bit should be easy enough to
> google. Do we print that on resize failure?

See my comments above.

Regards,
Bala
> 
> >
> > Regards,
> > Bala
> > >
> > > >         if (usable_size > xe->mem.vram.io_size)
> > > >                 drm_warn(&xe->drm, "Restricting VRAM size to PCI resource size (0x%llx->0x%llx)\n",
> > > >                          usable_size, xe->mem.vram.io_size);
> > > > --
> > > > 2.34.1
> > > >