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

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

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

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

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

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