Re: [PATCH 06/10] drm/gma500: Move GTT resume logic out of psb_gtt_init()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Mar 7, 2022 at 8:07 PM Sam Ravnborg <sam@xxxxxxxxxxxx> wrote:
>
> Hi Thomas,
>
> One comment below.
>
> On Sun, Mar 06, 2022 at 09:36:15PM +0100, Thomas Zimmermann wrote:
> > The current implementation of psb_gtt_init() also does resume
> > handling. Move the resume code into its own helper.
> >
> > Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > ---
> >  drivers/gpu/drm/gma500/gtt.c     | 122 ++++++++++++++++++++++++++-----
> >  drivers/gpu/drm/gma500/gtt.h     |   2 +-
> >  drivers/gpu/drm/gma500/psb_drv.c |   2 +-
> >  3 files changed, 104 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
> > index acd50ee26b03..43ad3ec38c80 100644
> > --- a/drivers/gpu/drm/gma500/gtt.c
> > +++ b/drivers/gpu/drm/gma500/gtt.c
> > @@ -209,7 +209,7 @@ static void psb_gtt_populate_resources(struct drm_psb_private *pdev)
> >       drm_dbg(dev, "Restored %u of %u gtt ranges (%u KB)", restored, total, (size / 1024));
> >  }
> >
> > -int psb_gtt_init(struct drm_device *dev, int resume)
> > +int psb_gtt_init(struct drm_device *dev)
> >  {
> >       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> >       struct pci_dev *pdev = to_pci_dev(dev->dev);
> > @@ -218,10 +218,8 @@ int psb_gtt_init(struct drm_device *dev, int resume)
> >       struct psb_gtt *pg;
> >       int ret = 0;
> >
> > -     if (!resume) {
> > -             mutex_init(&dev_priv->gtt_mutex);
> > -             mutex_init(&dev_priv->mmap_mutex);
> > -     }
> > +     mutex_init(&dev_priv->gtt_mutex);
> > +     mutex_init(&dev_priv->mmap_mutex);
> >
> >       pg = &dev_priv->gtt;
> >
> > @@ -290,13 +288,6 @@ int psb_gtt_init(struct drm_device *dev, int resume)
> >       dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n",
> >                       dev_priv->stolen_base, vram_stolen_size / 1024);
> >
> > -     if (resume && (gtt_pages != pg->gtt_pages) &&
> > -         (stolen_size != pg->stolen_size)) {
> > -             dev_err(dev->dev, "GTT resume error.\n");
> > -             ret = -EINVAL;
> > -             goto out_err;
> > -     }
> > -
> >       pg->gtt_pages = gtt_pages;
> >       pg->stolen_size = stolen_size;
> >       dev_priv->vram_stolen_size = vram_stolen_size;
> > @@ -304,19 +295,14 @@ int psb_gtt_init(struct drm_device *dev, int resume)
> >       /*
> >        *      Map the GTT and the stolen memory area
> >        */
> > -     if (!resume)
> > -             dev_priv->gtt_map = ioremap(pg->gtt_phys_start,
> > -                                             gtt_pages << PAGE_SHIFT);
> > +     dev_priv->gtt_map = ioremap(pg->gtt_phys_start, gtt_pages << PAGE_SHIFT);
> >       if (!dev_priv->gtt_map) {
> >               dev_err(dev->dev, "Failure to map gtt.\n");
> >               ret = -ENOMEM;
> >               goto out_err;
> >       }
> >
> > -     if (!resume)
> > -             dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base,
> > -                                              stolen_size);
> > -
> > +     dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base, stolen_size);
> >       if (!dev_priv->vram_addr) {
> >               dev_err(dev->dev, "Failure to map stolen base.\n");
> >               ret = -ENOMEM;
> > @@ -333,11 +319,107 @@ int psb_gtt_init(struct drm_device *dev, int resume)
> >       return ret;
> >  }
> >
> The below is a lot of duplicated complex code.
> Can you add one more helper for this?

I was thinking the same but figured it would be an easy follow up
patch. But sure, why not fix it here already.

While at it, the gtt enable/disable code could be put in helpers as well.

>
> > +static int psb_gtt_resume(struct drm_device *dev)
> > +{
> > +     struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> > +     struct pci_dev *pdev = to_pci_dev(dev->dev);
> > +     unsigned int gtt_pages;
> > +     unsigned long stolen_size, vram_stolen_size;
> > +     struct psb_gtt *pg;
> > +     int ret = 0;
> > +
> > +     pg = &dev_priv->gtt;
>
> static void psb_enable_gtt(..)
> {
> > +
> > +     /* Enable the GTT */
> > +     pci_read_config_word(pdev, PSB_GMCH_CTRL, &dev_priv->gmch_ctrl);
> > +     pci_write_config_word(pdev, PSB_GMCH_CTRL,
> > +                           dev_priv->gmch_ctrl | _PSB_GMCH_ENABLED);
> > +
> > +     dev_priv->pge_ctl = PSB_RVDC32(PSB_PGETBL_CTL);
> > +     PSB_WVDC32(dev_priv->pge_ctl | _PSB_PGETBL_ENABLED, PSB_PGETBL_CTL);
> > +     (void) PSB_RVDC32(PSB_PGETBL_CTL);
> > +
> > +     /* The root resource we allocate address space from */
> > +     dev_priv->gtt_initialized = 1;
> > +
> > +     pg->gtt_phys_start = dev_priv->pge_ctl & PAGE_MASK;
> > +
> > +     /*
> > +      *      The video mmu has a hw bug when accessing 0x0D0000000.
> > +      *      Make gatt start at 0x0e000,0000. This doesn't actually
> > +      *      matter for us but may do if the video acceleration ever
> > +      *      gets opened up.
> > +      */
> > +     pg->mmu_gatt_start = 0xE0000000;
> > +
> > +     pg->gtt_start = pci_resource_start(pdev, PSB_GTT_RESOURCE);
> > +     gtt_pages = pci_resource_len(pdev, PSB_GTT_RESOURCE) >> PAGE_SHIFT;
> > +     /* CDV doesn't report this. In which case the system has 64 gtt pages */
> > +     if (pg->gtt_start == 0 || gtt_pages == 0) {
> > +             dev_dbg(dev->dev, "GTT PCI BAR not initialized.\n");
> > +             gtt_pages = 64;
> > +             pg->gtt_start = dev_priv->pge_ctl;
> > +     }
> > +
> > +     pg->gatt_start = pci_resource_start(pdev, PSB_GATT_RESOURCE);
> > +     pg->gatt_pages = pci_resource_len(pdev, PSB_GATT_RESOURCE) >> PAGE_SHIFT;
> > +     dev_priv->gtt_mem = &pdev->resource[PSB_GATT_RESOURCE];
> > +
> > +     if (pg->gatt_pages == 0 || pg->gatt_start == 0) {
> > +             static struct resource fudge;   /* Preferably peppermint */
> > +             /*
> > +              * This can occur on CDV systems. Fudge it in this case. We
> > +              * really don't care what imaginary space is being allocated
> > +              * at this point.
> > +              */
> > +             dev_dbg(dev->dev, "GATT PCI BAR not initialized.\n");
> > +             pg->gatt_start = 0x40000000;
> > +             pg->gatt_pages = (128 * 1024 * 1024) >> PAGE_SHIFT;
> > +             /*
> > +              * This is a little confusing but in fact the GTT is providing
> > +              * a view from the GPU into memory and not vice versa. As such
> > +              *  this is really allocating space that is not the same as the
> > +              *  CPU address space on CDV.
> > +              */
> > +             fudge.start = 0x40000000;
> > +             fudge.end = 0x40000000 + 128 * 1024 * 1024 - 1;
> > +             fudge.name = "fudge";
> > +             fudge.flags = IORESOURCE_MEM;
> > +             dev_priv->gtt_mem = &fudge;
> > +     }
> > +
> > +     pci_read_config_dword(pdev, PSB_BSM, &dev_priv->stolen_base);
> > +     vram_stolen_size = pg->gtt_phys_start - dev_priv->stolen_base - PAGE_SIZE;
> > +     stolen_size = vram_stolen_size;
> > +
> > +     dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n",
> > +                     dev_priv->stolen_base, vram_stolen_size / 1024);
> }
>
> And then use this helper in both init and resume?
>
> > +
> > +     if ((gtt_pages != pg->gtt_pages) && (stolen_size != pg->stolen_size)) {
> > +             dev_err(dev->dev, "GTT resume error.\n");
> > +             ret = -EINVAL;
> > +             goto out_err;
> > +     }
> > +
>
> > +     pg->gtt_pages = gtt_pages;
> > +     pg->stolen_size = stolen_size;
> Not needed for resume, we just checked them.
>
> > +     dev_priv->vram_stolen_size = vram_stolen_size;
> > +
> > +     psb_gtt_clear(dev_priv);
> > +     psb_gtt_populate_stolen(dev_priv);
> > +
> > +     return 0;
> > +
> > +out_err:
> > +     psb_gtt_takedown(dev);
> > +     return ret;
> > +}
> > +
> >  int psb_gtt_restore(struct drm_device *dev)
> >  {
> >       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> >
> > -     psb_gtt_init(dev, 1);
> > +     psb_gtt_resume(dev);
> >
> >       psb_gtt_populate_resources(dev_priv);
> >
> > diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
> > index 31500533ac45..cb270ea40794 100644
> > --- a/drivers/gpu/drm/gma500/gtt.h
> > +++ b/drivers/gpu/drm/gma500/gtt.h
> > @@ -25,7 +25,7 @@ struct psb_gtt {
> >  };
> >
> >  /* Exported functions */
> > -extern int psb_gtt_init(struct drm_device *dev, int resume);
> > +int psb_gtt_init(struct drm_device *dev);
> >  extern void psb_gtt_takedown(struct drm_device *dev);
> >  extern int psb_gtt_restore(struct drm_device *dev);
> A cleanup patch to remove all extern would be nice.
> But that's not related to this series.
>
>         Sam



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux