[Intel-gfx] [RFC CABC v3 PATCH 2/2] drm/i915: CABC support for backlight control

[Daniel Vetter]

On Mon, Sep 14, 2015 at 01:10:01PM +0300, Jani Nikula wrote:
> On Mon, 14 Sep 2015, Daniel Vetter <daniel at 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 at 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).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch