lkml.org 
[lkml]   [2015]   [Mar]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around
    On Fri, Mar 27, 2015 at 2:56 PM, Ville Syrjälä <syrjala@sci.fi> wrote:
    > 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 <mcgrof@suse.com> wrote:
    >> > > On Sat, Mar 21, 2015 at 11:15:14AM +0200, Ville Syrjälä 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 = -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);
    >> > >>
    >> > >> 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* drivers this
    >> > > is not standardized and by no means recorded as a requirement. Obviously
    >> > > powers of 2 will work too and you'd end up neatly aligned as well. mtrr_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.

    According to the Intel SDM, volume 3, section 11.5.2.1, table 11-6,
    non-PAT CPUs that have a WC MTRR, PCD = 1, and PWT = 1 (aka UC) have
    an effective memory type of UC. Hence my suggestion to add
    ioremap_x86_uc and/or set_memory_x86_uc to punch a UC hole in an
    otherwise WC MTRR-covered region.

    ioremap_nocache is UC- (even on non-PAT unless I misunderstood how
    this stuff works), so ioremap_nocache by itself isn't good enough.

    --Andy


    \
     
     \ /
      Last update: 2015-03-27 23:41    [W:3.760 / U:0.196 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site