* Re: [PATCH v1 06/47] mtrr: add __arch_phys_wc_add()
From: Andy Lutomirski @ 2015-03-27 21:23 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Bjorn Helgaas, Ville Syrjälä, Mauro Carvalho Chehab,
Mike Marciniszyn, Luis R. Rodriguez, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, Juergen Gross, Jan Beulich, Borislav Petkov,
Suresh Siddha, venkatesh.pallipadi, Dave Airlie,
linux-kernel@vger.kernel.org, Linux Fbdev development list,
X86 ML, xen-devel@lists.xenproject.org, Ingo Molnar,
Daniel Vetter, Antonino Daplas, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen
In-Reply-To: <20150327203029.GM5622@wotan.suse.de>
On Fri, Mar 27, 2015 at 1:30 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Fri, Mar 27, 2015 at 12:58:02PM -0700, Andy Lutomirski wrote:
>> On Fri, Mar 27, 2015 at 12:53 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> > On Fri, Mar 20, 2015 at 04:48:46PM -0700, Andy Lutomirski wrote:
>> >> On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
>> >> <mcgrof@do-not-panic.com> wrote:
>> >> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
>> >> >
>> >> > Ideally on systems using PAT we can expect a swift
>> >> > transition away from MTRR. There can be a few exceptions
>> >> > to this, one is where device drivers are known to exist
>> >> > on PATs with errata, another situation is observed on
>> >> > old device drivers where devices had combined MMIO
>> >> > register access with whatever area they typically
>> >> > later wanted to end up using MTRR for on the same
>> >> > PCI BAR. This situation can still be addressed by
>> >> > splitting up ioremap'd PCI BAR into two ioremap'd
>> >> > calls, one for MMIO registers, and another for whatever
>> >> > is desirable for write-combining -- in order to
>> >> > accomplish this though quite a bit of driver
>> >> > restructuring is required.
>> >> >
>> >> > Device drivers which are known to require large
>> >> > amount of re-work in order to split ioremap'd areas
>> >> > can use __arch_phys_wc_add() to avoid regressions
>> >> > when PAT is enabled.
>> >> >
>> >> > For a good example driver where things are neatly
>> >> > split up on a PCI BAR refer the infiniband qib
>> >> > driver. For a good example of a driver where good
>> >> > amount of work is required refer to the infiniband
>> >> > ipath driver.
>> >> >
>> >> > This is *only* a transitive API -- and as such no new
>> >> > drivers are ever expected to use this.
>> >>
>> >> What's the exact layout that this helps? I'm sceptical that this can
>> >> ever be correct.
>> >>
>> >> Is there some awful driver that has a large ioremap that's supposed to
>> >> contain multiple different memtypes?
>> >
>> > Yes, I cc'd you just now on one where I made changes on a driver which uses one
>> > PCI with mixed memtypes and uses MTRR to hole in WC. A transition to
>> > arch_phys_wc_add() is therefore not possible if PAT is enabled as it would
>> > regress those drivers by making the MTRR WC hole trick non functional.
>> > The changes are non trivial and so in this series I supplied changes on
>> > one driver only to show the effort required. The other drivers which
>> > required this were:
>> >
>> > Driver File
>> > ------------------------------------------------------------
>> > fusion drivers/message/fusion/mptbase.c
>> > ivtv drivers/media/pci/ivtv/ivtvfb.c
>> > ipath drivers/infiniband/hw/ipath/ipath_driver.c
>> >
>> > This series makes those drivers use __arch_phys_wc_add() more as a
>> > transitory phase in hopes we can address the proper split as with the
>> > atyfb illustrates. For ipath the changes required have a nice template
>> > with the qib driver as they share very similar driver structure, the
>> > qib driver *did* do the nice split.
>> >
>> >> If so, can we ioremap + set_page_xyz instead?
>> >
>> > I'm not sure I see which call we'd use. Care to provide an example patch
>> > alternative for the atyfb as a case in point alternative to the work required
>> > to do the split?
>> >
>>
>> I'm still confused. Would it be insufficient to ioremap_nocache the
>> whole thing and then call set_memory_wc on parts of it? (Sorry,
>> set_page_xyz was a typo.)
>
> I think that would be a sexy alternative.
>
> In this driver's case the thing is a bit messy as it not only used
> the WC MTRR for a hole but it also then used a UC MTRR on top of
> it all, so since I already tried to address the split, and if we address
> the power of 2 woes, I think it'd be best to try to remove the UC MTRR
> and just avoid set_page_wc() in this driver's case, but for the other cases
> (fusion, ivtv, ipath) I think this makes sense.
>
> Thoughts?
Once that WC MTRR is in place, I think you really need UC and not UC-
if you want to override it. Otherwise I agree with all of this.
--Andy
^ permalink raw reply
* Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around
From: Ville Syrjälä @ 2015-03-27 21:56 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Andy Lutomirski, Bjorn Helgaas, Luis R. Rodriguez, Ingo Molnar,
Thomas Gleixner, H. Peter Anvin, Juergen Gross, Jan Beulich,
Borislav Petkov, Suresh Siddha, venkatesh.pallipadi, Dave Airlie,
linux-kernel@vger.kernel.org, Linux Fbdev development list,
X86 ML, xen-devel@lists.xenproject.org, Ingo Molnar,
Linus Torvalds, Daniel Vetter, Antonino Daplas,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen
In-Reply-To: <20150327195759.GK5622@wotan.suse.de>
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.
--
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/
^ permalink raw reply
* Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around
From: Andy Lutomirski @ 2015-03-27 22:02 UTC (permalink / raw)
To: Ville Syrjälä, Luis R. Rodriguez, Andy Lutomirski,
Bjorn Helgaas, Luis R. Rodriguez, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, Juergen Gross, Jan Beulich, Borislav Petkov,
Suresh Siddha, venkatesh.pallipadi, Dave Airlie,
linux-kernel@vger.kernel.org, Linux Fbdev development list,
X86 ML, xen-devel@lists.xenproject.org, Ingo Molnar,
Linus Torvalds, Daniel Vetter, Antonino Daplas,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen
In-Reply-To: <20150327215655.GA29933@sci.fi>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v1 06/47] mtrr: add __arch_phys_wc_add()
From: Luis R. Rodriguez @ 2015-03-27 23:04 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Bjorn Helgaas, Ville Syrjälä, Mauro Carvalho Chehab,
Mike Marciniszyn, Luis R. Rodriguez, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, Juergen Gross, Jan Beulich, Borislav Petkov,
Suresh Siddha, venkatesh.pallipadi, Dave Airlie,
linux-kernel@vger.kernel.org, Linux Fbdev development list,
X86 ML, xen-devel@lists.xenproject.org, Ingo Molnar,
Daniel Vetter, Antonino Daplas, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen
In-Reply-To: <CALCETrXVL=5O2_LwAgO5vFZRdEpR8WbyRE5-C-NBEER4ZQrE8Q@mail.gmail.com>
On Fri, Mar 27, 2015 at 02:23:16PM -0700, Andy Lutomirski wrote:
> On Fri, Mar 27, 2015 at 1:30 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > On Fri, Mar 27, 2015 at 12:58:02PM -0700, Andy Lutomirski wrote:
> >> On Fri, Mar 27, 2015 at 12:53 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> >> > On Fri, Mar 20, 2015 at 04:48:46PM -0700, Andy Lutomirski wrote:
> >> >> On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
> >> >> <mcgrof@do-not-panic.com> wrote:
> >> >> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >> >> >
> >> >> > Ideally on systems using PAT we can expect a swift
> >> >> > transition away from MTRR. There can be a few exceptions
> >> >> > to this, one is where device drivers are known to exist
> >> >> > on PATs with errata, another situation is observed on
> >> >> > old device drivers where devices had combined MMIO
> >> >> > register access with whatever area they typically
> >> >> > later wanted to end up using MTRR for on the same
> >> >> > PCI BAR. This situation can still be addressed by
> >> >> > splitting up ioremap'd PCI BAR into two ioremap'd
> >> >> > calls, one for MMIO registers, and another for whatever
> >> >> > is desirable for write-combining -- in order to
> >> >> > accomplish this though quite a bit of driver
> >> >> > restructuring is required.
> >> >> >
> >> >> > Device drivers which are known to require large
> >> >> > amount of re-work in order to split ioremap'd areas
> >> >> > can use __arch_phys_wc_add() to avoid regressions
> >> >> > when PAT is enabled.
> >> >> >
> >> >> > For a good example driver where things are neatly
> >> >> > split up on a PCI BAR refer the infiniband qib
> >> >> > driver. For a good example of a driver where good
> >> >> > amount of work is required refer to the infiniband
> >> >> > ipath driver.
> >> >> >
> >> >> > This is *only* a transitive API -- and as such no new
> >> >> > drivers are ever expected to use this.
> >> >>
> >> >> What's the exact layout that this helps? I'm sceptical that this can
> >> >> ever be correct.
> >> >>
> >> >> Is there some awful driver that has a large ioremap that's supposed to
> >> >> contain multiple different memtypes?
> >> >
> >> > Yes, I cc'd you just now on one where I made changes on a driver which uses one
> >> > PCI with mixed memtypes and uses MTRR to hole in WC. A transition to
> >> > arch_phys_wc_add() is therefore not possible if PAT is enabled as it would
> >> > regress those drivers by making the MTRR WC hole trick non functional.
> >> > The changes are non trivial and so in this series I supplied changes on
> >> > one driver only to show the effort required. The other drivers which
> >> > required this were:
> >> >
> >> > Driver File
> >> > ------------------------------------------------------------
> >> > fusion drivers/message/fusion/mptbase.c
> >> > ivtv drivers/media/pci/ivtv/ivtvfb.c
> >> > ipath drivers/infiniband/hw/ipath/ipath_driver.c
> >> >
> >> > This series makes those drivers use __arch_phys_wc_add() more as a
> >> > transitory phase in hopes we can address the proper split as with the
> >> > atyfb illustrates. For ipath the changes required have a nice template
> >> > with the qib driver as they share very similar driver structure, the
> >> > qib driver *did* do the nice split.
> >> >
> >> >> If so, can we ioremap + set_page_xyz instead?
> >> >
> >> > I'm not sure I see which call we'd use. Care to provide an example patch
> >> > alternative for the atyfb as a case in point alternative to the work required
> >> > to do the split?
> >> >
> >>
> >> I'm still confused. Would it be insufficient to ioremap_nocache the
> >> whole thing and then call set_memory_wc on parts of it? (Sorry,
> >> set_page_xyz was a typo.)
> >
> > I think that would be a sexy alternative.
> >
> > In this driver's case the thing is a bit messy as it not only used
> > the WC MTRR for a hole but it also then used a UC MTRR on top of
> > it all, so since I already tried to address the split, and if we address
> > the power of 2 woes, I think it'd be best to try to remove the UC MTRR
> > and just avoid set_page_wc() in this driver's case, but for the other cases
> > (fusion, ivtv, ipath) I think this makes sense.
> >
> > Thoughts?
>
> Once that WC MTRR is in place, I think you really need UC and not UC-
> if you want to override it. Otherwise I agree with all of this.
Do you mean that the UC MTRR work around that was in place might not
have really been effective? Not quite sure what you mean. I don't think
I follow.
Luis
^ permalink raw reply
* Re: [PATCH v1 06/47] mtrr: add __arch_phys_wc_add()
From: Andy Lutomirski @ 2015-03-27 23:10 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Bjorn Helgaas, Ville Syrjälä, Mauro Carvalho Chehab,
Mike Marciniszyn, Luis R. Rodriguez, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, Juergen Gross, Jan Beulich, Borislav Petkov,
Suresh Siddha, venkatesh.pallipadi, Dave Airlie,
linux-kernel@vger.kernel.org, Linux Fbdev development list,
X86 ML, xen-devel@lists.xenproject.org, Ingo Molnar,
Daniel Vetter, Antonino Daplas, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen
In-Reply-To: <20150327230440.GO5622@wotan.suse.de>
On Fri, Mar 27, 2015 at 4:04 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Fri, Mar 27, 2015 at 02:23:16PM -0700, Andy Lutomirski wrote:
>> On Fri, Mar 27, 2015 at 1:30 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> > On Fri, Mar 27, 2015 at 12:58:02PM -0700, Andy Lutomirski wrote:
>> >> On Fri, Mar 27, 2015 at 12:53 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> >> > On Fri, Mar 20, 2015 at 04:48:46PM -0700, Andy Lutomirski wrote:
>> >> >> On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
>> >> >> <mcgrof@do-not-panic.com> wrote:
>> >> >> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
>> >> >> >
>> >> >> > Ideally on systems using PAT we can expect a swift
>> >> >> > transition away from MTRR. There can be a few exceptions
>> >> >> > to this, one is where device drivers are known to exist
>> >> >> > on PATs with errata, another situation is observed on
>> >> >> > old device drivers where devices had combined MMIO
>> >> >> > register access with whatever area they typically
>> >> >> > later wanted to end up using MTRR for on the same
>> >> >> > PCI BAR. This situation can still be addressed by
>> >> >> > splitting up ioremap'd PCI BAR into two ioremap'd
>> >> >> > calls, one for MMIO registers, and another for whatever
>> >> >> > is desirable for write-combining -- in order to
>> >> >> > accomplish this though quite a bit of driver
>> >> >> > restructuring is required.
>> >> >> >
>> >> >> > Device drivers which are known to require large
>> >> >> > amount of re-work in order to split ioremap'd areas
>> >> >> > can use __arch_phys_wc_add() to avoid regressions
>> >> >> > when PAT is enabled.
>> >> >> >
>> >> >> > For a good example driver where things are neatly
>> >> >> > split up on a PCI BAR refer the infiniband qib
>> >> >> > driver. For a good example of a driver where good
>> >> >> > amount of work is required refer to the infiniband
>> >> >> > ipath driver.
>> >> >> >
>> >> >> > This is *only* a transitive API -- and as such no new
>> >> >> > drivers are ever expected to use this.
>> >> >>
>> >> >> What's the exact layout that this helps? I'm sceptical that this can
>> >> >> ever be correct.
>> >> >>
>> >> >> Is there some awful driver that has a large ioremap that's supposed to
>> >> >> contain multiple different memtypes?
>> >> >
>> >> > Yes, I cc'd you just now on one where I made changes on a driver which uses one
>> >> > PCI with mixed memtypes and uses MTRR to hole in WC. A transition to
>> >> > arch_phys_wc_add() is therefore not possible if PAT is enabled as it would
>> >> > regress those drivers by making the MTRR WC hole trick non functional.
>> >> > The changes are non trivial and so in this series I supplied changes on
>> >> > one driver only to show the effort required. The other drivers which
>> >> > required this were:
>> >> >
>> >> > Driver File
>> >> > ------------------------------------------------------------
>> >> > fusion drivers/message/fusion/mptbase.c
>> >> > ivtv drivers/media/pci/ivtv/ivtvfb.c
>> >> > ipath drivers/infiniband/hw/ipath/ipath_driver.c
>> >> >
>> >> > This series makes those drivers use __arch_phys_wc_add() more as a
>> >> > transitory phase in hopes we can address the proper split as with the
>> >> > atyfb illustrates. For ipath the changes required have a nice template
>> >> > with the qib driver as they share very similar driver structure, the
>> >> > qib driver *did* do the nice split.
>> >> >
>> >> >> If so, can we ioremap + set_page_xyz instead?
>> >> >
>> >> > I'm not sure I see which call we'd use. Care to provide an example patch
>> >> > alternative for the atyfb as a case in point alternative to the work required
>> >> > to do the split?
>> >> >
>> >>
>> >> I'm still confused. Would it be insufficient to ioremap_nocache the
>> >> whole thing and then call set_memory_wc on parts of it? (Sorry,
>> >> set_page_xyz was a typo.)
>> >
>> > I think that would be a sexy alternative.
>> >
>> > In this driver's case the thing is a bit messy as it not only used
>> > the WC MTRR for a hole but it also then used a UC MTRR on top of
>> > it all, so since I already tried to address the split, and if we address
>> > the power of 2 woes, I think it'd be best to try to remove the UC MTRR
>> > and just avoid set_page_wc() in this driver's case, but for the other cases
>> > (fusion, ivtv, ipath) I think this makes sense.
>> >
>> > Thoughts?
>>
>> Once that WC MTRR is in place, I think you really need UC and not UC-
>> if you want to override it. Otherwise I agree with all of this.
>
> Do you mean that the UC MTRR work around that was in place might not
> have really been effective? Not quite sure what you mean. I don't think
> I follow.
I mean that the UC MTRR that overrides the WC MTRR was probably fine
(I hope smaller MTRRs override larger MTRRs). But we should just
ditch UC MTRRs entirely, and setting UC in the page tables would work
on all CPUs *if we supported that*. We'd need to add a couple trivial
helpers to do that.
--Andy
>
> Luis
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around
From: Luis R. Rodriguez @ 2015-03-27 23:31 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Luis R. Rodriguez, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
Juergen Gross, Jan Beulich, Borislav Petkov, Suresh Siddha,
venkatesh.pallipadi, Dave Airlie, linux-kernel@vger.kernel.org,
Linux Fbdev development list, X86 ML,
xen-devel@lists.xenproject.org, Ingo Molnar, Linus Torvalds,
Daniel Vetter, Antonino Daplas, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen
In-Reply-To: <CALCETrX9g6FzSnVmBow58_7cxrn21iCS+Xt8F+ThnJjukkyLmw@mail.gmail.com>
On Fri, Mar 27, 2015 at 02:21:34PM -0700, Andy Lutomirski wrote:
> On Fri, Mar 27, 2015 at 1:12 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > On Fri, Mar 20, 2015 at 04:52:18PM -0700, Andy Lutomirski wrote:
> >> On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
> >> <mcgrof@do-not-panic.com> 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);
> >> > - 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.
I believe there is a misunderstanding of order of changes.
Let's split when we use mtrr_add() Vs arch_phys_wc_add() to avoid
confusion as in this patch we don't yet use arch_phys_wc_add(). That
is done in the next patch.
The commit log describes best the state of affairs prior to this
patch:
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.
Then this patch drops the MTRR_TYPE_UNCACHABLE rewrite thing
and instead corrects the ioremap() call for the framebuffer
to only be called for the framebuffer alone. For the MMIO
area we adjust to use then ioremap_nocache(). The MTRR left
is now only *for the framebuffer* and it should not be touching
the MMIO area. So the MMIO area has its own ioremap_nocache()
area alone, the framebuffer is left with an ioremap() followed
by an mtrr_add() call.
The next patch replaces the mtrr_add() with arch_phys_wc_add()
and then also uses ioremap_wc().
> On a PAT system that has no effect and all is well.
Yeah we're not doing arch_phys_wc_add() on the entire PCI BAR.
That was dumb, this fixes that, and on this patch mtrr_add()
is still used.
> 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.
We don't need to solve that problem here as the MTRR should only
be for the framebuffer.
Luis
^ permalink raw reply
* Re: [PATCH v1 06/47] mtrr: add __arch_phys_wc_add()
From: Luis R. Rodriguez @ 2015-03-27 23:33 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Bjorn Helgaas, Ville Syrjälä, Mauro Carvalho Chehab,
Mike Marciniszyn, Luis R. Rodriguez, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, Juergen Gross, Jan Beulich, Borislav Petkov,
Suresh Siddha, venkatesh.pallipadi, Dave Airlie,
linux-kernel@vger.kernel.org, Linux Fbdev development list,
X86 ML, xen-devel@lists.xenproject.org, Ingo Molnar,
Daniel Vetter, Antonino Daplas, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen
In-Reply-To: <CALCETrXcMC7jA2jLo_SMXVaSq+KES7_EaPN-o5xRZ+xM1dVeJQ@mail.gmail.com>
On Fri, Mar 27, 2015 at 04:10:03PM -0700, Andy Lutomirski wrote:
> On Fri, Mar 27, 2015 at 4:04 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > On Fri, Mar 27, 2015 at 02:23:16PM -0700, Andy Lutomirski wrote:
> >> On Fri, Mar 27, 2015 at 1:30 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> >> > On Fri, Mar 27, 2015 at 12:58:02PM -0700, Andy Lutomirski wrote:
> >> >> On Fri, Mar 27, 2015 at 12:53 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> >> >> > On Fri, Mar 20, 2015 at 04:48:46PM -0700, Andy Lutomirski wrote:
> >> >> >> On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
> >> >> >> <mcgrof@do-not-panic.com> wrote:
> >> >> >> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >> >> >> >
> >> >> >> > Ideally on systems using PAT we can expect a swift
> >> >> >> > transition away from MTRR. There can be a few exceptions
> >> >> >> > to this, one is where device drivers are known to exist
> >> >> >> > on PATs with errata, another situation is observed on
> >> >> >> > old device drivers where devices had combined MMIO
> >> >> >> > register access with whatever area they typically
> >> >> >> > later wanted to end up using MTRR for on the same
> >> >> >> > PCI BAR. This situation can still be addressed by
> >> >> >> > splitting up ioremap'd PCI BAR into two ioremap'd
> >> >> >> > calls, one for MMIO registers, and another for whatever
> >> >> >> > is desirable for write-combining -- in order to
> >> >> >> > accomplish this though quite a bit of driver
> >> >> >> > restructuring is required.
> >> >> >> >
> >> >> >> > Device drivers which are known to require large
> >> >> >> > amount of re-work in order to split ioremap'd areas
> >> >> >> > can use __arch_phys_wc_add() to avoid regressions
> >> >> >> > when PAT is enabled.
> >> >> >> >
> >> >> >> > For a good example driver where things are neatly
> >> >> >> > split up on a PCI BAR refer the infiniband qib
> >> >> >> > driver. For a good example of a driver where good
> >> >> >> > amount of work is required refer to the infiniband
> >> >> >> > ipath driver.
> >> >> >> >
> >> >> >> > This is *only* a transitive API -- and as such no new
> >> >> >> > drivers are ever expected to use this.
> >> >> >>
> >> >> >> What's the exact layout that this helps? I'm sceptical that this can
> >> >> >> ever be correct.
> >> >> >>
> >> >> >> Is there some awful driver that has a large ioremap that's supposed to
> >> >> >> contain multiple different memtypes?
> >> >> >
> >> >> > Yes, I cc'd you just now on one where I made changes on a driver which uses one
> >> >> > PCI with mixed memtypes and uses MTRR to hole in WC. A transition to
> >> >> > arch_phys_wc_add() is therefore not possible if PAT is enabled as it would
> >> >> > regress those drivers by making the MTRR WC hole trick non functional.
> >> >> > The changes are non trivial and so in this series I supplied changes on
> >> >> > one driver only to show the effort required. The other drivers which
> >> >> > required this were:
> >> >> >
> >> >> > Driver File
> >> >> > ------------------------------------------------------------
> >> >> > fusion drivers/message/fusion/mptbase.c
> >> >> > ivtv drivers/media/pci/ivtv/ivtvfb.c
> >> >> > ipath drivers/infiniband/hw/ipath/ipath_driver.c
> >> >> >
> >> >> > This series makes those drivers use __arch_phys_wc_add() more as a
> >> >> > transitory phase in hopes we can address the proper split as with the
> >> >> > atyfb illustrates. For ipath the changes required have a nice template
> >> >> > with the qib driver as they share very similar driver structure, the
> >> >> > qib driver *did* do the nice split.
> >> >> >
> >> >> >> If so, can we ioremap + set_page_xyz instead?
> >> >> >
> >> >> > I'm not sure I see which call we'd use. Care to provide an example patch
> >> >> > alternative for the atyfb as a case in point alternative to the work required
> >> >> > to do the split?
> >> >> >
> >> >>
> >> >> I'm still confused. Would it be insufficient to ioremap_nocache the
> >> >> whole thing and then call set_memory_wc on parts of it? (Sorry,
> >> >> set_page_xyz was a typo.)
> >> >
> >> > I think that would be a sexy alternative.
> >> >
> >> > In this driver's case the thing is a bit messy as it not only used
> >> > the WC MTRR for a hole but it also then used a UC MTRR on top of
> >> > it all, so since I already tried to address the split, and if we address
> >> > the power of 2 woes, I think it'd be best to try to remove the UC MTRR
> >> > and just avoid set_page_wc() in this driver's case, but for the other cases
> >> > (fusion, ivtv, ipath) I think this makes sense.
> >> >
> >> > Thoughts?
> >>
> >> Once that WC MTRR is in place, I think you really need UC and not UC-
> >> if you want to override it. Otherwise I agree with all of this.
> >
> > Do you mean that the UC MTRR work around that was in place might not
> > have really been effective? Not quite sure what you mean. I don't think
> > I follow.
>
> I mean that the UC MTRR that overrides the WC MTRR was probably fine
> (I hope smaller MTRRs override larger MTRRs). But we should just
> ditch UC MTRRs entirely,
Agreed, this series does that, and this patch addresses the last
UC MTRR ;)
> and setting UC in the page tables would work on all CPUs *if we supported
> that*. We'd need to add a couple trivial helpers to do that.
OK please check my latest reply and if you do not mind clarify what you mean
there as I am not sure if we're on the same page (no pun) here. I don't quite
follow this last statement.
Luis
^ permalink raw reply
* Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
From: Luis R. Rodriguez @ 2015-03-27 23:56 UTC (permalink / raw)
To: Toshi Kani
Cc: Luis R. Rodriguez, luto, mingo, tglx, hpa, jgross, JBeulich, bp,
suresh.b.siddha, venkatesh.pallipadi, airlied, linux-kernel,
linux-fbdev, x86, xen-devel, Ingo Molnar, Daniel Vetter,
Antonino Daplas, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Dave Hansen, Stefan Bader, konrad.wilk, ville.syrjala,
david.vrabel, bhelgaas, Roger Pau Monné, xen-devel
In-Reply-To: <1427488817.31093.54.camel@misato.fc.hp.com>
On Fri, Mar 27, 2015 at 02:40:17PM -0600, Toshi Kani wrote:
> On Fri, 2015-03-20 at 16:17 -0700, Luis R. Rodriguez wrote:
> :
> > @@ -734,6 +742,7 @@ void __init mtrr_bp_init(void)
> > }
> >
> > if (mtrr_if) {
> > + mtrr_enabled = true;
> > set_num_var_ranges();
> > init_table();
> > if (use_intel()) {
> get_mtrr_state();
>
> After setting mtrr_enabled to true, get_mtrr_state() reads
> MSR_MTRRdefType and sets 'mtrr_state.enabled', which also indicates if
> MTRRs are enabled or not on the system. So, potentially, we could have
> a case that mtrr_enabled is set to true, but mtrr_state.enabled is set
> to disabled when MTRRs are disabled by BIOS.
Thanks for the review, in this case then we should update mtrr_enabled to false.
> ps.
> I recently cleaned up this part of the MTRR code in the patch below,
> which is currently available in the -mm & -next trees.
> https://lkml.org/lkml/2015/3/24/1063
Great I will rebase and work with that and try to address this
consideration you have raised.
Luis
^ permalink raw reply
* Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around
From: Luis R. Rodriguez @ 2015-03-28 0:21 UTC (permalink / raw)
To: Ville Syrjälä, Andy Lutomirski, Bjorn Helgaas,
Luis R. Rodriguez, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
Juergen Gross, Jan Beulich, Borislav Petkov, Suresh Siddha,
venkatesh.pallipadi, Dave Airlie, linux-kernel@vger.kernel.org,
Linux Fbdev development list, X86 ML,
xen-devel@lists.xenproject.org, Ingo Molnar, Linus Torvalds,
Daniel Vetter, Antonino Daplas, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen
In-Reply-To: <20150327215655.GA29933@sci.fi>
On Fri, Mar 27, 2015 at 11:56:55PM +0200, Ville Syrjälä 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.
OK so the power of 2 requirement implicates us *having* to use a large
MTRR that includes the MMIo region in the shared PCI case?
Andy, Ville, are we 100% certain about this power of two requirement?
Is that for the base and size or just the size?
Luis
^ permalink raw reply
* Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around
From: Luis R. Rodriguez @ 2015-03-28 0:28 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Ville Syrjälä, Bjorn Helgaas, Luis R. Rodriguez,
Ingo Molnar, Thomas Gleixner, H. Peter Anvin, Juergen Gross,
Jan Beulich, Borislav Petkov, Suresh Siddha, venkatesh.pallipadi,
Dave Airlie, linux-kernel@vger.kernel.org,
Linux Fbdev development list, X86 ML,
xen-devel@lists.xenproject.org, Ingo Molnar, Linus Torvalds,
Daniel Vetter, Antonino Daplas, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen
In-Reply-To: <CALCETrWW9WrE_4x6SctQQBe=zXp1yMQ1i=qtsACTnHV0_Ku2iQ@mail.gmail.com>
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ä <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.
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
^ permalink raw reply
* Re: [PATCH v2 1/3] vt: add cursor blink interval escape sequence
From: Mike Frysinger @ 2015-03-28 0:35 UTC (permalink / raw)
To: Scot Doyle
Cc: Greg Kroah-Hartman, Michael Kerrisk, Jiri Slaby, Tomi Valkeinen,
Jean-Christophe Plagniol-Villard, Pavel Machek,
Geert Uytterhoeven, linux-kernel, linux-fbdev, linux-man,
linux-api
In-Reply-To: <alpine.DEB.2.11.1503261352200.2411@local>
[-- Attachment #1: Type: text/plain, Size: 610 bytes --]
On 26 Mar 2015 13:54, Scot Doyle wrote:
> Add an escape sequence to specify the current console's cursor blink
> interval. The interval is specified as a number of milliseconds until
> the next cursor display state toggle, from 50 to 65535. /proc/loadavg
> did not show a difference with a one msec interval, but the lower
> bound is set to 50 msecs since slower hardware wasn't tested.
if they want to be crazy, why not let them ? it's not like we generally prevent
the user from destroying their machine. i.e. just require the value to be > 0
(unless you want to let 0 disable things).
-mike
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/3] vt: add cursor blink interval escape sequence
From: Pavel Machek @ 2015-03-28 7:50 UTC (permalink / raw)
To: Scot Doyle, Greg Kroah-Hartman, Michael Kerrisk, Jiri Slaby,
Tomi Valkeinen, Jean-Christophe Plagniol-Villard,
Geert Uytterhoeven, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-man-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150328003503.GB27135@vapier>
On Fri 2015-03-27 20:35:03, Mike Frysinger wrote:
> On 26 Mar 2015 13:54, Scot Doyle wrote:
> > Add an escape sequence to specify the current console's cursor blink
> > interval. The interval is specified as a number of milliseconds until
> > the next cursor display state toggle, from 50 to 65535. /proc/loadavg
> > did not show a difference with a one msec interval, but the lower
> > bound is set to 50 msecs since slower hardware wasn't tested.
>
> if they want to be crazy, why not let them ? it's not like we generally prevent
> the user from destroying their machine. i.e. just require the value to be > 0
> (unless you want to let 0 disable things).
Because anyone with access to console can do this, and we only allow root
users to destroy the machine.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: [PATCH v2 3/3] console_codes.4: Add CSI sequence for cursor blink interval
From: Pavel Machek @ 2015-03-28 7:51 UTC (permalink / raw)
To: Scot Doyle
Cc: Michael Kerrisk, Greg Kroah-Hartman, Jiri Slaby, Tomi Valkeinen,
Jean-Christophe Plagniol-Villard, Geert Uytterhoeven,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-man-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <alpine.DEB.2.11.1503261356490.2411@local>
On Thu 2015-03-26 13:57:44, Scot Doyle wrote:
> Add a Console Private CSI sequence to specify the current console's
> cursor blink interval. The interval is specified as a number of
> milliseconds until the next cursor display state toggle, from 50 to
> 65535.
>
> Signed-off-by: Scot Doyle <lkml14@scotdoyle.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: [PATCH v2 0/3] add cursor blink interval terminal escape sequence
From: Pavel Machek @ 2015-03-28 7:54 UTC (permalink / raw)
To: Scot Doyle
Cc: Greg Kroah-Hartman, Michael Kerrisk, Jiri Slaby, Tomi Valkeinen,
Jean-Christophe Plagniol-Villard, Geert Uytterhoeven,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-man-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <alpine.DEB.2.11.1503261341260.2164@local>
On Thu 2015-03-26 13:51:04, Scot Doyle wrote:
> v2: Add documentation to console_codes man page (man-pages repo)
>
> This patch series adds an escape sequence to specify the current console's
> cursor blink interval. The default interval is set to fbcon's currently
> hardcoded 200 msecs.
Actually... Greg, can you import console_codes.4 into kernel tree somehow?
This will bring documentation for a bunch of currently undocumented kernel
interfaces into the kernel tree... where it belongs.
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* [PATCH 1/2] staging: sm7xxfb: reserve PCI resource
From: Sudip Mukherjee @ 2015-03-28 9:15 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-fbdev, devel, linux-kernel, Sudip Mukherjee
before starting to access any address inside the PCI region we should
reserve the resource and release the resource when the module exits.
Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
drivers/staging/sm7xxfb/sm7xxfb.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/staging/sm7xxfb/sm7xxfb.c b/drivers/staging/sm7xxfb/sm7xxfb.c
index 5b3e614..149286e 100644
--- a/drivers/staging/sm7xxfb/sm7xxfb.c
+++ b/drivers/staging/sm7xxfb/sm7xxfb.c
@@ -776,6 +776,12 @@ static int smtcfb_pci_probe(struct pci_dev *pdev,
if (err)
return err;
+ err = pci_request_region(pdev, 0, "sm7xxfb");
+ if (err < 0) {
+ dev_err(&pdev->dev, "cannot reserve framebuffer region\n");
+ goto failed_regions;
+ }
+
sprintf(smtcfb_fix.id, "sm%Xfb", ent->device);
sfb = smtc_alloc_fb_info(pdev);
@@ -905,6 +911,9 @@ failed_fb:
smtc_free_fb_info(sfb);
failed_free:
+ pci_release_region(pdev, 0);
+
+failed_regions:
pci_disable_device(pdev);
return err;
@@ -933,6 +942,7 @@ static void smtcfb_pci_remove(struct pci_dev *pdev)
smtc_unmap_mmio(sfb);
unregister_framebuffer(&sfb->fb);
smtc_free_fb_info(sfb);
+ pci_release_region(pdev, 0);
}
#ifdef CONFIG_PM
--
1.8.1.2
^ permalink raw reply related
* [PATCH 2/2] staging: sm7xxfb: disable pci device
From: Sudip Mukherjee @ 2015-03-28 9:15 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-fbdev, devel, linux-kernel, Sudip Mukherjee
In-Reply-To: <1427533421-9751-1-git-send-email-sudipm.mukherjee@gmail.com>
disable the pci device when the module exits.
Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
drivers/staging/sm7xxfb/sm7xxfb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/staging/sm7xxfb/sm7xxfb.c b/drivers/staging/sm7xxfb/sm7xxfb.c
index 149286e..77f51a0 100644
--- a/drivers/staging/sm7xxfb/sm7xxfb.c
+++ b/drivers/staging/sm7xxfb/sm7xxfb.c
@@ -943,6 +943,7 @@ static void smtcfb_pci_remove(struct pci_dev *pdev)
unregister_framebuffer(&sfb->fb);
smtc_free_fb_info(sfb);
pci_release_region(pdev, 0);
+ pci_disable_device(pdev);
}
#ifdef CONFIG_PM
--
1.8.1.2
^ permalink raw reply related
* Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around
From: Ville Syrjälä @ 2015-03-28 12:23 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Andy Lutomirski, Bjorn Helgaas, Luis R. Rodriguez, Ingo Molnar,
Thomas Gleixner, H. Peter Anvin, Juergen Gross, Jan Beulich,
Borislav Petkov, Suresh Siddha, venkatesh.pallipadi, Dave Airlie,
linux-kernel@vger.kernel.org, Linux Fbdev development list,
X86 ML, xen-devel@lists.xenproject.org, Ingo Molnar,
Linus Torvalds, Daniel Vetter, Antonino Daplas,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen
In-Reply-To: <20150328002818.GT5622@wotan.suse.de>
On Sat, Mar 28, 2015 at 01:28:18AM +0100, Luis R. Rodriguez wrote:
> 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ä <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.
>
> 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.
One potential complication is kernel vs. userspace mmap. MTRR applies to
the physical address, but PAT applies to the virtual address, so with
the WC MTRR you get WC for userspace "for free" as well. Also the
userspace mmaps request will have the length of smem_len (at most), so
it won't be the nice power of two in that case.
Also on PAT systems w/o a BIOS provided WC MTRR, the fbdev mmap seems
to be total crap at the moment. IIRC I have a patch to fix things a bit...
From 4e6d70d223f35953c8a11a58cf3376a8a001fa4f Mon Sep 17 00:00:00 2001
From: Ville Syrjala <syrjala@sci.fi>
Date: Fri, 15 Apr 2011 04:02:43 +0300
Subject: [PATCH] fb: writecombine fb
---
drivers/video/fbdev/core/fbmem.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 0705d88..ecbde0e 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1396,6 +1396,7 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
unsigned long mmio_pgoff;
unsigned long start;
u32 len;
+ bool mmio = false;
if (!info)
return -ENODEV;
@@ -1426,11 +1427,20 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
vma->vm_pgoff -= mmio_pgoff;
start = info->fix.mmio_start;
len = info->fix.mmio_len;
+ mmio = true;
}
mutex_unlock(&info->mm_lock);
+ if (!mmio) {
+ vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
+ vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
+
+ if (!vm_iomap_memory(vma, start, len))
+ return 0;
+ }
+
vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
- fb_pgprotect(file, vma, start);
+ vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
return vm_iomap_memory(vma, start, len);
}
Perhaps it's time I tried to send that upstream properly :P
--
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/
^ permalink raw reply related
* [PATCH 70/86] fbdev/gxt4500: use uapi/linux/pci_ids.h directly
From: Michael S. Tsirkin @ 2015-03-29 13:42 UTC (permalink / raw)
To: linux-kernel
Cc: Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Laurent Pinchart, Daniel Vetter, Rob Clark, Jingoo Han,
linux-fbdev
In-Reply-To: <1427635734-24786-1-git-send-email-mst@redhat.com>
Header moved from linux/pci_ids.h to uapi/linux/pci_ids.h,
use the new header directly so we can drop
the wrapper in include/linux/pci_ids.h.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/video/fbdev/gxt4500.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/gxt4500.c b/drivers/video/fbdev/gxt4500.c
index 135d78a..2415d3d 100644
--- a/drivers/video/fbdev/gxt4500.c
+++ b/drivers/video/fbdev/gxt4500.c
@@ -10,7 +10,7 @@
#include <linux/fb.h>
#include <linux/console.h>
#include <linux/pci.h>
-#include <linux/pci_ids.h>
+#include <uapi/linux/pci_ids.h>
#include <linux/delay.h>
#include <linux/string.h>
--
MST
^ permalink raw reply related
* [PATCH 71/86] fbdev/i740fb: use uapi/linux/pci_ids.h directly
From: Michael S. Tsirkin @ 2015-03-29 13:42 UTC (permalink / raw)
To: linux-kernel
Cc: Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Bjorn Helgaas,
Laurent Pinchart, Benoit Taine, linux-fbdev
In-Reply-To: <1427635734-24786-1-git-send-email-mst@redhat.com>
Header moved from linux/pci_ids.h to uapi/linux/pci_ids.h,
use the new header directly so we can drop
the wrapper in include/linux/pci_ids.h.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/video/fbdev/i740fb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/i740fb.c b/drivers/video/fbdev/i740fb.c
index a2b4204..abad615 100644
--- a/drivers/video/fbdev/i740fb.c
+++ b/drivers/video/fbdev/i740fb.c
@@ -21,7 +21,7 @@
#include <linux/fb.h>
#include <linux/init.h>
#include <linux/pci.h>
-#include <linux/pci_ids.h>
+#include <uapi/linux/pci_ids.h>
#include <linux/i2c.h>
#include <linux/i2c-algo-bit.h>
#include <linux/console.h>
--
MST
^ permalink raw reply related
* [PATCH 72/86] fbdev/i810: use uapi/linux/pci_ids.h directly
From: Michael S. Tsirkin @ 2015-03-29 13:42 UTC (permalink / raw)
To: linux-kernel
Cc: Antonino Daplas, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
linux-fbdev
In-Reply-To: <1427635734-24786-1-git-send-email-mst@redhat.com>
Header moved from linux/pci_ids.h to uapi/linux/pci_ids.h,
use the new header directly so we can drop
the wrapper in include/linux/pci_ids.h.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/video/fbdev/i810/i810_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/i810/i810_main.c b/drivers/video/fbdev/i810/i810_main.c
index bb674e4..c4e13bb 100644
--- a/drivers/video/fbdev/i810/i810_main.c
+++ b/drivers/video/fbdev/i810/i810_main.c
@@ -37,7 +37,7 @@
#include <linux/fb.h>
#include <linux/init.h>
#include <linux/pci.h>
-#include <linux/pci_ids.h>
+#include <uapi/linux/pci_ids.h>
#include <linux/resource.h>
#include <linux/unistd.h>
#include <linux/console.h>
--
MST
^ permalink raw reply related
* [PATCH 73/86] fbdev/riva: use uapi/linux/pci_ids.h directly
From: Michael S. Tsirkin @ 2015-03-29 13:42 UTC (permalink / raw)
To: linux-kernel
Cc: Antonino Daplas, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
linux-fbdev
In-Reply-To: <1427635734-24786-1-git-send-email-mst@redhat.com>
Header moved from linux/pci_ids.h to uapi/linux/pci_ids.h,
use the new header directly so we can drop
the wrapper in include/linux/pci_ids.h.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/video/fbdev/riva/nv_driver.c | 2 +-
drivers/video/fbdev/riva/riva_hw.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/riva/nv_driver.c b/drivers/video/fbdev/riva/nv_driver.c
index f3694cf..f597ee5 100644
--- a/drivers/video/fbdev/riva/nv_driver.c
+++ b/drivers/video/fbdev/riva/nv_driver.c
@@ -36,7 +36,7 @@
#include <linux/delay.h>
#include <linux/pci.h>
-#include <linux/pci_ids.h>
+#include <uapi/linux/pci_ids.h>
#include "nv_type.h"
#include "rivafb.h"
#include "nvreg.h"
diff --git a/drivers/video/fbdev/riva/riva_hw.c b/drivers/video/fbdev/riva/riva_hw.c
index 8bdf37f..0d399f6 100644
--- a/drivers/video/fbdev/riva/riva_hw.c
+++ b/drivers/video/fbdev/riva/riva_hw.c
@@ -48,7 +48,7 @@
#include <linux/kernel.h>
#include <linux/pci.h>
-#include <linux/pci_ids.h>
+#include <uapi/linux/pci_ids.h>
#include "riva_hw.h"
#include "riva_tbl.h"
#include "nv_type.h"
--
MST
^ permalink raw reply related
* [PATCH] sisfb: Replace creative use of the ternary operator
From: Ben Hutchings @ 2015-03-29 22:28 UTC (permalink / raw)
To: linux-fbdev
[-- Attachment #1: Type: text/plain, Size: 3242 bytes --]
GETBITSTR() and related macros are defined in such a way that two of
the macro arguments must be pairs of integers joined with a ':',
selected between by prepending '1?' or '0?'. This is rather clever
but not very readable. Redefine the macros and change the users so
that each integer is a separate argument.
While we're at it, add a BUILD_BUG_ON_ZERO() to check that the
redundant first part of the 'to' argument is consistent with the
other arguments.
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
drivers/video/fbdev/sis/init.c | 32 +++++++++++++++-----------------
1 file changed, 15 insertions(+), 17 deletions(-)
diff --git a/drivers/video/fbdev/sis/init.c b/drivers/video/fbdev/sis/init.c
index dfe3eb7..d0c2e72 100644
--- a/drivers/video/fbdev/sis/init.c
+++ b/drivers/video/fbdev/sis/init.c
@@ -3320,11 +3320,9 @@ SiSSetMode(struct SiS_Private *SiS_Pr, unsigned short ModeNo)
return true;
}
-#ifndef GETBITSTR
-#define GENBITSMASK(mask) GENMASK(1?mask,0?mask)
-#define GETBITS(var,mask) (((var) & GENBITSMASK(mask)) >> (0?mask))
-#define GETBITSTR(val,from,to) ((GETBITS(val,from)) << (0?to))
-#endif
+#define GETBITSTR(val, fromhigh, fromlow, tohigh, tolow) \
+ (((((val) & GENMASK(fromhigh, fromlow)) >> (fromlow)) << (tolow)) + \
+ BUILD_BUG_ON_ZERO((fromhigh) - (fromlow) != (tohigh) - (tolow)))
void
SiS_CalcCRRegisters(struct SiS_Private *SiS_Pr, int depth)
@@ -3363,23 +3361,23 @@ SiS_CalcCRRegisters(struct SiS_Private *SiS_Pr, int depth)
SiS_Pr->CCRT1CRTC[12] = (SiS_Pr->CVBlankEnd - 1) & 0xFF; /* CR16 */
SiS_Pr->CCRT1CRTC[13] = /* SRA */
- GETBITSTR((SiS_Pr->CVTotal -2), 10:10, 0:0) |
- GETBITSTR((SiS_Pr->CVDisplay -1), 10:10, 1:1) |
- GETBITSTR((SiS_Pr->CVBlankStart-1), 10:10, 2:2) |
- GETBITSTR((SiS_Pr->CVSyncStart -x), 10:10, 3:3) |
- GETBITSTR((SiS_Pr->CVBlankEnd -1), 8:8, 4:4) |
- GETBITSTR((SiS_Pr->CVSyncEnd ), 4:4, 5:5) ;
+ GETBITSTR((SiS_Pr->CVTotal -2), 10, 10, 0, 0) |
+ GETBITSTR((SiS_Pr->CVDisplay -1), 10, 10, 1, 1) |
+ GETBITSTR((SiS_Pr->CVBlankStart-1), 10, 10, 2, 2) |
+ GETBITSTR((SiS_Pr->CVSyncStart -x), 10, 10, 3, 3) |
+ GETBITSTR((SiS_Pr->CVBlankEnd -1), 8, 8, 4, 4) |
+ GETBITSTR((SiS_Pr->CVSyncEnd ), 4, 4, 5, 5) ;
SiS_Pr->CCRT1CRTC[14] = /* SRB */
- GETBITSTR((SiS_Pr->CHTotal >> 3) - 5, 9:8, 1:0) |
- GETBITSTR((SiS_Pr->CHDisplay >> 3) - 1, 9:8, 3:2) |
- GETBITSTR((SiS_Pr->CHBlankStart >> 3) - 1, 9:8, 5:4) |
- GETBITSTR((SiS_Pr->CHSyncStart >> 3) + 3, 9:8, 7:6) ;
+ GETBITSTR((SiS_Pr->CHTotal >> 3) - 5, 9, 8, 1, 0) |
+ GETBITSTR((SiS_Pr->CHDisplay >> 3) - 1, 9, 8, 3, 2) |
+ GETBITSTR((SiS_Pr->CHBlankStart >> 3) - 1, 9, 8, 5, 4) |
+ GETBITSTR((SiS_Pr->CHSyncStart >> 3) + 3, 9, 8, 7, 6) ;
SiS_Pr->CCRT1CRTC[15] = /* SRC */
- GETBITSTR((SiS_Pr->CHBlankEnd >> 3) - 1, 7:6, 1:0) |
- GETBITSTR((SiS_Pr->CHSyncEnd >> 3) + 3, 5:5, 2:2) ;
+ GETBITSTR((SiS_Pr->CHBlankEnd >> 3) - 1, 7, 6, 1, 0) |
+ GETBITSTR((SiS_Pr->CHSyncEnd >> 3) + 3, 5, 5, 2, 2) ;
}
void
--
Ben Hutchings
Sturgeon's Law: Ninety percent of everything is crap.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply related
* Re: [PATCH 6/8] mtd: replace CONFIG_8xx by CONFIG_PPC_8xx
From: Brian Norris @ 2015-03-31 0:55 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-fbdev, linux-serial, user-mode-linux-devel, David Woodhouse,
linux-kernel, linux-mtd, user-mode-linux-user, netdev,
linuxppc-dev
In-Reply-To: <20150312152404.D6C391A2421@localhost.localdomain>
On Thu, Mar 12, 2015 at 04:24:04PM +0100, Christophe Leroy wrote:
> Two config options exist to define powerpc MPC8xx:
> * CONFIG_PPC_8xx
> * CONFIG_8xx
> In addition, CONFIG_PPC_8xx also defines CONFIG_CPM1 as
> communication co-processor
>
> arch/powerpc/platforms/Kconfig.cputype has contained the following
> comment about CONFIG_8xx item for some years:
> "# this is temp to handle compat with arch=ppc"
>
> It looks like not many places still have that old CONFIG_8xx used,
> so it is likely to be a good time to get rid of it completely ?
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
I'm not sure if the consistency aspect is worked out for all symbols
(Geert's comment on the cover letter), but this one looks fine.
Applied this patch to l2-mtd.git.
Brian
^ permalink raw reply
* Re: [RFT PATCHv2] drm/exynos: Enable DP clock to fix display on Exynos5250 and other
From: Andreas Färber @ 2015-03-31 11:42 UTC (permalink / raw)
To: Javier Martinez Canillas, Krzysztof Kozlowski, Jingoo Han,
Inki Dae, Joonyoung Shim, Seung-Woo Kim, Kyungmin Park,
David Airlie, Kukjin Kim, dri-devel, linux-arm-kernel,
linux-samsung-soc, linux-kernel, linux-fbdev
Cc: Marek Szyprowski, Andrzej Hajda, stable
In-Reply-To: <5515ADB6.6050204@collabora.co.uk>
Am 27.03.2015 um 20:21 schrieb Javier Martinez Canillas:
> Hello Krzysztof,
>
> On 03/27/2015 05:08 PM, Krzysztof Kozlowski wrote:
>> After adding display power domain for Exynos5250 in commit
>> 2d2c9a8d0a4f ("ARM: dts: add display power domain for exynos5250") the
>> display on Chromebook Snow and others stopped working after boot.
>>
>> The reason for this suggested Andrzej Hajda: the DP clock was disabled.
>> This clock is required by Display Port and is enabled by bootloader.
>> However when FIMD driver probing was deferred, the display power domain
>> was turned off. This effectively reset the value of DP clock enable
>> register.
>>
>> When exynos-dp is later probed, the clock is not enabled and display is
>> not properly configured:
>>
>> exynos-dp 145b0000.dp-controller: Timeout of video streamclk ok
>> exynos-dp 145b0000.dp-controller: unable to config video
>>
>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> Reported-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> Fixes: 2d2c9a8d0a4f ("ARM: dts: add display power domain for exynos5250")
>> Cc: <stable@vger.kernel.org>
>>
>> ---
>>
>> This should fix issue reported by Javier [1][2].
>>
>
> I tested this patch and does indeed solves both issues I reported
> The exynos-dp probe deferral does not make the display to not be
> working and also disabling and enabling the display with:
>
> with /sys/devices/platform/exynos-drm/graphics/fb0/blank works.
>
> Thanks a lot for fixing this issue.
>
> On an Exynos5250 Snow Chromebook:
>
> Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Seems to fix Spring Chromebook as well,
Tested-by: Andreas Färber <afaerber@suse.de>
Thanks a lot,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)
^ permalink raw reply
* [PATCHv6 00/10] Cleanup and add support for SSD1305
From: Thomas Niederprüm @ 2015-03-31 18:27 UTC (permalink / raw)
To: plagnioj, tomi.valkeinen, maxime.ripard, kernel, shawn.guo,
robh+dt
Cc: linux-fbdev, linux-kernel, Thomas Niederprüm
In-Reply-To: <1423261694-5939-1-git-send-email-niederp@physik.uni-kl.de>
Hi,
this patch series is the result of making the ssd1307fb driver work with
a Newhaven OLED display using the Solomon SSD1305 controller. To achieve
this the intialization code for the SSD1306 and the SSD1307 is merged
and based on DT configuration to reflect the various possible wirings
of the SSD130X controller (04/10). Based on these changes it is straight
forward to add support for the SSD1305 controller (06/10).
While working on the driver I realized that it was not possible to
correctly mmap the video memory from userspace since the address handed
to the userspace app is a logical one where it should be a physical one.
Patch 01/10 fixes this. Furthermore the memory reserved by kzalloc is
not page aligned while the address handed to userspace is aligned to the
next page frame. This problem is fixed by using __get_free_pages() in 02/10.
Furthermore a module parameter is added to set the delay for the
deferred io update (07/10). Also the backlight class is implemented to make
the contrast setting available in userspace (09/10).
changes since v1 (thanks to Maxime for the feedback):
- dedicated patch for fixing smem_start address
- remove page reserve upon vmalloc
- remove return value check upon display turn-off at module unload
- use a module parameter refreshrate rather than delaydivider
- allocate fbdefio dynamically
- use sysfs_create_groups to create sysfs entries
- remove contrast, vhcom and dclk properties from DT since they are
not part of hw description. The contrast module parameter was added
to set contrast at load time. vhcom and dclk stays at it's default
values for now.
- add new DT properties to in tree users of ssd130X
- rebased to apply on top of linux-next
changes since v2 (thanks to Maxime again):
- free memory allocated by vmalloc on driver unload
- set default values in the init code to the ones of the existing ssd1307
init code
- added two ACKs (Maxime Ripard)
changes since v3:
- use backlight class rather than dedicated sysfs files to set the
contrast (Thanks to Tomi Valkeinen)
- remove [PATCHv3 08/10] fbdev: ssd1307fb: Add module parameter bitsperpixel
- add new patch to blank the display (unreviewed)
- allocate video memory through __get_free_pages() rather than vmalloc
(Thanks to Geert Uytterhoeven)
- minor rewordings of the commit messages
changes since v4 (thanks to Maxime):
- added two ACKs (Maxime Ripard)
- fixed typo: s/REFRASHRATE/REFRESHRATE
- updated the documentation to make clear the unit of com-offset
- move addition of the module parameter contrast to a separate patch (09/11)
- fix indentation errors
- get rid of device_id in the device_info struct
changes since v5:
- remove [PATCHv5 09/11] fbdev: ssd1307fb: Add module parameter to set
the initial contrast
- remove unnecessary variable initialization in the deviceinfo struct as
pointed out by Maxime
- make the variable dclk_div hold the actual divider rather than the
corresponding bit value
- remove ret variable in ssd1307fb_blank() (thanks to Olliver Schinagl)
- fixed bit mask for com_invdir as indicated by Olliver
- change initialization of the charge pump as suggested by Olliver
- change the default value for prechargep2 to 0x2 (thanks to Olliver)
- change DT property name "com-sequential" to "com-seq"
- added one ACK (Maxime Ripard)
- added Tested-by Olliver Schinagl
Thomas Niederprüm (10):
fbdev: ssd1307fb: fix memory address smem_start.
fbdev: ssd1307fb: Allocate page aligned video memory.
of: Add Solomon Systech vendor prefix.
fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT
ARM: mxs: fix in tree users of ssd1306
fbdev: ssd1307fb: Add support for SSD1305
fbdev: ssd1307fb: Add a module parameter to set the refresh rate
fbdev: ssd1307fb: Turn off display on driver unload.
fbdev: ssd1307fb: add backlight controls for setting the contrast
fbdev: ssd1307fb: Add blank mode
.../devicetree/bindings/vendor-prefixes.txt | 1 +
.../devicetree/bindings/video/ssd1307fb.txt | 23 +-
arch/arm/boot/dts/imx28-cfa10036.dts | 3 +
drivers/video/fbdev/Kconfig | 1 +
drivers/video/fbdev/ssd1307fb.c | 288 +++++++++++++++------
5 files changed, 234 insertions(+), 82 deletions(-)
--
2.3.0
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox