| Message ID | 20180307190205.GA14069@wotan.suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
> 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.
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
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
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
> 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.
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
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).
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
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.
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: