01.10.2023 12:31, Laszlo Ersek пишет:
On 10/1/23 08:15, Mark Cave-Ayland wrote:
On 30/09/2023 22:28, Laszlo Ersek wrote:
On 9/29/23 09:57, Mark Cave-Ayland wrote:
On 26/09/2023 09:00, Marc-André Lureau wrote:
Hi Laszlo
On Mon, Sep 25, 2023 at 7:36 PM Laszlo Ersek <ler...@redhat.com> wrote:
Has this been queued by someone? Both Gerd and Marc-André are "odd
fixers", so I'm not sure who should be sending a PR with these patches
(and I don't see a pending PULL at
<https://lists.gnu.org/archive/html/qemu-devel/2023-09/threads.html>
with these patch subjects included).
I have the series in my "ui" branch. I was waiting for a few more
patches to be accumulated. But if someone else takes this first, I'll
drop them.
Does this series fix the "../ui/console.c:818: dpy_get_ui_info:
Assertion `dpy_ui_info_supported(con)' failed." assert() on startup when
using gtk? It would be good to get this fixed in git master soon, as it
has been broken for a couple of weeks now, and -display sdl has issues
tracking the mouse correctly on my laptop here :(
... probably not; I've never seen that issue. Can you provide a
reproducer?
The environment is a standard Debian bookworm install building QEMU git
master with QEMU gtk support. The only difference I can think of is that
I do all my QEMU builds as a separate user, and then export the display
to my current user desktop i.e.
As my current user:
$ xhost +
As my QEMU build user:
$ export DISPLAY=:1
$ ./build/qemu-system-sparc
qemu-system-sparc: ../ui/console.c:818: dpy_get_ui_info: Assertion
`dpy_ui_info_supported(con)' failed.
Aborted (core dumped)
Also, it should be bisectable (over Marc-André's 52-part series I guess).
Indeed. I've just run git bisect and it returns the following:
a92e7bb4cad57cc5c8817fb18fb25650507b69f8 is the first bad commit
commit a92e7bb4cad57cc5c8817fb18fb25650507b69f8
Author: Marc-André Lureau <marcandre.lur...@redhat.com>
Date: Tue Sep 12 10:13:01 2023 +0400
ui: add precondition for dpy_get_ui_info()
Ensure that it only get called when dpy_ui_info_supported(). The
function should always return a result. There should be a non-null
console or active_console.
Modify the argument to be const as well.
Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
Reviewed-by: Albert Esteve <aest...@redhat.com>
include/ui/console.h | 2 +-
ui/console.c | 4 +++-
2 files changed, 4 insertions(+), 2 deletions(-)
This is the first time the assertion failure has been found (right after
merge of this series, Sep-13):
https://lore.kernel.org/qemu-devel/801ac36e-b572-665b-5148-81baabf78...@tls.msk.ru/
This is more discussion about it:
https://lore.kernel.org/qemu-devel/0cae6d58-1476-9b92-0b48-f593b8e92...@tls.msk.ru/
This commit looks plain wrong to me; or rather I don't understand the
argument.
The thing here is that different parts of the code uses different object
thinking it is the same thing. So it is confusing at best and smells
wrong. But it is how it's been for quite some time, this new assert()
just revealed the issue, I think.
/mjt
In the particular crash, we fail in gtk_display_init -> gtk_widget_show
-> ... -> gd_configure -> gd_set_ui_size -> dpy_get_ui_info, and when
the latter calls dpy_ui_info_supported(), we find that
"con->hw_ops->ui_info" is NULL. In this particular case, "con->hw_ops"
is "vga_ops", and indeed "vga_ops" does not provide an "ui_info" funcptr.
SDL is unaffected because with SDL, we never call dpy_get_ui_info().
There's something fishy in the GTK display code BTW, in my opinion. I
can't quite put my finger on it, but commit aeffd071ed81 ("ui: Deliver
refresh rate via QemuUIInfo", 2022-06-14) definitely plays a role.
Before commit aeffd071ed81, "ui/gtk.c" wouldn't call dpy_get_ui_info()
either! Instead, from gd_configure(), we'd call gd_set_ui_info(),
directly setting the size from the incoming GdkEventConfigure object.
In commit aeffd071ed81, solely for the sake of carrying over the refresh
rate, gd_set_ui_info() was renamed to gd_set_ui_size(). The width and
height coming from the GdkEventConfigure object would be propagated the
same way to dpy_set_ui_info(), but the *rest* of the QemuUIInfo object
would be initialized differently. Before, the other fields would be
zero, now they'd come from dpy_get_ui_info() -- most likely for the sake
of carrying over the new refresh_rate field.
This in itself wouldn't crash, but it set up the call chain that is now
affected by the (IMO too strict) assertion.
Why is a hw_ops-based ui_info needed for dpy_get_ui_info()?
dpy_get_ui_info() never tries to *call* that function, it just returns
&con->ui_info. So dpy_get_ui_info() *already* guarantees that it returns
non-NULL.
Laszlo
ATB,
Mark.
