Quoting Andi Shyti (2019-08-05 18:05:33)
> Hi Chris,
>
> > static void i915_switcheroo_set_state(struct pci_dev *pdev, enum vga_switcheroo_state state)
> > {
> > - struct drm_device *dev = pci_get_drvdata(pdev);
> > + struct drm_i915_private *i915 = pdev_to_i915(pdev);
> > pm_message_t pmm = { .event = PM_EVENT_SUSPEND };
> >
> > + if (!i915) {
> > + dev_err(&pdev->dev, "DRM not initialized, aborting switch.\n");
> > + return;
> > + }
> > +
> > if (state == VGA_SWITCHEROO_ON) {
> > pr_info("switched on\n");
> > - dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
> > + i915->drm.switch_power_state = DRM_SWITCH_POWER_CHANGING;
> > /* i915 resume handler doesn't set to D0 */
> > pci_set_power_state(pdev, PCI_D0);
> > - i915_resume_switcheroo(dev);
> > - dev->switch_power_state = DRM_SWITCH_POWER_ON;
> > + i915_resume_switcheroo(i915);
> > + i915->drm.switch_power_state = DRM_SWITCH_POWER_ON;
> > } else {
> > pr_info("switched off\n");
> > - dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
> > - i915_suspend_switcheroo(dev, pmm);
> > - dev->switch_power_state = DRM_SWITCH_POWER_OFF;
> > + i915->drm.switch_power_state = DRM_SWITCH_POWER_CHANGING;
> > + i915_suspend_switcheroo(i915, pmm);
> > + i915->drm.switch_power_state = DRM_SWITCH_POWER_OFF;
>
> doesn't have anything to do with this patch, but don't we care about
> the resume and suspend failures?
Go on, put a MacBook in CI, I dare you. And for a double dare, write some
igt to poke at vgaswitcheroo. It's has never been a priority for us, and
I've never even seen a vgaswitcheroo device to try it out.
> > static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
> > @@ -1841,7 +1847,8 @@ i915_driver_create(struct pci_dev *pdev, const struct pci_device_id *ent)
> >
> > i915->drm.pdev = pdev;
> > i915->drm.dev_private = i915;
> > - pci_set_drvdata(pdev, &i915->drm);
> > + BUILD_BUG_ON(offsetof(typeof(*i915), drm));
> > + pci_set_drvdata(pdev, i915);
>
> This looks a bit too fragile to me and it's not documented
> anywhere that need to have "drm" in a specific position.
Blinks. My memory says I put it at the start so that we could rely on the
equivalence between a NULL drm_device and a NULL i915_device. I find no
evidence that we cared though.
However, because I didn't want to guarantee that I had fixed up all code
that assumed anything about dev_get_drvdata, I thought documenting the
equivalence here would explain why it is used.
> At the end I wonder, why do we need "drm" to be there? Unless I
> missed it, I haven't seen anywhere any double reference to
> "i916"/"drm".
Maybe, but if you put something else at 0, you'll have to explain why
your favourite it the right choice :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
