[PATCH] drm/i915: expose a fixed brightness range to userspace

[PATCH] drm/i915: expose a fixed brightness range to userspace

[U. Artie Eoff]

A userspace brightness range that is larger than the hardware
brightness range does not have a 1:1 mapping and can result
in brightness != actual_brightness for some values.

Expose a fixed brightness range of [0..1000] to userspace so
that all values can map 1:1 into the hardware brightness
range.  This would assume that the hardware range is always
greater than 1000, otherwise we're right back to the original
issue.

This patch is based on Jani Nikula's proposed change in the
referenced ML thread, except use the range [0..1000] instead;
which was recommended by Jesse Barnes for smoother backlight
transitions.

Reference: http://lists.freedesktop.org/archives/intel-gfx/2014-November/055221.html
Signed-off-by: U. Artie Eoff <ullysses.a.eoff@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 4d63839..f74f5f2 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1057,7 +1057,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 = 1000;
 	props.brightness = scale_hw_to_user(connector,
 					    panel->backlight.level,
 					    props.max_brightness);
-- 
1.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: expose a fixed brightness range to

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform: baseline_drm_intel_nightly_pass_rate->patch_applied_pass_rate
BYT: pass/total=247/348->276/348
PNV: pass/total=326/328->324/328
ILK: pass/total=329/330->327/330
IVB: pass/total=544/546->544/546
SNB: pass/total=380/380->379/380
HSW: pass/total=586/591->589/591
BDW: pass/total=435/435->433/435
-------------------------------------Detailed-------------------------------------
test_platform: test_suite, test_case, result_with_drm_intel_nightly(count, machine_id...)...->result_with_patch_applied(count, machine_id)...
BYT: Intel_gpu_tools, igt_drv_hangman_error-state-basic, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_drv_hangman_error-state-capture-blt, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_drv_hangman_error-state-capture-bsd, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_drv_hangman_error-state-capture-render, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_drv_hangman_error-state-debugfs-entry, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_drv_hangman_error-state-sysfs-entry, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_bad_reloc_negative-reloc-lut, NSPT(1, M38)PASS(15, M31M29M38) -> NSPT(2, M38)PASS(2, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_ban-ctx-render, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_ban-render, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_close-pending-blt, BLACKLIST(1, M31)DMESG_WARN(11, M31M36M29)PASS(13, M31M36M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_close-pending-bsd, BLACKLIST(1, M31)DMESG_WARN(9, M36M29M31)PASS(15, M31M29M38M36) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_close-pending-ctx-render, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_close-pending-fork-blt, BLACKLIST(1, M31)DMESG_WARN(11, M31M36M29)PASS(13, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_close-pending-fork-bsd, BLACKLIST(1, M31)DMESG_WARN(12, M31M36M29)PASS(12, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_close-pending-fork-render, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_close-pending-fork-reverse-blt, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_close-pending-fork-reverse-bsd, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_close-pending-fork-reverse-render, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_close-pending-render, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_params, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_params-ctx-render, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_reset-count-blt, BLACKLIST(1, M31)DMESG_WARN(16, M31M36M29)PASS(8, M29M38M36) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_reset-count-bsd, BLACKLIST(1, M31)DMESG_WARN(14, M31M36M29)PASS(10, M36M38M29) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_reset-count-ctx-render, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_reset-count-render, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_reset-stats-blt, BLACKLIST(1, M31)DMESG_WARN(8, M36M29M31)PASS(16, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_reset-stats-bsd, BLACKLIST(1, M31)DMESG_WARN(9, M36M29M31)PASS(15, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_reset-stats-ctx-render, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_reset-stats-render, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_gem_reset_stats_unrelated-ctx-render, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
BYT: Intel_gpu_tools, igt_drv_hangman_ring-stop-sysfs-entry, BLACKLIST(1, M31)PASS(24, M31M36M29M38) -> PASS(1, M38)
PNV: Intel_gpu_tools, igt_gem_concurrent_blit_gpu-bcs-early-read, PASS(7, M25M23) -> DMESG_WARN(1, M25)PASS(3, M25)
PNV: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, DMESG_WARN(1, M25)TIMEOUT(15, M7M25M24M23)PASS(3, M24) -> DMESG_WARN(3, M25)TIMEOUT(1, M25)
PNV: Intel_gpu_tools, igt_gem_concurrent_blit_gttX-bcs-gpu-read-after-write-interruptible, PASS(7, M25) -> DMESG_WARN(1, M25)PASS(3, M25)
ILK: Intel_gpu_tools, igt_kms_3d, PASS(4, M26) -> DMESG_WARN(1, M26)PASS(3, M26)
ILK: Intel_gpu_tools, igt_kms_flip_flip-vs-expired-vblank, DMESG_WARN(1, M26)PASS(3, M26) -> DMESG_WARN(2, M26)PASS(2, M26)
ILK: Intel_gpu_tools, igt_kms_flip_flip-vs-modeset-interruptible, DMESG_WARN(2, M26)PASS(26, M37M26M6) -> DMESG_WARN(1, M26)PASS(3, M26)
ILK: Intel_gpu_tools, igt_kms_flip_flip-vs-rmfb, PASS(7, M26) -> DMESG_WARN(1, M26)PASS(3, M26)
IVB: Intel_gpu_tools, igt_kms_plane_plane-position-covered-pipe-A-plane-1, TIMEOUT(1, M34)PASS(27, M21M4M34) -> PASS(4, M21)
IVB: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, TIMEOUT(15, M34M21M4)PASS(1, M34) -> TIMEOUT(1, M21)PASS(3, M21)
SNB: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, TIMEOUT(9, M35M22)PASS(1, M35) -> TIMEOUT(1, M35)PASS(3, M35)
HSW: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, PASS(1, M40) -> TIMEOUT(1, M40)PASS(3, M40)
HSW: Intel_gpu_tools, igt_pm_rpm_legacy-planes, TIMEOUT(1, M40) -> TIMEOUT(3, M40)PASS(1, M40)
HSW: Intel_gpu_tools, igt_pm_rpm_legacy-planes-dpms, TIMEOUT(1, M40) -> TIMEOUT(3, M40)PASS(1, M40)
HSW: Intel_gpu_tools, igt_pm_rpm_universal-planes, TIMEOUT(1, M40) -> TIMEOUT(2, M40)PASS(1, M40)
HSW: Intel_gpu_tools, igt_pm_rpm_universal-planes-dpms, TIMEOUT(1, M40) -> PASS(1, M40)
BDW: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, TIMEOUT(15, M28M42M30)PASS(1, M42) -> TIMEOUT(1, M42)PASS(3, M42)
BDW: Intel_gpu_tools, igt_gem_reset_stats_ban-bsd, PASS(31, M42M30M28) -> DMESG_WARN(1, M42)PASS(3, M42)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: expose a fixed brightness range to userspace

[Eoff, Ullysses A]

Jani/Jesse, 

Does this patch look reasonable to you for merging?

-- U. Artie

> -----Original Message-----
> From: Eoff, Ullysses A
> Sent: Tuesday, November 11, 2014 12:31 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Eoff, Ullysses A
> Subject: [PATCH] drm/i915: expose a fixed brightness range to userspace
> 
> A userspace brightness range that is larger than the hardware
> brightness range does not have a 1:1 mapping and can result
> in brightness != actual_brightness for some values.
> 
> Expose a fixed brightness range of [0..1000] to userspace so
> that all values can map 1:1 into the hardware brightness
> range.  This would assume that the hardware range is always
> greater than 1000, otherwise we're right back to the original
> issue.
> 
> This patch is based on Jani Nikula's proposed change in the
> referenced ML thread, except use the range [0..1000] instead;
> which was recommended by Jesse Barnes for smoother backlight
> transitions.
> 
> Reference: http://lists.freedesktop.org/archives/intel-gfx/2014-November/055221.html
> Signed-off-by: U. Artie Eoff <ullysses.a.eoff@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 4d63839..f74f5f2 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1057,7 +1057,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 = 1000;
>  	props.brightness = scale_hw_to_user(connector,
>  					    panel->backlight.level,
>  					    props.max_brightness);
> --
> 1.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: expose a fixed brightness range to userspace

[Jesse Barnes]

On Tue, 11 Nov 2014 12:30:38 -0800
"U. Artie Eoff" <ullysses.a.eoff@intel.com> wrote:

> A userspace brightness range that is larger than the hardware
> brightness range does not have a 1:1 mapping and can result
> in brightness != actual_brightness for some values.
> 
> Expose a fixed brightness range of [0..1000] to userspace so
> that all values can map 1:1 into the hardware brightness
> range.  This would assume that the hardware range is always
> greater than 1000, otherwise we're right back to the original
> issue.
> 
> This patch is based on Jani Nikula's proposed change in the
> referenced ML thread, except use the range [0..1000] instead;
> which was recommended by Jesse Barnes for smoother backlight
> transitions.
> 
> Reference: http://lists.freedesktop.org/archives/intel-gfx/2014-November/055221.html
> Signed-off-by: U. Artie Eoff <ullysses.a.eoff@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 4d63839..f74f5f2 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1057,7 +1057,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 = 1000;
>  	props.brightness = scale_hw_to_user(connector,
>  					    panel->backlight.level,
>  					    props.max_brightness);

Yeah looks ok to me.  I guess Jani has to ack it too.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: expose a fixed brightness range to userspace

[Eoff, Ullysses A]

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.

----
U. Artie 

> -----Original Message-----
> From: Jesse Barnes [mailto:jbarnes@virtuousgeek.org]
> Sent: Tuesday, November 18, 2014 9:23 AM
> To: Eoff, Ullysses A
> Cc: intel-gfx@lists.freedesktop.org; Jani Nikula
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: expose a fixed brightness range to userspace
> 
> On Tue, 11 Nov 2014 12:30:38 -0800
> "U. Artie Eoff" <ullysses.a.eoff@intel.com> wrote:
> 
> > A userspace brightness range that is larger than the hardware
> > brightness range does not have a 1:1 mapping and can result
> > in brightness != actual_brightness for some values.
> >
> > Expose a fixed brightness range of [0..1000] to userspace so
> > that all values can map 1:1 into the hardware brightness
> > range.  This would assume that the hardware range is always
> > greater than 1000, otherwise we're right back to the original
> > issue.
> >
> > This patch is based on Jani Nikula's proposed change in the
> > referenced ML thread, except use the range [0..1000] instead;
> > which was recommended by Jesse Barnes for smoother backlight
> > transitions.
> >
> > Reference: http://lists.freedesktop.org/archives/intel-gfx/2014-November/055221.html
> > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@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 4d63839..f74f5f2 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -1057,7 +1057,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 = 1000;
> >  	props.brightness = scale_hw_to_user(connector,
> >  					    panel->backlight.level,
> >  					    props.max_brightness);
> 
> Yeah looks ok to me.  I guess Jani has to ack it too.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> --
> Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: expose a fixed brightness range to userspace

[Jesse Barnes]

We can still map 1000 entries back to 256, it will just mean some of
them won't result in any brightness change.

If that's not acceptable, I guess a dynamic 1:1 mapping is sufficient.

Jesse

On Tue, 18 Nov 2014 23:18:35 +0000
"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.
> 
> ----
> U. Artie 
> 
> > -----Original Message-----
> > From: Jesse Barnes [mailto:jbarnes@virtuousgeek.org]
> > Sent: Tuesday, November 18, 2014 9:23 AM
> > To: Eoff, Ullysses A
> > Cc: intel-gfx@lists.freedesktop.org; Jani Nikula
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: expose a fixed brightness range to userspace
> > 
> > On Tue, 11 Nov 2014 12:30:38 -0800
> > "U. Artie Eoff" <ullysses.a.eoff@intel.com> wrote:
> > 
> > > A userspace brightness range that is larger than the hardware
> > > brightness range does not have a 1:1 mapping and can result
> > > in brightness != actual_brightness for some values.
> > >
> > > Expose a fixed brightness range of [0..1000] to userspace so
> > > that all values can map 1:1 into the hardware brightness
> > > range.  This would assume that the hardware range is always
> > > greater than 1000, otherwise we're right back to the original
> > > issue.
> > >
> > > This patch is based on Jani Nikula's proposed change in the
> > > referenced ML thread, except use the range [0..1000] instead;
> > > which was recommended by Jesse Barnes for smoother backlight
> > > transitions.
> > >
> > > Reference: http://lists.freedesktop.org/archives/intel-gfx/2014-November/055221.html
> > > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@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 4d63839..f74f5f2 100644
> > > --- a/drivers/gpu/drm/i915/intel_panel.c
> > > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > > @@ -1057,7 +1057,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 = 1000;
> > >  	props.brightness = scale_hw_to_user(connector,
> > >  					    panel->backlight.level,
> > >  					    props.max_brightness);
> > 
> > Yeah looks ok to me.  I guess Jani has to ack it too.
> > 
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > 
> > --
> > Jesse Barnes, Intel Open Source Technology Center
> 


-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: expose a fixed brightness range to userspace

[Eoff, Ullysses A]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Eoff, Ullysses A
> Sent: Tuesday, November 18, 2014 3:19 PM
> To: Jani Nikula; Jesse Barnes
> Cc: stephane.marchesin@gmail.com; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: expose a fixed brightness range to userspace
> 
> 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.
> 
> ----
> U. Artie

In all fairness, I guess Jesse did elude to the fact that there
would be such devices in the previous thread:

<http://lists.freedesktop.org/archives/intel-gfx/2014-November/055221.html>

I don't see why we shouldn't just expose the full 1:1 range to
userspace.   And let userspace decide what level stepping
makes sense for providing smooth backlight level transitions
to the end-user if necessary.

--U. Artie

> 
> > -----Original Message-----
> > From: Jesse Barnes [mailto:jbarnes@virtuousgeek.org]
> > Sent: Tuesday, November 18, 2014 9:23 AM
> > To: Eoff, Ullysses A
> > Cc: intel-gfx@lists.freedesktop.org; Jani Nikula
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: expose a fixed brightness range to userspace
> >
> > On Tue, 11 Nov 2014 12:30:38 -0800
> > "U. Artie Eoff" <ullysses.a.eoff@intel.com> wrote:
> >
> > > A userspace brightness range that is larger than the hardware
> > > brightness range does not have a 1:1 mapping and can result
> > > in brightness != actual_brightness for some values.
> > >
> > > Expose a fixed brightness range of [0..1000] to userspace so
> > > that all values can map 1:1 into the hardware brightness
> > > range.  This would assume that the hardware range is always
> > > greater than 1000, otherwise we're right back to the original
> > > issue.
> > >
> > > This patch is based on Jani Nikula's proposed change in the
> > > referenced ML thread, except use the range [0..1000] instead;
> > > which was recommended by Jesse Barnes for smoother backlight
> > > transitions.
> > >
> > > Reference: http://lists.freedesktop.org/archives/intel-gfx/2014-November/055221.html
> > > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@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 4d63839..f74f5f2 100644
> > > --- a/drivers/gpu/drm/i915/intel_panel.c
> > > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > > @@ -1057,7 +1057,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 = 1000;
> > >  	props.brightness = scale_hw_to_user(connector,
> > >  					    panel->backlight.level,
> > >  					    props.max_brightness);
> >
> > Yeah looks ok to me.  I guess Jani has to ack it too.
> >
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> >
> > --
> > Jesse Barnes, 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

Re: [PATCH] drm/i915: expose a fixed brightness range to userspace

[Stéphane Marchesin]

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...

Stéphane


> ----
> U. Artie
>
>> -----Original Message-----
>> From: Jesse Barnes [mailto:jbarnes@virtuousgeek.org]
>> Sent: Tuesday, November 18, 2014 9:23 AM
>> To: Eoff, Ullysses A
>> Cc: intel-gfx@lists.freedesktop.org; Jani Nikula
>> Subject: Re: [Intel-gfx] [PATCH] drm/i915: expose a fixed brightness range to userspace
>>
>> On Tue, 11 Nov 2014 12:30:38 -0800
>> "U. Artie Eoff" <ullysses.a.eoff@intel.com> wrote:
>>
>> > A userspace brightness range that is larger than the hardware
>> > brightness range does not have a 1:1 mapping and can result
>> > in brightness != actual_brightness for some values.
>> >
>> > Expose a fixed brightness range of [0..1000] to userspace so
>> > that all values can map 1:1 into the hardware brightness
>> > range.  This would assume that the hardware range is always
>> > greater than 1000, otherwise we're right back to the original
>> > issue.
>> >
>> > This patch is based on Jani Nikula's proposed change in the
>> > referenced ML thread, except use the range [0..1000] instead;
>> > which was recommended by Jesse Barnes for smoother backlight
>> > transitions.
>> >
>> > Reference: http://lists.freedesktop.org/archives/intel-gfx/2014-November/055221.html
>> > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@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 4d63839..f74f5f2 100644
>> > --- a/drivers/gpu/drm/i915/intel_panel.c
>> > +++ b/drivers/gpu/drm/i915/intel_panel.c
>> > @@ -1057,7 +1057,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 = 1000;
>> >     props.brightness = scale_hw_to_user(connector,
>> >                                         panel->backlight.level,
>> >                                         props.max_brightness);
>>
>> Yeah looks ok to me.  I guess Jani has to ack it too.
>>
>> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>>
>> --
>> Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: expose a fixed brightness range to userspace

[Eoff, Ullysses A]

> -----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)].

--U. Artie

> Stéphane
> 
> 
> > ----
> > U. Artie
> >
> >> -----Original Message-----
> >> From: Jesse Barnes [mailto:jbarnes@virtuousgeek.org]
> >> Sent: Tuesday, November 18, 2014 9:23 AM
> >> To: Eoff, Ullysses A
> >> Cc: intel-gfx@lists.freedesktop.org; Jani Nikula
> >> Subject: Re: [Intel-gfx] [PATCH] drm/i915: expose a fixed brightness range to userspace
> >>
> >> On Tue, 11 Nov 2014 12:30:38 -0800
> >> "U. Artie Eoff" <ullysses.a.eoff@intel.com> wrote:
> >>
> >> > A userspace brightness range that is larger than the hardware
> >> > brightness range does not have a 1:1 mapping and can result
> >> > in brightness != actual_brightness for some values.
> >> >
> >> > Expose a fixed brightness range of [0..1000] to userspace so
> >> > that all values can map 1:1 into the hardware brightness
> >> > range.  This would assume that the hardware range is always
> >> > greater than 1000, otherwise we're right back to the original
> >> > issue.
> >> >
> >> > This patch is based on Jani Nikula's proposed change in the
> >> > referenced ML thread, except use the range [0..1000] instead;
> >> > which was recommended by Jesse Barnes for smoother backlight
> >> > transitions.
> >> >
> >> > Reference: http://lists.freedesktop.org/archives/intel-gfx/2014-November/055221.html
> >> > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@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 4d63839..f74f5f2 100644
> >> > --- a/drivers/gpu/drm/i915/intel_panel.c
> >> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> >> > @@ -1057,7 +1057,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 = 1000;
> >> >     props.brightness = scale_hw_to_user(connector,
> >> >                                         panel->backlight.level,
> >> >                                         props.max_brightness);
> >>
> >> Yeah looks ok to me.  I guess Jani has to ack it too.
> >>
> >> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> >>
> >> --
> >> Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: expose a fixed brightness range to userspace

[Jani Nikula]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: expose a fixed brightness range to userspace

[Eoff, Ullysses A]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: expose a fixed brightness range to userspace

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: expose a fixed brightness range to userspace

[Eoff, Ullysses A]

> -----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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx