* Re: [PATCH v4 2/2] IB/qib: use arch_phys_wc_add()
From: Doug Ledford @ 2015-04-22 16:57 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Luis R. Rodriguez, infinipath-ral2JQCrhuEAvxtiuMwx3w,
roland-DgEjT+Ai2ygdnm+yROfE0A, sean.hefty-ral2JQCrhuEAvxtiuMwx3w,
hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, luto-kltTT9wpgjJwATOyAt5JVQ,
mst-H+wXaHxf7aLQT0dZR+AlfA, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
cocci-/FJkirnvOdkvYVN+rsErww, Toshi Kani, Rickard Strandqvist,
Mike Marciniszyn, Roland Dreier, Dennis Dalessandro,
Suresh Siddha, Ingo Molnar, Thomas Gleixner, Juergen Gross,
Daniel Vetter, Dave Airlie, Bjorn Helgaas, Antonino Daplas,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen
In-Reply-To: <20150422153348.GD5622-B4tOwbsTzaBolqkO4TVVkw@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]
On Wed, 2015-04-22 at 17:33 +0200, Luis R. Rodriguez wrote:
> On Wed, Apr 22, 2015 at 09:54:38AM -0400, Doug Ledford wrote:
> > On Tue, 2015-04-21 at 14:50 -0700, Luis R. Rodriguez wrote:
> >
> > This:
> > > + /* MTRR was used if this is non-zero */
> > > + if (!dd->wc_cookie)
> > > vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
> >
> > And this:
> > > + dd->wc_cookie = arch_phys_wc_add(pioaddr, piolen);
> > > + if (dd->wc_cookie < 0)
> > > + ret = -EINVAL;
> >
> > don't agree on what wc_cookie will be on error.
>
> Can you elaborate? The one below is the one that starts things,
> and arch_phys_wc_add() will return 0 on PAT systems. For non-PAT
> systems it will return a number > 0 *iff* a valid MTRR was added.
> It will return negative onloy on error then.
>
> The change above is meant to replace a check put in place to see
> if PAT was enabled. The way we replace this is to ensure that
> arch_phys_wc_add() returned 0.
>
> If you disagree it'd be great if you can elaborate why.
Maybe I'm missing something, but in qib_enable_wc() you store the return
from arch_phys_wc_add into wc_cookie. That return is negative, so you
return from qib_enable_wc() to qib_init_one(), they see the ret value,
they print out a warning about bad performance, then they clear the
return value and continue with device initialization.
In all of this though, wc_cookie is never cleared and so it still has
the error condition in it. Then, much later at run time, you call
mmap_piobufs() and you check the contents of wc_cookie, and if it's
non-0 (which is still will be), you do the wrong thing, right? And what
about at shutdown when you call qib_disable_wc() and your cookie still
has an error code in it as well?
--
Doug Ledford <dledford@redhat.com>
GPG KeyID: 0E572FDD
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Andy Lutomirski @ 2015-04-22 16:53 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Jason Gunthorpe, Mike Marciniszyn, Mike Marciniszyn, linux-rdma,
Andy Walls, Toshi Kani, H. Peter Anvin, Ingo Molnar,
linux-kernel@vger.kernel.org, Hal Rosenstock, Sean Hefty,
Suresh Siddha, Rickard Strandqvist, Roland Dreier, Juergen Gross,
Mauro Carvalho Chehab, Borislav Petkov, Mel Gorman,
Vlastimil Babka, Davidlohr Bueso, Dave Hansen,
Jean-Christophe Plagniol-Villard, Thomas Gleixner, Ville Syrj?l?,
Linux Fbdev development list, linux-media, X86 ML,
Luis R. Rodriguez
In-Reply-To: <20150422152328.GB5622@wotan.suse.de>
On Wed, Apr 22, 2015 at 8:23 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Tue, Apr 21, 2015 at 11:39:39PM -0600, Jason Gunthorpe wrote:
>> On Wed, Apr 22, 2015 at 01:39:07AM +0200, Luis R. Rodriguez wrote:
>> > > Mike, do you think the time is right to just remove the iPath driver?
>> >
>> > With PAT now being default the driver effectively won't work
>> > with write-combining on modern kernels. Even if systems are old
>> > they likely had PAT support, when upgrading kernels PAT will work
>> > but write-combing won't on ipath.
>>
>> Sorry, do you mean the driver already doesn't get WC? Or do you mean
>> after some more pending patches are applied?
>
> No, you have to consider the system used and the effects of calls used
> on the driver in light of this table:
>
> ----------------------------------------------------------------------
> MTRR Non-PAT PAT Linux ioremap value Effective memory type
> ----------------------------------------------------------------------
> Non-PAT | PAT
> PAT
> |PCD
> ||PWT
> |||
> WC 000 WB _PAGE_CACHE_MODE_WB WC | WC
> WC 001 WC _PAGE_CACHE_MODE_WC WC* | WC
> WC 010 UC- _PAGE_CACHE_MODE_UC_MINUS WC* | UC
> WC 011 UC _PAGE_CACHE_MODE_UC UC | UC
> ----------------------------------------------------------------------
>
> (*) denotes implementation defined and is discouraged
>
> ioremap_nocache() will use _PAGE_CACHE_MODE_UC_MINUS by default today,
> in the future we want to flip the switch and make _PAGE_CACHE_MODE_UC
> the default. When that flip occurs it will mean ipath cannot get
> write-combining on both non-PAT and PAT systems. Now that is for
> the future, lets review the current situation for ipath.
>
> For PAT capable systems if mtrr_add() is used today on a Linux system on a
> region mapped with ioremap_nocache() that will mean you effectively nullify the
> mtrr_add() effect as the combinatorial effect above yields an effective memory
> type of UC.
Are you sure? I thought that ioremap_nocache currently is UC-, so
mtrr_add + ioremap_nocache gets WC even on PAT systems.
Going forward, when mtrr_add is gone, this will change, of course.
--Andy
^ permalink raw reply
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Luis R. Rodriguez @ 2015-04-22 16:51 UTC (permalink / raw)
To: Jason Gunthorpe, Juergen Gross, Andy Walls
Cc: Andy Lutomirski, mike.marciniszyn, infinipath, linux-rdma,
Toshi Kani, H. Peter Anvin, Ingo Molnar,
linux-kernel@vger.kernel.org, Hal Rosenstock, Sean Hefty,
Suresh Siddha, Rickard Strandqvist, Roland Dreier,
Mauro Carvalho Chehab, Borislav Petkov, Mel Gorman,
Vlastimil Babka, Davidlohr Bueso, Dave Hansen,
Jean-Christophe Plagniol-Villard, Thomas Gleixner, Ville Syrj?l?,
Linux Fbdev development list, linux-media, X86 ML, mcgrof
In-Reply-To: <20150422161755.GA19500@obsidianresearch.com>
On Wed, Apr 22, 2015 at 10:17:55AM -0600, Jason Gunthorpe wrote:
> On Wed, Apr 22, 2015 at 05:23:28PM +0200, Luis R. Rodriguez wrote:
> > On Tue, Apr 21, 2015 at 11:39:39PM -0600, Jason Gunthorpe wrote:
> > > On Wed, Apr 22, 2015 at 01:39:07AM +0200, Luis R. Rodriguez wrote:
> > > > > Mike, do you think the time is right to just remove the iPath driver?
> > > >
> > > > With PAT now being default the driver effectively won't work
> > > > with write-combining on modern kernels. Even if systems are old
> > > > they likely had PAT support, when upgrading kernels PAT will work
> > > > but write-combing won't on ipath.
> > >
> > > Sorry, do you mean the driver already doesn't get WC? Or do you mean
> > > after some more pending patches are applied?
> >
> > No, you have to consider the system used and the effects of calls used
> > on the driver in light of this table:
>
> So, just to be clear:
>
> At some point Linux started setting the PAT bits during
> ioremap_nocache, which overrides MTRR, and at that point the driver
> became broken on all PAT capable systems?
No, PAT code lacked quite a bit of love, and Juergen and some others have
been giving it some love and now we expect PAT to be enabled by default on
more systems. When and and on what systemes and as of what commits? Not
sure, there's quite a bit of PAT work but hoping Juergen might fill
in the details, CC'd.
> Not only that, but we've only just noticed it now, and no user ever
> complained?
No, well this is all recent, so we expect more PAT enabled systems now.
> So that means either no users exist, or all users are on non-PAT
> systems?
PAT was not being enabled before on most systems, it will now be on
most systems, for some systems there may be some errata that needs to
be addressed for PAT.
> This driver only works on x86-64 systems. Are there any x86-64 systems
> that are not PAT capable?
Not that I know of, but I've heard of some "PAT errata" systems, and
that may need some attention.
> IIRC even the first Opteron had PAT, but my
> memory is fuzzy from back then :|
>
> > Another option in order to enable this type of checks at run time
> > and still be able to build the driver on standard distributions and
> > just prevent if from loading on PAT systems is to have some code in
> > place which would prevent the driver from loading if PAT was
> > enabled, this would enable folks to disable PAT via a kernel command
> > line option, and if that was used then the driver probe would
> > complete.
>
> This seems like a reasonble option to me. At the very least we might
> learn if anyone is still using these cards.
OK great.
> I'd also love to remove the driver if it turns out there are actually
> no users. qib substantially replaces it except for a few very old
> cards.
>
> Mike?
By replacing do you mean same hardware? BTW below are the changes
which I describe above which would prevent both ipath and ivtv
to load on PAT enabled systems. I think its a reasonable compromise.
If this is OK I can proceed with a split of the patches and move
on with the last series that burries MTRR.
Andy Walls, please review too.
Luis
diff --git a/drivers/infiniband/hw/ipath/ipath_driver.c b/drivers/infiniband/hw/ipath/ipath_driver.c
index bd0caed..3ef592c 100644
--- a/drivers/infiniband/hw/ipath/ipath_driver.c
+++ b/drivers/infiniband/hw/ipath/ipath_driver.c
@@ -42,6 +42,9 @@
#include <linux/bitmap.h>
#include <linux/slab.h>
#include <linux/module.h>
+#ifdef CONFIG_X86_64
+#include <asm/pat.h>
+#endif
#include "ipath_kernel.h"
#include "ipath_verbs.h"
@@ -395,6 +398,14 @@ static int ipath_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
unsigned long long addr;
u32 bar0 = 0, bar1 = 0;
+#ifdef CONFIG_X86_64
+ if (WARN(pat_enabled,
+ "ipath needs PAT disabled, boot with nopat kernel parameter\n")) {
+ ret = EINVAL;
+ goto bail;
+ }
+#endif
+
dd = ipath_alloc_devdata(pdev);
if (IS_ERR(dd)) {
ret = PTR_ERR(dd);
@@ -542,6 +553,7 @@ static int ipath_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
dd->ipath_kregbase = __ioremap(addr, len,
(_PAGE_NO_CACHE|_PAGE_WRITETHRU));
#else
+ /* XXX: split this properly to enable on PAT */
dd->ipath_kregbase = ioremap_nocache(addr, len);
#endif
@@ -587,12 +599,8 @@ static int ipath_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
ret = ipath_enable_wc(dd);
- if (ret) {
- ipath_dev_err(dd, "Write combining not enabled "
- "(err %d): performance may be poor\n",
- -ret);
+ if (ret)
ret = 0;
- }
ipath_verify_pioperf(dd);
diff --git a/drivers/infiniband/hw/ipath/ipath_kernel.h b/drivers/infiniband/hw/ipath/ipath_kernel.h
index e08db70..f0f9471 100644
--- a/drivers/infiniband/hw/ipath/ipath_kernel.h
+++ b/drivers/infiniband/hw/ipath/ipath_kernel.h
@@ -463,9 +463,7 @@ struct ipath_devdata {
/* offset in HT config space of slave/primary interface block */
u8 ipath_ht_slave_off;
/* for write combining settings */
- unsigned long ipath_wc_cookie;
- unsigned long ipath_wc_base;
- unsigned long ipath_wc_len;
+ int wc_cookie;
/* ref count for each pkey */
atomic_t ipath_pkeyrefs[4];
/* shadow copy of struct page *'s for exp tid pages */
diff --git a/drivers/infiniband/hw/ipath/ipath_wc_x86_64.c b/drivers/infiniband/hw/ipath/ipath_wc_x86_64.c
index 70c1f3a..7b6e4c8 100644
--- a/drivers/infiniband/hw/ipath/ipath_wc_x86_64.c
+++ b/drivers/infiniband/hw/ipath/ipath_wc_x86_64.c
@@ -37,7 +37,6 @@
*/
#include <linux/pci.h>
-#include <asm/mtrr.h>
#include <asm/processor.h>
#include "ipath_kernel.h"
@@ -122,27 +121,14 @@ int ipath_enable_wc(struct ipath_devdata *dd)
}
if (!ret) {
- int cookie;
- ipath_cdbg(VERBOSE, "Setting mtrr for chip to WC "
- "(addr %llx, len=0x%llx)\n",
- (unsigned long long) pioaddr,
- (unsigned long long) piolen);
- cookie = mtrr_add(pioaddr, piolen, MTRR_TYPE_WRCOMB, 1);
- if (cookie < 0) {
- {
- dev_info(&dd->pcidev->dev,
- "mtrr_add() WC for PIO bufs "
- "failed (%d)\n",
- cookie);
- ret = -EINVAL;
- }
- } else {
- ipath_cdbg(VERBOSE, "Set mtrr for chip to WC, "
- "cookie is %d\n", cookie);
- dd->ipath_wc_cookie = cookie;
- dd->ipath_wc_base = (unsigned long) pioaddr;
- dd->ipath_wc_len = (unsigned long) piolen;
- }
+ dd->wc_cookie = arch_phys_wc_add(pioaddr, piolen);
+ if (dd->wc_cookie < 0) {
+ ipath_dev_err(dd, "Seting mtrr failed on PIO buffers\n");
+ ret = -ENODEV;
+ } else if (dd->wc_cookie = 0)
+ ipath_cdbg(VERBOSE, "Set mtrr for chip to WC not needed\n");
+ else
+ ipath_cdbg(VERBOSE, "Set mtrr for chip to WC\n");
}
return ret;
@@ -154,16 +140,5 @@ int ipath_enable_wc(struct ipath_devdata *dd)
*/
void ipath_disable_wc(struct ipath_devdata *dd)
{
- if (dd->ipath_wc_cookie) {
- int r;
- ipath_cdbg(VERBOSE, "undoing WCCOMB on pio buffers\n");
- r = mtrr_del(dd->ipath_wc_cookie, dd->ipath_wc_base,
- dd->ipath_wc_len);
- if (r < 0)
- dev_info(&dd->pcidev->dev,
- "mtrr_del(%lx, %lx, %lx) failed: %d\n",
- dd->ipath_wc_cookie, dd->ipath_wc_base,
- dd->ipath_wc_len, r);
- dd->ipath_wc_cookie = 0; /* even on failure */
- }
+ arch_phys_wc_del(dd->wc_cookie);
}
diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
index 9ff1230..369598c 100644
--- a/drivers/media/pci/ivtv/ivtvfb.c
+++ b/drivers/media/pci/ivtv/ivtvfb.c
@@ -44,8 +44,8 @@
#include <linux/ivtvfb.h>
#include <linux/slab.h>
-#ifdef CONFIG_MTRR
-#include <asm/mtrr.h>
+#ifdef CONFIG_X86_64
+#include <asm/pat.h>
#endif
#include "ivtv-driver.h"
@@ -155,12 +155,11 @@ struct osd_info {
/* Buffer size */
u32 video_buffer_size;
-#ifdef CONFIG_MTRR
/* video_base rounded down as required by hardware MTRRs */
unsigned long fb_start_aligned_physaddr;
/* video_base rounded up as required by hardware MTRRs */
unsigned long fb_end_aligned_physaddr;
-#endif
+ int wc_cookie;
/* Store the buffer offset */
int set_osd_coords_x;
@@ -1099,6 +1098,8 @@ static int ivtvfb_init_vidmode(struct ivtv *itv)
static int ivtvfb_init_io(struct ivtv *itv)
{
struct osd_info *oi = itv->osd_info;
+ /* Find the largest power of two that maps the whole buffer */
+ int size_shift = 31;
mutex_lock(&itv->serialize_lock);
if (ivtv_init_on_first_open(itv)) {
@@ -1120,6 +1121,7 @@ static int ivtvfb_init_io(struct ivtv *itv)
oi->video_buffer_size = 1704960;
oi->video_pbase = itv->base_addr + IVTV_DECODER_OFFSET + oi->video_rbase;
+ /* XXX: split this for PAT */
oi->video_vbase = itv->dec_mem + oi->video_rbase;
if (!oi->video_vbase) {
@@ -1132,29 +1134,16 @@ static int ivtvfb_init_io(struct ivtv *itv)
oi->video_pbase, oi->video_vbase,
oi->video_buffer_size / 1024);
-#ifdef CONFIG_MTRR
- {
- /* Find the largest power of two that maps the whole buffer */
- int size_shift = 31;
-
- while (!(oi->video_buffer_size & (1 << size_shift))) {
- size_shift--;
- }
- size_shift++;
- oi->fb_start_aligned_physaddr = oi->video_pbase & ~((1 << size_shift) - 1);
- oi->fb_end_aligned_physaddr = oi->video_pbase + oi->video_buffer_size;
- oi->fb_end_aligned_physaddr += (1 << size_shift) - 1;
- oi->fb_end_aligned_physaddr &= ~((1 << size_shift) - 1);
- if (mtrr_add(oi->fb_start_aligned_physaddr,
- oi->fb_end_aligned_physaddr - oi->fb_start_aligned_physaddr,
- MTRR_TYPE_WRCOMB, 1) < 0) {
- IVTVFB_INFO("disabled mttr\n");
- oi->fb_start_aligned_physaddr = 0;
- oi->fb_end_aligned_physaddr = 0;
- }
- }
-#endif
-
+ while (!(oi->video_buffer_size & (1 << size_shift)))
+ size_shift--;
+ size_shift++;
+ oi->fb_start_aligned_physaddr = oi->video_pbase & ~((1 << size_shift) - 1);
+ oi->fb_end_aligned_physaddr = oi->video_pbase + oi->video_buffer_size;
+ oi->fb_end_aligned_physaddr += (1 << size_shift) - 1;
+ oi->fb_end_aligned_physaddr &= ~((1 << size_shift) - 1);
+ oi->wc_cookie = arch_phys_wc_add(oi->fb_start_aligned_physaddr,
+ oi->fb_end_aligned_physaddr -
+ oi->fb_start_aligned_physaddr);
/* Blank the entire osd. */
memset_io(oi->video_vbase, 0, oi->video_buffer_size);
@@ -1172,14 +1161,8 @@ static void ivtvfb_release_buffers (struct ivtv *itv)
/* Release pseudo palette */
kfree(oi->ivtvfb_info.pseudo_palette);
-
-#ifdef CONFIG_MTRR
- if (oi->fb_end_aligned_physaddr) {
- mtrr_del(-1, oi->fb_start_aligned_physaddr,
- oi->fb_end_aligned_physaddr - oi->fb_start_aligned_physaddr);
- }
-#endif
-
+ iounmap(oi->video_vbase);
+ arch_phys_wc_del(oi->wc_cookie);
kfree(oi);
itv->osd_info = NULL;
}
@@ -1190,6 +1173,13 @@ static int ivtvfb_init_card(struct ivtv *itv)
{
int rc;
+#ifdef CONFIG_X86_64
+ if (WARN(pat_enabled,
+ "ivtv needs PAT disabled, boot with nopat kernel parameter\n")) {
+ return EINVAL;
+ }
+#endif
+
if (itv->osd_info) {
IVTVFB_ERR("Card %d already initialised\n", ivtvfb_card_id);
return -EBUSY;
^ permalink raw reply related
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Jason Gunthorpe @ 2015-04-22 16:17 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Andy Lutomirski, mike.marciniszyn, infinipath, linux-rdma, awalls,
Toshi Kani, H. Peter Anvin, Ingo Molnar,
linux-kernel@vger.kernel.org, Hal Rosenstock, Sean Hefty,
Suresh Siddha, Rickard Strandqvist, Roland Dreier, Juergen Gross,
Mauro Carvalho Chehab, Borislav Petkov, Mel Gorman,
Vlastimil Babka, Davidlohr Bueso, Dave Hansen,
Jean-Christophe Plagniol-Villard, Thomas Gleixner, Ville Syrj?l?,
Linux Fbdev development list, linux-media, X86 ML, mcgrof
In-Reply-To: <20150422152328.GB5622@wotan.suse.de>
On Wed, Apr 22, 2015 at 05:23:28PM +0200, Luis R. Rodriguez wrote:
> On Tue, Apr 21, 2015 at 11:39:39PM -0600, Jason Gunthorpe wrote:
> > On Wed, Apr 22, 2015 at 01:39:07AM +0200, Luis R. Rodriguez wrote:
> > > > Mike, do you think the time is right to just remove the iPath driver?
> > >
> > > With PAT now being default the driver effectively won't work
> > > with write-combining on modern kernels. Even if systems are old
> > > they likely had PAT support, when upgrading kernels PAT will work
> > > but write-combing won't on ipath.
> >
> > Sorry, do you mean the driver already doesn't get WC? Or do you mean
> > after some more pending patches are applied?
>
> No, you have to consider the system used and the effects of calls used
> on the driver in light of this table:
So, just to be clear:
At some point Linux started setting the PAT bits during
ioremap_nocache, which overrides MTRR, and at that point the driver
became broken on all PAT capable systems?
Not only that, but we've only just noticed it now, and no user ever
complained?
So that means either no users exist, or all users are on non-PAT
systems?
This driver only works on x86-64 systems. Are there any x86-64 systems
that are not PAT capable? IIRC even the first Opteron had PAT, but my
memory is fuzzy from back then :|
> Another option in order to enable this type of checks at run time
> and still be able to build the driver on standard distributions and
> just prevent if from loading on PAT systems is to have some code in
> place which would prevent the driver from loading if PAT was
> enabled, this would enable folks to disable PAT via a kernel command
> line option, and if that was used then the driver probe would
> complete.
This seems like a reasonble option to me. At the very least we might
learn if anyone is still using these cards.
I'd also love to remove the driver if it turns out there are actually
no users. qib substantially replaces it except for a few very old
cards.
Mike?
Jason
^ permalink raw reply
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Luis R. Rodriguez @ 2015-04-22 15:59 UTC (permalink / raw)
To: Jason Gunthorpe, Andy Walls
Cc: Andy Lutomirski, Mike Marciniszyn, Mike Marciniszyn, linux-rdma,
Toshi Kani, H. Peter Anvin, Ingo Molnar,
linux-kernel@vger.kernel.org, Hal Rosenstock, Sean Hefty,
Suresh Siddha, Rickard Strandqvist, Roland Dreier, Juergen Gross,
Mauro Carvalho Chehab, Borislav Petkov, Mel Gorman,
Vlastimil Babka, Davidlohr Bueso, Dave Hansen,
Jean-Christophe Plagniol-Villard, Thomas Gleixner, Ville Syrj?l?,
Linux Fbdev development list, linux-media, X86 ML,
Luis R. Rodriguez
In-Reply-To: <20150422155446.GF5622@wotan.suse.de>
On Wed, Apr 22, 2015 at 8:54 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Wed, Apr 22, 2015 at 05:23:28PM +0200, Luis R. Rodriguez wrote:
>> On Tue, Apr 21, 2015 at 11:39:39PM -0600, Jason Gunthorpe wrote:
>> > On Wed, Apr 22, 2015 at 01:39:07AM +0200, Luis R. Rodriguez wrote:
>> > > > Mike, do you think the time is right to just remove the iPath driver?
>> > >
>> > > With PAT now being default the driver effectively won't work
>> > > with write-combining on modern kernels. Even if systems are old
>> > > they likely had PAT support, when upgrading kernels PAT will work
>> > > but write-combing won't on ipath.
>> >
>> > Sorry, do you mean the driver already doesn't get WC? Or do you mean
>> > after some more pending patches are applied?
>>
>> No, you have to consider the system used and the effects of calls used
>> on the driver in light of this table:
>>
>> ----------------------------------------------------------------------
>> MTRR Non-PAT PAT Linux ioremap value Effective memory type
>> ----------------------------------------------------------------------
>> Non-PAT | PAT
>> PAT
>> |PCD
>> ||PWT
>> |||
>> WC 000 WB _PAGE_CACHE_MODE_WB WC | WC
>> WC 001 WC _PAGE_CACHE_MODE_WC WC* | WC
>> WC 010 UC- _PAGE_CACHE_MODE_UC_MINUS WC* | UC
>> WC 011 UC _PAGE_CACHE_MODE_UC UC | UC
>> ----------------------------------------------------------------------
>>
>> (*) denotes implementation defined and is discouraged
>>
>> ioremap_nocache() will use _PAGE_CACHE_MODE_UC_MINUS by default today,
>> in the future we want to flip the switch and make _PAGE_CACHE_MODE_UC
>> the default. When that flip occurs it will mean ipath cannot get
>> write-combining on both non-PAT and PAT systems. Now that is for
>> the future, lets review the current situation for ipath.
>>
>> For PAT capable systems if mtrr_add() is used today on a Linux system on a
>> region mapped with ioremap_nocache() that will mean you effectively nullify the
>> mtrr_add() effect as the combinatorial effect above yields an effective memory
>> type of UC. For PAT systems you want to use ioremap_wc() on the region in
>> which you need write-combining followed by arch_phys_wc_add() which will *only*
>> call mtrr_add() *iff* PAT was not enabled. This also means we need to split
>> the ioremap'd areas so that the area that is using ioremap_nocache() can never
>> get write-combining (_PAGE_CACHE_MODE_UC). The ipath driver needs the regions
>> split just as was done for the qib driver.
>>
>> Now we could just say that leaving things as-is is a non-issue if you are OK
>> with non-write-combining effects being the default behaviour left on the ipath
>> driver for PAT systems. In that case we can just use arch_phys_wc_add() on the
>> driver and while it won't trigger the mtrr_add() on PAT systems it sill won't
>> have any effect. We just typically don't want to see use of ioremap_nocache()
>> paired with arch_phys_wc_add(), grammatically the correct thing to do is pair
>> ioremap_wc() areas with a arch_phys_wc_add() to make the write-combining effects
>> on non-PAT systems. If the ipath driver is not going to get he work required
>> to split the regions though perhaps we can live with a corner case driver that
>> annotates PAT must be disabled on the systems that use it and convert it to
>> arch_phys_wc_add() to just help with phasing out of direct use of mtrr_add().
>> With this strategy if and when ipath driver gets a split done it would gain WC
>> on both PAT and non-PAT.
>
> Folks, after some thought I do believe the above temporary strategy would
> avoid issues and would not have to stir people up to go and make code
> changes. We can use the same strategy for both ivtv and ipath:
>
> * Annotate via Kconfig for the driver that it depends on !X86_PAT
> that will ensure that PAT systems won't use it, and convert it
> to use arch_phys_wc_add() to help phase out direct access to mtrr_add()
>
> This would be correct given that the current situation on the driver
> makes write-combining non-effective on PAT systems, we in fact gain
> avoiding these type of use-cases, and annotate this as a big TODO item
> for folks who do want it for PAT systems.
>
> Thoughts?
Another option in order to enable this type of checks at run time and
still be able to build the driver on standard distributions and just
prevent if from loading on PAT systems is to have some code in place
which would prevent the driver from loading if PAT was enabled, this
would enable folks to disable PAT via a kernel command line option,
and if that was used then the driver probe would complete.
Luis
^ permalink raw reply
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Luis R. Rodriguez @ 2015-04-22 15:54 UTC (permalink / raw)
To: Jason Gunthorpe, Andy Walls
Cc: Andy Lutomirski, mike.marciniszyn, infinipath, linux-rdma,
Toshi Kani, H. Peter Anvin, Ingo Molnar,
linux-kernel@vger.kernel.org, Hal Rosenstock, Sean Hefty,
Suresh Siddha, Rickard Strandqvist, Roland Dreier, Juergen Gross,
Mauro Carvalho Chehab, Borislav Petkov, Mel Gorman,
Vlastimil Babka, Davidlohr Bueso, Dave Hansen,
Jean-Christophe Plagniol-Villard, Thomas Gleixner, Ville Syrj?l?,
Linux Fbdev development list, linux-media, X86 ML, mcgrof
In-Reply-To: <20150422152328.GB5622@wotan.suse.de>
On Wed, Apr 22, 2015 at 05:23:28PM +0200, Luis R. Rodriguez wrote:
> On Tue, Apr 21, 2015 at 11:39:39PM -0600, Jason Gunthorpe wrote:
> > On Wed, Apr 22, 2015 at 01:39:07AM +0200, Luis R. Rodriguez wrote:
> > > > Mike, do you think the time is right to just remove the iPath driver?
> > >
> > > With PAT now being default the driver effectively won't work
> > > with write-combining on modern kernels. Even if systems are old
> > > they likely had PAT support, when upgrading kernels PAT will work
> > > but write-combing won't on ipath.
> >
> > Sorry, do you mean the driver already doesn't get WC? Or do you mean
> > after some more pending patches are applied?
>
> No, you have to consider the system used and the effects of calls used
> on the driver in light of this table:
>
> ----------------------------------------------------------------------
> MTRR Non-PAT PAT Linux ioremap value Effective memory type
> ----------------------------------------------------------------------
> Non-PAT | PAT
> PAT
> |PCD
> ||PWT
> |||
> WC 000 WB _PAGE_CACHE_MODE_WB WC | WC
> WC 001 WC _PAGE_CACHE_MODE_WC WC* | WC
> WC 010 UC- _PAGE_CACHE_MODE_UC_MINUS WC* | UC
> WC 011 UC _PAGE_CACHE_MODE_UC UC | UC
> ----------------------------------------------------------------------
>
> (*) denotes implementation defined and is discouraged
>
> ioremap_nocache() will use _PAGE_CACHE_MODE_UC_MINUS by default today,
> in the future we want to flip the switch and make _PAGE_CACHE_MODE_UC
> the default. When that flip occurs it will mean ipath cannot get
> write-combining on both non-PAT and PAT systems. Now that is for
> the future, lets review the current situation for ipath.
>
> For PAT capable systems if mtrr_add() is used today on a Linux system on a
> region mapped with ioremap_nocache() that will mean you effectively nullify the
> mtrr_add() effect as the combinatorial effect above yields an effective memory
> type of UC. For PAT systems you want to use ioremap_wc() on the region in
> which you need write-combining followed by arch_phys_wc_add() which will *only*
> call mtrr_add() *iff* PAT was not enabled. This also means we need to split
> the ioremap'd areas so that the area that is using ioremap_nocache() can never
> get write-combining (_PAGE_CACHE_MODE_UC). The ipath driver needs the regions
> split just as was done for the qib driver.
>
> Now we could just say that leaving things as-is is a non-issue if you are OK
> with non-write-combining effects being the default behaviour left on the ipath
> driver for PAT systems. In that case we can just use arch_phys_wc_add() on the
> driver and while it won't trigger the mtrr_add() on PAT systems it sill won't
> have any effect. We just typically don't want to see use of ioremap_nocache()
> paired with arch_phys_wc_add(), grammatically the correct thing to do is pair
> ioremap_wc() areas with a arch_phys_wc_add() to make the write-combining effects
> on non-PAT systems. If the ipath driver is not going to get he work required
> to split the regions though perhaps we can live with a corner case driver that
> annotates PAT must be disabled on the systems that use it and convert it to
> arch_phys_wc_add() to just help with phasing out of direct use of mtrr_add().
> With this strategy if and when ipath driver gets a split done it would gain WC
> on both PAT and non-PAT.
Folks, after some thought I do believe the above temporary strategy would
avoid issues and would not have to stir people up to go and make code
changes. We can use the same strategy for both ivtv and ipath:
* Annotate via Kconfig for the driver that it depends on !X86_PAT
that will ensure that PAT systems won't use it, and convert it
to use arch_phys_wc_add() to help phase out direct access to mtrr_add()
This would be correct given that the current situation on the driver
makes write-combining non-effective on PAT systems, we in fact gain
avoiding these type of use-cases, and annotate this as a big TODO item
for folks who do want it for PAT systems.
Thoughts?
Luis
^ permalink raw reply
* Re: [PATCH v4 2/2] IB/qib: use arch_phys_wc_add()
From: Luis R. Rodriguez @ 2015-04-22 15:33 UTC (permalink / raw)
To: Doug Ledford
Cc: Luis R. Rodriguez, infinipath-ral2JQCrhuEAvxtiuMwx3w,
roland-DgEjT+Ai2ygdnm+yROfE0A, sean.hefty-ral2JQCrhuEAvxtiuMwx3w,
hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, luto-kltTT9wpgjJwATOyAt5JVQ,
mst-H+wXaHxf7aLQT0dZR+AlfA, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
cocci-/FJkirnvOdkvYVN+rsErww, Toshi Kani, Rickard Strandqvist,
Mike Marciniszyn, Roland Dreier, Dennis Dalessandro,
Suresh Siddha, Ingo Molnar, Thomas Gleixner, Juergen Gross,
Daniel Vetter, Dave Airlie, Bjorn Helgaas, Antonino Daplas,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen
In-Reply-To: <1429710878.45956.94.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Wed, Apr 22, 2015 at 09:54:38AM -0400, Doug Ledford wrote:
> On Tue, 2015-04-21 at 14:50 -0700, Luis R. Rodriguez wrote:
>
> This:
> > + /* MTRR was used if this is non-zero */
> > + if (!dd->wc_cookie)
> > vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
>
> And this:
> > + dd->wc_cookie = arch_phys_wc_add(pioaddr, piolen);
> > + if (dd->wc_cookie < 0)
> > + ret = -EINVAL;
>
> don't agree on what wc_cookie will be on error.
Can you elaborate? The one below is the one that starts things,
and arch_phys_wc_add() will return 0 on PAT systems. For non-PAT
systems it will return a number > 0 *iff* a valid MTRR was added.
It will return negative onloy on error then.
The change above is meant to replace a check put in place to see
if PAT was enabled. The way we replace this is to ensure that
arch_phys_wc_add() returned 0.
If you disagree it'd be great if you can elaborate why.
Luis
^ permalink raw reply
* Re: [PATCH v4 1/2] IB/qib: add acounting for MTRR
From: Luis R. Rodriguez @ 2015-04-22 15:28 UTC (permalink / raw)
To: Doug Ledford
Cc: Luis R. Rodriguez, infinipath, roland, sean.hefty, hal.rosenstock,
linux-rdma, luto, mst, linux-kernel, cocci, Toshi Kani,
Suresh Siddha, Ingo Molnar, Thomas Gleixner, Juergen Gross,
Daniel Vetter, Dave Airlie, Antonino Daplas,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, linux-fbdev
In-Reply-To: <1429710292.45956.90.camel@redhat.com>
On Wed, Apr 22, 2015 at 09:44:52AM -0400, Doug Ledford wrote:
> On Tue, 2015-04-21 at 14:50 -0700, Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >
> > There is no good reason not to, we eventually delete it as well.
> >
> > Cc: Toshi Kani <toshi.kani@hp.com>
> > Cc: Suresh Siddha <sbsiddha@gmail.com>
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Juergen Gross <jgross@suse.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Andy Lutomirski <luto@amacapital.net>
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: Antonino Daplas <adaplas@gmail.com>
> > Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > Cc: Mike Marciniszyn <infinipath@intel.com>
> > Cc: Roland Dreier <roland@kernel.org>
> > Cc: Sean Hefty <sean.hefty@intel.com>
> > Cc: Hal Rosenstock <hal.rosenstock@gmail.com>
> > Cc: linux-rdma@vger.kernel.org
> > Cc: linux-fbdev@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> > ---
> > drivers/infiniband/hw/qib/qib_wc_x86_64.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/infiniband/hw/qib/qib_wc_x86_64.c b/drivers/infiniband/hw/qib/qib_wc_x86_64.c
> > index 81b225f..fe0850a 100644
> > --- a/drivers/infiniband/hw/qib/qib_wc_x86_64.c
> > +++ b/drivers/infiniband/hw/qib/qib_wc_x86_64.c
> > @@ -118,7 +118,7 @@ int qib_enable_wc(struct qib_devdata *dd)
> > if (!ret) {
> > int cookie;
> >
> > - cookie = mtrr_add(pioaddr, piolen, MTRR_TYPE_WRCOMB, 0);
> > + cookie = mtrr_add(pioaddr, piolen, MTRR_TYPE_WRCOMB, 1);
> > if (cookie < 0) {
> > {
> > qib_devinfo(dd->pcidev,
>
> Skip this patch please. You remove this line entirely in your next
> patch, so this becomes a single kernel out of all possible bisectable
> kernels with this accounting enabled, and then the very next kernel does
> away with it.
No, the next patch uses accounting enabled as well, it also makes somse
other changes. This change is done in order to add accounting to match
the grammar used by arch_phys_wc_add() so it is in fact an atomic
comittiable and highly recommmended bisectable commit to be present.
> It makes no sense to have a single outlying bisectable
> kernel like that.
This is an atomic difference worth keeping record of.
Luis
^ permalink raw reply
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Luis R. Rodriguez @ 2015-04-22 15:23 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Andy Lutomirski, mike.marciniszyn, infinipath, linux-rdma, awalls,
Toshi Kani, H. Peter Anvin, Ingo Molnar,
linux-kernel@vger.kernel.org, Hal Rosenstock, Sean Hefty,
Suresh Siddha, Rickard Strandqvist, Roland Dreier, Juergen Gross,
Mauro Carvalho Chehab, Borislav Petkov, Mel Gorman,
Vlastimil Babka, Davidlohr Bueso, Dave Hansen,
Jean-Christophe Plagniol-Villard, Thomas Gleixner, Ville Syrj?l?,
Linux Fbdev development list, linux-media, X86 ML, mcgrof
In-Reply-To: <20150422053939.GA29609@obsidianresearch.com>
On Tue, Apr 21, 2015 at 11:39:39PM -0600, Jason Gunthorpe wrote:
> On Wed, Apr 22, 2015 at 01:39:07AM +0200, Luis R. Rodriguez wrote:
> > > Mike, do you think the time is right to just remove the iPath driver?
> >
> > With PAT now being default the driver effectively won't work
> > with write-combining on modern kernels. Even if systems are old
> > they likely had PAT support, when upgrading kernels PAT will work
> > but write-combing won't on ipath.
>
> Sorry, do you mean the driver already doesn't get WC? Or do you mean
> after some more pending patches are applied?
No, you have to consider the system used and the effects of calls used
on the driver in light of this table:
----------------------------------------------------------------------
MTRR Non-PAT PAT Linux ioremap value Effective memory type
----------------------------------------------------------------------
Non-PAT | PAT
PAT
|PCD
||PWT
|||
WC 000 WB _PAGE_CACHE_MODE_WB WC | WC
WC 001 WC _PAGE_CACHE_MODE_WC WC* | WC
WC 010 UC- _PAGE_CACHE_MODE_UC_MINUS WC* | UC
WC 011 UC _PAGE_CACHE_MODE_UC UC | UC
----------------------------------------------------------------------
(*) denotes implementation defined and is discouraged
ioremap_nocache() will use _PAGE_CACHE_MODE_UC_MINUS by default today,
in the future we want to flip the switch and make _PAGE_CACHE_MODE_UC
the default. When that flip occurs it will mean ipath cannot get
write-combining on both non-PAT and PAT systems. Now that is for
the future, lets review the current situation for ipath.
For PAT capable systems if mtrr_add() is used today on a Linux system on a
region mapped with ioremap_nocache() that will mean you effectively nullify the
mtrr_add() effect as the combinatorial effect above yields an effective memory
type of UC. For PAT systems you want to use ioremap_wc() on the region in
which you need write-combining followed by arch_phys_wc_add() which will *only*
call mtrr_add() *iff* PAT was not enabled. This also means we need to split
the ioremap'd areas so that the area that is using ioremap_nocache() can never
get write-combining (_PAGE_CACHE_MODE_UC). The ipath driver needs the regions
split just as was done for the qib driver.
Now we could just say that leaving things as-is is a non-issue if you are OK
with non-write-combining effects being the default behaviour left on the ipath
driver for PAT systems. In that case we can just use arch_phys_wc_add() on the
driver and while it won't trigger the mtrr_add() on PAT systems it sill won't
have any effect. We just typically don't want to see use of ioremap_nocache()
paired with arch_phys_wc_add(), grammatically the correct thing to do is pair
ioremap_wc() areas with a arch_phys_wc_add() to make the write-combining effects
on non-PAT systems. If the ipath driver is not going to get he work required
to split the regions though perhaps we can live with a corner case driver that
annotates PAT must be disabled on the systems that use it and convert it to
arch_phys_wc_add() to just help with phasing out of direct use of mtrr_add().
With this strategy if and when ipath driver gets a split done it would gain WC
on both PAT and non-PAT.
Luis
^ permalink raw reply
* Re: [PATCH v4 2/2] IB/qib: use arch_phys_wc_add()
From: Doug Ledford @ 2015-04-22 13:54 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: infinipath, roland, sean.hefty, hal.rosenstock, linux-rdma, luto,
mst, linux-kernel, cocci, Luis R. Rodriguez, Toshi Kani,
Rickard Strandqvist, Mike Marciniszyn, Roland Dreier,
Dennis Dalessandro, Suresh Siddha, Ingo Molnar, Thomas Gleixner,
Juergen Gross, Daniel Vetter, Dave Airlie, Bjorn Helgaas,
Antonino Daplas, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Dave Hansen <dave>
In-Reply-To: <1429653035-19424-3-git-send-email-mcgrof@do-not-panic.com>
[-- Attachment #1: Type: text/plain, Size: 460 bytes --]
On Tue, 2015-04-21 at 14:50 -0700, Luis R. Rodriguez wrote:
This:
> + /* MTRR was used if this is non-zero */
> + if (!dd->wc_cookie)
> vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
And this:
> + dd->wc_cookie = arch_phys_wc_add(pioaddr, piolen);
> + if (dd->wc_cookie < 0)
> + ret = -EINVAL;
don't agree on what wc_cookie will be on error.
--
Doug Ledford <dledford@redhat.com>
GPG KeyID: 0E572FDD
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v4 1/2] IB/qib: add acounting for MTRR
From: Doug Ledford @ 2015-04-22 13:44 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: infinipath-ral2JQCrhuEAvxtiuMwx3w, roland-DgEjT+Ai2ygdnm+yROfE0A,
sean.hefty-ral2JQCrhuEAvxtiuMwx3w,
hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, luto-kltTT9wpgjJwATOyAt5JVQ,
mst-H+wXaHxf7aLQT0dZR+AlfA, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
cocci-/FJkirnvOdkvYVN+rsErww, Luis R. Rodriguez, Toshi Kani,
Suresh Siddha, Ingo Molnar, Thomas Gleixner, Juergen Gross,
Daniel Vetter, Dave Airlie, Antonino Daplas,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1429653035-19424-2-git-send-email-mcgrof-3uybbJdB1yH774rrrx3eTA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2038 bytes --]
On Tue, 2015-04-21 at 14:50 -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>
> There is no good reason not to, we eventually delete it as well.
>
> Cc: Toshi Kani <toshi.kani@hp.com>
> Cc: Suresh Siddha <sbsiddha@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Antonino Daplas <adaplas@gmail.com>
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Mike Marciniszyn <infinipath@intel.com>
> Cc: Roland Dreier <roland@kernel.org>
> Cc: Sean Hefty <sean.hefty@intel.com>
> Cc: Hal Rosenstock <hal.rosenstock@gmail.com>
> Cc: linux-rdma@vger.kernel.org
> Cc: linux-fbdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---
> drivers/infiniband/hw/qib/qib_wc_x86_64.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/hw/qib/qib_wc_x86_64.c b/drivers/infiniband/hw/qib/qib_wc_x86_64.c
> index 81b225f..fe0850a 100644
> --- a/drivers/infiniband/hw/qib/qib_wc_x86_64.c
> +++ b/drivers/infiniband/hw/qib/qib_wc_x86_64.c
> @@ -118,7 +118,7 @@ int qib_enable_wc(struct qib_devdata *dd)
> if (!ret) {
> int cookie;
>
> - cookie = mtrr_add(pioaddr, piolen, MTRR_TYPE_WRCOMB, 0);
> + cookie = mtrr_add(pioaddr, piolen, MTRR_TYPE_WRCOMB, 1);
> if (cookie < 0) {
> {
> qib_devinfo(dd->pcidev,
Skip this patch please. You remove this line entirely in your next
patch, so this becomes a single kernel out of all possible bisectable
kernels with this accounting enabled, and then the very next kernel does
away with it. It makes no sense to have a single outlying bisectable
kernel like that.
--
Doug Ledford <dledford@redhat.com>
GPG KeyID: 0E572FDD
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH 5/5] ASoC: omap-hdmi-audio: Fix invalid combination of DM_INH and CA
From: Jyri Sarha @ 2015-04-22 13:23 UTC (permalink / raw)
To: alsa-devel, linux-fbdev, linux-omap
Cc: peter.ujfalusi, broonie, liam.r.girdwood, tomi.valkeinen,
misael.lopez, Jyri Sarha
In-Reply-To: <cover.1429705991.git.jsarha@ti.com>
From: Misael Lopez Cruz <misael.lopez@ti.com>
DM_INH = 1 (stereo downmix prohibited) and CA = 0x00 (Channel
Allocation: FR, FL) is an invalid combination according to the
HDMI Compliance Test 7.31 "Audio InfoFrame".
Signed-off-by: Misael Lopez Cruz <misael.lopez@ti.com>
Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
sound/soc/omap/omap-hdmi-audio.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/sound/soc/omap/omap-hdmi-audio.c b/sound/soc/omap/omap-hdmi-audio.c
index 8df303f..aeef25c 100644
--- a/sound/soc/omap/omap-hdmi-audio.c
+++ b/sound/soc/omap/omap-hdmi-audio.c
@@ -217,7 +217,11 @@ static int hdmi_dai_hw_params(struct snd_pcm_substream *substream,
else
cea->db4_ca = 0x13;
- cea->db5_dminh_lsv = CEA861_AUDIO_INFOFRAME_DB5_DM_INH_PROHIBITED;
+ if (cea->db4_ca = 0x00)
+ cea->db5_dminh_lsv = CEA861_AUDIO_INFOFRAME_DB5_DM_INH_PERMITTED;
+ else
+ cea->db5_dminh_lsv = CEA861_AUDIO_INFOFRAME_DB5_DM_INH_PROHIBITED;
+
/* the expression is trivial but makes clear what we are doing */
cea->db5_dminh_lsv |= (0 & CEA861_AUDIO_INFOFRAME_DB5_LSV);
--
1.9.1
^ permalink raw reply related
* [PATCH 4/5] ASoC: omap-hdmi-audio: Force channel allocation only for OMAP4
From: Jyri Sarha @ 2015-04-22 13:23 UTC (permalink / raw)
To: alsa-devel, linux-fbdev, linux-omap
Cc: peter.ujfalusi, broonie, liam.r.girdwood, tomi.valkeinen,
misael.lopez, Jyri Sarha
In-Reply-To: <cover.1429705991.git.jsarha@ti.com>
From: Misael Lopez Cruz <misael.lopez@ti.com>
There is a constraint in the OMAP4 HDMI IP that requires to use
the 8-channel code when transmitting more than two channels.
The constraint doesn't apply for OMAP5 so don't force the channel
allocation in the sound driver as it can be done specifically for
OMAP4 later in the hdmi4 core.
Signed-off-by: Misael Lopez Cruz <misael.lopez@ti.com>
Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
drivers/video/fbdev/omap2/dss/hdmi4_core.c | 7 +++++++
sound/soc/omap/omap-hdmi-audio.c | 6 ++----
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/video/fbdev/omap2/dss/hdmi4_core.c b/drivers/video/fbdev/omap2/dss/hdmi4_core.c
index 2b8bd22..fa72e73 100644
--- a/drivers/video/fbdev/omap2/dss/hdmi4_core.c
+++ b/drivers/video/fbdev/omap2/dss/hdmi4_core.c
@@ -654,6 +654,13 @@ static void hdmi_core_audio_infoframe_cfg(struct hdmi_core_data *core,
hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(2), info_aud->db3);
sum += info_aud->db3;
+ /*
+ * The OMAP HDMI IP requires to use the 8-channel channel code when
+ * transmitting more than two channels.
+ */
+ if (info_aud->db4_ca != 0x00)
+ info_aud->db4_ca = 0x13;
+
hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(3), info_aud->db4_ca);
sum += info_aud->db4_ca;
diff --git a/sound/soc/omap/omap-hdmi-audio.c b/sound/soc/omap/omap-hdmi-audio.c
index 4775da4..8df303f 100644
--- a/sound/soc/omap/omap-hdmi-audio.c
+++ b/sound/soc/omap/omap-hdmi-audio.c
@@ -210,12 +210,10 @@ static int hdmi_dai_hw_params(struct snd_pcm_substream *substream,
cea->db3 = 0; /* not used, all zeros */
- /*
- * The OMAP HDMI IP requires to use the 8-channel channel code when
- * transmitting more than two channels.
- */
if (params_channels(params) = 2)
cea->db4_ca = 0x0;
+ else if (params_channels(params) = 6)
+ cea->db4_ca = 0xb;
else
cea->db4_ca = 0x13;
--
1.9.1
^ permalink raw reply related
* [PATCH 3/5] OMAPDSS: HDMI5: Fix AUDICONF3 bitfield offsets
From: Jyri Sarha @ 2015-04-22 13:22 UTC (permalink / raw)
To: alsa-devel, linux-fbdev, linux-omap
Cc: peter.ujfalusi, broonie, liam.r.girdwood, tomi.valkeinen,
misael.lopez, Jyri Sarha
In-Reply-To: <cover.1429705991.git.jsarha@ti.com>
From: Misael Lopez Cruz <misael.lopez@ti.com>
Downmix inhibit in HDMI_CORE_FC_AUDICONF3 register is in
bit 4 while CEA861_AUDIO_INFOFRAME_DB5_DM_INH sets bit 7.
Signed-off-by: Misael Lopez Cruz <misael.lopez@ti.com>
Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
drivers/video/fbdev/omap2/dss/hdmi5_core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/omap2/dss/hdmi5_core.c b/drivers/video/fbdev/omap2/dss/hdmi5_core.c
index 7a80beb..c3286bd 100644
--- a/drivers/video/fbdev/omap2/dss/hdmi5_core.c
+++ b/drivers/video/fbdev/omap2/dss/hdmi5_core.c
@@ -790,7 +790,9 @@ static void hdmi5_core_audio_infoframe_cfg(struct hdmi_core_data *core,
hdmi_write_reg(base, HDMI_CORE_FC_AUDICONF1, info_aud->db2_sf_ss);
hdmi_write_reg(base, HDMI_CORE_FC_AUDICONF2, info_aud->db4_ca);
- hdmi_write_reg(base, HDMI_CORE_FC_AUDICONF3, info_aud->db5_dminh_lsv);
+ hdmi_write_reg(base, HDMI_CORE_FC_AUDICONF3,
+ (info_aud->db5_dminh_lsv & CEA861_AUDIO_INFOFRAME_DB5_DM_INH) >> 3 |
+ (info_aud->db5_dminh_lsv & CEA861_AUDIO_INFOFRAME_DB5_LSV));
}
int hdmi5_audio_config(struct hdmi_core_data *core, struct hdmi_wp_data *wp,
--
1.9.1
^ permalink raw reply related
* [PATCH 2/5] OMAPDSS: HDMI5: Set valid sample order
From: Jyri Sarha @ 2015-04-22 13:22 UTC (permalink / raw)
To: alsa-devel, linux-fbdev, linux-omap
Cc: peter.ujfalusi, broonie, liam.r.girdwood, tomi.valkeinen,
misael.lopez, Jyri Sarha
In-Reply-To: <cover.1429705991.git.jsarha@ti.com>
From: Misael Lopez Cruz <misael.lopez@ti.com>
As per TRM, HDMI_WP_AUDIO_CFG[2] LEFT_BEFORE = 0 is reserved,
so it must always be set to 1 (the first sample is the left).
Signed-off-by: Misael Lopez Cruz <misael.lopez@ti.com>
Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
drivers/video/fbdev/omap2/dss/hdmi5_core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/fbdev/omap2/dss/hdmi5_core.c b/drivers/video/fbdev/omap2/dss/hdmi5_core.c
index a3cfe3d..7a80beb 100644
--- a/drivers/video/fbdev/omap2/dss/hdmi5_core.c
+++ b/drivers/video/fbdev/omap2/dss/hdmi5_core.c
@@ -870,6 +870,7 @@ int hdmi5_audio_config(struct hdmi_core_data *core, struct hdmi_wp_data *wp,
audio_format.samples_per_word = HDMI_AUDIO_ONEWORD_TWOSAMPLES;
audio_format.sample_size = HDMI_AUDIO_SAMPLE_16BITS;
audio_format.justification = HDMI_AUDIO_JUSTIFY_LEFT;
+ audio_format.sample_order = HDMI_AUDIO_SAMPLE_LEFT_FIRST;
/* only LPCM atm */
audio_format.type = HDMI_AUDIO_TYPE_LPCM;
--
1.9.1
^ permalink raw reply related
* [PATCH 1/5] OMAPDSS: HDMI4: Set correct CC for 8-channels layout
From: Jyri Sarha @ 2015-04-22 13:22 UTC (permalink / raw)
To: alsa-devel, linux-fbdev, linux-omap
Cc: peter.ujfalusi, broonie, liam.r.girdwood, tomi.valkeinen,
misael.lopez, Jyri Sarha
In-Reply-To: <cover.1429705991.git.jsarha@ti.com>
From: Misael Lopez Cruz <misael.lopez@ti.com>
OMAP4 HDMI IP uses the 8-channel layout with 8-channel speaker
allocation mask when transmitting more than two channels. But
the channel count field (CC) of the Audio InfoFrame's DB1 is
not updated for 8-channels.
As per HDMI Compliance Test 7.31 "Audio InfoFrame", CC = 7 is
required for 8-channels CA masks (0x13 and 0x1F).
Signed-off-by: Misael Lopez Cruz <misael.lopez@ti.com>
Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
drivers/video/fbdev/omap2/dss/hdmi4_core.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/omap2/dss/hdmi4_core.c b/drivers/video/fbdev/omap2/dss/hdmi4_core.c
index 7eafea5..2b8bd22 100644
--- a/drivers/video/fbdev/omap2/dss/hdmi4_core.c
+++ b/drivers/video/fbdev/omap2/dss/hdmi4_core.c
@@ -795,7 +795,9 @@ int hdmi4_audio_config(struct hdmi_core_data *core, struct hdmi_wp_data *wp,
/*
* the HDMI IP needs to enable four stereo channels when transmitting
- * more than 2 audio channels
+ * more than 2 audio channels. Similarly, the channel count in the
+ * Audio InfoFrame has to match the sample_present bits (some channels
+ * are padded with zeroes)
*/
if (channel_count = 2) {
audio_format.stereo_channels = HDMI_AUDIO_STEREO_ONECHANNEL;
@@ -807,6 +809,7 @@ int hdmi4_audio_config(struct hdmi_core_data *core, struct hdmi_wp_data *wp,
HDMI_AUDIO_I2S_SD1_EN | HDMI_AUDIO_I2S_SD2_EN |
HDMI_AUDIO_I2S_SD3_EN;
acore.layout = HDMI_AUDIO_LAYOUT_8CH;
+ audio->cea->db1_ct_cc = 7;
}
acore.en_spdif = false;
--
1.9.1
^ permalink raw reply related
* [PATCH 0/5] OMAPDSS: HDMI audio compliance fixes
From: Jyri Sarha @ 2015-04-22 13:22 UTC (permalink / raw)
To: alsa-devel, linux-fbdev, linux-omap
Cc: peter.ujfalusi, broonie, liam.r.girdwood, tomi.valkeinen,
misael.lopez, Jyri Sarha
I have rebased these patches from ti-linux-3.14.y on top of Linux
4.0.0. I tested them briefly on OMAP4 and OMAP5 in my environment, but
I could not test any channel count beyond stereo. However, there were
no conflicts in the rebase and each fix makes sense when looking at
the chip TRM and the relevant HDMI specs.
All the changes are really minor, but most of them apply to OMAPDSS side,
so it is probably best to merge them trough there.
Best regards,
Jyri
Misael Lopez Cruz (5):
OMAPDSS: HDMI4: Set correct CC for 8-channels layout
OMAPDSS: HDMI5: Set valid sample order
OMAPDSS: HDMI5: Fix AUDICONF3 bitfield offsets
ASoC: omap-hdmi-audio: Force channel allocation only for OMAP4
ASoC: omap-hdmi-audio: Fix invalid combination of DM_INH and CA
drivers/video/fbdev/omap2/dss/hdmi4_core.c | 12 +++++++++++-
drivers/video/fbdev/omap2/dss/hdmi5_core.c | 5 ++++-
sound/soc/omap/omap-hdmi-audio.c | 12 +++++++-----
3 files changed, 22 insertions(+), 7 deletions(-)
--
1.9.1
^ permalink raw reply
* Re: [PATCH v3 16/17] video: fbdev: atmel_lcdfb: use ioremap_wc() for framebuffer
From: Nicolas Ferre @ 2015-04-22 7:34 UTC (permalink / raw)
To: cocci
In-Reply-To: <1429647398-16983-17-git-send-email-mcgrof@do-not-panic.com>
Le 21/04/2015 22:16, Luis R. Rodriguez a écrit :
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>
> The driver doesn't use mtrr_add() or arch_phys_wc_add() but
> since we know the framebuffer is isolated already on an
> ioremap() we can take advantage of write combining for
> performance where possible.
>
> In this case there are a few motivations for this:
>
> a) Take advantage of PAT when available
>
> b) Help with the goal of eventually using _PAGE_CACHE_UC over
> _PAGE_CACHE_UC_MINUS on x86 on ioremap_nocache() (see commit
> de33c442e titled "x86 PAT: fix performance drop for glx,
> use UC minus for ioremap(), ioremap_nocache() and
> pci_mmap_page_range()")
>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Seems okay:
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Thanks for having taking care of atmel_lcdfb driver.
Bye,
> Cc: Suresh Siddha <sbsiddha@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Antonino Daplas <adaplas@gmail.com>
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: linux-fbdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---
> drivers/video/fbdev/atmel_lcdfb.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c
> index 94a8d04..abadc49 100644
> --- a/drivers/video/fbdev/atmel_lcdfb.c
> +++ b/drivers/video/fbdev/atmel_lcdfb.c
> @@ -1266,7 +1266,8 @@ static int __init atmel_lcdfb_probe(struct platform_device *pdev)
> goto stop_clk;
> }
>
> - info->screen_base = ioremap(info->fix.smem_start, info->fix.smem_len);
> + info->screen_base = ioremap_wc(info->fix.smem_start,
> + info->fix.smem_len);
> if (!info->screen_base) {
> ret = -ENOMEM;
> goto release_intmem;
>
--
Nicolas Ferre
^ permalink raw reply
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Jason Gunthorpe @ 2015-04-22 5:39 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Andy Lutomirski, mike.marciniszyn, infinipath, linux-rdma, awalls,
Toshi Kani, H. Peter Anvin, Ingo Molnar,
linux-kernel@vger.kernel.org, Hal Rosenstock, Sean Hefty,
Suresh Siddha, Rickard Strandqvist, Roland Dreier, Juergen Gross,
Mauro Carvalho Chehab, Borislav Petkov, Mel Gorman,
Vlastimil Babka, Davidlohr Bueso, Dave Hansen,
Jean-Christophe Plagniol-Villard, Thomas Gleixner, Ville Syrj?l?,
Linux Fbdev development list, linux-media, X86 ML, mcgrof
In-Reply-To: <20150421233907.GA5622@wotan.suse.de>
On Wed, Apr 22, 2015 at 01:39:07AM +0200, Luis R. Rodriguez wrote:
> > Mike, do you think the time is right to just remove the iPath driver?
>
> With PAT now being default the driver effectively won't work
> with write-combining on modern kernels. Even if systems are old
> they likely had PAT support, when upgrading kernels PAT will work
> but write-combing won't on ipath.
Sorry, do you mean the driver already doesn't get WC? Or do you mean
after some more pending patches are applied?
Jason
^ permalink raw reply
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Luis R. Rodriguez @ 2015-04-21 23:39 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Andy Lutomirski, mike.marciniszyn, infinipath, linux-rdma, awalls,
Toshi Kani, H. Peter Anvin, Ingo Molnar,
linux-kernel@vger.kernel.org, Hal Rosenstock, Sean Hefty,
Suresh Siddha, Rickard Strandqvist, Roland Dreier, Juergen Gross,
Mauro Carvalho Chehab, Borislav Petkov, Mel Gorman,
Vlastimil Babka, Davidlohr Bueso, Dave Hansen,
Jean-Christophe Plagniol-Villard, Thomas Gleixner,
Ville Syrjälä, Linux Fbdev development list,
linux-media, X86 ML, mcgrof
In-Reply-To: <20150421225732.GA17356@obsidianresearch.com>
On Tue, Apr 21, 2015 at 04:57:32PM -0600, Jason Gunthorpe wrote:
> On Wed, Apr 22, 2015 at 12:46:01AM +0200, Luis R. Rodriguez wrote:
>
> > are talking about annotating the qib driver as "known to be broken without PAT"
> > and since the ipath driver needs considerable work to be ported to
> > use PAT (the
>
> This only seems to be true for one of the chips that driver supports,
> not all possibilities.
>
> > userspace register is just one area) I wanted to review if we can just remove
> > MTRR use on the ipath driver and annotate write-combining with PAT as a TODO
> > item.
>
> AFAIK, dropping MTRR support will completely break the performance to
> the point the driver is unusable. If we drop MTRR we may as well
> remove the driver.
To be clear, the arch_phys_wc_add() API will be a no-op when PAT is
enabled on a system. Although some folks think PAT is new, its not,
its just that our implementation on Linux lacked a bit of push, recent
changes however make PAT support complete and that means now we'll have
PAT enabled on systems that likely didn't before on recent kernels.
There are other important motivations to use PAT:
* Xen won't work with MTRR, having a unified PAT enabled kernel
will ensure that when Xen is used write-combinging will take
effect
* Long term we want to make strong UC the default to ioremap() on
x86, right now its not, its UC-, we need to convert all drivers
that want write-combining over to use ioremap_wc() for their
specific area, and it must not overlap. There are issues with
using mtrr_add() on regions marked as UC-, since its the default
this means that mtrr_add() use on ioramp'd areas on PAT systems
will actually make write-combining *void*. Refer to this table
for combinatorial effects for non-PAT / PAT of wc MTRR:
https://marc.info/?l=linux-kernel&m\x142964809710517&w=1
So as the train of PAT enablement moves forward it means systems
that have PAT enabled now might not have write-combining effective.
In order to get the best of both worlds, non-PAT and PAT systems
we must design drivers cleanly for the non-writecombining and
write-combining areas. This mean using ioremap_nocache() for MMIO
and ioremap_wc() *only* for the desired, write-combining area,
followed by arch_phys_wc_add().
> Mike, do you think the time is right to just remove the iPath driver?
With PAT now being default the driver effectively won't work
with write-combining on modern kernels. Even if systems are old
they likely had PAT support, when upgrading kernels PAT will work
but write-combing won't on ipath.
Luis
^ permalink raw reply
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Luis R. Rodriguez @ 2015-04-21 23:14 UTC (permalink / raw)
To: Andy Walls
Cc: Andy Lutomirski, Hyong-Youb Kim, netdev@vger.kernel.org,
infinipath, jgunthorpe, mike.marciniszyn, Toshi Kani,
H. Peter Anvin, Ingo Molnar, linux-kernel@vger.kernel.org,
Hal Rosenstock, Sean Hefty, Suresh Siddha, Rickard Strandqvist,
Roland Dreier, Juergen Gross, Mauro Carvalho Chehab,
Borislav Petkov, Mel Gorman, Vlastimil Babka, Davidlohr Bueso,
Dave Hansen, Jean-Christophe Plagniol-Villard, Thomas Gleixner,
Ville Syrjälä, linux-fbdev, linux-media, X86ML
In-Reply-To: <5536d47a.95968c0a.1d12.ffffbf85@mx.google.com>
On Tue, Apr 21, 2015 at 06:51:26PM -0400, Andy Walls wrote:
> Sorry for the top post; mobile work email account.
>
> Luis,
>
> You do the changes to remove MTTR and point me to your dev repo and branch.
> Also point me to the new functions/primitives I'll need.
There is nothing new actually needed for ivtv, unless of course
the ivtv driver is bounded to use a large MTRR that includes
the non-framebuffer region, if so then the ioremap_uc() would
be needed, and you can just cherry pick that patch:
https://marc.info/?l=linux-kernel&m\x142964809110516&w=1
I'll bounce that patch to you as well. Might help reading this
patch too:
https://marc.info/?l=linux-kernel&m\x142964809710517&w=1
If your write-combining area is not restricted by size constraints
so that it also include the non-framebuffer areas then you can just
do a simple conversion of the driver to use ioremap_wc() on the
framebuffer followed by arch_phys_wc_add().
An example driver that required changes to split this with size
contraints is atyfb, here are the changes for it:
https://marc.info/?l=linux-kernel&m\x142964818810539&w=1
https://marc.info/?l=linux-kernel&m\x142964813610531&w=1
https://marc.info/?l=linux-kernel&m\x142964811010524&w=1
https://marc.info/?l=linux-kernel&m\x142964814810532&w=1
If you are not constrained by MTRR's limitation on size then
a simple trivial driver conversion is sufficient. For example:
https://marc.info/?l=linux-kernel&m\x142964744610286&w=1
I should also note that we are strivoing to also not use overlapping ioremap()
calls as we want to avoid that mess. Overlapping iroemap() calls with different
types could in theory work but its best we just design clean drivers and avoid
this.
As per Andy Lutomirski, what we'd need done on ivtv likely is
for it to ioremap() for an initial bring up of the device, then
infer the framebuffer offset, and only when that is being used
then iounmap and then ioremap() again split areas on the driver,
one with ioremap.
> I'll do the changes to add write-combining back into ivtv and ivtvfb, test
> them with my hardware and push them to my linuxtv.org git repo.
Great! The above sounded like a complexity you did not wish to
take on, but if you're up for the change, that'd be awesome!
> I know there is at least one English speaking user in India using ivtv with
> old PVR hardware, and probably folks in less developed places also using it.
If the above is too much work for that few amount of users I'd hope
we can just have them use older kernels, for the sake of sane APIs and
clean driver architecture.
Luis
^ permalink raw reply
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Jason Gunthorpe @ 2015-04-21 22:57 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Andy Lutomirski, mike.marciniszyn, infinipath, linux-rdma, awalls,
Toshi Kani, H. Peter Anvin, Ingo Molnar,
linux-kernel@vger.kernel.org, Hal Rosenstock, Sean Hefty,
Suresh Siddha, Rickard Strandqvist, Roland Dreier, Juergen Gross,
Mauro Carvalho Chehab, Borislav Petkov, Mel Gorman,
Vlastimil Babka, Davidlohr Bueso, Dave Hansen,
Jean-Christophe Plagniol-Villard, Thomas Gleixner,
Ville Syrjälä, Linux Fbdev development list,
linux-media, X86 ML, mcgrof
In-Reply-To: <20150421224601.GY5622@wotan.suse.de>
On Wed, Apr 22, 2015 at 12:46:01AM +0200, Luis R. Rodriguez wrote:
> are talking about annotating the qib driver as "known to be broken without PAT"
> and since the ipath driver needs considerable work to be ported to
> use PAT (the
This only seems to be true for one of the chips that driver supports,
not all possibilities.
> userspace register is just one area) I wanted to review if we can just remove
> MTRR use on the ipath driver and annotate write-combining with PAT as a TODO
> item.
AFAIK, dropping MTRR support will completely break the performance to
the point the driver is unusable. If we drop MTRR we may as well
remove the driver.
Mike, do you think the time is right to just remove the iPath driver?
Jason
^ permalink raw reply
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Luis R. Rodriguez @ 2015-04-21 22:46 UTC (permalink / raw)
To: Andy Lutomirski, jgunthorpe, mike.marciniszyn, infinipath,
linux-rdma, awalls
Cc: Toshi Kani, H. Peter Anvin, Ingo Molnar,
linux-kernel@vger.kernel.org, Hal Rosenstock, Sean Hefty,
Suresh Siddha, Rickard Strandqvist, Roland Dreier, Juergen Gross,
Mauro Carvalho Chehab, Borislav Petkov, Mel Gorman,
Vlastimil Babka, Davidlohr Bueso, Dave Hansen,
Jean-Christophe Plagniol-Villard, Thomas Gleixner,
Ville Syrjälä, Linux Fbdev development list,
linux-media, X86 ML, mcgrof
In-Reply-To: <CALCETrV0B7rp08-VYjp5=1CWJp7=xTUTBYo3uGxX317RxAQT+w@mail.gmail.com>
On Wed, Apr 15, 2015 at 01:42:47PM -0700, Andy Lutomirski wrote:
> On Mon, Apr 13, 2015 at 10:49 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>
> > c) ivtv: the driver does not have the PCI space mapped out separately, and
> > in fact it actually does not do the math for the framebuffer, instead it lets
> > the device's own CPU do that and assume where its at, see
> > ivtvfb_get_framebuffer() and CX2341X_OSD_GET_FRAMEBUFFER, it has a get
> > but not a setter. Its not clear if the firmware would make a split easy.
> > We'd need ioremap_ucminus() here too and __arch_phys_wc_add().
> >
>
> IMO this should be conceptually easy to split. Once we get the
> framebuffer address, just unmap it (or don't prematurely map it) and
> then ioremap the thing.
Side note to ipath driver folks: as reviewed with Andy Walls, the
ivtv driver cannot easily be ported to use PAT so we are evaluating
simply removing write-combing from that driver on future kernels.
>
> > From the beginning it seems only framebuffer devices used MTRR/WC, lately it
> > seems infiniband drivers also find good use for for it for PIO TX buffers to
> > blast some sort of data, in the future I would not be surprised if other
> > devices found use for it.
>
> IMO the Infiniband maintainers should fix their code. Especially in
> the server space, there aren't that many MTRRs to go around. I wrote
> arch_phys_wc_add in the first place because my server *ran out of
> MTRRs*.
>
> Hey, IB people: can you fix your drivers to use arch_phys_wc_add
> (which is permitted to be a no-op) along with ioremap_wc? Your users
ipath driver maintainers:
The ipath driver is one of two drivers left to convert over to
arch_phys_wc_add(). MTRR use is being deprecated, and its use is actually
highly discouraged now that we have proper PAT implemenation on Linux. Since we
are talking about annotating the qib driver as "known to be broken without PAT"
and since the ipath driver needs considerable work to be ported to use PAT (the
userspace register is just one area) I wanted to review if we can just remove
MTRR use on the ipath driver and annotate write-combining with PAT as a TODO
item.
This would help a lot in our journey to bury MTRR use.
Luis
^ permalink raw reply
* Re: [PATCH v4 2/2] IB/qib: use arch_phys_wc_add()
From: Jason Gunthorpe @ 2015-04-21 22:17 UTC (permalink / raw)
To: cocci
In-Reply-To: <1429653035-19424-3-git-send-email-mcgrof@do-not-panic.com>
On Tue, Apr 21, 2015 at 02:50:35PM -0700, Luis R. Rodriguez wrote:
> - if (qib_wc_pat) {
> - resource_size_t vl15off;
> - /*
> - * We do not set WC on the VL15 buffers to avoid
> - * a rare problem with unaligned writes from
> - * interrupt-flushed store buffers, so we need
> - * to map those separately here. We can't solve
> - * this for the rarely used mtrr case.
> - */
> - ret = init_chip_wc_pat(dd, 0);
> - if (ret)
> - goto bail;
> + /*
> + * We do not set WC on the VL15 buffers to avoid
> + * a rare problem with unaligned writes from
> + * interrupt-flushed store buffers, so we need
> + * to map those separately here. We can't solve
> + * this for the rarely used mtrr case.
> + */
This is a small change in behavior, but it doesn't seem important..
Mike, what do you think about adding:
if (dd->wc_cookie)
dev_err(.., "Using this device without CPU PAT support is known to be broken");
or similar..
Jason
^ permalink raw reply
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Andy Lutomirski @ 2015-04-21 22:09 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Andy Walls, Hyong-Youb Kim, netdev@vger.kernel.org, Toshi Kani,
H. Peter Anvin, Ingo Molnar, linux-kernel@vger.kernel.org,
Hal Rosenstock, Sean Hefty, Suresh Siddha, Rickard Strandqvist,
Mike Marciniszyn, Roland Dreier, Juergen Gross,
Mauro Carvalho Chehab, Borislav Petkov, Mel Gorman,
Vlastimil Babka, Davidlohr Bueso, Dave Hansen,
Jean-Christophe Plagniol-Villard, Thomas Gleixner,
Ville Syrjälä, linux-fbdev, linux-media, X86 ML
In-Reply-To: <CAB=NE6UFOi_CYbrgYGCfU3Uki30iGdPPL2+V5RLYww=NS7G0GA@mail.gmail.com>
On Tue, Apr 21, 2015 at 3:08 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Tue, Apr 21, 2015 at 3:02 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> Andy, can we live without MTRR support on this driver for future kernels? This
>> would only leave ipath as the only offending driver.
>
> Sorry to be clear, can we live with removal of write-combining on this driver?
>
I personally think so, but a driver maintainer's ack would be nice.
--Andy
^ 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