diff mbox

BACKLIGHT property for KMS case

Message ID 20090817140745.GA32497@suse.de (mailing list archive)
State Not Applicable
Headers show

Commit Message

Matthias Hopf Aug. 17, 2009, 2:07 p.m. UTC
The following patch adds BACKLIGHT property support in the intel driver
when KMS is active. It only tries to access the backlight via the kernel
based method (/sys/class/backlight/*), as we don't have access to mmio
space, and potentially also not to pci config space. Thus these paths
are hardcoded (compared to the original implementation).

Currently, this duplicates some similar code (from i830_lvds.c) - I can
abstract that into a new module with according data structures, but am
not 100% convinced that this is actually useful - i830_lvds.c will e.g.
always need the additional access methods.

Be also aware that the patch bears testing. So far it's only compile
tested, but it's similar to the quick-hack patch I did before (which
worked ;)

Comments?

Matthias

Comments

Greg KH Aug. 17, 2009, 4:17 p.m. UTC | #1
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
Matthias Hopf Aug. 17, 2009, 4:52 p.m. UTC | #2
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
Greg KH Aug. 17, 2009, 5:09 p.m. UTC | #3
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
Matthias Hopf Aug. 17, 2009, 5:32 p.m. UTC | #4
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
Matthew Garrett Aug. 17, 2009, 8:32 p.m. UTC | #5
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.
Keith Packard Aug. 17, 2009, 9:32 p.m. UTC | #6
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.
Greg KH Aug. 17, 2009, 10:12 p.m. UTC | #7
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
diff mbox

Patch

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