[PATCH 00/11] ingenic-drm cleanups and doublescan feature

[PATCH 00/11] ingenic-drm cleanups and doublescan feature

[Paul Cercueil]

Hi,

Here is a set of 11 patches for the ingenic-drm driver.

Patches 1-7 are mostly generic cleanups, which will grease up the way
for bigger changes to be introduced.

Patch 3 adds support for a private state structure, which is then used
to store state-specific information, which was previously stored in the
driver's private structure directly.

Patch 10 is the big one; it adds a double-scan feature emulated with DMA
descriptors. This trick makes it possible to support a handful of boards
which have strange panels with non-square pixels (320x480 4:3).

Patch 11 updates the driver to support one top-level bridge per encoder,
as it seems to be the norm now.

Cheers,
-Paul

Paul Cercueil (11):
  drm/ingenic: Remove dead code
  drm/ingenic: Simplify code by using hwdescs array
  drm/ingenic: Add support for private objects
  drm/ingenic: Move no_vblank to private state
  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: Support custom GEM object
  drm/ingenic: Add ingenic_drm_gem_fb_destroy() function
  drm/ingenic: Add doublescan feature
  drm/ingenic: Attach bridge chain to encoders

 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 414 ++++++++++++++++++----
 drivers/gpu/drm/ingenic/ingenic-ipu.c     | 127 ++++++-
 2 files changed, 458 insertions(+), 83 deletions(-)

-- 
2.30.2

[PATCH 01/11] drm/ingenic: Remove dead code

[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 <paul@crapouillou.net>
---
 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 5244f4763477..93c099e7464d 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -988,9 +988,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

[PATCH 02/11] drm/ingenic: Simplify code by using hwdescs array

[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 <paul@crapouillou.net>
---
 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 93c099e7464d..4e41bdf2f3fd 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -50,8 +50,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);
 };
@@ -142,6 +141,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)
@@ -563,6 +569,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) {
@@ -570,16 +577,14 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
 			drm_fb_cma_sync_non_coherent(&priv->drm, oldstate, newstate);
 
 		crtc_state = newstate->crtc->state;
+		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);
@@ -592,9 +597,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;
 		}
@@ -968,20 +973,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

[PATCH 03/11] drm/ingenic: Add support for private objects

[Paul Cercueil]

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

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

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

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 4e41bdf2f3fd..e81084eb3b0e 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -64,6 +64,10 @@ struct jz_soc_info {
 	unsigned int num_formats_f0, num_formats_f1;
 };
 
+struct ingenic_drm_private_state {
+	struct drm_private_state base;
+};
+
 struct ingenic_drm {
 	struct drm_device drm;
 	/*
@@ -99,8 +103,16 @@ struct ingenic_drm {
 	struct mutex clk_mutex;
 	bool update_clk_rate;
 	struct notifier_block clock_nb;
+
+	struct drm_private_obj private_obj;
 };
 
+static inline struct ingenic_drm_private_state *
+to_ingenic_drm_priv_state(struct drm_private_state *state)
+{
+	return container_of(state, struct ingenic_drm_private_state, base);
+}
+
 static bool ingenic_drm_writeable_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
@@ -790,6 +802,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 = {
@@ -863,6 +897,11 @@ static struct drm_mode_config_helper_funcs ingenic_drm_mode_config_helpers = {
 	.atomic_commit_tail = ingenic_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;
@@ -875,9 +914,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;
@@ -1158,6 +1203,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");
@@ -1168,6 +1227,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 61b6d9fdbba1..007cd547b285 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.
@@ -680,6 +692,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;
@@ -718,6 +757,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;
@@ -811,7 +851,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,
@@ -819,6 +872,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

[PATCH 04/11] drm/ingenic: Move no_vblank to private state

[Paul Cercueil]

This information is carried from the ".atomic_check" to the
".atomic_commit_tail"; as such it is state-specific, and should be moved
to the private state structure.

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

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index e81084eb3b0e..639994329c60 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -66,6 +66,8 @@ struct jz_soc_info {
 
 struct ingenic_drm_private_state {
 	struct drm_private_state base;
+
+	bool no_vblank;
 };
 
 struct ingenic_drm {
@@ -87,7 +89,6 @@ struct ingenic_drm {
 	dma_addr_t dma_hwdescs_phys;
 
 	bool panel_is_sharp;
-	bool no_vblank;
 
 	/*
 	 * clk_mutex is used to synchronize the pixel clock rate update with
@@ -113,6 +114,30 @@ to_ingenic_drm_priv_state(struct drm_private_state *state)
 	return container_of(state, struct ingenic_drm_private_state, base);
 }
 
+static struct ingenic_drm_private_state *
+ingenic_drm_get_priv_state(struct ingenic_drm *priv, struct drm_atomic_state *state)
+{
+	struct drm_private_state *priv_state;
+
+	priv_state = drm_atomic_get_private_obj_state(state, &priv->private_obj);
+	if (IS_ERR(priv_state))
+		return ERR_CAST(priv_state);
+
+	return to_ingenic_drm_priv_state(priv_state);
+}
+
+static struct ingenic_drm_private_state *
+ingenic_drm_get_new_priv_state(struct ingenic_drm *priv, struct drm_atomic_state *state)
+{
+	struct drm_private_state *priv_state;
+
+	priv_state = drm_atomic_get_new_private_obj_state(state, &priv->private_obj);
+	if (!priv_state)
+		return NULL;
+
+	return to_ingenic_drm_priv_state(priv_state);
+}
+
 static bool ingenic_drm_writeable_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
@@ -268,6 +293,7 @@ static int ingenic_drm_crtc_atomic_check(struct drm_crtc *crtc,
 									  crtc);
 	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
 	struct drm_plane_state *f1_state, *f0_state, *ipu_state = NULL;
+	struct ingenic_drm_private_state *priv_state;
 
 	if (crtc_state->gamma_lut &&
 	    drm_color_lut_size(crtc_state->gamma_lut) != ARRAY_SIZE(priv->dma_hwdescs->palette)) {
@@ -299,9 +325,13 @@ static int ingenic_drm_crtc_atomic_check(struct drm_crtc *crtc,
 			}
 		}
 
+		priv_state = ingenic_drm_get_priv_state(priv, state);
+		if (IS_ERR(priv_state))
+			return PTR_ERR(priv_state);
+
 		/* If all the planes are disabled, we won't get a VBLANK IRQ */
-		priv->no_vblank = !f1_state->fb && !f0_state->fb &&
-				  !(ipu_state && ipu_state->fb);
+		priv_state->no_vblank = !f1_state->fb && !f0_state->fb &&
+					!(ipu_state && ipu_state->fb);
 	}
 
 	return 0;
@@ -727,6 +757,7 @@ static void ingenic_drm_atomic_helper_commit_tail(struct drm_atomic_state *old_s
 	 */
 	struct drm_device *dev = old_state->dev;
 	struct ingenic_drm *priv = drm_device_get_priv(dev);
+	struct ingenic_drm_private_state *priv_state;
 
 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
 
@@ -736,7 +767,9 @@ static void ingenic_drm_atomic_helper_commit_tail(struct drm_atomic_state *old_s
 
 	drm_atomic_helper_commit_hw_done(old_state);
 
-	if (!priv->no_vblank)
+	priv_state = ingenic_drm_get_new_priv_state(priv, old_state);
+
+	if (!priv_state || !priv_state->no_vblank)
 		drm_atomic_helper_wait_for_vblanks(dev, old_state);
 
 	drm_atomic_helper_cleanup_planes(dev, old_state);
-- 
2.30.2

Re: [PATCH 04/11] drm/ingenic: Move no_vblank to private state

[Daniel Vetter]

On Fri, May 28, 2021 at 12:20:58AM +0100, Paul Cercueil wrote:
> This information is carried from the ".atomic_check" to the
> ".atomic_commit_tail"; as such it is state-specific, and should be moved
> to the private state structure.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Not sure this applies to your hw, but drm_crtc_state.no_vblank exists.
Might want to move to that instead of rolling your own. Or explain why you
need your own here in your own private state. It does look quite a bit
like you're just rolling your own version of this support that helpers
gained meanwhile.
-Daniel


> ---
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 41 ++++++++++++++++++++---
>  1 file changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index e81084eb3b0e..639994329c60 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -66,6 +66,8 @@ struct jz_soc_info {
>  
>  struct ingenic_drm_private_state {
>  	struct drm_private_state base;
> +
> +	bool no_vblank;
>  };
>  
>  struct ingenic_drm {
> @@ -87,7 +89,6 @@ struct ingenic_drm {
>  	dma_addr_t dma_hwdescs_phys;
>  
>  	bool panel_is_sharp;
> -	bool no_vblank;
>  
>  	/*
>  	 * clk_mutex is used to synchronize the pixel clock rate update with
> @@ -113,6 +114,30 @@ to_ingenic_drm_priv_state(struct drm_private_state *state)
>  	return container_of(state, struct ingenic_drm_private_state, base);
>  }
>  
> +static struct ingenic_drm_private_state *
> +ingenic_drm_get_priv_state(struct ingenic_drm *priv, struct drm_atomic_state *state)
> +{
> +	struct drm_private_state *priv_state;
> +
> +	priv_state = drm_atomic_get_private_obj_state(state, &priv->private_obj);
> +	if (IS_ERR(priv_state))
> +		return ERR_CAST(priv_state);
> +
> +	return to_ingenic_drm_priv_state(priv_state);
> +}
> +
> +static struct ingenic_drm_private_state *
> +ingenic_drm_get_new_priv_state(struct ingenic_drm *priv, struct drm_atomic_state *state)
> +{
> +	struct drm_private_state *priv_state;
> +
> +	priv_state = drm_atomic_get_new_private_obj_state(state, &priv->private_obj);
> +	if (!priv_state)
> +		return NULL;
> +
> +	return to_ingenic_drm_priv_state(priv_state);
> +}
> +
>  static bool ingenic_drm_writeable_reg(struct device *dev, unsigned int reg)
>  {
>  	switch (reg) {
> @@ -268,6 +293,7 @@ static int ingenic_drm_crtc_atomic_check(struct drm_crtc *crtc,
>  									  crtc);
>  	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>  	struct drm_plane_state *f1_state, *f0_state, *ipu_state = NULL;
> +	struct ingenic_drm_private_state *priv_state;
>  
>  	if (crtc_state->gamma_lut &&
>  	    drm_color_lut_size(crtc_state->gamma_lut) != ARRAY_SIZE(priv->dma_hwdescs->palette)) {
> @@ -299,9 +325,13 @@ static int ingenic_drm_crtc_atomic_check(struct drm_crtc *crtc,
>  			}
>  		}
>  
> +		priv_state = ingenic_drm_get_priv_state(priv, state);
> +		if (IS_ERR(priv_state))
> +			return PTR_ERR(priv_state);
> +
>  		/* If all the planes are disabled, we won't get a VBLANK IRQ */
> -		priv->no_vblank = !f1_state->fb && !f0_state->fb &&
> -				  !(ipu_state && ipu_state->fb);
> +		priv_state->no_vblank = !f1_state->fb && !f0_state->fb &&
> +					!(ipu_state && ipu_state->fb);
>  	}
>  
>  	return 0;
> @@ -727,6 +757,7 @@ static void ingenic_drm_atomic_helper_commit_tail(struct drm_atomic_state *old_s
>  	 */
>  	struct drm_device *dev = old_state->dev;
>  	struct ingenic_drm *priv = drm_device_get_priv(dev);
> +	struct ingenic_drm_private_state *priv_state;
>  
>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
>  
> @@ -736,7 +767,9 @@ static void ingenic_drm_atomic_helper_commit_tail(struct drm_atomic_state *old_s
>  
>  	drm_atomic_helper_commit_hw_done(old_state);
>  
> -	if (!priv->no_vblank)
> +	priv_state = ingenic_drm_get_new_priv_state(priv, old_state);
> +
> +	if (!priv_state || !priv_state->no_vblank)
>  		drm_atomic_helper_wait_for_vblanks(dev, old_state);
>  
>  	drm_atomic_helper_cleanup_planes(dev, old_state);
> -- 
> 2.30.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 04/11] drm/ingenic: Move no_vblank to private state

[Paul Cercueil]

Hi Daniel,

Le mar., juin 1 2021 at 17:48:12 +0200, Daniel Vetter <daniel@ffwll.ch> 
a écrit :
> On Fri, May 28, 2021 at 12:20:58AM +0100, Paul Cercueil wrote:
>>  This information is carried from the ".atomic_check" to the
>>  ".atomic_commit_tail"; as such it is state-specific, and should be 
>> moved
>>  to the private state structure.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> 
> Not sure this applies to your hw, but drm_crtc_state.no_vblank exists.
> Might want to move to that instead of rolling your own. Or explain 
> why you
> need your own here in your own private state. It does look quite a bit
> like you're just rolling your own version of this support that helpers
> gained meanwhile.

If I use drm_crtc_state->no_vblank, then I need a custom 
.atomic_commit_tail() that only calls 
drm_atomic_helper_wait_for_vblanks() when !no_vblank. That works, but I 
don't understand why drm_atomic_helper_commit_tail() doesn't do that by 
default, and makes me think I'm using it wrong.

Cheers,
-Paul

> -Daniel
> 
> 
>>  ---
>>   drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 41 
>> ++++++++++++++++++++---
>>   1 file changed, 37 insertions(+), 4 deletions(-)
>> 
>>  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c 
>> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  index e81084eb3b0e..639994329c60 100644
>>  --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  @@ -66,6 +66,8 @@ struct jz_soc_info {
>> 
>>   struct ingenic_drm_private_state {
>>   	struct drm_private_state base;
>>  +
>>  +	bool no_vblank;
>>   };
>> 
>>   struct ingenic_drm {
>>  @@ -87,7 +89,6 @@ struct ingenic_drm {
>>   	dma_addr_t dma_hwdescs_phys;
>> 
>>   	bool panel_is_sharp;
>>  -	bool no_vblank;
>> 
>>   	/*
>>   	 * clk_mutex is used to synchronize the pixel clock rate update 
>> with
>>  @@ -113,6 +114,30 @@ to_ingenic_drm_priv_state(struct 
>> drm_private_state *state)
>>   	return container_of(state, struct ingenic_drm_private_state, 
>> base);
>>   }
>> 
>>  +static struct ingenic_drm_private_state *
>>  +ingenic_drm_get_priv_state(struct ingenic_drm *priv, struct 
>> drm_atomic_state *state)
>>  +{
>>  +	struct drm_private_state *priv_state;
>>  +
>>  +	priv_state = drm_atomic_get_private_obj_state(state, 
>> &priv->private_obj);
>>  +	if (IS_ERR(priv_state))
>>  +		return ERR_CAST(priv_state);
>>  +
>>  +	return to_ingenic_drm_priv_state(priv_state);
>>  +}
>>  +
>>  +static struct ingenic_drm_private_state *
>>  +ingenic_drm_get_new_priv_state(struct ingenic_drm *priv, struct 
>> drm_atomic_state *state)
>>  +{
>>  +	struct drm_private_state *priv_state;
>>  +
>>  +	priv_state = drm_atomic_get_new_private_obj_state(state, 
>> &priv->private_obj);
>>  +	if (!priv_state)
>>  +		return NULL;
>>  +
>>  +	return to_ingenic_drm_priv_state(priv_state);
>>  +}
>>  +
>>   static bool ingenic_drm_writeable_reg(struct device *dev, unsigned 
>> int reg)
>>   {
>>   	switch (reg) {
>>  @@ -268,6 +293,7 @@ static int ingenic_drm_crtc_atomic_check(struct 
>> drm_crtc *crtc,
>>   									  crtc);
>>   	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>>   	struct drm_plane_state *f1_state, *f0_state, *ipu_state = NULL;
>>  +	struct ingenic_drm_private_state *priv_state;
>> 
>>   	if (crtc_state->gamma_lut &&
>>   	    drm_color_lut_size(crtc_state->gamma_lut) != 
>> ARRAY_SIZE(priv->dma_hwdescs->palette)) {
>>  @@ -299,9 +325,13 @@ static int 
>> ingenic_drm_crtc_atomic_check(struct drm_crtc *crtc,
>>   			}
>>   		}
>> 
>>  +		priv_state = ingenic_drm_get_priv_state(priv, state);
>>  +		if (IS_ERR(priv_state))
>>  +			return PTR_ERR(priv_state);
>>  +
>>   		/* If all the planes are disabled, we won't get a VBLANK IRQ */
>>  -		priv->no_vblank = !f1_state->fb && !f0_state->fb &&
>>  -				  !(ipu_state && ipu_state->fb);
>>  +		priv_state->no_vblank = !f1_state->fb && !f0_state->fb &&
>>  +					!(ipu_state && ipu_state->fb);
>>   	}
>> 
>>   	return 0;
>>  @@ -727,6 +757,7 @@ static void 
>> ingenic_drm_atomic_helper_commit_tail(struct drm_atomic_state *old_s
>>   	 */
>>   	struct drm_device *dev = old_state->dev;
>>   	struct ingenic_drm *priv = drm_device_get_priv(dev);
>>  +	struct ingenic_drm_private_state *priv_state;
>> 
>>   	drm_atomic_helper_commit_modeset_disables(dev, old_state);
>> 
>>  @@ -736,7 +767,9 @@ static void 
>> ingenic_drm_atomic_helper_commit_tail(struct drm_atomic_state *old_s
>> 
>>   	drm_atomic_helper_commit_hw_done(old_state);
>> 
>>  -	if (!priv->no_vblank)
>>  +	priv_state = ingenic_drm_get_new_priv_state(priv, old_state);
>>  +
>>  +	if (!priv_state || !priv_state->no_vblank)
>>   		drm_atomic_helper_wait_for_vblanks(dev, old_state);
>> 
>>   	drm_atomic_helper_cleanup_planes(dev, old_state);
>>  --
>>  2.30.2
>> 
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH 04/11] drm/ingenic: Move no_vblank to private state

[Daniel Vetter]

On Thu, Jun 10, 2021 at 04:09:19PM +0100, Paul Cercueil wrote:
> Hi Daniel,
> 
> Le mar., juin 1 2021 at 17:48:12 +0200, Daniel Vetter <daniel@ffwll.ch> a
> écrit :
> > On Fri, May 28, 2021 at 12:20:58AM +0100, Paul Cercueil wrote:
> > >  This information is carried from the ".atomic_check" to the
> > >  ".atomic_commit_tail"; as such it is state-specific, and should be
> > > moved
> > >  to the private state structure.
> > > 
> > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > 
> > Not sure this applies to your hw, but drm_crtc_state.no_vblank exists.
> > Might want to move to that instead of rolling your own. Or explain why
> > you
> > need your own here in your own private state. It does look quite a bit
> > like you're just rolling your own version of this support that helpers
> > gained meanwhile.
> 
> If I use drm_crtc_state->no_vblank, then I need a custom
> .atomic_commit_tail() that only calls drm_atomic_helper_wait_for_vblanks()
> when !no_vblank. That works, but I don't understand why
> drm_atomic_helper_commit_tail() doesn't do that by default, and makes me
> think I'm using it wrong.

So the recommendation is to have your own commit_tail and use
drm_atomic_helper_wait_for_flip_done(). But also if wait_for_vblanks dies
on you, there's a driver bug: If vblanks arent available, then the
drm_crtc_vblank_get should fail. If that's not the case then I guess some
bigger issues to be fixed because userspace might also do a vblank wait
(for timing the next frame), so that really needs to work correctly.

That's kinda why I put that wait_for_vblank in there by default, it forces
drivers to be correct.

If you're wondering how that's done: This is why the driver
->enable_vblank callback can return an error code. So maybe the real fix
here is in there, and everything else can stay as-is?

Another thing is that if you call drm_crtc_vblank_on/off correctly, this
should also work out correctly - attempted vblank waits outside of when
the vblank is running should fail.

Maybe something fell off here in this area because it's tricky, but the
infrastructure should be here already.
-Daniel

> 
> Cheers,
> -Paul
> 
> > -Daniel
> > 
> > 
> > >  ---
> > >   drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 41
> > > ++++++++++++++++++++---
> > >   1 file changed, 37 insertions(+), 4 deletions(-)
> > > 
> > >  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> > > b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> > >  index e81084eb3b0e..639994329c60 100644
> > >  --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> > >  +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> > >  @@ -66,6 +66,8 @@ struct jz_soc_info {
> > > 
> > >   struct ingenic_drm_private_state {
> > >   	struct drm_private_state base;
> > >  +
> > >  +	bool no_vblank;
> > >   };
> > > 
> > >   struct ingenic_drm {
> > >  @@ -87,7 +89,6 @@ struct ingenic_drm {
> > >   	dma_addr_t dma_hwdescs_phys;
> > > 
> > >   	bool panel_is_sharp;
> > >  -	bool no_vblank;
> > > 
> > >   	/*
> > >   	 * clk_mutex is used to synchronize the pixel clock rate update
> > > with
> > >  @@ -113,6 +114,30 @@ to_ingenic_drm_priv_state(struct
> > > drm_private_state *state)
> > >   	return container_of(state, struct ingenic_drm_private_state,
> > > base);
> > >   }
> > > 
> > >  +static struct ingenic_drm_private_state *
> > >  +ingenic_drm_get_priv_state(struct ingenic_drm *priv, struct
> > > drm_atomic_state *state)
> > >  +{
> > >  +	struct drm_private_state *priv_state;
> > >  +
> > >  +	priv_state = drm_atomic_get_private_obj_state(state,
> > > &priv->private_obj);
> > >  +	if (IS_ERR(priv_state))
> > >  +		return ERR_CAST(priv_state);
> > >  +
> > >  +	return to_ingenic_drm_priv_state(priv_state);
> > >  +}
> > >  +
> > >  +static struct ingenic_drm_private_state *
> > >  +ingenic_drm_get_new_priv_state(struct ingenic_drm *priv, struct
> > > drm_atomic_state *state)
> > >  +{
> > >  +	struct drm_private_state *priv_state;
> > >  +
> > >  +	priv_state = drm_atomic_get_new_private_obj_state(state,
> > > &priv->private_obj);
> > >  +	if (!priv_state)
> > >  +		return NULL;
> > >  +
> > >  +	return to_ingenic_drm_priv_state(priv_state);
> > >  +}
> > >  +
> > >   static bool ingenic_drm_writeable_reg(struct device *dev, unsigned
> > > int reg)
> > >   {
> > >   	switch (reg) {
> > >  @@ -268,6 +293,7 @@ static int ingenic_drm_crtc_atomic_check(struct
> > > drm_crtc *crtc,
> > >   									  crtc);
> > >   	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
> > >   	struct drm_plane_state *f1_state, *f0_state, *ipu_state = NULL;
> > >  +	struct ingenic_drm_private_state *priv_state;
> > > 
> > >   	if (crtc_state->gamma_lut &&
> > >   	    drm_color_lut_size(crtc_state->gamma_lut) !=
> > > ARRAY_SIZE(priv->dma_hwdescs->palette)) {
> > >  @@ -299,9 +325,13 @@ static int
> > > ingenic_drm_crtc_atomic_check(struct drm_crtc *crtc,
> > >   			}
> > >   		}
> > > 
> > >  +		priv_state = ingenic_drm_get_priv_state(priv, state);
> > >  +		if (IS_ERR(priv_state))
> > >  +			return PTR_ERR(priv_state);
> > >  +
> > >   		/* If all the planes are disabled, we won't get a VBLANK IRQ */
> > >  -		priv->no_vblank = !f1_state->fb && !f0_state->fb &&
> > >  -				  !(ipu_state && ipu_state->fb);
> > >  +		priv_state->no_vblank = !f1_state->fb && !f0_state->fb &&
> > >  +					!(ipu_state && ipu_state->fb);
> > >   	}
> > > 
> > >   	return 0;
> > >  @@ -727,6 +757,7 @@ static void
> > > ingenic_drm_atomic_helper_commit_tail(struct drm_atomic_state *old_s
> > >   	 */
> > >   	struct drm_device *dev = old_state->dev;
> > >   	struct ingenic_drm *priv = drm_device_get_priv(dev);
> > >  +	struct ingenic_drm_private_state *priv_state;
> > > 
> > >   	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> > > 
> > >  @@ -736,7 +767,9 @@ static void
> > > ingenic_drm_atomic_helper_commit_tail(struct drm_atomic_state *old_s
> > > 
> > >   	drm_atomic_helper_commit_hw_done(old_state);
> > > 
> > >  -	if (!priv->no_vblank)
> > >  +	priv_state = ingenic_drm_get_new_priv_state(priv, old_state);
> > >  +
> > >  +	if (!priv_state || !priv_state->no_vblank)
> > >   		drm_atomic_helper_wait_for_vblanks(dev, old_state);
> > > 
> > >   	drm_atomic_helper_cleanup_planes(dev, old_state);
> > >  --
> > >  2.30.2
> > > 
> > 
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 05/11] drm/ingenic: Move IPU scale settings to private state

[Paul Cercueil]

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

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

diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c b/drivers/gpu/drm/ingenic/ingenic-ipu.c
index 007cd547b285..b85d9a7f53d3 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

[PATCH 06/11] drm/ingenic: Set DMA descriptor chain register when starting CRTC

[Paul Cercueil]

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

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

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

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 639994329c60..5ba3283da97d 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -210,6 +210,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);
@@ -1218,10 +1222,6 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 		}
 	}
 
-	/* Set address of our DMA descriptor chain */
-	regmap_write(priv->map, JZ_REG_LCD_DA0, dma_hwdesc_phys_f0);
-	regmap_write(priv->map, JZ_REG_LCD_DA1, dma_hwdesc_phys_f1);
-
 	/* Enable OSD if available */
 	if (soc_info->has_osd)
 		regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
-- 
2.30.2

[PATCH 07/11] drm/ingenic: Upload palette before frame

[Paul Cercueil]

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

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

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

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 5ba3283da97d..ced2109e8f35 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -68,6 +68,7 @@ struct ingenic_drm_private_state {
 	struct drm_private_state base;
 
 	bool no_vblank;
+	bool use_palette;
 };
 
 struct ingenic_drm {
@@ -185,6 +186,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)
@@ -207,11 +215,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,
@@ -422,6 +438,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;
@@ -434,6 +451,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,
@@ -452,6 +473,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.
@@ -611,10 +635,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;
 
@@ -630,23 +655,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

[PATCH 08/11] drm/ingenic: Support custom GEM object

[Paul Cercueil]

Add boilerplate code to support a custom "ingenic_gem_object". Empty for
now, but it will be useful later when subsequent patches will introduce
object-specific driver data.

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

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index ced2109e8f35..1cac369f6293 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -64,6 +64,10 @@ struct jz_soc_info {
 	unsigned int num_formats_f0, num_formats_f1;
 };
 
+struct ingenic_gem_object {
+	struct drm_gem_cma_object base;
+};
+
 struct ingenic_drm_private_state {
 	struct drm_private_state base;
 
@@ -179,6 +183,11 @@ static inline struct ingenic_drm *drm_nb_get_priv(struct notifier_block *nb)
 	return container_of(nb, struct ingenic_drm, clock_nb);
 }
 
+static inline struct ingenic_gem_object *to_ingenic_gem_obj(struct drm_gem_object *gem_obj)
+{
+	return container_of(gem_obj, struct ingenic_gem_object, base.base);
+}
+
 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]);
@@ -853,15 +862,15 @@ static struct drm_gem_object *
 ingenic_drm_gem_create_object(struct drm_device *drm, size_t size)
 {
 	struct ingenic_drm *priv = drm_device_get_priv(drm);
-	struct drm_gem_cma_object *obj;
+	struct ingenic_gem_object *obj;
 
 	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
 	if (!obj)
 		return ERR_PTR(-ENOMEM);
 
-	obj->map_noncoherent = priv->soc_info->map_noncoherent;
+	obj->base.map_noncoherent = priv->soc_info->map_noncoherent;
 
-	return &obj->base;
+	return &obj->base.base;
 }
 
 static struct drm_private_state *
-- 
2.30.2

[PATCH 09/11] drm/ingenic: Add ingenic_drm_gem_fb_destroy() function

[Paul Cercueil]

Add a ingenic_drm_gem_fb_destroy() function, which currently only calls
gem_fb_destroy(), but will be extended in a subsequent patch.

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

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 1cac369f6293..2761478b16e8 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -846,16 +846,38 @@ static void ingenic_drm_disable_vblank(struct drm_crtc *crtc)
 	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL, JZ_LCD_CTRL_EOF_IRQ, 0);
 }
 
+static void ingenic_drm_gem_fb_destroy(struct drm_framebuffer *fb)
+{
+	drm_gem_fb_destroy(fb);
+}
+
+static const struct drm_framebuffer_funcs ingenic_drm_gem_fb_funcs = {
+	.destroy	= ingenic_drm_gem_fb_destroy,
+	.create_handle	= drm_gem_fb_create_handle,
+};
+
+static const struct drm_framebuffer_funcs ingenic_drm_gem_fb_funcs_dirty = {
+	.destroy	= ingenic_drm_gem_fb_destroy,
+	.create_handle	= drm_gem_fb_create_handle,
+	.dirty		= drm_atomic_helper_dirtyfb,
+};
+
 static struct drm_framebuffer *
 ingenic_drm_gem_fb_create(struct drm_device *drm, struct drm_file *file,
 			  const struct drm_mode_fb_cmd2 *mode_cmd)
 {
 	struct ingenic_drm *priv = drm_device_get_priv(drm);
+	const struct drm_framebuffer_funcs *fb_funcs;
+	struct drm_framebuffer *fb;
 
 	if (priv->soc_info->map_noncoherent)
-		return drm_gem_fb_create_with_dirty(drm, file, mode_cmd);
+		fb_funcs = &ingenic_drm_gem_fb_funcs_dirty;
+	else
+		fb_funcs = &ingenic_drm_gem_fb_funcs;
+
+	fb = drm_gem_fb_create_with_funcs(drm, file, mode_cmd, fb_funcs);
 
-	return drm_gem_fb_create(drm, file, mode_cmd);
+	return fb;
 }
 
 static struct drm_gem_object *
-- 
2.30.2

[PATCH 10/11] drm/ingenic: Add doublescan feature

[Paul Cercueil]

A lot of devices with an Ingenic SoC have a weird LCD panel attached,
where the pixels are not square. For instance, the AUO A030JTN01 and
Innolux EJ030NA panels have a resolution of 320x480 with a 4:3 aspect
ratio.

All userspace applications are built with the assumption that the
pixels are square. To be able to support these devices without too
much effort, add a doublescan feature, which allows the f0 and f1
planes to be used with only half of the screen's vertical resolution,
where each line of the input is displayed twice.

This is done using a chained list of DMA descriptors, one descriptor
per output line.

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

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 2761478b16e8..01d8490393d1 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -66,6 +66,8 @@ struct jz_soc_info {
 
 struct ingenic_gem_object {
 	struct drm_gem_cma_object base;
+	struct ingenic_dma_hwdesc *hwdescs;
+	dma_addr_t hwdescs_phys;
 };
 
 struct ingenic_drm_private_state {
@@ -73,6 +75,23 @@ struct ingenic_drm_private_state {
 
 	bool no_vblank;
 	bool use_palette;
+
+	/*
+	 * A lot of devices with an Ingenic SoC have a weird LCD panel attached,
+	 * where the pixels are not square. For instance, the AUO A030JTN01 and
+	 * Innolux EJ030NA panels have a resolution of 320x480 with a 4:3 aspect
+	 * ratio.
+	 *
+	 * All userspace applications are built with the assumption that the
+	 * pixels are square. To be able to support these devices without too
+	 * much effort, add a doublescan feature, which allows the f0 and f1
+	 * planes to be used with only half of the screen's vertical resolution,
+	 * where each line of the input is displayed twice.
+	 *
+	 * This is done using a chained list of DMA descriptors, one descriptor
+	 * per output line.
+	 */
+	bool doublescan;
 };
 
 struct ingenic_drm {
@@ -465,7 +484,7 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
 		return PTR_ERR(priv_state);
 
 	ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
-						  DRM_PLANE_HELPER_NO_SCALING,
+						  0x8000,
 						  DRM_PLANE_HELPER_NO_SCALING,
 						  priv->soc_info->has_osd,
 						  true);
@@ -482,6 +501,17 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
 	     (new_plane_state->src_h >> 16) != new_plane_state->crtc_h))
 		return -EINVAL;
 
+	/* Enable doublescan if the CRTC_H is twice the SRC_H. */
+	priv_state->doublescan = (new_plane_state->src_h >> 16) * 2 == new_plane_state->crtc_h;
+
+	/* Otherwise, fail if CRTC_H != SRC_H */
+	if (!priv_state->doublescan && (new_plane_state->src_h >> 16) != new_plane_state->crtc_h)
+		return -EINVAL;
+
+	/* Fail if CRTC_W != SRC_W */
+	if ((new_plane_state->src_w >> 16) != new_plane_state->crtc_w)
+		return -EINVAL;
+
 	priv_state->use_palette = new_plane_state->fb &&
 		new_plane_state->fb->format->format == DRM_FORMAT_C8;
 
@@ -647,7 +677,9 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
 	struct ingenic_drm_private_state *priv_state;
 	struct drm_crtc_state *crtc_state;
 	struct ingenic_dma_hwdesc *hwdesc;
-	unsigned int width, height, cpp;
+	unsigned int width, height, cpp, i;
+	struct drm_gem_object *gem_obj;
+	struct ingenic_gem_object *obj;
 	dma_addr_t addr, next_addr;
 	bool use_f1;
 	u32 fourcc;
@@ -664,17 +696,39 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
 		height = newstate->src_h >> 16;
 		cpp = newstate->fb->format->cpp[0];
 
+		gem_obj = drm_gem_fb_get_obj(newstate->fb, 0);
+		obj = to_ingenic_gem_obj(gem_obj);
+
 		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];
+		if (priv_state->doublescan) {
+			hwdesc = &obj->hwdescs[0];
+			/*
+			 * Use one DMA descriptor per output line, and display
+			 * each input line twice.
+			 */
+			for (i = 0; i < newstate->crtc_h; i++) {
+				hwdesc[i].next = obj->hwdescs_phys
+					+ (i + 1) * sizeof(*hwdesc);
+				hwdesc[i].addr = addr + (i / 2) * newstate->fb->pitches[0];
+				hwdesc[i].cmd = newstate->fb->pitches[0] / 4;
+			}
 
-		hwdesc->addr = addr;
-		hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
-		hwdesc->next = next_addr;
+			/* We want the EOF IRQ only on the very last transfer */
+			hwdesc[newstate->crtc_h - 1].cmd |= JZ_LCD_CMD_EOF_IRQ;
+			hwdesc[newstate->crtc_h - 1].next = next_addr;
+			priv->dma_hwdescs->hwdesc[use_f1] = *hwdesc;
+		} else {
+			/* Use one DMA descriptor for the whole frame. */
+			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;
@@ -848,6 +902,13 @@ static void ingenic_drm_disable_vblank(struct drm_crtc *crtc)
 
 static void ingenic_drm_gem_fb_destroy(struct drm_framebuffer *fb)
 {
+	struct ingenic_drm *priv = drm_device_get_priv(fb->dev);
+	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
+	struct ingenic_gem_object *obj = to_ingenic_gem_obj(gem_obj);
+
+	dma_free_coherent(priv->dev,
+			  sizeof(*obj->hwdescs) * fb->height,
+			  obj->hwdescs, obj->hwdescs_phys);
 	drm_gem_fb_destroy(fb);
 }
 
@@ -868,6 +929,8 @@ ingenic_drm_gem_fb_create(struct drm_device *drm, struct drm_file *file,
 {
 	struct ingenic_drm *priv = drm_device_get_priv(drm);
 	const struct drm_framebuffer_funcs *fb_funcs;
+	struct drm_gem_object *gem_obj;
+	struct ingenic_gem_object *obj;
 	struct drm_framebuffer *fb;
 
 	if (priv->soc_info->map_noncoherent)
@@ -876,6 +939,24 @@ ingenic_drm_gem_fb_create(struct drm_device *drm, struct drm_file *file,
 		fb_funcs = &ingenic_drm_gem_fb_funcs;
 
 	fb = drm_gem_fb_create_with_funcs(drm, file, mode_cmd, fb_funcs);
+	if (IS_ERR(fb))
+		return fb;
+
+	gem_obj = drm_gem_fb_get_obj(fb, 0);
+	obj = to_ingenic_gem_obj(gem_obj);
+
+	/*
+	 * Create (fb->height * 2) DMA descriptors, in case we want to use the
+	 * doublescan feature.
+	 */
+	obj->hwdescs = dma_alloc_coherent(priv->dev,
+					  sizeof(*obj->hwdescs) * fb->height * 2,
+					  &obj->hwdescs_phys,
+					  GFP_KERNEL);
+	if (!obj->hwdescs) {
+		drm_gem_fb_destroy(fb);
+		return ERR_PTR(-ENOMEM);
+	}
 
 	return fb;
 }
-- 
2.30.2

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

[Paul Cercueil]

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

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

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 01d8490393d1..f0242e917d6e 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>
@@ -132,6 +133,26 @@ struct ingenic_drm {
 	struct drm_private_obj private_obj;
 };
 
+struct ingenic_drm_bridge {
+	struct drm_encoder encoder;
+	struct drm_bridge bridge;
+	struct drm_bridge *next_bridge;
+
+	/*
+	 * FIXME: this should really be in ingenic_drm_private_state, but there
+	 * doesn't seem to be a way to retrieve a pointer to it from within
+	 * ingenic_drm_encoder_atomic_mode_set (no drm_atomic_state
+	 * back-pointers).
+	 */
+	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)
 {
@@ -749,11 +770,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;
@@ -766,19 +786,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;
@@ -804,20 +824,31 @@ 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 drm_encoder *encoder = bridge->encoder;
+	struct ingenic_drm_bridge *ingenic_bridge = to_ingenic_drm_bridge(encoder);
+
+	return drm_bridge_attach(encoder, ingenic_bridge->next_bridge,
+				 &ingenic_bridge->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 drm_encoder *encoder = bridge->encoder;
+	struct ingenic_drm_bridge *ingenic_bridge = to_ingenic_drm_bridge(encoder);
 
-	if (info->num_bus_formats != 1)
-		return -EINVAL;
+	ingenic_bridge->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:
 		/*
@@ -1056,8 +1087,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 = {
@@ -1097,12 +1136,14 @@ 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;
+	struct ingenic_drm_bridge *ingenic_bridge;
 	const struct jz_soc_info *soc_info;
 	struct ingenic_drm *priv;
 	struct clk *parent_clk;
 	struct drm_plane *primary;
 	struct drm_bridge *bridge;
 	struct drm_panel *panel;
+	struct drm_connector *connector;
 	struct drm_encoder *encoder;
 	struct drm_device *drm;
 	void __iomem *base;
@@ -1291,22 +1332,37 @@ 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);
+		ingenic_bridge = drmm_encoder_alloc(drm, struct ingenic_drm_bridge,
+						    encoder, NULL,
+						    DRM_MODE_ENCODER_DPI, NULL);
+		if (IS_ERR(ingenic_bridge)) {
+			ret = PTR_ERR(ingenic_bridge);
 			dev_err(dev, "Failed to init encoder: %d\n", ret);
 			return ret;
 		}
 
-		encoder->possible_crtcs = 1;
+		encoder = &ingenic_bridge->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);
+		ingenic_bridge->bridge.funcs = &ingenic_drm_bridge_funcs;
+		ingenic_bridge->next_bridge = bridge;
+
+		ret = drm_bridge_attach(encoder, &ingenic_bridge->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

Re: [PATCH 10/11] drm/ingenic: Add doublescan feature

[Daniel Vetter]

On Fri, May 28, 2021 at 12:22:05AM +0100, Paul Cercueil wrote:
> A lot of devices with an Ingenic SoC have a weird LCD panel attached,
> where the pixels are not square. For instance, the AUO A030JTN01 and
> Innolux EJ030NA panels have a resolution of 320x480 with a 4:3 aspect
> ratio.
> 
> All userspace applications are built with the assumption that the
> pixels are square. To be able to support these devices without too
> much effort, add a doublescan feature, which allows the f0 and f1
> planes to be used with only half of the screen's vertical resolution,
> where each line of the input is displayed twice.
> 
> This is done using a chained list of DMA descriptors, one descriptor
> per output line.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 93 +++++++++++++++++++++--
>  1 file changed, 87 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 2761478b16e8..01d8490393d1 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -66,6 +66,8 @@ struct jz_soc_info {
>  
>  struct ingenic_gem_object {
>  	struct drm_gem_cma_object base;
> +	struct ingenic_dma_hwdesc *hwdescs;
> +	dma_addr_t hwdescs_phys;
>  };
>  
>  struct ingenic_drm_private_state {
> @@ -73,6 +75,23 @@ struct ingenic_drm_private_state {
>  
>  	bool no_vblank;
>  	bool use_palette;
> +
> +	/*
> +	 * A lot of devices with an Ingenic SoC have a weird LCD panel attached,
> +	 * where the pixels are not square. For instance, the AUO A030JTN01 and
> +	 * Innolux EJ030NA panels have a resolution of 320x480 with a 4:3 aspect
> +	 * ratio.
> +	 *
> +	 * All userspace applications are built with the assumption that the
> +	 * pixels are square. To be able to support these devices without too
> +	 * much effort, add a doublescan feature, which allows the f0 and f1
> +	 * planes to be used with only half of the screen's vertical resolution,
> +	 * where each line of the input is displayed twice.
> +	 *
> +	 * This is done using a chained list of DMA descriptors, one descriptor
> +	 * per output line.
> +	 */
> +	bool doublescan;
>  };
>  
>  struct ingenic_drm {
> @@ -465,7 +484,7 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
>  		return PTR_ERR(priv_state);
>  
>  	ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
> -						  DRM_PLANE_HELPER_NO_SCALING,
> +						  0x8000,
>  						  DRM_PLANE_HELPER_NO_SCALING,
>  						  priv->soc_info->has_osd,
>  						  true);
> @@ -482,6 +501,17 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
>  	     (new_plane_state->src_h >> 16) != new_plane_state->crtc_h))
>  		return -EINVAL;
>  
> +	/* Enable doublescan if the CRTC_H is twice the SRC_H. */
> +	priv_state->doublescan = (new_plane_state->src_h >> 16) * 2 == new_plane_state->crtc_h;
> +
> +	/* Otherwise, fail if CRTC_H != SRC_H */
> +	if (!priv_state->doublescan && (new_plane_state->src_h >> 16) != new_plane_state->crtc_h)
> +		return -EINVAL;
> +
> +	/* Fail if CRTC_W != SRC_W */
> +	if ((new_plane_state->src_w >> 16) != new_plane_state->crtc_w)
> +		return -EINVAL;
> +
>  	priv_state->use_palette = new_plane_state->fb &&
>  		new_plane_state->fb->format->format == DRM_FORMAT_C8;
>  
> @@ -647,7 +677,9 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>  	struct ingenic_drm_private_state *priv_state;
>  	struct drm_crtc_state *crtc_state;
>  	struct ingenic_dma_hwdesc *hwdesc;
> -	unsigned int width, height, cpp;
> +	unsigned int width, height, cpp, i;
> +	struct drm_gem_object *gem_obj;
> +	struct ingenic_gem_object *obj;
>  	dma_addr_t addr, next_addr;
>  	bool use_f1;
>  	u32 fourcc;
> @@ -664,17 +696,39 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>  		height = newstate->src_h >> 16;
>  		cpp = newstate->fb->format->cpp[0];
>  
> +		gem_obj = drm_gem_fb_get_obj(newstate->fb, 0);
> +		obj = to_ingenic_gem_obj(gem_obj);
> +
>  		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];
> +		if (priv_state->doublescan) {
> +			hwdesc = &obj->hwdescs[0];
> +			/*
> +			 * Use one DMA descriptor per output line, and display
> +			 * each input line twice.
> +			 */
> +			for (i = 0; i < newstate->crtc_h; i++) {
> +				hwdesc[i].next = obj->hwdescs_phys
> +					+ (i + 1) * sizeof(*hwdesc);
> +				hwdesc[i].addr = addr + (i / 2) * newstate->fb->pitches[0];
> +				hwdesc[i].cmd = newstate->fb->pitches[0] / 4;
> +			}
>  
> -		hwdesc->addr = addr;
> -		hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
> -		hwdesc->next = next_addr;
> +			/* We want the EOF IRQ only on the very last transfer */
> +			hwdesc[newstate->crtc_h - 1].cmd |= JZ_LCD_CMD_EOF_IRQ;
> +			hwdesc[newstate->crtc_h - 1].next = next_addr;
> +			priv->dma_hwdescs->hwdesc[use_f1] = *hwdesc;
> +		} else {
> +			/* Use one DMA descriptor for the whole frame. */
> +			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;
> @@ -848,6 +902,13 @@ static void ingenic_drm_disable_vblank(struct drm_crtc *crtc)
>  
>  static void ingenic_drm_gem_fb_destroy(struct drm_framebuffer *fb)
>  {
> +	struct ingenic_drm *priv = drm_device_get_priv(fb->dev);
> +	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> +	struct ingenic_gem_object *obj = to_ingenic_gem_obj(gem_obj);
> +
> +	dma_free_coherent(priv->dev,
> +			  sizeof(*obj->hwdescs) * fb->height,
> +			  obj->hwdescs, obj->hwdescs_phys);

You can have multiple fb pointing at the same gem bo. I think with that
this blows up pretty badly.

I think right call is to just move the doublescan mapping to the fb object
entirely, and not have a subclassed gem bo. Userspace shouldn't recreate
fb objects just for fun, so caching at the gem bo level shouldn't be
needed. But if you do need it then you need to refcount it so multiple fb
objects all work out (and they can have different modes even used for
them).

Another option is attaching this to the drm_plane state and setting it up
in prepare_plane/cleanup_plane. But then you probably need some caching at
the obj level (but a simple single slot/last hit cache should be good
enough here). I think this would be the cleanest since doing very
expensive things attached to fb might cause issues, userspace can
create/destroy quite a few of them as part of atomic_test (which skips the
prepare/cleanup_plane part). So this would be the right option if setting
up this dma mapping for doublescan is a more expensive thing to do.
-Daniel

>  	drm_gem_fb_destroy(fb);
>  }
>  
> @@ -868,6 +929,8 @@ ingenic_drm_gem_fb_create(struct drm_device *drm, struct drm_file *file,
>  {
>  	struct ingenic_drm *priv = drm_device_get_priv(drm);
>  	const struct drm_framebuffer_funcs *fb_funcs;
> +	struct drm_gem_object *gem_obj;
> +	struct ingenic_gem_object *obj;
>  	struct drm_framebuffer *fb;
>  
>  	if (priv->soc_info->map_noncoherent)
> @@ -876,6 +939,24 @@ ingenic_drm_gem_fb_create(struct drm_device *drm, struct drm_file *file,
>  		fb_funcs = &ingenic_drm_gem_fb_funcs;
>  
>  	fb = drm_gem_fb_create_with_funcs(drm, file, mode_cmd, fb_funcs);
> +	if (IS_ERR(fb))
> +		return fb;
> +
> +	gem_obj = drm_gem_fb_get_obj(fb, 0);
> +	obj = to_ingenic_gem_obj(gem_obj);
> +
> +	/*
> +	 * Create (fb->height * 2) DMA descriptors, in case we want to use the
> +	 * doublescan feature.
> +	 */
> +	obj->hwdescs = dma_alloc_coherent(priv->dev,
> +					  sizeof(*obj->hwdescs) * fb->height * 2,
> +					  &obj->hwdescs_phys,
> +					  GFP_KERNEL);
> +	if (!obj->hwdescs) {
> +		drm_gem_fb_destroy(fb);
> +		return ERR_PTR(-ENOMEM);
> +	}
>  
>  	return fb;
>  }
> -- 
> 2.30.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch