Problem with framebuffer mmap on platforms with large addressing

Problem with framebuffer mmap on platforms with large addressing

[Dmitry Eremin-Solenikov]

Hello,

I'm trying to make framebuffer to work on PPC460EX board (canyonlands).

The peculiarity of this platform is the fact that it has
sizeof(unsigned long) = 4,
but physical address on it is 36 bits width. It is a common to various pieces
of the code to expect that unsigned long variable is able to contain physical
address. Most of those places are easy to fix.

The problem I'm stuck with is a fb_mmap() code. To find a right memory to map
it uses information from struct fb_fix_screeninfo provided by the driver.
This structure uses two unsigned long fields to hold physical addresses
(smem_start and mmio_start). It would be easy to change that structure
to use phys_addr_t instead of unsigned long, but this structure is a part
of userspace ABI. It is returned to userspace on FBIOGET_FSCREENINFO ioctl.
And now I'm stuck with it.

In my driver code I have just overwritten the fb_mmap function with
driver-private
fb_mmap callback supporting 64-bit addressing, but this doesn't look like
a generic and correct solution.

What is the best way to fix this problem? Should we break ABI with the goal
of correctness? Should we add new FBIOGET_FSCREENINFO2, which will
return a correct structure with phys_addr_t (or simply u64) fields and make
FBIOGET_FSCREENINFO a wrapper returning partially bogus structure
(with smem_start and mmio_start fields being truncated to just unsigned long)?
What would developers recommend?


Thank you.


-- 
With best wishes
Dmitry

Re: Problem with framebuffer mmap on platforms with large addressing

[Benjamin Herrenschmidt]

On Sat, 2012-03-17 at 20:04 +0400, Dmitry Eremin-Solenikov wrote:
> Hello,
> 
> I'm trying to make framebuffer to work on PPC460EX board (canyonlands).
> 
> The peculiarity of this platform is the fact that it has
> sizeof(unsigned long) = 4,
> but physical address on it is 36 bits width. It is a common to various pieces
> of the code to expect that unsigned long variable is able to contain physical
> address. Most of those places are easy to fix.

Yes. In fact, Tony (CC) has some patches to fix a lot of the DRM
infrastructure (we have radeon KMS working on a similar platform).

> The problem I'm stuck with is a fb_mmap() code. To find a right memory to map
> it uses information from struct fb_fix_screeninfo provided by the driver.
> This structure uses two unsigned long fields to hold physical addresses
> (smem_start and mmio_start). It would be easy to change that structure
> to use phys_addr_t instead of unsigned long, but this structure is a part
> of userspace ABI. It is returned to userspace on FBIOGET_FSCREENINFO ioctl.
> And now I'm stuck with it.

It's an old problem, which I think we described a while back on the
list. Back then the conclusion was to make a new version with a proper
__u64, a new ioctl to access is, and a "compatible" ioctl that blanks
the address fields (or fails) if they contain a value >32-bit.

We just never got to actually implement it.

In fact, we could make the new structure such that it doesn't break
userspace compatibility with 64-bit architectures at all, ie, the "new"
and "compat" ioctl could remain entirely equivalent on 64-bit.
 
> In my driver code I have just overwritten the fb_mmap function with
> driver-private
> fb_mmap callback supporting 64-bit addressing, but this doesn't look like
> a generic and correct solution.
> 
> What is the best way to fix this problem? Should we break ABI with the goal
> of correctness? Should we add new FBIOGET_FSCREENINFO2, which will
> return a correct structure with phys_addr_t (or simply u64) fields and make
> FBIOGET_FSCREENINFO a wrapper returning partially bogus structure
> (with smem_start and mmio_start fields being truncated to just unsigned long)?
> What would developers recommend?

Cheers,
Ben.

Re: Problem with framebuffer mmap on platforms with large addressing

[Dmitry Eremin-Solenikov]

On Sun, Mar 18, 2012 at 4:46 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Sat, 2012-03-17 at 20:04 +0400, Dmitry Eremin-Solenikov wrote:
>> Hello,
>>
>> I'm trying to make framebuffer to work on PPC460EX board (canyonlands).
>>
>> The peculiarity of this platform is the fact that it has
>> sizeof(unsigned long) = 4,
>> but physical address on it is 36 bits width. It is a common to various pieces
>> of the code to expect that unsigned long variable is able to contain physical
>> address. Most of those places are easy to fix.
>
> Yes. In fact, Tony (CC) has some patches to fix a lot of the DRM
> infrastructure (we have radeon KMS working on a similar platform).

That is interesting! Are those patches published or otherwise available
somewhere? We are also very interested in enabling Canyonlands
with Radeon KMS!

>
>> The problem I'm stuck with is a fb_mmap() code. To find a right memory to map
>> it uses information from struct fb_fix_screeninfo provided by the driver.
>> This structure uses two unsigned long fields to hold physical addresses
>> (smem_start and mmio_start). It would be easy to change that structure
>> to use phys_addr_t instead of unsigned long, but this structure is a part
>> of userspace ABI. It is returned to userspace on FBIOGET_FSCREENINFO ioctl.
>> And now I'm stuck with it.
>
> It's an old problem, which I think we described a while back on the
> list. Back then the conclusion was to make a new version with a proper
> __u64, a new ioctl to access is, and a "compatible" ioctl that blanks
> the address fields (or fails) if they contain a value >32-bit.
>
> We just never got to actually implement it.

I see. I will try to prepare patches.

>
> In fact, we could make the new structure such that it doesn't break
> userspace compatibility with 64-bit architectures at all, ie, the "new"
> and "compat" ioctl could remain entirely equivalent on 64-bit.

I remember stuff about compat_ioctl, but I have never used/implemented
that. Are there any details of requirements for the structures being passed?

-- 
With best wishes
Dmitry

Re: Problem with framebuffer mmap on platforms with large addressing

[Benjamin Herrenschmidt]

On Sun, 2012-03-18 at 18:04 +0400, Dmitry Eremin-Solenikov wrote:
> On Sun, Mar 18, 2012 at 4:46 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > On Sat, 2012-03-17 at 20:04 +0400, Dmitry Eremin-Solenikov wrote:
> >> Hello,
> >>
> >> I'm trying to make framebuffer to work on PPC460EX board (canyonlands).
> >>
> >> The peculiarity of this platform is the fact that it has
> >> sizeof(unsigned long) = 4,
> >> but physical address on it is 36 bits width. It is a common to various pieces
> >> of the code to expect that unsigned long variable is able to contain physical
> >> address. Most of those places are easy to fix.
> >
> > Yes. In fact, Tony (CC) has some patches to fix a lot of the DRM
> > infrastructure (we have radeon KMS working on a similar platform).
> 
> That is interesting! Are those patches published or otherwise available
> somewhere? We are also very interested in enabling Canyonlands
> with Radeon KMS!

You will run into additional problems with 460 due to the fact that it's
not cache coherent for DMA. Tony patches don't address that part of the
problem (they were used on a 476 based platform).

> > In fact, we could make the new structure such that it doesn't break
> > userspace compatibility with 64-bit architectures at all, ie, the "new"
> > and "compat" ioctl could remain entirely equivalent on 64-bit.
> 
> I remember stuff about compat_ioctl, but I have never used/implemented
> that. Are there any details of requirements for the structures being passed?

In that specific case, I meant something else. IE. The old ioctl could
remain unchanged, and the new ioctl make the same as the old one on
64-bit platforms.

Cheers,
Ben.

Re: Problem with framebuffer mmap on platforms with large addressing

[Dmitry Eremin-Solenikov]

On Mon, Mar 19, 2012 at 12:49 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Sun, 2012-03-18 at 18:04 +0400, Dmitry Eremin-Solenikov wrote:
>> On Sun, Mar 18, 2012 at 4:46 AM, Benjamin Herrenschmidt
>> <benh@kernel.crashing.org> wrote:
>> > On Sat, 2012-03-17 at 20:04 +0400, Dmitry Eremin-Solenikov wrote:
>> >> Hello,
>> >>
>> >> I'm trying to make framebuffer to work on PPC460EX board (canyonlands).
>> >>
>> >> The peculiarity of this platform is the fact that it has
>> >> sizeof(unsigned long) = 4,
>> >> but physical address on it is 36 bits width. It is a common to various pieces
>> >> of the code to expect that unsigned long variable is able to contain physical
>> >> address. Most of those places are easy to fix.
>> >
>> > Yes. In fact, Tony (CC) has some patches to fix a lot of the DRM
>> > infrastructure (we have radeon KMS working on a similar platform).
>>
>> That is interesting! Are those patches published or otherwise available
>> somewhere? We are also very interested in enabling Canyonlands
>> with Radeon KMS!
>
> You will run into additional problems with 460 due to the fact that it's
> not cache coherent for DMA. Tony patches don't address that part of the
> problem (they were used on a 476 based platform).

Hmm. Could you please spill a little bit more of details? Also are those patches
for 476 merged or present somewhere?

>
>> > In fact, we could make the new structure such that it doesn't break
>> > userspace compatibility with 64-bit architectures at all, ie, the "new"
>> > and "compat" ioctl could remain entirely equivalent on 64-bit.
>>
>> I remember stuff about compat_ioctl, but I have never used/implemented
>> that. Are there any details of requirements for the structures being passed?
>
> In that specific case, I meant something else. IE. The old ioctl could
> remain unchanged, and the new ioctl make the same as the old one on
> 64-bit platforms.

I don't think this kind of magic would be good. I'd just stick to the new
ioctl.

>
> Cheers,
> Ben.
>
>
>



-- 
With best wishes
Dmitry

Re: Problem with framebuffer mmap on platforms with large addressing

[Benjamin Herrenschmidt]

> >> That is interesting! Are those patches published or otherwise available
> >> somewhere? We are also very interested in enabling Canyonlands
> >> with Radeon KMS!
> >
> > You will run into additional problems with 460 due to the fact that it's
> > not cache coherent for DMA. Tony patches don't address that part of the
> > problem (they were used on a 476 based platform).
> 
> Hmm. Could you please spill a little bit more of details? Also are those patches
> for 476 merged or present somewhere?

Well, DMA on 46x isn't cache coherent. The DRM plays interesting games
with mapping/unmapping pages for DMA by the chip and I don't think we
have the right hooks to do the appropriate cache flushing on these guys,
but Tony might be able to comment, I don't know whether he tried or not.

On the other hand 476 has fully cache coherent DMA so the only problem
there is the >32-bit physical address space.

As for the patches, you'll have to wait for Tony to respond (I'll poke
him locally).

Cheers,
Ben.

> >> > In fact, we could make the new structure such that it doesn't break
> >> > userspace compatibility with 64-bit architectures at all, ie, the "new"
> >> > and "compat" ioctl could remain entirely equivalent on 64-bit.
> >>
> >> I remember stuff about compat_ioctl, but I have never used/implemented
> >> that. Are there any details of requirements for the structures being passed?
> >
> > In that specific case, I meant something else. IE. The old ioctl could
> > remain unchanged, and the new ioctl make the same as the old one on
> > 64-bit platforms.
> 
> I don't think this kind of magic would be good. I'd just stick to the new
> ioctl.
> 
> >
> > Cheers,
> > Ben.
> >
> >
> >
> 
> 
> 

Re: Problem with framebuffer mmap on platforms with large addressing

[Florian Tobias Schandinat]

On 03/19/2012 02:42 PM, Dmitry Eremin-Solenikov wrote:
> On Mon, Mar 19, 2012 at 12:49 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
>> On Sun, 2012-03-18 at 18:04 +0400, Dmitry Eremin-Solenikov wrote:
>>> On Sun, Mar 18, 2012 at 4:46 AM, Benjamin Herrenschmidt
>>> <benh@kernel.crashing.org> wrote:
>>>> In fact, we could make the new structure such that it doesn't break
>>>> userspace compatibility with 64-bit architectures at all, ie, the "new"
>>>> and "compat" ioctl could remain entirely equivalent on 64-bit.
>>>
>>> I remember stuff about compat_ioctl, but I have never used/implemented
>>> that. Are there any details of requirements for the structures being passed?
>>
>> In that specific case, I meant something else. IE. The old ioctl could
>> remain unchanged, and the new ioctl make the same as the old one on
>> 64-bit platforms.
> 
> I don't think this kind of magic would be good. I'd just stick to the new
> ioctl.

I finally found where we started to discuss this issue, for reference
"sm501fb.c: support mmap on PPC440SPe/PPC440EPx" back in May 2010.

The thing I don't remember is why we consider exporting the physical
address to userspace desirable (or even necessary). Fixing the generic
mmap would be trivial without breaking or adding any userspace ABI, I
think. Just adding those things to fb_info and adjusting fb_mmap should
do the trick, shouldn't it?


Best regards,

Florian Tobias Schandinat

Re: Problem with framebuffer mmap on platforms with large addressing

[Benjamin Herrenschmidt]

On Wed, 2012-03-21 at 19:45 +0000, Florian Tobias Schandinat wrote:
> I finally found where we started to discuss this issue, for reference
> "sm501fb.c: support mmap on PPC440SPe/PPC440EPx" back in May 2010.
> 
> The thing I don't remember is why we consider exporting the physical
> address to userspace desirable (or even necessary). Fixing the generic
> mmap would be trivial without breaking or adding any userspace ABI, I
> think. Just adding those things to fb_info and adjusting fb_mmap
> should
> do the trick, shouldn't it?

For historical reasons, things like X or directfb used it. Modern X
drivers shouldn't any more, but heh...

Cheers,
Ben.

Re: Problem with framebuffer mmap on platforms with large addressing

[Dmitry Eremin-Solenikov]

On Tue, Mar 20, 2012 at 9:40 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
>> >> That is interesting! Are those patches published or otherwise available
>> >> somewhere? We are also very interested in enabling Canyonlands
>> >> with Radeon KMS!
>> >
>> > You will run into additional problems with 460 due to the fact that it's
>> > not cache coherent for DMA. Tony patches don't address that part of the
>> > problem (they were used on a 476 based platform).
>>
>> Hmm. Could you please spill a little bit more of details? Also are those patches
>> for 476 merged or present somewhere?
>
> Well, DMA on 46x isn't cache coherent. The DRM plays interesting games
> with mapping/unmapping pages for DMA by the chip and I don't think we
> have the right hooks to do the appropriate cache flushing on these guys,
> but Tony might be able to comment, I don't know whether he tried or not.
>
> On the other hand 476 has fully cache coherent DMA so the only problem
> there is the >32-bit physical address space.
>
> As for the patches, you'll have to wait for Tony to respond (I'll poke
> him locally).

Any news on these patches? A dirty and "not for the upstream yet" version
would be sufficient for me for now.

>
> Cheers,
> Ben.
>

-- 
With best wishes
Dmitry

Re: Problem with framebuffer mmap on platforms with large addressing

[Benjamin Herrenschmidt]

On Mon, 2012-04-09 at 20:18 +0400, Dmitry Eremin-Solenikov wrote:
> > As for the patches, you'll have to wait for Tony to respond (I'll
> poke
> > him locally).
> 
> Any news on these patches? A dirty and "not for the upstream yet"
> version
> would be sufficient for me for now.

I'll poke again..

Cheers,
Ben.