Hi Am 27.04.21 um 08:18 schrieb Takashi Iwai: > On Mon, 26 Apr 2021 20:44:55 +0200, > Thomas Zimmermann wrote: >> >> Hi >> >> Am 21.04.21 um 10:08 schrieb Takashi Iwai: >>> On bochs DRM driver, the execution of "setterm --blank force" results >>> in a frozen screen instead of a blank screen. It's due to the lack of >>> the screen blanking support in its code. >>> >>> Actually, the QEMU bochs vga side can switch to the blanking mode when >>> the bit 0x20 is cleared on VGA_ATT_IW register (0x3c0), which updates >>> ar_index in QEMU side. So, essentially, we'd just need to clear the >>> bit at pipe disable callback; that's what this patch does essentially. >>> >>> However, a tricky part is that the access via VGA_ATT_IW is done in >>> "flip-flop"; the first write is for index and the second write is for >>> the data like palette. Meanwhile, in the current bochs DRM driver, >>> the flip-flop wasn't considered, and it calls only the register update >>> once with the value 0x20. >> >> I read up on the details of what the attribute registers do and what >> you're modifying is the PAS field in the attribute index register. It >> controls write access to the attribute fields. >> >> >> https://web.stanford.edu/class/cs140/projects/pintos/specs/freevga/vga/attrreg.htm#3C0 >> >> It's located in the index register and cleared while >> attributes/palettes are updated. I guess that in this mode the stdvga >> disables the palette entirely (hence the screen turns dark). >> >> While it works, it feels wrong to do this. >> >> I to do blanking/unblanking with the SR field in SEQ0 >> >> >> https://web.stanford.edu/class/cs140/projects/pintos/specs/freevga/vga/seqreg.htm#00 >> >> That's what drivers usually do AFAICT. I think the 'unblank' comment >> next to the existing code might be misleading. > > Yeah, when you look at the existing vga16fb.c in kernel, we can find a > relatively complex blanking procedure. OTOH, what I changed is rather > based on the the actual behavior of bochs is more or less simplified. > QEMU hw/display/vga.c contains a code like: > > static void vga_update_display(void *opaque) > { > VGACommonState *s = opaque; > DisplaySurface *surface = qemu_console_surface(s->con); > int full_update, graphic_mode; > > qemu_flush_coalesced_mmio_buffer(); > > if (surface_bits_per_pixel(surface) == 0) { > /* nothing to do */ > } else { > full_update = 0; > if (!(s->ar_index & 0x20)) { > graphic_mode = GMODE_BLANK; > } else { > graphic_mode = s->gr[VGA_GFX_MISC] & VGA_GR06_GRAPHICS_MODE; > } > if (graphic_mode != s->graphic_mode) { > s->graphic_mode = graphic_mode; > s->cursor_blink_time = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL); > full_update = 1; > } > switch(graphic_mode) { > case GMODE_TEXT: > vga_draw_text(s, full_update); > break; > case GMODE_GRAPH: > vga_draw_graphic(s, full_update); > break; > case GMODE_BLANK: > default: > vga_draw_blank(s, full_update); > break; > } > } > } > > So, it simply checks the ar_index () at each update and switches > from/to the blank mode depending on the bit 0x20. > > I'm fine to change in any better way, of course, so feel free to > modify the patch. If no one objects, I'll merge it as-is. It's somewhat wrong wrt to VGA, but apparently what qemu wants. Best regards Thomas > > > thanks, > > Takashi > >> >> Best regards >> Thomas >> >>> >>> The spec and the actual VGA implementation in QEMU suggests that the >>> flip flop flag is discarded by reading the CRTC index register >>> (VGA_IS1_RC, 0x3da). So, in this patch, we add the helper to read a >>> byte and the call to clear the flip flop flag before changing the >>> blank / unblank setup via VGA_ATT_IW register. >>> >>> v1->v2: >>> * discard ar_flip_flop by reading 0x3da, add bochs_vga_readb() >>> * include video/vga.h for VGA register definitions >>> * move the blank/unblank code to bochs_hw_blank() >>> >>> Signed-off-by: Takashi Iwai >>> --- >>> drivers/gpu/drm/bochs/bochs.h | 1 + >>> drivers/gpu/drm/bochs/bochs_hw.c | 25 ++++++++++++++++++++++++- >>> drivers/gpu/drm/bochs/bochs_kms.c | 8 ++++++++ >>> 3 files changed, 33 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h >>> index e5bd1d517a18..e9645c612aff 100644 >>> --- a/drivers/gpu/drm/bochs/bochs.h >>> +++ b/drivers/gpu/drm/bochs/bochs.h >>> @@ -78,6 +78,7 @@ struct bochs_device { >>> int bochs_hw_init(struct drm_device *dev); >>> void bochs_hw_fini(struct drm_device *dev); >>> +void bochs_hw_blank(struct bochs_device *bochs, bool blank); >>> void bochs_hw_setmode(struct bochs_device *bochs, >>> struct drm_display_mode *mode); >>> void bochs_hw_setformat(struct bochs_device *bochs, >>> diff --git a/drivers/gpu/drm/bochs/bochs_hw.c b/drivers/gpu/drm/bochs/bochs_hw.c >>> index 2d7380a9890e..7d3426d8cc69 100644 >>> --- a/drivers/gpu/drm/bochs/bochs_hw.c >>> +++ b/drivers/gpu/drm/bochs/bochs_hw.c >>> @@ -7,6 +7,7 @@ >>> #include >>> #include >>> +#include