Re: [PATCH 17/25] drm: rip out drm_core_has_MTRR checks

[David Herrmann <dh.herrmann@gmail.com>]

Hi

On Thu, Aug 8, 2013 at 3:41 PM, Daniel Vetter <daniel.vetter@ffwll.ch> 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 <dh.herrmann@gmail.com> wrote:
>> On Wed, Jul 10, 2013 at 5:22 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>> On Wed, Jul 10, 2013 at 3:51 PM, David Herrmann <dh.herrmann@gmail.com> 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 <dh.herrmann@gmail.com>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

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 <linux/idr.h>
>
>  #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
>