Re: [PATCH 4/8] fbdev: ssd1307fb: Use vmalloc to allocate video memory.
Am Fri, 20 Mar 2015 17:25:29 +0200 schrieb Tomi Valkeinen : > On 20/03/15 16:47, Maxime Ripard wrote: > > On Fri, Mar 20, 2015 at 01:37:50PM +0200, Tomi Valkeinen wrote: > >> On 15/03/15 00:02, Geert Uytterhoeven wrote: > >>> On Fri, Mar 13, 2015 at 10:31 PM, Thomas Niederprüm > >>> wrote: > Am Tue, 10 Mar 2015 13:28:25 +0200 > schrieb Tomi Valkeinen : > > Also, isn't doing __pa() for the memory returned by vmalloc > > plain wrong? > > > What was the crash about when using kmalloc? It would be good > > to fix defio, as I don't see why it should not work with > > kmalloced memory. > > The main challenge here is that the memory handed to userspace > upon mmap call needs to be page aligned. The memory returned by > kmalloc has no such alignment, but the pointer presented to the > userspace program gets aligned to next page boundary. It's not > clear to me whether there is an easy way to obtain page aligned > kmalloc memory. Memory allocated by vmalloc on the other hand is > always aligned to page boundaries. This is why I chose to go for > vmalloc. > >>> > >>> __get_free_pages()? > >> > >> I'm not that experienced with mem management, so I have to ask... > >> __get_free_pages() probably works fine, but isn't vmalloc better > >> here? > >> > >> __get_free_pages() will give you possibly a lot more memory than > >> you need. And the memory is contiguous, so it could be difficult > >> to allocate a larger memory area. The driver doesn't need > >> contiguous memory (except in the virtual sense). > > > > vmalloc also returns pages, so the size will be page-aligned. It > > doesn't make much of a difference here, since we will only use a > > single page in both case (the max resolution of these screens is > > 128x39, with one bit per pixel). > > Ok, that's not much, then =). > > In that case __get_free_pages sounds fine. Even if the resolution > would be slightly higher, we're only talking about a page or two > extra. > > Usually double-underscore in front of a func means "don't call this". > I don't know why this one has the underscores. This irritated me as well. But since it turned out that there is even a section on "get_free_page and Friends" in LDD3 that talks about __get_free_pages() without any word of caution, I assumed it's ok to call this functions. Thomas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] fbdev: ssd1307fb: Use vmalloc to allocate video memory.
Am Fri, 20 Mar 2015 16:24:29 +0100 schrieb Geert Uytterhoeven : > On Fri, Mar 20, 2015 at 3:47 PM, Maxime Ripard > wrote: > > On Fri, Mar 20, 2015 at 01:37:50PM +0200, Tomi Valkeinen wrote: > >> On 15/03/15 00:02, Geert Uytterhoeven wrote: > >> > On Fri, Mar 13, 2015 at 10:31 PM, Thomas Niederprüm > >> > wrote: > >> >> Am Tue, 10 Mar 2015 13:28:25 +0200 > >> >> schrieb Tomi Valkeinen : > >> >>> Also, isn't doing __pa() for the memory returned by vmalloc > >> >>> plain wrong? > >> >> > >> >>> What was the crash about when using kmalloc? It would be good > >> >>> to fix defio, as I don't see why it should not work with > >> >>> kmalloced memory. > >> >> > >> >> The main challenge here is that the memory handed to userspace > >> >> upon mmap call needs to be page aligned. The memory returned by > >> >> kmalloc has no such alignment, but the pointer presented to the > >> >> userspace program gets aligned to next page boundary. It's not > >> >> clear to me whether there is an easy way to obtain page aligned > >> >> kmalloc memory. Memory allocated by vmalloc on the other hand > >> >> is always aligned to page boundaries. This is why I chose to go > >> >> for vmalloc. > >> > > >> > __get_free_pages()? > >> > >> I'm not that experienced with mem management, so I have to ask... > >> __get_free_pages() probably works fine, but isn't vmalloc better > >> here? > >> > >> __get_free_pages() will give you possibly a lot more memory than > >> you need. And the memory is contiguous, so it could be difficult > >> to allocate a larger memory area. The driver doesn't need > >> contiguous memory (except in the virtual sense). > > > > vmalloc also returns pages, so the size will be page-aligned. It > > doesn't make much of a difference here, since we will only use a > > single page in both case (the max resolution of these screens is > > 128x39, with one bit per pixel). > > In that case I recommend get_zeroed_page(), to avoid the vmalloc() > overhead of setting up a mapping. I looked into get_zeroed_page() too but I thought __get_free_pages() might be more future proof since get_zeroed_page() will not reserve enough memory if more than one page is needed. This might occur if a new controller pops up that has more pixels than bits in a page or the driver is used on a system with a small page size. Also get_zeroed_page() is also just calling __get_free_pages() with the order parameter set to 0. Therefore I think a call like __get_free_pages(GFP_KERNEL | __GFP_ZERO, get_order(size)) does the same as get_zeroed_page() if only one page is needed but has the ability to reserve more pages if needed. Thomas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] fbdev: ssd1307fb: Use vmalloc to allocate video memory.
On 20/03/15 16:47, Maxime Ripard wrote: > On Fri, Mar 20, 2015 at 01:37:50PM +0200, Tomi Valkeinen wrote: >> On 15/03/15 00:02, Geert Uytterhoeven wrote: >>> On Fri, Mar 13, 2015 at 10:31 PM, Thomas Niederprüm >>> wrote: Am Tue, 10 Mar 2015 13:28:25 +0200 schrieb Tomi Valkeinen : > Also, isn't doing __pa() for the memory returned by vmalloc plain > wrong? > What was the crash about when using kmalloc? It would be good to fix > defio, as I don't see why it should not work with kmalloced memory. The main challenge here is that the memory handed to userspace upon mmap call needs to be page aligned. The memory returned by kmalloc has no such alignment, but the pointer presented to the userspace program gets aligned to next page boundary. It's not clear to me whether there is an easy way to obtain page aligned kmalloc memory. Memory allocated by vmalloc on the other hand is always aligned to page boundaries. This is why I chose to go for vmalloc. >>> >>> __get_free_pages()? >> >> I'm not that experienced with mem management, so I have to ask... >> __get_free_pages() probably works fine, but isn't vmalloc better here? >> >> __get_free_pages() will give you possibly a lot more memory than you >> need. And the memory is contiguous, so it could be difficult to allocate >> a larger memory area. The driver doesn't need contiguous memory (except >> in the virtual sense). > > vmalloc also returns pages, so the size will be page-aligned. It > doesn't make much of a difference here, since we will only use a > single page in both case (the max resolution of these screens is > 128x39, with one bit per pixel). Ok, that's not much, then =). In that case __get_free_pages sounds fine. Even if the resolution would be slightly higher, we're only talking about a page or two extra. Usually double-underscore in front of a func means "don't call this". I don't know why this one has the underscores. Tomi signature.asc Description: OpenPGP digital signature
Re: [PATCH 4/8] fbdev: ssd1307fb: Use vmalloc to allocate video memory.
On Fri, Mar 20, 2015 at 3:47 PM, Maxime Ripard
wrote:
> On Fri, Mar 20, 2015 at 01:37:50PM +0200, Tomi Valkeinen wrote:
>> On 15/03/15 00:02, Geert Uytterhoeven wrote:
>> > On Fri, Mar 13, 2015 at 10:31 PM, Thomas Niederprüm
>> > wrote:
>> >> Am Tue, 10 Mar 2015 13:28:25 +0200
>> >> schrieb Tomi Valkeinen :
>> >>> Also, isn't doing __pa() for the memory returned by vmalloc plain
>> >>> wrong?
>> >>
>> >>> What was the crash about when using kmalloc? It would be good to fix
>> >>> defio, as I don't see why it should not work with kmalloced memory.
>> >>
>> >> The main challenge here is that the memory handed to userspace upon
>> >> mmap call needs to be page aligned. The memory returned by kmalloc has
>> >> no such alignment, but the pointer presented to the userspace program
>> >> gets aligned to next page boundary. It's not clear to me whether there
>> >> is an easy way to obtain page aligned kmalloc memory. Memory
>> >> allocated by vmalloc on the other hand is always aligned to page
>> >> boundaries. This is why I chose to go for vmalloc.
>> >
>> > __get_free_pages()?
>>
>> I'm not that experienced with mem management, so I have to ask...
>> __get_free_pages() probably works fine, but isn't vmalloc better here?
>>
>> __get_free_pages() will give you possibly a lot more memory than you
>> need. And the memory is contiguous, so it could be difficult to allocate
>> a larger memory area. The driver doesn't need contiguous memory (except
>> in the virtual sense).
>
> vmalloc also returns pages, so the size will be page-aligned. It
> doesn't make much of a difference here, since we will only use a
> single page in both case (the max resolution of these screens is
> 128x39, with one bit per pixel).
In that case I recommend get_zeroed_page(), to avoid the vmalloc()
overhead of setting up a mapping.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] fbdev: ssd1307fb: Use vmalloc to allocate video memory.
On Fri, Mar 20, 2015 at 01:37:50PM +0200, Tomi Valkeinen wrote: > On 15/03/15 00:02, Geert Uytterhoeven wrote: > > On Fri, Mar 13, 2015 at 10:31 PM, Thomas Niederprüm > > wrote: > >> Am Tue, 10 Mar 2015 13:28:25 +0200 > >> schrieb Tomi Valkeinen : > >>> Also, isn't doing __pa() for the memory returned by vmalloc plain > >>> wrong? > >> > >>> What was the crash about when using kmalloc? It would be good to fix > >>> defio, as I don't see why it should not work with kmalloced memory. > >> > >> The main challenge here is that the memory handed to userspace upon > >> mmap call needs to be page aligned. The memory returned by kmalloc has > >> no such alignment, but the pointer presented to the userspace program > >> gets aligned to next page boundary. It's not clear to me whether there > >> is an easy way to obtain page aligned kmalloc memory. Memory > >> allocated by vmalloc on the other hand is always aligned to page > >> boundaries. This is why I chose to go for vmalloc. > > > > __get_free_pages()? > > I'm not that experienced with mem management, so I have to ask... > __get_free_pages() probably works fine, but isn't vmalloc better here? > > __get_free_pages() will give you possibly a lot more memory than you > need. And the memory is contiguous, so it could be difficult to allocate > a larger memory area. The driver doesn't need contiguous memory (except > in the virtual sense). vmalloc also returns pages, so the size will be page-aligned. It doesn't make much of a difference here, since we will only use a single page in both case (the max resolution of these screens is 128x39, with one bit per pixel). Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH 4/8] fbdev: ssd1307fb: Use vmalloc to allocate video memory.
On Fri, Mar 20, 2015 at 12:37 PM, Tomi Valkeinen wrote:
> On 15/03/15 00:02, Geert Uytterhoeven wrote:
>> On Fri, Mar 13, 2015 at 10:31 PM, Thomas Niederprüm
>> wrote:
>>> Am Tue, 10 Mar 2015 13:28:25 +0200
>>> schrieb Tomi Valkeinen :
Also, isn't doing __pa() for the memory returned by vmalloc plain
wrong?
>>>
What was the crash about when using kmalloc? It would be good to fix
defio, as I don't see why it should not work with kmalloced memory.
>>>
>>> The main challenge here is that the memory handed to userspace upon
>>> mmap call needs to be page aligned. The memory returned by kmalloc has
>>> no such alignment, but the pointer presented to the userspace program
>>> gets aligned to next page boundary. It's not clear to me whether there
>>> is an easy way to obtain page aligned kmalloc memory. Memory
>>> allocated by vmalloc on the other hand is always aligned to page
>>> boundaries. This is why I chose to go for vmalloc.
>>
>> __get_free_pages()?
>
> I'm not that experienced with mem management, so I have to ask...
> __get_free_pages() probably works fine, but isn't vmalloc better here?
>
> __get_free_pages() will give you possibly a lot more memory than you
> need. And the memory is contiguous, so it could be difficult to allocate
> a larger memory area. The driver doesn't need contiguous memory (except
> in the virtual sense).
I was responding to the question about obtaining page aligned kmalloc
memory.
If it doesn't need to be physically contiguous, vmalloc() is fine.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] fbdev: ssd1307fb: Use vmalloc to allocate video memory.
On 15/03/15 00:02, Geert Uytterhoeven wrote: > On Fri, Mar 13, 2015 at 10:31 PM, Thomas Niederprüm > wrote: >> Am Tue, 10 Mar 2015 13:28:25 +0200 >> schrieb Tomi Valkeinen : >>> Also, isn't doing __pa() for the memory returned by vmalloc plain >>> wrong? >> >>> What was the crash about when using kmalloc? It would be good to fix >>> defio, as I don't see why it should not work with kmalloced memory. >> >> The main challenge here is that the memory handed to userspace upon >> mmap call needs to be page aligned. The memory returned by kmalloc has >> no such alignment, but the pointer presented to the userspace program >> gets aligned to next page boundary. It's not clear to me whether there >> is an easy way to obtain page aligned kmalloc memory. Memory >> allocated by vmalloc on the other hand is always aligned to page >> boundaries. This is why I chose to go for vmalloc. > > __get_free_pages()? I'm not that experienced with mem management, so I have to ask... __get_free_pages() probably works fine, but isn't vmalloc better here? __get_free_pages() will give you possibly a lot more memory than you need. And the memory is contiguous, so it could be difficult to allocate a larger memory area. The driver doesn't need contiguous memory (except in the virtual sense). Tomi signature.asc Description: OpenPGP digital signature
Re: [PATCH 4/8] fbdev: ssd1307fb: Use vmalloc to allocate video memory.
On Fri, Mar 13, 2015 at 10:31 PM, Thomas Niederprüm
wrote:
> Am Tue, 10 Mar 2015 13:28:25 +0200
> schrieb Tomi Valkeinen :
>> Also, isn't doing __pa() for the memory returned by vmalloc plain
>> wrong?
>
>> What was the crash about when using kmalloc? It would be good to fix
>> defio, as I don't see why it should not work with kmalloced memory.
>
> The main challenge here is that the memory handed to userspace upon
> mmap call needs to be page aligned. The memory returned by kmalloc has
> no such alignment, but the pointer presented to the userspace program
> gets aligned to next page boundary. It's not clear to me whether there
> is an easy way to obtain page aligned kmalloc memory. Memory
> allocated by vmalloc on the other hand is always aligned to page
> boundaries. This is why I chose to go for vmalloc.
__get_free_pages()?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] fbdev: ssd1307fb: Use vmalloc to allocate video memory.
Am Tue, 10 Mar 2015 13:28:25 +0200 schrieb Tomi Valkeinen : > On 14/02/15 16:22, Thomas Niederprüm wrote: > > Am Thu, 12 Feb 2015 16:11:21 +0100 > > schrieb Maxime Ripard : > > > >> On Sat, Feb 07, 2015 at 04:35:41PM +0100, Thomas Niederprüm wrote: > >>> Am Sat, 7 Feb 2015 12:18:21 +0100 > >>> schrieb Maxime Ripard : > >>> > Hi, > > On Fri, Feb 06, 2015 at 11:28:10PM +0100, > nied...@physik.uni-kl.de wrote: > > From: Thomas Niederprüm > > > > It makes sense to use vmalloc to allocate the video buffer > > since it has to be page aligned memory for using it with mmap. > > Please wrap your commit log at 80 chars. > >>> > >>> I'll try to do so in future, sorry for that. > >>> > > It looks like there's numerous fbdev drivers using this > (especially since you copy pasted that code, without mentionning > it). > >>> > >>> Yes, I should have mentioned that in the commit message. As > >>> implicitly indicated in the cover letter the rvmalloc() and > >>> rvfree() are copy pasted from the vfb driver. Honestly, I didn't > >>> give this one too much thought. It seemed a viable solution to the > >>> mmap problem. For a bit more history on that, see my comment > >>> below. > >>> > > That should be turned into an allocator so that drivers all get > this right. > > > Also deffered io seems buggy in combination with kmalloc'ed > > memory (crash on unloading the module). > > And maybe that's the real issue to fix. > >>> > >>> The problem is solved by using vmalloc ;) > >> > >> Yep, but why do you need to mark the reserved pages? > >> > >> ... > > > > As far as I understood mmaped memory is marked as userspace memory > > in the page table and is therefore subject to swapping. The pages > > are marked reserved to make clear that this memory can not be > > swapped and thus lock the pages in memory. See discussions [0,1,2]. > > Why is it a problem if it is swapped? Only CPU uses the memory, as far > as I can see. It seems to be no problem at all. I was copying the allocation code from the vfb driver. The memory is no longer marked as reserved from v2 on. > Also, isn't doing __pa() for the memory returned by vmalloc plain > wrong? > What was the crash about when using kmalloc? It would be good to fix > defio, as I don't see why it should not work with kmalloced memory. The main challenge here is that the memory handed to userspace upon mmap call needs to be page aligned. The memory returned by kmalloc has no such alignment, but the pointer presented to the userspace program gets aligned to next page boundary. It's not clear to me whether there is an easy way to obtain page aligned kmalloc memory. Memory allocated by vmalloc on the other hand is always aligned to page boundaries. This is why I chose to go for vmalloc. Thomas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] fbdev: ssd1307fb: Use vmalloc to allocate video memory.
On 14/02/15 16:22, Thomas Niederprüm wrote: > Am Thu, 12 Feb 2015 16:11:21 +0100 > schrieb Maxime Ripard : > >> On Sat, Feb 07, 2015 at 04:35:41PM +0100, Thomas Niederprüm wrote: >>> Am Sat, 7 Feb 2015 12:18:21 +0100 >>> schrieb Maxime Ripard : >>> Hi, On Fri, Feb 06, 2015 at 11:28:10PM +0100, nied...@physik.uni-kl.de wrote: > From: Thomas Niederprüm > > It makes sense to use vmalloc to allocate the video buffer > since it has to be page aligned memory for using it with mmap. Please wrap your commit log at 80 chars. >>> >>> I'll try to do so in future, sorry for that. >>> It looks like there's numerous fbdev drivers using this (especially since you copy pasted that code, without mentionning it). >>> >>> Yes, I should have mentioned that in the commit message. As >>> implicitly indicated in the cover letter the rvmalloc() and >>> rvfree() are copy pasted from the vfb driver. Honestly, I didn't >>> give this one too much thought. It seemed a viable solution to the >>> mmap problem. For a bit more history on that, see my comment below. >>> That should be turned into an allocator so that drivers all get this right. > Also deffered io seems buggy in combination with kmalloc'ed > memory (crash on unloading the module). And maybe that's the real issue to fix. >>> >>> The problem is solved by using vmalloc ;) >> >> Yep, but why do you need to mark the reserved pages? >> >> ... > > As far as I understood mmaped memory is marked as userspace memory in > the page table and is therefore subject to swapping. The pages are > marked reserved to make clear that this memory can not be swapped and > thus lock the pages in memory. See discussions [0,1,2]. Why is it a problem if it is swapped? Only CPU uses the memory, as far as I can see. Also, isn't doing __pa() for the memory returned by vmalloc plain wrong? What was the crash about when using kmalloc? It would be good to fix defio, as I don't see why it should not work with kmalloced memory. Tomi signature.asc Description: OpenPGP digital signature
Re: [PATCH 4/8] fbdev: ssd1307fb: Use vmalloc to allocate video memory.
On Sat, Feb 14, 2015 at 03:22:12PM +0100, Thomas Niederprüm wrote: > Am Thu, 12 Feb 2015 16:11:21 +0100 > schrieb Maxime Ripard : > > > On Sat, Feb 07, 2015 at 04:35:41PM +0100, Thomas Niederprüm wrote: > > > Am Sat, 7 Feb 2015 12:18:21 +0100 > > > schrieb Maxime Ripard : > > > > > > > Hi, > > > > > > > > On Fri, Feb 06, 2015 at 11:28:10PM +0100, nied...@physik.uni-kl.de > > > > wrote: > > > > > From: Thomas Niederprüm > > > > > > > > > > It makes sense to use vmalloc to allocate the video buffer > > > > > since it has to be page aligned memory for using it with mmap. > > > > > > > > Please wrap your commit log at 80 chars. > > > > > > I'll try to do so in future, sorry for that. > > > > > > > > > > > It looks like there's numerous fbdev drivers using this > > > > (especially since you copy pasted that code, without mentionning > > > > it). > > > > > > Yes, I should have mentioned that in the commit message. As > > > implicitly indicated in the cover letter the rvmalloc() and > > > rvfree() are copy pasted from the vfb driver. Honestly, I didn't > > > give this one too much thought. It seemed a viable solution to the > > > mmap problem. For a bit more history on that, see my comment below. > > > > > > > > > > > That should be turned into an allocator so that drivers all get > > > > this right. > > > > > > > > > Also deffered io seems buggy in combination with kmalloc'ed > > > > > memory (crash on unloading the module). > > > > > > > > And maybe that's the real issue to fix. > > > > > > The problem is solved by using vmalloc ;) > > > > Yep, but why do you need to mark the reserved pages? > > > > ... > > As far as I understood mmaped memory is marked as userspace memory in > the page table and is therefore subject to swapping. The pages are > marked reserved to make clear that this memory can not be swapped and > thus lock the pages in memory. See discussions [0,1,2]. > > [0]http://stackoverflow.com/questions/10760479/mmap-kernel-buffer-to-user-space > [1]http://www.linuxquestions.org/questions/linux-kernel-70/why-why-setpagereserved-is-needed-when-map-a-kernel-space-to-user-space-885176/ > [2]https://sites.google.com/site/skartikeyan/mmap.html Hmmm, both https://lwn.net/Articles/28746/ and http://linux-mm.org/DeviceDriverMmap tell otherwise :) Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH 4/8] fbdev: ssd1307fb: Use vmalloc to allocate video memory.
Am Thu, 12 Feb 2015 16:11:21 +0100 schrieb Maxime Ripard : > On Sat, Feb 07, 2015 at 04:35:41PM +0100, Thomas Niederprüm wrote: > > Am Sat, 7 Feb 2015 12:18:21 +0100 > > schrieb Maxime Ripard : > > > > > Hi, > > > > > > On Fri, Feb 06, 2015 at 11:28:10PM +0100, nied...@physik.uni-kl.de > > > wrote: > > > > From: Thomas Niederprüm > > > > > > > > It makes sense to use vmalloc to allocate the video buffer > > > > since it has to be page aligned memory for using it with mmap. > > > > > > Please wrap your commit log at 80 chars. > > > > I'll try to do so in future, sorry for that. > > > > > > > > It looks like there's numerous fbdev drivers using this > > > (especially since you copy pasted that code, without mentionning > > > it). > > > > Yes, I should have mentioned that in the commit message. As > > implicitly indicated in the cover letter the rvmalloc() and > > rvfree() are copy pasted from the vfb driver. Honestly, I didn't > > give this one too much thought. It seemed a viable solution to the > > mmap problem. For a bit more history on that, see my comment below. > > > > > > > > That should be turned into an allocator so that drivers all get > > > this right. > > > > > > > Also deffered io seems buggy in combination with kmalloc'ed > > > > memory (crash on unloading the module). > > > > > > And maybe that's the real issue to fix. > > > > The problem is solved by using vmalloc ;) > > Yep, but why do you need to mark the reserved pages? > > ... As far as I understood mmaped memory is marked as userspace memory in the page table and is therefore subject to swapping. The pages are marked reserved to make clear that this memory can not be swapped and thus lock the pages in memory. See discussions [0,1,2]. [0]http://stackoverflow.com/questions/10760479/mmap-kernel-buffer-to-user-space [1]http://www.linuxquestions.org/questions/linux-kernel-70/why-why-setpagereserved-is-needed-when-map-a-kernel-space-to-user-space-885176/ [2]https://sites.google.com/site/skartikeyan/mmap.html > > > > > @@ -570,7 +608,7 @@ static int ssd1307fb_probe(struct i2c_client > > > > *client, info->var.blue.offset = 0; > > > > > > > > info->screen_base = (u8 __force __iomem *)vmem; > > > > - info->fix.smem_start = (unsigned long)vmem; > > > > + info->fix.smem_start = __pa(vmem); > > > > > > Why are you changing from virtual to physical address here? > > > > Oh, good catch! This is still a residual of my attempts to get this > > working with kmalloc'ed memory. In the current state the driver is > > presenting a completely wrong memory address upon mmap. As reported > > in [0] info->fix.smem_start has to hold the physical address of the > > video memory if it was allocated using kmalloc. Correcting this let > > me run into the problem that the kmalloc'ed memory was not page > > aligned but, the memory address handed to userspace mmap was > > aligned to the next full page, resulting in an inaccessable display > > region. At that point I just copied the vmalloc approach from the > > vfb driver. > > > > [0] > > http://stackoverflow.com/questions/22285151/kernel-panic-using-deferred-io-on-kmalloced-buffer > > And the documentation of fb_fix_screeninfo indeed state that this > should be the physical address: > http://lxr.free-electrons.com/source/include/uapi/linux/fb.h#L158 > > Could you make that a separate patch? Will be a separate patch in v2. > > Thanks, > Maxime > Thomas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] fbdev: ssd1307fb: Use vmalloc to allocate video memory.
On Sat, Feb 07, 2015 at 04:35:41PM +0100, Thomas Niederprüm wrote: > Am Sat, 7 Feb 2015 12:18:21 +0100 > schrieb Maxime Ripard : > > > Hi, > > > > On Fri, Feb 06, 2015 at 11:28:10PM +0100, nied...@physik.uni-kl.de > > wrote: > > > From: Thomas Niederprüm > > > > > > It makes sense to use vmalloc to allocate the video buffer since it > > > has to be page aligned memory for using it with mmap. > > > > Please wrap your commit log at 80 chars. > > I'll try to do so in future, sorry for that. > > > > > It looks like there's numerous fbdev drivers using this (especially > > since you copy pasted that code, without mentionning it). > > Yes, I should have mentioned that in the commit message. As > implicitly indicated in the cover letter the rvmalloc() and rvfree() are > copy pasted from the vfb driver. Honestly, I didn't give this one too > much thought. It seemed a viable solution to the mmap problem. For a > bit more history on that, see my comment below. > > > > > That should be turned into an allocator so that drivers all get this > > right. > > > > > Also deffered io seems buggy in combination with kmalloc'ed memory > > > (crash on unloading the module). > > > > And maybe that's the real issue to fix. > > The problem is solved by using vmalloc ;) Yep, but why do you need to mark the reserved pages? ... > > > @@ -570,7 +608,7 @@ static int ssd1307fb_probe(struct i2c_client > > > *client, info->var.blue.offset = 0; > > > > > > info->screen_base = (u8 __force __iomem *)vmem; > > > - info->fix.smem_start = (unsigned long)vmem; > > > + info->fix.smem_start = __pa(vmem); > > > > Why are you changing from virtual to physical address here? > > Oh, good catch! This is still a residual of my attempts to get this > working with kmalloc'ed memory. In the current state the driver is > presenting a completely wrong memory address upon mmap. As reported in > [0] info->fix.smem_start has to hold the physical address of the video > memory if it was allocated using kmalloc. Correcting this let me run > into the problem that the kmalloc'ed memory was not page aligned but, > the memory address handed to userspace mmap was aligned to the next > full page, resulting in an inaccessable display region. At that point I > just copied the vmalloc approach from the vfb driver. > > [0] > http://stackoverflow.com/questions/22285151/kernel-panic-using-deferred-io-on-kmalloced-buffer And the documentation of fb_fix_screeninfo indeed state that this should be the physical address: http://lxr.free-electrons.com/source/include/uapi/linux/fb.h#L158 Could you make that a separate patch? Thanks, Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH 4/8] fbdev: ssd1307fb: Use vmalloc to allocate video memory.
Am Sat, 7 Feb 2015 12:18:21 +0100
schrieb Maxime Ripard :
> Hi,
>
> On Fri, Feb 06, 2015 at 11:28:10PM +0100, nied...@physik.uni-kl.de
> wrote:
> > From: Thomas Niederprüm
> >
> > It makes sense to use vmalloc to allocate the video buffer since it
> > has to be page aligned memory for using it with mmap.
>
> Please wrap your commit log at 80 chars.
I'll try to do so in future, sorry for that.
>
> It looks like there's numerous fbdev drivers using this (especially
> since you copy pasted that code, without mentionning it).
Yes, I should have mentioned that in the commit message. As
implicitly indicated in the cover letter the rvmalloc() and rvfree() are
copy pasted from the vfb driver. Honestly, I didn't give this one too
much thought. It seemed a viable solution to the mmap problem. For a
bit more history on that, see my comment below.
>
> That should be turned into an allocator so that drivers all get this
> right.
>
> > Also deffered io seems buggy in combination with kmalloc'ed memory
> > (crash on unloading the module).
>
> And maybe that's the real issue to fix.
The problem is solved by using vmalloc ;)
>
> > Signed-off-by: Thomas Niederprüm
> > ---
> > drivers/video/fbdev/ssd1307fb.c | 43
> > +++-- 1 file changed, 41
> > insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/ssd1307fb.c
> > b/drivers/video/fbdev/ssd1307fb.c index 01cfb46..945ded9 100644
> > --- a/drivers/video/fbdev/ssd1307fb.c
> > +++ b/drivers/video/fbdev/ssd1307fb.c
> > @@ -11,6 +11,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -93,6 +94,43 @@ static struct fb_var_screeninfo ssd1307fb_var = {
> > .bits_per_pixel = 1,
> > };
> >
> > +static void *rvmalloc(unsigned long size)
> > +{
> > + void *mem;
> > + unsigned long adr;
> > +
> > + size = PAGE_ALIGN(size);
>
> Isn't vmalloc already taking care of that?
>
> > + mem = vmalloc_32(size);
>
> Why the 32 bits variant?
>
> > + if (!mem)
> > + return NULL;
> > +
> > + memset(mem, 0, size); /* Clear the ram out, no junk to the
> > user */
> > + adr = (unsigned long) mem;
> > + while (size > 0) {
> > + SetPageReserved(vmalloc_to_page((void *)adr));
>
> I'm not really sure it makes sense to mark all pages reserved if we're
> not even sure we're going to use mmap.
>
> And why do you need to mark these pages reserved in the first place?
>
> > + adr += PAGE_SIZE;
> > + size -= PAGE_SIZE;
> > + }
> > +
> > + return mem;
> > +}
> > +
> > +static void rvfree(void *mem, unsigned long size)
> > +{
> > + unsigned long adr;
> > +
> > + if (!mem)
> > + return;
> > +
> > + adr = (unsigned long) mem;
> > + while ((long) size > 0) {
> > + ClearPageReserved(vmalloc_to_page((void *)adr));
> > + adr += PAGE_SIZE;
> > + size -= PAGE_SIZE;
> > + }
> > + vfree(mem);
> > +}
> > +
> > static struct ssd1307fb_array *ssd1307fb_alloc_array(u32 len, u8
> > type) {
> > struct ssd1307fb_array *array;
> > @@ -538,13 +576,13 @@ static int ssd1307fb_probe(struct i2c_client
> > *client, if (of_property_read_u32(node, "solomon,vcomh",
> > &par->vcomh)) par->vcomh = par->device_info->default_vcomh;
> >
> > - vmem = devm_kzalloc(&client->dev, vmem_size, GFP_KERNEL);
> > if (of_property_read_u32(node, "solomon,dclk-div",
> > &par->dclk_div)) par->dclk_div = par->device_info->default_dclk_div;
> >
> > if (of_property_read_u32(node, "solomon,dclk-frq",
> > &par->dclk_frq)) par->dclk_frq =
> > par->device_info->default_dclk_frq;
> > + vmem = rvmalloc(vmem_size);
> > if (!vmem) {
> > dev_err(&client->dev, "Couldn't allocate graphical
> > memory.\n"); ret = -ENOMEM;
> > @@ -570,7 +608,7 @@ static int ssd1307fb_probe(struct i2c_client
> > *client, info->var.blue.offset = 0;
> >
> > info->screen_base = (u8 __force __iomem *)vmem;
> > - info->fix.smem_start = (unsigned long)vmem;
> > + info->fix.smem_start = __pa(vmem);
>
> Why are you changing from virtual to physical address here?
Oh, good catch! This is still a residual of my attempts to get this
working with kmalloc'ed memory. In the current state the driver is
presenting a completely wrong memory address upon mmap. As reported in
[0] info->fix.smem_start has to hold the physical address of the video
memory if it was allocated using kmalloc. Correcting this let me run
into the problem that the kmalloc'ed memory was not page aligned but,
the memory address handed to userspace mmap was aligned to the next
full page, resulting in an inaccessable display region. At that point I
just copied the vmalloc approach from the vfb driver.
[0]
http://stackoverflow.com/questions/22285151/kernel-panic-using-deferred-io-on-kmalloced-buffer
>
> Maxime
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.
Re: [PATCH 4/8] fbdev: ssd1307fb: Use vmalloc to allocate video memory.
Hi,
On Fri, Feb 06, 2015 at 11:28:10PM +0100, nied...@physik.uni-kl.de wrote:
> From: Thomas Niederprüm
>
> It makes sense to use vmalloc to allocate the video buffer since it
> has to be page aligned memory for using it with mmap.
Please wrap your commit log at 80 chars.
It looks like there's numerous fbdev drivers using this (especially
since you copy pasted that code, without mentionning it).
That should be turned into an allocator so that drivers all get this
right.
> Also deffered io seems buggy in combination with kmalloc'ed memory
> (crash on unloading the module).
And maybe that's the real issue to fix.
> Signed-off-by: Thomas Niederprüm
> ---
> drivers/video/fbdev/ssd1307fb.c | 43
> +++--
> 1 file changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
> index 01cfb46..945ded9 100644
> --- a/drivers/video/fbdev/ssd1307fb.c
> +++ b/drivers/video/fbdev/ssd1307fb.c
> @@ -11,6 +11,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -93,6 +94,43 @@ static struct fb_var_screeninfo ssd1307fb_var = {
> .bits_per_pixel = 1,
> };
>
> +static void *rvmalloc(unsigned long size)
> +{
> + void *mem;
> + unsigned long adr;
> +
> + size = PAGE_ALIGN(size);
Isn't vmalloc already taking care of that?
> + mem = vmalloc_32(size);
Why the 32 bits variant?
> + if (!mem)
> + return NULL;
> +
> + memset(mem, 0, size); /* Clear the ram out, no junk to the user */
> + adr = (unsigned long) mem;
> + while (size > 0) {
> + SetPageReserved(vmalloc_to_page((void *)adr));
I'm not really sure it makes sense to mark all pages reserved if we're
not even sure we're going to use mmap.
And why do you need to mark these pages reserved in the first place?
> + adr += PAGE_SIZE;
> + size -= PAGE_SIZE;
> + }
> +
> + return mem;
> +}
> +
> +static void rvfree(void *mem, unsigned long size)
> +{
> + unsigned long adr;
> +
> + if (!mem)
> + return;
> +
> + adr = (unsigned long) mem;
> + while ((long) size > 0) {
> + ClearPageReserved(vmalloc_to_page((void *)adr));
> + adr += PAGE_SIZE;
> + size -= PAGE_SIZE;
> + }
> + vfree(mem);
> +}
> +
> static struct ssd1307fb_array *ssd1307fb_alloc_array(u32 len, u8 type)
> {
> struct ssd1307fb_array *array;
> @@ -538,13 +576,13 @@ static int ssd1307fb_probe(struct i2c_client *client,
> if (of_property_read_u32(node, "solomon,vcomh", &par->vcomh))
> par->vcomh = par->device_info->default_vcomh;
>
> - vmem = devm_kzalloc(&client->dev, vmem_size, GFP_KERNEL);
> if (of_property_read_u32(node, "solomon,dclk-div", &par->dclk_div))
> par->dclk_div = par->device_info->default_dclk_div;
>
> if (of_property_read_u32(node, "solomon,dclk-frq", &par->dclk_frq))
> par->dclk_frq = par->device_info->default_dclk_frq;
>
> + vmem = rvmalloc(vmem_size);
> if (!vmem) {
> dev_err(&client->dev, "Couldn't allocate graphical memory.\n");
> ret = -ENOMEM;
> @@ -570,7 +608,7 @@ static int ssd1307fb_probe(struct i2c_client *client,
> info->var.blue.offset = 0;
>
> info->screen_base = (u8 __force __iomem *)vmem;
> - info->fix.smem_start = (unsigned long)vmem;
> + info->fix.smem_start = __pa(vmem);
Why are you changing from virtual to physical address here?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
signature.asc
Description: Digital signature
[PATCH 4/8] fbdev: ssd1307fb: Use vmalloc to allocate video memory.
From: Thomas Niederprüm
It makes sense to use vmalloc to allocate the video buffer since it has to be
page aligned memory for using it with
mmap. Also deffered io seems buggy in combination with kmalloc'ed memory (crash
on unloading the module).
Signed-off-by: Thomas Niederprüm
---
drivers/video/fbdev/ssd1307fb.c | 43 +++--
1 file changed, 41 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 01cfb46..945ded9 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -93,6 +94,43 @@ static struct fb_var_screeninfo ssd1307fb_var = {
.bits_per_pixel = 1,
};
+static void *rvmalloc(unsigned long size)
+{
+ void *mem;
+ unsigned long adr;
+
+ size = PAGE_ALIGN(size);
+ mem = vmalloc_32(size);
+ if (!mem)
+ return NULL;
+
+ memset(mem, 0, size); /* Clear the ram out, no junk to the user */
+ adr = (unsigned long) mem;
+ while (size > 0) {
+ SetPageReserved(vmalloc_to_page((void *)adr));
+ adr += PAGE_SIZE;
+ size -= PAGE_SIZE;
+ }
+
+ return mem;
+}
+
+static void rvfree(void *mem, unsigned long size)
+{
+ unsigned long adr;
+
+ if (!mem)
+ return;
+
+ adr = (unsigned long) mem;
+ while ((long) size > 0) {
+ ClearPageReserved(vmalloc_to_page((void *)adr));
+ adr += PAGE_SIZE;
+ size -= PAGE_SIZE;
+ }
+ vfree(mem);
+}
+
static struct ssd1307fb_array *ssd1307fb_alloc_array(u32 len, u8 type)
{
struct ssd1307fb_array *array;
@@ -538,13 +576,13 @@ static int ssd1307fb_probe(struct i2c_client *client,
if (of_property_read_u32(node, "solomon,vcomh", &par->vcomh))
par->vcomh = par->device_info->default_vcomh;
- vmem = devm_kzalloc(&client->dev, vmem_size, GFP_KERNEL);
if (of_property_read_u32(node, "solomon,dclk-div", &par->dclk_div))
par->dclk_div = par->device_info->default_dclk_div;
if (of_property_read_u32(node, "solomon,dclk-frq", &par->dclk_frq))
par->dclk_frq = par->device_info->default_dclk_frq;
+ vmem = rvmalloc(vmem_size);
if (!vmem) {
dev_err(&client->dev, "Couldn't allocate graphical memory.\n");
ret = -ENOMEM;
@@ -570,7 +608,7 @@ static int ssd1307fb_probe(struct i2c_client *client,
info->var.blue.offset = 0;
info->screen_base = (u8 __force __iomem *)vmem;
- info->fix.smem_start = (unsigned long)vmem;
+ info->fix.smem_start = __pa(vmem);
info->fix.smem_len = vmem_size;
fb_deferred_io_init(info);
@@ -614,6 +652,7 @@ panel_init_error:
};
reset_oled_error:
fb_deferred_io_cleanup(info);
+ rvfree(vmem, vmem_size);
fb_alloc_error:
framebuffer_release(info);
return ret;
--
2.1.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
