Re: ivtv: use arch_phys_wc_add() and require PAT disabled

[Nick French]

On Sat, Mar 10, 2018 at 10:20:23AM -0800, Andy Lutomirski wrote:
>>> Perhaps the easy answer is to change the fatal is-pat-enabled check to just
>>> a warning like "you have PAT enabled, so wc is disabled for the framebuffer.
>>> if you want wc, use the nopat parameter"?
>>
>> I like this idea more and more. I haven't experience any problems running
>> with PAT-enabled and no write-combining on the framebuffer. Any objections?
>>
>
> None from me.
>
> However, since you have the hardware, you could see if you can use the
> change_page_attr machinery to change the memory type on the framebuffer once
> you figure out where it is.

I am certainly willing to try this, but my understanding of the goal of the
changes that disabled ivtvfb originally is that it was trying to hide the
architecture-specific memory management from the driver.

Wouldn't (figuring out a way to) expose x86/mm/pageattr internals to the
driver be doing the opposite? (or maybe I misunderstand your suggestion)

- Nick

Re: ivtv: use arch_phys_wc_add() and require PAT disabled

[Nick French]

On Sun, Mar 11, 2018 at 01:19:03PM -0700, Andy Lutomirski wrote:
> From memory, I see two potentially reasonable real fixes. One is to find a 
> way to punch a hole in an ioremap.
> So you’d find the framebuffer, remove it from theproblematic mapping, and 
> then make a new mapping. 
> The second is to change the mapping type in place. 

For the changing-in-place method, is there already an exported API that exposes 
change_page_attr_set without first
calling reserve_memtype? I can't seem to find one.

> Or maybe you could just iounmap the whole thing after firmware is loaded and 
> the framebuffer is found and then
> redo the mapping right. 

I guess this would require a lock so that the ivtv-driver proper wasn't 
accessing the decoder's mapped memory 
during ivtvfb's iounmap-ioremap window. And a way to notify ivtv-driver proper 
if things go wrong? I think this method
would be very awkward because its not even memory owned by ivtvfb itself.

- Nick

[PATCH v2] media: ivtv: add parameter to enable ivtvfb on x86 PAT systems

[Nick French]

ivtvfb was previously disabled for x86 PAT-enabled systems
by commit 1bf1735b4780 ("x86/mm/pat, drivers/media/ivtv:
Use arch_phys_wc_add() and require PAT disabled") as a
workaround to abstract MTRR code away from device drivers.

The driver is not easily upgradable to the PAT-aware
ioremap_wc() API since the firmware hides the address
ranges that should be marked write-combined from the driver.
However, since a write-combined cache on the framebuffer
is only a performance enhancement not a requirement for
the framebuffer to function, completely disabling the driver
in this configuration is not necessary.

Add force_pat module parameter and a corresponding kernel
configuration parameter to optionally force initialization
on PAT-enabled x86 systems with a warning about the lack of
write-combined caching, and document the reasons the driver
cannot be easily updated to support wc caching on all systems.

Signed-off-by: Nick French <n...@ou.edu>
---
Changes in v2:
 - Add wording to Kconfig parameter to memorialize the reasoning
   this driver cannot easily be upgraded to use ioremap_wc()

 drivers/media/pci/ivtv/Kconfig  | 23 ---
 drivers/media/pci/ivtv/ivtvfb.c | 19 +--
 2 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/drivers/media/pci/ivtv/Kconfig b/drivers/media/pci/ivtv/Kconfig
index c72cbbd2d40c..e5ebf7ca145c 100644
--- a/drivers/media/pci/ivtv/Kconfig
+++ b/drivers/media/pci/ivtv/Kconfig
@@ -70,8 +70,25 @@ config VIDEO_FB_IVTV
  This is used in the Hauppauge PVR-350 card. There is a driver
  homepage at <http://www.ivtvdriver.org>.
 
- In order to use this module, you will need to boot with PAT disabled
- on x86 systems, using the nopat kernel parameter.
-
  To compile this driver as a module, choose M here: the
  module will be called ivtvfb.
+
+config VIDEO_FB_IVTV_FORCE_PAT
+   bool "force cx23415 frambuffer init with x86 PAT enabled"
+   depends on VIDEO_FB_IVTV && X86_PAT
+   default n
+   ---help---
+ With PAT enabled, the cx23415 framebuffer driver does not
+ utilize write-combined caching on the framebuffer memory.
+ For this reason, the driver will by default disable itself
+ when initializied on a kernel with PAT enabled (i.e. not
+ using the nopat kernel parameter).
+
+ The driver is not easily upgradable to the PAT-aware
+ ioremap_wc() API since the firmware hides the address
+ ranges that should be marked write-combined from the driver.
+
+ With this setting enabled, the framebuffer will initialize on
+ PAT-enabled systems but the framebuffer memory will be uncached.
+
+ If unsure, say N.
diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
index 621b2f613d81..5df74721aa19 100644
--- a/drivers/media/pci/ivtv/ivtvfb.c
+++ b/drivers/media/pci/ivtv/ivtvfb.c
@@ -55,6 +55,7 @@
 /* card parameters */
 static int ivtvfb_card_id = -1;
 static int ivtvfb_debug = 0;
+static bool ivtvfb_force_pat = IS_ENABLED(CONFIG_VIDEO_FB_IVTV_FORCE_PAT);
 static bool osd_laced;
 static int osd_depth;
 static int osd_upper;
@@ -64,6 +65,7 @@ static int osd_xres;
 
 module_param(ivtvfb_card_id, int, 0444);
 module_param_named(debug,ivtvfb_debug, int, 0644);
+module_param_named(force_pat, ivtvfb_force_pat, bool, 0644);
 module_param(osd_laced, bool, 0444);
 module_param(osd_depth, int, 0444);
 module_param(osd_upper, int, 0444);
@@ -79,6 +81,9 @@ MODULE_PARM_DESC(debug,
 "Debug level (bitmask). Default: errors only\n"
 "\t\t\t(debug = 3 gives full debugging)");
 
+MODULE_PARM_DESC(force_pat,
+"Force initialization on x86 PAT-enabled systems (bool).\n");
+
 /* Why upper, left, xres, yres, depth, laced ? To match terminology used
by fbset.
Why start at 1 for left & upper coordinate ? Because X doesn't allow 0 */
@@ -1169,8 +1174,18 @@ static int ivtvfb_init_card(struct ivtv *itv)
 
 #ifdef CONFIG_X86_64
if (pat_enabled()) {
-   pr_warn("ivtvfb needs PAT disabled, boot with nopat kernel 
parameter\n");
-   return -ENODEV;
+   if (ivtvfb_force_pat) {
+   pr_info("PAT is enabled. Write-combined framebuffer "
+   "caching will be disabled. To enable caching, "
+   "boot with nopat kernel parameter\n");
+   } else {
+   pr_warn("ivtvfb needs PAT disabled for write-combined "
+   "framebuffer caching. Boot with nopat kernel "
+   "parameter to use caching, or use the "
+   "force_pat module parameter to run with "
+   "caching disabled\n");
+   return -ENODEV;
+   }
}
 #endif
 
-- 
2.13.6

Re: ivtv: use arch_phys_wc_add() and require PAT disabled

[Nick French]

On Sun, Mar 11, 2018 at 11:24:38PM +, Ian Armstrong wrote:
> On Sat, 10 Mar 2018 16:57:41 +
> "French, Nicholas A."  wrote:
> 
> > > > No what if the framebuffer driver is just requested as a
> > > > secondary step after firmware loading?  
> > >
> > > Its a possibility. The decoder firmware gets loaded at the
> > > beginning of the decoder memory range and we know its length, so
> > > its possible to ioremap_nocache enough room for the firmware only
> > > on init and then ioremap the remaining non-firmware decoder memory
> > > areas appropriately after the firmware load succeeds...  
> > 
> > I looked in more detail, and this would be "hard" due to the way the
> > rest of the decoder offsets are determined by either making firmware
> > calls or scanning the decoder memory range for magic bytes and other
> > mess.
> 
> The buffers used for yuv output are fixed. They are located both before
> and after the framebuffer. Their offset is fixed at 'base_addr +
> IVTV_DECODER_OFFSET + yuv_offset[]'. The yuv offsets can be found in
> 'ivtv-yuv.c'. The buffers are 622080 bytes in length.
> 
> The range would be from 'base_addr + 0x0100 + 0x00029000' to
> 'base_addr + 0x0100 + 0x00748200 + 0x97dff'. This is larger than
> required, but will catch the framebuffer and should not cause any
> problems. If you wanted to render direct to the yuv buffers, you would
> probably want this region included anyway (not that the current driver
> supports that).

Am I correct that you are talking about the possibility of re-ioremap()-ing
the 'yuv-fb-yuv' area *after* loading the firmware, not of mapping ranges
correctly on the first go-around?

Because unless my math is letting me down, the decoder firmware is already
loaded from 'base_addr + 0x0100 + 0x0' to 'base_addr + 0x0100 + 0x3'
which overlaps the beginning of the yuv range.

- Nick

[PATCH] media: ivtv: add parameter to enable ivtvfb on x86 PAT systems

[Nick French]

ivtvfb was previously disabled for x86 PAT-enabled systems
by commit 1bf1735b4780 ("x86/mm/pat, drivers/media/ivtv:
Use arch_phys_wc_add() and require PAT disabled") as a
workaround to abstract MTRR code away from device drivers.

The driver is not easily upgradable to the PAT-aware
ioremap_wc() API since the firmware hides the address
ranges that should be marked write-combined from the driver.
However, since a write-combined cache on the framebuffer
is only a performance enhancement not a requirement for
the framebuffer to function, completely disabling the driver
in this configuration is not necessary.

Add force_pat module parameter and a corresponding kernel
configuration parameter to optionally force initialization
on PAT-enabled x86 systems with a warning about the lack of
write-combined caching.

Signed-off-by: Nick French <n...@ou.edu>
---
 drivers/media/pci/ivtv/Kconfig  | 19 ---
 drivers/media/pci/ivtv/ivtvfb.c | 19 +--
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/media/pci/ivtv/Kconfig b/drivers/media/pci/ivtv/Kconfig
index c72cbbd2d40c..9c63370ed721 100644
--- a/drivers/media/pci/ivtv/Kconfig
+++ b/drivers/media/pci/ivtv/Kconfig
@@ -70,8 +70,21 @@ config VIDEO_FB_IVTV
  This is used in the Hauppauge PVR-350 card. There is a driver
  homepage at <http://www.ivtvdriver.org>.
 
- In order to use this module, you will need to boot with PAT disabled
- on x86 systems, using the nopat kernel parameter.
-
  To compile this driver as a module, choose M here: the
  module will be called ivtvfb.
+
+config VIDEO_FB_IVTV_FORCE_PAT
+   bool "force cx23415 frambuffer init with x86 PAT enabled"
+   depends on VIDEO_FB_IVTV && X86_PAT
+   default n
+   ---help---
+ With PAT enabled, the cx23415 framebuffer driver is not able to
+ utilize write-combined caching on the framebuffer memory.
+ The default behaviour is to disable the framebuffer completely
+ when it detects PAT is enabled in the kernel (i.e. not using
+ the nopat kernel parameter)
+
+ With this setting enabled, the framebuffer will initialize on
+ PAT-enabled systems but the framebuffer memory will be uncached.
+
+ If unsure, say N.
diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
index 621b2f613d81..5df74721aa19 100644
--- a/drivers/media/pci/ivtv/ivtvfb.c
+++ b/drivers/media/pci/ivtv/ivtvfb.c
@@ -55,6 +55,7 @@
 /* card parameters */
 static int ivtvfb_card_id = -1;
 static int ivtvfb_debug = 0;
+static bool ivtvfb_force_pat = IS_ENABLED(CONFIG_VIDEO_FB_IVTV_FORCE_PAT);
 static bool osd_laced;
 static int osd_depth;
 static int osd_upper;
@@ -64,6 +65,7 @@ static int osd_xres;
 
 module_param(ivtvfb_card_id, int, 0444);
 module_param_named(debug,ivtvfb_debug, int, 0644);
+module_param_named(force_pat, ivtvfb_force_pat, bool, 0644);
 module_param(osd_laced, bool, 0444);
 module_param(osd_depth, int, 0444);
 module_param(osd_upper, int, 0444);
@@ -79,6 +81,9 @@ MODULE_PARM_DESC(debug,
 "Debug level (bitmask). Default: errors only\n"
 "\t\t\t(debug = 3 gives full debugging)");
 
+MODULE_PARM_DESC(force_pat,
+"Force initialization on x86 PAT-enabled systems (bool).\n");
+
 /* Why upper, left, xres, yres, depth, laced ? To match terminology used
by fbset.
Why start at 1 for left & upper coordinate ? Because X doesn't allow 0 */
@@ -1169,8 +1174,18 @@ static int ivtvfb_init_card(struct ivtv *itv)
 
 #ifdef CONFIG_X86_64
if (pat_enabled()) {
-   pr_warn("ivtvfb needs PAT disabled, boot with nopat kernel 
parameter\n");
-   return -ENODEV;
+   if (ivtvfb_force_pat) {
+   pr_info("PAT is enabled. Write-combined framebuffer "
+   "caching will be disabled. To enable caching, "
+   "boot with nopat kernel parameter\n");
+   } else {
+   pr_warn("ivtvfb needs PAT disabled for write-combined "
+   "framebuffer caching. Boot with nopat kernel "
+   "parameter to use caching, or use the "
+   "force_pat module parameter to run with "
+   "caching disabled\n");
+   return -ENODEV;
+   }
}
 #endif
 
-- 
2.13.6