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

[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?

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch