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

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

[Luis R. Rodriguez]

On Tue, Mar 06, 2018 at 09:01:10PM +0000, French, Nicholas A. wrote:
> any reason why PAT can't be enabled for ivtvfb as simply as in the attached
> patch?

diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
index 621b2f613d81..69de110726e8 100644
--- a/drivers/media/pci/ivtv/ivtvfb.c
+++ b/drivers/media/pci/ivtv/ivtvfb.c
@@ -1117,7 +1117,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;
-	oi->video_vbase = itv->dec_mem + oi->video_rbase;
+	oi->video_vbase = ioremap_wc(oi->video_pbase, oi->video_buffer_size);

Note that this is the OSD buffer setup. The OSD buffer info is setup at the
start of the routine:

struct osd_info *oi = itv->osd_info;

And note that itv->osd_info is kzalloc()'d via ivtvfb_init_card() right before
ivtvfb_init_io(), which is the routine you are modifying.

Prior to your change the OSD buffer was obtained using the itv->dec_mem + oi->video_rbase
given itv->dec_mem was initialized via the ivtv driver module, one of which's C files
is:

drivers/media/pci/ivtv/ivtv-driver.c

and has:

   if (itv->has_cx23415) {
	...
	itv->dec_mem = ioremap_nocache(itv->base_addr + IVTV_DECODER_OFFSET - oi->video_buffer_size,
                                IVTV_DECODER_SIZE);
	...
   else {
	itv->dec_mem = itv->enc_mem;
   }


The way it used to work then it seems to be that we have a main ivtv driver which
does the ioremap off of the decoder and uses that as offset. If its not
the special cx23415 it still sets the decoder mapped offset to the encoder
offset.

So if you wanted to do what you mention in the above hunk I think you'd then
have to also proactively reduce the size of the ioremap_nocache()'d size on
the ivtv driver first. It would probably make your programming easier if
you know if the cx23415 had no frame buffer too, as then the ivtvfb driver
would not have to be concerned for variants, or the ivtv change would only
be relevant for cx23415 varaint users.

So what I'd do is change the ioremap_nocache()'d size by substracting
oi->video_buffer_size -- but then you have to ask yourself how you'd get
that size. If its something you can figure out then great.

The ivtv driver is a bit odd in that ivtvfb_init() will issue
ivtvfb_callback_init() on each registered device the ivtv driver registered, so
care must be taken with order as well on tear down.

Good luck!

  Luis

@@ -1157,6 +1157,8 @@ static void ivtvfb_release_buffers (struct ivtv *itv)
 	/* Release pseudo palette */
 	kfree(oi->ivtvfb_info.pseudo_palette);
 	arch_phys_wc_del(oi->wc_cookie);
+	if (oi->video_vbase)
+		iounmap(oi->video_vbase);
 	kfree(oi);
 	itv->osd_info = NULL;
 }
@@ -1167,13 +1169,6 @@ static int ivtvfb_init_card(struct ivtv *itv)
 {
 	int rc;
 
-#ifdef CONFIG_X86_64
-	if (pat_enabled()) {
-		pr_warn("ivtvfb needs PAT disabled, boot with nopat kernel parameter\n");
-		return -ENODEV;
-	}
-#endif
-
 	if (itv->osd_info) {
 		IVTVFB_ERR("Card %d already initialised\n", ivtvfb_card_id);
 		return -EBUSY;

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

[French, Nicholas A.]

On Wed, Mar 07, 2018 at 07:02:05PM +0000, Luis R. Rodriguez wrote:
> On Tue, Mar 06, 2018 at 09:01:10PM +0000, French, Nicholas A. wrote:
> > any reason why PAT can't be enabled for ivtvfb as simply as in the attached
> > patch?
>
> Prior to your change the OSD buffer was obtained using the itv->dec_mem + oi->video_rbase
> given itv->dec_mem was initialized via [...]
>         itv->dec_mem = ioremap_nocache(itv->base_addr + IVTV_DECODER_OFFSET - oi->video_buffer_size,
>                                 IVTV_DECODER_SIZE);

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.

> So what I'd do is change the ioremap_nocache()'d size by substracting
> oi->video_buffer_size -- but then you have to ask yourself how you'd get
> that size. If its something you can figure out then great.

Size is easy since its hardcoded, but unfortunately getting the offset of the framebuffer inside the decoders memory to remove from the ioremap_nocache call is a chicken and egg problem: the offset is determined by querying the firmware that has been loaded to the decoder. the firmware itself will be loaded after the ioremap_nocache call at an offset from the address it returns.

So unless there is a io-re-remap to change the caching status of a subset of the decoder's memory once we find out what the framebuffer offset is inside the original iremap_nocache'd area, then its a no go for write combining to the framebuffer with PAT.

On the other hand, it works fine for me with a nocache'd framebuffer. It's certainly better for me personally to have a nocache framebuffer with PAT-enabled than the framebuffer completely disabled with PAT-enabled, but I don't think I would even propose to rollback the x86 nopat requirement in general. Apparently the throngs of people using this super-popular driver feature haven't complained in the last couple years, so maybe its OK for me to just patch the pat-enabled guard out and deal with a nocache'd framebuffer.

- Nick    

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

[Luis R. Rodriguez]

On Thu, Mar 08, 2018 at 03:16:29AM +0000, French, Nicholas A. wrote:
> On Wed, Mar 07, 2018 at 07:02:05PM +0000, Luis R. Rodriguez wrote:
> > On Tue, Mar 06, 2018 at 09:01:10PM +0000, French, Nicholas A. wrote:
> > > any reason why PAT can't be enabled for ivtvfb as simply as in the attached
> > > patch?
> >
> > Prior to your change the OSD buffer was obtained using the itv->dec_mem + oi->video_rbase
> > given itv->dec_mem was initialized via [...]
> >         itv->dec_mem = ioremap_nocache(itv->base_addr + IVTV_DECODER_OFFSET - oi->video_buffer_size,
> >                                 IVTV_DECODER_SIZE);
> 
> 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.

> > So what I'd do is change the ioremap_nocache()'d size by substracting
> > oi->video_buffer_size -- but then you have to ask yourself how you'd get
> > that size. If its something you can figure out then great.
> 
> Size is easy since its hardcoded, but unfortunately getting the offset of the
> framebuffer inside the decoders memory to remove from the ioremap_nocache
> call is a chicken and egg problem: the offset is determined by querying the
> firmware that has been loaded to the decoder. the firmware itself will be
> loaded after the ioremap_nocache call at an offset from the address it
> returns.

What I expected. Probably why no one had tried before to clean it up.

> So unless there is a io-re-remap to change the caching status of a subset of
> the decoder's memory once we find out what the framebuffer offset is inside
> the original iremap_nocache'd area, then its a no go for write combining to
> the framebuffer with PAT.

No what if the framebuffer driver is just requested as a secondary step
after firmware loading?

> On the other hand, it works fine for me with a nocache'd framebuffer. It's
> certainly better for me personally to have a nocache framebuffer with
> PAT-enabled than the framebuffer completely disabled with PAT-enabled, but I
> don't think I would even propose to rollback the x86 nopat requirement in
> general. Apparently the throngs of people using this super-popular driver
> feature haven't complained in the last couple years, so maybe its OK for me
> to just patch the pat-enabled guard out and deal with a nocache'd
> framebuffer.

Nope, best you add a feature to just let you disable wc stuff, to enable
life with PAT.

  Luis

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

[Luis R. Rodriguez]

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:
> > On Wed, Mar 07, 2018 at 07:02:05PM +0000, Luis R. Rodriguez wrote:
> > > On Tue, Mar 06, 2018 at 09:01:10PM +0000, French, Nicholas A. wrote:
> > > > any reason why PAT can't be enabled for ivtvfb as simply as in the attached
> > > > patch?
> > >
> > > Prior to your change the OSD buffer was obtained using the itv->dec_mem + oi->video_rbase
> > > given itv->dec_mem was initialized via [...]
> > >         itv->dec_mem = ioremap_nocache(itv->base_addr + IVTV_DECODER_OFFSET - oi->video_buffer_size,
> > >                                 IVTV_DECODER_SIZE);
> > 
> > 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?

  Luis

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

[French, Nicholas A.]

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 based my guess on some text i read in "PATting Linux" [1]:
"ioremap interfaces will succeed if there is an existing,
more lenient mapping. Example: If there is an existing
uncached mapping to a physical range, any request for
write-back or write-combine mapping will succeed, but
will eventually map the memory as uncached"

But I will try to get some debugpat going to confirm.

[1] = https://www.kernel.org/doc/ols/2008/ols2008v2-pages-135-144.pdf

> > So unless there is a io-re-remap to change the caching status of a subset of
> > the decoder's memory once we find out what the framebuffer offset is inside
> > the original iremap_nocache'd area, then its a no go for write combining to
> > the framebuffer with PAT.
> 
> 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...
 
> > > On the other hand, it works fine for me with a nocache'd framebuffer. It's
> > > certainly better for me personally to have a nocache framebuffer with
> > > PAT-enabled than the framebuffer completely disabled with PAT-enabled, but I
> > > don't think I would even propose to rollback the x86 nopat requirement in
> > > general. Apparently the throngs of people using this super-popular driver
> > > feature haven't complained in the last couple years, so maybe its OK for me
> > > to just patch the pat-enabled guard out and deal with a nocache'd
> > > framebuffer.
> > 
> > Nope, best you add a feature to just let you disable wc stuff, to enable
> > life with PAT.

I'm not sure I understand what you mean.

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"?

- Nick

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

[French, Nicholas A.]

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."
:-)

> 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?

- Nick

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

[Andy Lutomirski]

> On Mar 10, 2018, at 8:57 AM, French, Nicholas A. <naf@ou.edu> 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."
> :-)
> 
>> 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.

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

[Luis R. Rodriguez]

On Sat, Mar 10, 2018 at 8:57 AM, French, Nicholas A. <naf@ou.edu> 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);

  Luis

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

[Luis R. Rodriguez]

On Sat, Mar 10, 2018 at 11:03 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Sat, Mar 10, 2018 at 8:57 AM, French, Nicholas A. <naf@ou.edu> 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

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

[Andy Lutomirski]

> On Mar 11, 2018, at 12:51 PM, Nick French <naf@ou.edu> wrote:
> 
> 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.

Not really. The goal was to eliminate all code that touches MTRRs on PAT systems. So mtrr_add got unexported and the arch_phys are legacy hints that do nothing on modern machines. 

> 
> 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)

It doesn’t conflict at all.  Obviously the code should be tidy. 

>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. 

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

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

[Andy Lutomirski]

> On Mar 11, 2018, at 12:51 PM, Nick French <naf@ou.edu> wrote:
> 
> 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.

Not really. The goal was to eliminate all code that touches MTRRs on PAT systems. So mtrr_add got unexported and the arch_phys are legacy hints that do nothing on modern machines. 

> 
> 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)

It doesn’t conflict at all.  Obviously the code should be tidy. 

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. 

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

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

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

[Ian Armstrong]

On Sat, 10 Mar 2018 16:57:41 +0000
"French, Nicholas A." <naf@ou.edu> 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 + 0x01000000 + 0x00029000' to
'base_addr + 0x01000000 + 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).

-- 
Ian

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

[Nick French]

On Sun, Mar 11, 2018 at 11:24:38PM +0000, Ian Armstrong wrote:
> On Sat, 10 Mar 2018 16:57:41 +0000
> "French, Nicholas A." <naf@ou.edu> 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 + 0x01000000 + 0x00029000' to
> 'base_addr + 0x01000000 + 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 + 0x01000000 + 0x0' to 'base_addr + 0x01000000 + 0x3ffff'
which overlaps the beginning of the yuv range.

- Nick

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

[Ian Armstrong]

On Sun, 11 Mar 2018 23:04:02 -0500
Nick French <naf@ou.edu> wrote:

> On Sun, Mar 11, 2018 at 11:24:38PM +0000, Ian Armstrong wrote:
> > On Sat, 10 Mar 2018 16:57:41 +0000
> > "French, Nicholas A." <naf@ou.edu> 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 + 0x01000000 + 0x00029000' to
> > 'base_addr + 0x01000000 + 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 + 0x01000000 + 0x0' to 'base_addr +
> 0x01000000 + 0x3ffff' which overlaps the beginning of the yuv range.

Good catch. I'd forgotten that the firmware moves after being loaded.

Although ivtvfb.c requests the offset to the framebuffer via the
firmware, I don't believe it ever actually moves. The firmware allows
more memory to be allocated for video buffers, but will then reduce the
length of the framebuffer. At present the ivtv driver requests two more
video buffers, so the framebuffer is shortened (the start address
doesn't move). Those two buffers are located at base_addr + 0x16B0400
and 0x1748200. The now shortened framebuffer is of size 1704960 bytes
(0x1A0400).

These two offsets, plus those of the other video buffers are hardcoded
in ivtv-yuv.c. It was written on the assumption that the location of the
video buffers on the card are fixed. To the best of my knowledge, this
has never caused a problem.

>From ivtvfb.c :

/* The osd buffer size depends on the number of video buffers allocated
   on the PVR350 itself. For now we'll hardcode the smallest osd buffer
   size to prevent any overlap. */
	oi->video_buffer_size = 1704960;

We know that one of the additional video buffers is at 0x16B0400, and
that this is located at the end of the now shortened 1704960 byte
framebuffer.

0x16B0400 - 0x1A0400 = 0x1510000.

This gives us a 0x1A0400 byte framebuffer at base_addr + 0x1510000.

Assuming the hardware is a PVR350 and is running stock firmware, I
thinks its safe to say this won't change.

-- 
Ian