[PATCH] drm/rockchip: get rid of rockchip_drm_crtc_mode_config

[PATCH] drm/rockchip: get rid of rockchip_drm_crtc_mode_config

[Mark Yao]

We need to take care of the vop status when use
rockchip_drm_crtc_mode_config, if vop is disabled,
the function would failed, that is terrible.

Save connector type and output mode on drm_display_mode->private_flags
at encoder mode_fixup, then we can configure the type and mode safely
on crtc mode_set.

Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
---
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c |   37 +++++++---------
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c          |   44 ++++++++++---------
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c     |    5 +--
 drivers/gpu/drm/rockchip/inno_hdmi.c            |    5 +--
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h     |    2 -
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c     |   51 +++++++++--------------
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h     |    6 +++
 7 files changed, 69 insertions(+), 81 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index a1d94d8..0344e52 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -97,7 +97,21 @@ rockchip_dp_drm_encoder_mode_fixup(struct drm_encoder *encoder,
 				   const struct drm_display_mode *mode,
 				   struct drm_display_mode *adjusted_mode)
 {
-	/* do nothing */
+	/*
+	 * FIXME(Yakir): driver should configure the CRTC output video
+	 * mode with the display information which indicated the monitor
+	 * support colorimetry.
+	 *
+	 * But don't know why the CRTC driver seems could only output the
+	 * RGBaaa rightly. For example, if connect the "innolux,n116bge"
+	 * eDP screen, EDID would indicated that screen only accepted the
+	 * 6bpc mode. But if I configure CRTC to RGB666 output, then eDP
+	 * screen would show a blue picture (RGB888 show a green picture).
+	 * But if I configure CTRC to RGBaaa, and eDP driver still keep
+	 * RGB666 input video mode, then screen would works prefect.
+	 */
+	adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(eDP, AAAA);
+
 	return true;
 }
 
@@ -114,27 +128,6 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
 	int ret;
 	u32 val;
 
-	/*
-	 * FIXME(Yakir): driver should configure the CRTC output video
-	 * mode with the display information which indicated the monitor
-	 * support colorimetry.
-	 *
-	 * But don't know why the CRTC driver seems could only output the
-	 * RGBaaa rightly. For example, if connect the "innolux,n116bge"
-	 * eDP screen, EDID would indicated that screen only accepted the
-	 * 6bpc mode. But if I configure CRTC to RGB666 output, then eDP
-	 * screen would show a blue picture (RGB888 show a green picture).
-	 * But if I configure CTRC to RGBaaa, and eDP driver still keep
-	 * RGB666 input video mode, then screen would works prefect.
-	 */
-	ret = rockchip_drm_crtc_mode_config(encoder->crtc,
-					    DRM_MODE_CONNECTOR_eDP,
-					    ROCKCHIP_OUT_MODE_AAAA);
-	if (ret < 0) {
-		dev_err(dp->dev, "Could not set crtc mode config (%d)\n", ret);
-		return;
-	}
-
 	ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
 	if (ret < 0)
 		return;
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
index 7975158..49c4c72 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
@@ -875,11 +875,34 @@ static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder)
 	clk_disable_unprepare(dsi->pclk);
 }
 
+static bool dw_mipi_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
+					const struct drm_display_mode *mode,
+					struct drm_display_mode *adjusted_mode)
+{
+	struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);
+
+	switch (dsi->format) {
+	case MIPI_DSI_FMT_RGB888:
+		adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P888);
+		break;
+	case MIPI_DSI_FMT_RGB666:
+		adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P666);
+		break;
+	case MIPI_DSI_FMT_RGB565:
+		adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P565);
+		break;
+	default:
+		WARN_ON(1);
+		return false;
+	}
+
+	return true;
+}
+
 static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder)
 {
 	struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);
 	int mux = drm_of_encoder_active_endpoint_id(dsi->dev->of_node, encoder);
-	u32 interface_pix_fmt;
 	u32 val;
 
 	if (clk_prepare_enable(dsi->pclk)) {
@@ -895,24 +918,6 @@ static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder)
 
 	clk_disable_unprepare(dsi->pclk);
 
-	switch (dsi->format) {
-	case MIPI_DSI_FMT_RGB888:
-		interface_pix_fmt = ROCKCHIP_OUT_MODE_P888;
-		break;
-	case MIPI_DSI_FMT_RGB666:
-		interface_pix_fmt = ROCKCHIP_OUT_MODE_P666;
-		break;
-	case MIPI_DSI_FMT_RGB565:
-		interface_pix_fmt = ROCKCHIP_OUT_MODE_P565;
-		break;
-	default:
-		WARN_ON(1);
-		return;
-	}
-
-	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_DSI,
-				      interface_pix_fmt);
-
 	if (mux)
 		val = DSI0_SEL_VOP_LIT | (DSI0_SEL_VOP_LIT << 16);
 	else
@@ -924,6 +929,7 @@ static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder)
 
 static struct drm_encoder_helper_funcs
 dw_mipi_dsi_encoder_helper_funcs = {
+	.mode_fixup = dw_mipi_dsi_encoder_mode_fixup,
 	.commit = dw_mipi_dsi_encoder_commit,
 	.mode_set = dw_mipi_dsi_encoder_mode_set,
 	.disable = dw_mipi_dsi_encoder_disable,
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index d5cfef7..a72af30 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -186,6 +186,8 @@ dw_hdmi_rockchip_encoder_mode_fixup(struct drm_encoder *encoder,
 				    const struct drm_display_mode *mode,
 				    struct drm_display_mode *adj_mode)
 {
+	adj_mode->private_flags = ROCKCHIP_DSP_MODE(HDMIA, AAAA);
+
 	return true;
 }
 
@@ -201,9 +203,6 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
 	u32 val;
 	int mux;
 
-	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_HDMIA,
-				      ROCKCHIP_OUT_MODE_AAAA);
-
 	mux = drm_of_encoder_active_endpoint_id(hdmi->dev->of_node, encoder);
 	if (mux)
 		val = HDMI_SEL_VOP_LIT | (HDMI_SEL_VOP_LIT << 16);
diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index 10d62ff..ef6ad91 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -500,9 +500,6 @@ static void inno_hdmi_encoder_enable(struct drm_encoder *encoder)
 {
 	struct inno_hdmi *hdmi = to_inno_hdmi(encoder);
 
-	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_HDMIA,
-				      ROCKCHIP_OUT_MODE_P888);
-
 	inno_hdmi_set_pwr_mode(hdmi, NORMAL);
 }
 
@@ -517,6 +514,8 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
 					 const struct drm_display_mode *mode,
 					 struct drm_display_mode *adj_mode)
 {
+	adj_mode->private_flags = ROCKCHIP_DSP_MODE(HDMIA, P888);
+
 	return true;
 }
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index 00d17d7..683210b 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -68,8 +68,6 @@ void rockchip_drm_atomic_work(struct work_struct *work);
 int rockchip_register_crtc_funcs(struct drm_crtc *crtc,
 				 const struct rockchip_crtc_funcs *crtc_funcs);
 void rockchip_unregister_crtc_funcs(struct drm_crtc *crtc);
-int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc, int connector_type,
-				  int out_mode);
 int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
 				   struct device *dev);
 void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index a619f12..1a611cf 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -818,38 +818,6 @@ static const struct drm_plane_funcs vop_plane_funcs = {
 	.atomic_destroy_state = vop_atomic_plane_destroy_state,
 };
 
-int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc,
-				  int connector_type,
-				  int out_mode)
-{
-	struct vop *vop = to_vop(crtc);
-
-	if (WARN_ON(!vop->is_enabled))
-		return -EINVAL;
-
-	switch (connector_type) {
-	case DRM_MODE_CONNECTOR_LVDS:
-		VOP_CTRL_SET(vop, rgb_en, 1);
-		break;
-	case DRM_MODE_CONNECTOR_eDP:
-		VOP_CTRL_SET(vop, edp_en, 1);
-		break;
-	case DRM_MODE_CONNECTOR_HDMIA:
-		VOP_CTRL_SET(vop, hdmi_en, 1);
-		break;
-	case DRM_MODE_CONNECTOR_DSI:
-		VOP_CTRL_SET(vop, mipi_en, 1);
-		break;
-	default:
-		DRM_ERROR("unsupport connector_type[%d]\n", connector_type);
-		return -EINVAL;
-	};
-	VOP_CTRL_SET(vop, out_mode, out_mode);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(rockchip_drm_crtc_mode_config);
-
 static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
 {
 	struct vop *vop = to_vop(crtc);
@@ -943,6 +911,8 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
 	u16 vact_st = adjusted_mode->vtotal - adjusted_mode->vsync_start;
 	u16 vact_end = vact_st + vdisplay;
 	uint32_t val;
+	int type = ROCKCHIP_OUT_MODE_TYPE(adjusted_mode->private_flags);
+	int out_mode = ROCKCHIP_OUT_MODE(adjusted_mode->private_flags);
 
 	vop_enable(crtc);
 	/*
@@ -985,6 +955,23 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
 	val |= (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC) ? 0 : 1;
 	val |= (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC) ? 0 : (1 << 1);
 	VOP_CTRL_SET(vop, pin_pol, val);
+	switch (type) {
+	case DRM_MODE_CONNECTOR_LVDS:
+		VOP_CTRL_SET(vop, rgb_en, 1);
+		break;
+	case DRM_MODE_CONNECTOR_eDP:
+		VOP_CTRL_SET(vop, edp_en, 1);
+		break;
+	case DRM_MODE_CONNECTOR_HDMIA:
+		VOP_CTRL_SET(vop, hdmi_en, 1);
+		break;
+	case DRM_MODE_CONNECTOR_DSI:
+		VOP_CTRL_SET(vop, mipi_en, 1);
+		break;
+	default:
+		DRM_ERROR("unsupport connector_type[%d]\n", type);
+	}
+	VOP_CTRL_SET(vop, out_mode, out_mode);
 
 	VOP_CTRL_SET(vop, htotal_pw, (htotal << 16) | hsync_len);
 	val = hact_st << 16;
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
index 071ff0b..4c63a66 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
@@ -183,6 +183,12 @@ struct vop_data {
 /* for use special outface */
 #define ROCKCHIP_OUT_MODE_AAAA	15
 
+#define ROCKCHIP_OUT_MODE_TYPE(x)	((x) >> 16)
+#define ROCKCHIP_OUT_MODE(x)		((x) & 0xffff)
+#define ROCKCHIP_DSP_MODE(type, mode) \
+		((DRM_MODE_CONNECTOR_##type << 16) | \
+		 (ROCKCHIP_OUT_MODE_##mode & 0xffff))
+
 enum alpha_mode {
 	ALPHA_STRAIGHT,
 	ALPHA_INVERSE,
-- 
1.7.9.5

[PATCH] drm/rockchip: get rid of rockchip_drm_crtc_mode_config

[Mark Yao]

We need to take care of the vop status when use
rockchip_drm_crtc_mode_config, if vop is disabled,
the function would failed, that is terrible.

Save connector type and output mode on drm_display_mode->private_flags
at encoder mode_fixup, then we can configure the type and mode safely
on crtc mode_set.

Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
---
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c |   37 +++++++---------
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c          |   44 ++++++++++---------
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c     |    5 +--
 drivers/gpu/drm/rockchip/inno_hdmi.c            |    5 +--
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h     |    2 -
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c     |   51 +++++++++--------------
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h     |    6 +++
 7 files changed, 69 insertions(+), 81 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index a1d94d8..0344e52 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -97,7 +97,21 @@ rockchip_dp_drm_encoder_mode_fixup(struct drm_encoder *encoder,
 				   const struct drm_display_mode *mode,
 				   struct drm_display_mode *adjusted_mode)
 {
-	/* do nothing */
+	/*
+	 * FIXME(Yakir): driver should configure the CRTC output video
+	 * mode with the display information which indicated the monitor
+	 * support colorimetry.
+	 *
+	 * But don't know why the CRTC driver seems could only output the
+	 * RGBaaa rightly. For example, if connect the "innolux,n116bge"
+	 * eDP screen, EDID would indicated that screen only accepted the
+	 * 6bpc mode. But if I configure CRTC to RGB666 output, then eDP
+	 * screen would show a blue picture (RGB888 show a green picture).
+	 * But if I configure CTRC to RGBaaa, and eDP driver still keep
+	 * RGB666 input video mode, then screen would works prefect.
+	 */
+	adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(eDP, AAAA);
+
 	return true;
 }
 
@@ -114,27 +128,6 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
 	int ret;
 	u32 val;
 
-	/*
-	 * FIXME(Yakir): driver should configure the CRTC output video
-	 * mode with the display information which indicated the monitor
-	 * support colorimetry.
-	 *
-	 * But don't know why the CRTC driver seems could only output the
-	 * RGBaaa rightly. For example, if connect the "innolux,n116bge"
-	 * eDP screen, EDID would indicated that screen only accepted the
-	 * 6bpc mode. But if I configure CRTC to RGB666 output, then eDP
-	 * screen would show a blue picture (RGB888 show a green picture).
-	 * But if I configure CTRC to RGBaaa, and eDP driver still keep
-	 * RGB666 input video mode, then screen would works prefect.
-	 */
-	ret = rockchip_drm_crtc_mode_config(encoder->crtc,
-					    DRM_MODE_CONNECTOR_eDP,
-					    ROCKCHIP_OUT_MODE_AAAA);
-	if (ret < 0) {
-		dev_err(dp->dev, "Could not set crtc mode config (%d)\n", ret);
-		return;
-	}
-
 	ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
 	if (ret < 0)
 		return;
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
index 7975158..49c4c72 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
@@ -875,11 +875,34 @@ static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder)
 	clk_disable_unprepare(dsi->pclk);
 }
 
+static bool dw_mipi_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
+					const struct drm_display_mode *mode,
+					struct drm_display_mode *adjusted_mode)
+{
+	struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);
+
+	switch (dsi->format) {
+	case MIPI_DSI_FMT_RGB888:
+		adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P888);
+		break;
+	case MIPI_DSI_FMT_RGB666:
+		adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P666);
+		break;
+	case MIPI_DSI_FMT_RGB565:
+		adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P565);
+		break;
+	default:
+		WARN_ON(1);
+		return false;
+	}
+
+	return true;
+}
+
 static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder)
 {
 	struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);
 	int mux = drm_of_encoder_active_endpoint_id(dsi->dev->of_node, encoder);
-	u32 interface_pix_fmt;
 	u32 val;
 
 	if (clk_prepare_enable(dsi->pclk)) {
@@ -895,24 +918,6 @@ static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder)
 
 	clk_disable_unprepare(dsi->pclk);
 
-	switch (dsi->format) {
-	case MIPI_DSI_FMT_RGB888:
-		interface_pix_fmt = ROCKCHIP_OUT_MODE_P888;
-		break;
-	case MIPI_DSI_FMT_RGB666:
-		interface_pix_fmt = ROCKCHIP_OUT_MODE_P666;
-		break;
-	case MIPI_DSI_FMT_RGB565:
-		interface_pix_fmt = ROCKCHIP_OUT_MODE_P565;
-		break;
-	default:
-		WARN_ON(1);
-		return;
-	}
-
-	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_DSI,
-				      interface_pix_fmt);
-
 	if (mux)
 		val = DSI0_SEL_VOP_LIT | (DSI0_SEL_VOP_LIT << 16);
 	else
@@ -924,6 +929,7 @@ static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder)
 
 static struct drm_encoder_helper_funcs
 dw_mipi_dsi_encoder_helper_funcs = {
+	.mode_fixup = dw_mipi_dsi_encoder_mode_fixup,
 	.commit = dw_mipi_dsi_encoder_commit,
 	.mode_set = dw_mipi_dsi_encoder_mode_set,
 	.disable = dw_mipi_dsi_encoder_disable,
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index d5cfef7..a72af30 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -186,6 +186,8 @@ dw_hdmi_rockchip_encoder_mode_fixup(struct drm_encoder *encoder,
 				    const struct drm_display_mode *mode,
 				    struct drm_display_mode *adj_mode)
 {
+	adj_mode->private_flags = ROCKCHIP_DSP_MODE(HDMIA, AAAA);
+
 	return true;
 }
 
@@ -201,9 +203,6 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
 	u32 val;
 	int mux;
 
-	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_HDMIA,
-				      ROCKCHIP_OUT_MODE_AAAA);
-
 	mux = drm_of_encoder_active_endpoint_id(hdmi->dev->of_node, encoder);
 	if (mux)
 		val = HDMI_SEL_VOP_LIT | (HDMI_SEL_VOP_LIT << 16);
diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index 10d62ff..ef6ad91 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -500,9 +500,6 @@ static void inno_hdmi_encoder_enable(struct drm_encoder *encoder)
 {
 	struct inno_hdmi *hdmi = to_inno_hdmi(encoder);
 
-	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_HDMIA,
-				      ROCKCHIP_OUT_MODE_P888);
-
 	inno_hdmi_set_pwr_mode(hdmi, NORMAL);
 }
 
@@ -517,6 +514,8 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
 					 const struct drm_display_mode *mode,
 					 struct drm_display_mode *adj_mode)
 {
+	adj_mode->private_flags = ROCKCHIP_DSP_MODE(HDMIA, P888);
+
 	return true;
 }
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index 00d17d7..683210b 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -68,8 +68,6 @@ void rockchip_drm_atomic_work(struct work_struct *work);
 int rockchip_register_crtc_funcs(struct drm_crtc *crtc,
 				 const struct rockchip_crtc_funcs *crtc_funcs);
 void rockchip_unregister_crtc_funcs(struct drm_crtc *crtc);
-int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc, int connector_type,
-				  int out_mode);
 int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
 				   struct device *dev);
 void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index a619f12..1a611cf 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -818,38 +818,6 @@ static const struct drm_plane_funcs vop_plane_funcs = {
 	.atomic_destroy_state = vop_atomic_plane_destroy_state,
 };
 
-int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc,
-				  int connector_type,
-				  int out_mode)
-{
-	struct vop *vop = to_vop(crtc);
-
-	if (WARN_ON(!vop->is_enabled))
-		return -EINVAL;
-
-	switch (connector_type) {
-	case DRM_MODE_CONNECTOR_LVDS:
-		VOP_CTRL_SET(vop, rgb_en, 1);
-		break;
-	case DRM_MODE_CONNECTOR_eDP:
-		VOP_CTRL_SET(vop, edp_en, 1);
-		break;
-	case DRM_MODE_CONNECTOR_HDMIA:
-		VOP_CTRL_SET(vop, hdmi_en, 1);
-		break;
-	case DRM_MODE_CONNECTOR_DSI:
-		VOP_CTRL_SET(vop, mipi_en, 1);
-		break;
-	default:
-		DRM_ERROR("unsupport connector_type[%d]\n", connector_type);
-		return -EINVAL;
-	};
-	VOP_CTRL_SET(vop, out_mode, out_mode);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(rockchip_drm_crtc_mode_config);
-
 static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
 {
 	struct vop *vop = to_vop(crtc);
@@ -943,6 +911,8 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
 	u16 vact_st = adjusted_mode->vtotal - adjusted_mode->vsync_start;
 	u16 vact_end = vact_st + vdisplay;
 	uint32_t val;
+	int type = ROCKCHIP_OUT_MODE_TYPE(adjusted_mode->private_flags);
+	int out_mode = ROCKCHIP_OUT_MODE(adjusted_mode->private_flags);
 
 	vop_enable(crtc);
 	/*
@@ -985,6 +955,23 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
 	val |= (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC) ? 0 : 1;
 	val |= (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC) ? 0 : (1 << 1);
 	VOP_CTRL_SET(vop, pin_pol, val);
+	switch (type) {
+	case DRM_MODE_CONNECTOR_LVDS:
+		VOP_CTRL_SET(vop, rgb_en, 1);
+		break;
+	case DRM_MODE_CONNECTOR_eDP:
+		VOP_CTRL_SET(vop, edp_en, 1);
+		break;
+	case DRM_MODE_CONNECTOR_HDMIA:
+		VOP_CTRL_SET(vop, hdmi_en, 1);
+		break;
+	case DRM_MODE_CONNECTOR_DSI:
+		VOP_CTRL_SET(vop, mipi_en, 1);
+		break;
+	default:
+		DRM_ERROR("unsupport connector_type[%d]\n", type);
+	}
+	VOP_CTRL_SET(vop, out_mode, out_mode);
 
 	VOP_CTRL_SET(vop, htotal_pw, (htotal << 16) | hsync_len);
 	val = hact_st << 16;
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
index 071ff0b..4c63a66 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
@@ -183,6 +183,12 @@ struct vop_data {
 /* for use special outface */
 #define ROCKCHIP_OUT_MODE_AAAA	15
 
+#define ROCKCHIP_OUT_MODE_TYPE(x)	((x) >> 16)
+#define ROCKCHIP_OUT_MODE(x)		((x) & 0xffff)
+#define ROCKCHIP_DSP_MODE(type, mode) \
+		((DRM_MODE_CONNECTOR_##type << 16) | \
+		 (ROCKCHIP_OUT_MODE_##mode & 0xffff))
+
 enum alpha_mode {
 	ALPHA_STRAIGHT,
 	ALPHA_INVERSE,
-- 
1.7.9.5


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/rockchip: get rid of rockchip_drm_crtc_mode_config

[Mark Yao]

We need to take care of the vop status when use
rockchip_drm_crtc_mode_config, if vop is disabled,
the function would failed, that is terrible.

Save connector type and output mode on drm_display_mode->private_flags
at encoder mode_fixup, then we can configure the type and mode safely
on crtc mode_set.

Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
---
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c |   37 +++++++---------
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c          |   44 ++++++++++---------
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c     |    5 +--
 drivers/gpu/drm/rockchip/inno_hdmi.c            |    5 +--
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h     |    2 -
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c     |   51 +++++++++--------------
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h     |    6 +++
 7 files changed, 69 insertions(+), 81 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index a1d94d8..0344e52 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -97,7 +97,21 @@ rockchip_dp_drm_encoder_mode_fixup(struct drm_encoder *encoder,
 				   const struct drm_display_mode *mode,
 				   struct drm_display_mode *adjusted_mode)
 {
-	/* do nothing */
+	/*
+	 * FIXME(Yakir): driver should configure the CRTC output video
+	 * mode with the display information which indicated the monitor
+	 * support colorimetry.
+	 *
+	 * But don't know why the CRTC driver seems could only output the
+	 * RGBaaa rightly. For example, if connect the "innolux,n116bge"
+	 * eDP screen, EDID would indicated that screen only accepted the
+	 * 6bpc mode. But if I configure CRTC to RGB666 output, then eDP
+	 * screen would show a blue picture (RGB888 show a green picture).
+	 * But if I configure CTRC to RGBaaa, and eDP driver still keep
+	 * RGB666 input video mode, then screen would works prefect.
+	 */
+	adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(eDP, AAAA);
+
 	return true;
 }
 
@@ -114,27 +128,6 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
 	int ret;
 	u32 val;
 
-	/*
-	 * FIXME(Yakir): driver should configure the CRTC output video
-	 * mode with the display information which indicated the monitor
-	 * support colorimetry.
-	 *
-	 * But don't know why the CRTC driver seems could only output the
-	 * RGBaaa rightly. For example, if connect the "innolux,n116bge"
-	 * eDP screen, EDID would indicated that screen only accepted the
-	 * 6bpc mode. But if I configure CRTC to RGB666 output, then eDP
-	 * screen would show a blue picture (RGB888 show a green picture).
-	 * But if I configure CTRC to RGBaaa, and eDP driver still keep
-	 * RGB666 input video mode, then screen would works prefect.
-	 */
-	ret = rockchip_drm_crtc_mode_config(encoder->crtc,
-					    DRM_MODE_CONNECTOR_eDP,
-					    ROCKCHIP_OUT_MODE_AAAA);
-	if (ret < 0) {
-		dev_err(dp->dev, "Could not set crtc mode config (%d)\n", ret);
-		return;
-	}
-
 	ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
 	if (ret < 0)
 		return;
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
index 7975158..49c4c72 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
@@ -875,11 +875,34 @@ static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder)
 	clk_disable_unprepare(dsi->pclk);
 }
 
+static bool dw_mipi_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
+					const struct drm_display_mode *mode,
+					struct drm_display_mode *adjusted_mode)
+{
+	struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);
+
+	switch (dsi->format) {
+	case MIPI_DSI_FMT_RGB888:
+		adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P888);
+		break;
+	case MIPI_DSI_FMT_RGB666:
+		adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P666);
+		break;
+	case MIPI_DSI_FMT_RGB565:
+		adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P565);
+		break;
+	default:
+		WARN_ON(1);
+		return false;
+	}
+
+	return true;
+}
+
 static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder)
 {
 	struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);
 	int mux = drm_of_encoder_active_endpoint_id(dsi->dev->of_node, encoder);
-	u32 interface_pix_fmt;
 	u32 val;
 
 	if (clk_prepare_enable(dsi->pclk)) {
@@ -895,24 +918,6 @@ static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder)
 
 	clk_disable_unprepare(dsi->pclk);
 
-	switch (dsi->format) {
-	case MIPI_DSI_FMT_RGB888:
-		interface_pix_fmt = ROCKCHIP_OUT_MODE_P888;
-		break;
-	case MIPI_DSI_FMT_RGB666:
-		interface_pix_fmt = ROCKCHIP_OUT_MODE_P666;
-		break;
-	case MIPI_DSI_FMT_RGB565:
-		interface_pix_fmt = ROCKCHIP_OUT_MODE_P565;
-		break;
-	default:
-		WARN_ON(1);
-		return;
-	}
-
-	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_DSI,
-				      interface_pix_fmt);
-
 	if (mux)
 		val = DSI0_SEL_VOP_LIT | (DSI0_SEL_VOP_LIT << 16);
 	else
@@ -924,6 +929,7 @@ static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder)
 
 static struct drm_encoder_helper_funcs
 dw_mipi_dsi_encoder_helper_funcs = {
+	.mode_fixup = dw_mipi_dsi_encoder_mode_fixup,
 	.commit = dw_mipi_dsi_encoder_commit,
 	.mode_set = dw_mipi_dsi_encoder_mode_set,
 	.disable = dw_mipi_dsi_encoder_disable,
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index d5cfef7..a72af30 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -186,6 +186,8 @@ dw_hdmi_rockchip_encoder_mode_fixup(struct drm_encoder *encoder,
 				    const struct drm_display_mode *mode,
 				    struct drm_display_mode *adj_mode)
 {
+	adj_mode->private_flags = ROCKCHIP_DSP_MODE(HDMIA, AAAA);
+
 	return true;
 }
 
@@ -201,9 +203,6 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
 	u32 val;
 	int mux;
 
-	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_HDMIA,
-				      ROCKCHIP_OUT_MODE_AAAA);
-
 	mux = drm_of_encoder_active_endpoint_id(hdmi->dev->of_node, encoder);
 	if (mux)
 		val = HDMI_SEL_VOP_LIT | (HDMI_SEL_VOP_LIT << 16);
diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index 10d62ff..ef6ad91 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -500,9 +500,6 @@ static void inno_hdmi_encoder_enable(struct drm_encoder *encoder)
 {
 	struct inno_hdmi *hdmi = to_inno_hdmi(encoder);
 
-	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_HDMIA,
-				      ROCKCHIP_OUT_MODE_P888);
-
 	inno_hdmi_set_pwr_mode(hdmi, NORMAL);
 }
 
@@ -517,6 +514,8 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
 					 const struct drm_display_mode *mode,
 					 struct drm_display_mode *adj_mode)
 {
+	adj_mode->private_flags = ROCKCHIP_DSP_MODE(HDMIA, P888);
+
 	return true;
 }
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index 00d17d7..683210b 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -68,8 +68,6 @@ void rockchip_drm_atomic_work(struct work_struct *work);
 int rockchip_register_crtc_funcs(struct drm_crtc *crtc,
 				 const struct rockchip_crtc_funcs *crtc_funcs);
 void rockchip_unregister_crtc_funcs(struct drm_crtc *crtc);
-int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc, int connector_type,
-				  int out_mode);
 int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
 				   struct device *dev);
 void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index a619f12..1a611cf 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -818,38 +818,6 @@ static const struct drm_plane_funcs vop_plane_funcs = {
 	.atomic_destroy_state = vop_atomic_plane_destroy_state,
 };
 
-int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc,
-				  int connector_type,
-				  int out_mode)
-{
-	struct vop *vop = to_vop(crtc);
-
-	if (WARN_ON(!vop->is_enabled))
-		return -EINVAL;
-
-	switch (connector_type) {
-	case DRM_MODE_CONNECTOR_LVDS:
-		VOP_CTRL_SET(vop, rgb_en, 1);
-		break;
-	case DRM_MODE_CONNECTOR_eDP:
-		VOP_CTRL_SET(vop, edp_en, 1);
-		break;
-	case DRM_MODE_CONNECTOR_HDMIA:
-		VOP_CTRL_SET(vop, hdmi_en, 1);
-		break;
-	case DRM_MODE_CONNECTOR_DSI:
-		VOP_CTRL_SET(vop, mipi_en, 1);
-		break;
-	default:
-		DRM_ERROR("unsupport connector_type[%d]\n", connector_type);
-		return -EINVAL;
-	};
-	VOP_CTRL_SET(vop, out_mode, out_mode);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(rockchip_drm_crtc_mode_config);
-
 static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
 {
 	struct vop *vop = to_vop(crtc);
@@ -943,6 +911,8 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
 	u16 vact_st = adjusted_mode->vtotal - adjusted_mode->vsync_start;
 	u16 vact_end = vact_st + vdisplay;
 	uint32_t val;
+	int type = ROCKCHIP_OUT_MODE_TYPE(adjusted_mode->private_flags);
+	int out_mode = ROCKCHIP_OUT_MODE(adjusted_mode->private_flags);
 
 	vop_enable(crtc);
 	/*
@@ -985,6 +955,23 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
 	val |= (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC) ? 0 : 1;
 	val |= (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC) ? 0 : (1 << 1);
 	VOP_CTRL_SET(vop, pin_pol, val);
+	switch (type) {
+	case DRM_MODE_CONNECTOR_LVDS:
+		VOP_CTRL_SET(vop, rgb_en, 1);
+		break;
+	case DRM_MODE_CONNECTOR_eDP:
+		VOP_CTRL_SET(vop, edp_en, 1);
+		break;
+	case DRM_MODE_CONNECTOR_HDMIA:
+		VOP_CTRL_SET(vop, hdmi_en, 1);
+		break;
+	case DRM_MODE_CONNECTOR_DSI:
+		VOP_CTRL_SET(vop, mipi_en, 1);
+		break;
+	default:
+		DRM_ERROR("unsupport connector_type[%d]\n", type);
+	}
+	VOP_CTRL_SET(vop, out_mode, out_mode);
 
 	VOP_CTRL_SET(vop, htotal_pw, (htotal << 16) | hsync_len);
 	val = hact_st << 16;
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
index 071ff0b..4c63a66 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
@@ -183,6 +183,12 @@ struct vop_data {
 /* for use special outface */
 #define ROCKCHIP_OUT_MODE_AAAA	15
 
+#define ROCKCHIP_OUT_MODE_TYPE(x)	((x) >> 16)
+#define ROCKCHIP_OUT_MODE(x)		((x) & 0xffff)
+#define ROCKCHIP_DSP_MODE(type, mode) \
+		((DRM_MODE_CONNECTOR_##type << 16) | \
+		 (ROCKCHIP_OUT_MODE_##mode & 0xffff))
+
 enum alpha_mode {
 	ALPHA_STRAIGHT,
 	ALPHA_INVERSE,
-- 
1.7.9.5

Re: [PATCH] drm/rockchip: get rid of rockchip_drm_crtc_mode_config

[John Keeping]

Hi Mark,

On Mon, 18 Apr 2016 11:03:31 +0800, Mark Yao wrote:

> We need to take care of the vop status when use
> rockchip_drm_crtc_mode_config, if vop is disabled,
> the function would failed, that is terrible.
> 
> Save connector type and output mode on drm_display_mode->private_flags
> at encoder mode_fixup, then we can configure the type and mode safely
> on crtc mode_set.

Since Rockchip is atomic, shouldn't this be using atomic_check hooks and
a subclassed crtc_state structure?

> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
> ---
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c |   37 +++++++---------
>  drivers/gpu/drm/rockchip/dw-mipi-dsi.c          |   44 ++++++++++---------
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c     |    5 +--
>  drivers/gpu/drm/rockchip/inno_hdmi.c            |    5 +--
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h     |    2 -
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c     |   51 +++++++++--------------
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.h     |    6 +++
>  7 files changed, 69 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index a1d94d8..0344e52 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -97,7 +97,21 @@ rockchip_dp_drm_encoder_mode_fixup(struct drm_encoder *encoder,
>  				   const struct drm_display_mode *mode,
>  				   struct drm_display_mode *adjusted_mode)
>  {
> -	/* do nothing */
> +	/*
> +	 * FIXME(Yakir): driver should configure the CRTC output video
> +	 * mode with the display information which indicated the monitor
> +	 * support colorimetry.
> +	 *
> +	 * But don't know why the CRTC driver seems could only output the
> +	 * RGBaaa rightly. For example, if connect the "innolux,n116bge"
> +	 * eDP screen, EDID would indicated that screen only accepted the
> +	 * 6bpc mode. But if I configure CRTC to RGB666 output, then eDP
> +	 * screen would show a blue picture (RGB888 show a green picture).
> +	 * But if I configure CTRC to RGBaaa, and eDP driver still keep
> +	 * RGB666 input video mode, then screen would works prefect.
> +	 */
> +	adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(eDP, AAAA);
> +
>  	return true;
>  }
>  
> @@ -114,27 +128,6 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
>  	int ret;
>  	u32 val;
>  
> -	/*
> -	 * FIXME(Yakir): driver should configure the CRTC output video
> -	 * mode with the display information which indicated the monitor
> -	 * support colorimetry.
> -	 *
> -	 * But don't know why the CRTC driver seems could only output the
> -	 * RGBaaa rightly. For example, if connect the "innolux,n116bge"
> -	 * eDP screen, EDID would indicated that screen only accepted the
> -	 * 6bpc mode. But if I configure CRTC to RGB666 output, then eDP
> -	 * screen would show a blue picture (RGB888 show a green picture).
> -	 * But if I configure CTRC to RGBaaa, and eDP driver still keep
> -	 * RGB666 input video mode, then screen would works prefect.
> -	 */
> -	ret = rockchip_drm_crtc_mode_config(encoder->crtc,
> -					    DRM_MODE_CONNECTOR_eDP,
> -					    ROCKCHIP_OUT_MODE_AAAA);
> -	if (ret < 0) {
> -		dev_err(dp->dev, "Could not set crtc mode config (%d)\n", ret);
> -		return;
> -	}
> -
>  	ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
>  	if (ret < 0)
>  		return;
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> index 7975158..49c4c72 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> @@ -875,11 +875,34 @@ static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder)
>  	clk_disable_unprepare(dsi->pclk);
>  }
>  
> +static bool dw_mipi_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
> +					const struct drm_display_mode *mode,
> +					struct drm_display_mode *adjusted_mode)
> +{
> +	struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);
> +
> +	switch (dsi->format) {
> +	case MIPI_DSI_FMT_RGB888:
> +		adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P888);
> +		break;
> +	case MIPI_DSI_FMT_RGB666:
> +		adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P666);
> +		break;
> +	case MIPI_DSI_FMT_RGB565:
> +		adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P565);
> +		break;
> +	default:
> +		WARN_ON(1);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder)
>  {
>  	struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);
>  	int mux = drm_of_encoder_active_endpoint_id(dsi->dev->of_node, encoder);
> -	u32 interface_pix_fmt;
>  	u32 val;
>  
>  	if (clk_prepare_enable(dsi->pclk)) {
> @@ -895,24 +918,6 @@ static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder)
>  
>  	clk_disable_unprepare(dsi->pclk);
>  
> -	switch (dsi->format) {
> -	case MIPI_DSI_FMT_RGB888:
> -		interface_pix_fmt = ROCKCHIP_OUT_MODE_P888;
> -		break;
> -	case MIPI_DSI_FMT_RGB666:
> -		interface_pix_fmt = ROCKCHIP_OUT_MODE_P666;
> -		break;
> -	case MIPI_DSI_FMT_RGB565:
> -		interface_pix_fmt = ROCKCHIP_OUT_MODE_P565;
> -		break;
> -	default:
> -		WARN_ON(1);
> -		return;
> -	}
> -
> -	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_DSI,
> -				      interface_pix_fmt);
> -
>  	if (mux)
>  		val = DSI0_SEL_VOP_LIT | (DSI0_SEL_VOP_LIT << 16);
>  	else
> @@ -924,6 +929,7 @@ static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder)
>  
>  static struct drm_encoder_helper_funcs
>  dw_mipi_dsi_encoder_helper_funcs = {
> +	.mode_fixup = dw_mipi_dsi_encoder_mode_fixup,
>  	.commit = dw_mipi_dsi_encoder_commit,
>  	.mode_set = dw_mipi_dsi_encoder_mode_set,
>  	.disable = dw_mipi_dsi_encoder_disable,
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index d5cfef7..a72af30 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -186,6 +186,8 @@ dw_hdmi_rockchip_encoder_mode_fixup(struct drm_encoder *encoder,
>  				    const struct drm_display_mode *mode,
>  				    struct drm_display_mode *adj_mode)
>  {
> +	adj_mode->private_flags = ROCKCHIP_DSP_MODE(HDMIA, AAAA);
> +
>  	return true;
>  }
>  
> @@ -201,9 +203,6 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
>  	u32 val;
>  	int mux;
>  
> -	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_HDMIA,
> -				      ROCKCHIP_OUT_MODE_AAAA);
> -
>  	mux = drm_of_encoder_active_endpoint_id(hdmi->dev->of_node, encoder);
>  	if (mux)
>  		val = HDMI_SEL_VOP_LIT | (HDMI_SEL_VOP_LIT << 16);
> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> index 10d62ff..ef6ad91 100644
> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> @@ -500,9 +500,6 @@ static void inno_hdmi_encoder_enable(struct drm_encoder *encoder)
>  {
>  	struct inno_hdmi *hdmi = to_inno_hdmi(encoder);
>  
> -	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_HDMIA,
> -				      ROCKCHIP_OUT_MODE_P888);
> -
>  	inno_hdmi_set_pwr_mode(hdmi, NORMAL);
>  }
>  
> @@ -517,6 +514,8 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
>  					 const struct drm_display_mode *mode,
>  					 struct drm_display_mode *adj_mode)
>  {
> +	adj_mode->private_flags = ROCKCHIP_DSP_MODE(HDMIA, P888);
> +
>  	return true;
>  }
>  
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> index 00d17d7..683210b 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> @@ -68,8 +68,6 @@ void rockchip_drm_atomic_work(struct work_struct *work);
>  int rockchip_register_crtc_funcs(struct drm_crtc *crtc,
>  				 const struct rockchip_crtc_funcs *crtc_funcs);
>  void rockchip_unregister_crtc_funcs(struct drm_crtc *crtc);
> -int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc, int connector_type,
> -				  int out_mode);
>  int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
>  				   struct device *dev);
>  void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index a619f12..1a611cf 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -818,38 +818,6 @@ static const struct drm_plane_funcs vop_plane_funcs = {
>  	.atomic_destroy_state = vop_atomic_plane_destroy_state,
>  };
>  
> -int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc,
> -				  int connector_type,
> -				  int out_mode)
> -{
> -	struct vop *vop = to_vop(crtc);
> -
> -	if (WARN_ON(!vop->is_enabled))
> -		return -EINVAL;
> -
> -	switch (connector_type) {
> -	case DRM_MODE_CONNECTOR_LVDS:
> -		VOP_CTRL_SET(vop, rgb_en, 1);
> -		break;
> -	case DRM_MODE_CONNECTOR_eDP:
> -		VOP_CTRL_SET(vop, edp_en, 1);
> -		break;
> -	case DRM_MODE_CONNECTOR_HDMIA:
> -		VOP_CTRL_SET(vop, hdmi_en, 1);
> -		break;
> -	case DRM_MODE_CONNECTOR_DSI:
> -		VOP_CTRL_SET(vop, mipi_en, 1);
> -		break;
> -	default:
> -		DRM_ERROR("unsupport connector_type[%d]\n", connector_type);
> -		return -EINVAL;
> -	};
> -	VOP_CTRL_SET(vop, out_mode, out_mode);
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(rockchip_drm_crtc_mode_config);
> -
>  static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
>  {
>  	struct vop *vop = to_vop(crtc);
> @@ -943,6 +911,8 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
>  	u16 vact_st = adjusted_mode->vtotal - adjusted_mode->vsync_start;
>  	u16 vact_end = vact_st + vdisplay;
>  	uint32_t val;
> +	int type = ROCKCHIP_OUT_MODE_TYPE(adjusted_mode->private_flags);
> +	int out_mode = ROCKCHIP_OUT_MODE(adjusted_mode->private_flags);
>  
>  	vop_enable(crtc);
>  	/*
> @@ -985,6 +955,23 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
>  	val |= (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC) ? 0 : 1;
>  	val |= (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC) ? 0 : (1 << 1);
>  	VOP_CTRL_SET(vop, pin_pol, val);
> +	switch (type) {
> +	case DRM_MODE_CONNECTOR_LVDS:
> +		VOP_CTRL_SET(vop, rgb_en, 1);
> +		break;
> +	case DRM_MODE_CONNECTOR_eDP:
> +		VOP_CTRL_SET(vop, edp_en, 1);
> +		break;
> +	case DRM_MODE_CONNECTOR_HDMIA:
> +		VOP_CTRL_SET(vop, hdmi_en, 1);
> +		break;
> +	case DRM_MODE_CONNECTOR_DSI:
> +		VOP_CTRL_SET(vop, mipi_en, 1);
> +		break;
> +	default:
> +		DRM_ERROR("unsupport connector_type[%d]\n", type);
> +	}
> +	VOP_CTRL_SET(vop, out_mode, out_mode);
>  
>  	VOP_CTRL_SET(vop, htotal_pw, (htotal << 16) | hsync_len);
>  	val = hact_st << 16;
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> index 071ff0b..4c63a66 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> @@ -183,6 +183,12 @@ struct vop_data {
>  /* for use special outface */
>  #define ROCKCHIP_OUT_MODE_AAAA	15
>  
> +#define ROCKCHIP_OUT_MODE_TYPE(x)	((x) >> 16)
> +#define ROCKCHIP_OUT_MODE(x)		((x) & 0xffff)
> +#define ROCKCHIP_DSP_MODE(type, mode) \
> +		((DRM_MODE_CONNECTOR_##type << 16) | \
> +		 (ROCKCHIP_OUT_MODE_##mode & 0xffff))
> +
>  enum alpha_mode {
>  	ALPHA_STRAIGHT,
>  	ALPHA_INVERSE,

Re: [PATCH] drm/rockchip: get rid of rockchip_drm_crtc_mode_config

[John Keeping]

Hi Mark,

On Mon, 18 Apr 2016 11:03:31 +0800, Mark Yao wrote:

> We need to take care of the vop status when use
> rockchip_drm_crtc_mode_config, if vop is disabled,
> the function would failed, that is terrible.
> 
> Save connector type and output mode on drm_display_mode->private_flags
> at encoder mode_fixup, then we can configure the type and mode safely
> on crtc mode_set.

Since Rockchip is atomic, shouldn't this be using atomic_check hooks and
a subclassed crtc_state structure?

> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
> ---
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c |   37 +++++++---------
>  drivers/gpu/drm/rockchip/dw-mipi-dsi.c          |   44 ++++++++++---------
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c     |    5 +--
>  drivers/gpu/drm/rockchip/inno_hdmi.c            |    5 +--
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h     |    2 -
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c     |   51 +++++++++--------------
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.h     |    6 +++
>  7 files changed, 69 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index a1d94d8..0344e52 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -97,7 +97,21 @@ rockchip_dp_drm_encoder_mode_fixup(struct drm_encoder *encoder,
>  				   const struct drm_display_mode *mode,
>  				   struct drm_display_mode *adjusted_mode)
>  {
> -	/* do nothing */
> +	/*
> +	 * FIXME(Yakir): driver should configure the CRTC output video
> +	 * mode with the display information which indicated the monitor
> +	 * support colorimetry.
> +	 *
> +	 * But don't know why the CRTC driver seems could only output the
> +	 * RGBaaa rightly. For example, if connect the "innolux,n116bge"
> +	 * eDP screen, EDID would indicated that screen only accepted the
> +	 * 6bpc mode. But if I configure CRTC to RGB666 output, then eDP
> +	 * screen would show a blue picture (RGB888 show a green picture).
> +	 * But if I configure CTRC to RGBaaa, and eDP driver still keep
> +	 * RGB666 input video mode, then screen would works prefect.
> +	 */
> +	adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(eDP, AAAA);
> +
>  	return true;
>  }
>  
> @@ -114,27 +128,6 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
>  	int ret;
>  	u32 val;
>  
> -	/*
> -	 * FIXME(Yakir): driver should configure the CRTC output video
> -	 * mode with the display information which indicated the monitor
> -	 * support colorimetry.
> -	 *
> -	 * But don't know why the CRTC driver seems could only output the
> -	 * RGBaaa rightly. For example, if connect the "innolux,n116bge"
> -	 * eDP screen, EDID would indicated that screen only accepted the
> -	 * 6bpc mode. But if I configure CRTC to RGB666 output, then eDP
> -	 * screen would show a blue picture (RGB888 show a green picture).
> -	 * But if I configure CTRC to RGBaaa, and eDP driver still keep
> -	 * RGB666 input video mode, then screen would works prefect.
> -	 */
> -	ret = rockchip_drm_crtc_mode_config(encoder->crtc,
> -					    DRM_MODE_CONNECTOR_eDP,
> -					    ROCKCHIP_OUT_MODE_AAAA);
> -	if (ret < 0) {
> -		dev_err(dp->dev, "Could not set crtc mode config (%d)\n", ret);
> -		return;
> -	}
> -
>  	ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
>  	if (ret < 0)
>  		return;
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> index 7975158..49c4c72 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> @@ -875,11 +875,34 @@ static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder)
>  	clk_disable_unprepare(dsi->pclk);
>  }
>  
> +static bool dw_mipi_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
> +					const struct drm_display_mode *mode,
> +					struct drm_display_mode *adjusted_mode)
> +{
> +	struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);
> +
> +	switch (dsi->format) {
> +	case MIPI_DSI_FMT_RGB888:
> +		adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P888);
> +		break;
> +	case MIPI_DSI_FMT_RGB666:
> +		adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P666);
> +		break;
> +	case MIPI_DSI_FMT_RGB565:
> +		adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P565);
> +		break;
> +	default:
> +		WARN_ON(1);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder)
>  {
>  	struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);
>  	int mux = drm_of_encoder_active_endpoint_id(dsi->dev->of_node, encoder);
> -	u32 interface_pix_fmt;
>  	u32 val;
>  
>  	if (clk_prepare_enable(dsi->pclk)) {
> @@ -895,24 +918,6 @@ static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder)
>  
>  	clk_disable_unprepare(dsi->pclk);
>  
> -	switch (dsi->format) {
> -	case MIPI_DSI_FMT_RGB888:
> -		interface_pix_fmt = ROCKCHIP_OUT_MODE_P888;
> -		break;
> -	case MIPI_DSI_FMT_RGB666:
> -		interface_pix_fmt = ROCKCHIP_OUT_MODE_P666;
> -		break;
> -	case MIPI_DSI_FMT_RGB565:
> -		interface_pix_fmt = ROCKCHIP_OUT_MODE_P565;
> -		break;
> -	default:
> -		WARN_ON(1);
> -		return;
> -	}
> -
> -	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_DSI,
> -				      interface_pix_fmt);
> -
>  	if (mux)
>  		val = DSI0_SEL_VOP_LIT | (DSI0_SEL_VOP_LIT << 16);
>  	else
> @@ -924,6 +929,7 @@ static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder)
>  
>  static struct drm_encoder_helper_funcs
>  dw_mipi_dsi_encoder_helper_funcs = {
> +	.mode_fixup = dw_mipi_dsi_encoder_mode_fixup,
>  	.commit = dw_mipi_dsi_encoder_commit,
>  	.mode_set = dw_mipi_dsi_encoder_mode_set,
>  	.disable = dw_mipi_dsi_encoder_disable,
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index d5cfef7..a72af30 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -186,6 +186,8 @@ dw_hdmi_rockchip_encoder_mode_fixup(struct drm_encoder *encoder,
>  				    const struct drm_display_mode *mode,
>  				    struct drm_display_mode *adj_mode)
>  {
> +	adj_mode->private_flags = ROCKCHIP_DSP_MODE(HDMIA, AAAA);
> +
>  	return true;
>  }
>  
> @@ -201,9 +203,6 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
>  	u32 val;
>  	int mux;
>  
> -	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_HDMIA,
> -				      ROCKCHIP_OUT_MODE_AAAA);
> -
>  	mux = drm_of_encoder_active_endpoint_id(hdmi->dev->of_node, encoder);
>  	if (mux)
>  		val = HDMI_SEL_VOP_LIT | (HDMI_SEL_VOP_LIT << 16);
> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> index 10d62ff..ef6ad91 100644
> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> @@ -500,9 +500,6 @@ static void inno_hdmi_encoder_enable(struct drm_encoder *encoder)
>  {
>  	struct inno_hdmi *hdmi = to_inno_hdmi(encoder);
>  
> -	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_HDMIA,
> -				      ROCKCHIP_OUT_MODE_P888);
> -
>  	inno_hdmi_set_pwr_mode(hdmi, NORMAL);
>  }
>  
> @@ -517,6 +514,8 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
>  					 const struct drm_display_mode *mode,
>  					 struct drm_display_mode *adj_mode)
>  {
> +	adj_mode->private_flags = ROCKCHIP_DSP_MODE(HDMIA, P888);
> +
>  	return true;
>  }
>  
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> index 00d17d7..683210b 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> @@ -68,8 +68,6 @@ void rockchip_drm_atomic_work(struct work_struct *work);
>  int rockchip_register_crtc_funcs(struct drm_crtc *crtc,
>  				 const struct rockchip_crtc_funcs *crtc_funcs);
>  void rockchip_unregister_crtc_funcs(struct drm_crtc *crtc);
> -int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc, int connector_type,
> -				  int out_mode);
>  int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
>  				   struct device *dev);
>  void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index a619f12..1a611cf 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -818,38 +818,6 @@ static const struct drm_plane_funcs vop_plane_funcs = {
>  	.atomic_destroy_state = vop_atomic_plane_destroy_state,
>  };
>  
> -int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc,
> -				  int connector_type,
> -				  int out_mode)
> -{
> -	struct vop *vop = to_vop(crtc);
> -
> -	if (WARN_ON(!vop->is_enabled))
> -		return -EINVAL;
> -
> -	switch (connector_type) {
> -	case DRM_MODE_CONNECTOR_LVDS:
> -		VOP_CTRL_SET(vop, rgb_en, 1);
> -		break;
> -	case DRM_MODE_CONNECTOR_eDP:
> -		VOP_CTRL_SET(vop, edp_en, 1);
> -		break;
> -	case DRM_MODE_CONNECTOR_HDMIA:
> -		VOP_CTRL_SET(vop, hdmi_en, 1);
> -		break;
> -	case DRM_MODE_CONNECTOR_DSI:
> -		VOP_CTRL_SET(vop, mipi_en, 1);
> -		break;
> -	default:
> -		DRM_ERROR("unsupport connector_type[%d]\n", connector_type);
> -		return -EINVAL;
> -	};
> -	VOP_CTRL_SET(vop, out_mode, out_mode);
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(rockchip_drm_crtc_mode_config);
> -
>  static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
>  {
>  	struct vop *vop = to_vop(crtc);
> @@ -943,6 +911,8 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
>  	u16 vact_st = adjusted_mode->vtotal - adjusted_mode->vsync_start;
>  	u16 vact_end = vact_st + vdisplay;
>  	uint32_t val;
> +	int type = ROCKCHIP_OUT_MODE_TYPE(adjusted_mode->private_flags);
> +	int out_mode = ROCKCHIP_OUT_MODE(adjusted_mode->private_flags);
>  
>  	vop_enable(crtc);
>  	/*
> @@ -985,6 +955,23 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
>  	val |= (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC) ? 0 : 1;
>  	val |= (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC) ? 0 : (1 << 1);
>  	VOP_CTRL_SET(vop, pin_pol, val);
> +	switch (type) {
> +	case DRM_MODE_CONNECTOR_LVDS:
> +		VOP_CTRL_SET(vop, rgb_en, 1);
> +		break;
> +	case DRM_MODE_CONNECTOR_eDP:
> +		VOP_CTRL_SET(vop, edp_en, 1);
> +		break;
> +	case DRM_MODE_CONNECTOR_HDMIA:
> +		VOP_CTRL_SET(vop, hdmi_en, 1);
> +		break;
> +	case DRM_MODE_CONNECTOR_DSI:
> +		VOP_CTRL_SET(vop, mipi_en, 1);
> +		break;
> +	default:
> +		DRM_ERROR("unsupport connector_type[%d]\n", type);
> +	}
> +	VOP_CTRL_SET(vop, out_mode, out_mode);
>  
>  	VOP_CTRL_SET(vop, htotal_pw, (htotal << 16) | hsync_len);
>  	val = hact_st << 16;
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> index 071ff0b..4c63a66 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> @@ -183,6 +183,12 @@ struct vop_data {
>  /* for use special outface */
>  #define ROCKCHIP_OUT_MODE_AAAA	15
>  
> +#define ROCKCHIP_OUT_MODE_TYPE(x)	((x) >> 16)
> +#define ROCKCHIP_OUT_MODE(x)		((x) & 0xffff)
> +#define ROCKCHIP_DSP_MODE(type, mode) \
> +		((DRM_MODE_CONNECTOR_##type << 16) | \
> +		 (ROCKCHIP_OUT_MODE_##mode & 0xffff))
> +
>  enum alpha_mode {
>  	ALPHA_STRAIGHT,
>  	ALPHA_INVERSE,
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/rockchip: get rid of rockchip_drm_crtc_mode_config

[John Keeping]

Hi Mark,

On Mon, 18 Apr 2016 11:03:31 +0800, Mark Yao wrote:

> We need to take care of the vop status when use
> rockchip_drm_crtc_mode_config, if vop is disabled,
> the function would failed, that is terrible.
> 
> Save connector type and output mode on drm_display_mode->private_flags
> at encoder mode_fixup, then we can configure the type and mode safely
> on crtc mode_set.

Since Rockchip is atomic, shouldn't this be using atomic_check hooks and
a subclassed crtc_state structure?

> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
> ---
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c |   37 +++++++---------
>  drivers/gpu/drm/rockchip/dw-mipi-dsi.c          |   44 ++++++++++---------
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c     |    5 +--
>  drivers/gpu/drm/rockchip/inno_hdmi.c            |    5 +--
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h     |    2 -
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c     |   51 +++++++++--------------
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.h     |    6 +++
>  7 files changed, 69 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index a1d94d8..0344e52 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -97,7 +97,21 @@ rockchip_dp_drm_encoder_mode_fixup(struct drm_encoder *encoder,
>  				   const struct drm_display_mode *mode,
>  				   struct drm_display_mode *adjusted_mode)
>  {
> -	/* do nothing */
> +	/*
> +	 * FIXME(Yakir): driver should configure the CRTC output video
> +	 * mode with the display information which indicated the monitor
> +	 * support colorimetry.
> +	 *
> +	 * But don't know why the CRTC driver seems could only output the
> +	 * RGBaaa rightly. For example, if connect the "innolux,n116bge"
> +	 * eDP screen, EDID would indicated that screen only accepted the
> +	 * 6bpc mode. But if I configure CRTC to RGB666 output, then eDP
> +	 * screen would show a blue picture (RGB888 show a green picture).
> +	 * But if I configure CTRC to RGBaaa, and eDP driver still keep
> +	 * RGB666 input video mode, then screen would works prefect.
> +	 */
> +	adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(eDP, AAAA);
> +
>  	return true;
>  }
>  
> @@ -114,27 +128,6 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
>  	int ret;
>  	u32 val;
>  
> -	/*
> -	 * FIXME(Yakir): driver should configure the CRTC output video
> -	 * mode with the display information which indicated the monitor
> -	 * support colorimetry.
> -	 *
> -	 * But don't know why the CRTC driver seems could only output the
> -	 * RGBaaa rightly. For example, if connect the "innolux,n116bge"
> -	 * eDP screen, EDID would indicated that screen only accepted the
> -	 * 6bpc mode. But if I configure CRTC to RGB666 output, then eDP
> -	 * screen would show a blue picture (RGB888 show a green picture).
> -	 * But if I configure CTRC to RGBaaa, and eDP driver still keep
> -	 * RGB666 input video mode, then screen would works prefect.
> -	 */
> -	ret = rockchip_drm_crtc_mode_config(encoder->crtc,
> -					    DRM_MODE_CONNECTOR_eDP,
> -					    ROCKCHIP_OUT_MODE_AAAA);
> -	if (ret < 0) {
> -		dev_err(dp->dev, "Could not set crtc mode config (%d)\n", ret);
> -		return;
> -	}
> -
>  	ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
>  	if (ret < 0)
>  		return;
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> index 7975158..49c4c72 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> @@ -875,11 +875,34 @@ static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder)
>  	clk_disable_unprepare(dsi->pclk);
>  }
>  
> +static bool dw_mipi_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
> +					const struct drm_display_mode *mode,
> +					struct drm_display_mode *adjusted_mode)
> +{
> +	struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);
> +
> +	switch (dsi->format) {
> +	case MIPI_DSI_FMT_RGB888:
> +		adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P888);
> +		break;
> +	case MIPI_DSI_FMT_RGB666:
> +		adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P666);
> +		break;
> +	case MIPI_DSI_FMT_RGB565:
> +		adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P565);
> +		break;
> +	default:
> +		WARN_ON(1);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder)
>  {
>  	struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);
>  	int mux = drm_of_encoder_active_endpoint_id(dsi->dev->of_node, encoder);
> -	u32 interface_pix_fmt;
>  	u32 val;
>  
>  	if (clk_prepare_enable(dsi->pclk)) {
> @@ -895,24 +918,6 @@ static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder)
>  
>  	clk_disable_unprepare(dsi->pclk);
>  
> -	switch (dsi->format) {
> -	case MIPI_DSI_FMT_RGB888:
> -		interface_pix_fmt = ROCKCHIP_OUT_MODE_P888;
> -		break;
> -	case MIPI_DSI_FMT_RGB666:
> -		interface_pix_fmt = ROCKCHIP_OUT_MODE_P666;
> -		break;
> -	case MIPI_DSI_FMT_RGB565:
> -		interface_pix_fmt = ROCKCHIP_OUT_MODE_P565;
> -		break;
> -	default:
> -		WARN_ON(1);
> -		return;
> -	}
> -
> -	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_DSI,
> -				      interface_pix_fmt);
> -
>  	if (mux)
>  		val = DSI0_SEL_VOP_LIT | (DSI0_SEL_VOP_LIT << 16);
>  	else
> @@ -924,6 +929,7 @@ static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder)
>  
>  static struct drm_encoder_helper_funcs
>  dw_mipi_dsi_encoder_helper_funcs = {
> +	.mode_fixup = dw_mipi_dsi_encoder_mode_fixup,
>  	.commit = dw_mipi_dsi_encoder_commit,
>  	.mode_set = dw_mipi_dsi_encoder_mode_set,
>  	.disable = dw_mipi_dsi_encoder_disable,
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index d5cfef7..a72af30 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -186,6 +186,8 @@ dw_hdmi_rockchip_encoder_mode_fixup(struct drm_encoder *encoder,
>  				    const struct drm_display_mode *mode,
>  				    struct drm_display_mode *adj_mode)
>  {
> +	adj_mode->private_flags = ROCKCHIP_DSP_MODE(HDMIA, AAAA);
> +
>  	return true;
>  }
>  
> @@ -201,9 +203,6 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
>  	u32 val;
>  	int mux;
>  
> -	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_HDMIA,
> -				      ROCKCHIP_OUT_MODE_AAAA);
> -
>  	mux = drm_of_encoder_active_endpoint_id(hdmi->dev->of_node, encoder);
>  	if (mux)
>  		val = HDMI_SEL_VOP_LIT | (HDMI_SEL_VOP_LIT << 16);
> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> index 10d62ff..ef6ad91 100644
> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> @@ -500,9 +500,6 @@ static void inno_hdmi_encoder_enable(struct drm_encoder *encoder)
>  {
>  	struct inno_hdmi *hdmi = to_inno_hdmi(encoder);
>  
> -	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_HDMIA,
> -				      ROCKCHIP_OUT_MODE_P888);
> -
>  	inno_hdmi_set_pwr_mode(hdmi, NORMAL);
>  }
>  
> @@ -517,6 +514,8 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
>  					 const struct drm_display_mode *mode,
>  					 struct drm_display_mode *adj_mode)
>  {
> +	adj_mode->private_flags = ROCKCHIP_DSP_MODE(HDMIA, P888);
> +
>  	return true;
>  }
>  
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> index 00d17d7..683210b 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> @@ -68,8 +68,6 @@ void rockchip_drm_atomic_work(struct work_struct *work);
>  int rockchip_register_crtc_funcs(struct drm_crtc *crtc,
>  				 const struct rockchip_crtc_funcs *crtc_funcs);
>  void rockchip_unregister_crtc_funcs(struct drm_crtc *crtc);
> -int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc, int connector_type,
> -				  int out_mode);
>  int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
>  				   struct device *dev);
>  void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index a619f12..1a611cf 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -818,38 +818,6 @@ static const struct drm_plane_funcs vop_plane_funcs = {
>  	.atomic_destroy_state = vop_atomic_plane_destroy_state,
>  };
>  
> -int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc,
> -				  int connector_type,
> -				  int out_mode)
> -{
> -	struct vop *vop = to_vop(crtc);
> -
> -	if (WARN_ON(!vop->is_enabled))
> -		return -EINVAL;
> -
> -	switch (connector_type) {
> -	case DRM_MODE_CONNECTOR_LVDS:
> -		VOP_CTRL_SET(vop, rgb_en, 1);
> -		break;
> -	case DRM_MODE_CONNECTOR_eDP:
> -		VOP_CTRL_SET(vop, edp_en, 1);
> -		break;
> -	case DRM_MODE_CONNECTOR_HDMIA:
> -		VOP_CTRL_SET(vop, hdmi_en, 1);
> -		break;
> -	case DRM_MODE_CONNECTOR_DSI:
> -		VOP_CTRL_SET(vop, mipi_en, 1);
> -		break;
> -	default:
> -		DRM_ERROR("unsupport connector_type[%d]\n", connector_type);
> -		return -EINVAL;
> -	};
> -	VOP_CTRL_SET(vop, out_mode, out_mode);
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(rockchip_drm_crtc_mode_config);
> -
>  static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
>  {
>  	struct vop *vop = to_vop(crtc);
> @@ -943,6 +911,8 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
>  	u16 vact_st = adjusted_mode->vtotal - adjusted_mode->vsync_start;
>  	u16 vact_end = vact_st + vdisplay;
>  	uint32_t val;
> +	int type = ROCKCHIP_OUT_MODE_TYPE(adjusted_mode->private_flags);
> +	int out_mode = ROCKCHIP_OUT_MODE(adjusted_mode->private_flags);
>  
>  	vop_enable(crtc);
>  	/*
> @@ -985,6 +955,23 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
>  	val |= (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC) ? 0 : 1;
>  	val |= (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC) ? 0 : (1 << 1);
>  	VOP_CTRL_SET(vop, pin_pol, val);
> +	switch (type) {
> +	case DRM_MODE_CONNECTOR_LVDS:
> +		VOP_CTRL_SET(vop, rgb_en, 1);
> +		break;
> +	case DRM_MODE_CONNECTOR_eDP:
> +		VOP_CTRL_SET(vop, edp_en, 1);
> +		break;
> +	case DRM_MODE_CONNECTOR_HDMIA:
> +		VOP_CTRL_SET(vop, hdmi_en, 1);
> +		break;
> +	case DRM_MODE_CONNECTOR_DSI:
> +		VOP_CTRL_SET(vop, mipi_en, 1);
> +		break;
> +	default:
> +		DRM_ERROR("unsupport connector_type[%d]\n", type);
> +	}
> +	VOP_CTRL_SET(vop, out_mode, out_mode);
>  
>  	VOP_CTRL_SET(vop, htotal_pw, (htotal << 16) | hsync_len);
>  	val = hact_st << 16;
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> index 071ff0b..4c63a66 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> @@ -183,6 +183,12 @@ struct vop_data {
>  /* for use special outface */
>  #define ROCKCHIP_OUT_MODE_AAAA	15
>  
> +#define ROCKCHIP_OUT_MODE_TYPE(x)	((x) >> 16)
> +#define ROCKCHIP_OUT_MODE(x)		((x) & 0xffff)
> +#define ROCKCHIP_DSP_MODE(type, mode) \
> +		((DRM_MODE_CONNECTOR_##type << 16) | \
> +		 (ROCKCHIP_OUT_MODE_##mode & 0xffff))
> +
>  enum alpha_mode {
>  	ALPHA_STRAIGHT,
>  	ALPHA_INVERSE,

Re: [PATCH] drm/rockchip: get rid of rockchip_drm_crtc_mode_config

[Mark yao]

On 2016年04月18日 17:25, John Keeping wrote:
>> >We need to take care of the vop status when use
>> >rockchip_drm_crtc_mode_config, if vop is disabled,
>> >the function would failed, that is terrible.
>> >
>> >Save connector type and output mode on drm_display_mode->private_flags
>> >at encoder mode_fixup, then we can configure the type and mode safely
>> >on crtc mode_set.
> Since Rockchip is atomic, shouldn't this be using atomic_check hooks and
> a subclassed crtc_state structure?
>

Right, Good idea.

Thanks.

-- 
Ｍark Yao

Re: [PATCH] drm/rockchip: get rid of rockchip_drm_crtc_mode_config

[Mark yao]

On 2016年04月18日 17:25, John Keeping wrote:
>> >We need to take care of the vop status when use
>> >rockchip_drm_crtc_mode_config, if vop is disabled,
>> >the function would failed, that is terrible.
>> >
>> >Save connector type and output mode on drm_display_mode->private_flags
>> >at encoder mode_fixup, then we can configure the type and mode safely
>> >on crtc mode_set.
> Since Rockchip is atomic, shouldn't this be using atomic_check hooks and
> a subclassed crtc_state structure?
>

Right, Good idea.

Thanks.

-- 
Ｍark Yao


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/rockchip: get rid of rockchip_drm_crtc_mode_config

[Mark yao]

On 2016?04?18? 17:25, John Keeping wrote:
>> >We need to take care of the vop status when use
>> >rockchip_drm_crtc_mode_config, if vop is disabled,
>> >the function would failed, that is terrible.
>> >
>> >Save connector type and output mode on drm_display_mode->private_flags
>> >at encoder mode_fixup, then we can configure the type and mode safely
>> >on crtc mode_set.
> Since Rockchip is atomic, shouldn't this be using atomic_check hooks and
> a subclassed crtc_state structure?
>

Right, Good idea.

Thanks.

-- 
?ark Yao

Re: [PATCH] drm/rockchip: get rid of rockchip_drm_crtc_mode_config

[Mark yao]

On 2016年04月18日 17:25, John Keeping wrote:
> On Mon, 18 Apr 2016 11:03:31 +0800, Mark Yao wrote:
>
>> >We need to take care of the vop status when use
>> >rockchip_drm_crtc_mode_config, if vop is disabled,
>> >the function would failed, that is terrible.
>> >
>> >Save connector type and output mode on drm_display_mode->private_flags
>> >at encoder mode_fixup, then we can configure the type and mode safely
>> >on crtc mode_set.
> Since Rockchip is atomic, shouldn't this be using atomic_check hooks and
> a subclassed crtc_state structure?
>
Hi John

I try to use atomic_check with crtc_state, but it seems not easy,
there are two value need transmit from connector to vop: connector type 
and out_mode

the connector type I think we can loop the atomic state to find the 
connector type.
but the out_mode is a custom value, I can't find a generic way to 
transmit it with atomic state.

BTW, I think on atomic side, the drm_display_mode is under control by 
atomic state,
and the mode->private_flags is not use by drm framework, I found i915 
and gma500 also use
mode->private_flags to transmit custom value.

So I think it's no problem using mode->private_flags.

-- 
Ｍark Yao

Re: [PATCH] drm/rockchip: get rid of rockchip_drm_crtc_mode_config

[Mark yao]

On 2016年04月18日 17:25, John Keeping wrote:
> On Mon, 18 Apr 2016 11:03:31 +0800, Mark Yao wrote:
>
>> >We need to take care of the vop status when use
>> >rockchip_drm_crtc_mode_config, if vop is disabled,
>> >the function would failed, that is terrible.
>> >
>> >Save connector type and output mode on drm_display_mode->private_flags
>> >at encoder mode_fixup, then we can configure the type and mode safely
>> >on crtc mode_set.
> Since Rockchip is atomic, shouldn't this be using atomic_check hooks and
> a subclassed crtc_state structure?
>
Hi John

I try to use atomic_check with crtc_state, but it seems not easy,
there are two value need transmit from connector to vop: connector type 
and out_mode

the connector type I think we can loop the atomic state to find the 
connector type.
but the out_mode is a custom value, I can't find a generic way to 
transmit it with atomic state.

BTW, I think on atomic side, the drm_display_mode is under control by 
atomic state,
and the mode->private_flags is not use by drm framework, I found i915 
and gma500 also use
mode->private_flags to transmit custom value.

So I think it's no problem using mode->private_flags.

-- 
Ｍark Yao


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/rockchip: get rid of rockchip_drm_crtc_mode_config

[Mark yao]

On 2016?04?18? 17:25, John Keeping wrote:
> On Mon, 18 Apr 2016 11:03:31 +0800, Mark Yao wrote:
>
>> >We need to take care of the vop status when use
>> >rockchip_drm_crtc_mode_config, if vop is disabled,
>> >the function would failed, that is terrible.
>> >
>> >Save connector type and output mode on drm_display_mode->private_flags
>> >at encoder mode_fixup, then we can configure the type and mode safely
>> >on crtc mode_set.
> Since Rockchip is atomic, shouldn't this be using atomic_check hooks and
> a subclassed crtc_state structure?
>
Hi John

I try to use atomic_check with crtc_state, but it seems not easy,
there are two value need transmit from connector to vop: connector type 
and out_mode

the connector type I think we can loop the atomic state to find the 
connector type.
but the out_mode is a custom value, I can't find a generic way to 
transmit it with atomic state.

BTW, I think on atomic side, the drm_display_mode is under control by 
atomic state,
and the mode->private_flags is not use by drm framework, I found i915 
and gma500 also use
mode->private_flags to transmit custom value.

So I think it's no problem using mode->private_flags.

-- 
?ark Yao

Re: [PATCH] drm/rockchip: get rid of rockchip_drm_crtc_mode_config

[John Keeping]

On Tue, 19 Apr 2016 10:43:03 +0800, Mark yao wrote:

> On 2016年04月18日 17:25, John Keeping wrote:
> > On Mon, 18 Apr 2016 11:03:31 +0800, Mark Yao wrote:
> >  
> >> >We need to take care of the vop status when use
> >> >rockchip_drm_crtc_mode_config, if vop is disabled,
> >> >the function would failed, that is terrible.
> >> >
> >> >Save connector type and output mode on drm_display_mode->private_flags
> >> >at encoder mode_fixup, then we can configure the type and mode safely
> >> >on crtc mode_set.  
> > Since Rockchip is atomic, shouldn't this be using atomic_check hooks and
> > a subclassed crtc_state structure?
> >  
> 
> I try to use atomic_check with crtc_state, but it seems not easy,
> there are two value need transmit from connector to vop: connector type 
> and out_mode
> 
> the connector type I think we can loop the atomic state to find the 
> connector type.
> but the out_mode is a custom value, I can't find a generic way to 
> transmit it with atomic state.
> 
> BTW, I think on atomic side, the drm_display_mode is under control by 
> atomic state,
> and the mode->private_flags is not use by drm framework, I found i915 
> and gma500 also use
> mode->private_flags to transmit custom value.
> 
> So I think it's no problem using mode->private_flags.

The documentation for drm_display_mode::private says:

    It shouldn't be used by atomic drivers since they can store any
    additional data by subclassing state structures.

This applies to private_flags as well.

I think this means that we should do something like the patch below
(which isn't even compile tested and doesn't cover implementing
drm_encoder_helper_funcs::atomic_check on all of the encoders):

-- >8 --
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index 00d17d71aa4c..9d65fa9188f4 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -31,6 +31,14 @@
 struct drm_device;
 struct drm_connector;
 
+struct rockchip_crtc_state {
+	struct drm_crtc_state base;
+	int output_type;
+	int output_mode;
+};
+
+#define to_rockchip_crtc_state(s) container_of(x, struct rockchip_crtc_state, base)
+
 /*
  * Rockchip drm private crtc funcs.
  * @enable_vblank: enable crtc vblank irq.
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index a619f120f801..5cdf3123e1eb 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1044,13 +1044,34 @@ static void vop_crtc_destroy(struct drm_crtc *crtc)
 	drm_crtc_cleanup(crtc);
 }
 
+static struct drm_crtc_state *vop_crtc_duplicate_state(struct drm_crtc *crtc)
+{
+	struct rockchip_crtc_state *rockchip_state;
+
+	rockchip_state = kzalloc(sizeof(*rockchip_state), GFP_KERNEL);
+	if (!rockchip_state)
+		return NULL;
+
+	__drm_atomic_helper_crtc_duplicate_state(crtc, &rockchip_state->base);
+	return &rockchip_state->base;
+}
+
+static void vop_crtc_destroy_state(struct drm_crtc *crtc,
+				   struct drm_crtc_state *state)
+{
+	struct rockchip_crtc_state *s = to_rockchip_crtc_state(state);
+
+	__drm_atomic_helper_crtc_destroy_state(crtc, &s->base);
+	kfree(s);
+}
+
 static const struct drm_crtc_funcs vop_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
 	.page_flip = drm_atomic_helper_page_flip,
 	.destroy = vop_crtc_destroy,
 	.reset = drm_atomic_helper_crtc_reset,
-	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+	.atomic_duplicate_state = vop_crtc_duplicate_state,
+	.atomic_destroy_state = vop_crtc_destroy_state,
 };
 
 static bool vop_win_pending_is_complete(struct vop_win *vop_win)

Re: [PATCH] drm/rockchip: get rid of rockchip_drm_crtc_mode_config

[John Keeping]

On Tue, 19 Apr 2016 10:43:03 +0800, Mark yao wrote:

> On 2016年04月18日 17:25, John Keeping wrote:
> > On Mon, 18 Apr 2016 11:03:31 +0800, Mark Yao wrote:
> >  
> >> >We need to take care of the vop status when use
> >> >rockchip_drm_crtc_mode_config, if vop is disabled,
> >> >the function would failed, that is terrible.
> >> >
> >> >Save connector type and output mode on drm_display_mode->private_flags
> >> >at encoder mode_fixup, then we can configure the type and mode safely
> >> >on crtc mode_set.  
> > Since Rockchip is atomic, shouldn't this be using atomic_check hooks and
> > a subclassed crtc_state structure?
> >  
> 
> I try to use atomic_check with crtc_state, but it seems not easy,
> there are two value need transmit from connector to vop: connector type 
> and out_mode
> 
> the connector type I think we can loop the atomic state to find the 
> connector type.
> but the out_mode is a custom value, I can't find a generic way to 
> transmit it with atomic state.
> 
> BTW, I think on atomic side, the drm_display_mode is under control by 
> atomic state,
> and the mode->private_flags is not use by drm framework, I found i915 
> and gma500 also use
> mode->private_flags to transmit custom value.
> 
> So I think it's no problem using mode->private_flags.

The documentation for drm_display_mode::private says:

    It shouldn't be used by atomic drivers since they can store any
    additional data by subclassing state structures.

This applies to private_flags as well.

I think this means that we should do something like the patch below
(which isn't even compile tested and doesn't cover implementing
drm_encoder_helper_funcs::atomic_check on all of the encoders):

-- >8 --
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index 00d17d71aa4c..9d65fa9188f4 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -31,6 +31,14 @@
 struct drm_device;
 struct drm_connector;
 
+struct rockchip_crtc_state {
+	struct drm_crtc_state base;
+	int output_type;
+	int output_mode;
+};
+
+#define to_rockchip_crtc_state(s) container_of(x, struct rockchip_crtc_state, base)
+
 /*
  * Rockchip drm private crtc funcs.
  * @enable_vblank: enable crtc vblank irq.
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index a619f120f801..5cdf3123e1eb 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1044,13 +1044,34 @@ static void vop_crtc_destroy(struct drm_crtc *crtc)
 	drm_crtc_cleanup(crtc);
 }
 
+static struct drm_crtc_state *vop_crtc_duplicate_state(struct drm_crtc *crtc)
+{
+	struct rockchip_crtc_state *rockchip_state;
+
+	rockchip_state = kzalloc(sizeof(*rockchip_state), GFP_KERNEL);
+	if (!rockchip_state)
+		return NULL;
+
+	__drm_atomic_helper_crtc_duplicate_state(crtc, &rockchip_state->base);
+	return &rockchip_state->base;
+}
+
+static void vop_crtc_destroy_state(struct drm_crtc *crtc,
+				   struct drm_crtc_state *state)
+{
+	struct rockchip_crtc_state *s = to_rockchip_crtc_state(state);
+
+	__drm_atomic_helper_crtc_destroy_state(crtc, &s->base);
+	kfree(s);
+}
+
 static const struct drm_crtc_funcs vop_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
 	.page_flip = drm_atomic_helper_page_flip,
 	.destroy = vop_crtc_destroy,
 	.reset = drm_atomic_helper_crtc_reset,
-	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+	.atomic_duplicate_state = vop_crtc_duplicate_state,
+	.atomic_destroy_state = vop_crtc_destroy_state,
 };
 
 static bool vop_win_pending_is_complete(struct vop_win *vop_win)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/rockchip: get rid of rockchip_drm_crtc_mode_config

[John Keeping]

On Tue, 19 Apr 2016 10:43:03 +0800, Mark yao wrote:

> On 2016?04?18? 17:25, John Keeping wrote:
> > On Mon, 18 Apr 2016 11:03:31 +0800, Mark Yao wrote:
> >  
> >> >We need to take care of the vop status when use
> >> >rockchip_drm_crtc_mode_config, if vop is disabled,
> >> >the function would failed, that is terrible.
> >> >
> >> >Save connector type and output mode on drm_display_mode->private_flags
> >> >at encoder mode_fixup, then we can configure the type and mode safely
> >> >on crtc mode_set.  
> > Since Rockchip is atomic, shouldn't this be using atomic_check hooks and
> > a subclassed crtc_state structure?
> >  
> 
> I try to use atomic_check with crtc_state, but it seems not easy,
> there are two value need transmit from connector to vop: connector type 
> and out_mode
> 
> the connector type I think we can loop the atomic state to find the 
> connector type.
> but the out_mode is a custom value, I can't find a generic way to 
> transmit it with atomic state.
> 
> BTW, I think on atomic side, the drm_display_mode is under control by 
> atomic state,
> and the mode->private_flags is not use by drm framework, I found i915 
> and gma500 also use
> mode->private_flags to transmit custom value.
> 
> So I think it's no problem using mode->private_flags.

The documentation for drm_display_mode::private says:

    It shouldn't be used by atomic drivers since they can store any
    additional data by subclassing state structures.

This applies to private_flags as well.

I think this means that we should do something like the patch below
(which isn't even compile tested and doesn't cover implementing
drm_encoder_helper_funcs::atomic_check on all of the encoders):

-- >8 --
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index 00d17d71aa4c..9d65fa9188f4 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -31,6 +31,14 @@
 struct drm_device;
 struct drm_connector;
 
+struct rockchip_crtc_state {
+	struct drm_crtc_state base;
+	int output_type;
+	int output_mode;
+};
+
+#define to_rockchip_crtc_state(s) container_of(x, struct rockchip_crtc_state, base)
+
 /*
  * Rockchip drm private crtc funcs.
  * @enable_vblank: enable crtc vblank irq.
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index a619f120f801..5cdf3123e1eb 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1044,13 +1044,34 @@ static void vop_crtc_destroy(struct drm_crtc *crtc)
 	drm_crtc_cleanup(crtc);
 }
 
+static struct drm_crtc_state *vop_crtc_duplicate_state(struct drm_crtc *crtc)
+{
+	struct rockchip_crtc_state *rockchip_state;
+
+	rockchip_state = kzalloc(sizeof(*rockchip_state), GFP_KERNEL);
+	if (!rockchip_state)
+		return NULL;
+
+	__drm_atomic_helper_crtc_duplicate_state(crtc, &rockchip_state->base);
+	return &rockchip_state->base;
+}
+
+static void vop_crtc_destroy_state(struct drm_crtc *crtc,
+				   struct drm_crtc_state *state)
+{
+	struct rockchip_crtc_state *s = to_rockchip_crtc_state(state);
+
+	__drm_atomic_helper_crtc_destroy_state(crtc, &s->base);
+	kfree(s);
+}
+
 static const struct drm_crtc_funcs vop_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
 	.page_flip = drm_atomic_helper_page_flip,
 	.destroy = vop_crtc_destroy,
 	.reset = drm_atomic_helper_crtc_reset,
-	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+	.atomic_duplicate_state = vop_crtc_duplicate_state,
+	.atomic_destroy_state = vop_crtc_destroy_state,
 };
 
 static bool vop_win_pending_is_complete(struct vop_win *vop_win)

Re: [PATCH] drm/rockchip: get rid of rockchip_drm_crtc_mode_config

[Mark yao]

On 2016年04月19日 17:06, John Keeping wrote:
> On Tue, 19 Apr 2016 10:43:03 +0800, Mark yao wrote:
>
>> On 2016年04月18日 17:25, John Keeping wrote:
>>> On Mon, 18 Apr 2016 11:03:31 +0800, Mark Yao wrote:
>>>   
>>>>> We need to take care of the vop status when use
>>>>> rockchip_drm_crtc_mode_config, if vop is disabled,
>>>>> the function would failed, that is terrible.
>>>>>
>>>>> Save connector type and output mode on drm_display_mode->private_flags
>>>>> at encoder mode_fixup, then we can configure the type and mode safely
>>>>> on crtc mode_set.
>>> Since Rockchip is atomic, shouldn't this be using atomic_check hooks and
>>> a subclassed crtc_state structure?
>>>   
>> I try to use atomic_check with crtc_state, but it seems not easy,
>> there are two value need transmit from connector to vop: connector type
>> and out_mode
>>
>> the connector type I think we can loop the atomic state to find the
>> connector type.
>> but the out_mode is a custom value, I can't find a generic way to
>> transmit it with atomic state.
>>
>> BTW, I think on atomic side, the drm_display_mode is under control by
>> atomic state,
>> and the mode->private_flags is not use by drm framework, I found i915
>> and gma500 also use
>> mode->private_flags to transmit custom value.
>>
>> So I think it's no problem using mode->private_flags.
> The documentation for drm_display_mode::private says:
>
>      It shouldn't be used by atomic drivers since they can store any
>      additional data by subclassing state structures.
>
> This applies to private_flags as well.
>
> I think this means that we should do something like the patch below
> (which isn't even compile tested and doesn't cover implementing
> drm_encoder_helper_funcs::atomic_check on all of the encoders):
>
> -- >8 --
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> index 00d17d71aa4c..9d65fa9188f4 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> @@ -31,6 +31,14 @@
>   struct drm_device;
>   struct drm_connector;
>   
> +struct rockchip_crtc_state {
> +	struct drm_crtc_state base;
> +	int output_type;
> +	int output_mode;
> +};
> +
> +#define to_rockchip_crtc_state(s) container_of(x, struct rockchip_crtc_state, base)
> +
>   /*
>    * Rockchip drm private crtc funcs.
>    * @enable_vblank: enable crtc vblank irq.
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index a619f120f801..5cdf3123e1eb 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -1044,13 +1044,34 @@ static void vop_crtc_destroy(struct drm_crtc *crtc)
>   	drm_crtc_cleanup(crtc);
>   }
>   
> +static struct drm_crtc_state *vop_crtc_duplicate_state(struct drm_crtc *crtc)
> +{
> +	struct rockchip_crtc_state *rockchip_state;
> +
> +	rockchip_state = kzalloc(sizeof(*rockchip_state), GFP_KERNEL);
> +	if (!rockchip_state)
> +		return NULL;
> +
> +	__drm_atomic_helper_crtc_duplicate_state(crtc, &rockchip_state->base);
> +	return &rockchip_state->base;
> +}
> +
> +static void vop_crtc_destroy_state(struct drm_crtc *crtc,
> +				   struct drm_crtc_state *state)
> +{
> +	struct rockchip_crtc_state *s = to_rockchip_crtc_state(state);
> +
> +	__drm_atomic_helper_crtc_destroy_state(crtc, &s->base);
> +	kfree(s);
> +}
> +
>   static const struct drm_crtc_funcs vop_crtc_funcs = {
>   	.set_config = drm_atomic_helper_set_config,
>   	.page_flip = drm_atomic_helper_page_flip,
>   	.destroy = vop_crtc_destroy,
>   	.reset = drm_atomic_helper_crtc_reset,
> -	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> +	.atomic_duplicate_state = vop_crtc_duplicate_state,
> +	.atomic_destroy_state = vop_crtc_destroy_state,
>   };
>   
>   static bool vop_win_pending_is_complete(struct vop_win *vop_win)
>
>
>
Hi John

Good, I had implement it according to  your advise.

Thanks.

-- 
Ｍark Yao

Re: [PATCH] drm/rockchip: get rid of rockchip_drm_crtc_mode_config

[Mark yao]

On 2016年04月19日 17:06, John Keeping wrote:
> On Tue, 19 Apr 2016 10:43:03 +0800, Mark yao wrote:
>
>> On 2016年04月18日 17:25, John Keeping wrote:
>>> On Mon, 18 Apr 2016 11:03:31 +0800, Mark Yao wrote:
>>>   
>>>>> We need to take care of the vop status when use
>>>>> rockchip_drm_crtc_mode_config, if vop is disabled,
>>>>> the function would failed, that is terrible.
>>>>>
>>>>> Save connector type and output mode on drm_display_mode->private_flags
>>>>> at encoder mode_fixup, then we can configure the type and mode safely
>>>>> on crtc mode_set.
>>> Since Rockchip is atomic, shouldn't this be using atomic_check hooks and
>>> a subclassed crtc_state structure?
>>>   
>> I try to use atomic_check with crtc_state, but it seems not easy,
>> there are two value need transmit from connector to vop: connector type
>> and out_mode
>>
>> the connector type I think we can loop the atomic state to find the
>> connector type.
>> but the out_mode is a custom value, I can't find a generic way to
>> transmit it with atomic state.
>>
>> BTW, I think on atomic side, the drm_display_mode is under control by
>> atomic state,
>> and the mode->private_flags is not use by drm framework, I found i915
>> and gma500 also use
>> mode->private_flags to transmit custom value.
>>
>> So I think it's no problem using mode->private_flags.
> The documentation for drm_display_mode::private says:
>
>      It shouldn't be used by atomic drivers since they can store any
>      additional data by subclassing state structures.
>
> This applies to private_flags as well.
>
> I think this means that we should do something like the patch below
> (which isn't even compile tested and doesn't cover implementing
> drm_encoder_helper_funcs::atomic_check on all of the encoders):
>
> -- >8 --
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> index 00d17d71aa4c..9d65fa9188f4 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> @@ -31,6 +31,14 @@
>   struct drm_device;
>   struct drm_connector;
>   
> +struct rockchip_crtc_state {
> +	struct drm_crtc_state base;
> +	int output_type;
> +	int output_mode;
> +};
> +
> +#define to_rockchip_crtc_state(s) container_of(x, struct rockchip_crtc_state, base)
> +
>   /*
>    * Rockchip drm private crtc funcs.
>    * @enable_vblank: enable crtc vblank irq.
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index a619f120f801..5cdf3123e1eb 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -1044,13 +1044,34 @@ static void vop_crtc_destroy(struct drm_crtc *crtc)
>   	drm_crtc_cleanup(crtc);
>   }
>   
> +static struct drm_crtc_state *vop_crtc_duplicate_state(struct drm_crtc *crtc)
> +{
> +	struct rockchip_crtc_state *rockchip_state;
> +
> +	rockchip_state = kzalloc(sizeof(*rockchip_state), GFP_KERNEL);
> +	if (!rockchip_state)
> +		return NULL;
> +
> +	__drm_atomic_helper_crtc_duplicate_state(crtc, &rockchip_state->base);
> +	return &rockchip_state->base;
> +}
> +
> +static void vop_crtc_destroy_state(struct drm_crtc *crtc,
> +				   struct drm_crtc_state *state)
> +{
> +	struct rockchip_crtc_state *s = to_rockchip_crtc_state(state);
> +
> +	__drm_atomic_helper_crtc_destroy_state(crtc, &s->base);
> +	kfree(s);
> +}
> +
>   static const struct drm_crtc_funcs vop_crtc_funcs = {
>   	.set_config = drm_atomic_helper_set_config,
>   	.page_flip = drm_atomic_helper_page_flip,
>   	.destroy = vop_crtc_destroy,
>   	.reset = drm_atomic_helper_crtc_reset,
> -	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> +	.atomic_duplicate_state = vop_crtc_duplicate_state,
> +	.atomic_destroy_state = vop_crtc_destroy_state,
>   };
>   
>   static bool vop_win_pending_is_complete(struct vop_win *vop_win)
>
>
>
Hi John

Good, I had implement it according to  your advise.

Thanks.

-- 
Ｍark Yao


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/rockchip: get rid of rockchip_drm_crtc_mode_config

[Mark yao]

On 2016?04?19? 17:06, John Keeping wrote:
> On Tue, 19 Apr 2016 10:43:03 +0800, Mark yao wrote:
>
>> On 2016?04?18? 17:25, John Keeping wrote:
>>> On Mon, 18 Apr 2016 11:03:31 +0800, Mark Yao wrote:
>>>   
>>>>> We need to take care of the vop status when use
>>>>> rockchip_drm_crtc_mode_config, if vop is disabled,
>>>>> the function would failed, that is terrible.
>>>>>
>>>>> Save connector type and output mode on drm_display_mode->private_flags
>>>>> at encoder mode_fixup, then we can configure the type and mode safely
>>>>> on crtc mode_set.
>>> Since Rockchip is atomic, shouldn't this be using atomic_check hooks and
>>> a subclassed crtc_state structure?
>>>   
>> I try to use atomic_check with crtc_state, but it seems not easy,
>> there are two value need transmit from connector to vop: connector type
>> and out_mode
>>
>> the connector type I think we can loop the atomic state to find the
>> connector type.
>> but the out_mode is a custom value, I can't find a generic way to
>> transmit it with atomic state.
>>
>> BTW, I think on atomic side, the drm_display_mode is under control by
>> atomic state,
>> and the mode->private_flags is not use by drm framework, I found i915
>> and gma500 also use
>> mode->private_flags to transmit custom value.
>>
>> So I think it's no problem using mode->private_flags.
> The documentation for drm_display_mode::private says:
>
>      It shouldn't be used by atomic drivers since they can store any
>      additional data by subclassing state structures.
>
> This applies to private_flags as well.
>
> I think this means that we should do something like the patch below
> (which isn't even compile tested and doesn't cover implementing
> drm_encoder_helper_funcs::atomic_check on all of the encoders):
>
> -- >8 --
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> index 00d17d71aa4c..9d65fa9188f4 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> @@ -31,6 +31,14 @@
>   struct drm_device;
>   struct drm_connector;
>   
> +struct rockchip_crtc_state {
> +	struct drm_crtc_state base;
> +	int output_type;
> +	int output_mode;
> +};
> +
> +#define to_rockchip_crtc_state(s) container_of(x, struct rockchip_crtc_state, base)
> +
>   /*
>    * Rockchip drm private crtc funcs.
>    * @enable_vblank: enable crtc vblank irq.
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index a619f120f801..5cdf3123e1eb 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -1044,13 +1044,34 @@ static void vop_crtc_destroy(struct drm_crtc *crtc)
>   	drm_crtc_cleanup(crtc);
>   }
>   
> +static struct drm_crtc_state *vop_crtc_duplicate_state(struct drm_crtc *crtc)
> +{
> +	struct rockchip_crtc_state *rockchip_state;
> +
> +	rockchip_state = kzalloc(sizeof(*rockchip_state), GFP_KERNEL);
> +	if (!rockchip_state)
> +		return NULL;
> +
> +	__drm_atomic_helper_crtc_duplicate_state(crtc, &rockchip_state->base);
> +	return &rockchip_state->base;
> +}
> +
> +static void vop_crtc_destroy_state(struct drm_crtc *crtc,
> +				   struct drm_crtc_state *state)
> +{
> +	struct rockchip_crtc_state *s = to_rockchip_crtc_state(state);
> +
> +	__drm_atomic_helper_crtc_destroy_state(crtc, &s->base);
> +	kfree(s);
> +}
> +
>   static const struct drm_crtc_funcs vop_crtc_funcs = {
>   	.set_config = drm_atomic_helper_set_config,
>   	.page_flip = drm_atomic_helper_page_flip,
>   	.destroy = vop_crtc_destroy,
>   	.reset = drm_atomic_helper_crtc_reset,
> -	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> +	.atomic_duplicate_state = vop_crtc_duplicate_state,
> +	.atomic_destroy_state = vop_crtc_destroy_state,
>   };
>   
>   static bool vop_win_pending_is_complete(struct vop_win *vop_win)
>
>
>
Hi John

Good, I had implement it according to  your advise.

Thanks.

-- 
?ark Yao