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

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@crapouillou.net

drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 38 ++++++++++++----------- 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index e42eb43d8020..bc71ba44ccf4 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -49,8 +49,7 @@ struct ingenic_dma_hwdesc { } __aligned(16);

struct ingenic_dma_hwdescs {

• struct ingenic_dma_hwdesc hwdesc_f0;
• struct ingenic_dma_hwdesc hwdesc_f1;

• struct ingenic_dma_hwdesc hwdesc[2]; struct ingenic_dma_hwdesc hwdesc_pal; u16 palette[256] __aligned(16); };

@@ -141,6 +140,13 @@ static inline struct ingenic_drm *drm_nb_get_priv(struct notifier_block *nb) return container_of(nb, struct ingenic_drm, clock_nb); }

+static inline dma_addr_t dma_hwdesc_addr(const struct ingenic_drm *priv, bool use_f1)

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

+{

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

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

• return priv->dma_hwdescs_phys + offset;

+}

• static int ingenic_drm_update_pixclk(struct notifier_block *nb, unsigned long action, void *data)

@@ -562,6 +568,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, struct ingenic_dma_hwdesc *hwdesc; unsigned int width, height, cpp, offset; dma_addr_t addr;

• bool use_f1; u32 fourcc;

  if (newstate && newstate->fb) {

@@ -569,16 +576,14 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, drm_fb_cma_sync_non_coherent(&priv->drm, oldstate, newstate);

crtc_state = newstate->crtc->state;

• use_f1 = priv->soc_info->has_osd && plane != &priv->f0;
  
  

  addr = drm_fb_cma_get_gem_addr(newstate->fb, newstate, 0); width = newstate->src_w >> 16; height = newstate->src_h >> 16; cpp = newstate->fb->format->cpp[0];

• if (!priv->soc_info->has_osd || plane == &priv->f0)
  

• 	hwdesc = &priv->dma_hwdescs->hwdesc_f0;
  

• else
  

• 	hwdesc = &priv->dma_hwdescs->hwdesc_f1;
  

• hwdesc = &priv->dma_hwdescs->hwdesc[use_f1];
  

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

hwdesc->addr = addr;
hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);

@@ -591,9 +596,9 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, if (fourcc == DRM_FORMAT_C8) offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_pal); else

• 		offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
  

• 		offset = offsetof(struct ingenic_dma_hwdescs, hwdesc[0]);
  
  

• 	priv->dma_hwdescs->hwdesc_f0.next = priv->dma_hwdescs_phys + offset;
  

• 	priv->dma_hwdescs->hwdesc[0].next = priv->dma_hwdescs_phys + offset;
  

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

	crtc_state->color_mgmt_changed = fourcc == DRM_FORMAT_C8;
}

@@ -964,20 +969,17 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)

/* Configure DMA hwdesc for foreground0 plane */

• dma_hwdesc_phys_f0 = priv->dma_hwdescs_phys

• + offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
  

• priv->dma_hwdescs->hwdesc_f0.next = dma_hwdesc_phys_f0;
• priv->dma_hwdescs->hwdesc_f0.id = 0xf0;

• dma_hwdesc_phys_f0 = dma_hwdesc_addr(priv, 0);

• priv->dma_hwdescs->hwdesc[0].next = dma_hwdesc_phys_f0;

• priv->dma_hwdescs->hwdesc[0].id = 0xf0;

  /* Configure DMA hwdesc for foreground1 plane */

• dma_hwdesc_phys_f1 = priv->dma_hwdescs_phys

• + offsetof(struct ingenic_dma_hwdescs, hwdesc_f1);
  

• priv->dma_hwdescs->hwdesc_f1.next = dma_hwdesc_phys_f1;
• priv->dma_hwdescs->hwdesc_f1.id = 0xf1;

• dma_hwdesc_phys_f1 = dma_hwdesc_addr(priv, 1);

• priv->dma_hwdescs->hwdesc[1].next = dma_hwdesc_phys_f1;

• priv->dma_hwdescs->hwdesc[1].id = 0xf1;

  /* Configure DMA hwdesc for palette */

• priv->dma_hwdescs->hwdesc_pal.next = priv->dma_hwdescs_phys

• + offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
  

• priv->dma_hwdescs->hwdesc_pal.next = dma_hwdesc_phys_f0; priv->dma_hwdescs->hwdesc_pal.id = 0xc0; priv->dma_hwdescs->hwdesc_pal.addr = priv->dma_hwdescs_phys
  • offsetof(struct ingenic_dma_hwdescs, palette);

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

Best regards Thomas