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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 10 Sep 2015, Deepak M <m.deepak@intel.com> wrote:
> From: vkorjani <vikas.korjani@intel.com>
>
> 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@intel.com>
> Signed-off-by: Deepak M <m.deepak@intel.com>
> ---
>  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@intel.com

>  		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...

> +
> +			/* 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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx