Re: [PATCH 2/8] drm/ingenic: Simplify code by using hwdescs array

From: Paul Cercueil
Date: Sun Aug 08 2021 - 18:22:51 EST


Hi Thomas,

Le dim., août 8 2021 at 20:42:53 +0200, Thomas Zimmermann <tzimmermann@xxxxxxx> a écrit :
Hi

Am 08.08.21 um 15:45 schrieb Paul Cercueil:
Instead of having one 'hwdesc' variable for the plane #0 and one for the
plane #1, use a 'hwdesc[2]' array, where the DMA hardware descriptors
are indexed by the plane's number.

Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
---
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 38 ++++++++++++-----------
1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index e42eb43d8020..bc71ba44ccf4 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -49,8 +49,7 @@ struct ingenic_dma_hwdesc {
} __aligned(16);
 struct ingenic_dma_hwdescs {
- struct ingenic_dma_hwdesc hwdesc_f0;
- struct ingenic_dma_hwdesc hwdesc_f1;
+ struct ingenic_dma_hwdesc hwdesc[2];
struct ingenic_dma_hwdesc hwdesc_pal;
u16 palette[256] __aligned(16);
};
@@ -141,6 +140,13 @@ static inline struct ingenic_drm *drm_nb_get_priv(struct notifier_block *nb)
return container_of(nb, struct ingenic_drm, clock_nb);
}
+static inline dma_addr_t dma_hwdesc_addr(const struct ingenic_drm *priv, bool use_f1)

Using the plane index instead of a boolean would be more aligned to the way this function is being used.

Alright, I can do that.

+{
+ u32 offset = offsetof(struct ingenic_dma_hwdescs, hwdesc[use_f1]);

use_f1 is a function parameter. Is offsetof guaranteed to be evaluated at runtime?

The offsetof() macro could be defined like this:
#define offsetof(type, elm) ((size_t) &((type *) 0)->elm)

So I don't see a reason why this couldn't be evaluated at runtime, yes. It's just that the value of "offset" is not known at compilation time (unless the compiler does some constant propagation). In practice though, this code works fine.

+
+ return priv->dma_hwdescs_phys + offset;
+}
+
static int ingenic_drm_update_pixclk(struct notifier_block *nb,
unsigned long action,
void *data)
@@ -562,6 +568,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
struct ingenic_dma_hwdesc *hwdesc;
unsigned int width, height, cpp, offset;
dma_addr_t addr;
+ bool use_f1;
u32 fourcc;
 if (newstate && newstate->fb) {
@@ -569,16 +576,14 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
drm_fb_cma_sync_non_coherent(&priv->drm, oldstate, newstate);
 crtc_state = newstate->crtc->state;
+ use_f1 = priv->soc_info->has_osd && plane != &priv->f0;
 addr = drm_fb_cma_get_gem_addr(newstate->fb, newstate, 0);
width = newstate->src_w >> 16;
height = newstate->src_h >> 16;
cpp = newstate->fb->format->cpp[0];
- if (!priv->soc_info->has_osd || plane == &priv->f0)
- hwdesc = &priv->dma_hwdescs->hwdesc_f0;
- else
- hwdesc = &priv->dma_hwdescs->hwdesc_f1;
+ hwdesc = &priv->dma_hwdescs->hwdesc[use_f1];

Maybe add a helper that looks up the hwdesc field for a given plane?

Sure.

 hwdesc->addr = addr;
hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
@@ -591,9 +596,9 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
if (fourcc == DRM_FORMAT_C8)
offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_pal);
else
- offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
+ offset = offsetof(struct ingenic_dma_hwdescs, hwdesc[0]);
- priv->dma_hwdescs->hwdesc_f0.next = priv->dma_hwdescs_phys + offset;
+ priv->dma_hwdescs->hwdesc[0].next = priv->dma_hwdescs_phys + offset;

Maybe priv->dma_hwdescs_phys + offset could be computed in a more flexible helper than dma_hwdesc_addr().

 crtc_state->color_mgmt_changed = fourcc == DRM_FORMAT_C8;
}
@@ -964,20 +969,17 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
  /* Configure DMA hwdesc for foreground0 plane */
- dma_hwdesc_phys_f0 = priv->dma_hwdescs_phys
- + offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
- priv->dma_hwdescs->hwdesc_f0.next = dma_hwdesc_phys_f0;
- priv->dma_hwdescs->hwdesc_f0.id = 0xf0;
+ dma_hwdesc_phys_f0 = dma_hwdesc_addr(priv, 0);
+ priv->dma_hwdescs->hwdesc[0].next = dma_hwdesc_phys_f0;
+ priv->dma_hwdescs->hwdesc[0].id = 0xf0;
 /* Configure DMA hwdesc for foreground1 plane */
- dma_hwdesc_phys_f1 = priv->dma_hwdescs_phys
- + offsetof(struct ingenic_dma_hwdescs, hwdesc_f1);
- priv->dma_hwdescs->hwdesc_f1.next = dma_hwdesc_phys_f1;
- priv->dma_hwdescs->hwdesc_f1.id = 0xf1;
+ dma_hwdesc_phys_f1 = dma_hwdesc_addr(priv, 1);
+ priv->dma_hwdescs->hwdesc[1].next = dma_hwdesc_phys_f1;
+ priv->dma_hwdescs->hwdesc[1].id = 0xf1;
 /* Configure DMA hwdesc for palette */
- priv->dma_hwdescs->hwdesc_pal.next = priv->dma_hwdescs_phys
- + offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
+ priv->dma_hwdescs->hwdesc_pal.next = dma_hwdesc_phys_f0;
priv->dma_hwdescs->hwdesc_pal.id = 0xc0;
priv->dma_hwdescs->hwdesc_pal.addr = priv->dma_hwdescs_phys
+ offsetof(struct ingenic_dma_hwdescs, palette);


Could the setup in these three blocks be moved into a common helper?

Yes.

Thanks for the review.

Cheers,
-Paul