>-----Original Message----- >From: Jani Nikula [mailto:jani.nikula@xxxxxxxxxxxxxxx] >Sent: Thursday, September 17, 2015 7:01 PM >To: Deepak, M; intel-gfx@xxxxxxxxxxxxxxxxxxxxx >Cc: Deepak, M >Subject: Re: [MIPI SEQ PARSING v2 PATCH 04/11] drm/i915: Using >the approprite vbt size if vbt is not in mailbox4 of opregion > >On Thu, 17 Sep 2015, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote: >> On Thu, 10 Sep 2015, Deepak M <m.deepak@xxxxxxxxx> 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 <m.deepak@xxxxxxxxx> >>> --- >>> 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? > [Deepak M] Yes, there is a miss in the VBT spec and then these VBT`s are already used in the products. Here is what I thought to counter it, with an assumption: When the VBT is not present in the mailbox4 then VBT size needs to be read from the mailbox3 and this VBT size field is of 4 bytes which implies that it can be more than 64KB (2^16 - max size which can be stored in the VBT Table size in the VBT header )also. If the VBT size is more than 64KB then the VBT size field in the bdb header can't be relied. So assumption is ; it's better to consider the vbt size from the mailbox3 when the VBT is not present in mailbox4. >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 >>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >>> 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 Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
