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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> >>> --- >>> 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@xxxxxxxxx> >> >> *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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
