Re: [PATCH 2/9] drm/format-helper: Add blitter functions

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jun 25, 2020 at 02:00:04PM +0200, Thomas Zimmermann wrote:
> The blitter functions copy a framebuffer to I/O memory using one of
> the existing conversion functions.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Hm I guess reason for adding dst_pitch in the previous patch is so that
there wouldn't be a special case here. Makes sense.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_format_helper.c | 87 +++++++++++++++++++++++++++++
>  include/drm/drm_format_helper.h     |  8 +++
>  2 files changed, 95 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index 8d5a683afea7..0e885cd34107 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -344,3 +344,90 @@ void drm_fb_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
>  }
>  EXPORT_SYMBOL(drm_fb_xrgb8888_to_gray8);
>  
> +/**
> + * drm_fb_blit_rect_dstclip - Copy parts of a framebuffer to display memory
> + * @dst:	The display memory to copy to
> + * @dst_pitch:	Number of bytes between two consecutive scanlines within dst
> + * @dst_format:	FOURCC code of the display's color format
> + * @vmap:	The framebuffer memory to copy from
> + * @fb:		The framebuffer to copy from
> + * @clip:	Clip rectangle area to copy
> + *
> + * This function copies parts of a framebuffer to display memory. If the
> + * formats of the display and the framebuffer mismatch, the blit function
> + * will attempt to convert between them.
> + *
> + * Use drm_fb_blit_dstclip() to copy the full framebuffer.
> + *
> + * Returns:
> + * 0 on success, or
> + * -EINVAL if the color-format conversion failed, or
> + * a negative error code otherwise.
> + */
> +int drm_fb_blit_rect_dstclip(void __iomem *dst, unsigned int dst_pitch,
> +			     uint32_t dst_format, void *vmap,
> +			     struct drm_framebuffer *fb,
> +			     struct drm_rect *clip)
> +{
> +	uint32_t fb_format = fb->format->format;
> +
> +	/* treat alpha channel like filler bits */
> +	if (fb_format == DRM_FORMAT_ARGB8888)
> +		fb_format = DRM_FORMAT_XRGB8888;
> +	if (dst_format == DRM_FORMAT_ARGB8888)
> +		dst_format = DRM_FORMAT_XRGB8888;
> +
> +	if (dst_format == fb_format) {
> +		drm_fb_memcpy_dstclip(dst, dst_pitch, vmap, fb, clip);
> +		return 0;
> +
> +	} else if (dst_format == DRM_FORMAT_RGB565) {
> +		if (fb_format == DRM_FORMAT_XRGB8888) {
> +			drm_fb_xrgb8888_to_rgb565_dstclip(dst, dst_pitch,
> +							  vmap, fb, clip,
> +							  false);
> +			return 0;
> +		}
> +	} else if (dst_format == DRM_FORMAT_RGB888) {
> +		if (fb_format == DRM_FORMAT_XRGB8888) {
> +			drm_fb_xrgb8888_to_rgb888_dstclip(dst, dst_pitch,
> +							  vmap, fb, clip);
> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(drm_fb_blit_rect_dstclip);
> +
> +/**
> + * drm_fb_blit_dstclip - Copy framebuffer to display memory
> + * @dst:	The display memory to copy to
> + * @dst_pitch:	Number of bytes between two consecutive scanlines within dst
> + * @dst_format:	FOURCC code of the display's color format
> + * @vmap:	The framebuffer memory to copy from
> + * @fb:		The framebuffer to copy from
> + *
> + * This function copies a full framebuffer to display memory. If the formats
> + * of the display and the framebuffer mismatch, the copy function will
> + * attempt to convert between them.
> + *
> + * See drm_fb_blit_rect_dstclip() for more inforamtion.
> + *
> + * Returns:
> + * 0 on success, or a negative error code otherwise.
> + */
> +int drm_fb_blit_dstclip(void __iomem *dst, unsigned int dst_pitch,
> +			uint32_t dst_format, void *vmap,
> +			struct drm_framebuffer *fb)

I do wonder whether we shouldn't have to some safety checks for this
stuff, like maybe something like:

struct dst_info {
	void __iomem *dst;
	unsigned int size;
	unsigned int dst_pitch;
	uint32_t dst_format;
};

And then the helpers would splat a WARNING if a driver ever gets this
wrong. Thinking about this because syzkaller has found tons of little
off-by-a-bit bugs in the vt/fbcon/fbdev code, and maybe we should try to
be better :-)

But that's material for another patch, and maybe once we have a few more
helpers in this library we can figure out a nice struct to package up
these long&confusing argument lists a bit.

Cheers, Daniel

> +{
> +	struct drm_rect fullscreen = {
> +		.x1 = 0,
> +		.x2 = fb->width,
> +		.y1 = 0,
> +		.y2 = fb->height,
> +	};
> +	return drm_fb_blit_rect_dstclip(dst, dst_pitch, dst_format, vmap, fb,
> +					&fullscreen);
> +}
> +EXPORT_SYMBOL(drm_fb_blit_dstclip);
> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
> index 2b5036a5fbe7..4e0258a61311 100644
> --- a/include/drm/drm_format_helper.h
> +++ b/include/drm/drm_format_helper.h
> @@ -28,4 +28,12 @@ void drm_fb_xrgb8888_to_rgb888_dstclip(void __iomem *dst, unsigned int dst_pitch
>  void drm_fb_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
>  			      struct drm_rect *clip);
>  
> +int drm_fb_blit_rect_dstclip(void __iomem *dst, unsigned int dst_pitch,
> +			     uint32_t dst_format, void *vmap,
> +			     struct drm_framebuffer *fb,
> +			     struct drm_rect *rect);
> +int drm_fb_blit_dstclip(void __iomem *dst, unsigned int dst_pitch,
> +			uint32_t dst_format, void *vmap,
> +			struct drm_framebuffer *fb);
> +
>  #endif /* __LINUX_DRM_FORMAT_HELPER_H */
> -- 
> 2.27.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel