[PATCH 0/3] imx drm atomic mode setting fixups

[PATCH 0/3] imx drm atomic mode setting fixups

[Philipp Zabel]

Hi,

I have tested Liu Ying's imx drm atomic mode setting conversion series [1]
with three different outputs (DPI + eDP bridge, LVDS + panel, HDMI)
and noticed that in the LDB case bus_format handling doesn't work
correctly if the format is determined by the panel instead of the
device tree. In that case, the LVDS bus_format is not yet known at bind
time, but is filled into the display_info structure by the panel driver
in the get_modes callback. The LDB driver then has to translate from the
LVDS bus format to the corresponding internal parallel bus format and
let the crtc know about the format before crtc mode_set is called.
The same issue occurs when the bus_format is determined by a bridge driver
which implements its own connector. To solve this, and because crtc state
seems to be the right place for potentially dynamic crtc settings, I have
moved the bus_format, bus_flags, and di_*sync_pin configuration into an
imx-drm specific crtc state structure and removed imx_drm_encoder again.

I have also removed the now optional empty encoder mode_set callbacks
and turned the container_of helper macros into inline functions.

[1] https://lists.freedesktop.org/archives/dri-devel/2016-May/108218.html

regards
Philipp

Philipp Zabel (3):
  drm/imx: remove empty mode_set encoder callbacks
  drm/imx: store internal bus configuration in crtc state
  drm/imx: turn remaining container_of macros into inline functions

 drivers/gpu/drm/imx/dw_hdmi-imx.c      |  38 +++++----
 drivers/gpu/drm/imx/imx-drm.h          |   9 ++-
 drivers/gpu/drm/imx/imx-ldb.c          | 142 +++++++++++++++++++++------------
 drivers/gpu/drm/imx/imx-tve.c          |  45 ++++++++---
 drivers/gpu/drm/imx/ipuv3-crtc.c       |  72 +++++++++++++----
 drivers/gpu/drm/imx/ipuv3-plane.c      |   5 +-
 drivers/gpu/drm/imx/parallel-display.c |  85 +++++++++++---------
 7 files changed, 259 insertions(+), 137 deletions(-)

-- 
2.8.1

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

[PATCH 1/3] drm/imx: remove empty mode_set encoder callbacks

[Philipp Zabel]

With atomic modeset support, these callbacks are optional.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/dw_hdmi-imx.c      | 7 -------
 drivers/gpu/drm/imx/parallel-display.c | 7 -------
 2 files changed, 14 deletions(-)

diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
index 5f1d437..dce1ea5 100644
--- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
+++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
@@ -111,12 +111,6 @@ static void dw_hdmi_imx_encoder_disable(struct drm_encoder *encoder)
 {
 }
 
-static void dw_hdmi_imx_encoder_mode_set(struct drm_encoder *encoder,
-					 struct drm_display_mode *mode,
-					 struct drm_display_mode *adj_mode)
-{
-}
-
 static void dw_hdmi_imx_encoder_enable(struct drm_encoder *encoder)
 {
 	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
@@ -129,7 +123,6 @@ static void dw_hdmi_imx_encoder_enable(struct drm_encoder *encoder)
 }
 
 static const struct drm_encoder_helper_funcs dw_hdmi_imx_encoder_helper_funcs = {
-	.mode_set   = dw_hdmi_imx_encoder_mode_set,
 	.enable     = dw_hdmi_imx_encoder_enable,
 	.disable    = dw_hdmi_imx_encoder_disable,
 };
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index bb5dbd6..4a2942e 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -101,12 +101,6 @@ static void imx_pd_encoder_enable(struct drm_encoder *encoder)
 	drm_panel_enable(imxpd->panel);
 }
 
-static void imx_pd_encoder_mode_set(struct drm_encoder *encoder,
-			 struct drm_display_mode *orig_mode,
-			 struct drm_display_mode *mode)
-{
-}
-
 static void imx_pd_encoder_disable(struct drm_encoder *encoder)
 {
 	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
@@ -136,7 +130,6 @@ static const struct drm_encoder_funcs imx_pd_encoder_funcs = {
 };
 
 static const struct drm_encoder_helper_funcs imx_pd_encoder_helper_funcs = {
-	.mode_set = imx_pd_encoder_mode_set,
 	.enable = imx_pd_encoder_enable,
 	.disable = imx_pd_encoder_disable,
 };
-- 
2.8.1

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

[PATCH 2/3] drm/imx: store internal bus configuration in crtc state

[Philipp Zabel]

The internal bus configuration is imx-drm specific crtc state. Store it
in imx_crtc_state and let the encoder atomic_check callbacks determine
bus_flags, bus_format and the sync pins, possibly taking into account
the mode and the connector display info.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/dw_hdmi-imx.c      |  31 +++++---
 drivers/gpu/drm/imx/imx-drm.h          |   9 ++-
 drivers/gpu/drm/imx/imx-ldb.c          | 135 +++++++++++++++++++++------------
 drivers/gpu/drm/imx/imx-tve.c          |  38 +++++++---
 drivers/gpu/drm/imx/ipuv3-crtc.c       |  67 ++++++++++++----
 drivers/gpu/drm/imx/parallel-display.c |  71 +++++++++--------
 6 files changed, 236 insertions(+), 115 deletions(-)

diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
index dce1ea5..359cd27 100644
--- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
+++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
@@ -22,14 +22,17 @@
 
 #include "imx-drm.h"
 
-#define imx_enc_to_imx_hdmi(x) container_of(x, struct imx_hdmi, imx_encoder)
-
 struct imx_hdmi {
 	struct device *dev;
-	struct imx_drm_encoder imx_encoder;
+	struct drm_encoder encoder;
 	struct regmap *regmap;
 };
 
+static inline struct imx_hdmi *enc_to_imx_hdmi(struct drm_encoder *e)
+{
+	return container_of(e, struct imx_hdmi, encoder);
+}
+
 static const struct dw_hdmi_mpll_config imx_mpll_cfg[] = {
 	{
 		45250000, {
@@ -113,8 +116,7 @@ static void dw_hdmi_imx_encoder_disable(struct drm_encoder *encoder)
 
 static void dw_hdmi_imx_encoder_enable(struct drm_encoder *encoder)
 {
-	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
-	struct imx_hdmi *hdmi = imx_enc_to_imx_hdmi(imx_encoder);
+	struct imx_hdmi *hdmi = enc_to_imx_hdmi(encoder);
 	int mux = drm_of_encoder_active_port_id(hdmi->dev->of_node, encoder);
 
 	regmap_update_bits(hdmi->regmap, IOMUXC_GPR3,
@@ -122,9 +124,23 @@ static void dw_hdmi_imx_encoder_enable(struct drm_encoder *encoder)
 			   mux << IMX6Q_GPR3_HDMI_MUX_CTL_SHIFT);
 }
 
+static int dw_hdmi_imx_atomic_check(struct drm_encoder *encoder,
+				    struct drm_crtc_state *crtc_state,
+				    struct drm_connector_state *conn_state)
+{
+	struct imx_crtc_state *imx_crtc_state = to_imx_crtc_state(crtc_state);
+
+	imx_crtc_state->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+	imx_crtc_state->di_hsync_pin = 2;
+	imx_crtc_state->di_vsync_pin = 3;
+
+	return 0;
+}
+
 static const struct drm_encoder_helper_funcs dw_hdmi_imx_encoder_helper_funcs = {
 	.enable     = dw_hdmi_imx_encoder_enable,
 	.disable    = dw_hdmi_imx_encoder_disable,
+	.atomic_check = dw_hdmi_imx_atomic_check,
 };
 
 static const struct drm_encoder_funcs dw_hdmi_imx_encoder_funcs = {
@@ -205,10 +221,7 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
 	match = of_match_node(dw_hdmi_imx_dt_ids, pdev->dev.of_node);
 	plat_data = match->data;
 	hdmi->dev = &pdev->dev;
-	encoder = &hdmi->imx_encoder.encoder;
-	hdmi->imx_encoder.bus_format = MEDIA_BUS_FMT_RGB888_1X24;
-	hdmi->imx_encoder.di_hsync_pin = 2;
-	hdmi->imx_encoder.di_vsync_pin = 3;
+	encoder = &hdmi->encoder;
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
diff --git a/drivers/gpu/drm/imx/imx-drm.h b/drivers/gpu/drm/imx/imx-drm.h
index 39cef15..07d33e4 100644
--- a/drivers/gpu/drm/imx/imx-drm.h
+++ b/drivers/gpu/drm/imx/imx-drm.h
@@ -15,15 +15,18 @@ struct platform_device;
 
 unsigned int imx_drm_crtc_id(struct imx_drm_crtc *crtc);
 
-struct imx_drm_encoder {
-	struct drm_encoder			encoder;
+struct imx_crtc_state {
+	struct drm_crtc_state			base;
 	u32					bus_format;
 	u32					bus_flags;
 	int					di_hsync_pin;
 	int					di_vsync_pin;
 };
 
-#define enc_to_imx_enc(x) container_of(x, struct imx_drm_encoder, encoder)
+static inline struct imx_crtc_state *to_imx_crtc_state(struct drm_crtc_state *s)
+{
+	return container_of(s, struct imx_crtc_state, base);
+}
 
 struct imx_drm_crtc_helper_funcs {
 	int (*enable_vblank)(struct drm_crtc *crtc);
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 9a4190c..445b851 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -51,15 +51,13 @@
 #define LDB_BGREF_RMODE_INT		(1 << 15)
 
 #define con_to_imx_ldb_ch(x) container_of(x, struct imx_ldb_channel, connector)
-#define imx_enc_to_imx_ldb_ch(x)	\
-			container_of(x, struct imx_ldb_channel, imx_encoder)
 
 struct imx_ldb;
 
 struct imx_ldb_channel {
 	struct imx_ldb *ldb;
 	struct drm_connector connector;
-	struct imx_drm_encoder imx_encoder;
+	struct drm_encoder encoder;
 	struct drm_panel *panel;
 	struct device_node *child;
 	struct i2c_adapter *ddc;
@@ -68,8 +66,14 @@ struct imx_ldb_channel {
 	int edid_len;
 	struct drm_display_mode mode;
 	int mode_valid;
+	u32 bus_format;
 };
 
+static inline struct imx_ldb_channel *enc_to_imx_ldb_ch(struct drm_encoder *e)
+{
+	return container_of(e, struct imx_ldb_channel, encoder);
+}
+
 struct bus_mux {
 	int reg;
 	int shift;
@@ -104,8 +108,9 @@ static int imx_ldb_connector_get_modes(struct drm_connector *connector)
 		struct drm_display_info *di = &connector->display_info;
 
 		num_modes = imx_ldb_ch->panel->funcs->get_modes(imx_ldb_ch->panel);
-		if (!imx_ldb_ch->imx_encoder.bus_format && di->num_bus_formats)
-			imx_ldb_ch->imx_encoder.bus_format = di->bus_formats[0];
+		/* Store bus format for imx_ldb_encoder_enable */
+		if (!imx_ldb_ch->bus_format && di->num_bus_formats)
+			imx_ldb_ch->bus_format = di->bus_formats[0];
 		if (num_modes > 0)
 			return num_modes;
 	}
@@ -139,7 +144,7 @@ static struct drm_encoder *imx_ldb_connector_best_encoder(
 {
 	struct imx_ldb_channel *imx_ldb_ch = con_to_imx_ldb_ch(connector);
 
-	return &imx_ldb_ch->imx_encoder.encoder;
+	return &imx_ldb_ch->encoder;
 }
 
 static void imx_ldb_set_clock(struct imx_ldb *ldb, int mux, int chno,
@@ -172,8 +177,7 @@ static void imx_ldb_set_clock(struct imx_ldb *ldb, int mux, int chno,
 
 static void imx_ldb_encoder_enable(struct drm_encoder *encoder)
 {
-	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
-	struct imx_ldb_channel *imx_ldb_ch = imx_enc_to_imx_ldb_ch(imx_encoder);
+	struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
 	struct imx_ldb *ldb = imx_ldb_ch->ldb;
 	int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
 	int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
@@ -226,8 +230,7 @@ static void imx_ldb_encoder_mode_set(struct drm_encoder *encoder,
 			 struct drm_display_mode *orig_mode,
 			 struct drm_display_mode *mode)
 {
-	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
-	struct imx_ldb_channel *imx_ldb_ch = imx_enc_to_imx_ldb_ch(imx_encoder);
+	struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
 	struct imx_ldb *ldb = imx_ldb_ch->ldb;
 	int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
 	unsigned long serial_clk;
@@ -254,24 +257,53 @@ static void imx_ldb_encoder_mode_set(struct drm_encoder *encoder,
 	}
 
 	/* FIXME - assumes straight connections DI0 --> CH0, DI1 --> CH1 */
-	if (imx_ldb_ch == &ldb->channel[0]) {
+	if (imx_ldb_ch == &ldb->channel[0] || dual) {
 		if (mode->flags & DRM_MODE_FLAG_NVSYNC)
 			ldb->ldb_ctrl |= LDB_DI0_VS_POL_ACT_LOW;
 		else if (mode->flags & DRM_MODE_FLAG_PVSYNC)
 			ldb->ldb_ctrl &= ~LDB_DI0_VS_POL_ACT_LOW;
+
+		switch (imx_ldb_ch->bus_format) {
+		case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
+			ldb->ldb_ctrl &= ~(LDB_DATA_WIDTH_CH0_24 |
+					   LDB_BIT_MAP_CH0_JEIDA);
+			break;
+		case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
+			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH0_24;
+			ldb->ldb_ctrl &= ~LDB_BIT_MAP_CH0_JEIDA;
+			break;
+		case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
+			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH0_24 |
+					 LDB_BIT_MAP_CH0_JEIDA;
+			break;
+		}
 	}
-	if (imx_ldb_ch == &ldb->channel[1]) {
+	if (imx_ldb_ch == &ldb->channel[1] || dual) {
 		if (mode->flags & DRM_MODE_FLAG_NVSYNC)
 			ldb->ldb_ctrl |= LDB_DI1_VS_POL_ACT_LOW;
 		else if (mode->flags & DRM_MODE_FLAG_PVSYNC)
 			ldb->ldb_ctrl &= ~LDB_DI1_VS_POL_ACT_LOW;
+
+		switch (imx_ldb_ch->bus_format) {
+		case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
+			ldb->ldb_ctrl &= ~(LDB_DATA_WIDTH_CH1_24 |
+					   LDB_BIT_MAP_CH1_JEIDA);
+			break;
+		case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
+			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH1_24;
+			ldb->ldb_ctrl &= ~LDB_BIT_MAP_CH1_JEIDA;
+			break;
+		case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
+			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH1_24 |
+					 LDB_BIT_MAP_CH1_JEIDA;
+			break;
+		}
 	}
 }
 
 static void imx_ldb_encoder_disable(struct drm_encoder *encoder)
 {
-	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
-	struct imx_ldb_channel *imx_ldb_ch = imx_enc_to_imx_ldb_ch(imx_encoder);
+	struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
 	struct imx_ldb *ldb = imx_ldb_ch->ldb;
 	int mux, ret;
 
@@ -326,6 +358,37 @@ static void imx_ldb_encoder_disable(struct drm_encoder *encoder)
 	drm_panel_unprepare(imx_ldb_ch->panel);
 }
 
+static int imx_ldb_encoder_atomic_check(struct drm_encoder *encoder,
+					struct drm_crtc_state *crtc_state,
+					struct drm_connector_state *conn_state)
+{
+	struct imx_crtc_state *imx_crtc_state = to_imx_crtc_state(crtc_state);
+	struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
+	struct drm_display_info *di = &conn_state->connector->display_info;
+	u32 bus_format = imx_ldb_ch->bus_format;
+
+	/* LVDS bus format description in DT overrides panel display info. */
+	if (!bus_format && di->num_bus_formats)
+		bus_format = di->bus_formats[0];
+	switch (bus_format) {
+	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
+		imx_crtc_state->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
+		break;
+	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
+	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
+		imx_crtc_state->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	imx_crtc_state->di_hsync_pin = 2;
+	imx_crtc_state->di_hsync_pin = 3;
+
+	return 0;
+}
+
+
 static const struct drm_connector_funcs imx_ldb_connector_funcs = {
 	.dpms = drm_atomic_helper_connector_dpms,
 	.fill_modes = drm_helper_probe_single_connector_modes,
@@ -349,6 +412,7 @@ static const struct drm_encoder_helper_funcs imx_ldb_encoder_helper_funcs = {
 	.mode_set = imx_ldb_encoder_mode_set,
 	.enable = imx_ldb_encoder_enable,
 	.disable = imx_ldb_encoder_disable,
+	.atomic_check = imx_ldb_encoder_atomic_check,
 };
 
 static int imx_ldb_get_clk(struct imx_ldb *ldb, int chno)
@@ -370,10 +434,10 @@ static int imx_ldb_register(struct drm_device *drm,
 	struct imx_ldb_channel *imx_ldb_ch)
 {
 	struct imx_ldb *ldb = imx_ldb_ch->ldb;
+	struct drm_encoder *encoder = &imx_ldb_ch->encoder;
 	int ret;
 
-	ret = imx_drm_encoder_parse_of(drm, &imx_ldb_ch->imx_encoder.encoder,
-				       imx_ldb_ch->child);
+	ret = imx_drm_encoder_parse_of(drm, encoder, imx_ldb_ch->child);
 	if (ret)
 		return ret;
 
@@ -387,10 +451,9 @@ static int imx_ldb_register(struct drm_device *drm,
 			return ret;
 	}
 
-	drm_encoder_helper_add(&imx_ldb_ch->imx_encoder.encoder,
-			&imx_ldb_encoder_helper_funcs);
-	drm_encoder_init(drm, &imx_ldb_ch->imx_encoder.encoder,
-			 &imx_ldb_encoder_funcs, DRM_MODE_ENCODER_LVDS, NULL);
+	drm_encoder_helper_add(encoder, &imx_ldb_encoder_helper_funcs);
+	drm_encoder_init(drm, encoder, &imx_ldb_encoder_funcs,
+			 DRM_MODE_ENCODER_LVDS, NULL);
 
 	drm_connector_helper_add(&imx_ldb_ch->connector,
 			&imx_ldb_connector_helper_funcs);
@@ -400,8 +463,7 @@ static int imx_ldb_register(struct drm_device *drm,
 	if (imx_ldb_ch->panel)
 		drm_panel_attach(imx_ldb_ch->panel, &imx_ldb_ch->connector);
 
-	drm_mode_connector_attach_encoder(&imx_ldb_ch->connector,
-			&imx_ldb_ch->imx_encoder.encoder);
+	drm_mode_connector_attach_encoder(&imx_ldb_ch->connector, encoder);
 
 	return 0;
 }
@@ -618,31 +680,7 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 				bus_format);
 			return bus_format;
 		}
-		switch (bus_format) {
-		case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
-			bus_format = MEDIA_BUS_FMT_RGB666_1X18;
-			break;
-		case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
-			bus_format = MEDIA_BUS_FMT_RGB888_1X24;
-			if (i == 0 || dual)
-				imx_ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH0_24;
-			if (i == 1 || dual)
-				imx_ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH1_24;
-			break;
-		case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
-			bus_format = MEDIA_BUS_FMT_RGB888_1X24;
-			if (i == 0 || dual)
-				imx_ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH0_24 |
-						     LDB_BIT_MAP_CH0_JEIDA;
-			if (i == 1 || dual)
-				imx_ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH1_24 |
-						     LDB_BIT_MAP_CH1_JEIDA;
-			break;
-		}
-		channel->imx_encoder.bus_format = bus_format;
-
-		channel->imx_encoder.di_hsync_pin = 2;
-		channel->imx_encoder.di_vsync_pin = 3;
+		channel->bus_format = bus_format;
 
 		ret = imx_ldb_register(drm, channel);
 		if (ret)
@@ -667,8 +705,7 @@ static void imx_ldb_unbind(struct device *dev, struct device *master,
 			continue;
 
 		channel->connector.funcs->destroy(&channel->connector);
-		channel->imx_encoder.encoder.funcs->destroy(
-					&channel->imx_encoder.encoder);
+		channel->encoder.funcs->destroy(&channel->encoder);
 
 		kfree(channel->edid);
 		i2c_put_adapter(channel->ddc);
diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index cd92aac..d8512c0 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -99,7 +99,6 @@
 #define TVE_TVDAC_TEST_MODE_MASK	(0x7 << 0)
 
 #define con_to_tve(x) container_of(x, struct imx_tve, connector)
-#define imx_enc_to_tve(x) container_of(x, struct imx_tve, imx_encoder)
 
 enum {
 	TVE_MODE_TVOUT,
@@ -113,6 +112,8 @@ struct imx_tve {
 	spinlock_t lock;	/* register lock */
 	bool enabled;
 	int mode;
+	int di_hsync_pin;
+	int di_vsync_pin;
 
 	struct regmap *regmap;
 	struct regulator *dac_reg;
@@ -123,6 +124,11 @@ struct imx_tve {
 	struct clk *di_clk;
 };
 
+static inline struct imx_tve *enc_to_tve(struct drm_encoder *e)
+{
+	return container_of(e, struct imx_tve, imx_encoder.encoder)
+}
+
 static void tve_lock(void *__tve)
 __acquires(&tve->lock)
 {
@@ -277,8 +283,7 @@ static void imx_tve_encoder_mode_set(struct drm_encoder *encoder,
 				     struct drm_display_mode *orig_mode,
 				     struct drm_display_mode *mode)
 {
-	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
-	struct imx_tve *tve = imx_enc_to_tve(imx_encoder);
+	struct imx_tve *tve = enc_to_tve(encoder);
 	unsigned long rounded_rate;
 	unsigned long rate;
 	int div = 1;
@@ -315,20 +320,32 @@ static void imx_tve_encoder_mode_set(struct drm_encoder *encoder,
 
 static void imx_tve_encoder_enable(struct drm_encoder *encoder)
 {
-	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
-	struct imx_tve *tve = imx_enc_to_tve(imx_encoder);
+	struct imx_tve *tve = enc_to_tve(encoder);
 
 	tve_enable(tve);
 }
 
 static void imx_tve_encoder_disable(struct drm_encoder *encoder)
 {
-	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
-	struct imx_tve *tve = imx_enc_to_tve(imx_encoder);
+	struct imx_tve *tve = enc_to_tve(encoder);
 
 	tve_disable(tve);
 }
 
+static int imx_tve_atomic_check(struct drm_encoder *encoder,
+				struct drm_crtc_state *crtc_state,
+				struct drm_connector_state *conn_state)
+{
+	struct imx_crtc_state *imx_crtc_state = to_imx_crtc_state(crtc_state);
+	struct imx_tve *tve = enc_to_tve(encoder);
+
+	imx_crtc_state->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
+	imx_crtc_state->di_hsync_pin = tve->di_hsync_pin;
+	imx_crtc_state->di_vsync_pin = tve->di_vsync_pin;
+
+	return 0;
+}
+
 static const struct drm_connector_funcs imx_tve_connector_funcs = {
 	.dpms = drm_atomic_helper_connector_dpms,
 	.fill_modes = drm_helper_probe_single_connector_modes,
@@ -353,6 +370,7 @@ static const struct drm_encoder_helper_funcs imx_tve_encoder_helper_funcs = {
 	.mode_set = imx_tve_encoder_mode_set,
 	.enable = imx_tve_encoder_enable,
 	.disable = imx_tve_encoder_disable,
+	.atomic_check = imx_tve_atomic_check,
 };
 
 static irqreturn_t imx_tve_irq_handler(int irq, void *data)
@@ -564,7 +582,7 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
 
 	if (tve->mode == TVE_MODE_VGA) {
 		ret = of_property_read_u32(np, "fsl,hsync-pin",
-					   &tve->imx_encoder.di_hsync_pin);
+					   &tve->di_hsync_pin);
 
 		if (ret < 0) {
 			dev_err(dev, "failed to get vsync pin\n");
@@ -572,14 +590,12 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
 		}
 
 		ret |= of_property_read_u32(np, "fsl,vsync-pin",
-					    &tve->imx_encoder.di_vsync_pin);
+					    &tve->di_vsync_pin);
 
 		if (ret < 0) {
 			dev_err(dev, "failed to get vsync pin\n");
 			return ret;
 		}
-
-		tve->imx_encoder.bus_format = MEDIA_BUS_FMT_GBR888_1X24;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index 274b0e2..2791ef0 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -75,13 +75,56 @@ static void ipu_crtc_disable(struct drm_crtc *crtc)
 	spin_unlock_irq(&crtc->dev->event_lock);
 }
 
+static void imx_drm_crtc_reset(struct drm_crtc *crtc)
+{
+	struct imx_crtc_state *state;
+
+	if (crtc->state) {
+		if (crtc->state->mode_blob)
+			drm_property_unreference_blob(crtc->state->mode_blob);
+
+		state = to_imx_crtc_state(crtc->state);
+		memset(state, 0, sizeof(*state));
+	} else {
+		state = kzalloc(sizeof(*state), GFP_KERNEL);
+		if (!state)
+			return;
+		crtc->state = &state->base;
+	}
+
+	state->base.crtc = crtc;
+}
+
+static struct drm_crtc_state *imx_drm_crtc_duplicate_state(struct drm_crtc *crtc)
+{
+	struct imx_crtc_state *state;
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return NULL;
+
+	__drm_atomic_helper_crtc_duplicate_state(crtc, &state->base);
+
+	WARN_ON(state->base.crtc != crtc);
+	state->base.crtc = crtc;
+
+	return &state->base;
+}
+
+static void imx_drm_crtc_destroy_state(struct drm_crtc *crtc,
+				       struct drm_crtc_state *state)
+{
+	__drm_atomic_helper_crtc_destroy_state(state);
+	kfree(to_imx_crtc_state(state));
+}
+
 static const struct drm_crtc_funcs ipu_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
 	.destroy = drm_crtc_cleanup,
 	.page_flip = drm_atomic_helper_page_flip,
-	.reset = drm_atomic_helper_crtc_reset,
-	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+	.reset = imx_drm_crtc_reset,
+	.atomic_duplicate_state = imx_drm_crtc_duplicate_state,
+	.atomic_destroy_state = imx_drm_crtc_destroy_state,
 };
 
 static irqreturn_t ipu_irq_handler(int irq, void *dev_id)
@@ -142,9 +185,9 @@ static void ipu_crtc_mode_set_nofb(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_encoder *encoder;
-	struct imx_drm_encoder *imx_encoder = NULL;
 	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
 	struct drm_display_mode *mode = &crtc->state->adjusted_mode;
+	struct imx_crtc_state *imx_crtc_state = to_imx_crtc_state(crtc->state);
 	struct ipu_di_signal_cfg sig_cfg = {};
 	unsigned long encoder_types = 0;
 
@@ -154,10 +197,8 @@ static void ipu_crtc_mode_set_nofb(struct drm_crtc *crtc)
 			mode->vdisplay);
 
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
-		if (encoder->crtc == crtc) {
+		if (encoder->crtc == crtc)
 			encoder_types |= BIT(encoder->encoder_type);
-			imx_encoder = enc_to_imx_enc(encoder);
-		}
 	}
 
 	dev_dbg(ipu_crtc->dev, "%s: attached to encoder types 0x%lx\n",
@@ -176,20 +217,20 @@ static void ipu_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	else
 		sig_cfg.clkflags = 0;
 
-	sig_cfg.enable_pol = !(imx_encoder->bus_flags & DRM_BUS_FLAG_DE_LOW);
+	sig_cfg.enable_pol = !(imx_crtc_state->bus_flags & DRM_BUS_FLAG_DE_LOW);
 	/* Default to driving pixel data on negative clock edges */
-	sig_cfg.clk_pol = !!(imx_encoder->bus_flags &
+	sig_cfg.clk_pol = !!(imx_crtc_state->bus_flags &
 			     DRM_BUS_FLAG_PIXDATA_POSEDGE);
-	sig_cfg.bus_format = imx_encoder->bus_format;
+	sig_cfg.bus_format = imx_crtc_state->bus_format;
 	sig_cfg.v_to_h_sync = 0;
-	sig_cfg.hsync_pin = imx_encoder->di_hsync_pin;
-	sig_cfg.vsync_pin = imx_encoder->di_vsync_pin;
+	sig_cfg.hsync_pin = imx_crtc_state->di_hsync_pin;
+	sig_cfg.vsync_pin = imx_crtc_state->di_vsync_pin;
 
 	drm_display_mode_to_videomode(mode, &sig_cfg.mode);
 
 	ipu_dc_init_sync(ipu_crtc->dc, ipu_crtc->di,
 			 mode->flags & DRM_MODE_FLAG_INTERLACE,
-			 imx_encoder->bus_format, mode->hdisplay);
+			 imx_crtc_state->bus_format, mode->hdisplay);
 	ipu_di_init_sync_panel(ipu_crtc->di, &sig_cfg);
 }
 
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 4a2942e..26c3482 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -27,19 +27,23 @@
 #include "imx-drm.h"
 
 #define con_to_imxpd(x) container_of(x, struct imx_parallel_display, connector)
-#define imx_enc_to_imxpd(x)	\
-		container_of(x, struct imx_parallel_display, imx_encoder)
 
 struct imx_parallel_display {
 	struct drm_connector connector;
-	struct imx_drm_encoder imx_encoder;
+	struct drm_encoder encoder;
 	struct device *dev;
 	void *edid;
 	int edid_len;
+	u32 bus_format;
 	struct drm_display_mode mode;
 	struct drm_panel *panel;
 };
 
+static inline struct imx_parallel_display *enc_to_imxpd(struct drm_encoder *e)
+{
+	return container_of(e, struct imx_parallel_display, encoder);
+}
+
 static enum drm_connector_status imx_pd_connector_detect(
 		struct drm_connector *connector, bool force)
 {
@@ -54,12 +58,7 @@ static int imx_pd_connector_get_modes(struct drm_connector *connector)
 
 	if (imxpd->panel && imxpd->panel->funcs &&
 	    imxpd->panel->funcs->get_modes) {
-		struct drm_display_info *di = &connector->display_info;
-
 		num_modes = imxpd->panel->funcs->get_modes(imxpd->panel);
-		if (!imxpd->imx_encoder.bus_format && di->num_bus_formats)
-			imxpd->imx_encoder.bus_format = di->bus_formats[0];
-		imxpd->imx_encoder.bus_flags = di->bus_flags;
 		if (num_modes > 0)
 			return num_modes;
 	}
@@ -89,13 +88,12 @@ static struct drm_encoder *imx_pd_connector_best_encoder(
 {
 	struct imx_parallel_display *imxpd = con_to_imxpd(connector);
 
-	return &imxpd->imx_encoder.encoder;
+	return &imxpd->encoder;
 }
 
 static void imx_pd_encoder_enable(struct drm_encoder *encoder)
 {
-	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
-	struct imx_parallel_display *imxpd = imx_enc_to_imxpd(imx_encoder);
+	struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
 
 	drm_panel_prepare(imxpd->panel);
 	drm_panel_enable(imxpd->panel);
@@ -103,13 +101,31 @@ static void imx_pd_encoder_enable(struct drm_encoder *encoder)
 
 static void imx_pd_encoder_disable(struct drm_encoder *encoder)
 {
-	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
-	struct imx_parallel_display *imxpd = imx_enc_to_imxpd(imx_encoder);
+	struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
 
 	drm_panel_disable(imxpd->panel);
 	drm_panel_unprepare(imxpd->panel);
 }
 
+static int imx_pd_encoder_atomic_check(struct drm_encoder *encoder,
+				       struct drm_crtc_state *crtc_state,
+				       struct drm_connector_state *conn_state)
+{
+	struct imx_crtc_state *imx_crtc_state = to_imx_crtc_state(crtc_state);
+	struct drm_display_info *di = &conn_state->connector->display_info;
+	struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
+
+	imx_crtc_state->bus_flags = di->bus_flags;
+	if (!imxpd->bus_format && di->num_bus_formats)
+		imx_crtc_state->bus_format = di->bus_formats[0];
+	else
+		imx_crtc_state->bus_format = imxpd->bus_format;
+	imx_crtc_state->di_hsync_pin = 2;
+	imx_crtc_state->di_vsync_pin = 3;
+
+	return 0;
+}
+
 static const struct drm_connector_funcs imx_pd_connector_funcs = {
 	.dpms = drm_atomic_helper_connector_dpms,
 	.fill_modes = drm_helper_probe_single_connector_modes,
@@ -132,15 +148,16 @@ static const struct drm_encoder_funcs imx_pd_encoder_funcs = {
 static const struct drm_encoder_helper_funcs imx_pd_encoder_helper_funcs = {
 	.enable = imx_pd_encoder_enable,
 	.disable = imx_pd_encoder_disable,
+	.atomic_check = imx_pd_encoder_atomic_check,
 };
 
 static int imx_pd_register(struct drm_device *drm,
 	struct imx_parallel_display *imxpd)
 {
+	struct drm_encoder *encoder = &imxpd->encoder;
 	int ret;
 
-	ret = imx_drm_encoder_parse_of(drm, &imxpd->imx_encoder.encoder,
-				       imxpd->dev->of_node);
+	ret = imx_drm_encoder_parse_of(drm, encoder, imxpd->dev->of_node);
 	if (ret)
 		return ret;
 
@@ -151,10 +168,9 @@ static int imx_pd_register(struct drm_device *drm,
 	 */
 	imxpd->connector.dpms = DRM_MODE_DPMS_OFF;
 
-	drm_encoder_helper_add(&imxpd->imx_encoder.encoder,
-			       &imx_pd_encoder_helper_funcs);
-	drm_encoder_init(drm, &imxpd->imx_encoder.encoder,
-			 &imx_pd_encoder_funcs, DRM_MODE_ENCODER_NONE, NULL);
+	drm_encoder_helper_add(encoder, &imx_pd_encoder_helper_funcs);
+	drm_encoder_init(drm, encoder, &imx_pd_encoder_funcs,
+			 DRM_MODE_ENCODER_NONE, NULL);
 
 	drm_connector_helper_add(&imxpd->connector,
 			&imx_pd_connector_helper_funcs);
@@ -164,8 +180,7 @@ static int imx_pd_register(struct drm_device *drm,
 	if (imxpd->panel)
 		drm_panel_attach(imxpd->panel, &imxpd->connector);
 
-	drm_mode_connector_attach_encoder(&imxpd->connector,
-					  &imxpd->imx_encoder.encoder);
+	drm_mode_connector_attach_encoder(&imxpd->connector, encoder);
 
 	return 0;
 }
@@ -178,7 +193,6 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 	const u8 *edidp;
 	struct imx_parallel_display *imxpd;
 	int ret;
-	u32 bus_format = 0;
 	const char *fmt;
 
 	imxpd = devm_kzalloc(dev, sizeof(*imxpd), GFP_KERNEL);
@@ -192,17 +206,14 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 	ret = of_property_read_string(np, "interface-pix-fmt", &fmt);
 	if (!ret) {
 		if (!strcmp(fmt, "rgb24"))
-			bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+			imxpd->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
 		else if (!strcmp(fmt, "rgb565"))
-			bus_format = MEDIA_BUS_FMT_RGB565_1X16;
+			imxpd->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
 		else if (!strcmp(fmt, "bgr666"))
-			bus_format = MEDIA_BUS_FMT_RGB666_1X18;
+			imxpd->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
 		else if (!strcmp(fmt, "lvds666"))
-			bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
+			imxpd->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
 	}
-	imxpd->imx_encoder.bus_format = bus_format;
-	imxpd->imx_encoder.di_hsync_pin = 2;
-	imxpd->imx_encoder.di_vsync_pin = 3;
 
 	/* port@1 is the output port */
 	ep = of_graph_get_endpoint_by_regs(np, 1, -1);
@@ -235,7 +246,7 @@ static void imx_pd_unbind(struct device *dev, struct device *master,
 {
 	struct imx_parallel_display *imxpd = dev_get_drvdata(dev);
 
-	imxpd->imx_encoder.encoder.funcs->destroy(&imxpd->imx_encoder.encoder);
+	imxpd->encoder.funcs->destroy(&imxpd->encoder);
 	imxpd->connector.funcs->destroy(&imxpd->connector);
 
 	kfree(imxpd->edid);
-- 
2.8.1

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

[PATCH 3/3] drm/imx: turn remaining container_of macros into inline functions

[Philipp Zabel]

This allows the compiler to do type checking.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-ldb.c          | 7 +++++--
 drivers/gpu/drm/imx/imx-tve.c          | 7 +++++--
 drivers/gpu/drm/imx/ipuv3-crtc.c       | 5 ++++-
 drivers/gpu/drm/imx/ipuv3-plane.c      | 5 ++++-
 drivers/gpu/drm/imx/parallel-display.c | 7 +++++--
 5 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 445b851..e04c22e 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -50,8 +50,6 @@
 #define LDB_DI1_VS_POL_ACT_LOW		(1 << 10)
 #define LDB_BGREF_RMODE_INT		(1 << 15)
 
-#define con_to_imx_ldb_ch(x) container_of(x, struct imx_ldb_channel, connector)
-
 struct imx_ldb;
 
 struct imx_ldb_channel {
@@ -69,6 +67,11 @@ struct imx_ldb_channel {
 	u32 bus_format;
 };
 
+static inline struct imx_ldb_channel *con_to_imx_ldb_ch(struct drm_connector *c)
+{
+	return container_of(c, struct imx_ldb_channel, connector);
+}
+
 static inline struct imx_ldb_channel *enc_to_imx_ldb_ch(struct drm_encoder *e)
 {
 	return container_of(e, struct imx_ldb_channel, encoder);
diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index d8512c0..4c8ca09 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -98,8 +98,6 @@
 /* TVE_TST_MODE_REG */
 #define TVE_TVDAC_TEST_MODE_MASK	(0x7 << 0)
 
-#define con_to_tve(x) container_of(x, struct imx_tve, connector)
-
 enum {
 	TVE_MODE_TVOUT,
 	TVE_MODE_VGA,
@@ -124,6 +122,11 @@ struct imx_tve {
 	struct clk *di_clk;
 };
 
+static inline struct imx_tve *con_to_tve(struct drm_connector *c)
+{
+	return container_of(c, struct imx_tve, connector);
+}
+
 static inline struct imx_tve *enc_to_tve(struct drm_encoder *e)
 {
 	return container_of(e, struct imx_tve, imx_encoder.encoder)
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index 2791ef0..08e188b 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -46,7 +46,10 @@ struct ipu_crtc {
 	int			irq;
 };
 
-#define to_ipu_crtc(x) container_of(x, struct ipu_crtc, base)
+static inline struct ipu_crtc *to_ipu_crtc(struct drm_crtc *crtc)
+{
+	return container_of(crtc, struct ipu_crtc, base);
+}
 
 static void ipu_crtc_enable(struct drm_crtc *crtc)
 {
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 164333b..55549f9 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -23,7 +23,10 @@
 #include "video/imx-ipu-v3.h"
 #include "ipuv3-plane.h"
 
-#define to_ipu_plane(x)	container_of(x, struct ipu_plane, base)
+static inline struct ipu_plane *to_ipu_plane(struct drm_plane *p)
+{
+	return container_of(p, struct ipu_plane, base);
+}
 
 static const uint32_t ipu_plane_formats[] = {
 	DRM_FORMAT_ARGB1555,
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 26c3482..ccdda8a 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -26,8 +26,6 @@
 
 #include "imx-drm.h"
 
-#define con_to_imxpd(x) container_of(x, struct imx_parallel_display, connector)
-
 struct imx_parallel_display {
 	struct drm_connector connector;
 	struct drm_encoder encoder;
@@ -39,6 +37,11 @@ struct imx_parallel_display {
 	struct drm_panel *panel;
 };
 
+static inline struct imx_parallel_display *con_to_imxpd(struct drm_connector *c)
+{
+	return container_of(c, struct imx_parallel_display, connector);
+}
+
 static inline struct imx_parallel_display *enc_to_imxpd(struct drm_encoder *e)
 {
 	return container_of(e, struct imx_parallel_display, encoder);
-- 
2.8.1

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

Re: [PATCH 0/3] imx drm atomic mode setting fixups

[Ying Liu]

Hi Philipp,

On Wed, Jul 6, 2016 at 11:52 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi,
>
> I have tested Liu Ying's imx drm atomic mode setting conversion series [1]

Thanks for the testing!

> with three different outputs (DPI + eDP bridge, LVDS + panel, HDMI)
> and noticed that in the LDB case bus_format handling doesn't work
> correctly if the format is determined by the panel instead of the
> device tree. In that case, the LVDS bus_format is not yet known at bind
> time, but is filled into the display_info structure by the panel driver
> in the get_modes callback. The LDB driver then has to translate from the
> LVDS bus format to the corresponding internal parallel bus format and
> let the crtc know about the format before crtc mode_set is called.
> The same issue occurs when the bus_format is determined by a bridge driver
> which implements its own connector. To solve this, and because crtc state
> seems to be the right place for potentially dynamic crtc settings, I have
> moved the bus_format, bus_flags, and di_*sync_pin configuration into an
> imx-drm specific crtc state structure and removed imx_drm_encoder again.

I didn't program imx_ldb_ch->imx_encoder.bus_format correctly
in imx_ldb_connector_get_modes() - I forgot to translate the bus format
from LVDS bus format to internal bus format.  Also, data width and
LVDS data mapping(SPWG/JEIDA) can be set to ldb->ldb_ctrl in
the function. So, one option to fix the issue you found is to simply
modify the function and respin the particular patch in my patch set.
Actually, I have got a fix tested with the format determined by the panel.

I don't have strong opinion on storing bus_format, bus_flags and
di_*sync_pin in imx_crtc_state.  But, very likely, they will not be
dynamically changed.  Setting them again and again at atomic_check
is somewhat wasting CPU cycles...

BTW, the imx-drm atomic conversion patch set has reached version 3
which uses the new non-blocking atomic commit helpers.

Regards,
Liu Ying

>
> I have also removed the now optional empty encoder mode_set callbacks
> and turned the container_of helper macros into inline functions.
>
> [1] https://lists.freedesktop.org/archives/dri-devel/2016-May/108218.html
>
> regards
> Philipp
>
> Philipp Zabel (3):
>   drm/imx: remove empty mode_set encoder callbacks
>   drm/imx: store internal bus configuration in crtc state
>   drm/imx: turn remaining container_of macros into inline functions
>
>  drivers/gpu/drm/imx/dw_hdmi-imx.c      |  38 +++++----
>  drivers/gpu/drm/imx/imx-drm.h          |   9 ++-
>  drivers/gpu/drm/imx/imx-ldb.c          | 142 +++++++++++++++++++++------------
>  drivers/gpu/drm/imx/imx-tve.c          |  45 ++++++++---
>  drivers/gpu/drm/imx/ipuv3-crtc.c       |  72 +++++++++++++----
>  drivers/gpu/drm/imx/ipuv3-plane.c      |   5 +-
>  drivers/gpu/drm/imx/parallel-display.c |  85 +++++++++++---------
>  7 files changed, 259 insertions(+), 137 deletions(-)
>
> --
> 2.8.1
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] imx drm atomic mode setting fixups

[Philipp Zabel]

Hi Liu,

Am Donnerstag, den 07.07.2016, 15:44 +0800 schrieb Ying Liu:
> Hi Philipp,
> 
> On Wed, Jul 6, 2016 at 11:52 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > Hi,
> >
> > I have tested Liu Ying's imx drm atomic mode setting conversion series [1]
> 
> Thanks for the testing!
[...]
> I didn't program imx_ldb_ch->imx_encoder.bus_format correctly
> in imx_ldb_connector_get_modes() - I forgot to translate the bus format
> from LVDS bus format to internal bus format.  Also, data width and
> LVDS data mapping(SPWG/JEIDA) can be set to ldb->ldb_ctrl in
> the function. So, one option to fix the issue you found is to simply
> modify the function and respin the particular patch in my patch set.
> Actually, I have got a fix tested with the format determined by the panel.
> 
> I don't have strong opinion on storing bus_format, bus_flags and
> di_*sync_pin in imx_crtc_state.  But, very likely, they will not be
> dynamically changed.  Setting them again and again at atomic_check
> is somewhat wasting CPU cycles...

That really only should be a concern if you can measure the difference.
Further, with upcoming bridge support in the parallel-display and
imx-ldb drivers the encoder doesn't have direct access to the connector
anymore (get_modes is handled by the bridges' connector). I could also
think of bridges where the optimal input bus_format depends on the mode.
So I'd prefer the internal bus configuration to reside in imx_crtc_state
instead of imx_encoder.

> BTW, the imx-drm atomic conversion patch set has reached version 3
> which uses the new non-blocking atomic commit helpers.

Yes, thank you for the update. I chose the wrong link, I have tested
version 3. If you are willing to respin your series once more, feel free
to integrate all or part of my changes in the appropriate places (the
mode_set removal could be squashed into the "Legacy callback fixups"
patch, for example). I could then retest and potentially rebase the
remaining changes on your next version.

regards
Philipp

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

Re: [PATCH 0/3] imx drm atomic mode setting fixups

[Liu Ying]

Hi Philipp,

2016-07-07 16:29 GMT+08:00 Philipp Zabel <p.zabel@pengutronix.de>:
> Hi Liu,
>
> Am Donnerstag, den 07.07.2016, 15:44 +0800 schrieb Ying Liu:
>> Hi Philipp,
>>
>> On Wed, Jul 6, 2016 at 11:52 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>> > Hi,
>> >
>> > I have tested Liu Ying's imx drm atomic mode setting conversion series [1]
>>
>> Thanks for the testing!
> [...]
>> I didn't program imx_ldb_ch->imx_encoder.bus_format correctly
>> in imx_ldb_connector_get_modes() - I forgot to translate the bus format
>> from LVDS bus format to internal bus format.  Also, data width and
>> LVDS data mapping(SPWG/JEIDA) can be set to ldb->ldb_ctrl in
>> the function. So, one option to fix the issue you found is to simply
>> modify the function and respin the particular patch in my patch set.
>> Actually, I have got a fix tested with the format determined by the panel.
>>
>> I don't have strong opinion on storing bus_format, bus_flags and
>> di_*sync_pin in imx_crtc_state.  But, very likely, they will not be
>> dynamically changed.  Setting them again and again at atomic_check
>> is somewhat wasting CPU cycles...
>
> That really only should be a concern if you can measure the difference.
> Further, with upcoming bridge support in the parallel-display and
> imx-ldb drivers the encoder doesn't have direct access to the connector
> anymore (get_modes is handled by the bridges' connector). I could also
> think of bridges where the optimal input bus_format depends on the mode.
> So I'd prefer the internal bus configuration to reside in imx_crtc_state
> instead of imx_encoder.
>
>> BTW, the imx-drm atomic conversion patch set has reached version 3
>> which uses the new non-blocking atomic commit helpers.
>
> Yes, thank you for the update. I chose the wrong link, I have tested
> version 3. If you are willing to respin your series once more, feel free
> to integrate all or part of my changes in the appropriate places (the
> mode_set removal could be squashed into the "Legacy callback fixups"
> patch, for example). I could then retest and potentially rebase the
> remaining changes on your next version.

I'll respin to fix the LVDS bus format translation issue.
To keep my patch set as small as possible, I'll leave your 3 patches
for you to handle.  Please consider to rebase.

Regards,
Liu Ying

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

Re: [PATCH 0/3] imx drm atomic mode setting fixups

[Philipp Zabel]

Hi Liu,

Am Freitag, den 08.07.2016, 17:38 +0800 schrieb Liu Ying:
[...]
> I'll respin to fix the LVDS bus format translation issue.
> To keep my patch set as small as possible, I'll leave your 3 patches
> for you to handle.  Please consider to rebase.

Thanks, that works. I have rebased my changes on top:

git://git.pengutronix.de/git/pza/linux.git imx-drm/atomic

I plan to include this with my next pull request if there are no more
issues.

regards
Philipp

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

Re: [PATCH 0/3] imx drm atomic mode setting fixups

[Ying Liu]

Hi Philipp,

On Sat, Jul 9, 2016 at 1:24 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi Liu,
>
> Am Freitag, den 08.07.2016, 17:38 +0800 schrieb Liu Ying:
> [...]
>> I'll respin to fix the LVDS bus format translation issue.
>> To keep my patch set as small as possible, I'll leave your 3 patches
>> for you to handle.  Please consider to rebase.
>
> Thanks, that works. I have rebased my changes on top:
>
> git://git.pengutronix.de/git/pza/linux.git imx-drm/atomic

It looks there is build break issue on this branch for the imx-tve driver.
It seems that driver is not fully cleanup wrt imx_drm_encoder.

Regards,
Liu Ying

>
> I plan to include this with my next pull request if there are no more
> issues.
>
> regards
> Philipp
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] imx drm atomic mode setting fixups

[Philipp Zabel]

Am Montag, den 11.07.2016, 10:56 +0800 schrieb Ying Liu:
> Hi Philipp,
> 
> On Sat, Jul 9, 2016 at 1:24 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > Hi Liu,
> >
> > Am Freitag, den 08.07.2016, 17:38 +0800 schrieb Liu Ying:
> > [...]
> >> I'll respin to fix the LVDS bus format translation issue.
> >> To keep my patch set as small as possible, I'll leave your 3 patches
> >> for you to handle.  Please consider to rebase.
> >
> > Thanks, that works. I have rebased my changes on top:
> >
> > git://git.pengutronix.de/git/pza/linux.git imx-drm/atomic
> 
> It looks there is build break issue on this branch for the imx-tve driver.
> It seems that driver is not fully cleanup wrt imx_drm_encoder.

Thanks, it should be fixed now.

regards
Philipp

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