deprecating fix->mmio_start and smem_start

deprecating fix->mmio_start and smem_start

[Benjamin Herrenschmidt]

Hi !

We currently have a problem with those two members of struct
fb_fix_screeninfo. The struct contains an "unsigned long" which means
that:

- 64 bits kernels with 32 bits userspace can't pass a complete address
- 32 bits machines with 64 bits resource_size_t can't pass a complete
address
- The structure isn't even properly padded to be 32/64 bits neutral in
the first place.

We could define new versions of the struct with new get/set ioctls,
or we could try to just deprecate those fields. What do you guys think ?

If we do the later, we need another way to convey the informations.

For smem, I'm not sure it's very useful, we should just be able to mmap
the fbdev. The problem is more with mmio_start.

Thus the idea that we could do something to allow mmap'ing mmio via
mmap of /dev/fb via some specific offset... what do you think ?

Or we can just do a fb_fix_screeninfo2, with proper padding and u64
addresses, replace the kernel one with that, have translators for the
old ioctl's, and new ioctl's.

What do you guys think ?

Cheers,
Ben.



-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: deprecating fix->mmio_start and smem_start

[Geert Uytterhoeven]

On Tue, 22 Apr 2008, Benjamin Herrenschmidt wrote:
> We currently have a problem with those two members of struct
> fb_fix_screeninfo. The struct contains an "unsigned long" which means
> that:
> 
> - 64 bits kernels with 32 bits userspace can't pass a complete address
> - 32 bits machines with 64 bits resource_size_t can't pass a complete
> address
> - The structure isn't even properly padded to be 32/64 bits neutral in
> the first place.
> 
> We could define new versions of the struct with new get/set ioctls,
> or we could try to just deprecate those fields. What do you guys think ?

As userspace doesn't really need those fields[1], we can easily deprecate them.

> If we do the later, we need another way to convey the informations.
> 
> For smem, I'm not sure it's very useful, we should just be able to mmap
> the fbdev. The problem is more with mmio_start.
> 
> Thus the idea that we could do something to allow mmap'ing mmio via
> mmap of /dev/fb via some specific offset... what do you think ?

That's exactly what drivers/video/fbmem.c:fb_mmap() does[2].

But fb_mmap() does need the correct (resource_size_t) information.
That can be done by changing the offending fields in the kernel variant of
fb_fix_screeninfo to resource_size_t.

The user variant of fb_fix_screeninfo would stay the same, and only the
FBIOGET_FSCREENINFO case in drivers/video/fbmem.c:fb_ioctl() has to be
changed to convert from the kernel to the user variant.

> Or we can just do a fb_fix_screeninfo2, with proper padding and u64
> addresses, replace the kernel one with that, have translators for the
> old ioctl's, and new ioctl's.

I don't think the new ioctls are needed.

[1] Except when it wants to mmap /dev/mem using this info, but that can
    be done using [2].

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: deprecating fix->mmio_start and smem_start

[Benjamin Herrenschmidt]

> > We could define new versions of the struct with new get/set ioctls,
> > or we could try to just deprecate those fields. What do you guys think ?
> 
> As userspace doesn't really need those fields[1], we can easily deprecate them.

Well... yes and no. At least some X drivers use the mmio_start field to
map registers via /dev/mem no ?

> > If we do the later, we need another way to convey the informations.
> > 
> > For smem, I'm not sure it's very useful, we should just be able to mmap
> > the fbdev. The problem is more with mmio_start.
> > 
> > Thus the idea that we could do something to allow mmap'ing mmio via
> > mmap of /dev/fb via some specific offset... what do you think ?
> 
> That's exactly what drivers/video/fbmem.c:fb_mmap() does[2].

Ok, I had a look at it does indeed make MMIO appear after the fb. The
funny thing here is that it does use fix for that which means it's
broken on those platforms.

> But fb_mmap() does need the correct (resource_size_t) information.
> That can be done by changing the offending fields in the kernel variant of
> fb_fix_screeninfo to resource_size_t.

Yup, and we need to add some conversion to the "user" variant.

> The user variant of fb_fix_screeninfo would stay the same, and only the
> FBIOGET_FSCREENINFO case in drivers/video/fbmem.c:fb_ioctl() has to be
> changed to convert from the kernel to the user variant.

Yup. I could always set smem_start to 0 and mmio_stat to smem_len in
there while at it. This will break the day we have 4G of video RAM
though. Which is why I wonder if we should -also- make the new kernel
structure user-visible with a new ioctl, so that user space can migrate
and the day we have such monsters, we aren't totally broken ?

> [1] Except when it wants to mmap /dev/mem using this info, but that can
>     be done using [2].

Yes, but there are existing broken programs. I'm pretty sure there used
to be at least...

One option here is we can test if the values are > 32 bits, when
converting from the in-kernel 64 bits structure.

If that's not the case, we copy them as usual, thus we stay compatible
for cases that work today (such as a 64 bits G5 with 32 bits userspace
as all MMIO on the G5 is below 4G).

If they are >4G, we do something like putting 0xffffffff in smem_start
and mmio_start, which should make mmap attempts via /dev/mem to fail and
keep smem_len and mmio_len to the right values.

What do you think ?

Cheers,
Ben.



-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: deprecating fix->mmio_start and smem_start

[Geert Uytterhoeven]

On Tue, 22 Apr 2008, Benjamin Herrenschmidt wrote:
> > > We could define new versions of the struct with new get/set ioctls,
> > > or we could try to just deprecate those fields. What do you guys think ?
> > 
> > As userspace doesn't really need those fields[1], we can easily deprecate them.
> 
> Well... yes and no. At least some X drivers use the mmio_start field to
> map registers via /dev/mem no ?

Perhaps some do. But they're supposed to map mmio by mapping /dev/fb beyond
smem_len.

> > > If we do the later, we need another way to convey the informations.
> > > 
> > > For smem, I'm not sure it's very useful, we should just be able to mmap
> > > the fbdev. The problem is more with mmio_start.
> > > 
> > > Thus the idea that we could do something to allow mmap'ing mmio via
> > > mmap of /dev/fb via some specific offset... what do you think ?
> > 
> > That's exactly what drivers/video/fbmem.c:fb_mmap() does[2].
> 
> Ok, I had a look at it does indeed make MMIO appear after the fb. The
> funny thing here is that it does use fix for that which means it's
> broken on those platforms.

Yes, that's true.

> > But fb_mmap() does need the correct (resource_size_t) information.
> > That can be done by changing the offending fields in the kernel variant of
> > fb_fix_screeninfo to resource_size_t.
> 
> Yup, and we need to add some conversion to the "user" variant.
> 
> > The user variant of fb_fix_screeninfo would stay the same, and only the
> > FBIOGET_FSCREENINFO case in drivers/video/fbmem.c:fb_ioctl() has to be
> > changed to convert from the kernel to the user variant.
> 
> Yup. I could always set smem_start to 0 and mmio_stat to smem_len in
> there while at it. This will break the day we have 4G of video RAM

What does this solve? If userspace uses this to map /dev/mem, it will
map physical address 0?

Won't hurt on all platforms, but it will definitely hurt x86, as they
have system RAM at 0 ;-)

> though. Which is why I wonder if we should -also- make the new kernel
> structure user-visible with a new ioctl, so that user space can migrate
> and the day we have such monsters, we aren't totally broken ?

Hmm, that definitely means a new fb_fix_screeninfo, as smem_len is
currently u32, even on 64-bit.

> > [1] Except when it wants to mmap /dev/mem using this info, but that can
> >     be done using [2].
> 
> Yes, but there are existing broken programs. I'm pretty sure there used
> to be at least...
> 
> One option here is we can test if the values are > 32 bits, when
> converting from the in-kernel 64 bits structure.
> 
> If that's not the case, we copy them as usual, thus we stay compatible
> for cases that work today (such as a 64 bits G5 with 32 bits userspace
> as all MMIO on the G5 is below 4G).

Sounds OK to me.

> If they are >4G, we do something like putting 0xffffffff in smem_start
> and mmio_start, which should make mmap attempts via /dev/mem to fail and
> keep smem_len and mmio_len to the right values.

Sounds OK, too.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: deprecating fix->mmio_start and smem_start

[Scott D. Davilla]

>On Tue, 22 Apr 2008, Benjamin Herrenschmidt wrote:
>>  We currently have a problem with those two members of struct
>>  fb_fix_screeninfo. The struct contains an "unsigned long" which means
>>  that:
>>
>>  - 64 bits kernels with 32 bits userspace can't pass a complete address
>>  - 32 bits machines with 64 bits resource_size_t can't pass a complete
>>  address
>>  - The structure isn't even properly padded to be 32/64 bits neutral in
>>  the first place.
>>
>  > We could define new versions of the struct with new get/set ioctls,
>>  or we could try to just deprecate those fields. What do you guys think ?
>
>As userspace doesn't really need those fields[1], we can easily 
>deprecate them.
>

kexec uses FBIOGET_FSCREENINFO and FBIOGET_VSCREENINFO. So if these 
ioctls changes, it will possibly break kexec under vesafb and efifb.

Scott


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: deprecating fix->mmio_start and smem_start

[Geert Uytterhoeven]

On Tue, 22 Apr 2008, Scott D. Davilla wrote:
> >On Tue, 22 Apr 2008, Benjamin Herrenschmidt wrote:
> >>  We currently have a problem with those two members of struct
> >>  fb_fix_screeninfo. The struct contains an "unsigned long" which means
> >>  that:
> >>
> >>  - 64 bits kernels with 32 bits userspace can't pass a complete address
> >>  - 32 bits machines with 64 bits resource_size_t can't pass a complete
> >>  address
> >>  - The structure isn't even properly padded to be 32/64 bits neutral in
> >>  the first place.
> >>
> >  > We could define new versions of the struct with new get/set ioctls,
> >>  or we could try to just deprecate those fields. What do you guys think ?
> >
> >As userspace doesn't really need those fields[1], we can easily 
> >deprecate them.
> 
> kexec uses FBIOGET_FSCREENINFO and FBIOGET_VSCREENINFO. So if these 
> ioctls changes, it will possibly break kexec under vesafb and efifb.

Does kexec use the smem_start and mmio_start fields of
fb_fix_screeninfo?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: deprecating fix->mmio_start and smem_start

[Scott D. Davilla]

>On Tue, 22 Apr 2008, Scott D. Davilla wrote:
>>  >On Tue, 22 Apr 2008, Benjamin Herrenschmidt wrote:
>>  >>  We currently have a problem with those two members of struct
>>  >>  fb_fix_screeninfo. The struct contains an "unsigned long" which means
>>  >>  that:
>>  >>
>>  >>  - 64 bits kernels with 32 bits userspace can't pass a complete address
>>  >>  - 32 bits machines with 64 bits resource_size_t can't pass a complete
>>  >>  address
>>  >>  - The structure isn't even properly padded to be 32/64 bits neutral in
>>  >>  the first place.
>>  >>
>>  >  > We could define new versions of the struct with new get/set ioctls,
>>  >>  or we could try to just deprecate those fields. What do you guys think ?
>>  >
>>  >As userspace doesn't really need those fields[1], we can easily
>>  >deprecate them.
>>
>  > kexec uses FBIOGET_FSCREENINFO and FBIOGET_VSCREENINFO. So if these
>  > ioctls changes, it will possibly break kexec under vesafb and efifb.
>
>Does kexec use the smem_start and mmio_start fields of
>fb_fix_screeninfo?
>

In "kexec/arch/i386/x86-linux-setup.c", the routine 
"setup_linux_vesafb" uses smem_start, smem_len, and line_length.

This is used for vesafb and efifb framebuffer kernel boot param 
setup. This is required for PC hardware that do not contain a PC-BIOS 
and need the bootloader to setup the kernel video boot params.



-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: deprecating fix->mmio_start and smem_start

[Ville Syrjälä]

On Tue, Apr 22, 2008 at 10:00:10AM +0200, Geert Uytterhoeven wrote:
> On Tue, 22 Apr 2008, Benjamin Herrenschmidt wrote:
> > We currently have a problem with those two members of struct
> > fb_fix_screeninfo. The struct contains an "unsigned long" which means
> > that:
> > 
> > - 64 bits kernels with 32 bits userspace can't pass a complete address
> > - 32 bits machines with 64 bits resource_size_t can't pass a complete
> > address
> > - The structure isn't even properly padded to be 32/64 bits neutral in
> > the first place.
> > 
> > We could define new versions of the struct with new get/set ioctls,
> > or we could try to just deprecate those fields. What do you guys think ?
> 
> As userspace doesn't really need those fields[1], we can easily deprecate them.

DirectFB uses both.

smem_start is used to get the physical address of the frambuffer for v4l
capture purposes at least. It's also used by several of the hw specific
gfxdrivers for various reasons. For example the matrox driver uses it to
figure out the offset from the beginning of the video memory to the
portion allocated for CRTC2 by matroxfb. This only applies when matroxfb
is left in control of CRTC2. I doubt may people use DirectFB this way
since DirectFB can drive CRTC2+TVout itself and manage the full memory
for both heads. But the support is still there in the code FWIW. I'm
not actually sure what the other gfxdrivers use it for but I see several
references when I grep for it.

mmio_start is used to figure out where the registers actually lie within
the mmapped MMIO area. atyfb is the only fb driver that I know to need
this. The problem is that with new chips the aux register aperture is
used and the registers are at the beginning of that aperture but when the
aux register aperture is not used the registers occupy the last 2K
of the framebuffer. That means an offset of 2K on typical systems.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: deprecating fix->mmio_start and smem_start

[Geert Uytterhoeven]

On Tue, 22 Apr 2008, Scott D. Davilla wrote:
> > On Tue, 22 Apr 2008, Scott D. Davilla wrote:
> > >  >On Tue, 22 Apr 2008, Benjamin Herrenschmidt wrote:
> > >  >>  We currently have a problem with those two members of struct
> > >  >>  fb_fix_screeninfo. The struct contains an "unsigned long" which means
> > >  >>  that:
> > >  >>
> > >  >>  - 64 bits kernels with 32 bits userspace can't pass a complete
> > > address
> > >  >>  - 32 bits machines with 64 bits resource_size_t can't pass a complete
> > >  >>  address
> > >  >>  - The structure isn't even properly padded to be 32/64 bits neutral
> > > in
> > >  >>  the first place.
> > >  >>
> > >  >  > We could define new versions of the struct with new get/set ioctls,
> > >  >>  or we could try to just deprecate those fields. What do you guys
> > > think ?
> > >  >
> > >  >As userspace doesn't really need those fields[1], we can easily
> > >  >deprecate them.
> > > 
> >  > kexec uses FBIOGET_FSCREENINFO and FBIOGET_VSCREENINFO. So if these
> >  > ioctls changes, it will possibly break kexec under vesafb and efifb.
> > 
> > Does kexec use the smem_start and mmio_start fields of
> > fb_fix_screeninfo?
> > 
> 
> In "kexec/arch/i386/x86-linux-setup.c", the routine "setup_linux_vesafb" uses
> smem_start, smem_len, and line_length.
> 
> This is used for vesafb and efifb framebuffer kernel boot param setup. This is
> required for PC hardware that do not contain a PC-BIOS and need the bootloader
> to setup the kernel video boot params.

OK, but the problem doesn't show up on x86-32 (no >32-bit physical
addresses yet), as x86_linux_param_header.lfb_base is uint32_t and there's no
way it's gonna change.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: deprecating fix->mmio_start and smem_start

[Scott D. Davilla]

>On Tue, 22 Apr 2008, Scott D. Davilla wrote:
>>  > On Tue, 22 Apr 2008, Scott D. Davilla wrote:
>>  > >  >On Tue, 22 Apr 2008, Benjamin Herrenschmidt wrote:
>>  > >  >>  We currently have a problem with those two members of struct
>>  > >  >>  fb_fix_screeninfo. The struct contains an "unsigned long" 
>>which means
>>  > >  >>  that:
>>  > >  >>
>>  > >  >>  - 64 bits kernels with 32 bits userspace can't pass a complete
>  > > > address
>>  > >  >>  - 32 bits machines with 64 bits resource_size_t can't 
>>pass a complete
>>  > >  >>  address
>>  > >  >>  - The structure isn't even properly padded to be 32/64 bits neutral
>>  > > in
>>  > >  >>  the first place.
>>  > >  >>
>>  > >  >  > We could define new versions of the struct with new 
>>get/set ioctls,
>>  > >  >>  or we could try to just deprecate those fields. What do you guys
>>  > > think ?
>>  > >  >
>>  > >  >As userspace doesn't really need those fields[1], we can easily
>>  > >  >deprecate them.
>>  > >
>>  >  > kexec uses FBIOGET_FSCREENINFO and FBIOGET_VSCREENINFO. So if these
>>  >  > ioctls changes, it will possibly break kexec under vesafb and efifb.
>>  >
>>  > Does kexec use the smem_start and mmio_start fields of
>>  > fb_fix_screeninfo?
>>  >
>>
>>  In "kexec/arch/i386/x86-linux-setup.c", the routine 
>>"setup_linux_vesafb" uses
>>  smem_start, smem_len, and line_length.
>>
>>  This is used for vesafb and efifb framebuffer kernel boot param 
>>setup. This is
>>  required for PC hardware that do not contain a PC-BIOS and need 
>>the bootloader
>>  to setup the kernel video boot params.
>
>OK, but the problem doesn't show up on x86-32 (no >32-bit physical
>addresses yet), as x86_linux_param_header.lfb_base is uint32_t and there's no
>way it's gonna change.
>

I seem to remember the EFI/kexec guys have been doing kexec from 
32->64 kernel and I think 64->32. I'm 32 bit only so I have no 
experience under 64 bit kernels and don't really pay much attention 
to 64 bit kernel patches. Huang Ying was doing the EFI/kexec patches 
a few months ago.



-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: deprecating fix->mmio_start and smem_start

[Benjamin Herrenschmidt]

On Tue, 2008-04-22 at 21:41 +0300, Ville Syrjälä wrote:
> DirectFB uses both.
> 
> smem_start is used to get the physical address of the frambuffer for v4l
> capture purposes at least. It's also used by several of the hw specific
> gfxdrivers for various reasons. For example the matrox driver uses it to
> figure out the offset from the beginning of the video memory to the
> portion allocated for CRTC2 by matroxfb. This only applies when matroxfb
> is left in control of CRTC2. I doubt may people use DirectFB this way
> since DirectFB can drive CRTC2+TVout itself and manage the full memory
> for both heads. But the support is still there in the code FWIW. I'm
> not actually sure what the other gfxdrivers use it for but I see several
> references when I grep for it.
> 
> mmio_start is used to figure out where the registers actually lie within
> the mmapped MMIO area. atyfb is the only fb driver that I know to need
> this. The problem is that with new chips the aux register aperture is
> used and the registers are at the beginning of that aperture but when the
> aux register aperture is not used the registers occupy the last 2K
> of the framebuffer. That means an offset of 2K on typical systems.

So DirectFB can be fixed to use the "new" structure with 64 bits offsets
if we define such a thing right ? (with new ioctl's to access it).

Cheers,
Ben.




-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: deprecating fix->mmio_start and smem_start

[Benjamin Herrenschmidt]

On Tue, 2008-04-22 at 14:38 +0200, Geert Uytterhoeven wrote:
> > though. Which is why I wonder if we should -also- make the new
> kernel
> > structure user-visible with a new ioctl, so that user space can
> migrate
> > and the day we have such monsters, we aren't totally broken ?
> 
> Hmm, that definitely means a new fb_fix_screeninfo, as smem_len is
> currently u32, even on 64-bit.

Yup, I think that's the way to go.

I'll cook up a patch one of these days.

Cheers,
Ben.



-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone