| Message ID | 1533815202-11967-27-git-send-email-liran.alon@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | : vmsvga: Various fixes and enhancements | expand |
Hi, > + case SVGA_REG_DISPLAY_WIDTH: > + if ((s->display_id == 0) || (s->display_id == SVGA_ID_INVALID)) > + ret = s->new_width ? s->new_width : surface_width(surface); > + else > + ret = 0; > + break; > + case SVGA_REG_DISPLAY_HEIGHT: > + if ((s->display_id == 0) || (s->display_id == SVGA_ID_INVALID)) > + ret = s->new_height ? s->new_height : surface_height(surface); > + else > + ret = 0; > + break; What is the purpose of these registers? Hint for the guest about the host display size? If so you probably want wire up a callback for GraphicHwOps->ui_info. This will be called on display changes (i.e. user resizes qemu gtk window). See virtio_gpu_ui_info() for an example. cheers, Gerd
> On 10 Aug 2018, at 12:56, Gerd Hoffmann <kraxel@redhat.com> wrote: > > Hi, > >> + case SVGA_REG_DISPLAY_WIDTH: >> + if ((s->display_id == 0) || (s->display_id == SVGA_ID_INVALID)) >> + ret = s->new_width ? s->new_width : surface_width(surface); >> + else >> + ret = 0; >> + break; >> + case SVGA_REG_DISPLAY_HEIGHT: >> + if ((s->display_id == 0) || (s->display_id == SVGA_ID_INVALID)) >> + ret = s->new_height ? s->new_height : surface_height(surface); >> + else >> + ret = 0; >> + break; > > What is the purpose of these registers? Hint for the guest about the > host display size? If so you probably want wire up a callback for > GraphicHwOps->ui_info. This will be called on display changes (i.e. > user resizes qemu gtk window). See virtio_gpu_ui_info() for an example. > > cheers, > Gerd > These registers are suppose to indicate to guest the display monitor size (width & height). When the QEMU console framework timer will run gui_update()->dpy_refresh()->VNC dpy_refresh()->graphics_hw_update()->VMware-SVGA gfx_update() Then vmsvga_update_display()->vmsvga_check_size() makes sure that host display surface is set to have width/height as specified in vmsvga_state_s->{new_width, new_height}. Therefore, if new_width have been set, then host display will be changed to the value set by the guest. Thus, wiring up GraphicsHwOps->ui_info callback to return new info on SVGA_REG_DISPLAY_{WIDTH, HEIGHT} registers may be useful only for case that we want guest to respond to the fact that the host display have been resized. However, I am not sure there is a mechanism to notify guest from vmware-svga device that this even has occurred for guest to reread these registers. Both in Linux vmware-svga driver code and VMware SVGA development kit, the SVGA_IRQFLAG_* flags don’t indicate such an interrupt source. In addition, it seems that Linux vmware-svga driver code only reads these registers at vmw_kms_save_vga() which weirdly enough, seems to be unreachable code (not called from anywhere…). Therefore, I’m not sure it is important to do this change at this patch series. But I am not 100% familiar with the entire QEMU graphics stack so maybe I’m missing something trivial here. Regards, -Liran
Hi, > These registers are suppose to indicate to guest the display monitor > size (width & height). > Thus, wiring up GraphicsHwOps->ui_info callback to return new info on > SVGA_REG_DISPLAY_{WIDTH, HEIGHT} registers may be useful only for case > that we want guest to respond to the fact that the host display have > been resized. Yes, that is the purpose of the ui_info callback. virtio-vga is the easiest way to play with this: Resize the gtk window, and gnome should adapt the guest desktop to the new size, so you don't have a scaled guest desktop. > However, I am not sure there is a mechanism to notify guest from > vmware-svga device that this even has occurred for guest to reread > these registers. Both in Linux vmware-svga driver code and VMware > SVGA development kit, the SVGA_IRQFLAG_* flags don’t indicate such an > interrupt source. In addition, it seems that Linux vmware-svga driver > code only reads these registers at vmw_kms_save_vga() which weirdly > enough, seems to be unreachable code (not called from anywhere…). Hmm, that is strange ... > Therefore, I’m not sure it is important to do this change at this > patch series. It isn't important for the series anyway. Was just meant as hint where the vmware-svga emulation could possibly improved even more, now that you are at it. It can certainly go either later as separate series or not at all if it turns out it doesn't work anyway because the guests drivers don't check the registers. cheers, Gerd
diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c index b2f3456357bd..edd336e65005 100644 --- a/hw/display/vmware_vga.c +++ b/hw/display/vmware_vga.c @@ -81,6 +81,7 @@ struct vmsvga_state_s { uint32_t num_fifo_regs; uint32_t irq_mask; uint32_t irq_status; + uint32_t display_id; }; #define TYPE_VMWARE_SVGA "vmware-svga" @@ -103,6 +104,9 @@ struct pci_vmsvga_state_s { #define SVGA_ID_1 SVGA_MAKE_ID(1) #define SVGA_ID_2 SVGA_MAKE_ID(2) +/* "Invalid" value for all SVGA IDs. (Version ID, screen object ID, surface ID...) */ +#define SVGA_ID_INVALID 0xFFFFFFFF + #define SVGA_LEGACY_BASE_PORT 0x4560 #define SVGA_INDEX_PORT 0x0 #define SVGA_VALUE_PORT 0x1 @@ -164,6 +168,15 @@ enum { SVGA_REG_PITCHLOCK = 32, /* Fixed pitch for all modes */ SVGA_REG_IRQMASK = 33, /* Interrupt mask */ + /* Legacy multi-monitor support */ + SVGA_REG_NUM_GUEST_DISPLAYS = 34, /* Number of guest displays in X/Y direction */ + SVGA_REG_DISPLAY_ID = 35, /* Display ID for the following display attributes */ + SVGA_REG_DISPLAY_IS_PRIMARY = 36, /* Whether this is a primary display */ + SVGA_REG_DISPLAY_POSITION_X = 37, /* The display position x */ + SVGA_REG_DISPLAY_POSITION_Y = 38, /* The display position y */ + SVGA_REG_DISPLAY_WIDTH = 39, /* The display's width */ + SVGA_REG_DISPLAY_HEIGHT = 40, /* The display's height */ + /* Guest memory regions */ SVGA_REG_GMR_ID = 41, SVGA_REG_GMR_DESCRIPTOR = 42, @@ -195,6 +208,7 @@ enum { #define SVGA_CAP_MULTIMON (1 << 16) #define SVGA_CAP_PITCHLOCK (1 << 17) #define SVGA_CAP_IRQMASK (1 << 18) +#define SVGA_CAP_DISPLAY_TOPOLOGY (1 << 19) // Legacy multi-monitor support /* * FIFO offsets (seen as an array of 32-bit words) @@ -1227,6 +1241,7 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address) #endif caps |= SVGA_CAP_EXTENDED_FIFO; caps |= SVGA_CAP_IRQMASK; + caps |= SVGA_CAP_DISPLAY_TOPOLOGY; ret = caps; break; @@ -1288,6 +1303,32 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address) ret = s->irq_mask; break; + case SVGA_REG_NUM_GUEST_DISPLAYS: + ret = 1; + break; + case SVGA_REG_DISPLAY_ID: + ret = s->display_id; + break; + case SVGA_REG_DISPLAY_IS_PRIMARY: + ret = s->display_id == 0 ? 1 : 0; + break; + case SVGA_REG_DISPLAY_POSITION_X: + case SVGA_REG_DISPLAY_POSITION_Y: + ret = 0; + break; + case SVGA_REG_DISPLAY_WIDTH: + if ((s->display_id == 0) || (s->display_id == SVGA_ID_INVALID)) + ret = s->new_width ? s->new_width : surface_width(surface); + else + ret = 0; + break; + case SVGA_REG_DISPLAY_HEIGHT: + if ((s->display_id == 0) || (s->display_id == SVGA_ID_INVALID)) + ret = s->new_height ? s->new_height : surface_height(surface); + else + ret = 0; + break; + /* Guest memory regions */ case SVGA_REG_GMR_ID: case SVGA_REG_GMR_DESCRIPTOR: @@ -1431,6 +1472,28 @@ static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value) s->irq_mask = value; break; + /* We support single legacy screen at fixed offset of (0,0) */ + case SVGA_REG_DISPLAY_ID: + s->display_id = value; + break; + case SVGA_REG_NUM_GUEST_DISPLAYS: + case SVGA_REG_DISPLAY_IS_PRIMARY: + case SVGA_REG_DISPLAY_POSITION_X: + case SVGA_REG_DISPLAY_POSITION_Y: + break; + case SVGA_REG_DISPLAY_WIDTH: + if ((s->display_id == 0) && (value <= SVGA_MAX_WIDTH)) { + s->new_width = value; + s->invalidated = 1; + } + break; + case SVGA_REG_DISPLAY_HEIGHT: + if ((s->display_id == 0) && (value <= SVGA_MAX_HEIGHT)) { + s->new_height = value; + s->invalidated = 1; + } + break; + default: if (s->index >= SVGA_SCRATCH_BASE && s->index < SVGA_SCRATCH_BASE + s->scratch_size) { @@ -1534,6 +1597,7 @@ static void vmsvga_reset(DeviceState *dev) s->irq_mask = 0; s->irq_status = 0; s->last_fifo_cursor_count = 0; + s->display_id = SVGA_ID_INVALID; vga_dirty_log_start(&s->vga); } @@ -1568,6 +1632,7 @@ static int vmsvga_post_load(void *opaque, int version_id) s->irq_mask = 0; s->irq_status = 0; s->last_fifo_cursor_count = 0; + s->display_id = SVGA_ID_INVALID; } return 0; @@ -1598,6 +1663,7 @@ static const VMStateDescription vmstate_vmware_vga_internal = { VMSTATE_UINT32_V(irq_mask, struct vmsvga_state_s, 1), VMSTATE_UINT32_V(irq_status, struct vmsvga_state_s, 1), VMSTATE_UINT32_V(last_fifo_cursor_count, struct vmsvga_state_s, 1), + VMSTATE_UINT32_V(display_id, struct vmsvga_state_s, 1), VMSTATE_END_OF_LIST() } };