public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: yuchenlin <yuchenlin@synology.com>
To: Laszlo Ersek <lersek@redhat.com>
Cc: edk2-devel@lists.01.org, jordan.l.justen@intel.com,
	ard.biesheuvel@linaro.org, anthony.perard@citrix.com,
	julien.grall@linaro.org, Gerd Hoffmann <kraxel@redhat.com>,
	Phil Dennis-Jordan <phil@philjordan.eu>
Subject: Re: [PATCH] OvmfPkg: initialize bochs when initializing vmsvga
Date: Tue, 23 Oct 2018 19:12:55 +0800	[thread overview]
Message-ID: <3f1bacc3feaf1ce6de3b42f91cf689ea@synology.com> (raw)
In-Reply-To: <ddd3122a-1ec3-0aa1-e17b-3c602802a787@redhat.com>

Hi, Laszlo

On 2018-10-23 18:18, Laszlo Ersek wrote:
> (1) Adding Gerd (because he maintains video in QEMU), and Phil
> Dennis-Jordan (for authoring commit c137d9508169, 
> "OvmfPkg/QemuVideoDxe:
> VMWare SVGA device support", 2017-04-07).
> 
> 
> On 10/23/18 04:40, yuchenlin@synology.com wrote:
>> From: yuchenlin <yuchenlin@synology.com>
>> 
>> When driver doesn't set fifo config, the vmsvga will fall back
>> to std vga. However, we don't initialize vbe related port. It
>> causes blank screen in qemu console.
> 
> (2) The words "when driver doesn't set fifo config" tell me nothing. 
> The
> QemuVideoDxe directory has zero instances of the word "fifo". The same
> applies to "OvmfPkg/Include/IndustryStandard/VmwareSvga.h".
> 
> In addition, the "vmsvga will fall back to std vga" statement is also
> unclear. Is that a statement about the QEMU device model's behavior?
> 

About FIFO, you can refer to 
https://github.com/prepare/vmware-svga/blob/master/doc/svga_interface.txt#L34,

"The main advantage of VMware's SVGA device over alternatives like VBE 
is that the virtual
machine can explicitly indicate which ares of the screen have changed by 
sending update rectangles
through the device's command FIFO."

According to 
https://github.com/prepare/vmware-svga/blob/master/doc/svga_interface.txt#L533,
to use vmsvga's FIFO. Driver should setup FIFO and set 
SVGA_REG_CONFIG_DONE to 1.
However, OVMF doesn't do it. In this case, vmsvga will fall back to vga. 
It is QEMU
device model's behavior after commit 104bd1dc70 (vmsvga: fix 
vmsvga_update_display).

> I vaguely suspect that your intent might be to say, "QemuVideoDxe does
> not perform a necessary configuration step, and therefore it cannot
> drive QEMU's VMW SVGA device". However, if that is indeed your intent,
> then I believe something must have changed recently in QEMU, because
> QemuVideoDxe *definitely* worked when Phil contributed the VMW SVGA
> driver logic, in commit c137d9508169.
> 
> Are we talking about a QEMU regression, or a driver-side configuration
> step that QemuVideoDxe has always missed (and we're being punished for
> it only now)?
> 

This is QEMU behavior change in commit 104bd1dc70 (vmsvga: fix 
vmsvga_update_display).
In my opinion, it is a correct change.

> 
>> This patch will fix "Guest has not initialized the display (yet)"
>> when using qemu -device vmware-svga with ovmf.
> 
> Right, as I write above, this definitely worked earlier. I suggest
> bisecting QEMU (and/or testing older QEMU machine types) to identify 
> the
> QEMU side change. Once we know that, we can decide whether this is a
> QEMU regression, or just exposing a long-standing OVMF bug.

In my opinion, it is a long-standing OVMF bug, which is based on the 
wrong behavior of QEMU's vmsvga.
It relied on dirty memory region to call dpy_gfx_update for updating 
display.

> 
> Comments about the code below:
> 
> 
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: yuchenlin <yuchenlin@synology.com>
>> ---
>>  OvmfPkg/QemuVideoDxe/Driver.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>> 
>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c 
>> b/OvmfPkg/QemuVideoDxe/Driver.c
>> index 0dce80e59..255c01881 100644
>> --- a/OvmfPkg/QemuVideoDxe/Driver.c
>> +++ b/OvmfPkg/QemuVideoDxe/Driver.c
>> @@ -1067,8 +1067,7 @@ InitializeVmwareSvgaGraphicsMode (
>> 
>>    VmwareSvgaWrite (Private, VmwareSvgaRegEnable, 1);
>> 
>> -  SetDefaultPalette (Private);
>> -  ClearScreen (Private);
>> +  InitializeBochsGraphicsMode (Private, ModeData);
>>  }
>> 
>>  EFI_STATUS
>> 
> 
> (3) Calling InitializeBochsGraphicsMode() from within
> InitializeVmwareSvgaGraphicsMode() seems wrong, considering the current
> structure of the driver.
> 
> We only have InitializeXxxGraphicsMode() calls in
> QemuVideoGraphicsOutputSetMode(). In order to determine which variant 
> to
> call, we check the "Private->Variant" field, in a "switch" statement.
> Therefore:
> 
> (3a) If a general fallback from "VmwareSvga" to "Bochs" is necessary,
> then the fallback logic should be added to earlier code that sets the
> Variant field.
> 
> You can see an example for that in the QemuVideoControllerDriverStart()
> function, near the debug message "QemuVideo: No mmio bar, fallback to
> port io". There the Variant field is degraded from the originally
> detected QEMU_VIDEO_BOCHS_MMIO value, to QEMU_VIDEO_BOCHS.
> 
> (3b) Or else, if calling InitializeVmwareSvgaGraphicsMode() is fine as 
> a
> basis, but we also need the actions of InitializeBochsGraphicsMode() 
> *in
> addition*, then please:
> 
> - extract the common actions from InitializeBochsGraphicsMode() to a 
> new
> helper function, and call the helper from both
> InitializeBochsGraphicsMode() and InitializeVmwareSvgaGraphicsMode(),
> 
> - explain, in InitializeVmwareSvgaGraphicsMode(), *why* the Bochs 
> config
> actions are necessary, in addition to the VmwareSvga actions.
> 
> Thanks,
> Laszlo

Thanks,
yuchenlin


  reply	other threads:[~2018-10-23 11:12 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-23  2:40 [PATCH] OvmfPkg: initialize bochs when initializing vmsvga yuchenlin
2018-10-23 10:18 ` Laszlo Ersek
2018-10-23 11:12   ` yuchenlin [this message]
2018-10-23 12:45     ` Laszlo Ersek
2018-10-23 13:42       ` Gerd Hoffmann
2018-10-23 14:02         ` Laszlo Ersek
2018-10-24  1:33           ` yuchenlin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3f1bacc3feaf1ce6de3b42f91cf689ea@synology.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox