[PATCH 4/5] win32u: Move null user driver implementation from user32.
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- dlls/user32/driver.c | 209 +---------------------------- dlls/win32u/driver.c | 310 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 310 insertions(+), 209 deletions(-)
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=101534 Your paranoid android. === debiant2 (32 bit report) === user32: menu.c:2337: Test failed: test 25
On 11/9/21 13:55, Jacek Caban wrote:
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- dlls/user32/driver.c | 209 +---------------------------- dlls/win32u/driver.c | 310 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 310 insertions(+), 209 deletions(-)
A little bit related to my previous comment, this patch introduces a reset to "null" graphics driver on driver unload. I think it could also add the same __wine_set_display_driver( &null_driver, WINE_GDI_DRIVER_VERSION ); call when the "null" driver is explicitly requested and loaded, possibly addressing my previous comment too (as the pointer would not be lasy_load_driver anymore). -- Rémi Bernon <rbernon(a)codeweavers.com>
On 11/10/21 00:41, Rémi Bernon wrote:
On 11/9/21 13:55, Jacek Caban wrote:
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- dlls/user32/driver.c | 209 +---------------------------- dlls/win32u/driver.c | 310 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 310 insertions(+), 209 deletions(-)
A little bit related to my previous comment, this patch introduces a reset to "null" graphics driver on driver unload.
I think it could also add the same
__wine_set_display_driver( &null_driver, WINE_GDI_DRIVER_VERSION );
call when the "null" driver is explicitly requested and loaded, possibly addressing my previous comment too (as the pointer would not be lasy_load_driver anymore).
Also, regarding EnumDisplayMonitors and GetMonitorInfo, for which you added some FIXME, I have some patches to move the default implementation back to the corresponding user32 functions. I intended to send them after the previous nulldrv display device cache series I sent a week ago but I don't think they actually depend on it. (Similarly, I used the monitor cache to keep the default and initial monitor for GetMonitorInfo implementation) I rebased and pushed the patches there if you're interested, although only the commit up to c998fbadab6aa33e3bd2740b0651a0c0509d4c02 are really relevant for this: https://github.com/rbernon/wine/compare/master..wip/nulldrv/v2 Cheers, -- Rémi Bernon <rbernon(a)codeweavers.com>
On 11/10/21 12:47 AM, Rémi Bernon wrote:
On 11/10/21 00:41, Rémi Bernon wrote:
On 11/9/21 13:55, Jacek Caban wrote:
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- dlls/user32/driver.c | 209 +---------------------------- dlls/win32u/driver.c | 310 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 310 insertions(+), 209 deletions(-)
A little bit related to my previous comment, this patch introduces a reset to "null" graphics driver on driver unload.
I think it could also add the same
__wine_set_display_driver( &null_driver, WINE_GDI_DRIVER_VERSION );
call when the "null" driver is explicitly requested and loaded, possibly addressing my previous comment too (as the pointer would not be lasy_load_driver anymore).
Also, regarding EnumDisplayMonitors and GetMonitorInfo, for which you added some FIXME, I have some patches to move the default implementation back to the corresponding user32 functions.
I intended to send them after the previous nulldrv display device cache series I sent a week ago but I don't think they actually depend on it.
(Similarly, I used the monitor cache to keep the default and initial monitor for GetMonitorInfo implementation)
I rebased and pushed the patches there if you're interested, although only the commit up to c998fbadab6aa33e3bd2740b0651a0c0509d4c02 are really relevant for this:
I have null driver functions and some other sysparams.c functions moved to win32u in my tree. Those needed to be changed due to to advapi32 and setupapi calls. I used a cache as well, but instead of having two caches, I extended adapters cache to contain missing monitor rects informations. There are a few functions that need to enumerate monitors, they would look like this: update_display_cache(); pthread_mutex_lock( &display_lock ); LIST_FOR_EACH_ENTRY( adapter, &adapters, struct display_device, entry ) { LIST_FOR_EACH_ENTRY( monitor, &adapter->children, struct monitor, dev.entry ) { /* .... */ } } pthread_mutex_unlock( &display_lock ); What do you think about such approach? Thanks, Jacek
On 11/10/21 15:01, Jacek Caban wrote:
On 11/10/21 12:47 AM, Rémi Bernon wrote:
On 11/10/21 00:41, Rémi Bernon wrote:
On 11/9/21 13:55, Jacek Caban wrote:
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- dlls/user32/driver.c | 209 +---------------------------- dlls/win32u/driver.c | 310 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 310 insertions(+), 209 deletions(-)
A little bit related to my previous comment, this patch introduces a reset to "null" graphics driver on driver unload.
I think it could also add the same
__wine_set_display_driver( &null_driver, WINE_GDI_DRIVER_VERSION );
call when the "null" driver is explicitly requested and loaded, possibly addressing my previous comment too (as the pointer would not be lasy_load_driver anymore).
Also, regarding EnumDisplayMonitors and GetMonitorInfo, for which you added some FIXME, I have some patches to move the default implementation back to the corresponding user32 functions.
I intended to send them after the previous nulldrv display device cache series I sent a week ago but I don't think they actually depend on it.
(Similarly, I used the monitor cache to keep the default and initial monitor for GetMonitorInfo implementation)
I rebased and pushed the patches there if you're interested, although only the commit up to c998fbadab6aa33e3bd2740b0651a0c0509d4c02 are really relevant for this:
I have null driver functions and some other sysparams.c functions moved to win32u in my tree. Those needed to be changed due to to advapi32 and setupapi calls. I used a cache as well, but instead of having two caches, I extended adapters cache to contain missing monitor rects informations. There are a few functions that need to enumerate monitors, they would look like this:
update_display_cache();
pthread_mutex_lock( &display_lock );
LIST_FOR_EACH_ENTRY( adapter, &adapters, struct display_device, entry ) { LIST_FOR_EACH_ENTRY( monitor, &adapter->children, struct monitor, dev.entry ) {
/* .... */
} }
pthread_mutex_unlock( &display_lock );
What do you think about such approach?
Do you actually need to move the display device cache to win32u or is it just because of the non-trivial nulldrv functions being moved there too? In the tree I linked above, in the end after all the changes, the nulldrv functions are all mostly no-op and the default implementation lives in user32, updating the cache by enumerating setupapi devices. It also moves several things out of graphics drivers, such as reading / writing current display mode, to the user32 EnumDisplaySettingsExW / ChangeDisplaySettingsEx generic code. Overall I think it makes everything much simpler, and reduce the amount of entry points graphics driver would need, and reduce the amount of changes needed if you need to replace all setupapi / advapi32 calls. Cheers, -- Rémi Bernon <rbernon(a)codeweavers.com>
On 11/10/21 3:19 PM, Rémi Bernon wrote:
On 11/10/21 15:01, Jacek Caban wrote:
On 11/10/21 12:47 AM, Rémi Bernon wrote:
On 11/10/21 00:41, Rémi Bernon wrote:
On 11/9/21 13:55, Jacek Caban wrote:
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- dlls/user32/driver.c | 209 +---------------------------- dlls/win32u/driver.c | 310 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 310 insertions(+), 209 deletions(-)
A little bit related to my previous comment, this patch introduces a reset to "null" graphics driver on driver unload.
I think it could also add the same
__wine_set_display_driver( &null_driver, WINE_GDI_DRIVER_VERSION );
call when the "null" driver is explicitly requested and loaded, possibly addressing my previous comment too (as the pointer would not be lasy_load_driver anymore).
Also, regarding EnumDisplayMonitors and GetMonitorInfo, for which you added some FIXME, I have some patches to move the default implementation back to the corresponding user32 functions.
I intended to send them after the previous nulldrv display device cache series I sent a week ago but I don't think they actually depend on it.
(Similarly, I used the monitor cache to keep the default and initial monitor for GetMonitorInfo implementation)
I rebased and pushed the patches there if you're interested, although only the commit up to c998fbadab6aa33e3bd2740b0651a0c0509d4c02 are really relevant for this:
I have null driver functions and some other sysparams.c functions moved to win32u in my tree. Those needed to be changed due to to advapi32 and setupapi calls. I used a cache as well, but instead of having two caches, I extended adapters cache to contain missing monitor rects informations. There are a few functions that need to enumerate monitors, they would look like this:
update_display_cache();
pthread_mutex_lock( &display_lock );
LIST_FOR_EACH_ENTRY( adapter, &adapters, struct display_device, entry ) { LIST_FOR_EACH_ENTRY( monitor, &adapter->children, struct monitor, dev.entry ) {
/* .... */
} }
pthread_mutex_unlock( &display_lock );
What do you think about such approach?
Do you actually need to move the display device cache to win32u or is it just because of the non-trivial nulldrv functions being moved there too?
It all needs to be moved. A number of other win32u functionality depends on those functions.
In the tree I linked above, in the end after all the changes, the nulldrv functions are all mostly no-op and the default implementation lives in user32, updating the cache by enumerating setupapi devices.
It also moves several things out of graphics drivers, such as reading / writing current display mode, to the user32 EnumDisplaySettingsExW / ChangeDisplaySettingsEx generic code.
Overall I think it makes everything much simpler, and reduce the amount of entry points graphics driver would need, and reduce the amount of changes needed if you need to replace all setupapi / advapi32 calls.
Yes, I think that those are mostly the right direction. I could even imagine setting registry in wineandroid.drv and remove pEnumDisplayMonitors entry point. I already replaced problematic setupapi / advapi32 calls from sysparams.c in my tree. I'd prefer cache layout like I mentioned, but it should be easy to adopt to a different cache layout as well. Thanks, Jacek
On 11/10/21 17:33, Jacek Caban wrote:
On 11/10/21 3:19 PM, Rémi Bernon wrote:
On 11/10/21 15:01, Jacek Caban wrote:
On 11/10/21 12:47 AM, Rémi Bernon wrote:
On 11/10/21 00:41, Rémi Bernon wrote:
On 11/9/21 13:55, Jacek Caban wrote:
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- dlls/user32/driver.c | 209 +---------------------------- dlls/win32u/driver.c | 310 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 310 insertions(+), 209 deletions(-)
A little bit related to my previous comment, this patch introduces a reset to "null" graphics driver on driver unload.
I think it could also add the same
__wine_set_display_driver( &null_driver, WINE_GDI_DRIVER_VERSION );
call when the "null" driver is explicitly requested and loaded, possibly addressing my previous comment too (as the pointer would not be lasy_load_driver anymore).
Also, regarding EnumDisplayMonitors and GetMonitorInfo, for which you added some FIXME, I have some patches to move the default implementation back to the corresponding user32 functions.
I intended to send them after the previous nulldrv display device cache series I sent a week ago but I don't think they actually depend on it.
(Similarly, I used the monitor cache to keep the default and initial monitor for GetMonitorInfo implementation)
I rebased and pushed the patches there if you're interested, although only the commit up to c998fbadab6aa33e3bd2740b0651a0c0509d4c02 are really relevant for this:
I have null driver functions and some other sysparams.c functions moved to win32u in my tree. Those needed to be changed due to to advapi32 and setupapi calls. I used a cache as well, but instead of having two caches, I extended adapters cache to contain missing monitor rects informations. There are a few functions that need to enumerate monitors, they would look like this:
update_display_cache();
pthread_mutex_lock( &display_lock );
LIST_FOR_EACH_ENTRY( adapter, &adapters, struct display_device, entry ) { LIST_FOR_EACH_ENTRY( monitor, &adapter->children, struct monitor, dev.entry ) {
/* .... */
} }
pthread_mutex_unlock( &display_lock );
What do you think about such approach?
Do you actually need to move the display device cache to win32u or is it just because of the non-trivial nulldrv functions being moved there too?
It all needs to be moved. A number of other win32u functionality depends on those functions.
In the tree I linked above, in the end after all the changes, the nulldrv functions are all mostly no-op and the default implementation lives in user32, updating the cache by enumerating setupapi devices.
It also moves several things out of graphics drivers, such as reading / writing current display mode, to the user32 EnumDisplaySettingsExW / ChangeDisplaySettingsEx generic code.
Overall I think it makes everything much simpler, and reduce the amount of entry points graphics driver would need, and reduce the amount of changes needed if you need to replace all setupapi / advapi32 calls.
Yes, I think that those are mostly the right direction. I could even imagine setting registry in wineandroid.drv and remove pEnumDisplayMonitors entry point.
I already replaced problematic setupapi / advapi32 calls from sysparams.c in my tree. I'd prefer cache layout like I mentioned, but it should be easy to adopt to a different cache layout as well.
I prefer a flat array for its simplicity, as we don't really need any custom struct, as it also allows some initial values to be set conveniently through its initialized, and because I expect the number of display devices / monitors to be bounded, but it probably doesn't matter much. And it can very well be changed later. -- Rémi Bernon <rbernon(a)codeweavers.com>
Hi Rémi, On 11/10/21 12:41 AM, Rémi Bernon wrote:
On 11/9/21 13:55, Jacek Caban wrote:
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- dlls/user32/driver.c | 209 +---------------------------- dlls/win32u/driver.c | 310 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 310 insertions(+), 209 deletions(-)
A little bit related to my previous comment, this patch introduces a reset to "null" graphics driver on driver unload.
I think it could also add the same
__wine_set_display_driver( &null_driver, WINE_GDI_DRIVER_VERSION );
call when the "null" driver is explicitly requested and loaded, possibly addressing my previous comment too (as the pointer would not be lasy_load_driver anymore).
Good point. I think we will need to use __wine_set_user_driver here until more stuff is moved to win32u. I will send v2. Thanks, Jacek
participants (3)
-
Jacek Caban -
Marvin -
Rémi Bernon