On Wed, 16 Jan 2013 21:09:32 -0200
Rodrigo Vivi <rodrigo.vivi at gmail.com> wrote:
> On Tue, Jan 15, 2013 at 7:26 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
> > And, move it to where the rest of the logic is.
> >
> > Cc: Dave Airlie <airlied at redhat.com>
> > Signed-off-by: Ben Widawsky <ben at 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
> Other than that, feel free to use:
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi at gmail.com>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
--
Ben Widawsky, Intel Open Source Technology Center
