Re: [PATCH v2 3/6] drm: move tinydrm_xrgb8888_to_rgb565() to drm_fb_helper.c

[Noralf TrÃnnes]

Den 04.04.2019 17.24, skrev Gerd Hoffmann:
> Also rename to drm_fb_xrgb8888_to_rgb565().
> Pure code motion, no functional change.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> ---


> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index fabeb408dce6..087e49741094 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -3380,3 +3380,48 @@ void drm_fb_memcpy(void *dst, void *vaddr, struct drm_framebuffer *fb,
>  	}
>  }
>  EXPORT_SYMBOL(drm_fb_memcpy);
> +
> +/**
> + * drm_fb_xrgb8888_to_rgb565 - Convert XRGB8888 to RGB565 clip buffer
> + * @dst: RGB565 destination buffer
> + * @vaddr: XRGB8888 source buffer
> + * @fb: DRM framebuffer
> + * @clip: Clip rectangle area to copy
> + * @swap: Swap bytes
> + *
> + * Drivers can use this function for RGB565 devices that don't natively
> + * support XRGB8888.
> + */
> +void drm_fb_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
> +			       struct drm_framebuffer *fb,
> +			       struct drm_rect *clip, bool swap)
> +{
> +	size_t len = (clip->x2 - clip->x1) * sizeof(u32);
> +	unsigned int x, y;
> +	u32 *src, *buf;
> +	u16 val16;
> +
> +	buf = kmalloc(len, GFP_KERNEL);

It's not obvious what purpose this buffer serves and there's no help
from the surrounding functions either (tinydrm_swab16() has a comment).

I think we need a comment, something along these lines:

	/*
	 * Write-combined buffers have uncached reads.
	 * Speed up by fetching one line at a time.
	 */

Noralf.

> +	if (!buf)
> +		return;
> +
> +	for (y = clip->y1; y < clip->y2; y++) {
> +		src = vaddr + (y * fb->pitches[0]);
> +		src += clip->x1;
> +		memcpy(buf, src, len);
> +		src = buf;
> +		for (x = clip->x1; x < clip->x2; x++) {
> +			val16 = ((*src & 0x00F80000) >> 8) |
> +				((*src & 0x0000FC00) >> 5) |
> +				((*src & 0x000000F8) >> 3);
> +			src++;
> +			if (swap)
> +				*dst++ = swab16(val16);
> +			else
> +				*dst++ = val16;
> +		}
> +	}
> +
> +	kfree(buf);
> +}
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565);
> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> index f5c6f35e6161..f4b84b15da75 100644
> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> @@ -61,51 +61,6 @@ void tinydrm_swab16(u16 *dst, void *vaddr, struct drm_framebuffer *fb,
>  }
>  EXPORT_SYMBOL(tinydrm_swab16);
>  
> -/**
> - * tinydrm_xrgb8888_to_rgb565 - Convert XRGB8888 to RGB565 clip buffer
> - * @dst: RGB565 destination buffer
> - * @vaddr: XRGB8888 source buffer
> - * @fb: DRM framebuffer
> - * @clip: Clip rectangle area to copy
> - * @swap: Swap bytes
> - *
> - * Drivers can use this function for RGB565 devices that don't natively
> - * support XRGB8888.
> - */
> -void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
> -				struct drm_framebuffer *fb,
> -				struct drm_rect *clip, bool swap)
> -{
> -	size_t len = (clip->x2 - clip->x1) * sizeof(u32);
> -	unsigned int x, y;
> -	u32 *src, *buf;
> -	u16 val16;
> -
> -	buf = kmalloc(len, GFP_KERNEL);
> -	if (!buf)
> -		return;
> -
> -	for (y = clip->y1; y < clip->y2; y++) {
> -		src = vaddr + (y * fb->pitches[0]);
> -		src += clip->x1;
> -		memcpy(buf, src, len);
> -		src = buf;
> -		for (x = clip->x1; x < clip->x2; x++) {
> -			val16 = ((*src & 0x00F80000) >> 8) |
> -				((*src & 0x0000FC00) >> 5) |
> -				((*src & 0x000000F8) >> 3);
> -			src++;
> -			if (swap)
> -				*dst++ = swab16(val16);
> -			else
> -				*dst++ = val16;
> -		}
> -	}
> -
> -	kfree(buf);
> -}
> -EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
> -
>  /**
>   * tinydrm_xrgb8888_to_gray8 - Convert XRGB8888 to grayscale
>   * @dst: 8-bit grayscale destination buffer
> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> index e26fd61360a3..cf8df5fb7494 100644
> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> @@ -224,7 +224,7 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>  			drm_fb_memcpy(dst, src, fb, clip, false);
>  		break;
>  	case DRM_FORMAT_XRGB8888:
> -		tinydrm_xrgb8888_to_rgb565(dst, src, fb, clip, swap);
> +		drm_fb_xrgb8888_to_rgb565(dst, src, fb, clip, swap);
>  		break;
>  	default:
>  		dev_err_once(fb->dev->dev, "Format is not supported: %s\n",
>