* [PATCH] drm/i915: intel_backlight scale() math WA v2
@ 2014-09-24 15:54 Joe Konno
2014-09-24 16:06 ` Hans de Goede
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Joe Konno @ 2014-09-24 15:54 UTC (permalink / raw)
To: intel-gfx
From: Joe Konno <joe.konno@intel.com>
Improper truncated integer division in the scale() function causes
actual_brightness != brightness. This (partial) work-around should be
sufficient for a majority of use-cases, but it is by no means a complete
solution.
TODO: Determine how best to scale "user" values to "hw" values, and
vice-versa, when the ranges are of different sizes. That would be a
buggy scenario even with this work-around.
The issue was introduced in the following (v3.17-rc1) commit:
6dda730 drm/i915: respect the VBT minimum backlight brightness
v2: (thanks to Chris Wilson) clarify commit message, use rounded division
macro
Signed-off-by: Joe Konno <joe.konno@intel.com>
---
drivers/gpu/drm/i915/intel_panel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index f17ada3..dcdfbb3 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -421,7 +421,7 @@ static uint32_t scale(uint32_t source_val,
/* avoid overflows */
target_val = (uint64_t)(source_val - source_min) *
(target_max - target_min);
- do_div(target_val, source_max - source_min);
+ target_val = DIV_ROUND_CLOSEST(target_val, source_max - source_min);
target_val += target_min;
return target_val;
--
2.1.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH] drm/i915: intel_backlight scale() math WA v2 2014-09-24 15:54 [PATCH] drm/i915: intel_backlight scale() math WA v2 Joe Konno @ 2014-09-24 16:06 ` Hans de Goede 2014-09-24 17:07 ` Jani Nikula 2014-09-26 17:15 ` Eoff, Ullysses A 2014-09-29 13:07 ` Jani Nikula 2 siblings, 1 reply; 14+ messages in thread From: Hans de Goede @ 2014-09-24 16:06 UTC (permalink / raw) To: Joe Konno, intel-gfx Hi, On 09/24/2014 05:54 PM, Joe Konno wrote: > From: Joe Konno <joe.konno@intel.com> > > Improper truncated integer division in the scale() function causes > actual_brightness != brightness. This (partial) work-around should be > sufficient for a majority of use-cases, but it is by no means a complete > solution. > > TODO: Determine how best to scale "user" values to "hw" values, and > vice-versa, when the ranges are of different sizes. That would be a > buggy scenario even with this work-around. > > The issue was introduced in the following (v3.17-rc1) commit: > > 6dda730 drm/i915: respect the VBT minimum backlight brightness > > v2: (thanks to Chris Wilson) clarify commit message, use rounded division > macro I wonder why do scaling at all, why not simply shift hw_min - hw_max range to 0 - (hw_max - hw_min) range and set max_brightness as seen by userspace to (hw_max - hw_min) ? Regards, Hans > > Signed-off-by: Joe Konno <joe.konno@intel.com> > --- > drivers/gpu/drm/i915/intel_panel.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index f17ada3..dcdfbb3 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -421,7 +421,7 @@ static uint32_t scale(uint32_t source_val, > /* avoid overflows */ > target_val = (uint64_t)(source_val - source_min) * > (target_max - target_min); > - do_div(target_val, source_max - source_min); > + target_val = DIV_ROUND_CLOSEST(target_val, source_max - source_min); > target_val += target_min; > > return target_val; > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915: intel_backlight scale() math WA v2 2014-09-24 16:06 ` Hans de Goede @ 2014-09-24 17:07 ` Jani Nikula 2014-09-24 17:41 ` Eoff, Ullysses A 0 siblings, 1 reply; 14+ messages in thread From: Jani Nikula @ 2014-09-24 17:07 UTC (permalink / raw) To: Hans de Goede, Joe Konno, intel-gfx On Wed, 24 Sep 2014, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 09/24/2014 05:54 PM, Joe Konno wrote: >> From: Joe Konno <joe.konno@intel.com> >> >> Improper truncated integer division in the scale() function causes >> actual_brightness != brightness. This (partial) work-around should be >> sufficient for a majority of use-cases, but it is by no means a complete >> solution. >> >> TODO: Determine how best to scale "user" values to "hw" values, and >> vice-versa, when the ranges are of different sizes. That would be a >> buggy scenario even with this work-around. >> >> The issue was introduced in the following (v3.17-rc1) commit: >> >> 6dda730 drm/i915: respect the VBT minimum backlight brightness >> >> v2: (thanks to Chris Wilson) clarify commit message, use rounded division >> macro > > I wonder why do scaling at all, why not simply shift hw_min - hw_max range > to 0 - (hw_max - hw_min) range and set max_brightness as seen by userspace > to (hw_max - hw_min) ? Mostly in preparation for a future where we expose an arbitrary range, say 0..100 or 0..255 to the userspace. BR, Jani. > > Regards, > > Hans > > >> >> Signed-off-by: Joe Konno <joe.konno@intel.com> >> --- >> drivers/gpu/drm/i915/intel_panel.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c >> index f17ada3..dcdfbb3 100644 >> --- a/drivers/gpu/drm/i915/intel_panel.c >> +++ b/drivers/gpu/drm/i915/intel_panel.c >> @@ -421,7 +421,7 @@ static uint32_t scale(uint32_t source_val, >> /* avoid overflows */ >> target_val = (uint64_t)(source_val - source_min) * >> (target_max - target_min); >> - do_div(target_val, source_max - source_min); >> + target_val = DIV_ROUND_CLOSEST(target_val, source_max - source_min); >> target_val += target_min; >> >> return target_val; >> > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915: intel_backlight scale() math WA v2 2014-09-24 17:07 ` Jani Nikula @ 2014-09-24 17:41 ` Eoff, Ullysses A 2014-11-08 17:03 ` Eoff, Ullysses A 0 siblings, 1 reply; 14+ messages in thread From: Eoff, Ullysses A @ 2014-09-24 17:41 UTC (permalink / raw) To: Jani Nikula, Hans de Goede, Joe Konno, intel-gfx@lists.freedesktop.org > -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Jani Nikula > Sent: Wednesday, September 24, 2014 10:08 AM > To: Hans de Goede; Joe Konno; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915: intel_backlight scale() math WA v2 > > On Wed, 24 Sep 2014, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > > > On 09/24/2014 05:54 PM, Joe Konno wrote: > >> From: Joe Konno <joe.konno@intel.com> > >> > >> Improper truncated integer division in the scale() function causes > >> actual_brightness != brightness. This (partial) work-around should be > >> sufficient for a majority of use-cases, but it is by no means a complete > >> solution. > >> > >> TODO: Determine how best to scale "user" values to "hw" values, and > >> vice-versa, when the ranges are of different sizes. That would be a > >> buggy scenario even with this work-around. > >> > >> The issue was introduced in the following (v3.17-rc1) commit: > >> > >> 6dda730 drm/i915: respect the VBT minimum backlight brightness > >> > >> v2: (thanks to Chris Wilson) clarify commit message, use rounded division > >> macro > > > > I wonder why do scaling at all, why not simply shift hw_min - hw_max range > > to 0 - (hw_max - hw_min) range and set max_brightness as seen by userspace > > to (hw_max - hw_min) ? > > Mostly in preparation for a future where we expose an arbitrary range, > say 0..100 or 0..255 to the userspace. > The problem with this scaling method is that scaling from user level to hw level and back to user level is ambiguous since there isn't a 1:1 mapping between the user range and the hw range. On the other hand, this patch does fix a bug in the currently used method (scaling). That, at least, is an improvement nonetheless. U. Artie > BR, > Jani. > > > > > > Regards, > > > > Hans > > > > > >> > >> Signed-off-by: Joe Konno <joe.konno@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_panel.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > >> index f17ada3..dcdfbb3 100644 > >> --- a/drivers/gpu/drm/i915/intel_panel.c > >> +++ b/drivers/gpu/drm/i915/intel_panel.c > >> @@ -421,7 +421,7 @@ static uint32_t scale(uint32_t source_val, > >> /* avoid overflows */ > >> target_val = (uint64_t)(source_val - source_min) * > >> (target_max - target_min); > >> - do_div(target_val, source_max - source_min); > >> + target_val = DIV_ROUND_CLOSEST(target_val, source_max - source_min); > >> target_val += target_min; > >> > >> return target_val; > >> > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915: intel_backlight scale() math WA v2 2014-09-24 17:41 ` Eoff, Ullysses A @ 2014-11-08 17:03 ` Eoff, Ullysses A 2014-11-10 11:16 ` Jani Nikula 0 siblings, 1 reply; 14+ messages in thread From: Eoff, Ullysses A @ 2014-11-08 17:03 UTC (permalink / raw) To: Jani Nikula, Hans de Goede, Joe Konno, intel-gfx@lists.freedesktop.org On 09/24/2014 10:42 AM, Eoff, Ullysses A wrote: >> -----Original Message----- >> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Jani Nikula >> Sent: Wednesday, September 24, 2014 10:08 AM >> To: Hans de Goede; Joe Konno; intel-gfx@lists.freedesktop.org >> Subject: Re: [Intel-gfx] [PATCH] drm/i915: intel_backlight scale() math WA v2 >> >> On Wed, 24 Sep 2014, Hans de Goede <hdegoede@redhat.com> wrote: >>> Hi, >>> >>> On 09/24/2014 05:54 PM, Joe Konno wrote: >>>> From: Joe Konno <joe.konno@intel.com> >>>> >>>> Improper truncated integer division in the scale() function causes >>>> actual_brightness != brightness. This (partial) work-around should be >>>> sufficient for a majority of use-cases, but it is by no means a complete >>>> solution. >>>> >>>> TODO: Determine how best to scale "user" values to "hw" values, and >>>> vice-versa, when the ranges are of different sizes. That would be a >>>> buggy scenario even with this work-around. >>>> >>>> The issue was introduced in the following (v3.17-rc1) commit: >>>> >>>> 6dda730 drm/i915: respect the VBT minimum backlight brightness >>>> >>>> v2: (thanks to Chris Wilson) clarify commit message, use rounded division >>>> macro >>> I wonder why do scaling at all, why not simply shift hw_min - hw_max range >>> to 0 - (hw_max - hw_min) range and set max_brightness as seen by userspace >>> to (hw_max - hw_min) ? >> Mostly in preparation for a future where we expose an arbitrary range, >> say 0..100 or 0..255 to the userspace. >> > The problem with this scaling method is that scaling from user level to hw level and > back to user level is ambiguous since there isn't a 1:1 mapping between the user > range and the hw range. > > On the other hand, this patch does fix a bug in the currently used method (scaling). > That, at least, is an improvement nonetheless. > > U. Artie Apologies for resurrecting an old thread. But I think we still need to address this issue about not having a 1:1 mapping between user and hw levels. Right now, the problem is that the user range is larger than the hw range which results in one or more user levels mapping to the same hw level. And when userspace requests one of those levels, the result that is reported back to userspace might not be the same as what was requested. Take for example, on my system the hw range is [398, 7812] and the user range is [0, 7812]. Suppose userspace requests level 7017. This maps to hw level 7058. And when userspace requests the current level, 7018 is reported back (+1 from what was originally requested). In fact, with these particular ranges, there are exactly 398 values that this occurs. This problem will be compounded the larger the difference in length of the discrete ranges; so long as user range > hw range. Hans' solution would fix this problem, giving 1:1 mapping from hw to user levels. Jani's [future] solution would work too, since exposing a smaller range to userspace than the hw range would isolate the non 1:1 mapping inside the driver. U. Artie >> BR, >> Jani. >> >> >>> Regards, >>> >>> Hans >>> >>> >>>> Signed-off-by: Joe Konno <joe.konno@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_panel.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c >>>> index f17ada3..dcdfbb3 100644 >>>> --- a/drivers/gpu/drm/i915/intel_panel.c >>>> +++ b/drivers/gpu/drm/i915/intel_panel.c >>>> @@ -421,7 +421,7 @@ static uint32_t scale(uint32_t source_val, >>>> /* avoid overflows */ >>>> target_val = (uint64_t)(source_val - source_min) * >>>> (target_max - target_min); >>>> - do_div(target_val, source_max - source_min); >>>> + target_val = DIV_ROUND_CLOSEST(target_val, source_max - source_min); >>>> target_val += target_min; >>>> >>>> return target_val; >>>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> -- >> Jani Nikula, Intel Open Source Technology Center >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915: intel_backlight scale() math WA v2 2014-11-08 17:03 ` Eoff, Ullysses A @ 2014-11-10 11:16 ` Jani Nikula 2014-11-10 14:15 ` Jani Nikula 0 siblings, 1 reply; 14+ messages in thread From: Jani Nikula @ 2014-11-10 11:16 UTC (permalink / raw) To: Eoff, Ullysses A, Hans de Goede, Joe Konno, intel-gfx@lists.freedesktop.org On Sat, 08 Nov 2014, "Eoff, Ullysses A" <ullysses.a.eoff@intel.com> wrote: > On 09/24/2014 10:42 AM, Eoff, Ullysses A wrote: >>> -----Original Message----- >>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Jani Nikula >>> Sent: Wednesday, September 24, 2014 10:08 AM >>> To: Hans de Goede; Joe Konno; intel-gfx@lists.freedesktop.org >>> Subject: Re: [Intel-gfx] [PATCH] drm/i915: intel_backlight scale() math WA v2 >>> >>> On Wed, 24 Sep 2014, Hans de Goede <hdegoede@redhat.com> wrote: >>>> Hi, >>>> >>>> On 09/24/2014 05:54 PM, Joe Konno wrote: >>>>> From: Joe Konno <joe.konno@intel.com> >>>>> >>>>> Improper truncated integer division in the scale() function causes >>>>> actual_brightness != brightness. This (partial) work-around should be >>>>> sufficient for a majority of use-cases, but it is by no means a complete >>>>> solution. >>>>> >>>>> TODO: Determine how best to scale "user" values to "hw" values, and >>>>> vice-versa, when the ranges are of different sizes. That would be a >>>>> buggy scenario even with this work-around. >>>>> >>>>> The issue was introduced in the following (v3.17-rc1) commit: >>>>> >>>>> 6dda730 drm/i915: respect the VBT minimum backlight brightness >>>>> >>>>> v2: (thanks to Chris Wilson) clarify commit message, use rounded division >>>>> macro >>>> I wonder why do scaling at all, why not simply shift hw_min - hw_max range >>>> to 0 - (hw_max - hw_min) range and set max_brightness as seen by userspace >>>> to (hw_max - hw_min) ? >>> Mostly in preparation for a future where we expose an arbitrary range, >>> say 0..100 or 0..255 to the userspace. >>> >> The problem with this scaling method is that scaling from user level to hw level and >> back to user level is ambiguous since there isn't a 1:1 mapping between the user >> range and the hw range. >> >> On the other hand, this patch does fix a bug in the currently used method (scaling). >> That, at least, is an improvement nonetheless. >> >> U. Artie > Apologies for resurrecting an old thread. But I think we still need to > address > this issue about not having a 1:1 mapping between user and hw levels. > > Right now, the problem is that the user range is larger than the hw > range which > results in one or more user levels mapping to the same hw level. And when > userspace requests one of those levels, the result that is reported back to > userspace might not be the same as what was requested. Take for example, on > my system the hw range is [398, 7812] and the user range is [0, 7812]. > Suppose > userspace requests level 7017. This maps to hw level 7058. And when > userspace requests the current level, 7018 is reported back (+1 from what > was originally requested). In fact, with these particular ranges, there > are exactly > 398 values that this occurs. > > This problem will be compounded the larger the difference in length of the > discrete ranges; so long as user range > hw range. > > Hans' solution would fix this problem, giving 1:1 mapping from hw to user > levels. > > Jani's [future] solution would work too, since exposing a smaller range to > userspace than the hw range would isolate the non 1:1 mapping inside the > driver. I think we should just pick an arbitrary range, say 0..100, and be done with it. It's not like you'd be able to get much more than 100 distinct brightness levels out of the backlight anyway, no matter what the PWM settings. BR, Jani. > > U. Artie >>> BR, >>> Jani. >>> >>> >>>> Regards, >>>> >>>> Hans >>>> >>>> >>>>> Signed-off-by: Joe Konno <joe.konno@intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/intel_panel.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c >>>>> index f17ada3..dcdfbb3 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_panel.c >>>>> +++ b/drivers/gpu/drm/i915/intel_panel.c >>>>> @@ -421,7 +421,7 @@ static uint32_t scale(uint32_t source_val, >>>>> /* avoid overflows */ >>>>> target_val = (uint64_t)(source_val - source_min) * >>>>> (target_max - target_min); >>>>> - do_div(target_val, source_max - source_min); >>>>> + target_val = DIV_ROUND_CLOSEST(target_val, source_max - source_min); >>>>> target_val += target_min; >>>>> >>>>> return target_val; >>>>> >>>> _______________________________________________ >>>> Intel-gfx mailing list >>>> Intel-gfx@lists.freedesktop.org >>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >>> -- >>> Jani Nikula, Intel Open Source Technology Center >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> > -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915: intel_backlight scale() math WA v2 2014-11-10 11:16 ` Jani Nikula @ 2014-11-10 14:15 ` Jani Nikula 2014-11-10 17:08 ` Jesse Barnes 0 siblings, 1 reply; 14+ messages in thread From: Jani Nikula @ 2014-11-10 14:15 UTC (permalink / raw) To: Eoff, Ullysses A, Hans de Goede, Joe Konno, intel-gfx@lists.freedesktop.org On Mon, 10 Nov 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Sat, 08 Nov 2014, "Eoff, Ullysses A" <ullysses.a.eoff@intel.com> wrote: >> On 09/24/2014 10:42 AM, Eoff, Ullysses A wrote: >>>> -----Original Message----- >>>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Jani Nikula >>>> Sent: Wednesday, September 24, 2014 10:08 AM >>>> To: Hans de Goede; Joe Konno; intel-gfx@lists.freedesktop.org >>>> Subject: Re: [Intel-gfx] [PATCH] drm/i915: intel_backlight scale() math WA v2 >>>> >>>> On Wed, 24 Sep 2014, Hans de Goede <hdegoede@redhat.com> wrote: >>>>> Hi, >>>>> >>>>> On 09/24/2014 05:54 PM, Joe Konno wrote: >>>>>> From: Joe Konno <joe.konno@intel.com> >>>>>> >>>>>> Improper truncated integer division in the scale() function causes >>>>>> actual_brightness != brightness. This (partial) work-around should be >>>>>> sufficient for a majority of use-cases, but it is by no means a complete >>>>>> solution. >>>>>> >>>>>> TODO: Determine how best to scale "user" values to "hw" values, and >>>>>> vice-versa, when the ranges are of different sizes. That would be a >>>>>> buggy scenario even with this work-around. >>>>>> >>>>>> The issue was introduced in the following (v3.17-rc1) commit: >>>>>> >>>>>> 6dda730 drm/i915: respect the VBT minimum backlight brightness >>>>>> >>>>>> v2: (thanks to Chris Wilson) clarify commit message, use rounded division >>>>>> macro >>>>> I wonder why do scaling at all, why not simply shift hw_min - hw_max range >>>>> to 0 - (hw_max - hw_min) range and set max_brightness as seen by userspace >>>>> to (hw_max - hw_min) ? >>>> Mostly in preparation for a future where we expose an arbitrary range, >>>> say 0..100 or 0..255 to the userspace. >>>> >>> The problem with this scaling method is that scaling from user level to hw level and >>> back to user level is ambiguous since there isn't a 1:1 mapping between the user >>> range and the hw range. >>> >>> On the other hand, this patch does fix a bug in the currently used method (scaling). >>> That, at least, is an improvement nonetheless. >>> >>> U. Artie >> Apologies for resurrecting an old thread. But I think we still need to >> address >> this issue about not having a 1:1 mapping between user and hw levels. >> >> Right now, the problem is that the user range is larger than the hw >> range which >> results in one or more user levels mapping to the same hw level. And when >> userspace requests one of those levels, the result that is reported back to >> userspace might not be the same as what was requested. Take for example, on >> my system the hw range is [398, 7812] and the user range is [0, 7812]. >> Suppose >> userspace requests level 7017. This maps to hw level 7058. And when >> userspace requests the current level, 7018 is reported back (+1 from what >> was originally requested). In fact, with these particular ranges, there >> are exactly >> 398 values that this occurs. >> >> This problem will be compounded the larger the difference in length of the >> discrete ranges; so long as user range > hw range. >> >> Hans' solution would fix this problem, giving 1:1 mapping from hw to user >> levels. >> >> Jani's [future] solution would work too, since exposing a smaller range to >> userspace than the hw range would isolate the non 1:1 mapping inside the >> driver. > > I think we should just pick an arbitrary range, say 0..100, and be done > with it. It's not like you'd be able to get much more than 100 distinct > brightness levels out of the backlight anyway, no matter what the PWM > settings. > > BR, > Jani. PS. This (totally untested) patch should do it: diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index b001c90312e7..a6680081415b 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -1035,7 +1035,7 @@ static int intel_backlight_device_register(struct intel_connector *connector) * Note: Everything should work even if the backlight device max * presented to the userspace is arbitrarily chosen. */ - props.max_brightness = panel->backlight.max; + props.max_brightness = 100; props.brightness = scale_hw_to_user(connector, panel->backlight.level, props.max_brightness); > > > > >> >> U. Artie >>>> BR, >>>> Jani. >>>> >>>> >>>>> Regards, >>>>> >>>>> Hans >>>>> >>>>> >>>>>> Signed-off-by: Joe Konno <joe.konno@intel.com> >>>>>> --- >>>>>> drivers/gpu/drm/i915/intel_panel.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c >>>>>> index f17ada3..dcdfbb3 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_panel.c >>>>>> +++ b/drivers/gpu/drm/i915/intel_panel.c >>>>>> @@ -421,7 +421,7 @@ static uint32_t scale(uint32_t source_val, >>>>>> /* avoid overflows */ >>>>>> target_val = (uint64_t)(source_val - source_min) * >>>>>> (target_max - target_min); >>>>>> - do_div(target_val, source_max - source_min); >>>>>> + target_val = DIV_ROUND_CLOSEST(target_val, source_max - source_min); >>>>>> target_val += target_min; >>>>>> >>>>>> return target_val; >>>>>> >>>>> _______________________________________________ >>>>> Intel-gfx mailing list >>>>> Intel-gfx@lists.freedesktop.org >>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >>>> -- >>>> Jani Nikula, Intel Open Source Technology Center >>>> _______________________________________________ >>>> Intel-gfx mailing list >>>> Intel-gfx@lists.freedesktop.org >>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >>> >> > > -- > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915: intel_backlight scale() math WA v2 2014-11-10 14:15 ` Jani Nikula @ 2014-11-10 17:08 ` Jesse Barnes 0 siblings, 0 replies; 14+ messages in thread From: Jesse Barnes @ 2014-11-10 17:08 UTC (permalink / raw) To: Jani Nikula; +Cc: intel-gfx@lists.freedesktop.org, Joe Konno On Mon, 10 Nov 2014 16:15:57 +0200 Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Mon, 10 Nov 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Sat, 08 Nov 2014, "Eoff, Ullysses A" <ullysses.a.eoff@intel.com> > > wrote: > >> On 09/24/2014 10:42 AM, Eoff, Ullysses A wrote: > >>>> -----Original Message----- > >>>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] > >>>> On Behalf Of Jani Nikula Sent: Wednesday, September 24, 2014 > >>>> 10:08 AM To: Hans de Goede; Joe Konno; > >>>> intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH] > >>>> drm/i915: intel_backlight scale() math WA v2 > >>>> > >>>> On Wed, 24 Sep 2014, Hans de Goede <hdegoede@redhat.com> wrote: > >>>>> Hi, > >>>>> > >>>>> On 09/24/2014 05:54 PM, Joe Konno wrote: > >>>>>> From: Joe Konno <joe.konno@intel.com> > >>>>>> > >>>>>> Improper truncated integer division in the scale() function > >>>>>> causes actual_brightness != brightness. This (partial) > >>>>>> work-around should be sufficient for a majority of use-cases, > >>>>>> but it is by no means a complete solution. > >>>>>> > >>>>>> TODO: Determine how best to scale "user" values to "hw" > >>>>>> values, and vice-versa, when the ranges are of different > >>>>>> sizes. That would be a buggy scenario even with this > >>>>>> work-around. > >>>>>> > >>>>>> The issue was introduced in the following (v3.17-rc1) commit: > >>>>>> > >>>>>> 6dda730 drm/i915: respect the VBT minimum backlight > >>>>>> brightness > >>>>>> > >>>>>> v2: (thanks to Chris Wilson) clarify commit message, use > >>>>>> rounded division macro > >>>>> I wonder why do scaling at all, why not simply shift hw_min - > >>>>> hw_max range to 0 - (hw_max - hw_min) range and set > >>>>> max_brightness as seen by userspace to (hw_max - hw_min) ? > >>>> Mostly in preparation for a future where we expose an arbitrary > >>>> range, say 0..100 or 0..255 to the userspace. > >>>> > >>> The problem with this scaling method is that scaling from user > >>> level to hw level and back to user level is ambiguous since there > >>> isn't a 1:1 mapping between the user range and the hw range. > >>> > >>> On the other hand, this patch does fix a bug in the currently > >>> used method (scaling). That, at least, is an improvement > >>> nonetheless. > >>> > >>> U. Artie > >> Apologies for resurrecting an old thread. But I think we still > >> need to address > >> this issue about not having a 1:1 mapping between user and hw > >> levels. > >> > >> Right now, the problem is that the user range is larger than the hw > >> range which > >> results in one or more user levels mapping to the same hw level. > >> And when userspace requests one of those levels, the result that > >> is reported back to userspace might not be the same as what was > >> requested. Take for example, on my system the hw range is [398, > >> 7812] and the user range is [0, 7812]. Suppose > >> userspace requests level 7017. This maps to hw level 7058. And > >> when userspace requests the current level, 7018 is reported back > >> (+1 from what was originally requested). In fact, with these > >> particular ranges, there are exactly > >> 398 values that this occurs. > >> > >> This problem will be compounded the larger the difference in > >> length of the discrete ranges; so long as user range > hw range. > >> > >> Hans' solution would fix this problem, giving 1:1 mapping from hw > >> to user levels. > >> > >> Jani's [future] solution would work too, since exposing a smaller > >> range to userspace than the hw range would isolate the non 1:1 > >> mapping inside the driver. > > > > I think we should just pick an arbitrary range, say 0..100, and be > > done with it. It's not like you'd be able to get much more than 100 > > distinct brightness levels out of the backlight anyway, no matter > > what the PWM settings. > > > > BR, > > Jani. > > PS. This (totally untested) patch should do it: > > diff --git a/drivers/gpu/drm/i915/intel_panel.c > b/drivers/gpu/drm/i915/intel_panel.c index b001c90312e7..a6680081415b > 100644 --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -1035,7 +1035,7 @@ static int > intel_backlight_device_register(struct intel_connector *connector) > * Note: Everything should work even if the backlight device > max > * presented to the userspace is arbitrarily chosen. > */ > - props.max_brightness = panel->backlight.max; > + props.max_brightness = 100; > props.brightness = scale_hw_to_user(connector, > panel->backlight.level, > props.max_brightness); 100% agreed on exposing a fixed range. But iirc Keith did some playing around with fading in and out of backlights and found that we needed about 1000 levels to make it smooth (definitely possible on some platforms, though not all). So my only nitpick would be that we have a range that allows a bit more precision. Thanks, Jesse _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915: intel_backlight scale() math WA v2 2014-09-24 15:54 [PATCH] drm/i915: intel_backlight scale() math WA v2 Joe Konno 2014-09-24 16:06 ` Hans de Goede @ 2014-09-26 17:15 ` Eoff, Ullysses A 2014-09-29 13:07 ` Jani Nikula 2 siblings, 0 replies; 14+ messages in thread From: Eoff, Ullysses A @ 2014-09-26 17:15 UTC (permalink / raw) To: Joe Konno, intel-gfx@lists.freedesktop.org > -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Joe Konno > Sent: Wednesday, September 24, 2014 8:55 AM > To: intel-gfx@lists.freedesktop.org > Subject: [Intel-gfx] [PATCH] drm/i915: intel_backlight scale() math WA v2 > > From: Joe Konno <joe.konno@intel.com> > > Improper truncated integer division in the scale() function causes > actual_brightness != brightness. This (partial) work-around should be > sufficient for a majority of use-cases, but it is by no means a complete > solution. > > TODO: Determine how best to scale "user" values to "hw" values, and > vice-versa, when the ranges are of different sizes. That would be a > buggy scenario even with this work-around. > > The issue was introduced in the following (v3.17-rc1) commit: > > 6dda730 drm/i915: respect the VBT minimum backlight brightness > > v2: (thanks to Chris Wilson) clarify commit message, use rounded division > macro > Reviewed-by: U. Artie Eoff <ullysses.a.eoff@intel.com> > Signed-off-by: Joe Konno <joe.konno@intel.com> > --- > drivers/gpu/drm/i915/intel_panel.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index f17ada3..dcdfbb3 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -421,7 +421,7 @@ static uint32_t scale(uint32_t source_val, > /* avoid overflows */ > target_val = (uint64_t)(source_val - source_min) * > (target_max - target_min); > - do_div(target_val, source_max - source_min); > + target_val = DIV_ROUND_CLOSEST(target_val, source_max - source_min); > target_val += target_min; > > return target_val; > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915: intel_backlight scale() math WA v2 2014-09-24 15:54 [PATCH] drm/i915: intel_backlight scale() math WA v2 Joe Konno 2014-09-24 16:06 ` Hans de Goede 2014-09-26 17:15 ` Eoff, Ullysses A @ 2014-09-29 13:07 ` Jani Nikula 2014-09-29 17:50 ` Eoff, Ullysses A 2 siblings, 1 reply; 14+ messages in thread From: Jani Nikula @ 2014-09-29 13:07 UTC (permalink / raw) To: Joe Konno, intel-gfx On Wed, 24 Sep 2014, Joe Konno <joe.konno@linux.intel.com> wrote: > From: Joe Konno <joe.konno@intel.com> > > Improper truncated integer division in the scale() function causes > actual_brightness != brightness. This (partial) work-around should be > sufficient for a majority of use-cases, but it is by no means a complete > solution. > > TODO: Determine how best to scale "user" values to "hw" values, and > vice-versa, when the ranges are of different sizes. That would be a > buggy scenario even with this work-around. > > The issue was introduced in the following (v3.17-rc1) commit: > > 6dda730 drm/i915: respect the VBT minimum backlight brightness > > v2: (thanks to Chris Wilson) clarify commit message, use rounded division > macro > > Signed-off-by: Joe Konno <joe.konno@intel.com> > --- > drivers/gpu/drm/i915/intel_panel.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index f17ada3..dcdfbb3 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -421,7 +421,7 @@ static uint32_t scale(uint32_t source_val, > /* avoid overflows */ > target_val = (uint64_t)(source_val - source_min) * > (target_max - target_min); > - do_div(target_val, source_max - source_min); > + target_val = DIV_ROUND_CLOSEST(target_val, source_max - source_min); This fails to build with CONFIG_X86_32=y: drivers/built-in.o: In function `scale': intel_panel.c:(.text+0x12ab38): undefined reference to `__udivdi3' make: *** [vmlinux] Error 1 BR, Jani. > target_val += target_min; > > return target_val; > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915: intel_backlight scale() math WA v2 2014-09-29 13:07 ` Jani Nikula @ 2014-09-29 17:50 ` Eoff, Ullysses A 2014-09-29 18:46 ` Chris Wilson 2014-09-29 19:31 ` Damien Lespiau 0 siblings, 2 replies; 14+ messages in thread From: Eoff, Ullysses A @ 2014-09-29 17:50 UTC (permalink / raw) To: Jani Nikula, Joe Konno, intel-gfx@lists.freedesktop.org > -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Jani Nikula > Sent: Monday, September 29, 2014 6:07 AM > To: Joe Konno; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915: intel_backlight scale() math WA v2 > > On Wed, 24 Sep 2014, Joe Konno <joe.konno@linux.intel.com> wrote: > > From: Joe Konno <joe.konno@intel.com> > > > > Improper truncated integer division in the scale() function causes > > actual_brightness != brightness. This (partial) work-around should be > > sufficient for a majority of use-cases, but it is by no means a complete > > solution. > > > > TODO: Determine how best to scale "user" values to "hw" values, and > > vice-versa, when the ranges are of different sizes. That would be a > > buggy scenario even with this work-around. > > > > The issue was introduced in the following (v3.17-rc1) commit: > > > > 6dda730 drm/i915: respect the VBT minimum backlight brightness > > > > v2: (thanks to Chris Wilson) clarify commit message, use rounded division > > macro > > > > Signed-off-by: Joe Konno <joe.konno@intel.com> > > --- > > drivers/gpu/drm/i915/intel_panel.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > > index f17ada3..dcdfbb3 100644 > > --- a/drivers/gpu/drm/i915/intel_panel.c > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > @@ -421,7 +421,7 @@ static uint32_t scale(uint32_t source_val, > > /* avoid overflows */ > > target_val = (uint64_t)(source_val - source_min) * > > (target_max - target_min); > > - do_div(target_val, source_max - source_min); > > + target_val = DIV_ROUND_CLOSEST(target_val, source_max - source_min); > > This fails to build with CONFIG_X86_32=y: > > drivers/built-in.o: In function `scale': > intel_panel.c:(.text+0x12ab38): undefined reference to `__udivdi3' > make: *** [vmlinux] Error 1 > > Do you have a recommended workaround? Should we just use the v1 technique instead? U. Artie > BR, > Jani. > > > > target_val += target_min; > > > > return target_val; > > -- > > 2.1.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915: intel_backlight scale() math WA v2 2014-09-29 17:50 ` Eoff, Ullysses A @ 2014-09-29 18:46 ` Chris Wilson 2014-09-29 19:31 ` Damien Lespiau 1 sibling, 0 replies; 14+ messages in thread From: Chris Wilson @ 2014-09-29 18:46 UTC (permalink / raw) To: Eoff, Ullysses A; +Cc: intel-gfx@lists.freedesktop.org, Joe Konno On Mon, Sep 29, 2014 at 05:50:57PM +0000, Eoff, Ullysses A wrote: > > -----Original Message----- > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Jani Nikula > > Sent: Monday, September 29, 2014 6:07 AM > > To: Joe Konno; intel-gfx@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: intel_backlight scale() math WA v2 > > > > On Wed, 24 Sep 2014, Joe Konno <joe.konno@linux.intel.com> wrote: > > > From: Joe Konno <joe.konno@intel.com> > > > > > > Improper truncated integer division in the scale() function causes > > > actual_brightness != brightness. This (partial) work-around should be > > > sufficient for a majority of use-cases, but it is by no means a complete > > > solution. > > > > > > TODO: Determine how best to scale "user" values to "hw" values, and > > > vice-versa, when the ranges are of different sizes. That would be a > > > buggy scenario even with this work-around. > > > > > > The issue was introduced in the following (v3.17-rc1) commit: > > > > > > 6dda730 drm/i915: respect the VBT minimum backlight brightness > > > > > > v2: (thanks to Chris Wilson) clarify commit message, use rounded division > > > macro > > > > > > Signed-off-by: Joe Konno <joe.konno@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_panel.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > > > index f17ada3..dcdfbb3 100644 > > > --- a/drivers/gpu/drm/i915/intel_panel.c > > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > > @@ -421,7 +421,7 @@ static uint32_t scale(uint32_t source_val, > > > /* avoid overflows */ > > > target_val = (uint64_t)(source_val - source_min) * > > > (target_max - target_min); > > > - do_div(target_val, source_max - source_min); > > > + target_val = DIV_ROUND_CLOSEST(target_val, source_max - source_min); > > > > This fails to build with CONFIG_X86_32=y: > > > > drivers/built-in.o: In function `scale': > > intel_panel.c:(.text+0x12ab38): undefined reference to `__udivdi3' > > make: *** [vmlinux] Error 1 > > > > > > Do you have a recommended workaround? Should we just > use the v1 technique instead? Compromise and write DIV_ROUND_CLOSEST_ULL(). :| -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915: intel_backlight scale() math WA v2 2014-09-29 17:50 ` Eoff, Ullysses A 2014-09-29 18:46 ` Chris Wilson @ 2014-09-29 19:31 ` Damien Lespiau 2014-09-29 20:34 ` Eoff, Ullysses A 1 sibling, 1 reply; 14+ messages in thread From: Damien Lespiau @ 2014-09-29 19:31 UTC (permalink / raw) To: Eoff, Ullysses A; +Cc: intel-gfx@lists.freedesktop.org, Joe Konno On Mon, Sep 29, 2014 at 05:50:57PM +0000, Eoff, Ullysses A wrote: > > -----Original Message----- > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Jani Nikula > > Sent: Monday, September 29, 2014 6:07 AM > > To: Joe Konno; intel-gfx@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: intel_backlight scale() math WA v2 > > > > On Wed, 24 Sep 2014, Joe Konno <joe.konno@linux.intel.com> wrote: > > > From: Joe Konno <joe.konno@intel.com> > > > > > > Improper truncated integer division in the scale() function causes > > > actual_brightness != brightness. This (partial) work-around should be > > > sufficient for a majority of use-cases, but it is by no means a complete > > > solution. > > > > > > TODO: Determine how best to scale "user" values to "hw" values, and > > > vice-versa, when the ranges are of different sizes. That would be a > > > buggy scenario even with this work-around. > > > > > > The issue was introduced in the following (v3.17-rc1) commit: > > > > > > 6dda730 drm/i915: respect the VBT minimum backlight brightness > > > > > > v2: (thanks to Chris Wilson) clarify commit message, use rounded division > > > macro > > > > > > Signed-off-by: Joe Konno <joe.konno@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_panel.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > > > index f17ada3..dcdfbb3 100644 > > > --- a/drivers/gpu/drm/i915/intel_panel.c > > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > > @@ -421,7 +421,7 @@ static uint32_t scale(uint32_t source_val, > > > /* avoid overflows */ > > > target_val = (uint64_t)(source_val - source_min) * > > > (target_max - target_min); > > > - do_div(target_val, source_max - source_min); > > > + target_val = DIV_ROUND_CLOSEST(target_val, source_max - source_min); > > > > This fails to build with CONFIG_X86_32=y: > > > > drivers/built-in.o: In function `scale': > > intel_panel.c:(.text+0x12ab38): undefined reference to `__udivdi3' > > make: *** [vmlinux] Error 1 > > > > > > Do you have a recommended workaround? Should we just > use the v1 technique instead? The problem is target_val is 64 bits and we're trying to do a 64 bits division with the 32bits instruction set. That is usually handled by __udivdi3 in libgcc (in userspace). We already have a DIV_ROUND_CLOSEST_ULL() that uses do_div() in intel_display.c. -- Damien ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915: intel_backlight scale() math WA v2 2014-09-29 19:31 ` Damien Lespiau @ 2014-09-29 20:34 ` Eoff, Ullysses A 0 siblings, 0 replies; 14+ messages in thread From: Eoff, Ullysses A @ 2014-09-29 20:34 UTC (permalink / raw) To: Lespiau, Damien Cc: intel-gfx@lists.freedesktop.org, joe.konno@linux.intel.com On Mon, 2014-09-29 at 20:31 +0100, Damien Lespiau wrote: > On Mon, Sep 29, 2014 at 05:50:57PM +0000, Eoff, Ullysses A wrote: > > > -----Original Message----- > > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Jani Nikula > > > Sent: Monday, September 29, 2014 6:07 AM > > > To: Joe Konno; intel-gfx@lists.freedesktop.org > > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: intel_backlight scale() math WA v2 > > > > > > On Wed, 24 Sep 2014, Joe Konno <joe.konno@linux.intel.com> wrote: > > > > From: Joe Konno <joe.konno@intel.com> > > > > > > > > Improper truncated integer division in the scale() function causes > > > > actual_brightness != brightness. This (partial) work-around should be > > > > sufficient for a majority of use-cases, but it is by no means a complete > > > > solution. > > > > > > > > TODO: Determine how best to scale "user" values to "hw" values, and > > > > vice-versa, when the ranges are of different sizes. That would be a > > > > buggy scenario even with this work-around. > > > > > > > > The issue was introduced in the following (v3.17-rc1) commit: > > > > > > > > 6dda730 drm/i915: respect the VBT minimum backlight brightness > > > > > > > > v2: (thanks to Chris Wilson) clarify commit message, use rounded division > > > > macro > > > > > > > > Signed-off-by: Joe Konno <joe.konno@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_panel.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > > > > index f17ada3..dcdfbb3 100644 > > > > --- a/drivers/gpu/drm/i915/intel_panel.c > > > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > > > @@ -421,7 +421,7 @@ static uint32_t scale(uint32_t source_val, > > > > /* avoid overflows */ > > > > target_val = (uint64_t)(source_val - source_min) * > > > > (target_max - target_min); > > > > - do_div(target_val, source_max - source_min); > > > > + target_val = DIV_ROUND_CLOSEST(target_val, source_max - source_min); > > > > > > This fails to build with CONFIG_X86_32=y: > > > > > > drivers/built-in.o: In function `scale': > > > intel_panel.c:(.text+0x12ab38): undefined reference to `__udivdi3' > > > make: *** [vmlinux] Error 1 > > > > > > > > > > Do you have a recommended workaround? Should we just > > use the v1 technique instead? > > The problem is target_val is 64 bits and we're trying to do a 64 bits > division with the 32bits instruction set. That is usually handled by > __udivdi3 in libgcc (in userspace). We already have a > DIV_ROUND_CLOSEST_ULL() that uses do_div() in intel_display.c. > Ok, DIV_ROUND_CLOSEST_ULL() would be the one we want I take it. Would intel_drv.h be the appropriate header to move DIV_ROUND_CLOSEST_ULL() into so that we can use it in intel_panel.c, too? U. Artie ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-11-10 17:09 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-09-24 15:54 [PATCH] drm/i915: intel_backlight scale() math WA v2 Joe Konno 2014-09-24 16:06 ` Hans de Goede 2014-09-24 17:07 ` Jani Nikula 2014-09-24 17:41 ` Eoff, Ullysses A 2014-11-08 17:03 ` Eoff, Ullysses A 2014-11-10 11:16 ` Jani Nikula 2014-11-10 14:15 ` Jani Nikula 2014-11-10 17:08 ` Jesse Barnes 2014-09-26 17:15 ` Eoff, Ullysses A 2014-09-29 13:07 ` Jani Nikula 2014-09-29 17:50 ` Eoff, Ullysses A 2014-09-29 18:46 ` Chris Wilson 2014-09-29 19:31 ` Damien Lespiau 2014-09-29 20:34 ` Eoff, Ullysses A
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox