linux-kernel - Re: [PATCH v7 4/7] drm/log: Do not draw if drm

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: <bb6f5ff1-71c1-4b1b-a618-7f72310598f2@redhat.com>
Date: Fri, 8 Nov 2024 14:51:10 +0100
From: Jocelyn Falempe <jfalempe@...hat.com>
To: Thomas Zimmermann <tzimmermann@...e.de>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, David Airlie <airlied@...il.com>,
 Daniel Vetter <daniel@...ll.ch>, John Ogness <john.ogness@...utronix.de>,
 Javier Martinez Canillas <javierm@...hat.com>,
 "Guilherme G . Piccoli" <gpiccoli@...lia.com>,
 bluescreen_avenger@...izon.net, Caleb Connolly <caleb.connolly@...aro.org>,
 Petr Mladek <pmladek@...e.com>, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 4/7] drm/log: Do not draw if drm_master is taken

On 08/11/2024 14:33, Thomas Zimmermann wrote:
> Hi
> 
> Am 08.11.24 um 09:10 schrieb Jocelyn Falempe:
>> When userspace takes drm_master, the drm_client buffer is no more
>> visible, so drm_log shouldn't waste CPU cycle to draw on it.
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe@...hat.com>
>> ---
>>   drivers/gpu/drm/drm_log.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_log.c b/drivers/gpu/drm/drm_log.c
>> index 376ee173d99d9..226e206e8b6a3 100644
>> --- a/drivers/gpu/drm/drm_log.c
>> +++ b/drivers/gpu/drm/drm_log.c
>> @@ -18,6 +18,7 @@
>>   #include <drm/drm_print.h>
>>   #include "drm_draw.h"
>> +#include "drm_internal.h"
>>   #include "drm_log.h"
>>   MODULE_AUTHOR("Jocelyn Falempe");
>> @@ -308,8 +309,13 @@ static void drm_log_write_thread(struct console 
>> *con, struct nbcon_write_context
>>       if (!dlog->probed)
>>           drm_log_init_client(dlog);
>> -    for (i = 0; i < dlog->n_scanout; i++)
>> -        drm_log_draw_kmsg_record(&dlog->scanout[i], wctxt->outbuf, 
>> wctxt->len);
>> +    /* Check that we are still the master before drawing */
>> +    if (drm_master_internal_acquire(dlog->client.dev)) {
> 
> Just a note: it would be better to track this state in the client code 
> and handle these locks automatically. But it's ok for now. It would 
> require an overhaul of the fbdev helpers as well.
> 
>> +        drm_master_internal_release(dlog->client.dev);
> 
> Don't you have to release after drawing?

I'm not sure, the drawing code will only call 
drm_client_buffer_vmap_local() / unmap
and drm_client_framebuffer_flush(), and they don't require the master 
lock to be taken. I think master lock is needed only for modesetting.

Also the dlog->lock is taken by the console thread, so there are no race 
conditions with the drm_client callbacks (hotplug/suspend/resume).

> 
> Best regards
> Thomas
> 
> 
>> +
>> +        for (i = 0; i < dlog->n_scanout; i++)
>> +            drm_log_draw_kmsg_record(&dlog->scanout[i], wctxt- 
>> >outbuf, wctxt->len);
>> +    }
>>   }
>>   static void drm_log_lock(struct console *con, unsigned long *flags)
>