On Tue, 21 Sep 2010 08:39:05 Florian Tobias Schandinat [off-list ref] wrote:

Bruno Prémont schrieb:

quoted

On Tue, 21 Sep 2010 00:28:27 Florian Tobias Schandinat [off-list ref] wrote:

quoted

quoted

Tracking if/how often framebuffer is opened as such is a separate thing (though
all users that have the framebuffer opened hold a reference to fb_info).

That's what I said. So as long as refcount <= 1 it does not matter whether you 
just count on open/release or additionally on every framebuffer operation, just 
that the later produces more noise.

Hm, I don't count on every framebuffer operation... in most cases
fb_info is provided as function argument, in which case no further
counting is needed as the caller has a valid reference.

With my patch applied refcount for registered but unsed framebuffer was
2 (once for the driver, once for registered_fb entry) and went up to 3
when userspace opened framebuffer. fbcon's usage only incremented
refcount for very short timeframes when effectively using fb_info.

When starting with the FB minor I have to take a new reference.
(though I maybe should check if file's private data is set and use
that reference instead of looking up fb_info by minor as is currently
done)

For fbcon all the references are taken by FB minor (I wondered why
fbcon only remembers index into registered_fb aka minor instead of
fb_info itself)

True, I guess fb infrastructure and fbcon both could use a lot of work. At the 
moment I am more at fixing my driver but once that's done to an acceptable level 
I think I'll give it a try, too.

This year someone said he/she would look at making it possible to have
multiple concurrently active consoles on distinct framebuffers.
Hopefully something is happening on that front (that would certainly
also include some fbcon cleanup)

Maybe I should first change fbcon to stop using indexes into
registered_fb but use fb_info right away (this would also make it
possible to soften/drop the max_fb limit of 15 at a later time, e.g. by
using dynamic major/minors)
This way fbcon would not by affected (or at least much less affected)
by the refcounting of fb_info.

quoted

quoted

So I still don't see any advantage in counting users + uses.
Please note that I do not object the idea of the patch itself, it's only that I 
have a different preference on what to count. I only want to express that your 
way is more complicated than what I would recommend.

I don't think I see how you would do the refcounting... would you just
drop the changes in fb_open() and fb_release()?
Could you describe your approach (with pseudo-code) or the differences
to mine?

No, quite the opposite.
I would increase the refcount in fb_open in fbmem.c and in fbcon.c where 
fbops->fb_open is called; decrease the refcount on fb_release in fbmem.c and in 
fbcon.c where fbops->fb_release is called. This would be only 6 places in total 
which need to be changed and would be the same as what you said is currently used.
As we agree (I hope) that for framebuffer operations an already open framebuffer 
is required this would not change when the framebuffer will be freed (compared 
to your counting) so the part changing how a framebuffer is shut down can be 
changed just as you proposed. The advantage is that it does not require changes 
to framebuffer ioctls/read/write/mmap and probably also much less changes in 
fbcon. And if this is also what drivers want and there is no conflict with what 
you want, I don't see any reason to not provide this service but force them to 
do this kind of refcounting on their own.

Ok, will see if I can do it that way. But from Guennadi's reply it
seems users that can be synchronously notified of fb-changes would need
to be accounted for separately from the others (maybe they don't need
to be accounted for at all)

Though I definitely don't like current access to registered_fb
array which looks racy (I've seen nothing that prevents two
register_framebuffer() calls to race for the same minor or even
unregister_framebuffer to race with register_framebuffer())
fbcon's access should be safe enough though via the notification events
that are serialized with console sem.

Thanks,
Bruno

Thanks,

Florian Tobias Schandinat