Re: [Qemu-devel] Cirrus graphics bug list and fixes

[Andreas Färber]

Hi Mark,

Am 01.12.2013 15:09, schrieb Mark:
> I mentioned in my previous message
> (http://lists.gnu.org/archive/html/qemu-devel/2013-12/msg00006.html) that
> a list of the qemu Cirrus bugs fixed by the developer of WinUAE would be
> forthcoming. :)
> 
> Please see the WinUAE git at
>   https://github.com/tonioni/WinUAE/tree/master/qemuvga
> 
> Grep for "TW." in the source as explained below to see where the changes
> are. Hopefully someone involved with qemu development can look over the
> changes and integrate them into qemu.

That's not how QEMU development works. If you take a look at the history
of the repository subdirectory you're pointing to,
https://github.com/tonioni/WinUAE/commits/master/qemuvga
then you will find that none of the commits carry a Signed-off-by from
its author. So we can't just copy from that code without him asserting
that his changes to that MIT/X11-licensed code are GPL-compatible.
Cf.
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297

If you want to help get these changes into qemu.git, a) contact that
author asking for his "Signed-off-by: Full name <email>" line for those
Cirrus changes and ideally b) fork qemu.git and apply the fixes one by
one as commits with full description and his and your Signed-off-by and
c) submit them to qemu-devel according to
http://wiki.qemu.org/Contribute/SubmitAPatch or ask the author to do
b)+c) himself.

Otherwise if those fixes are invisible to x86 Linux guests, the core
contributors may be reluctant to spend time investigating and fixing
issues they can't reproduce themselves.

CC'ing Gerd in case he understands how to reproduce the below and fix
without dependency on the WinUAE code.

Regards,
Andreas

> === begin quote ===
> 
> I quickly found following changes: (Sources have " TW." in comments +
> explanation for change)
> 
> BLTUNSAFE() macro is broken. It assumes source and destination have same
> size and it rejects valid blits if blit is special, for example color
> expansion.
> 
> cirrus_do_copy(): "extra x, y", s->cirrus_blt_srcpitch or
> s->cirrus_blt_dstpitch can be zeros: division by zero. depth==0 in planar
> VGA modes, test and set it to 1.
> 
> cirrus_bitblt_start(): CIRRUS_BLTMODE_TRANSPARENTCOMP, "If color expansion
> is used with transparency, background pixels are not written."
> 
> CIRRUS_BLT_STATUS bit was missing. PicassoII driver polls this to check if
> blit has finished.
> 
> cirrus_write_bitblt(): Blitter CIRRUS_BLT_RESET start condition check was
> wrong.
> 
> cirrus_get_offsets(): CL 5434/36 "32bit/Pixel Data at Pixel Rate" = Offset
> doubled. Interlace and multiply vertical registers by two added.
> 
> cirrus_get_bpp16_depth(): Missing depth=8 check added.
> 
> cirrus_get_resolution(): "if 16 bit mode but SR7 bit 7 == 0: 2x width and
> palette mode". Interlace and multiply vertical registers by two added.
> 
> cirrus_linear_read() and cirrus_linear_write(): "linear vram also need
> planar handling" (PC hardware may not need this?)
> 
> cirrus_reset(): 5434 and newer have 4M support, was 5446 and newer.
> 
> DRAM Control register/valid_memory_config variable: handle memory mapping
> (fixes VRAM size detection if driver tries to find memory size by trying
> invalid settings for current hardware)
> 
> glue(glue(glue(cirrus_colorexpand_transp_, ROP_NAME), _),DEPTH),
> glue(glue(glue(cirrus_colorexpand_pattern_transp_, ROP_NAME), _),DEPTH):
> "Color expansion + transparency: fgcol, not bgcol"
> 
> glue(cirrus_bitblt_rop_fwd_, ROP_NAME)(CirrusVGAState *s,
> uint8_t *dst,const uint8_t *src,
> int dstpitch,int srcpitch,
> int bltwidth,int bltheight)
> glue(cirrus_bitblt_rop_bkwd_, ROP_NAME)(CirrusVGAState *s,
> uint8_t *dst,const uint8_t *src,
> int dstpitch,int srcpitch,
> int bltwidth,int bltheight)
> : optimize plain copy blits by using ROP_OP_32 when possible
> 
> vga.c: vga_draw_graphic() seems to assume shift_control =
> (s->gr[VGA_GFX_MODE] >> 5) & 3; must be >=2 (bit 6 of Mode Register is
> "256 color mode") or it uses standard VGA graphics mode, even if cirrus
> extended registers are used to enable extended (SVGA) modes. My workaround
> was to set bit 6 when any extended mode is enabled. This may be bug in my
> implementation.
> 
> === end quote ===
> 
> 
> -- Mark

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg