[PATCH v2 0/3] hw/display/bcm2835_fb: Remove DeviceReset() call in DeviceRealize()

[PATCH v2 0/3] hw/display/bcm2835_fb: Remove DeviceReset() call in DeviceRealize()

[Philippe Mathieu-Daudé]

Since v1:
Resize console on reset to create the surface, see:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg794307.html

Philippe Mathieu-Daudé (3):
  hw/display/bcm2835_fb: Resize console on reset
  hw/display/bcm2835_fb: Use bcm2835_fb_reconfigure in bcm2835_fb_reset
  hw/display/bcm2835_fb: Remove DeviceReset() call in DeviceRealize()

 hw/display/bcm2835_fb.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

-- 
2.26.2

[PATCH v2 1/3] hw/display/bcm2835_fb: Resize console on reset

[Philippe Mathieu-Daudé]

We want to remove the bcm2835_fb_reset() call in bcm2835_fb_realize()
but doing triggers:

  hw/display/bcm2835_fb.c:131:13: runtime error: store to null pointer of type 'uint32_t' (aka 'unsigned int')
  SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior hw/display/bcm2835_fb.c:131:13 in
  AddressSanitizer:DEADLYSIGNAL
  =================================================================
  ==195864==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x555d1d51e6d6 bp 0x7ffd25a31160 sp 0x7ffd25a30fb0 T0)
  ==195864==The signal is caused by a WRITE memory access.
  ==195864==Hint: address points to the zero page.
     #0 0x555d1d51e6d6 in draw_line_src16 hw/display/bcm2835_fb.c:131:30
     #1 0x555d1dd88d5f in framebuffer_update_display hw/display/framebuffer.c:107:13
     #2 0x555d1d51d081 in fb_update_display hw/display/bcm2835_fb.c:203:5
     #3 0x555d1ccb93d6 in graphic_hw_update ui/console.c:279:9
     #4 0x555d1dbc92cb in gd_refresh ui/gtk.c:492:5
     #5 0x555d1ccef1fc in dpy_refresh ui/console.c:1734:13
     #6 0x555d1ccee09c in gui_update ui/console.c:209:5
     #7 0x555d201f3cf2 in timerlist_run_timers util/qemu-timer.c:586:9
     #8 0x555d201f4061 in qemu_clock_run_timers util/qemu-timer.c:600:12
     #9 0x555d201f5029 in qemu_clock_run_all_timers util/qemu-timer.c:682:25
    #10 0x555d200c6f6c in main_loop_wait util/main-loop.c:541:5
    #11 0x555d1f06ba93 in qemu_main_loop softmmu/runstate.c:725:9
    #12 0x555d1cafe6ae in main softmmu/main.c:50:5
    #13 0x7f6e6991b081 in __libc_start_main (/lib64/libc.so.6+0x27081)
    #14 0x555d1ca249ed in _start (/mnt/scratch/qemu/sanitizer_aa64/qemu-system-aarch64+0x22999ed)

  AddressSanitizer can not provide additional info.
  SUMMARY: AddressSanitizer: SEGV hw/display/bcm2835_fb.c:131:30 in draw_line_src16
  ==195864==ABORTING

The graphic console timer kicks before the display device is realized.
By calling qemu_console_resize() in bcm2835_fb_reset() we force the
creation of the graphic console surface early enough.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/display/bcm2835_fb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
index 2be77bdd3a0..3e63d58e0b2 100644
--- a/hw/display/bcm2835_fb.c
+++ b/hw/display/bcm2835_fb.c
@@ -399,6 +399,7 @@ static void bcm2835_fb_reset(DeviceState *dev)
     s->config = s->initial_config;
 
     s->invalidate = true;
+    qemu_console_resize(s->con, s->initial_config.xres, s->initial_config.yres);
     s->lock = false;
 }
 
-- 
2.26.2

Re: [PATCH v2 1/3] hw/display/bcm2835_fb: Resize console on reset

[Peter Maydell]

On Tue, 23 Mar 2021 at 16:14, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> We want to remove the bcm2835_fb_reset() call in bcm2835_fb_realize()
> but doing triggers:
>
>   hw/display/bcm2835_fb.c:131:13: runtime error: store to null pointer of type 'uint32_t' (aka 'unsigned int')
>   SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior hw/display/bcm2835_fb.c:131:13 in
>   AddressSanitizer:DEADLYSIGNAL
>   =================================================================
>   ==195864==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x555d1d51e6d6 bp 0x7ffd25a31160 sp 0x7ffd25a30fb0 T0)
>   ==195864==The signal is caused by a WRITE memory access.
>   ==195864==Hint: address points to the zero page.
>      #0 0x555d1d51e6d6 in draw_line_src16 hw/display/bcm2835_fb.c:131:30
>      #1 0x555d1dd88d5f in framebuffer_update_display hw/display/framebuffer.c:107:13
>      #2 0x555d1d51d081 in fb_update_display hw/display/bcm2835_fb.c:203:5
>      #3 0x555d1ccb93d6 in graphic_hw_update ui/console.c:279:9
>      #4 0x555d1dbc92cb in gd_refresh ui/gtk.c:492:5
>      #5 0x555d1ccef1fc in dpy_refresh ui/console.c:1734:13
>      #6 0x555d1ccee09c in gui_update ui/console.c:209:5
>      #7 0x555d201f3cf2 in timerlist_run_timers util/qemu-timer.c:586:9
>      #8 0x555d201f4061 in qemu_clock_run_timers util/qemu-timer.c:600:12
>      #9 0x555d201f5029 in qemu_clock_run_all_timers util/qemu-timer.c:682:25
>     #10 0x555d200c6f6c in main_loop_wait util/main-loop.c:541:5
>     #11 0x555d1f06ba93 in qemu_main_loop softmmu/runstate.c:725:9
>     #12 0x555d1cafe6ae in main softmmu/main.c:50:5
>     #13 0x7f6e6991b081 in __libc_start_main (/lib64/libc.so.6+0x27081)
>     #14 0x555d1ca249ed in _start (/mnt/scratch/qemu/sanitizer_aa64/qemu-system-aarch64+0x22999ed)
>
>   AddressSanitizer can not provide additional info.
>   SUMMARY: AddressSanitizer: SEGV hw/display/bcm2835_fb.c:131:30 in draw_line_src16
>   ==195864==ABORTING
>
> The graphic console timer kicks before the display device is realized.
> By calling qemu_console_resize() in bcm2835_fb_reset() we force the
> creation of the graphic console surface early enough.
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/display/bcm2835_fb.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
> index 2be77bdd3a0..3e63d58e0b2 100644
> --- a/hw/display/bcm2835_fb.c
> +++ b/hw/display/bcm2835_fb.c
> @@ -399,6 +399,7 @@ static void bcm2835_fb_reset(DeviceState *dev)
>      s->config = s->initial_config;
>
>      s->invalidate = true;
> +    qemu_console_resize(s->con, s->initial_config.xres, s->initial_config.yres);
>      s->lock = false;
>  }

I don't really understand how the commit message and the code
change relate. reset happens after realize, and realize
already calls qemu_console_resize(), so how can adding a
call to resize here in reset cause the console surface to
be created any earlier than it already is ?

I also don't understand how the GUI timer can call us before
the device is realized, given that we only register ourselves
via graphics_console_init() in the device realize.

More generally, I think we should probably start by figuring out
what the requirements on graphics devices vs the UI layer
are or should be. Is it possible to get the UI layer to
not start calling into graphics devices until after the
system has been reset for the first time, for instance?
Gerd, do you have any views here ?

thanks
-- PMM

Re: [PATCH v2 1/3] hw/display/bcm2835_fb: Resize console on reset

[Philippe Mathieu-Daudé]

On 3/26/21 3:27 PM, Peter Maydell wrote:
> On Tue, 23 Mar 2021 at 16:14, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> We want to remove the bcm2835_fb_reset() call in bcm2835_fb_realize()
>> but doing triggers:
>>
>>   hw/display/bcm2835_fb.c:131:13: runtime error: store to null pointer of type 'uint32_t' (aka 'unsigned int')
>>   SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior hw/display/bcm2835_fb.c:131:13 in
>>   AddressSanitizer:DEADLYSIGNAL
>>   =================================================================
>>   ==195864==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x555d1d51e6d6 bp 0x7ffd25a31160 sp 0x7ffd25a30fb0 T0)
>>   ==195864==The signal is caused by a WRITE memory access.
>>   ==195864==Hint: address points to the zero page.
>>      #0 0x555d1d51e6d6 in draw_line_src16 hw/display/bcm2835_fb.c:131:30
>>      #1 0x555d1dd88d5f in framebuffer_update_display hw/display/framebuffer.c:107:13
>>      #2 0x555d1d51d081 in fb_update_display hw/display/bcm2835_fb.c:203:5
>>      #3 0x555d1ccb93d6 in graphic_hw_update ui/console.c:279:9
>>      #4 0x555d1dbc92cb in gd_refresh ui/gtk.c:492:5
>>      #5 0x555d1ccef1fc in dpy_refresh ui/console.c:1734:13
>>      #6 0x555d1ccee09c in gui_update ui/console.c:209:5
>>      #7 0x555d201f3cf2 in timerlist_run_timers util/qemu-timer.c:586:9
>>      #8 0x555d201f4061 in qemu_clock_run_timers util/qemu-timer.c:600:12
>>      #9 0x555d201f5029 in qemu_clock_run_all_timers util/qemu-timer.c:682:25
>>     #10 0x555d200c6f6c in main_loop_wait util/main-loop.c:541:5
>>     #11 0x555d1f06ba93 in qemu_main_loop softmmu/runstate.c:725:9
>>     #12 0x555d1cafe6ae in main softmmu/main.c:50:5
>>     #13 0x7f6e6991b081 in __libc_start_main (/lib64/libc.so.6+0x27081)
>>     #14 0x555d1ca249ed in _start (/mnt/scratch/qemu/sanitizer_aa64/qemu-system-aarch64+0x22999ed)
>>
>>   AddressSanitizer can not provide additional info.
>>   SUMMARY: AddressSanitizer: SEGV hw/display/bcm2835_fb.c:131:30 in draw_line_src16
>>   ==195864==ABORTING
>>
>> The graphic console timer kicks before the display device is realized.
>> By calling qemu_console_resize() in bcm2835_fb_reset() we force the
>> creation of the graphic console surface early enough.
>>
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/display/bcm2835_fb.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
>> index 2be77bdd3a0..3e63d58e0b2 100644
>> --- a/hw/display/bcm2835_fb.c
>> +++ b/hw/display/bcm2835_fb.c
>> @@ -399,6 +399,7 @@ static void bcm2835_fb_reset(DeviceState *dev)
>>      s->config = s->initial_config;
>>
>>      s->invalidate = true;
>> +    qemu_console_resize(s->con, s->initial_config.xres, s->initial_config.yres);
>>      s->lock = false;
>>  }
> 
> I don't really understand how the commit message and the code
> change relate.

The commit message is inconsistent, indeed. I tried to justify
the next patch.

> reset happens after realize,

Yes, so this patch is incorrect.

> and realize
> already calls qemu_console_resize(), so how can adding a
> call to resize here in reset cause the console surface to
> be created any earlier than it already is ?
> 
> I also don't understand how the GUI timer can call us before
> the device is realized, given that we only register ourselves
> via graphics_console_init() in the device realize.
> 
> More generally, I think we should probably start by figuring out
> what the requirements on graphics devices vs the UI layer
> are or should be.

Agreed.

> Is it possible to get the UI layer to
> not start calling into graphics devices until after the
> system has been reset for the first time, for instance?

I have no clue, so deferring to Gerd.

> Gerd, do you have any views here ?
> 
> thanks
> -- PMM
> 

Re: [PATCH v2 1/3] hw/display/bcm2835_fb: Resize console on reset

[Gerd Hoffmann]

  Hi,

> > and realize
> > already calls qemu_console_resize(), so how can adding a
> > call to resize here in reset cause the console surface to
> > be created any earlier than it already is ?
> > 
> > I also don't understand how the GUI timer can call us before
> > the device is realized, given that we only register ourselves
> > via graphics_console_init() in the device realize.

Yes, should not happen.  Also note that graphics_console_init()
creates a surface.

> > Is it possible to get the UI layer to
> > not start calling into graphics devices until after the
> > system has been reset for the first time, for instance?

Can that actually happen?  I don't think it could in the past,
but maybe now with the initialization changes.  We can add a
check to gui_update() ...

take care,
  Gerd

Re: [PATCH v2 1/3] hw/display/bcm2835_fb: Resize console on reset

[Philippe Mathieu-Daudé]

Hi Gerd,

On 3/29/21 7:59 AM, Gerd Hoffmann wrote:
>>> and realize
>>> already calls qemu_console_resize(), so how can adding a
>>> call to resize here in reset cause the console surface to
>>> be created any earlier than it already is ?
>>>
>>> I also don't understand how the GUI timer can call us before
>>> the device is realized, given that we only register ourselves
>>> via graphics_console_init() in the device realize.
> 
> Yes, should not happen.  Also note that graphics_console_init()
> creates a surface.
> 
>>> Is it possible to get the UI layer to
>>> not start calling into graphics devices until after the
>>> system has been reset for the first time, for instance?
> 
> Can that actually happen?  I don't think it could in the past,
> but maybe now with the initialization changes.  We can add a
> check to gui_update() ...

Do you mind sending a patch?

Thanks,

Phil.

[PATCH v2 2/3] hw/display/bcm2835_fb: Use bcm2835_fb_reconfigure in bcm2835_fb_reset

[Philippe Mathieu-Daudé]

Directly use bcm2835_fb_reconfigure() instead of open coding it.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/display/bcm2835_fb.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
index 3e63d58e0b2..a9c2e57d1c6 100644
--- a/hw/display/bcm2835_fb.c
+++ b/hw/display/bcm2835_fb.c
@@ -396,11 +396,7 @@ static void bcm2835_fb_reset(DeviceState *dev)
 
     s->pending = false;
 
-    s->config = s->initial_config;
-
-    s->invalidate = true;
-    qemu_console_resize(s->con, s->initial_config.xres, s->initial_config.yres);
-    s->lock = false;
+    bcm2835_fb_reconfigure(s, &s->initial_config);
 }
 
 static void bcm2835_fb_realize(DeviceState *dev, Error **errp)
-- 
2.26.2

[PATCH v2 3/3] hw/display/bcm2835_fb: Remove DeviceReset() call in DeviceRealize()

[Philippe Mathieu-Daudé]

When QDev objects have their DeviceReset handler set, they
shouldn't worry about calling it at realization stage (it
is handled by hw/core/qdev.c::device_set_realized).

Remove the pointless/confusing bcm2835_fb_reset() call.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/display/bcm2835_fb.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
index a9c2e57d1c6..d7a44771c44 100644
--- a/hw/display/bcm2835_fb.c
+++ b/hw/display/bcm2835_fb.c
@@ -421,8 +421,6 @@ static void bcm2835_fb_realize(DeviceState *dev, Error **errp)
     s->dma_mr = MEMORY_REGION(obj);
     address_space_init(&s->dma_as, s->dma_mr, TYPE_BCM2835_FB "-memory");
 
-    bcm2835_fb_reset(dev);
-
     s->con = graphic_console_init(dev, 0, &vgafb_ops, s);
     qemu_console_resize(s->con, s->config.xres, s->config.yres);
 }
-- 
2.26.2