On Tue, March 15, 2011 09:37, Chris Wilson wrote: > 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. Ah, okay, for the ASLE_SET_ALS_ILLUM/ASLE_SET_PFIT/ASLE_SET_PWM_FREQ cases. Hopefully this change doesn't cause regressions, that would be sad. >> 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). Pity, if they're still sold any bets are off. >> > 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? I'm blind. I checked all the rest of the code, but not the line just above it. Gah! >> Or perhaps just remove that #if 0 code chunk altogether? > > Read the changelog and thread on the patch that disabled this logic, the I just subscribed to intel-gfx, seemed like a good idea after the reject. > 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. Hopefully it's fixed by the next BIOS upgrade by HP... Everything would be a lot simpler if the BIOSes were open source. It's shocking with what you guys have to deal with. Good luck and thanks for all the hard work! Indan _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
