[Intel-gfx] [PATCH 1/2] drm/i915: Update VBT data structures to have MIPI block enhancements
Shobhit Kumar
shobhit.kumar at intel.com
Thu Feb 13 09:20:17 CET 2014
Hi
On Thursday 13 February 2014 12:47 PM, Jani Nikula wrote:
> On Thu, 13 Feb 2014, Shobhit Kumar <shobhit.kumar at 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 at 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
More information about the Intel-gfx
mailing list