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);
I have a few questions here.
So if I understood the problem correctly, you do not want the backlight
brightness set to 1 by systemd-backlight because it sets the backlight
on before the display driver is set up. My question is, how does pwm_bl
avoid this ? Even if it starts off in a power off state, won't the same
thing happen i.e won't systemd set the brightness and subsequently turn
on the backlight ?
Also, I didn't understand your patch. What does gbl->def_value specify?
bl->props.state represents the power state right ? Is your patch setting
it to 0 prevent systemd-backlight from kicking in because we are dealing
with the power property and not the brightness property ?
I was trying to understand some of the functions in gpio_backlight.c and
was confused about one more thing. What is the difference between
gpio_backlight_probe and gpio_backlight_probe_dt ?
I am also a little confused about how gpio backlight and pwm backlight
are separate/different (because both would require the gpio pins).
Is there any documentation here which I could refer to?
Thanks and regards,
Meghana
> 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.
>
> Noralf.
>
> >>bd->props.power = FB_BLANK_UNBLANK;
> >> bd->props.state &= ~BL_CORE_FBBLANK;
> >>
> >>This is my backlight node[1]:
> >>
> >>backlight: backlight {
> >> compatible = "gpio-backlight";
> >> gpios = <&gpio 18 0>; // GPIO_ACTIVE_HIGH
> >>};
> >>
> >>Not certain that this is the "correct" solution, but I can't use the
> >>gpio-backlight property default-on, because that would turn on backlight
> >>before the pipeline is enabled.
> >>
> >>You can dry-run the driver without a panel connected, it can work
> >>without being able to read from the controller.
> >>
> >>Noralf.
> >>
> >>[1] https://github.com/notro/tinydrm/blob/master/rpi-overlays/rpi-display-overlay.dts
> >>
> >>_______________________________________________
> >>dri-devel mailing list
> >>dri-devel@xxxxxxxxxxxxxxxxxxxxx
> >>https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>
> >
> >_______________________________________________
> >dri-devel mailing list
> >dri-devel@xxxxxxxxxxxxxxxxxxxxx
> >https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
