[RFC,1/3] DRM/I915: parse child device from VBT
diff mbox

Message ID 1249613719-22194-2-git-send-email-yakui.zhao@intel.com
State Rejected
Headers show

Commit Message

Zhao, Yakui Aug. 7, 2009, 2:55 a.m. UTC
From: Zhao Yakui <yakui.zhao@intel.com>

Parse the child device from VBT.

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c   |   11 ++++++
 drivers/gpu/drm/i915/i915_drv.h   |    2 +
 drivers/gpu/drm/i915/intel_bios.c |   64 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

Comments

Jesse Barnes Aug. 7, 2009, 5:02 p.m. UTC | #1
On Fri,  7 Aug 2009 10:55:17 +0800
yakui.zhao@intel.com wrote:

> From: Zhao Yakui <yakui.zhao@intel.com>
> 
> 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...

>  	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.

  
> +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"

>  /**
>   * 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?
Zhao, Yakui Aug. 13, 2009, 2:29 a.m. UTC | #2
On Sat, 2009-08-08 at 01:02 +0800, Jesse Barnes wrote:
> On Fri,  7 Aug 2009 10:55:17 +0800
> yakui.zhao@intel.com wrote:
> 
> > From: Zhao Yakui <yakui.zhao@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.
>

Patch
diff mbox

Index: linux-2.6/drivers/gpu/drm/i915/i915_drv.h
===================================================================
--- linux-2.6.orig/drivers/gpu/drm/i915/i915_drv.h	2009-08-07 10:39:56.000000000 +0800
+++ linux-2.6/drivers/gpu/drm/i915/i915_drv.h	2009-08-07 10:40:48.000000000 +0800
@@ -438,6 +438,8 @@ 
 		struct drm_i915_gem_phys_object *phys_objs[I915_MAX_PHYS_OBJECT];
 	} mm;
 	struct sdvo_device_mapping sdvo_mappings[2];
+	int child_dev_num;
+	struct child_device_config *child_dev;
 } drm_i915_private_t;
 
 /** driver private structure attached to each drm_gem_object */
Index: linux-2.6/drivers/gpu/drm/i915/i915_dma.c
===================================================================
--- linux-2.6.orig/drivers/gpu/drm/i915/i915_dma.c	2009-08-07 10:39:56.000000000 +0800
+++ linux-2.6/drivers/gpu/drm/i915/i915_dma.c	2009-08-07 10:40:48.000000000 +0800
@@ -1242,6 +1242,8 @@ 
 	}
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+		dev_priv->child_dev = NULL;
+		dev_priv->child_dev_num = 0;
 		ret = i915_load_modeset_init(dev, prealloc_size, agp_size);
 		if (ret < 0) {
 			DRM_ERROR("failed to init modeset\n");
@@ -1299,6 +1301,15 @@ 
 		mutex_unlock(&dev->struct_mutex);
 		drm_mm_takedown(&dev_priv->vram);
 		i915_gem_lastclose(dev);
+		/*
+		 * free the memory space allocated for the child device
+		 * config parsed from VBT
+		 */
+		if (dev_priv->child_dev_num && dev_priv->child_dev) {
+			kfree(dev_priv->child_dev);
+			dev_priv->child_dev = NULL;
+			dev_priv->child_dev_num = 0;
+		}
 	}
 
 	kfree(dev->dev_private);
Index: linux-2.6/drivers/gpu/drm/i915/intel_bios.c
===================================================================
--- linux-2.6.orig/drivers/gpu/drm/i915/intel_bios.c	2009-08-07 10:40:38.000000000 +0800
+++ linux-2.6/drivers/gpu/drm/i915/intel_bios.c	2009-08-07 10:48:41.000000000 +0800
@@ -315,6 +315,69 @@ 
 		dev_priv->edp_support = 1;
 }
 
+static void
+parse_device_mapping_from_vbt(struct drm_i915_private *dev_priv,
+		       struct bdb_header *bdb)
+{
+	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;
+	}
+	/* 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;
+	}
+	/* 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;
+	}
+	dev_priv->child_dev = kzalloc(sizeof(*p_child) * count, GFP_KERNEL);
+	if (!dev_priv->child_dev)
+		DRM_DEBUG("Can't allocate memory space for child device\n");
+
+	dev_priv->child_dev_num = count;
+	count = 0;
+	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;
+		}
+		child_dev_ptr = dev_priv->child_dev + count;
+		count++;
+		memcpy((void *)child_dev_ptr, (void *)p_child,
+					sizeof(*p_child));
+	}
+	return;
+}
 /**
  * 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);