From: "Luis R. Rodriguez" <[email protected]>
We are burrying direct access to MTRR code support on
x86 in order to take advantage of PAT. In the future we
also want to make the default behaviour of ioremap_nocache()
to use strong UC, use of mtrr_add() on those systems
would make write-combining void.
In order to help both enable us to later make strong
UC default and in order to phase out direct MTRR access
code port the driver over to arch_phys_wc_add() and
annotate that the device driver requires systems to
boot with PAT disabled, with the nopat kernel parameter.
This is a worthy comprmise given that the hardware is
really rare these days, and perhaps only some lost souls
in some third world country are expected to be using this
feature of the device driver.
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Andy Walls <[email protected]>
Cc: Suresh Siddha <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: Dave Airlie <[email protected]>
Cc: Bjorn Helgaas <[email protected]>
Cc: Antonino Daplas <[email protected]>
Cc: Jean-Christophe Plagniol-Villard <[email protected]>
Cc: Tomi Valkeinen <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Michael S. Tsirkin <[email protected]>
Cc: Stefan Bader <[email protected]>
Cc: Ville Syrjälä <[email protected]>
Cc: Mel Gorman <[email protected]>
Cc: Vlastimil Babka <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Davidlohr Bueso <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Roger Pau Monné <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/media/pci/ivtv/Kconfig | 3 +++
drivers/media/pci/ivtv/ivtvfb.c | 59 +++++++++++++++++------------------------
2 files changed, 27 insertions(+), 35 deletions(-)
diff --git a/drivers/media/pci/ivtv/Kconfig b/drivers/media/pci/ivtv/Kconfig
index dd6ee57e..b2a7f88 100644
--- a/drivers/media/pci/ivtv/Kconfig
+++ b/drivers/media/pci/ivtv/Kconfig
@@ -57,5 +57,8 @@ config VIDEO_FB_IVTV
This is used in the Hauppauge PVR-350 card. There is a driver
homepage at <http://www.ivtvdriver.org>.
+ If you have this hardware you will need to boot with PAT disabled
+ on your x86 systems, use the nopat kernel parameter.
+
To compile this driver as a module, choose M here: the
module will be called ivtvfb.
diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
index 9ff1230..552408b 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,7 @@ 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
-
+ arch_phys_wc_del(oi->wc_cookie);
kfree(oi);
itv->osd_info = NULL;
}
@@ -1190,6 +1172,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;
--
2.3.2.209.gd67f9d5.dirty
Hi Luis,
Sorry for the late reply.
Thank you for the patch! See my comments below:
On Wed, 2015-04-22 at 12:33 -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <[email protected]>
>
> We are burrying direct access to MTRR code support on
> x86 in order to take advantage of PAT. In the future we
> also want to make the default behaviour of ioremap_nocache()
> to use strong UC, use of mtrr_add() on those systems
> would make write-combining void.
>
> In order to help both enable us to later make strong
> UC default and in order to phase out direct MTRR access
> code port the driver over to arch_phys_wc_add() and
> annotate that the device driver requires systems to
> boot with PAT disabled, with the nopat kernel parameter.
>
> This is a worthy comprmise given that the hardware is
> really rare these days,
I'm OK with the compromise solution. It makes sense.
> and perhaps only some lost souls
> in some third world country are expected to be using this
> feature of the device driver.
>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Andy Walls <[email protected]>
> Cc: Suresh Siddha <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Juergen Gross <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: Dave Airlie <[email protected]>
> Cc: Bjorn Helgaas <[email protected]>
> Cc: Antonino Daplas <[email protected]>
> Cc: Jean-Christophe Plagniol-Villard <[email protected]>
> Cc: Tomi Valkeinen <[email protected]>
> Cc: Dave Hansen <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Michael S. Tsirkin <[email protected]>
> Cc: Stefan Bader <[email protected]>
> Cc: Ville Syrjälä <[email protected]>
> Cc: Mel Gorman <[email protected]>
> Cc: Vlastimil Babka <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Davidlohr Bueso <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: Roger Pau Monné <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Luis R. Rodriguez <[email protected]>
> ---
> drivers/media/pci/ivtv/Kconfig | 3 +++
> drivers/media/pci/ivtv/ivtvfb.c | 59 +++++++++++++++++------------------------
> 2 files changed, 27 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/media/pci/ivtv/Kconfig b/drivers/media/pci/ivtv/Kconfig
> index dd6ee57e..b2a7f88 100644
> --- a/drivers/media/pci/ivtv/Kconfig
> +++ b/drivers/media/pci/ivtv/Kconfig
> @@ -57,5 +57,8 @@ config VIDEO_FB_IVTV
> This is used in the Hauppauge PVR-350 card. There is a driver
> homepage at <http://www.ivtvdriver.org>.
>
> + If you have this hardware you will need to boot with PAT disabled
> + on your x86 systems, use the nopat kernel parameter.
> +
> To compile this driver as a module, choose M here: the
> module will be called ivtvfb.
> diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
> index 9ff1230..552408b 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 */
Please remove this comment. It is prescriptive of a particular
solution, and probably not the one I'm going to implement. The ivtv
main driver alreay splits the encoder, decoder, and register regions
into 3 mappings. The final solution will set the whole 8 MB decoder
region mapping to WC, and then fix up all calls in the ivtvfb and ivtv
drivers where writes to the decoder memory with WC enabled could be a
problem.
Also many other places in the driver need audit in a conversion to work
with PAT, so no need to call out this one location with a comment.
> 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,7 @@ 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
> -
> + arch_phys_wc_del(oi->wc_cookie);
> kfree(oi);
> itv->osd_info = NULL;
> }
> @@ -1190,6 +1172,13 @@ static int ivtvfb_init_card(struct ivtv *itv)
> {
> int rc;
>
> +#ifdef CONFIG_X86_64
> + if (WARN(pat_enabled,
This check might be better placed in ivtvfb_init(). This check is going
to have the same result for every PVR-350 card in the system that is
found by ivtvfb.
> + "ivtv needs PAT disabled, boot with nopat kernel parameter\n")) {
This needs to read "ivtvfb needs [...]" to avoid user confusion with the
main ivtv driver module.
This change is the only one I really care about. Then I can give my
Ack.
Regards,
Andy
> + return EINVAL;
> + }
> +#endif
> +
> if (itv->osd_info) {
> IVTVFB_ERR("Card %d already initialised\n", ivtvfb_card_id);
> return -EBUSY;
On Sat, Apr 25, 2015 at 07:12:05AM -0400, Andy Walls wrote:
> Hi Luis,
>
> Sorry for the late reply.
>
> Thank you for the patch! See my comments below:
>
> On Wed, 2015-04-22 at 12:33 -0700, Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" <[email protected]>
> >
> > We are burrying direct access to MTRR code support on
> > x86 in order to take advantage of PAT. In the future we
> > also want to make the default behaviour of ioremap_nocache()
> > to use strong UC, use of mtrr_add() on those systems
> > would make write-combining void.
> >
> > In order to help both enable us to later make strong
> > UC default and in order to phase out direct MTRR access
> > code port the driver over to arch_phys_wc_add() and
> > annotate that the device driver requires systems to
> > boot with PAT disabled, with the nopat kernel parameter.
> >
> > This is a worthy comprmise given that the hardware is
> > really rare these days,
>
> I'm OK with the compromise solution. It makes sense.
OK great!
> > diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
> > index 9ff1230..552408b 100644
> > --- a/drivers/media/pci/ivtv/ivtvfb.c
> > +++ b/drivers/media/pci/ivtv/ivtvfb.c
> > @@ -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 */
>
> Please remove this comment.
Done.
> > @@ -1190,6 +1172,13 @@ static int ivtvfb_init_card(struct ivtv *itv)
> > {
> > int rc;
> >
> > +#ifdef CONFIG_X86_64
> > + if (WARN(pat_enabled,
>
> This check might be better placed in ivtvfb_init(). This check is going
> to have the same result for every PVR-350 card in the system that is
> found by ivtvfb.
OK moved!
> > + "ivtv needs PAT disabled, boot with nopat kernel parameter\n")) {
>
> This needs to read "ivtvfb needs [...]" to avoid user confusion with the
> main ivtv driver module.
OK!
> This change is the only one I really care about. Then I can give my
> Ack.
OK!
Luis