[PATCH v14 0/3] Move backlight helper functions from tinydrm-helpers to linux/backlight

[Noralf Trønnes]

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.
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.

Noralf.

> Untangling the on/off vs brightness mess definitely sounds like a good
> idea, especially since some backlights have a minimal brightness (which
> doesn't match to off), because too low screws up the backlight and
> destroys the panel.
> -Daniel