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]

Message-ID: <875y6macxu.fsf@minerva.mail-host-address-is-not-set>
Date:   Fri, 14 Jul 2023 14:35:09 +0200
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     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>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

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

Hello Geert,

> Hi Javier,
>
> On Fri, Jul 14, 2023 at 12:14 PM Javier Martinez Canillas
> <javierm@...hat.com> wrote:
>> Geert Uytterhoeven <geert@...ux-m68k.org> writes:
>> 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 ?
>
> I did mean R1. I think you missed the double negation.
>

I did indeed. As a non-native english speaker, I find it very hard to
parse double negations :)

>> It's still used in ssd130x_clear_screen() though.
>
> I guess it became worthwhile to make ssd130x_clear_screen()
> do memset(data_array, 0, ...) and call ssd130x_write_data() directly,
> avoiding the pointless reshuffling of black pixels in
> ssd130x_update_rect()?
>

I think so, yeah.

>> > 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().
>
> The display should not be updated after .remove(), so I think plain
> devm_kcalloc() should be fine.
>

That was precisely my point, that there could be atomic commits even after
the driver has been removed (e.g: if using DRM fbdev emulation, user-space
can keep the /dev/fb0 opened and continue updating the framebuffer. That's
not released until the fd is closed and struct fb_ops .fb_destroy called.

But that's a general rule in DRM, any user-visible resource must not be
allocated using device managed resources and instead use the drm_device
managed resources helpers. To make sure that are not released until the
last call to drm_dev_put():

https://docs.kernel.org/gpu/drm-internals.html#device-instance-and-driver-handling

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat