[PATCH 33/39] drm: rip out drm_core_has_MTRR checks

[Andy Lutomirski]

On Wed, Jul 10, 2013 at 8:59 AM, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> On Wed, Jul 10, 2013 at 5:41 PM, David Herrmann <dh.herrmann at gmail.com> wrote:
>> On Wed, Jul 10, 2013 at 5:22 PM, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
>>> On Wed, Jul 10, 2013 at 3:51 PM, David Herrmann <dh.herrmann at 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 at 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 at 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.
>

Are all of those codepaths really inaccessible in non-legacy drm
drivers?  I didn't try to fully unravel all the ioctls and such, but
it seems like userspace could add bufs and map them.  Since the mtrr
code isn't very robust (reference counting?  what reference
counting?), I'm a little bit worried that potentially enabling it in
more cases, which your patch does, could be harmful.

The arch_phys_wc stuff puts a prettier interface on the mtrr code and
turns it off when PAT is available, but the underlying code is still
just as bad.

--Andy