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 :-)
> 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.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
