Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around

On Fri, Mar 27, 2015 at 1:12 PM, Luis R. Rodriguez [off-list ref] wrote:

On Fri, Mar 20, 2015 at 04:52:18PM -0700, Andy Lutomirski wrote:

quoted

On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
[off-list ref] wrote:

quoted

diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
index 8025624..8875e56 100644
--- a/drivers/video/fbdev/aty/atyfb_base.c
+++ b/drivers/video/fbdev/aty/atyfb_base.c

@@ -2630,21 +2630,10 @@ static int aty_init(struct fb_info *info)

 #ifdef CONFIG_MTRR
        par->mtrr_aper = -1;
-       par->mtrr_reg = -1;
        if (!nomtrr) {
-               /* Cover the whole resource. */
-               par->mtrr_aper = mtrr_add(par->res_start, par->res_size,
+               par->mtrr_aper = mtrr_add(info->fix.smem_start,
+                                         info->fix.smem_len,
                                          MTRR_TYPE_WRCOMB, 1);
-               if (par->mtrr_aper >= 0 && !par->aux_start) {
-                       /* Make a hole for mmio. */
-                       par->mtrr_reg = mtrr_add(par->res_start + 0x800000 -
-                                                GUI_RESERVE, GUI_RESERVE,
-                                                MTRR_TYPE_UNCACHABLE, 1);
-                       if (par->mtrr_reg < 0) {
-                               mtrr_del(par->mtrr_aper, 0, 0);
-                               par->mtrr_aper = -1;
-                       }
-               }
        }
 #endif

@@ -2776,10 +2765,6 @@ aty_init_exit:
        par->pll_ops->set_pll(info, &par->saved_pll);

 #ifdef CONFIG_MTRR
-       if (par->mtrr_reg >= 0) {
-               mtrr_del(par->mtrr_reg, 0, 0);
-               par->mtrr_reg = -1;
-       }
        if (par->mtrr_aper >= 0) {
                mtrr_del(par->mtrr_aper, 0, 0);
                par->mtrr_aper = -1;

@@ -3466,7 +3451,7 @@ static int atyfb_setup_generic(struct pci_dev *pdev, struct fb_info *info,
        }

        info->fix.mmio_start = raddr;
-       par->ati_regbase = ioremap(info->fix.mmio_start, 0x1000);
+       par->ati_regbase = ioremap_nocache(info->fix.mmio_start, 0x1000);

Double-check me, but I think that ioremap_nocache + WC MTRR = WC.

Precicely, in this case the WC hole was obtained by using MTRR WC. This
patch removes that WC hole trick and now we can be explciit about
only wanting ioremap_nocache() on the registers, that is WC is not
desired here and is not used. The patch does not highlight the fact
that there was left in place another ioremap() call for the framebuffer:

info->screen_base = ioremap(info->fix.smem_start, info->fix.smem_len);

That is the one that later after this patch we use ioremap_wc() for.
This patch just removes the hole solution. That's all.

I don't understand.

If I read it right, there's a 2^n byte BAR.  You're requesting WC for
the whole think using arch_phys_wc_add.  On a PAT system that has no
effect and all is well.  On a non-PAT system, it adds an MTRR.  That
means that you need to override the MTRR somehow for the mmio regs,
and UC- won't do the trick.

Or am I missing something here?

--Andy

  Luis

-- 
Andy Lutomirski
AMA Capital Management, LLC