Re: [PATCH 1/4] drm/gma500: Refactor backlight support
Hi Hans, > >> +static int gma_backlight_update_status(struct backlight_device *bd) > >> +{ > >> + struct drm_device *dev = bl_get_data(bd); > >> + int level = bd->props.brightness; > > > > Here backlight_get_brightness(bd) should be used. > > Ack, but the old set methods all 3 used: > > int level = bd->props.brightness; > > So that would be a small functional / behavior change. > > As such I would prefer to split using backlight_get_brightness(bd) > out into a separate patch for version 2 of the series. > Like how I also made the change from type = BACKLIGHT_PLATFORM -> > type = BACKLIGHT_RAW a separate change. > > Would that be ok with you ? That would be perfect! > > > > > >> int gma_backlight_init(struct drm_device *dev) > >> { > >> -#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE > >>struct drm_psb_private *dev_priv = to_drm_psb_private(dev); > >> + struct backlight_properties props = {}; > >> + int ret; > >> + > >>dev_priv->backlight_enabled = true; > >> - return dev_priv->ops->backlight_init(dev); > >> -#else > >> - return 0; > >> + dev_priv->backlight_level = 100; > >> + > >> + ret = dev_priv->ops->backlight_init(dev); > >> + if (ret) > >> + return ret; > >> + > >> +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE > >> + props.brightness = dev_priv->backlight_level; > >> + props.max_brightness = PSB_MAX_BRIGHTNESS; > >> + props.type = BACKLIGHT_PLATFORM; > >> + > >> + dev_priv->backlight_device = > >> + backlight_device_register(dev_priv->ops->backlight_name, > >> +dev->dev, dev, > >> +&gma_backlight_ops, &props); > > > > Consider using the devm_backlight_device_register() variant. > > Then you can drop gma_backlight_exit() - I think.. > > The problem is the rest of the driver does not use devm_foo functions, > so then psb_driver_unload() which runs before the devm cleanup functions > will already release various iommap-s before the backlight is unregistered. > > This leaves a race where the backlight device could be poked and then try > to use no longer valid pointers in the main driver struct to write to the hw. Thanks for the explanation. When someone update the driver to devn_ then they surely will include backlight too. (I do not try to persuade you to do it, your time is better spent on the bigger backlight picture). Sam
Re: [PATCH 1/4] drm/gma500: Refactor backlight support
Hi Sam, On 9/11/22 13:48, Sam Ravnborg wrote: > Hi Hans, > > just a few minor things. See comments. > I like the diff - removes much more than it adds. I'm glad you like it and thank you for the review. > On Sat, Sep 10, 2022 at 10:50:58PM +0200, Hans de Goede wrote: >> Refactor backlight support so that the gma_backlight_enable() / >> gma_backlight_disable() / gma_backlight_set() functions used by >> the Opregion handle will also work if no backlight_device gets >> registered. >> >> This is a preparation patch for not registering the gma500's own backlight >> device when acpi_video should be used, since registering 2 backlight >> devices for a single display really is undesirable. >> >> Since the acpi-video interface often uses the OpRegion we need to keep >> the OpRegion functional even when dev_priv->backlight_device is NULL. >> >> As a result of this refactor the actual backlight_device_register() >> call is moved to the shared backlight.c code and all #ifdefs related to >> CONFIG_BACKLIGHT_CLASS_DEVICE are now also limited to backlight.c . >> >> No functional changes intended. >> >> This has been tested on a Packard Bell Dot SC (Intel Atom N2600, cedarview) >> and a Sony Vaio vpc-x11s1e (Intel N540, poulsbo) laptop. >> >> Signed-off-by: Hans de Goede >> --- > >> +static int gma_backlight_update_status(struct backlight_device *bd) >> +{ >> +struct drm_device *dev = bl_get_data(bd); >> +int level = bd->props.brightness; > > Here backlight_get_brightness(bd) should be used. Ack, but the old set methods all 3 used: int level = bd->props.brightness; So that would be a small functional / behavior change. As such I would prefer to split using backlight_get_brightness(bd) out into a separate patch for version 2 of the series. Like how I also made the change from type = BACKLIGHT_PLATFORM -> type = BACKLIGHT_RAW a separate change. Would that be ok with you ? > > >> int gma_backlight_init(struct drm_device *dev) >> { >> -#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE >> struct drm_psb_private *dev_priv = to_drm_psb_private(dev); >> +struct backlight_properties props = {}; >> +int ret; >> + >> dev_priv->backlight_enabled = true; >> -return dev_priv->ops->backlight_init(dev); >> -#else >> -return 0; >> +dev_priv->backlight_level = 100; >> + >> +ret = dev_priv->ops->backlight_init(dev); >> +if (ret) >> +return ret; >> + >> +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE >> +props.brightness = dev_priv->backlight_level; >> +props.max_brightness = PSB_MAX_BRIGHTNESS; >> +props.type = BACKLIGHT_PLATFORM; >> + >> +dev_priv->backlight_device = >> +backlight_device_register(dev_priv->ops->backlight_name, >> + dev->dev, dev, >> + &gma_backlight_ops, &props); > > Consider using the devm_backlight_device_register() variant. > Then you can drop gma_backlight_exit() - I think.. The problem is the rest of the driver does not use devm_foo functions, so then psb_driver_unload() which runs before the devm cleanup functions will already release various iommap-s before the backlight is unregistered. This leaves a race where the backlight device could be poked and then try to use no longer valid pointers in the main driver struct to write to the hw. Regards, Hans > >> +if (IS_ERR(dev_priv->backlight_device)) >> +return PTR_ERR(dev_priv->backlight_device); >> #endif >> + >> +return 0; >> } >> >> void gma_backlight_exit(struct drm_device *dev) >> { >> #ifdef CONFIG_BACKLIGHT_CLASS_DEVICE >> struct drm_psb_private *dev_priv = to_drm_psb_private(dev); >> -if (dev_priv->backlight_device) { >> -dev_priv->backlight_device->props.brightness = 0; >> -backlight_update_status(dev_priv->backlight_device); >> + >> +if (dev_priv->backlight_device) >> backlight_device_unregister(dev_priv->backlight_device); >> -} >> #endif >> } >
Re: [PATCH 1/4] drm/gma500: Refactor backlight support
Hi Hans, just a few minor things. See comments. I like the diff - removes much more than it adds. Sam On Sat, Sep 10, 2022 at 10:50:58PM +0200, Hans de Goede wrote: > Refactor backlight support so that the gma_backlight_enable() / > gma_backlight_disable() / gma_backlight_set() functions used by > the Opregion handle will also work if no backlight_device gets > registered. > > This is a preparation patch for not registering the gma500's own backlight > device when acpi_video should be used, since registering 2 backlight > devices for a single display really is undesirable. > > Since the acpi-video interface often uses the OpRegion we need to keep > the OpRegion functional even when dev_priv->backlight_device is NULL. > > As a result of this refactor the actual backlight_device_register() > call is moved to the shared backlight.c code and all #ifdefs related to > CONFIG_BACKLIGHT_CLASS_DEVICE are now also limited to backlight.c . > > No functional changes intended. > > This has been tested on a Packard Bell Dot SC (Intel Atom N2600, cedarview) > and a Sony Vaio vpc-x11s1e (Intel N540, poulsbo) laptop. > > Signed-off-by: Hans de Goede > --- > +static int gma_backlight_update_status(struct backlight_device *bd) > +{ > + struct drm_device *dev = bl_get_data(bd); > + int level = bd->props.brightness; Here backlight_get_brightness(bd) should be used. > int gma_backlight_init(struct drm_device *dev) > { > -#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE > struct drm_psb_private *dev_priv = to_drm_psb_private(dev); > + struct backlight_properties props = {}; > + int ret; > + > dev_priv->backlight_enabled = true; > - return dev_priv->ops->backlight_init(dev); > -#else > - return 0; > + dev_priv->backlight_level = 100; > + > + ret = dev_priv->ops->backlight_init(dev); > + if (ret) > + return ret; > + > +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE > + props.brightness = dev_priv->backlight_level; > + props.max_brightness = PSB_MAX_BRIGHTNESS; > + props.type = BACKLIGHT_PLATFORM; > + > + dev_priv->backlight_device = > + backlight_device_register(dev_priv->ops->backlight_name, > + dev->dev, dev, > + &gma_backlight_ops, &props); Consider using the devm_backlight_device_register() variant. Then you can drop gma_backlight_exit() - I think.. > + if (IS_ERR(dev_priv->backlight_device)) > + return PTR_ERR(dev_priv->backlight_device); > #endif > + > + return 0; > } > > void gma_backlight_exit(struct drm_device *dev) > { > #ifdef CONFIG_BACKLIGHT_CLASS_DEVICE > struct drm_psb_private *dev_priv = to_drm_psb_private(dev); > - if (dev_priv->backlight_device) { > - dev_priv->backlight_device->props.brightness = 0; > - backlight_update_status(dev_priv->backlight_device); > + > + if (dev_priv->backlight_device) > backlight_device_unregister(dev_priv->backlight_device); > - } > #endif > }
[PATCH 1/4] drm/gma500: Refactor backlight support
Refactor backlight support so that the gma_backlight_enable() / gma_backlight_disable() / gma_backlight_set() functions used by the Opregion handle will also work if no backlight_device gets registered. This is a preparation patch for not registering the gma500's own backlight device when acpi_video should be used, since registering 2 backlight devices for a single display really is undesirable. Since the acpi-video interface often uses the OpRegion we need to keep the OpRegion functional even when dev_priv->backlight_device is NULL. As a result of this refactor the actual backlight_device_register() call is moved to the shared backlight.c code and all #ifdefs related to CONFIG_BACKLIGHT_CLASS_DEVICE are now also limited to backlight.c . No functional changes intended. This has been tested on a Packard Bell Dot SC (Intel Atom N2600, cedarview) and a Sony Vaio vpc-x11s1e (Intel N540, poulsbo) laptop. Signed-off-by: Hans de Goede --- drivers/gpu/drm/gma500/backlight.c | 94 +++- drivers/gpu/drm/gma500/cdv_device.c | 50 +++-- drivers/gpu/drm/gma500/oaktrail_device.c | 65 ++-- drivers/gpu/drm/gma500/opregion.c| 6 +- drivers/gpu/drm/gma500/psb_device.c | 73 +- drivers/gpu/drm/gma500/psb_drv.c | 13 +--- drivers/gpu/drm/gma500/psb_drv.h | 13 ++-- 7 files changed, 87 insertions(+), 227 deletions(-) diff --git a/drivers/gpu/drm/gma500/backlight.c b/drivers/gpu/drm/gma500/backlight.c index 46b9c0f13d6d..20c793de7775 100644 --- a/drivers/gpu/drm/gma500/backlight.c +++ b/drivers/gpu/drm/gma500/backlight.c @@ -13,69 +13,95 @@ #include "intel_bios.h" #include "power.h" -#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE -static void do_gma_backlight_set(struct drm_device *dev) -{ - struct drm_psb_private *dev_priv = to_drm_psb_private(dev); - backlight_update_status(dev_priv->backlight_device); -} -#endif - void gma_backlight_enable(struct drm_device *dev) { -#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE struct drm_psb_private *dev_priv = to_drm_psb_private(dev); + dev_priv->backlight_enabled = true; - if (dev_priv->backlight_device) { - dev_priv->backlight_device->props.brightness = dev_priv->backlight_level; - do_gma_backlight_set(dev); - } -#endif + dev_priv->ops->backlight_set(dev, dev_priv->backlight_level); } void gma_backlight_disable(struct drm_device *dev) { -#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE struct drm_psb_private *dev_priv = to_drm_psb_private(dev); + dev_priv->backlight_enabled = false; - if (dev_priv->backlight_device) { - dev_priv->backlight_device->props.brightness = 0; - do_gma_backlight_set(dev); - } -#endif + dev_priv->ops->backlight_set(dev, 0); } void gma_backlight_set(struct drm_device *dev, int v) { -#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE struct drm_psb_private *dev_priv = to_drm_psb_private(dev); + dev_priv->backlight_level = v; - if (dev_priv->backlight_device && dev_priv->backlight_enabled) { - dev_priv->backlight_device->props.brightness = v; - do_gma_backlight_set(dev); - } -#endif + if (dev_priv->backlight_enabled) + dev_priv->ops->backlight_set(dev, v); } +static int gma_backlight_get_brightness(struct backlight_device *bd) +{ + struct drm_device *dev = bl_get_data(bd); + struct drm_psb_private *dev_priv = to_drm_psb_private(dev); + + if (dev_priv->ops->backlight_get) + return dev_priv->ops->backlight_get(dev); + + return dev_priv->backlight_level; +} + +static int gma_backlight_update_status(struct backlight_device *bd) +{ + struct drm_device *dev = bl_get_data(bd); + int level = bd->props.brightness; + + /* Percentage 1-100% being valid */ + if (level < 1) + level = 1; + + gma_backlight_set(dev, level); + return 0; +} + +static const struct backlight_ops gma_backlight_ops = { + .get_brightness = gma_backlight_get_brightness, + .update_status = gma_backlight_update_status, +}; + int gma_backlight_init(struct drm_device *dev) { -#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE struct drm_psb_private *dev_priv = to_drm_psb_private(dev); + struct backlight_properties props = {}; + int ret; + dev_priv->backlight_enabled = true; - return dev_priv->ops->backlight_init(dev); -#else - return 0; + dev_priv->backlight_level = 100; + + ret = dev_priv->ops->backlight_init(dev); + if (ret) + return ret; + +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE + props.brightness = dev_priv->backlight_level; + props.max_brightness = PSB_MAX_BRIGHTNESS; + props.type = BACKLIGHT_PLATFORM; + + dev_priv->backlight_device = + backlight_device_register(dev_priv->ops->backlight_name, +