[PATCH 0/2] Support for new MIPI Blocks in VBT

[PATCH 0/2] Support for new MIPI Blocks in VBT

[Shobhit Kumar]

Hi,
To support a plethora of MIPI panels, the VBT has been enhanced to support
multiple panels in a generic manner. This involves adding one MIPI configuration
block (#52) which provide all panel specific configuration data along with the 
dphy configuration information as per panel/tcon specs. Also a sequence (#53) block 
is added to provide required sequences for panel enabling.

The driver will use this parsed data to enable the panel which is described in these
two block. The i-g-t tool intel_parse_bios changes to parse these new block is WIP.

The generic driver will be as one of sub-encoder driver in the existing design to support 
MIPI. Followup patches for this driver will come next.

Regards
Shobhit

Shobhit Kumar (2):
  drm/i915: Update VBT data structures to have MIPI block enhancements
  drm/i915: Add parsing support for new MIPI blocks in VBT

 drivers/gpu/drm/i915/i915_drv.h   |   6 ++
 drivers/gpu/drm/i915/intel_bios.c | 178 ++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_bios.h | 186 ++++++++++++++++++++++++++++++++------
 drivers/gpu/drm/i915/intel_dsi.h  |   3 +
 4 files changed, 340 insertions(+), 33 deletions(-)

-- 
1.8.3.2

[PATCH 1/2] drm/i915: Update VBT data structures to have MIPI block enhancements

[Shobhit Kumar]

MIPI Block #52 which provides configuration details for the MIPI panel
including dphy settings as per panel and tcon specs

Block #53 gives information on panel enable sequences

Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c |   5 +-
 drivers/gpu/drm/i915/intel_bios.h | 152 +++++++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_dsi.h  |   2 +
 3 files changed, 130 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 86b95ca..cf0b25d 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -30,6 +30,7 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 #include "intel_bios.h"
+#include "intel_dsi.h"
 
 #define	SLAVE_ADDR1	0x70
 #define	SLAVE_ADDR2	0x72
@@ -599,14 +600,14 @@ parse_mipi(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
 {
 	struct bdb_mipi *mipi;
 
-	mipi = find_section(bdb, BDB_MIPI);
+	mipi = find_section(bdb, BDB_MIPI_CONFIG);
 	if (!mipi) {
 		DRM_DEBUG_KMS("No MIPI BDB found");
 		return;
 	}
 
 	/* XXX: add more info */
-	dev_priv->vbt.dsi.panel_id = mipi->panel_id;
+	dev_priv->vbt.dsi.panel_id = MIPI_DSI_GENERIC_PANEL_ID;
 }
 
 static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 282de5e..8345e0e 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -104,7 +104,8 @@ struct vbios_data {
 #define BDB_LVDS_LFP_DATA	 42
 #define BDB_LVDS_BACKLIGHT	 43
 #define BDB_LVDS_POWER		 44
-#define BDB_MIPI		 50
+#define BDB_MIPI_CONFIG		 52
+#define BDB_MIPI_SEQUENCE	 53
 #define BDB_SKIP		254 /* VBIOS private block, ignore */
 
 struct bdb_general_features {
@@ -711,44 +712,141 @@ int intel_parse_bios(struct drm_device *dev);
 #define DVO_PORT_DPD	9
 #define DVO_PORT_DPA	10
 
-/* MIPI DSI panel info */
-struct bdb_mipi {
+/* Block 52 contains MIPI Panel info
+ * 6 such enteries will there. Index into correct
+ * entery is based on the panel_index in #40 LFP
+ */
+#define MAX_MIPI_CONFIGURATIONS	6
+struct mipi_config {
 	u16 panel_id;
-	u16 bridge_revision;
 
-	/* General params */
+	/* General Params */
 	u32 dithering:1;
-	u32 bpp_pixel_format:1;
 	u32 rsvd1:1;
-	u32 dphy_valid:1;
-	u32 resvd2:28;
-
-	u16 port_info;
-	u16 rsvd3:2;
-	u16 num_lanes:2;
-	u16 rsvd4:12;
-
-	/* DSI config */
-	u16 virt_ch_num:2;
-	u16 vtm:2;
-	u16 rsvd5:12;
+	u32 panel_type:1;
+	u32 panel_arch_type:2;
+	u32 cmd_mode:1;
+	u32 vtm:2;
+	u32 cabc:1;
+	u32 pwm_blc:1;
+
+	/* Bit 13:10
+	 * 000 - Reserved, 001 - RGB565, 002 - RGB666,
+	 * 011 - RGB666Loosely packed, 100 - RGB888,
+	 * others - rsvd
+	 */
+	u32 videomode_color_format:4;
 
-	u32 dsi_clock;
+	/* Bit 15:14
+	 * 0 - No rotation, 1 - 90 degree
+	 * 2 - 180 degree, 3 - 270 degree
+	 */
+	u32 rotation:2;
+	u32 bta:1;
+	u32 rsvd2:15;
+
+	/* 2 byte Port Description */
+	u16 dual_link:2;
+	u16 lane_cnt:2;
+	u16 rsvd3:12;
+
+	/* 2 byte DSI COntroller params */
+	/* 0 - Using DSI PHY, 1 - TE usage */
+	u16 dsi_usage:1;
+	u16 rsvd4:15;
+
+	u8 rsvd5[5];
+	u32 dsi_ddr_clk;
 	u32 bridge_ref_clk;
-	u16 rsvd_pwr;
 
-	/* Dphy Params */
-	u32 prepare_cnt:5;
-	u32 rsvd6:3;
+	u8 byte_clk_sel:2;
+	u8 rsvd6:6;
+
+	/* DPHY Flags */
+	u16 dphy_param_valid:1;
+	u16 eot_disabled:1;
+	u16 clk_stop:1;
+	u16 rsvd7:13;
+
+	u32 hs_tx_timeout;
+	u32 lp_rx_timeout;
+	u32 turn_around_timeout;
+	u32 device_reset_timer;
+	u32 master_init_timer;
+	u32 dbi_bw_timer;
+	u32 lp_byte_clk_val;
+
+	/*  4 byte Dphy Params */
+	u32 prepare_cnt:6;
+	u32 rsvd8:2;
 	u32 clk_zero_cnt:8;
 	u32 trail_cnt:5;
-	u32 rsvd7:3;
+	u32 rsvd9:3;
 	u32 exit_zero_cnt:6;
-	u32 rsvd8:2;
+	u32 rsvd10:2;
 
-	u32 hl_switch_cnt;
-	u32 lp_byte_clk;
 	u32 clk_lane_switch_cnt;
+	u32 hl_switch_cnt;
+
+	u32 rsvd11[6];
+
+	/* timings based on dphy spec */
+	u8 tclk_miss;
+	u8 tclk_post;
+	u8 rsvd12;
+	u8 tclk_pre;
+	u8 tclk_prepare;
+	u8 tclk_settle;
+	u8 tclk_term_enable;
+	u8 tclk_trail;
+	u16 tclk_prepare_clkzero;
+	u8 rsvd13;
+	u8 td_term_enable;
+	u8 teot;
+	u8 ths_exit;
+	u8 ths_prepare;
+	u16 ths_prepare_hszero;
+	u8 rsvd14;
+	u8 ths_settle;
+	u8 ths_skip;
+	u8 ths_trail;
+	u8 tinit;
+	u8 tlpx;
+	u8 rsvd15[3];
+
+	/* GPIOs */
+	u8 panel_enable;
+	u8 bl_enable;
+	u8 pwm_enable;
+	u8 reset_r_n;
+	u8 pwr_down_r;
+	u8 stdby_r_n;
+
 } __packed;
 
+struct bdb_mipi_config {
+	struct mipi_config config[0];
+};
+
+/* Block 52 contains MIPI configuration block
+ * 6 * bdb_mipi_config, followed by 6 pps data
+ * block below
+ */
+struct mipi_pps_data {
+	u16 panel_on_delay;
+	u16 bl_enable_delay;
+	u16 bl_disable_delay;
+	u16 panel_off_delay;
+	u16 panel_power_cycle_delay;
+};
+
+/* Block 53 contains MIPI sequences as needed by the panel
+ * for enabling it. This block can be variable in size and
+ * can be maximum of 6 blocks
+ */
+struct bdb_mipi_sequence {
+	u8 version;
+	void *data;
+};
+
 #endif /* _I830_BIOS_H_ */
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index b4a27ce..a0e42db 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -120,4 +120,6 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
 extern void vlv_enable_dsi_pll(struct intel_encoder *encoder);
 extern void vlv_disable_dsi_pll(struct intel_encoder *encoder);
 
+#define MIPI_DSI_GENERIC_PANEL_ID	1
+
 #endif /* _INTEL_DSI_H */
-- 
1.8.3.2

Re: [PATCH 1/2] drm/i915: Update VBT data structures to have MIPI block enhancements

[Jani Nikula]

On Thu, 13 Feb 2014, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> MIPI Block #52 which provides configuration details for the MIPI panel
> including dphy settings as per panel and tcon specs
>
> Block #53 gives information on panel enable sequences
>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_bios.c |   5 +-
>  drivers/gpu/drm/i915/intel_bios.h | 152 +++++++++++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_dsi.h  |   2 +
>  3 files changed, 130 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 86b95ca..cf0b25d 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -30,6 +30,7 @@
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
>  #include "intel_bios.h"
> +#include "intel_dsi.h"

Hmm, I think I'd like it better if the interface from bios to elsewhere
was defined in intel_bios.h, i.e. move the required #defines from
intel_dsi.h to intel_bios.h.

>  
>  #define	SLAVE_ADDR1	0x70
>  #define	SLAVE_ADDR2	0x72
> @@ -599,14 +600,14 @@ parse_mipi(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
>  {
>  	struct bdb_mipi *mipi;
>  
> -	mipi = find_section(bdb, BDB_MIPI);
> +	mipi = find_section(bdb, BDB_MIPI_CONFIG);
>  	if (!mipi) {
>  		DRM_DEBUG_KMS("No MIPI BDB found");
>  		return;
>  	}
>  
>  	/* XXX: add more info */
> -	dev_priv->vbt.dsi.panel_id = mipi->panel_id;
> +	dev_priv->vbt.dsi.panel_id = MIPI_DSI_GENERIC_PANEL_ID;
>  }
>  
>  static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index 282de5e..8345e0e 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -104,7 +104,8 @@ struct vbios_data {
>  #define BDB_LVDS_LFP_DATA	 42
>  #define BDB_LVDS_BACKLIGHT	 43
>  #define BDB_LVDS_POWER		 44
> -#define BDB_MIPI		 50
> +#define BDB_MIPI_CONFIG		 52
> +#define BDB_MIPI_SEQUENCE	 53
>  #define BDB_SKIP		254 /* VBIOS private block, ignore */
>  
>  struct bdb_general_features {
> @@ -711,44 +712,141 @@ int intel_parse_bios(struct drm_device *dev);
>  #define DVO_PORT_DPD	9
>  #define DVO_PORT_DPA	10
>  
> -/* MIPI DSI panel info */
> -struct bdb_mipi {
> +/* Block 52 contains MIPI Panel info
> + * 6 such enteries will there. Index into correct
> + * entery is based on the panel_index in #40 LFP
> + */
> +#define MAX_MIPI_CONFIGURATIONS	6
> +struct mipi_config {
>  	u16 panel_id;
> -	u16 bridge_revision;
>  
> -	/* General params */
> +	/* General Params */
>  	u32 dithering:1;
> -	u32 bpp_pixel_format:1;
>  	u32 rsvd1:1;
> -	u32 dphy_valid:1;
> -	u32 resvd2:28;
> -
> -	u16 port_info;
> -	u16 rsvd3:2;
> -	u16 num_lanes:2;
> -	u16 rsvd4:12;
> -
> -	/* DSI config */
> -	u16 virt_ch_num:2;
> -	u16 vtm:2;
> -	u16 rsvd5:12;
> +	u32 panel_type:1;
> +	u32 panel_arch_type:2;
> +	u32 cmd_mode:1;
> +	u32 vtm:2;
> +	u32 cabc:1;
> +	u32 pwm_blc:1;
> +
> +	/* Bit 13:10
> +	 * 000 - Reserved, 001 - RGB565, 002 - RGB666,
> +	 * 011 - RGB666Loosely packed, 100 - RGB888,
> +	 * others - rsvd
> +	 */
> +	u32 videomode_color_format:4;
>  
> -	u32 dsi_clock;
> +	/* Bit 15:14
> +	 * 0 - No rotation, 1 - 90 degree
> +	 * 2 - 180 degree, 3 - 270 degree
> +	 */
> +	u32 rotation:2;
> +	u32 bta:1;
> +	u32 rsvd2:15;
> +
> +	/* 2 byte Port Description */
> +	u16 dual_link:2;
> +	u16 lane_cnt:2;
> +	u16 rsvd3:12;
> +
> +	/* 2 byte DSI COntroller params */
> +	/* 0 - Using DSI PHY, 1 - TE usage */
> +	u16 dsi_usage:1;
> +	u16 rsvd4:15;
> +
> +	u8 rsvd5[5];
> +	u32 dsi_ddr_clk;
>  	u32 bridge_ref_clk;
> -	u16 rsvd_pwr;
>  
> -	/* Dphy Params */
> -	u32 prepare_cnt:5;
> -	u32 rsvd6:3;
> +	u8 byte_clk_sel:2;
> +	u8 rsvd6:6;

Per the spec you sent me, there's one more reserved byte here.

> +
> +	/* DPHY Flags */
> +	u16 dphy_param_valid:1;
> +	u16 eot_disabled:1;
> +	u16 clk_stop:1;
> +	u16 rsvd7:13;
> +
> +	u32 hs_tx_timeout;
> +	u32 lp_rx_timeout;
> +	u32 turn_around_timeout;
> +	u32 device_reset_timer;
> +	u32 master_init_timer;
> +	u32 dbi_bw_timer;
> +	u32 lp_byte_clk_val;
> +
> +	/*  4 byte Dphy Params */
> +	u32 prepare_cnt:6;
> +	u32 rsvd8:2;

Per the spec you sent me, prepare_cnt is 5 bits, reserved 3 bits.

>  	u32 clk_zero_cnt:8;
>  	u32 trail_cnt:5;
> -	u32 rsvd7:3;
> +	u32 rsvd9:3;
>  	u32 exit_zero_cnt:6;
> -	u32 rsvd8:2;
> +	u32 rsvd10:2;

For this reserved field the spec is clearly bogus...

>  
> -	u32 hl_switch_cnt;
> -	u32 lp_byte_clk;
>  	u32 clk_lane_switch_cnt;
> +	u32 hl_switch_cnt;
> +
> +	u32 rsvd11[6];
> +
> +	/* timings based on dphy spec */
> +	u8 tclk_miss;
> +	u8 tclk_post;
> +	u8 rsvd12;
> +	u8 tclk_pre;
> +	u8 tclk_prepare;
> +	u8 tclk_settle;
> +	u8 tclk_term_enable;
> +	u8 tclk_trail;
> +	u16 tclk_prepare_clkzero;
> +	u8 rsvd13;
> +	u8 td_term_enable;
> +	u8 teot;
> +	u8 ths_exit;
> +	u8 ths_prepare;
> +	u16 ths_prepare_hszero;
> +	u8 rsvd14;
> +	u8 ths_settle;
> +	u8 ths_skip;
> +	u8 ths_trail;
> +	u8 tinit;
> +	u8 tlpx;
> +	u8 rsvd15[3];

Per the spec you sent me, there's 1 byte reserved, and 5 bytes of GPIO
indexes below.

All in all, the size of the struct is different from the spec, shifting
everything for panel_type > 0. Which one is wrong?

> +
> +	/* GPIOs */
> +	u8 panel_enable;
> +	u8 bl_enable;
> +	u8 pwm_enable;
> +	u8 reset_r_n;
> +	u8 pwr_down_r;
> +	u8 stdby_r_n;
> +
>  } __packed;

All around I would like it if the field names were slightly more
descriptive by themselves, particularly for the most important ones,
with #defines for the values in some cases. For example panel_type above
could clearly be "is_bridge" or similar (now it's confusing with the
panel_type in intel_bios.c). Same for any booleans that could be
expressed as "is_something" or "something_enabled". Color formats and
rotations could have defines, so you wouldn't need to add comments for
them at all. Same for byte_clk_sel.

I definitely do *not* mean you should rewrite all of them. If you want,
I can reply with detailed suggestions on a per field basis.

>  
> +struct bdb_mipi_config {
> +	struct mipi_config config[0];

Why isn't this struct:

	struct mipi_config config[MAX_MIPI_CONFIGURATIONS];
        struct mipi_pps_data pps[MAX_MIPI_CONFIGURATIONS];

and you could replace this ugly bit in patch 2/2:

+	pps = (struct mipi_pps_data *) &start->config[MAX_MIPI_CONFIGURATIONS];

with:

+	pps = &start->pps[panel_type];

> +};
> +
> +/* Block 52 contains MIPI configuration block
> + * 6 * bdb_mipi_config, followed by 6 pps data
> + * block below
> + */
> +struct mipi_pps_data {
> +	u16 panel_on_delay;
> +	u16 bl_enable_delay;
> +	u16 bl_disable_delay;
> +	u16 panel_off_delay;
> +	u16 panel_power_cycle_delay;
> +};

I'd like the unconventional units mentioned in a comment.

> +
> +/* Block 53 contains MIPI sequences as needed by the panel
> + * for enabling it. This block can be variable in size and
> + * can be maximum of 6 blocks
> + */
> +struct bdb_mipi_sequence {
> +	u8 version;
> +	void *data;
> +};
> +
>  #endif /* _I830_BIOS_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
> index b4a27ce..a0e42db 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -120,4 +120,6 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
>  extern void vlv_enable_dsi_pll(struct intel_encoder *encoder);
>  extern void vlv_disable_dsi_pll(struct intel_encoder *encoder);
>  
> +#define MIPI_DSI_GENERIC_PANEL_ID	1
> +
>  #endif /* _INTEL_DSI_H */
> -- 
> 1.8.3.2
>

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/2] drm/i915: Update VBT data structures to have MIPI block enhancements

[Shobhit Kumar]

Hi

On Thursday 13 February 2014 12:47 PM, Jani Nikula wrote:
> On Thu, 13 Feb 2014, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>> MIPI Block #52 which provides configuration details for the MIPI panel
>> including dphy settings as per panel and tcon specs
>>
>> Block #53 gives information on panel enable sequences
>>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_bios.c |   5 +-
>>   drivers/gpu/drm/i915/intel_bios.h | 152 +++++++++++++++++++++++++++++++-------
>>   drivers/gpu/drm/i915/intel_dsi.h  |   2 +
>>   3 files changed, 130 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index 86b95ca..cf0b25d 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -30,6 +30,7 @@
>>   #include <drm/i915_drm.h>
>>   #include "i915_drv.h"
>>   #include "intel_bios.h"
>> +#include "intel_dsi.h"
>
> Hmm, I think I'd like it better if the interface from bios to elsewhere
> was defined in intel_bios.h, i.e. move the required #defines from
> intel_dsi.h to intel_bios.h.
>

Ok. Yeah looks a little awkward. Will move

>>
>>   #define	SLAVE_ADDR1	0x70
>>   #define	SLAVE_ADDR2	0x72
>> @@ -599,14 +600,14 @@ parse_mipi(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
>>   {
>>   	struct bdb_mipi *mipi;
>>
>> -	mipi = find_section(bdb, BDB_MIPI);
>> +	mipi = find_section(bdb, BDB_MIPI_CONFIG);
>>   	if (!mipi) {
>>   		DRM_DEBUG_KMS("No MIPI BDB found");
>>   		return;
>>   	}
>>
>>   	/* XXX: add more info */
>> -	dev_priv->vbt.dsi.panel_id = mipi->panel_id;
>> +	dev_priv->vbt.dsi.panel_id = MIPI_DSI_GENERIC_PANEL_ID;
>>   }
>>
>>   static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
>> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
>> index 282de5e..8345e0e 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.h
>> +++ b/drivers/gpu/drm/i915/intel_bios.h
>> @@ -104,7 +104,8 @@ struct vbios_data {
>>   #define BDB_LVDS_LFP_DATA	 42
>>   #define BDB_LVDS_BACKLIGHT	 43
>>   #define BDB_LVDS_POWER		 44
>> -#define BDB_MIPI		 50
>> +#define BDB_MIPI_CONFIG		 52
>> +#define BDB_MIPI_SEQUENCE	 53
>>   #define BDB_SKIP		254 /* VBIOS private block, ignore */
>>
>>   struct bdb_general_features {
>> @@ -711,44 +712,141 @@ int intel_parse_bios(struct drm_device *dev);
>>   #define DVO_PORT_DPD	9
>>   #define DVO_PORT_DPA	10
>>
>> -/* MIPI DSI panel info */
>> -struct bdb_mipi {
>> +/* Block 52 contains MIPI Panel info
>> + * 6 such enteries will there. Index into correct
>> + * entery is based on the panel_index in #40 LFP
>> + */
>> +#define MAX_MIPI_CONFIGURATIONS	6
>> +struct mipi_config {
>>   	u16 panel_id;
>> -	u16 bridge_revision;
>>
>> -	/* General params */
>> +	/* General Params */
>>   	u32 dithering:1;
>> -	u32 bpp_pixel_format:1;
>>   	u32 rsvd1:1;
>> -	u32 dphy_valid:1;
>> -	u32 resvd2:28;
>> -
>> -	u16 port_info;
>> -	u16 rsvd3:2;
>> -	u16 num_lanes:2;
>> -	u16 rsvd4:12;
>> -
>> -	/* DSI config */
>> -	u16 virt_ch_num:2;
>> -	u16 vtm:2;
>> -	u16 rsvd5:12;
>> +	u32 panel_type:1;
>> +	u32 panel_arch_type:2;
>> +	u32 cmd_mode:1;
>> +	u32 vtm:2;
>> +	u32 cabc:1;
>> +	u32 pwm_blc:1;
>> +
>> +	/* Bit 13:10
>> +	 * 000 - Reserved, 001 - RGB565, 002 - RGB666,
>> +	 * 011 - RGB666Loosely packed, 100 - RGB888,
>> +	 * others - rsvd
>> +	 */
>> +	u32 videomode_color_format:4;
>>
>> -	u32 dsi_clock;
>> +	/* Bit 15:14
>> +	 * 0 - No rotation, 1 - 90 degree
>> +	 * 2 - 180 degree, 3 - 270 degree
>> +	 */
>> +	u32 rotation:2;
>> +	u32 bta:1;
>> +	u32 rsvd2:15;
>> +
>> +	/* 2 byte Port Description */
>> +	u16 dual_link:2;
>> +	u16 lane_cnt:2;
>> +	u16 rsvd3:12;
>> +
>> +	/* 2 byte DSI COntroller params */
>> +	/* 0 - Using DSI PHY, 1 - TE usage */
>> +	u16 dsi_usage:1;
>> +	u16 rsvd4:15;
>> +
>> +	u8 rsvd5[5];
>> +	u32 dsi_ddr_clk;
>>   	u32 bridge_ref_clk;
>> -	u16 rsvd_pwr;
>>
>> -	/* Dphy Params */
>> -	u32 prepare_cnt:5;
>> -	u32 rsvd6:3;
>> +	u8 byte_clk_sel:2;
>> +	u8 rsvd6:6;
>
> Per the spec you sent me, there's one more reserved byte here.
>
>> +
>> +	/* DPHY Flags */
>> +	u16 dphy_param_valid:1;
>> +	u16 eot_disabled:1;
>> +	u16 clk_stop:1;
>> +	u16 rsvd7:13;
>> +
>> +	u32 hs_tx_timeout;
>> +	u32 lp_rx_timeout;
>> +	u32 turn_around_timeout;
>> +	u32 device_reset_timer;
>> +	u32 master_init_timer;
>> +	u32 dbi_bw_timer;
>> +	u32 lp_byte_clk_val;
>> +
>> +	/*  4 byte Dphy Params */
>> +	u32 prepare_cnt:6;
>> +	u32 rsvd8:2;
>
> Per the spec you sent me, prepare_cnt is 5 bits, reserved 3 bits.
>
>>   	u32 clk_zero_cnt:8;
>>   	u32 trail_cnt:5;
>> -	u32 rsvd7:3;
>> +	u32 rsvd9:3;
>>   	u32 exit_zero_cnt:6;
>> -	u32 rsvd8:2;
>> +	u32 rsvd10:2;
>
> For this reserved field the spec is clearly bogus...
>
>>
>> -	u32 hl_switch_cnt;
>> -	u32 lp_byte_clk;
>>   	u32 clk_lane_switch_cnt;
>> +	u32 hl_switch_cnt;
>> +
>> +	u32 rsvd11[6];
>> +
>> +	/* timings based on dphy spec */
>> +	u8 tclk_miss;
>> +	u8 tclk_post;
>> +	u8 rsvd12;
>> +	u8 tclk_pre;
>> +	u8 tclk_prepare;
>> +	u8 tclk_settle;
>> +	u8 tclk_term_enable;
>> +	u8 tclk_trail;
>> +	u16 tclk_prepare_clkzero;
>> +	u8 rsvd13;
>> +	u8 td_term_enable;
>> +	u8 teot;
>> +	u8 ths_exit;
>> +	u8 ths_prepare;
>> +	u16 ths_prepare_hszero;
>> +	u8 rsvd14;
>> +	u8 ths_settle;
>> +	u8 ths_skip;
>> +	u8 ths_trail;
>> +	u8 tinit;
>> +	u8 tlpx;
>> +	u8 rsvd15[3];
>
> Per the spec you sent me, there's 1 byte reserved, and 5 bytes of GPIO
> indexes below.
>
> All in all, the size of the struct is different from the spec, shifting
> everything for panel_type > 0. Which one is wrong?

Take that the code is correct and the spec wrong. What I sent still 
might be a little old, but the code is matched with header files used in 
GOP code precisely for this reason that spec is not always updated 
immediately.

>
>> +
>> +	/* GPIOs */
>> +	u8 panel_enable;
>> +	u8 bl_enable;
>> +	u8 pwm_enable;
>> +	u8 reset_r_n;
>> +	u8 pwr_down_r;
>> +	u8 stdby_r_n;
>> +
>>   } __packed;
>
> All around I would like it if the field names were slightly more
> descriptive by themselves, particularly for the most important ones,
> with #defines for the values in some cases. For example panel_type above
> could clearly be "is_bridge" or similar (now it's confusing with the
> panel_type in intel_bios.c). Same for any booleans that could be
> expressed as "is_something" or "something_enabled". Color formats and
> rotations could have defines, so you wouldn't need to add comments for
> them at all. Same for byte_clk_sel.

I just tried to match the names used in the VBT interface document as 
such. But we can make some of them more descriptive. Its only that while 
discussing parameters with those who talk in VBT document terms, it 
helps to be on same page

>
> I definitely do *not* mean you should rewrite all of them. If you want,
> I can reply with detailed suggestions on a per field basis.
>

I can give a shot at improving some of them. If after that you still 
feel changes are needed, you can suggest.

>>
>> +struct bdb_mipi_config {
>> +	struct mipi_config config[0];
>
> Why isn't this struct:
>
> 	struct mipi_config config[MAX_MIPI_CONFIGURATIONS];
>          struct mipi_pps_data pps[MAX_MIPI_CONFIGURATIONS];
>
> and you could replace this ugly bit in patch 2/2:
>
> +	pps = (struct mipi_pps_data *) &start->config[MAX_MIPI_CONFIGURATIONS];
>
> with:
>
> +	pps = &start->pps[panel_type];
>
>> +};

I will check. I think I just coded that way :) Should be doable as you 
suggest.

>> +
>> +/* Block 52 contains MIPI configuration block
>> + * 6 * bdb_mipi_config, followed by 6 pps data
>> + * block below
>> + */
>> +struct mipi_pps_data {
>> +	u16 panel_on_delay;
>> +	u16 bl_enable_delay;
>> +	u16 bl_disable_delay;
>> +	u16 panel_off_delay;
>> +	u16 panel_power_cycle_delay;
>> +};
>
> I'd like the unconventional units mentioned in a comment.

Will add the units

>
>> +
>> +/* Block 53 contains MIPI sequences as needed by the panel
>> + * for enabling it. This block can be variable in size and
>> + * can be maximum of 6 blocks
>> + */
>> +struct bdb_mipi_sequence {
>> +	u8 version;
>> +	void *data;
>> +};
>> +
>>   #endif /* _I830_BIOS_H_ */
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
>> index b4a27ce..a0e42db 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.h
>> +++ b/drivers/gpu/drm/i915/intel_dsi.h
>> @@ -120,4 +120,6 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
>>   extern void vlv_enable_dsi_pll(struct intel_encoder *encoder);
>>   extern void vlv_disable_dsi_pll(struct intel_encoder *encoder);
>>
>> +#define MIPI_DSI_GENERIC_PANEL_ID	1
>> +
>>   #endif /* _INTEL_DSI_H */
>> --
>> 1.8.3.2

Regards
Shobhit

Re: [PATCH 1/2] drm/i915: Update VBT data structures to have MIPI block enhancements

[Jani Nikula]

On Thu, 13 Feb 2014, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> Hi
>
> On Thursday 13 February 2014 12:47 PM, Jani Nikula wrote:
>> On Thu, 13 Feb 2014, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>> Per the spec you sent me, there's 1 byte reserved, and 5 bytes of GPIO
>> indexes below.
>>
>> All in all, the size of the struct is different from the spec, shifting
>> everything for panel_type > 0. Which one is wrong?
>
> Take that the code is correct and the spec wrong. What I sent still 
> might be a little old, but the code is matched with header files used in 
> GOP code precisely for this reason that spec is not always updated 
> immediately.

:(

>>
>>> +
>>> +	/* GPIOs */
>>> +	u8 panel_enable;
>>> +	u8 bl_enable;
>>> +	u8 pwm_enable;
>>> +	u8 reset_r_n;
>>> +	u8 pwr_down_r;
>>> +	u8 stdby_r_n;
>>> +
>>>   } __packed;
>>
>> All around I would like it if the field names were slightly more
>> descriptive by themselves, particularly for the most important ones,
>> with #defines for the values in some cases. For example panel_type above
>> could clearly be "is_bridge" or similar (now it's confusing with the
>> panel_type in intel_bios.c). Same for any booleans that could be
>> expressed as "is_something" or "something_enabled". Color formats and
>> rotations could have defines, so you wouldn't need to add comments for
>> them at all. Same for byte_clk_sel.
>
> I just tried to match the names used in the VBT interface document as 
> such. But we can make some of them more descriptive. Its only that while 
> discussing parameters with those who talk in VBT document terms, it 
> helps to be on same page

There's certain value in that. The flip side of the coin is that not
everyone will have the spec handy when reading the code.

An alternative to changing the field names is adding #defines for at
least the most important ones. In this case it might be a good idea to
add the #defines within the struct, next to the relevant field.

>> I definitely do *not* mean you should rewrite all of them. If you want,
>> I can reply with detailed suggestions on a per field basis.
>>
>
> I can give a shot at improving some of them. If after that you still 
> feel changes are needed, you can suggest.

Ack. Again: fine tuning, not rewrite. :)


Thanks,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/2] drm/i915: Update VBT data structures to have MIPI block enhancements

[Jani Nikula]

On Thu, 13 Feb 2014, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> +/* Block 53 contains MIPI sequences as needed by the panel
> + * for enabling it. This block can be variable in size and
> + * can be maximum of 6 blocks
> + */
> +struct bdb_mipi_sequence {
> +	u8 version;
> +	void *data;

Make this

	u8 data[0];

and the next patch will be easier.

> +};


-- 
Jani Nikula, Intel Open Source Technology Center

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

[Shobhit Kumar]

The parser extracts the config block(#52) and sequence(#53) data
and store in private data structures.

Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h   |   6 ++
 drivers/gpu/drm/i915/intel_bios.c | 175 ++++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_bios.h |  34 ++++++++
 drivers/gpu/drm/i915/intel_dsi.h  |   1 +
 4 files changed, 211 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 728b9c3..9bede78 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1250,7 +1250,13 @@ struct intel_vbt_data {
 
 	/* MIPI DSI */
 	struct {
+		u8 seq_version;
 		u16 panel_id;
+		struct mipi_config *config;
+		struct mipi_pps_data *pps;
+		u32 size;
+		u8 *data;
+		u8 *sequence[MIPI_SEQ_MAX];
 	} dsi;
 
 	int crt_ddc_pin;
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index cf0b25d..9d6d063 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -595,19 +595,184 @@ parse_edp(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
 	}
 }
 
+u8 *goto_next_sequence(u8 *data)
+{
+	u16 len;
+	/* goto first element */
+	data++;
+	while (1) {
+		switch (*data++) {
+		case MIPI_SEQ_ELEM_SEND_PKT:
+			/* skip by this element payload size
+			 * skip command flag and data type */
+			data += 2;
+			len = *((u16 *)data);
+			/* skip by len */
+			data = data + 2 + len;
+			break;
+		case MIPI_SEQ_ELEM_DELAY:
+			data += 4;
+			break;
+		case MIPI_SEQ_ELEM_GPIO:
+			data += 2;
+			break;
+		default:
+			DRM_ERROR("Unknown element\n");
+			break;
+		}
+
+		/* end of sequence ? */
+		if (*data == 0)
+			break;
+	}
+	/* goto next sequence or end of block byte */
+	data++;
+	return data;
+}
+
 static void
 parse_mipi(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
 {
-	struct bdb_mipi *mipi;
+	struct bdb_mipi_config *start;
+	struct bdb_mipi_sequence *sequence;
+	struct mipi_config *config;
+	struct mipi_pps_data *pps;
+	char *data, *seq_data, *seq_start;
+	int i, panel_id, seq_size;
+
+	/* Initialize this to undefined indicating no generic MIPI support */
+	dev_priv->vbt.dsi.panel_id = MIPI_DSI_UNDEFINED_PANEL_ID;
+
+	/* Block #40 is already parsed and panel_fixed_mode is
+	 * stored in dev_priv->lfp_lvds_vbt_mode
+	 * resuse this when needed
+	 */
 
-	mipi = find_section(bdb, BDB_MIPI_CONFIG);
-	if (!mipi) {
-		DRM_DEBUG_KMS("No MIPI BDB found");
+	/* Parse #52 for panel index used from panel_type already
+	 * parsed
+	 */
+	start = find_section(bdb, BDB_MIPI_CONFIG);
+	if (!start) {
+		DRM_DEBUG_KMS("No MIPI config BDB found");
 		return;
 	}
 
-	/* XXX: add more info */
+	DRM_DEBUG_DRIVER("Found MIPI Config block, panel index = %d\n",
+								panel_type);
+
+	/*
+	 * get hold of the correct configuration block and pps data as per
+	 * the panel_type as index
+	 */
+	config = &start->config[panel_type];
+	pps = (struct mipi_pps_data *) &start->config[MAX_MIPI_CONFIGURATIONS];
+	pps = &pps[panel_type];
+
+	/* store as of now full data. Trim when we realise all is not needed */
+	dev_priv->vbt.dsi.config =
+			kzalloc(sizeof(struct mipi_config), GFP_KERNEL);
+	if (!dev_priv->vbt.dsi.config)
+		return;
+
+	dev_priv->vbt.dsi.pps =
+			kzalloc(sizeof(struct mipi_pps_data), GFP_KERNEL);
+	if (!dev_priv->vbt.dsi.pps) {
+		kfree(dev_priv->vbt.dsi.config);
+		return;
+	}
+
+	memcpy(dev_priv->vbt.dsi.config, config, sizeof(*config));
+	memcpy(dev_priv->vbt.dsi.pps, pps, sizeof(*pps));
+
+	/* We have mandatory mipi config blocks. Initialize as generic panel */
 	dev_priv->vbt.dsi.panel_id = MIPI_DSI_GENERIC_PANEL_ID;
+
+	/* Check if we have sequence block as well */
+	sequence = find_section(bdb, BDB_MIPI_SEQUENCE);
+	if (!sequence) {
+		DRM_DEBUG_KMS("No MIPI Sequnece BDB found");
+		DRM_DEBUG_DRIVER("MIPI related vbt parsing complete\n");
+		return;
+	}
+
+	DRM_DEBUG_DRIVER("Found MIPI sequence block\n");
+
+	/*
+	 * parse the sequence block for individual sequences
+	 */
+	dev_priv->vbt.dsi.seq_version = sequence->version;
+
+	seq_data = (char *)((char *)sequence + 1);
+
+	/* sequnec block is variable length and hence we need to parse and
+	 * get the sequnece data for specific panel id */
+	for (i = 0; i < MAX_MIPI_CONFIGURATIONS; i++) {
+		panel_id = *seq_data;
+		seq_size = *((u16 *) (seq_data + 1));
+		if (panel_id == panel_type) {
+			seq_start = (char *) (seq_data + 3);
+			break;
+		}
+
+		/* skip the sequence including seq header of 3 bytes */
+		seq_data = seq_data + 3 + seq_size;
+	}
+
+	if (i == MAX_MIPI_CONFIGURATIONS)
+		return;
+
+	dev_priv->vbt.dsi.data = kzalloc(seq_size, GFP_KERNEL);
+	if (!dev_priv->vbt.dsi.data)
+		return;
+
+	memcpy(dev_priv->vbt.dsi.data, seq_start, seq_size);
+
+	/*
+	 * loop into the sequence data and split into multiple sequneces
+	 * There are only 5 types of sequences as of now
+	 */
+	data = dev_priv->vbt.dsi.data;
+	dev_priv->vbt.dsi.size = seq_size;
+
+	/* two consecutive 0x00 inidcate end of all sequences */
+	while (1) {
+		switch (*data) {
+		case MIPI_SEQ_ASSERT_RESET:
+			dev_priv->vbt.dsi.sequence[MIPI_SEQ_ASSERT_RESET] =
+									data;
+			DRM_DEBUG_DRIVER("Found MIPI_SEQ_ASSERT_RESET\n");
+			break;
+		case MIPI_SEQ_INIT_OTP:
+			dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP] = data;
+			DRM_DEBUG_DRIVER("Found MIPI_SEQ_INIT_OTP\n");
+			break;
+		case MIPI_SEQ_DISPLAY_ON:
+			dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_ON] = data;
+			DRM_DEBUG_DRIVER("Found MIPI_SEQ_DISPLAY_ON\n");
+			break;
+		case MIPI_SEQ_DISPLAY_OFF:
+			dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_OFF] = data;
+			DRM_DEBUG_DRIVER("Found MIPI_SEQ_DISPLAY_OFF\n");
+			break;
+		case MIPI_SEQ_DEASSERT_RESET:
+			dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET] =
+									data;
+			DRM_DEBUG_DRIVER("Found MIPI_SEQ_DEASSERT_RESET\n");
+			break;
+		case MIPI_SEQ_UNDEFINED:
+		default:
+			DRM_ERROR("undefined sequnce\n");
+			continue;
+		}
+
+		/* partial parsing to skip elements */
+		data = goto_next_sequence(data);
+
+		if (*data == 0)
+			break; /* end of sequence reached */
+	}
+
+	DRM_DEBUG_DRIVER("MIPI related vbt parsing complete\n");
 }
 
 static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 8345e0e..dcc4bc5 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -849,4 +849,38 @@ struct bdb_mipi_sequence {
 	void *data;
 };
 
+/* MIPI Sequnece Block definitions */
+enum MIPI_SEQ {
+	MIPI_SEQ_UNDEFINED = 0,
+	MIPI_SEQ_ASSERT_RESET,
+	MIPI_SEQ_INIT_OTP,
+	MIPI_SEQ_DISPLAY_ON,
+	MIPI_SEQ_DISPLAY_OFF,
+	MIPI_SEQ_DEASSERT_RESET,
+	MIPI_SEQ_MAX
+
+};
+
+enum MIPI_SEQ_ELEMENT {
+	MIPI_SEQ_ELEM_UNDEFINED = 0,
+	MIPI_SEQ_ELEM_SEND_PKT,
+	MIPI_SEQ_ELEM_DELAY,
+	MIPI_SEQ_ELEM_GPIO,
+	MIPI_SEQ_ELEM_STATUS,
+	MIPI_SEQ_ELEM_MAX
+
+};
+
+enum MIPI_GPIO_PIN_INDEX {
+	MIPI_GPIO_UNDEFINED = 0,
+	MIPI_GPIO_PANEL_ENABLE,
+	MIPI_GPIO_BL_ENABLE,
+	MIPI_GPIO_PWM_ENABLE,
+	MIPI_GPIO_RESET_N,
+	MIPI_GPIO_PWR_DOWN_R,
+	MIPI_GPIO_STDBY_RST_N,
+	MIPI_GPIO_MAX
+
+};
+
 #endif /* _I830_BIOS_H_ */
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index a0e42db..01b6752 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -120,6 +120,7 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
 extern void vlv_enable_dsi_pll(struct intel_encoder *encoder);
 extern void vlv_disable_dsi_pll(struct intel_encoder *encoder);
 
+#define MIPI_DSI_UNDEFINED_PANEL_ID	0
 #define MIPI_DSI_GENERIC_PANEL_ID	1
 
 #endif /* _INTEL_DSI_H */
-- 
1.8.3.2

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

[Jani Nikula]

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

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

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

Static.

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

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

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

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

See kmemdup for both of these.

> +
> +	/* We have mandatory mipi config blocks. Initialize as generic panel */
>  	dev_priv->vbt.dsi.panel_id = MIPI_DSI_GENERIC_PANEL_ID;
> +
> +	/* Check if we have sequence block as well */
> +	sequence = find_section(bdb, BDB_MIPI_SEQUENCE);
> +	if (!sequence) {
> +		DRM_DEBUG_KMS("No MIPI Sequnece BDB found");
> +		DRM_DEBUG_DRIVER("MIPI related vbt parsing complete\n");

One debug msg should be enough.

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

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

	seq_data = &sequence->data[0];

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

sequence ^

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

seq_start is redundant, you could just reuse seq_data here.

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

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

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

Might be worth a debug message.

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

kmemdup

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

This needs to check for reading past seq_size.

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

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

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

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

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

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

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

-- 
Jani Nikula, Intel Open Source Technology Center