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?

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

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

The rest of the patch looks quite straight forward.

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

Reply via email to