[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d5c8ea70-8596-42a1-8688-0f6131187b73@redhat.com>
Date: Mon, 4 Nov 2024 10:56:27 +0100
From: Jocelyn Falempe <jfalempe@...hat.com>
To: Petr Mladek <pmladek@...e.com>
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>,
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>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 5/6] drm/log: Implement suspend/resume
On 01/11/2024 17:00, Petr Mladek wrote:
> On Fri 2024-10-25 11:46:16, Jocelyn Falempe wrote:
>> On 25/10/2024 01:11, Jocelyn Falempe wrote:
>>> On 24/10/2024 16:34, Petr Mladek wrote:
>>>> On Wed 2024-10-23 14:00:13, Jocelyn Falempe wrote:
>>>>> The console is already suspended in printk.c.
>>>>
>>>> Does this mean that drm_log_client_suspend() is called
>>>> after suspend_console(), please?
>>>
>>> To be honest, I wasn't able to tell which one is called first, and if
>>> the order is enforced (I didn't check if drivers can be suspended in
>>> parallel, or if it's all sequential)..
>>>
>>> I then checked if it's possible to suspend the console, but didn't found
>>> an easy API to do so, so I went with this lazy patch, just ensuring
>>> we're not writing to a suspended graphic driver.
>>
>> I've run some tests on my hardware, and the console is suspended before the
>> graphic driver:
>>
>> [ 56.409604] printk: Suspending console(s) (use no_console_suspend to
>> debug)
>> [ 56.411430] serial 00:05: disabled
>> [ 56.421877] sd 0:0:0:0: [sda] Synchronizing SCSI cache
>> [ 56.421954] sd 4:0:0:0: [sdb] Synchronizing SCSI cache
>> [ 56.422545] ata1.00: Entering standby power mode
>> [ 56.422793] DRM log suspend
>>
>> But because there is the "no_console_suspend" parameter, and we should make
>> sure to not draw when the graphic driver is suspended, I think this patch is
>> needed, and good enough.
>> I will just rephrase the commit message, to make it clear, that some message
>> won't be drawn, only if "no_console_suspend" is set.
>
> Ah, I forgot about the "no_console_suspend" parameter. The problem
> with this patch is that it would quietly drop all pending messages.
>
> drm_log_write_thread() does not have any return value.
> nbcon_emit_next_record() would assume that the message was printed.
> And the kthread would continue emitting next message...
>
> In compare, CON_SUSPENDED would cause that console_is_usable()
> returns false. As a result, nbcon_kthread_func() would not try
> to emit any message and go into a sleep.
>
> If we set CON_SUSPENDED then the pending messages will get printed
> after the resume. If we use this patch, the messages would get lost.
>
>
> This is why I am not happy with this patch. I would prefer to
> block the console. I see three better solutions:
>
> 1. Set CON_SUSPENDED from drm_log_client_suspend even when
> "no_console_suspend" is used.
>
> It is a bit dirty and might cause some confusion.
>
>
> 2. Add a new flag, e.g. CON_BLOCKED or CON_DRIVER_BLOCKED,
> which might be used for this purpose.
>
>
> 3. Allow con->write_thread() to return an error code.
>
> The question is how exactly the error should be handled.
> The kthread would not know when the printing might succeed
> again.
>
>
> I personally prefer the 2nd variant.
I looked at what serial drivers are doing, because they can also have
their clock gated in suspend.
Would calling console_stop() in the suspend and console_start() in
resume work ?
https://elixir.bootlin.com/linux/v6.11.6/source/drivers/tty/serial/serial_core.c#L2462
https://elixir.bootlin.com/linux/v6.11.6/source/kernel/printk/printk.c#L3323
It looks like it should do exactly what we need ?
Best regards,
--
Jocelyn
>
>
> Best Regards,
> Petr
>
> PS: I am sorry for the late reply. I had vacation...
>
Powered by blists - more mailing lists