On 5/31/21 11:16 AM, Zhiyi Zhang wrote:
On 5/22/21 1:25 AM, Rémi Bernon wrote:
Hi Zhiyi!
I had a look, as I was working on the null graphics driver and as the monitor code is the only thing remaining for it to pass user32 tests.
IMHO this could at least be split (unless there's something I missed that makes it hard), with first the monitor info being sent to wineserver, then using it for enumeration and then to get monitor info in separate patches.
Hi Rémi,
Yes, I could split the patch that way. But it will then create a patch that does nothing with only the monitor information in the wineserver and they're not used anywhere. I was advised not to do create such patches before.
Then, I think duplicating the code between winex11.drv and winemac.drv is not great, and that it would be nice if this could be done in user32 instead.
It would be a bit more work to refactor the driver interface first, but I think it would be a nice way to reduce code duplication, and it would then be much easier to expose fake monitors and modes as a default implementation, for the null graphics driver.
For instance, I think the driver interface should probably expose get_gpus / get_adapters / get_monitors callbacks.
We could even maybe name them pEnumDisplayDevices / pEnumDisplayMonitors and keep it closer to their user32 entry point counterparts, and to make it more consistent with the other driver entry points.
It may only be worth the trouble if we can return the information we need from the drivers in the related user32 structures, as otherwise we'll need custom struct anyway.
I'm aware of the duplication for tje display device initialization code in winex11.drv and winemac.drv. I just don't think the refactoring is worth the trouble at this point. There is also of the question of whether it is appropriate to do display device enumeration in user32. I think you don't have to refactor the display device interface to refactor the null graphics driver.
Okay, maybe I'm wrong, but as far as I could see, to report fake display devices, monitors and modes, I basically had to reproduce most of winex11.drv / winemac.drv code to user32, which feels even more of a waste.
It may be possible to do it with less, I didn't spend much time on it.
Then, user32 would call these to enumerate the devices, factoring winex11.drv and winemac.drv init_display_devices into user32.
On 5/19/21 10:14 AM, Zhiyi Zhang wrote:
@@ -3974,9 +3975,35 @@ fail: BOOL CDECL nulldrv_GetMonitorInfo( HMONITOR handle, MONITORINFO *info ) { UINT index = (UINT_PTR)handle - 1; + NTSTATUS status; TRACE("(%p, %p)\n", handle, info); + SERVER_START_REQ( get_monitor_info ) + { + req->handle = wine_server_user_handle( handle ); + if (info->cbSize >= sizeof(MONITORINFOEXW))
I think this is enforced to be == (or == sizeof(MONITORINFO)) elsewhere, having >= feels pretty confusing.
Thanks. I will change it to == sizeof(MONITORINFOEXW)
+ wine_server_set_reply( req, ((MONITORINFOEXW *)info)->szDevice, + sizeof(((MONITORINFOEXW *)info)->szDevice) - sizeof(WCHAR) ); + if (!(status = wine_server_call( req ))) + { + SetRect( &info->rcMonitor, reply->monitor_rect.left, reply->monitor_rect.top, + reply->monitor_rect.right, reply->monitor_rect.bottom ); + SetRect( &info->rcWork, reply->work_rect.left, reply->work_rect.top, + reply->work_rect.right, reply->work_rect.bottom ); + if (!info->rcMonitor.left && !info->rcMonitor.top && info->rcMonitor.right && info->rcMonitor.bottom) + info->dwFlags = MONITORINFOF_PRIMARY; + else + info->dwFlags = 0; + if (info->cbSize >= sizeof(MONITORINFOEXW)) + ((MONITORINFOEXW *)info)->szDevice[wine_server_reply_size( req ) / sizeof(WCHAR)] = 0;
Is this even guaranteed to be zero-terminated? Also see comments below, as it's a fixed 32 WCHAR array, I think we could maybe just pass it around.
Yes, it's zero-terminated. Unless we want to store a '\0' in the wineserver and we don't have to do this. The previous review from Alexandre said that strings in wineserver shouldn't be zero-terminated.
Sure, but IMHO the server doesn't even have to care about this string length, the name could just be considered as a 32 WCHAR fixed size array, and its zero-terminated nature would be handled on the client side.
+/* Wine server monitor list management */
+struct wine_server_monitor_info +{ + unsigned int entry_count; + unsigned int entry_capacity; + struct update_monitor_entry *entries; +};
+static BOOL wine_server_add_monitor_info(struct wine_server_monitor_info *info, + const struct x11drv_monitor *monitor, int adapter_index)
I'm sure it's better to keep wine_server_ prefixes for the few core functions.
diff --git a/server/display.c b/server/display.c new file mode 100644 index 00000000000..8e8ac2d1338 --- /dev/null +++ b/server/display.c @@ -0,0 +1,133 @@ +/*
- Server-side display device management
- Copyright (C) 2021 Zhiyi Zhang for CodeWeavers
- This library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 2.1 of the License, or (at your option) any later version.
- This library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Lesser General Public License for more details.
- You should have received a copy of the GNU Lesser General Public
- License along with this library; if not, write to the Free Software
- Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
- */
+#include "config.h"
+#include <stdarg.h>
+#include "ntstatus.h" +#define WIN32_NO_STATUS +#include "winternl.h"
+#include "request.h" +#include "user.h"
+static struct list monitor_list = LIST_INIT(monitor_list); +static unsigned int monitor_count;
+/* retrieve a pointer to a monitor from its handle */ +static struct monitor *get_monitor( user_handle_t handle ) +{ + struct monitor *monitor;
+ if (!(monitor = get_user_object( handle, USER_MONITOR ))) + set_win32_error( ERROR_INVALID_MONITOR_HANDLE ); + return monitor; +}
+/* create a monitor */ +static void create_monitor( const struct update_monitor_entry *entry ) +{ + struct monitor *monitor;
+ if (!(monitor = mem_alloc( sizeof(*monitor) ))) + goto failed;
This is unnecessary and it actually makes the error path more complicated.
I think memory allocation failures should be handled.
Sure, but the first failure can just be a return as there's not cleanup to do, in which case the error code path will never be taken with monitor == 0, making the if there unnecessary.
+ if (!(monitor->adapter_name = memdup( entry->adapter_name, entry->adapter_name_len ))) + goto failed; + monitor->adapter_name_len = entry->adapter_name_len;
+ if (!(monitor->handle = alloc_user_handle( monitor, USER_MONITOR ))) + goto failed;
+ monitor->monitor_rect = entry->monitor_rect; + monitor->work_rect = entry->work_rect; + list_add_tail( &monitor_list, &monitor->entry ); + ++monitor_count; + return;
+failed: + if (monitor) + { + if (monitor->adapter_name) + free( monitor->adapter_name ); + free( monitor ); + } +}
To make things even simpler, I think you should move adapter_name to the end of the struct, as a WCHAR[32], and allocate all memory at once.
Then there would be only one error code path, when the handle allocation fails, and it can simply free the memory and return.
It's done this way to save memory.
Okay, but it's saving at most 62 bytes for each monitor, which doesn't seem very useful. Even then you could do a single allocation of the struct size and the string length, simplifying the error handling.