Implement direct pineview backlight control.

Implement direct pineview backlight control.

This adds a Kconfig variable CONFIG_DRM_I915_DIRECT_BACKLIGHT to
optionally include the new source file i915_backlight.c.  This file
registers a couple backlight set/get routines with the standard
drivers/video/backlight interface to add a directory under
/sys/class/backlight/.

The big problem I see with this modification is that the i915 driver
_already_ supports backlight level adjustment in i915_opregion.c.
Unfortunately, that requires an IRQ to occur as the result of writing
some ASLE (ACPI Source Language Event) register, and this requires
BIOS support for the ACPI hook.  It looks like we will not have such
support in our initial custom BIOS.

So I copy a small amount of code from i915_opregion.c to make backlight
adjustment directly available to the backlight framework.

Another problem is that I do not cover all the cases handled by the
i915_opregion.c code.  It would be too much untested code copying to
do so.  I enforce the lack of support by checking for IS_PINEVIEW()
in i915_backlight_init().

[jrbarnette, 2010-06-21 15:51:38]

http://codereview.chromium.org/2830015/diff/1/4
File drivers/gpu/drm/Kconfig (right):

http://codereview.chromium.org/2830015/diff/1/4#newcode127
drivers/gpu/drm/Kconfig:127: config DRM_I915_DIRECT_BACKLIGHT
I have questions about why we need a config setting here, and whether the
default settings are the right ones.  Could you explain your rationale?  Or
perhaps we can speak FTF?

http://codereview.chromium.org/2830015/diff/1/6
File drivers/gpu/drm/i915/i915_backlight.c (right):

http://codereview.chromium.org/2830015/diff/1/6#newcode44
drivers/gpu/drm/i915/i915_backlight.c:44: pwm_val = (blc_pwm_ctl &
BACKLIGHT_DUTY_CYCLE_MASK) >> 1;
Does the magic '1' shift constant have a name?

http://codereview.chromium.org/2830015/diff/1/6#newcode63
drivers/gpu/drm/i915/i915_backlight.c:63: (pwm_val << 1);
I assume the magic '1' shift constant is the same '1' in get_intensity()...  All
the more reason for it to have a name.

http://codereview.chromium.org/2830015/diff/1/6#newcode93
drivers/gpu/drm/i915/i915_backlight.c:93: backlight_update_status(bd);
If I've read i915_get_intensity() right, it performs its calculations assuming
the modulation frequency field in the PWM_CTL register is set to 0x1000.
What guarantee do we have that the field is set correctly at this point?
Even assuming that 0x1000 is the hardware default, I'd feel better with
explicit initialization here.

http://codereview.chromium.org/2830015/diff/1/7
File drivers/gpu/drm/i915/i915_dma.c (right):

http://codereview.chromium.org/2830015/diff/1/7#newcode1508
drivers/gpu/drm/i915/i915_dma.c:1508: i915_backlight_init(dev);
We're ignoring the return value from this function.  I don't necessarily mind,
but I'd prefer to see either a cast to void, or just change the function to
return
void.

[jrbarnette, 2010-06-21 20:21:42]

On Jun 21, 2010, at 8:51 AM, jrbarnette@chromium.org wrote:
> http://codereview.chromium.org/2830015/diff/1/4#newcode127
> drivers/gpu/drm/Kconfig:127: config DRM_I915_DIRECT_BACKLIGHT
> I have questions about why we need a config setting here, and whether
> the default settings are the right ones.  Could you explain your
> rationale?  Or perhaps we can speak FTF?
>
Following up on our offline conversation, I'll note my two concerns
about adding a config setting here:
   * My first concern was since the stated objective of the change
     is to support Google devices that don't have the desired ACPI
     capability, we should probably change the various *.flavour
     config files so that by default, this option is only present on
     specifically those devices that need it.  I think this is worth
     some effort.
   * My other concern was that the change adds in new code that
     will execute, even on commodity hardware platforms.  It might
     make more sense to disable the ACPI support if this config option
     is present.  However, I'm not convinced that this is a good idea
     because a) you've indicated that the two pieces of code play nice
     together, and b) if by default we disable this feature on platforms
     with the ACPI support, the point seems moot.


> http://codereview.chromium.org/2830015/show

-- jrb

[bfreed, 2010-06-21 23:45:21]

Yes, the two i915 backlight controling methods (i915_opregion.c and
i915_backlight.c) do indeed play nicely together.  So I think we should _not_
turn off i915_opregions.c at this time.

I redid the config settings a bit so we only turn on i915_backlight.c in
intel-menlow (32bit x86 chromeos) and x86-pineview (64bit experimental x86).  We
turn it off for all other i386.

I initialize backlight to 0 rather than try to parse what firmware may or may
not have initialized.  It makes more sense to let X bring up the backlight when
it is ready.
On 2010/06/21 20:21:42, jrbarnette wrote:
> On Jun 21, 2010, at 8:51 AM, mailto:jrbarnette@chromium.org wrote:
> > http://codereview.chromium.org/2830015/diff/1/4#newcode127
> > drivers/gpu/drm/Kconfig:127: config DRM_I915_DIRECT_BACKLIGHT
> > I have questions about why we need a config setting here, and whether
> > the default settings are the right ones.  Could you explain your
> > rationale?  Or perhaps we can speak FTF?
> >
> Following up on our offline conversation, I'll note my two concerns
> about adding a config setting here:
>    * My first concern was since the stated objective of the change
>      is to support Google devices that don't have the desired ACPI
>      capability, we should probably change the various *.flavour
>      config files so that by default, this option is only present on
>      specifically those devices that need it.  I think this is worth
>      some effort.
>    * My other concern was that the change adds in new code that
>      will execute, even on commodity hardware platforms.  It might
>      make more sense to disable the ACPI support if this config option
>      is present.  However, I'm not convinced that this is a good idea
>      because a) you've indicated that the two pieces of code play nice
>      together, and b) if by default we disable this feature on platforms
>      with the ACPI support, the point seems moot.
> 
> 
> > http://codereview.chromium.org/2830015/show
> 
> -- jrb
> 
>

[jrbarnette, 2010-06-22 16:38:49]

On 2010/06/21 23:45:21, bfreed wrote:
> Yes, the two i915 backlight controling methods (i915_opregion.c and
> i915_backlight.c) do indeed play nicely together.  So I think we should _not_
> turn off i915_opregions.c at this time.
> 
> I redid the config settings a bit so we only turn on i915_backlight.c in
> intel-menlow (32bit x86 chromeos) and x86-pineview (64bit experimental x86). 
We
> turn it off for all other i386.
> 
> I initialize backlight to 0 rather than try to parse what firmware may or may
> not have initialized.  It makes more sense to let X bring up the backlight
when
> it is ready.

Right, then.  LGTM!


> On 2010/06/21 20:21:42, jrbarnette wrote:
> > On Jun 21, 2010, at 8:51 AM, mailto:jrbarnette@chromium.org wrote:
> > > http://codereview.chromium.org/2830015/diff/1/4#newcode127
> > > drivers/gpu/drm/Kconfig:127: config DRM_I915_DIRECT_BACKLIGHT
> > > I have questions about why we need a config setting here, and whether
> > > the default settings are the right ones.  Could you explain your
> > > rationale?  Or perhaps we can speak FTF?
> > >
> > Following up on our offline conversation, I'll note my two concerns
> > about adding a config setting here:
> >    * My first concern was since the stated objective of the change
> >      is to support Google devices that don't have the desired ACPI
> >      capability, we should probably change the various *.flavour
> >      config files so that by default, this option is only present on
> >      specifically those devices that need it.  I think this is worth
> >      some effort.
> >    * My other concern was that the change adds in new code that
> >      will execute, even on commodity hardware platforms.  It might
> >      make more sense to disable the ACPI support if this config option
> >      is present.  However, I'm not convinced that this is a good idea
> >      because a) you've indicated that the two pieces of code play nice
> >      together, and b) if by default we disable this feature on platforms
> >      with the ACPI support, the point seems moot.
> > 
> > 
> > > http://codereview.chromium.org/2830015/show
> > 
> > -- jrb
> > 
> >

[jrbarnette, 2010-06-22 21:33:56]

OK.  S(till)LGTM!