Hi,
This patchset rework the ingenic-drm driver, improving the code in
various places.
The most important change is the last patch, which updates the
ingenic-drm driver to use a top-level bridge per output, making use of
the bus format and flag negociation implemented in the bridge code. All
the external bridges are now attached with
DRM_BRIDGE_ATTACH_NO_CONNECTOR.
Cheers,
-Paul
Paul Cercueil (8):
drm/ingenic: Remove dead code
drm/ingenic: Simplify code by using hwdescs array
drm/ingenic: Use standard drm_atomic_helper_commit_tail
drm/ingenic: Add support for private objects
drm/ingenic: Move IPU scale settings to private state
drm/ingenic: Set DMA descriptor chain register when starting CRTC
drm/ingenic: Upload palette before frame
drm/ingenic: Attach bridge chain to encoders
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 287 ++++++++++++++++------
drivers/gpu/drm/ingenic/ingenic-ipu.c | 127 ++++++++--
2 files changed, 322 insertions(+), 92 deletions(-)
--
2.30.2
Instead of having one 'hwdesc' variable for the plane #0 and one for the
plane #1, use a 'hwdesc[2]' array, where the DMA hardware descriptors
are indexed by the plane's number.
Signed-off-by: Paul Cercueil <[email protected]>
---
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 38 ++++++++++++-----------
1 file changed, 20 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index e42eb43d8020..bc71ba44ccf4 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -49,8 +49,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[2];
struct ingenic_dma_hwdesc hwdesc_pal;
u16 palette[256] __aligned(16);
};
@@ -141,6 +140,13 @@ 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, bool use_f1)
+{
+ u32 offset = offsetof(struct ingenic_dma_hwdescs, hwdesc[use_f1]);
+
+ return priv->dma_hwdescs_phys + offset;
+}
+
static int ingenic_drm_update_pixclk(struct notifier_block *nb,
unsigned long action,
void *data)
@@ -562,6 +568,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
struct ingenic_dma_hwdesc *hwdesc;
unsigned int width, height, cpp, offset;
dma_addr_t addr;
+ bool use_f1;
u32 fourcc;
if (newstate && newstate->fb) {
@@ -569,16 +576,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;
+ use_f1 = 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[use_f1];
hwdesc->addr = addr;
hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
@@ -591,9 +596,9 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
if (fourcc == DRM_FORMAT_C8)
offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_pal);
else
- offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
+ offset = offsetof(struct ingenic_dma_hwdescs, hwdesc[0]);
- priv->dma_hwdescs->hwdesc_f0.next = priv->dma_hwdescs_phys + offset;
+ priv->dma_hwdescs->hwdesc[0].next = priv->dma_hwdescs_phys + offset;
crtc_state->color_mgmt_changed = fourcc == DRM_FORMAT_C8;
}
@@ -964,20 +969,17 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
/* 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;
+ dma_hwdesc_phys_f0 = dma_hwdesc_addr(priv, 0);
+ priv->dma_hwdescs->hwdesc[0].next = dma_hwdesc_phys_f0;
+ priv->dma_hwdescs->hwdesc[0].id = 0xf0;
/* 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;
+ dma_hwdesc_phys_f1 = dma_hwdesc_addr(priv, 1);
+ priv->dma_hwdescs->hwdesc[1].next = dma_hwdesc_phys_f1;
+ priv->dma_hwdescs->hwdesc[1].id = 0xf1;
/* 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.next = dma_hwdesc_phys_f0;
priv->dma_hwdescs->hwdesc_pal.id = 0xc0;
priv->dma_hwdescs->hwdesc_pal.addr = priv->dma_hwdescs_phys
+ offsetof(struct ingenic_dma_hwdescs, palette);
--
2.30.2
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 <[email protected]>
---
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 3ed7c27a8dde..3fc01cffb00f 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -63,6 +63,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;
/*
@@ -98,8 +102,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) {
@@ -769,6 +781,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 = {
@@ -839,6 +873,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;
@@ -851,9 +890,15 @@ static void __maybe_unused ingenic_drm_release_rmem(void *d)
of_reserved_mem_device_release(d);
}
+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;
@@ -1132,6 +1177,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");
@@ -1142,6 +1201,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.30.2
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 <[email protected]>
---
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.30.2
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 <[email protected]>
---
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 3fc01cffb00f..2eef174165a2 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -184,6 +184,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.30.2
The priv->ipu_plane would get a different value further down the code,
without the first assigned value being read first; so the first
assignation can be dropped.
Signed-off-by: Paul Cercueil <[email protected]>
---
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index d261f7a03b18..e42eb43d8020 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -984,9 +984,6 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
priv->dma_hwdescs->hwdesc_pal.cmd = JZ_LCD_CMD_ENABLE_PAL
| (sizeof(priv->dma_hwdescs->palette) / 4);
- if (soc_info->has_osd)
- priv->ipu_plane = drm_plane_from_index(drm, 0);
-
primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
--
2.30.2
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.
Signed-off-by: Paul Cercueil <[email protected]>
---
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 69 +++++++++++++++++++----
1 file changed, 59 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 2eef174165a2..7ae48ead3ab6 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -65,6 +65,7 @@ struct jz_soc_info {
struct ingenic_drm_private_state {
struct drm_private_state base;
+ bool use_palette;
};
struct ingenic_drm {
@@ -112,6 +113,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) {
@@ -159,6 +184,13 @@ static inline dma_addr_t dma_hwdesc_addr(const struct ingenic_drm *priv, bool us
return priv->dma_hwdescs_phys + offset;
}
+static inline dma_addr_t dma_hwdesc_pal_addr(const struct ingenic_drm *priv)
+{
+ u32 offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_pal);
+
+ return priv->dma_hwdescs_phys + offset;
+}
+
static int ingenic_drm_update_pixclk(struct notifier_block *nb,
unsigned long action,
void *data)
@@ -181,11 +213,19 @@ 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;
+
+ priv_state = ingenic_drm_get_new_priv_state(priv, state);
+ if (WARN_ON(!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));
+ if (priv_state->use_palette)
+ regmap_write(priv->map, JZ_REG_LCD_DA0, dma_hwdesc_pal_addr(priv));
+ else
+ 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,
@@ -391,6 +431,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;
@@ -403,6 +444,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,
@@ -421,6 +466,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.
@@ -580,10 +628,11 @@ 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);
+ struct ingenic_drm_private_state *priv_state;
struct drm_crtc_state *crtc_state;
struct ingenic_dma_hwdesc *hwdesc;
- unsigned int width, height, cpp, offset;
- dma_addr_t addr;
+ unsigned int width, height, cpp;
+ dma_addr_t addr, next_addr;
bool use_f1;
u32 fourcc;
@@ -599,23 +648,23 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
height = newstate->src_h >> 16;
cpp = newstate->fb->format->cpp[0];
+ priv_state = ingenic_drm_get_new_priv_state(priv, state);
+ if (priv_state && priv_state->use_palette)
+ next_addr = dma_hwdesc_pal_addr(priv);
+ else
+ next_addr = dma_hwdesc_addr(priv, use_f1);
+
hwdesc = &priv->dma_hwdescs->hwdesc[use_f1];
hwdesc->addr = addr;
hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
+ hwdesc->next = next_addr;
if (drm_atomic_crtc_needs_modeset(crtc_state)) {
fourcc = newstate->fb->format->format;
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[0]);
-
- priv->dma_hwdescs->hwdesc[0].next = priv->dma_hwdescs_phys + offset;
-
crtc_state->color_mgmt_changed = fourcc == DRM_FORMAT_C8;
}
--
2.30.2
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 <[email protected]>
---
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 7ae48ead3ab6..09d5dd298078 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>
@@ -107,6 +108,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)
{
@@ -679,11 +693,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;
@@ -696,19 +709,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;
@@ -734,20 +747,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:
/*
@@ -911,8 +933,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 = {
@@ -958,7 +988,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;
@@ -1146,20 +1178,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.30.2
By making the CRTC's .vblank_enable() function return an error when it
is known that the hardware won't deliver a VBLANK, we can drop the
ingenic_drm_atomic_helper_commit_tail() function and use the standard
drm_atomic_helper_commit_tail() function instead.
Signed-off-by: Paul Cercueil <[email protected]>
---
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 28 ++++-------------------
1 file changed, 4 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index bc71ba44ccf4..3ed7c27a8dde 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -706,29 +706,6 @@ static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder,
}
}
-static void ingenic_drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
-{
- /*
- * Just your regular drm_atomic_helper_commit_tail(), but only calls
- * drm_atomic_helper_wait_for_vblanks() if priv->no_vblank.
- */
- struct drm_device *dev = old_state->dev;
- struct ingenic_drm *priv = drm_device_get_priv(dev);
-
- drm_atomic_helper_commit_modeset_disables(dev, old_state);
-
- drm_atomic_helper_commit_planes(dev, old_state, 0);
-
- drm_atomic_helper_commit_modeset_enables(dev, old_state);
-
- drm_atomic_helper_commit_hw_done(old_state);
-
- if (!priv->no_vblank)
- drm_atomic_helper_wait_for_vblanks(dev, old_state);
-
- drm_atomic_helper_cleanup_planes(dev, old_state);
-}
-
static irqreturn_t ingenic_drm_irq_handler(int irq, void *arg)
{
struct ingenic_drm *priv = drm_device_get_priv(arg);
@@ -749,6 +726,9 @@ static int ingenic_drm_enable_vblank(struct drm_crtc *crtc)
{
struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
+ if (priv->no_vblank)
+ return -EWOULDBLOCK;
+
regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
JZ_LCD_CTRL_EOF_IRQ, JZ_LCD_CTRL_EOF_IRQ);
@@ -856,7 +836,7 @@ static const struct drm_mode_config_funcs ingenic_drm_mode_config_funcs = {
};
static struct drm_mode_config_helper_funcs ingenic_drm_mode_config_helpers = {
- .atomic_commit_tail = ingenic_drm_atomic_helper_commit_tail,
+ .atomic_commit_tail = drm_atomic_helper_commit_tail,
};
static void ingenic_drm_unbind_all(void *d)
--
2.30.2
Am 08.08.21 um 15:45 schrieb Paul Cercueil:
> The priv->ipu_plane would get a different value further down the code,
> without the first assigned value being read first; so the first
> assignation can be dropped.
>
> Signed-off-by: Paul Cercueil <[email protected]>
Acked-by: Thomas Zimmermann <[email protected]>
> ---
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index d261f7a03b18..e42eb43d8020 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -984,9 +984,6 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
> priv->dma_hwdescs->hwdesc_pal.cmd = JZ_LCD_CMD_ENABLE_PAL
> | (sizeof(priv->dma_hwdescs->palette) / 4);
>
> - if (soc_info->has_osd)
> - priv->ipu_plane = drm_plane_from_index(drm, 0);
> -
> primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
>
> drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
Hi Joe,
Le dim., ao?t 8 2021 at 11:27:34 -0700, Joe Perches <[email protected]>
a ?crit :
> On Sun, 2021-08-08 at 19:58 +0200, Thomas Zimmermann wrote:
>>
>> Am 08.08.21 um 15:45 schrieb Paul Cercueil:
>> > The priv->ipu_plane would get a different value further down the
>> code,
>> > without the first assigned value being read first; so the first
>> > assignation can be dropped.
>> >
>> > Signed-off-by: Paul Cercueil <[email protected]>
>>
>> Acked-by: Thomas Zimmermann <[email protected]>
>
> I think this is at best an incomplete description.
>
> How is it known that this priv->ipu_plane assignment isn't
> necessary for any path of any failure path after this assignment
> and before the new assignment?
It is only used in the .atomic_begin and .atomic_check callbacks of the
CRTC. These will only ever be called after the call to
drm_dev_register() which happens at the end of the probe function.
Cheers,
-Paul
>> > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> []
>> > @@ -984,9 +984,6 @@ static int ingenic_drm_bind(struct device
>> *dev, bool has_components)
>> > priv->dma_hwdescs->hwdesc_pal.cmd = JZ_LCD_CMD_ENABLE_PAL
>> > | (sizeof(priv->dma_hwdescs->palette) / 4);
>> >
>> > - if (soc_info->has_osd)
>> > - priv->ipu_plane = drm_plane_from_index(drm, 0);
>> > -
>> > primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
>> >
>> > drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
>> >
>>
>
>
On Sun, 2021-08-08 at 19:58 +0200, Thomas Zimmermann wrote:
>
> Am 08.08.21 um 15:45 schrieb Paul Cercueil:
> > The priv->ipu_plane would get a different value further down the code,
> > without the first assigned value being read first; so the first
> > assignation can be dropped.
> >
> > Signed-off-by: Paul Cercueil <[email protected]>
>
> Acked-by: Thomas Zimmermann <[email protected]>
I think this is at best an incomplete description.
How is it known that this priv->ipu_plane assignment isn't
necessary for any path of any failure path after this assignment
and before the new assignment?
> > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
[]
> > @@ -984,9 +984,6 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
> > ?? priv->dma_hwdescs->hwdesc_pal.cmd = JZ_LCD_CMD_ENABLE_PAL
> > ?? | (sizeof(priv->dma_hwdescs->palette) / 4);
> >
> > - if (soc_info->has_osd)
> > - priv->ipu_plane = drm_plane_from_index(drm, 0);
> > -
> > ?? primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
> >
> > ?? drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
> >
>
> Am 08.08.2021 um 21:04 schrieb Paul Cercueil <[email protected]>:
>
> Hi Nikolaus,
>
> Le dim., août 8 2021 at 20:57:09 +0200, H. Nikolaus Schaller <[email protected]> a écrit :
>> Hi Paul,
>> all other patches apply cleanly but this one fails on top of v5.14-rc4.
>> What base are you using?
>> BR and thanks,
>> Nikolaus
>
> The base is drm-misc (https://cgit.freedesktop.org/drm/drm-misc), branch drm-misc-next.
Ok, fine!
BR and thanks,
Nikolaus
>
> Cheers,
> -Paul
>
>
>>> Am 08.08.2021 um 15:45 schrieb Paul Cercueil <[email protected]>:
>>> 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 <[email protected]>
>>> ---
>>> 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 7ae48ead3ab6..09d5dd298078 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>
>>> @@ -107,6 +108,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)
>>> {
>>> @@ -679,11 +693,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;
>>> @@ -696,19 +709,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;
>>> @@ -734,20 +747,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:
>>> /*
>>> @@ -911,8 +933,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 = {
>>> @@ -958,7 +988,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;
>>> @@ -1146,20 +1178,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.30.2
>
>
> Am 08.08.2021 um 21:06 schrieb H. Nikolaus Schaller <[email protected]>:
>
>
>
>> Am 08.08.2021 um 21:04 schrieb Paul Cercueil <[email protected]>:
>>
>> Hi Nikolaus,
>>
>> Le dim., août 8 2021 at 20:57:09 +0200, H. Nikolaus Schaller <[email protected]> a écrit :
>>> Hi Paul,
>>> all other patches apply cleanly but this one fails on top of v5.14-rc4.
>>> What base are you using?
>>> BR and thanks,
>>> Nikolaus
>>
>> The base is drm-misc (https://cgit.freedesktop.org/drm/drm-misc), branch drm-misc-next.
>
> Ok, fine!
Contains 3 patches for drm/ingenic and after taking them first, I can apply the series.
Again, BR and thanks,
Nikolaus
Hi
Am 08.08.21 um 15:45 schrieb Paul Cercueil:
> Instead of having one 'hwdesc' variable for the plane #0 and one for the
> plane #1, use a 'hwdesc[2]' array, where the DMA hardware descriptors
> are indexed by the plane's number.
>
> Signed-off-by: Paul Cercueil <[email protected]>
> ---
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 38 ++++++++++++-----------
> 1 file changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index e42eb43d8020..bc71ba44ccf4 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -49,8 +49,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[2];
> struct ingenic_dma_hwdesc hwdesc_pal;
> u16 palette[256] __aligned(16);
> };
> @@ -141,6 +140,13 @@ 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, bool use_f1)
Using the plane index instead of a boolean would be more aligned to the
way this function is being used.
> +{
> + u32 offset = offsetof(struct ingenic_dma_hwdescs, hwdesc[use_f1]);
use_f1 is a function parameter. Is offsetof guaranteed to be evaluated
at runtime?
> +
> + return priv->dma_hwdescs_phys + offset;
> +}
> +
> static int ingenic_drm_update_pixclk(struct notifier_block *nb,
> unsigned long action,
> void *data)
> @@ -562,6 +568,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
> struct ingenic_dma_hwdesc *hwdesc;
> unsigned int width, height, cpp, offset;
> dma_addr_t addr;
> + bool use_f1;
> u32 fourcc;
>
> if (newstate && newstate->fb) {
> @@ -569,16 +576,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;
> + use_f1 = 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[use_f1];
Maybe add a helper that looks up the hwdesc field for a given plane?
>
> hwdesc->addr = addr;
> hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
> @@ -591,9 +596,9 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
> if (fourcc == DRM_FORMAT_C8)
> offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_pal);
> else
> - offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
> + offset = offsetof(struct ingenic_dma_hwdescs, hwdesc[0]);
>
> - priv->dma_hwdescs->hwdesc_f0.next = priv->dma_hwdescs_phys + offset;
> + priv->dma_hwdescs->hwdesc[0].next = priv->dma_hwdescs_phys + offset;
Maybe priv->dma_hwdescs_phys + offset could be computed in a more
flexible helper than dma_hwdesc_addr().
>
> crtc_state->color_mgmt_changed = fourcc == DRM_FORMAT_C8;
> }
> @@ -964,20 +969,17 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>
>
> /* 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;
> + dma_hwdesc_phys_f0 = dma_hwdesc_addr(priv, 0);
> + priv->dma_hwdescs->hwdesc[0].next = dma_hwdesc_phys_f0;
> + priv->dma_hwdescs->hwdesc[0].id = 0xf0;
>
> /* 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;
> + dma_hwdesc_phys_f1 = dma_hwdesc_addr(priv, 1);
> + priv->dma_hwdescs->hwdesc[1].next = dma_hwdesc_phys_f1;
> + priv->dma_hwdescs->hwdesc[1].id = 0xf1;
>
> /* 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.next = dma_hwdesc_phys_f0;
> priv->dma_hwdescs->hwdesc_pal.id = 0xc0;
> priv->dma_hwdescs->hwdesc_pal.addr = priv->dma_hwdescs_phys
> + offsetof(struct ingenic_dma_hwdescs, palette);
>
Could the setup in these three blocks be moved into a common helper?
Best regards
Thomas
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
Hi Paul,
all other patches apply cleanly but this one fails on top of v5.14-rc4.
What base are you using?
BR and thanks,
Nikolaus
> Am 08.08.2021 um 15:45 schrieb Paul Cercueil <[email protected]>:
>
> 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 <[email protected]>
> ---
> 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 7ae48ead3ab6..09d5dd298078 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>
> @@ -107,6 +108,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)
> {
> @@ -679,11 +693,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;
> @@ -696,19 +709,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;
> @@ -734,20 +747,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:
> /*
> @@ -911,8 +933,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 = {
> @@ -958,7 +988,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;
> @@ -1146,20 +1178,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.30.2
>
Hi Nikolaus,
Le dim., ao?t 8 2021 at 20:57:09 +0200, H. Nikolaus Schaller
<[email protected]> a ?crit :
> Hi Paul,
> all other patches apply cleanly but this one fails on top of
> v5.14-rc4.
> What base are you using?
> BR and thanks,
> Nikolaus
The base is drm-misc (https://cgit.freedesktop.org/drm/drm-misc),
branch drm-misc-next.
Cheers,
-Paul
>> Am 08.08.2021 um 15:45 schrieb Paul Cercueil <[email protected]>:
>>
>> 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 <[email protected]>
>> ---
>> 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 7ae48ead3ab6..09d5dd298078 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>
>> @@ -107,6 +108,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)
>> {
>> @@ -679,11 +693,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;
>> @@ -696,19 +709,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;
>> @@ -734,20 +747,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:
>> /*
>> @@ -911,8 +933,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 = {
>> @@ -958,7 +988,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;
>> @@ -1146,20 +1178,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.30.2
>>
>
Le 08/08/2021 à 15:45, Paul Cercueil a écrit :
> By making the CRTC's .vblank_enable() function return an error when it
> is known that the hardware won't deliver a VBLANK, we can drop the
> ingenic_drm_atomic_helper_commit_tail() function and use the standard
> drm_atomic_helper_commit_tail() function instead.
>
> Signed-off-by: Paul Cercueil <[email protected]>
> ---
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 28 ++++-------------------
> 1 file changed, 4 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index bc71ba44ccf4..3ed7c27a8dde 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -706,29 +706,6 @@ static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder,
> }
> }
>
> -static void ingenic_drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
> -{
> - /*
> - * Just your regular drm_atomic_helper_commit_tail(), but only calls
> - * drm_atomic_helper_wait_for_vblanks() if priv->no_vblank.
> - */
> - struct drm_device *dev = old_state->dev;
> - struct ingenic_drm *priv = drm_device_get_priv(dev);
> -
> - drm_atomic_helper_commit_modeset_disables(dev, old_state);
> -
> - drm_atomic_helper_commit_planes(dev, old_state, 0);
> -
> - drm_atomic_helper_commit_modeset_enables(dev, old_state);
> -
> - drm_atomic_helper_commit_hw_done(old_state);
> -
> - if (!priv->no_vblank)
> - drm_atomic_helper_wait_for_vblanks(dev, old_state);
> -
> - drm_atomic_helper_cleanup_planes(dev, old_state);
> -}
>
Hi,
if this function is removed, shouldn't:
static struct drm_mode_config_helper_funcs
ingenic_drm_mode_config_helpers = {
.atomic_commit_tail = ingenic_drm_atomic_helper_commit_tail,
};
be updated as well?
I've not seen it in the serie.
Just my 2v.
CJ
Hi Christophe,
Le dim., ao?t 8 2021 at 21:50:04 +0200, Christophe JAILLET
<[email protected]> a ?crit :
> Le 08/08/2021 ? 15:45, Paul Cercueil a ?crit :
>> By making the CRTC's .vblank_enable() function return an error when
>> it
>> is known that the hardware won't deliver a VBLANK, we can drop the
>> ingenic_drm_atomic_helper_commit_tail() function and use the standard
>> drm_atomic_helper_commit_tail() function instead.
>>
>> Signed-off-by: Paul Cercueil <[email protected]>
>> ---
>> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 28
>> ++++-------------------
>> 1 file changed, 4 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> index bc71ba44ccf4..3ed7c27a8dde 100644
>> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> @@ -706,29 +706,6 @@ static int
>> ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder,
>> }
>> }
>> -static void ingenic_drm_atomic_helper_commit_tail(struct
>> drm_atomic_state *old_state)
>> -{
>> - /*
>> - * Just your regular drm_atomic_helper_commit_tail(), but only
>> calls
>> - * drm_atomic_helper_wait_for_vblanks() if priv->no_vblank.
>> - */
>> - struct drm_device *dev = old_state->dev;
>> - struct ingenic_drm *priv = drm_device_get_priv(dev);
>> -
>> - drm_atomic_helper_commit_modeset_disables(dev, old_state);
>> -
>> - drm_atomic_helper_commit_planes(dev, old_state, 0);
>> -
>> - drm_atomic_helper_commit_modeset_enables(dev, old_state);
>> -
>> - drm_atomic_helper_commit_hw_done(old_state);
>> -
>> - if (!priv->no_vblank)
>> - drm_atomic_helper_wait_for_vblanks(dev, old_state);
>> -
>> - drm_atomic_helper_cleanup_planes(dev, old_state);
>> -}
>>
>
> Hi,
> if this function is removed, shouldn't:
> static struct drm_mode_config_helper_funcs
> ingenic_drm_mode_config_helpers = {
> .atomic_commit_tail = ingenic_drm_atomic_helper_commit_tail,
> };
> be updated as well?
>
> I've not seen it in the serie.
It is there though :) At the bottom of this very patch.
> Just my 2v.
> CJ
Cheers,
-Paul
Le 08/08/2021 à 22:09, Paul Cercueil a écrit :
> Hi Christophe,
>
> Le dim., août 8 2021 at 21:50:04 +0200, Christophe JAILLET
> <[email protected]> a écrit :
>> Le 08/08/2021 à 15:45, Paul Cercueil a écrit :
>>> By making the CRTC's .vblank_enable() function return an error when it
>>> is known that the hardware won't deliver a VBLANK, we can drop the
>>> ingenic_drm_atomic_helper_commit_tail() function and use the standard
>>> drm_atomic_helper_commit_tail() function instead.
>>>
>>> Signed-off-by: Paul Cercueil <[email protected]>
>>> ---
>>> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 28 ++++-------------------
>>> 1 file changed, 4 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>> index bc71ba44ccf4..3ed7c27a8dde 100644
>>> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>> @@ -706,29 +706,6 @@ static int
>>> ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder,
>>> }
>>> }
>>> -static void ingenic_drm_atomic_helper_commit_tail(struct
>>> drm_atomic_state *old_state)
>>> -{
>>> - /*
>>> - * Just your regular drm_atomic_helper_commit_tail(), but only
>>> calls
>>> - * drm_atomic_helper_wait_for_vblanks() if priv->no_vblank.
>>> - */
>>> - struct drm_device *dev = old_state->dev;
>>> - struct ingenic_drm *priv = drm_device_get_priv(dev);
>>> -
>>> - drm_atomic_helper_commit_modeset_disables(dev, old_state);
>>> -
>>> - drm_atomic_helper_commit_planes(dev, old_state, 0);
>>> -
>>> - drm_atomic_helper_commit_modeset_enables(dev, old_state);
>>> -
>>> - drm_atomic_helper_commit_hw_done(old_state);
>>> -
>>> - if (!priv->no_vblank)
>>> - drm_atomic_helper_wait_for_vblanks(dev, old_state);
>>> -
>>> - drm_atomic_helper_cleanup_planes(dev, old_state);
>>> -}
>>>
>>
>> Hi,
>> if this function is removed, shouldn't:
>> static struct drm_mode_config_helper_funcs
>> ingenic_drm_mode_config_helpers = {
>> .atomic_commit_tail = ingenic_drm_atomic_helper_commit_tail,
>> };
>> be updated as well?
>>
>> I've not seen it in the serie.
>
> It is there though :) At the bottom of this very patch.
>
My email client played me some tricks, apparently!
Sorry for the noise.
CJ
>> Just my 2v.
>> CJ
>
> Cheers,
> -Paul
>
>
>
Hi Thomas,
Le dim., ao?t 8 2021 at 20:42:53 +0200, Thomas Zimmermann
<[email protected]> a ?crit :
> Hi
>
> Am 08.08.21 um 15:45 schrieb Paul Cercueil:
>> Instead of having one 'hwdesc' variable for the plane #0 and one for
>> the
>> plane #1, use a 'hwdesc[2]' array, where the DMA hardware descriptors
>> are indexed by the plane's number.
>>
>> Signed-off-by: Paul Cercueil <[email protected]>
>> ---
>> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 38
>> ++++++++++++-----------
>> 1 file changed, 20 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> index e42eb43d8020..bc71ba44ccf4 100644
>> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> @@ -49,8 +49,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[2];
>> struct ingenic_dma_hwdesc hwdesc_pal;
>> u16 palette[256] __aligned(16);
>> };
>> @@ -141,6 +140,13 @@ 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, bool use_f1)
>
> Using the plane index instead of a boolean would be more aligned to
> the way this function is being used.
Alright, I can do that.
>> +{
>> + u32 offset = offsetof(struct ingenic_dma_hwdescs, hwdesc[use_f1]);
>
> use_f1 is a function parameter. Is offsetof guaranteed to be
> evaluated at runtime?
The offsetof() macro could be defined like this:
#define offsetof(type, elm) ((size_t) &((type *) 0)->elm)
So I don't see a reason why this couldn't be evaluated at runtime, yes.
It's just that the value of "offset" is not known at compilation time
(unless the compiler does some constant propagation). In practice
though, this code works fine.
>> +
>> + return priv->dma_hwdescs_phys + offset;
>> +}
>> +
>> static int ingenic_drm_update_pixclk(struct notifier_block *nb,
>> unsigned long action,
>> void *data)
>> @@ -562,6 +568,7 @@ static void
>> ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>> struct ingenic_dma_hwdesc *hwdesc;
>> unsigned int width, height, cpp, offset;
>> dma_addr_t addr;
>> + bool use_f1;
>> u32 fourcc;
>> if (newstate && newstate->fb) {
>> @@ -569,16 +576,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;
>> + use_f1 = 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[use_f1];
>
> Maybe add a helper that looks up the hwdesc field for a given plane?
Sure.
>> hwdesc->addr = addr;
>> hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
>> @@ -591,9 +596,9 @@ static void
>> ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>> if (fourcc == DRM_FORMAT_C8)
>> offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_pal);
>> else
>> - offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
>> + offset = offsetof(struct ingenic_dma_hwdescs, hwdesc[0]);
>> - priv->dma_hwdescs->hwdesc_f0.next = priv->dma_hwdescs_phys +
>> offset;
>> + priv->dma_hwdescs->hwdesc[0].next = priv->dma_hwdescs_phys +
>> offset;
>
> Maybe priv->dma_hwdescs_phys + offset could be computed in a more
> flexible helper than dma_hwdesc_addr().
>
>> crtc_state->color_mgmt_changed = fourcc == DRM_FORMAT_C8;
>> }
>> @@ -964,20 +969,17 @@ static int ingenic_drm_bind(struct device
>> *dev, bool has_components)
>> /* 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;
>> + dma_hwdesc_phys_f0 = dma_hwdesc_addr(priv, 0);
>> + priv->dma_hwdescs->hwdesc[0].next = dma_hwdesc_phys_f0;
>> + priv->dma_hwdescs->hwdesc[0].id = 0xf0;
>> /* 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;
>> + dma_hwdesc_phys_f1 = dma_hwdesc_addr(priv, 1);
>> + priv->dma_hwdescs->hwdesc[1].next = dma_hwdesc_phys_f1;
>> + priv->dma_hwdescs->hwdesc[1].id = 0xf1;
>> /* 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.next = dma_hwdesc_phys_f0;
>> priv->dma_hwdescs->hwdesc_pal.id = 0xc0;
>> priv->dma_hwdescs->hwdesc_pal.addr = priv->dma_hwdescs_phys
>> + offsetof(struct ingenic_dma_hwdescs, palette);
>>
>
> Could the setup in these three blocks be moved into a common helper?
Yes.
Thanks for the review.
Cheers,
-Paul
Hi Paul,
quick feedback: our HDMI on top compiles fine after fixing 2 merge conflicts, but dos not yet work.
Will need some spare time with access to the CI20 board to research the issue, i.e. can not give feedback immediately.
BR and thanks,
Nikolaus
> Am 08.08.2021 um 21:12 schrieb H. Nikolaus Schaller <[email protected]>:
>
>
>
>> Am 08.08.2021 um 21:06 schrieb H. Nikolaus Schaller <[email protected]>:
>>
>>
>>
>>> Am 08.08.2021 um 21:04 schrieb Paul Cercueil <[email protected]>:
>>>
>>> Hi Nikolaus,
>>>
>>> Le dim., août 8 2021 at 20:57:09 +0200, H. Nikolaus Schaller <[email protected]> a écrit :
>>>> Hi Paul,
>>>> all other patches apply cleanly but this one fails on top of v5.14-rc4.
>>>> What base are you using?
>>>> BR and thanks,
>>>> Nikolaus
>>>
>>> The base is drm-misc (https://cgit.freedesktop.org/drm/drm-misc), branch drm-misc-next.
>>
>> Ok, fine!
>
> Contains 3 patches for drm/ingenic and after taking them first, I can apply the series.
>
> Again, BR and thanks,
> Nikolaus
>
> _______________________________________________
> https://projects.goldelico.com/p/gta04-kernel/
> Letux-kernel mailing list
> [email protected]
> http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel
Hi Nikolaus,
Le lun., ao?t 9 2021 at 13:14:03 +0200, H. Nikolaus Schaller
<[email protected]> a ?crit :
> Hi Paul,
> quick feedback: our HDMI on top compiles fine after fixing 2 merge
> conflicts, but dos not yet work.
> Will need some spare time with access to the CI20 board to research
> the issue, i.e. can not give feedback immediately.
Alright, no problem. I'll be back home in about 2 weeks and then I can
test on my CI20 as well.
Cheers,
-Paul
> BR and thanks,
> Nikolaus
>
>> Am 08.08.2021 um 21:12 schrieb H. Nikolaus Schaller
>> <[email protected]>:
>>
>>
>>
>>> Am 08.08.2021 um 21:06 schrieb H. Nikolaus Schaller
>>> <[email protected]>:
>>>
>>>
>>>
>>>> Am 08.08.2021 um 21:04 schrieb Paul Cercueil
>>>> <[email protected]>:
>>>>
>>>> Hi Nikolaus,
>>>>
>>>> Le dim., ao?t 8 2021 at 20:57:09 +0200, H. Nikolaus Schaller
>>>> <[email protected]> a ?crit :
>>>>> Hi Paul,
>>>>> all other patches apply cleanly but this one fails on top of
>>>>> v5.14-rc4.
>>>>> What base are you using?
>>>>> BR and thanks,
>>>>> Nikolaus
>>>>
>>>> The base is drm-misc (https://cgit.freedesktop.org/drm/drm-misc),
>>>> branch drm-misc-next.
>>>
>>> Ok, fine!
>>
>> Contains 3 patches for drm/ingenic and after taking them first, I
>> can apply the series.
>>
>> Again, BR and thanks,
>> Nikolaus
>>
>> _______________________________________________
>> https://projects.goldelico.com/p/gta04-kernel/
>> Letux-kernel mailing list
>> [email protected]
>> http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel
>
On Monday, 9 August 2021 18:22:12 CEST Paul Cercueil wrote:
>
> Le lun., ao?t 9 2021 at 13:14:03 +0200, H. Nikolaus Schaller
<[email protected]> a ?crit :
> >
> > quick feedback: our HDMI on top compiles fine after fixing 2 merge
> > conflicts, but dos not yet work.
> > Will need some spare time with access to the CI20 board to research
> > the issue, i.e. can not give feedback immediately.
>
> Alright, no problem. I'll be back home in about 2 weeks and then I can
> test on my CI20 as well.
Just for reference, I looked into this initialisation failure. The HDMI
peripheral driver gets initialised satisfactorily...
dw-hdmi-ingenic 10180000.hdmi: Detected HDMI TX controller v1.31a with HDCP
(DWC HDMI 3D TX PHY)
dw-hdmi-ingenic 10180000.hdmi: registered DesignWare HDMI I2C bus driver
But then the reported error occurs in the DRM driver:
ingenic-drm 13050000.lcdc0: Unable to init connector
ingenic-drm: probe of 13050000.lcdc0 failed with error -22
This originates in a call to drm_bridge_connector_init from ingenic_drm_bind:
connector = drm_bridge_connector_init(drm, encoder);
The invoked function iterates over the registered bridges, one of which seems
to be the HDMI peripheral (it has bridge operations defined identically to
those specified in the Synopsys driver), but the type member of the drm_bridge
structure is set to 0 (DRM_MODE_CONNECTOR_Unknown).
I might expect the bridge to expose a type acquired from its connector, but I
don't see this propagation occurring in the Synopsys driver: dw_hdmi_probe
sets the bridge operations and other members of the drm_bridge structure, but
it doesn't set the type.
Also, it might be possible that dw_hdmi_connector_detect (exposed as the
detect operation) is not getting called, and this would explain why the
bridge's connector member does not have the connector_type set, either (since
it is also set to 0).
Paul
Hi Paul,
Le mar., ao?t 10 2021 at 01:17:20 +0200, Paul Boddie
<[email protected]> a ?crit :
> On Monday, 9 August 2021 18:22:12 CEST Paul Cercueil wrote:
>>
>> Le lun., ao?t 9 2021 at 13:14:03 +0200, H. Nikolaus Schaller
> <[email protected]> a ?crit :
>> >
>> > quick feedback: our HDMI on top compiles fine after fixing 2 merge
>> > conflicts, but dos not yet work.
>> > Will need some spare time with access to the CI20 board to
>> research
>> > the issue, i.e. can not give feedback immediately.
>>
>> Alright, no problem. I'll be back home in about 2 weeks and then I
>> can
>> test on my CI20 as well.
>
> Just for reference, I looked into this initialisation failure. The
> HDMI
> peripheral driver gets initialised satisfactorily...
>
> dw-hdmi-ingenic 10180000.hdmi: Detected HDMI TX controller v1.31a
> with HDCP
> (DWC HDMI 3D TX PHY)
> dw-hdmi-ingenic 10180000.hdmi: registered DesignWare HDMI I2C bus
> driver
>
> But then the reported error occurs in the DRM driver:
>
> ingenic-drm 13050000.lcdc0: Unable to init connector
> ingenic-drm: probe of 13050000.lcdc0 failed with error -22
>
> This originates in a call to drm_bridge_connector_init from
> ingenic_drm_bind:
>
> connector = drm_bridge_connector_init(drm, encoder);
>
> The invoked function iterates over the registered bridges, one of
> which seems
> to be the HDMI peripheral (it has bridge operations defined
> identically to
> those specified in the Synopsys driver), but the type member of the
> drm_bridge
> structure is set to 0 (DRM_MODE_CONNECTOR_Unknown).
>
> I might expect the bridge to expose a type acquired from its
> connector, but I
> don't see this propagation occurring in the Synopsys driver:
> dw_hdmi_probe
> sets the bridge operations and other members of the drm_bridge
> structure, but
> it doesn't set the type.
>
> Also, it might be possible that dw_hdmi_connector_detect (exposed as
> the
> detect operation) is not getting called, and this would explain why
> the
> bridge's connector member does not have the connector_type set,
> either (since
> it is also set to 0).
From what I understand the last bridge in the chained list is supposed
to set the connector type. The HDMI driver's probe function should get
a pointer to the next bridge in the queue and attach it (see how
ite-it66121.c does it). The last bridge in the queue should be
"hdmi-connector" (display-connector.c) which will effectively set the
connector type.
Cheers,
-Paul
On Sun, Aug 08, 2021 at 03:45:21PM +0200, Paul Cercueil wrote:
> By making the CRTC's .vblank_enable() function return an error when it
> is known that the hardware won't deliver a VBLANK, we can drop the
> ingenic_drm_atomic_helper_commit_tail() function and use the standard
> drm_atomic_helper_commit_tail() function instead.
>
> Signed-off-by: Paul Cercueil <[email protected]>
> ---
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 28 ++++-------------------
> 1 file changed, 4 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index bc71ba44ccf4..3ed7c27a8dde 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -706,29 +706,6 @@ static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder,
> }
> }
>
> -static void ingenic_drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
> -{
> - /*
> - * Just your regular drm_atomic_helper_commit_tail(), but only calls
> - * drm_atomic_helper_wait_for_vblanks() if priv->no_vblank.
> - */
> - struct drm_device *dev = old_state->dev;
> - struct ingenic_drm *priv = drm_device_get_priv(dev);
> -
> - drm_atomic_helper_commit_modeset_disables(dev, old_state);
> -
> - drm_atomic_helper_commit_planes(dev, old_state, 0);
> -
> - drm_atomic_helper_commit_modeset_enables(dev, old_state);
> -
> - drm_atomic_helper_commit_hw_done(old_state);
> -
> - if (!priv->no_vblank)
> - drm_atomic_helper_wait_for_vblanks(dev, old_state);
> -
> - drm_atomic_helper_cleanup_planes(dev, old_state);
> -}
> -
> static irqreturn_t ingenic_drm_irq_handler(int irq, void *arg)
> {
> struct ingenic_drm *priv = drm_device_get_priv(arg);
> @@ -749,6 +726,9 @@ static int ingenic_drm_enable_vblank(struct drm_crtc *crtc)
> {
> struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>
> + if (priv->no_vblank)
> + return -EWOULDBLOCK;
I think other drivers return EINVAL here. I'm not sure exactly this is
specified, but the errno here is visible to userspace.
Maybe would be good to update the kerneldoc for this hook?
Anyway this is great, with the errno fixed:
Reviewed-by: Daniel Vetter <[email protected]>
> +
> regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
> JZ_LCD_CTRL_EOF_IRQ, JZ_LCD_CTRL_EOF_IRQ);
>
> @@ -856,7 +836,7 @@ static const struct drm_mode_config_funcs ingenic_drm_mode_config_funcs = {
> };
>
> static struct drm_mode_config_helper_funcs ingenic_drm_mode_config_helpers = {
> - .atomic_commit_tail = ingenic_drm_atomic_helper_commit_tail,
> + .atomic_commit_tail = drm_atomic_helper_commit_tail,
> };
>
> static void ingenic_drm_unbind_all(void *d)
> --
> 2.30.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi Daniel,
Le mar., ao?t 10 2021 at 12:20:38 +0200, Daniel Vetter
<[email protected]> a ?crit :
> On Sun, Aug 08, 2021 at 03:45:21PM +0200, Paul Cercueil wrote:
>> By making the CRTC's .vblank_enable() function return an error when
>> it
>> is known that the hardware won't deliver a VBLANK, we can drop the
>> ingenic_drm_atomic_helper_commit_tail() function and use the
>> standard
>> drm_atomic_helper_commit_tail() function instead.
>>
>> Signed-off-by: Paul Cercueil <[email protected]>
>> ---
>> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 28
>> ++++-------------------
>> 1 file changed, 4 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> index bc71ba44ccf4..3ed7c27a8dde 100644
>> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> @@ -706,29 +706,6 @@ static int
>> ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder,
>> }
>> }
>>
>> -static void ingenic_drm_atomic_helper_commit_tail(struct
>> drm_atomic_state *old_state)
>> -{
>> - /*
>> - * Just your regular drm_atomic_helper_commit_tail(), but only
>> calls
>> - * drm_atomic_helper_wait_for_vblanks() if priv->no_vblank.
>> - */
>> - struct drm_device *dev = old_state->dev;
>> - struct ingenic_drm *priv = drm_device_get_priv(dev);
>> -
>> - drm_atomic_helper_commit_modeset_disables(dev, old_state);
>> -
>> - drm_atomic_helper_commit_planes(dev, old_state, 0);
>> -
>> - drm_atomic_helper_commit_modeset_enables(dev, old_state);
>> -
>> - drm_atomic_helper_commit_hw_done(old_state);
>> -
>> - if (!priv->no_vblank)
>> - drm_atomic_helper_wait_for_vblanks(dev, old_state);
>> -
>> - drm_atomic_helper_cleanup_planes(dev, old_state);
>> -}
>> -
>> static irqreturn_t ingenic_drm_irq_handler(int irq, void *arg)
>> {
>> struct ingenic_drm *priv = drm_device_get_priv(arg);
>> @@ -749,6 +726,9 @@ static int ingenic_drm_enable_vblank(struct
>> drm_crtc *crtc)
>> {
>> struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>>
>> + if (priv->no_vblank)
>> + return -EWOULDBLOCK;
>
> I think other drivers return EINVAL here. I'm not sure exactly this is
> specified, but the errno here is visible to userspace.
>
> Maybe would be good to update the kerneldoc for this hook?
>
> Anyway this is great, with the errno fixed:
>
> Reviewed-by: Daniel Vetter <[email protected]>
I thought about it afterwards, that my error code wasn't great. In my
mind it was "the driver will hang while waiting for vblank" hence
-EWOULDBLOCK ;)
I'll need to v2 anyway so I'll change it to -EINVAL then.
Cheers,
-Paul
>
>> +
>> regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
>> JZ_LCD_CTRL_EOF_IRQ, JZ_LCD_CTRL_EOF_IRQ);
>>
>> @@ -856,7 +836,7 @@ static const struct drm_mode_config_funcs
>> ingenic_drm_mode_config_funcs = {
>> };
>>
>> static struct drm_mode_config_helper_funcs
>> ingenic_drm_mode_config_helpers = {
>> - .atomic_commit_tail = ingenic_drm_atomic_helper_commit_tail,
>> + .atomic_commit_tail = drm_atomic_helper_commit_tail,
>> };
>>
>> static void ingenic_drm_unbind_all(void *d)
>> --
>> 2.30.2
>>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
On Tuesday, 10 August 2021 09:52:36 CEST Paul Cercueil wrote:
>
> Le mar., ao?t 10 2021 at 01:17:20 +0200, Paul Boddie <[email protected]> a
?crit :
> >
> > But then the reported error occurs in the DRM driver:
> >
> > ingenic-drm 13050000.lcdc0: Unable to init connector
> > ingenic-drm: probe of 13050000.lcdc0 failed with error -22
> >
> > This originates in a call to drm_bridge_connector_init from
> > ingenic_drm_bind:
> >
> > connector = drm_bridge_connector_init(drm, encoder);
> >
> > The invoked function iterates over the registered bridges, one of
> > which seems to be the HDMI peripheral (it has bridge operations defined
> > identically to those specified in the Synopsys driver), but the type
> > member of the drm_bridge structure is set to 0
> > (DRM_MODE_CONNECTOR_Unknown).
Although I had fancy ideas about finding the connector node in the device tree
and populating the necessary type details on the bridge, I decided to just add
the following to the Synopsys driver's dw_hdmi_probe function:
hdmi->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
This then lets the above invocation succeed and the Ingenic DRM driver
initialises. However, I get "Input Not Supported" on my display.
Conveniently, when indicating the necessary boot arguments...
env set bootargs ... drm.debug=0x3f
...as suggested to me on a previous occasion, the /sys/kernel/debug/dri/0/
state file indicates the following:
plane[31]: plane-0
crtc=crtc-0
fb=36
allocated by = Xorg
refcount=2
format=XR24 little-endian (0x34325258)
modifier=0x0
size=1280x1024
layers:
size[0]=1280x1024
pitch[0]=5120
offset[0]=0
obj[0]:
name=0
refcount=3
start=00010000
size=5242880
imported=no
paddr=0x00c00000
vaddr=3eb0c080
crtc-pos=1280x1024+0+0
src-pos=1280.000000x1024.000000+0.000000+0.000000
rotation=1
normalized-zpos=0
color-encoding=ITU-R BT.601 YCbCr
color-range=YCbCr limited range
plane[33]: plane-1
crtc=(null)
fb=0
crtc-pos=0x0+0+0
src-pos=0.000000x0.000000+0.000000+0.000000
rotation=1
normalized-zpos=0
color-encoding=ITU-R BT.601 YCbCr
color-range=YCbCr limited range
crtc[32]: crtc-0
enable=1
active=1
self_refresh_active=0
planes_changed=0
mode_changed=0
active_changed=0
connectors_changed=0
color_mgmt_changed=0
plane_mask=1
connector_mask=1
encoder_mask=1
mode: "": 60 108000 1280 1328 1440 1688 1024 1025 1028 1066 0x0 0x5
connector[35]: HDMI-A-1
crtc=crtc-0
self_refresh_aware=0
I suspect that we may be dealing with an incompatible bus format again, but I
may be quite wrong about that, too.
Here is the result of running modetest using...
sudo ./modetest -D /dev/dri/card0 -M ingenic-drm
Encoders:
id crtc type possible crtcs possible clones
34 32 DPI 0x00000001 0x00000001
Connectors:
id encoder status name size (mm) modes
encoders
35 34 connected HDMI-A-1 340x270 17 34
modes:
index name refresh (Hz) hdisp hss hse htot vdisp vss vse vtot)
#0 1280x1024 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 flags:
phsync, pvsync; type: preferred, driver
#1 1280x1024 75.02 1280 1296 1440 1688 1024 1025 1028 1066 135000 flags:
phsync, pvsync; type: driver
#2 1280x960 60.00 1280 1376 1488 1800 960 961 964 1000 108000 flags: phsync,
pvsync; type: driver
#3 1152x864 75.00 1152 1216 1344 1600 864 865 868 900 108000 flags: phsync,
pvsync; type: driver
#4 1024x768 75.03 1024 1040 1136 1312 768 769 772 800 78750 flags: phsync,
pvsync; type: driver
#5 1024x768 70.07 1024 1048 1184 1328 768 771 777 806 75000 flags: nhsync,
nvsync; type: driver
#6 1024x768 60.00 1024 1048 1184 1344 768 771 777 806 65000 flags: nhsync,
nvsync; type: driver
#7 832x624 74.55 832 864 928 1152 624 625 628 667 57284 flags: nhsync,
nvsync; type: driver
#8 800x600 75.00 800 816 896 1056 600 601 604 625 49500 flags: phsync,
pvsync; type: driver
#9 800x600 72.19 800 856 976 1040 600 637 643 666 50000 flags: phsync,
pvsync; type: driver
#10 800x600 60.32 800 840 968 1056 600 601 605 628 40000 flags: phsync,
pvsync; type: driver
#11 800x600 56.25 800 824 896 1024 600 601 603 625 36000 flags: phsync,
pvsync; type: driver
#12 640x480 75.00 640 656 720 840 480 481 484 500 31500 flags: nhsync,
nvsync; type: driver
#13 640x480 72.81 640 664 704 832 480 489 492 520 31500 flags: nhsync,
nvsync; type: driver
#14 640x480 66.67 640 704 768 864 480 483 486 525 30240 flags: nhsync,
nvsync; type: driver
#15 640x480 59.94 640 656 752 800 480 490 492 525 25175 flags: nhsync,
nvsync; type: driver
#16 720x400 70.08 720 738 846 900 400 412 414 449 28320 flags: nhsync,
pvsync; type: driver
props:
1 EDID:
flags: immutable blob
blobs:
value:
00ffffffffffff00047232ad01010101
2d0e010380221b782aaea5a6544c9926
145054bfef0081808140714f01010101
010101010101302a009851002a403070
1300520e1100001e000000ff00343435
3030353444454330300a000000fc0041
4c313731350a202020202020000000fd
00384c1e520e000a2020202020200051
2 DPMS:
flags: enum
enums: On=0 Standby=1 Suspend=2 Off=3
value: 3
5 link-status:
flags: enum
enums: Good=0 Bad=1
value: 0
6 non-desktop:
flags: immutable range
values: 0 1
value: 0
4 TILE:
flags: immutable blob
blobs:
value:
20 CRTC_ID:
flags: object
value: 32
CRTCs:
id fb pos size
32 36 (0,0) (1280x1024)
#0 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 flags: phsync,
pvsync; type:
props:
22 ACTIVE:
flags: range
values: 0 1
value: 0
23 MODE_ID:
flags: blob
blobs:
value:
e0a5010000053005a005980600000004
010404042a0400003c00000005000000
00000000000000000000000000000000
00000000000000000000000000000000
00000000
19 OUT_FENCE_PTR:
flags: range
values: 0 18446744073709551615
value: 0
24 VRR_ENABLED:
flags: range
values: 0 1
value: 0
28 GAMMA_LUT:
flags: blob
blobs:
value:
29 GAMMA_LUT_SIZE:
flags: immutable range
values: 0 4294967295
value: 256
Planes:
id crtc fb CRTC x,y x,y gamma size possible crtcs
31 32 36 0,0 0,0 0 0x00000001
formats: XR15 RG16 RG24 XR24 XR30
props:
8 type:
flags: immutable enum
enums: Overlay=0 Primary=1 Cursor=2
value: 1
17 FB_ID:
flags: object
value: 36
18 IN_FENCE_FD:
flags: signed range
values: -1 2147483647
value: -1
20 CRTC_ID:
flags: object
value: 32
13 CRTC_X:
flags: signed range
values: -2147483648 2147483647
value: 0
14 CRTC_Y:
flags: signed range
values: -2147483648 2147483647
value: 0
15 CRTC_W:
flags: range
values: 0 2147483647
value: 1280
16 CRTC_H:
flags: range
values: 0 2147483647
value: 1024
9 SRC_X:
flags: range
values: 0 4294967295
value: 0
10 SRC_Y:
flags: range
values: 0 4294967295
value: 0
11 SRC_W:
flags: range
values: 0 4294967295
value: 83886080
12 SRC_H:
flags: range
values: 0 4294967295
value: 67108864
33 0 0 0,0 0,0 0 0x00000001
formats: C8 XR15 RG16 RG24 XR24 XR30
props:
8 type:
flags: immutable enum
enums: Overlay=0 Primary=1 Cursor=2
value: 0
17 FB_ID:
flags: object
value: 0
18 IN_FENCE_FD:
flags: signed range
values: -1 2147483647
value: -1
20 CRTC_ID:
flags: object
value: 0
13 CRTC_X:
flags: signed range
values: -2147483648 2147483647
value: 0
14 CRTC_Y:
flags: signed range
values: -2147483648 2147483647
value: 0
15 CRTC_W:
flags: range
values: 0 2147483647
value: 0
16 CRTC_H:
flags: range
values: 0 2147483647
value: 0
9 SRC_X:
flags: range
values: 0 4294967295
value: 0
10 SRC_Y:
flags: range
values: 0 4294967295
value: 0
11 SRC_W:
flags: range
values: 0 4294967295
value: 0
12 SRC_H:
flags: range
values: 0 4294967295
value: 0
Frame buffers:
id size pitch
Just in case that means anything to anyone.
Paul
Hi Paul,
[...]
> Encoders:
> id crtc type possible crtcs possible clones
> 34 32 DPI 0x00000001 0x00000001
>
> Connectors:
> id encoder status name size (mm) modes
> encoders
> 35 34 connected HDMI-A-1 340x270 17
> 34
> modes:
> index name refresh (Hz) hdisp hss hse htot vdisp vss vse vtot)
> #0 1280x1024 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000
> flags:
> phsync, pvsync; type: preferred, driver
> #1 1280x1024 75.02 1280 1296 1440 1688 1024 1025 1028 1066 135000
> flags:
> phsync, pvsync; type: driver
> #2 1280x960 60.00 1280 1376 1488 1800 960 961 964 1000 108000
> flags: phsync,
> pvsync; type: driver
> #3 1152x864 75.00 1152 1216 1344 1600 864 865 868 900 108000 flags:
> phsync,
> pvsync; type: driver
> #4 1024x768 75.03 1024 1040 1136 1312 768 769 772 800 78750 flags:
> phsync,
> pvsync; type: driver
> #5 1024x768 70.07 1024 1048 1184 1328 768 771 777 806 75000 flags:
> nhsync,
> nvsync; type: driver
> #6 1024x768 60.00 1024 1048 1184 1344 768 771 777 806 65000 flags:
> nhsync,
> nvsync; type: driver
> #7 832x624 74.55 832 864 928 1152 624 625 628 667 57284 flags:
> nhsync,
> nvsync; type: driver
> #8 800x600 75.00 800 816 896 1056 600 601 604 625 49500 flags:
> phsync,
> pvsync; type: driver
> #9 800x600 72.19 800 856 976 1040 600 637 643 666 50000 flags:
> phsync,
> pvsync; type: driver
> #10 800x600 60.32 800 840 968 1056 600 601 605 628 40000 flags:
> phsync,
> pvsync; type: driver
> #11 800x600 56.25 800 824 896 1024 600 601 603 625 36000 flags:
> phsync,
> pvsync; type: driver
> #12 640x480 75.00 640 656 720 840 480 481 484 500 31500 flags:
> nhsync,
> nvsync; type: driver
> #13 640x480 72.81 640 664 704 832 480 489 492 520 31500 flags:
> nhsync,
> nvsync; type: driver
> #14 640x480 66.67 640 704 768 864 480 483 486 525 30240 flags:
> nhsync,
> nvsync; type: driver
> #15 640x480 59.94 640 656 752 800 480 490 492 525 25175 flags:
> nhsync,
> nvsync; type: driver
> #16 720x400 70.08 720 738 846 900 400 412 414 449 28320 flags:
> nhsync,
> pvsync; type: driver
> props:
> 1 EDID:
> flags: immutable blob
> blobs:
>
> value:
> 00ffffffffffff00047232ad01010101
> 2d0e010380221b782aaea5a6544c9926
> 145054bfef0081808140714f01010101
> 010101010101302a009851002a403070
> 1300520e1100001e000000ff00343435
> 3030353444454330300a000000fc0041
> 4c313731350a202020202020000000fd
> 00384c1e520e000a2020202020200051
> 2 DPMS:
> flags: enum
> enums: On=0 Standby=1 Suspend=2 Off=3
> value: 3
> 5 link-status:
> flags: enum
> enums: Good=0 Bad=1
> value: 0
> 6 non-desktop:
> flags: immutable range
> values: 0 1
> value: 0
> 4 TILE:
> flags: immutable blob
> blobs:
>
> value:
> 20 CRTC_ID:
> flags: object
> value: 32
>
> CRTCs:
> id fb pos size
> 32 36 (0,0) (1280x1024)
> #0 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 flags:
> phsync,
> pvsync; type:
> props:
> 22 ACTIVE:
> flags: range
> values: 0 1
> value: 0
> 23 MODE_ID:
> flags: blob
> blobs:
>
> value:
> e0a5010000053005a005980600000004
> 010404042a0400003c00000005000000
> 00000000000000000000000000000000
> 00000000000000000000000000000000
> 00000000
> 19 OUT_FENCE_PTR:
> flags: range
> values: 0 18446744073709551615
> value: 0
> 24 VRR_ENABLED:
> flags: range
> values: 0 1
> value: 0
> 28 GAMMA_LUT:
> flags: blob
> blobs:
>
> value:
> 29 GAMMA_LUT_SIZE:
> flags: immutable range
> values: 0 4294967295
> value: 256
>
> Planes:
> id crtc fb CRTC x,y x,y gamma size
> possible crtcs
> 31 32 36 0,0 0,0 0
> 0x00000001
> formats: XR15 RG16 RG24 XR24 XR30
> props:
> 8 type:
> flags: immutable enum
> enums: Overlay=0 Primary=1 Cursor=2
> value: 1
> 17 FB_ID:
> flags: object
> value: 36
> 18 IN_FENCE_FD:
> flags: signed range
> values: -1 2147483647
> value: -1
> 20 CRTC_ID:
> flags: object
> value: 32
> 13 CRTC_X:
> flags: signed range
> values: -2147483648 2147483647
> value: 0
> 14 CRTC_Y:
> flags: signed range
> values: -2147483648 2147483647
> value: 0
> 15 CRTC_W:
> flags: range
> values: 0 2147483647
> value: 1280
> 16 CRTC_H:
> flags: range
> values: 0 2147483647
> value: 1024
> 9 SRC_X:
> flags: range
> values: 0 4294967295
> value: 0
> 10 SRC_Y:
> flags: range
> values: 0 4294967295
> value: 0
> 11 SRC_W:
> flags: range
> values: 0 4294967295
> value: 83886080
> 12 SRC_H:
> flags: range
> values: 0 4294967295
> value: 67108864
> 33 0 0 0,0 0,0 0
> 0x00000001
> formats: C8 XR15 RG16 RG24 XR24 XR30
> props:
> 8 type:
> flags: immutable enum
> enums: Overlay=0 Primary=1 Cursor=2
> value: 0
> 17 FB_ID:
> flags: object
> value: 0
> 18 IN_FENCE_FD:
> flags: signed range
> values: -1 2147483647
> value: -1
> 20 CRTC_ID:
> flags: object
> value: 0
> 13 CRTC_X:
> flags: signed range
> values: -2147483648 2147483647
> value: 0
> 14 CRTC_Y:
> flags: signed range
> values: -2147483648 2147483647
> value: 0
> 15 CRTC_W:
> flags: range
> values: 0 2147483647
> value: 0
> 16 CRTC_H:
> flags: range
> values: 0 2147483647
> value: 0
> 9 SRC_X:
> flags: range
> values: 0 4294967295
> value: 0
> 10 SRC_Y:
> flags: range
> values: 0 4294967295
> value: 0
> 11 SRC_W:
> flags: range
> values: 0 4294967295
> value: 0
> 12 SRC_H:
> flags: range
> values: 0 4294967295
> value: 0
>
> Frame buffers:
> id size pitch
>
> Just in case that means anything to anyone.
Everything looks good to me. Maybe add some debug in ingenic-drm to see
what bus flags and format it ends up choosing.
Cheers,
-Paul