| Message ID | 1358861996-27194-2-git-send-email-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 22, 2013 at 02:39:53PM +0100, Peter Ujfalusi wrote: > It is expected that board files would have: > static unsigned int bl_levels[] = { 0, 50, 100, 150, 200, 250, }; > > static struct platform_pwm_backlight_data bl_data = { > .levels = bl_levels, > .max_brightness = ARRAY_SIZE(bl_levels), > .dft_brightness = 4, > .pwm_period_ns = 7812500, > }; > > In this case the max_brightness would be out of range in the levels array. > Decrement the received max_brightness in every case (DT or non DT) when the > levels has been provided. What's wrong with specifying .max_brightness = ARRAY_SIZE(bl_levels) - 1 instead? Thierry
On 01/28/2013 10:01 PM, Thierry Reding wrote: > On Tue, Jan 22, 2013 at 02:39:53PM +0100, Peter Ujfalusi wrote: >> It is expected that board files would have: >> static unsigned int bl_levels[] = { 0, 50, 100, 150, 200, 250, }; >> >> static struct platform_pwm_backlight_data bl_data = { >> .levels = bl_levels, >> .max_brightness = ARRAY_SIZE(bl_levels), >> .dft_brightness = 4, >> .pwm_period_ns = 7812500, >> }; >> >> In this case the max_brightness would be out of range in the levels array. >> Decrement the received max_brightness in every case (DT or non DT) when the >> levels has been provided. > > What's wrong with specifying .max_brightness = ARRAY_SIZE(bl_levels) - 1 > instead? There is nothing wrong with that either but IMHO it is more natural for board files to use just ARRAY_SIZE(bl_levels). In this way the handling of data->max_brightness among non DT and DT booted kernel is more uniform in the driver itself. Right now all board files are using only the .max_brightness to specify the maximum value, I could not find any users of .levels in the kernel.
On Tue, Jan 29, 2013 at 09:17:04AM +0100, Peter Ujfalusi wrote: > On 01/28/2013 10:01 PM, Thierry Reding wrote: > > On Tue, Jan 22, 2013 at 02:39:53PM +0100, Peter Ujfalusi wrote: > >> It is expected that board files would have: > >> static unsigned int bl_levels[] = { 0, 50, 100, 150, 200, 250, }; > >> > >> static struct platform_pwm_backlight_data bl_data = { > >> .levels = bl_levels, > >> .max_brightness = ARRAY_SIZE(bl_levels), > >> .dft_brightness = 4, > >> .pwm_period_ns = 7812500, > >> }; > >> > >> In this case the max_brightness would be out of range in the levels array. > >> Decrement the received max_brightness in every case (DT or non DT) when the > >> levels has been provided. > > > > What's wrong with specifying .max_brightness = ARRAY_SIZE(bl_levels) - 1 > > instead? > > There is nothing wrong with that either but IMHO it is more natural for board > files to use just ARRAY_SIZE(bl_levels). In this way the handling of > data->max_brightness among non DT and DT booted kernel is more uniform in the > driver itself. The .max_brightness field is defined to be the maximum value that you can set the brightness to. If you use .levels and set .max_brightness to ARRAY_SIZE() then that's no longer true because the maximum value for the brightness will actually be ARRAY_SIZE() - 1. The reason why in the DT case we decrement .max_brightness is that it is used as a temporary variable to keep the number of levels, which corresponds to ARRAY_SIZE() in your example, and adjust it later on to match the definition. That's an implementation detail. Platform data content should be encoded properly without knowledge of the internal implementation. > Right now all board files are using only the .max_brightness to specify the > maximum value, I could not find any users of .levels in the kernel. Yes. But this is the legacy mode which should be considered deprecated. The reason why the concept of brightness-levels was introduced back when the DT bindings were created was that pretty much no hardware in existence actually works that way. This was a topic of discussion at the time when I first proposed these changes, see for example: http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg09472.html Thierry
On 01/29/2013 11:17 AM, Thierry Reding wrote: > On Tue, Jan 29, 2013 at 09:17:04AM +0100, Peter Ujfalusi wrote: >> On 01/28/2013 10:01 PM, Thierry Reding wrote: >>> On Tue, Jan 22, 2013 at 02:39:53PM +0100, Peter Ujfalusi wrote: >>>> It is expected that board files would have: >>>> static unsigned int bl_levels[] = { 0, 50, 100, 150, 200, 250, }; >>>> >>>> static struct platform_pwm_backlight_data bl_data = { >>>> .levels = bl_levels, >>>> .max_brightness = ARRAY_SIZE(bl_levels), >>>> .dft_brightness = 4, >>>> .pwm_period_ns = 7812500, >>>> }; >>>> >>>> In this case the max_brightness would be out of range in the levels array. >>>> Decrement the received max_brightness in every case (DT or non DT) when the >>>> levels has been provided. >>> >>> What's wrong with specifying .max_brightness = ARRAY_SIZE(bl_levels) - 1 >>> instead? >> >> There is nothing wrong with that either but IMHO it is more natural for board >> files to use just ARRAY_SIZE(bl_levels). In this way the handling of >> data->max_brightness among non DT and DT booted kernel is more uniform in the >> driver itself. > > The .max_brightness field is defined to be the maximum value that you > can set the brightness to. If you use .levels and set .max_brightness to > ARRAY_SIZE() then that's no longer true because the maximum value for > the brightness will actually be ARRAY_SIZE() - 1. Yes, in conjunction with .levels it would be better to have .nr_levels instead reusing the max_brightness. > The reason why in the DT case we decrement .max_brightness is that it is > used as a temporary variable to keep the number of levels, which > corresponds to ARRAY_SIZE() in your example, and adjust it later on to > match the definition. That's an implementation detail. > > Platform data content should be encoded properly without knowledge of > the internal implementation. > >> Right now all board files are using only the .max_brightness to specify the >> maximum value, I could not find any users of .levels in the kernel. > > Yes. But this is the legacy mode which should be considered deprecated. > The reason why the concept of brightness-levels was introduced back when > the DT bindings were created was that pretty much no hardware in > existence actually works that way. This was a topic of discussion at the > time when I first proposed these changes, see for example: > > http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg09472.html Right. Now I know the background. However I do not agree with the conclusion. Probably it is fine in some cases to limit the number of configurable duty cycles to have only distinct steps. To not go too far, on my laptop I have: # cat /sys/class/backlight/acpi_video0/max_brightness 15 # cat /sys/class/backlight/intel_backlight/max_brightness 93 In this case I would be more happier if the user space would use the intel_backlight than the acpi_video0. I'm fine if user space decides to allow me only 15 distinct steps on the intel_backlight but I would expect it to do so in a way when I change the brightness in the UI it would step down or up to the next user configurable level. Right now it uses the acpi_video0 to control the levels which means that I have (ugly) jumps in the backlight every time I adjust it. In my phone and tablet all transitions between backlight levels are smooth. This is not a case in my laptop (with exception when the backlight is set to auto, FW controlled). The perceived brightness change depends on the surrounding environment, you can not say that in high level you would need less steps or you need to have less steps in low brightness. If you in a dark room the low changes can be observed, while the same change when you are outside in a sunny day would not reflect the same. Sure we could do the ramps in driver, but what are the parameters? how many step between the two level? What is the time between the steps? If you are about to make a product you will end up specifying all the possible steps to provide the best user experience. So if the PWM have 127 duty cycle you will have levels from 0, 1, 2, 3, ...., 125, 126, 127.
On Tue, Jan 29, 2013 at 01:10:19PM +0100, Peter Ujfalusi wrote: > On 01/29/2013 11:17 AM, Thierry Reding wrote: > > On Tue, Jan 29, 2013 at 09:17:04AM +0100, Peter Ujfalusi wrote: > >> On 01/28/2013 10:01 PM, Thierry Reding wrote: > >>> On Tue, Jan 22, 2013 at 02:39:53PM +0100, Peter Ujfalusi wrote: > >>>> It is expected that board files would have: > >>>> static unsigned int bl_levels[] = { 0, 50, 100, 150, 200, 250, }; > >>>> > >>>> static struct platform_pwm_backlight_data bl_data = { > >>>> .levels = bl_levels, > >>>> .max_brightness = ARRAY_SIZE(bl_levels), > >>>> .dft_brightness = 4, > >>>> .pwm_period_ns = 7812500, > >>>> }; > >>>> > >>>> In this case the max_brightness would be out of range in the levels array. > >>>> Decrement the received max_brightness in every case (DT or non DT) when the > >>>> levels has been provided. > >>> > >>> What's wrong with specifying .max_brightness = ARRAY_SIZE(bl_levels) - 1 > >>> instead? > >> > >> There is nothing wrong with that either but IMHO it is more natural for board > >> files to use just ARRAY_SIZE(bl_levels). In this way the handling of > >> data->max_brightness among non DT and DT booted kernel is more uniform in the > >> driver itself. > > > > The .max_brightness field is defined to be the maximum value that you > > can set the brightness to. If you use .levels and set .max_brightness to > > ARRAY_SIZE() then that's no longer true because the maximum value for > > the brightness will actually be ARRAY_SIZE() - 1. > > Yes, in conjunction with .levels it would be better to have .nr_levels instead > reusing the max_brightness. > > > The reason why in the DT case we decrement .max_brightness is that it is > > used as a temporary variable to keep the number of levels, which > > corresponds to ARRAY_SIZE() in your example, and adjust it later on to > > match the definition. That's an implementation detail. > > > > Platform data content should be encoded properly without knowledge of > > the internal implementation. > > > >> Right now all board files are using only the .max_brightness to specify the > >> maximum value, I could not find any users of .levels in the kernel. > > > > Yes. But this is the legacy mode which should be considered deprecated. > > The reason why the concept of brightness-levels was introduced back when > > the DT bindings were created was that pretty much no hardware in > > existence actually works that way. This was a topic of discussion at the > > time when I first proposed these changes, see for example: > > > > http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg09472.html > > Right. Now I know the background. > However I do not agree with the conclusion. Probably it is fine in some cases > to limit the number of configurable duty cycles to have only distinct steps. > To not go too far, on my laptop I have: > # cat /sys/class/backlight/acpi_video0/max_brightness > 15 > # cat /sys/class/backlight/intel_backlight/max_brightness > 93 FWIW, I have hardware with an Intel chipset that has max_brightness 13666. That doesn't mean all of these are necessarily valid or useful. > In this case I would be more happier if the user space would use the > intel_backlight than the acpi_video0. I'm fine if user space decides to allow > me only 15 distinct steps on the intel_backlight but I would expect it to do > so in a way when I change the brightness in the UI it would step down or up to > the next user configurable level. Right now it uses the acpi_video0 to control > the levels which means that I have (ugly) jumps in the backlight every time I > adjust it. > > In my phone and tablet all transitions between backlight levels are smooth. > This is not a case in my laptop (with exception when the backlight is set to > auto, FW controlled). > > The perceived brightness change depends on the surrounding environment, you > can not say that in high level you would need less steps or you need to have > less steps in low brightness. If you in a dark room the low changes can be > observed, while the same change when you are outside in a sunny day would not > reflect the same. > > Sure we could do the ramps in driver, but what are the parameters? how many > step between the two level? What is the time between the steps? > > If you are about to make a product you will end up specifying all the possible > steps to provide the best user experience. So if the PWM have 127 duty cycle > you will have levels from 0, 1, 2, 3, ...., 125, 126, 127. That's not true. The duty-cycle merely defines a percentage of how long the PWM signal will be high. You can choose an arbitrary number of subdivisions. Since the brightness of a display isn't linearly proportional to the duty cycle of the PWM driving it, representing that brightness by a linear range is misleading. Furthermore some panels have regions where the backlight isn't lit at all or where changes in the PWM duty-cycle don't make any difference. Thierry
On 01/29/2013 01:30 PM, Thierry Reding wrote: >> Right. Now I know the background. >> However I do not agree with the conclusion. Probably it is fine in some cases >> to limit the number of configurable duty cycles to have only distinct steps. >> To not go too far, on my laptop I have: >> # cat /sys/class/backlight/acpi_video0/max_brightness >> 15 >> # cat /sys/class/backlight/intel_backlight/max_brightness >> 93 > > FWIW, I have hardware with an Intel chipset that has max_brightness > 13666. That doesn't mean all of these are necessarily valid or useful. For sure this range is overkill, but if you reduce it to let's say 15 would it be better? I don't think. It is up to the userspace to decide how to use it. User should have full control over the HW in hand. In this particular case I would expect userspace to give you around 20 steps to change brightness and when you change it would step between those so you will have nice, smooth changes and not big jumps. > That's not true. The duty-cycle merely defines a percentage of how long > the PWM signal will be high. You can choose an arbitrary number of > subdivisions. Sure all HW has limitation. The HW I have either have 127 or 255 time slots. Probably the best thing way to deal with the backlight is to have a range of 0 - 100% Nothing else. We should have an API to PWMs so user drivers could get the duty cycle from the HW drivers. In this way backlight would only expose percentage and the backlight driver would get the number of time slots from the PWM core to calculate the actual value for the selected percentage. > Since the brightness of a display isn't linearly proportional to the > duty cycle of the PWM driving it, representing that brightness by a > linear range is misleading. Furthermore some panels have regions where > the backlight isn't lit at all or where changes in the PWM duty-cycle > don't make any difference. and all of this also depend on the surrounding light conditions as well. If the same device is used in low light condition you care about the lower light ranges more compared to when the same device is used in bright environment. In these case the user space has to be adopted to the conditions and one should not need to recompile the kernel/dtb just because the device is used in different environment.
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 069983c..f0d6854 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -142,7 +142,6 @@ static int pwm_backlight_parse_dt(struct device *dev, } data->dft_brightness = value; - data->max_brightness--; } /* @@ -202,6 +201,7 @@ static int pwm_backlight_probe(struct platform_device *pdev) } if (data->levels) { + data->max_brightness--; max = data->levels[data->max_brightness]; pb->levels = data->levels; } else
It is expected that board files would have: static unsigned int bl_levels[] = { 0, 50, 100, 150, 200, 250, }; static struct platform_pwm_backlight_data bl_data = { .levels = bl_levels, .max_brightness = ARRAY_SIZE(bl_levels), .dft_brightness = 4, .pwm_period_ns = 7812500, }; In this case the max_brightness would be out of range in the levels array. Decrement the received max_brightness in every case (DT or non DT) when the levels has been provided. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- drivers/video/backlight/pwm_bl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)