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(-)
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;
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); }
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))
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);
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; }
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) {
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;
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
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
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
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
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
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.
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
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
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
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.
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".
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).
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.
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.
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
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/...
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
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/...
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
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
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
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:
- 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.
- 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
On Friday, 24 September 2021 10:29:02 CEST Paul Cercueil wrote:
Le ven., sept. 24 2021 at 00:51:39 +0200, Paul Boddie
- 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
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
- 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 00384c1e520e000a20202020202000512 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 0000000019 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
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
- 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
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
- 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
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
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.... [2]: https://git.goldelico.com/?p=letux-kernel.git;a=shortlog;h=refs/heads/upstre...
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;
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.
dri-devel@lists.freedesktop.org
-
H. Nikolaus Schaller -
Laurent Pinchart -
Paul Boddie -
Paul Cercueil