Re: [PATCH 2/3] Define fb_open_adj_file()

[Daniel Vetter]

On Tue, May 24, 2016 at 6:28 PM, Max Staudt <mstaudt@xxxxxxx> wrote:
> Hi Daniel,
>
> Thanks for the feedback! Comments below:
>
>
> On 05/23/2016 03:44 PM, Daniel Vetter wrote:
>> Do we _really_ care about fbdev mmap support so much that we want to add
>> more hacks all over the place (in each driver) to make it work? Given that
>> fbdev is officially in the "no more drivers" territory, I'm not sure we
>> want this either.
>>
>> The trouble is that fbdev mmap and kms dumb buffer mmap have different
>> semantics and so every driver needs to implement both if they're special
>> in any kind of way.
>
> DRM drivers already have fbdev hacks in place in order to implement both semantics.

A lot of these fbdev hacks just disappeared since we've gained generic
defio support in 4.7. Is your assertion still true on 4.7? I really
don't want to grow hacks all over the place for something we should
try to solve once for everyone.

> With this change, only bochsdrm(fb) will need to be touched, and if no further DRM driver implements fbdev support the way bochsdrmfb does, then no further driver will need this code.

See above - we've grown meanwhile enough drm drivers which need
defio/special mmap support that there's now a pile of generic code and
refactoring in 4.7. It's not just bochsdrm by far. Also pretty much
every drm driver has their own pile of fbdev mmap hacks.

> This patch set is really just about setting file->f_mapping in bochsdrmfb's fb_open(), so we finally get rid of the WARNING.
>
> We could also pass file as a third paramenter in fb_open(), changing the function definition in all fbdev drivers. I'm happy to change the patch in that way if this is preferred.
>
>
>> If we can't say no to fbdev mmap then I think we should implement
>> something that works for all drivers. I'm thinking of allocating just a
>> pile of pages in the fbdev emulation, and then copying them over using the
>> defio stuff we just merged for 4.7 into a dumb bo, plus calling dirtyfb.
>> Or at least something along those lines. Of couse just opt-in, in case the
>> driver can do a more traditional mmio fbdev mmapping.
>
> Well, the mmap code is standard fbdev and it's already there, so why not keep it?
>
> All this patch series does is allowing bochsdrmfb to work properly until fbdev support is finally removed from the kernel.

I don't think we can remove fbdev in the next 10 years unfortunately.
It's deprecated insofar that no new drivers are accepted. Instead
proper kms drivers (with fbdev emulation) should be created.

But that means another decade of new drivers in kms that need to add
hacks for fbdev mmap support, if we don't start to make this simpler
and avoid the need for such special cases.

Anyway, that's kinda the high-level reasons why I jumped on this. I
think for the next steps in the discussion you need to take a look at
what's new in 4.7. And then we need to figure out what we need to add
to improve fbdev mmap further.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch