* Re: [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver
2014-04-14 5:48 ` [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver Shobhit Kumar
@ 2014-05-15 16:48 ` Damien Lespiau
2014-05-16 9:23 ` Shobhit Kumar
2014-05-16 11:17 ` Damien Lespiau
2014-05-19 14:23 ` Damien Lespiau
2014-05-23 16:05 ` [v2] " Shobhit Kumar
2 siblings, 2 replies; 30+ messages in thread
From: Damien Lespiau @ 2014-05-15 16:48 UTC (permalink / raw)
To: Shobhit Kumar; +Cc: Jani Nikula, Daniel Vetter, intel-gfx, arjan.van.de.ven
On Mon, Apr 14, 2014 at 11:18:27AM +0530, Shobhit Kumar wrote:
> This driver makes use of the generic panel information from the VBT.
> Panel information is classified into two - panel configuration and panel
> power sequence which is unique to each panel. The generic driver uses the
> panel configuration and sequence parsed from VBT block #52 and #53
>
> v2: Address review comments by Jani
> - Move all of the things in driver c file from header
> - Make all functions static
> - Make use of video/mipi_display.c instead of redefining
> - Null checks during sequence execution
>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
I've done a first past on this. Overall looks reasonable. I'm missing
some documentation to double check the various LP->HS, HS->LP count and
other magic around the clocks (send you a mail about it) before I can
add my r-b tag.
I've added a few tiny comments as well along the road.
--
Damien
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/dsi_mod_vbt_generic.c | 598 +++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_dsi.c | 5 +
> drivers/gpu/drm/i915/intel_dsi.h | 2 +
> 4 files changed, 606 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/dsi_mod_vbt_generic.c
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index b1445b7..756a7a4 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -55,6 +55,7 @@ i915-y += dvo_ch7017.o \
> intel_dsi_cmd.o \
> intel_dsi.o \
> intel_dsi_pll.o \
> + dsi_mod_vbt_generic.o \
Should probaly start with intel_dsi_ to be consistent with the other
files. intel_dsi_vbt.c ?
[...]
> +/* This macro divides two integers and rounds fractional values up
> + * to the nearest integer value. */
> +#define CEIL_DIV(X, Y) (((X)%(Y)) ? ((X)/(Y)+1) : ((X)/(Y)))
DIV_ROUND_UP()?
> +#define GPI0_NC_0_HV_DDI0_HPD 0x4130
> +#define GPIO_NC_0_HV_DDI0_PAD 0x4138
> +#define GPIO_NC_1_HV_DDI0_DDC_SDA 0x4120
> +#define GPIO_NC_1_HV_DDI0_DDC_SDA_PAD 0x4128
> +#define GPIO_NC_2_HV_DDI0_DDC_SCL 0x4110
> +#define GPIO_NC_2_HV_DDI0_DDC_SCL_PAD 0x4118
> +#define GPIO_NC_3_PANEL0_VDDEN 0x4140
> +#define GPIO_NC_3_PANEL0_VDDEN_PAD 0x4148
> +#define GPIO_NC_4_PANEL0_BLKEN 0x4150
> +#define GPIO_NC_4_PANEL0_BLKEN_PAD 0x4158
> +#define GPIO_NC_5_PANEL0_BLKCTL 0x4160
> +#define GPIO_NC_5_PANEL0_BLKCTL_PAD 0x4168
> +#define GPIO_NC_6_PCONF0 0x4180
> +#define GPIO_NC_6_PAD 0x4188
> +#define GPIO_NC_7_PCONF0 0x4190
> +#define GPIO_NC_7_PAD 0x4198
> +#define GPIO_NC_8_PCONF0 0x4170
> +#define GPIO_NC_8_PAD 0x4178
> +#define GPIO_NC_9_PCONF0 0x4100
> +#define GPIO_NC_9_PAD 0x4108
> +#define GPIO_NC_10_PCONF0 0x40E0
> +#define GPIO_NC_10_PAD 0x40E8
> +#define GPIO_NC_11_PCONF0 0x40F0
> +#define GPIO_NC_11_PAD 0x40F8
> +
> +struct gpio_table {
> + u16 function_reg;
> + u16 pad_reg;
> + u8 init;
> +};
> +
> +static struct gpio_table gtable[] = {
const
> + { GPI0_NC_0_HV_DDI0_HPD, GPIO_NC_0_HV_DDI0_PAD, 0 },
> + { GPIO_NC_1_HV_DDI0_DDC_SDA, GPIO_NC_1_HV_DDI0_DDC_SDA_PAD, 0 },
> + { GPIO_NC_2_HV_DDI0_DDC_SCL, GPIO_NC_2_HV_DDI0_DDC_SCL_PAD, 0 },
> + { GPIO_NC_3_PANEL0_VDDEN, GPIO_NC_3_PANEL0_VDDEN_PAD, 0 },
> + { GPIO_NC_4_PANEL0_BLKEN, GPIO_NC_4_PANEL0_BLKEN_PAD, 0 },
> + { GPIO_NC_5_PANEL0_BLKCTL, GPIO_NC_5_PANEL0_BLKCTL_PAD, 0 },
> + { GPIO_NC_6_PCONF0, GPIO_NC_6_PAD, 0 },
> + { GPIO_NC_7_PCONF0, GPIO_NC_7_PAD, 0 },
> + { GPIO_NC_8_PCONF0, GPIO_NC_8_PAD, 0 },
> + { GPIO_NC_9_PCONF0, GPIO_NC_9_PAD, 0 },
> + { GPIO_NC_10_PCONF0, GPIO_NC_10_PAD, 0},
> + { GPIO_NC_11_PCONF0, GPIO_NC_11_PAD, 0}
> +};
> +
> +static u8 *mipi_exec_send_packet(struct intel_dsi *intel_dsi, u8 *data)
> +{
> + u8 type, byte, mode, vc, port;
> + u16 len;
> +
> + byte = *data++;
> + mode = (byte >> MIPI_TRANSFER_MODE_SHIFT) & 0x1;
> + vc = (byte >> MIPI_VIRTUAL_CHANNEL_SHIFT) & 0x3;
> + port = (byte >> MIPI_PORT_SHIFT) & 0x3;
> +
> + /* LP or HS mode */
> + intel_dsi->hs = mode;
> +
> + /* get packet type and increment the pointer */
> + type = *data++;
> +
> + len = *((u16 *) data);
> + data += 2;
> +
> + switch (type) {
> + case MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM:
> + dsi_vc_generic_write_0(intel_dsi, vc);
> + break;
> + case MIPI_DSI_GENERIC_SHORT_WRITE_1_PARAM:
> + dsi_vc_generic_write_1(intel_dsi, vc, *data);
> + break;
> + case MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM:
> + dsi_vc_generic_write_2(intel_dsi, vc, *data, *(data + 1));
> + break;
> + case MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM:
> + case MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM:
> + case MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM:
> + DRM_DEBUG_DRIVER("Generic Read not yet implemented or used\n");
> + break;
> + case MIPI_DSI_GENERIC_LONG_WRITE:
> + dsi_vc_generic_write(intel_dsi, vc, data, len);
> + break;
> + case MIPI_DSI_DCS_SHORT_WRITE:
> + dsi_vc_dcs_write_0(intel_dsi, vc, *data);
> + break;
> + case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
> + dsi_vc_dcs_write_1(intel_dsi, vc, *data, *(data + 1));
> + break;
> + case MIPI_DSI_DCS_READ:
> + DRM_DEBUG_DRIVER("DCS Read not yet implemented or used\n");
> + break;
> + case MIPI_DSI_DCS_LONG_WRITE:
> + dsi_vc_dcs_write(intel_dsi, vc, data, len);
> + break;
> + };
> +
> + data += len;
> +
> + return data;
> +}
> +
> +static u8 *mipi_exec_delay(struct intel_dsi *intel_dsi, u8 *data)
> +{
> + u32 delay = *((u32 *) data);
> +
> + DRM_DEBUG_DRIVER("MIPI: executing delay element\n");
A general observation for your tracing of the sequences being executed.
Either it's important to trace the sequence we're executing and we may
want to add more information (for instance here the amount we're waiting
for, what we're doing with the GPIOs, ...) or it's not important and we
can skip the messages altogether?
> + usleep_range(delay, delay + 10);
> + data += 4;
> +
> + return data;
> +}
> +
> +static u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, u8 *data)
> +{
> + u8 gpio, action;
> + u16 function, pad;
> + u32 val;
> + struct drm_device *dev = intel_dsi->base.base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + DRM_DEBUG_DRIVER("MIPI: executing gpio element\n");
> + gpio = *data++;
> +
> + /* pull up/down */
> + action = *data++;
> +
> + function = gtable[gpio].function_reg;
> + pad = gtable[gpio].pad_reg;
> +
> + mutex_lock(&dev_priv->dpio_lock);
> + if (!gtable[gpio].init) {
> + /* program the function */
> + vlv_gpio_nc_write(dev_priv, function, 0x2000CC00);
Any chance we can document that constant? (with defines of comment)
> + gtable[gpio].init = 1;
> + }
> +
> + val = 0x4 | action;
> +
> + /* pull up/down */
> + vlv_gpio_nc_write(dev_priv, pad, val);
> + mutex_unlock(&dev_priv->dpio_lock);
> +
> + return data;
> +}
> +
> +typedef u8 * (*fn_mipi_elem_exec)(struct intel_dsi *intel_dsi, u8 *data);
> +const fn_mipi_elem_exec exec_elem[] = {
static
> + NULL, /* reserved */
> + mipi_exec_send_packet,
> + mipi_exec_delay,
> + mipi_exec_gpio,
> + NULL, /* status read; later */
> +};
> +
> +/*
> + * MIPI Sequence from VBT #53 parsing logic
> + * We have already separated each seqence during bios parsing
> + * Following is generic execution function for any sequence
> + */
> +
> +static const char *seq_name[] = {
static const char * const
> + "UNDEFINED",
> + "MIPI_SEQ_ASSERT_RESET",
> + "MIPI_SEQ_INIT_OTP",
> + "MIPI_SEQ_DISPLAY_ON",
> + "MIPI_SEQ_DISPLAY_OFF",
> + "MIPI_SEQ_DEASSERT_RESET"
> +};
> +
> +static void generic_exec_sequence(struct intel_dsi *intel_dsi, char *sequence)
> +{
> + u8 *data = sequence;
> + fn_mipi_elem_exec mipi_elem_exec;
> + int index;
> +
> + if (!sequence)
> + return;
> +
> + DRM_DEBUG_DRIVER("Starting MIPI sequence - %s\n", seq_name[*data]);
> +
> + /* go to the first element of the sequence */
> + data++;
> +
> + /* parse each byte till we reach end of sequence byte - 0x00 */
> + while (1) {
> + index = *data;
> + mipi_elem_exec = exec_elem[index];
> + if (!mipi_elem_exec) {
> + DRM_ERROR("Unsupported MIPI element, skipping sequence execution\n");
> + return;
> + }
> +
> + /* goto element payload */
> + data++;
> +
> + /* execute the element specifc rotines */
specific routines
> + data = mipi_elem_exec(intel_dsi, data);
> +
> + /*
> + * After processing the element, data should point to
> + * next element or end of sequence
> + * check if have we reached end of sequence
> + */
> + if (*data == 0x00)
> + break;
> + }
> +}
> +
> +static bool generic_init(struct intel_dsi_device *dsi)
> +{
> + struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> + struct drm_device *dev = intel_dsi->base.base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct mipi_config *mipi_config = dev_priv->vbt.dsi.config;
> + struct mipi_pps_data *pps = dev_priv->vbt.dsi.pps;
> + struct drm_display_mode *mode = dev_priv->vbt.lfp_lvds_vbt_mode;
> + u32 bits_per_pixel = 24;
> + u32 tlpx_ns, extra_byte_count, bitrate, tlpx_ui;
> + u32 ui_num, ui_den;
> + u32 prepare_cnt, exit_zero_cnt, clk_zero_cnt, trail_cnt;
> + u32 ths_prepare_ns, tclk_trail_ns;
> + u32 tclk_prepare_clkzero, ths_prepare_hszero;
> +
> + DRM_DEBUG_KMS("\n");
> +
> + intel_dsi->eotp_pkt = mipi_config->eot_pkt_disabled ? 0 : 1;
> + intel_dsi->clock_stop = mipi_config->enable_clk_stop ? 1 : 0;
> + intel_dsi->lane_count = mipi_config->lane_cnt + 1;
> + intel_dsi->pixel_format = mipi_config->videomode_color_format << 7;
> +
> + if (intel_dsi->pixel_format == VID_MODE_FORMAT_RGB666)
> + bits_per_pixel = 18;
> + else if (intel_dsi->pixel_format == VID_MODE_FORMAT_RGB565)
> + bits_per_pixel = 16;
> +
> + bitrate = (mode->clock * bits_per_pixel) / intel_dsi->lane_count;
> +
> + intel_dsi->operation_mode = mipi_config->is_cmd_mode;
> + intel_dsi->video_mode_format = mipi_config->video_transfer_mode;
> + intel_dsi->escape_clk_div = mipi_config->byte_clk_sel;
> + intel_dsi->lp_rx_timeout = mipi_config->lp_rx_timeout;
> + intel_dsi->turn_arnd_val = mipi_config->turn_around_timeout;
> + intel_dsi->rst_timer_val = mipi_config->device_reset_timer;
> + intel_dsi->init_count = mipi_config->master_init_timer;
> + intel_dsi->bw_timer = mipi_config->dbi_bw_timer;
> + intel_dsi->video_frmt_cfg_bits = mipi_config->bta_enabled ? DISABLE_VIDEO_BTA : 0;
> +
> + switch (intel_dsi->escape_clk_div) {
> + case 0:
> + tlpx_ns = 50;
> + break;
> + case 1:
> + tlpx_ns = 100;
> + break;
> +
> + case 2:
> + tlpx_ns = 200;
> + break;
> + default:
> + tlpx_ns = 50;
> + break;
> + }
> +
> + switch (intel_dsi->lane_count) {
> + case 1:
> + case 2:
> + extra_byte_count = 2;
> + break;
> + case 3:
> + extra_byte_count = 4;
> + break;
> + case 4:
> + default:
> + extra_byte_count = 3;
> + break;
> + }
> +
> + /*
> + * ui(s) = 1/f [f in hz]
> + * ui(ns) = 10^9/f*10^6 [f in Mhz] -> 10^3/f(Mhz)
> + *
> + * LP byte clock = TLPX/8ui
> + *
> + * Since txddrclkhs_i is 2xUI, the count values programmed in
> + * DPHY param register are divided by 2
> + *
> + */
> +
> + /* in Kbps */
> + ui_num = bitrate;
> + ui_den = NS_MHZ_RATIO;
> +
> + tclk_prepare_clkzero = mipi_config->tclk_prepare_clkzero;
> + ths_prepare_hszero = mipi_config->ths_prepare_hszero;
> +
> + /* B060 */
> + intel_dsi->lp_byte_clk = CEIL_DIV(tlpx_ns * ui_num, 8 * ui_den);
> +
> + /* count values in UI = (ns value) * (bitrate / (2 * 10^6)) */
> + /* prepare count */
> + ths_prepare_ns =
> + (mipi_config->ths_prepare > mipi_config->tclk_prepare) ?
> + mipi_config->ths_prepare :
> + mipi_config->tclk_prepare;
> +
> + prepare_cnt = CEIL_DIV(ths_prepare_ns * ui_num, ui_den * 2);
> +
> + /* exit zero count */
> + exit_zero_cnt = CEIL_DIV(
> + (ths_prepare_hszero - ths_prepare_ns) * ui_num,
> + ui_den * 2
> + );
> +
> + /*
> + * Exit zero is unified val ths_zero and ths_exit
> + * minimum value for ths_exit = 110ns
> + * min (exit_zero_cnt * 2) = 110/UI
> + * exit_zero_cnt = 55/UI
> + */
> + if (exit_zero_cnt < (55 * ui_num / ui_den))
> + if ((55 * ui_num) % ui_den)
> + exit_zero_cnt += 1;
> +
> + /* clk zero count */
> + clk_zero_cnt = CEIL_DIV(
> + (tclk_prepare_clkzero - ths_prepare_ns)
> + * ui_num, 2 * ui_den);
> +
> + /* trail count */
> + tclk_trail_ns = (mipi_config->tclk_trail > mipi_config->ths_trail) ?
> + mipi_config->tclk_trail : mipi_config->ths_trail;
> + trail_cnt = CEIL_DIV(tclk_trail_ns * ui_num, 2 * ui_den);
> +
> + if (prepare_cnt > PREPARE_CNT_MAX ||
> + exit_zero_cnt > EXIT_ZERO_CNT_MAX ||
> + clk_zero_cnt > CLK_ZERO_CNT_MAX ||
> + trail_cnt > TRAIL_CNT_MAX)
> + DRM_DEBUG_DRIVER("Values crossing maximum limits\n");
> +
> + if (prepare_cnt > PREPARE_CNT_MAX)
> + prepare_cnt = PREPARE_CNT_MAX;
> +
> + if (exit_zero_cnt > EXIT_ZERO_CNT_MAX)
> + exit_zero_cnt = EXIT_ZERO_CNT_MAX;
> +
> + if (clk_zero_cnt > CLK_ZERO_CNT_MAX)
> + clk_zero_cnt = CLK_ZERO_CNT_MAX;
> +
> + if (trail_cnt > TRAIL_CNT_MAX)
> + trail_cnt = TRAIL_CNT_MAX;
> +
> + /* B080 */
> + intel_dsi->dphy_reg = exit_zero_cnt << 24 | trail_cnt << 16 |
> + clk_zero_cnt << 8 | prepare_cnt;
> +
> + /*
> + * LP to HS switch count = 4TLPX + PREP_COUNT * 2 + EXIT_ZERO_COUNT * 2
> + * + 10UI + Extra Byte Count
> + *
> + * HS to LP switch count = THS-TRAIL + 2TLPX + Extra Byte Count
> + * Extra Byte Count is calculated according to number of lanes.
> + * High Low Switch Count is the Max of LP to HS and
> + * HS to LP switch count
> + *
> + */
> + tlpx_ui = CEIL_DIV(tlpx_ns * ui_num, ui_den);
> +
> + /* B044 */
> + intel_dsi->hs_to_lp_count =
> + CEIL_DIV(
> + 4 * tlpx_ui + prepare_cnt * 2 +
> + exit_zero_cnt * 2 + 10,
> + 8);
The comment above says the reverse of what the code does (lp->hs Vs
hs->lp).
> +
> + intel_dsi->hs_to_lp_count += extra_byte_count;
> +
> + /* B088 */
> + /* LP -> HS for clock lanes
> + * LP clk sync + LP11 + LP01 + tclk_prepare + tclk_zero +
> + * extra byte count
> + * 2TPLX + 1TLPX + 1 TPLX(in ns) + prepare_cnt * 2 + clk_zero_cnt *
> + * 2(in UI) + extra byte count
> + * In byteclks = (4TLPX + prepare_cnt * 2 + clk_zero_cnt *2 (in UI)) /
> + * 8 + extra byte count
> + */
> + intel_dsi->clk_lp_to_hs_count =
> + CEIL_DIV(
> + 4 * tlpx_ui + prepare_cnt * 2 +
> + clk_zero_cnt * 2,
> + 8);
> +
> + intel_dsi->clk_lp_to_hs_count += extra_byte_count;
> +
> + /* HS->LP for Clock Lanes
> + * Low Power clock synchronisations + 1Tx byteclk + tclk_trail +
> + * Extra byte count
> + * 2TLPX + 8UI + (trail_count*2)(in UI) + Extra byte count
> + * In byteclks = (2*TLpx(in UI) + trail_count*2 +8)(in UI)/8 +
> + * Extra byte count
> + */
> + intel_dsi->clk_hs_to_lp_count =
> + CEIL_DIV(2 * tlpx_ui + trail_cnt * 2 + 8,
> + 8);
> + intel_dsi->clk_hs_to_lp_count += extra_byte_count;
> +
> + DRM_DEBUG_KMS("EOT %s\n", intel_dsi->eotp_pkt ? "ENABLED" : "DISABLED");
We don't usually use capital letters for debug messages.
> + DRM_DEBUG_KMS("CLOCKSTOP %s\n", intel_dsi->clock_stop ?
> + "ENABLED" : "DISABLED");
> + DRM_DEBUG_KMS("Mode %s\n", intel_dsi->operation_mode ? "COMMAND" : "VIDEO");
> + DRM_DEBUG_KMS("Pixel Format %d\n", intel_dsi->pixel_format);
> + DRM_DEBUG_KMS("TLPX %d\n", intel_dsi->escape_clk_div);
> + DRM_DEBUG_KMS("LP RX Timeout 0x%x\n", intel_dsi->lp_rx_timeout);
> + DRM_DEBUG_KMS("Turnaround Timeout 0x%x\n", intel_dsi->turn_arnd_val);
> + DRM_DEBUG_KMS("Init Count 0x%x\n", intel_dsi->init_count);
> + DRM_DEBUG_KMS("HS to LP Count 0x%x\n", intel_dsi->hs_to_lp_count);
> + DRM_DEBUG_KMS("LP Byte Clock %d\n", intel_dsi->lp_byte_clk);
> + DRM_DEBUG_KMS("DBI BW Timer 0x%x\n", intel_dsi->bw_timer);
> + DRM_DEBUG_KMS("LP to HS Clock Count 0x%x\n", intel_dsi->clk_lp_to_hs_count);
> + DRM_DEBUG_KMS("HS to LP Clock Count 0x%x\n", intel_dsi->clk_hs_to_lp_count);
> + DRM_DEBUG_KMS("BTA %s\n",
> + intel_dsi->video_frmt_cfg_bits & DISABLE_VIDEO_BTA ?
> + "DISABLED" : "ENABLED");
> + DRM_DEBUG_KMS("B060 = 0x%Xx, B080 = 0x%x, B044 = 0x%x, B088 = 0x%x\n",
> + intel_dsi->lp_byte_clk, intel_dsi->dphy_reg, intel_dsi->hs_to_lp_count,
> + (intel_dsi->clk_lp_to_hs_count << LP_HS_SSW_CNT_SHIFT) |
> + (intel_dsi->clk_hs_to_lp_count << HS_LP_PWR_SW_CNT_SHIFT));
Can we have symbolic names instead of register offsets? if needed at
all? also lp_byte_clk and hs_to_lp_count are already printed out a few
lines before.
> +
> + /* delays in VBT are in unit of 100us, so need to convert
> + * here in ms
> + * Delay (100us) * 100 /1000 = Delay / 10 (ms) */
> + intel_dsi->backlight_off_delay = pps->bl_disable_delay / 10;
> + intel_dsi->backlight_on_delay = pps->bl_enable_delay / 10;
> + intel_dsi->panel_on_delay = pps->panel_on_delay / 10;
> + intel_dsi->panel_off_delay = pps->panel_off_delay / 10;
> + intel_dsi->panel_pwr_cycle_delay = pps->panel_power_cycle_delay / 10;
> +
> + return true;
> +}
> +
> +static int generic_mode_valid(struct intel_dsi_device *dsi,
> + struct drm_display_mode *mode)
> +{
> + return MODE_OK;
> +}
> +
> +static bool generic_mode_fixup(struct intel_dsi_device *dsi,
> + const struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode) {
> + return true;
> +}
> +
> +static void generic_panel_reset(struct intel_dsi_device *dsi)
> +{
> + struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> + struct drm_device *dev = intel_dsi->base.base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_ASSERT_RESET];
> +
> + generic_exec_sequence(intel_dsi, sequence);
> +}
> +
> +static void generic_disable_panel_power(struct intel_dsi_device *dsi)
> +{
> + struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> + struct drm_device *dev = intel_dsi->base.base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET];
> +
> + generic_exec_sequence(intel_dsi, sequence);
> +}
> +
> +static void generic_send_otp_cmds(struct intel_dsi_device *dsi)
> +{
> + struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> + struct drm_device *dev = intel_dsi->base.base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP];
> +
> + generic_exec_sequence(intel_dsi, sequence);
> +}
> +
> +static void generic_enable(struct intel_dsi_device *dsi)
> +{
> + struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> + struct drm_device *dev = intel_dsi->base.base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_ON];
> +
> + generic_exec_sequence(intel_dsi, sequence);
> +}
> +
> +static void generic_disable(struct intel_dsi_device *dsi)
> +{
> + struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> + struct drm_device *dev = intel_dsi->base.base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_OFF];
> +
> + generic_exec_sequence(intel_dsi, sequence);
> +}
> +
> +static enum drm_connector_status generic_detect(struct intel_dsi_device *dsi)
> +{
> + return connector_status_connected;
> +}
> +
> +static bool generic_get_hw_state(struct intel_dsi_device *dev)
> +{
> + return true;
> +}
We don't seem to use struct intel_dsi_dev_ops's get_hw_state() anywhere
btw.
> +
> +static struct drm_display_mode *generic_get_modes(struct intel_dsi_device *dsi)
> +{
> + struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> + struct drm_device *dev = intel_dsi->base.base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + dev_priv->vbt.lfp_lvds_vbt_mode->type |= DRM_MODE_TYPE_PREFERRED;
> + return dev_priv->vbt.lfp_lvds_vbt_mode;
> +}
> +
> +static void generic_destroy(struct intel_dsi_device *dsi) { }
> +
> +/* Callbacks. We might not need them all. */
> +struct intel_dsi_dev_ops vbt_generic_dsi_display_ops = {
> + .init = generic_init,
> + .mode_valid = generic_mode_valid,
> + .mode_fixup = generic_mode_fixup,
> + .panel_reset = generic_panel_reset,
> + .disable_panel_power = generic_disable_panel_power,
> + .send_otp_cmds = generic_send_otp_cmds,
> + .enable = generic_enable,
> + .disable = generic_disable,
> + .detect = generic_detect,
> + .get_hw_state = generic_get_hw_state,
> + .get_modes = generic_get_modes,
> + .destroy = generic_destroy,
> +};
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 0d4dd54..7f1ddaa 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -35,6 +35,11 @@
>
> /* the sub-encoders aka panel drivers */
> static const struct intel_dsi_device intel_dsi_devices[] = {
> + {
> + .panel_id = MIPI_DSI_GENERIC_PANEL_ID,
> + .name = "vbt-generic-dsi-vid-mode-display",
> + .dev_ops = &vbt_generic_dsi_display_ops,
> + },
> };
>
> static void band_gap_reset(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
> index e3f4e91..31db33d 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -133,4 +133,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);
>
> +extern struct intel_dsi_dev_ops vbt_generic_dsi_display_ops;
> +
> #endif /* _INTEL_DSI_H */
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver
2014-05-15 16:48 ` Damien Lespiau
@ 2014-05-16 9:23 ` Shobhit Kumar
2014-05-16 11:17 ` Damien Lespiau
1 sibling, 0 replies; 30+ messages in thread
From: Shobhit Kumar @ 2014-05-16 9:23 UTC (permalink / raw)
To: Damien Lespiau; +Cc: Jani Nikula, Daniel Vetter, intel-gfx, arjan.van.de.ven
Thanks Damien for your review
On Thursday 15 May 2014 10:18 PM, Damien Lespiau wrote:
> On Mon, Apr 14, 2014 at 11:18:27AM +0530, Shobhit Kumar wrote:
>> >This driver makes use of the generic panel information from the VBT.
>> >Panel information is classified into two - panel configuration and panel
>> >power sequence which is unique to each panel. The generic driver uses the
>> >panel configuration and sequence parsed from VBT block #52 and #53
>> >
>> >v2: Address review comments by Jani
>> > - Move all of the things in driver c file from header
>> > - Make all functions static
>> > - Make use of video/mipi_display.c instead of redefining
>> > - Null checks during sequence execution
>> >
>> >Signed-off-by: Shobhit Kumar<shobhit.kumar@intel.com>
> I've done a first past on this. Overall looks reasonable. I'm missing
> some documentation to double check the various LP->HS, HS->LP count and
> other magic around the clocks (send you a mail about it) before I can
> add my r-b tag.
>
> I've added a few tiny comments as well along the road.
All look okay to me and Will push updated patch asap.
There is one issue which I am struggling for now. If we have all these
patches in, then disable sequence pipe off does not work and
wait_for_pipe_off gives a warn dump but everything works. Its not this
patch issue but DSI patches that are already merged. I know the fix is
to actually disable port after disabling pipe and plane but doing that
does not succeed enable in first attempt. Subsequent disable/enable
works. Looking into that and should have a fix by next week on that.
Regards
Shobhit
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver
2014-05-15 16:48 ` Damien Lespiau
2014-05-16 9:23 ` Shobhit Kumar
@ 2014-05-16 11:17 ` Damien Lespiau
1 sibling, 0 replies; 30+ messages in thread
From: Damien Lespiau @ 2014-05-16 11:17 UTC (permalink / raw)
To: Shobhit Kumar; +Cc: Jani Nikula, Daniel Vetter, intel-gfx
On Thu, May 15, 2014 at 05:48:57PM +0100, Damien Lespiau wrote:
> > +static struct gpio_table gtable[] = {
>
> const
>
Please, disregard this comment. It's being written to to track GPIO
initialization.
--
Damien
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver
2014-04-14 5:48 ` [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver Shobhit Kumar
2014-05-15 16:48 ` Damien Lespiau
@ 2014-05-19 14:23 ` Damien Lespiau
2014-05-20 16:16 ` Shobhit Kumar
2014-05-23 16:05 ` [v2] " Shobhit Kumar
2 siblings, 1 reply; 30+ messages in thread
From: Damien Lespiau @ 2014-05-19 14:23 UTC (permalink / raw)
To: Shobhit Kumar; +Cc: Jani Nikula, Daniel Vetter, intel-gfx, arjan.van.de.ven
On Mon, Apr 14, 2014 at 11:18:27AM +0530, Shobhit Kumar wrote:
> +#define NS_MHZ_RATIO 1000000
[...]
> +static bool generic_init(struct intel_dsi_device *dsi)
> +{
[...]
> + /*
> + * ui(s) = 1/f [f in hz]
> + * ui(ns) = 10^9/f*10^6 [f in Mhz] -> 10^3/f(Mhz)
ui(ns) = 10^9/(f*10^6)
> + *
> + * LP byte clock = TLPX/8ui
Mind putting that comment just above the appropriate computation?
Also, LP byte clock = Tlpx / (8UI)
> + *
> + * Since txddrclkhs_i is 2xUI, the count values programmed in
> + * DPHY param registers are divided by 2
That looks like a general comment that apply to a bunch of calculations
below, probably worth separating it from the UI comment.
> + *
> + */
> +
> + /* in Kbps */
> + ui_num = bitrate;
> + ui_den = NS_MHZ_RATIO;
I'm a bit confused here, most likely missing something, care to clarify?
- IIUC, you want the computations to happen in ns. I'm a bit puzzled by
that NS_MHZ_RATIO constant name when we're dealing with Kbps.
That constant is 10^6 which seems to be OK for KHz. So maybe just a
naming problem?
- UI is a period, so is homogeneous to time (s), but ui_num being in
s^-1 and ui_den a constant, ui_num/ui_den looks like a frequency. Or
could it be that UI = ui_den / ui_num? would be confusing, but the
code below would make more sense. In which case could we have UI =
ui_num / ui_den?
> +
> + tclk_prepare_clkzero = mipi_config->tclk_prepare_clkzero;
> + ths_prepare_hszero = mipi_config->ths_prepare_hszero;
> +
> + /* B060 */
> + intel_dsi->lp_byte_clk = CEIL_DIV(tlpx_ns * ui_num, 8 * ui_den);
> +
> + /* count values in UI = (ns value) * (bitrate / (2 * 10^6)) */
> + /* prepare count */
> + ths_prepare_ns =
> + (mipi_config->ths_prepare > mipi_config->tclk_prepare) ?
> + mipi_config->ths_prepare :
> + mipi_config->tclk_prepare;
That looks like max()
> +
> + prepare_cnt = CEIL_DIV(ths_prepare_ns * ui_num, ui_den * 2);
The formula above has a 10^6, why is that OK not to have it there? (in
which unit is bitrate in the formula? MHz?) Is this something like:
Count in UI = count(ns) / UI(ns)
and then as UI = ui_den/ui_num (?!) we end up with:
Count in UI = count(ns) * ui_num / ui_den
And then the / 2 comment applies.
> +
> + /* exit zero count */
> + exit_zero_cnt = CEIL_DIV(
> + (ths_prepare_hszero - ths_prepare_ns) * ui_num,
> + ui_den * 2
> + );
> +
> + /*
> + * Exit zero is unified val ths_zero and ths_exit
> + * minimum value for ths_exit = 110ns
> + * min (exit_zero_cnt * 2) = 110/UI
> + * exit_zero_cnt = 55/UI
> + */
> + if (exit_zero_cnt < (55 * ui_num / ui_den))
> + if ((55 * ui_num) % ui_den)
> + exit_zero_cnt += 1;
I'm not sure what we're achieving with the +=1 here, mind explaining?
> +
> + /* clk zero count */
> + clk_zero_cnt = CEIL_DIV(
> + (tclk_prepare_clkzero - ths_prepare_ns)
> + * ui_num, 2 * ui_den);
> +
> + /* trail count */
> + tclk_trail_ns = (mipi_config->tclk_trail > mipi_config->ths_trail) ?
> + mipi_config->tclk_trail : mipi_config->ths_trail;
max()
> + trail_cnt = CEIL_DIV(tclk_trail_ns * ui_num, 2 * ui_den);
> +
> + if (prepare_cnt > PREPARE_CNT_MAX ||
> + exit_zero_cnt > EXIT_ZERO_CNT_MAX ||
> + clk_zero_cnt > CLK_ZERO_CNT_MAX ||
> + trail_cnt > TRAIL_CNT_MAX)
> + DRM_DEBUG_DRIVER("Values crossing maximum limits\n");
Is that a situation that may happen in a normal case? or should we go
with a DRM_ERROR() here and potentially abort the modeset?
> +
> + if (prepare_cnt > PREPARE_CNT_MAX)
> + prepare_cnt = PREPARE_CNT_MAX;
> +
> + if (exit_zero_cnt > EXIT_ZERO_CNT_MAX)
> + exit_zero_cnt = EXIT_ZERO_CNT_MAX;
> +
> + if (clk_zero_cnt > CLK_ZERO_CNT_MAX)
> + clk_zero_cnt = CLK_ZERO_CNT_MAX;
> +
> + if (trail_cnt > TRAIL_CNT_MAX)
> + trail_cnt = TRAIL_CNT_MAX;
> +
> + /* B080 */
> + intel_dsi->dphy_reg = exit_zero_cnt << 24 | trail_cnt << 16 |
> + clk_zero_cnt << 8 | prepare_cnt;
> +
> + /*
> + * LP to HS switch count = 4TLPX + PREP_COUNT * 2 + EXIT_ZERO_COUNT * 2
> + * + 10UI + Extra Byte Count
> + *
> + * HS to LP switch count = THS-TRAIL + 2TLPX + Extra Byte Count
> + * Extra Byte Count is calculated according to number of lanes.
> + * High Low Switch Count is the Max of LP to HS and
> + * HS to LP switch count
> + *
> + */
> + tlpx_ui = CEIL_DIV(tlpx_ns * ui_num, ui_den);
> +
> + /* B044 */
> + intel_dsi->hs_to_lp_count =
> + CEIL_DIV(
> + 4 * tlpx_ui + prepare_cnt * 2 +
> + exit_zero_cnt * 2 + 10,
> + 8);
The previous was before I tried to look at the spec too closely. Mind
explaining why we don't look at the HS to LP switch count? ie why HS to
LP switch cound is always smaller than the LP to HS one?
> +
> + intel_dsi->hs_to_lp_count += extra_byte_count;
> +
> + /* B088 */
> + /* LP -> HS for clock lanes
> + * LP clk sync + LP11 + LP01 + tclk_prepare + tclk_zero +
> + * extra byte count
> + * 2TPLX + 1TLPX + 1 TPLX(in ns) + prepare_cnt * 2 + clk_zero_cnt *
> + * 2(in UI) + extra byte count
> + * In byteclks = (4TLPX + prepare_cnt * 2 + clk_zero_cnt *2 (in UI)) /
> + * 8 + extra byte count
> + */
> + intel_dsi->clk_lp_to_hs_count =
> + CEIL_DIV(
> + 4 * tlpx_ui + prepare_cnt * 2 +
> + clk_zero_cnt * 2,
> + 8);
> +
> + intel_dsi->clk_lp_to_hs_count += extra_byte_count;
> +
> + /* HS->LP for Clock Lanes
> + * Low Power clock synchronisations + 1Tx byteclk + tclk_trail +
> + * Extra byte count
> + * 2TLPX + 8UI + (trail_count*2)(in UI) + Extra byte count
> + * In byteclks = (2*TLpx(in UI) + trail_count*2 +8)(in UI)/8 +
> + * Extra byte count
> + */
> + intel_dsi->clk_hs_to_lp_count =
> + CEIL_DIV(2 * tlpx_ui + trail_cnt * 2 + 8,
> + 8);
> + intel_dsi->clk_hs_to_lp_count += extra_byte_count;
> +
> + DRM_DEBUG_KMS("EOT %s\n", intel_dsi->eotp_pkt ? "ENABLED" : "DISABLED");
> + DRM_DEBUG_KMS("CLOCKSTOP %s\n", intel_dsi->clock_stop ?
> + "ENABLED" : "DISABLED");
> + DRM_DEBUG_KMS("Mode %s\n", intel_dsi->operation_mode ? "COMMAND" : "VIDEO");
> + DRM_DEBUG_KMS("Pixel Format %d\n", intel_dsi->pixel_format);
> + DRM_DEBUG_KMS("TLPX %d\n", intel_dsi->escape_clk_div);
> + DRM_DEBUG_KMS("LP RX Timeout 0x%x\n", intel_dsi->lp_rx_timeout);
> + DRM_DEBUG_KMS("Turnaround Timeout 0x%x\n", intel_dsi->turn_arnd_val);
> + DRM_DEBUG_KMS("Init Count 0x%x\n", intel_dsi->init_count);
> + DRM_DEBUG_KMS("HS to LP Count 0x%x\n", intel_dsi->hs_to_lp_count);
> + DRM_DEBUG_KMS("LP Byte Clock %d\n", intel_dsi->lp_byte_clk);
> + DRM_DEBUG_KMS("DBI BW Timer 0x%x\n", intel_dsi->bw_timer);
> + DRM_DEBUG_KMS("LP to HS Clock Count 0x%x\n", intel_dsi->clk_lp_to_hs_count);
> + DRM_DEBUG_KMS("HS to LP Clock Count 0x%x\n", intel_dsi->clk_hs_to_lp_count);
> + DRM_DEBUG_KMS("BTA %s\n",
> + intel_dsi->video_frmt_cfg_bits & DISABLE_VIDEO_BTA ?
> + "DISABLED" : "ENABLED");
> + DRM_DEBUG_KMS("B060 = 0x%Xx, B080 = 0x%x, B044 = 0x%x, B088 = 0x%x\n",
> + intel_dsi->lp_byte_clk, intel_dsi->dphy_reg, intel_dsi->hs_to_lp_count,
> + (intel_dsi->clk_lp_to_hs_count << LP_HS_SSW_CNT_SHIFT) |
> + (intel_dsi->clk_hs_to_lp_count << HS_LP_PWR_SW_CNT_SHIFT));
> +
> + /* delays in VBT are in unit of 100us, so need to convert
> + * here in ms
> + * Delay (100us) * 100 /1000 = Delay / 10 (ms) */
> + intel_dsi->backlight_off_delay = pps->bl_disable_delay / 10;
> + intel_dsi->backlight_on_delay = pps->bl_enable_delay / 10;
> + intel_dsi->panel_on_delay = pps->panel_on_delay / 10;
> + intel_dsi->panel_off_delay = pps->panel_off_delay / 10;
> + intel_dsi->panel_pwr_cycle_delay = pps->panel_power_cycle_delay / 10;
> +
> + return true;
> +}
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver
2014-05-19 14:23 ` Damien Lespiau
@ 2014-05-20 16:16 ` Shobhit Kumar
2014-05-20 20:55 ` Damien Lespiau
0 siblings, 1 reply; 30+ messages in thread
From: Shobhit Kumar @ 2014-05-20 16:16 UTC (permalink / raw)
To: Damien Lespiau; +Cc: Jani Nikula, Daniel Vetter, intel-gfx, arjan.van.de.ven
On Monday 19 May 2014 07:53 PM, Damien Lespiau wrote:
> On Mon, Apr 14, 2014 at 11:18:27AM +0530, Shobhit Kumar wrote:
>> +#define NS_MHZ_RATIO 1000000
>
> [...]
>
>> +static bool generic_init(struct intel_dsi_device *dsi)
>> +{
>
> [...]
>
>> + /*
>> + * ui(s) = 1/f [f in hz]
>> + * ui(ns) = 10^9/f*10^6 [f in Mhz] -> 10^3/f(Mhz)
>
> ui(ns) = 10^9/(f*10^6)
>
>> + *
>> + * LP byte clock = TLPX/8ui
>
> Mind putting that comment just above the appropriate computation?
> Also, LP byte clock = Tlpx / (8UI)
>
>> + *
>> + * Since txddrclkhs_i is 2xUI, the count values programmed in
>> + * DPHY param registers are divided by 2
>
> That looks like a general comment that apply to a bunch of calculations
> below, probably worth separating it from the UI comment.
>
Will update as suggested.
>> + *
>> + */
>> +
>> + /* in Kbps */
>> + ui_num = bitrate;
>> + ui_den = NS_MHZ_RATIO;
>
> I'm a bit confused here, most likely missing something, care to clarify?
>
> - IIUC, you want the computations to happen in ns. I'm a bit puzzled by
> that NS_MHZ_RATIO constant name when we're dealing with Kbps.
>
> That constant is 10^6 which seems to be OK for KHz. So maybe just a
> naming problem?
Yeah, I now realize that it should be something like NS_KHZ_RATIO to
avoid confusion
>
> - UI is a period, so is homogeneous to time (s), but ui_num being in
> s^-1 and ui_den a constant, ui_num/ui_den looks like a frequency. Or
> could it be that UI = ui_den / ui_num? would be confusing, but the
> code below would make more sense. In which case could we have UI =
> ui_num / ui_den?
I just kept ui_num and ui_den separately to take care of precision loss,
but I see how it is adding to confusion. Actually it is ui_den / ui_num
and we have all computations as 1/UI so it works. I think I will compute
UI directly as UI = (NS_KHZ_RATIO * 1000) /bitrate and divide by 1000
wherever we use to maintain precision. Sounds ok ?
>
>> +
>> + tclk_prepare_clkzero = mipi_config->tclk_prepare_clkzero;
>> + ths_prepare_hszero = mipi_config->ths_prepare_hszero;
>> +
>> + /* B060 */
>> + intel_dsi->lp_byte_clk = CEIL_DIV(tlpx_ns * ui_num, 8 * ui_den);
>> +
>> + /* count values in UI = (ns value) * (bitrate / (2 * 10^6)) */
>> + /* prepare count */
>> + ths_prepare_ns =
>> + (mipi_config->ths_prepare > mipi_config->tclk_prepare) ?
>> + mipi_config->ths_prepare :
>> + mipi_config->tclk_prepare;
>
> That looks like max()
>
>> +
>> + prepare_cnt = CEIL_DIV(ths_prepare_ns * ui_num, ui_den * 2);
>
> The formula above has a 10^6, why is that OK not to have it there? (in
> which unit is bitrate in the formula? MHz?) Is this something like:
>
> Count in UI = count(ns) / UI(ns)
>
> and then as UI = ui_den/ui_num (?!) we end up with:
>
> Count in UI = count(ns) * ui_num / ui_den
>
> And then the / 2 comment applies.
Yeah actually its like this. I will correct as suggested above.
>
>> +
>> + /* exit zero count */
>> + exit_zero_cnt = CEIL_DIV(
>> + (ths_prepare_hszero - ths_prepare_ns) * ui_num,
>> + ui_den * 2
>> + );
>> +
>> + /*
>> + * Exit zero is unified val ths_zero and ths_exit
>> + * minimum value for ths_exit = 110ns
>> + * min (exit_zero_cnt * 2) = 110/UI
>> + * exit_zero_cnt = 55/UI
>> + */
>> + if (exit_zero_cnt < (55 * ui_num / ui_den))
>> + if ((55 * ui_num) % ui_den)
>> + exit_zero_cnt += 1;
>
> I'm not sure what we're achieving with the +=1 here, mind explaining?
This is as per MIPI host controller spec to ceil the value
>
>> +
>> + /* clk zero count */
>> + clk_zero_cnt = CEIL_DIV(
>> + (tclk_prepare_clkzero - ths_prepare_ns)
>> + * ui_num, 2 * ui_den);
>> +
>> + /* trail count */
>> + tclk_trail_ns = (mipi_config->tclk_trail > mipi_config->ths_trail) ?
>> + mipi_config->tclk_trail : mipi_config->ths_trail;
>
> max()
>
>> + trail_cnt = CEIL_DIV(tclk_trail_ns * ui_num, 2 * ui_den);
>> +
>> + if (prepare_cnt > PREPARE_CNT_MAX ||
>> + exit_zero_cnt > EXIT_ZERO_CNT_MAX ||
>> + clk_zero_cnt > CLK_ZERO_CNT_MAX ||
>> + trail_cnt > TRAIL_CNT_MAX)
>> + DRM_DEBUG_DRIVER("Values crossing maximum limits\n");
>
> Is that a situation that may happen in a normal case? or should we go
> with a DRM_ERROR() here and potentially abort the modeset?
>
Generally it should not happen. We should not abort but clip to max
values though there is no guarantee it will work, but there is high
chance that it will work.
>> +
>> + if (prepare_cnt > PREPARE_CNT_MAX)
>> + prepare_cnt = PREPARE_CNT_MAX;
>> +
>> + if (exit_zero_cnt > EXIT_ZERO_CNT_MAX)
>> + exit_zero_cnt = EXIT_ZERO_CNT_MAX;
>> +
>> + if (clk_zero_cnt > CLK_ZERO_CNT_MAX)
>> + clk_zero_cnt = CLK_ZERO_CNT_MAX;
>> +
>> + if (trail_cnt > TRAIL_CNT_MAX)
>> + trail_cnt = TRAIL_CNT_MAX;
>> +
>> + /* B080 */
>> + intel_dsi->dphy_reg = exit_zero_cnt << 24 | trail_cnt << 16 |
>> + clk_zero_cnt << 8 | prepare_cnt;
>> +
>> + /*
>> + * LP to HS switch count = 4TLPX + PREP_COUNT * 2 + EXIT_ZERO_COUNT * 2
>> + * + 10UI + Extra Byte Count
>> + *
>> + * HS to LP switch count = THS-TRAIL + 2TLPX + Extra Byte Count
>> + * Extra Byte Count is calculated according to number of lanes.
>> + * High Low Switch Count is the Max of LP to HS and
>> + * HS to LP switch count
>> + *
>> + */
>> + tlpx_ui = CEIL_DIV(tlpx_ns * ui_num, ui_den);
>> +
>> + /* B044 */
>> + intel_dsi->hs_to_lp_count =
>> + CEIL_DIV(
>> + 4 * tlpx_ui + prepare_cnt * 2 +
>> + exit_zero_cnt * 2 + 10,
>> + 8);
>
> The previous was before I tried to look at the spec too closely. Mind
> explaining why we don't look at the HS to LP switch count? ie why HS to
> LP switch cound is always smaller than the LP to HS one?
Because LP to HS uses exit_zero_count which is generally higher than
clk_zero_count. So just directly used LP to HS which amounts to saying
that switching from HS to LP takes lesser time than switching from LP to
HS. I can/should add code to compute max of the two.
>
>> +
>> + intel_dsi->hs_to_lp_count += extra_byte_count;
>> +
>> + /* B088 */
>> + /* LP -> HS for clock lanes
>> + * LP clk sync + LP11 + LP01 + tclk_prepare + tclk_zero +
>> + * extra byte count
>> + * 2TPLX + 1TLPX + 1 TPLX(in ns) + prepare_cnt * 2 + clk_zero_cnt *
>> + * 2(in UI) + extra byte count
>> + * In byteclks = (4TLPX + prepare_cnt * 2 + clk_zero_cnt *2 (in UI)) /
>> + * 8 + extra byte count
>> + */
>> + intel_dsi->clk_lp_to_hs_count =
>> + CEIL_DIV(
>> + 4 * tlpx_ui + prepare_cnt * 2 +
>> + clk_zero_cnt * 2,
>> + 8);
>> +
>> + intel_dsi->clk_lp_to_hs_count += extra_byte_count;
>> +
>> + /* HS->LP for Clock Lanes
>> + * Low Power clock synchronisations + 1Tx byteclk + tclk_trail +
>> + * Extra byte count
>> + * 2TLPX + 8UI + (trail_count*2)(in UI) + Extra byte count
>> + * In byteclks = (2*TLpx(in UI) + trail_count*2 +8)(in UI)/8 +
>> + * Extra byte count
>> + */
>> + intel_dsi->clk_hs_to_lp_count =
>> + CEIL_DIV(2 * tlpx_ui + trail_cnt * 2 + 8,
>> + 8);
>> + intel_dsi->clk_hs_to_lp_count += extra_byte_count;
>> +
>> + DRM_DEBUG_KMS("EOT %s\n", intel_dsi->eotp_pkt ? "ENABLED" : "DISABLED");
>> + DRM_DEBUG_KMS("CLOCKSTOP %s\n", intel_dsi->clock_stop ?
>> + "ENABLED" : "DISABLED");
>> + DRM_DEBUG_KMS("Mode %s\n", intel_dsi->operation_mode ? "COMMAND" : "VIDEO");
>> + DRM_DEBUG_KMS("Pixel Format %d\n", intel_dsi->pixel_format);
>> + DRM_DEBUG_KMS("TLPX %d\n", intel_dsi->escape_clk_div);
>> + DRM_DEBUG_KMS("LP RX Timeout 0x%x\n", intel_dsi->lp_rx_timeout);
>> + DRM_DEBUG_KMS("Turnaround Timeout 0x%x\n", intel_dsi->turn_arnd_val);
>> + DRM_DEBUG_KMS("Init Count 0x%x\n", intel_dsi->init_count);
>> + DRM_DEBUG_KMS("HS to LP Count 0x%x\n", intel_dsi->hs_to_lp_count);
>> + DRM_DEBUG_KMS("LP Byte Clock %d\n", intel_dsi->lp_byte_clk);
>> + DRM_DEBUG_KMS("DBI BW Timer 0x%x\n", intel_dsi->bw_timer);
>> + DRM_DEBUG_KMS("LP to HS Clock Count 0x%x\n", intel_dsi->clk_lp_to_hs_count);
>> + DRM_DEBUG_KMS("HS to LP Clock Count 0x%x\n", intel_dsi->clk_hs_to_lp_count);
>> + DRM_DEBUG_KMS("BTA %s\n",
>> + intel_dsi->video_frmt_cfg_bits & DISABLE_VIDEO_BTA ?
>> + "DISABLED" : "ENABLED");
>> + DRM_DEBUG_KMS("B060 = 0x%Xx, B080 = 0x%x, B044 = 0x%x, B088 = 0x%x\n",
>> + intel_dsi->lp_byte_clk, intel_dsi->dphy_reg, intel_dsi->hs_to_lp_count,
>> + (intel_dsi->clk_lp_to_hs_count << LP_HS_SSW_CNT_SHIFT) |
>> + (intel_dsi->clk_hs_to_lp_count << HS_LP_PWR_SW_CNT_SHIFT));
>> +
>> + /* delays in VBT are in unit of 100us, so need to convert
>> + * here in ms
>> + * Delay (100us) * 100 /1000 = Delay / 10 (ms) */
>> + intel_dsi->backlight_off_delay = pps->bl_disable_delay / 10;
>> + intel_dsi->backlight_on_delay = pps->bl_enable_delay / 10;
>> + intel_dsi->panel_on_delay = pps->panel_on_delay / 10;
>> + intel_dsi->panel_off_delay = pps->panel_off_delay / 10;
>> + intel_dsi->panel_pwr_cycle_delay = pps->panel_power_cycle_delay / 10;
>> +
>> + return true;
>> +}
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver
2014-05-20 16:16 ` Shobhit Kumar
@ 2014-05-20 20:55 ` Damien Lespiau
2014-05-22 7:45 ` Kumar, Shobhit
0 siblings, 1 reply; 30+ messages in thread
From: Damien Lespiau @ 2014-05-20 20:55 UTC (permalink / raw)
To: Shobhit Kumar; +Cc: Jani Nikula, Daniel Vetter, intel-gfx, arjan.van.de.ven
On Tue, May 20, 2014 at 09:46:01PM +0530, Shobhit Kumar wrote:
> >- UI is a period, so is homogeneous to time (s), but ui_num being in
> > s^-1 and ui_den a constant, ui_num/ui_den looks like a frequency. Or
> > could it be that UI = ui_den / ui_num? would be confusing, but the
> > code below would make more sense. In which case could we have UI =
> > ui_num / ui_den?
>
> I just kept ui_num and ui_den separately to take care of precision
> loss, but I see how it is adding to confusion. Actually it is ui_den
> / ui_num and we have all computations as 1/UI so it works. I think I
> will compute UI directly as UI = (NS_KHZ_RATIO * 1000) /bitrate and
> divide by 1000 wherever we use to maintain precision. Sounds ok ?
I think just exchanging the two variable names (ui_num and ui_den)
should be less work for you and should be enough. It's really just about
having ui_num being the UI numerator so the reader is not too surprised
> >>+ /* B044 */
> >>+ intel_dsi->hs_to_lp_count =
> >>+ CEIL_DIV(
> >>+ 4 * tlpx_ui + prepare_cnt * 2 +
> >>+ exit_zero_cnt * 2 + 10,
> >>+ 8);
> >
> >The previous was before I tried to look at the spec too closely. Mind
> >explaining why we don't look at the HS to LP switch count? ie why HS to
> >LP switch cound is always smaller than the LP to HS one?
>
> Because LP to HS uses exit_zero_count which is generally higher than
> clk_zero_count. So just directly used LP to HS which amounts to
> saying that switching from HS to LP takes lesser time than switching
> from LP to HS. I can/should add code to compute max of the two.
This could go to a separate task if you don't have time right now,
Thanks for your answers!
--
Damien
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver
2014-05-20 20:55 ` Damien Lespiau
@ 2014-05-22 7:45 ` Kumar, Shobhit
0 siblings, 0 replies; 30+ messages in thread
From: Kumar, Shobhit @ 2014-05-22 7:45 UTC (permalink / raw)
To: Damien Lespiau; +Cc: Jani Nikula, Daniel Vetter, intel-gfx, arjan.van.de.ven
On 5/21/2014 2:25 AM, Damien Lespiau wrote:
> On Tue, May 20, 2014 at 09:46:01PM +0530, Shobhit Kumar wrote:
>>> - UI is a period, so is homogeneous to time (s), but ui_num being in
>>> s^-1 and ui_den a constant, ui_num/ui_den looks like a frequency. Or
>>> could it be that UI = ui_den / ui_num? would be confusing, but the
>>> code below would make more sense. In which case could we have UI =
>>> ui_num / ui_den?
>>
>> I just kept ui_num and ui_den separately to take care of precision
>> loss, but I see how it is adding to confusion. Actually it is ui_den
>> / ui_num and we have all computations as 1/UI so it works. I think I
>> will compute UI directly as UI = (NS_KHZ_RATIO * 1000) /bitrate and
>> divide by 1000 wherever we use to maintain precision. Sounds ok ?
>
> I think just exchanging the two variable names (ui_num and ui_den)
> should be less work for you and should be enough. It's really just about
> having ui_num being the UI numerator so the reader is not too surprised
Yeah. Will fix this
>
>>>> + /* B044 */
>>>> + intel_dsi->hs_to_lp_count =
>>>> + CEIL_DIV(
>>>> + 4 * tlpx_ui + prepare_cnt * 2 +
>>>> + exit_zero_cnt * 2 + 10,
>>>> + 8);
>>>
>>> The previous was before I tried to look at the spec too closely. Mind
>>> explaining why we don't look at the HS to LP switch count? ie why HS to
>>> LP switch cound is always smaller than the LP to HS one?
>>
>> Because LP to HS uses exit_zero_count which is generally higher than
>> clk_zero_count. So just directly used LP to HS which amounts to
>> saying that switching from HS to LP takes lesser time than switching
>> from LP to HS. I can/should add code to compute max of the two.
>
> This could go to a separate task if you don't have time right now,
>
Most likely I can do this as well. Will push the updated patch by
sometime tomorrow.
Regards
Shobhit
^ permalink raw reply [flat|nested] 30+ messages in thread
* [v2] drm/i915: Add support for Generic MIPI panel driver
2014-04-14 5:48 ` [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver Shobhit Kumar
2014-05-15 16:48 ` Damien Lespiau
2014-05-19 14:23 ` Damien Lespiau
@ 2014-05-23 16:05 ` Shobhit Kumar
2014-05-27 11:02 ` Damien Lespiau
2 siblings, 1 reply; 30+ messages in thread
From: Shobhit Kumar @ 2014-05-23 16:05 UTC (permalink / raw)
To: intel-gfx; +Cc: Jani Nikula, Daniel Vetter
This driver makes use of the generic panel information from the VBT.
Panel information is classified into two - panel configuration and panel
power sequence which is unique to each panel. The generic driver uses the
panel configuration and sequence parsed from VBT block #52 and #53
v2: Address review comments by Jani
- Move all of the things in driver c file from header
- Make all functions static
- Make use of video/mipi_display.c instead of redefining
- Null checks during sequence execution
v3: Address review comments by Damien
- Rename the panel driver file as intel_dsi_panel_vbt.c
- Fix style changes as suggested
- Correct comments for lp->hs and hs->lp count calculations
- General updating comments to have more clarity
- using max() instead of ternary operator
- Fix names (ui_num, ui_den) while using UI in calculations
- compute max of lp_to_hs switch and hs_to_lp switch while computing
hs_lp_switch_count
Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/intel_dsi.c | 5 +
drivers/gpu/drm/i915/intel_dsi.h | 2 +
drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 589 +++++++++++++++++++++++++++++
4 files changed, 597 insertions(+)
create mode 100644 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 7b2f3be..cad1683 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -62,6 +62,7 @@ i915-y += dvo_ch7017.o \
intel_dsi_cmd.o \
intel_dsi.o \
intel_dsi_pll.o \
+ intel_dsi_panel_vbt.o \
intel_dvo.o \
intel_hdmi.o \
intel_i2c.o \
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 2525cdd..e73bec6 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -35,6 +35,11 @@
/* the sub-encoders aka panel drivers */
static const struct intel_dsi_device intel_dsi_devices[] = {
+ {
+ .panel_id = MIPI_DSI_GENERIC_PANEL_ID,
+ .name = "vbt-generic-dsi-vid-mode-display",
+ .dev_ops = &vbt_generic_dsi_display_ops,
+ },
};
static void band_gap_reset(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index e3f4e91..31db33d 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -133,4 +133,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);
+extern struct intel_dsi_dev_ops vbt_generic_dsi_display_ops;
+
#endif /* _INTEL_DSI_H */
diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
new file mode 100644
index 0000000..21a0d34
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -0,0 +1,589 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Author: Shobhit Kumar <shobhit.kumar@intel.com>
+ *
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_edid.h>
+#include <drm/i915_drm.h>
+#include <linux/slab.h>
+#include <video/mipi_display.h>
+#include <asm/intel-mid.h>
+#include <video/mipi_display.h>
+#include "i915_drv.h"
+#include "intel_drv.h"
+#include "intel_dsi.h"
+#include "intel_dsi_cmd.h"
+
+#define MIPI_TRANSFER_MODE_SHIFT 0
+#define MIPI_VIRTUAL_CHANNEL_SHIFT 1
+#define MIPI_PORT_SHIFT 3
+
+#define PREPARE_CNT_MAX 0x3F
+#define EXIT_ZERO_CNT_MAX 0x3F
+#define CLK_ZERO_CNT_MAX 0xFF
+#define TRAIL_CNT_MAX 0x1F
+
+#define NS_KHZ_RATIO 1000000
+
+#define GPI0_NC_0_HV_DDI0_HPD 0x4130
+#define GPIO_NC_0_HV_DDI0_PAD 0x4138
+#define GPIO_NC_1_HV_DDI0_DDC_SDA 0x4120
+#define GPIO_NC_1_HV_DDI0_DDC_SDA_PAD 0x4128
+#define GPIO_NC_2_HV_DDI0_DDC_SCL 0x4110
+#define GPIO_NC_2_HV_DDI0_DDC_SCL_PAD 0x4118
+#define GPIO_NC_3_PANEL0_VDDEN 0x4140
+#define GPIO_NC_3_PANEL0_VDDEN_PAD 0x4148
+#define GPIO_NC_4_PANEL0_BLKEN 0x4150
+#define GPIO_NC_4_PANEL0_BLKEN_PAD 0x4158
+#define GPIO_NC_5_PANEL0_BLKCTL 0x4160
+#define GPIO_NC_5_PANEL0_BLKCTL_PAD 0x4168
+#define GPIO_NC_6_PCONF0 0x4180
+#define GPIO_NC_6_PAD 0x4188
+#define GPIO_NC_7_PCONF0 0x4190
+#define GPIO_NC_7_PAD 0x4198
+#define GPIO_NC_8_PCONF0 0x4170
+#define GPIO_NC_8_PAD 0x4178
+#define GPIO_NC_9_PCONF0 0x4100
+#define GPIO_NC_9_PAD 0x4108
+#define GPIO_NC_10_PCONF0 0x40E0
+#define GPIO_NC_10_PAD 0x40E8
+#define GPIO_NC_11_PCONF0 0x40F0
+#define GPIO_NC_11_PAD 0x40F8
+
+struct gpio_table {
+ u16 function_reg;
+ u16 pad_reg;
+ u8 init;
+};
+
+static struct gpio_table gtable[] = {
+ { GPI0_NC_0_HV_DDI0_HPD, GPIO_NC_0_HV_DDI0_PAD, 0 },
+ { GPIO_NC_1_HV_DDI0_DDC_SDA, GPIO_NC_1_HV_DDI0_DDC_SDA_PAD, 0 },
+ { GPIO_NC_2_HV_DDI0_DDC_SCL, GPIO_NC_2_HV_DDI0_DDC_SCL_PAD, 0 },
+ { GPIO_NC_3_PANEL0_VDDEN, GPIO_NC_3_PANEL0_VDDEN_PAD, 0 },
+ { GPIO_NC_4_PANEL0_BLKEN, GPIO_NC_4_PANEL0_BLKEN_PAD, 0 },
+ { GPIO_NC_5_PANEL0_BLKCTL, GPIO_NC_5_PANEL0_BLKCTL_PAD, 0 },
+ { GPIO_NC_6_PCONF0, GPIO_NC_6_PAD, 0 },
+ { GPIO_NC_7_PCONF0, GPIO_NC_7_PAD, 0 },
+ { GPIO_NC_8_PCONF0, GPIO_NC_8_PAD, 0 },
+ { GPIO_NC_9_PCONF0, GPIO_NC_9_PAD, 0 },
+ { GPIO_NC_10_PCONF0, GPIO_NC_10_PAD, 0},
+ { GPIO_NC_11_PCONF0, GPIO_NC_11_PAD, 0}
+};
+
+static u8 *mipi_exec_send_packet(struct intel_dsi *intel_dsi, u8 *data)
+{
+ u8 type, byte, mode, vc, port;
+ u16 len;
+
+ byte = *data++;
+ mode = (byte >> MIPI_TRANSFER_MODE_SHIFT) & 0x1;
+ vc = (byte >> MIPI_VIRTUAL_CHANNEL_SHIFT) & 0x3;
+ port = (byte >> MIPI_PORT_SHIFT) & 0x3;
+
+ /* LP or HS mode */
+ intel_dsi->hs = mode;
+
+ /* get packet type and increment the pointer */
+ type = *data++;
+
+ len = *((u16 *) data);
+ data += 2;
+
+ switch (type) {
+ case MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM:
+ dsi_vc_generic_write_0(intel_dsi, vc);
+ break;
+ case MIPI_DSI_GENERIC_SHORT_WRITE_1_PARAM:
+ dsi_vc_generic_write_1(intel_dsi, vc, *data);
+ break;
+ case MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM:
+ dsi_vc_generic_write_2(intel_dsi, vc, *data, *(data + 1));
+ break;
+ case MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM:
+ case MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM:
+ case MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM:
+ DRM_DEBUG_DRIVER("Generic Read not yet implemented or used\n");
+ break;
+ case MIPI_DSI_GENERIC_LONG_WRITE:
+ dsi_vc_generic_write(intel_dsi, vc, data, len);
+ break;
+ case MIPI_DSI_DCS_SHORT_WRITE:
+ dsi_vc_dcs_write_0(intel_dsi, vc, *data);
+ break;
+ case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
+ dsi_vc_dcs_write_1(intel_dsi, vc, *data, *(data + 1));
+ break;
+ case MIPI_DSI_DCS_READ:
+ DRM_DEBUG_DRIVER("DCS Read not yet implemented or used\n");
+ break;
+ case MIPI_DSI_DCS_LONG_WRITE:
+ dsi_vc_dcs_write(intel_dsi, vc, data, len);
+ break;
+ };
+
+ data += len;
+
+ return data;
+}
+
+static u8 *mipi_exec_delay(struct intel_dsi *intel_dsi, u8 *data)
+{
+ u32 delay = *((u32 *) data);
+
+ usleep_range(delay, delay + 10);
+ data += 4;
+
+ return data;
+}
+
+static u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, u8 *data)
+{
+ u8 gpio, action;
+ u16 function, pad;
+ u32 val;
+ struct drm_device *dev = intel_dsi->base.base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+
+ gpio = *data++;
+
+ /* pull up/down */
+ action = *data++;
+
+ function = gtable[gpio].function_reg;
+ pad = gtable[gpio].pad_reg;
+
+ mutex_lock(&dev_priv->dpio_lock);
+ if (!gtable[gpio].init) {
+ /* program the function */
+ /* FIXME: remove constant below */
+ vlv_gpio_nc_write(dev_priv, function, 0x2000CC00);
+ gtable[gpio].init = 1;
+ }
+
+ val = 0x4 | action;
+
+ /* pull up/down */
+ vlv_gpio_nc_write(dev_priv, pad, val);
+ mutex_unlock(&dev_priv->dpio_lock);
+
+ return data;
+}
+
+typedef u8 * (*fn_mipi_elem_exec)(struct intel_dsi *intel_dsi, 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 Sequence from VBT #53 parsing logic
+ * We have already separated each seqence during bios parsing
+ * Following is generic execution function for any sequence
+ */
+
+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"
+};
+
+static void generic_exec_sequence(struct intel_dsi *intel_dsi, char *sequence)
+{
+ u8 *data = sequence;
+ fn_mipi_elem_exec mipi_elem_exec;
+ int index;
+
+ if (!sequence)
+ return;
+
+ DRM_DEBUG_DRIVER("Starting MIPI sequence - %s\n", seq_name[*data]);
+
+ /* go to the first element of the sequence */
+ data++;
+
+ /* parse each byte till we reach end of sequence byte - 0x00 */
+ while (1) {
+ index = *data;
+ mipi_elem_exec = exec_elem[index];
+ if (!mipi_elem_exec) {
+ DRM_ERROR("Unsupported MIPI element, skipping sequence execution\n");
+ return;
+ }
+
+ /* goto element payload */
+ data++;
+
+ /* execute the element specific rotines */
+ data = mipi_elem_exec(intel_dsi, data);
+
+ /*
+ * After processing the element, data should point to
+ * next element or end of sequence
+ * check if have we reached end of sequence
+ */
+ if (*data == 0x00)
+ break;
+ }
+}
+
+static bool generic_init(struct intel_dsi_device *dsi)
+{
+ struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+ struct drm_device *dev = intel_dsi->base.base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct mipi_config *mipi_config = dev_priv->vbt.dsi.config;
+ struct mipi_pps_data *pps = dev_priv->vbt.dsi.pps;
+ struct drm_display_mode *mode = dev_priv->vbt.lfp_lvds_vbt_mode;
+ u32 bits_per_pixel = 24;
+ u32 tlpx_ns, extra_byte_count, bitrate, tlpx_ui;
+ u32 ui_num, ui_den;
+ u32 prepare_cnt, exit_zero_cnt, clk_zero_cnt, trail_cnt;
+ u32 ths_prepare_ns, tclk_trail_ns;
+ u32 tclk_prepare_clkzero, ths_prepare_hszero;
+ u32 lp_to_hs_switch, hs_to_lp_switch;
+
+ DRM_DEBUG_KMS("\n");
+
+ intel_dsi->eotp_pkt = mipi_config->eot_pkt_disabled ? 0 : 1;
+ intel_dsi->clock_stop = mipi_config->enable_clk_stop ? 1 : 0;
+ intel_dsi->lane_count = mipi_config->lane_cnt + 1;
+ intel_dsi->pixel_format = mipi_config->videomode_color_format << 7;
+
+ if (intel_dsi->pixel_format == VID_MODE_FORMAT_RGB666)
+ bits_per_pixel = 18;
+ else if (intel_dsi->pixel_format == VID_MODE_FORMAT_RGB565)
+ bits_per_pixel = 16;
+
+ bitrate = (mode->clock * bits_per_pixel) / intel_dsi->lane_count;
+
+ intel_dsi->operation_mode = mipi_config->is_cmd_mode;
+ intel_dsi->video_mode_format = mipi_config->video_transfer_mode;
+ intel_dsi->escape_clk_div = mipi_config->byte_clk_sel;
+ intel_dsi->lp_rx_timeout = mipi_config->lp_rx_timeout;
+ intel_dsi->turn_arnd_val = mipi_config->turn_around_timeout;
+ intel_dsi->rst_timer_val = mipi_config->device_reset_timer;
+ intel_dsi->init_count = mipi_config->master_init_timer;
+ intel_dsi->bw_timer = mipi_config->dbi_bw_timer;
+ intel_dsi->video_frmt_cfg_bits = mipi_config->bta_enabled ? DISABLE_VIDEO_BTA : 0;
+
+ switch (intel_dsi->escape_clk_div) {
+ case 0:
+ tlpx_ns = 50;
+ break;
+ case 1:
+ tlpx_ns = 100;
+ break;
+
+ case 2:
+ tlpx_ns = 200;
+ break;
+ default:
+ tlpx_ns = 50;
+ break;
+ }
+
+ switch (intel_dsi->lane_count) {
+ case 1:
+ case 2:
+ extra_byte_count = 2;
+ break;
+ case 3:
+ extra_byte_count = 4;
+ break;
+ case 4:
+ default:
+ extra_byte_count = 3;
+ break;
+ }
+
+ /*
+ * ui(s) = 1/f [f in hz]
+ * ui(ns) = 10^9 / (f*10^6) [f in Mhz] -> 10^3/f(Mhz)
+ */
+
+ /* in Kbps */
+ ui_num = NS_KHZ_RATIO;
+ ui_den = bitrate;
+
+ tclk_prepare_clkzero = mipi_config->tclk_prepare_clkzero;
+ ths_prepare_hszero = mipi_config->ths_prepare_hszero;
+
+ /*
+ * B060
+ * LP byte clock = TLPX/ (8UI)
+ */
+ intel_dsi->lp_byte_clk = DIV_ROUND_UP(tlpx_ns * ui_den, 8 * ui_num);
+
+ /* count values in UI = (ns value) * (bitrate / (2 * 10^6))
+ *
+ * Since txddrclkhs_i is 2xUI, all the count values programmed in
+ * DPHY param register are divided by 2
+ *
+ * prepare count
+ */
+ ths_prepare_ns = max(mipi_config->ths_prepare, mipi_config->tclk_prepare);
+ prepare_cnt = DIV_ROUND_UP(ths_prepare_ns * ui_den, ui_num * 2);
+
+ /* exit zero count */
+ exit_zero_cnt = DIV_ROUND_UP(
+ (ths_prepare_hszero - ths_prepare_ns) * ui_den,
+ ui_num * 2
+ );
+
+ /*
+ * Exit zero is unified val ths_zero and ths_exit
+ * minimum value for ths_exit = 110ns
+ * min (exit_zero_cnt * 2) = 110/UI
+ * exit_zero_cnt = 55/UI
+ */
+ if (exit_zero_cnt < (55 * ui_den / ui_num))
+ if ((55 * ui_den) % ui_num)
+ exit_zero_cnt += 1;
+
+ /* clk zero count */
+ clk_zero_cnt = DIV_ROUND_UP(
+ (tclk_prepare_clkzero - ths_prepare_ns)
+ * ui_den, 2 * ui_num);
+
+ /* trail count */
+ tclk_trail_ns = max(mipi_config->tclk_trail, mipi_config->ths_trail);
+ trail_cnt = DIV_ROUND_UP(tclk_trail_ns * ui_den, 2 * ui_num);
+
+ if (prepare_cnt > PREPARE_CNT_MAX ||
+ exit_zero_cnt > EXIT_ZERO_CNT_MAX ||
+ clk_zero_cnt > CLK_ZERO_CNT_MAX ||
+ trail_cnt > TRAIL_CNT_MAX)
+ DRM_DEBUG_DRIVER("Values crossing maximum limits, restricting to max values\n");
+
+ if (prepare_cnt > PREPARE_CNT_MAX)
+ prepare_cnt = PREPARE_CNT_MAX;
+
+ if (exit_zero_cnt > EXIT_ZERO_CNT_MAX)
+ exit_zero_cnt = EXIT_ZERO_CNT_MAX;
+
+ if (clk_zero_cnt > CLK_ZERO_CNT_MAX)
+ clk_zero_cnt = CLK_ZERO_CNT_MAX;
+
+ if (trail_cnt > TRAIL_CNT_MAX)
+ trail_cnt = TRAIL_CNT_MAX;
+
+ /* B080 */
+ intel_dsi->dphy_reg = exit_zero_cnt << 24 | trail_cnt << 16 |
+ clk_zero_cnt << 8 | prepare_cnt;
+
+ /*
+ * LP to HS switch count = 4TLPX + PREP_COUNT * 2 + EXIT_ZERO_COUNT * 2
+ * + 10UI + Extra Byte Count
+ *
+ * HS to LP switch count = THS-TRAIL + 2TLPX + Extra Byte Count
+ * Extra Byte Count is calculated according to number of lanes.
+ * High Low Switch Count is the Max of LP to HS and
+ * HS to LP switch count
+ *
+ */
+ tlpx_ui = DIV_ROUND_UP(tlpx_ns * ui_den, ui_num);
+
+ /* B044 */
+ /* FIXME:
+ * The comment above does not match with the code */
+ lp_to_hs_switch = DIV_ROUND_UP(4 * tlpx_ui + prepare_cnt * 2 +
+ exit_zero_cnt * 2 + 10, 8);
+
+ hs_to_lp_switch = DIV_ROUND_UP(mipi_config->ths_trail + 2 * tlpx_ui, 8);
+
+ intel_dsi->hs_to_lp_count = max(lp_to_hs_switch, hs_to_lp_switch);
+ intel_dsi->hs_to_lp_count += extra_byte_count;
+
+ /* B088 */
+ /* LP -> HS for clock lanes
+ * LP clk sync + LP11 + LP01 + tclk_prepare + tclk_zero +
+ * extra byte count
+ * 2TPLX + 1TLPX + 1 TPLX(in ns) + prepare_cnt * 2 + clk_zero_cnt *
+ * 2(in UI) + extra byte count
+ * In byteclks = (4TLPX + prepare_cnt * 2 + clk_zero_cnt *2 (in UI)) /
+ * 8 + extra byte count
+ */
+ intel_dsi->clk_lp_to_hs_count =
+ DIV_ROUND_UP(
+ 4 * tlpx_ui + prepare_cnt * 2 +
+ clk_zero_cnt * 2,
+ 8);
+
+ intel_dsi->clk_lp_to_hs_count += extra_byte_count;
+
+ /* HS->LP for Clock Lanes
+ * Low Power clock synchronisations + 1Tx byteclk + tclk_trail +
+ * Extra byte count
+ * 2TLPX + 8UI + (trail_count*2)(in UI) + Extra byte count
+ * In byteclks = (2*TLpx(in UI) + trail_count*2 +8)(in UI)/8 +
+ * Extra byte count
+ */
+ intel_dsi->clk_hs_to_lp_count =
+ DIV_ROUND_UP(2 * tlpx_ui + trail_cnt * 2 + 8,
+ 8);
+ intel_dsi->clk_hs_to_lp_count += extra_byte_count;
+
+ DRM_DEBUG_KMS("Eot %s\n", intel_dsi->eotp_pkt ? "enabled" : "disabled");
+ DRM_DEBUG_KMS("Clockstop %s\n", intel_dsi->clock_stop ?
+ "disabled" : "enabled");
+ DRM_DEBUG_KMS("Mode %s\n", intel_dsi->operation_mode ? "command" : "video");
+ DRM_DEBUG_KMS("Pixel Format %d\n", intel_dsi->pixel_format);
+ DRM_DEBUG_KMS("TLPX %d\n", intel_dsi->escape_clk_div);
+ DRM_DEBUG_KMS("LP RX Timeout 0x%x\n", intel_dsi->lp_rx_timeout);
+ DRM_DEBUG_KMS("Turnaround Timeout 0x%x\n", intel_dsi->turn_arnd_val);
+ DRM_DEBUG_KMS("Init Count 0x%x\n", intel_dsi->init_count);
+ DRM_DEBUG_KMS("HS to LP Count 0x%x\n", intel_dsi->hs_to_lp_count);
+ DRM_DEBUG_KMS("LP Byte Clock %d\n", intel_dsi->lp_byte_clk);
+ DRM_DEBUG_KMS("DBI BW Timer 0x%x\n", intel_dsi->bw_timer);
+ DRM_DEBUG_KMS("LP to HS Clock Count 0x%x\n", intel_dsi->clk_lp_to_hs_count);
+ DRM_DEBUG_KMS("HS to LP Clock Count 0x%x\n", intel_dsi->clk_hs_to_lp_count);
+ DRM_DEBUG_KMS("BTA %s\n",
+ intel_dsi->video_frmt_cfg_bits & DISABLE_VIDEO_BTA ?
+ "disabled" : "enabled");
+
+ /* delays in VBT are in unit of 100us, so need to convert
+ * here in ms
+ * Delay (100us) * 100 /1000 = Delay / 10 (ms) */
+ intel_dsi->backlight_off_delay = pps->bl_disable_delay / 10;
+ intel_dsi->backlight_on_delay = pps->bl_enable_delay / 10;
+ intel_dsi->panel_on_delay = pps->panel_on_delay / 10;
+ intel_dsi->panel_off_delay = pps->panel_off_delay / 10;
+ intel_dsi->panel_pwr_cycle_delay = pps->panel_power_cycle_delay / 10;
+
+ return true;
+}
+
+static int generic_mode_valid(struct intel_dsi_device *dsi,
+ struct drm_display_mode *mode)
+{
+ return MODE_OK;
+}
+
+static bool generic_mode_fixup(struct intel_dsi_device *dsi,
+ const struct drm_display_mode *mode,
+ struct drm_display_mode *adjusted_mode) {
+ return true;
+}
+
+static void generic_panel_reset(struct intel_dsi_device *dsi)
+{
+ struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+ struct drm_device *dev = intel_dsi->base.base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+
+ char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_ASSERT_RESET];
+
+ generic_exec_sequence(intel_dsi, sequence);
+}
+
+static void generic_disable_panel_power(struct intel_dsi_device *dsi)
+{
+ struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+ struct drm_device *dev = intel_dsi->base.base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+
+ char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET];
+
+ generic_exec_sequence(intel_dsi, sequence);
+}
+
+static void generic_send_otp_cmds(struct intel_dsi_device *dsi)
+{
+ struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+ struct drm_device *dev = intel_dsi->base.base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+
+ char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP];
+
+ generic_exec_sequence(intel_dsi, sequence);
+}
+
+static void generic_enable(struct intel_dsi_device *dsi)
+{
+ struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+ struct drm_device *dev = intel_dsi->base.base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+
+ char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_ON];
+
+ generic_exec_sequence(intel_dsi, sequence);
+}
+
+static void generic_disable(struct intel_dsi_device *dsi)
+{
+ struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+ struct drm_device *dev = intel_dsi->base.base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+
+ char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_OFF];
+
+ generic_exec_sequence(intel_dsi, sequence);
+}
+
+static enum drm_connector_status generic_detect(struct intel_dsi_device *dsi)
+{
+ return connector_status_connected;
+}
+
+static bool generic_get_hw_state(struct intel_dsi_device *dev)
+{
+ return true;
+}
+
+static struct drm_display_mode *generic_get_modes(struct intel_dsi_device *dsi)
+{
+ struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+ struct drm_device *dev = intel_dsi->base.base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+
+ dev_priv->vbt.lfp_lvds_vbt_mode->type |= DRM_MODE_TYPE_PREFERRED;
+ return dev_priv->vbt.lfp_lvds_vbt_mode;
+}
+
+static void generic_destroy(struct intel_dsi_device *dsi) { }
+
+/* Callbacks. We might not need them all. */
+struct intel_dsi_dev_ops vbt_generic_dsi_display_ops = {
+ .init = generic_init,
+ .mode_valid = generic_mode_valid,
+ .mode_fixup = generic_mode_fixup,
+ .panel_reset = generic_panel_reset,
+ .disable_panel_power = generic_disable_panel_power,
+ .send_otp_cmds = generic_send_otp_cmds,
+ .enable = generic_enable,
+ .disable = generic_disable,
+ .detect = generic_detect,
+ .get_hw_state = generic_get_hw_state,
+ .get_modes = generic_get_modes,
+ .destroy = generic_destroy,
+};
--
1.8.3.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [v2] drm/i915: Add support for Generic MIPI panel driver
2014-05-23 16:05 ` [v2] " Shobhit Kumar
@ 2014-05-27 11:02 ` Damien Lespiau
2014-05-27 11:21 ` Kumar, Shobhit
0 siblings, 1 reply; 30+ messages in thread
From: Damien Lespiau @ 2014-05-27 11:02 UTC (permalink / raw)
To: Shobhit Kumar; +Cc: Jani Nikula, Daniel Vetter, intel-gfx
On Fri, May 23, 2014 at 09:35:27PM +0530, Shobhit Kumar wrote:
> This driver makes use of the generic panel information from the VBT.
> Panel information is classified into two - panel configuration and panel
> power sequence which is unique to each panel. The generic driver uses the
> panel configuration and sequence parsed from VBT block #52 and #53
>
> v2: Address review comments by Jani
> - Move all of the things in driver c file from header
> - Make all functions static
> - Make use of video/mipi_display.c instead of redefining
> - Null checks during sequence execution
>
> v3: Address review comments by Damien
> - Rename the panel driver file as intel_dsi_panel_vbt.c
> - Fix style changes as suggested
> - Correct comments for lp->hs and hs->lp count calculations
> - General updating comments to have more clarity
> - using max() instead of ternary operator
> - Fix names (ui_num, ui_den) while using UI in calculations
> - compute max of lp_to_hs switch and hs_to_lp switch while computing
> hs_lp_switch_count
>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
Hopefully still works after all that :)
Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
There's a small style issue I noticed with a last peek at the patch.
> +static bool generic_mode_fixup(struct intel_dsi_device *dsi,
> + const struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode) {
> + return true;
> +}
--
Damien
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [v2] drm/i915: Add support for Generic MIPI panel driver
2014-05-27 11:02 ` Damien Lespiau
@ 2014-05-27 11:21 ` Kumar, Shobhit
2014-05-27 11:37 ` Daniel Vetter
2014-05-27 11:39 ` Daniel Vetter
0 siblings, 2 replies; 30+ messages in thread
From: Kumar, Shobhit @ 2014-05-27 11:21 UTC (permalink / raw)
To: Damien Lespiau; +Cc: Jani Nikula, Daniel Vetter, intel-gfx
On 5/27/2014 4:32 PM, Damien Lespiau wrote:
> On Fri, May 23, 2014 at 09:35:27PM +0530, Shobhit Kumar wrote:
>> This driver makes use of the generic panel information from the VBT.
>> Panel information is classified into two - panel configuration and panel
>> power sequence which is unique to each panel. The generic driver uses the
>> panel configuration and sequence parsed from VBT block #52 and #53
>>
>> v2: Address review comments by Jani
>> - Move all of the things in driver c file from header
>> - Make all functions static
>> - Make use of video/mipi_display.c instead of redefining
>> - Null checks during sequence execution
>>
>> v3: Address review comments by Damien
>> - Rename the panel driver file as intel_dsi_panel_vbt.c
>> - Fix style changes as suggested
>> - Correct comments for lp->hs and hs->lp count calculations
>> - General updating comments to have more clarity
>> - using max() instead of ternary operator
>> - Fix names (ui_num, ui_den) while using UI in calculations
>> - compute max of lp_to_hs switch and hs_to_lp switch while computing
>> hs_lp_switch_count
>>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>
> Hopefully still works after all that :)
It does, at least on AsusT100 :) Thanks for the review
Regards
Shobhit
>
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
>
> There's a small style issue I noticed with a last peek at the patch.
>> +static bool generic_mode_fixup(struct intel_dsi_device *dsi,
>> + const struct drm_display_mode *mode,
>> + struct drm_display_mode *adjusted_mode) {
>> + return true;
>> +}
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [v2] drm/i915: Add support for Generic MIPI panel driver
2014-05-27 11:21 ` Kumar, Shobhit
@ 2014-05-27 11:37 ` Daniel Vetter
2014-05-27 11:39 ` Daniel Vetter
1 sibling, 0 replies; 30+ messages in thread
From: Daniel Vetter @ 2014-05-27 11:37 UTC (permalink / raw)
To: Kumar, Shobhit; +Cc: Jani Nikula, Daniel Vetter, intel-gfx
On Tue, May 27, 2014 at 04:51:55PM +0530, Kumar, Shobhit wrote:
> On 5/27/2014 4:32 PM, Damien Lespiau wrote:
> >On Fri, May 23, 2014 at 09:35:27PM +0530, Shobhit Kumar wrote:
> >>This driver makes use of the generic panel information from the VBT.
> >>Panel information is classified into two - panel configuration and panel
> >>power sequence which is unique to each panel. The generic driver uses the
> >>panel configuration and sequence parsed from VBT block #52 and #53
> >>
> >>v2: Address review comments by Jani
> >> - Move all of the things in driver c file from header
> >> - Make all functions static
> >> - Make use of video/mipi_display.c instead of redefining
> >> - Null checks during sequence execution
> >>
> >>v3: Address review comments by Damien
> >> - Rename the panel driver file as intel_dsi_panel_vbt.c
> >> - Fix style changes as suggested
> >> - Correct comments for lp->hs and hs->lp count calculations
> >> - General updating comments to have more clarity
> >> - using max() instead of ternary operator
> >> - Fix names (ui_num, ui_den) while using UI in calculations
> >> - compute max of lp_to_hs switch and hs_to_lp switch while computing
> >> hs_lp_switch_count
> >>
> >>Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> >
> >Hopefully still works after all that :)
>
> It does, at least on AsusT100 :) Thanks for the review
Queued for -next, thanks for the patch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [v2] drm/i915: Add support for Generic MIPI panel driver
2014-05-27 11:21 ` Kumar, Shobhit
2014-05-27 11:37 ` Daniel Vetter
@ 2014-05-27 11:39 ` Daniel Vetter
2014-05-27 11:42 ` Kumar, Shobhit
1 sibling, 1 reply; 30+ messages in thread
From: Daniel Vetter @ 2014-05-27 11:39 UTC (permalink / raw)
To: Kumar, Shobhit; +Cc: Jani Nikula, Daniel Vetter, intel-gfx
On Tue, May 27, 2014 at 04:51:55PM +0530, Kumar, Shobhit wrote:
> On 5/27/2014 4:32 PM, Damien Lespiau wrote:
> >On Fri, May 23, 2014 at 09:35:27PM +0530, Shobhit Kumar wrote:
> >>This driver makes use of the generic panel information from the VBT.
> >>Panel information is classified into two - panel configuration and panel
> >>power sequence which is unique to each panel. The generic driver uses the
> >>panel configuration and sequence parsed from VBT block #52 and #53
> >>
> >>v2: Address review comments by Jani
> >> - Move all of the things in driver c file from header
> >> - Make all functions static
> >> - Make use of video/mipi_display.c instead of redefining
> >> - Null checks during sequence execution
> >>
> >>v3: Address review comments by Damien
> >> - Rename the panel driver file as intel_dsi_panel_vbt.c
> >> - Fix style changes as suggested
> >> - Correct comments for lp->hs and hs->lp count calculations
> >> - General updating comments to have more clarity
> >> - using max() instead of ternary operator
> >> - Fix names (ui_num, ui_den) while using UI in calculations
> >> - compute max of lp_to_hs switch and hs_to_lp switch while computing
> >> hs_lp_switch_count
> >>
> >>Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> >
> >Hopefully still works after all that :)
>
> It does, at least on AsusT100 :) Thanks for the review
Aside: checkpatch.pl complained a bit about style issues in your patch
(like unpretty alignment of continuation lines and stuff like that).
Please feed your patches to checkpatch if you don't have an editor that
simply gets this right.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [v2] drm/i915: Add support for Generic MIPI panel driver
2014-05-27 11:39 ` Daniel Vetter
@ 2014-05-27 11:42 ` Kumar, Shobhit
2014-05-27 11:51 ` Daniel Vetter
0 siblings, 1 reply; 30+ messages in thread
From: Kumar, Shobhit @ 2014-05-27 11:42 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Jani Nikula, Daniel Vetter, intel-gfx
On 5/27/2014 5:09 PM, Daniel Vetter wrote:
> On Tue, May 27, 2014 at 04:51:55PM +0530, Kumar, Shobhit wrote:
>> On 5/27/2014 4:32 PM, Damien Lespiau wrote:
>>> On Fri, May 23, 2014 at 09:35:27PM +0530, Shobhit Kumar wrote:
>>>> This driver makes use of the generic panel information from the VBT.
>>>> Panel information is classified into two - panel configuration and panel
>>>> power sequence which is unique to each panel. The generic driver uses the
>>>> panel configuration and sequence parsed from VBT block #52 and #53
>>>>
>>>> v2: Address review comments by Jani
>>>> - Move all of the things in driver c file from header
>>>> - Make all functions static
>>>> - Make use of video/mipi_display.c instead of redefining
>>>> - Null checks during sequence execution
>>>>
>>>> v3: Address review comments by Damien
>>>> - Rename the panel driver file as intel_dsi_panel_vbt.c
>>>> - Fix style changes as suggested
>>>> - Correct comments for lp->hs and hs->lp count calculations
>>>> - General updating comments to have more clarity
>>>> - using max() instead of ternary operator
>>>> - Fix names (ui_num, ui_den) while using UI in calculations
>>>> - compute max of lp_to_hs switch and hs_to_lp switch while computing
>>>> hs_lp_switch_count
>>>>
>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>>
>>> Hopefully still works after all that :)
>>
>> It does, at least on AsusT100 :) Thanks for the review
>
> Aside: checkpatch.pl complained a bit about style issues in your patch
> (like unpretty alignment of continuation lines and stuff like that).
> Please feed your patches to checkpatch if you don't have an editor that
> simply gets this right.
Hmm, I will check my vim config and be more careful next time. Thanks
for pointing out. Have you taken care of these or I will push another
patch to fix the alignment issues ?
Regards
Shobhit
> -Daniel
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [v2] drm/i915: Add support for Generic MIPI panel driver
2014-05-27 11:42 ` Kumar, Shobhit
@ 2014-05-27 11:51 ` Daniel Vetter
2014-05-27 12:26 ` Damien Lespiau
2014-05-27 13:53 ` [PATCH] drm/i915: Fix checkpatch errors Shobhit Kumar
0 siblings, 2 replies; 30+ messages in thread
From: Daniel Vetter @ 2014-05-27 11:51 UTC (permalink / raw)
To: Kumar, Shobhit; +Cc: Jani Nikula, Daniel Vetter, intel-gfx
On Tue, May 27, 2014 at 1:42 PM, Kumar, Shobhit <shobhit.kumar@intel.com> wrote:
> On 5/27/2014 5:09 PM, Daniel Vetter wrote:
>>
>> On Tue, May 27, 2014 at 04:51:55PM +0530, Kumar, Shobhit wrote:
>>>
>>> On 5/27/2014 4:32 PM, Damien Lespiau wrote:
>>>>
>>>> On Fri, May 23, 2014 at 09:35:27PM +0530, Shobhit Kumar wrote:
>>>>>
>>>>> This driver makes use of the generic panel information from the VBT.
>>>>> Panel information is classified into two - panel configuration and
>>>>> panel
>>>>> power sequence which is unique to each panel. The generic driver uses
>>>>> the
>>>>> panel configuration and sequence parsed from VBT block #52 and #53
>>>>>
>>>>> v2: Address review comments by Jani
>>>>> - Move all of the things in driver c file from header
>>>>> - Make all functions static
>>>>> - Make use of video/mipi_display.c instead of redefining
>>>>> - Null checks during sequence execution
>>>>>
>>>>> v3: Address review comments by Damien
>>>>> - Rename the panel driver file as intel_dsi_panel_vbt.c
>>>>> - Fix style changes as suggested
>>>>> - Correct comments for lp->hs and hs->lp count calculations
>>>>> - General updating comments to have more clarity
>>>>> - using max() instead of ternary operator
>>>>> - Fix names (ui_num, ui_den) while using UI in calculations
>>>>> - compute max of lp_to_hs switch and hs_to_lp switch while
>>>>> computing
>>>>> hs_lp_switch_count
>>>>>
>>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>>>
>>>>
>>>> Hopefully still works after all that :)
>>>
>>>
>>> It does, at least on AsusT100 :) Thanks for the review
>>
>>
>> Aside: checkpatch.pl complained a bit about style issues in your patch
>> (like unpretty alignment of continuation lines and stuff like that).
>> Please feed your patches to checkpatch if you don't have an editor that
>> simply gets this right.
>
>
> Hmm, I will check my vim config and be more careful next time. Thanks for
> pointing out. Have you taken care of these or I will push another patch to
> fix the alignment issues ?
Yeah, a quick fixup patch would be pretty. For the vim configuration I
highly recommend to use
set cinoptions=:0,(0 " no indent for case and align function arguments to (
Then you can realign function parameter lists or multi-line if
condiditions with the = command, e.g. =a(
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [v2] drm/i915: Add support for Generic MIPI panel driver
2014-05-27 11:51 ` Daniel Vetter
@ 2014-05-27 12:26 ` Damien Lespiau
2014-05-27 13:53 ` [PATCH] drm/i915: Fix checkpatch errors Shobhit Kumar
1 sibling, 0 replies; 30+ messages in thread
From: Damien Lespiau @ 2014-05-27 12:26 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Jani Nikula, intel-gfx, Daniel Vetter
On Tue, May 27, 2014 at 01:51:22PM +0200, Daniel Vetter wrote:
> > Hmm, I will check my vim config and be more careful next time. Thanks for
> > pointing out. Have you taken care of these or I will push another patch to
> > fix the alignment issues ?
>
> Yeah, a quick fixup patch would be pretty. For the vim configuration I
> highly recommend to use
>
> set cinoptions=:0,(0 " no indent for case and align function arguments to (
>
> Then you can realign function parameter lists or multi-line if
> condiditions with the = command, e.g. =a(
FWIW, this is what I use:
set cinoptions=:0,t0,(0,u0,w1,m1
--
Damien
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH] drm/i915: Fix checkpatch errors
2014-05-27 11:51 ` Daniel Vetter
2014-05-27 12:26 ` Damien Lespiau
@ 2014-05-27 13:53 ` Shobhit Kumar
2014-05-27 14:19 ` Damien Lespiau
1 sibling, 1 reply; 30+ messages in thread
From: Shobhit Kumar @ 2014-05-27 13:53 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter
Fix warnings introduced by the following commit -
commit 9c92da2c7c17eea79b6321b37592df0a002d24df
Author: Shobhit Kumar <shobhit.kumar@intel.com>
Date: Fri May 23 21:35:27 2014 +0530
drm/i915: Add support for Generic MIPI panel driver
Fixed all except the DRM logging which go beyond line 80
Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 21a0d34..47c7584 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -143,7 +143,7 @@ static u8 *mipi_exec_send_packet(struct intel_dsi *intel_dsi, u8 *data)
case MIPI_DSI_DCS_LONG_WRITE:
dsi_vc_dcs_write(intel_dsi, vc, data, len);
break;
- };
+ }
data += len;
@@ -294,7 +294,8 @@ static bool generic_init(struct intel_dsi_device *dsi)
intel_dsi->rst_timer_val = mipi_config->device_reset_timer;
intel_dsi->init_count = mipi_config->master_init_timer;
intel_dsi->bw_timer = mipi_config->dbi_bw_timer;
- intel_dsi->video_frmt_cfg_bits = mipi_config->bta_enabled ? DISABLE_VIDEO_BTA : 0;
+ intel_dsi->video_frmt_cfg_bits =
+ mipi_config->bta_enabled ? DISABLE_VIDEO_BTA : 0;
switch (intel_dsi->escape_clk_div) {
case 0:
@@ -351,7 +352,8 @@ static bool generic_init(struct intel_dsi_device *dsi)
*
* prepare count
*/
- ths_prepare_ns = max(mipi_config->ths_prepare, mipi_config->tclk_prepare);
+ ths_prepare_ns = max(mipi_config->ths_prepare,
+ mipi_config->tclk_prepare);
prepare_cnt = DIV_ROUND_UP(ths_prepare_ns * ui_den, ui_num * 2);
/* exit zero count */
--
1.8.3.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH] drm/i915: Fix checkpatch errors
2014-05-27 13:53 ` [PATCH] drm/i915: Fix checkpatch errors Shobhit Kumar
@ 2014-05-27 14:19 ` Damien Lespiau
2014-05-27 14:34 ` Kumar, Shobhit
0 siblings, 1 reply; 30+ messages in thread
From: Damien Lespiau @ 2014-05-27 14:19 UTC (permalink / raw)
To: Shobhit Kumar; +Cc: Daniel Vetter, intel-gfx
On Tue, May 27, 2014 at 07:23:46PM +0530, Shobhit Kumar wrote:
> Fix warnings introduced by the following commit -
>
> commit 9c92da2c7c17eea79b6321b37592df0a002d24df
> Author: Shobhit Kumar <shobhit.kumar@intel.com>
> Date: Fri May 23 21:35:27 2014 +0530
>
> drm/i915: Add support for Generic MIPI panel driver
>
> Fixed all except the DRM logging which go beyond line 80
>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
To squash into previous commit?
Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
--
Damien
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] drm/i915: Fix checkpatch errors
2014-05-27 14:19 ` Damien Lespiau
@ 2014-05-27 14:34 ` Kumar, Shobhit
2014-05-27 14:39 ` Damien Lespiau
0 siblings, 1 reply; 30+ messages in thread
From: Kumar, Shobhit @ 2014-05-27 14:34 UTC (permalink / raw)
To: Damien Lespiau; +Cc: Daniel Vetter, intel-gfx
On 5/27/2014 7:49 PM, Damien Lespiau wrote:
> On Tue, May 27, 2014 at 07:23:46PM +0530, Shobhit Kumar wrote:
>> Fix warnings introduced by the following commit -
>>
>> commit 9c92da2c7c17eea79b6321b37592df0a002d24df
>> Author: Shobhit Kumar <shobhit.kumar@intel.com>
>> Date: Fri May 23 21:35:27 2014 +0530
>>
>> drm/i915: Add support for Generic MIPI panel driver
>>
>> Fixed all except the DRM logging which go beyond line 80
>>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>
> To squash into previous commit?
Can be done. I am not sure how this is handled for queued patches
Regards
Shobhit
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] drm/i915: Fix checkpatch errors
2014-05-27 14:34 ` Kumar, Shobhit
@ 2014-05-27 14:39 ` Damien Lespiau
2014-05-27 17:03 ` Daniel Vetter
0 siblings, 1 reply; 30+ messages in thread
From: Damien Lespiau @ 2014-05-27 14:39 UTC (permalink / raw)
To: Kumar, Shobhit; +Cc: Daniel Vetter, intel-gfx
On Tue, May 27, 2014 at 08:04:43PM +0530, Kumar, Shobhit wrote:
> On 5/27/2014 7:49 PM, Damien Lespiau wrote:
> >On Tue, May 27, 2014 at 07:23:46PM +0530, Shobhit Kumar wrote:
> >>Fix warnings introduced by the following commit -
> >>
> >>commit 9c92da2c7c17eea79b6321b37592df0a002d24df
> >>Author: Shobhit Kumar <shobhit.kumar@intel.com>
> >>Date: Fri May 23 21:35:27 2014 +0530
> >>
> >> drm/i915: Add support for Generic MIPI panel driver
> >>
> >>Fixed all except the DRM logging which go beyond line 80
> >>
> >>Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> >
> >To squash into previous commit?
>
> Can be done. I am not sure how this is handled for queued patches
Daniel does a fair bit of this kind of editing when applying patches.
--
Damien
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] drm/i915: Fix checkpatch errors
2014-05-27 14:39 ` Damien Lespiau
@ 2014-05-27 17:03 ` Daniel Vetter
0 siblings, 0 replies; 30+ messages in thread
From: Daniel Vetter @ 2014-05-27 17:03 UTC (permalink / raw)
To: Damien Lespiau; +Cc: intel-gfx, Daniel Vetter
On Tue, May 27, 2014 at 03:39:37PM +0100, Damien Lespiau wrote:
> On Tue, May 27, 2014 at 08:04:43PM +0530, Kumar, Shobhit wrote:
> > On 5/27/2014 7:49 PM, Damien Lespiau wrote:
> > >On Tue, May 27, 2014 at 07:23:46PM +0530, Shobhit Kumar wrote:
> > >>Fix warnings introduced by the following commit -
> > >>
> > >>commit 9c92da2c7c17eea79b6321b37592df0a002d24df
> > >>Author: Shobhit Kumar <shobhit.kumar@intel.com>
> > >>Date: Fri May 23 21:35:27 2014 +0530
> > >>
> > >> drm/i915: Add support for Generic MIPI panel driver
> > >>
> > >>Fixed all except the DRM logging which go beyond line 80
> > >>
> > >>Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> > >
> > >To squash into previous commit?
> >
> > Can be done. I am not sure how this is handled for queued patches
>
> Daniel does a fair bit of this kind of editing when applying patches.
I think I'll keep this one separate, as a cautionary tale ;-)
Queued for -next, thanks for the patch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 30+ messages in thread