Re: [PATCH 04/17] drm/i915: Use drm_i915_private directly from drv_get_drvdata()

[Chris Wilson <chris@chris-wilson.co.uk>]

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx