Re: [PATCH v2] drm/bochs: Add screen blanking support

[Takashi Iwai <tiwai@suse.de>]

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