| Message ID | 1249613719-22194-2-git-send-email-yakui.zhao@intel.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
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?
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. >
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);