From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751950AbbCTXwp (ORCPT ); Fri, 20 Mar 2015 19:52:45 -0400 Received: from mail-la0-f51.google.com ([209.85.215.51]:32817 "EHLO mail-la0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751712AbbCTXwk (ORCPT ); Fri, 20 Mar 2015 19:52:40 -0400 MIME-Version: 1.0 In-Reply-To: <1426893517-2511-10-git-send-email-mcgrof@do-not-panic.com> References: <1426893517-2511-1-git-send-email-mcgrof@do-not-panic.com> <1426893517-2511-10-git-send-email-mcgrof@do-not-panic.com> From: Andy Lutomirski Date: Fri, 20 Mar 2015 16:52:18 -0700 Message-ID: Subject: Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around To: "Luis R. Rodriguez" Cc: Ingo Molnar , Thomas Gleixner , "H. Peter Anvin" , Juergen Gross , Jan Beulich , Borislav Petkov , Suresh Siddha , venkatesh.pallipadi@intel.com, Dave Airlie , "linux-kernel@vger.kernel.org" , Linux Fbdev development list , X86 ML , "xen-devel@lists.xenproject.org" , "Luis R. Rodriguez" , Ingo Molnar , Linus Torvalds , Daniel Vetter , Antonino Daplas , Jean-Christophe Plagniol-Villard , Tomi Valkeinen Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez wrote: > From: "Luis R. Rodriguez" > > The atyfb driver uses an MTRR work around since some > cards use the same PCI BAR for the framebuffer and MMIO. > In such cards the last page is used for MMIO, the rest for > the framebuffer, so on those cards we ioremap() the MMIO > page alone, then again ioremap() the full framebuffer > including the MMIO space *and* ___then___ use an MTRR with > MTRR_TYPE_WRCOMB on the full PCI BAR... and finally "hole" > in an MTRR_TYPE_UNCACHABLE MTRR only for MMIO. > > This is a terrible fucking work around, and should by no means > be necessary however evidence through a large series of conversion > of drivers to ioremap_wc() for the framebuffer shows that around > the time MTRR started becoming popular devices did not have things > lined up for easily separating the framebuffer and MMIO register > access. In some cases a driver requires significant intrusive > changes in order to make the split for an ioremap() for MMIO registers > and another ioremap_wc() for the framebuffer, at other times a > bit of careful study of the driver suffices. This example driver > falls into the later category. > > We can replace the MTRR MTRR_TYPE_UNCACHABLE > work around by using ioremap_nocache(), the length of the > MMIO space should already be correct. The other part we > need to correct is ensuring we ioremap() for the framebuffer > only the required size. Since the ioremap() happens early > on probe for PCI devices before aty_init() where we typically > adjust the length and know how to do it, we can fix this by > pegging the bus type as PCI on PCI probe, and finally fudging > and framebuffer length just as we do on aty_init(). > > The last thing we do must do to remain sane is ensure we > use the info->fix.smem_start and info->fix.smem_len for > the framebuffer MTRR as we know that is always well adjusted. > The *one* concern here would be if the MTRR is not in units > of 4K __but__ we already know that in the PCI case this cannot > happen, in the shared space setting the MTRR would be up to > 0x7ff000 and assuming a 4K page: > > ; 0x7ff000 / 0x1000 > 2047 > > Also, internally when MTRR is used mtrr_add() will use mtrr_check() > and that should splat a warning when the MTRR base and size are > not compatible with what is expected for MTRR usage. > > This fix lets us nuke the MTRR_TYPE_UNCACHABLE MTRR "hole". > > Cc: Suresh Siddha > Cc: Venkatesh Pallipadi > Cc: Ingo Molnar > Cc: Linus Torvalds > Cc: Thomas Gleixner > Cc: Juergen Gross > Cc: Daniel Vetter > Cc: Andy Lutomirski > Cc: Dave Airlie > Cc: Antonino Daplas > Cc: Jean-Christophe Plagniol-Villard > Cc: Tomi Valkeinen > Cc: linux-fbdev@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Luis R. Rodriguez > --- > drivers/video/fbdev/aty/atyfb.h | 1 - > drivers/video/fbdev/aty/atyfb_base.c | 28 ++++++---------------------- > 2 files changed, 6 insertions(+), 23 deletions(-) > > diff --git a/drivers/video/fbdev/aty/atyfb.h b/drivers/video/fbdev/aty/atyfb.h > index 1f39a62..89ec439 100644 > --- a/drivers/video/fbdev/aty/atyfb.h > +++ b/drivers/video/fbdev/aty/atyfb.h > @@ -184,7 +184,6 @@ struct atyfb_par { > spinlock_t int_lock; > #ifdef CONFIG_MTRR > int mtrr_aper; > - int mtrr_reg; > #endif > u32 mem_cntl; > struct crtc saved_crtc; > 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. I think we might need ioremap_nocache_me_harder (or maybe ioremap_x86_uc if you prefer that bikeshed color) for this. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Lutomirski Date: Fri, 20 Mar 2015 23:52:18 +0000 Subject: Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around Message-Id: List-Id: References: <1426893517-2511-1-git-send-email-mcgrof@do-not-panic.com> <1426893517-2511-10-git-send-email-mcgrof@do-not-panic.com> In-Reply-To: <1426893517-2511-10-git-send-email-mcgrof@do-not-panic.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: "Luis R. Rodriguez" Cc: Ingo Molnar , Thomas Gleixner , "H. Peter Anvin" , Juergen Gross , Jan Beulich , Borislav Petkov , Suresh Siddha , venkatesh.pallipadi@intel.com, Dave Airlie , "linux-kernel@vger.kernel.org" , Linux Fbdev development list , X86 ML , "xen-devel@lists.xenproject.org" , "Luis R. Rodriguez" , Ingo Molnar , Linus Torvalds , Daniel Vetter , Antonino Daplas , Jean-Christophe Plagniol-Villard , Tomi Valkeinen On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez wrote: > From: "Luis R. Rodriguez" > > The atyfb driver uses an MTRR work around since some > cards use the same PCI BAR for the framebuffer and MMIO. > In such cards the last page is used for MMIO, the rest for > the framebuffer, so on those cards we ioremap() the MMIO > page alone, then again ioremap() the full framebuffer > including the MMIO space *and* ___then___ use an MTRR with > MTRR_TYPE_WRCOMB on the full PCI BAR... and finally "hole" > in an MTRR_TYPE_UNCACHABLE MTRR only for MMIO. > > This is a terrible fucking work around, and should by no means > be necessary however evidence through a large series of conversion > of drivers to ioremap_wc() for the framebuffer shows that around > the time MTRR started becoming popular devices did not have things > lined up for easily separating the framebuffer and MMIO register > access. In some cases a driver requires significant intrusive > changes in order to make the split for an ioremap() for MMIO registers > and another ioremap_wc() for the framebuffer, at other times a > bit of careful study of the driver suffices. This example driver > falls into the later category. > > We can replace the MTRR MTRR_TYPE_UNCACHABLE > work around by using ioremap_nocache(), the length of the > MMIO space should already be correct. The other part we > need to correct is ensuring we ioremap() for the framebuffer > only the required size. Since the ioremap() happens early > on probe for PCI devices before aty_init() where we typically > adjust the length and know how to do it, we can fix this by > pegging the bus type as PCI on PCI probe, and finally fudging > and framebuffer length just as we do on aty_init(). > > The last thing we do must do to remain sane is ensure we > use the info->fix.smem_start and info->fix.smem_len for > the framebuffer MTRR as we know that is always well adjusted. > The *one* concern here would be if the MTRR is not in units > of 4K __but__ we already know that in the PCI case this cannot > happen, in the shared space setting the MTRR would be up to > 0x7ff000 and assuming a 4K page: > > ; 0x7ff000 / 0x1000 > 2047 > > Also, internally when MTRR is used mtrr_add() will use mtrr_check() > and that should splat a warning when the MTRR base and size are > not compatible with what is expected for MTRR usage. > > This fix lets us nuke the MTRR_TYPE_UNCACHABLE MTRR "hole". > > Cc: Suresh Siddha > Cc: Venkatesh Pallipadi > Cc: Ingo Molnar > Cc: Linus Torvalds > Cc: Thomas Gleixner > Cc: Juergen Gross > Cc: Daniel Vetter > Cc: Andy Lutomirski > Cc: Dave Airlie > Cc: Antonino Daplas > Cc: Jean-Christophe Plagniol-Villard > Cc: Tomi Valkeinen > Cc: linux-fbdev@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Luis R. Rodriguez > --- > drivers/video/fbdev/aty/atyfb.h | 1 - > drivers/video/fbdev/aty/atyfb_base.c | 28 ++++++---------------------- > 2 files changed, 6 insertions(+), 23 deletions(-) > > diff --git a/drivers/video/fbdev/aty/atyfb.h b/drivers/video/fbdev/aty/atyfb.h > index 1f39a62..89ec439 100644 > --- a/drivers/video/fbdev/aty/atyfb.h > +++ b/drivers/video/fbdev/aty/atyfb.h > @@ -184,7 +184,6 @@ struct atyfb_par { > spinlock_t int_lock; > #ifdef CONFIG_MTRR > int mtrr_aper; > - int mtrr_reg; > #endif > u32 mem_cntl; > struct crtc saved_crtc; > 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. I think we might need ioremap_nocache_me_harder (or maybe ioremap_x86_uc if you prefer that bikeshed color) for this.