On 21 May 2016 at 15:08, Lukas Wunner lukas@wunner.de wrote:
Hi Emil,
On Fri, May 20, 2016 at 12:41:04AM +0100, Emil Velikov wrote:
On 19 May 2016 at 15:39, Lukas Wunner lukas@wunner.de wrote:
+bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev) +{
if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) {Not sure if we want/need this, yet at least. This changes behaviour which is not what refactoring patches should be doing, right ? if needed it ought to be a separate patch, imho.
Well, the commit message doesn't claim "no functional change", does it?
It does implicitly via "vga_switcheroo: Add helper for deferred probing". If you look through the kernel (and other projects) you'll notice that very few patches have these three magic words. Even when they don't have any functional changes.
Daniel Vetter explicitly wanted the ability to use the helper in vga_switcheroo audio clients, and those shouldn't run the apple-gmux test. I think it makes sense to enclose it in the above-quoted if-block *now* even though it's not needed. Once someone adds a check for an audio client, chances are they'll forget to add this if-block.
Absolutely no arguments against any of that. Having a helper makes sense even without any of the above arguments - it's a 'nasty looking' if statement, duplicated multiple times. Speaking of which ... shouldn't there be a similar hunk for amdgpu ?
Then again throwing everything into one patch does _not_ make sense. Don't know why people would insist on having re-factoring and functionality change as a single commit. Esp. in a place where hw combinations, and thus chances of things going wrong, are pretty high.
Furthermore on nouveau and AMD (iirc) front, some dual-gpu/optimus systems use PCI_CLASS_DISPLAY_3D. So if we add DISPLAY_VGA perhaps we should also check for DISPLAY_3D ?
Fair enough, I've changed it to match PCI_BASE_CLASS_DISPLAY and sent it out as v5 a few minutes ago.
Tweaking the exact class id (bitfield) is likely to end up drawn out and obnoxious. That's why I'm suggesting to keep it separate.
Regards, Emil
On Mon, May 23, 2016 at 09:40:59AM +0100, Emil Velikov wrote:
On 21 May 2016 at 15:08, Lukas Wunner lukas@wunner.de wrote:
Daniel Vetter explicitly wanted the ability to use the helper in vga_switcheroo audio clients, and those shouldn't run the apple-gmux test. I think it makes sense to enclose it in the above-quoted if-block *now* even though it's not needed. Once someone adds a check for an audio client, chances are they'll forget to add this if-block.
Absolutely no arguments against any of that. Having a helper makes sense even without any of the above arguments - it's a 'nasty looking' if statement, duplicated multiple times. Speaking of which ... shouldn't there be a similar hunk for amdgpu ?
Currently vga_switcheroo_client_probe_defer() is only needed for the MacBook Pro and the AMD GPUs used by Apple are only supported by radeon. I've amended the commit message in v6 to explain that.
Then again throwing everything into one patch does _not_ make sense. Don't know why people would insist on having re-factoring and functionality change as a single commit. Esp. in a place where hw combinations, and thus chances of things going wrong, are pretty high.
Noone insists, v1 of this patch didn't contain the functional change you're taking exception to, I added it in v2 on Daniel's request and didn't want to complicate things further, seemed like a trivial change to me. Nevertheless I've spun this out into a separate commit now, no problem at all.
Best regards,
Lukas
dri-devel@lists.freedesktop.org
-
Emil Velikov -
Lukas Wunner