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

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

[Paul Cercueil]

Hi,

I resend the V3 of my patchset for drm/ingenic, verbatim.

The previous submission of my V3 received a lot of replies, but none of
these replies were actually talking about the patches themselves.

Cheers,
-Paul


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

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

-- 
2.33.0

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

[Paul Cercueil]

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

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

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

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

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

[Christophe Branchereau]

Hi Paul,

Reviewed-by: Christophe Branchereau <cbranchereau@gmail.com>



On Tue, Oct 26, 2021 at 8:12 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
> Instead of having one 'hwdesc' variable for the plane #0, one for the
> plane #1 and one for the palette, use a 'hwdesc[3]' array, where the
> DMA hardware descriptors are indexed by the plane's number.
>
> v2: dma_hwdesc_addr() extended to support palette hwdesc. The palette
>     hwdesc is now hwdesc[3] to simplify things. Add
>     ingenic_drm_configure_hwdesc*() functions to factorize code.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 78 ++++++++++++++---------
>  1 file changed, 48 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index a5df1c8d34cd..95c12c2aba14 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -41,6 +41,8 @@
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_vblank.h>
>
> +#define HWDESC_PALETTE 2
> +
>  struct ingenic_dma_hwdesc {
>         u32 next;
>         u32 addr;
> @@ -49,9 +51,7 @@ struct ingenic_dma_hwdesc {
>  } __aligned(16);
>
>  struct ingenic_dma_hwdescs {
> -       struct ingenic_dma_hwdesc hwdesc_f0;
> -       struct ingenic_dma_hwdesc hwdesc_f1;
> -       struct ingenic_dma_hwdesc hwdesc_pal;
> +       struct ingenic_dma_hwdesc hwdesc[3];
>         u16 palette[256] __aligned(16);
>  };
>
> @@ -141,6 +141,14 @@ static inline struct ingenic_drm *drm_nb_get_priv(struct notifier_block *nb)
>         return container_of(nb, struct ingenic_drm, clock_nb);
>  }
>
> +static inline dma_addr_t dma_hwdesc_addr(const struct ingenic_drm *priv,
> +                                        unsigned int idx)
> +{
> +       u32 offset = offsetof(struct ingenic_dma_hwdescs, hwdesc[idx]);
> +
> +       return priv->dma_hwdescs_phys + offset;
> +}
> +
>  static int ingenic_drm_update_pixclk(struct notifier_block *nb,
>                                      unsigned long action,
>                                      void *data)
> @@ -558,9 +566,9 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>         struct ingenic_drm *priv = drm_device_get_priv(plane->dev);
>         struct drm_plane_state *newstate = drm_atomic_get_new_plane_state(state, plane);
>         struct drm_plane_state *oldstate = drm_atomic_get_old_plane_state(state, plane);
> +       unsigned int width, height, cpp, next_id, plane_id;
>         struct drm_crtc_state *crtc_state;
>         struct ingenic_dma_hwdesc *hwdesc;
> -       unsigned int width, height, cpp, offset;
>         dma_addr_t addr;
>         u32 fourcc;
>
> @@ -569,16 +577,14 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>                         drm_fb_cma_sync_non_coherent(&priv->drm, oldstate, newstate);
>
>                 crtc_state = newstate->crtc->state;
> +               plane_id = !!(priv->soc_info->has_osd && plane != &priv->f0);
>
>                 addr = drm_fb_cma_get_gem_addr(newstate->fb, newstate, 0);
>                 width = newstate->src_w >> 16;
>                 height = newstate->src_h >> 16;
>                 cpp = newstate->fb->format->cpp[0];
>
> -               if (!priv->soc_info->has_osd || plane == &priv->f0)
> -                       hwdesc = &priv->dma_hwdescs->hwdesc_f0;
> -               else
> -                       hwdesc = &priv->dma_hwdescs->hwdesc_f1;
> +               hwdesc = &priv->dma_hwdescs->hwdesc[plane_id];
>
>                 hwdesc->addr = addr;
>                 hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
> @@ -588,12 +594,8 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>
>                         ingenic_drm_plane_config(priv->dev, plane, fourcc);
>
> -                       if (fourcc == DRM_FORMAT_C8)
> -                               offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_pal);
> -                       else
> -                               offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
> -
> -                       priv->dma_hwdescs->hwdesc_f0.next = priv->dma_hwdescs_phys + offset;
> +                       next_id = fourcc == DRM_FORMAT_C8 ? HWDESC_PALETTE : 0;
> +                       priv->dma_hwdescs->hwdesc[0].next = dma_hwdesc_addr(priv, next_id);
>
>                         crtc_state->color_mgmt_changed = fourcc == DRM_FORMAT_C8;
>                 }
> @@ -846,6 +848,35 @@ static void __maybe_unused ingenic_drm_release_rmem(void *d)
>         of_reserved_mem_device_release(d);
>  }
>
> +static void ingenic_drm_configure_hwdesc(struct ingenic_drm *priv,
> +                                        unsigned int hwdesc,
> +                                        unsigned int next_hwdesc, u32 id)
> +{
> +       struct ingenic_dma_hwdesc *desc = &priv->dma_hwdescs->hwdesc[hwdesc];
> +
> +       desc->next = dma_hwdesc_addr(priv, next_hwdesc);
> +       desc->id = id;
> +}
> +
> +static void ingenic_drm_configure_hwdesc_palette(struct ingenic_drm *priv)
> +{
> +       struct ingenic_dma_hwdesc *desc;
> +
> +       ingenic_drm_configure_hwdesc(priv, HWDESC_PALETTE, 0, 0xc0);
> +
> +       desc = &priv->dma_hwdescs->hwdesc[HWDESC_PALETTE];
> +       desc->addr = priv->dma_hwdescs_phys
> +               + offsetof(struct ingenic_dma_hwdescs, palette);
> +       desc->cmd = JZ_LCD_CMD_ENABLE_PAL
> +               | (sizeof(priv->dma_hwdescs->palette) / 4);
> +}
> +
> +static void ingenic_drm_configure_hwdesc_plane(struct ingenic_drm *priv,
> +                                              unsigned int plane)
> +{
> +       ingenic_drm_configure_hwdesc(priv, plane, plane, 0xf0 | plane);
> +}
> +
>  static int ingenic_drm_bind(struct device *dev, bool has_components)
>  {
>         struct platform_device *pdev = to_platform_device(dev);
> @@ -942,27 +973,14 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>         if (!priv->dma_hwdescs)
>                 return -ENOMEM;
>
> -
>         /* Configure DMA hwdesc for foreground0 plane */
> -       dma_hwdesc_phys_f0 = priv->dma_hwdescs_phys
> -               + offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
> -       priv->dma_hwdescs->hwdesc_f0.next = dma_hwdesc_phys_f0;
> -       priv->dma_hwdescs->hwdesc_f0.id = 0xf0;
> +       ingenic_drm_configure_hwdesc_plane(priv, 0);
>
>         /* Configure DMA hwdesc for foreground1 plane */
> -       dma_hwdesc_phys_f1 = priv->dma_hwdescs_phys
> -               + offsetof(struct ingenic_dma_hwdescs, hwdesc_f1);
> -       priv->dma_hwdescs->hwdesc_f1.next = dma_hwdesc_phys_f1;
> -       priv->dma_hwdescs->hwdesc_f1.id = 0xf1;
> +       ingenic_drm_configure_hwdesc_plane(priv, 1);
>
>         /* Configure DMA hwdesc for palette */
> -       priv->dma_hwdescs->hwdesc_pal.next = priv->dma_hwdescs_phys
> -               + offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
> -       priv->dma_hwdescs->hwdesc_pal.id = 0xc0;
> -       priv->dma_hwdescs->hwdesc_pal.addr = priv->dma_hwdescs_phys
> -               + offsetof(struct ingenic_dma_hwdescs, palette);
> -       priv->dma_hwdescs->hwdesc_pal.cmd = JZ_LCD_CMD_ENABLE_PAL
> -               | (sizeof(priv->dma_hwdescs->palette) / 4);
> +       ingenic_drm_configure_hwdesc_palette(priv);
>
>         primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
>
> --
> 2.33.0
>

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

[Paul Cercueil]

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

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

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

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

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

[Christophe Branchereau]

Reviewed-by: Christophe Branchereau <cbranchereau@gmail.com>

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

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

[Paul Cercueil]

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

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

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

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

[Christophe Branchereau]

Reviewed-by: Christophe Branchereau <cbranchereau@gmail.com>

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

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

[Paul Cercueil]

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

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

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

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

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

[Christophe Branchereau]

Reviewed-by: Christophe Branchereau <cbranchereau@gmail.com>

On Tue, Oct 26, 2021 at 8:13 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
> Setting the DMA descriptor chain register in the probe function has been
> fine until now, because we only ever had one descriptor per foreground.
>
> As the driver will soon have real descriptor chains, and the DMA
> descriptor chain register updates itself to point to the current
> descriptor being processed, this register needs to be reset after a full
> modeset to point to the first descriptor of the chain.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 5dbeca0f8f37..cbc76cede99e 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -186,6 +186,10 @@ static void ingenic_drm_crtc_atomic_enable(struct drm_crtc *crtc,
>
>         regmap_write(priv->map, JZ_REG_LCD_STATE, 0);
>
> +       /* Set address of our DMA descriptor chain */
> +       regmap_write(priv->map, JZ_REG_LCD_DA0, dma_hwdesc_addr(priv, 0));
> +       regmap_write(priv->map, JZ_REG_LCD_DA1, dma_hwdesc_addr(priv, 1));
> +
>         regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
>                            JZ_LCD_CTRL_ENABLE | JZ_LCD_CTRL_DISABLE,
>                            JZ_LCD_CTRL_ENABLE);
> --
> 2.33.0
>

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

[Paul Cercueil]

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

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

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

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

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

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

[Christophe Branchereau]

Reviewed-by: Christophe Branchereau <cbranchereau@gmail.com>

On Tue, Oct 26, 2021 at 8:13 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
> When using C8 color mode, make sure that the palette is always uploaded
> before a frame; otherwise the very first frame will have wrong colors.
>
> Do that by changing the link order of the DMA descriptors.
>
> v3: Fix ingenic_drm_get_new_priv_state() called instead of
>     ingenic_drm_get_priv_state()
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 53 ++++++++++++++++++++---
>  1 file changed, 47 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index cbc76cede99e..a5e2880e07a1 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -66,6 +66,7 @@ struct jz_soc_info {
>
>  struct ingenic_drm_private_state {
>         struct drm_private_state base;
> +       bool use_palette;
>  };
>
>  struct ingenic_drm {
> @@ -113,6 +114,30 @@ to_ingenic_drm_priv_state(struct drm_private_state *state)
>         return container_of(state, struct ingenic_drm_private_state, base);
>  }
>
> +static struct ingenic_drm_private_state *
> +ingenic_drm_get_priv_state(struct ingenic_drm *priv, struct drm_atomic_state *state)
> +{
> +       struct drm_private_state *priv_state;
> +
> +       priv_state = drm_atomic_get_private_obj_state(state, &priv->private_obj);
> +       if (IS_ERR(priv_state))
> +               return ERR_CAST(priv_state);
> +
> +       return to_ingenic_drm_priv_state(priv_state);
> +}
> +
> +static struct ingenic_drm_private_state *
> +ingenic_drm_get_new_priv_state(struct ingenic_drm *priv, struct drm_atomic_state *state)
> +{
> +       struct drm_private_state *priv_state;
> +
> +       priv_state = drm_atomic_get_new_private_obj_state(state, &priv->private_obj);
> +       if (!priv_state)
> +               return NULL;
> +
> +       return to_ingenic_drm_priv_state(priv_state);
> +}
> +
>  static bool ingenic_drm_writeable_reg(struct device *dev, unsigned int reg)
>  {
>         switch (reg) {
> @@ -183,11 +208,18 @@ static void ingenic_drm_crtc_atomic_enable(struct drm_crtc *crtc,
>                                            struct drm_atomic_state *state)
>  {
>         struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
> +       struct ingenic_drm_private_state *priv_state;
> +       unsigned int next_id;
> +
> +       priv_state = ingenic_drm_get_priv_state(priv, state);
> +       if (WARN_ON(IS_ERR(priv_state)))
> +               return;
>
>         regmap_write(priv->map, JZ_REG_LCD_STATE, 0);
>
> -       /* Set address of our DMA descriptor chain */
> -       regmap_write(priv->map, JZ_REG_LCD_DA0, dma_hwdesc_addr(priv, 0));
> +       /* Set addresses of our DMA descriptor chains */
> +       next_id = priv_state->use_palette ? HWDESC_PALETTE : 0;
> +       regmap_write(priv->map, JZ_REG_LCD_DA0, dma_hwdesc_addr(priv, next_id));
>         regmap_write(priv->map, JZ_REG_LCD_DA1, dma_hwdesc_addr(priv, 1));
>
>         regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
> @@ -393,6 +425,7 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
>         struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
>                                                                                  plane);
>         struct ingenic_drm *priv = drm_device_get_priv(plane->dev);
> +       struct ingenic_drm_private_state *priv_state;
>         struct drm_crtc_state *crtc_state;
>         struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc;
>         int ret;
> @@ -405,6 +438,10 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
>         if (WARN_ON(!crtc_state))
>                 return -EINVAL;
>
> +       priv_state = ingenic_drm_get_priv_state(priv, state);
> +       if (IS_ERR(priv_state))
> +               return PTR_ERR(priv_state);
> +
>         ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
>                                                   DRM_PLANE_HELPER_NO_SCALING,
>                                                   DRM_PLANE_HELPER_NO_SCALING,
> @@ -423,6 +460,9 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
>              (new_plane_state->src_h >> 16) != new_plane_state->crtc_h))
>                 return -EINVAL;
>
> +       priv_state->use_palette = new_plane_state->fb &&
> +               new_plane_state->fb->format->format == DRM_FORMAT_C8;
> +
>         /*
>          * Require full modeset if enabling or disabling a plane, or changing
>          * its position, size or depth.
> @@ -583,6 +623,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>         struct drm_plane_state *newstate = drm_atomic_get_new_plane_state(state, plane);
>         struct drm_plane_state *oldstate = drm_atomic_get_old_plane_state(state, plane);
>         unsigned int width, height, cpp, next_id, plane_id;
> +       struct ingenic_drm_private_state *priv_state;
>         struct drm_crtc_state *crtc_state;
>         struct ingenic_dma_hwdesc *hwdesc;
>         dma_addr_t addr;
> @@ -600,19 +641,19 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>                 height = newstate->src_h >> 16;
>                 cpp = newstate->fb->format->cpp[0];
>
> -               hwdesc = &priv->dma_hwdescs->hwdesc[plane_id];
> +               priv_state = ingenic_drm_get_new_priv_state(priv, state);
> +               next_id = (priv_state && priv_state->use_palette) ? HWDESC_PALETTE : plane_id;
>
> +               hwdesc = &priv->dma_hwdescs->hwdesc[plane_id];
>                 hwdesc->addr = addr;
>                 hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
> +               hwdesc->next = dma_hwdesc_addr(priv, next_id);
>
>                 if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>                         fourcc = newstate->fb->format->format;
>
>                         ingenic_drm_plane_config(priv->dev, plane, fourcc);
>
> -                       next_id = fourcc == DRM_FORMAT_C8 ? HWDESC_PALETTE : 0;
> -                       priv->dma_hwdescs->hwdesc[0].next = dma_hwdesc_addr(priv, next_id);
> -
>                         crtc_state->color_mgmt_changed = fourcc == DRM_FORMAT_C8;
>                 }
>
> --
> 2.33.0
>

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

[Paul Cercueil]

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

All the bridges are now attached with DRM_BRIDGE_ATTACH_NO_CONNECTOR.

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

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

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

[Christophe Branchereau]

Reviewed-by: Christophe Branchereau <cbranchereau@gmail.com>

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

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

[Paul Cercueil]

Series applied, thanks!

Cheers,
-Paul


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

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

[H. Nikolaus Schaller]

Hi Paul,

> Am 26.10.2021 um 20:12 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi,
> 
> I resend the V3 of my patchset for drm/ingenic, verbatim.
> 
> The previous submission of my V3 received a lot of replies, but none of
> these replies were actually talking about the patches themselves.

Indeed. And since we have finally managed to add jz4780 HDMI support
(I didn't find to work in the latest comments) on top of the series as is,
please go ahead and add my

tested-by: Nikolaus Schaller <hns@goldelico.com>

BR and thanks,
Nikolaus

> 
> Cheers,
> -Paul
> 
> 
> Paul Cercueil (6):
>  drm/ingenic: Simplify code by using hwdescs array
>  drm/ingenic: Add support for private objects
>  drm/ingenic: Move IPU scale settings to private state
>  drm/ingenic: Set DMA descriptor chain register when starting CRTC
>  drm/ingenic: Upload palette before frame
>  drm/ingenic: Attach bridge chain to encoders
> 
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 278 +++++++++++++++++-----
> drivers/gpu/drm/ingenic/ingenic-ipu.c     | 127 ++++++++--
> 2 files changed, 333 insertions(+), 72 deletions(-)
> 
> -- 
> 2.33.0
>