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 13:10:01 +0300	[thread overview]
Message-ID: <87twqxjoqu.fsf@intel.com> (raw)
In-Reply-To: <20150914093950.GZ3383@phenom.ffwll.local>

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.

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

BR,
Jani.

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