[Intel-gfx] [RFC PATCH 4/5] DRM/I915: Use the child device to decide whether the LVDS should be intialized

[Adam Jackson]

On Wed, 2009-11-18 at 10:39 +0800, ykzhao wrote:
> On Tue, 2009-11-17 at 23:47 +0800, Adam Jackson wrote:
> > On Tue, 2009-11-17 at 15:43 +0800, yakui.zhao at intel.com wrote:
> > > +int lvds_is_present_in_vbt(struct drm_device *dev)
> > > +{
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	struct child_device_config *p_child;
> > > +	int i, ret;
> > > +
> > > +	if (!dev_priv->child_dev_num)
> > > +		return 1;
> > > +
> > > +	ret = 0;
> > > +	for (i = 0; i < dev_priv->child_dev_num; i++) {
> > > +		p_child = dev_priv->child_dev + i;
> > > +		/*
> > > +		 * If the device type is not LFP, continue.
> > > +		 */
> > > +		if (p_child->device_type != INT_LFP_CLASS)
> > > +			continue;
> > 
> > Not that I have ROMs to prove this, but I see a whole bunch of other LFP
> > DEVICE_TYPE defines in intel_bios.h.  I suspect we should treat them the
> > same.
> Can you send me the vbios.dump on your box so that we can check whether
> it can work?

I wasn't being snarky.  I really don't have ROMs like this.  All the
ROMs I have, have 0x1022 for the device type for LFP.

However, the source documents more LFP device types.  I have to assume
those numbers came from some Intel-internal source and weren't just
invented out of thin air.  Therefore they should probably be treated the
same as 0x1022.

> > > @@ -936,6 +974,10 @@ void intel_lvds_init(struct drm_device *dev)
> > >  	if (dmi_check_system(intel_no_lvds))
> > >  		return;
> > >  
> > > +	if (!lvds_is_present_in_vbt(dev)) {
> > > +		DRM_DEBUG_KMS("LVDS is not present in VBT\n");
> > > +		return;
> > > +	}
> > >  	/* Assume that any device without an ACPI LID device also doesn't
> > >  	 * have an integrated LVDS.  We would be better off parsing the BIOS
> > >  	 * to get a reliable indicator, but that code isn't written yet.
> > 
> > The juxtaposition between code and comment here is pretty funny.
> > Therefore I'm reasonably sure it's wrong.
>
> The above comment is not related with this patch. It is the comment
> about the patch that uses the LID device to check whether the LVDS is
> present.

It _is_ related.  The comment says "we should parse the BIOS for LVDS
presence instead of assuming it's correlated with lid presence".  You
wrote code to parse the BIOS for LVDS presence, but did _not_ remove the
code that conflates lid with LVDS.

Consider - just as a thought exercise - a tablet machine.  Something
like this, for example:

http://www.ngtmedical.com/products/hardware/motion/

No, I do not have one.  Just think about what its firmware is going to
look like.  It's going to say "I have an LVDS panel attached", because
it does.  It's also not going to have a lid device in ACPI, because
there's no lid.

- ajax
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20091118/1bcaac29/attachment.sig>