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