| Message ID | 87389ftvon.fsf@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Jani, First of all, thank you for your explanation. I was unaware of the motivations behind the current implementation. You are exactly right that the whole reason for all this is that userspace is expecting actual_brightness to always match the brightness it set. I would like to point out that I never meant to suggest there was a "functional" bug in the driver. And my motivations were more about testability via sysfs than anything else and the assumption that brightness was supposed to always equal actual_brightness. I really should have been more explicit in pointing this out much sooner if it wasn't clear. Thanks for providing a link to the documentation... I wish I had been more diligent in looking for this in the first place. I'm always more inclined to defer to the documentation. And in light of it, I stand corrected. Although your new patch would likely work, I don't think it's necessary anymore to make brightness==actual_brightness always hold true; since testing for that is the incorrect thing to do in the first place (based on the documentation). Nonetheless, testing brightness from userspace will just have to get a little more creative. Apologies for all the noise... now I'll go make some elsewhere. ;) U. Artie > -----Original Message----- > From: Jani Nikula [mailto:jani.nikula@linux.intel.com] > Sent: Wednesday, November 19, 2014 12:57 AM > To: Eoff, Ullysses A; Stéphane Marchesin > Cc: Jesse Barnes; intel-gfx@lists.freedesktop.org > Subject: RE: [Intel-gfx] [PATCH] drm/i915: expose a fixed brightness range to userspace > > On Wed, 19 Nov 2014, "Eoff, Ullysses A" <ullysses.a.eoff@intel.com> wrote: > >> -----Original Message----- > >> From: Stéphane Marchesin [mailto:stephane.marchesin@gmail.com] > >> Sent: Tuesday, November 18, 2014 3:53 PM > >> To: Eoff, Ullysses A > >> Cc: Jani Nikula; Jesse Barnes; intel-gfx@lists.freedesktop.org > >> Subject: Re: [Intel-gfx] [PATCH] drm/i915: expose a fixed brightness range to userspace > >> > >> On Tue, Nov 18, 2014 at 3:18 PM, Eoff, Ullysses A > >> <ullysses.a.eoff@intel.com> wrote: > >> > Thanks Jesse for the ack. > >> > > >> > Unfortunately I just learned from Stéphane that there > >> > are certain devices which only support 256 levels, so this > >> > patch would do us no good at solving the real issue for > >> > such devices. > >> > > >> > Why can't we just use a dynamic 1:1 mapping as was > >> > suggested before? I would vote for that instead. > >> > > >> > >> Right, from my (consumer's) perspective, a 1:1 mapping is simpler. But > >> the confusing part for me is that (as far as I can see) the current > >> mapping should be 1:1 (because the user and hw ranges are the same), > >> even though it goes through the scale logic. Is the scale() function > >> maybe not the identity? If it isn't, maybe we just need to make it > >> so... > >> > > > > Yes, if the user and hw ranges are the same, then there will be a > > 1:1 mapping, currently, and no issue. It's the other case where > > the hw range is smaller than the user range we end up with > > brightness != actual_brightness in sysfs. The scale logic rounds > > into discrete values of the ranges where multiple user values can > > scale to the same hw value in this case. Right now, user range is > > [0..max hw] and hw range is [min_hw..max_hw]. If min_hw > 0, > > then we encounter the problem. The proposal is to set the user > > range to [0..(hw_max - hw_min)]. > > Some things to consider. > > Have you heard of any requirements to support changing backlight PWM > frequency run time? We currently don't support it, and it would require > a fixed range. The backlight class interface does not support changing > max brightness on the fly. Sure, we can implement this later if > required, but we now have most of what's needed for this in place. > > The luminance of the backlight is not a linear function of the > brightness value set. Currently a single brightness step has a different > luminance change depending on the absolute value. There's been talk > about letting userspace fix this, but I'm not convinced the userspace > has any chance of abstracting the plethora of hardware out there. As it > happens, the ACPI opregion the driver has access to, does have a lookup > table for this. We could fix this in the driver, but not if we commit to > having 1:1 mapping. > > Another thing to consider is that the max value we currently expose is > quite meaningless to the userspace. I question the point of exposing a > range of, say, 0..10000 when in reality you'll only get maybe 100 > distinct levels of brightness, depending on the backlight frequency. > > An interesting and perhaps counter intuitive detail, the higher the PWM > frequency, i.e. the higher the exposed max, the fewer user > distinguishable levels you can actually get from the backlight. This is > due to the rise and fall times in the backlight following the PWM > signal. > > Finally, it seems to me the problem with the scaling boils down to > userspace expecting actual_brightness to always match the brightness it > set. That's the value read back from the hardware. The ABI explicitly > says the brightness stored in the driver may not be the actual > brightness [1]. I don't think there are guarantees that all hardware > would or could maintain the precision either. I think that's broken in > userspace, but we're not supposed to say such things. > > Soo... here's an attempt to be constructive after all the whining > above. ;) How about this to always return the same value if the actual > brightness duty cycle in the hardware has not changed? Totally untested, > of course. > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index 4d63839bd9b4..8678467d5d83 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -1024,7 +1024,12 @@ static int intel_backlight_device_get_brightness(struct backlight_device *bd) > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > > hw_level = intel_panel_get_backlight(connector); > - ret = scale_hw_to_user(connector, hw_level, bd->props.max_brightness); > + if (hw_level == scale_user_to_hw(connector, bd->props.brightness, > + bd->props.max_brightness)) > + ret = bd->props.brightness; > + else > + ret = scale_hw_to_user(connector, hw_level, > + bd->props.max_brightness); > > drm_modeset_unlock(&dev->mode_config.connection_mutex); > intel_runtime_pm_put(dev_priv); > > > BR, > Jani. > > > > [1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/stable/sysfs-class-backlight > > > -- > Jani Nikula, Intel Open Source Technology Center
On Wed, 19 Nov 2014, "Eoff, Ullysses A" <ullysses.a.eoff@intel.com> wrote: > Jani, > > First of all, thank you for your explanation. I was unaware of the > motivations behind the current implementation. You are exactly > right that the whole reason for all this is that userspace is expecting > actual_brightness to always match the brightness it set. > > I would like to point out that I never meant to suggest there was a > "functional" bug in the driver. And my motivations were more about > testability via sysfs than anything else and the assumption that brightness > was supposed to always equal actual_brightness. I really should have > been more explicit in pointing this out much sooner if it wasn't clear. Maybe you missed it, but we did have a reported bug [1] which was fixed (or worked around) by your commit 673e7bbdb3920b62cfc6c710bea626b0a9b0f43a Author: U. Artie Eoff <ullysses.a.eoff@intel.com> Date: Mon Sep 29 15:49:32 2014 -0700 drm/i915: intel_backlight scale() math WA and there was the same discussion about scaling problems as here. It's a real issue, no need for regrets on your part. [1] https://bugzilla.kernel.org/show_bug.cgi?id=85861 > Thanks for providing a link to the documentation... I wish I had been > more diligent in looking for this in the first place. I'm always more > inclined to defer to the documentation. And in light of it, I stand > corrected. > > Although your new patch would likely work, I don't think it's necessary > anymore to make brightness==actual_brightness always hold true; since > testing for that is the incorrect thing to do in the first place (based on > the documentation). Nonetheless, testing brightness from userspace will > just have to get a little more creative. I don't think there's anything really horribly wrong with the patch, so I'm starting to think maybe we should just apply it. I assume it would help your scenario too. If there's still userspace out there that makes the assumption anyway, and fails because of it, we may not even have a choice. Thoughts? > Apologies for all the noise... now I'll go make some elsewhere. ;) Oh, never mind about that. BR, Jani. > > U. Artie > >> -----Original Message----- >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com] >> Sent: Wednesday, November 19, 2014 12:57 AM >> To: Eoff, Ullysses A; Stéphane Marchesin >> Cc: Jesse Barnes; intel-gfx@lists.freedesktop.org >> Subject: RE: [Intel-gfx] [PATCH] drm/i915: expose a fixed brightness range to userspace >> >> On Wed, 19 Nov 2014, "Eoff, Ullysses A" <ullysses.a.eoff@intel.com> wrote: >> >> -----Original Message----- >> >> From: Stéphane Marchesin [mailto:stephane.marchesin@gmail.com] >> >> Sent: Tuesday, November 18, 2014 3:53 PM >> >> To: Eoff, Ullysses A >> >> Cc: Jani Nikula; Jesse Barnes; intel-gfx@lists.freedesktop.org >> >> Subject: Re: [Intel-gfx] [PATCH] drm/i915: expose a fixed brightness range to userspace >> >> >> >> On Tue, Nov 18, 2014 at 3:18 PM, Eoff, Ullysses A >> >> <ullysses.a.eoff@intel.com> wrote: >> >> > Thanks Jesse for the ack. >> >> > >> >> > Unfortunately I just learned from Stéphane that there >> >> > are certain devices which only support 256 levels, so this >> >> > patch would do us no good at solving the real issue for >> >> > such devices. >> >> > >> >> > Why can't we just use a dynamic 1:1 mapping as was >> >> > suggested before? I would vote for that instead. >> >> > >> >> >> >> Right, from my (consumer's) perspective, a 1:1 mapping is simpler. But >> >> the confusing part for me is that (as far as I can see) the current >> >> mapping should be 1:1 (because the user and hw ranges are the same), >> >> even though it goes through the scale logic. Is the scale() function >> >> maybe not the identity? If it isn't, maybe we just need to make it >> >> so... >> >> >> > >> > Yes, if the user and hw ranges are the same, then there will be a >> > 1:1 mapping, currently, and no issue. It's the other case where >> > the hw range is smaller than the user range we end up with >> > brightness != actual_brightness in sysfs. The scale logic rounds >> > into discrete values of the ranges where multiple user values can >> > scale to the same hw value in this case. Right now, user range is >> > [0..max hw] and hw range is [min_hw..max_hw]. If min_hw > 0, >> > then we encounter the problem. The proposal is to set the user >> > range to [0..(hw_max - hw_min)]. >> >> Some things to consider. >> >> Have you heard of any requirements to support changing backlight PWM >> frequency run time? We currently don't support it, and it would require >> a fixed range. The backlight class interface does not support changing >> max brightness on the fly. Sure, we can implement this later if >> required, but we now have most of what's needed for this in place. >> >> The luminance of the backlight is not a linear function of the >> brightness value set. Currently a single brightness step has a different >> luminance change depending on the absolute value. There's been talk >> about letting userspace fix this, but I'm not convinced the userspace >> has any chance of abstracting the plethora of hardware out there. As it >> happens, the ACPI opregion the driver has access to, does have a lookup >> table for this. We could fix this in the driver, but not if we commit to >> having 1:1 mapping. >> >> Another thing to consider is that the max value we currently expose is >> quite meaningless to the userspace. I question the point of exposing a >> range of, say, 0..10000 when in reality you'll only get maybe 100 >> distinct levels of brightness, depending on the backlight frequency. >> >> An interesting and perhaps counter intuitive detail, the higher the PWM >> frequency, i.e. the higher the exposed max, the fewer user >> distinguishable levels you can actually get from the backlight. This is >> due to the rise and fall times in the backlight following the PWM >> signal. >> >> Finally, it seems to me the problem with the scaling boils down to >> userspace expecting actual_brightness to always match the brightness it >> set. That's the value read back from the hardware. The ABI explicitly >> says the brightness stored in the driver may not be the actual >> brightness [1]. I don't think there are guarantees that all hardware >> would or could maintain the precision either. I think that's broken in >> userspace, but we're not supposed to say such things. >> >> Soo... here's an attempt to be constructive after all the whining >> above. ;) How about this to always return the same value if the actual >> brightness duty cycle in the hardware has not changed? Totally untested, >> of course. >> >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c >> index 4d63839bd9b4..8678467d5d83 100644 >> --- a/drivers/gpu/drm/i915/intel_panel.c >> +++ b/drivers/gpu/drm/i915/intel_panel.c >> @@ -1024,7 +1024,12 @@ static int intel_backlight_device_get_brightness(struct backlight_device *bd) >> drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); >> >> hw_level = intel_panel_get_backlight(connector); >> - ret = scale_hw_to_user(connector, hw_level, bd->props.max_brightness); >> + if (hw_level == scale_user_to_hw(connector, bd->props.brightness, >> + bd->props.max_brightness)) >> + ret = bd->props.brightness; >> + else >> + ret = scale_hw_to_user(connector, hw_level, >> + bd->props.max_brightness); >> >> drm_modeset_unlock(&dev->mode_config.connection_mutex); >> intel_runtime_pm_put(dev_priv); >> >> >> BR, >> Jani. >> >> >> >> [1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/stable/sysfs-class-backlight >> >> >> -- >> Jani Nikula, Intel Open Source Technology Center
> -----Original Message----- > From: Jani Nikula [mailto:jani.nikula@linux.intel.com] > Sent: Thursday, November 20, 2014 12:58 AM > To: Eoff, Ullysses A; Stéphane Marchesin > Cc: Jesse Barnes; intel-gfx@lists.freedesktop.org > Subject: RE: [Intel-gfx] [PATCH] drm/i915: expose a fixed brightness range to userspace > > On Wed, 19 Nov 2014, "Eoff, Ullysses A" <ullysses.a.eoff@intel.com> wrote: > > Jani, > > > > First of all, thank you for your explanation. I was unaware of the > > motivations behind the current implementation. You are exactly > > right that the whole reason for all this is that userspace is expecting > > actual_brightness to always match the brightness it set. > > > > I would like to point out that I never meant to suggest there was a > > "functional" bug in the driver. And my motivations were more about > > testability via sysfs than anything else and the assumption that brightness > > was supposed to always equal actual_brightness. I really should have > > been more explicit in pointing this out much sooner if it wasn't clear. > > Maybe you missed it, but we did have a reported bug [1] which was fixed > (or worked around) by your > > commit 673e7bbdb3920b62cfc6c710bea626b0a9b0f43a > Author: U. Artie Eoff <ullysses.a.eoff@intel.com> > Date: Mon Sep 29 15:49:32 2014 -0700 > > drm/i915: intel_backlight scale() math WA > > and there was the same discussion about scaling problems as here. It's a > real issue, no need for regrets on your part. > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=85861 > I was not aware of this other report. But yeah, that patch was a byproduct of the assumption that brightness always matches actual_brightness. It was that assumption that led me to discover the rounding errors... so the assumption isn't all bad ;) > > Thanks for providing a link to the documentation... I wish I had been > > more diligent in looking for this in the first place. I'm always more > > inclined to defer to the documentation. And in light of it, I stand > > corrected. > > > > Although your new patch would likely work, I don't think it's necessary > > anymore to make brightness==actual_brightness always hold true; since > > testing for that is the incorrect thing to do in the first place (based on > > the documentation). Nonetheless, testing brightness from userspace will > > just have to get a little more creative. > > I don't think there's anything really horribly wrong with the patch, so > I'm starting to think maybe we should just apply it. I assume it would > help your scenario too. If there's still userspace out there that makes > the assumption anyway, and fails because of it, we may not even have a > choice. > > Thoughts? > Right, there's really no harm in having your patch. It will definitely help solve the scenario I'm working on. I'm fine either way even though I know the assumption shouldn't be made from userspace now. > > Apologies for all the noise... now I'll go make some elsewhere. ;) > > Oh, never mind about that. > > Thank you Jani. U. Artie > BR, > Jani. > > > > > U. Artie > > > >> -----Original Message----- > >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com] > >> Sent: Wednesday, November 19, 2014 12:57 AM > >> To: Eoff, Ullysses A; Stéphane Marchesin > >> Cc: Jesse Barnes; intel-gfx@lists.freedesktop.org > >> Subject: RE: [Intel-gfx] [PATCH] drm/i915: expose a fixed brightness range to userspace > >> > >> On Wed, 19 Nov 2014, "Eoff, Ullysses A" <ullysses.a.eoff@intel.com> wrote: > >> >> -----Original Message----- > >> >> From: Stéphane Marchesin [mailto:stephane.marchesin@gmail.com] > >> >> Sent: Tuesday, November 18, 2014 3:53 PM > >> >> To: Eoff, Ullysses A > >> >> Cc: Jani Nikula; Jesse Barnes; intel-gfx@lists.freedesktop.org > >> >> Subject: Re: [Intel-gfx] [PATCH] drm/i915: expose a fixed brightness range to userspace > >> >> > >> >> On Tue, Nov 18, 2014 at 3:18 PM, Eoff, Ullysses A > >> >> <ullysses.a.eoff@intel.com> wrote: > >> >> > Thanks Jesse for the ack. > >> >> > > >> >> > Unfortunately I just learned from Stéphane that there > >> >> > are certain devices which only support 256 levels, so this > >> >> > patch would do us no good at solving the real issue for > >> >> > such devices. > >> >> > > >> >> > Why can't we just use a dynamic 1:1 mapping as was > >> >> > suggested before? I would vote for that instead. > >> >> > > >> >> > >> >> Right, from my (consumer's) perspective, a 1:1 mapping is simpler. But > >> >> the confusing part for me is that (as far as I can see) the current > >> >> mapping should be 1:1 (because the user and hw ranges are the same), > >> >> even though it goes through the scale logic. Is the scale() function > >> >> maybe not the identity? If it isn't, maybe we just need to make it > >> >> so... > >> >> > >> > > >> > Yes, if the user and hw ranges are the same, then there will be a > >> > 1:1 mapping, currently, and no issue. It's the other case where > >> > the hw range is smaller than the user range we end up with > >> > brightness != actual_brightness in sysfs. The scale logic rounds > >> > into discrete values of the ranges where multiple user values can > >> > scale to the same hw value in this case. Right now, user range is > >> > [0..max hw] and hw range is [min_hw..max_hw]. If min_hw > 0, > >> > then we encounter the problem. The proposal is to set the user > >> > range to [0..(hw_max - hw_min)]. > >> > >> Some things to consider. > >> > >> Have you heard of any requirements to support changing backlight PWM > >> frequency run time? We currently don't support it, and it would require > >> a fixed range. The backlight class interface does not support changing > >> max brightness on the fly. Sure, we can implement this later if > >> required, but we now have most of what's needed for this in place. > >> > >> The luminance of the backlight is not a linear function of the > >> brightness value set. Currently a single brightness step has a different > >> luminance change depending on the absolute value. There's been talk > >> about letting userspace fix this, but I'm not convinced the userspace > >> has any chance of abstracting the plethora of hardware out there. As it > >> happens, the ACPI opregion the driver has access to, does have a lookup > >> table for this. We could fix this in the driver, but not if we commit to > >> having 1:1 mapping. > >> > >> Another thing to consider is that the max value we currently expose is > >> quite meaningless to the userspace. I question the point of exposing a > >> range of, say, 0..10000 when in reality you'll only get maybe 100 > >> distinct levels of brightness, depending on the backlight frequency. > >> > >> An interesting and perhaps counter intuitive detail, the higher the PWM > >> frequency, i.e. the higher the exposed max, the fewer user > >> distinguishable levels you can actually get from the backlight. This is > >> due to the rise and fall times in the backlight following the PWM > >> signal. > >> > >> Finally, it seems to me the problem with the scaling boils down to > >> userspace expecting actual_brightness to always match the brightness it > >> set. That's the value read back from the hardware. The ABI explicitly > >> says the brightness stored in the driver may not be the actual > >> brightness [1]. I don't think there are guarantees that all hardware > >> would or could maintain the precision either. I think that's broken in > >> userspace, but we're not supposed to say such things. > >> > >> Soo... here's an attempt to be constructive after all the whining > >> above. ;) How about this to always return the same value if the actual > >> brightness duty cycle in the hardware has not changed? Totally untested, > >> of course. > >> > >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > >> index 4d63839bd9b4..8678467d5d83 100644 > >> --- a/drivers/gpu/drm/i915/intel_panel.c > >> +++ b/drivers/gpu/drm/i915/intel_panel.c > >> @@ -1024,7 +1024,12 @@ static int intel_backlight_device_get_brightness(struct backlight_device *bd) > >> drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > >> > >> hw_level = intel_panel_get_backlight(connector); > >> - ret = scale_hw_to_user(connector, hw_level, bd->props.max_brightness); > >> + if (hw_level == scale_user_to_hw(connector, bd->props.brightness, > >> + bd->props.max_brightness)) > >> + ret = bd->props.brightness; > >> + else > >> + ret = scale_hw_to_user(connector, hw_level, > >> + bd->props.max_brightness); > >> > >> drm_modeset_unlock(&dev->mode_config.connection_mutex); > >> intel_runtime_pm_put(dev_priv); > >> > >> > >> BR, > >> Jani. > >> > >> > >> > >> [1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/stable/sysfs-class-backlight > >> > >> > >> -- > >> Jani Nikula, Intel Open Source Technology Center > > -- > Jani Nikula, Intel Open Source Technology Center
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 4d63839bd9b4..8678467d5d83 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -1024,7 +1024,12 @@ static int intel_backlight_device_get_brightness(struct backlight_device *bd) drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); hw_level = intel_panel_get_backlight(connector); - ret = scale_hw_to_user(connector, hw_level, bd->props.max_brightness); + if (hw_level == scale_user_to_hw(connector, bd->props.brightness, + bd->props.max_brightness)) + ret = bd->props.brightness; + else + ret = scale_hw_to_user(connector, hw_level, + bd->props.max_brightness); drm_modeset_unlock(&dev->mode_config.connection_mutex); intel_runtime_pm_put(dev_priv);