| Message ID | 1477169904-14997-1-git-send-email-l.majewski@majess.pl |
|---|---|
| State | Superseded |
| Headers | show |
On Saturday, October 22, 2016, Lukasz Majewski wrote: > > The commit: a55944ca82d287ca099ca90413af857af9086773 has posed some extra Please add the subject of the patch, in order to let people know which patch you mention exactly. Please loot at other commits that fixed bugs or behavior of other patches. > restrictions on blanking and unblanking frame buffer device. > > Unfortunately, pwm_bl driver's probe did not initialize members of struct > backlight_device necessary for further blank/unblank operation. > > This code in case of initial unblank of backlight device (default > behaviour) sets use_count to 1 and marks this particular backlight device > as used by all available fb devices (since it is not known during probe > how much and which fb devices will be assigned). > > Without this code, the backlight works properly until one tries to blank > it manually from sysfs with "echo 1 > /sys/class/graphics/fb0/blank". > Since fb_bl_on[0] and use_count were both set to 0, the logic at > fb_notifier_callback (@backlight.c) thought that we didn't turn on > (unblanked) the backlight device and refuses to disable (blank) it. > As a result we see garbage from fb displayed. > > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl> > --- > The patch has been tested on i.MX6q with vanilla 4.7 and 4.8 kernels. > It applies on 4.9-rcX SHA1: dcd4693cf47801b7d988ea897519de90dfd25d17. > > --- > drivers/video/backlight/pwm_bl.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/video/backlight/pwm_bl.c > b/drivers/video/backlight/pwm_bl.c > index 8040fd6..def39e8 100644 > --- a/drivers/video/backlight/pwm_bl.c > +++ b/drivers/video/backlight/pwm_bl.c > @@ -203,7 +203,7 @@ static int pwm_backlight_probe(struct platform_device > *pdev) > struct pwm_bl_data *pb; > int initial_blank = FB_BLANK_UNBLANK; > struct pwm_args pargs; > - int ret; > + int ret, i; > > if (!data) { > ret = pwm_backlight_parse_dt(&pdev->dev, &defdata); @@ - > 349,6 +349,14 @@ static int pwm_backlight_probe(struct platform_device > *pdev) > > bl->props.brightness = data->dft_brightness; > bl->props.power = initial_blank; > + > + if (initial_blank == FB_BLANK_UNBLANK) { > + for (i = 0; i < FB_MAX; i++) > + bl->fb_bl_on[i] = true; > + > + bl->use_count = 1; > + } > + > backlight_update_status(bl); > > platform_set_drvdata(pdev, bl); > -- > 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jingoo, > On Saturday, October 22, 2016, Lukasz Majewski wrote: > > > > The commit: a55944ca82d287ca099ca90413af857af9086773 has posed some > > extra > > Please add the subject of the patch, in order to let people know > which patch you mention exactly. Please loot at other commits that > fixed bugs or behavior > of other patches. Thanks for the tip. The mentioned patch: commit a55944ca82d287ca099ca90413af857af9086773 Author: Liu Ying <Ying.Liu@freescale.com> Date: Thu Apr 3 14:48:54 2014 -0700 backlight: update bd state & fb_blank properties when necessary We don't have to update the state and fb_blank properties of a backlight device every time a blanking or unblanking event comes because they may have already been what we want. Another thought is that one backlight device may be shared by multiple framebuffers. The backlight driver should take the backlight device as a resource shared by all the associated framebuffers. This patch adds some logic to record each framebuffer's backlight usage to determine the backlight device use count and whether the two properties should be updated or not. 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 an unblanked framebuffer or blank a blanked framebuffer. Could you provide more feedback regarding provided fix? Thanks in advance, Łukasz Majewski > > > restrictions on blanking and unblanking frame buffer device. > > > > Unfortunately, pwm_bl driver's probe did not initialize members of > > struct backlight_device necessary for further blank/unblank > > operation. > > > > This code in case of initial unblank of backlight device (default > > behaviour) sets use_count to 1 and marks this particular backlight > > device as used by all available fb devices (since it is not known > > during probe how much and which fb devices will be assigned). > > > > Without this code, the backlight works properly until one tries to > > blank it manually from sysfs with "echo 1 > > > /sys/class/graphics/fb0/blank". Since fb_bl_on[0] and use_count > > > were both set to 0, the logic at > > fb_notifier_callback (@backlight.c) thought that we didn't turn on > > (unblanked) the backlight device and refuses to disable (blank) it. > > As a result we see garbage from fb displayed. > > > > > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl> > > --- > > The patch has been tested on i.MX6q with vanilla 4.7 and 4.8 > > kernels. It applies on 4.9-rcX SHA1: > > dcd4693cf47801b7d988ea897519de90dfd25d17. > > > > --- > > drivers/video/backlight/pwm_bl.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/video/backlight/pwm_bl.c > > b/drivers/video/backlight/pwm_bl.c > > index 8040fd6..def39e8 100644 > > --- a/drivers/video/backlight/pwm_bl.c > > +++ b/drivers/video/backlight/pwm_bl.c > > @@ -203,7 +203,7 @@ static int pwm_backlight_probe(struct > > platform_device *pdev) > > struct pwm_bl_data *pb; > > int initial_blank = FB_BLANK_UNBLANK; > > struct pwm_args pargs; > > - int ret; > > + int ret, i; > > > > if (!data) { > > ret = pwm_backlight_parse_dt(&pdev->dev, > > &defdata); @@ - 349,6 +349,14 @@ static int > > pwm_backlight_probe(struct platform_device *pdev) > > > > bl->props.brightness = data->dft_brightness; > > bl->props.power = initial_blank; > > + > > + if (initial_blank == FB_BLANK_UNBLANK) { > > + for (i = 0; i < FB_MAX; i++) > > + bl->fb_bl_on[i] = true; > > + > > + bl->use_count = 1; > > + } > > + > > backlight_update_status(bl); > > > > platform_set_drvdata(pdev, bl); > > -- > > 2.1.4 > >
On Monday, October 24, 2016, Lukasz Majewski wrote: > > Hi Jingoo, > > > On Saturday, October 22, 2016, Lukasz Majewski wrote: > > > > > > The commit: a55944ca82d287ca099ca90413af857af9086773 has posed some > > > extra Please add the 'subject' of patch as below. The commit a55944ca82d2 ("backlight: update bd state & fb_blank properties when necessary ") has posted some extra. Best regards, Jingoo Han > > > > Please add the subject of the patch, in order to let people know > > which patch you mention exactly. Please loot at other commits that > > fixed bugs or behavior > > of other patches. > > Thanks for the tip. > > The mentioned patch: > > commit a55944ca82d287ca099ca90413af857af9086773 > Author: Liu Ying <Ying.Liu@freescale.com> > Date: Thu Apr 3 14:48:54 2014 -0700 > > backlight: update bd state & fb_blank properties when necessary > > We don't have to update the state and fb_blank properties of a > backlight > device every time a blanking or unblanking event comes because they > may > have already been what we want. Another thought is that one backlight > device may be shared by multiple framebuffers. The backlight driver > should take the backlight device as a resource shared by all the > associated framebuffers. > > This patch adds some logic to record each framebuffer's backlight > usage > to determine the backlight device use count and whether the two > properties should be updated or not. 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 an unblanked framebuffer or blank a > blanked framebuffer. > > > Could you provide more feedback regarding provided fix? > > Thanks in advance, > > Łukasz Majewski > > > > > > restrictions on blanking and unblanking frame buffer device. > > > > > > Unfortunately, pwm_bl driver's probe did not initialize members of > > > struct backlight_device necessary for further blank/unblank > > > operation. > > > > > > This code in case of initial unblank of backlight device (default > > > behaviour) sets use_count to 1 and marks this particular backlight > > > device as used by all available fb devices (since it is not known > > > during probe how much and which fb devices will be assigned). > > > > > > Without this code, the backlight works properly until one tries to > > > blank it manually from sysfs with "echo 1 > > > > /sys/class/graphics/fb0/blank". Since fb_bl_on[0] and use_count > > > > were both set to 0, the logic at > > > fb_notifier_callback (@backlight.c) thought that we didn't turn on > > > (unblanked) the backlight device and refuses to disable (blank) it. > > > As a result we see garbage from fb displayed. > > > > > > > > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl> > > > --- > > > The patch has been tested on i.MX6q with vanilla 4.7 and 4.8 > > > kernels. It applies on 4.9-rcX SHA1: > > > dcd4693cf47801b7d988ea897519de90dfd25d17. > > > > > > --- > > > drivers/video/backlight/pwm_bl.c | 10 +++++++++- > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/video/backlight/pwm_bl.c > > > b/drivers/video/backlight/pwm_bl.c > > > index 8040fd6..def39e8 100644 > > > --- a/drivers/video/backlight/pwm_bl.c > > > +++ b/drivers/video/backlight/pwm_bl.c > > > @@ -203,7 +203,7 @@ static int pwm_backlight_probe(struct > > > platform_device *pdev) > > > struct pwm_bl_data *pb; > > > int initial_blank = FB_BLANK_UNBLANK; > > > struct pwm_args pargs; > > > - int ret; > > > + int ret, i; > > > > > > if (!data) { > > > ret = pwm_backlight_parse_dt(&pdev->dev, > > > &defdata); @@ - 349,6 +349,14 @@ static int > > > pwm_backlight_probe(struct platform_device *pdev) > > > > > > bl->props.brightness = data->dft_brightness; > > > bl->props.power = initial_blank; > > > + > > > + if (initial_blank == FB_BLANK_UNBLANK) { > > > + for (i = 0; i < FB_MAX; i++) > > > + bl->fb_bl_on[i] = true; > > > + > > > + bl->use_count = 1; > > > + } > > > + > > > backlight_update_status(bl); > > > > > > platform_set_drvdata(pdev, bl); > > > -- > > > 2.1.4 > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jingoo, > On Monday, October 24, 2016, Lukasz Majewski wrote: > > > > Hi Jingoo, > > > > > On Saturday, October 22, 2016, Lukasz Majewski wrote: > > > > > > > > The commit: a55944ca82d287ca099ca90413af857af9086773 has posed > > > > some extra > > Please add the 'subject' of patch as below. > > The commit a55944ca82d2 ("backlight: update bd state & fb_blank > properties when necessary ") has posted some extra. Ok, I will add such information. However, I'm more interested in some technical comments regarding proposed fix. Could you provide some feedback on the technical side of the patch? Best regards, Łukasz Majewski > > Best regards, > Jingoo Han > > > > > > > > Please add the subject of the patch, in order to let people know > > > which patch you mention exactly. Please loot at other commits that > > > fixed bugs or behavior > > > of other patches. > > > > Thanks for the tip. > > > > The mentioned patch: > > > > commit a55944ca82d287ca099ca90413af857af9086773 > > Author: Liu Ying <Ying.Liu@freescale.com> > > Date: Thu Apr 3 14:48:54 2014 -0700 > > > > backlight: update bd state & fb_blank properties when necessary > > > > We don't have to update the state and fb_blank properties of a > > backlight > > device every time a blanking or unblanking event comes because > > they may > > have already been what we want. Another thought is that one > > backlight device may be shared by multiple framebuffers. The > > backlight driver should take the backlight device as a resource > > shared by all the associated framebuffers. > > > > This patch adds some logic to record each framebuffer's > > backlight usage > > to determine the backlight device use count and whether the two > > properties should be updated or not. 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 an unblanked framebuffer or > > blank a blanked framebuffer. > > > > > > Could you provide more feedback regarding provided fix? > > > > Thanks in advance, > > > > Łukasz Majewski > > > > > > > > > restrictions on blanking and unblanking frame buffer device. > > > > > > > > Unfortunately, pwm_bl driver's probe did not initialize members > > > > of struct backlight_device necessary for further blank/unblank > > > > operation. > > > > > > > > This code in case of initial unblank of backlight device > > > > (default behaviour) sets use_count to 1 and marks this > > > > particular backlight device as used by all available fb devices > > > > (since it is not known during probe how much and which fb > > > > devices will be assigned). > > > > > > > > Without this code, the backlight works properly until one tries > > > > to blank it manually from sysfs with "echo 1 > > > > > /sys/class/graphics/fb0/blank". Since fb_bl_on[0] and > > > > > use_count were both set to 0, the logic at > > > > fb_notifier_callback (@backlight.c) thought that we didn't turn > > > > on (unblanked) the backlight device and refuses to disable > > > > (blank) it. As a result we see garbage from fb displayed. > > > > > > > > > > > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl> > > > > --- > > > > The patch has been tested on i.MX6q with vanilla 4.7 and 4.8 > > > > kernels. It applies on 4.9-rcX SHA1: > > > > dcd4693cf47801b7d988ea897519de90dfd25d17. > > > > > > > > --- > > > > drivers/video/backlight/pwm_bl.c | 10 +++++++++- > > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/video/backlight/pwm_bl.c > > > > b/drivers/video/backlight/pwm_bl.c > > > > index 8040fd6..def39e8 100644 > > > > --- a/drivers/video/backlight/pwm_bl.c > > > > +++ b/drivers/video/backlight/pwm_bl.c > > > > @@ -203,7 +203,7 @@ static int pwm_backlight_probe(struct > > > > platform_device *pdev) > > > > struct pwm_bl_data *pb; > > > > int initial_blank = FB_BLANK_UNBLANK; > > > > struct pwm_args pargs; > > > > - int ret; > > > > + int ret, i; > > > > > > > > if (!data) { > > > > ret = pwm_backlight_parse_dt(&pdev->dev, > > > > &defdata); @@ - 349,6 +349,14 @@ static int > > > > pwm_backlight_probe(struct platform_device *pdev) > > > > > > > > bl->props.brightness = data->dft_brightness; > > > > bl->props.power = initial_blank; > > > > + > > > > + if (initial_blank == FB_BLANK_UNBLANK) { > > > > + for (i = 0; i < FB_MAX; i++) > > > > + bl->fb_bl_on[i] = true; > > > > + > > > > + bl->use_count = 1; > > > > + } > > > > + > > > > backlight_update_status(bl); > > > > > > > > platform_set_drvdata(pdev, bl); > > > > -- > > > > 2.1.4 > > > > > > > >
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 8040fd6..def39e8 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -203,7 +203,7 @@ static int pwm_backlight_probe(struct platform_device *pdev) struct pwm_bl_data *pb; int initial_blank = FB_BLANK_UNBLANK; struct pwm_args pargs; - int ret; + int ret, i; if (!data) { ret = pwm_backlight_parse_dt(&pdev->dev, &defdata); @@ -349,6 +349,14 @@ static int pwm_backlight_probe(struct platform_device *pdev) bl->props.brightness = data->dft_brightness; bl->props.power = initial_blank; + + if (initial_blank == FB_BLANK_UNBLANK) { + for (i = 0; i < FB_MAX; i++) + bl->fb_bl_on[i] = true; + + bl->use_count = 1; + } + backlight_update_status(bl); platform_set_drvdata(pdev, bl);
The commit: a55944ca82d287ca099ca90413af857af9086773 has posed some extra restrictions on blanking and unblanking frame buffer device. Unfortunately, pwm_bl driver's probe did not initialize members of struct backlight_device necessary for further blank/unblank operation. This code in case of initial unblank of backlight device (default behaviour) sets use_count to 1 and marks this particular backlight device as used by all available fb devices (since it is not known during probe how much and which fb devices will be assigned). Without this code, the backlight works properly until one tries to blank it manually from sysfs with "echo 1 > /sys/class/graphics/fb0/blank". Since fb_bl_on[0] and use_count were both set to 0, the logic at fb_notifier_callback (@backlight.c) thought that we didn't turn on (unblanked) the backlight device and refuses to disable (blank) it. As a result we see garbage from fb displayed. Signed-off-by: Lukasz Majewski <l.majewski@majess.pl> --- The patch has been tested on i.MX6q with vanilla 4.7 and 4.8 kernels. It applies on 4.9-rcX SHA1: dcd4693cf47801b7d988ea897519de90dfd25d17. --- drivers/video/backlight/pwm_bl.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)