Re: [PATCH 11/13] drm/i915: Cut out the infamous ILK w/a from AGP layer

[Ben Widawsky <ben@bwidawsk.net>]

On Wed, 16 Jan 2013 21:09:32 -0200
Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:

> On Tue, Jan 15, 2013 at 7:26 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> > And, move it to where the rest of the logic is.
> >
> > Cc: Dave Airlie <airlied@redhat.com>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/char/agp/intel-gtt.c        | 27 ---------------------------
> >  drivers/gpu/drm/i915/i915_drv.h     |  1 +
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 27 ++++++++++++++++++++++++---
> >  include/drm/intel-gtt.h             |  2 --
> >  4 files changed, 25 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> > index a531377..f54ddc0 100644
> > --- a/drivers/char/agp/intel-gtt.c
> > +++ b/drivers/char/agp/intel-gtt.c
> > @@ -849,9 +849,6 @@ static int intel_fake_agp_insert_entries(struct agp_memory *mem,
> >  {
> >         int ret = -EINVAL;
> >
> > -       if (intel_private.base.do_idle_maps)
> > -               return -ENODEV;
> > -
> >         if (intel_private.clear_fake_agp) {
> >                 int start = intel_private.stolen_size / PAGE_SIZE;
> >                 int end = intel_private.gtt_mappable_entries;
> > @@ -916,9 +913,6 @@ static int intel_fake_agp_remove_entries(struct agp_memory *mem,
> >         if (mem->page_count == 0)
> >                 return 0;
> >
> > -       if (intel_private.base.do_idle_maps)
> > -               return -ENODEV;
> > -
> 
> I'm not convinced new logic replaces these do_idle_maps asserts

As the author of that original logic, it should have always just been a
sanity check. I think Chris even mentioned in the original patch we
shouldn't bother with the check, but meh.

Anyway, you are right that it's not replaced, but that should be fine.

> 
> >         intel_gtt_clear_range(pg_start, mem->page_count);
> >
> >         if (intel_private.needs_dmar) {
> > @@ -1078,24 +1072,6 @@ static void i965_write_entry(dma_addr_t addr,
> >         writel(addr | pte_flags, intel_private.gtt + entry);
> >  }
> >
> > -/* Certain Gen5 chipsets require require idling the GPU before
> > - * unmapping anything from the GTT when VT-d is enabled.
> > - */
> > -static inline int needs_idle_maps(void)
> > -{
> > -#ifdef CONFIG_INTEL_IOMMU
> > -       const unsigned short gpu_devid = intel_private.pcidev->device;
> > -
> > -       /* Query intel_iommu to see if we need the workaround. Presumably that
> > -        * was loaded first.
> > -        */
> > -       if ((gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_HB ||
> > -            gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_IG) &&
> > -            intel_iommu_gfx_mapped)
> > -               return 1;
> > -#endif
> > -       return 0;
> > -}
> >
> >  static int i9xx_setup(void)
> >  {
> > @@ -1124,9 +1100,6 @@ static int i9xx_setup(void)
> >                 break;
> >         }
> >
> > -       if (needs_idle_maps())
> > -               intel_private.base.do_idle_maps = 1;
> > -
> >         intel_i9xx_setup_flush();
> >
> >         return 0;
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 201da6d..6c8b0b8 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -385,6 +385,7 @@ struct i915_gtt {
> >         /** "Graphics Stolen Memory" holds the global PTEs */
> >         void __iomem *gsm;
> >
> > +       bool do_idle_maps;
> >  };
> >  #define gtt_total_entries(gtt) ((gtt).total >> PAGE_SHIFT)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 2acca75..afd5ec7 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -330,11 +330,30 @@ void i915_gem_init_ppgtt(struct drm_device *dev)
> >         }
> >  }
> >
> > +extern int intel_iommu_gfx_mapped;
> > +/* Certain Gen5 chipsets require require idling the GPU before
> > + * unmapping anything from the GTT when VT-d is enabled.
> > + */
> > +static inline bool needs_idle_maps(struct drm_device *dev)
> > +{
> > +#ifdef CONFIG_INTEL_IOMMU
> > +       const u16 gpu_devid = dev->pdev->device;
> > +
> > +       /* Query intel_iommu to see if we need the workaround. Presumably that
> > +        * was loaded first.
> > +        */
> > +       if ((gpu_devid == 0x0044 || gpu_devid == 0x46) &&
> 
> I would prefer to stay with defied names instead of direct ids.
> 

There is one problem, apparently we don't support 0x44 in our kernel,
and I didn't want to have the workaround there for an undefined chipset.
I see no reason to add 0x44 if Intel never shipped one. So I'd be happy
to use an existing define for 0x46, and leave 0x44 hardcoded if that
pleases people.

> > +            intel_iommu_gfx_mapped)
> > +               return true;
> > +#endif
> > +       return false;
> > +}
> > +
> >  static bool do_idling(struct drm_i915_private *dev_priv)
> >  {
> >         bool ret = dev_priv->mm.interruptible;
> >
> > -       if (unlikely(dev_priv->mm.gtt->do_idle_maps)) {
> > +       if (unlikely(dev_priv->gtt.do_idle_maps)) {
> >                 dev_priv->mm.interruptible = false;
> >                 if (i915_gpu_idle(dev_priv->dev)) {
> >                         DRM_ERROR("Couldn't idle GPU\n");
> > @@ -348,11 +367,10 @@ static bool do_idling(struct drm_i915_private *dev_priv)
> >
> >  static void undo_idling(struct drm_i915_private *dev_priv, bool interruptible)
> >  {
> > -       if (unlikely(dev_priv->mm.gtt->do_idle_maps))
> > +       if (unlikely(dev_priv->gtt.do_idle_maps))
> >                 dev_priv->mm.interruptible = interruptible;
> >  }
> >
> > -
> >  static void i915_ggtt_clear_range(struct drm_device *dev,
> >                                  unsigned first_entry,
> >                                  unsigned num_entries)
> > @@ -701,6 +719,9 @@ int i915_gem_gtt_init(struct drm_device *dev)
> >                         intel_gmch_remove();
> >                         return -ENODEV;
> >                 }
> > +
> > +               dev_priv->gtt.do_idle_maps = needs_idle_maps(dev);
> > +
> >                 return 0;
> >         }
> >
> > diff --git a/include/drm/intel-gtt.h b/include/drm/intel-gtt.h
> > index 63157c5..4982dca 100644
> > --- a/include/drm/intel-gtt.h
> > +++ b/include/drm/intel-gtt.h
> > @@ -5,8 +5,6 @@
> >
> >  struct intel_gtt {
> >         /* Whether we idle the gpu before mapping/unmapping */
> > -       unsigned int do_idle_maps : 1;
> > -       /* Share the scratch page dma with ppgtts. */
> >         dma_addr_t scratch_page_dma;
> >         struct page *scratch_page;
> >  } *intel_gtt_get(void);
> > --
> > 1.8.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> Other than that, feel free to use:
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> 
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br



-- 
Ben Widawsky, Intel Open Source Technology Center