[PATCH v2] drm/tinydrm: Move backlight helpers to a separate file
Daniel Vetter
daniel at ffwll.ch
Wed Sep 27 08:49:15 UTC 2017
On Tue, Sep 26, 2017 at 03:53:33PM +0200, Noralf Trønnes wrote:
>
> 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 at gmail.com>
> > > > > > > ---
> > > > > > 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.
Oh dear, sounds like we have an all around mess, and the real job is a lot
bigger than the todo.
Meghana, I think this isn't really a good idea for an application period
task, but it might be something we could make into an entire internship
even.
But since there's some good discussions and findings here already, can you
pls do a patch to todo.rst, with a link to this discussion from the web
archive and a short summary? Please make sure it renders correctly when
building the docs with
$ make htmldocs
Thanks, Daniel
>
> > 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 at lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > >
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list