[PATCH v4 3/6] winex11.drv: Store monitor information in the wineserver for EnumDisplayMonitors().
Rémi Bernon
rbernon at codeweavers.com
Fri May 21 12:25:42 CDT 2021
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.
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.
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.
> + 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.
>
> +/* 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.
> +
> + 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.
> +/* modify the list of monitors */
> +DECL_HANDLER(update_monitors)
> +{
> + const struct update_monitor_entry *entries;
> + struct monitor *monitor, *monitor2;
> + unsigned int entry_count, i;
> +
> + LIST_FOR_EACH_ENTRY_SAFE(monitor, monitor2, &monitor_list, struct monitor, entry)
> + {
> + list_remove( &monitor->entry );
> + free_user_handle( monitor->handle );
> + free( monitor->adapter_name );
> + free( monitor );
> + }
> + monitor_count = 0;
> +
> + entries = get_req_data();
> + entry_count = get_req_data_size() / sizeof(*entries);
> + for (i = 0; i < entry_count; ++i)
> + create_monitor( &entries[i] );
> +}
I think this is going to change the handles every time the list is
updated, is this going to be an issue?
> diff --git a/server/protocol.def b/server/protocol.def
> index 6d8208b128b..3d536a0a237 100644
> --- a/server/protocol.def
> +++ b/server/protocol.def
> @@ -2693,6 +2693,45 @@ enum coords_relative
> #define SET_USER_OBJECT_GET_FULL_NAME 2
>
>
> +struct update_monitor_entry
> +{
> + rectangle_t monitor_rect; /* monitor rectangle */
> + rectangle_t work_rect; /* monitor work area rectangle */
> + WCHAR adapter_name[32]; /* adapter name */
> + data_size_t adapter_name_len; /* adapter name length in bytes */
> +};
Do we need the length here? As I understand it, this will only ever be
used to fill MONITORINFOEXW structures, which have a fixed size buffer
as well.
Could we just pass the fixed size WCHAR array? Although I don't know if
it's good practice.
> +
> +struct enum_monitor_entry
> +{
> + user_handle_t handle; /* handle to the monitor */
> + rectangle_t monitor_rect; /* monitor rectangle */
> +};
> +
> +
> +/* Modify the list of monitors */
> + at REQ(update_monitors)
> + VARARG(monitors,update_monitor_entry); /* A list of monitors to be created */
> + at END
> +
> +
> +/* Get information about a monitor */
> + at REQ(get_monitor_info)
> + user_handle_t handle; /* handle to the monitor */
> + at REPLY
> + rectangle_t monitor_rect; /* monitor rectangle */
> + rectangle_t work_rect; /* monitor work area rectangle */
> + VARARG(adapter,unicode_str); /* adapter name */
> + at END
> +
> +
> +/* Enumerate monitors */
> + at REQ(enum_monitors)
> + at REPLY
> + unsigned int count; /* total count of monitors */
> + VARARG(monitors,enum_monitor_entry); /* A list of monitors enumerated */
> + at END
> +
> +
> /* Register a hotkey */
> @REQ(register_hotkey)
> user_handle_t window; /* handle to the window */
> diff --git a/server/user.h b/server/user.h
> index 80f7e91f12c..a9807f1402d 100644
> --- a/server/user.h
> +++ b/server/user.h
> @@ -36,7 +36,8 @@ enum user_object
> {
> USER_WINDOW = 1,
> USER_HOOK,
> - USER_CLIENT /* arbitrary client handle */
> + USER_CLIENT, /* arbitrary client handle */
> + USER_MONITOR
> };
>
> #define DESKTOP_ATOM ((atom_t)32769)
> @@ -79,6 +80,16 @@ struct desktop
> unsigned char keystate[256]; /* asynchronous key state */
> };
>
> +struct monitor
> +{
> + user_handle_t handle; /* monitor handle */
> + struct list entry; /* entry in global monitor list */
> + rectangle_t monitor_rect; /* monitor rectangle */
> + rectangle_t work_rect; /* monitor work area rectangle */
> + WCHAR *adapter_name; /* adapter name */
> + data_size_t adapter_name_len; /* adapter name length */
> +};
> +
Same comment as above.
Cheers,
--
Rémi Bernon <rbernon at codeweavers.com>
More information about the wine-devel
mailing list