Re: [RFC CABC v3 PATCH 2/2] drm/i915: CABC support for backlight control

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Deepak M <m.deepak@intel.com>, intel-gfx@lists.freedesktop.org
Subject: Re: [RFC CABC v3 PATCH 2/2] drm/i915: CABC support for backlight control
Date: Mon, 14 Sep 2015 16:59:01 +0300	[thread overview]
Message-ID: <87egi1je56.fsf@intel.com> (raw)
In-Reply-To: <20150914131953.GI3383@phenom.ffwll.local>

On Mon, 14 Sep 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Sep 14, 2015 at 01:10:01PM +0300, Jani Nikula wrote:
>> On Mon, 14 Sep 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Fri, Sep 11, 2015 at 02:28:02PM +0300, Jani Nikula wrote:
>> >> On Fri, 11 Sep 2015, Deepak M <m.deepak@intel.com> wrote:
>> >> > In CABC (Content Adaptive Brightness Control) content grey level
>> >> > scale can be increased while simultaneously decreasing
>> >> > brightness of the backlight to achieve same perceived brightness.
>> >> >
>> >> > The CABC is not standardized and panel vendors are free to follow
>> >> > their implementation. The CABC implementaion here assumes that the
>> >> > panels use standard SW register for control.
>> >> >
>> >> > In this design there will be no PWM signal from the SoC and DCS
>> >> > commands are sent to enable and control the backlight brightness.
>> >> >
>> >> > v2:
>> >> > - Created a new backlight driver for cabc, which will be registered
>> >> >   only when it cabc is supported by panel. (Daniel Vetter)
>> >> 
>> >> I don't know what Daniel's been telling you, but I think this does need
>> >> to get bolted into the regular backlight control mechanism. We'll also
>> >> need eDP specific backlight control, and there's the VLV/CVH pwm driver
>> >> absed backlight control, so we need to cover this more gracefully than
>> >> an if-else ladder anyway.
>> >> 
>> >> My idea all along has been that the backlight hooks in dev_priv.display
>> >> need to be moved to connectors (probably intel_panel), and connector
>> >> backlight setup can do what they want with these. All the boilerplate
>> >> code for sysfs and scaling and so on would be there already.
>> >> 
>> >> I do not approve of the approach here.
>> >
>> > The current design of backlights in linux is that userspace picks the
>> > right backlight device. We've enabled/disabled the backlight pwm
>> > ourselves too, but that was always just for the native backlight power
>> > thing, not any of the others.
>> 
>> What does that have to do with this series?
>> 
>> > Imo this is the right approach since it fits into the current design.
>> 
>> No, it does *not* fit into the current design, which you can see from
>> the if ladders there.
>
> Well those if ladders are partially breaking the current design, we've
> never done that for any of the other backlight drivers. For those
> userspace is expected to call them before a modeset.
>
> Adding if ladders to the kernel is probably not needed if userspace holds
> up it's part of the deal.
>
>> > Fixing the current design so that i915 can get at the right backlight
>> > driver should imo be a separate series. I.e. yes this is what I
>> > recommended roughly (but I didn't check details nor can I test with
>> > xf86-video-intel to make sure it actually works correctly).
>> 
>> It is important this gets done the right way up front, because there's
>> now two series in flight to rip up the backlight code that's now neat
>> and tidy.
>> 
>> I am *not* volunteering to clean up the backlight code again. I've
>> already done it once.
>
> This isn't about cleaning up the i915 backlight code, it's about pulling
> the backlight selection logic from libbacklight into the kernel. At least
> for the if ladders in the encoder enable/disable hooks. I assumed that
> neither for capc and for the edp dpcd backlight we'd need those (userspace
> should frob the backlight for us).

I can't be bothered to summarize the rather frustrated IRC discussion on
this, but the bottom line is that the kernel implementation should be
done using the backlight hooks approach I suggested in [1]. That does
not exclude the possibility of exposing more backlight interfaces if
needed, but it keeps the kernel implementation neat.

BR,
Jani.

[1] http://mid.gmane.org/cover.1442227790.git.jani.nikula@intel.com

> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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