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,
> >> +_backlight_ops, );
> >
> > 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,
>> + _backlight_ops, );
>
> 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,
> + _backlight_ops, );
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,
+
