From patchwork Wed Nov 19 08:56:56 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jani Nikula X-Patchwork-Id: 5335731 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 4BED7C11AC for ; Wed, 19 Nov 2014 10:13:56 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 5AE23200C6 for ; Wed, 19 Nov 2014 10:13:55 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 5E8B22008F for ; Wed, 19 Nov 2014 10:13:54 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 8CE216EC03; Wed, 19 Nov 2014 02:13:53 -0800 (PST) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTP id 60CF56E4ED for ; Wed, 19 Nov 2014 02:13:52 -0800 (PST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP; 19 Nov 2014 00:50:27 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.07,415,1413270000"; d="scan'208";a="624746219" Received: from jnikula-mobl.fi.intel.com (HELO localhost) ([10.237.72.54]) by fmsmga001.fm.intel.com with ESMTP; 19 Nov 2014 00:56:52 -0800 From: Jani Nikula To: "Eoff\, Ullysses A" , =?utf-8?Q?St=C3=A9pha?= =?utf-8?Q?ne?= Marchesin In-Reply-To: <8B5BDC21C48FAF4CA5D7E3FE1A63A908709057DF@ORSMSX111.amr.corp.intel.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <1415737838-9640-1-git-send-email-ullysses.a.eoff@intel.com> <20141118092239.53d5a429@jbarnes-hsw> <8B5BDC21C48FAF4CA5D7E3FE1A63A90870904E10@ORSMSX111.amr.corp.intel.com> <8B5BDC21C48FAF4CA5D7E3FE1A63A908709057DF@ORSMSX111.amr.corp.intel.com> User-Agent: Notmuch/0.19~rc1+1~g08b4944 (http://notmuchmail.org) Emacs/24.4.1 (x86_64-pc-linux-gnu) Date: Wed, 19 Nov 2014 10:56:56 +0200 Message-ID: <87389ftvon.fsf@intel.com> MIME-Version: 1.0 Cc: "intel-gfx@lists.freedesktop.org" Subject: Re: [Intel-gfx] [PATCH] drm/i915: expose a fixed brightness range to userspace X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, 19 Nov 2014, "Eoff, Ullysses A" 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 >> 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. BR, Jani. [1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/stable/sysfs-class-backlight 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);