[PATCH v2] backlight: turn backlight on/off when necessary

[PATCH v2] backlight: turn backlight on/off when necessary

[Liu Ying]

We don't have to turn backlight on/off everytime a blanking
or unblanking event comes because the backlight status may
have already been what we want. Another thought is that one
backlight device may be shared by multiple framebuffers. We
don't hope blanking one of the framebuffers may turn the
backlight off for all the other framebuffers, since they are
likely being active to display something. This patch adds
some logics to record each framebuffer's backlight usage to
determine the backlight device use count and whether the
backlight should be turned on or off. To be more specific,
only one unblank operation on a certain blanked framebuffer
may increase the backlight device's use count by one, while
one blank operation on a certain unblanked framebuffer may
decrease the use count by one, because the userspace is
likely to unblank a unblanked framebuffer or blank a blanked
framebuffer.

Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
---
v1 can be found at https://lkml.org/lkml/2013/5/30/139

v1->v2:
* Make the commit message be more specific about the condition
  in which backlight device use count can be increased/decreased.
* Correct the setting for bd->props.fb_blank.

 drivers/video/backlight/backlight.c |   28 +++++++++++++++++++++-------
 include/linux/backlight.h           |    6 ++++++
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 5d05555..42044be 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -34,13 +34,15 @@ static const char *const backlight_types[] = {
 			   defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE))
 /* This callback gets called when something important happens inside a
  * framebuffer driver. We're looking if that important event is blanking,
- * and if it is, we're switching backlight power as well ...
+ * and if it is and necessary, we're switching backlight power as well ...
  */
 static int fb_notifier_callback(struct notifier_block *self,
 				unsigned long event, void *data)
 {
 	struct backlight_device *bd;
 	struct fb_event *evdata = data;
+	int node = evdata->info->node;
+	int fb_blank = 0;
 
 	/* If we aren't interested in this event, skip it immediately ... */
 	if (event != FB_EVENT_BLANK && event != FB_EVENT_CONBLANK)
@@ -51,12 +53,24 @@ static int fb_notifier_callback(struct notifier_block *self,
 	if (bd->ops)
 		if (!bd->ops->check_fb ||
 		    bd->ops->check_fb(bd, evdata->info)) {
-			bd->props.fb_blank = *(int *)evdata->data;
-			if (bd->props.fb_blank == FB_BLANK_UNBLANK)
-				bd->props.state &= ~BL_CORE_FBBLANK;
-			else
-				bd->props.state |= BL_CORE_FBBLANK;
-			backlight_update_status(bd);
+			fb_blank = *(int *)evdata->data;
+			if (fb_blank == FB_BLANK_UNBLANK &&
+			    !bd->fb_bl_on[node]) {
+				bd->fb_bl_on[node] = true;
+				if (!bd->use_count++) {
+					bd->props.state &= ~BL_CORE_FBBLANK;
+					bd->props.fb_blank = FB_BLANK_UNBLANK;
+					backlight_update_status(bd);
+				}
+			} else if (fb_blank != FB_BLANK_UNBLANK &&
+				   bd->fb_bl_on[node]) {
+				bd->fb_bl_on[node] = false;
+				if (!(--bd->use_count)) {
+					bd->props.state |= BL_CORE_FBBLANK;
+					bd->props.fb_blank = FB_BLANK_POWERDOWN;
+					backlight_update_status(bd);
+				}
+			}
 		}
 	mutex_unlock(&bd->ops_lock);
 	return 0;
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 5f9cd96..7264742 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -9,6 +9,7 @@
 #define _LINUX_BACKLIGHT_H
 
 #include <linux/device.h>
+#include <linux/fb.h>
 #include <linux/mutex.h>
 #include <linux/notifier.h>
 
@@ -104,6 +105,11 @@ struct backlight_device {
 	struct list_head entry;
 
 	struct device dev;
+
+	/* Multiple framebuffers may share one backlight device */
+	bool fb_bl_on[FB_MAX];
+
+	int use_count;
 };
 
 static inline void backlight_update_status(struct backlight_device *bd)
-- 
1.7.9.5

Re:[PATCH v2] backlight: turn backlight on/off when necessary

[Liu Ying]

Ping...

Regards,
Liu Ying

On 01/20/2014 12:52 PM, Liu Ying wrote:
> We don't have to turn backlight on/off everytime a blanking
> or unblanking event comes because the backlight status may
> have already been what we want. Another thought is that one
> backlight device may be shared by multiple framebuffers. We
> don't hope blanking one of the framebuffers may turn the
> backlight off for all the other framebuffers, since they are
> likely being active to display something. This patch adds
> some logics to record each framebuffer's backlight usage to
> determine the backlight device use count and whether the
> backlight should be turned on or off. To be more specific,
> only one unblank operation on a certain blanked framebuffer
> may increase the backlight device's use count by one, while
> one blank operation on a certain unblanked framebuffer may
> decrease the use count by one, because the userspace is
> likely to unblank a unblanked framebuffer or blank a blanked
> framebuffer.
> 
> Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
> ---
> v1 can be found at https://lkml.org/lkml/2013/5/30/139
> 
> v1->v2:
> * Make the commit message be more specific about the condition
>   in which backlight device use count can be increased/decreased.
> * Correct the setting for bd->props.fb_blank.
> 
>  drivers/video/backlight/backlight.c |   28 +++++++++++++++++++++-------
>  include/linux/backlight.h           |    6 ++++++
>  2 files changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 5d05555..42044be 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -34,13 +34,15 @@ static const char *const backlight_types[] = {
>                            defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE))
>  /* This callback gets called when something important happens inside a
>   * framebuffer driver. We're looking if that important event is blanking,
> - * and if it is, we're switching backlight power as well ...
> + * and if it is and necessary, we're switching backlight power as well ...
>   */
>  static int fb_notifier_callback(struct notifier_block *self,
>                                 unsigned long event, void *data)
>  {
>         struct backlight_device *bd;
>         struct fb_event *evdata = data;
> +       int node = evdata->info->node;
> +       int fb_blank = 0;
> 
>         /* If we aren't interested in this event, skip it immediately ... */
>         if (event != FB_EVENT_BLANK && event != FB_EVENT_CONBLANK)
> @@ -51,12 +53,24 @@ static int fb_notifier_callback(struct notifier_block *self,
>         if (bd->ops)
>                 if (!bd->ops->check_fb ||
>                     bd->ops->check_fb(bd, evdata->info)) {
> -                       bd->props.fb_blank = *(int *)evdata->data;
> -                       if (bd->props.fb_blank == FB_BLANK_UNBLANK)
> -                               bd->props.state &= ~BL_CORE_FBBLANK;
> -                       else
> -                               bd->props.state |= BL_CORE_FBBLANK;
> -                       backlight_update_status(bd);
> +                       fb_blank = *(int *)evdata->data;
> +                       if (fb_blank == FB_BLANK_UNBLANK &&
> +                           !bd->fb_bl_on[node]) {
> +                               bd->fb_bl_on[node] = true;
> +                               if (!bd->use_count++) {
> +                                       bd->props.state &= ~BL_CORE_FBBLANK;
> +                                       bd->props.fb_blank = FB_BLANK_UNBLANK;
> +                                       backlight_update_status(bd);
> +                               }
> +                       } else if (fb_blank != FB_BLANK_UNBLANK &&
> +                                  bd->fb_bl_on[node]) {
> +                               bd->fb_bl_on[node] = false;
> +                               if (!(--bd->use_count)) {
> +                                       bd->props.state |= BL_CORE_FBBLANK;
> +                                       bd->props.fb_blank = FB_BLANK_POWERDOWN;
> +                                       backlight_update_status(bd);
> +                               }
> +                       }
>                 }
>         mutex_unlock(&bd->ops_lock);
>         return 0;
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 5f9cd96..7264742 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -9,6 +9,7 @@
>  #define _LINUX_BACKLIGHT_H
> 
>  #include <linux/device.h>
> +#include <linux/fb.h>
>  #include <linux/mutex.h>
>  #include <linux/notifier.h>
> 
> @@ -104,6 +105,11 @@ struct backlight_device {
>         struct list_head entry;
> 
>         struct device dev;
> +
> +       /* Multiple framebuffers may share one backlight device */
> +       bool fb_bl_on[FB_MAX];
> +
> +       int use_count;
>  };
> 
>  static inline void backlight_update_status(struct backlight_device *bd)
> --
> 1.7.9.5

Re: [PATCH v2] backlight: turn backlight on/off when necessary

[Jingoo Han]

On Wednesday, January 22, 2014 2:04 PM, Liu Ying wrote:
> 
> Ping...
> 
> Regards,
> Liu Ying

Please, don't send the ping within 2 days.
It is not a good practice. You sent the v1 patch 6 months ago.
However, why I should review the patch within 2 days?
Please wait.

Best regards,
Jingoo Han

> 
> On 01/20/2014 12:52 PM, Liu Ying wrote:
> > We don't have to turn backlight on/off everytime a blanking
> > or unblanking event comes because the backlight status may
> > have already been what we want. Another thought is that one
> > backlight device may be shared by multiple framebuffers. We
> > don't hope blanking one of the framebuffers may turn the
> > backlight off for all the other framebuffers, since they are
> > likely being active to display something. This patch adds
> > some logics to record each framebuffer's backlight usage to
> > determine the backlight device use count and whether the
> > backlight should be turned on or off. To be more specific,
> > only one unblank operation on a certain blanked framebuffer
> > may increase the backlight device's use count by one, while
> > one blank operation on a certain unblanked framebuffer may
> > decrease the use count by one, because the userspace is
> > likely to unblank a unblanked framebuffer or blank a blanked
> > framebuffer.
> >
> > Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
> > ---
> > v1 can be found at https://lkml.org/lkml/2013/5/30/139
> >
> > v1->v2:
> > * Make the commit message be more specific about the condition
> >   in which backlight device use count can be increased/decreased.
> > * Correct the setting for bd->props.fb_blank.
> >
> >  drivers/video/backlight/backlight.c |   28 +++++++++++++++++++++-------
> >  include/linux/backlight.h           |    6 ++++++
> >  2 files changed, 27 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> > index 5d05555..42044be 100644
> > --- a/drivers/video/backlight/backlight.c
> > +++ b/drivers/video/backlight/backlight.c
> > @@ -34,13 +34,15 @@ static const char *const backlight_types[] = {
> >                            defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE))
> >  /* This callback gets called when something important happens inside a
> >   * framebuffer driver. We're looking if that important event is blanking,
> > - * and if it is, we're switching backlight power as well ...
> > + * and if it is and necessary, we're switching backlight power as well ...
> >   */
> >  static int fb_notifier_callback(struct notifier_block *self,
> >                                 unsigned long event, void *data)
> >  {
> >         struct backlight_device *bd;
> >         struct fb_event *evdata = data;
> > +       int node = evdata->info->node;
> > +       int fb_blank = 0;
> >
> >         /* If we aren't interested in this event, skip it immediately ... */
> >         if (event != FB_EVENT_BLANK && event != FB_EVENT_CONBLANK)
> > @@ -51,12 +53,24 @@ static int fb_notifier_callback(struct notifier_block *self,
> >         if (bd->ops)
> >                 if (!bd->ops->check_fb ||
> >                     bd->ops->check_fb(bd, evdata->info)) {
> > -                       bd->props.fb_blank = *(int *)evdata->data;
> > -                       if (bd->props.fb_blank == FB_BLANK_UNBLANK)
> > -                               bd->props.state &= ~BL_CORE_FBBLANK;
> > -                       else
> > -                               bd->props.state |= BL_CORE_FBBLANK;
> > -                       backlight_update_status(bd);
> > +                       fb_blank = *(int *)evdata->data;
> > +                       if (fb_blank == FB_BLANK_UNBLANK &&
> > +                           !bd->fb_bl_on[node]) {
> > +                               bd->fb_bl_on[node] = true;
> > +                               if (!bd->use_count++) {
> > +                                       bd->props.state &= ~BL_CORE_FBBLANK;
> > +                                       bd->props.fb_blank = FB_BLANK_UNBLANK;
> > +                                       backlight_update_status(bd);
> > +                               }
> > +                       } else if (fb_blank != FB_BLANK_UNBLANK &&
> > +                                  bd->fb_bl_on[node]) {
> > +                               bd->fb_bl_on[node] = false;
> > +                               if (!(--bd->use_count)) {
> > +                                       bd->props.state |= BL_CORE_FBBLANK;
> > +                                       bd->props.fb_blank = FB_BLANK_POWERDOWN;
> > +                                       backlight_update_status(bd);
> > +                               }
> > +                       }
> >                 }
> >         mutex_unlock(&bd->ops_lock);
> >         return 0;
> > diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> > index 5f9cd96..7264742 100644
> > --- a/include/linux/backlight.h
> > +++ b/include/linux/backlight.h
> > @@ -9,6 +9,7 @@
> >  #define _LINUX_BACKLIGHT_H
> >
> >  #include <linux/device.h>
> > +#include <linux/fb.h>
> >  #include <linux/mutex.h>
> >  #include <linux/notifier.h>
> >
> > @@ -104,6 +105,11 @@ struct backlight_device {
> >         struct list_head entry;
> >
> >         struct device dev;
> > +
> > +       /* Multiple framebuffers may share one backlight device */
> > +       bool fb_bl_on[FB_MAX];
> > +
> > +       int use_count;
> >  };
> >
> >  static inline void backlight_update_status(struct backlight_device *bd)
> > --
> > 1.7.9.5

Re: [PATCH v2] backlight: turn backlight on/off when necessary

[Jani Nikula]

On Mon, 20 Jan 2014, Liu Ying <Ying.Liu@freescale.com> wrote:
> We don't have to turn backlight on/off everytime a blanking
> or unblanking event comes because the backlight status may
> have already been what we want. Another thought is that one
> backlight device may be shared by multiple framebuffers. We
> don't hope blanking one of the framebuffers may turn the
> backlight off for all the other framebuffers, since they are
> likely being active to display something. This patch adds
> some logics to record each framebuffer's backlight usage to
> determine the backlight device use count and whether the
> backlight should be turned on or off. To be more specific,
> only one unblank operation on a certain blanked framebuffer
> may increase the backlight device's use count by one, while
> one blank operation on a certain unblanked framebuffer may
> decrease the use count by one, because the userspace is
> likely to unblank a unblanked framebuffer or blank a blanked
> framebuffer.
>
> Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
> ---
> v1 can be found at https://lkml.org/lkml/2013/5/30/139
>
> v1->v2:
> * Make the commit message be more specific about the condition
>   in which backlight device use count can be increased/decreased.
> * Correct the setting for bd->props.fb_blank.
>
>  drivers/video/backlight/backlight.c |   28 +++++++++++++++++++++-------
>  include/linux/backlight.h           |    6 ++++++
>  2 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 5d05555..42044be 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -34,13 +34,15 @@ static const char *const backlight_types[] = {
>  			   defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE))
>  /* This callback gets called when something important happens inside a
>   * framebuffer driver. We're looking if that important event is blanking,
> - * and if it is, we're switching backlight power as well ...
> + * and if it is and necessary, we're switching backlight power as well ...
>   */
>  static int fb_notifier_callback(struct notifier_block *self,
>  				unsigned long event, void *data)
>  {
>  	struct backlight_device *bd;
>  	struct fb_event *evdata = data;
> +	int node = evdata->info->node;
> +	int fb_blank = 0;
>  
>  	/* If we aren't interested in this event, skip it immediately ... */
>  	if (event != FB_EVENT_BLANK && event != FB_EVENT_CONBLANK)
> @@ -51,12 +53,24 @@ static int fb_notifier_callback(struct notifier_block *self,
>  	if (bd->ops)
>  		if (!bd->ops->check_fb ||
>  		    bd->ops->check_fb(bd, evdata->info)) {
> -			bd->props.fb_blank = *(int *)evdata->data;
> -			if (bd->props.fb_blank == FB_BLANK_UNBLANK)
> -				bd->props.state &= ~BL_CORE_FBBLANK;
> -			else
> -				bd->props.state |= BL_CORE_FBBLANK;
> -			backlight_update_status(bd);
> +			fb_blank = *(int *)evdata->data;
> +			if (fb_blank == FB_BLANK_UNBLANK &&
> +			    !bd->fb_bl_on[node]) {
> +				bd->fb_bl_on[node] = true;
> +				if (!bd->use_count++) {
> +					bd->props.state &= ~BL_CORE_FBBLANK;
> +					bd->props.fb_blank = FB_BLANK_UNBLANK;
> +					backlight_update_status(bd);
> +				}
> +			} else if (fb_blank != FB_BLANK_UNBLANK &&
> +				   bd->fb_bl_on[node]) {
> +				bd->fb_bl_on[node] = false;
> +				if (!(--bd->use_count)) {
> +					bd->props.state |= BL_CORE_FBBLANK;
> +					bd->props.fb_blank = FB_BLANK_POWERDOWN;
> +					backlight_update_status(bd);
> +				}
> +			}

Anything backlight worries me a little, and there are actually three
changes bundled into one patch here:

1. Changing bd->props.state and bd->props.fb_blank only when use_count
   changes from 0->1 or 1->0.

2. Calling backlight_update_status() only with the above change, and not
   on all notifier callbacks.

3. Setting bd->props.fb_blank always to either FB_BLANK_UNBLANK or
   FB_BLANK_POWERDOWN instead of *(int *)evdata->data.

The rationale in the commit message seems plausible, and AFAICT the code
does what it says on the box, so for that (and for that alone) you can
have my

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

*BUT* it would be laborous to figure out whether this change in
behaviour might regress some drivers. I'm just punting on that. And that
brings us back to the three changes above - in a bisect POV it might be
helpful to split the patch up. Up to the maintainers.

HTH,
Jani.


>  		}
>  	mutex_unlock(&bd->ops_lock);
>  	return 0;
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 5f9cd96..7264742 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -9,6 +9,7 @@
>  #define _LINUX_BACKLIGHT_H
>  
>  #include <linux/device.h>
> +#include <linux/fb.h>
>  #include <linux/mutex.h>
>  #include <linux/notifier.h>
>  
> @@ -104,6 +105,11 @@ struct backlight_device {
>  	struct list_head entry;
>  
>  	struct device dev;
> +
> +	/* Multiple framebuffers may share one backlight device */
> +	bool fb_bl_on[FB_MAX];
> +
> +	int use_count;
>  };
>  
>  static inline void backlight_update_status(struct backlight_device *bd)
> -- 
> 1.7.9.5
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v2] backlight: turn backlight on/off when necessary

[Liu Ying]

On 01/22/2014 05:35 PM, Jani Nikula wrote:
> On Mon, 20 Jan 2014, Liu Ying <Ying.Liu@freescale.com> wrote:
>> We don't have to turn backlight on/off everytime a blanking
>> or unblanking event comes because the backlight status may
>> have already been what we want. Another thought is that one
>> backlight device may be shared by multiple framebuffers. We
>> don't hope blanking one of the framebuffers may turn the
>> backlight off for all the other framebuffers, since they are
>> likely being active to display something. This patch adds
>> some logics to record each framebuffer's backlight usage to
>> determine the backlight device use count and whether the
>> backlight should be turned on or off. To be more specific,
>> only one unblank operation on a certain blanked framebuffer
>> may increase the backlight device's use count by one, while
>> one blank operation on a certain unblanked framebuffer may
>> decrease the use count by one, because the userspace is
>> likely to unblank a unblanked framebuffer or blank a blanked
>> framebuffer.
>>
>> Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
>> ---
>> v1 can be found at https://lkml.org/lkml/2013/5/30/139
>>
>> v1->v2:
>> * Make the commit message be more specific about the condition
>>   in which backlight device use count can be increased/decreased.
>> * Correct the setting for bd->props.fb_blank.
>>
>>  drivers/video/backlight/backlight.c |   28 +++++++++++++++++++++-------
>>  include/linux/backlight.h           |    6 ++++++
>>  2 files changed, 27 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
>> index 5d05555..42044be 100644
>> --- a/drivers/video/backlight/backlight.c
>> +++ b/drivers/video/backlight/backlight.c
>> @@ -34,13 +34,15 @@ static const char *const backlight_types[] = {
>>  			   defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE))
>>  /* This callback gets called when something important happens inside a
>>   * framebuffer driver. We're looking if that important event is blanking,
>> - * and if it is, we're switching backlight power as well ...
>> + * and if it is and necessary, we're switching backlight power as well ...
>>   */
>>  static int fb_notifier_callback(struct notifier_block *self,
>>  				unsigned long event, void *data)
>>  {
>>  	struct backlight_device *bd;
>>  	struct fb_event *evdata = data;
>> +	int node = evdata->info->node;
>> +	int fb_blank = 0;
>>  
>>  	/* If we aren't interested in this event, skip it immediately ... */
>>  	if (event != FB_EVENT_BLANK && event != FB_EVENT_CONBLANK)
>> @@ -51,12 +53,24 @@ static int fb_notifier_callback(struct notifier_block *self,
>>  	if (bd->ops)
>>  		if (!bd->ops->check_fb ||
>>  		    bd->ops->check_fb(bd, evdata->info)) {
>> -			bd->props.fb_blank = *(int *)evdata->data;
>> -			if (bd->props.fb_blank == FB_BLANK_UNBLANK)
>> -				bd->props.state &= ~BL_CORE_FBBLANK;
>> -			else
>> -				bd->props.state |= BL_CORE_FBBLANK;
>> -			backlight_update_status(bd);
>> +			fb_blank = *(int *)evdata->data;
>> +			if (fb_blank == FB_BLANK_UNBLANK &&
>> +			    !bd->fb_bl_on[node]) {
>> +				bd->fb_bl_on[node] = true;
>> +				if (!bd->use_count++) {
>> +					bd->props.state &= ~BL_CORE_FBBLANK;
>> +					bd->props.fb_blank = FB_BLANK_UNBLANK;
>> +					backlight_update_status(bd);
>> +				}
>> +			} else if (fb_blank != FB_BLANK_UNBLANK &&
>> +				   bd->fb_bl_on[node]) {
>> +				bd->fb_bl_on[node] = false;
>> +				if (!(--bd->use_count)) {
>> +					bd->props.state |= BL_CORE_FBBLANK;
>> +					bd->props.fb_blank = FB_BLANK_POWERDOWN;

Looking at the patch again, I think we should set fb_blank to bd->props.fb_blank here to minimize the logic change.
I'll do more test for this and provide v3 if necessary.

>> +					backlight_update_status(bd);
>> +				}
>> +			}
> 
> Anything backlight worries me a little, and there are actually three
> changes bundled into one patch here:
> 
> 1. Changing bd->props.state and bd->props.fb_blank only when use_count
>    changes from 0->1 or 1->0.
> 
> 2. Calling backlight_update_status() only with the above change, and not
>    on all notifier callbacks.
> 
> 3. Setting bd->props.fb_blank always to either FB_BLANK_UNBLANK or
>    FB_BLANK_POWERDOWN instead of *(int *)evdata->data.
> 
> The rationale in the commit message seems plausible, and AFAICT the code
> does what it says on the box, so for that (and for that alone) you can
> have my
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Thanks for your review.

The backlight on my board is driving two separate display interfaces.
Instead of applying this patch to my kernel tree every time I upgrade it, I chose to send it to folks for review.

As the essential idea of this patch looks reasonable to me, I hope change could be done in other drivers in case this patch regresses them.

Liu Ying

> 
> *BUT* it would be laborous to figure out whether this change in
> behaviour might regress some drivers. I'm just punting on that. And that
> brings us back to the three changes above - in a bisect POV it might be
> helpful to split the patch up. Up to the maintainers.
> 
> HTH,
> Jani.
> 
> 
>>  		}
>>  	mutex_unlock(&bd->ops_lock);
>>  	return 0;
>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>> index 5f9cd96..7264742 100644
>> --- a/include/linux/backlight.h
>> +++ b/include/linux/backlight.h
>> @@ -9,6 +9,7 @@
>>  #define _LINUX_BACKLIGHT_H
>>  
>>  #include <linux/device.h>
>> +#include <linux/fb.h>
>>  #include <linux/mutex.h>
>>  #include <linux/notifier.h>
>>  
>> @@ -104,6 +105,11 @@ struct backlight_device {
>>  	struct list_head entry;
>>  
>>  	struct device dev;
>> +
>> +	/* Multiple framebuffers may share one backlight device */
>> +	bool fb_bl_on[FB_MAX];
>> +
>> +	int use_count;
>>  };
>>  
>>  static inline void backlight_update_status(struct backlight_device *bd)
>> -- 
>> 1.7.9.5
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH v2] backlight: turn backlight on/off when necessary

[Jingoo Han]

On Wednesday, January 22, 2014 6:36 PM, Jani Nikula wrote:
> On Mon, 20 Jan 2014, Liu Ying <Ying.Liu@freescale.com> wrote:
> > We don't have to turn backlight on/off everytime a blanking
> > or unblanking event comes because the backlight status may
> > have already been what we want. Another thought is that one
> > backlight device may be shared by multiple framebuffers. We
> > don't hope blanking one of the framebuffers may turn the
> > backlight off for all the other framebuffers, since they are
> > likely being active to display something. This patch adds
> > some logics to record each framebuffer's backlight usage to
> > determine the backlight device use count and whether the
> > backlight should be turned on or off. To be more specific,
> > only one unblank operation on a certain blanked framebuffer
> > may increase the backlight device's use count by one, while
> > one blank operation on a certain unblanked framebuffer may
> > decrease the use count by one, because the userspace is
> > likely to unblank a unblanked framebuffer or blank a blanked
> > framebuffer.
> >
> > Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
> > ---
> > v1 can be found at https://lkml.org/lkml/2013/5/30/139
> >
> > v1->v2:
> > * Make the commit message be more specific about the condition
> >   in which backlight device use count can be increased/decreased.
> > * Correct the setting for bd->props.fb_blank.
> >
> >  drivers/video/backlight/backlight.c |   28 +++++++++++++++++++++-------
> >  include/linux/backlight.h           |    6 ++++++
> >  2 files changed, 27 insertions(+), 7 deletions(-)
> >

[.....]
> 
> Anything backlight worries me a little, and there are actually three
> changes bundled into one patch here:
> 
> 1. Changing bd->props.state and bd->props.fb_blank only when use_count
>    changes from 0->1 or 1->0.
> 
> 2. Calling backlight_update_status() only with the above change, and not
>    on all notifier callbacks.
> 
> 3. Setting bd->props.fb_blank always to either FB_BLANK_UNBLANK or
>    FB_BLANK_POWERDOWN instead of *(int *)evdata->data.
> 
> The rationale in the commit message seems plausible, and AFAICT the code
> does what it says on the box, so for that (and for that alone) you can
> have my
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> *BUT* it would be laborous to figure out whether this change in
> behaviour might regress some drivers. I'm just punting on that. And that
> brings us back to the three changes above - in a bisect POV it might be
> helpful to split the patch up. Up to the maintainers.

I agree with Jani Nikula's opinion.
Please split this patch into three patches as above mentioned.

Best regards,
Jingoo Han

Re: [PATCH v2] backlight: turn backlight on/off when necessary

[Liu Ying]

On 01/23/2014 01:44 PM, Jingoo Han wrote:
> On Wednesday, January 22, 2014 6:36 PM, Jani Nikula wrote:
>> On Mon, 20 Jan 2014, Liu Ying <Ying.Liu@freescale.com> wrote:
>>> We don't have to turn backlight on/off everytime a blanking
>>> or unblanking event comes because the backlight status may
>>> have already been what we want. Another thought is that one
>>> backlight device may be shared by multiple framebuffers. We
>>> don't hope blanking one of the framebuffers may turn the
>>> backlight off for all the other framebuffers, since they are
>>> likely being active to display something. This patch adds
>>> some logics to record each framebuffer's backlight usage to
>>> determine the backlight device use count and whether the
>>> backlight should be turned on or off. To be more specific,
>>> only one unblank operation on a certain blanked framebuffer
>>> may increase the backlight device's use count by one, while
>>> one blank operation on a certain unblanked framebuffer may
>>> decrease the use count by one, because the userspace is
>>> likely to unblank a unblanked framebuffer or blank a blanked
>>> framebuffer.
>>>
>>> Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
>>> ---
>>> v1 can be found at https://lkml.org/lkml/2013/5/30/139
>>>
>>> v1->v2:
>>> * Make the commit message be more specific about the condition
>>>   in which backlight device use count can be increased/decreased.
>>> * Correct the setting for bd->props.fb_blank.
>>>
>>>  drivers/video/backlight/backlight.c |   28 +++++++++++++++++++++-------
>>>  include/linux/backlight.h           |    6 ++++++
>>>  2 files changed, 27 insertions(+), 7 deletions(-)
>>>
> 
> [.....]
>>
>> Anything backlight worries me a little, and there are actually three
>> changes bundled into one patch here:
>>
>> 1. Changing bd->props.state and bd->props.fb_blank only when use_count
>>    changes from 0->1 or 1->0.
>>
>> 2. Calling backlight_update_status() only with the above change, and not
>>    on all notifier callbacks.
>>
>> 3. Setting bd->props.fb_blank always to either FB_BLANK_UNBLANK or
>>    FB_BLANK_POWERDOWN instead of *(int *)evdata->data.

Since I have already post v3(https://lkml.org/lkml/2014/1/22/126) to change the setting for bd->props.fb_blank, the idea of the 3rd point is not very appropriate any more.

>>
>> The rationale in the commit message seems plausible, and AFAICT the code
>> does what it says on the box, so for that (and for that alone) you can
>> have my
>>
>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>>
>> *BUT* it would be laborous to figure out whether this change in
>> behaviour might regress some drivers. I'm just punting on that. And that
>> brings us back to the three changes above - in a bisect POV it might be
>> helpful to split the patch up. Up to the maintainers.
> 
> I agree with Jani Nikula's opinion.
> Please split this patch into three patches as above mentioned.
> 

I am open to split the patch up.
However, IMHO, this patch is somewhat self-contained.
For example, if we try to create 2 patches for the 1st point and the 2nd point Jani mentioned, one patch would invent the use_count and call backlight_update_status() on all notifier callbacks(just
ignore the use_count).
Do you think this is a good patch?

It also doesn't look straightforward for me to create 2 patches for the 1st point and the 2nd point.

Please advice.

Regards,
Liu Ying

Re: [PATCH v2] backlight: turn backlight on/off when necessary

[Liu Ying]

On 01/23/2014 05:27 PM, Liu Ying wrote:
> On 01/23/2014 01:44 PM, Jingoo Han wrote:
>> On Wednesday, January 22, 2014 6:36 PM, Jani Nikula wrote:
>>> On Mon, 20 Jan 2014, Liu Ying <Ying.Liu@freescale.com> wrote:
>>>> We don't have to turn backlight on/off everytime a blanking
>>>> or unblanking event comes because the backlight status may
>>>> have already been what we want. Another thought is that one
>>>> backlight device may be shared by multiple framebuffers. We
>>>> don't hope blanking one of the framebuffers may turn the
>>>> backlight off for all the other framebuffers, since they are
>>>> likely being active to display something. This patch adds
>>>> some logics to record each framebuffer's backlight usage to
>>>> determine the backlight device use count and whether the
>>>> backlight should be turned on or off. To be more specific,
>>>> only one unblank operation on a certain blanked framebuffer
>>>> may increase the backlight device's use count by one, while
>>>> one blank operation on a certain unblanked framebuffer may
>>>> decrease the use count by one, because the userspace is
>>>> likely to unblank a unblanked framebuffer or blank a blanked
>>>> framebuffer.
>>>>
>>>> Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
>>>> ---
>>>> v1 can be found at https://lkml.org/lkml/2013/5/30/139
>>>>
>>>> v1->v2:
>>>> * Make the commit message be more specific about the condition
>>>>   in which backlight device use count can be increased/decreased.
>>>> * Correct the setting for bd->props.fb_blank.
>>>>
>>>>  drivers/video/backlight/backlight.c |   28 +++++++++++++++++++++-------
>>>>  include/linux/backlight.h           |    6 ++++++
>>>>  2 files changed, 27 insertions(+), 7 deletions(-)
>>>>
>>
>> [.....]
>>>
>>> Anything backlight worries me a little, and there are actually three
>>> changes bundled into one patch here:
>>>
>>> 1. Changing bd->props.state and bd->props.fb_blank only when use_count
>>>    changes from 0->1 or 1->0.
>>>
>>> 2. Calling backlight_update_status() only with the above change, and not
>>>    on all notifier callbacks.
>>>
>>> 3. Setting bd->props.fb_blank always to either FB_BLANK_UNBLANK or
>>>    FB_BLANK_POWERDOWN instead of *(int *)evdata->data.
> 
> Since I have already post v3(https://lkml.org/lkml/2014/1/22/126) to change the setting for bd->props.fb_blank, the idea of the 3rd point is not very appropriate any more.
> 
>>>
>>> The rationale in the commit message seems plausible, and AFAICT the code
>>> does what it says on the box, so for that (and for that alone) you can
>>> have my
>>>
>>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>>>
>>> *BUT* it would be laborous to figure out whether this change in
>>> behaviour might regress some drivers. I'm just punting on that. And that
>>> brings us back to the three changes above - in a bisect POV it might be
>>> helpful to split the patch up. Up to the maintainers.
>>
>> I agree with Jani Nikula's opinion.
>> Please split this patch into three patches as above mentioned.
>>
> 
> I am open to split the patch up.
> However, IMHO, this patch is somewhat self-contained.
> For example, if we try to create 2 patches for the 1st point and the 2nd point Jani mentioned, one patch would invent the use_count and call backlight_update_status() on all notifier callbacks(just
> ignore the use_count).
> Do you think this is a good patch?
> 
> It also doesn't look straightforward for me to create 2 patches for the 1st point and the 3rd point.
                                                                                            ^ Sorry, fix typo(2nd -> 3rd).

> 
> Please advice.
> 

Re: [PATCH v2] backlight: turn backlight on/off when necessary

[Jingoo Han]

On Thursday, January 23, 2014 6:28 PM, Liu Ying wrote:
> On 01/23/2014 01:44 PM, Jingoo Han wrote:
> > On Wednesday, January 22, 2014 6:36 PM, Jani Nikula wrote:
> >> On Mon, 20 Jan 2014, Liu Ying <Ying.Liu@freescale.com> wrote:
> >>> We don't have to turn backlight on/off everytime a blanking
> >>> or unblanking event comes because the backlight status may
> >>> have already been what we want. Another thought is that one
> >>> backlight device may be shared by multiple framebuffers. We
> >>> don't hope blanking one of the framebuffers may turn the
> >>> backlight off for all the other framebuffers, since they are
> >>> likely being active to display something. This patch adds
> >>> some logics to record each framebuffer's backlight usage to
> >>> determine the backlight device use count and whether the
> >>> backlight should be turned on or off. To be more specific,
> >>> only one unblank operation on a certain blanked framebuffer
> >>> may increase the backlight device's use count by one, while
> >>> one blank operation on a certain unblanked framebuffer may
> >>> decrease the use count by one, because the userspace is
> >>> likely to unblank a unblanked framebuffer or blank a blanked
> >>> framebuffer.
> >>>
> >>> Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
> >>> ---
> >>> v1 can be found at https://lkml.org/lkml/2013/5/30/139
> >>>
> >>> v1->v2:
> >>> * Make the commit message be more specific about the condition
> >>>   in which backlight device use count can be increased/decreased.
> >>> * Correct the setting for bd->props.fb_blank.
> >>>
> >>>  drivers/video/backlight/backlight.c |   28 +++++++++++++++++++++-------
> >>>  include/linux/backlight.h           |    6 ++++++
> >>>  2 files changed, 27 insertions(+), 7 deletions(-)
> >>>
> >
> > [.....]
> >>
> >> Anything backlight worries me a little, and there are actually three
> >> changes bundled into one patch here:
> >>
> >> 1. Changing bd->props.state and bd->props.fb_blank only when use_count
> >>    changes from 0->1 or 1->0.
> >>
> >> 2. Calling backlight_update_status() only with the above change, and not
> >>    on all notifier callbacks.
> >>
> >> 3. Setting bd->props.fb_blank always to either FB_BLANK_UNBLANK or
> >>    FB_BLANK_POWERDOWN instead of *(int *)evdata->data.
> 
> Since I have already post v3(https://lkml.org/lkml/2014/1/22/126)
> to change the setting for bd->props.fb_blank, the idea of the 3rd point
> is not very appropriate any more.

OK, I see.

> 
> >>
> >> The rationale in the commit message seems plausible, and AFAICT the code
> >> does what it says on the box, so for that (and for that alone) you can
> >> have my
> >>
> >> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> >>
> >> *BUT* it would be laborous to figure out whether this change in
> >> behaviour might regress some drivers. I'm just punting on that. And that
> >> brings us back to the three changes above - in a bisect POV it might be
> >> helpful to split the patch up. Up to the maintainers.
> >
> > I agree with Jani Nikula's opinion.
> > Please split this patch into three patches as above mentioned.
> >
> 
> I am open to split the patch up.
> However, IMHO, this patch is somewhat self-contained.
> For example, if we try to create 2 patches for the 1st point and
>  the 2nd point Jani mentioned, one patch would invent the use_count
>  and call backlight_update_status() on all notifier callbacks(just
> ignore the use_count).
> Do you think this is a good patch?

The calling backlight_update_status() regardless the use_count
Will make the critical side effect? I don't think so.
Also, these two patches will be merged at the same time.
Please, split the patch into two patches. It would be clearer.

One more thing, please keep the indent using "Enter", when
sending your reply mail.

Best regards,
Jingoo Han