Re: [PATCH 4/8] fbdev: ssd1307fb: Use vmalloc to allocate video memory.

[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 <maxime.ripard@xxxxxxxxxxxxxxxxxx>:
> 
> > Hi,
> > 
> > On Fri, Feb 06, 2015 at 11:28:10PM +0100, niederp@xxxxxxxxxxxxxxxx
> > wrote:
> > > From: Thomas Niederprüm <niederp@xxxxxxxxxxxxxxxx>
> > > 
> > > 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

Attachment: signature.asc
Description: Digital signature