* [PATCH v2] drm/bochs: Add screen blanking support @ 2021-04-21 8:08 Takashi Iwai 2021-04-26 18:44 ` Thomas Zimmermann 0 siblings, 1 reply; 6+ messages in thread From: Takashi Iwai @ 2021-04-21 8:08 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: dri-devel, Thomas Zimmermann, virtualization 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. 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 <tiwai@suse.de> --- 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 <drm/drm_drv.h> #include <drm/drm_fourcc.h> +#include <video/vga.h> #include "bochs.h" /* ---------------------------------------------------------------------- */ @@ -24,6 +25,19 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val) } } +static u8 bochs_vga_readb(struct bochs_device *bochs, u16 ioport) +{ + if (WARN_ON(ioport < 0x3c0 || ioport > 0x3df)) + return 0xff; + + if (bochs->mmio) { + int offset = ioport - 0x3c0 + 0x400; + return readb(bochs->mmio + offset); + } else { + return inb(ioport); + } +} + static u16 bochs_dispi_read(struct bochs_device *bochs, u16 reg) { u16 ret = 0; @@ -205,6 +219,15 @@ void bochs_hw_fini(struct drm_device *dev) kfree(bochs->edid); } +void bochs_hw_blank(struct bochs_device *bochs, bool blank) +{ + DRM_DEBUG_DRIVER("hw_blank %d\n", blank); + /* discard ar_flip_flop */ + (void)bochs_vga_readb(bochs, VGA_IS1_RC); + /* blank or unblank; we need only update index and set 0x20 */ + bochs_vga_writeb(bochs, VGA_ATT_W, blank ? 0 : 0x20); +} + void bochs_hw_setmode(struct bochs_device *bochs, struct drm_display_mode *mode) { @@ -223,7 +246,7 @@ void bochs_hw_setmode(struct bochs_device *bochs, bochs->xres, bochs->yres, bochs->bpp, bochs->yres_virtual); - bochs_vga_writeb(bochs, 0x3c0, 0x20); /* unblank */ + bochs_hw_blank(bochs, false); bochs_dispi_write(bochs, VBE_DISPI_INDEX_ENABLE, 0); bochs_dispi_write(bochs, VBE_DISPI_INDEX_BPP, bochs->bpp); diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c index 853081d186d5..99410e77d51a 100644 --- a/drivers/gpu/drm/bochs/bochs_kms.c +++ b/drivers/gpu/drm/bochs/bochs_kms.c @@ -57,6 +57,13 @@ static void bochs_pipe_enable(struct drm_simple_display_pipe *pipe, bochs_plane_update(bochs, plane_state); } +static void bochs_pipe_disable(struct drm_simple_display_pipe *pipe) +{ + struct bochs_device *bochs = pipe->crtc.dev->dev_private; + + bochs_hw_blank(bochs, true); +} + static void bochs_pipe_update(struct drm_simple_display_pipe *pipe, struct drm_plane_state *old_state) { @@ -67,6 +74,7 @@ static void bochs_pipe_update(struct drm_simple_display_pipe *pipe, static const struct drm_simple_display_pipe_funcs bochs_pipe_funcs = { .enable = bochs_pipe_enable, + .disable = bochs_pipe_disable, .update = bochs_pipe_update, .prepare_fb = drm_gem_vram_simple_display_pipe_prepare_fb, .cleanup_fb = drm_gem_vram_simple_display_pipe_cleanup_fb, -- 2.26.2 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/bochs: Add screen blanking support 2021-04-21 8:08 [PATCH v2] drm/bochs: Add screen blanking support Takashi Iwai @ 2021-04-26 18:44 ` Thomas Zimmermann 2021-04-27 6:18 ` Takashi Iwai 0 siblings, 1 reply; 6+ messages in thread From: Thomas Zimmermann @ 2021-04-26 18:44 UTC (permalink / raw) To: Takashi Iwai, Gerd Hoffmann; +Cc: dri-devel, virtualization [-- Attachment #1.1.1: Type: text/plain, Size: 6271 bytes --] 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. 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 <tiwai@suse.de> > --- > 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 <drm/drm_drv.h> > #include <drm/drm_fourcc.h> > > +#include <video/vga.h> > #include "bochs.h" > > /* ---------------------------------------------------------------------- */ > @@ -24,6 +25,19 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val) > } > } > > +static u8 bochs_vga_readb(struct bochs_device *bochs, u16 ioport) > +{ > + if (WARN_ON(ioport < 0x3c0 || ioport > 0x3df)) > + return 0xff; > + > + if (bochs->mmio) { > + int offset = ioport - 0x3c0 + 0x400; > + return readb(bochs->mmio + offset); > + } else { > + return inb(ioport); > + } > +} > + > static u16 bochs_dispi_read(struct bochs_device *bochs, u16 reg) > { > u16 ret = 0; > @@ -205,6 +219,15 @@ void bochs_hw_fini(struct drm_device *dev) > kfree(bochs->edid); > } > > +void bochs_hw_blank(struct bochs_device *bochs, bool blank) > +{ > + DRM_DEBUG_DRIVER("hw_blank %d\n", blank); > + /* discard ar_flip_flop */ > + (void)bochs_vga_readb(bochs, VGA_IS1_RC); > + /* blank or unblank; we need only update index and set 0x20 */ > + bochs_vga_writeb(bochs, VGA_ATT_W, blank ? 0 : 0x20); > +} > + > void bochs_hw_setmode(struct bochs_device *bochs, > struct drm_display_mode *mode) > { > @@ -223,7 +246,7 @@ void bochs_hw_setmode(struct bochs_device *bochs, > bochs->xres, bochs->yres, bochs->bpp, > bochs->yres_virtual); > > - bochs_vga_writeb(bochs, 0x3c0, 0x20); /* unblank */ > + bochs_hw_blank(bochs, false); > > bochs_dispi_write(bochs, VBE_DISPI_INDEX_ENABLE, 0); > bochs_dispi_write(bochs, VBE_DISPI_INDEX_BPP, bochs->bpp); > diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c > index 853081d186d5..99410e77d51a 100644 > --- a/drivers/gpu/drm/bochs/bochs_kms.c > +++ b/drivers/gpu/drm/bochs/bochs_kms.c > @@ -57,6 +57,13 @@ static void bochs_pipe_enable(struct drm_simple_display_pipe *pipe, > bochs_plane_update(bochs, plane_state); > } > > +static void bochs_pipe_disable(struct drm_simple_display_pipe *pipe) > +{ > + struct bochs_device *bochs = pipe->crtc.dev->dev_private; > + > + bochs_hw_blank(bochs, true); > +} > + > static void bochs_pipe_update(struct drm_simple_display_pipe *pipe, > struct drm_plane_state *old_state) > { > @@ -67,6 +74,7 @@ static void bochs_pipe_update(struct drm_simple_display_pipe *pipe, > > static const struct drm_simple_display_pipe_funcs bochs_pipe_funcs = { > .enable = bochs_pipe_enable, > + .disable = bochs_pipe_disable, > .update = bochs_pipe_update, > .prepare_fb = drm_gem_vram_simple_display_pipe_prepare_fb, > .cleanup_fb = drm_gem_vram_simple_display_pipe_cleanup_fb, > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 840 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/bochs: Add screen blanking support 2021-04-26 18:44 ` Thomas Zimmermann @ 2021-04-27 6:18 ` Takashi Iwai 2021-04-27 6:58 ` Thomas Zimmermann 0 siblings, 1 reply; 6+ messages in thread From: Takashi Iwai @ 2021-04-27 6:18 UTC (permalink / raw) To: Thomas Zimmermann; +Cc: dri-devel, Gerd Hoffmann, virtualization 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. 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 <tiwai@suse.de> > > --- > > 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 <drm/drm_drv.h> > > #include <drm/drm_fourcc.h> > > +#include <video/vga.h> > > #include "bochs.h" > > /* > > ---------------------------------------------------------------------- > > */ > > @@ -24,6 +25,19 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val) > > } > > } > > +static u8 bochs_vga_readb(struct bochs_device *bochs, u16 ioport) > > +{ > > + if (WARN_ON(ioport < 0x3c0 || ioport > 0x3df)) > > + return 0xff; > > + > > + if (bochs->mmio) { > > + int offset = ioport - 0x3c0 + 0x400; > > + return readb(bochs->mmio + offset); > > + } else { > > + return inb(ioport); > > + } > > +} > > + > > static u16 bochs_dispi_read(struct bochs_device *bochs, u16 reg) > > { > > u16 ret = 0; > > @@ -205,6 +219,15 @@ void bochs_hw_fini(struct drm_device *dev) > > kfree(bochs->edid); > > } > > +void bochs_hw_blank(struct bochs_device *bochs, bool blank) > > +{ > > + DRM_DEBUG_DRIVER("hw_blank %d\n", blank); > > + /* discard ar_flip_flop */ > > + (void)bochs_vga_readb(bochs, VGA_IS1_RC); > > + /* blank or unblank; we need only update index and set 0x20 */ > > + bochs_vga_writeb(bochs, VGA_ATT_W, blank ? 0 : 0x20); > > +} > > + > > void bochs_hw_setmode(struct bochs_device *bochs, > > struct drm_display_mode *mode) > > { > > @@ -223,7 +246,7 @@ void bochs_hw_setmode(struct bochs_device *bochs, > > bochs->xres, bochs->yres, bochs->bpp, > > bochs->yres_virtual); > > - bochs_vga_writeb(bochs, 0x3c0, 0x20); /* unblank */ > > + bochs_hw_blank(bochs, false); > > bochs_dispi_write(bochs, VBE_DISPI_INDEX_ENABLE, 0); > > bochs_dispi_write(bochs, VBE_DISPI_INDEX_BPP, bochs->bpp); > > diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c > > index 853081d186d5..99410e77d51a 100644 > > --- a/drivers/gpu/drm/bochs/bochs_kms.c > > +++ b/drivers/gpu/drm/bochs/bochs_kms.c > > @@ -57,6 +57,13 @@ static void bochs_pipe_enable(struct drm_simple_display_pipe *pipe, > > bochs_plane_update(bochs, plane_state); > > } > > +static void bochs_pipe_disable(struct drm_simple_display_pipe > > *pipe) > > +{ > > + struct bochs_device *bochs = pipe->crtc.dev->dev_private; > > + > > + bochs_hw_blank(bochs, true); > > +} > > + > > static void bochs_pipe_update(struct drm_simple_display_pipe *pipe, > > struct drm_plane_state *old_state) > > { > > @@ -67,6 +74,7 @@ static void bochs_pipe_update(struct drm_simple_display_pipe *pipe, > > static const struct drm_simple_display_pipe_funcs > > bochs_pipe_funcs = > { > > .enable = bochs_pipe_enable, > > + .disable = bochs_pipe_disable, > > .update = bochs_pipe_update, > > .prepare_fb = drm_gem_vram_simple_display_pipe_prepare_fb, > > .cleanup_fb = drm_gem_vram_simple_display_pipe_cleanup_fb, > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer > > [2 OpenPGP digital signature <application/pgp-signature (7bit)>] > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/bochs: Add screen blanking support 2021-04-27 6:18 ` Takashi Iwai @ 2021-04-27 6:58 ` Thomas Zimmermann 2021-04-27 9:56 ` Gerd Hoffmann 0 siblings, 1 reply; 6+ messages in thread From: Thomas Zimmermann @ 2021-04-27 6:58 UTC (permalink / raw) To: Takashi Iwai; +Cc: Gerd Hoffmann, dri-devel, virtualization [-- Attachment #1.1.1: Type: text/plain, Size: 9043 bytes --] 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 <tiwai@suse.de> >>> --- >>> 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 <drm/drm_drv.h> >>> #include <drm/drm_fourcc.h> >>> +#include <video/vga.h> >>> #include "bochs.h" >>> /* >>> ---------------------------------------------------------------------- >>> */ >>> @@ -24,6 +25,19 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val) >>> } >>> } >>> +static u8 bochs_vga_readb(struct bochs_device *bochs, u16 ioport) >>> +{ >>> + if (WARN_ON(ioport < 0x3c0 || ioport > 0x3df)) >>> + return 0xff; >>> + >>> + if (bochs->mmio) { >>> + int offset = ioport - 0x3c0 + 0x400; >>> + return readb(bochs->mmio + offset); >>> + } else { >>> + return inb(ioport); >>> + } >>> +} >>> + >>> static u16 bochs_dispi_read(struct bochs_device *bochs, u16 reg) >>> { >>> u16 ret = 0; >>> @@ -205,6 +219,15 @@ void bochs_hw_fini(struct drm_device *dev) >>> kfree(bochs->edid); >>> } >>> +void bochs_hw_blank(struct bochs_device *bochs, bool blank) >>> +{ >>> + DRM_DEBUG_DRIVER("hw_blank %d\n", blank); >>> + /* discard ar_flip_flop */ >>> + (void)bochs_vga_readb(bochs, VGA_IS1_RC); >>> + /* blank or unblank; we need only update index and set 0x20 */ >>> + bochs_vga_writeb(bochs, VGA_ATT_W, blank ? 0 : 0x20); >>> +} >>> + >>> void bochs_hw_setmode(struct bochs_device *bochs, >>> struct drm_display_mode *mode) >>> { >>> @@ -223,7 +246,7 @@ void bochs_hw_setmode(struct bochs_device *bochs, >>> bochs->xres, bochs->yres, bochs->bpp, >>> bochs->yres_virtual); >>> - bochs_vga_writeb(bochs, 0x3c0, 0x20); /* unblank */ >>> + bochs_hw_blank(bochs, false); >>> bochs_dispi_write(bochs, VBE_DISPI_INDEX_ENABLE, 0); >>> bochs_dispi_write(bochs, VBE_DISPI_INDEX_BPP, bochs->bpp); >>> diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c >>> index 853081d186d5..99410e77d51a 100644 >>> --- a/drivers/gpu/drm/bochs/bochs_kms.c >>> +++ b/drivers/gpu/drm/bochs/bochs_kms.c >>> @@ -57,6 +57,13 @@ static void bochs_pipe_enable(struct drm_simple_display_pipe *pipe, >>> bochs_plane_update(bochs, plane_state); >>> } >>> +static void bochs_pipe_disable(struct drm_simple_display_pipe >>> *pipe) >>> +{ >>> + struct bochs_device *bochs = pipe->crtc.dev->dev_private; >>> + >>> + bochs_hw_blank(bochs, true); >>> +} >>> + >>> static void bochs_pipe_update(struct drm_simple_display_pipe *pipe, >>> struct drm_plane_state *old_state) >>> { >>> @@ -67,6 +74,7 @@ static void bochs_pipe_update(struct drm_simple_display_pipe *pipe, >>> static const struct drm_simple_display_pipe_funcs >>> bochs_pipe_funcs = >> { >>> .enable = bochs_pipe_enable, >>> + .disable = bochs_pipe_disable, >>> .update = bochs_pipe_update, >>> .prepare_fb = drm_gem_vram_simple_display_pipe_prepare_fb, >>> .cleanup_fb = drm_gem_vram_simple_display_pipe_cleanup_fb, >>> >> >> -- >> Thomas Zimmermann >> Graphics Driver Developer >> SUSE Software Solutions Germany GmbH >> Maxfeldstr. 5, 90409 Nürnberg, Germany >> (HRB 36809, AG Nürnberg) >> Geschäftsführer: Felix Imendörffer >> >> [2 OpenPGP digital signature <application/pgp-signature (7bit)>] >> > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 840 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/bochs: Add screen blanking support 2021-04-27 6:58 ` Thomas Zimmermann @ 2021-04-27 9:56 ` Gerd Hoffmann 2021-04-27 12:21 ` Thomas Zimmermann 0 siblings, 1 reply; 6+ messages in thread From: Gerd Hoffmann @ 2021-04-27 9:56 UTC (permalink / raw) To: Thomas Zimmermann; +Cc: dri-devel, virtualization > > 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. No objections. Acked-by: Gerd Hoffmann <kraxel@redhat.com> FYI: cirrus is in the same situation, the modesetting works with qemu but is possibly incomplete and might not work on cirrus real hardware (it only binds to the qemu subsystem id for that reason). take care, Gerd _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/bochs: Add screen blanking support 2021-04-27 9:56 ` Gerd Hoffmann @ 2021-04-27 12:21 ` Thomas Zimmermann 0 siblings, 0 replies; 6+ messages in thread From: Thomas Zimmermann @ 2021-04-27 12:21 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: dri-devel, virtualization [-- Attachment #1.1.1: Type: text/plain, Size: 992 bytes --] Am 27.04.21 um 11:56 schrieb Gerd Hoffmann: >>> 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. > > No objections. > > Acked-by: Gerd Hoffmann <kraxel@redhat.com> Great. Merged now. Thanks everyone. > > FYI: cirrus is in the same situation, the modesetting works with qemu > but is possibly incomplete and might not work on cirrus real hardware > (it only binds to the qemu subsystem id for that reason). > > take care, > Gerd > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 840 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-27 12:21 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-21 8:08 [PATCH v2] drm/bochs: Add screen blanking support Takashi Iwai 2021-04-26 18:44 ` Thomas Zimmermann 2021-04-27 6:18 ` Takashi Iwai 2021-04-27 6:58 ` Thomas Zimmermann 2021-04-27 9:56 ` Gerd Hoffmann 2021-04-27 12:21 ` Thomas Zimmermann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).