linux-kernel - Re: [PATCH 4/8] drm/ssd130x: Add support for DRM_FORMAT

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 14 Jul 2023 12:14:44 +0200
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>
Cc:     dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        Geert Uytterhoeven <geert@...ux-m68k.org>
Subject: Re: [PATCH 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

Geert Uytterhoeven <geert@...ux-m68k.org> writes:

Hello Geert,

Thanks a lot for your patch, this has been on my TODO for some time!

> The native display format is monochrome light-on-dark (R1).
> Hence add support for R1, so monochrome applications can avoid the
> overhead of back-and-forth conversions between R1 and XR24.
>
> Signed-off-by: Geert Uytterhoeven <geert@...ux-m68k.org>
> ---
> This work interfered with commit 49d7d581ceaf4cf8 ("drm/ssd130x: Don't
> allocate buffers on each plane update") in drm-misc/for-linux-next,
> which always allocates the buffer upfront, while it is no longer needed
> when never using XR24.
>

you mean R1 here, right ? It's still used in ssd130x_clear_screen() though.

> Probably ssd130x->buffer should be allocated on first use.

Yes, that makes sense.

> And why not allocate the buffers using devm_kcalloc()?

I think there are some lifetimes discrepancies between struct device and
struct drm_device objects. But we could use drm_device managed resources
helpers, i.e: drmm_kzalloc().

> ---
>  drivers/gpu/drm/solomon/ssd130x.c | 57 ++++++++++++++++++++++---------
>  1 file changed, 40 insertions(+), 17 deletions(-)
>

[...]

> +	case DRM_FORMAT_XRGB8888:
> +		dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
> +		buf = ssd130x->buffer;
> +		if (!buf)
> +			return 0;
> +

I think this check is not needed anymore now that the driver won't attempt
to update planes for disabled CRTCs ?

It's OK for me to be paranoid though, specially after the other issue that
you found. So I'll let you decide if you think is worth to keep the check.

Reviewed-by: Javier Martinez Canillas <javierm@...hat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat