Re: [PATCH 12/12] drm/format-helper: Move destination-buffer handling into internal helper

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 14509 bytes --]

Hi Sam

Am 05.08.22 um 19:52 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Wed, Jul 27, 2022 at 01:33:12PM +0200, Thomas Zimmermann wrote:
>> The format-convertion helpers handle several cases for different
>> values of destination buffer and pitch. Move that code into the
>> internal helper drm_fb_xfrm() and avoid quite a bit of duplucation.
> 
> This is very nice patch that should come before all the conversion
> patches - but then you have had to come up with another name.
> So I think this is fine.
> 
> A few comments below, mostly in the same area as the comments from José.
> 
> 	Sam
> 
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/drm_format_helper.c | 169 +++++++++++-----------------
>>   1 file changed, 64 insertions(+), 105 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
>> index d296d181659d..35aebdb90165 100644
>> --- a/drivers/gpu/drm/drm_format_helper.c
>> +++ b/drivers/gpu/drm/drm_format_helper.c
>> @@ -41,11 +41,11 @@ unsigned int drm_fb_clip_offset(unsigned int pitch, const struct drm_format_info
>>   }
>>   EXPORT_SYMBOL(drm_fb_clip_offset);
>>   
>> -/* TODO: Make this functon work with multi-plane formats. */
>> -static int drm_fb_xfrm(void *dst, unsigned long dst_pitch, unsigned long dst_pixsize,
>> -		       const void *vaddr, const struct drm_framebuffer *fb,
>> -		       const struct drm_rect *clip, bool vaddr_cached_hint,
>> -		       void (*xfrm_line)(void *dbuf, const void *sbuf, unsigned int npixels))
>> +/* TODO: Make this function work with multi-plane formats. */
>> +static int __drm_fb_xfrm(void *dst, unsigned long dst_pitch, unsigned long dst_pixsize,
>> +			 const void *vaddr, const struct drm_framebuffer *fb,
>> +			 const struct drm_rect *clip, bool vaddr_cached_hint,
>> +			 void (*xfrm_line)(void *dbuf, const void *sbuf, unsigned int npixels))
>>   {
>>   	unsigned long linepixels = drm_rect_width(clip);
>>   	unsigned long lines = drm_rect_height(clip);
>> @@ -84,11 +84,11 @@ static int drm_fb_xfrm(void *dst, unsigned long dst_pitch, unsigned long dst_pix
>>   	return 0;
>>   }
>>   
>> -/* TODO: Make this functon work with multi-plane formats. */
>> -static int drm_fb_xfrm_toio(void __iomem *dst, unsigned long dst_pitch, unsigned long dst_pixsize,
>> -			    const void *vaddr, const struct drm_framebuffer *fb,
>> -			    const struct drm_rect *clip, bool vaddr_cached_hint,
>> -			    void (*xfrm_line)(void *dbuf, const void *sbuf, unsigned int npixels))
>> +/* TODO: Make this function work with multi-plane formats. */
>> +static int __drm_fb_xfrm_toio(void __iomem *dst, unsigned long dst_pitch, unsigned long dst_pixsize,
>> +			      const void *vaddr, const struct drm_framebuffer *fb,
>> +			      const struct drm_rect *clip, bool vaddr_cached_hint,
>> +			      void (*xfrm_line)(void *dbuf, const void *sbuf, unsigned int npixels))
>>   {
>>   	unsigned long linepixels = drm_rect_width(clip);
>>   	unsigned long lines = drm_rect_height(clip);
>> @@ -129,6 +129,29 @@ static int drm_fb_xfrm_toio(void __iomem *dst, unsigned long dst_pitch, unsigned
>>   	return 0;
>>   }
>>   
>> +/* TODO: Make this function work with multi-plane formats. */
>> +static int drm_fb_xfrm(struct iosys_map *dst,
>> +		       const unsigned int *dst_pitch, const u8 *dst_pixsize,
>> +		       const struct iosys_map *vmap, const struct drm_framebuffer *fb,
>> +		       const struct drm_rect *clip, bool vaddr_cached_hint,
>> +		       void (*xfrm_line)(void *dbuf, const void *sbuf, unsigned int npixels))
>> +{
> Just to repeat myself a little, this assumes src (vmap) is always system
> memory (not io).
> 
>> +	static const unsigned int default_dst_pitch[DRM_FORMAT_MAX_PLANES] = {
>> +		0, 0, 0, 0
>> +	};
>> +
>> +	if (!dst_pitch)
>> +		dst_pitch = default_dst_pitch;
>> +
>> +	if (dst[0].is_iomem)
>> +		return __drm_fb_xfrm_toio(dst[0].vaddr_iomem, dst_pitch[0], dst_pixsize[0],
>> +					  vmap[0].vaddr, fb, clip, false, xfrm_line);
>> +	else
>> +		return __drm_fb_xfrm(dst[0].vaddr, dst_pitch[0], dst_pixsize[0],
>> +				     vmap[0].vaddr, fb, clip, false, xfrm_line);
> 
> It looks like vaddr_cached_hint is always false, so can we remove it?

Always passing false is a bug. drm_fb_swab() uses vaddr_cached_hint and 
we should forward the value here.

> 
>> +}
>> +
>> +
>>   /**
>>    * drm_fb_memcpy - Copy clip buffer
>>    * @dst: Array of destination buffers
>> @@ -213,14 +236,10 @@ void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
>>   		 const struct iosys_map *vmap, const struct drm_framebuffer *fb,
>>   		 const struct drm_rect *clip, bool cached)
>>   {
>> -	static const unsigned int default_dst_pitch[DRM_FORMAT_MAX_PLANES] = {
>> -		0, 0, 0, 0
>> -	};
>>   	const struct drm_format_info *format = fb->format;
>> -	u8 cpp = format->cpp[0];
>>   	void (*swab_line)(void *dbuf, const void *sbuf, unsigned int npixels);
>>   
>> -	switch (cpp) {
>> +	switch (format->cpp[0]) {
>>   	case 4:
>>   		swab_line = drm_fb_swab32_line;
>>   		break;
>> @@ -230,21 +249,10 @@ void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
>>   	default:
>>   		drm_warn_once(fb->dev, "Format %p4cc has unsupported pixel size.\n",
>>   			      &format->format);
>> -		swab_line = NULL;
>> -		break;
>> -	}
>> -	if (!swab_line)
>>   		return;
>> +	}
>>   
>> -	if (!dst_pitch)
>> -		dst_pitch = default_dst_pitch;
>> -
>> -	if (dst->is_iomem)
>> -		drm_fb_xfrm_toio(dst[0].vaddr_iomem, dst_pitch[0], cpp,
>> -				 vmap[0].vaddr, fb, clip, cached, swab_line);
>> -	else
>> -		drm_fb_xfrm(dst[0].vaddr, dst_pitch[0], cpp, vmap[0].vaddr, fb,
>> -			    clip, cached, swab_line);
>> +	drm_fb_xfrm(dst, dst_pitch, format->cpp, vmap, fb, clip, cached, swab_line);
> 
> In this case we pass fb->format-cpp as dst_pitch - so we could retreive
> is via the fb pointer.

I don't understand this comment. We're passing format->cpp as 
dst_pixsize. I've meanwhile updated the code to compute the value from 
drm_format_info_bpp().

> 
>>   }
>>   EXPORT_SYMBOL(drm_fb_swab);
>>   
>> @@ -277,19 +285,12 @@ void drm_fb_xrgb8888_to_rgb332(struct iosys_map *dst, const unsigned int *dst_pi
>>   			       const struct iosys_map *vmap, const struct drm_framebuffer *fb,
>>   			       const struct drm_rect *clip)
>>   {
>> -	static const unsigned int default_dst_pitch[DRM_FORMAT_MAX_PLANES] = {
>> -		0, 0, 0, 0
>> +	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
>> +		1,
>>   	};
>>   
>> -	if (!dst_pitch)
>> -		dst_pitch = default_dst_pitch;
>> -
>> -	if (dst[0].is_iomem)
>> -		drm_fb_xfrm_toio(dst[0].vaddr_iomem, dst_pitch[0], 1, vmap[0].vaddr, fb, clip,
>> -				 false, drm_fb_xrgb8888_to_rgb332_line);
>> -	else
>> -		drm_fb_xfrm(dst[0].vaddr, dst_pitch[0], 1, vmap[0].vaddr, fb, clip,
>> -			    false, drm_fb_xrgb8888_to_rgb332_line);
>> +	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, vmap, fb, clip, false,
>> +		    drm_fb_xrgb8888_to_rgb332_line);
> 
> Here we construct the dst_pixsize.
> What is needed to make us trust fb->format->cpp so we can just fetch the
> info from format_info and drop dst_pixsize?
> 
> I do not see any lookup being necessary here or in the functions below.
> 
> If we use cpp (or even better using a helper function that avoid the
> deprecated cpp), then adding support for planes is simpler. For now
> dst_pixsize only pass the size for the first plane and there are a lot
> of updates required to support additional planes.
> 
> Maybe I miss something obvious?!?

I'm not sure what you're up to. As I said to Jose, we could pass the 
destination format around. But that would require an O(n) lookup of the 
format info, or changes to the calling drivers. Both are not really 
appropriate in this patchset.

For now, I store the pixsize here and that's it. We already had these 
constants in the code here, so at least it doesn't get worse.

Best regards
Thomas

> 
> 
>>   }
>>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb332);
>>   
>> @@ -344,9 +345,10 @@ void drm_fb_xrgb8888_to_rgb565(struct iosys_map *dst, const unsigned int *dst_pi
>>   			       const struct iosys_map *vmap, const struct drm_framebuffer *fb,
>>   			       const struct drm_rect *clip, bool swab)
>>   {
>> -	static const unsigned int default_dst_pitch[DRM_FORMAT_MAX_PLANES] = {
>> -		0, 0, 0, 0
>> +	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
>> +		2,
>>   	};
>> +
>>   	void (*xfrm_line)(void *dbuf, const void *sbuf, unsigned int npixels);
>>   
>>   	if (swab)
>> @@ -354,15 +356,7 @@ void drm_fb_xrgb8888_to_rgb565(struct iosys_map *dst, const unsigned int *dst_pi
>>   	else
>>   		xfrm_line = drm_fb_xrgb8888_to_rgb565_line;
>>   
>> -	if (!dst_pitch)
>> -		dst_pitch = default_dst_pitch;
>> -
>> -	if (dst[0].is_iomem)
>> -		drm_fb_xfrm_toio(dst[0].vaddr_iomem, dst_pitch[0], 2, vmap[0].vaddr, fb, clip,
>> -				 false, xfrm_line);
>> -	else
>> -		drm_fb_xfrm(dst[0].vaddr, dst_pitch[0], 2, vmap[0].vaddr, fb, clip,
>> -			    false, xfrm_line);
>> +	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, vmap, fb, clip, false, xfrm_line);
>>   }
>>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565);
>>   
>> @@ -396,19 +390,12 @@ void drm_fb_xrgb8888_to_rgb888(struct iosys_map *dst, const unsigned int *dst_pi
>>   			       const struct iosys_map *vmap, const struct drm_framebuffer *fb,
>>   			       const struct drm_rect *clip)
>>   {
>> -	static const unsigned int default_dst_pitch[DRM_FORMAT_MAX_PLANES] = {
>> -		0, 0, 0, 0
>> +	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
>> +		3,
>>   	};
>>   
>> -	if (!dst_pitch)
>> -		dst_pitch = default_dst_pitch;
>> -
>> -	if (dst[0].is_iomem)
>> -		drm_fb_xfrm_toio(dst[0].vaddr_iomem, dst_pitch[0], 3, vmap[0].vaddr, fb,
>> -				 clip, false, drm_fb_xrgb8888_to_rgb888_line);
>> -	else
>> -		drm_fb_xfrm(dst[0].vaddr, dst_pitch[0], 3, vmap[0].vaddr, fb,
>> -			    clip, false, drm_fb_xrgb8888_to_rgb888_line);
>> +	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, vmap, fb, clip, false,
>> +		    drm_fb_xrgb8888_to_rgb888_line);
>>   }
>>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb888);
>>   
>> @@ -435,19 +422,12 @@ static void drm_fb_rgb565_to_xrgb8888(struct iosys_map *dst, const unsigned int
>>   				      const struct drm_framebuffer *fb,
>>   				      const struct drm_rect *clip)
>>   {
>> -	static const unsigned int default_dst_pitch[DRM_FORMAT_MAX_PLANES] = {
>> -		0, 0, 0, 0
>> +	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
>> +		4,
>>   	};
>>   
>> -	if (!dst_pitch)
>> -		dst_pitch = default_dst_pitch;
>> -
>> -	if (dst[0].is_iomem)
>> -		drm_fb_xfrm_toio(dst[0].vaddr_iomem, dst_pitch[0], 4, vmap[0].vaddr, fb,
>> -				 clip, false, drm_fb_rgb565_to_xrgb8888_line);
>> -	else
>> -		drm_fb_xfrm(dst[0].vaddr, dst_pitch[0], 4, vmap[0].vaddr, fb,
>> -			    clip, false, drm_fb_rgb565_to_xrgb8888_line);
>> +	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, vmap, fb, clip, false,
>> +		    drm_fb_rgb565_to_xrgb8888_line);
>>   }
>>   
>>   static void drm_fb_rgb888_to_xrgb8888_line(void *dbuf, const void *sbuf, unsigned int pixels)
>> @@ -470,19 +450,12 @@ static void drm_fb_rgb888_to_xrgb8888(struct iosys_map *dst, const unsigned int
>>   				      const struct drm_framebuffer *fb,
>>   				      const struct drm_rect *clip)
>>   {
>> -	static const unsigned int default_dst_pitch[DRM_FORMAT_MAX_PLANES] = {
>> -		0, 0, 0, 0
>> +	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
>> +		4,
>>   	};
>>   
>> -	if (!dst_pitch)
>> -		dst_pitch = default_dst_pitch;
>> -
>> -	if (dst[0].is_iomem)
>> -		drm_fb_xfrm_toio(dst[0].vaddr_iomem, dst_pitch[0], 4, vmap[0].vaddr, fb,
>> -				 clip, false, drm_fb_rgb888_to_xrgb8888_line);
>> -	else
>> -		drm_fb_xfrm(dst[0].vaddr, dst_pitch[0], 4, vmap[0].vaddr, fb,
>> -			    clip, false, drm_fb_rgb888_to_xrgb8888_line);
>> +	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, vmap, fb, clip, false,
>> +		    drm_fb_rgb888_to_xrgb8888_line);
>>   }
>>   
>>   static void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels)
>> @@ -518,19 +491,12 @@ void drm_fb_xrgb8888_to_xrgb2101010(struct iosys_map *dst, const unsigned int *d
>>   				    const struct iosys_map *vmap, const struct drm_framebuffer *fb,
>>   				    const struct drm_rect *clip)
>>   {
>> -	static const unsigned int default_dst_pitch[DRM_FORMAT_MAX_PLANES] = {
>> -		0, 0, 0, 0
>> +	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
>> +		4,
>>   	};
>>   
>> -	if (!dst_pitch)
>> -		dst_pitch = default_dst_pitch;
>> -
>> -	if (dst[0].is_iomem)
>> -		drm_fb_xfrm_toio(dst[0].vaddr_iomem, dst_pitch[0], 4, vmap[0].vaddr, fb,
>> -				 clip, false, drm_fb_xrgb8888_to_xrgb2101010_line);
>> -	else
>> -		drm_fb_xfrm(dst[0].vaddr, dst_pitch[0], 4, vmap[0].vaddr, fb,
>> -			    clip, false, drm_fb_xrgb8888_to_xrgb2101010_line);
>> +	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, vmap, fb, clip, false,
>> +		    drm_fb_xrgb8888_to_xrgb2101010_line);
>>   }
>>   
>>   static void drm_fb_xrgb8888_to_gray8_line(void *dbuf, const void *sbuf, unsigned int pixels)
>> @@ -571,19 +537,12 @@ void drm_fb_xrgb8888_to_gray8(struct iosys_map *dst, const unsigned int *dst_pit
>>   			      const struct iosys_map *vmap, const struct drm_framebuffer *fb,
>>   			      const struct drm_rect *clip)
>>   {
>> -	static const unsigned int default_dst_pitch[DRM_FORMAT_MAX_PLANES] = {
>> -		0, 0, 0, 0
>> +	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
>> +		1,
>>   	};
>>   
>> -	if (!dst_pitch)
>> -		dst_pitch = default_dst_pitch;
>> -
>> -	if (dst[0].is_iomem)
>> -		drm_fb_xfrm_toio(dst[0].vaddr_iomem, dst_pitch[0], 1, vmap[0].vaddr, fb,
>> -				 clip, false, drm_fb_xrgb8888_to_gray8_line);
>> -	else
>> -		drm_fb_xfrm(dst[0].vaddr, dst_pitch[0], 1, vmap[0].vaddr, fb,
>> -			    clip, false, drm_fb_xrgb8888_to_gray8_line);
>> +	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, vmap, fb, clip, false,
>> +		    drm_fb_xrgb8888_to_gray8_line);
>>   }
>>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_gray8);
>>   
>> -- 
>> 2.37.1

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]