All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] drm/fb-helper/generic: Only restore when in use
@ 2018-11-26 15:38 Noralf Trønnes
  2018-11-26 20:07 ` Daniel Vetter
  0 siblings, 1 reply; 3+ messages in thread
From: Noralf Trønnes @ 2018-11-26 15:38 UTC (permalink / raw)
  To: dri-devel

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(-)

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

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] drm/fb-helper/generic: Only restore when in use
  2018-11-26 15:38 [PATCH v2] drm/fb-helper/generic: Only restore when in use Noralf Trønnes
@ 2018-11-26 20:07 ` Daniel Vetter
  2018-11-27 17:04   ` Noralf Trønnes
  0 siblings, 1 reply; 3+ messages in thread
From: Daniel Vetter @ 2018-11-26 20:07 UTC (permalink / raw)
  To: Noralf Trønnes; +Cc: dri-devel

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] drm/fb-helper/generic: Only restore when in use
  2018-11-26 20:07 ` Daniel Vetter
@ 2018-11-27 17:04   ` Noralf Trønnes
  0 siblings, 0 replies; 3+ messages in thread
From: Noralf Trønnes @ 2018-11-27 17:04 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-11-27 17:04 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-26 15:38 [PATCH v2] drm/fb-helper/generic: Only restore when in use Noralf Trønnes
2018-11-26 20:07 ` Daniel Vetter
2018-11-27 17:04   ` Noralf Trønnes

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.