Re: [MIPI SEQ PARSING v2 PATCH 04/11] drm/i915: Using the approprite vbt size if vbt is not in mailbox4 of opregion

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux