Message ID | 20090817140745.GA32497@suse.de (mailing list archive) |
---|---|
State | Not Applicable |
Headers | show |
On Mon, Aug 17, 2009 at 04:07:45PM +0200, Matthias Hopf wrote: > +#define BACKLIGHT_CLASS "/sys/class/backlight" Why can't we use libudev to get this kind of device instead of hard-coding it? > + > +/* > + * List of available kernel interfaces in priority order > + */ > +static char *backlight_interfaces[] = { > + "asus-laptop", > + "eeepc", > + "thinkpad_screen", > + "acpi_video1", > + "acpi_video0", > + "fujitsu-laptop", > + "sony", > + NULL, > +}; You forgot to add "samsung" :) Yeah, I know you want to do this instead of using the samsung-backlight driver, but what about for instances that people are not running this intel driver and they want control over the backlight of their hardware? That would mean that the samsung-backlight driver is needed for them. thanks, greg k-h
On Aug 17, 09 09:17:02 -0700, Greg KH wrote: > On Mon, Aug 17, 2009 at 04:07:45PM +0200, Matthias Hopf wrote: > > +#define BACKLIGHT_CLASS "/sys/class/backlight" > > Why can't we use libudev to get this kind of device instead of > hard-coding it? This is just copy and paste from the original code. Typically libudev is frowned upon a bit in X due to drivers being compilable on *BSD and other OSes, and I'm unsure about the state there (is udev used there? is the kernel interface different? do they support KMS already at all?). From a (very) quick glance at the reference manual I don't see how I can get the device name from libudev - I guess I need some help from you there. > > +/* > > + * List of available kernel interfaces in priority order > > + */ > > +static char *backlight_interfaces[] = { > > + "asus-laptop", > > + "eeepc", > > + "thinkpad_screen", > > + "acpi_video1", > > + "acpi_video0", > > + "fujitsu-laptop", > > + "sony", > > + NULL, > > +}; > > You forgot to add "samsung" :) This is the old list. Expansion should be done as a separate commit. Also, as written multiple times I think your driver shouldn't be called samsung but i915 or i830 or the like (according to the X11 driver the method should work for i830 and up, but native access method should be implemented as well). > Yeah, I know you want to do this instead of using the samsung-backlight No, I don't. I'd love to have a kernel driver, this patch is *only* about the RandR property. In fact, this patch will *not* enable backlight control on the Samsung NC130 without your driver (and as the driver name isn't included yet, also not yet with your driver ;-) I tested on a different machine that has a working ACPI backlight interface. > driver, but what about for instances that people are not running this > intel driver and they want control over the backlight of their hardware? > That would mean that the samsung-backlight driver is needed for them. The control method you implemented is AFAICS intel chipset specific. It is definitely not Samsung specific, and I very much assume that there are other laptops in the wild with broken ACPI interface that would benefit from the driver. I, personally, don't care whether this is implemented in the DRM driver or in a separate driver. I can imagine that it is easier to implement in DRM because there's some infrastructure to analyse the chipset, but that's about it. CU Matthias
On Mon, Aug 17, 2009 at 06:52:37PM +0200, Matthias Hopf wrote: > On Aug 17, 09 09:17:02 -0700, Greg KH wrote: > > On Mon, Aug 17, 2009 at 04:07:45PM +0200, Matthias Hopf wrote: > > > +#define BACKLIGHT_CLASS "/sys/class/backlight" > > > > Why can't we use libudev to get this kind of device instead of > > hard-coding it? > > This is just copy and paste from the original code. Typically libudev > is frowned upon a bit in X due to drivers being compilable on *BSD and > other OSes, and I'm unsure about the state there (is udev used there? is > the kernel interface different? do they support KMS already at all?). We still care about other OSes? :) I really doubt that any other operating system has /sys/class/backlight/ so I don't see how this is any different. And as HAL is depreciated for libudev, I thought that was the way forward for xorg. > From a (very) quick glance at the reference manual I don't see how I can > get the device name from libudev - I guess I need some help from you > there. You iterate over the class device names for a specific class, I can knock up some example code if you want it. > > > +/* > > > + * List of available kernel interfaces in priority order > > > + */ > > > +static char *backlight_interfaces[] = { > > > + "asus-laptop", > > > + "eeepc", > > > + "thinkpad_screen", > > > + "acpi_video1", > > > + "acpi_video0", > > > + "fujitsu-laptop", > > > + "sony", > > > + NULL, > > > +}; > > > > You forgot to add "samsung" :) > > This is the old list. Expansion should be done as a separate commit. > > Also, as written multiple times I think your driver shouldn't be called > samsung but i915 or i830 or the like (according to the X11 driver the > method should work for i830 and up, but native access method should be > implemented as well). So you want 2 different drivers controlling the same hardware? That's a recipe for disaster :) I don't care what to call this driver, but I think calling it i915 might cause big issues as you can't have 2 of the same driver name in Linux... "intel-backlight" perhaps? > > Yeah, I know you want to do this instead of using the samsung-backlight > > No, I don't. I'd love to have a kernel driver, this patch is *only* > about the RandR property. In fact, this patch will *not* enable > backlight control on the Samsung NC130 without your driver (and as the > driver name isn't included yet, also not yet with your driver ;-) > > I tested on a different machine that has a working ACPI backlight > interface. > > > driver, but what about for instances that people are not running this > > intel driver and they want control over the backlight of their hardware? > > That would mean that the samsung-backlight driver is needed for them. > > The control method you implemented is AFAICS intel chipset specific. It > is definitely not Samsung specific, and I very much assume that there > are other laptops in the wild with broken ACPI interface that would > benefit from the driver. I have not heard of any yet, but would not rule it out. thanks, greg k-h
On Aug 17, 09 10:09:09 -0700, Greg KH wrote: > We still care about other OSes? :) Well - I do. Even if I only used Linux so far ;) > And as HAL is depreciated for libudev, I thought that was the way > forward for xorg. I, honestly, have no clue in this area. maybe you are right. > > From a (very) quick glance at the reference manual I don't see how I can > > get the device name from libudev - I guess I need some help from you > > there. > > You iterate over the class device names for a specific class, I can > knock up some example code if you want it. That doesn't give you any priorities. However, it's probably better to *only* sort the acpi drivers to the front. > So you want 2 different drivers controlling the same hardware? That's a > recipe for disaster :) Hm. Right. Idea not good. > I don't care what to call this driver, but I think calling it i915 might > cause big issues as you can't have 2 of the same driver name in Linux... > > "intel-backlight" perhaps? Yep. Sounds good. > > The control method you implemented is AFAICS intel chipset specific. It > > is definitely not Samsung specific, and I very much assume that there > > are other laptops in the wild with broken ACPI interface that would > > benefit from the driver. > > I have not heard of any yet, but would not rule it out. Thanks Matthias
On Mon, Aug 17, 2009 at 10:09:09AM -0700, Greg KH wrote: > On Mon, Aug 17, 2009 at 06:52:37PM +0200, Matthias Hopf wrote: > So you want 2 different drivers controlling the same hardware? That's a > recipe for disaster :) samsung-laptop (in its current form) is controlling the same hardware that i915 is already. Implementing it properly involves parsing the BIOS tables to tell you the minimum acceptable value (driving the backlight controller outside its specced range probably isn't a good idea) and potentially also touching the MMIO backlight registers. I'd really recommend not merging this as a separate driver. It's slightly more work to do it in the drm driver, but it's also the only way to do it properly.
On Mon, 2009-08-17 at 09:17 -0700, Greg KH wrote: > On Mon, Aug 17, 2009 at 04:07:45PM +0200, Matthias Hopf wrote: > > +#define BACKLIGHT_CLASS "/sys/class/backlight" > > Why can't we use libudev to get this kind of device instead of > hard-coding it? The X server currently uses hal for input device hot-plug; while we'd love to move that to libudev, we don't have any configuration infrastructure available in that environment, and so I suspect the core server will grow some libudev infrastructure in the near future. Anything we do with either hal or libudev is likely to be invalidated by this plan, so I'd suggest doing the easy thing for now and fixing it later when real libudev support appears.
On Mon, Aug 17, 2009 at 02:32:43PM -0700, Keith Packard wrote: > On Mon, 2009-08-17 at 09:17 -0700, Greg KH wrote: > > On Mon, Aug 17, 2009 at 04:07:45PM +0200, Matthias Hopf wrote: > > > +#define BACKLIGHT_CLASS "/sys/class/backlight" > > > > Why can't we use libudev to get this kind of device instead of > > hard-coding it? > > The X server currently uses hal for input device hot-plug; while we'd > love to move that to libudev, we don't have any configuration > infrastructure available in that environment, and so I suspect the core > server will grow some libudev infrastructure in the near future. > Anything we do with either hal or libudev is likely to be invalidated by > this plan, so I'd suggest doing the easy thing for now and fixing it > later when real libudev support appears. The easiest is to do it all in a different driver, but I don't think that is what others want :) thanks, greg k-h
From dedab3ddb3722dd7f6aeae782ed535b0e8e7e268 Mon Sep 17 00:00:00 2001 From: Matthias Hopf <mhopf@suse.de> Date: Mon, 17 Aug 2009 15:53:15 +0200 Subject: [PATCH] Add BACKLIGHT property support in KMS case. --- src/drmmode_display.c | 226 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 226 insertions(+), 0 deletions(-) diff --git a/src/drmmode_display.c b/src/drmmode_display.c index 814743b..9c87491 100644 --- a/src/drmmode_display.c +++ b/src/drmmode_display.c @@ -29,6 +29,10 @@ #include "config.h" #endif +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h> #include <errno.h> #include "xorgVersion.h" @@ -74,11 +78,156 @@ typedef struct { drmmode_prop_ptr props; void *private_data; int dpms_mode; + char *backlight_iface; + int backlight_active_level; + int backlight_max; } drmmode_output_private_rec, *drmmode_output_private_ptr; static void drmmode_output_dpms(xf86OutputPtr output, int mode); +#define BACKLIGHT_CLASS "/sys/class/backlight" + +/* + * List of available kernel interfaces in priority order + */ +static char *backlight_interfaces[] = { + "asus-laptop", + "eeepc", + "thinkpad_screen", + "acpi_video1", + "acpi_video0", + "fujitsu-laptop", + "sony", + NULL, +}; +/* + * Must be long enough for BACKLIGHT_CLASS + '/' + longest in above table + + * '/' + "max_backlight" + */ +#define BACKLIGHT_PATH_LEN 80 +/* Enough for 10 digits of backlight + '\n' + '\0' */ +#define BACKLIGHT_VALUE_LEN 12 + +static void +drmmode_backlight_set(xf86OutputPtr output, int level) +{ + drmmode_output_private_ptr drmmode_output = output->driver_private; + char path[BACKLIGHT_PATH_LEN], val[BACKLIGHT_VALUE_LEN]; + int fd, len, ret; + + if (level > drmmode_output->backlight_max) + level = drmmode_output->backlight_max; + if (! drmmode_output->backlight_iface || level < 0) + return; + + len = snprintf(val, BACKLIGHT_VALUE_LEN, "%d\n", level); + sprintf(path, "%s/%s/brightness", + BACKLIGHT_CLASS, drmmode_output->backlight_iface); + fd = open(path, O_RDWR); + if (fd == -1) { + xf86DrvMsg(output->scrn->scrnIndex, X_ERROR, "failed to open %s for backlight " + "control: %s\n", path, strerror(errno)); + return; + } + + ret = write(fd, val, len); + if (ret == -1) { + xf86DrvMsg(output->scrn->scrnIndex, X_ERROR, "write to %s for backlight " + "control failed: %s\n", path, strerror(errno)); + } + + close(fd); +} + +static int +drmmode_backlight_get(xf86OutputPtr output) +{ + drmmode_output_private_ptr drmmode_output = output->driver_private; + char path[BACKLIGHT_PATH_LEN], val[BACKLIGHT_VALUE_LEN]; + int fd, level; + + if (! drmmode_output->backlight_iface) + return -1; + + sprintf(path, "%s/%s/actual_brightness", + BACKLIGHT_CLASS, drmmode_output->backlight_iface); + fd = open(path, O_RDONLY); + if (fd == -1) { + xf86DrvMsg(output->scrn->scrnIndex, X_ERROR, "failed to open %s " + "for backlight control: %s\n", path, strerror(errno)); + return -1; + } + + memset(val, 0, sizeof(val)); + if (read(fd, val, BACKLIGHT_VALUE_LEN) == -1) { + close(fd); + return -1; + } + + close(fd); + + level = atoi(val); + if (level > drmmode_output->backlight_max) + level = drmmode_output->backlight_max; + if (level < 0) + level = -1; + return level; +} + +static int +drmmode_backlight_get_max(xf86OutputPtr output) +{ + drmmode_output_private_ptr drmmode_output = output->driver_private; + char path[BACKLIGHT_PATH_LEN], val[BACKLIGHT_VALUE_LEN]; + int fd, max = 0; + + sprintf(path, "%s/%s/max_brightness", + BACKLIGHT_CLASS, drmmode_output->backlight_iface); + fd = open(path, O_RDONLY); + if (fd == -1) { + xf86DrvMsg(output->scrn->scrnIndex, X_ERROR, "failed to open %s " + "for backlight control: %s\n", path, strerror(errno)); + return 0; + } + + memset(val, 0, sizeof(val)); + if (read(fd, val, BACKLIGHT_VALUE_LEN) == -1) { + close(fd); + return -1; + } + + close(fd); + + max = atoi(val); + if (max <= 0) + max = -1; + return max; +} + +static void +drmmode_backlight_init(xf86OutputPtr output) +{ + drmmode_output_private_ptr drmmode_output = output->driver_private; + char path[BACKLIGHT_PATH_LEN]; + struct stat buf; + int i; + + for (i = 0; backlight_interfaces[i] != NULL; i++) { + sprintf(path, "%s/%s", BACKLIGHT_CLASS, backlight_interfaces[i]); + if (!stat(path, &buf)) { + drmmode_output->backlight_iface = backlight_interfaces[i]; + xf86DrvMsg(output->scrn->scrnIndex, X_INFO, + "found backlight control interface %s\n", path); + drmmode_output->backlight_max = drmmode_backlight_get_max(output); + drmmode_output->backlight_active_level = drmmode_backlight_get(output); + return; + } + } + drmmode_output->backlight_iface = NULL; +} + + static void drmmode_ConvertFromKMode(ScrnInfoPtr scrn, drmModeModeInfoPtr kmode, @@ -708,11 +857,32 @@ drmmode_output_destroy(xf86OutputPtr output) xfree(drmmode_output->private_data); drmmode_output->private_data = NULL; } + if (drmmode_output->backlight_iface) + drmmode_backlight_set(output, drmmode_output->backlight_active_level); xfree(drmmode_output); output->driver_private = NULL; } static void +drmmode_output_dpms_backlight(xf86OutputPtr output, int oldmode, int mode) +{ + drmmode_output_private_ptr drmmode_output = output->driver_private; + + if (!drmmode_output->backlight_iface) + return; + + if (mode == DPMSModeOn) { + /* If we're going from off->on we may need to turn on the backlight. */ + drmmode_backlight_set(output, drmmode_output->backlight_active_level); + } else { + /* Only save the current backlight value if we're going from on to off. */ + if (oldmode == DPMSModeOn) + drmmode_output->backlight_active_level = drmmode_backlight_get(output); + drmmode_backlight_set(output, 0); + } +} + +static void drmmode_output_dpms(xf86OutputPtr output, int mode) { drmmode_output_private_ptr drmmode_output = output->driver_private; @@ -731,6 +901,9 @@ drmmode_output_dpms(xf86OutputPtr output, int mode) drmmode_output->output_id, props->prop_id, mode); + drmmode_output_dpms_backlight(output, + drmmode_output->dpms_mode, + mode); drmmode_output->dpms_mode = mode; drmModeFreeProperty(props); return; @@ -763,6 +936,9 @@ drmmode_property_ignore(drmModePropertyPtr prop) return FALSE; } +#define BACKLIGHT_NAME "BACKLIGHT" +static Atom backlight_atom; + static void drmmode_output_create_resources(xf86OutputPtr output) { @@ -847,6 +1023,35 @@ drmmode_output_create_resources(xf86OutputPtr output) } } } + + if (drmmode_output->backlight_iface) { + INT32 data, backlight_range[2]; + /* Set up the backlight property, which takes effect immediately + * and accepts values only within the backlight_range. + * + * FIXME: there is no get_property yet. + */ + backlight_atom = MakeAtom(BACKLIGHT_NAME, sizeof(BACKLIGHT_NAME) - 1, + TRUE); + + backlight_range[0] = 0; + backlight_range[1] = drmmode_output->backlight_max; + err = RRConfigureOutputProperty(output->randr_output, backlight_atom, + FALSE, TRUE, FALSE, 2, backlight_range); + if (err != 0) { + xf86DrvMsg(output->scrn->scrnIndex, X_ERROR, + "RRConfigureOutputProperty error, %d\n", err); + } + /* Set the current value of the backlight property */ + data = drmmode_output->backlight_active_level; + err = RRChangeOutputProperty(output->randr_output, backlight_atom, + XA_INTEGER, 32, PropModeReplace, 1, &data, + FALSE, TRUE); + if (err != 0) { + xf86DrvMsg(output->scrn->scrnIndex, X_ERROR, + "RRChangeOutputProperty error, %d\n", err); + } + } } static Bool @@ -857,6 +1062,26 @@ drmmode_output_set_property(xf86OutputPtr output, Atom property, drmmode_ptr drmmode = drmmode_output->drmmode; int i; + if (property == backlight_atom) { + INT32 val; + + if (value->type != XA_INTEGER || value->format != 32 || + value->size != 1) + { + return FALSE; + } + + val = *(INT32 *)value->data; + if (val < 0 || val > drmmode_output->backlight_max) + return FALSE; + + if (val != drmmode_output->backlight_active_level) { + drmmode_backlight_set(output, val); + drmmode_output->backlight_active_level = val; + } + return TRUE; + } + for (i = 0; i < drmmode_output->num_props; i++) { drmmode_prop_ptr p = &drmmode_output->props[i]; @@ -992,6 +1217,7 @@ drmmode_output_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, int num) if (!drmmode_output->private_data) xf86DrvMsg(pScrn->scrnIndex, X_ERROR, "Can't allocate private memory for LVDS.\n"); + drmmode_backlight_init(output); } drmmode_output->output_id = drmmode->mode_res->connectors[num]; drmmode_output->mode_output = koutput; -- 1.6.0.2