diff mbox

Legacy backlight control with KMS and BACKLIGHT property

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

Commit Message

Matthias Hopf Aug. 14, 2009, 5:54 p.m. UTC
Currently with KMS legacy backlight control isn't working at all.
Also, the driver doesn't expose the BACKLIGHT RandR property in any
case, so xbacklight doesn't work with KMS.

I've been working on a patch as a quick-hack for this (attached; please
DON'T apply), and am finally understanding enough to actually implement
this correctly together with GregKH.


What we're supposing is

- a kernel driver for the /sys/class/backlight/ api for the legacy method
- support in the driver to actually open this one (trivial)
- support for the BACKLIGHT property in KMS case, *only* using the
  kernel api method for now (will need some abstraction)


Also I noted that the BACKLIGHT property is

- not preceeded with _
- not yet documented in randrproto.txt

I suggest that I add this in randrproto.txt, as it seems plausible to me
to actually make this a known property.


Does this sound reasonable?

Matthias

Comments

Jesse Barnes Aug. 14, 2009, 5:56 p.m. UTC | #1
On Fri, 14 Aug 2009 19:54:05 +0200
Matthias Hopf <mhopf@suse.de> wrote:

> Currently with KMS legacy backlight control isn't working at all.
> Also, the driver doesn't expose the BACKLIGHT RandR property in any
> case, so xbacklight doesn't work with KMS.
> 
> I've been working on a patch as a quick-hack for this (attached;
> please DON'T apply), and am finally understanding enough to actually
> implement this correctly together with GregKH.
> 
> 
> What we're supposing is
> 
> - a kernel driver for the /sys/class/backlight/ api for the legacy
> method
> - support in the driver to actually open this one (trivial)
> - support for the BACKLIGHT property in KMS case, *only* using the
>   kernel api method for now (will need some abstraction)

This sounds like the correct approach.  We have an RFE open on this too:
http://bugs.freedesktop.org/show_bug.cgi?id=20963

> Also I noted that the BACKLIGHT property is
> 
> - not preceeded with _
> - not yet documented in randrproto.txt
> 
> I suggest that I add this in randrproto.txt, as it seems plausible to
> me to actually make this a known property.
> 
> 
> Does this sound reasonable?

Yeah, sounds good to me.
Greg KH Aug. 14, 2009, 6:13 p.m. UTC | #2
On Fri, Aug 14, 2009 at 10:56:39AM -0700, Jesse Barnes wrote:
> On Fri, 14 Aug 2009 19:54:05 +0200
> Matthias Hopf <mhopf@suse.de> wrote:
> 
> > Currently with KMS legacy backlight control isn't working at all.
> > Also, the driver doesn't expose the BACKLIGHT RandR property in any
> > case, so xbacklight doesn't work with KMS.
> > 
> > I've been working on a patch as a quick-hack for this (attached;
> > please DON'T apply), and am finally understanding enough to actually
> > implement this correctly together with GregKH.
> > 
> > 
> > What we're supposing is
> > 
> > - a kernel driver for the /sys/class/backlight/ api for the legacy
> > method
> > - support in the driver to actually open this one (trivial)
> > - support for the BACKLIGHT property in KMS case, *only* using the
> >   kernel api method for now (will need some abstraction)
> 
> This sounds like the correct approach.  We have an RFE open on this too:
> http://bugs.freedesktop.org/show_bug.cgi?id=20963

My question is, does this belong in the kernel side, to expose the
backlight through /sys/class/backlight/, or do we do this on the
userspace side in perhaps the xorg intel driver (where we can touch
pci config space just as easily)?

What does KMS expect to have happen?

thanks,

greg k-h
Jesse Barnes Aug. 14, 2009, 6:20 p.m. UTC | #3
On Fri, 14 Aug 2009 11:13:28 -0700
Greg KH <greg@kroah.com> wrote:

> On Fri, Aug 14, 2009 at 10:56:39AM -0700, Jesse Barnes wrote:
> > On Fri, 14 Aug 2009 19:54:05 +0200
> > Matthias Hopf <mhopf@suse.de> wrote:
> > 
> > > Currently with KMS legacy backlight control isn't working at all.
> > > Also, the driver doesn't expose the BACKLIGHT RandR property in
> > > any case, so xbacklight doesn't work with KMS.
> > > 
> > > I've been working on a patch as a quick-hack for this (attached;
> > > please DON'T apply), and am finally understanding enough to
> > > actually implement this correctly together with GregKH.
> > > 
> > > 
> > > What we're supposing is
> > > 
> > > - a kernel driver for the /sys/class/backlight/ api for the legacy
> > > method
> > > - support in the driver to actually open this one (trivial)
> > > - support for the BACKLIGHT property in KMS case, *only* using the
> > >   kernel api method for now (will need some abstraction)
> > 
> > This sounds like the correct approach.  We have an RFE open on this
> > too: http://bugs.freedesktop.org/show_bug.cgi?id=20963
> 
> My question is, does this belong in the kernel side, to expose the
> backlight through /sys/class/backlight/, or do we do this on the
> userspace side in perhaps the xorg intel driver (where we can touch
> pci config space just as easily)?

We're trying to avoid any direct PCI access from userland these days.
I was hoping we could settle on the /sys/class/backlight interface.

> What does KMS expect to have happen?

KMS doesn't have a specific interface for this; apps like g-p-m
typically go through the server's xrandr interface or
access /sys/class/backlight directly.
Greg KH Aug. 14, 2009, 6:29 p.m. UTC | #4
On Fri, Aug 14, 2009 at 11:20:57AM -0700, Jesse Barnes wrote:
> On Fri, 14 Aug 2009 11:13:28 -0700
> Greg KH <greg@kroah.com> wrote:
> 
> > On Fri, Aug 14, 2009 at 10:56:39AM -0700, Jesse Barnes wrote:
> > > On Fri, 14 Aug 2009 19:54:05 +0200
> > > Matthias Hopf <mhopf@suse.de> wrote:
> > > 
> > > > Currently with KMS legacy backlight control isn't working at all.
> > > > Also, the driver doesn't expose the BACKLIGHT RandR property in
> > > > any case, so xbacklight doesn't work with KMS.
> > > > 
> > > > I've been working on a patch as a quick-hack for this (attached;
> > > > please DON'T apply), and am finally understanding enough to
> > > > actually implement this correctly together with GregKH.
> > > > 
> > > > 
> > > > What we're supposing is
> > > > 
> > > > - a kernel driver for the /sys/class/backlight/ api for the legacy
> > > > method
> > > > - support in the driver to actually open this one (trivial)
> > > > - support for the BACKLIGHT property in KMS case, *only* using the
> > > >   kernel api method for now (will need some abstraction)
> > > 
> > > This sounds like the correct approach.  We have an RFE open on this
> > > too: http://bugs.freedesktop.org/show_bug.cgi?id=20963
> > 
> > My question is, does this belong in the kernel side, to expose the
> > backlight through /sys/class/backlight/, or do we do this on the
> > userspace side in perhaps the xorg intel driver (where we can touch
> > pci config space just as easily)?
> 
> We're trying to avoid any direct PCI access from userland these days.
> I was hoping we could settle on the /sys/class/backlight interface.

Ok, I'll go knock up a Samsung laptop driver that handles this properly
then.  Thanks for the information.

> > What does KMS expect to have happen?
> 
> KMS doesn't have a specific interface for this; apps like g-p-m
> typically go through the server's xrandr interface or
> access /sys/class/backlight directly.

Good, that makes sense.

thanks,

greg k-h
Greg KH Aug. 14, 2009, 7:08 p.m. UTC | #5
On Fri, Aug 14, 2009 at 11:29:39AM -0700, Greg KH wrote:
> On Fri, Aug 14, 2009 at 11:20:57AM -0700, Jesse Barnes wrote:
> > On Fri, 14 Aug 2009 11:13:28 -0700
> > Greg KH <greg@kroah.com> wrote:
> > 
> > > On Fri, Aug 14, 2009 at 10:56:39AM -0700, Jesse Barnes wrote:
> > > > On Fri, 14 Aug 2009 19:54:05 +0200
> > > > Matthias Hopf <mhopf@suse.de> wrote:
> > > > 
> > > > > Currently with KMS legacy backlight control isn't working at all.
> > > > > Also, the driver doesn't expose the BACKLIGHT RandR property in
> > > > > any case, so xbacklight doesn't work with KMS.
> > > > > 
> > > > > I've been working on a patch as a quick-hack for this (attached;
> > > > > please DON'T apply), and am finally understanding enough to
> > > > > actually implement this correctly together with GregKH.
> > > > > 
> > > > > 
> > > > > What we're supposing is
> > > > > 
> > > > > - a kernel driver for the /sys/class/backlight/ api for the legacy
> > > > > method
> > > > > - support in the driver to actually open this one (trivial)
> > > > > - support for the BACKLIGHT property in KMS case, *only* using the
> > > > >   kernel api method for now (will need some abstraction)
> > > > 
> > > > This sounds like the correct approach.  We have an RFE open on this
> > > > too: http://bugs.freedesktop.org/show_bug.cgi?id=20963
> > > 
> > > My question is, does this belong in the kernel side, to expose the
> > > backlight through /sys/class/backlight/, or do we do this on the
> > > userspace side in perhaps the xorg intel driver (where we can touch
> > > pci config space just as easily)?
> > 
> > We're trying to avoid any direct PCI access from userland these days.
> > I was hoping we could settle on the /sys/class/backlight interface.
> 
> Ok, I'll go knock up a Samsung laptop driver that handles this properly
> then.  Thanks for the information.

Here's a first cut at such a driver.  It works for me on this laptop,
but note that it will try to work on _any_ Intel-based system.  I'll
figure out how to do this only for specific machines based on DMI data
next.

Any objections to this?

thanks,

greg k-h
Greg KH Aug. 14, 2009, 7:28 p.m. UTC | #6
On Fri, Aug 14, 2009 at 12:08:21PM -0700, Greg KH wrote:
> Here's a first cut at such a driver.  It works for me on this laptop,
> but note that it will try to work on _any_ Intel-based system.  I'll
> figure out how to do this only for specific machines based on DMI data
> next.

And here's one that is set for DMI data to only load on the proper ones.

I'll push this upstream now, if there are no objections.

thanks,

greg k-h
Keith Packard Aug. 14, 2009, 7:49 p.m. UTC | #7
On Fri, 2009-08-14 at 11:13 -0700, Greg KH wrote:


> My question is, does this belong in the kernel side, to expose the
> backlight through /sys/class/backlight/, or do we do this on the
> userspace side in perhaps the xorg intel driver (where we can touch
> pci config space just as easily)?

Also, /sys/class/backlight makes it hard to deal with machines that have
two screens with different backlights, so we should at least hide this
from user space for now and provide a suitable abstraction through KMS
properties...
Greg KH Aug. 14, 2009, 8:10 p.m. UTC | #8
On Fri, Aug 14, 2009 at 12:49:32PM -0700, Keith Packard wrote:
> On Fri, 2009-08-14 at 11:13 -0700, Greg KH wrote:
> 
> 
> > My question is, does this belong in the kernel side, to expose the
> > backlight through /sys/class/backlight/, or do we do this on the
> > userspace side in perhaps the xorg intel driver (where we can touch
> > pci config space just as easily)?
> 
> Also, /sys/class/backlight makes it hard to deal with machines that have
> two screens with different backlights, so we should at least hide this
> from user space for now and provide a suitable abstraction through KMS
> properties...

Why?  /sys/class/backlight should expose 2 backlights if they are known
and present, right?  Or is the problem that we don't expose new screens
in sysfs for userspace to be able to work out which is which?

If so, then that should be fixed as well :)

thanks,

greg k-h
Keith Packard Aug. 14, 2009, 8:34 p.m. UTC | #9
On Fri, 2009-08-14 at 13:10 -0700, Greg KH wrote:

> Why?  /sys/class/backlight should expose 2 backlights if they are known
> and present, right?  Or is the problem that we don't expose new screens
> in sysfs for userspace to be able to work out which is which?

We don't know which backlight goes with which LVDS device...

> If so, then that should be fixed as well :)

It would be nice, but I haven't seen a proposal that attempts to address
this. Hiding the backlight behind a DRM interface would mean that future
fixes could be folded in without requiring changes in user-mode code.
Greg KH Aug. 14, 2009, 8:45 p.m. UTC | #10
On Fri, Aug 14, 2009 at 01:34:20PM -0700, Keith Packard wrote:
> On Fri, 2009-08-14 at 13:10 -0700, Greg KH wrote:
> 
> > Why?  /sys/class/backlight should expose 2 backlights if they are known
> > and present, right?  Or is the problem that we don't expose new screens
> > in sysfs for userspace to be able to work out which is which?
> 
> We don't know which backlight goes with which LVDS device...

That's a problem, isn't there some way to figure it out from within the
kernel?

> > If so, then that should be fixed as well :)
> 
> It would be nice, but I haven't seen a proposal that attempts to address
> this. Hiding the backlight behind a DRM interface would mean that future
> fixes could be folded in without requiring changes in user-mode code.

But then what about all of the existing drivers already using the
/sys/class/blacklight interface?

Doesn't acpi export this information on lots of machines as well
already?

The only reason I had to write a new driver for the Samsung hardware,
was because they were not properly exporting the ACPI tables needed for
backlight support.  If they had done that, then no one would even be
having this conversation, which leads me to believe that lots of people
already depend on the /sys/class/backlight interface today, and that
will have to be resolved somehow no matter what.

thanks,

greg k-h
Keith Packard Aug. 14, 2009, 9:03 p.m. UTC | #11
On Fri, 2009-08-14 at 13:45 -0700, Greg KH wrote:

> That's a problem, isn't there some way to figure it out from within the
> kernel?

Not that I know of. Fortunately, we don't have machines with multiple
backlights, at least not many of them.

> But then what about all of the existing drivers already using the
> /sys/class/blacklight interface?

The X server was wrapping this and exposing it as a RandR property, and
so applications could rely on RandR for backlight control for a specific
display. Of course, what we've mostly seen is applications just
using /sys/class/backlight and not caring which monitor it was
affecting.

> Doesn't acpi export this information on lots of machines as well
> already?

Yup, but again, with only one backlight, things are pretty easy.

> The only reason I had to write a new driver for the Samsung hardware,
> was because they were not properly exporting the ACPI tables needed for
> backlight support.  If they had done that, then no one would even be
> having this conversation, which leads me to believe that lots of people
> already depend on the /sys/class/backlight interface today, and that
> will have to be resolved somehow no matter what.

Using either DRM or /sys/class/backlight seems fine to me, aside for the
small problem of associating the /sys/class/backlight with a specific
monitor.
Greg KH Aug. 14, 2009, 9:33 p.m. UTC | #12
On Fri, Aug 14, 2009 at 02:03:02PM -0700, Keith Packard wrote:
> On Fri, 2009-08-14 at 13:45 -0700, Greg KH wrote:
> > But then what about all of the existing drivers already using the
> > /sys/class/blacklight interface?
> 
> The X server was wrapping this and exposing it as a RandR property, and
> so applications could rely on RandR for backlight control for a specific
> display. Of course, what we've mostly seen is applications just
> using /sys/class/backlight and not caring which monitor it was
> affecting.

So if individual drivers provide this interface, everything should be
fine, right?  With the exception of the dual-display issue, which is
a different problem.

thanks,

greg k-h
Jesse Barnes Aug. 15, 2009, 5:16 a.m. UTC | #13
On Fri, 14 Aug 2009 12:28:47 -0700
Greg KH <greg@kroah.com> wrote:

> On Fri, Aug 14, 2009 at 12:08:21PM -0700, Greg KH wrote:
> > Here's a first cut at such a driver.  It works for me on this
> > laptop, but note that it will try to work on _any_ Intel-based
> > system.  I'll figure out how to do this only for specific machines
> > based on DMI data next.
> 
> And here's one that is set for DMI data to only load on the proper
> ones.
> 
> I'll push this upstream now, if there are no objections.

Yeah, looks fairly reasonable.  This code should work on most pre-965
chipsets too, since they share the LBB regs.  There are a couple of
other things to take into consideration: I think the VBIOS indicates
whether the controls are inverted (i.e. a value of 0xff is minimum
brightness) and there may also be some stepping info.

So ultimately I'd like to see this be part of the i915 driver as one of
a few different methods supported (hopefully we can autodetect them
better than the 2D driver though, using the VBIOS and DMI in the
kernel).

The other three methods that need to export sysfs interfaces are
OpRegion (already done), i2c (some machines have external controllers;
the VBIOS describes how to interact with them) and PWM (the "native"
power modulation regs).

Thanks,
Jesse
Mike Lothian Aug. 15, 2009, 12:33 p.m. UTC | #14
2009/8/15 Jesse Barnes <jbarnes@virtuousgeek.org>:
> On Fri, 14 Aug 2009 12:28:47 -0700
> Greg KH <greg@kroah.com> wrote:
>
>> On Fri, Aug 14, 2009 at 12:08:21PM -0700, Greg KH wrote:
>> > Here's a first cut at such a driver.  It works for me on this
>> > laptop, but note that it will try to work on _any_ Intel-based
>> > system.  I'll figure out how to do this only for specific machines
>> > based on DMI data next.
>>
>> And here's one that is set for DMI data to only load on the proper
>> ones.
>>
>> I'll push this upstream now, if there are no objections.
>
> Yeah, looks fairly reasonable.  This code should work on most pre-965
> chipsets too, since they share the LBB regs.  There are a couple of
> other things to take into consideration: I think the VBIOS indicates
> whether the controls are inverted (i.e. a value of 0xff is minimum
> brightness) and there may also be some stepping info.
>
> So ultimately I'd like to see this be part of the i915 driver as one of
> a few different methods supported (hopefully we can autodetect them
> better than the 2D driver though, using the VBIOS and DMI in the
> kernel).
>
> The other three methods that need to export sysfs interfaces are
> OpRegion (already done), i2c (some machines have external controllers;
> the VBIOS describes how to interact with them) and PWM (the "native"
> power modulation regs).

This is great news guys, hopefully I'll finally be able to control the
screen brightness of my GM45 in my Samsung R510

If you want a guinea pig to test your code please feel free to forward patches

Cheers

Mike
Matthew Garrett Aug. 16, 2009, 1:26 p.m. UTC | #15
On Fri, Aug 14, 2009 at 12:28:47PM -0700, Greg KH wrote:
> On Fri, Aug 14, 2009 at 12:08:21PM -0700, Greg KH wrote:
> > Here's a first cut at such a driver.  It works for me on this laptop,
> > but note that it will try to work on _any_ Intel-based system.  I'll
> > figure out how to do this only for specific machines based on DMI data
> > next.
> 
> And here's one that is set for DMI data to only load on the proper ones.
> 
> I'll push this upstream now, if there are no objections.

This seems basically right, but I don't think DMI's the correct 
approach. We should be trying the registers on any device that doesn't 
implement the ACPI code and doesn't provide a more generic 
platform-specific interface. Speaking of which, are we sure there isn't 
a Samsung-specific interface? Using a platform interface is always 
preferable to hitting the registers directly, since that way we don't 
have the firmware getting out of sync with the hardware.
Greg KH Aug. 16, 2009, 10:10 p.m. UTC | #16
On Sun, Aug 16, 2009 at 02:26:22PM +0100, Matthew Garrett wrote:
> On Fri, Aug 14, 2009 at 12:28:47PM -0700, Greg KH wrote:
> > On Fri, Aug 14, 2009 at 12:08:21PM -0700, Greg KH wrote:
> > > Here's a first cut at such a driver.  It works for me on this laptop,
> > > but note that it will try to work on _any_ Intel-based system.  I'll
> > > figure out how to do this only for specific machines based on DMI data
> > > next.
> > 
> > And here's one that is set for DMI data to only load on the proper ones.
> > 
> > I'll push this upstream now, if there are no objections.
> 
> This seems basically right, but I don't think DMI's the correct 
> approach. We should be trying the registers on any device that doesn't 
> implement the ACPI code and doesn't provide a more generic 
> platform-specific interface.

How do we know this?  I'll gladly change the driver if there is some
other better way.  Right now I'm only adding laptops that we know need
this kind of configuration as there is no ACPI interface for the
control.

> Speaking of which, are we sure there isn't a Samsung-specific
> interface? Using a platform interface is always preferable to hitting
> the registers directly, since that way we don't have the firmware
> getting out of sync with the hardware.

From the rumors I have heard from some samsung contacts, they
recommended touching the pci config space directly, which implies that
there is no other way to do this right now :(

thanks,

greg k-h
Greg KH Aug. 16, 2009, 10:11 p.m. UTC | #17
On Fri, Aug 14, 2009 at 10:16:06PM -0700, Jesse Barnes wrote:
> On Fri, 14 Aug 2009 12:28:47 -0700
> Greg KH <greg@kroah.com> wrote:
> 
> > On Fri, Aug 14, 2009 at 12:08:21PM -0700, Greg KH wrote:
> > > Here's a first cut at such a driver.  It works for me on this
> > > laptop, but note that it will try to work on _any_ Intel-based
> > > system.  I'll figure out how to do this only for specific machines
> > > based on DMI data next.
> > 
> > And here's one that is set for DMI data to only load on the proper
> > ones.
> > 
> > I'll push this upstream now, if there are no objections.
> 
> Yeah, looks fairly reasonable.  This code should work on most pre-965
> chipsets too, since they share the LBB regs.  There are a couple of
> other things to take into consideration: I think the VBIOS indicates
> whether the controls are inverted (i.e. a value of 0xff is minimum
> brightness) and there may also be some stepping info.

Where can I find the register information so that I can properly read
this kind of thing?

> So ultimately I'd like to see this be part of the i915 driver as one of
> a few different methods supported (hopefully we can autodetect them
> better than the 2D driver though, using the VBIOS and DMI in the
> kernel).

That would be nice, I have no objection for that happening eventually.
Feel free to take the driver and merge it into yours if you feel that is
better.

thanks,

greg k-h
Matthew Garrett Aug. 16, 2009, 10:21 p.m. UTC | #18
On Sun, Aug 16, 2009 at 03:10:03PM -0700, Greg KH wrote:
> On Sun, Aug 16, 2009 at 02:26:22PM +0100, Matthew Garrett wrote:
> > This seems basically right, but I don't think DMI's the correct 
> > approach. We should be trying the registers on any device that doesn't 
> > implement the ACPI code and doesn't provide a more generic 
> > platform-specific interface.
> 
> How do we know this?  I'll gladly change the driver if there is some
> other better way.  Right now I'm only adding laptops that we know need
> this kind of configuration as there is no ACPI interface for the
> control.

The probability of the backlight control registers being hooked up is 
massively greater than the probability of it being done entirely in some 
out of band manner, and even then you'll just have register writes that 
do nothing. As Jesse says, the proper solution includes supporting the 
hardware with off-chip backlight controllers - but I don't think docs on 
how those are handled have been released by Intel.

What you probably do want to be doing is checking the mmio backlight 
register and seeing whether the chip's in legacy, native or combination 
mode. The opregion code already does this, so that probably wants to be 
factored out and another entry point added for systems without any other 
control method. DMI-strings and hardcoded PCI config writes will work, 
but it's not really the correct solution.

> > Speaking of which, are we sure there isn't a Samsung-specific
> > interface? Using a platform interface is always preferable to hitting
> > the registers directly, since that way we don't have the firmware
> > getting out of sync with the hardware.
> 
> From the rumors I have heard from some samsung contacts, they
> recommended touching the pci config space directly, which implies that
> there is no other way to do this right now :(

Fair enough.
Zhao, Yakui Aug. 17, 2009, 1:22 a.m. UTC | #19
On Mon, 2009-08-17 at 06:21 +0800, Matthew Garrett wrote:
> On Sun, Aug 16, 2009 at 03:10:03PM -0700, Greg KH wrote:
> > On Sun, Aug 16, 2009 at 02:26:22PM +0100, Matthew Garrett wrote:
> > > This seems basically right, but I don't think DMI's the correct 
> > > approach. We should be trying the registers on any device that doesn't 
> > > implement the ACPI code and doesn't provide a more generic 
> > > platform-specific interface.
> > 
> > How do we know this?  I'll gladly change the driver if there is some
> > other better way.  Right now I'm only adding laptops that we know need
> > this kind of configuration as there is no ACPI interface for the
> > control.
> 
> The probability of the backlight control registers being hooked up is 
> massively greater than the probability of it being done entirely in some 
> out of band manner, and even then you'll just have register writes that 
> do nothing. As Jesse says, the proper solution includes supporting the 
> hardware with off-chip backlight controllers - but I don't think docs on 
> how those are handled have been released by Intel.
> 
> What you probably do want to be doing is checking the mmio backlight 
> register and seeing whether the chip's in legacy, native or combination 
> mode. The opregion code already does this, so that probably wants to be 
> factored out and another entry point added for systems without any other 
> control method. DMI-strings and hardcoded PCI config writes will work, 
> but it's not really the correct solution.
This is what we have done in UMS code. It will firstly check whether
there exists the acpi backlight interface under
the /sys/class/backlight/. If it exists, it will use the acpi backlight
method to adjust the brightness. If it doesn't exist, it will read the
mmio register and select the backlight control method.(legacy, native,
combination). And the backlight control method is also switched by using
xrandr tool. (Opregion code is realized as the generic acpi control
method.)

In KMS mode the backlight property is not exposed to xrandr tool. In
such case the backlight can't be controlled by the xrandr tool. More
worse is that there is no interface that can control the backlight if
there is no acpi backlight interface under /sys/class/backlight/. 

In fact in KMS case if we expose a backlight property to xrandr in i915
driver, then the backlight can be controlled by the xrandr tool. But the
problem is which control method should be exposed to user-space.(acpi,
native, legacy, combo)


Another issue is the inconsistence between the different control
methods. In UMS case if we select the legacy control method by using
xrandr tool, we will get the incorrect brightness
through /sys/class/backlight/* when the backlight is adjusted through
xrandr tool. So we had better expose only one backlight interface under
the /sys/class/backlight/ and this interface is also hooked up by the
xrandr tool. In such case the driver will also register the backlight
interface under the /sys/class/backlight/.

But another problem arises. In kernel side three drivers can register
the backlight interface.
  >Acpi video (generic acpi method)
  >platform driver
  >i915 driver.
  Which should be selected? What rule should be followed to select the
backlight interface? 

Thanks.



> 
> > > Speaking of which, are we sure there isn't a Samsung-specific
> > > interface? Using a platform interface is always preferable to hitting
> > > the registers directly, since that way we don't have the firmware
> > > getting out of sync with the hardware.
> > 
> > From the rumors I have heard from some samsung contacts, they
> > recommended touching the pci config space directly, which implies that
> > there is no other way to do this right now :(
> 
> Fair enough.
>
Matthias Hopf Aug. 17, 2009, 9:47 a.m. UTC | #20
On Aug 14, 09 11:29:39 -0700, Greg KH wrote:
> > We're trying to avoid any direct PCI access from userland these days.
> > I was hoping we could settle on the /sys/class/backlight interface.
> Ok, I'll go knock up a Samsung laptop driver that handles this properly
> then.  Thanks for the information.

Greg, this wouldn't be a Samsung specific driver - it's an i915 specific
driver that provides the legacy interface. It usable on more than just
Samsung.

CU

Matthias
Greg KH Aug. 17, 2009, 4:11 p.m. UTC | #21
On Sun, Aug 16, 2009 at 11:21:16PM +0100, Matthew Garrett wrote:
> On Sun, Aug 16, 2009 at 03:10:03PM -0700, Greg KH wrote:
> > On Sun, Aug 16, 2009 at 02:26:22PM +0100, Matthew Garrett wrote:
> > > This seems basically right, but I don't think DMI's the correct 
> > > approach. We should be trying the registers on any device that doesn't 
> > > implement the ACPI code and doesn't provide a more generic 
> > > platform-specific interface.
> > 
> > How do we know this?  I'll gladly change the driver if there is some
> > other better way.  Right now I'm only adding laptops that we know need
> > this kind of configuration as there is no ACPI interface for the
> > control.
> 
> The probability of the backlight control registers being hooked up is 
> massively greater than the probability of it being done entirely in some 
> out of band manner, and even then you'll just have register writes that 
> do nothing. As Jesse says, the proper solution includes supporting the 
> hardware with off-chip backlight controllers - but I don't think docs on 
> how those are handled have been released by Intel.
> 
> What you probably do want to be doing is checking the mmio backlight 
> register and seeing whether the chip's in legacy, native or combination 
> mode.

How do I do this?

> The opregion code already does this, so that probably wants to be
> factored out and another entry point added for systems without any
> other control method. DMI-strings and hardcoded PCI config writes will
> work, but it's not really the correct solution.

As stated before, Samsung said this was the best way to control the
backlight on these laptops, so should I ignore them?  :)

thanks,

greg k-h
Greg KH Aug. 17, 2009, 4:12 p.m. UTC | #22
On Mon, Aug 17, 2009 at 11:47:01AM +0200, Matthias Hopf wrote:
> On Aug 14, 09 11:29:39 -0700, Greg KH wrote:
> > > We're trying to avoid any direct PCI access from userland these days.
> > > I was hoping we could settle on the /sys/class/backlight interface.
> > Ok, I'll go knock up a Samsung laptop driver that handles this properly
> > then.  Thanks for the information.
> 
> Greg, this wouldn't be a Samsung specific driver - it's an i915 specific
> driver that provides the legacy interface. It usable on more than just
> Samsung.

It is?  What other hardware does this work on that does not provide the
"proper" ACPI interfaces?

Right now it's the only way to get the Samsung laptops working, so I'm
sticking with only supporting them, unless other companies say it's how
to control their hardware as well.

Sound reasonable?

thanks,

greg k-h
Matthew Garrett Aug. 17, 2009, 4:29 p.m. UTC | #23
On Mon, Aug 17, 2009 at 09:11:26AM -0700, Greg KH wrote:
> On Sun, Aug 16, 2009 at 11:21:16PM +0100, Matthew Garrett wrote:
> > What you probably do want to be doing is checking the mmio backlight 
> > register and seeing whether the chip's in legacy, native or combination 
> > mode.
> 
> How do I do this?

Check the function at the top of i915_opregion.c - the mmio registers 
are documented in volume 3 of the 965 docs, the PCI register is in 
volume 1.

> > The opregion code already does this, so that probably wants to be
> > factored out and another entry point added for systems without any
> > other control method. DMI-strings and hardcoded PCI config writes will
> > work, but it's not really the correct solution.
> 
> As stated before, Samsung said this was the best way to control the
> backlight on these laptops, so should I ignore them?  :)

Basically, yeah. It may be that Samsungs always use the legacy register, 
but there's other hardware that also doesn't have a platform driver or 
ACPI support for this.
Jesse Barnes Aug. 17, 2009, 4:33 p.m. UTC | #24
On Sun, 16 Aug 2009 15:11:07 -0700
Greg KH <greg@kroah.com> wrote:

> On Fri, Aug 14, 2009 at 10:16:06PM -0700, Jesse Barnes wrote:
> > On Fri, 14 Aug 2009 12:28:47 -0700
> > Greg KH <greg@kroah.com> wrote:
> > 
> > > On Fri, Aug 14, 2009 at 12:08:21PM -0700, Greg KH wrote:
> > > > Here's a first cut at such a driver.  It works for me on this
> > > > laptop, but note that it will try to work on _any_ Intel-based
> > > > system.  I'll figure out how to do this only for specific
> > > > machines based on DMI data next.
> > > 
> > > And here's one that is set for DMI data to only load on the proper
> > > ones.
> > > 
> > > I'll push this upstream now, if there are no objections.
> > 
> > Yeah, looks fairly reasonable.  This code should work on most
> > pre-965 chipsets too, since they share the LBB regs.  There are a
> > couple of other things to take into consideration: I think the
> > VBIOS indicates whether the controls are inverted (i.e. a value of
> > 0xff is minimum brightness) and there may also be some stepping
> > info.
> 
> Where can I find the register information so that I can properly read
> this kind of thing?

The best answer I have for this right now is
intel_bios.h/intel_bios.c.  I think the table type is documented in the
header, but we haven't written any code to get it out of the VBIOS in
intel_bios.c yet.

> 
> > So ultimately I'd like to see this be part of the i915 driver as
> > one of a few different methods supported (hopefully we can
> > autodetect them better than the 2D driver though, using the VBIOS
> > and DMI in the kernel).
> 
> That would be nice, I have no objection for that happening eventually.
> Feel free to take the driver and merge it into yours if you feel that
> is better.

Great, thanks a lot for putting this together Greg.
Matthias Hopf Aug. 17, 2009, 4:58 p.m. UTC | #25
On Aug 17, 09 09:12:52 -0700, Greg KH wrote:
> > Greg, this wouldn't be a Samsung specific driver - it's an i915 specific
> > driver that provides the legacy interface. It usable on more than just
> > Samsung.
> 
> It is?  What other hardware does this work on that does not provide the
> "proper" ACPI interfaces?
> 
> Right now it's the only way to get the Samsung laptops working, so I'm
> sticking with only supporting them, unless other companies say it's how
> to control their hardware as well.
> 
> Sound reasonable?

It's up to you. I have a different feeling, but can perfectly live with
your decision if that's it.

CU

Matthias, getting a bit confused due to the number of threads basically
running around the same topic ;-)
diff mbox

Patch

diff -urp xf86-video-intel-2.8.0.orig/src/drmmode_display.c xf86-video-intel-2.8.0/src/drmmode_display.c
--- xf86-video-intel-2.8.0.orig/src/drmmode_display.c	2009-07-11 01:31:10.000000000 -0400
+++ xf86-video-intel-2.8.0/src/drmmode_display.c	2009-08-14 17:15:56.000000000 -0400
@@ -74,6 +74,7 @@  typedef struct {
     drmmode_prop_ptr props;
     void *private_data;
     int dpms_mode;
+    struct backlight_ctrl *backlight;
 } drmmode_output_private_rec, *drmmode_output_private_ptr;
 
 static void
@@ -712,11 +713,50 @@  drmmode_output_destroy(xf86OutputPtr out
 		xfree(drmmode_output->private_data);
 		drmmode_output->private_data = NULL;
 	}
+	if (drmmode_output->backlight) {
+		drmmode_output->backlight->set_backlight(output,
+			 drmmode_output->backlight->backlight_duty_cycle);
+		xfree(drmmode_output->backlight);
+	}
 	xfree(drmmode_output);
 	output->driver_private = NULL;
 }
 
 static void
+drmmode_output_dpms_backlight(xf86OutputPtr output, struct backlight_ctrl *bl,
+	int oldmode, int mode)
+{
+	ScrnInfoPtr pScrn = output->scrn;
+	I830Ptr     pI830 = I830PTR(pScrn);
+
+	if (!bl)
+		return;
+
+	if (mode == DPMSModeOn) {
+		/*
+		 * If we're going from off->on we may need to turn on the backlight.
+		 * We should use the saved value whenever possible, but on some
+		 * machines 0 is a valid backlight value (due to an external
+		 * backlight controller for example), so on them, when turning LVDS
+		 * back on, they'll always re-maximize the brightness.
+		 */
+		if (mode != oldmode &&
+		    bl->backlight_duty_cycle == 0 &&
+		    pI830->backlight_control_method < BCM_KERNEL)
+		    bl->backlight_duty_cycle = bl->backlight_max;
+
+		bl->set_backlight(output, bl->backlight_duty_cycle);
+	} else {
+		/*
+		 * Only save the current backlight value if we're going from on to off.
+		 */
+		if (mode != oldmode)
+			bl->backlight_duty_cycle = bl->get_backlight(output);
+		bl->set_backlight(output, 0);
+	}
+}
+
+static void
 drmmode_output_dpms(xf86OutputPtr output, int mode)
 {
 	drmmode_output_private_ptr drmmode_output = output->driver_private;
@@ -735,8 +775,13 @@  drmmode_output_dpms(xf86OutputPtr output
                                 drmmode_output->output_id,
                                 props->prop_id,
                                 mode);
+			drmmode_output_dpms_backlight(output,
+				drmmode_output->backlight,
+				drmmode_output->dpms_mode,
+				mode);
 			drmmode_output->dpms_mode = mode;
                         drmModeFreeProperty(props);
+
                         return;
 		}
 		drmModeFreeProperty(props);
@@ -767,6 +812,9 @@  drmmode_property_ignore(drmModePropertyP
     return FALSE;
 }
 
+#define BACKLIGHT_NAME  "BACKLIGHT"
+static Atom backlight_atom;
+
 static void
 drmmode_output_create_resources(xf86OutputPtr output)
 {
@@ -774,7 +822,8 @@  drmmode_output_create_resources(xf86Outp
     drmModeConnectorPtr mode_output = drmmode_output->mode_output;
     drmmode_ptr drmmode = drmmode_output->drmmode;
     drmModePropertyPtr drmmode_prop;
-    int i, j, err;
+    int i, j, err, data;
+    INT32 backlight_range[2];
 
     drmmode_output->props = xcalloc(mode_output->count_props, sizeof(drmmode_prop_rec));
     if (!drmmode_output->props)
@@ -851,6 +900,35 @@  drmmode_output_create_resources(xf86Outp
 	    }
 	}
     }
+
+    if (drmmode_output->backlight) {
+	/* Set up the backlight property, which takes effect immediately
+	 * and accepts values only within the backlight_range.
+	 *
+	 * XXX: Currently, RandR doesn't verify that properties set are
+	 * within the backlight_range.
+	 */
+	backlight_atom = MakeAtom(BACKLIGHT_NAME, sizeof(BACKLIGHT_NAME) - 1,
+	    TRUE);
+
+	backlight_range[0] = 0;
+	backlight_range[1] = drmmode_output->backlight->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->backlight_duty_cycle;
+	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
@@ -861,6 +939,27 @@  drmmode_output_set_property(xf86OutputPt
     drmmode_ptr drmmode = drmmode_output->drmmode;
     int i;
 
+    if (property == backlight_atom) {
+	struct backlight_ctrl *bl = drmmode_output->backlight;
+        INT32 val;
+
+        if (value->type != XA_INTEGER || value->format != 32 ||
+            value->size != 1)
+        {
+            return FALSE;
+        }
+
+        val = *(INT32 *)value->data;
+        if (val < 0 || val > bl->backlight_max)
+            return FALSE;
+
+        if (val != bl->backlight_duty_cycle) {
+            bl->set_backlight(output, val);
+            bl->backlight_duty_cycle = val;
+        }
+        return TRUE;
+    }
+
     for (i = 0; i < drmmode_output->num_props; i++) {
 	drmmode_prop_ptr p = &drmmode_output->props[i];
 
@@ -996,6 +1095,10 @@  drmmode_output_init(ScrnInfoPtr pScrn, d
 		if (!drmmode_output->private_data)
 			xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
 				"Can't allocate private memory for LVDS.\n");
+		i830_backlightonly_init(output, &drmmode_output->backlight);
+		if (!drmmode_output->backlight)
+		    xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+			       "Can't allocate backlight control memory for LVDS.\n");
 	}
 	drmmode_output->output_id = drmmode->mode_res->connectors[num];
 	drmmode_output->mode_output = koutput;
diff -urp xf86-video-intel-2.8.0.orig/src/i830.h xf86-video-intel-2.8.0/src/i830.h
--- xf86-video-intel-2.8.0.orig/src/i830.h	2009-07-21 01:41:16.000000000 -0400
+++ xf86-video-intel-2.8.0/src/i830.h	2009-08-14 16:51:32.000000000 -0400
@@ -316,6 +316,14 @@  enum backlight_control {
     BCM_COMBO,
     BCM_KERNEL,
 };
+struct backlight_ctrl {
+    void (*set_backlight)(xf86OutputPtr output, int level);
+    int (*get_backlight)(xf86OutputPtr output);
+    int backlight_max;
+    /* restore backlight to this value */
+    int             backlight_duty_cycle;
+};
+
 
 enum dri_type {
     DRI_DISABLED,
@@ -755,6 +763,7 @@  void i830_hdmi_init(ScrnInfoPtr pScrn, i
 
 /* i830_lvds.c */
 void i830_lvds_init(ScrnInfoPtr pScrn);
+void i830_backlightonly_init(xf86OutputPtr output, struct backlight_ctrl **backlight);
 
 /* i830_memory.c */
 Bool i830_bind_all_memory(ScrnInfoPtr pScrn);
diff -urp xf86-video-intel-2.8.0.orig/src/i830_lvds.c xf86-video-intel-2.8.0/src/i830_lvds.c
--- xf86-video-intel-2.8.0.orig/src/i830_lvds.c	2009-06-26 12:30:59.000000000 -0400
+++ xf86-video-intel-2.8.0/src/i830_lvds.c	2009-08-14 17:12:40.000000000 -0400
@@ -1624,3 +1624,89 @@  disable_exit:
     xf86DestroyI2CBusRec(intel_output->pDDCBus, TRUE, TRUE);
     xf86OutputDestroy(output);
 }
+
+void
+i830_backlightonly_init(xf86OutputPtr output,
+			struct backlight_ctrl **backlight)
+{
+    I830Ptr		   pI830 = I830PTR(output->scrn);
+    struct backlight_ctrl *ctrl;
+
+    /* FIXME: this should still parse the bios.
+     * The function in i830_bios.c doesn't do that ATM either, though. */
+    /* For mobile chip, set default as true */
+    if (IS_MOBILE(pI830) && !IS_I830(pI830))
+        pI830->integrated_lvds = TRUE;
+
+    if (!pI830->integrated_lvds) {
+	xf86DrvMsg(output->scrn->scrnIndex, X_INFO,
+		   "Skipping LVDS from driver feature BDB's LVDS config info.\n");
+	return;
+    }
+
+    if (pI830->quirk_flag & QUIRK_IGNORE_LVDS)
+	return;
+
+    /* Blacklist machines with BIOSes that list an LVDS panel without actually
+     * having one.
+     */
+    if (pI830->quirk_flag & QUIRK_IGNORE_MACMINI_LVDS) {
+	/* It's a Mac Mini or Macbook Pro.
+	 *
+	 * Apple hardware is out to get us.  The macbook pro has a real
+	 * LVDS panel, but the mac mini does not, and they have the same
+	 * device IDs.  We'll distinguish by panel size, on the assumption
+	 * that Apple isn't about to make any machines with an 800x600
+	 * display.
+	 */
+
+	if (pI830->lvds_fixed_mode != NULL &&
+		pI830->lvds_fixed_mode->HDisplay == 800 &&
+		pI830->lvds_fixed_mode->VDisplay == 600)
+	{
+	    xf86DrvMsg(output->scrn->scrnIndex, X_INFO,
+		    "Suspected Mac Mini, ignoring the LVDS\n");
+	    return;
+	}
+    }
+
+    ctrl = xcalloc (1, sizeof (struct backlight_ctrl));
+    if (!ctrl)
+	return;
+
+    /* Try to figure out which backlight control method to use */
+    /* Can't access VGA space due to KMS - assuming native method is handled by kernel */
+    if (i830_kernel_backlight_available(output))
+	pI830->backlight_control_method = BCM_KERNEL;
+    else
+	pI830->backlight_control_method = BCM_LEGACY;
+
+    switch (pI830->backlight_control_method) {
+    case BCM_NATIVE:
+	ctrl->set_backlight = i830_lvds_set_backlight_native;
+	ctrl->get_backlight = i830_lvds_get_backlight_native;
+	ctrl->backlight_max = i830_lvds_get_backlight_max_native(output);
+	break;
+    case BCM_LEGACY:
+	ctrl->set_backlight = i830_lvds_set_backlight_legacy;
+	ctrl->get_backlight = i830_lvds_get_backlight_legacy;
+	ctrl->backlight_max = 0xff;
+	break;
+    case BCM_COMBO:
+	ctrl->set_backlight = i830_lvds_set_backlight_combo;
+	ctrl->get_backlight = i830_lvds_get_backlight_combo;
+	ctrl->backlight_max = i830_lvds_get_backlight_max_combo(output);
+	break;
+    case BCM_KERNEL:
+	ctrl->set_backlight = i830_lvds_set_backlight_kernel;
+	ctrl->get_backlight = i830_lvds_get_backlight_kernel;
+	ctrl->backlight_max = i830_lvds_get_backlight_max_kernel(output);
+	break;
+    default:
+	xf86DrvMsg(output->scrn->scrnIndex, X_ERROR, "bad backlight control method\n");
+	break;
+    }
+
+    ctrl->backlight_duty_cycle = ctrl->get_backlight(output);
+    *backlight = ctrl;
+}