Date:   Mon, 09 Aug 2021 00:22:38 +0200
From:   Paul Cercueil <paul@...pouillou.net>
To:     Thomas Zimmermann <tzimmermann@...e.de>
Cc:     David Airlie <airlied@...ux.ie>, Daniel Vetter <daniel@...ll.ch>,
        "H . Nikolaus Schaller" <hns@...delico.com>,
        Paul Boddie <paul@...die.org.uk>, list@...ndingux.net,
        Sam Ravnborg <sam@...nborg.org>, linux-mips@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/8] drm/ingenic: Simplify code by using hwdescs array

Hi Thomas,

Le dim., août 8 2021 at 20:42:53 +0200, Thomas Zimmermann 
<tzimmermann@...e.de> 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@...pouillou.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.

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

Powered by blists - more mailing lists