From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Herrmann Subject: Re: [PATCH 17/25] drm: rip out drm_core_has_MTRR checks Date: Thu, 15 Aug 2013 14:44:26 +0200 Message-ID: References: <1375969295-18929-1-git-send-email-daniel.vetter@ffwll.ch> <1375969295-18929-18-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ie0-f182.google.com (mail-ie0-f182.google.com [209.85.223.182]) by gabe.freedesktop.org (Postfix) with ESMTP id 20F7BE81C6 for ; Thu, 15 Aug 2013 05:44:27 -0700 (PDT) Received: by mail-ie0-f182.google.com with SMTP id tp5so1095791ieb.41 for ; Thu, 15 Aug 2013 05:44:26 -0700 (PDT) In-Reply-To: <1375969295-18929-18-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: DRI Development , Andy Lutomirski List-Id: dri-devel@lists.freedesktop.org Hi On Thu, Aug 8, 2013 at 3:41 PM, Daniel Vetter wrote: > The new arch_phys_wc_add/del functions do the right thing both with > and without MTRR support in the kernel. So we can drop these > additional checks. > > David Herrmann suggest to also kill the DRIVER_USE_MTRR flag since > it's now unused, which spurred me to do a bit a better audit of the > affected drivers. David helped a lot in that. Quoting our mail > discussion: > > On Wed, Jul 10, 2013 at 5:41 PM, David Herrmann wrote: >> On Wed, Jul 10, 2013 at 5:22 PM, Daniel Vetter wrote: >>> On Wed, Jul 10, 2013 at 3:51 PM, David Herrmann wrote: >>>>> -#if __OS_HAS_MTRR >>>>> -static inline int drm_core_has_MTRR(struct drm_device *dev) >>>>> -{ >>>>> - return drm_core_check_feature(dev, DRIVER_USE_MTRR); >>>>> -} >>>>> -#else >>>>> -#define drm_core_has_MTRR(dev) (0) >>>>> -#endif >>>>> - >>>> >>>> That was the last user of DRIVER_USE_MTRR (apart from drivers setting >>>> it in .driver_features). Any reason to keep it around? >>> >>> Yeah, I guess we could rip things out. Which will also force me to >>> properly audit drivers for the eventual behaviour change this could >>> entail (in case there's an x86 driver which did not ask for an mtrr, >>> but iirc there isn't). >> >> david@david-mb ~/dev/kernel/linux $ for i in drivers/gpu/drm/* ; do if >> test -d "$i" ; then if ! grep -q USE_MTRR -r $i ; then echo $i ; fi ; >> fi ; done >> drivers/gpu/drm/exynos >> drivers/gpu/drm/gma500 >> drivers/gpu/drm/i2c >> drivers/gpu/drm/nouveau >> drivers/gpu/drm/omapdrm >> drivers/gpu/drm/qxl >> drivers/gpu/drm/rcar-du >> drivers/gpu/drm/shmobile >> drivers/gpu/drm/tilcdc >> drivers/gpu/drm/ttm >> drivers/gpu/drm/udl >> drivers/gpu/drm/vmwgfx >> david@david-mb ~/dev/kernel/linux $ >> >> So for x86 gma500,nouveau,qxl,udl,vmwgfx don't set DRIVER_USE_MTRR. >> But I cannot tell whether they break if we call arch_phys_wc_add/del, >> anyway. At least nouveau seemed to work here, but it doesn't use AGP >> or drm_bufs, I guess. > > Cool, thanks a lot for stitching together the list of drivers to look > at. So for real KMS drivers it's the drives responsibility to add an > mtrr if it needs one. nouvea, radeon, mgag200, i915 and vmwgfx do that > already. Somehow the savage driver also ends up doing that, I have no > idea why. > > Note that gma500 as a pure KMS driver doesn't need MTRR setup since > the platforms that it supports all support PAT. So no MTRRs needed to > get wc iomappings. > > The mtrr support in the drm core is all for legacy mappings of garts, > framebuffers and registers. All legacy drivers set the USE_MTRR flag, > so we're good there. > > All in all I think we can really just ditch this > > /endquote > > v2: Also kill DRIVER_USE_MTRR as suggested by David Herrmann > > v3: Rebase on top of David Herrmann's agp setup/cleanup changes. > > Cc: David Herrmann > Cc: Andy Lutomirski > Signed-off-by: Daniel Vetter Reviewed-by: David Herrmann Regards David > --- > drivers/gpu/drm/ast/ast_drv.c | 2 +- > drivers/gpu/drm/cirrus/cirrus_drv.c | 2 +- > drivers/gpu/drm/drm_bufs.c | 13 +++++-------- > drivers/gpu/drm/drm_pci.c | 14 ++++++-------- > drivers/gpu/drm/drm_vm.c | 3 +-- > drivers/gpu/drm/i810/i810_drv.c | 2 +- > drivers/gpu/drm/i915/i915_drv.c | 2 +- > drivers/gpu/drm/mga/mga_drv.c | 2 +- > drivers/gpu/drm/mgag200/mgag200_drv.c | 2 +- > drivers/gpu/drm/r128/r128_drv.c | 2 +- > drivers/gpu/drm/radeon/radeon_drv.c | 4 ++-- > drivers/gpu/drm/savage/savage_drv.c | 2 +- > drivers/gpu/drm/sis/sis_drv.c | 2 +- > drivers/gpu/drm/tdfx/tdfx_drv.c | 1 - > drivers/gpu/drm/via/via_drv.c | 2 +- > include/drm/drmP.h | 11 ----------- > 16 files changed, 24 insertions(+), 42 deletions(-) > > diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c > index 60f1ce3..32e270d 100644 > --- a/drivers/gpu/drm/ast/ast_drv.c > +++ b/drivers/gpu/drm/ast/ast_drv.c > @@ -197,7 +197,7 @@ static const struct file_operations ast_fops = { > }; > > static struct drm_driver driver = { > - .driver_features = DRIVER_USE_MTRR | DRIVER_MODESET | DRIVER_GEM, > + .driver_features = DRIVER_MODESET | DRIVER_GEM, > .dev_priv_size = 0, > > .load = ast_driver_load, > diff --git a/drivers/gpu/drm/cirrus/cirrus_drv.c b/drivers/gpu/drm/cirrus/cirrus_drv.c > index dd9c908..138364d 100644 > --- a/drivers/gpu/drm/cirrus/cirrus_drv.c > +++ b/drivers/gpu/drm/cirrus/cirrus_drv.c > @@ -87,7 +87,7 @@ static const struct file_operations cirrus_driver_fops = { > #endif > }; > static struct drm_driver driver = { > - .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_USE_MTRR, > + .driver_features = DRIVER_MODESET | DRIVER_GEM, > .load = cirrus_driver_load, > .unload = cirrus_driver_unload, > .fops = &cirrus_driver_fops, > diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c > index 5f73f0a..f63133b 100644 > --- a/drivers/gpu/drm/drm_bufs.c > +++ b/drivers/gpu/drm/drm_bufs.c > @@ -207,12 +207,10 @@ static int drm_addmap_core(struct drm_device * dev, resource_size_t offset, > return 0; > } > > - if (drm_core_has_MTRR(dev)) { > - if (map->type == _DRM_FRAME_BUFFER || > - (map->flags & _DRM_WRITE_COMBINING)) { > - map->mtrr = > - arch_phys_wc_add(map->offset, map->size); > - } > + if (map->type == _DRM_FRAME_BUFFER || > + (map->flags & _DRM_WRITE_COMBINING)) { > + map->mtrr = > + arch_phys_wc_add(map->offset, map->size); > } > if (map->type == _DRM_REGISTERS) { > if (map->flags & _DRM_WRITE_COMBINING) > @@ -464,8 +462,7 @@ int drm_rmmap_locked(struct drm_device *dev, struct drm_local_map *map) > iounmap(map->handle); > /* FALLTHROUGH */ > case _DRM_FRAME_BUFFER: > - if (drm_core_has_MTRR(dev)) > - arch_phys_wc_del(map->mtrr); > + arch_phys_wc_del(map->mtrr); > break; > case _DRM_SHM: > vfree(map->handle); > diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c > index 0f54ad8..3fca2db 100644 > --- a/drivers/gpu/drm/drm_pci.c > +++ b/drivers/gpu/drm/drm_pci.c > @@ -272,12 +272,11 @@ static int drm_pci_agp_init(struct drm_device *dev) > DRM_ERROR("Cannot initialize the agpgart module.\n"); > return -EINVAL; > } > - if (drm_core_has_MTRR(dev)) { > - if (dev->agp) > - dev->agp->agp_mtrr = arch_phys_wc_add( > - dev->agp->agp_info.aper_base, > - dev->agp->agp_info.aper_size * > - 1024 * 1024); > + if (dev->agp) { > + dev->agp->agp_mtrr = arch_phys_wc_add( > + dev->agp->agp_info.aper_base, > + dev->agp->agp_info.aper_size * > + 1024 * 1024); > } > } > return 0; > @@ -286,8 +285,7 @@ static int drm_pci_agp_init(struct drm_device *dev) > static void drm_pci_agp_destroy(struct drm_device *dev) > { > if (drm_core_has_AGP(dev) && dev->agp) { > - if (drm_core_has_MTRR(dev)) > - arch_phys_wc_del(dev->agp->agp_mtrr); > + arch_phys_wc_del(dev->agp->agp_mtrr); > drm_agp_clear(dev); > drm_agp_destroy(dev->agp); > dev->agp = NULL; > diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c > index feb2003..b5c5af7 100644 > --- a/drivers/gpu/drm/drm_vm.c > +++ b/drivers/gpu/drm/drm_vm.c > @@ -251,8 +251,7 @@ static void drm_vm_shm_close(struct vm_area_struct *vma) > switch (map->type) { > case _DRM_REGISTERS: > case _DRM_FRAME_BUFFER: > - if (drm_core_has_MTRR(dev)) > - arch_phys_wc_del(map->mtrr); > + arch_phys_wc_del(map->mtrr); > iounmap(map->handle); > break; > case _DRM_SHM: > diff --git a/drivers/gpu/drm/i810/i810_drv.c b/drivers/gpu/drm/i810/i810_drv.c > index d85c05b..d8180d2 100644 > --- a/drivers/gpu/drm/i810/i810_drv.c > +++ b/drivers/gpu/drm/i810/i810_drv.c > @@ -57,7 +57,7 @@ static const struct file_operations i810_driver_fops = { > > static struct drm_driver driver = { > .driver_features = > - DRIVER_USE_AGP | DRIVER_REQUIRE_AGP | DRIVER_USE_MTRR | > + DRIVER_USE_AGP | DRIVER_REQUIRE_AGP | > DRIVER_HAVE_DMA, > .dev_priv_size = sizeof(drm_i810_buf_priv_t), > .load = i810_driver_load, > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 9411a74..eec47bd 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1006,7 +1006,7 @@ static struct drm_driver driver = { > * deal with them for Intel hardware. > */ > .driver_features = > - DRIVER_USE_AGP | DRIVER_REQUIRE_AGP | /* DRIVER_USE_MTRR |*/ > + DRIVER_USE_AGP | DRIVER_REQUIRE_AGP | > DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM | DRIVER_PRIME, > .load = i915_driver_load, > .unload = i915_driver_unload, > diff --git a/drivers/gpu/drm/mga/mga_drv.c b/drivers/gpu/drm/mga/mga_drv.c > index fe71e1e..6b1a87c 100644 > --- a/drivers/gpu/drm/mga/mga_drv.c > +++ b/drivers/gpu/drm/mga/mga_drv.c > @@ -58,7 +58,7 @@ static const struct file_operations mga_driver_fops = { > > static struct drm_driver driver = { > .driver_features = > - DRIVER_USE_AGP | DRIVER_USE_MTRR | DRIVER_PCI_DMA | > + DRIVER_USE_AGP | DRIVER_PCI_DMA | > DRIVER_HAVE_DMA | DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED, > .dev_priv_size = sizeof(drm_mga_buf_priv_t), > .load = mga_driver_load, > diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c > index b570127..fcce7b2 100644 > --- a/drivers/gpu/drm/mgag200/mgag200_drv.c > +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c > @@ -88,7 +88,7 @@ static const struct file_operations mgag200_driver_fops = { > }; > > static struct drm_driver driver = { > - .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_USE_MTRR, > + .driver_features = DRIVER_GEM | DRIVER_MODESET, > .load = mgag200_driver_load, > .unload = mgag200_driver_unload, > .fops = &mgag200_driver_fops, > diff --git a/drivers/gpu/drm/r128/r128_drv.c b/drivers/gpu/drm/r128/r128_drv.c > index c2338cb..5bd307c 100644 > --- a/drivers/gpu/drm/r128/r128_drv.c > +++ b/drivers/gpu/drm/r128/r128_drv.c > @@ -56,7 +56,7 @@ static const struct file_operations r128_driver_fops = { > > static struct drm_driver driver = { > .driver_features = > - DRIVER_USE_AGP | DRIVER_USE_MTRR | DRIVER_PCI_DMA | DRIVER_SG | > + DRIVER_USE_AGP | DRIVER_PCI_DMA | DRIVER_SG | > DRIVER_HAVE_DMA | DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED, > .dev_priv_size = sizeof(drm_r128_buf_priv_t), > .load = r128_driver_load, > diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c > index 3e52331..1f93dd5 100644 > --- a/drivers/gpu/drm/radeon/radeon_drv.c > +++ b/drivers/gpu/drm/radeon/radeon_drv.c > @@ -275,7 +275,7 @@ static const struct file_operations radeon_driver_old_fops = { > > static struct drm_driver driver_old = { > .driver_features = > - DRIVER_USE_AGP | DRIVER_USE_MTRR | DRIVER_PCI_DMA | DRIVER_SG | > + DRIVER_USE_AGP | DRIVER_PCI_DMA | DRIVER_SG | > DRIVER_HAVE_IRQ | DRIVER_HAVE_DMA | DRIVER_IRQ_SHARED, > .dev_priv_size = sizeof(drm_radeon_buf_priv_t), > .load = radeon_driver_load, > @@ -382,7 +382,7 @@ static const struct file_operations radeon_driver_kms_fops = { > > static struct drm_driver kms_driver = { > .driver_features = > - DRIVER_USE_AGP | DRIVER_USE_MTRR | > + DRIVER_USE_AGP | > DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM | > DRIVER_PRIME, > .dev_priv_size = 0, > diff --git a/drivers/gpu/drm/savage/savage_drv.c b/drivers/gpu/drm/savage/savage_drv.c > index 9135c8b..3c03021 100644 > --- a/drivers/gpu/drm/savage/savage_drv.c > +++ b/drivers/gpu/drm/savage/savage_drv.c > @@ -50,7 +50,7 @@ static const struct file_operations savage_driver_fops = { > > static struct drm_driver driver = { > .driver_features = > - DRIVER_USE_AGP | DRIVER_USE_MTRR | DRIVER_HAVE_DMA | DRIVER_PCI_DMA, > + DRIVER_USE_AGP | DRIVER_HAVE_DMA | DRIVER_PCI_DMA, > .dev_priv_size = sizeof(drm_savage_buf_priv_t), > .load = savage_driver_load, > .firstopen = savage_driver_firstopen, > diff --git a/drivers/gpu/drm/sis/sis_drv.c b/drivers/gpu/drm/sis/sis_drv.c > index b88b2d3..4383b74 100644 > --- a/drivers/gpu/drm/sis/sis_drv.c > +++ b/drivers/gpu/drm/sis/sis_drv.c > @@ -102,7 +102,7 @@ void sis_driver_postclose(struct drm_device *dev, struct drm_file *file) > } > > static struct drm_driver driver = { > - .driver_features = DRIVER_USE_AGP | DRIVER_USE_MTRR, > + .driver_features = DRIVER_USE_AGP, > .load = sis_driver_load, > .unload = sis_driver_unload, > .open = sis_driver_open, > diff --git a/drivers/gpu/drm/tdfx/tdfx_drv.c b/drivers/gpu/drm/tdfx/tdfx_drv.c > index 951ec13..3492ca5 100644 > --- a/drivers/gpu/drm/tdfx/tdfx_drv.c > +++ b/drivers/gpu/drm/tdfx/tdfx_drv.c > @@ -55,7 +55,6 @@ static const struct file_operations tdfx_driver_fops = { > }; > > static struct drm_driver driver = { > - .driver_features = DRIVER_USE_MTRR, > .fops = &tdfx_driver_fops, > .name = DRIVER_NAME, > .desc = DRIVER_DESC, > diff --git a/drivers/gpu/drm/via/via_drv.c b/drivers/gpu/drm/via/via_drv.c > index 4487999..92684a9 100644 > --- a/drivers/gpu/drm/via/via_drv.c > +++ b/drivers/gpu/drm/via/via_drv.c > @@ -72,7 +72,7 @@ static const struct file_operations via_driver_fops = { > > static struct drm_driver driver = { > .driver_features = > - DRIVER_USE_AGP | DRIVER_USE_MTRR | DRIVER_HAVE_IRQ | > + DRIVER_USE_AGP | DRIVER_HAVE_IRQ | > DRIVER_IRQ_SHARED, > .load = via_driver_load, > .unload = via_driver_unload, > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 914055e..5b9462d 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -76,7 +76,6 @@ > #include > > #define __OS_HAS_AGP (defined(CONFIG_AGP) || (defined(CONFIG_AGP_MODULE) && defined(MODULE))) > -#define __OS_HAS_MTRR (defined(CONFIG_MTRR)) > > struct module; > > @@ -141,7 +140,6 @@ int drm_err(const char *func, const char *format, ...); > /* driver capabilities and requirements mask */ > #define DRIVER_USE_AGP 0x1 > #define DRIVER_REQUIRE_AGP 0x2 > -#define DRIVER_USE_MTRR 0x4 > #define DRIVER_PCI_DMA 0x8 > #define DRIVER_SG 0x10 > #define DRIVER_HAVE_DMA 0x20 > @@ -1219,15 +1217,6 @@ static inline int drm_core_has_AGP(struct drm_device *dev) > #define drm_core_has_AGP(dev) (0) > #endif > > -#if __OS_HAS_MTRR > -static inline int drm_core_has_MTRR(struct drm_device *dev) > -{ > - return drm_core_check_feature(dev, DRIVER_USE_MTRR); > -} > -#else > -#define drm_core_has_MTRR(dev) (0) > -#endif > - > static inline void drm_device_set_unplugged(struct drm_device *dev) > { > smp_wmb(); > -- > 1.8.3.2 >