* [PATCH v3 1/6] drm/ingenic: Simplify code by using hwdescs array
2021-09-22 20:55 [PATCH v3 0/6] drm/ingenic: Various improvements v3 Paul Cercueil
@ 2021-09-22 20:55 ` Paul Cercueil
2021-09-22 20:55 ` [PATCH v3 2/6] drm/ingenic: Add support for private objects Paul Cercueil
` (4 subsequent siblings)
5 siblings, 0 replies; 27+ messages in thread
From: Paul Cercueil @ 2021-09-22 20:55 UTC (permalink / raw)
To: David Airlie, Daniel Vetter
Cc: linux-mips, list, dri-devel, linux-kernel, Paul Cercueil
Instead of having one 'hwdesc' variable for the plane #0, one for the
plane #1 and one for the palette, use a 'hwdesc[3]' array, where the
DMA hardware descriptors are indexed by the plane's number.
v2: dma_hwdesc_addr() extended to support palette hwdesc. The palette
hwdesc is now hwdesc[3] to simplify things. Add
ingenic_drm_configure_hwdesc*() functions to factorize code.
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 78 ++++++++++++++---------
1 file changed, 48 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index a5df1c8d34cd..95c12c2aba14 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -41,6 +41,8 @@
#include <drm/drm_probe_helper.h>
#include <drm/drm_vblank.h>
+#define HWDESC_PALETTE 2
+
struct ingenic_dma_hwdesc {
u32 next;
u32 addr;
@@ -49,9 +51,7 @@ struct ingenic_dma_hwdesc {
} __aligned(16);
struct ingenic_dma_hwdescs {
- struct ingenic_dma_hwdesc hwdesc_f0;
- struct ingenic_dma_hwdesc hwdesc_f1;
- struct ingenic_dma_hwdesc hwdesc_pal;
+ struct ingenic_dma_hwdesc hwdesc[3];
u16 palette[256] __aligned(16);
};
@@ -141,6 +141,14 @@ static inline struct ingenic_drm *drm_nb_get_priv(struct notifier_block *nb)
return container_of(nb, struct ingenic_drm, clock_nb);
}
+static inline dma_addr_t dma_hwdesc_addr(const struct ingenic_drm *priv,
+ unsigned int idx)
+{
+ u32 offset = offsetof(struct ingenic_dma_hwdescs, hwdesc[idx]);
+
+ return priv->dma_hwdescs_phys + offset;
+}
+
static int ingenic_drm_update_pixclk(struct notifier_block *nb,
unsigned long action,
void *data)
@@ -558,9 +566,9 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
struct ingenic_drm *priv = drm_device_get_priv(plane->dev);
struct drm_plane_state *newstate = drm_atomic_get_new_plane_state(state, plane);
struct drm_plane_state *oldstate = drm_atomic_get_old_plane_state(state, plane);
+ unsigned int width, height, cpp, next_id, plane_id;
struct drm_crtc_state *crtc_state;
struct ingenic_dma_hwdesc *hwdesc;
- unsigned int width, height, cpp, offset;
dma_addr_t addr;
u32 fourcc;
@@ -569,16 +577,14 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
drm_fb_cma_sync_non_coherent(&priv->drm, oldstate, newstate);
crtc_state = newstate->crtc->state;
+ plane_id = !!(priv->soc_info->has_osd && plane != &priv->f0);
addr = drm_fb_cma_get_gem_addr(newstate->fb, newstate, 0);
width = newstate->src_w >> 16;
height = newstate->src_h >> 16;
cpp = newstate->fb->format->cpp[0];
- if (!priv->soc_info->has_osd || plane == &priv->f0)
- hwdesc = &priv->dma_hwdescs->hwdesc_f0;
- else
- hwdesc = &priv->dma_hwdescs->hwdesc_f1;
+ hwdesc = &priv->dma_hwdescs->hwdesc[plane_id];
hwdesc->addr = addr;
hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
@@ -588,12 +594,8 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
ingenic_drm_plane_config(priv->dev, plane, fourcc);
- if (fourcc == DRM_FORMAT_C8)
- offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_pal);
- else
- offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
-
- priv->dma_hwdescs->hwdesc_f0.next = priv->dma_hwdescs_phys + offset;
+ next_id = fourcc == DRM_FORMAT_C8 ? HWDESC_PALETTE : 0;
+ priv->dma_hwdescs->hwdesc[0].next = dma_hwdesc_addr(priv, next_id);
crtc_state->color_mgmt_changed = fourcc == DRM_FORMAT_C8;
}
@@ -846,6 +848,35 @@ static void __maybe_unused ingenic_drm_release_rmem(void *d)
of_reserved_mem_device_release(d);
}
+static void ingenic_drm_configure_hwdesc(struct ingenic_drm *priv,
+ unsigned int hwdesc,
+ unsigned int next_hwdesc, u32 id)
+{
+ struct ingenic_dma_hwdesc *desc = &priv->dma_hwdescs->hwdesc[hwdesc];
+
+ desc->next = dma_hwdesc_addr(priv, next_hwdesc);
+ desc->id = id;
+}
+
+static void ingenic_drm_configure_hwdesc_palette(struct ingenic_drm *priv)
+{
+ struct ingenic_dma_hwdesc *desc;
+
+ ingenic_drm_configure_hwdesc(priv, HWDESC_PALETTE, 0, 0xc0);
+
+ desc = &priv->dma_hwdescs->hwdesc[HWDESC_PALETTE];
+ desc->addr = priv->dma_hwdescs_phys
+ + offsetof(struct ingenic_dma_hwdescs, palette);
+ desc->cmd = JZ_LCD_CMD_ENABLE_PAL
+ | (sizeof(priv->dma_hwdescs->palette) / 4);
+}
+
+static void ingenic_drm_configure_hwdesc_plane(struct ingenic_drm *priv,
+ unsigned int plane)
+{
+ ingenic_drm_configure_hwdesc(priv, plane, plane, 0xf0 | plane);
+}
+
static int ingenic_drm_bind(struct device *dev, bool has_components)
{
struct platform_device *pdev = to_platform_device(dev);
@@ -942,27 +973,14 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
if (!priv->dma_hwdescs)
return -ENOMEM;
-
/* Configure DMA hwdesc for foreground0 plane */
- dma_hwdesc_phys_f0 = priv->dma_hwdescs_phys
- + offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
- priv->dma_hwdescs->hwdesc_f0.next = dma_hwdesc_phys_f0;
- priv->dma_hwdescs->hwdesc_f0.id = 0xf0;
+ ingenic_drm_configure_hwdesc_plane(priv, 0);
/* Configure DMA hwdesc for foreground1 plane */
- dma_hwdesc_phys_f1 = priv->dma_hwdescs_phys
- + offsetof(struct ingenic_dma_hwdescs, hwdesc_f1);
- priv->dma_hwdescs->hwdesc_f1.next = dma_hwdesc_phys_f1;
- priv->dma_hwdescs->hwdesc_f1.id = 0xf1;
+ ingenic_drm_configure_hwdesc_plane(priv, 1);
/* Configure DMA hwdesc for palette */
- priv->dma_hwdescs->hwdesc_pal.next = priv->dma_hwdescs_phys
- + offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
- priv->dma_hwdescs->hwdesc_pal.id = 0xc0;
- priv->dma_hwdescs->hwdesc_pal.addr = priv->dma_hwdescs_phys
- + offsetof(struct ingenic_dma_hwdescs, palette);
- priv->dma_hwdescs->hwdesc_pal.cmd = JZ_LCD_CMD_ENABLE_PAL
- | (sizeof(priv->dma_hwdescs->palette) / 4);
+ ingenic_drm_configure_hwdesc_palette(priv);
primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
--
2.33.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 2/6] drm/ingenic: Add support for private objects
2021-09-22 20:55 [PATCH v3 0/6] drm/ingenic: Various improvements v3 Paul Cercueil
2021-09-22 20:55 ` [PATCH v3 1/6] drm/ingenic: Simplify code by using hwdescs array Paul Cercueil
@ 2021-09-22 20:55 ` Paul Cercueil
2021-09-22 20:55 ` [PATCH v3 3/6] drm/ingenic: Move IPU scale settings to private state Paul Cercueil
` (3 subsequent siblings)
5 siblings, 0 replies; 27+ messages in thread
From: Paul Cercueil @ 2021-09-22 20:55 UTC (permalink / raw)
To: David Airlie, Daniel Vetter
Cc: linux-mips, list, dri-devel, linux-kernel, Paul Cercueil
Until now, the ingenic-drm as well as the ingenic-ipu drivers used to
put state-specific information in their respective private structure.
Add boilerplate code to support private objects in the two drivers, so
that state-specific information can be put in the state-specific private
structure.
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 61 +++++++++++++++++++++++
drivers/gpu/drm/ingenic/ingenic-ipu.c | 54 ++++++++++++++++++++
2 files changed, 115 insertions(+)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 95c12c2aba14..5dbeca0f8f37 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -64,6 +64,10 @@ struct jz_soc_info {
unsigned int num_formats_f0, num_formats_f1;
};
+struct ingenic_drm_private_state {
+ struct drm_private_state base;
+};
+
struct ingenic_drm {
struct drm_device drm;
/*
@@ -99,8 +103,16 @@ struct ingenic_drm {
struct mutex clk_mutex;
bool update_clk_rate;
struct notifier_block clock_nb;
+
+ struct drm_private_obj private_obj;
};
+static inline struct ingenic_drm_private_state *
+to_ingenic_drm_priv_state(struct drm_private_state *state)
+{
+ return container_of(state, struct ingenic_drm_private_state, base);
+}
+
static bool ingenic_drm_writeable_reg(struct device *dev, unsigned int reg)
{
switch (reg) {
@@ -766,6 +778,28 @@ ingenic_drm_gem_create_object(struct drm_device *drm, size_t size)
return &obj->base;
}
+static struct drm_private_state *
+ingenic_drm_duplicate_state(struct drm_private_obj *obj)
+{
+ struct ingenic_drm_private_state *state = to_ingenic_drm_priv_state(obj->state);
+
+ state = kmemdup(state, sizeof(*state), GFP_KERNEL);
+ if (!state)
+ return NULL;
+
+ __drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
+
+ return &state->base;
+}
+
+static void ingenic_drm_destroy_state(struct drm_private_obj *obj,
+ struct drm_private_state *state)
+{
+ struct ingenic_drm_private_state *priv_state = to_ingenic_drm_priv_state(state);
+
+ kfree(priv_state);
+}
+
DEFINE_DRM_GEM_CMA_FOPS(ingenic_drm_fops);
static const struct drm_driver ingenic_drm_driver_data = {
@@ -836,6 +870,11 @@ static struct drm_mode_config_helper_funcs ingenic_drm_mode_config_helpers = {
.atomic_commit_tail = drm_atomic_helper_commit_tail,
};
+static const struct drm_private_state_funcs ingenic_drm_private_state_funcs = {
+ .atomic_duplicate_state = ingenic_drm_duplicate_state,
+ .atomic_destroy_state = ingenic_drm_destroy_state,
+};
+
static void ingenic_drm_unbind_all(void *d)
{
struct ingenic_drm *priv = d;
@@ -877,9 +916,15 @@ static void ingenic_drm_configure_hwdesc_plane(struct ingenic_drm *priv,
ingenic_drm_configure_hwdesc(priv, plane, plane, 0xf0 | plane);
}
+static void ingenic_drm_atomic_private_obj_fini(struct drm_device *drm, void *private_obj)
+{
+ drm_atomic_private_obj_fini(private_obj);
+}
+
static int ingenic_drm_bind(struct device *dev, bool has_components)
{
struct platform_device *pdev = to_platform_device(dev);
+ struct ingenic_drm_private_state *private_state;
const struct jz_soc_info *soc_info;
struct ingenic_drm *priv;
struct clk *parent_clk;
@@ -1148,6 +1193,20 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
goto err_devclk_disable;
}
+ private_state = kzalloc(sizeof(*private_state), GFP_KERNEL);
+ if (!private_state) {
+ ret = -ENOMEM;
+ goto err_clk_notifier_unregister;
+ }
+
+ drm_atomic_private_obj_init(drm, &priv->private_obj, &private_state->base,
+ &ingenic_drm_private_state_funcs);
+
+ ret = drmm_add_action_or_reset(drm, ingenic_drm_atomic_private_obj_fini,
+ &priv->private_obj);
+ if (ret)
+ goto err_private_state_free;
+
ret = drm_dev_register(drm, 0);
if (ret) {
dev_err(dev, "Failed to register DRM driver\n");
@@ -1158,6 +1217,8 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
return 0;
+err_private_state_free:
+ kfree(private_state);
err_clk_notifier_unregister:
clk_notifier_unregister(parent_clk, &priv->clock_nb);
err_devclk_disable:
diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c b/drivers/gpu/drm/ingenic/ingenic-ipu.c
index aeb8a757d213..c819293b8317 100644
--- a/drivers/gpu/drm/ingenic/ingenic-ipu.c
+++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c
@@ -45,6 +45,10 @@ struct soc_info {
unsigned int weight, unsigned int offset);
};
+struct ingenic_ipu_private_state {
+ struct drm_private_state base;
+};
+
struct ingenic_ipu {
struct drm_plane plane;
struct drm_device *drm;
@@ -60,6 +64,8 @@ struct ingenic_ipu {
struct drm_property *sharpness_prop;
unsigned int sharpness;
+
+ struct drm_private_obj private_obj;
};
/* Signed 15.16 fixed-point math (for bicubic scaling coefficients) */
@@ -73,6 +79,12 @@ static inline struct ingenic_ipu *plane_to_ingenic_ipu(struct drm_plane *plane)
return container_of(plane, struct ingenic_ipu, plane);
}
+static inline struct ingenic_ipu_private_state *
+to_ingenic_ipu_priv_state(struct drm_private_state *state)
+{
+ return container_of(state, struct ingenic_ipu_private_state, base);
+}
+
/*
* Apply conventional cubic convolution kernel. Both parameters
* and return value are 15.16 signed fixed-point.
@@ -679,6 +691,33 @@ static const struct drm_plane_funcs ingenic_ipu_plane_funcs = {
.atomic_set_property = ingenic_ipu_plane_atomic_set_property,
};
+static struct drm_private_state *
+ingenic_ipu_duplicate_state(struct drm_private_obj *obj)
+{
+ struct ingenic_ipu_private_state *state = to_ingenic_ipu_priv_state(obj->state);
+
+ state = kmemdup(state, sizeof(*state), GFP_KERNEL);
+ if (!state)
+ return NULL;
+
+ __drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
+
+ return &state->base;
+}
+
+static void ingenic_ipu_destroy_state(struct drm_private_obj *obj,
+ struct drm_private_state *state)
+{
+ struct ingenic_ipu_private_state *priv_state = to_ingenic_ipu_priv_state(state);
+
+ kfree(priv_state);
+}
+
+static const struct drm_private_state_funcs ingenic_ipu_private_state_funcs = {
+ .atomic_duplicate_state = ingenic_ipu_duplicate_state,
+ .atomic_destroy_state = ingenic_ipu_destroy_state,
+};
+
static irqreturn_t ingenic_ipu_irq_handler(int irq, void *arg)
{
struct ingenic_ipu *ipu = arg;
@@ -717,6 +756,7 @@ static const struct regmap_config ingenic_ipu_regmap_config = {
static int ingenic_ipu_bind(struct device *dev, struct device *master, void *d)
{
struct platform_device *pdev = to_platform_device(dev);
+ struct ingenic_ipu_private_state *private_state;
const struct soc_info *soc_info;
struct drm_device *drm = d;
struct drm_plane *plane;
@@ -810,7 +850,20 @@ static int ingenic_ipu_bind(struct device *dev, struct device *master, void *d)
return err;
}
+ private_state = kzalloc(sizeof(*private_state), GFP_KERNEL);
+ if (!private_state) {
+ err = -ENOMEM;
+ goto err_clk_unprepare;
+ }
+
+ drm_atomic_private_obj_init(drm, &ipu->private_obj, &private_state->base,
+ &ingenic_ipu_private_state_funcs);
+
return 0;
+
+err_clk_unprepare:
+ clk_unprepare(ipu->clk);
+ return err;
}
static void ingenic_ipu_unbind(struct device *dev,
@@ -818,6 +871,7 @@ static void ingenic_ipu_unbind(struct device *dev,
{
struct ingenic_ipu *ipu = dev_get_drvdata(dev);
+ drm_atomic_private_obj_fini(&ipu->private_obj);
clk_unprepare(ipu->clk);
}
--
2.33.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 3/6] drm/ingenic: Move IPU scale settings to private state
2021-09-22 20:55 [PATCH v3 0/6] drm/ingenic: Various improvements v3 Paul Cercueil
2021-09-22 20:55 ` [PATCH v3 1/6] drm/ingenic: Simplify code by using hwdescs array Paul Cercueil
2021-09-22 20:55 ` [PATCH v3 2/6] drm/ingenic: Add support for private objects Paul Cercueil
@ 2021-09-22 20:55 ` Paul Cercueil
2021-09-22 20:55 ` [PATCH v3 4/6] drm/ingenic: Set DMA descriptor chain register when starting CRTC Paul Cercueil
` (2 subsequent siblings)
5 siblings, 0 replies; 27+ messages in thread
From: Paul Cercueil @ 2021-09-22 20:55 UTC (permalink / raw)
To: David Airlie, Daniel Vetter
Cc: linux-mips, list, dri-devel, linux-kernel, Paul Cercueil
The IPU scaling information is computed in the plane's ".atomic_check"
callback, and used in the ".atomic_update" callback. As such, it is
state-specific, and should be moved to a private state structure.
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
drivers/gpu/drm/ingenic/ingenic-ipu.c | 73 ++++++++++++++++++++-------
1 file changed, 54 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c b/drivers/gpu/drm/ingenic/ingenic-ipu.c
index c819293b8317..2737fc521e15 100644
--- a/drivers/gpu/drm/ingenic/ingenic-ipu.c
+++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c
@@ -47,6 +47,8 @@ struct soc_info {
struct ingenic_ipu_private_state {
struct drm_private_state base;
+
+ unsigned int num_w, num_h, denom_w, denom_h;
};
struct ingenic_ipu {
@@ -58,8 +60,6 @@ struct ingenic_ipu {
const struct soc_info *soc_info;
bool clk_enabled;
- unsigned int num_w, num_h, denom_w, denom_h;
-
dma_addr_t addr_y, addr_u, addr_v;
struct drm_property *sharpness_prop;
@@ -85,6 +85,30 @@ to_ingenic_ipu_priv_state(struct drm_private_state *state)
return container_of(state, struct ingenic_ipu_private_state, base);
}
+static struct ingenic_ipu_private_state *
+ingenic_ipu_get_priv_state(struct ingenic_ipu *priv, struct drm_atomic_state *state)
+{
+ struct drm_private_state *priv_state;
+
+ priv_state = drm_atomic_get_private_obj_state(state, &priv->private_obj);
+ if (IS_ERR(priv_state))
+ return ERR_CAST(priv_state);
+
+ return to_ingenic_ipu_priv_state(priv_state);
+}
+
+static struct ingenic_ipu_private_state *
+ingenic_ipu_get_new_priv_state(struct ingenic_ipu *priv, struct drm_atomic_state *state)
+{
+ struct drm_private_state *priv_state;
+
+ priv_state = drm_atomic_get_new_private_obj_state(state, &priv->private_obj);
+ if (!priv_state)
+ return NULL;
+
+ return to_ingenic_ipu_priv_state(priv_state);
+}
+
/*
* Apply conventional cubic convolution kernel. Both parameters
* and return value are 15.16 signed fixed-point.
@@ -305,11 +329,16 @@ static void ingenic_ipu_plane_atomic_update(struct drm_plane *plane,
const struct drm_format_info *finfo;
u32 ctrl, stride = 0, coef_index = 0, format = 0;
bool needs_modeset, upscaling_w, upscaling_h;
+ struct ingenic_ipu_private_state *ipu_state;
int err;
if (!newstate || !newstate->fb)
return;
+ ipu_state = ingenic_ipu_get_new_priv_state(ipu, state);
+ if (WARN_ON(!ipu_state))
+ return;
+
finfo = drm_format_info(newstate->fb->format->format);
if (!ipu->clk_enabled) {
@@ -482,27 +511,27 @@ static void ingenic_ipu_plane_atomic_update(struct drm_plane *plane,
if (ipu->soc_info->has_bicubic)
ctrl |= JZ_IPU_CTRL_ZOOM_SEL;
- upscaling_w = ipu->num_w > ipu->denom_w;
+ upscaling_w = ipu_state->num_w > ipu_state->denom_w;
if (upscaling_w)
ctrl |= JZ_IPU_CTRL_HSCALE;
- if (ipu->num_w != 1 || ipu->denom_w != 1) {
+ if (ipu_state->num_w != 1 || ipu_state->denom_w != 1) {
if (!ipu->soc_info->has_bicubic && !upscaling_w)
- coef_index |= (ipu->denom_w - 1) << 16;
+ coef_index |= (ipu_state->denom_w - 1) << 16;
else
- coef_index |= (ipu->num_w - 1) << 16;
+ coef_index |= (ipu_state->num_w - 1) << 16;
ctrl |= JZ_IPU_CTRL_HRSZ_EN;
}
- upscaling_h = ipu->num_h > ipu->denom_h;
+ upscaling_h = ipu_state->num_h > ipu_state->denom_h;
if (upscaling_h)
ctrl |= JZ_IPU_CTRL_VSCALE;
- if (ipu->num_h != 1 || ipu->denom_h != 1) {
+ if (ipu_state->num_h != 1 || ipu_state->denom_h != 1) {
if (!ipu->soc_info->has_bicubic && !upscaling_h)
- coef_index |= ipu->denom_h - 1;
+ coef_index |= ipu_state->denom_h - 1;
else
- coef_index |= ipu->num_h - 1;
+ coef_index |= ipu_state->num_h - 1;
ctrl |= JZ_IPU_CTRL_VRSZ_EN;
}
@@ -513,13 +542,13 @@ static void ingenic_ipu_plane_atomic_update(struct drm_plane *plane,
/* Set the LUT index register */
regmap_write(ipu->map, JZ_REG_IPU_RSZ_COEF_INDEX, coef_index);
- if (ipu->num_w != 1 || ipu->denom_w != 1)
+ if (ipu_state->num_w != 1 || ipu_state->denom_w != 1)
ingenic_ipu_set_coefs(ipu, JZ_REG_IPU_HRSZ_COEF_LUT,
- ipu->num_w, ipu->denom_w);
+ ipu_state->num_w, ipu_state->denom_w);
- if (ipu->num_h != 1 || ipu->denom_h != 1)
+ if (ipu_state->num_h != 1 || ipu_state->denom_h != 1)
ingenic_ipu_set_coefs(ipu, JZ_REG_IPU_VRSZ_COEF_LUT,
- ipu->num_h, ipu->denom_h);
+ ipu_state->num_h, ipu_state->denom_h);
/* Clear STATUS register */
regmap_write(ipu->map, JZ_REG_IPU_STATUS, 0);
@@ -531,7 +560,8 @@ static void ingenic_ipu_plane_atomic_update(struct drm_plane *plane,
dev_dbg(ipu->dev, "Scaling %ux%u to %ux%u (%u:%u horiz, %u:%u vert)\n",
newstate->src_w >> 16, newstate->src_h >> 16,
newstate->crtc_w, newstate->crtc_h,
- ipu->num_w, ipu->denom_w, ipu->num_h, ipu->denom_h);
+ ipu_state->num_w, ipu_state->denom_w,
+ ipu_state->num_h, ipu_state->denom_h);
}
static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
@@ -545,6 +575,7 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
struct ingenic_ipu *ipu = plane_to_ingenic_ipu(plane);
struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc;
struct drm_crtc_state *crtc_state;
+ struct ingenic_ipu_private_state *ipu_state;
if (!crtc)
return 0;
@@ -553,6 +584,10 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
if (WARN_ON(!crtc_state))
return -EINVAL;
+ ipu_state = ingenic_ipu_get_priv_state(ipu, state);
+ if (IS_ERR(ipu_state))
+ return PTR_ERR(ipu_state);
+
/* Request a full modeset if we are enabling or disabling the IPU. */
if (!old_plane_state->crtc ^ !new_plane_state->crtc)
crtc_state->mode_changed = true;
@@ -605,10 +640,10 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
if (num_h > max_h)
return -EINVAL;
- ipu->num_w = num_w;
- ipu->num_h = num_h;
- ipu->denom_w = denom_w;
- ipu->denom_h = denom_h;
+ ipu_state->num_w = num_w;
+ ipu_state->num_h = num_h;
+ ipu_state->denom_w = denom_w;
+ ipu_state->denom_h = denom_h;
out_check_damage:
if (ingenic_drm_map_noncoherent(ipu->master))
--
2.33.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 4/6] drm/ingenic: Set DMA descriptor chain register when starting CRTC
2021-09-22 20:55 [PATCH v3 0/6] drm/ingenic: Various improvements v3 Paul Cercueil
` (2 preceding siblings ...)
2021-09-22 20:55 ` [PATCH v3 3/6] drm/ingenic: Move IPU scale settings to private state Paul Cercueil
@ 2021-09-22 20:55 ` Paul Cercueil
2021-09-22 20:55 ` [PATCH v3 5/6] drm/ingenic: Upload palette before frame Paul Cercueil
2021-09-22 20:55 ` [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders Paul Cercueil
5 siblings, 0 replies; 27+ messages in thread
From: Paul Cercueil @ 2021-09-22 20:55 UTC (permalink / raw)
To: David Airlie, Daniel Vetter
Cc: linux-mips, list, dri-devel, linux-kernel, Paul Cercueil
Setting the DMA descriptor chain register in the probe function has been
fine until now, because we only ever had one descriptor per foreground.
As the driver will soon have real descriptor chains, and the DMA
descriptor chain register updates itself to point to the current
descriptor being processed, this register needs to be reset after a full
modeset to point to the first descriptor of the chain.
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 5dbeca0f8f37..cbc76cede99e 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -186,6 +186,10 @@ static void ingenic_drm_crtc_atomic_enable(struct drm_crtc *crtc,
regmap_write(priv->map, JZ_REG_LCD_STATE, 0);
+ /* Set address of our DMA descriptor chain */
+ regmap_write(priv->map, JZ_REG_LCD_DA0, dma_hwdesc_addr(priv, 0));
+ regmap_write(priv->map, JZ_REG_LCD_DA1, dma_hwdesc_addr(priv, 1));
+
regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
JZ_LCD_CTRL_ENABLE | JZ_LCD_CTRL_DISABLE,
JZ_LCD_CTRL_ENABLE);
--
2.33.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 5/6] drm/ingenic: Upload palette before frame
2021-09-22 20:55 [PATCH v3 0/6] drm/ingenic: Various improvements v3 Paul Cercueil
` (3 preceding siblings ...)
2021-09-22 20:55 ` [PATCH v3 4/6] drm/ingenic: Set DMA descriptor chain register when starting CRTC Paul Cercueil
@ 2021-09-22 20:55 ` Paul Cercueil
2021-09-22 20:55 ` [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders Paul Cercueil
5 siblings, 0 replies; 27+ messages in thread
From: Paul Cercueil @ 2021-09-22 20:55 UTC (permalink / raw)
To: David Airlie, Daniel Vetter
Cc: linux-mips, list, dri-devel, linux-kernel, Paul Cercueil
When using C8 color mode, make sure that the palette is always uploaded
before a frame; otherwise the very first frame will have wrong colors.
Do that by changing the link order of the DMA descriptors.
v3: Fix ingenic_drm_get_new_priv_state() called instead of
ingenic_drm_get_priv_state()
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 53 ++++++++++++++++++++---
1 file changed, 47 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index cbc76cede99e..a5e2880e07a1 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -66,6 +66,7 @@ struct jz_soc_info {
struct ingenic_drm_private_state {
struct drm_private_state base;
+ bool use_palette;
};
struct ingenic_drm {
@@ -113,6 +114,30 @@ to_ingenic_drm_priv_state(struct drm_private_state *state)
return container_of(state, struct ingenic_drm_private_state, base);
}
+static struct ingenic_drm_private_state *
+ingenic_drm_get_priv_state(struct ingenic_drm *priv, struct drm_atomic_state *state)
+{
+ struct drm_private_state *priv_state;
+
+ priv_state = drm_atomic_get_private_obj_state(state, &priv->private_obj);
+ if (IS_ERR(priv_state))
+ return ERR_CAST(priv_state);
+
+ return to_ingenic_drm_priv_state(priv_state);
+}
+
+static struct ingenic_drm_private_state *
+ingenic_drm_get_new_priv_state(struct ingenic_drm *priv, struct drm_atomic_state *state)
+{
+ struct drm_private_state *priv_state;
+
+ priv_state = drm_atomic_get_new_private_obj_state(state, &priv->private_obj);
+ if (!priv_state)
+ return NULL;
+
+ return to_ingenic_drm_priv_state(priv_state);
+}
+
static bool ingenic_drm_writeable_reg(struct device *dev, unsigned int reg)
{
switch (reg) {
@@ -183,11 +208,18 @@ static void ingenic_drm_crtc_atomic_enable(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
+ struct ingenic_drm_private_state *priv_state;
+ unsigned int next_id;
+
+ priv_state = ingenic_drm_get_priv_state(priv, state);
+ if (WARN_ON(IS_ERR(priv_state)))
+ return;
regmap_write(priv->map, JZ_REG_LCD_STATE, 0);
- /* Set address of our DMA descriptor chain */
- regmap_write(priv->map, JZ_REG_LCD_DA0, dma_hwdesc_addr(priv, 0));
+ /* Set addresses of our DMA descriptor chains */
+ next_id = priv_state->use_palette ? HWDESC_PALETTE : 0;
+ regmap_write(priv->map, JZ_REG_LCD_DA0, dma_hwdesc_addr(priv, next_id));
regmap_write(priv->map, JZ_REG_LCD_DA1, dma_hwdesc_addr(priv, 1));
regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
@@ -393,6 +425,7 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
plane);
struct ingenic_drm *priv = drm_device_get_priv(plane->dev);
+ struct ingenic_drm_private_state *priv_state;
struct drm_crtc_state *crtc_state;
struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc;
int ret;
@@ -405,6 +438,10 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
if (WARN_ON(!crtc_state))
return -EINVAL;
+ priv_state = ingenic_drm_get_priv_state(priv, state);
+ if (IS_ERR(priv_state))
+ return PTR_ERR(priv_state);
+
ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
DRM_PLANE_HELPER_NO_SCALING,
DRM_PLANE_HELPER_NO_SCALING,
@@ -423,6 +460,9 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
(new_plane_state->src_h >> 16) != new_plane_state->crtc_h))
return -EINVAL;
+ priv_state->use_palette = new_plane_state->fb &&
+ new_plane_state->fb->format->format == DRM_FORMAT_C8;
+
/*
* Require full modeset if enabling or disabling a plane, or changing
* its position, size or depth.
@@ -583,6 +623,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
struct drm_plane_state *newstate = drm_atomic_get_new_plane_state(state, plane);
struct drm_plane_state *oldstate = drm_atomic_get_old_plane_state(state, plane);
unsigned int width, height, cpp, next_id, plane_id;
+ struct ingenic_drm_private_state *priv_state;
struct drm_crtc_state *crtc_state;
struct ingenic_dma_hwdesc *hwdesc;
dma_addr_t addr;
@@ -600,19 +641,19 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
height = newstate->src_h >> 16;
cpp = newstate->fb->format->cpp[0];
- hwdesc = &priv->dma_hwdescs->hwdesc[plane_id];
+ priv_state = ingenic_drm_get_new_priv_state(priv, state);
+ next_id = (priv_state && priv_state->use_palette) ? HWDESC_PALETTE : plane_id;
+ hwdesc = &priv->dma_hwdescs->hwdesc[plane_id];
hwdesc->addr = addr;
hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
+ hwdesc->next = dma_hwdesc_addr(priv, next_id);
if (drm_atomic_crtc_needs_modeset(crtc_state)) {
fourcc = newstate->fb->format->format;
ingenic_drm_plane_config(priv->dev, plane, fourcc);
- next_id = fourcc == DRM_FORMAT_C8 ? HWDESC_PALETTE : 0;
- priv->dma_hwdescs->hwdesc[0].next = dma_hwdesc_addr(priv, next_id);
-
crtc_state->color_mgmt_changed = fourcc == DRM_FORMAT_C8;
}
--
2.33.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders
2021-09-22 20:55 [PATCH v3 0/6] drm/ingenic: Various improvements v3 Paul Cercueil
` (4 preceding siblings ...)
2021-09-22 20:55 ` [PATCH v3 5/6] drm/ingenic: Upload palette before frame Paul Cercueil
@ 2021-09-22 20:55 ` Paul Cercueil
2021-09-23 5:52 ` H. Nikolaus Schaller
5 siblings, 1 reply; 27+ messages in thread
From: Paul Cercueil @ 2021-09-22 20:55 UTC (permalink / raw)
To: David Airlie, Daniel Vetter
Cc: linux-mips, list, dri-devel, linux-kernel, Paul Cercueil
Attach a top-level bridge to each encoder, which will be used for
negociating the bus format and flags.
All the bridges are now attached with DRM_BRIDGE_ATTACH_NO_CONNECTOR.
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 92 +++++++++++++++++------
1 file changed, 70 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index a5e2880e07a1..a05a9fa6e115 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -21,6 +21,7 @@
#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
#include <drm/drm_bridge.h>
+#include <drm/drm_bridge_connector.h>
#include <drm/drm_color_mgmt.h>
#include <drm/drm_crtc.h>
#include <drm/drm_crtc_helper.h>
@@ -108,6 +109,19 @@ struct ingenic_drm {
struct drm_private_obj private_obj;
};
+struct ingenic_drm_bridge {
+ struct drm_encoder encoder;
+ struct drm_bridge bridge, *next_bridge;
+
+ struct drm_bus_cfg bus_cfg;
+};
+
+static inline struct ingenic_drm_bridge *
+to_ingenic_drm_bridge(struct drm_encoder *encoder)
+{
+ return container_of(encoder, struct ingenic_drm_bridge, encoder);
+}
+
static inline struct ingenic_drm_private_state *
to_ingenic_drm_priv_state(struct drm_private_state *state)
{
@@ -668,11 +682,10 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
{
struct ingenic_drm *priv = drm_device_get_priv(encoder->dev);
struct drm_display_mode *mode = &crtc_state->adjusted_mode;
- struct drm_connector *conn = conn_state->connector;
- struct drm_display_info *info = &conn->display_info;
+ struct ingenic_drm_bridge *bridge = to_ingenic_drm_bridge(encoder);
unsigned int cfg, rgbcfg = 0;
- priv->panel_is_sharp = info->bus_flags & DRM_BUS_FLAG_SHARP_SIGNALS;
+ priv->panel_is_sharp = bridge->bus_cfg.flags & DRM_BUS_FLAG_SHARP_SIGNALS;
if (priv->panel_is_sharp) {
cfg = JZ_LCD_CFG_MODE_SPECIAL_TFT_1 | JZ_LCD_CFG_REV_POLARITY;
@@ -685,19 +698,19 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
if (mode->flags & DRM_MODE_FLAG_NVSYNC)
cfg |= JZ_LCD_CFG_VSYNC_ACTIVE_LOW;
- if (info->bus_flags & DRM_BUS_FLAG_DE_LOW)
+ if (bridge->bus_cfg.flags & DRM_BUS_FLAG_DE_LOW)
cfg |= JZ_LCD_CFG_DE_ACTIVE_LOW;
- if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
+ if (bridge->bus_cfg.flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
cfg |= JZ_LCD_CFG_PCLK_FALLING_EDGE;
if (!priv->panel_is_sharp) {
- if (conn->connector_type == DRM_MODE_CONNECTOR_TV) {
+ if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV) {
if (mode->flags & DRM_MODE_FLAG_INTERLACE)
cfg |= JZ_LCD_CFG_MODE_TV_OUT_I;
else
cfg |= JZ_LCD_CFG_MODE_TV_OUT_P;
} else {
- switch (*info->bus_formats) {
+ switch (bridge->bus_cfg.format) {
case MEDIA_BUS_FMT_RGB565_1X16:
cfg |= JZ_LCD_CFG_MODE_GENERIC_16BIT;
break;
@@ -723,20 +736,29 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
regmap_write(priv->map, JZ_REG_LCD_RGBC, rgbcfg);
}
-static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder,
- struct drm_crtc_state *crtc_state,
- struct drm_connector_state *conn_state)
+static int ingenic_drm_bridge_attach(struct drm_bridge *bridge,
+ enum drm_bridge_attach_flags flags)
+{
+ struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
+
+ return drm_bridge_attach(bridge->encoder, ib->next_bridge,
+ &ib->bridge, flags);
+}
+
+static int ingenic_drm_bridge_atomic_check(struct drm_bridge *bridge,
+ struct drm_bridge_state *bridge_state,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state)
{
- struct drm_display_info *info = &conn_state->connector->display_info;
struct drm_display_mode *mode = &crtc_state->adjusted_mode;
+ struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
- if (info->num_bus_formats != 1)
- return -EINVAL;
+ ib->bus_cfg = bridge_state->output_bus_cfg;
if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV)
return 0;
- switch (*info->bus_formats) {
+ switch (bridge_state->output_bus_cfg.format) {
case MEDIA_BUS_FMT_RGB888_3X8:
case MEDIA_BUS_FMT_RGB888_3X8_DELTA:
/*
@@ -900,8 +922,16 @@ static const struct drm_crtc_helper_funcs ingenic_drm_crtc_helper_funcs = {
};
static const struct drm_encoder_helper_funcs ingenic_drm_encoder_helper_funcs = {
- .atomic_mode_set = ingenic_drm_encoder_atomic_mode_set,
- .atomic_check = ingenic_drm_encoder_atomic_check,
+ .atomic_mode_set = ingenic_drm_encoder_atomic_mode_set,
+};
+
+static const struct drm_bridge_funcs ingenic_drm_bridge_funcs = {
+ .attach = ingenic_drm_bridge_attach,
+ .atomic_check = ingenic_drm_bridge_atomic_check,
+ .atomic_reset = drm_atomic_helper_bridge_reset,
+ .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+ .atomic_get_input_bus_fmts = drm_atomic_helper_bridge_propagate_bus_fmt,
};
static const struct drm_mode_config_funcs ingenic_drm_mode_config_funcs = {
@@ -976,7 +1006,9 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
struct drm_plane *primary;
struct drm_bridge *bridge;
struct drm_panel *panel;
+ struct drm_connector *connector;
struct drm_encoder *encoder;
+ struct ingenic_drm_bridge *ib;
struct drm_device *drm;
void __iomem *base;
long parent_rate;
@@ -1154,20 +1186,36 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
bridge = devm_drm_panel_bridge_add_typed(dev, panel,
DRM_MODE_CONNECTOR_DPI);
- encoder = drmm_plain_encoder_alloc(drm, NULL, DRM_MODE_ENCODER_DPI, NULL);
- if (IS_ERR(encoder)) {
- ret = PTR_ERR(encoder);
+ ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder,
+ NULL, DRM_MODE_ENCODER_DPI, NULL);
+ if (IS_ERR(ib)) {
+ ret = PTR_ERR(ib);
dev_err(dev, "Failed to init encoder: %d\n", ret);
return ret;
}
- encoder->possible_crtcs = 1;
+ encoder = &ib->encoder;
+ encoder->possible_crtcs = drm_crtc_mask(&priv->crtc);
drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
- ret = drm_bridge_attach(encoder, bridge, NULL, 0);
- if (ret)
+ ib->bridge.funcs = &ingenic_drm_bridge_funcs;
+ ib->next_bridge = bridge;
+
+ ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
+ DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+ if (ret) {
+ dev_err(dev, "Unable to attach bridge\n");
return ret;
+ }
+
+ connector = drm_bridge_connector_init(drm, encoder);
+ if (IS_ERR(connector)) {
+ dev_err(dev, "Unable to init connector\n");
+ return PTR_ERR(connector);
+ }
+
+ drm_connector_attach_encoder(connector, encoder);
}
drm_for_each_encoder(encoder, drm) {
--
2.33.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders
2021-09-22 20:55 ` [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders Paul Cercueil
@ 2021-09-23 5:52 ` H. Nikolaus Schaller
2021-09-23 8:49 ` Paul Cercueil
0 siblings, 1 reply; 27+ messages in thread
From: H. Nikolaus Schaller @ 2021-09-23 5:52 UTC (permalink / raw)
To: Paul Cercueil
Cc: David Airlie, Daniel Vetter, linux-mips, list, dri-devel,
linux-kernel, Laurent Pinchart
Hi Paul,
thanks for another update.
We have been delayed to rework the CI20 HDMI code on top of your series
but it basically works in some situations. There is for example a problem
if the EDID reports DRM_COLOR_FORMAT_YCRCB422 but it appears to be outside
of your series.
The only issue we have is described below.
> Am 22.09.2021 um 22:55 schrieb Paul Cercueil <paul@crapouillou.net>:
>
> Attach a top-level bridge to each encoder, which will be used for
> negociating the bus format and flags.
>
> All the bridges are now attached with DRM_BRIDGE_ATTACH_NO_CONNECTOR.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 92 +++++++++++++++++------
> 1 file changed, 70 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index a5e2880e07a1..a05a9fa6e115 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -21,6 +21,7 @@
> #include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_bridge.h>
> +#include <drm/drm_bridge_connector.h>
> #include <drm/drm_color_mgmt.h>
> #include <drm/drm_crtc.h>
> #include <drm/drm_crtc_helper.h>
> @@ -108,6 +109,19 @@ struct ingenic_drm {
> struct drm_private_obj private_obj;
> };
>
> +struct ingenic_drm_bridge {
> + struct drm_encoder encoder;
> + struct drm_bridge bridge, *next_bridge;
> +
> + struct drm_bus_cfg bus_cfg;
> +};
> +
> +static inline struct ingenic_drm_bridge *
> +to_ingenic_drm_bridge(struct drm_encoder *encoder)
> +{
> + return container_of(encoder, struct ingenic_drm_bridge, encoder);
> +}
> +
> static inline struct ingenic_drm_private_state *
> to_ingenic_drm_priv_state(struct drm_private_state *state)
> {
> @@ -668,11 +682,10 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
> {
> struct ingenic_drm *priv = drm_device_get_priv(encoder->dev);
> struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> - struct drm_connector *conn = conn_state->connector;
> - struct drm_display_info *info = &conn->display_info;
> + struct ingenic_drm_bridge *bridge = to_ingenic_drm_bridge(encoder);
> unsigned int cfg, rgbcfg = 0;
>
> - priv->panel_is_sharp = info->bus_flags & DRM_BUS_FLAG_SHARP_SIGNALS;
> + priv->panel_is_sharp = bridge->bus_cfg.flags & DRM_BUS_FLAG_SHARP_SIGNALS;
>
> if (priv->panel_is_sharp) {
> cfg = JZ_LCD_CFG_MODE_SPECIAL_TFT_1 | JZ_LCD_CFG_REV_POLARITY;
> @@ -685,19 +698,19 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
> cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
> if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> cfg |= JZ_LCD_CFG_VSYNC_ACTIVE_LOW;
> - if (info->bus_flags & DRM_BUS_FLAG_DE_LOW)
> + if (bridge->bus_cfg.flags & DRM_BUS_FLAG_DE_LOW)
> cfg |= JZ_LCD_CFG_DE_ACTIVE_LOW;
> - if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> + if (bridge->bus_cfg.flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> cfg |= JZ_LCD_CFG_PCLK_FALLING_EDGE;
>
> if (!priv->panel_is_sharp) {
> - if (conn->connector_type == DRM_MODE_CONNECTOR_TV) {
> + if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV) {
> if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> cfg |= JZ_LCD_CFG_MODE_TV_OUT_I;
> else
> cfg |= JZ_LCD_CFG_MODE_TV_OUT_P;
> } else {
> - switch (*info->bus_formats) {
> + switch (bridge->bus_cfg.format) {
> case MEDIA_BUS_FMT_RGB565_1X16:
> cfg |= JZ_LCD_CFG_MODE_GENERIC_16BIT;
> break;
> @@ -723,20 +736,29 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
> regmap_write(priv->map, JZ_REG_LCD_RGBC, rgbcfg);
> }
>
> -static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder,
> - struct drm_crtc_state *crtc_state,
> - struct drm_connector_state *conn_state)
> +static int ingenic_drm_bridge_attach(struct drm_bridge *bridge,
> + enum drm_bridge_attach_flags flags)
> +{
> + struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
> +
> + return drm_bridge_attach(bridge->encoder, ib->next_bridge,
> + &ib->bridge, flags);
> +}
> +
> +static int ingenic_drm_bridge_atomic_check(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state)
> {
> - struct drm_display_info *info = &conn_state->connector->display_info;
> struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> + struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
>
> - if (info->num_bus_formats != 1)
> - return -EINVAL;
> + ib->bus_cfg = bridge_state->output_bus_cfg;
>
> if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV)
> return 0;
>
> - switch (*info->bus_formats) {
> + switch (bridge_state->output_bus_cfg.format) {
> case MEDIA_BUS_FMT_RGB888_3X8:
> case MEDIA_BUS_FMT_RGB888_3X8_DELTA:
> /*
> @@ -900,8 +922,16 @@ static const struct drm_crtc_helper_funcs ingenic_drm_crtc_helper_funcs = {
> };
>
> static const struct drm_encoder_helper_funcs ingenic_drm_encoder_helper_funcs = {
> - .atomic_mode_set = ingenic_drm_encoder_atomic_mode_set,
> - .atomic_check = ingenic_drm_encoder_atomic_check,
> + .atomic_mode_set = ingenic_drm_encoder_atomic_mode_set,
> +};
> +
> +static const struct drm_bridge_funcs ingenic_drm_bridge_funcs = {
> + .attach = ingenic_drm_bridge_attach,
> + .atomic_check = ingenic_drm_bridge_atomic_check,
> + .atomic_reset = drm_atomic_helper_bridge_reset,
> + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> + .atomic_get_input_bus_fmts = drm_atomic_helper_bridge_propagate_bus_fmt,
> };
>
> static const struct drm_mode_config_funcs ingenic_drm_mode_config_funcs = {
> @@ -976,7 +1006,9 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
> struct drm_plane *primary;
> struct drm_bridge *bridge;
> struct drm_panel *panel;
> + struct drm_connector *connector;
> struct drm_encoder *encoder;
> + struct ingenic_drm_bridge *ib;
> struct drm_device *drm;
> void __iomem *base;
> long parent_rate;
> @@ -1154,20 +1186,36 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
> bridge = devm_drm_panel_bridge_add_typed(dev, panel,
> DRM_MODE_CONNECTOR_DPI);
>
> - encoder = drmm_plain_encoder_alloc(drm, NULL, DRM_MODE_ENCODER_DPI, NULL);
> - if (IS_ERR(encoder)) {
> - ret = PTR_ERR(encoder);
> + ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder,
> + NULL, DRM_MODE_ENCODER_DPI, NULL);
> + if (IS_ERR(ib)) {
> + ret = PTR_ERR(ib);
> dev_err(dev, "Failed to init encoder: %d\n", ret);
> return ret;
> }
>
> - encoder->possible_crtcs = 1;
> + encoder = &ib->encoder;
> + encoder->possible_crtcs = drm_crtc_mask(&priv->crtc);
>
> drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
>
> - ret = drm_bridge_attach(encoder, bridge, NULL, 0);
> - if (ret)
> + ib->bridge.funcs = &ingenic_drm_bridge_funcs;
> + ib->next_bridge = bridge;
> +
> + ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
> + DRM_BRIDGE_ATTACH_NO_CONNECTOR);
DRM_BRIDGE_ATTACH_NO_CONNECTOR makes it fundamentally incompatible
with synopsys/dw_hdmi.c
That driver checks for DRM_BRIDGE_ATTACH_NO_CONNECTOR being NOT present,
since it wants to register its own connector through dw_hdmi_connector_create().
It does it for a reason: the dw-hdmi is a multi-function driver which does
HDMI and DDC/EDID stuff in a single driver (because I/O registers and power
management seem to be shared).
Since I do not see who could split this into a separate bridge and a connector driver
and test it on multiple SoC platforms (there are at least 3 or 4), I think modifying
the fundamentals of the dw-hdmi architecture just to get CI20 HDMI working is not
our turf.
Therefore the code here should be able to detect if drm_bridge_attach() already
creates and attaches a connector and then skip the code below.
> + if (ret) {
> + dev_err(dev, "Unable to attach bridge\n");
> return ret;
> + }
> +
> + connector = drm_bridge_connector_init(drm, encoder);
> + if (IS_ERR(connector)) {
> + dev_err(dev, "Unable to init connector\n");
> + return PTR_ERR(connector);
> + }
> +
> + drm_connector_attach_encoder(connector, encoder);
> }
>
> drm_for_each_encoder(encoder, drm) {
> --
> 2.33.0
I haven't replaced v2 with v3 in our test tree yet, but will do asap.
BR and thanks,
Nikolaus
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders
2021-09-23 5:52 ` H. Nikolaus Schaller
@ 2021-09-23 8:49 ` Paul Cercueil
2021-09-23 9:19 ` H. Nikolaus Schaller
2021-09-23 9:23 ` Laurent Pinchart
0 siblings, 2 replies; 27+ messages in thread
From: Paul Cercueil @ 2021-09-23 8:49 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: David Airlie, Daniel Vetter, linux-mips, list, dri-devel,
linux-kernel, Laurent Pinchart
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
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders
2021-09-23 8:49 ` Paul Cercueil
@ 2021-09-23 9:19 ` H. Nikolaus Schaller
2021-09-23 9:27 ` Laurent Pinchart
2021-09-23 9:23 ` Laurent Pinchart
1 sibling, 1 reply; 27+ messages in thread
From: H. Nikolaus Schaller @ 2021-09-23 9:19 UTC (permalink / raw)
To: Paul Cercueil
Cc: David Airlie, Daniel Vetter, linux-mips, list, dri-devel,
linux-kernel, Laurent Pinchart
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders
2021-09-23 9:19 ` H. Nikolaus Schaller
@ 2021-09-23 9:27 ` Laurent Pinchart
2021-09-23 9:55 ` H. Nikolaus Schaller
0 siblings, 1 reply; 27+ messages in thread
From: Laurent Pinchart @ 2021-09-23 9:27 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Paul Cercueil, David Airlie, Daniel Vetter, linux-mips, list,
dri-devel, linux-kernel
Hi Nikolaus,
On Thu, Sep 23, 2021 at 11:19:23AM +0200, H. Nikolaus Schaller wrote:
> > Am 23.09.2021 um 10:49 schrieb Paul Cercueil:
> > Le jeu., sept. 23 2021 at 07:52:08 +0200, H. Nikolaus Schaller a écrit :
> >> Hi Paul,
> >> thanks for another update.
> >> We have been delayed to rework the CI20 HDMI code on top of your series
> >> but it basically works in some situations. There is for example a problem
> >> if the EDID reports DRM_COLOR_FORMAT_YCRCB422 but it appears to be outside
> >> of your series.
> >
> > I think the SoC can output YCbCr as well, but I never tried to use it.
>
> Maybe there is code missing or something else. We have not yet deeply researched.
> Except that when ignoring DRM_COLOR_FORMAT_YCRCB422 capability it uses RGB
> and works.
>
> >>> + ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
> >>> + DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> >>
> >> DRM_BRIDGE_ATTACH_NO_CONNECTOR makes it fundamentally incompatible
> >> with synopsys/dw_hdmi.c
> >> That driver checks for DRM_BRIDGE_ATTACH_NO_CONNECTOR being NOT present,
> >> since it wants to register its own connector through dw_hdmi_connector_create().
> >> It does it for a reason: the dw-hdmi is a multi-function driver which does
> >> HDMI and DDC/EDID stuff in a single driver (because I/O registers and power
> >> management seem to be shared).
> >
> > The IT66121 driver does all of that too, and does not need
> > DRM_BRIDGE_ATTACH_NO_CONNECTOR. The drm_bridge_funcs struct has
> > callbacks to handle cable detection and DDC stuff.
> >
> >> Since I do not see who could split this into a separate bridge and a connector driver
> >> and test it on multiple SoC platforms (there are at least 3 or 4), I think modifying
> >> the fundamentals of the dw-hdmi architecture just to get CI20 HDMI working is not
> >> our turf.
> >
> > You could have a field in the dw-hdmi pdata structure, that would
> > instruct the driver whether or not it should use the new API. Ugly,
> > I know, and would probably duplicate a lot of code, but that would
> > allow other drivers to be updated at a later date.
>
> Yes, would be very ugly.
>
> But generally who has the knowledge (and time) to do this work?
> And has a working platform to test (jz4780 isn't a good development environment)?
>
> The driver seems to have a turbulent history starting 2013 in staging/imx and
> apparently it was generalized since then... Is Laurent currently dw-hdmi maintainer?
"Maintainer" would be an overstatement. I've worked on that driver in
the past, and I still use it, but don't have time to really maintain it.
I've also been told that Synopsys required all patches for that driver
developed using documentation under NDA to be submitted internally to
them first before being published, so I decided to stop contributing
instead of agreeing with this insane process. There's public
documentation about the IP in some NXP reference manuals though, so it
should be possible to still move forward without abiding by this rule.
> >> Therefore the code here should be able to detect if drm_bridge_attach() already
> >> creates and attaches a connector and then skip the code below.
> >
> > Not that easy, unfortunately. On one side we have dw-hdmi which
> > checks that DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set, and on the
> > other side we have other drivers like the IT66121 which will fail if
> > this flag is not set.
>
> Ok, I see. You have to handle contradicting cases here.
>
> Would it be possible to run it with DRM_BRIDGE_ATTACH_NO_CONNECTOR first
> and retry if it fails without?
>
> But IMHO the return value (in error case) is not well defined. So there
> must be a test if a connector has been created (I do not know how this
> would work).
>
> Another suggestion: can you check if there is a downstream connector defined in
> device tree (dw-hdmi does not need such a definition)?
> If not we call it with 0 and if there is one we call it with
> DRM_BRIDGE_ATTACH_NO_CONNECTOR and create one?
I haven't followed the ful conversation, what the reason why
DRM_BRIDGE_ATTACH_NO_CONNECTOR can't always be use here ? We're moving
towards requiring DRM_BRIDGE_ATTACH_NO_CONNECTOR for all new code, so it
will have to be done eventually.
> Just some ideas how to solve without touching hdmi drivers.
>
> BR and thanks,
> Nikolaus
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders
2021-09-23 9:27 ` Laurent Pinchart
@ 2021-09-23 9:55 ` H. Nikolaus Schaller
2021-09-23 10:03 ` Laurent Pinchart
0 siblings, 1 reply; 27+ messages in thread
From: H. Nikolaus Schaller @ 2021-09-23 9:55 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Paul Cercueil, David Airlie, Daniel Vetter, linux-mips, list,
dri-devel, linux-kernel, Paul Boddie
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders
2021-09-23 9:55 ` H. Nikolaus Schaller
@ 2021-09-23 10:03 ` Laurent Pinchart
2021-09-23 11:41 ` H. Nikolaus Schaller
0 siblings, 1 reply; 27+ messages in thread
From: Laurent Pinchart @ 2021-09-23 10:03 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Paul Cercueil, David Airlie, Daniel Vetter, linux-mips, list,
dri-devel, linux-kernel, Paul Boddie
Hi Nikolaus,
On Thu, Sep 23, 2021 at 11:55:56AM +0200, H. Nikolaus Schaller wrote:
> > Am 23.09.2021 um 11:27 schrieb Laurent Pinchart:
> > On Thu, Sep 23, 2021 at 11:19:23AM +0200, H. Nikolaus Schaller wrote:
> >>
> >>>>> + ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
> >>>>> + DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> >>>>
> >>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR makes it fundamentally incompatible
> >>>> with synopsys/dw_hdmi.c
> >>>> That driver checks for DRM_BRIDGE_ATTACH_NO_CONNECTOR being NOT present,
> >>>> since it wants to register its own connector through dw_hdmi_connector_create().
> >>>> It does it for a reason: the dw-hdmi is a multi-function driver which does
> >>>> HDMI and DDC/EDID stuff in a single driver (because I/O registers and power
> >>>> management seem to be shared).
> >>>
> >>> The IT66121 driver does all of that too, and does not need
> >>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. The drm_bridge_funcs struct has
> >>> callbacks to handle cable detection and DDC stuff.
> >>>
> >>>> Since I do not see who could split this into a separate bridge and a connector driver
> >>>> and test it on multiple SoC platforms (there are at least 3 or 4), I think modifying
> >>>> the fundamentals of the dw-hdmi architecture just to get CI20 HDMI working is not
> >>>> our turf.
> >>>
> >>> You could have a field in the dw-hdmi pdata structure, that would
> >>> instruct the driver whether or not it should use the new API. Ugly,
> >>> I know, and would probably duplicate a lot of code, but that would
> >>> allow other drivers to be updated at a later date.
> >>
> >> Yes, would be very ugly.
> >>
> >> But generally who has the knowledge (and time) to do this work?
> >> And has a working platform to test (jz4780 isn't a good development environment)?
> >>
> >> The driver seems to have a turbulent history starting 2013 in staging/imx and
> >> apparently it was generalized since then... Is Laurent currently dw-hdmi maintainer?
> >
> > "Maintainer" would be an overstatement. I've worked on that driver in
> > the past, and I still use it, but don't have time to really maintain it.
> > I've also been told that Synopsys required all patches for that driver
> > developed using documentation under NDA to be submitted internally to
> > them first before being published, so I decided to stop contributing
> > instead of agreeing with this insane process. There's public
> > documentation about the IP in some NXP reference manuals though, so it
> > should be possible to still move forward without abiding by this rule.
> >
> >>>> Therefore the code here should be able to detect if drm_bridge_attach() already
> >>>> creates and attaches a connector and then skip the code below.
> >>>
> >>> Not that easy, unfortunately. On one side we have dw-hdmi which
> >>> checks that DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set, and on the
> >>> other side we have other drivers like the IT66121 which will fail if
> >>> this flag is not set.
> >>
> >> Ok, I see. You have to handle contradicting cases here.
> >>
> >> Would it be possible to run it with DRM_BRIDGE_ATTACH_NO_CONNECTOR first
> >> and retry if it fails without?
> >>
> >> But IMHO the return value (in error case) is not well defined. So there
> >> must be a test if a connector has been created (I do not know how this
> >> would work).
> >>
> >> Another suggestion: can you check if there is a downstream connector defined in
> >> device tree (dw-hdmi does not need such a definition)?
> >> If not we call it with 0 and if there is one we call it with
> >> DRM_BRIDGE_ATTACH_NO_CONNECTOR and create one?
> >
> > I haven't followed the ful conversation, what the reason why
> > DRM_BRIDGE_ATTACH_NO_CONNECTOR can't always be use here ?
>
> The synopsys driver creates its own connector through dw_hdmi_connector_create()
> because the IP handles DDC/EDID directly.
That doesn't require creating a connector though. The driver implements
drm_bridge_funcs.get_edid(), which is used to get the EDID without the
need to create a connector in the dw-hdmi driver.
> Hence it checks for ! DRM_BRIDGE_ATTACH_NO_CONNECTOR which seems to be the
> right thing to do on current platforms that use it.
>
> For CI20/jz4780 we just add a specialisation of the generic dw-hdmi to
> make HDMI work.
>
> Now this patch for drm/ingenic wants the opposite definition and create its own
> connector. This fails even if we remove the check (then we have two interfering
> connectors).
>
> > We're moving
> > towards requiring DRM_BRIDGE_ATTACH_NO_CONNECTOR for all new code, so it
> > will have to be done eventually.
>
> So from my view drm/ingenic wants to already enforce this rule and breaks dw-hdmi.
>
> IMHO it should either handle this situation gracefully or include a fix for
> dw-hdmi.c to keep it compatible.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders
2021-09-23 10:03 ` Laurent Pinchart
@ 2021-09-23 11:41 ` H. Nikolaus Schaller
2021-09-23 11:56 ` H. Nikolaus Schaller
2021-09-23 13:30 ` Paul Cercueil
0 siblings, 2 replies; 27+ messages in thread
From: H. Nikolaus Schaller @ 2021-09-23 11:41 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Paul Cercueil, David Airlie, Daniel Vetter, linux-mips, list,
dri-devel, linux-kernel, Paul Boddie
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders
2021-09-23 11:41 ` H. Nikolaus Schaller
@ 2021-09-23 11:56 ` H. Nikolaus Schaller
2021-09-23 13:30 ` Paul Cercueil
1 sibling, 0 replies; 27+ messages in thread
From: H. Nikolaus Schaller @ 2021-09-23 11:56 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Paul Cercueil, David Airlie, Daniel Vetter, linux-mips, list,
dri-devel, linux-kernel, Paul Boddie
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders
2021-09-23 11:41 ` H. Nikolaus Schaller
2021-09-23 11:56 ` H. Nikolaus Schaller
@ 2021-09-23 13:30 ` Paul Cercueil
[not found] ` <ABE75744-46FE-4F37-A14C-D996F36B7B0E@goldelico.com>
1 sibling, 1 reply; 27+ messages in thread
From: Paul Cercueil @ 2021-09-23 13:30 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Laurent Pinchart, David Airlie, Daniel Vetter, linux-mips, list,
dri-devel, linux-kernel, Paul Boddie
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
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders
2021-09-23 8:49 ` Paul Cercueil
2021-09-23 9:19 ` H. Nikolaus Schaller
@ 2021-09-23 9:23 ` Laurent Pinchart
1 sibling, 0 replies; 27+ messages in thread
From: Laurent Pinchart @ 2021-09-23 9:23 UTC (permalink / raw)
To: Paul Cercueil
Cc: H. Nikolaus Schaller, David Airlie, Daniel Vetter, linux-mips,
list, dri-devel, linux-kernel
Hello,
On Thu, Sep 23, 2021 at 09:49:03AM +0100, Paul Cercueil wrote:
> Le jeu., sept. 23 2021 at 07:52:08 +0200, H. Nikolaus Schaller a écrit :
> > Hi Paul,
> > thanks for another update.
> >
> > We have been delayed to rework the CI20 HDMI code on top of your series
> > but it basically works in some situations. There is for example a problem
> > if the EDID reports DRM_COLOR_FORMAT_YCRCB422 but it appears to be outside
> > of your series.
>
> I think the SoC can output YCbCr as well, but I never tried to use it.
>
> > The only issue we have is described below.
> >
> >> Am 22.09.2021 um 22:55 schrieb Paul Cercueil <paul@crapouillou.net>:
> >>
> >> Attach a top-level bridge to each encoder, which will be used for
> >> negociating the bus format and flags.
> >>
> >> All the bridges are now attached with DRM_BRIDGE_ATTACH_NO_CONNECTOR.
> >>
> >> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> >> ---
> >> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 92 +++++++++++++++++------
> >> 1 file changed, 70 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >> index a5e2880e07a1..a05a9fa6e115 100644
> >> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >> @@ -21,6 +21,7 @@
> >> #include <drm/drm_atomic.h>
> >> #include <drm/drm_atomic_helper.h>
> >> #include <drm/drm_bridge.h>
> >> +#include <drm/drm_bridge_connector.h>
> >> #include <drm/drm_color_mgmt.h>
> >> #include <drm/drm_crtc.h>
> >> #include <drm/drm_crtc_helper.h>
> >> @@ -108,6 +109,19 @@ struct ingenic_drm {
> >> struct drm_private_obj private_obj;
> >> };
> >>
> >> +struct ingenic_drm_bridge {
> >> + struct drm_encoder encoder;
> >> + struct drm_bridge bridge, *next_bridge;
> >> +
> >> + struct drm_bus_cfg bus_cfg;
> >> +};
> >> +
> >> +static inline struct ingenic_drm_bridge *
> >> +to_ingenic_drm_bridge(struct drm_encoder *encoder)
> >> +{
> >> + return container_of(encoder, struct ingenic_drm_bridge, encoder);
> >> +}
> >> +
> >> static inline struct ingenic_drm_private_state *
> >> to_ingenic_drm_priv_state(struct drm_private_state *state)
> >> {
> >> @@ -668,11 +682,10 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
> >> {
> >> struct ingenic_drm *priv = drm_device_get_priv(encoder->dev);
> >> struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> >> - struct drm_connector *conn = conn_state->connector;
> >> - struct drm_display_info *info = &conn->display_info;
> >> + struct ingenic_drm_bridge *bridge = to_ingenic_drm_bridge(encoder);
> >> unsigned int cfg, rgbcfg = 0;
> >>
> >> - priv->panel_is_sharp = info->bus_flags & DRM_BUS_FLAG_SHARP_SIGNALS;
> >> + priv->panel_is_sharp = bridge->bus_cfg.flags & DRM_BUS_FLAG_SHARP_SIGNALS;
> >>
> >> if (priv->panel_is_sharp) {
> >> cfg = JZ_LCD_CFG_MODE_SPECIAL_TFT_1 | JZ_LCD_CFG_REV_POLARITY;
> >> @@ -685,19 +698,19 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
> >> cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
> >> if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> >> cfg |= JZ_LCD_CFG_VSYNC_ACTIVE_LOW;
> >> - if (info->bus_flags & DRM_BUS_FLAG_DE_LOW)
> >> + if (bridge->bus_cfg.flags & DRM_BUS_FLAG_DE_LOW)
> >> cfg |= JZ_LCD_CFG_DE_ACTIVE_LOW;
> >> - if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> >> + if (bridge->bus_cfg.flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> >> cfg |= JZ_LCD_CFG_PCLK_FALLING_EDGE;
> >>
> >> if (!priv->panel_is_sharp) {
> >> - if (conn->connector_type == DRM_MODE_CONNECTOR_TV) {
> >> + if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV) {
> >> if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> >> cfg |= JZ_LCD_CFG_MODE_TV_OUT_I;
> >> else
> >> cfg |= JZ_LCD_CFG_MODE_TV_OUT_P;
> >> } else {
> >> - switch (*info->bus_formats) {
> >> + switch (bridge->bus_cfg.format) {
> >> case MEDIA_BUS_FMT_RGB565_1X16:
> >> cfg |= JZ_LCD_CFG_MODE_GENERIC_16BIT;
> >> break;
> >> @@ -723,20 +736,29 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
> >> regmap_write(priv->map, JZ_REG_LCD_RGBC, rgbcfg);
> >> }
> >>
> >> -static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder,
> >> - struct drm_crtc_state *crtc_state,
> >> - struct drm_connector_state *conn_state)
> >> +static int ingenic_drm_bridge_attach(struct drm_bridge *bridge,
> >> + enum drm_bridge_attach_flags flags)
> >> +{
> >> + struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
> >> +
> >> + return drm_bridge_attach(bridge->encoder, ib->next_bridge,
> >> + &ib->bridge, flags);
> >> +}
> >> +
> >> +static int ingenic_drm_bridge_atomic_check(struct drm_bridge *bridge,
> >> + struct drm_bridge_state *bridge_state,
> >> + struct drm_crtc_state *crtc_state,
> >> + struct drm_connector_state *conn_state)
> >> {
> >> - struct drm_display_info *info = &conn_state->connector->display_info;
> >> struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> >> + struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
> >>
> >> - if (info->num_bus_formats != 1)
> >> - return -EINVAL;
> >> + ib->bus_cfg = bridge_state->output_bus_cfg;
> >>
> >> if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV)
> >> return 0;
> >>
> >> - switch (*info->bus_formats) {
> >> + switch (bridge_state->output_bus_cfg.format) {
> >> case MEDIA_BUS_FMT_RGB888_3X8:
> >> case MEDIA_BUS_FMT_RGB888_3X8_DELTA:
> >> /*
> >> @@ -900,8 +922,16 @@ static const struct drm_crtc_helper_funcs ingenic_drm_crtc_helper_funcs = {
> >> };
> >>
> >> static const struct drm_encoder_helper_funcs ingenic_drm_encoder_helper_funcs = {
> >> - .atomic_mode_set = ingenic_drm_encoder_atomic_mode_set,
> >> - .atomic_check = ingenic_drm_encoder_atomic_check,
> >> + .atomic_mode_set = ingenic_drm_encoder_atomic_mode_set,
> >> +};
> >> +
> >> +static const struct drm_bridge_funcs ingenic_drm_bridge_funcs = {
> >> + .attach = ingenic_drm_bridge_attach,
> >> + .atomic_check = ingenic_drm_bridge_atomic_check,
> >> + .atomic_reset = drm_atomic_helper_bridge_reset,
> >> + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> >> + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> >> + .atomic_get_input_bus_fmts = drm_atomic_helper_bridge_propagate_bus_fmt,
> >> };
> >>
> >> static const struct drm_mode_config_funcs ingenic_drm_mode_config_funcs = {
> >> @@ -976,7 +1006,9 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
> >> struct drm_plane *primary;
> >> struct drm_bridge *bridge;
> >> struct drm_panel *panel;
> >> + struct drm_connector *connector;
> >> struct drm_encoder *encoder;
> >> + struct ingenic_drm_bridge *ib;
> >> struct drm_device *drm;
> >> void __iomem *base;
> >> long parent_rate;
> >> @@ -1154,20 +1186,36 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
> >> bridge = devm_drm_panel_bridge_add_typed(dev, panel,
> >> DRM_MODE_CONNECTOR_DPI);
> >>
> >> - encoder = drmm_plain_encoder_alloc(drm, NULL, DRM_MODE_ENCODER_DPI, NULL);
> >> - if (IS_ERR(encoder)) {
> >> - ret = PTR_ERR(encoder);
> >> + ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder,
> >> + NULL, DRM_MODE_ENCODER_DPI, NULL);
> >> + if (IS_ERR(ib)) {
> >> + ret = PTR_ERR(ib);
> >> dev_err(dev, "Failed to init encoder: %d\n", ret);
> >> return ret;
> >> }
> >>
> >> - encoder->possible_crtcs = 1;
> >> + encoder = &ib->encoder;
> >> + encoder->possible_crtcs = drm_crtc_mask(&priv->crtc);
> >>
> >> drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
> >>
> >> - ret = drm_bridge_attach(encoder, bridge, NULL, 0);
> >> - if (ret)
> >> + ib->bridge.funcs = &ingenic_drm_bridge_funcs;
> >> + ib->next_bridge = bridge;
> >> +
> >> + ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
> >> + DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> >
> > DRM_BRIDGE_ATTACH_NO_CONNECTOR makes it fundamentally incompatible
> > with synopsys/dw_hdmi.c
> >
> > That driver checks for DRM_BRIDGE_ATTACH_NO_CONNECTOR being NOT present,
> > since it wants to register its own connector through
> > dw_hdmi_connector_create().
Does it ? The driver has
static int dw_hdmi_bridge_attach(struct drm_bridge *bridge,
enum drm_bridge_attach_flags flags)
{
struct dw_hdmi *hdmi = bridge->driver_private;
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
return drm_bridge_attach(bridge->encoder, hdmi->next_bridge,
bridge, flags);
return dw_hdmi_connector_create(hdmi);
}
If DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set, it will create a
connector, otherwise it will just attach to the next bridge. I'm using
it on a Renesas platform with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag
set, without any issue as far as I can tell.
> > It does it for a reason: the dw-hdmi is a multi-function driver which does
> > HDMI and DDC/EDID stuff in a single driver (because I/O registers and power
> > management seem to be shared).
>
> The IT66121 driver does all of that too, and does not need
> DRM_BRIDGE_ATTACH_NO_CONNECTOR. The drm_bridge_funcs struct has
> callbacks to handle cable detection and DDC stuff.
>
> > Since I do not see who could split this into a separate bridge and a connector driver
> > and test it on multiple SoC platforms (there are at least 3 or 4), I think modifying
> > the fundamentals of the dw-hdmi architecture just to get CI20 HDMI working is not
> > our turf.
>
> You could have a field in the dw-hdmi pdata structure, that would
> instruct the driver whether or not it should use the new API. Ugly, I
> know, and would probably duplicate a lot of code, but that would allow
> other drivers to be updated at a later date.
>
> > Therefore the code here should be able to detect if drm_bridge_attach() already
> > creates and attaches a connector and then skip the code below.
>
> Not that easy, unfortunately. On one side we have dw-hdmi which checks
> that DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set, and on the other side
> we have other drivers like the IT66121 which will fail if this flag is
> not set.
> >> + if (ret) {
> >> + dev_err(dev, "Unable to attach bridge\n");
> >> return ret;
> >> + }
> >> +
> >> + connector = drm_bridge_connector_init(drm, encoder);
> >> + if (IS_ERR(connector)) {
> >> + dev_err(dev, "Unable to init connector\n");
> >> + return PTR_ERR(connector);
> >> + }
> >> +
> >> + drm_connector_attach_encoder(connector, encoder);
> >> }
> >>
> >> drm_for_each_encoder(encoder, drm) {
> >> --
> >> 2.33.0
> >
> > I haven't replaced v2 with v3 in our test tree yet, but will do asap.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 27+ messages in thread