[PATCH v2] drm/tinydrm: Move backlight helpers to a separate file
Noralf Trønnes
noralf at tronnes.org
Tue Sep 26 13:06:06 UTC 2017
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.
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
>>>>
More information about the dri-devel
mailing list