Re: [PATCH] ACPI/Intel: Rework Opregion support

[Chris Wilson]

On Tue, 15 Mar 2011 02:18:02 +0100 (CET), "Indan Zupancic" <indan@xxxxxx> wrote:
> Hello,
> 
> Some nitpicks below.
> 
> On Mon, March 14, 2011 18:59, Chris Wilson wrote:
> > Note: neither the opregion_dev interface or the alse_set_* properly report
> > failures. As such we have a slight change in behaviour on Ironlake+
> > platforms and an uncorrected bug for earlier chipsets.
> > -Chris
> 
> What uncorrected bug?

For later chipsets we currently report the failure to respond to the ALSE
requests, for earlier we do not. The patch harmonises the two code paths
reusing the earlier code for later chipsets, hence the change in behaviour
and potential regression. Alternatively, actually reporting the failure
for earlier chipsets may also break existing setups.

> And are there earlier chipsets with ASLE support at all, besides gen 4?
> If there are no gen 2 or gen 3 chipsets with ASLE then the backlight
> code can be simplified further.

The OpRegion interface was devised midway through gen3 (afaik), and you
find it on some i915-class hw and not others. In theory, there is nothing
to prevent a BIOS/ACPI from being rewritten for it to be of use in gen2,
and who knows one such beast may already exist (considering much to our
horror you can still buy gen2 chipsets).

> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index 4e5ff59..51565bb 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -261,8 +261,8 @@ intel_panel_detect(struct drm_device *dev)
> >  	 * appears to be either the BIOS or Linux ACPI fault */
> >  #if 0
> >  	/* Assume that the BIOS does not lie through the OpRegion... */
> > -	if (dev_priv->opregion.lid_state)
> > -		return ioread32(dev_priv->opregion.lid_state) & 0x1 ?
> > +	if (dev_priv->opregion_dev.opregion.acpi)
> > +		return ioread32(&dev_priv->opregion_dev.opregion.acpi->clid) & 0x1 ?
> 
> What guarantees that opregion.acpi != NULL here?

You mean other than the explicit test for opregion.acpi != NULL?

> Or perhaps just remove that #if 0 code chunk altogether?

Read the changelog and thread on the patch that disabled this logic, the
failure (or at least inconsistent behaviour with the expectations of the
HP BIOS authors) appears to be in how we initialise ACPI on the HP
machines that causes the initial value of lid state to be incorrect. Since
one of the laptops that Dave tests drm-next on is a HP, he was bitten by
the bug and temporarily (we hope) disabled the logic. Or else once again,
we will continue to light up the panel on a closed laptop.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/