On Thu, 17 Sep 2015, Jani Nikula <[email protected]> wrote: > On Thu, 10 Sep 2015, Deepak M <[email protected]> wrote: >> Currently the field in bdb header which indicates the VBT size >> is of 2 bytes, but there are some cases where VBT size exceeds >> 64KB in which case this field may not contain the correct VBT size. >> So its better to get the VBT size from the mailbox3 if >> VBT is not present in the mailbox4 of opregion. >> >> v2: - Use opregion filed from dev_priv struct instead of creating >> a new field in dev_priv (Jani) >> - Have vbt_size field vaild in all scenarios (Jani) >> - rebase >> >> Signed-off-by: Deepak M <[email protected]> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 2 ++ >> drivers/gpu/drm/i915/intel_bios.c | 42 >> +++++++++++++++++++-------------- >> drivers/gpu/drm/i915/intel_opregion.c | 6 +++++ >> 3 files changed, 32 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h >> index 507d57a..91ccbc6 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1777,6 +1777,8 @@ struct drm_i915_private { >> u32 pm_rps_events; >> u32 pipestat_irq_mask[I915_MAX_PIPES]; >> >> + u32 vbt_size; > > No. dev_priv is not a random area to throw things into. The place you're > looking for is dev_priv->opregion, i.e. struct intel_opregion. > >> + >> struct i915_hotplug hotplug; >> struct i915_fbc fbc; >> struct i915_drrs drrs; >> diff --git a/drivers/gpu/drm/i915/intel_bios.c >> b/drivers/gpu/drm/i915/intel_bios.c >> index 1932a86..34a1042 100644 >> --- a/drivers/gpu/drm/i915/intel_bios.c >> +++ b/drivers/gpu/drm/i915/intel_bios.c >> @@ -37,17 +37,19 @@ >> static int panel_type; >> >> static const void * >> -find_section(const void *_bdb, int section_id) >> +find_section(struct drm_i915_private *dev_priv, >> + const void *_bdb, int section_id) >> { >> const struct bdb_header *bdb = _bdb; >> const u8 *base = _bdb; >> int index = 0; >> - u16 total, current_size; >> + u32 total, current_size; >> u8 current_id; >> >> /* skip to first section */ >> index += bdb->header_size; >> - total = bdb->bdb_size; >> + >> + total = dev_priv->vbt_size; > > vbt_size != bdb_size. See below. > >> >> /* walk the sections looking for section_id */ >> while (index + 3 < total) { >> @@ -179,7 +181,7 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv, >> struct drm_display_mode *panel_fixed_mode; >> int drrs_mode; >> >> - lvds_options = find_section(bdb, BDB_LVDS_OPTIONS); >> + lvds_options = find_section(dev_priv, bdb, BDB_LVDS_OPTIONS); >> if (!lvds_options) >> return; >> >> @@ -211,11 +213,12 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv, >> break; >> } >> >> - lvds_lfp_data = find_section(bdb, BDB_LVDS_LFP_DATA); >> + lvds_lfp_data = find_section(dev_priv, bdb, BDB_LVDS_LFP_DATA); >> if (!lvds_lfp_data) >> return; >> >> - lvds_lfp_data_ptrs = find_section(bdb, BDB_LVDS_LFP_DATA_PTRS); >> + lvds_lfp_data_ptrs = find_section(dev_priv, bdb, >> + BDB_LVDS_LFP_DATA_PTRS); >> if (!lvds_lfp_data_ptrs) >> return; >> >> @@ -257,7 +260,7 @@ parse_lfp_backlight(struct drm_i915_private *dev_priv, >> const struct bdb_lfp_backlight_data *backlight_data; >> const struct bdb_lfp_backlight_data_entry *entry; >> >> - backlight_data = find_section(bdb, BDB_LVDS_BACKLIGHT); >> + backlight_data = find_section(dev_priv, bdb, BDB_LVDS_BACKLIGHT); >> if (!backlight_data) >> return; >> >> @@ -305,14 +308,15 @@ parse_sdvo_panel_data(struct drm_i915_private >> *dev_priv, >> if (index == -1) { >> const struct bdb_sdvo_lvds_options *sdvo_lvds_options; >> >> - sdvo_lvds_options = find_section(bdb, BDB_SDVO_LVDS_OPTIONS); >> + sdvo_lvds_options = find_section(dev_priv, bdb, >> + BDB_SDVO_LVDS_OPTIONS); >> if (!sdvo_lvds_options) >> return; >> >> index = sdvo_lvds_options->panel_type; >> } >> >> - dvo_timing = find_section(bdb, BDB_SDVO_PANEL_DTDS); >> + dvo_timing = find_section(dev_priv, bdb, BDB_SDVO_PANEL_DTDS); >> if (!dvo_timing) >> return; >> >> @@ -349,7 +353,7 @@ parse_general_features(struct drm_i915_private *dev_priv, >> struct drm_device *dev = dev_priv->dev; >> const struct bdb_general_features *general; >> >> - general = find_section(bdb, BDB_GENERAL_FEATURES); >> + general = find_section(dev_priv, bdb, BDB_GENERAL_FEATURES); >> if (general) { >> dev_priv->vbt.int_tv_support = general->int_tv_support; >> dev_priv->vbt.int_crt_support = general->int_crt_support; >> @@ -374,7 +378,7 @@ parse_general_definitions(struct drm_i915_private >> *dev_priv, >> { >> const struct bdb_general_definitions *general; >> >> - general = find_section(bdb, BDB_GENERAL_DEFINITIONS); >> + general = find_section(dev_priv, bdb, BDB_GENERAL_DEFINITIONS); >> if (general) { >> u16 block_size = get_blocksize(general); >> if (block_size >= sizeof(*general)) { >> @@ -405,7 +409,7 @@ parse_sdvo_device_mapping(struct drm_i915_private >> *dev_priv, >> int i, child_device_num, count; >> u16 block_size; >> >> - p_defs = find_section(bdb, BDB_GENERAL_DEFINITIONS); >> + p_defs = find_section(dev_priv, bdb, BDB_GENERAL_DEFINITIONS); >> if (!p_defs) { >> DRM_DEBUG_KMS("No general definition block is found, unable to >> construct sdvo mapping.\n"); >> return; >> @@ -491,7 +495,7 @@ parse_driver_features(struct drm_i915_private *dev_priv, >> { >> const struct bdb_driver_features *driver; >> >> - driver = find_section(bdb, BDB_DRIVER_FEATURES); >> + driver = find_section(dev_priv, bdb, BDB_DRIVER_FEATURES); >> if (!driver) >> return; >> >> @@ -519,7 +523,7 @@ parse_edp(struct drm_i915_private *dev_priv, const >> struct bdb_header *bdb) >> const struct edp_power_seq *edp_pps; >> const struct edp_link_params *edp_link_params; >> >> - edp = find_section(bdb, BDB_EDP); >> + edp = find_section(dev_priv, bdb, BDB_EDP); >> if (!edp) { >> if (dev_priv->vbt.edp_support) >> DRM_DEBUG_KMS("No eDP BDB found but eDP panel >> supported.\n"); >> @@ -630,7 +634,7 @@ parse_psr(struct drm_i915_private *dev_priv, const >> struct bdb_header *bdb) >> const struct bdb_psr *psr; >> const struct psr_table *psr_table; >> >> - psr = find_section(bdb, BDB_PSR); >> + psr = find_section(dev_priv, bdb, BDB_PSR); >> if (!psr) { >> DRM_DEBUG_KMS("No PSR BDB found.\n"); >> return; >> @@ -768,7 +772,7 @@ parse_mipi(struct drm_i915_private *dev_priv, const >> struct bdb_header *bdb) >> /* Parse #52 for panel index used from panel_type already >> * parsed >> */ >> - start = find_section(bdb, BDB_MIPI_CONFIG); >> + start = find_section(dev_priv, bdb, BDB_MIPI_CONFIG); >> if (!start) { >> DRM_DEBUG_KMS("No MIPI config BDB found"); >> return; >> @@ -799,7 +803,7 @@ parse_mipi(struct drm_i915_private *dev_priv, const >> struct bdb_header *bdb) >> dev_priv->vbt.dsi.panel_id = MIPI_DSI_GENERIC_PANEL_ID; >> >> /* Check if we have sequence block as well */ >> - sequence = find_section(bdb, BDB_MIPI_SEQUENCE); >> + sequence = find_section(dev_priv, bdb, BDB_MIPI_SEQUENCE); >> if (!sequence) { >> DRM_DEBUG_KMS("No MIPI Sequence found, parsing complete\n"); >> return; >> @@ -1077,7 +1081,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv, >> u8 expected_size; >> u16 block_size; >> >> - p_defs = find_section(bdb, BDB_GENERAL_DEFINITIONS); >> + p_defs = find_section(dev_priv, bdb, BDB_GENERAL_DEFINITIONS); >> if (!p_defs) { >> DRM_DEBUG_KMS("No general definition block is found, no devices >> defined.\n"); >> return; >> @@ -1280,6 +1284,8 @@ intel_parse_bios(struct drm_device *dev) >> pci_unmap_rom(pdev, bios); >> return -1; >> } >> + >> + dev_priv->vbt_size = bdb->bdb_size; >> } >> >> /* Grab useful general definitions */ >> diff --git a/drivers/gpu/drm/i915/intel_opregion.c >> b/drivers/gpu/drm/i915/intel_opregion.c >> index 3a43db9..c2f1a4a 100644 >> --- a/drivers/gpu/drm/i915/intel_opregion.c >> +++ b/drivers/gpu/drm/i915/intel_opregion.c >> @@ -793,6 +793,12 @@ int intel_opregion_setup(struct drm_device *dev) >> goto err_vbt; >> } >> >> + /* Assigning the vbt_size based on the VBT location */ >> + if (opregion->header->opregion_ver >= 2 && opregion->asle->rvda) >> + dev_priv->vbt_size = opregion->asle->rvds; > > IIUC this one includes VBT Header... > >> + else >> + dev_priv->vbt_size = bdb->bdb_size; > > ...and this one doesn't, so one of them will be wrong in the size checks > in find_section. > > If you have a variable called "vbt_size" it better include all of VBT, > and not just part of it.
Okay, this is not your fault, but it's hard for me to review the code because the VBT spec still has these inconsistensies: VBT Table Size in VBT Header includes "VBT header, BDB Header and all Data blocks". VBT Table Size is a 16-bit value. How can the MIPI Sequence Block require a separate 32-bit size field, when it then couldn't fit in the VBT? BDB Size in BDB Header includes "BDB Header and all Data blocks". BDB Size is a 16-bit value. Same thing. BR, Jani. > > > BR, > Jani. > > >> + >> dev_priv->bdb_start = bdb; >> opregion->vbt = vbt_base; >> >> -- >> 1.7.9.5 >> >> _______________________________________________ >> Intel-gfx mailing list >> [email protected] >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
