Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added patch to make csmt checkbox work on Staging tab #57

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
7 participants
@SveSop
Copy link

commented Mar 30, 2018

As wined3d-csmt.dll no longer is being produced, the "Enable csmt" checkbox does not do other than write to the registry a value that is not being used, as the .dll is not produced.

I have attempted to write a simple patch that makes the checkbox DISABLE csmt (as it is default enabled) by writing DWORD csmt=0x0 to the registry, and then delete it if the checkbox is cleared.

Probably better ways to do this, and should perhaps check the value in the registry incase it is set to 0x1 manually with a reg edit, as the checkbox will indicate csmt being disabled. Clearing the checkbox will remove the reg-key and make csmt default (enabled) again.

Feel free to fix my code, or integrate that to 0001 patch for a cleaner approach :)

@SveSop

This comment has been minimized.

Copy link
Owner Author

commented on 336ac77 Mar 30, 2018

First attempt to try to make the CSMT checkbox on the "Staging" tab useful for disable/enable CSMT. Default csmt is ENABLED, so setting DWORD reg key csmt=0 disables it, while not having the key enables it.
Worst case is if the reg-key exists and is set to 1 manually by editing registry, the tab would THINK it is disabled. Removing it will delete the key, and make it enabled again. Probably a more elegant way would be to check the value of the DWORD in the key, but i could not figure out how to do that...

@zfigura

This comment has been minimized.

Copy link
Member

commented Mar 30, 2018

Hello SveSop, thanks for the PR.

You'd want to use RegQueryValueExW() to grab the value of the CSMT key in this case.

I think the idea behind this patch makes sense. I can also say, however, that Henri will never accept it into Wine, and accordingly I'm not sure there's a lot of point in keeping it in Staging. Unlike (e.g.) those options related to desktop integration, CSMT is supposed to be used and work all of the time. In his words, "we don't want to be in a situation where users are expected to enable csmt for some applications, but disable them for others, etc."

And the fact that this checkbox has been basically completely ineffective since even before the 3.3 release sort of suggests to me that the number of bugs caused by CSMT is not really large enough that it'd be worth having this option anyway.

@SveSop

This comment has been minimized.

Copy link
Author

commented Mar 30, 2018

@zfigura I agree somewhat in what you are saying. Was more of something i wanted to do to see if i could do it :)
The option was there, and i know i do tests with/without csmt sometimes to check stuff, and it has many times degraded performance leaving it enabled...

Testing with __GL_THREADED_OPTIMIZATIONS (for nVidia) and with/without csmt sometimes makes a difference for performance.. and since the option actually is there and HAS worked in the past, i saw no harm in trying to make it work for something in the staging branch, as a quick way of setting it when doing benchmarks and similar stuff. Its not a question of "breaking stuff", but more of a performance thing. Numerous posts about this when testing games like eg. World of Warcraft, where stuff not only depend on drawcalls to the graphics engine. Hence.. it IS sometimes necessary to disable csmt to gain performance.

Will twiddle a bit more with it, if not for anything else than my own learning, and you guys decide if its something worth having/fixing FOR NOW, or for that matter, just delete the checkbox altogether, cos as it is, it's not doing anything and might confuse more than it does good.

@zfigura

This comment has been minimized.

Copy link
Member

commented Mar 30, 2018

The option to disable CSMT via the registry (or via winetricks) still exists, and I'd honestly be surprised if it ever ends up getting removed, but the idea is that putting it in winecfg isn't really appropriate. To that end, yeah, I think we should remove the checkbox altogether. I'll wait for feedback from Alistair/overwhelming negative feedback from the community on that option, though.

@IngeniousDox

This comment has been minimized.

Copy link

commented Mar 31, 2018

Before using Wine-PBA, using CSMT just gave you much higher CPU usage, with 1 or 2 fps loss on main pc, and even more on my laptop. That at least was in the game I play (Blizzard only). This is due to that gpu bottleneck PBA now actually fixes. But PBA isn't in mainline wine, so some people might still run up against the bottleneck, and disabling CSMT will actually increase their fps.

Now, does this warrants a nice checkbox in winecfg? Perhaps not in normal wine, there you should address the bottleneck instead. But until the mainline bottleneck is fixed, it might be handy to have it in staging.

Personally I have no problem changing stuff in the registry, but I rather not tell people to have to use that to increase their fps...but switching something in winecfg is easy enough for most.

@alesliehughes

This comment has been minimized.

Copy link

commented Mar 31, 2018

I agree that the option should toggle CSMT. Most users don't want to use winetricks or modify the registry. Once the issues have been resolved upstream with respect to performance, it can be removed. An error will appear in the log if a value has been set anyway.

@pchome

This comment has been minimized.

Copy link

commented Mar 31, 2018

this:

--- a/programs/winecfg/staging.c        2018-03-31 20:49:07.923373839 +0300
+++ b/programs/winecfg/staging.c        2018-03-31 20:57:45.887358201 +0300
@@ -36,14 +36,14 @@
 static BOOL csmt_get(void)
 {
     BOOL ret;
-    char *value = get_reg_key(config_key, keypath("DllRedirects"), "wined3d", NULL);
-    ret = (value && !strcmp(value, "wined3d-csmt.dll"));
+    char *value = get_reg_key(config_key, keypath("Direct3D"), "csmt", "1");
+    ret = (value || !strcmp(value, "1"));
     HeapFree(GetProcessHeap(), 0, value);
     return ret;
 }
 static void csmt_set(BOOL status)
 {
-    set_reg_key(config_key, keypath("DllRedirects"), "wined3d", status ? "wined3d-csmt.dll" : NULL);
+    set_reg_key(config_key, keypath("Direct3D"), "csmt", status ? NULL : "0");
 }
 
 /*
@SveSop

This comment has been minimized.

Copy link
Author

commented Mar 31, 2018

@pchome Does set_reg_key write a DWORD with value 0x00000000? I could only get it to write a "key"..
void set_reg_key(HKEY root, const char *path, const char *name, const char *value); from winecfg.h

Was why i used the set_reg_key_dword in my code, and had trouble reading the value from it, cos it is not a "char" when it is a DWORD.

CSMT
[DWORD Value (REG_DWORD): Enable (0x1, default) or disable (0x0)

From the winehq wiki.

@pchome

This comment has been minimized.

Copy link

commented Mar 31, 2018

@SveSop
My fault, csmt REG_SZ 0 in regedit

@zfigura

This comment has been minimized.

Copy link
Member

commented Apr 1, 2018

This should be fixed by 6e3fbe2.

@zfigura zfigura closed this Apr 1, 2018

@SveSop

This comment has been minimized.

Copy link
Author

commented Apr 2, 2018

@zfigura I see the problem is kinda the same i cant wrap my head around.. and that is that get_reg_key does not really seem to read the DWORD correctly.. or something, cos the checkbox is set even tho the value is 0x00000000, and toggling the box will just swap the checkbox to enabled again no matter what value is in the registry.

@IngeniousDox

This comment has been minimized.

Copy link

commented Apr 2, 2018

Someone on Reddit said the same, that the checkbox is always on.

@bobwya

This comment has been minimized.

Copy link

commented Apr 2, 2018

I think the csmt_get() function should actually be:

static BOOL csmt_get(void)
{
    char *buf = get_reg_key(config_key, "Direct3D", "csmt", NULL);
    BOOL ret = (*buf != '\0') ? !!*buf : TRUE;
    HeapFree(GetProcessHeap(), 0, buf);
   return ret;
}

I'm just testing that out now... ;-)

@pchome

This comment has been minimized.

Copy link

commented Apr 2, 2018

@bobwya

BOOL ret = (buf && *buf == '\0') ? FALSE : TRUE;
@zfigura

This comment has been minimized.

Copy link
Member

commented Apr 2, 2018

Thanks for the catch, it's fixed now.

@ghost

This comment has been minimized.

Copy link

commented Apr 4, 2018

I'm too lazy to fork..
currently csmt is labeled depreciated in winecfg, which is ambiguous

cheaper than a diff
sed -i 's/ (deprecated)//' patches/winecfg-Staging/000{1..4}*

@zfigura

This comment has been minimized.

Copy link
Member

commented Apr 4, 2018

The addition was intentional. As explained above, disabling CSMT via winecfg should not be treated as a permanent solution.

@ghost

This comment has been minimized.

Copy link

commented Apr 4, 2018

nope, still ambiguous.

(deprecated) ?
what is depreciated?

  • the upstream csmt?
  • the toggle?

I believe that label was for the dllredirect, which was deprecated in light of upstream's csmt
this commit changed the meaning of that label. since winecfg now toggles upstream csmt and not the dllredirect , which is no longer depreciated. but deceased.

@zfigura

This comment has been minimized.

Copy link
Member

commented Apr 4, 2018

The toggle is deprecated. It is not really true that the DLL redirect is deprecated, since, as you say, it does not exist. Rather, the option to disable CSMT via winecfg should not be depended on, as it will eventually be removed.

@ghost

This comment has been minimized.

Copy link

commented Apr 4, 2018

hmm, well maybe it was , but now it does something different.
It no longer sets up the, recently deceased and at the time depreciated, wined3d-csmt.dll redirect.
No, now it toggles the upstream dword for csmt feature.

perhaps the box should renamed to
upstream CSMT which would remove any and all ambiguity.

@SveSop

This comment has been minimized.

Copy link
Author

commented Apr 5, 2018

Deprecated - meaning "tolerated but not recommended." is fine, as i guess if the wine devs are planning to get rid of the function completely, it could indicate a "function on its way out"? :)

I guess @zfigura have more inside info on what functions are planned removed than me, so i wont argue :)

@ghost

This comment has been minimized.

Copy link

commented Apr 5, 2018

no no no.
please read
https://en.wikipedia.org/wiki/Deprecation#Software_deprecation

I doubt they will be getting rid of csmt upstream anytime soon, they only recently set it on as the default.
you are confused by the ambiguity of that depreciated label.

either

  1. re-name the toggle box , akin to #57 (comment)
  2. or.. just remove it entirely

1 is probably preferable , since it removes ambiguity
2 is less favorable since people will get mad

omg ! the devs removed csmt from staging

@zfigura

This comment has been minimized.

Copy link
Member

commented Apr 5, 2018

No more information than I've given above. Henri has assured me that such a change to winecfg would not be accepted upstream. Modifying via the registry or winetricks is still acceptable. I am not aware of any plans to remove the option to toggle CSMT altogether.

@ghost

This comment has been minimized.

Copy link

commented Apr 5, 2018

so why is the toggle marked depreciated?

@goldpaw

This comment has been minimized.

Copy link

commented Apr 5, 2018

Reverse the switch and have it disable upstream CSMT instead of enabling it, and then change the text to something like "Disable upstream CSMT (not recommended)"?

Because as it stands now, I would believe that CSMT itself is deprecated and to be avoided. Which is totally the opposite of what we want.

@ghost

This comment has been minimized.

Copy link

commented Apr 5, 2018

I agree with @goldpaw
I would also add, that the default state ( unticked ) should null the key i.e. not set a dword

  • "csmt"=dword:00000000 ( explicitly off )
  • remove csmt key ( upstream decides )
@SveSop

This comment has been minimized.

Copy link
Author

commented Apr 6, 2018

@goldpaw @Firerat
Guys.. if you keep this up, you will end up with my initial PR, where i actually did that.. ie. the box was default unticked, and ticking it SET the csmt key to 0, clearing it deleted the reg dword... The checkbox reading "DISABLE! &CSMT for testing purposes"

@SveSop

This comment has been minimized.

Copy link
Author

commented Apr 6, 2018

@zfigura If there is no plans on removing the CSMT option from the registry, why is this function "deprecated"?
The "Staging" tab in itself, i dont think is any point on including in upstream, but if the functions tunable by the "Staging" tab is meant to be included in winecfg, it should be set on one of the other tabs. EAX under the "Sound" tab or whatever.

@ghost

This comment has been minimized.

Copy link

commented Apr 6, 2018

@SveSop since you already have a fork, do a new PR
Personally I prefer @goldpaw 's wording.

@SveSop

This comment has been minimized.

Copy link
Author

commented Apr 6, 2018

@Firerat
I suck at retro-fitting patches 0001->0005 to work, and i guess id rather not do as i did last time, and write a 0006 that "fixes" the others..
So.. Do as you wish, but changing /programs/winecfg/staging.c to this makes the tick-box work:

/*
 * Command stream multithreading
 */
static BOOL csmt_get(void)
{
    char *buf = get_reg_key(config_key, "Direct3D", "csmt", NULL);
    BOOL ret = (buf && *buf == '\0') ? TRUE : FALSE;
    HeapFree(GetProcessHeap(), 0, buf);
    return ret;
}
static void csmt_set(BOOL status)
{
    if (!status) set_reg_key(config_key, "Direct3D", "csmt", NULL);
    else
    set_reg_key_dword(config_key, "Direct3D", "csmt", 0);
}

And programs/winecfg/winecfg.rc
CONTROL "Disable upstream CSMT (not recommended)",IDC_ENABLE_CSMT,"Button",BS_AUTOCHECKBOX | WS_TABSTOP,16,40,230,8

I could not get set_reg_key_dword to "clear" the reg-key by using "NULL", as that made some weird ambiguity that it is not a "dword", so i just used if/else to clear the reg-key with set_reg_key.

@SveSop

This comment has been minimized.

Copy link
Author

commented Apr 6, 2018

Well.. i try #59

@ghost

This comment has been minimized.

Copy link

commented Apr 6, 2018

@SveSop

I was just about to post : D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.