[Intel-gfx] [RFC Patch 1/3] DRM/I915: parse child device from VBT

[ykzhao]

On Sat, 2009-08-08 at 01:02 +0800, Jesse Barnes wrote:
> On Fri,  7 Aug 2009 10:55:17 +0800
> yakui.zhao at intel.com wrote:
> 
> > From: Zhao Yakui <yakui.zhao at intel.com>
Thanks for the comments.
> > 
> > Parse the child device from VBT.
> 
> Can you make the changelog a bit more verbose?  What types of child
> devices are available?  On what ranges of chipsets?  Just a little bit
> of description for someone browsing the history...
I will add more explanation about it.

> >  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> > +		dev_priv->child_dev = NULL;
> > +		dev_priv->child_dev_num = 0;
> 
> Not needed, the dev_priv is allocated and zeroed already.
Yes. This is unnecessary. 
> 
>   
> > +static void
> > +parse_device_mapping_from_vbt(struct drm_i915_private *dev_priv,
> > +		       struct bdb_header *bdb)
> 
> The _vbt suffix is redundant since the whole file deals with VBT
> parsing.  So you can just call it "parse_child_devices" or something
> similar.
> 
> > +{
> > +	struct bdb_general_definitions *p_defs;
> > +	struct child_device_config *p_child, *child_dev_ptr;
> > +	int i, child_device_num, count;
> > +	u16	block_size, *block_ptr;
> > +
> > +	p_defs = find_section(bdb, BDB_GENERAL_DEFINITIONS);
> > +	if (!p_defs) {
> > +		DRM_DEBUG("No general definition block is found\n");
> > +		return;
> > +	}
> 
> Eric may have already mentioned this, but we don't do Hungarian
> notation in the kernel, so you can drop the "p_" prefixes to pointer
> types.  We should probably clean up parse_sdvo_Device_mapping at some
> point too (maybe this code was initially copy & pasted from that?).
> 
> > +	/* judge whether the size of child device meets the
> > requirements.
> > +	 * If the child device size obtained from general definition
> > block
> > +	 * is different with sizeof(struct child_device_config),
> > skip the
> > +	 * parsing of sdvo device info
> > +	 */
> > +	if (p_defs->child_dev_size != sizeof(*p_child)) {
> > +		/* different child dev size . Ignore it */
> > +		DRM_DEBUG("different child size is found.
> > Invalid.\n");
> > +		return;
> > +	}
> 
> The message should be "Unsupported child device size, ignoring.\n".
> 
> > +	/* get the block size of general definitions */
> > +	block_ptr = (u16 *)((char *)p_defs - 2);
> > +	block_size = *block_ptr;
> > +	/* get the number of child device */
> > +	child_device_num = (block_size - sizeof(*p_defs)) /
> > +				sizeof(*p_child);
> > +	count = 0;
> > +	/* get the number of child device that is present */
> > +	for (i = 0; i < child_device_num; i++) {
> > +		p_child = &(p_defs->devices[i]);
> > +		if (!p_child->device_type) {
> > +			/* skip the device block if device type is
> > invalid */
> > +			continue;
> > +		}
> > +		count++;
> > +	}
> > +	if (!count) {
> > +		DRM_DEBUG("no child dev is parsed from VBT \n");
> > +		return;
> > +	}
> 
> More wordsmithing: "VBT contains no child devices.\n"
> 
Ok.
> >  /**
> >   * intel_init_bios - initialize VBIOS settings & find VBT
> >   * @dev: DRM device
> > @@ -365,6 +428,7 @@
> >  	parse_lfp_panel_data(dev_priv, bdb);
> >  	parse_sdvo_panel_data(dev_priv, bdb);
> >  	parse_sdvo_device_mapping(dev_priv, bdb);
> > +	parse_device_mapping_from_vbt(dev_priv, bdb);
> >  	parse_driver_features(dev_priv, bdb);
> >  
> >  	pci_unmap_rom(pdev, bios);
> 
> Do we know how reliable the child device structures are?  Should we
> limit the parsing to G4x devices?
The child device structure is reliable on the i915/945/965/g4x platform.
In fact this patch set is mainly to solve the issue on the boxes based
on G4X platform. Maybe what you said is right. We can limit this patch
to G4X platform.
thanks.
>