[PATCH v3 0/5] drm/gma500: Backlight handling changes

[PATCH v3 0/5] drm/gma500: Backlight handling changes

[Hans de Goede]

Hi All,

Here is a patch-series changing gma500's backlight handling to match
the changes done to the other major x86 GPU drivers in the just landed
backlight detection refactor patch series:
https://lore.kernel.org/dri-devel/261afe3d-7790-e945-adf6-a2c96c9b1eff@redhat.com/

The main goal is here is to only register one backlight class device instead
of registering both "acpi_video0" and "psb-bl" backlight class devices;
in preparation for implementing the new backlight userspace-API from:
https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86d2e@redhat.com/

Changes in v2:
- Add "Use backlight_get_brightness() to get the brightness" patch

Changes in v3:
- Fix unused variable warnings when CONFIG_BACKLIGHT is not selected by
  marking the 2 variables as  __maybe_unused.

Regards,

Hans


Hans de Goede (5):
  drm/gma500: Refactor backlight support (v2)
  drm/gma500: Change registered backlight device type to raw/native
  drm/gma500: Use backlight_get_brightness() to get the brightness
  drm/gma500: Don't register backlight when another backlight should be
    used
  drm/gma500: Call acpi_video_register_backlight()

 drivers/gpu/drm/gma500/backlight.c       | 102 +++++++++++++++--------
 drivers/gpu/drm/gma500/cdv_device.c      |  50 ++---------
 drivers/gpu/drm/gma500/oaktrail_device.c |  65 ++-------------
 drivers/gpu/drm/gma500/opregion.c        |   6 +-
 drivers/gpu/drm/gma500/psb_device.c      |  73 +---------------
 drivers/gpu/drm/gma500/psb_drv.c         |  15 +---
 drivers/gpu/drm/gma500/psb_drv.h         |  13 +--
 7 files changed, 97 insertions(+), 227 deletions(-)

-- 
2.37.3

[PATCH v3 1/5] drm/gma500: Refactor backlight support (v2)

[Hans de Goede]

Refactor backlight support so that the gma_backlight_enable() /
gma_backlight_disable() / gma_backlight_set() functions used by
the Opregion handle will also work if no backlight_device gets
registered.

This is a preparation patch for not registering the gma500's own backlight
device when acpi_video should be used, since registering 2 backlight
devices for a single display really is undesirable.

Since the acpi-video interface often uses the OpRegion we need to keep
the OpRegion functional even when dev_priv->backlight_device is NULL.

As a result of this refactor the actual backlight_device_register()
call is moved to the shared backlight.c code and all #ifdefs related to
CONFIG_BACKLIGHT_CLASS_DEVICE are now also limited to backlight.c .

No functional changes intended.

This has been tested on a Packard Bell Dot SC (Intel Atom N2600, cedarview)
and a Sony Vaio vpc-x11s1e (Intel N540, poulsbo) laptop.

Changes in v2:
- Fix unused variable warnings when CONFIG_BACKLIGHT is not selected by
  marking the 2 variables as  __maybe_unused.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/gma500/backlight.c       | 94 +++++++++++++++---------
 drivers/gpu/drm/gma500/cdv_device.c      | 50 +++----------
 drivers/gpu/drm/gma500/oaktrail_device.c | 65 ++--------------
 drivers/gpu/drm/gma500/opregion.c        |  6 +-
 drivers/gpu/drm/gma500/psb_device.c      | 73 +-----------------
 drivers/gpu/drm/gma500/psb_drv.c         | 13 +---
 drivers/gpu/drm/gma500/psb_drv.h         | 13 ++--
 7 files changed, 87 insertions(+), 227 deletions(-)

diff --git a/drivers/gpu/drm/gma500/backlight.c b/drivers/gpu/drm/gma500/backlight.c
index 46b9c0f13d6d..b804325eb64a 100644
--- a/drivers/gpu/drm/gma500/backlight.c
+++ b/drivers/gpu/drm/gma500/backlight.c
@@ -13,69 +13,95 @@
 #include "intel_bios.h"
 #include "power.h"
 
-#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
-static void do_gma_backlight_set(struct drm_device *dev)
-{
-	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
-	backlight_update_status(dev_priv->backlight_device);
-}
-#endif
-
 void gma_backlight_enable(struct drm_device *dev)
 {
-#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
+
 	dev_priv->backlight_enabled = true;
-	if (dev_priv->backlight_device) {
-		dev_priv->backlight_device->props.brightness = dev_priv->backlight_level;
-		do_gma_backlight_set(dev);
-	}
-#endif	
+	dev_priv->ops->backlight_set(dev, dev_priv->backlight_level);
 }
 
 void gma_backlight_disable(struct drm_device *dev)
 {
-#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
+
 	dev_priv->backlight_enabled = false;
-	if (dev_priv->backlight_device) {
-		dev_priv->backlight_device->props.brightness = 0;
-		do_gma_backlight_set(dev);
-	}
-#endif
+	dev_priv->ops->backlight_set(dev, 0);
 }
 
 void gma_backlight_set(struct drm_device *dev, int v)
 {
-#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
+
 	dev_priv->backlight_level = v;
-	if (dev_priv->backlight_device && dev_priv->backlight_enabled) {
-		dev_priv->backlight_device->props.brightness = v;
-		do_gma_backlight_set(dev);
-	}
-#endif
+	if (dev_priv->backlight_enabled)
+		dev_priv->ops->backlight_set(dev, v);
 }
 
+static int gma_backlight_get_brightness(struct backlight_device *bd)
+{
+	struct drm_device *dev = bl_get_data(bd);
+	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
+
+	if (dev_priv->ops->backlight_get)
+		return dev_priv->ops->backlight_get(dev);
+
+	return dev_priv->backlight_level;
+}
+
+static int gma_backlight_update_status(struct backlight_device *bd)
+{
+	struct drm_device *dev = bl_get_data(bd);
+	int level = bd->props.brightness;
+
+	/* Percentage 1-100% being valid */
+	if (level < 1)
+		level = 1;
+
+	gma_backlight_set(dev, level);
+	return 0;
+}
+
+static const struct backlight_ops gma_backlight_ops __maybe_unused = {
+	.get_brightness = gma_backlight_get_brightness,
+	.update_status  = gma_backlight_update_status,
+};
+
 int gma_backlight_init(struct drm_device *dev)
 {
-#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
+	struct backlight_properties props __maybe_unused = {};
+	int ret;
+
 	dev_priv->backlight_enabled = true;
-	return dev_priv->ops->backlight_init(dev);
-#else
-	return 0;
+	dev_priv->backlight_level = 100;
+
+	ret = dev_priv->ops->backlight_init(dev);
+	if (ret)
+		return ret;
+
+#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
+	props.brightness = dev_priv->backlight_level;
+	props.max_brightness = PSB_MAX_BRIGHTNESS;
+	props.type = BACKLIGHT_PLATFORM;
+
+	dev_priv->backlight_device =
+		backlight_device_register(dev_priv->ops->backlight_name,
+					  dev->dev, dev,
+					  &gma_backlight_ops, &props);
+	if (IS_ERR(dev_priv->backlight_device))
+		return PTR_ERR(dev_priv->backlight_device);
 #endif
+
+	return 0;
 }
 
 void gma_backlight_exit(struct drm_device *dev)
 {
 #ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
-	if (dev_priv->backlight_device) {
-		dev_priv->backlight_device->props.brightness = 0;
-		backlight_update_status(dev_priv->backlight_device);
+
+	if (dev_priv->backlight_device)
 		backlight_device_unregister(dev_priv->backlight_device);
-	}
 #endif
 }
diff --git a/drivers/gpu/drm/gma500/cdv_device.c b/drivers/gpu/drm/gma500/cdv_device.c
index ce96234f3df2..3065596257e9 100644
--- a/drivers/gpu/drm/gma500/cdv_device.c
+++ b/drivers/gpu/drm/gma500/cdv_device.c
@@ -5,7 +5,6 @@
  *
  **************************************************************************/
 
-#include <linux/backlight.h>
 #include <linux/delay.h>
 
 #include <drm/drm.h>
@@ -62,14 +61,10 @@ static int cdv_output_init(struct drm_device *dev)
 	return 0;
 }
 
-#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
-
 /*
  *	Cedartrail Backlght Interfaces
  */
 
-static struct backlight_device *cdv_backlight_device;
-
 static int cdv_backlight_combination_mode(struct drm_device *dev)
 {
 	return REG_READ(BLC_PWM_CTL2) & PWM_LEGACY_MODE;
@@ -92,9 +87,8 @@ static u32 cdv_get_max_backlight(struct drm_device *dev)
 	return max;
 }
 
-static int cdv_get_brightness(struct backlight_device *bd)
+static int cdv_get_brightness(struct drm_device *dev)
 {
-	struct drm_device *dev = bl_get_data(bd);
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 	u32 val = REG_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
 
@@ -106,20 +100,13 @@ static int cdv_get_brightness(struct backlight_device *bd)
 		val *= lbpc;
 	}
 	return (val * 100)/cdv_get_max_backlight(dev);
-
 }
 
-static int cdv_set_brightness(struct backlight_device *bd)
+static void cdv_set_brightness(struct drm_device *dev, int level)
 {
-	struct drm_device *dev = bl_get_data(bd);
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
-	int level = bd->props.brightness;
 	u32 blc_pwm_ctl;
 
-	/* Percentage 1-100% being valid */
-	if (level < 1)
-		level = 1;
-
 	level *= cdv_get_max_backlight(dev);
 	level /= 100;
 
@@ -136,38 +123,18 @@ static int cdv_set_brightness(struct backlight_device *bd)
 	blc_pwm_ctl = REG_READ(BLC_PWM_CTL) & ~BACKLIGHT_DUTY_CYCLE_MASK;
 	REG_WRITE(BLC_PWM_CTL, (blc_pwm_ctl |
 				(level << BACKLIGHT_DUTY_CYCLE_SHIFT)));
-	return 0;
 }
 
-static const struct backlight_ops cdv_ops = {
-	.get_brightness = cdv_get_brightness,
-	.update_status  = cdv_set_brightness,
-};
-
 static int cdv_backlight_init(struct drm_device *dev)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
-	struct backlight_properties props;
-
-	memset(&props, 0, sizeof(struct backlight_properties));
-	props.max_brightness = 100;
-	props.type = BACKLIGHT_PLATFORM;
-
-	cdv_backlight_device = backlight_device_register("psb-bl",
-					NULL, (void *)dev, &cdv_ops, &props);
-	if (IS_ERR(cdv_backlight_device))
-		return PTR_ERR(cdv_backlight_device);
-
-	cdv_backlight_device->props.brightness =
-			cdv_get_brightness(cdv_backlight_device);
-	backlight_update_status(cdv_backlight_device);
-	dev_priv->backlight_device = cdv_backlight_device;
-	dev_priv->backlight_enabled = true;
+
+	dev_priv->backlight_level = cdv_get_brightness(dev);
+	cdv_set_brightness(dev, dev_priv->backlight_level);
+
 	return 0;
 }
 
-#endif
-
 /*
  *	Provide the Cedarview specific chip logic and low level methods
  *	for power management
@@ -613,9 +580,10 @@ const struct psb_ops cdv_chip_ops = {
 	.hotplug = cdv_hotplug_event,
 	.hotplug_enable = cdv_hotplug_enable,
 
-#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
 	.backlight_init = cdv_backlight_init,
-#endif
+	.backlight_get = cdv_get_brightness,
+	.backlight_set = cdv_set_brightness,
+	.backlight_name = "psb-bl",
 
 	.init_pm = cdv_init_pm,
 	.save_regs = cdv_save_display_registers,
diff --git a/drivers/gpu/drm/gma500/oaktrail_device.c b/drivers/gpu/drm/gma500/oaktrail_device.c
index f90e628cb482..2531959d3d77 100644
--- a/drivers/gpu/drm/gma500/oaktrail_device.c
+++ b/drivers/gpu/drm/gma500/oaktrail_device.c
@@ -5,7 +5,6 @@
  *
  **************************************************************************/
 
-#include <linux/backlight.h>
 #include <linux/delay.h>
 #include <linux/dmi.h>
 #include <linux/module.h>
@@ -37,29 +36,18 @@ static int oaktrail_output_init(struct drm_device *dev)
  *	Provide the low level interfaces for the Moorestown backlight
  */
 
-#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
-
 #define MRST_BLC_MAX_PWM_REG_FREQ	    0xFFFF
 #define BLC_PWM_PRECISION_FACTOR 100	/* 10000000 */
 #define BLC_PWM_FREQ_CALC_CONSTANT 32
 #define MHz 1000000
 #define BLC_ADJUSTMENT_MAX 100
 
-static struct backlight_device *oaktrail_backlight_device;
-static int oaktrail_brightness;
-
-static int oaktrail_set_brightness(struct backlight_device *bd)
+static void oaktrail_set_brightness(struct drm_device *dev, int level)
 {
-	struct drm_device *dev = bl_get_data(oaktrail_backlight_device);
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
-	int level = bd->props.brightness;
 	u32 blc_pwm_ctl;
 	u32 max_pwm_blc;
 
-	/* Percentage 1-100% being valid */
-	if (level < 1)
-		level = 1;
-
 	if (gma_power_begin(dev, 0)) {
 		/* Calculate and set the brightness value */
 		max_pwm_blc = REG_READ(BLC_PWM_CTL) >> 16;
@@ -82,19 +70,9 @@ static int oaktrail_set_brightness(struct backlight_device *bd)
 		REG_WRITE(BLC_PWM_CTL, (max_pwm_blc << 16) | blc_pwm_ctl);
 		gma_power_end(dev);
 	}
-	oaktrail_brightness = level;
-	return 0;
-}
-
-static int oaktrail_get_brightness(struct backlight_device *bd)
-{
-	/* return locally cached var instead of HW read (due to DPST etc.) */
-	/* FIXME: ideally return actual value in case firmware fiddled with
-	   it */
-	return oaktrail_brightness;
 }
 
-static int device_backlight_init(struct drm_device *dev)
+static int oaktrail_backlight_init(struct drm_device *dev)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 	unsigned long core_clock;
@@ -123,44 +101,11 @@ static int device_backlight_init(struct drm_device *dev)
 		REG_WRITE(BLC_PWM_CTL, value | (value << 16));
 		gma_power_end(dev);
 	}
-	return 0;
-}
-
-static const struct backlight_ops oaktrail_ops = {
-	.get_brightness = oaktrail_get_brightness,
-	.update_status  = oaktrail_set_brightness,
-};
 
-static int oaktrail_backlight_init(struct drm_device *dev)
-{
-	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
-	int ret;
-	struct backlight_properties props;
-
-	memset(&props, 0, sizeof(struct backlight_properties));
-	props.max_brightness = 100;
-	props.type = BACKLIGHT_PLATFORM;
-
-	oaktrail_backlight_device = backlight_device_register("oaktrail-bl",
-				NULL, (void *)dev, &oaktrail_ops, &props);
-
-	if (IS_ERR(oaktrail_backlight_device))
-		return PTR_ERR(oaktrail_backlight_device);
-
-	ret = device_backlight_init(dev);
-	if (ret < 0) {
-		backlight_device_unregister(oaktrail_backlight_device);
-		return ret;
-	}
-	oaktrail_backlight_device->props.brightness = 100;
-	oaktrail_backlight_device->props.max_brightness = 100;
-	backlight_update_status(oaktrail_backlight_device);
-	dev_priv->backlight_device = oaktrail_backlight_device;
+	oaktrail_set_brightness(dev, PSB_MAX_BRIGHTNESS);
 	return 0;
 }
 
-#endif
-
 /*
  *	Provide the Moorestown specific chip logic and low level methods
  *	for power management
@@ -545,9 +490,9 @@ const struct psb_ops oaktrail_chip_ops = {
 
 	.output_init = oaktrail_output_init,
 
-#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
 	.backlight_init = oaktrail_backlight_init,
-#endif
+	.backlight_set = oaktrail_set_brightness,
+	.backlight_name = "oaktrail-bl",
 
 	.save_regs = oaktrail_save_display_registers,
 	.restore_regs = oaktrail_restore_display_registers,
diff --git a/drivers/gpu/drm/gma500/opregion.c b/drivers/gpu/drm/gma500/opregion.c
index dc494df71a48..0c271072af63 100644
--- a/drivers/gpu/drm/gma500/opregion.c
+++ b/drivers/gpu/drm/gma500/opregion.c
@@ -150,21 +150,17 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 	struct opregion_asle *asle = dev_priv->opregion.asle;
-	struct backlight_device *bd = dev_priv->backlight_device;
 
 	DRM_DEBUG_DRIVER("asle set backlight %x\n", bclp);
 
 	if (!(bclp & ASLE_BCLP_VALID))
 		return ASLE_BACKLIGHT_FAILED;
 
-	if (bd == NULL)
-		return ASLE_BACKLIGHT_FAILED;
-
 	bclp &= ASLE_BCLP_MSK;
 	if (bclp > 255)
 		return ASLE_BACKLIGHT_FAILED;
 
-	gma_backlight_set(dev, bclp * bd->props.max_brightness / 255);
+	gma_backlight_set(dev, bclp * PSB_MAX_BRIGHTNESS / 255);
 
 	asle->cblv = (bclp * 0x64) / 0xff | ASLE_CBLV_VALID;
 
diff --git a/drivers/gpu/drm/gma500/psb_device.c b/drivers/gpu/drm/gma500/psb_device.c
index 71534f4ca834..3c294c38bdb4 100644
--- a/drivers/gpu/drm/gma500/psb_device.c
+++ b/drivers/gpu/drm/gma500/psb_device.c
@@ -5,8 +5,6 @@
  *
  **************************************************************************/
 
-#include <linux/backlight.h>
-
 #include <drm/drm.h>
 
 #include "gma_device.h"
@@ -24,8 +22,6 @@ static int psb_output_init(struct drm_device *dev)
 	return 0;
 }
 
-#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
-
 /*
  *	Poulsbo Backlight Interfaces
  */
@@ -41,18 +37,6 @@ static int psb_output_init(struct drm_device *dev)
 #define PSB_BACKLIGHT_PWM_POLARITY_BIT_CLEAR (0xFFFE)
 #define PSB_BACKLIGHT_PWM_CTL_SHIFT	(16)
 
-static int psb_brightness;
-static struct backlight_device *psb_backlight_device;
-
-static int psb_get_brightness(struct backlight_device *bd)
-{
-	/* return locally cached var instead of HW read (due to DPST etc.) */
-	/* FIXME: ideally return actual value in case firmware fiddled with
-	   it */
-	return psb_brightness;
-}
-
-
 static int psb_backlight_setup(struct drm_device *dev)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
@@ -86,62 +70,13 @@ static int psb_backlight_setup(struct drm_device *dev)
 		REG_WRITE(BLC_PWM_CTL,
 			(value << PSB_BACKLIGHT_PWM_CTL_SHIFT) | (value));
 	}
-	return 0;
-}
-
-static int psb_set_brightness(struct backlight_device *bd)
-{
-	struct drm_device *dev = bl_get_data(psb_backlight_device);
-	int level = bd->props.brightness;
-
-	/* Percentage 1-100% being valid */
-	if (level < 1)
-		level = 1;
-
-	psb_intel_lvds_set_brightness(dev, level);
-	psb_brightness = level;
-	return 0;
-}
-
-static const struct backlight_ops psb_ops = {
-	.get_brightness = psb_get_brightness,
-	.update_status  = psb_set_brightness,
-};
-
-static int psb_backlight_init(struct drm_device *dev)
-{
-	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
-	int ret;
-	struct backlight_properties props;
-
-	memset(&props, 0, sizeof(struct backlight_properties));
-	props.max_brightness = 100;
-	props.type = BACKLIGHT_PLATFORM;
-
-	psb_backlight_device = backlight_device_register("psb-bl",
-					NULL, (void *)dev, &psb_ops, &props);
-	if (IS_ERR(psb_backlight_device))
-		return PTR_ERR(psb_backlight_device);
-
-	ret = psb_backlight_setup(dev);
-	if (ret < 0) {
-		backlight_device_unregister(psb_backlight_device);
-		psb_backlight_device = NULL;
-		return ret;
-	}
-	psb_backlight_device->props.brightness = 100;
-	psb_backlight_device->props.max_brightness = 100;
-	backlight_update_status(psb_backlight_device);
-	dev_priv->backlight_device = psb_backlight_device;
 
+	psb_intel_lvds_set_brightness(dev, PSB_MAX_BRIGHTNESS);
 	/* This must occur after the backlight is properly initialised */
 	psb_lid_timer_init(dev_priv);
-
 	return 0;
 }
 
-#endif
-
 /*
  *	Provide the Poulsbo specific chip logic and low level methods
  *	for power management
@@ -345,9 +280,9 @@ const struct psb_ops psb_chip_ops = {
 
 	.output_init = psb_output_init,
 
-#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
-	.backlight_init = psb_backlight_init,
-#endif
+	.backlight_init = psb_backlight_setup,
+	.backlight_set = psb_intel_lvds_set_brightness,
+	.backlight_name = "psb-bl",
 
 	.init_pm = psb_init_pm,
 	.save_regs = psb_save_display_registers,
diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index 9168bc620628..d3dc16193a29 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -169,8 +169,7 @@ static void psb_driver_unload(struct drm_device *dev)
 
 	/* TODO: Kill vblank etc here */
 
-	if (dev_priv->backlight_device)
-		gma_backlight_exit(dev);
+	gma_backlight_exit(dev);
 	psb_modeset_cleanup(dev);
 
 	gma_irq_uninstall(dev);
@@ -415,16 +414,6 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags)
 	return ret;
 }
 
-static inline void get_brightness(struct backlight_device *bd)
-{
-#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
-	if (bd) {
-		bd->props.brightness = bd->ops->get_brightness(bd);
-		backlight_update_status(bd);
-	}
-#endif
-}
-
 static int psb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	struct drm_psb_private *dev_priv;
diff --git a/drivers/gpu/drm/gma500/psb_drv.h b/drivers/gpu/drm/gma500/psb_drv.h
index 049701ce8a5f..ae544b69fc47 100644
--- a/drivers/gpu/drm/gma500/psb_drv.h
+++ b/drivers/gpu/drm/gma500/psb_drv.h
@@ -172,6 +172,8 @@
 #define PSB_WATCHDOG_DELAY (HZ * 2)
 #define PSB_LID_DELAY (HZ / 10)
 
+#define PSB_MAX_BRIGHTNESS		100
+
 #define PSB_PWR_STATE_ON		1
 #define PSB_PWR_STATE_OFF		2
 
@@ -522,10 +524,6 @@ struct drm_psb_private {
 
 	struct drm_fb_helper *fb_helper;
 
-	/* Panel brightness */
-	int brightness;
-	int brightness_adjusted;
-
 	bool dsr_enable;
 	u32 dsr_fb_update;
 	bool dpi_panel_on[3];
@@ -594,10 +592,13 @@ struct psb_ops {
 	void (*disable_sr)(struct drm_device *dev);
 
 	void (*lvds_bl_power)(struct drm_device *dev, bool on);
-#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
+
 	/* Backlight */
 	int (*backlight_init)(struct drm_device *dev);
-#endif
+	void (*backlight_set)(struct drm_device *dev, int level);
+	int (*backlight_get)(struct drm_device *dev);
+	const char *backlight_name;
+
 	int i2c_bus;		/* I2C bus identifier for Moorestown */
 };
 
-- 
2.37.3

[PATCH v3 2/5] drm/gma500: Change registered backlight device type to raw/native

[Hans de Goede]

Change the type for the registered backlight class device from platform
to raw/native.

The poulsbo/cedarview/oaktrail backlight support is using native GPU
backlight control and as such the type should be raw (aka native) as
is done by all the other native GPU backlight driver code.

Note this will not change much from userspace's point of view.
poulsbo/cedarview laptops typically offer both an ACPI-video
backlight interface as well as the native GPU backlight interface.

The /sys/class/backlight/acpi_video0 has a type of firmware and
userspace typically looks for firmware devices before looking
for platform devices. The typical standard lookup order is:
firmware -> platform -> raw

This means that both before and after this change typical userspace
backlight consumers (sich as e.g. GNOME) will prefer the firmware
acpi_video0 backlight device.

This has been tested on a Packard Bell Dot SC (Intel Atom N2600, cedarview)
and a Sony Vaio vpc-x11s1e (Intel N540, poulsbo) laptop.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/gma500/backlight.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/gma500/backlight.c b/drivers/gpu/drm/gma500/backlight.c
index b804325eb64a..5a40414e5748 100644
--- a/drivers/gpu/drm/gma500/backlight.c
+++ b/drivers/gpu/drm/gma500/backlight.c
@@ -83,7 +83,7 @@ int gma_backlight_init(struct drm_device *dev)
 #ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
 	props.brightness = dev_priv->backlight_level;
 	props.max_brightness = PSB_MAX_BRIGHTNESS;
-	props.type = BACKLIGHT_PLATFORM;
+	props.type = BACKLIGHT_RAW;
 
 	dev_priv->backlight_device =
 		backlight_device_register(dev_priv->ops->backlight_name,
-- 
2.37.3

[PATCH v3 3/5] drm/gma500: Use backlight_get_brightness() to get the brightness

[Hans de Goede]

Use backlight_get_brightness() instead of directly referencing
bd->props.brightness. This will take backlight_is_blank() into account,
properly setting brightness to 0 when screen-blanking has been requested
through the backlight sysfs interface.

Suggested-by: Sam Ravnborg <sam@ravnborg.org>
Acked-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2 of the patch-set:
- New patch in v2 of the patch-set
---
 drivers/gpu/drm/gma500/backlight.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/gma500/backlight.c b/drivers/gpu/drm/gma500/backlight.c
index 5a40414e5748..e85fd181f2f3 100644
--- a/drivers/gpu/drm/gma500/backlight.c
+++ b/drivers/gpu/drm/gma500/backlight.c
@@ -52,7 +52,7 @@ static int gma_backlight_get_brightness(struct backlight_device *bd)
 static int gma_backlight_update_status(struct backlight_device *bd)
 {
 	struct drm_device *dev = bl_get_data(bd);
-	int level = bd->props.brightness;
+	int level = backlight_get_brightness(bd);
 
 	/* Percentage 1-100% being valid */
 	if (level < 1)
-- 
2.37.3

[PATCH v3 4/5] drm/gma500: Don't register backlight when another backlight should be used

[Hans de Goede]

Before this commit when we want userspace to use the acpi_video backlight
device we register both the GPU's native backlight device and acpi_video's
firmware acpi_video# backlight device. This relies on userspace preferring
firmware type backlight devices over native ones.

Registering 2 backlight devices for a single display really is
undesirable, don't register the GPU's native backlight device when
another backlight device should be used.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/gma500/backlight.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/gma500/backlight.c b/drivers/gpu/drm/gma500/backlight.c
index e85fd181f2f3..577a4987b193 100644
--- a/drivers/gpu/drm/gma500/backlight.c
+++ b/drivers/gpu/drm/gma500/backlight.c
@@ -7,6 +7,8 @@
  * Authors: Eric Knopp
  */
 
+#include <acpi/video.h>
+
 #include "psb_drv.h"
 #include "psb_intel_reg.h"
 #include "psb_intel_drv.h"
@@ -80,6 +82,12 @@ int gma_backlight_init(struct drm_device *dev)
 	if (ret)
 		return ret;
 
+	if (!acpi_video_backlight_use_native()) {
+		drm_info(dev, "Skipping %s backlight registration\n",
+			 dev_priv->ops->backlight_name);
+		return 0;
+	}
+
 #ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
 	props.brightness = dev_priv->backlight_level;
 	props.max_brightness = PSB_MAX_BRIGHTNESS;
-- 
2.37.3

[PATCH v3 5/5] drm/gma500: Call acpi_video_register_backlight()

[Hans de Goede]

On machines without an Intel video opregion the acpi_video driver
immediately probes the ACPI video bus and used to also immediately
register acpi_video# backlight devices when supported.

Once the drm/kms driver then loaded later and possibly registered
a native backlight device then the drivers/acpi/video_detect.c code
unregistered the acpi_video0 device to avoid there being 2 backlight
devices (when acpi_video_get_backlight_type()==native).

This means that userspace used to briefly see 2 devices and the
disappearing of acpi_video0 after a brief time confuses the systemd
backlight level save/restore code, see e.g.:
https://bbs.archlinux.org/viewtopic.php?id=269920

To fix this the ACPI video code has been modified to make backlight class
device registration a separate step, relying on the drm/kms driver to
ask for the acpi_video backlight registration after it is done setting up
its native backlight device.

Add a call to the new acpi_video_register_backlight() function after
setting up the gma500's native backlight, so that the acpi_video backlight
device gets registered on systems where the gma500's native backlight
device is not registered.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/gma500/psb_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index d3dc16193a29..cd9c73f5a64a 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -398,6 +398,8 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags)
 		if (gma_encoder->type == INTEL_OUTPUT_LVDS ||
 		    gma_encoder->type == INTEL_OUTPUT_MIPI) {
 			ret = gma_backlight_init(dev);
+			if (ret == 0)
+				acpi_video_register_backlight();
 			break;
 		}
 	}
-- 
2.37.3

Re: [PATCH v3 0/5] drm/gma500: Backlight handling changes

[Patrik Jakobsson]

On Sat, Sep 17, 2022 at 10:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> Here is a patch-series changing gma500's backlight handling to match
> the changes done to the other major x86 GPU drivers in the just landed
> backlight detection refactor patch series:
> https://lore.kernel.org/dri-devel/261afe3d-7790-e945-adf6-a2c96c9b1eff@redhat.com/
>
> The main goal is here is to only register one backlight class device instead
> of registering both "acpi_video0" and "psb-bl" backlight class devices;
> in preparation for implementing the new backlight userspace-API from:
> https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86d2e@redhat.com/

Hi Hans,

Quite some time ago I wrote a backlight driver [1] for a MacBook to
work around an issue in the i915 driver. My driver spoke directly to
an external backlight driver chip. By doing so I could turn off the
signal coming from the GPU and instead program my own PWM value
directly. I remember it being a bit tricky to make my driver get
priority over the i915 driver. Not sure what the actual issue was but
I did get it to work properly in the end (perhaps with an xorg.conf
change).

I understand that this is a corner case but I'm just curious how/if
this can be handled with the new API. Is it possible to kick out an
existing non-acpi backlight driver if you know yours is better?

[1] https://github.com/patjak/mba6x_bl

>
> Changes in v2:
> - Add "Use backlight_get_brightness() to get the brightness" patch
>
> Changes in v3:
> - Fix unused variable warnings when CONFIG_BACKLIGHT is not selected by
>   marking the 2 variables as  __maybe_unused.

This looks good to me. I don't have access to my DIM setup in a couple
of days so please push these yourself if possible.

For the entire series:
Acked-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>

>
> Regards,
>
> Hans
>
>
> Hans de Goede (5):
>   drm/gma500: Refactor backlight support (v2)
>   drm/gma500: Change registered backlight device type to raw/native
>   drm/gma500: Use backlight_get_brightness() to get the brightness
>   drm/gma500: Don't register backlight when another backlight should be
>     used
>   drm/gma500: Call acpi_video_register_backlight()
>
>  drivers/gpu/drm/gma500/backlight.c       | 102 +++++++++++++++--------
>  drivers/gpu/drm/gma500/cdv_device.c      |  50 ++---------
>  drivers/gpu/drm/gma500/oaktrail_device.c |  65 ++-------------
>  drivers/gpu/drm/gma500/opregion.c        |   6 +-
>  drivers/gpu/drm/gma500/psb_device.c      |  73 +---------------
>  drivers/gpu/drm/gma500/psb_drv.c         |  15 +---
>  drivers/gpu/drm/gma500/psb_drv.h         |  13 +--
>  7 files changed, 97 insertions(+), 227 deletions(-)
>
> --
> 2.37.3
>

Re: [PATCH v3 0/5] drm/gma500: Backlight handling changes

[Sam Ravnborg]

Hi Hans,

> > Changes in v3:
> > - Fix unused variable warnings when CONFIG_BACKLIGHT is not selected by
> >   marking the 2 variables as  __maybe_unused.
> 
> This looks good to me. I don't have access to my DIM setup in a couple
> of days so please push these yourself if possible.
> 
> For the entire series:
> Acked-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>

I did not find time today - sorry.
I you want to wait then I can take a look in the weekend - but not until
then.

	Sam

Re: [PATCH v3 0/5] drm/gma500: Backlight handling changes

[Hans de Goede]

Hi Patrik,

On 9/18/22 20:22, Patrik Jakobsson wrote:
> On Sat, Sep 17, 2022 at 10:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi All,
>>
>> Here is a patch-series changing gma500's backlight handling to match
>> the changes done to the other major x86 GPU drivers in the just landed
>> backlight detection refactor patch series:
>> https://lore.kernel.org/dri-devel/261afe3d-7790-e945-adf6-a2c96c9b1eff@redhat.com/
>>
>> The main goal is here is to only register one backlight class device instead
>> of registering both "acpi_video0" and "psb-bl" backlight class devices;
>> in preparation for implementing the new backlight userspace-API from:
>> https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86d2e@redhat.com/
> 
> Hi Hans,
> 
> Quite some time ago I wrote a backlight driver [1] for a MacBook to
> work around an issue in the i915 driver. My driver spoke directly to
> an external backlight driver chip. By doing so I could turn off the
> signal coming from the GPU and instead program my own PWM value
> directly. I remember it being a bit tricky to make my driver get
> priority over the i915 driver. Not sure what the actual issue was but
> I did get it to work properly in the end (perhaps with an xorg.conf
> change).
> 
> I understand that this is a corner case but I'm just curious how/if
> this can be handled with the new API. Is it possible to kick out an
> existing non-acpi backlight driver if you know yours is better?
> 
> [1] https://github.com/patjak/mba6x_bl

After the main refactoring series which is in linux-next now:
https://lore.kernel.org/all/20220825143726.269890-1-hdegoede@redhat.com/
this should be possible, it should be easy even.

On x86/ACPI platforms the idea is that all backlight-drivers there
call acpi_video_get_backlight_type() which returns one of:

enum acpi_backlight_type {
        acpi_backlight_none = 0,
        acpi_backlight_video,
        acpi_backlight_vendor,
        acpi_backlight_native,
        acpi_backlight_nvidia_wmi_ec,
        acpi_backlight_apple_gmux,
};

(defined in include/acpi/video.h)

And then when acpi_video_get_backlight_type() returns a type which
does not match the driver which calls it, then that driver will
return without registering its backlight sysfs interface.

E.g. drivers/acpi/acpi_video.c does:

        if (acpi_video_get_backlight_type() != acpi_backlight_video)
                return 0;

before registering the /sys/class/backlight/acpi_video# interface(s).

And likewise the i915 driver now does:

        if (!acpi_video_backlight_use_native())
                return 0;

(Note native (GPU) backlight drivers use a special helper since that
helper also lets the video-detect code now that the GPU driver is
capable of doing native backlight control which is part of the heuristics
to pick which backlight control method to use).

So getting i915 out of the way now is as simple as making
acpi_video_get_backlight_type() return something else then 
acpi_backlight_native which can be done through e.g. a DMI quirk.

The acpi_backlight_vendor type here is typically used by backlight code
in drivers like dell-laptop acer-wmi, asus-wmi, thinkpad_acpi, etc.
None of which I expect to load on your macbook.

So you could make your mba6x_bl have a:

        if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
                return 0;

Check to avoid registering. All backlight drivers used on x86/acpi
platforms should have such a check going forward so that
acpi_video_get_backlight_type() is the sole place in the kernel
which decides which backlight-control method actually gets registered.

And then pass: "acpi_backlight=vendor" on the kernel commandline to
switch from the default to your driver (and users can also use this
to switch back once you have made vendor the default on affected
macbooks).

If this works (which I expect it will) and once you have your driver
merged into the mainline kernel you can then add DMI based quirks to
drivers/acpi/video_detect.c to default to acpi_backlight=vendor on
these macbooks (I see that you already use DMI based auto-loading
in your driver).

So as you can hopefully see in linux-next with my refactoring in
place having another driver take over from the i915 driver should
be simple since there is a single point now
(acpi_video_get_backlight_type()) which controls which driver will
load and which ones will not load.

Regards,

Hans


p.s.

Note the above has nothing to do with the new userspace API for
backlight control. But sorting out there being multiple drivers
loaded/registered at the same time for a single panel/backlight
was a prerequisite for the new userspace API.
For the proposed new userspace API see:
https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86d2e@redhat.com/









> 
>>
>> Changes in v2:
>> - Add "Use backlight_get_brightness() to get the brightness" patch
>>
>> Changes in v3:
>> - Fix unused variable warnings when CONFIG_BACKLIGHT is not selected by
>>   marking the 2 variables as  __maybe_unused.
> 
> This looks good to me. I don't have access to my DIM setup in a couple
> of days so please push these yourself if possible.
> 
> For the entire series:
> Acked-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
> 
>>
>> Regards,
>>
>> Hans
>>
>>
>> Hans de Goede (5):
>>   drm/gma500: Refactor backlight support (v2)
>>   drm/gma500: Change registered backlight device type to raw/native
>>   drm/gma500: Use backlight_get_brightness() to get the brightness
>>   drm/gma500: Don't register backlight when another backlight should be
>>     used
>>   drm/gma500: Call acpi_video_register_backlight()
>>
>>  drivers/gpu/drm/gma500/backlight.c       | 102 +++++++++++++++--------
>>  drivers/gpu/drm/gma500/cdv_device.c      |  50 ++---------
>>  drivers/gpu/drm/gma500/oaktrail_device.c |  65 ++-------------
>>  drivers/gpu/drm/gma500/opregion.c        |   6 +-
>>  drivers/gpu/drm/gma500/psb_device.c      |  73 +---------------
>>  drivers/gpu/drm/gma500/psb_drv.c         |  15 +---
>>  drivers/gpu/drm/gma500/psb_drv.h         |  13 +--
>>  7 files changed, 97 insertions(+), 227 deletions(-)
>>
>> --
>> 2.37.3
>>
> 

Re: [PATCH v3 0/5] drm/gma500: Backlight handling changes

[Hans de Goede]

Hi Sam,

On 9/18/22 20:51, Sam Ravnborg wrote:
> Hi Hans,
> 
>>> Changes in v3:
>>> - Fix unused variable warnings when CONFIG_BACKLIGHT is not selected by
>>>   marking the 2 variables as  __maybe_unused.
>>
>> This looks good to me. I don't have access to my DIM setup in a couple
>> of days so please push these yourself if possible.
>>
>> For the entire series:
>> Acked-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
> 
> I did not find time today - sorry.

No worries.

> I you want to wait then I can take a look in the weekend - but not until
> then.

I just got an Ack from Patrik for these and I'm going to push them
to drm-misc-next now. So another review is not necessary, thank you for
the offer though!

Regards,

Hans