Re: [MIPI SEQ PARSING v2 PATCH 05/11] drm/i915: Added support the v3 mipi sequence block

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

 




>-----Original Message-----
>From: Jani Nikula [mailto:jani.nikula@xxxxxxxxxxxxxxx]
>Sent: Thursday, September 17, 2015 8:09 PM
>To: Deepak, M; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>Cc: Deepak, M
>Subject: Re:  [MIPI SEQ PARSING v2 PATCH 05/11] drm/i915: Added
>support the v3 mipi sequence block
>
>On Thu, 10 Sep 2015, Deepak M <m.deepak@xxxxxxxxx> wrote:
>> From: vkorjani <vikas.korjani@xxxxxxxxx>
>>
>> The Block 53 of the VBT, which is the MIPI sequence block has
>> undergone a design change because of which the parsing logic has to be
>> changed.
>>
>> The current code will handle the parsing of v3 and other lower
>> versions of the MIPI sequence block.
>>
>> v2: rebase
>>
>> Signed-off-by: vkorjani <vikas.korjani@xxxxxxxxx>
>> Signed-off-by: Deepak M <m.deepak@xxxxxxxxx>
>> ---
>>  drivers/gpu/drm/i915/intel_bios.c          |  119
>+++++++++++++++++++++++-----
>>  drivers/gpu/drm/i915/intel_bios.h          |    8 ++
>>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c |    7 ++
>>  3 files changed, 114 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c
>> b/drivers/gpu/drm/i915/intel_bios.c
>> index 34a1042..cea641f 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -45,6 +45,7 @@ find_section(struct drm_i915_private *dev_priv,
>>  	int index = 0;
>>  	u32 total, current_size;
>>  	u8 current_id;
>> +	u8 version;
>>
>>  	/* skip to first section */
>>  	index += bdb->header_size;
>> @@ -56,7 +57,17 @@ find_section(struct drm_i915_private *dev_priv,
>>  		current_id = *(base + index);
>>  		index++;
>>
>> -		current_size = *((const u16 *)(base + index));
>> +		if (current_id == BDB_MIPI_SEQUENCE) {
>> +			version = *(base + index + 2);
>> +			if (version >= 3)
>> +				current_size = *((const u32 *)(base +
>> +								index + 3));
>> +			else
>> +				current_size = *((const u16 *)(base + index));
>> +		} else {
>> +			current_size = *((const u16 *)(base + index));
>> +		}
>> +
>
>While reviewing I've realized the old kernels will hit this hard. I've submitted a
>patch [1] to be applied to v4.3-rc and older stable kernels so that they fail
>gracefully instead of starting to parse garbage. The real parsing is too big to
>backport to upstream stable. Please review.
>
>[1] http://mid.gmane.org/1442497327-27033-1-git-send-email-
>jani.nikula@xxxxxxxxx
>
>>  		index += 2;
>>
>>  		if (index + current_size > total)
>> @@ -745,6 +756,55 @@ static u8 *goto_next_sequence(u8 *data, int *size)
>>  	return data;
>>  }
>>
>> +static u8 *goto_next_sequence_v3(u8 *data, int *size) {
>> +	int tmp = *size;
>> +	int op_size;
>> +
>> +	if (--tmp < 0)
>> +		return NULL;
>> +
>> +	/* Skip the panel id and the sequence size */
>
>It's not panel id, it's the sequence byte, right?
>
>You could also store data + 1 + size of sequence, and check whether data ends
>up pointing at the same place in the end. They should.
>
>Shouldn't you also take 4 bytes of sequence size field into account in tmp?
>
>> +	data = data + 5;
>> +	while (*data != 0) {
>> +		u8 element_type = *data++;
>> +
>> +		switch (element_type) {
>
>Would be helpful to refer to operation_byte like in the spec.
>
>> +		default:
>> +			DRM_ERROR("Unknown element type %d\n",
>element_type);
>> +		case MIPI_SEQ_ELEM_SEND_PKT:
>> +		case MIPI_SEQ_ELEM_DELAY:
>> +		case MIPI_SEQ_ELEM_GPIO:
>> +		case MIPI_SEQ_ELEM_I2C:
>> +		case MIPI_SEQ_ELEM_SPI:
>> +		case MIPI_SEQ_ELEM_PMIC:
>> +			/*
>> +			 * skip by this element payload size
>> +			 * skip elem id, command flag and data type
>> +			 */
>> +			op_size = *data++;
>> +			tmp = tmp - (op_size + 1);
>> +			if (tmp < 0)
>> +				return NULL;
>
>Isn't each operation operation byte | size of operation | payload size, i.e. your
>tmp change is one byte short?
>
>The fact that the goto_next_sequence* functions increase data and decrease
>size is getting increasingly confusing to follow. One simple alternative would
>be to calculate some endp = start + size up front, and then pass the endp
>around, and check if we're about to go past the end marker.
>
>This is not a problem with your series, it was there already. And fixing it
>doesn't have to be part of your series. It just really takes ages to review this
>approach of range checking. Unless I close my eyes and trust there are no off-
>by-ones anywhere. But that kind of defeats the purpose of review...
>
[Deepak M] Okay will try to simplify the logic.
>> +
>> +			/* skip by len */
>> +			data += op_size;
>> +			break;
>> +		}
>> +	}
>> +
>> +	/* goto next sequence or end of block byte */
>> +	if (--tmp < 0)
>> +		return NULL;
>> +
>> +	/* Skip the end element marker */
>> +	data++;
>> +
>> +	/* update amount of data left for the sequence block to be parsed */
>> +	*size = tmp;
>> +	return data;
>> +}
>> +
>>  static void
>>  parse_mipi(struct drm_i915_private *dev_priv, const struct bdb_header
>> *bdb)  { @@ -754,7 +814,7 @@ parse_mipi(struct drm_i915_private
>> *dev_priv, const struct bdb_header *bdb)
>>  	const struct mipi_pps_data *pps;
>>  	u8 *data;
>>  	const u8 *seq_data;
>> -	int i, panel_id, seq_size;
>> +	int i, panel_id, panel_seq_size;
>>  	u16 block_size;
>>
>>  	/* parse MIPI blocks only if LFP type is MIPI */ @@ -811,29 +871,40
>> @@ parse_mipi(struct drm_i915_private *dev_priv, const struct
>> bdb_header *bdb)
>>
>>  	DRM_DEBUG_DRIVER("Found MIPI sequence block\n");
>>
>> -	block_size = get_blocksize(sequence);
>> -
>>  	/*
>>  	 * parse the sequence block for individual sequences
>>  	 */
>>  	dev_priv->vbt.dsi.seq_version = sequence->version;
>>
>>  	seq_data = &sequence->data[0];
>> +	if (dev_priv->vbt.dsi.seq_version >= 3) {
>> +		block_size = *((unsigned int *)seq_data);
>
>(const u32 *)
>
>> +		seq_data = seq_data + 4;
>> +	} else
>> +		block_size = get_blocksize(sequence);
>
>block_size should be changed to u32.
>
>>
>>  	/*
>>  	 * sequence block is variable length and hence we need to parse and
>>  	 * get the sequence data for specific panel id
>>  	 */
>>  	for (i = 0; i < MAX_MIPI_CONFIGURATIONS; i++) {
>> -		panel_id = *seq_data;
>> -		seq_size = *((u16 *) (seq_data + 1));
>> +		panel_id = *seq_data++;
>> +		if (dev_priv->vbt.dsi.seq_version >= 3) {
>> +			panel_seq_size = *((u32 *)seq_data);
>> +			seq_data += sizeof(u32);
>> +		} else {
>> +			panel_seq_size = *((u16 *)seq_data);
>> +			seq_data += sizeof(u16);
>> +		}
>> +
>>  		if (panel_id == panel_type)
>>  			break;
>>
>> -		/* skip the sequence including seq header of 3 bytes */
>> -		seq_data = seq_data + 3 + seq_size;
>> +		seq_data += panel_seq_size;
>> +
>>  		if ((seq_data - &sequence->data[0]) > block_size) {
>> -			DRM_ERROR("Sequence start is beyond sequence
>block size, corrupted sequence block\n");
>> +			DRM_ERROR("Sequence start is beyond seq block
>size\n");
>> +			DRM_ERROR("Corrupted sequence block\n");
>
>Please don't add two consecutive DRM_ERRORs for the same error.
>
>>  			return;
>>  		}
>>  	}
>> @@ -845,13 +916,14 @@ parse_mipi(struct drm_i915_private *dev_priv,
>> const struct bdb_header *bdb)
>>
>>  	/* check if found sequence is completely within the sequence block
>>  	 * just being paranoid */
>> -	if (seq_size > block_size) {
>> +	if (panel_seq_size > block_size) {
>>  		DRM_ERROR("Corrupted sequence/size, bailing out\n");
>>  		return;
>>  	}
>>
>> -	/* skip the panel id(1 byte) and seq size(2 bytes) */
>> -	dev_priv->vbt.dsi.data = kmemdup(seq_data + 3, seq_size,
>GFP_KERNEL);
>> +
>> +	dev_priv->vbt.dsi.data = kmemdup(seq_data, panel_seq_size,
>> +GFP_KERNEL);
>> +
>>  	if (!dev_priv->vbt.dsi.data)
>>  		return;
>>
>> @@ -860,29 +932,36 @@ parse_mipi(struct drm_i915_private *dev_priv,
>const struct bdb_header *bdb)
>>  	 * There are only 5 types of sequences as of now
>>  	 */
>>  	data = dev_priv->vbt.dsi.data;
>> -	dev_priv->vbt.dsi.size = seq_size;
>> +	dev_priv->vbt.dsi.size = panel_seq_size;
>>
>>  	/* two consecutive 0x00 indicate end of all sequences */
>> -	while (1) {
>> +	while (*data != 0) {
>>  		int seq_id = *data;
>> +		int seq_size;
>
>u32
>
>> +
>>  		if (MIPI_SEQ_MAX > seq_id && seq_id >
>MIPI_SEQ_UNDEFINED) {
>>  			dev_priv->vbt.dsi.sequence[seq_id] = data;
>>  			DRM_DEBUG_DRIVER("Found mipi sequence - %d\n",
>seq_id);
>>  		} else {
>> -			DRM_ERROR("undefined sequence\n");
>> -			goto err;
>> +			DRM_ERROR("undefined sequence - %d\n", seq_id);
>> +			seq_size = *(data + 1);
>
>Needs to be *((const u32 *)(data + 1)) or you'll ignore 3 highest order bytes.
>
>> +			if (dev_priv->vbt.dsi.seq_version >= 3) {
>> +				data = data + seq_size + 1;
>> +				continue;
>> +			} else
>> +				goto err;
>>  		}
>>
>>  		/* partial parsing to skip elements */
>> -		data = goto_next_sequence(data, &seq_size);
>> +		if (dev_priv->vbt.dsi.seq_version >= 3)
>> +			data = goto_next_sequence_v3(data,
>&panel_seq_size);
>> +		else
>> +			data = goto_next_sequence(data, &panel_seq_size);
>>
>>  		if (data == NULL) {
>>  			DRM_ERROR("Sequence elements going beyond
>block itself. Sequence block parsing failed\n");
>>  			goto err;
>>  		}
>> -
>> -		if (*data == 0)
>> -			break; /* end of sequence reached */
>>  	}
>>
>>  	DRM_DEBUG_DRIVER("MIPI related vbt parsing complete\n"); diff --
>git
>> a/drivers/gpu/drm/i915/intel_bios.h
>> b/drivers/gpu/drm/i915/intel_bios.h
>> index 21a7f3f..7a4ba41 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.h
>> +++ b/drivers/gpu/drm/i915/intel_bios.h
>> @@ -948,6 +948,12 @@ enum mipi_seq {
>>  	MIPI_SEQ_DISPLAY_ON,
>>  	MIPI_SEQ_DISPLAY_OFF,
>>  	MIPI_SEQ_DEASSERT_RESET,
>> +	MIPI_SEQ_BACKLIGHT_ON,
>> +	MIPI_SEQ_BACKLIGHT_OFF,
>> +	MIPI_SEQ_TEAR_ON,
>> +	MIPI_SEQ_TEAR_OFF,
>> +	MIPI_SEQ_POWER_ON,
>> +	MIPI_SEQ_POWER_OFF,
>>  	MIPI_SEQ_MAX
>>  };
>>
>> @@ -957,6 +963,8 @@ enum mipi_seq_element {
>>  	MIPI_SEQ_ELEM_DELAY,
>>  	MIPI_SEQ_ELEM_GPIO,
>>  	MIPI_SEQ_ELEM_I2C,
>> +	MIPI_SEQ_ELEM_SPI,
>> +	MIPI_SEQ_ELEM_PMIC,
>>  	MIPI_SEQ_ELEM_STATUS,
>
>Again, MIPI_SEQ_ELEM_STATUS is not spec.
>
>>  	MIPI_SEQ_ELEM_MAX
>>  };
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> index 9989f61..c6a6fa1 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> @@ -317,6 +317,8 @@ static const char * const seq_name[] = {
>>
>>  static void generic_exec_sequence(struct intel_dsi *intel_dsi, const
>> u8 *data)  {
>> +	struct drm_device *dev = intel_dsi->base.base.dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	fn_mipi_elem_exec mipi_elem_exec;
>>  	int index;
>>
>> @@ -327,6 +329,8 @@ static void generic_exec_sequence(struct intel_dsi
>> *intel_dsi, const u8 *data)
>>
>>  	/* go to the first element of the sequence */
>>  	data++;
>> +	if (dev_priv->vbt.dsi.seq_version >= 3)
>> +		data = data + 4;
>>
>>  	/* parse each byte till we reach end of sequence byte - 0x00 */
>>  	while (1) {
>> @@ -340,6 +344,9 @@ static void generic_exec_sequence(struct intel_dsi
>*intel_dsi, const u8 *data)
>>  		/* goto element payload */
>>  		data++;
>>
>> +		if (dev_priv->vbt.dsi.seq_version >= 3)
>> +			data++;
>> +
>>  		/* execute the element specific rotines */
>>  		data = mipi_elem_exec(intel_dsi, data);
>>
>> --
>> 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
_______________________________________________
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