From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around Date: Fri, 27 Mar 2015 23:56:55 +0200 Message-ID: <20150327215655.GA29933__19155.2880140536$1427531091$gmane$org@sci.fi> References: <1426893517-2511-1-git-send-email-mcgrof@do-not-panic.com> <1426893517-2511-10-git-send-email-mcgrof@do-not-panic.com> <20150321091514.GA22926@sci.fi> <20150327193813.GH5622@wotan.suse.de> <20150327195759.GK5622@wotan.suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YbcFZ-0004LE-Rb for xen-devel@lists.xenproject.org; Fri, 27 Mar 2015 21:57:10 +0000 Content-Disposition: inline In-Reply-To: <20150327195759.GK5622@wotan.suse.de> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Luis R. Rodriguez" Cc: Linux Fbdev development list , Daniel Vetter , Jan Beulich , "H. Peter Anvin" , Suresh Siddha , X86 ML , Tomi Valkeinen , "xen-devel@lists.xenproject.org" , Ingo Molnar , Borislav Petkov , Jean-Christophe Plagniol-Villard , Antonino Daplas , Dave Airlie , Bjorn Helgaas , Thomas Gleixner , Ingo Molnar , Juergen Gross , "Luis R. Rodriguez" , "linux-kernel@vger.kernel.org" , Andy Lutomirski , venkatesh.pallipadi@intel.com, Linus Torvalds List-Id: xen-devel@lists.xenproject.org On Fri, Mar 27, 2015 at 08:57:59PM +0100, Luis R. Rodriguez wrote: > On Fri, Mar 27, 2015 at 12:43:55PM -0700, Andy Lutomirski wrote: > > On Fri, Mar 27, 2015 at 12:38 PM, Luis R. Rodriguez w= rote: > > > On Sat, Mar 21, 2015 at 11:15:14AM +0200, Ville Syrj=E4l=E4 wrote: > > >> On Fri, Mar 20, 2015 at 04:17:59PM -0700, Luis R. Rodriguez wrote: > > >> > 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 =3D -1; > > >> > - par->mtrr_reg =3D -1; > > >> > if (!nomtrr) { > > >> > - /* Cover the whole resource. */ > > >> > - par->mtrr_aper =3D mtrr_add(par->res_start, par->res_s= ize, > > >> > + par->mtrr_aper =3D mtrr_add(info->fix.smem_start, > > >> > + info->fix.smem_len, > > >> > MTRR_TYPE_WRCOMB, 1); > > >> > > >> MTRRs need power of two size, so how is this supposed to work? > > > > > > As per mtrr_add_page() [0] the base and size are just supposed to be = in units > > > of 4 KiB, although the practice is to use powers of 2 in *some* drive= rs this > > > is not standardized and by no means recorded as a requirement. Obviou= sly > > > powers of 2 will work too and you'd end up neatly aligned as well. mt= rr_add() > > > will use mtrr_check() to verify the the same requirement. Furthermore, > > > as per my commit log message: > > = > > Whatever the code may or may not do, the x86 architecture uses > > power-of-two MTRR sizes. So I'm confused. > = > There should be no confusion, I simply did not know that *was* the > requirement for x86, if that is the case we should add a check for that > and perhaps generalize a helper that does the power of two helper changes, > the cleanest I found was the vesafb driver solution. > = > Thoughts? The vesafb solution is bad since you'll only end up covering only the first 4MB of the framebuffer instead of the almost 8MB you want. Which in practice will mean throwing away half the VRAM since you really don't want the massive performance hit from accessing it as UC. And that would mean giving up decent display resolutions as well :( And the other option of trying to cover the remainder with multiple ever smaller MTRRs doesn't work either since you'll run out of MTRRs very quickly. This is precisely why I used the hole method in atyfb in the first place. I don't really like the idea of any new mtrr code not supporting that use case, especially as these things tend to be present in older machines where PAT isn't an option. -- = Ville Syrj=E4l=E4 syrjala@sci.fi http://www.sci.fi/~syrjala/