* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Andy Walls @ 2015-04-15 23:59 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Luis R. Rodriguez, 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 development list,
linux-media, X86 ML
In-Reply-To: <CALCETrUFtEMYh8i00ke0f939=17bAQxMDOBZMn_3yk3Nz1AnFA@mail.gmail.com>
On Wed, 2015-04-15 at 16:42 -0700, Andy Lutomirski wrote:
> On Wed, Apr 15, 2015 at 3:38 PM, Andy Walls <awalls@md.metrocast.net> wrote:
> >
>
> IMO the right solution would be to avoid ioremapping the whole bar at
> startup. Instead ioremap pieces once the driver learns what they are.
> This wouldn't have any of these problems -- you'd ioremap() register
> regions and you'd ioremap_wc() the framebuffer once you find it. If
> there are regions of unknown purpose, just don't map them all.
>
> Would this be feasible?
Feasible? Maybe.
Worth the time and effort for end-of-life, convential PCI hardware so I
can have an optimally performing X display on a Standard Def Analog TV
screen? Nope. I don't have that level of nostalgia.
We sort of know where some things are in the MMIO space due to
experimentation and past efforts examining the firmware binary.
Documentation/video4linux/cx2341x/fw-*.txt documents some things. The
driver code actually codifies a little bit more knowledge.
The driver code for doing transfers between host and card is complex and
fragile with some streams that use DMA, other streams that use PIO,
digging VBI data straight out of card memory, and scatter-gather being
broken on newer firmwares. Playing around with ioremapping will be hard
to get right and likely cause something in the code to break for the
primary use case of the ivtv supported cards.
Regards,
Andy
^ permalink raw reply
* [PATCH] gpio-backlight: Discover driver during boot time
From: Arun Bharadwaj @ 2015-04-16 0:21 UTC (permalink / raw)
To: linux-fbdev, linux-kernel
Cc: Jingoo Han, Lee Jones, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, Ash Charles
The gpio-backlight driver seems to be missing the
MODULE_DEVICE_TABLE line which is preventing it from
being modprobed during boot time even if the
gpio-backlight device exists.
This seems to be a bug and this patch attempts to
fix that.
Signed-off-by: Arun Bharadwaj <arun@gumstix.com>
Signed-off-by: Ash Charles <ashcharles@gmail.com>
---
drivers/video/backlight/gpio_backlight.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
index 439feb2..5fbbc2e 100644
--- a/drivers/video/backlight/gpio_backlight.c
+++ b/drivers/video/backlight/gpio_backlight.c
@@ -146,6 +146,8 @@ static struct of_device_id gpio_backlight_of_match[] = {
{ .compatible = "gpio-backlight" },
{ /* sentinel */ }
};
+
+MODULE_DEVICE_TABLE(of, gpio_backlight_of_match);
#endif
static struct platform_driver gpio_backlight_driver = {
--
1.9.1
^ permalink raw reply related
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Andy Walls @ 2015-04-16 0:33 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Luis R. Rodriguez, linux-rdma, 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 development list,
linux-media, X86 ML
In-Reply-To: <CALCETrU9FEoXgWxV+XXwRdKTxUxYj7CD3ropnFb4Pq1cMkucaQ@mail.gmail.com>
On Wed, 2015-04-15 at 16:52 -0700, Andy Lutomirski wrote:
> On Wed, Apr 15, 2015 at 3:50 PM, Andy Walls <awalls@md.metrocast.net> wrote:
> > On Wed, 2015-04-15 at 13:42 -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.
> >
> > Not so easy. The main ivtv driver has already set up the PCI device and
> > done the mapping for the MPEG-2 decoder/video output engine. The video
> > decoder/output device nodes might already be open by user space calling
> > into the main driver, before the ivtvfb module is even loaded.
>
> Surely the MPEG-2 decoder/video engine won't overlap the framebuffer,
> though. Am I missing something?
ivtvfb is stealing the decoders' OSD for use as a framebuffer.
The decoder video output memory doesn't overlap the decoder OSD memory,
but there is a functional overlap. ivtv driver video output device
nodes can manipulate the OSD that ivtvfb is stealing.
It would be a dumb thing for the user to want to use ivtvfb, and to also
manipulate the OSD via the video output device nodes at the same time,
for anything other than setting up the TV video standard. However the
current ivtv driver code doesn't prevent the OSD from being manipulated
by the video output device nodes when ivtvfb is in use.
-Andy
^ permalink raw reply
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Andy Lutomirski @ 2015-04-16 0:58 UTC (permalink / raw)
To: Andy Walls
Cc: Luis R. Rodriguez, 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 development list,
linux-media, X86 ML
In-Reply-To: <1429142387.1899.57.camel@palomino.walls.org>
On Wed, Apr 15, 2015 at 4:59 PM, Andy Walls <awalls@md.metrocast.net> wrote:
> On Wed, 2015-04-15 at 16:42 -0700, Andy Lutomirski wrote:
>> On Wed, Apr 15, 2015 at 3:38 PM, Andy Walls <awalls@md.metrocast.net> wrote:
>
>> >
>>
>> IMO the right solution would be to avoid ioremapping the whole bar at
>> startup. Instead ioremap pieces once the driver learns what they are.
>> This wouldn't have any of these problems -- you'd ioremap() register
>> regions and you'd ioremap_wc() the framebuffer once you find it. If
>> there are regions of unknown purpose, just don't map them all.
>>
>> Would this be feasible?
>
> Feasible? Maybe.
>
> Worth the time and effort for end-of-life, convential PCI hardware so I
> can have an optimally performing X display on a Standard Def Analog TV
> screen? Nope. I don't have that level of nostalgia.
>
The point is actually to let us unexport or delete mtrr_add. We can
either severely regress performance on ivtv on PAT-capable hardware if
we naively switch it to arch_phys_wc_add or we can do something else.
The something else remains to be determined.
>
> We sort of know where some things are in the MMIO space due to
> experimentation and past efforts examining the firmware binary.
>
> Documentation/video4linux/cx2341x/fw-*.txt documents some things. The
> driver code actually codifies a little bit more knowledge.
>
> The driver code for doing transfers between host and card is complex and
> fragile with some streams that use DMA, other streams that use PIO,
> digging VBI data straight out of card memory, and scatter-gather being
> broken on newer firmwares. Playing around with ioremapping will be hard
> to get right and likely cause something in the code to break for the
> primary use case of the ivtv supported cards.
Ick.
If the only thing that really wants WC is the esoteric framebuffer
thing, could we just switch to arch_phys_wc_add and assume that no one
will care about the regression on new CPUs with ivtv cards?
--Andy
^ permalink raw reply
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Andy Walls @ 2015-04-16 1:01 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Luis R. Rodriguez, 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 development list,
linux-media, X86 ML
In-Reply-To: <CALCETrWRjGYqcYPNizrbiVFwFHhrLf=8NTTCLVZh7Q6MgAWj=Q@mail.gmail.com>
On Wed, 2015-04-15 at 17:58 -0700, Andy Lutomirski wrote:
> On Wed, Apr 15, 2015 at 4:59 PM, Andy Walls <awalls@md.metrocast.net> wrote:
> > On Wed, 2015-04-15 at 16:42 -0700, Andy Lutomirski wrote:
> >> On Wed, Apr 15, 2015 at 3:38 PM, Andy Walls <awalls@md.metrocast.net> wrote:
> >
> >> >
> >>
> >> IMO the right solution would be to avoid ioremapping the whole bar at
> >> startup. Instead ioremap pieces once the driver learns what they are.
> >> This wouldn't have any of these problems -- you'd ioremap() register
> >> regions and you'd ioremap_wc() the framebuffer once you find it. If
> >> there are regions of unknown purpose, just don't map them all.
> >>
> >> Would this be feasible?
> >
> > Feasible? Maybe.
> >
> > Worth the time and effort for end-of-life, convential PCI hardware so I
> > can have an optimally performing X display on a Standard Def Analog TV
> > screen? Nope. I don't have that level of nostalgia.
> >
>
> The point is actually to let us unexport or delete mtrr_add.
Understood.
> We can
> either severely regress performance on ivtv on PAT-capable hardware if
> we naively switch it to arch_phys_wc_add or we can do something else.
> The something else remains to be determined.
Maybe ioremap the decoder register area as UC, and ioremap the rest of
the decoder region to WC. (Does that suck up too many PAT resources?)
Then add PCI reads following any sort of singleton PCI writes in the WC
region. I assume PCI rules about write postings before reads still
apply when WC is set.
> >
> > We sort of know where some things are in the MMIO space due to
> > experimentation and past efforts examining the firmware binary.
> >
> > Documentation/video4linux/cx2341x/fw-*.txt documents some things. The
> > driver code actually codifies a little bit more knowledge.
> >
> > The driver code for doing transfers between host and card is complex and
> > fragile with some streams that use DMA, other streams that use PIO,
> > digging VBI data straight out of card memory, and scatter-gather being
> > broken on newer firmwares. Playing around with ioremapping will be hard
> > to get right and likely cause something in the code to break for the
> > primary use case of the ivtv supported cards.
>
> Ick.
Yeah.
> If the only thing that really wants WC is the esoteric framebuffer
> thing,
That appears to be it.
> could we just switch to arch_phys_wc_add and assume that no one
> will care about the regression on new CPUs with ivtv cards?
That's on the table in my mind. Not sure if it is the friendliest thing
to do to users. Quite honestly though, modern graphics cards have much
better ouput resolution and performance. Anyone with a modern system
really should be using one. (i.e. MythTV gave up on support for PVR-350
output for video playback years ago in May 2010.)
BTW, my 2005 system with multiple conventional PCI slots in it shows a
'pat' flag in /proc/cpuinfo. (AMD Athlon(tm) 64 X2 Dual Core Processor
4200+) I didn't know it was considered "new". :)
Regards,
Andy
^ permalink raw reply
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Andy Walls @ 2015-04-16 1:07 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Hyong-Youb Kim, netdev, Andy Lutomirski, Toshi Kani,
H. Peter Anvin, Ingo Molnar, linux-kernel, 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, plagnioj, tglx, Ville Syrjälä, linux-fbdev,
linux-media, x86
In-Reply-To: <20150415235816.GG5622@wotan.suse.de>
On Thu, 2015-04-16 at 01:58 +0200, Luis R. Rodriguez wrote:
> Hey Andy, thanks for your review, adding Hyong-Youb Kim for review of the
> full range ioremap_wc() idea below.
>
> On Wed, Apr 15, 2015 at 06:38:51PM -0400, Andy Walls wrote:
> > Hi All,
> >
> > On Mon, 2015-04-13 at 19:49 +0200, Luis R. Rodriguez wrote:
> > > From the beginning it seems only framebuffer devices used MTRR/WC,
> > [snip]
> > > The ivtv device is a good example of the worst type of
> > > situations and these days. So perhap __arch_phys_wc_add() and a
> > > ioremap_ucminus() might be something more than transient unless hardware folks
> > > get a good memo or already know how to just Do The Right Thing (TM).
> >
> > Just to reiterate a subtle point, use of the ivtvfb is *optional*. A
> > user may or may not load it. When the user does load the ivtvfb driver,
> > the ivtv driver has already been initialized and may have functions of
> > the card already in use by userspace.
>
> I suspected this and its why I note that a rewrite to address a clean
> split with separate ioremap seems rather difficult in this case.
>
> > Hopefully no one is trying to use the OSD as framebuffer and the video
> > decoder/output engine for video display at the same time.
>
> Worst case concern I have also is the implications of having overlapping
> ioremap() calls (as proposed in my last reply) for different memory types
> and having the different virtual memory addresse used by different parts
> of the driver. Its not clear to me what the hardware implications of this
> are.
>
> > But the video
> > decoder/output device nodes may already be open for performing ioctl()
> > functions so unmapping the decoder IO space out from under them, when
> > loading the ivtvfb driver module, might not be a good thing.
>
> Using overlapping ioremap() calls with different memory types would address
> this concern provided hardware won't barf both on the device and CPU. Hardware
> folks could provide feedback or an ivtvfb user could test the patch supplied
> on both non-PAT and PAT systems. Even so, who knows, this might work on some
> systems while not on others, only hardware folks would know.
The CX2341[56] firmware+hardware has a track record for being really
picky about sytem hardware. It's primary symptoms are for the DMA
engine or Mailbox protocol to get hung up. So yeah, it could barf
easily on some users.
> An alternative... is to just ioremap_wc() the entire region, including
> MMIO registers for these old devices.
That's my thought; as long as implementing PCI write then read can force
writes to be posted and that setting that many pages as WC doesn't cause
some sort of PAT resource exhaustion. (I know very little about PAT).
> I see one ethernet driver that does
> this, myri10ge, and am curious how and why they ended up deciding this
> and if they have run into any issues. I wonder if this is a reasonable
> comrpomise for these 2 remaining corner cases.
>
> Luis
Regards,
Andy
^ permalink raw reply
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Hyong-Youb Kim @ 2015-04-16 4:18 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Andy Walls, Hyong-Youb Kim, netdev, Andy Lutomirski, Toshi Kani,
H. Peter Anvin, Ingo Molnar, linux-kernel, 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, plagnioj, tglx, Ville Syrjälä, linux-fbdev,
linux-media, x86
In-Reply-To: <20150415235816.GG5622@wotan.suse.de>
On Thu, Apr 16, 2015 at 01:58:16AM +0200, Luis R. Rodriguez wrote:
>
> An alternative... is to just ioremap_wc() the entire region, including
> MMIO registers for these old devices. I see one ethernet driver that does
> this, myri10ge, and am curious how and why they ended up deciding this
> and if they have run into any issues. I wonder if this is a reasonable
> comrpomise for these 2 remaining corner cases.
>
For myri10ge, it a performance thing. Descriptor rings are in NIC
memory BAR0, not in host memory. Say, to send a packet, the driver
writes the send descriptor to the ioremap'd NIC memory. It is a
multi-word descriptor. So, to send it as one PCIE MWr transaction,
the driver maps the whole BAR0 as WC and does "copy descriptor; wmb".
Without WC, descriptors would end up as multiple 4B or 8B MWr packets
to the NIC, which has a pretty big performance impact on this
particular NIC.
Most registers that do not want WC are actually in BAR2, which is not
mapped as WC. For registers that are in BAR0, we do "write to the
register; wmb". If we want to wait till the NIC has seen the write,
we do "write; wmb; read".
This approach has worked for this device for many years. I cannot say
whether it works for other devices, though.
^ permalink raw reply
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Luis R. Rodriguez @ 2015-04-16 18:54 UTC (permalink / raw)
To: Hyong-Youb Kim
Cc: Andy Walls, Hyong-Youb Kim, netdev, Andy Lutomirski, Toshi Kani,
H. Peter Anvin, Ingo Molnar, linux-kernel, 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,
Johannes Berg, Felix Fietkau, Benjamin Poirier, dave.hansen,
plagnioj, tglx, Ville Syrjälä, linux-fbdev, linux-media,
x86
In-Reply-To: <20150416041837.GA5712@hykim-PC>
On Thu, Apr 16, 2015 at 01:18:37PM +0900, Hyong-Youb Kim wrote:
> On Thu, Apr 16, 2015 at 01:58:16AM +0200, Luis R. Rodriguez wrote:
> >
> > An alternative... is to just ioremap_wc() the entire region, including
> > MMIO registers for these old devices. I see one ethernet driver that does
> > this, myri10ge, and am curious how and why they ended up deciding this
> > and if they have run into any issues. I wonder if this is a reasonable
> > comrpomise for these 2 remaining corner cases.
> >
>
> For myri10ge, it a performance thing. Descriptor rings are in NIC
> memory BAR0, not in host memory. Say, to send a packet, the driver
> writes the send descriptor to the ioremap'd NIC memory. It is a
> multi-word descriptor. So, to send it as one PCIE MWr transaction,
> the driver maps the whole BAR0 as WC and does "copy descriptor; wmb".
Interesting, so you burst write multi-word descriptor writes using
write-combining here for the Ethernet device.
> Without WC, descriptors would end up as multiple 4B or 8B MWr packets
> to the NIC, which has a pretty big performance impact on this
> particular NIC.
How big are the descriptors?
> Most registers that do not want WC are actually in BAR2, which is not
> mapped as WC. For registers that are in BAR0, we do "write to the
> register; wmb". If we want to wait till the NIC has seen the write,
> we do "write; wmb; read".
Interesting, thanks, yeah using this as a work around to the problem sounds
plausible however it still would require likely making just as many changes to
the ivtv and ipath driver as to just do a proper split. I do wonder however if
this sort of work around can be generalized somehow though so that others could
use, if this sort of thing is going to become prevalent. If so then this would
serve two purposes: work around for the corner cases of MTRR use on Linux and
also these sorts of device constraints.
In order to determine if this is likely to be generally useful could you elaborate
a bit more about the detals of the performance issues of not bursting writes
for the descriptor on this device.
Even if that is done a conversion over to this work around seems it may require
device specific nitpicks. For instance I note in myri10ge_submit_req() for
small writes you just do a reverse write and do the first set last, then
finally the last 32 bits are rewritten, I guess to trigger something?
> This approach has worked for this device for many years. I cannot say
> whether it works for other devices, though.
I think it should but the more interesting question would be exactly
*why* it was needed for this device, who determined that, and why?
Luis
^ permalink raw reply
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Andy Lutomirski @ 2015-04-16 19:19 UTC (permalink / raw)
To: Andy Walls
Cc: Jean-Christophe Plagniol-Villard, Roland Dreier,
Linux Fbdev development list, Juergen Gross, X86 ML,
Mauro Carvalho Chehab, Mike Marciniszyn, Suresh Siddha,
Ville Syrjälä, Borislav Petkov, Sean Hefty,
Rickard Strandqvist, Hal Rosenstock, linux-kernel@vger.kernel.org,
H. Peter Anvin, Mel Gorman, Vlastimil Babka, Luis R. Rodriguez,
Toshi Kani, Thomas Gleixner, Dave Hansen, Davidlohr Bueso,
linux-media, Ingo Molnar
In-Reply-To: <1429146083.1899.94.camel@palomino.walls.org>
On Apr 15, 2015 6:54 PM, "Andy Walls" <awalls@md.metrocast.net> wrote:
>
> On Wed, 2015-04-15 at 17:58 -0700, Andy Lutomirski wrote:
> > On Wed, Apr 15, 2015 at 4:59 PM, Andy Walls <awalls@md.metrocast.net> wrote:
> > > On Wed, 2015-04-15 at 16:42 -0700, Andy Lutomirski wrote:
> > >> On Wed, Apr 15, 2015 at 3:38 PM, Andy Walls <awalls@md.metrocast.net> wrote:
> > >
> > >> >
> > >>
> > >> IMO the right solution would be to avoid ioremapping the whole bar at
> > >> startup. Instead ioremap pieces once the driver learns what they are.
> > >> This wouldn't have any of these problems -- you'd ioremap() register
> > >> regions and you'd ioremap_wc() the framebuffer once you find it. If
> > >> there are regions of unknown purpose, just don't map them all.
> > >>
> > >> Would this be feasible?
> > >
> > > Feasible? Maybe.
> > >
> > > Worth the time and effort for end-of-life, convential PCI hardware so I
> > > can have an optimally performing X display on a Standard Def Analog TV
> > > screen? Nope. I don't have that level of nostalgia.
> > >
> >
> > The point is actually to let us unexport or delete mtrr_add.
>
> Understood.
>
>
> > We can
> > either severely regress performance on ivtv on PAT-capable hardware if
> > we naively switch it to arch_phys_wc_add or we can do something else.
> > The something else remains to be determined.
>
> Maybe ioremap the decoder register area as UC, and ioremap the rest of
> the decoder region to WC. (Does that suck up too many PAT resources?
PAT resources are unlimited.
> Then add PCI reads following any sort of singleton PCI writes in the WC
> region. I assume PCI rules about write postings before reads still
> apply when WC is set.
>
I think we need sfence, too, but that's easy. We also lose the write
sizes. That is, adjacent writes get combined. Maybe that's okay.
> > >
> > > We sort of know where some things are in the MMIO space due to
> > > experimentation and past efforts examining the firmware binary.
> > >
> > > Documentation/video4linux/cx2341x/fw-*.txt documents some things. The
> > > driver code actually codifies a little bit more knowledge.
> > >
> > > The driver code for doing transfers between host and card is complex and
> > > fragile with some streams that use DMA, other streams that use PIO,
> > > digging VBI data straight out of card memory, and scatter-gather being
> > > broken on newer firmwares. Playing around with ioremapping will be hard
> > > to get right and likely cause something in the code to break for the
> > > primary use case of the ivtv supported cards.
> >
> > Ick.
>
> Yeah.
>
> > If the only thing that really wants WC is the esoteric framebuffer
> > thing,
>
> That appears to be it.
>
> > could we just switch to arch_phys_wc_add and assume that no one
> > will care about the regression on new CPUs with ivtv cards?
>
> That's on the table in my mind. Not sure if it is the friendliest thing
> to do to users. Quite honestly though, modern graphics cards have much
> better ouput resolution and performance. Anyone with a modern system
> really should be using one. (i.e. MythTV gave up on support for PVR-350
> output for video playback years ago in May 2010.)
>
>
> BTW, my 2005 system with multiple conventional PCI slots in it shows a
> 'pat' flag in /proc/cpuinfo. (AMD Athlon(tm) 64 X2 Dual Core Processor
> 4200+) I didn't know it was considered "new". :)
Tons of CPUs have that ability, but we often turn it off due to errata
on older CPUs.
--Andy
>
> Regards,
> Andy
>
>
^ permalink raw reply
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Hyong-Youb Kim @ 2015-04-17 8:00 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Andy Walls, Hyong-Youb Kim, netdev, Andy Lutomirski, Toshi Kani,
H. Peter Anvin, Ingo Molnar, linux-kernel, 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,
Johannes Berg, Felix Fietkau, Benjamin Poirier, dave.hansen,
plagnioj, tglx, Ville Syrjälä, linux-fbdev, linux-media,
x86
In-Reply-To: <20150416185426.GH5622@wotan.suse.de>
On Thu, Apr 16, 2015 at 08:54:26PM +0200, Luis R. Rodriguez wrote:
> > Without WC, descriptors would end up as multiple 4B or 8B MWr packets
> > to the NIC, which has a pretty big performance impact on this
> > particular NIC.
>
> How big are the descriptors?
Some are 64B (a batch of eight 8B descriptors). Some are 16B.
> > Most registers that do not want WC are actually in BAR2, which is not
> > mapped as WC. For registers that are in BAR0, we do "write to the
> > register; wmb". If we want to wait till the NIC has seen the write,
> > we do "write; wmb; read".
>
> Interesting, thanks, yeah using this as a work around to the problem sounds
> plausible however it still would require likely making just as many changes to
> the ivtv and ipath driver as to just do a proper split. I do wonder however if
> this sort of work around can be generalized somehow though so that others could
> use, if this sort of thing is going to become prevalent. If so then this would
> serve two purposes: work around for the corner cases of MTRR use on Linux and
> also these sorts of device constraints.
These Myricom devices are very non-standard in my opinion, at least in
the Ethernet world. Few, if any, other devices depend so much on WC
like these do. I think almost all devices now have rings in host
memory. The NIC pulls them via DMA. No need for WC, and no need to
special case registers...
> In order to determine if this is likely to be generally useful could you elaborate
> a bit more about the detals of the performance issues of not bursting writes
> for the descriptor on this device.
For this particular Myricom device, performance penalty stems from the
use of slow path in the firmware. They are not about how effectively
we use PCI Express or latency or bandwidth. Small MWr packets end up
casuing slow path processing via the firmware in this device.
There are HPC low latency NICs that use WC for different reasons. To
reduce latency as much as possible, some of these copy small packets
to the NIC memory via PIO (BAR0, and so on), instead of DMA. They
want WC mapping to minimize PCI Express packets/transactions.
I do not know about video adapters and their use for WC.
> Even if that is done a conversion over to this work around seems it may require
> device specific nitpicks. For instance I note in myri10ge_submit_req() for
> small writes you just do a reverse write and do the first set last, then
> finally the last 32 bits are rewritten, I guess to trigger something?
Right. The device polls the last word to start sending, DMA, etc.
> > This approach has worked for this device for many years. I cannot say
> > whether it works for other devices, though.
>
> I think it should but the more interesting question would be exactly
> *why* it was needed for this device, who determined that, and why?
Hopefully, the above text answers some of your questions.
^ permalink raw reply
* [PATCH] video/logo: fix use logo after free prevention
From: Manfred Schlaegl @ 2015-04-17 9:48 UTC (permalink / raw)
To: Jean-Christophe Plagniol-Villard, Tomi Valkeinen
Cc: Manfred Schlaegl, linux-fbdev, linux-kernel, Manfred Schlaegl
After 92b004d1aa9f367c372511ca0330f58216b25703 the logos disappeared on
Freescale i.MX53 and i.MX6 SoC's (detected on linux-3.12.37).
This happens because the fb_find_logo function is validly called
(initdata still not freed) AFTER newly introduced latecall
fb_logo_late_init.
Instead of stetting a logos_freed flag somewhere in lateinit, this patch
uses system_state=SYSTEM_BOOTING as indication for valid initdata.
Signed-off-by: Manfred Schlaegl <manfred.schlaegl@gmx.at>
---
drivers/video/logo/logo.c | 21 +++++----------------
1 file changed, 5 insertions(+), 16 deletions(-)
diff --git a/drivers/video/logo/logo.c b/drivers/video/logo/logo.c
index 10fbfd8..ad37561 100644
--- a/drivers/video/logo/logo.c
+++ b/drivers/video/logo/logo.c
@@ -21,21 +21,6 @@ static bool nologo;
module_param(nologo, bool, 0);
MODULE_PARM_DESC(nologo, "Disables startup logo");
-/*
- * Logos are located in the initdata, and will be freed in kernel_init.
- * Use late_init to mark the logos as freed to prevent any further use.
- */
-
-static bool logos_freed;
-
-static int __init fb_logo_late_init(void)
-{
- logos_freed = true;
- return 0;
-}
-
-late_initcall(fb_logo_late_init);
-
/* logo's are marked __initdata. Use __init_refok to tell
* modpost that it is intended that this function uses data
* marked __initdata.
@@ -44,7 +29,11 @@ const struct linux_logo * __init_refok fb_find_logo(int depth)
{
const struct linux_logo *logo = NULL;
- if (nologo || logos_freed)
+ /*
+ * Logos are located in the initdata, and will be freed in kernel_init.
+ * Use system_state to determine, if initdata is still useable.
+ */
+ if (nologo || system_state != SYSTEM_BOOTING)
return NULL;
if (depth >= 1) {
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH] video/logo: fix use logo after free prevention
From: Tomi Valkeinen @ 2015-04-17 10:40 UTC (permalink / raw)
To: Manfred Schlaegl, Jean-Christophe Plagniol-Villard
Cc: linux-fbdev, linux-kernel, Manfred Schlaegl
In-Reply-To: <5530D6D2.8080103@gmx.at>
Hi,
On 04/17/2015 12:48 PM, Manfred Schlaegl wrote:
> After 92b004d1aa9f367c372511ca0330f58216b25703 the logos disappeared on
> Freescale i.MX53 and i.MX6 SoC's (detected on linux-3.12.37).
> This happens because the fb_find_logo function is validly called
> (initdata still not freed) AFTER newly introduced latecall
> fb_logo_late_init.
>
> Instead of stetting a logos_freed flag somewhere in lateinit, this patch
> uses system_state=SYSTEM_BOOTING as indication for valid initdata.
The kernel init does free_initmem() call before setting the system_state
to SYSTEM_RUNNING, so there's a period of time when the logos are freed,
but the check in you patch does not catch it.
Tomi
^ permalink raw reply
* [PATCH] staging: sm750fb: Cleaning up a few return statements
From: Julian Gindi @ 2015-04-17 20:44 UTC (permalink / raw)
To: linux-fbdev
Signed-off-by: Julian Gindi <juliangindi@gmail.com>
---
drivers/staging/sm750fb/ddk750_chip.c | 4 +---
drivers/staging/sm750fb/sm750.c | 6 +-----
drivers/staging/sm750fb/sm750_hw.c | 5 +----
3 files changed, 3 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/sm750fb/ddk750_chip.c b/drivers/staging/sm750fb/ddk750_chip.c
index 7b28328..ac88623 100644
--- a/drivers/staging/sm750fb/ddk750_chip.c
+++ b/drivers/staging/sm750fb/ddk750_chip.c
@@ -499,7 +499,6 @@ pll_value_t *pPLL /* Structure to hold the value to be set in PLL */
{
unsigned int M, N, OD, POD = 0, diff, pllClk, odPower, podPower;
unsigned int bestDiff = 0xffffffff; /* biggest 32 bit unsigned number */
- unsigned int ret;
/* Init PLL structure to know states */
pPLL->M = 0;
pPLL->N = 0;
@@ -589,8 +588,7 @@ pll_value_t *pPLL /* Structure to hold the value to be set in PLL */
//DDKDEBUGPRINT((DISPLAY_LEVEL, "calcPllValue: Input CLK = %dHz, M=%d, N=%d, OD=%d, POD=%d\n", pPLL->inputFreq, pPLL->M, pPLL->N, pPLL->OD, pPLL->POD));
/* Return actual frequency that the PLL can set */
- ret = calcPLL(pPLL);
- return ret;
+ return calcPLL(pPLL);
}
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index 3c7ea95..e05bb64 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -303,8 +303,6 @@ static int lynxfb_ops_pan_display(struct fb_var_screeninfo *var,
{
struct lynxfb_par *par;
struct lynxfb_crtc *crtc;
- int ret;
-
if (!info)
return -EINVAL;
@@ -312,9 +310,7 @@ static int lynxfb_ops_pan_display(struct fb_var_screeninfo *var,
ret = 0;
par = info->par;
crtc = &par->crtc;
- ret = crtc->proc_panDisplay(crtc, var, info);
-
- return ret;
+ return crtc->proc_panDisplay(crtc, var, info);
}
static int lynxfb_ops_set_par(struct fb_info *info)
diff --git a/drivers/staging/sm750fb/sm750_hw.c b/drivers/staging/sm750fb/sm750_hw.c
index 9f0d06d..8f2432d 100644
--- a/drivers/staging/sm750fb/sm750_hw.c
+++ b/drivers/staging/sm750fb/sm750_hw.c
@@ -203,10 +203,7 @@ int hw_sm750_inithw(struct lynx_share* share, struct pci_dev * pdev)
resource_size_t hw_sm750_getVMSize(struct lynx_share * share)
{
- resource_size_t ret;
-
- ret = ddk750_getVMSize();
- return ret;
+ return ddk750_getVMSize();
}
--
1.9.1
^ permalink raw reply related
* what's the difference between smem_start and mmio_start?
From: z f @ 2015-04-18 7:46 UTC (permalink / raw)
To: linux-fbdev
what's the difference between smem_start member and mmio_start member
in struct fb_fix_screeninfo?
I have looked the fb_mmap() and cant understand its behavior:
http://lxr.free-electrons.com/source/drivers/video/fbdev/core/fbmem.c#L1413
^ permalink raw reply
* Question abouf FB actual size
From: Paolo Niccolò Giubelli @ 2015-04-19 17:39 UTC (permalink / raw)
To: linux-fbdev
Hello world!
I'm developing a simple city simulator in C, and I'd like to draw the
city map on the framebuffer (it's just for debugging purposes).
There's something wrong with the math, because i have a 1366x768x32
FB, but my finfo.line_length (finfo is obtained via
FBIOGET_FSCREENINFO) is 5504 bytes. Well, 5504/4 (4 is 4 bytes per
pixel) is 1376 that is 10 pixels more than 1366.
Actually if I draw a full line using 5504 bytes everything is ok, if I
use 1366*4 I don't reach the edge of the monitor.
I'm using memset on a memory mapped /dev/fb0 file.
Is there something wrong or am I missing something?
Thank you in advance.
^ permalink raw reply
* Re: Question abouf FB actual size
From: Geert Uytterhoeven @ 2015-04-19 19:29 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <CAFAVkqVPVG_h+Kwq-KjdivMWh+2WthJdJvQqRcf1m-qjHTpX1A@mail.gmail.com>
On Sun, Apr 19, 2015 at 7:39 PM, Paolo Niccolò Giubelli
<paoloniccolo.giubelli@gmail.com> wrote:
> There's something wrong with the math, because i have a 1366x768x32
> FB, but my finfo.line_length (finfo is obtained via
> FBIOGET_FSCREENINFO) is 5504 bytes. Well, 5504/4 (4 is 4 bytes per
> pixel) is 1376 that is 10 pixels more than 1366.
1366 * 4 is not a multiple of 64. 5504 is.
I guess your hardware requires each line to start at an address which is a
multiple of 64.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: Question abouf FB actual size
From: Paolo Niccolò Giubelli @ 2015-04-19 20:01 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <CAFAVkqVPVG_h+Kwq-KjdivMWh+2WthJdJvQqRcf1m-qjHTpX1A@mail.gmail.com>
Thank you Geert! So I guess there's some kind of padding after the end
of the line, where I can write anything that has no effect. Do you
know if there's something to check into fb.h so to guess the alignment
requirements?
Greetings,
Paolo Niccolò
On Sun, Apr 19, 2015 at 9:29 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Sun, Apr 19, 2015 at 7:39 PM, Paolo Niccolò Giubelli
> <paoloniccolo.giubelli@gmail.com> wrote:
>> There's something wrong with the math, because i have a 1366x768x32
>> FB, but my finfo.line_length (finfo is obtained via
>> FBIOGET_FSCREENINFO) is 5504 bytes. Well, 5504/4 (4 is 4 bytes per
>> pixel) is 1376 that is 10 pixels more than 1366.
>
> 1366 * 4 is not a multiple of 64. 5504 is.
> I guess your hardware requires each line to start at an address which is a
> multiple of 64.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
^ permalink raw reply
* Re: Question abouf FB actual size
From: Geert Uytterhoeven @ 2015-04-19 20:15 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <CAFAVkqVPVG_h+Kwq-KjdivMWh+2WthJdJvQqRcf1m-qjHTpX1A@mail.gmail.com>
On Sun, Apr 19, 2015 at 10:01 PM, Paolo Niccolò Giubelli
<paoloniccolo.giubelli@gmail.com> wrote:
> On Sun, Apr 19, 2015 at 9:29 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Sun, Apr 19, 2015 at 7:39 PM, Paolo Niccolò Giubelli
>> <paoloniccolo.giubelli@gmail.com> wrote:
>>> There's something wrong with the math, because i have a 1366x768x32
>>> FB, but my finfo.line_length (finfo is obtained via
>>> FBIOGET_FSCREENINFO) is 5504 bytes. Well, 5504/4 (4 is 4 bytes per
>>> pixel) is 1376 that is 10 pixels more than 1366.
>>
>> 1366 * 4 is not a multiple of 64. 5504 is.
>> I guess your hardware requires each line to start at an address which is a
>> multiple of 64.
>
> Thank you Geert! So I guess there's some kind of padding after the end
> of the line, where I can write anything that has no effect. Do you
> know if there's something to check into fb.h so to guess the alignment
> requirements?
The only way to find out is to look at fb_fix_screeninfo.line_length.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: Question abouf FB actual size
From: Paolo Niccolò Giubelli @ 2015-04-19 20:20 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <CAFAVkqVPVG_h+Kwq-KjdivMWh+2WthJdJvQqRcf1m-qjHTpX1A@mail.gmail.com>
Perfect! Thank you so much, you've been very precious! :-D
Greetings,
Paolo
On Sun, Apr 19, 2015 at 10:15 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Sun, Apr 19, 2015 at 10:01 PM, Paolo Niccolò Giubelli
> <paoloniccolo.giubelli@gmail.com> wrote:
>> On Sun, Apr 19, 2015 at 9:29 PM, Geert Uytterhoeven
>> <geert@linux-m68k.org> wrote:
>>> On Sun, Apr 19, 2015 at 7:39 PM, Paolo Niccolò Giubelli
>>> <paoloniccolo.giubelli@gmail.com> wrote:
>>>> There's something wrong with the math, because i have a 1366x768x32
>>>> FB, but my finfo.line_length (finfo is obtained via
>>>> FBIOGET_FSCREENINFO) is 5504 bytes. Well, 5504/4 (4 is 4 bytes per
>>>> pixel) is 1376 that is 10 pixels more than 1366.
>>>
>>> 1366 * 4 is not a multiple of 64. 5504 is.
>>> I guess your hardware requires each line to start at an address which is a
>>> multiple of 64.
>>
>> Thank you Geert! So I guess there's some kind of padding after the end
>> of the line, where I can write anything that has no effect. Do you
>> know if there's something to check into fb.h so to guess the alignment
>> requirements?
>
> The only way to find out is to look at fb_fix_screeninfo.line_length.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
^ permalink raw reply
* [GIT PULL] fbdev changes for 4.1
From: Tomi Valkeinen @ 2015-04-20 12:39 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Linux Fbdev development list, LKML
[-- Attachment #1: Type: text/plain, Size: 4017 bytes --]
Hi Linus,
Please pull fbdev changes for 4.1.
Tomi
The following changes since commit c517d838eb7d07bbe9507871fab3931deccff539:
Linux 4.0-rc1 (2015-02-22 18:21:14 -0800)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git tags/fbdev-4.1
for you to fetch changes up to 6b75b54c841a18ef114704aa5cf2cdf43487b0ae:
Merge omapdss topic branch for fbdev 4.1 (2015-04-20 12:09:31 +0300)
----------------------------------------------------------------
fbdev changes for v4.1
* Small fixes and improvements to various fbdev drivers
----------------------------------------------------------------
Dan Carpenter (1):
fbdev: pm3fb: cleanup some confusing indenting
Geert Uytterhoeven (1):
fbdev: sh_mobile_lcdc: Fix destruction of uninitialized mutex
Grygorii Strashko (1):
omapdss: extend pm notifier to handle hibernation
Javier Martinez Canillas (1):
OMAPDSS: Correct video ports description file path in DT binding doc
Lad, Prabhakar (1):
fbdev: via/via_clock: fix sparse warning
Laurent Pinchart (1):
video: fbdev: sh_mobile_lcdcfb: Fix ROP3 sysfs attribute parsing
Nicholas Mc Guire (2):
video: fbdev: use msecs_to_jiffies for time conversions
hyperv: hyperv_fb: match wait_for_completion_timeout return type
Sanjeev Sharma (1):
video: fbdev: make of_device_id array const
Sudip Mukherjee (1):
fbdev: sm501fb: use memset_io
Tomi Valkeinen (15):
OMAPDSS: fix AM43xx minimum pixel clock divider
OMAPDSS: HDMI5: Increase DDC SDA-HOLD time
OMAPDSS: fix paddr check for TILER addresses
OMAPDSS: TFP410: fix input sync signals
OMAPDSS: DISPC: remove OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES
OMAPDSS: DISPC: explicit handling for sync and de levels
OMAPDSS: change signal_level & signal_edge enum values
OMAPDSS: DISPC: change sync_pclk_edge default value
OMAPDSS: DISPC: fix div by zero issue in overlay scaling
OMAPDSS: DISPC: lock access to DISPC_CONTROL & DISPC_CONFIG
OMAPDSS: setup default fifo thresholds
OMAPDSS: Add support for MFLAG
OMAPDSS: workaround for MFLAG + NV12 issue
OMAPDSS: disable VT switch
Merge omapdss topic branch for fbdev 4.1
.../devicetree/bindings/video/ti,omap-dss.txt | 4 +-
drivers/gpu/drm/omapdrm/omap_connector.c | 2 +-
drivers/video/fbdev/hyperv_fb.c | 6 +-
drivers/video/fbdev/imxfb.c | 2 +-
.../video/fbdev/omap2/displays-new/connector-dvi.c | 2 +-
.../fbdev/omap2/displays-new/encoder-tfp410.c | 11 ++
.../omap2/displays-new/panel-lgphilips-lb035q02.c | 2 +-
.../omap2/displays-new/panel-sharp-ls037v7dw01.c | 2 +-
.../omap2/displays-new/panel-sony-acx565akm.c | 2 +-
.../omap2/displays-new/panel-tpo-td028ttec1.c | 2 +-
.../omap2/displays-new/panel-tpo-td043mtea1.c | 2 +-
drivers/video/fbdev/omap2/dss/core.c | 4 +
drivers/video/fbdev/omap2/dss/dispc.c | 147 +++++++++++++++++++--
drivers/video/fbdev/omap2/dss/display.c | 2 +-
drivers/video/fbdev/omap2/dss/dsi.c | 2 +-
drivers/video/fbdev/omap2/dss/dss.c | 3 +
drivers/video/fbdev/omap2/dss/dss_features.c | 2 +-
drivers/video/fbdev/omap2/dss/hdmi5_core.c | 2 +-
drivers/video/fbdev/omap2/dss/rfbi.c | 2 +-
drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 4 +-
drivers/video/fbdev/pm3fb.c | 6 +-
drivers/video/fbdev/pxafb.c | 6 +-
drivers/video/fbdev/sh_mobile_lcdcfb.c | 15 ++-
drivers/video/fbdev/sm501fb.c | 2 +-
drivers/video/fbdev/via/via_clock.c | 2 +-
include/video/omapdss.h | 7 +-
26 files changed, 193 insertions(+), 50 deletions(-)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR
From: Luis R. Rodriguez @ 2015-04-21 17:35 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Andy Walls, 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 development list,
linux-media, X86 ML, Greg Kroah-Hartman, mcgrof
In-Reply-To: <CALCETrWRjGYqcYPNizrbiVFwFHhrLf=8NTTCLVZh7Q6MgAWj=Q@mail.gmail.com>
On Wed, Apr 15, 2015 at 05:58:14PM -0700, Andy Lutomirski wrote:
> On Wed, Apr 15, 2015 at 4:59 PM, Andy Walls <awalls@md.metrocast.net> wrote:
> > On Wed, 2015-04-15 at 16:42 -0700, Andy Lutomirski wrote:
> >> On Wed, Apr 15, 2015 at 3:38 PM, Andy Walls <awalls@md.metrocast.net> wrote:
> >
> >> >
> >>
> >> IMO the right solution would be to avoid ioremapping the whole bar at
> >> startup. Instead ioremap pieces once the driver learns what they are.
> >> This wouldn't have any of these problems -- you'd ioremap() register
> >> regions and you'd ioremap_wc() the framebuffer once you find it. If
> >> there are regions of unknown purpose, just don't map them all.
> >>
> >> Would this be feasible?
> >
> > Feasible? Maybe.
> >
> > Worth the time and effort for end-of-life, convential PCI hardware so I
> > can have an optimally performing X display on a Standard Def Analog TV
> > screen? Nope. I don't have that level of nostalgia.
> >
>
> The point is actually to let us unexport or delete mtrr_add. We can
> either severely regress performance on ivtv on PAT-capable hardware if
> we naively switch it to arch_phys_wc_add or we can do something else.
> The something else remains to be determined.
Back to square one: I can't come up with anything not too instrusive
or that dotes not requires substantial amount of work as an alternative to
removing MTRR completely right now (with the long term goal of also
making strong UC default) and its because of 2 device drivers:
* ivtv: firmware API is poo and device is legacy, no one cares
* ipath: driver needs a clean split and work is considerable,
maintainers have not been responsive, do they care?
What do we want to do with these drivers? Let us be straight shooters,
if we are serious about having a performance regression on the drivers
for the sake of removing MTRR why not just seriously discuss removal
of these drivers. This way we can remain sane, upkeep a policy to
never even consider overlapping ioremap*() calls, and have a clean
expected strategy we can expect for new drivers.
I'm going to split up my patches now into 4 series:
1) things which are straight forward in converting drivers over
to arch_phys_wc_add() and ioremap_wc(). These are subsystem
wide though, so just a heads up, my hope is that each subsystem
maintainer can take their own series unless someone else is
comfortable in taking this in for x86
2) a few helpers in the like of ioremap_wc() needed for other drivers.
These are straight forward but since they depend on x86 / core
helpers it would be nice for them to go I guess through x86 folks.
What maintainer is up to take these?
3) MTRR run time changes
4) corner cases - TBD - lets discuss here what we want to do with
ivtv and ipath. I will however remove fusion's mtrr code use
as its all commented out.
Luis
^ permalink raw reply
* Re: [PATCH v1 05/47] pci: add pci_iomap_wc() variants
From: Michael S. Tsirkin @ 2015-04-21 19:25 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Luis R. Rodriguez, Andy Lutomirski, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, jgross, Jan Beulich, Borislav Petkov,
Suresh Siddha, venkatesh.pallipadi, Dave Airlie,
linux-kernel@vger.kernel.org, linux-fbdev, x86@kernel.org,
xen-devel@lists.xenproject.org, Luis R. Rodriguez, Ingo Molnar,
Daniel Vetter, Antonino Daplas, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, Dave Hansen, Arnd Bergmann, Stefan Bader,
Konrad Rzeszutek Wilk, Ville Syrjälä, David Vrabel,
Toshi Kani, Roger Pau Monné, xen-devel
In-Reply-To: <CAErSpo5LfgTM879jmt58yLrXD6e7spsaX1wNDcc2ppU9JrMXOg@mail.gmail.com>
On Mon, Mar 23, 2015 at 12:20:47PM -0500, Bjorn Helgaas wrote:
> pci_iomap_range() already makes a cacheable mapping if
> IORESOURCE_CACHEABLE; I'm guessing that you would like it to
> automatically use WC if the BAR if IORESOURCE_PREFETCH, e.g.,
>
> if (flags & IORESOURCE_CACHEABLE)
> return ioremap(start, len);
> if (flags & IORESOURCE_PREFETCH)
> return ioremap_wc(start, len);
> return ioremap_nocache(start, len);
>
> Is there a reason not to do that?
I think that's wrong and will break a bunch of things.
PCI prefetch bit merely means bridges can combine writes and prefetch
reads. Prefetch does not affect ordering rules and does not allow
writes to be collapsed.
WC is stronger: it allows collapsing and changes ordering rules.
WC can also hurt latency as small writes are buffered.
To summarise, driver needs to know what it's doing,
we can't set WC in the pci core automatically.
--
MST
^ permalink raw reply
* Re: [PATCH v1 05/47] pci: add pci_iomap_wc() variants
From: Luis R. Rodriguez @ 2015-04-21 19:27 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Bjorn Helgaas, Andy Lutomirski, 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, x86@kernel.org,
xen-devel@lists.xenproject.org, Ingo Molnar, Daniel Vetter,
Antonino Daplas, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Dave Hansen, Arnd Bergmann, Stefan Bader, Konrad Rzeszutek Wilk,
Ville Syrjälä, David Vrabel, Toshi Kani,
Roger Pau Monné, xen-devel
In-Reply-To: <20150421211557-mutt-send-email-mst@redhat.com>
On Tue, Apr 21, 2015 at 12:25 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> To summarise, driver needs to know what it's doing,
> we can't set WC in the pci core automatically.
Thanks, I'll document this and proceed with device driver helpers to
aid with this.
Luis
^ permalink raw reply
* [PATCH v3] staging: sm750fb: use arch_phys_wc_add() and ioremap_wc()
From: Luis R. Rodriguez @ 2015-04-21 20:12 UTC (permalink / raw)
To: cocci
From: "Luis R. Rodriguez" <mcgrof@suse.com>
The same area used for ioremap() is used for the MTRR area.
Convert the driver from using the x86 specific MTRR code to
the architecture agnostic arch_phys_wc_add(). arch_phys_wc_add()
will avoid MTRR if write-combining is available, in order to
take advantage of that also ensure the ioremap'd area is requested
as write-combining.
There are a few motivations for this:
a) Take advantage of PAT when available
b) Help bury MTRR code away, MTRR is architecture specific and on
x86 its replaced by PAT
c) 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()")
The conversion done is expressed by the following Coccinelle
SmPL patch, it additionally required manual intervention to
address all the #ifdery and removal of redundant things which
arch_phys_wc_add() already addresses such as verbose message
about when MTRR fails and doing nothing when we didn't get
an MTRR.
@ mtrr_found @
expression index, base, size;
@@
-index = mtrr_add(base, size, MTRR_TYPE_WRCOMB, 1);
+index = arch_phys_wc_add(base, size);
@ mtrr_rm depends on mtrr_found @
expression mtrr_found.index, mtrr_found.base, mtrr_found.size;
@@
-mtrr_del(index, base, size);
+arch_phys_wc_del(index);
@ mtrr_rm_zero_arg depends on mtrr_found @
expression mtrr_found.index;
@@
-mtrr_del(index, 0, 0);
+arch_phys_wc_del(index);
@ mtrr_rm_fb_info depends on mtrr_found @
struct fb_info *info;
expression mtrr_found.index;
@@
-mtrr_del(index, info->fix.smem_start, info->fix.smem_len);
+arch_phys_wc_del(index);
@ ioremap_replace_nocache depends on mtrr_found @
struct fb_info *info;
expression base, size;
@@
-info->screen_base = ioremap_nocache(base, size);
+info->screen_base = ioremap_wc(base, size);
@ ioremap_replace_default depends on mtrr_found @
struct fb_info *info;
expression base, size;
@@
-info->screen_base = ioremap(base, size);
+info->screen_base = ioremap_wc(base, size);
Generated-by: Coccinelle SmPL
Cc: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
Cc: Teddy Wang <teddy.wang@siliconmotion.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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: devel@driverdev.osuosl.org
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
drivers/staging/sm750fb/sm750.c | 36 ++++--------------------------------
drivers/staging/sm750fb/sm750.h | 3 ---
drivers/staging/sm750fb/sm750_hw.c | 3 +--
3 files changed, 5 insertions(+), 37 deletions(-)
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index 3c7ea95..cf57e3e 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -16,9 +16,6 @@
#include<linux/vmalloc.h>
#include<linux/pagemap.h>
#include <linux/console.h>
-#ifdef CONFIG_MTRR
-#include <asm/mtrr.h>
-#endif
#include <asm/fb.h>
#include "sm750.h"
#include "sm750_hw.h"
@@ -47,9 +44,7 @@ typedef int (*PROC_SPEC_INITHW)(struct lynx_share*, struct pci_dev*);
/* common var for all device */
static int g_hwcursor = 1;
static int g_noaccel;
-#ifdef CONFIG_MTRR
static int g_nomtrr;
-#endif
static const char *g_fbmode[] = {NULL, NULL};
static const char *g_def_fbmode = "800x600-16@60";
static char *g_settings = NULL;
@@ -1126,11 +1121,8 @@ static int lynxfb_pci_probe(struct pci_dev *pdev,
pr_info("share->revid = %02x\n", share->revid);
share->pdev = pdev;
-#ifdef CONFIG_MTRR
share->mtrr_off = g_nomtrr;
share->mtrr.vram = 0;
- share->mtrr.vram_added = 0;
-#endif
share->accel_off = g_noaccel;
share->dual = g_dualview;
spin_lock_init(&share->slock);
@@ -1158,22 +1150,9 @@ static int lynxfb_pci_probe(struct pci_dev *pdev,
goto err_map;
}
-#ifdef CONFIG_MTRR
- if (!share->mtrr_off) {
- pr_info("enable mtrr\n");
- share->mtrr.vram = mtrr_add(share->vidmem_start,
- share->vidmem_size,
- MTRR_TYPE_WRCOMB, 1);
-
- if (share->mtrr.vram < 0) {
- /* don't block driver with the failure of MTRR */
- pr_err("Unable to setup MTRR.\n");
- } else {
- share->mtrr.vram_added = 1;
- pr_info("MTRR added succesfully\n");
- }
- }
-#endif
+ if (!share->mtrr_off)
+ share->mtrr.vram = arch_phys_wc_add(share->vidmem_start,
+ share->vidmem_size);
memset_io(share->pvMem, 0, share->vidmem_size);
@@ -1274,12 +1253,7 @@ static void __exit lynxfb_pci_remove(struct pci_dev *pdev)
/* release frame buffer */
framebuffer_release(info);
}
-#ifdef CONFIG_MTRR
- if (share->mtrr.vram_added)
- mtrr_del(share->mtrr.vram,
- share->vidmem_start,
- share->vidmem_size);
-#endif
+ arch_phys_wc_del(share->mtrr.vram);
iounmap(share->pvReg);
iounmap(share->pvMem);
@@ -1321,10 +1295,8 @@ static int __init lynxfb_setup(char *options)
/* options that mean for any lynx chips are configured here */
if (!strncmp(opt, "noaccel", strlen("noaccel")))
g_noaccel = 1;
-#ifdef CONFIG_MTRR
else if (!strncmp(opt, "nomtrr", strlen("nomtrr")))
g_nomtrr = 1;
-#endif
else if (!strncmp(opt, "dual", strlen("dual")))
g_dualview = 1;
else {
diff --git a/drivers/staging/sm750fb/sm750.h b/drivers/staging/sm750fb/sm750.h
index 0847d2b..5528912 100644
--- a/drivers/staging/sm750fb/sm750.h
+++ b/drivers/staging/sm750fb/sm750.h
@@ -51,13 +51,10 @@ struct lynx_share{
struct lynx_accel accel;
int accel_off;
int dual;
-#ifdef CONFIG_MTRR
int mtrr_off;
struct{
int vram;
- int vram_added;
}mtrr;
-#endif
/* all smi graphic adaptor got below attributes */
unsigned long vidmem_start;
unsigned long vidreg_start;
diff --git a/drivers/staging/sm750fb/sm750_hw.c b/drivers/staging/sm750fb/sm750_hw.c
index 9f0d06d..4b77eb1 100644
--- a/drivers/staging/sm750fb/sm750_hw.c
+++ b/drivers/staging/sm750fb/sm750_hw.c
@@ -85,8 +85,7 @@ int hw_sm750_map(struct lynx_share* share, struct pci_dev* pdev)
}
#endif
- share->pvMem = ioremap(share->vidmem_start,
- share->vidmem_size);
+ share->pvMem = ioremap_wc(share->vidmem_start, share->vidmem_size);
if(!share->pvMem){
pr_err("Map video memory failed\n");
--
2.3.2.209.gd67f9d5.dirty
^ permalink raw reply related
* [PATCH v3] staging: sm750fb: use arch_phys_wc_add() and ioremap_wc()
From: Luis R. Rodriguez @ 2015-04-21 20:13 UTC (permalink / raw)
To: cocci
In-Reply-To: <1429647123-16777-1-git-send-email-mcgrof@do-not-panic.com>
From: "Luis R. Rodriguez" <mcgrof@suse.com>
The same area used for ioremap() is used for the MTRR area.
Convert the driver from using the x86 specific MTRR code to
the architecture agnostic arch_phys_wc_add(). arch_phys_wc_add()
will avoid MTRR if write-combining is available, in order to
take advantage of that also ensure the ioremap'd area is requested
as write-combining.
There are a few motivations for this:
a) Take advantage of PAT when available
b) Help bury MTRR code away, MTRR is architecture specific and on
x86 its replaced by PAT
c) 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()")
The conversion done is expressed by the following Coccinelle
SmPL patch, it additionally required manual intervention to
address all the #ifdery and removal of redundant things which
arch_phys_wc_add() already addresses such as verbose message
about when MTRR fails and doing nothing when we didn't get
an MTRR.
@ mtrr_found @
expression index, base, size;
@@
-index = mtrr_add(base, size, MTRR_TYPE_WRCOMB, 1);
+index = arch_phys_wc_add(base, size);
@ mtrr_rm depends on mtrr_found @
expression mtrr_found.index, mtrr_found.base, mtrr_found.size;
@@
-mtrr_del(index, base, size);
+arch_phys_wc_del(index);
@ mtrr_rm_zero_arg depends on mtrr_found @
expression mtrr_found.index;
@@
-mtrr_del(index, 0, 0);
+arch_phys_wc_del(index);
@ mtrr_rm_fb_info depends on mtrr_found @
struct fb_info *info;
expression mtrr_found.index;
@@
-mtrr_del(index, info->fix.smem_start, info->fix.smem_len);
+arch_phys_wc_del(index);
@ ioremap_replace_nocache depends on mtrr_found @
struct fb_info *info;
expression base, size;
@@
-info->screen_base = ioremap_nocache(base, size);
+info->screen_base = ioremap_wc(base, size);
@ ioremap_replace_default depends on mtrr_found @
struct fb_info *info;
expression base, size;
@@
-info->screen_base = ioremap(base, size);
+info->screen_base = ioremap_wc(base, size);
Generated-by: Coccinelle SmPL
Cc: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
Cc: Teddy Wang <teddy.wang@siliconmotion.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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: devel@driverdev.osuosl.org
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
drivers/staging/sm750fb/sm750.c | 36 ++++--------------------------------
drivers/staging/sm750fb/sm750.h | 3 ---
drivers/staging/sm750fb/sm750_hw.c | 3 +--
3 files changed, 5 insertions(+), 37 deletions(-)
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index 3c7ea95..cf57e3e 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -16,9 +16,6 @@
#include<linux/vmalloc.h>
#include<linux/pagemap.h>
#include <linux/console.h>
-#ifdef CONFIG_MTRR
-#include <asm/mtrr.h>
-#endif
#include <asm/fb.h>
#include "sm750.h"
#include "sm750_hw.h"
@@ -47,9 +44,7 @@ typedef int (*PROC_SPEC_INITHW)(struct lynx_share*, struct pci_dev*);
/* common var for all device */
static int g_hwcursor = 1;
static int g_noaccel;
-#ifdef CONFIG_MTRR
static int g_nomtrr;
-#endif
static const char *g_fbmode[] = {NULL, NULL};
static const char *g_def_fbmode = "800x600-16@60";
static char *g_settings = NULL;
@@ -1126,11 +1121,8 @@ static int lynxfb_pci_probe(struct pci_dev *pdev,
pr_info("share->revid = %02x\n", share->revid);
share->pdev = pdev;
-#ifdef CONFIG_MTRR
share->mtrr_off = g_nomtrr;
share->mtrr.vram = 0;
- share->mtrr.vram_added = 0;
-#endif
share->accel_off = g_noaccel;
share->dual = g_dualview;
spin_lock_init(&share->slock);
@@ -1158,22 +1150,9 @@ static int lynxfb_pci_probe(struct pci_dev *pdev,
goto err_map;
}
-#ifdef CONFIG_MTRR
- if (!share->mtrr_off) {
- pr_info("enable mtrr\n");
- share->mtrr.vram = mtrr_add(share->vidmem_start,
- share->vidmem_size,
- MTRR_TYPE_WRCOMB, 1);
-
- if (share->mtrr.vram < 0) {
- /* don't block driver with the failure of MTRR */
- pr_err("Unable to setup MTRR.\n");
- } else {
- share->mtrr.vram_added = 1;
- pr_info("MTRR added succesfully\n");
- }
- }
-#endif
+ if (!share->mtrr_off)
+ share->mtrr.vram = arch_phys_wc_add(share->vidmem_start,
+ share->vidmem_size);
memset_io(share->pvMem, 0, share->vidmem_size);
@@ -1274,12 +1253,7 @@ static void __exit lynxfb_pci_remove(struct pci_dev *pdev)
/* release frame buffer */
framebuffer_release(info);
}
-#ifdef CONFIG_MTRR
- if (share->mtrr.vram_added)
- mtrr_del(share->mtrr.vram,
- share->vidmem_start,
- share->vidmem_size);
-#endif
+ arch_phys_wc_del(share->mtrr.vram);
iounmap(share->pvReg);
iounmap(share->pvMem);
@@ -1321,10 +1295,8 @@ static int __init lynxfb_setup(char *options)
/* options that mean for any lynx chips are configured here */
if (!strncmp(opt, "noaccel", strlen("noaccel")))
g_noaccel = 1;
-#ifdef CONFIG_MTRR
else if (!strncmp(opt, "nomtrr", strlen("nomtrr")))
g_nomtrr = 1;
-#endif
else if (!strncmp(opt, "dual", strlen("dual")))
g_dualview = 1;
else {
diff --git a/drivers/staging/sm750fb/sm750.h b/drivers/staging/sm750fb/sm750.h
index 0847d2b..5528912 100644
--- a/drivers/staging/sm750fb/sm750.h
+++ b/drivers/staging/sm750fb/sm750.h
@@ -51,13 +51,10 @@ struct lynx_share{
struct lynx_accel accel;
int accel_off;
int dual;
-#ifdef CONFIG_MTRR
int mtrr_off;
struct{
int vram;
- int vram_added;
}mtrr;
-#endif
/* all smi graphic adaptor got below attributes */
unsigned long vidmem_start;
unsigned long vidreg_start;
diff --git a/drivers/staging/sm750fb/sm750_hw.c b/drivers/staging/sm750fb/sm750_hw.c
index 9f0d06d..4b77eb1 100644
--- a/drivers/staging/sm750fb/sm750_hw.c
+++ b/drivers/staging/sm750fb/sm750_hw.c
@@ -85,8 +85,7 @@ int hw_sm750_map(struct lynx_share* share, struct pci_dev* pdev)
}
#endif
- share->pvMem = ioremap(share->vidmem_start,
- share->vidmem_size);
+ share->pvMem = ioremap_wc(share->vidmem_start, share->vidmem_size);
if(!share->pvMem){
pr_err("Map video memory failed\n");
--
2.3.2.209.gd67f9d5.dirty
^ permalink raw reply related
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