On Sat, Mar 10, 2018 at 11:03 AM, Luis R. Rodriguez <mcgrof@xxxxxxxxxx> wrote: > On Sat, Mar 10, 2018 at 8:57 AM, French, Nicholas A. <naf@xxxxxx> wrote: >> On Wed, Mar 07, 2018 at 11:23:09PM -0600, French, Nicholas A. wrote: >>> On Thu, Mar 08, 2018 at 04:14:11AM +0000, Luis R. Rodriguez wrote: >>> > On Thu, Mar 08, 2018 at 04:06:01AM +0000, Luis R. Rodriguez wrote: >>> > > On Thu, Mar 08, 2018 at 03:16:29AM +0000, French, Nicholas A. wrote: >>> > > > >>> > > > Ah, I see. So my proposed ioremap_wc call was only "working" by aliasing the >>> > > > ioremap_nocache()'d mem area and not actually using write combining at all. >>> > > >>> > > There are some debugging PAT toys out there I think but I haven't played with >>> > > them yet or I forgot how to to confirm or deny this sort of effort, but >>> > > likeley. >>> > >>> > In fact come to think of it I believe some neurons are telling me that if >>> > two type does not match we'd get an error? >> >> I can confirm that my original suggested patch just aliases to ivtv-driver's nocache mapping: >> $ sudo modprobe ivtvfb >> $ sudo dmesg >> ... >> x86/PAT: Overlap at 0xd5000000-0xd5800000 >> x86/PAT: reserve_memtype added [mem 0xd5510000-0xd56b0fff], track uncached-minus, req write-combining, ret uncached-minus >> ivtvfb0: Framebuffer at 0xd5510000, mapped to 0x00000000c6a7ed52, size 1665k >> ... >> $ sudo cat /sys/kernel/debug/x86/pat_memtype_list | grep 0xd5 >> uncached-minus @ 0xd5000000-0xd5800000 >> uncached-minus @ 0xd5510000-0xd56b1000 >> >> So nix that. >> >>> > 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. >> >> I think some smart guy named mcgrof apparently came to the same conclusion >> in a really old email chain I found [https://lists.gt.net/linux/kernel/2387536]: >> "The ivtv case is the *worst* example we can expect where the firmware >> hides from us the exact ranges for write-combining, that we should somehow >> just hope no one will ever do again." >> :-) > > This is tribal knowledge worth formalizing a bit more for the long run > for this ivtv driver. > >>> 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? > > I think its worth it, and perhaps best folded under a new kernel > parameter option which also documents the limitation noted above, > thereby knocking two birds with one stone. This way also users who > *want* to opt-in to PAT do so willing-fully and knowing of the > limitation. The kconfig option can just enable a module parameter to a > default value, which if the kconfig is disabled would otherwise be > unset. > > static bool ivtv_force_pat = IS_ENABLED(CONFIG_IVTV_WHATEVER); > module_param_named(force_pat, ivtv_force_pat, bool, S_IRUGO | S_IWUSR); And I wonder if its better to have a generic kconfig option so that in case other drivers have similar issue they can make use of it as well. For now that's a non-issue, but worth pointing out if we're going to do this for more than one driver later. Luis
