drm/i915/bios: mipi sequence block v3, etc.

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

Signed-off-by Jani Nikula <[email protected]>

---
 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(-)

Documentation/DocBook/gpu.tmpl +6 -0

index 6c6e81a9eaf4..f96a5d467f84 100644
@@ -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>

drivers/gpu/drm/i915/intel_bios.c +23 -1

index eba3e0f87181..d487f602a10e 100644
@@ -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.

drivers/gpu/drm/i915/intel_bios.h +28 -10

index 54eac1003a1e..2dc46a98c332 100644
@@ -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 */

Just for OCD.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_bios.h +3 -3

index 2dc46a98c332..21c162e01189 100644
@@ -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_ */

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

drivers/gpu/drm/i915/intel_bios.c +19 -8

index d487f602a10e..91540ab15e0b 100644
@@ -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)

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

drivers/gpu/drm/i915/intel_bios.c +18 -18

index 91540ab15e0b..7393596df37d 100644
@@ -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)

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 7 lines · 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 <[email protected]>

Reviewed-by Daniel Vetter <[email protected]>

Quoted 73 lines

---
 drivers/gpu/drm/i915/intel_bios.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

drivers/gpu/drm/i915/intel_bios.c +18 -18

index 91540ab15e0b..7393596df37d 100644
@@ -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)

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Make the whole thing easier to read.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.c | 76 +++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 34 deletions(-)

drivers/gpu/drm/i915/intel_bios.c +4 -1

index 7393596df37d..9511a5341562 100644
@@ -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,

drivers/gpu/drm/i915/intel_bios.c +27 -25

+		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;
-	}
-

drivers/gpu/drm/i915/intel_bios.c +0 -0

 	if (!dev_priv->vbt.dsi.data)
 		return;
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 24 lines · Jani Nikula

Make the whole thing easier to read.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.c | 76 +++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 34 deletions(-)

drivers/gpu/drm/i915/intel_bios.c +1 -1

index 7393596df37d..9511a5341562 100644
@@ -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;
 }
 

drivers/gpu/drm/i915/intel_bios.c parse_mipi_config(struct drm_i915_private *dev_priv, · line 833

Quoted 13 lines

drivers/gpu/drm/i915/intel_bios.c +13 -0

+/* 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.

Quoted 76 lines

drivers/gpu/drm/i915/intel_bios.c +27 -31

+		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;
-	}
 

drivers/gpu/drm/i915/intel_bios.c parse_mipi_sequence(struct drm_i915_private *dev_priv, · line 897

Quoted 3 lines

drivers/gpu/drm/i915/intel_bios.c +1 -2

-	/* 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 <[email protected]>

Quoted 10 lines

drivers/gpu/drm/i915/intel_bios.c +0 -0

 	if (!dev_priv->vbt.dsi.data)
 		return;
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 25 lines · Daniel Vetter

Quoted 24 lines · Jani Nikula

Make the whole thing easier to read.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.c | 76 +++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 34 deletions(-)

drivers/gpu/drm/i915/intel_bios.c +1 -1

index 7393596df37d..9511a5341562 100644
@@ -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;
 }
 

Quoted 16 lines

drivers/gpu/drm/i915/intel_bios.c parse_mipi_config(struct drm_i915_private *dev_priv, · line 833

Quoted 13 lines

drivers/gpu/drm/i915/intel_bios.c +13 -0

+/* 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. ;)

Quoted 76 lines

Quoted 76 lines

drivers/gpu/drm/i915/intel_bios.c +27 -31

+		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;
-	}
 

Quoted 5 lines

drivers/gpu/drm/i915/intel_bios.c parse_mipi_sequence(struct drm_i915_private *dev_priv, · line 897

Quoted 3 lines

drivers/gpu/drm/i915/intel_bios.c +1 -2

-	/* 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?

Make everything a bit more readable and clear.

Signed-off-by Jani Nikula <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/i915_drv.h +1 -1

index 9b82c4532893..07c4fc539680 100644
@@ -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;

drivers/gpu/drm/i915/intel_bios.c +54 -104

index 9511a5341562..d6eaf32f33e5 100644
@@ -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));
 }
 

drivers/gpu/drm/i915/intel_bios.h +2 -2

index 21c162e01189..4e87df16e7b3 100644
@@ -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,

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +15 -7

index a5e99ac305da..45512e0df57a 100644
@@ -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++;

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +3 -6

index 45512e0df57a..ba5355506590 100644
@@ -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)
 

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +2 -3

+				  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);

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 37 lines · 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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +3 -6

index 45512e0df57a..ba5355506590 100644
@@ -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)
 

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data) · line 280

Quoted 10 lines

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +4 -4

 	/* 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 <[email protected]>

Quoted 18 lines

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +2 -3

+				  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);

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.c | 5 +++++
 drivers/gpu/drm/i915/intel_bios.h | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_bios.c +0 -0

index d6eaf32f33e5..45a7a2bc96c6 100644

drivers/gpu/drm/i915/intel_bios.c +0 -0

 		default:
 			DRM_ERROR("Unknown operation byte\n");
 			return 0;

drivers/gpu/drm/i915/intel_bios.h +1 -1

index 4e87df16e7b3..411b33794536 100644
@@ -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
 };
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 9 lines · 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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.c | 5 +++++
 drivers/gpu/drm/i915/intel_bios.h | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_bios.c static int goto_next_sequence(const u8 *data, int index, int total) · line 813

Quoted 7 lines

drivers/gpu/drm/i915/intel_bios.c +0 -0

index d6eaf32f33e5..45a7a2bc96c6 100644
@@ -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.

drivers/gpu/drm/i915/intel_bios.c static int goto_next_sequence(const u8 *data, int index, int total) · line 819

Quoted 7 lines

drivers/gpu/drm/i915/intel_bios.c +5 -0

 			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?

Quoted 24 lines

drivers/gpu/drm/i915/intel_bios.c +0 -0

 		default:
 			DRM_ERROR("Unknown operation byte\n");
 			return 0;

drivers/gpu/drm/i915/intel_bios.h +1 -1

index 4e87df16e7b3..411b33794536 100644
@@ -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
 };
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 10 lines · Ville Syrjälä

Quoted 9 lines · 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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.c | 5 +++++
 drivers/gpu/drm/i915/intel_bios.h | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

Quoted 12 lines

drivers/gpu/drm/i915/intel_bios.c static int goto_next_sequence(const u8 *data, int index, int total) · line 813

Quoted 7 lines

drivers/gpu/drm/i915/intel_bios.c +0 -0

index d6eaf32f33e5..45a7a2bc96c6 100644
@@ -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"...

Quoted 11 lines

drivers/gpu/drm/i915/intel_bios.c static int goto_next_sequence(const u8 *data, int index, int total) · line 819

Quoted 7 lines

drivers/gpu/drm/i915/intel_bios.c +5 -0

 			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.

Quoted 2 lines

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.

Quoted 24 lines

Quoted 24 lines

drivers/gpu/drm/i915/intel_bios.c +0 -0

 		default:
 			DRM_ERROR("Unknown operation byte\n");
 			return 0;

drivers/gpu/drm/i915/intel_bios.h +1 -1

index 4e87df16e7b3..411b33794536 100644
@@ -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
 };
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center

Quoted 51 lines · Jani Nikula

Quoted 10 lines · Ville Syrjälä

Quoted 9 lines · 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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.c | 5 +++++
 drivers/gpu/drm/i915/intel_bios.h | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

Quoted 12 lines

drivers/gpu/drm/i915/intel_bios.c static int goto_next_sequence(const u8 *data, int index, int total) · line 813

Quoted 7 lines

drivers/gpu/drm/i915/intel_bios.c +0 -0

index d6eaf32f33e5..45a7a2bc96c6 100644
@@ -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"...

Quoted 11 lines

drivers/gpu/drm/i915/intel_bios.c static int goto_next_sequence(const u8 *data, int index, int total) · line 819

Quoted 7 lines

drivers/gpu/drm/i915/intel_bios.c +5 -0

 			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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 33 lines

BR,
Jani.

Quoted 24 lines

Quoted 24 lines

drivers/gpu/drm/i915/intel_bios.c +0 -0

 		default:
 			DRM_ERROR("Unknown operation byte\n");
 			return 0;

drivers/gpu/drm/i915/intel_bios.h +1 -1

index 4e87df16e7b3..411b33794536 100644
@@ -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
 };
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center

Quoted 61 lines · Ville Syrjälä

Quoted 51 lines · Jani Nikula

Quoted 10 lines · Ville Syrjälä

Quoted 9 lines · 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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.c | 5 +++++
 drivers/gpu/drm/i915/intel_bios.h | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

Quoted 12 lines

drivers/gpu/drm/i915/intel_bios.c static int goto_next_sequence(const u8 *data, int index, int total) · line 813

Quoted 7 lines

drivers/gpu/drm/i915/intel_bios.c +0 -0

index d6eaf32f33e5..45a7a2bc96c6 100644
@@ -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"...

Quoted 11 lines

drivers/gpu/drm/i915/intel_bios.c static int goto_next_sequence(const u8 *data, int index, int total) · line 819

Quoted 7 lines

drivers/gpu/drm/i915/intel_bios.c +5 -0

 			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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 8 lines

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.

Quoted 33 lines

Quoted 33 lines

BR,
Jani.

Quoted 24 lines

Quoted 24 lines

drivers/gpu/drm/i915/intel_bios.c +0 -0

 		default:
 			DRM_ERROR("Unknown operation byte\n");
 			return 0;

drivers/gpu/drm/i915/intel_bios.h +1 -1

index 4e87df16e7b3..411b33794536 100644
@@ -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
 };
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center

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

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.h | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/i915/intel_bios.h +0 -0

index 411b33794536..6146f1b0cf48 100644

drivers/gpu/drm/i915/intel_bios.h +0 -0

 	MIPI_SEQ_MAX
 };
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 12 lines · Jani Nikula

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

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.h | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/i915/intel_bios.h +0 -0

index 411b33794536..6146f1b0cf48 100644

drivers/gpu/drm/i915/intel_bios.h enum mipi_seq { · line 965

Quoted 6 lines

drivers/gpu/drm/i915/intel_bios.h +3 -0

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

Quoted 11 lines

drivers/gpu/drm/i915/intel_bios.h +0 -0

 	MIPI_SEQ_MAX
 };
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 27 lines · Ville Syrjälä

Quoted 12 lines · Jani Nikula

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

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.h | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/i915/intel_bios.h +0 -0

index 411b33794536..6146f1b0cf48 100644

drivers/gpu/drm/i915/intel_bios.h enum mipi_seq { · line 965

Quoted 6 lines

drivers/gpu/drm/i915/intel_bios.h +3 -0

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

Quoted 2 lines

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.

Quoted 11 lines

Quoted 11 lines

drivers/gpu/drm/i915/intel_bios.h +0 -0

 	MIPI_SEQ_MAX
 };
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center

From: vkorjani <[email protected]>

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 <[email protected]>

Signed-off-by vkorjani <[email protected]>

Signed-off-by Deepak M <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 58 ++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +10 -0

index ba5355506590..8fcfb0f63dc1 100644
@@ -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;

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +38 -0

+	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,
 };
 
 /*

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 55 lines · Jani Nikula

From: vkorjani <[email protected]>

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 <[email protected]>

Signed-off-by vkorjani <[email protected]>

Signed-off-by Deepak M <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 58 ++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +16 -0

index ba5355506590..8fcfb0f63dc1 100644
@@ -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;

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c static struct gpio_table gtable[] = { · line 126

Quoted 4 lines

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +4 -0

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

Quoted 56 lines

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +38 -0

+	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,
 };
 
 /*

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 56 lines · Ville Syrjälä

Quoted 55 lines · Jani Nikula

From: vkorjani <[email protected]>

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 <[email protected]>

Signed-off-by vkorjani <[email protected]>

Signed-off-by Deepak M <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 58 ++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +16 -0

index ba5355506590..8fcfb0f63dc1 100644
@@ -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;

Quoted 8 lines

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c static struct gpio_table gtable[] = { · line 126

Quoted 4 lines

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +4 -0

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

Quoted 56 lines

Quoted 56 lines

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +38 -0

+	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,
 };
 
 /*

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 66 lines · Jani Nikula

Quoted 64 lines · Ville Syrjälä

Quoted 49 lines · Jani Nikula

From: vkorjani <[email protected]>

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 <[email protected]>

Signed-off-by vkorjani <[email protected]>

Signed-off-by Deepak M <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 58 ++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +10 -0

index ba5355506590..8fcfb0f63dc1 100644
@@ -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;

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c static struct gpio_table gtable[] = { · line 126

Quoted 10 lines

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +10 -0

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

Quoted 5 lines

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

Quoted 61 lines

BR,
Jani.

Quoted 56 lines

Quoted 56 lines

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +38 -0

+	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,
 };
 
 /*

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center

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

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +6 -0

index 9bd920809697..b98cec635676 100644
@@ -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,
 };
 
 /*

Signature · 1 line

2.1.4

New sequences, new operations within sequences.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.h | 5 +++++
 1 file changed, 5 insertions(+)

drivers/gpu/drm/i915/intel_bios.h +5 -0

index 6146f1b0cf48..350d4e0f75a4 100644
@@ -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
 };
 

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx