Re: [v3 6/7] drm/mediatek: change the dsi phytiming calculate method

On Sun, May 19, 2019 at 05:25:36PM +0800, Jitao Shi wrote:

Change the method of frame rate calc which can get more accurate frame rate.

data rate = pixel_clock * bit_per_pixel / lanes Adjust hfp_wc to adapt the additional phy_data

if MIPI_DSI_MODE_VIDEO_BURST hfp_wc = hfp * bpp - data_phy_cycles * lanes - 12 - 6; else hfp_wc = hfp * bpp - data_phy_cycles * lanes - 12;

Note: //(2: 1 for sync, 1 for phy idle) data_phy_cycles = T_hs_exit + T_lpx + T_hs_prepare + T_hs_zero + 2;

bpp: bit per pixel

Signed-off-by: Jitao Shi jitao.shi@mediatek.com Tested-by: Ryan Case ryandcase@chromium.org

---

drivers/gpu/drm/mediatek/mtk_dsi.c | 119 +++++++++++++++++++++-------- 1 file changed, 86 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index 1165ff944889..3f51b2000c68 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -158,6 +158,25 @@ (type == MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM) || \ (type == MIPI_DSI_DCS_READ))

+struct mtk_phy_timing {

• u32 lpx;
• u32 da_hs_prepare;
• u32 da_hs_zero;
• u32 da_hs_trail;
• u32 ta_go;
• u32 ta_sure;
• u32 ta_get;
• u32 da_hs_exit;
• u32 clk_hs_zero;
• u32 clk_hs_trail;
• u32 clk_hs_prepare;
• u32 clk_hs_post;
• u32 clk_hs_exit;

+};

struct phy;

struct mtk_dsi_driver_data { @@ -182,12 +201,13 @@ struct mtk_dsi { struct clk *digital_clk; struct clk *hs_clk;

• u32 data_rate;

• u64 data_rate;

This results in 64-bit divide operations and thus build failures with 32-bit builds. More on that below.

unsigned long mode_flags; enum mipi_dsi_pixel_format format; unsigned int lanes; struct videomode vm;

• struct mtk_phy_timing phy_timing; int refcount; bool enabled; u32 irq_data;

@@ -221,17 +241,39 @@ static void mtk_dsi_phy_timconfig(struct mtk_dsi *dsi) { u32 timcon0, timcon1, timcon2, timcon3; u32 ui, cycle_time;

• struct mtk_phy_timing *timing = &dsi->phy_timing;
• ui = 1000000000 / dsi->data_rate;
• cycle_time = 8000000000 / dsi->data_rate;

This results in 64-bit divide operations. On top of that, 8000000000 is larger than 0xffffffff, resulting in an integer overflow on 32-bit systems; it should be provided as 8000000000ULL.

• timing->lpx = NS_TO_CYCLE(60, cycle_time);
• timing->da_hs_prepare = NS_TO_CYCLE((40 + 5 * ui), cycle_time);
• timing->da_hs_zero = NS_TO_CYCLE((110 + 6 * ui), cycle_time);
• timing->da_hs_trail = NS_TO_CYCLE(((0x4 * ui) + 80), cycle_time);
• if (timing->da_hs_zero > timing->da_hs_prepare)

• timing->da_hs_zero -= timing->da_hs_prepare;
  

• timing->ta_go = 4 * timing->lpx;
• timing->ta_sure = 3 * timing->lpx / 2;
• timing->ta_get = 5 * timing->lpx;
• timing->da_hs_exit = 2 * timing->lpx;
• timing->clk_hs_zero = NS_TO_CYCLE(0x150, cycle_time);
• timing->clk_hs_trail = NS_TO_CYCLE(0x64, cycle_time) + 0xa;

• ui = 1000 / dsi->data_rate + 0x01;
• cycle_time = 8000 / dsi->data_rate + 0x01;

• timing->clk_hs_prepare = NS_TO_CYCLE(0x40, cycle_time);
• timing->clk_hs_post = NS_TO_CYCLE(80 + 52 * ui, cycle_time);
• timing->clk_hs_exit = 2 * timing->lpx;

• timcon0 = T_LPX | T_HS_PREP << 8 | T_HS_ZERO << 16 | T_HS_TRAIL << 24;
• timcon1 = 4 * T_LPX | (3 * T_LPX / 2) << 8 | 5 * T_LPX << 16 |

•   T_HS_EXIT << 24;
  

• timcon2 = ((NS_TO_CYCLE(0x64, cycle_time) + 0xa) << 24) |

•   (NS_TO_CYCLE(0x150, cycle_time) << 16);
  

• timcon3 = NS_TO_CYCLE(0x40, cycle_time) | (2 * T_LPX) << 16 |

•   NS_TO_CYCLE(80 + 52 * ui, cycle_time) << 8;
  

• timcon0 = timing->lpx | timing->da_hs_prepare << 8 |

•   timing->da_hs_zero << 16 | timing->da_hs_trail << 24;
  

• timcon1 = timing->ta_go | timing->ta_sure << 8 |

•   timing->ta_get << 16 | timing->da_hs_exit << 24;
  

• timcon2 = 1 << 8 | timing->clk_hs_zero << 16 |

•   timing->clk_hs_trail << 24;
  

• timcon3 = timing->clk_hs_prepare | timing->clk_hs_post << 8 |

•   timing->clk_hs_exit << 16;
  
  

  writel(timcon0, dsi->regs + DSI_PHY_TIMECON0); writel(timcon1, dsi->regs + DSI_PHY_TIMECON1);

@@ -418,7 +460,8 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) u32 horizontal_sync_active_byte; u32 horizontal_backporch_byte; u32 horizontal_frontporch_byte;

• u32 dsi_tmp_buf_bpp;

• u32 dsi_tmp_buf_bpp, data_phy_cycles;

• struct mtk_phy_timing *timing = &dsi->phy_timing;

  struct videomode *vm = &dsi->vm;

@@ -433,7 +476,8 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) writel(vm->vactive, dsi->regs + DSI_VACT_NL);

if (dsi->driver_data->has_size_ctl)

• writel(vm->vactive << 16 | vm->hactive, dsi->regs + DSI_SIZE_CON);
  

• writel(vm->vactive << 16 | vm->hactive,
  

•        dsi->regs + DSI_SIZE_CON);
  
  

  horizontal_sync_active_byte = (vm->hsync_len * dsi_tmp_buf_bpp - 10);

@@ -444,7 +488,34 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) horizontal_backporch_byte = ((vm->hback_porch + vm->hsync_len) * dsi_tmp_buf_bpp - 10);

• horizontal_frontporch_byte = (vm->hfront_porch * dsi_tmp_buf_bpp - 12);

• data_phy_cycles = timing->lpx + timing->da_hs_prepare +

• 		  timing->da_hs_zero + timing->da_hs_exit + 2;
  

• if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {

• if (vm->hfront_porch * dsi_tmp_buf_bpp >
  

•     data_phy_cycles * dsi->lanes + 18) {
  

• 	horizontal_frontporch_byte = vm->hfront_porch *
  

• 				     dsi_tmp_buf_bpp -
  

• 				     data_phy_cycles *
  

• 				     dsi->lanes - 18;
  

• } else {
  

• 	DRM_WARN("HFP less than d-phy, FPS will under 60Hz\n");
  

• 	horizontal_frontporch_byte = vm->hfront_porch *
  

• 				     dsi_tmp_buf_bpp;
  

• }
  

• } else {

• if (vm->hfront_porch * dsi_tmp_buf_bpp >
  

•     data_phy_cycles * dsi->lanes + 12) {
  

• 	horizontal_frontporch_byte = vm->hfront_porch *
  

• 				     dsi_tmp_buf_bpp -
  

• 				     data_phy_cycles *
  

• 				     dsi->lanes - 12;
  

• } else {
  

• 	DRM_WARN("HFP less than d-phy, FPS will under 60Hz\n");
  

• 	horizontal_frontporch_byte = vm->hfront_porch *
  

• 				     dsi_tmp_buf_bpp;
  

• }
  

• }

  writel(horizontal_sync_active_byte, dsi->regs + DSI_HSA_WC); writel(horizontal_backporch_byte, dsi->regs + DSI_HBP_WC);

@@ -544,8 +615,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi) { struct device *dev = dsi->dev; int ret;

• u64 pixel_clock, total_bits;
• u32 htotal, htotal_bits, bit_per_pixel, overhead_cycles, overhead_bits;

• u32 bit_per_pixel;

  if (++dsi->refcount != 1) return 0;

@@ -564,24 +634,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi) break; }

• /**

• * htotal_time = htotal * byte_per_pixel / num_lanes
  

• * overhead_time = lpx + hs_prepare + hs_zero + hs_trail + hs_exit
  

• * mipi_ratio = (htotal_time + overhead_time) / htotal_time
  

• * data_rate = pixel_clock * bit_per_pixel * mipi_ratio / num_lanes;
  

• */
  

• pixel_clock = dsi->vm.pixelclock;
• htotal = dsi->vm.hactive + dsi->vm.hback_porch + dsi->vm.hfront_porch +

• 	dsi->vm.hsync_len;
  

• htotal_bits = htotal * bit_per_pixel;
• overhead_cycles = T_LPX + T_HS_PREP + T_HS_ZERO + T_HS_TRAIL +

• 	T_HS_EXIT;
  

• overhead_bits = overhead_cycles * dsi->lanes * 8;
• total_bits = htotal_bits + overhead_bits;
• dsi->data_rate = DIV_ROUND_UP_ULL(pixel_clock * total_bits,

• 			  htotal * dsi->lanes);
  

• dsi->data_rate = dsi->vm.pixelclock * bit_per_pixel / dsi->lanes;

pixelclock and bit_per_pixel are not u64, and neither is dsi->lanes. The above will thus be a 32-bit operation on 32-bit systems, and never really assign a true 64-bit value to data_rate. On top of that, clk_set_rate() expects an unsigned long argument. Declaring data_rate as anything but unsigned long has therefore no value.

Note that the old code took possible overflow conditions due to interim results larger than u32 / unsigned long into account. This is no longer the case. I don't know if this is a concern, but it might be worth watching out for. If it is not a concern because the code is not anymore expected to run on 32-bit systems, it should be restricted to 64-bit builds.

Guenter

ret = clk_set_rate(dsi->hs_clk, dsi->data_rate); if (ret < 0) {