Re: [Intel-gfx] [Nouveau] [PATCH v4] vga_switcheroo: Add helper for deferred probing

[Lukas Wunner]

On Mon, May 23, 2016 at 09:40:59AM +0100, Emil Velikov wrote:
> On 21 May 2016 at 15:08, Lukas Wunner  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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [Nouveau] [PATCH v4] vga_switcheroo: Add helper for deferred probing

[Jani Nikula]

On Sat, 21 May 2016, Lukas Wunner  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  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?
>
> 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.

Please always keep functional changes separate from non-functional
refactoring. Combining them into one just makes everyone's life
harder. This may seem like a simple case here, but you should get into
the habit and stick to it.

If you do a non-functional refactoring change, it's easy for people to
review that change. I could've reviewed your patch in a few minutes;
that's what I was in fact going to do until I stumbled on the functional
change. Maybe I don't have the time to dig all the implications in that
change right now. Bummer.

If you do a non-functional refactoring change, and claim it's
non-functional in the commit message, it'll help anyone landing on that
commit as a result of bisecting a regression. Either the bisect was
bogus, or your patch was unintentionally functional after all. Either
should be easier to verify than a combined change.

If you do the functional change separately, and that regresses, you
don't have to throw out perfectly good refactoring changes when you
revert the functional change. Or maybe the refactoring change doesn't
revert anymore, and you'll have to do a more involved fix on top. It's
easy to do reverts of clearly regressing commits, even by people who
don't fully understand everything about the commit. Anything more is
harder. Not rocket science, but it just adds up to the burden of the
already swamped maintainers.

Usually, like here, you should do the non-functional refactoring first,
and then the functional change on top. The notable exception to this is
when the functional change is a fix, especially of the kind that may
need to get backported. Then you should do the fix first, even if it's
harder and more verbose that way, because you don't want to add a stable
backport dependency on a refactoring commit (which may exceed stable
patch limits) and the stable maintainers frown upon manually backported
patches.

> Fair enough, I've changed it to match PCI_BASE_CLASS_DISPLAY
> and sent it out as v5 a few minutes ago.

Please make that a v6 and split up the patch.


Thanks,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [Nouveau] [PATCH v4] vga_switcheroo: Add helper for deferred probing

[Emil Velikov]

On 21 May 2016 at 15:08, Lukas Wunner  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  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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [Nouveau] [PATCH v4] vga_switcheroo: Add helper for deferred probing

[Lukas Wunner]

Hi Emil,

On Fri, May 20, 2016 at 12:41:04AM +0100, Emil Velikov wrote:
> On 19 May 2016 at 15:39, Lukas Wunner  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?

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.

> 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.

Thanks,

Lukas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [Nouveau] [PATCH v4] vga_switcheroo: Add helper for deferred probing

[Emil Velikov]

Hi Lukas,

On 19 May 2016 at 15:39, Lukas Wunner  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.

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 ?

Regards,
Emil
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx