[PATCH RFC 0/4] Linking DRM Connectors to Backlight Devices

[PATCH RFC 0/4] Linking DRM Connectors to Backlight Devices

[David Herrmann]

Hi

This series introduces a link between DRM connectors and backlight-class
devices. It tries to solve some long standing issues:

 * User-space currently has a hard-time figuring out which backlight device to
   use, and which backlight device belongs to which display. So far, most
   systems only provide backlight-devices for internal displays, so figuring out
   the connection is easy, but that might change with more capable external
   connectors.
   If multiple backlights are available, the easiest solution is to simply write
   to all of them and hope at least one of them works. This obviously fails if
   the devices interact badly, so it's not really a solution.

 * User-space needs root privileges to write to sysfs. There are no char-devs
   that can be used to control access-management to the backlight API, so /sys
   is the only interface available. So far, udev policy has been "/sys is
   read-only for non-root" and it's not going to change. char-devs are *THE*
   standard way to provide non-root user-space APIs, so use them!

Several solutions have been discussed so far, but we never agreed on one. I'm
aware of at least the following solutions:

 * Provide char-devs for backlight devices and a sysfs link from connectors to
   backlight devices. This way, user-space can managed access to char-devs *and*
   get proper topology information.
   However, this might be an n:m relationship (n:1 for displays exposed as
   multiple connectors, 1:m for boards with multiple backlights? whatever..) and
   it sounds like a waste of resources to add char-devs just to write brightness
   values.

 * Do everything in privileged user-space. One daemon that provides an
   unprivileged-API and forwards writes to /sys. It reads topology information
   from udev hwdb.
   This circumvents the ugly sysfs API and not really solves the problem. It
   should work, though ugly.

 * Add a DRM connector property that takes brightness values.
   This is the nicest solution for user-space, but introduces a whole lot of
   issues on the kernel side. There already is a backlight-class and we had a
   hard-time trying to move it into DRM drivers, so no-one ever did the work.

This series tries to solve this problem with a much simpler approach:
Instead of moving backlights into DRM, we simply link DRM properties to a
backlight device. That is, the kernel manages a link between a connector and a
backlight device (or n backlight devices) which can be modified by udev in case
the kernel got it wrong (we don't want huge board-fixup-tables in the kernel).
User-space can now use the simpl DRM API to manage backlights, and the kernel
does not need any special driver code to make it work.

Patch #1 and #2 are cleanups and can be applied right away. #3 adds in-kernel
backlight APIs and #4 implements the connector-backlight link.

Comments welcome!
David

David Herrmann (4):
  backlight: use static initializers
  backlight: use spin-lock to protect device list
  backlight: add kernel-internal backlight API
  drm: link connectors to backlight devices

 drivers/gpu/drm/Kconfig             |   1 +
 drivers/gpu/drm/Makefile            |   2 +-
 drivers/gpu/drm/drm_backlight.c     | 387 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc.c          |  45 +++--
 drivers/gpu/drm/drm_drv.c           |  11 +
 drivers/gpu/drm/drm_sysfs.c         |  53 +++++
 drivers/video/backlight/backlight.c |  91 +++++++--
 include/drm/drm_backlight.h         |  44 ++++
 include/drm/drm_crtc.h              |   3 +
 include/linux/backlight.h           |  17 ++
 10 files changed, 622 insertions(+), 32 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_backlight.c
 create mode 100644 include/drm/drm_backlight.h

-- 
2.1.0

[PATCH RFC 0/4] Linking DRM Connectors to Backlight Devices

[David Herrmann]

Hi

This series introduces a link between DRM connectors and backlight-class
devices. It tries to solve some long standing issues:

 * User-space currently has a hard-time figuring out which backlight device to
   use, and which backlight device belongs to which display. So far, most
   systems only provide backlight-devices for internal displays, so figuring out
   the connection is easy, but that might change with more capable external
   connectors.
   If multiple backlights are available, the easiest solution is to simply write
   to all of them and hope at least one of them works. This obviously fails if
   the devices interact badly, so it's not really a solution.

 * User-space needs root privileges to write to sysfs. There are no char-devs
   that can be used to control access-management to the backlight API, so /sys
   is the only interface available. So far, udev policy has been "/sys is
   read-only for non-root" and it's not going to change. char-devs are *THE*
   standard way to provide non-root user-space APIs, so use them!

Several solutions have been discussed so far, but we never agreed on one. I'm
aware of at least the following solutions:

 * Provide char-devs for backlight devices and a sysfs link from connectors to
   backlight devices. This way, user-space can managed access to char-devs *and*
   get proper topology information.
   However, this might be an n:m relationship (n:1 for displays exposed as
   multiple connectors, 1:m for boards with multiple backlights? whatever..) and
   it sounds like a waste of resources to add char-devs just to write brightness
   values.

 * Do everything in privileged user-space. One daemon that provides an
   unprivileged-API and forwards writes to /sys. It reads topology information
   from udev hwdb.
   This circumvents the ugly sysfs API and not really solves the problem. It
   should work, though ugly.

 * Add a DRM connector property that takes brightness values.
   This is the nicest solution for user-space, but introduces a whole lot of
   issues on the kernel side. There already is a backlight-class and we had a
   hard-time trying to move it into DRM drivers, so no-one ever did the work.

This series tries to solve this problem with a much simpler approach:
Instead of moving backlights into DRM, we simply link DRM properties to a
backlight device. That is, the kernel manages a link between a connector and a
backlight device (or n backlight devices) which can be modified by udev in case
the kernel got it wrong (we don't want huge board-fixup-tables in the kernel).
User-space can now use the simpl DRM API to manage backlights, and the kernel
does not need any special driver code to make it work.

Patch #1 and #2 are cleanups and can be applied right away. #3 adds in-kernel
backlight APIs and #4 implements the connector-backlight link.

Comments welcome!
David

David Herrmann (4):
  backlight: use static initializers
  backlight: use spin-lock to protect device list
  backlight: add kernel-internal backlight API
  drm: link connectors to backlight devices

 drivers/gpu/drm/Kconfig             |   1 +
 drivers/gpu/drm/Makefile            |   2 +-
 drivers/gpu/drm/drm_backlight.c     | 387 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc.c          |  45 +++--
 drivers/gpu/drm/drm_drv.c           |  11 +
 drivers/gpu/drm/drm_sysfs.c         |  53 +++++
 drivers/video/backlight/backlight.c |  91 +++++++--
 include/drm/drm_backlight.h         |  44 ++++
 include/drm/drm_crtc.h              |   3 +
 include/linux/backlight.h           |  17 ++
 10 files changed, 622 insertions(+), 32 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_backlight.c
 create mode 100644 include/drm/drm_backlight.h

-- 
2.1.0

[PATCH RFC 1/4] backlight: use static initializers

[David Herrmann]

Use static initializers instead of setting up global variables during
runtime. This reduces code size and execution time.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/video/backlight/backlight.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index bddc8b1..726c6c6 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -21,9 +21,9 @@
 #include <asm/backlight.h>
 #endif
 
-static struct list_head backlight_dev_list;
-static struct mutex backlight_dev_list_mutex;
-static struct blocking_notifier_head backlight_notifier;
+static LIST_HEAD(backlight_dev_list);
+static DEFINE_MUTEX(backlight_dev_list_mutex);
+static BLOCKING_NOTIFIER_HEAD(backlight_notifier);
 
 static const char *const backlight_types[] = {
 	[BACKLIGHT_RAW] = "raw",
@@ -582,9 +582,6 @@ static int __init backlight_class_init(void)
 
 	backlight_class->dev_groups = bl_device_groups;
 	backlight_class->pm = &backlight_class_dev_pm_ops;
-	INIT_LIST_HEAD(&backlight_dev_list);
-	mutex_init(&backlight_dev_list_mutex);
-	BLOCKING_INIT_NOTIFIER_HEAD(&backlight_notifier);
 
 	return 0;
 }
-- 
2.1.0

[PATCH RFC 1/4] backlight: use static initializers

[David Herrmann]

Use static initializers instead of setting up global variables during
runtime. This reduces code size and execution time.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/video/backlight/backlight.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index bddc8b1..726c6c6 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -21,9 +21,9 @@
 #include <asm/backlight.h>
 #endif
 
-static struct list_head backlight_dev_list;
-static struct mutex backlight_dev_list_mutex;
-static struct blocking_notifier_head backlight_notifier;
+static LIST_HEAD(backlight_dev_list);
+static DEFINE_MUTEX(backlight_dev_list_mutex);
+static BLOCKING_NOTIFIER_HEAD(backlight_notifier);
 
 static const char *const backlight_types[] = {
 	[BACKLIGHT_RAW] = "raw",
@@ -582,9 +582,6 @@ static int __init backlight_class_init(void)
 
 	backlight_class->dev_groups = bl_device_groups;
 	backlight_class->pm = &backlight_class_dev_pm_ops;
-	INIT_LIST_HEAD(&backlight_dev_list);
-	mutex_init(&backlight_dev_list_mutex);
-	BLOCKING_INIT_NOTIFIER_HEAD(&backlight_notifier);
 
 	return 0;
 }
-- 
2.1.0

Re: [PATCH RFC 1/4] backlight: use static initializers

[Jani Nikula]

On Wed, 10 Sep 2014, David Herrmann <dh.herrmann@gmail.com> wrote:
> Use static initializers instead of setting up global variables during
> runtime. This reduces code size and execution time.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/video/backlight/backlight.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index bddc8b1..726c6c6 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -21,9 +21,9 @@
>  #include <asm/backlight.h>
>  #endif
>  
> -static struct list_head backlight_dev_list;
> -static struct mutex backlight_dev_list_mutex;
> -static struct blocking_notifier_head backlight_notifier;
> +static LIST_HEAD(backlight_dev_list);
> +static DEFINE_MUTEX(backlight_dev_list_mutex);
> +static BLOCKING_NOTIFIER_HEAD(backlight_notifier);
>  
>  static const char *const backlight_types[] = {
>  	[BACKLIGHT_RAW] = "raw",
> @@ -582,9 +582,6 @@ static int __init backlight_class_init(void)
>  
>  	backlight_class->dev_groups = bl_device_groups;
>  	backlight_class->pm = &backlight_class_dev_pm_ops;
> -	INIT_LIST_HEAD(&backlight_dev_list);
> -	mutex_init(&backlight_dev_list_mutex);
> -	BLOCKING_INIT_NOTIFIER_HEAD(&backlight_notifier);
>  
>  	return 0;
>  }
> -- 
> 2.1.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH RFC 1/4] backlight: use static initializers

[Jani Nikula]

On Wed, 10 Sep 2014, David Herrmann <dh.herrmann@gmail.com> wrote:
> Use static initializers instead of setting up global variables during
> runtime. This reduces code size and execution time.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/video/backlight/backlight.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index bddc8b1..726c6c6 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -21,9 +21,9 @@
>  #include <asm/backlight.h>
>  #endif
>  
> -static struct list_head backlight_dev_list;
> -static struct mutex backlight_dev_list_mutex;
> -static struct blocking_notifier_head backlight_notifier;
> +static LIST_HEAD(backlight_dev_list);
> +static DEFINE_MUTEX(backlight_dev_list_mutex);
> +static BLOCKING_NOTIFIER_HEAD(backlight_notifier);
>  
>  static const char *const backlight_types[] = {
>  	[BACKLIGHT_RAW] = "raw",
> @@ -582,9 +582,6 @@ static int __init backlight_class_init(void)
>  
>  	backlight_class->dev_groups = bl_device_groups;
>  	backlight_class->pm = &backlight_class_dev_pm_ops;
> -	INIT_LIST_HEAD(&backlight_dev_list);
> -	mutex_init(&backlight_dev_list_mutex);
> -	BLOCKING_INIT_NOTIFIER_HEAD(&backlight_notifier);
>  
>  	return 0;
>  }
> -- 
> 2.1.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH RFC 2/4] backlight: use spin-lock to protect device list

[David Herrmann]

There is really no reason to use a mutex to protect a simple list. Convert
the list-lock to a simple spinlock instead.

The spin-locks prepare for a backlight_find() helper, which should
preferably be usable from atomic context. A mutex would prevent that, so
use an irq-save spinlock instead.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/video/backlight/backlight.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 726c6c6..33b64be 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -16,13 +16,14 @@
 #include <linux/err.h>
 #include <linux/fb.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 
 #ifdef CONFIG_PMAC_BACKLIGHT
 #include <asm/backlight.h>
 #endif
 
 static LIST_HEAD(backlight_dev_list);
-static DEFINE_MUTEX(backlight_dev_list_mutex);
+static DEFINE_SPINLOCK(backlight_dev_list_lock);
 static BLOCKING_NOTIFIER_HEAD(backlight_notifier);
 
 static const char *const backlight_types[] = {
@@ -369,9 +370,9 @@ struct backlight_device *backlight_device_register(const char *name,
 	mutex_unlock(&pmac_backlight_mutex);
 #endif
 
-	mutex_lock(&backlight_dev_list_mutex);
+	spin_lock_irq(&backlight_dev_list_lock);
 	list_add(&new_bd->entry, &backlight_dev_list);
-	mutex_unlock(&backlight_dev_list_mutex);
+	spin_unlock_irq(&backlight_dev_list_lock);
 
 	blocking_notifier_call_chain(&backlight_notifier,
 				     BACKLIGHT_REGISTERED, new_bd);
@@ -384,15 +385,16 @@ bool backlight_device_registered(enum backlight_type type)
 {
 	bool found = false;
 	struct backlight_device *bd;
+	unsigned long flags;
 
-	mutex_lock(&backlight_dev_list_mutex);
+	spin_lock_irqsave(&backlight_dev_list_lock, flags);
 	list_for_each_entry(bd, &backlight_dev_list, entry) {
 		if (bd->props.type == type) {
 			found = true;
 			break;
 		}
 	}
-	mutex_unlock(&backlight_dev_list_mutex);
+	spin_unlock_irqrestore(&backlight_dev_list_lock, flags);
 
 	return found;
 }
@@ -409,9 +411,9 @@ void backlight_device_unregister(struct backlight_device *bd)
 	if (!bd)
 		return;
 
-	mutex_lock(&backlight_dev_list_mutex);
+	spin_lock_irq(&backlight_dev_list_lock);
 	list_del(&bd->entry);
-	mutex_unlock(&backlight_dev_list_mutex);
+	spin_unlock_irq(&backlight_dev_list_lock);
 
 #ifdef CONFIG_PMAC_BACKLIGHT
 	mutex_lock(&pmac_backlight_mutex);
-- 
2.1.0

[PATCH RFC 2/4] backlight: use spin-lock to protect device list

[David Herrmann]

There is really no reason to use a mutex to protect a simple list. Convert
the list-lock to a simple spinlock instead.

The spin-locks prepare for a backlight_find() helper, which should
preferably be usable from atomic context. A mutex would prevent that, so
use an irq-save spinlock instead.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/video/backlight/backlight.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 726c6c6..33b64be 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -16,13 +16,14 @@
 #include <linux/err.h>
 #include <linux/fb.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 
 #ifdef CONFIG_PMAC_BACKLIGHT
 #include <asm/backlight.h>
 #endif
 
 static LIST_HEAD(backlight_dev_list);
-static DEFINE_MUTEX(backlight_dev_list_mutex);
+static DEFINE_SPINLOCK(backlight_dev_list_lock);
 static BLOCKING_NOTIFIER_HEAD(backlight_notifier);
 
 static const char *const backlight_types[] = {
@@ -369,9 +370,9 @@ struct backlight_device *backlight_device_register(const char *name,
 	mutex_unlock(&pmac_backlight_mutex);
 #endif
 
-	mutex_lock(&backlight_dev_list_mutex);
+	spin_lock_irq(&backlight_dev_list_lock);
 	list_add(&new_bd->entry, &backlight_dev_list);
-	mutex_unlock(&backlight_dev_list_mutex);
+	spin_unlock_irq(&backlight_dev_list_lock);
 
 	blocking_notifier_call_chain(&backlight_notifier,
 				     BACKLIGHT_REGISTERED, new_bd);
@@ -384,15 +385,16 @@ bool backlight_device_registered(enum backlight_type type)
 {
 	bool found = false;
 	struct backlight_device *bd;
+	unsigned long flags;
 
-	mutex_lock(&backlight_dev_list_mutex);
+	spin_lock_irqsave(&backlight_dev_list_lock, flags);
 	list_for_each_entry(bd, &backlight_dev_list, entry) {
 		if (bd->props.type == type) {
 			found = true;
 			break;
 		}
 	}
-	mutex_unlock(&backlight_dev_list_mutex);
+	spin_unlock_irqrestore(&backlight_dev_list_lock, flags);
 
 	return found;
 }
@@ -409,9 +411,9 @@ void backlight_device_unregister(struct backlight_device *bd)
 	if (!bd)
 		return;
 
-	mutex_lock(&backlight_dev_list_mutex);
+	spin_lock_irq(&backlight_dev_list_lock);
 	list_del(&bd->entry);
-	mutex_unlock(&backlight_dev_list_mutex);
+	spin_unlock_irq(&backlight_dev_list_lock);
 
 #ifdef CONFIG_PMAC_BACKLIGHT
 	mutex_lock(&pmac_backlight_mutex);
-- 
2.1.0

Re: [PATCH RFC 2/4] backlight: use spin-lock to protect device list

[Jani Nikula]

On Wed, 10 Sep 2014, David Herrmann <dh.herrmann@gmail.com> wrote:
> There is really no reason to use a mutex to protect a simple list. Convert
> the list-lock to a simple spinlock instead.
>
> The spin-locks prepare for a backlight_find() helper, which should
> preferably be usable from atomic context. A mutex would prevent that, so
> use an irq-save spinlock instead.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/video/backlight/backlight.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 726c6c6..33b64be 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -16,13 +16,14 @@
>  #include <linux/err.h>
>  #include <linux/fb.h>
>  #include <linux/slab.h>
> +#include <linux/spinlock.h>
>  
>  #ifdef CONFIG_PMAC_BACKLIGHT
>  #include <asm/backlight.h>
>  #endif
>  
>  static LIST_HEAD(backlight_dev_list);
> -static DEFINE_MUTEX(backlight_dev_list_mutex);
> +static DEFINE_SPINLOCK(backlight_dev_list_lock);
>  static BLOCKING_NOTIFIER_HEAD(backlight_notifier);
>  
>  static const char *const backlight_types[] = {
> @@ -369,9 +370,9 @@ struct backlight_device *backlight_device_register(const char *name,
>  	mutex_unlock(&pmac_backlight_mutex);
>  #endif
>  
> -	mutex_lock(&backlight_dev_list_mutex);
> +	spin_lock_irq(&backlight_dev_list_lock);
>  	list_add(&new_bd->entry, &backlight_dev_list);
> -	mutex_unlock(&backlight_dev_list_mutex);
> +	spin_unlock_irq(&backlight_dev_list_lock);
>  
>  	blocking_notifier_call_chain(&backlight_notifier,
>  				     BACKLIGHT_REGISTERED, new_bd);
> @@ -384,15 +385,16 @@ bool backlight_device_registered(enum backlight_type type)
>  {
>  	bool found = false;
>  	struct backlight_device *bd;
> +	unsigned long flags;
>  
> -	mutex_lock(&backlight_dev_list_mutex);
> +	spin_lock_irqsave(&backlight_dev_list_lock, flags);
>  	list_for_each_entry(bd, &backlight_dev_list, entry) {
>  		if (bd->props.type == type) {
>  			found = true;
>  			break;
>  		}
>  	}
> -	mutex_unlock(&backlight_dev_list_mutex);
> +	spin_unlock_irqrestore(&backlight_dev_list_lock, flags);
>  
>  	return found;
>  }
> @@ -409,9 +411,9 @@ void backlight_device_unregister(struct backlight_device *bd)
>  	if (!bd)
>  		return;
>  
> -	mutex_lock(&backlight_dev_list_mutex);
> +	spin_lock_irq(&backlight_dev_list_lock);
>  	list_del(&bd->entry);
> -	mutex_unlock(&backlight_dev_list_mutex);
> +	spin_unlock_irq(&backlight_dev_list_lock);
>  
>  #ifdef CONFIG_PMAC_BACKLIGHT
>  	mutex_lock(&pmac_backlight_mutex);
> -- 
> 2.1.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH RFC 2/4] backlight: use spin-lock to protect device list

[Jani Nikula]

On Wed, 10 Sep 2014, David Herrmann <dh.herrmann@gmail.com> wrote:
> There is really no reason to use a mutex to protect a simple list. Convert
> the list-lock to a simple spinlock instead.
>
> The spin-locks prepare for a backlight_find() helper, which should
> preferably be usable from atomic context. A mutex would prevent that, so
> use an irq-save spinlock instead.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/video/backlight/backlight.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 726c6c6..33b64be 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -16,13 +16,14 @@
>  #include <linux/err.h>
>  #include <linux/fb.h>
>  #include <linux/slab.h>
> +#include <linux/spinlock.h>
>  
>  #ifdef CONFIG_PMAC_BACKLIGHT
>  #include <asm/backlight.h>
>  #endif
>  
>  static LIST_HEAD(backlight_dev_list);
> -static DEFINE_MUTEX(backlight_dev_list_mutex);
> +static DEFINE_SPINLOCK(backlight_dev_list_lock);
>  static BLOCKING_NOTIFIER_HEAD(backlight_notifier);
>  
>  static const char *const backlight_types[] = {
> @@ -369,9 +370,9 @@ struct backlight_device *backlight_device_register(const char *name,
>  	mutex_unlock(&pmac_backlight_mutex);
>  #endif
>  
> -	mutex_lock(&backlight_dev_list_mutex);
> +	spin_lock_irq(&backlight_dev_list_lock);
>  	list_add(&new_bd->entry, &backlight_dev_list);
> -	mutex_unlock(&backlight_dev_list_mutex);
> +	spin_unlock_irq(&backlight_dev_list_lock);
>  
>  	blocking_notifier_call_chain(&backlight_notifier,
>  				     BACKLIGHT_REGISTERED, new_bd);
> @@ -384,15 +385,16 @@ bool backlight_device_registered(enum backlight_type type)
>  {
>  	bool found = false;
>  	struct backlight_device *bd;
> +	unsigned long flags;
>  
> -	mutex_lock(&backlight_dev_list_mutex);
> +	spin_lock_irqsave(&backlight_dev_list_lock, flags);
>  	list_for_each_entry(bd, &backlight_dev_list, entry) {
>  		if (bd->props.type == type) {
>  			found = true;
>  			break;
>  		}
>  	}
> -	mutex_unlock(&backlight_dev_list_mutex);
> +	spin_unlock_irqrestore(&backlight_dev_list_lock, flags);
>  
>  	return found;
>  }
> @@ -409,9 +411,9 @@ void backlight_device_unregister(struct backlight_device *bd)
>  	if (!bd)
>  		return;
>  
> -	mutex_lock(&backlight_dev_list_mutex);
> +	spin_lock_irq(&backlight_dev_list_lock);
>  	list_del(&bd->entry);
> -	mutex_unlock(&backlight_dev_list_mutex);
> +	spin_unlock_irq(&backlight_dev_list_lock);
>  
>  #ifdef CONFIG_PMAC_BACKLIGHT
>  	mutex_lock(&pmac_backlight_mutex);
> -- 
> 2.1.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH RFC 3/4] backlight: add kernel-internal backlight API

[David Herrmann]

So far backlights have only been controlled via sysfs. However, sysfs is
not a proper user-space API for runtime modifications, and never was
intended to provide such. The DRM drivers are now prepared to provide
such a backlight link so user-space can control backlight via DRM
connector properties. This allows us to employ the same access-management
we use for mode-setting.

This patch adds few kernel-internal backlight helpers so we can modify
backlights from within DRM.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/video/backlight/backlight.c | 65 +++++++++++++++++++++++++++++++++++++
 include/linux/backlight.h           | 16 +++++++++
 2 files changed, 81 insertions(+)

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 33b64be..04f323b 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -480,6 +480,71 @@ int backlight_unregister_notifier(struct notifier_block *nb)
 EXPORT_SYMBOL(backlight_unregister_notifier);
 
 /**
+ * backlight_device_lookup - find a backlight device
+ * @name: sysname of the backlight device
+ *
+ * @return Reference to the backlight device, NULL if not found.
+ *
+ * This searches through all registered backlight devices for a device with the
+ * given device name. In case none is found, NULL is returned, otherwise a
+ * new reference to the backlight device is returned. You must drop this
+ * reference via backlight_device_unref() once done.
+ * Note that the devices might get unregistered at any time. You need to lock
+ * around this lookup and inside of your backlight-notifier if you need to know
+ * when a device gets unregistered.
+ *
+ * This function can be safely called from IRQ context.
+ */
+struct backlight_device *backlight_device_lookup(const char *name)
+{
+	struct backlight_device *bd;
+	unsigned long flags;
+	const char *t;
+
+	spin_lock_irqsave(&backlight_dev_list_lock, flags);
+
+	list_for_each_entry(bd, &backlight_dev_list, entry) {
+		t = dev_name(&bd->dev);
+		if (t && !strcmp(t, name))
+			goto out;
+	}
+
+	bd = NULL;
+
+out:
+	spin_unlock_irqrestore(&backlight_dev_list_lock, flags);
+	backlight_device_ref(bd);
+	return bd;
+}
+EXPORT_SYMBOL_GPL(backlight_device_lookup);
+
+/**
+ * backlight_set_brightness - set brightness on a backlight device
+ * @bd: backlight device to operate on
+ * @value: brightness value to set on the device
+ * @reason: backlight-change reason to use for notifications
+ *
+ * This is the in-kernel API equivalent of writing into the 'brightness' sysfs
+ * file. It calls into the underlying backlight driver to change the brightness
+ * value. The value is clamped according to device bounds.
+ * A uevent notification is sent with the reason set to @reason.
+ */
+void backlight_set_brightness(struct backlight_device *bd, unsigned int value,
+			      enum backlight_update_reason reason)
+{
+	mutex_lock(&bd->ops_lock);
+	if (bd->ops) {
+		value = clamp(value, 0U, (unsigned)bd->props.max_brightness);
+		pr_debug("set brightness to %u\n", value);
+		bd->props.brightness = value;
+		backlight_update_status(bd);
+	}
+	mutex_unlock(&bd->ops_lock);
+	backlight_generate_event(bd, reason);
+}
+EXPORT_SYMBOL_GPL(backlight_set_brightness);
+
+/**
  * devm_backlight_device_register - resource managed backlight_device_register()
  * @dev: the device to register
  * @name: the name of the device
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index adb14a8..bcc0dec 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -141,6 +141,22 @@ extern bool backlight_device_registered(enum backlight_type type);
 extern int backlight_register_notifier(struct notifier_block *nb);
 extern int backlight_unregister_notifier(struct notifier_block *nb);
 
+struct backlight_device *backlight_device_lookup(const char *name);
+void backlight_set_brightness(struct backlight_device *bd, unsigned int value,
+			      enum backlight_update_reason reason);
+
+static inline void backlight_device_ref(struct backlight_device *bd)
+{
+	if (bd)
+		get_device(&bd->dev);
+}
+
+static inline void backlight_device_unref(struct backlight_device *bd)
+{
+	if (bd)
+		put_device(&bd->dev);
+}
+
 #define to_backlight_device(obj) container_of(obj, struct backlight_device, dev)
 
 static inline void * bl_get_data(struct backlight_device *bl_dev)
-- 
2.1.0

Re: [PATCH RFC 3/4] backlight: add kernel-internal backlight API

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1389 bytes --]

On Wed, Sep 10, 2014 at 05:54:22PM +0200, David Herrmann wrote:
[...]
> +void backlight_set_brightness(struct backlight_device *bd, unsigned int value,
> +			      enum backlight_update_reason reason)
> +{
> +	mutex_lock(&bd->ops_lock);
> +	if (bd->ops) {
> +		value = clamp(value, 0U, (unsigned)bd->props.max_brightness);

max_brightness should really be unsigned to begin with...

> +		pr_debug("set brightness to %u\n", value);

dev_dbg(&bd->dev, ...)?

> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index adb14a8..bcc0dec 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -141,6 +141,22 @@ extern bool backlight_device_registered(enum backlight_type type);
>  extern int backlight_register_notifier(struct notifier_block *nb);
>  extern int backlight_unregister_notifier(struct notifier_block *nb);
>  
> +struct backlight_device *backlight_device_lookup(const char *name);
> +void backlight_set_brightness(struct backlight_device *bd, unsigned int value,
> +			      enum backlight_update_reason reason);
> +
> +static inline void backlight_device_ref(struct backlight_device *bd)
> +{
> +	if (bd)
> +		get_device(&bd->dev);
> +}

Perhaps for consistency with get_device() this should return bd? That
way you can chain things like so:

	priv->backlight = backlight_device_ref(bd);

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC 3/4] backlight: add kernel-internal backlight API

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1389 bytes --]

On Wed, Sep 10, 2014 at 05:54:22PM +0200, David Herrmann wrote:
[...]
> +void backlight_set_brightness(struct backlight_device *bd, unsigned int value,
> +			      enum backlight_update_reason reason)
> +{
> +	mutex_lock(&bd->ops_lock);
> +	if (bd->ops) {
> +		value = clamp(value, 0U, (unsigned)bd->props.max_brightness);

max_brightness should really be unsigned to begin with...

> +		pr_debug("set brightness to %u\n", value);

dev_dbg(&bd->dev, ...)?

> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index adb14a8..bcc0dec 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -141,6 +141,22 @@ extern bool backlight_device_registered(enum backlight_type type);
>  extern int backlight_register_notifier(struct notifier_block *nb);
>  extern int backlight_unregister_notifier(struct notifier_block *nb);
>  
> +struct backlight_device *backlight_device_lookup(const char *name);
> +void backlight_set_brightness(struct backlight_device *bd, unsigned int value,
> +			      enum backlight_update_reason reason);
> +
> +static inline void backlight_device_ref(struct backlight_device *bd)
> +{
> +	if (bd)
> +		get_device(&bd->dev);
> +}

Perhaps for consistency with get_device() this should return bd? That
way you can chain things like so:

	priv->backlight = backlight_device_ref(bd);

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 3/4] backlight: add kernel-internal backlight API

[David Herrmann]

Hi

On Thu, Sep 11, 2014 at 1:10 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Wed, Sep 10, 2014 at 05:54:22PM +0200, David Herrmann wrote:
> [...]
>> +void backlight_set_brightness(struct backlight_device *bd, unsigned int value,
>> +                           enum backlight_update_reason reason)
>> +{
>> +     mutex_lock(&bd->ops_lock);
>> +     if (bd->ops) {
>> +             value = clamp(value, 0U, (unsigned)bd->props.max_brightness);
>
> max_brightness should really be unsigned to begin with...
>
>> +             pr_debug("set brightness to %u\n", value);
>
> dev_dbg(&bd->dev, ...)?

I agree with both comments, but I tried to be consistent with what
brightness_store() does.

>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>> index adb14a8..bcc0dec 100644
>> --- a/include/linux/backlight.h
>> +++ b/include/linux/backlight.h
>> @@ -141,6 +141,22 @@ extern bool backlight_device_registered(enum backlight_type type);
>>  extern int backlight_register_notifier(struct notifier_block *nb);
>>  extern int backlight_unregister_notifier(struct notifier_block *nb);
>>
>> +struct backlight_device *backlight_device_lookup(const char *name);
>> +void backlight_set_brightness(struct backlight_device *bd, unsigned int value,
>> +                           enum backlight_update_reason reason);
>> +
>> +static inline void backlight_device_ref(struct backlight_device *bd)
>> +{
>> +     if (bd)
>> +             get_device(&bd->dev);
>> +}
>
> Perhaps for consistency with get_device() this should return bd? That
> way you can chain things like so:
>
>         priv->backlight = backlight_device_ref(bd);

Makes sense, will change it. Same is actually true for _unref(), which
should return NULL unconditionally. This way, you can use:
  priv->backlight = backlight_device_unref(priv->backlight);
to release a reference and reset the pointer at the same time.

Thanks
David

Re: [PATCH RFC 3/4] backlight: add kernel-internal backlight API

[David Herrmann]

Hi

On Thu, Sep 11, 2014 at 1:10 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Wed, Sep 10, 2014 at 05:54:22PM +0200, David Herrmann wrote:
> [...]
>> +void backlight_set_brightness(struct backlight_device *bd, unsigned int value,
>> +                           enum backlight_update_reason reason)
>> +{
>> +     mutex_lock(&bd->ops_lock);
>> +     if (bd->ops) {
>> +             value = clamp(value, 0U, (unsigned)bd->props.max_brightness);
>
> max_brightness should really be unsigned to begin with...
>
>> +             pr_debug("set brightness to %u\n", value);
>
> dev_dbg(&bd->dev, ...)?

I agree with both comments, but I tried to be consistent with what
brightness_store() does.

>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>> index adb14a8..bcc0dec 100644
>> --- a/include/linux/backlight.h
>> +++ b/include/linux/backlight.h
>> @@ -141,6 +141,22 @@ extern bool backlight_device_registered(enum backlight_type type);
>>  extern int backlight_register_notifier(struct notifier_block *nb);
>>  extern int backlight_unregister_notifier(struct notifier_block *nb);
>>
>> +struct backlight_device *backlight_device_lookup(const char *name);
>> +void backlight_set_brightness(struct backlight_device *bd, unsigned int value,
>> +                           enum backlight_update_reason reason);
>> +
>> +static inline void backlight_device_ref(struct backlight_device *bd)
>> +{
>> +     if (bd)
>> +             get_device(&bd->dev);
>> +}
>
> Perhaps for consistency with get_device() this should return bd? That
> way you can chain things like so:
>
>         priv->backlight = backlight_device_ref(bd);

Makes sense, will change it. Same is actually true for _unref(), which
should return NULL unconditionally. This way, you can use:
  priv->backlight = backlight_device_unref(priv->backlight);
to release a reference and reset the pointer at the same time.

Thanks
David

Re: [PATCH RFC 3/4] backlight: add kernel-internal backlight API

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2490 bytes --]

On Thu, Sep 11, 2014 at 01:14:31PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Sep 11, 2014 at 1:10 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Wed, Sep 10, 2014 at 05:54:22PM +0200, David Herrmann wrote:
> > [...]
> >> +void backlight_set_brightness(struct backlight_device *bd, unsigned int value,
> >> +                           enum backlight_update_reason reason)
> >> +{
> >> +     mutex_lock(&bd->ops_lock);
> >> +     if (bd->ops) {
> >> +             value = clamp(value, 0U, (unsigned)bd->props.max_brightness);
> >
> > max_brightness should really be unsigned to begin with...
> >
> >> +             pr_debug("set brightness to %u\n", value);
> >
> > dev_dbg(&bd->dev, ...)?
> 
> I agree with both comments, but I tried to be consistent with what
> brightness_store() does.

Fair enough, this can be cleaned up in separate patches.

> >> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> >> index adb14a8..bcc0dec 100644
> >> --- a/include/linux/backlight.h
> >> +++ b/include/linux/backlight.h
> >> @@ -141,6 +141,22 @@ extern bool backlight_device_registered(enum backlight_type type);
> >>  extern int backlight_register_notifier(struct notifier_block *nb);
> >>  extern int backlight_unregister_notifier(struct notifier_block *nb);
> >>
> >> +struct backlight_device *backlight_device_lookup(const char *name);
> >> +void backlight_set_brightness(struct backlight_device *bd, unsigned int value,
> >> +                           enum backlight_update_reason reason);
> >> +
> >> +static inline void backlight_device_ref(struct backlight_device *bd)
> >> +{
> >> +     if (bd)
> >> +             get_device(&bd->dev);
> >> +}
> >
> > Perhaps for consistency with get_device() this should return bd? That
> > way you can chain things like so:
> >
> >         priv->backlight = backlight_device_ref(bd);
> 
> Makes sense, will change it. Same is actually true for _unref(), which
> should return NULL unconditionally. This way, you can use:
>   priv->backlight = backlight_device_unref(priv->backlight);
> to release a reference and reset the pointer at the same time.

That looks somewhat odd to me. Wouldn't priv->backlight typically go
away after the unref anyway (presumably because priv is going to get
freed soon after)?

But I have no strong objections to returning NULL from _unref(), if code
doesn't need it it can always choose not to use the return value.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC 3/4] backlight: add kernel-internal backlight API

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2490 bytes --]

On Thu, Sep 11, 2014 at 01:14:31PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Sep 11, 2014 at 1:10 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Wed, Sep 10, 2014 at 05:54:22PM +0200, David Herrmann wrote:
> > [...]
> >> +void backlight_set_brightness(struct backlight_device *bd, unsigned int value,
> >> +                           enum backlight_update_reason reason)
> >> +{
> >> +     mutex_lock(&bd->ops_lock);
> >> +     if (bd->ops) {
> >> +             value = clamp(value, 0U, (unsigned)bd->props.max_brightness);
> >
> > max_brightness should really be unsigned to begin with...
> >
> >> +             pr_debug("set brightness to %u\n", value);
> >
> > dev_dbg(&bd->dev, ...)?
> 
> I agree with both comments, but I tried to be consistent with what
> brightness_store() does.

Fair enough, this can be cleaned up in separate patches.

> >> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> >> index adb14a8..bcc0dec 100644
> >> --- a/include/linux/backlight.h
> >> +++ b/include/linux/backlight.h
> >> @@ -141,6 +141,22 @@ extern bool backlight_device_registered(enum backlight_type type);
> >>  extern int backlight_register_notifier(struct notifier_block *nb);
> >>  extern int backlight_unregister_notifier(struct notifier_block *nb);
> >>
> >> +struct backlight_device *backlight_device_lookup(const char *name);
> >> +void backlight_set_brightness(struct backlight_device *bd, unsigned int value,
> >> +                           enum backlight_update_reason reason);
> >> +
> >> +static inline void backlight_device_ref(struct backlight_device *bd)
> >> +{
> >> +     if (bd)
> >> +             get_device(&bd->dev);
> >> +}
> >
> > Perhaps for consistency with get_device() this should return bd? That
> > way you can chain things like so:
> >
> >         priv->backlight = backlight_device_ref(bd);
> 
> Makes sense, will change it. Same is actually true for _unref(), which
> should return NULL unconditionally. This way, you can use:
>   priv->backlight = backlight_device_unref(priv->backlight);
> to release a reference and reset the pointer at the same time.

That looks somewhat odd to me. Wouldn't priv->backlight typically go
away after the unref anyway (presumably because priv is going to get
freed soon after)?

But I have no strong objections to returning NULL from _unref(), if code
doesn't need it it can always choose not to use the return value.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC 4/4] drm: link connectors to backlight devices

[David Herrmann]

Backlight devices have always been managed independently of display
controllers. They're often controlled via different hardware interfaces
and their relationship to display-controllers varies vastly between
different boards. However, display brightness is obviously a property of
a display, and thus of a DRM connector. Therefore, it'd be really
appreciated if user-space APIs would highlight this relationship.

The main runtime users of backlight interfaces are user-space compositors.
But currently they have to jump through hoops to find the correct
backlight device for a given connector. Furthermore, they need root
privileges to write to sysfs. sysfs has never been designed as run-time
non-root API. It does not provide file-contexts, run-time management or
any kind of API control. There is no way to control access to sysfs via
different links (in that case: mounts). Char-devs provide all this!

So far, backlight APIs have been fairly trivial, so adding char-devs to
backlights is rather heavy-weight. Therefore, this patch introduces a new
API interface to modify backlight brightness via DRM: A "BRIGHTNESS"
property on DRM connectors.

Instead of adding backlight hardware support to DRM, we rely on the
backlight-class and simply add a new API. Each DRM Connector can
optionally be linked to a backlight class device. Modifying the connector
property will have the same effect as writing into the "brightness" sysfs
file of the linked backlight class device. However, we now can manage
access to backlight devices via the same interface as access to
mode-setting on the underlying display. Furthermore, the connection
between displays and their backlight devices are visible in user-space.

Obviously, matching backlights to displays cannot be solved magically
with this link. Therefore, we also add a user-space attribute to DRM
connectors called 'backlight'. If a DRM driver is incapable of matching
existing backlights to a connector, or if a given board has just crappy
backlight drivers, udev can write the name of a backlight-device into this
attribute and the connector-property will be re-linked to this backlight
device. The udev hwdb can be easily employed to track such quirks and
fixups for different board+GPU combinations.
Note that the name written into the 'backlight' attribute is saved on the
connector, so in case the real backlight device is probed after the DRM
card, the backlight will still get properly attached once probed.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/Kconfig             |   1 +
 drivers/gpu/drm/Makefile            |   2 +-
 drivers/gpu/drm/drm_backlight.c     | 387 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc.c          |  45 +++--
 drivers/gpu/drm/drm_drv.c           |  11 +
 drivers/gpu/drm/drm_sysfs.c         |  53 +++++
 drivers/video/backlight/backlight.c |   3 +
 include/drm/drm_backlight.h         |  44 ++++
 include/drm/drm_crtc.h              |   3 +
 include/linux/backlight.h           |   1 +
 10 files changed, 530 insertions(+), 20 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_backlight.c
 create mode 100644 include/drm/drm_backlight.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index e3b4b0f..46bca34 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -12,6 +12,7 @@ menuconfig DRM
 	select I2C
 	select I2C_ALGOBIT
 	select DMA_SHARED_BUFFER
+	select BACKLIGHT_CLASS_DEVICE
 	help
 	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
 	  introduced in XFree86 4.0. If you say Y here, you need to select
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 9292a76..224544d 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -14,7 +14,7 @@ drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
 		drm_info.o drm_debugfs.o drm_encoder_slave.o \
 		drm_trace_points.o drm_global.o drm_prime.o \
 		drm_rect.o drm_vma_manager.o drm_flip_work.o \
-		drm_modeset_lock.o
+		drm_modeset_lock.o drm_backlight.o
 
 drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
diff --git a/drivers/gpu/drm/drm_backlight.c b/drivers/gpu/drm/drm_backlight.c
new file mode 100644
index 0000000..92d231a
--- /dev/null
+++ b/drivers/gpu/drm/drm_backlight.c
@@ -0,0 +1,387 @@
+/*
+ * DRM Backlight Helpers
+ * Copyright (c) 2014 David Herrmann
+ */
+
+#include <linux/backlight.h>
+#include <linux/fs.h>
+#include <linux/list.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <drm/drmP.h>
+#include <drm/drm_backlight.h>
+
+/**
+ * DOC: Backlight Devices
+ *
+ * Backlight devices have always been managed as a separate subsystem,
+ * independent of DRM. They are usually controlled via separate hardware
+ * interfaces than the display controller, so the split works out fine.
+ * However, backlight brightness is a property of a display, and thus a
+ * property of a DRM connector. We already manage DPMS states via connector
+ * properties, so it is natural to keep brightness control at the same place.
+ *
+ * This DRM backlight interface implements generic backlight properties on
+ * connectors. It does not handle any hardware backends but simply forwards
+ * the requests to an available and linked backlight device. The links between
+ * connectors and backlight devices have to be established by DRM drivers and
+ * can be modified by user-space via sysfs (and udev rules). The name of the
+ * backlight device can be written to a sysfs attribute called 'backlight'.
+ * The device is looked up and linked to the connector (replacing a possible
+ * previous backlight device). A 'change' uevent is sent whenever a link is
+ * modified.
+ *
+ * Drivers have to call drm_backlight_alloc() after allocating a connector via
+ * drm_connector_init(). This will automatically add a backlight device to the
+ * given connector. No hardware device is linked to the connector by default.
+ * Drivers can set up a default device via drm_backlight_set_name(), but are
+ * free to leave it empty. User-space will then have to set up the link.
+ */
+
+struct drm_backlight {
+	struct list_head list;
+	struct drm_connector *connector;
+	char *link_name;
+	struct backlight_device *link;
+	struct work_struct work;
+	unsigned int set_value;
+	bool changed : 1;
+};
+
+static LIST_HEAD(drm_backlight_list);
+static DEFINE_SPINLOCK(drm_backlight_lock);
+
+/* caller must hold @drm_backlight_lock */
+static bool __drm_backlight_is_registered(struct drm_backlight *b)
+{
+	/* a device is live if it is linked to @drm_backlight_list */
+	return !list_empty(&b->list);
+}
+
+/* caller must hold @drm_backlight_lock */
+static void __drm_backlight_schedule(struct drm_backlight *b)
+{
+	if (__drm_backlight_is_registered(b))
+		schedule_work(&b->work);
+}
+
+static void __drm_backlight_worker(struct work_struct *w)
+{
+	struct drm_backlight *b = container_of(w, struct drm_backlight, work);
+	static char *ep[] = { "BACKLIGHT=1", NULL };
+	struct backlight_device *bd;
+	bool send_uevent;
+	unsigned int v;
+
+	spin_lock(&drm_backlight_lock);
+	send_uevent = b->changed;
+	b->changed = false;
+	v = b->set_value;
+	bd = b->link;
+	backlight_device_ref(bd);
+	spin_unlock(&drm_backlight_lock);
+
+	if (bd) {
+		backlight_set_brightness(bd, v, BACKLIGHT_UPDATE_DRM);
+		backlight_device_unref(bd);
+	}
+
+	if (send_uevent)
+		kobject_uevent_env(&b->connector->kdev->kobj, KOBJ_CHANGE, ep);
+}
+
+/* caller must hold @drm_backlight_lock */
+static void __drm_backlight_prop_changed(struct drm_backlight *b, uint64_t v)
+{
+	uint64_t max;
+
+	if (!b->link)
+		return;
+
+	max = b->link->props.max_brightness;
+	if (v >= U16_MAX)
+		b->set_value = max;
+	else
+		b->set_value = (v * max) >> 16;
+	__drm_backlight_schedule(b);
+}
+
+/* caller must hold @drm_backlight_lock */
+static void __drm_backlight_real_changed(struct drm_backlight *b, uint64_t v)
+{
+	struct drm_mode_config *config = &b->connector->dev->mode_config;
+	unsigned int max, set;
+
+	if (!b->link)
+		return;
+
+	set = v;
+	max = b->link->props.max_brightness;
+	if (max < 1)
+		return;
+
+	if (set >= max)
+		set = U16_MAX;
+	else if (max <= U16_MAX)
+		set = (set << 16) / max;
+	else
+		set = div_u64(v << 16, max);
+
+	drm_object_property_set_value(&b->connector->base,
+				      config->brightness_property, v);
+}
+
+/* caller must hold @drm_backlight_lock */
+static void __drm_backlight_link(struct drm_backlight *b,
+				 struct backlight_device *bd)
+{
+	if (bd != b->link) {
+		backlight_device_unref(b->link);
+		b->link = bd;
+		backlight_device_ref(b->link);
+		if (bd)
+			__drm_backlight_real_changed(b, bd->props.brightness);
+		b->changed = true;
+		__drm_backlight_schedule(b);
+	}
+}
+
+/* caller must hold @drm_backlight_lock */
+static void __drm_backlight_lookup(struct drm_backlight *b)
+{
+	struct backlight_device *bd;
+
+	if (b->link_name)
+		bd = backlight_device_lookup(b->link_name);
+	else
+		bd = NULL;
+
+	__drm_backlight_link(b, bd);
+	backlight_device_unref(bd);
+}
+
+/**
+ * drm_backlight_alloc - add backlight capability to a connector
+ * @connector: connector to add backlight to
+ *
+ * This allocates a new DRM-backlight device and links it to @connector. This
+ * *must* be called before registering the connector. The backlight device will
+ * be automatically registered in sync with the connector. It will also get
+ * removed once the connector is removed.
+ *
+ * The connector will not have any hardware backlight linked by default. You
+ * need to call drm_backlight_set_name() if you want to set a default
+ * backlight. User-space can overwrite those via sysfs.
+ *
+ * Returns: 0 on success, negative error code on failure.
+ */
+int drm_backlight_alloc(struct drm_connector *connector)
+{
+	struct drm_mode_config *config = &connector->dev->mode_config;
+	struct drm_backlight *b;
+
+	b = kzalloc(sizeof(*b), GFP_KERNEL);
+	if (!b)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&b->list);
+	INIT_WORK(&b->work, __drm_backlight_worker);
+	b->connector = connector;
+	connector->backlight = b;
+
+	drm_object_attach_property(&connector->base,
+				   config->brightness_property, U16_MAX);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_backlight_alloc);
+
+void drm_backlight_free(struct drm_connector *connector)
+{
+	struct drm_backlight *b = connector->backlight;
+
+	if (b) {
+		WARN_ON(__drm_backlight_is_registered(b));
+		WARN_ON(b->link);
+
+		kfree(b->link_name);
+		kfree(b);
+		connector->backlight = NULL;
+	}
+}
+
+void drm_backlight_register(struct drm_backlight *b)
+{
+	if (!b)
+		return;
+
+	WARN_ON(__drm_backlight_is_registered(b));
+
+	spin_lock(&drm_backlight_lock);
+	list_add(&b->list, &drm_backlight_list);
+	__drm_backlight_lookup(b);
+	spin_unlock(&drm_backlight_lock);
+}
+
+void drm_backlight_unregister(struct drm_backlight *b)
+{
+	if (!b)
+		return;
+
+	WARN_ON(!__drm_backlight_is_registered(b));
+
+	spin_lock(&drm_backlight_lock);
+	list_del_init(&b->list);
+	__drm_backlight_link(b, NULL);
+	spin_unlock(&drm_backlight_lock);
+
+	cancel_work_sync(&b->work);
+}
+
+/**
+ * drm_backlight_get_name - retrieve name of linked backlight device
+ * @b: DRM backlight to retrieve name of
+ * @buf: target buffer for name
+ * @max: size of the target buffer
+ *
+ * This retrieves the name of the backlight device linked to @b and writes it
+ * into @buf. If @buf is NULL or @max is 0, no name will be retrieved, but this
+ * function only tests whether a link is set.
+ * Otherwise, the name will always be written into @buf and will always be
+ * zero-terminated (truncated if too long).
+ *
+ * If no backlight device is linked to @b, this returns -ENOENT. Otherwise, the
+ * length of the written name (excluding the terminating 0 character) is
+ * returned.
+ * Note that if a device name has been set but the underlying backlight device
+ * does not exist, this will still return the linked name. -ENOENT is only
+ * returned if no device name has been set, yet (or has been cleared).
+ *
+ * Returns: On success the length of the written name, on failure a negative
+ *          error code.
+ */
+int drm_backlight_get_name(struct drm_backlight *b, char *buf, size_t max)
+{
+	int r;
+
+	spin_lock(&drm_backlight_lock);
+
+	if (!b->link_name) {
+		r = -ENOENT;
+		goto unlock;
+	}
+
+	r = 0;
+	if (buf && max > 0) {
+		r = strlen(b->link_name);
+		if (r + 1 > max)
+			r = max - 1;
+		buf[r] = 0;
+		memcpy(buf, b->link_name, r);
+	}
+
+unlock:
+	spin_unlock(&drm_backlight_lock);
+	return r;
+}
+EXPORT_SYMBOL(drm_backlight_get_name);
+
+/**
+ * drm_backlight_set_name - Change the device link of a DRM backlight
+ * @b: DRM backlight to modify
+ * @name: name of backlight device
+ *
+ * This changes the backlight device-link on @b to the hardware device with
+ * name @name. @name is stored on the backlight device, even if no such
+ * hardware device is registered, yet. If a backlight device appears later on,
+ * it will be automatically linked to all matching DRM backlight devices. If a
+ * real hardware backlight device already exists with such a name, it is linked
+ * with immediate effect.
+ *
+ * Whenever a real hardware backlight is linked or unlinked from a DRM connector
+ * an uevent with "BACKLIGHT=1" is generated on the connector.
+ *
+ * Returns: 0 on success, negative error code on failure.
+ */
+int drm_backlight_set_name(struct drm_backlight *b, const char *name)
+{
+	char *namecopy;
+
+	if (name && *name) {
+		namecopy = kstrdup(name, GFP_KERNEL);
+		if (!namecopy)
+			return -ENOMEM;
+	} else {
+		namecopy = NULL;
+	}
+
+	spin_lock(&drm_backlight_lock);
+
+	kfree(b->link_name);
+	b->link_name = namecopy;
+	if (__drm_backlight_is_registered(b))
+		__drm_backlight_lookup(b);
+
+	spin_unlock(&drm_backlight_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_backlight_set_name);
+
+void drm_backlight_set_brightness(struct drm_backlight *b, uint64_t value)
+{
+	spin_lock(&drm_backlight_lock);
+	__drm_backlight_prop_changed(b, value);
+	spin_unlock(&drm_backlight_lock);
+}
+
+static int drm_backlight_notify(struct notifier_block *self,
+				unsigned long event, void *data)
+{
+	struct backlight_device *bd = data;
+	struct drm_backlight *b;
+	const char *name;
+
+	spin_lock(&drm_backlight_lock);
+
+	switch (event) {
+	case BACKLIGHT_REGISTERED:
+		name = dev_name(&bd->dev);
+		if (!name)
+			break;
+
+		list_for_each_entry(b, &drm_backlight_list, list)
+			if (!b->link && !strcmp(name, b->link_name))
+				__drm_backlight_link(b, bd);
+
+		break;
+	case BACKLIGHT_UNREGISTERED:
+		list_for_each_entry(b, &drm_backlight_list, list)
+			if (b->link == bd)
+				__drm_backlight_link(b, NULL);
+
+		break;
+	}
+
+	spin_unlock(&drm_backlight_lock);
+
+	return 0;
+}
+
+static struct notifier_block drm_backlight_notifier = {
+	.notifier_call = drm_backlight_notify,
+};
+
+int drm_backlight_init(void)
+{
+	return backlight_register_notifier(&drm_backlight_notifier);
+}
+
+void drm_backlight_exit(void)
+{
+	backlight_unregister_notifier(&drm_backlight_notifier);
+}
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 7d7c1fd..1e8caa3 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -34,6 +34,7 @@
 #include <linux/slab.h>
 #include <linux/export.h>
 #include <drm/drmP.h>
+#include <drm/drm_backlight.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_fourcc.h>
@@ -918,6 +919,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
 	ida_remove(&drm_connector_enum_list[connector->connector_type].ida,
 		   connector->connector_type_id);
 
+	drm_backlight_free(connector);
 	drm_mode_object_put(dev, &connector->base);
 	kfree(connector->name);
 	connector->name = NULL;
@@ -963,6 +965,7 @@ int drm_connector_register(struct drm_connector *connector)
 	int ret;
 
 	drm_mode_object_register(connector->dev, &connector->base);
+	drm_backlight_register(connector->backlight);
 
 	ret = drm_sysfs_connector_add(connector);
 	if (ret)
@@ -986,6 +989,7 @@ EXPORT_SYMBOL(drm_connector_register);
  */
 void drm_connector_unregister(struct drm_connector *connector)
 {
+	drm_backlight_unregister(connector->backlight);
 	drm_sysfs_connector_remove(connector);
 	drm_debugfs_connector_remove(connector);
 }
@@ -1309,28 +1313,25 @@ EXPORT_SYMBOL(drm_plane_force_disable);
 
 static int drm_mode_create_standard_connector_properties(struct drm_device *dev)
 {
-	struct drm_property *edid;
-	struct drm_property *dpms;
-	struct drm_property *dev_path;
+	struct drm_property *p;
 
 	/*
 	 * Standard properties (apply to all connectors)
 	 */
-	edid = drm_property_create(dev, DRM_MODE_PROP_BLOB |
-				   DRM_MODE_PROP_IMMUTABLE,
-				   "EDID", 0);
-	dev->mode_config.edid_property = edid;
-
-	dpms = drm_property_create_enum(dev, 0,
-				   "DPMS", drm_dpms_enum_list,
-				   ARRAY_SIZE(drm_dpms_enum_list));
-	dev->mode_config.dpms_property = dpms;
-
-	dev_path = drm_property_create(dev,
-				       DRM_MODE_PROP_BLOB |
-				       DRM_MODE_PROP_IMMUTABLE,
-				       "PATH", 0);
-	dev->mode_config.path_property = dev_path;
+	p = drm_property_create(dev, DRM_MODE_PROP_BLOB |
+				DRM_MODE_PROP_IMMUTABLE, "EDID", 0);
+	dev->mode_config.edid_property = p;
+
+	p = drm_property_create_enum(dev, 0, "DPMS", drm_dpms_enum_list,
+				     ARRAY_SIZE(drm_dpms_enum_list));
+	dev->mode_config.dpms_property = p;
+
+	p = drm_property_create(dev, DRM_MODE_PROP_BLOB |
+				DRM_MODE_PROP_IMMUTABLE, "PATH", 0);
+	dev->mode_config.path_property = p;
+
+	p = drm_property_create_range(dev, 0, "BRIGHTNESS", 0, U16_MAX);
+	dev->mode_config.brightness_property = p;
 
 	return 0;
 }
@@ -4145,12 +4146,18 @@ static int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
 {
 	int ret = -EINVAL;
 	struct drm_connector *connector = obj_to_connector(obj);
+	struct drm_mode_config *config = &connector->dev->mode_config;
 
 	/* Do DPMS ourselves */
-	if (property == connector->dev->mode_config.dpms_property) {
+	if (property == config->dpms_property) {
 		if (connector->funcs->dpms)
 			(*connector->funcs->dpms)(connector, (int)value);
 		ret = 0;
+	} else if (property == config->brightness_property) {
+		if (connector->backlight)
+			drm_backlight_set_brightness(connector->backlight,
+						     value);
+		ret = 0;
 	} else if (connector->funcs->set_property)
 		ret = connector->funcs->set_property(connector, property, value);
 
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 6645669..76c9401 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -33,6 +33,7 @@
 #include <linux/mount.h>
 #include <linux/slab.h>
 #include <drm/drmP.h>
+#include <drm/drm_backlight.h>
 #include <drm/drm_core.h>
 #include "drm_legacy.h"
 
@@ -890,9 +891,18 @@ static int __init drm_core_init(void)
 		goto err_p3;
 	}
 
+	ret = drm_backlight_init();
+	if (ret < 0) {
+		DRM_ERROR("Cannot initialize backlight interface\n");
+		goto err_p4;
+	}
+
 	DRM_INFO("Initialized %s %d.%d.%d %s\n",
 		 CORE_NAME, CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL, CORE_DATE);
 	return 0;
+
+err_p4:
+	debugfs_remove(drm_debugfs_root);
 err_p3:
 	drm_sysfs_destroy();
 err_p2:
@@ -905,6 +915,7 @@ err_p1:
 
 static void __exit drm_core_exit(void)
 {
+	drm_backlight_exit();
 	debugfs_remove(drm_debugfs_root);
 	drm_sysfs_destroy();
 
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index ab1a5f6..bc5d7f3 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -18,6 +18,7 @@
 #include <linux/err.h>
 #include <linux/export.h>
 
+#include <drm/drm_backlight.h>
 #include <drm/drm_sysfs.h>
 #include <drm/drm_core.h>
 #include <drm/drmP.h>
@@ -256,6 +257,57 @@ static ssize_t modes_show(struct device *device,
 	return written;
 }
 
+static ssize_t backlight_show(struct device *device,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct drm_connector *connector = to_drm_connector(device);
+	int r;
+
+	if (!connector->backlight)
+		return -ENOTSUPP;
+
+	r = drm_backlight_get_name(connector->backlight, buf, PAGE_SIZE);
+	if (r < 0)
+		return r;
+
+	if (r + 1 < PAGE_SIZE) {
+		buf[r++] = '\n';
+		buf[r] = 0;
+	}
+
+	return r;
+}
+
+static ssize_t backlight_store(struct device *device,
+			       struct device_attribute *attr,
+			       const char *buf, size_t size)
+{
+	struct drm_connector *connector = to_drm_connector(device);
+	const char *t;
+	char *name;
+	size_t len;
+	int r;
+
+	if (!connector->backlight)
+		return -ENOTSUPP;
+
+	t = strchrnul(buf, '\n');
+	len = t - buf;
+
+	name = kstrndup(buf, len, GFP_TEMPORARY);
+	if (!name)
+		return -ENOMEM;
+
+	r = drm_backlight_set_name(connector->backlight, name);
+	kfree(name);
+
+	if (r < 0)
+		return r;
+
+	return len;
+}
+
 static ssize_t subconnector_show(struct device *device,
 			   struct device_attribute *attr,
 			   char *buf)
@@ -343,6 +395,7 @@ static struct device_attribute connector_attrs[] = {
 	__ATTR_RO(enabled),
 	__ATTR_RO(dpms),
 	__ATTR_RO(modes),
+	__ATTR_RW(backlight),
 };
 
 /* These attributes are for both DVI-I connectors and all types of tv-out. */
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 04f323b..a1bc533 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -113,6 +113,9 @@ static void backlight_generate_event(struct backlight_device *bd,
 	case BACKLIGHT_UPDATE_HOTKEY:
 		envp[0] = "SOURCE=hotkey";
 		break;
+	case BACKLIGHT_UPDATE_DRM:
+		envp[0] = "SOURCE=drm";
+		break;
 	default:
 		envp[0] = "SOURCE=unknown";
 		break;
diff --git a/include/drm/drm_backlight.h b/include/drm/drm_backlight.h
new file mode 100644
index 0000000..9f0c31b
--- /dev/null
+++ b/include/drm/drm_backlight.h
@@ -0,0 +1,44 @@
+#ifndef __DRM_BACKLIGHT_H__
+#define __DRM_BACKLIGHT_H__
+
+/*
+ * Copyright (c) 2014 David Herrmann <dh.herrmann@gmail.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+
+struct drm_backlight;
+struct drm_connector;
+
+int drm_backlight_init(void);
+void drm_backlight_exit(void);
+
+int drm_backlight_alloc(struct drm_connector *connector);
+void drm_backlight_free(struct drm_connector *connector);
+void drm_backlight_register(struct drm_backlight *b);
+void drm_backlight_unregister(struct drm_backlight *b);
+
+int drm_backlight_get_name(struct drm_backlight *b, char *buf, size_t max);
+int drm_backlight_set_name(struct drm_backlight *b, const char *name);
+void drm_backlight_set_brightness(struct drm_backlight *b, uint64_t value);
+
+#endif /* __DRM_BACKLIGHT_H__ */
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c40070a..ce3b2f0 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -542,6 +542,8 @@ struct drm_connector {
 
 	/* requested DPMS state */
 	int dpms;
+	/* backlight link */
+	struct drm_backlight *backlight;
 
 	void *helper_private;
 
@@ -823,6 +825,7 @@ struct drm_mode_config {
 	struct list_head property_blob_list;
 	struct drm_property *edid_property;
 	struct drm_property *dpms_property;
+	struct drm_property *brightness_property;
 	struct drm_property *path_property;
 	struct drm_property *plane_type_property;
 	struct drm_property *rotation_property;
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index bcc0dec..8615f54 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -31,6 +31,7 @@
 enum backlight_update_reason {
 	BACKLIGHT_UPDATE_HOTKEY,
 	BACKLIGHT_UPDATE_SYSFS,
+	BACKLIGHT_UPDATE_DRM,
 };
 
 enum backlight_type {
-- 
2.1.0

[PATCH RFC 4/4] drm: link connectors to backlight devices

[David Herrmann]

Backlight devices have always been managed independently of display
controllers. They're often controlled via different hardware interfaces
and their relationship to display-controllers varies vastly between
different boards. However, display brightness is obviously a property of
a display, and thus of a DRM connector. Therefore, it'd be really
appreciated if user-space APIs would highlight this relationship.

The main runtime users of backlight interfaces are user-space compositors.
But currently they have to jump through hoops to find the correct
backlight device for a given connector. Furthermore, they need root
privileges to write to sysfs. sysfs has never been designed as run-time
non-root API. It does not provide file-contexts, run-time management or
any kind of API control. There is no way to control access to sysfs via
different links (in that case: mounts). Char-devs provide all this!

So far, backlight APIs have been fairly trivial, so adding char-devs to
backlights is rather heavy-weight. Therefore, this patch introduces a new
API interface to modify backlight brightness via DRM: A "BRIGHTNESS"
property on DRM connectors.

Instead of adding backlight hardware support to DRM, we rely on the
backlight-class and simply add a new API. Each DRM Connector can
optionally be linked to a backlight class device. Modifying the connector
property will have the same effect as writing into the "brightness" sysfs
file of the linked backlight class device. However, we now can manage
access to backlight devices via the same interface as access to
mode-setting on the underlying display. Furthermore, the connection
between displays and their backlight devices are visible in user-space.

Obviously, matching backlights to displays cannot be solved magically
with this link. Therefore, we also add a user-space attribute to DRM
connectors called 'backlight'. If a DRM driver is incapable of matching
existing backlights to a connector, or if a given board has just crappy
backlight drivers, udev can write the name of a backlight-device into this
attribute and the connector-property will be re-linked to this backlight
device. The udev hwdb can be easily employed to track such quirks and
fixups for different board+GPU combinations.
Note that the name written into the 'backlight' attribute is saved on the
connector, so in case the real backlight device is probed after the DRM
card, the backlight will still get properly attached once probed.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/Kconfig             |   1 +
 drivers/gpu/drm/Makefile            |   2 +-
 drivers/gpu/drm/drm_backlight.c     | 387 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc.c          |  45 +++--
 drivers/gpu/drm/drm_drv.c           |  11 +
 drivers/gpu/drm/drm_sysfs.c         |  53 +++++
 drivers/video/backlight/backlight.c |   3 +
 include/drm/drm_backlight.h         |  44 ++++
 include/drm/drm_crtc.h              |   3 +
 include/linux/backlight.h           |   1 +
 10 files changed, 530 insertions(+), 20 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_backlight.c
 create mode 100644 include/drm/drm_backlight.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index e3b4b0f..46bca34 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -12,6 +12,7 @@ menuconfig DRM
 	select I2C
 	select I2C_ALGOBIT
 	select DMA_SHARED_BUFFER
+	select BACKLIGHT_CLASS_DEVICE
 	help
 	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
 	  introduced in XFree86 4.0. If you say Y here, you need to select
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 9292a76..224544d 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -14,7 +14,7 @@ drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
 		drm_info.o drm_debugfs.o drm_encoder_slave.o \
 		drm_trace_points.o drm_global.o drm_prime.o \
 		drm_rect.o drm_vma_manager.o drm_flip_work.o \
-		drm_modeset_lock.o
+		drm_modeset_lock.o drm_backlight.o
 
 drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
diff --git a/drivers/gpu/drm/drm_backlight.c b/drivers/gpu/drm/drm_backlight.c
new file mode 100644
index 0000000..92d231a
--- /dev/null
+++ b/drivers/gpu/drm/drm_backlight.c
@@ -0,0 +1,387 @@
+/*
+ * DRM Backlight Helpers
+ * Copyright (c) 2014 David Herrmann
+ */
+
+#include <linux/backlight.h>
+#include <linux/fs.h>
+#include <linux/list.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <drm/drmP.h>
+#include <drm/drm_backlight.h>
+
+/**
+ * DOC: Backlight Devices
+ *
+ * Backlight devices have always been managed as a separate subsystem,
+ * independent of DRM. They are usually controlled via separate hardware
+ * interfaces than the display controller, so the split works out fine.
+ * However, backlight brightness is a property of a display, and thus a
+ * property of a DRM connector. We already manage DPMS states via connector
+ * properties, so it is natural to keep brightness control at the same place.
+ *
+ * This DRM backlight interface implements generic backlight properties on
+ * connectors. It does not handle any hardware backends but simply forwards
+ * the requests to an available and linked backlight device. The links between
+ * connectors and backlight devices have to be established by DRM drivers and
+ * can be modified by user-space via sysfs (and udev rules). The name of the
+ * backlight device can be written to a sysfs attribute called 'backlight'.
+ * The device is looked up and linked to the connector (replacing a possible
+ * previous backlight device). A 'change' uevent is sent whenever a link is
+ * modified.
+ *
+ * Drivers have to call drm_backlight_alloc() after allocating a connector via
+ * drm_connector_init(). This will automatically add a backlight device to the
+ * given connector. No hardware device is linked to the connector by default.
+ * Drivers can set up a default device via drm_backlight_set_name(), but are
+ * free to leave it empty. User-space will then have to set up the link.
+ */
+
+struct drm_backlight {
+	struct list_head list;
+	struct drm_connector *connector;
+	char *link_name;
+	struct backlight_device *link;
+	struct work_struct work;
+	unsigned int set_value;
+	bool changed : 1;
+};
+
+static LIST_HEAD(drm_backlight_list);
+static DEFINE_SPINLOCK(drm_backlight_lock);
+
+/* caller must hold @drm_backlight_lock */
+static bool __drm_backlight_is_registered(struct drm_backlight *b)
+{
+	/* a device is live if it is linked to @drm_backlight_list */
+	return !list_empty(&b->list);
+}
+
+/* caller must hold @drm_backlight_lock */
+static void __drm_backlight_schedule(struct drm_backlight *b)
+{
+	if (__drm_backlight_is_registered(b))
+		schedule_work(&b->work);
+}
+
+static void __drm_backlight_worker(struct work_struct *w)
+{
+	struct drm_backlight *b = container_of(w, struct drm_backlight, work);
+	static char *ep[] = { "BACKLIGHT=1", NULL };
+	struct backlight_device *bd;
+	bool send_uevent;
+	unsigned int v;
+
+	spin_lock(&drm_backlight_lock);
+	send_uevent = b->changed;
+	b->changed = false;
+	v = b->set_value;
+	bd = b->link;
+	backlight_device_ref(bd);
+	spin_unlock(&drm_backlight_lock);
+
+	if (bd) {
+		backlight_set_brightness(bd, v, BACKLIGHT_UPDATE_DRM);
+		backlight_device_unref(bd);
+	}
+
+	if (send_uevent)
+		kobject_uevent_env(&b->connector->kdev->kobj, KOBJ_CHANGE, ep);
+}
+
+/* caller must hold @drm_backlight_lock */
+static void __drm_backlight_prop_changed(struct drm_backlight *b, uint64_t v)
+{
+	uint64_t max;
+
+	if (!b->link)
+		return;
+
+	max = b->link->props.max_brightness;
+	if (v >= U16_MAX)
+		b->set_value = max;
+	else
+		b->set_value = (v * max) >> 16;
+	__drm_backlight_schedule(b);
+}
+
+/* caller must hold @drm_backlight_lock */
+static void __drm_backlight_real_changed(struct drm_backlight *b, uint64_t v)
+{
+	struct drm_mode_config *config = &b->connector->dev->mode_config;
+	unsigned int max, set;
+
+	if (!b->link)
+		return;
+
+	set = v;
+	max = b->link->props.max_brightness;
+	if (max < 1)
+		return;
+
+	if (set >= max)
+		set = U16_MAX;
+	else if (max <= U16_MAX)
+		set = (set << 16) / max;
+	else
+		set = div_u64(v << 16, max);
+
+	drm_object_property_set_value(&b->connector->base,
+				      config->brightness_property, v);
+}
+
+/* caller must hold @drm_backlight_lock */
+static void __drm_backlight_link(struct drm_backlight *b,
+				 struct backlight_device *bd)
+{
+	if (bd != b->link) {
+		backlight_device_unref(b->link);
+		b->link = bd;
+		backlight_device_ref(b->link);
+		if (bd)
+			__drm_backlight_real_changed(b, bd->props.brightness);
+		b->changed = true;
+		__drm_backlight_schedule(b);
+	}
+}
+
+/* caller must hold @drm_backlight_lock */
+static void __drm_backlight_lookup(struct drm_backlight *b)
+{
+	struct backlight_device *bd;
+
+	if (b->link_name)
+		bd = backlight_device_lookup(b->link_name);
+	else
+		bd = NULL;
+
+	__drm_backlight_link(b, bd);
+	backlight_device_unref(bd);
+}
+
+/**
+ * drm_backlight_alloc - add backlight capability to a connector
+ * @connector: connector to add backlight to
+ *
+ * This allocates a new DRM-backlight device and links it to @connector. This
+ * *must* be called before registering the connector. The backlight device will
+ * be automatically registered in sync with the connector. It will also get
+ * removed once the connector is removed.
+ *
+ * The connector will not have any hardware backlight linked by default. You
+ * need to call drm_backlight_set_name() if you want to set a default
+ * backlight. User-space can overwrite those via sysfs.
+ *
+ * Returns: 0 on success, negative error code on failure.
+ */
+int drm_backlight_alloc(struct drm_connector *connector)
+{
+	struct drm_mode_config *config = &connector->dev->mode_config;
+	struct drm_backlight *b;
+
+	b = kzalloc(sizeof(*b), GFP_KERNEL);
+	if (!b)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&b->list);
+	INIT_WORK(&b->work, __drm_backlight_worker);
+	b->connector = connector;
+	connector->backlight = b;
+
+	drm_object_attach_property(&connector->base,
+				   config->brightness_property, U16_MAX);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_backlight_alloc);
+
+void drm_backlight_free(struct drm_connector *connector)
+{
+	struct drm_backlight *b = connector->backlight;
+
+	if (b) {
+		WARN_ON(__drm_backlight_is_registered(b));
+		WARN_ON(b->link);
+
+		kfree(b->link_name);
+		kfree(b);
+		connector->backlight = NULL;
+	}
+}
+
+void drm_backlight_register(struct drm_backlight *b)
+{
+	if (!b)
+		return;
+
+	WARN_ON(__drm_backlight_is_registered(b));
+
+	spin_lock(&drm_backlight_lock);
+	list_add(&b->list, &drm_backlight_list);
+	__drm_backlight_lookup(b);
+	spin_unlock(&drm_backlight_lock);
+}
+
+void drm_backlight_unregister(struct drm_backlight *b)
+{
+	if (!b)
+		return;
+
+	WARN_ON(!__drm_backlight_is_registered(b));
+
+	spin_lock(&drm_backlight_lock);
+	list_del_init(&b->list);
+	__drm_backlight_link(b, NULL);
+	spin_unlock(&drm_backlight_lock);
+
+	cancel_work_sync(&b->work);
+}
+
+/**
+ * drm_backlight_get_name - retrieve name of linked backlight device
+ * @b: DRM backlight to retrieve name of
+ * @buf: target buffer for name
+ * @max: size of the target buffer
+ *
+ * This retrieves the name of the backlight device linked to @b and writes it
+ * into @buf. If @buf is NULL or @max is 0, no name will be retrieved, but this
+ * function only tests whether a link is set.
+ * Otherwise, the name will always be written into @buf and will always be
+ * zero-terminated (truncated if too long).
+ *
+ * If no backlight device is linked to @b, this returns -ENOENT. Otherwise, the
+ * length of the written name (excluding the terminating 0 character) is
+ * returned.
+ * Note that if a device name has been set but the underlying backlight device
+ * does not exist, this will still return the linked name. -ENOENT is only
+ * returned if no device name has been set, yet (or has been cleared).
+ *
+ * Returns: On success the length of the written name, on failure a negative
+ *          error code.
+ */
+int drm_backlight_get_name(struct drm_backlight *b, char *buf, size_t max)
+{
+	int r;
+
+	spin_lock(&drm_backlight_lock);
+
+	if (!b->link_name) {
+		r = -ENOENT;
+		goto unlock;
+	}
+
+	r = 0;
+	if (buf && max > 0) {
+		r = strlen(b->link_name);
+		if (r + 1 > max)
+			r = max - 1;
+		buf[r] = 0;
+		memcpy(buf, b->link_name, r);
+	}
+
+unlock:
+	spin_unlock(&drm_backlight_lock);
+	return r;
+}
+EXPORT_SYMBOL(drm_backlight_get_name);
+
+/**
+ * drm_backlight_set_name - Change the device link of a DRM backlight
+ * @b: DRM backlight to modify
+ * @name: name of backlight device
+ *
+ * This changes the backlight device-link on @b to the hardware device with
+ * name @name. @name is stored on the backlight device, even if no such
+ * hardware device is registered, yet. If a backlight device appears later on,
+ * it will be automatically linked to all matching DRM backlight devices. If a
+ * real hardware backlight device already exists with such a name, it is linked
+ * with immediate effect.
+ *
+ * Whenever a real hardware backlight is linked or unlinked from a DRM connector
+ * an uevent with "BACKLIGHT=1" is generated on the connector.
+ *
+ * Returns: 0 on success, negative error code on failure.
+ */
+int drm_backlight_set_name(struct drm_backlight *b, const char *name)
+{
+	char *namecopy;
+
+	if (name && *name) {
+		namecopy = kstrdup(name, GFP_KERNEL);
+		if (!namecopy)
+			return -ENOMEM;
+	} else {
+		namecopy = NULL;
+	}
+
+	spin_lock(&drm_backlight_lock);
+
+	kfree(b->link_name);
+	b->link_name = namecopy;
+	if (__drm_backlight_is_registered(b))
+		__drm_backlight_lookup(b);
+
+	spin_unlock(&drm_backlight_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_backlight_set_name);
+
+void drm_backlight_set_brightness(struct drm_backlight *b, uint64_t value)
+{
+	spin_lock(&drm_backlight_lock);
+	__drm_backlight_prop_changed(b, value);
+	spin_unlock(&drm_backlight_lock);
+}
+
+static int drm_backlight_notify(struct notifier_block *self,
+				unsigned long event, void *data)
+{
+	struct backlight_device *bd = data;
+	struct drm_backlight *b;
+	const char *name;
+
+	spin_lock(&drm_backlight_lock);
+
+	switch (event) {
+	case BACKLIGHT_REGISTERED:
+		name = dev_name(&bd->dev);
+		if (!name)
+			break;
+
+		list_for_each_entry(b, &drm_backlight_list, list)
+			if (!b->link && !strcmp(name, b->link_name))
+				__drm_backlight_link(b, bd);
+
+		break;
+	case BACKLIGHT_UNREGISTERED:
+		list_for_each_entry(b, &drm_backlight_list, list)
+			if (b->link == bd)
+				__drm_backlight_link(b, NULL);
+
+		break;
+	}
+
+	spin_unlock(&drm_backlight_lock);
+
+	return 0;
+}
+
+static struct notifier_block drm_backlight_notifier = {
+	.notifier_call = drm_backlight_notify,
+};
+
+int drm_backlight_init(void)
+{
+	return backlight_register_notifier(&drm_backlight_notifier);
+}
+
+void drm_backlight_exit(void)
+{
+	backlight_unregister_notifier(&drm_backlight_notifier);
+}
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 7d7c1fd..1e8caa3 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -34,6 +34,7 @@
 #include <linux/slab.h>
 #include <linux/export.h>
 #include <drm/drmP.h>
+#include <drm/drm_backlight.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_fourcc.h>
@@ -918,6 +919,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
 	ida_remove(&drm_connector_enum_list[connector->connector_type].ida,
 		   connector->connector_type_id);
 
+	drm_backlight_free(connector);
 	drm_mode_object_put(dev, &connector->base);
 	kfree(connector->name);
 	connector->name = NULL;
@@ -963,6 +965,7 @@ int drm_connector_register(struct drm_connector *connector)
 	int ret;
 
 	drm_mode_object_register(connector->dev, &connector->base);
+	drm_backlight_register(connector->backlight);
 
 	ret = drm_sysfs_connector_add(connector);
 	if (ret)
@@ -986,6 +989,7 @@ EXPORT_SYMBOL(drm_connector_register);
  */
 void drm_connector_unregister(struct drm_connector *connector)
 {
+	drm_backlight_unregister(connector->backlight);
 	drm_sysfs_connector_remove(connector);
 	drm_debugfs_connector_remove(connector);
 }
@@ -1309,28 +1313,25 @@ EXPORT_SYMBOL(drm_plane_force_disable);
 
 static int drm_mode_create_standard_connector_properties(struct drm_device *dev)
 {
-	struct drm_property *edid;
-	struct drm_property *dpms;
-	struct drm_property *dev_path;
+	struct drm_property *p;
 
 	/*
 	 * Standard properties (apply to all connectors)
 	 */
-	edid = drm_property_create(dev, DRM_MODE_PROP_BLOB |
-				   DRM_MODE_PROP_IMMUTABLE,
-				   "EDID", 0);
-	dev->mode_config.edid_property = edid;
-
-	dpms = drm_property_create_enum(dev, 0,
-				   "DPMS", drm_dpms_enum_list,
-				   ARRAY_SIZE(drm_dpms_enum_list));
-	dev->mode_config.dpms_property = dpms;
-
-	dev_path = drm_property_create(dev,
-				       DRM_MODE_PROP_BLOB |
-				       DRM_MODE_PROP_IMMUTABLE,
-				       "PATH", 0);
-	dev->mode_config.path_property = dev_path;
+	p = drm_property_create(dev, DRM_MODE_PROP_BLOB |
+				DRM_MODE_PROP_IMMUTABLE, "EDID", 0);
+	dev->mode_config.edid_property = p;
+
+	p = drm_property_create_enum(dev, 0, "DPMS", drm_dpms_enum_list,
+				     ARRAY_SIZE(drm_dpms_enum_list));
+	dev->mode_config.dpms_property = p;
+
+	p = drm_property_create(dev, DRM_MODE_PROP_BLOB |
+				DRM_MODE_PROP_IMMUTABLE, "PATH", 0);
+	dev->mode_config.path_property = p;
+
+	p = drm_property_create_range(dev, 0, "BRIGHTNESS", 0, U16_MAX);
+	dev->mode_config.brightness_property = p;
 
 	return 0;
 }
@@ -4145,12 +4146,18 @@ static int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
 {
 	int ret = -EINVAL;
 	struct drm_connector *connector = obj_to_connector(obj);
+	struct drm_mode_config *config = &connector->dev->mode_config;
 
 	/* Do DPMS ourselves */
-	if (property == connector->dev->mode_config.dpms_property) {
+	if (property == config->dpms_property) {
 		if (connector->funcs->dpms)
 			(*connector->funcs->dpms)(connector, (int)value);
 		ret = 0;
+	} else if (property == config->brightness_property) {
+		if (connector->backlight)
+			drm_backlight_set_brightness(connector->backlight,
+						     value);
+		ret = 0;
 	} else if (connector->funcs->set_property)
 		ret = connector->funcs->set_property(connector, property, value);
 
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 6645669..76c9401 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -33,6 +33,7 @@
 #include <linux/mount.h>
 #include <linux/slab.h>
 #include <drm/drmP.h>
+#include <drm/drm_backlight.h>
 #include <drm/drm_core.h>
 #include "drm_legacy.h"
 
@@ -890,9 +891,18 @@ static int __init drm_core_init(void)
 		goto err_p3;
 	}
 
+	ret = drm_backlight_init();
+	if (ret < 0) {
+		DRM_ERROR("Cannot initialize backlight interface\n");
+		goto err_p4;
+	}
+
 	DRM_INFO("Initialized %s %d.%d.%d %s\n",
 		 CORE_NAME, CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL, CORE_DATE);
 	return 0;
+
+err_p4:
+	debugfs_remove(drm_debugfs_root);
 err_p3:
 	drm_sysfs_destroy();
 err_p2:
@@ -905,6 +915,7 @@ err_p1:
 
 static void __exit drm_core_exit(void)
 {
+	drm_backlight_exit();
 	debugfs_remove(drm_debugfs_root);
 	drm_sysfs_destroy();
 
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index ab1a5f6..bc5d7f3 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -18,6 +18,7 @@
 #include <linux/err.h>
 #include <linux/export.h>
 
+#include <drm/drm_backlight.h>
 #include <drm/drm_sysfs.h>
 #include <drm/drm_core.h>
 #include <drm/drmP.h>
@@ -256,6 +257,57 @@ static ssize_t modes_show(struct device *device,
 	return written;
 }
 
+static ssize_t backlight_show(struct device *device,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct drm_connector *connector = to_drm_connector(device);
+	int r;
+
+	if (!connector->backlight)
+		return -ENOTSUPP;
+
+	r = drm_backlight_get_name(connector->backlight, buf, PAGE_SIZE);
+	if (r < 0)
+		return r;
+
+	if (r + 1 < PAGE_SIZE) {
+		buf[r++] = '\n';
+		buf[r] = 0;
+	}
+
+	return r;
+}
+
+static ssize_t backlight_store(struct device *device,
+			       struct device_attribute *attr,
+			       const char *buf, size_t size)
+{
+	struct drm_connector *connector = to_drm_connector(device);
+	const char *t;
+	char *name;
+	size_t len;
+	int r;
+
+	if (!connector->backlight)
+		return -ENOTSUPP;
+
+	t = strchrnul(buf, '\n');
+	len = t - buf;
+
+	name = kstrndup(buf, len, GFP_TEMPORARY);
+	if (!name)
+		return -ENOMEM;
+
+	r = drm_backlight_set_name(connector->backlight, name);
+	kfree(name);
+
+	if (r < 0)
+		return r;
+
+	return len;
+}
+
 static ssize_t subconnector_show(struct device *device,
 			   struct device_attribute *attr,
 			   char *buf)
@@ -343,6 +395,7 @@ static struct device_attribute connector_attrs[] = {
 	__ATTR_RO(enabled),
 	__ATTR_RO(dpms),
 	__ATTR_RO(modes),
+	__ATTR_RW(backlight),
 };
 
 /* These attributes are for both DVI-I connectors and all types of tv-out. */
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 04f323b..a1bc533 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -113,6 +113,9 @@ static void backlight_generate_event(struct backlight_device *bd,
 	case BACKLIGHT_UPDATE_HOTKEY:
 		envp[0] = "SOURCE=hotkey";
 		break;
+	case BACKLIGHT_UPDATE_DRM:
+		envp[0] = "SOURCE=drm";
+		break;
 	default:
 		envp[0] = "SOURCE=unknown";
 		break;
diff --git a/include/drm/drm_backlight.h b/include/drm/drm_backlight.h
new file mode 100644
index 0000000..9f0c31b
--- /dev/null
+++ b/include/drm/drm_backlight.h
@@ -0,0 +1,44 @@
+#ifndef __DRM_BACKLIGHT_H__
+#define __DRM_BACKLIGHT_H__
+
+/*
+ * Copyright (c) 2014 David Herrmann <dh.herrmann@gmail.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+
+struct drm_backlight;
+struct drm_connector;
+
+int drm_backlight_init(void);
+void drm_backlight_exit(void);
+
+int drm_backlight_alloc(struct drm_connector *connector);
+void drm_backlight_free(struct drm_connector *connector);
+void drm_backlight_register(struct drm_backlight *b);
+void drm_backlight_unregister(struct drm_backlight *b);
+
+int drm_backlight_get_name(struct drm_backlight *b, char *buf, size_t max);
+int drm_backlight_set_name(struct drm_backlight *b, const char *name);
+void drm_backlight_set_brightness(struct drm_backlight *b, uint64_t value);
+
+#endif /* __DRM_BACKLIGHT_H__ */
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c40070a..ce3b2f0 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -542,6 +542,8 @@ struct drm_connector {
 
 	/* requested DPMS state */
 	int dpms;
+	/* backlight link */
+	struct drm_backlight *backlight;
 
 	void *helper_private;
 
@@ -823,6 +825,7 @@ struct drm_mode_config {
 	struct list_head property_blob_list;
 	struct drm_property *edid_property;
 	struct drm_property *dpms_property;
+	struct drm_property *brightness_property;
 	struct drm_property *path_property;
 	struct drm_property *plane_type_property;
 	struct drm_property *rotation_property;
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index bcc0dec..8615f54 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -31,6 +31,7 @@
 enum backlight_update_reason {
 	BACKLIGHT_UPDATE_HOTKEY,
 	BACKLIGHT_UPDATE_SYSFS,
+	BACKLIGHT_UPDATE_DRM,
 };
 
 enum backlight_type {
-- 
2.1.0

Re: [PATCH RFC 4/4] drm: link connectors to backlight devices

[Daniel Vetter]

On Wed, Sep 10, 2014 at 05:54:23PM +0200, David Herrmann wrote:
> Backlight devices have always been managed independently of display
> controllers. They're often controlled via different hardware interfaces
> and their relationship to display-controllers varies vastly between
> different boards. However, display brightness is obviously a property of
> a display, and thus of a DRM connector. Therefore, it'd be really
> appreciated if user-space APIs would highlight this relationship.
> 
> The main runtime users of backlight interfaces are user-space compositors.
> But currently they have to jump through hoops to find the correct
> backlight device for a given connector. Furthermore, they need root
> privileges to write to sysfs. sysfs has never been designed as run-time
> non-root API. It does not provide file-contexts, run-time management or
> any kind of API control. There is no way to control access to sysfs via
> different links (in that case: mounts). Char-devs provide all this!
> 
> So far, backlight APIs have been fairly trivial, so adding char-devs to
> backlights is rather heavy-weight. Therefore, this patch introduces a new
> API interface to modify backlight brightness via DRM: A "BRIGHTNESS"
> property on DRM connectors.
> 
> Instead of adding backlight hardware support to DRM, we rely on the
> backlight-class and simply add a new API. Each DRM Connector can
> optionally be linked to a backlight class device. Modifying the connector
> property will have the same effect as writing into the "brightness" sysfs
> file of the linked backlight class device. However, we now can manage
> access to backlight devices via the same interface as access to
> mode-setting on the underlying display. Furthermore, the connection
> between displays and their backlight devices are visible in user-space.
> 
> Obviously, matching backlights to displays cannot be solved magically
> with this link. Therefore, we also add a user-space attribute to DRM
> connectors called 'backlight'. If a DRM driver is incapable of matching
> existing backlights to a connector, or if a given board has just crappy
> backlight drivers, udev can write the name of a backlight-device into this
> attribute and the connector-property will be re-linked to this backlight
> device. The udev hwdb can be easily employed to track such quirks and
> fixups for different board+GPU combinations.
> Note that the name written into the 'backlight' attribute is saved on the
> connector, so in case the real backlight device is probed after the DRM
> card, the backlight will still get properly attached once probed.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

Nice you skid around all the pitfalls and trapdoors, I guess we've all
been rather blind ;-)

Two high-level comments:
- We also want to forward "bl_power". cros was totally not happy when we
  stopped treating brightness == 0 as completely off (it upsets some
  panels terminally, so there's a vbt lower limit). Instead we expose this
  now through the bl_power knob.

  While at it I think we should expose all the other backlight properties
  too (read-only ofc for actual/max_brightness).

- How does udev match on the drm connector name? They are not terribly
  stable atm, and if you reload your drm driver, or much more likely, have
  two gpus with two drm drivers they change. We probably should change the
  name allocation scheme to be per device instance instead of global
  first. Within a driver probe order is hopefully deterministic on a given
  platform, since even with super dynamic setups (based on dt/acpi) the
  firmware tables should change really.

Cheers, Daniel
> ---
>  drivers/gpu/drm/Kconfig             |   1 +
>  drivers/gpu/drm/Makefile            |   2 +-
>  drivers/gpu/drm/drm_backlight.c     | 387 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc.c          |  45 +++--
>  drivers/gpu/drm/drm_drv.c           |  11 +
>  drivers/gpu/drm/drm_sysfs.c         |  53 +++++
>  drivers/video/backlight/backlight.c |   3 +
>  include/drm/drm_backlight.h         |  44 ++++
>  include/drm/drm_crtc.h              |   3 +
>  include/linux/backlight.h           |   1 +
>  10 files changed, 530 insertions(+), 20 deletions(-)
>  create mode 100644 drivers/gpu/drm/drm_backlight.c
>  create mode 100644 include/drm/drm_backlight.h
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index e3b4b0f..46bca34 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -12,6 +12,7 @@ menuconfig DRM
>  	select I2C
>  	select I2C_ALGOBIT
>  	select DMA_SHARED_BUFFER
> +	select BACKLIGHT_CLASS_DEVICE
>  	help
>  	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
>  	  introduced in XFree86 4.0. If you say Y here, you need to select
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 9292a76..224544d 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -14,7 +14,7 @@ drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
>  		drm_info.o drm_debugfs.o drm_encoder_slave.o \
>  		drm_trace_points.o drm_global.o drm_prime.o \
>  		drm_rect.o drm_vma_manager.o drm_flip_work.o \
> -		drm_modeset_lock.o
> +		drm_modeset_lock.o drm_backlight.o
>  
>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
> diff --git a/drivers/gpu/drm/drm_backlight.c b/drivers/gpu/drm/drm_backlight.c
> new file mode 100644
> index 0000000..92d231a
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_backlight.c
> @@ -0,0 +1,387 @@
> +/*
> + * DRM Backlight Helpers
> + * Copyright (c) 2014 David Herrmann
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/fs.h>
> +#include <linux/list.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/notifier.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <drm/drmP.h>
> +#include <drm/drm_backlight.h>
> +
> +/**
> + * DOC: Backlight Devices
> + *
> + * Backlight devices have always been managed as a separate subsystem,
> + * independent of DRM. They are usually controlled via separate hardware
> + * interfaces than the display controller, so the split works out fine.
> + * However, backlight brightness is a property of a display, and thus a
> + * property of a DRM connector. We already manage DPMS states via connector
> + * properties, so it is natural to keep brightness control at the same place.
> + *
> + * This DRM backlight interface implements generic backlight properties on
> + * connectors. It does not handle any hardware backends but simply forwards
> + * the requests to an available and linked backlight device. The links between
> + * connectors and backlight devices have to be established by DRM drivers and
> + * can be modified by user-space via sysfs (and udev rules). The name of the
> + * backlight device can be written to a sysfs attribute called 'backlight'.
> + * The device is looked up and linked to the connector (replacing a possible
> + * previous backlight device). A 'change' uevent is sent whenever a link is
> + * modified.
> + *
> + * Drivers have to call drm_backlight_alloc() after allocating a connector via
> + * drm_connector_init(). This will automatically add a backlight device to the
> + * given connector. No hardware device is linked to the connector by default.
> + * Drivers can set up a default device via drm_backlight_set_name(), but are
> + * free to leave it empty. User-space will then have to set up the link.
> + */
> +
> +struct drm_backlight {
> +	struct list_head list;
> +	struct drm_connector *connector;
> +	char *link_name;
> +	struct backlight_device *link;
> +	struct work_struct work;
> +	unsigned int set_value;
> +	bool changed : 1;
> +};
> +
> +static LIST_HEAD(drm_backlight_list);
> +static DEFINE_SPINLOCK(drm_backlight_lock);
> +
> +/* caller must hold @drm_backlight_lock */
> +static bool __drm_backlight_is_registered(struct drm_backlight *b)
> +{
> +	/* a device is live if it is linked to @drm_backlight_list */
> +	return !list_empty(&b->list);
> +}
> +
> +/* caller must hold @drm_backlight_lock */
> +static void __drm_backlight_schedule(struct drm_backlight *b)
> +{
> +	if (__drm_backlight_is_registered(b))
> +		schedule_work(&b->work);
> +}
> +
> +static void __drm_backlight_worker(struct work_struct *w)
> +{
> +	struct drm_backlight *b = container_of(w, struct drm_backlight, work);
> +	static char *ep[] = { "BACKLIGHT=1", NULL };
> +	struct backlight_device *bd;
> +	bool send_uevent;
> +	unsigned int v;
> +
> +	spin_lock(&drm_backlight_lock);
> +	send_uevent = b->changed;
> +	b->changed = false;
> +	v = b->set_value;
> +	bd = b->link;
> +	backlight_device_ref(bd);
> +	spin_unlock(&drm_backlight_lock);
> +
> +	if (bd) {
> +		backlight_set_brightness(bd, v, BACKLIGHT_UPDATE_DRM);
> +		backlight_device_unref(bd);
> +	}
> +
> +	if (send_uevent)
> +		kobject_uevent_env(&b->connector->kdev->kobj, KOBJ_CHANGE, ep);
> +}
> +
> +/* caller must hold @drm_backlight_lock */
> +static void __drm_backlight_prop_changed(struct drm_backlight *b, uint64_t v)
> +{
> +	uint64_t max;
> +
> +	if (!b->link)
> +		return;
> +
> +	max = b->link->props.max_brightness;
> +	if (v >= U16_MAX)
> +		b->set_value = max;
> +	else
> +		b->set_value = (v * max) >> 16;
> +	__drm_backlight_schedule(b);
> +}
> +
> +/* caller must hold @drm_backlight_lock */
> +static void __drm_backlight_real_changed(struct drm_backlight *b, uint64_t v)
> +{
> +	struct drm_mode_config *config = &b->connector->dev->mode_config;
> +	unsigned int max, set;
> +
> +	if (!b->link)
> +		return;
> +
> +	set = v;
> +	max = b->link->props.max_brightness;
> +	if (max < 1)
> +		return;
> +
> +	if (set >= max)
> +		set = U16_MAX;
> +	else if (max <= U16_MAX)
> +		set = (set << 16) / max;
> +	else
> +		set = div_u64(v << 16, max);
> +
> +	drm_object_property_set_value(&b->connector->base,
> +				      config->brightness_property, v);
> +}
> +
> +/* caller must hold @drm_backlight_lock */
> +static void __drm_backlight_link(struct drm_backlight *b,
> +				 struct backlight_device *bd)
> +{
> +	if (bd != b->link) {
> +		backlight_device_unref(b->link);
> +		b->link = bd;
> +		backlight_device_ref(b->link);
> +		if (bd)
> +			__drm_backlight_real_changed(b, bd->props.brightness);
> +		b->changed = true;
> +		__drm_backlight_schedule(b);
> +	}
> +}
> +
> +/* caller must hold @drm_backlight_lock */
> +static void __drm_backlight_lookup(struct drm_backlight *b)
> +{
> +	struct backlight_device *bd;
> +
> +	if (b->link_name)
> +		bd = backlight_device_lookup(b->link_name);
> +	else
> +		bd = NULL;
> +
> +	__drm_backlight_link(b, bd);
> +	backlight_device_unref(bd);
> +}
> +
> +/**
> + * drm_backlight_alloc - add backlight capability to a connector
> + * @connector: connector to add backlight to
> + *
> + * This allocates a new DRM-backlight device and links it to @connector. This
> + * *must* be called before registering the connector. The backlight device will
> + * be automatically registered in sync with the connector. It will also get
> + * removed once the connector is removed.
> + *
> + * The connector will not have any hardware backlight linked by default. You
> + * need to call drm_backlight_set_name() if you want to set a default
> + * backlight. User-space can overwrite those via sysfs.
> + *
> + * Returns: 0 on success, negative error code on failure.
> + */
> +int drm_backlight_alloc(struct drm_connector *connector)
> +{
> +	struct drm_mode_config *config = &connector->dev->mode_config;
> +	struct drm_backlight *b;
> +
> +	b = kzalloc(sizeof(*b), GFP_KERNEL);
> +	if (!b)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&b->list);
> +	INIT_WORK(&b->work, __drm_backlight_worker);
> +	b->connector = connector;
> +	connector->backlight = b;
> +
> +	drm_object_attach_property(&connector->base,
> +				   config->brightness_property, U16_MAX);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_backlight_alloc);
> +
> +void drm_backlight_free(struct drm_connector *connector)
> +{
> +	struct drm_backlight *b = connector->backlight;
> +
> +	if (b) {
> +		WARN_ON(__drm_backlight_is_registered(b));
> +		WARN_ON(b->link);
> +
> +		kfree(b->link_name);
> +		kfree(b);
> +		connector->backlight = NULL;
> +	}
> +}
> +
> +void drm_backlight_register(struct drm_backlight *b)
> +{
> +	if (!b)
> +		return;
> +
> +	WARN_ON(__drm_backlight_is_registered(b));
> +
> +	spin_lock(&drm_backlight_lock);
> +	list_add(&b->list, &drm_backlight_list);
> +	__drm_backlight_lookup(b);
> +	spin_unlock(&drm_backlight_lock);
> +}
> +
> +void drm_backlight_unregister(struct drm_backlight *b)
> +{
> +	if (!b)
> +		return;
> +
> +	WARN_ON(!__drm_backlight_is_registered(b));
> +
> +	spin_lock(&drm_backlight_lock);
> +	list_del_init(&b->list);
> +	__drm_backlight_link(b, NULL);
> +	spin_unlock(&drm_backlight_lock);
> +
> +	cancel_work_sync(&b->work);
> +}
> +
> +/**
> + * drm_backlight_get_name - retrieve name of linked backlight device
> + * @b: DRM backlight to retrieve name of
> + * @buf: target buffer for name
> + * @max: size of the target buffer
> + *
> + * This retrieves the name of the backlight device linked to @b and writes it
> + * into @buf. If @buf is NULL or @max is 0, no name will be retrieved, but this
> + * function only tests whether a link is set.
> + * Otherwise, the name will always be written into @buf and will always be
> + * zero-terminated (truncated if too long).
> + *
> + * If no backlight device is linked to @b, this returns -ENOENT. Otherwise, the
> + * length of the written name (excluding the terminating 0 character) is
> + * returned.
> + * Note that if a device name has been set but the underlying backlight device
> + * does not exist, this will still return the linked name. -ENOENT is only
> + * returned if no device name has been set, yet (or has been cleared).
> + *
> + * Returns: On success the length of the written name, on failure a negative
> + *          error code.
> + */
> +int drm_backlight_get_name(struct drm_backlight *b, char *buf, size_t max)
> +{
> +	int r;
> +
> +	spin_lock(&drm_backlight_lock);
> +
> +	if (!b->link_name) {
> +		r = -ENOENT;
> +		goto unlock;
> +	}
> +
> +	r = 0;
> +	if (buf && max > 0) {
> +		r = strlen(b->link_name);
> +		if (r + 1 > max)
> +			r = max - 1;
> +		buf[r] = 0;
> +		memcpy(buf, b->link_name, r);
> +	}
> +
> +unlock:
> +	spin_unlock(&drm_backlight_lock);
> +	return r;
> +}
> +EXPORT_SYMBOL(drm_backlight_get_name);
> +
> +/**
> + * drm_backlight_set_name - Change the device link of a DRM backlight
> + * @b: DRM backlight to modify
> + * @name: name of backlight device
> + *
> + * This changes the backlight device-link on @b to the hardware device with
> + * name @name. @name is stored on the backlight device, even if no such
> + * hardware device is registered, yet. If a backlight device appears later on,
> + * it will be automatically linked to all matching DRM backlight devices. If a
> + * real hardware backlight device already exists with such a name, it is linked
> + * with immediate effect.
> + *
> + * Whenever a real hardware backlight is linked or unlinked from a DRM connector
> + * an uevent with "BACKLIGHT=1" is generated on the connector.
> + *
> + * Returns: 0 on success, negative error code on failure.
> + */
> +int drm_backlight_set_name(struct drm_backlight *b, const char *name)
> +{
> +	char *namecopy;
> +
> +	if (name && *name) {
> +		namecopy = kstrdup(name, GFP_KERNEL);
> +		if (!namecopy)
> +			return -ENOMEM;
> +	} else {
> +		namecopy = NULL;
> +	}
> +
> +	spin_lock(&drm_backlight_lock);
> +
> +	kfree(b->link_name);
> +	b->link_name = namecopy;
> +	if (__drm_backlight_is_registered(b))
> +		__drm_backlight_lookup(b);
> +
> +	spin_unlock(&drm_backlight_lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_backlight_set_name);
> +
> +void drm_backlight_set_brightness(struct drm_backlight *b, uint64_t value)
> +{
> +	spin_lock(&drm_backlight_lock);
> +	__drm_backlight_prop_changed(b, value);
> +	spin_unlock(&drm_backlight_lock);
> +}
> +
> +static int drm_backlight_notify(struct notifier_block *self,
> +				unsigned long event, void *data)
> +{
> +	struct backlight_device *bd = data;
> +	struct drm_backlight *b;
> +	const char *name;
> +
> +	spin_lock(&drm_backlight_lock);
> +
> +	switch (event) {
> +	case BACKLIGHT_REGISTERED:
> +		name = dev_name(&bd->dev);
> +		if (!name)
> +			break;
> +
> +		list_for_each_entry(b, &drm_backlight_list, list)
> +			if (!b->link && !strcmp(name, b->link_name))
> +				__drm_backlight_link(b, bd);
> +
> +		break;
> +	case BACKLIGHT_UNREGISTERED:
> +		list_for_each_entry(b, &drm_backlight_list, list)
> +			if (b->link == bd)
> +				__drm_backlight_link(b, NULL);
> +
> +		break;
> +	}
> +
> +	spin_unlock(&drm_backlight_lock);
> +
> +	return 0;
> +}
> +
> +static struct notifier_block drm_backlight_notifier = {
> +	.notifier_call = drm_backlight_notify,
> +};
> +
> +int drm_backlight_init(void)
> +{
> +	return backlight_register_notifier(&drm_backlight_notifier);
> +}
> +
> +void drm_backlight_exit(void)
> +{
> +	backlight_unregister_notifier(&drm_backlight_notifier);
> +}
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 7d7c1fd..1e8caa3 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -34,6 +34,7 @@
>  #include <linux/slab.h>
>  #include <linux/export.h>
>  #include <drm/drmP.h>
> +#include <drm/drm_backlight.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_fourcc.h>
> @@ -918,6 +919,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  	ida_remove(&drm_connector_enum_list[connector->connector_type].ida,
>  		   connector->connector_type_id);
>  
> +	drm_backlight_free(connector);
>  	drm_mode_object_put(dev, &connector->base);
>  	kfree(connector->name);
>  	connector->name = NULL;
> @@ -963,6 +965,7 @@ int drm_connector_register(struct drm_connector *connector)
>  	int ret;
>  
>  	drm_mode_object_register(connector->dev, &connector->base);
> +	drm_backlight_register(connector->backlight);
>  
>  	ret = drm_sysfs_connector_add(connector);
>  	if (ret)
> @@ -986,6 +989,7 @@ EXPORT_SYMBOL(drm_connector_register);
>   */
>  void drm_connector_unregister(struct drm_connector *connector)
>  {
> +	drm_backlight_unregister(connector->backlight);
>  	drm_sysfs_connector_remove(connector);
>  	drm_debugfs_connector_remove(connector);
>  }
> @@ -1309,28 +1313,25 @@ EXPORT_SYMBOL(drm_plane_force_disable);
>  
>  static int drm_mode_create_standard_connector_properties(struct drm_device *dev)
>  {
> -	struct drm_property *edid;
> -	struct drm_property *dpms;
> -	struct drm_property *dev_path;
> +	struct drm_property *p;
>  
>  	/*
>  	 * Standard properties (apply to all connectors)
>  	 */
> -	edid = drm_property_create(dev, DRM_MODE_PROP_BLOB |
> -				   DRM_MODE_PROP_IMMUTABLE,
> -				   "EDID", 0);
> -	dev->mode_config.edid_property = edid;
> -
> -	dpms = drm_property_create_enum(dev, 0,
> -				   "DPMS", drm_dpms_enum_list,
> -				   ARRAY_SIZE(drm_dpms_enum_list));
> -	dev->mode_config.dpms_property = dpms;
> -
> -	dev_path = drm_property_create(dev,
> -				       DRM_MODE_PROP_BLOB |
> -				       DRM_MODE_PROP_IMMUTABLE,
> -				       "PATH", 0);
> -	dev->mode_config.path_property = dev_path;
> +	p = drm_property_create(dev, DRM_MODE_PROP_BLOB |
> +				DRM_MODE_PROP_IMMUTABLE, "EDID", 0);
> +	dev->mode_config.edid_property = p;
> +
> +	p = drm_property_create_enum(dev, 0, "DPMS", drm_dpms_enum_list,
> +				     ARRAY_SIZE(drm_dpms_enum_list));
> +	dev->mode_config.dpms_property = p;
> +
> +	p = drm_property_create(dev, DRM_MODE_PROP_BLOB |
> +				DRM_MODE_PROP_IMMUTABLE, "PATH", 0);
> +	dev->mode_config.path_property = p;
> +
> +	p = drm_property_create_range(dev, 0, "BRIGHTNESS", 0, U16_MAX);
> +	dev->mode_config.brightness_property = p;
>  
>  	return 0;
>  }
> @@ -4145,12 +4146,18 @@ static int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
>  {
>  	int ret = -EINVAL;
>  	struct drm_connector *connector = obj_to_connector(obj);
> +	struct drm_mode_config *config = &connector->dev->mode_config;
>  
>  	/* Do DPMS ourselves */
> -	if (property == connector->dev->mode_config.dpms_property) {
> +	if (property == config->dpms_property) {
>  		if (connector->funcs->dpms)
>  			(*connector->funcs->dpms)(connector, (int)value);
>  		ret = 0;
> +	} else if (property == config->brightness_property) {
> +		if (connector->backlight)
> +			drm_backlight_set_brightness(connector->backlight,
> +						     value);
> +		ret = 0;
>  	} else if (connector->funcs->set_property)
>  		ret = connector->funcs->set_property(connector, property, value);
>  
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 6645669..76c9401 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -33,6 +33,7 @@
>  #include <linux/mount.h>
>  #include <linux/slab.h>
>  #include <drm/drmP.h>
> +#include <drm/drm_backlight.h>
>  #include <drm/drm_core.h>
>  #include "drm_legacy.h"
>  
> @@ -890,9 +891,18 @@ static int __init drm_core_init(void)
>  		goto err_p3;
>  	}
>  
> +	ret = drm_backlight_init();
> +	if (ret < 0) {
> +		DRM_ERROR("Cannot initialize backlight interface\n");
> +		goto err_p4;
> +	}
> +
>  	DRM_INFO("Initialized %s %d.%d.%d %s\n",
>  		 CORE_NAME, CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL, CORE_DATE);
>  	return 0;
> +
> +err_p4:
> +	debugfs_remove(drm_debugfs_root);
>  err_p3:
>  	drm_sysfs_destroy();
>  err_p2:
> @@ -905,6 +915,7 @@ err_p1:
>  
>  static void __exit drm_core_exit(void)
>  {
> +	drm_backlight_exit();
>  	debugfs_remove(drm_debugfs_root);
>  	drm_sysfs_destroy();
>  
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index ab1a5f6..bc5d7f3 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -18,6 +18,7 @@
>  #include <linux/err.h>
>  #include <linux/export.h>
>  
> +#include <drm/drm_backlight.h>
>  #include <drm/drm_sysfs.h>
>  #include <drm/drm_core.h>
>  #include <drm/drmP.h>
> @@ -256,6 +257,57 @@ static ssize_t modes_show(struct device *device,
>  	return written;
>  }
>  
> +static ssize_t backlight_show(struct device *device,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct drm_connector *connector = to_drm_connector(device);
> +	int r;
> +
> +	if (!connector->backlight)
> +		return -ENOTSUPP;
> +
> +	r = drm_backlight_get_name(connector->backlight, buf, PAGE_SIZE);
> +	if (r < 0)
> +		return r;
> +
> +	if (r + 1 < PAGE_SIZE) {
> +		buf[r++] = '\n';
> +		buf[r] = 0;
> +	}
> +
> +	return r;
> +}
> +
> +static ssize_t backlight_store(struct device *device,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t size)
> +{
> +	struct drm_connector *connector = to_drm_connector(device);
> +	const char *t;
> +	char *name;
> +	size_t len;
> +	int r;
> +
> +	if (!connector->backlight)
> +		return -ENOTSUPP;
> +
> +	t = strchrnul(buf, '\n');
> +	len = t - buf;
> +
> +	name = kstrndup(buf, len, GFP_TEMPORARY);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	r = drm_backlight_set_name(connector->backlight, name);
> +	kfree(name);
> +
> +	if (r < 0)
> +		return r;
> +
> +	return len;
> +}
> +
>  static ssize_t subconnector_show(struct device *device,
>  			   struct device_attribute *attr,
>  			   char *buf)
> @@ -343,6 +395,7 @@ static struct device_attribute connector_attrs[] = {
>  	__ATTR_RO(enabled),
>  	__ATTR_RO(dpms),
>  	__ATTR_RO(modes),
> +	__ATTR_RW(backlight),
>  };
>  
>  /* These attributes are for both DVI-I connectors and all types of tv-out. */
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 04f323b..a1bc533 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -113,6 +113,9 @@ static void backlight_generate_event(struct backlight_device *bd,
>  	case BACKLIGHT_UPDATE_HOTKEY:
>  		envp[0] = "SOURCE=hotkey";
>  		break;
> +	case BACKLIGHT_UPDATE_DRM:
> +		envp[0] = "SOURCE=drm";
> +		break;
>  	default:
>  		envp[0] = "SOURCE=unknown";
>  		break;
> diff --git a/include/drm/drm_backlight.h b/include/drm/drm_backlight.h
> new file mode 100644
> index 0000000..9f0c31b
> --- /dev/null
> +++ b/include/drm/drm_backlight.h
> @@ -0,0 +1,44 @@
> +#ifndef __DRM_BACKLIGHT_H__
> +#define __DRM_BACKLIGHT_H__
> +
> +/*
> + * Copyright (c) 2014 David Herrmann <dh.herrmann@gmail.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +
> +struct drm_backlight;
> +struct drm_connector;
> +
> +int drm_backlight_init(void);
> +void drm_backlight_exit(void);
> +
> +int drm_backlight_alloc(struct drm_connector *connector);
> +void drm_backlight_free(struct drm_connector *connector);
> +void drm_backlight_register(struct drm_backlight *b);
> +void drm_backlight_unregister(struct drm_backlight *b);
> +
> +int drm_backlight_get_name(struct drm_backlight *b, char *buf, size_t max);
> +int drm_backlight_set_name(struct drm_backlight *b, const char *name);
> +void drm_backlight_set_brightness(struct drm_backlight *b, uint64_t value);
> +
> +#endif /* __DRM_BACKLIGHT_H__ */
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index c40070a..ce3b2f0 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -542,6 +542,8 @@ struct drm_connector {
>  
>  	/* requested DPMS state */
>  	int dpms;
> +	/* backlight link */
> +	struct drm_backlight *backlight;
>  
>  	void *helper_private;
>  
> @@ -823,6 +825,7 @@ struct drm_mode_config {
>  	struct list_head property_blob_list;
>  	struct drm_property *edid_property;
>  	struct drm_property *dpms_property;
> +	struct drm_property *brightness_property;
>  	struct drm_property *path_property;
>  	struct drm_property *plane_type_property;
>  	struct drm_property *rotation_property;
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index bcc0dec..8615f54 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -31,6 +31,7 @@
>  enum backlight_update_reason {
>  	BACKLIGHT_UPDATE_HOTKEY,
>  	BACKLIGHT_UPDATE_SYSFS,
> +	BACKLIGHT_UPDATE_DRM,
>  };
>  
>  enum backlight_type {
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH RFC 4/4] drm: link connectors to backlight devices

[Daniel Vetter]

On Wed, Sep 10, 2014 at 05:54:23PM +0200, David Herrmann wrote:
> Backlight devices have always been managed independently of display
> controllers. They're often controlled via different hardware interfaces
> and their relationship to display-controllers varies vastly between
> different boards. However, display brightness is obviously a property of
> a display, and thus of a DRM connector. Therefore, it'd be really
> appreciated if user-space APIs would highlight this relationship.
> 
> The main runtime users of backlight interfaces are user-space compositors.
> But currently they have to jump through hoops to find the correct
> backlight device for a given connector. Furthermore, they need root
> privileges to write to sysfs. sysfs has never been designed as run-time
> non-root API. It does not provide file-contexts, run-time management or
> any kind of API control. There is no way to control access to sysfs via
> different links (in that case: mounts). Char-devs provide all this!
> 
> So far, backlight APIs have been fairly trivial, so adding char-devs to
> backlights is rather heavy-weight. Therefore, this patch introduces a new
> API interface to modify backlight brightness via DRM: A "BRIGHTNESS"
> property on DRM connectors.
> 
> Instead of adding backlight hardware support to DRM, we rely on the
> backlight-class and simply add a new API. Each DRM Connector can
> optionally be linked to a backlight class device. Modifying the connector
> property will have the same effect as writing into the "brightness" sysfs
> file of the linked backlight class device. However, we now can manage
> access to backlight devices via the same interface as access to
> mode-setting on the underlying display. Furthermore, the connection
> between displays and their backlight devices are visible in user-space.
> 
> Obviously, matching backlights to displays cannot be solved magically
> with this link. Therefore, we also add a user-space attribute to DRM
> connectors called 'backlight'. If a DRM driver is incapable of matching
> existing backlights to a connector, or if a given board has just crappy
> backlight drivers, udev can write the name of a backlight-device into this
> attribute and the connector-property will be re-linked to this backlight
> device. The udev hwdb can be easily employed to track such quirks and
> fixups for different board+GPU combinations.
> Note that the name written into the 'backlight' attribute is saved on the
> connector, so in case the real backlight device is probed after the DRM
> card, the backlight will still get properly attached once probed.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

Nice you skid around all the pitfalls and trapdoors, I guess we've all
been rather blind ;-)

Two high-level comments:
- We also want to forward "bl_power". cros was totally not happy when we
  stopped treating brightness == 0 as completely off (it upsets some
  panels terminally, so there's a vbt lower limit). Instead we expose this
  now through the bl_power knob.

  While at it I think we should expose all the other backlight properties
  too (read-only ofc for actual/max_brightness).

- How does udev match on the drm connector name? They are not terribly
  stable atm, and if you reload your drm driver, or much more likely, have
  two gpus with two drm drivers they change. We probably should change the
  name allocation scheme to be per device instance instead of global
  first. Within a driver probe order is hopefully deterministic on a given
  platform, since even with super dynamic setups (based on dt/acpi) the
  firmware tables should change really.

Cheers, Daniel
> ---
>  drivers/gpu/drm/Kconfig             |   1 +
>  drivers/gpu/drm/Makefile            |   2 +-
>  drivers/gpu/drm/drm_backlight.c     | 387 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc.c          |  45 +++--
>  drivers/gpu/drm/drm_drv.c           |  11 +
>  drivers/gpu/drm/drm_sysfs.c         |  53 +++++
>  drivers/video/backlight/backlight.c |   3 +
>  include/drm/drm_backlight.h         |  44 ++++
>  include/drm/drm_crtc.h              |   3 +
>  include/linux/backlight.h           |   1 +
>  10 files changed, 530 insertions(+), 20 deletions(-)
>  create mode 100644 drivers/gpu/drm/drm_backlight.c
>  create mode 100644 include/drm/drm_backlight.h
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index e3b4b0f..46bca34 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -12,6 +12,7 @@ menuconfig DRM
>  	select I2C
>  	select I2C_ALGOBIT
>  	select DMA_SHARED_BUFFER
> +	select BACKLIGHT_CLASS_DEVICE
>  	help
>  	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
>  	  introduced in XFree86 4.0. If you say Y here, you need to select
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 9292a76..224544d 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -14,7 +14,7 @@ drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
>  		drm_info.o drm_debugfs.o drm_encoder_slave.o \
>  		drm_trace_points.o drm_global.o drm_prime.o \
>  		drm_rect.o drm_vma_manager.o drm_flip_work.o \
> -		drm_modeset_lock.o
> +		drm_modeset_lock.o drm_backlight.o
>  
>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
> diff --git a/drivers/gpu/drm/drm_backlight.c b/drivers/gpu/drm/drm_backlight.c
> new file mode 100644
> index 0000000..92d231a
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_backlight.c
> @@ -0,0 +1,387 @@
> +/*
> + * DRM Backlight Helpers
> + * Copyright (c) 2014 David Herrmann
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/fs.h>
> +#include <linux/list.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/notifier.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <drm/drmP.h>
> +#include <drm/drm_backlight.h>
> +
> +/**
> + * DOC: Backlight Devices
> + *
> + * Backlight devices have always been managed as a separate subsystem,
> + * independent of DRM. They are usually controlled via separate hardware
> + * interfaces than the display controller, so the split works out fine.
> + * However, backlight brightness is a property of a display, and thus a
> + * property of a DRM connector. We already manage DPMS states via connector
> + * properties, so it is natural to keep brightness control at the same place.
> + *
> + * This DRM backlight interface implements generic backlight properties on
> + * connectors. It does not handle any hardware backends but simply forwards
> + * the requests to an available and linked backlight device. The links between
> + * connectors and backlight devices have to be established by DRM drivers and
> + * can be modified by user-space via sysfs (and udev rules). The name of the
> + * backlight device can be written to a sysfs attribute called 'backlight'.
> + * The device is looked up and linked to the connector (replacing a possible
> + * previous backlight device). A 'change' uevent is sent whenever a link is
> + * modified.
> + *
> + * Drivers have to call drm_backlight_alloc() after allocating a connector via
> + * drm_connector_init(). This will automatically add a backlight device to the
> + * given connector. No hardware device is linked to the connector by default.
> + * Drivers can set up a default device via drm_backlight_set_name(), but are
> + * free to leave it empty. User-space will then have to set up the link.
> + */
> +
> +struct drm_backlight {
> +	struct list_head list;
> +	struct drm_connector *connector;
> +	char *link_name;
> +	struct backlight_device *link;
> +	struct work_struct work;
> +	unsigned int set_value;
> +	bool changed : 1;
> +};
> +
> +static LIST_HEAD(drm_backlight_list);
> +static DEFINE_SPINLOCK(drm_backlight_lock);
> +
> +/* caller must hold @drm_backlight_lock */
> +static bool __drm_backlight_is_registered(struct drm_backlight *b)
> +{
> +	/* a device is live if it is linked to @drm_backlight_list */
> +	return !list_empty(&b->list);
> +}
> +
> +/* caller must hold @drm_backlight_lock */
> +static void __drm_backlight_schedule(struct drm_backlight *b)
> +{
> +	if (__drm_backlight_is_registered(b))
> +		schedule_work(&b->work);
> +}
> +
> +static void __drm_backlight_worker(struct work_struct *w)
> +{
> +	struct drm_backlight *b = container_of(w, struct drm_backlight, work);
> +	static char *ep[] = { "BACKLIGHT=1", NULL };
> +	struct backlight_device *bd;
> +	bool send_uevent;
> +	unsigned int v;
> +
> +	spin_lock(&drm_backlight_lock);
> +	send_uevent = b->changed;
> +	b->changed = false;
> +	v = b->set_value;
> +	bd = b->link;
> +	backlight_device_ref(bd);
> +	spin_unlock(&drm_backlight_lock);
> +
> +	if (bd) {
> +		backlight_set_brightness(bd, v, BACKLIGHT_UPDATE_DRM);
> +		backlight_device_unref(bd);
> +	}
> +
> +	if (send_uevent)
> +		kobject_uevent_env(&b->connector->kdev->kobj, KOBJ_CHANGE, ep);
> +}
> +
> +/* caller must hold @drm_backlight_lock */
> +static void __drm_backlight_prop_changed(struct drm_backlight *b, uint64_t v)
> +{
> +	uint64_t max;
> +
> +	if (!b->link)
> +		return;
> +
> +	max = b->link->props.max_brightness;
> +	if (v >= U16_MAX)
> +		b->set_value = max;
> +	else
> +		b->set_value = (v * max) >> 16;
> +	__drm_backlight_schedule(b);
> +}
> +
> +/* caller must hold @drm_backlight_lock */
> +static void __drm_backlight_real_changed(struct drm_backlight *b, uint64_t v)
> +{
> +	struct drm_mode_config *config = &b->connector->dev->mode_config;
> +	unsigned int max, set;
> +
> +	if (!b->link)
> +		return;
> +
> +	set = v;
> +	max = b->link->props.max_brightness;
> +	if (max < 1)
> +		return;
> +
> +	if (set >= max)
> +		set = U16_MAX;
> +	else if (max <= U16_MAX)
> +		set = (set << 16) / max;
> +	else
> +		set = div_u64(v << 16, max);
> +
> +	drm_object_property_set_value(&b->connector->base,
> +				      config->brightness_property, v);
> +}
> +
> +/* caller must hold @drm_backlight_lock */
> +static void __drm_backlight_link(struct drm_backlight *b,
> +				 struct backlight_device *bd)
> +{
> +	if (bd != b->link) {
> +		backlight_device_unref(b->link);
> +		b->link = bd;
> +		backlight_device_ref(b->link);
> +		if (bd)
> +			__drm_backlight_real_changed(b, bd->props.brightness);
> +		b->changed = true;
> +		__drm_backlight_schedule(b);
> +	}
> +}
> +
> +/* caller must hold @drm_backlight_lock */
> +static void __drm_backlight_lookup(struct drm_backlight *b)
> +{
> +	struct backlight_device *bd;
> +
> +	if (b->link_name)
> +		bd = backlight_device_lookup(b->link_name);
> +	else
> +		bd = NULL;
> +
> +	__drm_backlight_link(b, bd);
> +	backlight_device_unref(bd);
> +}
> +
> +/**
> + * drm_backlight_alloc - add backlight capability to a connector
> + * @connector: connector to add backlight to
> + *
> + * This allocates a new DRM-backlight device and links it to @connector. This
> + * *must* be called before registering the connector. The backlight device will
> + * be automatically registered in sync with the connector. It will also get
> + * removed once the connector is removed.
> + *
> + * The connector will not have any hardware backlight linked by default. You
> + * need to call drm_backlight_set_name() if you want to set a default
> + * backlight. User-space can overwrite those via sysfs.
> + *
> + * Returns: 0 on success, negative error code on failure.
> + */
> +int drm_backlight_alloc(struct drm_connector *connector)
> +{
> +	struct drm_mode_config *config = &connector->dev->mode_config;
> +	struct drm_backlight *b;
> +
> +	b = kzalloc(sizeof(*b), GFP_KERNEL);
> +	if (!b)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&b->list);
> +	INIT_WORK(&b->work, __drm_backlight_worker);
> +	b->connector = connector;
> +	connector->backlight = b;
> +
> +	drm_object_attach_property(&connector->base,
> +				   config->brightness_property, U16_MAX);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_backlight_alloc);
> +
> +void drm_backlight_free(struct drm_connector *connector)
> +{
> +	struct drm_backlight *b = connector->backlight;
> +
> +	if (b) {
> +		WARN_ON(__drm_backlight_is_registered(b));
> +		WARN_ON(b->link);
> +
> +		kfree(b->link_name);
> +		kfree(b);
> +		connector->backlight = NULL;
> +	}
> +}
> +
> +void drm_backlight_register(struct drm_backlight *b)
> +{
> +	if (!b)
> +		return;
> +
> +	WARN_ON(__drm_backlight_is_registered(b));
> +
> +	spin_lock(&drm_backlight_lock);
> +	list_add(&b->list, &drm_backlight_list);
> +	__drm_backlight_lookup(b);
> +	spin_unlock(&drm_backlight_lock);
> +}
> +
> +void drm_backlight_unregister(struct drm_backlight *b)
> +{
> +	if (!b)
> +		return;
> +
> +	WARN_ON(!__drm_backlight_is_registered(b));
> +
> +	spin_lock(&drm_backlight_lock);
> +	list_del_init(&b->list);
> +	__drm_backlight_link(b, NULL);
> +	spin_unlock(&drm_backlight_lock);
> +
> +	cancel_work_sync(&b->work);
> +}
> +
> +/**
> + * drm_backlight_get_name - retrieve name of linked backlight device
> + * @b: DRM backlight to retrieve name of
> + * @buf: target buffer for name
> + * @max: size of the target buffer
> + *
> + * This retrieves the name of the backlight device linked to @b and writes it
> + * into @buf. If @buf is NULL or @max is 0, no name will be retrieved, but this
> + * function only tests whether a link is set.
> + * Otherwise, the name will always be written into @buf and will always be
> + * zero-terminated (truncated if too long).
> + *
> + * If no backlight device is linked to @b, this returns -ENOENT. Otherwise, the
> + * length of the written name (excluding the terminating 0 character) is
> + * returned.
> + * Note that if a device name has been set but the underlying backlight device
> + * does not exist, this will still return the linked name. -ENOENT is only
> + * returned if no device name has been set, yet (or has been cleared).
> + *
> + * Returns: On success the length of the written name, on failure a negative
> + *          error code.
> + */
> +int drm_backlight_get_name(struct drm_backlight *b, char *buf, size_t max)
> +{
> +	int r;
> +
> +	spin_lock(&drm_backlight_lock);
> +
> +	if (!b->link_name) {
> +		r = -ENOENT;
> +		goto unlock;
> +	}
> +
> +	r = 0;
> +	if (buf && max > 0) {
> +		r = strlen(b->link_name);
> +		if (r + 1 > max)
> +			r = max - 1;
> +		buf[r] = 0;
> +		memcpy(buf, b->link_name, r);
> +	}
> +
> +unlock:
> +	spin_unlock(&drm_backlight_lock);
> +	return r;
> +}
> +EXPORT_SYMBOL(drm_backlight_get_name);
> +
> +/**
> + * drm_backlight_set_name - Change the device link of a DRM backlight
> + * @b: DRM backlight to modify
> + * @name: name of backlight device
> + *
> + * This changes the backlight device-link on @b to the hardware device with
> + * name @name. @name is stored on the backlight device, even if no such
> + * hardware device is registered, yet. If a backlight device appears later on,
> + * it will be automatically linked to all matching DRM backlight devices. If a
> + * real hardware backlight device already exists with such a name, it is linked
> + * with immediate effect.
> + *
> + * Whenever a real hardware backlight is linked or unlinked from a DRM connector
> + * an uevent with "BACKLIGHT=1" is generated on the connector.
> + *
> + * Returns: 0 on success, negative error code on failure.
> + */
> +int drm_backlight_set_name(struct drm_backlight *b, const char *name)
> +{
> +	char *namecopy;
> +
> +	if (name && *name) {
> +		namecopy = kstrdup(name, GFP_KERNEL);
> +		if (!namecopy)
> +			return -ENOMEM;
> +	} else {
> +		namecopy = NULL;
> +	}
> +
> +	spin_lock(&drm_backlight_lock);
> +
> +	kfree(b->link_name);
> +	b->link_name = namecopy;
> +	if (__drm_backlight_is_registered(b))
> +		__drm_backlight_lookup(b);
> +
> +	spin_unlock(&drm_backlight_lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_backlight_set_name);
> +
> +void drm_backlight_set_brightness(struct drm_backlight *b, uint64_t value)
> +{
> +	spin_lock(&drm_backlight_lock);
> +	__drm_backlight_prop_changed(b, value);
> +	spin_unlock(&drm_backlight_lock);
> +}
> +
> +static int drm_backlight_notify(struct notifier_block *self,
> +				unsigned long event, void *data)
> +{
> +	struct backlight_device *bd = data;
> +	struct drm_backlight *b;
> +	const char *name;
> +
> +	spin_lock(&drm_backlight_lock);
> +
> +	switch (event) {
> +	case BACKLIGHT_REGISTERED:
> +		name = dev_name(&bd->dev);
> +		if (!name)
> +			break;
> +
> +		list_for_each_entry(b, &drm_backlight_list, list)
> +			if (!b->link && !strcmp(name, b->link_name))
> +				__drm_backlight_link(b, bd);
> +
> +		break;
> +	case BACKLIGHT_UNREGISTERED:
> +		list_for_each_entry(b, &drm_backlight_list, list)
> +			if (b->link == bd)
> +				__drm_backlight_link(b, NULL);
> +
> +		break;
> +	}
> +
> +	spin_unlock(&drm_backlight_lock);
> +
> +	return 0;
> +}
> +
> +static struct notifier_block drm_backlight_notifier = {
> +	.notifier_call = drm_backlight_notify,
> +};
> +
> +int drm_backlight_init(void)
> +{
> +	return backlight_register_notifier(&drm_backlight_notifier);
> +}
> +
> +void drm_backlight_exit(void)
> +{
> +	backlight_unregister_notifier(&drm_backlight_notifier);
> +}
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 7d7c1fd..1e8caa3 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -34,6 +34,7 @@
>  #include <linux/slab.h>
>  #include <linux/export.h>
>  #include <drm/drmP.h>
> +#include <drm/drm_backlight.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_fourcc.h>
> @@ -918,6 +919,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  	ida_remove(&drm_connector_enum_list[connector->connector_type].ida,
>  		   connector->connector_type_id);
>  
> +	drm_backlight_free(connector);
>  	drm_mode_object_put(dev, &connector->base);
>  	kfree(connector->name);
>  	connector->name = NULL;
> @@ -963,6 +965,7 @@ int drm_connector_register(struct drm_connector *connector)
>  	int ret;
>  
>  	drm_mode_object_register(connector->dev, &connector->base);
> +	drm_backlight_register(connector->backlight);
>  
>  	ret = drm_sysfs_connector_add(connector);
>  	if (ret)
> @@ -986,6 +989,7 @@ EXPORT_SYMBOL(drm_connector_register);
>   */
>  void drm_connector_unregister(struct drm_connector *connector)
>  {
> +	drm_backlight_unregister(connector->backlight);
>  	drm_sysfs_connector_remove(connector);
>  	drm_debugfs_connector_remove(connector);
>  }
> @@ -1309,28 +1313,25 @@ EXPORT_SYMBOL(drm_plane_force_disable);
>  
>  static int drm_mode_create_standard_connector_properties(struct drm_device *dev)
>  {
> -	struct drm_property *edid;
> -	struct drm_property *dpms;
> -	struct drm_property *dev_path;
> +	struct drm_property *p;
>  
>  	/*
>  	 * Standard properties (apply to all connectors)
>  	 */
> -	edid = drm_property_create(dev, DRM_MODE_PROP_BLOB |
> -				   DRM_MODE_PROP_IMMUTABLE,
> -				   "EDID", 0);
> -	dev->mode_config.edid_property = edid;
> -
> -	dpms = drm_property_create_enum(dev, 0,
> -				   "DPMS", drm_dpms_enum_list,
> -				   ARRAY_SIZE(drm_dpms_enum_list));
> -	dev->mode_config.dpms_property = dpms;
> -
> -	dev_path = drm_property_create(dev,
> -				       DRM_MODE_PROP_BLOB |
> -				       DRM_MODE_PROP_IMMUTABLE,
> -				       "PATH", 0);
> -	dev->mode_config.path_property = dev_path;
> +	p = drm_property_create(dev, DRM_MODE_PROP_BLOB |
> +				DRM_MODE_PROP_IMMUTABLE, "EDID", 0);
> +	dev->mode_config.edid_property = p;
> +
> +	p = drm_property_create_enum(dev, 0, "DPMS", drm_dpms_enum_list,
> +				     ARRAY_SIZE(drm_dpms_enum_list));
> +	dev->mode_config.dpms_property = p;
> +
> +	p = drm_property_create(dev, DRM_MODE_PROP_BLOB |
> +				DRM_MODE_PROP_IMMUTABLE, "PATH", 0);
> +	dev->mode_config.path_property = p;
> +
> +	p = drm_property_create_range(dev, 0, "BRIGHTNESS", 0, U16_MAX);
> +	dev->mode_config.brightness_property = p;
>  
>  	return 0;
>  }
> @@ -4145,12 +4146,18 @@ static int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
>  {
>  	int ret = -EINVAL;
>  	struct drm_connector *connector = obj_to_connector(obj);
> +	struct drm_mode_config *config = &connector->dev->mode_config;
>  
>  	/* Do DPMS ourselves */
> -	if (property == connector->dev->mode_config.dpms_property) {
> +	if (property == config->dpms_property) {
>  		if (connector->funcs->dpms)
>  			(*connector->funcs->dpms)(connector, (int)value);
>  		ret = 0;
> +	} else if (property == config->brightness_property) {
> +		if (connector->backlight)
> +			drm_backlight_set_brightness(connector->backlight,
> +						     value);
> +		ret = 0;
>  	} else if (connector->funcs->set_property)
>  		ret = connector->funcs->set_property(connector, property, value);
>  
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 6645669..76c9401 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -33,6 +33,7 @@
>  #include <linux/mount.h>
>  #include <linux/slab.h>
>  #include <drm/drmP.h>
> +#include <drm/drm_backlight.h>
>  #include <drm/drm_core.h>
>  #include "drm_legacy.h"
>  
> @@ -890,9 +891,18 @@ static int __init drm_core_init(void)
>  		goto err_p3;
>  	}
>  
> +	ret = drm_backlight_init();
> +	if (ret < 0) {
> +		DRM_ERROR("Cannot initialize backlight interface\n");
> +		goto err_p4;
> +	}
> +
>  	DRM_INFO("Initialized %s %d.%d.%d %s\n",
>  		 CORE_NAME, CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL, CORE_DATE);
>  	return 0;
> +
> +err_p4:
> +	debugfs_remove(drm_debugfs_root);
>  err_p3:
>  	drm_sysfs_destroy();
>  err_p2:
> @@ -905,6 +915,7 @@ err_p1:
>  
>  static void __exit drm_core_exit(void)
>  {
> +	drm_backlight_exit();
>  	debugfs_remove(drm_debugfs_root);
>  	drm_sysfs_destroy();
>  
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index ab1a5f6..bc5d7f3 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -18,6 +18,7 @@
>  #include <linux/err.h>
>  #include <linux/export.h>
>  
> +#include <drm/drm_backlight.h>
>  #include <drm/drm_sysfs.h>
>  #include <drm/drm_core.h>
>  #include <drm/drmP.h>
> @@ -256,6 +257,57 @@ static ssize_t modes_show(struct device *device,
>  	return written;
>  }
>  
> +static ssize_t backlight_show(struct device *device,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct drm_connector *connector = to_drm_connector(device);
> +	int r;
> +
> +	if (!connector->backlight)
> +		return -ENOTSUPP;
> +
> +	r = drm_backlight_get_name(connector->backlight, buf, PAGE_SIZE);
> +	if (r < 0)
> +		return r;
> +
> +	if (r + 1 < PAGE_SIZE) {
> +		buf[r++] = '\n';
> +		buf[r] = 0;
> +	}
> +
> +	return r;
> +}
> +
> +static ssize_t backlight_store(struct device *device,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t size)
> +{
> +	struct drm_connector *connector = to_drm_connector(device);
> +	const char *t;
> +	char *name;
> +	size_t len;
> +	int r;
> +
> +	if (!connector->backlight)
> +		return -ENOTSUPP;
> +
> +	t = strchrnul(buf, '\n');
> +	len = t - buf;
> +
> +	name = kstrndup(buf, len, GFP_TEMPORARY);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	r = drm_backlight_set_name(connector->backlight, name);
> +	kfree(name);
> +
> +	if (r < 0)
> +		return r;
> +
> +	return len;
> +}
> +
>  static ssize_t subconnector_show(struct device *device,
>  			   struct device_attribute *attr,
>  			   char *buf)
> @@ -343,6 +395,7 @@ static struct device_attribute connector_attrs[] = {
>  	__ATTR_RO(enabled),
>  	__ATTR_RO(dpms),
>  	__ATTR_RO(modes),
> +	__ATTR_RW(backlight),
>  };
>  
>  /* These attributes are for both DVI-I connectors and all types of tv-out. */
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 04f323b..a1bc533 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -113,6 +113,9 @@ static void backlight_generate_event(struct backlight_device *bd,
>  	case BACKLIGHT_UPDATE_HOTKEY:
>  		envp[0] = "SOURCE=hotkey";
>  		break;
> +	case BACKLIGHT_UPDATE_DRM:
> +		envp[0] = "SOURCE=drm";
> +		break;
>  	default:
>  		envp[0] = "SOURCE=unknown";
>  		break;
> diff --git a/include/drm/drm_backlight.h b/include/drm/drm_backlight.h
> new file mode 100644
> index 0000000..9f0c31b
> --- /dev/null
> +++ b/include/drm/drm_backlight.h
> @@ -0,0 +1,44 @@
> +#ifndef __DRM_BACKLIGHT_H__
> +#define __DRM_BACKLIGHT_H__
> +
> +/*
> + * Copyright (c) 2014 David Herrmann <dh.herrmann@gmail.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +
> +struct drm_backlight;
> +struct drm_connector;
> +
> +int drm_backlight_init(void);
> +void drm_backlight_exit(void);
> +
> +int drm_backlight_alloc(struct drm_connector *connector);
> +void drm_backlight_free(struct drm_connector *connector);
> +void drm_backlight_register(struct drm_backlight *b);
> +void drm_backlight_unregister(struct drm_backlight *b);
> +
> +int drm_backlight_get_name(struct drm_backlight *b, char *buf, size_t max);
> +int drm_backlight_set_name(struct drm_backlight *b, const char *name);
> +void drm_backlight_set_brightness(struct drm_backlight *b, uint64_t value);
> +
> +#endif /* __DRM_BACKLIGHT_H__ */
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index c40070a..ce3b2f0 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -542,6 +542,8 @@ struct drm_connector {
>  
>  	/* requested DPMS state */
>  	int dpms;
> +	/* backlight link */
> +	struct drm_backlight *backlight;
>  
>  	void *helper_private;
>  
> @@ -823,6 +825,7 @@ struct drm_mode_config {
>  	struct list_head property_blob_list;
>  	struct drm_property *edid_property;
>  	struct drm_property *dpms_property;
> +	struct drm_property *brightness_property;
>  	struct drm_property *path_property;
>  	struct drm_property *plane_type_property;
>  	struct drm_property *rotation_property;
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index bcc0dec..8615f54 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -31,6 +31,7 @@
>  enum backlight_update_reason {
>  	BACKLIGHT_UPDATE_HOTKEY,
>  	BACKLIGHT_UPDATE_SYSFS,
> +	BACKLIGHT_UPDATE_DRM,
>  };
>  
>  enum backlight_type {
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH RFC 4/4] drm: link connectors to backlight devices

[David Herrmann]

Hi

On Thu, Sep 11, 2014 at 8:48 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> Nice you skid around all the pitfalls and trapdoors, I guess we've all
> been rather blind ;-)
>
> Two high-level comments:
> - We also want to forward "bl_power". cros was totally not happy when we
>   stopped treating brightness == 0 as completely off (it upsets some
>   panels terminally, so there's a vbt lower limit). Instead we expose this
>   now through the bl_power knob.
>
>   While at it I think we should expose all the other backlight properties
>   too (read-only ofc for actual/max_brightness).

bl_power is easy to add. I guess v2 will have:
  "BACKLIGHT-POWER" (range 0-4)

actual-brightness is a bit more tricky. Currently, DRM caches property
values, so there is no read_property() hook. We'd have to add this.
But it'll be quite nasty as we have to call into the backlight driver.
So I think we want to run an async-interruptible worker on the
backlight, drop the locks in the ioctl and wait for the job to finish.
Not sure whether it's worth it.. maybe we can add this later.

> - How does udev match on the drm connector name? They are not terribly
>   stable atm, and if you reload your drm driver, or much more likely, have
>   two gpus with two drm drivers they change. We probably should change the
>   name allocation scheme to be per device instance instead of global
>   first. Within a driver probe order is hopefully deterministic on a given
>   platform, since even with super dynamic setups (based on dt/acpi) the
>   firmware tables should change really.

You can match on EDID attributes. Ok, so far this is pretty ugly as
the EDID property is binary. But we can add rather trivial udev
extensions to make EDID binary against text matching possible.

While we're at it, I don't really like the brightness-value
re-scaling. I currently expose BRIGHTNESS as rang 0-65535 and scale it
to the backlight range. This works perfectly well as the backlight is
usually a really small range, but it would be much simpler if we could
expose the real range. However, this would require DRM property
hotplugging. This is currently not supported by DRM.. and it would
require multiple different properties for each connector as each might
have a different range. But then, we have to suffix the name as we
cannot have multiple properties with the same name..
Eh.. re-scaling doesn't sound that bad, does it?

Ok, we could expose a separate property called MAX-BRIGHTNESS and
drivers simply ignore the range-bounds of the BRIGHTNESS value and use
MAX-BRIGHTNESS instead? Sounds ok'ish.

Thanks
David

Re: [PATCH RFC 4/4] drm: link connectors to backlight devices

[Daniel Vetter]

On Thu, Sep 11, 2014 at 02:22:55PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Sep 11, 2014 at 8:48 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > Nice you skid around all the pitfalls and trapdoors, I guess we've all
> > been rather blind ;-)
> >
> > Two high-level comments:
> > - We also want to forward "bl_power". cros was totally not happy when we
> >   stopped treating brightness == 0 as completely off (it upsets some
> >   panels terminally, so there's a vbt lower limit). Instead we expose this
> >   now through the bl_power knob.
> >
> >   While at it I think we should expose all the other backlight properties
> >   too (read-only ofc for actual/max_brightness).
> 
> bl_power is easy to add. I guess v2 will have:
>   "BACKLIGHT-POWER" (range 0-4)
> 
> actual-brightness is a bit more tricky. Currently, DRM caches property
> values, so there is no read_property() hook. We'd have to add this.
> But it'll be quite nasty as we have to call into the backlight driver.
> So I think we want to run an async-interruptible worker on the
> backlight, drop the locks in the ioctl and wait for the job to finish.
> Not sure whether it's worth it.. maybe we can add this later.

See Jani's reply - we probably don't need it, at least not in version 1.
> 
> > - How does udev match on the drm connector name? They are not terribly
> >   stable atm, and if you reload your drm driver, or much more likely, have
> >   two gpus with two drm drivers they change. We probably should change the
> >   name allocation scheme to be per device instance instead of global
> >   first. Within a driver probe order is hopefully deterministic on a given
> >   platform, since even with super dynamic setups (based on dt/acpi) the
> >   firmware tables should change really.
> 
> You can match on EDID attributes. Ok, so far this is pretty ugly as
> the EDID property is binary. But we can add rather trivial udev
> extensions to make EDID binary against text matching possible.

Why EDID? This is purely about the drm connector name, e.g. if I have 2
gpus, both with an eDP connector (optimus, so just one panel) then the
first driver gets eDP-1 as the name of it and the 2nd one eDP-2. Which is
ok if both should control backlight brightness through the same driver,
but a total mess if not just gpus get switched, but also backlight
controllers.

And if you reload you get then eDP-2 and eDP-3. Well at least in the past,
that hilarity at least was fixed in

commit b21e3afe2357c0f49348a5fb61247012bf8262ec
Author: Ilia Mirkin <imirkin@alum.mit.edu>
Date:   Wed Aug 7 22:34:48 2013 -0400

    drm: use ida to allocate connector id

What I think we need to do is to make these ida allocators per-device, so
that both drivers have an eDP-1 connector. Otherwise you need to either
match both or do funny tricks like "the first eDP connector, no matter
which one on this gpu". After all we can now support more than one eDP
(and more than one LVDS since a long time actually).

Or how exactly is the udev hw db supposed to match this stuff for special
cases. In general we need to duplicate the existing logic from
libbacklight, like Matthew suggested.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH RFC 4/4] drm: link connectors to backlight devices

[Daniel Vetter]

On Thu, Sep 11, 2014 at 02:22:55PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Sep 11, 2014 at 8:48 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > Nice you skid around all the pitfalls and trapdoors, I guess we've all
> > been rather blind ;-)
> >
> > Two high-level comments:
> > - We also want to forward "bl_power". cros was totally not happy when we
> >   stopped treating brightness == 0 as completely off (it upsets some
> >   panels terminally, so there's a vbt lower limit). Instead we expose this
> >   now through the bl_power knob.
> >
> >   While at it I think we should expose all the other backlight properties
> >   too (read-only ofc for actual/max_brightness).
> 
> bl_power is easy to add. I guess v2 will have:
>   "BACKLIGHT-POWER" (range 0-4)
> 
> actual-brightness is a bit more tricky. Currently, DRM caches property
> values, so there is no read_property() hook. We'd have to add this.
> But it'll be quite nasty as we have to call into the backlight driver.
> So I think we want to run an async-interruptible worker on the
> backlight, drop the locks in the ioctl and wait for the job to finish.
> Not sure whether it's worth it.. maybe we can add this later.

See Jani's reply - we probably don't need it, at least not in version 1.
> 
> > - How does udev match on the drm connector name? They are not terribly
> >   stable atm, and if you reload your drm driver, or much more likely, have
> >   two gpus with two drm drivers they change. We probably should change the
> >   name allocation scheme to be per device instance instead of global
> >   first. Within a driver probe order is hopefully deterministic on a given
> >   platform, since even with super dynamic setups (based on dt/acpi) the
> >   firmware tables should change really.
> 
> You can match on EDID attributes. Ok, so far this is pretty ugly as
> the EDID property is binary. But we can add rather trivial udev
> extensions to make EDID binary against text matching possible.

Why EDID? This is purely about the drm connector name, e.g. if I have 2
gpus, both with an eDP connector (optimus, so just one panel) then the
first driver gets eDP-1 as the name of it and the 2nd one eDP-2. Which is
ok if both should control backlight brightness through the same driver,
but a total mess if not just gpus get switched, but also backlight
controllers.

And if you reload you get then eDP-2 and eDP-3. Well at least in the past,
that hilarity at least was fixed in

commit b21e3afe2357c0f49348a5fb61247012bf8262ec
Author: Ilia Mirkin <imirkin@alum.mit.edu>
Date:   Wed Aug 7 22:34:48 2013 -0400

    drm: use ida to allocate connector id

What I think we need to do is to make these ida allocators per-device, so
that both drivers have an eDP-1 connector. Otherwise you need to either
match both or do funny tricks like "the first eDP connector, no matter
which one on this gpu". After all we can now support more than one eDP
(and more than one LVDS since a long time actually).

Or how exactly is the udev hw db supposed to match this stuff for special
cases. In general we need to duplicate the existing logic from
libbacklight, like Matthew suggested.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH RFC 4/4] drm: link connectors to backlight devices

[David Herrmann]

Hi

On Thu, Sep 11, 2014 at 3:06 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Sep 11, 2014 at 02:22:55PM +0200, David Herrmann wrote:
>> actual-brightness is a bit more tricky. Currently, DRM caches property
>> values, so there is no read_property() hook. We'd have to add this.
>> But it'll be quite nasty as we have to call into the backlight driver.
>> So I think we want to run an async-interruptible worker on the
>> backlight, drop the locks in the ioctl and wait for the job to finish.
>> Not sure whether it's worth it.. maybe we can add this later.
>
> See Jani's reply - we probably don't need it, at least not in version 1.

I couldn't see any comment regarding "actual-brightness". But I'm
totally fine with dropping this.

>>
>> > - How does udev match on the drm connector name? They are not terribly
>> >   stable atm, and if you reload your drm driver, or much more likely, have
>> >   two gpus with two drm drivers they change. We probably should change the
>> >   name allocation scheme to be per device instance instead of global
>> >   first. Within a driver probe order is hopefully deterministic on a given
>> >   platform, since even with super dynamic setups (based on dt/acpi) the
>> >   firmware tables should change really.
>>
>> You can match on EDID attributes. Ok, so far this is pretty ugly as
>> the EDID property is binary. But we can add rather trivial udev
>> extensions to make EDID binary against text matching possible.
>
> Why EDID? This is purely about the drm connector name, e.g. if I have 2
> gpus, both with an eDP connector (optimus, so just one panel) then the
> first driver gets eDP-1 as the name of it and the 2nd one eDP-2. Which is
> ok if both should control backlight brightness through the same driver,
> but a total mess if not just gpus get switched, but also backlight
> controllers.
>
> And if you reload you get then eDP-2 and eDP-3. Well at least in the past,
> that hilarity at least was fixed in
>
> commit b21e3afe2357c0f49348a5fb61247012bf8262ec
> Author: Ilia Mirkin <imirkin@alum.mit.edu>
> Date:   Wed Aug 7 22:34:48 2013 -0400
>
>     drm: use ida to allocate connector id
>
> What I think we need to do is to make these ida allocators per-device, so
> that both drivers have an eDP-1 connector. Otherwise you need to either
> match both or do funny tricks like "the first eDP connector, no matter
> which one on this gpu". After all we can now support more than one eDP
> (and more than one LVDS since a long time actually).
>
> Or how exactly is the udev hw db supposed to match this stuff for special
> cases. In general we need to duplicate the existing logic from
> libbacklight, like Matthew suggested.

Yeah, I get what you mean. Names are not stable so even if we can
match on the internal card, we cannot match on "lowest available eDP
display". per-device IDs should be totally fine and fix this issue. We
prefix connectors with the device name anyway, so no conflicts can
arise.

But I think we want to make this a udev builtin anyway. So we can
easily implement the same logic as libbacklight.

Thanks
David

Re: [PATCH RFC 4/4] drm: link connectors to backlight devices

[Jani Nikula]

On Thu, 11 Sep 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Sep 10, 2014 at 05:54:23PM +0200, David Herrmann wrote:
>> Backlight devices have always been managed independently of display
>> controllers. They're often controlled via different hardware interfaces
>> and their relationship to display-controllers varies vastly between
>> different boards. However, display brightness is obviously a property of
>> a display, and thus of a DRM connector. Therefore, it'd be really
>> appreciated if user-space APIs would highlight this relationship.
>> 
>> The main runtime users of backlight interfaces are user-space compositors.
>> But currently they have to jump through hoops to find the correct
>> backlight device for a given connector. Furthermore, they need root
>> privileges to write to sysfs. sysfs has never been designed as run-time
>> non-root API. It does not provide file-contexts, run-time management or
>> any kind of API control. There is no way to control access to sysfs via
>> different links (in that case: mounts). Char-devs provide all this!
>> 
>> So far, backlight APIs have been fairly trivial, so adding char-devs to
>> backlights is rather heavy-weight. Therefore, this patch introduces a new
>> API interface to modify backlight brightness via DRM: A "BRIGHTNESS"
>> property on DRM connectors.
>> 
>> Instead of adding backlight hardware support to DRM, we rely on the
>> backlight-class and simply add a new API. Each DRM Connector can
>> optionally be linked to a backlight class device. Modifying the connector
>> property will have the same effect as writing into the "brightness" sysfs
>> file of the linked backlight class device. However, we now can manage
>> access to backlight devices via the same interface as access to
>> mode-setting on the underlying display. Furthermore, the connection
>> between displays and their backlight devices are visible in user-space.
>> 
>> Obviously, matching backlights to displays cannot be solved magically
>> with this link. Therefore, we also add a user-space attribute to DRM
>> connectors called 'backlight'. If a DRM driver is incapable of matching
>> existing backlights to a connector, or if a given board has just crappy
>> backlight drivers, udev can write the name of a backlight-device into this
>> attribute and the connector-property will be re-linked to this backlight
>> device. The udev hwdb can be easily employed to track such quirks and
>> fixups for different board+GPU combinations.
>> Note that the name written into the 'backlight' attribute is saved on the
>> connector, so in case the real backlight device is probed after the DRM
>> card, the backlight will still get properly attached once probed.
>> 
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>
> Nice you skid around all the pitfalls and trapdoors, I guess we've all
> been rather blind ;-)
>
> Two high-level comments:
> - We also want to forward "bl_power". cros was totally not happy when we
>   stopped treating brightness == 0 as completely off (it upsets some
>   panels terminally, so there's a vbt lower limit). Instead we expose this
>   now through the bl_power knob.

Part of the reason was that their backlight handling userspace only uses
the sysfs interface, not drm, and thus doing dpms to switch the display
off would be more work. (And slow, but that's another matter.) OTOH if
you are already frobbing the connector, it's easy to do dpms, right?

(Side note, another issue with using brightness == 0 for a kind of easy
dpms is that, at least in theory, there are displays that work with
ambient light when the backlight is off. So it doesn't really switch the
display off anyway. Don't know if such displays are common though.)

>   While at it I think we should expose all the other backlight properties
>   too (read-only ofc for actual/max_brightness).

The trouble here, and I think also the reason David chose to use range
0..U16_MAX for the backlight property, is the change that occurs when
the connector-backlight link gets changed. If we expose max, and deal
with the problems, then that doesn't need to be a separate property,
just the max value for the brightness property.

> - How does udev match on the drm connector name? They are not terribly
>   stable atm, and if you reload your drm driver, or much more likely, have
>   two gpus with two drm drivers they change. We probably should change the
>   name allocation scheme to be per device instance instead of global
>   first. Within a driver probe order is hopefully deterministic on a given
>   platform, since even with super dynamic setups (based on dt/acpi) the
>   firmware tables should change really.

Are the backlight sysfs names stable, are acpi_backlightN always
enumerated in the same order?


BR,
Jani.

>
> Cheers, Daniel
>> ---
>>  drivers/gpu/drm/Kconfig             |   1 +
>>  drivers/gpu/drm/Makefile            |   2 +-
>>  drivers/gpu/drm/drm_backlight.c     | 387 ++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/drm_crtc.c          |  45 +++--
>>  drivers/gpu/drm/drm_drv.c           |  11 +
>>  drivers/gpu/drm/drm_sysfs.c         |  53 +++++
>>  drivers/video/backlight/backlight.c |   3 +
>>  include/drm/drm_backlight.h         |  44 ++++
>>  include/drm/drm_crtc.h              |   3 +
>>  include/linux/backlight.h           |   1 +
>>  10 files changed, 530 insertions(+), 20 deletions(-)
>>  create mode 100644 drivers/gpu/drm/drm_backlight.c
>>  create mode 100644 include/drm/drm_backlight.h
>> 
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index e3b4b0f..46bca34 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -12,6 +12,7 @@ menuconfig DRM
>>  	select I2C
>>  	select I2C_ALGOBIT
>>  	select DMA_SHARED_BUFFER
>> +	select BACKLIGHT_CLASS_DEVICE
>>  	help
>>  	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
>>  	  introduced in XFree86 4.0. If you say Y here, you need to select
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 9292a76..224544d 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -14,7 +14,7 @@ drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
>>  		drm_info.o drm_debugfs.o drm_encoder_slave.o \
>>  		drm_trace_points.o drm_global.o drm_prime.o \
>>  		drm_rect.o drm_vma_manager.o drm_flip_work.o \
>> -		drm_modeset_lock.o
>> +		drm_modeset_lock.o drm_backlight.o
>>  
>>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
>>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
>> diff --git a/drivers/gpu/drm/drm_backlight.c b/drivers/gpu/drm/drm_backlight.c
>> new file mode 100644
>> index 0000000..92d231a
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_backlight.c
>> @@ -0,0 +1,387 @@
>> +/*
>> + * DRM Backlight Helpers
>> + * Copyright (c) 2014 David Herrmann
>> + */
>> +
>> +#include <linux/backlight.h>
>> +#include <linux/fs.h>
>> +#include <linux/list.h>
>> +#include <linux/math64.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/notifier.h>
>> +#include <linux/slab.h>
>> +#include <linux/spinlock.h>
>> +#include <drm/drmP.h>
>> +#include <drm/drm_backlight.h>
>> +
>> +/**
>> + * DOC: Backlight Devices
>> + *
>> + * Backlight devices have always been managed as a separate subsystem,
>> + * independent of DRM. They are usually controlled via separate hardware
>> + * interfaces than the display controller, so the split works out fine.
>> + * However, backlight brightness is a property of a display, and thus a
>> + * property of a DRM connector. We already manage DPMS states via connector
>> + * properties, so it is natural to keep brightness control at the same place.
>> + *
>> + * This DRM backlight interface implements generic backlight properties on
>> + * connectors. It does not handle any hardware backends but simply forwards
>> + * the requests to an available and linked backlight device. The links between
>> + * connectors and backlight devices have to be established by DRM drivers and
>> + * can be modified by user-space via sysfs (and udev rules). The name of the
>> + * backlight device can be written to a sysfs attribute called 'backlight'.
>> + * The device is looked up and linked to the connector (replacing a possible
>> + * previous backlight device). A 'change' uevent is sent whenever a link is
>> + * modified.
>> + *
>> + * Drivers have to call drm_backlight_alloc() after allocating a connector via
>> + * drm_connector_init(). This will automatically add a backlight device to the
>> + * given connector. No hardware device is linked to the connector by default.
>> + * Drivers can set up a default device via drm_backlight_set_name(), but are
>> + * free to leave it empty. User-space will then have to set up the link.
>> + */
>> +
>> +struct drm_backlight {
>> +	struct list_head list;
>> +	struct drm_connector *connector;
>> +	char *link_name;
>> +	struct backlight_device *link;
>> +	struct work_struct work;
>> +	unsigned int set_value;
>> +	bool changed : 1;
>> +};
>> +
>> +static LIST_HEAD(drm_backlight_list);
>> +static DEFINE_SPINLOCK(drm_backlight_lock);
>> +
>> +/* caller must hold @drm_backlight_lock */
>> +static bool __drm_backlight_is_registered(struct drm_backlight *b)
>> +{
>> +	/* a device is live if it is linked to @drm_backlight_list */
>> +	return !list_empty(&b->list);
>> +}
>> +
>> +/* caller must hold @drm_backlight_lock */
>> +static void __drm_backlight_schedule(struct drm_backlight *b)
>> +{
>> +	if (__drm_backlight_is_registered(b))
>> +		schedule_work(&b->work);
>> +}
>> +
>> +static void __drm_backlight_worker(struct work_struct *w)
>> +{
>> +	struct drm_backlight *b = container_of(w, struct drm_backlight, work);
>> +	static char *ep[] = { "BACKLIGHT=1", NULL };
>> +	struct backlight_device *bd;
>> +	bool send_uevent;
>> +	unsigned int v;
>> +
>> +	spin_lock(&drm_backlight_lock);
>> +	send_uevent = b->changed;
>> +	b->changed = false;
>> +	v = b->set_value;
>> +	bd = b->link;
>> +	backlight_device_ref(bd);
>> +	spin_unlock(&drm_backlight_lock);
>> +
>> +	if (bd) {
>> +		backlight_set_brightness(bd, v, BACKLIGHT_UPDATE_DRM);
>> +		backlight_device_unref(bd);
>> +	}
>> +
>> +	if (send_uevent)
>> +		kobject_uevent_env(&b->connector->kdev->kobj, KOBJ_CHANGE, ep);
>> +}
>> +
>> +/* caller must hold @drm_backlight_lock */
>> +static void __drm_backlight_prop_changed(struct drm_backlight *b, uint64_t v)
>> +{
>> +	uint64_t max;
>> +
>> +	if (!b->link)
>> +		return;
>> +
>> +	max = b->link->props.max_brightness;
>> +	if (v >= U16_MAX)
>> +		b->set_value = max;
>> +	else
>> +		b->set_value = (v * max) >> 16;
>> +	__drm_backlight_schedule(b);
>> +}
>> +
>> +/* caller must hold @drm_backlight_lock */
>> +static void __drm_backlight_real_changed(struct drm_backlight *b, uint64_t v)
>> +{
>> +	struct drm_mode_config *config = &b->connector->dev->mode_config;
>> +	unsigned int max, set;
>> +
>> +	if (!b->link)
>> +		return;
>> +
>> +	set = v;
>> +	max = b->link->props.max_brightness;
>> +	if (max < 1)
>> +		return;
>> +
>> +	if (set >= max)
>> +		set = U16_MAX;
>> +	else if (max <= U16_MAX)
>> +		set = (set << 16) / max;
>> +	else
>> +		set = div_u64(v << 16, max);
>> +
>> +	drm_object_property_set_value(&b->connector->base,
>> +				      config->brightness_property, v);
>> +}
>> +
>> +/* caller must hold @drm_backlight_lock */
>> +static void __drm_backlight_link(struct drm_backlight *b,
>> +				 struct backlight_device *bd)
>> +{
>> +	if (bd != b->link) {
>> +		backlight_device_unref(b->link);
>> +		b->link = bd;
>> +		backlight_device_ref(b->link);
>> +		if (bd)
>> +			__drm_backlight_real_changed(b, bd->props.brightness);
>> +		b->changed = true;
>> +		__drm_backlight_schedule(b);
>> +	}
>> +}
>> +
>> +/* caller must hold @drm_backlight_lock */
>> +static void __drm_backlight_lookup(struct drm_backlight *b)
>> +{
>> +	struct backlight_device *bd;
>> +
>> +	if (b->link_name)
>> +		bd = backlight_device_lookup(b->link_name);
>> +	else
>> +		bd = NULL;
>> +
>> +	__drm_backlight_link(b, bd);
>> +	backlight_device_unref(bd);
>> +}
>> +
>> +/**
>> + * drm_backlight_alloc - add backlight capability to a connector
>> + * @connector: connector to add backlight to
>> + *
>> + * This allocates a new DRM-backlight device and links it to @connector. This
>> + * *must* be called before registering the connector. The backlight device will
>> + * be automatically registered in sync with the connector. It will also get
>> + * removed once the connector is removed.
>> + *
>> + * The connector will not have any hardware backlight linked by default. You
>> + * need to call drm_backlight_set_name() if you want to set a default
>> + * backlight. User-space can overwrite those via sysfs.
>> + *
>> + * Returns: 0 on success, negative error code on failure.
>> + */
>> +int drm_backlight_alloc(struct drm_connector *connector)
>> +{
>> +	struct drm_mode_config *config = &connector->dev->mode_config;
>> +	struct drm_backlight *b;
>> +
>> +	b = kzalloc(sizeof(*b), GFP_KERNEL);
>> +	if (!b)
>> +		return -ENOMEM;
>> +
>> +	INIT_LIST_HEAD(&b->list);
>> +	INIT_WORK(&b->work, __drm_backlight_worker);
>> +	b->connector = connector;
>> +	connector->backlight = b;
>> +
>> +	drm_object_attach_property(&connector->base,
>> +				   config->brightness_property, U16_MAX);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_backlight_alloc);
>> +
>> +void drm_backlight_free(struct drm_connector *connector)
>> +{
>> +	struct drm_backlight *b = connector->backlight;
>> +
>> +	if (b) {
>> +		WARN_ON(__drm_backlight_is_registered(b));
>> +		WARN_ON(b->link);
>> +
>> +		kfree(b->link_name);
>> +		kfree(b);
>> +		connector->backlight = NULL;
>> +	}
>> +}
>> +
>> +void drm_backlight_register(struct drm_backlight *b)
>> +{
>> +	if (!b)
>> +		return;
>> +
>> +	WARN_ON(__drm_backlight_is_registered(b));
>> +
>> +	spin_lock(&drm_backlight_lock);
>> +	list_add(&b->list, &drm_backlight_list);
>> +	__drm_backlight_lookup(b);
>> +	spin_unlock(&drm_backlight_lock);
>> +}
>> +
>> +void drm_backlight_unregister(struct drm_backlight *b)
>> +{
>> +	if (!b)
>> +		return;
>> +
>> +	WARN_ON(!__drm_backlight_is_registered(b));
>> +
>> +	spin_lock(&drm_backlight_lock);
>> +	list_del_init(&b->list);
>> +	__drm_backlight_link(b, NULL);
>> +	spin_unlock(&drm_backlight_lock);
>> +
>> +	cancel_work_sync(&b->work);
>> +}
>> +
>> +/**
>> + * drm_backlight_get_name - retrieve name of linked backlight device
>> + * @b: DRM backlight to retrieve name of
>> + * @buf: target buffer for name
>> + * @max: size of the target buffer
>> + *
>> + * This retrieves the name of the backlight device linked to @b and writes it
>> + * into @buf. If @buf is NULL or @max is 0, no name will be retrieved, but this
>> + * function only tests whether a link is set.
>> + * Otherwise, the name will always be written into @buf and will always be
>> + * zero-terminated (truncated if too long).
>> + *
>> + * If no backlight device is linked to @b, this returns -ENOENT. Otherwise, the
>> + * length of the written name (excluding the terminating 0 character) is
>> + * returned.
>> + * Note that if a device name has been set but the underlying backlight device
>> + * does not exist, this will still return the linked name. -ENOENT is only
>> + * returned if no device name has been set, yet (or has been cleared).
>> + *
>> + * Returns: On success the length of the written name, on failure a negative
>> + *          error code.
>> + */
>> +int drm_backlight_get_name(struct drm_backlight *b, char *buf, size_t max)
>> +{
>> +	int r;
>> +
>> +	spin_lock(&drm_backlight_lock);
>> +
>> +	if (!b->link_name) {
>> +		r = -ENOENT;
>> +		goto unlock;
>> +	}
>> +
>> +	r = 0;
>> +	if (buf && max > 0) {
>> +		r = strlen(b->link_name);
>> +		if (r + 1 > max)
>> +			r = max - 1;
>> +		buf[r] = 0;
>> +		memcpy(buf, b->link_name, r);
>> +	}
>> +
>> +unlock:
>> +	spin_unlock(&drm_backlight_lock);
>> +	return r;
>> +}
>> +EXPORT_SYMBOL(drm_backlight_get_name);
>> +
>> +/**
>> + * drm_backlight_set_name - Change the device link of a DRM backlight
>> + * @b: DRM backlight to modify
>> + * @name: name of backlight device
>> + *
>> + * This changes the backlight device-link on @b to the hardware device with
>> + * name @name. @name is stored on the backlight device, even if no such
>> + * hardware device is registered, yet. If a backlight device appears later on,
>> + * it will be automatically linked to all matching DRM backlight devices. If a
>> + * real hardware backlight device already exists with such a name, it is linked
>> + * with immediate effect.
>> + *
>> + * Whenever a real hardware backlight is linked or unlinked from a DRM connector
>> + * an uevent with "BACKLIGHT=1" is generated on the connector.
>> + *
>> + * Returns: 0 on success, negative error code on failure.
>> + */
>> +int drm_backlight_set_name(struct drm_backlight *b, const char *name)
>> +{
>> +	char *namecopy;
>> +
>> +	if (name && *name) {
>> +		namecopy = kstrdup(name, GFP_KERNEL);
>> +		if (!namecopy)
>> +			return -ENOMEM;
>> +	} else {
>> +		namecopy = NULL;
>> +	}
>> +
>> +	spin_lock(&drm_backlight_lock);
>> +
>> +	kfree(b->link_name);
>> +	b->link_name = namecopy;
>> +	if (__drm_backlight_is_registered(b))
>> +		__drm_backlight_lookup(b);
>> +
>> +	spin_unlock(&drm_backlight_lock);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_backlight_set_name);
>> +
>> +void drm_backlight_set_brightness(struct drm_backlight *b, uint64_t value)
>> +{
>> +	spin_lock(&drm_backlight_lock);
>> +	__drm_backlight_prop_changed(b, value);
>> +	spin_unlock(&drm_backlight_lock);
>> +}
>> +
>> +static int drm_backlight_notify(struct notifier_block *self,
>> +				unsigned long event, void *data)
>> +{
>> +	struct backlight_device *bd = data;
>> +	struct drm_backlight *b;
>> +	const char *name;
>> +
>> +	spin_lock(&drm_backlight_lock);
>> +
>> +	switch (event) {
>> +	case BACKLIGHT_REGISTERED:
>> +		name = dev_name(&bd->dev);
>> +		if (!name)
>> +			break;
>> +
>> +		list_for_each_entry(b, &drm_backlight_list, list)
>> +			if (!b->link && !strcmp(name, b->link_name))
>> +				__drm_backlight_link(b, bd);
>> +
>> +		break;
>> +	case BACKLIGHT_UNREGISTERED:
>> +		list_for_each_entry(b, &drm_backlight_list, list)
>> +			if (b->link == bd)
>> +				__drm_backlight_link(b, NULL);
>> +
>> +		break;
>> +	}
>> +
>> +	spin_unlock(&drm_backlight_lock);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct notifier_block drm_backlight_notifier = {
>> +	.notifier_call = drm_backlight_notify,
>> +};
>> +
>> +int drm_backlight_init(void)
>> +{
>> +	return backlight_register_notifier(&drm_backlight_notifier);
>> +}
>> +
>> +void drm_backlight_exit(void)
>> +{
>> +	backlight_unregister_notifier(&drm_backlight_notifier);
>> +}
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 7d7c1fd..1e8caa3 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -34,6 +34,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/export.h>
>>  #include <drm/drmP.h>
>> +#include <drm/drm_backlight.h>
>>  #include <drm/drm_crtc.h>
>>  #include <drm/drm_edid.h>
>>  #include <drm/drm_fourcc.h>
>> @@ -918,6 +919,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
>>  	ida_remove(&drm_connector_enum_list[connector->connector_type].ida,
>>  		   connector->connector_type_id);
>>  
>> +	drm_backlight_free(connector);
>>  	drm_mode_object_put(dev, &connector->base);
>>  	kfree(connector->name);
>>  	connector->name = NULL;
>> @@ -963,6 +965,7 @@ int drm_connector_register(struct drm_connector *connector)
>>  	int ret;
>>  
>>  	drm_mode_object_register(connector->dev, &connector->base);
>> +	drm_backlight_register(connector->backlight);
>>  
>>  	ret = drm_sysfs_connector_add(connector);
>>  	if (ret)
>> @@ -986,6 +989,7 @@ EXPORT_SYMBOL(drm_connector_register);
>>   */
>>  void drm_connector_unregister(struct drm_connector *connector)
>>  {
>> +	drm_backlight_unregister(connector->backlight);
>>  	drm_sysfs_connector_remove(connector);
>>  	drm_debugfs_connector_remove(connector);
>>  }
>> @@ -1309,28 +1313,25 @@ EXPORT_SYMBOL(drm_plane_force_disable);
>>  
>>  static int drm_mode_create_standard_connector_properties(struct drm_device *dev)
>>  {
>> -	struct drm_property *edid;
>> -	struct drm_property *dpms;
>> -	struct drm_property *dev_path;
>> +	struct drm_property *p;
>>  
>>  	/*
>>  	 * Standard properties (apply to all connectors)
>>  	 */
>> -	edid = drm_property_create(dev, DRM_MODE_PROP_BLOB |
>> -				   DRM_MODE_PROP_IMMUTABLE,
>> -				   "EDID", 0);
>> -	dev->mode_config.edid_property = edid;
>> -
>> -	dpms = drm_property_create_enum(dev, 0,
>> -				   "DPMS", drm_dpms_enum_list,
>> -				   ARRAY_SIZE(drm_dpms_enum_list));
>> -	dev->mode_config.dpms_property = dpms;
>> -
>> -	dev_path = drm_property_create(dev,
>> -				       DRM_MODE_PROP_BLOB |
>> -				       DRM_MODE_PROP_IMMUTABLE,
>> -				       "PATH", 0);
>> -	dev->mode_config.path_property = dev_path;
>> +	p = drm_property_create(dev, DRM_MODE_PROP_BLOB |
>> +				DRM_MODE_PROP_IMMUTABLE, "EDID", 0);
>> +	dev->mode_config.edid_property = p;
>> +
>> +	p = drm_property_create_enum(dev, 0, "DPMS", drm_dpms_enum_list,
>> +				     ARRAY_SIZE(drm_dpms_enum_list));
>> +	dev->mode_config.dpms_property = p;
>> +
>> +	p = drm_property_create(dev, DRM_MODE_PROP_BLOB |
>> +				DRM_MODE_PROP_IMMUTABLE, "PATH", 0);
>> +	dev->mode_config.path_property = p;
>> +
>> +	p = drm_property_create_range(dev, 0, "BRIGHTNESS", 0, U16_MAX);
>> +	dev->mode_config.brightness_property = p;
>>  
>>  	return 0;
>>  }
>> @@ -4145,12 +4146,18 @@ static int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
>>  {
>>  	int ret = -EINVAL;
>>  	struct drm_connector *connector = obj_to_connector(obj);
>> +	struct drm_mode_config *config = &connector->dev->mode_config;
>>  
>>  	/* Do DPMS ourselves */
>> -	if (property == connector->dev->mode_config.dpms_property) {
>> +	if (property == config->dpms_property) {
>>  		if (connector->funcs->dpms)
>>  			(*connector->funcs->dpms)(connector, (int)value);
>>  		ret = 0;
>> +	} else if (property == config->brightness_property) {
>> +		if (connector->backlight)
>> +			drm_backlight_set_brightness(connector->backlight,
>> +						     value);
>> +		ret = 0;
>>  	} else if (connector->funcs->set_property)
>>  		ret = connector->funcs->set_property(connector, property, value);
>>  
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 6645669..76c9401 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -33,6 +33,7 @@
>>  #include <linux/mount.h>
>>  #include <linux/slab.h>
>>  #include <drm/drmP.h>
>> +#include <drm/drm_backlight.h>
>>  #include <drm/drm_core.h>
>>  #include "drm_legacy.h"
>>  
>> @@ -890,9 +891,18 @@ static int __init drm_core_init(void)
>>  		goto err_p3;
>>  	}
>>  
>> +	ret = drm_backlight_init();
>> +	if (ret < 0) {
>> +		DRM_ERROR("Cannot initialize backlight interface\n");
>> +		goto err_p4;
>> +	}
>> +
>>  	DRM_INFO("Initialized %s %d.%d.%d %s\n",
>>  		 CORE_NAME, CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL, CORE_DATE);
>>  	return 0;
>> +
>> +err_p4:
>> +	debugfs_remove(drm_debugfs_root);
>>  err_p3:
>>  	drm_sysfs_destroy();
>>  err_p2:
>> @@ -905,6 +915,7 @@ err_p1:
>>  
>>  static void __exit drm_core_exit(void)
>>  {
>> +	drm_backlight_exit();
>>  	debugfs_remove(drm_debugfs_root);
>>  	drm_sysfs_destroy();
>>  
>> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
>> index ab1a5f6..bc5d7f3 100644
>> --- a/drivers/gpu/drm/drm_sysfs.c
>> +++ b/drivers/gpu/drm/drm_sysfs.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/err.h>
>>  #include <linux/export.h>
>>  
>> +#include <drm/drm_backlight.h>
>>  #include <drm/drm_sysfs.h>
>>  #include <drm/drm_core.h>
>>  #include <drm/drmP.h>
>> @@ -256,6 +257,57 @@ static ssize_t modes_show(struct device *device,
>>  	return written;
>>  }
>>  
>> +static ssize_t backlight_show(struct device *device,
>> +			      struct device_attribute *attr,
>> +			      char *buf)
>> +{
>> +	struct drm_connector *connector = to_drm_connector(device);
>> +	int r;
>> +
>> +	if (!connector->backlight)
>> +		return -ENOTSUPP;
>> +
>> +	r = drm_backlight_get_name(connector->backlight, buf, PAGE_SIZE);
>> +	if (r < 0)
>> +		return r;
>> +
>> +	if (r + 1 < PAGE_SIZE) {
>> +		buf[r++] = '\n';
>> +		buf[r] = 0;
>> +	}
>> +
>> +	return r;
>> +}
>> +
>> +static ssize_t backlight_store(struct device *device,
>> +			       struct device_attribute *attr,
>> +			       const char *buf, size_t size)
>> +{
>> +	struct drm_connector *connector = to_drm_connector(device);
>> +	const char *t;
>> +	char *name;
>> +	size_t len;
>> +	int r;
>> +
>> +	if (!connector->backlight)
>> +		return -ENOTSUPP;
>> +
>> +	t = strchrnul(buf, '\n');
>> +	len = t - buf;
>> +
>> +	name = kstrndup(buf, len, GFP_TEMPORARY);
>> +	if (!name)
>> +		return -ENOMEM;
>> +
>> +	r = drm_backlight_set_name(connector->backlight, name);
>> +	kfree(name);
>> +
>> +	if (r < 0)
>> +		return r;
>> +
>> +	return len;
>> +}
>> +
>>  static ssize_t subconnector_show(struct device *device,
>>  			   struct device_attribute *attr,
>>  			   char *buf)
>> @@ -343,6 +395,7 @@ static struct device_attribute connector_attrs[] = {
>>  	__ATTR_RO(enabled),
>>  	__ATTR_RO(dpms),
>>  	__ATTR_RO(modes),
>> +	__ATTR_RW(backlight),
>>  };
>>  
>>  /* These attributes are for both DVI-I connectors and all types of tv-out. */
>> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
>> index 04f323b..a1bc533 100644
>> --- a/drivers/video/backlight/backlight.c
>> +++ b/drivers/video/backlight/backlight.c
>> @@ -113,6 +113,9 @@ static void backlight_generate_event(struct backlight_device *bd,
>>  	case BACKLIGHT_UPDATE_HOTKEY:
>>  		envp[0] = "SOURCE=hotkey";
>>  		break;
>> +	case BACKLIGHT_UPDATE_DRM:
>> +		envp[0] = "SOURCE=drm";
>> +		break;
>>  	default:
>>  		envp[0] = "SOURCE=unknown";
>>  		break;
>> diff --git a/include/drm/drm_backlight.h b/include/drm/drm_backlight.h
>> new file mode 100644
>> index 0000000..9f0c31b
>> --- /dev/null
>> +++ b/include/drm/drm_backlight.h
>> @@ -0,0 +1,44 @@
>> +#ifndef __DRM_BACKLIGHT_H__
>> +#define __DRM_BACKLIGHT_H__
>> +
>> +/*
>> + * Copyright (c) 2014 David Herrmann <dh.herrmann@gmail.com>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/types.h>
>> +
>> +struct drm_backlight;
>> +struct drm_connector;
>> +
>> +int drm_backlight_init(void);
>> +void drm_backlight_exit(void);
>> +
>> +int drm_backlight_alloc(struct drm_connector *connector);
>> +void drm_backlight_free(struct drm_connector *connector);
>> +void drm_backlight_register(struct drm_backlight *b);
>> +void drm_backlight_unregister(struct drm_backlight *b);
>> +
>> +int drm_backlight_get_name(struct drm_backlight *b, char *buf, size_t max);
>> +int drm_backlight_set_name(struct drm_backlight *b, const char *name);
>> +void drm_backlight_set_brightness(struct drm_backlight *b, uint64_t value);
>> +
>> +#endif /* __DRM_BACKLIGHT_H__ */
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index c40070a..ce3b2f0 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -542,6 +542,8 @@ struct drm_connector {
>>  
>>  	/* requested DPMS state */
>>  	int dpms;
>> +	/* backlight link */
>> +	struct drm_backlight *backlight;
>>  
>>  	void *helper_private;
>>  
>> @@ -823,6 +825,7 @@ struct drm_mode_config {
>>  	struct list_head property_blob_list;
>>  	struct drm_property *edid_property;
>>  	struct drm_property *dpms_property;
>> +	struct drm_property *brightness_property;
>>  	struct drm_property *path_property;
>>  	struct drm_property *plane_type_property;
>>  	struct drm_property *rotation_property;
>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>> index bcc0dec..8615f54 100644
>> --- a/include/linux/backlight.h
>> +++ b/include/linux/backlight.h
>> @@ -31,6 +31,7 @@
>>  enum backlight_update_reason {
>>  	BACKLIGHT_UPDATE_HOTKEY,
>>  	BACKLIGHT_UPDATE_SYSFS,
>> +	BACKLIGHT_UPDATE_DRM,
>>  };
>>  
>>  enum backlight_type {
>> -- 
>> 2.1.0
>> 
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH RFC 0/4] Linking DRM Connectors to Backlight Devices

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2838 bytes --]

On Wed, 2014-09-10 at 17:54 +0200, David Herrmann wrote:

>  * User-space currently has a hard-time figuring out which backlight device to
>    use, and which backlight device belongs to which display. So far, most
>    systems only provide backlight-devices for internal displays, so figuring out
>    the connection is easy, but that might change with more capable external
>    connectors.

The parent device of the backlight will be the correct display, if the
kernel has a meaningful way to determine that. We could do a better job
in the ACPI code than we currently do, but (unfortunately) that requires
us to know the ACPI IDs that each GPU vendor uses.

>    If multiple backlights are available, the easiest solution is to simply write
>    to all of them and hope at least one of them works. This obviously fails if
>    the devices interact badly, so it's not really a solution.

There's a pretty well-defined process for that, although it sucks - if
it's an LVDS or eDP display and there's a backlight of type "firmware",
use it. If there's no "firmware" backlight, but there is a "platform"
one, use that. Otherwise look for a "native" backlight that has the
output as a parent. libbacklight does all of this for you. 

>  * User-space needs root privileges to write to sysfs. There are no char-devs
>    that can be used to control access-management to the backlight API, so /sys
>    is the only interface available. So far, udev policy has been "/sys is
>    read-only for non-root" and it's not going to change. char-devs are *THE*
>    standard way to provide non-root user-space APIs, so use them!

Yeah, this is a problem.

> This series tries to solve this problem with a much simpler approach:
> Instead of moving backlights into DRM, we simply link DRM properties to a
> backlight device. That is, the kernel manages a link between a connector and a
> backlight device (or n backlight devices) which can be modified by udev in case
> the kernel got it wrong (we don't want huge board-fixup-tables in the kernel).
> User-space can now use the simpl DRM API to manage backlights, and the kernel
> does not need any special driver code to make it work.

This doesn't really simplify userspace significantly - something's still
going to have to make the same policy decision as we do right now, and
the kernel isn't really the right place to do that. It does have the
benefit of allowing that policy decision to be made at boot time and
then allow that to be consumed by all later userspace, so there is
*some* benefit, but I think the "make unprivileged userspace possible"
argument is much more compelling.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH RFC 0/4] Linking DRM Connectors to Backlight Devices

[Matthew Garrett]

On Wed, 2014-09-10 at 17:54 +0200, David Herrmann wrote:

>  * User-space currently has a hard-time figuring out which backlight device to
>    use, and which backlight device belongs to which display. So far, most
>    systems only provide backlight-devices for internal displays, so figuring out
>    the connection is easy, but that might change with more capable external
>    connectors.

The parent device of the backlight will be the correct display, if the
kernel has a meaningful way to determine that. We could do a better job
in the ACPI code than we currently do, but (unfortunately) that requires
us to know the ACPI IDs that each GPU vendor uses.

>    If multiple backlights are available, the easiest solution is to simply write
>    to all of them and hope at least one of them works. This obviously fails if
>    the devices interact badly, so it's not really a solution.

There's a pretty well-defined process for that, although it sucks - if
it's an LVDS or eDP display and there's a backlight of type "firmware",
use it. If there's no "firmware" backlight, but there is a "platform"
one, use that. Otherwise look for a "native" backlight that has the
output as a parent. libbacklight does all of this for you. 

>  * User-space needs root privileges to write to sysfs. There are no char-devs
>    that can be used to control access-management to the backlight API, so /sys
>    is the only interface available. So far, udev policy has been "/sys is
>    read-only for non-root" and it's not going to change. char-devs are *THE*
>    standard way to provide non-root user-space APIs, so use them!

Yeah, this is a problem.

> This series tries to solve this problem with a much simpler approach:
> Instead of moving backlights into DRM, we simply link DRM properties to a
> backlight device. That is, the kernel manages a link between a connector and a
> backlight device (or n backlight devices) which can be modified by udev in case
> the kernel got it wrong (we don't want huge board-fixup-tables in the kernel).
> User-space can now use the simpl DRM API to manage backlights, and the kernel
> does not need any special driver code to make it work.

This doesn't really simplify userspace significantly - something's still
going to have to make the same policy decision as we do right now, and
the kernel isn't really the right place to do that. It does have the
benefit of allowing that policy decision to be made at boot time and
then allow that to be consumed by all later userspace, so there is
*some* benefit, but I think the "make unprivileged userspace possible"
argument is much more compelling.

-- 
Matthew Garrett <matthew.garrett@nebula.com>

Re: [PATCH RFC 0/4] Linking DRM Connectors to Backlight Devices

[David Herrmann]

Hi

On Wed, Sep 10, 2014 at 10:40 PM, Matthew Garrett
<matthew.garrett@nebula.com> wrote:
> On Wed, 2014-09-10 at 17:54 +0200, David Herrmann wrote:
>
>>  * User-space currently has a hard-time figuring out which backlight device to
>>    use, and which backlight device belongs to which display. So far, most
>>    systems only provide backlight-devices for internal displays, so figuring out
>>    the connection is easy, but that might change with more capable external
>>    connectors.
>
> The parent device of the backlight will be the correct display, if the
> kernel has a meaningful way to determine that. We could do a better job
> in the ACPI code than we currently do, but (unfortunately) that requires
> us to know the ACPI IDs that each GPU vendor uses.

We also probe ACPI devices independently of PCI devices (or other
buses). So the actual DRM device might be created much later than the
backlight, thus it cannot be a parent of the backlight. We can try to
find a common ancestor, though.

>> This series tries to solve this problem with a much simpler approach:
>> Instead of moving backlights into DRM, we simply link DRM properties to a
>> backlight device. That is, the kernel manages a link between a connector and a
>> backlight device (or n backlight devices) which can be modified by udev in case
>> the kernel got it wrong (we don't want huge board-fixup-tables in the kernel).
>> User-space can now use the simpl DRM API to manage backlights, and the kernel
>> does not need any special driver code to make it work.
>
> This doesn't really simplify userspace significantly - something's still
> going to have to make the same policy decision as we do right now, and
> the kernel isn't really the right place to do that.

This patch allows to add really simple udev rules that implement any
policy we want. This way, we can keep the policy in user-space, but at
the same time it's no longer part of the compositors. Instead, we have
an independent place (udev rules) where to write that policy and tell
the kernel. I think this is an improvement. But of course, the
unprivileged access is the much more compelling argument.

Thanks
David

Re: [PATCH RFC 0/4] Linking DRM Connectors to Backlight Devices

[David Herrmann]

Hi

On Wed, Sep 10, 2014 at 10:40 PM, Matthew Garrett
<matthew.garrett@nebula.com> wrote:
> On Wed, 2014-09-10 at 17:54 +0200, David Herrmann wrote:
>
>>  * User-space currently has a hard-time figuring out which backlight device to
>>    use, and which backlight device belongs to which display. So far, most
>>    systems only provide backlight-devices for internal displays, so figuring out
>>    the connection is easy, but that might change with more capable external
>>    connectors.
>
> The parent device of the backlight will be the correct display, if the
> kernel has a meaningful way to determine that. We could do a better job
> in the ACPI code than we currently do, but (unfortunately) that requires
> us to know the ACPI IDs that each GPU vendor uses.

We also probe ACPI devices independently of PCI devices (or other
buses). So the actual DRM device might be created much later than the
backlight, thus it cannot be a parent of the backlight. We can try to
find a common ancestor, though.

>> This series tries to solve this problem with a much simpler approach:
>> Instead of moving backlights into DRM, we simply link DRM properties to a
>> backlight device. That is, the kernel manages a link between a connector and a
>> backlight device (or n backlight devices) which can be modified by udev in case
>> the kernel got it wrong (we don't want huge board-fixup-tables in the kernel).
>> User-space can now use the simpl DRM API to manage backlights, and the kernel
>> does not need any special driver code to make it work.
>
> This doesn't really simplify userspace significantly - something's still
> going to have to make the same policy decision as we do right now, and
> the kernel isn't really the right place to do that.

This patch allows to add really simple udev rules that implement any
policy we want. This way, we can keep the policy in user-space, but at
the same time it's no longer part of the compositors. Instead, we have
an independent place (udev rules) where to write that policy and tell
the kernel. I think this is an improvement. But of course, the
unprivileged access is the much more compelling argument.

Thanks
David

[PATCH 0/6] Rebase of David Herrmann drm connector link to backlight device

[Marta Lofstedt]

Hi David,

I am currently investigating:
https://bugs.freedesktop.org/show_bug.cgi?id=96572

Martin Peres suggested that your patches:
https://lists.freedesktop.org/archives/dri-devel/2014-September/thread.html#67984
could solve the xf86-video-modesetting backlight issues.

I have rebased your patches and I am working on an IGT
test for the functionality. With my i915 implementation
and the small included bug-fix, I can update the drm BACKLIGHT
property and the value is updated in the backlight class device.
However, if I set the brigness value through the sysfs file of
the backlight class device the drm BRIGHTNESS property does not
update which would be confusing to users.

My understanding is that DRM properties are cached and, by design,
do not have the capability to read the status from the driver.

What do we want to do about this?

Marta

David Herrmann (4):
  backlight: use static initializers
  backlight: use spin-lock to protect device list
  backlight: add kernel-internal backlight API
  drm: link connectors to backlight devices

Marta Lofstedt (2):
  i915: Use drm backlight
  drm: drm_backlight use the connect value to set brightness property

 drivers/gpu/drm/Kconfig             |   1 +
 drivers/gpu/drm/Makefile            |   2 +-
 drivers/gpu/drm/drm_backlight.c     | 387 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_connector.c     |  11 +
 drivers/gpu/drm/drm_crtc.c          |   6 +
 drivers/gpu/drm/drm_drv.c           |   8 +
 drivers/gpu/drm/drm_sysfs.c         |  54 +++++
 drivers/gpu/drm/i915/intel_panel.c  |   5 +
 drivers/video/backlight/backlight.c |  91 +++++++--
 include/drm/drm_backlight.h         |  44 ++++
 include/drm/drm_connector.h         |   3 +
 include/drm/drm_crtc.h              |   5 +
 include/linux/backlight.h           |  17 ++
 13 files changed, 621 insertions(+), 13 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_backlight.c
 create mode 100644 include/drm/drm_backlight.h

-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/6] backlight: use static initializers

[Marta Lofstedt]

From: David Herrmann <dh.herrmann at gmail.com>

Use static initializers instead of setting up global variables during
runtime. This reduces code size and execution time.

Signed-off-by: David Herrmann <dh.herrmann at gmail.com>
---
 drivers/video/backlight/backlight.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 288318a..37f6173 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -21,9 +21,9 @@
 #include <asm/backlight.h>
 #endif
 
-static struct list_head backlight_dev_list;
-static struct mutex backlight_dev_list_mutex;
-static struct blocking_notifier_head backlight_notifier;
+static LIST_HEAD(backlight_dev_list);
+static DEFINE_MUTEX(backlight_dev_list_mutex);
+static BLOCKING_NOTIFIER_HEAD(backlight_notifier);
 
 static const char *const backlight_types[] = {
 	[BACKLIGHT_RAW] = "raw",
@@ -591,9 +591,6 @@ static int __init backlight_class_init(void)
 
 	backlight_class->dev_groups = bl_device_groups;
 	backlight_class->pm = &backlight_class_dev_pm_ops;
-	INIT_LIST_HEAD(&backlight_dev_list);
-	mutex_init(&backlight_dev_list_mutex);
-	BLOCKING_INIT_NOTIFIER_HEAD(&backlight_notifier);
 
 	return 0;
 }
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/6] backlight: use spin-lock to protect device list

[Marta Lofstedt]

From: David Herrmann <dh.herrmann at gmail.com>

There is really no reason to use a mutex to protect a simple list. Convert
the list-lock to a simple spinlock instead.

The spin-locks prepare for a backlight_find() helper, which should
preferably be usable from atomic context. A mutex would prevent that, so
use an irq-save spinlock instead.

Signed-off-by: David Herrmann <dh.herrmann at gmail.com>
---
 drivers/video/backlight/backlight.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 37f6173..716c091 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -16,13 +16,14 @@
 #include <linux/err.h>
 #include <linux/fb.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 
 #ifdef CONFIG_PMAC_BACKLIGHT
 #include <asm/backlight.h>
 #endif
 
 static LIST_HEAD(backlight_dev_list);
-static DEFINE_MUTEX(backlight_dev_list_mutex);
+static DEFINE_SPINLOCK(backlight_dev_list_lock);
 static BLOCKING_NOTIFIER_HEAD(backlight_notifier);
 
 static const char *const backlight_types[] = {
@@ -378,9 +379,9 @@ struct backlight_device *backlight_device_register(const char *name,
 	mutex_unlock(&pmac_backlight_mutex);
 #endif
 
-	mutex_lock(&backlight_dev_list_mutex);
+	spin_lock_irq(&backlight_dev_list_lock);
 	list_add(&new_bd->entry, &backlight_dev_list);
-	mutex_unlock(&backlight_dev_list_mutex);
+	spin_unlock_irq(&backlight_dev_list_lock);
 
 	blocking_notifier_call_chain(&backlight_notifier,
 				     BACKLIGHT_REGISTERED, new_bd);
@@ -393,15 +394,16 @@ struct backlight_device *backlight_device_get_by_type(enum backlight_type type)
 {
 	bool found = false;
 	struct backlight_device *bd;
+	unsigned long flags;
 
-	mutex_lock(&backlight_dev_list_mutex);
+	spin_lock_irqsave(&backlight_dev_list_lock, flags);
 	list_for_each_entry(bd, &backlight_dev_list, entry) {
 		if (bd->props.type == type) {
 			found = true;
 			break;
 		}
 	}
-	mutex_unlock(&backlight_dev_list_mutex);
+	spin_unlock_irqrestore(&backlight_dev_list_lock, flags);
 
 	return found ? bd : NULL;
 }
@@ -418,9 +420,9 @@ void backlight_device_unregister(struct backlight_device *bd)
 	if (!bd)
 		return;
 
-	mutex_lock(&backlight_dev_list_mutex);
+	spin_lock_irq(&backlight_dev_list_lock);
 	list_del(&bd->entry);
-	mutex_unlock(&backlight_dev_list_mutex);
+	spin_unlock_irq(&backlight_dev_list_lock);
 
 #ifdef CONFIG_PMAC_BACKLIGHT
 	mutex_lock(&pmac_backlight_mutex);
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/6] backlight: add kernel-internal backlight API

[Marta Lofstedt]

From: David Herrmann <dh.herrmann at gmail.com>

So far backlights have only been controlled via sysfs. However, sysfs is
not a proper user-space API for runtime modifications, and never was
intended to provide such. The DRM drivers are now prepared to provide
such a backlight link so user-space can control backlight via DRM
connector properties. This allows us to employ the same access-management
we use for mode-setting.

This patch adds few kernel-internal backlight helpers so we can modify
backlights from within DRM.

Signed-off-by: David Herrmann <dh.herrmann at gmail.com>
---
 drivers/video/backlight/backlight.c | 65 +++++++++++++++++++++++++++++++++++++
 include/linux/backlight.h           | 16 +++++++++
 2 files changed, 81 insertions(+)

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 716c091..c7b335c 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -489,6 +489,71 @@ int backlight_unregister_notifier(struct notifier_block *nb)
 EXPORT_SYMBOL(backlight_unregister_notifier);
 
 /**
+ * backlight_device_lookup - find a backlight device
+ * @name: sysname of the backlight device
+ *
+ * @return Reference to the backlight device, NULL if not found.
+ *
+ * This searches through all registered backlight devices for a device with the
+ * given device name. In case none is found, NULL is returned, otherwise a
+ * new reference to the backlight device is returned. You must drop this
+ * reference via backlight_device_unref() once done.
+ * Note that the devices might get unregistered at any time. You need to lock
+ * around this lookup and inside of your backlight-notifier if you need to know
+ * when a device gets unregistered.
+ *
+ * This function can be safely called from IRQ context.
+ */
+struct backlight_device *backlight_device_lookup(const char *name)
+{
+	struct backlight_device *bd;
+	unsigned long flags;
+	const char *t;
+
+	spin_lock_irqsave(&backlight_dev_list_lock, flags);
+
+	list_for_each_entry(bd, &backlight_dev_list, entry) {
+		t = dev_name(&bd->dev);
+		if (t && !strcmp(t, name))
+			goto out;
+	}
+
+	bd = NULL;
+
+out:
+	spin_unlock_irqrestore(&backlight_dev_list_lock, flags);
+	backlight_device_ref(bd);
+	return bd;
+}
+EXPORT_SYMBOL_GPL(backlight_device_lookup);
+
+/**
+ * backlight_set_brightness - set brightness on a backlight device
+ * @bd: backlight device to operate on
+ * @value: brightness value to set on the device
+ * @reason: backlight-change reason to use for notifications
+ *
+ * This is the in-kernel API equivalent of writing into the 'brightness' sysfs
+ * file. It calls into the underlying backlight driver to change the brightness
+ * value. The value is clamped according to device bounds.
+ * A uevent notification is sent with the reason set to @reason.
+ */
+void backlight_set_brightness(struct backlight_device *bd, unsigned int value,
+			      enum backlight_update_reason reason)
+{
+	mutex_lock(&bd->ops_lock);
+	if (bd->ops) {
+		value = clamp(value, 0U, (unsigned)bd->props.max_brightness);
+		pr_debug("set brightness to %u\n", value);
+		bd->props.brightness = value;
+		backlight_update_status(bd);
+	}
+	mutex_unlock(&bd->ops_lock);
+	backlight_generate_event(bd, reason);
+}
+EXPORT_SYMBOL_GPL(backlight_set_brightness);
+
+/**
  * devm_backlight_device_register - resource managed backlight_device_register()
  * @dev: the device to register
  * @name: the name of the device
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 5f2fd61..895c4661 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -146,6 +146,22 @@ extern int backlight_unregister_notifier(struct notifier_block *nb);
 extern struct backlight_device *backlight_device_get_by_type(enum backlight_type type);
 extern int backlight_device_set_brightness(struct backlight_device *bd, unsigned long brightness);
 
+struct backlight_device *backlight_device_lookup(const char *name);
+void backlight_set_brightness(struct backlight_device *bd, unsigned int value,
+			      enum backlight_update_reason reason);
+
+static inline void backlight_device_ref(struct backlight_device *bd)
+{
+	if (bd)
+		get_device(&bd->dev);
+}
+
+static inline void backlight_device_unref(struct backlight_device *bd)
+{
+	if (bd)
+		put_device(&bd->dev);
+}
+
 #define to_backlight_device(obj) container_of(obj, struct backlight_device, dev)
 
 static inline void * bl_get_data(struct backlight_device *bl_dev)
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/6] drm: link connectors to backlight devices

[Marta Lofstedt]

From: David Herrmann <dh.herrmann at gmail.com>

Backlight devices have always been managed independently of display
controllers. They're often controlled via different hardware interfaces
and their relationship to display-controllers varies vastly between
different boards. However, display brightness is obviously a property of
a display, and thus of a DRM connector. Therefore, it'd be really
appreciated if user-space APIs would highlight this relationship.

The main runtime users of backlight interfaces are user-space compositors.
But currently they have to jump through hoops to find the correct
backlight device for a given connector. Furthermore, they need root
privileges to write to sysfs. sysfs has never been designed as run-time
non-root API. It does not provide file-contexts, run-time management or
any kind of API control. There is no way to control access to sysfs via
different links (in that case: mounts). Char-devs provide all this!

So far, backlight APIs have been fairly trivial, so adding char-devs to
backlights is rather heavy-weight. Therefore, this patch introduces a new
API interface to modify backlight brightness via DRM: A "BRIGHTNESS"
property on DRM connectors.

Instead of adding backlight hardware support to DRM, we rely on the
backlight-class and simply add a new API. Each DRM Connector can
optionally be linked to a backlight class device. Modifying the connector
property will have the same effect as writing into the "brightness" sysfs
file of the linked backlight class device. However, we now can manage
access to backlight devices via the same interface as access to
mode-setting on the underlying display. Furthermore, the connection
between displays and their backlight devices are visible in user-space.

Obviously, matching backlights to displays cannot be solved magically
with this link. Therefore, we also add a user-space attribute to DRM
connectors called 'backlight'. If a DRM driver is incapable of matching
existing backlights to a connector, or if a given board has just crappy
backlight drivers, udev can write the name of a backlight-device into this
attribute and the connector-property will be re-linked to this backlight
device. The udev hwdb can be easily employed to track such quirks and
fixups for different board+GPU combinations.
Note that the name written into the 'backlight' attribute is saved on the
connector, so in case the real backlight device is probed after the DRM
card, the backlight will still get properly attached once probed.

Signed-off-by: David Herrmann <dh.herrmann at gmail.com>
---
 drivers/gpu/drm/Kconfig             |   1 +
 drivers/gpu/drm/Makefile            |   2 +-
 drivers/gpu/drm/drm_backlight.c     | 387 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_connector.c     |  11 +
 drivers/gpu/drm/drm_crtc.c          |   6 +
 drivers/gpu/drm/drm_drv.c           |   8 +
 drivers/gpu/drm/drm_sysfs.c         |  54 +++++
 drivers/video/backlight/backlight.c |   3 +
 include/drm/drm_backlight.h         |  44 ++++
 include/drm/drm_connector.h         |   3 +
 include/drm/drm_crtc.h              |   5 +
 include/linux/backlight.h           |   1 +
 12 files changed, 524 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/drm_backlight.c
 create mode 100644 include/drm/drm_backlight.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 483059a..1ce6a1c 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -12,6 +12,7 @@ menuconfig DRM
 	select I2C
 	select I2C_ALGOBIT
 	select DMA_SHARED_BUFFER
+	select BACKLIGHT_CLASS_DEVICE
 	help
 	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
 	  introduced in XFree86 4.0. If you say Y here, you need to select
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 74579d2..f016847 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -15,7 +15,7 @@ drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
 		drm_modeset_lock.o drm_atomic.o drm_bridge.o \
 		drm_framebuffer.o drm_connector.o drm_blend.o \
 		drm_encoder.o drm_mode_object.o drm_property.o \
-		drm_plane.o drm_color_mgmt.o
+		drm_plane.o drm_color_mgmt.o drm_backlight.o
 
 drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
diff --git a/drivers/gpu/drm/drm_backlight.c b/drivers/gpu/drm/drm_backlight.c
new file mode 100644
index 0000000..92d231a
--- /dev/null
+++ b/drivers/gpu/drm/drm_backlight.c
@@ -0,0 +1,387 @@
+/*
+ * DRM Backlight Helpers
+ * Copyright (c) 2014 David Herrmann
+ */
+
+#include <linux/backlight.h>
+#include <linux/fs.h>
+#include <linux/list.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <drm/drmP.h>
+#include <drm/drm_backlight.h>
+
+/**
+ * DOC: Backlight Devices
+ *
+ * Backlight devices have always been managed as a separate subsystem,
+ * independent of DRM. They are usually controlled via separate hardware
+ * interfaces than the display controller, so the split works out fine.
+ * However, backlight brightness is a property of a display, and thus a
+ * property of a DRM connector. We already manage DPMS states via connector
+ * properties, so it is natural to keep brightness control at the same place.
+ *
+ * This DRM backlight interface implements generic backlight properties on
+ * connectors. It does not handle any hardware backends but simply forwards
+ * the requests to an available and linked backlight device. The links between
+ * connectors and backlight devices have to be established by DRM drivers and
+ * can be modified by user-space via sysfs (and udev rules). The name of the
+ * backlight device can be written to a sysfs attribute called 'backlight'.
+ * The device is looked up and linked to the connector (replacing a possible
+ * previous backlight device). A 'change' uevent is sent whenever a link is
+ * modified.
+ *
+ * Drivers have to call drm_backlight_alloc() after allocating a connector via
+ * drm_connector_init(). This will automatically add a backlight device to the
+ * given connector. No hardware device is linked to the connector by default.
+ * Drivers can set up a default device via drm_backlight_set_name(), but are
+ * free to leave it empty. User-space will then have to set up the link.
+ */
+
+struct drm_backlight {
+	struct list_head list;
+	struct drm_connector *connector;
+	char *link_name;
+	struct backlight_device *link;
+	struct work_struct work;
+	unsigned int set_value;
+	bool changed : 1;
+};
+
+static LIST_HEAD(drm_backlight_list);
+static DEFINE_SPINLOCK(drm_backlight_lock);
+
+/* caller must hold @drm_backlight_lock */
+static bool __drm_backlight_is_registered(struct drm_backlight *b)
+{
+	/* a device is live if it is linked to @drm_backlight_list */
+	return !list_empty(&b->list);
+}
+
+/* caller must hold @drm_backlight_lock */
+static void __drm_backlight_schedule(struct drm_backlight *b)
+{
+	if (__drm_backlight_is_registered(b))
+		schedule_work(&b->work);
+}
+
+static void __drm_backlight_worker(struct work_struct *w)
+{
+	struct drm_backlight *b = container_of(w, struct drm_backlight, work);
+	static char *ep[] = { "BACKLIGHT=1", NULL };
+	struct backlight_device *bd;
+	bool send_uevent;
+	unsigned int v;
+
+	spin_lock(&drm_backlight_lock);
+	send_uevent = b->changed;
+	b->changed = false;
+	v = b->set_value;
+	bd = b->link;
+	backlight_device_ref(bd);
+	spin_unlock(&drm_backlight_lock);
+
+	if (bd) {
+		backlight_set_brightness(bd, v, BACKLIGHT_UPDATE_DRM);
+		backlight_device_unref(bd);
+	}
+
+	if (send_uevent)
+		kobject_uevent_env(&b->connector->kdev->kobj, KOBJ_CHANGE, ep);
+}
+
+/* caller must hold @drm_backlight_lock */
+static void __drm_backlight_prop_changed(struct drm_backlight *b, uint64_t v)
+{
+	uint64_t max;
+
+	if (!b->link)
+		return;
+
+	max = b->link->props.max_brightness;
+	if (v >= U16_MAX)
+		b->set_value = max;
+	else
+		b->set_value = (v * max) >> 16;
+	__drm_backlight_schedule(b);
+}
+
+/* caller must hold @drm_backlight_lock */
+static void __drm_backlight_real_changed(struct drm_backlight *b, uint64_t v)
+{
+	struct drm_mode_config *config = &b->connector->dev->mode_config;
+	unsigned int max, set;
+
+	if (!b->link)
+		return;
+
+	set = v;
+	max = b->link->props.max_brightness;
+	if (max < 1)
+		return;
+
+	if (set >= max)
+		set = U16_MAX;
+	else if (max <= U16_MAX)
+		set = (set << 16) / max;
+	else
+		set = div_u64(v << 16, max);
+
+	drm_object_property_set_value(&b->connector->base,
+				      config->brightness_property, v);
+}
+
+/* caller must hold @drm_backlight_lock */
+static void __drm_backlight_link(struct drm_backlight *b,
+				 struct backlight_device *bd)
+{
+	if (bd != b->link) {
+		backlight_device_unref(b->link);
+		b->link = bd;
+		backlight_device_ref(b->link);
+		if (bd)
+			__drm_backlight_real_changed(b, bd->props.brightness);
+		b->changed = true;
+		__drm_backlight_schedule(b);
+	}
+}
+
+/* caller must hold @drm_backlight_lock */
+static void __drm_backlight_lookup(struct drm_backlight *b)
+{
+	struct backlight_device *bd;
+
+	if (b->link_name)
+		bd = backlight_device_lookup(b->link_name);
+	else
+		bd = NULL;
+
+	__drm_backlight_link(b, bd);
+	backlight_device_unref(bd);
+}
+
+/**
+ * drm_backlight_alloc - add backlight capability to a connector
+ * @connector: connector to add backlight to
+ *
+ * This allocates a new DRM-backlight device and links it to @connector. This
+ * *must* be called before registering the connector. The backlight device will
+ * be automatically registered in sync with the connector. It will also get
+ * removed once the connector is removed.
+ *
+ * The connector will not have any hardware backlight linked by default. You
+ * need to call drm_backlight_set_name() if you want to set a default
+ * backlight. User-space can overwrite those via sysfs.
+ *
+ * Returns: 0 on success, negative error code on failure.
+ */
+int drm_backlight_alloc(struct drm_connector *connector)
+{
+	struct drm_mode_config *config = &connector->dev->mode_config;
+	struct drm_backlight *b;
+
+	b = kzalloc(sizeof(*b), GFP_KERNEL);
+	if (!b)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&b->list);
+	INIT_WORK(&b->work, __drm_backlight_worker);
+	b->connector = connector;
+	connector->backlight = b;
+
+	drm_object_attach_property(&connector->base,
+				   config->brightness_property, U16_MAX);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_backlight_alloc);
+
+void drm_backlight_free(struct drm_connector *connector)
+{
+	struct drm_backlight *b = connector->backlight;
+
+	if (b) {
+		WARN_ON(__drm_backlight_is_registered(b));
+		WARN_ON(b->link);
+
+		kfree(b->link_name);
+		kfree(b);
+		connector->backlight = NULL;
+	}
+}
+
+void drm_backlight_register(struct drm_backlight *b)
+{
+	if (!b)
+		return;
+
+	WARN_ON(__drm_backlight_is_registered(b));
+
+	spin_lock(&drm_backlight_lock);
+	list_add(&b->list, &drm_backlight_list);
+	__drm_backlight_lookup(b);
+	spin_unlock(&drm_backlight_lock);
+}
+
+void drm_backlight_unregister(struct drm_backlight *b)
+{
+	if (!b)
+		return;
+
+	WARN_ON(!__drm_backlight_is_registered(b));
+
+	spin_lock(&drm_backlight_lock);
+	list_del_init(&b->list);
+	__drm_backlight_link(b, NULL);
+	spin_unlock(&drm_backlight_lock);
+
+	cancel_work_sync(&b->work);
+}
+
+/**
+ * drm_backlight_get_name - retrieve name of linked backlight device
+ * @b: DRM backlight to retrieve name of
+ * @buf: target buffer for name
+ * @max: size of the target buffer
+ *
+ * This retrieves the name of the backlight device linked to @b and writes it
+ * into @buf. If @buf is NULL or @max is 0, no name will be retrieved, but this
+ * function only tests whether a link is set.
+ * Otherwise, the name will always be written into @buf and will always be
+ * zero-terminated (truncated if too long).
+ *
+ * If no backlight device is linked to @b, this returns -ENOENT. Otherwise, the
+ * length of the written name (excluding the terminating 0 character) is
+ * returned.
+ * Note that if a device name has been set but the underlying backlight device
+ * does not exist, this will still return the linked name. -ENOENT is only
+ * returned if no device name has been set, yet (or has been cleared).
+ *
+ * Returns: On success the length of the written name, on failure a negative
+ *          error code.
+ */
+int drm_backlight_get_name(struct drm_backlight *b, char *buf, size_t max)
+{
+	int r;
+
+	spin_lock(&drm_backlight_lock);
+
+	if (!b->link_name) {
+		r = -ENOENT;
+		goto unlock;
+	}
+
+	r = 0;
+	if (buf && max > 0) {
+		r = strlen(b->link_name);
+		if (r + 1 > max)
+			r = max - 1;
+		buf[r] = 0;
+		memcpy(buf, b->link_name, r);
+	}
+
+unlock:
+	spin_unlock(&drm_backlight_lock);
+	return r;
+}
+EXPORT_SYMBOL(drm_backlight_get_name);
+
+/**
+ * drm_backlight_set_name - Change the device link of a DRM backlight
+ * @b: DRM backlight to modify
+ * @name: name of backlight device
+ *
+ * This changes the backlight device-link on @b to the hardware device with
+ * name @name. @name is stored on the backlight device, even if no such
+ * hardware device is registered, yet. If a backlight device appears later on,
+ * it will be automatically linked to all matching DRM backlight devices. If a
+ * real hardware backlight device already exists with such a name, it is linked
+ * with immediate effect.
+ *
+ * Whenever a real hardware backlight is linked or unlinked from a DRM connector
+ * an uevent with "BACKLIGHT=1" is generated on the connector.
+ *
+ * Returns: 0 on success, negative error code on failure.
+ */
+int drm_backlight_set_name(struct drm_backlight *b, const char *name)
+{
+	char *namecopy;
+
+	if (name && *name) {
+		namecopy = kstrdup(name, GFP_KERNEL);
+		if (!namecopy)
+			return -ENOMEM;
+	} else {
+		namecopy = NULL;
+	}
+
+	spin_lock(&drm_backlight_lock);
+
+	kfree(b->link_name);
+	b->link_name = namecopy;
+	if (__drm_backlight_is_registered(b))
+		__drm_backlight_lookup(b);
+
+	spin_unlock(&drm_backlight_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_backlight_set_name);
+
+void drm_backlight_set_brightness(struct drm_backlight *b, uint64_t value)
+{
+	spin_lock(&drm_backlight_lock);
+	__drm_backlight_prop_changed(b, value);
+	spin_unlock(&drm_backlight_lock);
+}
+
+static int drm_backlight_notify(struct notifier_block *self,
+				unsigned long event, void *data)
+{
+	struct backlight_device *bd = data;
+	struct drm_backlight *b;
+	const char *name;
+
+	spin_lock(&drm_backlight_lock);
+
+	switch (event) {
+	case BACKLIGHT_REGISTERED:
+		name = dev_name(&bd->dev);
+		if (!name)
+			break;
+
+		list_for_each_entry(b, &drm_backlight_list, list)
+			if (!b->link && !strcmp(name, b->link_name))
+				__drm_backlight_link(b, bd);
+
+		break;
+	case BACKLIGHT_UNREGISTERED:
+		list_for_each_entry(b, &drm_backlight_list, list)
+			if (b->link == bd)
+				__drm_backlight_link(b, NULL);
+
+		break;
+	}
+
+	spin_unlock(&drm_backlight_lock);
+
+	return 0;
+}
+
+static struct notifier_block drm_backlight_notifier = {
+	.notifier_call = drm_backlight_notify,
+};
+
+int drm_backlight_init(void)
+{
+	return backlight_register_notifier(&drm_backlight_notifier);
+}
+
+void drm_backlight_exit(void)
+{
+	backlight_unregister_notifier(&drm_backlight_notifier);
+}
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 2db7fb5..bb02c75 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -23,6 +23,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_connector.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_backlight.h>
 
 #include "drm_crtc_internal.h"
 #include "drm_internal.h"
@@ -324,6 +325,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
 	struct drm_device *dev = connector->dev;
 	struct drm_display_mode *mode, *t;
 
+	drm_backlight_free(connector);
 	/* The connector should have been removed from userspace long before
 	 * it is finally destroyed.
 	 */
@@ -379,6 +381,7 @@ int drm_connector_register(struct drm_connector *connector)
 	if (connector->registered)
 		return 0;
 
+	drm_backlight_register(connector->backlight);
 	ret = drm_sysfs_connector_add(connector);
 	if (ret)
 		return ret;
@@ -415,6 +418,8 @@ EXPORT_SYMBOL(drm_connector_register);
  */
 void drm_connector_unregister(struct drm_connector *connector)
 {
+	drm_backlight_unregister(connector->backlight);
+
 	if (!connector->registered)
 		return;
 
@@ -957,10 +962,16 @@ int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
 {
 	int ret = -EINVAL;
 	struct drm_connector *connector = obj_to_connector(obj);
+	struct drm_mode_config *config = &connector->dev->mode_config;
 
 	/* Do DPMS ourselves */
 	if (property == connector->dev->mode_config.dpms_property) {
 		ret = (*connector->funcs->dpms)(connector, (int)value);
+	} else if (property == config->brightness_property) {
+		if (connector->backlight)
+			drm_backlight_set_brightness(connector->backlight,
+						     value);
+		ret = 0;
 	} else if (connector->funcs->set_property)
 		ret = connector->funcs->set_property(connector, property, value);
 
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 13441e2..64f41ff 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -41,6 +41,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_auth.h>
 #include <drm/drm_debugfs_crc.h>
+#include <drm/drm_backlight.h>
 
 #include "drm_crtc_internal.h"
 #include "drm_internal.h"
@@ -451,6 +452,11 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.gamma_lut_size_property = prop;
 
+	prop = drm_property_create_range(dev, DRM_MODE_PROP_RANGE, "BRIGHTNESS", 0, U16_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.brightness_property = prop;
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 6efdba4..99553fe 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -33,6 +33,7 @@
 #include <linux/mount.h>
 #include <linux/slab.h>
 #include <drm/drmP.h>
+#include <drm/drm_backlight.h>
 #include "drm_crtc_internal.h"
 #include "drm_legacy.h"
 #include "drm_internal.h"
@@ -826,6 +827,7 @@ static const struct file_operations drm_stub_fops = {
 
 static void drm_core_exit(void)
 {
+	drm_backlight_exit();
 	unregister_chrdev(DRM_MAJOR, "drm");
 	debugfs_remove(drm_debugfs_root);
 	drm_sysfs_destroy();
@@ -855,6 +857,12 @@ static int __init drm_core_init(void)
 		goto error;
 	}
 
+	ret = drm_backlight_init();
+	if (ret < 0) {
+		DRM_ERROR("Cannot initialize backlight interface\n");
+		goto error;
+	}
+
 	ret = register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops);
 	if (ret < 0)
 		goto error;
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 9a37196..685211e 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -20,6 +20,7 @@
 
 #include <drm/drm_sysfs.h>
 #include <drm/drmP.h>
+#include <drm/drm_backlight.h>
 #include "drm_internal.h"
 
 #define to_drm_minor(d) dev_get_drvdata(d)
@@ -215,16 +216,69 @@ static ssize_t modes_show(struct device *device,
 	return written;
 }
 
+static ssize_t backlight_show(struct device *device,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct drm_connector *connector = to_drm_connector(device);
+	int r;
+
+	if (!connector->backlight)
+		return -ENOTSUPP;
+
+	r = drm_backlight_get_name(connector->backlight, buf, PAGE_SIZE);
+	if (r < 0)
+		return r;
+
+	if (r + 1 < PAGE_SIZE) {
+		buf[r++] = '\n';
+		buf[r] = 0;
+	}
+
+	return r;
+}
+
+static ssize_t backlight_store(struct device *device,
+			       struct device_attribute *attr,
+			       const char *buf, size_t size)
+{
+	struct drm_connector *connector = to_drm_connector(device);
+	const char *t;
+	char *name;
+	size_t len;
+	int r;
+
+	if (!connector->backlight)
+		return -ENOTSUPP;
+
+	t = strchrnul(buf, '\n');
+	len = t - buf;
+
+	name = kstrndup(buf, len, GFP_TEMPORARY);
+	if (!name)
+		return -ENOMEM;
+
+	r = drm_backlight_set_name(connector->backlight, name);
+	kfree(name);
+
+	if (r < 0)
+		return r;
+
+	return len;
+}
+
 static DEVICE_ATTR_RW(status);
 static DEVICE_ATTR_RO(enabled);
 static DEVICE_ATTR_RO(dpms);
 static DEVICE_ATTR_RO(modes);
+static DEVICE_ATTR_RW(backlight);
 
 static struct attribute *connector_dev_attrs[] = {
 	&dev_attr_status.attr,
 	&dev_attr_enabled.attr,
 	&dev_attr_dpms.attr,
 	&dev_attr_modes.attr,
+	&dev_attr_backlight.attr,
 	NULL
 };
 
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index c7b335c..c6303d1 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -113,6 +113,9 @@ static void backlight_generate_event(struct backlight_device *bd,
 	case BACKLIGHT_UPDATE_HOTKEY:
 		envp[0] = "SOURCE=hotkey";
 		break;
+	case BACKLIGHT_UPDATE_DRM:
+		envp[0] = "SOURCE=drm";
+		break;
 	default:
 		envp[0] = "SOURCE=unknown";
 		break;
diff --git a/include/drm/drm_backlight.h b/include/drm/drm_backlight.h
new file mode 100644
index 0000000..b34c6bf
--- /dev/null
+++ b/include/drm/drm_backlight.h
@@ -0,0 +1,44 @@
+#ifndef __DRM_BACKLIGHT_H__
+#define __DRM_BACKLIGHT_H__
+
+/*
+ * Copyright (c) 2014 David Herrmann <dh.herrmann at gmail.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+
+struct drm_backlight;
+struct drm_connector;
+
+int drm_backlight_init(void);
+void drm_backlight_exit(void);
+
+int drm_backlight_alloc(struct drm_connector *connector);
+void drm_backlight_free(struct drm_connector *connector);
+void drm_backlight_register(struct drm_backlight *b);
+void drm_backlight_unregister(struct drm_backlight *b);
+
+int drm_backlight_get_name(struct drm_backlight *b, char *buf, size_t max);
+int drm_backlight_set_name(struct drm_backlight *b, const char *name);
+void drm_backlight_set_brightness(struct drm_backlight *b, uint64_t value);
+
+#endif /* __DRM_BACKLIGHT_H__ */
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index ac9d7d8..28e5d65 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -682,6 +682,9 @@ struct drm_connector {
 	uint8_t num_h_tile, num_v_tile;
 	uint8_t tile_h_loc, tile_v_loc;
 	uint16_t tile_h_size, tile_v_size;
+
+	/* backlight link */
+	struct drm_backlight *backlight;
 };
 
 #define obj_to_connector(x) container_of(x, struct drm_connector, base)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 284c1b3..a70e4b4 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1140,6 +1140,11 @@ struct drm_mode_config {
 	 */
 	struct drm_property *dpms_property;
 	/**
+	 * @brightness_property: Default connector property to control the
+	 * connector's backlight brightness.
+	 */
+        struct drm_property *brightness_property;
+        /**
 	 * @path_property: Default connector property to hold the DP MST path
 	 * for the port.
 	 */
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 895c4661..4b9b8ca 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -31,6 +31,7 @@
 enum backlight_update_reason {
 	BACKLIGHT_UPDATE_HOTKEY,
 	BACKLIGHT_UPDATE_SYSFS,
+	BACKLIGHT_UPDATE_DRM,
 };
 
 enum backlight_type {
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/6] i915: Use drm backlight

[Marta Lofstedt]

Use the drm backlight class.

Signed-off-by: Marta Lofstedt <marta.lofstedt@intel.com>
---
 drivers/gpu/drm/i915/intel_panel.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index be4b4d5..0765b4c 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -34,6 +34,7 @@
 #include <linux/moduleparam.h>
 #include <linux/pwm.h>
 #include "intel_drv.h"
+#include <drm/drm_backlight.h>
 
 #define CRC_PMIC_PWM_PERIOD_NS	21333
 
@@ -1720,6 +1721,10 @@ int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe)
 
 	panel->backlight.present = true;
 
+	/* This connector has backlight attached */
+	if (!drm_backlight_alloc(connector))
+	        drm_backlight_set_name(connector->backlight, "intel_backlight");
+
 	DRM_DEBUG_KMS("Connector %s backlight initialized, %s, brightness %u/%u\n",
 		      connector->name,
 		      panel->backlight.enabled ? "enabled" : "disabled",
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/6] drm: drm_backlight use the connect value to set brightness property

[Marta Lofstedt]

The brightness property was set with the incoming value
instead of the calculated value.

Signed-off-by: Marta Lofstedt <marta.lofstedt@intel.com>
---
 drivers/gpu/drm/drm_backlight.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_backlight.c b/drivers/gpu/drm/drm_backlight.c
index 92d231a..e73ec8e 100644
--- a/drivers/gpu/drm/drm_backlight.c
+++ b/drivers/gpu/drm/drm_backlight.c
@@ -132,7 +132,7 @@ static void __drm_backlight_real_changed(struct drm_backlight *b, uint64_t v)
 		set = div_u64(v << 16, max);
 
 	drm_object_property_set_value(&b->connector->base,
-				      config->brightness_property, v);
+				      config->brightness_property, set);
 }
 
 /* caller must hold @drm_backlight_lock */
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/6] Rebase of David Herrmann drm connector link to backlight device

[Daniel Vetter]

On Mon, Oct 24, 2016 at 04:08:47PM +0300, Marta Lofstedt wrote:
> Hi David,
> 
> I am currently investigating:
> https://bugs.freedesktop.org/show_bug.cgi?id=96572
> 
> Martin Peres suggested that your patches:
> https://lists.freedesktop.org/archives/dri-devel/2014-September/thread.html#67984
> could solve the xf86-video-modesetting backlight issues.
> 
> I have rebased your patches and I am working on an IGT
> test for the functionality. With my i915 implementation
> and the small included bug-fix, I can update the drm BACKLIGHT
> property and the value is updated in the backlight class device.
> However, if I set the brigness value through the sysfs file of
> the backlight class device the drm BRIGHTNESS property does not
> update which would be confusing to users.
> 
> My understanding is that DRM properties are cached and, by design,
> do not have the capability to read the status from the driver.
> 
> What do we want to do about this?

Fix it I'd say. But right now there's no callback for drivers to return
non-ATOMIC properties. Only old property values are cached like this. I
guess we can add a get_property hook, which is optional, and which would
fall back to the cached value if need. A bit a pain to wire up, but should
get the job done. Maybe just start out with a callback for connectors
only, instead of all kms objects which can have a property attached.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel