Re: [PATCH v2] drm/tinydrm: Move backlight helpers to a separate file

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Den 26.09.2017 15.06, skrev Noralf Trønnes:

Den 26.09.2017 13.32, skrev Daniel Vetter:
On Tue, Sep 26, 2017 at 04:46:53PM +0530, Meghana Madhyastha wrote:
On Mon, Sep 25, 2017 at 06:31:58PM +0200, Noralf Trønnes wrote:
Den 25.09.2017 16.56, skrev Noralf Trønnes:
Hi Meghana,


Den 22.09.2017 17.09, skrev Meghana Madhyastha:
Move backlight helpers from tinydrm-helpers.c to
tinydrm-backlight.c. This is because it is organizationally
simpler to understand and advantageous to group functions
performing a similar function to a separate file as opposed to
having one helper file with heteregenous helper functions.

Signed-off-by: Meghana Madhyastha <meghana.madhyastha@xxxxxxxxx>
---
I don't think there is much gain in just moving the code like this.

The idea is to add a drm_backlight helper that can be useful for all
DRM drivers that use the backlight subsystem.
Yes I agree. That definitely makes more sense.
The full path to that helper would be:
drivers/gpu/drm/drm_backlight.c

This is what the TODO says:
https://dri.freedesktop.org/docs/drm/gpu/todo.html#tinydrm

- backlight helpers, probably best to put them into a new drm_backlight.c.    This is because drivers/video is de-facto unmaintained. We could also    move drivers/video/backlight to drivers/gpu/backlight and take it all
   over within drm-misc, but that’s more work.

There is also this discussion to take into account:
KMS backlight ABI proposition
https://lists.freedesktop.org/archives/dri-devel/2017-February/133206.html


I don't remember what came out of that discussion.

Noralf.
After having discussed this with Daniel on the #dri-devel irc channel,
here are some of the points suggested.

Daniel suggested that I first look into the usage of shared backlight
helpers in drm (specifically backlight_update_status to begin with). The idea was to see whether there is any pattern in usage and/or code dupication. If there is, then the next step would be to write helper functions which
can be used by other drivers (and not just tinydrm).

To start with, I went through the instances of backlight_update_status
in the drm code, and made the following observations(most of them are
very simple/naive observations).

- backlight_update_status is usually called in backlight init (and
   sometimes exit) functions of the drivers just after the device is registered.    So backlight_update_status is called with the registered device as the
   parameter.

Here are the following cases of properties changed/set before
backlight_update_status is called.

- CASE 1: Brightness is changed (either a macro BRIGHTNESS_MAX_LEVEL 100    is defined or it is manually set) This happens in the following files:

   gma500/cdv_device.c, gma500/mdfld_device.c, gma500/oaktrail_device.c,    gma500/psb_device.c, noveau/noveau_backlight.c(here brightness is determined by fuction
   static int nv50_get_intensity)

- CASE 2: Power property is set (to FB_BLANK_UNBLANK mostly)
   This happens in the following files:

   omapdrm/displays/panel-dpi.c, panel/panel-innolux-p079zca.c,
   panel/panel-jdi-lt070me05000.c, panel/panel-sharp-lq101r1sx01.c,
   panel/panel-sharp-ls043t1le01.c, tilcdc/tilcdc_panel.c
   - CASE 3: State is set
   This happens in the following files:

   tinydrm/tinydrm-helpers.c

- CASE 4: Power and brightness properties are set
   This happens in the following files:

   atombios_encoders.c, radeon/radeon_legacy_encoders.c,
   shmobile/shmob_drm_backlight.c

- CASE 5: Power and the state properties are set
   This happens in the following files:

   panel/panel-lvds.c, panel/panel-panasonic-vvx10f034n00.c,
   panel/panel-simple.c, panel/panel-sitronix-st7789v.c

Please let me know if I am wrong / missed something. As for next steps,
wouldn't it be an overkill to have a separate helper function for each
of these cases ? Perhaps a generic helper function which would somehow
address these cases would be more appropriate ? Thank you for your
time/patience.
I suspect that a lot of these combinations are just plain wrong, but
happen to kinda work with the combination of gpu driver and backlight
driver they're used on. tbh I have no idea which one is the correct
version for enabling a backlight correctly ...

So definitely a good task to refactor this into a proper helper, but looks
a lot more involved than at first sight.

Backlight is tricky.

One annoying thing from a DRM point of view is that it's tied to fbdev
with a notifier fb_notifier_callback() that turns it off on
FB_EVENT_BLANK and FB_EVENT_CONBLANK. And the logic in that function is
very convoluted.

And if we look at the gpio backlight driver, we see that there are
3 properties that turn off the backlight:

static int gpio_backlight_update_status(struct backlight_device *bl)
{
    struct gpio_backlight *gbl = bl_get_data(bl);
    int brightness = bl->props.brightness;

    if (bl->props.power != FB_BLANK_UNBLANK ||
        bl->props.fb_blank != FB_BLANK_UNBLANK ||
        bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
        brightness = 0;

    gpiod_set_value_cansleep(gbl->gpiod, brightness);

    return 0;
}

This may account for the different ways of controlling backlight in DRM.

Some other aspects:
- The backlight device has it's own suspend/resume functions.
- systemd tries to be smart and turn on/off backlight, but that is just
  annoying with tinydrm drivers that are loaded late since it turns on
  an uninitialized display. Disabling the service fixes it.


And some more:
The backlight driver is probed before the DRM driver and if DRM should
control backlight, then backlight should be initially off. The pwm_bl
driver has a default-brightness-level property that is set when the
pwm_bl driver probes. If the DRM driver should control backlight, then
this has to be zero/off, or the backlight is turned on before the
display is initialized. So how can we now communicate the default
brightness level to the pwm_bl driver?

The gpio_backlight driver is off by default.

I noticed that backlight is listed under leds in DT bindings now.

Noralf.

Do you have any of the hardware supported by any of these drivers? lsmod
and then comparing with the modules you're building in your own tree
should help you figure this out.
-Daniel

-Meghana

Changes in v2:
   -Improved commit message by explaining why the changes were made.

   drivers/gpu/drm/tinydrm/core/Makefile            | 2 +-
   drivers/gpu/drm/tinydrm/core/tinydrm-backlight.c | 103
+++++++++++++++++++++++
   drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c   | 94
---------------------
   drivers/gpu/drm/tinydrm/mi0283qt.c               | 1 +
   drivers/gpu/drm/tinydrm/mipi-dbi.c               | 1 +
   include/drm/tinydrm/tinydrm-backlight.h          | 18 ++++
   6 files changed, 124 insertions(+), 95 deletions(-)
   create mode 100644 drivers/gpu/drm/tinydrm/core/tinydrm-backlight.c
   create mode 100644 include/drm/tinydrm/tinydrm-backlight.h

diff --git a/drivers/gpu/drm/tinydrm/core/Makefile
b/drivers/gpu/drm/tinydrm/core/Makefile
index fb221e6..389ca7a 100644
--- a/drivers/gpu/drm/tinydrm/core/Makefile
+++ b/drivers/gpu/drm/tinydrm/core/Makefile
@@ -1,3 +1,3 @@
-tinydrm-y := tinydrm-core.o tinydrm-pipe.o tinydrm-helpers.o
+tinydrm-y := tinydrm-core.o tinydrm-pipe.o tinydrm-backlight.o
tinydrm-helpers.o
     obj-$(CONFIG_DRM_TINYDRM) += tinydrm.o
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-backlight.c
b/drivers/gpu/drm/tinydrm/core/tinydrm-backlight.c
new file mode 100644
index 0000000..dc6f17d
--- /dev/null
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-backlight.c
@@ -0,0 +1,103 @@
+#include <linux/backlight.h>
+#include <linux/dma-buf.h>
+#include <linux/pm.h>
+#include <linux/swab.h>
+
+#include <drm/tinydrm/tinydrm.h>
+#include <drm/tinydrm/tinydrm-backlight.h>
+
+/**
+ * tinydrm_of_find_backlight - Find backlight device in device-tree
+ * @dev: Device
+ *
+ * This function looks for a DT node pointed to by a property named
'backlight'
+ * and uses of_find_backlight_by_node() to get the backlight device.
+ * Additionally if the brightness property is zero, it is set to
+ * max_brightness.
+ *
+ * Returns:
+ * NULL if there's no backlight property.
+ * Error pointer -EPROBE_DEFER if the DT node is found, but no
backlight device
+ * is found.
+ * If the backlight device is found, a pointer to the structure is
returned.
+ */
+
+struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
+{
+    struct backlight_device *backlight;
+    struct device_node *np;
+
+    np = of_parse_phandle(dev->of_node, "backlight", 0);
+    if (!np)
+        return NULL;
+
+    backlight = of_find_backlight_by_node(np);
+    of_node_put(np);
+
+    if (!backlight)
+        return ERR_PTR(-EPROBE_DEFER);
+
+    if (!backlight->props.brightness) {
+        backlight->props.brightness = backlight->props.max_brightness;
+        DRM_DEBUG_KMS("Backlight brightness set to %d\n",
+                  backlight->props.brightness);
+    }
+
+    return backlight;
+}
+EXPORT_SYMBOL(tinydrm_of_find_backlight);
+
+/**
+ * tinydrm_enable_backlight - Enable backlight helper
+ * @backlight: Backlight device
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int tinydrm_enable_backlight(struct backlight_device *backlight)
+{
+    unsigned int old_state;
+    int ret;
+
+    if (!backlight)
+        return 0;
+
+    old_state = backlight->props.state;
+    backlight->props.state &= ~BL_CORE_FBBLANK;
+    DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,
+              backlight->props.state);
+
+    ret = backlight_update_status(backlight);
+    if (ret)
+        DRM_ERROR("Failed to enable backlight %d\n", ret);
+
+    return ret;
+}
+EXPORT_SYMBOL(tinydrm_enable_backlight);
+
+/**
+ * tinydrm_disable_backlight - Disable backlight helper
+ * @backlight: Backlight device
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int tinydrm_disable_backlight(struct backlight_device *backlight)
+{
+    unsigned int old_state;
+    int ret;
+
+    if (!backlight)
+        return 0;
+
+    old_state = backlight->props.state;
+    backlight->props.state |= BL_CORE_FBBLANK;
+    DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,
+              backlight->props.state);
+    ret = backlight_update_status(backlight);
+    if (ret)
+        DRM_ERROR("Failed to disable backlight %d\n", ret);
+
+    return ret;
+}
+EXPORT_SYMBOL(tinydrm_disable_backlight);
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
index bd6cce0..ee8ad8c 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
@@ -236,100 +236,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void
*vaddr, struct drm_framebuffer *fb,
   }
   EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
   -/**
- * tinydrm_of_find_backlight - Find backlight device in device-tree
- * @dev: Device
- *
- * This function looks for a DT node pointed to by a property named
'backlight'
- * and uses of_find_backlight_by_node() to get the backlight device.
- * Additionally if the brightness property is zero, it is set to
- * max_brightness.
- *
- * Returns:
- * NULL if there's no backlight property.
- * Error pointer -EPROBE_DEFER if the DT node is found, but no
backlight device
- * is found.
- * If the backlight device is found, a pointer to the structure is
returned.
- */
-struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
-{
-    struct backlight_device *backlight;
-    struct device_node *np;
-
-    np = of_parse_phandle(dev->of_node, "backlight", 0);
-    if (!np)
-        return NULL;
-
-    backlight = of_find_backlight_by_node(np);
-    of_node_put(np);
-
-    if (!backlight)
-        return ERR_PTR(-EPROBE_DEFER);
-
-    if (!backlight->props.brightness) {
-        backlight->props.brightness = backlight->props.max_brightness;
-        DRM_DEBUG_KMS("Backlight brightness set to %d\n",
-                  backlight->props.brightness);
-    }
-
-    return backlight;
-}
-EXPORT_SYMBOL(tinydrm_of_find_backlight);
-
-/**
- * tinydrm_enable_backlight - Enable backlight helper
- * @backlight: Backlight device
- *
- * Returns:
- * Zero on success, negative error code on failure.
- */
-int tinydrm_enable_backlight(struct backlight_device *backlight)
-{
-    unsigned int old_state;
-    int ret;
-
-    if (!backlight)
-        return 0;
-
-    old_state = backlight->props.state;
-    backlight->props.state &= ~BL_CORE_FBBLANK;
-    DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,
-              backlight->props.state);
-
-    ret = backlight_update_status(backlight);
-    if (ret)
-        DRM_ERROR("Failed to enable backlight %d\n", ret);
-
-    return ret;
-}
-EXPORT_SYMBOL(tinydrm_enable_backlight);
-
-/**
- * tinydrm_disable_backlight - Disable backlight helper
- * @backlight: Backlight device
- *
- * Returns:
- * Zero on success, negative error code on failure.
- */
-int tinydrm_disable_backlight(struct backlight_device *backlight)
-{
-    unsigned int old_state;
-    int ret;
-
-    if (!backlight)
-        return 0;
-
-    old_state = backlight->props.state;
-    backlight->props.state |= BL_CORE_FBBLANK;
-    DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,
-              backlight->props.state);
-    ret = backlight_update_status(backlight);
-    if (ret)
-        DRM_ERROR("Failed to disable backlight %d\n", ret);
-
-    return ret;
-}
-EXPORT_SYMBOL(tinydrm_disable_backlight);
     #if IS_ENABLED(CONFIG_SPI)
   diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c
b/drivers/gpu/drm/tinydrm/mi0283qt.c
index 7e5bb7d..c161d45 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -12,6 +12,7 @@
   #include <drm/tinydrm/ili9341.h>
   #include <drm/tinydrm/mipi-dbi.h>
   #include <drm/tinydrm/tinydrm-helpers.h>
+#include <drm/tinydrm/tinydrm-backlight.h>
   #include <linux/delay.h>
   #include <linux/gpio/consumer.h>
   #include <linux/module.h>
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c
b/drivers/gpu/drm/tinydrm/mipi-dbi.c
index 2caeabc..dc84f26 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -11,6 +11,7 @@
     #include <drm/tinydrm/mipi-dbi.h>
   #include <drm/tinydrm/tinydrm-helpers.h>
+#include <drm/tinydrm/tinydrm-backlight.h>
   #include <linux/debugfs.h>
   #include <linux/dma-buf.h>
   #include <linux/gpio/consumer.h>
diff --git a/include/drm/tinydrm/tinydrm-backlight.h
b/include/drm/tinydrm/tinydrm-backlight.h
new file mode 100644
index 0000000..6a7b6d5
--- /dev/null
+++ b/include/drm/tinydrm/tinydrm-backlight.h
@@ -0,0 +1,18 @@
+/*
+ * Copyright (C) 2016 Noralf Trønnes
+ *
+ * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __LINUX_TINYDRM_BACKLIGHT_H
+#define __LINUX_TINYDRM_BACKLIGHT_H
+
+struct backlight_device;
+struct backlight_device *tinydrm_of_find_backlight(struct device *dev);
+int tinydrm_enable_backlight(struct backlight_device *backlight);
+int tinydrm_disable_backlight(struct backlight_device *backlight);
+
+#endif /* __LINUX_TINYDRM_BACKLIGHT_H */
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel


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


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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux