[PATCH] Add sysfs support for fbdefio delay

[PATCH] Add sysfs support for fbdefio delay

[Rick L. Vinyard Jr.]

This patch adds support for examining and modifying the fbdefio delay
parameter through sysfs. It also adds two driver definable minimum
and maximum bounds.

The default behavior is to not permit modifications if delay_max is 0,
thus preventing modification of the delay if the driver does not
explicitly permit modification.

Signed-off-by: Rick L. Vinyard, Jr <rvinyard@cs.nmsu.edu>
---
 .../ABI/testing/sysfs-class-graphics-defio         |   35 ++++++++
 drivers/video/fbsysfs.c                            |   87 ++++++++++++++++++++
 include/linux/fb.h                                 |    9 ++-
 3 files changed, 130 insertions(+), 1 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-graphics-defio

diff --git a/Documentation/ABI/testing/sysfs-class-graphics-defio b/Documentation/ABI/testing/sysfs-class-graphics-defio
new file mode 100644
index 0000000..e0ef924
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-graphics-defio
@@ -0,0 +1,35 @@
+What:		/sys/class/graphics/<fb>/defio_delay
+Date:		February 2010
+KernelVersion:	2.6.34
+Contact:	Rick L Vinyard Jr <rvinyard@cs.nmsu.edu>
+Description:
+		Set the deferred I/O delay of the framebuffer in ms.
+		This value can be used to throttle deferred I/O updates.
+		Most framebuffer devices do not have or need support for
+		deferred I/O. Accessing a framebuffer without deferred I/O
+		support will return -ENODEV. Can be read but not modified if
+		/sys/class/graphics/<fb>/defio_delay_max is 0. When modifying,
+		the value must be greater than or equal to
+		/sys/class/graphics/<fb>/defio_delay_min and less than or equal
+		to /sys/class/graphics/<fb>/defio_delay_max.
+
+What:		/sys/class/graphics/<fb>/defio_delay_min
+Date:		February 2010
+KernelVersion:	2.6.34
+Contact:	Rick L Vinyard Jr <rvinyard@cs.nmsu.edu>
+Description:
+		Minimum deferred I/O value in ms for this framebuffer.
+		This value is specified by the driver and cannot be modified
+		from sysfs. Default is 0.
+
+What:		/sys/class/graphics/<fb>/defio_delay_min
+Date:		February 2010
+KernelVersion:	2.6.34
+Contact:	Rick L Vinyard Jr <rvinyard@cs.nmsu.edu>
+Description:
+		Maximum deferred I/O value in ms for this framebuffer.
+		This value is specified by the driver and cannot be modified
+		from sysfs. Default is 0.
+		If this value is 0 /sys/class/graphics/<fb>/defio_delay cannot
+		be modified, but can be read.
+
diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
index d4a2c11..d00ea2d 100644
--- a/drivers/video/fbsysfs.c
+++ b/drivers/video/fbsysfs.c
@@ -484,6 +484,87 @@ static ssize_t show_bl_curve(struct device *device,
 }
 #endif
 
+#ifdef CONFIG_FB_DEFERRED_IO
+static ssize_t store_defio_delay(struct device *device,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct fb_info *fb_info = dev_get_drvdata(device);
+	unsigned long delay_ms = 0;
+	unsigned long delay;
+	int error;
+	char *last = NULL;
+
+	/* Check to see whether this is a deferred I/O driver */
+	if (!fb_info || !fb_info->fbdefio)
+		return -ENODEV;
+
+	/* Check whether delay_max permits setting of delay */
+	if (fb_info->fbdefio->delay_max = 0)
+		return -EPERM;
+
+	error = strict_strtoul(buf, 10, &delay_ms);
+	if (error < 0)
+		return error;
+
+	delay = delay_ms * HZ / 1000;
+
+	if (delay < fb_info->fbdefio->delay_min ||
+	    delay > fb_info->fbdefio->delay_max)
+		return -EINVAL;
+
+	fb_info->fbdefio->delay = delay;
+
+	return count;
+}
+
+static ssize_t show_defio_delay(struct device *device,
+				struct device_attribute *attr, char *buf)
+{
+	struct fb_info *fb_info = dev_get_drvdata(device);
+	unsigned long delay_ms;
+
+	/* Check to see whether this is a deferred I/O driver */
+	if (!fb_info || !fb_info->fbdefio)
+		return -ENODEV;
+
+	delay_ms = fb_info->fbdefio->delay * 1000 / HZ;
+
+	return snprintf(buf, PAGE_SIZE, "%lu\n", delay_ms);
+}
+
+static ssize_t show_defio_delay_min(struct device *device,
+				    struct device_attribute *attr, char *buf)
+{
+	struct fb_info *fb_info = dev_get_drvdata(device);
+	unsigned long delay_ms;
+
+	/* Check to see whether this is a deferred I/O driver */
+	if (!fb_info || !fb_info->fbdefio)
+		return -ENODEV;
+
+	delay_ms = fb_info->fbdefio->delay_min * 1000 / HZ;
+
+	return snprintf(buf, PAGE_SIZE, "%lu\n", delay_ms);
+}
+
+static ssize_t show_defio_delay_max(struct device *device,
+				    struct device_attribute *attr, char *buf)
+{
+	struct fb_info *fb_info = dev_get_drvdata(device);
+	unsigned long delay_ms;
+
+	/* Check to see whether this is a deferred I/O driver */
+	if (!fb_info || !fb_info->fbdefio)
+		return -ENODEV;
+
+	delay_ms = fb_info->fbdefio->delay_max * 1000 / HZ;
+
+	return snprintf(buf, PAGE_SIZE, "%lu\n", delay_ms);
+}
+
+#endif
+
 /* When cmap is added back in it should be a binary attribute
  * not a text one. Consideration should also be given to converting
  * fbdev to use configfs instead of sysfs */
@@ -503,6 +584,12 @@ static struct device_attribute device_attrs[] = {
 #ifdef CONFIG_FB_BACKLIGHT
 	__ATTR(bl_curve, S_IRUGO|S_IWUSR, show_bl_curve, store_bl_curve),
 #endif
+#ifdef CONFIG_FB_DEFERRED_IO
+	__ATTR(defio_delay, S_IRUGO|S_IWUSR,
+	       show_defio_delay, store_defio_delay),
+	__ATTR(defio_delay_min, S_IRUGO, show_defio_delay_min, NULL),
+	__ATTR(defio_delay_max, S_IRUGO, show_defio_delay_max, NULL),
+#endif
 };
 
 int fb_init_device(struct fb_info *fb_info)
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 369767b..76f35fd 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -591,8 +591,15 @@ struct fb_pixmap {
 
 #ifdef CONFIG_FB_DEFERRED_IO
 struct fb_deferred_io {
-	/* delay between mkwrite and deferred handler */
+	/* delay in jiffies between mkwrite and deferred handler */
 	unsigned long delay;
+	/* The minimum delay in jiffies that may be set through sysfs */
+	unsigned long delay_min;
+	/*
+	 * The maximum delay in jiffies that may be set through sysfs.
+	 * If delay_max is 0, delay cannot be set through sysfs.
+	 */
+	unsigned long delay_max;
 	struct mutex lock; /* mutex that protects the page list */
 	struct list_head pagelist; /* list of touched pages */
 	/* callback */
-- 
1.6.6.1

Re: [PATCH] Add sysfs support for fbdefio delay

[Bruno Prémont]

On Thu, 25 February 2010 "Rick L. Vinyard Jr." <rvinyard@cs.nmsu.edu> wrote:
> This patch adds support for examining and modifying the fbdefio delay
> parameter through sysfs. It also adds two driver definable minimum
> and maximum bounds.
> 
> The default behavior is to not permit modifications if delay_max is 0,
> thus preventing modification of the delay if the driver does not
> explicitly permit modification.
> 

...

> @@ -503,6 +584,12 @@ static struct device_attribute device_attrs[] = {
>  #ifdef CONFIG_FB_BACKLIGHT
>  	__ATTR(bl_curve, S_IRUGO|S_IWUSR, show_bl_curve, store_bl_curve),
>  #endif
> +#ifdef CONFIG_FB_DEFERRED_IO
> +	__ATTR(defio_delay, S_IRUGO|S_IWUSR,
> +	       show_defio_delay, store_defio_delay),
> +	__ATTR(defio_delay_min, S_IRUGO, show_defio_delay_min, NULL),
> +	__ATTR(defio_delay_max, S_IRUGO, show_defio_delay_max, NULL),
> +#endif
>  };
>  
>  int fb_init_device(struct fb_info *fb_info)

Would it be reasonable to add these attributes in
fb_deferred_io_init() and remove them in fb_deferred_io_cleanup()?
This would also make it possible to add write permission to delay
attribute only when it effectively can be modified.

IMHO having only attributes pertinent to the features supported by the
framebuffer is better than having all the possible ones and those of
unsupported features returning -ENODEV.

Bruno

Re: [PATCH] Add sysfs support for fbdefio delay

[Rick L. Vinyard, Jr.]

Bruno Prémont wrote:
> On Thu, 25 February 2010 "Rick L. Vinyard Jr." <rvinyard@cs.nmsu.edu>
> wrote:
>> This patch adds support for examining and modifying the fbdefio delay
>> parameter through sysfs. It also adds two driver definable minimum
>> and maximum bounds.
>>
>> The default behavior is to not permit modifications if delay_max is 0,
>> thus preventing modification of the delay if the driver does not
>> explicitly permit modification.
>>
>
> ...
>
>> @@ -503,6 +584,12 @@ static struct device_attribute device_attrs[] = {
>>  #ifdef CONFIG_FB_BACKLIGHT
>>  	__ATTR(bl_curve, S_IRUGO|S_IWUSR, show_bl_curve, store_bl_curve),
>>  #endif
>> +#ifdef CONFIG_FB_DEFERRED_IO
>> +	__ATTR(defio_delay, S_IRUGO|S_IWUSR,
>> +	       show_defio_delay, store_defio_delay),
>> +	__ATTR(defio_delay_min, S_IRUGO, show_defio_delay_min, NULL),
>> +	__ATTR(defio_delay_max, S_IRUGO, show_defio_delay_max, NULL),
>> +#endif
>>  };
>>
>>  int fb_init_device(struct fb_info *fb_info)
>
> Would it be reasonable to add these attributes in
> fb_deferred_io_init() and remove them in fb_deferred_io_cleanup()?
> This would also make it possible to add write permission to delay
> attribute only when it effectively can be modified.
>
> IMHO having only attributes pertinent to the features supported by the
> framebuffer is better than having all the possible ones and those of
> unsupported features returning -ENODEV.
>
> Bruno
>

I looked at that, but it seems like all the sysfs stuff has been
co-located in fbsysfs.c, so I tried to just follow the pattern of the
backlight code which takes the same approach.

-- 

Rick

Re: [PATCH] Add sysfs support for fbdefio delay

[Jaya Kumar]

Hi Rick,

On Fri, Feb 26, 2010 at 6:15 AM, Rick L. Vinyard Jr.
<rvinyard@cs.nmsu.edu> wrote:
> This patch adds support for examining and modifying the fbdefio delay
> parameter through sysfs. It also adds two driver definable minimum
> and maximum bounds.

Thanks for posting this. I've now read through this patch and your
past patches, but I couldn't get a good understanding of how this
userspace exposed defio delay would interact with most existing
systems.

> +               Set the deferred I/O delay of the framebuffer in ms.
> +               This value can be used to throttle deferred I/O updates.

Please help us understand how userspace should use the ability to set
this delay. Who in userspace is intended to use it? Would this be the
X server, cairo, or papyrus? A separate daemon? Human interaction?
When should said userspace entity throttle updates? How would this
entity know when throttling is needed? Could this throttling possibly
or should this possibly be done automatically by the driver itself?

> +               Most framebuffer devices do not have or need support for
> +               deferred I/O. Accessing a framebuffer without deferred I/O
> +               support will return -ENODEV. Can be read but not modified if
> +               /sys/class/graphics/<fb>/defio_delay_max is 0. When modifying,
> +               the value must be greater than or equal to
> +               /sys/class/graphics/<fb>/defio_delay_min and less than or equal
> +               to /sys/class/graphics/<fb>/defio_delay_max.

Please help the reader of above understand the sequence of userspace
changing the defio delay, and how and when that would affect the
associated drivers. Will the delay behaviour be standard for all defio
client drivers? What happens in all the various timing scenarios, eg:
if a defio delay is changed in the middle of a display update or a
defio page clean or before? The sysfs parameter description mentioning
min/max above could use some elaboration as reading it doesn't make
clear if userspace can also affect min/max or if it is completely
owned by the driver.

Thanks,
jaya

ps: I only found your patches by accident when reading through the
fbdev mailing list. I'm very interested in defio (I happen to be the
author of it) and also drivers that use defio so I would appreciate
being CCed on such changes. Thanks a bunch.

Re: [PATCH] Add sysfs support for fbdefio delay

[Bruno Prémont]

Hi Jaya,

On Fri, 26 February 2010 Jaya Kumar <jayakumar.lkml@gmail.com> wrote:
> On Fri, Feb 26, 2010 at 6:15 AM, Rick L. Vinyard Jr.
> <rvinyard@cs.nmsu.edu> wrote:
> > This patch adds support for examining and modifying the fbdefio
> > delay parameter through sysfs. It also adds two driver definable
> > minimum and maximum bounds.
> 
> Thanks for posting this. I've now read through this patch and your
> past patches, but I couldn't get a good understanding of how this
> userspace exposed defio delay would interact with most existing
> systems.

In the case of my PicoLCD driver, the hardware can barely do 2 full
redraws of the display per second but in case only a single tile is
affected it can do more refreshes of such a single tile per second.

> > +               Set the deferred I/O delay of the framebuffer in ms.
> > +               This value can be used to throttle deferred I/O updates.
> 
> Please help us understand how userspace should use the ability to set
> this delay. Who in userspace is intended to use it? Would this be the
> X server, cairo, or papyrus? A separate daemon? Human interaction?
> When should said userspace entity throttle updates? How would this
> entity know when throttling is needed? Could this throttling possibly
> or should this possibly be done automatically by the driver itself?

For me the driver would start with a default delay that matches the
full-redraw throughput of the device but userspace could reduce the
delay when it knows it will mostly just refresh small parts of the
display (one or two tiles) and would like those done at a higher rate.

A sample application would be displaying a media player interface
like the one of XMMS and clones where Umeter (the part displaying
volume per frequency range) could be refreshed ten times a second,
the current position once a second and all the rest only on song
change.

Knowing the size of the display, probability that it's being used
directly by X server is very small, it would rather be some application
using it as a sideport display.

> > +               Most framebuffer devices do not have or need support for
> > +               deferred I/O. Accessing a framebuffer without deferred I/O
> > +               support will return -ENODEV. Can be read but not modified if
> > +               /sys/class/graphics/<fb>/defio_delay_max is 0. When modifying,
> > +               the value must be greater than or equal to
> > +               /sys/class/graphics/<fb>/defio_delay_min and less than or equal
> > +               to /sys/class/graphics/<fb>/defio_delay_max.
> 
> Please help the reader of above understand the sequence of userspace
> changing the defio delay, and how and when that would affect the
> associated drivers. Will the delay behaviour be standard for all defio
> client drivers? What happens in all the various timing scenarios, eg:
> if a defio delay is changed in the middle of a display update or a
> defio page clean or before? The sysfs parameter description mentioning
> min/max above could use some elaboration as reading it doesn't make
> clear if userspace can also affect min/max or if it is completely
> owned by the driver.

Thanks,
Bruno

Re: [PATCH] Add sysfs support for fbdefio delay

[Jaya Kumar]

On Fri, Feb 26, 2010 at 6:53 PM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
> For me the driver would start with a default delay that matches the
> full-redraw throughput of the device but userspace could reduce the
> delay when it knows it will mostly just refresh small parts of the
> display (one or two tiles) and would like those done at a higher rate.

Who in userspace will know to reduce the delay? How will it know that
the delay should be reduced?

>
> A sample application would be displaying a media player interface
> like the one of XMMS and clones where Umeter (the part displaying
> volume per frequency range) could be refreshed ten times a second,
> the current position once a second and all the rest only on song
> change.

xmms/umeter will talk to this sysfs entry?

>
> Knowing the size of the display, probability that it's being used
> directly by X server is very small, it would rather be some application
> using it as a sideport display.
>

Yes, I'd like to know which applications these are.

Thanks,
jaya

Re: [PATCH] Add sysfs support for fbdefio delay

[Bruno Prémont]

On Fri, 26 February 2010 Jaya Kumar <jayakumar.lkml@gmail.com> wrote:
> On Fri, Feb 26, 2010 at 6:53 PM, Bruno Prémont wrote:
> > For me the driver would start with a default delay that matches the
> > full-redraw throughput of the device but userspace could reduce the
> > delay when it knows it will mostly just refresh small parts of the
> > display (one or two tiles) and would like those done at a higher
> > rate.
> 
> Who in userspace will know to reduce the delay? How will it know that
> the delay should be reduced?

The default (maybe even the largest) period would indicate what the
hardware can do in worst case and the smallest period what the hardware
can do in best/special cases (like single-tile update for PicoLCD)

Any application that wants to output information might want to know
how often the information can be displayed (what's the need to
calculate 1000 frames per second if only 2 of them will ever be seen
on display?). When possible and they know their display needs they
might benefit from ability to tune the result.

> > A sample application would be displaying a media player interface
> > like the one of XMMS and clones where Umeter (the part displaying
> > volume per frequency range) could be refreshed ten times a second,
> > the current position once a second and all the rest only on song
> > change.
> 
> xmms/umeter will talk to this sysfs entry?

Probably not XMMS itself but either a plugin for it or a human
interface to player that takes input from a remote control and
displays status to LCD (instead of/in addition to OSD)

> > Knowing the size of the display, probability that it's being used
> > directly by X server is very small, it would rather be some
> > application using it as a sideport display.
> >
> 
> Yes, I'd like to know which applications these are.

I don't have an example at hand. I would rather see plugins for media
players (like those that use LCD4Linux or LCDproc) benefit from such
a feature.

Thanks,
Bruno

Re: [PATCH] Add sysfs support for fbdefio delay

[Rick L. Vinyard, Jr.]

Jaya Kumar wrote:
> Hi Rick,
>
> On Fri, Feb 26, 2010 at 6:15 AM, Rick L. Vinyard Jr.
> <rvinyard@cs.nmsu.edu> wrote:
>> This patch adds support for examining and modifying the fbdefio delay
>> parameter through sysfs. It also adds two driver definable minimum
>> and maximum bounds.
>
> Thanks for posting this. I've now read through this patch and your
> past patches, but I couldn't get a good understanding of how this
> userspace exposed defio delay would interact with most existing
> systems.
>
>> +               Set the deferred I/O delay of the framebuffer in ms.
>> +               This value can be used to throttle deferred I/O updates.
>
> Please help us understand how userspace should use the ability to set
> this delay.

I think that there isn't one specific answer since, just like the approach
to priority of processes there isn't one specific answer. The real answer
is: it depends.

Bruno has already provided some feedback on his driver so I'll add a few
comments on what motivated it for the G13.

One of the factors I was concerned about was having two or more G13
devices on the same bus with other traffic. Each frame is sent as an
interrupt transfer out, and I was concerned that the framebuffer updates
might be detrimental to other traffic.

I know that a pair of G13 devices running at 30 FPS would only consume
about 5% of the USB 1.1 bandwidth (~500 Mbit/s) for framebuffer transfers
(sans headers and other control messages), but I was concerned that it
could introduce enough latency to make other devices laggy.

There is a bit of translation that has to go on for each framebuffer as
well. The G13 doesn't use the traditional bitwise layout where 0,0 is the
high bit of the first byte and 7,0 is the low bit of the first byte. The
format the G13 expects is 0,0 in the high bit and 0,7 in the low bit.

As a result of this translation, I was concerned with the driver consuming
too much CPU time when doing multiple translations for multiple devices...
or even one translation on a limited or saturated CPU at a high frame
rate.

Extending this beyond the simple case of the G13, I think it could be
useful for any deferred framebuffer that consumes a limited critical
resource, whether it be USB bandwidth, CPU usage, or some other resource.

One driver in particular that I noticed is the xen-fbfront. The update
rate is set at 20FPS, but what if someone wanted 30 FPS?

> Who in userspace is intended to use it? Would this be the
> X server, cairo, or papyrus? A separate daemon? Human interaction?

Again, that has the same issue as who sets priority on processes... It
depends.

As for the G13 I was planning, for the current time, on putting a slider
in the configuration window that allowed the user to modify the update
rate. The slider would populate off the sysfs values.

I think it would be an interesting possibility to develop a daemon that
watched traffic on buses with USB devices that could be throttled and
throttle them according to bus saturation.

However, at this point I wasn't planning on going there. But, having the
ability to throttle would be a key point for developing such daemons.

> When should said userspace entity throttle updates? How would this
> entity know when throttling is needed? Could this throttling possibly
> or should this possibly be done automatically by the driver itself?
>

I don't think it should be incorporated into the driver because I believe
most circumstances will involve factors outside the purview of the driver,
such as bandwidth utilization. One factor could be rather subjective, like
'laggy'. What is 'laggy' to one user may not be to another.

>> +               Most framebuffer devices do not have or need support for
>> +               deferred I/O. Accessing a framebuffer without deferred
>> I/O
>> +               support will return -ENODEV. Can be read but not
>> modified if
>> +               /sys/class/graphics/<fb>/defio_delay_max is 0. When
>> modifying,
>> +               the value must be greater than or equal to
>> +               /sys/class/graphics/<fb>/defio_delay_min and less than
>> or equal
>> +               to /sys/class/graphics/<fb>/defio_delay_max.
>
> Please help the reader of above understand the sequence of userspace
> changing the defio delay, and how and when that would affect the
> associated drivers. Will the delay behaviour be standard for all defio
> client drivers? What happens in all the various timing scenarios, eg:
> if a defio delay is changed in the middle of a display update or a
> defio page clean or before?

I think this could be taken care of by explaining that the parameter will
only effect the next queued framebuffer update. But, it should definitely
be in there.

> The sysfs parameter description mentioning
> min/max above could use some elaboration as reading it doesn't make
> clear if userspace can also affect min/max or if it is completely
> owned by the driver.
>

I can add that as well.

> Thanks,
> jaya
>
> ps: I only found your patches by accident when reading through the
> fbdev mailing list. I'm very interested in defio (I happen to be the
> author of it) and also drivers that use defio so I would appreciate
> being CCed on such changes. Thanks a bunch.
>

No problem. I didn't mean to leave you out.

-- 

Rick

Re: [PATCH] Add sysfs support for fbdefio delay

[Jaya Kumar]

On Tue, Mar 2, 2010 at 12:10 AM, Rick L. Vinyard, Jr.
<rvinyard@cs.nmsu.edu> wrote:
> Jaya Kumar wrote:
>> Hi Rick,
>>
>> On Fri, Feb 26, 2010 at 6:15 AM, Rick L. Vinyard Jr.
>> <rvinyard@cs.nmsu.edu> wrote:
>>
>>> +               Set the deferred I/O delay of the framebuffer in ms.
>>> +               This value can be used to throttle deferred I/O updates.
>>
>> Please help us understand how userspace should use the ability to set
>> this delay.
>
> I think that there isn't one specific answer since, just like the approach
> to priority of processes there isn't one specific answer. The real answer
> is: it depends.

Rick,

Thanks for your reply.

>
> One of the factors I was concerned about was having two or more G13
> devices on the same bus with other traffic. Each frame is sent as an
> interrupt transfer out, and I was concerned that the framebuffer updates
> might be detrimental to other traffic.
>
> I know that a pair of G13 devices running at 30 FPS would only consume
> about 5% of the USB 1.1 bandwidth (~500 Mbit/s) for framebuffer transfers
> (sans headers and other control messages), but I was concerned that it
> could introduce enough latency to make other devices laggy.

You mentioned in your responses and above that you're concerned about
saturating the USB bus and lagginess, and I think it is good to have
those concerns. Defio delay may be an easy parameter to expose, and in
at least a specific set of drivers, changing it would have a
meaningful effect on bus utilization, but are you convinced that
exposing it generally (remember your patch makes it apply to all fb
drivers) to userspace is a right way or even a good way to address bus
saturation concerns?

> As a result of this translation, I was concerned with the driver consuming
> too much CPU time when doing multiple translations for multiple devices...
> or even one translation on a limited or saturated CPU at a high frame
> rate.

Being concerned about CPU utilization is a good thing. But say for
example, your USB ethernet driver or USB audio driver is taking a lot
of cpu time packetizing traffic, then would I be correct that your
desire is to expose an inter-packetization driver specific sleep time
controllable by userspace via sysfs for all ethernet, audio, etc
drivers? (by driver specific, I mean the sysfs parameter would be
presented by all drivers but the value would be specific to each one,
which is the way your current patch would behave for all fb drivers).

> I think it would be an interesting possibility to develop a daemon that
> watched traffic on buses with USB devices that could be throttled and
> throttle them according to bus saturation.
>
> However, at this point I wasn't planning on going there. But, having the
> ability to throttle would be a key point for developing such daemons.

Just curious, do such sysfs throttling parameters exist for any other
subsystems? If so, are userspace apps/services using them, and are
they happy with it?

Thanks,
jaya

Re: [PATCH] Add sysfs support for fbdefio delay

[Rick L. Vinyard, Jr.]

Jaya Kumar wrote:
> On Tue, Mar 2, 2010 at 12:10 AM, Rick L. Vinyard, Jr.
> <rvinyard@cs.nmsu.edu> wrote:
>> Jaya Kumar wrote:
>>> Hi Rick,
>>>
>>> On Fri, Feb 26, 2010 at 6:15 AM, Rick L. Vinyard Jr.
>>> <rvinyard@cs.nmsu.edu> wrote:
>>>
>>>> +               Set the deferred I/O delay of the framebuffer in ms.
>>>> +               This value can be used to throttle deferred I/O
>>>> updates.
>>>
>>> Please help us understand how userspace should use the ability to set
>>> this delay.
>>
>> I think that there isn't one specific answer since, just like the
>> approach
>> to priority of processes there isn't one specific answer. The real
>> answer
>> is: it depends.
>
> Rick,
>
> Thanks for your reply.
>
>>
>> One of the factors I was concerned about was having two or more G13
>> devices on the same bus with other traffic. Each frame is sent as an
>> interrupt transfer out, and I was concerned that the framebuffer updates
>> might be detrimental to other traffic.
>>
>> I know that a pair of G13 devices running at 30 FPS would only consume
>> about 5% of the USB 1.1 bandwidth (~500 Mbit/s) for framebuffer
>> transfers
>> (sans headers and other control messages), but I was concerned that it
>> could introduce enough latency to make other devices laggy.
>
> You mentioned in your responses and above that you're concerned about
> saturating the USB bus and lagginess, and I think it is good to have
> those concerns. Defio delay may be an easy parameter to expose, and in
> at least a specific set of drivers, changing it would have a
> meaningful effect on bus utilization, but are you convinced that
> exposing it generally (remember your patch makes it apply to all fb
> drivers) to userspace is a right way or even a good way to address bus
> saturation concerns?
>
>> As a result of this translation, I was concerned with the driver
>> consuming
>> too much CPU time when doing multiple translations for multiple
>> devices...
>> or even one translation on a limited or saturated CPU at a high frame
>> rate.
>
> Being concerned about CPU utilization is a good thing. But say for
> example, your USB ethernet driver or USB audio driver is taking a lot
> of cpu time packetizing traffic, then would I be correct that your
> desire is to expose an inter-packetization driver specific sleep time
> controllable by userspace via sysfs for all ethernet, audio, etc
> drivers? (by driver specific, I mean the sysfs parameter would be
> presented by all drivers but the value would be specific to each one,
> which is the way your current patch would behave for all fb drivers).
>

I think the answer is yes, whether this were a fb, network, audio, etc. If
there is a clearly defined parameter at which resource utilization can be
adjusted in a standard way.

Although the value is specific to each one, the min/max parameters provide
the bounds and standardized units, allowing userspace to modify the delay
parameter in a standardized way.

I also tried to make it where the driver had to explicitly modify the
min/max parameters so that unless a driver sets min/max values the delay
parameter can be examined from userspace but not modified.

I agree that the motivation for throttling varies by device, and even as
in this case there could be different motivations for the same device (USB
bus bandwidth and/or CPU utilization).

I think that what also distinguishes the fbdefio case is the fact that
there is a single defined point at which resource utilization would be
throttled and that's the fb_deferred_io structure's deferred_io hook.

>> I think it would be an interesting possibility to develop a daemon that
>> watched traffic on buses with USB devices that could be throttled and
>> throttle them according to bus saturation.
>>
>> However, at this point I wasn't planning on going there. But, having the
>> ability to throttle would be a key point for developing such daemons.
>
> Just curious, do such sysfs throttling parameters exist for any other
> subsystems? If so, are userspace apps/services using them, and are
> they happy with it?

An example of something like this is the ACPI CPU thermal class. It
provides two parameters, cur_state and max_state. The minimum is presumed
to be 0.

In this case though, I think fbdefio needs to provide both a min and a max
as a driver may want to set the minimum refresh rate to be non-zero.

Another example would be the CPU frequency subsystem, and in particular
the userspace governor. There are similar sysfs attributes for obtaining
the current frequency, setting the frequency, and obtaining the min/max
frequencies.

In particular they used:
     scaling_cur_freq (ro)
     scaling_min_freq (ro)
     scaling_max_freq (ro)
     scaling_setspeed (wo)

I used a single read/write attribute for read/write on the delay instead
of the two parameter approach used for frequencies. I used:
     defio_delay     (rw)
     defio_delay_min (ro)
     defio_delay_max (ro)

But, it wouldn't be hard to use an analogous set like:
     defio_cur_delay (ro)
     defio_min_delay (ro)
     defio_max_delay (ro)
     defio_set_delay (wo)

-- 

Rick

[PATCH] Add sysfs support for fbdefio delay

[Rick L. Vinyard Jr.]

This patch adds support for examining and modifying the fbdefio delay
parameter through sysfs. It also adds two driver definable minimum
and maximum bounds.

The default behavior is to not permit modifications if delay_max is 0,
thus preventing modification of the delay if the driver does not
explicitly permit modification.

Signed-off-by: Rick L. Vinyard, Jr <rvinyard@cs.nmsu.edu>
---
 .../ABI/testing/sysfs-class-graphics-defio         |   35 ++++++++
 drivers/video/fbsysfs.c                            |   87 ++++++++++++++++++++
 include/linux/fb.h                                 |    9 ++-
 3 files changed, 130 insertions(+), 1 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-graphics-defio

diff --git a/Documentation/ABI/testing/sysfs-class-graphics-defio b/Documentation/ABI/testing/sysfs-class-graphics-defio
new file mode 100644
index 0000000..e0ef924
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-graphics-defio
@@ -0,0 +1,35 @@
+What:		/sys/class/graphics/<fb>/defio_delay
+Date:		February 2010
+KernelVersion:	2.6.34
+Contact:	Rick L Vinyard Jr <rvinyard@cs.nmsu.edu>
+Description:
+		Set the deferred I/O delay of the framebuffer in ms.
+		This value can be used to throttle deferred I/O updates.
+		Most framebuffer devices do not have or need support for
+		deferred I/O. Accessing a framebuffer without deferred I/O
+		support will return -ENODEV. Can be read but not modified if
+		/sys/class/graphics/<fb>/defio_delay_max is 0. When modifying,
+		the value must be greater than or equal to
+		/sys/class/graphics/<fb>/defio_delay_min and less than or equal
+		to /sys/class/graphics/<fb>/defio_delay_max.
+
+What:		/sys/class/graphics/<fb>/defio_delay_min
+Date:		February 2010
+KernelVersion:	2.6.34
+Contact:	Rick L Vinyard Jr <rvinyard@cs.nmsu.edu>
+Description:
+		Minimum deferred I/O value in ms for this framebuffer.
+		This value is specified by the driver and cannot be modified
+		from sysfs. Default is 0.
+
+What:		/sys/class/graphics/<fb>/defio_delay_min
+Date:		February 2010
+KernelVersion:	2.6.34
+Contact:	Rick L Vinyard Jr <rvinyard@cs.nmsu.edu>
+Description:
+		Maximum deferred I/O value in ms for this framebuffer.
+		This value is specified by the driver and cannot be modified
+		from sysfs. Default is 0.
+		If this value is 0 /sys/class/graphics/<fb>/defio_delay cannot
+		be modified, but can be read.
+
diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
index d4a2c11..d00ea2d 100644
--- a/drivers/video/fbsysfs.c
+++ b/drivers/video/fbsysfs.c
@@ -484,6 +484,87 @@ static ssize_t show_bl_curve(struct device *device,
 }
 #endif
 
+#ifdef CONFIG_FB_DEFERRED_IO
+static ssize_t store_defio_delay(struct device *device,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct fb_info *fb_info = dev_get_drvdata(device);
+	unsigned long delay_ms = 0;
+	unsigned long delay;
+	int error;
+	char *last = NULL;
+
+	/* Check to see whether this is a deferred I/O driver */
+	if (!fb_info || !fb_info->fbdefio)
+		return -ENODEV;
+
+	/* Check whether delay_max permits setting of delay */
+	if (fb_info->fbdefio->delay_max = 0)
+		return -EPERM;
+
+	error = strict_strtoul(buf, 10, &delay_ms);
+	if (error < 0)
+		return error;
+
+	delay = delay_ms * HZ / 1000;
+
+	if (delay < fb_info->fbdefio->delay_min ||
+	    delay > fb_info->fbdefio->delay_max)
+		return -EINVAL;
+
+	fb_info->fbdefio->delay = delay;
+
+	return count;
+}
+
+static ssize_t show_defio_delay(struct device *device,
+				struct device_attribute *attr, char *buf)
+{
+	struct fb_info *fb_info = dev_get_drvdata(device);
+	unsigned long delay_ms;
+
+	/* Check to see whether this is a deferred I/O driver */
+	if (!fb_info || !fb_info->fbdefio)
+		return -ENODEV;
+
+	delay_ms = fb_info->fbdefio->delay * 1000 / HZ;
+
+	return snprintf(buf, PAGE_SIZE, "%lu\n", delay_ms);
+}
+
+static ssize_t show_defio_delay_min(struct device *device,
+				    struct device_attribute *attr, char *buf)
+{
+	struct fb_info *fb_info = dev_get_drvdata(device);
+	unsigned long delay_ms;
+
+	/* Check to see whether this is a deferred I/O driver */
+	if (!fb_info || !fb_info->fbdefio)
+		return -ENODEV;
+
+	delay_ms = fb_info->fbdefio->delay_min * 1000 / HZ;
+
+	return snprintf(buf, PAGE_SIZE, "%lu\n", delay_ms);
+}
+
+static ssize_t show_defio_delay_max(struct device *device,
+				    struct device_attribute *attr, char *buf)
+{
+	struct fb_info *fb_info = dev_get_drvdata(device);
+	unsigned long delay_ms;
+
+	/* Check to see whether this is a deferred I/O driver */
+	if (!fb_info || !fb_info->fbdefio)
+		return -ENODEV;
+
+	delay_ms = fb_info->fbdefio->delay_max * 1000 / HZ;
+
+	return snprintf(buf, PAGE_SIZE, "%lu\n", delay_ms);
+}
+
+#endif
+
 /* When cmap is added back in it should be a binary attribute
  * not a text one. Consideration should also be given to converting
  * fbdev to use configfs instead of sysfs */
@@ -503,6 +584,12 @@ static struct device_attribute device_attrs[] = {
 #ifdef CONFIG_FB_BACKLIGHT
 	__ATTR(bl_curve, S_IRUGO|S_IWUSR, show_bl_curve, store_bl_curve),
 #endif
+#ifdef CONFIG_FB_DEFERRED_IO
+	__ATTR(defio_delay, S_IRUGO|S_IWUSR,
+	       show_defio_delay, store_defio_delay),
+	__ATTR(defio_delay_min, S_IRUGO, show_defio_delay_min, NULL),
+	__ATTR(defio_delay_max, S_IRUGO, show_defio_delay_max, NULL),
+#endif
 };
 
 int fb_init_device(struct fb_info *fb_info)
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 369767b..76f35fd 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -591,8 +591,15 @@ struct fb_pixmap {
 
 #ifdef CONFIG_FB_DEFERRED_IO
 struct fb_deferred_io {
-	/* delay between mkwrite and deferred handler */
+	/* delay in jiffies between mkwrite and deferred handler */
 	unsigned long delay;
+	/* The minimum delay in jiffies that may be set through sysfs */
+	unsigned long delay_min;
+	/*
+	 * The maximum delay in jiffies that may be set through sysfs.
+	 * If delay_max is 0, delay cannot be set through sysfs.
+	 */
+	unsigned long delay_max;
 	struct mutex lock; /* mutex that protects the page list */
 	struct list_head pagelist; /* list of touched pages */
 	/* callback */
-- 
1.6.6.1

Re: [PATCH] Add sysfs support for fbdefio delay

[Jaya Kumar]

On Tue, Mar 2, 2010 at 11:36 PM, Rick L. Vinyard, Jr.
<rvinyard@cs.nmsu.edu> wrote:
>
> Jaya Kumar wrote:
>> On Tue, Mar 2, 2010 at 12:10 AM, Rick L. Vinyard, Jr.
>> <rvinyard@cs.nmsu.edu> wrote:
>>> Jaya Kumar wrote:
>>
>> Being concerned about CPU utilization is a good thing. But say for
>> example, your USB ethernet driver or USB audio driver is taking a lot
>> of cpu time packetizing traffic, then would I be correct that your
>> desire is to expose an inter-packetization driver specific sleep time
>> controllable by userspace via sysfs for all ethernet, audio, etc
>> drivers? (by driver specific, I mean the sysfs parameter would be
>> presented by all drivers but the value would be specific to each one,
>> which is the way your current patch would behave for all fb drivers).
>>
>
> I think the answer is yes, whether this were a fb, network, audio, etc. If
> there is a clearly defined parameter at which resource utilization can be
> adjusted in a standard way.

Hi Rick,

Sure, we can find these driver resource utilization choke points, and
maybe even make it sort of standard, but that does not mean that we
should expose all of this to userspace. Adding more userspace tunables
for each driver in order to effect fb, network, audio bus utilization,
is not appealing. So I think we disagree about the fundamentals.

My conclusion is: I'm opposed this patch, because it exposes the defio
delay parameter for _all_ fb drivers, _each_ through a
/sys/class/graphics/fb_driver_name/defio_delay sysfs entry and also
adds a min/max issue. The behaviour and system impact seen by
userspace when changing the parameter is not standard across the
typical range of systems and devices. More importantly, exposing each
driver's defio delay to userspace is not a good way to address the 2
underlying functional goals that were raised during this discussion:
1) being able to control a driver's bus/cpu utilization, and 2)
allowing certain plugins/etc to be able to increase display update
frequency for subregions of the display.

Just to be verbose, please don't let my rejection of this specific
patch affect your other patches as I see those as being very useful
and close to being suitable for merging.

Thanks,
jaya

Re: [PATCH] Add sysfs support for fbdefio delay

[Andrew Morton]

On Tue, 02 Mar 2010 11:47:34 -0700
"Rick L. Vinyard Jr." <rvinyard@cs.nmsu.edu> wrote:

> This patch adds support for examining and modifying the fbdefio delay
> parameter through sysfs. It also adds two driver definable minimum
> and maximum bounds.
> 
> The default behavior is to not permit modifications if delay_max is 0,
> thus preventing modification of the delay if the driver does not
> explicitly permit modification.

Why is this being added?  You have some device which doesn't function
properly without this change?

If so, that's pretty critical changelog info - we presently have no
reason to merge the patch!

Thanks.

Re: [PATCH] Add sysfs support for fbdefio delay

[Rick L. Vinyard, Jr.]

Jaya Kumar wrote:
> On Tue, Mar 2, 2010 at 11:36 PM, Rick L. Vinyard, Jr.
> <rvinyard@cs.nmsu.edu> wrote:
>>
>> Jaya Kumar wrote:
>>> On Tue, Mar 2, 2010 at 12:10 AM, Rick L. Vinyard, Jr.
>>> <rvinyard@cs.nmsu.edu> wrote:
>>>> Jaya Kumar wrote:
>>>
>>> Being concerned about CPU utilization is a good thing. But say for
>>> example, your USB ethernet driver or USB audio driver is taking a lot
>>> of cpu time packetizing traffic, then would I be correct that your
>>> desire is to expose an inter-packetization driver specific sleep time
>>> controllable by userspace via sysfs for all ethernet, audio, etc
>>> drivers? (by driver specific, I mean the sysfs parameter would be
>>> presented by all drivers but the value would be specific to each one,
>>> which is the way your current patch would behave for all fb drivers).
>>>
>>
>> I think the answer is yes, whether this were a fb, network, audio, etc.
>> If
>> there is a clearly defined parameter at which resource utilization can
>> be
>> adjusted in a standard way.
>
> Hi Rick,
>
> Sure, we can find these driver resource utilization choke points, and
> maybe even make it sort of standard, but that does not mean that we
> should expose all of this to userspace. Adding more userspace tunables
> for each driver in order to effect fb, network, audio bus utilization,
> is not appealing. So I think we disagree about the fundamentals.
>
> My conclusion is: I'm opposed this patch, because it exposes the defio
> delay parameter for _all_ fb drivers, _each_ through a
> /sys/class/graphics/fb_driver_name/defio_delay sysfs entry and also
> adds a min/max issue. The behaviour and system impact seen by
> userspace when changing the parameter is not standard across the
> typical range of systems and devices. More importantly, exposing each
> driver's defio delay to userspace is not a good way to address the 2
> underlying functional goals that were raised during this discussion:
> 1) being able to control a driver's bus/cpu utilization, and 2)
> allowing certain plugins/etc to be able to increase display update
> frequency for subregions of the display.
>
> Just to be verbose, please don't let my rejection of this specific
> patch affect your other patches as I see those as being very useful
> and close to being suitable for merging.
>

No problem. I understand the desire to be conservative.