[PATCH v14 0/3] Move backlight helper functions from tinydrm-helpers to linux/backlight
Noralf Trønnes
noralf at tronnes.org
Thu Dec 21 17:06:49 UTC 2017
Den 21.12.2017 15.08, skrev Daniel Vetter:
> On Thu, Dec 21, 2017 at 2:44 PM, Noralf Trønnes <noralf at tronnes.org> wrote:
>> Den 21.12.2017 14.05, skrev Daniel Vetter:
>>> On Thu, Dec 21, 2017 at 11:52:43AM +0100, Noralf Trønnes wrote:
>>>> Den 11.12.2017 18.56, skrev Noralf Trønnes:
>>>>> Den 11.12.2017 18.45, skrev Noralf Trønnes:
>>>>>> Den 11.12.2017 15.58, skrev Meghana Madhyastha:
>>>>>>> On Mon, Dec 11, 2017 at 03:12:06PM +0100, Noralf Trønnes wrote:
>>>>>>>> Den 11.12.2017 14.17, skrev Meghana Madhyastha:
>>>>>>>>> On Sat, Dec 09, 2017 at 03:09:28PM +0100, Noralf Trønnes wrote:
>>>>>>>>>> Den 21.10.2017 13.55, skrev Meghana Madhyastha:
>>>>>>>>>>> Changes in v14:
>>>>>>>>>>> - s/backlight_get/of_find_backlight/ in patch 2/3
>>>>>>>>>>> - Change commit message in patch 3/3 from requiring to acquiring
>>>>>>>>>>>
>>>>>>>>>>> Meghana Madhyastha (3):
>>>>>>>>>>> drm/tinydrm: Move helper functions from
>>>>>>>>>>> tinydrm-helpers to backlight.h
>>>>>>>>>>> drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c
>>>>>>>>>>> drm/tinydrm: Add devres versions of of_find_backlight
>>>>>>>>>> I tried the patchset and this is what I got:
>>>>>>>>>>
>>>>>>>>>> [ 8.057792] Unable to handle kernel paging
>>>>>>>>>> request at virtual address
>>>>>>>>>> fffffe6b
>>>>> <snip>
>>>>>>>>>> [ 9.144181] ---[ end trace 149c05934b6a6dcc ]---
>>>>>>>>> Is the reason possibly because we have omitted error checking on the
>>>>>>>>> return value of backlight_enable function ?
>>>>>>>>> tinydrm_enable_backlight and
>>>>>>>>> tinydrm_disable_baclight do this.
>>>>>>>>> Eg.
>>>>>>>>> ret = backlight_update_status(backlight);
>>>>>>>>> if (ret)
>>>>>>>>> DRM_ERROR("Failed to enable backlight %d\n", ret);
>>>>>>>>>
>>>>>>>>> I'm not sure, just asking whether this could be a possible reason
>>>>>>>>> for the above trace.
>>>>>>>> The crash happens during probe.
>>>>>>>> I guess you'll figure this out when you get some hw to test on.
>>>>>>> I have set up the raspberry pi and have built and boot into the
>>>>>>> custom kernel
>>>>>>> but I am waiting for the panel to arrive. Meanwhile, any thoughts on
>>>>>>> error message ? Sorry for the trivial question, but I did not quite
>>>>>>> understand the crash message and how to replicate it.
>>>>>> of_find_backlight() can return an error pointer (-EPROBE_DEFER):
>>>>>>
>>>>>> diff --git a/drivers/video/backlight/backlight.c
>>>>>> b/drivers/video/backlight/backlight.c
>>>>>> index 4bb7bf3ee443..57370c5d51f0 100644
>>>>>> --- a/drivers/video/backlight/backlight.c
>>>>>> +++ b/drivers/video/backlight/backlight.c
>>>>>> @@ -635,8 +635,8 @@ struct backlight_device
>>>>>> *devm_of_find_backlight(struct device *dev)
>>>>>> int ret;
>>>>>>
>>>>>> bd = of_find_backlight(dev);
>>>>>> - if (!bd)
>>>>>> - return NULL;
>>>>>> + if (IS_ERR_OR_NULL(bd))
>>>>>> + return bd;
>>>>>>
>>>>>> ret = devm_add_action(dev, devm_backlight_put, bd);
>>>>>> if (ret) {
>>>>>>
>>>>>> That solved the crash, but the backlight didn't turn on.
>>>>>> I had to do this as well:
>>>>>>
>>>>>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>>>>>> index 5c441d4c049c..6f9925f10a7c 100644
>>>>>> --- a/include/linux/backlight.h
>>>>>> +++ b/include/linux/backlight.h
>>>>>> @@ -139,6 +139,8 @@ static inline int backlight_enable(struct
>>>>>> backlight_device *bd)
>>>>>> if (!bd)
>>>>>> return 0;
>>>>>>
>>>>>> + if (!bd->props.brightness)
>>>>>> + bd->props.brightness = bd->props.max_brightness;
>>>>> No, this is wrong, it should happen on probe, not every time it's
>>>>> enabled.
>>>>> So maybe it should be done in of_find_backlight() or something.
>>>>> I see that I'm currently doing it in tinydrm_of_find_backlight().
>>>>>
>>>> I'm not happy with this brightness hack of mine really.
>>>>
>>>> Since I last looked at this I see that pwm_bl has gained the ability to
>>>> start in a power off state (pwm_backlight_initial_power_state()).
>>>> However the gpio_backlight driver doesn't do this. gpio_backlight has a
>>>> 'default-on' property, but the problem is that it sets brightness, not
>>>> power state. So the absense of the property sets brightness to zero,
>>>> which makes the driver turn off backlight on probe. This seems to be
>>>> fine, but then systemd-backlight comes along and restores brightness
>>>> to 1, since that's what it was on shutdown. This turns on backlight and
>>>> now I have a glaring white uninitialized panel waiting for the display
>>>> driver to set it up.
>>>>
>>>> This patch would solve my problem:
>>>>
>>>> diff --git a/drivers/video/backlight/gpio_backlight.c
>>>> b/drivers/video/backlight/gpio_backlight.c
>>>> index e470da95d806..54bb722e1ea3 100644
>>>> --- a/drivers/video/backlight/gpio_backlight.c
>>>> +++ b/drivers/video/backlight/gpio_backlight.c
>>>> @@ -142,7 +142,8 @@ static int gpio_backlight_probe(struct
>>>> platform_device
>>>> *pdev)
>>>> return PTR_ERR(bl);
>>>> }
>>>>
>>>> - bl->props.brightness = gbl->def_value;
>>>> + bl->props.brightness = 1;
>>>> + bl->props.state = gbl->def_value ? 0 : BL_CORE_FBBLANK;
>>>> backlight_update_status(bl);
>>>>
>>>> platform_set_drvdata(pdev, bl);
>>>>
>>>> The problem is that this will most likely break 2 in-kernel users of
>>>> gpio-backlight which doesn't set the 'default-on' property:
>>>> arch/arm/boot/dts/omap4-var-dvk-om44.dts
>>>> arm/boot/dts/imx27-eukrea-mbimxsd27-baseboard.dts
>>>>
>>>> AFAICT they rely on systemd-backlight to turn on backlight by setting
>>>> brightness to 1.
>>>>
>>>> So maybe my hack is _the_ soulution after all, but I'm no expert on
>>>> the backlight subsystem and it's corner cases.
>>> Can we fix the dts instead?
>>
>> Isn't Device Tree ABI, ie. new kernels should work backwards with
>> existing dtb's? We will break that contract if we change gpio_backlight
>> like I proposed.
> It's only a regression if someone reports a bug :-)
>
>> Another solution is to add a new DT property: 'default-off':
>>
>> Optional properties:
>> - default-on: enable the backlight at boot.
>> - default-off: disable the backlight at boot by setting backlight in power
>> state off with brightness set to one.
>>
>> Not sure how well that will fly with Rob, it smells like a hack.
> tbh if we indeed go with "we can't touch this, it's baked in", which
> feels a bit silly, then default-off is the only reasonable thing to do
> really. At least for gpio-backlight, since atm the current
> implementation boils down to default-on == nothing. Or we just say the
> kernel doesn't follow the spec and fix it. I guess this is up to dt
> maintainers to decide.
I have tried the pwm backlight driver (pwm_bl) and it behaves as it
should and systemd-backlight does it's job of preserving brightness
across reboots without lighting up the display.
So it would be nice to have gpio_backlight adhere to the same rules.
Noralf.
More information about the dri-devel
mailing list