[PATCH v3 0/6] drm/ingenic: Various improvements v3

[PATCH v3 0/6] drm/ingenic: Various improvements v3

[Paul Cercueil]

Hi,

A V3 of my patchset for the ingenic-drm driver.

The patches "drm/ingenic: Remove dead code" and
"drm/ingenic: Use standard drm_atomic_helper_commit_tail"
that were present in V1 have been merged in drm-misc-next,
so they are not in this V3.

Changelog since V2:

[PATCH 5/6]:
    Fix ingenic_drm_get_new_priv_state() called instead of
    ingenic_drm_get_priv_state()

Cheers,
-Paul

Paul Cercueil (6):
  drm/ingenic: Simplify code by using hwdescs array
  drm/ingenic: Add support for private objects
  drm/ingenic: Move IPU scale settings to private state
  drm/ingenic: Set DMA descriptor chain register when starting CRTC
  drm/ingenic: Upload palette before frame
  drm/ingenic: Attach bridge chain to encoders

 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 278 +++++++++++++++++-----
 drivers/gpu/drm/ingenic/ingenic-ipu.c     | 127 ++++++++--
 2 files changed, 333 insertions(+), 72 deletions(-)

-- 
2.33.0

[PATCH v3 1/6] drm/ingenic: Simplify code by using hwdescs array

[Paul Cercueil]

Instead of having one 'hwdesc' variable for the plane #0, one for the
plane #1 and one for the palette, use a 'hwdesc[3]' array, where the
DMA hardware descriptors are indexed by the plane's number.

v2: dma_hwdesc_addr() extended to support palette hwdesc. The palette
    hwdesc is now hwdesc[3] to simplify things. Add
    ingenic_drm_configure_hwdesc*() functions to factorize code.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 78 ++++++++++++++---------
 1 file changed, 48 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index a5df1c8d34cd..95c12c2aba14 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -41,6 +41,8 @@
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_vblank.h>
 
+#define HWDESC_PALETTE 2
+
 struct ingenic_dma_hwdesc {
 	u32 next;
 	u32 addr;
@@ -49,9 +51,7 @@ struct ingenic_dma_hwdesc {
 } __aligned(16);
 
 struct ingenic_dma_hwdescs {
-	struct ingenic_dma_hwdesc hwdesc_f0;
-	struct ingenic_dma_hwdesc hwdesc_f1;
-	struct ingenic_dma_hwdesc hwdesc_pal;
+	struct ingenic_dma_hwdesc hwdesc[3];
 	u16 palette[256] __aligned(16);
 };
 
@@ -141,6 +141,14 @@ static inline struct ingenic_drm *drm_nb_get_priv(struct notifier_block *nb)
 	return container_of(nb, struct ingenic_drm, clock_nb);
 }
 
+static inline dma_addr_t dma_hwdesc_addr(const struct ingenic_drm *priv,
+					 unsigned int idx)
+{
+	u32 offset = offsetof(struct ingenic_dma_hwdescs, hwdesc[idx]);
+
+	return priv->dma_hwdescs_phys + offset;
+}
+
 static int ingenic_drm_update_pixclk(struct notifier_block *nb,
 				     unsigned long action,
 				     void *data)
@@ -558,9 +566,9 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
 	struct ingenic_drm *priv = drm_device_get_priv(plane->dev);
 	struct drm_plane_state *newstate = drm_atomic_get_new_plane_state(state, plane);
 	struct drm_plane_state *oldstate = drm_atomic_get_old_plane_state(state, plane);
+	unsigned int width, height, cpp, next_id, plane_id;
 	struct drm_crtc_state *crtc_state;
 	struct ingenic_dma_hwdesc *hwdesc;
-	unsigned int width, height, cpp, offset;
 	dma_addr_t addr;
 	u32 fourcc;
 
@@ -569,16 +577,14 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
 			drm_fb_cma_sync_non_coherent(&priv->drm, oldstate, newstate);
 
 		crtc_state = newstate->crtc->state;
+		plane_id = !!(priv->soc_info->has_osd && plane != &priv->f0);
 
 		addr = drm_fb_cma_get_gem_addr(newstate->fb, newstate, 0);
 		width = newstate->src_w >> 16;
 		height = newstate->src_h >> 16;
 		cpp = newstate->fb->format->cpp[0];
 
-		if (!priv->soc_info->has_osd || plane == &priv->f0)
-			hwdesc = &priv->dma_hwdescs->hwdesc_f0;
-		else
-			hwdesc = &priv->dma_hwdescs->hwdesc_f1;
+		hwdesc = &priv->dma_hwdescs->hwdesc[plane_id];
 
 		hwdesc->addr = addr;
 		hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
@@ -588,12 +594,8 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
 
 			ingenic_drm_plane_config(priv->dev, plane, fourcc);
 
-			if (fourcc == DRM_FORMAT_C8)
-				offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_pal);
-			else
-				offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
-
-			priv->dma_hwdescs->hwdesc_f0.next = priv->dma_hwdescs_phys + offset;
+			next_id = fourcc == DRM_FORMAT_C8 ? HWDESC_PALETTE : 0;
+			priv->dma_hwdescs->hwdesc[0].next = dma_hwdesc_addr(priv, next_id);
 
 			crtc_state->color_mgmt_changed = fourcc == DRM_FORMAT_C8;
 		}
@@ -846,6 +848,35 @@ static void __maybe_unused ingenic_drm_release_rmem(void *d)
 	of_reserved_mem_device_release(d);
 }
 
+static void ingenic_drm_configure_hwdesc(struct ingenic_drm *priv,
+					 unsigned int hwdesc,
+					 unsigned int next_hwdesc, u32 id)
+{
+	struct ingenic_dma_hwdesc *desc = &priv->dma_hwdescs->hwdesc[hwdesc];
+
+	desc->next = dma_hwdesc_addr(priv, next_hwdesc);
+	desc->id = id;
+}
+
+static void ingenic_drm_configure_hwdesc_palette(struct ingenic_drm *priv)
+{
+	struct ingenic_dma_hwdesc *desc;
+
+	ingenic_drm_configure_hwdesc(priv, HWDESC_PALETTE, 0, 0xc0);
+
+	desc = &priv->dma_hwdescs->hwdesc[HWDESC_PALETTE];
+	desc->addr = priv->dma_hwdescs_phys
+		+ offsetof(struct ingenic_dma_hwdescs, palette);
+	desc->cmd = JZ_LCD_CMD_ENABLE_PAL
+		| (sizeof(priv->dma_hwdescs->palette) / 4);
+}
+
+static void ingenic_drm_configure_hwdesc_plane(struct ingenic_drm *priv,
+					       unsigned int plane)
+{
+	ingenic_drm_configure_hwdesc(priv, plane, plane, 0xf0 | plane);
+}
+
 static int ingenic_drm_bind(struct device *dev, bool has_components)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -942,27 +973,14 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 	if (!priv->dma_hwdescs)
 		return -ENOMEM;
 
-
 	/* Configure DMA hwdesc for foreground0 plane */
-	dma_hwdesc_phys_f0 = priv->dma_hwdescs_phys
-		+ offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
-	priv->dma_hwdescs->hwdesc_f0.next = dma_hwdesc_phys_f0;
-	priv->dma_hwdescs->hwdesc_f0.id = 0xf0;
+	ingenic_drm_configure_hwdesc_plane(priv, 0);
 
 	/* Configure DMA hwdesc for foreground1 plane */
-	dma_hwdesc_phys_f1 = priv->dma_hwdescs_phys
-		+ offsetof(struct ingenic_dma_hwdescs, hwdesc_f1);
-	priv->dma_hwdescs->hwdesc_f1.next = dma_hwdesc_phys_f1;
-	priv->dma_hwdescs->hwdesc_f1.id = 0xf1;
+	ingenic_drm_configure_hwdesc_plane(priv, 1);
 
 	/* Configure DMA hwdesc for palette */
-	priv->dma_hwdescs->hwdesc_pal.next = priv->dma_hwdescs_phys
-		+ offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
-	priv->dma_hwdescs->hwdesc_pal.id = 0xc0;
-	priv->dma_hwdescs->hwdesc_pal.addr = priv->dma_hwdescs_phys
-		+ offsetof(struct ingenic_dma_hwdescs, palette);
-	priv->dma_hwdescs->hwdesc_pal.cmd = JZ_LCD_CMD_ENABLE_PAL
-		| (sizeof(priv->dma_hwdescs->palette) / 4);
+	ingenic_drm_configure_hwdesc_palette(priv);
 
 	primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
 
-- 
2.33.0

[PATCH v3 2/6] drm/ingenic: Add support for private objects

[Paul Cercueil]

Until now, the ingenic-drm as well as the ingenic-ipu drivers used to
put state-specific information in their respective private structure.

Add boilerplate code to support private objects in the two drivers, so
that state-specific information can be put in the state-specific private
structure.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 61 +++++++++++++++++++++++
 drivers/gpu/drm/ingenic/ingenic-ipu.c     | 54 ++++++++++++++++++++
 2 files changed, 115 insertions(+)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 95c12c2aba14..5dbeca0f8f37 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -64,6 +64,10 @@ struct jz_soc_info {
 	unsigned int num_formats_f0, num_formats_f1;
 };
 
+struct ingenic_drm_private_state {
+	struct drm_private_state base;
+};
+
 struct ingenic_drm {
 	struct drm_device drm;
 	/*
@@ -99,8 +103,16 @@ struct ingenic_drm {
 	struct mutex clk_mutex;
 	bool update_clk_rate;
 	struct notifier_block clock_nb;
+
+	struct drm_private_obj private_obj;
 };
 
+static inline struct ingenic_drm_private_state *
+to_ingenic_drm_priv_state(struct drm_private_state *state)
+{
+	return container_of(state, struct ingenic_drm_private_state, base);
+}
+
 static bool ingenic_drm_writeable_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
@@ -766,6 +778,28 @@ ingenic_drm_gem_create_object(struct drm_device *drm, size_t size)
 	return &obj->base;
 }
 
+static struct drm_private_state *
+ingenic_drm_duplicate_state(struct drm_private_obj *obj)
+{
+	struct ingenic_drm_private_state *state = to_ingenic_drm_priv_state(obj->state);
+
+	state = kmemdup(state, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return NULL;
+
+	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
+
+	return &state->base;
+}
+
+static void ingenic_drm_destroy_state(struct drm_private_obj *obj,
+				      struct drm_private_state *state)
+{
+	struct ingenic_drm_private_state *priv_state = to_ingenic_drm_priv_state(state);
+
+	kfree(priv_state);
+}
+
 DEFINE_DRM_GEM_CMA_FOPS(ingenic_drm_fops);
 
 static const struct drm_driver ingenic_drm_driver_data = {
@@ -836,6 +870,11 @@ static struct drm_mode_config_helper_funcs ingenic_drm_mode_config_helpers = {
 	.atomic_commit_tail = drm_atomic_helper_commit_tail,
 };
 
+static const struct drm_private_state_funcs ingenic_drm_private_state_funcs = {
+	.atomic_duplicate_state = ingenic_drm_duplicate_state,
+	.atomic_destroy_state = ingenic_drm_destroy_state,
+};
+
 static void ingenic_drm_unbind_all(void *d)
 {
 	struct ingenic_drm *priv = d;
@@ -877,9 +916,15 @@ static void ingenic_drm_configure_hwdesc_plane(struct ingenic_drm *priv,
 	ingenic_drm_configure_hwdesc(priv, plane, plane, 0xf0 | plane);
 }
 
+static void ingenic_drm_atomic_private_obj_fini(struct drm_device *drm, void *private_obj)
+{
+	drm_atomic_private_obj_fini(private_obj);
+}
+
 static int ingenic_drm_bind(struct device *dev, bool has_components)
 {
 	struct platform_device *pdev = to_platform_device(dev);
+	struct ingenic_drm_private_state *private_state;
 	const struct jz_soc_info *soc_info;
 	struct ingenic_drm *priv;
 	struct clk *parent_clk;
@@ -1148,6 +1193,20 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 		goto err_devclk_disable;
 	}
 
+	private_state = kzalloc(sizeof(*private_state), GFP_KERNEL);
+	if (!private_state) {
+		ret = -ENOMEM;
+		goto err_clk_notifier_unregister;
+	}
+
+	drm_atomic_private_obj_init(drm, &priv->private_obj, &private_state->base,
+				    &ingenic_drm_private_state_funcs);
+
+	ret = drmm_add_action_or_reset(drm, ingenic_drm_atomic_private_obj_fini,
+				       &priv->private_obj);
+	if (ret)
+		goto err_private_state_free;
+
 	ret = drm_dev_register(drm, 0);
 	if (ret) {
 		dev_err(dev, "Failed to register DRM driver\n");
@@ -1158,6 +1217,8 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 
 	return 0;
 
+err_private_state_free:
+	kfree(private_state);
 err_clk_notifier_unregister:
 	clk_notifier_unregister(parent_clk, &priv->clock_nb);
 err_devclk_disable:
diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c b/drivers/gpu/drm/ingenic/ingenic-ipu.c
index aeb8a757d213..c819293b8317 100644
--- a/drivers/gpu/drm/ingenic/ingenic-ipu.c
+++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c
@@ -45,6 +45,10 @@ struct soc_info {
 			  unsigned int weight, unsigned int offset);
 };
 
+struct ingenic_ipu_private_state {
+	struct drm_private_state base;
+};
+
 struct ingenic_ipu {
 	struct drm_plane plane;
 	struct drm_device *drm;
@@ -60,6 +64,8 @@ struct ingenic_ipu {
 
 	struct drm_property *sharpness_prop;
 	unsigned int sharpness;
+
+	struct drm_private_obj private_obj;
 };
 
 /* Signed 15.16 fixed-point math (for bicubic scaling coefficients) */
@@ -73,6 +79,12 @@ static inline struct ingenic_ipu *plane_to_ingenic_ipu(struct drm_plane *plane)
 	return container_of(plane, struct ingenic_ipu, plane);
 }
 
+static inline struct ingenic_ipu_private_state *
+to_ingenic_ipu_priv_state(struct drm_private_state *state)
+{
+	return container_of(state, struct ingenic_ipu_private_state, base);
+}
+
 /*
  * Apply conventional cubic convolution kernel. Both parameters
  *  and return value are 15.16 signed fixed-point.
@@ -679,6 +691,33 @@ static const struct drm_plane_funcs ingenic_ipu_plane_funcs = {
 	.atomic_set_property	= ingenic_ipu_plane_atomic_set_property,
 };
 
+static struct drm_private_state *
+ingenic_ipu_duplicate_state(struct drm_private_obj *obj)
+{
+	struct ingenic_ipu_private_state *state = to_ingenic_ipu_priv_state(obj->state);
+
+	state = kmemdup(state, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return NULL;
+
+	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
+
+	return &state->base;
+}
+
+static void ingenic_ipu_destroy_state(struct drm_private_obj *obj,
+				      struct drm_private_state *state)
+{
+	struct ingenic_ipu_private_state *priv_state = to_ingenic_ipu_priv_state(state);
+
+	kfree(priv_state);
+}
+
+static const struct drm_private_state_funcs ingenic_ipu_private_state_funcs = {
+	.atomic_duplicate_state = ingenic_ipu_duplicate_state,
+	.atomic_destroy_state = ingenic_ipu_destroy_state,
+};
+
 static irqreturn_t ingenic_ipu_irq_handler(int irq, void *arg)
 {
 	struct ingenic_ipu *ipu = arg;
@@ -717,6 +756,7 @@ static const struct regmap_config ingenic_ipu_regmap_config = {
 static int ingenic_ipu_bind(struct device *dev, struct device *master, void *d)
 {
 	struct platform_device *pdev = to_platform_device(dev);
+	struct ingenic_ipu_private_state *private_state;
 	const struct soc_info *soc_info;
 	struct drm_device *drm = d;
 	struct drm_plane *plane;
@@ -810,7 +850,20 @@ static int ingenic_ipu_bind(struct device *dev, struct device *master, void *d)
 		return err;
 	}
 
+	private_state = kzalloc(sizeof(*private_state), GFP_KERNEL);
+	if (!private_state) {
+		err = -ENOMEM;
+		goto err_clk_unprepare;
+	}
+
+	drm_atomic_private_obj_init(drm, &ipu->private_obj, &private_state->base,
+				    &ingenic_ipu_private_state_funcs);
+
 	return 0;
+
+err_clk_unprepare:
+	clk_unprepare(ipu->clk);
+	return err;
 }
 
 static void ingenic_ipu_unbind(struct device *dev,
@@ -818,6 +871,7 @@ static void ingenic_ipu_unbind(struct device *dev,
 {
 	struct ingenic_ipu *ipu = dev_get_drvdata(dev);
 
+	drm_atomic_private_obj_fini(&ipu->private_obj);
 	clk_unprepare(ipu->clk);
 }
 
-- 
2.33.0

[PATCH v3 3/6] drm/ingenic: Move IPU scale settings to private state

[Paul Cercueil]

The IPU scaling information is computed in the plane's ".atomic_check"
callback, and used in the ".atomic_update" callback. As such, it is
state-specific, and should be moved to a private state structure.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/ingenic/ingenic-ipu.c | 73 ++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c b/drivers/gpu/drm/ingenic/ingenic-ipu.c
index c819293b8317..2737fc521e15 100644
--- a/drivers/gpu/drm/ingenic/ingenic-ipu.c
+++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c
@@ -47,6 +47,8 @@ struct soc_info {
 
 struct ingenic_ipu_private_state {
 	struct drm_private_state base;
+
+	unsigned int num_w, num_h, denom_w, denom_h;
 };
 
 struct ingenic_ipu {
@@ -58,8 +60,6 @@ struct ingenic_ipu {
 	const struct soc_info *soc_info;
 	bool clk_enabled;
 
-	unsigned int num_w, num_h, denom_w, denom_h;
-
 	dma_addr_t addr_y, addr_u, addr_v;
 
 	struct drm_property *sharpness_prop;
@@ -85,6 +85,30 @@ to_ingenic_ipu_priv_state(struct drm_private_state *state)
 	return container_of(state, struct ingenic_ipu_private_state, base);
 }
 
+static struct ingenic_ipu_private_state *
+ingenic_ipu_get_priv_state(struct ingenic_ipu *priv, struct drm_atomic_state *state)
+{
+	struct drm_private_state *priv_state;
+
+	priv_state = drm_atomic_get_private_obj_state(state, &priv->private_obj);
+	if (IS_ERR(priv_state))
+		return ERR_CAST(priv_state);
+
+	return to_ingenic_ipu_priv_state(priv_state);
+}
+
+static struct ingenic_ipu_private_state *
+ingenic_ipu_get_new_priv_state(struct ingenic_ipu *priv, struct drm_atomic_state *state)
+{
+	struct drm_private_state *priv_state;
+
+	priv_state = drm_atomic_get_new_private_obj_state(state, &priv->private_obj);
+	if (!priv_state)
+		return NULL;
+
+	return to_ingenic_ipu_priv_state(priv_state);
+}
+
 /*
  * Apply conventional cubic convolution kernel. Both parameters
  *  and return value are 15.16 signed fixed-point.
@@ -305,11 +329,16 @@ static void ingenic_ipu_plane_atomic_update(struct drm_plane *plane,
 	const struct drm_format_info *finfo;
 	u32 ctrl, stride = 0, coef_index = 0, format = 0;
 	bool needs_modeset, upscaling_w, upscaling_h;
+	struct ingenic_ipu_private_state *ipu_state;
 	int err;
 
 	if (!newstate || !newstate->fb)
 		return;
 
+	ipu_state = ingenic_ipu_get_new_priv_state(ipu, state);
+	if (WARN_ON(!ipu_state))
+		return;
+
 	finfo = drm_format_info(newstate->fb->format->format);
 
 	if (!ipu->clk_enabled) {
@@ -482,27 +511,27 @@ static void ingenic_ipu_plane_atomic_update(struct drm_plane *plane,
 	if (ipu->soc_info->has_bicubic)
 		ctrl |= JZ_IPU_CTRL_ZOOM_SEL;
 
-	upscaling_w = ipu->num_w > ipu->denom_w;
+	upscaling_w = ipu_state->num_w > ipu_state->denom_w;
 	if (upscaling_w)
 		ctrl |= JZ_IPU_CTRL_HSCALE;
 
-	if (ipu->num_w != 1 || ipu->denom_w != 1) {
+	if (ipu_state->num_w != 1 || ipu_state->denom_w != 1) {
 		if (!ipu->soc_info->has_bicubic && !upscaling_w)
-			coef_index |= (ipu->denom_w - 1) << 16;
+			coef_index |= (ipu_state->denom_w - 1) << 16;
 		else
-			coef_index |= (ipu->num_w - 1) << 16;
+			coef_index |= (ipu_state->num_w - 1) << 16;
 		ctrl |= JZ_IPU_CTRL_HRSZ_EN;
 	}
 
-	upscaling_h = ipu->num_h > ipu->denom_h;
+	upscaling_h = ipu_state->num_h > ipu_state->denom_h;
 	if (upscaling_h)
 		ctrl |= JZ_IPU_CTRL_VSCALE;
 
-	if (ipu->num_h != 1 || ipu->denom_h != 1) {
+	if (ipu_state->num_h != 1 || ipu_state->denom_h != 1) {
 		if (!ipu->soc_info->has_bicubic && !upscaling_h)
-			coef_index |= ipu->denom_h - 1;
+			coef_index |= ipu_state->denom_h - 1;
 		else
-			coef_index |= ipu->num_h - 1;
+			coef_index |= ipu_state->num_h - 1;
 		ctrl |= JZ_IPU_CTRL_VRSZ_EN;
 	}
 
@@ -513,13 +542,13 @@ static void ingenic_ipu_plane_atomic_update(struct drm_plane *plane,
 	/* Set the LUT index register */
 	regmap_write(ipu->map, JZ_REG_IPU_RSZ_COEF_INDEX, coef_index);
 
-	if (ipu->num_w != 1 || ipu->denom_w != 1)
+	if (ipu_state->num_w != 1 || ipu_state->denom_w != 1)
 		ingenic_ipu_set_coefs(ipu, JZ_REG_IPU_HRSZ_COEF_LUT,
-				      ipu->num_w, ipu->denom_w);
+				      ipu_state->num_w, ipu_state->denom_w);
 
-	if (ipu->num_h != 1 || ipu->denom_h != 1)
+	if (ipu_state->num_h != 1 || ipu_state->denom_h != 1)
 		ingenic_ipu_set_coefs(ipu, JZ_REG_IPU_VRSZ_COEF_LUT,
-				      ipu->num_h, ipu->denom_h);
+				      ipu_state->num_h, ipu_state->denom_h);
 
 	/* Clear STATUS register */
 	regmap_write(ipu->map, JZ_REG_IPU_STATUS, 0);
@@ -531,7 +560,8 @@ static void ingenic_ipu_plane_atomic_update(struct drm_plane *plane,
 	dev_dbg(ipu->dev, "Scaling %ux%u to %ux%u (%u:%u horiz, %u:%u vert)\n",
 		newstate->src_w >> 16, newstate->src_h >> 16,
 		newstate->crtc_w, newstate->crtc_h,
-		ipu->num_w, ipu->denom_w, ipu->num_h, ipu->denom_h);
+		ipu_state->num_w, ipu_state->denom_w,
+		ipu_state->num_h, ipu_state->denom_h);
 }
 
 static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
@@ -545,6 +575,7 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
 	struct ingenic_ipu *ipu = plane_to_ingenic_ipu(plane);
 	struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc;
 	struct drm_crtc_state *crtc_state;
+	struct ingenic_ipu_private_state *ipu_state;
 
 	if (!crtc)
 		return 0;
@@ -553,6 +584,10 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
 	if (WARN_ON(!crtc_state))
 		return -EINVAL;
 
+	ipu_state = ingenic_ipu_get_priv_state(ipu, state);
+	if (IS_ERR(ipu_state))
+		return PTR_ERR(ipu_state);
+
 	/* Request a full modeset if we are enabling or disabling the IPU. */
 	if (!old_plane_state->crtc ^ !new_plane_state->crtc)
 		crtc_state->mode_changed = true;
@@ -605,10 +640,10 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
 	if (num_h > max_h)
 		return -EINVAL;
 
-	ipu->num_w = num_w;
-	ipu->num_h = num_h;
-	ipu->denom_w = denom_w;
-	ipu->denom_h = denom_h;
+	ipu_state->num_w = num_w;
+	ipu_state->num_h = num_h;
+	ipu_state->denom_w = denom_w;
+	ipu_state->denom_h = denom_h;
 
 out_check_damage:
 	if (ingenic_drm_map_noncoherent(ipu->master))
-- 
2.33.0

[PATCH v3 4/6] drm/ingenic: Set DMA descriptor chain register when starting CRTC

[Paul Cercueil]

Setting the DMA descriptor chain register in the probe function has been
fine until now, because we only ever had one descriptor per foreground.

As the driver will soon have real descriptor chains, and the DMA
descriptor chain register updates itself to point to the current
descriptor being processed, this register needs to be reset after a full
modeset to point to the first descriptor of the chain.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 5dbeca0f8f37..cbc76cede99e 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -186,6 +186,10 @@ static void ingenic_drm_crtc_atomic_enable(struct drm_crtc *crtc,
 
 	regmap_write(priv->map, JZ_REG_LCD_STATE, 0);
 
+	/* Set address of our DMA descriptor chain */
+	regmap_write(priv->map, JZ_REG_LCD_DA0, dma_hwdesc_addr(priv, 0));
+	regmap_write(priv->map, JZ_REG_LCD_DA1, dma_hwdesc_addr(priv, 1));
+
 	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
 			   JZ_LCD_CTRL_ENABLE | JZ_LCD_CTRL_DISABLE,
 			   JZ_LCD_CTRL_ENABLE);
-- 
2.33.0

[PATCH v3 5/6] drm/ingenic: Upload palette before frame

[Paul Cercueil]

When using C8 color mode, make sure that the palette is always uploaded
before a frame; otherwise the very first frame will have wrong colors.

Do that by changing the link order of the DMA descriptors.

v3: Fix ingenic_drm_get_new_priv_state() called instead of
    ingenic_drm_get_priv_state()

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 53 ++++++++++++++++++++---
 1 file changed, 47 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index cbc76cede99e..a5e2880e07a1 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -66,6 +66,7 @@ struct jz_soc_info {
 
 struct ingenic_drm_private_state {
 	struct drm_private_state base;
+	bool use_palette;
 };
 
 struct ingenic_drm {
@@ -113,6 +114,30 @@ to_ingenic_drm_priv_state(struct drm_private_state *state)
 	return container_of(state, struct ingenic_drm_private_state, base);
 }
 
+static struct ingenic_drm_private_state *
+ingenic_drm_get_priv_state(struct ingenic_drm *priv, struct drm_atomic_state *state)
+{
+	struct drm_private_state *priv_state;
+
+	priv_state = drm_atomic_get_private_obj_state(state, &priv->private_obj);
+	if (IS_ERR(priv_state))
+		return ERR_CAST(priv_state);
+
+	return to_ingenic_drm_priv_state(priv_state);
+}
+
+static struct ingenic_drm_private_state *
+ingenic_drm_get_new_priv_state(struct ingenic_drm *priv, struct drm_atomic_state *state)
+{
+	struct drm_private_state *priv_state;
+
+	priv_state = drm_atomic_get_new_private_obj_state(state, &priv->private_obj);
+	if (!priv_state)
+		return NULL;
+
+	return to_ingenic_drm_priv_state(priv_state);
+}
+
 static bool ingenic_drm_writeable_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
@@ -183,11 +208,18 @@ static void ingenic_drm_crtc_atomic_enable(struct drm_crtc *crtc,
 					   struct drm_atomic_state *state)
 {
 	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
+	struct ingenic_drm_private_state *priv_state;
+	unsigned int next_id;
+
+	priv_state = ingenic_drm_get_priv_state(priv, state);
+	if (WARN_ON(IS_ERR(priv_state)))
+		return;
 
 	regmap_write(priv->map, JZ_REG_LCD_STATE, 0);
 
-	/* Set address of our DMA descriptor chain */
-	regmap_write(priv->map, JZ_REG_LCD_DA0, dma_hwdesc_addr(priv, 0));
+	/* Set addresses of our DMA descriptor chains */
+	next_id = priv_state->use_palette ? HWDESC_PALETTE : 0;
+	regmap_write(priv->map, JZ_REG_LCD_DA0, dma_hwdesc_addr(priv, next_id));
 	regmap_write(priv->map, JZ_REG_LCD_DA1, dma_hwdesc_addr(priv, 1));
 
 	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
@@ -393,6 +425,7 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
 	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
 										 plane);
 	struct ingenic_drm *priv = drm_device_get_priv(plane->dev);
+	struct ingenic_drm_private_state *priv_state;
 	struct drm_crtc_state *crtc_state;
 	struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc;
 	int ret;
@@ -405,6 +438,10 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
 	if (WARN_ON(!crtc_state))
 		return -EINVAL;
 
+	priv_state = ingenic_drm_get_priv_state(priv, state);
+	if (IS_ERR(priv_state))
+		return PTR_ERR(priv_state);
+
 	ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
 						  DRM_PLANE_HELPER_NO_SCALING,
 						  DRM_PLANE_HELPER_NO_SCALING,
@@ -423,6 +460,9 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
 	     (new_plane_state->src_h >> 16) != new_plane_state->crtc_h))
 		return -EINVAL;
 
+	priv_state->use_palette = new_plane_state->fb &&
+		new_plane_state->fb->format->format == DRM_FORMAT_C8;
+
 	/*
 	 * Require full modeset if enabling or disabling a plane, or changing
 	 * its position, size or depth.
@@ -583,6 +623,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
 	struct drm_plane_state *newstate = drm_atomic_get_new_plane_state(state, plane);
 	struct drm_plane_state *oldstate = drm_atomic_get_old_plane_state(state, plane);
 	unsigned int width, height, cpp, next_id, plane_id;
+	struct ingenic_drm_private_state *priv_state;
 	struct drm_crtc_state *crtc_state;
 	struct ingenic_dma_hwdesc *hwdesc;
 	dma_addr_t addr;
@@ -600,19 +641,19 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
 		height = newstate->src_h >> 16;
 		cpp = newstate->fb->format->cpp[0];
 
-		hwdesc = &priv->dma_hwdescs->hwdesc[plane_id];
+		priv_state = ingenic_drm_get_new_priv_state(priv, state);
+		next_id = (priv_state && priv_state->use_palette) ? HWDESC_PALETTE : plane_id;
 
+		hwdesc = &priv->dma_hwdescs->hwdesc[plane_id];
 		hwdesc->addr = addr;
 		hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
+		hwdesc->next = dma_hwdesc_addr(priv, next_id);
 
 		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
 			fourcc = newstate->fb->format->format;
 
 			ingenic_drm_plane_config(priv->dev, plane, fourcc);
 
-			next_id = fourcc == DRM_FORMAT_C8 ? HWDESC_PALETTE : 0;
-			priv->dma_hwdescs->hwdesc[0].next = dma_hwdesc_addr(priv, next_id);
-
 			crtc_state->color_mgmt_changed = fourcc == DRM_FORMAT_C8;
 		}
 
-- 
2.33.0

[PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[Paul Cercueil]

Attach a top-level bridge to each encoder, which will be used for
negociating the bus format and flags.

All the bridges are now attached with DRM_BRIDGE_ATTACH_NO_CONNECTOR.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 92 +++++++++++++++++------
 1 file changed, 70 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index a5e2880e07a1..a05a9fa6e115 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -21,6 +21,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
+#include <drm/drm_bridge_connector.h>
 #include <drm/drm_color_mgmt.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
@@ -108,6 +109,19 @@ struct ingenic_drm {
 	struct drm_private_obj private_obj;
 };
 
+struct ingenic_drm_bridge {
+	struct drm_encoder encoder;
+	struct drm_bridge bridge, *next_bridge;
+
+	struct drm_bus_cfg bus_cfg;
+};
+
+static inline struct ingenic_drm_bridge *
+to_ingenic_drm_bridge(struct drm_encoder *encoder)
+{
+	return container_of(encoder, struct ingenic_drm_bridge, encoder);
+}
+
 static inline struct ingenic_drm_private_state *
 to_ingenic_drm_priv_state(struct drm_private_state *state)
 {
@@ -668,11 +682,10 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
 {
 	struct ingenic_drm *priv = drm_device_get_priv(encoder->dev);
 	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
-	struct drm_connector *conn = conn_state->connector;
-	struct drm_display_info *info = &conn->display_info;
+	struct ingenic_drm_bridge *bridge = to_ingenic_drm_bridge(encoder);
 	unsigned int cfg, rgbcfg = 0;
 
-	priv->panel_is_sharp = info->bus_flags & DRM_BUS_FLAG_SHARP_SIGNALS;
+	priv->panel_is_sharp = bridge->bus_cfg.flags & DRM_BUS_FLAG_SHARP_SIGNALS;
 
 	if (priv->panel_is_sharp) {
 		cfg = JZ_LCD_CFG_MODE_SPECIAL_TFT_1 | JZ_LCD_CFG_REV_POLARITY;
@@ -685,19 +698,19 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
 		cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
 	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
 		cfg |= JZ_LCD_CFG_VSYNC_ACTIVE_LOW;
-	if (info->bus_flags & DRM_BUS_FLAG_DE_LOW)
+	if (bridge->bus_cfg.flags & DRM_BUS_FLAG_DE_LOW)
 		cfg |= JZ_LCD_CFG_DE_ACTIVE_LOW;
-	if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
+	if (bridge->bus_cfg.flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
 		cfg |= JZ_LCD_CFG_PCLK_FALLING_EDGE;
 
 	if (!priv->panel_is_sharp) {
-		if (conn->connector_type == DRM_MODE_CONNECTOR_TV) {
+		if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV) {
 			if (mode->flags & DRM_MODE_FLAG_INTERLACE)
 				cfg |= JZ_LCD_CFG_MODE_TV_OUT_I;
 			else
 				cfg |= JZ_LCD_CFG_MODE_TV_OUT_P;
 		} else {
-			switch (*info->bus_formats) {
+			switch (bridge->bus_cfg.format) {
 			case MEDIA_BUS_FMT_RGB565_1X16:
 				cfg |= JZ_LCD_CFG_MODE_GENERIC_16BIT;
 				break;
@@ -723,20 +736,29 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
 	regmap_write(priv->map, JZ_REG_LCD_RGBC, rgbcfg);
 }
 
-static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder,
-					    struct drm_crtc_state *crtc_state,
-					    struct drm_connector_state *conn_state)
+static int ingenic_drm_bridge_attach(struct drm_bridge *bridge,
+				     enum drm_bridge_attach_flags flags)
+{
+	struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
+
+	return drm_bridge_attach(bridge->encoder, ib->next_bridge,
+				 &ib->bridge, flags);
+}
+
+static int ingenic_drm_bridge_atomic_check(struct drm_bridge *bridge,
+					   struct drm_bridge_state *bridge_state,
+					   struct drm_crtc_state *crtc_state,
+					   struct drm_connector_state *conn_state)
 {
-	struct drm_display_info *info = &conn_state->connector->display_info;
 	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
+	struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
 
-	if (info->num_bus_formats != 1)
-		return -EINVAL;
+	ib->bus_cfg = bridge_state->output_bus_cfg;
 
 	if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV)
 		return 0;
 
-	switch (*info->bus_formats) {
+	switch (bridge_state->output_bus_cfg.format) {
 	case MEDIA_BUS_FMT_RGB888_3X8:
 	case MEDIA_BUS_FMT_RGB888_3X8_DELTA:
 		/*
@@ -900,8 +922,16 @@ static const struct drm_crtc_helper_funcs ingenic_drm_crtc_helper_funcs = {
 };
 
 static const struct drm_encoder_helper_funcs ingenic_drm_encoder_helper_funcs = {
-	.atomic_mode_set	= ingenic_drm_encoder_atomic_mode_set,
-	.atomic_check		= ingenic_drm_encoder_atomic_check,
+	.atomic_mode_set        = ingenic_drm_encoder_atomic_mode_set,
+};
+
+static const struct drm_bridge_funcs ingenic_drm_bridge_funcs = {
+	.attach			= ingenic_drm_bridge_attach,
+	.atomic_check		= ingenic_drm_bridge_atomic_check,
+	.atomic_reset		= drm_atomic_helper_bridge_reset,
+	.atomic_duplicate_state	= drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state	= drm_atomic_helper_bridge_destroy_state,
+	.atomic_get_input_bus_fmts = drm_atomic_helper_bridge_propagate_bus_fmt,
 };
 
 static const struct drm_mode_config_funcs ingenic_drm_mode_config_funcs = {
@@ -976,7 +1006,9 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 	struct drm_plane *primary;
 	struct drm_bridge *bridge;
 	struct drm_panel *panel;
+	struct drm_connector *connector;
 	struct drm_encoder *encoder;
+	struct ingenic_drm_bridge *ib;
 	struct drm_device *drm;
 	void __iomem *base;
 	long parent_rate;
@@ -1154,20 +1186,36 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 			bridge = devm_drm_panel_bridge_add_typed(dev, panel,
 								 DRM_MODE_CONNECTOR_DPI);
 
-		encoder = drmm_plain_encoder_alloc(drm, NULL, DRM_MODE_ENCODER_DPI, NULL);
-		if (IS_ERR(encoder)) {
-			ret = PTR_ERR(encoder);
+		ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder,
+					NULL, DRM_MODE_ENCODER_DPI, NULL);
+		if (IS_ERR(ib)) {
+			ret = PTR_ERR(ib);
 			dev_err(dev, "Failed to init encoder: %d\n", ret);
 			return ret;
 		}
 
-		encoder->possible_crtcs = 1;
+		encoder = &ib->encoder;
+		encoder->possible_crtcs = drm_crtc_mask(&priv->crtc);
 
 		drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
 
-		ret = drm_bridge_attach(encoder, bridge, NULL, 0);
-		if (ret)
+		ib->bridge.funcs = &ingenic_drm_bridge_funcs;
+		ib->next_bridge = bridge;
+
+		ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
+					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+		if (ret) {
+			dev_err(dev, "Unable to attach bridge\n");
 			return ret;
+		}
+
+		connector = drm_bridge_connector_init(drm, encoder);
+		if (IS_ERR(connector)) {
+			dev_err(dev, "Unable to init connector\n");
+			return PTR_ERR(connector);
+		}
+
+		drm_connector_attach_encoder(connector, encoder);
 	}
 
 	drm_for_each_encoder(encoder, drm) {
-- 
2.33.0

Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[H. Nikolaus Schaller]

Hi Paul,
thanks for another update.

We have been delayed to rework the CI20 HDMI code on top of your series
but it basically works in some situations. There is for example a problem
if the EDID reports DRM_COLOR_FORMAT_YCRCB422 but it appears to be outside
of your series.

The only issue we have is described below.

> Am 22.09.2021 um 22:55 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Attach a top-level bridge to each encoder, which will be used for
> negociating the bus format and flags.
> 
> All the bridges are now attached with DRM_BRIDGE_ATTACH_NO_CONNECTOR.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 92 +++++++++++++++++------
> 1 file changed, 70 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index a5e2880e07a1..a05a9fa6e115 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -21,6 +21,7 @@
> #include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_bridge.h>
> +#include <drm/drm_bridge_connector.h>
> #include <drm/drm_color_mgmt.h>
> #include <drm/drm_crtc.h>
> #include <drm/drm_crtc_helper.h>
> @@ -108,6 +109,19 @@ struct ingenic_drm {
> 	struct drm_private_obj private_obj;
> };
> 
> +struct ingenic_drm_bridge {
> +	struct drm_encoder encoder;
> +	struct drm_bridge bridge, *next_bridge;
> +
> +	struct drm_bus_cfg bus_cfg;
> +};
> +
> +static inline struct ingenic_drm_bridge *
> +to_ingenic_drm_bridge(struct drm_encoder *encoder)
> +{
> +	return container_of(encoder, struct ingenic_drm_bridge, encoder);
> +}
> +
> static inline struct ingenic_drm_private_state *
> to_ingenic_drm_priv_state(struct drm_private_state *state)
> {
> @@ -668,11 +682,10 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
> {
> 	struct ingenic_drm *priv = drm_device_get_priv(encoder->dev);
> 	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> -	struct drm_connector *conn = conn_state->connector;
> -	struct drm_display_info *info = &conn->display_info;
> +	struct ingenic_drm_bridge *bridge = to_ingenic_drm_bridge(encoder);
> 	unsigned int cfg, rgbcfg = 0;
> 
> -	priv->panel_is_sharp = info->bus_flags & DRM_BUS_FLAG_SHARP_SIGNALS;
> +	priv->panel_is_sharp = bridge->bus_cfg.flags & DRM_BUS_FLAG_SHARP_SIGNALS;
> 
> 	if (priv->panel_is_sharp) {
> 		cfg = JZ_LCD_CFG_MODE_SPECIAL_TFT_1 | JZ_LCD_CFG_REV_POLARITY;
> @@ -685,19 +698,19 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
> 		cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
> 	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> 		cfg |= JZ_LCD_CFG_VSYNC_ACTIVE_LOW;
> -	if (info->bus_flags & DRM_BUS_FLAG_DE_LOW)
> +	if (bridge->bus_cfg.flags & DRM_BUS_FLAG_DE_LOW)
> 		cfg |= JZ_LCD_CFG_DE_ACTIVE_LOW;
> -	if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> +	if (bridge->bus_cfg.flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> 		cfg |= JZ_LCD_CFG_PCLK_FALLING_EDGE;
> 
> 	if (!priv->panel_is_sharp) {
> -		if (conn->connector_type == DRM_MODE_CONNECTOR_TV) {
> +		if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV) {
> 			if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> 				cfg |= JZ_LCD_CFG_MODE_TV_OUT_I;
> 			else
> 				cfg |= JZ_LCD_CFG_MODE_TV_OUT_P;
> 		} else {
> -			switch (*info->bus_formats) {
> +			switch (bridge->bus_cfg.format) {
> 			case MEDIA_BUS_FMT_RGB565_1X16:
> 				cfg |= JZ_LCD_CFG_MODE_GENERIC_16BIT;
> 				break;
> @@ -723,20 +736,29 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
> 	regmap_write(priv->map, JZ_REG_LCD_RGBC, rgbcfg);
> }
> 
> -static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder,
> -					    struct drm_crtc_state *crtc_state,
> -					    struct drm_connector_state *conn_state)
> +static int ingenic_drm_bridge_attach(struct drm_bridge *bridge,
> +				     enum drm_bridge_attach_flags flags)
> +{
> +	struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
> +
> +	return drm_bridge_attach(bridge->encoder, ib->next_bridge,
> +				 &ib->bridge, flags);
> +}
> +
> +static int ingenic_drm_bridge_atomic_check(struct drm_bridge *bridge,
> +					   struct drm_bridge_state *bridge_state,
> +					   struct drm_crtc_state *crtc_state,
> +					   struct drm_connector_state *conn_state)
> {
> -	struct drm_display_info *info = &conn_state->connector->display_info;
> 	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> +	struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
> 
> -	if (info->num_bus_formats != 1)
> -		return -EINVAL;
> +	ib->bus_cfg = bridge_state->output_bus_cfg;
> 
> 	if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV)
> 		return 0;
> 
> -	switch (*info->bus_formats) {
> +	switch (bridge_state->output_bus_cfg.format) {
> 	case MEDIA_BUS_FMT_RGB888_3X8:
> 	case MEDIA_BUS_FMT_RGB888_3X8_DELTA:
> 		/*
> @@ -900,8 +922,16 @@ static const struct drm_crtc_helper_funcs ingenic_drm_crtc_helper_funcs = {
> };
> 
> static const struct drm_encoder_helper_funcs ingenic_drm_encoder_helper_funcs = {
> -	.atomic_mode_set	= ingenic_drm_encoder_atomic_mode_set,
> -	.atomic_check		= ingenic_drm_encoder_atomic_check,
> +	.atomic_mode_set        = ingenic_drm_encoder_atomic_mode_set,
> +};
> +
> +static const struct drm_bridge_funcs ingenic_drm_bridge_funcs = {
> +	.attach			= ingenic_drm_bridge_attach,
> +	.atomic_check		= ingenic_drm_bridge_atomic_check,
> +	.atomic_reset		= drm_atomic_helper_bridge_reset,
> +	.atomic_duplicate_state	= drm_atomic_helper_bridge_duplicate_state,
> +	.atomic_destroy_state	= drm_atomic_helper_bridge_destroy_state,
> +	.atomic_get_input_bus_fmts = drm_atomic_helper_bridge_propagate_bus_fmt,
> };
> 
> static const struct drm_mode_config_funcs ingenic_drm_mode_config_funcs = {
> @@ -976,7 +1006,9 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
> 	struct drm_plane *primary;
> 	struct drm_bridge *bridge;
> 	struct drm_panel *panel;
> +	struct drm_connector *connector;
> 	struct drm_encoder *encoder;
> +	struct ingenic_drm_bridge *ib;
> 	struct drm_device *drm;
> 	void __iomem *base;
> 	long parent_rate;
> @@ -1154,20 +1186,36 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
> 			bridge = devm_drm_panel_bridge_add_typed(dev, panel,
> 								 DRM_MODE_CONNECTOR_DPI);
> 
> -		encoder = drmm_plain_encoder_alloc(drm, NULL, DRM_MODE_ENCODER_DPI, NULL);
> -		if (IS_ERR(encoder)) {
> -			ret = PTR_ERR(encoder);
> +		ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder,
> +					NULL, DRM_MODE_ENCODER_DPI, NULL);
> +		if (IS_ERR(ib)) {
> +			ret = PTR_ERR(ib);
> 			dev_err(dev, "Failed to init encoder: %d\n", ret);
> 			return ret;
> 		}
> 
> -		encoder->possible_crtcs = 1;
> +		encoder = &ib->encoder;
> +		encoder->possible_crtcs = drm_crtc_mask(&priv->crtc);
> 
> 		drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
> 
> -		ret = drm_bridge_attach(encoder, bridge, NULL, 0);
> -		if (ret)
> +		ib->bridge.funcs = &ingenic_drm_bridge_funcs;
> +		ib->next_bridge = bridge;
> +
> +		ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
> +					DRM_BRIDGE_ATTACH_NO_CONNECTOR);

DRM_BRIDGE_ATTACH_NO_CONNECTOR makes it fundamentally incompatible
with synopsys/dw_hdmi.c

That driver checks for DRM_BRIDGE_ATTACH_NO_CONNECTOR being NOT present,
since it wants to register its own connector through dw_hdmi_connector_create().

It does it for a reason: the dw-hdmi is a multi-function driver which does
HDMI and DDC/EDID stuff in a single driver (because I/O registers and power
management seem to be shared).

Since I do not see who could split this into a separate bridge and a connector driver
and test it on multiple SoC platforms (there are at least 3 or 4), I think modifying
the fundamentals of the dw-hdmi architecture just to get CI20 HDMI working is not
our turf.

Therefore the code here should be able to detect if drm_bridge_attach() already
creates and attaches a connector and then skip the code below.

> +		if (ret) {
> +			dev_err(dev, "Unable to attach bridge\n");
> 			return ret;
> +		}
> +
> +		connector = drm_bridge_connector_init(drm, encoder);
> +		if (IS_ERR(connector)) {
> +			dev_err(dev, "Unable to init connector\n");
> +			return PTR_ERR(connector);
> +		}
> +
> +		drm_connector_attach_encoder(connector, encoder);
> 	}
> 
> 	drm_for_each_encoder(encoder, drm) {
> -- 
> 2.33.0

I haven't replaced v2 with v3 in our test tree yet, but will do asap.

BR and thanks,
Nikolaus

Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[Paul Cercueil]

Hi Nikolaus,

Le jeu., sept. 23 2021 at 07:52:08 +0200, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Paul,
> thanks for another update.
> 
> We have been delayed to rework the CI20 HDMI code on top of your 
> series
> but it basically works in some situations. There is for example a 
> problem
> if the EDID reports DRM_COLOR_FORMAT_YCRCB422 but it appears to be 
> outside
> of your series.

I think the SoC can output YCbCr as well, but I never tried to use it.

> The only issue we have is described below.
> 
>>  Am 22.09.2021 um 22:55 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>>  Attach a top-level bridge to each encoder, which will be used for
>>  negociating the bus format and flags.
>> 
>>  All the bridges are now attached with 
>> DRM_BRIDGE_ATTACH_NO_CONNECTOR.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 92 
>> +++++++++++++++++------
>>  1 file changed, 70 insertions(+), 22 deletions(-)
>> 
>>  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c 
>> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  index a5e2880e07a1..a05a9fa6e115 100644
>>  --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  @@ -21,6 +21,7 @@
>>  #include <drm/drm_atomic.h>
>>  #include <drm/drm_atomic_helper.h>
>>  #include <drm/drm_bridge.h>
>>  +#include <drm/drm_bridge_connector.h>
>>  #include <drm/drm_color_mgmt.h>
>>  #include <drm/drm_crtc.h>
>>  #include <drm/drm_crtc_helper.h>
>>  @@ -108,6 +109,19 @@ struct ingenic_drm {
>>  	struct drm_private_obj private_obj;
>>  };
>> 
>>  +struct ingenic_drm_bridge {
>>  +	struct drm_encoder encoder;
>>  +	struct drm_bridge bridge, *next_bridge;
>>  +
>>  +	struct drm_bus_cfg bus_cfg;
>>  +};
>>  +
>>  +static inline struct ingenic_drm_bridge *
>>  +to_ingenic_drm_bridge(struct drm_encoder *encoder)
>>  +{
>>  +	return container_of(encoder, struct ingenic_drm_bridge, encoder);
>>  +}
>>  +
>>  static inline struct ingenic_drm_private_state *
>>  to_ingenic_drm_priv_state(struct drm_private_state *state)
>>  {
>>  @@ -668,11 +682,10 @@ static void 
>> ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
>>  {
>>  	struct ingenic_drm *priv = drm_device_get_priv(encoder->dev);
>>  	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
>>  -	struct drm_connector *conn = conn_state->connector;
>>  -	struct drm_display_info *info = &conn->display_info;
>>  +	struct ingenic_drm_bridge *bridge = 
>> to_ingenic_drm_bridge(encoder);
>>  	unsigned int cfg, rgbcfg = 0;
>> 
>>  -	priv->panel_is_sharp = info->bus_flags & 
>> DRM_BUS_FLAG_SHARP_SIGNALS;
>>  +	priv->panel_is_sharp = bridge->bus_cfg.flags & 
>> DRM_BUS_FLAG_SHARP_SIGNALS;
>> 
>>  	if (priv->panel_is_sharp) {
>>  		cfg = JZ_LCD_CFG_MODE_SPECIAL_TFT_1 | JZ_LCD_CFG_REV_POLARITY;
>>  @@ -685,19 +698,19 @@ static void 
>> ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
>>  		cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
>>  	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
>>  		cfg |= JZ_LCD_CFG_VSYNC_ACTIVE_LOW;
>>  -	if (info->bus_flags & DRM_BUS_FLAG_DE_LOW)
>>  +	if (bridge->bus_cfg.flags & DRM_BUS_FLAG_DE_LOW)
>>  		cfg |= JZ_LCD_CFG_DE_ACTIVE_LOW;
>>  -	if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
>>  +	if (bridge->bus_cfg.flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
>>  		cfg |= JZ_LCD_CFG_PCLK_FALLING_EDGE;
>> 
>>  	if (!priv->panel_is_sharp) {
>>  -		if (conn->connector_type == DRM_MODE_CONNECTOR_TV) {
>>  +		if (conn_state->connector->connector_type == 
>> DRM_MODE_CONNECTOR_TV) {
>>  			if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>>  				cfg |= JZ_LCD_CFG_MODE_TV_OUT_I;
>>  			else
>>  				cfg |= JZ_LCD_CFG_MODE_TV_OUT_P;
>>  		} else {
>>  -			switch (*info->bus_formats) {
>>  +			switch (bridge->bus_cfg.format) {
>>  			case MEDIA_BUS_FMT_RGB565_1X16:
>>  				cfg |= JZ_LCD_CFG_MODE_GENERIC_16BIT;
>>  				break;
>>  @@ -723,20 +736,29 @@ static void 
>> ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
>>  	regmap_write(priv->map, JZ_REG_LCD_RGBC, rgbcfg);
>>  }
>> 
>>  -static int ingenic_drm_encoder_atomic_check(struct drm_encoder 
>> *encoder,
>>  -					    struct drm_crtc_state *crtc_state,
>>  -					    struct drm_connector_state *conn_state)
>>  +static int ingenic_drm_bridge_attach(struct drm_bridge *bridge,
>>  +				     enum drm_bridge_attach_flags flags)
>>  +{
>>  +	struct ingenic_drm_bridge *ib = 
>> to_ingenic_drm_bridge(bridge->encoder);
>>  +
>>  +	return drm_bridge_attach(bridge->encoder, ib->next_bridge,
>>  +				 &ib->bridge, flags);
>>  +}
>>  +
>>  +static int ingenic_drm_bridge_atomic_check(struct drm_bridge 
>> *bridge,
>>  +					   struct drm_bridge_state *bridge_state,
>>  +					   struct drm_crtc_state *crtc_state,
>>  +					   struct drm_connector_state *conn_state)
>>  {
>>  -	struct drm_display_info *info = 
>> &conn_state->connector->display_info;
>>  	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
>>  +	struct ingenic_drm_bridge *ib = 
>> to_ingenic_drm_bridge(bridge->encoder);
>> 
>>  -	if (info->num_bus_formats != 1)
>>  -		return -EINVAL;
>>  +	ib->bus_cfg = bridge_state->output_bus_cfg;
>> 
>>  	if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV)
>>  		return 0;
>> 
>>  -	switch (*info->bus_formats) {
>>  +	switch (bridge_state->output_bus_cfg.format) {
>>  	case MEDIA_BUS_FMT_RGB888_3X8:
>>  	case MEDIA_BUS_FMT_RGB888_3X8_DELTA:
>>  		/*
>>  @@ -900,8 +922,16 @@ static const struct drm_crtc_helper_funcs 
>> ingenic_drm_crtc_helper_funcs = {
>>  };
>> 
>>  static const struct drm_encoder_helper_funcs 
>> ingenic_drm_encoder_helper_funcs = {
>>  -	.atomic_mode_set	= ingenic_drm_encoder_atomic_mode_set,
>>  -	.atomic_check		= ingenic_drm_encoder_atomic_check,
>>  +	.atomic_mode_set        = ingenic_drm_encoder_atomic_mode_set,
>>  +};
>>  +
>>  +static const struct drm_bridge_funcs ingenic_drm_bridge_funcs = {
>>  +	.attach			= ingenic_drm_bridge_attach,
>>  +	.atomic_check		= ingenic_drm_bridge_atomic_check,
>>  +	.atomic_reset		= drm_atomic_helper_bridge_reset,
>>  +	.atomic_duplicate_state	= 
>> drm_atomic_helper_bridge_duplicate_state,
>>  +	.atomic_destroy_state	= drm_atomic_helper_bridge_destroy_state,
>>  +	.atomic_get_input_bus_fmts = 
>> drm_atomic_helper_bridge_propagate_bus_fmt,
>>  };
>> 
>>  static const struct drm_mode_config_funcs 
>> ingenic_drm_mode_config_funcs = {
>>  @@ -976,7 +1006,9 @@ static int ingenic_drm_bind(struct device 
>> *dev, bool has_components)
>>  	struct drm_plane *primary;
>>  	struct drm_bridge *bridge;
>>  	struct drm_panel *panel;
>>  +	struct drm_connector *connector;
>>  	struct drm_encoder *encoder;
>>  +	struct ingenic_drm_bridge *ib;
>>  	struct drm_device *drm;
>>  	void __iomem *base;
>>  	long parent_rate;
>>  @@ -1154,20 +1186,36 @@ static int ingenic_drm_bind(struct device 
>> *dev, bool has_components)
>>  			bridge = devm_drm_panel_bridge_add_typed(dev, panel,
>>  								 DRM_MODE_CONNECTOR_DPI);
>> 
>>  -		encoder = drmm_plain_encoder_alloc(drm, NULL, 
>> DRM_MODE_ENCODER_DPI, NULL);
>>  -		if (IS_ERR(encoder)) {
>>  -			ret = PTR_ERR(encoder);
>>  +		ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder,
>>  +					NULL, DRM_MODE_ENCODER_DPI, NULL);
>>  +		if (IS_ERR(ib)) {
>>  +			ret = PTR_ERR(ib);
>>  			dev_err(dev, "Failed to init encoder: %d\n", ret);
>>  			return ret;
>>  		}
>> 
>>  -		encoder->possible_crtcs = 1;
>>  +		encoder = &ib->encoder;
>>  +		encoder->possible_crtcs = drm_crtc_mask(&priv->crtc);
>> 
>>  		drm_encoder_helper_add(encoder, 
>> &ingenic_drm_encoder_helper_funcs);
>> 
>>  -		ret = drm_bridge_attach(encoder, bridge, NULL, 0);
>>  -		if (ret)
>>  +		ib->bridge.funcs = &ingenic_drm_bridge_funcs;
>>  +		ib->next_bridge = bridge;
>>  +
>>  +		ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
>>  +					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> 
> DRM_BRIDGE_ATTACH_NO_CONNECTOR makes it fundamentally incompatible
> with synopsys/dw_hdmi.c
> 
> That driver checks for DRM_BRIDGE_ATTACH_NO_CONNECTOR being NOT 
> present,
> since it wants to register its own connector through 
> dw_hdmi_connector_create().
> 
> It does it for a reason: the dw-hdmi is a multi-function driver which 
> does
> HDMI and DDC/EDID stuff in a single driver (because I/O registers and 
> power
> management seem to be shared).

The IT66121 driver does all of that too, and does not need 
DRM_BRIDGE_ATTACH_NO_CONNECTOR. The drm_bridge_funcs struct has 
callbacks to handle cable detection and DDC stuff.

> Since I do not see who could split this into a separate bridge and a 
> connector driver
> and test it on multiple SoC platforms (there are at least 3 or 4), I 
> think modifying
> the fundamentals of the dw-hdmi architecture just to get CI20 HDMI 
> working is not
> our turf.

You could have a field in the dw-hdmi pdata structure, that would 
instruct the driver whether or not it should use the new API. Ugly, I 
know, and would probably duplicate a lot of code, but that would allow 
other drivers to be updated at a later date.

> Therefore the code here should be able to detect if 
> drm_bridge_attach() already
> creates and attaches a connector and then skip the code below.

Not that easy, unfortunately. On one side we have dw-hdmi which checks 
that DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set, and on the other side 
we have other drivers like the IT66121 which will fail if this flag is 
not set.

Cheers,
-Paul

>>  +		if (ret) {
>>  +			dev_err(dev, "Unable to attach bridge\n");
>>  			return ret;
>>  +		}
>>  +
>>  +		connector = drm_bridge_connector_init(drm, encoder);
>>  +		if (IS_ERR(connector)) {
>>  +			dev_err(dev, "Unable to init connector\n");
>>  +			return PTR_ERR(connector);
>>  +		}
>>  +
>>  +		drm_connector_attach_encoder(connector, encoder);
>>  	}
>> 
>>  	drm_for_each_encoder(encoder, drm) {
>>  --
>>  2.33.0
> 
> I haven't replaced v2 with v3 in our test tree yet, but will do asap.
> 
> BR and thanks,
> Nikolaus
> 
> 

Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[H. Nikolaus Schaller]

Hi Paul,

> Am 23.09.2021 um 10:49 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus,
> 
> Le jeu., sept. 23 2021 at 07:52:08 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> Hi Paul,
>> thanks for another update.
>> We have been delayed to rework the CI20 HDMI code on top of your series
>> but it basically works in some situations. There is for example a problem
>> if the EDID reports DRM_COLOR_FORMAT_YCRCB422 but it appears to be outside
>> of your series.
> 
> I think the SoC can output YCbCr as well, but I never tried to use it.

Maybe there is code missing or something else. We have not yet deeply researched.
Except that when ignoring DRM_COLOR_FORMAT_YCRCB422 capability it uses RGB
and works.

> 
>>> +		ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
>>> +					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>> DRM_BRIDGE_ATTACH_NO_CONNECTOR makes it fundamentally incompatible
>> with synopsys/dw_hdmi.c
>> That driver checks for DRM_BRIDGE_ATTACH_NO_CONNECTOR being NOT present,
>> since it wants to register its own connector through dw_hdmi_connector_create().
>> It does it for a reason: the dw-hdmi is a multi-function driver which does
>> HDMI and DDC/EDID stuff in a single driver (because I/O registers and power
>> management seem to be shared).
> 
> The IT66121 driver does all of that too, and does not need DRM_BRIDGE_ATTACH_NO_CONNECTOR. The drm_bridge_funcs struct has callbacks to handle cable detection and DDC stuff.
> 
>> Since I do not see who could split this into a separate bridge and a connector driver
>> and test it on multiple SoC platforms (there are at least 3 or 4), I think modifying
>> the fundamentals of the dw-hdmi architecture just to get CI20 HDMI working is not
>> our turf.
> 
> You could have a field in the dw-hdmi pdata structure, that would instruct the driver whether or not it should use the new API. Ugly, I know, and would probably duplicate a lot of code, but that would allow other drivers to be updated at a later date.

Yes, would be very ugly.

But generally who has the knowledge (and time) to do this work?
And has a working platform to test (jz4780 isn't a good development environment)?

The driver seems to have a turbulent history starting 2013 in staging/imx and
apparently it was generalized since then... Is Laurent currently dw-hdmi maintainer?

> 
>> Therefore the code here should be able to detect if drm_bridge_attach() already
>> creates and attaches a connector and then skip the code below.
> 
> Not that easy, unfortunately. On one side we have dw-hdmi which checks that DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set, and on the other side we have other drivers like the IT66121 which will fail if this flag is not set.

Ok, I see. You have to handle contradicting cases here.

Would it be possible to run it with DRM_BRIDGE_ATTACH_NO_CONNECTOR first
and retry if it fails without?

But IMHO the return value (in error case) is not well defined. So there
must be a test if a connector has been created (I do not know how this
would work).

Another suggestion: can you check if there is a downstream connector defined in
device tree (dw-hdmi does not need such a definition)?
If not we call it with 0 and if there is one we call it with
DRM_BRIDGE_ATTACH_NO_CONNECTOR and create one?

Just some ideas how to solve without touching hdmi drivers.

BR and thanks,
Nikolaus

Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[Laurent Pinchart]

Hello,

On Thu, Sep 23, 2021 at 09:49:03AM +0100, Paul Cercueil wrote:
> Le jeu., sept. 23 2021 at 07:52:08 +0200, H. Nikolaus Schaller a écrit :
> > Hi Paul,
> > thanks for another update.
> > 
> > We have been delayed to rework the CI20 HDMI code on top of your series
> > but it basically works in some situations. There is for example a problem
> > if the EDID reports DRM_COLOR_FORMAT_YCRCB422 but it appears to be outside
> > of your series.
> 
> I think the SoC can output YCbCr as well, but I never tried to use it.
> 
> > The only issue we have is described below.
> > 
> >>  Am 22.09.2021 um 22:55 schrieb Paul Cercueil <paul@crapouillou.net>:
> >> 
> >>  Attach a top-level bridge to each encoder, which will be used for
> >>  negociating the bus format and flags.
> >> 
> >>  All the bridges are now attached with DRM_BRIDGE_ATTACH_NO_CONNECTOR.
> >> 
> >>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> >>  ---
> >>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 92 +++++++++++++++++------
> >>  1 file changed, 70 insertions(+), 22 deletions(-)
> >> 
> >>  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >>  index a5e2880e07a1..a05a9fa6e115 100644
> >>  --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >>  +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >>  @@ -21,6 +21,7 @@
> >>  #include <drm/drm_atomic.h>
> >>  #include <drm/drm_atomic_helper.h>
> >>  #include <drm/drm_bridge.h>
> >>  +#include <drm/drm_bridge_connector.h>
> >>  #include <drm/drm_color_mgmt.h>
> >>  #include <drm/drm_crtc.h>
> >>  #include <drm/drm_crtc_helper.h>
> >>  @@ -108,6 +109,19 @@ struct ingenic_drm {
> >>  	struct drm_private_obj private_obj;
> >>  };
> >> 
> >>  +struct ingenic_drm_bridge {
> >>  +	struct drm_encoder encoder;
> >>  +	struct drm_bridge bridge, *next_bridge;
> >>  +
> >>  +	struct drm_bus_cfg bus_cfg;
> >>  +};
> >>  +
> >>  +static inline struct ingenic_drm_bridge *
> >>  +to_ingenic_drm_bridge(struct drm_encoder *encoder)
> >>  +{
> >>  +	return container_of(encoder, struct ingenic_drm_bridge, encoder);
> >>  +}
> >>  +
> >>  static inline struct ingenic_drm_private_state *
> >>  to_ingenic_drm_priv_state(struct drm_private_state *state)
> >>  {
> >>  @@ -668,11 +682,10 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
> >>  {
> >>  	struct ingenic_drm *priv = drm_device_get_priv(encoder->dev);
> >>  	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> >>  -	struct drm_connector *conn = conn_state->connector;
> >>  -	struct drm_display_info *info = &conn->display_info;
> >>  +	struct ingenic_drm_bridge *bridge = to_ingenic_drm_bridge(encoder);
> >>  	unsigned int cfg, rgbcfg = 0;
> >> 
> >>  -	priv->panel_is_sharp = info->bus_flags & DRM_BUS_FLAG_SHARP_SIGNALS;
> >>  +	priv->panel_is_sharp = bridge->bus_cfg.flags & DRM_BUS_FLAG_SHARP_SIGNALS;
> >> 
> >>  	if (priv->panel_is_sharp) {
> >>  		cfg = JZ_LCD_CFG_MODE_SPECIAL_TFT_1 | JZ_LCD_CFG_REV_POLARITY;
> >>  @@ -685,19 +698,19 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
> >>  		cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
> >>  	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> >>  		cfg |= JZ_LCD_CFG_VSYNC_ACTIVE_LOW;
> >>  -	if (info->bus_flags & DRM_BUS_FLAG_DE_LOW)
> >>  +	if (bridge->bus_cfg.flags & DRM_BUS_FLAG_DE_LOW)
> >>  		cfg |= JZ_LCD_CFG_DE_ACTIVE_LOW;
> >>  -	if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> >>  +	if (bridge->bus_cfg.flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> >>  		cfg |= JZ_LCD_CFG_PCLK_FALLING_EDGE;
> >> 
> >>  	if (!priv->panel_is_sharp) {
> >>  -		if (conn->connector_type == DRM_MODE_CONNECTOR_TV) {
> >>  +		if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV) {
> >>  			if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> >>  				cfg |= JZ_LCD_CFG_MODE_TV_OUT_I;
> >>  			else
> >>  				cfg |= JZ_LCD_CFG_MODE_TV_OUT_P;
> >>  		} else {
> >>  -			switch (*info->bus_formats) {
> >>  +			switch (bridge->bus_cfg.format) {
> >>  			case MEDIA_BUS_FMT_RGB565_1X16:
> >>  				cfg |= JZ_LCD_CFG_MODE_GENERIC_16BIT;
> >>  				break;
> >>  @@ -723,20 +736,29 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
> >>  	regmap_write(priv->map, JZ_REG_LCD_RGBC, rgbcfg);
> >>  }
> >> 
> >>  -static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder,
> >>  -					    struct drm_crtc_state *crtc_state,
> >>  -					    struct drm_connector_state *conn_state)
> >>  +static int ingenic_drm_bridge_attach(struct drm_bridge *bridge,
> >>  +				     enum drm_bridge_attach_flags flags)
> >>  +{
> >>  +	struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
> >>  +
> >>  +	return drm_bridge_attach(bridge->encoder, ib->next_bridge,
> >>  +				 &ib->bridge, flags);
> >>  +}
> >>  +
> >>  +static int ingenic_drm_bridge_atomic_check(struct drm_bridge *bridge,
> >>  +					   struct drm_bridge_state *bridge_state,
> >>  +					   struct drm_crtc_state *crtc_state,
> >>  +					   struct drm_connector_state *conn_state)
> >>  {
> >>  -	struct drm_display_info *info = &conn_state->connector->display_info;
> >>  	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> >>  +	struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
> >> 
> >>  -	if (info->num_bus_formats != 1)
> >>  -		return -EINVAL;
> >>  +	ib->bus_cfg = bridge_state->output_bus_cfg;
> >> 
> >>  	if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV)
> >>  		return 0;
> >> 
> >>  -	switch (*info->bus_formats) {
> >>  +	switch (bridge_state->output_bus_cfg.format) {
> >>  	case MEDIA_BUS_FMT_RGB888_3X8:
> >>  	case MEDIA_BUS_FMT_RGB888_3X8_DELTA:
> >>  		/*
> >>  @@ -900,8 +922,16 @@ static const struct drm_crtc_helper_funcs ingenic_drm_crtc_helper_funcs = {
> >>  };
> >> 
> >>  static const struct drm_encoder_helper_funcs ingenic_drm_encoder_helper_funcs = {
> >>  -	.atomic_mode_set	= ingenic_drm_encoder_atomic_mode_set,
> >>  -	.atomic_check		= ingenic_drm_encoder_atomic_check,
> >>  +	.atomic_mode_set        = ingenic_drm_encoder_atomic_mode_set,
> >>  +};
> >>  +
> >>  +static const struct drm_bridge_funcs ingenic_drm_bridge_funcs = {
> >>  +	.attach			= ingenic_drm_bridge_attach,
> >>  +	.atomic_check		= ingenic_drm_bridge_atomic_check,
> >>  +	.atomic_reset		= drm_atomic_helper_bridge_reset,
> >>  +	.atomic_duplicate_state	= drm_atomic_helper_bridge_duplicate_state,
> >>  +	.atomic_destroy_state	= drm_atomic_helper_bridge_destroy_state,
> >>  +	.atomic_get_input_bus_fmts = drm_atomic_helper_bridge_propagate_bus_fmt,
> >>  };
> >> 
> >>  static const struct drm_mode_config_funcs ingenic_drm_mode_config_funcs = {
> >>  @@ -976,7 +1006,9 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
> >>  	struct drm_plane *primary;
> >>  	struct drm_bridge *bridge;
> >>  	struct drm_panel *panel;
> >>  +	struct drm_connector *connector;
> >>  	struct drm_encoder *encoder;
> >>  +	struct ingenic_drm_bridge *ib;
> >>  	struct drm_device *drm;
> >>  	void __iomem *base;
> >>  	long parent_rate;
> >>  @@ -1154,20 +1186,36 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
> >>  			bridge = devm_drm_panel_bridge_add_typed(dev, panel,
> >>  								 DRM_MODE_CONNECTOR_DPI);
> >> 
> >>  -		encoder = drmm_plain_encoder_alloc(drm, NULL, DRM_MODE_ENCODER_DPI, NULL);
> >>  -		if (IS_ERR(encoder)) {
> >>  -			ret = PTR_ERR(encoder);
> >>  +		ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder,
> >>  +					NULL, DRM_MODE_ENCODER_DPI, NULL);
> >>  +		if (IS_ERR(ib)) {
> >>  +			ret = PTR_ERR(ib);
> >>  			dev_err(dev, "Failed to init encoder: %d\n", ret);
> >>  			return ret;
> >>  		}
> >> 
> >>  -		encoder->possible_crtcs = 1;
> >>  +		encoder = &ib->encoder;
> >>  +		encoder->possible_crtcs = drm_crtc_mask(&priv->crtc);
> >> 
> >>  		drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
> >> 
> >>  -		ret = drm_bridge_attach(encoder, bridge, NULL, 0);
> >>  -		if (ret)
> >>  +		ib->bridge.funcs = &ingenic_drm_bridge_funcs;
> >>  +		ib->next_bridge = bridge;
> >>  +
> >>  +		ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
> >>  +					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> > 
> > DRM_BRIDGE_ATTACH_NO_CONNECTOR makes it fundamentally incompatible
> > with synopsys/dw_hdmi.c
> > 
> > That driver checks for DRM_BRIDGE_ATTACH_NO_CONNECTOR being NOT present,
> > since it wants to register its own connector through 
> > dw_hdmi_connector_create().

Does it ? The driver has

static int dw_hdmi_bridge_attach(struct drm_bridge *bridge,
                                 enum drm_bridge_attach_flags flags)
{
	struct dw_hdmi *hdmi = bridge->driver_private;

	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
		return drm_bridge_attach(bridge->encoder, hdmi->next_bridge,
					 bridge, flags);

	return dw_hdmi_connector_create(hdmi);
}

If DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set, it will create a
connector, otherwise it will just attach to the next bridge. I'm using
it on a Renesas platform with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag
set, without any issue as far as I can tell.

> > It does it for a reason: the dw-hdmi is a multi-function driver which does
> > HDMI and DDC/EDID stuff in a single driver (because I/O registers and power
> > management seem to be shared).
> 
> The IT66121 driver does all of that too, and does not need 
> DRM_BRIDGE_ATTACH_NO_CONNECTOR. The drm_bridge_funcs struct has 
> callbacks to handle cable detection and DDC stuff.
> 
> > Since I do not see who could split this into a separate bridge and a connector driver
> > and test it on multiple SoC platforms (there are at least 3 or 4), I think modifying
> > the fundamentals of the dw-hdmi architecture just to get CI20 HDMI working is not
> > our turf.
> 
> You could have a field in the dw-hdmi pdata structure, that would 
> instruct the driver whether or not it should use the new API. Ugly, I 
> know, and would probably duplicate a lot of code, but that would allow 
> other drivers to be updated at a later date.
> 
> > Therefore the code here should be able to detect if drm_bridge_attach() already
> > creates and attaches a connector and then skip the code below.
> 
> Not that easy, unfortunately. On one side we have dw-hdmi which checks 
> that DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set, and on the other side 
> we have other drivers like the IT66121 which will fail if this flag is 
> not set.

> >>  +		if (ret) {
> >>  +			dev_err(dev, "Unable to attach bridge\n");
> >>  			return ret;
> >>  +		}
> >>  +
> >>  +		connector = drm_bridge_connector_init(drm, encoder);
> >>  +		if (IS_ERR(connector)) {
> >>  +			dev_err(dev, "Unable to init connector\n");
> >>  +			return PTR_ERR(connector);
> >>  +		}
> >>  +
> >>  +		drm_connector_attach_encoder(connector, encoder);
> >>  	}
> >> 
> >>  	drm_for_each_encoder(encoder, drm) {
> >>  --
> >>  2.33.0
> > 
> > I haven't replaced v2 with v3 in our test tree yet, but will do asap.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[Laurent Pinchart]

Hi Nikolaus,

On Thu, Sep 23, 2021 at 11:19:23AM +0200, H. Nikolaus Schaller wrote:
> > Am 23.09.2021 um 10:49 schrieb Paul Cercueil:
> > Le jeu., sept. 23 2021 at 07:52:08 +0200, H. Nikolaus Schaller a écrit :
> >> Hi Paul,
> >> thanks for another update.
> >> We have been delayed to rework the CI20 HDMI code on top of your series
> >> but it basically works in some situations. There is for example a problem
> >> if the EDID reports DRM_COLOR_FORMAT_YCRCB422 but it appears to be outside
> >> of your series.
> > 
> > I think the SoC can output YCbCr as well, but I never tried to use it.
> 
> Maybe there is code missing or something else. We have not yet deeply researched.
> Except that when ignoring DRM_COLOR_FORMAT_YCRCB422 capability it uses RGB
> and works.
> 
> >>> +		ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
> >>> +					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> >>
> >> DRM_BRIDGE_ATTACH_NO_CONNECTOR makes it fundamentally incompatible
> >> with synopsys/dw_hdmi.c
> >> That driver checks for DRM_BRIDGE_ATTACH_NO_CONNECTOR being NOT present,
> >> since it wants to register its own connector through dw_hdmi_connector_create().
> >> It does it for a reason: the dw-hdmi is a multi-function driver which does
> >> HDMI and DDC/EDID stuff in a single driver (because I/O registers and power
> >> management seem to be shared).
> > 
> > The IT66121 driver does all of that too, and does not need
> > DRM_BRIDGE_ATTACH_NO_CONNECTOR. The drm_bridge_funcs struct has
> > callbacks to handle cable detection and DDC stuff.
> > 
> >> Since I do not see who could split this into a separate bridge and a connector driver
> >> and test it on multiple SoC platforms (there are at least 3 or 4), I think modifying
> >> the fundamentals of the dw-hdmi architecture just to get CI20 HDMI working is not
> >> our turf.
> > 
> > You could have a field in the dw-hdmi pdata structure, that would
> > instruct the driver whether or not it should use the new API. Ugly,
> > I know, and would probably duplicate a lot of code, but that would
> > allow other drivers to be updated at a later date.
> 
> Yes, would be very ugly.
> 
> But generally who has the knowledge (and time) to do this work?
> And has a working platform to test (jz4780 isn't a good development environment)?
> 
> The driver seems to have a turbulent history starting 2013 in staging/imx and
> apparently it was generalized since then... Is Laurent currently dw-hdmi maintainer?

"Maintainer" would be an overstatement. I've worked on that driver in
the past, and I still use it, but don't have time to really maintain it.
I've also been told that Synopsys required all patches for that driver
developed using documentation under NDA to be submitted internally to
them first before being published, so I decided to stop contributing
instead of agreeing with this insane process. There's public
documentation about the IP in some NXP reference manuals though, so it
should be possible to still move forward without abiding by this rule.

> >> Therefore the code here should be able to detect if drm_bridge_attach() already
> >> creates and attaches a connector and then skip the code below.
> > 
> > Not that easy, unfortunately. On one side we have dw-hdmi which
> > checks that DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set, and on the
> > other side we have other drivers like the IT66121 which will fail if
> > this flag is not set.
> 
> Ok, I see. You have to handle contradicting cases here.
> 
> Would it be possible to run it with DRM_BRIDGE_ATTACH_NO_CONNECTOR first
> and retry if it fails without?
> 
> But IMHO the return value (in error case) is not well defined. So there
> must be a test if a connector has been created (I do not know how this
> would work).
> 
> Another suggestion: can you check if there is a downstream connector defined in
> device tree (dw-hdmi does not need such a definition)?
> If not we call it with 0 and if there is one we call it with
> DRM_BRIDGE_ATTACH_NO_CONNECTOR and create one?

I haven't followed the ful conversation, what the reason why
DRM_BRIDGE_ATTACH_NO_CONNECTOR can't always be use here ? We're moving
towards requiring DRM_BRIDGE_ATTACH_NO_CONNECTOR for all new code, so it
will have to be done eventually.

> Just some ideas how to solve without touching hdmi drivers.
> 
> BR and thanks,
> Nikolaus

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[H. Nikolaus Schaller]

Hi Laurent,

> Am 23.09.2021 um 11:27 schrieb Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> 
> Hi Nikolaus,
> 
> On Thu, Sep 23, 2021 at 11:19:23AM +0200, H. Nikolaus Schaller wrote:
>> 
>>>>> +		ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
>>>>> +					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>>> 
>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR makes it fundamentally incompatible
>>>> with synopsys/dw_hdmi.c
>>>> That driver checks for DRM_BRIDGE_ATTACH_NO_CONNECTOR being NOT present,
>>>> since it wants to register its own connector through dw_hdmi_connector_create().
>>>> It does it for a reason: the dw-hdmi is a multi-function driver which does
>>>> HDMI and DDC/EDID stuff in a single driver (because I/O registers and power
>>>> management seem to be shared).
>>> 
>>> The IT66121 driver does all of that too, and does not need
>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. The drm_bridge_funcs struct has
>>> callbacks to handle cable detection and DDC stuff.
>>> 
>>>> Since I do not see who could split this into a separate bridge and a connector driver
>>>> and test it on multiple SoC platforms (there are at least 3 or 4), I think modifying
>>>> the fundamentals of the dw-hdmi architecture just to get CI20 HDMI working is not
>>>> our turf.
>>> 
>>> You could have a field in the dw-hdmi pdata structure, that would
>>> instruct the driver whether or not it should use the new API. Ugly,
>>> I know, and would probably duplicate a lot of code, but that would
>>> allow other drivers to be updated at a later date.
>> 
>> Yes, would be very ugly.
>> 
>> But generally who has the knowledge (and time) to do this work?
>> And has a working platform to test (jz4780 isn't a good development environment)?
>> 
>> The driver seems to have a turbulent history starting 2013 in staging/imx and
>> apparently it was generalized since then... Is Laurent currently dw-hdmi maintainer?
> 
> "Maintainer" would be an overstatement. I've worked on that driver in
> the past, and I still use it, but don't have time to really maintain it.
> I've also been told that Synopsys required all patches for that driver
> developed using documentation under NDA to be submitted internally to
> them first before being published, so I decided to stop contributing
> instead of agreeing with this insane process. There's public
> documentation about the IP in some NXP reference manuals though, so it
> should be possible to still move forward without abiding by this rule.
> 
>>>> Therefore the code here should be able to detect if drm_bridge_attach() already
>>>> creates and attaches a connector and then skip the code below.
>>> 
>>> Not that easy, unfortunately. On one side we have dw-hdmi which
>>> checks that DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set, and on the
>>> other side we have other drivers like the IT66121 which will fail if
>>> this flag is not set.
>> 
>> Ok, I see. You have to handle contradicting cases here.
>> 
>> Would it be possible to run it with DRM_BRIDGE_ATTACH_NO_CONNECTOR first
>> and retry if it fails without?
>> 
>> But IMHO the return value (in error case) is not well defined. So there
>> must be a test if a connector has been created (I do not know how this
>> would work).
>> 
>> Another suggestion: can you check if there is a downstream connector defined in
>> device tree (dw-hdmi does not need such a definition)?
>> If not we call it with 0 and if there is one we call it with
>> DRM_BRIDGE_ATTACH_NO_CONNECTOR and create one?
> 
> I haven't followed the ful conversation, what the reason why
> DRM_BRIDGE_ATTACH_NO_CONNECTOR can't always be use here ?

The synopsys driver creates its own connector through dw_hdmi_connector_create()
because the IP handles DDC/EDID directly.

Hence it checks for ! DRM_BRIDGE_ATTACH_NO_CONNECTOR which seems to be the
right thing to do on current platforms that use it.

For CI20/jz4780 we just add a specialisation of the generic dw-hdmi to
make HDMI work.

Now this patch for drm/ingenic wants the opposite definition and create its own
connector. This fails even if we remove the check (then we have two interfering
connectors).

> We're moving
> towards requiring DRM_BRIDGE_ATTACH_NO_CONNECTOR for all new code, so it
> will have to be done eventually.

So from my view drm/ingenic wants to already enforce this rule and breaks dw-hdmi.

IMHO it should either handle this situation gracefully or include a fix for
dw-hdmi.c to keep it compatible.

BR and thanks,
Nikolaus

Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[Laurent Pinchart]

Hi Nikolaus,

On Thu, Sep 23, 2021 at 11:55:56AM +0200, H. Nikolaus Schaller wrote:
> > Am 23.09.2021 um 11:27 schrieb Laurent Pinchart:
> > On Thu, Sep 23, 2021 at 11:19:23AM +0200, H. Nikolaus Schaller wrote:
> >> 
> >>>>> +		ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
> >>>>> +					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> >>>> 
> >>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR makes it fundamentally incompatible
> >>>> with synopsys/dw_hdmi.c
> >>>> That driver checks for DRM_BRIDGE_ATTACH_NO_CONNECTOR being NOT present,
> >>>> since it wants to register its own connector through dw_hdmi_connector_create().
> >>>> It does it for a reason: the dw-hdmi is a multi-function driver which does
> >>>> HDMI and DDC/EDID stuff in a single driver (because I/O registers and power
> >>>> management seem to be shared).
> >>> 
> >>> The IT66121 driver does all of that too, and does not need
> >>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. The drm_bridge_funcs struct has
> >>> callbacks to handle cable detection and DDC stuff.
> >>> 
> >>>> Since I do not see who could split this into a separate bridge and a connector driver
> >>>> and test it on multiple SoC platforms (there are at least 3 or 4), I think modifying
> >>>> the fundamentals of the dw-hdmi architecture just to get CI20 HDMI working is not
> >>>> our turf.
> >>> 
> >>> You could have a field in the dw-hdmi pdata structure, that would
> >>> instruct the driver whether or not it should use the new API. Ugly,
> >>> I know, and would probably duplicate a lot of code, but that would
> >>> allow other drivers to be updated at a later date.
> >> 
> >> Yes, would be very ugly.
> >> 
> >> But generally who has the knowledge (and time) to do this work?
> >> And has a working platform to test (jz4780 isn't a good development environment)?
> >> 
> >> The driver seems to have a turbulent history starting 2013 in staging/imx and
> >> apparently it was generalized since then... Is Laurent currently dw-hdmi maintainer?
> > 
> > "Maintainer" would be an overstatement. I've worked on that driver in
> > the past, and I still use it, but don't have time to really maintain it.
> > I've also been told that Synopsys required all patches for that driver
> > developed using documentation under NDA to be submitted internally to
> > them first before being published, so I decided to stop contributing
> > instead of agreeing with this insane process. There's public
> > documentation about the IP in some NXP reference manuals though, so it
> > should be possible to still move forward without abiding by this rule.
> > 
> >>>> Therefore the code here should be able to detect if drm_bridge_attach() already
> >>>> creates and attaches a connector and then skip the code below.
> >>> 
> >>> Not that easy, unfortunately. On one side we have dw-hdmi which
> >>> checks that DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set, and on the
> >>> other side we have other drivers like the IT66121 which will fail if
> >>> this flag is not set.
> >> 
> >> Ok, I see. You have to handle contradicting cases here.
> >> 
> >> Would it be possible to run it with DRM_BRIDGE_ATTACH_NO_CONNECTOR first
> >> and retry if it fails without?
> >> 
> >> But IMHO the return value (in error case) is not well defined. So there
> >> must be a test if a connector has been created (I do not know how this
> >> would work).
> >> 
> >> Another suggestion: can you check if there is a downstream connector defined in
> >> device tree (dw-hdmi does not need such a definition)?
> >> If not we call it with 0 and if there is one we call it with
> >> DRM_BRIDGE_ATTACH_NO_CONNECTOR and create one?
> > 
> > I haven't followed the ful conversation, what the reason why
> > DRM_BRIDGE_ATTACH_NO_CONNECTOR can't always be use here ?
> 
> The synopsys driver creates its own connector through dw_hdmi_connector_create()
> because the IP handles DDC/EDID directly.

That doesn't require creating a connector though. The driver implements
drm_bridge_funcs.get_edid(), which is used to get the EDID without the
need to create a connector in the dw-hdmi driver.

> Hence it checks for ! DRM_BRIDGE_ATTACH_NO_CONNECTOR which seems to be the
> right thing to do on current platforms that use it.
> 
> For CI20/jz4780 we just add a specialisation of the generic dw-hdmi to
> make HDMI work.
> 
> Now this patch for drm/ingenic wants the opposite definition and create its own
> connector. This fails even if we remove the check (then we have two interfering
> connectors).
> 
> > We're moving
> > towards requiring DRM_BRIDGE_ATTACH_NO_CONNECTOR for all new code, so it
> > will have to be done eventually.
> 
> So from my view drm/ingenic wants to already enforce this rule and breaks dw-hdmi.
> 
> IMHO it should either handle this situation gracefully or include a fix for
> dw-hdmi.c to keep it compatible.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[H. Nikolaus Schaller]

Hi Laurent,

> Am 23.09.2021 um 12:03 schrieb Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> 
> Hi Nikolaus,
> 
> On Thu, Sep 23, 2021 at 11:55:56AM +0200, H. Nikolaus Schaller wrote:
>>> Am 23.09.2021 um 11:27 schrieb Laurent Pinchart:
>>> On Thu, Sep 23, 2021 at 11:19:23AM +0200, H. Nikolaus Schaller wrote:
>>>> 
>>>>>>> +		ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
>>>>>>> +					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>>>>> 
>>>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR makes it fundamentally incompatible
>>>>>> with synopsys/dw_hdmi.c
>>>>>> That driver checks for DRM_BRIDGE_ATTACH_NO_CONNECTOR being NOT present,
>>>>>> since it wants to register its own connector through dw_hdmi_connector_create().
>>>>>> It does it for a reason: the dw-hdmi is a multi-function driver which does
>>>>>> HDMI and DDC/EDID stuff in a single driver (because I/O registers and power
>>>>>> management seem to be shared).
>>>>> 
>>>>> The IT66121 driver does all of that too, and does not need
>>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. The drm_bridge_funcs struct has
>>>>> callbacks to handle cable detection and DDC stuff.
>>>>> 
>>>>>> Since I do not see who could split this into a separate bridge and a connector driver
>>>>>> and test it on multiple SoC platforms (there are at least 3 or 4), I think modifying
>>>>>> the fundamentals of the dw-hdmi architecture just to get CI20 HDMI working is not
>>>>>> our turf.
>>>>> 
>>>>> You could have a field in the dw-hdmi pdata structure, that would
>>>>> instruct the driver whether or not it should use the new API. Ugly,
>>>>> I know, and would probably duplicate a lot of code, but that would
>>>>> allow other drivers to be updated at a later date.
>>>> 
>>>> Yes, would be very ugly.
>>>> 
>>>> But generally who has the knowledge (and time) to do this work?
>>>> And has a working platform to test (jz4780 isn't a good development environment)?
>>>> 
>>>> The driver seems to have a turbulent history starting 2013 in staging/imx and
>>>> apparently it was generalized since then... Is Laurent currently dw-hdmi maintainer?
>>> 
>>> "Maintainer" would be an overstatement. I've worked on that driver in
>>> the past, and I still use it, but don't have time to really maintain it.
>>> I've also been told that Synopsys required all patches for that driver
>>> developed using documentation under NDA to be submitted internally to
>>> them first before being published, so I decided to stop contributing
>>> instead of agreeing with this insane process. There's public
>>> documentation about the IP in some NXP reference manuals though, so it
>>> should be possible to still move forward without abiding by this rule.
>>> 
>>>>>> Therefore the code here should be able to detect if drm_bridge_attach() already
>>>>>> creates and attaches a connector and then skip the code below.
>>>>> 
>>>>> Not that easy, unfortunately. On one side we have dw-hdmi which
>>>>> checks that DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set, and on the
>>>>> other side we have other drivers like the IT66121 which will fail if
>>>>> this flag is not set.
>>>> 
>>>> Ok, I see. You have to handle contradicting cases here.
>>>> 
>>>> Would it be possible to run it with DRM_BRIDGE_ATTACH_NO_CONNECTOR first
>>>> and retry if it fails without?
>>>> 
>>>> But IMHO the return value (in error case) is not well defined. So there
>>>> must be a test if a connector has been created (I do not know how this
>>>> would work).
>>>> 
>>>> Another suggestion: can you check if there is a downstream connector defined in
>>>> device tree (dw-hdmi does not need such a definition)?
>>>> If not we call it with 0 and if there is one we call it with
>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR and create one?
>>> 
>>> I haven't followed the ful conversation, what the reason why
>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR can't always be use here ?
>> 
>> The synopsys driver creates its own connector through dw_hdmi_connector_create()
>> because the IP handles DDC/EDID directly.
> 
> That doesn't require creating a connector though. The driver implements
> drm_bridge_funcs.get_edid(), which is used to get the EDID without the
> need to create a connector in the dw-hdmi driver.

Ah, ok.

But then we still have issues.

Firstly I would assume that get_edid only works properly if it is initialized
through dw_hdmi_connector_create().

Next, in the current code, passing DRM_BRIDGE_ATTACH_NO_CONNECTOR to 
dw_hdmi_bridge_attach() indeed does not call dw_hdmi_connector_create()
but returns 0.

This patch 6/6 makes drm/ingenic unconditionally require a connector
to be attached which is defined somewhere else (device tree e.g. "connector-hdmi")
unrelated to dw-hdmi. Current upstream code for drm/ingenic does not init/attach
such a connector on its own so it did work before.

I.e. I think we can't just use parts of dw-hdmi.

If drm_bridge_attach() would return some errno if DRM_BRIDGE_ATTACH_NO_CONNECTOR
is set, initialization in ingenic_drm_bind() would fail likewise with "Unable to attach bridge".

So in any case dw-hdmi is broken by this drm/ingenic patch unless someone
reworks it to make it compatible.

Another issue is that dw_hdmi_connector_create() does not only do dcd/edid
but appears to detects hot plug and does some special initialization.
So we probably loose hotplug detect if we just use drm_bridge_funcs.get_edid().

I come to the conclusion that not creating a specific connector in dw-hdmi
and relying on a generic connector does not seem to be an option with current
code proposals.

In such a situation the question is what the least invasive surgery is to
avoid complications and lenghty regression tests on unknown platforms.
IMHO it is leaving (mature) dw-hdmi untouched and make attachment of a connector
in ingenic_drm_bind() depend on some condition.

BR and thanks,
Nikolaus

Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[H. Nikolaus Schaller]

Hi Laurent,

> IMHO it is leaving (mature) dw-hdmi untouched and make attachment of a connector
> in ingenic_drm_bind() depend on some condition.

Since I don't know details of the DRM bridge/encoder/connector APIs), 
let me reformulate the quersion for a condition specifically.

How can one find out in this code fragment from Paul's patch
if drm_brige_attach() did create a connector or not?

I.e. did call drm_connector_attach_encoder(connector, hdmi->bridge.encoder);
on its own?

@@ -1154,20 +1186,36 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
			bridge = devm_drm_panel_bridge_add_typed(dev, panel,
								 DRM_MODE_CONNECTOR_DPI);

		drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);

-		ret = drm_bridge_attach(encoder, bridge, NULL, 0);
-		if (ret)
+		ib->bridge.funcs = &ingenic_drm_bridge_funcs;
+		ib->next_bridge = bridge;
+
+		ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
+					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+		if (ret) {
+			dev_err(dev, "Unable to attach bridge\n");
			return ret;
+		}
+
+		connector = drm_bridge_connector_init(drm, encoder);
+		if (IS_ERR(connector)) {
+			dev_err(dev, "Unable to init connector\n");
+			return PTR_ERR(connector);
+		}
+
+		drm_connector_attach_encoder(connector, encoder);
	}

A problem may be that "connector" is unknown before drm_bridge_connector_init()
is called.

Then I think I can propose a fallback solution to drm_bridge_attach(, 0) if
drm_bridge_attach(, DRM_BRIDGE_ATTACH_NO_CONNECTOR) fails. 

BR and thanks,
Nikolaus

Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[Paul Cercueil]

Hi Nikolaus,

Le jeu., sept. 23 2021 at 13:41:28 +0200, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Laurent,
> 
>>  Am 23.09.2021 um 12:03 schrieb Laurent Pinchart 
>> <laurent.pinchart@ideasonboard.com>:
>> 
>>  Hi Nikolaus,
>> 
>>  On Thu, Sep 23, 2021 at 11:55:56AM +0200, H. Nikolaus Schaller 
>> wrote:
>>>>  Am 23.09.2021 um 11:27 schrieb Laurent Pinchart:
>>>>  On Thu, Sep 23, 2021 at 11:19:23AM +0200, H. Nikolaus Schaller 
>>>> wrote:
>>>>> 
>>>>>>>>  +		ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
>>>>>>>>  +					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>>>>>> 
>>>>>>>  DRM_BRIDGE_ATTACH_NO_CONNECTOR makes it fundamentally 
>>>>>>> incompatible
>>>>>>>  with synopsys/dw_hdmi.c
>>>>>>>  That driver checks for DRM_BRIDGE_ATTACH_NO_CONNECTOR being 
>>>>>>> NOT present,
>>>>>>>  since it wants to register its own connector through 
>>>>>>> dw_hdmi_connector_create().
>>>>>>>  It does it for a reason: the dw-hdmi is a multi-function 
>>>>>>> driver which does
>>>>>>>  HDMI and DDC/EDID stuff in a single driver (because I/O 
>>>>>>> registers and power
>>>>>>>  management seem to be shared).
>>>>>> 
>>>>>>  The IT66121 driver does all of that too, and does not need
>>>>>>  DRM_BRIDGE_ATTACH_NO_CONNECTOR. The drm_bridge_funcs struct has
>>>>>>  callbacks to handle cable detection and DDC stuff.
>>>>>> 
>>>>>>>  Since I do not see who could split this into a separate bridge 
>>>>>>> and a connector driver
>>>>>>>  and test it on multiple SoC platforms (there are at least 3 or 
>>>>>>> 4), I think modifying
>>>>>>>  the fundamentals of the dw-hdmi architecture just to get CI20 
>>>>>>> HDMI working is not
>>>>>>>  our turf.
>>>>>> 
>>>>>>  You could have a field in the dw-hdmi pdata structure, that 
>>>>>> would
>>>>>>  instruct the driver whether or not it should use the new API. 
>>>>>> Ugly,
>>>>>>  I know, and would probably duplicate a lot of code, but that 
>>>>>> would
>>>>>>  allow other drivers to be updated at a later date.
>>>>> 
>>>>>  Yes, would be very ugly.
>>>>> 
>>>>>  But generally who has the knowledge (and time) to do this work?
>>>>>  And has a working platform to test (jz4780 isn't a good 
>>>>> development environment)?
>>>>> 
>>>>>  The driver seems to have a turbulent history starting 2013 in 
>>>>> staging/imx and
>>>>>  apparently it was generalized since then... Is Laurent currently 
>>>>> dw-hdmi maintainer?
>>>> 
>>>>  "Maintainer" would be an overstatement. I've worked on that 
>>>> driver in
>>>>  the past, and I still use it, but don't have time to really 
>>>> maintain it.
>>>>  I've also been told that Synopsys required all patches for that 
>>>> driver
>>>>  developed using documentation under NDA to be submitted 
>>>> internally to
>>>>  them first before being published, so I decided to stop 
>>>> contributing
>>>>  instead of agreeing with this insane process. There's public
>>>>  documentation about the IP in some NXP reference manuals though, 
>>>> so it
>>>>  should be possible to still move forward without abiding by this 
>>>> rule.
>>>> 
>>>>>>>  Therefore the code here should be able to detect if 
>>>>>>> drm_bridge_attach() already
>>>>>>>  creates and attaches a connector and then skip the code below.
>>>>>> 
>>>>>>  Not that easy, unfortunately. On one side we have dw-hdmi which
>>>>>>  checks that DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set, and on 
>>>>>> the
>>>>>>  other side we have other drivers like the IT66121 which will 
>>>>>> fail if
>>>>>>  this flag is not set.
>>>>> 
>>>>>  Ok, I see. You have to handle contradicting cases here.
>>>>> 
>>>>>  Would it be possible to run it with 
>>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR first
>>>>>  and retry if it fails without?
>>>>> 
>>>>>  But IMHO the return value (in error case) is not well defined. 
>>>>> So there
>>>>>  must be a test if a connector has been created (I do not know 
>>>>> how this
>>>>>  would work).
>>>>> 
>>>>>  Another suggestion: can you check if there is a downstream 
>>>>> connector defined in
>>>>>  device tree (dw-hdmi does not need such a definition)?
>>>>>  If not we call it with 0 and if there is one we call it with
>>>>>  DRM_BRIDGE_ATTACH_NO_CONNECTOR and create one?
>>>> 
>>>>  I haven't followed the ful conversation, what the reason why
>>>>  DRM_BRIDGE_ATTACH_NO_CONNECTOR can't always be use here ?
>>> 
>>>  The synopsys driver creates its own connector through 
>>> dw_hdmi_connector_create()
>>>  because the IP handles DDC/EDID directly.
>> 
>>  That doesn't require creating a connector though. The driver 
>> implements
>>  drm_bridge_funcs.get_edid(), which is used to get the EDID without 
>> the
>>  need to create a connector in the dw-hdmi driver.
> 
> Ah, ok.
> 
> But then we still have issues.
> 
> Firstly I would assume that get_edid only works properly if it is 
> initialized
> through dw_hdmi_connector_create().
> 
> Next, in the current code, passing DRM_BRIDGE_ATTACH_NO_CONNECTOR to
> dw_hdmi_bridge_attach() indeed does not call 
> dw_hdmi_connector_create()
> but returns 0.
> 
> This patch 6/6 makes drm/ingenic unconditionally require a connector
> to be attached which is defined somewhere else (device tree e.g. 
> "connector-hdmi")
> unrelated to dw-hdmi. Current upstream code for drm/ingenic does not 
> init/attach
> such a connector on its own so it did work before.
> 
> I.e. I think we can't just use parts of dw-hdmi.

The fact that Laurent is using dw-hdmi with 
DRM_BRIDGE_ATTACH_NO_CONNECTOR on Renesas makes me think that it's 
possible here as well. There's no reason why it shouldn't work with 
ingenic-drm.

The ingenic-drm driver does not need to create any connector. The 
"connector-hdmi" is connected to dw-hdmi as the "next bridge" in the 
list.

> If drm_bridge_attach() would return some errno if 
> DRM_BRIDGE_ATTACH_NO_CONNECTOR
> is set, initialization in ingenic_drm_bind() would fail likewise with 
> "Unable to attach bridge".
> 
> So in any case dw-hdmi is broken by this drm/ingenic patch unless 
> someone
> reworks it to make it compatible.

Where would the errno be returned? Why would drm_bridge_attach() return 
an error code?

> Another issue is that dw_hdmi_connector_create() does not only do 
> dcd/edid
> but appears to detects hot plug and does some special initialization.
> So we probably loose hotplug detect if we just use 
> drm_bridge_funcs.get_edid().

There's drm_bridge_funcs.detect().

Cheers,
-Paul

> I come to the conclusion that not creating a specific connector in 
> dw-hdmi
> and relying on a generic connector does not seem to be an option with 
> current
> code proposals.
> 
> In such a situation the question is what the least invasive surgery 
> is to
> avoid complications and lenghty regression tests on unknown platforms.
> IMHO it is leaving (mature) dw-hdmi untouched and make attachment of 
> a connector
> in ingenic_drm_bind() depend on some condition.
> 
> BR and thanks,
> Nikolaus
> 
> 

Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[Paul Cercueil]

Le jeu., sept. 23 2021 at 20:52:23 +0200, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Paul,
> 
>>  Am 23.09.2021 um 15:30 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>>  Hi Nikolaus,
>> 
>>  Le jeu., sept. 23 2021 at 13:41:28 +0200, H. Nikolaus Schaller 
>> <hns@goldelico.com> a écrit :
>>>  Hi Laurent,
>>>  Ah, ok.
>>>  But then we still have issues.
>>>  Firstly I would assume that get_edid only works properly if it is 
>>> initialized
>>>  through dw_hdmi_connector_create().
>>>  Next, in the current code, passing DRM_BRIDGE_ATTACH_NO_CONNECTOR 
>>> to
>>>  dw_hdmi_bridge_attach() indeed does not call 
>>> dw_hdmi_connector_create()
>>>  but returns 0.
>>>  This patch 6/6 makes drm/ingenic unconditionally require a 
>>> connector
>>>  to be attached which is defined somewhere else (device tree e.g. 
>>> "connector-hdmi")
>>>  unrelated to dw-hdmi. Current upstream code for drm/ingenic does 
>>> not init/attach
>>>  such a connector on its own so it did work before.
>>>  I.e. I think we can't just use parts of dw-hdmi.
>> 
>>  The fact that Laurent is using dw-hdmi with 
>> DRM_BRIDGE_ATTACH_NO_CONNECTOR on Renesas makes me think that it's 
>> possible here as well. There's no reason why it shouldn't work with 
>> ingenic-drm.
> 
> That is interesting and Laurent can probably comment on differences 
> between
> his setup (I wasn't able to deduce what device you are referring to) 
> and dw-hdmi.
> 
> For jz4780 we tried that first. I do not remember the exact reasons 
> but we wasted
> weeks trying to but failed to get it working. While the dw-hdmi 
> connector simply works
> on top of upstream and fails only if we apply your patch.
> 
> Another issue is how you want to tell connector-hdmi to use the extra 
> i2c bus driver
> for ddc which is not available directly as a standard i2c controller 
> of the jz4780.
> 
> hdmi-connector.yaml defines:
> 
>   ddc-i2c-bus:
> 	description: phandle link to the I2C controller used for DDC EDID 
> probing
> 	$ref: /schemas/types.yaml#/definitions/phandle
> 
> So we would need some ddc-i2c-bus = <&i2c-controller-inside-the 
> dw-hdmi>.
> 
> But that i2c-controller-inside-the dw-hdmi does not exist in device 
> tree
> and can not be added unless someone significantly rewrites dw-hdmi to
> register and expose it as i2c controller.

No, you don't need to do that at all. Just don't set the "ddc-i2c-bus" 
property.

>> 
>>  The ingenic-drm driver does not need to create any connector. The 
>> "connector-hdmi" is connected to dw-hdmi as the "next bridge" in the 
>> list.
> 
> Sure. It does not *create* a connector. It expects that it can safely 
> call
> drm_bridge_connector_init() to get a pointer to a newly created 
> connector.
> 
> But if we use the dw-hdmi connector, there is no such connector and 
> "next bridge".

We don't want to use the dw-hdmi connector. Your "next bridge" is the 
"hdmi-connector" that should be wired in DTS.

> Or can you tell me how to make the dw-hdmi connector created by
> dw_hdmi_connector_create() become the "next bridge" in the list for 
> your driver?
> But without significantly rewriting dw-hdmi.c (and testing).

Wire it to the LCD node in DTS...

See how we do it for the IT66121 driver:
https://github.com/OpenDingux/linux/blob/jz-5.15/arch/mips/boot/dts/ingenic/rg350m.dts#L114-L134

>> 
>>>  If drm_bridge_attach() would return some errno if 
>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR
>>>  is set, initialization in ingenic_drm_bind() would fail likewise 
>>> with "Unable to attach bridge".
>>>  So in any case dw-hdmi is broken by this drm/ingenic patch unless 
>>> someone
>>>  reworks it to make it compatible.
>> 
>>  Where would the errno be returned? Why would drm_bridge_attach() 
>> return an error code?
> 
> Currently dw_hdmi_bridge_attach() returns 0 if it is asked to support
> DRM_BRIDGE_ATTACH_NO_CONNECTOR.
> 
> This is not treated as an error by drm_bridge_attach().
> 
> Here it could return an error (-ENOTSUPP?) instead, to allow for 
> error handling
> by its caller.

And why would you do that? If you don't want to attach a connector, 
then drm_bridge_attach() doesn't need to do much. So it's normal that 
it returns zero.

> But that raises an error message like "failed to attach bridge to 
> encoder" and
> the bridge is reset and detached.
> 
>> 
>>>  Another issue is that dw_hdmi_connector_create() does not only do 
>>> dcd/edid
>>>  but appears to detects hot plug and does some special 
>>> initialization.
>>>  So we probably loose hotplug detect if we just use 
>>> drm_bridge_funcs.get_edid().
>> 
>>  There's drm_bridge_funcs.detect().
> 
> You mean in dw-hdmi? Yes, it calls dw_hdmi_bridge_detect() which 
> calls dw_hdmi_detect().
> This does some read_hpd.
> 
> Anyways, this does not solve the problem that with your drm/ingenic 
> proposal the
> dw-hdmi subsystem (hdmi + ddc) can no longer be initialized properly 
> unless someone
> fixes either.
> 
> So IMHO this should be treated as a significant blocking point for 
> your patch
> because it breaks something that is working upstream and there seems 
> to be no
> rationale to change it.
> 
> Your commit message just says:
> "All the bridges are now attached with 
> DRM_BRIDGE_ATTACH_NO_CONNECTOR."
> but gives no reason why.
> 
> I fully understand that you want to change it and Laurent said that 
> it will become
> standard in the far future. Therefore I suggest to find a way that 
> you can find out
> if a connector has already been created by drm_bridge_attach() to 
> stay compatible
> with current upstream code.

No, absolutely not. There is nothing upstream yet that can bind the 
ingenic-drm driver with the dw-hdmi driver. This is your downstream 
patch. I'm not breaking anything that's upstream, so there is no 
blocking point.

Besides, even with your downstream patch I don't see any reason why the 
dw-hdmi driver wouldn't work with this patch, provided it's wired 
properly, and you never did show a proof of failure either. You come up 
with "possible points where it will fail" but these are based on your 
assumptions on how the drivers should be working together, and I think 
you somehow miss the whole picture.

Start by wiring things properly, like in my previously linked DTS, and 
*test*. If it fails, tell us where it fails. Because your "it doesn't 
work" arguments have zero weight otherwise.

If I can find some time this weekend I will test it myself.

Cheers,
-Paul

> I even want to help here but I don't know how to detect the inverse of
> drm_connector_attach_encoder(), i.e. 
> is_drm_encoder_attached_to_any_connector().
> 
> BR and thanks,
> Nikolaus
> 
> 
> 

Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[H. Nikolaus Schaller]

Hi Paul,


> Am 23.09.2021 um 21:39 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> 
> 
> Le jeu., sept. 23 2021 at 20:52:23 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> Hi Paul,
>>> Am 23.09.2021 um 15:30 schrieb Paul Cercueil <paul@crapouillou.net>:
>>> Hi Nikolaus,
>>> Le jeu., sept. 23 2021 at 13:41:28 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>>>> Hi Laurent,
>>>> Ah, ok.
>>>> But then we still have issues.
>>>> Firstly I would assume that get_edid only works properly if it is initialized
>>>> through dw_hdmi_connector_create().
>>>> Next, in the current code, passing DRM_BRIDGE_ATTACH_NO_CONNECTOR to
>>>> dw_hdmi_bridge_attach() indeed does not call dw_hdmi_connector_create()
>>>> but returns 0.
>>>> This patch 6/6 makes drm/ingenic unconditionally require a connector
>>>> to be attached which is defined somewhere else (device tree e.g. "connector-hdmi")
>>>> unrelated to dw-hdmi. Current upstream code for drm/ingenic does not init/attach
>>>> such a connector on its own so it did work before.
>>>> I.e. I think we can't just use parts of dw-hdmi.
>>> The fact that Laurent is using dw-hdmi with DRM_BRIDGE_ATTACH_NO_CONNECTOR on Renesas makes me think that it's possible here as well. There's no reason why it shouldn't work with ingenic-drm.
>> That is interesting and Laurent can probably comment on differences between
>> his setup (I wasn't able to deduce what device you are referring to) and dw-hdmi.
>> For jz4780 we tried that first. I do not remember the exact reasons but we wasted
>> weeks trying to but failed to get it working. While the dw-hdmi connector simply works
>> on top of upstream and fails only if we apply your patch.
>> Another issue is how you want to tell connector-hdmi to use the extra i2c bus driver
>> for ddc which is not available directly as a standard i2c controller of the jz4780.
>> hdmi-connector.yaml defines:
>>  ddc-i2c-bus:
>> 	description: phandle link to the I2C controller used for DDC EDID probing
>> 	$ref: /schemas/types.yaml#/definitions/phandle
>> So we would need some ddc-i2c-bus = <&i2c-controller-inside-the dw-hdmi>.
>> But that i2c-controller-inside-the dw-hdmi does not exist in device tree
>> and can not be added unless someone significantly rewrites dw-hdmi to
>> register and expose it as i2c controller.
> 
> No, you don't need to do that at all. Just don't set the "ddc-i2c-bus" property.

And then ddc from dw-hdmi works?

> 
>>> The ingenic-drm driver does not need to create any connector. The "connector-hdmi" is connected to dw-hdmi as the "next bridge" in the list.
>> Sure. It does not *create* a connector. It expects that it can safely call
>> drm_bridge_connector_init() to get a pointer to a newly created connector.
>> But if we use the dw-hdmi connector, there is no such connector and "next bridge".
> 
> We don't want to use the dw-hdmi connector. Your "next bridge" is the "hdmi-connector" that should be wired in DTS.
> 
>> Or can you tell me how to make the dw-hdmi connector created by
>> dw_hdmi_connector_create() become the "next bridge" in the list for your driver?
>> But without significantly rewriting dw-hdmi.c (and testing).
> 
> Wire it to the LCD node in DTS...
> 
> See how we do it for the IT66121 driver:
> https://github.com/OpenDingux/linux/blob/jz-5.15/arch/mips/boot/dts/ingenic/rg350m.dts#L114-L134

That is how we already tried.

> 
>>>> If drm_bridge_attach() would return some errno if DRM_BRIDGE_ATTACH_NO_CONNECTOR
>>>> is set, initialization in ingenic_drm_bind() would fail likewise with "Unable to attach bridge".
>>>> So in any case dw-hdmi is broken by this drm/ingenic patch unless someone
>>>> reworks it to make it compatible.
>>> Where would the errno be returned? Why would drm_bridge_attach() return an error code?
>> Currently dw_hdmi_bridge_attach() returns 0 if it is asked to support
>> DRM_BRIDGE_ATTACH_NO_CONNECTOR.
>> This is not treated as an error by drm_bridge_attach().
>> Here it could return an error (-ENOTSUPP?) instead, to allow for error handling
>> by its caller.
> 
> And why would you do that? If you don't want to attach a connector, then drm_bridge_attach() doesn't need to do much. So it's normal that it returns zero.
> 
>> But that raises an error message like "failed to attach bridge to encoder" and
>> the bridge is reset and detached.
>>>> Another issue is that dw_hdmi_connector_create() does not only do dcd/edid
>>>> but appears to detects hot plug and does some special initialization.
>>>> So we probably loose hotplug detect if we just use drm_bridge_funcs.get_edid().
>>> There's drm_bridge_funcs.detect().
>> You mean in dw-hdmi? Yes, it calls dw_hdmi_bridge_detect() which calls dw_hdmi_detect().
>> This does some read_hpd.
>> Anyways, this does not solve the problem that with your drm/ingenic proposal the
>> dw-hdmi subsystem (hdmi + ddc) can no longer be initialized properly unless someone
>> fixes either.
>> So IMHO this should be treated as a significant blocking point for your patch
>> because it breaks something that is working upstream and there seems to be no
>> rationale to change it.
>> Your commit message just says:
>> "All the bridges are now attached with DRM_BRIDGE_ATTACH_NO_CONNECTOR."
>> but gives no reason why.
>> I fully understand that you want to change it and Laurent said that it will become
>> standard in the far future. Therefore I suggest to find a way that you can find out
>> if a connector has already been created by drm_bridge_attach() to stay compatible
>> with current upstream code.
> 
> No, absolutely not. There is nothing upstream yet that can bind the ingenic-drm driver with the dw-hdmi driver.

We have proposed it a while ago using nothing special.

> This is your downstream patch. I'm not breaking anything that's upstream, so there is no blocking point.
> 
> Besides, even with your downstream patch I don't see any reason why the dw-hdmi driver wouldn't work with this patch, provided it's wired properly, and you never did show a proof of failure either. You come up with "possible points where it will fail" but these are based on your assumptions on how the drivers should be working together, and I think you somehow miss the whole picture.

Yes, maybe.

> 
> Start by wiring things properly, like in my previously linked DTS, and *test*. If it fails, tell us where it fails.

Well, I tell where drm_bridge_attach fails with DRM_BRIDGE_ATTACH_NO_CONNECTOR...

> Because your "it doesn't work" arguments have zero weight otherwise.

I hope I still can find it. So I can't promise anything.
We have had it complete in DTS and added code to parse it.
It may have been wiped out by cleaning up patch series during rebase.

> 
> If I can find some time this weekend I will test it myself.

You may be faster than me.

BR and thanks,
Nikolaus

Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[Paul Boddie]

On Thursday, 23 September 2021 22:23:28 CEST H. Nikolaus Schaller wrote:
> 
> > Am 23.09.2021 um 21:39 schrieb Paul Cercueil <paul@crapouillou.net>:
> > 
> > Start by wiring things properly, like in my previously linked DTS, and
> > *test*. If it fails, tell us where it fails.
>
> Well, I tell where drm_bridge_attach fails with
> DRM_BRIDGE_ATTACH_NO_CONNECTOR...

I tried to piece together this entire discussion from the mailing list 
archives, but there appear to be two approaches that "work", in that they 
activate the LCD controller with the HDMI peripheral:

1. Nikolaus's approach, which involves getting the Synopsys driver to create a 
connector and then avoiding the call to drm_bridge_connector_init in the 
Ingenic DRM driver.

2. My approach, which just involves changing the Synopsys driver to set the 
bridge type in dw_hdmi_probe like this:

  hdmi->bridge.type = DRM_MODE_CONNECTOR_HDMIA;

Otherwise, I don't see how the bridge's (struct drm_bridge) type will be set. 
And this causes drm_bridge_connector_init to fail because it tests the bridge 
type.

Now, I just reintroduced the HDMI connector to the device tree as follows:

        hdmi_connector {
                compatible = "hdmi-connector";
                label = "hdmi";

                type = "a";

                port {
                        hdmi_connector_in: endpoint {
                                remote-endpoint = <&dw_hdmi_out>;
                        };
                };
        };

And then I added a second port to the HDMI peripheral node as follows:

                port@1 {
                        reg = <1>;
                        dw_hdmi_out: endpoint {
                                remote-endpoint = <&hdmi_connector_in>;
                        };
                };

And I removed any of the above hacks. What I observe, apart from an inactive 
LCD controller (and ingenic-drm driver), is the following in /sys/devices/
platform/10180000.hdmi/:

consumer:platform:13050000.lcdc0
consumer:platform:hdmi_connector

Maybe I don't understand the significance of "consumer" here, but the LCD 
controller and the HDMI connector obviously have rather different roles. Then 
again, the device tree is defining bidirectional relationships, so maybe this 
is how they manifest themselves.

> > Because your "it doesn't work" arguments have zero weight otherwise.
> 
> I hope I still can find it. So I can't promise anything.
> We have had it complete in DTS and added code to parse it.
> It may have been wiped out by cleaning up patch series during rebase.

I suppose the question is whether this is actually handled already. I would 
have thought that either the DRM framework would be able to identify such 
relationships in a generic way or that the Synopsys driver would need to do 
so. This might actually be happening, given that the sysfs entries are there, 
but I might also imagine that something extra would be required to set the 
bridge type.

I did start writing some code to look up a remote endpoint for the second 
port, find the connector type, and then set it, but it was probably after 
midnight on that occasion as well. Short-circuiting this little dance and 
setting the bridge type indicated that this might ultimately be the right 
approach, but it would probably also mean introducing a point of 
specialisation to the Synopsys driver so that device-specific drivers can 
define a function to set the connector type.

Otherwise, I can't see the Synopsys driver working for devices like the 
JZ4780, but then again, it seems that all the other devices seem to 
incorporate the Synopsys functionality in a different way and do not need to 
deal with connectors at all.

> > If I can find some time this weekend I will test it myself.
> 
> You may be faster than me.

So, when I wrote about approaches that "work", I can seemingly get the LCD 
controller and HDMI peripheral registers set up to match a non-Linux 
environment where I can demonstrate a functioning display, and yet I don't get 
a valid signal in the Linux environment.

Nikolaus can actually get HDMI output, but there may be other factors 
introduced by the Linux environment that frustrate success for me, whereas my 
non-Linux environment is much simpler and can reliably reproduce a successful 
result.

For me, running modetest yields plenty of information about encoders, 
connectors (and the supported modes via the EDID, thanks to my HDMI-A hack), 
CRTCs, and planes. But no framebuffers are reported.

Paul

Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[Paul Cercueil]

Hi Paul,

Le ven., sept. 24 2021 at 00:51:39 +0200, Paul Boddie 
<paul@boddie.org.uk> a écrit :
> On Thursday, 23 September 2021 22:23:28 CEST H. Nikolaus Schaller 
> wrote:
>> 
>>  > Am 23.09.2021 um 21:39 schrieb Paul Cercueil 
>> <paul@crapouillou.net>:
>>  >
>>  > Start by wiring things properly, like in my previously linked 
>> DTS, and
>>  > *test*. If it fails, tell us where it fails.
>> 
>>  Well, I tell where drm_bridge_attach fails with
>>  DRM_BRIDGE_ATTACH_NO_CONNECTOR...
> 
> I tried to piece together this entire discussion from the mailing list
> archives, but there appear to be two approaches that "work", in that 
> they
> activate the LCD controller with the HDMI peripheral:
> 
> 1. Nikolaus's approach, which involves getting the Synopsys driver to 
> create a
> connector and then avoiding the call to drm_bridge_connector_init in 
> the
> Ingenic DRM driver.
> 
> 2. My approach, which just involves changing the Synopsys driver to 
> set the
> bridge type in dw_hdmi_probe like this:
> 
>   hdmi->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
> 
> Otherwise, I don't see how the bridge's (struct drm_bridge) type will 
> be set.

The bridge's type is set in hdmi-connector, from DTS. The 'type = "a"' 
will result in the bridge's .type to be set to DRM_MODE_CONNECTOR_HDMIA.

> And this causes drm_bridge_connector_init to fail because it tests 
> the bridge
> type.
> 
> Now, I just reintroduced the HDMI connector to the device tree as 
> follows:
> 
>         hdmi_connector {
>                 compatible = "hdmi-connector";
>                 label = "hdmi";
> 
>                 type = "a";
> 
>                 port {
>                         hdmi_connector_in: endpoint {
>                                 remote-endpoint = <&dw_hdmi_out>;
>                         };
>                 };
>         };
> 
> And then I added a second port to the HDMI peripheral node as follows:
> 
>                 port@1 {
>                         reg = <1>;
>                         dw_hdmi_out: endpoint {
>                                 remote-endpoint = 
> <&hdmi_connector_in>;
>                         };
>                 };
> 
> And I removed any of the above hacks. What I observe, apart from an 
> inactive
> LCD controller (and ingenic-drm driver), is the following in 
> /sys/devices/
> platform/10180000.hdmi/:
> 
> consumer:platform:13050000.lcdc0
> consumer:platform:hdmi_connector
> 
> Maybe I don't understand the significance of "consumer" here, but the 
> LCD
> controller and the HDMI connector obviously have rather different 
> roles. Then
> again, the device tree is defining bidirectional relationships, so 
> maybe this
> is how they manifest themselves.
> 
>>  > Because your "it doesn't work" arguments have zero weight 
>> otherwise.
>> 
>>  I hope I still can find it. So I can't promise anything.
>>  We have had it complete in DTS and added code to parse it.
>>  It may have been wiped out by cleaning up patch series during 
>> rebase.
> 
> I suppose the question is whether this is actually handled already. I 
> would
> have thought that either the DRM framework would be able to identify 
> such
> relationships in a generic way or that the Synopsys driver would need 
> to do
> so. This might actually be happening, given that the sysfs entries 
> are there,
> but I might also imagine that something extra would be required to 
> set the
> bridge type.
> 
> I did start writing some code to look up a remote endpoint for the 
> second
> port, find the connector type, and then set it, but it was probably 
> after
> midnight on that occasion as well. Short-circuiting this little dance 
> and
> setting the bridge type indicated that this might ultimately be the 
> right
> approach, but it would probably also mean introducing a point of
> specialisation to the Synopsys driver so that device-specific drivers 
> can
> define a function to set the connector type.
> 
> Otherwise, I can't see the Synopsys driver working for devices like 
> the
> JZ4780, but then again, it seems that all the other devices seem to
> incorporate the Synopsys functionality in a different way and do not 
> need to
> deal with connectors at all.
> 
>>  > If I can find some time this weekend I will test it myself.
>> 
>>  You may be faster than me.
> 
> So, when I wrote about approaches that "work", I can seemingly get 
> the LCD
> controller and HDMI peripheral registers set up to match a non-Linux
> environment where I can demonstrate a functioning display, and yet I 
> don't get
> a valid signal in the Linux environment.
> 
> Nikolaus can actually get HDMI output, but there may be other factors
> introduced by the Linux environment that frustrate success for me, 
> whereas my
> non-Linux environment is much simpler and can reliably reproduce a 
> successful
> result.
> 
> For me, running modetest yields plenty of information about encoders,
> connectors (and the supported modes via the EDID, thanks to my HDMI-A 
> hack),
> CRTCs, and planes. But no framebuffers are reported.

Could you paste the result of "modetest -a -c -p" somewhere maybe?

If you have info about the CRTCs, encoders, connectors and EDID info, 
then I would assume it is very close to working fine.

For your "no framebuffer" issue, keep in mind that CONFIG_FB and 
CONFIG_FRAMEBUFFER_CONSOLE are now disabled by default.

If that doesn't fix anything, that probably means that one 
.atomic_check() fails, so it would be a good place to start debugging.

Cheers,
-Paul

Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[H. Nikolaus Schaller]

Hi Paul,

> Am 23.09.2021 um 22:23 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> 
>> Because your "it doesn't work" arguments have zero weight otherwise.
> 
> I hope I still can find it. So I can't promise anything.
> We have had it complete in DTS and added code to parse it.
> It may have been wiped out by cleaning up patch series during rebase.

I was able to locate it and place it on top of your ingenic-drm-drv v3
and our synopsys hdmi v3 [1] (+ unpublished work).

This [2] should save you a lot of time making dw-hdmi work on jz4780 at all, so you can
focus on our mistakes instead of starting from scratch.

Features:
- based on v5.15-rc2
- (the first two patches are LetuxOS and build system related and can be ignored for this discussion)
- contains some significant patch from drm-next not yet upstream
- contains your v3 series as is
- (initially) disables your DRM_BRIDGE_ATTACH_NO_CONNECTOR (is reverted in the last patch)
- adds synopsys stuff and DT schema
- adds jz4780.dtsi and ci20.dts
- adds ci20_defconfig
- (adds some (optional) jz4780 specific features we likely do not need now)
- adds something to dw-hdmi to properly notify HPD
- adds a hdmi-regulator so that HPD power can be turned on/off
- (attempt to configure the dw-hdmi unwedge feature)
- then we add the hdmi-connector to replace the dw-hdmi connector to device tree
- and finally re-enable DRM_BRIDGE_ATTACH_NO_CONNECTOR

The result is
a) without the last patch I get a proper setup with framebuffer and edid.
Unfortunateley without any image on HDMI.

b) if last patch is included
(so that DRM_BRIDGE_ATTACH_NO_CONNECTOR is required as by your [patch v3 6/6] again)
I get:

[    4.351200] [drm:drm_bridge_attach [drm]] *ERROR* failed to attach bridge /hdmi@10180000 to encoder DPI-34: -22
[    4.474346] [drm:drm_bridge_attach [drm]] *ERROR* failed to attach bridge (null) to encoder DPI-34: -22
[    4.562125] ingenic-drm 13050000.lcdc0: Unable to attach bridge
[    4.568103] ingenic-drm: probe of 13050000.lcdc0 failed with error -22

Maybe you can spot the bug in the code much quicker than we can.

I do not know what Paul Boddie did differently if this initialization
with connector-hdmi works for him and does not fail likewise.

BR and thanks,
Nikolaus


[1]: https://lore.kernel.org/linux-mips/8e873f17fcc9aeb326d99b7c2c8cd25b61dca6f5.1628399442.git.hns@goldelico.com/T/
[2]: https://git.goldelico.com/?p=letux-kernel.git;a=shortlog;h=refs/heads/upstream%2Bjz4780%2Bhdmi-connector

Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[Paul Boddie]

On Friday, 24 September 2021 10:29:02 CEST Paul Cercueil wrote:
> 
> Le ven., sept. 24 2021 at 00:51:39 +0200, Paul Boddie
> >
> > 2. My approach, which just involves changing the Synopsys driver to
> > set the bridge type in dw_hdmi_probe like this:
> >
> >   hdmi->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
> > 
> > Otherwise, I don't see how the bridge's (struct drm_bridge) type will
> > be set.
> 
> The bridge's type is set in hdmi-connector, from DTS. The 'type = "a"'
> will result in the bridge's .type to be set to DRM_MODE_CONNECTOR_HDMIA.

Actually, I found that hdmi-connector might not have been available because 
CONFIG_DRM_DISPLAY_CONNECTOR was not enabled. Rectifying this, the connector 
does get detected and enabled. However, the Synopsys driver remains unaware of 
it, and so the bridge type in the Synopsys driver remains unset.

I do see that the connector sets the type on a bridge in its own private 
structure, so there would be a need to propagate this type to the actual 
bridge. In other words, what the connector does is distinct from the Synopsys 
driver which acts as the bridge with regard to the Ingenic driver.

Perhaps the Synopsys driver should set the connector's bridge as the next 
bridge, or maybe something is supposed to discover that the connector may act 
as (or provide) a bridge after the Synopsys driver in the chain and then back-
propagate the bridge type along the chain.

[...]

> > And I removed any of the above hacks. What I observe, apart from an
> > inactive LCD controller (and ingenic-drm driver), is the following in
> > /sys/devices/platform/10180000.hdmi/:
> > 
> > consumer:platform:13050000.lcdc0
> > consumer:platform:hdmi_connector

Interestingly, with the connector driver present, these sysfs entries no 
longer appear.

[...]

> > For me, running modetest yields plenty of information about encoders,
> > connectors (and the supported modes via the EDID, thanks to my HDMI-A
> > hack), CRTCs, and planes. But no framebuffers are reported.
> 
> Could you paste the result of "modetest -a -c -p" somewhere maybe?

I had to specify -M ingenic-drm as well, but here you go...

----

Connectors:
id	encoder	status		name		size (mm)	modes	encoders
35	34	connected	HDMI-A-1       	340x270		17	34
  modes:
	index name refresh (Hz) hdisp hss hse htot vdisp vss vse vtot)
  #0 1280x1024 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 flags: 
phsync, pvsync; type: preferred, driver
  #1 1280x1024 75.02 1280 1296 1440 1688 1024 1025 1028 1066 135000 flags: 
phsync, pvsync; type: driver
  #2 1280x960 60.00 1280 1376 1488 1800 960 961 964 1000 108000 flags: phsync, 
pvsync; type: driver
  #3 1152x864 75.00 1152 1216 1344 1600 864 865 868 900 108000 flags: phsync, 
pvsync; type: driver
  #4 1024x768 75.03 1024 1040 1136 1312 768 769 772 800 78750 flags: phsync, 
pvsync; type: driver
  #5 1024x768 70.07 1024 1048 1184 1328 768 771 777 806 75000 flags: nhsync, 
nvsync; type: driver
  #6 1024x768 60.00 1024 1048 1184 1344 768 771 777 806 65000 flags: nhsync, 
nvsync; type: driver
  #7 832x624 74.55 832 864 928 1152 624 625 628 667 57284 flags: nhsync, 
nvsync; type: driver
  #8 800x600 75.00 800 816 896 1056 600 601 604 625 49500 flags: phsync, 
pvsync; type: driver
  #9 800x600 72.19 800 856 976 1040 600 637 643 666 50000 flags: phsync, 
pvsync; type: driver
  #10 800x600 60.32 800 840 968 1056 600 601 605 628 40000 flags: phsync, 
pvsync; type: driver
  #11 800x600 56.25 800 824 896 1024 600 601 603 625 36000 flags: phsync, 
pvsync; type: driver
  #12 640x480 75.00 640 656 720 840 480 481 484 500 31500 flags: nhsync, 
nvsync; type: driver
  #13 640x480 72.81 640 664 704 832 480 489 492 520 31500 flags: nhsync, 
nvsync; type: driver
  #14 640x480 66.67 640 704 768 864 480 483 486 525 30240 flags: nhsync, 
nvsync; type: driver
  #15 640x480 59.94 640 656 752 800 480 490 492 525 25175 flags: nhsync, 
nvsync; type: driver
  #16 720x400 70.08 720 738 846 900 400 412 414 449 28320 flags: nhsync, 
pvsync; type: driver
  props:
	1 EDID:
		flags: immutable blob
		blobs:

		value:
			00ffffffffffff00047232ad01010101
			2d0e010380221b782aaea5a6544c9926
			145054bfef0081808140714f01010101
			010101010101302a009851002a403070
			1300520e1100001e000000ff00343435
			3030353444454330300a000000fc0041
			4c313731350a202020202020000000fd
			00384c1e520e000a2020202020200051
	2 DPMS:
		flags: enum
		enums: On=0 Standby=1 Suspend=2 Off=3
		value: 0
	5 link-status:
		flags: enum
		enums: Good=0 Bad=1
		value: 0
	6 non-desktop:
		flags: immutable range
		values: 0 1
		value: 0
	4 TILE:
		flags: immutable blob
		blobs:

		value:
	20 CRTC_ID:
		flags: object
		value: 32

CRTCs:
id	fb	pos	size
32	39	(0,0)	(1280x1024)
  #0  60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 flags: phsync, 
pvsync; type: 
  props:
	22 ACTIVE:
		flags: range
		values: 0 1
		value: 1
	23 MODE_ID:
		flags: blob
		blobs:

		value:
			e0a5010000053005a005980600000004
			010404042a0400003c00000005000000
			00000000000000000000000000000000
			00000000000000000000000000000000
			00000000
	19 OUT_FENCE_PTR:
		flags: range
		values: 0 18446744073709551615
		value: 0
	24 VRR_ENABLED:
		flags: range
		values: 0 1
		value: 0
	28 GAMMA_LUT:
		flags: blob
		blobs:

		value:
	29 GAMMA_LUT_SIZE:
		flags: immutable range
		values: 0 4294967295
		value: 256

Planes:
id	crtc	fb	CRTC x,y	x,y	gamma size	possible crtcs
31	32	39	0,0		0,0	0       	0x00000001
  formats: XR15 RG16 RG24 XR24 XR30
  props:
	8 type:
		flags: immutable enum
		enums: Overlay=0 Primary=1 Cursor=2
		value: 1
	17 FB_ID:
		flags: object
		value: 39
	18 IN_FENCE_FD:
		flags: signed range
		values: -1 2147483647
		value: -1
	20 CRTC_ID:
		flags: object
		value: 32
	13 CRTC_X:
		flags: signed range
		values: -2147483648 2147483647
		value: 0
	14 CRTC_Y:
		flags: signed range
		values: -2147483648 2147483647
		value: 0
	15 CRTC_W:
		flags: range
		values: 0 2147483647
		value: 1280
	16 CRTC_H:
		flags: range
		values: 0 2147483647
		value: 1024
	9 SRC_X:
		flags: range
		values: 0 4294967295
		value: 0
	10 SRC_Y:
		flags: range
		values: 0 4294967295
		value: 0
	11 SRC_W:
		flags: range
		values: 0 4294967295
		value: 83886080
	12 SRC_H:
		flags: range
		values: 0 4294967295
		value: 67108864
33	0	0	0,0		0,0	0       	0x00000001
  formats: C8   XR15 RG16 RG24 XR24 XR30
  props:
	8 type:
		flags: immutable enum
		enums: Overlay=0 Primary=1 Cursor=2
		value: 0
	17 FB_ID:
		flags: object
		value: 0
	18 IN_FENCE_FD:
		flags: signed range
		values: -1 2147483647
		value: -1
	20 CRTC_ID:
		flags: object
		value: 0
	13 CRTC_X:
		flags: signed range
		values: -2147483648 2147483647
		value: 0
	14 CRTC_Y:
		flags: signed range
		values: -2147483648 2147483647
		value: 0
	15 CRTC_W:
		flags: range
		values: 0 2147483647
		value: 0
	16 CRTC_H:
		flags: range
		values: 0 2147483647
		value: 0
	9 SRC_X:
		flags: range
		values: 0 4294967295
		value: 0
	10 SRC_Y:
		flags: range
		values: 0 4294967295
		value: 0
	11 SRC_W:
		flags: range
		values: 0 4294967295
		value: 0
	12 SRC_H:
		flags: range
		values: 0 4294967295
		value: 0

----

> If you have info about the CRTCs, encoders, connectors and EDID info,
> then I would assume it is very close to working fine.
> 
> For your "no framebuffer" issue, keep in mind that CONFIG_FB and
> CONFIG_FRAMEBUFFER_CONSOLE are now disabled by default.

Yes, I discovered that CONFIG_FB was not enabled, so I did so.

> If that doesn't fix anything, that probably means that one
> .atomic_check() fails, so it would be a good place to start debugging.

There will be other things to verify in the Ingenic driver. As noted many 
months ago, colour depth information has to be set in the DMA descriptors and 
not the control register, but we are managing to do this successfully, as far 
as I can tell, although there is always the potential for error.

Paul

Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[Paul Cercueil]

Hi Paul & Nikolaus,

If you spent some time debugging the issue instead of complaining that 
my patchset breaks things...

The fix is a one-liner in your downstream ingenic-dw-hdmi.c:
.output_port = 1
in the ingenic_dw_hdmi_plat_data struct.

Absolutely nothing else needs to be changed for HDMI to work here.

Cheers,
-Paul


Le sam., sept. 25 2021 at 17:55:03 +0200, Paul Boddie 
<paul@boddie.org.uk> a écrit :
> On Friday, 24 September 2021 10:29:02 CEST Paul Cercueil wrote:
>> 
>>  Le ven., sept. 24 2021 at 00:51:39 +0200, Paul Boddie
>>  >
>>  > 2. My approach, which just involves changing the Synopsys driver 
>> to
>>  > set the bridge type in dw_hdmi_probe like this:
>>  >
>>  >   hdmi->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
>>  >
>>  > Otherwise, I don't see how the bridge's (struct drm_bridge) type 
>> will
>>  > be set.
>> 
>>  The bridge's type is set in hdmi-connector, from DTS. The 'type = 
>> "a"'
>>  will result in the bridge's .type to be set to 
>> DRM_MODE_CONNECTOR_HDMIA.
> 
> Actually, I found that hdmi-connector might not have been available 
> because
> CONFIG_DRM_DISPLAY_CONNECTOR was not enabled. Rectifying this, the 
> connector
> does get detected and enabled. However, the Synopsys driver remains 
> unaware of
> it, and so the bridge type in the Synopsys driver remains unset.
> 
> I do see that the connector sets the type on a bridge in its own 
> private
> structure, so there would be a need to propagate this type to the 
> actual
> bridge. In other words, what the connector does is distinct from the 
> Synopsys
> driver which acts as the bridge with regard to the Ingenic driver.
> 
> Perhaps the Synopsys driver should set the connector's bridge as the 
> next
> bridge, or maybe something is supposed to discover that the connector 
> may act
> as (or provide) a bridge after the Synopsys driver in the chain and 
> then back-
> propagate the bridge type along the chain.
> 
> [...]
> 
>>  > And I removed any of the above hacks. What I observe, apart from 
>> an
>>  > inactive LCD controller (and ingenic-drm driver), is the 
>> following in
>>  > /sys/devices/platform/10180000.hdmi/:
>>  >
>>  > consumer:platform:13050000.lcdc0
>>  > consumer:platform:hdmi_connector
> 
> Interestingly, with the connector driver present, these sysfs entries 
> no
> longer appear.
> 
> [...]
> 
>>  > For me, running modetest yields plenty of information about 
>> encoders,
>>  > connectors (and the supported modes via the EDID, thanks to my 
>> HDMI-A
>>  > hack), CRTCs, and planes. But no framebuffers are reported.
>> 
>>  Could you paste the result of "modetest -a -c -p" somewhere maybe?
> 
> I had to specify -M ingenic-drm as well, but here you go...
> 
> ----
> 
> Connectors:
> id	encoder	status		name		size (mm)	modes	encoders
> 35	34	connected	HDMI-A-1       	340x270		17	34
>   modes:
> 	index name refresh (Hz) hdisp hss hse htot vdisp vss vse vtot)
>   #0 1280x1024 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 
> flags:
> phsync, pvsync; type: preferred, driver
>   #1 1280x1024 75.02 1280 1296 1440 1688 1024 1025 1028 1066 135000 
> flags:
> phsync, pvsync; type: driver
>   #2 1280x960 60.00 1280 1376 1488 1800 960 961 964 1000 108000 
> flags: phsync,
> pvsync; type: driver
>   #3 1152x864 75.00 1152 1216 1344 1600 864 865 868 900 108000 flags: 
> phsync,
> pvsync; type: driver
>   #4 1024x768 75.03 1024 1040 1136 1312 768 769 772 800 78750 flags: 
> phsync,
> pvsync; type: driver
>   #5 1024x768 70.07 1024 1048 1184 1328 768 771 777 806 75000 flags: 
> nhsync,
> nvsync; type: driver
>   #6 1024x768 60.00 1024 1048 1184 1344 768 771 777 806 65000 flags: 
> nhsync,
> nvsync; type: driver
>   #7 832x624 74.55 832 864 928 1152 624 625 628 667 57284 flags: 
> nhsync,
> nvsync; type: driver
>   #8 800x600 75.00 800 816 896 1056 600 601 604 625 49500 flags: 
> phsync,
> pvsync; type: driver
>   #9 800x600 72.19 800 856 976 1040 600 637 643 666 50000 flags: 
> phsync,
> pvsync; type: driver
>   #10 800x600 60.32 800 840 968 1056 600 601 605 628 40000 flags: 
> phsync,
> pvsync; type: driver
>   #11 800x600 56.25 800 824 896 1024 600 601 603 625 36000 flags: 
> phsync,
> pvsync; type: driver
>   #12 640x480 75.00 640 656 720 840 480 481 484 500 31500 flags: 
> nhsync,
> nvsync; type: driver
>   #13 640x480 72.81 640 664 704 832 480 489 492 520 31500 flags: 
> nhsync,
> nvsync; type: driver
>   #14 640x480 66.67 640 704 768 864 480 483 486 525 30240 flags: 
> nhsync,
> nvsync; type: driver
>   #15 640x480 59.94 640 656 752 800 480 490 492 525 25175 flags: 
> nhsync,
> nvsync; type: driver
>   #16 720x400 70.08 720 738 846 900 400 412 414 449 28320 flags: 
> nhsync,
> pvsync; type: driver
>   props:
> 	1 EDID:
> 		flags: immutable blob
> 		blobs:
> 
> 		value:
> 			00ffffffffffff00047232ad01010101
> 			2d0e010380221b782aaea5a6544c9926
> 			145054bfef0081808140714f01010101
> 			010101010101302a009851002a403070
> 			1300520e1100001e000000ff00343435
> 			3030353444454330300a000000fc0041
> 			4c313731350a202020202020000000fd
> 			00384c1e520e000a2020202020200051
> 	2 DPMS:
> 		flags: enum
> 		enums: On=0 Standby=1 Suspend=2 Off=3
> 		value: 0
> 	5 link-status:
> 		flags: enum
> 		enums: Good=0 Bad=1
> 		value: 0
> 	6 non-desktop:
> 		flags: immutable range
> 		values: 0 1
> 		value: 0
> 	4 TILE:
> 		flags: immutable blob
> 		blobs:
> 
> 		value:
> 	20 CRTC_ID:
> 		flags: object
> 		value: 32
> 
> CRTCs:
> id	fb	pos	size
> 32	39	(0,0)	(1280x1024)
>   #0  60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 flags: 
> phsync,
> pvsync; type:
>   props:
> 	22 ACTIVE:
> 		flags: range
> 		values: 0 1
> 		value: 1
> 	23 MODE_ID:
> 		flags: blob
> 		blobs:
> 
> 		value:
> 			e0a5010000053005a005980600000004
> 			010404042a0400003c00000005000000
> 			00000000000000000000000000000000
> 			00000000000000000000000000000000
> 			00000000
> 	19 OUT_FENCE_PTR:
> 		flags: range
> 		values: 0 18446744073709551615
> 		value: 0
> 	24 VRR_ENABLED:
> 		flags: range
> 		values: 0 1
> 		value: 0
> 	28 GAMMA_LUT:
> 		flags: blob
> 		blobs:
> 
> 		value:
> 	29 GAMMA_LUT_SIZE:
> 		flags: immutable range
> 		values: 0 4294967295
> 		value: 256
> 
> Planes:
> id	crtc	fb	CRTC x,y	x,y	gamma size	possible crtcs
> 31	32	39	0,0		0,0	0       	0x00000001
>   formats: XR15 RG16 RG24 XR24 XR30
>   props:
> 	8 type:
> 		flags: immutable enum
> 		enums: Overlay=0 Primary=1 Cursor=2
> 		value: 1
> 	17 FB_ID:
> 		flags: object
> 		value: 39
> 	18 IN_FENCE_FD:
> 		flags: signed range
> 		values: -1 2147483647
> 		value: -1
> 	20 CRTC_ID:
> 		flags: object
> 		value: 32
> 	13 CRTC_X:
> 		flags: signed range
> 		values: -2147483648 2147483647
> 		value: 0
> 	14 CRTC_Y:
> 		flags: signed range
> 		values: -2147483648 2147483647
> 		value: 0
> 	15 CRTC_W:
> 		flags: range
> 		values: 0 2147483647
> 		value: 1280
> 	16 CRTC_H:
> 		flags: range
> 		values: 0 2147483647
> 		value: 1024
> 	9 SRC_X:
> 		flags: range
> 		values: 0 4294967295
> 		value: 0
> 	10 SRC_Y:
> 		flags: range
> 		values: 0 4294967295
> 		value: 0
> 	11 SRC_W:
> 		flags: range
> 		values: 0 4294967295
> 		value: 83886080
> 	12 SRC_H:
> 		flags: range
> 		values: 0 4294967295
> 		value: 67108864
> 33	0	0	0,0		0,0	0       	0x00000001
>   formats: C8   XR15 RG16 RG24 XR24 XR30
>   props:
> 	8 type:
> 		flags: immutable enum
> 		enums: Overlay=0 Primary=1 Cursor=2
> 		value: 0
> 	17 FB_ID:
> 		flags: object
> 		value: 0
> 	18 IN_FENCE_FD:
> 		flags: signed range
> 		values: -1 2147483647
> 		value: -1
> 	20 CRTC_ID:
> 		flags: object
> 		value: 0
> 	13 CRTC_X:
> 		flags: signed range
> 		values: -2147483648 2147483647
> 		value: 0
> 	14 CRTC_Y:
> 		flags: signed range
> 		values: -2147483648 2147483647
> 		value: 0
> 	15 CRTC_W:
> 		flags: range
> 		values: 0 2147483647
> 		value: 0
> 	16 CRTC_H:
> 		flags: range
> 		values: 0 2147483647
> 		value: 0
> 	9 SRC_X:
> 		flags: range
> 		values: 0 4294967295
> 		value: 0
> 	10 SRC_Y:
> 		flags: range
> 		values: 0 4294967295
> 		value: 0
> 	11 SRC_W:
> 		flags: range
> 		values: 0 4294967295
> 		value: 0
> 	12 SRC_H:
> 		flags: range
> 		values: 0 4294967295
> 		value: 0
> 
> ----
> 
>>  If you have info about the CRTCs, encoders, connectors and EDID 
>> info,
>>  then I would assume it is very close to working fine.
>> 
>>  For your "no framebuffer" issue, keep in mind that CONFIG_FB and
>>  CONFIG_FRAMEBUFFER_CONSOLE are now disabled by default.
> 
> Yes, I discovered that CONFIG_FB was not enabled, so I did so.
> 
>>  If that doesn't fix anything, that probably means that one
>>  .atomic_check() fails, so it would be a good place to start 
>> debugging.
> 
> There will be other things to verify in the Ingenic driver. As noted 
> many
> months ago, colour depth information has to be set in the DMA 
> descriptors and
> not the control register, but we are managing to do this 
> successfully, as far
> as I can tell, although there is always the potential for error.
> 
> Paul
> 
> 

Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[H. Nikolaus Schaller]

Hi Paul,

> Am 25.09.2021 um 21:08 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Paul & Nikolaus,
> 
> If you spent some time debugging the issue

we did ...

> instead of complaining that my patchset breaks things...

... we did have a working version (without hdmi-connector)
and bisect pointed at your patch... So we debugged that.

So the lesson is: don't trust bisect.

And failed to make it work with hdmi-connector because the
ingenic-drm-drv reported errors.

> 
> The fix is a one-liner in your downstream ingenic-dw-hdmi.c:
> .output_port = 1
> in the ingenic_dw_hdmi_plat_data struct.

Cool. How did you find that?

> 
> Absolutely nothing else needs to be changed for HDMI to work here.

Great and thanks.

Will test asap and if it works as well, we can clean up a v4 patch set
for next week review.

BR and thanks,
Nikolaus

> 
> Cheers,
> -Paul
> 
> 
> Le sam., sept. 25 2021 at 17:55:03 +0200, Paul Boddie <paul@boddie.org.uk> a écrit :
>> On Friday, 24 September 2021 10:29:02 CEST Paul Cercueil wrote:
>>> Le ven., sept. 24 2021 at 00:51:39 +0200, Paul Boddie
>>> >
>>> > 2. My approach, which just involves changing the Synopsys driver to
>>> > set the bridge type in dw_hdmi_probe like this:
>>> >
>>> >   hdmi->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
>>> >
>>> > Otherwise, I don't see how the bridge's (struct drm_bridge) type will
>>> > be set.
>>> The bridge's type is set in hdmi-connector, from DTS. The 'type = "a"'
>>> will result in the bridge's .type to be set to DRM_MODE_CONNECTOR_HDMIA.
>> Actually, I found that hdmi-connector might not have been available because
>> CONFIG_DRM_DISPLAY_CONNECTOR was not enabled. Rectifying this, the connector
>> does get detected and enabled. However, the Synopsys driver remains unaware of
>> it, and so the bridge type in the Synopsys driver remains unset.
>> I do see that the connector sets the type on a bridge in its own private
>> structure, so there would be a need to propagate this type to the actual
>> bridge. In other words, what the connector does is distinct from the Synopsys
>> driver which acts as the bridge with regard to the Ingenic driver.
>> Perhaps the Synopsys driver should set the connector's bridge as the next
>> bridge, or maybe something is supposed to discover that the connector may act
>> as (or provide) a bridge after the Synopsys driver in the chain and then back-
>> propagate the bridge type along the chain.
>> [...]
>>> > And I removed any of the above hacks. What I observe, apart from an
>>> > inactive LCD controller (and ingenic-drm driver), is the following in
>>> > /sys/devices/platform/10180000.hdmi/:
>>> >
>>> > consumer:platform:13050000.lcdc0
>>> > consumer:platform:hdmi_connector
>> Interestingly, with the connector driver present, these sysfs entries no
>> longer appear.
>> [...]
>>> > For me, running modetest yields plenty of information about encoders,
>>> > connectors (and the supported modes via the EDID, thanks to my HDMI-A
>>> > hack), CRTCs, and planes. But no framebuffers are reported.
>>> Could you paste the result of "modetest -a -c -p" somewhere maybe?
>> I had to specify -M ingenic-drm as well, but here you go...
>> ----
>> Connectors:
>> id	encoder	status		name		size (mm)	modes	encoders
>> 35	34	connected	HDMI-A-1       	340x270		17	34
>>  modes:
>> 	index name refresh (Hz) hdisp hss hse htot vdisp vss vse vtot)
>>  #0 1280x1024 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 flags:
>> phsync, pvsync; type: preferred, driver
>>  #1 1280x1024 75.02 1280 1296 1440 1688 1024 1025 1028 1066 135000 flags:
>> phsync, pvsync; type: driver
>>  #2 1280x960 60.00 1280 1376 1488 1800 960 961 964 1000 108000 flags: phsync,
>> pvsync; type: driver
>>  #3 1152x864 75.00 1152 1216 1344 1600 864 865 868 900 108000 flags: phsync,
>> pvsync; type: driver
>>  #4 1024x768 75.03 1024 1040 1136 1312 768 769 772 800 78750 flags: phsync,
>> pvsync; type: driver
>>  #5 1024x768 70.07 1024 1048 1184 1328 768 771 777 806 75000 flags: nhsync,
>> nvsync; type: driver
>>  #6 1024x768 60.00 1024 1048 1184 1344 768 771 777 806 65000 flags: nhsync,
>> nvsync; type: driver
>>  #7 832x624 74.55 832 864 928 1152 624 625 628 667 57284 flags: nhsync,
>> nvsync; type: driver
>>  #8 800x600 75.00 800 816 896 1056 600 601 604 625 49500 flags: phsync,
>> pvsync; type: driver
>>  #9 800x600 72.19 800 856 976 1040 600 637 643 666 50000 flags: phsync,
>> pvsync; type: driver
>>  #10 800x600 60.32 800 840 968 1056 600 601 605 628 40000 flags: phsync,
>> pvsync; type: driver
>>  #11 800x600 56.25 800 824 896 1024 600 601 603 625 36000 flags: phsync,
>> pvsync; type: driver
>>  #12 640x480 75.00 640 656 720 840 480 481 484 500 31500 flags: nhsync,
>> nvsync; type: driver
>>  #13 640x480 72.81 640 664 704 832 480 489 492 520 31500 flags: nhsync,
>> nvsync; type: driver
>>  #14 640x480 66.67 640 704 768 864 480 483 486 525 30240 flags: nhsync,
>> nvsync; type: driver
>>  #15 640x480 59.94 640 656 752 800 480 490 492 525 25175 flags: nhsync,
>> nvsync; type: driver
>>  #16 720x400 70.08 720 738 846 900 400 412 414 449 28320 flags: nhsync,
>> pvsync; type: driver
>>  props:
>> 	1 EDID:
>> 		flags: immutable blob
>> 		blobs:
>> 		value:
>> 			00ffffffffffff00047232ad01010101
>> 			2d0e010380221b782aaea5a6544c9926
>> 			145054bfef0081808140714f01010101
>> 			010101010101302a009851002a403070
>> 			1300520e1100001e000000ff00343435
>> 			3030353444454330300a000000fc0041
>> 			4c313731350a202020202020000000fd
>> 			00384c1e520e000a2020202020200051
>> 	2 DPMS:
>> 		flags: enum
>> 		enums: On=0 Standby=1 Suspend=2 Off=3
>> 		value: 0
>> 	5 link-status:
>> 		flags: enum
>> 		enums: Good=0 Bad=1
>> 		value: 0
>> 	6 non-desktop:
>> 		flags: immutable range
>> 		values: 0 1
>> 		value: 0
>> 	4 TILE:
>> 		flags: immutable blob
>> 		blobs:
>> 		value:
>> 	20 CRTC_ID:
>> 		flags: object
>> 		value: 32
>> CRTCs:
>> id	fb	pos	size
>> 32	39	(0,0)	(1280x1024)
>>  #0  60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 flags: phsync,
>> pvsync; type:
>>  props:
>> 	22 ACTIVE:
>> 		flags: range
>> 		values: 0 1
>> 		value: 1
>> 	23 MODE_ID:
>> 		flags: blob
>> 		blobs:
>> 		value:
>> 			e0a5010000053005a005980600000004
>> 			010404042a0400003c00000005000000
>> 			00000000000000000000000000000000
>> 			00000000000000000000000000000000
>> 			00000000
>> 	19 OUT_FENCE_PTR:
>> 		flags: range
>> 		values: 0 18446744073709551615
>> 		value: 0
>> 	24 VRR_ENABLED:
>> 		flags: range
>> 		values: 0 1
>> 		value: 0
>> 	28 GAMMA_LUT:
>> 		flags: blob
>> 		blobs:
>> 		value:
>> 	29 GAMMA_LUT_SIZE:
>> 		flags: immutable range
>> 		values: 0 4294967295
>> 		value: 256
>> Planes:
>> id	crtc	fb	CRTC x,y	x,y	gamma size	possible crtcs
>> 31	32	39	0,0		0,0	0       	0x00000001
>>  formats: XR15 RG16 RG24 XR24 XR30
>>  props:
>> 	8 type:
>> 		flags: immutable enum
>> 		enums: Overlay=0 Primary=1 Cursor=2
>> 		value: 1
>> 	17 FB_ID:
>> 		flags: object
>> 		value: 39
>> 	18 IN_FENCE_FD:
>> 		flags: signed range
>> 		values: -1 2147483647
>> 		value: -1
>> 	20 CRTC_ID:
>> 		flags: object
>> 		value: 32
>> 	13 CRTC_X:
>> 		flags: signed range
>> 		values: -2147483648 2147483647
>> 		value: 0
>> 	14 CRTC_Y:
>> 		flags: signed range
>> 		values: -2147483648 2147483647
>> 		value: 0
>> 	15 CRTC_W:
>> 		flags: range
>> 		values: 0 2147483647
>> 		value: 1280
>> 	16 CRTC_H:
>> 		flags: range
>> 		values: 0 2147483647
>> 		value: 1024
>> 	9 SRC_X:
>> 		flags: range
>> 		values: 0 4294967295
>> 		value: 0
>> 	10 SRC_Y:
>> 		flags: range
>> 		values: 0 4294967295
>> 		value: 0
>> 	11 SRC_W:
>> 		flags: range
>> 		values: 0 4294967295
>> 		value: 83886080
>> 	12 SRC_H:
>> 		flags: range
>> 		values: 0 4294967295
>> 		value: 67108864
>> 33	0	0	0,0		0,0	0       	0x00000001
>>  formats: C8   XR15 RG16 RG24 XR24 XR30
>>  props:
>> 	8 type:
>> 		flags: immutable enum
>> 		enums: Overlay=0 Primary=1 Cursor=2
>> 		value: 0
>> 	17 FB_ID:
>> 		flags: object
>> 		value: 0
>> 	18 IN_FENCE_FD:
>> 		flags: signed range
>> 		values: -1 2147483647
>> 		value: -1
>> 	20 CRTC_ID:
>> 		flags: object
>> 		value: 0
>> 	13 CRTC_X:
>> 		flags: signed range
>> 		values: -2147483648 2147483647
>> 		value: 0
>> 	14 CRTC_Y:
>> 		flags: signed range
>> 		values: -2147483648 2147483647
>> 		value: 0
>> 	15 CRTC_W:
>> 		flags: range
>> 		values: 0 2147483647
>> 		value: 0
>> 	16 CRTC_H:
>> 		flags: range
>> 		values: 0 2147483647
>> 		value: 0
>> 	9 SRC_X:
>> 		flags: range
>> 		values: 0 4294967295
>> 		value: 0
>> 	10 SRC_Y:
>> 		flags: range
>> 		values: 0 4294967295
>> 		value: 0
>> 	11 SRC_W:
>> 		flags: range
>> 		values: 0 4294967295
>> 		value: 0
>> 	12 SRC_H:
>> 		flags: range
>> 		values: 0 4294967295
>> 		value: 0
>> ----
>>> If you have info about the CRTCs, encoders, connectors and EDID info,
>>> then I would assume it is very close to working fine.
>>> For your "no framebuffer" issue, keep in mind that CONFIG_FB and
>>> CONFIG_FRAMEBUFFER_CONSOLE are now disabled by default.
>> Yes, I discovered that CONFIG_FB was not enabled, so I did so.
>>> If that doesn't fix anything, that probably means that one
>>> .atomic_check() fails, so it would be a good place to start debugging.
>> There will be other things to verify in the Ingenic driver. As noted many
>> months ago, colour depth information has to be set in the DMA descriptors and
>> not the control register, but we are managing to do this successfully, as far
>> as I can tell, although there is always the potential for error.
>> Paul
> 
> 

Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[Paul Cercueil]

Le sam., sept. 25 2021 at 21:26:42 +0200, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Paul,
> 
>>  Am 25.09.2021 um 21:08 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>>  Hi Paul & Nikolaus,
>> 
>>  If you spent some time debugging the issue
> 
> we did ...

By saying that you didn't debug, I mean that you did not try to see why 
you had these errors - where the error codes were coming from, etc., to 
have a clear understanding of why it fails.

>>  instead of complaining that my patchset breaks things...
> 
> ... we did have a working version (without hdmi-connector)
> and bisect pointed at your patch... So we debugged that.
> 
> So the lesson is: don't trust bisect.
> 
> And failed to make it work with hdmi-connector because the
> ingenic-drm-drv reported errors.
> 
>> 
>>  The fix is a one-liner in your downstream ingenic-dw-hdmi.c:
>>  .output_port = 1
>>  in the ingenic_dw_hdmi_plat_data struct.
> 
> Cool. How did you find that?

You had this:
[    4.474346] [drm:drm_bridge_attach [drm]] *ERROR* failed to attach 
bridge (null) to encoder DPI-34: -22

(null) means you're printing a NULL pointer. So I could see that 
hdmi->next_bridge was NULL. The place that sets it is dw_hdmi_parse_dt, 
which will return early with code 0, before next_bridge is set, if 
plat_data->output_port == 0, which was your case.

Since your hdmi-connector is wired at port #1, then .output_port should 
be 1 as well.

Cheers,
-Paul

>> 
>>  Absolutely nothing else needs to be changed for HDMI to work here.
> 
> Great and thanks.
> 
> Will test asap and if it works as well, we can clean up a v4 patch set
> for next week review.
> 
> BR and thanks,
> Nikolaus
> 
>> 
>>  Cheers,
>>  -Paul
>> 
>> 
>>  Le sam., sept. 25 2021 at 17:55:03 +0200, Paul Boddie 
>> <paul@boddie.org.uk> a écrit :
>>>  On Friday, 24 September 2021 10:29:02 CEST Paul Cercueil wrote:
>>>>  Le ven., sept. 24 2021 at 00:51:39 +0200, Paul Boddie
>>>>  >
>>>>  > 2. My approach, which just involves changing the Synopsys 
>>>> driver to
>>>>  > set the bridge type in dw_hdmi_probe like this:
>>>>  >
>>>>  >   hdmi->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
>>>>  >
>>>>  > Otherwise, I don't see how the bridge's (struct drm_bridge) 
>>>> type will
>>>>  > be set.
>>>>  The bridge's type is set in hdmi-connector, from DTS. The 'type = 
>>>> "a"'
>>>>  will result in the bridge's .type to be set to 
>>>> DRM_MODE_CONNECTOR_HDMIA.
>>>  Actually, I found that hdmi-connector might not have been 
>>> available because
>>>  CONFIG_DRM_DISPLAY_CONNECTOR was not enabled. Rectifying this, the 
>>> connector
>>>  does get detected and enabled. However, the Synopsys driver 
>>> remains unaware of
>>>  it, and so the bridge type in the Synopsys driver remains unset.
>>>  I do see that the connector sets the type on a bridge in its own 
>>> private
>>>  structure, so there would be a need to propagate this type to the 
>>> actual
>>>  bridge. In other words, what the connector does is distinct from 
>>> the Synopsys
>>>  driver which acts as the bridge with regard to the Ingenic driver.
>>>  Perhaps the Synopsys driver should set the connector's bridge as 
>>> the next
>>>  bridge, or maybe something is supposed to discover that the 
>>> connector may act
>>>  as (or provide) a bridge after the Synopsys driver in the chain 
>>> and then back-
>>>  propagate the bridge type along the chain.
>>>  [...]
>>>>  > And I removed any of the above hacks. What I observe, apart 
>>>> from an
>>>>  > inactive LCD controller (and ingenic-drm driver), is the 
>>>> following in
>>>>  > /sys/devices/platform/10180000.hdmi/:
>>>>  >
>>>>  > consumer:platform:13050000.lcdc0
>>>>  > consumer:platform:hdmi_connector
>>>  Interestingly, with the connector driver present, these sysfs 
>>> entries no
>>>  longer appear.
>>>  [...]
>>>>  > For me, running modetest yields plenty of information about 
>>>> encoders,
>>>>  > connectors (and the supported modes via the EDID, thanks to my 
>>>> HDMI-A
>>>>  > hack), CRTCs, and planes. But no framebuffers are reported.
>>>>  Could you paste the result of "modetest -a -c -p" somewhere maybe?
>>>  I had to specify -M ingenic-drm as well, but here you go...
>>>  ----
>>>  Connectors:
>>>  id	encoder	status		name		size (mm)	modes	encoders
>>>  35	34	connected	HDMI-A-1       	340x270		17	34
>>>   modes:
>>>  	index name refresh (Hz) hdisp hss hse htot vdisp vss vse vtot)
>>>   #0 1280x1024 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 
>>> flags:
>>>  phsync, pvsync; type: preferred, driver
>>>   #1 1280x1024 75.02 1280 1296 1440 1688 1024 1025 1028 1066 135000 
>>> flags:
>>>  phsync, pvsync; type: driver
>>>   #2 1280x960 60.00 1280 1376 1488 1800 960 961 964 1000 108000 
>>> flags: phsync,
>>>  pvsync; type: driver
>>>   #3 1152x864 75.00 1152 1216 1344 1600 864 865 868 900 108000 
>>> flags: phsync,
>>>  pvsync; type: driver
>>>   #4 1024x768 75.03 1024 1040 1136 1312 768 769 772 800 78750 
>>> flags: phsync,
>>>  pvsync; type: driver
>>>   #5 1024x768 70.07 1024 1048 1184 1328 768 771 777 806 75000 
>>> flags: nhsync,
>>>  nvsync; type: driver
>>>   #6 1024x768 60.00 1024 1048 1184 1344 768 771 777 806 65000 
>>> flags: nhsync,
>>>  nvsync; type: driver
>>>   #7 832x624 74.55 832 864 928 1152 624 625 628 667 57284 flags: 
>>> nhsync,
>>>  nvsync; type: driver
>>>   #8 800x600 75.00 800 816 896 1056 600 601 604 625 49500 flags: 
>>> phsync,
>>>  pvsync; type: driver
>>>   #9 800x600 72.19 800 856 976 1040 600 637 643 666 50000 flags: 
>>> phsync,
>>>  pvsync; type: driver
>>>   #10 800x600 60.32 800 840 968 1056 600 601 605 628 40000 flags: 
>>> phsync,
>>>  pvsync; type: driver
>>>   #11 800x600 56.25 800 824 896 1024 600 601 603 625 36000 flags: 
>>> phsync,
>>>  pvsync; type: driver
>>>   #12 640x480 75.00 640 656 720 840 480 481 484 500 31500 flags: 
>>> nhsync,
>>>  nvsync; type: driver
>>>   #13 640x480 72.81 640 664 704 832 480 489 492 520 31500 flags: 
>>> nhsync,
>>>  nvsync; type: driver
>>>   #14 640x480 66.67 640 704 768 864 480 483 486 525 30240 flags: 
>>> nhsync,
>>>  nvsync; type: driver
>>>   #15 640x480 59.94 640 656 752 800 480 490 492 525 25175 flags: 
>>> nhsync,
>>>  nvsync; type: driver
>>>   #16 720x400 70.08 720 738 846 900 400 412 414 449 28320 flags: 
>>> nhsync,
>>>  pvsync; type: driver
>>>   props:
>>>  	1 EDID:
>>>  		flags: immutable blob
>>>  		blobs:
>>>  		value:
>>>  			00ffffffffffff00047232ad01010101
>>>  			2d0e010380221b782aaea5a6544c9926
>>>  			145054bfef0081808140714f01010101
>>>  			010101010101302a009851002a403070
>>>  			1300520e1100001e000000ff00343435
>>>  			3030353444454330300a000000fc0041
>>>  			4c313731350a202020202020000000fd
>>>  			00384c1e520e000a2020202020200051
>>>  	2 DPMS:
>>>  		flags: enum
>>>  		enums: On=0 Standby=1 Suspend=2 Off=3
>>>  		value: 0
>>>  	5 link-status:
>>>  		flags: enum
>>>  		enums: Good=0 Bad=1
>>>  		value: 0
>>>  	6 non-desktop:
>>>  		flags: immutable range
>>>  		values: 0 1
>>>  		value: 0
>>>  	4 TILE:
>>>  		flags: immutable blob
>>>  		blobs:
>>>  		value:
>>>  	20 CRTC_ID:
>>>  		flags: object
>>>  		value: 32
>>>  CRTCs:
>>>  id	fb	pos	size
>>>  32	39	(0,0)	(1280x1024)
>>>   #0  60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 flags: 
>>> phsync,
>>>  pvsync; type:
>>>   props:
>>>  	22 ACTIVE:
>>>  		flags: range
>>>  		values: 0 1
>>>  		value: 1
>>>  	23 MODE_ID:
>>>  		flags: blob
>>>  		blobs:
>>>  		value:
>>>  			e0a5010000053005a005980600000004
>>>  			010404042a0400003c00000005000000
>>>  			00000000000000000000000000000000
>>>  			00000000000000000000000000000000
>>>  			00000000
>>>  	19 OUT_FENCE_PTR:
>>>  		flags: range
>>>  		values: 0 18446744073709551615
>>>  		value: 0
>>>  	24 VRR_ENABLED:
>>>  		flags: range
>>>  		values: 0 1
>>>  		value: 0
>>>  	28 GAMMA_LUT:
>>>  		flags: blob
>>>  		blobs:
>>>  		value:
>>>  	29 GAMMA_LUT_SIZE:
>>>  		flags: immutable range
>>>  		values: 0 4294967295
>>>  		value: 256
>>>  Planes:
>>>  id	crtc	fb	CRTC x,y	x,y	gamma size	possible crtcs
>>>  31	32	39	0,0		0,0	0       	0x00000001
>>>   formats: XR15 RG16 RG24 XR24 XR30
>>>   props:
>>>  	8 type:
>>>  		flags: immutable enum
>>>  		enums: Overlay=0 Primary=1 Cursor=2
>>>  		value: 1
>>>  	17 FB_ID:
>>>  		flags: object
>>>  		value: 39
>>>  	18 IN_FENCE_FD:
>>>  		flags: signed range
>>>  		values: -1 2147483647
>>>  		value: -1
>>>  	20 CRTC_ID:
>>>  		flags: object
>>>  		value: 32
>>>  	13 CRTC_X:
>>>  		flags: signed range
>>>  		values: -2147483648 2147483647
>>>  		value: 0
>>>  	14 CRTC_Y:
>>>  		flags: signed range
>>>  		values: -2147483648 2147483647
>>>  		value: 0
>>>  	15 CRTC_W:
>>>  		flags: range
>>>  		values: 0 2147483647
>>>  		value: 1280
>>>  	16 CRTC_H:
>>>  		flags: range
>>>  		values: 0 2147483647
>>>  		value: 1024
>>>  	9 SRC_X:
>>>  		flags: range
>>>  		values: 0 4294967295
>>>  		value: 0
>>>  	10 SRC_Y:
>>>  		flags: range
>>>  		values: 0 4294967295
>>>  		value: 0
>>>  	11 SRC_W:
>>>  		flags: range
>>>  		values: 0 4294967295
>>>  		value: 83886080
>>>  	12 SRC_H:
>>>  		flags: range
>>>  		values: 0 4294967295
>>>  		value: 67108864
>>>  33	0	0	0,0		0,0	0       	0x00000001
>>>   formats: C8   XR15 RG16 RG24 XR24 XR30
>>>   props:
>>>  	8 type:
>>>  		flags: immutable enum
>>>  		enums: Overlay=0 Primary=1 Cursor=2
>>>  		value: 0
>>>  	17 FB_ID:
>>>  		flags: object
>>>  		value: 0
>>>  	18 IN_FENCE_FD:
>>>  		flags: signed range
>>>  		values: -1 2147483647
>>>  		value: -1
>>>  	20 CRTC_ID:
>>>  		flags: object
>>>  		value: 0
>>>  	13 CRTC_X:
>>>  		flags: signed range
>>>  		values: -2147483648 2147483647
>>>  		value: 0
>>>  	14 CRTC_Y:
>>>  		flags: signed range
>>>  		values: -2147483648 2147483647
>>>  		value: 0
>>>  	15 CRTC_W:
>>>  		flags: range
>>>  		values: 0 2147483647
>>>  		value: 0
>>>  	16 CRTC_H:
>>>  		flags: range
>>>  		values: 0 2147483647
>>>  		value: 0
>>>  	9 SRC_X:
>>>  		flags: range
>>>  		values: 0 4294967295
>>>  		value: 0
>>>  	10 SRC_Y:
>>>  		flags: range
>>>  		values: 0 4294967295
>>>  		value: 0
>>>  	11 SRC_W:
>>>  		flags: range
>>>  		values: 0 4294967295
>>>  		value: 0
>>>  	12 SRC_H:
>>>  		flags: range
>>>  		values: 0 4294967295
>>>  		value: 0
>>>  ----
>>>>  If you have info about the CRTCs, encoders, connectors and EDID 
>>>> info,
>>>>  then I would assume it is very close to working fine.
>>>>  For your "no framebuffer" issue, keep in mind that CONFIG_FB and
>>>>  CONFIG_FRAMEBUFFER_CONSOLE are now disabled by default.
>>>  Yes, I discovered that CONFIG_FB was not enabled, so I did so.
>>>>  If that doesn't fix anything, that probably means that one
>>>>  .atomic_check() fails, so it would be a good place to start 
>>>> debugging.
>>>  There will be other things to verify in the Ingenic driver. As 
>>> noted many
>>>  months ago, colour depth information has to be set in the DMA 
>>> descriptors and
>>>  not the control register, but we are managing to do this 
>>> successfully, as far
>>>  as I can tell, although there is always the potential for error.
>>>  Paul
>> 
>> 
> 

Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[H. Nikolaus Schaller]

Hi Paul,

> Am 25.09.2021 um 21:39 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> 
> 
> Le sam., sept. 25 2021 at 21:26:42 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> Hi Paul,
>>> Am 25.09.2021 um 21:08 schrieb Paul Cercueil <paul@crapouillou.net>:
>>> Hi Paul & Nikolaus,
>>> If you spent some time debugging the issue
>> we did ...
> 
> By saying that you didn't debug,

We did - but sometimes you don't see the wood for the trees.

> (null) means you're printing a NULL pointer. So I could see that hdmi->next_bridge was NULL.

I remember we did find this, but did not understand that it should be initialized by dw-hdmi.
And because we though that dw-hdmi has it its own connector, it is ok that way.

> The place that sets it is dw_hdmi_parse_dt, which will return early with code 0, before next_bridge is set, if plat_data->output_port == 0, which was your case.

Well, we were still at 5.14 when we did these initial attempts to use hdmi-connector with synopsys.
Back then, there was no dw_hdmi_parse_dt and no output_port.

IAW: we did not even have a chance to make it work on top of 5.14 the hdmi-connector way. And were sucessful.

I just noticed this when trying to backport the last puzzle piece...

Well, it is always difficult to hit a moving target.

> Since your hdmi-connector is wired at port #1, then .output_port should be 1 as well.

Anyways it works now for 5.14.8 (our internal test) and 5.15-rc3.

And v4 of the jz4780 hdmi stuff will follow.

BR and thanks,
Nikolaus