diff mbox

[01/26] drm/rockchip: dw-mipi-dsi: use mode from display state

Message ID 20160919171747.28512-2-john@metanate.com (mailing list archive)
State New, archived
Headers show

Commit Message

John Keeping Sept. 19, 2016, 5:17 p.m. UTC 
There is no need to keep a pointer to the mode around since we know it
will be present in the connector state.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

Comments

Chris Zhong Jan. 17, 2017, 10:38 a.m. UTC | #1 
On 09/20/2016 01:17 AM, John Keeping wrote:
> There is no need to keep a pointer to the mode around since we know it
> will be present in the connector state.
>
> Signed-off-by: John Keeping <john@metanate.com>
> ---
>   drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 31 ++++++++++++++++---------------
>   1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> index ca22e5ee89ca..a87037556f5c 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> @@ -286,7 +286,6 @@ struct dw_mipi_dsi {
>   	u32 format;
>   	u16 input_div;
>   	u16 feedback_div;
> -	struct drm_display_mode *mode;
>   
>   	const struct dw_mipi_dsi_plat_data *pdata;
>   };
> @@ -332,9 +331,10 @@ static int max_mbps_to_testdin(unsigned int max_mbps)
>    */
>   static void dw_mipi_dsi_wait_for_two_frames(struct dw_mipi_dsi *dsi)
>   {
> +	struct drm_display_mode *mode = &dsi->connector.state->crtc->state->adjusted_mode;
>   	int refresh, two_frames;
>   
> -	refresh = drm_mode_vrefresh(dsi->mode);
> +	refresh = drm_mode_vrefresh(mode);
>   	two_frames = DIV_ROUND_UP(MSEC_PER_SEC, refresh) * 2;
>   	msleep(two_frames);
>   }
> @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
>   
>   static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi)
>   {
> +	struct drm_display_mode *mode = &dsi->connector.state->crtc->state->adjusted_mode;
>   	unsigned int i, pre;
>   	unsigned long mpclk, pllref, tmp;
>   	unsigned int m = 1, n = 1, target_mbps = 1000;
> @@ -474,7 +475,7 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi)
>   		return bpp;
>   	}
>   
> -	mpclk = DIV_ROUND_UP(dsi->mode->clock, MSEC_PER_SEC);
> +	mpclk = DIV_ROUND_UP(mode->clock, MSEC_PER_SEC);
>   	if (mpclk) {
>   		/* take 1 / 0.9, since mbps must big than bandwidth of RGB */
>   		tmp = mpclk * (bpp / dsi->lanes) * 10 / 9;
> @@ -689,9 +690,9 @@ static void dw_mipi_dsi_init(struct dw_mipi_dsi *dsi)
>   	dsi_write(dsi, DSI_LPCLK_CTRL, PHY_TXREQUESTCLKHS);
>   }
>   
> -static void dw_mipi_dsi_dpi_config(struct dw_mipi_dsi *dsi,
> -				   struct drm_display_mode *mode)
> +static void dw_mipi_dsi_dpi_config(struct dw_mipi_dsi *dsi)
>   {
> +	struct drm_display_mode *mode = &dsi->connector.state->crtc->state->adjusted_mode;
>   	u32 val = 0, color = 0;
>   
>   	switch (dsi->format) {
> @@ -726,9 +727,10 @@ static void dw_mipi_dsi_packet_handler_config(struct dw_mipi_dsi *dsi)
>   	dsi_write(dsi, DSI_PCKHDL_CFG, EN_CRC_RX | EN_ECC_RX | EN_BTA);
>   }
>   
> -static void dw_mipi_dsi_video_packet_config(struct dw_mipi_dsi *dsi,
> -					    struct drm_display_mode *mode)
> +static void dw_mipi_dsi_video_packet_config(struct dw_mipi_dsi *dsi)
>   {
> +	struct drm_display_mode *mode = &dsi->connector.state->crtc->state->adjusted_mode;
> +
>   	dsi_write(dsi, DSI_VID_PKT_SIZE, VID_PKT_SIZE(mode->hdisplay));
>   }
>   
> @@ -744,12 +746,13 @@ static void dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi)
>   static u32 dw_mipi_dsi_get_hcomponent_lbcc(struct dw_mipi_dsi *dsi,
>   					   u32 hcomponent)
>   {
> +	struct drm_display_mode *mode = &dsi->connector.state->crtc->state->adjusted_mode;
>   	u32 frac, lbcc;
>   
>   	lbcc = hcomponent * dsi->lane_mbps * MSEC_PER_SEC / 8;
>   
> -	frac = lbcc % dsi->mode->clock;
> -	lbcc = lbcc / dsi->mode->clock;
> +	frac = lbcc % mode->clock;
> +	lbcc = lbcc / mode->clock;
>   	if (frac)
>   		lbcc++;
>   
> @@ -759,7 +762,7 @@ static u32 dw_mipi_dsi_get_hcomponent_lbcc(struct dw_mipi_dsi *dsi,
>   static void dw_mipi_dsi_line_timer_config(struct dw_mipi_dsi *dsi)
>   {
>   	u32 htotal, hsa, hbp, lbcc;
> -	struct drm_display_mode *mode = dsi->mode;
> +	struct drm_display_mode *mode = &dsi->connector.state->crtc->state->adjusted_mode;
>   
>   	htotal = mode->htotal;
>   	hsa = mode->hsync_end - mode->hsync_start;
> @@ -778,7 +781,7 @@ static void dw_mipi_dsi_line_timer_config(struct dw_mipi_dsi *dsi)
>   static void dw_mipi_dsi_vertical_timing_config(struct dw_mipi_dsi *dsi)
>   {
>   	u32 vactive, vsa, vfp, vbp;
> -	struct drm_display_mode *mode = dsi->mode;
> +	struct drm_display_mode *mode = &dsi->connector.state->crtc->state->adjusted_mode;
>   
>   	vactive = mode->vdisplay;
>   	vsa = mode->vsync_end - mode->vsync_start;
> @@ -821,8 +824,6 @@ static void dw_mipi_dsi_encoder_mode_set(struct drm_encoder *encoder,
>   	struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);
>   	int ret;
>   
> -	dsi->mode = adjusted_mode;
> -
I prefer to keep the original method, although this"dsi->mode" pointer 
is same as "&dsi->connector.state->crtc->state->adjusted_mode; "
I still think dsi->mode makes the process easier to read


>   	ret = dw_mipi_dsi_get_lane_bps(dsi);
>   	if (ret < 0)
>   		return;
> @@ -833,10 +834,10 @@ static void dw_mipi_dsi_encoder_mode_set(struct drm_encoder *encoder,
>   	}
>   
>   	dw_mipi_dsi_init(dsi);
> -	dw_mipi_dsi_dpi_config(dsi, mode);
> +	dw_mipi_dsi_dpi_config(dsi);
>   	dw_mipi_dsi_packet_handler_config(dsi);
>   	dw_mipi_dsi_video_mode_config(dsi);
> -	dw_mipi_dsi_video_packet_config(dsi, mode);
> +	dw_mipi_dsi_video_packet_config(dsi);
>   	dw_mipi_dsi_command_mode_config(dsi);
>   	dw_mipi_dsi_line_timer_config(dsi);
>   	dw_mipi_dsi_vertical_timing_config(dsi);
yao mark Jan. 18, 2017, 2:21 a.m. UTC | #2 
On 2017年01月17日 18:38, Chris Zhong wrote:
>> @@ -821,8 +824,6 @@ static void dw_mipi_dsi_encoder_mode_set(struct 
>> drm_encoder *encoder,
>>       struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);
>>       int ret;
>>   -    dsi->mode = adjusted_mode;
>> -
> I prefer to keep the original method, although this"dsi->mode" pointer 
> is same as "&dsi->connector.state->crtc->state->adjusted_mode; "
> I still think dsi->mode makes the process easier to read
"&dsi->connector.state->crtc->state->adjusted_mode;" is too long, and 
dsi->connector.state->crtc can be NULL, seems not good.

agree with Chris, I think dsi->mode is better.

Bug the origin code "dsi->mode = adjusted_mode; " also has a bug, 
adjusted_mode's lift time is unknown, use a pointer from adjusted_mode 
is dangerous.

I think we can use like that, copy a display mode to dsi->mode:
struct dw_mipi_dsi
{
    -    struct drm_display_mode *mode;
    +    struct drm_display_mode mode;
}
   xxx_encoder_mode_set()
{
    drm_mode_copy(&dsi->mode, adjusted_mode);
}
diff mbox

Patch

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
index ca22e5ee89ca..a87037556f5c 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
@@ -286,7 +286,6 @@  struct dw_mipi_dsi {
 	u32 format;
 	u16 input_div;
 	u16 feedback_div;
-	struct drm_display_mode *mode;
 
 	const struct dw_mipi_dsi_plat_data *pdata;
 };
@@ -332,9 +331,10 @@  static int max_mbps_to_testdin(unsigned int max_mbps)
  */
 static void dw_mipi_dsi_wait_for_two_frames(struct dw_mipi_dsi *dsi)
 {
+	struct drm_display_mode *mode = &dsi->connector.state->crtc->state->adjusted_mode;
 	int refresh, two_frames;
 
-	refresh = drm_mode_vrefresh(dsi->mode);
+	refresh = drm_mode_vrefresh(mode);
 	two_frames = DIV_ROUND_UP(MSEC_PER_SEC, refresh) * 2;
 	msleep(two_frames);
 }
@@ -461,6 +461,7 @@  static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
 
 static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi)
 {
+	struct drm_display_mode *mode = &dsi->connector.state->crtc->state->adjusted_mode;
 	unsigned int i, pre;
 	unsigned long mpclk, pllref, tmp;
 	unsigned int m = 1, n = 1, target_mbps = 1000;
@@ -474,7 +475,7 @@  static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi)
 		return bpp;
 	}
 
-	mpclk = DIV_ROUND_UP(dsi->mode->clock, MSEC_PER_SEC);
+	mpclk = DIV_ROUND_UP(mode->clock, MSEC_PER_SEC);
 	if (mpclk) {
 		/* take 1 / 0.9, since mbps must big than bandwidth of RGB */
 		tmp = mpclk * (bpp / dsi->lanes) * 10 / 9;
@@ -689,9 +690,9 @@  static void dw_mipi_dsi_init(struct dw_mipi_dsi *dsi)
 	dsi_write(dsi, DSI_LPCLK_CTRL, PHY_TXREQUESTCLKHS);
 }
 
-static void dw_mipi_dsi_dpi_config(struct dw_mipi_dsi *dsi,
-				   struct drm_display_mode *mode)
+static void dw_mipi_dsi_dpi_config(struct dw_mipi_dsi *dsi)
 {
+	struct drm_display_mode *mode = &dsi->connector.state->crtc->state->adjusted_mode;
 	u32 val = 0, color = 0;
 
 	switch (dsi->format) {
@@ -726,9 +727,10 @@  static void dw_mipi_dsi_packet_handler_config(struct dw_mipi_dsi *dsi)
 	dsi_write(dsi, DSI_PCKHDL_CFG, EN_CRC_RX | EN_ECC_RX | EN_BTA);
 }
 
-static void dw_mipi_dsi_video_packet_config(struct dw_mipi_dsi *dsi,
-					    struct drm_display_mode *mode)
+static void dw_mipi_dsi_video_packet_config(struct dw_mipi_dsi *dsi)
 {
+	struct drm_display_mode *mode = &dsi->connector.state->crtc->state->adjusted_mode;
+
 	dsi_write(dsi, DSI_VID_PKT_SIZE, VID_PKT_SIZE(mode->hdisplay));
 }
 
@@ -744,12 +746,13 @@  static void dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi)
 static u32 dw_mipi_dsi_get_hcomponent_lbcc(struct dw_mipi_dsi *dsi,
 					   u32 hcomponent)
 {
+	struct drm_display_mode *mode = &dsi->connector.state->crtc->state->adjusted_mode;
 	u32 frac, lbcc;
 
 	lbcc = hcomponent * dsi->lane_mbps * MSEC_PER_SEC / 8;
 
-	frac = lbcc % dsi->mode->clock;
-	lbcc = lbcc / dsi->mode->clock;
+	frac = lbcc % mode->clock;
+	lbcc = lbcc / mode->clock;
 	if (frac)
 		lbcc++;
 
@@ -759,7 +762,7 @@  static u32 dw_mipi_dsi_get_hcomponent_lbcc(struct dw_mipi_dsi *dsi,
 static void dw_mipi_dsi_line_timer_config(struct dw_mipi_dsi *dsi)
 {
 	u32 htotal, hsa, hbp, lbcc;
-	struct drm_display_mode *mode = dsi->mode;
+	struct drm_display_mode *mode = &dsi->connector.state->crtc->state->adjusted_mode;
 
 	htotal = mode->htotal;
 	hsa = mode->hsync_end - mode->hsync_start;
@@ -778,7 +781,7 @@  static void dw_mipi_dsi_line_timer_config(struct dw_mipi_dsi *dsi)
 static void dw_mipi_dsi_vertical_timing_config(struct dw_mipi_dsi *dsi)
 {
 	u32 vactive, vsa, vfp, vbp;
-	struct drm_display_mode *mode = dsi->mode;
+	struct drm_display_mode *mode = &dsi->connector.state->crtc->state->adjusted_mode;
 
 	vactive = mode->vdisplay;
 	vsa = mode->vsync_end - mode->vsync_start;
@@ -821,8 +824,6 @@  static void dw_mipi_dsi_encoder_mode_set(struct drm_encoder *encoder,
 	struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);
 	int ret;
 
-	dsi->mode = adjusted_mode;
-
 	ret = dw_mipi_dsi_get_lane_bps(dsi);
 	if (ret < 0)
 		return;
@@ -833,10 +834,10 @@  static void dw_mipi_dsi_encoder_mode_set(struct drm_encoder *encoder,
 	}
 
 	dw_mipi_dsi_init(dsi);
-	dw_mipi_dsi_dpi_config(dsi, mode);
+	dw_mipi_dsi_dpi_config(dsi);
 	dw_mipi_dsi_packet_handler_config(dsi);
 	dw_mipi_dsi_video_mode_config(dsi);
-	dw_mipi_dsi_video_packet_config(dsi, mode);
+	dw_mipi_dsi_video_packet_config(dsi);
 	dw_mipi_dsi_command_mode_config(dsi);
 	dw_mipi_dsi_line_timer_config(dsi);
 	dw_mipi_dsi_vertical_timing_config(dsi);