[PATCH 00/15] drm/i915/bios: mipi sequence block v3, etc.

[PATCH 00/15] drm/i915/bios: mipi sequence block v3, etc.

[Jani Nikula]

Hi all, this is follow-up to my earlier opregion/vbt series [1]. This
covers patches 1 and 7 of Deepak's series [2], but does quite a bunch of
rework to make everything as neat as it can be given the spec. There's
also some generic VBT documentation.

Unfortunately I haven't been able to test this on a machine that
actually has MIPI sequence block v3. However most of the series does not
touch that yet.

BR,
Jani.


[1] http://mid.gmane.org/cover.1450089383.git.jani.nikula@intel.com
[2] http://mid.gmane.org/1448923632-16760-1-git-send-email-m.deepak@intel.com

Jani Nikula (14):
  drm/i915/bios: add proper documentation for the Video BIOS Table (VBT)
  drm/i915/bios: fix header define name for intel_bios.h
  drm/i915/bios: split the MIPI DSI VBT block parsing to two
  drm/i915/bios: have get_blocksize() support MIPI sequence block v3+
  drm/i915/bios: abstract finding the panel sequence block
  drm/i915/bios: rewrite sequence block parsing
  drm/i915/dsi: be defensive about out of bounds sequence id
  drm/i915/dsi: be defensive about out of bounds operation byte
  drm/i915/bios: interpret the i2c element
  drm/i915/bios: add sequences for MIPI sequence block v2
  drm/i915/bios: add defines for v3 sequence block
  drm/i915/bios: add support for MIPI sequence block v3
  drm/i915/dsi: skip unknown elements for sequence block v3+
  drm/i915/dsi: reduce tedious repetition

vkorjani (1):
  drm/i915: Adding the parsing logic for the i2c element

 Documentation/DocBook/gpu.tmpl             |   6 +
 drivers/gpu/drm/i915/i915_drv.h            |   2 +-
 drivers/gpu/drm/i915/intel_bios.c          | 376 +++++++++++++++++------------
 drivers/gpu/drm/i915/intel_bios.h          |  60 +++--
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 198 +++++++++------
 5 files changed, 397 insertions(+), 245 deletions(-)

-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/15] drm/i915/bios: add proper documentation for the Video BIOS Table (VBT)

[Jani Nikula]

Add an overview and documentation for the VBT/BDB header structures.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 Documentation/DocBook/gpu.tmpl    |  6 ++++++
 drivers/gpu/drm/i915/intel_bios.c | 24 +++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_bios.h | 38 ++++++++++++++++++++++++++++----------
 3 files changed, 57 insertions(+), 11 deletions(-)

diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
index 6c6e81a9eaf4..f96a5d467f84 100644
--- a/Documentation/DocBook/gpu.tmpl
+++ b/Documentation/DocBook/gpu.tmpl
@@ -3507,6 +3507,12 @@ int num_ioctls;</synopsis>
 !Pdrivers/gpu/drm/i915/intel_csr.c csr support for dmc
 !Idrivers/gpu/drm/i915/intel_csr.c
       </sect2>
+      <sect2>
+	<title>Video BIOS Table (VBT)</title>
+!Pdrivers/gpu/drm/i915/intel_bios.c Video BIOS Table (VBT)
+!Idrivers/gpu/drm/i915/intel_bios.c
+!Idrivers/gpu/drm/i915/intel_bios.h
+      </sect2>
     </sect1>
 
     <sect1>
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index eba3e0f87181..d487f602a10e 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -31,6 +31,28 @@
 #include "i915_drv.h"
 #include "intel_bios.h"
 
+/**
+ * DOC: Video BIOS Table (VBT)
+ *
+ * The Video BIOS Table, or VBT, provides platform and board specific
+ * configuration information to the driver that is not discoverable or available
+ * through other means. The configuration is mostly related to display
+ * hardware. The VBT is available via the ACPI OpRegion or, on older systems, in
+ * the PCI ROM.
+ *
+ * The VBT consists of a VBT Header (defined as &struct vbt_header), BDB Header
+ * (&struct bdb_header), and a number of BIOS Data Blocks (BDB) that contain the
+ * actual configuration information. The VBT Header, and thus the VBT, begins
+ * with "$VBT" signature. The VBT Header contains the offset of the BDB
+ * Header. The data blocks are concatenated after the BDB Header. The data
+ * blocks have a 1-byte Block ID, 2-byte Block Size, and Block Size bytes of
+ * data. (Block 53, the MIPI Sequence Block is an exception.)
+ *
+ * The driver parses the VBT during load. The relevant information is stored in
+ * driver private data for ease of use, and the actual VBT is not read after
+ * that.
+ */
+
 #define	SLAVE_ADDR1	0x70
 #define	SLAVE_ADDR2	0x72
 
@@ -1285,7 +1307,7 @@ static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
 
 /**
  * intel_bios_init - find VBT and initialize settings from the BIOS
- * @dev: DRM device
+ * @dev_priv: i915 device instance
  *
  * Loads the Video BIOS and checks that the VBT exists.  Sets scratch registers
  * to appropriate values.
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 54eac1003a1e..2dc46a98c332 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -28,22 +28,40 @@
 #ifndef _I830_BIOS_H_
 #define _I830_BIOS_H_
 
+/**
+ * struct vbt_header - VBT Header structure
+ * @signature:		VBT signature, always starts with "$VBT"
+ * @version:		Version of this structure
+ * @header_size:	Size of this structure
+ * @vbt_size:		Size of VBT (VBT Header, BDB Header and data blocks)
+ * @vbt_checksum:	Checksum
+ * @reserved0:		Reserved
+ * @bdb_offset:		Offset of &struct bdb_header from beginning of VBT
+ * @aim_offset:		Offsets of add-in data blocks from beginning of VBT
+ */
 struct vbt_header {
-	u8 signature[20];		/**< Always starts with 'VBT$' */
-	u16 version;			/**< decimal */
-	u16 header_size;		/**< in bytes */
-	u16 vbt_size;			/**< in bytes */
+	u8 signature[20];
+	u16 version;
+	u16 header_size;
+	u16 vbt_size;
 	u8 vbt_checksum;
 	u8 reserved0;
-	u32 bdb_offset;			/**< from beginning of VBT */
-	u32 aim_offset[4];		/**< from beginning of VBT */
+	u32 bdb_offset;
+	u32 aim_offset[4];
 } __packed;
 
+/**
+ * struct bdb_header - BDB Header structure
+ * @signature:		BDB signature "BIOS_DATA_BLOCK"
+ * @version:		Version of the data block definitions
+ * @header_size:	Size of this structure
+ * @bdb_size:		Size of BDB (BDB Header and data blocks)
+ */
 struct bdb_header {
-	u8 signature[16];		/**< Always 'BIOS_DATA_BLOCK' */
-	u16 version;			/**< decimal */
-	u16 header_size;		/**< in bytes */
-	u16 bdb_size;			/**< in bytes */
+	u8 signature[16];
+	u16 version;
+	u16 header_size;
+	u16 bdb_size;
 } __packed;
 
 /* strictly speaking, this is a "skip" block, but it has interesting info */
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/15] drm/i915/bios: fix header define name for intel_bios.h

[Jani Nikula]

Just for OCD.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 2dc46a98c332..21c162e01189 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -25,8 +25,8 @@
  *
  */
 
-#ifndef _I830_BIOS_H_
-#define _I830_BIOS_H_
+#ifndef _INTEL_BIOS_H_
+#define _INTEL_BIOS_H_
 
 /**
  * struct vbt_header - VBT Header structure
@@ -983,4 +983,4 @@ enum mipi_gpio_pin_index {
 	MIPI_GPIO_MAX
 };
 
-#endif /* _I830_BIOS_H_ */
+#endif /* _INTEL_BIOS_H_ */
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/15] drm/i915/bios: split the MIPI DSI VBT block parsing to two

[Jani Nikula]

There's two blocks to parse, have one function per block. The existing
one cuts neatly into two.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index d487f602a10e..91540ab15e0b 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -765,16 +765,12 @@ static u8 *goto_next_sequence(u8 *data, int *size)
 }
 
 static void
-parse_mipi(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
+parse_mipi_config(struct drm_i915_private *dev_priv,
+		  const struct bdb_header *bdb)
 {
 	const struct bdb_mipi_config *start;
-	const struct bdb_mipi_sequence *sequence;
 	const struct mipi_config *config;
 	const struct mipi_pps_data *pps;
-	u8 *data;
-	const u8 *seq_data;
-	int i, panel_id, seq_size;
-	u16 block_size;
 
 	/* parse MIPI blocks only if LFP type is MIPI */
 	if (!dev_priv->vbt.has_mipi)
@@ -820,8 +816,22 @@ parse_mipi(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
 
 	/* We have mandatory mipi config blocks. Initialize as generic panel */
 	dev_priv->vbt.dsi.panel_id = MIPI_DSI_GENERIC_PANEL_ID;
+}
+
+static void
+parse_mipi_sequence(struct drm_i915_private *dev_priv,
+		    const struct bdb_header *bdb)
+{
+	const struct bdb_mipi_sequence *sequence;
+	const u8 *seq_data;
+	u8 *data;
+	u16 block_size;
+	int i, panel_id, seq_size;
+
+	/* Only our generic panel driver uses the sequence block. */
+	if (dev_priv->vbt.dsi.panel_id != MIPI_DSI_GENERIC_PANEL_ID)
+		return;
 
-	/* Check if we have sequence block as well */
 	sequence = find_section(bdb, BDB_MIPI_SEQUENCE);
 	if (!sequence) {
 		DRM_DEBUG_KMS("No MIPI Sequence found, parsing complete\n");
@@ -1359,7 +1369,8 @@ intel_bios_init(struct drm_i915_private *dev_priv)
 	parse_driver_features(dev_priv, bdb);
 	parse_edp(dev_priv, bdb);
 	parse_psr(dev_priv, bdb);
-	parse_mipi(dev_priv, bdb);
+	parse_mipi_config(dev_priv, bdb);
+	parse_mipi_sequence(dev_priv, bdb);
 	parse_ddi_ports(dev_priv, bdb);
 
 	if (bios)
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/15] drm/i915/bios: have get_blocksize() support MIPI sequence block v3+

[Jani Nikula]

Have get_blocksize() support the special case of MIPI sequence block v3+
which has a separate field for size. Provide and use abstractions for
getting the blocksize given a pointer to the block "envelope",
i.e. pointer to the block id, and given a pointer to the block payload
data.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 91540ab15e0b..7393596df37d 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -58,6 +58,22 @@
 
 static int panel_type;
 
+/* Get BDB block size given a pointer to Block ID. */
+static u32 _get_blocksize(const u8 *block_base)
+{
+	/* The MIPI Sequence Block v3+ has a separate size field. */
+	if (*block_base == BDB_MIPI_SEQUENCE && *(block_base + 3) >= 3)
+		return *((const u32 *)(block_base + 4));
+	else
+		return *((const u16 *)(block_base + 1));
+}
+
+/* Get BDB block size give a pointer to data after Block ID and Block Size. */
+static u32 get_blocksize(const void *block_data)
+{
+	return _get_blocksize(block_data - 3);
+}
+
 static const void *
 find_section(const void *_bdb, int section_id)
 {
@@ -74,14 +90,8 @@ find_section(const void *_bdb, int section_id)
 	/* walk the sections looking for section_id */
 	while (index + 3 < total) {
 		current_id = *(base + index);
-		index++;
-
-		current_size = *((const u16 *)(base + index));
-		index += 2;
-
-		/* The MIPI Sequence Block v3+ has a separate size field. */
-		if (current_id == BDB_MIPI_SEQUENCE && *(base + index) >= 3)
-			current_size = *((const u32 *)(base + index + 1));
+		current_size = _get_blocksize(base + index);
+		index += 3;
 
 		if (index + current_size > total)
 			return NULL;
@@ -95,16 +105,6 @@ find_section(const void *_bdb, int section_id)
 	return NULL;
 }
 
-static u16
-get_blocksize(const void *p)
-{
-	u16 *block_ptr, block_size;
-
-	block_ptr = (u16 *)((char *)p - 2);
-	block_size = *block_ptr;
-	return block_size;
-}
-
 static void
 fill_detail_timing_data(struct drm_display_mode *panel_fixed_mode,
 			const struct lvds_dvo_timing *dvo_timing)
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/15] drm/i915/bios: abstract finding the panel sequence block

[Jani Nikula]

Make the whole thing easier to read.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c | 76 +++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 7393596df37d..9511a5341562 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -697,7 +697,7 @@ parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
 	dev_priv->vbt.psr.tp2_tp3_wakeup_time = psr_table->tp2_tp3_wakeup_time;
 }
 
-static u8 *goto_next_sequence(u8 *data, int *size)
+static u8 *goto_next_sequence(u8 *data, u16 *size)
 {
 	u16 len;
 	int tmp = *size;
@@ -818,15 +818,52 @@ parse_mipi_config(struct drm_i915_private *dev_priv,
 	dev_priv->vbt.dsi.panel_id = MIPI_DSI_GENERIC_PANEL_ID;
 }
 
+/* Find the sequence block and size for the given panel. */
+static const u8 *
+find_panel_sequence_block(const struct bdb_mipi_sequence *sequence,
+			  u16 panel_id, u16 *seq_size)
+{
+	u32 total = get_blocksize(sequence);
+	const u8 *data = &sequence->data[0];
+	u8 current_id;
+	u16 current_size;
+	int index = 0;
+	int i;
+
+	for (i = 0; i < MAX_MIPI_CONFIGURATIONS && index + 3 < total; i++) {
+		current_id = *(data + index);
+		index++;
+
+		current_size = *((const u16 *)(data + index));
+		index += 2;
+
+		if (index + current_size > total) {
+			DRM_ERROR("Invalid sequence block\n");
+			return NULL;
+		}
+
+		if (current_id == panel_id) {
+			*seq_size = current_size;
+			return data + index;
+		}
+
+		index += current_size;
+	}
+
+	DRM_ERROR("Sequence block detected but no valid configuration\n");
+
+	return NULL;
+}
+
 static void
 parse_mipi_sequence(struct drm_i915_private *dev_priv,
 		    const struct bdb_header *bdb)
 {
 	const struct bdb_mipi_sequence *sequence;
 	const u8 *seq_data;
+	u16 seq_size;
 	u8 *data;
 	u16 block_size;
-	int i, panel_id, seq_size;
 
 	/* Only our generic panel driver uses the sequence block. */
 	if (dev_priv->vbt.dsi.panel_id != MIPI_DSI_GENERIC_PANEL_ID)
@@ -853,40 +890,11 @@ parse_mipi_sequence(struct drm_i915_private *dev_priv,
 	 */
 	dev_priv->vbt.dsi.seq_version = sequence->version;
 
-	seq_data = &sequence->data[0];
-
-	/*
-	 * 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));
-		if (panel_id == panel_type)
-			break;
-
-		/* skip the sequence including seq header of 3 bytes */
-		seq_data = seq_data + 3 + seq_size;
-		if ((seq_data - &sequence->data[0]) > block_size) {
-			DRM_ERROR("Sequence start is beyond sequence block size, corrupted sequence block\n");
-			return;
-		}
-	}
-
-	if (i == MAX_MIPI_CONFIGURATIONS) {
-		DRM_ERROR("Sequence block detected but no valid configuration\n");
+	seq_data = find_panel_sequence_block(sequence, panel_type, &seq_size);
+	if (!seq_data)
 		return;
-	}
-
-	/* check if found sequence is completely within the sequence block
-	 * just being paranoid */
-	if (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, seq_size, GFP_KERNEL);
 	if (!dev_priv->vbt.dsi.data)
 		return;
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 06/15] drm/i915/bios: rewrite sequence block parsing

[Jani Nikula]

Make everything a bit more readable and clear.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h   |   2 +-
 drivers/gpu/drm/i915/intel_bios.c | 158 +++++++++++++-------------------------
 drivers/gpu/drm/i915/intel_bios.h |   4 +-
 3 files changed, 57 insertions(+), 107 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9b82c4532893..07c4fc539680 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1487,7 +1487,7 @@ struct intel_vbt_data {
 		u8 seq_version;
 		u32 size;
 		u8 *data;
-		u8 *sequence[MIPI_SEQ_MAX];
+		const u8 *sequence[MIPI_SEQ_MAX];
 	} dsi;
 
 	int crt_ddc_pin;
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 9511a5341562..d6eaf32f33e5 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -697,73 +697,6 @@ parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
 	dev_priv->vbt.psr.tp2_tp3_wakeup_time = psr_table->tp2_tp3_wakeup_time;
 }
 
-static u8 *goto_next_sequence(u8 *data, u16 *size)
-{
-	u16 len;
-	int tmp = *size;
-
-	if (--tmp < 0)
-		return NULL;
-
-	/* goto first element */
-	data++;
-	while (1) {
-		switch (*data) {
-		case MIPI_SEQ_ELEM_SEND_PKT:
-			/*
-			 * skip by this element payload size
-			 * skip elem id, command flag and data type
-			 */
-			tmp -= 5;
-			if (tmp < 0)
-				return NULL;
-
-			data += 3;
-			len = *((u16 *)data);
-
-			tmp -= len;
-			if (tmp < 0)
-				return NULL;
-
-			/* skip by len */
-			data = data + 2 + len;
-			break;
-		case MIPI_SEQ_ELEM_DELAY:
-			/* skip by elem id, and delay is 4 bytes */
-			tmp -= 5;
-			if (tmp < 0)
-				return NULL;
-
-			data += 5;
-			break;
-		case MIPI_SEQ_ELEM_GPIO:
-			tmp -= 3;
-			if (tmp < 0)
-				return NULL;
-
-			data += 3;
-			break;
-		default:
-			DRM_ERROR("Unknown element\n");
-			return NULL;
-		}
-
-		/* end of sequence ? */
-		if (*data == 0)
-			break;
-	}
-
-	/* goto next sequence or end of block byte */
-	if (--tmp < 0)
-		return NULL;
-
-	data++;
-
-	/* update amount of data left for the sequence block to be parsed */
-	*size = tmp;
-	return data;
-}
-
 static void
 parse_mipi_config(struct drm_i915_private *dev_priv,
 		  const struct bdb_header *bdb)
@@ -855,6 +788,39 @@ find_panel_sequence_block(const struct bdb_mipi_sequence *sequence,
 	return NULL;
 }
 
+static int goto_next_sequence(const u8 *data, int index, int total)
+{
+	u16 len;
+
+	/* Skip Sequence Byte. */
+	for (index = index + 1; index < total; index += len) {
+		u8 operation_byte = *(data + index);
+		index++;
+
+		switch (operation_byte) {
+		case MIPI_SEQ_ELEM_END:
+			return index;
+		case MIPI_SEQ_ELEM_SEND_PKT:
+			if (index + 4 > total)
+				return 0;
+
+			len = *((const u16 *)(data + index + 2)) + 4;
+			break;
+		case MIPI_SEQ_ELEM_DELAY:
+			len = 4;
+			break;
+		case MIPI_SEQ_ELEM_GPIO:
+			len = 2;
+			break;
+		default:
+			DRM_ERROR("Unknown operation byte\n");
+			return 0;
+		}
+	}
+
+	return 0;
+}
+
 static void
 parse_mipi_sequence(struct drm_i915_private *dev_priv,
 		    const struct bdb_header *bdb)
@@ -863,7 +829,7 @@ parse_mipi_sequence(struct drm_i915_private *dev_priv,
 	const u8 *seq_data;
 	u16 seq_size;
 	u8 *data;
-	u16 block_size;
+	int index = 0;
 
 	/* Only our generic panel driver uses the sequence block. */
 	if (dev_priv->vbt.dsi.panel_id != MIPI_DSI_GENERIC_PANEL_ID)
@@ -883,59 +849,43 @@ parse_mipi_sequence(struct drm_i915_private *dev_priv,
 
 	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 = find_panel_sequence_block(sequence, panel_type, &seq_size);
 	if (!seq_data)
 		return;
 
-	dev_priv->vbt.dsi.data = kmemdup(seq_data, seq_size, GFP_KERNEL);
-	if (!dev_priv->vbt.dsi.data)
+	data = kmemdup(seq_data, seq_size, GFP_KERNEL);
+	if (!data)
 		return;
 
-	/*
-	 * 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;
+	/* Parse the sequences, store pointers to each sequence. */
+	for (;;) {
+		u8 seq_id = *(data + index);
+		if (seq_id == MIPI_SEQ_END)
+			break;
 
-	/* two consecutive 0x00 indicate end of all sequences */
-	while (1) {
-		int seq_id = *data;
-		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");
+		if (seq_id >= MIPI_SEQ_MAX) {
+			DRM_ERROR("Unknown sequence %u\n", seq_id);
 			goto err;
 		}
 
-		/* partial parsing to skip elements */
-		data = goto_next_sequence(data, &seq_size);
+		dev_priv->vbt.dsi.sequence[seq_id] = data + index;
 
-		if (data == NULL) {
-			DRM_ERROR("Sequence elements going beyond block itself. Sequence block parsing failed\n");
+		index = goto_next_sequence(data, index, seq_size);
+		if (!index) {
+			DRM_ERROR("Invalid sequence %u\n", seq_id);
 			goto err;
 		}
-
-		if (*data == 0)
-			break; /* end of sequence reached */
 	}
 
-	DRM_DEBUG_DRIVER("MIPI related vbt parsing complete\n");
+	dev_priv->vbt.dsi.data = data;
+	dev_priv->vbt.dsi.size = seq_size;
+	dev_priv->vbt.dsi.seq_version = sequence->version;
+
+	DRM_DEBUG_DRIVER("MIPI related VBT parsing complete\n");
 	return;
-err:
-	kfree(dev_priv->vbt.dsi.data);
-	dev_priv->vbt.dsi.data = NULL;
 
-	/* error during parsing so set all pointers to null
-	 * because of partial parsing */
+err:
+	kfree(data);
 	memset(dev_priv->vbt.dsi.sequence, 0, sizeof(dev_priv->vbt.dsi.sequence));
 }
 
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 21c162e01189..4e87df16e7b3 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -954,7 +954,7 @@ struct bdb_mipi_sequence {
 
 /* MIPI Sequnece Block definitions */
 enum mipi_seq {
-	MIPI_SEQ_UNDEFINED = 0,
+	MIPI_SEQ_END = 0,
 	MIPI_SEQ_ASSERT_RESET,
 	MIPI_SEQ_INIT_OTP,
 	MIPI_SEQ_DISPLAY_ON,
@@ -964,7 +964,7 @@ enum mipi_seq {
 };
 
 enum mipi_seq_element {
-	MIPI_SEQ_ELEM_UNDEFINED = 0,
+	MIPI_SEQ_ELEM_END = 0,
 	MIPI_SEQ_ELEM_SEND_PKT,
 	MIPI_SEQ_ELEM_DELAY,
 	MIPI_SEQ_ELEM_GPIO,
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/15] drm/i915/dsi: be defensive about out of bounds sequence id

[Jani Nikula]

Untie the VBT based generic panel driver from the VBT parsing, so that
the two don't have to be updated in lockstep.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index a5e99ac305da..45512e0df57a 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -246,14 +246,21 @@ static const fn_mipi_elem_exec exec_elem[] = {
  */
 
 static const char * const seq_name[] = {
-	"UNDEFINED",
-	"MIPI_SEQ_ASSERT_RESET",
-	"MIPI_SEQ_INIT_OTP",
-	"MIPI_SEQ_DISPLAY_ON",
-	"MIPI_SEQ_DISPLAY_OFF",
-	"MIPI_SEQ_DEASSERT_RESET"
+	[MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET",
+	[MIPI_SEQ_INIT_OTP] = "MIPI_SEQ_INIT_OTP",
+	[MIPI_SEQ_DISPLAY_ON] = "MIPI_SEQ_DISPLAY_ON",
+	[MIPI_SEQ_DISPLAY_OFF]  = "MIPI_SEQ_DISPLAY_OFF",
+	[MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET",
 };
 
+static const char *sequence_name(enum mipi_seq seq_id)
+{
+	if (seq_id < ARRAY_SIZE(seq_name) && seq_name[seq_id])
+		return seq_name[seq_id];
+	else
+		return "(unknown)";
+}
+
 static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
 {
 	fn_mipi_elem_exec mipi_elem_exec;
@@ -262,7 +269,8 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
 	if (!data)
 		return;
 
-	DRM_DEBUG_DRIVER("Starting MIPI sequence - %s\n", seq_name[*data]);
+	DRM_DEBUG_DRIVER("Starting MIPI sequence %u - %s\n",
+			 *data, sequence_name(*data));
 
 	/* go to the first element of the sequence */
 	data++;
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/15] drm/i915/dsi: be defensive about out of bounds operation byte

[Jani Nikula]

Untie the VBT based generic panel driver from the VBT parsing, so that
the two don't have to be updated in lockstep.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 45512e0df57a..ba5355506590 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -232,11 +232,9 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
 typedef const u8 * (*fn_mipi_elem_exec)(struct intel_dsi *intel_dsi,
 					const u8 *data);
 static const fn_mipi_elem_exec exec_elem[] = {
-	NULL, /* reserved */
-	mipi_exec_send_packet,
-	mipi_exec_delay,
-	mipi_exec_gpio,
-	NULL, /* status read; later */
+	[MIPI_SEQ_ELEM_SEND_PKT] = mipi_exec_send_packet,
+	[MIPI_SEQ_ELEM_DELAY] = mipi_exec_delay,
+	[MIPI_SEQ_ELEM_GPIO] = mipi_exec_gpio,
 };
 
 /*
@@ -264,7 +262,6 @@ static const char *sequence_name(enum mipi_seq seq_id)
 static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
 {
 	fn_mipi_elem_exec mipi_elem_exec;
-	int index;
 
 	if (!data)
 		return;
@@ -277,15 +274,14 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
 
 	/* parse each byte till we reach end of sequence byte - 0x00 */
 	while (1) {
-		index = *data;
-		mipi_elem_exec = exec_elem[index];
-		if (!mipi_elem_exec) {
-			DRM_ERROR("Unsupported MIPI element, skipping sequence execution\n");
+		u8 operation_byte = *data++;
+		if (operation_byte >= ARRAY_SIZE(exec_elem) ||
+		    !exec_elem[operation_byte]) {
+			DRM_ERROR("Unsupported MIPI operation byte %u\n",
+				  operation_byte);
 			return;
 		}
-
-		/* goto element payload */
-		data++;
+		mipi_elem_exec = exec_elem[operation_byte];
 
 		/* execute the element specific rotines */
 		data = mipi_elem_exec(intel_dsi, data);
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/15] drm/i915/bios: interpret the i2c element

[Jani Nikula]

Add parsing of the i2c element, defined in MIPI sequence block v2. Drop
the status operation byte while at it, that does not exist.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c | 5 +++++
 drivers/gpu/drm/i915/intel_bios.h | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index d6eaf32f33e5..45a7a2bc96c6 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -812,6 +812,11 @@ static int goto_next_sequence(const u8 *data, int index, int total)
 		case MIPI_SEQ_ELEM_GPIO:
 			len = 2;
 			break;
+		case MIPI_SEQ_ELEM_I2C:
+			if (index + 7 > total)
+				return 0;
+			len = *(data + index + 6) + 7;
+			break;
 		default:
 			DRM_ERROR("Unknown operation byte\n");
 			return 0;
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 4e87df16e7b3..411b33794536 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -968,7 +968,7 @@ enum mipi_seq_element {
 	MIPI_SEQ_ELEM_SEND_PKT,
 	MIPI_SEQ_ELEM_DELAY,
 	MIPI_SEQ_ELEM_GPIO,
-	MIPI_SEQ_ELEM_STATUS,
+	MIPI_SEQ_ELEM_I2C,		/* sequence block v2+ */
 	MIPI_SEQ_ELEM_MAX
 };
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/15] drm/i915/bios: add sequences for MIPI sequence block v2

[Jani Nikula]

Properly parse the new sequences added in MIPI sequence block v2.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 411b33794536..6146f1b0cf48 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -960,6 +960,9 @@ enum mipi_seq {
 	MIPI_SEQ_DISPLAY_ON,
 	MIPI_SEQ_DISPLAY_OFF,
 	MIPI_SEQ_DEASSERT_RESET,
+	MIPI_SEQ_BACKLIGHT_ON,		/* sequence block v2+ */
+	MIPI_SEQ_BACKLIGHT_OFF,		/* sequence block v2+ */
+	MIPI_SEQ_TEAR_ON,		/* sequence block v2+ */
 	MIPI_SEQ_MAX
 };
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/15] drm/i915: Adding the parsing logic for the i2c element

[Jani Nikula]

From: vkorjani <vikas.korjani@intel.com>

New sequence element for i2c is been added in the
mipi sequence block of the VBT. This patch parses
and executes the i2c sequence.

v2: Add i2c_put_adapter call(Jani), rebase

v3: corrected the retry loop(Jani), rebase

v4 by Jani:
 - don't put the adapter if get fails
 - print an error message if all retries exhausted
 - use a for loop
 - fix warnings for unused variables

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: vkorjani <vikas.korjani@intel.com>
Signed-off-by: Deepak M <m.deepak@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 58 ++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index ba5355506590..8fcfb0f63dc1 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -31,6 +31,7 @@
 #include <drm/drm_panel.h>
 #include <linux/slab.h>
 #include <video/mipi_display.h>
+#include <linux/i2c.h>
 #include <asm/intel-mid.h>
 #include <video/mipi_display.h>
 #include "i915_drv.h"
@@ -104,6 +105,62 @@ static struct gpio_table gtable[] = {
 	{ GPIO_NC_11_PCONF0, GPIO_NC_11_PAD, 0}
 };
 
+static const u8 *mipi_exec_i2c(struct intel_dsi *intel_dsi, const u8 *data)
+{
+	struct i2c_adapter *adapter;
+	int ret, i;
+	u8 reg_offset, payload_size;
+	struct i2c_msg msg;
+	u8 *transmit_buffer;
+	u8 flag, bus_number;
+	u16 slave_add;
+
+	flag = *data++;
+	data++; /* index, unused */
+	bus_number = *data++;
+	slave_add = *(u16 *)(data);
+	data += 2;
+	reg_offset = *data++;
+	payload_size = *data++;
+
+	adapter = i2c_get_adapter(bus_number);
+	if (!adapter) {
+		DRM_ERROR("i2c_get_adapter(%u)\n", bus_number);
+		goto out;
+	}
+
+	transmit_buffer = kmalloc(1 + payload_size, GFP_TEMPORARY);
+	if (!transmit_buffer)
+		goto out_put;
+
+	transmit_buffer[0] = reg_offset;
+	memcpy(&transmit_buffer[1], data, payload_size);
+
+	msg.addr = slave_add;
+	msg.flags = 0;
+	msg.len = payload_size + 1;
+	msg.buf = &transmit_buffer[0];
+
+	for (i = 0; i < 6; i++) {
+		ret = i2c_transfer(adapter, &msg, 1);
+		if (ret == 1) {
+			goto out_free;
+		} else if (ret == -EAGAIN) {
+			usleep_range(1000, 2500);
+		} else {
+			break;
+		}
+	}
+
+	DRM_ERROR("i2c transfer failed: %d\n", ret);
+out_free:
+	kfree(transmit_buffer);
+out_put:
+	i2c_put_adapter(adapter);
+out:
+	return data + payload_size;
+}
+
 static inline enum port intel_dsi_seq_port_to_port(u8 port)
 {
 	return port ? PORT_C : PORT_A;
@@ -235,6 +292,7 @@ static const fn_mipi_elem_exec exec_elem[] = {
 	[MIPI_SEQ_ELEM_SEND_PKT] = mipi_exec_send_packet,
 	[MIPI_SEQ_ELEM_DELAY] = mipi_exec_delay,
 	[MIPI_SEQ_ELEM_GPIO] = mipi_exec_gpio,
+	[MIPI_SEQ_ELEM_I2C] = mipi_exec_i2c,
 };
 
 /*
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 12/15] drm/i915/bios: add defines for v3 sequence block

[Jani Nikula]

New sequences, new operations within sequences.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 6146f1b0cf48..350d4e0f75a4 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -963,6 +963,9 @@ enum mipi_seq {
 	MIPI_SEQ_BACKLIGHT_ON,		/* sequence block v2+ */
 	MIPI_SEQ_BACKLIGHT_OFF,		/* sequence block v2+ */
 	MIPI_SEQ_TEAR_ON,		/* sequence block v2+ */
+	MIPI_SEQ_TEAR_OFF,		/* sequence block v3+ */
+	MIPI_SEQ_POWER_ON,		/* sequence block v3+ */
+	MIPI_SEQ_POWER_OFF,		/* sequence block v3+ */
 	MIPI_SEQ_MAX
 };
 
@@ -972,6 +975,8 @@ enum mipi_seq_element {
 	MIPI_SEQ_ELEM_DELAY,
 	MIPI_SEQ_ELEM_GPIO,
 	MIPI_SEQ_ELEM_I2C,		/* sequence block v2+ */
+	MIPI_SEQ_ELEM_SPI,		/* sequence block v3+ */
+	MIPI_SEQ_ELEM_PMIC,		/* sequence block v3+ */
 	MIPI_SEQ_ELEM_MAX
 };
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 13/15] drm/i915/bios: add support for MIPI sequence block v3

[Jani Nikula]

The changes since the sequence block v2 are:

* The whole MIPI bios data block has a separate 32-bit size field since
  v3, stored after the version. This facilitates big sequences.

* The size of the panel specific sequence blocks has grown to 32
  bits. This facilitates big sequences.

* The elements within sequences now have an 8-bit size field following
  the operation byte. This facilitates skipping unknown new operation
  bytes, i.e. forward compatibility.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c          | 84 ++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c |  9 ++++
 2 files changed, 84 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 45a7a2bc96c6..f3c93bb0e3a7 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -754,21 +754,30 @@ parse_mipi_config(struct drm_i915_private *dev_priv,
 /* Find the sequence block and size for the given panel. */
 static const u8 *
 find_panel_sequence_block(const struct bdb_mipi_sequence *sequence,
-			  u16 panel_id, u16 *seq_size)
+			  u16 panel_id, u32 *seq_size)
 {
 	u32 total = get_blocksize(sequence);
 	const u8 *data = &sequence->data[0];
 	u8 current_id;
-	u16 current_size;
+	u32 current_size;
 	int index = 0;
 	int i;
 
+	/* skip new block size */
+	if (sequence->version >= 3)
+		data += 4;
+
 	for (i = 0; i < MAX_MIPI_CONFIGURATIONS && index + 3 < total; i++) {
 		current_id = *(data + index);
 		index++;
 
-		current_size = *((const u16 *)(data + index));
-		index += 2;
+		if (sequence->version >= 3) {
+			current_size = *((const u32 *)(data + index));
+			index += 4;
+		} else {
+			current_size = *((const u16 *)(data + index));
+			index += 2;
+		}
 
 		if (index + current_size > total) {
 			DRM_ERROR("Invalid sequence block\n");
@@ -826,13 +835,66 @@ static int goto_next_sequence(const u8 *data, int index, int total)
 	return 0;
 }
 
+static int goto_next_sequence_v3(const u8 *data, int index, int total)
+{
+	int seq_end;
+	u16 len;
+
+	/*
+	 * Could skip sequence based on Size of Sequence alone, but also do some
+	 * checking on the structure.
+	 */
+	seq_end = index + *((const u32 *)(data + 1));
+	if (seq_end > total) {
+		DRM_ERROR("Invalid sequence size\n");
+		return 0;
+	}
+
+	/* Skip Sequence Byte and Size of Sequence. */
+	for (index = index + 5; index < total; index += len) {
+		u8 operation_byte = *(data + index);
+		index++;
+
+		if (operation_byte == MIPI_SEQ_ELEM_END) {
+			if (index != seq_end) {
+				DRM_ERROR("Invalid element structure\n");
+				return 0;
+			}
+			return index;
+		}
+
+		len = *(data + index);
+		index++;
+
+		/*
+		 * FIXME: Would be nice to check elements like for v1/v2 in
+		 * goto_next_sequence() above.
+		 */
+		switch (operation_byte) {
+		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:
+			break;
+		default:
+			DRM_DEBUG_KMS("Unknown operation byte %u\n",
+				      operation_byte);
+			break;
+		}
+	}
+
+	return 0;
+}
+
 static void
 parse_mipi_sequence(struct drm_i915_private *dev_priv,
 		    const struct bdb_header *bdb)
 {
 	const struct bdb_mipi_sequence *sequence;
 	const u8 *seq_data;
-	u16 seq_size;
+	u32 seq_size;
 	u8 *data;
 	int index = 0;
 
@@ -847,12 +909,13 @@ parse_mipi_sequence(struct drm_i915_private *dev_priv,
 	}
 
 	/* Fail gracefully for forward incompatible sequence block. */
-	if (sequence->version >= 3) {
-		DRM_ERROR("Unable to parse MIPI Sequence Block v3+\n");
+	if (sequence->version >= 4) {
+		DRM_ERROR("Unable to parse MIPI Sequence Block v%u\n",
+			  sequence->version);
 		return;
 	}
 
-	DRM_DEBUG_DRIVER("Found MIPI sequence block\n");
+	DRM_DEBUG_DRIVER("Found MIPI sequence block v%u\n", sequence->version);
 
 	seq_data = find_panel_sequence_block(sequence, panel_type, &seq_size);
 	if (!seq_data)
@@ -875,7 +938,10 @@ parse_mipi_sequence(struct drm_i915_private *dev_priv,
 
 		dev_priv->vbt.dsi.sequence[seq_id] = data + index;
 
-		index = goto_next_sequence(data, index, seq_size);
+		if (sequence->version >= 3)
+			index = goto_next_sequence_v3(data, index, seq_size);
+		else
+			index = goto_next_sequence(data, index, seq_size);
 		if (!index) {
 			DRM_ERROR("Invalid sequence %u\n", seq_id);
 			goto err;
diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 8fcfb0f63dc1..eabfd9eb9cc0 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -319,6 +319,7 @@ static const char *sequence_name(enum mipi_seq seq_id)
 
 static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
 {
+	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
 	fn_mipi_elem_exec mipi_elem_exec;
 
 	if (!data)
@@ -330,6 +331,10 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
 	/* go to the first element of the sequence */
 	data++;
 
+	/* Skip Size of Sequence. */
+	if (dev_priv->vbt.dsi.seq_version >= 3)
+		data += 4;
+
 	/* parse each byte till we reach end of sequence byte - 0x00 */
 	while (1) {
 		u8 operation_byte = *data++;
@@ -341,6 +346,10 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
 		}
 		mipi_elem_exec = exec_elem[operation_byte];
 
+		/* Skip Size of Operation. */
+		if (dev_priv->vbt.dsi.seq_version >= 3)
+			data++;
+
 		/* execute the element specific rotines */
 		data = mipi_elem_exec(intel_dsi, data);
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 14/15] drm/i915/dsi: skip unknown elements for sequence block v3+

[Jani Nikula]

The sequence block has sizes of elements after the operation byte since
sequence block v3. Use it to skip elements we don't support yet.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 43 +++++++++++++++++-------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index eabfd9eb9cc0..1f9c80d21904 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -335,31 +335,36 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
 	if (dev_priv->vbt.dsi.seq_version >= 3)
 		data += 4;
 
-	/* parse each byte till we reach end of sequence byte - 0x00 */
 	while (1) {
 		u8 operation_byte = *data++;
-		if (operation_byte >= ARRAY_SIZE(exec_elem) ||
-		    !exec_elem[operation_byte]) {
+		u8 operation_size = 0;
+
+		if (operation_byte == MIPI_SEQ_ELEM_END)
+			break;
+
+		if (operation_byte < ARRAY_SIZE(exec_elem) &&
+		    exec_elem[operation_byte])
+			mipi_elem_exec = exec_elem[operation_byte];
+		else
+			mipi_elem_exec = NULL;
+
+		/* Size of Operation. */
+		if (dev_priv->vbt.dsi.seq_version >= 3)
+			operation_size = *data++;
+
+		if (mipi_elem_exec) {
+			data = mipi_elem_exec(intel_dsi, data);
+		} else if (operation_size) {
+			/* We have size, skip. */
+			DRM_DEBUG_KMS("Unsupported MIPI operation byte %u\n",
+				      operation_byte);
+			data += operation_size;
+		} else {
+			/* No size, can't skip without parsing. */
 			DRM_ERROR("Unsupported MIPI operation byte %u\n",
 				  operation_byte);
 			return;
 		}
-		mipi_elem_exec = exec_elem[operation_byte];
-
-		/* Skip Size of Operation. */
-		if (dev_priv->vbt.dsi.seq_version >= 3)
-			data++;
-
-		/* execute the element specific rotines */
-		data = mipi_elem_exec(intel_dsi, data);
-
-		/*
-		 * After processing the element, data should point to
-		 * next element or end of sequence
-		 * check if have we reached end of sequence
-		 */
-		if (*data == 0x00)
-			break;
 	}
 }
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 15/15] drm/i915/dsi: reduce tedious repetition

[Jani Nikula]

Make it a bit tidier.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 62 +++++++++++-------------------
 1 file changed, 22 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 1f9c80d21904..f0116a6c14cd 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -317,18 +317,30 @@ static const char *sequence_name(enum mipi_seq seq_id)
 		return "(unknown)";
 }
 
-static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
+static void generic_exec_sequence(struct drm_panel *panel, enum mipi_seq seq_id)
 {
+	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
+	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
 	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
+	const u8 *data;
 	fn_mipi_elem_exec mipi_elem_exec;
 
-	if (!data)
+	if (WARN_ON(seq_id >= ARRAY_SIZE(dev_priv->vbt.dsi.sequence)))
 		return;
 
-	DRM_DEBUG_DRIVER("Starting MIPI sequence %u - %s\n",
-			 *data, sequence_name(*data));
+	data = dev_priv->vbt.dsi.sequence[seq_id];
+	if (!data) {
+		DRM_DEBUG_KMS("MIPI sequence %d - %s not available\n",
+			      seq_id, sequence_name(seq_id));
+		return;
+	}
 
-	/* go to the first element of the sequence */
+	WARN_ON(*data != seq_id);
+
+	DRM_DEBUG_KMS("Starting MIPI sequence %d - %s\n",
+		      seq_id, sequence_name(seq_id));
+
+	/* Skip Sequence Byte. */
 	data++;
 
 	/* Skip Size of Sequence. */
@@ -370,59 +382,29 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
 
 static int vbt_panel_prepare(struct drm_panel *panel)
 {
-	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
-	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
-	struct drm_device *dev = intel_dsi->base.base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	const u8 *sequence;
-
-	sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_ASSERT_RESET];
-	generic_exec_sequence(intel_dsi, sequence);
-
-	sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP];
-	generic_exec_sequence(intel_dsi, sequence);
+	generic_exec_sequence(panel, MIPI_SEQ_ASSERT_RESET);
+	generic_exec_sequence(panel, MIPI_SEQ_INIT_OTP);
 
 	return 0;
 }
 
 static int vbt_panel_unprepare(struct drm_panel *panel)
 {
-	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
-	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
-	struct drm_device *dev = intel_dsi->base.base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	const u8 *sequence;
-
-	sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET];
-	generic_exec_sequence(intel_dsi, sequence);
+	generic_exec_sequence(panel, MIPI_SEQ_DEASSERT_RESET);
 
 	return 0;
 }
 
 static int vbt_panel_enable(struct drm_panel *panel)
 {
-	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
-	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
-	struct drm_device *dev = intel_dsi->base.base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	const u8 *sequence;
-
-	sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_ON];
-	generic_exec_sequence(intel_dsi, sequence);
+	generic_exec_sequence(panel, MIPI_SEQ_DISPLAY_ON);
 
 	return 0;
 }
 
 static int vbt_panel_disable(struct drm_panel *panel)
 {
-	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
-	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
-	struct drm_device *dev = intel_dsi->base.base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	const u8 *sequence;
-
-	sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_OFF];
-	generic_exec_sequence(intel_dsi, sequence);
+	generic_exec_sequence(panel, MIPI_SEQ_DISPLAY_OFF);
 
 	return 0;
 }
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ warning: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on e858593f63757a993fa56f282cb1493c57810a20 drm-intel-nightly: 2015y-12m-21d-12h-06m-20s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-render:
                dmesg-warn -> PASS       (skl-i5k-2)
                pass       -> DMESG-WARN (skl-i7k-2)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (ilk-hp8440p)
        Subgroup basic-flip-vs-modeset:
                dmesg-warn -> PASS       (skl-i5k-2)
                dmesg-warn -> PASS       (bsw-nuc-2)
                pass       -> DMESG-WARN (hsw-xps12)
                pass       -> DMESG-WARN (hsw-gt2)
                pass       -> DMESG-WARN (byt-nuc)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a:
                pass       -> DMESG-WARN (snb-x220t)
        Subgroup read-crc-pipe-b:
                dmesg-warn -> PASS       (snb-dellxps)
        Subgroup read-crc-pipe-b-frame-sequence:
                dmesg-warn -> PASS       (hsw-xps12)
Test kms_setmode:
        Subgroup basic-clone-single-crtc:
                pass       -> DMESG-WARN (snb-dellxps)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (bdw-ultra)
        Subgroup basic-rte:
                dmesg-warn -> PASS       (bdw-ultra)

bdw-nuci7        total:132  pass:122  dwarn:1   dfail:0   fail:0   skip:9  
bdw-ultra        total:132  pass:124  dwarn:2   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:135  pass:114  dwarn:1   dfail:0   fail:0   skip:20 
byt-nuc          total:135  pass:119  dwarn:3   dfail:0   fail:0   skip:13 
hsw-brixbox      total:135  pass:127  dwarn:1   dfail:0   fail:0   skip:7  
hsw-gt2          total:135  pass:129  dwarn:2   dfail:0   fail:0   skip:4  
hsw-xps12        total:132  pass:125  dwarn:3   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:135  pass:100  dwarn:0   dfail:0   fail:0   skip:35 
ivb-t430s        total:135  pass:127  dwarn:2   dfail:0   fail:0   skip:6  
skl-i5k-2        total:135  pass:123  dwarn:4   dfail:0   fail:0   skip:8  
skl-i7k-2        total:135  pass:122  dwarn:5   dfail:0   fail:0   skip:8  
snb-dellxps      total:135  pass:121  dwarn:2   dfail:0   fail:0   skip:12 
snb-x220t        total:135  pass:121  dwarn:2   dfail:0   fail:1   skip:11 

Results at /archive/results/CI_IGT_test/Patchwork_776/

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/15] drm/i915/bios: mipi sequence block v3, etc.

[Jani Nikula]

On Mon, 21 Dec 2015, Jani Nikula <jani.nikula@intel.com> wrote:
> Hi all, this is follow-up to my earlier opregion/vbt series [1]. This
> covers patches 1 and 7 of Deepak's series [2], but does quite a bunch of
> rework to make everything as neat as it can be given the spec. There's
> also some generic VBT documentation.

This also seems to enable display on Surface 3 tablet:
https://bugzilla.kernel.org/show_bug.cgi?id=97941

>
> Unfortunately I haven't been able to test this on a machine that
> actually has MIPI sequence block v3. However most of the series does not
> touch that yet.
>
> BR,
> Jani.
>
>
> [1] http://mid.gmane.org/cover.1450089383.git.jani.nikula@intel.com
> [2] http://mid.gmane.org/1448923632-16760-1-git-send-email-m.deepak@intel.com
>
> Jani Nikula (14):
>   drm/i915/bios: add proper documentation for the Video BIOS Table (VBT)
>   drm/i915/bios: fix header define name for intel_bios.h
>   drm/i915/bios: split the MIPI DSI VBT block parsing to two
>   drm/i915/bios: have get_blocksize() support MIPI sequence block v3+
>   drm/i915/bios: abstract finding the panel sequence block
>   drm/i915/bios: rewrite sequence block parsing
>   drm/i915/dsi: be defensive about out of bounds sequence id
>   drm/i915/dsi: be defensive about out of bounds operation byte
>   drm/i915/bios: interpret the i2c element
>   drm/i915/bios: add sequences for MIPI sequence block v2
>   drm/i915/bios: add defines for v3 sequence block
>   drm/i915/bios: add support for MIPI sequence block v3
>   drm/i915/dsi: skip unknown elements for sequence block v3+
>   drm/i915/dsi: reduce tedious repetition
>
> vkorjani (1):
>   drm/i915: Adding the parsing logic for the i2c element
>
>  Documentation/DocBook/gpu.tmpl             |   6 +
>  drivers/gpu/drm/i915/i915_drv.h            |   2 +-
>  drivers/gpu/drm/i915/intel_bios.c          | 376 +++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_bios.h          |  60 +++--
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 198 +++++++++------
>  5 files changed, 397 insertions(+), 245 deletions(-)

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

Re: [PATCH 01/15] drm/i915/bios: add proper documentation for the Video BIOS Table (VBT)

[Jani Nikula]

On Mon, 21 Dec 2015, Jani Nikula <jani.nikula@intel.com> wrote:
> Add an overview and documentation for the VBT/BDB header structures.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Pushed this one patch to drm-intel-next-queued with Daniel's irc
ack. Fixed a small grammatical error while at it (*a* BDB Header).

BR,
Jani.

> ---
>  Documentation/DocBook/gpu.tmpl    |  6 ++++++
>  drivers/gpu/drm/i915/intel_bios.c | 24 +++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_bios.h | 38 ++++++++++++++++++++++++++++----------
>  3 files changed, 57 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> index 6c6e81a9eaf4..f96a5d467f84 100644
> --- a/Documentation/DocBook/gpu.tmpl
> +++ b/Documentation/DocBook/gpu.tmpl
> @@ -3507,6 +3507,12 @@ int num_ioctls;</synopsis>
>  !Pdrivers/gpu/drm/i915/intel_csr.c csr support for dmc
>  !Idrivers/gpu/drm/i915/intel_csr.c
>        </sect2>
> +      <sect2>
> +	<title>Video BIOS Table (VBT)</title>
> +!Pdrivers/gpu/drm/i915/intel_bios.c Video BIOS Table (VBT)
> +!Idrivers/gpu/drm/i915/intel_bios.c
> +!Idrivers/gpu/drm/i915/intel_bios.h
> +      </sect2>
>      </sect1>
>  
>      <sect1>
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index eba3e0f87181..d487f602a10e 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -31,6 +31,28 @@
>  #include "i915_drv.h"
>  #include "intel_bios.h"
>  
> +/**
> + * DOC: Video BIOS Table (VBT)
> + *
> + * The Video BIOS Table, or VBT, provides platform and board specific
> + * configuration information to the driver that is not discoverable or available
> + * through other means. The configuration is mostly related to display
> + * hardware. The VBT is available via the ACPI OpRegion or, on older systems, in
> + * the PCI ROM.
> + *
> + * The VBT consists of a VBT Header (defined as &struct vbt_header), BDB Header
> + * (&struct bdb_header), and a number of BIOS Data Blocks (BDB) that contain the
> + * actual configuration information. The VBT Header, and thus the VBT, begins
> + * with "$VBT" signature. The VBT Header contains the offset of the BDB
> + * Header. The data blocks are concatenated after the BDB Header. The data
> + * blocks have a 1-byte Block ID, 2-byte Block Size, and Block Size bytes of
> + * data. (Block 53, the MIPI Sequence Block is an exception.)
> + *
> + * The driver parses the VBT during load. The relevant information is stored in
> + * driver private data for ease of use, and the actual VBT is not read after
> + * that.
> + */
> +
>  #define	SLAVE_ADDR1	0x70
>  #define	SLAVE_ADDR2	0x72
>  
> @@ -1285,7 +1307,7 @@ static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
>  
>  /**
>   * intel_bios_init - find VBT and initialize settings from the BIOS
> - * @dev: DRM device
> + * @dev_priv: i915 device instance
>   *
>   * Loads the Video BIOS and checks that the VBT exists.  Sets scratch registers
>   * to appropriate values.
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index 54eac1003a1e..2dc46a98c332 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -28,22 +28,40 @@
>  #ifndef _I830_BIOS_H_
>  #define _I830_BIOS_H_
>  
> +/**
> + * struct vbt_header - VBT Header structure
> + * @signature:		VBT signature, always starts with "$VBT"
> + * @version:		Version of this structure
> + * @header_size:	Size of this structure
> + * @vbt_size:		Size of VBT (VBT Header, BDB Header and data blocks)
> + * @vbt_checksum:	Checksum
> + * @reserved0:		Reserved
> + * @bdb_offset:		Offset of &struct bdb_header from beginning of VBT
> + * @aim_offset:		Offsets of add-in data blocks from beginning of VBT
> + */
>  struct vbt_header {
> -	u8 signature[20];		/**< Always starts with 'VBT$' */
> -	u16 version;			/**< decimal */
> -	u16 header_size;		/**< in bytes */
> -	u16 vbt_size;			/**< in bytes */
> +	u8 signature[20];
> +	u16 version;
> +	u16 header_size;
> +	u16 vbt_size;
>  	u8 vbt_checksum;
>  	u8 reserved0;
> -	u32 bdb_offset;			/**< from beginning of VBT */
> -	u32 aim_offset[4];		/**< from beginning of VBT */
> +	u32 bdb_offset;
> +	u32 aim_offset[4];
>  } __packed;
>  
> +/**
> + * struct bdb_header - BDB Header structure
> + * @signature:		BDB signature "BIOS_DATA_BLOCK"
> + * @version:		Version of the data block definitions
> + * @header_size:	Size of this structure
> + * @bdb_size:		Size of BDB (BDB Header and data blocks)
> + */
>  struct bdb_header {
> -	u8 signature[16];		/**< Always 'BIOS_DATA_BLOCK' */
> -	u16 version;			/**< decimal */
> -	u16 header_size;		/**< in bytes */
> -	u16 bdb_size;			/**< in bytes */
> +	u8 signature[16];
> +	u16 version;
> +	u16 header_size;
> +	u16 bdb_size;
>  } __packed;
>  
>  /* strictly speaking, this is a "skip" block, but it has interesting info */

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

Re: [PATCH 02/15] drm/i915/bios: fix header define name for intel_bios.h

[Daniel Vetter]

On Mon, Dec 21, 2015 at 03:10:53PM +0200, Jani Nikula wrote:
> Just for OCD.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_bios.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index 2dc46a98c332..21c162e01189 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -25,8 +25,8 @@
>   *
>   */
>  
> -#ifndef _I830_BIOS_H_
> -#define _I830_BIOS_H_
> +#ifndef _INTEL_BIOS_H_
> +#define _INTEL_BIOS_H_
>  
>  /**
>   * struct vbt_header - VBT Header structure
> @@ -983,4 +983,4 @@ enum mipi_gpio_pin_index {
>  	MIPI_GPIO_MAX
>  };
>  
> -#endif /* _I830_BIOS_H_ */
> +#endif /* _INTEL_BIOS_H_ */
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/15] drm/i915/bios: split the MIPI DSI VBT block parsing to two

[Daniel Vetter]

On Mon, Dec 21, 2015 at 03:10:54PM +0200, Jani Nikula wrote:
> There's two blocks to parse, have one function per block. The existing
> one cuts neatly into two.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_bios.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index d487f602a10e..91540ab15e0b 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -765,16 +765,12 @@ static u8 *goto_next_sequence(u8 *data, int *size)
>  }
>  
>  static void
> -parse_mipi(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
> +parse_mipi_config(struct drm_i915_private *dev_priv,
> +		  const struct bdb_header *bdb)
>  {
>  	const struct bdb_mipi_config *start;
> -	const struct bdb_mipi_sequence *sequence;
>  	const struct mipi_config *config;
>  	const struct mipi_pps_data *pps;
> -	u8 *data;
> -	const u8 *seq_data;
> -	int i, panel_id, seq_size;
> -	u16 block_size;
>  
>  	/* parse MIPI blocks only if LFP type is MIPI */
>  	if (!dev_priv->vbt.has_mipi)
> @@ -820,8 +816,22 @@ parse_mipi(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
>  
>  	/* We have mandatory mipi config blocks. Initialize as generic panel */
>  	dev_priv->vbt.dsi.panel_id = MIPI_DSI_GENERIC_PANEL_ID;
> +}
> +
> +static void
> +parse_mipi_sequence(struct drm_i915_private *dev_priv,
> +		    const struct bdb_header *bdb)
> +{
> +	const struct bdb_mipi_sequence *sequence;
> +	const u8 *seq_data;
> +	u8 *data;
> +	u16 block_size;
> +	int i, panel_id, seq_size;
> +
> +	/* Only our generic panel driver uses the sequence block. */
> +	if (dev_priv->vbt.dsi.panel_id != MIPI_DSI_GENERIC_PANEL_ID)
> +		return;
>  
> -	/* Check if we have sequence block as well */
>  	sequence = find_section(bdb, BDB_MIPI_SEQUENCE);
>  	if (!sequence) {
>  		DRM_DEBUG_KMS("No MIPI Sequence found, parsing complete\n");
> @@ -1359,7 +1369,8 @@ intel_bios_init(struct drm_i915_private *dev_priv)
>  	parse_driver_features(dev_priv, bdb);
>  	parse_edp(dev_priv, bdb);
>  	parse_psr(dev_priv, bdb);
> -	parse_mipi(dev_priv, bdb);
> +	parse_mipi_config(dev_priv, bdb);
> +	parse_mipi_sequence(dev_priv, bdb);
>  	parse_ddi_ports(dev_priv, bdb);
>  
>  	if (bios)
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/15] drm/i915/bios: have get_blocksize() support MIPI sequence block v3+

[Daniel Vetter]

On Mon, Dec 21, 2015 at 03:10:55PM +0200, Jani Nikula wrote:
> Have get_blocksize() support the special case of MIPI sequence block v3+
> which has a separate field for size. Provide and use abstractions for
> getting the blocksize given a pointer to the block "envelope",
> i.e. pointer to the block id, and given a pointer to the block payload
> data.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_bios.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 91540ab15e0b..7393596df37d 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -58,6 +58,22 @@
>  
>  static int panel_type;
>  
> +/* Get BDB block size given a pointer to Block ID. */
> +static u32 _get_blocksize(const u8 *block_base)
> +{
> +	/* The MIPI Sequence Block v3+ has a separate size field. */
> +	if (*block_base == BDB_MIPI_SEQUENCE && *(block_base + 3) >= 3)
> +		return *((const u32 *)(block_base + 4));
> +	else
> +		return *((const u16 *)(block_base + 1));
> +}
> +
> +/* Get BDB block size give a pointer to data after Block ID and Block Size. */
> +static u32 get_blocksize(const void *block_data)
> +{
> +	return _get_blocksize(block_data - 3);
> +}
> +
>  static const void *
>  find_section(const void *_bdb, int section_id)
>  {
> @@ -74,14 +90,8 @@ find_section(const void *_bdb, int section_id)
>  	/* walk the sections looking for section_id */
>  	while (index + 3 < total) {
>  		current_id = *(base + index);
> -		index++;
> -
> -		current_size = *((const u16 *)(base + index));
> -		index += 2;
> -
> -		/* The MIPI Sequence Block v3+ has a separate size field. */
> -		if (current_id == BDB_MIPI_SEQUENCE && *(base + index) >= 3)
> -			current_size = *((const u32 *)(base + index + 1));
> +		current_size = _get_blocksize(base + index);
> +		index += 3;
>  
>  		if (index + current_size > total)
>  			return NULL;
> @@ -95,16 +105,6 @@ find_section(const void *_bdb, int section_id)
>  	return NULL;
>  }
>  
> -static u16
> -get_blocksize(const void *p)
> -{
> -	u16 *block_ptr, block_size;
> -
> -	block_ptr = (u16 *)((char *)p - 2);
> -	block_size = *block_ptr;
> -	return block_size;
> -}
> -
>  static void
>  fill_detail_timing_data(struct drm_display_mode *panel_fixed_mode,
>  			const struct lvds_dvo_timing *dvo_timing)
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/15] drm/i915/bios: abstract finding the panel sequence block

[Daniel Vetter]

On Mon, Dec 21, 2015 at 03:10:56PM +0200, Jani Nikula wrote:
> Make the whole thing easier to read.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_bios.c | 76 +++++++++++++++++++++------------------
>  1 file changed, 42 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 7393596df37d..9511a5341562 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -697,7 +697,7 @@ parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
>  	dev_priv->vbt.psr.tp2_tp3_wakeup_time = psr_table->tp2_tp3_wakeup_time;
>  }
>  
> -static u8 *goto_next_sequence(u8 *data, int *size)
> +static u8 *goto_next_sequence(u8 *data, u16 *size)
>  {
>  	u16 len;
>  	int tmp = *size;
> @@ -818,15 +818,52 @@ parse_mipi_config(struct drm_i915_private *dev_priv,
>  	dev_priv->vbt.dsi.panel_id = MIPI_DSI_GENERIC_PANEL_ID;
>  }
>  
> +/* Find the sequence block and size for the given panel. */
> +static const u8 *
> +find_panel_sequence_block(const struct bdb_mipi_sequence *sequence,
> +			  u16 panel_id, u16 *seq_size)
> +{
> +	u32 total = get_blocksize(sequence);
> +	const u8 *data = &sequence->data[0];
> +	u8 current_id;
> +	u16 current_size;
> +	int index = 0;
> +	int i;
> +
> +	for (i = 0; i < MAX_MIPI_CONFIGURATIONS && index + 3 < total; i++) {

Commit message should mention that you make the parsin more robust and
ensure we don't walk over the end of the allocated buffer.

> +		current_id = *(data + index);
> +		index++;
> +
> +		current_size = *((const u16 *)(data + index));
> +		index += 2;
> +
> +		if (index + current_size > total) {
> +			DRM_ERROR("Invalid sequence block\n");
> +			return NULL;
> +		}
> +
> +		if (current_id == panel_id) {
> +			*seq_size = current_size;
> +			return data + index;
> +		}
> +
> +		index += current_size;
> +	}
> +
> +	DRM_ERROR("Sequence block detected but no valid configuration\n");
> +
> +	return NULL;
> +}
> +
>  static void
>  parse_mipi_sequence(struct drm_i915_private *dev_priv,
>  		    const struct bdb_header *bdb)
>  {
>  	const struct bdb_mipi_sequence *sequence;
>  	const u8 *seq_data;
> +	u16 seq_size;
>  	u8 *data;
>  	u16 block_size;
> -	int i, panel_id, seq_size;
>  
>  	/* Only our generic panel driver uses the sequence block. */
>  	if (dev_priv->vbt.dsi.panel_id != MIPI_DSI_GENERIC_PANEL_ID)
> @@ -853,40 +890,11 @@ parse_mipi_sequence(struct drm_i915_private *dev_priv,
>  	 */
>  	dev_priv->vbt.dsi.seq_version = sequence->version;
>  
> -	seq_data = &sequence->data[0];
> -
> -	/*
> -	 * 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));
> -		if (panel_id == panel_type)
> -			break;
> -
> -		/* skip the sequence including seq header of 3 bytes */
> -		seq_data = seq_data + 3 + seq_size;
> -		if ((seq_data - &sequence->data[0]) > block_size) {
> -			DRM_ERROR("Sequence start is beyond sequence block size, corrupted sequence block\n");
> -			return;
> -		}
> -	}
> -
> -	if (i == MAX_MIPI_CONFIGURATIONS) {
> -		DRM_ERROR("Sequence block detected but no valid configuration\n");
> +	seq_data = find_panel_sequence_block(sequence, panel_type, &seq_size);
> +	if (!seq_data)
>  		return;
> -	}
> -
> -	/* check if found sequence is completely within the sequence block
> -	 * just being paranoid */
> -	if (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, seq_size, GFP_KERNEL);

Should dropping the +3 be in a separate patch?

Otherwise looks good, with the above 2 addressed

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  	if (!dev_priv->vbt.dsi.data)
>  		return;
>  
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/15] drm/i915/bios: abstract finding the panel sequence block

[Jani Nikula]

On Tue, 05 Jan 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Dec 21, 2015 at 03:10:56PM +0200, Jani Nikula wrote:
>> Make the whole thing easier to read.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_bios.c | 76 +++++++++++++++++++++------------------
>>  1 file changed, 42 insertions(+), 34 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index 7393596df37d..9511a5341562 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -697,7 +697,7 @@ parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
>>  	dev_priv->vbt.psr.tp2_tp3_wakeup_time = psr_table->tp2_tp3_wakeup_time;
>>  }
>>  
>> -static u8 *goto_next_sequence(u8 *data, int *size)
>> +static u8 *goto_next_sequence(u8 *data, u16 *size)
>>  {
>>  	u16 len;
>>  	int tmp = *size;
>> @@ -818,15 +818,52 @@ parse_mipi_config(struct drm_i915_private *dev_priv,
>>  	dev_priv->vbt.dsi.panel_id = MIPI_DSI_GENERIC_PANEL_ID;
>>  }
>>  
>> +/* Find the sequence block and size for the given panel. */
>> +static const u8 *
>> +find_panel_sequence_block(const struct bdb_mipi_sequence *sequence,
>> +			  u16 panel_id, u16 *seq_size)
>> +{
>> +	u32 total = get_blocksize(sequence);
>> +	const u8 *data = &sequence->data[0];
>> +	u8 current_id;
>> +	u16 current_size;
>> +	int index = 0;
>> +	int i;
>> +
>> +	for (i = 0; i < MAX_MIPI_CONFIGURATIONS && index + 3 < total; i++) {
>
> Commit message should mention that you make the parsin more robust and
> ensure we don't walk over the end of the allocated buffer.

Agreed. Although it's implied in the Signed-off-by line. ;)

>
>> +		current_id = *(data + index);
>> +		index++;
>> +
>> +		current_size = *((const u16 *)(data + index));
>> +		index += 2;
>> +
>> +		if (index + current_size > total) {
>> +			DRM_ERROR("Invalid sequence block\n");
>> +			return NULL;
>> +		}
>> +
>> +		if (current_id == panel_id) {
>> +			*seq_size = current_size;
>> +			return data + index;
>> +		}
>> +
>> +		index += current_size;
>> +	}
>> +
>> +	DRM_ERROR("Sequence block detected but no valid configuration\n");
>> +
>> +	return NULL;
>> +}
>> +
>>  static void
>>  parse_mipi_sequence(struct drm_i915_private *dev_priv,
>>  		    const struct bdb_header *bdb)
>>  {
>>  	const struct bdb_mipi_sequence *sequence;
>>  	const u8 *seq_data;
>> +	u16 seq_size;
>>  	u8 *data;
>>  	u16 block_size;
>> -	int i, panel_id, seq_size;
>>  
>>  	/* Only our generic panel driver uses the sequence block. */
>>  	if (dev_priv->vbt.dsi.panel_id != MIPI_DSI_GENERIC_PANEL_ID)
>> @@ -853,40 +890,11 @@ parse_mipi_sequence(struct drm_i915_private *dev_priv,
>>  	 */
>>  	dev_priv->vbt.dsi.seq_version = sequence->version;
>>  
>> -	seq_data = &sequence->data[0];
>> -
>> -	/*
>> -	 * 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));
>> -		if (panel_id == panel_type)
>> -			break;
>> -
>> -		/* skip the sequence including seq header of 3 bytes */
>> -		seq_data = seq_data + 3 + seq_size;
>> -		if ((seq_data - &sequence->data[0]) > block_size) {
>> -			DRM_ERROR("Sequence start is beyond sequence block size, corrupted sequence block\n");
>> -			return;
>> -		}
>> -	}
>> -
>> -	if (i == MAX_MIPI_CONFIGURATIONS) {
>> -		DRM_ERROR("Sequence block detected but no valid configuration\n");
>> +	seq_data = find_panel_sequence_block(sequence, panel_type, &seq_size);
>> +	if (!seq_data)
>>  		return;
>> -	}
>> -
>> -	/* check if found sequence is completely within the sequence block
>> -	 * just being paranoid */
>> -	if (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, seq_size, GFP_KERNEL);
>
> Should dropping the +3 be in a separate patch?

Really I'd rather not if you don't mind. The end result is the same, but
I'd have to think the function over again just to add a throwaway
intermediate step. Unless I just replace the return statement with
"return data + index - 3" which would feel a bit silly, don't you think?

BR,
Jani.

>
> Otherwise looks good, with the above 2 addressed
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
>>  	if (!dev_priv->vbt.dsi.data)
>>  		return;
>>  
>> -- 
>> 2.1.4
>> 
>> _______________________________________________
>> 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

Re: [PATCH 04/15] drm/i915/bios: have get_blocksize() support MIPI sequence block v3+

[Jani Nikula]

On Tue, 05 Jan 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Dec 21, 2015 at 03:10:55PM +0200, Jani Nikula wrote:
>> Have get_blocksize() support the special case of MIPI sequence block v3+
>> which has a separate field for size. Provide and use abstractions for
>> getting the blocksize given a pointer to the block "envelope",
>> i.e. pointer to the block id, and given a pointer to the block payload
>> data.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Patches 2-4 pushed to drm-intel-next-queued, thanks for the review.

BR,
Jani.

>
>> ---
>>  drivers/gpu/drm/i915/intel_bios.c | 36 ++++++++++++++++++------------------
>>  1 file changed, 18 insertions(+), 18 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index 91540ab15e0b..7393596df37d 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -58,6 +58,22 @@
>>  
>>  static int panel_type;
>>  
>> +/* Get BDB block size given a pointer to Block ID. */
>> +static u32 _get_blocksize(const u8 *block_base)
>> +{
>> +	/* The MIPI Sequence Block v3+ has a separate size field. */
>> +	if (*block_base == BDB_MIPI_SEQUENCE && *(block_base + 3) >= 3)
>> +		return *((const u32 *)(block_base + 4));
>> +	else
>> +		return *((const u16 *)(block_base + 1));
>> +}
>> +
>> +/* Get BDB block size give a pointer to data after Block ID and Block Size. */
>> +static u32 get_blocksize(const void *block_data)
>> +{
>> +	return _get_blocksize(block_data - 3);
>> +}
>> +
>>  static const void *
>>  find_section(const void *_bdb, int section_id)
>>  {
>> @@ -74,14 +90,8 @@ find_section(const void *_bdb, int section_id)
>>  	/* walk the sections looking for section_id */
>>  	while (index + 3 < total) {
>>  		current_id = *(base + index);
>> -		index++;
>> -
>> -		current_size = *((const u16 *)(base + index));
>> -		index += 2;
>> -
>> -		/* The MIPI Sequence Block v3+ has a separate size field. */
>> -		if (current_id == BDB_MIPI_SEQUENCE && *(base + index) >= 3)
>> -			current_size = *((const u32 *)(base + index + 1));
>> +		current_size = _get_blocksize(base + index);
>> +		index += 3;
>>  
>>  		if (index + current_size > total)
>>  			return NULL;
>> @@ -95,16 +105,6 @@ find_section(const void *_bdb, int section_id)
>>  	return NULL;
>>  }
>>  
>> -static u16
>> -get_blocksize(const void *p)
>> -{
>> -	u16 *block_ptr, block_size;
>> -
>> -	block_ptr = (u16 *)((char *)p - 2);
>> -	block_size = *block_ptr;
>> -	return block_size;
>> -}
>> -
>>  static void
>>  fill_detail_timing_data(struct drm_display_mode *panel_fixed_mode,
>>  			const struct lvds_dvo_timing *dvo_timing)
>> -- 
>> 2.1.4
>> 
>> _______________________________________________
>> 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

[PATCH v2] drm/i915/bios: abstract finding the panel sequence block

[Jani Nikula]

Make the whole thing easier to read. While at it, make the parsing more
robust, and ensure we don't read past buffer being parsed.

v2: improve commit message (Daniel)

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c | 76 +++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 961ae7f6d075..422ba76700e3 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -697,7 +697,7 @@ parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
 	dev_priv->vbt.psr.tp2_tp3_wakeup_time = psr_table->tp2_tp3_wakeup_time;
 }
 
-static u8 *goto_next_sequence(u8 *data, int *size)
+static u8 *goto_next_sequence(u8 *data, u16 *size)
 {
 	u16 len;
 	int tmp = *size;
@@ -818,15 +818,52 @@ parse_mipi_config(struct drm_i915_private *dev_priv,
 	dev_priv->vbt.dsi.panel_id = MIPI_DSI_GENERIC_PANEL_ID;
 }
 
+/* Find the sequence block and size for the given panel. */
+static const u8 *
+find_panel_sequence_block(const struct bdb_mipi_sequence *sequence,
+			  u16 panel_id, u16 *seq_size)
+{
+	u32 total = get_blocksize(sequence);
+	const u8 *data = &sequence->data[0];
+	u8 current_id;
+	u16 current_size;
+	int index = 0;
+	int i;
+
+	for (i = 0; i < MAX_MIPI_CONFIGURATIONS && index + 3 < total; i++) {
+		current_id = *(data + index);
+		index++;
+
+		current_size = *((const u16 *)(data + index));
+		index += 2;
+
+		if (index + current_size > total) {
+			DRM_ERROR("Invalid sequence block\n");
+			return NULL;
+		}
+
+		if (current_id == panel_id) {
+			*seq_size = current_size;
+			return data + index;
+		}
+
+		index += current_size;
+	}
+
+	DRM_ERROR("Sequence block detected but no valid configuration\n");
+
+	return NULL;
+}
+
 static void
 parse_mipi_sequence(struct drm_i915_private *dev_priv,
 		    const struct bdb_header *bdb)
 {
 	const struct bdb_mipi_sequence *sequence;
 	const u8 *seq_data;
+	u16 seq_size;
 	u8 *data;
 	u16 block_size;
-	int i, panel_id, seq_size;
 
 	/* Only our generic panel driver uses the sequence block. */
 	if (dev_priv->vbt.dsi.panel_id != MIPI_DSI_GENERIC_PANEL_ID)
@@ -853,40 +890,11 @@ parse_mipi_sequence(struct drm_i915_private *dev_priv,
 	 */
 	dev_priv->vbt.dsi.seq_version = sequence->version;
 
-	seq_data = &sequence->data[0];
-
-	/*
-	 * 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));
-		if (panel_id == panel_type)
-			break;
-
-		/* skip the sequence including seq header of 3 bytes */
-		seq_data = seq_data + 3 + seq_size;
-		if ((seq_data - &sequence->data[0]) > block_size) {
-			DRM_ERROR("Sequence start is beyond sequence block size, corrupted sequence block\n");
-			return;
-		}
-	}
-
-	if (i == MAX_MIPI_CONFIGURATIONS) {
-		DRM_ERROR("Sequence block detected but no valid configuration\n");
+	seq_data = find_panel_sequence_block(sequence, panel_type, &seq_size);
+	if (!seq_data)
 		return;
-	}
-
-	/* check if found sequence is completely within the sequence block
-	 * just being paranoid */
-	if (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, seq_size, GFP_KERNEL);
 	if (!dev_priv->vbt.dsi.data)
 		return;
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/15] drm/i915/bios: abstract finding the panel sequence block

[Daniel Vetter]

On Tue, Jan 05, 2016 at 02:45:00PM +0200, Jani Nikula wrote:
> On Tue, 05 Jan 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Mon, Dec 21, 2015 at 03:10:56PM +0200, Jani Nikula wrote:
> >> Make the whole thing easier to read.
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_bios.c | 76 +++++++++++++++++++++------------------
> >>  1 file changed, 42 insertions(+), 34 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> >> index 7393596df37d..9511a5341562 100644
> >> --- a/drivers/gpu/drm/i915/intel_bios.c
> >> +++ b/drivers/gpu/drm/i915/intel_bios.c
> >> @@ -697,7 +697,7 @@ parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
> >>  	dev_priv->vbt.psr.tp2_tp3_wakeup_time = psr_table->tp2_tp3_wakeup_time;
> >>  }
> >>  
> >> -static u8 *goto_next_sequence(u8 *data, int *size)
> >> +static u8 *goto_next_sequence(u8 *data, u16 *size)
> >>  {
> >>  	u16 len;
> >>  	int tmp = *size;
> >> @@ -818,15 +818,52 @@ parse_mipi_config(struct drm_i915_private *dev_priv,
> >>  	dev_priv->vbt.dsi.panel_id = MIPI_DSI_GENERIC_PANEL_ID;
> >>  }
> >>  
> >> +/* Find the sequence block and size for the given panel. */
> >> +static const u8 *
> >> +find_panel_sequence_block(const struct bdb_mipi_sequence *sequence,
> >> +			  u16 panel_id, u16 *seq_size)
> >> +{
> >> +	u32 total = get_blocksize(sequence);
> >> +	const u8 *data = &sequence->data[0];
> >> +	u8 current_id;
> >> +	u16 current_size;
> >> +	int index = 0;
> >> +	int i;
> >> +
> >> +	for (i = 0; i < MAX_MIPI_CONFIGURATIONS && index + 3 < total; i++) {
> >
> > Commit message should mention that you make the parsin more robust and
> > ensure we don't walk over the end of the allocated buffer.
> 
> Agreed. Although it's implied in the Signed-off-by line. ;)
> 
> >
> >> +		current_id = *(data + index);
> >> +		index++;
> >> +
> >> +		current_size = *((const u16 *)(data + index));
> >> +		index += 2;
> >> +
> >> +		if (index + current_size > total) {
> >> +			DRM_ERROR("Invalid sequence block\n");
> >> +			return NULL;
> >> +		}
> >> +
> >> +		if (current_id == panel_id) {
> >> +			*seq_size = current_size;
> >> +			return data + index;
> >> +		}
> >> +
> >> +		index += current_size;
> >> +	}
> >> +
> >> +	DRM_ERROR("Sequence block detected but no valid configuration\n");
> >> +
> >> +	return NULL;
> >> +}
> >> +
> >>  static void
> >>  parse_mipi_sequence(struct drm_i915_private *dev_priv,
> >>  		    const struct bdb_header *bdb)
> >>  {
> >>  	const struct bdb_mipi_sequence *sequence;
> >>  	const u8 *seq_data;
> >> +	u16 seq_size;
> >>  	u8 *data;
> >>  	u16 block_size;
> >> -	int i, panel_id, seq_size;
> >>  
> >>  	/* Only our generic panel driver uses the sequence block. */
> >>  	if (dev_priv->vbt.dsi.panel_id != MIPI_DSI_GENERIC_PANEL_ID)
> >> @@ -853,40 +890,11 @@ parse_mipi_sequence(struct drm_i915_private *dev_priv,
> >>  	 */
> >>  	dev_priv->vbt.dsi.seq_version = sequence->version;
> >>  
> >> -	seq_data = &sequence->data[0];
> >> -
> >> -	/*
> >> -	 * 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));
> >> -		if (panel_id == panel_type)
> >> -			break;
> >> -
> >> -		/* skip the sequence including seq header of 3 bytes */
> >> -		seq_data = seq_data + 3 + seq_size;
> >> -		if ((seq_data - &sequence->data[0]) > block_size) {
> >> -			DRM_ERROR("Sequence start is beyond sequence block size, corrupted sequence block\n");
> >> -			return;
> >> -		}
> >> -	}
> >> -
> >> -	if (i == MAX_MIPI_CONFIGURATIONS) {
> >> -		DRM_ERROR("Sequence block detected but no valid configuration\n");
> >> +	seq_data = find_panel_sequence_block(sequence, panel_type, &seq_size);
> >> +	if (!seq_data)
> >>  		return;
> >> -	}
> >> -
> >> -	/* check if found sequence is completely within the sequence block
> >> -	 * just being paranoid */
> >> -	if (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, seq_size, GFP_KERNEL);
> >
> > Should dropping the +3 be in a separate patch?
> 
> Really I'd rather not if you don't mind. The end result is the same, but
> I'd have to think the function over again just to add a throwaway
> intermediate step. Unless I just replace the return statement with
> "return data + index - 3" which would feel a bit silly, don't you think?

I got confused with all the different stuff going on and thought you're
indeed chaning the pointer you pass to kmemdup. Checking again shows that
it looks correct.

On v2: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/15] drm/i915/bios: rewrite sequence block parsing

[Daniel Vetter]

On Mon, Dec 21, 2015 at 03:10:57PM +0200, Jani Nikula wrote:
> Make everything a bit more readable and clear.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

A real pain to check, but I think I convinced myself that this is
equivalent code.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


> ---
>  drivers/gpu/drm/i915/i915_drv.h   |   2 +-
>  drivers/gpu/drm/i915/intel_bios.c | 158 +++++++++++++-------------------------
>  drivers/gpu/drm/i915/intel_bios.h |   4 +-
>  3 files changed, 57 insertions(+), 107 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9b82c4532893..07c4fc539680 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1487,7 +1487,7 @@ struct intel_vbt_data {
>  		u8 seq_version;
>  		u32 size;
>  		u8 *data;
> -		u8 *sequence[MIPI_SEQ_MAX];
> +		const u8 *sequence[MIPI_SEQ_MAX];
>  	} dsi;
>  
>  	int crt_ddc_pin;
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 9511a5341562..d6eaf32f33e5 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -697,73 +697,6 @@ parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
>  	dev_priv->vbt.psr.tp2_tp3_wakeup_time = psr_table->tp2_tp3_wakeup_time;
>  }
>  
> -static u8 *goto_next_sequence(u8 *data, u16 *size)
> -{
> -	u16 len;
> -	int tmp = *size;
> -
> -	if (--tmp < 0)
> -		return NULL;
> -
> -	/* goto first element */
> -	data++;
> -	while (1) {
> -		switch (*data) {
> -		case MIPI_SEQ_ELEM_SEND_PKT:
> -			/*
> -			 * skip by this element payload size
> -			 * skip elem id, command flag and data type
> -			 */
> -			tmp -= 5;
> -			if (tmp < 0)
> -				return NULL;
> -
> -			data += 3;
> -			len = *((u16 *)data);
> -
> -			tmp -= len;
> -			if (tmp < 0)
> -				return NULL;
> -
> -			/* skip by len */
> -			data = data + 2 + len;
> -			break;
> -		case MIPI_SEQ_ELEM_DELAY:
> -			/* skip by elem id, and delay is 4 bytes */
> -			tmp -= 5;
> -			if (tmp < 0)
> -				return NULL;
> -
> -			data += 5;
> -			break;
> -		case MIPI_SEQ_ELEM_GPIO:
> -			tmp -= 3;
> -			if (tmp < 0)
> -				return NULL;
> -
> -			data += 3;
> -			break;
> -		default:
> -			DRM_ERROR("Unknown element\n");
> -			return NULL;
> -		}
> -
> -		/* end of sequence ? */
> -		if (*data == 0)
> -			break;
> -	}
> -
> -	/* goto next sequence or end of block byte */
> -	if (--tmp < 0)
> -		return NULL;
> -
> -	data++;
> -
> -	/* update amount of data left for the sequence block to be parsed */
> -	*size = tmp;
> -	return data;
> -}
> -
>  static void
>  parse_mipi_config(struct drm_i915_private *dev_priv,
>  		  const struct bdb_header *bdb)
> @@ -855,6 +788,39 @@ find_panel_sequence_block(const struct bdb_mipi_sequence *sequence,
>  	return NULL;
>  }
>  
> +static int goto_next_sequence(const u8 *data, int index, int total)
> +{
> +	u16 len;
> +
> +	/* Skip Sequence Byte. */
> +	for (index = index + 1; index < total; index += len) {
> +		u8 operation_byte = *(data + index);
> +		index++;
> +
> +		switch (operation_byte) {
> +		case MIPI_SEQ_ELEM_END:
> +			return index;
> +		case MIPI_SEQ_ELEM_SEND_PKT:
> +			if (index + 4 > total)
> +				return 0;
> +
> +			len = *((const u16 *)(data + index + 2)) + 4;
> +			break;
> +		case MIPI_SEQ_ELEM_DELAY:
> +			len = 4;
> +			break;
> +		case MIPI_SEQ_ELEM_GPIO:
> +			len = 2;
> +			break;
> +		default:
> +			DRM_ERROR("Unknown operation byte\n");
> +			return 0;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static void
>  parse_mipi_sequence(struct drm_i915_private *dev_priv,
>  		    const struct bdb_header *bdb)
> @@ -863,7 +829,7 @@ parse_mipi_sequence(struct drm_i915_private *dev_priv,
>  	const u8 *seq_data;
>  	u16 seq_size;
>  	u8 *data;
> -	u16 block_size;
> +	int index = 0;
>  
>  	/* Only our generic panel driver uses the sequence block. */
>  	if (dev_priv->vbt.dsi.panel_id != MIPI_DSI_GENERIC_PANEL_ID)
> @@ -883,59 +849,43 @@ parse_mipi_sequence(struct drm_i915_private *dev_priv,
>  
>  	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 = find_panel_sequence_block(sequence, panel_type, &seq_size);
>  	if (!seq_data)
>  		return;
>  
> -	dev_priv->vbt.dsi.data = kmemdup(seq_data, seq_size, GFP_KERNEL);
> -	if (!dev_priv->vbt.dsi.data)
> +	data = kmemdup(seq_data, seq_size, GFP_KERNEL);
> +	if (!data)
>  		return;
>  
> -	/*
> -	 * 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;
> +	/* Parse the sequences, store pointers to each sequence. */
> +	for (;;) {
> +		u8 seq_id = *(data + index);
> +		if (seq_id == MIPI_SEQ_END)
> +			break;
>  
> -	/* two consecutive 0x00 indicate end of all sequences */
> -	while (1) {
> -		int seq_id = *data;
> -		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");
> +		if (seq_id >= MIPI_SEQ_MAX) {
> +			DRM_ERROR("Unknown sequence %u\n", seq_id);
>  			goto err;
>  		}
>  
> -		/* partial parsing to skip elements */
> -		data = goto_next_sequence(data, &seq_size);
> +		dev_priv->vbt.dsi.sequence[seq_id] = data + index;
>  
> -		if (data == NULL) {
> -			DRM_ERROR("Sequence elements going beyond block itself. Sequence block parsing failed\n");
> +		index = goto_next_sequence(data, index, seq_size);
> +		if (!index) {
> +			DRM_ERROR("Invalid sequence %u\n", seq_id);
>  			goto err;
>  		}
> -
> -		if (*data == 0)
> -			break; /* end of sequence reached */
>  	}
>  
> -	DRM_DEBUG_DRIVER("MIPI related vbt parsing complete\n");
> +	dev_priv->vbt.dsi.data = data;
> +	dev_priv->vbt.dsi.size = seq_size;
> +	dev_priv->vbt.dsi.seq_version = sequence->version;
> +
> +	DRM_DEBUG_DRIVER("MIPI related VBT parsing complete\n");
>  	return;
> -err:
> -	kfree(dev_priv->vbt.dsi.data);
> -	dev_priv->vbt.dsi.data = NULL;
>  
> -	/* error during parsing so set all pointers to null
> -	 * because of partial parsing */
> +err:
> +	kfree(data);
>  	memset(dev_priv->vbt.dsi.sequence, 0, sizeof(dev_priv->vbt.dsi.sequence));
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index 21c162e01189..4e87df16e7b3 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -954,7 +954,7 @@ struct bdb_mipi_sequence {
>  
>  /* MIPI Sequnece Block definitions */
>  enum mipi_seq {
> -	MIPI_SEQ_UNDEFINED = 0,
> +	MIPI_SEQ_END = 0,
>  	MIPI_SEQ_ASSERT_RESET,
>  	MIPI_SEQ_INIT_OTP,
>  	MIPI_SEQ_DISPLAY_ON,
> @@ -964,7 +964,7 @@ enum mipi_seq {
>  };
>  
>  enum mipi_seq_element {
> -	MIPI_SEQ_ELEM_UNDEFINED = 0,
> +	MIPI_SEQ_ELEM_END = 0,
>  	MIPI_SEQ_ELEM_SEND_PKT,
>  	MIPI_SEQ_ELEM_DELAY,
>  	MIPI_SEQ_ELEM_GPIO,
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/15] drm/i915/dsi: be defensive about out of bounds sequence id

[Daniel Vetter]

On Mon, Dec 21, 2015 at 03:10:58PM +0200, Jani Nikula wrote:
> Untie the VBT based generic panel driver from the VBT parsing, so that
> the two don't have to be updated in lockstep.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index a5e99ac305da..45512e0df57a 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -246,14 +246,21 @@ static const fn_mipi_elem_exec exec_elem[] = {
>   */
>  
>  static const char * const seq_name[] = {
> -	"UNDEFINED",
> -	"MIPI_SEQ_ASSERT_RESET",
> -	"MIPI_SEQ_INIT_OTP",
> -	"MIPI_SEQ_DISPLAY_ON",
> -	"MIPI_SEQ_DISPLAY_OFF",
> -	"MIPI_SEQ_DEASSERT_RESET"
> +	[MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET",
> +	[MIPI_SEQ_INIT_OTP] = "MIPI_SEQ_INIT_OTP",
> +	[MIPI_SEQ_DISPLAY_ON] = "MIPI_SEQ_DISPLAY_ON",
> +	[MIPI_SEQ_DISPLAY_OFF]  = "MIPI_SEQ_DISPLAY_OFF",
> +	[MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET",
>  };
>  
> +static const char *sequence_name(enum mipi_seq seq_id)
> +{
> +	if (seq_id < ARRAY_SIZE(seq_name) && seq_name[seq_id])
> +		return seq_name[seq_id];
> +	else
> +		return "(unknown)";
> +}
> +
>  static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
>  {
>  	fn_mipi_elem_exec mipi_elem_exec;
> @@ -262,7 +269,8 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
>  	if (!data)
>  		return;
>  
> -	DRM_DEBUG_DRIVER("Starting MIPI sequence - %s\n", seq_name[*data]);
> +	DRM_DEBUG_DRIVER("Starting MIPI sequence %u - %s\n",
> +			 *data, sequence_name(*data));
>  
>  	/* go to the first element of the sequence */
>  	data++;
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/15] drm/i915/dsi: be defensive about out of bounds operation byte

[Daniel Vetter]

On Mon, Dec 21, 2015 at 03:10:59PM +0200, Jani Nikula wrote:
> Untie the VBT based generic panel driver from the VBT parsing, so that
> the two don't have to be updated in lockstep.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index 45512e0df57a..ba5355506590 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -232,11 +232,9 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>  typedef const u8 * (*fn_mipi_elem_exec)(struct intel_dsi *intel_dsi,
>  					const u8 *data);
>  static const fn_mipi_elem_exec exec_elem[] = {
> -	NULL, /* reserved */
> -	mipi_exec_send_packet,
> -	mipi_exec_delay,
> -	mipi_exec_gpio,
> -	NULL, /* status read; later */
> +	[MIPI_SEQ_ELEM_SEND_PKT] = mipi_exec_send_packet,
> +	[MIPI_SEQ_ELEM_DELAY] = mipi_exec_delay,
> +	[MIPI_SEQ_ELEM_GPIO] = mipi_exec_gpio,
>  };
>  
>  /*
> @@ -264,7 +262,6 @@ static const char *sequence_name(enum mipi_seq seq_id)
>  static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
>  {
>  	fn_mipi_elem_exec mipi_elem_exec;
> -	int index;
>  
>  	if (!data)
>  		return;
> @@ -277,15 +274,14 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
>  
>  	/* parse each byte till we reach end of sequence byte - 0x00 */
>  	while (1) {
> -		index = *data;
> -		mipi_elem_exec = exec_elem[index];
> -		if (!mipi_elem_exec) {
> -			DRM_ERROR("Unsupported MIPI element, skipping sequence execution\n");
> +		u8 operation_byte = *data++;
> +		if (operation_byte >= ARRAY_SIZE(exec_elem) ||
> +		    !exec_elem[operation_byte]) {
> +			DRM_ERROR("Unsupported MIPI operation byte %u\n",

Maybe DRM_ERROR in the previous patch too? Just for ocd consistency.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +				  operation_byte);
>  			return;
>  		}
> -
> -		/* goto element payload */
> -		data++;
> +		mipi_elem_exec = exec_elem[operation_byte];
>  
>  		/* execute the element specific rotines */
>  		data = mipi_elem_exec(intel_dsi, data);
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 14/15] drm/i915/dsi: skip unknown elements for sequence block v3+

[Daniel Vetter]

On Mon, Dec 21, 2015 at 03:11:05PM +0200, Jani Nikula wrote:
> The sequence block has sizes of elements after the operation byte since
> sequence block v3. Use it to skip elements we don't support yet.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 43 +++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index eabfd9eb9cc0..1f9c80d21904 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -335,31 +335,36 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
>  	if (dev_priv->vbt.dsi.seq_version >= 3)
>  		data += 4;
>  
> -	/* parse each byte till we reach end of sequence byte - 0x00 */
>  	while (1) {
>  		u8 operation_byte = *data++;
> -		if (operation_byte >= ARRAY_SIZE(exec_elem) ||
> -		    !exec_elem[operation_byte]) {
> +		u8 operation_size = 0;
> +
> +		if (operation_byte == MIPI_SEQ_ELEM_END)
> +			break;
> +
> +		if (operation_byte < ARRAY_SIZE(exec_elem) &&
> +		    exec_elem[operation_byte])

&& exec_elem[operation_byte] is redundant since you assing it to NULL
anyway in the else clause. While I bikeshed: Might look prettier with ?:


> +			mipi_elem_exec = exec_elem[operation_byte];
> +		else
> +			mipi_elem_exec = NULL;
> +
> +		/* Size of Operation. */
> +		if (dev_priv->vbt.dsi.seq_version >= 3)
> +			operation_size = *data++;
> +
> +		if (mipi_elem_exec) {
> +			data = mipi_elem_exec(intel_dsi, data);
> +		} else if (operation_size) {
> +			/* We have size, skip. */
> +			DRM_DEBUG_KMS("Unsupported MIPI operation byte %u\n",
> +				      operation_byte);

DRM_ERROR, like in the other cases we fail to parse the sequence fully?

Either way on both bikesheds: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


> +			data += operation_size;
> +		} else {
> +			/* No size, can't skip without parsing. */
>  			DRM_ERROR("Unsupported MIPI operation byte %u\n",
>  				  operation_byte);
>  			return;
>  		}
> -		mipi_elem_exec = exec_elem[operation_byte];
> -
> -		/* Skip Size of Operation. */
> -		if (dev_priv->vbt.dsi.seq_version >= 3)
> -			data++;
> -
> -		/* execute the element specific rotines */
> -		data = mipi_elem_exec(intel_dsi, data);
> -
> -		/*
> -		 * After processing the element, data should point to
> -		 * next element or end of sequence
> -		 * check if have we reached end of sequence
> -		 */
> -		if (*data == 0x00)
> -			break;
>  	}
>  }
>  
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 15/15] drm/i915/dsi: reduce tedious repetition

[Daniel Vetter]

On Mon, Dec 21, 2015 at 03:11:06PM +0200, Jani Nikula wrote:
> Make it a bit tidier.

Also more save.

> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 62 +++++++++++-------------------
>  1 file changed, 22 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index 1f9c80d21904..f0116a6c14cd 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -317,18 +317,30 @@ static const char *sequence_name(enum mipi_seq seq_id)
>  		return "(unknown)";
>  }
>  
> -static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
> +static void generic_exec_sequence(struct drm_panel *panel, enum mipi_seq seq_id)
>  {
> +	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
> +	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
>  	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
> +	const u8 *data;
>  	fn_mipi_elem_exec mipi_elem_exec;
>  
> -	if (!data)
> +	if (WARN_ON(seq_id >= ARRAY_SIZE(dev_priv->vbt.dsi.sequence)))
>  		return;
>  
> -	DRM_DEBUG_DRIVER("Starting MIPI sequence %u - %s\n",
> -			 *data, sequence_name(*data));
> +	data = dev_priv->vbt.dsi.sequence[seq_id];
> +	if (!data) {
> +		DRM_DEBUG_KMS("MIPI sequence %d - %s not available\n",
> +			      seq_id, sequence_name(seq_id));
> +		return;
> +	}
>  
> -	/* go to the first element of the sequence */
> +	WARN_ON(*data != seq_id);
> +
> +	DRM_DEBUG_KMS("Starting MIPI sequence %d - %s\n",
> +		      seq_id, sequence_name(seq_id));
> +
> +	/* Skip Sequence Byte. */
>  	data++;
>  
>  	/* Skip Size of Sequence. */
> @@ -370,59 +382,29 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
>  
>  static int vbt_panel_prepare(struct drm_panel *panel)
>  {
> -	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
> -	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
> -	struct drm_device *dev = intel_dsi->base.base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	const u8 *sequence;
> -
> -	sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_ASSERT_RESET];
> -	generic_exec_sequence(intel_dsi, sequence);
> -
> -	sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP];
> -	generic_exec_sequence(intel_dsi, sequence);
> +	generic_exec_sequence(panel, MIPI_SEQ_ASSERT_RESET);
> +	generic_exec_sequence(panel, MIPI_SEQ_INIT_OTP);
>  
>  	return 0;
>  }
>  
>  static int vbt_panel_unprepare(struct drm_panel *panel)
>  {
> -	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
> -	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
> -	struct drm_device *dev = intel_dsi->base.base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	const u8 *sequence;
> -
> -	sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET];
> -	generic_exec_sequence(intel_dsi, sequence);
> +	generic_exec_sequence(panel, MIPI_SEQ_DEASSERT_RESET);
>  
>  	return 0;
>  }
>  
>  static int vbt_panel_enable(struct drm_panel *panel)
>  {
> -	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
> -	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
> -	struct drm_device *dev = intel_dsi->base.base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	const u8 *sequence;
> -
> -	sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_ON];
> -	generic_exec_sequence(intel_dsi, sequence);
> +	generic_exec_sequence(panel, MIPI_SEQ_DISPLAY_ON);
>  
>  	return 0;
>  }
>  
>  static int vbt_panel_disable(struct drm_panel *panel)
>  {
> -	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
> -	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
> -	struct drm_device *dev = intel_dsi->base.base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	const u8 *sequence;
> -
> -	sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_OFF];
> -	generic_exec_sequence(intel_dsi, sequence);
> +	generic_exec_sequence(panel, MIPI_SEQ_DISPLAY_OFF);
>  
>  	return 0;
>  }
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/15] drm/i915/dsi: be defensive about out of bounds operation byte

[Jani Nikula]

On Tue, 05 Jan 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Dec 21, 2015 at 03:10:59PM +0200, Jani Nikula wrote:
>> Untie the VBT based generic panel driver from the VBT parsing, so that
>> the two don't have to be updated in lockstep.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 22 +++++++++-------------
>>  1 file changed, 9 insertions(+), 13 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> index 45512e0df57a..ba5355506590 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> @@ -232,11 +232,9 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>>  typedef const u8 * (*fn_mipi_elem_exec)(struct intel_dsi *intel_dsi,
>>  					const u8 *data);
>>  static const fn_mipi_elem_exec exec_elem[] = {
>> -	NULL, /* reserved */
>> -	mipi_exec_send_packet,
>> -	mipi_exec_delay,
>> -	mipi_exec_gpio,
>> -	NULL, /* status read; later */
>> +	[MIPI_SEQ_ELEM_SEND_PKT] = mipi_exec_send_packet,
>> +	[MIPI_SEQ_ELEM_DELAY] = mipi_exec_delay,
>> +	[MIPI_SEQ_ELEM_GPIO] = mipi_exec_gpio,
>>  };
>>  
>>  /*
>> @@ -264,7 +262,6 @@ static const char *sequence_name(enum mipi_seq seq_id)
>>  static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
>>  {
>>  	fn_mipi_elem_exec mipi_elem_exec;
>> -	int index;
>>  
>>  	if (!data)
>>  		return;
>> @@ -277,15 +274,14 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
>>  
>>  	/* parse each byte till we reach end of sequence byte - 0x00 */
>>  	while (1) {
>> -		index = *data;
>> -		mipi_elem_exec = exec_elem[index];
>> -		if (!mipi_elem_exec) {
>> -			DRM_ERROR("Unsupported MIPI element, skipping sequence execution\n");
>> +		u8 operation_byte = *data++;
>> +		if (operation_byte >= ARRAY_SIZE(exec_elem) ||
>> +		    !exec_elem[operation_byte]) {
>> +			DRM_ERROR("Unsupported MIPI operation byte %u\n",
>
> Maybe DRM_ERROR in the previous patch too? Just for ocd consistency.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Pushed up to and including this patch. I took the liberty of pushing
as-is, leaving the nitpicks for follow-up. Thanks for the review.

BR,
Jani.


>
>> +				  operation_byte);
>>  			return;
>>  		}
>> -
>> -		/* goto element payload */
>> -		data++;
>> +		mipi_elem_exec = exec_elem[operation_byte];
>>  
>>  		/* execute the element specific rotines */
>>  		data = mipi_elem_exec(intel_dsi, data);
>> -- 
>> 2.1.4
>> 
>> _______________________________________________
>> 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

Re: [PATCH 14/15] drm/i915/dsi: skip unknown elements for sequence block v3+

[Jani Nikula]

On Tue, 05 Jan 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Dec 21, 2015 at 03:11:05PM +0200, Jani Nikula wrote:
>> The sequence block has sizes of elements after the operation byte since
>> sequence block v3. Use it to skip elements we don't support yet.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 43 +++++++++++++++++-------------
>>  1 file changed, 24 insertions(+), 19 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> index eabfd9eb9cc0..1f9c80d21904 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> @@ -335,31 +335,36 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
>>  	if (dev_priv->vbt.dsi.seq_version >= 3)
>>  		data += 4;
>>  
>> -	/* parse each byte till we reach end of sequence byte - 0x00 */
>>  	while (1) {
>>  		u8 operation_byte = *data++;
>> -		if (operation_byte >= ARRAY_SIZE(exec_elem) ||
>> -		    !exec_elem[operation_byte]) {
>> +		u8 operation_size = 0;
>> +
>> +		if (operation_byte == MIPI_SEQ_ELEM_END)
>> +			break;
>> +
>> +		if (operation_byte < ARRAY_SIZE(exec_elem) &&
>> +		    exec_elem[operation_byte])
>
> && exec_elem[operation_byte] is redundant since you assing it to NULL
> anyway in the else clause. While I bikeshed: Might look prettier with ?:

Silly me.

I might prefer an if-else if the ?: doesn't fit on one line.

>
>
>> +			mipi_elem_exec = exec_elem[operation_byte];
>> +		else
>> +			mipi_elem_exec = NULL;
>> +
>> +		/* Size of Operation. */
>> +		if (dev_priv->vbt.dsi.seq_version >= 3)
>> +			operation_size = *data++;
>> +
>> +		if (mipi_elem_exec) {
>> +			data = mipi_elem_exec(intel_dsi, data);
>> +		} else if (operation_size) {
>> +			/* We have size, skip. */
>> +			DRM_DEBUG_KMS("Unsupported MIPI operation byte %u\n",
>> +				      operation_byte);
>
> DRM_ERROR, like in the other cases we fail to parse the sequence fully?

I suppose we could add that in intel_bios.c to do it a limited number of
times at driver load, but here it would keep spewing out the errors
every modeset and possibly more. This is not necessarily a showstopper
with sequence v3, as we have the size to move on to the next one.

BR,
Jani.

>
> Either way on both bikesheds: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
>
>> +			data += operation_size;
>> +		} else {
>> +			/* No size, can't skip without parsing. */
>>  			DRM_ERROR("Unsupported MIPI operation byte %u\n",
>>  				  operation_byte);
>>  			return;
>>  		}
>> -		mipi_elem_exec = exec_elem[operation_byte];
>> -
>> -		/* Skip Size of Operation. */
>> -		if (dev_priv->vbt.dsi.seq_version >= 3)
>> -			data++;
>> -
>> -		/* execute the element specific rotines */
>> -		data = mipi_elem_exec(intel_dsi, data);
>> -
>> -		/*
>> -		 * After processing the element, data should point to
>> -		 * next element or end of sequence
>> -		 * check if have we reached end of sequence
>> -		 */
>> -		if (*data == 0x00)
>> -			break;
>>  	}
>>  }
>>  
>> -- 
>> 2.1.4
>> 
>> _______________________________________________
>> 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

[PATCH v2] drm/i915/bios: add support for MIPI sequence block v3

[Jani Nikula]

The changes since the sequence block v2 are:

* The whole MIPI bios data block has a separate 32-bit size field since
  v3, stored after the version. This facilitates big sequences.

* The size of the panel specific sequence blocks has grown to 32
  bits. This facilitates big sequences.

* The elements within sequences now have an 8-bit size field following
  the operation byte. This facilitates skipping unknown new operation
  bytes, i.e. forward compatibility.

v2 (of the patch): use DRM_ERROR for unknown operation byte

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c          | 84 ++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c |  9 ++++
 2 files changed, 84 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 69040a73c09a..6c09b5643942 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -754,21 +754,30 @@ parse_mipi_config(struct drm_i915_private *dev_priv,
 /* Find the sequence block and size for the given panel. */
 static const u8 *
 find_panel_sequence_block(const struct bdb_mipi_sequence *sequence,
-			  u16 panel_id, u16 *seq_size)
+			  u16 panel_id, u32 *seq_size)
 {
 	u32 total = get_blocksize(sequence);
 	const u8 *data = &sequence->data[0];
 	u8 current_id;
-	u16 current_size;
+	u32 current_size;
 	int index = 0;
 	int i;
 
+	/* skip new block size */
+	if (sequence->version >= 3)
+		data += 4;
+
 	for (i = 0; i < MAX_MIPI_CONFIGURATIONS && index + 3 < total; i++) {
 		current_id = *(data + index);
 		index++;
 
-		current_size = *((const u16 *)(data + index));
-		index += 2;
+		if (sequence->version >= 3) {
+			current_size = *((const u32 *)(data + index));
+			index += 4;
+		} else {
+			current_size = *((const u16 *)(data + index));
+			index += 2;
+		}
 
 		if (index + current_size > total) {
 			DRM_ERROR("Invalid sequence block\n");
@@ -826,13 +835,66 @@ static int goto_next_sequence(const u8 *data, int index, int total)
 	return 0;
 }
 
+static int goto_next_sequence_v3(const u8 *data, int index, int total)
+{
+	int seq_end;
+	u16 len;
+
+	/*
+	 * Could skip sequence based on Size of Sequence alone, but also do some
+	 * checking on the structure.
+	 */
+	seq_end = index + *((const u32 *)(data + 1));
+	if (seq_end > total) {
+		DRM_ERROR("Invalid sequence size\n");
+		return 0;
+	}
+
+	/* Skip Sequence Byte and Size of Sequence. */
+	for (index = index + 5; index < total; index += len) {
+		u8 operation_byte = *(data + index);
+		index++;
+
+		if (operation_byte == MIPI_SEQ_ELEM_END) {
+			if (index != seq_end) {
+				DRM_ERROR("Invalid element structure\n");
+				return 0;
+			}
+			return index;
+		}
+
+		len = *(data + index);
+		index++;
+
+		/*
+		 * FIXME: Would be nice to check elements like for v1/v2 in
+		 * goto_next_sequence() above.
+		 */
+		switch (operation_byte) {
+		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:
+			break;
+		default:
+			DRM_ERROR("Unknown operation byte %u\n",
+				  operation_byte);
+			break;
+		}
+	}
+
+	return 0;
+}
+
 static void
 parse_mipi_sequence(struct drm_i915_private *dev_priv,
 		    const struct bdb_header *bdb)
 {
 	const struct bdb_mipi_sequence *sequence;
 	const u8 *seq_data;
-	u16 seq_size;
+	u32 seq_size;
 	u8 *data;
 	int index = 0;
 
@@ -847,12 +909,13 @@ parse_mipi_sequence(struct drm_i915_private *dev_priv,
 	}
 
 	/* Fail gracefully for forward incompatible sequence block. */
-	if (sequence->version >= 3) {
-		DRM_ERROR("Unable to parse MIPI Sequence Block v3+\n");
+	if (sequence->version >= 4) {
+		DRM_ERROR("Unable to parse MIPI Sequence Block v%u\n",
+			  sequence->version);
 		return;
 	}
 
-	DRM_DEBUG_DRIVER("Found MIPI sequence block\n");
+	DRM_DEBUG_DRIVER("Found MIPI sequence block v%u\n", sequence->version);
 
 	seq_data = find_panel_sequence_block(sequence, panel_type, &seq_size);
 	if (!seq_data)
@@ -875,7 +938,10 @@ parse_mipi_sequence(struct drm_i915_private *dev_priv,
 
 		dev_priv->vbt.dsi.sequence[seq_id] = data + index;
 
-		index = goto_next_sequence(data, index, seq_size);
+		if (sequence->version >= 3)
+			index = goto_next_sequence_v3(data, index, seq_size);
+		else
+			index = goto_next_sequence(data, index, seq_size);
 		if (!index) {
 			DRM_ERROR("Invalid sequence %u\n", seq_id);
 			goto err;
diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 5303d1c446d5..7f67749eb0ef 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -319,6 +319,7 @@ static const char *sequence_name(enum mipi_seq seq_id)
 
 static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
 {
+	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
 	fn_mipi_elem_exec mipi_elem_exec;
 
 	if (!data)
@@ -330,6 +331,10 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
 	/* go to the first element of the sequence */
 	data++;
 
+	/* Skip Size of Sequence. */
+	if (dev_priv->vbt.dsi.seq_version >= 3)
+		data += 4;
+
 	/* parse each byte till we reach end of sequence byte - 0x00 */
 	while (1) {
 		u8 operation_byte = *data++;
@@ -341,6 +346,10 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
 		}
 		mipi_elem_exec = exec_elem[operation_byte];
 
+		/* Skip Size of Operation. */
+		if (dev_priv->vbt.dsi.seq_version >= 3)
+			data++;
+
 		/* execute the element specific rotines */
 		data = mipi_elem_exec(intel_dsi, data);
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915/dsi: skip unknown elements for sequence block v3+

[Jani Nikula]

The sequence block has sizes of elements after the operation byte since
sequence block v3. Use it to skip elements we don't support yet.

v2: remove redundant exec_elem[operation_byte] check (Daniel)

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 42 ++++++++++++++++--------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 7f67749eb0ef..4a6f9a593ea2 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -335,31 +335,35 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
 	if (dev_priv->vbt.dsi.seq_version >= 3)
 		data += 4;
 
-	/* parse each byte till we reach end of sequence byte - 0x00 */
 	while (1) {
 		u8 operation_byte = *data++;
-		if (operation_byte >= ARRAY_SIZE(exec_elem) ||
-		    !exec_elem[operation_byte]) {
+		u8 operation_size = 0;
+
+		if (operation_byte == MIPI_SEQ_ELEM_END)
+			break;
+
+		if (operation_byte < ARRAY_SIZE(exec_elem))
+			mipi_elem_exec = exec_elem[operation_byte];
+		else
+			mipi_elem_exec = NULL;
+
+		/* Size of Operation. */
+		if (dev_priv->vbt.dsi.seq_version >= 3)
+			operation_size = *data++;
+
+		if (mipi_elem_exec) {
+			data = mipi_elem_exec(intel_dsi, data);
+		} else if (operation_size) {
+			/* We have size, skip. */
+			DRM_DEBUG_KMS("Unsupported MIPI operation byte %u\n",
+				      operation_byte);
+			data += operation_size;
+		} else {
+			/* No size, can't skip without parsing. */
 			DRM_ERROR("Unsupported MIPI operation byte %u\n",
 				  operation_byte);
 			return;
 		}
-		mipi_elem_exec = exec_elem[operation_byte];
-
-		/* Skip Size of Operation. */
-		if (dev_priv->vbt.dsi.seq_version >= 3)
-			data++;
-
-		/* execute the element specific rotines */
-		data = mipi_elem_exec(intel_dsi, data);
-
-		/*
-		 * After processing the element, data should point to
-		 * next element or end of sequence
-		 * check if have we reached end of sequence
-		 */
-		if (*data == 0x00)
-			break;
 	}
 }
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915/dsi: add debug printing of the new sequence block names

[Jani Nikula]

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 70fc11e0869d..a5817bef2d71 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -307,6 +307,12 @@ static const char * const seq_name[] = {
 	[MIPI_SEQ_DISPLAY_ON] = "MIPI_SEQ_DISPLAY_ON",
 	[MIPI_SEQ_DISPLAY_OFF]  = "MIPI_SEQ_DISPLAY_OFF",
 	[MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET",
+	[MIPI_SEQ_BACKLIGHT_ON] = "MIPI_SEQ_BACKLIGHT_ON",
+	[MIPI_SEQ_BACKLIGHT_OFF] = "MIPI_SEQ_BACKLIGHT_OFF",
+	[MIPI_SEQ_TEAR_ON] = "MIPI_SEQ_TEAR_ON",
+	[MIPI_SEQ_TEAR_OFF] = "MIPI_SEQ_TEAR_OFF",
+	[MIPI_SEQ_POWER_ON] = "MIPI_SEQ_POWER_ON",
+	[MIPI_SEQ_POWER_OFF] = "MIPI_SEQ_POWER_OFF",
 };
 
 static const char *sequence_name(enum mipi_seq seq_id)
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ failure: Fi.CI.BAT

[Patchwork]

== Summary ==

HEAD is now at 865e245 drm-intel-nightly: 2016y-01m-05d-15h-23m-53s UTC integration manifest
Applying: drm/i915/bios: add proper documentation for the Video BIOS Table (VBT)
Using index info to reconstruct a base tree...
M	Documentation/DocBook/gpu.tmpl
M	drivers/gpu/drm/i915/intel_bios.c
M	drivers/gpu/drm/i915/intel_bios.h
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_bios.h
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_bios.h
Auto-merging drivers/gpu/drm/i915/intel_bios.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_bios.c
Patch failed at 0001 drm/i915/bios: add proper documentation for the Video BIOS Table (VBT)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/15] drm/i915/bios: interpret the i2c element

[Ville Syrjälä]

On Mon, Dec 21, 2015 at 03:11:00PM +0200, Jani Nikula wrote:
> Add parsing of the i2c element, defined in MIPI sequence block v2. Drop
> the status operation byte while at it, that does not exist.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_bios.c | 5 +++++
>  drivers/gpu/drm/i915/intel_bios.h | 2 +-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index d6eaf32f33e5..45a7a2bc96c6 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -812,6 +812,11 @@ static int goto_next_sequence(const u8 *data, int index, int total)
>  		case MIPI_SEQ_ELEM_GPIO:
>  			len = 2;

Somewhat off topic, but I wonder if this is correct. The "structure"
diagram shows it as 2 bytes for v1 and v2, but I'm not sure that section
isn't there just as an example. Later the describing the GPIO block it
seems to say it's 2 bytes for v1, and three bytes for v2.

>  			break;
> +		case MIPI_SEQ_ELEM_I2C:
> +			if (index + 7 > total)
> +				return 0;
> +			len = *(data + index + 6) + 7;
> +			break;

This one isn't show in the structure diagrams at all, so I guess we get
to trust the other section. It says this was introduced in v2. Should be
add a paranoia check for that?

Should we also check that the payload length is below the specified max
of 240 bytes?

>  		default:
>  			DRM_ERROR("Unknown operation byte\n");
>  			return 0;
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index 4e87df16e7b3..411b33794536 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -968,7 +968,7 @@ enum mipi_seq_element {
>  	MIPI_SEQ_ELEM_SEND_PKT,
>  	MIPI_SEQ_ELEM_DELAY,
>  	MIPI_SEQ_ELEM_GPIO,
> -	MIPI_SEQ_ELEM_STATUS,
> +	MIPI_SEQ_ELEM_I2C,		/* sequence block v2+ */
>  	MIPI_SEQ_ELEM_MAX
>  };
>  
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/15] drm/i915/bios: interpret the i2c element

[Jani Nikula]

On Tue, 05 Jan 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Mon, Dec 21, 2015 at 03:11:00PM +0200, Jani Nikula wrote:
>> Add parsing of the i2c element, defined in MIPI sequence block v2. Drop
>> the status operation byte while at it, that does not exist.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_bios.c | 5 +++++
>>  drivers/gpu/drm/i915/intel_bios.h | 2 +-
>>  2 files changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index d6eaf32f33e5..45a7a2bc96c6 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -812,6 +812,11 @@ static int goto_next_sequence(const u8 *data, int index, int total)
>>  		case MIPI_SEQ_ELEM_GPIO:
>>  			len = 2;
>
> Somewhat off topic, but I wonder if this is correct. The "structure"
> diagram shows it as 2 bytes for v1 and v2, but I'm not sure that section
> isn't there just as an example. Later the describing the GPIO block it
> seems to say it's 2 bytes for v1, and three bytes for v2.

I've held on to some old spec versions (bdb version 177 has sequence v1,
bdb version 185 has sequence v2). Both v1 and v2 have 2 bytes payload
for the GPIO element.

The *meaning* of the first of those bytes has changed from v1->v2
though. Can't do much about that here, it's up to the generic vbt dsi
"driver"...

>
>>  			break;
>> +		case MIPI_SEQ_ELEM_I2C:
>> +			if (index + 7 > total)
>> +				return 0;
>> +			len = *(data + index + 6) + 7;
>> +			break;
>
> This one isn't show in the structure diagrams at all, so I guess we get
> to trust the other section. It says this was introduced in v2. Should be
> add a paranoia check for that?

The spec with bdb version 185 has this.

I contemplated adding a check for v2, but then I thought it probably
doesn't really matter all that much. If we get an i2c elem with v1 and
reject it, we'll probably end up with a black screen. If we just assume
it's an i2c element but it isn't, it'll trip over something else later.

> Should we also check that the payload length is below the specified max
> of 240 bytes?

You'll love this. In v2 the max is actually the whole byte i.e. 255. In
v3 they added a length field for these operations for forward
compatibility (can now skip unknown new operations). And that's 8
bits. So the header + payload for the i2c data can't exceed 255, so
there's now an arbitrary 240 byte limit for i2c payload in v3.

BR,
Jani.


>
>>  		default:
>>  			DRM_ERROR("Unknown operation byte\n");
>>  			return 0;
>> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
>> index 4e87df16e7b3..411b33794536 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.h
>> +++ b/drivers/gpu/drm/i915/intel_bios.h
>> @@ -968,7 +968,7 @@ enum mipi_seq_element {
>>  	MIPI_SEQ_ELEM_SEND_PKT,
>>  	MIPI_SEQ_ELEM_DELAY,
>>  	MIPI_SEQ_ELEM_GPIO,
>> -	MIPI_SEQ_ELEM_STATUS,
>> +	MIPI_SEQ_ELEM_I2C,		/* sequence block v2+ */
>>  	MIPI_SEQ_ELEM_MAX
>>  };
>>  
>> -- 
>> 2.1.4
>> 
>> _______________________________________________
>> 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

Re: [PATCH 09/15] drm/i915/bios: interpret the i2c element

[Ville Syrjälä]

On Thu, Jan 07, 2016 at 11:31:25AM +0200, Jani Nikula wrote:
> On Tue, 05 Jan 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Mon, Dec 21, 2015 at 03:11:00PM +0200, Jani Nikula wrote:
> >> Add parsing of the i2c element, defined in MIPI sequence block v2. Drop
> >> the status operation byte while at it, that does not exist.
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_bios.c | 5 +++++
> >>  drivers/gpu/drm/i915/intel_bios.h | 2 +-
> >>  2 files changed, 6 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> >> index d6eaf32f33e5..45a7a2bc96c6 100644
> >> --- a/drivers/gpu/drm/i915/intel_bios.c
> >> +++ b/drivers/gpu/drm/i915/intel_bios.c
> >> @@ -812,6 +812,11 @@ static int goto_next_sequence(const u8 *data, int index, int total)
> >>  		case MIPI_SEQ_ELEM_GPIO:
> >>  			len = 2;
> >
> > Somewhat off topic, but I wonder if this is correct. The "structure"
> > diagram shows it as 2 bytes for v1 and v2, but I'm not sure that section
> > isn't there just as an example. Later the describing the GPIO block it
> > seems to say it's 2 bytes for v1, and three bytes for v2.
> 
> I've held on to some old spec versions (bdb version 177 has sequence v1,
> bdb version 185 has sequence v2). Both v1 and v2 have 2 bytes payload
> for the GPIO element.
> 
> The *meaning* of the first of those bytes has changed from v1->v2
> though. Can't do much about that here, it's up to the generic vbt dsi
> "driver"...
> 
> >
> >>  			break;
> >> +		case MIPI_SEQ_ELEM_I2C:
> >> +			if (index + 7 > total)
> >> +				return 0;
> >> +			len = *(data + index + 6) + 7;
> >> +			break;
> >
> > This one isn't show in the structure diagrams at all, so I guess we get
> > to trust the other section. It says this was introduced in v2. Should be
> > add a paranoia check for that?
> 
> The spec with bdb version 185 has this.
> 
> I contemplated adding a check for v2, but then I thought it probably
> doesn't really matter all that much. If we get an i2c elem with v1 and
> reject it, we'll probably end up with a black screen. If we just assume
> it's an i2c element but it isn't, it'll trip over something else later.
> 
> > Should we also check that the payload length is below the specified max
> > of 240 bytes?
> 
> You'll love this. In v2 the max is actually the whole byte i.e. 255. In
> v3 they added a length field for these operations for forward
> compatibility (can now skip unknown new operations). And that's 8
> bits. So the header + payload for the i2c data can't exceed 255, so
> there's now an arbitrary 240 byte limit for i2c payload in v3.

I don't really see where the 240 comes from. The spec lists 240 byte
payload size limit for i2c, spi, and send packet operations, but the
size of the header is different for those so I can't see how all
would end up with the same payload length limitation. So to me it seems
like the max payload limit should be 255-7 for i2c, 255-6 for spi, and
255-4 for send packet (since the "size of operation" byte doesn't seem
to include itself or the "operation byte"). So to me the 240 seems like
a totally arbitrary limit.

> 
> BR,
> Jani.
> 
> 
> >
> >>  		default:
> >>  			DRM_ERROR("Unknown operation byte\n");
> >>  			return 0;
> >> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> >> index 4e87df16e7b3..411b33794536 100644
> >> --- a/drivers/gpu/drm/i915/intel_bios.h
> >> +++ b/drivers/gpu/drm/i915/intel_bios.h
> >> @@ -968,7 +968,7 @@ enum mipi_seq_element {
> >>  	MIPI_SEQ_ELEM_SEND_PKT,
> >>  	MIPI_SEQ_ELEM_DELAY,
> >>  	MIPI_SEQ_ELEM_GPIO,
> >> -	MIPI_SEQ_ELEM_STATUS,
> >> +	MIPI_SEQ_ELEM_I2C,		/* sequence block v2+ */
> >>  	MIPI_SEQ_ELEM_MAX
> >>  };
> >>  
> >> -- 
> >> 2.1.4
> >> 
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/15] drm/i915/bios: add sequences for MIPI sequence block v2

[Ville Syrjälä]

On Mon, Dec 21, 2015 at 03:11:01PM +0200, Jani Nikula wrote:
> Properly parse the new sequences added in MIPI sequence block v2.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_bios.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index 411b33794536..6146f1b0cf48 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -960,6 +960,9 @@ enum mipi_seq {
>  	MIPI_SEQ_DISPLAY_ON,
>  	MIPI_SEQ_DISPLAY_OFF,
>  	MIPI_SEQ_DEASSERT_RESET,
> +	MIPI_SEQ_BACKLIGHT_ON,		/* sequence block v2+ */
> +	MIPI_SEQ_BACKLIGHT_OFF,		/* sequence block v2+ */
> +	MIPI_SEQ_TEAR_ON,		/* sequence block v2+ */

Can't comment on the v2+ part since the spec fails to mention it, but
otherwise looks sane.

It's a bit hard to review w/o an explicit assignments for each, but
assuming the MIPI_SEQ_DEASSERT_RESET value is correct these should be
correct too.

Not sure why we stopped at "tear on" though? The spec has "tear off",
"panel on" and "panel off" listed as well.

>  	MIPI_SEQ_MAX
>  };
>  
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/15] drm/i915/bios: add sequences for MIPI sequence block v2

[Jani Nikula]

On Thu, 07 Jan 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Mon, Dec 21, 2015 at 03:11:01PM +0200, Jani Nikula wrote:
>> Properly parse the new sequences added in MIPI sequence block v2.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_bios.h | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
>> index 411b33794536..6146f1b0cf48 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.h
>> +++ b/drivers/gpu/drm/i915/intel_bios.h
>> @@ -960,6 +960,9 @@ enum mipi_seq {
>>  	MIPI_SEQ_DISPLAY_ON,
>>  	MIPI_SEQ_DISPLAY_OFF,
>>  	MIPI_SEQ_DEASSERT_RESET,
>> +	MIPI_SEQ_BACKLIGHT_ON,		/* sequence block v2+ */
>> +	MIPI_SEQ_BACKLIGHT_OFF,		/* sequence block v2+ */
>> +	MIPI_SEQ_TEAR_ON,		/* sequence block v2+ */
>
> Can't comment on the v2+ part since the spec fails to mention it, but
> otherwise looks sane.
>
> It's a bit hard to review w/o an explicit assignments for each, but
> assuming the MIPI_SEQ_DEASSERT_RESET value is correct these should be
> correct too.
>
> Not sure why we stopped at "tear on" though? The spec has "tear off",
> "panel on" and "panel off" listed as well.

Just for lols. The sequence block v2 stops there. The rest are added at
v3. Words fail me.

BR,
Jani.

>
>>  	MIPI_SEQ_MAX
>>  };
>>  
>> -- 
>> 2.1.4
>> 
>> _______________________________________________
>> 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

Re: [PATCH 11/15] drm/i915: Adding the parsing logic for the i2c element

[Ville Syrjälä]

On Mon, Dec 21, 2015 at 03:11:02PM +0200, Jani Nikula wrote:
> From: vkorjani <vikas.korjani@intel.com>
> 
> New sequence element for i2c is been added in the
> mipi sequence block of the VBT. This patch parses
> and executes the i2c sequence.
> 
> v2: Add i2c_put_adapter call(Jani), rebase
> 
> v3: corrected the retry loop(Jani), rebase
> 
> v4 by Jani:
>  - don't put the adapter if get fails
>  - print an error message if all retries exhausted
>  - use a for loop
>  - fix warnings for unused variables
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: vkorjani <vikas.korjani@intel.com>
> Signed-off-by: Deepak M <m.deepak@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 58 ++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index ba5355506590..8fcfb0f63dc1 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -31,6 +31,7 @@
>  #include <drm/drm_panel.h>
>  #include <linux/slab.h>
>  #include <video/mipi_display.h>
> +#include <linux/i2c.h>
>  #include <asm/intel-mid.h>
>  #include <video/mipi_display.h>
>  #include "i915_drv.h"
> @@ -104,6 +105,62 @@ static struct gpio_table gtable[] = {
>  	{ GPIO_NC_11_PCONF0, GPIO_NC_11_PAD, 0}
>  };
>  
> +static const u8 *mipi_exec_i2c(struct intel_dsi *intel_dsi, const u8 *data)
> +{
> +	struct i2c_adapter *adapter;
> +	int ret, i;
> +	u8 reg_offset, payload_size;
> +	struct i2c_msg msg;
> +	u8 *transmit_buffer;
> +	u8 flag, bus_number;
> +	u16 slave_add;
> +
> +	flag = *data++;
> +	data++; /* index, unused */
> +	bus_number = *data++;
> +	slave_add = *(u16 *)(data);
> +	data += 2;
> +	reg_offset = *data++;
> +	payload_size = *data++;
> +
> +	adapter = i2c_get_adapter(bus_number);

I'm trying to find who is responsible for registering i2c busses with
these magic bus numbers. So far I can't see anything that would be doing
it.

> +	if (!adapter) {
> +		DRM_ERROR("i2c_get_adapter(%u)\n", bus_number);
> +		goto out;
> +	}
> +
> +	transmit_buffer = kmalloc(1 + payload_size, GFP_TEMPORARY);
> +	if (!transmit_buffer)
> +		goto out_put;
> +
> +	transmit_buffer[0] = reg_offset;
> +	memcpy(&transmit_buffer[1], data, payload_size);
> +
> +	msg.addr = slave_add;
> +	msg.flags = 0;
> +	msg.len = payload_size + 1;
> +	msg.buf = &transmit_buffer[0];
> +
> +	for (i = 0; i < 6; i++) {
> +		ret = i2c_transfer(adapter, &msg, 1);
> +		if (ret == 1) {
> +			goto out_free;
> +		} else if (ret == -EAGAIN) {
> +			usleep_range(1000, 2500);
> +		} else {
> +			break;
> +		}
> +	}
> +
> +	DRM_ERROR("i2c transfer failed: %d\n", ret);
> +out_free:
> +	kfree(transmit_buffer);
> +out_put:
> +	i2c_put_adapter(adapter);
> +out:
> +	return data + payload_size;
> +}
> +
>  static inline enum port intel_dsi_seq_port_to_port(u8 port)
>  {
>  	return port ? PORT_C : PORT_A;
> @@ -235,6 +292,7 @@ static const fn_mipi_elem_exec exec_elem[] = {
>  	[MIPI_SEQ_ELEM_SEND_PKT] = mipi_exec_send_packet,
>  	[MIPI_SEQ_ELEM_DELAY] = mipi_exec_delay,
>  	[MIPI_SEQ_ELEM_GPIO] = mipi_exec_gpio,
> +	[MIPI_SEQ_ELEM_I2C] = mipi_exec_i2c,
>  };
>  
>  /*
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/15] drm/i915/bios: add sequences for MIPI sequence block v2

[Ville Syrjälä]

On Thu, Jan 07, 2016 at 04:54:00PM +0200, Jani Nikula wrote:
> On Thu, 07 Jan 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Mon, Dec 21, 2015 at 03:11:01PM +0200, Jani Nikula wrote:
> >> Properly parse the new sequences added in MIPI sequence block v2.
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_bios.h | 3 +++
> >>  1 file changed, 3 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> >> index 411b33794536..6146f1b0cf48 100644
> >> --- a/drivers/gpu/drm/i915/intel_bios.h
> >> +++ b/drivers/gpu/drm/i915/intel_bios.h
> >> @@ -960,6 +960,9 @@ enum mipi_seq {
> >>  	MIPI_SEQ_DISPLAY_ON,
> >>  	MIPI_SEQ_DISPLAY_OFF,
> >>  	MIPI_SEQ_DEASSERT_RESET,
> >> +	MIPI_SEQ_BACKLIGHT_ON,		/* sequence block v2+ */
> >> +	MIPI_SEQ_BACKLIGHT_OFF,		/* sequence block v2+ */
> >> +	MIPI_SEQ_TEAR_ON,		/* sequence block v2+ */
> >
> > Can't comment on the v2+ part since the spec fails to mention it, but
> > otherwise looks sane.
> >
> > It's a bit hard to review w/o an explicit assignments for each, but
> > assuming the MIPI_SEQ_DEASSERT_RESET value is correct these should be
> > correct too.
> >
> > Not sure why we stopped at "tear on" though? The spec has "tear off",
> > "panel on" and "panel off" listed as well.
> 
> Just for lols. The sequence block v2 stops there. The rest are added at
> v3. Words fail me.

Okay, that's a good enough excuse in my book:

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/15] drm/i915/bios: add defines for v3 sequence block

[Ville Syrjälä]

On Mon, Dec 21, 2015 at 03:11:03PM +0200, Jani Nikula wrote:
> New sequences, new operations within sequences.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_bios.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index 6146f1b0cf48..350d4e0f75a4 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -963,6 +963,9 @@ enum mipi_seq {
>  	MIPI_SEQ_BACKLIGHT_ON,		/* sequence block v2+ */
>  	MIPI_SEQ_BACKLIGHT_OFF,		/* sequence block v2+ */
>  	MIPI_SEQ_TEAR_ON,		/* sequence block v2+ */
> +	MIPI_SEQ_TEAR_OFF,		/* sequence block v3+ */
> +	MIPI_SEQ_POWER_ON,		/* sequence block v3+ */
> +	MIPI_SEQ_POWER_OFF,		/* sequence block v3+ */
>  	MIPI_SEQ_MAX
>  };
>  
> @@ -972,6 +975,8 @@ enum mipi_seq_element {
>  	MIPI_SEQ_ELEM_DELAY,
>  	MIPI_SEQ_ELEM_GPIO,
>  	MIPI_SEQ_ELEM_I2C,		/* sequence block v2+ */
> +	MIPI_SEQ_ELEM_SPI,		/* sequence block v3+ */
> +	MIPI_SEQ_ELEM_PMIC,		/* sequence block v3+ */
>  	MIPI_SEQ_ELEM_MAX
>  };
>  
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/bios: add support for MIPI sequence block v3

[Ville Syrjälä]

On Tue, Jan 05, 2016 at 05:01:15PM +0200, Jani Nikula wrote:
> The changes since the sequence block v2 are:
> 
> * The whole MIPI bios data block has a separate 32-bit size field since
>   v3, stored after the version. This facilitates big sequences.
> 
> * The size of the panel specific sequence blocks has grown to 32
>   bits. This facilitates big sequences.
> 
> * The elements within sequences now have an 8-bit size field following
>   the operation byte. This facilitates skipping unknown new operation
>   bytes, i.e. forward compatibility.
> 
> v2 (of the patch): use DRM_ERROR for unknown operation byte
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_bios.c          | 84 ++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c |  9 ++++
>  2 files changed, 84 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 69040a73c09a..6c09b5643942 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -754,21 +754,30 @@ parse_mipi_config(struct drm_i915_private *dev_priv,
>  /* Find the sequence block and size for the given panel. */
>  static const u8 *
>  find_panel_sequence_block(const struct bdb_mipi_sequence *sequence,
> -			  u16 panel_id, u16 *seq_size)
> +			  u16 panel_id, u32 *seq_size)
>  {
>  	u32 total = get_blocksize(sequence);
>  	const u8 *data = &sequence->data[0];
>  	u8 current_id;
> -	u16 current_size;
> +	u32 current_size;
>  	int index = 0;
>  	int i;
>  
> +	/* skip new block size */
> +	if (sequence->version >= 3)
> +		data += 4;
> +
>  	for (i = 0; i < MAX_MIPI_CONFIGURATIONS && index + 3 < total; i++) {

The index vs. total check should be updated too.

>  		current_id = *(data + index);
>  		index++;
>  
> -		current_size = *((const u16 *)(data + index));
> -		index += 2;
> +		if (sequence->version >= 3) {
> +			current_size = *((const u32 *)(data + index));
> +			index += 4;
> +		} else {
> +			current_size = *((const u16 *)(data + index));
> +			index += 2;
> +		}
>  
>  		if (index + current_size > total) {
>  			DRM_ERROR("Invalid sequence block\n");
> @@ -826,13 +835,66 @@ static int goto_next_sequence(const u8 *data, int index, int total)
>  	return 0;
>  }
>  
> +static int goto_next_sequence_v3(const u8 *data, int index, int total)
> +{
> +	int seq_end;
> +	u16 len;
> +
> +	/*
> +	 * Could skip sequence based on Size of Sequence alone, but also do some
> +	 * checking on the structure.
> +	 */
> +	seq_end = index + *((const u32 *)(data + 1));

Should we check total to make sure we don't access beyond what's
available?

> +	if (seq_end > total) {
> +		DRM_ERROR("Invalid sequence size\n");
> +		return 0;
> +	}
> +
> +	/* Skip Sequence Byte and Size of Sequence. */
> +	for (index = index + 5; index < total; index += len) {
> +		u8 operation_byte = *(data + index);
> +		index++;
> +
> +		if (operation_byte == MIPI_SEQ_ELEM_END) {
> +			if (index != seq_end) {
> +				DRM_ERROR("Invalid element structure\n");
> +				return 0;
> +			}
> +			return index;
> +		}
> +
> +		len = *(data + index);
> +		index++;
> +
> +		/*
> +		 * FIXME: Would be nice to check elements like for v1/v2 in
> +		 * goto_next_sequence() above.
> +		 */
> +		switch (operation_byte) {
> +		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:
> +			break;
> +		default:
> +			DRM_ERROR("Unknown operation byte %u\n",
> +				  operation_byte);
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static void
>  parse_mipi_sequence(struct drm_i915_private *dev_priv,
>  		    const struct bdb_header *bdb)
>  {
>  	const struct bdb_mipi_sequence *sequence;
>  	const u8 *seq_data;
> -	u16 seq_size;
> +	u32 seq_size;
>  	u8 *data;
>  	int index = 0;
>  
> @@ -847,12 +909,13 @@ parse_mipi_sequence(struct drm_i915_private *dev_priv,
>  	}
>  
>  	/* Fail gracefully for forward incompatible sequence block. */
> -	if (sequence->version >= 3) {
> -		DRM_ERROR("Unable to parse MIPI Sequence Block v3+\n");
> +	if (sequence->version >= 4) {
> +		DRM_ERROR("Unable to parse MIPI Sequence Block v%u\n",
> +			  sequence->version);
>  		return;
>  	}
>  
> -	DRM_DEBUG_DRIVER("Found MIPI sequence block\n");
> +	DRM_DEBUG_DRIVER("Found MIPI sequence block v%u\n", sequence->version);
>  
>  	seq_data = find_panel_sequence_block(sequence, panel_type, &seq_size);
>  	if (!seq_data)
> @@ -875,7 +938,10 @@ parse_mipi_sequence(struct drm_i915_private *dev_priv,
>  
>  		dev_priv->vbt.dsi.sequence[seq_id] = data + index;
>  
> -		index = goto_next_sequence(data, index, seq_size);
> +		if (sequence->version >= 3)
> +			index = goto_next_sequence_v3(data, index, seq_size);
> +		else
> +			index = goto_next_sequence(data, index, seq_size);
>  		if (!index) {
>  			DRM_ERROR("Invalid sequence %u\n", seq_id);
>  			goto err;
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index 5303d1c446d5..7f67749eb0ef 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -319,6 +319,7 @@ static const char *sequence_name(enum mipi_seq seq_id)
>  
>  static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
>  	fn_mipi_elem_exec mipi_elem_exec;
>  
>  	if (!data)
> @@ -330,6 +331,10 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
>  	/* go to the first element of the sequence */
>  	data++;
>  
> +	/* Skip Size of Sequence. */
> +	if (dev_priv->vbt.dsi.seq_version >= 3)
> +		data += 4;
> +
>  	/* parse each byte till we reach end of sequence byte - 0x00 */
>  	while (1) {
>  		u8 operation_byte = *data++;
> @@ -341,6 +346,10 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
>  		}
>  		mipi_elem_exec = exec_elem[operation_byte];
>  
> +		/* Skip Size of Operation. */
> +		if (dev_priv->vbt.dsi.seq_version >= 3)
> +			data++;
> +
>  		/* execute the element specific rotines */
>  		data = mipi_elem_exec(intel_dsi, data);
>  
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dsi: add debug printing of the new sequence block names

[Ville Syrjälä]

On Tue, Jan 05, 2016 at 05:08:17PM +0200, Jani Nikula wrote:
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index 70fc11e0869d..a5817bef2d71 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -307,6 +307,12 @@ static const char * const seq_name[] = {
>  	[MIPI_SEQ_DISPLAY_ON] = "MIPI_SEQ_DISPLAY_ON",
>  	[MIPI_SEQ_DISPLAY_OFF]  = "MIPI_SEQ_DISPLAY_OFF",
>  	[MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET",
> +	[MIPI_SEQ_BACKLIGHT_ON] = "MIPI_SEQ_BACKLIGHT_ON",
> +	[MIPI_SEQ_BACKLIGHT_OFF] = "MIPI_SEQ_BACKLIGHT_OFF",
> +	[MIPI_SEQ_TEAR_ON] = "MIPI_SEQ_TEAR_ON",
> +	[MIPI_SEQ_TEAR_OFF] = "MIPI_SEQ_TEAR_OFF",
> +	[MIPI_SEQ_POWER_ON] = "MIPI_SEQ_POWER_ON",
> +	[MIPI_SEQ_POWER_OFF] = "MIPI_SEQ_POWER_OFF",
>  };

Could avoid a bit of typing/potential typos with some preprocessor tricks.

But anyways
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  static const char *sequence_name(enum mipi_seq seq_id)
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/15] drm/i915/bios: interpret the i2c element

[Jani Nikula]

On Thu, 07 Jan 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Jan 07, 2016 at 11:31:25AM +0200, Jani Nikula wrote:
>> On Tue, 05 Jan 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > On Mon, Dec 21, 2015 at 03:11:00PM +0200, Jani Nikula wrote:
>> >> Add parsing of the i2c element, defined in MIPI sequence block v2. Drop
>> >> the status operation byte while at it, that does not exist.
>> >> 
>> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/intel_bios.c | 5 +++++
>> >>  drivers/gpu/drm/i915/intel_bios.h | 2 +-
>> >>  2 files changed, 6 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> >> index d6eaf32f33e5..45a7a2bc96c6 100644
>> >> --- a/drivers/gpu/drm/i915/intel_bios.c
>> >> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> >> @@ -812,6 +812,11 @@ static int goto_next_sequence(const u8 *data, int index, int total)
>> >>  		case MIPI_SEQ_ELEM_GPIO:
>> >>  			len = 2;
>> >
>> > Somewhat off topic, but I wonder if this is correct. The "structure"
>> > diagram shows it as 2 bytes for v1 and v2, but I'm not sure that section
>> > isn't there just as an example. Later the describing the GPIO block it
>> > seems to say it's 2 bytes for v1, and three bytes for v2.
>> 
>> I've held on to some old spec versions (bdb version 177 has sequence v1,
>> bdb version 185 has sequence v2). Both v1 and v2 have 2 bytes payload
>> for the GPIO element.
>> 
>> The *meaning* of the first of those bytes has changed from v1->v2
>> though. Can't do much about that here, it's up to the generic vbt dsi
>> "driver"...
>> 
>> >
>> >>  			break;
>> >> +		case MIPI_SEQ_ELEM_I2C:
>> >> +			if (index + 7 > total)
>> >> +				return 0;
>> >> +			len = *(data + index + 6) + 7;
>> >> +			break;
>> >
>> > This one isn't show in the structure diagrams at all, so I guess we get
>> > to trust the other section. It says this was introduced in v2. Should be
>> > add a paranoia check for that?
>> 
>> The spec with bdb version 185 has this.
>> 
>> I contemplated adding a check for v2, but then I thought it probably
>> doesn't really matter all that much. If we get an i2c elem with v1 and
>> reject it, we'll probably end up with a black screen. If we just assume
>> it's an i2c element but it isn't, it'll trip over something else later.
>> 
>> > Should we also check that the payload length is below the specified max
>> > of 240 bytes?
>> 
>> You'll love this. In v2 the max is actually the whole byte i.e. 255. In
>> v3 they added a length field for these operations for forward
>> compatibility (can now skip unknown new operations). And that's 8
>> bits. So the header + payload for the i2c data can't exceed 255, so
>> there's now an arbitrary 240 byte limit for i2c payload in v3.
>
> I don't really see where the 240 comes from. The spec lists 240 byte
> payload size limit for i2c, spi, and send packet operations, but the
> size of the header is different for those so I can't see how all
> would end up with the same payload length limitation. So to me it seems
> like the max payload limit should be 255-7 for i2c, 255-6 for spi, and
> 255-4 for send packet (since the "size of operation" byte doesn't seem
> to include itself or the "operation byte"). So to me the 240 seems like
> a totally arbitrary limit.

We're in agreement that the spec seems just whimsical about this.

However this patch is for mipi vbt sequence block v2, which doesn't have
the limit. Can you r-b so we can move forward please?

BR,
Jani.



>
>> 
>> BR,
>> Jani.
>> 
>> 
>> >
>> >>  		default:
>> >>  			DRM_ERROR("Unknown operation byte\n");
>> >>  			return 0;
>> >> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
>> >> index 4e87df16e7b3..411b33794536 100644
>> >> --- a/drivers/gpu/drm/i915/intel_bios.h
>> >> +++ b/drivers/gpu/drm/i915/intel_bios.h
>> >> @@ -968,7 +968,7 @@ enum mipi_seq_element {
>> >>  	MIPI_SEQ_ELEM_SEND_PKT,
>> >>  	MIPI_SEQ_ELEM_DELAY,
>> >>  	MIPI_SEQ_ELEM_GPIO,
>> >> -	MIPI_SEQ_ELEM_STATUS,
>> >> +	MIPI_SEQ_ELEM_I2C,		/* sequence block v2+ */
>> >>  	MIPI_SEQ_ELEM_MAX
>> >>  };
>> >>  
>> >> -- 
>> >> 2.1.4
>> >> 
>> >> _______________________________________________
>> >> Intel-gfx mailing list
>> >> Intel-gfx@lists.freedesktop.org
>> >> 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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/15] drm/i915: Adding the parsing logic for the i2c element

[Jani Nikula]

On Thu, 07 Jan 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Mon, Dec 21, 2015 at 03:11:02PM +0200, Jani Nikula wrote:
>> From: vkorjani <vikas.korjani@intel.com>
>> 
>> New sequence element for i2c is been added in the
>> mipi sequence block of the VBT. This patch parses
>> and executes the i2c sequence.
>> 
>> v2: Add i2c_put_adapter call(Jani), rebase
>> 
>> v3: corrected the retry loop(Jani), rebase
>> 
>> v4 by Jani:
>>  - don't put the adapter if get fails
>>  - print an error message if all retries exhausted
>>  - use a for loop
>>  - fix warnings for unused variables
>> 
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: vkorjani <vikas.korjani@intel.com>
>> Signed-off-by: Deepak M <m.deepak@intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 58 ++++++++++++++++++++++++++++++
>>  1 file changed, 58 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> index ba5355506590..8fcfb0f63dc1 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> @@ -31,6 +31,7 @@
>>  #include <drm/drm_panel.h>
>>  #include <linux/slab.h>
>>  #include <video/mipi_display.h>
>> +#include <linux/i2c.h>
>>  #include <asm/intel-mid.h>
>>  #include <video/mipi_display.h>
>>  #include "i915_drv.h"
>> @@ -104,6 +105,62 @@ static struct gpio_table gtable[] = {
>>  	{ GPIO_NC_11_PCONF0, GPIO_NC_11_PAD, 0}
>>  };
>>  
>> +static const u8 *mipi_exec_i2c(struct intel_dsi *intel_dsi, const u8 *data)
>> +{
>> +	struct i2c_adapter *adapter;
>> +	int ret, i;
>> +	u8 reg_offset, payload_size;
>> +	struct i2c_msg msg;
>> +	u8 *transmit_buffer;
>> +	u8 flag, bus_number;
>> +	u16 slave_add;
>> +
>> +	flag = *data++;
>> +	data++; /* index, unused */
>> +	bus_number = *data++;
>> +	slave_add = *(u16 *)(data);
>> +	data += 2;
>> +	reg_offset = *data++;
>> +	payload_size = *data++;
>> +
>> +	adapter = i2c_get_adapter(bus_number);
>
> I'm trying to find who is responsible for registering i2c busses with
> these magic bus numbers. So far I can't see anything that would be doing
> it.

*cough* I'm not sure either *cough*

Would you prefer adding a dummy mipi_exec_i2c that just eats and ignores
the data at this point? It would be a step forward. Now the driver just
gives up on encountering an unknown element.

BR,
Jani.


>
>> +	if (!adapter) {
>> +		DRM_ERROR("i2c_get_adapter(%u)\n", bus_number);
>> +		goto out;
>> +	}
>> +
>> +	transmit_buffer = kmalloc(1 + payload_size, GFP_TEMPORARY);
>> +	if (!transmit_buffer)
>> +		goto out_put;
>> +
>> +	transmit_buffer[0] = reg_offset;
>> +	memcpy(&transmit_buffer[1], data, payload_size);
>> +
>> +	msg.addr = slave_add;
>> +	msg.flags = 0;
>> +	msg.len = payload_size + 1;
>> +	msg.buf = &transmit_buffer[0];
>> +
>> +	for (i = 0; i < 6; i++) {
>> +		ret = i2c_transfer(adapter, &msg, 1);
>> +		if (ret == 1) {
>> +			goto out_free;
>> +		} else if (ret == -EAGAIN) {
>> +			usleep_range(1000, 2500);
>> +		} else {
>> +			break;
>> +		}
>> +	}
>> +
>> +	DRM_ERROR("i2c transfer failed: %d\n", ret);
>> +out_free:
>> +	kfree(transmit_buffer);
>> +out_put:
>> +	i2c_put_adapter(adapter);
>> +out:
>> +	return data + payload_size;
>> +}
>> +
>>  static inline enum port intel_dsi_seq_port_to_port(u8 port)
>>  {
>>  	return port ? PORT_C : PORT_A;
>> @@ -235,6 +292,7 @@ static const fn_mipi_elem_exec exec_elem[] = {
>>  	[MIPI_SEQ_ELEM_SEND_PKT] = mipi_exec_send_packet,
>>  	[MIPI_SEQ_ELEM_DELAY] = mipi_exec_delay,
>>  	[MIPI_SEQ_ELEM_GPIO] = mipi_exec_gpio,
>> +	[MIPI_SEQ_ELEM_I2C] = mipi_exec_i2c,
>>  };
>>  
>>  /*
>> -- 
>> 2.1.4
>> 
>> _______________________________________________
>> 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

[PATCH v3] drm/i915/bios: add support for MIPI sequence block v3

[Jani Nikula]

The changes since the sequence block v2 are:

* The whole MIPI bios data block has a separate 32-bit size field since
  v3, stored after the version. This facilitates big sequences.

* The size of the panel specific sequence blocks has grown to 32
  bits. This facilitates big sequences.

* The elements within sequences now have an 8-bit size field following
  the operation byte. This facilitates skipping unknown new operation
  bytes, i.e. forward compatibility.

v2 (of the patch): use DRM_ERROR for unknown operation byte
v3 (of the patch): even more bounds checking (Ville)

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c          | 96 ++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c |  9 +++
 2 files changed, 94 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 69040a73c09a..15ba52bd2538 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -754,21 +754,33 @@ parse_mipi_config(struct drm_i915_private *dev_priv,
 /* Find the sequence block and size for the given panel. */
 static const u8 *
 find_panel_sequence_block(const struct bdb_mipi_sequence *sequence,
-			  u16 panel_id, u16 *seq_size)
+			  u16 panel_id, u32 *seq_size)
 {
 	u32 total = get_blocksize(sequence);
 	const u8 *data = &sequence->data[0];
 	u8 current_id;
-	u16 current_size;
+	u32 current_size;
+	int header_size = sequence->version >= 3 ? 5 : 3;
 	int index = 0;
 	int i;
 
-	for (i = 0; i < MAX_MIPI_CONFIGURATIONS && index + 3 < total; i++) {
+	/* skip new block size */
+	if (sequence->version >= 3)
+		data += 4;
+
+	for (i = 0; i < MAX_MIPI_CONFIGURATIONS && index < total; i++) {
+		if (index + header_size > total) {
+			DRM_ERROR("Invalid sequence block (header)\n");
+			return NULL;
+		}
+
 		current_id = *(data + index);
-		index++;
+		if (sequence->version >= 3)
+			current_size = *((const u32 *)(data + index + 1));
+		else
+			current_size = *((const u16 *)(data + index + 1));
 
-		current_size = *((const u16 *)(data + index));
-		index += 2;
+		index += header_size;
 
 		if (index + current_size > total) {
 			DRM_ERROR("Invalid sequence block\n");
@@ -826,13 +838,71 @@ static int goto_next_sequence(const u8 *data, int index, int total)
 	return 0;
 }
 
+static int goto_next_sequence_v3(const u8 *data, int index, int total)
+{
+	int seq_end;
+	u16 len;
+
+	/*
+	 * Could skip sequence based on Size of Sequence alone, but also do some
+	 * checking on the structure.
+	 */
+	if (total < 5) {
+		DRM_ERROR("Too small sequence size\n");
+		return 0;
+	}
+
+	seq_end = index + *((const u32 *)(data + 1));
+	if (seq_end > total) {
+		DRM_ERROR("Invalid sequence size\n");
+		return 0;
+	}
+
+	/* Skip Sequence Byte and Size of Sequence. */
+	for (index = index + 5; index < total; index += len) {
+		u8 operation_byte = *(data + index);
+		index++;
+
+		if (operation_byte == MIPI_SEQ_ELEM_END) {
+			if (index != seq_end) {
+				DRM_ERROR("Invalid element structure\n");
+				return 0;
+			}
+			return index;
+		}
+
+		len = *(data + index);
+		index++;
+
+		/*
+		 * FIXME: Would be nice to check elements like for v1/v2 in
+		 * goto_next_sequence() above.
+		 */
+		switch (operation_byte) {
+		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:
+			break;
+		default:
+			DRM_ERROR("Unknown operation byte %u\n",
+				  operation_byte);
+			break;
+		}
+	}
+
+	return 0;
+}
+
 static void
 parse_mipi_sequence(struct drm_i915_private *dev_priv,
 		    const struct bdb_header *bdb)
 {
 	const struct bdb_mipi_sequence *sequence;
 	const u8 *seq_data;
-	u16 seq_size;
+	u32 seq_size;
 	u8 *data;
 	int index = 0;
 
@@ -847,12 +917,13 @@ parse_mipi_sequence(struct drm_i915_private *dev_priv,
 	}
 
 	/* Fail gracefully for forward incompatible sequence block. */
-	if (sequence->version >= 3) {
-		DRM_ERROR("Unable to parse MIPI Sequence Block v3+\n");
+	if (sequence->version >= 4) {
+		DRM_ERROR("Unable to parse MIPI Sequence Block v%u\n",
+			  sequence->version);
 		return;
 	}
 
-	DRM_DEBUG_DRIVER("Found MIPI sequence block\n");
+	DRM_DEBUG_DRIVER("Found MIPI sequence block v%u\n", sequence->version);
 
 	seq_data = find_panel_sequence_block(sequence, panel_type, &seq_size);
 	if (!seq_data)
@@ -875,7 +946,10 @@ parse_mipi_sequence(struct drm_i915_private *dev_priv,
 
 		dev_priv->vbt.dsi.sequence[seq_id] = data + index;
 
-		index = goto_next_sequence(data, index, seq_size);
+		if (sequence->version >= 3)
+			index = goto_next_sequence_v3(data, index, seq_size);
+		else
+			index = goto_next_sequence(data, index, seq_size);
 		if (!index) {
 			DRM_ERROR("Invalid sequence %u\n", seq_id);
 			goto err;
diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 5303d1c446d5..7f67749eb0ef 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -319,6 +319,7 @@ static const char *sequence_name(enum mipi_seq seq_id)
 
 static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
 {
+	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
 	fn_mipi_elem_exec mipi_elem_exec;
 
 	if (!data)
@@ -330,6 +331,10 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
 	/* go to the first element of the sequence */
 	data++;
 
+	/* Skip Size of Sequence. */
+	if (dev_priv->vbt.dsi.seq_version >= 3)
+		data += 4;
+
 	/* parse each byte till we reach end of sequence byte - 0x00 */
 	while (1) {
 		u8 operation_byte = *data++;
@@ -341,6 +346,10 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
 		}
 		mipi_elem_exec = exec_elem[operation_byte];
 
+		/* Skip Size of Operation. */
+		if (dev_priv->vbt.dsi.seq_version >= 3)
+			data++;
+
 		/* execute the element specific rotines */
 		data = mipi_elem_exec(intel_dsi, data);
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[REPLACEMENT PATCH 11/15] drm/i915: skip the i2c element in the generic VBT DSI driver

[Jani Nikula]

Don't choke on unknown elements when we do know how to skip them.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 9bd920809697..b98cec635676 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -229,12 +229,18 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
 	return data;
 }
 
+static const u8 *mipi_exec_i2c_skip(struct intel_dsi *intel_dsi, const u8 *data)
+{
+	return data + *(data + 6) + 7;
+}
+
 typedef const u8 * (*fn_mipi_elem_exec)(struct intel_dsi *intel_dsi,
 					const u8 *data);
 static const fn_mipi_elem_exec exec_elem[] = {
 	[MIPI_SEQ_ELEM_SEND_PKT] = mipi_exec_send_packet,
 	[MIPI_SEQ_ELEM_DELAY] = mipi_exec_delay,
 	[MIPI_SEQ_ELEM_GPIO] = mipi_exec_gpio,
+	[MIPI_SEQ_ELEM_I2C] = mipi_exec_i2c_skip,
 };
 
 /*
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v5] drm/i915: Adding the parsing logic for the i2c element

[Jani Nikula]

New sequence element for i2c is been added in the
mipi sequence block of the VBT. This patch parses
and executes the i2c sequence.

v2: Add i2c_put_adapter call(Jani), rebase

v3: corrected the retry loop(Jani), rebase

v4 by Jani:
 - don't put the adapter if get fails
 - print an error message if all retries exhausted
 - use a for loop
 - fix warnings for unused variables

v5 by Jani:
 - rebase on the skip i2c element patch

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: vkorjani <vikas.korjani@intel.com>
Signed-off-by: Deepak M <m.deepak@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 58 ++++++++++++++++++++++++++++--
 1 file changed, 55 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 1d43e6f37fc1..213d9ef2d712 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -31,6 +31,7 @@
 #include <drm/drm_panel.h>
 #include <linux/slab.h>
 #include <video/mipi_display.h>
+#include <linux/i2c.h>
 #include <asm/intel-mid.h>
 #include <video/mipi_display.h>
 #include "i915_drv.h"
@@ -229,9 +230,60 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
 	return data;
 }
 
-static const u8 *mipi_exec_i2c_skip(struct intel_dsi *intel_dsi, const u8 *data)
+static const u8 *mipi_exec_i2c(struct intel_dsi *intel_dsi, const u8 *data)
 {
-	return data + *(data + 6) + 7;
+	struct i2c_adapter *adapter;
+	int ret, i;
+	u8 reg_offset, payload_size;
+	struct i2c_msg msg;
+	u8 *transmit_buffer;
+	u8 flag, bus_number;
+	u16 slave_add;
+
+	flag = *data++;
+	data++; /* index, unused */
+	bus_number = *data++;
+	slave_add = *(u16 *)(data);
+	data += 2;
+	reg_offset = *data++;
+	payload_size = *data++;
+
+	adapter = i2c_get_adapter(bus_number);
+	if (!adapter) {
+		DRM_ERROR("i2c_get_adapter(%u)\n", bus_number);
+		goto out;
+	}
+
+	transmit_buffer = kmalloc(1 + payload_size, GFP_TEMPORARY);
+	if (!transmit_buffer)
+		goto out_put;
+
+	transmit_buffer[0] = reg_offset;
+	memcpy(&transmit_buffer[1], data, payload_size);
+
+	msg.addr = slave_add;
+	msg.flags = 0;
+	msg.len = payload_size + 1;
+	msg.buf = &transmit_buffer[0];
+
+	for (i = 0; i < 6; i++) {
+		ret = i2c_transfer(adapter, &msg, 1);
+		if (ret == 1) {
+			goto out_free;
+		} else if (ret == -EAGAIN) {
+			usleep_range(1000, 2500);
+		} else {
+			break;
+		}
+	}
+
+	DRM_ERROR("i2c transfer failed: %d\n", ret);
+out_free:
+	kfree(transmit_buffer);
+out_put:
+	i2c_put_adapter(adapter);
+out:
+	return data + payload_size;
 }
 
 typedef const u8 * (*fn_mipi_elem_exec)(struct intel_dsi *intel_dsi,
@@ -240,7 +292,7 @@ static const fn_mipi_elem_exec exec_elem[] = {
 	[MIPI_SEQ_ELEM_SEND_PKT] = mipi_exec_send_packet,
 	[MIPI_SEQ_ELEM_DELAY] = mipi_exec_delay,
 	[MIPI_SEQ_ELEM_GPIO] = mipi_exec_gpio,
-	[MIPI_SEQ_ELEM_I2C] = mipi_exec_i2c_skip,
+	[MIPI_SEQ_ELEM_I2C] = mipi_exec_i2c,
 };
 
 /*
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ failure: Fi.CI.BAT

[Patchwork]

== Summary ==

HEAD is now at ff88655 drm-intel-nightly: 2016y-01m-11d-07h-30m-16s UTC integration manifest
Applying: drm/i915/bios: add proper documentation for the Video BIOS Table (VBT)
Using index info to reconstruct a base tree...
M	Documentation/DocBook/gpu.tmpl
M	drivers/gpu/drm/i915/intel_bios.c
M	drivers/gpu/drm/i915/intel_bios.h
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_bios.h
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_bios.h
Auto-merging drivers/gpu/drm/i915/intel_bios.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_bios.c
Patch failed at 0001 drm/i915/bios: add proper documentation for the Video BIOS Table (VBT)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/15] drm/i915: Adding the parsing logic for the i2c element

[Jani Nikula]

On Mon, 11 Jan 2016, Jani Nikula <jani.nikula@intel.com> wrote:
> On Thu, 07 Jan 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> On Mon, Dec 21, 2015 at 03:11:02PM +0200, Jani Nikula wrote:
>>> From: vkorjani <vikas.korjani@intel.com>
>>> 
>>> New sequence element for i2c is been added in the
>>> mipi sequence block of the VBT. This patch parses
>>> and executes the i2c sequence.
>>> 
>>> v2: Add i2c_put_adapter call(Jani), rebase
>>> 
>>> v3: corrected the retry loop(Jani), rebase
>>> 
>>> v4 by Jani:
>>>  - don't put the adapter if get fails
>>>  - print an error message if all retries exhausted
>>>  - use a for loop
>>>  - fix warnings for unused variables
>>> 
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Signed-off-by: vkorjani <vikas.korjani@intel.com>
>>> Signed-off-by: Deepak M <m.deepak@intel.com>
>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 58 ++++++++++++++++++++++++++++++
>>>  1 file changed, 58 insertions(+)
>>> 
>>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>>> index ba5355506590..8fcfb0f63dc1 100644
>>> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>>> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>>> @@ -31,6 +31,7 @@
>>>  #include <drm/drm_panel.h>
>>>  #include <linux/slab.h>
>>>  #include <video/mipi_display.h>
>>> +#include <linux/i2c.h>
>>>  #include <asm/intel-mid.h>
>>>  #include <video/mipi_display.h>
>>>  #include "i915_drv.h"
>>> @@ -104,6 +105,62 @@ static struct gpio_table gtable[] = {
>>>  	{ GPIO_NC_11_PCONF0, GPIO_NC_11_PAD, 0}
>>>  };
>>>  
>>> +static const u8 *mipi_exec_i2c(struct intel_dsi *intel_dsi, const u8 *data)
>>> +{
>>> +	struct i2c_adapter *adapter;
>>> +	int ret, i;
>>> +	u8 reg_offset, payload_size;
>>> +	struct i2c_msg msg;
>>> +	u8 *transmit_buffer;
>>> +	u8 flag, bus_number;
>>> +	u16 slave_add;
>>> +
>>> +	flag = *data++;
>>> +	data++; /* index, unused */
>>> +	bus_number = *data++;
>>> +	slave_add = *(u16 *)(data);
>>> +	data += 2;
>>> +	reg_offset = *data++;
>>> +	payload_size = *data++;
>>> +
>>> +	adapter = i2c_get_adapter(bus_number);
>>
>> I'm trying to find who is responsible for registering i2c busses with
>> these magic bus numbers. So far I can't see anything that would be doing
>> it.
>
> *cough* I'm not sure either *cough*
>
> Would you prefer adding a dummy mipi_exec_i2c that just eats and ignores
> the data at this point? It would be a step forward. Now the driver just
> gives up on encountering an unknown element.

I did just that, there's now a new 11/15 just skipping, and this one is
moved at the end. Also sent the rebased version of that.

BR,
Jani.


>
> BR,
> Jani.
>
>
>>
>>> +	if (!adapter) {
>>> +		DRM_ERROR("i2c_get_adapter(%u)\n", bus_number);
>>> +		goto out;
>>> +	}
>>> +
>>> +	transmit_buffer = kmalloc(1 + payload_size, GFP_TEMPORARY);
>>> +	if (!transmit_buffer)
>>> +		goto out_put;
>>> +
>>> +	transmit_buffer[0] = reg_offset;
>>> +	memcpy(&transmit_buffer[1], data, payload_size);
>>> +
>>> +	msg.addr = slave_add;
>>> +	msg.flags = 0;
>>> +	msg.len = payload_size + 1;
>>> +	msg.buf = &transmit_buffer[0];
>>> +
>>> +	for (i = 0; i < 6; i++) {
>>> +		ret = i2c_transfer(adapter, &msg, 1);
>>> +		if (ret == 1) {
>>> +			goto out_free;
>>> +		} else if (ret == -EAGAIN) {
>>> +			usleep_range(1000, 2500);
>>> +		} else {
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>> +	DRM_ERROR("i2c transfer failed: %d\n", ret);
>>> +out_free:
>>> +	kfree(transmit_buffer);
>>> +out_put:
>>> +	i2c_put_adapter(adapter);
>>> +out:
>>> +	return data + payload_size;
>>> +}
>>> +
>>>  static inline enum port intel_dsi_seq_port_to_port(u8 port)
>>>  {
>>>  	return port ? PORT_C : PORT_A;
>>> @@ -235,6 +292,7 @@ static const fn_mipi_elem_exec exec_elem[] = {
>>>  	[MIPI_SEQ_ELEM_SEND_PKT] = mipi_exec_send_packet,
>>>  	[MIPI_SEQ_ELEM_DELAY] = mipi_exec_delay,
>>>  	[MIPI_SEQ_ELEM_GPIO] = mipi_exec_gpio,
>>> +	[MIPI_SEQ_ELEM_I2C] = mipi_exec_i2c,
>>>  };
>>>  
>>>  /*
>>> -- 
>>> 2.1.4
>>> 
>>> _______________________________________________
>>> 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

Re: [PATCH v5] drm/i915: Adding the parsing logic for the i2c element

[Jani Nikula]

I screwed up something, the authorship for this one should have remained

From: vkorjani <vikas.korjani@intel.com>

Can be fixed while applying, will not resend now.

BR,
Jani.

On Mon, 11 Jan 2016, Jani Nikula <jani.nikula@intel.com> wrote:
> New sequence element for i2c is been added in the
> mipi sequence block of the VBT. This patch parses
> and executes the i2c sequence.
>
> v2: Add i2c_put_adapter call(Jani), rebase
>
> v3: corrected the retry loop(Jani), rebase
>
> v4 by Jani:
>  - don't put the adapter if get fails
>  - print an error message if all retries exhausted
>  - use a for loop
>  - fix warnings for unused variables
>
> v5 by Jani:
>  - rebase on the skip i2c element patch
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: vkorjani <vikas.korjani@intel.com>
> Signed-off-by: Deepak M <m.deepak@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 58 ++++++++++++++++++++++++++++--
>  1 file changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index 1d43e6f37fc1..213d9ef2d712 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -31,6 +31,7 @@
>  #include <drm/drm_panel.h>
>  #include <linux/slab.h>
>  #include <video/mipi_display.h>
> +#include <linux/i2c.h>
>  #include <asm/intel-mid.h>
>  #include <video/mipi_display.h>
>  #include "i915_drv.h"
> @@ -229,9 +230,60 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>  	return data;
>  }
>  
> -static const u8 *mipi_exec_i2c_skip(struct intel_dsi *intel_dsi, const u8 *data)
> +static const u8 *mipi_exec_i2c(struct intel_dsi *intel_dsi, const u8 *data)
>  {
> -	return data + *(data + 6) + 7;
> +	struct i2c_adapter *adapter;
> +	int ret, i;
> +	u8 reg_offset, payload_size;
> +	struct i2c_msg msg;
> +	u8 *transmit_buffer;
> +	u8 flag, bus_number;
> +	u16 slave_add;
> +
> +	flag = *data++;
> +	data++; /* index, unused */
> +	bus_number = *data++;
> +	slave_add = *(u16 *)(data);
> +	data += 2;
> +	reg_offset = *data++;
> +	payload_size = *data++;
> +
> +	adapter = i2c_get_adapter(bus_number);
> +	if (!adapter) {
> +		DRM_ERROR("i2c_get_adapter(%u)\n", bus_number);
> +		goto out;
> +	}
> +
> +	transmit_buffer = kmalloc(1 + payload_size, GFP_TEMPORARY);
> +	if (!transmit_buffer)
> +		goto out_put;
> +
> +	transmit_buffer[0] = reg_offset;
> +	memcpy(&transmit_buffer[1], data, payload_size);
> +
> +	msg.addr = slave_add;
> +	msg.flags = 0;
> +	msg.len = payload_size + 1;
> +	msg.buf = &transmit_buffer[0];
> +
> +	for (i = 0; i < 6; i++) {
> +		ret = i2c_transfer(adapter, &msg, 1);
> +		if (ret == 1) {
> +			goto out_free;
> +		} else if (ret == -EAGAIN) {
> +			usleep_range(1000, 2500);
> +		} else {
> +			break;
> +		}
> +	}
> +
> +	DRM_ERROR("i2c transfer failed: %d\n", ret);
> +out_free:
> +	kfree(transmit_buffer);
> +out_put:
> +	i2c_put_adapter(adapter);
> +out:
> +	return data + payload_size;
>  }
>  
>  typedef const u8 * (*fn_mipi_elem_exec)(struct intel_dsi *intel_dsi,
> @@ -240,7 +292,7 @@ static const fn_mipi_elem_exec exec_elem[] = {
>  	[MIPI_SEQ_ELEM_SEND_PKT] = mipi_exec_send_packet,
>  	[MIPI_SEQ_ELEM_DELAY] = mipi_exec_delay,
>  	[MIPI_SEQ_ELEM_GPIO] = mipi_exec_gpio,
> -	[MIPI_SEQ_ELEM_I2C] = mipi_exec_i2c_skip,
> +	[MIPI_SEQ_ELEM_I2C] = mipi_exec_i2c,
>  };
>  
>  /*

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

✗ failure: Fi.CI.BAT

[Patchwork]

== Summary ==

HEAD is now at ff88655 drm-intel-nightly: 2016y-01m-11d-07h-30m-16s UTC integration manifest
Applying: drm/i915/bios: add proper documentation for the Video BIOS Table (VBT)
Using index info to reconstruct a base tree...
M	Documentation/DocBook/gpu.tmpl
M	drivers/gpu/drm/i915/intel_bios.c
M	drivers/gpu/drm/i915/intel_bios.h
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_bios.h
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_bios.h
Auto-merging drivers/gpu/drm/i915/intel_bios.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_bios.c
Patch failed at 0001 drm/i915/bios: add proper documentation for the Video BIOS Table (VBT)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/bios: add support for MIPI sequence block v3

[Ville Syrjälä]

On Mon, Jan 11, 2016 at 03:15:02PM +0200, Jani Nikula wrote:
> The changes since the sequence block v2 are:
> 
> * The whole MIPI bios data block has a separate 32-bit size field since
>   v3, stored after the version. This facilitates big sequences.
> 
> * The size of the panel specific sequence blocks has grown to 32
>   bits. This facilitates big sequences.
> 
> * The elements within sequences now have an 8-bit size field following
>   the operation byte. This facilitates skipping unknown new operation
>   bytes, i.e. forward compatibility.
> 
> v2 (of the patch): use DRM_ERROR for unknown operation byte
> v3 (of the patch): even more bounds checking (Ville)
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_bios.c          | 96 ++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c |  9 +++
>  2 files changed, 94 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 69040a73c09a..15ba52bd2538 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -754,21 +754,33 @@ parse_mipi_config(struct drm_i915_private *dev_priv,
>  /* Find the sequence block and size for the given panel. */
>  static const u8 *
>  find_panel_sequence_block(const struct bdb_mipi_sequence *sequence,
> -			  u16 panel_id, u16 *seq_size)
> +			  u16 panel_id, u32 *seq_size)
>  {
>  	u32 total = get_blocksize(sequence);
>  	const u8 *data = &sequence->data[0];
>  	u8 current_id;
> -	u16 current_size;
> +	u32 current_size;
> +	int header_size = sequence->version >= 3 ? 5 : 3;
>  	int index = 0;
>  	int i;
>  
> -	for (i = 0; i < MAX_MIPI_CONFIGURATIONS && index + 3 < total; i++) {
> +	/* skip new block size */
> +	if (sequence->version >= 3)
> +		data += 4;
> +
> +	for (i = 0; i < MAX_MIPI_CONFIGURATIONS && index < total; i++) {
> +		if (index + header_size > total) {
> +			DRM_ERROR("Invalid sequence block (header)\n");
> +			return NULL;
> +		}
> +
>  		current_id = *(data + index);
> -		index++;
> +		if (sequence->version >= 3)
> +			current_size = *((const u32 *)(data + index + 1));
> +		else
> +			current_size = *((const u16 *)(data + index + 1));
>  
> -		current_size = *((const u16 *)(data + index));
> -		index += 2;
> +		index += header_size;
>  
>  		if (index + current_size > total) {
>  			DRM_ERROR("Invalid sequence block\n");
> @@ -826,13 +838,71 @@ static int goto_next_sequence(const u8 *data, int index, int total)
>  	return 0;
>  }
>  
> +static int goto_next_sequence_v3(const u8 *data, int index, int total)
> +{
> +	int seq_end;
> +	u16 len;
> +
> +	/*
> +	 * Could skip sequence based on Size of Sequence alone, but also do some
> +	 * checking on the structure.
> +	 */
> +	if (total < 5) {
> +		DRM_ERROR("Too small sequence size\n");
> +		return 0;
> +	}
> +
> +	seq_end = index + *((const u32 *)(data + 1));
> +	if (seq_end > total) {
> +		DRM_ERROR("Invalid sequence size\n");
> +		return 0;
> +	}
> +
> +	/* Skip Sequence Byte and Size of Sequence. */
> +	for (index = index + 5; index < total; index += len) {
> +		u8 operation_byte = *(data + index);
> +		index++;
> +
> +		if (operation_byte == MIPI_SEQ_ELEM_END) {
> +			if (index != seq_end) {
> +				DRM_ERROR("Invalid element structure\n");
> +				return 0;
> +			}
> +			return index;
> +		}
> +
> +		len = *(data + index);
> +		index++;
> +
> +		/*
> +		 * FIXME: Would be nice to check elements like for v1/v2 in
> +		 * goto_next_sequence() above.
> +		 */
> +		switch (operation_byte) {
> +		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:
> +			break;
> +		default:
> +			DRM_ERROR("Unknown operation byte %u\n",
> +				  operation_byte);
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static void
>  parse_mipi_sequence(struct drm_i915_private *dev_priv,
>  		    const struct bdb_header *bdb)
>  {
>  	const struct bdb_mipi_sequence *sequence;
>  	const u8 *seq_data;
> -	u16 seq_size;
> +	u32 seq_size;
>  	u8 *data;
>  	int index = 0;
>  
> @@ -847,12 +917,13 @@ parse_mipi_sequence(struct drm_i915_private *dev_priv,
>  	}
>  
>  	/* Fail gracefully for forward incompatible sequence block. */
> -	if (sequence->version >= 3) {
> -		DRM_ERROR("Unable to parse MIPI Sequence Block v3+\n");
> +	if (sequence->version >= 4) {
> +		DRM_ERROR("Unable to parse MIPI Sequence Block v%u\n",
> +			  sequence->version);
>  		return;
>  	}
>  
> -	DRM_DEBUG_DRIVER("Found MIPI sequence block\n");
> +	DRM_DEBUG_DRIVER("Found MIPI sequence block v%u\n", sequence->version);
>  
>  	seq_data = find_panel_sequence_block(sequence, panel_type, &seq_size);
>  	if (!seq_data)
> @@ -875,7 +946,10 @@ parse_mipi_sequence(struct drm_i915_private *dev_priv,
>  
>  		dev_priv->vbt.dsi.sequence[seq_id] = data + index;
>  
> -		index = goto_next_sequence(data, index, seq_size);
> +		if (sequence->version >= 3)
> +			index = goto_next_sequence_v3(data, index, seq_size);
> +		else
> +			index = goto_next_sequence(data, index, seq_size);
>  		if (!index) {
>  			DRM_ERROR("Invalid sequence %u\n", seq_id);
>  			goto err;
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index 5303d1c446d5..7f67749eb0ef 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -319,6 +319,7 @@ static const char *sequence_name(enum mipi_seq seq_id)
>  
>  static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
>  	fn_mipi_elem_exec mipi_elem_exec;
>  
>  	if (!data)
> @@ -330,6 +331,10 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
>  	/* go to the first element of the sequence */
>  	data++;
>  
> +	/* Skip Size of Sequence. */
> +	if (dev_priv->vbt.dsi.seq_version >= 3)
> +		data += 4;
> +
>  	/* parse each byte till we reach end of sequence byte - 0x00 */
>  	while (1) {
>  		u8 operation_byte = *data++;
> @@ -341,6 +346,10 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
>  		}
>  		mipi_elem_exec = exec_elem[operation_byte];
>  
> +		/* Skip Size of Operation. */
> +		if (dev_priv->vbt.dsi.seq_version >= 3)
> +			data++;
> +
>  		/* execute the element specific rotines */
>  		data = mipi_elem_exec(intel_dsi, data);
>  
> -- 
> 2.1.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [REPLACEMENT PATCH 11/15] drm/i915: skip the i2c element in the generic VBT DSI driver

[Ville Syrjälä]

On Mon, Jan 11, 2016 at 03:29:08PM +0200, Jani Nikula wrote:
> Don't choke on unknown elements when we do know how to skip them.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index 9bd920809697..b98cec635676 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -229,12 +229,18 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>  	return data;
>  }
>  
> +static const u8 *mipi_exec_i2c_skip(struct intel_dsi *intel_dsi, const u8 *data)
> +{
> +	return data + *(data + 6) + 7;
> +}
> +
>  typedef const u8 * (*fn_mipi_elem_exec)(struct intel_dsi *intel_dsi,
>  					const u8 *data);
>  static const fn_mipi_elem_exec exec_elem[] = {
>  	[MIPI_SEQ_ELEM_SEND_PKT] = mipi_exec_send_packet,
>  	[MIPI_SEQ_ELEM_DELAY] = mipi_exec_delay,
>  	[MIPI_SEQ_ELEM_GPIO] = mipi_exec_gpio,
> +	[MIPI_SEQ_ELEM_I2C] = mipi_exec_i2c_skip,
>  };
>  
>  /*
> -- 
> 2.1.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/15] drm/i915/bios: interpret the i2c element

[Ville Syrjälä]

On Mon, Jan 11, 2016 at 02:46:52PM +0200, Jani Nikula wrote:
> On Thu, 07 Jan 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Jan 07, 2016 at 11:31:25AM +0200, Jani Nikula wrote:
> >> On Tue, 05 Jan 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >> > On Mon, Dec 21, 2015 at 03:11:00PM +0200, Jani Nikula wrote:
> >> >> Add parsing of the i2c element, defined in MIPI sequence block v2. Drop
> >> >> the status operation byte while at it, that does not exist.
> >> >> 
> >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/i915/intel_bios.c | 5 +++++
> >> >>  drivers/gpu/drm/i915/intel_bios.h | 2 +-
> >> >>  2 files changed, 6 insertions(+), 1 deletion(-)
> >> >> 
> >> >> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> >> >> index d6eaf32f33e5..45a7a2bc96c6 100644
> >> >> --- a/drivers/gpu/drm/i915/intel_bios.c
> >> >> +++ b/drivers/gpu/drm/i915/intel_bios.c
> >> >> @@ -812,6 +812,11 @@ static int goto_next_sequence(const u8 *data, int index, int total)
> >> >>  		case MIPI_SEQ_ELEM_GPIO:
> >> >>  			len = 2;
> >> >
> >> > Somewhat off topic, but I wonder if this is correct. The "structure"
> >> > diagram shows it as 2 bytes for v1 and v2, but I'm not sure that section
> >> > isn't there just as an example. Later the describing the GPIO block it
> >> > seems to say it's 2 bytes for v1, and three bytes for v2.
> >> 
> >> I've held on to some old spec versions (bdb version 177 has sequence v1,
> >> bdb version 185 has sequence v2). Both v1 and v2 have 2 bytes payload
> >> for the GPIO element.
> >> 
> >> The *meaning* of the first of those bytes has changed from v1->v2
> >> though. Can't do much about that here, it's up to the generic vbt dsi
> >> "driver"...
> >> 
> >> >
> >> >>  			break;
> >> >> +		case MIPI_SEQ_ELEM_I2C:
> >> >> +			if (index + 7 > total)
> >> >> +				return 0;
> >> >> +			len = *(data + index + 6) + 7;
> >> >> +			break;
> >> >
> >> > This one isn't show in the structure diagrams at all, so I guess we get
> >> > to trust the other section. It says this was introduced in v2. Should be
> >> > add a paranoia check for that?
> >> 
> >> The spec with bdb version 185 has this.
> >> 
> >> I contemplated adding a check for v2, but then I thought it probably
> >> doesn't really matter all that much. If we get an i2c elem with v1 and
> >> reject it, we'll probably end up with a black screen. If we just assume
> >> it's an i2c element but it isn't, it'll trip over something else later.
> >> 
> >> > Should we also check that the payload length is below the specified max
> >> > of 240 bytes?
> >> 
> >> You'll love this. In v2 the max is actually the whole byte i.e. 255. In
> >> v3 they added a length field for these operations for forward
> >> compatibility (can now skip unknown new operations). And that's 8
> >> bits. So the header + payload for the i2c data can't exceed 255, so
> >> there's now an arbitrary 240 byte limit for i2c payload in v3.
> >
> > I don't really see where the 240 comes from. The spec lists 240 byte
> > payload size limit for i2c, spi, and send packet operations, but the
> > size of the header is different for those so I can't see how all
> > would end up with the same payload length limitation. So to me it seems
> > like the max payload limit should be 255-7 for i2c, 255-6 for spi, and
> > 255-4 for send packet (since the "size of operation" byte doesn't seem
> > to include itself or the "operation byte"). So to me the 240 seems like
> > a totally arbitrary limit.
> 
> We're in agreement that the spec seems just whimsical about this.
> 
> However this patch is for mipi vbt sequence block v2, which doesn't have
> the limit. Can you r-b so we can move forward please?

Sure. I'll take your word on the v2 vs. v3 thing since the spec is
absolutely useless here.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> BR,
> Jani.
> 
> 
> 
> >
> >> 
> >> BR,
> >> Jani.
> >> 
> >> 
> >> >
> >> >>  		default:
> >> >>  			DRM_ERROR("Unknown operation byte\n");
> >> >>  			return 0;
> >> >> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> >> >> index 4e87df16e7b3..411b33794536 100644
> >> >> --- a/drivers/gpu/drm/i915/intel_bios.h
> >> >> +++ b/drivers/gpu/drm/i915/intel_bios.h
> >> >> @@ -968,7 +968,7 @@ enum mipi_seq_element {
> >> >>  	MIPI_SEQ_ELEM_SEND_PKT,
> >> >>  	MIPI_SEQ_ELEM_DELAY,
> >> >>  	MIPI_SEQ_ELEM_GPIO,
> >> >> -	MIPI_SEQ_ELEM_STATUS,
> >> >> +	MIPI_SEQ_ELEM_I2C,		/* sequence block v2+ */
> >> >>  	MIPI_SEQ_ELEM_MAX
> >> >>  };
> >> >>  
> >> >> -- 
> >> >> 2.1.4
> >> >> 
> >> >> _______________________________________________
> >> >> Intel-gfx mailing list
> >> >> Intel-gfx@lists.freedesktop.org
> >> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> 
> >> -- 
> >> Jani Nikula, Intel Open Source Technology Center
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/15] drm/i915: Adding the parsing logic for the i2c element

[Ville Syrjälä]

On Mon, Jan 11, 2016 at 03:31:27PM +0200, Jani Nikula wrote:
> On Mon, 11 Jan 2016, Jani Nikula <jani.nikula@intel.com> wrote:
> > On Thu, 07 Jan 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >> On Mon, Dec 21, 2015 at 03:11:02PM +0200, Jani Nikula wrote:
> >>> From: vkorjani <vikas.korjani@intel.com>
> >>> 
> >>> New sequence element for i2c is been added in the
> >>> mipi sequence block of the VBT. This patch parses
> >>> and executes the i2c sequence.
> >>> 
> >>> v2: Add i2c_put_adapter call(Jani), rebase
> >>> 
> >>> v3: corrected the retry loop(Jani), rebase
> >>> 
> >>> v4 by Jani:
> >>>  - don't put the adapter if get fails
> >>>  - print an error message if all retries exhausted
> >>>  - use a for loop
> >>>  - fix warnings for unused variables
> >>> 
> >>> Cc: Jani Nikula <jani.nikula@intel.com>
> >>> Signed-off-by: vkorjani <vikas.korjani@intel.com>
> >>> Signed-off-by: Deepak M <m.deepak@intel.com>
> >>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 58 ++++++++++++++++++++++++++++++
> >>>  1 file changed, 58 insertions(+)
> >>> 
> >>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> >>> index ba5355506590..8fcfb0f63dc1 100644
> >>> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> >>> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> >>> @@ -31,6 +31,7 @@
> >>>  #include <drm/drm_panel.h>
> >>>  #include <linux/slab.h>
> >>>  #include <video/mipi_display.h>
> >>> +#include <linux/i2c.h>
> >>>  #include <asm/intel-mid.h>
> >>>  #include <video/mipi_display.h>
> >>>  #include "i915_drv.h"
> >>> @@ -104,6 +105,62 @@ static struct gpio_table gtable[] = {
> >>>  	{ GPIO_NC_11_PCONF0, GPIO_NC_11_PAD, 0}
> >>>  };
> >>>  
> >>> +static const u8 *mipi_exec_i2c(struct intel_dsi *intel_dsi, const u8 *data)
> >>> +{
> >>> +	struct i2c_adapter *adapter;
> >>> +	int ret, i;
> >>> +	u8 reg_offset, payload_size;
> >>> +	struct i2c_msg msg;
> >>> +	u8 *transmit_buffer;
> >>> +	u8 flag, bus_number;
> >>> +	u16 slave_add;
> >>> +
> >>> +	flag = *data++;
> >>> +	data++; /* index, unused */
> >>> +	bus_number = *data++;
> >>> +	slave_add = *(u16 *)(data);
> >>> +	data += 2;
> >>> +	reg_offset = *data++;
> >>> +	payload_size = *data++;
> >>> +
> >>> +	adapter = i2c_get_adapter(bus_number);
> >>
> >> I'm trying to find who is responsible for registering i2c busses with
> >> these magic bus numbers. So far I can't see anything that would be doing
> >> it.
> >
> > *cough* I'm not sure either *cough*
> >
> > Would you prefer adding a dummy mipi_exec_i2c that just eats and ignores
> > the data at this point? It would be a step forward. Now the driver just
> > gives up on encountering an unknown element.
> 
> I did just that, there's now a new 11/15 just skipping, and this one is
> moved at the end. Also sent the rebased version of that.

As far as the i2c bus numbering goes, I think we ought to add a check to
make sure the magic bus number is below __i2c_first_dynamic_bus_num.
That might catch the cases where some firmware related code forgot to
register the magic bus numbers.

Also the spec says 0xff would mean gmbus. So seems like we should handle
that in a special way. But I can't see where it would actually specify
the actual gmbus pins we should use, so I guessing no one actually even
tried to use this option since it simply can't work.

> 
> BR,
> Jani.
> 
> 
> >
> > BR,
> > Jani.
> >
> >
> >>
> >>> +	if (!adapter) {
> >>> +		DRM_ERROR("i2c_get_adapter(%u)\n", bus_number);
> >>> +		goto out;
> >>> +	}
> >>> +
> >>> +	transmit_buffer = kmalloc(1 + payload_size, GFP_TEMPORARY);
> >>> +	if (!transmit_buffer)
> >>> +		goto out_put;
> >>> +
> >>> +	transmit_buffer[0] = reg_offset;
> >>> +	memcpy(&transmit_buffer[1], data, payload_size);
> >>> +
> >>> +	msg.addr = slave_add;
> >>> +	msg.flags = 0;
> >>> +	msg.len = payload_size + 1;
> >>> +	msg.buf = &transmit_buffer[0];
> >>> +
> >>> +	for (i = 0; i < 6; i++) {
> >>> +		ret = i2c_transfer(adapter, &msg, 1);
> >>> +		if (ret == 1) {
> >>> +			goto out_free;
> >>> +		} else if (ret == -EAGAIN) {
> >>> +			usleep_range(1000, 2500);
> >>> +		} else {
> >>> +			break;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	DRM_ERROR("i2c transfer failed: %d\n", ret);
> >>> +out_free:
> >>> +	kfree(transmit_buffer);
> >>> +out_put:
> >>> +	i2c_put_adapter(adapter);
> >>> +out:
> >>> +	return data + payload_size;
> >>> +}
> >>> +
> >>>  static inline enum port intel_dsi_seq_port_to_port(u8 port)
> >>>  {
> >>>  	return port ? PORT_C : PORT_A;
> >>> @@ -235,6 +292,7 @@ static const fn_mipi_elem_exec exec_elem[] = {
> >>>  	[MIPI_SEQ_ELEM_SEND_PKT] = mipi_exec_send_packet,
> >>>  	[MIPI_SEQ_ELEM_DELAY] = mipi_exec_delay,
> >>>  	[MIPI_SEQ_ELEM_GPIO] = mipi_exec_gpio,
> >>> +	[MIPI_SEQ_ELEM_I2C] = mipi_exec_i2c,
> >>>  };
> >>>  
> >>>  /*
> >>> -- 
> >>> 2.1.4
> >>> 
> >>> _______________________________________________
> >>> Intel-gfx mailing list
> >>> Intel-gfx@lists.freedesktop.org
> >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5] drm/i915: Adding the parsing logic for the i2c element

[Jani Nikula]

Pushed the rest of the series *except* this patch. Thanks for the
review.

BR,
Jani.


On Mon, 11 Jan 2016, Jani Nikula <jani.nikula@intel.com> wrote:
> I screwed up something, the authorship for this one should have remained
>
> From: vkorjani <vikas.korjani@intel.com>
>
> Can be fixed while applying, will not resend now.
>
> BR,
> Jani.
>
> On Mon, 11 Jan 2016, Jani Nikula <jani.nikula@intel.com> wrote:
>> New sequence element for i2c is been added in the
>> mipi sequence block of the VBT. This patch parses
>> and executes the i2c sequence.
>>
>> v2: Add i2c_put_adapter call(Jani), rebase
>>
>> v3: corrected the retry loop(Jani), rebase
>>
>> v4 by Jani:
>>  - don't put the adapter if get fails
>>  - print an error message if all retries exhausted
>>  - use a for loop
>>  - fix warnings for unused variables
>>
>> v5 by Jani:
>>  - rebase on the skip i2c element patch
>>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: vkorjani <vikas.korjani@intel.com>
>> Signed-off-by: Deepak M <m.deepak@intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 58 ++++++++++++++++++++++++++++--
>>  1 file changed, 55 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> index 1d43e6f37fc1..213d9ef2d712 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> @@ -31,6 +31,7 @@
>>  #include <drm/drm_panel.h>
>>  #include <linux/slab.h>
>>  #include <video/mipi_display.h>
>> +#include <linux/i2c.h>
>>  #include <asm/intel-mid.h>
>>  #include <video/mipi_display.h>
>>  #include "i915_drv.h"
>> @@ -229,9 +230,60 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>>  	return data;
>>  }
>>  
>> -static const u8 *mipi_exec_i2c_skip(struct intel_dsi *intel_dsi, const u8 *data)
>> +static const u8 *mipi_exec_i2c(struct intel_dsi *intel_dsi, const u8 *data)
>>  {
>> -	return data + *(data + 6) + 7;
>> +	struct i2c_adapter *adapter;
>> +	int ret, i;
>> +	u8 reg_offset, payload_size;
>> +	struct i2c_msg msg;
>> +	u8 *transmit_buffer;
>> +	u8 flag, bus_number;
>> +	u16 slave_add;
>> +
>> +	flag = *data++;
>> +	data++; /* index, unused */
>> +	bus_number = *data++;
>> +	slave_add = *(u16 *)(data);
>> +	data += 2;
>> +	reg_offset = *data++;
>> +	payload_size = *data++;
>> +
>> +	adapter = i2c_get_adapter(bus_number);
>> +	if (!adapter) {
>> +		DRM_ERROR("i2c_get_adapter(%u)\n", bus_number);
>> +		goto out;
>> +	}
>> +
>> +	transmit_buffer = kmalloc(1 + payload_size, GFP_TEMPORARY);
>> +	if (!transmit_buffer)
>> +		goto out_put;
>> +
>> +	transmit_buffer[0] = reg_offset;
>> +	memcpy(&transmit_buffer[1], data, payload_size);
>> +
>> +	msg.addr = slave_add;
>> +	msg.flags = 0;
>> +	msg.len = payload_size + 1;
>> +	msg.buf = &transmit_buffer[0];
>> +
>> +	for (i = 0; i < 6; i++) {
>> +		ret = i2c_transfer(adapter, &msg, 1);
>> +		if (ret == 1) {
>> +			goto out_free;
>> +		} else if (ret == -EAGAIN) {
>> +			usleep_range(1000, 2500);
>> +		} else {
>> +			break;
>> +		}
>> +	}
>> +
>> +	DRM_ERROR("i2c transfer failed: %d\n", ret);
>> +out_free:
>> +	kfree(transmit_buffer);
>> +out_put:
>> +	i2c_put_adapter(adapter);
>> +out:
>> +	return data + payload_size;
>>  }
>>  
>>  typedef const u8 * (*fn_mipi_elem_exec)(struct intel_dsi *intel_dsi,
>> @@ -240,7 +292,7 @@ static const fn_mipi_elem_exec exec_elem[] = {
>>  	[MIPI_SEQ_ELEM_SEND_PKT] = mipi_exec_send_packet,
>>  	[MIPI_SEQ_ELEM_DELAY] = mipi_exec_delay,
>>  	[MIPI_SEQ_ELEM_GPIO] = mipi_exec_gpio,
>> -	[MIPI_SEQ_ELEM_I2C] = mipi_exec_i2c_skip,
>> +	[MIPI_SEQ_ELEM_I2C] = mipi_exec_i2c,
>>  };
>>  
>>  /*

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