On Thu, Dec 21, 2017 at 8:08 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Thu, Dec 21, 2017 at 2:44 PM, Noralf Trønnes <noralf@xxxxxxxxxxx> 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 :-)
+1 to that.
>> 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.
Of the 2 properties, default-off actually makes more sense to me.
Ideally, we'd have no property for the *default*, common case and a
property only for the exception. I'd think on would be the common case
because what kind of device with a builtin display do you not want to
turn it on on boot. Really, controlling this at the backlight level
seems like the wrong level. The backlight should just follow the
display state.
I don't really have an issue adding this property, but adding a
property doesn't help you here. You have to not break a new kernel
working with an older dtb.
Rob
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
