Re: [PATCH v2.1 1/3] vga_switcheroo: Add support for switching only the DDC

From: Lukas Wunner <lukas@wunner.de>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Seth Forshee <seth.forshee@canonical.com>,
	dri-devel@lists.freedesktop.org, Dave Airlie <airlied@redhat.com>
Subject: Re: [PATCH v2.1 1/3] vga_switcheroo: Add support for switching only the DDC
Date: Mon, 17 Aug 2015 21:11:07 +0200	[thread overview]
Message-ID: <20150817191107.GA6718@wunner.de> (raw)
In-Reply-To: <20150817103653.GB8453@ulmo.nvidia.com>

Hi Thierry,

Thanks a lot for your comments!

On Mon, Aug 17, 2015 at 12:36:55PM +0200, Thierry Reding wrote:
> On Fri, Aug 14, 2015 at 06:50:15PM +0200, Lukas Wunner wrote:
> > +int vga_switcheroo_lock_ddc(struct pci_dev *pdev)
> > +{
> > +	int id;
> > +
> > +	mutex_lock(&vgasr_priv.ddc_lock);
> > +
> > +	if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc)
> > +		return vgasr_priv.old_ddc_owner = -ENODEV;
> 
> I find this very hard to read. Can this be split across two lines?

Ok, will rectify in v2.2.

> > +	id = vgasr_priv.handler->get_client_id(pdev);
> > +	return vgasr_priv.old_ddc_owner = vgasr_priv.handler->switch_ddc(id);
> 
> This too. I also notice that the only place you call this from doesn't
> care about the return value, so why even bother returning one?

I carried this over from Seth Forshee's and Dave Airlie's patches,
I believe the rationale is that the ->switch_ddc handler callback
might return an error and that is customarily passed up to the caller.

While drm_get_edid() does indeed ignore that return value (it will
just try to probe DDC anyway), some future function that invokes
vga_switcheroo_lock_ddc() might want to do something useful with it.

So either way has its merits and tbh I don't feel in a position to
judge what's right or wrong here. I'd be grateful if you or some
other maintainer would decide whether to make the return value
void or not and I'll be happy to change the patch accordingly.

> > +static inline int vga_switcheroo_lock_ddc(struct pci_dev *pdev) { return -ENODEV; }
> > +static inline int vga_switcheroo_unlock_ddc(struct pci_dev *pdev) { return -ENODEV; }
> 
> If you care about the return value you'll want to return 0 here to make
> sure kernels without VGA switcheroo support will continue to work
> properly.

Maybe I'm mistaken but I believe -ENODEV is correct. If there's no error
then vga_switcheroo_lock_ddc/unlock_ddc() return the old_ddc_owner which
is numbered from 0. E.g. on the MacBook Pro, 0 is IGD and 1 is DIS.
So returning 0 would mean "okay, successfully switched, was previously
switched to the integrated GPU".

Best regards,

Lukas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel