[Intel-gfx] [DRM/I915]: Check the LID device to decide whether the LVDS should be initialized

[yakui_zhao]

On Fri, 2009-06-26 at 00:37 +0800, Jesse Barnes wrote:
> On Thu, 25 Jun 2009 13:41:09 +0800
> yakui_zhao <yakui.zhao at intel.com> wrote:
> 
> > Hi,
> >    This patch is to decide whether the integrated lVDS should be
> > initialized by checking whether there exists the LID device.
> >    At the same time I am working on the another feature proposed by
> > Jesse that use the lid state to check whether the LVDS is
> > connected/disconnected. That means that the LVDS should be
> > disconnected when LID is closed. When the LID is reopened, the LVDS
> > is connected. (But there exists the module dependency. At the same
> > time the LID event will be handled by the acpid. Maybe the system
> > will enter the suspend state when the LID is closed. So this feature
> > will be deferred for some time.)
> 
> Minor nit on the changelog & summary:  summary should read
>   [PATCH] drm/i915: Check the LID device to decide whether the LVDS should be initialized
> just to be consistent with how we've been doing things.
> 
> You should also separate out the actual changelog stuff (what's below)
> from comments about the work (above).  You can use -- to separate
> portions of the mail.
Ok. Agree with what you have said.
> 
> > On some boxes the mobile chipset is used and there is no LVDS device.
> > In such case we had better not initialize the LVDS output device so
> > that one pipe can be used for other output device. For example: E-TOP.
> > 
> > But unfortunately the LVDS device is still initialized on the boxes
> > based on mobile chipset in KMS mode. It brings that this pipe
> > occupied by LVDS can't be used for other output device.
> > 
> > After checking the acpidump we find that there is no LID device on
> > such boxes. In such case we can use the LID device to decide whether
> > the LVDS device should be initialized. 
> > 
> > If there is no LID device, we can think that there is no LVDS device.
> > It is unnecessary to initialize the LVDS output device.
> > If there exists the LID device, it will continue the current
> > flowchart.
> > 
> > Maybe on some boxes there is no LVDS device but the LID device is
> > found. In such case it had better be added to the quirk table. 
> 
> I agree, using the lid as an LVDS detection heuristic is an improvement
> over what we have, and in cases where an ACPI LID device is present but
> there's still no LVDS attached a quirk is appropriate (probably a much
> smaller list than we have at present too).
If one box with the LID present has no LVDS device, it should be added
to the quirk list.
But unfortunately now I don't find such case. So there is no quirk list.

If the user has such a box, we add it to the quirk list.
> 
> Of course, ideally we'd figure out how to parse the various VBIOS
> versions for even better accuracy.
> 
> > +/**
> > + * check whether there exists the ACPI LID device by enumerating the
> > ACPI
> > + * device tree.
> > + * If ACPI is disabled, there is no ACPI device tree. 0 is returned.
> > + * If the LID device is found, it will return zero.
> > + * If no LID device is found, it will return  -ENODEV.
> > + */
> > +static int intel_lvds_find_lid(void)
> > +{
> > +	int lid_count = 0;
> > +
> > +	if (acpi_disabled) {
> > +		/*
> > +		 * if ACPI is disabled, there is no ACPI device
> > tree. And
> > +		 * we don't know whether there exists the LID device.
> > +		 * In such case we will return 0.
> > +		 */
> > +		return 0;
> > +	}
> > +
> > +	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> > +				ACPI_UINT32_MAX,
> > +				check_lid_device, &lid_count, NULL);
> > +
> > +	if (!lid_count) {
> > +		/* No LID device is not found. Return -ENODEV */
> > +		return -ENODEV;
> > +	}
> >  
> > +	return 0;
> > +}
> > +#else
> > +static inline int intel_lvds_find_lid(void) { return 0; }
> > +#endif
> 
> More nit picking:  intel_lid_present returning a bool would probably be
> easier to read...  Also, there's no existing #define for the ACPI LID
> device PNP identifier?  Seems like something that should go into an
> ACPI header somewhere rather than the lid walking function...
The PNP identifier for LID device is not defined in the public ACP
header file. Instead it is defined in the ACPI button driver. And I have
to add the Pnp identifier again in our driver.

Of course I agree with your advice that changes the function
name/prototype.

thanks for your advice.
> 
> >  /**
> >   * intel_lvds_init - setup LVDS connectors on this device
> >   * @dev: drm device
> > @@ -811,6 +879,17 @@
> >  	if (dmi_check_system(intel_no_lvds))
> >  		return;
> >  
> > +	if (intel_lvds_find_lid()) {
> > +		/* If there is no LID device, we can think that
> > there is
> > +		 * no LVDS output device. In such case it is
> > unnecessary to
> > +		 * create the LVDS output device.
> > +		 * But maybe on some boxes there is no LVDS device
> > while the
> > +		 * LID device is found. If so, it had better be
> > added to
> > +		 * the quirk list.
> > +		 */
> > +		return;
> > +	}
> > +
> 
> Then this would just be:
>   if (!intel_lid_present())
>     return;
> (along with your comment about the heuristic nature of the call of
> course).
> 
> Assuming you fix that stuff up, you can add
> Reviewed-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> to the next patch you send out.
> 
> Thanks,