| Message ID | 20181126153848.64966-1-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/fb-helper/generic: Only restore when in use | expand |
On Mon, Nov 26, 2018 at 04:38:48PM +0100, Noralf Trønnes wrote: > On drm_driver->last_close the generic fbdev emulation will restore fbdev > regardless of it being used or not. This is a problem for e-ink displays > which don't want to be overwritten with zeroes when DRM userspace closes. > > Amend this by adding an open counter to track use in order to know when > to restore. > > v1: Avoid special-casing and move the check to drm_fbdev_client_restore() > (Daniel Vetter) > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > --- > > I was tied up in my previous attempt to solve this which predated the > generic emulation, so I failed to see that I could solve this within > the scope of the generic fbdev. Still kinda annoying that between this and drm_fb_helper_is_bound we'll have mutliple ways to figure out who owns the fbdev. And the special-casing is still there, it's just moved around - it still only works for drivers using the new client helpers. The other issue with not putting this into is_bound() is that we now have different rules between hotplug and restore (lastclose is only called when there's not master and hence is_bound() will return true). So if you lastclose, it won't be restored, but if you then plug something it, it will. So I still think we should fix this consistently for all drivers (and yeah that means rolling out fb_open/release callbacks), and also consistently between hotplug and lastclose. -Daniel > drivers/gpu/drm/drm_fb_helper.c | 24 +++++++++++++++++++++++- > include/drm/drm_fb_helper.h | 10 ++++++++++ > 2 files changed, 33 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 9a69ad7e9f3b..be93e7393704 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -2942,16 +2942,32 @@ EXPORT_SYMBOL(drm_fb_helper_output_poll_changed); > static int drm_fbdev_fb_open(struct fb_info *info, int user) > { > struct drm_fb_helper *fb_helper = info->par; > + unsigned int fb_open_count; > > if (!try_module_get(fb_helper->dev->driver->fops->owner)) > return -ENODEV; > > + mutex_lock(&fb_helper->lock); > + fb_open_count = fb_helper->fb_open_count++; > + mutex_unlock(&fb_helper->lock); > + > + if (!fb_open_count) > + drm_fb_helper_blank(FB_BLANK_UNBLANK, info); > + > return 0; > } > > static int drm_fbdev_fb_release(struct fb_info *info, int user) > { > struct drm_fb_helper *fb_helper = info->par; > + unsigned int fb_open_count; > + > + mutex_lock(&fb_helper->lock); > + fb_open_count = --fb_helper->fb_open_count; > + mutex_unlock(&fb_helper->lock); > + > + if (!fb_open_count) > + drm_fb_helper_blank(FB_BLANK_POWERDOWN, info); > > module_put(fb_helper->dev->driver->fops->owner); > > @@ -3143,8 +3159,14 @@ static void drm_fbdev_client_unregister(struct drm_client_dev *client) > static int drm_fbdev_client_restore(struct drm_client_dev *client) > { > struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client); > + unsigned int fb_open_count; > > - drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper); > + mutex_lock(&fb_helper->lock); > + fb_open_count = fb_helper->fb_open_count; > + mutex_unlock(&fb_helper->lock); > + > + if (fb_open_count) > + drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper); > > return 0; > } > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h > index bb9acea61369..e6ca1119d524 100644 > --- a/include/drm/drm_fb_helper.h > +++ b/include/drm/drm_fb_helper.h > @@ -247,6 +247,16 @@ struct drm_fb_helper { > * See also: @deferred_setup > */ > int preferred_bpp; > + > + /** > + * @fb_open_count: > + * > + * The generic fbdev emulation uses this to track userspace/fbcon opens > + * to know when to restore. > + * > + * Protected by @lock. > + */ > + unsigned int fb_open_count; > }; > > static inline struct drm_fb_helper * > -- > 2.15.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Den 26.11.2018 21.07, skrev Daniel Vetter: > On Mon, Nov 26, 2018 at 04:38:48PM +0100, Noralf Trønnes wrote: >> On drm_driver->last_close the generic fbdev emulation will restore fbdev >> regardless of it being used or not. This is a problem for e-ink displays >> which don't want to be overwritten with zeroes when DRM userspace closes. >> >> Amend this by adding an open counter to track use in order to know when >> to restore. >> >> v1: Avoid special-casing and move the check to drm_fbdev_client_restore() >> (Daniel Vetter) >> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >> --- >> >> I was tied up in my previous attempt to solve this which predated the >> generic emulation, so I failed to see that I could solve this within >> the scope of the generic fbdev. > Still kinda annoying that between this and drm_fb_helper_is_bound we'll > have mutliple ways to figure out who owns the fbdev. And the > special-casing is still there, it's just moved around - it still only > works for drivers using the new client helpers. > > The other issue with not putting this into is_bound() is that we now have > different rules between hotplug and restore (lastclose is only called when > there's not master and hence is_bound() will return true). So if you > lastclose, it won't be restored, but if you then plug something it, it > will. So I still think we should fix this consistently for all drivers > (and yeah that means rolling out fb_open/release callbacks), and also > consistently between hotplug and lastclose. Ok, I haven't got the necessary steam to tackle this broader scope right now. I'll deal with it if a bug report for the tinydrm/repaper driver shows up. Not sure if anyone is actually using DRM userspace with that driver. Btw, your tweet gave me a good laugh: "fbdev: everything is horrible" Noralf. > -Daniel > >> drivers/gpu/drm/drm_fb_helper.c | 24 +++++++++++++++++++++++- >> include/drm/drm_fb_helper.h | 10 ++++++++++ >> 2 files changed, 33 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >> index 9a69ad7e9f3b..be93e7393704 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -2942,16 +2942,32 @@ EXPORT_SYMBOL(drm_fb_helper_output_poll_changed); >> static int drm_fbdev_fb_open(struct fb_info *info, int user) >> { >> struct drm_fb_helper *fb_helper = info->par; >> + unsigned int fb_open_count; >> >> if (!try_module_get(fb_helper->dev->driver->fops->owner)) >> return -ENODEV; >> >> + mutex_lock(&fb_helper->lock); >> + fb_open_count = fb_helper->fb_open_count++; >> + mutex_unlock(&fb_helper->lock); >> + >> + if (!fb_open_count) >> + drm_fb_helper_blank(FB_BLANK_UNBLANK, info); >> + >> return 0; >> } >> >> static int drm_fbdev_fb_release(struct fb_info *info, int user) >> { >> struct drm_fb_helper *fb_helper = info->par; >> + unsigned int fb_open_count; >> + >> + mutex_lock(&fb_helper->lock); >> + fb_open_count = --fb_helper->fb_open_count; >> + mutex_unlock(&fb_helper->lock); >> + >> + if (!fb_open_count) >> + drm_fb_helper_blank(FB_BLANK_POWERDOWN, info); >> >> module_put(fb_helper->dev->driver->fops->owner); >> >> @@ -3143,8 +3159,14 @@ static void drm_fbdev_client_unregister(struct drm_client_dev *client) >> static int drm_fbdev_client_restore(struct drm_client_dev *client) >> { >> struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client); >> + unsigned int fb_open_count; >> >> - drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper); >> + mutex_lock(&fb_helper->lock); >> + fb_open_count = fb_helper->fb_open_count; >> + mutex_unlock(&fb_helper->lock); >> + >> + if (fb_open_count) >> + drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper); >> >> return 0; >> } >> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h >> index bb9acea61369..e6ca1119d524 100644 >> --- a/include/drm/drm_fb_helper.h >> +++ b/include/drm/drm_fb_helper.h >> @@ -247,6 +247,16 @@ struct drm_fb_helper { >> * See also: @deferred_setup >> */ >> int preferred_bpp; >> + >> + /** >> + * @fb_open_count: >> + * >> + * The generic fbdev emulation uses this to track userspace/fbcon opens >> + * to know when to restore. >> + * >> + * Protected by @lock. >> + */ >> + unsigned int fb_open_count; >> }; >> >> static inline struct drm_fb_helper * >> -- >> 2.15.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 9a69ad7e9f3b..be93e7393704 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -2942,16 +2942,32 @@ EXPORT_SYMBOL(drm_fb_helper_output_poll_changed); static int drm_fbdev_fb_open(struct fb_info *info, int user) { struct drm_fb_helper *fb_helper = info->par; + unsigned int fb_open_count; if (!try_module_get(fb_helper->dev->driver->fops->owner)) return -ENODEV; + mutex_lock(&fb_helper->lock); + fb_open_count = fb_helper->fb_open_count++; + mutex_unlock(&fb_helper->lock); + + if (!fb_open_count) + drm_fb_helper_blank(FB_BLANK_UNBLANK, info); + return 0; } static int drm_fbdev_fb_release(struct fb_info *info, int user) { struct drm_fb_helper *fb_helper = info->par; + unsigned int fb_open_count; + + mutex_lock(&fb_helper->lock); + fb_open_count = --fb_helper->fb_open_count; + mutex_unlock(&fb_helper->lock); + + if (!fb_open_count) + drm_fb_helper_blank(FB_BLANK_POWERDOWN, info); module_put(fb_helper->dev->driver->fops->owner); @@ -3143,8 +3159,14 @@ static void drm_fbdev_client_unregister(struct drm_client_dev *client) static int drm_fbdev_client_restore(struct drm_client_dev *client) { struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client); + unsigned int fb_open_count; - drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper); + mutex_lock(&fb_helper->lock); + fb_open_count = fb_helper->fb_open_count; + mutex_unlock(&fb_helper->lock); + + if (fb_open_count) + drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper); return 0; } diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index bb9acea61369..e6ca1119d524 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -247,6 +247,16 @@ struct drm_fb_helper { * See also: @deferred_setup */ int preferred_bpp; + + /** + * @fb_open_count: + * + * The generic fbdev emulation uses this to track userspace/fbcon opens + * to know when to restore. + * + * Protected by @lock. + */ + unsigned int fb_open_count; }; static inline struct drm_fb_helper *
On drm_driver->last_close the generic fbdev emulation will restore fbdev regardless of it being used or not. This is a problem for e-ink displays which don't want to be overwritten with zeroes when DRM userspace closes. Amend this by adding an open counter to track use in order to know when to restore. v1: Avoid special-casing and move the check to drm_fbdev_client_restore() (Daniel Vetter) Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- I was tied up in my previous attempt to solve this which predated the generic emulation, so I failed to see that I could solve this within the scope of the generic fbdev. drivers/gpu/drm/drm_fb_helper.c | 24 +++++++++++++++++++++++- include/drm/drm_fb_helper.h | 10 ++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-)