| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <10bc08a5-0e64-d8ac-b71f-d44e16da8f61@suse.de>
Date: Thu, 31 Aug 2023 10:01:46 +0200
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Geert Uytterhoeven <geert@...ux-m68k.org>,
Javier Martinez Canillas <javierm@...hat.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>
Cc: dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1
Hi
Am 24.08.23 um 17:08 schrieb Geert Uytterhoeven:
> The native display format is monochrome light-on-dark (R1).
> Hence add support for R1, so monochrome applications not only look
> better, but also avoid the overhead of back-and-forth conversions
> between R1 and XR24.
>
> Do not allocate the intermediate conversion buffer when it is not
> needed, and reorder the two buffer allocations to streamline operation.
>
> Signed-off-by: Geert Uytterhoeven <geert@...ux-m68k.org>
> ---
> v2:
> - Rework on top op commit 8c3926367ac9df6c ("drm/ssd130x: Use
> shadow-buffer helpers when managing plane's state") in drm/drm-next.
> Hence I did not add Javier's tags given on v1.
> - Do not allocate intermediate buffer when not needed.
> ---
> drivers/gpu/drm/solomon/ssd130x.c | 76 +++++++++++++++++++++----------
> 1 file changed, 51 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 78272b1f9d5b167f..18007cb4f3de5aa1 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -449,15 +449,14 @@ static int ssd130x_init(struct ssd130x_device *ssd130x)
>
> static int ssd130x_update_rect(struct ssd130x_device *ssd130x,
> struct ssd130x_plane_state *ssd130x_state,
> + u8 *buf, unsigned int pitch,
> struct drm_rect *rect)
> {
> unsigned int x = rect->x1;
> unsigned int y = rect->y1;
> - u8 *buf = ssd130x_state->buffer;
> u8 *data_array = ssd130x_state->data_array;
> unsigned int width = drm_rect_width(rect);
> unsigned int height = drm_rect_height(rect);
> - unsigned int line_length = DIV_ROUND_UP(width, 8);
> unsigned int page_height = ssd130x->device_info->page_height;
> unsigned int pages = DIV_ROUND_UP(height, page_height);
> struct drm_device *drm = &ssd130x->drm;
> @@ -516,7 +515,7 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x,
> u8 data = 0;
>
> for (k = 0; k < m; k++) {
> - u8 byte = buf[(8 * i + k) * line_length + j / 8];
> + u8 byte = buf[(8 * i + k) * pitch + j / 8];
> u8 bit = (byte >> (j % 8)) & 1;
>
> data |= bit << k;
> @@ -602,27 +601,51 @@ static int ssd130x_fb_blit_rect(struct drm_plane_state *state,
The handing of different formats in this function is confusing. I'd
rather implement ssd130x_fb_blit_rect_r1 and
ssd130x_fb_blit_rect_xrgb8888 which then handle a single format.
> struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
> unsigned int page_height = ssd130x->device_info->page_height;
> struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state);
> - u8 *buf = ssd130x_state->buffer;
> struct iosys_map dst;
> unsigned int dst_pitch;
> int ret = 0;
> + u8 *buf;
>
> /* Align y to display page boundaries */
> rect->y1 = round_down(rect->y1, page_height);
> rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height);
>
> - dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
> + switch (fb->format->format) {
> + case DRM_FORMAT_R1:
> + /* Align x to byte boundaries */
> + rect->x1 = round_down(rect->x1, 8);
> + rect->x2 = round_up(rect->x2, 8);
Is rounding to multiples of 8 guaranteed to work? I know it did on
VGA-compatible HW, because VGA enforces it. But is that generally the case?
>
> - ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> - if (ret)
> - return ret;
> + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> + if (ret)
> + return ret;
> +
> + dst_pitch = fb->pitches[0];
> + buf = vmap[0].vaddr + rect->y1 * dst_pitch + rect->x1 / 8;
> +
> + ssd130x_update_rect(ssd130x, ssd130x_state, buf, dst_pitch,
> + rect);
> +
> + drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> + break;
> +
> + case DRM_FORMAT_XRGB8888:
> + dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
> + buf = ssd130x_state->buffer;
> +
> + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> + if (ret)
> + return ret;
>
> - iosys_map_set_vaddr(&dst, buf);
> - drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect);
> + iosys_map_set_vaddr(&dst, buf);
> + drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect);
>
> - drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> + drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>
> - ssd130x_update_rect(ssd130x, ssd130x_state, rect);
> + ssd130x_update_rect(ssd130x, ssd130x_state, buf, dst_pitch,
> + rect);
> + break;
> + }
>
> return ret;
> }
> @@ -644,22 +667,24 @@ static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
> if (ret)
> return ret;
>
> - fi = drm_format_info(DRM_FORMAT_R1);
> - if (!fi)
> - return -EINVAL;
> + ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
> + if (!ssd130x_state->data_array)
> + return -ENOMEM;
>
> - pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
> + if (plane_state->fb->format->format != DRM_FORMAT_R1) {
> + fi = drm_format_info(DRM_FORMAT_R1);
> + if (!fi)
> + return -EINVAL;
>
> - ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
> - if (!ssd130x_state->buffer)
> - return -ENOMEM;
> + pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
>
> - ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
> - if (!ssd130x_state->data_array) {
> - kfree(ssd130x_state->buffer);
> - /* Set to prevent a double free in .atomic_destroy_state() */
> - ssd130x_state->buffer = NULL;
> - return -ENOMEM;
> + ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
> + if (!ssd130x_state->buffer) {
> + kfree(ssd130x_state->data_array);
> + /* Set to prevent a double free in .atomic_destroy_state() */
> + ssd130x_state->data_array = NULL;
> + return -ENOMEM;
> + }
> }
>
> return 0;
> @@ -898,6 +923,7 @@ static const struct drm_mode_config_funcs ssd130x_mode_config_funcs = {
> };
>
> static const uint32_t ssd130x_formats[] = {
> + DRM_FORMAT_R1,
> DRM_FORMAT_XRGB8888,
> };
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)
Powered by blists - more mailing lists