Re: [PATCH 2/2] drm/i915: Add parsing support for new MIPI blocks in VBT

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

On Thu, 13 Feb 2014, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> The parser extracts the config block(#52) and sequence(#53) data
> and store in private data structures.
>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |   6 ++
>  drivers/gpu/drm/i915/intel_bios.c | 175 ++++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_bios.h |  34 ++++++++
>  drivers/gpu/drm/i915/intel_dsi.h  |   1 +
>  4 files changed, 211 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 728b9c3..9bede78 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1250,7 +1250,13 @@ struct intel_vbt_data {
>  
>  	/* MIPI DSI */
>  	struct {
> +		u8 seq_version;
>  		u16 panel_id;
> +		struct mipi_config *config;
> +		struct mipi_pps_data *pps;
> +		u32 size;
> +		u8 *data;
> +		u8 *sequence[MIPI_SEQ_MAX];

Please group sequence related fields (seq_version, data, size, sequence)
together.

>  	} dsi;
>  
>  	int crt_ddc_pin;
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index cf0b25d..9d6d063 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -595,19 +595,184 @@ parse_edp(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
>  	}
>  }
>  
> +u8 *goto_next_sequence(u8 *data)

Static.

As I'll explain below, you need to pass remaining size here, and make
sure you don't overrun the buffer.

> +{
> +	u16 len;
> +	/* goto first element */
> +	data++;
> +	while (1) {
> +		switch (*data++) {
> +		case MIPI_SEQ_ELEM_SEND_PKT:
> +			/* skip by this element payload size
> +			 * skip command flag and data type */
> +			data += 2;
> +			len = *((u16 *)data);
> +			/* skip by len */
> +			data = data + 2 + len;
> +			break;
> +		case MIPI_SEQ_ELEM_DELAY:
> +			data += 4;
> +			break;
> +		case MIPI_SEQ_ELEM_GPIO:
> +			data += 2;
> +			break;
> +		default:
> +			DRM_ERROR("Unknown element\n");
> +			break;
> +		}
> +
> +		/* end of sequence ? */
> +		if (*data == 0)
> +			break;
> +	}
> +	/* goto next sequence or end of block byte */
> +	data++;
> +	return data;
> +}
> +
>  static void
>  parse_mipi(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
>  {
> -	struct bdb_mipi *mipi;
> +	struct bdb_mipi_config *start;
> +	struct bdb_mipi_sequence *sequence;
> +	struct mipi_config *config;
> +	struct mipi_pps_data *pps;
> +	char *data, *seq_data, *seq_start;
> +	int i, panel_id, seq_size;
> +
> +	/* Initialize this to undefined indicating no generic MIPI support */
> +	dev_priv->vbt.dsi.panel_id = MIPI_DSI_UNDEFINED_PANEL_ID;
> +
> +	/* Block #40 is already parsed and panel_fixed_mode is
> +	 * stored in dev_priv->lfp_lvds_vbt_mode
> +	 * resuse this when needed
> +	 */
>  
> -	mipi = find_section(bdb, BDB_MIPI_CONFIG);
> -	if (!mipi) {
> -		DRM_DEBUG_KMS("No MIPI BDB found");
> +	/* Parse #52 for panel index used from panel_type already
> +	 * parsed
> +	 */
> +	start = find_section(bdb, BDB_MIPI_CONFIG);
> +	if (!start) {
> +		DRM_DEBUG_KMS("No MIPI config BDB found");
>  		return;
>  	}
>  
> -	/* XXX: add more info */
> +	DRM_DEBUG_DRIVER("Found MIPI Config block, panel index = %d\n",
> +								panel_type);
> +
> +	/*
> +	 * get hold of the correct configuration block and pps data as per
> +	 * the panel_type as index
> +	 */
> +	config = &start->config[panel_type];
> +	pps = (struct mipi_pps_data *) &start->config[MAX_MIPI_CONFIGURATIONS];
> +	pps = &pps[panel_type];

Please change this as proposed in my review of patch 1/2.

> +
> +	/* store as of now full data. Trim when we realise all is not needed */
> +	dev_priv->vbt.dsi.config =
> +			kzalloc(sizeof(struct mipi_config), GFP_KERNEL);
> +	if (!dev_priv->vbt.dsi.config)
> +		return;
> +
> +	dev_priv->vbt.dsi.pps =
> +			kzalloc(sizeof(struct mipi_pps_data), GFP_KERNEL);
> +	if (!dev_priv->vbt.dsi.pps) {
> +		kfree(dev_priv->vbt.dsi.config);
> +		return;
> +	}
> +
> +	memcpy(dev_priv->vbt.dsi.config, config, sizeof(*config));
> +	memcpy(dev_priv->vbt.dsi.pps, pps, sizeof(*pps));

See kmemdup for both of these.

> +
> +	/* We have mandatory mipi config blocks. Initialize as generic panel */
>  	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);
> +	if (!sequence) {
> +		DRM_DEBUG_KMS("No MIPI Sequnece BDB found");
> +		DRM_DEBUG_DRIVER("MIPI related vbt parsing complete\n");

One debug msg should be enough.

> +		return;
> +	}
> +
> +	DRM_DEBUG_DRIVER("Found MIPI sequence block\n");
> +
> +	/*
> +	 * parse the sequence block for individual sequences
> +	 */
> +	dev_priv->vbt.dsi.seq_version = sequence->version;
> +
> +	seq_data = (char *)((char *)sequence + 1);

If ->data is turned into u8 data[0] as suggested in my other mail, you
can do:

	seq_data = &sequence->data[0];

> +
> +	/* sequnec block is variable length and hence we need to parse and

sequence ^

> +	 * get the sequnece data for specific panel id */
> +	for (i = 0; i < MAX_MIPI_CONFIGURATIONS; i++) {
> +		panel_id = *seq_data;
> +		seq_size = *((u16 *) (seq_data + 1));
> +		if (panel_id == panel_type) {
> +			seq_start = (char *) (seq_data + 3);

seq_start is redundant, you could just reuse seq_data here.

> +			break;
> +		}
> +
> +		/* skip the sequence including seq header of 3 bytes */
> +		seq_data = seq_data + 3 + seq_size;

You need to make sure this doesn't go beyond the bdb block size. The
same for the block you find for panel_id == panel_type. I'm afraid we
can't trust the bios here; it might be plain wrong, or the specs might
change, or something.

> +	}
> +
> +	if (i == MAX_MIPI_CONFIGURATIONS)

Might be worth a debug message.

> +		return;
> +
> +	dev_priv->vbt.dsi.data = kzalloc(seq_size, GFP_KERNEL);
> +	if (!dev_priv->vbt.dsi.data)
> +		return;
> +
> +	memcpy(dev_priv->vbt.dsi.data, seq_start, seq_size);

kmemdup

> +
> +	/*
> +	 * loop into the sequence data and split into multiple sequneces
> +	 * There are only 5 types of sequences as of now
> +	 */
> +	data = dev_priv->vbt.dsi.data;
> +	dev_priv->vbt.dsi.size = seq_size;
> +
> +	/* two consecutive 0x00 inidcate end of all sequences */
> +	while (1) {

This needs to check for reading past seq_size.

> +		switch (*data) {
> +		case MIPI_SEQ_ASSERT_RESET:
> +			dev_priv->vbt.dsi.sequence[MIPI_SEQ_ASSERT_RESET] =
> +									data;
> +			DRM_DEBUG_DRIVER("Found MIPI_SEQ_ASSERT_RESET\n");
> +			break;
> +		case MIPI_SEQ_INIT_OTP:
> +			dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP] = data;
> +			DRM_DEBUG_DRIVER("Found MIPI_SEQ_INIT_OTP\n");
> +			break;
> +		case MIPI_SEQ_DISPLAY_ON:
> +			dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_ON] = data;
> +			DRM_DEBUG_DRIVER("Found MIPI_SEQ_DISPLAY_ON\n");
> +			break;
> +		case MIPI_SEQ_DISPLAY_OFF:
> +			dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_OFF] = data;
> +			DRM_DEBUG_DRIVER("Found MIPI_SEQ_DISPLAY_OFF\n");
> +			break;
> +		case MIPI_SEQ_DEASSERT_RESET:
> +			dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET] =
> +									data;
> +			DRM_DEBUG_DRIVER("Found MIPI_SEQ_DEASSERT_RESET\n");

All of the above are identical apart from the array index (which you
have in *data), and the debug message. Maybe it's sufficient to have
*data in the debug message too?

> +			break;
> +		case MIPI_SEQ_UNDEFINED:
> +		default:
> +			DRM_ERROR("undefined sequnce\n");
> +			continue;
> +		}
> +
> +		/* partial parsing to skip elements */
> +		data = goto_next_sequence(data);

You need to pass the remaining size to goto_next_sequence and handle
buffer overruns there too.

> +
> +		if (*data == 0)
> +			break; /* end of sequence reached */
> +	}
> +
> +	DRM_DEBUG_DRIVER("MIPI related vbt parsing complete\n");
>  }
>  
>  static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index 8345e0e..dcc4bc5 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -849,4 +849,38 @@ struct bdb_mipi_sequence {
>  	void *data;
>  };
>  
> +/* MIPI Sequnece Block definitions */
> +enum MIPI_SEQ {
> +	MIPI_SEQ_UNDEFINED = 0,
> +	MIPI_SEQ_ASSERT_RESET,
> +	MIPI_SEQ_INIT_OTP,
> +	MIPI_SEQ_DISPLAY_ON,
> +	MIPI_SEQ_DISPLAY_OFF,
> +	MIPI_SEQ_DEASSERT_RESET,
> +	MIPI_SEQ_MAX
> +
> +};
> +
> +enum MIPI_SEQ_ELEMENT {
> +	MIPI_SEQ_ELEM_UNDEFINED = 0,
> +	MIPI_SEQ_ELEM_SEND_PKT,
> +	MIPI_SEQ_ELEM_DELAY,
> +	MIPI_SEQ_ELEM_GPIO,
> +	MIPI_SEQ_ELEM_STATUS,
> +	MIPI_SEQ_ELEM_MAX
> +
> +};
> +
> +enum MIPI_GPIO_PIN_INDEX {
> +	MIPI_GPIO_UNDEFINED = 0,
> +	MIPI_GPIO_PANEL_ENABLE,
> +	MIPI_GPIO_BL_ENABLE,
> +	MIPI_GPIO_PWM_ENABLE,
> +	MIPI_GPIO_RESET_N,
> +	MIPI_GPIO_PWR_DOWN_R,
> +	MIPI_GPIO_STDBY_RST_N,
> +	MIPI_GPIO_MAX
> +
> +};
> +
>  #endif /* _I830_BIOS_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
> index a0e42db..01b6752 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -120,6 +120,7 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
>  extern void vlv_enable_dsi_pll(struct intel_encoder *encoder);
>  extern void vlv_disable_dsi_pll(struct intel_encoder *encoder);
>  
> +#define MIPI_DSI_UNDEFINED_PANEL_ID	0
>  #define MIPI_DSI_GENERIC_PANEL_ID	1

As said, please move these to intel_bios.h to only have one-way
dependency.

>  
>  #endif /* _INTEL_DSI_H */
> -- 
> 1.8.3.2
>

-- 
Jani Nikula, Intel Open Source Technology Center