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

[Max Staudt]

On 05/24/2016 06:51 PM, Daniel Vetter wrote:
> 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


Thanks, I'll look into it.

Hm, sounds like it's best to ask the maintainer for advice, too.


Gerd, could you please have a look at this thread?



Thanks

Max