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 <[email protected]>
---
.../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 <[email protected]>
+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 <[email protected]>
+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 <[email protected]>
+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
On Thu, 25 February 2010 "Rick L. Vinyard Jr." <[email protected]> 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
Bruno Prémont wrote:
> On Thu, 25 February 2010 "Rick L. Vinyard Jr." <[email protected]>
> 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
Hi Rick,
On Fri, Feb 26, 2010 at 6:15 AM, Rick L. Vinyard Jr.
<[email protected]> 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.
Hi Jaya,
On Fri, 26 February 2010 Jaya Kumar <[email protected]> wrote:
> On Fri, Feb 26, 2010 at 6:15 AM, Rick L. Vinyard Jr.
> <[email protected]> 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
On Fri, Feb 26, 2010 at 6:53 PM, Bruno Pr?mont
<[email protected]> 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
On Fri, 26 February 2010 Jaya Kumar <[email protected]> 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
Jaya Kumar wrote:
> On Tue, Mar 2, 2010 at 11:36 PM, Rick L. Vinyard, Jr.
> <[email protected]> wrote:
>>
>> Jaya Kumar wrote:
>>> On Tue, Mar 2, 2010 at 12:10 AM, Rick L. Vinyard, Jr.
>>> <[email protected]> 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.