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

On Fri, Mar 27, 2015 at 03:02:10PM -0700, Andy Lutomirski wrote:

On Fri, Mar 27, 2015 at 2:56 PM, Ville Syrjälä [off-list ref] wrote:

quoted

On Fri, Mar 27, 2015 at 08:57:59PM +0100, Luis R. Rodriguez wrote:

quoted

On Fri, Mar 27, 2015 at 12:43:55PM -0700, Andy Lutomirski wrote:

quoted

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

quoted

On Sat, Mar 21, 2015 at 11:15:14AM +0200, Ville Syrjälä wrote:

quoted

On Fri, Mar 20, 2015 at 04:17:59PM -0700, Luis R. Rodriguez 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);

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.

OK I think I get it now.

And I take it this would hopefully only be used for non-PAT systems?
Would there be a use case for PAT systems? I wonder if we can wrap
this under some APIs to make it clean and hide this dirty thing
behind the scenes, it seems a fragile and error prone and my hope
would be that we won't need more specialization in this area for
PAT systems.

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

Thanks for the clarification.

 Luis