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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- 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